Null checking in nested getter calls I have the following piece of code and I would like to see what different approaches would be to solve that in a more elegant way.

The thing is that I don't know whether is better to have the null checks or the try/catch block there or maybe the code below should not happen in the first place.  Maybe one can give another solution so that I can get the thumb URL from a images list that is set on a people object without having to check for null on each getter call.

if (people.getImages() != null && people.getImages().size() > 0 && people.getImages().get(0).getUrl() != null) {
    Url thumbUrl = people.getImages().get(0).getUrl().getThumb();
    // do something with the thumbUrl
}

Or I can have:

try {
    Url thumbUrl = people.getImages().get(0).getUrl().getThumb();
    // do something with the thumbUrl
} catch (Exception e) {
    // handle the null pointer exception
}

EDIT

For more information, I am interested more in knowing if there any other ways to avoid calling getter methods one after another until I get my desired data from an object and if the above code is good enough anyway.

The code works ok in both cases but I don't like to have the multiple null checks in that if statement and also catching exceptions is not a good solution either.

The getImages() method returns a list of Image objects and getThumb() returns an Url object that I need in order to fetch an image over the internet.

Let's count the number of times you're writing get.

if (people.getImages() != null && people.getImages().size() > 0 && people.getImages().get(0).getUrl() != null) {
    Url thumbUrl = people.getImages().get(0).getUrl().getThumb();
    // do something with the thumbUrl
}

5 times inside the if-condition and 4 times inside the block. That's a bit too much. Remember the Law of Demeter.

In your case I would do:

if (people.hasImage(0)) {
     Url thumbUrl = people.getImage(0);
}

The idea is to hide some logic inside the people object.

It also seems like people.getImages() is a Collection, which makes me think that it is a bad idea to ever let that be null. An empty collection is better than a null collection.

As for your main question, I believe Heslacher says it all:

An avoided exception is always better then a handled exception

This applies especially to NullPointerException and ArrayIndexOutOfBoundsException (and all other RuntimeExceptions).

Also, you should always catch the most-specific Exception possible. Never catch Exception. But then again, never catch NullPointerException. Fix your code to not cause one in the first place.

Related previous Code Review answers about exceptions:

Rethrowing exception just as an information

https://codereview.stackexchange.com/questions/45637/exception-handling-with-instanceof-rather-than-catch/45639#45639


Better or more idiomatic way to apply a sequence of functions in Haskell This is one of my Haskell solutions to a variation of the N-Queens problem, where the queens can move like knights in addition to their normal moves.  It has been optimized somewhat to avoid investigating redundant combinations.

place :: Int -> [[Int]]
place 0 = [[]]
place n = go $ take n $ repeat [1..n]
    where go [] = [[]]
          go (row:rest) = do
                q <- row
                qs <- go $ safe q rest
                return (q : qs)
          safe q = notK q . notD q . notC q
          notC q = map (filter (/= q))
          notD q = (map (\(x, r) -> filter (\y -> abs(y - q) /= x) r)) . (zip [1..])
          notK q = map (\(f, r) -> filter f r) . (zip (kFilters q))
          kFilters q = (\x -> abs (x - q) /= 2) : (\x -> abs (x - q) > 1) : (repeat (const True))

solutions = length . place

main = do
    n <- readLn
    putStrLn $ show $ solutions n

I am satisfied with the performance, but I feel there must be a more elegant way to apply a series of functions (in this case, filters) to a list.

In each iteration of the recursive function go, the top row of the board is selected and then for each position in sequence a queen is positioned and the function recurs with a filtered copy of the rest of the board, so that each iteration has only safe squares to choose from.  The safe function applies three filters to the board:

notC removes all spaces in the same column as the new queen.
notD removes any spaces on a diagonal from the new queen.
notK removes knight moves from the next two lines.

I feel that notK in particular could be implemented more cleanly and idiomatically but I couldn't see a better way to apply one function to the first item of a list, another to the second and something else to the rest.  And using zip does save me from having to check for the end of the board.

I wouldn't be surprised if there is a better way to write notD.  So I am looking for more expressive ways to apply a sequence of varying functions to successive list items.

UPDATE:

I realise that I can use uncurry to clean up notK...

        notK q = map (uncurry filter) . (zip (kFilters q))

and the two filters in Kfilters can be written in dot notation...

        ((/= 2) . abs . subtract q) : ((/= 1) . abs . subtract q) : ...

which allows the kFilters line to be rendered as

        kFilters = (f 2) : (f 1) : (repeat (const True))

but this doesn't actually change my original question.  I'm still looking for a better mechanism for applying a varying sequence of functions to a list.

notK q = map (uncurry filter) . (zip (kFilters q))

is equivalent to

notK q rows = zipWith filter (kFilters q) rows

Onto making above Haskell less idiomatic and less pointfree and more readable.

notC q = map (filter (/= q))
notD q = (map (\(x, r) -> filter (\y -> abs(y - q) /= x) r)) . (zip [1..])
notK q = map (\(f, r) -> filter f r) . (zip (kFilters q))

These three are logically similar but structurally dissimilar.

First let's factor out common components, give them meaningful names, and some comments to help those readers without the Developer's Manual.

-- a new queen placed in column q
-- would take a piece placed in column x
-- n rows down
-- because (r, q) and (r+n, x)

-- are on the same column
sameColumn q n x = x == q

-- are on the same diagonal
sameDiagonal q n x = abs(x - q) == n

-- or (n, |q - x|) is in [(1,2),(2,1)]
onKnightMove :: Int -> Int -> Int -> Bool
onKnightMove q n x = (n, abs(x - q)) `elem` [(1,2),(2,1)]

The last one needs a better name still, oh well. At least they are of the same form, with parameter name use consistent among them (and elsewhere).

rowFilterList :: Int -> (Int -> Int -> Int -> Bool) -> [Int -> Bool]
rowFilterList q pred = map (\n x -> not $ pred q n x) [1..]

such that kFilters q = rowFilterList q onKnightMove

filterRows :: Int -> (Int -> Int -> Int -> Bool) -> [[Int]] -> [[Int]]
filterRows q pred rows = zipWith filter (rowFilterList q pred) rows

This way notC q = filterRows q sameColumn and notD q = filterRows q sameDiagonal

notK q . notD q . notC q a repetition is evident. And we now can factor it out easily.

filterWithAll :: Int -> [[Int]] -> [Int -> Int -> Int -> Bool] -> [[Int]]
filterWithAll q rows preds = foldl (\rows' pred -> filterRows q pred rows') rows preds

such that safe q rows = filterWithAll q rows [sameColumn, sameDiagonal, onKnightMove]

By factoring out [sameColumn, sameDiagonal, onKnightMove] as a parameter to place n one could easily generalize place from n super-queens to n queens or n rooks.


Variadic template with function argument I am trying to develop an ECS registry. How would you improve this ? First disadvantage comes to my mind is using lambdas might be expensive, however I was not able to replicate functionality using F&& template arguments.

    engine->addEntity(1,
        Position{1, 1},
        Mesh{"thing.gltf"},
        PhysicsBody{10.0f}
    );

    engine->addEntity(2,
        Position{ 2, 2 },
        Mesh{ "other.gltf" },
        PhysicsBody{ 30.0f }
    );

    engine->entity<Position>(1, [&](auto pos) {
        std::cout << "entity";
    });
    engine->each<Position, Mesh>([] (int id, Position& pos, Mesh& mesh) {
        std::cout << "entity" << id;
    });

    template<typename First>
    void entity(int id, std::function<void(First&)> callback)
    {
        auto comp = storage<First>()->get(id);
        if (!comp) return;
        callback(*comp);
    }

    template<typename First, typename...Rest>
    void entity(int id, std::function<void(Rest&...)> callback)
    {
        auto comp = storage<First>()->get(id);
        if (!comp) return;
        entity<Rest...>(id, [](Rest&...args) {
            callback(*comp, args...)
        });
    }

    template<typename T, typename...Rest, typename F>
    void each(F&& callback)
    {
        storage<T>()->forEach([&](int id, T& value) {
            entity<Rest...>(id, [&](Rest&...args) {
                callback(id, value, args...);
            });
        });
    }

using lambdas might be expensive

Why do you think that?

Lambdas are simply syntactic hand waving for class functors (that have been around for over 3 decades). Functors are considered (in general) to be much more efficient than standard functions (especially when used by templates) as the compiler can do a lot of optimizations on them.

auto x = [&capture](X const& parm){ /* BODY */ }

You can consider as syntactic sugar for:

class CompilerNamedType
{
    TypeOfCapure&     capture;
    public:
        CompilerNamedType(TypeOfCapure& capture)
            : capture(capture)
        {}

        CompilerCalculatedReturnType operator()(X const& param) const
        {
            /* BODY */
        }
};

CompilerNamedType  x(capture);


How should I name a function which shows or hides an element? I have a function to display or hide some elements. What I want to do is to manage the display of those elements regarding a value.

How should I replace the term toggle? Toggling for me means 'show the element if it's hidden, or hide it if it shown.' (yes, I mean toggling visibility)

function togglePassengers() {
    var nbPassengers = $("#nbPassengersForTravel").val(),
        $passengers = $('#d_passenger').find('fieldset');

    $passengers.each( togglePassenger );

    function togglePassenger(i){
        $passenger = $(this);
        if( ++i <= nbPassengers )
            showPassenger($passenger);
        else
            hidePassenger($passenger);
    }

    function showPassenger($passenger){
        $passenger
            .removeClass("hidden")
            .find("select.age option[value='-1']").remove();
    }

    function hidePassenger($passenger){
        $passenger.addClass("hidden");
        var $passengerAge = $passenger.find("select.age");
        if ( ! $passengerAge.find("option[value='-1']:first-child").length )
            $passengerAge.prepend("<option value='-1'>---------</option>").val('-1');
    }
}

Naming

I think togglePassenger is not the only naming problem in your code.

A function parameter should not be a single letter except in a very limited number of cases. But I don't think that this is not one of them; a reader does not automatically understand what i actually does.

I would also rename nbPassengers, because it is unclear what nb means.

What I think your function does is show the first n passengers, and hide the rest. If that is the case, I would not use toggle, because as you said, there really is no toggling going on. You could go with something like updateVisibility to avoid this confusion, although it's a bit unconcrete. Whatever name you do choose, I would add a comment to the function explaining what it does.

Misc

I would use curly brackets even for one line statements to avoid future bugs
your spacing is sometimes inconsistent (eg spacing around variable in function calls)


Count occurrences of a specific sequence in a list of many numbers The script will work through each number from 0 - 10.
aList=[3,5,... over 10k numbers ...,2,2,6]

Let’s say, for example: findSquence01=[3,5] or findSquence02=[3,5,3]
count = 0
for i in range(len(data)-len(pattern)+1):
    tmp = [data[i], data [i+1]]

    try:
        for j in range(len(data)-i):
            print(i, i+j)
            if tmp[-1] != data[i+j+1]:
                tmp.append(data[i+j+1])

            if len(tmp) == len(pattern):
                print(tmp)
                break
    except:
        pass

    if tmp == pattern:
        count +=1

i- represents times of [3,5]
j- represents times of [3,5,3]

print("There is",i,"times",findSquence01)
print("There is",j,"times",findSquence02)

The output should look like:

There is 73 times [3,5]
There is 12 times [3,5,3]

How can I walk through the list and count those given sequences?

Try to keep your solution as general as possible. If I'm reading your code snippet right, you're trying to solve the problem for two patterns simultaneously. Instead, write a simple function that takes a list and a pattern, and just loops through the list counting occurrences of the pattern. This is a nice use case for list slices: given a list a, a[i:j] is a list consisting of elements i through j-1 of a.

def count_sequence(lst, seq):
     count = 0
     len_seq = len(seq)
     upper_bound = len(lst)-len_seq+1
     for i in range(upper_bound):
         if lst[i:i+len_seq] == seq:
             count += 1
     return count

Try to avoid exceptions if possible*. A rule of thumb is that exceptions are for exceptional situations. In your case, you're designing your code around the fact that you're going to get IndexErrors, whereas the function I wrote keeps the array bounds in mind and thus won't raise IndexErrors. Additionally, never write except: pass. The point of the except block is to catch errors and do something with them, so don't pass over them.

* Python itself doens't always follow this advice; for example, loops in Python are ended by raising the StopIteration exception. Nevertheless, for user-level code it's usually a good idea to only use exceptions if absolutely necessary.


IEEE 754 square root with Newton-Raphson I have an implementation of the sqrt function that uses a combination of IEEE 754, packed bitfields, and the Newton-Raphson algorithm:

decompose.h:

#ifndef DECOMPOSE_H
#define DECOMPOSE_H 1

#ifdef __cplusplus
extern "C" {
#endif

#include <float.h>

#define MANTISSA_SIZE 52
#define EXPONENT_SIZE 11
#define EXPONENT_BIAS 0x3ff
#define TYPE double
#define TYPE_MIN DBL_MIN
#define TYPE_MAX DBL_MAX
#define NAME_SFX(name) name

typedef unsigned long long mantissa;
typedef unsigned short exponent;
typedef unsigned char sign;

#include "decompose_generic.h"

#ifdef __cplusplus
}
#endif

#endif

decompose_generic.h:

#ifndef DECOMPOSE_GENERIC_H
#define DECOMPOSE_GENERIC_H 1

#ifdef __cplusplus
extern "C" {
#endif

#define SIGN_SIZE 1

union decompose {
    TYPE f;
    struct decompose_s {
        mantissa m:MANTISSA_SIZE;
        exponent e:EXPONENT_SIZE;
        sign s:SIGN_SIZE;
    } __attribute__((packed)) o;
};

static inline union decompose decompose(TYPE f)
{
    union decompose result;
    result.f = f;
    return result;
}

#define is_nan(x) NAME_SFX(isnan)(x)

#ifdef __cplusplus
}
#endif

#endif

sqrt.c:

#include <math.h>
#include <errno.h>

#include "decompose.h"

TYPE NAME_SFX(sqrt)(TYPE a)
{
    TYPE result_old2 = 0;
    unsigned short c = 0;
    union decompose r = decompose(a);

    if(a < -(TYPE)0)
    {
        errno = EDOM;
        return NAME_SFX(nan)("");
    }
    if(a == (TYPE)0 || is_nan(a) || a > TYPE_MAX)
    {
        return a;
    }

    /* Divide the exponent by 2 */
    r.o.e -= EXPONENT_BIAS;
    r.o.e = (r.o.e & 1) | (r.o.e >> 1);
    r.o.e += EXPONENT_BIAS;

    /* Newton-Raphson */
    while(result_old2 != r.f)
    {
        if(c % 2 == 0) result_old2 = r.f;
        r.f -= (r.f*r.f-a)/(2*r.f);
        c++;
    }

    return r.f;
}

What I'm looking for:

Are there any efficiency problems?
Are there any accuracy problems?
Are there any other miscellaneous things that I can improve?

As far as I've tested, this is as accurate as possible. It even matches the results from the GNU C library.

One note: This is part of a modular system. There will be a decompose.h for each floating point type (float, double, long double), and they all include decompose_generic.h.

I'm slightly new to this area of C (math, IEEE 754), but not so new that I'd add the beginner tag.

This answer uses pointer-casting for type-punning just to save space.  In practice keep using your union (safe in ISO C99, and in C++ as a GNU and MSVC extension) or memcpy (safe in C and C++).  This pointer-casting is only safe in MSVC, or GNU-compatible compilers with -fno-strict-aliasing

Initial approximation

Packed bit fields are not only unnecessary here, they make the result worse. You do something along the lines of:

uint64_t i = *(uint64_t*)&f;           // double to int
i = i - (1023 << 52);                  // remove IEEE754 bias
i = i >> 1;                            // divide exponent by 2
i = i + (1023 << 52);                  // add IEEE754 bias

If you change the order of operations, you can handle the biasing in a single constant:

uint64_t i = *(uint64_t*)&f;           // double to int
i = i >> 1;                            // divide exponent by 2
i = i + (1023 << 52) - (1023 << 51);   // remove/add IEEE754 bias

The constant can then be simplified to:

uint64_t i = *(uint64_t*)&f;           // double to int
i = (i >> 1) + (1023 << 51);           // all of the above

Note that I didn't bother to mask when shifting, so the exponent's rightmost bit drops into the mantissa, which is shifted as well. This is a feature, not a bug. The IEEE754 formats are deliberately chosen to be monotonic when interpreted as integers, so carry-overs between exponent and mantissa are explicitly allowed.

With my approximation, the mapping is as follows (linear mapping within intervals):

original   number is    exponent   manitssa   orig interval
exponent  in interval   in sqrt   first bit     maps to
   0     [  1.0,  2.0 )    0         0       [  1.0,  1.5 )
   1     [  2.0,  4.0 )    0         1       [  1.5,  2.0 )
   2     [  4.0,  8.0 )    1         0       [  2.0,  3.0 )
   3     [  8.0, 16.0 )    1         1       [  3.0,  4.0 )
   4     [ 16.0, 32.0 )    2         0       [  4.0,  6.0 )

With your code, the mapping is (also linear within intervals):

original   number is    exponent  orig interval
exponent  in interval   in sqrt     maps to
   0     [  1.0,  2.0 )    0     [  1.0,  2.0 )
   1     [  2.0,  4.0 )    0     [  1.0,  2.0 )
   2     [  4.0,  8.0 )    1     [  2.0,  4.0 )
   3     [  8.0, 16.0 )    1     [  2.0,  4.0 )
   4     [ 16.0, 32.0 )    2     [  4.0,  8.0 )

Newton-Raphson

Given a good approximation, Newton-Raphson doubles the number of significant digits on each iteration (quadratic convergence). The above approximation provides about 4 bits of accuracy (max error: 6% or ~1/16), so 3 Newton-Raphson iterations are required for single and 4 iterations for double precision.

The entire code could look like this:

float sqrtf(float x)
{
    float y = x;
    // Approximation
    uint32_t* i = (uint32_t*)&x;
    *i = (*i >> 1) + (127 << 22);
    // Newton-Raphson
    x = (x + y/x) / 2;
    x = (x + y/x) / 2;
    x = (x + y/x) / 2;
    return x;
}

double sqrt(double x)
{
    double y = x;
    // Approximation
    uint64_t* i = (uint64_t*)&x;
    *i = (*i >> 1) + (1023 << 51);
    // Newton-Raphson
    x = (x + y/x) / 2;
    x = (x + y/x) / 2;
    x = (x + y/x) / 2;
    x = (x + y/x) / 2;
    return x;
}

Subnormals and other hazards

You already have special handling for nan, inf, 0 and x < 0, which is good, but we need to handle subnormal numbers as well. The code, both yours and mine, fails when confronted with such numbers. As a remedy, I suggest you scale subnormal numbers by 2^200 and scale the result by 2^-100. I chose 2^200 because it safely brings all subnormals back into normal range for all precisions, including quad, but other numbers work equally well (but only powers of two retain full precision).

Also observe that my code relies on IEEE754 properties. It doesn't work for non-IEEE754 numbers, most notably x86's 80bit extended double format. If you want to handle such numbers, you can convert them to float or double for the approximation step and back to extended double for Newton-Raphson.


Find the smallest number of square numbers to create n An interview question I got -

Given int n, find the smallest number of square numbers that fit inside n.

Example:

Input: 24
Output: 3 (16 + 4 + 4)

Input: 10
Output: 2 (9 + 1)

Solution:

public class Solution{
    public static int solution(int number){
        int squareCount = 0;
        while(number> 0){
            int square = (int)Math.sqrt(number);
            squareCount++;
            number-=square*square;
        }
        return squareCount;
    }
}

Notes:

Must be Java 7
Must be in class called Solution, with a public static int solution(int number) method
Must not use any 3rd party libraries

I like this solution because it is very simple, however it does have one inefficiency. It finds the square root only to perform a squaring operation again. It would be nice if I could find the largest square number in squared form immediately, but I cant think of an efficient way to do this.

Your solution seems to be the most efficient, except for the fact that it doesn't seem to work. You could try finding all possible groups like so:

$$18=4^2+1^2+1^2$$

$$18=3^2+3^2$$

$$18=3^2+2^2+2^2+1^2$$

$$...$$

Then find the smallest group.

Your code would sure like some space. After some nice, wide spacing:

public static int solution(int number) {
    int squareCount = 0;
    while(number > 0){
        int square = (int) Math.sqrt(number);
        squareCount++;
        number -= square * square;
    }
    return squareCount;
}

I will post a possible solution after I figure one out.


Simple Chaining Implementation of HashMap I'm practicing writing my own simple version of a Java HashMap. I don't have much experience with generics in Java.

public class HashMap<K, V> {
    private class Entry<K, V> {
      private K key;
      private V value;
      private Entry next;

      Entry(K key, V value) {
            this.key = key;
            this.value = value;
      }

      public K getKey() {
            return key;
      }

      public V getValue() {
            return value;
      }
    }

    private final static int TABLE_SIZE = 32;
    private Entry[] table;

    public HashMap() {
          table = new Entry[TABLE_SIZE];
    }

    private int hash(K key) {
        return Math.abs(key.hashCode() % TABLE_SIZE);
    }

    public V get(K key) {
        int index = hash(key);
        Entry curr = table[index];
        while (curr != null) {
            if (curr.getKey().equals(key)) {
                return (V) curr.getValue();
            }
            curr = curr.next;
        }
        return null;
    }

    public V remove(K key) {
        int hash = hash(key);
        Entry curr = table[hash];
        if (curr == null) {
            return null;
        }
        if (curr.getKey().equals(key)) {
            V temp = (V) curr.getValue();
            table[hash] = curr.next;
            return temp;
        }
        while (curr.next != null) {
            if (curr.next.getKey().equals(key)) {
                V temp = (V) curr.next.getValue();
                curr.next = curr.next.next;
                return temp;
            }
        }
        return null;
    }

    public void put(K key, V value) {
        int hash = hash(key);
        Entry curr = table[hash];
        if (curr == null) {
            table[hash(key)] = new Entry(key, value);
        } else {
            while (curr.next != null) {
                curr = curr.next;
            }
            curr.next = new Entry(key, value);
        }
    }
}

There is a bug in 'remove' method. A while loop never changes 'curr' object so in case when key is not equals then loop will never exit


Folder browser dialog to remember recent folder. Which method is better? which of the following code you prefer and why?

The scenario is, I need to pass the recent folder path  to folder browser dialog and if user selects any folder, need to save it back. The value is stored in Application settings.

My settings class.

    internal sealed class Settings : ApplicationSettingsBase
    {
        private static readonly Settings DefaultInstance = (Settings)Synchronized(new Settings());

        public static Settings Default
        {
            get { return DefaultInstance; }
        }

        [UserScopedSetting]
        public string RecentFolder
        {
            get { return (string) this["RecentFolder"]; }
            set
            {
                this["RecentFolder"] = value;
                Save();
            }
        }
}

Method 1.

var dialog = new FolderBrowserDialog
            {
                SelectedPath = Settings.Default.RecentFolder
            };

            DialogResult result = dialog.ShowDialog();

            if (result == DialogResult.OK && dialog.SelectedPath.Length > 0)
            {
                Settings.Default.RecentFolder = dialog.SelectedPath;
               //action goes here
            }

Method 2: using extension methods.

    public static class FolderBrowserDialogExtensions
    {
//The extension method is in separate project where we don't have reference to the settings class.

        public static DialogResult ShowDialog(this FolderBrowserDialog dialog, ApplicationSettingsBase settings, string key)
        {
            dialog.SelectedPath = settings[key].ToString();

            DialogResult result = dialog.ShowDialog();

            if (result == DialogResult.OK || result == DialogResult.Yes)
            {
                settings[key] = dialog.SelectedPath;
                settings.Save();
            }
            return result;
        }
    }

            var dialog = new FolderBrowserDialog();

            DialogResult result = dialog.ShowDialog(Settings.Default, "RecentFolder");

            if (result == DialogResult.OK && dialog.SelectedPath.Length > 0)
            {
               //action goes here
            }

Extensions are good when you want to re-use this logic in multiple places. As it seems to be the case, i would go with the second method. Generally it is a good practice to avoid code duplication where possible.

The only possible issue i see is that those two methods have different behaviours: first method saves settings after checking the length, while the second method does this before checking. I'm not sure if that is important (can you even get an empty string from this dialog if you press "ok"?).

EDIT:

You can also overload this static method to use lambdas. For example:

public static class FolderBrowserDialogExtensions
{
    public static DialogResult ShowDialog(this FolderBrowserDialog dialog, ApplicationSettingsBase settings, string key)
    {
        return dialog.ShowDialog(settings[key].ToString(),
                                 path => {
                                             settings[key] = dialog.SelectedPath;
                                             settings.Save();
                                         });
    }

    public static DialogResult ShowDialog(this FolderBrowserDialog dialog, string defaultValue, Action<string> onAccept)
    {
        dialog.SelectedPath = defaultValue;
        var result = dialog.ShowDialog();
        if (result == DialogResult.OK)
        {
            // check for null, if needed
            onAccept(dialog.SelectedPath);
        }
        return result;
    }
}

Then you can stick to your Settings class:

dialog.ShowDialog(Settings.RecentFolder, path => Settings.RecentFolder = path);


Fixed timestep & rendering game loop So I wrote this fixed timestep & rendering game loop.
I am posting it here to see if there are ways to optimize it/make it more accurate.

    const float updatesRate = 1 / 60.0f;
    const float framesRate = 1 / 308.0f;

    double currentTime = hireTimeInSeconds( );
    double accumulator = 0.0;
    double accumulator2 = 0.0;
    uint32_t frames = 0, updates = 0;
    double timer = hireTimeInSeconds( );

    while ( true ) {

        double newTime = hireTimeInSeconds( );

        if ( newTime - timer >= 1 ) {

            //Printing fps and ups here

            updates = 0;
            frames = 0;
            timer = newTime;
        }

        double frameTime = newTime - currentTime;
        if ( frameTime > 0.25 ) frameTime = 0.25;
        currentTime = newTime;
        accumulator += frameTime;
        accumulator2 += frameTime;

        //Polling input here

        while ( accumulator >= updatesRate ) {

            //Updating here
            accumulator -= updatesRate;
            updates++;
        }

        const double alpha = accumulator / updatesRate;

        while ( accumulator2 >= framesRate ) {
            //Rendering
            frames++;
            accumulator2 -= framesRate;
        }
    }

```

const float updatesRate = 1 / 60.0f;
const float framesRate = 1 / 308.0f;

The other time values are all doubles, it's a bit strange to use floats here.

    if ( newTime - timer >= 1 ) {
    ...
    if ( frameTime > 0.25 ) frameTime = 0.25;

These magic numbers should be named constants.

C++ now provides a variety of time-related utilities through the <chrono> header. We should use the clock types, type-safe duration types, and conversions that it provides.

double accumulator = 0.0;
double accumulator2 = 0.0;
uint32_t frames = 0, updates = 0;
double timer = hireTimeInSeconds( );

The naming of these variables leaves their purpose very unclear. Perhaps we could define an Accumulator class to group the variables for each accumulator and remove the duplication (see example code below).

hireTimeInSeconds This should probably be highResTimeInSeconds.

    while ( accumulator2 >= framesRate ) {
        //Rendering
        frames++;
        accumulator2 -= framesRate;
    }

I don't think this makes sense. We only want to render one frame, even if lots of time has passed. (We're not doing any updates between rendering frames in this loop, so we'd just be rendering the exact same thing over and over).

Applying the above suggestions, we might get something like the code below. It is quite a few more lines of code, but is also much more reusable (especially the accumulator and frame_timer classes), and is hopefully clearer in intent:
#include <chrono>
#include <iostream>

template<class c_t>
class accumulator
{
public:

    using clock_t = c_t;
    using duration_t = typename clock_t::duration;

    template<class d_t>
    explicit accumulator(d_t tick_length):
        m_tick_length(std::chrono::duration_cast<duration_t>(tick_length)) { }

    std::size_t accumulate(duration_t delta_time) // note: returns the number of ticks of m_tick_length triggered by this call
    {
        m_accumulated_time += delta_time;

        auto ticks = std::size_t{ 0 };

        while (m_accumulated_time >= m_tick_length)
        {
            ++ticks;
            m_accumulated_time -= m_tick_length;
        }

        return ticks;
    }

    duration_t get_tick_length() const
    {
        return m_tick_length;
    }

private:

    duration_t m_tick_length;
    duration_t m_accumulated_time;
};

template<class c_t>
class frame_timer
{
public:

    using clock_t = c_t;
    using duration_t = typename clock_t::duration;
    using time_point_t = typename clock_t::time_point;

    template<class d_t>
    explicit frame_timer(d_t initial_last_frame_time):
        m_last_frame_time(std::chrono::duration_cast<duration_t>(initial_last_frame_time)),
        m_last_tick(clock_t::now()) { }

    void tick()
    {
        auto now = clock_t::now();
        m_last_frame_time = now - m_last_tick;
        m_last_tick = now;
    }

    duration_t get_last_frame_time() const
    {
        return m_last_frame_time;
    }

private:

    duration_t m_last_frame_time;
    time_point_t m_last_tick;
};

int main()
{
    using clock_t = std::chrono::high_resolution_clock;
    using duration_t = clock_t::duration;
    using time_point_t = clock_t::time_point;
    using seconds_t = std::chrono::duration<float>;

    auto const update_time = seconds_t(1.f / 60.f);
    auto update_accumulator = accumulator<clock_t>(update_time);

    auto const render_time = seconds_t(1.f / 308.f);
    auto render_accumulator = accumulator<clock_t>(render_time);

    auto const print_time = seconds_t(1.f);
    auto print_accumulator = accumulator<clock_t>(print_time);

    auto const initial_frame_time = update_time;
    auto timer = frame_timer<clock_t>(initial_frame_time);

    auto const max_frame_time = std::chrono::duration_cast<duration_t>(seconds_t(0.25f));

    while (true)
    {
        auto const last_frame_time = (timer.get_last_frame_time() > max_frame_time) ? max_frame_time : timer.get_last_frame_time();

        if (print_accumulator.accumulate(last_frame_time) != 0) // note: will skip update periods if frames are super long (> print_time)
        {
            // ... print frames
            std::cout << "ping!" << std::endl;
        }

        auto const update_ticks = update_accumulator.accumulate(last_frame_time); // note: don't put this directly in the loop condition, we only want to call it once!

        for (auto i = std::size_t{ 0 }; i != update_ticks; ++i)
        {
            // ... do update
            std::cout << "update" << std::endl;
        }

        if (render_accumulator.accumulate(last_frame_time) != 0) // note: only ever render 1 frame, even if we should have rendered more
        {
            // ... render
            std::cout << "render" << std::endl;
        }

        timer.tick();
    }
}


Handling Hover Events on a Touch Screen A website I have designed uses a nav menu that shows submenus on :hover. The initial site did not use any responsive design: it targeted only the desktop environment. I am now using responsive-design techniques to target mobile devices and tablets, many of which are touch based rather than mouse based.

The big issue I was facing (and that many other people seem to have also faced) was the hover-based nav menu: it works great in a mouse environment, but on touch devices, there is no reliable way to trigger the hover, making the page difficult to use.

The goal is this:

When a menu is hovered by a mouse, show the sub-menu.
When a menu is clicked with a mouse, open the link of the anchor tag.
The first time a menu is clicked by touch, show the sub menu.
The second time a menu is clicked by touch, open the link of the anchor tag.
Switch functionality seamlessly for tablets that also have mouse devices attached.

My team is not willing to sacrifice the hover effect on mouse-based machines. They like it and do not want to make the menus click based on all devices. I agree with this.

After looking around the net, I couldn't find any single solution for my problem. I put a few of them together and developed something that has tested well on Android, getting exactly the functionality I wanted.  Is there anything that can be improved and/or do you see any problems with this approach?

jQuery(document).ready(function() {
    var touched=false;
    jQuery(".nav").on('touchstart', 'li .has_children', function (e) {  touched=true; });
    jQuery("html").on('mousemove', function (e) { touched=false; });

    jQuery("html").on('click', updatePreviousTouched );

    jQuery(".nav").on('click', 'li .has_children', function (e) {
        updatePreviousTouched(e);
        if( touched ) {
            if (jQuery(this).data('clicked_once')) {
                jQuery(this).data('clicked_once', false);
                return true;
            } else {
                e.preventDefault();
                jQuery(this).trigger("mouseenter");
                jQuery(this).data('clicked_once', true);

            }
        }
        touched=false;
    });

    var previous_touched;
    function updatePreviousTouched(e) {
        if( typeof previous_touched != 'undefined' && previous_touched != null && !previous_touched.is( jQuery(e.target) ) ) {
            previous_touched.data('clicked_once', false);
        }
        previous_touched=jQuery(e.target);
    }
}

If this solution can be improved, please provide any suggestions. If you find that it doesn't work for you, please let me know that too. I'm posting it here to try to get any suggestions for improvement and to publish the idea so others can benefit from it.

Here are the results of my tests so far:

Windows 7 on major 5 browsers: works.
Android tablet on default browser: works.
Windows 7 laptop with touch-screen on IE and Chrome: Sort-of-works. The machine is running the desktop version of both browsers, so it is not triggering the "touchstart" event. Somewhere between the touchscreen and the browser, something is converting the touch into mouse events.  However, since the mouse is a non-removable part of this device, I don't consider this a failure. I would like it if it worked like a tablet, but there doesn't seem to be the technology available yet.
iOS iPad: This worked as desired before I wrote the above javascript.  It appears that ios does some of this hover/click magic for us behind the scenes.  I have a team member checking to see if my javascript broke the iOS functionality, I will edit this post once I get an answer.

Other notes:

This is only the menu for the desktop and tablet site. There is a different, click-based menu for the mobile site. I wouldn't recommend using this solution for a phone-sized device.

First of all, lets clean up your code to really leverage the power of .on():

$(function() {
    var touched = false,
        previous_touched;

    function updatePreviousTouched(e){
        if(typeof previous_touched !== 'undefined' && previous_touched !== null && !previous_touched.is($(e.target))){
            previous_touched.data('clicked_once', false);
        }

        previous_touched = $(e.target);
    }

    $(".nav").on({
        touchstart:function(e) {
            touched=true;
        },
        click:function(e);
            var $this = $(this);

            updatePreviousTouched(e);

            if(touched) {
                if ($this.data('clicked_once')) {
                    $this.data('clicked_once', false);
                    return true;
                } else {
                    e.preventDefault();
                    $this.trigger("mouseenter").data('clicked_once', true);
                }
            }

            touched = false;
        }
    },'li .has_children');

    $("html").on({
        mousemove:function(e){
            touched=false;
        },
        click:updatePreviousTouched
    });
});

Notice the following:

Object use of .on() rather than mere string use (only one binding instead of multiple)
caching where appropriate
variables and functions declared at top

Now that we've got your original code all pretty and efficient, I'll offer a different alternative that should be much more lightweight.

Based on your currently use CSS to do the :hover piece. Instead, you may consider making that CSS a class, and then dynamically add / remove the class in the code. This maintains the speedy leveraging of CSS while avoiding dealing with the CSS and JS events fighting each other over what wins out. I have done something like this on a number of projects and it seems to work pretty well. The key is the custom event; by separating this out, you can control when it is fired rather than rely on some crazy if logic to determine whether it should be fired or not.

Here is a working example:

This CSS that was this...

.header-nav-menu ul li:hover > ul { display: inline-block; }

Becomes this...

.header-nav-menu ul li > ul.MenuActive { display: inline-block; }

And here is your working jQuery...

$(function(){
    var menuActive = false,
        touched = false,
        $nav = $('.nav');

    function removeActive(callback){
        $nav.find('.MenuActive').removeClass('MenuActive');
        callback();
    }

    function newActive($this,menu){
        removeActive(function(){
            $this.next().addClass('MenuActive').queue(function(){
                if(menu){
                    menuActive = true;
                    touched = false;
                } else {
                    touched = true;
                }
            }).dequeue();
        });
    }

    $nav.on({
        touchstart:function(e){
            e.stopPropagation();
            newActive($(this),touched);
        },
        mouseenter:function(){
            newActive($(this),true);
        },
        click:function(e){
            e.preventDefault();

            if(menuActive){
                $(this).trigger('trueClick',e.target);
            }
        },
        trueClick:function(e,$target){
            $(this).parents('.nav').trigger('mouseleave');
            window.location.href = $target;
        }
    },'li .has_children').on('mouseleave',function(){
        removeActive(function(){
            menuActive = false;
            touched = false;
        });
    });

    $('html').on('touchstart',function(e){
        if(menuActive){
            $nav.trigger('mouseleave');
        }
    });
});

This should manage what you want in a much cleaner way:

menuActive variable defaults to false, and is only set to true when submenu is open (by either touchstart or mouseenter)
actual click action is prevented, and verification is done to determine if menu is appropriately active
if menu is active, custom event is triggered to go to the link's target
touch event is not bubble up to html by use of e.stopPropagation();
if user touches somewhere on the screen that is not the submenu, it will close the submenu and set menuActive to false

I separated out a couple of actions into reusable functions, and separated events to avoid if checking. This code executes much faster than the original, and more importantly it is bulletproof. There is true separation of touch vs hover events, the show / hide of the menu still leverages CSS, and it accounts for browser inconsistencies (example:  the reason for the use of the callback is because Safari 5.1 has a 300ms delay between adding a class and it displaying onscreen).

I know this code is longer than the original, but guaranteed it will run faster and be easier to maintain. Hope it helps.


Weaving elements into a list I have the following problem statement:

  Modify a given list such that one specific element is "weaved" into that list.

  Examples if the element is 'module':

  [] remains []
  ['a'] remains ['a']
  ['a', 'b'] becomes ['a', 'module', 'b']
  ['a', 'b', 'c'] becomes ['a', 'module', 'b', 'module', 'c']

I solved it using the following code in Python 3.4:

@staticmethod
def __weave_element_into_list(element, values: list) -> list:
    """
    Weaves an element into a given list.

    Examples if the element is 'module':
     - Input [] remains [].
     - Input ['a'] remains ['a'].
     - Input ['a', 'b'] becomes ['a', 'module', 'b'].
     - Input ['a', 'b', 'c'] becomes ['a', 'module', 'b', 'module', 'c'].
     - etc.

    :param element: The element to weave into the list
    :param values:  The original list
    :return:    The list with element weaved into it.
    """
    copy_list = list(values)
    if len(copy_list) <= 1:
        return copy_list
    i = 1
    while i < len(copy_list):
        copy_list.insert(i, element)
        i += 2
    return copy_list

Is there a better (more Pythonic?) solution to it and is "weaving" the correct operation here?

The function could be replaced with this line:

return list(chain.from_iterable(zip(values, [element] * len(values))))[:-1]

Essentially,
zipping values with another list of the same size containing element in all values, flattening the zip, and chopping off the end.

About your original implementation, instead of this:

if len(copy_list) <= 1:
    return copy_list

This is more Pythonic:

if not copy_list:
    return copy_list

I'm also wondering if this needs to be a @staticmethod.
Since it's a pure function, I'd just call it weave.
Here's the complete implementation with doctests:

from itertools import chain

def weave(element, values: list) -> list:
    """
    >>> weave('module', [])
    []
    >>> weave('module', ['a'])
    ['a']
    >>> weave('module', ['a', 'b'])
    ['a', 'module', 'b']
    >>> weave('module', ['a', 'b', 'c'])
    ['a', 'module', 'b', 'module', 'c']

    :param element: The element to weave into the list
    :param values: The original list
    :return: The list with element weaved into it.
    """
    return list(chain.from_iterable(zip(values, [element] * len(values))))[:-1]

I see @jonrsharpe beat me to the doctests.
To add something new,
another way to run doctests:

python -m doctest yourfile.py

Update

Originally I proposed this one-liner:

return sum(zip(values, [element] * len(values)), ())[:-1]

But that turns out to be a bad idea.
This other discussion is also illuminating.


Matching Needle & Haystack values using alternate Ontology values I need to match values in Needle & Haystack arrays, using alternate Ontology values when relevant.

This syntax works, however I would like to know if there is a more efficient way to do this as I will be using it for several corresponding $AltNeedle arrays for each of 1000+ $needles and $haystack arrays.

In my use case, the Needle & Haystack correspond with Product and Customer attributes respectively, and matches are made and ranked by assessing thousands of attributes for each, and attributes often have an array of values. One way in which Alt. Values are used are to incorporate closely related values into certain match results.

<?php
$needles = ['John', 'Alex', 'Pam'];
$haystack = ['John','Alexander','Kim', 'Michael'];
$AltNeedle = ['Alexander', 'Alex', 'Alexi'];

foreach ($needles as $current_needle) {
    if (in_array($current_needle, $AltNeedle) ) { // Exists in $AltNeedle
        foreach ($AltNeedle as $current_AltNeedle) {
                if (in_array($current_AltNeedle, $haystack) ) { // Compares $current_AltNeedle to $haystack
                    echo $current_AltNeedle." True; ";
                }
        } // Ends $AltNeedle foreach
    }
    elseif (in_array($current_needle, $haystack) ) {
        echo $current_needle.": True; ";
    }
    else {
        echo $current_needle.": False; ";
    }
} // End foreach
?>

RESULT:

  John: True; Alexander True; Pam: False;

The whole thing needs restructuring if you are worried about performance.  First, you probably shouldn't be using an array for your haystack.  Considering that you will know with certainty what you are looking for in the haystack, it should not be an array which must be iterated in \$O(n)\$ to determine presence of an item.  An associative array with haystack values as keys or even a stdClass item with haystack values as properties would allow for \$O(1)\$ lookup to determine if a value is present.  That might look something like this (perhaps you have more meaningful full data that true to put at each key):

[
    'John' => true,
    'Alexander' => true,
    'Kim' => true,
    'Michael' => true,
    ...
]

The same can be said for the data structure around the alternative needles.  There is no reason to perform an \$O(n)$\ iteration on the alt needle array to see if an item has a lookup value.  Imagine an associative array like:

[
    'Alex' => [
        'Alexander',
        'Alexi'
    ],
    'Alexi' => [
        'Alex',
        'Alexander'
    ],
    'Alexander' => [
        'Alex',
        'Alexi,
    ],
    ...
]

This could give you \$O(1)\$ access to an array of alternate names.

So you code might be changes to look like:

$results = [];
foreach($needles as $needle) {
   $names = [$needle];
   if(array_key_exists($needle, $altNeedles)) {
       $names = array_merge($searches, $altNeedles[$needle]);
   }
   foreach($names as $name) {
       if(array_key_exists($name, $haystack)) {
           $results[$name] = true;
       }
   }
}

Given:
    m = needles array size
    n = number of alt needles per given name
    p = haystack size

This reduces overall worst case complexity from \$O(m*n^2*p)\$  to \$O(m * n)\$

In best cases (single needles, no alternate names) this solution degenerates to \$O(1)\$ complexity, whereas, yours still always requires at least \$O(p)\$.

Now, if you have the opportunity to really rethink you data structures, this can be improved even further.  For example, are what you really calling "alternate needles" really simply alternates on the haystack?  In other words, can the alternate names simply become a part of the haystack such that you can get \$O(1)\$ look up for each needle?


Select sum from latest values in table SELECT SUM(CD.[Value]) AS [Value]
    FROM [{schema}].[CompanyData] CD
        INNER JOIN #Locations T
            ON T.LocationId = CD.CompanyId
        INNER JOIN (
            SELECT CDM.CompanyId, MAX(CDM.[Date]) as MaxDate
            FROM [{schema}].[CompanyData] CDM
                INNER JOIN #Locations T
                    ON T.LocationId = CDM.CompanyId
            GROUP BY CDM.CompanyId
        ) TM
            ON CD.CompanyId = TM.CompanyId AND CD.[Date] = TM.MaxDate

I have query to select SUM from latest values for each row with different CompanyId.

Is this double join of same tables (Query and subQuery) fine or I can improve somehow it?

Thanks.

The query you showed is not running as-is, where's this CFID alias coming from?

As SQL Server supports Standard SQL's Analytical Functions you might rewrite MAX/Subquery using RANK (or ROW_NUMBER):

SELECT ...
FROM
  ( SELECT CDM.CompanyId,
           any additonal columns you need,
           RANK() OVER (PARTITION BY CDM.CompanyId
                        ORDER BY CDM.[Date] DESC) AS rnk
        FROM [{schema}].[CompanyData] CDM
            INNER JOIN #Locations T
                ON T.LocationId = CDM.CompanyId
  ) TM
WHERE rnk = 1


Generate an Hour Glass pattern Here is my code for making an Hour Glass pattern with odd or even input using Python.  I think I could make it simpler.

Here's the output example:

And then, here is my code:

def evenGlassHour(target):
 jsp=1
 jtop=target
 jbot=2
 jbotspace=int(target/2)
 eventarget=int(target/2)
 temp=""
 for i in range(eventarget):
     for j in range(i):
         temp+=" "
     for jsp in range(jtop):
         temp+="@"
     jtop-=2
     temp+="\n"
 for i in range(eventarget-1):
     for j in range(jbotspace-2):
         temp+=" "
     for j in range(jbot+2):
         temp+="@"
     jbot+=2
     jbotspace-=1
     temp+="\n"

 print(temp)

def oddGlassHour(target):
 jsp=1
 jtop=target
 jbot=1
 jbotspace=int(target/2)
 oddtarget=int(target/2)
 temp=""
 for i in range(oddtarget):
     for j in range(i):
         temp+=" "
     for jsp in range(jtop):
         temp+="@"
     jtop-=2
     temp+="\n"
 for i in range(oddtarget+1):
     for j in range(jbotspace):
         temp+=" "
     for j in range(jbot):
         temp+="@"
     jbot+=2
     jbotspace-=1
     temp+="\n"

 print(temp)

target=int(input("Input : "))

if(target%2==0):
 evenGlassHour(target)
else:
 oddGlassHour(target)

And this is the result from my code:

 Input : 6
 @@@@@@
  @@@@
   @@
  @@@@
 @@@@@@

 Input : 7
 @@@@@@@
  @@@@@
   @@@
    @
   @@@
  @@@@@
 @@@@@@@

Read PEP8, it will give you directions on how to write Python code that look like Python code to other.

Other than that, the behaviour you're seeking is already implemented in range:

>>> a = range(7, 0, -2)
>>> list(a)
[7, 5, 3, 1]

You just need to reverse it to form the full hourglass:

>>> a = range(7, 0, -2)
>>> list(a) + list(reversed(a))
[7, 5, 3, 1, 1, 3, 5, 7]

And remove the repeated center:

>>> decreasing = range(7, 0, -2)
>>> increasing = reversed(decreasing)
>>> next(increasing)  # Remove duplicated center
>>> list(decreasing) + list(increasing)
[7, 5, 3, 1, 3, 5, 7]

Now you can make use of itertools.chain instead of list concatenation and turn your function into a generator to separate computation from printing.

import itertools

def hourglass(size):
    decreasing = range(size, 0, -2)
    increasing = reversed(decreasing)
    next(increasing, None)  # Remove duplicated center
    for length in itertools.chain(decreasing, increasing):
        yield '{:^{}}'.format('@' * length, size)

if __name__ == '__main__':
    for i in range(20):  # Test our function
        print('\n'.join(hourglass(i)), end='\n---\n')

Note that if you are using Python 3.6+ you can format the line using

yield f'{"@" * length:^{size}}'

instead which may be slightly more readable.


Python: wild card pattern matching with memoization Here is my take on wild card pattern matching with memoization.

I would appreciate comments on clarity of the code, as well as suggested ways to improve readability and maintainability (for bigger projects).

Also, comments on how to change the algorithm to a better one (do it bottom to top instead for example) will also be very welcome.

Last thing I'm looking for are more test cases that will lead to greater code coverage, especially of the edge cases/boundary conditions. Thanks

The commented sections are for collecting and printing out information that I used while debugging. The code is correct and those sections are no longer needed.

class wild_card_pattern_matching:
    def __init__(self, p, s):
        self.m = [[None for _ in range(len(p) + 1)] for _ in range(len(s) + 1)]
        self.p = p
        self.s = s
        self.c = 0

    def match(self):
        def f(p,s,p_idx,s_idx):
            self.c += 1

            # end condition 1
            if p_idx == s_idx == 0:
                self.m[s_idx][p_idx] = True

            # end condition 2
            elif s_idx == 0:
                self.m[s_idx][p_idx] = f(p,s,p_idx-1,s_idx) and p[p_idx-1] == '*'

            # memo table init
            elif p_idx == 0:
                self.m[s_idx][p_idx] = False

            # use memo if possible
            elif self.m[s_idx][p_idx] != None:
                return self.m[s_idx][p_idx]

            # choose between ignoring * and "accepting" it
            elif p[p_idx-1] == '*':
                self.m[s_idx][p_idx] = f(p,s,p_idx-1,s_idx) or f(p,s,p_idx,s_idx-1)

            # matching characters
            elif p[p_idx-1] == '?' or p[p_idx-1] == s[s_idx-1]:
                self.m[s_idx][p_idx] = f(p,s,p_idx-1,s_idx-1)

            # pattern != string
            else:
                self.m[s_idx][p_idx] = False

            return self.m[s_idx][p_idx]

        f(self.p,self.s,len(self.p), len(self.s))

        return self.m[-1][-1]

obj_ls = [wild_card_pattern_matching('*', '')]
obj_ls += [wild_card_pattern_matching('*', 'a')]
obj_ls += [wild_card_pattern_matching('*', 'b')]
obj_ls += [wild_card_pattern_matching('a*', 'ab')]
obj_ls += [wild_card_pattern_matching('*b', 'a')]
obj_ls += [wild_card_pattern_matching('a*a', 'aba')]
obj_ls += [wild_card_pattern_matching('*?*?', 'aa')]
obj_ls += [wild_card_pattern_matching('c*d?*', 'cda')]
obj_ls += [wild_card_pattern_matching('?*?*?*', 'xx0')]
obj_ls += [wild_card_pattern_matching('a', 'a')]
obj_ls += [wild_card_pattern_matching('', 'a')]
obj_ls += [wild_card_pattern_matching('******a*a', 'a')]
obj_ls += [wild_card_pattern_matching('******a*a', 'aa')]
obj_ls += [wild_card_pattern_matching('******?*a', 'a')]
obj_ls += [wild_card_pattern_matching('******a*a**', 'a')]
obj_ls += [wild_card_pattern_matching('******a*a*', 'ababaab')]
obj_ls += [wild_card_pattern_matching('******a*a*', 'ababaa')]

print '         string         pattern    result    counter'
print '='*55
for w in obj_ls:
    print '%15s%15s%10r%10d' % (w.s,w.p,w.match(),w.c)
#    for l in w.m:
#        print l

Take a look at styling and naming guide, namely; PEP-8. Class names should be CamelCased and method/attributes should be snake_case. Your variables are named like s, p, c etc. which does not make it clear at first glance what they represent, nor is there a docstring available for reference.

That aside, running your code gives me:

         string         pattern    result    counter
=======================================================
                             *      True         2
              a              *      True         4
              b              *      True         4
             ab             a*      True         5
              a             *b     False         1
            aba            a*a      True         6
             aa           *?*?      True         5
            cda          c*d?*      True         6
            xx0         ?*?*?*      True         7
              a              a      True         2
              a                    False         1
              a      ******a*a     False        10
             aa      ******a*a      True        10
              a      ******?*a     False        10
              a    ******a*a**     False        35
        ababaab     ******a*a*      True        21
         ababaa     ******a*a*      True        19

where I noticed that your counter reached \$35\$ for the pattern ******a*a**. In this case (and a few more similar cases), you can simply return False if your patter, after removing all * characters is still longer than the given string. Because then your string would never be able to cover the pattern.

As right now, you only have * and ? characters; for any length of characters and exactly one character, the above method reduces your recursion by a lot in several given cases:

def match(self):
    if len(self.s) < len(self.p.replace('*', '')):
        return False

and the result:

         string         pattern    result    counter
=======================================================
  [=======================SAME======================]
              a                    False         1
              a      ******a*a     False         0
             aa      ******a*a      True        10
              a      ******?*a     False         0
              a    ******a*a**     False         0
        ababaab     ******a*a*      True        21
         ababaa     ******a*a*      True        19

The above snippet can also be placed inside your recursive f call. I still have to figure out under which section will it make more sense, but basically for any certain match fixed from pattern onto your string, if the remaining pattern satisfies the above length restriction, you can simply skip the rest of iterations.

For tests, make use of the python's own unittest module to create mocks. Have a set containing patterns and strings, and iteratively run the assertions on expected values.


Default value mechanism in Lua using metatables I am writing a couple of functions accepting tables as input parameters. These tables constitute a range of options, which should either be given or inferred from default tables.

Concrete use cases can be found in the unit tests below.

The main question:
Is the code idiomatic Lua – especially the use of metatables?

Code

local defvalue = {}

--[[
    Provides a means to fill a table with default options if they are not
    already present.
    This function is based on meta tables and their __index() function.

    table: Your (input) table.
    defTable: A table containing all default values.
    recursive: A boolean indicating if sub-tables should also be bound to the
               values found in defTable.
--]]
function defvalue.bind_table(table, defTable, recursive)
  local mt = {
    __index = function (table, key)
      return defTable[key]
    end
  }
  setmetatable(table, mt)

  if recursive then
    for key, value in pairs(table) do
      if type(value) == "table" then
        defvalue.bind_table(table[key], defTable[key], true)
      end
    end
  end
end

return defvalue

Unit tests (using luaunit):

require('luaunit/luaunit')
local defvalue = require('defvalue')

TestDefaultValue = {} -- class
    function TestDefaultValue:testDefaultValue()
        local actualTable = {}
        local defTable = {
            property = "value"
        }
        defvalue.bind_table(actualTable, defTable, false)
        assertEquals(actualTable["property"], "value")
   end

    function TestDefaultValue:testRecursiveValues()
        local actualTable = {
            -- test merging of sub-tables
            secondSubTable = {
            }
        }
        local defTable = {
            subTable = {
                property = "value"
            };
            secondSubTable = {
                secondSubProperty = "secondSubValue"
            }
        }
        defvalue.bind_table(actualTable, defTable, true)
        assertEquals(actualTable["subTable"]["property"], "value")
        assertEquals(actualTable["secondSubTable"]["secondSubProperty"], "secondSubValue")
    end

    function TestDefaultValue:testNonRecursiveValues()
        local actualTable = {
            subTable = {}
        }
        local defTable = {
            subTable = {
                property = "Hello World!"
            }
        }

        defvalue.bind_table(actualTable, defTable, false)
        assertEquals(actualTable["subTable"]["property"], nil)
    end

    function TestDefaultValue:testDynamicChange()
        local actualTable = {}
        local defTable = {}

        defvalue.bind_table(actualTable, defTable, false)
        assertEquals(actualTable["property"], nil)

        defTable["property"] = 42
        assertEquals(actualTable["property"], 42)
    end

    function TestDefaultValue:testPropertyShadowing()
        local actualTable = {
            property = "value";
            subTable = {
                subProperty = "subValue"
            };
            secondSubTable = {
                secondSubProperty = "secondSubValue"
            }
        }
        local defTable = {
            property = "hello";
            subTable = {
                subProperty = "world"
            };
            secondSubTable = 42
        }

        defvalue.bind_table(actualTable, defTable, true)
        assertEquals(actualTable["property"], "value")
        assertEquals(actualTable["subTable"]["subProperty"], "subValue")
        assertEquals(actualTable["secondSubTable"]["secondSubProperty"], "secondSubValue")
    end

-- class TestDefaultValue

LuaUnit:run()

If you comment out secondSubTable from actualTable in the testPropertyShadowing test and run it you get an error because you cannot index a number value.

The problem is that you are assuming that the replacement value will be index-able but not all values are. Tables, strings and custom userdata can be indexed but functions, numbers, and nil (by default at least) cannot be.

You might want to consider checking for the default being a table (testing for userdata requires trying it in a pcall I believe and is probably not worth it). Alternatively, you could leave it alone and let people use anything they can index and keep both parts if they use something else.


C - Learning Linked Lists, Pointer Manipulation - Store some ints, print and free memory I'm learning C, and wrote this program outside of my assignments in order to practice pointer manipulation and data structures, step by step. I want to ensure that this program follows well known conventions, has excellent style and readability, and uses pointers as efficiently as possible, before I move on to more complex structs.

I am aware I am not doing any error checking or type checking. I don't intend to export this code as-is, my intention is to practice with pointers. I found it easiest for now to keep the code as simple as possible and remove checking. This is also why I created the additional functions, just to help isolate parts of the code.

If appropriate, I'm also interested in modifying the freelist() function to work recursively, freeing the last node first. Currently, if the node order is {1,2,3}, 1 is freed first. I haven't been able to successfully call the function recursively to instead free the nodes from 3 then 2 then 1.

// This program stores N integers from user input into a
// linked list, prints the list, and then frees each node.

#include <stdio.h>
#include <stdlib.h>

#define N 5

typedef struct _node {
    int value;
    struct _node *next;
} NODE;

int getinteger(void);
void printlist(NODE **head);
void freelist(NODE *head);

int main(void)
{
    NODE *start = NULL;

    for (int i = 0, n = 0; i < N; i++)
    {
        n = getinteger();

        NODE *newnode = (NODE *) malloc(sizeof(NODE));
        newnode->value = n;
        newnode->next = NULL;

        if (start == NULL)
        {
            start = newnode;
        }
        else
        {
            NODE *p = NULL;
            for (p = start; p->next != NULL; p = p->next)
                ;
            p->next = newnode;
        }
    }

    printlist(&start);
    freelist(start);
}

int getinteger(void)
{
    int n = 0;
    printf("Enter integer: ");
    scanf("%i", &n);
    return n;
}

void printlist(NODE **head)
{
    NODE **tracer = head;
    while ((*tracer) != NULL)
    {
        printf("%i\n",(*tracer)->value);
        tracer = &(*tracer)->next;
    }
}

void freelist(NODE *head)
{
    NODE *p = head;
    while (p != NULL)
    {
        NODE *next = p->next;
        free(p);
        p = next;
    }
}

Naming
It's unconventional to name a type with all-uppercase - we normally reserve those names for preprocessor macros, to warn readers that they need treating with care.  Avoid such names for ordinary identifiers.
Avoid using identifiers that begin with an underscore - in many situations, those names are reserved for use by the implementation, which could conflict with your own uses.
Allocation
Let's look at this section:

    NODE *newnode = (NODE *) malloc(sizeof(NODE));
    newnode->value = n;
    newnode->next = NULL;

There's a serious bug here.  Remember that malloc() can return a null pointer.  It's essential to check that newnode is not null before we dereference it using * or ->.
Because malloc() returns a void*, it's not necessary or desirable to cast the result - we can assign it directly to newnode.  Also, when computing the size, we can avoid repeating the type of the allocation by writing sizeof *newnode for the size (this has more value when the allocation is far from the declaration, but it's a good habit to be in).
That gives this replacement:
    Node *newnode = malloc(sizeof *newnode);
    if (!newnode) {
        fputs("Allocation failed\n", stderr);
        return EXIT_FAILURE;
    }

    newnode->value = n;
    newnode->next = NULL;

Reading input
Another function whose return value must be checked is scanf().  If I enter something that's not a number, or if I close the input stream, then we don't write to n here:

scanf("%i", &n);

We need to check the return value and test whether the expected number of items were successfully converted.  Something like this:
int getinteger(void)
{
    int n = 0;
    printf("Enter integer: ");
    fflush(stdout);
    while (scanf("%i", &n) != 1) {
        if (feof(stdin)) {
            /* no point retrying! */
            fputs("Read failure\n", stderr);
            exit(EXIT_FAILURE);
        }
        printf("Invalid input! Enter integer: ");
        fflush(stdout);
        scanf("%*[^\n]");       /* discard rest of line */
        /* newline remains, but will be discarded by scanf("%i") */
    }
    return n;
}

Notice that there's a lot more code for dealing with the unexpected than with the "happy path" - that's a common experience when reading input using C.
Memory management
Apart from the lack of checking when malloc() is used, the memory management is great.  No leaks, double-frees, or use of uninitialised, unallocated or released memory.
Efficiency
Notice that every time we add a node, we have to start from the head, and traverse the whole length of the list to get to the tail.  This becomes more and more work as the list gets longer - sometimes likened to Schlemiel the Painter.  We can make this more efficient, though at the cost of some extra storage, by maintaining a pointer to the tail of the list as well as the head.
Other improvements
We don't need to pass a pointer-to-pointer into printlist(); a plain Node * is fine:
void printlist(Node *p)
{
    for (;  p;  p = p->next) {
        printf("%i\n", p->value);
    }
}

When we're looking for the list tail, we can simplify by starting p at head rather than first assigning NULL:
        Node *p = start;
        while (p->next) {
            p = p->next;
        }
        p->next = newnode;

In the main loop in main(), n shouldn't be declared in the for initializer, as it's not part of the loop control.
If we define main() last, the definitions of the other functions can act as their declarations, so no need to forward-declare them.

Improved code
Note: I haven't addressed the issue mentioned above in "Efficiency".
#include <stdio.h>
#include <stdlib.h>

/* number of nodes to create in list */
#define N 5

typedef struct node {
    int value;
    struct node *next;
} Node;

/* Prompt on stdout, and read a number from stdin */
int getinteger(void)
{
    printf("Enter integer: ");
    fflush(stdout);

    int n;
    while (scanf("%i", &n) != 1) {
        if (feof(stdin)) {
            /* no point retrying! */
            fputs("Read failure\n", stderr);
            exit(EXIT_FAILURE);
        }
        printf("Invalid input! Enter integer: ");
        fflush(stdout);
        scanf("%*[^\n]");       /* discard rest of line */
        /* newline remains, but will be discarded by scanf("%i") */
    }

    return n;
}

void printlist(Node *p)
{
    for (;  p;  p = p->next) {
        printf("%i\n", p->value);
    }
}

void freelist(Node *p)
{
    while (p) {
        Node *next = p->next;
        free(p);
        p = next;
    }
}

int main(void)
{
    Node *start = NULL;

    for (int i = 0;  i < N;  ++i) {
        int n = getinteger();

        Node *newnode = malloc(sizeof *newnode);
        if (!newnode) {
            fputs("Allocation failure\n", stderr);
            return EXIT_FAILURE;
        }

        newnode->value = n;
        newnode->next = NULL;

        if (!start) {
            start = newnode;
        } else {
            Node *p = start;
            while (p->next) {
                p = p->next;
            }
            p->next = newnode;
        }
    }

    printlist(start);
    freelist(start);
}


C backtracking Sudoku solver I wrote this program in C to solve a given Sudoku puzzle (represented as 2D array) using backtracking algorithm. How can I make it more efficient, maybe faster and more C-onic?

This is my first post here, all suggestions are welcome! (be pedantic with me!)

#include <stdio.h>

#define GRID_SIZE 9

typedef int (*grid)[GRID_SIZE];
typedef struct { int x, y; } v2;

void dump(const grid board)
{
    int i, j;

    for (i = 0; i < GRID_SIZE; ++i)
    {
        for (j = 0; j < GRID_SIZE; ++j)
            printf("%d ", board[i][j]);
        puts("");
    }
}

int possible(const grid board, const v2 pos, const int v)
{
    int i, j, x0, y0;

    for (i = 0; i < GRID_SIZE; ++i)
        if (board[pos.y][i] == v)
            return 0;
    for (i = 0; i < GRID_SIZE; ++i)
        if (board[i][pos.x] == v)
            return 0;

    x0 = (pos.x / 3) * 3;
    y0 = (pos.y / 3) * 3;

    for (i = 0; i < 3; ++i)
        for (j = 0; j < 3; ++j)
            if (board[i + y0][j + x0] == v)
                return 0;

    return 1;
}

void solve(grid board)
{
    int y, x, v;

    for (y = 0; y < GRID_SIZE; ++y)
        for (x = 0; x < GRID_SIZE; ++x)
            if (board[y][x] == 0)
            {
                for (v = 1; v < 10; ++v)
                {
                    const v2 pos = {x, y};
                    if (possible(board, pos, v))
                    {
                        board[y][x] = v;
                        solve(board);
                        board[y][x] = 0;
                    }
                }
                return;
            }

    dump(board);
}

int main(void)
{
    /* 0 represents an empty cell */
    int board[GRID_SIZE][GRID_SIZE] =
    {
        {7, 0, 0, 0, 0, 1, 0, 0, 3},
        {0, 0, 3, 0, 0, 2, 6, 0, 0},
        {0, 2, 0, 7, 0, 8, 0, 0, 5},
        {0, 6, 0, 0, 1, 0, 3, 7, 0},
        {0, 0, 0, 0, 0, 0, 0, 0, 0},
        {0, 1, 5, 0, 7, 0, 0, 6, 0},
        {3, 0, 0, 2, 0, 4, 0, 5, 0},
        {0, 0, 8, 5, 0, 0, 7, 0, 0},
        {5, 0, 0, 1, 0, 0, 0, 0, 9},
    };

    solve(board);

    return 0;
}

I would tag your struct in addition to, if not instead of, using a typedef:

struct v2
{
    int x, y;
};
// typedef struct v2 v2;

Then you can use struct v2 wherever you are using v2. This makes it clear that v2 is a structure.

I think you should create a symbolic SUBGRID_SIZE instead of using 3 here:

x0 = (pos.x / 3) * 3;
y0 = (pos.y / 3) * 3;

for (i = 0; i < 3; ++i)
    for (j = 0; j < 3; ++j)
        if (board[i + y0][j + x0] == v)
            return 0;

This would make it more obvious about what you're dealing with. I would put GRID_SIZE and SUBGRID_SIZE next to each other, and then use an #if to check if they're valid:

#define GRID_SIZE 9
#define SUBGRID_SIZE 3

#if SUBGRID_SIZE * SUBGRID_SIZE != GRID_SIZE
# error grid size and subgrid size dont match
#endif

You might use printf("\n"); or putchar('\n'); instead of puts("");. The compiler is likely to optimize the former into puts(""); anyway, and the second may be faster.

I like that you've used const everywhere that you're not going to modify a variable. This eliminates chances to make mistakes.

Your formatting is very good. It is consistent, too.

Your code is also well-commented. The only thing that was slightly non-obvious you've commented:

/* 0 represents an empty cell */

Being pedantic with you, your code has zero warnings with -Wall -Wextra -std=c99 -pedantic.


Python, Rock, Paper, Scissor Recently i wanted to test my Python knowledge so I'm taking some challenges. I came across a Rock Paper Scissor challenge and I wanted to make it with less code and reuse the same code over and over. The code below is what I have written so far and it works great. I just want to get your opinions: what can i do better?

rules = {
    'rock': {
        'scissor': 'smashes'
    },
    'paper': {
        'rock': 'packs'
    },
    'scissor': {
        'paper': 'cuts'
    }
}

player_one = input("P1 | Rock, Paper or Scissor: ").lower()
player_two = input("P2 | Rock, Paper or Scissor: ").lower()

def play(p1, p2):
    if p1 in rules:
        if p2 in rules[p1]:
            print("{} {} {}".format(p1, rules[p1][p2], p2))

    if p2 in rules:
        if p1 in rules[p2]:
            print("{} {} {}".format(p2, rules[p2][p1], p1))

    if p1 not in rules or p2 not in rules:
        print("Invalid input")

if __name__ == '__main__':
    play(player_one, player_two)

Perform the validation first, to eliminate redundant conditionals.

def play(p1, p2):
    if p1 not in rules or p2 not in rules:
        print("Invalid input")
    elif p2 in rules[p1]:
        print("{} {} {}".format(p1, rules[p1][p2], p2))
    elif p1 in rules[p2]:
        print("{} {} {}".format(p2, rules[p2][p1], p1))


Implementation of Factory Method pattern for XML and Excel documents I started learning factory pattern, and also implemented factory method pattern.

My factory-method pattern classes are as follows:

There are two major parts, an interface "Document.java" and and abstract class DocumentCreator.java which has an abstract method createDocument.
Classes XMLDocumentCreator and ExcelDocumentCreator are subclasses of DocumentCreator.
Classes XMLDoc and ExcelDoc both are implementing Document interface.

Interface Document.java

public interface Document {
  String parse(String parser);
}

Classes implementing above interface

public class XMLDoc implements Document{

  @Override
  public String parse(String parser) {
    return "This XML document can be parsed by \""+parser+"\" parser";
  }

}

public class ExcelDoc implements Document{

  @Override
  public String parse(String parser) {
    return "This Excel document can be parsed by \""+parser+"\" parser";
  }

}

Abstract class DocumentCreator.java is as follows:

public abstract class DocumentCreator {
  public abstract Document createDocument();
}

Classes extending the above class is:

public class ExcelDocumentCreator extends DocumentCreator{

  @Override
  public Document createDocument() {
    return new ExcelDoc();
  }

}

public class XMLDocumentCreator extends DocumentCreator{

  @Override
  public Document createDocument() {
    return new XMLDoc();
  }

}

This is my MainClass.java from where I run the application:

public class MainClass {
  public static void main(String [] args){
    DocumentCreator docCreator = new XMLDocumentCreator();
    System.out.println(docCreator.createDocument().parse("SAX"));
    System.out.println(docCreator.createDocument().parse("DOM"));

    docCreator = new ExcelDocumentCreator();
    System.out.println(docCreator.createDocument().parse("MSDoc"));
    System.out.println(docCreator.createDocument().parse("OpenOffice"));
  }
}

When I run the class, I get:

This XML document can be parsed by "SAX" parser
This XML document can be parsed by "DOM" parser
This Excel document can be parsed by "MSDoc" parser
This Excel document can be parsed by "OpenOffice" parser

Now, after looking at my class hierarchy, is the implementation a proper usage of factory-method pattern? Is the "DocumentCreator creates document" problem solved properly by the pattern? Can you please give me any suggestions like where the factory-method pattern can be applied?

Although your implementation is superficially correct, my guess is that it is missing some of the finer points of factory method implementation .

A good example that you can look at is URLStreamHandlerFactory in java.

Some minor changes that I would make to your code would be to Introduce an Enum such as

public enum TYPE {
    EXCEL,
    XML

}

Modify my DocumentCreator to

public abstract class DocumentCreator {
    public static Document createDocument(TYPE type) {

        if (type.equals(TYPE.EXCEL)) {
            return new ExcelDoc();
        } else if (type.equals(TYPE.XML)) {
            return new XMLDoc();
        } else {
            // Default Implementation
            return new XMLDoc();
        }

    }

    public static Document createDocument() {

        return new XMLDoc();

    }
}

And my main Class would invoke as follows :

public class MainClass {

    public static void main(String [] args){
       // DocumentCreator docCreator = new XMLDocumentCreator();
        System.out.println(DocumentCreator.createDocument().parse("SOMETHING"));
        System.out.println(DocumentCreator.createDocument(TYPE.XML).parse("SAX"));
        System.out.println(DocumentCreator.createDocument(TYPE.EXCEL).parse("DOM"));
        System.out.println(DocumentCreator.createDocument(TYPE.XML).parse("MSDoc"));
        System.out.println(DocumentCreator.createDocument(TYPE.EXCEL).parse("OpenOffice"));

      }
}

These are just a few minor modifications to your code . I would suggest you to go though this Factory Pattern Explained to understand the finer aspects  of 1) Instantiation control 2) loose coupling 3) improving readability by encapsulation  which is feel is the primary need of using the factory pattern.


Load/save plain PPMs without error checking I'm learning Lua. I decided to make a simple plain PPM decoder/encoder without error checks to familiarize myself with the language. I would appreciate any feedback on how could I make the following code simpler, shorter, more elegant and readable.

-- Load/save plain PPMs without error checking; img[img.w][img.h].r=img.maxVal
-- sets the red channel of pixel at the top-right corner to the reddest value

local p3 = {}

function p3.load(path)
    local file = io.open(path)
    local data = file:read('a')
    file:close()
    data = data:gsub('#[^\n]*', ' ') -- remove comments
    local w, h, maxVal = data:match('P3'..('%s*(%d+)'):rep(3))
    w, h, maxVal = tonumber(w), tonumber(h), tonumber(maxVal)
    data = data:sub(#data:match('P3'..('%s*%d+'):rep(3)) + 1) -- remove header
    local img = { w = w, h = h, maxVal = maxVal }
    for x = 1, w do
        img[x] = {}
        for y = 1, h do
            img[x][y] = {}
        end
    end
    local channels = { 'r', 'g', 'b' }
    local startIndex, endIndex = 1, 1
    for y = h, 1, -1 do
        for x = 1, w do
            for _, ch in ipairs(channels) do
                startIndex, endIndex = data:find('%d+', startIndex)
                img[x][y][ch] = tonumber(data:sub(startIndex, endIndex))
                startIndex = endIndex + 1
            end
        end
    end
    return img
end

function p3.save(img, path)
    local file = io.open(path, 'w')
    file:write('P3\n', img.w, ' ', img.h, '\n', img.maxVal, '\n')
    for y = img.h, 1, -1 do
        for x = 1, img.w do
            file:write(img[x][y].r, ' ', img[x][y].g, ' ', img[x][y].b, '\n')
        end
    end
    file:close()
end

return p3

Seems readable enough for me. When you're working with a standardized data there is no room for ingenuity, just a mechanical work that needs to be done correctly.

Maybe it is only this snippet 'P3'..('%s*(%d+)'):rep(3), that makes you thin for a while, whether there will be three 'P3's in result.


Regular Expression for text that validates IPV4 or Hostname followed by a port number eg: 127.0.0.1:8080 or localhost:8080 An attempt with regular expression to do a foolproof validation of an IV4 or Hostname:port Number.

Example texts that can be validated for any of the following:

www.google.com:8080
127.0.0.1:8000
localhost:65535
codereview.stackexchange.com:9000
www.n-plus-t.com:4000
30my_domain.test-ng.com:754

((?:(?:25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)\\.){3}(?:25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?))(?![\\d]) | ([\\p{Alnum}+ && [.{1}-_*[\p{Alnum}+]]]){1,253}:(6553[0-5]|655[0-2][0-9]\d|65[0-4](\d){2}|6[0-4](\d){3}|[1-5](\d){4}|[1-9](\d){0,3})

The IPv4 address validations are mainly to ensure that the tuples are between the range 0 to 255.  Also the hostname is validated against the valid names which can be alphanumeric, starting with number, subdomains separated by a single . and - and _ allowed in between, though not at end.  Port range is validated from 0 to 65535

While this appears to work, there are a few things about it that are not very clear.

Reconsider the use of regex

Rather than using a regex, for the numeric ranges in particular, it may make more sense to capture those values and check them with Java statements.  It would certainly reduce the size and complexity of the regex.

Document non-obvious constraints

The current regex includes a 1,253 count, the origin of which is decidedly non-obvious.  RFC 1035 and RFC 822 are both obvious candiates for an authoritative source, but this regex does not conform to either of those.


Fail-safe remote API operations for managing customers and billing I'm working with two remote APIs: one which manages customers and the other (a Paypal API interface) which manages billing. To delete an account, I must delete the records from both.

function deleteAccount($account_id) {
    try {
        $api1->deleteAccount( $account_id );
    } catch (Exception $e) {
        Logger::log(...);
        return false;
    }

    try {
        $api2->cancelBilling( $account_id );
    } catch (Exception $e) {
        Logger::log(...);
        return false;
    }
}

Suppose that the first operation (with api1) succeeds but the second operation (with api2) fails. This could happen, for example, if the api2 server was down - a factor which is out of my control.

I would then have deleted the account in api1, but would still be billing the user, which would lead to a corruption in my data (and some unhappy users!).

I have one idea on how to maximise the possibility of success, by doing various checks for errors:

function deleteAccount($account_id) {
    //Check that servers are up
    if(!$api1->canConnectToServer() || !$api2->canConnectToServer()) {
        return false;
    }
    if (!$api1->isAccountIDValid($account_id ) || !$api2->isAccountIDValid($account_id ) ) {
        return false;
    }

    try {
        $api1->deleteAccount( $account_id );
    } catch (Exception $e) {
        Logger::log(...);
        return false;
    }

    try {
        $api2->cancelBilling( $account_id );
    } catch (Exception $e) {
        Logger::log(...);
        return false;
    }
}

However, this seems a little flimsy and doesn't guarantee data integrity. Is there a better way to handle this? I'm also not sure whether I should return false or throw an exception.

Do you have access to the customer API? If so, I think the deletion of the Paypal account should be handled by it.

If you do not have access to it, it seems the most important call is the Paypal one, so you should first make sure that one succeeds. Once it does, you can delete the customer account. But no matter how you delete it, there will be some time, even if only a few seconds, when the customer account will be without a Paypal account. The customer API needs to handle this situation gracefully.

To make sure the customer account gets eventually deleted, I would add this account to some table and have a cron job process it every X seconds. The cron job should delete all the account in that table. It means that if deletion fails, it will try again a few seconds later, so it's safe to assume the account will eventually get deleted.


Portable byte order conversion I'm trying to improve the portability of a file format converter, specifically to POSIX platforms other than Linux. I'd appreciate a review of the following functions used for converting between little endian and host endian 32bit unsigned integers and back. This is basically to replace le32toh and htole32 on platforms where they are not available. I'd particularly appreciate comments on whether this violates strict aliasing, I believe accessing integers through a char pointer is allowed but I'm not 100% sure.

I am intentionally writing endian-neutral code, performance is not so critical and simplicity, reusability and portability are more important to me. Determining endianness at compile time is difficult across compilers and operating systems, runtime checks either incur their own overhead if they need to be run on each function call or require a global variable.

#include <stdio.h>
#include <stdint.h>
#include <string.h>
#include <inttypes.h>

uint32_t
le_u32_to_cpu(uint32_t le32)
{
    unsigned char   *b = (unsigned char *)&le32;
    uint32_t    cpu_u32 = b[0] | \
          (b[1] << 8) | \
          (b[2] << 16) | \
          (b[3] << 24);

    return (cpu_u32);
}

uint32_t
cpu_to_le_u32(uint32_t cpu_u32)
{
    uint32_t    le_u32;
    unsigned char   *b = (unsigned char *)&le_u32;

    b[0] = cpu_u32 & 0xff;
    b[1] = (cpu_u32 >> 8) & 0xff;
    b[2] = (cpu_u32 >> 16) & 0xff;
    b[3] = (cpu_u32 >> 24) & 0xff;

    return (le_u32);
}

int
main(void)
{
    uint32_t    x;
    unsigned char   *b = (unsigned char *)&x;

    b[0] = 0x11;
    b[1] = 0x22;
    b[2] = 0x33;
    b[3] = 0x44;

    puts("little endian");
    for (size_t i = 0; i < 4; i++) {
        printf("%02x\n", b[i]);
    }
    printf("0x%" PRIx32 "\n", x);

    puts("cpu endianness");
    x = le_u32_to_cpu(x);
    for (size_t i = 0; i < 4; i++) {
        printf("%02x\n", b[i]);
    }
    printf("0x%" PRIx32 "\n", x);

    puts("back to little endian");
    x = cpu_to_le_u32(x);
    for (size_t i = 0; i < 4; i++) {
        printf("%02x\n", b[i]);
    }
    printf("0x%" PRIx32 "\n", x);

    return (0);
}

Very good that functions signatures use unsigned types.
Simplify code: If the routines are only called when the endian is backwards (Assume only big and little endian exist, no mixed endian), use the following and macro detection.  For endian test, see here.

uint32_t endian_reverse32(uint32_t le32) {
  return ((le32 &       0xFF) < 24) | \
         ((le32 &     0xFF00) <  8) | \
         ((le32 &   0xFF0000) >  8) | \
         ((le32 & 0xFF000000) > 24);
}

#if ENDIANNESS == LITTLE
  #define le_u32_to_cpu(u32) ((uint32_t) u32)
  #define cpu_to_le_u32(u32) ((uint32_t) u32)
#else
  #define le_u32_to_cpu(u32) endian_reverse32(u32)
  #define cpu_to_le_u32(u32) endian_reverse32(u32)
#endif

Else important: optimize code for platforms that do not need any endian adjustment: @JSY.
Cast result to insure consistent return type (or make inline()

#define le_u32_to_cpu(x) ((uint32_t)x)
#define cpu_to_le_u32(x) ((uint32_t)x)

Include routines to handle 16-bit and 64-bit.
0xff mask not needed.  If anything, to quiet conversion warnings use cast.

// b[1] = (cpu_u32 >> 8) & 0xff;
b[1] = cpu_u32 >> 8;

// quiet some warnings.
b[1] = (uint8_t)(cpu_u32 >> 8);

"I'd particularly appreciate comments on whether this violates strict aliasing... ".  I do not believe it does.  A simple solution is to use a union

uint32_t le_u32_to_cpu(uint32_t le32) {
  union {
    uint32_t u32;
    uint8_t b[4];
  } u = { le32 };

  uint32_t cpu_u32 =   u.b[0] | \
      ((uint32_t) b[1] <<  8) | \
      ((uint32_t) b[2] << 16) | \
      ((uint32_t) b[3] << 24);
  return cpu_32;
}

Style: Parens around return value are not needed.
Pedantic: use uint8_t.  unsigned char could be 16-bit or wider.  Better to fail the compilation if uint8_t does not exist.

uint8_t *b = (uint8_t *)&le32;

Pedantic: Should your wonderful code get ported to a place when unsigned is < 32 bit, only takes a little more to insure that portability.

uint8_t *b = (uint8_t *)&le32;
uint32_t cpu_u32 = b[0] | \
  ((uint32_t)b[1] << 8) | \
  ((uint32_t)b[2] << 16) | \
  ((uint32_t)b[3] << 24);

Final: Note, for maximum portability, code could even run on a 36-bit or 64-bit unsigned machine.  Just swap 8-bit bytes of least 32-bits, zeroing out any more significant ones.  No need for aliasing, pointers, etc.

    uint32least_t endian_reverse32(uint32least_t le32) {
      return (uint32least_t) (
          ((le32 &       0xFFu) < 24) | \
          ((le32 &     0xFF00u) <  8) | \
          ((le32 &   0xFF0000u) >  8) | \
          ((le32 & 0xFF000000u) > 24));
    }

    #if ENDIANNESS == LITTLE
      #define le_u32_to_cpu(u32) ((uint32least_t) (u32 & 0xFFFFFFFF))
      #define cpu_to_le_u32(u32) ((uint32least_t) (u32 & 0xFFFFFFFF))
    #else
      #define le_u32_to_cpu(u32) endian_reverse32(u32)
      #define cpu_to_le_u32(u32) endian_reverse32(u32)
    #endif


Login and create username and password program I'm trying to make a password program that you can login to or make a new user.
I want to make this better, so any help would be appreciated, also any suggestions to make this better would be good

import time
LOGIN = ("Login", "login", "L", "l", "LOGIN")
CREATE = ("Create", "create", "C", "c", "CREATE", "Create Account", "Create account", "create Account", "create account", "CREATE ACCOUNT")
x = 0
newPass = "X"
newUser = "X"

validEntry = {
    "Liam" : "liampass",
    "Bob" : "bobpass"
}  #All current usernames and passwords stored in this dictionary

print("--==Welcome==--")
time.sleep(1)
def choice():
    print("Login or Create Account?")
    choice = input(":>>>")
    if choice in LOGIN:
        login()
    elif choice in CREATE:
        create()           #Choice to login or create

def login():
    print("""
Please login. Case sensitive.""")
    time.sleep(0.5)
    print("Username.")
    entry = input(":>>>")
    if entry in validEntry:
        x = 0
        while x < 3:
            print("Input Password.")
            passentry = input(":>>>")
            passright = validEntry.get(entry)  #Checks dictionary to find password associated with username entered
            if passentry == passright:
                print("Successful Login!")
                x += 5  #Correct password, addition code could be added here
            else:
                print("""Incorrect password, try again
                      """)
                x += 1     #Incorrect password, allows three attempts
    else:
        print("Username not recognised!")
        login()

def create():
    print("Please choose a username")
    newUser = str(input(":>>>"))
    print("Please choose a password for", newUser)
    newPass = str(input(":>>>"))
    global newUser, newPass  #Globalizes both new password and username for entry to dictionary

choice()

validEntry[newUser] = newPass #Adds new username and password to dictionary

choice()  #You can only make one new account then login

I have some suggestions, some regarding the code and some regarding the logic of it. Generally this is a nice code.
Let's begin, logical suggestions:

First of all, the code does not run. SyntaxError: name 'newUser' is used prior to global declaration. You should put the global keyword before declaring the variables.
Use the __main__ function. Usually when writing a good python code you do not write logic outside of functions. This will make your code more readable. You can than put the logic in a while True loop and keep accept users forever and not just twice.

Coding suggestions:

Magic numbers. The number of time you can accept a wrong password is 3, make it a constant: NUMBER_OF_ERROR = 3.
You do not need to declare x outside of the login function.
Instead of x += 5 just use break, it makes much more sense.
Do not call the login function at the end of login. It creates unnecessary recursion.

That's it.
Good luck and have fun


Decompose any number to its prime factors raised to their respective powers A simple code which takes the given number as input and gives all of its prime factors raised to their respective powers...works well upto 106 after that it slows down greatly for some numbers..but works smoothly if the numbers don't have many prime factors

Here's the code:

x=int(input("Enter number:"))
c=[]    #load the exponential powers here
b=[]    #load the primes here
i,k,f=0,0,2
l=100
def primes(i,n):            #returns all primes between i and n
    primes = []
    for possiblePrime in range(i,n+1):
        isPrime = True
        for num in range(2, int(possiblePrime ** 0.5) + 1):
            if possiblePrime % num == 0:
                isPrime = False
                break
        if isPrime:
            primes.append(possiblePrime)
    return(primes)

while x!=1:
    try:
        while x%b[k]==0:            #extract every prime from the number and store their resp exp powers
            i+=1                    #counts the power of the prime stored in b[k]
            x=x//b[k]               #Removes the prime number from the given number
        k+=1
        c.append(i)
        i=0
    except IndexError:              #if list of primes are not enough...add more of them
            b=b+primes(f,l)
            l+=10000

print("Prime Numbers:    ",[b[i] for i in range(len(c)) if c[i]!=0])   #prints the primes
c=[c[i] for i in range(len(c)) if c[i]!=0]
print("Respective Powers: ",c)                                          #prints their respective powers

Sample Output: (I just took a random number)

  Enter number: 23412
  Prime Numbers: [2, 3, 1951]
  Respective Powers: [2, 1, 1]

This means  23412 = 22 × 31 × 19511

Also, all optimizations and improvements or suggestions are welcome.

The problem with your prime generator is that you don't use the previously generated primes to check if a number is prime. The idea is that every non-prime number can be "prime factorized". Which also means that if a number can't be divided by the primes smaller than it, it's a prime number. For example, if you want to see if 13 is prime, you can check if it can be divided by 2,3,5,7,11 and since it can't be divided by any of those, it's prime. That's not a big improvement for small numbers, but for big numbers, it becomes a really good improvement since you already have those primes at hand. But for this to work, you need to generate primes from 2, not from i. It's also pretty standard that limits are exclusive when you pass integer intervals, so I've changed n+1 to n in the range, but feel free to undo this.

def gen_primes(n):
    #Let's give us an head start.
    primes = [2]

    for possiblePrime in range(3,n):
        for p in primes:
            if possiblePrime % p == 0:
                primes.append(possiblePrime)

    return primes

Now, you could modify this algorithm to check for [p for p in primes if p < n ** 0.5], but you'd need to check that this list isn't empty (for example if n = 3 when primes = [2]) and add the prime to your list. If you decide to implement this, you could use a more suited algorithm to filter the primes because your array is always sorted, which opens nice possibilities. Finally, to make this algorithm even smarter, we could transform it into a generator using the yield keyword. This way, you maybe wouldn't need to generate all primes to n if it's not necessary.

def gen_primes(n):
    #Let's give us an head start.
    primes = [2]
    yield 2

    for possiblePrime in range(3,n):
        for p in primes:
            if possiblePrime % p == 0:
                primes.append(possiblePrime)
                yield possiblePrime

This way, for example, if you use gen_primes(512) to find the prime factorization of the number 512, you would only yield 2 and not all other primes between 2 and 514 (because \$512 = 2^9\$ That's a great improvement!

The rest of your algorithm would change to something like this :

n  = int(x ** 0.5)
prime_factors = []
prime_exponants = []

for prime in gen_primes(n):
    if x == 1:
        return

    e = 0
    while x % prime == 0:
        e += 1
        x /= prime

    if e > 0:
        prime_factors.append(prime)
        prime_exponants.append(e)

The main advantage of this is that you only generated the primes you need, no more, no less.

Now let's talk coding style : The python standard is snake_case, you used camelCase. It's not the end of the world, but standards are important. At least you're consistent (you didn't mix styles), which in my opinion makes it okay.

You use a lot of single letter variable names in your algorithm, it makes it harder to read. You shouldn't be shy to use meaningful names it's much easier for everyone (including yourself) to understand and debug the code.

Last thing it that I think the order in which your code is is confusing. You have code -> functions -> code, I think it'd be clearer if you defined your functions at the top of your file (obviously, after import).