Google Code Jam - T9 Spelling with C++ I am trying to improve my coding skills. As a result, I solved the T9 Spelling question from the Google Code Jam dashboard. I will appreciate it if anyone could critique the code, and provide illustrations (if possible) of how I could make my code clean and simple. For me, simple is really beautiful.

Google judges the output of the code to be correct.  Here is the problem statement.

#include <iostream>
#include <vector>
#include <string>
#include <sstream>
#include <fstream>

using namespace std;
string process_ln(string line);

int main(){
    ifstream infile;
    ofstream outfile;
    infile.open("C-large-practice.in");
    outfile.open("C-large-practice-result.in");

    int i=0,n=1;
    int N;
    string line;
    vector<string> all;

    infile>>N;  //number of test cases
    while(i<N+1){
        if(i<1){
        getline(infile,line);
        i++;
    }
    else{
      string result;
      getline(infile,line);
      result = process_ln(line);
      all.push_back(result);
      outfile<<"Case #"<<n<<": "<<all[i-1]<<endl;
     n++,i++;
    }
}
return 0;
}

string process_ln(string line){
    int j;
    char letter;
    vector<string> val,temp;
    vector<char> v;
    string keep;

for(j=0; j<line.length(); j++)
    v.push_back(line[j]);

for(int i=0; i<j; i++){
switch(v[i]){
    case ' ': val.push_back("0"); temp.push_back("0"); break;
    case 'a': val.push_back("2"); temp.push_back("2"); break;
    case 'b': val.push_back("2"); temp.push_back("22"); break;
    case 'c': val.push_back("2"); temp.push_back("222"); break;
    case 'd': val.push_back("3"); temp.push_back("3"); break;
    case 'e': val.push_back("3"); temp.push_back("33"); break;
    case 'f': val.push_back("3"); temp.push_back("333"); break;
    case 'g': val.push_back("4"); temp.push_back("4"); break;
    case 'h': val.push_back("4"); temp.push_back("44"); break;
    case 'i': val.push_back("4"); temp.push_back("444"); break;
    case 'j': val.push_back("5"); temp.push_back("5"); break;
    case 'k': val.push_back("5"); temp.push_back("55"); break;
    case 'l': val.push_back("5"); temp.push_back("555"); break;
    case 'm': val.push_back("6"); temp.push_back("6"); break;
    case 'n': val.push_back("6"); temp.push_back("66"); break;
    case 'o': val.push_back("6"); temp.push_back("666"); break;
    case 'p': val.push_back("7"); temp.push_back("7"); break;
    case 'q': val.push_back("7"); temp.push_back("77"); break;
    case 'r': val.push_back("7"); temp.push_back("777"); break;
    case 's': val.push_back("7"); temp.push_back("7777"); break;
    case 't': val.push_back("8"); temp.push_back("8"); break;
    case 'u': val.push_back("8"); temp.push_back("88"); break;
    case 'v': val.push_back("8"); temp.push_back("888"); break;
    case 'w': val.push_back("9"); temp.push_back("9"); break;
    case 'x': val.push_back("9"); temp.push_back("99"); break;
    case 'y': val.push_back("9"); temp.push_back("999"); break;
    case 'z': val.push_back("9"); temp.push_back("9999"); break;
    default : break;
      }//end switch
  if(i>0 && val[i]==val[i-1])
     keep +=" ";
  keep += temp[i];
   }//end for loop
   return keep;
}//end process_ln

I see several opportunities for simplification and improvement in your main() function:

int main(){
    ifstream infile;
    ofstream outfile;
    infile.open("C-large-practice.in");
    outfile.open("C-large-practice-result.in");

std::ifstream and std::ofstream both have constructors that take the filename to open as an argument. You can collapse these lines to:

    ifstream infile("C-large-practice.in");
    ofstream outfile("C-large-practice-result.in");

You don't need the all vector; see below for why.

    int i=0,n=1;
    int N;
    string line;
    vector<string> all;

This piece of code skips reads in the number of lines remaining in the file, then has special case handling to discard the remainder of the line:

    infile>>N;  //number of test cases
    while(i<N+1){
        if(i<1){
        getline(infile,line);
        i++;
    }

Instead of putting a special case inside the while-loop, move that std::getline() call to above the while loop. Then the whole thing simplifies to:

    infile>>N;  //number of test cases
    getline(infile,line); // Discard the rest of the line.

    while(i < N) {
    // Note: condition in the while() has changed because we don't have
    // the extra line at the start
        string result;
        getline(infile,line);
        result = process_ln(line);
        all.push_back(result);
        outfile<<"Case #"<<n<<": "<<all[i-1]<<endl;
        n++,i++;
    }

You're storing the processed line in a vector, but the only place you're using that vector is right on the next line, where you output the string that you just put in there. So you could simplify further to:

        string result = process_ln(line);
        outfile << "Case #" << n << ": " <<  result << endl;

This eliminates the need for the all vector, as mentioned above. Note that I've combined the declaration and initialization of result. This is generally a good habit to get into when using C++.

Another useful optimization to note is that the return value from std::getline() is the stream that is passed as its first argument, and streams have an implicit conversion to bool so they can be used as the conditional in while() or for() loops. If you use the return from getline() as the conditional part of a for() loop, you can declare the string in the same for() statement:

    int n = 1;
    for(string line; getline(infile, line); ++n) {
        string result = process_ln(line);
        outfile << "Case #" << n << ": " << result <<endl;
    }

Then you don't need to declare i, line or all earlier in main()

The return at the end of main() is not necessary in C++; if you omit it, the compiler will generate code for the return 0 for you.

return 0;
}

In the process_ln() function, there are also some opportunities for improvement:

string process_ln(string line){
    int j;
    char letter;
    vector<string> val,temp;
    vector<char> v;
    string keep;

std::string has an indexing operator, so you don't need to extract each character from line here, you can simply index into it in the switch statement below and remove this code:

for(j=0; j<line.length(); j++)
    v.push_back(line[j]);

The C++ standard library has the std::ostringstream class which is very handy for building up strings one piece at a time, so in the big switch statement, where you've got temp.push_back("....") in your code, you could replace it with:

std::ostringstream  keep;
/// process the line, building it up a piece at a time.
return keep.str();

In the for loop, you're storing all the digits that you use in the val vector. You really only need the current and previous digits to keep track of this. You're also storing all the fragments for each digit, where you really only need to keep one. Since your original code also swallowed invalid input, I'm doing the same here:

        std::ostringstream  keep;

        // Note: initialized to an invalid digit value to guarantee that we|
        // don't inject a space at the start of the output string.
        char    last_digit = 'X';
        char    this_digit;
        string  digits;

        for(int i = 0; i < line.length(); i++){

        switch(line[i]){
            case ' ': this_digit = '0'; digits = "0"; break;
            case 'a': this_digit = '2'; digits = "2"; break;
            case 'b': this_digit = '2'; digits = "22"; break;
            /// and so on for the rest of the letters.
            default : this_digit = 'X'; digits = ""; break;
        }//end switch

        if(this_digit == last_digit)
        {
            keep << " ";
        }
        keep << digits;
        last_digit = this_digit;
    }//end for loop
    return keep;
}//end process_ln

Note that if you follow all of my advice, you won't need any of the variables j, letter, val, temp or v declared at the start of the function (though I admit I did introduce a few new ones).


SpringData: filtering fields of Entity in Repository My entity is Office. In office repository I would like to find all office Addresses:

  default List<Address> findOfficeLocations() {
    Iterable<Office> offices = findAll();
    return StreamSupport.stream(offices.spliterator(), false)
        .map(Office::getOfficeLocation).distinct().collect(Collectors.toList());
  }

Is it good practice or better to do it in the Service?

I think that this is not a good practice, but it still may be acceptable in some situations.

The reasons to avoid this:

1) default methods are not intended to introduce new functionality, but rather to provide backwards compatibility for older APIs. The official tutorial says:

  Default methods enable you to add new functionality to the interfaces of your libraries and ensure binary compatibility with code written for older versions of those interfaces.

2) It looks like you are using JPA. In a Spring repository, you can achieve the same result by using @Query, without detailing the implementation:

@Query("SELECT DISTINCT ol.addresses FROM OfficeLocation ol ...") // TODO complete the HQL
List<Address> findOfficeLocations();

3) If it cannot be done with a query, you are right, a better place for it is a service.


RSS to HTML script - version two I wrote this script that downloads an RSS feed and converts it to HTML. One of my only concerns is my variable naming because I suck at naming things.

# -*- coding: utf-8 -*-
"""Simple RSS to HTML converter."""

__version__ = "0.0.2"
__author__ = "Ricky L Wilson"

from bs4 import BeautifulSoup
from feedparser import parse as parse_feed

TEMPLATE = u"""

{title}

{title}
{summary}
"""

def entry_to_html(**kwargs):
    """Formats feedparser entry."""
    return TEMPLATE.format(**kwargs).encode('utf-8')

def convert_feed(url):
    """Main loop."""
    html_fragments = [entry_to_html(**entry) for entry in parse_feed(url).entries]
    return BeautifulSoup("\n".join(html_fragments), 'lxml').prettify()

def save_file(url, filename):
    """Saves data to disc."""
    with open(filename, 'w') as file_object:
        file_object.write(convert_feed(url).encode('utf-8'))

if __name__ == '__main__':
    save_file('http://stackoverflow.com/feeds', 'index.html')
    with open('index.html') as fobj:
        print fobj.read()

About the generated HTML

Each feed item should become an article. This would also allow you to include the generated HTML on any page without having to adjust the heading levels. Unless you know that h2 would always be the correct heading element in your case, you might want to use h1 instead.

You could give the a element the bookmark link type.

Unless you remove markup from the feed item’s description, you might want to use a div instead of a span, because a span can’t contain many elements that might appear in the description.

You could specify a class on the article element, and omit the classes on its child elements. Less potential to conflict with classes that might already be used in the document.

{title}

  {title}

{summary}


Generating all keypad possibilities I ran into this question on Stack Overflow. It looked like a really cool thing to try myself, being that I haven't done any recursion for ages (read at least 2 years).

var keypadPossibilities = (function()
{
    var keypad_mapping = [
        [],
        ['a','b','c'],
        ['d','e','f'],
        ['g','h','i'],
        ['j','k','l'],
        ['m','n','o'],
        ['p','q','r','s'],
        ['t','u','v'],
        ['w','x','y','z']
    ];

    function getPosibilities(keypad_mapping_indexes)
    {
        //if only 1 keypad mapping index: simply return the characters
        if ( keypad_mapping_indexes.length === 1)
        {
            return keypad_mapping[keypad_mapping_indexes.shift()];
        }

        //still multiple keypad mapping index left. lets do some magic
        var currentIndex = keypad_mapping_indexes.shift(), posibilities = [],
            characters = keypad_mapping[currentIndex];

        var charactersToGlueAtEnd = getPosibilities(keypad_mapping_indexes);

        for ( var i in characters )
        {
            for ( var j in charactersToGlueAtEnd )
            {
                posibilities.push(characters[i]+charactersToGlueAtEnd[j]);
            }
        }

        return posibilities;
    }

    return function(pressed_keys)
    {
        return getPosibilities(pressed_keys);
    }

})();

Usage would be:

keypadPossibilities([1,2,3]);

It works, so thats nice (no debugging required, that was a first). So, shoot, code remarks, performance remakrs, ... You can go full package. Everything to learn new cool stuff about JS.

Overall, I'd say recursion is a good way to go. Good use of an IIFE to keep things tidy as well. I do however have some concerns.

Input checking

Firstly, if I pass an empty array, I get a stack overflow, since you only check for a length of 1 - not for a length < 1. I am technically passing the correct type (an array) to the function, so it should handle it (if I pass something that's not an array, then it's my own fault).

Also, if I pass [9] (or [31415]) I just get undefined back, though an empty array would probably be more fitting.

shift() is destructive

Be careful with shift()! You're modifying the array that's been passed to the function, even though it doesn't really "belong to you". The caller might still have a use for it.

For instance,

var numbers = [1, 2, 3];
var words = keypadPossibilities(numbers);
console.log("You typed " + numbers.length + " digits, producing " + words.length + " possible words");

will print

You typed 0 digits, producing 27 possible words

Wait, zero digits? Thing is, numbers is suddenly empty; everything's been shifted out by passing it to keypadPossibilities.

This of course also happens when the function recurses, which could spell trouble. Luckily, in your case, you don't use the input array for anything after having recursed, but if you did you'd find that the recursion had truncated it down to empty.

Don't use for...in for arrays

for...in will iterate properties of an object, and with no guaranteed order. It usually works for arrays, but it's not a sure thing. It's semantically different from iterating the actual, indexed elements in the array. So use either a regular ol' for loop, or - if you're targeting modern runtimes, maybe a forEach().

However, you could also use map() and reduce() in this case (see below).

Other stuff

This function-wrapper around a call to getPosibilities is unnecessary:

return function(pressed_keys)
{
    return getPosibilities(pressed_keys);
}

(Oh, and there's a typo: getPosibilities is missing an extra "s". I'll just use the corrected name from hereon out)

You could simply replace the above with just:

return getPossibilities;

and done.

However, with the point about shift() in mind, the easiest thing to do would probably be something like

return function(pressed_keys)
{
    return getPossibilities(pressed_keys.slice());
}

Now, pressed_keys gets sliced (duplicated) before it's passed to getPossibilities.

You could also do the slicing in getPossibilities (and do the direct return, shown above), but - as the code's already proved - you don't really need it there. But in the interest of being thorough, it'd look something like:

var currentIndex = keypad_mapping_indexes[0]
// ... snip ...
var charactersToGlueAtEnd = getPossibilities(digits.slice(1)); // duplicate array from index 1 and up

A few notes on style

In terms of style, I'm not a fan of brace-on-new-line in JavaScript. Yes, it works, but it can bite you, since JS will sometimes do automatic semicolon insertion at newlines and break your code. So the convention is to use brace-on-same-line style.

Also by convention, all names in JS should be camelCase. You're using a bit of both; aim for consistency.

Here's a possible version incorporating the points above

var keypadPossibilities = (function() {
  var keypadMapping = [
    null,
    ['a','b','c'],
    ['d','e','f'],
    ['g','h','i'],
    ['j','k','l'],
    ['m','n','o'],
    ['p','q','r','s'],
    ['t','u','v'],
    ['w','x','y','z']
  ];

  function getPossibilities(digits) {
    var characters = keypadMapping[digits.shift()],
        charactersToGlueAtEnd;

    if(!digits.length) {
      return characters || [];
    }

    charactersToGlueAtEnd = getPossibilities(digits);

    return characters.reduce(function (memo, character) {
      var words = charactersToGlueAtEnd.map(function (string) { return character + string; });
      return memo.concat(words);
    }, []);
  }

  return function (digits) {
    return getPossibilities(digits.slice()); // make sure we work on a copy of the input array
  };
})();


Replace color in image measured by Euclidean distance This script replaces the red hair in image to black color (replace one color to another)

Three major parts in this script

Not just replace color red to black, I hope the output image looks natural, so I will replace the color close to red to black. This "close" is measured by Euclidean distance.
And not just replace by black color, user can modify the red color's r, g, b value separately, like add more green.
Choose the area to be changed, rather than change the whole picture

Suggestions I am looking for:

I did some NumPy practice before, so any suggestions about NumPy are welcome.
I think the code to replace target area is not so elegant, I am not quite sure about this part:

(c1, r1), (c2, r2) = area
for i in range(r1, r2+1):
    l, r = i*h+c1, i*h+c2+1
    data[l:r,:k][D[l:r]] = modification(data[l:r,:k][D[l:r]])

Full code:

import numpy as np
from PIL import Image
from scipy.spatial.distance import cdist

def replace_corlor(image, original_corlor, modification, area=None, distance=1000, output="new_test.jpg"):
    print("[*] START Replace Color")
    img = Image.open(image)
    data = np.asarray(img, dtype="int32")
    w,h,k = data.shape
    data = np.reshape(data, (w*h,k))
    distMatrix = cdist(data, np.array([original_corlor]))
    D = distMatrix<=distance
    D = np.reshape(D,w*h)
    if area is None:
        data[:,:k][D] = modification(data[:,:k][D])
    else:
        (c1, r1), (c2, r2) = area
        for i in range(r1, r2+1):
            l, r = i*h+c1, i*h+c2+1
            data[l:r,:k][D[l:r]] = modification(data[l:r,:k][D[l:r]])
    data = np.reshape(data, (w,h,k))
    img = Image.fromarray(np.asarray(np.clip(data, 0, 255), dtype="uint8"), "RGB")
    img.save(output)
    print("[*] DONE Replace Color")

if __name__ == "__main__":
    def modification(color):
        return color * [2,0,0]
        # return [255,0,0]
    replace_corlor("test.jpg",(36,35,30),modification, ((0,0),(400,100)))

Nice code.
Let's look at the function signatures.
As gbartonowen noted, two typos on corlor for color.
The keyword defaults are lovely,
thank you for appropriately dealing with a magic number, and defaulting the filespec.
Lots of keywords will lead to a somewhat long signature, though, as reported by $ flake8:

E501 line too long (106 > 79 characters)

Recommend you always run such a linter before sharing code.
And heed the linter's advice.
In the same vein we see things like PEP-8 wants spaces in the w, h, k assignment,
or (w * h, k) expression.
And, this being python rather than java,
snake-case identifiers of d and dist_matrix would be much more appropriate.
Python convention says the Gentle Reader should expect D to be a class.
(Yes, I know this clashes with conventions of the mathematical community,
something had to give. Ok, on to more substantive comments.)

An identifier of data is always on the vague side,
it's a bit like naming your variable the_thing.
Yes, it's accurate, but often it could be a little more informative.
Consider a rename along these lines:

    img = np.asarray(Image.open(image), dtype='int32')

(Hmmm, as I look at that, maybe we'd like identifiers of in_file and out_file?)

The cdist() call, which defaults to Euclidean, is pretty interesting,
since RGB is not a perceptually uniform color scheme.
Consider mapping to Lab* colorspace and using that for comparisons.

It is slightly tricky for a naive caller to correctly pass in area,
so it warrants mention in the docstring you're going to write,
or at least in a comment.
Similarly for the modification signature.
Consider raising an error if c1 < c2 or r1 < r2 does not hold.

The l, r identifiers are well chosen,
and I eventually puzzled out their meaning,
but I wouldn't mind a comment mentioning "left, right",
as I kept thinking in terms of "row".
I also wouldn't mind seeing comments explaining the need for each reshape().

If speed is a concern, then figuring out how to get modification()
to broadcast values to a sub-rectangle would be the thing to focus on.

The saturating aspect of [2, 0, 0] is slightly surprising,
please comment on it, and how it deliberately interacts with clip().
Also, the list is not pythonic, this should definitely be the tuple (2, 0, 0).
A more descriptive name, perhaps red_modification, would be appropriate.

Protecting def modification so import won't see it
kind of make sense, but is a little weird.
You don't want that example to be part of your public API, that's fine.
But consider using def _modification in the usual way,
so the __main__ clause is just a one-liner.

Well, OK, two lines, as the magic tuple (36, 35, 30)
needs a name like dark_gray.

Looks good, ship it!


Checking if a bag has enough coins in program The objective of the program is to check that the weight of the bag that the user inputs and then calculate how many coins are needed to the added/removed to ensure the bag has the right amount of coins in. It needs to display the difference in weight and the number of coins to be added/removed and a total value of coins added/removed. The weights can be found within the file.

The bags for 1p and 2p add up to £1. The bags for 5p and 10p add up to £5. The bags for 20p and 50p add up to £10. The bags for 100p and 200p add up to £20.

print("Calculation")
print("===========================\n")

one_coin_weight = 3.56
one_bag_amount = 1 / 0.01
one_bag_weight = one_coin_weight * one_bag_amount

two_coin_weight = 7.12
two_bag_amount = 1 / 0.01
two_bag_weight = two_coin_weight * two_bag_amount

five_coin_weight = 3.25
five_bag_amount = 5 / 0.05
five_bag_weight = five_coin_weight * five_bag_amount

ten_coin_weight = 6.50
ten_bag_amount = 5 / 0.10
ten_bag_weight = ten_coin_weight * ten_bag_amount

twenty_coin_weight = 5.00
twenty_bag_amount = 10 / 0.20
twenty_bag_weight = twenty_coin_weight * twenty_bag_amount

fifty_coin_weight = 8.00
fifty_bag_amount = 10 / 0.50
fifty_bag_weight = fifty_coin_weight * fifty_bag_amount

onehundred_coin_weight = 9.50
onehundred_bag_amount = 20 / 1.00
onehundred_bag_weight = onehundred_coin_weight * onehundred_bag_amount

twohundred_coin_weight = 12.00
twohundred_bag_amount = 20 / 2.00
twohundred_bag_weight = twohundred_coin_weight * twohundred_bag_amount

coins_value_added = 0
coins_value_removed = 0

bag_count = int(input("How many bags do you have? "))
print("\n===========================\n")

for x in range(bag_count):
    print("Bag", x + 1, "\n")

    bag_weight = float(input("How much does the bag weigh in grams? "))
    coin_type = int(input("Which type of coin is in the bag in pence? "))

    if coin_type == 1:
        weight_difference = one_bag_weight - bag_weight
        coins_changed = weight_difference / one_coin_weight
    elif coin_type == 2:
        weight_difference = two_bag_weight - bag_weight
        coins_changed = weight_difference / two_coin_weight
    elif coin_type == 5:
        weight_difference = five_bag_weight - bag_weight
        coins_changed = weight_difference / five_coin_weight
    elif coin_type == 10:
        weight_difference = ten_bag_weight - bag_weight
        coins_changed = weight_difference / ten_coin_weight
    elif coin_type == 20:
        weight_difference = twenty_bag_weight - bag_weight
        coins_changed = weight_difference / twenty_coin_weight
    elif coin_type == 50:
        weight_difference = fifty_bag_weight - bag_weight
        coins_changed = weight_difference / fifty_coin_weight
    elif coin_type == 100:
        weight_difference = onehundred_bag_weight - bag_weight
        coins_changed = weight_difference / onehundred_coin_weight
    elif coin_type == 200:
        weight_difference = twohundred_bag_weight - bag_weight
        coins_changed = weight_difference / twohundred_coin_weight

    print("\nThe weight difference is", str(round(weight_difference)) + "g.")

    if coins_changed > 0:
        print("Add", round(coins_changed), "coins to the bag.\n")
        coins_value_added += coin_type
    else:
        print("Remove", round(coins_changed), "coins from the bag.\n")
        coins_value_removed += coin_type

    print("\n===========================\n")

print("There were", bag_count, "bags checked.")
print("There is", coins_value_added, "pence that needs to be added.")
print("There is", coins_value_removed, "pence that needs to be removed.")

1) There's too much duplication. You can define a map, such as:

#This defines a mapping from coin value to a tuple (coin weight, monetary sack value)
coin_types = { 1: (3.56, 1), 2: (7.12, 1), ... }

Then, it's easy to calculate weights and amounts as such:

#This defines a mapping from coin value to a tuple (amount of coins in sack, sack weight)
coins_in_sack_by_type = {k: (v[1] / k, v[1] / k * v[2]) for (k, v) in coin_types.items()}

2) Now, most of your code in the main loop can be collapsed into:

weight_difference = coins_in_sack_by_type[coin_type][1] - sack_weight
coins_changed = weight_difference / coin_types[coin_type][0]


Finding the relationship between two values My code was made as an practice for CodeChef. Here's the brief from that page:

  Relational Operators are operators which check the relationship between two values.
  Given two numerical values A and B you need to help chef in finding the relationship between them that is:
  First one is greater than second or
  First one is less than second or
  First and second one are equal.

I couldn't submit the code because of an error so I'd like to see the criticism of the Code Review community.

package main;

import java.util.Scanner;

public class Method1 {
    public static float a, b, t;
    public static Scanner scanner;

    public static void main(String[] args) {
        scanner = new Scanner(System.in);
        System.out.println("Type your first Value please.");
        a = scanner.nextFloat();
        System.out.println("Type your second Value please.");
        b = scanner.nextFloat();
        if (a == b) {
            System.out.println('=');
        }
       if (a > b){
           System.out.println('>');
       }
       if (a < b){
           System.out.println("<");
       }
    }
}

I would like to note that I know its very basic and easy to make but its just a practice for me to see where I'm at in this kinda stuff.

public class Method1 {

Since this will be the name of the file too, (if you save it) I would recommend naming it something descriptive so that you don't have to open it in order to know what it hides.

public static float a, b, t;
public static Scanner scanner;

Try and declare as close to the use of the variables as possible. There is also no need for these to be globals, since they are only used in one place.

Next, read the question very carefully. It states that A and B are both integers, so a float is the wrong type to use. It may also give rounding errors, and comparing floats is trickier because of how floating point numbers are stored. 0.1 + 0.1 does not always equal 0.2, it may give 0.200000001, so this can lead to a bug.

Lastly, t is also going to be an integer. You can tell this is the case because it is a count of how many test cases there are, 2.3 testcases doesn't make sense.

HINT TOWARDS AN ERROR

System.out.println("Type your first Value please.");

Why is this here? Does the question ask for it in the output?

Now onto the code overview. Strictly speaking, your code doesn't work at all. You never actually took in all the input. If its input looks like

3
10 20
20 10
10 10

You scan 3, and 10. That is it. All the rest of the input is ignored. There is a reason it tells you how many test cases there will be, its important to use that information.

if (a == b) {
    System.out.println('=');
}
if (a > b){
    System.out.println('>');
}
if (a < b){
    System.out.println("<");
}

This is the perfect time for if/else statements.

One good piece of logic here is if they aren't equal, and a isn't bigger than b, we know the answer to a < b without needing to check.

There are more things to mention, like moving the check if two numbers are equal to a method, and making use of the library of methods available, but get it working first, then post a follow up. Best of luck


Select depart date, 5 days from today and return date, 6 days from today in Selenium I am automating a scenario of flight booking on Cleartrip.com(https://www.cleartrip.com/flights) in which I have to enter depart date, 5 days from today and return date, 6 days from today. I have written below code.

Date dt = new Date();
Calendar calendar = Calendar.getInstance();
calendar.setTime(dt);
calendar.add(Calendar.DATE, 5);
dt = calendar.getTime();
String departdate = new SimpleDateFormat("dd/mm/yyyy").format(dt);

//enter Journy date in the field
WebElement depart = driver.findElement(By.id("DepartDate"));
depart.sendKeys(departdate);

Date dt1 = new Date();
Calendar calendar = Calendar.getInstance();
calendar.setTime(dt1);
calendar.add(Calendar.DATE, 6);
dt1 = calendar.getTime();
String returndate = new SimpleDateFormat("dd/mm/yyyy").format(dt1);

//enter returndate in the field
WebElement return= driver.findElement(By.id("ReturnDate"));
return.sendKeys(returndate);

You don't need the Date type. You can do it all with a Calendar instance and a DateFormat instance:

    DateFormat dateFormat = new SimpleDateFormat("dd/mm/yyyy");
    Calendar now = Calendar.getInstance(); // gets current date
    now.add(Calendar.DATE, 5);             // add five days
    String firstDate = dateFormat.format(now.getTime());
    ...
    now.add(Calendar.DATE, 1);             // add one more day
    String secondDate = dateFormat.format(now.getTime());


Design of a N-ary tree A friend of mine and I recently started a project and at this moment we are writing a common library. He began writing an N-ary three and he is convinced that he has the best design with which I am not agree.

I want to convince him, that his design of the N-ary Tree is very bad, and with this design we will face problems. Due to lack of experience in complex projects from my side -- I can't convince him with my arguments.

Here is the current design and implementation:

template <typename T>
class Tree {
private:
    T data;
    vector< Tree<T> > successors;

public:
    Tree(T x) {
      data = x;
    }

    T root() {
        return data;
    }
    vector< Tree<T> > succ() {
        return successors;
    }
    void addChild(Tree<T> t) {
        successors.push_back(t);
    }
    void member(T element) {
        if (element == data)
            return true;
        for (int i = 0; i < successors.size(); i++) {
            if(successors[i].member(element)) {
                return true;
            }
        }
        return false;
    }
    void fromRightParentToLeft() {
        if (successors.empty()) {
            cout << data << "\n";
            return;
        }
        if (successors.size() == 1) {
            successors[0].fromRightParentToLeft();
            cout << data << "\n";
        }
        else {
          for (int i = succ().size(); i > 1; i--) {
            succ()[i-1].fromRightParentToLeft();
          }

          cout << data << "\n";
          succ()[0].fromRightParentToLeft();
        }
    }
};

So far I can see few problems:

We will face problem when he will implement a method for node deletion, because it is somehow wrong a node to have the ability to delete itself.
He traverses the tree in a inorder-ish manner, which is pointless when dealing with general N-ary trees.

Can someone point other potential traps with this approach?

The tree must meet the following requirements:

Inteface: node addition, node deletion, search for a particular node, basic tree traversal.
Other: It should be: easily maintainable, with fast performance.

In the general case the tree will have ~10k nodes, which will contain integers.

Some things:

By definition, an N-ary tree is a tree where any node must have no more than N children. However, you never allow the user to define that N, and nowhere do you specify the invariant that for any node, said node must have at most N children. Your tree is not an N-ary tree, it's just a "tree" (with an arbitrary number of children per node).

You're returning bool values in void member(T).

You should decouple traversal from the tree itself. Instead of thinking about traversal like tree.traverse() you should think like traverse(tree).

Reason: to separate and clear up responsibilities. A data structure holds data and nothing else. If you implement traversal behavior inside the tree, the user implementing his own would break any sort of consistency, unless he modifies the structure itself or extends it, which is not exactly desired in this kind of situations (class MySpecialPostOrderTraversalTree : Tree). Furthermore, separating the behavior from the data structure leaves room for other extensions (e.g. a generic traversal algorithm that works on your tree, and on some other kinds of trees, defined by your user).

You should definitely remove the console output from your tree, even if you leave traversal there. At least, you should allow your user to specify a method to call for each visited node. If he wants to print the content of the nodes, he can do it himself. You don't want to give someone a tree that randomly starts to print out stuff to stdout. Also see the concept of mapping functions over collections.

You keep working with values. For example, succ() returns a copy of the vector of children. On large trees this will have a negative effect, constantly copying vectors of elements which contain vectors of elements (and so on) is not exactly a good idea. Look into pointers, and base your tree on them. Start with std::unique_ptr and std::shared_ptr, don't use naked pointers.

See janos's answer for naming suggestions that I feel there's no need to duplicate here :)


Simple Linked List that holds an int I am inexperienced in C++.  I was able to get my LinkedList to be fully functional.  I was hoping to get some feedback as to whether or not I followed bad practices and if there are any risks associated with my code.

LinkedList.h

#pragma once

#include <string>

#include "Node.h"

class LinkedList
{
private:
    Node *head;
    int count;
public:
    LinkedList();
    ~LinkedList();
    int get_count();
    void AddNode(int);
    std::string GetList();
    bool Contains(int);
    void Remove(int);
    int GetValue(int);
};

LinkedList.cpp

#include <iostream>

#include "LinkedList.h"

LinkedList::LinkedList()
{
    head = new Node(0);
}

LinkedList::~LinkedList()
{
}

int LinkedList::get_count(){
    return count;
}

void LinkedList::AddNode(int _value){
    Node *node = new Node(_value);
    Node *previousNode = head;
    while (previousNode->next_node && previousNode->next_node->get_value() <= node->get_value()){
        previousNode = previousNode->next_node;
    }
    if (previousNode->next_node){
        node->next_node = previousNode->next_node;
    }
    previousNode->next_node = node;
    count++;
}

std::string LinkedList::GetList(){
    std::string list;
    Node *currentNode = head;
    while (currentNode->next_node){
        currentNode = currentNode->next_node;
        list += std::to_string(currentNode->get_value()) + " ";
    }
    return list;
}

bool LinkedList::Contains(int number){
    Node *currentNode = head;
    while (currentNode->next_node){
        currentNode = currentNode->next_node;
        if (currentNode->get_value() == number){
            return true;
        }
        if (currentNode->get_value() > number){
            return false;
        }
    }
    return false;
}

void LinkedList::Remove(int number){
    Node *previousNode = head;
    Node *currentNode = head;
    while (currentNode->next_node){
        previousNode = currentNode;
        currentNode = currentNode->next_node;
        if (currentNode->get_value() == number){
            previousNode->next_node = currentNode->next_node;
            delete currentNode;
            count--;
            return;
        }
    }
}

int LinkedList::GetValue(int index){
    Node *currentNode = head;
    int currentIndex = -1;
    while (currentNode->next_node){
        currentNode = currentNode->next_node;
        currentIndex++;
        if (index == currentIndex){
            return currentNode->get_value();
        }
    }
    return -1;
}

Node.h

#pragma once

class Node
{
private:
    int value;
public:
    Node(int);
    ~Node();
    Node *next_node;
    int get_value();
    void set_value(int);
};

Node.cpp

#include "Node.h"

Node::Node(int _value)
{
    value = _value;
}

Node::~Node()
{
}

int Node::get_value(){
    return value;
}
void Node::set_value(int _value){
    value = _value;
}

Instead of having Node use getters and setters, just make it a struct declared within the private section of LinkedList.  It won't need its own files, either.
All but one of your functions are in PascalCase.  Whether you choose that or camelCase, choose just one and maintain that.  This should be kept consistent.
When creating a new linked list, the head should first point to NULL (or nullptr in C++11) to indicate an empty list.  You're instead having the head point to a new node, so the list isn't really considered empty at the start.
You have an empty destructor, even though you allocate memory with new.  In order to prevent memory leaks, the destructor should use delete on each allocated Node.  This is typically done with a loop through the list.
Member functions that don't modify data members should be const.  This also prevents any accidental modifications to data members within these functions.

An example with your get_count() function:

int LinkedList::get_count() const {
    return count;
}


MySQL IMDB Query Speed I'm using data from a database from the IMDB website. The database consists of five relevant tables.

Actor (id, fname, lname, gender)
Movie (id, name, year, rank)
Director (id, fname, lname)
Cast (pid, mid, role)
Movie_Director (did, mid)

It's worth noting that the id column in Actor, Movie & Director tables is a key for the respective table.

Cast.pid refers to Actor.id, and Cast.mid refers to Movie.id.

Here's the prompt and my initial attempt at solving it.  Any help in improving/speeding up the query would be greatly appreciated.

/* For every pair of male and female actors that appear together in some film,
   find the total number of films in which they appear together.
   Sort the answers in decreasing order of the total number of films. */

SELECT a1.fname, a1.lname, a2.fname, a2.lname, count(*) AS num_films
FROM Actor AS a1, Actor AS a2, Cast AS c1, Cast AS c2
WHERE c1.mid = c2.mid AND
      a1.id = c1.pid    AND
      a2.id = c2.pid    AND
      a1.gender = 'M' AND
      a2.gender = 'F'
GROUP BY a1.id, a2.id, a1.fname, a1.lname, a2.fname, a2.lname
HAVING COUNT(*) > 0
ORDER BY COUNT(*) DESC;

Alias notation
Your table aliases a1/a2 and c1/c2 are not good. In this very short query it may not matter, however names for aliases, variables, parameters, etc. should be descriptive enough to say something about what they represent. Besides, with 4-5 character table names, is it really that much to type? I would use Actor1/Actor2 and Cast1/Cast2.
In fact, given the nature of your query, you could give them even more useful names... how about MaleAct/MaleCast and FemAct/FemCast!
Old style JOIN
This JOIN syntax should not be used:
FROM Actor AS a1, Actor AS a2, Cast AS c1, Cast AS c2
WHERE c1.mid = c2.mid AND
      a1.id = c1.pid    AND
      a2.id = c2.pid    AND

Instead:
FROM Actor AS MaleAct
    INNER JOIN Cast AS MaleCast ON MaleAct.id = MaleCast.pid
    INNER JOIN Cast AS FemCast ON MaleCast.id = FemCast.id
    INNER JOIN Actor AS FemAct ON FemAct.id = FemCast.pid

Multiple counting
I see you COUNT(*) 3 times within your query. Since you're already selecting the COUNT(*) into your result set, and MySQL will allow you to use column aliases in GROUP BY, ORDER BY, or HAVING clauses.
This:
HAVING COUNT(*) > 0
ORDER BY COUNT(*) DESC;

Instead:
HAVING num_films > 0
ORDER BY num_films DESC;

That should speed up your query a bit too.
Everything together
SELECT MaleAct.fname, MaleAct.lname, FemAct.fname, FemAct.lname, count(*) AS num_films
FROM Actor AS MaleAct
    INNER JOIN Cast AS MaleCast ON MaleAct.id = MaleCast.pid
    INNER JOIN Cast AS FemCast ON MaleCast.id = FemCast.id
    INNER JOIN Actor AS FemAct ON FemAct.id = FemCast.pid
WHERE
      MaleAct.gender = 'M' AND
      FemAct.gender = 'F'
GROUP BY MaleAct.id, FemAct.id, MaleAct.fname, MaleAct.lname, FemAct.fname, FemAct.lname,
HAVING num_films > 0
ORDER BY num_films DESC;


Matching PEP8 style with long one line for loop I'm working with Python 2.7.5 and I'm trying to make it match the PEP8 coding style.  I have a few one line for loops but the issue is that they are each above 80 characters in length. Is it bad practice for me to make them multiple lines for the sake of PEP8?

So should something like this:

patient = ([d['patient'] for d in doctor for s in sections if d == s['doctor']])

Be changed to something like this:

patient = []
for d in doctor:
  for s in sections:
   if d == s['doctor']:
    patient.append(d['patient'])

I find the first code block easier to read but it doesn't follow the PEP8 style guide so I was wondering if you had any tips.

How about breaking the lines like this:

patient = [doctor['patient']
           for doctor in doctors
           for section in sections if doctor == section['doctor']]

I find this more readable and it's PEP8 compliant.
I also removed the unnecessary (...) from around the expression,
and renamed doctor to the more intuitive doctors,
and used more meaningful loop variable names.

But the truth is I find it difficult to follow PEP8 always,
even though I really like clean code.
So I usually cheat,
and write a wrapper shell script to check my project and hide some PEP8 violations that I don't really want to follow.
Something like this:

#!/bin/bash

cd $(dirname "$0")

# E121 continuation line indentation is not a multiple of four
# E123 closing bracket does not match indentation of opening bracket's line
# E126 continuation line over-indented for hanging indent
# E128 continuation line under-indented for visual indent
# E501 line too long > 79 characters
pep8 . | grep -v \
    -e E121 \
    -e E123 \
    -e E126 \
    -e E128 \
    -e E501


Python algorithm that converts array-like data into MathJax I frequently use MathJax, and often I need to write matrices. Since writing matrices using MathJax is very tiresome task, I decided to automate it.

Code:

import numpy as np
import pyperclip

class Conversion():

    def __init__(self, matrix, n = None, m = None):
        if matrix == None:
            self.matrix = np.random.randint(-10,10, size = [n,m])
        else:
            '''If variable "matrix", for example, contains [1,2,3,4], instead of [[1,2,3,4]], functions below
            will throw error, hence we need to make sure that the data type is correct.'''
            if type(matrix[0]) in (str,int,float):
                self.matrix = np.array([matrix,])
            else:
                self.matrix = np.array(matrix)

        self.rows = len(self.matrix)
        self.cols = len(self.matrix[0])

    def single(self, choosen_brackets = ')'):
        available_brackets = {' ' : 'matrix',
                              ')' : 'pmatrix',
                              ']' : 'bmatrix',
                              '}' : 'Bmatrix',
                              '|' : 'vmatrix',
                              '||': 'Vmatrix'  }
        choosen_brackets = available_brackets[choosen_brackets]
        body = '$$\\begin {} \n'.format('{' + choosen_brackets + '}')
        for row in self.matrix:
            row = [str(x) for x in row]
            body += ' & '.join(row) + '\\\\' + '\n'
        body +='\end {} $$'.format('{' + choosen_brackets + '}')
        print(body)
        pyperclip.copy(body)

    def augmented(self, choosen_brackets = '[]'):
        '''We are assuming that the last column of the given matrix is a vector.'''

        pos_of_the_verticar_bar = '{' + 'c'*(self.cols-1) + '|' + 'c' + '}'
        body = '$$\\left [ \\begin {array} %s \n' % (pos_of_the_verticar_bar)
        for row in self.matrix:
            row = [str(x) for x in row]
            body += ' & '.join(row) + '\\\\' + '\n'
        body +='\end {array} \\right ]$$'
        print(body)
        pyperclip.copy(body)

Notes:

Since function augmented is quite similar to single, I could've merged  them into one. However, I think that keeping them separate makes the code a little bit more readable.
On MSE, mathjax equations are enclosed with $$ instead of \$
"single" is a bad name for a function, I admit. I haven't found any better options. Feel free to offer suggestions.

What can be improved?

Written for Python 3.6.5.

This looks like a handy tool to have around, maybe we can make it even better.

Style

As per the official Python Style Guide (often known by his nickname PEP8), keyword-arguments of functions should have no whitespace around the =.

Matrix size

numpy arrays have a property called .shape which describes their number of rows and number of cols (in case of a 2D array at least). So

self.rows = len(self.matrix)
self.cols = len(self.matrix[0])

could become

self.rows, self.cols = self.matrix.shape

Since your code only works for 2D matrices, it's maybe also a good idea to check for that. The .ndim attribute can be of help here.

Type checking

type(matrix[0]) in (str,int,float) should instead be isinstance(matrix[0], (str, int, float)).

Checking None

The official recommendation is to always use if sth is None: when checking for None.

String joining

row = [str(x) for x in row]
body += ' & '.join(row) + '\\\\' + '\n'

could be done in a single line:

body += ' & '.join(str(x) for x in row) + '\\\\' + '\n'

This means you also don't have to reassign row to be something different than it was before.

String formatting

Since you said you're using Python 3.6, maybe have a look at f-strings for even more concise string formatting syntax. No matter if you choose to do this, maybe at least get rid of the old-style % formatting in augmented(...) by escaping { and } using a leading \.

Function output

I find it preferable if you'd let the user decide what he wants to do with what your function returns. So instead of printing and copying the formatted matrix code, maybe just return body and let the caller decide how to proceed from here. You could even define a "convenience" function (to_clipboard) that basically only does pyperclip.copy(...), although this is likely not necessary. Another idea would be to make at least the copy part opt-in via a "flag" bool parameter.

Class

Maybe a class is not the right tool here. A possible alternative would be to move what's done in __init__ into a "private" (read name starts with _) helper function and get rid of the class.

Missing backslash

There seems to be a little bug in both of your functions (see how bad code duplication is ;-)): body +='\end ... is likely missing an additional \.

Unused argument

If I'm not mistaken, the choosen_brackets argument of augmented is not used in the function.

Typo

choosen_brackets should likely be chosen_brackets


How should I efficiently delete elements from a PHP array? In this case I have an array that is parsed from a JSON file. Normally I use all the elements in the array, but for AJAX crawling I only show the parts the search engine asked for:

<?php $fitstyles = json_decode(file_get_contents(DATA_FILE), true); ?>
…
<?php
if ($base == "fits" || $base == "gallery") {
    /* For fragment handler, delete out the parts of the fits array that aren't needed. */
    $keep_fit = $fragments[1];
    $keep_style = $fragments[2];
    foreach ($fitstyles as $fit => $styles) {
        if ($fit != $keep_fit) {
            unset($fitstyles[$fit]);
            continue;
        }
        foreach ($styles as $style => $styleinfo) {
            if ($style != $keep_style) {
                unset($fitstyles[$fit][$style]);
                continue;
            }
        }
    }
} ?>

Here, $fragments looks like:

(
    [0] => fits
    [1] => straight
    [2] => utility
)

and $fitstyles takes the following form:

Array
(
    [straight] => Array
        (
            [utility] => Array
                ( … )
        )
    [anotherfit] => Array
        (
            [anotherstyle] => ( … )
        )
)

The approach appears to work; however, please give me feedback as to the effectiveness, efficiency and readability of this approach.

Your code is just fine, exactly how I would do it.

There isn't much to discuss really, it's kind of simple. It's as effective / efficient as it could be, and fairly readable.


Keeping data processing out of controllers I would like to start by mentioning that I am LAMP stack guy who happens to be making my first ever .NET C# web app and I'm seeking general advises for best practices and to see how more experienced people would have done it.

I am using code first development pattern with latest Entity Framework . I have a AppUser model. The user could be on of few types, one of them is employee.

public class AppUser{
  public int Id { get; set; }
  public UserType Type { get; set; }
  public Title Title { get; set; }
  public string FirstName { get; set; }
  public string MiddleName { get; set; }
  public string LastName { get; set; }
  public virtual AddressDetails AddressDetails { get; set; }
  public virtual Company Company { get; set; }
  ..................................................
}

public class AddressDetails
{
  public int Id { get; set; }
  public string AddressLine { get; set; }
  public string City { get; set; }
  public string County { get; set; }
  public string PostCode { get; set; }
  public float Latitude { get; set; }
  public float Longtitude { get; set; }
  [JsonIgnore]
  public virtual AppUser User { get; set; }
}

I believe that smaller methods are the better. So, I am trying to keep controllers as small as possible. I made classes which handles all interactions with database and keeps business logic out of Models and Controllers. First class is abstract MasterRepository ,which has  logic for Database Update/Add/SaveChanges. All other "repositories" inherit it.
( "AppDB" is inheriting from DbContext and holds all DbSets)

public abstract class MasterRep
{
  protected AppDB _db { get; set; }
  protected AppUser _currentUser { get; set; }

  protected int _companyId { get; set; }
  protected string _applicationUserId { get; set; }
  public MasterRep(string applicationUserId, AppDB db = null)
  {
    _db = db ?? new AppDB ();
    _currentUser = GetUserByApplicationId(applicationUserId);
    _companyId = _currentUser.Company.Id;
    _applicationUserId = applicationUserId;
   }
  }
  .........................................

Each model has it own "repository". So, User of type "employee" has:

 public class EmployeeRep : UserRep
{
    public EmployeeRep(string applicationUserId, AppDB db = null) :      base(applicationUserId, db) { }

    private IEnumerable<AppUser> Employees
    {
        get
        {
            IEnumerable<AppUser> employees = _db.AppUsers.Where(
               r =>
               r.Company.Id == _companyId &&
               r.Type == UserType.Employee &&
               r.IsDeleted == false
            );
            return employees;
        }
    }
   public User GetById(int Id)
    {
        User employee = Employees.FirstOrDefault(r =>
            r.Id == Id
        );
        return employee;
    }
    .......................

The EmployeeRep holds all logic for getting/updating all employees.

And here is example of retrieving a single employee in controller

 public class HomeController : Controller
  {
    private EmployeeRep _employeeRep { get; set; }
    protected override void Initialize(System.Web.Routing.RequestContext requestContext)
    {
        base.Initialize(requestContext);

            _employeeRep = (User.Identity.GetUserId() != null) ? new EmployeeRep(User.Identity.GetUserId()) : null;

    }
    public ActionResult Index(int id)
    {
        NHUser currentUser = _employeeRep.CurrentUser;

        NHUser employee = _employeeRep.GetById(id);
        ViewBag.CurrentUser = currentUser;

        return View(employee );
    .............................................

So, controllers are in charge of what get displayed and "Repositories" in charge of all business logic/database interactions. All methods are small and doing only one task. However, I am confused by open source projects which I found on github. It looks like everyone doing their own thing.

Am I on a right track here? What is more common way of achieving same goal? i.e having a separate class which handles data processing.

Your brace style is inconsistent. Allman style is usually applied in C#.

public class AppUser{
    // snipped
}

public class AddressDetails
{
    // snipped
}

Longtitude is a spelling error, it should be Longitude.

Underscores are usually only used for fields; properties should be PascalCase:

  protected AppDB _db { get; set; }
  protected AppUser _currentUser { get; set; }

Don't abbreviate unnecessarily: I don't know what Rep is, I do know what Repository is.

public class EmployeeRep : UserRep

Is this a field or a property:

private EmployeeRep _employeeRep { get; set; }

The { get; set; } says "property", but the private and the underscore at the start of the name says "field".

Avoid "negative checks" like you do here:

_employeeRep = (User.Identity.GetUserId() != null)
    ? new EmployeeRep(User.Identity.GetUserId())
    : null;

Invert that logic and things become a lot clearer:

_employeeRep = (User.Identity.GetUserId() == null)
    ? null
    : new EmployeeRep(User.Identity.GetUserId());

Also, since you call User.Identity.GetUserId() twice, consider calling it once, storing the result in a variable and then using that variable.


Angular promise in service I'm new with Angular promise and I would like to know how to improve my code.

I have a service which preload media or data by calling an API.
The media API return an array of urls to preload.

'use strict';

angular.module('myApp').service('PreloaderService', ['$http', '$q', 'localStorageService', function PreloaderService($http, $q, localStorageService) {
  this.preload = function(key){
    var current = 0;
    var deferred = $q.defer();
    switch (key) {
      case 'media':
        // Simplified, in fact it call  a config service to get baseUrl
        var url = 'http://myapi/media';

        $http.get(url, {cache: true}).then(function(data) {
          var data = data.data;
          preloadImage(data, localStorageService.get('cachetoken'));
        }, function(data) {
          deferred.reject(data);
        });
        break;
      case 'data':
        var url = 'http://myapi/data';
        $http.get(url, {cache: true}).then(function(data) {
          var data = data.data;
          localStorageService.add('data', data);
          deferred.resolve(data);
        }, function(data) {
          deferred.reject(data);
        });
        break;
    }

    function preloadImage(arrayOfImages, cachetoken) {
      var promises = [];
      var totalImg = arrayOfImages.length;
      for (var i = 0; i < totalImg; i++) {
        (function(url, promise) {
          var img = new Image();
          img.onload = function() {
            var currentStatus = {
              currentImg: ++current,
              totalImg: totalImg
            };
            deferred.notify(currentStatus);
            promise.resolve();
          };
          img.src = url + '&tok=' + cachetoken;
        })(arrayOfImages[i], promises[i] = $.Deferred());
      }

      $.when.apply($, promises).done(function() {
        deferred.resolve('RESOLVED');
      }).fail(function(data) {
        deferred.reject(data);
      });
    }

    return deferred.promise;
  };
}]);

And I call it with

  var promise = Preloader.preload('media');
  promise.then(
    function(){
      $rootScope.preload.imagesLoaded = true;
    },
    function(){},
    function(currentStatus){
      $rootScope.preload.currentImg = currentStatus.currentImg;
      $rootScope.preload.totalImg = currentStatus.totalImg;
    }
  );

This took a while to parse, it's probably me, I found few flaws with the code, except for a distinct lack of comments.

I would have made url in preload an optional, 2nd parameter, I would probably want to call preload a few times with different URL's ( for 'data' )
I think using a switch for media and data is overkill, I would have done a simple if statement.
Since you have a number of times function(data) {deferred.reject(data);});, I would have assigned this to an actual function.
{cache: true}, for data seem unfortunately hardcoded, consider this as well as optional parameter ?
You define current on top, but totalImg in preloadImage, pick one spot and put both vars there
Sometimes you postfix with Img, sometimes with Images. I would stick everywhere to Images, it is so much more readable
Except for the angular.module('myApp').service( line, you have succinct lines of code, nice to read
No need to store a shortcut to data.data in var data = data.data;preloadImage(data, localStorageService.get('cachetoken'));
I would not have gone down the road of managing an array of promises, I probably would have done a simple if (current===totalImg) deferred.resolve('RESOLVED');
'RESOLVED' seems kind of pointless to provide as a parameter to resolve, we know it's resolved ;) Perhaps totalImg + ' images loaded' ?


Simplifying loop for Incidence Matrix I had to work in the Transport Problem and we're asked to try to make the fastest code for big matrices, so we're advised to try to avoid loops. And the only one I've got and I cannot imagine how to simplify it is this one for making an incidence matrix (with 1s, 0s and -1s) like that:

b<-diag(-1,n2)
a<-b
for (i in 1:(n1-1)) {
  a<-cbind(a,b)
}

Can this be simplified?

I can't think of anything faster than:

a <- matrix(diag(-1, n2), nrow = n2, ncol = n1 * n2)

The n2 * n2 values inside diag(-1L, n2) will be recycled n1 times to fill the final matrix of dimensions n2-by-n1*n2.


Custom error page with information tokens implementation I have created custom error pages (for 500 and 404 http codes) for asp.net web site. After an error, application should create new entry in database in Logs table with information about error. Each entry has unique hash string (it's not primary key) that is returned to end user on error page. With this hash string user can contact with support for advice.

protected void Application_Error(object sender, EventArgs e)
            {
                var exception = Server.GetLastError();

                if (exception is Httpexceptionception httpexceptionception)
                {
                    if (httpexceptionception.GetHttpCode() == 404)
                    {
                        HttpContexceptiont.Current.ClearError();
                        Response.Redirect($"~/NotFound.aspx", false);
                        return;
                    }
                }

                DatabaseLog log = new DatabaseLog()
                {
                    Timestamp = DateTime.UtcNow,
                    Stacktrace = exception.StackTrace
                };

                long id = log.InsertLog();

                string hash;

                using (var md5 = MD5.Create())
                {
                    hash = String.Concat(md5.ComputeHash(BitConverter
                      .GetBytes(id))
                      .Select(x => x.ToString("x2")))
                      .Remove(0, 26);
                }

                log.InterfaceId = hash;
                log.UpdateLog(id);

                HttpContexceptiont.Current.ClearError();
                Response.Redirect($"~/Error.aspx?id={hash}", false);
            }

Following code is responsible for creating new entry in database, id of inserted entry is returned, finally random hash token based on inserted id is generated for user (and inserted entry is updated with this token). I use md5 because I want unique random sequence of characters, but I only want 6 char length token and I don't want use GUID. I think that 6 char tokn length is sufficient. I don't want to return primary key of database entry to error page. Generated hash is returned in query string and this hash string is display to end user.

        public class DatabaseLogger
    {
        public string InterfaceId { get; set; }
        public DateTime Timestamp { get; set; }
        public string Stacktrace { get; set; }

        string _conString = /* ... con string ...*/

        public long InsertLog()
        {
            using (SqlConnection con = new SqlConnection(_conString))
            {
                con.Open();
                string sql = $@"insert into Logs (TimeStamp, Stacktrace) output INSERTED.Id
                                values (@timeStamp, @stackTrace)";
                using (SqlCommand cmd = new SqlCommand(sql, con))
                {
                    cmd.Parameters.AddWithValue("@timeStamp", this.Timestamp);
                    cmd.Parameters.AddWithValue("@stackTrace", this.Stacktrace);
                    return (long)cmd.ExecuteScalar();
                }

            }
        }

        public void UpdateLog(long id)
        {
            using (SqlConnection con = new SqlConnection(_conString))
            {
                con.Open();
                string sql = $"update Logs set InterfaceId = @interfaceId where Id=@id";
                using (SqlCommand cmd = new SqlCommand(sql, con))
                {
                    cmd.Parameters.AddWithValue("@id", id);
                    cmd.Parameters.AddWithValue("@interfaceId", this.InterfaceId);
                    cmd.ExecuteNonQuery();
                }
            }
        }
    }

And table has columns:

Id - long, primary key
InterfaceId - string
Timestamp - DateTime
Stacktrace - string

How can I improve it? Thanks.

First of all I'd consider if hashing is required. Think that:

It's longer to type and impossible to remember.
If user is calling technical support by phone then he will have hard time to spell it out (especially if not native speaker).
It has not any benefit over a simpler short number.
You're computing a MD5 hash (128 bits, 32 bytes) then converting to hex string (64 characters) then removing the first 26. Are you sure you do not introduce collisions in your hash?!

You already have an ID then you might want to simply return that ID (eventually you may add a check-digit to prevent simple mistakes). This will also simplify your code because you do not need an UPDATE to store the calculated hash (check digit may simply be recomputed). Hashing here is (IMO) just overthinking (if you want to obfuscate the ID simply return its hexadecimal value).

You're using long (mapped to bigint). Do you plan to have more than 2^32 errors in your log? 16 Gb just to store the IDs...it's unlikely, don't consider just the number itself but disk space used for each row which has also a very long string).

You're logging stack trace but you're ignoring exception message and exception types which are probably more important than the stack.

You're logging the exception as-is but think about MethodInvocationException, AggregateException, TypeLoadException (just to mention few) where content of InnerException (or InnerExceptions) is much more important than its wrapper. You have a nice Exception.GetBaseException() to get the root cause of the current exception.

I don't know how you throw your own exceptions but often I use Exception.Data to provide more context and useful details. It may be not your case but don't forget about it.

You hard-coded 404 HTTP status code, why don't you use HttpStatusCode.NotFound?

DatabaseLogger.InsertLog() (fix your calling code because it's DatabaseLog) is a missed opportunity of generalization. I'd not go with complex architectures, factories and abstractions (but a simple IoC container might do well here), the fact that storage is in a database is a detail.

Also class should be sealed (currently you do not have extension points and it's not supposed to be a base class):

sealed Logger {
    public int Write(Exception exception) {
        var timestamp = DateTime.UtcNow;
        var content = TranslateExceptionToText(exception);

        // Code to write to DB storage...
    }

    private static string TranslateExceptionToText(Exception exception)
        => exception.GetBaseException().ToString();
}

Notes:

TranslateExceptionToText() is pretty naive, better logic should be used.
I moved responsibility to extract useful information from your error handler to the logger class but you may consider to introduce a 3rd class for this when logic is more complex.
Move SQL code to private const string fields, it's easier to find and edit keeping your code cleaner.
Do not use AddWithValue(). It has some, not obvious, drawbacks. It's not always a bad choice but I think it's easier to stop using it than determining each time if its usage is good or not.
Logging may fail and you dot have any error handling! What's your recovery policy in this case?
Strings for SQL code are not (do not use) interpolated strings. Drop $.
Strings for SQL code are not (do not use) verbatim strings. Drop @.

Application_Error might be simplified:

protected void Application_Error(object sender, EventArgs e)
{
    var exception = Server.GetLastException();
    HttpContextException.Current.ClearError();

    if (IsPageNotFoundException(exception))
        RedirectToNotFound();
    else
        RedirectToError(Logger.Default.Write(exception));
}

Implementation of IsPageNotFoundException(), RedirectToNotFound() and RedirectToError() should be clear but let's write IsPageNotFoundException():

private static bool IsPageNotFoundException(Exception exception)
{
    if (exception is HttpException httpException)
        return httpException.GetHttpCode() == (int)HttpStatusCode.NotFound;

    return false;
}


Very Basic Java Piano I've been reading "Head First Java" and some of the early projects surround basic swing and basic MIDI.

So I've put the two together and written a basic "Synthesiser"/"Piano" using what I have learned.

I know that I need to add some safety features and there's probably some method optimisation staring me in the face but would love to know what you all think!

import javax.swing.*;
import javax.sound.midi.*;
import java.awt.*;
import java.awt.event.*;
import java.util.ArrayList;

public class VirtualKeyboard implements ActionListener {

    public static ArrayList<String> note = new ArrayList<String>();
    public static ArrayList<String> extendedNote = new ArrayList<String>();
    JTextField octaveChoice;
    JTextField instrumentChoice;

    public static void main (String[] args) {
        VirtualKeyboard gui = new VirtualKeyboard();

        String[] preNote = {"C","D","E","F","G","A","B"};
        String[] preExtendedNote = {"C","C#","D","D#","E","F","F#","G","G#","A","A#","B"};

        for (int i = 0; i < 7; i++) {
            note.add(preNote[i]);
        }

        for (int i = 0; i < 12; i++) {
            extendedNote.add(preExtendedNote[i]);
        }

        gui.setGUI();
    }

    public void setGUI() {
        JFrame frame = new JFrame("Rhysy's Virtual Keyboard!!!");
        JPanel keyPanel = new JPanel();
        JPanel controlPanel = new JPanel();

        JButton[] key = new JButton[7];

        for (int i = 0; i < 7; i++) {
            key[i] = new JButton(note.get(i));
            key[i].addActionListener(this);
            keyPanel.add(key[i]);
        }

        JLabel instrumentChoiceLabel = new JLabel("Instrument Choice: ");
        instrumentChoice = new JTextField(1);

        JLabel octaveChoiceLabel = new JLabel("Octave: ");
        octaveChoice = new JTextField(1);

        controlPanel.add(instrumentChoiceLabel);
        controlPanel.add(instrumentChoice);
        controlPanel.add(octaveChoiceLabel);
        controlPanel.add(octaveChoice);

        frame.getContentPane().add(BorderLayout.NORTH, keyPanel);
        frame.getContentPane().add(BorderLayout.SOUTH, controlPanel);
        frame.setSize(500,200);
        frame.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
        frame.setVisible(true);
    }

    public void actionPerformed(ActionEvent e) {
        int finalNote = 0;
        Object source = e.getSource();
        if (source instanceof JButton) {
            JButton but = (JButton) source;
            System.out.print(but.getText());

            finalNote = (Integer.parseInt(octaveChoice.getText()) * 12) + extendedNote.indexOf(but.getText());

            System.out.print(" (" + finalNote + ")\n");

            playNote(finalNote, Integer.parseInt(instrumentChoice.getText()));
        }
    }

    public void playNote(int finalNote, int finalInstrument) {
        try {

            Sequencer sequencer = MidiSystem.getSequencer();
            sequencer.open();
            Sequence sequence = new Sequence(Sequence.PPQ,4);
            Track track = sequence.createTrack();

            MidiEvent event = null;

            ShortMessage first = new ShortMessage();
            first.setMessage(192,1,finalInstrument,0);
            MidiEvent changeInstrument = new MidiEvent(first, 1);
            track.add(changeInstrument);

            ShortMessage a = new ShortMessage();
            a.setMessage(144,1,finalNote,100);
            MidiEvent noteOn = new MidiEvent(a, 1);
            track.add(noteOn);

            ShortMessage b = new ShortMessage();
            b.setMessage(128,1,finalNote,100);
            MidiEvent noteOff = new MidiEvent(b, 16);
            track.add(noteOff);

            sequencer.setSequence(sequence);
            sequencer.start();
        } catch (Exception ex) { ex.printStackTrace(); }

    }
}

import javax.swing.*;
import javax.sound.midi.*;
import java.awt.*;
import java.awt.event.*;

Most style guides will frown upon the wildcard import -- is it so much extra work to list the classes you actually depend on?  This will also help you to avoid conflicts, where you want List to mean java.util.List rather than java.awt.List

public static ArrayList<String> note = new ArrayList<String>();
public static ArrayList<String> extendedNote = new ArrayList<String>();

ArrayList is an implementation detail, that consumers generally don't need to know about.  The more usual declaration would look like:

public static List<String> note = new ArrayList<String>();
public static List<String> extendedNote = new ArrayList<String>();

Furthermore, by specifying that these are lists, you are claiming that they support insert.  Is it really a good idea to allow other code to start inserting new notes into your lists?

An alternative would be to expose the notes as Iterable, rather than List, which helps disguise the fact that the underlying container supports inserts.  Better would be to make that data private, and provide methods that give access to the notes.

(Note: the spelling change of the variable name; this thing isn't a note, it's a collection of notes).

private static List<String> notes = new ArrayList<String>();

public static List<String> getNotes() {
    return Collections.unmodifiableList(notes);
}

The best answer isn't clear, as your example doesn't include any external consumers.

As a rule, you should try to keep variable declarations near their use -- it makes the code clearer to read, and helps with refactoring the code later.

public static void main (String[] args) {

    String[] preNote = {"C","D","E","F","G","A","B"};
    for (int i = 0; i < 7; i++) {
        note.add(preNote[i]);
    }

    String[] preExtendedNote = {"C","C#","D","D#","E","F","F#","G","G#","A","A#","B"};
    for (int i = 0; i < 12; i++) {
        extendedNote.add(preExtendedNote[i]);
    }

    VirtualKeyboard gui = new VirtualKeyboard();
    gui.setGUI();
}

Magic numbers are poor form; you should avoid them where possible.

    String[] preNote = {"C","D","E","F","G","A","B"};
    for (int i = 0; i < preNote.length; i++) {
        note.add(preNote[i]);
    }

You should also avoid writing code that has already been written for you.

    String[] preNote = {"C","D","E","F","G","A","B"};
    List<String> notes = Arrays.asList(preNote);

Or more directly

    List<String> note = Arrays.asList("C","D","E","F","G","A","B");

As an aside - it may make more sense to treat the notes as Enumerations, rather than Strings.

    JButton[] key = new JButton[7];

    for (int i = 0; i < 7; i++) {
        key[i] = new JButton(note.get(i));
        key[i].addActionListener(this);
        keyPanel.add(key[i]);
    }

Two problems here -- you're repeating your magic number again; and also you are creating an array that you never use.

    for (int i = 0; i < note.size(); i++) {
        JButton key = new JButton(note.get(i));
        key.addActionListener(this);
        keyPanel.add(key);
    }

But this is silly -- you put notes in a List, you should use it

    for (String currentNote : note) {
        JButton key = new JButton(currentNote);
        key.addActionListener(this);
        keyPanel.add(key);
    }

OK, let's get some real meat...

public void setGUI() {
    JFrame frame = new JFrame("Rhysy's Virtual Keyboard!!!");
    JPanel keyPanel = new JPanel();
    JPanel controlPanel = new JPanel();

    for (String currentNote : note) {
        JButton key = new JButton(currentNote);
        key.addActionListener(this);
        keyPanel.add(key);
    }

    JLabel instrumentChoiceLabel = new JLabel("Instrument Choice: ");
    instrumentChoice = new JTextField(1);

    JLabel octaveChoiceLabel = new JLabel("Octave: ");
    octaveChoice = new JTextField(1);

    controlPanel.add(instrumentChoiceLabel);
    controlPanel.add(instrumentChoice);
    controlPanel.add(octaveChoiceLabel);
    controlPanel.add(octaveChoice);

    frame.getContentPane().add(BorderLayout.NORTH, keyPanel);
    frame.getContentPane().add(BorderLayout.SOUTH, controlPanel);
    frame.setSize(500,200);
    frame.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
    frame.setVisible(true);
}

OK, what you are doing here is building an object graph -- creating instances of objects and establishing the relationships between them.  That shouldn't happen in an object that also does anything else (separation of concerns).

The VirtualKeyboard is an ActionListener that plays sounds.  So it shouldn't also be doing the job of building it's own object graph.  Building things is the domain of Factory objects, so you should implement the creation of the object graph elsewhere.

But - it's not easy here.  The action listener holds two of the objects that are needed to build the UI, but all the others are created somewhere else.  The riddle is solved by passing the JTextFields to the VirtualKeyboard, rather than the other way around....

private final JTextField octaveChoice;
private final JTextField instrumentChoice;

public VirtualKeyboard(JTextField octaveChoice, JTextField instrumentChoice) {
    this.octaveChoice = octaveChoice;
    this.instrumentChoice = instrumentChoice;
}

So we need the JTextField before we create VirtualKeyboard, which means that we need to create those objects in main().  This is actually pretty common -- main is a good place to put the object assembly you need when bootstrapping your application.

public static void main (String[] args) {
    JTextField instrumentChoice = new JTextField(1);
    JTextField octaveChoice = new JTextField(1);

    VirtualKeyboard gui = new VirtualKeyboard(octaveChoice, instrumentChoice);

    JFrame frame = new JFrame("Rhysy's Virtual Keyboard!!!");
    JPanel keyPanel = new JPanel();
    JPanel controlPanel = new JPanel();

    for (String currentNote : note) {
        JButton key = new JButton(currentNote);
        key.addActionListener(gui);
        keyPanel.add(key);
    }

    JLabel instrumentChoiceLabel = new JLabel("Instrument Choice: ");

    JLabel octaveChoiceLabel = new JLabel("Octave: ");

    controlPanel.add(instrumentChoiceLabel);
    controlPanel.add(instrumentChoice);
    controlPanel.add(octaveChoiceLabel);
    controlPanel.add(octaveChoice);

    frame.getContentPane().add(BorderLayout.NORTH, keyPanel);
    frame.getContentPane().add(BorderLayout.SOUTH, controlPanel);
    frame.setSize(500,200);
    frame.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
    frame.setVisible(true);

The code should be re-organized so that you can see how the parts relate....

public static void main (String[] args) {

    JFrame frame = new JFrame("Rhysy's Virtual Keyboard!!!");

    JPanel controlPanel = new JPanel();

    JLabel instrumentChoiceLabel = new JLabel("Instrument Choice: ");
    controlPanel.add(instrumentChoiceLabel);

    JLabel octaveChoiceLabel = new JLabel("Octave: ");
    controlPanel.add(octaveChoiceLabel);

    JTextField instrumentChoice = new JTextField(1);
    controlPanel.add(instrumentChoice);

    JTextField octaveChoice = new JTextField(1);
    controlPanel.add(octaveChoice);

    VirtualKeyboard gui = new VirtualKeyboard(octaveChoice, instrumentChoice);

    frame.getContentPane().add(BorderLayout.SOUTH, controlPanel);

    JPanel keyPanel = new JPanel();

    for (String currentNote : note) {
        JButton key = new JButton(currentNote);
        key.addActionListener(gui);
        keyPanel.add(key);
    }
    frame.getContentPane().add(BorderLayout.NORTH, keyPanel);

    frame.setSize(500,200);
    frame.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
    frame.setVisible(true);

It's common to create classes that do all this work -- done well, it tends to improve both testability and re-use.

Let's look at the action code again

public void actionPerformed(ActionEvent e) {
    int finalNote = 0;
    Object source = e.getSource();
    if (source instanceof JButton) {
        JButton but = (JButton) source;
        System.out.print(but.getText());

        finalNote = (Integer.parseInt(octaveChoice.getText()) * 12) + extendedNote.indexOf(but.getText());

        System.out.print(" (" + finalNote + ")\n");

        playNote(finalNote, Integer.parseInt(instrumentChoice.getText()));
    }
}

There are at least three things going on here...

public void actionPerformed(ActionEvent e) {
    Object source = e.getSource();
    if (source instanceof JButton) {
        JButton but = (JButton) source;

        String note = but.getText();
        String octave = octaveChoice.getText();
        String instrument = instrumentChoice.getText();

        int finalNote = (Integer.parseInt(octave) * 12) + extendedNote.indexOf(note);
        int finalInstrument = Integer.parseInt(instrument);

        playNote(finalNote, finalInstrument);
    }
}

You've got the ActionListener, which is reading Strings out of awt, you've got a Parser, which knows how to turn those strings into notes and instruments, and you've got a Player that knows how to turn notes and instruments into sounds.

This separation is important, because you should be able to do test that the parser does the right thing without assembling the entire UI for it.  You should be able to replace this player with another one that uses a different set of magic numbers to generate ShortMessages.


Count the number of cyclic words in an input Cyclic Words

  Problem Statement

  We can think of a cyclic word as a word written in a circle. To
  represent a cyclic word, we choose an arbitrary starting position and
  read the characters in clockwise order. So, "picture" and "turepic"
  are representations for the same cyclic word.

  You are given a String[] words, each element of which is a
  representation of a cyclic word. Return the number of different cyclic
  words that are represented.

I have included my attempt below, and I'm not sure whether to be pleased with it or not (because I'm new to algos). I want to solve this problem in the most efficient way, preferably in an imperative language such as Java.

I would appreciate criticism of my code in terms of both function and form.

private static int countCyclicWords(String[] input) {
    HashSet<String> hashSet = new HashSet<String>();
    String permutation;
    int count = 0;

    for (String s : input) {
        if (hashSet.contains(s)) {
            continue;
        } else {
            count++;
            for (int i = 0; i < s.length(); i++) {
                permutation = s.substring(1) + s.substring(0, 1);
                s = permutation;
                hashSet.add(s);
            }
        }
    }

    return count;
}

There's one trick which you can use to get a reduction in memory usage which is very worthwhile if the strings are long. String.substring returns a new String object but wrapping the same underlying char[] as the original. So your code:

        for (int i = 0; i < s.length(); i++) {
            permutation = s.substring(1) + s.substring(0, 1);
            s = permutation;
            hashSet.add(s);
        }

could be replaced with

        String dbl = s + s;
        for (int i = 0; i < s.length(); i++) {
            hashSet.add(dbl.substring(i, i + s.length()));
        }

and will contain the same strings but only store 2 * s.length() characters rather than s.length() * s.length().

The next trick to consider using would be a CharSequence implementation which wraps the original string and an offset for the starting character.

Finally, you could consider normalising each cyclic word. Given the s.length() possible rotations, choose the lexicographically smallest and insert only that one into the set. Whether this gives an efficiency improvement will depend on the statistics of the strings, but if they're random and of a length comparable to the size of the alphabet then a naïve algorithm for finding the smallest will be essentially linear, and you gain two things: firstly, the set becomes smaller (less memory usage, faster lookups); and secondly, you can drop count because the number of cyclic words will be just the size of the set.


Displaying an overlay when hovering over photos I have some very illogical jQuery code and I want to see if someone finds a better way to do the exact same thing:

jQuery:

$(".col-md-3 img").hover(function() {
  $(this).parent().children(".search").show();
  $(this).parent().children(".photo").css("opacity","0.4");
 }, function(){
  $(this).parent().children(".search").hide();
  $(this).parent().children(".photo").css("opacity","1");
 });
html {
 background-color: #3498db;
}

.photo {
 width:100%;
 height: auto;
 margin: 20px 0px;
}

.search {
 position: absolute;
 margin-left: auto;
 margin-right: auto;
 top:40%;
 left: 0;
 right: 0;
 display: none;
}

.search:hover{
 cursor:pointer;
}

I have multiple similar divs.

I don't like the fact that I am accessing the DOM many times with:

$(this).parent().children();

I'm going back and forth and I think it might be a better solution.

The problem with the siblings() method is that one image is on top of the other image, with position absolute and this causes some tricky hover issues. The live website here: live

Instead of

 $(this).parent().children(".search").show();

you can use

$(this).siblings(".search").show();


Testing higher order reducer with jest I'm trying to understand conception of creating unit tests for the frontend applications.

I have created higher order reducer:

// @flow

type action = {
    type: string,
    payload?: any
};

/**
 * Async Reducer Factory to reduce duplicated code in async reducers.
 * Higher Order Reducer.
 *
 * @param {String} name - Reducer name.
 * @returns {Function}
 */
export const asyncReducerFactory = (name: String) => {
    return (
        state = { data: null, isLoading: false, error: null },
        action: action
    ) => {
        switch (action.type) {
            case `FETCH_${name}_STARTED`:
                return { data: null, isLoading: true, error: null };
            case `FETCH_${name}_SUCCESS`:
                return { data: action.payload, isLoading: false, error: null };
            case `FETCH_${name}_ERROR`:
                return { data: null, isLoading: false, error: action.payload };
            default:
                return state;
        }
    };
};

Tests:

import { asyncReducerFactory } from "./factories";

describe("Test async reducers factory", () => {
    const factory = asyncReducerFactory("TEST");

    it("should create reducer", () => {
        expect(factory).not.toBe(null);
    });

    it("should start fetching", () => {
        expect(factory({}, { type: "FETCH_TEST_STARTED" })).toEqual({
            data: null,
            isLoading: true,
            error: null
        });
    });

    it("should end fetching with success", () => {
        expect(
            factory({}, { type: "FETCH_TEST_SUCCESS", payload: "success" })
        ).toEqual({
            data: "success",
            isLoading: false,
            error: null
        });
    });

    it("should end fetching with error", () => {
        expect(
            factory({}, { type: "FETCH_TEST_ERROR", payload: "error" })
        ).toEqual({
            data: null,
            isLoading: false,
            error: "error"
        });
    });

    it("should return default state", () => {
        expect(factory({}, { type: "DIFFERENT" })).toEqual({});
    });
});

I would really appreciate, if you could let me know:

if i'm using flow correctly?
if my tests are reliable?
how could i make it more generic?

I have very minimal flow experience, but the action definition looks right. You may also be able to define a state type for this reducer.

It's pretty generic right now, so I'm not sure I would push it much more. Maybe it should be called fetchReducerFactory, just to keep the terminology consistent.

The tests look good. By "reliable", you probably mean "good." They are readable and look to provide good coverage for the code and branches.

You may want to test that the default state is correct.
You may want to test that unknown actions do nothing to the state.

NOTE: The following comment is pretty minor, but wanted to bring it up. The way the code is structured, the tests make sense. But there are implicit cases that work because of the code structure. Let me explain:

You expect FETCH_TEST_SUCCESS to do 3 things: (1) clear the isLoading flag (2) set data to the payload, and (3) set error to null. These three behaviors are wrapped together in one test. And, the only data that is actually changed by the reducer in this test is the data property; isLoading and error are not changed. So, for this test, you could make sure all values are changed by passing in a different state:

expect(
       factory({ data: null, isLoading: true, error: 3 },
               { type: "FETCH_TEST_SUCCESS", payload: "success" })
       ).toEqual({
         data: "success",
         isLoading: false,
         error: null
  });

This will guard against code changes later where someone refactors. Or, you could just have three small tests, each that test single property mutations. Again, small potatoes.


All possible lists from ups and downs between elements in python list I have a function that compares sequential elements from a python list and returns 1 and -1:

>>> up_down([0, 2, 1, 3])
[1, -1, 1]

I need a function to return all possible lists from a '-1, 1' list using the function up_down.

>>> possible_lists([1, -1])
[[0, 2, 1], [1, 0, 2]]

I'd like to know if I can write these functions in a better way. My code is below:

import itertools

def comparison(a, b):
    if b > a:
        return 1
    if b < a:
        return -1
    else:
        return 0

def up_down(data, n=1):
    return [comparison(data[pos], data[pos + n]) for pos, value in enumerate(data[:-n])]

def possible_lists(data, n=1):
    size = len(data) + n
    all_lists = itertools.permutations(range(size), size)

    return [list(el) for el in all_lists if up_down(el, n) == data]

Your comparison function is the same as the builtin cmp function.

Your filtering technique to find the matching lists may be not the most efficient. Permutations will generate a lot of lists which fail the test. On the other hand, itertools is written in C, which means it'll probably be faster for small data sets.


A web scraper that looks for pre-defined words in news articles I'm still fairly new to Python and web-scraping, but a colleague asked asked me if I could build a web-scraper that could be used by a think tank he's a member of to find news articles matching a pre-defined list of trigger words of interest to the organisation.

The purpose of the following script is as follows:

Parse an RSS feed of news articles published by a British news
outlet
Read each news article listed in the feed by accessing the link
for each item in the RSS feed.
Search each article for pre-defined words and phrases
Write the results (matching trigger word/phrase and link to article)
to a CSV.

Here's the code:

import feedparser
from bs4 import BeautifulSoup
from lxml import html
import requests
import re

import csv

words = ['divorce', 'custody battle', 'meal ticket', 'behind closed doors', 'detail of the case emerged on a legal database', ]

hit_article = []
links_list = []
hits = []
hit_link = []

d = feedparser.parse('http://www.dailymail.co.uk/articles.rss')

for item in d.entries:
    link = ( item[ "link" ] )
    links_list.append(link)
    title = ( item ["title"])

for link in links_list[:100]:
    page = requests.get(link)
    tree = html.fromstring(page.content)

    soup = BeautifulSoup(page.text, 'html.parser')

    text = soup.find('body')
    text = text.text

    for word in words:
        regex = r"\b"+ re.escape(word) + r"\b"
        match = re.search(regex, text)
        if match:
            print (word, ' ', title, "________found")
            hits.append(word)
            hit_link.append(str(link))

match_dictionary = dict(zip(hit_link, hits))

print (match_dictionary)

w = csv.writer(open("output.csv", "w"))
for key, val in match_dictionary.items():
    w.writerow([key, val])

The code does what I want it to do, but I know it stinks (particularly where I fall back into two for loops.

How can I achieve the same result more cleanly and Pythonically?

At first glance, I'd say that this code isn't bad.  I am particularly impressed by this line:

regex = r"\b"+ re.escape(word) + r"\b"

because you took the care to escape the word, and to ensure that it is starts and ends at word boundaries.  (You might want to do case-insensitive searches, though?  Also, you might want to treat all whitespace as equivalent, in case a newline occurs in the middle of a phrase.)

You do have a bug, though, in that the on-screen output reports all hits as coming from the last title in the RSS feed.

Code organization

This program is starting to be long enough that you should break it up into functions.  In particular, when the code gets long, all of the variables (hit_article, links_list, hits, hit_link, d, etc.) act as global variables, making it hard to keep track of how they are used.  That is the root cause of your title bug.

Wasted work

The silliest mistake is that tree = html.fromstring(page.content) is never used, so you used the lxml library to parse the HTML a second time for no reason.

Less obviously, you have a problem if one article contains multiple search terms.  The on-screen printout will report all terms that were found.  However, when you do dict(zip(hit_link, hits)), you will only store the last hit per link.  You'll need to decide whether you want to:

report all search terms that are found in each article (in which case you need to modify the data structure to store more results, or eliminate the dictionary entirely)
report only the first search term in your list that appears in each article (in which case you can break from the if match: block)
report only the first occurrence in the article of any of the search terms (in which case you should construct the regex to look for any of the search terms: regex = r'\b(?:' + '|'.join(re.escape(word) for word in words) + ')\b', instead of looping)

Comprehensions

In general, whenever you see the pattern:

output_list = []
for item in input_list:
     output_list.append(transform(item))

… you can write it more elegantly using a list comprehension.

For example, instead of:

list_list = []
d = feedparser.parse('http://www.dailymail.co.uk/articles.rss')
for item in d.entries:
    link = ( item[ "link" ] )
    links_list.append(link)

You should write (with a bit of renaming for clarity):

 rss_url = …
 links = [entry['link'] for entry in feedparser.parse(rss_url).entries]

Suggested solution

Here, I've elected to report all terms found in each article, by eliminating the match_dictionary altogether.  I've used two generator functions.

from bs4 import BeautifulSoup
import csv
import feedparser
import re
import requests

def search_article(url, phrases):
    """
    Yield all of the specified phrases that occur in the HTML body of the URL.
    """
    response = requests.get(url)
    text = BeautifulSoup(response.text, 'html.parser').find('body').text
    for phrase in phrases:
        if re.search(r'\b' + re.escape(phrase) + r'\b', text):
            yield phrase

def search_rss(rss_entries, phrases):
    """
    Search articles listed in the RSS entries for phases, yielding
    (url, article_title, phrase) tuples.
    """
    for entry in rss_entries:
        for hit_phrase in search_article(entry['link'], phrases):
            yield entry['link'], entry['title'], hit_phrase

def main(rss_url, phrases, output_csv_path, rss_limit=None):
    rss_entries = feedparser.parse(rss_url).entries[:rss_limit]
    with open(output_csv_path, 'w') as f:
        w = csv.writer(f)
        for url, title, phrase in search_rss(rss_entries, phrases):
            print('"{0}" found in "{1}"'.format(phrase, title))
            w.writerow([url, phrase])

if __name__ == '__main__':
    rss_url = 'http://www.dailymail.co.uk/articles.rss'
    phrases = ['divorce', 'custody battle', …]
    main(rss_url, phrases, 'output.csv', 100)


Converting a float to a fraction as a string I was inspired by this video by Matt Parker to use the Farey Sequence to convert a float into a string representation of a fraction, how did I do? How well did I follow F# workflow and patterns (I'm new to the language)? Can it be done better, or rather, how would a "professional" F# developer write this function? Thanks in advance!
let toFraction num =
  let rec farey n0 d0 n1 d1 =
    let n2 = n0 + n1
    let d2 = d0 + d1

    match (n2 / d2) with
    | x when abs (x - num) < 1e-5 -> string n2 + " / " + string d2
    | x when x < num -> farey n2 d2 n1 d1
    | x when x > num -> farey n0 d0 n2 d2
    | _ -> "" // Compiler detects "incomplete pattern matches on this expression without this wildcard,
    // maybe this is where my method can be improved?

  farey 0. 1. 1. 1.

Additionally, this problem assumes the input num is in the set 0 <= num < 1

There isn't much to say about the function and algorithm it self. It's an ordinary recursive function - with tail call - which is optimal for recursive functions.
I don't find a string as return value really useful. Instead I think, I would return a tuple of two integers representing the numerator and denominator
let toFraction (num: float): (int * int) = etc...

Alternatively you could define a discriminated union as something like:
type Fraction = Fraction of int * int

... used like:
let toFraction num =
if num <= 0.0 || num >= 1.0 then
    failwith "Invalid input"
else
  let rec farey n0 d0 n1 d1 =
    let n2 = n0 + n1
    let d2 = d0 + d1

    match float n2 / float d2 with
    | x when abs (x - num) < 1e-10 -> Fraction(n2, d2)
    | x when x < num -> farey n2 d2 n1 d1
    | x when x > num -> farey n0 d0 n2 d2
    | _ -> failwith "Something went completely wrong" // just to fulfill the pattern matching - it should never happen

  farey 0 1 1 1

and called as
let (Fraction(num, denom)) = toFraction value
printfn "%d / %d" num denom

As shown, I've chosen to run farey with integers instead of floats. You should test if that is more efficient than using floats.

match (n2 / d2) with

You don't need the parentheses.

num is in the set 0 <= num < 1

0 = num is an edge case that is calculated wrongly to
1 / 100001

If you want 0 to be included in the domain, you need to start faray with the values -1 0 1 1. Then it will return "0 / 1".

Even though you specify in the comment, that it is assumed that num should be between zero and one, you should guard against invalid input, especially because invalid input may result in an infinite recursive loop.