Parsing CAN frames from a ByteArray Here's my method used for parsing a ByteArray into multiple frames of CAN interface. It works now correctly but it's somewhat ugly in my opinion.

It's written in Kotlin with use of RxJava2

private fun parseCanFrameBytes(frame: ByteArray): Observable<CanFrame> {
    return Observable.create { emitter ->
        var index = 0
        while (index < frame.size) {
            val byte = frame[index]

            if(byte xor IFM_FRAME_HEADER == BYTE_ZERO) {
                try {
                    val ifmIndex = index
                    val length = frame[ifmIndex + 1].toInt()
                    val dlc1 = length - 3
                    val dlc2 = length - 5
                    val command = frame[ifmIndex + 2].toInt()
                    val data = frame.copyOfRange(ifmIndex + 3, ifmIndex + 2 + length)
                    val isChecksumValid = frame[ifmIndex + 2 + length].toInt() == frame.copyOfRange(ifmIndex, ifmIndex + 2 + length).calculate8XorChecksum()
                    if (!isChecksumValid) {
                        index++
                        continue
                    }

                    val canFrame = when (command) {
                        0 -> {
                            val pgn = data.copyOfRange(0, 2).toHexString()
                            CanFrame(
                                    type = "DLC1",
                                    pgn = pgn,
                                    id = pgn,
                                    value = data.copyOfRange(2, 2 + dlc1).toHexString(),
                                    ts = timestamp(),
                                    data = frame.copyOfRange(ifmIndex, ifmIndex + 4 + length).toHexString()
                            )
                        }
                        2 -> {
                            val pgn = data.copyOfRange(0, 4).toHexString()
                            CanFrame(
                                    type = "DLC2",
                                    pgn = pgn,
                                    id = pgn,
                                    value = data.copyOfRange(4, 4 + dlc2).toHexString(),
                                    ts = timestamp(),
                                    data = frame.copyOfRange(ifmIndex, ifmIndex + 4 + length).toHexString()
                            )
                        }
                        else -> null
                    } ?: continue

                    emitter.onNext(canFrame)
                    index = ifmIndex + length + 4
                } catch (ex: IllegalArgumentException) {
                    index++
                }
            } else {
                index++
            }
        }
    }
}

Nesting Branches

You were worried your code might be a bit ugly. I consider deeply nested branches ugly aka a penalty in readability when they can easily be avoided. For example, your function indents the entire while-loop body to perform this little branch at the end:

if (byte xor IFM_FRAME_HEADER == BYTE_ZERO) {
    // branch body ..
} else {
    index++
}

You could invert the branch condition, use an early continue and avoid nesting branches.

if (byte xor IFM_FRAME_HEADER != BYTE_ZERO) {
    index++
    continue
}
// other code in while-loop no longer needs indentation ..

Shifting the input

You have 3 occasions where you shift the input array to the next index index++. 2 of these have conditions that guard when to shift, the 3th is an exception handler for type IllegalArgumentException. Consider refactoring the code to have only a single connection point in the code where you perform the shift index++. This increases maintainability of the code. You can do this be either (1) trying to avoid the exception and using a condition instead, and use a single flag shouldShift marked true on any of the conditions or (2) throw exceptions for all conditions that require shifting and have a single exception handler performing the shift.

Flow Integrity

The callback invocation  emitter.onNext(canFrame) sits within the try-catch block. This means that if the callback would throw an IllegalArgumentException you'd be shifting the input index++ while you shouldn't have. Ensure callback invocation does not trigger such shifting.


Kattis Challenge - Recount - count who has the most votes in a list of names I am attempting to solve the Kattis problem Recount.

  The recent schoolboard elections were hotly contested: a proposal to swap school start times for elementary and high school students, a controversial new dress code proposal that bans athletic clothes in school, and a proposal to raise real-estate taxes to pay for a new football practice facility, and the list goes on and on. It is now hours after the polls have closed and a winner has yet to emerge!

  In their desperation, the election officials turn to you and ask you to write a program to count the vote!

  Input

  The input consists of a single test case, which is a list of votes cast. Each line in the input contains the name of a candidate for whom a vote was cast. A name may consist of multiple words, separated by spaces. Words contain letters or hyphens, but no other punctuation characters. There will be at least 2 votes on the list. The list of votes ends with a single line containing the characters ***. This line should not be counted. There can be up to 100,000 valid votes.

  Output

  If a candidate obtained a simple or absolute majority of all votes cast (that is, more than any other candidate), output the name of this candidate! If no candidate obtained a simple majority, output: “Runoff!” (don’t forget to include the exclamation mark!)

  Sample Input 1

Penny Franklin
Marti Graham
Connie Froggatt
Joseph Ivers
Connie Froggatt
Penny Franklin
Connie Froggatt
Bruce Stanger
Connie Froggatt
Barbara Skinner
Barbara Skinner
***

  Sample Output 1

Connie Froggatt

  Sample Input 2

Penny Franklin
Connie Froggatt
Barbara Skinner
Connie Froggatt
Jose Antonio Gomez-Iglesias
Connie Froggatt
Bruce Stanger
Barbara Skinner
Barbara Skinner
***

  Sample Output 2

Runoff!

#include <iostream>
#include <vector>
#include <string>
#include <algorithm>
#include <functional>
using namespace std;

struct voter
{
    string name;
    int numvotes;
    bool operator==(const string   &) const;
    bool operator<(const voter   &) const;
    bool operator>(const voter   &) const;

};
bool voter::operator==(const string &f) const
{
    return name == f;
}

bool voter::operator<(const voter &f) const
{
    return (numvotes < f.numvotes);
}
bool voter::operator>(const voter &f) const
{
    return (numvotes > f.numvotes);
}

bool myfunction(voter i, voter j) { return (i<j); }

int main() {
    vector<voter> voters;
    string name;

    while (getline(cin, name))
    {
        if (name == "***")
        {
            break;
        }
        _Vector_iterator<_Vector_val<_Simple_types<voter>>> it = find(voters.begin(), voters.end(), name);
        if (it != voters.end()) {
            it->numvotes += 1;
        }
        else
        {
            voter a;
            a.name = name;
            a.numvotes = 1;
            voters.push_back(a);
        }
    }
    sort(voters.begin(), voters.end(), greater<voter>());

    if (voters[0].numvotes == voters[1].numvotes)
    {
        cout << "Runoff!" << endl;
    }
    else
    {
        _Vector_iterator<_Vector_val<_Simple_types<voter>>>  tt = max_element(voters.begin(), voters.end());
        cout << tt->name << endl;
    }
    return 0;
}

My program will pass all of the tests except the very last one where it "times out". Any input I can get on this will be greatly appreciated. This code does compile and run in VS and here: http://cpp.sh/8l3b

To get this to run on cpp.sh I did have to change the _Vector_iterator<_Vector_val<_Simple_types<voter>>> on lines 43 and 63 to auto. I don't quite understand why, so I would also be very appreciative if you could explain what I should use as a type to be portable.

There are 2 important performance issues here:

find(...) in a vector performs a linear search, \$O(n)\$. You would do better by using a hash map instead, that would allow constant-time search, \$O(1)\$.
Sorting the candidates by vote count is unnecessary to find the ones with the maximum count. Sorting is an \$O(n \log n)\$ operation. You can find the maximum with a linear search, \$O(n)\$, and in a second linear pass you could verify if a run-off is needed. (You could as well make this part of step 1 and do it in a single pass, but that wouldn't change the order of complexity of the algorithm.)

Lastly, a minor thing, but the implementation will fail if there are less than two candidates :-)


Prime generator in Python and C I have written the Rabin-Miller test in Python and ported it to C. But somehow, the C version is ~20 times slower on my system, and I am not sure how. Anyway, here is my code.

Python

import random
import time

def rabin_miller(n):
    if n in [2, 3, 5]:
        return True
    if n < 2 or n % 2 == 0:
        return False

    s = 0
    d = n - 1
    while d % 2 == 0:
        s += 1
        d >>= 1

    for k in range(2 ** 4):
        a = random.randrange(2, n - 1)
        x = pow(a, d, n)
        if x == 1 or x == n - 1:
            continue
        c = False
        for r in range(s - 1):
            x = pow(x, 2, n)
            if x == 1:
                return False
            if x == n - 1:
                break
        else:
            return False
    return True

for i in range(10000):
    rabin_miller(i)

C

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

long mod_pow(long b, long e, long m){
    if(m == 1)
        return 0;
    long c = 1;
    for(long i = 0; i < e; i++)
        c = (c * b) % m;
    return c;
}

int rabin_miller(long n){
    if(n == 2 || n == 3)
        return 1;
    if(n < 2 || n % 2 == 0)
        return 0;

    long s = 0, d = n - 1, k = 10, a, x, stop;
    while(d % 2 == 0)
        s += 1, d >>= 1;

    srand(time(NULL));
    for(long i = 0; i < k; i++){
        a = (long) (((float) rand() / RAND_MAX) * (n - 3)) + 2;
        x = mod_pow(a, d, n);
        if(x == 1 || x == n - 1)
            continue;
        stop = 1;
        for(long r = 0; r < s - 1; r++){
            x = mod_pow(x, 2, n);
            if(x == 1)
                return 0;
            if(x == n - 1){
                stop = 0;
                break;
            }
        }
        if(stop){
            return 0;
        }
    }
    return 1;
}

int main(int argc, char *argv[]){
    for(long i = 1; i < 10000; i++){
        rabin_miller(i);
    }
    return 0;
}

I believe the problem is that long mod_pow is very inefficient, as it takes e steps to complete, as compared to Python's approach which likely only takes log(e) steps. To fix this, you should use an algorithm similar to the following

function modular_pow(base, exponent, modulus)
if modulus = 1 then return 0
Assert :: (modulus - 1) * (modulus - 1) does not overflow base
result := 1
base := base mod modulus
while exponent > 0
    if (exponent mod 2 == 1):
       result := (result * base) mod modulus
    exponent := exponent >> 1
    base := (base * base) mod modulus
return result


Checking for spelling errors with a trie I have been lately working in my C++, and I finished up this trie, and I want to know how my C++ is looking so far.

Node.hpp

#pragma once

#include <memory>

static const int ALPHABETSIZE = 26;

struct Node {
    bool isEnd;
    int prefixCount;
    std::shared_ptr<Node> child[ALPHABETSIZE];
};

Trie.hpp

#pragma once

#include "Node.hpp"
#include <string>
#include <vector>

class Trie {
    std::shared_ptr<Node> head;

public:
    Trie();

    void insert(const std::string& word) const;
    bool search(const std::string& word) const;

    unsigned int wordsWithPrefix(const std::string& prefix) const;
};

Trie.cpp

#include "Trie.hpp"

Trie::Trie() {
    head = std::make_unique<Node>();
    head->isEnd = false;
    head->prefixCount = 0;
    for (int i = 0; i < ALPHABETSIZE; ++i) {
        head->child[i] = nullptr;
    }
}

void Trie::insert(const std::string& word) const{
    auto current = head;
    for (int i = 0; i < word.length(); ++i) {
        int letter = word[i] - 'a';
        if (!current->child[letter])
            current->child[letter] = std::make_shared<Node>();
        current->child[letter]->prefixCount++;
        current = current->child[letter];
    }

    current->isEnd = true;
}

bool Trie::search(const std::string& word) const{
    auto current = head;
    for (int i = 0; i < word.length(); ++i) {
        auto letter = word[i] - 'a';
        if (!current->child[letter])
            return false;
        current = current->child[letter];
    }

    return current->isEnd;
}

unsigned int Trie::wordsWithPrefix(const std::string& prefix) const{
    auto current = head;
    for (int i = 0; i < prefix.length(); ++i) {
        auto letter = prefix[i] - 'a';
        if (!current->child[letter])
            return 0;
        current = current->child[letter];
    }
    return current->prefixCount;
}

Source.cpp

#include <iostream>
#include <fstream>
#include <algorithm>
#include "Trie.hpp"

int main() {
    Trie trie;
    std::ifstream file;
    std::string word = " ", input;

    file.open("dictionary.txt");

    //Ask for input
    std::cin >> input;
    std::transform(input.begin(), input.end(), input.begin(), ::tolower);

    auto beenHere = false;
    //Add every word to the trie.
    while (std::getline(file, word)) {
        std::transform(word.begin(), word.end(), word.begin(), ::tolower);
        //if word's first character equals to input's, go on, else, dont add it onto the trie.
        if (word[0] == input[0]){
            beenHere = true;
            trie.insert(word);
        }
        //If you've already been on the word's first character
        //, and it isn't the same as the input's you are done.
        if (beenHere && word[0] != input[0]) break;
    }

    std::string x = "";
    for (int i = 0, length = input.length(); i < length; ++i) {
        x += input.at(i);
        if (trie.wordsWithPrefix(x) == 0) {
            std::cout << x << "<" << input.substr(i+1, length - 1);
            break;
        }
    }

    file.close();
}

Node

Raw arrays:

std::shared_ptr<Node> child[ALPHABETSIZE];

are really awkward. There's little reason to prefer to use one here. Use a std::array instead:

std::array<std::shared_ptr<Node>, ALPHABETSIZE> children;

Also I pluralized your array name. Seems to make more sense. You also want to make sure that a default-constructed Node is valid, so I find it clearer to just add data member initializers:

struct Node {
    bool isEnd = false;
    int prefixCount = 0;
    std::array<std::shared_ptr<Node>, ALPHABETSIZE> children;
};

Now you don't have to worry about default/value initialization.

You current construct your head via:

head = std::make_unique<Node>();

But head is a shared_ptr so you should do:

head = std::make_shared<Node>();

And with the added data member initializers, that's all we need to do in our constructor. Done!

insertion

Prefer a range-based for loop anytime you want to loop over everything in a container without needing the iterator. It expresses intent better:

for (char c : word) {
    int letter = c - 'a';
    ...
}

Also, what if word contains anything other than a lower-case letter? What if it's a capital letter or a space or a hyphen? This does no error checking here, so leaves open the possibility of undefined behavior due to invalid memory access.

Choice of data structure

Does having 26 children for each node make sense? That could be very memory intensive if our data set is sparse! I'd expect something more like:

std::unordered_map<char, std::shared_ptr<Node>>

with which this would just look like:

auto& next = current->children[letter];
if (!next) next = std::make_shared<Node>();
next->prefixCount++;
current = next;

searching

I would rename search() to contains(), and same points apply about sanity checking your inputs and using a range-based for loop.

wordsWithPrefix

Note that your search and wordsWithPrefix do the same thing: we walk the length of the string, break early if we run out of children, and do something with the result. That suggests adding a helper function:

std::shared_ptr<Node> atPrefix(const std::string& prefix) const;

which does that work for us. With that function, we can implement:

bool contains(const std::string& word) const
{
    auto node = atPrefix(word);
    return node && node->isEnd;
}

unsigned int wordsWithPrefix(const std::string& prefix) const
{
    auto node = atPrefix(prefix);
    return node ? node->prefixCount : 0;
}

Trie construction

You may want to add a constructor that will iterate over and insert a bunch of words. Maybe take two iterators, same as the other standard containers.

cin and getline

Be careful about mixing those two. It doesn't always work.

Also, don't do this:

std::string word = " ", input;

Space isn't at a premium, you can declare your variables on separate lines.

main()

What is input? I don't understand your usage here. Are you mandating a sorted dictionary.txt for this to work? Some comments indicating this would be helpful.

final loop

at() vs operator[] just does range-checking. You don't need that here, so simply use operator[]. Also if what you want to do is divide input, you don't need a second argument to substr() - just the one will automatically print the remainder of the string from there on out. Besides, your length argument is incorrect anyway.

Prefer:

std::string x;
for (char c : input) {
    x += c;
    if (trie.wordsWithPrefix(x) == 0) {
        std::cout << x << '<' << input.substr(x.size());
        break;
    }
}

file.close()

Unnecessary. The ifstream destructor will take care of itself. RAII for the win.


Counting without zeroes Below is a python script that generates the sequence ['a', 'b' .. 'z', 'aa', 'ab' .. 'az', 'ba', 'bb' .. 'zz', 'aaa', 'aab', ..]

This is essentially counting in base 27, replacing every digit with the n-th letter of the alphabet, but skipping any number that would have a '0'.

import string

def gen_labels():
    i = 0
    n = len(string.ascii_lowercase) + 1
    while True:
        i += 1
        j = i
        result = ''
        while True:
            c = j % n
            if not c:
                break
            result = string.ascii_lowercase[c-1] + result
            if j < n:
                break
            j = j // n
        if c:
            yield result

print(list(zip(gen_labels(), range(1000))))

However, the code seems overly long to me for generating such a straightforward series and it's doing a lot of work to break down values that would have a '0' in them in base 27.

What is a more efficient way of generating the exact same (infinite) series?

Note that I'm not worried that much about speed, but mainly about the brevity / simplicity of the algorithm - it seems overly complicated, but I don't really see my way to an efficient realisation.

Well, what you want is just a product of the alphabet, with increasing numbers of elements. You can use itertools.product for this:

from itertools import product, count
from string import ascii_lowercase

def generate_labels():
    """Yields labels of the following form:
       a, b, ..., z, aa, ab, ..., zz, aaa, aab, ..., zzz, ...
    """
    for n in count(start=1):
        yield from map("".join, product(*[ascii_lowercase]*n))

Here is what it outputs:

from itertools import islice

print(list(islice(generate_labels(), 1000)))
# ['a', 'b', ..., 'z', 'aa', 'ab', ..., 'az', 'ba', 'bb', ..., 'bz', ..., 'za', ..., 'zz', 'aaa', 'aab', ..., 'all']

This has the slight disadvantage that the list being passed to product gets larger every iteration. But already with \$n=5\$ you can generate \$\sum_{k=1}^n 26^k = 12,356,630\$ labels, and the list is only about sys.getsizeof([ascii_lowercase]*5) + sys.getsizeof(ascii_lowercase) * 5 = 479 bytes large, so in practice this should not be a problem.

I also made the name a bit longer (and clearer IMO) and added a docstring to briefly describe what the function is doing.


Dependency Inversion Principle and Dependency Injection in C# I was doing some sample example which were perfectly fine. But, when I read about SOLID design principle, I had to change my code.

I have implemented DIP and DI (Constructor Injection) in my code. Please let me know whether I have implemented successfully or not.

Old code before SOLID design principlss:

class Program
{
    static void Main(string[] args)
    {
        Higherclass obj = new Higherclass("Female");
        obj.CallLowerClass();
        Console.ReadLine();
    }

}

public class Higherclass
{
    public readonly string sex;
    lowerClass_Male objLowerClass_Male = null;
    lowerClass_Female objLowerClass_Female = null;

    public Higherclass(string _sex)
    {
        sex = _sex;
    }

    public void CallLowerClass()
    {
        if (sex == "Male")
        {
            objLowerClass_Male = new lowerClass_Male();
            objLowerClass_Male.DoSomeAction();
        }
        else
        {
            objLowerClass_Female = new lowerClass_Female();
            objLowerClass_Female.DoSomeAction();
        }

    }
}

public class lowerClass_Male
{
    public void DoSomeAction()
    {
        Console.WriteLine("Hey It's Male. Do Some Action");
    }
}

public class lowerClass_Female
{
    public void DoSomeAction()
    {
        Console.WriteLine("Hey It's FeMale. Do Some Action");
    }
}

Newly modified code:

class Program
{
    static void Main(string[] args)
    {
        //Now, It depends on my requirement to call any sex class.Let's Say, I want to call Female Class.
        lowerClass_FeMale objLowerfemale = new lowerClass_FeMale();
        Higherclass obj = new Higherclass(objLowerfemale);
        obj.CallLowerClass();
        Console.ReadLine();
    }

}

public interface ISex
{
    void DoSomeAction();
}

public class lowerClass_Male : ISex
{
    public void DoSomeAction()
    {
        Console.WriteLine("Hey It's Male. Do Some Action");
    }
}

public class lowerClass_FeMale : ISex
{
    public void DoSomeAction()
    {
        Console.WriteLine("Hey It's FeMale. Do Some Action");
    }
}

public class Higherclass
{
    public ISex MySexInterface = null;
    public Higherclass(ISex objISex)
    {
        this.MySexInterface = objISex;
    }

    public void CallLowerClass()
    {
        MySexInterface.DoSomeAction();
    }

}

The new code seems OK to me.

One way to know if you're on the right way is trying to unit test your code. In the old code you couldn't unit test the higher class, as you needed to create real instances of the lower classes inside the code, so you needed to test both the higher and the lower classes at the same time.

In the new code you can test the lower classes and the higher class separately, as now you can inject a mock of the ISex interface into the higher class and test that the proper methods of the lower classes are invoked.

Note that you have two options to inject the lower class. If the lower class is only going to help in one method of the higher class, maybe it will be better to inject the lower class as a parameter of that method. If the lower class is going to help all along the higher class, then pass it as a parameter of the constructor, as you have it now.


Handling 8 toggle buttons in a point-of-sale application I am self-taught and trying to improve my programming skills.  My project is a point-of-sale application, with lots of buttons. I am curious how improve this section of code, as I seem to be going against the DRY principle.

I have a Customer object named cust, and each customer has an ID that is set to one of 8 different toggle buttons.

    //update the sales to the customers
    //if cust1 toggle button is selected:
    if (cust1.isSelected()){
        //get the id number from the button
        cust = custDAO.getCustById(Integer.getInteger(cust1.getText()));
        //add the sales to the current value
        int sales = cust.getTotalSales();
        //set the value (note: cust.setTotalSales(cust.getTotalSales() + subtotal) doesn't work
        cust.setTotalSales(sales + subtotal);
        //update the Customer
        custDAO.updateCustomer(cust);
    }else if (cust2.isSelected()){
        cust = custDAO.getCustById(Integer.getInteger(cust2.getText()));
        int sales = cust.getTotalSales();
        cust.setTotalSales(sales + subtotal);
        custDAO.updateCustomer(cust);
    }else if (cust3.isSelected()){
        cust = custDAO.getCustById(Integer.getInteger(cust3.getText()));
        int sales = cust.getTotalSales();
        cust.setTotalSales(sales + subtotal);
        custDAO.updateCustomer(cust);
    }else if(cust4.isSelected()){
        cust = custDAO.getCustById(Integer.getInteger(cust4.getText()));
        int sales = cust.getTotalSales();
        cust.setTotalSales(sales + subtotal);
        custDAO.updateCustomer(cust);
    }else if (cust5.isSelected()){
        cust = custDAO.getCustById(Integer.getInteger(cust5.getText()));
        int sales = cust.getTotalSales();
        cust.setTotalSales(sales + subtotal);
        custDAO.updateCustomer(cust);
    }else if(cust6.isSelected()){
        cust = custDAO.getCustById(Integer.getInteger(cust6.getText()));
        int sales = cust.getTotalSales();
        cust.setTotalSales(sales + subtotal);
        custDAO.updateCustomer(cust);
    }else if(cust7.isSelected()){
        cust = custDAO.getCustById(Integer.getInteger(cust7.getText()));
        int sales = cust.getTotalSales();
        cust.setTotalSales(sales + subtotal);
        custDAO.updateCustomer(cust);
    }else if(cust8.isSelected()){
        cust = custDAO.getCustById(Integer.getInteger(cust8.getText()));
        int sales = cust.getTotalSales();
        cust.setTotalSales(sales + subtotal);
        custDAO.updateCustomer(cust);
    }else{
        //No button selected
        System.out.println("No customer selected");
    }

Short answer: You need the Java Collections framework.

Instead of eight standalone variables cust1...cust8, you should put them into a Collection so that you can easily iterate through them:

// assuming the buttons are of JButton type
Collection<JButton> buttons = Arrays.asList(cust1, cust2, /* ... */, cust8);
for (JButton button : buttons) {
    if (button.isSelected()) {
        cust = custDAO.getCustById(Integer.getInteger(button.getText()));
        int sales = cust.getTotalSales();
        cust.setTotalSales(sales + subtotal);
        custDAO.updateCustomer(cust);
    }
}

Longer answer...

It is not known how these lines are being run, but you can also consider writing event listeners, which is the typical implementation for checking if buttons have selected. For example, to use an ActionListener:

JButton button = new JButton();
// lambda expression below requires Java 8
button.addActionListener(actionEvent -> {
    JButton source = (JButton) actionEvent.getSource();
    // using valueOf() as getInteger() looks at System.getProperty(),
    // unless that is really the intended usage
    cust = custDAO.getCustById(Integer.valueOf(source.getText()));
    int sales = cust.getTotalSales();
    cust.setTotalSales(sales + subtotal);
    custDAO.updateCustomer(cust);
});

Instead of having cust as a method, or even class, variable, you should scope it appropriately to eliminate any possible mis-references. Also, you can consider adding a more expressive method updateTotalSales(int incoming) that returns the object itself, so that you do not have to perform that 'manually' here:

public Customer {

    // ...

    public Customer updateTotalSales(int incoming) {
        setTotalSales(getTotalSales() + incoming);
        return this;
    }
}

JButton button = new JButton();
button.addActionListener(actionEvent -> {
    JButton source = (JButton) actionEvent.getSource();
    Customer cust = custDAO.getCustById(Integer.valueOf(source.getText()));
    custDAO.updateCustomer(cust.updateTotalSales(subtotal));
});

In the unlikely (?) event that you still prefer/require the original iteration method by checking on isSelected(), the Collection-based implementation, coupled with stream-based processing, can also achieve the same outcome with the following, for simplicity:

// buttons from the short answer
buttons.stream().filter(JButton::isSelected).findFirst()
        .map(i -> custDAO.getCustById(Integer.valueOf(i.getText())))
        .ifPresent(cust -> custDAO.updateCustomer(cust.updateTotalSales(subtotal)));

By using a method reference JButton::isSelected, filter for the JButton where that is true.
Use findFirst() to 'get' it in the form of an Optional wrapper.
From the JButton instance, map() it to the desired Customer object, and finally call custDAO to 'consume' it via the ifPresent(Consumer) method.


Java Singleton getter/setter I've created working  setters/getters for an application, that will use a few different classes. This classes will use one class, that will store all data. I know, when I will use a standard constructor of container class, I'll get tons on nulls, due to different instance of container class.

I've created container class within a singleton, that works, but I wanted to ask if anything could be done better, or if this code follow the best practice.

public class Container {

    private String appName = null;
    private String appOwner = null;

    private Container() {
        System.out.println("Start");
    }

    private static class SingletonHolder {
        private final static Container INSTANCE = new Container();
    }

    public static Container getInstance() {
        return SingletonHolder.INSTANCE;
    }

    public String getAppName() {
        return appName;
    }

    public void setAppName(String appName) {
        this.appName = appName;
    }

    public String getAppOwner() {
        return appOwner;
    }

    public void setAppOwner(String appOwner) {
        this.appOwner = appOwner;
    }
}

Sample class, that will use this container:

public class SecondClass {
     Container ctn = Container.getInstance();
}

Right now, when I use in main class:

ctn.setAppOwner(owner);

I get a proper value in any other classes, when I call this:

Container ctn = Container.getInstance();
ctn.getAppOwner();

Is that a good approach?

There are a couple bad things in the code

First of all, this is weird in many ways:

private Container() {
    System.out.println("Start");
}

You print out "Start" to the standard output. This basically restrict any usage of this class to have access to a console. What if it is used in a GUI application? What if this is used in a server application?

Also, "Start" what? Whoever read the console will have no clue what is started?

The object is constructed as a static field, and Java did not guarantee when these static constructor are run (they only guarantee it is run before the class methods is run, so it is basically you have a console print that is shown in non-deterministic time.

This is not great too.

public void setAppName(String appName) {
    this.appName = appName;
}

Whoever got access to the Container object instance can call this method, which means you basically have a global variable that anyone can access. That's a bad idea, because it will makes debugging really difficult.

From these classes, it appears to me that all you needed is to let the app name and app owner initialized once and get access anywhere, you can make that by simply having a Configuration class with a static property.

public class Configuration
{
    private static String appName;
    private static String appOwner;
    public static void Initialize(String appName, String appOwner)
    {
        Configuration.appName = appName;
        Configuration.appOwner = appOwner;
    }
    public static String AppName() { return Configuration.appName; }
    public static String AppOwner() { return Configuration.appOwner; }
}

That would be it, simple and clear.


Validating a Domain or Service Method I'm trying to determine at which level I should validate a method on my class.  In my class method I throw an exception if it ever gets to that point.  In my service layer I do some validation and return a validation object to be used by the controller.  Here is my code.  Does it seem like I am moving in the right direction?  Let me know if you need any additional code.

// Class Method
public void Delete()
{
    if (Status == "UnIndexed")
        Deleted = true;
    else
        throw new InvalidOperationException("Only an UnIndexed thing may be deleted.");
}

// Service Layer
public IValidator<Thing> RemoveThing(int id)
{
    var thing = _repository.Get(id);
    if (thing == null)
        return null;

    var validator = new ThingValidator(thing);
    var isValid = validator.Validate(new StatusSpecification("UnIndexed"));

    if (isValid)
    {
        thing.Delete();
        _repository.Save(thing);
    }
    else
    {
        validator.AddError("Only an UnIndexed thing may be deleted.");
    }

    return validator;
}

// Controller
public JsonResult DeleteThing(int thingId)
{
    var command = _service.RemoveThing(thingId);

    if (command == null)
        return Json(HttpStatusCode.NotFound);

    if (command.IsValid())
    {
        return Json(HttpStatusCode.OK);
    }
    else
    {
        return Json(command.ListErrors());
    }
}

Some things that stuck out to me.  FWIW.

//NOTE: for me, this validator can only describe what went wrong without
//        giving you a specific way to handle each kind of thing that happened.
//        e.g.  NULL (translates to) NotFound
//        e.g.  AddError(<string>) can only be interpreted as "other" errors since they are just descriptions of the error and not the type
public IValidator<Thing> RemoveThing(int id)
{
    var thing = _repository.Get(id);
    // NOTE: this result has to be interpreted by the caller
    if (thing == null)
        return null;

    var validator = new ThingValidator(thing);
    var isValid = validator.Validate(new StatusSpecification("UnIndexed"));

    if (isValid)
    {
        thing.Delete();
        _repository.Save(thing);
    }
    else
    {
        // NOTE: since this message comes from outside of the class, it
        //         it not the nearest source of truth about what went wrong.
        //         i think this message should come from the class itself.
        //          this message is also typed twice in your code.
        validator.AddError("Only an UnIndexed thing may be deleted.");
    }

    return validator;
}

public JsonResult DeleteThing(int thingId)
{
    var command = _service.RemoveThing(thingId);

    // NOTE: this could be set up so that you don't have to interpret null as "not found"
    if (command == null)
        return Json(HttpStatusCode.NotFound);

    if (command.IsValid())
    {
        return Json(HttpStatusCode.OK);
    }
    else
    {
        return Json(command.ListErrors());
    }
}

OPTION #1

short and to the point.
idiomatic solution
very little logic.
explicitly typed errors for try/catch handling

THROW ALL THE EXCEPTIONS!!!

public void Delete()
{
    if (Status == "UnIndexed")
        Deleted = true;
    else
        throw new UnIndexedException();
}

public void RemoveThing(int id)
{
    var thing = _repository.Get(id);
    if (thing == null)
        throw new ThingNotFoundException(id);

    thing.Delete();
    _repository.Save(thing);
}

// Controller
public JsonResult DeleteThing(int thingId)
{
    try
    {
        _service.RemoveThing(thingId);
        return Json(HttpStatusCode.OK);
    }
    catch (ThingNotFoundException e)
    {
        return Json(HttpStatusCode.NotFound);
    }
    catch (Exception e)
    {
        throw;
    }
}

OPTION #2

less short but still to the point.
inching towards a more declarative solution. (which is good)
not that much logic.
explicitly stated errors for if/then handling

NEVER THROW EXCEPTIONS!!!

class RemoveThingResult
{
    private RemoveThingResult(bool isOk, boo isNotFound, bool isUnIndexed)
    {
        IsOk = isOk; IsNotFound = isNotFound; IsUnIndexed = isUnIndexed;
    }

    public readonly bool IsOk;
    public readonly bool IsNotFound;
    public readonly bool IsUnIndexed;

    public static RemoveThingResult Ok()
    {
        return new RemoveThingResult(true, false, false);
    }
    public static RemoveThingResult NotFound()
    {
        return new RemoveThingResult(false, true, false);
    }
    public static RemoveThingResult UnIndexed()
    {
        return new RemoveThingResult(false, false, true);
    }
}

// Service Layer
public RemoveThingResult RemoveThing(int id)
{
    var thing = _repository.Get(id);

    if (thing == null)
        return RemoveThingResult.NoFound();

    var validator = new ThingValidator(thing);
    if (!validator.Validate(new StatusSpecification("UnIndexed")))
        return RemoveThingResult.UnIndexed();

    thing.Delete();
    _repository.Save(thing);

    return RemoveThingResult.Ok();
}

// Controller
public JsonResult DeleteThing(int thingId)
{
    var command = _service.RemoveThing(thingId);

    if (command.IsNotFound)
        return Json(HttpStatusCode.NotFound);

    if (command.IsOk)
        return Json(HttpStatusCode.OK);

    if (command.IsUnIndexed)
        return Json("Only an UnIndexed thing may be deleted.");

}


Using GetHashCode in Equals I have a scenario when I want to have a HashSet<ITrackableObjectData>, where ITrackableObjectData has string TypeName and object Id readonly immutable properties. Id is a primitive value, ie. string, int, long, Guid etc.

I want to use these equality rules:

If the TypeName property is different, should anyway be considered non-equal.
If the Id is zero, null etc, should use the base object hashcode (reference equal).
If Id is not zero, then it should compare the TypeName and Id.

I created the following comparer to provided to the HashSet's constructor, and I'd like to hear from you of any improvements/vandalism in my code. As you can see I'm using the GetHashCode in the Equals method, is this a bad-idea?
Note that these are my equality requirements in my scenario.

class ITrackableObjectDataComparer : EqualityComparer<ITrackableObjectData>
{
  public override bool Equals(ITrackableObjectData x, ITrackableObjectData y) =>
    GetHashCode(x) == GetHashCode(y);

  public override int GetHashCode(ITrackableObjectData obj)
  {
    if (obj == null) return 0;
    var typeHash = obj.TypeName == null ? 0 : obj.TypeName.GetHashCode();
    var idHash = obj.Id == null ? 0 : obj.Id.GetHashCode();
    idHash = idHash == 0 ? obj.GetHashCode() : idHash;
    return typeHash ^ idHash;
  }
}

Note that this EqualityComparer is a private class and is only meant to serve the purpose of this specific HashSet which is not exposed at all.
In fact I don't also really care about the Equals, but rather about the GetHashCode, so I know whether an object with my specs have already been added to a HashSet.

Update:

This is basically the equality checker that fulfills my requirements (but again, I don't need an equality comparer, I need a hash equality-comparer)

public override bool Equals(ITrackableObjectData x, ITrackableObjectData y)
{
  if (x == y) return true; //ref eq.
  else if (x == null)
    return y == null;
  else if (y == null)
    return false;
  else if (x.TypeName != y.TypeName)
    return false;
  else
  {
    if (x.Id == null)
      return y.Id == null;
    else if (y.Id == null)
      return false;
    else
    {
      int xId = x.Id.GetHashCode(),
          yId = y.Id.GetHashCode();
      if (xId == 0 || yId == 0)
        return x.Equals(y); //use default eq.
      else
        return xId == yId;
    }
  }
}

I think you are missing the point. Different objects can produce same hashcode. This can happen:

obj.Equals(otherObj)  // false
obj.GetHashCode() == otherObj.GetHashCode() //true

If for some reason you want your Equals methods to return true when hashcodes are equal, then by all means. Make sure to document this behavior though.

However if you want to compare references , then use ReferenceEquals method instead. Hashcode comparison will not work for that scenario, because hashcode equality does not guarantee that object are (reference-)equal.

P.S. Also keep in mind, that hashcodes should not change, otherwise hash-tables might break. Make sure that Id and TypeName are read-only properties.


Student project that calculates the return on an investment This is a simple student project that calculates the return on an investment with a given investment amount, number of years invested, and annual interest rate.  I know this is pretty basic, but I'm just looking for feedback for formatting and other general improvements I can make to the code.

// Author: Joshua Ferrell
// Date: 3/25/2017

import javafx.application.Application;
import javafx.geometry.Pos;
import javafx.geometry.HPos;
import javafx.scene.Scene;
import javafx.scene.control.Button;
import javafx.scene.control.Label;
import javafx.scene.control.TextField;
import javafx.scene.layout.GridPane;
import javafx.stage.Stage;

// A program that caculates the return on an investment.

public class Exercise15_5 extends Application {

    // globals
    private TextField tfInvestmentAmount = new TextField();
    private TextField tfNumYears = new TextField();
    private TextField tfAnnualInterestRate = new TextField();
    private TextField tfFutureValue = new TextField();
    private Button btCalc = new Button("Calculate");

    @Override
    public void start(Stage primaryStage) {

        // Create UI
        GridPane gridPane = new GridPane();
        gridPane.setHgap(5);
        gridPane.setVgap(5);
        gridPane.add(new Label("Investment Amount:"), 0, 0);
        gridPane.add(tfInvestmentAmount, 1, 0);
        gridPane.add(new Label("Number of Years:"), 0, 1);
        gridPane.add(tfNumYears, 1, 1);
        gridPane.add(new Label("Annual Interest Rate:"), 0, 2);
        gridPane.add(tfAnnualInterestRate, 1, 2);
        gridPane.add(new Label("Future Value:"), 0, 3);
        gridPane.add(tfFutureValue, 1, 3);
        gridPane.add(btCalc, 1, 4);

        // Set Properties for UI
        gridPane.setAlignment(Pos.CENTER);
        tfInvestmentAmount.setAlignment(Pos.BOTTOM_RIGHT);
        tfNumYears.setAlignment(Pos.BOTTOM_RIGHT);
        tfAnnualInterestRate.setAlignment(Pos.BOTTOM_RIGHT);
        tfFutureValue.setAlignment(Pos.BOTTOM_RIGHT);
        tfFutureValue.setEditable(false);
        gridPane.setHalignment(btCalc, HPos.RIGHT);

        // Process event (caculate future value)
        btCalc.setOnAction(e -> calculateFutureValue());

        // Create a scene and place it in the stage
        primaryStage.setScene(new Scene(gridPane, 300, 200));
        primaryStage.setTitle("Exercise15_05"); // Set Title
        primaryStage.show(); // Display the Stage
    }

    private void calculateFutureValue() {

        // Get Values from text fields
        double amount = Double.parseDouble(tfInvestmentAmount.getText());
        double interest = Double.parseDouble(tfAnnualInterestRate.getText());
        int years = Integer.parseInt(tfNumYears.getText());

        // Get Monthly interest rate
        double monthlyInterestRate = interest / 12 / 100;

        // Get Future Value
        double futureValue = amount * Math.pow(1 + monthlyInterestRate, years * 12);

        // set futureValue to tfFutureValue
        tfFutureValue.setText(String.format("$%.2f", futureValue));
    }
}

The first thing that is noticeable when looking at this code is that there are too many comments. Let me explain.

Example 1 - stating the obvious

    // set futureValue to tfFutureValue
    tfFutureValue.setText(String.format("$%.2f", futureValue));

This comment does not add any value to your program, it creates mess. The code itself is self-explanatory, and even if I had 2 weeks experience in Java, I would know what it does.

Example 2 - stating the untrue

public class Exercise15_5 extends Application {

    // globals
    private TextField tfInvestmentAmount = new TextField();
    private TextField tfNumYears = new TextField();
    private TextField tfAnnualInterestRate = new TextField();

The comment that says //globals is simply wrong - the TextFields are private fields of Exercise15_5 class. You can create a "global" variable in Java by creating a public static field - then you can use it everywhere, but of course you don't want to do that in your application

Example 3 - extract code to a method instead of commenting

In your start() method, you have 4 big blocks of code that are doing different things and are commented accordingly, like:

    // Set Properties for UI
    gridPane.setAlignment(Pos.CENTER);
    tfInvestmentAmount.setAlignment(Pos.BOTTOM_RIGHT);
    tfNumYears.setAlignment(Pos.BOTTOM_RIGHT);
    tfAnnualInterestRate.setAlignment(Pos.BOTTOM_RIGHT);
    tfFutureValue.setAlignment(Pos.BOTTOM_RIGHT);
    tfFutureValue.setEditable(false);
    gridPane.setHalignment(btCalc, HPos.RIGHT);

We can extract those lines to a new method and give it some meaningful name... Just like we would comment those lines!

private void setPropertiesForUI() {
    gridPane.setAlignment(Pos.CENTER);
    tfInvestmentAmount.setAlignment(Pos.BOTTOM_RIGHT);
    tfNumYears.setAlignment(Pos.BOTTOM_RIGHT);
    tfAnnualInterestRate.setAlignment(Pos.BOTTOM_RIGHT);
    tfFutureValue.setAlignment(Pos.BOTTOM_RIGHT);
    tfFutureValue.setEditable(false);
    gridPane.setHalignment(btCalc, HPos.RIGHT);
}

Then, your start() method becomes much cleaner:

@Override
public void start(Stage primaryStage) {
    createUI();
    setPropertiesForUI();
    calculateFutureValue();
    createSceneAndPlaceItInTheStage();
}

Split logic and UI into separate classes

I also believe that the calculateFutureValue() method should not be in the Exercise15_5 class. It should be extracted to another class, possibly something named like InvestmentCalculator. The method should take doubles and int as input parameters, not Strings (it doesn't know about textfields and UI).

You can make the calculateFutureValue() static in the InvestmentCalculator class since it's just performing some calculations and returning a value - you don't need an instance of it. You can even make InvestmentCalculators constructor private to prevent creating an instance of it - in other words, making the InvestmentCalculator a Util Class.

I don't want to go into the implementation details since this is tagged as homework, but you can always edit your question or post another one after the refactoring.


Efficient parsing of FASTQ FASTQ is a notoriously bad format. This is because it uses the same @ character for the id line as it does for quality scores. Deciding what is a quality score and what is an id is a tricky endeavor with many pitfalls.

I'd like your opinion of my while 1 approach in the function read_fastq.

This works, but I'd like your tips and ideas on ways of improving it.

def read_fastq(fileH):
    """
    takes a fastq file as input
    yields idSeq, sequence and score
    for each fastq entry
    """

    #initialize the idSeq, sequence, score and index
    idSeq, sequence, score = None, None, None

    """
    main loop structure:

    An outer while loop will run until the file runs out of lines.

    If the line starts with @ and score exists, yield the id, sequence and score.
        this is where the yielding happens in our loop, it could be considered where each
        round of the loop ends

    The first id is recorded on the first iteration.

    If there is no sequence, begin an inner while loop,
        read lines into sequence until we hit a + character, and break out of inner while loop

    if there is no score, begin an inner while loop where we will increment score until we have
         the same number of characters as sequence, this prevents interpretting a header as a score

    """

    while 1:

        line = fileH.readline()

        #break if we hit the end of the file
        if not line:
            break

        if line.startswith('@') and score:

            #before yielding the sequence, remove non-ascii characters
            sequence = legalChar(sequence)

            yield idSeq, sequence, score

            #reset to default values
            sequence = None
            score = None
            idSeq = line.rstrip()

        elif not idSeq:
            #get our first idSeq
            idSeq = line.rstrip()
            continue

        elif not sequence:
            sequence = ""
            while not line.startswith('+'):
                sequence += line.rstrip().replace(' ', '')
                line = fileH.readline()
        elif not score:
            score = []
            #begin collecting our score, only collect as many chars as in our sequence
            while 1:
                score += line.rstrip().replace(' ', '')
                if len(score) >= len(sequence):
                    break
                else:
                    line = fileH.readline()

    #yield our final idSeq, sequence and score
    yield idSeq, sequence, score

1. Bugs

All the bugs in this section are to do with handling of bad input; see §2.1 below for more about this issue.

If there are no sequences in the input, your function wrongly yields an output sequence:

>>> from StringIO import StringIO
>>> list(read_fastq(StringIO('')))
[(None, None, None)]

If the input is wrongly formatted, your program can go into an infinite loop waiting for a line starting with a plus sign:

>>> list(read_fastq(StringIO('@ID\nSEQUENCE\n')))
^C
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "cr32897.py", line 155, in read_fastq
    sequence += line.rstrip().replace(' ', '')
KeyboardInterrupt

If the input is wrongly formatted, your program can go into an infinite loop waiting for the end of the quality data:

>>> list(read_fastq(StringIO('@ID\nSEQUENCE\n+ID\nSCORE')))
^C
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "cr32897.py", line 162, in read_fastq
    if len(score) >= len(sequence):
KeyboardInterrupt

2. Other comments on your code

You don't detect or report problems in the input. You just ignore or suppress them. This leads to outright bugs, as noted in §1 above, but it also means that you don't detect other problems with the input, like these:

missing sequence id;
junk lines before the first sequence or between sequences;
sequence id on + line fails to match sequence id on @ line;
length of quality data fails to match length of sequence data;
quality data contains characters outside the correct range.

You shouldn't ignore or suppress errors in the input, because you need to know about these errors in order to discover problems with your processing pipeline. What if you are being fed the wrong input? What if the input is in the wrong format (for example, FASTA instead of FASTQ)? What if there is a bug in the code that generated the input? If you don't detect errors then you will end up generating bogus output and causing trouble for whatever process is next in your processing pipeline.

When writing code that parses input data, the code for detecting and reporting errors typically constitutes the majority of the code. See §3 below for how I would go about writing this kind of parser.
The first item of each output tuple (the sequence id) starts with the @ sign, but the @ sign is not part of the id.
Your claim, "Deciding what is a quality score and what is an id is a tricky endeavor with many pitfalls" is a bit exaggerated. As far as I can see there is just one pitfall, namely that you might incorrectly stop reading the quality data when you encounter a line starting with @. If there are other pitfalls, you should probably mention them in a comment in the code so that we can check that they are all avoided.
It's not clear to me what the H means in the variable name fileH. Does it stand for "handle"? If that's what you mean, you should write it out for clarity: file_handle. But really there's no reason not just to use the name file here.
Your code contains two docstrings, but only the first is actually available to a user via the help function. The second docstring should be a comment instead — or better still, break it up into small comments that appear next to the relevant bits of code.
The actual comments could be better. There is really no need to write comments like this:

line = fileH.readline()

#break if we hit the end of the file
if not line:
    break

You can expect Python programmers to know that file.readline returns an empty string to indicate end-of-file; or if not, they can find out by running help(file.readline).

Some of the comments are wrong, for example here:

#get our first idSeq
idSeq = line.rstrip()

This only gets the "first" idSeq if there was no previous line starting with an @.
For an infinite loop, you write:

while 1:

but it's conventional to write:

while True:

In Python, a file object is also an iterator that yields the lines in the file, so instead of

line = fileH.readline()

#break if we hit the end of the file
if not line:
    break

you can write:

line = next(fileH)

This raises StopIteration when there are no more lines.
You build up the sequence string by repeated addition:

sequence = ""
while not line.startswith('+'):
    sequence += line.rstrip().replace(' ', '')
    line = fileH.readline()

In Python this is an anti-pattern that results in quadratic space and time consumption. The efficient way to build up a string from components is to put the components in a list and then call str.join:

sequence_lines = []
while not line.startswith('+'):
    sequence_lines.append(line.rstrip().replace(' ', ''))
    line = fileH.readline()
sequence = ''.join(sequence_lines)

You build up the score in the form of a list rather than a string:

score = []
while 1:
    score += line.rstrip().replace(' ', '')
    ...

which means that your results come out like this:

('@ID', 'GATTTGGG', ['!', "'", "'", '*', '(', '(', '(', '('])

Is that really what you want? I would have expected score to come out as a string, like this:

('ID', 'GATTTGGG', "!''*((((")

3. Revised code

Here's some revised code that fixes the problems above, and is much more robust in its detection and reporting of errors in the input.

class Error(Exception):
    pass

class Line(str):
    """A line of text with associated filename and line number."""
    def error(self, message):
        """Return an error relating to this line."""
        return Error("{0}({1}): {2}\n{3}"
                     .format(self.filename, self.lineno, message, self))

class Lines(object):
    """Lines(filename, iterator) wraps 'iterator' so that it yields Line
    objects, with line numbers starting from 1. 'filename' is used in
    error messages.

    """
    def __init__(self, filename, iterator):
        self.filename = filename
        self.lines = enumerate(iterator, start=1)

    def __iter__(self):
        return self

    def __next__(self):
        lineno, s = next(self.lines)
        line = Line(s)
        line.filename = self.filename
        line.lineno = lineno
        return line

    # For compatibility with Python 2.
    next = __next__

def read_fastq(filename, iterator):
    """Read FASTQ data from 'iterator' (which may be a file object or any
    other iterator that yields strings) and generate tuples (sequence
    name, sequence data, quality data). 'filename' is used in error
    messages.

    """
    # This implementation follows the FASTQ specification given here:
    # <http://nar.oxfordjournals.org/content/38/6/1767.full>
    import re
    at_seqname_re = re.compile(r'@(.+)$')
    sequence_re = re.compile(r'[!-*,-~]*$')
    plus_seqname_re = re.compile(r'\+(.*)$')
    quality_re = re.compile(r'[!-~]*$')

    lines = Lines(filename, iterator)
    for line in lines:
        # First line of block is @<seqname>.
        m = at_seqname_re.match(line)
        if not m:
            raise line.error("Expected @<seqname> but found:")
        seqname = m.group(1)
        try:
            # One or more lines of sequence data.
            sequence = []
            for line in lines:
                m = sequence_re.match(line)
                if not m:
                    break
                sequence.append(m.group(0))
            if not sequence:
                raise line.error("Expected <sequence> but found:")

            # The line following the sequence data consists of a plus
            # sign and an optional sequence name (if supplied, it must
            # match the sequence name from the start of the block).
            m = plus_seqname_re.match(line)
            if not m:
                raise line.error("Expected +[<seqname>] but found:")
            if m.group(1) not in ['', seqname]:
                raise line.error("Expected +{} but found:".format(seqname))

            # One or more lines of quality data, containing the same
            # number of characters as the sequence data.
            quality = []
            n = sum(map(len, sequence))
            while n > 0:
                line = next(lines)
                m = quality_re.match(line)
                if not m:
                    raise line.error("Expected <quality> but found:")
                n -= len(m.group(0))
                if n < 0:
                    raise line.error("<quality> is longer than <sequence>:")
                quality.append(m.group(0))

            yield seqname, ''.join(sequence), ''.join(quality)

        except StopIteration:
            raise line.error("End of input before sequence was complete:")


SQL procedure with cursor to check if values in table 1 are also in table 2 The problem is that I'm checking if result from cursor exists in CDN.ZamNag table for every iteration. Is there any better way to check if values in table 1 are also in table 2?

Time of executing below procedure takes 1 minute and 30 seconds - I'm searching for any ideas which will make executing shorter.

IF (SELECT object_id('TempDB..#temp_table')) IS NOT NULL
BEGIN
DROP TABLE #temp_table
END

DECLARE @Seria_Nazwa VARCHAR(15)
DECLARE @Magazyn_id INT
DECLARE @Magazyn_Kod varchar(15)
DECLARE @Zlozono varchar(12)
DECLARE @dzisiaj varchar(30)
SET @dzisiaj = @data --datetime = convert(datetime,DATEADD(DAY, 0, DATEDIFF(DAY, 0, CURRENT_TIMESTAMP)),120)
Declare @jutro datetime = convert(datetime,DATEADD(DAY, 1, @dzisiaj),120)

CREATE TABLE #temp_Table (
seria varchar(15),
magazynkod varchar(15),
zlozono varchar(5),
data varchar(30)
)
DECLARE @get CURSOR
------------------------------------------------------
SET @get = CURSOR FOR
    SELECT Ser_Nazwa,Mag_GidNumer,Mag_Kod
    FROM CDN.Serie, CDN.Magazyny
    WHERE Mag_Kod in('1406','1407','1408')
    and SER_Nazwa like 's%'
    and  SER_Nazwa not like 'sk%'
    and  SER_Nazwa not like 'sa%'
    and  SER_Nazwa not like 'st%'
    and  SER_Nazwa not like 'su%'
    and  SER_Nazwa not like 's14/%'
    and  SER_Nazwa not like 's'
    and  SER_Nazwa not like 's10'
    and  SER_Nazwa not like 's24'
    and  SER_Nazwa not like 's07'
    and  SER_Nazwa not like 's28'
    and  SER_Nazwa not like 's32'
    and  SER_Nazwa not like 's61'

    OPEN @get
    FETCH NEXT
    FROM @get into @Seria_Nazwa,@Magazyn_Id,@Magazyn_Kod
    WHILE @@FETCH_STATUS = 0
    BEGIN
        IF (EXISTS(
        SELECT ZaN_MagNumer,Zan_ZamSeria,Zan_DataRealizacji
        FROM CDN.ZamNag
        WHERE
        Zan_MagNumer = @Magazyn_Id
        and ZaN_ZamSeria = @Seria_Nazwa
        and DateAdd(day, ZaN_DataRealizacji,CONVERT(DATETIME,'1800-12-28',120)) >= @dzisiaj
        AND DateAdd(day, ZaN_DataRealizacji,CONVERT(DATETIME,'1800-12-28',120)) <  @jutro
        ))
    SET @zlozono = 'tak'
    ELSE SET @zlozono = 'nie'
    INSERT INTO #temp_Table VALUES(@Seria_Nazwa,@Magazyn_Kod,@Zlozono,@dzisiaj)
    FETCH NEXT
    FROM @get INTO @Seria_Nazwa,@Magazyn_Id,@Magazyn_Kod
END
CLOSE @get
DEALLOCATE @get

select * from #Temp_Table
order by seria

END

Doing that (checking if values in Table1 are or aren't in Table2) is typically done with a LEFT JOIN. Or a RIGHT JOIN if you want to get creative. Using a CURSOR in T-SQL should be you last resort!
Something like this:
SELECT t1.[columns]
FROM [table1] t1
LEFT JOIN [table2] t2 ON t1.[key] = t2.[key]
WHERE t2.[key] IS NULL

Style
There are a number of accepted standards for writing T-SQL. Some people write the keywords in ALLCAPS, others write them in lowercase - which convention you chose is entirely up to you and, in the end, doesn't matter.
What does matter though, is consistency.

and DateAdd(day, ZaN_DataRealizacji,CONVERT(DATETIME,'1800-12-28',120)) >= @dzisiaj
AND DateAdd(day, ZaN_DataRealizacji,CONVERT(DATETIME,'1800-12-28',120)) <  @jutro

and

select * from #Temp_Table
order by seria

shouldn't be in the same script. Pick a convention, stick to it, and thank yourself later :)
One helpful convention I've adopted, is comma-first. This:

SELECT ZaN_MagNumer,Zan_ZamSeria,Zan_DataRealizacji

Would look like that:
SELECT
     ZaN_MagNumer
    ,Zan_ZamSeria
    ,Zan_DataRealizacji

This allows you to easily comment-out, remove, add or reorder columns while reducing the room for error.


Finding pupils to tutor I am a Math tutor and I found the site https://www.yksityisopetus.fi/ where one can search pupils. I made a Python parser to find pupils that want Math teaching with at least 25 €/hour. Are there any improvements for this code?
import requests
import tkinter as tk

link = "https://www.yksityisopetus.fi/opiskelijat/matematiikka/min-25-per-tunti/uusin-ensin"
f = requests.get(link)
lista = f.text.split('\n')
ilmot = []
for i in range(len(lista)):
    if "<a href=\"/" in lista[i] and "<a href=\"/opettajat" not in lista[i] and "")
    for j in range(len(f)):
        if f[j] == '\n<br /':
            parsittu.append(f[j+1])
s = ""
parsilmot = []
for i in range(len(parsittu)):
    if parsittu[i].endswith('</div'):
        parsilmot.append(parsittu[i][0:len(parsittu[i])-5])

p = list(set(parsilmot))
quote = ""
for i in p:
    quote += i+"\n"

root = tk.Tk()
S = tk.Scrollbar(root)
T = tk.Text(root, height=4, width=100)
S.pack(side=tk.RIGHT, fill=tk.Y)
T.pack(side=tk.LEFT, fill=tk.Y)
S.config(command=T.yview)
T.config(yscrollcommand=S.set)
T.insert(tk.END, quote)
tk.mainloop()

Move your code into functions
Functions are easier to test, and make it easier to modify your code over time.
For example, I think the first block requests information from a server and creates a list of links. I think the second block processes that list of links and generates some other sort of list. I think the third block iterates over the list from the second block and generates yet another list.
If these blocks were in functions, then the main logic would be self explanatory. For example, your code could look like:
...
ilmot = get_students(link)
parsittu = some_other_function(ilmot)
parsilmot = another_function(parsittu)
quote = create_quote(parsilmot)
...

If those functions all had reasonable names, the code would be very easy to understand, and it would be easy to test each function in isolation.
Not only that, but you can more easily move all of that code into a function so that you can implement a "refresh" feature into the UI so that you don't have to restart the script every time you want to update the data.


Pair up elements of a sequence without overlap I have a sequence (in this case actually an array) like this:

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

And want to produce a sequence of pairs like this:

[(1, 2); (3, 4); (5, 6); (7, 8); ...]

The following code works, but I can't help feeling that it's a lot of code. Is there a shorter, more idiomatic or more elegant way to do this.

[| 1; 2; 3; 4; 5; 6; 7; 8; 9; 10|]
|> Seq.pairwise
|> Seq.mapi (fun i t -> (i, t))
|> Seq.filter (fun (i, t) -> i % 2 = 0)
|> Seq.map (fun (i, t) -> t)

Here's another way:

[| 1; 2; 3; 4; 5; 6; 7; 8; 9; 10|]
|> Seq.chunkBySize 2
|> Seq.choose (function [|x; y|] -> Some (x, y) | _ -> None)

So, chunkBySize will create a sequence like seq [ [|1; 2|]; [|3; 4|]; .. ]

Then choose will try to match each element of the sequence with an array of two elements, everything else will be filtered out, which accounts for a trailing array of a single element if the input length is not even.

It could be even shorter by changing the last line to:

|> Seq.collect Array.pairwise

Which would map pairwise to the each element, transforming for instance [|1;2|] to [|(1;2)|] and it will return [||] when there are less than two elements. So this will return a sequence of an array but since I'm using collect it will concatenate all elements. Since single elements were transformed to empty arrays it will concatenate nothing in those cases, if you think about it this is similar to the mechanics of choose.


Resizing a discrete uniform CSRNG distribution I have a requirement to generate a uniform distribution of cryptographically secure random numbers. To generate these numbers I only know of the RNGCryptoServiceProvider class, that has the method GetBytes(byte[] data). This is good, but I have to then change this discrete uniform distribution ranging from 0 - 256 to a different discrete uniform distribution ranging from 0 - limit.

And so I thought of three ways to do this. The first two ways were using % and /, which don't work if limit is not a divisor of 256. As this leads to a non-uniform distribution. However filtering all numbers that a not less than the limit works and gives a uniform distribution, however has terrible performance problems when limit is a small number.

I'm currently using:

private static IEnumerable<object> Cycle(object value=null)
{
    while (true)
    {
        yield return value;
    }
}

private static IEnumerable<byte> RandomBytes(int chunkSize)
{
    return Cycle().SelectMany(unused =>
    {
        var b = new byte[chunkSize];
        new RNGCryptoServiceProvider().GetBytes(b);
        return b;
    });
}

private static IEnumerable<int> RandomFilter(int chunkSize, int limit)
{
    return RandomBytes(chunkSize).Where(n => n < limit).Select(n => (int)n);
}

Since I tried using % and /, below is the code that I used to check if there is a uniform distribution, and those two solutions.

private static IEnumerable<int> RandomModulo(int chunkSize, int limit)
{
    return RandomBytes(chunkSize).Select(n => n % limit);
}

private static IEnumerable<int> RandomResize(int chunkSize, int limit)
{
    var denom = 256.0/limit;
    return RandomBytes(chunkSize).Select(n => (int)(n / denom));
}

private static void TestDistribution(int limit, int amount, int chunkSize, Func<int, int, IEnumerable<int>> fn)
{
    var counter = new Dictionary<int, int>();
    foreach (var i in Enumerable.Range(0, limit))
    {
        counter[i] = 0;
    }

    foreach (var i in fn(chunkSize, limit).Take(amount))
    {
        counter[i] += 1;
    }
    Console.WriteLine($"{limit} {amount} {chunkSize} {counter.Select(kv => kv.Value).Aggregate(0, (acc, x) => acc + x)}");
    Console.WriteLine(string.Join(", ", from kv in counter select $"{kv.Key}"));
    Console.WriteLine(string.Join(", ", from kv in counter select $"{kv.Value}"));
    Console.WriteLine();

}

private static void Main()
{
    TestDistribution(45, 100000000, 1000, RandomFilter);
    TestDistribution(32, 100000000, 1000, RandomFilter);
    Console.WriteLine();

    TestDistribution(45, 100000000, 1000, RandomModulo);
    TestDistribution(32, 100000000, 1000, RandomModulo);
    Console.WriteLine();

    TestDistribution(45, 100000000, 1000, RandomResize);
    TestDistribution(32, 100000000, 1000, RandomResize);
    Console.ReadLine();
}

There are things you can do to improve the efficiency of your filter solution with small values as the limit.

More efficient filtering

The most obvious one is actually given in the example of the function on the MSDN network - which is basically to extend the filter range to the highest multiple of limit, less than 256. I.e. if the limit is 20, then filter up to 240, and then the modulo is still uniformly distributed:

Copied from that page:

private static bool IsFairRoll(byte roll, byte numSides)
{
    // There are MaxValue / numSides full sets of numbers that can come up
    // in a single byte.  For instance, if we have a 6 sided die, there are
    // 42 full sets of 1-6 that come up.  The 43rd set is incomplete.
    int fullSetsOfValues = Byte.MaxValue / numSides;

    // If the roll is within this range of fair values, then we let it continue.
    // In the 6 sided die case, a roll between 0 and 251 is allowed.  (We use
    // < rather than <= since the = portion allows through an extra 0 value).
    // 252 through 255 would provide an extra 0, 1, 2, 3 so they are not fair
    // to use.
    return roll < numSides * fullSetsOfValues;
}

Statistically insignificant errors

You have another alternative too, which is to reduce any modulo bias to a statistically insignificant amount. You do this by using significantly bigger random numbers (bigger than 256). Combining multiple bytes for each output byte would accomplish this, for example, using 4 bytes. Using more bytes would further reduce any modulo bias:

/* Convert 4 bytes to a long */
private static IEnumerable<long> RandomLongs()
{
    var csrng = new RNGCryptoServiceProvider();
    return Cycle().Select(unused =>
    {
        var bytes = new byte[4];
        csrng.GetBytes(bytes);
        return  (long)(BitConverter.ToUInt32( bytes, 0));
    });
}

private static IEnumerable<int> RandomExtended(int chunkSize, int limit)
{
    var source = RandomBytes(chunkSize);
    foreach (long lng in RandomLongs())
    {
        var nxt = lng % limit;
        yield return (int)nxt;
    }

}

Using 4 bytes of source for your modulo (with max limit 256) will reduce the bias error to about 1 in \$2^{24}\$ ... which would be hard to measure.

This version is also shown here on ideone: https://ideone.com/maiQ70

Removed

On the other hand, there's a better way, that still guarantees uniform distribution, but relies on knowing what the previous value was. Conceptually, this solution relies on having uniform "gaps" between results, and so you can add a uniform random gap to the previous value, and then do the modulo on that.

I implemented it as:

private static IEnumerable<int> RandomWrap(int chunkSize, int limit)
{
    int prev = 0;
    foreach (byte b in RandomBytes(chunkSize))
    {
        prev = (prev + b) % limit;
        yield return prev;
    }

}

Note that it takes the byte-value as a "gap", adds it to the previous result, and then "wraps" it around in the limit range.

You can see the results here in ideone: https://ideone.com/BEbwsA


Solving a race condition In my application, I have a message dispatcher. Each message gets relayed to a dispatcher thread.

In some scenarios, I can get two responses from the third party in the same millisecond:

An "I received your message" ACKnowledgement
"My response to your message" RESPONSE

When I get both messages at the same time, this can create a race condition. It is important to have my EventHandler handle the ACK before I get the RESPONSE, but I cannot guarantee ordering from the Asynchronous networking IO library; I must do it myself.

Below is a working MVCE of this scenario. Most of it is just framework, it's not how it works in the real code, but it's enough to get the bit I'm asking about - the ordering logic - to show it's value. The Proposed Solution to this race condition is the ensureAck method, which is called in the handleResponse method. If you comment out that call, and run the application repeatedly, you will see that there is no guarantee of method ordering. Additionally, and this is the difficult bit: I don't want the RESPONSE to be ignored if, for some reason, the ACK never shows up (a dropped network packet or something) - I want the RESPONSE to get handled, reluctantly, eventually.

Aside: the bit about NOTHING messages are just to warm up the ExecutorService so you can actually see the race condition.

How is my solution?

import java.util.concurrent.ExecutorService;
import java.util.concurrent.Executors;

public class RaceSolution {
  private static enum State {
    NEW, READY;
  }

  private static enum Type {
    ACK, RESPONSE, NOTHING;
  }

  private static class EventHandler {
    private volatile State state = State.NEW;
    private final Object lock = new Object();

    public void handleEvent(Event event) {
      switch(event.t) {
      case ACK:
        handleAck(event);
        break;
      case RESPONSE:
        handleResponse(event);
        break;
       default:
      }
    }

    private void handleResponse(Event event) {
      // COMMENT OUT THE NEXT LINE TO SEE THE RACE CONDITION
      ensureAck();
      synchronized(lock) {
        System.out.println("Received response event: " + state);
      }
    }

    private void handleAck(Event event) {
      synchronized(lock) {
        System.out.println("Received ack Event: " + state);
        state = State.READY;
      }
    }

    /* THIS IS THE IMPORTANT BIT */
    private void ensureAck() {
      int tries = 0;
      try {
          while(state == State.NEW && tries < 10) {
              Thread.sleep(1);
              tries++;
          }
      } catch (InterruptedException e) {

      }
    }
  }

  private static class Event {
    Type t;

    public Event(Type t) {
      this.t = t;
    }
  }

  public static void main(String... args) throws InterruptedException {
    RaceSolution solution = new RaceSolution();
    Event nothing = new Event(Type.NOTHING);
    Event ack = new Event(Type.ACK);
    Event response = new Event(Type.RESPONSE);

    for(int i = 0; i < 10; i++) {
      solution.submit(nothing);
    }

    Thread.sleep(100);

    solution.submit(ack);
    solution.submit(response);
    solution.shutdown();
  }

  private final ExecutorService dispatcher = Executors.newCachedThreadPool();
  private final EventHandler handler = new EventHandler();

  public void shutdown() {
    if (!dispatcher.isShutdown()) {
      dispatcher.shutdownNow();
    }
  }

  public void submit(final Event e) {
    dispatcher.execute(new Runnable() {
      @Override
      public void run() {
        handler.handleEvent(e);
      }
    });
  }
}

Concurrency Strategy

In general, using volatile is complicated, partially because the meaning of volatile changed in Java 1.4, and also because it is hard to spot. I recommend against using it at all. Instead, you should use a more visible concurrency mechanism like synchronization, java.util.concurrent.* classes, java.util.concurrent.locks.*, or java.util.concurrent.atomics.*

So, while I recommend against volatile, what's a real problem is using multiple different locking schemes in the same code, and you use both volatile and synchronized.

The use of synchronization as your concurrency strategy should be fine all on its own in this case.

General

The private final Object lock = new Object() is great. Good to be final, and I prefer that to locking on the instance itself (like synchronized(this)). Makes it more clear.
Println in a synchronized block is almost never a good idea:

synchronized(lock) {
  System.out.println("Received ack Event: " + state);
  state = State.READY;
}

A better solution would be:

State copy = null;
synchronized(lock) {
  copy = state;
  state = State.READY;
}
System.out.println("Received ack Event: " + copy);

Timed sleeps while waiting for a lock are a horrible solution:

Thread.sleep(1);

Better algorithm

The right way to solve this problem, though is to used timed-waits on the lock:

private void handleAck(Event event) {
    State copy = null;
    synchronized(lock) {
      copy = state;
      state = State.READY;
      // tell any waiting threads the state has changed.
      lock.notifyAll();
    }
    System.out.println("Received ack Event: " + copy);
}

/* THIS IS THE IMPORTANT BIT */
private void ensureAck() {
  // how long to wait... 10 milliseconds
  try {
      synchronized(lock) {
          if (state == State.NEW) {
              long waited = 0; // milliseconds
              long start = System.currentTimeMillis();
              while (state == State.NEW && waited < 10) {
                  lock.wait(10 - waited);
                  waited = System.currentTimeMillis() - start;
              }
          }
      }
  } catch (InterruptedException e) {
      // do more than nothing.....
  }
}

The above strategy does not need anything to be volatile, and it does not 'poll' the state, it returns immediately when the state changes....


Reduce nested for loops and if statements using Iterator Blocks? Is it possible to reduce this long nested for loops and if conditions to increase its readability and to optimize it for future reference.

While writing code for my scheduling app, I ended up with a method as shown below. Really, I have a data structure like this. Here, I checks - Is there any stages (Inside LCycle) which using same Tool at same time and If its found so, another method LCycleTimeShift is called to make a rearrangment.

But I want to check whether the new arrangement is adaptable and the for loop counter is reset so that it will check the new arrangment again. I think this is not the better way to write the code for  better readabilty. A little research on the topic found that Enumerators can help me in this. But I don't know how can I accomplish this with the following code.

public List<LCycle> ToolArrangment(List<LCycle> TimeLineInit)
{
    for (int i = 0; i < TimeLineInit.Count; i++)//Each LIfeCycles In TimeLine
    {
        for (int j = 0; j < TimeLineInit[i].Stage.Count; j++)//Each Stages inTimeLine
        {
            for (int k = 0; k < i; k++)//Each L esvd =ifeCycles Upto Current LifeCycle
            {
                for (int l = 0; l < TimeLineInit[k].Stage.Count; l++)//Each Stages of (LifeCycle upto current LifeCycle)
                {
                    for (int m = 0; m < TimeLineInit[i].Stage[j].ToolList.Count; m++)//each tools in stage of timelkine
                    {
                        for (int n = 0; n < TimeLineInit[k].Stage[l].ToolList.Count;n++ )// Each tools In that stage (for loop outer of outer)
                        {
                            if (TimeLineInit[i].Stage[j].ToolList[m].ToolName == TimeLineInit[k].Stage[l].ToolList[n].ToolName)//If both tools are same (satidfying above for loop conditions)
                            {
                                if (IsTimeOverLaps(TimeLineInit[i].Stage[j].StageSpan, TimeLineInit[k].Stage[l].StageSpan))
                                {//tool using at same time.
                                    Stage ReplaceStage = TimeLineInit[i].Stage[j].DeepCopy();//Taking Copy of stage Span to make time shift
                                    Double TimeDifference=(ReplaceStage.StageSpan.ToTime-ReplaceStage.StageSpan.FromTime).TotalMinutes;//Calculating required time shift
                                    ReplaceStage.StageSpan.FromTime=TimeLineInit[k].Stage[l].StageSpan.ToTime;//FromTime changed accordingly
                                    ReplaceStage.StageSpan.ToTime=ReplaceStage.StageSpan.ToTime.AddMinutes(TimeDifference);//To Time Changed accordingly
                                    LCycleTimeShift(TimeLineInit[i], ReplaceStage);//passing refernce
                                    j = 0; k = 0; l = 0; m = 0; n = 0;//Counter Reset to validate the new arrangment
                                }
                            }
                        }
                    }
                }
            }
        }
    }

    return TimeLineInit;
}

First, having this many nested loops indicates that your algorithm could have really bad time complexity. And with the way you're resetting the indexes, I'm not even sure your code is guaranteed to complete. There may be a more efficient algorithm to do what you want, but I'm not going to try to find that.

Second, the i, j, k naming convention for loop variables is okay if you have two or maybe three nested loops. But with six levels, it makes your code much less readable (and more error-prone). You should consider using locals with reasonable names instead of expressions like TimeLineInit[i].Stage[j].ToolList[m]. Or even better, foreach loops, which basically do the same with less code.

Third, I think the code you're using for resetting is wrong. For example, when i = 0 and you reset the loop variables, you end up running the innermost loop with i = 0 and k = 0, which should never happen (since the invariant is k < i).

Fourth, I think that a method that takes something as a parameter, modifies it and then returns it is a code smell. One exception would be a fluent interface, but that doesn't seem to be your case, since the method is not an extension method.

Fifth, you should use normal .Net naming conventions (e.g. camelCase for local variables, instead of PascalCase).

Sixth, you should do all your time calculations in TimeSpans, instead of doubles representing minutes, because it's simpler.

Seventh, I think you could make your code more readable using LINQ.

The whole code would look something like:

public void ToolArrangment(List<LCycle> timeLineInit)
{
    foreach (var lifeCycle1 in timeLineInit)
    {
        bool repeat;
        do
        {
            repeat = false;

            var stages = (from stage1 in lifeCycle1.Stage
                          from lifeCycle2 in timeLineInit.TakeWhile(lc2 => lc2 != lifeCycle1)
                          from stage2 in lifeCycle2.Stage
                          from tool1 in stage1.ToolList
                          from tool2 in stage2.ToolList
                          where tool1.ToolName == tool2.ToolName
                          where IsTimeOverLaps(stage1.StageSpan, stage2.StageSpan)
                          select new { stage1, stage2 })
                .FirstOrDefault();

            if (stages != null)
            {
                var replaceStage = stages.stage1.DeepCopy();
                var timeDifference = replaceStage.StageSpan.ToTime - replaceStage.StageSpan.FromTime;
                replaceStage.StageSpan.FromTime = stages.stage2.StageSpan.ToTime;
                replaceStage.StageSpan.ToTime = replaceStage.StageSpan.ToTime + timeDifference;
                LCycleTimeShift(lifeCycle1, replaceStage);
                repeat = true;
            }
        } while (repeat);
    }
}

I gave it some thought and I think a much more efficient algorithm to do the same is not that difficult. You would have a hash table indexed by ToolName and for each tool, you would keep a list of all known stages that use it. You would walk though all tools in all stages in all timelines and add them to the list of stages for that tool. If you couldn't do that because of an overlap, you would move the stage, just like you do right now (if I understand your code correctly).

I haven't though out all the details, but I think something like this should work and be much faster than your solution.


Loading screen animation I am trying to make a loading screen but my code isn't DRY.  How could I make it DRY? I have thought about using CSS3 keyframes but I might have problems cross-platform.

$(document).ready(function() {

var lc = $('.loaderContainer'),
    wh = $('.welcomeH'),
    th = $('.toH'),
    meeuh = $('.meeuH'),
    se = $('.siteEnter'),
    sc = $('.seContainer');

lc.delay(3500).fadeOut('slow');
wh.delay(3800).fadeIn('slow').delay(2000).fadeOut('slow');
th.delay(6800).fadeIn('slow').delay(2000).fadeOut('slow');
meeuh.delay(9800).fadeIn('slow');

sc.delay(11800).queue(function(){
    sc.css('left', '-120%');
    sc.dequeue();
});

se.delay(12000).queue(function(){
    se.css('margin-left', '-120%');
    se.dequeue();
});
});

I have tried to use callbacks, but I don't know if I have done it correctly. I have only done callbacks to the first block of code, not the second two; I'm still stuck on it.

lc.delay(3500).fadeOut(400, function() {
    wh.delay(800).fadeIn(400, function() {
        $(this).delay(800).fadeOut(400, function() {
            th.delay(800).fadeIn(400, function() {
                $(this).delay(800).fadeOut(400, function() {
                    meeuh.delay(800).fadeIn(400)
                });
            });
        });
    });
});

sc.delay(12000).queue(function(){
    sc.css('left', '-120%');
    sc.dequeue();
});

se.delay(12300).queue(function(){
    se.css('margin-left', '-120%');
    se.dequeue();
});

Using jQuery you can't make it more DRY than it already is.
I tried encapsulating your code in a function with a foreach statement but it only got uglier and generally less readable.

Using callbacks is a good practice but in this case it made your code scary and reduced the readability.

Unfortunately moving css3 animations is not supported on IE9 & IE8 and I'm assuming that from question that this is an issue.

Below are two implementations of the fade in and fade out functionality with animation-delay for reference.

The first implementation is by using jQuery.

$(document).ready(function() {
  var a = $('#A'),
    b = $('#B');

  // The reason I'm adding the animation delay in javascript and not setting it
  // in css is to demonstrate the control over when the animation starts.
  a.css('animation-delay', '3.5s').addClass('fadeOut');
  b.css('animation-delay', '3.8s').addClass('fadeIn');
});
#A,
#B {
  width: 500px;
  height: 500px;
  position: absolute;
  top: 0;
  left: 0;
}
#A {
  background: red;
}
#B {
  background: blue;
  opacity: 0;
}
.fadeIn {
  animation: fadein 2s;
}
.fadeOut {
  animation: fadeout 2s;
}
@keyframes fadein {
  from {
    opacity: 0;
  }
  to {
    opacity: 1;
  }
}
@keyframes fadeout {
  from {
    opacity: 1;
  }
  to {
    opacity: 0;
  }
}

The second implementation is without jQuery (Vanilla JS)

  document.addEventListener('DOMContentLoaded', function() {
    var a = document.getElementById('A'),
      b = document.getElementById('B');
    a.style.setProperty('animation-delay', '3.5s');
    a.classList.add('fadeOut');
    b.style.setProperty('animation-delay', '3.8s');
    b.classList.add('fadeIn');
  });
#A,

#B {

  width: 500px;

  height: 500px;

  position: absolute;

  top: 0;

  left: 0;

}

#A {

  background: red;

}

#B {

  background: blue;

  opacity: 0;

}
.fadeIn {
  animation: fadein 2s;
}
.fadeOut {
  animation: fadeout 2s;
}
@keyframes fadein {
  from {
    opacity: 0;
  }
  to {
    opacity: 1;
  }
}
@keyframes fadeout {
  from {
    opacity: 1;
  }
  to {
    opacity: 0;
  }
}


Decrease Image downloading time with HttpURLConnection This is piece of code is using to download images. Can somebody tell me how to optimize this code to decrease download time for each image?

        URL url;
        HttpURLConnection connection = null;
        InputStream input = null;
        System.setProperty("http.keepAlive", "true");
        try {
            url = new URL(urlString);
            connection = (HttpURLConnection) url.openConnection();
            connection.setConnectTimeout(HTTP_CONNECTION_TIMEOUT);
            connection.setRequestProperty("Connection", "Keep-Alive");
            input = connection.getInputStream();
            return BitmapFactory.decodeStream(input);
        } catch (MalformedURLException e) {
            e.printStackTrace();
            return null;
        } catch (IOException e) {
            e.printStackTrace();
            return null;
        } finally {
            currentRequestInProgress.remove(urlString);
            if (connection != null)
                connection.disconnect();
            if(input != null){
                try {
                    input.close();
                } catch (IOException e) {
                    e.printStackTrace();
                }
            }

        }

Buffered streams usually result in greatly improved performance:

input = new BufferedInputStream(connection.getInputStream());

The buffer size is 8kb per default, tuning it may be an option (but not without measuring).

And, if you download multiple images: parallelize it with multiple threads (most of the time is consumed by I/O waits anyway), this usually improves performance by factors.


Project Euler Problem 31 (Coin Sum) I just finished Project Euler 31: Coin sums, which asks how many ways there are to make £2 using British coins (1p, 2p, 5p, 10p, 20p, 50p, £1, and £2).

When I compared my code and the problem review's algorithms, I found that my code was faster than theirs. Both of them use dynamic programming, but for some unknown reasons, my code meets the recursive limits.

Project Euler  Reference

def problem_31_dynamic_programming(money, coin_index):
    count = 0
    if coin_index <= 0:
        return 1
    m = money
    if memoiz_list[m][coin_index] > 0:
        return memoiz_list[m][coin_index]
    while money >= 0:
        count += problem_31_dynamic_programming(money, coin_index - 1)
        money -= coin_list[coin_index]
    memoiz_list[m][coin_index] = count
    return count

My solution

import time

def problem_31(money, coin_index): #My solution (cannot solve big number such as 10000)
    if coin_index < 0:
        return 0
    if coin_index == 0 or money == 0:
        return 1
    if memoiz_list[money][coin_index] > 0:
        return memoiz_list[money][coin_index]
    if coin_list[coin_index] > money:
        return problem_31(money, coin_index - 1)
    memoiz_list[money][coin_index] = problem_31(money, coin_index-1)+ \
                                     problem_31(money-coin_list[coin_index],coin_index)
    return memoiz_list[money][coin_index]

start = time.time()
coin_list = [1,2,5,10,20,50,100,200]
memoiz_list = [[0,0,0,0,0,0,0,0] for i in range(201)]
print(problem_31_dynamic_programming(200,7)) #Replace problem_31_dynamic_programming() with problem_31
elapsed = time.time() - start
print("Result found in %f seconds"%(elapsed))

Depth analysis

A bit of tweaking of your code leads to results about the maximum function call depth:

def problem_31_a(money, coin_index, depth=1):
    global glob_depth
    glob_depth = max(glob_depth, depth)
    count = 0
    if coin_index <= 0:
        return 1
    m = money
    if memoiz_list[m][coin_index] > 0:
        return memoiz_list[m][coin_index]
    while money >= 0:
        count += problem_31_a(money, coin_index - 1, depth=depth+1)
        money -= coin_list[coin_index]
    memoiz_list[m][coin_index] = count
    return count

def problem_31_b(money, coin_index, depth=1):
    global glob_depth
    glob_depth = max(glob_depth, depth)
    if coin_index < 0:
        return 0
    if coin_index == 0 or money == 0:
        return 1
    if memoiz_list[money][coin_index] > 0:
        return memoiz_list[money][coin_index]
    if coin_list[coin_index] > money:
        return problem_31_b(money, coin_index - 1, depth=depth+1)
    memoiz_list[money][coin_index] = problem_31_b(money, coin_index-1, depth=depth+1)+ \
                                     problem_31_b(money-coin_list[coin_index],coin_index, depth=depth+1)
    return memoiz_list[money][coin_index]

coin_list = [1,2,5,10,20,50,100,200]

for func in [problem_31_a, problem_31_b]:
    glob_depth = 0
    start = time.time()
    memoiz_list = [[0,0,0,0,0,0,0,0] for i in range(201)]
    print(func(200,7))
    elapsed = time.time() - start
    print("Result found in %f seconds - depth:%d" % (elapsed, glob_depth))

And indeed, we get:

73682
Result found in 0.003184 seconds - depth:8
73682
Result found in 0.000919 seconds - depth:107

Your code appears to be faster but also goes much deeper in the function calls. If you exceed the system limits, you could update sys.setrecursionlimit. However, it could be a good idea to try to fix your code.

You could write a solution that doesn't perform any recursive calls: instead of trying to solve your problems by solving smaller and smaller problems and saving the solution as you go, you could simply update all the problems from the smallest to the biggest you need.

For instance, you could write:

def problem_31_c(money, unused):
    nb_ways = [1] + [0] * money
    for c in coin_list:
        for v in range(money + 1 - c):
            nb_ways[v + c] += nb_ways[v]
    return nb_ways[-1]

Actual code review

For both functions, it could be a good idea to make it obvious that we have the following pattern:

if value_from_memoiz_list:
    return value_from_memoiz_list
compute_value
store_value_in_memoiz_list
return value

In problem_31 for instance, we can see that a situation leads to a result being computed and returned without being store in the memoized list. Also, that case could be handled with less duplicated logic:

count = problem_31_b(money, coin_index-1)
coin_value = coin_list[coin_index]
if coin_value <= money:
    count += problem_31_b(money-coin_value,coin_index)

Finally, your strategy reusing computed value assumes that 0 is a non-existing result. You could use None for this. In your case it doesn't make a difference because no expensive computation leads to 0 but it makes the intent of your code clearer.

def problem_31_a(money, coin_index):
    if coin_index <= 0:
        return 1
    money_rem = money
    memo_value = memoiz_list[money][coin_index]
    if memo_value is not None:
        return memo_value
    count = 0
    while money_rem >= 0:
        count += problem_31_a(money_rem, coin_index - 1)
        money_rem -= coin_list[coin_index]
    memoiz_list[money][coin_index] = count
    return count

def problem_31_b(money, coin_index):
    if coin_index < 0:
        return 0
    if coin_index == 0 or money == 0:
        return 1
    memo_value = memoiz_list[money][coin_index]
    if memo_value is not None:
        return memo_value
    count = problem_31_b(money, coin_index-1)
    coin_value = coin_list[coin_index]
    if coin_value <= money:
        count += problem_31_b(money-coin_value,coin_index)
    memoiz_list[money][coin_index] = count
    return count

Also, you could use a decorator to implement your memoization strategy.

Reusing a generic decorator from https://wiki.python.org/moin/PythonDecoratorLibrary#Memoize , you could write:

coin_list = [1,2,5,10,20,50,100,200]

class memoized(object):
   '''Decorator. Caches a function's return value each time it is called.
   If called later with the same arguments, the cached value is returned
   (not reevaluated).
   '''
   def __init__(self, func):
      self.func = func
      self.memoiz_list = [[None]*len(coin_list) for i in range(201)]
   def __call__(self, money, coin_index):
      try:
          memo_value = self.memoiz_list[money][coin_index]
          if memo_value is not None:
              return memo_value
      except IndexError:
          pass
      ret = self.func(money, coin_index)
      try:
          self.memoiz_list[money][coin_index] = ret
      except IndexError:
          pass
      return ret
   def __repr__(self):
      '''Return the function's docstring.'''
      return self.func.__doc__
   def __get__(self, obj, objtype):
      '''Support instance methods.'''
      return functools.partial(self.__call__, obj)

@memoized
def problem_31_a(money, coin_index):
    if coin_index <= 0:
        return 1
    money_rem = money
    count = 0
    while money_rem >= 0:
        count += problem_31_a(money_rem, coin_index - 1)
        money_rem -= coin_list[coin_index]
    return count

@memoized
def problem_31_b(money, coin_index):
    if coin_index < 0:
        return 0
    if coin_index == 0 or money == 0:
        return 1
    count = problem_31_b(money, coin_index-1)
    coin_value = coin_list[coin_index]
    if coin_value <= money:
        count += problem_31_b(money-coin_value,coin_index)
    return count


Simplifying multiple click events into something more extendable I have the following code which changes the text in a certain element on click depending on the text value present in the element at the time the event is fired.

http://jsfiddle.net/TNDhL/

$('#left').on('click', function (){
if ($("#textContainer:contains('something')").length) {
    $('#textContainer').text('third text replacement');
    $('.elsewhere').text('more here');
}
else if ($("#textContainer:contains('third text replacement')").length) {
    $('#textContainer').text('now the next item');
    $('.elsewhere').text('something new here');
}
else if ($("#textContainer:contains('now the next item')").length) {
    $('#textContainer').text('new text here');
    $('.elsewhere').text('something else here');
}
else if ($("#textContainer:contains('new text here')").length) {
    $('#textContainer').text('something');
    $('.elsewhere').text('text here');
}
});

$('#right').on('click', function (){
if ($("#textContainer:contains('something')").length) {
    $('#textContainer').text('new text here');
    $('.elsewhere').text('something else here');
}
else if ($("#textContainer:contains('new text here')").length) {
    $('#textContainer').text('now the next item');
    $('.elsewhere').text('something new here');
}
else if ($("#textContainer:contains('now the next item')").length) {
    $('#textContainer').text('third text replacement');
    $('.elsewhere').text('more here');
}
else if ($("#textContainer:contains('third text replacement')").length) {
    $('#textContainer').text('something');
    $('.elsewhere').text('text here');
}
});

Please see fiddle above for working version.

In this instance clicking on 'Left' or 'Right' would change the text above.

Is there a better way to handle this case? Main goal is to allow for easier extension of these simple functions into something more complex that involves several similar click events. Something less error prone and easier to maintain/grow. Any suggestions?

The easier (imho) solution would be to store what you want in an nested object containing the matches :

var selectors = {
  left: {
    'something': ['third text replacement', 'more here'],
    'third text replacement': ['now the next item', 'something new here'],
  }
};

var $textContainer = $('#textContainer')
  , $elsewhere = $('.elsewhere');
for (var sel in selectors) {
  void function(sel, matches) {
    $('#'+sel).click(function() {
      for (var i = 0; i < matches.length; ++i) {
        // change "this.value" if it's not an input
        if (this.value == matches[i]) {
          // we found a match, let's update the value
          $textContainer.text(matches[i][0]);
          $elseWhere.text(matches[i][1]);
          // and return early
          break;
        }
      }
    });
  }(sel, selectors[sel]);
}