Do you think this design pattern is good for game portal content management system I am developing game portal content management system using java and j2ee technology. However i am stuck in my design implementation.

Here is the scenario:

Each Game object has attributes like id, name, developer, price, platformType, platformName and so on...

However the release date may be different for each platform - PC, Mobile, Console and so on.
Of course it is the same with the price.

The other problem is that for example if the game is for PC platform it has requirements like RAM, Video card and so on. So I decided to make another class for the platformType.

Do you think that the following design is good?

Here is the main Game object class:

public class Game
{
   private long systemId;
   private String name;
   private String developer;
   private Date registerDate;
   private ArrayList<GamePlatform> gamePlatforms;
   private GameScore gameScore;
   private GameRank gameRank;
   private ArrayList<String> genre;
}

Here is the GamePlatform class and its subclasses

public abstract class GamePlatform
{
   private Date releaseDate;
   private int price;
   private String name;
   private PlatformType platformType;

   public GamePlatform(Date releaseDate, int price, String name, PlatformType  platformType) {

       this.releaseDate = releaseDate;
       this.price = price;
       this.name = name;
       this.platformType = platformType;
   }

   public abstract PlatformType getType();
}

child class 1

public class ConsolePlatform extends GamePlatform
{

   public ConsolePlatform(Date releaseDate, int price, String name, PlatformType platformType) {
       super(releaseDate, price, name, platformType);
   }

   @Override
   public PlatformType getType()
   {
       return PlatformType.CONSOLE;
   }

}

child class 2

public class MobilePlatform extends GamePlatform
{

public MobilePlatform(Date releaseDate, int price, String name, PlatformType platformType)
   {
       super(releaseDate, price, name, platformType);
   }

   @Override
   public PlatformType getType()
   {
       return PlatformType.MOBILE;
   }

child class 3

public class PCPlatform extends GamePlatform
{

   private PCRequirements pcRequirements;

   public PCPlatform(Date releaseDate, int price, String name, PlatformType platformType, PCRequirements pcRequirements)
   {
       super(releaseDate, price, name,platformType);
       this.pcRequirements = pcRequirements;
   }

   public PCRequirements getPcRequirements() {
       return pcRequirements;
   }

   public void setPcRequirements(PCRequirements pcRequirements) {
       this.pcRequirements = pcRequirements;
   }

   @Override
   public PlatformType getType()
   {
       return PlatformType.PC;
   }

}

And finally i have Enum class for the platform types. Here it is:

public enum PlatformType
{
   MOBILE
   {
       @Override
       public String toString(){ return "Mobile";}
   },
   CONSOLE
   {
       @Override
       public String toString(){ return "Console";}
   },
   PC
   {
       @Override
       public String toString(){ return "PC";}
   };
}

ArrayList<...> reference types should be simply List<...>. See: Effective Java, 2nd edition, Item 52: Refer to objects by their interfaces

List<GamePlatform> gamePlatforms;

I'd consider using BigDecimal instead of int price. It can store numbers like $1.99.
I'd rename PCPlatform to PcPlatform. From Effective Java, 2nd edition, Item 56: Adhere to generally accepted naming conventions:

  While uppercase may be more common,
  a strong argument can made in favor of capitalizing only the first
  letter: even if multiple acronyms occur back-to-back, you can still
  tell where one word starts and the next word ends.
  Which class name would you rather see, HTTPURL or HttpUrl?

The PlatformType enum seems as a duplication here and if I'm right it's purpose is to use it in switch-case statements. Consider polymorphism here. I'd put a public abstract String getPlatformName() method in the GamePlatform class and remove PlatformType completely. Two might be useful reading:

Refactoring: Improving the Design of Existing Code by Martin Fowler: Replacing the Conditional Logic on Price Code with Polymorphism
Replace Conditional with Polymorphism

Constructor parameters should be validated. Does it make sense to create a GamePlatform with null or empty string name? If not, check it and throw a NullPointerException or an IllegalArgumentException. (Effective Java, Second Edition, Item 38: Check parameters for validity)


Coconut, Sailors, and Monkeys Problem

There was an interesting problem on the dailyprogrammer subreddit:

  A number of sailors (let's call it N) are stranded on an island with a
  huge pile of coconuts and a monkey. During the night, each sailor (in
  turn) does the following without the others knowing:

  He takes one N'th (e.g. if N=5, one fifth) of the coconuts in the pile and hides them
  The division leaves one coconut left over, which is given to the monkey.

  In the morning, they split the remaining coconuts between them. This
  time the split is even. There's nothing left over for the monkey.

  Your task: Given the number of sailors (N), how many coconuts were in
  the pile to begin with (lowest possible number)?

I was hoping to get feedback on my solution, and possibly suggestions as to better implementation.

I am not terribly satisfied with my use of a potential infinite while loop (replace with a for and an upper bound - if so, what's the upper bound)?
Should I type cast to double when I calculate the multiplier value?

Example

The solution for 2 sailors is 11 coconuts.
11 => 11 / 2 (since first sailor takes 1/2 of coconuts) => 5 remainder 1 (which goes to the monkey)
5 => 5 / 2 (since seconds sailor takes 1/2 of remaining coconuts) => 2 remainder 1 (which again goes to the monkey)
2 can be split evenly between the 2 sailors

Implementation

public class CoconutCalculatorImpl implements CoconutCalculator {
    @Override
    public double calculateCoconuts(final long sailors) {
        long factor = 1;
        final double multiplier = (double) sailors / (double) (sailors - 1);
        while (true) {
            double baseValue = (factor * sailors);
            long counter = 0;
            while ((0 == baseValue % (sailors - 1)) & (counter < sailors)) {
                baseValue = (baseValue * multiplier) + 1;
                counter++;
            }

            if (counter == sailors) {
                return baseValue;
            }

            factor++;
        }

    }
}

As you are dealing with whole numbers and remainders do not use floating point numbers. It's inaccurate and furthermore confusing because others may think that the result may be not a whole number.
As the solutions will get very high very fast consider to use BigInteger
During calculation make sure your divisions will always be without rest
Avoid return statements within the loop. Programmers have to search for breaking conditions through the code if you do not provide them in the loop header. Beside readability unexpected return, break or continue statements cause serious problems when extending the code or applying refactorings like "extract method".
Use lazy evaluation of boolean operators (&& instead of &). In complex evaluations you give other developers the chance to count on the evaluation and produce side effects.
Separate responsibilities. Currently two algorithms are fighting against each other. One is providing a factor and the other is using the factor to try a calculation. But maybe the second algorithm is not satisfied by the factor what is evaluated during calculation. This should be modelled explicitly.

I am not a mathematician. Maybe there is a more efficient way (a shortcut) to calculate it. But I won't cover that.

So here is a refactored version. It's more verbose but it has less accuracy problems and shows the responsibilites of the two algorithms. They communicate through a result object that contains the calculated base value so far and the information if the method was satisfied by the factor given so the base value is a valid result.

    public static BigInteger calculateCoconuts(final long sailors) {

        long factor = 1;

        CalculationResult calculationResult = null;

        do {

            calculationResult = tryCoconutCalculation(sailors, factor);
            factor++;

        } while (!calculationResult.isSatisfied());

        return calculationResult.getBaseValue();

    }

    private static CalculationResult tryCoconutCalculation(final long sailors, long factor) {

        BigInteger factorBI = BigInteger.valueOf(factor);
        BigInteger sailorsBI = BigInteger.valueOf(sailors);
        BigInteger baseValueBI = factorBI.multiply(sailorsBI);

        long counter = 0;

        boolean satisfied = baseValueBI.remainder(sailorsBI.subtract(BigInteger.ONE)).equals(BigInteger.ZERO);

        while (satisfied && (counter < sailors)) {
            baseValueBI = baseValueBI.divide(sailorsBI.subtract(BigInteger.ONE)).multiply(sailorsBI).add(BigInteger.ONE);
            counter++;
            if (counter < sailors) {
                satisfied = baseValueBI.remainder(sailorsBI.subtract(BigInteger.ONE)).equals(BigInteger.ZERO);
            }
        }

        return new CalculationResult(baseValueBI, satisfied);
    }

    public static class CalculationResult {

        private BigInteger baseValue;
        private boolean satisfied;

        public CalculationResult(BigInteger baseValue, boolean satisfied) {
            super();
            this.baseValue = baseValue;
            this.satisfied = satisfied;
        }

        public BigInteger getBaseValue() {
            return baseValue;
        }

        public boolean isSatisfied() {
            return satisfied;
        }

    }


Optimize a generic foreach method that converts Datatable to my object using Reflection I need to optimize this code block below, this method converts a Datatable object to the Object that I am passing by parameter, in another words, this serializes the object, however I have to run this block 1 million times when reading data from the database, so this is the bottleneck of my application.

  Obs: All my objects that I use in this method have a property "Id"
  which is the PrimaryKey of the table, but the name of the primary key
  in the database is different, so that's why I have that "if command"
  asking the name of the primary key.

How can I optimize this code below?

public static List<T> BindDataList<T>(DataTable dt, string primaryKey)
{
    // Get all columns' name
    var columns = (from DataColumn dc in dt.Columns select dc.ColumnName);

    var properties = typeof(T).GetProperties().ToList();

    var lst = new List<T>();

    foreach (DataRow dr in dt.Rows)
    {
        // Create object
        var ob = Activator.CreateInstance<T>();

        // Get all properties
        properties.ForEach(propertyInfo =>
        {
            if (columns.Any(s => s.Equals(propertyInfo.Name, StringComparison.OrdinalIgnoreCase)))
            {
                // Fill the data into the property
                if (!(dr[propertyInfo.Name] is DBNull))
                    propertyInfo.SetValue(ob, dr[propertyInfo.Name]);
            }
            else
            {
                if (propertyInfo.Name.ToUpper().Equals("ID"))
                {
                    // Fill the data into the property
                    if (!(dr[primaryKey] is DBNull))
                    {
                        propertyInfo.SetValue(ob, dr[primaryKey].SafeToInt());
                    }
                }
            }
        });

        lst.Add(ob);
    }
    return lst;
}

A few points tickle:

Why is it a static method?
None of the comments are helpful. Good comments say why, not what. I'd remove them all.
Bracing is inconsistent. Consider:

if (!(dr[propertyInfo.Name] is DBNull))
    propertyInfo.SetValue(ob, dr[propertyInfo.Name]);

And then:

if (!(dr[primaryKey] is DBNull))
{
    propertyInfo.SetValue(ob, dr[primaryKey].SafeToInt());
}

I prefer the latter, as explicit scopes are always less error-prone.
Naming is also inconsistent. You call a PropertyInfo, propertyInfo (♥++)... but then you have a List<T> that you call lst instead of list - although since that's the method's return value I'd just call it result.
Return value should probably be exposed as an IEnumerable<T> - it's your method's job to add items to that list, not the client code's. Right?

If you can return an IEnumerable<T>, then you can yield return ob, and drop the lst altogether.

Instead of using reflection to create an instance of T, I would use a : new() generic type constraint to ensure T has a parameterless constructor, and create the instance like this instead:

var ob = new T();

There is no generic type constraint at all, so technically this call would be valid:

var foo = WhateverTheClassIs.BindDataList<int>(dt, "Id");

I would consider changing the signature to include a class generic type constraint that ensures T is a reference type:

public static List<T> BindDataList<T>(DataTable dt, string primaryKey)
    where T : class, new()

ob is an awful name. I understand the intent was to call it object and that would have clashed with System.Object - I would rather see @object than ob, to tell you the truth. But instance is probably a better fit.
dr could be called row instead.


Extension method to retrieve list of days in the current week I want to get all days of the current week (from Monday to Sunday) with a custom time. To do this, I have written an extension method for TimeSpan:

public static DateTime[] Daily(this TimeSpan ts)
{
    var days = DayOfWeek.Monday - DateTime.UtcNow.DayOfWeek;
    var startDate = DateTime.UtcNow.AddDays(days);
    List<DateTime> dates = new List<DateTime>();

    for(var i = 0; i < 7; i++)
    {
        dates.Add(startDate.Date + ts);
        startDate = startDate.AddDays(1);
    }

    return dates.ToArray();
}

As an example of how it would be used, new TimeSpan(17, 0, 0).Daily() will return:

23/01/2016 17:00:00
24/01/2016 17:00:00
25/01/2016 17:00:00
26/01/2016 17:00:00
27/01/2016 17:00:00
28/01/2016 17:00:00
29/01/2016 17:00:00

Any suggestions on ways to improve my code?

Let's talk about two major things in this: building a modular API, and Unit Testing.

This can be easily unit tested, you should have 7 tests, one for a date in each day of the week. Let's pick a Week (past, present or future, doesn't matter), for this I'm going to pick an easy week. Since DayOfWeek.Monday is the first day of your week, we'll pick the first week of a month that starts on a Monday.

Scrolling backwards in my calendar I found August, 2016. August 1st, 2016 is a Monday, so we'll test that week.

The problem right now with your code is that it cannot be tested. You have to reset the system clock to test it. This is not a hard problem to fix. We'll redefine the API a little bit (add a default parameter, basically):

public static DateTime[] Daily(this TimeSpan ts, DateTime? checkDay = null)

So that's easy, now we can specify a specific day to check. All we have to do in the code is use that variable.

var weekDate = checkDay ?? DateTime.UtcNow;

Bam. That should be the first line of the method. That will make sure we get weekDate set to a valid DateTime.

Alright, good. So we redesigned the API a little and made it support an extra feature with no effort. Now we need to talk about that return.

Generally speaking: robust, modular API's that return a collection should return an IEnumerable<T> of some sort. I may have the user specify a specific DayOfWeek and try to find the first date in a give week that is that DayOfWeek, so I don't need the entire list (potentially). This is because each caller has different requirements (potentially), and to most closely follow SRP we shouldn't care about the resulting collection.

Our API doesn't need to worry about the collection as a whole, only generating each element in it. This may be a bizarre concept to understand at first, but our method should not care what the caller is going to do with the collection itself. We just generate it, so we don't need a list or array, we just need to return each element as it's generated.

We do this by returning an IEnumerable<DateTime> so that we can use the yield return statement instead of return. This will allow us to make a more modular API that doesn't always have to enumerate everything. It's the caller's responsibility to call .ToArray() if they need an array, or .ToList(), or enumerate it, ect.

public static IEnumerable<DateTime> Daily(this TimeSpan ts, DateTime? checkDay = null)

Lastly, different users' may have their week start on a different day. We want to accommodate for that, so we'll create a new default parameter for that.

public static IEnumerable<DateTime> Daily(this TimeSpan ts, DayOfWeek startDayOfWeek = DayOfWeek.Monday, DateTime? checkDay = null)

Bam. Now we've built a much more robust API that can be tested more easily as well. It should be trivial to implement this, in fact, we only need to change a few parts of your original code.

public static IEnumerable<DateTime> Daily(this TimeSpan ts, DayOfWeek startDayOfWeek = DayOfWeek.Monday, DateTime? checkDay = null)
{
    var compDate = checkDay ?? DateTime.UtcNow;
    var days = startDayOfWeek - compDate.DayOfWeek;
    var startDate = compDate.AddDays(days);

    for (var i = 0; i < 7; i++)
    {
        yield return startDate.AddDays(i).Date + ts;
    }
}

It's even shorter now, supports your original use-case, and add's many more supported use-cases.

Then we build Unit-tests, they'll all have mostly the same body:

var expected = new DateTime[]
{
    new DateTime(2016, 8, 1, 15, 0, 0),
    new DateTime(2016, 8, 2, 15, 0, 0),
    new DateTime(2016, 8, 3, 15, 0, 0),
    new DateTime(2016, 8, 4, 15, 0, 0),
    new DateTime(2016, 8, 5, 15, 0, 0),
    new DateTime(2016, 8, 6, 15, 0, 0),
    new DateTime(2016, 8, 7, 15, 0, 0)
};
var inputTimeSpan = new TimeSpan(15, 0, 0);
var inputDate = new DateTime(2016, 8, 1);

var actual = Daily(inputTimeSpan, checkDay: inputDate).ToArray();

CollectionAssert.AreEquivalent(expected, actual);

Just change inputDate. Once we do this we'll see that on 2016-08-01 we get a failed result. No big deal, we just need to make sure we fix our method. (Easy fix too.)

Our bug is the offset, on Sunday you'll get Monday-Sunday for the upcoming week. (So on 7 Aug 2016 you get 8 Aug 2016 - 14 Aug 2016.) That's fine, to fix it we'll just check if days > 0 and if so we'll subtract from it.

days = days > 0 ? days - 7 : days;

We'll just add that after we assign days the first time, and bam. We're done.

Final Code

public static IEnumerable<DateTime> Daily(this TimeSpan ts, DayOfWeek startDayOfWeek = DayOfWeek.Monday, DateTime? checkDay = null)
{
    var compDate = checkDay ?? DateTime.UtcNow;
    var days = startDayOfWeek - compDate.DayOfWeek;
    days = days > 0 ? days - 7 : days;
    var startDate = compDate.AddDays(days);

    for (var i = 0; i < 7; i++)
    {
        yield return startDate.AddDays(i).Date + ts;
    }
}

As you can see, it's still nice and short, and it's more robust than the original. :) This was an excellent start, and hopefully you take more out of this than just a bug fix. :)


Is this the proper way to find the subclass of a polymorphic superclass? I came across polymorphism in the book that I'm reading and decided to do a little experiment. Essentially what I did was to create a base class called Asset and two subclasses that derive from Asset, called Property and Stock. I created instances of these two types and passed them to this function:

public static void PrintAsset(Asset theAsset)
{
    Console.WriteLine(theAsset.name);
    Console.WriteLine(theAsset.GetType().ToString());

    string x;

    if (theAsset.GetType() == typeof(Stock))
    {
        Stock theStock = (Stock)theAsset;
        x = (theStock.numShares * theStock.stockPrice).ToString();
    }
    else
    {
        Property theStock = (Property)theAsset;
        x = (theStock.value).ToString();
    }

    Console.WriteLine(x + "\n");
}

Initially, the method only consisted of the first two lines, and the output shocked me since I would've figured the incoming reference (theAsset) would've been cast to Asset, but .GetType().ToString() surprisingly produced "...Stock" and "...Property" in the Console output.

I'm assuming the fact that they allow you to see the true class of the object being passed to the method for a reason, so doing something like this should be considered acceptable, but I was wondering if perhaps you SO/SE folks might disagree. Is there some unforeseen problem that this causes?

You have to learn a lot about OOP... There is no polymorphism in your code, as this principle is designed to tackle exactly the kind of code you've written.

The code that uses Asset should never branch its logic based on actual derived type (otherwise it would brake Open-Closed Principle), instead classes should use polymorphism to specify differences in logic. Here is simple example based on your code:

public abstract class Asset
{
    public string Name {get; set;}
    public abstract decimal CalculateValue();
    //... other common properties and methods of Asset
}

public class Property : Asset
{
    public decimal Value {get;set;}

    public override decimal CalculateValue()
    {
        return Value;
    }
}

//usage
public static void PrintAsset(Asset theAsset)
{
    Console.WriteLine(theAsset.Name);
    Console.WriteLine(theAsset.GetType().ToString());

    string assetValue = theAsset.CalculateValue().ToString();
    Console.WriteLine(assetValue + "\n");
}


Crawling SPOJ through cURL and C++ I am trying to write industry standard code.
https://www.quora.com/How-do-I-follow-a-user-on-Spoj-for-solving-problems-Refer-Details

Someone gave me this A2A.
And I wrote this code for it

//compile with g++ -std=c++11
#include <bits/stdc++.h>
using namespace std;
struct PROBLEM
{
    string name;
    string links;
    int users;
    double points;
};
bool compare (PROBLEM & p1 , PROBLEM & p2)
{
    return p1.users < p2.users;
}
void savePage(string & username)
{
    string url = "http://www.spoj.com/users/" + username +"/";
    string cmd = "curl " + url + " >" + username + ".txt";
    system (cmd.c_str());
}
bool checkRegex(string & str)
{
    return regex_match (str , std::regex("^\\s{0,}<td\\salign.{0,}$"));
}
bool checkRegexProb(string & str)
{
    return regex_match(str , std::regex("^\\s{0,}' ; i++);
    i++;
    for(;str[i] != '<' ; i++)
        temp += str[i];
    return stoi(temp);
}
int getUsers(string & str)
{
    string url = "http://www.spoj.com/ranks/" + str + "/";
    string cmd = "curl " +url + " >" + str +".txt";
    system(cmd.c_str());
    string filename = str + ".txt";
    int Count = 0;
    ifstream myfile(filename.c_str());
    if (myfile)
    {
        string line;
        while (getline( myfile, line ))
        {
            if (checkRegexProb(line) )
            {
                Count ++;
                if(Count == 2)
                {
                    myfile.close();
                    string deleteFile = "rm -r " + filename;
                    system(deleteFile.c_str());
                    return getNum(line);
                }
            }
        }
    }
}
vector<PROBLEM> readFile(string & username)
{
    std::vector<PROBLEM> v;
    string filename = username + ".txt";
    ifstream myfile(filename.c_str());
    if (myfile)
    {
        string line;
        while (getline( myfile, line ))
        {
            if (checkRegex(line) )
            {
                string temp;
                size_t found = line.find("/status/");
                for(size_t i = found + 8 ; line[i]!=',' ; i++)
                    temp += line[i];
                if(temp != "")
                {
                    PROBLEM p;
                    p.name = temp;
                    p.users = getUsers(p.name);
                    while(p.users == 0)
                        p.users = getUsers(p.name); // bug -> curl gives empty response
                    v.push_back(p);
                }
            }
        }
        myfile.close();
    }
    return v;

}
void makeLink(std::vector<PROBLEM> & v)
{
    for(int i = 0 ; i < v.size() ; i++)
        v[i].points = 80.0/(40.0+ v[i].users),
        v[i].links = " " + v[i].name +" "+ to_string(v[i].points) +"
";
}
string makeHtml(std::vector<PROBLEM> & v)
{
    string list ;
    for (int i = 0 ; i < v.size() ; i++ )
        list += v[i].links;
    string HTML =
    ""+ list +"";
    return HTML;
}
int main(int argc, char const *argv[])
{
    string username ;
    cin >> username ;
    savePage(username) ;
    std::vector<PROBLEM> v = readFile(username);
    sort(v.rbegin(), v.rend(),compare);
    makeLink(v);
    std::ofstream outfile ((username + ".html").c_str());
    outfile <<makeHtml(v);
    system(("rm -f "+ username + ".txt").c_str());
    system(("gnome-open " + username + ".html").c_str());
    return 0;
}

How can I improve it so that it matches industry standard?

A few more possibilities to consider:

pre-compille your regular expressions

Compiling a regular expression is (potentially) a somewhat expensive operation. In this case, the regular expressions remain constant throughout the program, so you might as well compile them once, and use the compiled regular expression after that:

bool checkRegex(string & str)
{
    static std::regex pattern{"^\\s{0,}<td\\salign.{0,}$"};

    return regex_match (str , pattern);
}

use operator overloading where sensible

For example, your compare function:

bool compare (PROBLEM & p1 , PROBLEM & p2)
{
    return p1.users < p2.users;
}

...really makes more sense as an overload of operator<:

bool operator<(PROBLEM & p1 , PROBLEM & p2)
{
    return p1.users < p2.users;
}

const correctness

Taking the operator above a step further, we don't want x < y to modify either x or y, so we should pass the operands by const reference:

bool operator<(PROBLEM const &p1 , PROBLEM const &p2)
{
    return p1.users < p2.users;
}

This way, when we sort our vector of Problem objects, we don't need to explicitly specify how to compare them:

sort(v.rbegin(), v.rend());

Use the force library, Luke

In addition to a command line utility, cURL is available as a library that you can use in your program. There's also a cURLpp library that provides a C++ wrapper around libcurl, which you may prefer. Either can help you write code that's quite a bit simpler and more readable.

Work with the library, not against it

Right now, you have (for example) a readFile to read the contents of a file into a vector of PROBLEM. That's not really how the standard library is meant to work though. To work optimally with it, you really just want to overload operator>> to read one PROBLEM from the file. Once you have that, you can (for example) use stream iterators to read from a file into a vector.

class Problem {
    // ...

    friend std::istream &operator>>(std::istream &is, Problem &p) {
        // code to read one Problem from the stream goes here
        return is;
    }
};

Then we can initialize our vector from a pair of iterators:

std::ifstream in("input.txt");
std::vector<Problem> v{ std::istream_iterator<Problem>(in), {} };


Finding a sum of unique triplet that is close to a given number in an array I asked one question on code review yesterday about my code style, logic and readability of code. Hence, today I kept all those suggestions in mind while solving a new problem question.

The question is such: Given an array S of n integers, find three integers in S such that the sum is closest to a given number, target. Return the sum of the three integers. You may assume that each input would have exactly one solution.

For example: given array S = {-1 2 1 -4}, and target = 1.
The sum that is closest to the target is 2. (-1 + 2 + 1 = 2).

This is my solution:

 public int threeSumClosest(int[] num, int target) {
    if(num.length < 3) {
        return 0;
    }
      Arrays.sort(num);
    int tempSum = Math.abs( target - ( num[0] + num[1] + num[2] ));
    int result = num[0] + num[1] + num[2];

    for (int index = 0; index < num.length - 1; index++) {
        int j = index + 1;
        int k = num.length-1;
        while (j < k){
            int newSum = Math.abs( target - ( num[index] + num[j] + num[k] ) );
            if( newSum == 0 ) {
                return ( num[index] + num[j] + num[k] ) ;
            }
            else if ( num[index] + num[j] + num[k] < target ) {
                if( newSum < tempSum ) {
                    tempSum = newSum;
                    result = num[index] + num[j] + num[k];
                }

                j++;
            }
            else {
                if( newSum < tempSum ) {
                    tempSum = newSum;
                    result = num[index] + num[j] + num[k];
                }
                k--;
            }
        }//end of while loop
    }// end of for loop
    return result;
}

It would be great to hear back if I need to make further improvements. Any help will be highly appreciated. Also, Is there an approach which can solve this problem in less than \$O(n^2)\$.

Algorithm - Reducing search space

I'd define kMax as num.length - 1.

Then, do a binary search for the point at which num[0] + num[1] + num[k - 1] is too low, and num[0] + num[1] + num[k] is too high. Set kMax to k.

Next, do a binary search for the point at which num[i] + num[kMax - 1] + num[kMax] is too high, and num[i - 1] + num[kMax - 1] + num[kMax] is too low. Define iMin to be i - 1.

Start your for loop loop with index at iMin. At the start of each iteration, set k to kMax.

Instead of your current moving up/moving down scheme, just start downing k. As long as [i][j][k-1] is too high, kMax = k. When [i][j][k-1] is no longer too high, resume your uppy-downy scheme.

Whenever you increment i, check if [i][k-1][k] is too low. If so, increase i until [i][k-1][k] is no longer too low. Check if [i-1][k-1][k] gives a better match and store it as temp.

AFter changing scope like this, check the opposite end of the scope. So if you just reduced k, check if [i][k-1][k] is too low. If you just increased i, check if [i][i+1][k] is too high. Do this until scope is stable.

Perhaps binary search could speed this (upping i and downing k) up too, but I'm not sure.

What this would do is potentially eliminate is a lot of your search space.

You see, if I gave you the number 42 to find in an array that contained random numbers between -500 and +200, -500, +199, +200 is simply too low. So you can kill off any number that's below -357, provided it's not the last number at or below -357.

This is a reduction of 20% of your search space with a simple binary search.

Additionally, once we get to -70, any number greater than 182 that is not the last number at or above 182 is not going to help.

Binary search is blazingly fast. 10000 elements can be searched through in 13-14 array accesses. 100000 elements - 17 array accesses.

Consider stopping binary search when your scope size is less than 128 (your code should go blazingly fast at that point anyway).

Your code

for (int index = 0; index < num.length - 1; index++)

For an array of 3 elements... index < num.length - 1 means index < 3 - 1 means index < 2.

So you've got a problem; You'll eventually check [1][2][2]. Your fix for this is while(j < k), but I think you'll be better served by changing the for loop header to be for (int index = 0; index < num.length - 2; index++) (changing index < num.length - 1 to index < num.length - 2). And then adding a comment WHY it is 2.

        int newSum = Math.abs( target - ( num[index] + num[j] + num[k] ) );
        if( newSum == 0 ) {
            return ( num[index] + num[j] + num[k] ) ;
        }
        else if ( num[index] + num[j] + num[k] < target ) {
            //code
            result = num[index] + num[j] + num[k];

You have here, in a single iteration, 2 to 3 summations of num[index] + num[j] + num[k]. Some languages will cache these values. Some languages will not. I'd get in the habit of summing them once, then doing what you must.

        else if ( num[index] + num[j] + num[k] < target ) {
            if( newSum < tempSum ) {
                tempSum = newSum;
                result = num[index] + num[j] + num[k];
            }

            j++;
        }
        else {
            if( newSum < tempSum ) {
                tempSum = newSum;
                result = num[index] + num[j] + num[k];
            }
            k--;
        }

Haaang on.

I think you can unduplicate your code here:

        else {
            if( newSum < tempSum ) {
                tempSum = newSum;
                result = num[index] + num[j] + num[k];
            }
            if(num[index] + num[j] + num[k] < target){
                k--;
            } else {
                j++;
            }
        }

Combine it with the idea of making the sum variable and you get the following:

        int sum = num[index] + num[j] + num[k];
        if( sum == 0 ) {
            return sum;
        }
        else {
            int newSum = Math.abs( target - sum );
            if( newSum < tempSum ) {
                tempSum = newSum;
                result = sum;
            }
            if(sum < target){
                k--;
            } else {
                j++;
            }
        }

Additionally, Math.abs is not yet required for your first if. So leaving that one out until it's needed speeds things up a bit.

And since a return statement exits the function, we can use an implicit else:

        int sum = num[index] + num[j] + num[k];
        if( sum == 0 ) {
            return sum;
        }
        int newSum = Math.abs( target - sum );
        if( newSum < tempSum ) {
            tempSum = newSum;
            result = sum;
        }
        if(sum < target){
            k--;
        } else {
            j++;
        }


About handling data from an API call through RX and Retrofit I started using Retrofit and RxJava recently and I love it. I am working on an Android project and I am following MVP Repository pattern as specified in Android Architecture Components. I'm still learning though.

Whenever I have to make an API call, I do it like this in the Presenter:

fruitsRepository
  .getFeaturedFruits()
  .subscribeOn(Schedulers.io())
  .observeOn(AndroidSchedulers.mainThread())
  .subscribe(new Observer<UniversalResponse<Fruit>>() {

    @Override
    public void onSubscribe(Disposable d) {
        compositeDisposable.add(d);
    }

    @Override
    public void onNext(UniversalResponse<Fruit> universalResponse) {

        if (view.isLost()) return;
        view.setProgressVisibility(false);

        if (universalResponse.isSessionExpired()) {

            view.onSessionExpired();
            return;
        }

        if (universalResponse.isError()) {

            if (universalResponse.isServerError()) {
                view.showErrorViewerPage(universalResponse.getServerErrorMessage());
            }

            view.showFeaturedFruitsError(universalResponse.getMessage());
            return;
        }

        List<Fruit> fruits = universalResponse.getItems();
        if (fruits != null && fruits.size() > 0) {

            view.showFeaturedFruits(fruits);
        }
        else {
            view.showNoFeaturedFruits();
        }
    }

    @Override
    public void onError(Throwable e) {

        if (view.isLost()) return;
        view.setProgressVisibility(false);

        view.showFeaturedFruitsError(e.getMessage());
    }

    @Override
    public void onComplete() {

        if (view.isLost()) return;
        view.setProgressVisibility(false);
    }
});

fruitsRepository is a Repository that can get data from local or remote source.
.getFeaturedFruits() calls retrofit internally if remote source is selected
compositeDisposable is CompositeDisposable
UniversalResponse is a class having httpStatusCode, isSuccess and List<T>
view is an implementation of UI from FeaturedFruitsContract.View

I am looking for some improvements to this. Can I reduce this somehow? If I have to make 5 different API calls, my presenter gets really bigger and repetitive. I was thinking about making an adapter for Observer I am using to subscribe.

Let me start by saying I am by no means an expert in Rx. I have no authoritative references to back up my suggested improvements. They come from the little experience I have, as well as some statements in articles from memory. Do no consider my suggestions as "A is bad and B is good so always use B" and only take them into consideration if you agree they are actual improvements.

It makes little sense for a retrofit api call to be represented as an Observable. An api call either succeeds with 1 item (the response) or it errs. Rx has a great match for this: Single. There might be exceptions to this but I do not know of any. If anyone does, please let me know. I would suggest you change the method signature to Single<List<Fruit>> getFeaturedFruits()
The scheduler on which an operation should run should be decided by the component that does the actual operation. subscribeOn should therefore be in the fruitsRepository.getFeaturedFruits. It might look like this

Single<List<Fruit>> getFeaturedFruits() {
    fruitsApi.getFeaturedFruits()
            .subscribeOn(Schedulers.io())
}

In case you have an observable that only emits 1 item and then completes, which is the case if you have a retrofit api call wrapped in an obserable, it makes little sense to implement onComplete in the subscriber if you already implemented onNext and onError. onComplete is more commonly used when you have a hot observable can emit multiple items and you don't know how many there might be. If you use a Single instead, you don't even have to think about this because there is only success or error, no next and complete.
You have if (view.isLost() return; in every callback. This seems like you might not be disposing of the observables properly. I don't know what .isLost() is doing, but consider instead having the view tell the presenter when it is "lost" and have the presenter clean up the compositeDisposable when that happens.
Use lambdas. Especially when using Rx where you use anonymous classes all over the place, they will greatly reduce the lines of code and (debatable) readability of the Rx chain. In recent versions of the android sdk the lambda notation is supported natively, otherwise you can use "retrolambda". This will let you reduce

.subscribe(new Observer<UniversalResponse<Fruit>>() {

    @Override
    public void onSubscribe(Disposable d) {
    }

    @Override
    public void onNext(UniversalResponse<Fruit> universalResponse) {
    }

    @Override
    public void onError(Throwable e) {
    }

    @Override
    public void onComplete() {
    }
}

to something like

.subscribe(disposable -> {}, universalReponse -> {}, e -> {}, {})

If you were to use a Single and just handle the success and error callbacks, you don't have void onSubscribe(Disposable d) from where you can add d to the compositeSubscription anymore. However, the .subscribe() call returns a disposable that you can handle the same way:

compositeDisposable.add(getFeaturedFruits().blabla.subscribe());


SICP - exercise 1.7 better end test for square root approximation Here is exercise 1.7 from SICP:

  Exercise 1.7

  The good-enough? test used in computing square roots will not be very
  effective for finding the square roots of very small numbers. Also, in
  real computers, arithmetic operations are almost always performed with
  limited precision. This makes our test inadequate for very large
  numbers. Explain these statements, with examples showing how the test
  fails for small and large numbers. An alternative strategy for
  implementing good-enough? is to watch how guess changes from one
  iteration to the next and to stop when the change is a very small
  fraction of the guess. Design a square-root procedure that uses this
  kind of end test. Does this work better for small and large numbers?

I believe "a very small fraction of the guess" means guess*a-very-small-number. I think it is working correctly. I haven't found any values that could fail the good-enough? test. Is there something wrong here I haven't discovered? Please review my code. How can I improve it?

(define (sqrt x)
  (sqrt-iter 1.0 x))

(define (sqrt-iter guess x)
  (if (good-enough? guess (improve guess x))
      guess
      (sqrt-iter (improve guess x) x)))

(define (improve guess x)
  (average guess (/ x guess)))

(define (average x y)
  (/ (+ x y) 2))

(define (good-enough? guess next-guess)
  (< (abs (- guess next-guess)) (* guess 0.000000000000000000000000000001)))

(define (square x)
  (* x x))

sqrt-iter

You compute (improve guess x) twice - this is a waste.
You return guess instead of the improved guess, i.e., you return a worse approximation than you could.

E.g.:

(define (sqrt-iter guess x)
  (let ((next-guess (improve guess x)))
    (if (good-enough? guess next-guess)
        next-guess
        (sqrt-iter next-guess x))))

good-enough?

0.000000000000000000000000000001 is unreadable. 1d-30 is better.
1d-30 is far too high precision. With such an epsilon, you might as well use = instead of good-enough? - the "oscillation between two adjacent floats" problem will be the same. Read up on machine epsilon.
This number should actually be an argument to sqrt.
Adding a line break would make the function more readable.


MATLAB and Armadillo, hints on translating i'm rewriting some Matlab code to c++ using armadillo, a libs for linear algebra. really good libs, IMHO. but i had to translate some matlab construct because armadillo isn't matlab.

i want to share my little snippet because i think there should be a better way (in terms of speed, mainly) to solve thoose little problems. i hope someone here could help me to improve my code!

static mat log(mat A) {
    /*
     * log function operates element-wise on matrix A
     * MATLAB code> log(A)
     */
    mat X(A);
    mat::iterator a = X.begin();
    mat::iterator b = X.end();

    for(mat::iterator i=a; i!=b; ++i) {
        (*i) = log(*i);
    }
    return X;
}

-

static mat vectorize(mat A) {
    /*
     * vectorise a matrix (ie. concatenate all the columns or rows)
     * MATLAB code> A(:)
     */
    mat B = mat(A);
    B.reshape(B.n_rows*B.n_cols, 1);
    return B;
}

-

static bool any(mat X, double n) {
    /*
     * check if there are some n in X
     * MATLAB code> any(X==n)
     * TODO: i'm not sure of description but it works for me
     */
    uvec _s = find(, n);
    if ( _s.is_empty() )
        return true;
    else
        return false;
}

-

static double sum(mat X) {
    /*
     * sum a matrix
     * MATLAB code> sum(X)
     */
    return sum(sum(X))
}

-

static field<rowvec num2cell(mat X) {
    /*
     *  converts matrix X into field by placing each row of X into a separate row
     *  this method assume that a cell is a field<rowvec>, mayebe a template should be used to generalize..
     *  MATLAB code> num2cell(X)
     */
    field<rowvec> data1(X.n_rows,1);
for (uint r = 0; r < X.n_rows; ++r) {
    data1(r,0) = X.row(r);
}
    return data1;
}

You're doing a copy in most of those snippets: if it was possible to do without a copy, it could be faster. Libraries such as Boost often offer two version a specific function: one which copies and another one which modifies in place.
*i is enough, you don't need (*i):

*i = log(*i);

This code:

if ( _s.is_empty() )
    return true;
else
    return false;

can be written as:

return _s.is_empty();


Espresso tests that check if Snackbar is displayed and if there is an internet connection I have recently began writing tests for Android using the espresso library.
Here I have three tests.

The first test is to check if a snackbar appears when searching with blank text.
The second is a search for a restaraunt not in the database.
Finally I have a test that checks if there is an internet connection.
Can anyone verify that my tests are doing what I intend them to do and if there is anything I am missing or doing wrong?

    public class MainScreenTest {

    //Rules are interceptors which are executed for each test method
    //and will run before any set up code in the @Before method
    @Rule
   public ActivityTestRule<MainScreen> mainScreenActivityTestRule =
            new ActivityTestRule<MainScreen>(MainScreen.class);

    @Test
    public void testSearchWithBlankText() throws Exception{
        Espresso.onView(withId(R.id.searchEditText)).perform(typeText(""),pressImeActionButton());
        Espresso.onView(withText(R.string.invalidSearch))
                .check(matches(isDisplayed()));

    }

    @Test
    public void testSearchWithInvalidRestaurant() throws Exception{
        Espresso.onView(withId(R.id.searchEditText))
                .perform(typeText("Hoffman"),pressImeActionButton());
        Espresso.onView(withId(R.id.noResultsTV)).check(matches(withText("No results, try another restaurant")));
    }

    @Test
    public void testWhenInternetIsAvailable() throws Exception{
        Context context = mainScreenActivityTestRule.getActivity().getBaseContext();
        ConnectivityManager cm = (ConnectivityManager)
                context.getSystemService(Context.CONNECTIVITY_SERVICE);
        boolean isConnected = cm.getActiveNetworkInfo().isConnectedOrConnecting();
        Assert.assertEquals(true, isConnected);
    }

}

Test#1 looks good, but I would add a case with blank text "_____" too.
Test#2 looks good, but I would name as userGetsErrorWhenSearchingInvalidRestaurant()
Test#3 needs attention. You verify only that an internet connection is good but you don't test user interaction. There are 2 options:

Left 'as is' but rename test method to `isConnectedOrConnectingReturnsTrueWhenInternetIsAvailable()
Add user interaction to make sure the data from internet was loaded successfully.


Synchronization Event with awaitAny In Java, there is no way I know of to wait for multiple events at the same time (see Stack Overflow). Since I would like to use that feature (similar to WaitForMultipleObjects() in the Win32 API), I decided to write my own event class that includes this feature. While the class is pretty straight-forward, thread synchronization always has some hidden pitfalls. I would like to uncover those, if any are present.

For some more specific questions:

Are there any possible deadlocks, race conditions, data races or other bugs present in the code?
Is the synchronization sufficient, or should bSignaled be made volatile?
Are there redundant checks or other things reducing the performance of the code?
Is there a library class that I have missed, offering the same functionality?

public class Event
{
    private boolean bSignaled = false;

    private static final Object anySignaler = new Object();

    /**
     * Wait for the event to signal. If the event has been signaled before,
     * return immediately.
     */
    public void await() throws InterruptedException
    {
        if (!bSignaled) {
            synchronized (this) {
                while (!bSignaled) {
                    this.wait();
                }
            }
        }
    }

    /**
     * Signal the event. This notifies all threads that are waiting for this
     * event to occur, as well as all threads that are waiting for any event.
     */
    public void signal()
    {
        bSignaled = true;
        sendNotify(this);
        sendNotify(anySignaler);
    }

    /**
     * Helper function that calls notifiyAll() inside a locked block.
     */
    private static final void sendNotify(Object lock)
    {
        synchronized (lock) {
            lock.notifyAll();
        }
    }

    /**
     * Return the signaled state of this event.
     */
    public boolean isSignaled()
    {
        return bSignaled;
    }

    /**
     * @see Event#awaitAll(Event[])
     */
    public static void awaitAll(Event ev0,Event ... events) throws InterruptedException
    {
        ev0.await();
        for (Event e: events) e.await();
    }

    /**
     * @see Event#awaitAll(Event[])
     */
    public static void awaitAll(List<Event> events) throws InterruptedException
    {
        for (Event e: events) e.await();
    }

    /**
     * Wait until all events have been signaled.
     */
    public static void awaitAll(Event[] events) throws InterruptedException
    {
            for (Event e: events) e.await();
    }

    /**
     * @see Event#awaitAny(Event[])
     */
    public static int awaitAny(Event ev0,Event ... events) throws InterruptedException
    {
        Event[] array = new Event[events.length+1];
        array[0] = ev0;
        if (events.length > 0) System.arraycopy(events,0,array,1,events.length);
        return awaitAny(array);
    }

    /**
     * @see Event#awaitAny(Event[])
     */
    public static int awaitAny(List<Event> events) throws InterruptedException
    {
        return awaitAny(events.toArray(new Event[events.size()]));
    }

    /**
     * Wait until any of the given events have been signaled. If any of the
     * events have already been signaled, return immediately.
     * @return the index of the event that was first found to be signaled.
     */
    public static int awaitAny(Event[] events) throws InterruptedException
    {
        int res = checkSignals(events);
        if (res < 0) {
            synchronized (anySignaler) {
                while ((res = checkSignals(events)) < 0) {
                    anySignaler.wait();
                }
            }
        }
        return res;
    }

    /**
     * Helper function to check if any of the events has been signaled.
     * @return the index of the first signaled event found,
     *         or -1 if no event was signaled.
     */
    private static int checkSignals(Event[] events)
    {
        int len = events.length;
        for (int i = 0; i < len; i++) {
            if (events[i].isSignaled()) return i;
        }
        return -1;
    }
}

In general, you should use one, and only one synchronization strategy in any class. You have synchronization, and are contemplating volatiles too.

The best-practice mechanism for now in Java is using the classes from java.util.concurrent.* to manage locking, and memory synchronization. The benefit from there that will help you most, is the Condition concept, as well as the fine-grained management of locks.

Your locking at the moment relies on a 'global' lock that all events use, thus if you have many collections of events running, each collection uses the same lock. Thus, if some other Events are happening in some other code block somewhere, we will still be notified or each of those 'remote' event completions, and our completions will notify their awaits....

Additionally, I did not like the way you manage the await-any concept. You literally scan the Events for changes, and check for one to complete.

In essence, I dislike:

using a static lock
using synchronization
using messy awaitAny logic

Finally, I did not like the method you chose as the core await container... Instead of the awaitAll(Event[]) I prefer the awaitAll(List<Event>...). I cannot defend that preference right now though. Not sure why I prefer it.

Still, having thrown all that criticism at you, here's how I would probably do it. Note, the logic change for the await....

You have a single static lock. What I suggest is to create a 'completion listener'. In the case of awaitAll, each Event has it's own listener. In the case of awaitAny, they all share a single listener, so there's just one notification.

The system below makes each await*(...) an independent operation. The only signals are what's happening that's relevant to the completion service.

This is untested, so go through and understand it well ;-)

import java.util.ArrayList;
import java.util.Arrays;
import java.util.List;
import java.util.concurrent.locks.Condition;
import java.util.concurrent.locks.Lock;
import java.util.concurrent.locks.ReentrantLock;

public class Event {

    private static final class EventCompletion {
        private final Lock plock = new ReentrantLock();
        private final Condition pcond = plock.newCondition();
        private boolean complete = false;

        void completed() {
            plock.lock();
            try {
                if (!complete) {
                    complete = true;
                    pcond.signalAll();
                }
            } finally {
                plock.unlock();
            }
        }

        void awaitCompletion() throws InterruptedException {
            plock.lock();
            try {
                while (!complete) {
                    pcond.await();
                }
            } finally {
                plock.unlock();
            }
        }

    }

    private final Lock lock = new ReentrantLock();
    private boolean complete = false;

    private final List<EventCompletion> listeners = new ArrayList<>();

    private void addListener(EventCompletion ec) {
        boolean notifyNow = false;
        lock.lock();
        try {
            if (!complete) {
                listeners.add(ec);
            } else {
                notifyNow = true;
            }
        } finally {
            lock.unlock();
        }
        if (notifyNow) {
            // immediate notification of completion...
            ec.completed();
        }
    }

    /**
     * Return the signaled state of this Event.
     */
    public boolean isSignaled() {
        lock.lock();
        try {
           return complete;
        } finally {
            lock.unlock();
        }
    }

    /**
     * Return the signaled state of this Event.
     */
    public void signal() {
        EventCompletion[] toNotify = null;
        lock.lock();
        try {
            if (complete) {
                return;
            }
            toNotify = listeners.toArray(new EventCompletion[listeners.size()]);
            listeners.clear(); // no need to remember previous notifications....
        } finally {
            lock.unlock();
        }
        for (EventCompletion ec : toNotify) {
            ec.completed();
        }
    }

    public static void awaitAll(Event ev0, Event... events) throws InterruptedException {
        List<Event> toWait = new ArrayList<>(1 + events.length);
        toWait.add(ev0);
        for (Event e : events) {
            toWait.add(e);
        }
        awaitAll(toWait);
    }

    public static void awaitAll(Event[] events) throws InterruptedException {
        awaitAll(Arrays.asList(events));
    }

    public static void awaitAll(List<Event> events) throws InterruptedException {
        EventCompletion[] toComplete = new EventCompletion[events.size()];
        int pos = 0;
        for (Event e : events) {
            EventCompletion ec = new EventCompletion();
            e.addListener(ec);
            toComplete[pos++] = ec;
        }

        for (EventCompletion ec : toComplete) {
            ec.awaitCompletion();
        }

    }

    public static int awaitAny(Event ev0, Event... events) throws InterruptedException {
        List<Event> toWait = new ArrayList<>(1 + events.length);
        toWait.add(ev0);
        for (Event e : events) {
            toWait.add(e);
        }
        return awaitAny(toWait);
    }

    public static int awaitAny(Event[] events) throws InterruptedException {
        return awaitAny(Arrays.asList(events));
    }

    public static int awaitAny(List<Event> events) throws InterruptedException {
        // all events notify the same completion service.
        EventCompletion ec = new EventCompletion();
        for (Event e : events) {
            e.addListener(ec);
        }
        ec.awaitCompletion();
        int pos = 0;
        for (Event e : events) {
            if (e.isSignaled()) {
                // first completed event is returned.
                return pos;
            }
            pos++;
        }
        throw new IllegalStateException("One of the events must have been signalled... This Exception should never happen!");

    }

}

Update: As per your comment, the use of ReentrantLock and Condition is not necessary. I prefer to use consistent locking mechanisms in my code, and I have 'standardized' on Locks. Locks can do anything synchronization can do, and then more too. Unless you have many thousands of events all happening at once, the memory cost of the locks would be negligible.

Still, I agree with the sentiment you expressed. The same can be done with synchronization:

import java.util.ArrayList;
import java.util.Arrays;
import java.util.List;

public class Event {

    /* Private class can be used as a synchronization lock too */
    private static final class EventCompletion {
        private boolean complete = false;

        synchronized void completed() {
            if (!complete) {
                complete = true;
                this.notifyAll();
            }
        }

        synchronized void awaitCompletion() throws InterruptedException {
            while (!complete) {
                this.wait();
            }
        }

    }

    private boolean complete = false;

    // use private-final listeners class as private lock on public class.
    private final List<EventCompletion> listeners = new ArrayList<>();

    private void addListener(EventCompletion ec) {
        boolean notifyNow = false;
        synchronized (listeners) {
            if (!complete) {
                listeners.add(ec);
            } else {
                notifyNow = true;
            }
        }
        if (notifyNow) {
            // immediate notification of completion...
            ec.completed();
        }
    }

    /**
     * Return the signaled state of this Event.
     */
    public boolean isSignaled() {
        synchronized(listeners) {
           return complete;
        }
    }

    /**
     * Return the signaled state of this Event.
     */
    public void signal() {
        EventCompletion[] toNotify = null;
        synchronized(listeners) {
            if (complete) {
                return;
            }
            toNotify = listeners.toArray(new EventCompletion[listeners.size()]);
            listeners.clear(); // no need to remember previous notifications....
        }
        for (EventCompletion ec : toNotify) {
            ec.completed();
        }
    }

    public static void awaitAll(Event ev0, Event... events) throws InterruptedException {
        List<Event> toWait = new ArrayList<>(1 + events.length);
        toWait.add(ev0);
        for (Event e : events) {
            toWait.add(e);
        }
        awaitAll(toWait);
    }

    public static void awaitAll(Event[] events) throws InterruptedException {
        awaitAll(Arrays.asList(events));
    }

    public static void awaitAll(List<Event> events) throws InterruptedException {
        EventCompletion[] toComplete = new EventCompletion[events.size()];
        int pos = 0;
        for (Event e : events) {
            EventCompletion ec = new EventCompletion();
            e.addListener(ec);
            toComplete[pos++] = ec;
        }

        for (EventCompletion ec : toComplete) {
            ec.awaitCompletion();
        }

    }

    public static int awaitAny(Event ev0, Event... events) throws InterruptedException {
        List<Event> toWait = new ArrayList<>(1 + events.length);
        toWait.add(ev0);
        for (Event e : events) {
            toWait.add(e);
        }
        return awaitAny(toWait);
    }

    public static int awaitAny(Event[] events) throws InterruptedException {
        return awaitAny(Arrays.asList(events));
    }

    public static int awaitAny(List<Event> events) throws InterruptedException {
        // all events notify the same completion service.
        EventCompletion ec = new EventCompletion();
        for (Event e : events) {
            e.addListener(ec);
        }
        ec.awaitCompletion();
        int pos = 0;
        for (Event e : events) {
            if (e.isSignaled()) {
                // first completed event is returned.
                return pos;
            }
            pos++;
        }
        throw new IllegalStateException("One of the events must have been signalled... This Exception should never happen!");

    }

}


Create ranges from an array of integers Given an array of ints, return a string identifying the range of
  numbers.

  Example:

  Input arr - [0 1 2 7 21 22 1098 1099]
  Output - "0-2,7,21-22,1098-1099"

Is there any improvement possible in this implementation?

#include<iostream>
#include<vector>
#include<algorithm>
#include<sstream>

std::string  range_creator(std::vector<int> & vec)
{
    std::sort(std::begin(vec),std::end(vec));

    int first=vec.at(0);
    int prev=first-1;

    std::stringbuf buffer;
    std::ostream os (&buffer);

    for(auto &x : vec)
    {
       if(++prev == x)
           continue;
        else
        {

           if(first != --prev)
           {
               os << first<<"-"<<prev<<",";
               prev=x;
               first=x;
           }
           else
           {
               os<<first<<",";
               prev=x;
               first=x;
           }

        }

    }
    if(first != prev)
    {
        os<<first<<"-"<<prev;
    }
    else
    {
       os<<first;
    }
    return buffer.str();
}

int main()
{
    std::vector<int> vec={0,1,2,7,8,9,10,21,22,23,24,25,27,1098,1099,1100,2000};
    std::cout<<range_creator(vec);

    return 0;
}

You can make good use of std::adjacent_find. Like this:

std::string range_creator(std::vector<int> & vec)
{
    auto first = std::begin(vec), last = std::end(vec);
    if(first == last)
        return "";
    std::sort(first, last);
    std::stringbuf buffer;
    std::ostream os (&buffer);
    while(true)
    {
        auto mid = std::adjacent_find(first, last,
                [](int x, int y){ return x + 1 != y; });
        if(mid == last)
            break;
        if(first == mid)
            os << *first << ",";
        else
            os << *first << "-" << *mid << ",";
        first = ++mid;
    }
    if(first == --last)
        os << *first;
    else
        os << *first << "-" << *last;
    return buffer.str();
}

I think my code is still subject to further improvements, but I'm no expert, so I'll leave it to the community.


Javascript Module Design I've been trying to create a tree module in JavaScript for implementing a "family tree" style structure. I'm not very experienced in newer JavaScript design styles and want to make sure I am going along the right path with the code I have written so far. Can someone let me know if I am on the right track with how this is organized?

var TREE = (function() {

var treeList = [], //Tree container

    //Constructor for Member object
    Member = function(name, mother, father, children){
        this.name = name || null;
        this.mother = mother || null;
        this.father = father || null;
        this.children = children || null;
    },

    //Constructor for Tree object
    Tree = function(name){
        this.name = name || null;
        this.memberList = [(new Member("root"))];
    };

//Tree methods
Tree.prototype.addMember = function(name, mother, father, children){
    this.memberList.push(new Member(name, mother, father, children));
}
Tree.prototype.getMemberList = function(){
    return this.memberList;
}
return {
    create : function(treeName){
        treeList.push(new Tree(treeName));
        return treeList[treeList.length - 1];
    },
    getTreeList : function(){
        return treeList;
    }
};

}());

Looks okay to me,

I would not expose function names mentioning node, but rather familyMember or member.
You expose only name through addNode, but you have name, mother, father, children in the constructor of Node.
Your var statement is not well indented, making the code harder to understand
I am curious as to treeList, what use case do you have in mind for that?


Code review of forward invocation Here's the situation. I'm writing a simple game and I had two main actors: GameController and GridView.

GridView is a UIView subclass displaying a grid with which the user interacts. It defines its custom delegate protocol (GridViewDelegate) for handling callbacks such as gridView:didSelectTile and others.
GameController is the game controller (duh!) which instantiate a GridView and set itself as a delegate, therefore implementing the GridViewDelegate protocol.

So far so good, then I decided to add HUD components on top of the grid (a score, a timer and other stuff). The most reasonable choice seemed to wrap such HUD components along with the grid into a new UIView subclass called GameBoard.

And here comes the design issue: I need the controller to talk to the GridView and I think there's two reasonable options here.

Expose a gridView property and do something like

[self.gameBoard.gridView doStuff];

Forward the invocation made to GameBoard directly to GridView overriding the forwardInvocation: method of GameBoard

The first options looks like the most convenient, but I cannot get myself into liking it, due to the Law of Demeter.

So I decided to go for the second approach and do something like

// The GameBoard serves a proxy between the GameContoller and the GridView
- (void)forwardInvocation:(NSInvocation *)anInvocation {
    if ([self.gridView respondsToSelector:anInvocation.selector]) {
        [anInvocation invokeWithTarget:self.gridView];
    } else {
        [super forwardInvocation:anInvocation];
    }
}

// This method is necessary for the above forwardInvocation to work
- (NSMethodSignature *)methodSignatureForSelector:(SEL)aSelector {
    NSMethodSignature * signature = [super methodSignatureForSelector:aSelector];
    if (!signature) {
        signature = [self.gridView methodSignatureForSelector:aSelector];
    }
    return signature;
}

It works as expected, but I'd like to have a second opinion on my design choice.

I generally tend to avoid overusing of the dynamic features of Objective-C, but it looked like an elegant way to achieve my result fulfilling the Law of Demeter.

I think you trying to overengineer solution a bit. First of all View should not do any stuff at all. View is just a View, sheet of paper. Maximum of view's responsibility is layouting himself. So controller should take care of other view-related stuff.

In this case I probably prefer to think about gridView as legitimate subview of GameBoard. I really doubt Demetra suffering when you doing something like [self.view.titleLabel sizeToFit].

So my advice is to use [self.gameBoard.gridView doStuff]; as simplest and straightforward solution.


Implementing an encryption standard over message text I wrote the below code and tested some example files on it.  I didn't find any errors, but I still want to be thorough about this.

Also, any suggestions in improving the code?

import string
""" this implements an encryption standard over the message text
it replaces the odd place alphabets by their ordinal number plus place number increased.
it replaces the even place alphabets by their ordinal number minus place number decreased.
for example "hello" becomes "icoht" and "abc" becomes "bzf".
"""
input_filename = raw_input("Enter the input file name: ")
output_filename = raw_input("Enter the output dumping file name: ")
mode = raw_input("Do you want to encrypt or decrypt (e/d)?: ")
with open(input_filename,'r') as inputfile:
    with open(output_filename,'w') as outputfile:
        inp = inputfile.read()
        encode = lambda s: "".join(
            (chr(ord('a')+((ord(c)-(ord('a')+i+1))%26))) if ((c.isalpha()) and ((i+1)% 2 ==0))
            else
            string.ascii_lowercase[(string.ascii_lowercase.rfind(c.lower())+i+1)%26]  if ((c.isalpha()) and ((i+1)% 2 ==1))
            else
            c
            for i,c in enumerate(s.lower())
            )
        decode = lambda s: "".join(
            string.ascii_lowercase[(string.ascii_lowercase.rfind(c.lower())+i+1)%26] if ((c.isalpha()) and ((i+1)% 2 ==0))
            else
            (chr(ord('a')+((ord(c)-(ord('a')+i+1))%26))) if ((c.isalpha()) and ((i+1)% 2 ==1))
            else
            c
            for i,c in enumerate(s.lower())

            )
        if(mode.lower() == 'e'):
            outputfile.write(encode(inp))
        else :#(mode.lower() =='d'):
            outputfile.write(decode(inp))
    s = {'e':'Encoding','d':'Decoding'}
    print s.get(mode)+" Sucessfull"

Functions are your friend. The remove repeated code and improve readability. I have absolutely no idea what the block of text does inside the "".join(XXXX) calls. There is too much going on there that I'm not going to take the time to see what it does or if it can be done better.

However, it does appear that you can replace that mess with something like this:

def doA():
  pass
def doB():
  pass
def makeJoinArgument(value, which_step,step1, step2):
  return step1() if (which_step) else step2() for x in value
def encode(value):
  return "".join("abc", True, doA, doB)
def decode(value):
  return "".join("abc", True, doB, doA)

This obviously doesn't run (and not exactly what you have), but it is much more manageable than what you have now.

Another note (from PEP 8: Style Guide)

  Always use a def statement instead of an assignment statement that
  binds a lambda expression directly to a name.

  Yes:

  def f(x): return 2*x

  No:

  f = lambda x: 2*x

  The first form means that the name of the resulting function object is
  specifically 'f' instead of the generic ''. This is more
  useful for tracebacks and string representations in general. The use
  of the assignment statement eliminates the sole benefit a lambda
  expression can offer over an explicit def statement (i.e. that it can
  be embedded inside a larger expression)


Nested for-loop to create list of lists - Python I have posted the below also at stackoverflow in this thread, but a poster there recommended I post this here.  Thanks for any suggestions.

I have the below code, and as someone new to Python, I was hoping for suggestions on making it more Pythonic.  Posters in this thread have provided very helpful numpy and list comprehension solutions; however, I'd like to improve my more basic nested loop solution solely for the sake of becoming better with basic Python tasks (please do let me know if I should have posted this in the prior thread instead of starting a new semi-duplicate...apologies if that's what I should have done).

Here's the current code (which works as desired):

sales_data = [[201703, 'Bob', 3000], [201703, 'Sarah', 6000], [201703,
'Jim', 9000], [201704, 'Bob', 8000], [201704, 'Sarah', 7000], [201704,
'Jim', 12000], [201705, 'Bob', 15000], [201705, 'Sarah', 14000], [201705,
'Jim', 8000], [201706, 'Bob', 10000], [201706, 'Sarah', 18000]]

sorted_sales_data = sorted(sales_data, key=lambda x: -x[2])
date_list = []
sales_ranks = []

for i in sales_data:
    date_list.append(i[0])

sorted_dates = sorted(set(date_list), reverse=True)

for i in sorted_dates:
    tmp_lst = []
    tmp_lst.append(i)
    for j in sorted_sales_data:
        if j[0] == i:
            tmp_lst.append(j[1])
    sales_ranks.append(tmp_lst)

print(sales_ranks)

Any suggestions on making this more Pythonic (other than numpy or list comprehension solutions) would be greatly appreciated.  I am using Python 3.6.

Using a pure Python approach, this is exactly the job for itertools.groupby. But for it to work efficiently, you need to sort your data by the same key you will group them by (i.e. the date). Since your first sort also sort by the third field, you need to sort your list by multiple attributes

I would also build a generator to help reuse code and separate the core of your computation from the testing code using if __name__ == '__main__'. Something along the lines of:

from operator import itemgetter
import itertools

def generate_ranks(sales_data):
    sorted_data = sorted(sales_data, key=itemgetter(0, 2), reverse=True)
    for date, group in itertools.groupby(sorted_data, key=itemgetter(0)):
        yield [date] + [entry[1] for entry in group]

if __name__ == '__main__':
    sales_data = [
        [201703, 'Bob', 3000],
        [201703, 'Sarah', 6000],
        [201703, 'Jim', 9000],
        [201704, 'Bob', 8000],
        [201704, 'Sarah', 7000],
        [201704, 'Jim', 12000],
        [201705, 'Bob', 15000],
        [201705, 'Sarah', 14000],
        [201705, 'Jim', 8000],
        [201706, 'Bob', 10000],
        [201706, 'Sarah', 18000],
    ]
    sales_ranks = list(generate_ranks(sales_data))
    print(sales_ranks)


Javascript startsWith prototype I'm a Java guy in a JavaScript world, and I'm used to certain things about Java Strings that just aren't there for in JavaScript. Some of this I can accept, for example Strings are freaking everywhere for some reason.

Now I realize this could be simply implemented with return this.indexOf(needle) === 0; (or, even more JavaScripty, return this.indexOf(needle); but the flaw in that is that it will search the entire (potentially somewhat lengthy, you never know) String in its endeavor to find the index, when I really only care if its non-zero.

My current solution, which passes my testing so far, is thus. I'm open to all feedback, but I'm most interested in

Breaks from conventional JavaScript (the bad stuff, the smells)
Corner-cases this fails for
To a reasonable extent, style

I've also decided to tack it onto the class itself (I have to admit, I like being able to do that!) Is that bad practice?

String.prototype.startsWith = function(needle) {
    // This was dubious, if only because most times "" gets passed
    // it's by mistake... but decided since it was technically correct
    // that it should stay
    if(needle === "") return true;
    // but in all other cases, falsey values were considered false
    if(!needle) return false;

    // our mission, to find the needle in the haystack - is this tacky?
    var haystack = this;

    // primarily here to prevent AIOOBEs in the loop - is this the most
    // JavaScripty way to preempt that error?
    if(needle.length > haystack.length) {
        return false;
    }
    // I've read that 'i' is not scoped to this for loop, and that I would
    // need let instead. I must support older browsers, is there a better
    // way to do this?
    for(var i = 0; i < needle.length; i++) {
        if(haystack.charAt(i) !== needle.charAt(i)) {
            return false;
        }
    }
    return true;

}

I'd suggest using String.prototype.slice to implement startsWith like this:

String.prototype.startsWith = function(str) {
  return this.slice(0, str.length) === str
}

For type-checking (not needed in this example, but generally speaking) you might want to use the typeof operator:

if (typeof str !== 'string') return false

You can also use the instanceof operator if the value your dealing w/ isn't a literal or you need to check every value in the prototype chain, e.g. str instanceof String

haystack = this? Super-tacky ;p, but each to their own

Not sure if JS has an AIOOBE equivalent but 'blah'.charAt(6) === '' which tells me that check isn't needed.

And I'd suggest using Array iterators rather than for loops in the future:

var haystackStartsWithNeedle = needle.split('').
  forEach((v,i) => v === haystack[i]).find(v => v)

You might want to check babeljs.io. It transpiles new & flavoured JS to code most browsers can use.


Using a destructuring assignment on a style object to extract two members Recently, one of my colleague decided to destructure the React-Native styles object this way:

const { header, headerHint, bodyContainer, body, normalText, buttonWrapper, button, footer, footerLinks } = styles;

And then used the variables in the React code:

<View style={header}>
  <Text style={headerHint}hint</Text>
 ...
</View>

This felt quite wrong to me as it doesn't provide much value to the code, destructuring an object to grab more than 5-6 variables makes the code more difficult to read in my opinion.
Also, keeping the styles in the styles object let you know your styles comes from the StyleSheet. In some cases we might want to create variables to merge several styles, they become much easier to spot.

He argues destructuring reduces areas that you need to change - if you need to change them like if the styles object changes to something else, you don't have to remove all the references to styles.

I'm wondering what is your opinion on this point and if you follow any style guide about destructuring object like this one?

I think it is a matter of preference, and also of the size of the file. If it is larger with more variables then I would tend to using the styles. prefix to disambiguate. You could also compromise and go for s = styles followed by s.headerHint.

As to the name changing issue, you could always just write styles = newGangledStyles however most decent IDEs will refactor for you and even if they don't then this type of search and replace is usually pretty straight-forward and risk free also. I also suspect that, if the styles object did change, then it would be likely that the style names would also change. Using a styles. prefix actually makes it easier to locate usages.


Implementing a DNA codon table in C I decided to redo the first program I wrote (after the hello world thing). So, what it does is to convert a DNA sequence string to an array of strings that represent the amino acids that are made in a cell by ribosomes when that DNA sequence gets used (DNA codon table).

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

char* codon_table[4][4][4] =
{
  {
    {"Phe", "Phe", "Len", "Len"},
    {"Ser", "Ser", "Ser", "Ser"},
    {"Tyr", "Tyr", "Stop", "Stop"},
    {"Cys", "Cys", "Stop", "Trp"}
  },
  {
    {"Len", "Len", "Len", "Len"},
    {"Pro", "Pro", "Pro", "Pro"},
    {"His", "His", "Gln", "Gln"},
    {"Arg", "Arg", "Arg", "Arg"}
  },
  {
    {"Ile", "Ile", "Ile", "Met"},
    {"Thr", "Thr", "Thr", "Thr"},
    {"Asn", "Asn", "Lys", "Lys"},
    {"Ser", "Ser", "Arg", "Arg"}
  },
  {
    {"Val", "Val", "Val", "Val"},
    {"Ala", "Ala", "Ala", "Ala"},
    {"Asp", "Asp", "Glu", "Glu"},
    {"Gly", "Gly", "Gly", "Gly"}
  }
};

int check_nucleobase_string(char* nucleobase_string)
{
  if(nucleobase_string==NULL)
  {
    return 1;
  }
  for(size_t i = 0; nucleobase_string[i] != '\0'; i+=1)
  {
    if(
      nucleobase_string[i] != 't' &&
      nucleobase_string[i] != 'c' &&
      nucleobase_string[i] != 'a' &&
      nucleobase_string[i] != 'g'
    )
    {
      return 0;
    }
  }
  return 1;
}

size_t nucleobase_to_aminoacid(char* nucleobase_string, char*** aminoacid_string)
{
  if(!check_nucleobase_string(nucleobase_string))
  {
    printf("Erroneous input: %s\n", nucleobase_string);
    return 0;
  }

  size_t nucleobase_count ;
  for(nucleobase_count = 0; nucleobase_string[nucleobase_count] != '\0'; nucleobase_count+=1);
  size_t aminoacid_count = nucleobase_count/3;

  (*aminoacid_string) = malloc(aminoacid_count);

  for(size_t i = 0, slow_i = 0; slow_i < aminoacid_count; i+=3, slow_i+=1)
  {
    (*aminoacid_string)[slow_i] =
      codon_table[
        nucleobase_string[i] == 't' ? 0 :
        nucleobase_string[i] == 'c' ? 1 :
        nucleobase_string[i] == 'a' ? 2 :
        nucleobase_string[i] == 'g' ? 3 : -1
      ][
        nucleobase_string[i+1] == 't' ? 0 :
        nucleobase_string[i+1] == 'c' ? 1 :
        nucleobase_string[i+1] == 'a' ? 2 :
        nucleobase_string[i+1] == 'g' ? 3 : -1
      ][
        nucleobase_string[i+2] == 't' ? 0 :
        nucleobase_string[i+2] == 'c' ? 1 :
        nucleobase_string[i+2] == 'a' ? 2 :
        nucleobase_string[i+2] == 'g' ? 3 : -1
      ];
  }

  return aminoacid_count;
}

int main()
{
  printf("Enter DNA sequence in 5' → 3' direction.\n");

  char* input_string = NULL;
  size_t n = 0;
  ssize_t line_length = getline(&input_string, &n, stdin);
  if(line_length == -1)
  {
    printf("Failed to read a line.\n");
    return 0;
  }

  char* nucleobase_string = malloc(line_length);
  memcpy(nucleobase_string, input_string, line_length-1);
  nucleobase_string[line_length-1] = '\0';

  char** aminoacid_string = NULL;
  size_t aminoacid_count = nucleobase_to_aminoacid(nucleobase_string, &aminoacid_string);

  for(size_t i = 0; i < aminoacid_count; i+=1)
  {
    if(i!=0)
    {
      printf(" + ");
    }
    printf("%s", aminoacid_string[i]);
  }
  printf("\n");

  return 0;
};

An array of char* is not a great data structure for storing AA or codon sequence data.  A pointer takes 4 or 8 bytes per codon, but there are only 64 possible codons so you could easily store one codon per byte as a uint8_t.  And with only about 22 amino acids, that's close to being able to fit 2 AAs per byte, but it doesn't quite work and it's definitely easier to still use a uint8_t.

Then when you want to print, use that integer as an index into a table of strings.

Or if all you want to do is print out a string, then don't construct an array of strings at all, and just printf as you convert.  Or memcpy 4-byte strings like "Phe " into a buffer to construct a string yourself instead of calling printf separately for each AA name.

(Compilers are good at 4-byte copies, because that's the size of an integer register.  Especially from constant data, because they can inline the data as an immediate if that's optimal.  So make your codon table static const)

slow_i is an odd variable name.  Use aa_idx or something else meaningful, or maybe j.  Or use one i variable and use i*3 + 0, i*3 + 1, and i*3 + 2 to let the compiler figure out what it wants to do.

This very long line: for(nucleobase_count = 0; nucleobase_string[nucleobase_count] != '\0'; nucleobase_count+=1); should be nucleobase_count = strlen(nucleobase_string);.  Use library functions instead of re-inventing the wheel in a hard-to-read long line.  The libc string functions usually have hand-optimized asm implementations that will be fast especially for long strings.

Or even better, have your nucleotide checking function return a count if it doesn't find problem!  You're already looping over the whole string; you don't need to do it again to count the length.

Or even better than that, have your caller pass you the length so you can allocate memory right away and combine checking with converting.  You could have the caller pass you an already-allocated output buffer as well as the length, so it has the option of using a C99 variable-length-array, reusing a buffer it already has allocated, or whatever, instead of baking malloc into your conversion routine.

 codon_table[  // your code
    nucleobase_string[i] == 't' ? 0 :
    nucleobase_string[i] == 'c' ? 1 :
    nucleobase_string[i] == 'a' ? 2 :
    nucleobase_string[i] == 'g' ? 3 : -1
  ][...][...]

If a bad nucleotide did slip through the cracks and make it to this code, you'd have undefined behaviour from reading codon_table[-1][0][0]; out of bounds access.  You could remove the == g part and just have 3 be the last step in the ternary operator, with a comment to indicate that it's g if it wasn't one of the other 3.  Also, that block of ternary operators should go in a tiny helper function like int base_to_idx(char base); instead of being repeated 3 times.

Then you can re-implement it with a table lookup or a perfect hash function.  e.g.

// probably not faster than a table lookup, unless you make a SIMD version
/*
 'a'    0b1100001 -> 0b00
 'c'    0b1100011 -> 0b10
 'g'    0b1100111 -> 0b01
 't'    0b1110100 -> 0b11
*/
unsigned nuc_to_idx(unsigned base) {
    if (base == 'a' || base == 'c' || base == 'g' || base == 't')
        return (base*3 & 0x3<<2)>>2;
    return -128U;
}

or a table lookup version that does a whole codon and returns an integer code.  I used C designated-initializer syntax, plus a GNU extension to for ranges to make the table static const.  You could also init it at runtime with memset(table, -1,  UCHAR_MAX); and then assign the 4 valid values.

#include <stdint.h>
#include <limits.h>

// int8_t sign extends to the width of int / unsigned int
// so all the high bits get set before treating as unsigned (assuming 2's complement), for BAD_NUC inputs.
// But we don't depend on that: we still get an unsigned value > 64
// on sign/magnitude or one's complement machines if any of the 3 chars is bogus.

#define BAD_NUC -128
static const int8_t nuc_table[UCHAR_MAX+1] = {
    [0 ... 255] = BAD_NUC,  // ranges are a GNU extension
    // last init takes precedence  https://gcc.gnu.org/onlinedocs/gcc/Designated-Inits.html
    ['a'] = 0,
    ['c'] = 1,
    ['g'] = 2,
    ['t'] = 3,
};
// Fun fact: doesn't depend on ASCII encoding because of the designated-initializers.

// unsigned char* so high-ASCII -> 128..255, not negative,
// and works as an index into a 256 entry table
unsigned codon_to_idx_LUT(unsigned char *p) {

    unsigned idx = nuc_table[p[0]];
    idx = idx*4 + nuc_table[p[1]];
    idx = idx*4 + nuc_table[p[2]];
    return idx;
    // 0..63 for in-range values, otherwise has higher bits set
    // so you only have to range-check once per codon, speeding up the common case of no error.

    // to map to AA codes, maybe use a lookup table on this 0..63 value
}

The table version compiles really efficiently on x86 with gcc -O3  (on the Godbolt compiler explorer), or hopefully with any sane compiler.  (The if condition in the perfect-hash version does compile to an immediate bitmap, which is neat, but it still has to test/branch multiple times for every character instead of just doing 2 loads.)

codon_to_idx_LUT:                # gcc7.3 -O3 for x86-64 System V calling convention: pointer arg in RDI
    movzx   eax, BYTE PTR [rdi]
    movsx   edx, BYTE PTR nuc_table[rax]
    movzx   eax, BYTE PTR [rdi+1]
    movsx   eax, BYTE PTR nuc_table[rax]
    lea     edx, [rax+rdx*4]              # this is why I wrote it as idx*4 + nuc_table[...] instead of some other way of multiplying and adding the results.
    movzx   eax, BYTE PTR [rdi+2]
    movsx   eax, BYTE PTR nuc_table[rax]
    lea     eax, [rax+rdx*4]
    ret

This would run even faster if the compiler did one wider load and unpacked it with ALU instructions instead of loading each string byte separately.  Running this in a loop is going to bottleneck on 2 loads per clock on mainstream Intel/AMD CPUs, so about 1 nucleotide per clock cycle, which is not bad.  The overhead of checking the index for in-range (to detect bogus nucleotide characters) should be negligible compared to the string loads + table lookups.  Especially on CPUs that can only do one load per clock.

(Related this Q&A for more about tweaking your C source to hand-hold the compiler into making better asm.  You could do that here, but only at the cost of readability, I think.  You'd probably have to do a dodgy pointer-cast or a safe memcpy and read 4 bytes as a uint32_t and unpack that.)

You can use this 0..63 codon as an index into a table of strings for printing.  (Just flatten your current 3D array, although I didn't make any effort to be consistent with which numbers go with which DNA base in the hash or table vs. your version.)


Generic util method to convert in single map from list of array of Object generally result of JPQL/ hibernate I have designed a generic method to convert list of array of Objects to result into single map.  This is working fine, but I want to check weather a key object is valid key (whether it is overriding equals and hashcode method or immutable instance). Am I missing anything else in my code?

/**
@parms
objects : list of array of object generally result of HQL select
keyIndx : index of key object in the array
valueIndex: index of value object in the array
*/

public static Map<? extends Object, ? extends Object> getSingleMapFromLisOfArrayOfObject(List<? extends Object[]> Objects, int keyIdex, int valueIndex)
        throws Exception
    {
        Map<Object, Object> map = null;
        if (Objects != null) {
            map = new HashMap<Object, Object>(Objects.size());
            for (Object[] array : Objects) {
                if (Math.max(keyIdex, valueIndex) > (array.length - 1)) {
                    throw new IndexOutOfBoundsException(keyIdex + " OR " + valueIndex + " out of bound for array");
                }
                if (array[keyIdex] != null) {
                    map.put(array[keyIdex], array[valueIndex]);
                }
            }
        }
    return (map == null ? Collections.EMPTY_MAP : map);

}

There is not much "generic" here, the code is as type safe as with raw types. Further, throws Exception usually isn't a good idea. Last but not least, method arguments should be written lower-case.

Here my attempt (off the top of my head, couldn't test it):

public static <K,V> Map<K, V> getMapFromObjectList(
       List<? extends Object[]> objects,
       Class<K> keyClass, Class<V> valueClass,
       int keyIndex, int valueIndex)
    {
        if (object == null) {
          return Collections.EMPTY_MAP;
        }
        Map<K, V> map = new HashMap<K,V>(objects.size());
        for (Object[] array : objects) {
            if (Math.max(keyIndex, valueIndex) > (array.length - 1)) {
                    throw new IndexOutOfBoundsException(keyIndex + " OR " + valueIndex + " out of bound for array");
            }
            if (array[keyIdex] != null) {
                map.put(keyClass.cast(array[keyIdex]),
                        valueClass.cast(array[valueIndex]);
            }
        }
        return map;
    }

Of course this code will fail if the types are not the expected ones, but at least it will fail early, and it is already inherently "dangerous" code. On the other hand, it's reasonable to expect that Hibernate doesn't mess it up out of the blue, once the types are correct.

Note that HashMap supports null keys (although IMHO this design decision is questionable).


Implement array element swap in a functional language Sorting algorithms involve swapping element positions in an array. This is my implementation of swap:

let swap (array: 'a[]) (first: int) (second: int): 'a[] =
    let originalFirst = array.[first]
    array.[first] <- array.[second];
    array.[second] <- originalFirst;
    array

Though it works, I am wondering whether mutating an array is canonical F#. If not, what is the canonical way to swap array element positions?

The F# documentation says that arrays are mutable. Am I misunderstanding immutability?

Any and all comments on the above implementation are most welcome.

First and foremost: while mutation may be generally frowned upon in F#, in some cases (like this one) it's acceptable to make a tradeoff of immutability vs. performance or maintainability. In this case, it serves both of those goals.

Let's say you want to do this idiomatically, and you want to avoid directly mutating the array. It isn't that hard:

let swap el1 el2 arr =
    arr |> Array.mapi (fun i v ->
        match i with
        | _ when i = el1 -> arr.[el2]
        | _ when i = el2 -> arr.[el1]
        | _ -> v)

This uses the mapi function, which provides the current index and value (in that order, defined by the int -> 'T -> 'U signature) which we can then match with our el1 (first element to swap) and el2 (second element to swap). If we hit either of those simply return the other element.

But then, this isn't enough. What if we have several pairs to swap? We have to call swap a1 b1 arr then a swap a2 b2 arr, which could have a substantial performance penalty.

We could define a swapMany function which would take a list of tuples which are the first and second elements to swap and then handle them.

let swapMany els arr =
    arr |> Array.mapi (fun i v ->
        match els |> List.tryFind (fun el ->
            match el with
            | (el1, el2) when el1 = i || el2 = i -> true
            | _ -> false) with
        | Some (el1, el2) when el1 = i -> arr.[el2]
        | Some (el1, el2) when el2 = i -> arr.[el1]
        | _ -> v)

Now we handle all the swaps in one pass. If they're ordered, then the cost is even lower.

let values = [|1..10|]
let value =
    values
    |> swapMany [(4, 8); (2, 5)]

Result:

[|1; 2; 6; 4; 9; 3; 7; 8; 5; 10|]

But we're still not done, let's say we want to write our swapMany such that it doesn't create the entire result array, we want to stream it, sort of like an IEnumerable in C#.

In F# this isn't nearly as hard as it seems, all we have to do here is rewrite our swapMany as a sequence generator, which turns it into:

let swapMany els arr =
    seq {
        for i in 0..(Array.length arr - 1) ->
            match els |> List.tryFind (fun el ->
                match el with
                | (el1, el2) when el1 = i || el2 = i -> true
                | _ -> false) with
            | Some (el1, el2) when el1 = i -> arr.[el2]
            | Some (el1, el2) when el2 = i -> arr.[el1]
            | _ -> arr.[i]
    }

Now, you might say "why would I want to write a sequence generator for an array?" Consider you have a huge array:

let values = [|1..100000000|]

And you want to swapMany on it, but you're then going to enumerate the array for the first 10 values:

let value =
    values
    |> swapMany [(4, 8); (20, 50)]
printfn "%A" (value |> Seq.take 10 |> Seq.toArray)

You don't have to swap everything in the array now, you just have to swap the elements you need.

Second, the only issue I see with your code is the order of arguments. When writing functions it's nice to be able to use the pipe-right operator (|>) to push the left value as a parameter to the function on the right. In the case of your function, if I were to try to pipe an array to the right, it won't work. This is because the pipe-right (or pipe-forward) operator pipes the left side as the last argument to the right side.

let swap (array: 'a[]) (first: int) (second: int): 'a[] =
    let originalFirst = array.[first]
    array.[first] <- array.[second];
    array.[second] <- originalFirst;
    array

[|1..10|] |> swap 4 5

The above code is invalid, because the argument 4 is expected to be the 'a[] array, and the argument 5 is the first int, and the array [|1..10|] is expected to be the second int argument.

let swap (first: int) (second: int) (array: 'a[]): 'a[] =
    let originalFirst = array.[first]
    array.[first] <- array.[second];
    array.[second] <- originalFirst;
    array

[|1..10|] |> swap 4 5

F# also includes a double-forward-pipe and triple-forward-pipe operator, which do the same thing but pipe a tuple of either two or three values, respectively, to the right side as the last two or three arguments, respectively.

Consider an example:

let append str1 str2 str3 = str1 + "." + str2 + "." + str3
printfn "%s" ("ghi" |> append "abc" "def")
printfn "%s" (("def", "ghi") ||> append "abc")
printfn "%s" (("abc", "def", "ghi") |||> append)

All three of the printfn statements will print abc.def.ghi.

You can see more of this looking through the framework. You'll see that most functions in List and Seq take the list or sequence as the last parameter, so that you can pipe the result of one function to another.

Consider how much more painful the following code would have been to write had the framework designers chosen to not take this factor into account:

let values = [1..100]
let value =
    values
    |> List.map (fun v -> Math.Sin(float v))
    |> List.choose (fun v -> if v >= 0.0 then Some v else None)
    |> List.rev
    |> List.takeWhile (fun v -> v > 0.05)
    |> List.fold (fun v acc -> acc + v) 0.0
printfn "%f" value


Paint cost calculator This is the task I was given after completing my program. I don't have a clue what procedures and functions are to be honest. Therefore I have no clue on how to break down my program into different elements of it.

#paint calculator

RoomWidth = 0
RoomHeight = 0
RoomTotal = 0
RoomPaint = 0

RoomWidth = float(input("Enter the room width, it must be between 1m-25m: "))
RoomHeight = float(input("Enter the room height, it must be between 2m-6m: "))
if ((RoomWidth <=25 and RoomWidth >=1) and (RoomHeight <=6 and RoomHeight >=2)):

    RoomTotal = ((RoomWidth * RoomHeight)*4)

    PaintSelection = str (input("Please choose from a selection of paints: 'Luxury', 'Standard', or 'Economy'."))
    if PaintSelection == "Luxury":
        RoomPaint = RoomTotal * 1.45
    elif PaintSelection == "Standard" :
        RoomPaint = RoomTotal * 1.00
    elif PaintSelection == "Economy" :
        RoomPaint = RoomTotal * 0.45

    UndercoatAnswer = str (input("Would you like to use an undercoat? Y/N: "))
    if UndercoatAnswer == "Y":
        UndercoatingCost = (RoomTotal * 0.5)
    elif UndercoatAnswer == "N":
        UndercoatingCost = 0

    TotalCost = RoomPaint + UndercoatingCost
    print ("The total cost of painting this room is £",TotalCost,)

else:
    print ("Invalid input")

Functions are described in the Python tutorial - they are separate blocks of code which take defined inputs (argument values) and provide defined outputs (return values), and usually have a descriptive comment ("docstring") at the top:

def function(argument):
    """A simple function to double things."""
    output = argument * 2
    return output

As to how you can proceed on refactoring your code to use these functions, I suggest you take the following steps:

Step 1: Abstract repeated code into standalone functions:

def get_float_input(prompt, min_, max_):
    """Take valid float input between min_ and max_ from the user."""
    ...

room_width = get_float_input("Enter the room width, it must be between 1m-25m: ", 1, 25)
room_height = get_float_input("Enter the room height, it must be between 2m-6m: ", 2, 6)

You can use this community wiki to help you write the functionget_float_input.

Step 2: Use dictionaries to simplify multiple elifs, and continue to factor out repeats:

def get_str_input(prompt, choices):
    """Take valid string input from the user from items in choices."""
    ...

prompt = "Please choose from a selection of paints: 'Luxury', 'Standard', or 'Economy'."
paint_factor = {"Luxury": 1.45, "Standard": 1.0, "Economy": 0.45}
room_paint = room_total * paint_factor[get_str_input(prompt, paint_factor)]

prompt = "Would you like to use an undercoat? Y/N: "
undercoats = {"Y": 0.5, "N": 0}
room_undercoat = room_total * undercoats[get_str_input(prompt, undercoats)]

Step 3: Organise whole thing into sensible sub-functions that each do one thing:

def room_size():
    """Calculate the size of a room from user input."""
    width = get_float_input(...)
    height = get_float_input(...)
    return width * height

def paint_cost(size):
    """Calculate the cost of paint for a room."""
    prompt = "..."
    paint_factor = {...}
    return size * paint_factor[get_str_input(prompt, paint_factor)]

def undercoat_cost(size):
    """Calculate the cost of undercoat for a room."""
    prompt = "..."
    undercoats = {...}
    return size * undercoats[get_str_input(prompt, undercoats)]

Step 4: Write one function to bring the rest together:

def main():
    size = room_size()
    total_cost = paint_cost(size) + undercoat_cost(size)
    print("The total cost of painting this room is £{0:.2f}".format(total_cost))

Step 5: Add a call at the bottom of your script to set the whole thing off:

if __name__ == "__main__":
    main()

(see this question if you don't know what that comparison does).

Throughout: Follow the Python style guide, PEP-0008.