Static class for constants with reference to java resources I want to store the colors I use in the colors.xml file. In addition to using them in an xml-layout, I also want to use these colors in my Java code, when drawing things to the Canvas.

The thing is, I am not always using the colors in a class that extends Application. For example, I have classes for objects that get drawn to the Canvas.

The thing is, I don't want to store the context in each class that needs access to my resources file.

My current solution is the following:

colors.xml

<?xml version="1.0" encoding="utf-8"?>
<resources>
    <color name="colorOne">#123456</color>
</resources>

A static class called Attributes for constants, including colors:

public class Attributes {
    private static final Resources RESOURCES = MainActivity.getContext().getResources();

    public static final int ONE_COLOR = RESOURCES.getColor(R.color.colorOne);
    public static final int SOME_OTHER_CONSTANT = 1;
}

MainActivity:

public class MainActivity extends Activity {

    private static Context context;

    @Override
    protected void onCreate(Bundle savedInstanceState) {
        super.onCreate(savedInstanceState);
        this.context = getApplicationContext();

    @Override
    protected void onPause() {
        super.onPause();
    }

    @Override
    protected void onResume() {
        super.onResume();
    }

    public static Context getContext() {
        return context;
    }
}

And, finally in my object class:

public class ScreenArea {
    private Rect area;
    private Paint paint;

    public ScreenArea(Rect area) {
        this.area = area;
        paint = new Paint();
        paint.setColor(Attributes.COLOR_ONE);
    }

    public void draw(Canvas canvas) {
        canvas.drawRect(area, paint);
    }
}

Okay, so first of all, this works. But, I am wondering if this is the right way to go. I have been reading about the Singleton class, but most examples are very basic an not focusing on storing constants, like I am doing right now.

So my questions:

Is this the right way to go or should I implement a Singleton?
If so, who can point me in the right direction?

well as pointed out using singletons is considered bad practice and using global constants in an object-orientatd world is bad practice as well...

So, what is your suggestion?

Consider the purpose of the attribute

you want to define a app-specific attribute (color), an attribute you want to configure - this is the very reason why you use ressources, right. Or do you want to define a constant?

 1. it's a constant

if you don't want to specify a configurable attribute you should define a constant in your app

private static final int COLOR_ONE = 0x123456;

 2. it's an attribute

but if you want to specify a configurable attribute you simply define it as such in your app:

int colorOne = MainActivity.getContext().getResources().getColor(R.color.colorOne);

you are right, this might be a bit intricately so it would be just as simple to wrap this call in a method

private int getColorFromRessource(int colorId){
    return MainActivity.getContext().getResources().getColor(colorId);
}

this would be applied in your setup of the Paint in an quite easy way:

public ScreenArea(Rect area) {
    this.area = area;
    paint = new Paint();
    paint.setColor(getColorFromRessource(R.color.colorOne)); //obvious
  //paint.setColor(Attributes.COLOR_ONE);  far less obvious
}


Return the largest product of 2 numbers that is a multiple of 9 in O(n) Given an array of integers, I have to return the largest product of any 2 numbers, and that product has to be a multiple of 9.

Example input 1:
int[] a = {6, 5, 7, 2, 4, 8, 9};
Example output:
72

Example input 2:
int[] b = {1, 6, 7, 3};
Example output:
18

This is easy with 2 nested loops, calculating all products between every element of the array and checking if the product is a multiple of 9, but that solution is \$O(n^2)\$. I was told I can solve this problem in \$O(n)\$ only, but I'm struggling to cover all the cases.

I have the following code:

    int max_number = -1;
    int max_multiple_9 = -1;

    for(int i = 0; i < input.length; i++) {
        if(input[i] % 9 == 0) {
            if(input[i] > max_multiple_9) max_multiple_9 = input[i];
            else if(input[i] > max_number) max_number = input[i];
        } else {
            if(input[i] > max_number) max_number = input[i];
        }
    }

    return max_multiple_9*max_number;

This only works for cases where there is a multiple of 9 in the array (like the first example), because I'm looking for the greatest multiple of 9 and multiplying it for the greatest any other number. But how would I change the code to also find products that don't involve multiples of 9, such as 6*3 in the second example?

The solution will be of one of two forms :

  a * b where a is divisible by 9

or

  c * d where c is divisible by 3 and d is divisible by 3

To find the maximum of the first from we need to find the greatest number divisible by 9, and combine it with the greatest number, or the second greatest if the one divisible by 9 is the greatest.

To find the maximum of the second form we need to find the two greatest numbers divisible by 3.

So you can loop all numbers and determine for each number if it is a better candidate for a,b, c or d.

After that loop, calculate a*b and c*d, and the greatest of both wins, and is the result.

This algorithm results in \$O(n)\$ complexity.


Generic function, which populates a collection based on collection interface implementation As the above title states I have a function that I use to populate a collection's status names, currency names and other information that I do not store in the database.

This is my function:

    public static List<T> PopulateStatusNames<T>(List<T> items) where T : class
    {

        if (typeof(IStatusResult).IsAssignableFrom(typeof(T)))
        {
            Dictionary<int, string> statuses = new Dictionary<int, string>();

            if (typeof(IActivityStatusResult).IsAssignableFrom(typeof(T))) statuses = GlobalConstants.GetActivityStatuses();
            if (typeof(IPurchaseStatusResult).IsAssignableFrom(typeof(T))) statuses = GlobalConstants.GetPurchaseStatuses();
            if (typeof(IQuotationStatusResult).IsAssignableFrom(typeof(T))) statuses = GlobalConstants.GetQuotationStatuses();
            if (typeof(IInvoiceStatusResult).IsAssignableFrom(typeof(T))) statuses = GlobalConstants.GetInvoiceStatuses();

            foreach (var item in items.OfType<IStatusResult>())
            {
                item.StatusName = statuses[item.Status];
            }
        }

        if (typeof(ICurrencyResult).IsAssignableFrom(typeof(T)))
        {
            Dictionary<int, string> currencies = GlobalConstants.GetCurrencies();

            foreach (var item in items.OfType<ICurrencyResult>())
            {
                item.CurrencyName = currencies[item.Currency];
            }
        }

        return items;
    }

So if a collection implements an interface of type IStatusResult I look up the approapriate status names and loop through the collection to set the status name.

As you also notice in the above that I loop through my collection twice because if  my collection implements another type of interface I need to get a different set key value pairs so that I can look up the name

Is there a way to re-write the below function to populate status names, currency names in a single for loop

Sample POCO that implement these interfaces

Invoice:

public class InvoiceSearchResult : IInvoiceStatusResult, ICurrencyResult
{
    public int Id { get; set; }
    public string InvoiceNo { get; set; }
    public DateTime InvoiceDate { get; set; }
    public string Project { get; set; }
    public string Location { get; set; }
    public string Customer { get; set; }
    public int Currency { get; set; }
    public string CurrencyName { get; set; }
    public int Status { get; set; }
    public string StatusName { get; set; }
    public decimal Subtotal { get; set; }
    public decimal TotalTax { get; set; }
    public decimal Total { get; set; }
}

Location:

public class LocationSearchResult : IActivityStatusResult
{
    public int Id { get; set; }
    public string Name { get; set; }
    public int Status { get; set; }
    public string StatusName { get; set; }
}

So as you can see I have a different combination of collections that implement these interfaces, based on the interfaces I am trying to figure out what needs to be populated

If you look closely there is actually four loops in your code. Two is explicit and other two from code like items.OfType<IStatusResult>() where if you look at the implementation of OfType<T> is looks something like below

foreach (object obj in source)
      {
        if (obj is TResult)
          yield return (TResult) obj;
      }

I would rather prefer to have one foreach loop to loop through all the items once and based on the type of each item, assign the appropriate properties something like below

foreach (var item in items)
            {
                if (typeof(ICurrencyResult).IsAssignableFrom(item.GetType()))
                {
                    item.CurrencyName = currencies[item.Currency];
                }
                else if (typeof(IStatusResult).IsAssignableFrom(item.GetType()))
                {
                    item.StatusName = statuses[item.Status];
                }
            }


Uppercase the initial char of every string in a list My problem is that I have a sequence, potentially infinite, of strings. Let's suppose it to be

{"Mickey Mouse", "Bugs Bunny", "Winnie the Pooh"}

I have to make an exthension-method Initials that returns a sequence of their capital letters in uppercase, so in this case, doing:

var names = new string [] {"Mickey  Mouse", "Bugs    Bunny", "Winnie  the Pooh"}
foreach (var cl in names . Initials ())
Console.WriteLine(cl);

returns, in order, MM, BB, WTP. Note that whitespaces have not to be considered.

After hours of staring at the screen, i managed to write this not so goodlooking code:

public static IEnumerable<string> Initials(this IEnumerable<string> names)
{
    var res = new List<string>();
    var  listOfArrayOfStrings = names.Select(s => s.Split());
    foreach (var arrayOfStrings in listOfArrayOfStrings)
    {
        string newString = "";
        foreach (var singleString in arrayOfStrings)
        {
            if (!string.IsNullOrWhiteSpace(singleString)) //no whitespaces
            {
                var temp =  char.ToUpper(singleString[0]).ToString();       //first upper String
                newString += temp;
            }
        }
        res.Add(newString);
    }
    return res;
}

Then, my disliked R# suggested me to do some refacory and then it became the following:

var  listOfArrayOfStrings = names.Select(s => s.Split());
            return listOfArrayOfStrings.Select(arrayOfStrings =>
                (from singleString in arrayOfStrings
                 where !string.IsNullOrWhiteSpace(singleString)
                 select char.ToUpper(singleString[0]).ToString())
                 .Aggregate("", (current, temp) => current + temp))
                 .ToList();

Now, I have two questions about this:

Is this, in your opinion, a good way to reach my aim?
I will have to do a written test where I'll have to do something like that. How can I "guess" something like that without R#? I mean, I'm still having difficulties understanding all that Linq stuff. Have you got any idea to how I can approach problems like this? Often these test are about managing IEnumerables, so Linq could be very helpful, but I rarely understand the right sequence of Linq queries.

Start by writing a method that returns the initials of a single string.

private static string GetInitials(string name)
{
    ???
}

You have the right idea, but there are some overloads we can use that will make the code a bit nicer:

private static string GetInitials(string name)
{
    var initials = name
        .Split((char[])null, StringSplitOptions.RemoveEmptyEntries)
        .Select(w => char.ToUpper(w[0]));
    return new string(initials.ToArray());
}

Here we've used the overload of Split that won't return empty entries, so we don't have to check for empty entries. We've also used the string(char[]) constructor instead of building up the string ourselves.

Now Initials we be written as

return names.Select(GetInitials);

There's a problem with both versions in that the method will not return if the input sequence is infinite (which you've mentioned is a case you want to handle).

Suppose we have this helper method to create an infinite sequence of a single value

private static IEnumerable<T> Repeat<T>(T value)
{
    while (true)
    {
        yield return value;
    }
}

Then our method should be able to handle this client code:

foreach (var cl in Repeat("Mickey Mouse").Initials().Take(5))
{
    Console.WriteLine(cl);
}

The versions posted must process every element of the sequence before they return, which isn't possible for infinite sequences.


Optimizing run time of Python solution to Project Euler Problem 34 This is a brute force solution to Project Euler Problem 34 with only 1 optimization (i.e.: caching factorials):

import time, math
start = time.time()

total = 0

# Cache factorials
factorials = [math.factorial(x) for x in xrange(0,10)]
range_end = 7 * factorials[9]

for i in range(3,range_end+1):
    number = i
    factorials_sum = 0

    while(number > 0):
        digit = number % 10
        number /= 10
        factorials_sum += factorials[digit]

    if(factorials_sum == i):
        total += factorials_sum

print "Time elapsed: %5.2f" % (time.time() - start)
print total

I know it could be further optimized (f.e. by choosing numbers that the for loop could pass on. However, I would like to ask why is this algorithm running 10.3 s on my computer (2.66 GHz Intel Core 2 Duo)? Similar solutions I have found run in ~2 seconds or less. Is there any general optimization I could make?

You can shave some (minimal) time by bailing out early of your factorial sum calculation, and using the divmod operator:

while number and factorial_sum <= number:
    number, digit = divmod(number, 10)
    factorial_sum += factorials[digit]

But that is still going to be fairly slow, and take several seconds to compute. If you want a quick solution, you need to stop rechecking numbers that you already know cannot fulfill the condition. This is best done checking the digits from most significant to least significant. A possible implementation using recursion would be:

factorials = [1, 1]
for j in range(2, 10):
    factorials.append(j * factorials[-1])

def factorion_sum(number, factorial_sum):
    number *= 10
    total = 0
    for digit in range(10):
        new_number = number + digit
        new_factorial_sum = factorial_sum + factorials[digit]
        if new_number == new_factorial_sum:
            total += new_number
        # If the smallest number in the next iteration exceeds
        # the next largest factorial sum, we are done here
        if new_number*10 <= new_factorial_sum + factorials[-1]:
            total += factorion_sum(new_number, new_factorial_sum)
    return total

print sum(factorion_sum(j, factorials[j]) for j in range(1, 10)

This produces the right result in a fraction of a second.


Moody game characters I've been doing some self-imposed exercises to get accustomed to Python and below is my code.

import random
from random import randint
from random import randrange
from mainlist import legendaryitems
from mainlist import waystodie
from mainlist import powerup
from mainlist import mood
from mainlist import ponder
import time

class Player(object):
    def __init__(self, name):
        self.name = name
        self.lifecondition = True
        self.hitpoints = 100
        self.items = []
        self.waystodie = []
        self.mood = ""

    def death(self):
        self.lifecondition = False

    def ways_to_die(self, death):
        self.waystodie.append(death)

    def  add_items(self, item):
            self.items.append(item)

    def change_hp(self, hp):
        self.hitpoints = hp

    def change_mood(self, mood):
        self.mood = mood

    def __repr__(self):
        return repr(self.name)

def addplayers(alist):
    players = []
    for item in alist:
        item = Player("%s" % item)
        players.append(item)
        print "%s enters the game with %s HP of life and feels pretty much alive" %  (item.name, item.hitpoints, )
    return players

def player_mood(playerlist, moodlist):
    for item in playerlist:
        mood = random.choice(moodlist)
        item.change_mood(mood)

playerlist = ["Player1", "Player2", "Player3", "Player4", "Player5", "Player6", "Player7", "Player8", "Player9", "Player10"]

players = addplayers(playerlist)
player_mood(players, mood)

print players[1].mood
print players[2].mood

player_mood(players, mood)

print players[1].mood
print players[2].mood

My intention is to create actions depending on the mood of characters. Basically it's gonna be a "There can be only one!" game. The reason of my post is not so that someone else can write the code for me but for feedback on how to write more clean, efficient code following Python's philosophy at the same time.

I am calling the instances through the players list but is there any other way I should be using to recall them from the class?

I'm able to convey what I want to do into code but is it ok to interact with standalone functions with my class? Or should I be building methods within another Class (utilize inheritance as well) to interact with Player()?

Note that you can significantly simplify the import statements (two of which you never even use):

import random
import time

from mainlist import legendary_items, mood, ponder, powerup, ways_to_die

I have also arranged and renamed them as recommended by the style guide.

The methods in your class aren't consistently named. For example, ways_to_die and add_items are named differently but do similar things; based on what they actually do I would have named them add_way_to_die and add_item, although as the lists they add to are public attributes you could just let the user append to them without the convenience functions. Similarly, death should probably be named kill - instance methods generally contain verbs, as they do things to the instance.

The change_hp and change_mood methods are more consistently named, but also redundant; why not let the user do e.g. player.mood = 'happy' rather than player.change_mood('happy')? If you need to control e.g. the range of valid values later, you can make it a property:

class Player(object):

    VALID_MOODS = ['happy', 'sad']

    @property
    def mood(self):
        return self._mood  # note "private by convention" leading underscore

    @mood.setter
    def mood(self, mood):
        if mood not in self.VALID_MOODS:
            raise ValueError('Invalid mood: {}'.format(mood))
        self._mood = mood

playerlist (should be player_list, as above) can be built much more efficiently with a list comprehension, factoring out addplayers entirely:

player_list = [Player("Player{}".format(n+1)) for n in range(10)]

this makes it easier to change the number of players, and harder to accidentally leave one of the numbers out (e.g. [..., 'Player4', 'Player6', ...]) or make a typo (e.g. [..., 'Player4', 'Players5', 'Player6', ...]). If you still want to introduce them, you can easily do so:

for player in player_list:
    print ("{0.name} enters the game with {0.hit_points} HP of life "
           "and feels pretty much alive").format(player)

Note the use of Python's implicit concatenation of strings to keep the line length under 80 characters, and str.format rather than the more old-fashioned % formatting.

You could also get rid of player_mood (which is not very well-named), for example by:

adding mood as an initialisation parameter (def __init__(self, name, mood):) and calling random.choice in the list comprehension;
adding mood as an optional parameter (def __init__(self, name, mood=None):) and calling random.choice in the __init__ method if mood is None:; or
providing a convenience function Player.set_random_mood to the class (this could be combined with 2.: if mood is None: self.set_random_mood()).

Python indexing is 0-based, so:

print players[1].mood
print players[2].mood

will actually show the mood attributes of 'Player2' and 'Player3'. Not a deal-breaker, but you should be aware of it!

You should generally write docstrings for your modules, classes, methods and functions; this provides useful information to anyone using your code (including you!)


Formatting a CHANGES text file as HTML I have a function which is used to convert text (a CHANGES file) into HTML for reading. The function is essentially line-based: if the line is of this type, do this thing; if some other type, do that thing; etc. Further logic needs to be added before the fallback

b.WriteString("
" + line + "
\n")

but I wanted to better organize my code before adding the more complex logic.

As written the code looks like a nightmare to me -- there has to be some better, more idiomatic way to write this. I'm new enough to Go that I'm not sure what the best way forward is.

var h3Regex = regexp.MustCompile("^Done for version ([1-9]\\.[0-9]\\.[0-9]) \\(released ([0-9/-]+)\\):$")
var h4Regex = regexp.MustCompile("^  (Fixed|Added|Changed|Removed)$")
var variable = "[a-zA-Z][a-zA-Z0-9_]*(?:\\(\\))?"
var stuff = "^  (?:[A-Z]+ )?[0-9]+- GP functions? (" + variable + "(?:, ?" + variable + ")*)$"
var GPRegex = regexp.MustCompile(stuff)

func layout(text string) string {
    lines := strings.Split(html.EscapeString(text), "\n")
    var b bytes.Buffer
    for _, line := range lines {
        match := h3Regex.FindStringSubmatch(line)
        if len(match) > 0 {
            b.WriteString("
Version " + match[1] + ", released " + match[2] + "
\n")
            continue
        }
        match = h4Regex.FindStringSubmatch(line)
        if len(match) > 0 {
            b.WriteString("
" + match[1] + "
\n")
            continue
        }
        match = GPRegex.FindStringSubmatch(line)
        if len(match) > 0 {
            funcs := strings.Split(match[1], ",")
            if len(funcs) > 1 {
                b.WriteString("
GP functions ")
            } else {
                b.WriteString("
GP function ")
            }
            for idx, f := range funcs {
                if idx > 0 {
                    b.WriteString(", ")
                }
                b.WriteString(`<code class="GP">` + f + `</code>`)
            }
            b.WriteString("
\n")
            continue
        }
        b.WriteString("
" + line + "
\n")
    }
    return b.String()
}

Take a look at html/template package from standard library. It will help you a lot in separating parsing code and HTML generation. Also it will escape all input by default.

Some other notes:

It is handy to use ` instead of " when writing regular expressions with lots of escapes.
GPRegex variable will be exported. Should it be?
Kudos for using bytes.Buffer and not concating strings by hand (:


Inverting a binary tree I have written this code to invert a binary tree.

Thus if the original tree is:

Then the tree returned should be:

/**
 * Definition for a binary tree node.
 * struct TreeNode {
 *     int val;
 *     TreeNode *left;
 *     TreeNode *right;
 *     TreeNode(int x) : val(x), left(NULL), right(NULL) {}
 * };
 */
class Solution {
public:
    TreeNode* invertTree(TreeNode* root) {
        if(root==NULL)
            return NULL;

        /* Performing post-order traversal of the tree */

        //Visiting the left node first
        if(root->left != NULL)
            invertTree(root->left);

        //Visiting the right node next
        if(root->right != NULL)
            invertTree(root->right);

        //Visiting the current node; swapping the left and the right nodes of the current node
        TreeNode* tn = new TreeNode(0);
        tn=root->right;
        root->right=root->left;
        root->left=tn;

        return root;
    }
};

While the code runs correctly and returns the required output, I think I am invoking undefined behavior when the control hits the leaf nodes.  Since, at the leaf, a new node is created and it is assigned (incorrectly) the right of the current node and so on.

Could someone please clarify if what I am doing is indeed wrong?

Note: The question and the images are taken from LeetCode.com

Here you are creating a node unnecessarily:

   TreeNode* tn = new TreeNode(0);
    tn=root->right;

You could write simpler as:

   TreeNode* tn = root->right;

Because you're not using the original value of tn anyway, you only need it as a work variable for swapping the left and right nodes.

Also, your implementation doesn't use the return value of the recursive invertTree calls. You can use them better and get rid of the conditional statements, making the solution more compact:

TreeNode* invertTree(TreeNode* root) {
    if (root == NULL) {
        return NULL;
    }

    TreeNode* work = root->right;
    root->right = invertTree(root->left);
    root->left = invertTree(work);
    return root;
}

   I think I am invoking undefined behavior when the control hits the leaf nodes.

No, you're not invoking "undefined behavior" when hitting the leaf nodes. Actually I think you meant to say intermediary nodes. At leaf nodes, the function receives a null node and you return null. At intermediary nodes, you allocate an unnecessary node, as explained earlier. The unnecessary allocation doesn't trigger an undefined behavior, but since you never free the allocated memory, this is a memory leak, and indeed a problem if the program was used for a substantial amount of time, accumulating unreleased memory.


Lightweight C Exception Library // names are unique within a function, any valid variable name
// a jump table try, allowing return values to determine the exception thrown(index in a jump table, -1 being success for example)
// you only need to put in what you need, not all exceptions, or total exception count
#define GTRY(name, exc) void* __jt_##name[exc];int __jtix_##name = 0;
// register an exception in the jump table. you don't need to do them all
#define GTEX(name, exception) __jtix_##name;__jt_##name[__jtix_##name++] = &&__tr_##exception##name;
// resets the jump table index to a given value, useful for overwriting exceptions without a new try.
#define GTRST(name, index) __jtix_##name = index;
// a standard try, doesn't actually add anything, but for cleanliness.
#define TRY(name) ;
// throw an exception by name
#define THROW(name, exception) goto __tr_##exception##name;
// throws an exception by jump table index
#define GTHROW(name, exn) goto *__jt_##name[exn];
// begins a catch block
#define CATCH(name, exception) goto __trend_##name;__tr_##exception##name:;
// signifies where the try ends
#define ENDTRY(name) __trend_##name:;
// 'finally' blocks need only be put after an end try.

Usage Example:

int main(int argc, char* argv[]) {
    TRY(magic)
    THROW(magic, EOF)
    CATCH(magic, EOF)
    printf("eof!\n");
    ENDTRY(magic)
    printf("complete\n");
    GTRY(fagic, 2)
    int eof = GTEX(fagic, EOF)
    int rst = GTEX(fagic, RST)
    GTHROW(fagic, rst)
    CATCH(fagic, EOF)
    printf("eof2!\n");
    CATCH(fagic, RST)
    printf("rst\n");
    ENDTRY(fagic);
    //
    TRY(nest) {
        THROW(nest, EOF);
    } CATCH(nest, EOF) {
        printf("eof3!\n");
    }
    ENDTRY(nest)
    printf("complete3\n");
    return 0;
}

My purpose was to create a macro-based exception system that can run on embedded devices with ease(magic is done compile-time). There is a potential performance malice if CPUs don't support labels as values and GTRY is used.

Semantics
In the languages that implement try, throw and catch the scope is generally larger than a single function. The scope of the exception handling is set by the try block and the catch block. The C goto is limited to the current function, to jump outside of the current function you need to use setjmp() and longjmp(). Your implementation does not seem to use setjmp() and longjmp().

Maintainability
Using a goto is generally decreasing maintainability, hiding a goto is a bad idea. If I have to maintain code that contains a goto I want to be able to immediately know that there is a goto and I want to be able to find the label immediately as well.

Performance
A reason to use goto is to improve performance. When goto's actually degrade performance that is another reason not to use them.


Repository/persistence-service implementation I have the following situation:
I have...

Models like ServerRoom and ServerCabinet
Repositories of the Model instances (in-memory collection)
PersistenceServices which are interfacing the persistence-method I chose (mssql db)

The problem I am facing is repetition.
Basically, the implementation of a PersistenceService is not really bound to a specific Model, only to it's instantiation (from a DataRow) and conversion (from Business Object to DataRow), so I thought the following would be good:

From

/// <summary>
/// Implements generic database-crud operations to prevent repetition
/// Most of the CRUD-operations are the exactly the same, and those are implemented
/// </summary>
/// <typeparam name="T"></typeparam>
public abstract class DbPersistenceService<T> : IPersistenceService<T>
    where T : class
{
    protected DataTable _dataTable;

    // Update, Delete, GetAll, Save, Delete, Contains and FindRow excluded

    /// <summary>
    /// Add an instance to the repository
    /// </summary>
    /// <param name="instance"></param>
    public void Add(T instance)
    {
        _dataTable.Rows.Add(ObjectToRow(instance));
    }

    /// <summary>
    /// Get the instance with the specified id
    /// </summary>
    /// <param name="id"></param>
    /// <returns></returns>
    public T GetById(long id)
    {
        var row = FindRow(id);
        if (row == null)
            return null;
        return RowToObject(FindRow(id));
    }

    /// <summary>
    /// Converts a datarow to the domain object
    /// </summary>
    /// <param name="row"></param>
    /// <returns></returns>
    protected abstract T RowToObject(DataRow row);

    /// <summary>
    /// Converts a domain object to a datarow
    /// </summary>
    /// <param name="instance"></param>
    /// <returns></returns>
    protected abstract DataRow ObjectToRow(T instance);

}

So, the CRUD operations are the same for each model, only the conversion between domain and persistence layer is different (constructor of ServerCabinet needs other information from a DataRow than a ServerRoom) and this conversion is must be implemented by overriding RowToObject and ObjectToRow.
I am not satisfied with this design, as I still have to maintain one PersistenceService-class for each Model (e.g. ServerCabinetDbPersistenceService). I tried things like an interface IBusinessDataConverter, but it doesnt work, because that would be instance-methods whereas methods like GetById (above) cannot access an instance, only static, class-level methods.

How would you design it, do you have other general info about bad-practices I used?

          /* Using an ORM like EntityFramework is not an option */

I would tend to agree with Adrian's comment that you might be better off finding some lightweight / micro ORM to start from, if bigger things such as EF or NHibernate aren't options for you.

Still, if you really want to go all the way long your own route with generics, then, FWIW, I'd probably put it along the lines of something like:

(just a rough, compilable sketch; untested, clearly not thread-safe, with no provisions or hints on how to go from the DataTables to your DB engine's SQL, etc)

public interface IStore<TItem>
{
    TPersistent Deserialize<TPersistent>(TItem item)
        where TPersistent : class, new();

    TItem Serialize<TPersistent>(TPersistent instance)
        //if only for consistency with Deserialize:
        where TPersistent : class, new();
}

public interface IPersistence<TStore, TItem>
    where TStore : IStore<TItem>
{
    void Add<TPersistent>(TPersistent instance)
        //if only for consistency with GetById, or other various
        //Getters/Finders/Selectors:
        where TPersistent : class, new();

    TPersistent GetById<TPersistent>(long id)
        where TPersistent : class, new();
}

public abstract class Persistence<TStore, TItem> : IPersistence<TStore, TItem>
    where TStore : IStore<TItem>
{
    protected abstract TItem Get<TPersistent>(long id);

    protected abstract void Insert(TItem item);

    // Useful to use TStore here (and not just IStore<TItem>) to allow for
    // type-safe internal friendship between the store and persistence implementations later on,
    // if needed
    protected TStore Store { get; private set; }

    protected Persistence(IStore<TItem> store)
    {
        // Note we *want* this to barf at construction time,
        // if it turns out that store (the parameter)
        // does not conform to TStore
        Store = (TStore)store;
    }

    void IPersistence<TStore, TItem>.Add<TPersistent>(TPersistent instance)
    {
        Insert(Store.Serialize(instance));
    }

    TPersistent IPersistence<TStore, TItem>.GetById<TPersistent>(long id)
    {
        return Store.Deserialize<TPersistent>(Get<TPersistent>(id));
    }
}

/// <summary>
/// Friend of DataTablePersistence (via its internal DataSet)
/// </summary>
public class DataTableStore : IStore<DataRow>
{
    internal readonly DataSet DataSet;

    internal DataTable EnsureTable(Type instanceType)
    {
        var tableName = instanceType.Name;
        DataTable table;
        if (!DataSet.Tables.Contains(tableName))
        {
            table = new DataTable(tableName);
            //(left to-do) also define the table schema (columns),
            //after what instanceType tells us
            DataSet.Tables.Add(tableName);
        }
        else
        {
            table = DataSet.Tables[tableName];
        }
        return table;
    }

    public DataTableStore()
    {
        DataSet = new DataSet();
    }

    TPersistent IStore<DataRow>.Deserialize<TPersistent>(DataRow item)
    {
        var instance = new TPersistent();
        //e.g., use System.Reflection to hydrate instance (from item)
        return instance;
    }

    DataRow IStore<DataRow>.Serialize<TPersistent>(TPersistent instance)
    {
        var table = EnsureTable(instance.GetType());
        var item = table.NewRow();
        //e.g., use System.Reflection to dehydrate instance (into item)
        return item;
    }
}

/// <summary>
/// Friend of DataTableStore (via DataTableStore's internal DataSet)
/// </summary>
public class DataTablePersistence : Persistence<DataTableStore, DataRow>
{
    protected override DataRow Get<TPersistent>(long id)
    {
        var table = Store.EnsureTable(typeof(TPersistent));
        return table.Rows.Find(id);
    }

    protected override void Insert(DataRow item)
    {
        // presumably, item was created by some DataSet[<table name>].NewRow(),
        // so, it already knows the table in which it ought to be inserted
        item.Table.Rows.Add(item);
    }

    public DataTablePersistence()
        : base(new DataTableStore())
    {
    }
}

public class Program
{
    public class Person
    {
        public long Id { get; set; }

        public string First { get; set; }

        public string Last { get; set; }
    }

    public class Asset
    {
        public long Id { get; set; }

        public string Category { get; set; }

        public string Name { get; set; }
    }

    public static void Main(string[] args)
    {
        IPersistence<DataTableStore, DataRow> persistence = new DataTablePersistence();

        persistence.Add(new Person { First = "John", Last = "Smith" });

        persistence.Add(new Asset { Category = "Vehicle", Name = "Cadillac" });

        var john = persistence.GetById<Person>(1);

        var asset = persistence.GetById<Asset>(1);

        //etc, etc
    }
}

Finally, it'll get more interesting (challenging) if you want to support dehydrating (i.e., emitting SQL insert/update/delete) arbitrary object graphs where some instances are transient/freshly created, while others come with their existing keys from your underlying DB, etc)

'Hope this helps.


Haskell Servant - login endpoint with two error states Here is one a handler for dealing with logins, I've recently refactored it to try and make dealing with the different error cases a bit more elegant, though I still think there is room for improvement:

import Config
import Control.Monad.Reader
import Data.Aeson
import Data.Aeson.Types
import Data.Bifunctor (bimap)
import Data.Char
import Data.List as List
import Data.Text.Lazy
import Data.Text.Lazy.Encoding
import Database.Persist
import Database.Persist.Postgresql
import GHC.Generics
import Servant
import Servant.Auth.Server
import Types (Account(..))
import Queries

type Route = "login" :> ReqBody '[JSON] Credentials :> Post '[JSON] LoginAttempt

instance FromJWT (Key Account)
instance ToJWT (Key Account)

data Credentials = Credentials
    { loginEmail :: String
    , loginPass  :: String
    } deriving (Eq, Show, Read, Generic)

instance FromJSON Credentials

instance ToJSON Credentials where
    toJSON creds =
        object
            [ "account" .= loginEmail creds
            ]

data LoginAttempt
    = Ok { authToken :: Text }
    | AuthenticationErr
    | TokenGenerationErr
    deriving (Generic)

instance ToJSON LoginAttempt where
    toJSON (Ok token) =
        object [ "token" .= token ]
    toJSON AuthenticationErr =
        object [ "error" .= ("Invalid username or password" :: Text) ]
    toJSON TokenGenerationErr =
        object [ "error" .= ("Error generating token" :: Text) ]

action :: Config -> JWTSettings -> Credentials -> Handler LoginAttempt
action config jwt (Credentials email pass) = do
  login <- runSqlPool (Queries.validateAccount email pass) $ getPool config
  result <- either (const $ return AuthenticationErr) generateToken login
  case result of
    AuthenticationErr -> throwError $ err401 { errBody = encode . toJSON $ result }
    TokenGenerationErr -> throwError $ err500 { errBody = encode . toJSON $ result }
    _ -> return result
  where
    generateToken :: Key Account -> Handler LoginAttempt
    generateToken k = do
      token <- liftIO $ makeJWT k jwt Nothing
      pure $ either (const TokenGenerationErr) (Ok . decodeUtf8) token

Since the Handler LoginAttempt from action only ever contains Ok, its result should instead be Handler Text. And then nobody ever sees a LoginAttempt object, so we don't need it.

action :: Config -> JWTSettings -> Credentials -> Handler Text
action config jwt (Credentials email pass) = do
  login <- either authenticationError pure =<<
    runSqlPool (Queries.validateAccount email pass) (getPool config)
  token <- either tokenGenerationError pure =<<
    liftIO (makeJWT login jwt Nothing)
  pure $ decodeUtf8 token -- perhaps: pure $ decode $ object [ "token" .= decodeUtf8 token ]
  where
    autheticationError   _ = throwError $ err401 { errBody = encode $ object [ "error" .= ("Invalid username or password" :: Text) ] }
    tokenGenerationError _ = throwError $ err500 { errBody = encode $ object [ "error" .= ("Error generating token"       :: Text) ] }


Ruby ActiveRecord importing library In my Rails app I need to import some files from CSV and Excel files. I needed it in 2 models so I have written lib/importable.rb:

module Importable
  def self.included(base)
    base.send :extend, ActAsMethods
  end

  module ActAsMethods
    def act_as_importable(&block)
      @import_block = block
      send :extend, ImportableMethods
    end

    attr_reader :import_block
  end

  module ImportableMethods
    def import_from(file, *args)
      raise 'This model is not importable' if import_block.nil?

      send(:before_import, *args) if respond_to? :before_import

      sheet = open_spreadsheet(file)

      header = sheet.row(1).map(&:to_sym)

      (2..sheet.last_row).map do |i|
        row = Hash[header.zip(sheet.row(i))]

        import_block.call(row, *args)
      end
    end

    private
    def open_spreadsheet(file)
      logger.info "Importing file #{file.original_filename}"
      case File.extname(file.original_filename)
      when '.csv'  then Csv.new(file.path, nil, :ignore)
      when '.xls'  then Excel.new(file.path, nil, :ignore)
      when '.xlsx' then Excelx.new(file.path, nil, :ignore)
      when '.ods'  then Openoffice.new(file.path, nil, :ignore)
      else raise "Unknown file type: #{file.original_filename}"
      end
    rescue
      raise "Cannot open file: #{file.original_filename}"
    end
  end
end

class ActiveRecord::Base
  include Importable
end

And in model I need to write, i.e.:

class Invoice < ActiveRecord::Base
  act_as_importable do |row, company|
    invoice = company.invoices.find_or_initialize_by_name(row[:name].to_s)

    vat_in = if row[:client].is_a? String
               row[:client]
             else
               row[:client].to_i
             end
    client = company.clients.find_by_vat_in(vat_in.to_s)

    invoice.client = client

    invoice.pay_date = row[:pay_date]
    invoice.full_amount = row[:full_amount].to_i
    invoice.paid_amount = row[:paid_amount].to_i

    invoice.save!

    invoice
  end
end

The importing in controller looks like:

  def save
    file = params[:file]

    logger.debug file.path

    Invoice.transaction do
      @invoices = Invoice.import_from(file, current_company)
      @invoices.each(&:save)
    end
  rescue RuntimeError => error
    logger.debug error
    redirect_to import_company_clients_path, alert: I18n.t('invalid_file')
  rescue ActiveRecord::RecordInvalid
    redirect_to import_company_clients_path, alert: I18n.t('invalid_record')
  else
    respond_to do |format|
      format.html
      format.json { render json: @invoices }
    end
  end

But IMHO it's a little overbloated. Have you any suggedtions to make it more reagable and Gemified?

Notes:

Your code looks pretty good, but I personally wouldn't have written it as a mixable module, this code seemps completely independent from ActiveRecord. I'd write a SpreadSheetReader class and just use it from wherever you want (this way the code is more modular).
Where is import_from called from? it seems some code is missing.
This vat_in conditional seems too verbose. Which are the possible values of row[:client]?

I'd write something like:

company = company_from_somewhere_i_dont_know
columns = [:name, :client, <other columns>...]

SpreadSheetReader.rows(:columns => columns).map do |row|
  invoice = company.invoices.find_or_initialize_by_name(row[:name].to_s)
  vat_in = row[:client].is_a?(String) ? row[:client] : row[:client].to_i.to_s
  client = company.clients.find_by_vat_in!(vat_in)
  invoice.update_attributes!({
    :pay_date => row[:pay_date],
    :full_amount => row[:full_amount].to_i,
    :paid_amount => row[:paid_amount].to_i,
  })
  invoice
end


Save all HTML field names in Controller - it is bad or not bad idea? I have sample controller InboxController in my Inbox app. I want control with field name from my controller. If I will control with fields name from controller will not be destroyed MVC rules?

<?php

namespace App\Http\Controllers;

use App\User;
use Illuminate\Http\Request;
use Illuminate\Support\Facades\Validator;

class InboxController extends Controller
{
    /*
    |--------------------------------------------------------------------------
    | Field names in HTML form
    |--------------------------------------------------------------------------
    */

    /**
     * Recipient field name.
     *
     * @var string
     */

    public $recipient = 'recipient';

    /**
     * Sender field name.
     *
     * @var string
     */

    public $sender = 'sender';

    /**
     * Subject field name.
     *
     * @var string
     */
}

Html code:

<input type='text' name="{{ $sender }}">

I recomend you to use Symfony like form builder. There you can create one form and reuse it anywhere.

$form = FormFactory::create(FormType::class, $user)
    ->add('name', TextType::class)
    ->add('email', EmailType::class, [
        'rules' => 'unique:users,email',
    ])
    ->add('save', SubmitType::class, ['label' => 'Save user']);

What is also good is with this form you can also validate your input at the same time.

 $form->handleRequest();

 if ($form->isSubmitted() && $form->isValid()) {
      //Your code
 }

Here is link for repository


Using generators to print the tree-like structure of a project The goal of this code is to print the entity tree of a VHDL project. There are a readme and very minimal tests on the github repo.

I am trying to refactor the code to use generators in order to familiarize myself with yield, and to think in terms of iterators, not raw data structures. Here is my current attempt at doing so:

import re
from sys import argv
from os import walk
from os.path import join as pjoin

EXCLUDES = ["implementation", "testbench"]
BASIC_ID_REGEX = "[a-z][a-z0-9]*(?:_[a-z0-9]+)*"

def _vhdltree(level, filepath, pattern, vhd_files):
    for entity, component in find_entities(filepath, pattern):
        """Codereview: I am not specifically interested in feedbacks for the
        following snippet (except about the recursive design), but if you
        have a particularly elegant
        solution which keeps the "UI" part very minimal, suggestions
        are welcome"""
        path = vhd_files.get(component.lower(), "Not Found")
        print("    "*level + entity + " : " + path)
        if path != "Not Found":#Probably ugly, but lazy
            _vhdltree(level+1, path, pattern, vhd_files)

def find_entities(filepath, pattern):
    with open(filepath) as f:
        for l in f:
            m = pattern.match(l)
            if m:
                yield m.group('entity'), m.group('component').split(".")[-1]

def find_vhd(proot):
    for (dirpath, dirnames, filenames) in walk(proot):
        if not isexcluded(dirpath.lower()):
            for fn in filenames:
                if fn[-4:].lower() == ".vhd":
                    yield fn[:-4].lower(), pjoin(dirpath, fn)

def isexcluded(path):
    for excluder in EXCLUDES:
        if excluder in path:
            return True
    return False

def vhdltree(filepath, proot):
    instantiation_regex = ("\s*(?P<entity>{0})\s*:\s*entity\s*(?P<component>{0}(?:\.{0})*)"  # NOQA
                           .format(BASIC_ID_REGEX))
    p = re.compile(instantiation_regex, re.IGNORECASE)
    vhd_files = dict(find_vhd(proot))
    _vhdltree(0, filepath, p, vhd_files)

if __name__ == "__main__":
    #Codereview: user is trusted, don't care about this part
    vhdltree(argv[1], argv[2])

Any feedback about the use of generators is welcome. Specifically, I wonder if it would be possible to avoid "realizing" the vhd_files generator as a dict while keeping the search in _vhdltree as simple as a get? Are there more pythonic alternatives or more efficient solutions for a structure designed to be one-wa searchable (I know this is probably never where the program spends its time, but I'd like to know for my general culture). I looked in itertools but didn't find an immediate solution.

I am interested in modern, Python3 solutions for my general knowledge. From a practical point of view however, the code is supposed to be 2-compatible.

Generators

Since you were asking about generators specifically, let’s start with some suggestions on how you’re using them:

Your find_entities() function opens a file, then iterates over the lines in that file. That’s a good use of Python’s ability to iterate directly over the lines in a file, but it’s a bit fiddly to unit test because you have to create files.

I often write my generators to take an iterable of lines (which could be an open file object, or something else) – this makes the code easier to unit test, and more generic. I can read the lines from an arbitrary iterator, not just a file. It’s a small change:

def find_entities(lines, pattern):
    for line in lines:
        m = pattern.match(line)
        if m:
            yield m.group('entity'), m.group('component').split('.')[-1]

Your _vhdltree() function is really doing two things: it’s working out the level, entity and path for an element in the iterator, and then printing it. Problem with printing is that it becomes hard to reuse this computation elsewhere. Better if you split the printing out from this function, and defer that to the caller.

Something like:

def parse_vhdltree(level, filepath, pattern, vhd_files):
    for entity, component in find_entities(open(file path), vhd_files):
        try:
            path = vhd_files[component.lower()]
        except KeyError:
            continue
        yield level, entity, path
        for entry in parse_vhdltree(level+1, path, pattern, vhd_files):
            yield entry

...

for level, entity, path in parse_vhdltree(0, filepath, p, vhd_files):
    print('    ' * level + entity + ' : ' + path)

(I’ll talk about the KeyError in a minute.)
I really like your find_vhd() generator, and it’s a good example of how generators can clean up code in Python. All the messy logic of deciding which files to count is absorbed in this function.

One tweak I would make is to throw away the dirnames data – you’re not using it. Using an underscore in its place is a common convention to indicate you’re not interested in a particular value:

def find_vhd(proot):
    for (dirpath, _, filenames) in walk(proot):
        if not isexcluded(dirpath.lower()):
            for fn in filenames:
                if fn[-4:].lower() == ".vhd":
                    yield fn[:-4].lower(), pjoin(dirpath, fn)

General Python style

Generators aside, here are some comments on your general Python style:

I have a bit of a hobby horse about use of regular expressions. I think you’re using them for a sensible reason, but you could be better about commenting, and explaining what they’re trying to match.

Breaking it over multiple lines, and explaining exactly what you’re trying to match against, will make it much easier to read. Additionally, when you come back to this code in six months, you’ll thank your past self for those comments.

I would also suggest pre-compiling the instantiation_regex at the top of the file, alongside BASIC_ID_REGEX.
Your isexcluded() function could be replaced with a one-liner:

return any(excluder in path for excluder in EXCLUDES)

This is using a lazy generator expression as the argument, and the nice thing about any() is that it exits as soon as it gets a positive element.

I see that you’re already using an equivalent expression with all() in the GitHub version, which is similarly lazy. It exits as soon as it finds a negative element.
I’m not a super fan of from foo import bar style imports, especially from the standard library. Even worse if from foo import bar as baz, as you’ve done with os.path.join.

Characters are cheap – use the full, stdlib name. It will make your code much easier to read for other Python developers, who don’t need to worry about what pjoin() does. They can just recognise os.path.join.
Rather than using a sentinel value from dict.get() to denote a missing key in the dictionary, do a lookup with square brackets and catch the KeyError. This is more Pythonic, and avoids problems if the sentinel value turns up in the dictionary.

And I think using a dictionary here is fine. It’s simple, clean, and it works.


Rendering drop-down boxes for country and region In PHP how can I follow the DRY (don't repeat yourself) principle in this piece of code? The 2 classes in question share an awful lot of code.

<?php

interface iSelect
{
    public static function display();
}

class CountrySelect implements iSelect {

    static $template = 'select_template.php';

    public static function display() {

        if ( class_exists( 'View' ) ) {

            // Get the full path to the template file.

            $templatePath = dirname( __FILE__ ) . '/' . static::$template;

            $viewData = array(
                "options" => '_countries',
                "optionsText" => 'name',
                "optionsValue" => 'geonameId',
                "value" => 'selectedCountry',
                "caption" => 'Country'
            );

            // Return the rendered HTML
            return View::render( $templatePath, $viewData );

        }
        else {
            return "You are trying to render a template, but we can't find the View Class";
        }
    }
}

class RegionSelect implements iSelect {

    static $template = 'select_template.php';

    public static function display() {

        if ( class_exists( 'View' ) ) {

            // Get the full path to the template file.

            $templatePath = dirname( __FILE__ ) . '/' . static::$template;

            $viewData = array(
                "options" => '_regions',
                "optionsText" => 'name',
                "optionsValue" => 'name',
                "value" => 'selectedCity',
                "caption" => 'Region'
            );

            // Return the rendered HTML
            return View::render( $templatePath, $viewData );

        }
        else {
            return "You are trying to render a template, but we can't find the View Class";
        }
    }
}

?>

The code in question is for php templating. Here is the template:

        <select data-bind="options: <? echo $options ?>,
            optionsText: '<? echo $optionsText ?>',
            optionsValue: '<? echo $optionsValue ?>',
            value: <? echo $value ?>,
            optionsCaption: '<? echo $caption ?>'">
        </select>

Here is the View.php which takes the $ViewData and the $template file and then renders it:

<?php

/** View.php **/

class View {

    /**
     * -------------------------------------
     * Render a Template.
     * -------------------------------------
     *
     * @param $filePath - include path to the template.
     * @param null $viewData - any data to be used within the template.
     * @return string -
     *
     */
    public static function render( $filePath, $viewData = null ) {

        // Was any data sent through?
        ( $viewData ) ? extract( $viewData ) : null;

        ob_start();
        include ( $filePath );
        $template = ob_get_contents();
        ob_end_clean();

        return $template;
    }
}
?>

In PHP how can I follow the DRY (don't repeat yourself) principle in this piece of code? The 2 classes in question share an awful lot of code.

One approach would be to create a common superclass and extract the common elements of the function. As everything except the viewData is the same, just pass that to the newly created common function:

<?php
interface iSelect {
    public static function display();
}

abstract class AreaSelect implements iSelect {
    public static function displayData($viewData) {
            if ( class_exists( 'View' ) ) {

                // Get the full path to the template file.

                $templatePath = dirname( __FILE__ ) . '/' . static::$template;

                // Return the rendered HTML
                return View::render( $templatePath, $viewData );

            }
            else {
                return "You are trying to render a template, but we can't find the View Class";
            }
    }
}

class CountrySelect extends AreaSelect {
    public static function display() {
        $viewData = array(
            "options" => '_countries',
            "optionsText" => 'name',
            "optionsValue" => 'geonameId',
            "value" => 'selectedCountry',
            "caption" => 'Country'
        );
        parent::displayData($viewData);
    }
}

class RegionSelect extends AreaSelect {
    public static function display() {
        $viewData = array(
            "options" => '_regions',
            "optionsText" => 'name',
            "optionsValue" => 'name',
            "value" => 'selectedCity',
            "caption" => 'Region'
        );
        parent::displayData($viewData);
    }
}

The displayData is now so generic, that it might make sense to create a AbstractSelect class instead of an AreaSelect as it is likely that other selects will use the exact same method as well.

Extract

You should always be careful when using extract like this. If you ever have an entry that is called filePath, you would overwrite the $filePath argument and thus break your code.

Additionally, if $viewData is ever user-controlled, you would have an LFI vulnerability. It's unlikely, but I would always try to make generic methods such as this as secure as possible (the calling code is unlikely to expect a security issue), even for unlikely use (that's also why I would probably check filePath for directory traversal, just in case, as well as html-encode all variables in the template); at the very minimum, I would document the issue.

Misc

why are you checking if View exists (it should always exist, right?), but not if $template exists (it's more likely that one of many templates gets misplaced than that the single view class gets lost)?
your placement of curly brakets is not consistent.
your comments aren't really needed. Stuff like // Return the rendered HTML is already clear from the code. It would be better to remove comments such as these, and add PHPDoc style comments documenting the methods behaviour instead.


C++ Implementation of a dictionary data structure I am quite new to C++ and would like some feedback on my implementation of a dictionary data structure. The code is below.

#ifndef DICTIONARY_H_INCLUDED
#define DICTIONARY_H_INCLUDED

#include <initializer_list>
#include <algorithm>
#include <vector>

template <typename T, typename U>
class Dictionary{
    private:
        std::vector<T> keys;
        std::vector<U> values;
    public:
        Dictionary();
        Dictionary(std::initializer_list<std::pair<T,U>>);
        bool has(T) const;
        void add(T,U);
        T* begin();
        T* end();
        U operator[](T);
};

template <typename T, typename U>
T* Dictionary<T,U>::begin(){
    return &(keys[0]);
}

template <typename T, typename U>
T* Dictionary<T,U>::end(){
    return &(keys[keys.size()-1])+1;
}

template <typename T, typename U>
Dictionary<T,U>::Dictionary (std::initializer_list<std::pair<T,U>> store){
    for (std::pair<T,U> object : store){
        keys.push_back(object.first);
        values.push_back(object.second);
    }
}

template <typename T, typename U>
bool Dictionary<T,U>::has(T targetKey) const{
    for (T currentKey : keys){
        if (currentKey == targetKey){
            return true;
        }
    }
    return false;
}

template <typename T, typename U>
void Dictionary<T,U>::add (T key, U value){
    keys.push_back(key);
    values.push_back(value);
}

template <typename T, typename U>
U Dictionary<T,U>::operator[] (T key){
    unsigned int pos = std::find(keys.begin(), keys.end(), key) - keys.begin();
    return values[pos];
}

#endif // DICTIONARY_H_INCLUDED

The representation (as a parallel pair of vectors) isn't as good as the alternative (a single vector of pairs), because:

any time the vectors have to grow, there's two separate allocations rather than just one, and
whenever they are used, the data may be in different regions of memory (the code has poorer spatial locality).

Consider instead writing

using value_type = std::pair<T, U>;
std::vector<value_type> entries;

This will change the interface a little - in particular, begin() and end() will now give iterators that access these pairs.  That's more consistent with std::map interface, so I recommend that.

We can reduce duplication a little: in the initializer-list constructor, we can simply call add() for each element rather than re-writing its body:

template <typename T, typename U>
Dictionary<T,U>::Dictionary (std::initializer_list<std::pair<T,U>> store)
{
    for (std::pair<T,U> object : store) {
        add(object.first, object.second);
    }
}

On the other hand, if we've changed our representation as above, we simply have

template <typename T, typename U>
Dictionary<T,U>::Dictionary (std::initializer_list<std::pair<T,U>> store)
{
    for (std::pair<T,U> object : store) {
        entries.push_back(object);
    }
}

The indexing operator is surprising.  If key doesn't exist in the dictionary, then we have undefined behaviour (accessing values out of range).  That's a valid choice, but worthy of a comment (particularly as it diverges from the Standard Library interface).  Since it doesn't create as necessary, and only ever returns a copy of the value (rather than a reference) it probably ought to be declared const.

We should probably check whether key already exists in add().  We can then either ignore it, or update the existing value.  What we have at the moment is the worst possible choice - we add data that can never be used, which only serves to slow future operations.

Some other useful accessors are missing - constant and/or reverse iterators spring to mind as an obvious example, as do methods of removing values.  A swap() member would also be very useful.

The has function can be simplified a bit with the use of a standard algorithm: std::any_of() can replace the loop.

Algorithmic complexity

Our operator[] uses linear search with std::find().  Whilst this works, it will get slower in proportion to the number of elements contained - in "big O" notation, we say that it scales as O(n).  Once we have the interface as we we want, we'll then want to look at changing the data structure to improve look-up times (and we might need to trade against insertion speed).  The good news is that storing items as pairs will work well with whatever storage we choose, and it will be much easier to change the implementation than with separate storage of keys and values.

It may well be worth moving to pair-based storage, and implementing the other review items, and then returning for a performance review of the new code.


uplifitng return value error reporting to Exception based error reporting In Framework Design guideline book there is a chapter about Exception and they talk about return-value-based error reporting and exception based error reporting and the fact that we in a O.O language like C# we should avoid return-value-based error reporting and use exceptions. With that in mind I was looking at our code that eight years ago was written in Visual Basic and last year with a automatic tool got converted to C#!

So here is a method I am looking at, I was wondering if the advice from that book applies to such a method and if yes, then what would be a better approach for rewriting this method?

public int Update(CaseStep oCaseStepIn)
{
    int result = 0;
    //Update the master object with the passed in object

    result = UCommonIndep.gnUPDATE_FAILED;
    if (Validate(oCaseStepIn) == UCommonIndep.gnVALIDATE_FAILED)
    {
        return result;
    }

    CaseStep oCaseStep = get_ItemByObjectKey(oCaseStepIn.CopyOfObjectKey);
    if (oCaseStep == null)
    {
        return result;
    }

    if (oCaseStep.Update(oCaseStepIn) == UCommonIndep.gnUPDATE_SUCCESSFUL)
    {
        //*******************************
        //FYI - Insert code here to update any Key values that might have changed.
        result = UCommonIndep.gnUPDATE_SUCCESSFUL;
    }
    return result;
}

Yes, the refactoring "Replace error codes with exceptions" work perfectly here.
As a first step you need to create an exception type that suits your need, something like

public class UpdateException : Exception
{
    //your custom properties, fields etc.
}

and then use it in your method (an example below)

public void Update(CaseStep oCaseStepIn)
{
    //Update the master object with the passed in object

    if (Validate(oCaseStepIn) == UCommonIndep.gnVALIDATE_FAILED)
        throw new UpdateException(); //pass in constructor useful messages

    CaseStep oCaseStep = get_ItemByObjectKey(oCaseStepIn.CopyOfObjectKey);
    if (oCaseStep == null)
        throw new UpdateException(); //pass in constructor useful messages

    if (oCaseStep.Update(oCaseStepIn) != UCommonIndep.gnUPDATE_SUCCESSFUL)
        throw new UpdateException(); //pass in constructor useful messages

    //*******************************
    //FYI - Insert code here to update any Key values that might have changed.
}

You can create different Exception subclasses for each type of error. It's even better.


Monster Buff/Debuff System Design Below are a collection of classes that compose my buff/debuff system. The largest flaw in my design that I couldn't overcome is the base class Effect having an enum filled with the different types of buffs and debuffs. If there is a workaround that could clean up that part of the code I'd be really grateful. Otherwise, I'm simply looking for suggestions on potential bugs, mishaps, inefficient methods, etc.

The base class for Effects

public abstract class Effect {

    public static enum Type { NIL, ATTACKBUFF, ATTACKBREAK, DEFENSEBUFF, DEFENSEBREAK }

    private Effect.Type type;

    public Effect(Effect.Type type) {

        this.type = type;

    }

    public abstract void apply();
    public abstract void expire();
    public abstract void update();

    public static Effect newEffect(Effect.Type type, Monster monster, int duration) {

        Effect effect;

        switch(type) {
        default:
        case NIL:
            effect = new NilEffect();
            break;
        case ATTACKBUFF:
            effect = new AttackBuff(monster, duration);
            break;
        case ATTACKBREAK:
            effect = new AttackBreak(monster, duration);
            break;
        case DEFENSEBUFF:
            effect = new DefenseBuff(monster, duration);
            break;
        case DEFENSEBREAK:
            effect = new DefenseBreak(monster, duration);
            break;
        }

        return effect;

    }

    public Effect.Type getType() {

        return type;

    }

}

A class for specific StatModifiers

abstract class StatModifier extends Effect {

    protected final Monster monster;
    protected final Stat originalStat;
    protected final Stat.Type statType;
    protected final double modifier;

    private int duration;

    public StatModifier(Monster monster, Effect.Type effectType, Stat.Type statType, double modifier, int duration) {

        super(effectType);

        this.monster = monster;
        this.statType = statType;
        originalStat = monster.getStat(statType);
        this.modifier = modifier;
        this.duration = duration;

    }

    @Override
    public void apply() {

        final int augmentedBaseStatValue = (int) Math.ceil(originalStat.getAugmentedStat() * modifier);
        final Stat stat = new Stat(augmentedBaseStatValue, 0, 0);
        monster.setStat(statType, stat);

    }

    @Override
    public void expire() {

        monster.setStat(statType, originalStat);

    }

    @Override
    public void update() {

        duration--;

        if(duration <= 0) {

            expire();

        }

    }

}

An example of a StatModifier class

public final class AttackBuff extends StatModifier {

    public AttackBuff(Monster monster, int duration) {

        super(monster, Effect.Type.ATTACKBUFF, Stat.Type.ATTACK, 1.5 ,duration);

    }

}

The Monster class (partial)

public class Monster {

    private final Map<Effect.Type, Effect> effects;

    public Monster() {

        effects = new HashMap<>();

    }

    public void addEffect(Effect.Type type, int duration) {

        if(effects.containsKey(type)) {

            effects.get(type).expire();
            effects.remove(type);

        }

        Effect effect = Effect.newEffect(type, this, duration);
        effect.apply();
        effects.put(effect.getType(), effect);

    }

    public void updateEffects() {

        effects.values()
               .parallelStream()
               .forEach(effect ->

                   effect.update()

               );

    }

}

Visibility modifiers and names

enum types are by default public static. You can choose to remove them.

Type is a very broad name for your enum, so you can consider just calling it EffectType.

switch and return

Instead of assigning the result to effect, you should return directly from each case, i.e.:

switch(type) {
case ATTACKBUFF:
    return new AttackBuff(monster, duration);
case ATTACKBREAK:
    return new AttackBreak(monster, duration);
case DEFENSEBUFF:
    return new DefenseBuff(monster, duration);
case DEFENSEBREAK:
    return new DefenseBreak(monster, duration);
// shifted default case as the last, it's more 'conventional'
default:
// case NIL:
    return new NilEffect();
}

You can also choose to place this as method for EffectType instead, but from an OOP approach, this depends if an EffectType is supposed to 'know' an Effect (and Monster) or not. Your approach is still fine.

Adding and updating effects

Since you are on Java 8, you can consider return-ing null from Map.computeIfPresent() to expire the type and remove the entry in one go:

public void addEffect(Effect.Type type, int duration) {
    effects.computeIfPresent(type, (k, effect) -> { effect.expire(); return null; });
    Effect effect = Effect.newEffect(type, this, duration);
    effect.apply();
    effects.put(effect.getType(), effect);
}

Do you really need to invoke parallelStream() when updating effects? Since the maximum number of elements you can have in your Map is bounded by the number of EffectType values (BTW, you may want to switch to an EnumMap), I think the overhead of doing a concurrent access on your Map is not required. Also the forEach() operation can be done using a method reference as well, in this case:

public void updateEffects() {
    effects.values().stream().forEach(Effect::update);
}


Iterating through list and calling method with different parameters for each item I am iterating over a list of environments but calling the same method each time. The only difference is the parameters are changing depending on what environment it is. The result so far is similar to the code below, i know this is not great but cant find a better way to solve it. Any ideas?

       foreach (var environment in Environments)
        {
            if(environment == "environment1")
            {
                FillList("connectionstring1", query1);
            }
            else if (environment == "environment2")
            {
                FillList("connectionstring1", query2);
            }
            else if (environment == "environment3")
            {
                FillList("connectionstring2", query1);
            }
        }

The second parameter has the same value twice (query1) the first parameter also has the same value twice ("connectionstring1"), this means you can remove 1 if statement if you create 2 variables with initial values like this:

foreach (var environment in Environments)
{
    string connectionString = "connectionstring1";
    var query = query1;
    if (environment == "environment2")
    {
        query = query2;
    }
    else if (environment == "environment3")
    {
        connectionString = "connectionstring2";
    }
    FillList(connectionString, query);
}

Another option would be to add Dictionary<string, Action> which will allow you to easily map string -> method call like this:

Dictionary<string, Action> dictionary = new Dictionary<string, Action>
{
    ["environment1"] = () => FillList("connectionstring1", query1),
    ["environment2"] = () => FillList("connectionstring1", query2),
    ["environment3"] = () => FillList("connectionstring2", query1)
};
foreach (var environment in Environments)
{
    dictionary[environment].Invoke();
}


Formatting a birthday in years def age_post(post) do

    birthday = Timex.date({post.birthday_year, post.birthday_month, post.birthday_day})
    now = Date.now

    years = Timex.diff(now, birthday, :years)

    Integer.to_string(years) <> " ans"

  end

I'm quite new with the elixir language, I'm pretty sure you can get the same result than that code in one line.

You could cut out some of those intermediate variables using the pipeline operator: |>

def age_post(post) do
  birthday = Timex.date({post.birthday_year, post.birthday_month, post.birthday_day})

  (Date.now |> Timex.diff(birthday, :years) |> Integer.to_string) <> " ans"
end


Internal function that determines when to resize by width or height in a Image Resizer utility Description:

Given an image X with known dimensions. We call a method CreateThumbnail to make a thumbnail (or not) with a given max-width and max-height (both optional).

CreateThumbnail call ComputeSize method (bad name?) which is responsable for doing the following logic:

If no max-width/max-height, the width and height should return as is.
If only one is provided, then we call a method ResizeWidth on the Dimention2D object to return a new dimension based on the new width size (aspectRation is kept).

Same applies for height.

If both are provided, use the one, that while keeping the aspect ration, won't be bigger than their corresponding maximum.

If the image dimension are smaller than the maximum, no change to the dimensions.

Then, with the new size, the Resize is actually applied (or not) (this is outside the scope of this method).

private Dimention2D ComputeSize(Image image, int maxWidth, int maxHeight)
{
    if (image == null) throw new ArgumentNullException(nameof(image));
    var sourceDimension = new Dimention2D(image.Width, image.Height);

        bool computeWidth = maxWidth > 0 && sourceDimension.Width > maxWidth;
        bool computeHeight = maxHeight > 0 && sourceDimension.Height > maxHeight;

    if (computeWidth && computeHeight)
    {
        if (sourceDimension.Width / maxWidth > sourceDimension.Height / maxHeight)
            computeHeight = false;
        else
            computeWidth = false;
    }

    if (computeWidth)
        return sourceDimension.ResizeWidth(maxWidth);

    else if (computeHeight)
        return sourceDimension.ResizeHeight(maxHeight);

    else return sourceDimension;
}

I'm open to any suggestion.
Variables/methods names, the use (or lack of) { and } on if/else, the if/else. and whatever thing you can think of making it prettier.

You can replace this:

if (computeWidth)
    return sourceDimension.ResizeWidth(maxWidth);

else if (computeHeight)
    return sourceDimension.ResizeHeight(maxHeight);

else return sourceDimension;

with this:

if (computeWidth)
    return sourceDimension.ResizeWidth(maxWidth);
if (computeHeight)
    return sourceDimension.ResizeHeight(maxHeight);
return sourceDimension;

I would rename the method and input parameters, as it would add more sense to the method name and input parameters.

private Dimention2D  ReSize(Image image, int requireWidth, int requireHeight);


Stop Watch Application 2.0 After the tips from my previous review. I have come up with these changes:

Changed my braces to start on a new line
Used the Single Responsibility Principle
Return early whenever possible
Use a namespace that doesn't start with an underscore

I'm not entirely sure I have used the namespace correctly. If it's wrong, can you please give me a push in the right direction? Any other comments would be appreciated.

namespace Difficult.StopWatch0002
{
    public partial class StopWatchForm : Form
    {
    StateEnum state;
    Stopwatch stopWatch;
    TimeSpan timeSpan;
    string elapsedTime;
    List<string> times = new List<string>();

    public StopWatchForm()
    {
        InitializeComponent();
        state = StateEnum.Stoped;
    }

    private void StartAndLapButton_Click(object sender, EventArgs e)
    {
        if (state == StateEnum.Stoped)
        {
            stopWatch = Stopwatch.StartNew();
            state = StateEnum.Started;
            ChangeButtonText(StartAndLapButton, "Lap");
            ChangeFormTitle("Started");
            timer1.Enabled = true;
        } else if (state == StateEnum.Started)
        {
            times.Add(elapsedTime);
        }
    }

    private void StopButton_Click(object sender, EventArgs e)
    {
        if (state == StateEnum.Started)
        {
            ChangeButtonText(StartAndLapButton, "Start");
            ChangeFormTitle("Stop Watch");
            stopWatch.Stop();
            times.Add(elapsedTime);
            state = StateEnum.Stoped;
            timer1.Enabled = false;
        }
    }

    private void FileButton_Click(object sender, EventArgs e)
    {
        if (!times.Any())
        {
            return;
        }
        var filename = PromtSaveAsFileName();
        if (filename == null)
        {
            return;
        }
        WriteResultsToFile(filename);
        times.Clear();
    }

    private void WriteResultsToFile(string filename)
    {
        System.IO.File.WriteAllLines(filename, times);
    }

    private string PromtSaveAsFileName()
    {
        var dialog = new SaveFileDialog
        {
            DefaultExt = "txt",
            Filter = "txt files (*.txt)|*.txt|All files (*.*)|*.*",
            FilterIndex = 2,
            RestoreDirectory = true
        };

        if (dialog.ShowDialog() == DialogResult.OK)
        {
            return dialog.FileName;
        }

        return null;
    }

    void ChangeButtonText(Button button, string text)
    {
        button.Text = text;
    }

    private void ChangeFormTitle(string text)
    {
        this.Text = text;
    }

    private void timer1_Tick(object sender, EventArgs e)
    {
        timeSpan = stopWatch.Elapsed;
        elapsedTime = String.Format("{0:00}:{1:00}:{2:00}", timeSpan.Hours, timeSpan.Minutes, timeSpan.Seconds);
        TimeKeeperText.Text = elapsedTime;
    }
}
}

Use Correct Types in the Model

This is your model:

Stopwatch stopWatch;
TimeSpan timeSpan;
string elapsedTime;
List<string> times = new List<string>();

This is an example of stringly typed programming. Store data in meaningful types and convert them to other types (string, byte[] etc) for transfer, display etc. as necessary. For example storing times as List<TimeSpan> you can convert this to a list of running totals or display a graph etc without parsing, or worse trying to parse, the strings back to a meaningful type.

Looking from another perspective, you model should depend on the requirements, and as little else as possible. reading the problem description, and not bothering with menial details, thus not putting the cart before the horse,  you would come up with some model like this:

Stopwatch stopWatch; // from (start lap stop) requirement
List<TimeSpan> times = new List<TimeSpan>(); // from (write to file) requirement

Separation of Concerns

We see here several consequences of a user action bunched up:

stopWatch = Stopwatch.StartNew();
state = StateEnum.Started;
ChangeButtonText(StartAndLapButton, "Lap");
ChangeFormTitle("Started");
timer1.Enabled = true;

Suppose you decided to change one aspect, say how times are stored, then you will have to look for every code block that refers to it, and try to judge whether the change you will make affect the behavior of the code in the same code block below the changes.

Another way of organizing things is the OO way, group data and related behavior together:

event Action OnStart = () => {};
event Action OnLap = () => {};
event Action OnStop = () => {};

void ConfigureStopWatchBehavior()
{
    OnStart += stopWatch.Restart;
    OnStop += stopWatch.Stop;
}

void ConfigureTimesBehavior()
{
    Action storeElapsedTime = () => times.Add(stopWatch.Elapsed);
    OnLap += storeElapsedTime;
    OnStop += storeElapsedTime;
}

void ConfigureViewBehavior()
{
    OnStart += () => {
        StartAndLapButton.Text = "Lap";
        this.Text = "Started";
    };

    OnStop += () => {
        StartAndLapButton.Text = "Start";
        this.Text = "Stop Watch";
    };
}

void ConfigureViewStateBehavior()
{
    OnStart += () => state = StateEnum.Started;
    OnStop += () => state = StateEnum.Stoped;
}

void ConfigureTimerBehavior()
{
    OnStart += timer1.Start;
    OnStop += timer1.Stop;
}

Which causes form event handlers to look like these:

private void StartAndLapButton_Click(object sender, EventArgs e)
{
    if (state == StateEnum.Stoped) OnStart();
    else if (state == StateEnum.Started) OnLap();
}

private void StopButton_Click(object sender, EventArgs e)
{
    if (state == StateEnum.Started) OnStop();
}


Is there a better way than lambda to filter on attributes/methods of objects? Is there a better (more pythonic?) way to filter a list on attributes or methods of objects than relying on lamda functions?

contexts_to_display = ...
tasks = Task.objects.all()
tasks = filter(lambda t: t.matches_contexts(contexts_to_display), tasks)
tasks = filter(lambda t: not t.is_future(), tasks)
tasks = sorted(tasks, Task.compare_by_due_date)

Here, matches_contexts and is_future are methods of Task.  Should I make those free-functions to be able to use filter(is_future, tasks)?

Any other comment?

I would use a list comprehension:

contexts_to_display = ...
tasks = [t for t in Task.objects.all()
         if t.matches_contexts(contexts_to_display)
         if not t.is_future()]
tasks.sort(cmp=Task.compare_by_due_date)

Since you already have a list, I see no reason not to sort it directly, and that simplifies the code a bit.

The cmp keyword parameter is more of a reminder that this is 2.x code and will need to be changed to use a key in 3.x (but you can start using a key now, too):

import operator
tasks.sort(key=operator.attrgetter("due_date"))
# or
tasks.sort(key=lambda t: t.due_date)

You can combine the comprehension and sort, but this is probably less readable:

tasks = sorted((t for t in Task.objects.all()
                if t.matches_contexts(contexts_to_display)
                if not t.is_future()),
               cmp=Task.compare_by_due_date)


Sorting Each Entry (code review + optimization) I have a requirement where there are 3 selection criteria user selects countries from a map displayed as lets say in 3 categories , i am maintaining 3 arraylist for the same.Each time he selects a country i have to display list of country names displayed in Alphabetical order in a bottom bar with font color distinguishing each category .I have written a code which will add all the values of arraylist in a different arraylist and sort it and then based on category it will create font for each and display them in the bottom so each time user  selects a country this method is called.Please review and let me know if i can improve the performance.Below is  the code

Lets say issuingOffice,selectedCountries and deSelectedCountries are arraylist declared in global scope and have values.

function updateBottomBar(){
    var initalContent="Countires: ";
    require(["dojox/collections/ArrayList"],function(arrayList){
    var bottomBar = new arrayList();
    var it = issuingOffice.getIterator();
    while (!it.atEnd()) {
        var country = it.get();
        bottomBar.add(country);
    }
    var scit = selectedCountries.getIterator();
    while (!scit.atEnd()) {
        var country = scit.get();
        bottomBar.add(country);
    }
    var descit = deSelectedCountries.getIterator();
    while (!descit.atEnd()) {
        var country = descit.get();
        bottomBar.add(country);
    }
    bottomBar.sort();
    var bbit = bottomBar.getIterator();
    while (!bbit.atEnd()) {
        var country = bbit.get();
        if(selectedCountries.contains(country)){
            initalContent!=""+country+"";
        }else if(selectedCountries.contains(country)){
            initalContent!=""+country+"";
        } else{
            initalContent!=""+country+"";
        }
    }
    initalContent+="";
    dojo.byId("displayCountryName")=initalContent;
    });
}

There are many improvements that can be done, but here are a few things I identified:

I wouldn't have the require call within the function itself. I would rather create a top-level module with all listed dependencies. The updateBottomBar function would be defined in this module:

require(["dojox/collections/ArrayList"/*, other deps...*/], function(ArrayList) {
    function updateBottomBar() {
        //...
    }
});

Have a look at https://dojotoolkit.org/documentation/tutorials/1.8/modules/

It's very inefficient to merge the multiple arrays every-time. You should rather maintain an additional array that stays in sync with the others. When a new item is pushed in one of the other arrays, you reflect the changes in that additional array directly (same logic with removal). However, if you can, I would rather have a single collection to maintain and use a discriminator such as a category property to categorize items, so instead of holding strings, the collection would contain { country: String, category: String } objects.
With your current implementation, you also throw away some useful information that you need to recompute later. For example if(selectedCountries.contains(country)) shouldn't be necessary since you had that knowledge when you pushed the items in the bottomBar list. Instead of pushing a simple string, push an object like proposed in point #2. Then you will be able to check if the items belongs to the selected category in O(1) time by doing something like:

switch (country.category) {
    'selected': //do something break;
}

Concatenating multiple strings with the + operator isin't the most efficient way. Instead you should push your strings in an array and then use theArray.join('') to perform the concatenation.

Also note that you can create DOM elements dynamically with document.createElement and make the process efficient by using document fragments (document.createDocumentFragment).
You have a typo -> var initalContent="Countires: <label>";
initalContent != ... Did you mean += ?


Improving the performance of interpolating between two arrays The Problem

I am currently writing a script that converts images into numerical array representation and then calculates "in-between" images based on linear interpolation between the start and end array.

My code does exactly what I want but goes over many nested loops which strikes me as something that will lead to very high computation times for many interpolation steps or big images.

The Code

The code is in python

import numpy as np

# Helper function that calculates the interpolation between two points
def interpolate_points(p1, p2, n_steps=3):
    # interpolate ratios between the points
    ratios = np.linspace(0, 1, num=n_steps)
    # linear interpolate vectors
    vectors = list()
    for ratio in ratios:
        v = (1.0 - ratio) * p1 + ratio * p2
        vectors.append(v)
    return np.asarray(vectors)

# final function that interpolates arrays
def interpolate_arrays(start_array,end_array,n_steps=10):
    n = 0
    array_interpolation = []
    while n < n_steps:
        i = 0
        x = []
        while i < len(start_array):
            e = interpolate_points(start_array[i],end_array[i],n_steps)[n]
            x.append(e)
            i += 1
        array_interpolation += [x]
        n += 1
    return array_interpolation

This results in:

#Test
X1 = [1,1]
X2 = [3,3]

interpolate_arrays(X1,X2,n_steps=3)
#[[1.0, 1.0], [2.0, 2.0], [3.0, 3.0]]

There are some easy wins here. Your interpolate_points doesn't need a loop:

def interpolate_points(p1, p2, n_steps=3):
    """Helper function that calculates the interpolation between two points"""
    # interpolate ratios between the points
    ratios = np.linspace(0, 1, num=n_steps)
    # linear interpolate vectors
    vectors = (1.0 - ratios) * p1 + ratios * p2
    return vectors

Also, even without further vectorization, you should be making use of range in your main function:

def interpolate_arrays(start_array, end_array, n_steps=10):
    """final function that interpolates arrays"""
    array_interpolation = []
    for n in range(n_steps):
        x = []
        for i in range(len(start_array)):
            e = interpolate_points(start_array[i], end_array[i], n_steps)[n]
            x.append(e)
        array_interpolation += [x]
    return array_interpolation

However, all of that can be replaced with a call to interp1d:

import numpy as np
from scipy.interpolate import interp1d

def interpolate_arrays(bounds, n_steps=10):
    """final function that interpolates arrays"""
    bounds = np.array(bounds)

    fun = interp1d(
        x=[0, 1],
        y=bounds.T,
    )
    y = fun(np.linspace(0, 1, n_steps))

    return y

def test():
    X1 = [1.5, 1]
    X2 = [5.5, 3]

    y = interpolate_arrays([X1, X2], n_steps=3)
    assert y.T.tolist() == [[1.5, 1.0], [3.5, 2.0], [5.5, 3.0]]

Even easier:

def interpolate_arrays(X1, X2, n_steps=10):
    """final function that interpolates arrays"""
    return np.linspace(X1, X2, n_steps)

def test():
    X1 = [1.5, 1]
    X2 = [5.5, 3]

    y = interpolate_arrays(X1, X2, n_steps=3)
    assert y.tolist() == [[1.5, 1.0], [3.5, 2.0], [5.5, 3.0]]

Notes:

If you use interp1d, it would be better if your inputs and outputs are both two-dimensional np.ndarray; in their current form they need a transposition
Write some unit tests such as the one shown, although it would be a better idea to call isclose since this is floating-point math
If you want, it is trivial to make this extrapolate as well as interpolate

Basically: if there is a math thing in your head, before even thinking about what it would take to implement it yourself, do a search through scipy/numpy to see if it has already been done for you.


Asking for a number in Java Swing Is it good way of asking for a number? Should that function return 0 if there is an error?

private int getNumberDialog(String message) {
            try {
                String resultInString = JOptionPane.showInputDialog(message);
                int result = Integer.parseInt(resultInString);
                return result;
            } catch (NumberFormatException e) {
                JOptionPane.showMessageDialog(null, "You didn't gave a number");
            }
            return 0;
        }

Avoid using "special return values" in code for when errors occur. It should be up to the calling code how to handle the situation.

I would let the getNumberDialog utility function re-throw the exception:

private int getNumberDialog(String message) throws NumberFormatException {
    String resultInString = JOptionPane.showInputDialog(message);
    int result = Integer.parseInt(resultInString);
    return result;
}

And in your usage code, you catch the exception:

try {
    int times = getNumberDialog("How many times?");
    game.playForMeNrTimes(times);
} catch (NumberFormatException ex) {
    JOptionPane.showMessageDialog(null, "You didn't gave a number");
}

The key here is that the utility-function getNumberDialog doesn't know about any context. It doesn't know how the returned value will be used, so how can it possibly decide on a default value to use in the case of an error?


Insert a node at a specific position in a Linked List This code is built to insert a Node into a specific position of a linked-list. It passes all the test cases for the interview, but I was wondering about the coding style regarding techniques, and especially variable naming, and any other best-practices you can help with.

Node* InsertNth(Node *head, int data, int position)
{
    // Complete this method only
    // Do not write main function.
    Node * sentinel = new Node();
    sentinel->data = -1;
    sentinel->next = head;
    Node* node = sentinel;
    int i = 0;
    while (node->next && i < position) {
        node = node->next;
        i++;
    }

    Node* preNext = node->next;
    Node* newNode = new Node();
    newNode->data = data;
    newNode->next = preNext;
    node->next = newNode;
    head = sentinel->next;
    delete sentinel;

    return head;

}

This is how I would have done it:

Node* InsertNth(Node *head, int data, int position)
{
    // Use a sentinel as this leads to simpler code
    // and no need to test for the empty list case.
    Node sentinel{-1, head};

    // Find the position we want to insert in.
    // Note: If there are less than `position` elements then
    //       the new element will be added to the end.
    Node* current = &sentinel;
    for(; position != 0 && current->next;--position, current = current->next)
    {}

    // Create the new Node and insert it into the chain.
    Node* newNode = new Node{data, current->next};
    current->next = newNode;

    // Update head.
    // In the case of null list or inserting at position 0
    // this will do something. Otherwise this is a no-op.
    head = sentinel.next;

    return newNode;
}

Notes:

Make notes of assumptions you have made in the comments.
I like the for() expression over the while() expression.
It expresses my intent more clearly (ie that I am just chaining along the list).
No need to dynamically create the sentinel. By declaring it on the stack. It works better.
I have assumed node has a reasonable constructor to make the code cleaner.