A stream version of Fizz Buzz I'm trying to learn streams and hashmaps, in doing so I want to make a very scalable, very clean version of FizzBuzz while adhering to the book Clean Code by Robert Cecil Martin.

public class Main {

    public static void fizzBuzzHundredTimes(Map<String, Integer> fizzDivisor) throws ArithmeticException{
        String output; // Avoid recreating String each iteration
        for (int i = 1; i <= 100; i++) {
            final int finalI = i; // Variables used in lambda functions need to be final.
            output = fizzDivisor.entrySet()
                    .stream()
                    .filter(entry -> finalI % entry.getValue() == 0)
                    .map(entry -> entry.getKey())
                    .collect(Collectors.joining(""));
            if (output.length() > 0) {
                // the value was divisible.
                System.out.println(output);
            } else {
                System.out.println(i);
            }

        }

    }

    public static void main(String[] args) {

        Map<String, Integer> fizzMap = new HashMap<>();
        fizzMap.put("Fizz",3);
        fizzMap.put("Buzz",5);
        fizzMap.put("Fuzz",7);
        fizzMap.put("Bizz",11);
        fizzMap.put("Biff",13);
        try{
            // Avoid negative numbers
            fizzBuzzHundredTimes(fizzMap);
        } catch(Exception e){
            System.err.println(e + "Was thrown");
        }

    }
}

You can use output.isEmpty() instead of checking its length.
You might want to use a LinkedHashMap which presists the order of the elements as well. Right now the ordering will be seemingly random (Will it be BuzzFizzFuzzBiffBizz? Or FizzFuzzBiffBizzBuzz?) (technically it's deterministic, but it's based on the hashcodes of the strings, which is seemingly random)
Your exception handling System.err.println(e + "Was thrown"); doesn't tell you anything about why the exception happened. I would recommend using a logger (Slf4j / Log4j) and log the exception properly, or use e.printStackTrace() to make it easier for you to debug the problems.

In your final version however, you should not need any exception handling as all the bugs that could cause them to appear should have been fixed - as this is an application based on pure logic and no network calls and stuff.
String output; // Avoid recreating String each iteration technically, the string is recreated each iteration anyway. There's nothing you can do to avoid that.

You're only declaring it once. It's best practice to declare it in a small scope as possible, so I would recommend declaring it only when you initialize it.
You could create another method to return a single string for a single number, instead of having one method to process all the 100 numbers.
// Avoid negative numbers I don't see how that comment is relevant at the location it is written. Negative numbers are prevented by the for-loop in your code.


Length prefixed strings in C In want to implement length prefixed strings in C (not null terminated), with some idiosyncrasies: malloc is prohibited, memory efficiency is important, each string (except for literal-backed ones) is modifiable and can change its length, but it has a fixed (at declaration) allocated length (maximum 255), C code is generated by a code generator.  I want this to work on as many architectures and C compilers as possible.

Suppose I do the following:

typedef struct PLS {
   uint8_t maxlen;
   uint8_t curlen;
   char buf[1];  // actual allocated size is in maxlen
} PLS ;

Then I could declare, say

  struct {
    ...
    PLS s1;   // real allocated size: 100
    char s1buf[99];
    PLS s2;  // real allocated size: 64
    char s2buf[63];
    ...
    };

   // initialization:
   mystruct.s1.maxlen=100;
   mystruct.s1.curlen=0;
   mystruct.s2.maxlen=64;
   mystruct.s2.curlen=0;

   // PLS library:

   void PLS_copy(PLS *to,PLS *from) {
       int tc = from-> curlen; // chars to copy
       if(tc > to->maxlen) tc = to->maxlen;
       memcpy(to->buf,from->buf,tc);
   }

Does this look ok? Any pitfalls? Would some compiler give warnings?
I believe that data structure alignment should not padding between s1 and s1buf, but even then it should work, right?

For literals, if I want to avoid a duplication of allocated chars, things seem trickier (remember, though, that code is machine-generated). Would this be objectionable?

 // static declaration of literal "hello" (5 chars) trailing null byte wasted
 char lit1[] = "\x05\x05hello";
// can the above be casted to PLS? (I won't modif it, I swear)
 PLS * lit1Pls = &(PLS)(lit1);  // smells funny, though...

Copy function improvements

Your copy function could be better:

   void PLS_copy(PLS *to,PLS *from) {
       int tc = from-> curlen; // chars to copy
       if(tc > to->maxlen) tc = to->maxlen;
       memcpy(to->buf,from->buf,tc);
   }

You could use const on your from argument.
You never update the length of the to string.  I'm assuming this is like a strcpy, so the to string should have its length set to the length that you just copied.

Here is how I would write it:

   void PLS_copy(PLS *to, const PLS *from)
   {
       uint8_t bytesToCopy = from->curlen;

       if (bytesToCopy > to->maxlen)
           bytesToCopy = to->maxlen;
       memcpy(to->buf, from->buf, bytesToCopy);
       to->curlen = bytesToCopy;
   }

Flexible array member

I agree with the comment by M.M about using flexible array members instead of buf[1]:

typedef struct PLS {
   uint8_t maxlen;
   uint8_t curlen;
   char buf[];  // actual allocated size is in maxlen
} PLS;

struct {
    ...
    PLS s1;   // real allocated size: 100
    char s1buf[100];
    PLS s2;  // real allocated size: 64
    char s2buf[64];
    ...
};

Also, you could use a macro to help declare these fields.  Something like this:

#define PLS_FIELD(name, length)  \
    PLS name;                    \
    char name##buf[length]

Then you could define your struct as:

struct {
    ...
    PLS_FIELD(s1, 100);
    PLS_FIELD(s2, 64);
    ...
};

Casting literal

For your literal casting, I would do this:

char lit1[] = "\x05\x05hello";
const PLS *lit1Pls = (const PLS *) lit1;

The const keyword here will prevent you from writing to the literal.  And the cast I used is simpler than the one you used.  I don't see anything objectionable about doing this.


Star Wars API GET request in Python I am working with a Star Wars API from http://swapi.co/api/. Here's the problem that I was working on for the Star Wars API problem. And the code works and prints out the exact output that I desired. I would like to get some code review.

>

# You are a Coordinator for the Star Wars rebel forces and are responsible for planning troop deployments.
# You need a tool to help find starships with enough passenger capacity and pilots to fly them.

# Build a CLI tool that takes as its single argument the number of people that
# need to be transported and outputs a list of candidate pilot and starship names
# that are each a valid possibility.
# Assume that any pilot can be a candidate regardless of allegiance.
# Assume the entire group of passengers must fit in a single starship.

# You may skip starships with no pilots or with unknown passenger capacity.
# Your tool must use the Star Wars API (http://swapi.co) to obtain the necessary data.

# You may not use any of the official Star Wars API helper libraries but can use any other libraries you want
# (http client, rest client, json).

  Example:

> # print-pilots-and-ships with minimum of 20 passengers
>
> # Luke Skywalker, Imperial shuttle
>
> # Chewbacca, Imperial shuttle
>
> # Han Solo, Imperial shuttle
>
> # Obi-Wan Kenobi, Trade Federation cruiser
>
> # Anakin Skywalker, Trade Federation cruiser

Python 3 solution:

import sys
import requests
import json
import urllib.parse

#number of pages in JSON feed

def print_page(page_num, num_passenger):
    endpoint = "https://swapi.co/api/starships/?"
    type = 'json'

    #specifies api parameters
    url = endpoint + urllib.parse.urlencode({"format": type, "page": page_num})

    #gets info
    json_data = requests.get(url).json()
    # number_of_ship = json_data['count']
    if 'results' in json_data:
      for ship in json_data['results']:
          if has_pilot(ship) and has_enough_passenger(ship, num_passenger):
              print_pilots_on(ship)

def get_pilot_name(pilot):
    type = 'json'

    #specifies api parameters
    url = pilot

    #gets info
    json_data = requests.get(url).json()
    return json_data["name"]

def print_pilots_on(ship):
    for pilot in ship['pilots']:
       print(get_pilot_name(pilot), ship['name'])

def has_pilot(ship):
    if ship['pilots']:
      return True
    return False

def has_enough_passenger(ship, num):
    if ship['passengers'] != "unknown" and int(ship['passengers']) >= num:
      return True
    return False

def print_pilots_and_ships(num_passenger):

    page_list = [1,2,3,4,5,6,7,8,9]
    # list to store names

    for page in page_list:
        print_page(page, num_passenger)

if __name__ == '__main__':
    num_passenger = int(20)
    print_pilots_and_ships(num_passenger)

This is pretty good. I only have a few small nitpicks

Return directly

You do this a few times:

 if ship['passengers'] != "unknown" and int(ship['passengers']) >= num:
    return True
return False

The above can be rewritten as:

return ship['passengers'] != "unknown" and int(ship['passengers']) >= num

Superfluous int call

int(20), here the int call does nothing, 20 is already an integer.
You can get more out of the request module.

Python's requests lib is very powerful and has a lot of features you can use.

It supports adding params for instance, so instead of:

#specifies api parameters
url = endpoint + urllib.parse.urlencode({"format": type, "page": page_num})
#gets info
json_data = requests.get(url).json()

You could do:

json_data = requests.get(endpoint, params={"format": "json", "page": page_num}).json()

Remove unused imports

sys module is not used, I suggest removing it.


Optimize calculation of string self-similarly with its suffixes I am trying to solve a Hacker Rank problem about string suffixes:

  For two strings A and B, we define the similarity of the strings to be
  the length of the longest prefix common to both strings. For example,
  the similarity of strings "abc" and "abd" is 2, while the similarity
  of strings "aaa" and "aaab" is 3.

  Calculate the sum of similarities of a string S with each of its
  suffixes.

I have the following code:

import System.IO
import Control.Monad
import Data.Maybe
import Data.List
import qualified Data.ByteString.Char8 as BSC

main = do
    n <- liftM getIntFromBS BSC.getLine
    replicateM_ n $ do
        s <- BSC.getLine
        putStrLn . show $ sum $ map (cntPrefix s) $ tail $ sort $ BSC.tails s
    where
        getIntFromBS = fst.fromJust.BSC.readInt
        cntPrefix str pref = length $ takeWhile (\t -> fst t == snd t) $ BSC.zip pref str

But performance is really bad. I am doing this challenges to learn Haskell and thus my skills aren't good enough to optimize this program. Any help is appreciated.

As far as I can tell your solution is not only slow but incorrect because of your usage of sort. Dropping that single function from your pipeline nets an exponential improvement, and it stopped including the similarity of s to itself in the answer.

Make sure also you're compiling with -O2, otherwise sum won't be optimized into a strict fold and you'll blow up the stack with thunks.

I also came up with a version targeted toward readability, but it's ~3x slower than your (corrected) ByteString based version so I'll just include it here as a curiosity.

import Control.Monad (replicateM_)
import Data.List (tails)

main :: IO ()
main = do
        n <- readLn
        replicateM_ n $ do
            s <- getLine
            print $ sum $ map (similarity s) (suffixes s)

similarity :: (Eq a) => [a] -> [a] -> Int
similarity s = length . takeWhile id . zipWith (==) s

suffixes :: [a] -> [[a]]
suffixes = tail . tails


Printing all subsets I have tried to incorporate comments from my previous code review into this. So, I am hoping for less mistakes. If you spot any, please let me know mercilessly!

#include <iostream>
#include <queue>

void printsub(int* p, int len, std::queue<int> q) {

    if (!len) {
        while(!q.empty()) {
            std::cout << q.front() << " ";
            q.pop();
        }
        std::cout << "\n";
        return;
    }

    std::queue<int> t1(q);
    t1.push(p[0]);
    printsub(&(p[1]), len - 1, t1);

    std::queue<int> t2(q);
    printsub(&(p[1]), len - 1, t2);
}

int main() {
    int arr[] = {1, 2, 3, 4, 5, 6};

    std::queue<int> q;
    printsub(arr, ((sizeof(arr))/(sizeof(arr[0]))), q);
}

You could translate this recursive Python code.

def xcombinations(items, n):
  if n==0: yield []
  else:
    for i in xrange(len(items)):
        for cc in xcombinations(items[:i]+items[i+1:],n-1):
            yield [items[i]]+cc

in C++ roughly (untested, but you get the idea):

using std::set;

set<set<int>> xcombinations(const set<int> &items, size_t n) {
    set<set<int>> result; // collect for 'return' instead of pythons 'yield'
    if(n==0) return result;
    for(int elem : items) { // ranged-for from C++11
        // 'items' without 'elem', python 'items[:i]+items[i+1:]'
        set<int> temp(items); // copy
        temp.erase(elem); // ...or use 'std::remove_copy_if'
        for(auto cc : xcombinations(temp, n-1)) { // ranged-for from C++11
            cc.insert(elem);
            result.insert(cc); // python yield;
        }
    }
    return result;
}

Note that auto cc creates a copy in the inner loop, where one can add elem before adding it to result.

Instead if the new C++11 ranged-for and auto one can of course just use lengthy iterator-based for-loops.

Using vector instead of set would also work, I guess. There are no set-operations involved here. Actually, now that I think of it, vector would be much better... hmm...

One could save some copying by pulling temp out of the loop, initialize it with a a size of n-1 and use std::remove_copy_if to put the values into it. And if one does that, one can even save the copy of all values by just replacing one in the inner loop, but I can not see how right away.

Ah, well and you need a printing routine, of course.

std::ostream& operator<<(std::ostream &os, const set<int>& items) {
    for(int elem: items)
        os << elem << " ";
    return os;
}
std::ostream operator<<(std::ostream &os, const set<set<int>>& itemss) {
    for(const auto &cc: itemss)
        os << "  {" << cc << "}\n";  // or use 'ostream_iterator' on 'cc'
    return os;
}

int main(/*...*/) {
    set<int> data = {1, 2, 3, 4, 5, 6}; // C++11 init-list
    std::cout << xcombinations(data, data.size()) << std::endl;
}


jQuery script to toggle an element and handle a Close button First of all: I am a total javascript beginner and therefore I am asking you to rate my script and tell me whether its okay or just a big mess. Note: it does work, but I guess it could be improved.

My main goal was to create a script, than can be used multiple times and does not depend on any class or id name (thats why I am using the data attribute).

$(function(){
    var toggleOpen      = $('*.[data-toggle="open"]');
    var toggleContent   = $('*.[data-toggle="content"]');
    var toggleClose     = $('*.[data-toggle="close"]');
    var toggleSpeed     = 500;
    //Set height of toggle content to avoid animation jumping
    toggleContent.css('height', toggleContent.height() + 'px');
    //Find content to toggle
    function findNextContent(target){
        var nextContent = target.parent().parent().parent().find(toggleContent);
        return nextContent;
    }
    //Toggle content
    function slideToggle(target){
        target.stop(true,true).slideToggle(toggleSpeed);
    }
    //CLose toggled content
    function closeToggle(target){
        target.slideUp(toggleSpeed);
    }
    //On Open Click
    toggleOpen.click(function(){
        var clicked     = $(this);
        var nextContent = findNextContent(clicked);
        //Check if hidden to either scroll to bottom or not
        if(nextContent.is(':hidden')){
            slideToggle(nextContent);
            smoothScrolling(toggleClose);
        }else{
            slideToggle(nextContent);
        }
        return false;
    });
    //On Close click
    toggleClose.click(function(){
        var clicked     = $(this);
        var nextContent = findNextContent(clicked);
        closeToggle(nextContent);
        return false;
    });
});

What it does: It toggles an element and it also takes care of a seperate close button.

I am glad for any feedback - be it positive or negative!

function findNextContent(target){
    var nextContent = target.parent().parent().parent().find(toggleContent);
    return nextContent;
}

You could possibly use the jQuery closest function here.
Or you could remove the variable and just do return target.parent().parent().parent().find(toggleContent);

toggleClose.click(function(){
    var clicked     = $(this);
    var nextContent = findNextContent(clicked);
    closeToggle(nextContent);
    return false;
});

There is no need for the clicked variable. You can just write findNextContent($(this));

You might also want to look into the aria-expanded attribute.


Registering a plist to NSUserDefaults I've been struggling with registering a plist into NSUserDefaults and I've finally got it working, though I'm not sure that its written properly. I'm hoping someone here can shine some light on this process.

My intention with this code:

I want to be able to set all of my values in a plist and essentially use that to "seed" my NSUserDefaults. My end goal is to use this as a user settings menu.

My question:

What is the correct way to register a plist into the NSUserDefaults?

I followed the example from this this post and I received compiler errors. So I started experimenting. I could only get the code from the post to work if I added setObject(dict, forKey: "defaults") first.

Once added, it seemed to render the registerDefaults line pointless as the code runs with or without it.

The lines that I am questioning are these:

userDefaults.setObject(dict, forKey: "defaults") // without this my code doesn't work

userDefaults.registerDefaults(dict as! [NSObject : AnyObject])  // with or without this code works... do I need this?

I've placed the entirety of the code below to put above lines in context. This is all of the code so you can copy it right into Xcode and it will work.

ViewController.swift

class ViewController: UIViewController {

    let userDefaults = NSUserDefaults.standardUserDefaults()

    override func viewDidLoad() {
        super.viewDidLoad()

        let prefs = NSBundle.mainBundle().pathForResource("Settings", ofType: "plist")
        let dict = NSDictionary(contentsOfFile: prefs!)
        userDefaults.setObject(dict, forKey: "defaults")                // without this code doesn't work
        userDefaults.registerDefaults(dict as! [NSObject : AnyObject])  // with or without this code works... do I need this?
        userDefaults.synchronize()

        // this just toggles the aBool value
        if let defaults = userDefaults.valueForKey("aBool") as? NSNumber {

            if defaults as NSObject == 1 {
                println("inside the conditional: \(defaults)")
                userDefaults.setValue(0, forKey: "aBool")
                userDefaults.synchronize()
                println("Setting aBool to true")
            } else {
                userDefaults.setValue(1, forKey: "aBool")
                userDefaults.synchronize()
                println("setting aBool to false")
            }
        }
    }

    override func didReceiveMemoryWarning() {
        super.didReceiveMemoryWarning()

    }
}

Settings.plist

override func didReceiveMemoryWarning() {
    super.didReceiveMemoryWarning()
}

If all your method does is call up to the super class's implementation of the method, you can safely omit the method entirely.  I understand this is boilerplate code that Xcode automatically inserts any time you create a new view controller, but at the end of the day, it's just clutter that should be cleaned up.

override func viewDidLoad() {
    super.viewDidLoad()

    let prefs = NSBundle.mainBundle().pathForResource("Settings", ofType: "plist")
    let dict = NSDictionary(contentsOfFile: prefs!)
    userDefaults.setObject(dict, forKey: "defaults")                // without this code doesn't work
    userDefaults.registerDefaults(dict as! [NSObject : AnyObject])  // with or without this code works... do I need this?
    userDefaults.synchronize()

    // this just toggles the aBool value
    if let defaults = userDefaults.valueForKey("aBool") as? NSNumber {
       if defaults as NSObject == 1 {
            println("inside the conditional: \(defaults)")
            userDefaults.setValue(0, forKey: "aBool")
            userDefaults.synchronize()
            println("Setting aBool to true")
        } else {
            userDefaults.setValue(1, forKey: "aBool")
            userDefaults.synchronize()
            println("setting aBool to false")
        }
    }
}

I don't want to say this is objectively too much code for viewDidLoad, but it's not appropriate for this to all go in viewDidLoad.  This really doesn't belong in the viewDidLoad of any view controller, really.  This is code we seem to only want to load once.  This really belongs in our app delegate, in application(_:didFinishLaunchingWithOptions:).

Once we get our list loaded into NSUserDefaults, we just need to access the values through NSUserDefaults, which is a singleton we can easily access from anywhere.

As a final note about all of this, calling synchronize is generally going to be unnecessary.  NSUserDefaults will synchronize itself when it finds a convenient time.  Moreover, all synchronize does is synchronize what NSUserDefaults has stored in memory with what it has stored in long-term storage.  The only time you can lose something that you put in NSUserDefaults is if your app crashes after you changed a value and before NSUserDefaults had a chance to synchronize.  This is extraordinarily rare.  And for emphasis, realize that I said this happens when your app crashes.  If the user exits your app, NSUserDefaults will synchronize before the app is allowed to completely exit.

And when you access the values in NSUserDefaults, you'll get the value it was most recently set to (which isn't necessarily what is saved to the permanent storage at that moment).

Calling synchronize just forces the synchronization to happen at that very instant, which is entirely unnecessary.

And, for the record, our inner most conditional can be simplified:

if let defaults = userDefaults.valueForKey("aBool") as? NSNumber {
    userDefaults.setValue(!defaults.boolValue, forKey: "aBool")
}


Return True if num is within 2 of a multiple of 10 Is there a way to make my code simpler/more pythonic?

  Given a non-negative number "num", return True if num is within 2 of a multiple of 10. Note: (a % b) is the remainder of dividing a by b, so (7 % 5) is 2. See also: Introduction to Mod

  near_ten(12) → True

  near_ten(17) → False

  near_ten(19) → True

def near_ten(num):

    a = num % 10
    if  (10 - (10-a)) <= 2 or (10 - a) <= 2:
        return True
    else:
        return False

You could simplify your code to make it faster and also avoid the 'if-else' construct:

def near_ten(num):
    a = num % 10
    return 8 <= a or 2 >= a

Here's another version: (Thanks to the comment of @mkrieger1)

def near_ten(num):
    return not(2 < (num % 10) < 8)

I think the name of the function is misleading. Maybe you should rename it to something like: near_multiple_of_ten(num)


GUI design in Java Swing I am developing a GUI using Swing, but before my project gets too complex.  I would like to have a good design structure. I have a few ideas and will use a simple example to illustrate.  Some feedback on whether or not this is a good approach would be great.

This is what my GUI looks like:

There are 2 Jpanels LeftPanel and RightPanel. When Show Name is clicked the name in that panel pops up.

My approach for this is:

If a component is used more than once, I write a class(GetNameButton,NameLabel).
I write a class for each panel.  As the project gets bigger, the JFrame class won't get too cluttered and changes to a panel won't affect the JFrame class.

Here is the code:

public class MyFrame extends JFrame{

    public MyFrame(){
        this.setLayout(null);
        this.setBounds(100,100,400,400);
        this.add(new LeftPanel());
        this.add(new RightPanel());
        this.setVisible(true);
    }

    public static void main(String[] args) {
        new MyFrame();
    }

}

LeftPanel

public class LeftPanel extends JPanel implements ActionListener{
    CloseButton cb = new CloseButton();
    JLabel name = new JLabel("foo");
    GetNameButton gnb = new GetNameButton();

    public LeftPanel(){
        this.setLayout(null);
        this.setBounds(20, 20, 160, 320);
        this.setBorder(new SoftBevelBorder(BevelBorder.RAISED, null, null, null, null));
        this.add(new NameLabel());

        name.setBounds(50,10,40,10);
        this.add(name);
        this.add(cb);

        this.add(gnb);
        gnb.addActionListener(this);
    }

    @Override
    public void actionPerformed(ActionEvent e) {
        if(e.getSource()==gnb){
            JOptionPane.showMessageDialog(null, name.getText());
        }

    }
}

RightPanel

public class RightPanel extends JPanel implements ActionListener{
    JLabel name = new JLabel("bar");
    GetNameButton gnb = new GetNameButton();

    public RightPanel(){
        this.setLayout(null);
        this.setBounds(200, 20, 160, 320);
        this.setBorder(new SoftBevelBorder(BevelBorder.RAISED, null, null, null, null));
        this.add(new NameLabel());

        JLabel name = new JLabel("bar");
        name.setBounds(50,10,40,10);
        this.add(name);

        this.add(gnb);
        gnb.addActionListener(this);
    }

    @Override
    public void actionPerformed(ActionEvent e) {
        if(e.getSource()==gnb){
            JOptionPane.showMessageDialog(null, name.getText());
        }
    }
}

GetNameButton

public class GetNameButton extends JButton{

    public GetNameButton(){
        this.setText("Show Name");
        this.setBounds(20,100,120,20);
    }

}

NameLabel

public class NameLabel extends JLabel{
    public NameLabel(){
        this.setBounds(10,10,40,10);
        this.setText("NAME: ");
    }

}

CloseButton

public class CloseButton extends JButton implements ActionListener{

    public CloseButton(){
        this.setText("Close");
        this.setBounds(20,50,100,20);
        this.addActionListener(this);
    }

    @Override
    public void actionPerformed(ActionEvent e) {
        if(e.getSource()==this){
            System.exit(8);
        }
    }

}

Variable names. Imagine yourself a couple of months from now on, when you look back at the code. Will you remember without looking at the variable declaration what gnb is short for? I suggest you use longer variable names so that you even won't have to try to remember it. getNameBtn would be a better name, and it's still not too long. Using closeBtn instead of cb is also better. cb could just as well mean clearbright or something else totally irrelevant.
You seem to use setBounds to set the exact layout positions in pixels for your components. This is not recommended as this will not be flexible for resizing the windows. Instead use a proper LayoutManager such as BorderLayout and add some margins to make your buttons be placed approximately where they belong and still be flexible for if someone wants to resize the window. (Trust me, this can be more important than you think).
You seem to extend a lot of JButtons and other things. Technically, what's different between a CloseButton and a GetNameButton? The only differences are their positions, labels and I suppose there will be a difference in what happens when you click them. All these things are properties, it does not warrant extension of the original class. Instead you can use a public static method that creates a JButton with the initialized properties of a "close-button" or a "get-name-button". (The same goes for NameLabel, it does not need to extend JLabel)

Here's code for such a public static method:

public static JButton createGetNameButton() {
    JButton btn = new JButton();
    btn.setText("Show Name");
    btn.setBounds(20,100,120,20);
    return btn;
}

jliv902 has a good point in his comment of putting common features into a base class for your panels. However, think about whether or not you really should extend them or not. Personally I think that extending a JPanel or a JFrame is more OK than extending a button, but I want you to know that there are even reasons for why a JFrame shouldn't be extended. Instead of extending, consider this:

public class MyJPanelContainer implements ActionListener {
    private final JPanel panel;
    public MyJPanelContainer() {
        this.panel = new JPanel();
        // code to setup the panel the way you want it
        someComponent.addActionListener(this);
    }

    @Override
    public void actionPerformed(ActionEvent e) {
        // ...
    }
    public JPanel getPanel() {
        // Technically, in another approach, you might not even need this getter, but that's a whole other story.
        return this.panel;
    }
}

If you would write a class like that, it will encapsulate the JPanel and thus hide several methods. The easiest effect to describe for using this approach is that it will drastically decrease the number of visible elements when you press Ctrl + Space, which will make it easier to see the method you are looking for.


Review of reservoir sampling I've been studying Reservoir sampling for a couple of days.  What I've tried here is draw a uniformly random sample of size 3 from bigger data (the 26 characters of the English alphabet) via reservoir sampling.
Below is what I've come up with.

If anyone could review my code and offer some suggestions, that'd be great.

import java.util.TreeMap;

public class test1 {
    public test1(){
        String[] data = "A B C D E F G H I J K L M O P".split(" ");
        double[] freq = new double[data.length];
        int k = 3;
        String[] sample = new String[k];

        // the tree map stores the count of appearance of each letter during
        // the total sampling procedure
        TreeMap<String, Double> dmap = new TreeMap<String, Double>();
        for (int index = 0; index < data.length; index++) {
            dmap.put(data[index], 0.0);
        }

        for (int loop = 0; loop < 1000000; loop++) {
            int i = 0;
            // initiate the sample
            while (i < k) {
                sample[i] = data[i++];
            }
            // start sampling from the reservoir
            for (; i < data.length; i++) {
                int r = (int) (Math.random() * (i + 1));
                if (r < k) {
                    sample[r] = data[i];
                }
            }
            // update the count of each entry in the map
            for (String s : sample) {
                dmap.put(s, dmap.get(s) + 1);
            }
        }
        int index = 0;
        for (Double s : dmap.values().toArray(new Double[dmap.size()])) {
            freq[index++] = s;
            // System.out.println(s);
        }
        // calculate statistics
        double mean, stddev , cov;
        mean = stats.mean(freq);
        stddev = stats.stddev(freq);
        cov = (stddev / mean) * 100;
        System.out.println(" mean                   : " + mean);
        System.out.println(" coeff of variation(%)  : " + cov);     }

    public static void main(String[] args) {
        new test1();
    }
}

Stats class:

final class stats {

    private stats() {}

    public static double sum(double[] a) {
        double sum = 0.0;
        for (int i = 0; i < a.length; i++) {
            sum += a[i];
        }
        return sum;
    }

    public static double mean(double[] a) {
        if (a.length == 0)
            return Double.NaN;
        double sum = sum(a);
        return (double) sum / a.length;
    }

    public static double stddev(double[] a) {
        return Math.sqrt(var(a));
    }

    public static double var(double[] a) {
        if (a.length == 0)
            return Double.NaN;
        double avg = mean(a);
        double sum = 0.0;
        for (int i = 0; i < a.length; i++) {
            sum += (a[i] - avg) * (a[i] - avg);
        }
        return sum / (a.length);
    }
}

My output:

 mean                   : 200000.0
 coeff of variation(%)  : 0.18734211130086761

Edit

I'v received some very helpful suggestions regarding the structure of the code .
the generic version provided by 200_success was really a nice one.
But some of the "procedural"-ness i feel that my code has , is mainly because the api for the actual code for which this was a test-case , demanded a static method which only read the input and gave a sample output. Hence to keep things visually similar , i tried putting them all in one method... The constructor ( not an ideal option many would rightly argue ).

Why the constructor? Well , it made me get away with just one small line in the main method. ( I was feeling pretty lazy back then .)

Edit 2 :
Found out about R yesterday, after a little youtube-ing for for tutorials , made a few plots . Thought i'd upload a couple of them , just for fun. (and a better view of the data)

Box-plot of the number of times each letter of the alphabet(A-Z) had come up

Bar-plot of the same data

The algorithm itself looks solid, and your code looks good!  I just wanted to provide some general style tips for Java.

First, why are you performing the core of your logic in the test1 constructor?  That's highly irregular for Java.  A constructor in general should only perform processing to initialize values unique to each instance of the class.  It would be much more object-oriented to either move all that code directly into your main(String[] args) method or into a different static method.  Having a test1 instance doesn't even make sense here.

Second (and much less importantly), you are breaking with several Java style conventions.  I'm assuming you come from a Python background, because your code "feels" very Pythonic.  That's not a bad thing, but they're two very different languages.  Python attempts to always be short and to the point, while Java emphasizes verbosity and long, descriptive names for things.  As such, here are a few tips to make your code more Java-esque (if you care):

Class names should have each word capitalized, like in ThisTestClassName.  So yours should be Test1 and Stats.
Class names and variable names are also generally not abbreviated.  So for example, your stats class would likely be called Statistics.
Likewise, your method names would likely be renamed to something more descriptive.  While anyone with a statistics background will know what you mean by stddev, it's standard for Java methods to be unabbreviated verb phrases.  So it would be more appropriate to rename this method as getStandardDeviation().  Believe it or not, this really does make a difference in code readability.  For example, when scanning your code, I was very confused by this line at first (Math.sqrt(var(a))) because it seemed like you were trying to do some odd JavaScript casting of a at a glance.  A more descriptive method name like getVariation() would have easily avoided that confusion.
To a lesser degree the "make things more descriptive" points from above apply to variable names as well, though it's much more common (if still frowned upon) to see this in local scope variables.  For example, your code gets a little obfuscated in your test1() constructor because you use at least four variables which are only one letter: i, k, r, and s.  In theory, with well-chosen variable names, I should be able to understand what the code is trying to do at any point without much surrounding context.  That's the goal of making "self-documenting code".

And finally, one small tip which might clean up your code a little bit concerning this line:

for (Double s : dmap.values().toArray(new Double[dmap.size()]))

Here you can actually just use .toArray(new Double[0]) if you feel that would make your code less wordy.  As you can see in the documentation, you don't actually have to pass in an array of the appropriate size to the toArray() method; it only uses your array for infer the type to return to you.  So by just initializing an empty, zero-length array like this, you may be able to improve code readability a bit (and performance by just a small amount, by saving on memory allocation).  There's no consensus or standard on this.  Just personal preference.


Is this a good example of Reactive Extensions? How can I make this better? using System;
using System.Collections.Generic;
using System.Linq;
using System.Reactive.Linq;
using System.Threading;
using Newtonsoft.Json;
using Newtonsoft.Json.Linq;
using RestSharp;

namespace ReactiveExtensions
    {
    class Program
        {
        static void Main(string[] args)
        {
            var start = Observable.Start(() =>
            {
                Console.WriteLine("Getting some work done");
                for (var i = 0; i < 10; i++)
                    {
                    Thread.Sleep(10000);
                    //go call stackoverflow
                    var d = CallStackOverFlow();
                    //send the result for checking to a delegate
                    foreach (var q in d.questions)
                        {
                            var list = (q.tags as JArray).Values().Select(v => v.ToString()).ToList();
                            var result = FindByTag(list);
                            if(result)
                                Console.WriteLine("Quesion title :{0}",q.title);
                        }
                 }
                return "I am done observing";
            });
            Console.WriteLine("Subscribing"); // Make visible when we subscribe
            start.Subscribe(Console.WriteLine);
            Console.ReadKey();
        }

        private static readonly Func<List<String>,bool> FindByTag =(x => x.Contains("c#"));

        private static dynamic CallStackOverFlow()
        {
            var client = new RestClient("http://api.stackoverflow.com/1.1");
            var request = new RestRequest("/questions/no-answers", Method.POST);
            var response = client.Execute(request);
            var content = response.Content; // raw content as string
            dynamic deserialised = JsonConvert.DeserializeObject(content);
            return deserialised;
        }
     }
  }

Please add the tag for ReactiveExtensions , my reputation does not allow me to do that.

possible improvements :

make CallStackOverflow asynchronous
use Observavle.Timer instead of thread.sleep

Here is what it would look like

class Program
{
   static void Main(string[] args)
   {

        Observable
        .Timer(TimeSpan.Zero,TimeSpan.FromSeconds(10))
        .Take(10)
        .Select(() => CallStackOverFlow().ToObservable())
        .Select(d => {
             //send the result for checking to a delegate
               foreach (var q in d.questions)
                   {
                       var list = (q.tags as JArray).Values().Select(v => v.ToString()).ToList();
                       var result = FindByTag(list);
                       if(result)
                           Console.WriteLine("Quesion title :{0}",q.title);
                   }
         })
         .Wait();

       Console.WriteLine("Subscribing"); // Make visible when we subscribe
       start.Subscribe(Console.WriteLine);
       Console.ReadKey();
   }

   private static readonly Func<List<String>,bool> FindByTag =(x => x.Contains("c#"));

   private static async  Task<dynamic> CallStackOverFlow()
   {
       var client = new RestClient("http://api.stackoverflow.com/1.1");
       var request = new RestRequest("/questions/no-answers", Method.POST);
       // -- async invocation
       var response = await client.ExecuteAsync(request);
       // or  var response = await Task.Factory.FromAsync<TResponse>(client.BeginExecute, client.EndExecute, null);
       var content = response.Content; // raw content as string
       dynamic deserialised = JsonConvert.DeserializeObject(content);
       return deserialised;
   }
}


Merge Sort: exercise I've decided to learn Haskell by following the current Coursera algorithms course using Haskell to implement the algorithms discussed.

First up is merge sort, which I've implemented as shown here. I'm brand new to Haskell so looking for any suggestions for improvements to the code - it seems quite verbose - and pointers on anything I may be doing wrong or in a weird way because I don't know what I'm doing.

module Main where

import System.Environment

main :: IO ()
main = do
  args <- getArgs
  mapM_ putStrLn $ sort args

sort :: (Ord a) => [a] -> [a]
sort xs = reduce True $ map (arr) xs

arr :: a -> [a]
arr x = [x]

reduce :: (Ord a) => Bool -> [[a]] -> [a]
reduce _ []            = []
reduce forwards (x:[]) = if forwards then x else reverse x
reduce forwards xs     = reduce (not forwards) (combine forwards [] xs)

combine :: (Ord a)   => Bool -> [[a]] -> [[a]] -> [[a]]
combine forwards acc []       = acc
combine forwards acc (x:[])   = (reverse x):acc
combine forwards acc (x:y:zs) = let merged = merge forwards x y
                                    acc'   = merged:acc
                                in  combine forwards acc' zs

merge :: (Ord a) => Bool -> [a] -> [a] -> [a]
merge forwards xs ys = mergeIter forwards [] xs ys

mergeIter :: (Ord a) => Bool -> [a] -> [a] -> [a] -> [a]
mergeIter _        acc [] []     = acc
mergeIter forwards acc (x:xs) [] = mergeIter forwards (x:acc) xs []
mergeIter forwards acc [] (y:ys) = mergeIter forwards (y:acc) [] ys
mergeIter forwards acc (x:xs) (y:ys)
    | x <=  y && forwards     = mergeIter forwards (x:acc) xs (y:ys)
    | x > y && not forwards   = mergeIter forwards (x:acc) xs (y:ys)
    | otherwise               = mergeIter forwards (y:acc) (x:xs) ys

One specific question I have is around recursive function with accumulator arguments - you'll see in the listing that I've had to create a mergeIter function just to allow a default for the initial argument for the accumulator.

As far as I can see Haskell doesn't support default values for arguments - should I be doing something else instead? A fold maybe?

OK, so let's see. arr x = [x] is usually called wrap, or just the operator section (:[]) is used. But map can also be written with list comprehension, giving us

sort :: (Ord a) => [a] -> [a]
sort xs = reduce True [[x] | x <- xs]

I find it much more visually apparent that way. We usually use short variable names in Haskell, with the same goal. When I see a variable fw and then not fw I get it that it is a toggle, a flag of some sorts, I don't need to see the long name like forwardorbackwardsflag explaining it to me in words while at the same time obscuring this visually. So we get

  where
    reduce _ []    = []
    reduce fw [xs] = if fw then xs else reverse xs
    reduce fw s    = reduce (not fw) (combine fw [] s)

we make it an internal function to sort, because this is what it is. It has no otherwise uses on its own. It's too tailored for the specific use here. -- (x:[]) is written also [x], which is a lot simpler. -- We don't have to annotate types of everything. Here reduce is defined right under its use site, so we see that the initial value of fw is True. For me, it is self-documenting enough, but YMMV. Next, combine. There's no need to name all those interim entities. The name of the function itself isn't right, it's too generic. What it does, specifically, is process the elements of its input list in pairs:

    rpairs fw acc []       = acc
    rpairs fw acc [x]      = reverse x : acc
    rpairs fw acc (x:y:zs) = rpairs fw (merge fw x y : acc) zs

It also builds its result in reversed order. We can make it an internal function to reduce, so that its argument fw could be referred to in rpairs body. At the same time there is no need in the separate accumulator really, we can just build the result naturally, in lazy fashion:

  where
    reduce _ []    = []
    reduce fw [xs] = if fw then xs else reverse xs
    reduce fw s    = reduce (not fw) (reverse $ pairs s)
      where
        pairs []       = []
        pairs [x]      = [reverse x]
        pairs (x:y:zs) = merge fw x y : pairs zs

For merge to be able to refer to fw it too should be put here as an internal function to reduce. It is usual to split such functions into two like you did, the interface function ("the wrapper") and the internal "worker" function, except that normally the worker is made internal to the wrapper. But here the wrapper itself is too specialized, it reverses its result so is unlikely to be useable in general, so no need for the split. Thus we arrive at:

sort    :: (Ord a) => [a] -> [a]
sort xs = reduce True [[x] | x <- xs]
  where
    reduce _  []   = []
    reduce fw [xs] = if fw then xs else reverse xs
    reduce fw s    = reduce (not fw) . reverse . pairs $ s
      where
        pairs (x:y:t)     = rmerge [] x y : pairs t
        pairs [x]         = [reverse x]
        pairs []          = []

        rmerge acc [] []  = acc
        rmerge acc xs []  = reverse xs ++ acc
        rmerge acc [] ys  = reverse ys ++ acc
        rmerge acc (x:xs) (y:ys)
          | fw && x <= y || not fw && x > y
                          = rmerge (x:acc) xs (y:ys)
          | otherwise     = rmerge (y:acc) (x:xs) ys

There's no shame in calling rmerge with the initial value as we do here, it's just an internal function here, called from another internal function.

But actually the reversals that rmerge performs are unnecessary, because of Haskell's lazy evaluation. merge now becomes a general function, usable in general. pairs too. reduce performs the same processing step on its input iteratively until a condition is met - there's a higher order function until that already captures this pattern. This leads to some more simplification of the code:

sort    :: (Ord a) => [a] -> [a]
sort [] = []
sort xs = head $ until (null . tail) (pairwise merge) [[x] | x <- xs]
                                  -- (reverse . pairwise merge)
pairwise f (x:y:t) = f x y : pairwise f t
pairwise _ t       = t

merge xs []        = xs
merge [] ys        = ys
merge (x:xs) (y:ys)
       | x <= y    = x : merge  xs (y:ys)
       | otherwise = y : merge (x:xs) ys

I think we would need to toggle between <= and < in merge to ensure the sort's stability, if we were to proceed by (reverse . pairwise merge) steps. But, perhaps we can just throw that reverse away.


Workflow and Design Feedback on an Add / Edit Form I was looking for advice, feedback and information on best practices when designing ASP.NET Web Forms in a .NET web application.

Allow me to post a sample of how I currently do things. I am omitting the ASPX page as it only has 2 textboxes and a button. PersonEntity validates, loads, and saves in this example. Real-world would have a "Person" business object that saves, loads.

The Codebehind file of an "Add / Edit" page:

public partial class PersonAddUpdate : System.Web.UI.Page
{
    public int PersonId
    {
        get
        {
            int _personId = 0;
            if (Int32.TryParse(Request.QueryString["id"], out _personId))
            {
                return _personId;
            }
            else
            {
                return 0;
            }
        }
    }

    protected void Page_Load(object sender, EventArgs e)
    {
        if (!Page.IsPostBack)
        {
            if (PersonId != 0)
            {
                var person = GetPersonById(PersonId);

                if (person != null)
                {
                    BindFormToData(person);
                    ViewState.Add("PersonEntity", person);
                }
            }
        }
    }

    protected bool BindDataToForm(PersonEntity person)
    {
        person.FirstName = txtFirstName.Text;
        person.Lastname = txtLastName.Text;

        ViewState.Add("PersonEntity", person);

        // internal entity validation
        return person.Validate();
    }

    protected void BindFormToData(PersonEntity person)
    {
        txtFirstName.Text = person.FirstName;
        txtLastName.Text = person.Lastname;
    }

    protected PersonEntity GetPersonById(int personId)
    {
        PersonEntity person = new PersonEntity();
        return person.LoadById(personId);
    }

    protected void btnSave_Click(object sender, EventArgs e)
    {
        if (!Page.IsValid)
        {
            return;
        }

        PersonEntity person;

        if (ViewState["PersonEntity"] == null)
        {
            person = new PersonEntity();
        }
        else
        {
            person = (PersonEntity)ViewState["PersonEntity"];
        }

        if (!BindDataToForm(person))
        {
            // failed entity validation
            return;
        }

        if (!person.Save())
        {
            // problem saving
            return;
        }
        else
        {
            Response.Redirect("PersonDetail.aspx?id=" + person.Id.ToString());
        }
    }
}

Workflow:

Page Load:
Get the id from querystring. If a problem arises, return 0. No records in the system have a "0" id. Try to load data. If there's data, set the form values and add it to viewstate.

On Save: Stop if page is invalid. Create an uninitialized PersonEntity. If viewstate contains an entity, then form binding (gets form values) routine does not need new PersonEntity. If viewstate is null, then we are a new record.

Call BindDataToForm which gets form field values and returns false if it fails internal entity validation. Attempt Save and continue if OK.

Questions:

I've been using this pattern for a while now. Do you see anything I can do to improve it? I was trying to find samples of complete forms that handle Add / Edit, but I was unable to find ones that worked as good as this with ORM-generated code.

Any comments, advice, ideas, or questions?

I hope the code is simple to read and understand.

This is my feedback, Please let me know if I am misinterpreting :

1) Storing the entire PersonEntity object in viewstate will have following problems

It will make viewstate heavy and thus make page load slower.
It may not represent the current state of the object.

You can store the personId, recreate personentity object and then call save.
A lot more about viewstate here http://www.freshegg.com/blog/creating-lean-fast-web-pages-view-state_3377

2) You can make the LoadById method static


equal? predicate for lists Exercise 2.54

  Two lists are said to
  be equal? if they contain equal
  elements arranged in the same order.

  For example,

(equal? '(this is a list) '(this is a list))

  is true, but

(equal? '(this is a list) '(this (is a) list))

  is false. To be more precise, we can
  define equal? recursively in terms of
  the basic eq? equality of symbols by
  saying that a and b are equal? if they
  are both symbols and the symbols are
  eq?, or if they are both lists such
  that (car a) is equal? to (car b) and
  (cdr a) is equal? to (cdr b). Using
  this idea, implement equal? as a
  procedure.

I wrote the following:

(define (equal? a b)
  (or (and (and (symbol? a) (symbol? b)) (eq? a b))
      (and (null? a) (null? b))
      (and (and (and (not (null? a))
                     (list? a))
                (and (not (null? b))
                     (list? b)))
           (and (equal? (car a) (car b))
                (equal? (cdr a) (cdr b))))))

Can this be improved?

Since the and operation is associative, one could write:

(and (and (symbol? a) (symbol? b)) (eq? a b))

as:

(and (symbol? a) (symbol? b) (eq? a b))

Similarly:

(and (and (and (not (null? a))
               (list? a))
          (and (not (null? b))
               (list? b)))
     (and (equal? (car a) (car b))
          (equal? (cdr a) (cdr b))))

is equivalent to:

(and (not (null? a))
     (not (null? b))
     (list? a)
     (list? b)
     (equal? (car a) (car b))
     (equal? (cdr a) (cdr b)))

One could extend the definition of equal? to work with both lists and pairs simply by changing the test list? to pair?.  This also obviates the need to test for non-nullity within the same and clause.

Using the above guidelines, one may write a shorter definition thus:

(define (equal? a b)
  (or (and (symbol? a) (symbol? b) (eq? a b))
      (and (null? a) (null? b))
      (and (pair? a)
           (pair? b)
           (equal? (car a) (car b))
           (equal? (cdr a) (cdr b)))))

Conceptually, one may think of null, symbol, and pair as types.  To test for equality of two objects, one needs to check that their types are the same and then test for equality in that type.

Thus, to test for equality, if we see that objects are symbols, test for eq?; if we see that objects are null, no further test is necessary so they're equal; if we see that objects are pairs, test for equality of car and cdr.

This insight allows one to extend the definition of equal? to other types (hash tables, vectors, etc.) if one should choose to do so in the future.


jQuery 'flip' plugin A little while ago I wrote some jQuery code to make a div flip (similar to the Apple Dashboard). I know there is already code that has this idea from Jon Raasch, but this code positioned divs absolutely, which I did not need for my cause.

I just wondered if anyone could give me some advice as to how to tidy this up and make it cleaner.

(function ($) {
  $.fn.extend({
    jFlip: function (options) {
        var defaults = {
            trigF: '.flipDiv',
            trigB: '.flipDivBack'
        };
        var o = $.extend({}, defaults, options);
        return this.each(function () {
            var f = $('.front');
                b = $('.back');

            margin = f.width() / 2;
            width = f.width();
            height = f.height();
            b.hide().css({
                width: '0px',
                height: '' + height + 'px',
                marginLeft: '' + margin + 'px',
                opacity: '0'
            });

            $(o.trigF).on('click', function () {
                var $this = $(this);

                $this.parents().closest(f).animate({
                    width: '0px',
                    height: '' + height + 'px',
                    marginLeft: '' + margin + 'px',
                    opacity: '0'
                }, {
                    duration: 500
                });

                window.setTimeout(function () {
                    $this.parents().next(b).animate({
                        width: '' + width + 'px',
                        height: '' + height + 'px',
                        marginLeft: '0px',
                        opacity: '1'
                    }, {
                        duration: 500
                    });
                }, 500);
            });

            $(o.trigB).on('click', function () {
                $(this).parents().closest(b).animate({
                    width: '0px',
                    height: '' + height + 'px',
                    marginLeft: '' +margin + 'px',
                    opacity: '0',
                    display: 'none'
                }, {
                    duration: 500
                });
                var $this = $(this).parents().siblings(f);
                window.setTimeout(function () {
                    $this.stop().animate({
                        width: '' + width + 'px',
                        height: '' + height + 'px',
                        marginLeft: '0px',
                        opacity: '1'
                    }, {
                        duration: 500
                    });
                }, 500);
            });
        });
    }
  });
})(jQuery);

The div structure is this:

Front of the div

        <button class="flipDiv">Flip</button>

Back of the div

        <button class="flipDivBack">Flip Back</button>

And this is the initiation:

The only options as of yet however are trigF and trigB, which allows you to specify the name of your buttons which will flip the div.

You can see a jsFiddle of this in action here.

Looks cool, nice job!

One this I would say is that the plugin makes too many assumptions about the client's DOM structure. Having fixed selectors like $('.front') and $('.back') within your plugin script is dangerous -- what if I'm working in a page that has a 'front' class on the body?

It's best to let the client pass in exactly what DOM elements they want to manipulate. You could, for example, do something like this:

The html:

I'm in front!

I'm in back!

The initialization:

$('#flipper').jFlip();

In your plugin script, you now know exactly which elements the client wants to work with:

...
return this.each(function() {
 ...
 $flipper = $(this);
 $front = $flipper.find('[data-face=front]:first');
 $back = $flipper.find('[data-face=back]:first');
 ...
});

Now you don't have to deal with any pesky DOM tree traversal, which carries a risk of finding the wrong element.

A couple other notes:

When you're specifying css attrs with jquery, height:
someNumberVariable works just fine. You don't need to wrap it with
'' + someNumberVar + 'px'
Why not set the duration as an optional param?
It would be nice to have a $('#myEl').jsFlip('flip') method. Adding methods like this will add some complexity to your code, but the good news is there are some good plugin patterns out there that you can work off of. IMO, this is the prettiest one I've seen.
Once you've figured out how to add methods to your plugin, it's fairly simple (and best practice) to add a 'destroy' method, that would unbind all of your event handlers. To do this, you need to namespace your events.

For example:

to bind: $flipBtn.on('click.jFlip', function() { ... })

to destroy: $flipBtn.off('.jFlip');   // unbind all events in .jFlip namespace

One other thing: this plugin replicates the functionality of existing CSS3 animations, which will always be faster. This plugin is still useful as a fallback for browsers that are not up to speed on CSS3, but it would be best to check for that capability first, and implement with CSS3 if available.

I hope this is helpful. Keep on rocking the plugins!


Function to get type of the value entered in a textbox Just for fun, I tried creating a function that takes the value entered in a textbox, and determines the type of it. Below is the full code.

 public string GetType(string inputValue)
    {
        //Variables for the parse result
        //Can be discarded. Used because parse function mandates output variable
        int intParseResult;
        decimal decimalParseResult;
        bool boolParseResult;

        //Parse for different datatypes and check the output
        if (int.TryParse(inputValue, out intParseResult))
        {
            return typeof(int).Name;
        }
        if (decimal.TryParse(inputValue, out decimalParseResult))
        {
            return typeof(decimal).Name;
        }
        if (bool.TryParse(inputValue, out boolParseResult))
        {
            return typeof(bool).Name;
        }
        //If all parse fails, return string type.
        return typeof(string).Name;
    }

Since the text property is a string, I am trying to parse the input into different types, and returning the type name based on it.

As of now, I see two quick issues. One is that it will need one if condition for each data type. Second is that function is creating variables and discarding them unnecessarily.

I am looking for suggestions on how to improve this code. Again, this code is created just for fun, so no urgency. I will just like to see different possible ways to improve the code.

Here's what you can do:

Since you are not using the out var variable you can remove them altogether and use the discard character which is the underscore to just ignore them _.
I usually prefer to use {} in ifs but this method is so short and all conditions are so similar that I'd make them one-liners.
Don't call the parameter inputValue but simply value. It might be an input outside but inside this method input has no meaning. The value can come from any source.
Don't return a string but let the user of this method do with the Type whatever he likes or call this method GetTypeName so its purpose is clear.

Example:

public Type GetType(string value)
{
    if (int.TryParse(value, out _)) return typeof(int);
    if (decimal.TryParse(value, out _)) return typeof(decimal);
    if (bool.TryParse(value, out _)) return typeof(bool);

    //If all parse fails, return string type.
    return typeof(string);
}


Map a range of integers to a 64-bit integer Given a 64-bit integer x (which is always larger than 0), we wish to map it to an index so that:

when x is between 0..64, we map it to 0,
when x is between 65..128, we map it to 1,
when x is between 129..192, we map it to 2, etc.

Writing such a function get_index(x) is simple, consider say the following program:

#include <iostream>
#include <cstdint>
#include <cmath>
#include <cassert>

// For benchmarking
#include <vector>
#include <algorithm>

std::uint64_t get_index(std::uint64_t x)
{
    return std::ceil(x / 64.0) - 1;
}

int main()
{
    // To see the expected outcome.
    /*
    for (int i = 1; i <= 129; ++i)
    {
        std::int64_t idx = get_index(i);
        std::cout << "i : " << i << " = " << idx << "\n";
    }
    */

    // A small performance benchmark.
    // Just in case print something so that the compiler doesn't optimize things away.
    std::vector<std::uint64_t> dummy;
    const std::uint64_t MAX = 10000000;
    for (std::uint64_t i = 1; i <= MAX; ++i)
    {
        dummy.push_back(get_index(i));
    }

    std::cout << dummy.size() << " " << *max_element(dummy.begin(), dummy.end()) << "\n";
}

Is there a (much) faster implementation for get_index(x)? Likely one that completely avoids floating-point math, ceilings etc.

For instance, I know that dividing by 64 is the same thing as bit-shifting x to the right by 6, but I'm not much of a bit hack wizard to figure out the rest without resorting to things that seem messy.

std::uint64_t get_index(std::uint64_t x)
{
    return 0u == x ? 0u : (x-1u)>> 6;
}

Will it faster, I doubt.

By the way, your code for x = 0 will yield -1, which is not same as your description. if you do not need x = 0 input, then you can remove the if statement as

std::uint64_t get_index(std::uint64_t x)
{
    return (x-1u)>> 6;  // for this, x must be not 0u.
}


Accessing list elements when counting the number of rectilinear paths I am trying to solve the RIVALS problem on SPOJ: Starting from (0, 0), how many paths are there to reach point (X, Y), only taking one-unit steps moving to the right or up?  Constraints: 0 ≤ X ≤ 106, 0 ≤ Y ≤ 106, answer is to be given modulo (109 + 7).

I am facing a performance issue with accessing elements of a list. I am also facing a runtime error if I am trying to access the list element.

In order to get the nth element I am using [1,1,1,2]!!n, but I have been told that this runs in O(n) which is very slow.  Is there any other data structure in Haskell which gives O(1) access time for nth element?

About my algorithm: the number of way to move form (0,0) to (x,y) in 2D plain and answer is:

$$\frac{(x+y)!}{x!\ y!}$$

{-# LANGUAGE BangPatterns #-}
{-# OPTIONS_GHC -O2 #-}
import Data.List
import Data.Bits
import Data.Maybe
import Control.Monad
import qualified Data.ByteString.Char8 as C

seqI :: Integer -> Integer
seqI x = seq x x

seqB :: Bool -> Bool
seqB x = seq x x

modExp' :: Integer -> Integer -> Integer -> Integer -> Integer
modExp' r b p m = let nextR = seqI $ (seqI $ r*b)`rem` m
                      nextB = seqI $ (seqI $ b*b)`rem` m
                      nextP = seqI $ shiftR  p 1
                      incl  = seqB $ testBit p 0
                      cont  = seqB $ nextP  /= 0
                  in  if incl
                         then if cont
                                 then modExp' nextR nextB nextP m
                                 else nextR
                         else if cont
                                 then modExp' r     nextB nextP m
                                 else r

modExp :: Integer -> Integer -> Integer -> Integer
modExp b p m = modExp' 1 b p m

readInts :: IO [Int]
readInts = fmap (map read.words) getLine

read2Ints :: IO [Int]
read2Ints = do
     ints <- readInts
     if length ints == 2 then return ints else do
         putStrLn ("sorry, could you give me exactly two integers, "
                  ++ "separated by spaces?")
         read2Ints

p = 1000000007::Integer
ip = 1000000005::Integer
mtimes a b =  (a * b)`rem`p
facts = scanl mtimes 1 [1..2000001]

anstable ::[Integer]
anstable  =  facts

solve ::Int->Int-> Integer
solve x y = anstable!!(x+y)*(modExp (anstable!!x) ip p)*(modExp (anstable!!y) ip p)`rem`1000000007

main :: IO ()
main = do
    n <- readLn
    forM_ [1..n::Int] (\i -> do
       xs <- read2Ints
       print $(solve (head xs) (last xs)))

Code organization

The definitions of readInts and read2Ints disrupt the mathematical code.  The I/O routines belong further down, just before main.

import Data.Maybe and import qualified Data.ByteString.Char8 as C appear to be superfluous.

Abstraction

It is not immediately obvious from reading

solve x y = anstable!!(x+y)*(modExp (anstable!!x) ip p)*(modExp (anstable!!y) ip p)`rem`1000000007

that it is calculating

$$ \left(\begin{array}{c} x+y \\ y \end{array}\right) = \frac{(x + y)!}{x!\ y!}\mod p $$

I would expect to see

solve x y = modCombination (x + y) y p

modCombination n r p =
  (factorial n) * (modInvFactorial r) * (modFactorial (n - r)) `rem` p'
  where …

In some ways, you come close, but anstable obscures your intention.  Why not ansTable, with the customary capitalization so that it doesn't look like an stable? Why did you rename it from fact in the first place, making it more confusing?  Why is 1000000007 hard-coded at the end, when you could have used p?

What is ip?  1000000005 is a magic number that appears nowhere in the stated challenge.  What about 2000001?  Where did that limit come from?


Python program that obfuscates an email address I decided to resume my (limited) Python skills after not coding for a long time.

Here's a very simple self-contained program to convert the characters of an email address into their HTML entities (the purpose is to make the email invisible to spamrobots).

Any comment for improvement? (e.g. args parsing, code style, etc.)
Apart from the -h option in argparse, what is the standard way to add some documentation for it, such as a manpage or some embedded help?

#!/usr/bin/python

#
# mung - Convert a string into its HTML entities
#

import argparse

parser = argparse.ArgumentParser(description = 'Convert a string into its HTML entities.')
parser.add_argument('string_to_mung', help = 'String to convert')
parser.add_argument('-l', '--link', action = 'store_true', help = 'Embed the munged string into a mailto: link')
args = parser.parse_args()

def mung(plain):
    munged = ''
    for c in plain:
        munged = munged + '&#' + str(ord(c)) + ';'
    return munged

string_munged = mung(args.string_to_mung)

if (args.link):
    print('')
    print(string_munged + '')
else:
    print(string_munged)

The code is straightforward and reads well, but:

string concatenation is not very efficient, especially in a loop. You'd be better off using str.join on an iterable;
encoding the mailto: part yourself impairs readability and maintenance, if only you had a function to do it for you. Oh wait...
The comment at the beginning would be better as a module docstrings, you would then be able to use it as the argparse description using __doc__;
You should avoid interleaving code and function definition, and protect top level code using an if __name__ == '__main__': guard.

Proposed improvements:

#!/usr/bin/python

"""Convert a string into its HTML entities"""

import argparse

def command_line_parser():
    parser = argparse.ArgumentParser(description=__doc__)
    parser.add_argument('string_to_mung', help='String to convert')
    parser.add_argument('-l', '--link', action='store_true',
                        help='Embed the munged string into a mailto: link')
    return parser

def mung(plain):
    return ''.join('&#{};'.format(ord(c)) for c in plain)

if __name__ == '__main__':
    args = command_line_parser().parse_args()
    string_munged = mung(args.string_to_mung)
    if (args.link):
        string_munged = '{1}'.format(mung('mailto:'), string_munged)
    print(string_munged)


Rust nth prime implementation I have implemented an nth prime algorithm in Rust that uses value lookup for n < 6, trial division for n < 1000, and a sieve function for n > 999. How can it be better optimized?

pub fn nth(x: u32) -> Result<u64, String> {

match x > 0 {
    true => match x {
        1 => Ok(2),
        2 => Ok(3),
        3 => Ok(5),
        4 => Ok(7),
        5 => Ok(11),
        x if x < 1000 => Ok(trial_div(x)),
        _ => Ok(sieve(x)),
    },
    false => Err(String::from("invalid input")),
  }
}

fn trial_div(x: u32) -> u64 {
  let mut primes: Vec<u64> = Vec::with_capacity(x as usize);

  primes.push(2);

  if x > 1 {
    primes.push(3);
  }

  let mut next_checked = *primes.last().unwrap() + 2;

  while primes.len() < x as usize {
        if primes.iter().all(|&i| next_checked % i != 0) {
            primes.push(next_checked)
        }
        next_checked += 2;
  }
  *primes.last().unwrap()
}

fn sieve(x: u32) -> u64 {
  let x = x as f64;

  let upper_limit = (x * ((x*x.ln()).ln())).floor() as usize;
  let mut prime_indices = vec![true; upper_limit];

  for i in 2..upper_limit {
    if prime_indices[i] {
        let mut counter = 0;
        while (i.pow(2) + (counter * i)) < upper_limit {
            prime_indices[(i.pow(2) + (counter * i)) as usize] = false;
            counter += 1
        }
    }
  }

  let mut prime_x = 1;
  let mut prime_count = 0;
  for i in 2..upper_limit {
    if prime_indices[i] {
        prime_count += 1;
        prime_x = i
    }
    if prime_count == x as u64 {
        break
    }
  }
  prime_x as u64
}

There are some optimizations from the original algorithm that can be included here: the base iteration of the prime p before the sieve is filled with its multiples can start with p = 2 and subsequently with odd numbers only (2, 3, 5, ..., upper_limit). Another useful refinement is that starting from 3, we only need to set the indices of the sieve with a step by 2 p. So for p = 3, we'd only have to clear (9, 15, 21, 27, ...). The rest of the review will be focused around idiomatic Rust, while integrating these optimizations.

In function nth, matching on a boolean value is something awkward to do, since if statements are also expressions and are more readable. Even better here is to turn it into another match clause for the error. Values of type u32 cannot be negative, so testing for 0 is enough.

match x {
    0 => Err("invalid input".to_string()),
    1 => Ok(2),
    2 => Ok(3),
    3 => Ok(5),
    4 => Ok(7),
    5 => Ok(11),
    x if x < 1000 => Ok(trial_div(x)),
    x => Ok(sieve(x)),
}

To create a String out of a &str, you can also use the to_string() or to_owned() methods. "invalid input".into() would also work here because the program already expects a String for the error type and String implements From<&str>.

We can see multiple uses of indexing operations over the prime_indices vector (e.g. prime_indices[i.pow(2) + (counter * i)] and if prime_indices[i]). While these uses are not necessarily wrong, the operation is bound-checked at run-time and it isn't guaranteed that the compiler will optimize them out. The preferred way of manipulating collections and other sequences of data is with the Iterator API. The use of iterators can become tricky when dealing with mutable collections, since there cannot be multiple borrows to the same vector. Nevertheless, we can remove most of these uses, resulting in better optimized and more idiomatic code, potentially less prone to indexing mistakes that could lead to a panic.

At the beginning of sieve, line 42, we have one of these cases where it is best to leave the ranged iteration alone, because the inner loop will modify the same content the outer loop is attempting to read. When filling the sieve for each multiple of i, we can rethink this operation over the vector as a stepped iteration starting from i.pow(2) (this can be replaced with i * i) with a step i. The step_by iteration adaptor is unfortunately not stable yet, but there is an equivalent Itertools#step method in the itertools crate. The result:

for i in 2..upper_limit {
    if prime_indices[i] {
        for p in prime_indices.iter_mut().skip(i * i).step(i) {
            *p = false;
        }
    }
}

Or even better, with the aforementioned optimizations:

for p in prime_indices.iter_mut().skip(4).step(2) {
    *p = false;
}
for i in (3..upper_limit).step(2) {
    if prime_indices[i] {
        for p in prime_indices.iter_mut().skip(i * i).step(2 * i) {
            *p = false;
        }
    }
}

Then, we want the xth prime number from the vector. We can achieve the same outcome as your loop with the following sequence of adaptors, commented one by one for clarity:

let prime_x = prime_indices.into_iter()
    .enumerate()           // keep track of original indices
    .skip(2)               // start at number 2
    .filter(|&(_, p)| p)   // primes only
    .map(|(i, _)| i)       // discard boolean
    .nth((x - 1) as usize) // take the xth prime (1-indexed minus 2 skipped)
    .unwrap_or(1);         // default to 1

My final advice is to consider running the rustfmt tool to format the entire code, as I've seen some irregular indentations. Here is the full result in a Rust Playground.


Notify one of hundreds of viewmodels with MVVM Light messenger Assume a viewmodel for hosting a collection of item viewmodels. There can be some hundreds of items in the collection. The items are part of the business model and have an id and a name. The name for a specific item can be changed somewhere in the application and the viewmodel in the collection needs to be notified to refresh the property:

public interface IItem
{
    Guid Id { get; }
    string Name { get; }
}

public class ItemModel : ViewModelBase
{
    private IItem _item;

    public ItemModel(IItem item)
    {
        _item = item;
    }

    public void Refresh()
    {
        RaisePropertyChanged(nameof(Name));
    }

    public Guid Id { get { return _item.Id; } }

    public string Name { get { return _item.Name; } }

    private bool _isSelected;

    public bool IsSelected
    {
        get { return _isSelected; }
        set { Set(ref _isSelected, value); }
    }
}

public class ListModel : ViewModelBase
{
    public ListModel(IEnumerable<ItemModel> items)
    {
        Items = new ObservableCollection<ItemModel>(items);
    }

    public ObservableCollection<ItemModel> Items { get; }
}

I have tried the following solutions, but i am not sure what the best one is:

register every item at creation and let the messenger always notify them all

public class ItemModel : ViewModelBase
{
    public ItemModel(IItem item)
    {
        _item = item;

        MessengerInstance.Register<NotificationMessage>(this, (msg) => {
            if (msg.Notification == "REFRESH" && IsSelected)
            {
                Refresh();
            }
        });
    }

    [...]

register / unregister messenger on selection, so there will be only one viewmodel be registered

public class ItemModel : ViewModelBase
{
    [...]

    public bool IsSelected
    {
        get { return _isSelected; }
        set
        {
            if (Set(ref _isSelected, value))
            {
                if (value)
                {
                    MessengerInstance.Register<NotificationMessage>(this, (msg) => {
                        if (msg.Notification == "REFRESH")
                        {
                            Refresh();
                        }
                    });
                }
                else
                {
                    MessengerInstance.Unregister<NotificationMessage>(this);
                }
            };
        }
    }
}

register the hostig viewmodel and iterate over the items, so there wil be only one registration too

    public ListModel(IEnumerable<ItemModel> items)
    {
        [...]

        MessengerInstance.Register<NotificationMessage<Guid>>(this, (msg) => {
            if (msg.Notification == "REFRESH")
            {
                var item = Items.Single<ItemModel>(x => x.IsSelected);
                item.Refresh();
            }
        });
    }

which solution would you prefer? register and unregister on every selection or having hundereds of viewmodels registered to the messenger or itereate the items yourself? maybe there is also a better solution?

It should be up to that ViewModel how it handles messages and if it wants to ignore them or act on them.  So that rules out option 3.  That just leaves option 1 and 2.  Option 2 will work but as a developer I wouldn't normally think that setting a property of IsSelected to have side effects and we all know side effects are bad.  I just think it adds a bit more in the maintenance that doesn't need.   I typically just do option 1 and I typically have one method in a class that handles all the registrations for the messages and what they should do.  Just makes maintenance easier if you have a message issue you know where they are all getting registered at.

And since this is CodeReview and all code is subject to review I would recommend you get rid of "magic" strings and make a class that holds them all.  Something like

public static class Messages
{
    public const string Refresh = "REFRESH";
}


Check and clear last value I have a method like this:

public boolean wasUserInactive() {
    boolean result = userInactive;
    userInactive = false;
    return result;
}

This works, but is there a “smarter” way to code this?

This method satisfies two concerns:

mark users as active
return the last status of inactivity

This should be two separate methods so as to not violate SRP and enable checking of that status without resetting it.

wasUserInactive should be isUserInactive and just return userInactive;. Additionally there should be a method resetting the inactive status to false.

Don't introduce quantum behaviour in your code, where observing a state changes it!


Doubly linked list in C++ Please, review the code:

#include <iostream>
using namespace std;

template <class T>
struct Node
{
  Node(T data) : data(data), next(NULL), prev(NULL) {}
  T data;
  Node * next;
  Node * prev;
};

template <class T>
class DoublyLinkedList
{
public:
  DoublyLinkedList() : head(NULL), tail(NULL) {}
  DoublyLinkedList(const DoublyLinkedList<T> & dll);
  ~DoublyLinkedList();
  void addNode(T data);
  //void insertNodeBefore(int data, int before);
  //void insertNodeAfter(int data, int after);
  void deleteNode(T data);
  template <class U>
  friend std::ostream & operator<<(std::ostream & os, const DoublyLinkedList<U> & dll);
private:
  Node<T> * head;
  Node<T> * tail;
};

template <class U>
std::ostream & operator<<(std::ostream & os, const DoublyLinkedList<U> & dll)
{
  Node<U> * tmp = dll.head;
  while (tmp)
    {
      os << tmp->data << " ";
      tmp = tmp->next;
    }

  return os;
}

template <class T>
DoublyLinkedList<T>::~DoublyLinkedList()
{
  Node<T> * tmp = NULL;
  while (head)
    {
      tmp = head;
      head = head->next;
      delete tmp;
    }
  head = tail = NULL;
}

template <class T>
void DoublyLinkedList<T>::addNode(T data)
{
  Node<T> * t = new Node<T>(data);

  Node<T> * tmp = head;
  if (tmp == NULL)
    {
      head = tail = t;
    }
  else
    {
      while (tmp->next != NULL)
        {
          tmp = tmp->next;
        }

      tmp->next = t;
      t->prev = tail;

      tail = t;
    }
}

template <class T>
void DoublyLinkedList<T>::deleteNode(T data)
{
  Node<T> * tmp = head;
  while (tmp && tmp->data != data)
    {
      tmp = tmp->next;
    }

  if (tmp)
    {
      if (tmp->prev && tmp->next) // no change to head or tail
        {
          tmp->prev->next = tmp->next;
          tmp->next->prev = tmp->prev;
        }
      else if (tmp->prev) // change to tail
        {
          tmp->prev->next = tmp->next;
          tail = tmp->prev;
        }
      else if (tmp->next) // change to head
        {
          tmp->next->prev = tmp->prev;
          head = tmp->next;
        }

      delete tmp;
    }
}

template <class T>
DoublyLinkedList<T>::DoublyLinkedList(const DoublyLinkedList<T> & dll)
{
  Node<T> * tmp = dll.head;
  while (tmp)
    {
      this->addNode(tmp->data);
      tmp = tmp->next;
    }
}

int main()
{
  DoublyLinkedList<int> dll;

  dll.addNode(1);
  dll.addNode(2);
  dll.addNode(3);
  dll.addNode(4);
  dll.addNode(5);

  cout << dll << endl;
  DoublyLinkedList<int> dll2(dll);
  cout << dll2 << endl;

  dll.deleteNode(3);
  dll.deleteNode(1);
  dll.deleteNode(5);

  cout << dll << endl;
  cout << dll2 << endl;

  return 0;
}

Consider replacing Node<T> with:

typedef Node<T> node_t;

...

node_t node;

Consider making your implementation STL-enabled. If I were user of this class, the first thing I'd ask you is "how do I iterate over this list?". Doubly-linked lists are great when adding items in the middle, how do I do it? (it's about operations like insertBefore(), insertAfter()). How do I sort it? How do I check if item is on the list? How do I search using predicate?
Why do you call the method deleteNode if it's only argument is of type T? It's more to findNodeByValueAndDeleteIt(T).
What about assignment operator? You should either declare it private (if you don't support it), or implement it explicitly depending on semantics you want to support (deep copy (most likely what you want), reference, COW, etc):

YourList<int> a; // do something with a

YourList<int> b; // do something with b

a = b; // what is "a" here? what if I add nodes to a? will it affect b?

// imagine, scope ends here, dtor for b is called, everything's fine

// dtor for a is called - here you're going to crash, since

// both a and b were using the same memory and this memory is already

// free, since b's dtor has already been called at this point

For methods like void addNode(T data) - why do you pass data by value? Since you store item values by value, in either case you're going to call their copy-ctor. Copy-ctor requires const-reference, so you should consider replacing addNode with void addNode(T const& data).
using namespace std; - is it only for cout instead of std::cout in your main()?

Checked nothing from the "algorithmic" standpoint, C++ only.


Iterator of Container of Container Based on this question I though I would pull together an iterator that iterated over a container of containers.

template<typename C>
class IteratorForContainerOfContainer
{
    public:
        using Container             = C;
        using ContainerContainer    = typename Container::value_type;
        using Contained             = typename ContainerContainer::value_type;

        using Level1Iter            = typename Container::iterator;
        using Level2Iter            = typename ContainerContainer::iterator;

        using iterator_category     = std::forward_iterator_tag;
        using value_type            = Contained;
        using difference_type       = std::size_t;
        using pointer               = value_type*;
        using reference             = value_type&;

        IteratorForContainerOfContainer(C& c)
            : outerLoop(std::begin(c))
            , outerEnd(std::end(c))
            , innerLoop(std::begin(*outerLoop))
            , innerEnd(std::end(*outerLoop))
        {
            nextValidPos();
        }
        IteratorForContainerOfContainer()
            : outerLoop()
            , outerEnd()
            , innerLoop()
            , innerEnd()
        {}

        IteratorForContainerOfContainer& operator++()
        {
            ++innerLoop;
            nextValidPos();
            return *this;
        }
        value_type& operator*()
        {
            return *innerLoop;
        }
        bool operator!=(IteratorForContainerOfContainer const& rhs) const
        {
            return outerLoop != rhs.outerLoop || innerLoop != rhs.innerLoop;
        }

        private:
            void nextValidPos()
            {
                while ((innerLoop == innerEnd) && (outerLoop != outerEnd)) {
                    ++outerLoop;
                    if (outerLoop != outerEnd) {
                        innerLoop = std::begin(*outerLoop);
                        innerEnd  = std::end(*outerLoop);
                    }
                }
                if (outerLoop == outerEnd) {
                    outerLoop   = Level1Iter();
                    outerEnd    = Level1Iter();
                    innerLoop   = Level2Iter();
                    innerEnd    = Level2Iter();
                }
            }
            Level1Iter          outerLoop;
            Level1Iter          outerEnd;
            Level2Iter          innerLoop;
            Level2Iter          innerEnd;
};

So an example of simple usage would be:

int main()
{
    using CC = std::vector<std::vector<int>>;
    using It = IteratorForContainerOfContainer<CC>;

    CC   data = {{1,2,3},{4,5,6},{7,8,9}};

     for(It loop(data);loop != It(); ++loop) {
        std::cout << *loop << " ";
    }
    std::cout << "\n";

}

If you try to get an IteratorForContainerOfContainer for an empty container, your constructor will run into Undefined Behavior when it tries to get the inner iterators because the outer container is empty, outerLoop will be the end iterator, and you can't dereference the end iterator.

You're using a default constructed object to double as an end iterator.  Consider adding in a static method to return one, end(C &c), with appropriate changes to nextValidPos.

If you need to work with a const object, you won't be able to get one of these flattened iterators for it.

You don't define operator++(int), so using it++ won't compile.


Number-guessing game using functions First time writing a "game" in a sense. Used a functions approach, it's simple, looking for a review of the way it's written and of course looking for how should it be written. Also using random for first time (I know I'm a nube).

#Number Guess Game

import random

def gameNumber():
     while True:
        difficulty = input("Type: E, M, H for easy, medium, or hard number range:\n").lower()

        if difficulty == 'e':
            return random.randint(0,20)

        elif difficulty == 'm':
            return random.randint(0,50)

        elif difficulty == 'h':
            return random.randint(0,100)

        else:
            print("Incorrect input")

def numberGuessGame():
    number = gameNumber()
    while True:
        try:
           guess = int(input("Your guess: "))
           if number == guess:
               print("Correct guess!\n")
               return numberGuessGame() if input("play again?(y/any key to exit) ").lower() == 'y' else 0
           print("Too High" if number < guess else "Too Low")
        except ValueError:
            print("must be an integer value\n")

def main():
    numberGuessGame()

if __name__ == '__main__':
    main()

What you have is very good and it certainly functions as expected. I have just a few ideas to consider implementing (more in relation to the game than the code):

Print the number ranges for each difficulty at the start, so that the user knows what range to guess in.
Check if the guess is outside the range.
Count the number of guesses and report back to the user at the end.
Edit the prompts slightly so user knows when input is expected.

I have shown my code below, implementing these features. I reduced it to a single function (to avoid having to use global variables). I have used a dictionary to store the top end of the range of each difficulty (difficulty_ranges) so this can be checked easily later, using fewer if else checks.

I also corrected a few PEP8 style warnings (spacing, use snake case instead of camel case for function name etc).

import random

def number_guess_game():
    # Set up main variables
    guesses = 0
    difficulty_ranges = {'e': 20, 'm': 50, 'h': 100}

    while True:
        difficulty = input(
            "Type E, M or H for easy (0-20), medium (0-50), or hard (0-100) number range:\n").lower()
        try:
            number = random.randint(0, difficulty_ranges[difficulty])
            break  # Need to break the while loop here
        except KeyError:
            print("Incorrect input! Choose again...")  # Encourage user input

    while True:
        try:
            guess = int(input("Your guess: "))
            guesses += 1
            if number == guess:
                print("Correct guess! You used {} guesses.".format(guesses))
                return number_guess_game() if input(
                    "Play again? Press Y to play again or any key to exit.").lower() == 'y' else 0
            elif guess < 0 or guess > difficulty_ranges[difficulty]:
                print("Your guess was out of range!")
            else:
                print("Too High. Guess again..." if number < guess else "Too Low. Guess again...")
        except ValueError:
            print("Must be an integer value.\n")

def main():
    number_guess_game()

if __name__ == '__main__':
    main()

I should add that I am not a Python expert, so other users will likely provide more useful information on the code. I think your layout is good and the logic is easy to follow. I wanted to suggest a few of these extra features as it is a game and these may improve the experience.


Loop + Take for repeatedly taking elements from the same collection I sometimes need to enumerate the same collection multiple times so I wrote an extension that does this for me. I call it Loop. It takes one optional parameter startAt which allows it to start at an offset.

The extension works with enumerators. If it could not MoveNext then it gets a new enumerator and tries again. If the second try failed then the collection is empty, otherwise it starts from the beginning. It uses a local function that handles the enumerator by resetting it if necessary.

public static IEnumerable<T> Loop<T>(this IEnumerable<T> values, int startAt = 0)
{
    if (values == null)
    {
        throw new ArgumentNullException(nameof(values));
    }

    var moves = 0;

    // ReSharper disable once PossibleMultipleEnumeration
    var enumerator = values.GetEnumerator();

    try
    {
        while (true)
        {
            if (TryMoveNext(enumerator, out enumerator))
            {
                moves++;

                if (startAt > 0 && moves <= startAt)
                {
                    continue;
                }

                yield return enumerator.Current;
            }
            else
            {
                yield break;
            }
        }
    }
    finally
    {
        enumerator.Dispose();
    }

    bool TryMoveNext(IEnumerator<T> currentEnumerator, out IEnumerator<T> newEnumerator)
    {
        if (currentEnumerator.MoveNext())
        {
            newEnumerator = currentEnumerator;
            return true;
        }
        else
        {
            // Get a new enumerator because we took all elements and try again.

            currentEnumerator.Dispose();

            // ReSharper disable once PossibleMultipleEnumeration
            newEnumerator = values.GetEnumerator();

            // If we couldn't move after reset then we're done trying because the collection is empty.
            return newEnumerator.MoveNext();
        }
    }
}

I have the feeling like I might have overcomplicated the implementation.

Here are the tests I wrote for it:

[TestClass]
public class enumerableTest
{
    [TestMethod]
    public void Loop_EmptyCollection_NoneTakes()
    {
        var numbers = new int[0];
        Assert.IsFalse(numbers.Loop().Take(2).Any());
    }

    [TestMethod]
    public void Loop_LessThanAvailable_NoLoop()
    {
        var numbers = new[] { 1, 2, 3 };
        Assert.That.Collection().AreEqual(new[] { 1, 2 }, numbers.Loop().Take(2));
    }

    [TestMethod]
    public void Loop_MoreThanAvailable_OneLoop()
    {
        var numbers = new[] { 1, 2, 3 };
        Assert.That.Collection().AreEqual(new[] { 1, 2, 3, 1 }, numbers.Loop().Take(4));
    }

    [TestMethod]
    public void Loop_TwiceTheAvailable_TwoLoops()
    {
        var numbers = new[] { 1, 2, 3 };
        Assert.That.Collection().AreEqual(new[] { 1, 2, 3, 1, 2, 3, 1, 2 }, numbers.Loop().Take(8));
    }

    [TestMethod]
    public void Loop_MoreThanAvailableWithStartAt_OneLoop()
    {
        var numbers = new[] { 1, 2, 3 };
        Assert.That.Collection().AreEqual(new[] { 2, 3, 1, 2, 3, 1 }, numbers.Loop(1).Take(6));
    }

}

Code optimization

This can be shortened:

while (true)
{
    if (TryMoveNext(enumerator, out enumerator))
    {
        moves++;

        if (startAt > 0 && moves <= startAt)
        {
                continue;
        }

        yield return enumerator.Current;
    }
    else
    {
        yield break;
    }
}

To:

while (TryMoveNext(enumerator, out enumerator))
{
    moves++;

    if (startAt > 0 && moves <= startAt)
    {
        continue;
    }

    yield return enumerator.Current;
}

You added the if for "boolean breaking", but at the same time you've subverted the inherent "boolean breaking" that a while loop provides.

Enumeration needed?

Since you are looking to repeat the same collection multiple times, this collection is cyclic in nature. Whenever I think about cyclic data, the modulo operator always comes to mind. It's by far the most common use case for the modulo operator.

According to this SO answer, though there may be a minor difference, the performance difference between enumeration and indexed arrays is reasonably negligible.
Keeping that in mind, I would advocate using a modulo operator simply because it keeps the code cleaner.

Note: The linked answer does not address re-enumeration, which is relevant for your code. But tbh, I'm not sure if re-enumeration is cached (thus not costing any meaningful overhead) or not. I'm going to continue my assumption that the performance differences are too small to be considered relevant for your current use case.

public static IEnumerable<T> Loop<T>(this IEnumerable<T> values, int startPosition = 0)
{
    if (values == null)
    {
        throw new ArgumentNullException(nameof(values));
    }

    T[] array = values.ToArray();
    int count = values.Count();

    while (true)
    {
        yield return array[startPosition++ % count];
    }
}

I kept the example terse (e.g. by inlining startPosition++), but I hope it highlights the reduced complexity of the code in general.

A slight improvement would be to not let startPosition grow beyond the array's size:

    while (true)
    {
        startPosition = (++startPosition) % count;
        yield return array[startPosition];
    }

Though I think this is only really useful if you expect to enumerate up to a point of integer overflow, which I doubt is the case here.

Footnote

I'm torn about the startPosition variable name. Once we start using it as a counter, its name is no longer correct (it's the current position). However, I do think it fits as a parameter name.
We can assign the value to a variable with a better name; but I'm not sure if it's really that necessary for such a terse piece of code.

I can see arguments for either case. I'm unable to make a conclusive decision on whether it's necessary. I'm open to feedback on this.

If you do want to make the name more appropriate, I would suggest the following:

public static IEnumerable<T> Loop<T>(this IEnumerable<T> values, int? startPosition = null)
{
     //...

     int counter = startPosition ?? 0;

     //...
}