Single Linked List - Improvements - 1 - not complete TO DO

exception handling
merge with dynamic arrary

*SO Bug here--remove this line and Control K will not work

/*
Added in comments from Jerry, Ed and Loki
This code borrows from from http://cslibrary.stanford.edu/
C++ Version - lnode, lnode_reverse, lnode_push, lnode_print, lnode_count
*/
#include "c_arclib.cpp"
template <class T1> class sl_list
  {
  private:
    class node_1
      {
      public:
        T1 data;
        node_1 *next;
        node_1(T1 data, node_1 *next = NULL) : data(data), next(next) {}
      };
    node_1 *head;
    int count;
  public:
    sl_list() : count(0), head(NULL){}
    void push(int data)
      {
      node_1 *newNode = new node_1(data, head);
      ++count;
      head = newNode;
      }
    void print()
      {
      node_1 *current=head;
      while (current != NULL)
        {
        cout << current->data << endl;
        current = current->next;
        }
      }
    void reverse()
      {
      node_1 *previous, *current, *future;
      previous=NULL;
      current=head;
      future=head;
      while(current!=NULL)
        {
        future=current->next;
        current->next=previous;
        previous=current;
        current=future;
        }
      head=previous;
      }
    void break_heap()
      {
      int i=0;
      while(1)
        {
        i++;
        push(1);
        }
      }
    };
/*
C Version - lnode, lnode_reverse, lnode_push, lnode_print, lnode_count
*/

typedef struct node_type
  {
  int data;
  struct node_type* next;
  } lnode_type;

struct lnode
  {
  int data;
  struct lnode* next;
  };
void lnode_reverse(struct lnode*& head)
  {
  struct lnode *previous, *current, *future;
  previous=NULL;
  current=head;
  future=head;
  while(current!=NULL)
    {
    future=current->next;
    current->next=previous;
    previous=current;
    current=future;
    }
  head=previous;
  }
void lnode_push(struct lnode*& head, int data)
  {
  struct lnode* newNode = new lnode;
  newNode->data = data;
  newNode->next = head;
  head = newNode;
  }
void lnode_print(struct lnode* current)
  {
  while (current != NULL)
    {
    cout << current->data;
    current = current->next;
    }
  }
int lnode_count(struct lnode* current)
  {
  int count = 0;
  while (current != NULL)
    {
    count++;
    current = current->next;
    }
  return count;
  }
void break_heap()
  {
  struct lnode* head = NULL;
  int i=0;
  while(1)
    {
    i++;
    lnode_push(head,1);
    }
  }
int main()
  {
  sl_list<int> list1;
  list1.push(10);
  list1.push(20);
  list1.push(30);
  list1.push(40);
  list1.push(50);
  list1.push(60);

  list1.print();
  list1.reverse();
  list1.print();
  }

/* C++ Version - lnode, lnode_reverse, lnode_push, lnode_print, lnode_count
   I need node_type turned into a templated class..How does one do this?
*/
typedef struct node_type
  {
  int data;
  struct node_type* next;
  } lnode_type;

Turning it into a template is pretty straightforward:

template <class T>
class node_type {
   T data;
   node_type *next;
};

Note that the typedef isn't really necessary (or desirable) in C++. You normally just want to use class whatever, and then the body of the class definition. This makes whatever usable as a name by itself, just about like the typedef would in C.

There's one other thing we probably want to do though: the whole basic idea of a linked-list node is really just an implementation detail of the linked list, so we really want to make the node type a nested class inside of the linked-list class:

template <class T>
class sl_list {

    struct node {
        T data;
        node *next;
    } head;

public:
    // Other stuff here.
};

From there, we want to write the functions that work with the data type to use T instead of int. For example, instead of your push(int), you might write something like this:

void push(T data) {
  ++count; // prefer pre-increment when you don't need the old value.
  node* newNode = new node;
  newNode->data = data;
  newNode->next = head;
  head = newNode;
}

We might prefer to add a ctor to the node class/struct, something like this:

node(T data, node *next = NULL) : data(data), next(next) {}

With that, push becomes something like this:

void push(T data) {
    node *newNode = new node(data, head);
    ++count;
    head = newNode;
}

[Edit2: Note that, as recommended by @LokiAstari, I've also moved the ++count to after allocating the new node. This way if the attempted allocation fails (and new throws an exception), count will not be incremented, so it retains the correct values. If and only if the allocation succeeds, it's incremented to reflect the new node having been added to the list. ]

[Edit: and node would look roughly like this:

    struct node {
        T data;
        node *next;

        node(T data, node *next = NULL) : data(data), next(next) {}
    } head;

]


Coin problem through brute force I have some code that will brute force solve the following problem:

  Given a set of x coins and a target sum to reach, what is the fewest number of coins required to reach that target?

The code so far:

import java.util.ArrayList;
import java.util.Arrays;

public class coinsSum {
    public static int min = Integer.MAX_VALUE;
    public static int[] combination;
    public static final int TARGET = 59;

    public static void main(String[] args) {
        long start = System.nanoTime();

        int[] validCoins = new int[] {1, 2, 5, 10, 20};
        Arrays.sort(validCoins);
        int len = validCoins.length;

        ArrayList<Integer> maxList = new ArrayList<Integer>();
        for(int c : validCoins) {
            maxList.add(TARGET / c);
        }

        int[] max = new int[len];
        for(int i = 0; i < len; i++) {
            max[i] = maxList.get(i).intValue();
        }

        permutations(new int[len], max, validCoins, 0); // bread&butter

        if(min != Integer.MAX_VALUE) {
            System.out.println();
            System.out.println("The combination " + Arrays.toString(combination) + " uses " + min + " coins to make the target of: " + TARGET);
        } else {
            System.out.println("The target was not reachable using these coins");
        }

        System.out.println("TOOK: " + (System.nanoTime() - start) / 1000000 + "ms");
    }

    public static void permutations(int[] workspace, int[] choices, int[] coins, int pos) {
        if(pos == workspace.length) {
            int sum = 0, coinCount = 0;
            System.out.println("TRYING " + Arrays.toString(workspace));
            for(int a = 0; a < coins.length; a++) {
                sum += workspace[a] * coins[a];
                coinCount += workspace[a];
            }
            if(sum == TARGET) {
                // System.out.println(Arrays.toString(n)); //valid combinations
                if(coinCount < min) {
                    min = coinCount;
                    combination = workspace;
                    System.out.println(Arrays.toString(combination)+" uses " + min + " coins");
                }
            }
            return;
        }
        for(int i = 0; i <= choices[pos]; i++) {
            workspace[pos] = i;
            permutations(workspace, choices, coins, pos + 1);
        }
    }
}

This solution uses recursion. Is there any way to compute combinations in Java using loops?

What other improvements can be made to the algorithm?

Your code is quite complicated. What makes it so complicated is that you have recursive calls interacting with global objects. This makes it almost impossible to understand what the state is at a given point.

When you write an algorithm in a recursive way, your goal should be to make the (smallest amount of) information go in one direction only through the method parameters (and no global state whatsoever) and to use return values to give the answer back.

This being said, let's try to see how to make this work.

This idea is quite simple here:

If I have to reach 59 with 1, 2, 5, 10, 20, I can try to take 0 time 20 and see how I can reach 59 with 1, 2, 5, 10. I can also try to take 1 time 20 and see how I can reach the remaining 39 with 1, 2, 5, 10. I can also try to take 2 times 20 and see how I can reach 19 with the remaining 1, 2, 5, 10.

I've reduced solving the problem to the resolution of smaller (because we have a smaller number of coins) problems. The question is "when should I stop ?". If you have reached the target, you can stop. If you have no coin left to try, you can stop;

Once expressing this with code, you can write:

import java.util.ArrayList;
import java.util.Arrays;

public class coinsSum {
    public static final int TARGET = 59;

    public static void main(String[] args) {
        long start = System.nanoTime();

        int[] validCoins = new int[] {1, 2, 5, 10, 20};
        Arrays.sort(validCoins);

        int min = permutations(TARGET, validCoins);
        System.out.println("TOOK: " + (System.nanoTime() - start) / 1000000 + "ms to find " + min);
    }

    public static int permutations(int target, int[] coins) {
        return permutations(target, coins, coins.length);
    }

    public static int permutations(int target, int[] coins, int array_size) {
        System.out.println("TRYING " + target + " with " + array_size + " different coins");
        assert target >= 0;
        assert array_size >= 0;
        assert array_size <= coins.length;
        if (target==0) // target is reached (in 0 coin)
            return 0;
        if (array_size == 0) // no more coins to reach a positive target
            return -1;
        array_size--;
        int coin = coins[array_size]; // let's consider the biggest coin
        int nb = target/coin; // and check how many times it fits in the target
        int sol = -1; // we haven't found a solution so far (Integer.MAX_VALUE would be another way to do this)
        for (int i = 0; i <= nb; i++) // let's try all possible values
        {
            // to check if we can reach the target with the other coins
            int sol_cand = permutations(target - i * coin, coins, array_size);
            if (sol_cand >= 0) // if we did
            {
                if (sol == -1 || sol_cand + i < sol) // and the new solution is better
                    sol = sol_cand + i; // then we are happy
            }
        }
        return sol;
    }
}

This is already 24 times faster than your example.

Now, you can try to add optimisations. A simple optimisation is to say that if you can reach the current target with the biggest coin, there's no need to go any further because you'll need more coins if you use smaller coins.

    int coin = coins[array_size]; // let's consider the biggest coin
    int nb = target/coin; // and check how many times it fits in the target
    if (nb * coin == target) // if it fits an exact number of time, we won't beat this
        return nb;

This makes the code 70 times faster on the example. (If you do so, the check that target == 0 is not relevant anymore because we'll always have target > 0).

Also, not a particular optimisation but if you want to use with Integer.MAX_VALUE just like you used to, you can rewrite the loop:

    int sol = Integer.MAX_VALUE; // we haven't found a solution so far (Integer.MAX_VALUE would be another way to do this)
    for (int i = 0; i <= nb; i++) // let's try all possible values
    {
        sol = Math.min(sol, i + permutations(target - i * coin, coins, array_size));
    }

This is as simple as it gets. Then, you can write unit tests to check that your code behaves the way you want. Finally, if you want to, you can consider various optimisations: using a cache to avoid computing the same thing multiple times, add conditions to stop the loop earlier, etc.


Using a certain query field based on state I need to be able to do a select to a table using a where clause that can use an email or phone field, depending on if they are set/not empty or not. Given this context, how can I simplify this PHP code?

 if(isset($email) && !empty($email) && isset($phone) && !empty($phone)){
        $partQuery = 'account_id = '.$account->id.' AND (phone = "'.(int)$phone.'" OR email = "'.$email.'")';
    }

    if(isset($phone) && !empty($phone)){
        $partQuery = 'account_id = '.$account->id.' AND phone = "'.(int)$phone.'"';
    }

    if(isset($email) && !empty($email)){
        $partQuery = 'account_id = '.$account->id.' AND email = "'. $email.'"';
    }

First of all, use prepared statements.

The account id part is always the same, so extract it to the beginning.

Also, you can extract the isset and not empty part to a function to simplify the code.

$partQuery = 'account_id = '.$account->id;

if(isNotEmpty($phone) && isNotEmpty($email)) {
    $partQuery .= ' AND (phone = "'.(int)$phone.'" OR email = "'.$email.'")';
}
if(isNotEmpty($phone)) {
    $partQuery .= ' AND phone = "'.(int)$phone.'"';
}
if(isNotEmpty($email)) {
    $partQuery .= ' AND email = "'. $email.'"';
}

function isNotEmpty($var) {
    return isset($var) && !empty($var);
}

Although I would assume that that is not actually what you want, as it adds two additional ANDs in case neither phone nor email is empty.

It also seems like an odd query in general. You are fine with a wrong phone number as long as the email is correct and vise versa?

Also note that you don't actually need isset, you can just use !empty, it will not generate a warning.


Using custom aliases for physical units Since I use C++ mainly to do physical calculations, I often come across the problem that I forget which units a certain variable is in, e.g. radian or degree, or meter vs. millimeter.

For this reason, I started using a custom header file defining my units, which is posted below. The basic idea is that I never create something like:

double myAngle = 10;

Instead of using:

radian_t myAngle = 10;

Later, when passing it somewhere else, nobody will know whether it is in radian or degree. The upsides are listed below.

Are there any downsides to this?

Example implementation:

units.h:

#ifndef INCLUDE_UNITS_H_
#define INCLUDE_UNITS_H_

namespace units {
  using meter_t  = double;
  using newton_t = double;
  using radian_t = double;
  using joule_t  = double;

} // units

#endif /* INCLUDE_UNITS_H_ */

sample.h:

using namespace units;

joule_t myEnergy (newton_t force, meter_t length)
{ joule_t myEnergy = force * length; }

The upsides are:

Fast unit lookup: In Eclipse, I only need to hover over a variable and instantly know the unit it is in, simple by seeing its type (meter_t myLength vs double myLength).
Easy debugging: The following code is obviously wrong, since Joule is Newton * meter:

joule_t myEnergy (newton_t force, millimeter_t length)
{ joule_t myEnergy = force * length; }

Is this function correct? Who knows!

double myEnergy (double force, double length)
{ double myEnergy = force * length; }

Overloading? In theory, I could even implement classes instead of using "using" declarations. Is that correct? This would let me overload the operators, making sure that sin.operator*(radian_t rad) is different from sin.operator*(degree_t deg)?

My question is, are there any downsides to this?

There's a huge downside. One is that the compiler does not enforce your units. Your type names are aliases. You could pass a variable of type newton_t to a function expecting a meter_t. The only way to detect this error is via inspection. Your use of units give a false sense of security.

Another is that it invites you to create a plethora of units such as millimeter_t, etc. This will get you in trouble. My advice: Except for input and output, stick to strict SI units. No millimeters or kilometers, no minutes or days, no grams or tonnes. Lengths internally are always in meters, time durations are always in seconds, masses are always in kilograms (except for star and planet masses, which are expressed in meters3/seconds2; see https://en.wikipedia.org/wiki/Standard_gravitational_parameter).

Another way to look at it is to look at Wolfram Alpha. Dozens, if not hundreds, of human years of development effort went into this product -- and it still messes up with units. You are not going to replicate that effort. Using consistent units throughout is the easiest way to confront the units problem.


Extract Pages from PDF based on search in python Everything is working fine except timing.
it takes lot time for my file containing 1000 pages and having 100 pages of interest.

import re
from PyPDF2 import PdfFileReader, PdfFileWriter
import glob, os

# find pages
def  findText(f, slist):
    file = open(f, 'rb')
    pdfDoc = PdfFileReader(file)
    pages = []
    for i in range(pdfDoc.getNumPages()):
        content = pdfDoc.getPage(i).extractText().lower()
        for s in slist:
            if re.search(s.lower(), content) is not None:
                if i not in pages:
                    pages.append(i)
    return pages

#extract pages
def extractPage(f, fOut, pages):
    file = open(f, 'rb')
    output = PdfFileWriter()
    pdfOne = PdfFileReader(file)
    for i in pages:
        output.addPage(pdfOne.getPage(i))
    outputStream = open(fOut, "wb")
    output.write(outputStream)
    outputStream.close()
    return

os.chdir(r"path\to\mydir")
for pdfFile in glob.glob("*.pdf"):
    print(pdfFile)
    outPdfFile = pdfFile.replace(".pdf","_searched_extracted.pdf")
    stringList = ["string1", "string2"]
    extractPage(pdfFile, outPdfFile, findText(pdfFile, stringList))

Updated code after suggestions is at:

https://gist.github.com/pra007/099f10b07be5b7126a36438c67ad7a1f

You could try profiling but the code is simple enough that I think you're spending most of the time in PyPDF2 code. Two options:

You can preprocess your PDF files to store their text somewhere, which will make the search phase much faster, especially if you run multiples queries on the same PDF files
You can try another parser such as a Python 3 version of PDFMiner, or even a parser written in a faster language


How to replace plain URLs with links, in javascript? I've nearly got this working.  I wanted to know if there is a much better way.  One problem is that no matter what there will be cases where a URL is incorrectly identified and as such the end result will be dependent on requirements.  My requirements are lackadaisy, I just want something that works most of the time.  However some of the URLs in my fiddle are not being detected correctly and I'd like advice on the regex part of this for overcome these.  The main issue here is that the overall design is in question, the should be a simpler method.

Root problem

Fiddle

function replaceURLWithHTMLLinks(text) {
    text = text.replace(/a/g, "--ucsps--");
    text = text.replace(/b/g, "--uspds--");
    var arrRegex = [
        /(\([^)]*\b)((?:https?|ftp|file):\/\/[-A-Za-z0-9+&@#\/%?=~_()|!:,.;]*[-A-Za-z0-9+&@#\/%=~_()|])(.?\))/ig,
        /()(\b(?:https?|ftp|file):\/\/[-a-z0-9+&@#\/%?=~_()|!:,.;]*[-a-z0-9+&@#\/%=~_()|])(.?\b)/ig];
    for (i = 0; i < arrRegex.length; i++) {
        text = text.replace(arrRegex[i], "$1a$2b$3");
    }
    text = text.replace(/a([^b]*)b/g, "$1");
    text = text.replace(/--ucsps--/g, "a");
    text = text.replace(/--uspds--/g, "b");
    return text;
}
var elm = document.getElementById('trythis');
elm.innerHTML = replaceURLWithHTMLLinks(elm.innerHTML);

Any thoughts?

I think that it's nutty to replace all characters 'a' and 'b' with junk, then un-replace them later, just so that you can use 'a' and 'b' as landmarks to indicate which points your two regular expressions have in common.  If you're going to do that, then why not just use "--ucsps--" and "--uspds--" as your landmarks in the first place?  Of course, using any special strings as landmarks is a bad idea.

The fundamental problem is that you want to exclude the same number of closing parentheses as there are open parentheses.  You can't use a regular expression to match left and right parentheses, because such an expression would not be regular.  Well, some flavours of "regular" expressions support backreferences within the expression itself, which could do the job.  RegExps in JavaScript don't.

What JavaScript can do, though, is let you specify your own code to generate the replacement text.  Here's what I came up with:

function replaceURLWithHTMLLinks(text) {
    var re = /(\(.*?)?\b((?:https?|ftp|file):\/\/[-a-z0-9+&@#\/%?=~_()|!:,.;]*[-a-z0-9+&@#\/%=~_()|])/ig;
    return text.replace(re, function(match, lParens, url) {
        var rParens = '';
        lParens = lParens || '';

        // Try to strip the same number of right parens from url
        // as there are left parens.  Here, lParenCounter must be
        // a RegExp object.  You cannot use a literal
        //     while (/\(/g.exec(lParens)) { ... }
        // because an object is needed to store the lastIndex state.
        var lParenCounter = /\(/g;
        while (lParenCounter.exec(lParens)) {
            var m;
            // We want m[1] to be greedy, unless a period precedes the
            // right parenthesis.  These tests cannot be simplified as
            //     /(.*)(\.?\).*)/.exec(url)
            // because if (.*) is greedy then \.? never gets a chance.
            if (m = /(.*)(\.\).*)/.exec(url) ||
                    /(.*)(\).*)/.exec(url)) {
                url = m[1];
                rParens = m[2] + rParens;
            }
        }
        return lParens + "" + url + "" + rParens;
    });
}


Generate all permutations I have written this Java snippet to generate all permutations of a given array of integers. Can somebody review it and give some pointers as to how to make it more generic? I.e. instead of dealing with only ints, it should be able to handle a collection of any data type.

import java.util.Arrays;
public class PrintPermutations{
        private int[] taperedArray(int[] arr,int idx){
                int nArray[] = new int[arr.length-1];
                for(int i=0,j=0;i<nArray.length;i++,j++){
                  if(j == idx){j+=1;}
                  nArray[i] = arr[j];
                }
                return nArray;
        }

        public void printPermutations(String prefix,int[] arr){
                if(arr.length == 1){
                  System.out.println(prefix + arr[0]);
                }else{
                 for(int i=0;i<arr.length;i++){
                   printPermutations(prefix+arr[i]+",",taperedArray(arr,i));
                 }
                }
        }

        public static void main(String[] args){
                PrintPermutations pp = new PrintPermutations();
                int data[] = new int[]{1,2,3};
                pp.printPermutations("",data);
        }
}

public class PrintPermutations{

Classes are types of things, so they usually get noun names, e.g.

public class PermutationPrinter<T> {

You mention that you want this to work with generic types.  The way to do that is to add a generic parameter.

        public void printPermutations(String prefix,int[] arr){

Unfortunately, there are no generic arrays in Java, so you have to switch to a different data type.  You have a list of items, so let's switch to List.

    public void permuteAndPrint(String prefix, List<T> items) {

Your method actually does two things, not one.  Thus, the compound name.  This actually violates something called the Single Responsibility Principle, but I'm going to ignore that for now.

I prefer plural names for collections, so I changed the now inaccurate arr to items.

Side note:  you use eight column indent for the first two levels and then use smaller indents thereafter.  The usual suggestion is to always tab the same amount.  I switched to a consistent four column tab, as that's pretty common (standard) in Java programming.

                if(arr.length == 1){
                  System.out.println(prefix + arr[0]);
                }else{

Rather than use an if/else construct, it is common to return from the base case of a recursive function:

        if (items.size() == 1) {
            System.out.println(prefix + items.get(0));
            return;
        }

This allows the rest of the function to work without extra indent and makes it more recognizable as a base case.

                 for(int i=0;i<arr.length;i++){
                   printPermutations(prefix+arr[i]+",",taperedArray(arr,i));
                 }

There are several things that I don't like about this, starting with the index-based for loop.  I'd rather use the foreach format:

        for ( T item : items ) {
            List<T> remnants = new ArrayList<T>(items);
            remnants.remove(item);
            permuteAndPrint(prefix + item + ",", remnants);
        }

A useful feature of List is that it includes a remove function that replaces your taperedArray function.

Now rather than saying arr[i], we can say a more readable item.

So the whole class is now

public class PermutationPrinter<T> {

    public void permuteAndPrint(String prefix, List<T> items) {
        if (items.size() == 1) {
            System.out.println(prefix + items.get(0));
            return;
        }

        for ( T item : items ) {
            List<T> remnants = new ArrayList<T>(items);
            remnants.remove(item);
            permuteAndPrint(prefix + item + ",", remnants);
        }
    }

    public static void main(String[] args) {
        PermutationPrinter<Integer> printer = new PermutationPrinter<Integer>();
        List<Integer> data = Arrays.asList(1,2,3);
        printer.permuteAndPrint("", data);
    }

}

This does mostly the same steps as your original and still violates the Single Responsibility Principle.  Normally we'd write main something like

    public static void main(String[] args) {
        List<Integer> data = Arrays.asList(1,2,3);
        PermutationPrinter<Integer> printer = new PermutationPrinter<Integer>(data);
        printer.permute();
        printer.print();
    }

This has several advantages.  For one, it saves the caller from passing an empty string to the recursive function.  Instead, we can now write

public class PermutationPrinter<T> {
    List<T> items;
    List<List<T>> permutations = new ArrayList<>();

    public PermutationPrinter(List<T> data) {
        items = data;
    }

    private void _permute(List<T> permutation, List<T> data) {
        if (data.size() <= 0) {
            permutations.add(permutation);
            return;
        }

        for ( T datum : data ) {
            List<T> remnants = new ArrayList<T>(data);
            remnants.remove(datum);
            List<T> elements = new ArrayList<T>(permutation);
            elements.add(datum);
            _permute(elements, remnants);
        }
    }

    private void permute() {
        List<T> permutation = new ArrayList<T>();
        _permute(permutation, items);
    }

    private void print() {
        for ( List<T> permutation : permutations ) {
            T last = permutation.remove(permutation.size() - 1);

            for ( T element : permutation ) {
                System.out.print(element + ", ");
            }

            System.out.println(last);
        }
    }

}

Notice how this hides the details of how the recursive function works inside a public method.  This allows the recursive method to be private.  We could change its interface tomorrow with no impact outside this class.

Another advantage is that it is easier to unit test the state of variables than output.  So we may not be able to unit test print, but we can certainly unit test permute and _permute.

This also allows us to move main out of the class and into its own class.  This class shouldn't need a main method.

Note:  I haven't attempted to optimize the algorithm.  I just rewrote it in a new style.


Check if it's a square or rectangle I want to check in my if and else if statement if the inputs form a square or a rectangle, now my if and else if statements seems to be unreadable, now I'm thinking of ways to improve it.

  /* input are angles in degrees, 1st side is parallel to 3rd side, 2nd side is parallel to 4th side*/
  static void Main(string[] args)
  {
     Console.Write("Input first: ");
     int first = int.Parse(Console.ReadLine());

     Console.Write("Input second: ");
     int second = int.Parse(Console.ReadLine());

     Console.Write("Input third: ");
     int third = int.Parse(Console.ReadLine());

     Console.Write("Input fourth: ");
     int fourth = int.Parse(Console.ReadLine());

     // all sides are equal
     // logic for if and else-if, needs codereview for improvement
     if (first == second && first == third && first == fourth && second == third && second == fourth && third == fourth)
     {
        Console.WriteLine("Square");
     }
     else if (first != second && first == third && first != fourth && second != third && second == fourth )
     {
        Console.WriteLine("Rectangle");
     }
     else
     {
        Console.WriteLine("Not a square nor a rectangle");
     }

     Console.ReadKey();
  }

Simplify the if by just doing

if (first == second && second == third && third == fourth)
{....}

You said the inputs are degrees, now it’s been awhile since geometry but squares and rectangles are determined by length of side, not angle degree. Both are 4 sided objects so every angle is the same (unless you’re talking about something like a trapezoid, rhombus).


Python Dictionary Black Magic I am defining a subclass of a the python dictionary object mpCmd where every item is converted to a lambda.  The intended usage is that every item in the dictionary can be called with a single list argument (row) that will return a value based on row.  Integers and optionally strings are considered indexes of the row. When converted to indexes, strings are converted as though they were Excel style column names.

A single integer n will become lambda row: row[n]. A tuple with a function and a sequence of indexes transforms as

(func, (n1, n2, n3)) => lambda row: func(row[n1], row[n2], row[n3])

Here are some basic examples

>>>command = mpCmd({0: 0, 'B': (sum, ([1, 2, 4],)), 'C': 'SPAM'})
>>>command[0](['Idle', 'Palin', 'Cleese', 'Chapman', 'Gilliam', 'Jones']))
'Idle'
>>>command[1]([1, 2, 3, 4, 5])
10
>>>command[2](ANYTHING)
'SPAM'

I have tested it and it works.  Any advice appreciated, even if it's just confirming this is black magic.

def rmap(func, sequence):
    return [rmap(func, i) if isinstance(i, (tuple, list))
            #elif isinstance(i, dict) ???
            else func(i)
            for i in sequence]

def name_to_index(col_name):
    """Converts Excel Style column name to zero offset index
    """
    return reduce((lambda index, char: index*26 + int(char, 36) - 9),
                  col_name, 0) - 1

class mpCmd(dict):
    """Stores user defined maps and converts them to f(vector) = scalar

    Every item stored in mpCmd will be converted to int: (lambda row: some_func)
    """

    def __init__(self, map_dict, offset=0,
                 int_is_index=True, str_is_name=False):
        self.offset = offset
        self.int_is_index = int_is_index
        self.str_is_name = str_is_name
        super(mpCmd, self).__init__(self._convert_dict(map_dict))

    # Override setters, do no override accessors
    def __setitem__(self, key, val):
        super(mpCmd, self).__setitem__(*self._convert_item(key, val))

    def update(self, other):
        super(mpCmd, self).update(self._convert_dict(other))

    # Macro functions
    def _convert_dict(self, other):
        return {self._convert_key(key): self._convert_val(val)
                    for key, val in other.items()}

    def _convert_item(self, key, val):
        return self._convert_key(key), self._convert_val(val)

    # actual replacement
    def _convert_val(self, val):
        if (isinstance(val, int) and self.int_is_index
                or isinstance(val, str) and self.str_is_name):
            return (lambda row, index=self._convert_key(val): row[index])

        elif isinstance(val, (tuple, list)):
            func, indexes = val
            assert callable(func)
            indexes = rmap(self._convert_key, indexes)
            return (lambda row: func(*rmap((lambda i: row[i]), indexes)))

        else:
            return (lambda *args, **kwargs: val)

    def _convert_key(self, key):
        if isinstance(key, str):
            return name_to_index(key)
        elif isinstance(key, int):
            return key - self.offset
        else:
            raise TypeError

I have a few points I would like to suggest.

Firstly, I would rename the name_to_index function to fit the verb-first function naming convention. Based on your other naming conventions, I would use something like convert_name_to_index.

The next thing I would do is rename your mpCmd class. The Pythonic convention is to use PascalCase instead of camelCase for class names. Also, shortening words can be handy in some places:

management --> mgt
character --> char

While, in others places it can inhibit understanding:

# Is this a comfy cat? A comfy cot? Comfy cut?
ct.description = 'Comfy'

Now, in your context it is pretty easy to know what the name is supposed to mean. However, especially in class names, its better to have too many characters than have too few. I would rename your class to something like CommandMap.

My next points touch on your convert_val function.

Python understands the conditional in your if-statement just fine (because or has a higher precidence than and). However, to the human eye, it can be ambiguous:

# Is the grouping this?
if (part1 and part2) or (part3 and part4):

# Or this?
if ((part1 and part2) or part3) and part4:

# Or this?
if (part1 and (part2 or part3)) and part4:

I would use the structure of my first example above in your code to help the statement feel less ambiguous.

The second part of convert_val function that I would change is your assert callable(func) line. The assert keyword is generally used in testing and just feels odd here. Here is how I would structure that section of code:

elif isinstance(val, (tuple, list)):
    func, indexes = val
    indexes = rmap(self._convert_key, indexes)

    if not callable(func):
        raise TypeError('{} is not callable'.format(func))

    return (lambda row: func(*rmap((lambda i: row[i]), indexes)))

My final point is in your convert_key function. If the input is not either an int or a str then you raise TypeError. As you have it now this is what prints:

>>>raise TypeError
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError

Instead of raising the class TypeError, raise an instance of type TypeError with its own message:

>>>raise TypeError('Key must be an int or a string.')
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: Key must be an int or a string.

Other than these suggestions, your code looks nice.


Find the shortest & best starting move in Mancala (Kalah) The version of Mancala impelemented in this game is as follows:

The board looks like this:

   O   O   O   O   O   O
@                         @
   O   O   O   O   O   O

Each O represents a pit that contains four 'seeds'. The @ is called the 'store' where seeds are accumulated, as points.

Each player takes one side (top or bottom) of the board, and play begins by a player removing the seeds from one pit on their side of the board and 'sowing' them one seed per pit in the adjacent pits, in a counter-clockwise order. A player deposits a seed in their own 'store' when passing it. The opponents 'store' is ignored. If the last seed in a players hand is sown in the 'store' the player can move again, picking any pit on their side of the board.

The version of the game coded for allows for 'relay' play, i.e. a turn ends only when the last seed in the players hand is sown in an empty pit. Otherwise, the player can scoop up the seeds in the last pit sown and continue sowing into the next pit, etc.

If the last seed in sown in an empty pit on the player's own side of the board, they capture the seeds in the pit opposite it (on the opponent's side) and deposit them in their store.

This code searches for two things:

The shortest sequences of choices that will win on the first move (>24 points)
The sequence that gives the most points on the first move.

using System;
using System.Collections.Generic;
using System.Linq;

namespace Mancala
{
    static public class Program2
    {
        static (int[] seq, int count) shortest, max;

        public static void Main(string[] args)
        {
            Branch(Enumerable.Empty<int>(), new int[] { 4, 4, 4, 4, 4, 4, 0, 4, 4, 4, 4, 4, 4 });
            Console.WriteLine($"Shortest:     ({shortest.count}) | {string.Join(",", shortest.seq)}");
            Console.WriteLine($"Max:          ({max.count}) | {string.Join(",", max.seq)}");
            Console.ReadKey();
        }

        static void Branch(IEnumerable<int> seq, int[] initialState)
        {
            for (int i = 0; i <= 5; i++)
            {
                var pit = i;
                var state = initialState.ToArray();

                do {
                    var hand = state[pit];
                    state[pit] = 0;
                    for (int j = pit + 1; j <= pit + hand; j++) state[j % 13]++;
                    pit = (pit + hand) % 13;
                } while (pit != 6 && state[pit] > 1) ;
                if (pit < 6 && state[pit] == 1) state[6] += state[12 - pit];  // capture rule
                if (state[6] > 24) Update(seq.Concat(new int[] { i }), state[6]);
                if (pit == 6) Branch(seq.Concat(new int[] { i }), state);
            }
        }

        static void Update(IEnumerable<int> seq, int count)
        {
            var vseq = seq.ToArray();
            if (shortest.count == 0 || vseq.Length < shortest.seq.Length || vseq.Length == shortest.seq.Length && count > shortest.count)
                shortest = (vseq, count);
            if (count > max.count || count == max.count && vseq.Length < max.seq.Length)
                max = (vseq, count);
        }
    }
}

Kind of surprised no one else has offered constructive criticisms.  Let's get the ball rolling.  Keep in mind I don't know anything about Mancala.

Overall code organization could be better.  Rather than static methods in Program2 class, I would go with separate class(es).  I suggest you review posts on Tic-Tac-Toe, where multiple classes are used for the board versus players versus moves.

There are a lot of magic numbers in the code: 5, 6, 12, 13, and 24 to name a few.  I'd suggest using named constants or else change the code a bit, e.g. if 6 is a named constant, sometimes use < Constant instead of 5, and <= Constant for 6.  As it is, it gets confusing for someone to follow your logic.

initialState.ToArray() is totally unnecessary, unless you are trying to have a independent clone.  Your intent is not apparent from the code.  And I find that true about much of your code.  It's hard to follow your intent.

You may consider using List instead of the combination of arrays and IEnumerable.

The ValueTuple pair named (seq, count) has a few issues.  I think the spelling should be Pascal-cased, with seq spelled out as Sequence.  Plus, count seems confusing.  It's not the count of the associated sequence but rather seems to the the point tally of the sequence.  Thus I would rename count to Points.

I would rather use a class than ValueTuple.  You can have a variety of constructors, plus:

Override ToString() to simplify the 2 times you use it in Main.
Create specific methods to compare 2 instances for shorter sequence length or higher point tally.  This may require implementing IEquatable or IComparable.


Is it ok to use goto in catch? start:
try
{
    oleDbDataAdapter1.Fill(myDataTable);
}
catch (Exception ex) {
    DialogResult res = MessageBox.Show("Can't load data:\n" + ex.Message, "Error",
        MessageBoxButtons.RetryCancel, MessageBoxIcon.Error);
    if (res == DialogResult.Retry)
    {
        goto start;
    }
}

Is it ok to use goto operators, which lead outside a try/catch block?  What consequences may it lead to?
How can I make this code better?

I'm personally not a fan of goto's and to be honest I don't even know if I've ever used on in c#.  I tend to find they are a bit hard to follow as you have to jump around a bit too much and the natural flow of the code is potentially broken.

Here's an idea for using a different approach (Updated as suggested by svick)

string errorMessage;
while(!TryTableFill(myDataTable, out errorMessage))
{
    DialogResult result = MessageBox.Show("Can't load data:\n" + errorMessage, "Error", MessageBoxButtons.RetryCancel, MessageBoxIcon.Error);

    if(result != DialogResult.Retry)
        break;
}

private boolean TryTableFill(myDataTable, out string errorMessage)
{
    errorMessage = string.Empty;

    try
    {
       oleDbDataAdapter1.Fill(myDataTable);
       return true;
    }
    catch (Exception ex) {
       errorMessage = ex.Message;
       return false;
    }
}

or alternatively (I like this way but I'm not sure if it's better.  Added just to provide an alternative):

private boolean TryTableFill(myDataTable, out string errorMessage)
{
    errorMessage = string.Empty;

    try
    {
       oleDbDataAdapter1.Fill(myDataTable);
    }
    catch (Exception ex) {
       errorMessage = string.Format("Error occured: {0}", ex.Message);
    }

    return !string.isNullOrEmpty(errorMessage);
}


Refactor nested foreach loop based on object property I'm using a levenshtein matching algorithm to match a title to a filename (for anime). I either use the main show title for matching or its alternative titles (flexget_titles).

For that I have the following snippet of code.

public function matchFilenameToShow($filename)
{
    $shows = $this->shows->getAll();
    $shortest = -1;
    $clean_name = $this->removeEpisodeNumber($this->cleanFilename($filename));
    foreach ($shows as $show) {
        if (!empty($show->flexget_titles_array)) {
            foreach ($show->flexget_titles_array as $title) {
                // calculate the distance between the input string filename,
                // and the current show name
                $lev = levenshtein(strtolower($clean_name), strtolower($title));
                // if this distance is less than the next found shortest
                // distance, OR if a next shortest distance to the word has not yet been found
                if ($lev <= $shortest || $shortest < 0) {
                    // set the closest match, and shortest distance
                    $matchedShow = $show;
                    $shortest = $lev;
                }
            }
        } else {
            // calculate the distance between the input string filename,
            // and the current show name
            $lev = levenshtein(strtolower($clean_name), strtolower($show->title));
            // if this distance is less than the next found shortest
            // distance, OR if a next shortest distance to the word has not yet been found
            if ($lev <= $shortest || $shortest < 0) {
                // set the closest match, and shortest distance
                $matchedShow = $show;
                $shortest = $lev;
            }
        }
    }
    return $matchedShow;
}

Obviously there is code repetition and I want to refactor this. However I'm not sure how to do it. I have a feeling that I need to extract that levenshtein part to a separate method, but I need to return 2 variables.

You can create "fallback" version of nested array in the case when $show->flexget_titles_array is empty and handle it with the same nested foreach loop:

...
foreach ($shows as $show) {
    ($arr = $show->flexget_titles_array) || ($arr = array($show->title));
    foreach ($arr as $title) {
        // calculate the distance between the input string filename,
        // and the current show name
        $lev = levenshtein(strtolower($clean_name), strtolower($title));
        // if this distance is less than the next found shortest
        // distance, OR if a next shortest distance to the word has not yet been found
        if ($lev <= $shortest || $shortest < 0) {
            // set the closest match, and shortest distance
            $matchedShow = $show;
            $shortest = $lev;
        }
    }
}
...


Ising model simulation using metropolis algorithm I am new to this community; I have tried my best to respect the policy of the community. I have written the Monte Carlo metropolis algorithm for the ising model. I want to optimize the code. I have tried my best. I want to optimize it further. The following is the code:

(I have used tricks like finding exponential only once, careful generation of random number, etc.)

import numpy as np
import time
import random

def monteCarlo(N,state,Energy,Mag,Beta,sweeps):
    if sweeps > 10000:
        print("Warning:Number of sweeps exceeded 10000\n\tsetting number of sweeps to 10000")
        sweeps = 10000
    start_time = time.time()
    expBeta = np.exp(-Beta*np.arange(0,9))
    E = np.zeros(sweeps)
    M = np.zeros(sweeps)
    for t in range(sweeps):
        for tt in range(N*N):
            a = random.randint(0, N-1)
            b = random.randint(0, N-1)
            s = state[a,b]
            delta_E = 2*s*(state[(a+1)%N,b] + state[a,(b+1)%N] + state[(a-1)%N,b] + state[a,(b-1)%N])
            if delta_E < 0:
                s *= -1
                Energy += delta_E
                Mag += 2*s
            elif random.random() < expBeta[delta_E]:
                s *= -1
                Energy += delta_E
                Mag += 2*s
            state[a, b] = s
        E[t] = Energy
        M[t] = Mag
    print("%d monte carlo sweeps completed in %d seconds" %(sweeps,time.time()-start_time))
    return E,M #returning list of Energy and Magnetization set

#####lattice config#####
"""N is lattice size
nt is number of Temperature points
sweeps are number of mc steps per spin"""
print("Starting Ising Model Simulation")
N = int(input("Enter lattice size : "))
startTime = time.time()
nt = 10
N2 = N*N
sweeps = 10000 #mc steps per spin
"""we will plot the following wrt temperature, T"""
T = np.linspace(2, 3, nt) #you can set temperature range
"""preparing lattice with all spins up"""
state = np.ones((N,N),dtype="int")
Energy = -N2
Mag = N2
#temperature loop
#for k in tqdm_gui(range(nt)):
for k in range(nt):
    temp = T[k]
    Beta=1/temp
    print("____________________________________\nTemperature is %0.2f, time is %d" %(temp,time.time()-startTime))
    E, M = monteCarlo(N,state,Energy,Mag,Beta,sweeps) #list of Energy and Magnetization
    Energy = E[-1]
    Mag = M[-1]

#further code is finding magnetization, autocorrelation, specific heat, autocorrelation, etc.
```

I cannot yet comment due to my reputation, so I'll write this as an answer to your comment on Andrew's answer, and delete if someone comments this information or Andrew updates his answer.

Saying that numpy's random is not a good random number generator does not seem right to me.
From numpy's reference :

  By default, Generator uses bits provided by PCG64 which has better statistical properties than the legacy MT19937 used in RandomState.

So to me it seems that :

numpy uses the PCG64 random generator which, according to numpy, has better statistical properties than legacy MT19937
numpy used to use MT19937
you can still chose to use the MT19937 random number generator

from numpy.random import Generator, MT19937
rg = Generator(MT19937(12345))
rg.random()

Maybe there is something I'm missing, and maybe it is in this part of your comment

  It produces 53-bit precision floats and has a period of 2**19937-1

If so, I'd be interested to know how numpy's random would still be flawed for a Monte-Carlo analysis.


Extracting and reformatting dates from text The task is find by pattern d--m--y---- all occurrences in text, reformat it to yyyy:mm:dd and print it separately from initial text. Actually I done all except  formatting. And even in my written code I have some doubts because I just begun to learn this language. Maybe someone would help me and say what's wrong with my code and how to finish that task on a better way. Any comments are appreciated.

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

#define MAXLINE 1000
#define CONT 1000000

int getlin(char *, int);
int pickDates(char *, char *);
int isDate(char *, int);

int main()
{
    int len;
    char line[MAXLINE];
    char accum[CONT];
    char *dates = malloc(sizeof(char *) * CONT);

    while ((len = getlin(line, MAXLINE)) > 0)
        strcat(accum, line);

    pickDates(dates, accum);
    printf("%s\n", dates);

    free(dates);

    return 0;
}

int getlin(char *s, int lim) {
    int i, c, j = 0;

    for (i = 0; (c = getchar()) != EOF && c != '\n'; ++i)
    if (i < lim - 2)
    {
        s[j] = c;
        ++j;
    }
    if (c == '\n')
    {
        s[j] = c;
        ++i;
        ++j;
    }
    s[j] = '\0';

    return i;
}

int pickDates(char *dates, char *cont)
{
    int j = 0;
    const char *template = "d--m--y----";
    char *date;
    int temp_len = strlen(template);

    for (int i = 0; i < strlen(cont); i++)
        if (cont[i] == template[0] && cont[i+3] == template[3] && cont[i+6] == template[6])
        {
            date = malloc(sizeof(char) * temp_len);
            memcpy(date, &cont[i], temp_len);
            if (isDate(date, temp_len))
            {
                j += 8;
                strcat(dates, date);
            }
            free(date);
        }

    dates = realloc(dates, sizeof(char) * j);

    return 0;
}

int isDate(char *date_str, int len)
{
    int i = 0;
    int dd = atoi(&date_str[i+1]);
    int mm = atoi(&date_str[i+4]);
    int yy = atoi(&date_str[i+7]);
    char tmp[5] = {'0'};

    memset(date_str, 0, len - 3);
    date_str = realloc(date_str, sizeof(char) * (len - 3));

    if (dd < 32 && dd > 0 && mm < 13 && mm > 0 && yy > 0)
    {
        sprintf(tmp, "%04d", yy);
        strcat(date_str, tmp);
        sprintf(tmp, "%02d", mm);
        strcat(date_str, tmp);
        sprintf(tmp, "%02d", dd);
        strcat(date_str, tmp);

        return 1;
    }
    else return -1;
}

Whenever declaring a function that you only expect to use in the current file, mark it static.

The (anonymous) user in the comments above is correct to indicate that accum is quite large; however, rather than allocating it statically as a global I'd suggest that it be allocated from the heap (malloc).

printf("%s\n", dates) is equivalent to puts(dates), but the latter is more efficient.

Having to predeclare variables in C hasn't been needed since the 90s. Do not predeclare variables. Declare them where they're used; i.e.

for (int i = 0;

That loop doesn't do what you think it does. You're missing braces. The loop will only apply to the first if.

j += 8

Where does 8 come from? Declare this as a constant. Magic numbers are bad.

It's pointless to have a return value for pickDates, so make it void.

date_str in isDate should be a const char * because you shouldn't be modifying it. The same applies to dates in pickDates.

Later, where you have a series of sprintf / strcat, do not use strcat, nor a tmp array. You can write to date_str via a temporary pointer that you increment based on the return value of sprintf.

I suggest inverting this logic:

if (dd < 32 && dd > 0 && mm < 13 && mm > 0 && yy > 0)
{
    // ...
    return 1;
}
else return -1;

to

if (dd < 0 || dd > 31 || mm < 0 || mm > 12 || yy < 0)
    return -1;
// ...
return 1;

Also, if that return value indicates success or failure, you should use a boolean from stdbool.h and use true/false instead of an integer.

This:

int i = 0;
int dd = atoi(&date_str[i+1]);
int mm = atoi(&date_str[i+4]);
int yy = atoi(&date_str[i+7]);

doesn't make a whole lot of sense; i might as well not exist and you might as well do

int dd = atoi(date_str + 1),
    mm = atoi(date_str + 4),
    yy = atoi(date_str + 7);


Finding the maximum number in a 2D vector Several people who work with Rust have said that if you find yourself using a lot of clones, you're probably doing something wrong. I'm trying to rewrite some code I wrote to find the maximum number in a 2D vector to use RefCells for the inner vector due to the fact that (as much as I hate it) I need interior mutability on the 2D vec for my physics engine. However, all my code related to this now needs a ton of clone calls. I'm using the "maximum integer" part of my codebase just because its the shortest and easiest to explain piece. Here is my code:

let max_row = |row: &RefCell<Vec<Unit>>| -> Option<usize> {
    row.clone()
       .into_inner()
       .iter()
       .fold(None, |max, unit: &Unit| match max {
        None => Some(unit.tiles.len()),
        el @ Some(_) => {
            if el.unwrap() < unit.tiles.len() {
                Some(unit.tiles.len())
            } else {
                el
            }
        }
    })
};

// find the highest height in this list of rows. Same as above, but uses above. (:
let toplevel = world.map
                    .iter()
                    .fold(None, |max, vec| match max {
    None => max_row(vec),
    r @ Some(_) => {
        match max_row(vec) {
            None => r,
            z @ Some(_) => {
                if r.unwrap() < z.unwrap() { z } else { r }
            }
        }
    }
});

This is a tad ugly, but the worst part is the use of .clone and .into_inner right in a row after sending it as a pointer. For some reason, it just feels icky, probably because I did that as a fix, not how I originally was hoping it could turn out. I have only a mediocre understanding of borrowing, which could be the problem.

To clarify the code, here is Unit:

#[derive(Clone)]
pub struct Unit {
  pub tiles: Vec<Tile>,
}

// And a basic version of Tile

#[derive(Copy, Clone)]
pub enum Tile {
  Wall,
  Floor,
}

And world is just a Vec<RefCell<Vec<Unit>>>.

I really hope there is a better way, and if there is, could you explain it in such a way that I could apply it to other ugly areas of my codebase where this is also a problem? For instance, I have another area where I need to use a vector from inside a closure, but also need to modify it later, so in the closure I am cloning it, which seems like an ugly solution.

Normally, I start by walking through the code and improving it step-by-step, but this time I'll start with one piece of advice and showing my final solution. Read the Iterator docs and commit the method names and a rough idea of what they do to memory!

let max = world.iter().flat_map(|r| {
    let r = r.borrow();
    r.iter().map(|unit| unit.tiles.len()).max()
}).max();

Instead of matching, seeing if something is a Some, then unwrapping it, just bind the pattern to a variable:

match max {
    None => max_row(vec),
    Some(old_max) => {
        match max_row(vec) {
            None => Some(old_max),
            Some(new_max) => {
                if old_max < new_max {
                    Some(new_max)
                } else {
                    Some(old_max)
                }
            }
        }
    }
}

Extract that max_row call to before the match, since it's done unconditionally.
Getting a default from an Option is a common pattern: Option::unwrap_or and Option::unwrap_or_else. Converting an Option<T> to another Option<U> , but only if it's Some, is also a common pattern: Option::map and Option::and_then. You should also memorize what capabilities Option and Result have.

let mr = max_row(vec);
max.map(|old_max| {
    match mr {
        None => Some(old_max),
        Some(new_max) => {
            if old_max < new_max {
                Some(new_max)
            } else {
                Some(old_max)
            }
        }
    }
}).unwrap_or(mr)

Repeat.

max.map(|old_max| {
    mr.map(|new_max| {
        if old_max < new_max {
            Some(new_max)
        } else {
            Some(old_max)
        }
    }).unwrap_or(Some(old_max))
}).unwrap_or(mr)

map(F).unwrap_or(D) is also a common pattern: Option::map_or:

max.map_or(mr, |old_max| {
    mr.map_or(Some(old_max), |new_max| {
        if old_max < new_max {
            Some(new_max)
        } else {
            Some(old_max)
        }
    })
})

I don't know how to incrementally get out of this code. One possibility is to switch to a match statement to try and flatten some of the cases:

match (max, max_row(vec)) {
    (None, new_max) => new_max,
    (Some(old_max), None) => Some(old_max),
    (Some(old_max), Some(new_max)) => {
        if new_max > old_max {
            Some(new_max)
        } else {
            Some(old_max)
        }
    }
}

std::cmp::max exists, so I'd use that:

match (max, max_row(vec)) {
    (None, new_max) => new_max,
    (Some(old_max), None) => Some(old_max),
    (Some(old_max), Some(new_max)) => Some(cmp::max(new_max, old_max)),
}

Again, I don't know a way to incrementally get from here to iterator methods. This is why I encourage everyone to memorize what functions exist so that you can read the iterator documentation when you know there's something that solves a problem.


"Ringing in the New Year (2011→ 2012)" puzzle I am  trying to solve the puzzle RINGING IN THE NEW YEAR.

  Start at 2011. By moving through the maze and doing any arithmetic operations you encounter, exit the maze with a result of 2012. You may pass through an operation several times, but not twice in a row.

  2011  2012

My solution is to construct an breadth first search tree of the graph, and go through it, do the arithmetic operations.

In my windows 7 machine, it takes 29s to get the result.

How to make it better? (Improvement of algorithm or better way of compiling)

module Main where

import Data.Ratio
import Data.Tree

data Label = Label
  { f1     :: (Rational -> Rational)
  , accept :: Bool
  , f2     :: (Rational -> Rational)
  , value  :: Rational
  , disp   :: [String]
  }

initial = Node (Label (const 2011) False (const 2011) 2011 [""])
               [ Node (Label (const 2011) False (+7) 0 ["+7"]) center1
               ]

label1, label2, label3, label4 :: Label
label1 = Label (+7)         False (/2)          0 ["+7", "/2"]
label2 = Label (/2)         False (+7)          0 ["/2", "+7"]
label3 = Label (subtract 5) True  (*3)          0 ["-5", "*3"]
label4 = Label (*3)         True  (subtract 5) 0 ["*3", "-5"]

tree1, tree2, tree3, tree4 :: Tree Label
tree1 = Node label1 center2
tree2 = Node label2 center1
tree3 = Node label3 center4
tree4 = Node label4 center3

center1, center2, center3, center4 :: Forest Label
center1 = [ tree3, tree4, tree2 ]
center2 = [ tree3, tree4, tree1 ]
center3 = [ tree4, tree1, tree2 ]
center4 = [ tree3, tree1, tree2 ]

travel :: Tree Label -> [Label]
travel t = map rootLabel $
                      concat $ takeWhile (not . null) $
                      iterate (\ts -> concat $ map step ts) [t]

step :: Tree Label -> Forest Label
step (Node l sub) = map update sub
  where
    update (Node l' sub') = Node l'{ accept = (accept l') && ((f1 l') v == 2012)
                                   , value = ((f2 l').(f1 l')) v, disp = disp l ++ disp l'} sub'
    v = value l

main = (print . (\l -> concat (disp l) ++ " = " ++ show (value l))) $ head $ filter accept $ travel initial

I wrote this solution that runs in 0.2 seconds on my computer when compiled. It also does a BFS with memoisation. However, it uses two heuristics: Don't divide by two unless the denominator is 1 and don't go above 10000. A more elegant way would be to use a priority queue that prioritises "states" like these. However, using these heuristics does not guarantee an optimal solution.

As a side note, this question might be better for code review than Stack Overflow.

import Data.Ratio
import Data.Map (Map)
import qualified Data.Map as M
import Data.Sequence hiding (reverse)

-- Left, Middle, or Right
data Pos = L | M | R
 deriving (Eq,Ord,Show)

-- Up/Down Left/Right
data Lbl = UL | UR | DL | DR
 deriving (Eq,Ord,Show)

-- The operators in the labels
op :: Lbl -> Rational -> Rational
op UL = (+7)
op UR = (*3)
op DL = (/2)
op DR = (subtract 5)

-- Gives the possible candidates for a position
candidates :: Pos -> [(Pos,Lbl)]
candidates = \p -> case p of M -> map (L,) left ++ map (R,) right
                             L -> map (M,) left
                             R -> map (M,) right
  where right = [UR,DR]
        left  = [UL,DL]

-- The state is the value, position and the last label (cannot revisit it)
type St = (Rational,Pos,Lbl)

-- The memo map, remembers if a state has been visited before and
-- which was the previos state
type Memo = Map St St

-- Is this a solved state?
solved :: St -> Bool
solved (2012,R,_) = True
solved _          = False

-- The initial state
initSt :: St
initSt = (2011,L,DR)  -- ugly hack, say the last label was DR ;)

-- Solve, with a queue of states to visit and a memo map
solve :: Seq St -> Memo -> [St]
solve (viewl -> top@(v,p,l) :< rest) memo
  | solved top = reverse (retrievePath memo top)
  | otherwise  =
     let new = [ (st,top)
               | (p',l') <- candidates p
               -- ^ Get the candidate locations from here
               , l /= l'
               -- ^ Don't go through the same label twice
               , let st@(v',_,_) = (op l' v,p',l')
               -- ^ Calculate the new state
               -- Two heuristics:
               , not (l == DL && denominator v' /= 1)
               -- ^ 1) don't divide by two unless the denominator is 1
               , v' < 10000
               -- ^ 2) don't go above 10000
               , st `M.notMember` memo
               -- ^ Don't revisit a state
               ]
     in  solve (rest >< fromList (map fst new))
               -- ^ Enqueue the new states
               (memo `M.union` M.fromList new)
               -- ^ Add these to the memoization

-- Retrieve the path from the memo map
retrievePath :: Memo -> St -> [St]
retrievePath memo st@(v,p,l)
  | st == initSt = [initSt]
  | otherwise    = st : retrievePath memo (memo M.! st)

-- This is the problem with 2011 and 2012
problem = solve (singleton initSt) (M.singleton initSt initSt)

main = print problem


Merge sort implementation weirdly faster than system sort I wrote the following functions to merge sort an array of Comparables. It seems to work well (I am testing the output to make sure it is sorted). It takes about 2.5 sec for 10 million randomly generated strings. However, when I use the system sort by calling Arrays.sort(), it takes about 18 seconds. Why this huge discrepancy?

private static void merge(Comparable[] a, Comparable[] aux, int low, int mid, int high) {
    int i = low;
    int j = mid;
    for (int k = low; k <= high; k++) {
        aux[k] = a[i];
    }
    for (int k = low; k <= high; k++) {
        if (i < mid) {
            if (j > high) {
                a[k] = aux[i++];
            } else {
                if (aux[i].compareTo(aux[j]) <= 0) {
                    a[k] = aux[i++];
                } else if (aux[i].compareTo(aux[j]) > 0) {
                    a[k] = aux[j++];
                }
            }
        } else {
            a[k] = aux[j++];
        }
    }
}

private static void mergesort(Comparable[] a, Comparable[] aux, int start, int end) {
    int N = end - start + 1;
    if (N < 2) return;
    int lo = start;
    int mid = start + N/2;
    int hi = end;
    mergesort(a, aux, lo, mid-1);
    mergesort(a, aux, mid, hi);

    merge(a, aux, lo, mid, hi);
}

private static void msort(Comparable[] a) {
    int N = a.length;
    if (N < 2) return;
    Comparable[] aux = new Comparable[N];
    //System.arraycopy(a, 0, aux, 0, N);
    mergesort(a, aux, 0, N - 1);
}

private static boolean isSorted(Comparable[] a) {
    int N = a.length;
    if (N < 2) return true;
    for (int i = 1; i < N; i++) {
        if (a[i-1].compareTo(a[i]) > 0) {
            return false;
        }
    }
    return true;
}

private static String createRandString(int lo, int hi) {
    int len = myrandgen.uniform(lo, hi+1);
    StringBuilder sb = new StringBuilder();
    for (int i = 0; i < len; i++) {
        char ch = (char) myrandgen.uniform(97, 123);
        sb.append(ch);
    }
    return sb.toString();
}

private static void sortTester(int inputSize) {
    String[] s = new String[inputSize];
    for (int i = 0; i < inputSize; i++) {
        String t = createRandString(5,15);
        s[i] = t;
    }
    long start = System.currentTimeMillis();
    //Arrays.sort(s);
    msort(s);
    long end = System.currentTimeMillis();

    if (isSorted(s)) {
        System.out.println("Good Sort.");
        System.out.printf("Time taken: %d for %d elements.\n", end - start, inputSize);
    } else {
        System.out.println("Bad Sort.");
    }
}
/**
 * Unit tests merge sort.
 */
public static void main(String[] args) {
    sortTester(10000000);
}

That's because your implementation is not correct. You have

private static void merge(Comparable[] a, Comparable[] aux, int low, int mid, int high) {
    int i = low;
    int j = mid;
    for (int k = low; k <= high; k++) {
        aux[k] = a[i];
    }
    for (int k = low; k <= high; k++) {
        ...

yet I believe you need:

private static void merge(Comparable[] a, Comparable[] aux, int low, int mid, int high) {
    int i = low;
    int j = mid;
    for (int k = low; k <= high; k++) {
        aux[k] = a[i++]; // <--- INCREMENT i
    }
    i = low; // <--- RESET i
    ...

The demonstration program is here.

The output is

Good Sort.
Time taken: 29846 for 10000000 elements.
Good.
Arrays.sort in 25226 ms.
true

Hope that helps.


Optimising an iterative function over long strings I'm doing a coding challenge for fun and to work on my skills - some of you may remember the Advent of Code challenge from last December, I'm working through that. I've got this code as the solution to one of the problems, which works, but it's uselessly slow.

inp = "1113122113"

def iterate(num):
    pos = 0
    new = ""
    while pos < len(num):
        counter = 0
        d = num[pos]
        for i in range(pos, len(num)):
            if num[i] == d:
                counter += 1
            else:
                break
        new+=str(counter)
        new+=str(d)
        pos += counter
    print len(new)
    return new

for i in range (50):
    inp = iterate(inp)

Past iteration 35 or so, it gets to the point where it's taking several minutes for each iteration. The objective is to generate a look-and-say sequence - so 1 goes to 11, then 21, 1211, 111221, 3112211 and so on. I need to find the length of the string after the 50th iteration, but it's 360154 characters long after 40 iterations and my code just isn't optimised enough to handle strings that long in a reasonable time. Can anyone give me some pointers?

So let's look at what the basic idea of the sequence is (if I understand correctly):

Break the sequence into contiguous blocks of identical values.
Figure out the lengths of those blocks.
Create a new sequence with the length of each block followed by the value of that block.

Step 1 seems to be the problem here.  The key issue is to me is that you are rolling your own solution when python provides something to do step 1 for you: itertools.groupby.

So consider your last number:

>>> from itertools import groupby
>>> inp = "1113122113"
>>> print([(k, list(g)) for k, g in groupby(inp)])
[('1', ['1', '1', '1']), ('3', ['3']), ('1', ['1']), ('2', ['2', '2']), ('1', ['1', '1']), ('3', ['3'])]

This gives you an iterator of tuples, where the first tuple is the value of the group and the second is the group itself.  You can use the value of the group for second element of each number pair and length of the group as the first element.  Then you just have to put them together into a new string.

So the algorithm can be reduced to:

new = ''.join(str(sum(1 for _ in g))+k for k, g in groupby(inp))

Other comments:

Your multi-run is done in the root of the script.  It would be better to give it its own function.
You are printing in the same function that generates the sequence.  This assumes you will always want to print.  Better to just return the results and let whatever calls the function determine what to do with those results.
You should put all the automatically-run code in if __name__ = '__main__': so you can use the functions elsewhere.
You should use from __future__ import print_function to make your code python3 compatible.
Use _ in place of i for loop indices you don't care about.
You can use default function arguments to make the code a bit simpler to run

So here is my complete solution:

from __future__ import print_function

def iterate(num):
    return ''.join(str(sum(1 for _ in g))+k for k, g in groupby(num))

def multiiter(n, inp='1'):
    for _ in range(n):
        print(inp, len(inp))
        inp = iterate(inp)

if __name__ == '__main__':
    multiiter(n=50, inp='1113122113')

Using a version that commented out the print took 5.53 seconds to run 50 answers.

Edit: Included Copperfield's sum suggestion.


Vowels in a string, patterns Checking if all five vowels are present in a string.

String phrase = "Hello, how are you keeping?";
String vowelsInPhrase = phrase.toLowerCase().replaceAll("[^aeiou]", "");
String temp = vowelsInPhrase;
String usedVowels = "";
    while(temp.length() > 0){
        usedVowels += temp.substring(0, 1);
        temp = temp.replaceAll(temp.substring(0, 1), "");
    }
    if(usedVowels.length() == 5){
        result += "At least one occurrence of each vowel.\n";
    }

Does anyone have a better solution?? I am thinking some sort of pattern to check if each required character has appeared one or more times but I don't know if it is possible. This would save using the while loop.

Something like this is what I was thinking:

String pattern = "[[a]+[e]+[i]+[o]+[u]+]";
if(usedVowels.matches(pattern){
        result += "At least one occurrence of each vowel.\n";
    }

Any solution to this would have to be at least O(n), because worst case you're going to have to check every character in the String.

A very simple solution to this problem is to create an array of 5 booleans, corresponding to the 5 vowels. Then iterate through the string, and if you see a vowel, mark that index (e.g. a = 0, e = 1, etc.) as true. As soon as all 5 are true, return true, as all the vowels occur (assuming that you don't care if they occur more than once).

That has the same time complexity as your regex solution and is far simpler to implement/read/maintain.

You could also sort the string and then binary search for each of the required characters. This has a higher upfront cost (nlogn for the sort, logn for each binary search), but if you're making multiple queries on the same string, might be more efficient in the long run.


Image Carousel in JavaScript I created a very basic "image projector" that shows the current one and shows the next on clicking into the image.

And yeah, it works, but I don't think that's a very elegant way to do it. Any advice please?

Bear in mind I'm just starting my JavaScript classes and that's my third assignment, but the teacher is not really helpful when it comes to clean code, for him if it works, it works :/

Anyway, here's the full code: http://jsfiddle.net/AngelCJ/F5Hg7/

And the messy part:

/* loads the first image of the ul element using the n counter */
window.onload = function loadFirst() {
    var n=0;
    document.getElementById("list").children[n].style.display="block";
    /* onclick event, add +1 to the counter */
    document.getElementById("btn").onclick = function slider(){
        n++;
        /* and display the corresponding image */
        document.getElementById("list").children[n].style.display="block";
        /* while deleteing the previous one */
        document.getElementById("list").children[n-1].style.display="none";
        /* if we reach the final image */
        if(n==3) {
            /* shows the final image */
            document.getElementById("list").children[3].style.display="block";
            /* and after we click on it */
            document.getElementById("btn").onclick = function slider(){
                /* deletes it to leave the space to the first one */
                document.getElementById("list").children[3].style.display="none";
                /* returning to it loading the function to start again */
                loadFirst();
            }
            return;
        }
    }
}

In your example you're adding events inside events, this is not good idea because it creates unnecessary closures and can lead to memory leaks.

First I would cache the elements so you only query the DOM once, improving performance. Then you can create a function to hide all images, and a function to go to the next image, and reuse that function for both events:

// IIFE (Immediately invoked function expression)
// to not leak variables to global scope
(function(){
    // Cache elements
    var list = document.getElementById('list');
    var btn = document.getElementById('btn');

    var current = 0;

    // Hide all images
    function hide() {
        for (var i=0; i<list.children.length; i++) {
            list.children[i].style.display = 'none';
        }
    }

    // Go to next image
    function next() {
        hide();
        list.children[current++].style.display = 'block';
        // Did we reach the end?
        if (current >= list.children.length) {
            current = 0;
        }
    }

    // Reuse function
    window.onload = btn.onclick = next;
}());

Demo: http://jsfiddle.net/F5Hg7/2/


Small sitemap checker .aspx file I wrote a small sitemap-checker.aspx, the goal of this code is to find the pages, that respond with the codes other than 200. I've only put the C# part here for the review:
    void Start_Click(object sender, EventArgs e)
    {
        CheckUrls(UrlTextBox.Text);
    }

    private void CheckUrls(string url)
    {
        try
        {
            var xmlDoc = LoadXml(url);
            var urlNodes = xmlDoc.GetElementsByTagName("url");
            var numberOfNodes = urlNodes.Count;
            var index = 0;

            Log("### Total nodes: " + numberOfNodes+ "
");
            Log("Here is the list of URLs, with the status code other than 200:
");
            foreach (XmlNode urlNode in urlNodes)
            {
                CheckUrlNode(urlNode, xmlDoc);
                index++;

                if(index % 10 == 0)
                {
                    Log("" + index + " nodes of " + numberOfNodes + " are finished, please wait...");
                }
            };
            Log("### Sitemap check finished. Please analyze the reported urls

");
        }
        catch (Exception e)
        {
            Log("Some problems were encountered while checking the sitemap
");
            Log(e.Message + "
");
            Log(e.StackTrace + "
");
        }
    }

    private XmlDocument LoadXml(string url)
    {
        var m_strFilePath = url;
        string xmlStr;
        using (var wc = new WebClient())
        {
            xmlStr = wc.DownloadString(m_strFilePath);
        }
        var xmlDoc = new XmlDocument();
        xmlDoc.LoadXml(xmlStr);
        return xmlDoc;
    }

    private void CheckUrlNode(XmlNode xmlNode, XmlDocument xmlDoc)
    {
        var urlList = new List<string>() { xmlNode["loc"].InnerText };

        var nsmgr = new XmlNamespaceManager(xmlDoc.NameTable);
        nsmgr.AddNamespace("xhtml", "http://www.w3.org/1999/xhtml");

        var hrefLangUrls = xmlNode
            .SelectNodes("xhtml:link", nsmgr)
            .Cast<XmlNode>()
            .Select(x => x.Attributes["href"].Value);

        urlList.AddRange(hrefLangUrls);

        foreach (var url in urlList)
        {
            string logMessage;
            if (!CheckUrl(url, out logMessage))
            {
                Log(logMessage);
            }
        }
    }

    private bool CheckUrl(string url, out string logMessage)
    {
        logMessage = null;
        try
        {
            var request = HttpWebRequest.Create(url) as HttpWebRequest;
            request.Timeout = 5000; //set the timeout to 5 seconds to keep the user from waiting too long for the page to load
            request.Method = "HEAD"; //Get only the header information -- no need to download any content

            using (var response = request.GetResponse() as HttpWebResponse)
            {
                int statusCode = (int)response.StatusCode;
                if (statusCode == 200) //Good requests
                {
                    return true;
                }
                else
                {
                    logMessage = String.Format("{0}, status code: {1}", url, statusCode);
                    return false;
                }
            }
        }
        catch (WebException ex)
        {
            logMessage = String.Format("{0}, status: {1}", url, ex.Status);
        }
        catch (Exception ex)
        {
            logMessage = String.Format("Could not test url {0}, exeption message: {1}", url, ex.Message);
        }

        return false;
    }

I think you can achieve the same with less code by utilizing some libraries and language features.
Google's Sitemap Protocol is not supported by default in the .NET Framework / Core.
But there are couple of packages which defines the corresponding C# classes, for example:

Google-Sitemap
X.Web.Sitemap

I have chosen the second one. So, the parsing logic would look like this:
public static async Task<XDocument> LoadSitemap(string filePath = "./sitemap.xml")
{
    var sitemap = new FileStream(filePath, FileMode.Open, FileAccess.Read, FileShare.None);
    return await XDocument.LoadAsync(sitemap, LoadOptions.None, CancellationToken.None);
}

public static Sitemap ParseSitemap(XDocument document)
{
    var serializer = new XmlSerializer(typeof(Sitemap));
    return (Sitemap) serializer.Deserialize(document.CreateReader());
}

The HEAD retrieval could be done by the following helper method:
private static readonly HttpClient client = new HttpClient();

private static async Task<string> GetHead(string location,
    Func<HttpResponseMessage, string> successHandler,
    Func<Exception, string, string> failureHandler)
{
    try
    {
        var timeoutPolicy = new CancellationTokenSource(5000);
        var request = new HttpRequestMessage(HttpMethod.Head, location);
        var response = await client.SendAsync(request, timeoutPolicy.Token);
        return successHandler(response);
    }
    catch (Exception ex)
    {
        return failureHandler(ex, location);
    }
}

The success and failure handlers can be implemented like this:
private static string HandleResponse(HttpResponseMessage response)
{
    var url = response.RequestMessage.RequestUri;
    return response.StatusCode == HttpStatusCode.OK
        ? $"{url} - OK"
        : $"{url} - Status code: {response.StatusCode}";
}

private static string HandleError(Exception exception, string url)
{
    return exception switch
    {
        HttpRequestException hre => $"{url} - unreachable due to {hre.Message}",
        TaskCanceledException tce => $"{url} - timed out due to {tce.Message}",
        _ => $"{url} - failed due to {exception}"
    };
}

Let's put all these pieces together:
static async Task Main(string[] args)
{
    var siteMapXml = await LoadSitemap();
    var siteMap = ParseSitemap(siteMapXml);

    var siteChecks = siteMap
        .Select(url => GetHead(url.Location, HandleResponse, HandleError))
        .ToList();

    await Task.WhenAll(siteChecks);

    foreach (var siteCheck in siteChecks)
    {
        Console.WriteLine(await siteCheck);
    }

    Console.WriteLine("Finished");
}


Retrieving the first occurrence of every unique value from a CSV column A large .csv file I was given has a large table of flight data. A function I wrote to help parse it iterates over the column of Flight IDs, and then returns a dictionary containing the index and value of every unique Flight ID in order of first appearance.

Dictionary = { Index: FID, ... }

This comes as a quick adjustment to an older function that didn't require having to worry about FID repeats in the column (a few hundred thousand rows later...).

Example:

20110117559515, ...
20110117559515, ...
20110117559515, ...
20110117559572, ...
20110117559572, ...
20110117559572, ...
20110117559574, ...
20110117559587, ...
20110117559588, ...

and so on for 5.3 million some rows.

Right now, I have it iterating over and comparing each value in order. If a unique value appears, it only stores the first occurrence in the dictionary. I changed it to now also check if that value has already occurred before, and if so, to skip it.

def DiscoverEarliestIndex(self, number):
    result = {}
    columnvalues = self.column(number)
    column_enum = {}
    for a, b in enumerate(columnvalues):
        column_enum[a] = b
        i = 0
    while i < (len(columnvalues) - 1):
        next = column_enum[i+1]
        if columnvalues[i] == next:
            i += 1
        else:
            if next in result.values():
                i += 1
                continue
            else:
                result[i+1]= next
                i += 1
    else:
        return result

It's very inefficient, and slows down as the dictionary grows. The column has 5.2 million rows, so it's obviously not a good idea to handle this much with Python, but I'm stuck with it for now.

Is there a more efficient way to write this function?

You've given us a small piece of your problem to review, and Janne's suggestion seems reasonable if that piece is considered on its own. But I have the feeling that this isn't the only bit of analysis that you are doing on your data, and if so, you probably want to think about using a proper database.

Python comes with a built-in relational database engine in the form of the sqlite3 module. So you could easily read your CSV directly into a SQLite table, either using the .import command in SQLite's command-line shell, or via Python if you need more preprocessing:

import sqlite3
import csv

def load_flight_csv(db_filename, csv_filename):
    """
    Load flight data from `csv_filename` into the SQLite database in
    `db_filename`.
    """
    with sqlite3.connect(db_filename) as conn, open(csv_filename, 'rb') as f:
        c = conn.cursor()
        c.execute('''CREATE TABLE IF NOT EXISTS flight
                     (id INTEGER PRIMARY KEY AUTOINCREMENT, fid TEXT)''')
        c.execute('''CREATE INDEX IF NOT EXISTS flight_fid ON flight (fid)''')
        c.executemany('''INSERT INTO flight (fid) VALUES (?)''', csv.reader(f))
        conn.commit()

(Obviously you'd need more fields in the CREATE TABLE statement, but since you didn't show them in your question, I can't guess what they might be.)

And then you can analyze the data by issuing SQL queries:

>>> db = 'flight.db'
>>> load_flight_csv(db, 'flight.csv')
>>> conn = sqlite3.connect(db)
>>> from pprint import pprint
>>> pprint(conn.execute('''SELECT MIN(id), fid FROM flight GROUP BY fid''').fetchall())
[(1, u'20110117559515'),
 (4, u'20110117559572'),
 (7, u'20110117559574'),
 (8, u'20110117559587'),
 (9, u'20110117559588')]


SQL message logger run loop I am learning Java by reading source code of other authors. This Java method handles communication with SQL database. While the code does work several things concern me. For instance the try  catch block at the end of the file used as a workaround. What is the correct way of doing it? What other problems can you spot?

    public void run() {
        if (running) {
            return;
        }
        running = true;
        while(null == Common.server || null == Common.database || !ConnectionsPool.isInitialized()) {
            // Wait until the database is set before continuing...
            try {
                Thread.sleep(1000);
            }
            catch(Exception ex) {}
        }
        while(running) {
            final Connections cons = ConnectionsPool.getConnections();
            Connection  con  = null;
            while(!entries.isEmpty()) {
                if (null == con) {
                    con = cons.getConnection();
                }
                SQLLogEntry entry = entries.remove();
                if (null != entry) {
                    try {
                        write(entry, con); //find usages
                    }
                    catch (SQLException ex) {
                        writeLogFile("Could not write entry to SQL", ex);
                    }
                }
            }
            if (null != con) {
                try {
                    con.commit();
                }
                catch (SQLException ex) {
                    writeLogFile("Could not commit to SQL", ex);
                    try {
                        con.rollback();
                    }
                    catch (SQLException ex1) {
                    }

                    // log
                    final StringWriter errorOut = new StringWriter();
                    ex.printStackTrace(new PrintWriter(errorOut));
                    EditorTransactionUtil.writeLogFile(errorOut.toString());
                    // for user
                    final String msg = "Exception: " + EditorUtil.getErrorMessage(ex.getMessage());
                    try {
                        SwingUtilities.invokeAndWait(() -> {
                            JOptionPane.showMessageDialog(null, msg);
                        });
                    }
                    catch (Throwable ex1) {
                    }
                }
                finally {
                    cons.returnConnection(con);
                }
                con = null;
            }

            synchronized(entries) {
                try {
                    entries.wait(1000);
                }
                catch (InterruptedException ex) {
                    // This is a workaround to process this loop...
                }
            }
        }
        writeLogFile("SQLMsgLogger run stopping...");
    }

As a quick first note ( I will add more later when I have time), I assume the first lines;

   if (running) {
            return;
   }
   running = true;

are attempting to ensure that this code will only be running once at a time... You would have to use a lock to ensure this as currently a race condition could cause;

1.) A thread checks running is false

2.) A second thread checks this before the first sets it to true

3.) Both threads continue

This is probably not what you want to happen....

Regarding your question about the InterruptedException, you should NOT just swallow it. I suggest reading the following article which covers the reasoning behind this: http://www.ibm.com/developerworks/java/library/j-jtp05236/index.html?ca=drs-

You also seem to swallow Exceptions else where in the code (Once the base Exception class and not a subclass). This is a very bad habbit and you should consider which exceptions could happen and what that would imply for your code, then deal with them explicitly.


JavaScript calculator with two inputs and four operations i build a calc & want to a review that what i did wrong & what is right method to do.

      Enter first number
      <input type="number" id="valueOfX">
      Enter second number
      <input type="number" id="valueOfY">
      <button onclick="operation('add')"> Add </button>
      <button onclick="operation('sub')"> Sub</button>
      <button onclick="operation('mul')"> Mult</button>
      <button onclick="operation('div')"> Divi</button>

To get the ball rolling, a few things that come to mind:
JavaScript
Storing DOM-elements
In getInputs and showOuput you use document.getElementById to get the same DOM elements over and over again. You can simply store or "cache" them in variable an re-use them anytime you want:
var inputX = document.getElementById('x'),
    inputY = document.getElementById('y'),
    output = document.getElementById('output');

showOutput which seems to have an unused value in its signature, would now look like:
var showOutput = function(value) {
    output.innerHTML = value;
}

Filter input
getInputs already uses parseInt to filter the user's input. This will still result in NaN for all inputs that can't be parsed to integer, like "hey".
You should test for this scenario and give the user a hint that something failed:
var operation = function(operationName) {
    var x = getInputs(inputX),
        y = getInputs(inputY);

    if (isNaN(x) || isNaN(y)) {
        // show an error message or give the inputs a red border etc.
        return;
    }

    // […]
};

By the way, your calculator doesn't make it clear, that it can only process integer values, which might lead to confusing results with inputs like this:

1.5 + 2.5 = 3

Improve the UX and leave the visitor a note.
manuplateAs
I think you meant to call the function manipulate. But why don't you simply call it calculate?
You can also use a switch-statement and shorter variable names to increase readability:
var calculate = function(operation, x, y) {
    switch(operation) {
        case 'add': return x + y;
        case 'sub': return x - y;
        case 'mul': return x * y;
        case 'div': return x / y;
    };
}

Again your calculator doesn't handle edge cases. For example it will show Infinity for division by 0 or NaN if numbers get to big.
You could test the result for NaN or Infinity and show a proper message to the user:
var result = calculate('div', x, y);

if (!isFinite(result)) {
    // show an error message
    return;
}

HTML
<head>
Event though it's a very simply example, add these two things in your header:
<meta charset="UTF-8">
<title>Simple Calculator</title>

It makes sure that:

The correct character set is chosen.
The user can find the tab with the calculator more easily.

Form
You're using form elements that are not descendants of a form element, nor do you link them to a form using the form-attribute. That is invalid HTML. Simply wrap everything into a form:
<form>
    <input type="number" id="x">
    <input type="number" id="y">
</form>

A button is by default a submit button. You can change this by setting the type-attribute:
<button type="button" onclick="operation('add')">Add</button>

To further improve UX create labels to couple your text with a corresponding input. This also makes sure, that the input is focused as soon as a user clicks the label's text:
<label>
    First number
    <input type="number" id="x">
</label>

Or alternatively:
<label for="x">First number</label>
<input type="number" id="x">

Personally I wouldn't use a h1-element to output the result. Maybe a disabled input field works here, too.


Creating method to use one foreach instead multiple I'm working with C#, and I have working code where I repeated same code every line, and that's because I'm creating list, conversions, extracting data, etc.

I have multiple foreach clauses, multiple lists, multiple conversions of list to datatables. My question is, what can I do to refactor this in order to have clean code?

 private void BtnLoadReport_Click(object sender, EventArgs e)
        {
            var db = new SQLDataMgr();

            List<string> DesignStatusList = new List<string>();
            List<string> ShopStatusList = new List<string>();
            List<string> CustomerTypeList = new List<string>();
            List<string> CustomerList = new List<string>();
            List<string> ResellerList = new List<string>();
            List<string> StateList = new List<string>();
            List<string> ProjectManagerList = new List<string>();
            List<string> SalesRepresentativeList = new List<string>();

            var checkedDesignStatus = cboDesignStatus.CheckBoxItems.Where(x => x.Checked);
            var checkedShopStatus = cboShopStatus.CheckBoxItems.Where(x => x.Checked);
            var checkedCustomerType = cboShopStatus.CheckBoxItems.Where(x => x.Checked);
            var checkedCustomer = cboShopStatus.CheckBoxItems.Where(x => x.Checked);
            var checkedReseller = cboShopStatus.CheckBoxItems.Where(x => x.Checked);
            var checkedState = cboShopStatus.CheckBoxItems.Where(x => x.Checked);
            var checkedProjectManager = cboShopStatus.CheckBoxItems.Where(x => x.Checked);
            var checkedSalesRepresentative = cboShopStatus.CheckBoxItems.Where(x => x.Checked);

            foreach (var i in checkedDesignStatus)
            {
                DesignStatusList.Add(i.Text);
            }
            foreach (var i in checkedShopStatus)
            {
                ShopStatusList.Add(i.Text);
            }
            foreach (var i in checkedCustomerType)
            {
                CustomerTypeList.Add(i.Text);
            }

            foreach (var i in checkedCustomer)
            {
                CustomerList.Add(i.Text);
            }
            foreach (var i in checkedReseller)
            {
                ResellerList.Add(i.Text);
            }
            foreach (var i in checkedState)
            {
                StateList.Add(i.Text);
            }
            foreach (var i in checkedProjectManager)
            {
                ProjectManagerList.Add(i.Text);
            }
            foreach (var i in checkedSalesRepresentative)
            {
                SalesRepresentativeList.Add(i.Text);
            }
            DataTable designStatusParameters = ToStringDataTable(DesignStatusList);
            DataTable shopStatusParameters = ToStringDataTable(ShopStatusList);
            DataTable customerTypeParameters = ToStringDataTable(CustomerTypeList);
            DataTable customerParameters = ToStringDataTable(CustomerList);
            DataTable resellerParameters = ToStringDataTable(ResellerList);
            DataTable stateParameters = ToStringDataTable(StateList);
            DataTable projectManagerParameters = ToStringDataTable(ProjectManagerList);
            DataTable salesRepresentativerParameters = ToStringDataTable(SalesRepresentativeList);

        }

First step is to look at the repeated code then copy an single into a method.
In this case a could method name would seem to be CreateDataTable.

What are the commonalities?

A ComboBox that has checked items.
Get an IQueryable or IEnumerable depending on which is returned by the where clause
Creates a list of the text value from each checked item
Creates a DataTable using ToStringDataTable(parameters

Since the type in and type out is all the same, this makes it much easier to refactor out into a method. The question then is what input and what output?

Input = ComboBox, output = `DataTable'. This give the method signature needed:

DataTable CreateDataTable(ComboBox cbo) {
}

Now put in those lines that are repeated and change the *Status to cbo and instead of assigning the DataTable just return the result of ToStringDataTable(parameters).

DataTable CreateDataTable(ComboBox cbo) {

    var checkedItems = cbo.CheckBoxItems.Where(x => x.Checked);
    List<string> parameterList = new List<string>();
    foreach (var i in checkedItms) {
        parameter.Add(i.Text);
    }

    return ToStringDataTable(parameters);
}

Now clean up the original method:

public class Refactor {
  private void BtnLoadReport_Click(object sender, EventArgs e)
    {
        var db = new SQLDataMgr();

        var designStatusParameters = CreateDataTable(cboDesignStatus);
        var shopStatusParameters = CreateDataTable(cboShopStatus);
        var customerTypeParameters = CreateDataTable(cboCustomerType);
        var customerParameters = CreateDataTable(cboCustomer);
        var resellerParameters = CreateDataTable(cboReseller);
        var stateParameters = CreateDataTable(cboState);
        var projectManagerParameters = CreateDataTable(cboProjectManager);
        var salesRepresentativeParameters = CreateDataTable(cboSalesRepresentative);
        //Rest of the method
    }
}

This could be further improved by using some additional LINQ chains if the items returned by the where clause supports them.

Example:

List<String> parameters = cbo.CheckBoxItems.Where(x => x.Checked).Select(x => x.Text).ToList();

which would simplifying the method even further:

DataTable CreateDataTable(ComboBox cbo) {

    List<string> parameterList = List<String> parameters = cbo.CheckBoxItems.Where(x => x.Checked).Select(x => x.Text).ToList();

    return ToStringDataTable(parameters);
}


FoldLeft in Terms of FoldRight I [believe that I] wrote foldl in terms of foldr:

myFoldl :: (a -> b -> a) -> a -> [b] -> a
myFoldl f base xs = foldr (flip f) base (reverse xs)

Is this an idiomatic implementation?

Yes, I believe it's equivalent to the third duality theorem which states that for any finite list xs,

foldr op u xs = foldl (flip op) u (reverse xs)

Without appealing to the theorem, we can still prove your definition correct for any finite list xs. We'll need  definitions of foldl and foldr:

Definition

(1) foldr _ u [] = u
(2) foldr op u (x:xs) = op x (foldr op u xs)

Definition

(3) foldl _ u [] = u
(4) foldl op u (x:xs) = foldl op (op u x) xs

And the following:

Lemma For finite lists xs, ys, foldr op u (ys ++ xs) = foldr op (foldr op u xs) ys.

Case []

RHS = foldr op (foldr op u xs) []
    = foldr op u xs -- by (1)
    = foldr op u ([] ++ xs)
    = LHS

Case y:ys

RHS = foldr op (foldr op u xs) (y:ys)
    = op y (foldr op (foldr op u xs) ys) -- by (2)
    = op y (foldr op u (ys ++ xs)) -- by induction hypothesis
    = foldr op u (y:ys ++ xs) -- by (2)
    = LHS

∎

Claim For any finite list xs, foldr (flip op) u (reverse xs) = foldl op u xs.

Case []

LHS = foldr (flip op) u (reverse [])
    = foldr (flip op) u []
    = [] -- by (1)
    = foldl op u [] -- by (3)
    = RHS

Case x:xs

LHS = foldr (flip op) u (reverse (x:xs))
    = foldr (flip op) u ((reverse xs) ++ [x])
    = foldr (flip op) (foldr (flip op) u [x]) (reverse xs) -- by the lemma
    = foldr (flip op) ((flip op) x (foldr (flip op) u [])) (reverse xs) -- by (2)
    = foldr (flip op) ((flip op) x u) (reverse xs) -- by (1)
    = foldl op ((flip op) x u) xs -- by induction hypothesis
    = foldl op (op u x) xs -- by definition of flip
    = foldl op u (x:xs) -- by (4)
    = RHS

∎