A* implementation is not running efficiently but gets correct result I'm not entirely sure what code other people will need to see, if I miss something off that is important please leave me a comment and I will update it as soon as I possibly can.

I have coded an A* algorithm following the pseudo code from Wikipedia. The problem is, from a relatively short route I am getting about 180 loops through the while loop where as from other people I've heard that they only get about 30

@Override
public RouteInfoInterface GetRoute(String[] start, String[] end, int routetype) {
    int pos = vertex.GetItem(start[0], start[1]);
    if (pos == -1) {
        return null;
    }
    Vertex pStart = vertex.data[pos];
    pos = vertex.GetItem(end[0], end[1]);
    if (pos == -1) {
        return null;
    }
    Vertex pEnd = vertex.data[pos];
    Heap openSet = new Heap();   //known nodes
    openSet.Push(pStart);                 //only start initially
    openSet.heap[1].gScore = 0;
    openSet.heap[1].fScore = heuristicFormula(pStart, pEnd); //complete heuristic guess

    while (openSet.GetLastItem() != 0) {  //THIS IS THE LOOP GETTING EXCESSIVE INVOCATIONS
        Vertex current = openSet.Pop();   //get node in openSet with lowest fScore and remove it from the openSet
        if (current == pEnd) {
            eVector route = new eVector();
            while (current.whereFrom != null) {
                route.AddItem(current.whereFrom);
                current = current.whereFrom.from;
            }
            RouteInfoInterface rI = new RouteInfo(route);
            resetData();
            return rI;
        }

        current.closed = true; //add openSet item to closedSet

        for (int i = 0; i < current.edges.length; i++) { //for each neighbour
            double d = current.gScore + (current.edges[i].distance);    //calc tenative distance
            if (d >= current.edges[i].to.gScore) {
                continue;
            }
            if (current.edges[i].to.closed) { //if neighbour in closed set skip
                continue;
            }
            if (!checkOpenSet(openSet, current.edges[i].to)) { //discovered new node?
                openSet.Push(current.edges[i].to); //add it to openSet

            }
            current.edges[i].to.whereFrom = current.edges[i];
            current.edges[i].to.gScore = d;
            current.edges[i].to.fScore = current.edges[i].to.gScore + heuristicFormula(current.edges[i].to, pEnd);
        }
        openSet.BuildMinHeap();
    }
}

A note on the Heuristic, the distance data given is in meters, I divide by 1609.34 to put it into miles as I use miles for other sections of the program that are not shown here

private double heuristicFormula(Vertex from, Vertex to) {
    double result = (Math.sqrt(Math.abs((from.x - to.x) ^ 2 + (from.y - to.y) ^ 2)) / 1609.34);
    return result;
}

My algorithm does get the right route however I am trying to make the algorithm as fast as possible. Again if theres anything else people need then please let me know.

Oh, I see your problem all right.

The ^ operator does not do what you think it does.

So your heuristic is wrong. It is possible that for some cases it is inadmissible -- remember, a heuristic is required to under-estimate path length, and yours might sometimes over-estimate due to the error.  What's more likely though is that it is massively under-estimating, and your algorithm is little better than Dijkstra's Algorithm at finding the shortest path quickly.

Most of the time, your heuristic will give the square root of the Manhattan distance, which is going to be pretty small compared to the Euclidean distance.

The moral of the story is test your subsystems.  If you'd written a test that verifies that the heuristic gives the right answer, you'd have found the bug yourself.

(Also, there's no need to do an abs on the sum of two squares. It's already positive!)

(Also, make a constant -- it pains me to type this, but you are a Java programmer so you get the joy of SHOUTY_SNAKE_CASE -- called METERS_PER_MILE and set it to 1609. Don't litter your program with unexplained magical numbers.  Give them names.)


Java Palindrome - Time & Space Complexity Which of these three methods is the most efficient?

Also, what is the time & space complexity of each?

public class Palindrome {

    public static boolean isStringBased(String word) {
        if (word == null || "".equals(word)) {
            return false;
        }
        String lowerCasedWord = word.toLowerCase();
        int i = 0;
        int j = lowerCasedWord.length() - 1;

        while (i < j) {
            if (lowerCasedWord.charAt(i) != lowerCasedWord.charAt(j))
                return false;
            i++;
            j--;
        }
        return true;
    }

    public static boolean isPalindromeRecursive(String input) {
        if(input.length() == 0 || input.length() == 1) {
            return true;
        }
        if(input.charAt(0) == input.charAt(input.length()-1)) {
            return isPalindromeRecursive(input.substring(1, input.length()-1));
        }
        return false;
    }

    public static boolean isPalindromeIterative(String input) {
        boolean retValue = false;
        int length = input.length();
        for( int i = 0; i < length/2; i++ ) {
            if (input.charAt(i) == input.charAt(length-i-1)) {
                retValue = true;
            }
        }
        return retValue;
    }
}

First solution requires O(1) space and O(n) complexity.

The last one looks to be less than 100% efficient, because you could directly return false when you find out it is not a palindrome, but it continues until the loop ends.


Should I return an object wrapped by an article tag? Not sure if this is the right board for this topic but since it will affect my SEO i guess it's not that wrong to ask about this in here.

I just took on a website that lists some stores around your location.
You can enter some conditions and it will return a list of nearby stores.

I noticed that the List of stores will be returned as a
 and each store is represented by an
.

                               Ice Cream Store 1

                                                foo

                    foo

                               Ice Cream Store 2

                                                foo2

                    foo

            ....

Is this good code style? I thought
 should be represent ...well...articles about some topic like news or articles about a specific topic. Instead these articles are some stores which will change every few weeks. Whats the best way to represent a list like this?

Spec:

  The article element represents a complete, or self-contained, composition in a document, page, application, or site and that is, in principle, independently distributable or reusable, e.g. in syndication. This could be a forum post, a magazine or newspaper article, a blog entry, a user-submitted comment, an interactive widget or gadget, or any other independent item of content.

Complete and self-contained? Yes - it has information about foostore which doesn't require the rest of the document to make sense.
Composition? A work of music, literature, or art? Not really. It would work better if it included a review.
One of the below:

A post? No.
An article? No.
Blog entry? No.
User-submitted content? Is it?
Interactive? No.

So probably not, as long as it isn't user-generated reviews.


Animated scrolling when intra-page links are clicked I have the following code to get the ID of a   and go to their respective div
:

$('a[href=#certificados]').click(function(e){
    e.preventDefault();
    $("html, body").animate({ scrollTop: $('#certificados').offset().top }, 1000);
});

$('a[href=#team]').click(function(e){
    e.preventDefault();
    $("html, body").animate({ scrollTop: $('#team').offset().top }, 1000);
});

$('a[href=#house]').click(function(e){
    e.preventDefault();
    $("html, body").animate({ scrollTop: $('#house').offset().top }, 1000);
});

$('a[href=#contact]').click(function(e){
    e.preventDefault();
    $("html, body").animate({ scrollTop: $('#contact').offset().top }, 1000);
});

How could I optimize the code?

Get the "respective" div in a more general way from the link's href attribute:

$('a[href=#certificados], a[href=#team], a[href=#house], a[href=#contact]').click(function(e){
    e.preventDefault();
    $("html, body").animate({ scrollTop: $(this.hash).offset().top }, 1000);
    // this.hash would be equivalent to $(this).attr("href") in your case
});

Probably you also can use a much better selector now - maybe selecting those links by a common class.


How is my program for computing the Hardy-Ramanujan number? I'm just wanting feedback on the efficiency, design and layout of this code if possible please. I suspect it's weird having a constructor with nothing in. I'd be grateful if it could be explained how to modify the code to fit in with conventions,  especially in this respect, but also in others.

package cubes;

import java.util.ArrayList;

public class Cubes {

    ArrayList<Integer> sum = new ArrayList<Integer>();
    ArrayList<Integer> x = new ArrayList<Integer>();
    ArrayList<Integer> y = new ArrayList<Integer>();
    boolean found = false;
    int  cSum = 0, a = 0 , b = 0, c = 0, d = 0, k, floor;
    Integer cubeSum, X, Y, answer;
    double K, J, cs;
    int iterations;

    public static void main(String[] args) {

        Cubes c = new Cubes();
        c.find();
    }

    public Cubes()
    {

    }

    private void find()
    {
        int i = 3;

        K = (double)2;
        J = (double)1;
        cs = Math.pow(K,3.0)+Math.pow(J,3.0);
        cubeSum = new Integer((int) cs);

        sum.add(cubeSum);
        x.add(1);
        y.add(2);

        i = 4;

        while(found!=true)
        {
            floor = (int)Math.floor(i/2);

            for(int j = 1; j < floor+1; j++)
            {
                k=i-j;
                K = (double)k;
                J = (double)j;
                cs = Math.pow(K,3.0)+Math.pow(J,3.0);
                cubeSum = new Integer((int) cs);
                X = new Integer((int) j);
                Y = new Integer((int) k);

                for (int l = 0; l < sum.size(); l++)
                {
                    a = (int)x.get(l);
                    b = (int)y.get(l);
                    if(cubeSum.compareTo(sum.get(l))==0)
                    {
                        if (a != j && a != k && b != j && b != k)
                        {
                            c=j;
                            d=k;
                            System.out.println("The first positive integer "
                            + "expressible as the sum\nof 2 different positive"
                            + " cubes in 2 different ways\nis "
                            + cubeSum + " = " +a+"^3 + "+b+"^3 = "
                            + c+"^3 + "+d+"^3");
                            found=true;
                        }
                    }
                }

                sum.add(cubeSum);
                x.add(X);
                y.add(Y);
            }
            i++;
        }
    }
}

Discrete Mathematics

Discrete calculations should not be done using floating-point math.  You should be suspicious of any results contaminated by floating-point math.  The way to compute the cube of an integer is n * n * n.  For larger powers, do repeated squaring, or use BigInteger.pow() (which is slower, but immune to overflow).

Variable Usage

To be honest, I found your code extremely difficult to read, mainly due to the way you misuse variables.

Scope: You should declare and define variables as close as possible to the point of use.  If a variable only applies within a loop, declare and define it inside the loop.  If it's local to a function, define it as a local variable in the function.  Instead, you've used a lot of instance variables, which, for all practical purposes, are "global" (since you only have one class).  As you really only have one function, all of the variables should have been local!
Proliferation: You've declared 19 instance variables.  Surely fewer variables would suffice?  A human brain can keep track of about 7 items at a time.  You have cSum (an int), cubeSum (an Integer), and cs (a double) — which raises the question, how are they related?  (Answer: cSum is never used, and cs is just a temporary variable that shouldn't have been needed at all.)  Also, answer and iterations are never used.  You've got so many variables that you couldn't keep track of the dead ones to prune.
Cryptic naming: Without studying the code, can you remember what x, y, k, X, Y, K, J, and floor mean?  If not, then your variables are named too cryptically.
Unconventional naming: By connotation, I would expect x to represent a floating-point number, not a list of Integers.  Then, you've also got X, which is a boxed version of j.
Misuse for flow control: To terminate the loop, why do you set a variable and test for found != true elsewhere?  Just return after printing the result — you're done!

Language Disfluency

If you familiarize yourself with the Java language a bit more, you can express yourself less clumsily.

Prefer primitives: You should always prefer the primitives (e.g. int) to their boxed counterparts (Integer).  Code written using primitives reads better and performs better.  The boxed types exist mainly so that you can store them in collections such as ArrayList<Integer>.  Even then, the compiler will automatically box and unbox the values for you, so you don't have to say new Integer(n).
Type Promotion: The compiler will automatically perform widening conversions, such as promoting int to double, so casting is unnecessary in those cases.
Integer division: When dividing an integer by an integer, the result will also be integral (with truncation towards 0).  When you do (int)Math.floor(i/2), you're just promoting i / 2 to a double argument, rounding it to the same double, and casting the result back to an int.  It would have been sufficient to just do i / 2.

Time Out… Let's Rewrite!

Let's rewrite the code using the trivial simplifications mentioned so far.

import java.util.ArrayList;

public class Cubes {

    public static void main(String[] args) {
        new Cubes().find();
    }

    private void find()
    {
        ArrayList<Integer> sum = new ArrayList<Integer>();
        ArrayList<Integer> x = new ArrayList<Integer>();
        ArrayList<Integer> y = new ArrayList<Integer>();
        int a, b, c, d;

        sum.add((2 * 2 * 2) + (1 * 1 * 1));
        x.add(1);
        y.add(2);

        int i = 4;

        while(true)
        {
            for(int j = 1; j < i/2 +1; j++)
            {
                int k=i-j;
                int cubeSum = (k * k * k) + (j * j * j);

                for (int l = 0; l < sum.size(); l++)
                {
                    a = (int)x.get(l);
                    b = (int)y.get(l);
                    if(cubeSum == sum.get(l))
                    {
                        if (a != j && a != k && b != j && b != k)
                        {
                            c=j;
                            d=k;
                            System.out.println("The first positive integer "
                            + "expressible as the sum\nof 2 different positive"
                            + " cubes in 2 different ways\nis "
                            + cubeSum + " = " +a+"^3 + "+b+"^3 = "
                            + c+"^3 + "+d+"^3");
                            return;
                        }
                    }
                }

                sum.add(cubeSum);
                x.add(j);
                y.add(k);
            }
            i++;
        }
    }
}

I find the rewritten version slightly more readable than the original, though I still can't really understand it.

Now, back to our regularly scheduled programming…

Algorithm

Magic numbers: You start with x.add(1), y.add(2), and i = 4.  Where did those magic numbers come from?  They're not naturally part of the problem, and you didn't explain.
x vs. y: What do lists x and y represent?  How does a number end up on these lists, why would it appear on x rather than y, or y rather than x?
Commenting: What is the strategy of your code?  Please outline it in a comment.  Mathematical algorithms in general are complicated, and algorithms, more than anything else, need comments!
False proof: How do you know that you've found the minimum answer rather than just any answer?  I'm not convinced at all.  In fact, if you remove the return after System.out.println() and just let it run, you'll see that it prints 110808 = 6^3 + 48^3 = 27^3 + 45^3 before 110656 = 4^3 + 48^3 = 36^3 + 40^3.  Therefore, the fact that your program prints 1729 is luck, not skill.
Separation of concerns: If you force yourself to separate the code that does input/output from the code that does pure computation, then you will also force yourself to write well organized code.

Proposed Solution

Since your algorithm is bogus, we'll have to start from scratch.  I propose an algorithm in which we iterate through n = 1, 2, 3, 4, … until we find one that is the sum of two cubes.  Then we test if it is also expressible as a sum of another two cubes.  Since we're testing n in increasing sequence, we can be sure that the first result is indeed the minimum number meeting the criteria.

To test whether n is a sum of two cubes, we can successively try subtracting 13, 23, 33, 43, … and test whether the difference is also a perfect cube.  To test whether the difference is a perfect cube, we could take its cube root (floating point — nope!), or check it against a list of perfect cubes (better — raising integers to the third power is easy).  I've written a class called Cubes to help enumerate all perfect cubes.

There's a lot of verbose supporting code.  However, if you look at the heart of the solution, which is hardyRamanujanBreakdown(long n), you'll find it to be very readable.

I've also written it such that you can easily modify the program to find subsequent numbers that are representable as sums of two cubes in two ways.  If you let this run infinitely, it's vulnerable to overflow, but you'll have hit CtrlC long before that happens. ☺

It's also slower than your original, but the original strategy is flawed, so it doesn't count.

import java.util.Iterator;
import java.util.SortedSet;
import java.util.TreeSet;

//////////////////////////////////////////////////////////////////////

/**
 * An iterator of positive perfect cubes, starting from 1, and ignoring
 * overflow.
 */
class Cubes implements Iterator<Long> {
    /**
     * An "infinite" set of all perfect cubes.
     */
    private static SortedSet<Long> allCubes = new TreeSet<Long>();

    /**
     * Helper to generate more members of allCubes as needed.
     */
    private static Cubes allIter = new Cubes();
    static {
        allCubes.add(allIter.next());
    }

    public static boolean isCube(long num) {
        while (num > allCubes.last()) {
            allCubes.add(allIter.next());
        }
        return allCubes.contains(num);
    }

    public static int cubeRoot(long num) {
        while (num > allCubes.last()) {
            allCubes.add(allIter.next());
        }
        return 1 + allCubes.headSet(num).size();
    }

    public static final long cube(int num) {
        return (long)num * num * num;
    }

    private int i = 1;

    @Override
    public boolean hasNext() {
        return true;
    }

    @Override
    public void remove() {
        throw new UnsupportedOperationException();
    }

    @Override
    public Long next() {
        return cube(this.i++);
    }

    public Cubes dup() {
        Cubes moreCubes = new Cubes();
        moreCubes.i = this.i;
        return moreCubes;
    }
}

//////////////////////////////////////////////////////////////////////

public class HardyRamanujan implements Iterator<Long> {

    private long n;
    private long[] breakdown;

    public HardyRamanujan() {
        this.n = 1;
    }

    @Override
    public boolean hasNext() {
        // Ignores overflow
        return true;
    }

    @Override
    public void remove() {
        throw new UnsupportedOperationException();
    }

    @Override
    public Long next() {
        while (null == (this.breakdown = hardyRamanujanBreakdown(this.n))) {
            this.n++;
        }
        return this.n++;
    }

    /**
     * Returns the breakdown of the current Hardy-Ramanujan number n as an
     * array of four perfect cubes [a, b, c, d].  n = a + b = c + d.
     * a < c <= n/2 <= d < b.  Returns null if next() has never been called.
     */
    public long[] breakdown() {
        return this.breakdown;
    }

    private static long[] hardyRamanujanBreakdown(long n) {
        long a, b, c, d;

        // Exists n == a + b, where a and b are cubes?
        Cubes cubes = new Cubes();
        while ((a = cubes.next()) < n / 2) {
            if (!Cubes.isCube(b = n - a)) {
                continue;
            }

            // If so, exists n == c + d, where c and d are cubes, and c > a ?
            Cubes moreCubes = cubes.dup();
            while ((c = moreCubes.next()) <= n / 2) {
                if (!Cubes.isCube(d = n - c)) {
                    continue;
                }
                return new long[] { a, b, c, d };
            }
        }
        return null;
    }

    public static void main(String[] args) {
        HardyRamanujan hrIter = new HardyRamanujan();
        do {
            long hrNum = hrIter.next();
            long[] breakdown = hrIter.breakdown();
            System.out.format("%d = %d^3 + %d^3 = %d^3 + %d^3\n",
                              hrNum,
                              Cubes.cubeRoot(breakdown[0]),
                              Cubes.cubeRoot(breakdown[1]),
                              Cubes.cubeRoot(breakdown[2]),
                              Cubes.cubeRoot(breakdown[3]));
        } while (false); // Change to true to get more!
    }
}


What Data Annotations need to be shared/different between my Model & ViewModel to keep seperation of concerns? I read this question and answers, about if Display Annotations on model properties violates the separation of concerns between view and model.  And my interpretation of their answer was "No, not if you're using a ViewModel for Display Annotations".  But additional separation of concerns questions arose as I began implementing.

I have two questions but both are kind of related to each other. In both, I am wondering if I am violating the separation of concerns:

I'm using localized strings from my views/resources.resx in my Model for Display Attributes, does this violate the separation of concerns since it's a resource in my views folder intended for views?
Do I need a ViewModel for my Model to keep separation of concerns?  And if so, which Annotations need to go on my model's properties, and which need to go on the ViewModel properties?

I think I need to keep all the Display Annotations and Validation Annotations (like [required] and [range(x,y)]) that I have in my ViewModel. In my Model I think I need to keep just the Validation Annotations.  But, If this is true, then I have two classes to maintain Validation Annotations if I decide to add another property!?

The reason I think that I need Validation Annotations on both classes is so that the Model class can tell EF to put the correct constraints into database when I change schema with DbMigrations.  Also, so the ViewModel tells the browser to tell the user when he's putting an invalid value in.

The ViewModel

public class RestaurantReviewViewModel
{
    public int Id { get; set; }
    [Range(1, 10)]
    [Required]
    public int Rating { get; set; }

    [Required]
    [StringLength(1024)]
    [Display(Name = Resources.Description)]
    public string Body { get; set; }

    [Display(Name = Resources.UserName)]
    [DisplayFormat(NullDisplayText = "[Anonymous]")]
    public string ReviewerName { get; set; }
    public int RestaurantId { get; set; }
}

The attributes that I commented out in the next class are the attributes I think i need to remove for the Model.

The Model

public class RestaurantReviewModel
{
    public int Id { get; set; }
    [Range(1, 10)]
    [Required]
    public int Rating { get; set; }

    [Required]
    [StringLength(1024)]
    //[Display(Name = Resources.Description)]
    public string Body { get; set; }

    //[Display(Name = Resources.UserName)]
    //[DisplayFormat(NullDisplayText = "[Anonymous]")]
    public string ReviewerName { get; set; }
    public int RestaurantId { get; set; }
}

I'm using localized strings from my views/resources.resx in my Model
  for Display Attributes, does this violate the separation of concerns
  since it's a resource in my views folder intended for views?

I would probably say yes.  You are tying UI concerns in your business models.  Why does your model even care about the text that is used to display it?  It should be more concerned with buiseness rules and performing buiseness operations I think.  Let your UI layer/layers deal with presenting it to the user.

  Do I need a ViewModel for my Model to keep separation of concerns? And
  if so, which Annotations need to go on my model's properties, and
  which need to go on the ViewModel properties?

No I don't think you necessarilly need a viewmodel and there is plenty of debate on this on the internet.  However I personally like the viewmodel approach as it provides the flexibility of adding any UI specified fields I would like without the overhead of my view being tied directly to the buiseness model.  Of course this means also that you now have to have the rules copied twice like you mentioned.  However if you were using jquery validation this would be the case anyway (or so I believe).


Parsing XML with double nested tags using mindom I want to retrieve id and name per skill. It works but is it well done? I would like to stay with minidom but all advices will be appreciated.

# This is only part of XML that interesting me:
# <skill>
#   <id>14</id>
#   <skill>
#     <name>C++</name>
#   </skill>
# </skill>
# <skill>
#   <id>15</id>
#   <skill>
#     <name>Java</name>
#   </skill>
# </skill>

skills = document.getElementsByTagName('skill')

for skill in skills:
    try:
        id_ = skill.getElementsByTagName('id')[0].firstChild.nodeValue
        name = skill.getElementsByTagName('name')[0].firstChild.nodeValue

        my_object.create(name=name.strip(),
                         id=id_.strip())
    except IndexError:
        pass

This is probably as good as you can get with minidom.
However, consider ditching minidom--it's really only there when you absolutely, postively need some kind of DOM api and only have the standard library. Note the documentation for minidom.

Users who are not already proficient with the DOM should consider using the xml.etree.ElementTree module for their XML processing instead
Warning: The xml.dom.minidom module is not secure against maliciously constructed data. If you need to parse untrusted or unauthenticated data see XML vulnerabilities.

XML in Python is almost always processed with the ElementTree interface, not a DOM interface. There are many implementations of ElementTree including xml.etree.ElementTree (pure Python, in stdlib) and xml.etree.cElementTree (CPython, in stdlib), and lxml (third-party all-singing, all-dancing xml processing library that uses libxml2).
Here is how I would do this:
try:
    # On Python 2.x, use the faster C implementation if available
    from xml.etree import cElementTree as ET
except ImportError:
    # pure-python fallback
    # In Python3 just use this, not the one above:
    # Python3 will automatically choose the fastest implementation available.
    from xml.etree import ElementTree as ET

xmlstr = """<root>
<skill>
   <id>14</id>
   <name>C++</name>
 </skill>
 <skill>
   <id>15</id>
   <name>Java</name>
 </skill>
</root>"""

root = ET.fromstring(xmlstr)

def get_subelem_texts(elem, subelems):
    """Return {subelem: textval,...} or None if any subelems are missing (present but empty is ok)"""
    attrs = {}
    for sa in skillattrs:
        textval = skill.findtext(sa)
        if textval is None:
            return None
        attrs[sa] = textval.strip()
    return attrs

skillattrs = 'id name'.split()

for skill in root.find('skill'):
    args = get_subelem_texts(skill, skillattrs)
    if args is not None:
        my_object.create(**args)


Factorizing integers I'm working on this method in Java, which is giving me the factors of a number. I'll be using this method a lot, and I was wondering if there isn't a better way of doing it. Like, with just one loop. Although this code works great and it returns what I expect, I just want to know if there's a better workaround.

The method:

public static void main(String[] args) {
    System.out.print("Enter a positive number: ");
    Scanner scanner = new Scanner (System.in);
    int number = scanner.nextInt();
    int count;
    for (int i = 2; i<=(number); i++) {
        count=0;
        while (number % i == 0) {
            number /= i;
            count++;
        }
    if (count == 0) continue;
        System.out.println(i+ "**" + count);
    }
}

The output I'm expecting:

number = 288;
2**5
3**2

Even if looping from 2 to sqrt(number) is an optimization as suggested by palacsint, you would run better by looping as long as number > 1:

for (int i = 2; number > 1; i++) {
    ...
}

Since you are only working on relatively small numbers (Integer.MAX_VALUE or less), you should also consider using a prime number table instead of trying each and every divisor from 2 to sqrt(number). There are roughly 4.800 prime numbers less than sqrt(Integer.MAX_VALUE), which cover all possible factors. Keeping them in an array of ints and testing only those numbers as possible factors should also gain some performance.

Better factorizing algorithms than this brute force approach are not known.


c# Factory Pattern Generic Exception Handler using dot net core My task was to create exception handler on some system that has multiple types of database and big business behavior.

At first, I had done a switch/case that handled the exceptions by type using the Object.GetType() method. After some additional thought, I realized that the types of exceptions are endless --- which meant that, every time a new error is thrown, I would have to add more logic to the switch/case. That will make the switch/case so ugly.

So, I have done it by this way. I need your help if i can improve it more or do I need to change the implementation? Also, where are the weaknesses of this implementation?

IExceptionHandlerFactory

private readonly IHttpContextAccessor HttpContext;
private readonly ILogManager LogManager;
private Dictionary<ExceptionsType, Func<ExceptionHandler>> EntityTypeMapper =
    new Dictionary<ExceptionsType, Func<ExceptionHandler>>();

public ExceptionHandlerFactory(IHttpContextAccessor httpContext, ILogManager logManager)
{
    HttpContext = httpContext;
    LogManager = logManager;
    EntityTypeMapper.Add(ExceptionsType.EFException, () =>
        { return new EFExceptionHandler(HttpContext, LogManager); });
    EntityTypeMapper.Add(ExceptionsType.Exception, () =>
        { return new GenericExceptionHandler(HttpContext, LogManager); });
    EntityTypeMapper.Add(ExceptionsType.OracleException, () =>
        { return new OracleExceptionHandler(HttpContext, LogManager); });
}

public ExceptionHandler GetExceptionHandlerBasedOnType(ExceptionsType type)
{
    return EntityTypeMapper[type]();
}

ExceptionHandler

public abstract class ExceptionHandler : Exception
{
    private readonly ILogManager LogManager;

    public ExceptionsType ExceptionsType { get; set; }

    public abstract void Handle(Exception exception);

    public ExceptionHandler(ILogManager logManager)
    {
        LogManager = logManager;
    }

    public void AddLogForException<T>(
        T exception,
        IHttpContextAccessor httpContext,
        string description)
        where T : Exception
    {

        LogManager.AddLog()
    }

}

GenericExceptionHandler

public class GenericExceptionHandler : ExceptionHandler
{
    private readonly IHttpContextAccessor httpContext;

    public ILogManager LogManager { get; }

    public GenericExceptionHandler(
        IHttpContextAccessor httpContext,
        ILogManager logManager) : base(logManager)
    {
        this.httpContext = httpContext;
    }

    public override void Handle(Exception exception)
    {
        AddLogForException(exception, httpContext, string.Empty);
    }
}

Well, AddLogForException isn't doing anything with it's parameters, perhaps make it virtual or do something with them?

You could also add a rule engine stating if a exception is registered with a specific action then do that, else do default.

Your registration could be a dictionary or thread safe dictionary or dictionary

say your class would have a backing field like this:

Dictionary<exception, Action<Exception,IHttpContextAccessor,string>> dict = new Dictionary<exception, Action<Exception,IHttpContextAccessor,string>>();

and you can register items like this

public void Register(Exception ex, Action<Exception,IHttpContextAccessor,string> del)
{
   lock(this)
   {
      dict.Add(ex, del);
   }
}

public override AddLogForException<T>(
        T exception,
        IHttpContextAccessor httpContext,
        string description)
        where T : Exception
    {

       if(dict.TryGetValue(exception, out var delegate)
       {
           delegate.DynamicInvoke(new object[]{exception,httpContext,description});
       }
       else
           base.AddLogForException(exception,httpContext,description)
    }

This registration is similat to a swich statement only this is a way to manage the options in the consuming project and leaves room for updates as you go on a and adapt it on a case by case as long as your exceptions are granular enough that you can add a "rule" on them.

I have no access to VS now but I guess it will compile just fine. if you have an issue just send me comment and I will have a look for you.


Hackerrank's Sherlock and Array Challenge can be found here

  Problem Statement

  Watson gives Sherlock an array A of length N. Then he
  asks him to determine if there exists an element in the array such
  that the sum of the elements on its left is equal to the sum of the
  elements on its right. If there are no elements to the left/right,
  then the sum is considered to be zero.  Formally, find an i, such
  that, A1+A2...Ai-1 =Ai+1+Ai+2...AN.

  Input Format:  The first line contains T, the number of test cases. For
  each test case, the first line contains N, the number of elements in
  the array A. The second line for each test case contains N
  space-separated integers, denoting the array A.

  Output Format:  For each test case print YES if there exists an element
  in the array, such that the sum of the elements on its left is equal
  to the sum of the elements on its right; otherwise print NO.

  Constraints:

  \$1 \le T \le 10\$
  \$1 \le N \le 10^5\$
  \$1 \le Ai \le 2×10^4\$
  \$1 \le i \le N\$

I'm having timeout issues on 2 of the test cases

I have tried two different approaches. Both is of \$O(n^2)\$

First was a recursive approach:

public static boolean isEven(int[] arr, int index, int leftSum, int rightSum) {
        int i = index-1;
        int j = index+1;

        while(i > -1) {
            leftSum += arr[i--];
        }

        while(j < arr.length) {
            rightSum += arr[j++];
        }
        return (leftSum == rightSum) ? true : (index == arr.length-1) ? false : isEven(arr, index+1, 0, 0);
    }

Other one was with the use of Navigable map:

public static boolean isEven(NavigableMap<Integer, Integer> map) {
       int left = 0;
       int right = 0;
       int n = map.size();
       while(n-- > 0) {
           left = right = 0;
           for(Integer i : map.tailMap(n+1).values())  right += i;
           for(Integer j : map.headMap(n).values()) left += j;
           if (left == right) return true;
       }
       return false;
   }

Here is how I read the input:

public static void main(String[] args) {
        Scanner s = new Scanner(System.in);
        final int N = s.nextInt();

        for(int i = 0; i < N; i++) {
            int NN = s.nextInt();
            int[] arr = new int[NN];
            for(int j = 0; j < NN; j++) {
                arr[j] = s.nextInt();
            }
            System.out.println(isEven(arr, 0, 0, 0) ? "YES" : "NO");
        }
    }

To avoid an \$O(n^2)\$ solution, I can't check every element in the array, or can I?

Your first example looks much nicer, so I'll focus on that.

First, let's remove the tail recursion in the most obvious way possible:

public static boolean isEven(int[] arr, int index, int leftSum, int rightSum) {
    while (true) {
        int i = index-1;
        int j = index+1;

        while(i > -1) {
            leftSum += arr[i--];
        }

        while(j < arr.length) {
            rightSum += arr[j++];
        }

        if (leftSum == rightSum) {
            return true;
        }
        if (index == arr.length-1) {
            return false
        }

        index += 1;
        leftSum = 0;
        rightSum = 0;
    }
}

Now let's clean it up:

public static boolean isEven(int[] arr) {
    for (int index = 0; index < arr.length; index++) {
        int leftSum = 0;
        for (int i = index -  1; i > -1; i--) {
            leftSum += arr[i];
        }

        int rightSum = 0;
        for (int i = index+1; i < arr.length; i++) {
            rightSum += arr[i];
        }

        if (leftSum == rightSum) {
            return true;
        }
    }

    return false;
}

We can combine the sum variables:

public static boolean isEven(int[] arr) {
    for (int index = 0; index < arr.length; index++) {
        int difference = 0;

        for (int i = index -  1; i > -1; i--) {
            difference += arr[i];
        }
        for (int i = index+1; i < arr.length; i++) {
            difference -= arr[i];
        }

        if (difference == 0) {
            return true;
        }
    }

    return false;
}

Now consider that we don't need to recalculate difference each time. If one iteration we have

|-A-| * |--B--|
1 2 3 4 5 6 7 8

The next we have

|-A-|++ --|-B--|
1 2 3 4 5 6 7 8

Namely, we add 4 to A and subtract 5 from B, which means we add 4 and 5 to the difference. We should also check for empty arrays.

public static boolean isEven(int[] arr) {
    if (arr.length == 0) {
        // Alternatively, return false since there
        // is no element that satisfies the condition.
        throw new IllegalArgumentException();
    }

    int difference = arr[0] - Arrays.stream(arr).sum();
    if (difference == 0) {
        return true;
    }

    for (int i = 1; i < arr.length; i++) {
        difference += arr[i-1];
        difference += arr[i];

        if (difference == 0) {
            return true;
        }
    }

    return false;
}


Simple file-renaming program in python I’m learning python and have built a very simple program that just renames files. It makes all letters lowercase and replaces spaces and %20-instances with underscores and then copies them into a ‘Finished’ folder. I’m looking for better methods and approaches of writing and structuring my code.
Can anybody find any obvious imperfections or methods of improvement? If anyone has any advice then that would be great.
My code can be found below:

import os
from shutil import copyfile

files_to_improve_dir = 'Files to Improve'
finished_files_dir = 'Finished'

print('isdir', os.path.isdir(files_to_improve_dir))

# 1 - copy files into 'finished' folder
for f in os.listdir(files_to_improve_dir):
    copyfile(os.path.join(files_to_improve_dir, f),
             os.path.join(finished_files_dir, f))

for f in os.listdir(finished_files_dir):
    new_filename = f.replace(' ', '_').replace('%20', '_').lower()
    print('f', f, 'new_filename', new_filename)
    if (f != new_filename):
        os.rename(os.path.join(finished_files_dir, f),
                  os.path.join(finished_files_dir, new_filename))

Try using pathlib over os when you're dealing with file/glob operations. It has a lot cleaner interface, and is part of standard library in python 3.
Name constants in your code with the CAPITAL_SNAKE_CASE. It is recommended guideline from the PEP-8.
Split your code into individual functions doing a single task each.
Put the execution flow for your code inside an if __name__ == "__main__" block.


Find string length using pointers I'm trying to write a function that will work out the length of two string char arrays, using pointer arithmetic. It seems to be working but I'm still getting used to pointers and addresses etc and would be grateful if you could let me know what you think. If possible could tell me if my understanding is correct in thinking that, when finding the length of a string char array, you're actually finding how many bytes it is? Thank you.

Here is the code:

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

int cmpstr(const char *,const char *);

int main()
{

    const char hello[10] = "hello";
    const char world[10] = "world";

    printf("%d", cmpstr(hello, world));

    return 0;
}

int cmpstr(const char *s1, const char *s2)
{
    int start1 = s1; // start address for s1 string

    while(*s1 != '\0')
    {
        s1++;        // increase address until the end
    }

    s1 = s1-start1;  // subtract start address from end address

    int start2 = s2; // start address for s1 string

    while(*s2 != '\0')
    {
        s2++;       // increase address until the end
    }

    s2 = s2-start2; // subtract start address from end address

    if(s1==s2)      // if s1 string is equal to s2 string
    {
        return 0;
    }
    else if(s1>s2)  // if s1 string is greater than s2 string
    {
        return 1;
    }
    else if(s1<s2)  // if s1 string is less than s2 string
    {
        return -1;
    }

}

type of difference between pointers

pointer_one - pointer_two

The result of this expression is a ptrdiff_t (§6.5.6/9).  Assigning it back to a pointer converts an integer type to a pointer, which IIRC should give you a compiler warning as it can lead to all kind of trouble.

relational operators with pointers

Comparing pointers with relative operators is undefined behavior if the pointers don't point into the same array (§6.5.8/5).

else if(s1>s2) // undefined behavior
{
  return 1;
}
else if(s1<s2) // undefined behavior
{
  return -1;
}

naming

cmpstr isn't really a suitable name for a function that compares the lengths of two strings.

comments

  // if s1 string is equal to s2 string

You don't compare strings for equality.  You (try to) compare their lengths.

Also, your comments should explain why you wrote the code the way you did. Don't tell what the code is doing. With well written code that should be self apparent.

do one thing per function

cmpstr calculates the length of two strings (code duplication!) and then compares these.  That's too much.  Delegate that length calculation to another function: strlen.  Or - for learning purpose only - write your own replacement.

ptrdiff_t vs size_t

Wondered why strlen returns a size_t instead of a ptrdiff_t?  That's because a string might be longer than the maximum difference that can be represented by ptrdiff_t.  Thus end - start may actually be undefined behavior when done with really long strings.  Better keep a size_t counter that you increase while searching for the '\0'.

program output should end with a '\n'

This is only minor, but otherwise it may corrupt the controlling terminal a bit.  Or like in ideone drop the output completely.

printf("%d\n", cmpstr(hello, world));
//        ^^

test your code

You really should!  Every (in an ideal world) piece of code that you write should be tested. You might want to look into TDD (test driven development) and build up good habits from the start.

making it faster

Do you really need to obtain the length of both strings?  Take these two strings as example:

""
"the quick brown fox did something but I don't remember what"

When you start at the first char of each of those, at how many chars do you need to look to determine which string is longer?


Python code to encrypt and email PDF file using PyPDF2 This is my first programming project with real world application.  It's purpose is to take a file from a directory, encrypt it with a predetermined password, and email to appropriate recipient.
ID Email and Password are found in a CSV file that I created EmailDict from.
ID and Filename are parsed and dropped into FileDict.  Any feedback is valued and appreciated.

import os
import re
import csv
import PyPDF2
import smtplib
from email.mime.text import MIMEText
from email.mime.application import MIMEApplication
from email.mime.multipart import MIMEMultipart

# Create a dictionary with the csv values for ID, Email and Password
EmailDict = dict()
with open('commissionrepemaillist.csv', 'r') as infile:
    reader = csv.reader(infile)
    for row in reader :
        REP = row[0]
        EMAIL = row[1]
        PASSWORD = row[2]
        EmailDict[REP] = EMAIL, PASSWORD

# create dictionary of IDs and Pdf Files
FileDict = dict()
path = "C:\\Apps\\CorVu\\DATA\\Reports\\Monthly Commission Reports\\Output\\pdcom1"
for FILE in os.listdir(path):
    split = re.split("[_.]", FILE)
    ID = split[1]
    FileDict[ID] = FILE

# encrypt PDF files in FileDict based on ID and Password from EmailDict
for ID in FileDict:
    if ID in EmailDict:
        path = "C:\\Apps\\CorVu\\DATA\\Reports\\Monthly Commission Reports\\Output\\pdcom1\\"
        file = os.path.join(path + FileDict[ID])

        inputStream = PyPDF2.PdfFileReader(file)
        output = PyPDF2.PdfFileWriter()
        output.appendPagesFromReader(inputStream)
        output.encrypt(EmailDict[ID][1])

        with open(file, 'wb') as outputStream:
            output.write(outputStream)

    else : continue

# email encrypted pdf file to appropriate rep

for ID in FileDict:
    path = "C:\\Apps\\CorVu\\DATA\\Reports\\Monthly Commission Reports\\Output\\pdcom1\\"
    file = os.path.join(path + FileDict[ID])
    with open(file, 'rb') as pdf :
        pdfAttachment = MIMEApplication(pdf.read(), _subtype = 'pdf')
        pdfAttachment.add_header('content-disposition', 'attachment', filename=('MonthlyPaidCommission_%s.pdf' % ID))
        text = MIMEText(
        """For Rep Number : %s
        Please use the password previously emailed to open attached file.
        If you have any questions or need assistance please contact  me.
        Thank you""" % ID)
    msg = MIMEMultipart(_subparts = (text, pdfAttachment))
    msg['SUBJECT'] = "Commission Report"
    msg['FROM'] = "me@gmail.com"
    msg['TO'] = EMAIL

    s = smtplib.SMTP("LOCALHOST:587")
    s.starttls()
    s.login('user', 'pwd')
    s.sendmail('me@123.com', EMAIL, msg.as_string())
    s.quit()

Your code is easy enough to follow.  All of your open() calls are done using with blocks, which is a good habit.

Your variables — such as REP and EmailDict — are unconventionally named.  Use ALL_CAPS for constants, and lower_case for variables.  Ironically, the one variable that should be a constant, path, is defined several times, and not written as a constant.

Speaking of path, you are misusing os.path.join().  The whole point of the function is to ensure that the path delimiter is automatically inserted for you between the components.  So, you should write os.path.join(path, FileDict[ID]) instead.  Also, use raw strings to make your backslashes less cumbersome.

I doubt that there is any value in writing all of those encrypted versions of the files to disk.  You could just create a transient encrypted copy in memory, mail it off, and move on to the next recipient.  To accomplish that, you can write to a StringIO object instead of a file.

The code to encrypt the PDF and compose the mail message feels like it could belong logically together.  I'd define a function for that:

def message_for(unencrypted_pdf_path, rep_id, email, password):
    """
    Creates a MIME message for a rep containing an encrypted version
    of the specified PDF.
    """
    buf = StringIO()
    …
    encrypted_pdf = buf.getvalue()

    text = MIMEText(… % rep_id)

    pdf_attachment = MIMEApplication(encrypted_pdf, _subtype='pdf')
    pdf_attachment.add_header('content-disposition', 'attachment', filename=('MonthlyPaidCommission_%s.pdf' % rep_id))

    msg = MIMEMultipart(_subparts=[text, pdf_attachment])
    msg['SUBJECT'] = 'Commission Report'
    msg['FROM'] = 'me'
    msg['TO'] = rep['EMAIL']
    return msg

Your code would be more expressive if you defined the dictionary entries "all at once" using a dict comprehension.  I would also avoid storing the e-mail and password as a tuple, since it makes you write more cryptic code like EmailDict[ID][1] later.

I think that you would be better off not defining FileDict at all.  Just process each directory entry as you encounter it.  You can even send multiple messages within the same SMTP connection.  Just make sure you keep a log of what was sent, in case something goes wrong.

PATH = r'C:\Apps\CorVu\DATA\Reports\Monthly Commission Reports\Output\pdcom1'

COLS = ['rep_id', 'email', 'password']
with open('commissionrepemaillist.csv', 'r') as infile:
    email_dict = {
        row[0]: dict(zip(COLS, row)) for row in csv.reader(infile)
    }

smtp = smtplib.SMTP('localhost:587')
smtp.starttls()
smtp.login(…)

for filename in os.listdir(PATH):
    rep_id = re.split('[_.]', filename)[1]
    rep = email_dict.get(rep_id)
    if rep is None:
        continue
    msg = message_for(os.path.join(PATH, filename), *rep)
    smtp.sendmail('me', rep['email'], msg.as_string())
    print("Sent %s to %s" % (filename, rep['email']))

smtp.quit()


Partitioning an array based on a condition in Javascript Using the array.filter function, I can efficiently pull out all elements that do or do not meet a condition:

let large = [12, 5, 8, 130, 44].filter((x) => x > 10);
let small = [12, 5, 8, 130, 44].filter((x) => !(x > 10));

However, in the example above, I'm iterating over the array twice and performing the same test each time. Is there a simple way to generate both 'large' and 'small' in a single pass over the array? In particular, if the callback to evaluate whether an element should be kept is expensive, I'd like to avoid calling it twice.

Using TypeScript/ECMAScript 6 syntax it can be achieved this way. I am not sure whether it's more or less elegant compared to the original variant, but

It does the job;
Requires only one run;
Can be further chained with map () or other functions.

const [small, large] =                             // Use "deconstruction" style assignment
  [12, 5, 8, 130, 44]
    .reduce((result, element) => {
      result[element <= 10 ? 0 : 1].push(element); // Determine and push to small/large arr
      return result;
    },
    [[], []]);                                     // Default small/large arrays are empty

More options can be found in various StackOverflow questions.


Simple speed up of C++ OpenMP kernel This function calculates the standard deviation of a patch, given a kernel size and greyscale OpenCV image. The middle pixel of the patch is kept if stdev of the patch is below the given threshold, else it is rejected. This is done for each pixel except the border.

I have never worked with OpenMP or optimization of C++, so all help is welcome. I'm probably doing some very stupid things that slow down the process drastically. It doesn't need to be the fastest, but I think some easy tricks will significantly speed it up.

#include "stdafx.h"
#include "opencv2/imgproc/imgproc.hpp"
#include "opencv2/highgui/highgui.hpp"
#include "opencv2/photo/photo.hpp"
#include <stdlib.h>
#include <stdio.h>
#include "utils.h"
#include <windows.h>
#include <string.h>
#include <math.h>
#include <numeric>

using namespace cv;
using namespace std;
Mat low_pass_filter(Mat img, int threshold, int kernelSize)
{
    unsigned char *input = (unsigned char*)(img.data);
    Mat output = Mat::zeros(img.size(), CV_8UC1);
    unsigned char *output_ptr = (unsigned char*)(output.data);

    #pragma omp parallel for
    for (int i = (kernelSize - 1) / 2; i < img.rows - (kernelSize - 1) / 2; i++){
        for (int j = (kernelSize - 1) / 2; j < img.cols - (kernelSize - 1) / 2; j++){
            double sum, m, accum, stdev;
            vector<double> v;
            v.reserve(kernelSize*kernelSize);
            // Kernel Patch
            for (int kx = i - (kernelSize - 1) / 2; kx <= i + (kernelSize - 1) / 2; kx++){
                for (int ky = j - (kernelSize - 1) / 2; ky <= j + (kernelSize - 1) / 2; ky++){
                    v.push_back((double)input[img.step * kx + ky]);//.at<uchar>(kx, ky));
                }
            }
            sum = std::accumulate(std::begin(v), std::end(v), 0.0);
            m = sum / v.size();

            accum = 0.0;
            std::for_each(std::begin(v), std::end(v), [&](const double d) {
                accum += (d - m) * (d - m);
            });

            stdev = sqrt(accum / (v.size() - 1));
            if (stdev < threshold){
                output_ptr[img.step * i + j] = input[img.step * i + j];
            }
        }
    }
    return output;
}

Vector v is not required.  Instead of adding items to it, iterate directly over your source array, and then use variance = E(v²) / E(v)² so that your inner code becomes:

        double sum = 0;
        int n = kernelSize * kernelSize;
        // Kernel Patch
        for (int kx = ...) {
            for (int ky = ...) {
                double d = (double)input[img.step * kx + ky]);
                sum += d;
            }
        }
        const double mean = sum/n;

        double sum2 = 0;
        for (int kx = ...) {
            for (int ky = ...) {
                double d = (double)input[img.step * kx + ky]);
                sum2 += (d - mean) * (d - mean);
            }
        }
        const double stddev = sqrt(sum2/n);
        if (stddev < threshold) {
            ...;
        }

After that, consider that the sum of elements centred around (x+1,y) can be found from the result for (x,y) simply by subtracting all the elements in the previous left-hand column, and adding all the elements in the new right-hand column.  An analogous operation works vertically.

Also, check your compiler options - are you auto-vectorizing loops, and using SIMD instructions (if available)?


Writing information from a List of KeyValuePairs into a DataRow I have the following algorithm which retrieves information in a List of KeyValuePair (prices) and write them in a DataRow. I was wondering if I could achieve the same result without j.

if (prices != null)
{
   for (int i = 8, j=0; j < prices.Count; i+=2, j++)
   {
      res[i] = prices[j].Value;
      res[i+1] = prices[j].Key;
   }
}

The ouput is a DataRow as follow :

res[8] = prices[0].Value
res[9] = prices[0].Key
res[10] = prices[1].Value
res[11] = prices[1].Key
res[12] = prices[2].Value
res[13] = prices[2].Key

etc.

I would approach this first by flattening the prices dictionary into a sequence of values eg { Key, Value, Key, Value, Key, Value }.

// This method is returning object for now since I'm not clear what types are in your data row..
IEnumerable<object> Flatten<K, V>(IEnumerable<KeyValuePair<K, V>> pairs)
{
    foreach (var pair in pairs)
    {
        yield return pair.Value;
        yield return pair.Key;
    }
}

Using this I can simply loop over the flattened dictionary and copy the values into the data row:

int offset = 8;
foreach (var x in Flatten(prices))
{
    res[offset++] = x;
}

I suspect there is a far better term than "flatten" for what I'm doing with the dictionary here.


Pythonic split list into n random chunks of roughly equal size As part of my implementation of cross-validation, I find myself needing to split a list into chunks of roughly equal size.

import random

def chunk(xs, n):
    ys = list(xs)
    random.shuffle(ys)
    ylen = len(ys)
    size = int(ylen / n)
    chunks = [ys[0+size*i : size*(i+1)] for i in xrange(n)]
    leftover = ylen - size*n
    edge = size*n
    for i in xrange(leftover):
            chunks[i%n].append(ys[edge+i])
    return chunks

This works as intended

>>> chunk(range(10), 3)
[[4, 1, 2, 7], [5, 3, 6], [9, 8, 0]]

But it seems rather long and boring. Is there a library function that could perform this operation? Are there pythonic improvements that can be made to my code?

import random

def chunk(xs, n):
    ys = list(xs)

Copies of lists are usually taken using xs[:]

    random.shuffle(ys)
    ylen = len(ys)

I don't think storing the length in a variable actually helps your code much

    size = int(ylen / n)

Use size = ylen // n // is the integer division operator

    chunks = [ys[0+size*i : size*(i+1)] for i in xrange(n)]

Why the 0+?

    leftover = ylen - size*n

Actually, you can find size and leftover using size, leftover = divmod(ylen, n)

    edge = size*n
    for i in xrange(leftover):
            chunks[i%n].append(ys[edge+i])

You can't have len(leftovers) >= n. So you can do:

   for chunk, value in zip(chunks, leftover):
       chunk.append(value)

    return chunks

Some more improvement could be had if you used numpy. If this is part of a number crunching code you should look into it.


C program to convert string to floating point From K&R:

  Extend atof to handle scientific notation of the form
  123.45e-6
  where a floating-point number may be followed by e or E and an optionally
  signed exponent.

I only added the part that handles the scientific notation. It compiles and works fine. I think it's way too long and could be shortened. I also don't like the names.

#include <stdio.h>
#include <stdlib.h>
#include <ctype.h>
#define MAX_SIZE 20

double atof(const char *s){
    int i;
    for(i = 0; isspace(s[i]); ++i);
        /*skip white space*/

    int sign;
    sign = (s[i] == '-')? -1 : 1; /*The sign of the number*/

    if(s[i] == '-' || s[i] == '+'){
        ++i;
    }

    double value;
    for(value = 0.0; isdigit(s[i]); ++i){
        value = value * 10.0 + (s[i] - '0');
    }

    if(s[i] == '.'){
        ++i;
    }

    double power;
    for(power = 1.0; isdigit(s[i]); ++i){
        value = value * 10.0 + (s[i] - '0');
        power *= 10.0;
    }

    if(s[i] == 'e' || s[i] == 'E'){
        ++i;
    }
    else{
        return sign * value/power;
    }

    int powersign; /*The sign following the E*/
    powersign = (s[i] == '-')? -1 : 1;

    if(s[i] == '-' || s[i] == '+'){
        ++i;
    }

    int power2; /*The number following the E*/
    for(power2 = 0; isdigit(s[i]); ++i){
        power2 = power2 * 10 + (s[i] - '0');
    }

    if(powersign == -1){
        while(power2 != 0){
            power *= 10;
            --power2;
        }
    }
    else{
        while(power2 != 0){
            power /= 10;
            --power2;
        }
    }

    return sign * value/power;
}

int main(void){
    char string[MAX_SIZE];

    fgets(string, sizeof(string), stdin);

    printf("%.9f", atof(string));

    return 0;
}

Thanks for including a test program - that's always valuable!

However, I'm going to change it, to parse a battery of test cases instead of reading from stdin:

#include <stdio.h>
int main(void)
{
    static const char *const strings[] = {
        /* these should parse fully */
        "12",
        "12.0",
        "08",                   /* not octal! */
        "+12.34",
        ".34",
        "\t \n2.",
        "1e0",
        "1e+0",
        "1e-0",
        "1.e4",
        ".1e-4",
        "-5e006",
        "-5e+16",
        "-.05",
        "-.0",
        "-1e6",
        /* these should parse only the initial part */
        "5c5",
        "10ee5",
        "0x06",                 /* not hex! */
        "--1" ,
        "-+1" ,
        "1e--4" ,
        "-1e.4",
        "1e 4",
        "1e-g",
        "", "foobar",           /* both 0 */
        " e5",                  /* also 0 */
        "-1e6",
        /* overflow/underflow */
        "1e500000",
        "1e-500000",
        "-1e500000",
        "-1e-500000",
    };

    static const int max = sizeof strings / sizeof strings[0];
    for (int i = 0;  i < max;  ++i)
        printf("%20s = > %.9g\n", strings[i], extended_atof(strings[i]));
}

(I changed the function name to extended_atof() so as to be safely distinct from the standard library atof().)

Your implementation passes all these tests.  Now we can look at refactoring.

Remove duplication

The things that we parse in multiple places are:

optional sign + or -
digit sequences

So perhaps we can refactor each of those into a function?  Instead of using an integer index into the supplied string, I prefer to just move the string pointer, and eliminate the need for i:

/* return true for positive, false for negative,
   and advance `*s` to next position */
static bool parse_sign(const char **s)
{
    switch (**s) {
    case '-': ++*s; return false;
    case '+': ++*s; return true;
    default: return true;
    }
}

Let's make use of that in the function:

double extended_atof(const char *s)
{
    /*skip white space*/
    while (isspace(*s))
        ++s;

    int sign = parse_sign(&s) ? 1 : -1; /*The sign of the number*/

    double value = 0.0;
    while (isdigit(*s))
        value = value * 10.0 + (*s++ - '0');

    if (*s == '.') {
        ++s;
    }

    double power = 1.0;
    while (isdigit(*s)) {
        value = value * 10.0 + (*s++ - '0');
        power *= 10.0;
    }

    if (tolower(*s) == 'e') {
        ++s;
    } else {
        return sign * value/power;
    }

    bool powersign = parse_sign(&s); /*The sign following the E*/

    int power2 = 0.0; /*The number following the E*/
    while (isdigit(*s))
        power2 = power2 * 10.0 + (*s++ - '0');

    if (powersign) {
        while (power2 != 0) {
            power /= 10;
            --power2;
        }
    } else {
        while (power2 != 0) {
            power *= 10;
            --power2;
        }
    }

    return sign * value/power;
}

It's slightly shorter, and it still passes all the tests.

Let's see if we can read digit strings in a function, and replace the three places we do that.  We'll make it update a count of how many digits wore parsed, so we don't lose leading zeros in the fractional part:

double extended_atof(const char *s)
{
    /*skip white space*/
    while (isspace(*s))
        ++s;

    int sign = parse_sign(&s) ? 1 : -1; /*The sign of the number*/

    double value = parse_digits(&s, NULL);

    if (*s == '.') {
        ++s;
        int d;                  /* digits in fraction */
        double fraction = parse_digits(&s, &d);
        while (d--)
            fraction /= 10.0;
        value += fraction;
    }

    value *= sign;

    if (tolower(*s) == 'e') {
        ++s;
    } else {
        return value;
    }

    bool powersign = parse_sign(&s); /*The sign following the E*/

    int power2 = parse_digits(&s, NULL); /*The number following the E*/

    double power = 1.0;
    if (powersign) {
        while (power2 != 0) {
            power /= 10;
            --power2;
        }
    } else {
        while (power2 != 0) {
            power *= 10;
            --power2;
        }
    }

    return value/power;
}

Tests still pass; what's next?

if (tolower(*s) == 'e') {
    ++s;
} else {
    return value;
}

This can be reversed, and if we're returning, it doesn't matter what we do to s:

if (tolower(*s++) != 'e')
    return value;

Here's some near-duplicate blocks:

double power = 1.0;
if (powersign) {
    while (power2 != 0) {
        power /= 10;
        --power2;
    }
} else {
    while (power2 != 0) {
        power *= 10;
        --power2;
    }
}

Dividing by 10 is the same as multiplying by 0.1, so we can move the test into the loop:

double power = 1.0;
while (power2 != 0) {
    power *= powersign ? 0.1 : 10;
    --power2;
}

We could go further, and capture powersign ? 0.1 : 10 into a variable.  We can also eliminate the power variable from here, and multiply value directly:

const double exponentsign = parse_sign(&s) ? 10. : .1;
int exponent = parse_digits(&s, NULL);

while (exponent--)
    value *= exponentsign;

Final version

Here's what I finished up with:

#include <ctype.h>
#include <stdbool.h>
#include <stdlib.h>

/* return true for positive, false for negative,
   and advance `*s` to next position */
static bool parse_sign(const char **const s)
{
    switch (**s) {
    case '-': ++*s; return false;
    case '+': ++*s; return true;
    default: return true;
    }
}

/* return decimal value of digits,
   advancing `*s` to the next character,
   and storing the number of digits read into *count */
static double parse_digits(const char **const s, int *const count)
{
    double value = 0.0;
    int c = 0;
    while (isdigit(**s)) {
        value = value * 10.0 + (*(*s)++ - '0');
        ++c;
    }
    if (count)
        *count = c;
    return value;
}

double extended_atof(const char *s)
{
    /*skip white space*/
    while (isspace(*s))
        ++s;

    const bool valuesign = parse_sign(&s); /* sign of the number */
    double value = parse_digits(&s, NULL);

    if (*s == '.') {
        int d;                  /* number of digits in fraction */
        ++s;
        double fraction = parse_digits(&s, &d);
        while (d--)
            fraction /= 10.0;
        value += fraction;
    }

    if (!valuesign)
        value = -value;

    if (tolower(*s++) != 'e')
        return value;

    /* else, we have an exponent; parse its sign and value */
    const double exponentsign = parse_sign(&s) ? 10. : .1;
    int exponent = parse_digits(&s, NULL);
    while (exponent--)
        value *= exponentsign;

    return value;
}

/* Test program */
#include <stdio.h>
int main(void)
{
    static const char *const strings[] = {
        /* these should parse fully */
        "12",
        "12.0",
        "08",                   /* not octal! */
        "+12.34",
        ".34",
        "\t \n2.",
        "1e0",
        "1e+0",
        "1e-0",
        "1.e4",
        ".1e-4",
        "-5e006",
        "-5e+16",
        "-.05",
        "-.0",
        "-1e6",
        /* these should parse only the initial part */
        "5c5",
        "10ee5",
        "0x06",                 /* not hex! */
        "--1" ,
        "-+1" ,
        "1e--4" ,
        "-1e.4",
        "1e 4",
        "1e-g",
        "", "foobar",           /* both 0 */
        " e5",                  /* also 0 */
        "-1e6",
        /* overflow/underflow */
        "1e500000",
        "1e-500000",
        "-1e500000",
        "-1e-500000",
    };

    static const int max = sizeof strings / sizeof strings[0];
    for (int i = 0;  i < max;  ++i)
        printf("%20s = > %.9g\n", strings[i], extended_atof(strings[i]));
}

There's still an opportunity for a small improvement: an extremely long fractional part could overflow double (this problem existed in your original).  Instead of returning a large value from parse_int(), you could consider always returning a fractional value in the range [0...1), and use the number of digits to scale up the integer parts.  Then we'd just end up with lost precision at the lower end.  That would look like:

static double parse_digits(const char **const s, int *const count)
{
    double value = 0.0;
    double increment = 0.1;
    int c = 0;
    while (isdigit(**s)) {
        value += increment * (*(*s)++ - '0');
        increment /= 10;
        ++c;
    }
    if (count)
        *count = c;
    return value;
}

The corresponding uses would be:

double extended_atof(const char *s)
{
    /*skip white space*/
    while (isspace(*s))
        ++s;

    int d;                  /* number of digits */
    const bool valuesign = parse_sign(&s); /* sign of the number */
    double value = parse_digits(&s, &d);
    while (d--)
        value *= 10;

    if (*s == '.') {
        ++s;
        double fraction = parse_digits(&s, NULL);
        value += fraction;
    }

    if (!valuesign)
        value = -value;

    if (tolower(*s++) != 'e')
        return value;

    /* else, we have an exponent; parse its sign and value */
    const double exponentsign = parse_sign(&s) ? 10. : .1;
    double exponent_f = parse_digits(&s, &d);
    while (d--)
        exponent_f *= 10;

    unsigned long exponent = exponent_f;
    while (exponent-->0)
        value *= exponentsign;

    return value;
}


Tower of Hanoi solver in Ruby Solved the classic Tower of Hanoi problem in Ruby, using recursion. Would love your feedback on this.

# Excellent explanation of the solution at
# http://www.mathcs.emory.edu/~cheung/Courses/170/Syllabus/13/hanoi.html

Move =
  Struct.new :disk, :from, :to do
    def to_s
      "Disk #{disk}: #{from} -> #{to}"
    end
  end

def spare_peg(from, to)
  # returns the peg that is not 'from' nor 'to'
  # e.g. if from="A", to="C" ... then spare="B"
  [*"A".."C"].each {|e| return e unless [from, to].include? e}
end

def hanoi(num, from, to)
  if num == 1 # base case
    return [Move.new(num, from, to)]
  end

  spare = spare_peg(from, to)
  moves = hanoi(num - 1, from, spare) # move everything to the spare peg
  moves << Move.new(num, from, to) # move the sole remaining disk to the 'to' peg
  moves += hanoi(num - 1, spare, to) # move all the disks on top of the 'to' peg
end

Sample output:

  puts hanoi(3, "A", "B").each {|move| move.to_s}

Disk 1: A -> B
Disk 2: A -> C
Disk 1: B -> C
Disk 3: A -> B
Disk 1: C -> A
Disk 2: C -> B
Disk 1: A -> B

Obviously this is one of those things which is opinionated but:

1) It might be simpler to use num.zero? as your base case

2) I would pass the spare tower as a parameter rather than calculating it all the time.

Something like:

def hanoi(num, from, to, spare)
  return [] if num.zero?                   # base case

  moves =  hanoi(num - 1, from, spare, to) # move everything to the spare peg
  moves << Move.new(num, from, to)         # move the sole remaining disk to the 'to' peg
  moves += hanoi(num - 1, spare, to, from) # move all the disks on top of the 'to' peg
end


Restructuring nested maps I am in the process of making an assembler and one of the needed steps is actually getting the opcode from a table. However, the instruction set I'm working on has a handful of different addressing modes which give different opcodes. On top of that, even if you have the same mode the parameter size changes the opcode as well. This means to get an opcode I need to provide an instruction, an addressing mode, and a size. I also need to be able to get the average size for an addressing mode for setting an estimated size for another portion of my code.

My first solution was to make a large map to hold all the values. This has turned into a 200+ line triple-nested map. Something like this:

// <Instruction, <Addressing Mode, <Size, Opcode>>>
std::map<std::string, std::map<AddressMode, std::map<int, int>>> OpcodeTable = {
  {"ADC", {{Indirect_X, {{2, 0x61}}},
          {Stack, {{2, 0x63}}},
          {Direct, {{2, 0x65}, {3, 0x6D}, {4, 0x6F}}},
          {Indirect_Long, {{2, 0x67}}},
          {Immediate, {{2, 0x69}}},
          {Indirect_Y, {{2, 0x71}}},
          {Indirect, {{2, 0x72}}},
          {Stack_Y, {{2, 0x73}}},
          {Indexed_X, {{2, 0x75}, {3, 0x7D}, {4, 0x7F}}},
          {Indirect_Long_Y, {{2, 0x77}}},
          {Indexed_Y, {{3, 0x79}}}}},
   // and so on for every instruction
};

Currently I access this table through a function called get_opcode which hides the ugliness of navigating this map.

// Checks whether a given value exists in the table or not
bool has_opcode(std::string& instr, AddressMode& mode, int size)
{
  return (detail::OpcodeTable[instr].count(mode) == 1) &&
         (detail::OpcodeTable[instr][mode].count(size) == 1);
}

uint8_t get_opcode(std::string& instr, AddressMode& mode, int size)
{
  if (has_opcode(instr, mode, size))
  {
    return detail::OpcodeTable[instr][mode][size];
  }

  // If an invalid instruction is found, throw an exception with details on what it found.
  throw InvalidInstructionException(
          "Invalid instruction: " + instr +
          " with size " + util::to_string(size) +
          " and mode " + AddressModeString[mode] +
          " does not exist.");
}

I have seen ideas for using a pair as a key, and that helps slightly. It effectively turns into this, but I am not sure if this is a notable improvement.

std::map<std::pair<std::string, AddressMode>, std::map<int, int>> OpcodeTable = {
  {{"ADC", Indirect_X}, {{2, 0x61}}},
  {{"ADC", Stack}, {{2, 0x63}}},
  {{"ADC", Direct}, {{2, 0x65}, {3, 0x6D}, {4, 0x6F}}},
  {{"ADC", Indirect_Long}, {{2, 0x67}}},
  {{"ADC", Immediate}, {{2, 0x69}}},
  {{"ADC", Indirect_Y}, {{2, 0x71}}},
  {{"ADC", Indirect}, {{2, 0x72}}},
  {{"ADC", Stack_Y}, {{2, 0x73}}},
  {{"ADC", Indexed_X}, {{2, 0x75}, {3, 0x7D}, {4, 0x7F}}},
  {{"ADC", Indirect_Long_Y}, {{2, 0x77}}},
  {{"ADC", Indexed_Y}, {{3, 0x79}}},
  // and so on for every instruction
};

Is there any cleaner way to structure this data? I don't need to avoid maps entirely, but I feel like I might have gone overboard with this setup.

Take your "pair" version to its logical conclusion:

using Mnemonic = std::string;
enum AddressMode { /* ... */ };
using Size = int;
using InstructionDescription = std::tuple<Mnemonic, AddressMode, Size>;
using Opcode = uint8_t;
std::map<InstructionDescription, Opcode> OpcodeTable = {
    {{"ADC", Indirect_X, 2},    0x61 },
    {{"ADC", Stack, 2},         0x63 },
    {{"ADC", Direct, 2},        0x65 },
    {{"ADC", Indirect_Long, 2}, 0x67 },
    {{"ADC", Immediate, 2},     0x69 },
    {{"ADC", Direct, 3},        0x6D },
    {{"ADC", Direct, 4},        0x6F },
    {{"ADC", Indirect_Y, 2},    0x71 },
    {{"ADC", Indirect, 2},      0x72 },
    {{"ADC", Stack_Y, 2},       0x73 },
    // ...
};

There will only be 256 rows in this table, so it's totally manageable.

Or, do what real assembler-writers do: read the instruction set specification to learn how the bits of the opcode correspond to the meaning of the instruction, and then turn that into a short mathematical formula instead of a huge lookup table.

For example:

enum AddressMode : uint8_t {
    Indirect_X = 0x01,
    Stack = 0x03,
    Direct = 0x05,
    Indirect_Long = 0x07,
    Immediate = 0x09,
    Indirect_Y = 0x11,
    Indirect = 0x12,
    Stack_Y = 0x13,
    // ...
};

Opcode get_opcode(Mnemonic instr, AddressMode mode, Size size)
{
    if (instr == "ADC") {
        unsigned size_mask = (size == 2) ? 0 : (size == 3) ? 8 : 10;
        return 0x60 + int(mode) + size_mask;
    } else if // ...
}

  I'd think having to search all the instructions every time (up to 3 times per instruction based on optimizations) instead of an O(1) lookup seems like it may increase the time taken to assemble the code which may be over 100k lines in some tests, but I don't have any data to back that up.

Lookups in a std::map are not \$O(1)\$; they're \$O(\log n)\$ and involve a ton of pointer traversals. Doing the same \$O(\log n)\$ operations on a sorted array (which is going to be contiguous in memory, therefore cache-friendly) will be faster — probably even faster than using some actually "\$O(1)\$" (cough handwave cough) yet still cache-unfriendly container such as std::unordered_map.

Anyway, my second code example above is what you'll want to do if you really need the speed (which I'm sure you don't). Simple arithmetic operations will always be lightning-fast compared to any kind of container lookup. But at some point, probably long before any of this starts mattering, the real performance bottleneck will be in how fast you can read that 100K of assembly code off the filesystem and how fast you can write the machine code back out. The code that turns mnemonics into opcodes isn't the bottleneck; I guarantee it.


Using attributes with properties in asp.net MVC I have an auto generated partial class in my Model folder with lots of properties and I need to modify some of the setters in these properties(annotate property), to ensure that I don't repeat myself in the future (after a refresh) I have another partial class with the same properties by use of attributes, where these properties cannot be modified, only via provided attributes.

So I have built a custom attribute class to modify those properties. Now my question is, am I doing this correctly or am I just creating a mess. Is there a better or another way to achieve my goal?
Please explain how to approach this delicate matter.

This is an auto generated class in Models folder:

public partial class Stud
{
    public string Property1{ get; set; }
    public string Property20{ get; set; }
}

This is the class for mine metadata with properties that I want to modify:

public class MetadataClasses
{

    public class StudMetadata
    {
        [Required, DisplayName("Property one"), UpperCaseAttribute]
        public string Property1 { get; set; }

        [Required, DisplayName("Property two"), ReplaceNrAttribute]
        public string Property2{ get; set; }
    }
}

This is an custom attribute class in different folder called Attributes:

public class UpperCaseAttribute : ValidationAttribute
{
    protected override ValidationResult IsValid(object value, ValidationContext validationContext)
    {
        try
        {
            validationContext
                .ObjectType
                .GetProperty(validationContext.MemberName)
                .SetValue(validationContext.ObjectInstance, value.ToString().ToUpper(), null);
        }
        catch (System.Exception)
        {
        }
        return null;
    }
}

This is an partial class named "PartialClasses.cs" in Models folder:

[MetadataType(typeof(MetadataClasses.StudMetadata))]
public abstract partial class Stud
{
}

I hope it's not too short for an answer but:

VALIDATION attributes MUST NOT manipulate and change the property they decorate.

Do it in the property logic or use some other kind of attribute for that (with your own logic) but do not misuse validation.

Working with generated code isn't easy in this case. It depends on how much freedom you have to instruct generator (for example using a template for setter you may easily achieve desired result). EF (in case you're using it) lets you customize .tt file (but that's usually a pain, I find easier to move to a code-first approach). If you don't have usable extension points you then you may give a chance to an AOP.

About your code, ignoring attribute usage, you should not catch Exception. In this case you don't know what may happen but at least you should ignore critical exceptions like OutOfMemoryException and similar.

try
{
}
catch (Exception e) when (IsNotCriticalException(e))
{
}

Where IsNotCriticalException() is a method you write to determine if exception can be ignored or it must escalate.

If value might be null, in this case you're throwing a NullReferenceException. Handle the case instead of throwing.

[UpperCaseAttribute] may simply be [UpperCase] or, better, [TransformToUpperCase] because it communicates what it will do.


Ways of forming multiple elements in a list comprehension without duplicates (Euler #109) I was working on Project Euler #109 (Potential spoilers for people who haven't done so yet); I got the correct solution, but I'm not satisfied with how one of my lines of code ended up looking, and was wondering if someone knows a better or more idiomatic way to do this.

singles = [1..20]++[25]
doubles = map (*2) singles
triples = map (*3) [1..20]

alls = singles ++ doubles ++ triples

oneshots = doubles
twoshots = [a+b | a <- doubles , b <- alls]
threeshots = [a+(alls!!b)+(alls!!c) | a <- doubles, b <- [0..((length alls)-1)], c <- [b..((length alls)-1)]]

main = do
    print $ length $ filter (< 100) $ (++oneshots) $ (++twoshots) threeshots

Specifically, the line:

threeshots = [a+(alls!!b)+(alls!!c) | a <- doubles, b <- [0..((length alls)-1)], c <- [b..((length alls)-1)]]

which gives me all of the unordered pairs of elements in alls, gives me pause.  If it was a simple range, then I could just do something like:

threeshots = [a+b+c | a <- doubles, b <- [0..100], c <- [b..100]]

and if the list was sorted and each element unique, I could even do:

threeshots = [a+b+c | a <- doubles, b <- alls, c <- alls, c < b]`

However, none of these hold true and my line ends up looking pretty cluttered.  What would people recommend as far as reformatting this line (or is it just something I'll have to deal looking at)?

Your threeshots line

Since using the operator !! is not very Haskelly, let's get rid of that and think of a way to get your b and c using typical list operations. Since you want a list of all pairs, let's create a new function called pairs.

pairs :: [a] -> [(a,a)]
pairs [] = []
pairs (x:xs) = map (\y -> (x, y)) (x:xs) ++ pairs xs

Your new threeshots would thus be:

threeshots = [a + b + c | a <- doubles, (b, c) <- pairs alls]

In a nutshell, pairs takes the first element of the given list, forms tuples with every element of that list and continues with the rest of the list until it's empty.

If you prefer, you could write pairs (x:xs) as map ((,) x) (x:xs) ++ pairs xs. I find that one to be less clear, though.

Other Nitpicks

As a rule of thumb, every top level declaration should have a type annotation.

You've declared your main in a do-block, which is not necessary since you're not using any monadic stuff. The (++oneshots) $ (++twoshots) threeshots part looks unusual, too.

main = print . length . filter (< 100) $ threeshots ++ twoshots ++ oneshots

You can keep the $ in your main if you prefer; in this scenario, it's probably just a matter of taste. Using . is less noise though - to me at least.

Putting it all together

singles, doubles, triples, alls :: [Int]
singles = [1..20] ++ [25]
doubles = map (*2) singles
triples = map (*3) [1..20]
alls    = singles ++ doubles ++ triples

pairs :: [a] -> [(a, a)]
pairs []     = []
pairs (x:xs) = map (\y -> (x, y)) (x:xs) ++ pairs xs

oneshots, twoshots, threeshots :: [Int]
oneshots   = doubles
twoshots   = [a + b | a <- doubles, b <- alls]
threeshots = [a + b + c | a <- doubles, (b, c) <- pairs alls]

main :: IO ()
main = print . length . filter (< 100) $ threeshots ++ twoshots ++ oneshots


Python decorator to support static properties Classes in Python do not have native support for static properties. A meta-class can rather easily add this support as shown below. Are there any problems programmers might experience if they use this implementation?

#! /usr/bin/env python3
class StaticProperty(type):
    def __getattribute__(cls, name):
        attribute = super().__getattribute__(name)
        try:
            return attribute.__get__(cls, type(cls))
        except AttributeError:
            return attribute

    def __setattr__(cls, name, value):
        try:
            super().__getattribute__(name).__set__(cls, value)
        except AttributeError:
            super().__setattr__(name, value)

class Test(metaclass=StaticProperty):
    __static_variable = None

    @property
    def static_variable(cls):
        assert isinstance(cls, StaticProperty)
        return cls.__static_variable

    @static_variable.setter
    def static_variable(cls, value):
        assert isinstance(cls, StaticProperty)
        cls.__static_variable = value

    def __init__(self):
        self.__value = None

    @property
    def value(self):
        assert isinstance(self, Test)
        return self.__value

    @value.setter
    def value(self, value):
        assert isinstance(self, Test)
        self.__value = value

def main():
    print(repr(Test.static_variable))
    Test.static_variable = '1st Hello, world!'
    print(repr(Test.static_variable))

    instance = Test()
    print(repr(instance.value))
    instance.value = '2nd Hello, world!'
    print(repr(instance.value))

    assert Test._Test__static_variable == '1st Hello, world!'
    assert instance._Test__value == '2nd Hello, world!'

if __name__ == '__main__':
    main()

My first inclination is that the property class should be sub-classed as static_property and should be checked for in StaticProperty.__new__ to ensure it is being used properly.

You've masked a bug, in __setattr__ a property raises an AttributeError if the setter hasn't been defined. This causes you to overwrite the property.
(As you've said) There's no distinction between a class property and an instance property. You can change it so there is, but it doesn't allow the property to only be defined on the class, not the instance.
You can just define the static properties on the metaclass. This removes a lot of the headaches.
If you really want to define everything onto the class not the metaclass then you can make the metaclass hoist the wanted functions into a new metaclass. This means everything works as if you only defined two metaclasses with the properties correctly defined.

No fancy changes:

class MyMeta(type):
    @property
    def class_(self):
        return self._class

    @class_.setter
    def class_(self, value):
        self._class = value

    @property
    def class_instance(self):
        return self._class_instance

    @class_instance.setter
    def class_instance(self, value):
        self._class_instance = value

class Test(metaclass=MyMeta):
    class_instance = MyMeta.class_instance

    @property
    def instance(self):
        return self._instance

    @instance.setter
    def instance(self, value):
        self._instance = value

Hoisting:

class classproperty(property):
    pass

class classinstanceproperty(property):
    pass

class StaticProperty(type):
    def __new__(self, name, bases, props):
        class_properties = {}
        to_remove = {}
        for key, value in props.items():
            if isinstance(value, (classproperty, classinstanceproperty)):
                class_properties[key] = value
                if isinstance(value, classproperty):
                    to_remove[key] = value

        for key in to_remove:
            props.pop(key)

        HoistMeta = type('HoistMeta', (type,), class_properties)
        return HoistMeta(name, bases, props)

class Test(metaclass=StaticProperty):
    @classproperty
    def class_(self):
        return self._class

    @class_.setter
    def class_(self, value):
        self._class = value

    @classinstanceproperty
    def class_instance(self):
        return self._class_instance

    @class_instance.setter
    def class_instance(self, value):
        self._class_instance = value

    @property
    def instance(self):
        return self._instance

    @instance.setter
    def instance(self, value):
        self._instance = value

These both pass the following tests: (I could only get your approach to work with instance and class instance)

test = Test()
test._instance = None
test.instance = True
assert test._instance is True
assert test.instance is True
test.instance = False
assert test._instance is False
assert test.instance is False

Test._instance = None
Test.instance = True
Test.instance = False
assert Test._instance is None
test._instance = True
if Test._instance is not True:
    print("instance can't be used after modifying class")

Test._class_instance = None
Test.class_instance = True
assert Test._class_instance is True
Test.class_instance = False
assert Test._class_instance is False

test = Test()
test._class_instance = None
test.class_instance = True
assert test._class_instance is True
assert Test._class_instance is False
test.class_instance = False
assert test._class_instance is False

Test._class = None
Test.class_ = True
assert Test._class is True
Test.class_ = False
assert Test._class is False

test = Test()
test._class = None
test.class_ = True
assert test._class is None
assert Test._class is False
test.class_ = False
assert test._class is None


factory pattern in spring I have the following factory class, wonder if this code can be implemented in better way. I saw few discussions about spring factory bean, but don't find good examples.

@service
public CustomerSearchFactory{
  @Autowired
  private CustomerSearchByID searchById;
  @Autowired
  private CustomerSearchByName searcyByName;
  @Autowired
  private CustomerSearchBySSN searchBySSN;

  public CustomerSearch getInstance(Request param){
    if (StringUtils.isNotBlank(param.getID()){
        return searchById;
    }else if (StringUtils.isNotBlank(param.getName()){
        return searcyByName;
    }else if (StringUtils.isNotBlank(param.getSSN()){
        return searchBySSN;
    }
  }
}

@service
public CustomerSearchByID implements CustomerSearch{
  @Autowired
  private Service1 service1;
  public Response search(Request request){
   --
   --
   ReturnResponse rs = service1.performSearch(inputRequest);
   --
   return response;
 }
}

@service
public CustomerSearchBySSN implements CustomerSearch{
  @Autowired
  private Service2 service2;
  public Response search(Request request){
   --
   --
   ReturnResponse rs = service2.performSSNSearch(inputRequest);
   --enter code here`
   return response;
 }
}

caller code:

CustomerSearch customerSearch= customerSearchFactory.getInstance(param)
response = customerSearch.search(request);

I am using the spring 3.1 framework and want to know if this code can be optimized.

I would advise not to use a different CustomerSearch per search criteria.
As your functionality will evolve you could need a CustomerSearch that is using the country the Customer is from.
But what if you want to search on the Country in combination with part of the name?
Soon you will be combining different search criteria together which will require one smart CustomerSearch bean.


Creating new team matches I have a class which creates a new match. My problem is that I am sure the code is unnecessary long and can get much improved (just keep in mind it is beginner level).

Specifically, I would like to know:

Can I change the below into 1 setter and 1 getter method?
I would like to use the rand() function for match ID can I do this inside the setter function of setMatchId or should it be done outside of the class?
When I upload the class to a db I assume I would need to create a new method and code the DB upload inside the method or should this be done outside of the class.

<?php
class match{
private $matchId;
private $team1;
private $team2;
private $venue;

function __construct($pMatchId, $pTeam1, $pTeam2, $pVenue){
    $this->matchId = $pMatchId;
    $this->team1 = $pTeam1;
    $this->team2 = $pTeam2;
    $this->venue = $pVenue;
}
    function setMatchId($pMatchId){
        $this->matchId = $pMatchId;

    }

    function getMatchId(){
        return $this->matchId;

    }
    function setTeam1($pTeam1){
        $this->team1 = $pTeam1;
    }
    function getTeam1(){
        return $this->team1;

    }

    function setTeam2($pTeam2){
        $this->team2 = $pTeam2;
    }
    function getTeam2(){
        return $this->team2;

    }
    function setVenue($pVenue){
        $this->venue = $pVenue;

    }
    function getVenue(){
        return $this->venue;

    }

} // c;lass match

$x = new match("1", "Patriots", "Chargers", "Newlands");
echo $x->getMatchId();
echo'
';
echo $x->getTeam1();
echo'
';
echo $x->getTeam2();
echo'
';
echo $x->getVenue();
?>

Can I change the below into 1 setter and 1 getter method?

You could use magic getters and setters, but you probably shouldn't (see the top-rated answer in that question; additionally, with the primitive approach, you would make all your private fields accessible from the outside).

You should also ask yourself if you even need setters. If the properties of the object never change after construction, you should make it immutable, as immutable objects are easier to handle (you know that they never change).

  I would like to use the rand() function for match ID can I do this inside the setter function of setMatchId or should it be done outside of the class?

You should not add a method setMatchId() that doesn't accept an argument, but just generates the id. Setters should set something, not generate something.

On the other hand, a method such as regenerateMatchId would be perfectly acceptable (although I'm not sure why you would want the id to be random).

  When I upload the class to a db I assume I would need to create a new method and code the DB upload inside the method or should this be done outside of the Class

By "upload to the db", you mean save in the database? Basically, you would have three options:

add the "save to db" method to this class
create a MatchDAO which saves matches to the db
use an ORM such as doctrine

Misc

class names should ideally start with an upper-case letter (see for example PSR-1)
your indentation is off, as is your vertical whitespace
what does the p in your argument names stand for? It's unclear, I would just remove it.


JUnit assertion for concurrency test I'm learning Java concurrency by a TDD approach. Therefore I wrote a basic test case in JUnit to test the behaviour / expected results of 2 threads. I'm looking for feedback for this code. In particular, the feedback for using CountDownLatch. I'm not sure if it is used correctly.

package io.mincongh.concurrency;

import static org.junit.Assert.assertEquals;

import java.util.Map;
import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.CountDownLatch;
import java.util.concurrent.TimeUnit;
import org.junit.After;
import org.junit.Before;
import org.junit.Test;

/**
 * @author Mincong Huang
 */
public class BasicConcurrencyTest {

  private CountDownLatch countDownLatch;
  private Map<Integer, Integer> map;

  @Before
  public void setUp() {
    countDownLatch = new CountDownLatch(2);
    map = new ConcurrentHashMap<>();
  }

  @After
  public void tearDown() {
    countDownLatch = null;
    map = null;
  }

  private class CounterThread extends Thread {

    @Override
    public void run() {
      for (int i = 0; i < 10; i++) {
        int prev = map.getOrDefault(i, 0);
        map.put(i, prev + 1);
        try {
          Thread.sleep(100);
        } catch (InterruptedException e) {
          e.printStackTrace();
        }
      }
      countDownLatch.countDown();
    }
  }

  @Test
  public void testCounterThread() throws InterruptedException {
    CounterThread t1 = new CounterThread();
    CounterThread t2 = new CounterThread();
    t1.start();
    t2.start();
    int tries = 0;
    while (countDownLatch.getCount() > 0 && tries < 3) {
      countDownLatch.await(1, TimeUnit.SECONDS);
      tries++;
    }
    assertEquals(2, (int) map.get(1));
  }

}

Monitor

I encourage you to think about critical code fragments and restrict concurrent access to it.

The critical code fragments are the following statements:

...
int prev = map.getOrDefault(i, 0);
map.put(i, prev + 1);
...

They have to be executed single-threaded. As you allow multithreaded execution here, two threads may execute "getOrDefault" directly in sequence thanks to ConcurrentHashMap. But they both will get 0! So your next put-statement will bring your model into an inconsistent state.

So the first thing to do is to define a correct Monitor:

public class CountAtIndex {

    private HashMap<Integer, Integer> map;

    public CountAtIndex() {
        map = new HashMap<>();
    }

    public synchronized void countAt(int index) {
        int previousValue = map.getOrDefault(index, 0);
        map.put(index, previousValue + 1);
    }

    public synchronized int get(int index) {
        return this.map.get(index);
    }

}

I do not use a ConcurrentHashMap as it is not necessary anymore. And it is wrong considering the usecase. Those datastructures are not that beneficial as they seem in the first place as you have to know the usecase to chose the proper datastructure to be synchronized.

Synchronizing is done via the synchronized method modifier. Both, the read access method and the modification access method allow only ONE thread to be in the monitor.

Some may argue to avoid the synchronized modifier because it will introduce a lock on a visible object. Other developers may use the object as a lock as well and a deadlock may happen. My point of view is: If you put a lock on an object that you do not "own" you should know what you are doing. And I think it is a wrong message to allow other developers to not know what they are doing.

Thread

The next to adapt is your thread. First you should define a Runnable instead of deriving from Thread directly. Furthermore the thread should know the usecase specific thread-safe datastructure:

private class CounterRunnable implements Runnable {

    private CountAtIndex monitor;

    public CounterRunnable(CountAtIndex monitor) {
        this.monitor = monitor;
    }

    @Override
    public void run() {

        for (int index = 0; index < 10; index++) {

            try {
                monitor.countAt(index);
                Thread.sleep(100);
            } catch (InterruptedException e) {
                e.printStackTrace();
            }

        }

    }

}

Then you can use the Runnable to create a Thread.

...
Thread counterThread = new Thread(new CounterRunnable(countAtIndex))
...

Test

The only thing to determine thread-safety is to acknowledge that your monitor works. All other things can be tested without considering concurrency.

To determine if a monitor is working is to "attack" the critical code fragments.

Write access

As mentioned above following statements are critical:

...
int prev = map.getOrDefault(i, 0); // (1)
map.put(i, prev + 1); // (2)
...

A correct test would be to ...

... run a thread executing statement 1 and stop

... run a second thread and check if it runs into the monitor lock

... if the second thread ran into the monitor lock -> continue with thread 1

... If the thread 1 leaves the monitor -> thread 2 should enter the monitor, execute the statements and leave the monitor.

Read access

Another issue is that the HashMap is not thread-safe anymore so the "get"-method of "CountAtIndex" must prevent modificiation access during read access.

Reading concurrently only is never a problem. So this is not an issue of concurrency.

To test this scenario you have to ...

... start a thread that is modifying (countAt) and stop before it will leave the monitor.

... start thread 2 that will query the index count (get). You have to check if it ran into the monitor lock.

... thread 1 then has to continue and should leave the thread

... thread 2 now should continue and get the proper value.