Counting Islands In Matrix Of Boolean Code objective: Read a txt file, parse it into a matrix and count (and also print) the number of unique "islands" in the map formed by the matrix.

0 = Sea;
1 = Land;
Islands are orthogonal groups of land.

testfile.txt :

5 9
00000
00010
10000
01101
10000
00100
01001
10010
01101

Python code:

# Reads the file and creates the matrix map.
with open('testfile.txt') as f:
    w = int(f.read(1))
    f.read(1)
    h = int(f.read(1))
    f.read(1)
    map = [[2 for x in range(w)] for y in range(h)]
    for y in range(h):
        for x in range(w):
            map[y][x] = int(f.read(1))
        f.read(1)

# Swipes the matrix map after NEW land chunks.
def swipe():
    counter = 0
    for x in range(h):
        for y in range(w):
            if map[x][y] == 1:
                counter += 1
                landInSight(map, x, y, 99)
    print(counter)

# Recursive function to hide any land attached to a chunk already swiped.
def landInSight(m, h, w, c):
    if m[h][w] == 1:
        m[h][w] = c
        if w < len(m[0]) - 1: landInSight(m, h, w + 1, c)
        if h < len(m) - 1: landInSight(m, h + 1, w, c)
        if w > 0: landInSight(m, h, w - 1, c)
        if h > 0: landInSight(m, h - 1, w, c)

# Calls the swipe function.
swipe()

It is a very simple code, but I took way too long coding it. This is my first "program" using Python and although it works, it seems too rough. I am looking for any constructive inputs from Python people.

First of all, your solution for this task looks pretty elegant, from my point of view. I would just provide minor comments to optimize this code.

Your input data is a boolean matrix, so instead of keeping every value as a byte you can use just one bit. For that purpose you can use bitarray module.

Another point, that you are loading whole matrix to the memory. It can be too expensive in certain circumstances. I can suggest one stream-based solution which will calculate a number of islands in dynamic manner. This method was discussed here.


Find all multiples of a number in given range in reverse order I have a problem set which requires me to print all the multiples of a number in given range in reverse order.

So far I have tried this but not sure it is optimized or not!

#import <objc/objc.h>
#import <objc/Object.h>

@implementation TestObj

int main()
{
    int limit = 0;
    int divisor = 5;
    printf("enter a number : ");
    scanf("%d",&limit);

    printf("enter divisor : ");
    scanf("%d",&divisor);

    printf("Multiples of %d in reverse order till limit %d are : ",divisor, limit);
    for(int i = limit/divisor; i > 0 ; i--){
        printf("%d ",i*divisor);
    }

    return 0;
}

@end

Multiplication is not a very cheap operation.
Subtraction is much cheaper.
It will be a better and cleaner algorithm to start from the highest multiple,
and then keep subtracting until reaching 0:

int truncatedCount = limit / divisor;
int highestMultiple = truncatedCount * divisor;

for (int multiple = highestMultiple; multiple > 0; multiple -= divisor) {
    printf("%d ", multiple);
}

The improvements:

multiplication replaced by subtractions
more meaningful variable names (instead of i, for example)
more conventional spacing (around operators and around (...) and { ... }


Split string by character in Java I have this code, which goes through a string and split it using a char delimiter.

I am concerned where it can be improved.

Main concerns are speed and memory usage with speed as the priority.
Any other improvements are welcome too.
Is there a better way (Without regex based split and without 3rd party libraries).

What it does:

  Iterate through string and extract all portions of strings delimited by given char
  return all portions, including empty ones.
  StringUtility class name is used because in future I can add other utility methods when necessory

Code:

public class StringUtility {

    /**
     * Split a string using a single char delimiter
     * @param strToSplit string to use
     * @param delimiter delimiter
     * @return String[] of portions
     */
    public static String[] split(String strToSplit, char delimiter) {
        ArrayList<String> arr = new ArrayList<String>();
        StringBuilder sb = new StringBuilder();
        for (int i = 0; i < strToSplit.length(); i++) {
            char at = strToSplit.charAt(i);
            if (at == delimiter) {
                arr.add(sb.toString());
                sb = new StringBuilder();
            } else {
                sb.append(at);
            }
        }
        arr.add(sb.toString());
        return arr.toArray(new String[0]);
    }
}

Unit Test:

This code currently passes following jUnit Test Cases

public class StringUtilityTest {

    @Test
    public void test1() {
        final String str1 = "Because";
        final String str2 = "I'm";
        final String str3 = "Batman";
        final char delim = ' ';
        String[] parts = StringUtility.split(str1 + delim + str2 + delim + str3,
                delim);
        Assert.assertTrue(parts.length == 3);
        Assert.assertTrue(parts[0].equals(str1));
        Assert.assertTrue(parts[1].equals(str2));
        Assert.assertTrue(parts[2].equals(str3));
    }

    @Test
    public void test2() {
        final String str1 = "";
        final String str2 = "I'm";
        final String str3 = "Batman";
        final char delim = ' ';
        String[] parts = StringUtility.split(str1 + delim + str2 + delim + str3,
                delim);
        Assert.assertTrue(parts.length == 3);
        Assert.assertTrue(parts[0].equals(str1));
        Assert.assertTrue(parts[1].equals(str2));
        Assert.assertTrue(parts[2].equals(str3));
    }

    @Test
    public void test3() {
        final String str1 = "";
        final char delim = ' ';
        String[] parts = StringUtility.split(str1,
                delim);
        Assert.assertTrue(parts.length == 1);
        Assert.assertTrue(parts[0].equals(str1));
    }
}

Speed Test:

From fastest to slowest

janos/Heslacher : split using index of and subString
Emanuele Paolini : split using subString
janos : split by setting string builder to zero length
me : original

Sloppiness

There are several careless mistakes that are easy to fix.

Don't use implementation types in declaration, unless you really need to use a specific feature of that implementation:

ArrayList<String> arr = new ArrayList<String>();

Use the interface type instead:

List<String> arr = new ArrayList<String>();

Instead of creating a new StringBuilder, it might be faster to reset it, like this:

sb.setLength(0);

See this discussion for more details.

Don't convert a List to an array like this:

return arr.toArray(new String[0]);

It's recommended to pass a correctly sized array:

return arr.toArray(new String[arr.size()]);

Better unit tests

Give your test cases meaningful names. So that when your IDE reports some of them failing, it will be easier to understand what failed. For example:

@Test
public void testNormalSentence() {
    final String str1 = "Because";
    final String str2 = "I'm";
    final String str3 = "Batman";
    final char delim = ' ';
    String[] parts = StringUtility.split(str1 + delim + str2 + delim + str3, delim);
    Assert.assertEquals(Arrays.asList(str1, str2, str3), Arrays.asList(parts));
}

@Test
public void testStartingWithDelim() {
    final String str1 = "";
    final String str2 = "I'm";
    final String str3 = "Batman";
    final char delim = ' ';
    String[] parts = StringUtility.split(str1 + delim + str2 + delim + str3, delim);
    Assert.assertEquals(Arrays.asList(str1, str2, str3), Arrays.asList(parts));
}

@Test
public void testEmptyString() {
    final String str1 = "";
    final char delim = ' ';
    String[] parts = StringUtility.split(str1, delim);
    Assert.assertEquals(Arrays.asList(str1), Arrays.asList(parts));
}

These new names describe well exactly what they are testing. This should help understanding the code.

The assertions can be simplified, instead of:

String[] parts = StringUtility.split(str1 + delim + str2 + delim + str3, delim);
Assert.assertTrue(parts.length == 3);
Assert.assertTrue(parts[0].equals(str1));
Assert.assertTrue(parts[1].equals(str2));
Assert.assertTrue(parts[2].equals(str3));

You could simplify like this:

String[] parts = StringUtility.split(str1 + delim + str2 + delim + str3, delim);
Assert.assertEquals(Arrays.asList(str1, str2, str3), Arrays.asList(parts));

This is exactly the same, but shorter, and a lot easier to write.

I would add some more test cases:

@Test
public void testOnlyDelim() {
    final String str1 = "";
    final char delim = ' ';
    String[] parts = StringUtility.split("" + delim + delim, delim);
    Assert.assertEquals(Arrays.asList(str1, str1, str1), Arrays.asList(parts));
}

@Test
public void testNotContainingDelim() {
    final String str1 = "hello";
    final char delim = 'x';
    String[] parts = StringUtility.split(str1, delim);
    Assert.assertEquals(Arrays.asList(str1), Arrays.asList(parts));
}

Suggested implementation

@heslacher's implementation is very close to my taste. Here's a variation of that, with some minor changes:

public static String[] split(String strToSplit, char delimiter) {
    List<String> arr = new ArrayList<>();
    int foundPosition;
    int startIndex = 0;
    while ((foundPosition = strToSplit.indexOf(delimiter, startIndex)) > -1) {
        arr.add(strToSplit.substring(startIndex, foundPosition));
        startIndex = foundPosition + 1;
    }
    arr.add(strToSplit.substring(startIndex));
    return arr.toArray(new String[arr.size()]);
}


Polymorphic pointers-to-functions whose parameters are void pointers as arguments in C My question is about this sort of table that's simplified here. Often, I find myself putting this element, (int (*)(const void *, const void *))&strcmp for bsearch. I go between prototyping with void pointers, creating dummy functions, and casting, but none is really a satisfactory, especially when the functions are complex.

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

static void number(int *const);
static void letter(char *const);

static void (*const action[])(void *const) = {
    (void (*)(void *))&number,
    (void (*)(void *))&letter
};

/* corresponds to the 'action' index */
enum Type { NUMBER = 0, LETTER = 1 };

struct Thing {
    enum Type type;
    void *data;
};

int main(void) {
    const char aah  = 'a';
    const int  five = 5;
    const int  six  = 6;
    const struct Thing things[] = {
        { LETTER, (void *)&aah },
        { NUMBER, (void *)&five },
        { NUMBER, (void *)&six }
    };
    const int things_size = sizeof things / sizeof(struct Thing);
    const struct Thing *t;

    /* prints a 5 6 */
    for(t = things; t < things + things_size; t++) {
        action[t->type](t->data);
    }

    return EXIT_SUCCESS;
}

static void number(int *const dp) {
    printf("%d\n", *dp);
}

static void letter(char *const cp) {
    printf("%c\n", *cp);
}

This https://stackoverflow.com/questions/559581/casting-a-function-pointer-to-another-type/559671#559671 seems to suggest that it's dubious to do this. What is the cleanest, most correct way to achieve this sort of polymorphism?

This doesn't really answer your question, but when using function pointers it is usually easier to use a typedef:

typedef void (*Action) (void * const);
static Action action[] = { (Action) number, (Action) letter };

Also the calling of the action would be better expressed as

struct Thing {
    Action action;
    void *data;
};
...
const struct Thing things[] = {
    { (Action) letter, (void *)&aah },
    { (Action) number, (void *)&five },
    { (Action) number, (void *)&six }
};
for(t = things; t < things + things_size; t++) {
    t->action(t->data);
}


Evaluate joint probability density function of a Markov random field The following code evaluates probability mass function for all possible states of a model.

def PDF(size):
    b = np.random.randn(size)
    J = np.random.randn(size, size)

    density_func = np.zeros(2**size)
    states = dec2bin(size)

    for i in range(2**size):
        density_func[i] = np.exp((np.dot(b, states[i,:]) + np.dot(np.dot(states[i],J),states[i])))

    Z = np.sum(density_func)
    density_func = density_func / Z

    return density_func

utility functions

def bitfield(n,size):
    x = [int(x) for x in  bin(n) [ 2 :]]
    x = [0] * (size - len(x)) + x
    return x

def dec2bin(size):
    states = []
    for i in range(2**size):
        binary = bitfield(i, size)
        states.append(binary)
    return np.array(states)

The model is grid graph Markov random field. Each node of the graph can have two states {0, 1}, so the total number of possible states of the model is 2total number of nodes. Each instance of the for loop is calculating the probability of that state. At end I want to calculate the joint probability distribution of all the nodes. The joint probability distribution of this model is as follows

$$
p(\mathbf{x}) = \tfrac{1}{z} exp(\mathbf{b}\cdot\mathbf{x} + \mathbf{x}\cdot\mathbf{J}\cdot\mathbf{x})
$$

\$b\$ and \$J\$ are model parameters and \$x\$ is a vector of present states of the system.

\$Z\$ is the normalizing constant which is calculated by summing up 'values' of each possible state of the system to convert them into probabilities.

When the variable size is greater than 25 the code takes long time to execute. Is there a way to vectorize this code and speed it up?

A first go at the problem:

Generating the states

Since you are generating all possible bit patterns with a given number of bits, it's wasteful to first create a binary representation as a string, and then convert it back to integers digit by digit. As often itertools can help you here. Especially itertools.product. Your use case is basically one of the examples given in its documentation.

from itertools import product

def generate_all_possible_states(size):
    states = np.empty((2**size, size))
    for i, value in enumerate(product((0, 1), repeat=size)):
        states[i, :] = value
    return states

The reimplementation also avoids the dynamically growing list and allocates a numpy array of appropriate size beforehand.

Moving repeated computations

You are calculating exp for every single scalar that you put in density_func. That is wasteful since it does not allow numpy to play its strengths, namely applying the same operation to a lot of values aka vectorization. Fortunately, there is an easy way out (ot the loop ;-)) and you can just compute np.exp on the whole density_func vector.

The first part of the sum can also be easily moved out of the loop.

density_func = np.empty(2**size)
for i in range(2**size):
    density_func[i] = np.dot(np.dot(states[i], J), states[i])

density_func += np.dot(states, b)
density_func = np.exp(density_func)
# maybe a teeny-tiny bit faster:
# density_func = np.exp(density_func + np.dot(states, b))

The order of operation matters here. Intializing density_func to np.dot(states, b) and then adding the results in the loop like density_func[i]  += np.dot(np.dot(states[i], J), states[i]) is consistently slower.

Preliminary timing

As an example, for size = 18, your original implementation takes about \$5.5s\$ here on my machine. With just the two modifications above, you can get down to \$1.9s\$ (both with the same fixed seed for the random numbers).

size = 20: \$23.4s\$ vs. \$8.0s\$
size = 21: \$47.9s\$ vs. \$16.4s\$

Maybe I will have time to revisit this later on this week to update the answer with a proper analysis on how both of them scale for larger sizes. My strong intuition at this point would be that they both have a complexity of \$\mathcal{O}(2^n)\$ and the difference between them will actually remain a constant factor (\$\approx 3\times\$ faster).

Things that didn't work (so far)

Things that I tried, that didn't work for me:

Eliminating the magic of np.dot and explicitely using np.inner or np.matmul / @ (in Python 3). np.dot seems to have some tricks up its sleeves and is still faster. Maybe I will have to read the doc more carefully.
Trying to compute the second part of the sum in a vectorized manner. I'm a little bit too tired at the moment to come up with a clever solution to this.


Merging key-value sets as specified by a command private void merge(ArrayList<String> operations, ArrayList<LinkedHashSet<String>> setOfStrings) {
        String toMerge = operations.get(1);
        String fromMerge = operations.get(2);
        boolean enteredFirstToMerge = false;
        boolean enteredFirstForMerge = false;
        // references that points one on toMerge and the other on fromMerge
        LinkedHashSet<String> toMergeAndAfterDelete = null;
        LinkedHashSet<String> addOnFromMerge = null;
        for (LinkedHashSet<String> subSet : setOfStrings) {
        if (subSet.contains(toMerge) && subSet.contains(fromMerge))
            break;
        else {
            if (subSet.contains(toMerge) && !enteredFirstToMerge) {
            toMergeAndAfterDelete = subSet;
            enteredFirstToMerge = true;
            }
            if (subSet.contains(fromMerge) && !enteredFirstForMerge) {
            addOnFromMerge = subSet;
            enteredFirstForMerge = true;
            }
            if ((enteredFirstForMerge && enteredFirstToMerge)) {
            break;
            }
          }
        }
        /***********************************************/
        //outside Loop i call the remove on the arraylist
        //that are very expensive
        /*************************************************/
        if (enteredFirstForMerge && enteredFirstToMerge) {
        // first i delete from the array the linkedHashSet toMerge and
        // fromMerge and after i add a
        // new linkedHashSet with the subSets merged
        setOfStrings.remove(toMergeAndAfterDelete);
        setOfStrings.remove(addToMergeOnFromMerge);
        addOnFromMerge.addAll(toMergeAndAfterDelete);
        setOfStrings.add(addOnFromMerge);
        }
}

This function takes as parameter an arrayList of operations, and an ArrayList<LinkedHashSet<String>>:

For the arrayList of operations, I always get a specific position, so I don't iterate.    It is always \$O(1)\$.

For example, if I have as operation move foo bar, I have to do these steps:

First of all, I have to find where are located foo and bar:

Inside setOfStrings, I can have this situation:

    position x : {bar tree hotel}
    ...
    position y : {foo lemon coffee}

When I find them, I have to  concat the foo string into bar string  in this way:

            position x : {bar tree hotel foo lemon coffee}
            ...
            position y : {}

How can I improve the efficiency of this function?

Abstract as much as possible over type: List instead of ArrayList.
Use OO where appropriate: List<String> operations should clearly be some
class MoveOperation(String to, String from).
It's not clear why you are using LinkedHashSet instead of List.
The algorithm is ill-defined: what happens if there are numerous
foo's or bar's.

I implemented something below, but it is in functional style, so it might be hard to read.  There are three implicit loops: two for the filters and one for the map.  You might also notice that I never modify a collection, but always create a new one.  Again, this is the functional style.  In your algorithm, you can actually shoot yourself in the foot if you modify some collections at the wrong moment.

private List<List<String>> merge2(List<String> operations, List<List<String>> listOfSentences) {
    Stream<List<String>> sentencesWithTo = listOfSentences.stream()
                                           .filter(sentence -> sentence.contains(toMerge));
    Stream<List<String>> sentencesWithFrom = listOfSentences.stream()
                                             .filter(sentence -> sentence.contains(fromMerge));

    long nTo = sentencesWithTo.count();
    long nFrom = sentencesWithFrom.count();

    if (nTo == 0  || nFrom == 0) {
        return listOfSentences;
    } else if (nTo > 1 || nFrom > 1) {
        // what to do here??
    } else {
        List<String> sentenceTo = sentencesWithTo.collect(Collectors.toList()).get(0); // should only be one element ?
        List<String> sentenceFrom = sentencesWithFrom.collect(Collectors.toList()).get(0); // should only be one element ?
        Stream<List<String>> updatedSentences = listOfSentences.stream().map(sentence -> {
            if (sentence.equals(sentenceTo))
                return Stream.concat(sentenceTo.stream(), sentenceFrom.stream()).collect(Collectors.toList());
            else if (sentence.equals(sentenceFrom))
                return new ArrayList<>();
            else
                return sentence;
        });
        return updatedSentences.collect(Collectors.toList());
    }
}


Pandas replace rare values in a pipeline A common preprocessing in machine learning consists in replacing rare values in the data by a label stating "rare". So that subsequent learning algorithms will not try to generalize a value with few occurences.
Pipelines enable to describe a sequence of preprocessing and learning algorithms to end up with a single object that takes raw data, treats it, and output a prediction. scikit-learn expects the steps to have a specific syntax (fit / transform or fit / predict). I wrote the following class to take care of this task so that it can be run inside a pipeline. (More details about the motivation can be found here: pandas replace rare values)
Is there a way to improve this code in term of performance or reusability ?
class RemoveScarceValuesFeatureEngineer:

    def __init__(self, min_occurences):
        self._min_occurences = min_occurences
        self._column_value_counts = {}

    def fit(self, X, y):
        for column in X.columns:
            self._column_value_counts[column] = X[column].value_counts()
        return self

    def transform(self, X):
        for column in X.columns:
            X.loc[self._column_value_counts[column][X[column]].values
                  < self._min_occurences, column] = "RARE_VALUE"

        return X

    def fit_transform(self, X, y):
        self.fit(X, y)
        return self.transform(X)

And the following can be appended to the above class to make sure the methods work as expected:
if __name__ == "__main__":
    import pandas as pd

    sample_train = pd.DataFrame(
        [{"a": 1, "s": "a"}, {"a": 1, "s": "a"}, {"a": 1, "s": "b"}])
    rssfe = RemoveScarceValuesFeatureEngineer(2)
    print(sample_train)
    print(rssfe.fit_transform(sample_train, None))
    print(20*"=")

    sample_test = pd.DataFrame([{"a": 1, "s": "a"}, {"a": 1, "s": "b"}])
    print(sample_test)
    print(rssfe.transform(sample_test))

You could use scikit-learn's TransformerMixin which provides an implementation of fit_transform for you (its implementation is available here for interest).

I'd consider renaming RemoveScarceValuesFeatureEngineer to something that fits a bit more with other classes in scikit-learn. How about RareValueTransformer instead?

What do you want to happen if an unseen value is transformed? Take, for example

sample_test = pd.DataFrame([{"a": 1, "s": "a"}, {"a": 2, "s": "b"}])
print(sample_test)
print(rssfe.transform(sample_test))

This raises a KeyError, which isn't what I expected. I'd either rework your code to ignore unseen values, or return a nicer error if this is what you want to happen. To me, ignoring seems more reasonable, but it's up to you! Making some unit tests would give you more confidence in cases like this, too.
A pedantic aside: you have a typo of 'occurrence' in min_occurences, which is easily amended.


Graph implementation using FIFO lists I've made a little implementation of an undirected graph, using FIFO lists, for one of my homeworks. Never really knew how to write a proper FIFO data structure, so I would be glad to see some opinion of yours.

#include <iostream>
#include <fstream>

using namespace std;

class Graph
{
    class Node
    {
        struct adj
        {
            int data;
            adj* next;
        };

        adj* head;
        adj* tail;
        int num;
    public:
        Node();
        Node(const Node&);
        Node& operator=(const Node&);
        bool is_empty();
        bool is_leaf();
        void push(int);
        int pop();
        void print();
        ~Node();
    };

    int num_nodes;
    Node* nodes;
public:
    Graph(int);
    Graph(ifstream&);
    Graph(const Graph&);
    void add_edge(int, int);
    Node& operator[](int);
    void print();
    ~Graph();
};

Graph::Node::Node()
{
    head=NULL;
    tail=NULL;
    num=0;
}

Graph::Node::Node(const Node& F)
{
    head=NULL;
    tail=NULL;
    num=0;

    adj* tmp=F.head;

    while(tmp)
    {
        push(tmp->data);
        tmp=tmp->next;
    }
}

Graph::Node& Graph::Node::operator=(const Node& F)
{
    if(this!=&F)
    {
        head=NULL;
        tail=NULL;
        num=0;

        adj* tmp=F.head;

        while(tmp)
        {
            push(tmp->data);
            tmp=tmp->next;
        }
    }

    return *this;
}

bool Graph::Node::is_empty()
{
    return head==NULL;
}

bool Graph::Node::is_leaf()
{
    return num==1;
}

void Graph::Node::push(int value)
{
    adj* tmp=new adj;

    tmp->data=value;
    tmp->next=NULL;

    if(is_empty())
    {
        head=tmp;
    }
    else
    {
        tail->next=tmp;
    }

    tail=tmp;

    num++;
}

int Graph::Node::pop()
{
    if(!is_empty())
    {
        adj* tmp=head;

        head=head->next;

        int value=tmp->data;

        delete tmp;

        num--;

        return value;
    }
    else
    {
        return -1;
    }
}

void Graph::Node::print()
{
    adj*tmp=head;

    while(tmp)
    {
        cout<<tmp->data<<" ";
        tmp=tmp->next;
    }
    cout<<endl;
}

Graph::Node::~Node()
{
    while(!is_empty())
    {
        pop();
    }
}

Graph::Graph(int num_nodes)
{
    this->num_nodes=num_nodes;
    nodes=new Node[num_nodes+1];
}

Graph::Graph(ifstream& f)
{
    f>>num_nodes;

    nodes=new Node[num_nodes+1];

    int start, end;

    while(f>>start>>end)
    {
        add_edge(start, end);
    }
}

Graph::Graph(const Graph& G)
{
    num_nodes=G.num_nodes;

    nodes=new Node[num_nodes+1];

    for(int i=1; i<=num_nodes; i++)
    {
        nodes[i]=G.nodes[i];
    }
}

void Graph::add_edge(int start, int end)
{
    nodes[start].push(end);
    if(start!=end)
    {
        nodes[end].push(start);
    }
}

Graph::Node& Graph::operator[](int i)
{
    return nodes[i];
}

void Graph::print()
{
    for(int i=1; i<=num_nodes; i++)
    {
        nodes[i].print();
    }

    cout<<endl;
}

Graph::~Graph()
{
    delete[] nodes;
}

int main()
{
    ifstream f("graph.txt");

    Graph G(f);

    G.print();

    return 0;
}

Add spaces to improve readability:

head = NULL;
while (tmp)
if (this != &F)

Member functions that do not change any of the class members should be declared with the const specifier:

bool is_empty() const;
bool is_leaf() const;

Some of your while loops could be rewritten as for loops.  For example, the loop in the Node copy constructor could be rewritten as

for (adj* tmp = F.head; tmp != NULL; tmp = tmp->next)
    push(tmp->data);

Should a pop with an empty list be tolerated?  Currently you silently ignore the problem by returning a -1.  Would throwing an exception (to report the error) be better?  Or at least a std::assert(false); before the return, so you can detect the problem in a debug build.  Also, you don't need the else there, since the body of the if will return.

You have no error checking.  In particular, add_edge will happily try to add an edge to a node that doesn't exist, resulting in Undefined Behavior and (if you're lucky) a crash.  (The same applies to Graph::operator[].  You could add an assert check, or an at method that would check the bounds, similar to how std::vector operates.)

Your Graph copy constructor does not copy nodes[0].

Why are you using raw memory for Graph's nodes?  You should use std::vector or some form of smart pointer.


Associate an object with a regular expression in Python Is there a way I can link/associate an object (in this particular case a function) with a regular expression pattern?

Here's the function I'm working on:

def get_command(url):
    """
    Return command for downloading a particular url.

    >>> get_command('http://randomsite.com/XYZ')
    ['wget', '--content-disposition', '--continue', '--quiet', 'http://randomsite.com/XYZ']

    >>> get_command('http://examplesite.com/XYZ')
    ['/usr/bin/torify', 'wget', '--content-disposition', '--continue', '--quiet', '--referer=http://referersite.com', 'http://examplesite.com/XYZ']

    >>> get_command('https://mega.nz/%23!xd432Afds')
    ['megadl', '--no-progress', 'https://mega.nz/#!xd432Afds']

    >>> get_command('https://mega.nz/#!s2JHj1fds')
    ['megadl', '--no-progress', 'https://mega.nz/#!s2JHj1fds']

    >>> get_command('http://othersite.com/XYZ')
    ['downloadtool', 'http://othersite.com/XYZ']
    """
    import re

    base = ['wget', '--content-disposition', '--continue', '--quiet']

    # examplesite.com via torified wget and special referer
    if re.match(r'(https?://)?examplesite\.com/.+$', url):
        return ['/usr/bin/torify'] + base + \
            ['--referer=http://referersite.com', url]
    # MEGA via megadl of megatools
    elif re.match(r'https://mega\.nz/.+$', url):
        # fix url if necessary
        return ['megadl', '--no-progress', url.replace('%23', '#', 1)]
    # othersite.com via a dedicated download tool
    elif re.match(r'(https?://)?othersite\.com/.+$', url):
        return ['downloadtool', url]
    # default wget command
    else:
        return base + [url]

I think the aforementioned code is pretty straightforward, but it got me thinking whether there's a good way to refactor this. As the if-chain of re.match(pattern, url) gets longer, it starts to look more and more necessary.

Perhaps ideally I'd have a dictionary of patterns with their associated functions. Anyways here's what I ended up with:

def get_command(url):
    """Return command for downloading a particular url."""
    import re

    base = ['wget', '--content-disposition', '--continue', '--quiet']

    commands = [
        # examplesite.com via torified wget and special referer
        (r'(https?://)?examplesite\.com/.+$',
         lambda u: ['/usr/bin/torify'] + base +
         ['--referer=http://referersite.com', u]),
        # MEGA via megadl of megatools (fix url if necessary)
        (r'https://mega\.nz/.+$',
         lambda u: ['megadl', '--no-progress', u.replace('%23', '#', 1)]),
        # othersite.com via a dedicated download tool
        (r'(https?://)?othersite\.com/.+$',
         lambda u: ['downloadtool', u])
    ]

    return next(
        (f for p, f in commands if re.match(p, url)), lambda u: base + [u]
    )(url)

Doesn't seem like much of an improvement, potentially the opposite. Anyone have a good way to refactor this?

You should be able to see that you do lambda u: data + [u] on all of the commands.
The exception to this is when you do u.replace('%23', '#', 1),
this can be replaced by urllib.unquote and will allow for more urls.

You should put base, commands and re into global scope.
If you also change the commands so that you don't use lambda's and just have the lists then you can get:

try:
    #Python2
    from urllib import unquote
except ImportError:
    #Python3
    from urllib.parse import unquote
import re

BASE = ['wget', '--content-disposition', '--continue', '--quiet']
COMMANDS = [
    (
        r'(https?://)?examplesite\.com/.+$',
        ['/usr/bin/torify'] + BASE + ['--referer=http://referersite.com']
    ),
    (
        r'https://mega\.nz/.+$',
        ['megadl', '--no-progress']
    ),
    (
        r'(https?://)?othersite\.com/.+$',
        ['downloadtool']
    )
]

def get_command(url):
    """Return command for downloading a particular url."""
    url = unquote(url)
    commands = next((f for p, f in COMMANDS if re.match(p, url)), BASE)
    return commands + [url]


Thread Safe Evicting Queue in Java Just wrote an Evicting Queue implementation, as an ordinary Linked List working under the hood, in Java; which, I hope, I've made thread safe, could you review?
Code:
package com.threadsafedatastructs.EvictingQueue;

import java.util.ArrayDeque;
import java.util.LinkedList;
import java.util.List;
import java.util.NoSuchElementException;

public class EvictingQueue<T> {
private final int MAX_SIZE;
private List<T> data = new LinkedList<>();
private volatile T o;
private volatile ArrayDeque<T> dequeO;

public EvictingQueue(int _maxSize) {
    this.MAX_SIZE = _maxSize;
}

public synchronized boolean isEmpty() {
    return data.isEmpty();
}

public synchronized int size() {
    return data.size();
}

public T head() {    /* Peek the first element */
    return this.data.get(0);
}

public T tail() {    /* Peek the last element */
    return this.data.get(this.data.size() - 1);
}

public synchronized T popFirst() {
    if (!this.data.isEmpty()) {
        o = data.remove(0);
        notifyAll();
        return o;
    }
    throw new NoSuchElementException();
}

public synchronized T popLast() {
    if (!this.data.isEmpty()) {
        o = data.remove(data.size() - 1);
        notifyAll();
        return o;
    }
    throw new NoSuchElementException();
}

public synchronized boolean push(T item) {
    if (this.data.size() <= MAX_SIZE) {
        if (this.data.size() == MAX_SIZE) {
            this.popFirst();
            this.data.add(0, item);
            notifyAll();
            return true;
        }
        this.data.add(0, item);
        notifyAll();
        return true;
    }
    throw new IndexOutOfBoundsException();
}

public synchronized boolean append(T item) {
    if (this.data.size() <= MAX_SIZE) {
        if (this.data.size() == MAX_SIZE) {
            this.popFirst();
            boolean obj = this.data.add(item);
            notifyAll();
            return obj;
        }
        boolean obj = this.data.add(item);
        notifyAll();
        return obj;
    }
    throw new IndexOutOfBoundsException();
}

public synchronized ArrayDeque<T> pollAll() {
    dequeO = new ArrayDeque<>(this.data);
    this.data.clear();
    notifyAll();
    return dequeO;
}

@Override
public String toString() {
    return data.toString();
}

public int getMAX_SIZE() {
    return MAX_SIZE;
}

}
I'm interested in that volatile variables, are they necessary? Or maybe I should add more? notifyAll() do those synchronized methods need this call?
Also, this: if (this.data.size() <= MAX_SIZE) condition seems redundant, because list data shouldn't be longer than 100 anyway; but this is the simplest way for me to limit Linked List length in Java (also, may help with tests).

those volatile variables: not only do they seem redundant, they actually breach thread safety.  volatile variables are used when you actually want the variable to be accessed by multiple threads (for example, when you want one thread to set a boolean and then have another read that variable).  as far as I could detect, the two variables can be replaced by local ones.
those notifyAll() calls: they are mostly redundant. the method is only useful if a client thread has called wait().  that is usually the case if the queue is empty and a client wants to block until a new item has ben added. if the list is not empty, the client usually pops an item and goes about processing it. so you need to understand when clients will want to wait for an event to happen and get notified about it.  Secondly, calling notifyAll() only makes sense if you release the lock immediately after the notification.  calling notifyAll() in a loop without releasing the lock will not do anything.
as an exercise, this solution will work. However, it is not efficient. synchronizing all the methods mean that any access to the queue will prevent all other access. for example, if one thread calls isEmpty() it has locked the entire queue. you may think that isEmpty() is such a small method, but what if the thread just entered the wethod and got a context switch from the OS. it can wait for 2-3 milliseconds before releasing the lock (and that is a long time in the computation universe).  so in order to be more efficient, you need better granularity on the scope of the locking.  For example, set a boolean if the queue is in the process of being modified and only at these times lock the reading methods.  and you can get more efficient with more sophisticated (and complex) mechanisms (like locking only part of the queue this is being modified etc)
ALL_CAPITAL names in Java (and other languages), are used for constant variables, not for those that hold state. (i.e. recevied from arguments). even if they are immutable.

EDIT

Regarding the question in the comment:

  What happen when isEmpty and size don't be synchronized? Do we
  care?

well, it can be that one thread will ask isEmpty at the same time that another is pushing an item into the queue. that is why I believe that in the body of the reading methods you need to create a synchronized block that is dependant on a boolean that is set when a writing method is called.

Another advice I can give you is to have a look at java.util.concurrent package.  it contains several lock mechanisms that are more powerful and flexible than synchronized blocks.  For example, ReentrantLock has the following text in its documentation:

  A reentrant mutual exclusion Lock with the same basic behavior and
  semantics as the implicit monitor lock accessed using synchronized
  methods and statements, but with extended capabilities.

there is a useful example in this topic it shows how you can use a ReentrantLock instance across methods, something that may be useful in your case.

lastly, have a look at java.util.concurrent.ConcurrentLinkedQueue which seems to fit at least some of your requirements. if you are doind this asan exercise, then take a look at the class' source code and see hoe they dealt with thread safety issues.


Calculating e^x by math.h and by own means For this program, the user needs to enter an exponent and the program will calculate \$e\$ (Euler's number) to the power of the exponent the user inputs.

This is done by two ways:

By the math.h library
By a for loop (Series)

Then the program calculates the difference of these two functions (which we now call \$d\$) and calculates \$n\$.

\$n = \frac{d}{0,001}\$

#include <math.h>
#include <stdio.h>

main()
{
int a, exponent, h;
float math, iteraties, c, stdio, z, verschil;

    char stop='n';
    while (stop!='j')
    {

      printf("vul exponent in:\t\t");
      scanf("%d",&exponent);
      syntax: math= exp(exponent);
      c=1.0;
      h=0;
      stdio=1.0;
      a=0;
      for (iteraties=1.0; a<1; iteraties++)
      {
        h=h+1;
        Syntax: z= pow(exponent,h);

        c=c*iteraties;
        stdio=stdio+(z/c);
        verschil=math-stdio;
        if(verschil<0.01)
        {
            a=2;
        }
      }
      printf("\nexp (%d) uit math.h is:\t\t%f",exponent, math);
      printf("\nexp (%d) m.b.v. de reeks is:\t%f",exponent,stdio);
      printf("\nhet verschil is: \t\t%f", verschil);
      printf("\naantal iteraties is:\t\t%.f", iteraties);

      printf("\n\nstoppen n/j\t\t\t");
      getchar();
      scanf("%c",&stop);
    }

}

What's the point of the syntax: and Syntax: labels?
You don't use them, so remove them.
And if you must use two labels, make them less similar to each other.

When the condition in the while loop is true from the start,
consider rewriting it as a do { ... } while loop.

You don't need the a variable.
You can easily rewrite the program without it.

The coding style is very odd:

Excessive vertical spacing (many empty lines) at many places
Too compact: it's recommended to put spaces around operators

Other tips:

Instead of h=h+1 you can do ++h, as you did in the for loop
Instead of x = x * y you can use x *= y
Instead of x = x + y you can use x += y

With the above suggestions (and some more) applied, the code becomes slightly simpler:

#include <math.h>
#include <stdio.h>

main()
{
    int exponent, h;
    float math, iteraties, c, stdio, z, verschil;

    char stop = 'n';
    do
    {
        printf("vul exponent in:\t\t");
        scanf("%d", &exponent);
        math = exp(exponent);
        c = 1.0;
        h = 0;
        stdio = 1.0;
        for (iteraties = 1.0; ; iteraties++)
        {
            ++h;
            z = pow(exponent, h);

            c *= iteraties;
            stdio += z / c;
            verschil = math - stdio;
            if (verschil < 0.01)
            {
                iteraties++;
                break;
            }
        }
        printf("\nexp (%d) uit math.h is:\t\t%f",exponent, math);
        printf("\nexp (%d) m.b.v. de reeks is:\t%f",exponent,stdio);
        printf("\nhet verschil is: \t\t%f", verschil);
        printf("\naantal iteraties is:\t\t%.f", iteraties);

        printf("\n\nstoppen n/j\t\t\t");
        getchar();
        scanf("%c", &stop);
    } while (stop != 'j');
}


Hearthstone Style Deck Stats for TCG Right now I have this messy method to collect the deck statistics while building a window that will display those statistics.  These statistics are things like the type and mana cost of the card.  I know that this method is too large, so I would like to get some advice for how to make it smaller.

The challenge is that I have to check if the current property in the for loop matches certain predetermined strings before putting the updated information back into the map.  This is messy now, but it will get much messier later when even more properties need to be evaluated.

Here is the method:

private void buildDeckInfoBox() {
    this.deckInfoLabelBox.getChildren().clear();

    Map<Integer, Integer> manaCostValues = new HashMap<>();
    Map<Integer, Integer> scrapCostValues = new HashMap<>();
    Map<String, Integer> creatureTypes = new HashMap<>();

    for (int cardId : this.activeDeckConfig.getChosen().keySet()) {

        CardInfoMessage card = this.cardList.get(cardId);
        int cardCount = this.activeDeckConfig.getChosen().get(cardId);

        for (Map.Entry<String, Object> entry : card.getProperties().entrySet()) {
            String key = entry.getKey();
            String value = String.valueOf(entry.getValue());

            //The value is used as the key for the new map
            //increment the number of cards and put it back in the map
            if (key.equals("creatureType")) {
                if (creatureTypes.get(value) == null) {
                    creatureTypes.put(value, cardCount);
                } else {
                    int creatureCount = creatureTypes.get(value);
                    int newCreatureCount = creatureCount + cardCount;
                    creatureTypes.put(value, newCreatureCount);
                }
            } else if (key.equals("MANA_COST")) {
                if (manaCostValues.get(Integer.parseInt(value)) == null) {
                    manaCostValues.put(Integer.parseInt(value), cardCount);
                } else {
                    int manaCostCount = manaCostValues.get(Integer.parseInt(value));
                    int newManaCostCount = manaCostCount + cardCount;
                    manaCostValues.put(Integer.parseInt(value), newManaCostCount);
                }
            } else if (key.equals("SCRAP_COST")) {
                if (scrapCostValues.get(Integer.parseInt(value)) == null) {
                    scrapCostValues.put(Integer.parseInt(value), cardCount);
                } else {
                    int scrapCostCount = scrapCostValues.get(Integer.parseInt(value));
                    int newScrapCostCount = scrapCostCount + cardCount;
                    scrapCostValues.put(Integer.parseInt(value), newScrapCostCount);
                }
            }
        }
    }

    for (int manaCost : manaCostValues.keySet()) {
        Label manaCostLabel = new Label();
        manaCostLabel.setText(String.format("Mana Cost = %d, Count = %d", manaCost, manaCostValues.get(manaCost)));
        this.deckInfoLabelBox.getChildren().add(manaCostLabel);
    }
    for (int scrapCost : scrapCostValues.keySet()) {
        Label scrapCostLabel = new Label();
        scrapCostLabel.setText(String.format("Scrap Cost = %d, Count = %d", scrapCost, scrapCostValues.get(scrapCost)));
        this.deckInfoLabelBox.getChildren().add(scrapCostLabel);
    }
    for (String creatureType : creatureTypes.keySet()) {
        Label creatureTypeLabel = new Label();
        creatureTypeLabel.setText(String.format("Creature Type %s, Count = %d", creatureType, creatureTypes.get(creatureType)));
        this.deckInfoLabelBox.getChildren().add(creatureTypeLabel);
    }
}

Here is an example of the output:

Eventually I will turn these labels into a fancy statistics graph like the one in Hearthstone, separated by the type of property.

First of all, there are only three specific keys that you are interested in, so there is no need to loop over the map. Just grab the keys that are interesting and see if they are null or not.

Secondly, it's possible to extract a method for increasing the actual card counts in each map.

Object creatureType = card.getProperties().get("creatureType");
Object manaCost = card.getProperties().get("MANA_COST");
Object scrapCost = card.getProperties().get("SCRAP_COST");
if (creatureType != null) {
    increase(creatureTypes, (String) creatureType, cardCount);
}
if (manaCost != null) {
    increase(manaCostValues, Integer.parseInt(String.valueOf(manaCost)), cardCount);
}
if (scrapCost != null) {
    increase(scrapCostValues, Integer.parseInt(String.valueOf(scrapCost)), cardCount);
}

Where the increase method is:

private <T> void increase(Map<T, Integer> map, T key, int cardCount) {
    if (map.get(key) == null) {
        map.put(key, cardCount);
    } else {
        int oldValue = map.get(key);
        int newValue = oldValue + cardCount;
        map.put(key, newValue);
    }
}

Now, the folks over at Oracle noticed that this was a very common pattern in code, so in Java 8 they introduced the Map.merge method:

private <T> void increase(Map<T, Integer> map, T key, int cardCount) {
    map.merge(key, cardCount, (a, b) -> a + b);
}

But what about this pattern?

Object creatureType = card.getProperties().get("creatureType");
if (creatureType != null) {
    increase(creatureTypes, (String) creatureType, cardCount);
}

Well, the new Java 8 Optional class to the rescue!

Optional.ofNullable(card.getProperties().get("creatureType"))
    .ifPresent(obj -> increase(creatureTypes, (String) obj, cardCount));

So your entire code can be changed to:

private void buildDeckInfoBox() {
    this.deckInfoLabelBox.getChildren().clear();

    Map<Integer, Integer> manaCostValues = new HashMap<>();
    Map<Integer, Integer> scrapCostValues = new HashMap<>();
    Map<String, Integer> creatureTypes = new HashMap<>();

    for (int cardId : this.activeDeckConfig.getChosen().keySet()) {

        CardInfoMessage card = this.cardList.get(cardId);
        int cardCount = this.activeDeckConfig.getChosen().get(cardId);

        Optional.ofNullable(card.getProperties().get("creatureType"))
            .ifPresent(obj -> increase(creatureTypes, (String) obj, cardCount));
        Optional.ofNullable(card.getProperties().get("MANA_COST"))
            .ifPresent(obj -> increase(manaCostValues, (Integer) obj, cardCount));
        Optional.ofNullable(card.getProperties().get("SCRAP_COST"))
            .ifPresent(obj -> increase(scrapCostValues, (Integer) obj, cardCount));
    }

    for (int manaCost : manaCostValues.keySet()) {
        Label manaCostLabel = new Label();
        manaCostLabel.setText(String.format("Mana Cost = %d, Count = %d", manaCost, manaCostValues.get(manaCost)));
        this.deckInfoLabelBox.getChildren().add(manaCostLabel);
    }
    for (int scrapCost : scrapCostValues.keySet()) {
        Label scrapCostLabel = new Label();
        scrapCostLabel.setText(String.format("Scrap Cost = %d, Count = %d", scrapCost, scrapCostValues.get(scrapCost)));
        this.deckInfoLabelBox.getChildren().add(scrapCostLabel);
    }
    for (String creatureType : creatureTypes.keySet()) {
        Label creatureTypeLabel = new Label();
        creatureTypeLabel.setText(String.format("Creature Type %s, Count = %d", creatureType, creatureTypes.get(creatureType)));
        this.deckInfoLabelBox.getChildren().add(creatureTypeLabel);
    }
}

private <T> void increase(Map<T, Integer> map, T key, int cardCount) {
    map.merge(key, cardCount, (a, b) -> a + b);
}

It is possible to refactor those last three for loops as well, but I am not sure if it is entirely worth it. Perhaps I'll show how to do that some other time.


react native refactor theme handling I am styling my react native component according to some conditions, here the code, The question is how do I make this cleaner? functional style?
const configColors = (isSingle, isDarkMode) => {
  let colors = {};

  if (isDarkMode){
    colors = {
      ...colors,
      configAxisLabelColor : Colors.white,
      configAxisGridColor : Colors.gridLineGray
    }
  }else{
    colors = {
      ...colors,
      configAxisLabelColor : Colors.lineGray,
      configAxisGridColor : Colors.transparent
    }
  }

  if (isSingle && !isDarkMode) {
    return colors = {
      ...colors,
      configAxisColor: Colors.transparent,
      configLineColor: Colors.lineGreen,
      configTooltipBackground: Colors.lineGreen,
    }
  }
  if (isSingle && isDarkMode) {
    return colors = {
      ...colors,
      configAxisColor: Colors.white,
      configLineColor: Colors.lineBlue,
      configTooltipBackground: Colors.lineBlue,
    }
  }
  if (!isSingle && !isDarkMode) {
    return colors = {
      ...colors,
      configAxisColor: Colors.lightBarGray,
      configLineColor: Colors.lineGray,
      configTooltipBackground: Colors.lineBlue,
    }
  }

}

Thanks in advance!

I think what you are looking for is a ternary operator.
const configColors = (isSingle, isDarkMode) => {
  return {
      configAxisLabelColor : isDarkMode ? Colors.white : Colors.lineGray,
      configAxisGridColor : isDarkMode ? Colors.gridLineGray : Colors.transparent
      configAxisColor: isSingle ? (isDarkMode ? Colors.white: Colors.transparent) : Colors.lightBarGray
      // so on
  }

Another option that reads better could be to create a separate config for each possible combination you want to return.
const configColors = (isSingle, isDarkMode) => {

  const configSingleDark = {
    configAxisLabelColor : Colors.white,
    configAxisGridColor : Colors.gridLineGray
  }

  if (isSingle && isDarkMode) {
    return configSingleDark
  } else if (isSingle && !isDarkMode) {
    return configSingleLight
  } else {
    //
  }
}


Determine if 2 javascript objects have strictly the same properties I need to compare 2 objects to determine if they have strictly the same properties.

This is what I came up with so far:

import isEmpty from 'lodash/isEmpty';

const shallowPropertiesMatch = (firstObject, secondObject) => {
  if (isEmpty(firstObject) || isEmpty(secondObject)) return false;

  const firstObjectKeys = Object.keys(firstObject);
  const secondObjectKeys = Object.keys(secondObject);

  if (firstObjectKeys.length !== secondObjectKeys.length) return false;
  if (!firstObjectKeys.every(value => secondObjectKeys.includes(value))) return false;

  return true;
};

I was wondering if there is a more efficient, elegant or simpler way of doing this?

By this condition, the function returns false when both objects are empty:

if (isEmpty(firstObject) || isEmpty(secondObject)) return false;

I would expect true in this case, and rewrite the condition as:

if (isEmpty(firstObject) != isEmpty(secondObject)) return false;

That is, return false if one of them is empty while the other is not.

In fact, this special treatment is not even necessary, because the rest of the function naturally handles the case of empty objects.

  I was wondering if there is a more efficient, elegant or simpler way of doing this?

The current implementation is not efficient,
because of this step:

if (!firstObjectKeys.every(value => secondObjectKeys.includes(value))) return false;

The problem is that secondObjectKeys is an array,
and therefore .includes does a linear lookup.
You can improve the linear-time lookup to constant-time lookup by converting secondObjectKeys to a set.


Java generics with wildcard vs non generic List to deal with non generic data There is a problem on Exercism Java track that requires writing a function to flatten an arbitrarily nested list to a flattened list without null values.
For Example
input: [1,[2,3,null,4],[null],5]
output: [1,2,3,4,5]
Here is the solution I wrote for it without using wildcards.
public List flatten(List<Object> two) {
    Function<Object, List> mapper = item -> (item instanceof List) ? flatten((List) item) : Collections.singletonList(item);
    return (List) two.stream()
            .filter(x -> x != null)
            .map(mapper)
            .flatMap(x -> x.stream())
            .collect(Collectors.toList());
}

Here is my solution with the wildcard generics. My IDE was happier with this solution.
public List<Object> flatten(List<?> two) {
    Function<Object, List<?>> mapper = item -> (item instanceof List) ? flatten((List<?>) item) : Collections.singletonList(item);
    return two.stream()
            .filter(Objects::nonNull)
            .map(mapper)
            .flatMap(List::stream)
            .collect(Collectors.toList());
}

Is it ok to use the wildcard in this case? Which solution would you prefer? Any other comments about the code are also welcome.

@TorbenPutkonen already provided a great review, I just wanted to add something about usability.
Depending on your use case, returning a List<Object> means that you need to do the casting later. For example:
List<Object> result = flatten(myList);
for(Object o : result) {
    if(o instanceof String) {
        String myString = (String) o;
        // do something with the string
    }
    // etc...
}

If you know the type of the elements you can pass it to the method:
@SuppressWarnings("unchecked")
public <T> List<T> flatten(List<?> list, Class<T> itemType) {
    return list.stream()
            .filter(Objects::nonNull)
            .flatMap(i -> i instanceof List ? flatten((List<?>)i, itemType).stream() : Stream.of((T)i))
            .collect(Collectors.toList());
}

And then use it like:
List<String> result = flatten(myList, String.class);
for(String s : result) {
    // do something with the string
}

This is the version without streams:
@SuppressWarnings("unchecked")
public static <T> List<T> flatten(List<?> list, Class<T> itemType) {
    List<T> result = new ArrayList<>();
    for(Object o : list) {
        if(o instanceof List) {
            result.addAll(flatten((List<?>) o, itemType));
        }
        else if (o != null){
            result.add((T) o);
        }
    }
    return result;
}


Nested blocks in JS remapping function I have an array of objects that have a key attribute with the name of a country and a values attribute that is an array of values that contain data by year:

I needed the opposite: an array of objects where the key is the year and the values an array containing data for each country in the original array.

I wrote this script to remap it, but it feels like a huge hack to me. How can I do this in fewer lines of code and make it more readable?

self.dataByYear = []

$.each(self.dataByGroup, function(i,v) {
    $.each(v.values, function(j,x) {
        var inYears = self.dataByYear.filter(function(d) { return d.key == x.xgroup});
        if (inYears.length > 0 )  {
            if (self.dataByYear[j] !== undefined) {
                self.dataByYear[j].values.push(x);
            }
        } else {
            newObj = {};
            newObj.key = x.xgroup;
            newObj.values = [x];
            self.dataByYear.push(newObj);
        }
    })
});

I'm not sure of any implementation that can actually trim this down (it's doomed to be at least 3 levels of loops), but here's an alternative approach in doing it which I think is much readable.

We group them by year first by going through each country, create an array for each year, and pushing them into the array. Then, we extract the key-value pairs and convert them into an array of objects containing key and value.

var yearsToCountries = self.dataByGroup.reduce(function(dataByYear, group){
  group.values.forEach(function(year){

    // If the country appears just once, then we can do this:
    (dataByYear[year] = dataByYear[year] || []).push(group.key);

    // If the country can appear more than once, we guard the push:
    dataByYear[year] = dataByYear[year] || [];
    if(!~dataByYear[year].indexOf(group.key)) dataByYear.push(group.key);

  });
  return dataByYear;
}, {});

// {
//   1990: ['US', 'Italy', ...],
//   1989: ['US', 'France', ...],
//   ...
// }

var dataByYears = Object.keys(yearsToCountries).map(function(year){
  return { key: year, values: yearsToCountries[year]};
});

// [
//   {key: '1990', values: ['US', 'Italy', ...]},
//   {key: '1989', values: ['US', 'France', ...]},
//   ...
// ]


ruby code  case when not so dry Ruby code below seems to be not so dry. Can you please help me reduce the code.

self.value = case self.type
when 'fast'
  Increment.first.max_hours * Incrementor.first.fast_completion_day
when 'super_fast'
  Increment.first.max_hours * Incrementor.first.super_fast_completion_day
when 'ludicrous'
  Increment.first.max_hours * Incrementor.first.ludicrous_completion_day
else
  Increment.first.max_hours * Incrementor.first.budget_completion_day
end

I think we can use Ruby metaprogamming here to reduce the code. But I am not very good with ruby meta programming at the moment.

You can try:

day = case self.type
  when 'fast'       then :fast_completion_day
  when 'super_fast' then :super_fast_completion_day
  when 'ludicrous'  then :ludicrous_completion_day
  else                   :budget_completion_day
end

self.value = Increment.first.max_hours * Incrementor.first.send(day)

From a friend that knows more than me:

prefix = %w[ fast super_fast ludicrous ].include?(self.type) ? self.type : "budget"
self.value = Increment.first.max_hours * Incrementor.first.send(:"#{prefix}_completion_day")

This is probably about as short and dry as you can get it. That said, I'm not sure that this is more clear/easy to read. I'd probably add a method to Incrementor like Incrementor#completion_day_for(type) that would take care of this logic for you.


Verify that each named sequence is strictly increasing The master launch sequence consists of several independent sequences for different systems. Your goal is to verify that all the individual system sequences are in strictly increasing order. In other words, for any two elements i and j (i < j) of the master launch sequence that belong to the same system (having systemNames[i] = systemNames[j]), their values should be in strictly increasing order (i.e. stepNumbers[i] < stepNumbers[j]).

The input systemNames[i] contains the name of the system to which the ith element of the master launch sequence belongs. Guaranteed constraints:

1 ≤ systemNames.length ≤ 5 · 104,
1 ≤ systemNames[i].length ≤ 10.

The input stepNumbers[i] is an array of positive integers that contains the value of the ith element of the master launch sequence. Guaranteed constraints:

stepNumbers.length = systemNames.length,
1 ≤ stepNumbers[i] ≤ 109.

Return true if all the individual system sequences are in strictly increasing order, otherwise return false.

Example

For systemNames = ["stage_1", "stage_2", "dragon", "stage_1", "stage_2", "dragon"] and stepNumbers = [1, 10, 11, 2, 12, 111], the output should be
    launchSequenceChecker(systemNames, stepNumbers) = true.

There are three independent sequences for systems stage_1, stage_2, and dragon. These sequences are [1, 2], [10, 12], and [11, 111], respectively. The elements are in strictly increasing order for all three.

Here is my code.

(defun launchSequenceChecker (systemNames stepNumbers)
  (setf mmm (remove-duplicates (copy-seq systemNames) :test #'equal))
  (setf lname (map 'list #'identity mmm))
    (setf lnum (make-list  (length mmm):initial-element '0))
  (setf lnum1 (make-list (length mmm):initial-element '1000000001))

    (loop named haha for x from 0 to (1- (length systemNames))
          for name = (aref systemNames x)
          for name1 = (aref systemNames (- (1-(length systemNames)) x))
          for num = (aref stepNumbers x)
          for num1 = (aref stepNumbers (- (1-(length systemNames)) x))
          for pos = (position name lname :test #'equal)
          for pos1 = (position name1 lname :test #'equal)

          if(>= (nth pos lnum) num)
          do(return-from haha nil)
          else
          if(<= (nth pos1 lnum1) num1)
          do(return-from haha nil)
          else
         do
           (setf (nth pos lnum) num )
          (setf (nth pos1 lnum1)num1)
           finally (return-from haha t)))

The problem I am having is that this code passes the tests but exceeds the 6-second time limit.

You are using a list to store the association between names and the previous maximum value. This makes your algorithm O(N^2) because for each element, you might have to go through a list of all previous elements.

You can use a hash table (which has O(1) lookup) to make your algorithm O(N).

You can also add keys when they are not found rather than doing it all at the start for simpler code.

Try to avoid using indices when you can iterate by elements.

quick example solution:

(defun launchSequenceChecker (systemNames stepNumbers)
  (let ((ht (make-hash-table :test 'equal)))
    (loop named l
      for name in systemNames
      for num  in stepNumbers
      do (let ((prev (gethash name ht)))
           (if prev
             (progn
               (when (> prev num)
                 (return-from l nil))
               (setf (gethash name ht)
                     (max prev num)))
             (setf (gethash name ht) num)))
      finally  (return-from l t))))


Entity Framework with multiple connections open? I'm learning Entity Framework and I'm wondering if this code is the proper way of doing this.  Basically this is my attempt at refactoring a very long controller in ASP.Net MVC 4.  I'm using Entity Framework 5 as well.

What I did was I separated out some code into a more common method (buildUpdatedAnswerDetailRecord).

Notice in my parent function I have a connection open because I'm doing other DB related operations (population of data into properties).  But then in the buildUpdatedAnswerDetailRecord method I'm opening another connection and then passing back a AnswerDetail.

Is this the right way to do this or should I have just kept 1 connection open and just duplicated code?

using (var db = new LEAP_Professional_DAL.DAL.LEAPEntitiesDAL())
{

//local variable of a AnswerDetail
AnswerDetail adItem;

//Doing a lot of other Database work (filling other properties)
....

adItem = buildUpdatedAnswerDetailRecord(evaluation.IsLeader, item, backButtonItem.AnswerDetailKey);
db.Entry(adItem).State = EntityState.Modified;

....
//Doing a lot of other Database work (filling other properties)

}

    private AnswerDetail buildUpdatedAnswerDetailRecord(Boolean isLeader, EvaluationObject item, int? detailKey)
    {
        try
        {
            AnswerDetail buildAdItem;
            using (var db = new DAL.DAL.EntitiesDAL())
            {
                buildAdItem = db.AnswerDetails.Find(detailKey);
                if (buildAdItem != null)
                {
                    ((IObjectContextAdapter)db).ObjectContext.Detach(buildAdItem);
                    if (isLeader == true)
                    {
                        buildAdItem.Comment = item.LeaderComment;
                        buildAdItem.AnswerOptionKey = item.LeaderAnswerOptionKey.Value;
                        buildAdItem.UpdatedBy = userName;
                        buildAdItem.UpdateStamp = DateTime.Now;
                    }
                    else
                    {
                        buildAdItem.Comment = item.TeacherComment;
                        buildAdItem.AnswerOptionKey = item.TeacherAnswerOptionKey.Value;
                        buildAdItem.UpdatedBy = userName;
                        buildAdItem.UpdateStamp = DateTime.Now;
                    }

                }
            }
            return buildAdItem;
        }
        catch (Exception ex)
        {
            throw new Exception("buildUpdatedAnswerDetailRecord Error Occured: " + ex.Message.ToString());
        }
    }

Just a quick note about the way exceptions are handled:

catch (Exception ex)
{
    throw new Exception("buildUpdatedAnswerDetailRecord Error Occured: " + ex.Message.ToString());
}

You're shooting yourself in the foot: you're receiving a potentially very meaningful and helpful exception, with a very detailed stack trace, then you're grabbing its message and throwing everything else away (including the stack trace), and throwing a meaningless System.Exception.

At the very least, you should be putting ex as the new exception's InnerException, so as to retain all the details about the exception that was thrown. See this answer for more details about why throwing System.Exception is bad.


Ideas for reducing repetition in control binding I have seven checkboxes on a data maintenance WinForm, which indicate the days of the week that are "valid" for a particular operation to be performed. These "map" to a list of DayOfWeek enum values on the backing domain object; if the value is present in the list, the box should be checked, otherwise not. Any combination of boxes can be checked. In the "bind" method that sets the UI controls to the proper values of the domain object, here's how those checkboxes are set:

chkValidDayMon.Checked = myModel.ValidDaysOfWeek.Contains(DayOfWeek.Monday);
chkValidDayTue.Checked = myModel.ValidDaysOfWeek.Contains(DayOfWeek.Tuesday);
chkValidDayWed.Checked = myModel.ValidDaysOfWeek.Contains(DayOfWeek.Wednesday);
chkValidDayThu.Checked = myModel.ValidDaysOfWeek.Contains(DayOfWeek.Thursday);
chkValidDayFri.Checked = myModel.ValidDaysOfWeek.Contains(DayOfWeek.Friday);
chkValidDaySat.Checked = myModel.ValidDaysOfWeek.Contains(DayOfWeek.Saturday);
chkValidDaySun.Checked = myModel.ValidDaysOfWeek.Contains(DayOfWeek.Sunday);

The code to read the UI controls' state back out to the domain object is similar; if the box is checked, add the corresponding value to a "clean" (initially empty) list and assign it to the object.

This smells. I was thinking of adding a Dictionary containing the checkboxes and keyed to their corresponding DayOfWeek. But, that sounds like a bit much as well (it certainly wouldn't save many LoC). Any other suggestions?

I'd create an extension method that calculates the day by the name of the checkbox.

I'm using dynamic because I don't know the name of your domain class, just replace it with your name.

Also note, I'm using the web version of CheckBox, substitute ID with whatever version you are using.

Here is the class for the extension:

public static class CheckboxExtensions
{
    public static void DetermineStateFromModel(this CheckBox me, dynamic myModel)
    {
        if (!me.ID.Contains("chkValidDay"))
        {
            // Could throw an exception if required.
            return;
        }

        var dayOfWeek = DayOfWeekFromString(me.ID.Replace("chkValidDay", string.Empty));

        me.Checked = myModel.ValidDaysOfWeek.Contains(dayOfWeek);
    }

    private static DayOfWeek DayOfWeekFromString(string dayOfWeek)
    {
        switch (dayOfWeek)
        {
            case "Mon":
                return DayOfWeek.Monday;
            case "Tue":
                return DayOfWeek.Tuesday;
            // ...
            default:
                throw new ApplicationException("Invalid dayOfWeek specified.");

        }
    }
}

Then your calls would be

chkValidDayMon.DetermineStateFromModel(myModel);
chkValidDayTues.DetermineStateFromModel(myModel);
...

Another thing you might look into is to put the CheckBox objects into a list, then you could just iterate it:

dayCheckBoxList.ForEach(checkBox => checkBox.DetermineStateFromModel(myModel));

Another note, Hungarian casing on variables is not really accepted anymore.  It clutters up the code, and there are too many variable types to abbreviate them all to 3 letters.  Read this.  A better naming convention would be mondayIsValidCheckBox.  You would have to modify my extension method to use this new convention.


Sign-up serialization in Python I built a small project using Django-Rest-Framework. Please comment on the best practice style for the code below, which will make it simpler.

class LoginSerializer(serializers.Serializer):
    phone = serializers.CharField()
    password = serializers.CharField()

    def validate(self, data):
        phone = data.get("phone", "")
        password = data.get("password", "")
        # need simplify the structure below
        if phone and password:
            user = authenticate(phone=phone, password=password)
            if user:
                if user.is_active:
                    data['user'] = user
                else:
                    raise exceptions.ValidationError('User is deactivated.')
            else:
                raise exceptions.ValidationError(
                    'Unable to login with given credentials.')
        else:
            raise exceptions.ValidationError(
                'Must provide username and password both.')
        return data

How I can make this more Pythonic?

Returning early makes code not follow the arrow anti-pattern. And makes the flow of the code simpler.
There's otherwise not much to comment on.

class LoginSerializer(serializers.Serializer):
    phone = serializers.CharField()
    password = serializers.CharField()

    def validate(self, data):
        phone = data.get("phone", "")
        password = data.get("password", "")
        # need simplify the structure below
        if not (phone and password):
            raise exceptions.ValidationError('Must provide username and password both.')

        user = authenticate(phone=phone, password=password)
        if not user:
            raise exceptions.ValidationError('Unable to login with given credentials.')

        if not user.is_active:
            raise exceptions.ValidationError('User is deactivated.')

        data['user'] = user
        return data


Singly linked list exercise in C Being completely new to the world of C, I started off with the excercise of building a singly linked list of integers:

/*
  Single linked list test
*/
#include <stdlib.h>
#include <stdio.h>

typedef struct ListElement {
    int         value;
    struct ListElement  *next;
} ListElement;

void printList(ListElement *root) {
    ListElement *current;
    current = root;

    while (current->next) {
        printf("Value: %i\n", current->value);
        current = current->next;
    }

    printf("Value: %i\n", current->value);
}

int freeLast(ListElement *root) {
    int retval;
    retval = 1;
    ListElement *previous;
    ListElement *current;
    current = root;

    while (current->next) {
        previous = current;
        current = current->next;
    }

    if (current == root) {
        retval = 0;
    }

    previous->next = NULL;
    printf("Freeing %i\n", current->value);
    free(current);
    return retval;
}

void freeList(ListElement *root) {
    while(freeLast(root));
}

ListElement * createList(int size) {
    ListElement *root;
    ListElement *current;
    root = malloc(sizeof(ListElement));

    if (root != NULL) {
        root->value = 0;
        current = root;
        int i;

        for (i = 1; i < size; i++) {
            current->next = malloc(sizeof(ListElement));

            if (current->next != NULL) {
                current->next->value = i;
                current = current->next;
            } else {
                freeList(root);
                return NULL;
            }
        }

        return root;
    } else {
        return NULL;
    }
}

int main() {
    ListElement *root = createList(100);
    printList(root);
    freeList(root);
}

While the code compiles and runs as intended I'd like to have some feedback on the coding style, program structure and possibly existing quirks with the memory handling in C, which I hope I got right.

This is pretty good. Minor points:

You can return just root in createList:

ListElement * createList(int size) {
    ListElement *root;
    ListElement *current;
    root = malloc(sizeof(ListElement));

    if (root != NULL) {
        root->value = 0;
        current = root;
        int i;

        for (i = 1; i < size; i++) {
            current->next = malloc(sizeof(ListElement));

            if (current->next != NULL) {
                current->next->value = i;
                current = current->next;
            } else {
                freeList(root);
                return NULL;
            }
        }
    }

    return root;
}

And your freeList is O(n^2) complex, because you are removing from the end. Removing from the beginning would make it O(n).


Classes for sentence-like readability If we have a set of classes or structs such as:

struct Point
{
    float x;
    float y;
};

struct Square
{
    Point topLeft;
    float width;
};

struct Circle
{
    float radius;
    Point middle;
};

If we need to add some functionality to to these, does something like below make sense?

inline Point middleOf(const Square &square)
{
    float halfWidth(square.width/2.0f);
    return Point{
        square.topLeft.x + halfWidth,
        square.topLeft.y + halfWidth};
}

inline Point middleOf(const Circle &circle)
{
    return circle.middle;
}

class MoveSquare
{
public:
    MoveSquare(Square &squareToMove)
    :
    squareToMove(squareToMove)
    {}

    void to(const Point &target)
    {
        squareToMove.topLeft = offsetHalfWidth(target);
    }

private:
    Point offsetHalfWidth(Point p)
    {
        const float halfWidth(squareToMove.width / 2.0f);
        return Point{
            p.x - halfWidth,
            p.y - halfWidth};
    }

private:
    Square &squareToMove;
};

class MoveCircle
{
public:
    MoveCircle(Circle &circle)
    :
    circleToMove(circle)
    {}

    void to(Point position)
    {
        circleToMove.middle = position;
    }

private:
    Circle &circleToMove;
};

inline MoveSquare move(Square &square)
{
    return MoveSquare{square};
}

inline MoveCircle move(Circle &circle)
{
    return MoveCircle{circle};
}

The main idea is being able to have code that reads like a sentence. Such as:

int main() {

    Square aSquare;
    aSquare.topLeft = Point{1,1};
    aSquare.width = 3;

    Circle aCircle;
    aCircle.middle = Point{1,1};
    aCircle.radius = 2;

    move(aSquare).to(Point{1,1});
    move(aSquare).to(middleOf(aCircle));

    move(aCircle).to(middleOf(aSquare));

    return 0;
}

Firstly, a negative remark. The power of C++ means that it is not uncommon for new C++ developers to try and make C++ look like a language they feel more familiar with - sometimes English, sometimes a programming language they already know. This is almost always a bad thing - you need to speak like a native.

However, whilst C++ used for mainstream development does have generally understood idioms, one of its capabilities and original design aims is to try to support different styles of programming.

As well as trying to create an application syntax which is more English like, you are externalizing functionality into proxy objects rather than using the OOP paradigm of member functions. IMHO, this is perfectly legitimate as an experiment.

What you'll likely find is that as you pursue this you gave to make various compromises that finally result in you deciding that maybe this is not he way to go. Or maybe not. See what you find.

If I were to go down this path, I'd probably play with using templates to try and get some generic behaviour, given that the OOP approach us not being used. How exactly this goes will depend on whatever other functionality is needed.

For example:

template<class T>
class Move
{
public:
    Move(T& thing)
        : thing_(thing)
    {}
    virtual void to(const Point& position) = 0;
    T& thing() { return thing_; }
private:
    T& thing_;
};

class MoveSquare : public Move<Square>
{
public:
    MoveSquare(Square &squareToMove)
        : Move<Square>(squareToMove)
    {}

    virtual void to(const Point &target)
    {
        thing().topLeft = offsetHalfWidth(target);
    }

private:
    Point offsetHalfWidth(const Point& p)
    {
        const float halfWidth(thing().width / 2.0f);
        return Point{ p.x - halfWidth, p.y - halfWidth };
    }
};

class MoveCircle : public  Move<Circle>
{
public:
    MoveCircle(Circle &circle)
        : Move<Circle>(circle)
    {}

    virtual void to(const Point& position)
    {
        thing().middle = position;
    }

};


Working with group of checkboxes in MS Excel I have the following checkbox as depicted by the image below:

The checkboxes are in 3 groups. Confidentiality, Integrity ad Availability. When the first in is clicked, the Confidentiality turns Red for High, Orange for Medium and Green for Low.

This is a lot of work to be done as you need to create a subroutine for each checkbox control. Below is a sample code for 2 checkboxes in the first line in the image above.

Private Sub CheckBox21_Change()

With Me.CheckBox21
If .Value Then ClearGroup .GroupName, .Name
End With

End Sub

Private Sub CheckBox22_Change()

With Me.CheckBox22
If .Value Then ClearGroup .GroupName, .Name
End With

End Sub

Private Sub ClearGroup(sGroup As String, sName As String)
    Dim ole As OLEObject

        For Each ole In Me.OLEObjects
            If TypeName(ole.Object) = "CheckBox" Then
                If ole.Object.GroupName = sGroup And ole.Name <> sName Then
                    ole.Object.Value = False
                End If
            End If
        Next ole
End Sub

Private Sub CheckBox21_Click()
    If CheckBox21.Value = True Then
    Range("i6", "m6").Value = "High"
    Range("r3").Value = "H"
    Range("i6", "m6").Interior.Color = RGB(217, 0, 0)
    Range("r3").Interior.Color = RGB(217, 0, 0)

    Else
    CheckBox21.Value = False
    Range("i6", "m6").Value = "Low"
    Range("r3").Value = "L"
    Range("i6", "m6").Interior.Color = RGB(153, 204, 0)
    Range("r3").Interior.Color = RGB(153, 204, 0)
    End If

End Sub
Private Sub CheckBox22_Click()
    If CheckBox22.Value = True Then
    Range("i6", "m6").Value = "Medium"
    Range("r3").Value = "M"
    Range("i6", "m6").Interior.Color = RGB(255, 204, 0)
    Range("r3").Interior.Color = RGB(255, 204, 0)

    Else
    CheckBox22.Value = False
    Range("i6", "m6").Value = "Low"
    Range("r3").Value = "L"
    Range("i6", "m6").Interior.Color = RGB(153, 204, 0)
    Range("r3").Interior.Color = RGB(153, 204, 0)
    End If
End Sub

Is there any other way I can write the VBA code such that the overall line of code will be reduced?

Let's address your question first, then we'll talk about some other things that can be improved.

  This is a lot of work to be done as you need to create a subroutine for each checkbox control.

Well, yes and no. You do need to create an event routine for each control, but there's no reason to repeat all of that code over and over inside each of them. Extract the logic into a paramaterized method. Then, your event procedures will look something like this.

Private Sub CheckBox21_Click()
    HandleCheckBoxClick() 'we'll get to the parameters later
End Sub

Private Sub CheckBox22_Click()
    HandleCheckBoxClick()
End Sub

Which isn't really so much work, is it?

However, to make this work cleanly, we'll need to define a simple data structre and populate it with appropriate values for high, medium, and low. I've used a Type here, but it might be worth creating a full blown class for down the road.

Private Type TStatus
    Name As String
    Abbreviation As String
    Color As Long
End Type

You'll want to store your three different statuses at the module level and create them at an appropriate time. Perhaps on the sheet activate event. Otherwise, you'll need to create them on the fly each time the procedure is run. I'm not sure which way I'd go, but the important part is abstracting the data right now.

Private high As TStatus
Private medium As TStatus
Private low As TStatus

Private Sub InitStatusVariables()
    high.Name = "High"
    high.Abbreviation = "H"
    high.Color = RGB(217, 0, 0) 'Red

    medium.Name = "Medium"
    medium.Abbreviation = "M"
    medium.Color = RGB(255, 204, 0) 'Orange

    low.Name = "Low"
    low.Abbreviation = "L"
    low.Color = RGB(153, 204, 0) ' Green
End Sub

So now, we can go ahead and write that HandleCheckBoxClick method. We'll pass in the actual checkbox control along with our predefined true state and false state.

Private Sub HandleCheckBoxClick(cntrl As CheckBox, trueState As TStatus, falseState As TStatus)
    If cntrl.Value = True Then
        Range("i6", "m6").Value = trueState.Name
        Range("r3").Value = trueState.Abbreviation
        Range("i6", "m6").Interior.Color = trueState.Color
        Range("r3").Interior.Color = trueState.Color

    Else
        cntrl.Value = False
        Range("i6", "m6").Value = falseState.Name
        Range("r3").Value = falseState
        Range("i6", "m6").Interior.Color = falseState.Color
        Range("r3").Interior.Color = falseState.Color
    End If
End Sub

Awesome, but now the duplication inside of the method has become really obvious, but thankfully, easier to deal with. So let's refactor again.

Private Sub HandleCheckBoxClick(cntrl As CheckBox, trueState As TStatus, falseState As TStatus)

    Dim state As TStatus
    If cntrl.Value = True Then
        state = trueState
    Else
        cntrl.Value = False
        state = falseState
    End If

    Range("i6", "m6").Value = state.Name
    Range("r3").Value = state.Abbreviation
    Range("i6", "m6").Interior.Color = state.Color
    Range("r3").Interior.Color = state.Color

End Sub

Note that in your full code, you will likely need to also pass the appropriate ranges into this method as well. Also, I'm not sure if you need this line of code, so I've left it's equivalent in the above method.

Else
CheckBox22.Value = False

Why set the value of the checkbox to False if it's not True? Shouldn't it already be false? Or can the value of a checkbox be "null"? I can't remember, but it's probably worth a comment if you're doing this to make sure the value is false instead of Empty.

This feels like an abuse of the With statement to me.

With Me.CheckBox21
If .Value Then ClearGroup .GroupName, .Name
End With

Why the With statement here? I don't see a real need for it. Maybe it's personal preference, but I think this version of it is nicer.

If Me.CheckBox21.Value = True Then
    ClearGroup Me.Checkbox21.GroupName, Me.CheckBox21.Name
End If

Even better, instead of passing strings to the ClearGroup method, pass the whole CheckBox control.

 If Me.CheckBox21.Value = True Then ClearGroup Me.CheckBox21

There ya go. One line, nice and neat. It would also remove the need to check the type of the control within the method, because you already know it's a checkbox. It has to be, or you get a type mismatch error.

Your indentation is inconsistent. You do very well in some places, but very poorly in others.

Good!

    For Each ole In Me.OLEObjects
        If TypeName(ole.Object) = "CheckBox" Then
            If ole.Object.GroupName = sGroup And ole.Name <> sName Then
                ole.Object.Value = False
            End If
        End If
    Next ole

Bad!

Private Sub CheckBox21_Click()
    If CheckBox21.Value = True Then
    Range("i6", "m6").Value = "High"
    Range("r3").Value = "H"
    Range("i6", "m6").Interior.Color = RGB(217, 0, 0)
    Range("r3").Interior.Color = RGB(217, 0, 0)

    Else
    CheckBox21.Value = False
    Range("i6", "m6").Value = "Low"
    Range("r3").Value = "L"
    Range("i6", "m6").Interior.Color = RGB(153, 204, 0)
    Range("r3").Interior.Color = RGB(153, 204, 0)
    End If

End Sub

Everything inside of an If block should be indented one level.

This is a bit harsh, but I'm not sure how else to put it... Your control names suck. CheckBox21, CheckBox22, CheckBox23... It goes on and on and on doesn't it?

Take the time to give these meaningful names. Yes, it's going to take a while to name all of those checkboxes, but you'll thank yourself later, trust me.

Nifty little program you have there. I think you'll do well if you read up a bit about "Abstract Data Structures" and "Abstraction". All in all you write pretty neat and readable code. Those are good things.


Prime number sieve using difference of two squares Here is a working pari/gp implementation of prime number sieve. Is there something that I could change in code to achieve a better running time?

Sieve(g)=
{
A=vector(floor((g-1)/2));
for(n=1,floor((g-1)/2),
   A[n]=2*n+1);
for(x=3,floor(sqrt(g)),
   if(Mod(x,2)==1,i=0,i=1);
   forstep(y=i,x-3,[2],
      A[(x^2-y^2-1)/2]=0));
for(x=ceil(sqrt(g)),ceil((g+9)/6),
   i=ceil(sqrt(x^2-g));
   if(Mod(x+i,2)==0,i=i+1);
   forstep(y=i,x-3,[2],
      A[(x^2-y^2-1)/2]=0));
A=concat(2,A);
for(j=0,floor((g-1)/2),
   if(!(A[j+1]==0),print(A[j+1])))
}

Good comments make code more maintainable. I had to reverse engineer what this code was doing, when a few comments could have made it clear.

For my taste the code could use a lot more whitespace, but I'm going to try to stick to your style in the proposed changes.

Separation of responsibilities: it would be cleaner to return a vector rather than to "return" with print.

There's a lot in common between the two main loops:

for(x=3,floor(sqrt(g)),
   if(Mod(x,2)==1,i=0,i=1);
   forstep(y=i,x-3,[2],
      A[(x^2-y^2-1)/2]=0));
for(x=ceil(sqrt(g)),ceil((g+9)/6),
   i=ceil(sqrt(x^2-g));
   if(Mod(x+i,2)==0,i=i+1);
   forstep(y=i,x-3,[2],
      A[(x^2-y^2-1)/2]=0));

It might clarify things to refactor them into one:

for(x=3,floor((g+9)/6),
  forstep(y=x-3,0,-2,
    if(x^2-y^2>g,break);
    A[(x^2-y^2-1)/2]=0
  ));

Note that this fixes an out-by-one in the upper bound of x.

Now I'm ready to look at performance. The low-hanging fruit here is x^2-y^2: it's usually faster to calculate a constant product or square of a loop index by addition. I'm also going to use bitwise operations for multiplication and division by 2 because that means not having to worry about how clever the compiler is.

for(x=3,floor((g+9)/6),
  d=(x<<1)-1;
  forstep(y=x-3,0,-2,
    d+=(y+1)<<2;
    if(d>g,break);
    A[d>>1]=0
  ));

You should easily be able to prove the invariant that d == x^2 - y^2, and since it's odd (d-1)/2 == d>>1. In fact, we can instead work with (x^2-y^2)>>1:

cap=(g-1)>>1;
for(x=3,floor((g+9)/6),
  d=x-1;
  forstep(yp=(x-2)<<1,2,-4,
    d+=yp;
    if(d>cap,break);
    A[d]=0
  ));

With Try It Online! and pushing towards the size limit for allocation of A I see a speedup from 4.4s (user) to 3.5s. (Output removed for timing purposes. Timings available by expanding the "Debug" section).

Other optimisations:

If the vector is just initialised as vector((g-1)>>1) then it will initially contain 0 at all indices. Then change A[d]=0 to A[d]=1 and change the logic in the output loop.
Ditch the concat and output 2 as a special case.

Beyond that, you need to look at different algorithms. The binary quadratic form x^2-y^2 doesn't seem to be a common one in prime sieving. You could see how it compares with Sundaram's sieve (BQF x+y+2xy) or failing that, if you're working with large enough ranges, Atkin-Bernstein (e.g. a combination of 3x^2+y^2, 4x^2+y^2, 5x^2+3y^2, 15x^2+y^2, and 3x^2-y^2 for different totients of 60).


Produce the nth lexicographic permutation of a string consisting of letters H and V I have tried many possible methods here but I am still not able to reduce the time complexity of below algorithm.

//This function returns the next lexicographical permutation of the StringBuilder s
static StringBuilder getKRankString(StringBuilder s)
{
    char ch[] = (s.toString()).toCharArray();
    int i = 0, j=0;
    //This loop gets the highest i for which ch[i]<ch[i+1]
    for(int k=0;k<s.length()-2;k++){
        if(ch[s.length()-2-k]<ch[s.length()-1-k]){
            i = s.length()-2-k;
            break;
        }

    }
    //System.out.println(i);
    //This loop gets the highest j for which ch[j]>ch[i]
    for(int k=s.length()-1;k>i;k--){
        if(ch[k]>ch[i]){
            j = k;
            break;
        }

    }
    //System.out.println(j);
    //swap characters at i and j
    char temp = ch[i];
    ch[i] = ch[j];
    ch[j] = temp;
    //Append the original string till i to reversed string from i till end
    StringBuilder swapped = new StringBuilder(new String(ch));
    //System.out.println(swapped);
    String sb1 = swapped.substring(0, i+1);
    String sb2 = (new StringBuilder(swapped.substring(i+1, s.length())).reverse()).toString();
    StringBuilder str = new StringBuilder(sb1+sb2);
    return str;
}

static String[] gridLand(String[] inp) {
    String[] outputArr = new String[inp.length];
    for(int i=0;i<inp.length;i++){
    //get the first line of input and get integer x and y
        int x = Integer.valueOf(inp[i].split(" ")[0]);
        int y = Integer.valueOf(inp[i].split(" ")[1]);
        int k = Integer.valueOf(inp[i].split(" ")[2]);
        StringBuilder sb = new StringBuilder();
    //form the original string using x H's and y V's.
    //This will be the first String of this order
        for(int j = 0; j < x; j++)
            sb = sb.append("H");
        for(int j = 0; j < y; j++)
            sb = sb.append("V");
        //String s1 = sb.toString();
    //run the below functoin K times to get Kth lexicographical string
        for(int m = 0; m < k; m++)
            sb = getKRankString(sb);
        outputArr[i] = sb.toString();
    }
    return outputArr;

}

The basic layout of my problem is as follows:
I have a string that is composed of only H's and V's. There can be not more than 10 H's and 10 V's possible. I need to find the Kth lexicographical order of such pattern. Also, inp[] array contains input is format:

2 2 3
4 5 4

which means that total H = 2, total V = 2 and I need to print 3rd lexicographical permutation of HHVV.
Similarly for 4 5 4, I need to print 4th lexicographical permutation of HHHHVVVVV.

Let P(n,m) be the number of permutations of n Hs and m Vs.  For m>0 and n>0, P(n,m) = (n + m)!/(n! * m!).

Let's say you want the kth permutation of n Hs and m Vs:

If k <= P(n-1,m), then the answer is an H followed by the kth permutation of n-1 Hs and m Vs.

Otherwise the answer is a V followed by the (k-P(n-1,m))th permutation of n Hs and m-1 Vs.


My implementation of dynamic array Below is my implementation of dynamic array without help of library functions. Kindly provide your suggestions on design, coding style and algorithm.

MyDynamicArray.java

import java.util.NoSuchElementException;

public class MyDynamicArray<T>
{
    private int positionPointer=0;
    private int arraySize;
    private T[] dynamicArray;
    private static final int DEFAULT_ARRAY_SIZE=10;

    public MyDynamicArray()
    {
        this(DEFAULT_ARRAY_SIZE);
    }

    public MyDynamicArray(int arraySize)
    {
        this.arraySize=arraySize;
        dynamicArray=(T[]) new Object[arraySize];
    }

    public void addElement(T element)
    {
        adjustSize();
        dynamicArray[positionPointer]=element;
        positionPointer++;
    }

    public void addElementAtNode(int index, T element)
    {
        if(index<positionPointer)
        {
            dynamicArray[index]=element;
        }
        else
        {
            addElement(element);
            throw new ArrayIndexOutOfBoundsException("index "+index+" is greater than the size of array "+(positionPointer-1)+" \nElement added to end of array..");
        }
    }

    private void adjustSize()
    {
        if(positionPointer==arraySize)
        {
            increaseSize();
        }
        else if(positionPointer==(arraySize/4-1))
        {
            decreaseSize();
        }
    }

    private void increaseSize()
    {
        T[] tempArray=(T[]) new Object[2*arraySize];
        for(int i=0;i<positionPointer;i++)
        {
            tempArray[i]=dynamicArray[i];
        }
        dynamicArray=tempArray;
        arraySize=2*arraySize;
    }

    private void decreaseSize()
    {
        T[] tempArray=(T[]) new Object[(arraySize/4)];
        for(int i=0;i<positionPointer;i++)
        {
            tempArray[i]=dynamicArray[i];
        }
        dynamicArray=tempArray;
        arraySize=arraySize/4;
    }

    public int searchElement(T element)
    {
        for(int i=0;i<positionPointer;i++)
        {
            if(dynamicArray[i].equals(element))
            {
                return i;
            }
        }
        throw new NoSuchElementException("Element not found : "+element.toString());
    }

    public T getElementAtIndex(int index)
    {
        if(index<positionPointer)
        {
            return dynamicArray[index];
        }
        else
        {
            throw new ArrayIndexOutOfBoundsException("index "+index+" is greater than the size of array "+positionPointer);
        }
    }

    public void removeElement(T element)
    {
        int index=searchElement(element);
        if(index>0)
        {
            removeElementAtIndex(index);
        }
    }

    public void removeElementAtIndex(int index)
    {
        if(index<positionPointer)
        {
            for(int i=index;i<positionPointer-1;i++)
            {
                dynamicArray[index]=dynamicArray[index+1];
            }
            dynamicArray[positionPointer-1]=null;
            positionPointer--;
            adjustSize();
        }
        else
        {
            throw new ArrayIndexOutOfBoundsException("index "+index+" is greater than the size of array "+positionPointer);
        }
    }

    public int size()
    {
        return positionPointer;
    }

}

In almost all methods for example in this one instead of

public void removeElement(T element)
{
    int index=searchElement(element);
    if(index>0)
    {
        removeElementAtIndex(index);
    }
}

should be

public void removeElement(T element)
{
    int index=searchElement(element);
    if(index >= 0)
    {
        removeElementAtIndex(index);
    }
}

Because otherwise the first element will get you an error.


Check for pixel spacing between two folder images Im trying to match the pixel spacing between two folders, here matches has 994 keys and it shows that i may take more than a day to compute this. Is there any way i can speed up this operation.

the load series function looks like this

import SimpleITK as sitk
def load_series(path):
    reader = sitk.ImageSeriesReader()
    dicom_names = reader.GetGDCMSeriesFileNames(path)
    reader.SetFileNames(dicom_names)
    return reader.Execute()

import csv
bar = ProgressBar()
file = open('BenignTest.csv', 'w')
writer = csv.writer(file)
for item in bar(matches.keys()):
    for case in matches[item]:
        load_case = load_series(case)
        if load_case.GetSpacing() == load_series(item).GetSpacing():
            writer.writerow((item , case))
            break

The matches dictionary looks like this,

{'/mnt/sdd1/DSB2017/stage1/0030a160d58723ff36d73f41b170ec21': ['/mnt/sde1/Cancer/128443/01-02-1999-LSS-59120/2-0OPAGELSPLUSD3602.512060.00.11.5-98506',
  '/mnt/sde1/Cancer/213485/01-02-1999-ACRIN-13247/3-0OPAGELSPLUSLUNG3502.51204026.71.5-85097',
  '/mnt/sde1/Cancer/206342/01-02-1999-ACRIN-68321/3-0OPAGELSPLUSLUNG3702.51205033.31.5-72233',
  '/mnt/sde1/Cancer/200209/01-02-2000-CRIN-10216/3-1OPAGELSPLUSLUNG4002.51204026.71.5-42354']...........}

Thanks in advance

The simplest improvement that you can do right now is to avoid recomputing the same reference image over and over for each case of an item: just store load_series(item).GetSpacing() before looping over each case.

You can also use writer.writerows and feed it a list or a generator instead of writer.writerow. This should optimize disk writes a bit, even though reading images should be the task that take up the most resources here.

Proposed improvements:

import csv
import SimpleITK as sitk

def load_series(path):
    reader = sitk.ImageSeriesReader()
    dicom_names = reader.GetGDCMSeriesFileNames(path)
    reader.SetFileNames(dicom_names)
    return reader.Execute()

def generate_equal_spacing(matches, progress_bar=None):
    items = matches.keys()
    if progress_bar is not None:
        items = progress_bar(items)

    for item in items:
        reference_spacing = load_series(item).GetSpacing()
        for case in matches[item]:
            if load_series(case).GetSpacing() == reference_spacing:
                yield (item, case)
                break

def main(matches, output_filename='BenignTest.csv'):
    with open(output_filename, 'w') as f:
        writer = csv.writer(f)
        writer.writerows(generate_equal_spacing(matches, ProgressBar()))

if __name__ == '__main__':
    main({
        '/mnt/sdd1/DSB2017/stage1/0030a160d58723ff36d73f41b170ec21': [
            '/mnt/sde1/Cancer/128443/01-02-1999-LSS-59120/2-0OPAGELSPLUSD3602.512060.00.11.5-98506',
            '/mnt/sde1/Cancer/213485/01-02-1999-ACRIN-13247/3-0OPAGELSPLUSLUNG3502.51204026.71.5-85097',
            '/mnt/sde1/Cancer/206342/01-02-1999-ACRIN-68321/3-0OPAGELSPLUSLUNG3702.51205033.31.5-72233',
            '/mnt/sde1/Cancer/200209/01-02-2000-CRIN-10216/3-1OPAGELSPLUSLUNG4002.51204026.71.5-42354',
        ],
        ...
    })