Loading module to a templated page I am trying to create a web app (CMS) as part of my research project, and I use the code below to load modules into a page.  Please review this code.

public function LoadModule($module,$position){

    if(file_exists('modules/'.$module.'.tpl'))
    {
        //todo: might have to get a better thing for this !
        $mod = 'modules/'.$module.'.tpl';
        ob_start();
        include($mod);
        $mod = ob_get_clean();

        $this->html = str_replace('__{'.$position.'}__', $mod ,$this->html);
    }
    else{
        if(function_exists('LOGIT')){

            LOGIT('$module - module not found.');
        }
        else {
            $this->html = str_replace('__{'.$position.'}__', "$module Modlule not found" ,$this->html);
        }
    }
}

I don't know if it's a paste-issue, but you should intent your code ;)

You are building the file-path twice. You should move that line:

$mod = 'modules/'.$module.'.tpl';

To the begining of the function (and give it a better name):

public function LoadModule($module,$position)
{

    $absTemplatePath = 'modules/'.$module.'.tpl';
    if(file_exists($absTemplatePath ))
    {

That Line:

LOGIT('$module - module not found.');

should be (else $module wouldn't get replaced):

LOGIT("$module - module not found");

If the template file does not include php-code you should use file_get_contents instead of ob_start, include, ob_get_clean (performance-wise). If you do have php-code in your templates, you might have a look at zend-framework's Zend_View.


Improved stack implementation using a linked list This is a followup to

Simple stack implementation using linked list

Since my original stack implementation has lots of bugs that I did not notice, I thought I would recreate and try to improve it. Please review this version.

Note about pointers: I know using std::unique_ptr is better so we don't need to do manual deletes, but when I tried it here it results in disaster. I think I don't yet know how to really use it. So I did not use it in this code.

template<class T>
struct Node {
    T item;
    Node* next;
    Node(const T& t, Node* link) :item{t}, next{link} { }
};

template<class T>
class Stack {
public:
    Stack() : first{nullptr}, n{0} {}
    int size() const { return n; }
    bool empty() const { return n == 0; }
    Stack(const Stack&);
    Stack(Stack&&);
    Stack& operator=(const Stack&);
    Stack& operator=(Stack&&);
    void push(const T&);
    void pop();
    T peek() const;
    ~Stack() {
        while (!empty()) {
            pop();
        }
    }
private:
    Node<T>* first;
    std::size_t n;
};

template<class T>
Stack<T>::Stack(const Stack& s) :first{nullptr}, n{0}{
    for (auto t = s.first; t != nullptr; t = t->next) {
        push(t->item);
    }
}

template<class T>
Stack<T>& Stack<T>::operator=(const Stack& s) {
    for (auto t = s.first; t != nullptr; t = t->next) {
        push(t->item);
    }
    return *this;
}

template<class T>
Stack<T>::Stack(Stack&& s) :first{s.first}, n{s.n} {
    s.first = nullptr;
    s.n = 0;
}

template<class T>
Stack<T>& Stack<T>::operator=(Stack&& s) {
    first = s.first;
    n = s.n;
    s.first = nullptr;
    s.n = 0;
    return *this;
}

template<class T>
void Stack<T>::push(const T& t) {
    first = new Node<T>{t,first};
    ++n;
}

template<class T>
void Stack<T>::pop() {
    if (empty()) {
        throw std::out_of_range("underflow");
    }
    Node<T>* oldfirst = first;
    first = first->next;
    delete oldfirst;
    --n;
}

template<class T>
T Stack<T>::peek() const {
    if (empty()) {
        throw std::out_of_range("underflow");
    }
    return first->item;
}

How can I make this code better, particularly copy and move?

Don't repeat code:

template<class T>
Stack<T>& Stack<T>::operator=(const Stack& s) {
    for (auto t = s.first; t != nullptr; t = t->next) {
        push(t->item);
    }
    return *this;
}

This is not really an assignment but an append. I think most people would not expect that. You should probably release the original list before copying over the source into this object.

An easier way to implement the assignment operator is to use the copy and swap idiom.

// Notice the pass by value creates an implicit copy.
// You then just swap the content of the current object
// with the parameter s.
// When the parameter goes out of scope it deletes the old
// data stack.
Stack<T>& Stack<T>::operator=(Stack s) {
    std::swap(first,   s.first);
    std::swap(n,       s.n);
    return *this;
}

You move assignment leaks:

template<class T>
Stack<T>& Stack<T>::operator=(Stack&& s) {
    first = s.first;
    n = s.n;
    s.first = nullptr;
    s.n = 0;
    return *this;
}

The current value of first is leaked. The best way to do this is to swap the content of this object with the incomming object.

template<class T>
Stack<T>& Stack<T>::operator=(Stack&& s) {
    std::swap(first,   s.first);
    std::swap(n,       s.n);
    return *this;
}

Let the other object do the clean up when it goes out of scope.

You implemented a push by copy:

void push(const T&);

Why not have a push by move:

void push(T&&);

I would make the Node structure a private implementation detail of stack. That way other people can not rely on that detail when they use your code.

template<class T>
class Stack {
     struct Node {....}
};

Your peak creates a copy of the object.

   T Stack<T>::peek() const {
// ^ Return by value creates a copy.

I would return by reference. I would also rovide two version. One to return by const reference (for const version of the stack), and a normal return by reference so you can look at the top value and potentially alter it without removing it from the stack:

T const& Stack<T>::peek() const { ....
T&       Stack<T>::peek()       { ....

Your destructor checks empty() then calls pop() which checks empty(). This is a bit redundant. You could have a private version of pop that does not check empty.

~Stack() {
    while (!empty()) {
        noneCheckedPop();
    }

void Stack<T>::pop() {
    if (empty()) {
        throw std::out_of_range("underflow");
    }
    noneCheckedPop();
}

private:
void Stack<T>::noneCheckedPop() {
    Node<T>* oldfirst = first;
    first = first->next;
    delete oldfirst;
    --n;
}


Adding items to an array in Javascript I am working on a JavaScript exercise. I am running into issues with arrays.

NOTE: I posted this code in this forum because the code works. But, I would appreciate people reviewing it, and letting me know there are better ways to do this.

I am creating two arrays.

I am putting items into the first array. These are the separate parts of a string that represent a time: hours:minutes:seconds:milliseconds. You will see the times in the input string in my code.

I am putting those time segments into the first array, and then I am pushing that array into the second array (muti dimensional). I am doing this because I don't know how many times will be in an input string. I am also doing this because I need to eventually add up these times in the input string, and then add another set of time segments from a second input string that I did not include here in this code. (I realize that I have to convert these to numbers eventually.)

I tried the code below without the commented out line:

// arrSubtitlesSeparate = [];

I don't get what I expected.
When I include this line of code, I get the multi dimensional array I want.

Here is the results of the console logs. You will see them if you run the code.

You can see that the times are being found with the regex. But, the multi dimensional array is being filled with the last set of time segments that is found. You can easily see this just by looking at the last three digits which are the milliseconds in the time strings that are found.

found time: 00:13:37,243

arrSubtitlesSepMultiDim: 00,13,37,243

found time: 00:13:39,744

arrSubtitlesSepMultiDim: 00,13,39,744,00,13,39,744

found time: 00:13:39,779

arrSubtitlesSepMultiDim: 00,13,39,779,00,13,39,779,00,13,39,779

found time: 00:13:43,548

arrSubtitlesSepMultiDim: 00,13,43,548,00,13,43,548,00,13,43,548,00,13,43,548

If I add/uncomment that line of code, I get what I want.
The times are all added to the multi dimensional array.

My questions about coding this:

What is the proper way of creating the multidimensional array in this case?

Why is this happening when I don't include that line of code?

Does this have something to do with the fact that the first set of time segment values found in the input string are the first set of values being put into the array?

Would I be better off initializing the sub array with the first set of values, and then using the splice method to replace the values?

Here is my code. You should be able to copy/paste into an HTML doc.

    <script>

var subtitles = "188 00:13:37,243 --> 00:13:39,744 30 minutes. Everyone's ready. 189 00:13:39,779 --> 00:13:43,548 ♪ ";
var arrSubtitles = subtitles.split(" ");

console.log("arrSubtitles: " + arrSubtitles);

var arrSubtitlesSeparate = [];
var arrSubtitlesSepMultiDim = [];

for(var i = 0; i < arrSubtitles.length; i++){
    if(/[0-9]{2}:[0-5][0-9]:[0-5][0-9],[0-9]{3}/.test(arrSubtitles[i]) === true){
        console.log("found time: " + arrSubtitles[i]);
    arrSubtitlesSeparate[0] = arrSubtitles[i].charAt(0) + arrSubtitles[i].charAt(1);
    arrSubtitlesSeparate[1] = arrSubtitles[i].charAt(3) + arrSubtitles[i].charAt(4);
    arrSubtitlesSeparate[2] = arrSubtitles[i].charAt(6) + arrSubtitles[i].charAt(7);
    arrSubtitlesSeparate[3] = arrSubtitles[i].charAt(9) + arrSubtitles[i].charAt(10) + arrSubtitles[i].charAt(11);
    arrSubtitlesSepMultiDim.push(arrSubtitlesSeparate);
    // arrSubtitlesSeparate = [];
    console.log("arrSubtitlesSepMultiDim: " + arrSubtitlesSepMultiDim);
    }
}

</script>

First of all, when presenting a piece of code to fellow developers, you help them the most by letting them know what your code does and what the inputs and expected outputs are. You immediately start by describing how your code does something, which is a bit confusing.

Regarding your code, I suggest the following:

Both arrSubtitles and subtitles are arrays, but only one has the 'arr' prefix which is confusing. I find it more helpful to name variables according to their role instead of their type. You are splitting subtitles into some arbitrary whitespace delimited tokens, so how about tokens instead of arrSubtitles?
arrSubtitlesSeparate and arrSubtitlesSepMultiDim are bulky, nondescript names. Following above advice, how about time and times?
There is no need to perform a strict boolean comparison === true within your if-clause, your code looks neater by simply omitting that.
Within the loop body, you access arrSubtitles[i] 10 times. To avoid those many lookups and improve readability, you could assign that array element to a temporary variable. Or you use a for ... of loop instead, which iterates over array values instead of array indices.
Replace successive string.charAt concatenations with a single call to string.substr.
Leverage the block scope of const and let instead of using the function scoped var.

Applying those suggestions to your code would result in e.g.

const regex = /[0-9]{2}:[0-5][0-9]:[0-5][0-9],[0-9]{3}/;
const times = [];

for (const token of subtitles.split(" ")) {
  if (regex.test(token)) {
    const time = [
      token.substr(0, 2),
      token.substr(3, 2),
      token.substr(6, 2),
      token.substr(9, 3)
    ];
    times.push(time);
  }
}
console.log(times);

You asked what happens when you don't initialize arrSubtitlesSeparate = [] in the loop body?

Initially you create a single array instance arrSubtitlesSeparate. Then, during each loop iteration, you modify the content of this array and push it onto arrSubtitlesSepMultiDim. So in the end, each index of arrSubtitlesSepMultiDim contains a reference to the same arrSubtitlesSeparate array, similar to this:

arrSubtitlesSepMultiDim = [arrSubtitlesSeparate, arrSubtitlesSeparate, ...];

See also Is JavaScript a pass-by-reference or pass-by-value language?

Now, if we look again at your implementation, we note that you already perform a regex test. We could perform a regex matching instead and get rid of all those redundant string.substr calls. I wrapped this new implementation in a documented function and ended up with:

/**
 * Find and parse all timestamps of type "hours:minutes:seconds,milliseconds"
 * within a given subtitles string.
 * @param {string} subtitles
 * @return {Array} Array of times [hours, minutes, seconds, milliseconds]
 */
function findTimes(subtitles) {
  const regex = /([0-9]{2}):([0-5][0-9]):([0-5][0-9]),([0-9]{3})/g;
  const times = [];

  let matches;
  while (matches = regex.exec(subtitles)) {
    times.push(matches.slice(1));
  }
  return times;
}


Large dataset with pyspark - optimizing join, sort, compare between rows and group by with aggregation I have a csv file with more than 700,000,000 records in this structure:

product_id      start_date       end_date
1               19-Jan-2000      20-Mar-2000
1               20-Mar-2000      25-Apr-2000
1               20-May-2000      27-Jul-2000
1               27-Jul-2000
2               20-Mar-2000      25-Apr-2000
3               12-Jan-2010      30-Mar-2010
3               30-Mar-2010

End_date is null means product currently is in used.

End_date can mean 2 things, 1 - disable product, 2 - battery replace

If End_date is the same as the next start_date, then it is battery replacement.

The expect result is, product_id along with the start_date of its current lifecycle (battery replace is counted in current lifecycle).

Which mean, the start_date should be the date after its last disability. For example above, output would be:

product_id      start_date
1               20-May-2000
3               12-Jan-2010

My code is as below. It's kind of ugly, so if you could please review and advise if this code can run well with 700,000,000 records or there are better ways/methods to solve this challenge. I have run this code and seem a little bit slow for 100 records test file.Thank you for your help.

Code:

# csv input
df = spark.read.csv('productlist.csv', header=True, inferSchema=True)

# filter out stopped product id
df2 = df.select("product_id").filter("end_date is null")
df = df.join(df2, ["product_id"])

# sort dataframe by product id & start date desc
df = df.sort(['product_id', 'start_date'],ascending=False)

# create window to add next start date of the product
w = Window.partitionBy("product_id").orderBy(desc("product_id"))
df = df.withColumn("next_time", F.lag(df.start_date).over(w))

# add column to classify if the change of the current record is product disability or battery change.
df = df.withColumn('diff', F.when(F.isnull(df.end_date), 0)
                  .otherwise(F.when((df.end_date != df.next_start_date), 1).otherwise(0)))

# add column to classify if the product has been disabled at least once or not
df3 = df.groupBy('product_id').agg(F.sum("diff").alias("disable"))
df = df.join(df3, ["product_id"])

# get requested start date for those products have not been disabled
df1 = df.filter(df.disable == 0).groupBy("product_id").agg(F.min("start_date").alias("first_start_date"))

# get requested date for those products have been disabled once,
# which is the first next start date at the most recent disable date
df2 = df.filter(df.diff == 1).groupBy("product_id").agg(F.max("next_start_date").alias("first_start_date"))

I believe the below solution should solve for what you are looking to do in a more efficient way. Your current method involves a lot of "shuffle" operations (group by, sorting, joining). The below should help reduce the number of shuffle operation in your Spark job.

get leading start date
get disabled records
add column indicating whether product ever disabled (max of is disabled)
capture replacement dataset
get max replacement date
create indicator for current lifecycle records
filter data for current lifecycle records.

# csv input
df = spark.read.csv('productlist.csv', header=True, inferSchema=True)
# get ordered and unordered windows
wo = Window.partitionBy("product_id").orderBy("start_date")
wu = Window.partitionBy("product_id")
df1 = df.withColumn("lead_start_date", F.lead(col("start_date"), 1).over(wo))\
        .withColumn("is_disabled", F.when((col("end_date").isNotNull()) &
                                          ((col("end_date") != col("lead_start_date")) | (col("lead_start_date").isNull())), 1).otherwise(0))\
        .withColumn("has_been_disabled", F.max(col("is_disabled")).over(wu))\
        .withColumn("replacement_date", F.when((col("end_date").isNotNull()) &
                                          (col("end_date") == col("lead_start_date")) & (col("lead_start_date").isNotNull()), col("start_date")).otherwise(lit(None)))\
        .withColumn("max_replacement_date", F.max(col("replacement_date")).over(wu))\
        .withColumn("is_current_lifecycle_record", F.when(((col("replacement_date") == col("max_replacement_date")) & col("replacement_date").isNotNull()) |
                                                            ((col("has_been_disabled") == 0) & (col("max_replacement_date").isNull())), 1).otherwise(0)) # never disabled / replaced
# filter for current lifecycle record and select target columns
df_final = df1.filter(col("is_current_lifecycle_record") == 1).select(["product_id", "start_date"])


Counting the number of page turns to arrive at some specified page Here is a HackerRank challenge.  Given a book with n pages (with 1 ≤ n ≤ 105), how many page turns are needed to arrive at page p, starting from either the front or the back?  The book is laid out with page 1 on the right.

I have got following solution to the given problem.

import java.util.*;
public class Solution {

    static int solve(int n, int p){
        // Complete this function
        if(n==p)
            return 0;
        if(p%2==0){
            if(n-p==p)
                return (p-1)/2;
            else{
                if(n-p>p)
                    return p/2;
                else
                    return (n-p)/2;
            }
        }
        if(n%2==0 && p%2!=0){
            if(n-p==p)
                return (p-1)/2;
            else{
                if(n-p>p)
                    return p/2;
                else
                    return (n-p+2)/2;
                }
        }
        else

             if(n-p>p)
                    return p/2;
                else
                    return (n-p)/2;

    }
    public static void main(String[] args) {
        Scanner in = new Scanner(System.in);
        int n = in.nextInt();
        int p = in.nextInt();
        int result = solve(n, p);
        System.out.println(result);
    }
}

Although it is working code, I am not satisfied with it. It seems very messy. How can I reduce branch statements in my code, or otherwise improve it?

Math.min(p / 2, n / 2 - p / 2)

Explanation:

If we go from start page \$1\$ takes \$0\$; page \$2,3\$ takes \$1\$; page \$4,5\$ takes \$2\$ and so on. So for \$2k,2k+1\$ we require \$k\$ turns. Hence \$p=2k,2k+1\implies \lfloor p/2\rfloor = k\$
If we go from the back  it depends on:

If \$n\$ is odd then \$n,n-1\$ take \$0\$; \$n-2,n-3\$ takes \$1\$ and so on. So \$n-2k,n-(2k+1)\$ takes \$k\$.

Let \$n=2q+1\$ then \$p=2q-2k+1,2q-2k\implies \lfloor p/2\rfloor = q-k\$

i.e. \$k=q-\lfloor p/2\rfloor=\lfloor n/2\rfloor -\lfloor p/2\rfloor \$
If \$n\$ is even then \$n\$ takes \$0\$; \$n-1,n-2\$ takes \$1\$ and so on. So \$n-(2k-1),n-2k\$ takes \$k\$.

Let \$n=2q\$ then \$p=2q-2k,2q-2k+1\implies \lfloor p/2\rfloor=q-k\$

i.e. \$k=q-\lfloor p/2\rfloor=\lfloor n/2\rfloor -\lfloor p/2\rfloor\$


Caesar Cipher Cmd Tool I know there are plenty of Caesar Cipher reviews on here, but I especially wanted to see how other programmers felt about my use of the cmd module and the efficiency of my functions.

Though this program in its current state is fully functional, I feel the design may be gridlocked and lacking flexibility, due to my lack of knowledge regarding the process of creating commands for the user to utilize.

The current design makes it difficult to implement the varying command attributes I hoped to add later on. How should I go about doing this? A format I see in other cmd programs goes something like command --numbers --property=number --filename

import cmd

def sym_extract(text,syms):
    text = text[text.find(syms[0])+1:text.find(syms[1])]
    return text

def del_punc(string):
    string = ''.join(ch if ch.isalnum() else '' if ch!=' ' else ' ' for ch in string)
    return(string.lower())

def cipher(text, shift):
    text = del_punc(text)
    secret = ''
    for c in text:
        if c in 'abcdefghijklmnopqrstuvwxyz':
            num = ord(c)
            num += shift
            if num > ord('z'):
                num -= 26
            elif num < ord('a'):
                num += 26
            secret = secret + chr(num)
        else:
            secret = secret + c
    return secret

class CryptoPromptCmd(cmd.Cmd):
    prompt = '\n> '

    def default(self, arg):
        print('invalid command, try typing "help"')

    def do_quit(self, arg):
        '''exits the program'''
        return True

    def do_encrypt(self, arg):
        '''usage: encrypt (provided string or /filename)[shift number]'''
        if arg != '':
            text  = del_punc(sym_extract(arg,'()'))
            shift = int(sym_extract(arg,'[]'))
            encrypted = cipher(text,shift)
            print('\n ',encrypted.upper())
        else:
            print('invalid command\nusage: encrypt(provided string or /filename)[shift number]')
            return

    def do_decrypt(self, arg):
        '''usage:\noutput to file: decrypt(provided string or /filename)[file]\nattempt manually: decrypt(provided string or /filename)[shift number]'''
        if arg != '':
            text  = del_punc(sym_extract(arg,'()'))
            shift = -1*int(sym_extract(arg,'[]'))
            decrypted = cipher(text,shift)
            print('\n ',decrypted.upper())
        else:
            print('invalid command\nusage:')
            print('output to file: decrypt(provided string or /filename)[output /filename]')
            print('attempt manually: decrypt(provided string or /filename)[shift number]')
            return

if __name__ == '__main__':
    print('\n'* 3)
    print("Caesar Cipher Tool")
    print('==================')
    print()
    print('type "help" for commands')
    print()
    CryptoPromptCmd().cmdloop()
    print('\nexiting...\n')

If you're trying to make a shell-like utility to encrypt and decrypt strings the cmd module is exactly what you want.

In order to add argument lists to each command you could use the argparse module similarly to the following.

def do_decrypt(self, arg):
    parser = argparse.ArgumentParser(prog='decrypt')
    parser.add_argument('--filename', type=file, help='The file to decrypt')
    parser.add_argument('--shift', type=int, default=13, help='The amount to shift')
    try:
        args = parser.parse_args([arg])
    except SystemExit:
        print('The previous command was not understood')

    text = del_punc(args.file)
    shift = -1*args.shift
    decrypted = cypher(text, shift)
    ...

Some people may complain that the SystemExit exception shouldn't be caught you could also create a class that subclasses argparser.ArgumentParser and overloads the argparser.ArgumentParser.error function


Slow-running File parser I've got a method that parses a file.  I take all the words and add them to a SortedSet.  Every word contains a list of Lines that contain said word.  Words are not strings but a class I created:

class Word : IComparable<Word>
{
    public Word()
    {
        Lines = new List<Line>();
    }
    public string WordStr { get; set; }
    public List<Line> Lines { get; set; }

    public int CompareTo(Word other)
    {
        return string.Compare(this.WordStr, other.WordStr, StringComparison.OrdinalIgnoreCase);
    }
}

And the method that parses the file (I suspect I am not doing something properly here):

private void AddFile(string path)
    {
        Regex regex = new Regex("[^A-Za-z0-9\\s\\']");
        FileInfo fi = new FileInfo(path);
        if (!fi.Exists || Files.Contains(path.ToLower())) //File does not exist or file already indexed
        {
            return;
        }
        Files.Add(path.ToLower());
        StreamReader sr = new StreamReader(path);
        string file = sr.ReadToEnd();
        string saniFile = regex.Replace(file, "");
        string[] saniLines = saniFile.Split(new char[]{'\r', '\n'}, StringSplitOptions.RemoveEmptyEntries);
        int lineNo = 1;
        foreach (var l in saniLines)
        {
            Line line = new Line(l, path, lineNo);
            string[] words = l.Split(' ');
            foreach (var word in words)
            {
                Word w = new Word();
                w.WordStr = word;

                if (Words.Contains(w, new WordComparer())) //Set already contains the word
                {
                    Word wordToAdd = (from wors in Words where wors.WordStr.ToLower() == w.WordStr.ToLower() select wors).First();
                    if (!wordToAdd.Lines.Contains(line))
                        wordToAdd.Lines.Add(line);
                }
                else
                {
                    w.Lines.Add(line);
                    Words.Add(w);
                }
            }
            lineNo++;
        }
    }

I have the exact same functionality working in C++ and it's orders of magnitudes faster.  So is there something I am doing incorrectly?  What if I used a SortedDictionary instead of a SortedSet for the words?  Then the key could be the string that is the word and the value would be the list of lines that contain that word.

For reference, a 618KB text file takes a few seconds to parse and index in C++.  It's taking me minutes to do it in C#.

Your instincts are basically right that maybe a dictionary of some sort could help. Let's look at some key lines of your code.

foreach (var l in saniLines)
{
    Line line = new Line(l, path, lineNo);
    string[] words = l.Split(' ');
    foreach (var word in words)
    {
        Word w = new Word();
        w.WordStr = word;

        if (Words.Contains(w, new WordComparer())) //Set already contains the word
        {
            Word wordToAdd = (from wors in Words where wors.WordStr.ToLower() == w.WordStr.ToLower() select wors).First();
            if (!wordToAdd.Lines.Contains(line))
                wordToAdd.Lines.Add(line);

I see 5 nested loops here. If you step through the code, you'll see them, too. The outer 2 are obvious, you've coded them explicitly. But stepping through the code will reveal the looping being done at Words.Contains. The Linq query is also an abstracted loop. And wordToAdd.Lines.Contains is also going to be a loop. You're going to pay a high price for these.

Profiling will always help. But I would suggest perhaps changing Words to some sort of IDictionary<string, Word>, where you can replace

if (Words.Contains(w, new WordComparer())) //Set already contains the word
{
    Word wordToAdd = (from wors in Words where wors.WordStr.ToLower() == w.WordStr.ToLower() select wors).First();

With something more like

if (Words.ContainsKey(w.WordStr))
{
    Word wordToAdd = Words[w.WordStr];
    // ...
}

And you could also change Lines into a HashSet<Line> instead of a list. That should improve the performance of

if (!wordToAdd.Lines.Contains(line))
    wordToAdd.Lines.Add(line);

In fact, HashSet<T>.Add(T val) could be used by itself, without the call to Contains, as Add returns a bool indicating if the addition could be performed. If a matching value is already in the set, it simply returns false without modifying its contents.

Try these changes, measure the performance before and after and see where you are.

Unrelated, but I also recommend that you wrap your StreamReader in a using () { } block so that the resource is properly disposed when you're through with it.


Calculating binary tree height as asked in the interview Input:

  The first line contains the number of vertices \$n\$. The second line
  contains \$n\$ integer numbers from the range \$\left[−1, n − 1\right]\$ representing the \$0\$-based index of the parent for each vertex.
  An index of \$-1\$ designates that vertex as a root without a parent.

  It is guaranteed that there is exactly one root.It is guaranteed that
  the input represents a tree.

Output:

  Output the height of the tree.

Code:

console.clear();

function heightSlow(parent) {
  var maxHeight = 0;
  // for each vertex check its height
  // and keep track of maximum height so far.
  for (var i = 0; i < parent.length; i++) {
    var height = 0;
    // O(n*n) // n is the number of vertex
    for (var vertex = i; vertex != -1; vertex = parent[vertex]) {
      height++;
    }
    // vertex should be -1 here
    maxHeight = Math.max(maxHeight, height);
  }
  return maxHeight;
}

function heightFast(parent) {
  var depth = [];
  var N = parent.length;

  function recur(parent, i, depth) {
    if (depth[i] !== 0) return;

    if (parent[i] === -1) {
      depth[i] = 1;
      return;
    }

    // Find the depth of parent
    if (depth[parent[i]] === 0) recur(parent, parent[i], depth);

    // We have depth of the parent now
    depth[i] = depth[parent[i]] + 1;
  }

  var vertex;
  // fill with default depth, although I could use null
  // but stick with general notion of invalid value mathematically
  // Theta(N)
  for (vertex = 0; vertex < N; vertex++)
    depth[vertex] = 0;

  // find depth of each node
  // recursion would be called for each node but
  // it would return immidiately hence we can ignore that call?
  // if yes then recur would be called N times exactly. Hence,
  // Theta(N)
  for (vertex = 0; vertex < N; vertex++)
    recur(parent, vertex, depth);

  // get the max depth, which is height
  // Theta(N)
  var height = 0;
  for (vertex = 0; vertex < N; vertex++) {
    height = Math.max(height, depth[vertex]);
  }
  return height;
}

// O(N^2)
console.log(heightSlow([4, -1, 4, 1, 1])); // 3
console.log(heightSlow([-1, 0, 4, 0, 3])); // 4

// Theta(N)
console.log(heightFast([4, -1, 4, 1, 1])); // 3

console.log(heightFast([-1, 0, 4, 0, 3])); // 4

Disclaimer:

I am more interested in fail proof approaches like writing correct loops, making quick tests without any testing frameworks to check the code correctness and finally reasoning about the run time complexity.

Having said that I would like to adhere to the problem specification in past I saw people arguing about the best practices which doesn't match with the problem statement and hence becomes immaterial.

There is no input/output handling in your code.

Even though it is fine to implement the algorithm in isolation, and also to base your unit tests on the backing implementation, handling input and output was part of the problem specification.

If this was for an interview, you would have missed to fulfill the specification.

  Having said that I would like to adhere to the problem specification in past I saw people arguing about the best practices which doesn't match with the problem statement and hence becomes immaterial.

Even if the problem specification gives you guarantees about the input being well formatted, you still should take care to handle invalid inputs gracefully.

Both of your implementations e.g. get caught in an infinite loop, should the input contain a directed graph with loops instead of a tree. Your second implementation will even exhaust the call stack eventually.

Simply limiting the recursion depth / iteration count to the number of input elements would have protected you against that.

Verifying that there is only a single root node, or that all indexes are valid would have easily fit within the budget as well. Even though your algorithms would just throw an exception / yield nonsensical results in that case, so it's not as critical as the loop case.

As it stands, your code is a potential vector for a DoS attack and should therefor not be used in any publicly accessible environment.

function recur(parent, i, depth) {
  if (depth[i] !== 0) return;

  if (parent[i] === -1) {
    depth[i] = 1;
    return;
  }

  // Find the depth of parent
  if (depth[parent[i]] === 0) recur(parent, parent[i], depth);

  // We have depth of the parent now
  depth[i] = depth[parent[i]] + 1;
}

Recursions are nasty in JavaScript, because the maximum recursion depth is pretty low on some platforms: https://stackoverflow.com/a/7828803/2879325

You should try to express this in iterative form if possible, especially since the problem specification does not provide you with an upper bound!

// O(N^2)
console.log(heightSlow([4, -1, 4, 1, 1])); // 3
console.log(heightSlow([-1, 0, 4, 0, 3])); // 4

// Theta(N)
console.log(heightFast([4, -1, 4, 1, 1])); // 3
console.log(heightFast([-1, 0, 4, 0, 3])); // 4

That's an usage example, but it's not a unit test. If you don't want to use a fully fleshed testing framework, the least you can do is something this:

function test(functor, expected) {
  var result;
  try {
    result = functor();
  } catch(e) {
    console.error(Function.prototype.toString.call(functor), e, expected);
  }
  // Object comparison hack
  if (JSON.stringify(result) !== JSON.stringify(expected)) {
    console.error(Function.prototype.toString.call(functor), result, expected);
  }
}

// O(N^2)
test(function (){return heightSlow([4, -1, 4, 1, 1])}, 3);
test(function (){return heightSlow([-1, 0, 4, 0, 3])}, 4);

// Theta(N)
test(function (){return heightFast([4, -1, 4, 1, 1])}, 3);
test(function (){return heightFast([-1, 0, 4, 0, 3])}, 4);

That provides you with simple unit tests which are both machine checked and easy to comprehend in case of an error.

Just beware that this is not exactly robust with regard to run time errors, such as exhausting the recursion limit!

As for your choice of test cases, you didn't test anything but trivial cases.

The most bugs however usually hide in edge cases, such as:

Minimal input
Degenerated input (sequential list or all nodes are children of root)
Malformed input (duplicate root, no root, loops)
Large inputs (long sequential list, exhausting the recursion limit!)

// find depth of each node
// recursion would be called for each node but
// it would return immidiately hence we can ignore that call?
// if yes then recur would be called N times exactly. Hence,
// Theta(N)

The conclusion is correct, but the argumentation is incoherent.

Your argument is that you are calculating the height of each node exactly once, plus a total of \$n\$ comparisons to verify that each node has either been computed yet, which amounts to a total of \$2n\$ calls to recur().


Parameter guards for Firebase calls I have code to ensure parameters existence all over the place. Is there a better way to do it? So that I do not have so many repeated codes inside different functions

export const requestUpdateAccountDetail = (
  firebase,
  uid,
  accountId,
  accountDetail = {}
) => {
  if (firebase === undefined) {
    throw Error('`firebase` instance is required')
  } else if (uid === undefined) {
    throw Error('`uid is required to update account detail`')
  } else if (accountId === undefined) {
    throw Error('`accountId` is required to update account detail')
  }

  return firebase.update(`/accounts/${uid}/${accountId}`, accountDetail)
}

export const requestAddNewAccount = (firebase, uid) => {
  if (firebase === undefined) {
    throw Error('`firebase` instance is required')
  } else if (uid === undefined) {
    throw Error('`uid is required to update account detail`')
  }

  const EMPTY_ACCOUNT_OBJ = {
    name: '',
    note: '',
    owner: uid
  }

  return firebase.push(`/accounts/${uid}`, EMPTY_ACCOUNT_OBJ)
}

export const requestDeleteAccount = (firebase, uid, accountId) => {
  if (firebase === undefined) {
    throw Error('`firebase` instance is required')
  } else if (uid === undefined) {
    throw Error('`uid is required to update account detail`')
  } else if (accountId === undefined) {
    throw Error('`accountId` is required to update account detail')
  }

  return firebase.remove(`/accounts/${uid}/${accountId}`)
}

So given this is a blanket rule across all the functions i.e. val === undefined then throw you could introduce a common validator function you could share across all your functions e.g.

function assertParams(params) {
   for (const prop in params) {
       if (params[prop] === undefined) {
           throw new Error(`Argument ${prop} is undefined`);
       }
   }
}
...
export const function requestAddNew(firebase, uid, accountId) {
    assertParams({ firebase, uid, accountId });
    ...
}
export const function requestDeleteAccount(firebase, uid, accountId) {
    assertParams({ firebase, uid, accountId });
    ...
}

you'd need to call it in every function, however, this would give you the same effect of what you already have in one line as opposed to X lines based on the number of parameters you have.

If your using Babel, something to look at would be perhaps moving it to a decorator so it's completely outside the scope of your logic e.g.

@assertParams()
export const function requestAddNew(...)

The above code wouldn't just "work" in the decorator, would need a little bit of tweaking but I'm pretty sure it's doable.


Different way of writing multiple click functions I have the following .click() functions:

$('.tab').click(function(){
    $('.tab').removeClass('activeTab');
    $(this).addClass('activeTab');
});

$('.edit').click(function(){
    $(this).hide();
    $(this).next().show();
});

$('.cancel').click(function(){
    $(this).parent().hide();
    $(this).parent().prev().show();
});

Is it possible to combine these functions doing something like this?

Pseudocode:

$.click(function(){
    for $('.tab'){
        //do this
    }
    for $('.edit'){
        //do this
    }
    for $('.cancel'){
        //do this
    }
});

HTML:

Edit

    Cancel |
    Save

First off, I'd rewrite the HTML to get rid of the javascript href attributes. I'd also use a data-action attribute for the action name. You can still use class to do the styling of course, but style is not behavior, so keep those things separate (i.e. the class might be action-button primary highlighted or something, but the action would still be save):

Edit

    Cancel |
    Save

Code-wise, what each link has in common is that they should preventDefault so the links aren't "followed" when clicked. So at the very least, we'll need this:

$("a[data-action]").on("click", function (event) {
  event.preventDefault();
});

Then of course, there are the actions/behaviors themselves. I'd suggest putting the logic for each in an object, and expanding the generic click handler like so:

var actions = {
  edit:   function (event) { ... },
  cancel: function (event) { ... },
  save:   function (event) { ... }
};

$("a[data-action]").on("click", function (event) {
  var link = $(this),
      action = link.data("action");

  event.preventDefault();

  // If there's an action with the given name, call it
  if( typeof actions[action] === "function" ) {
    actions[action].call(this, event);
  }
});

Now you have a generic click handler, and an easily extensible list of actions. The action functions themselves behave exactly like normal jQuery event handlers (i.e. this will refer to the link clicked, and the first argument will be the event).


Calculate min, max, mean and median out of array Another C exercise:

  Write a function that takes an array of 'ints' as its input and finds the smallest and largest elements. It should also compute the median and mean. Use a struct holding the result as the return value

I found it quite awkward that there is no function to copy an array into another array, so I wrote one intdup to use it in the median calculation.

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

struct Result {
    int smallest;
    int largest;
    double median;
    double mean;
};

int cmp(void const *lhs, void const *rhs) {
    const int *left = (const int *)lhs;
    const int *right = (const int *)rhs;
    return *left - *right;
}

int * intdup(const int * source, const size_t len)
{
    assert(source);

    int * p = malloc(len * sizeof(int));

    if (p == NULL)
        exit(1);

    memcpy(p, source, len * sizeof(int));
    return p;
}

int find_smallest(const int* array, const size_t len)
{
    assert(array);

    size_t i = 0;
    int smallest = INT_MAX;

    for (i = 0; i < len; ++i) {
        if (array[i] < smallest)
            smallest = array[i];
    }

    return smallest;
}

int find_largest(const int* array, const size_t len)
{
    assert(array);

    size_t i = 0;
    int largest = INT_MIN;

    for (i = 0; i < len; ++i) {
        if (array[i] > largest)
            largest = array[i];
    }

    return largest;
}

double calculate_median(const int* array, const size_t len)
{
    assert(array);

    int* calc_array = intdup(array, len);
    int tmp = 0;

    qsort(array, len, sizeof(int), cmp);

    if (len % 2 == 0) { // is even
        // return the arithmetic middle of the two middle values
        return  (array[(len - 1) / 2] + array[len / 2] ) /2.0;
    }
    else {  // is odd
        // return the middle
        return array[len / 2];
    }
}

double calculate_mean(const int* array, const size_t len)
{
    assert(array);

    double mean = 0;
    size_t i = 0;

    for (i = 0; i < len; ++i)
        mean += array[i];

    return mean / len;
}

struct Result calculate_values(const int* array, const size_t len)
{
    assert(array);

    struct Result result = { 0 };

    result.smallest = find_smallest(array, len);
    result.largest = find_largest(array, len);
    result.median = calculate_median(array, len);
    result.mean = calculate_mean(array, len);

    return result;
}

void print_result(const struct Result* result)
{
    assert(result);

    printf("smallest: %i\n", result->smallest);
    printf("largest: %i\n", result->largest);
    printf("median: %f\n", result->median);
    printf("mean: %f\n\n", result->mean);
}

int main()
{
    int numbers[] = { 1,7,3,4,5,6,7,8,9 };      // 9 elements
//  int numbers[] = { 1,7,3,4,5,6,7,8 };        // 8 elements

    int len = sizeof(numbers) / sizeof(numbers[0]);
    struct Result result = calculate_values(&numbers, len);

    print_result(&result);
    getchar();
    return 0;
}

See the the revised code her: Calculate min, max, mean and median out of array Version 2

I see a number of things that may help you improve your program.

Don't violate const

The calculate_median routine takes a const array, makes a duplicate and then sorts the original!  I think you meant to sort the duplicated array instead.

Understand pointers

In main there's this line:

int numbers[] = { 1,7,3,4,5,6,7,8,9 }; // 9 elements
// other stuff
struct Result result = calculate_values(&numbers, len);

The problem is that &numbers is not really what you want.  With an array, the name is already effectively a pointer to the array, so you don't need & at the front in this context.

Don't leak memory

The intdup function allocates and copies the passed array, but then that memory is never freed.  I'd suggest freeing the memory again as soon as the function is done with the duplicate array.

Think about naming

The variables len and mean are good because they suggest the significance of these variables in the context of the program.  However, result and numbers are a little generic.  I'd suggest perhaps summaryData and testArray as possible replacement names.

Eliminate unused variables

The tmp variable within calculate_median is never used.  Since unused variables are a sign of poor code quality, you should seek to eliminate them.  Your compiler is probably smart enough to warn you about such things if you know how to ask it to do so.

Think about efficiency

This suggestion is last for a reason.  It's important to have a correct program first, and then optimize for space/time efficiency.  In this case, a productive approach is suggested by the wording of the question itself:

  Write a function that takes an array of 'ints' as its input and finds the smallest and largest elements. It should also compute the median and mean. Use a struct holding the result as the return value.

It says to write a function and that's a good strategy for this.  Since you're sorting the array anyway to get the median, you could also use the sorted array to easily get the min and max values from that sorted array.  The mean could be calculated the same way you're currently doing it, but this would reduce the number of times that the array would need to be traversed.


Iterating over a range of dates in a shell script I have been working on a script that can be used to automatically pull the batch ids that were processed 4 days ago and which need to be processed tomorrow on the server. The batch IDs from 3 days ago need to be processed the day after tomorrow, etc.

The script only needs to output the processing dates for all batch IDs from the last 4 days and send the output via email with the format below.

Please have a look and let me know of any improvement we can make, especially to the performance.

#!/bin/bash

matchdate () { date +%Y%m%d --date "$1" }

touch $result

dbhost='192.168.0.1'
dbuser='test_db'
dbpass='temp#100'
dbschema='demo'

batchdate=$(matchdate "4 days ago")

current_date=$(matchdate "today")

while [[ $batchdate != $current_date ]]; do

  batchdate=$(matchdate "$batchdate + 1 day")

   batchwupos=$(date -d "$batchdate" +%Y-%m-%d)

   processdate=$(date +%d-%m-%Y -d "$matchdate + 1 day")

  #echo "$batchdate and $batchwupos"

 recordpull=`mysql -h $dbhost -u $dbuser -p $dbpass -e "use $dbschema;SELECT il.BatchID FROM tbl_batch_table il WHERE il.batch_name like '%$batchdate%' and il.batch_name like '%$batchwupos%' and il.status='SUCCESS'"`

batch_result="`cat $recordpull | tr -s ' ' | cut -d " " -f2- | tr '\n' ',' | sed 's/,$//' | sed 's/........//' $recordpull`"

echo -e "\n Below are the expected batches which we will processing on server on specified date." >> $result

echo -e "\n $processdate : $batch_result" >> $result

done

Here are the expected batches which will process on the server on the specified date:

12-09-2016: (10642, 10643, 10644, 10646, 10647)
13-09-2016: (10648, 10649, 10654, 10655, 10656, 10659)
14-09-2016: (10657, 10658, 10661, 10665, 10666)
15-09-2016: (10668, 10669, 10670, 10671)

Start with the query and work backwards.  A poorly-written, poorly-performing query is not a good foundation to build on.

Here is your query (re-formatted to more readable)

SELECT il.BatchID
FROM tbl_batch_table AS il
WHERE
  il.batch_name LIKE '%$batchdate%'
  AND il.batch_name LIKE '%$batchwupos%'
  AND il.status='SUCCESS'

This query will not be able to leverage an index on batch_name because you are using a LIKE condition that starts with a wild card character.

Can you split apart the batch date information in your DB table into it's own column?  That would give you the ability to do things like:

Query on a date range AND use index to do so.
Perform date-based math (adding/subtracting days)

Imagine this sort of query:

SELECT
    il.batch_date,
    /* Create calculated field representing processing date four days
       after batch date */
    DATE_ADD(il.batch_date, INTERVAL 4 DAY) AS processing_date,
    il.BatchID
FROM tbl_batch_table AS il
WHERE
  /* Filter for records batched in last 4 days */
  il.batch_date >= DATE_SUB(CURDATE(), INTERVAL 4 DAY)
  AND il.status='SUCCESS'
/* Order your results */
ORDER BY il.batch_date ASC, il.BatchID ASC

Here you can leverage an index on batch_date for WHERE clause and ordering.  You can also totally eliminate having multiple queries as you are getting records for the entire range of dates at once.

If you wanted to go with the suggestion from other answer and aggregate information for each date with concatenated results, you could add GROUP BY and GROUP CONCAT to this query like follows:

SELECT
    /* Create calculated field representing processing date four days
       after batch date */
    DATE_ADD(il.batch_date, INTERVAL 4 DAY) AS processing_date,
    GROUP_CONCAT(il.BatchID) AS batch_ids
FROM tbl_batch_table AS il
WHERE
  /* Filter for records batched in last 4 days */
  il.batch_date >= DATE_SUB(CURDATE(), INTERVAL 4 DAY)
  AND il.status='SUCCESS'
GROUP BY processing_date
/* Order your results */
ORDER BY il.batch_date ASC, il.BatchID ASC


Secret Santa random pair allocator Please review my Secret Santa random pair allocator written in JavaScript. Random pairs will be generated every time you run the code.

Input data is hardcoded for now (line # 41).

I would love to know if you have any suggestions on improving the algorithm or any other feedback that you might have.

function getSecretSantaRandomPairs(inputArray){

  if(inputArray.length % 2 !== 0){ // array length should be even
    return;
  }

  var allocatedPairs = [], // variables for housekeeping
      randomNumber,
      randomPerson;

  while(inputArray.length !== 0){ // process the inputArray
    randomNumber = getRandomInteger(0, inputArray.length-1); // get a random number
    randomPerson = inputArray[randomNumber]; // the corresponding random person from the given list
    allocatedPairs = allocate(allocatedPairs, randomPerson); // allocate the randomPerson and update the array to be returned - allocatedPairs
    inputArray.splice(randomNumber, 1); // remove from the inputArray so that the person is not allocated again
  }

  return allocatedPairs;

}

function allocate(allocatedPairs, candidate){ // in the allocatedPairs array, mantain the pairs as objects, example: {person1: 'Peter', person2: 'Jordan'}
  // first allocate as 'person1' in the object
  if(allocatedPairs.length === 0 || (allocatedPairs[allocatedPairs.length-1].hasOwnProperty('person1') && allocatedPairs[allocatedPairs.length-1].hasOwnProperty('person2') )){ // allocatedPairs is either an empty array or the object at the bottom of the allocatedPairs array had both person1 and person2
    allocatedPairs.push({person1: candidate});
  } else{ // otherwise 'person2'
    allocatedPairs[allocatedPairs.length-1].person2 = candidate;
  }

  return allocatedPairs;
}

// Source: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Math/random
function getRandomInteger(min, max) {
  return Math.round(Math.random() * (max - min)) + min;
}

// lets test this out

var newList = getSecretSantaRandomPairs(['John', 'Amy', 'Harry', 'Aliston', 'Jeff', 'Chris', 'Kurt', 'Jessica']);

if(newList !== undefined){
  console.log('Secret Santa Random Pairs: ', newList);

  var pairsUL = document.getElementById('pairs'),
      pairsLI,
      textLI;

  // iterate over the newList to create DOM elements
  newList.forEach(function(currentValue, index){
    pairsLI = document.createElement('li'); // the LI element
    textLI = newList[index].person1 + ' and ' + newList[index].person2; // the text for the LI
    pairsLI.appendChild(document.createTextNode(textLI)); // add text to the LI
    pairsUL.appendChild(pairsLI); // append the LI to the UL
  });
} else {
  document.body.innerHTML = '
Please input an even number of people!
'; // source: http://stackoverflow.com/a/5419800/586051
}

Equivalent jsFiddle

As far as I remember, the game isn't strictly pairs. It's everyone's names in a hat, and everyone gets to draw a random name, redraw if it's your own name. This means you don't need an even number of names.

var pairsUL = document.getElementById('pairs'),
      pairsLI,
      textLI;

newList.forEach(function(currentValue, index){
  pairsLI = document.createElement('li'); // the LI element
  textLI = newList[index].person1 + ' and ' + newList[index].person2; // the text for the LI
  pairsLI.appendChild(document.createTextNode(textLI)); // add text to the LI
  pairsUL.appendChild(pairsLI); // append the LI to the UL
});

DOM manipulation is slow. Instead of appending to a live element on each iteration, use a fragment. It's something like an in-memory, not-in-DOM element-like container. Use document.createDocumentFragment() to create one, and append your lis to it. After the loop, append the fragment to your list.

function getRandomInteger(min, max) {
  return Math.round(Math.random() * (max - min)) + min;
}

Instead of Math.round, use Math.floor or append | 0 to the operation. Arrays are zero-indexed. Math.random values range from 0 to 1, excluding 1 itself. This means you'll never get length * 1 and floor will always round down down to the integer before length * 1, the last index in the array.

Readable code is good enough to explain what it's doing. Comments should not explain what the code already does. Instead, it should explain why it was done in that manner. Additionally, you're overloading the lines with long comments. Put them before the code described and limit them to something like 80 characters or 120 characters per line.


Decoding name/value pairs I have this code in a class sharing a private static array of data. Internally it's encoded, but I need the iterator for a foreach loop to show the decoded values.

Is there a native array function that can remap the keys and values via a callback function?

public function getIterator( )
{
  $arr = array();
  foreach ( self::$data as $encName => $encValue )
  {
    $name = $this->decode( $encName );
    $value = $this->decode( $encValue );
    $arr[$name] = $value;
  }
  return new ArrayIterator( $arr );
}

Implementing Iterator is the way to go in my opinion. As for your question, array_map would allow you to decode the values but not the keys. But you can combine that with other array operations to get the desired result.

public function decodeArray($array) {
    $decode = array($this, 'decode');
    return array_combine(
            array_map(array_keys($array), $decode),
            array_map(array_values($array), $decode)
        );
}


Searching for combinations of words and numbers My model called Customer contains a method called generate_fields which creates records based on its return values.

def generate_fields
  words.map do |word|
    numbers.map do |number|
      content = find_content(word, number)
      next if content.nil?

      {
        kind: word,
        value: content
      }
    end.compact
  end.flatten
end

#[
#  { kind: "S", value: "BRL" },
#  { kind: "M", value: "AUS" },
#  { kind: "L", value: "PER" },
#]

The methods words and numbers are private and return an array of strings. Their implementation is not relevant to this discussion. generate_fields returns an array of hashes which is used to create multiple customers at the database.

Customer.create(generate_fields)

The problem is compact and flatten create a new array for every time they are called as pointed out by @David Aldridge. How can this function be rewritten to avoid excessive array creation? Thanks in advance.

I'm not a fan of the latter because the compact and flatten methods create new arrays.

Here's another variation with two additional style options: use of product to combine the two arrays, and each_with_object:

def generate_fields_3
  words   = ["foo", "bar", "bla"]
  numbers = [1, 2, 3]

  words.product(numbers).each_with_object([]) do |(word, number), fields|
    content = find_content(word, number)
    next if content.nil?

    fields << {
      kind: word,
      value: content
    }
  end
end


Date composition from day, month and year input I need to concatenate 3 input fields into a string which is formatted as a date dd-mm-yyyy, using type="date" or new Date() will not work in my scenario because Firebase doesn't allow it for some reason. I came up with a rather ugly solution, but I'm not quite sure how to improve it. delete is bad for performance, but I think that it is needed because I need to completely remove the properties from the object once they've been used.

$scope.addWeek = function(week) {

  var startDate = '' + week.startDate.day + '-' + week.startDate.month + '-' + week.startDate.year,
      endDate = '' + week.endDate.day + '-' + week.endDate.month + '-' + week.endDate.year;

  week.startDate = startDate;
  week.endDate = endDate;

  delete week.startDate.day;
  delete week.startDate.month;
  delete week.startDate.year;
  delete week.endDate.day;
  delete week.endDate.month;
  delete week.endDate.year;
}

The week object looks like this:

week = {
  number: 32,
  startDate: {
    day: 01,
    month: 07,
    year: 2015
  },
  endDate: {
    day: 08,
    month: 09,
    year: 2016
  }
}

After we're done it needs to look like this:

week = {
  number: 32,
  startDate: 01-07-2015,
  endDate: 08-09-2016
}

Any suggestions on how to improve it?

As Ekkhard.Homer said, none of you delete lines do anything, because startDate and endDate are strings at that point: They don't have any day, month or year properties.

Anyway, since you need to do the same thing twice, I'd suggest you make a function for it. I've spruced this one up a bit by mapping properties, but that's not really necessary.

$scope.addWeek = function(week) {

  function formatDate(obj) {
    return ['day', 'month', 'year'].map(function (prop) {
      return String(obj[prop]);
    }).join('-');
  }

  week.startDate = formatDate(week.startDate);
  week.endDate = formatDate(week.endDate);
}


Switched from MySQL to MySQLi requires the connection parameter I was working with really old PHP I learned years ago and decided to finally update it, so I changed all the mysql to mysqli. I noticed some MySQLi functions require you to pass the connection in the parameters. I ended doing something like this:

$con = getCon();
$result = mysqli_query($con, "SELECT * FROM sometable WHERE deleted=0");

getCon() is a return like return Database::getConnection(); that uses the next class:

class Database {
    private static $db;
    private $connection;

    private function __construct() {
        $servername = "localhost";
        $username = "...";
        $password = "...";
        $dbname = "...";

        $this->connection = new mysqli($servername, $username, $password, $dbname);
    }

    function __destruct() {
        $this->connection->close();
    }

    public static function getConnection() {
        if(self::$db == null) {
            self::$db=new Database();
        }

        return self::$db->connection;
    }
}

I don't like adding the line $con = getCon(); to the top of every function that uses mysqli_query. Is there a way to avoid this? (it is one extra line multiplied by every function that uses mysqli calls)

Use object-oriented style:

$con = getCon();
$result = $con->query("SELECT * FROM sometable WHERE deleted=0");

Refer to Example #1 on the PHP documentation for mysqli_query(), as well as the Dual interface quick start page for more information.

Looking back at the functions like deprecated mysql_query() it appears that it used the "last link opened by mysql_connect()". If you really wanted to replicate that, you could define a wrapper function (potentially using call_user_func()):

function mysqli_query_con($query, $resultmode = MYSQLI_STORE_RESULT)
    $con = getCon();
    return mysqli_query($con, $query, $resultmode);
}


Flying-balls on a canvas element I have made this little program for fun, to add on my personal webpage. I remember seeing something like it some time ago, don't remember exactly where, and I tried to reproduce it myself.

As it turns out, I figured out that it isn't quite fun to have your CPU stressed out by 25% because of a funny single homepage, so I won't be using it anymore, but I thought it'd be a good chance to put my efforts to test here on Code Review.

Any thoughts are welcome. I'm specially curious if the way I'm creating the dots looks good or if it is of good performance or not. I profiled the code a few days ago and realized that the main source of processing tasks were related to canvas drawing.

Working example here

var canvas,
    ctx,
    colors = ['#FF6600', '#0099FF', '#BCBCBC', '#3D3D3D'],
    dots = [],
    maxDots = 35;

function Dot() {
    this.alive = true;
    this.x = Math.round(Math.random() * canvas.width);
    this.y = Math.round(Math.random() * canvas.height);
    this.size = Math.round(Math.random() * 7);
    this.velocity = {
        x: (Math.random() < 0.5 ? -1 : 1) * Math.random() * 0.7,
        y: (Math.random() < 0.5 ? -1 : 1) * Math.random() * 0.7
    }
    this.alpha = 0.01;
    this.hexColor = colors[Math.round(Math.random() * (colors.length - 1))];
    this.rgbaStringColor = getRGBA(this.hexColor, this.alpha);
}

Dot.prototype.draw = function () {
    ctx.beginPath();
    ctx.fillStyle = this.rgbaStringColor;
    ctx.arc(this.x, this.y, this.size, 0, 2 * Math.PI);
    ctx.fill();
}

function getRGBA(hex, alpha) {
    var r = parseInt(hex.substring(1, 3), 16),
        g = parseInt(hex.substring(3, 5), 16),
        b = parseInt(hex.substring(6, 8), 16),
        a = alpha;
    return 'rgba(' + r + ', ' + g + ', ' + b + ', ' + a + ')';
}

/*updates both colors and position of the dot*/
Dot.prototype.update = function () {
    if (this.alpha < 0.7) {
        this.alpha += 0.01;
        this.rgbaStringColor = getRGBA(this.hexColor, this.alpha);
    }

    this.x += this.velocity.x;
    this.y += this.velocity.y;

    if (this.x > (canvas.width) + 5 || this.x < -5) {
        this.alive = false;
    }
    if (this.y > (canvas.height) - 15 || this.y < -5) {
        this.alpha -= 0.05;
        if (this.alpha <= 0.1) {
            this.alive = false;
        }
    }
}

function createDots() {
    dots = dots.filter(function (dot) {
        return dot.alive;
    });
    for (var length = dots.length; length < maxDots; length++) {
        dots.push(new Dot());
    }
}

function update() {
    ctx.clearRect(0, 0, canvas.width, canvas.height);
    for (var i = 0; i < dots.length; i++) {
        dots[i].draw();
        dots[i].update();
    }
    createDots();
    window.requestAnimationFrame(update);
}

function init() {
    createDots();
    update();
}

function setCanvasSize() {
    canvas.width = window.innerWidth;
    canvas.height = 300;
}

document.addEventListener('DOMContentLoaded', function () {
    canvas = document.getElementById('universe');
    ctx = canvas.getContext('2d');
    setCanvasSize();
    init();
});

The CPU load doesn't look so bad. On my 2011 MBP the linked example runs easily at 60fps, taking between 0.4 and 0.85ms (of the 16.6ms available for that framerate) to run the animation frame callback. No sweat.

As for code style; it's generally pretty clear and easy to follow, though I would suggest  name-spacing your state variables and methods instead of using global variables. This is generally considered better practice, it's safer, can in theory improve performance and make the code easier to read. For example, you could do something like:

FlyingBalls = {
    colors: [...],
    dots: [],
    maxDots: [],
    init: function initDots(canvasId) { this.canvas = ... }
    create: function createDots() { ... },
    update: function updateDots() { ... }
}

Or use a constructor like:

function FlyingBalls(canvasId) {
    var canvas,
        ctx,
        ...;

    function createDots() { ... }

    function updateDots() { ... }

    // init code here
    canvas = document.getElementById(canvasId);
    ...
}


Setting fixed height panes based on window's width I have a webapp that follows responsive design and has following layout structure. The left layout appears when the window's width > 768px and right layout shows otherwise.

Based on preliminary research I think there are no modern pure CSS ways of doing it, so I implemented some jQuery code to solve it.

$(window).resize(function() {
    if (window.innerWidth > 768) {
        fixedHeight = window.innerHeight - $('.search__results').position().top;
        $('.search__results').addClass('overflow-y-scroll').height(fixedHeight);
    } else {
        $('.search__results').removeClass('overflow-y-scroll').height('auto');
    }
});

My CSS class is as follows:

.overflow-y-scroll {
    overflow-y: scroll;
}

I would like to leave my code up here for review and seek for DRY-er code, some spots I think may be up for improvement.

768px being fixed, should it be called from a global variable where it is the single point for setting the breakpoints? If so, I wonder where it should be placed so CSS & JavaScript can both use it?

Should I name $('.search__results') as a variable? Perhaps even make it a more generic helper function? If so, does any one have any tips on how to write it?

I'm also curious as to what is usually the best practice when creating such responsive layouts.

Your provided code is fairly small, so there's not much to review, but nevertheless.

Your comment on making 768 a constant is a valid idea,

const MAX_WINDOW_WIDTH = 768;

...

if (window.innerWidth > MAX_WINDOW_WIDTH) {

for magic numbers (unexplained numbers meaning unexplained things), it's best to declare as a const, but your version of JavaScript may not support this, so you can also use var.

I don't particularly understand why you have double underlines in '.search__results', and to answer your question: you could declare it as a variable if you like, but it really wouldn't matter either way.

I think 'overflow-y-scroll' is a bad name for a CSS class as it's, basically the content of the class.

  where it is the single point for setting the breakpoints?

You can set breakpoints in the relevant debugger on your browser, usually labelled Developer Options, or something similar.

You don't really need to worry about your code violating DRY principles, the if-else loop cases aren't really similar enough for that. If you had something like twenty identical lines in there, maybe so.

Good questions, and clean code, good work!


This yet another way to do it:

sed -r '1{s/$/ /;s/ / \n/g};:a;$!N;s/$/ /;:b;s/\n(.*\n+)(\S+\s)/\2@!@\1/;tb;s/@!@/\n/g;${s/ \n/\n/g;s/\n+$//;q};ba'

This method is somewhat faster and only uses single delimiter which may be crafted to be unique. i.e. @!@ in this example


Python - use a list of names to find exact match in pandas column containing emails I have an Excel file containing a free-form text column (that sometimes is structured like an email), where I need to find all first and last names and add an extra columns saying TRUE/FALSE to these fields. I do not need to extract matched data (i.e. note it down in an adjacent column), although that could be an advantage.
NB: I do not know the names that I need to find, so it is pure guesswork. I have a list of registered first names with 40k+ entries, as well as a list of most common last names with another 16k+ entries.
So far, I managed to filter out roughly 10000 rows out out ~20000 row file, although my solution contains a lot of false positives. e.g. some rows marked TRUE for first names, contain text like "Det er OK.", where Python (I assume) merges the entire text together and extracts any matching substing to a name from a list, in this case I guess that could be "t er O" or "r OK", since my list has names "Tero" and "Rok" (although the case does not match and it combines letters from 2/3 separate words, which is not what I want)... Weirdly enough, this is NOT TRUE for the same text written in lowercase and without "." at the end, i.e. "det er ok", which is marked as FALSE! P.S. there are unfortunatelly few names in the emails that are written in lowercase letters and not sentence case as it should be...
Sample email (with names Thomas, Lars, Ole, Per):

Hej Thomas,
De 24 timer var en af mange sager som vi havde med til møde med Lars og Ole. De har godkendt den under dette møde.
Mvh. Per

My code:
# Import datasets and create lists/variables
import pandas as pd
from pandas import ExcelWriter

namesdf = pd.read_excel('names.xlsx', sheet_name='Alle Navne')
names = list(namesdf['Names'])

lastnamesdf = pd.read_excel('names.xlsx', sheet_name='Frie Efternavne')
lastnames = list(lastnamesdf['Frie Efternavne'])

# Import dataset and drop NULLS
df = pd.read_excel(r'Entreprise Beskeder.xlsx', sheet_name='dataark')
df["Besked"].dropna(inplace = True)

# Compare dataset to the created lists to match first and last names
df["Navner"] = df["Besked"].str.contains("|".join(names)) # Creates new column and adds TRUE/FALSE for first names
df["Efternavner"] = df["Besked"].str.contains("|".join(lastnames)) # Creates new column and adds TRUE/FALSE for last names

# Save the result
writer = ExcelWriter('PythonExport.xlsx')
df.to_excel(writer)
writer.save()

I would appreciate any suggestions that could potentially improve my code and reduce manual work that it will take to filter out all of these false positive cells that I found! I think the best case scenario would be a case sensitive piece of code that finds only the specific name without merging the text together. Also, it would be great if I could extract a specific string that Python finds a match in, as that would reduce manual work when trying to figure out why exactly a specific block of text was marked as TRUE. All in all, every suggestion is welcome! Thanks :)

It sounds like the thing you're trying to do is somewhat insane. With 40k first names to search for, false positives are inevitable. At the same time, with only 40k names, false negatives are also inevitable. People's names are untidy; hopefully you have plans to accommodate. Even when you get correct matches for a "first" and "last" name, as your example email shows, there's no guarantee that they'll be the first and last names of the same person.
Maybe someone with experience in natural-language-processing AI would be able to solve your problem in a robust way. More likely you've resigned yourself to a solution that simply isn't robust. You still pretty definitely need case-sensitivity and "whole word" matching.
I'm not convinced by the example you give of a false positive. The pandas function you're using is regex-based. r'tero' does not match 't er o'; it does match 'interoperability'. With name lists as long as you're using, it seems more likely that you over-looked some other match in the email in question. I would kinda expect just a few of the names to be responsible for the majority of false-positives; outputting the matched text will help you identify them.

Case-sensitive regex matching should be the default.
I think \b...\b as a regex pattern will give the kind of "whole word" matching you need.
pandas.extract will do the capturing.

Given the size of your datasets, you may be a bit concerned with the performance. Or you may not, it's up to you.
I haven't tested this at all:
# Import datasets and create lists/variables
import pandas as pd
from pandas import ExcelWriter
from typing import Iterable

# Document, sheet, and column names:
names_source_file = 'names.xlsx'
first_names_sheet = 'Alle Navne'
first_names_column = 'Names'
last_names_sheet = 'Frie Efternavne'
last_names_column = 'Frie Efternavne'
subject_file = 'Entreprise Beskeder.xlsx'
subject_sheet = 'dataark'
subject_column = 'Besked'
output_first_name = 'Navner'
output_last_name = 'Efternavner'
output_file = 'PythonExport.xlsx'

# Build (very large!) search patterns:
first_names_df = pd.read_excel(names_file, sheet_name=first_names_sheet)
first_names: Iterable[str] = namesdf[first_names_column]
first_names_regex = '''\b{}\b'''.format('|'.join(first_names))
last_names_df = pd.read_excel(names_file, sheet_name=last_names_sheet)
last_names: Iterable[str] = lastnamesdf[last_names_column]
last_names_regex = '''\b{}\b'''.format('|'.join(last_names))

# Import dataset and drop NULLS:
data_frame = pd.read_excel(subject_file, sheet_name=subject_sheet)
data_frame[subject_column].dropna(inplace=True)

# Add columns for found first and last names:
data_frame[output_first_name] = data_frame[subject_column].str.extract(
    first_names_regex,
    expand=False
)
data_frame[output_last_name] = data_frame[subject_column].str.extract(
    last_names_regex,
    expand=False
)

# Save the result
writer = ExcelWriter(output_file)
df.to_excel(writer)
writer.save()

One obvious problem that I still haven't talked about is that there may be multiple name matches in a given subject. Assuming that you care about multiple matches, you can probably do something with extractall.


Simple C# multiplication table program I'm pretty new to C# and have decided that I wanted to make a program that calculate the multiplication table of a selected number to practice on loops etc. I'm pretty satisfied, but I wondered if there is anything I can do to make the code better and more concise.

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;

namespace Multiplication
{
    class Program
    {
        static void Main(string[] args)
        {
            Console.WriteLine("Write a number and I'll calculate the multiplication table of the number");
            int userValue_number = int.Parse(Console.ReadLine());

            Console.WriteLine("How far do you want to go? E.g. 12, then {0} x 12 is the highest I will go ", userValue_number);
            int userValue_length = int.Parse(Console.ReadLine());

            int n = userValue_number;
            int i = 1;
            while (n <= (userValue_number * userValue_length))
            {
                Console.WriteLine("{0} x {1} = {2}", i, userValue_number, n);
                n=n+userValue_number;
                i++;
            }
            Console.ReadLine();
        }
    }
}

int.Parse will throw an exception if input isn't an integer. Consider using int.TryParse instead.

You should verify that the user doesn't enter a negative integer for the 2nd argument.

Your while loop should be a for loop.

A multiplication isn't much more expensive than an addition, especially compared with the performance cost of calling Console.WriteLine; so I'd use multiplication instead if that makes the code clearer:

for (int i = 1; i <= userValue_length ; ++i)
{
    Console.WriteLine("{0} x {1} = {2}", i, userValue_number, i * userValue_number);
}

userValue_number is a strange mix of camelCase and using_underscores. camelCase is more conventional in C#. I think I'd rename those to chosenNumber and chosenTimes or something like that.

The final Console.ReadLine(); is ugly and uncessary. You probably use it for debugging; instead (to inspect output from a debugger session before the console window auto-closes) put a debugger breakpoint on the final curly brace of main.

Your message E.g. 12, then {0} x 12 is ... doesn't match your output {0} x {1} = ...; instead i should be the 2nd (not 1st) number in your output.


Groups/Communities in Facebook snap dataset using networkx in python I was using facebook dataset and making a graph on it. But was not been able to find different groups/communities in it. The code was using is this:

import networkx as nx
import matplotlib.pyplot as plt

g = nx.read_edgelist('facebook_combined.txt', create_using=nx.Graph(), nodetype=int)
print nx.info(g)

sp = nx.spring_layout(g)
nx.draw_networkx(g, pos=sp, with_labels=False, node_size=35)
# plt.axes('off')
plt.show()

The result i was getting is this:

But was not been able to find groups or communities in it.

The link to the dataset is here

The source to the is here

After searching and researching to find the solution to this question i came up with this answer:

To find/detect communities in facebook snap dataset:

import networkx as nx
import matplotlib.pyplot as plt
import community

'''Exploratory Data Analysis'''
g = nx.read_edgelist('facebook_combined.txt', create_using=nx.Graph(), nodetype=int)
print nx.info(g)

'''Simple Graph'''
# sp = nx.spring_layout(g)
# nx.draw_networkx(g, pos=sp, with_labels=False, node_size=35)
# # plt.axes("off")
# plt.show()

'''Graph with communities dectection'''
spring_pos = nx.spring_layout(g)
parts = community.best_partition(g)
values = [parts.get(node) for node in g.nodes()]

plt.axis("off")
nx.draw_networkx(g, pos=spring_pos, cmap=plt.get_cmap("jet"), node_color=values, node_size=35, with_labels=False)
plt.show()

this will give the following output:


Counting Ways to Make Change -- Is this good functional/Lisp style? I have just started learning some Scheme this weekend. I recently solved a problem that goes something like:

Count the number of ways possible to give out a certain amount of change using
1 5 10 25 and 50 cent pieces.
--SICP 1.16

So my code to this question looks like this:

(define (make-change amount)
  (define coin-total 5)
  ;;#Function that actually does the calculating
  (define (calculate amount coins)

    ;;#Lookup values on the table vector, if none is found find it and add it.
    (define (lookup)

      ;;#Location on the table vector
      (define position (- (+ coins (* coin-total (- amount 1))) 1))

      ;;#Get the value in the table vector
      (define (search-for amount coins)
        (vector-ref table position))

      ;;#Not quite so functional, but we need to add it somehow
      (define (update val)
        (begin (vector-set! table position val)
               val))

      (let ((result (search-for amount coins)))
        (if (= result 0)
          (let* ((left-branch (calculate amount (- coins 1)))
                 (right-branch (calculate (- amount (coin-value coins)) coins)))
           (update (+ left-branch right-branch)))
          result)))

    (cond ((= amount 0) 1 )
          ((or (< amount 0) (= coins 0)) 0)
          (else (lookup))))

  ;;#Table stuff
  (define table (make-vector (* amount coin-total) 0))
  (define (init-table position)
    (if (< position 0)
      0
      (begin (vector-set! table position 1)
             (init-table (- position 1)))))

    ;;#The entry point, sets up the table and starts the calculation
    (begin (init-table 4) (calculate amount coin-total)))

    ;;#Mapping from coin number to value, order doesn't matter
    (define (coin-value n)
      (cond ((= n 1) 1)
          ((= n 2) 5)
          ((= n 3) 10)
          ((= n 4) 25)
          ((= n 5) 50)))

The logic in this code is sound, it "works" but I'm worried that I'm not writing it very idiomatically. What suggestions or alterations would you make for this code?

Note: I stuck this # in the comments here so that the code highlighting would stop treating it like code and messing up the syntax highlighting

Well, I could try to rewrite it to not use mutation, but it looks like the mutation is just there as a memoization optimization.  You could either pass the vector as an argument into calculate, or you could abstract the memoization via something like define/memo provided in memoize.plt, or just roll your own customized one as done here, which I think is just fine.

There are some surface changes you can make as well.  For starters, the search-for function doesn't do anything with its arguments, so they can be removed.  Next, defines have an explicit begin around the body, so the begin in update can be removed as can the begin near what your comments call the entry point.

The (init-table 4) is an optimization (works without it) that I'm not sure is even worth putting in; especially since I think it deserves a comment to explain that the magic number 4 is because (make-change n) should return 1 for 1 <= n <= 4. If you decide to leave it in, at least replace it by (vector-fill! v 1 0 4) assuming your using srfi-43.  It might be different if your scheme dialect has it's own vector libs.

Finally, mapping coin number to value could use case  instead of cond for a little less syntax as in:

(define (coin-value n)
  (case n
    ((1) 1)
    ((2) 5)
    ((3) 10)
    ((4) 25)
    ((5) 50)))


Optimising two for loops in Python I would like to make this code more efficient, I have isolated the problem as being these two for loops:

    for i, device in enumerate(list_devices):
        for data_type_attr_name in data_types:
            result_list_element = {
                "device_reference": device.name,
                "device_name": "REF - " + device.name,
                "data_type": data_type_attr_name,
                "type": next(
                    (data_type["type"] for data_type in DATA_TYPES if data_type["name"] == data_type_attr_name)
                ),
                "data_points": getattr(device, data_type_attr_name)(
                    is_last_value=is_last_value,
                    from_timestamp=from_timestamp,
                    to_timestamp=to_timestamp,
                    aggregate_period_name=aggregate_period_name,
                    aggregate_operation_name=aggregate_operation_name,
                    decimal_places=decimal_places,
                ),
            }
            if not isinstance(result_list_element["data_points"], list):
                raise TypeError("`data_points` must be returned as a list, even if it contains only one element.")
            result_list.append(result_list_element)

    return result_list

list_devices is a list of Django model objects, data_types is a list of strings, each one representing a data type.

Is there any way of losing one of the for loops while maintaining the same output?

Thanks

Assume you are referring to speed, not conciseness when asking about efficiency. In which case reducing the amount of loops will not necessarily improve performance.

"type": next((data_type["type"] for data_type in DATA_TYPES if data_type["name"] == data_type_attr_name))

This can be improved so that you are not needing to go over DATA_TYPES looking for a matching name to data_type_attr_name every iteration. Instead you can make a reverse lookup dictionary once before the loop where data_type["name"] is the key.

Whenever you are searching through a list often, it can be very beneficial to create a dictionary.


Comparing cells of two Worksheets With the following Code I try to compare the content of two Worksheets.
The first column contains a key that is unique and the following columns contain  data that I want to compare (My data sets contain around 7000 rows and up to 40 columns).

My question is if there is a way to make the code run faster. Since the program has to find the key first, it takes longer and longer the further down the row with the key is on the worksheet. A second thing I noticed is that the code generally runs a lot faster with Excel 2007 than 2013. Any ideas why?

Count = 1
LastRowNew= Sheets("Data_new").Cells(Rows.Count, "A").End(xlUp).Row
LastRowOld = Sheets("Data_old").Cells(Rows.Count, "A").End(xlUp).Row
LastColumn = Sheets("Data_old").UsedRange.Columns.Count
For i = 1 To LastRowOld
Set rngA = Sheets("Data_new").Range(Sheets("Data_new").Cells(1, 1), Sheets("Data_new").Cells(LastRowNew, 1)).Find(Sheets("Data_old").Cells(i, 1), lookat:=xlWhole)
If Not rngA Is Nothing Then
    Count = Count + 1
    Sheets("Comparison").Cells(Count, 1).Value = Sheets("Data_old").Cells(i, 1)
    For j = 2 To LastColumn
        If Sheets("Data_old").Cells(i, j).Value <> Sheets("Data_new").Cells(rngA.Row, j).Value Then
            Sheets("Comparison").Cells(Count, j).Value = "NOK"
        Else
            Sheets("Comparison").Cells(Count, j).Value = "OK"
        End If
    Next j
Else
    Count = Count + 1
    Sheets("Comparison").Cells(Count, 1).Value = Sheets("Data_old").Cells(i, 1)
    Sheets("Comparison").Cells(Count, 2).Value = "Doesn't exist in old system"
End If
Application.StatusBar = "Comparing entries " & Format(i / LastRowOld, "0 %") & "..."
Next i

The short answer is don't use VBA at all. Use native Excel formulas. We like to code, so it's natural that we all too often reach for the IDE when we should be heading to the formula bar.

Consider sheet Data_new

And sheet Data_old

You can use a VLookup from Data_old to find the corresponding values in DataNew.

=VLOOKUP(Data_new!$A2,Data_new!$A:$B,2,FALSE)

And then wrap the lookup in an IF formula to populate the final result.

=IF(B2 = VLOOKUP(Data_new!$A2,Data_new!$A:$B,2,FALSE),"OK","NOK")

resulting in the new & improved sheet Data_old.


Class for formatting topics using BBCodes I'm developing a program for users from a VBulletin's forum. This forum has a lot of people interested in write good articles but some of them don't know how to use the formatting bar in his favor. Sometimes, the others members don't read this threads/articles because it doesn't have a interesting formatting (crazy world, huh?).
So here's my idea:

Idea

The user don't need to worry about formatting the thread, so he can be focused just on the writing and the content. The formatting part (the boring part) will be generated by the program, the user just need choose colors for titles and text content.

Code

I've created a class named Markup where all models for formatting will be set.

public class Markup {}

Inside this class I created a private enum to define Which BBcodes are accepted by VBulletin, something like:

enum BBcode {
  BOLD("B"),
  ITALIC("I"),
  IMAGE("IMG"),
  UNDERLINE("U"),
  VIDEO("VIDEO");
  /* many others... */

  final String code;
  private BBcode(String code) {
    this.code = code;
  }

  public String code(){
    return this.code;
  }
}

Still inside Markup class I've created a private class named Syntax to define how the BBcode syntax will created., like this:

class Syntax {
  public String in(BBcode bbcode){
    return "[" + bbcode.code() + "]"; // will return "[BOLD]" for example.
  }

  public String inValued(BBcode bbcode, String value){
    return "[" + bbcode.code() + "=" + value + "]"; // will return "[COLOR=#333]" for example.
  }

  public String out(BBcode bbcode){
    return "[/" + bbcode.code() + "]"; // will return "[/BOLD]" for example.
  }
}

So I can handle it like:

public String setContents(BBcode bbcode, String contents){
  return syntax.in(bbcode) + contents + syntax.out(bbcode); // will return "[BOLD]foo[/BOLD]
}

My questions:

The names for class and methods are ok? Other peoples can be part of the project and I want them to know what the class does.
All this stuff are inside the Markup class (because the BBcode and the Syntax will be util just for the Markup). It's a good practice group similar things inside a unique class?

First of all you make 1 big fault by saying that your classes inside MarkUp are private.
They aren't, they are package private, more info on site of oracle.

Then, let's start with the enum.
Your final String code is also package private. => change them to private.
Your getter for the code is also a little off the normalisation, let's try to call it getCode.

Final result could be :

private enum BBcode {

    BOLD("B"),
    ITALIC("I"),
    IMAGE("IMG"),
    UNDERLINE("U"),
    VIDEO("VIDEO");
    /* many others... */

    private final String code;

    private BBcode(String code) {
        this.code = code;
    }

    public String getCode() {
        return this.code;
    }
}

The class Syntax you could add constants in stead of repeating your chars.
Then also, you have repeating stuff what you can put together.
The naming in, out are for me to broad.

Example of how I should make the class :

private class Syntax {

    private static final char START_OPENING_TAG = '[';
    private static final String START_CLOSING_TAG = "[/";
    private static final char CLOSING_TAG = ']';
    private static final char EQUAL_SIGN = '=';

    public String getOpeningTag(BBcode bbcode) {
        return addStartOpeningTag( bbcode.code()); // will return "[BOLD]" for example.
    }

    public String getOpeningTag(BBcode bbcode, String value) {
        return addStartOpeningTag(bbcode.code() + EQUAL_SIGN + value); // will return "[COLOR=#333]" for example.
    }

    public String getClosingTag(BBcode bbcode) {
        return addStartClosingTag( bbcode.code()); // will return "[/BOLD]" for example.
    }

    private String addStartClosingTag(String value) {
        return START_CLOSING_TAG + value + CLOSING_TAG;
    }

    private String addStartOpeningTag(String value) {
        return START_OPENING_TAG + value + CLOSING_TAG;
    }
}

As you can see now, I can easily refactor the code.
Let's say you class grows and we need to change the ] to >
For me it's one little change and it's completly done.
Your old class, you have already to change it 3 places, possible fault's .(forgetting one to change) waiting to happen when your class grows.


Generating a histogram efficiently from the datasets in a Map I have a map in which I store total bytes as the key and count of users as the value:

Map<Integer, Integer> userHistogramInfo = new HashMap<Integer, Integer>();

Now I need to make a histogram as mentioned below:

How many users have data with 200,000 bytes (200 kilo bytes)
How many users have data with 500 bytes
How many users have data with 200 bytes
How many users have data with 100 bytes

As an example it might be possible that 100 million users have data of 500 bytes. I need to make a histogram of current data usage of the users.

We can define a certain bucket size under which I need to find out how many users have data within that bucket.

public void generateHistogram(Map<Integer, Integer> userHistogramInfo) {
    int[] definitionInBytes = { 10, 20, 40, 80, 160, 320, 640, 1280, 2560, 5120, 10240, 20480, 40960, 81920 };
    long[] buckets = new long[definitionInBytes.length];
    System.out.println("Below is the Histogram List sorted on the bytes: \n");
    System.out.println("BytesStored           Number");
    SortedSet<Integer> keys = new TreeSet<Integer>(userHistogramInfo.keySet());
    for (Integer key : keys) {
        Integer value = userHistogramInfo.get(key);
        System.out.println(key + "                      " + value);
    }

    System.out.println();
    for (Integer time : userHistogramInfo.keySet()) {
        for (int i = definitionInBytes.length - 1; i >= 0; i--) {
            if (time >= definitionInBytes[i]) {
                buckets[i] += userHistogramInfo.get(time);
                break;
            }
        }
    }
    for (int i = 0; i < definitionInBytes.length; i++) {
        String period = "";
        if (i == definitionInBytes.length - 1) {
            period = "greater than " + definitionInBytes[i] + " bytes";
        } else {
            period = "between " + (definitionInBytes[i] + 1) + " and " + definitionInBytes[i + 1] + " bytes";
        }
        System.out.println(buckets[i] + " came back " + period);
    }
}

Is there any better way of doing this and making the histogram? I need to get a break down of how many users have x bytes of data. I am doing the right thing by choosing the bucket size?

Summary

Fix the bugs.
Make sure your source code can be understood.
Further improve your symbol names.
Write more, shorter methods.
Use Formatter.
Use smarter algorithms.
Use Stream.collect() to create the second histogram.
Unit test.

Fix the bugs

When you have a user which consumes less than 10 bytes of data, you don't count that. I think that's a bug. I cannot conclude from the way the source code is written that this behavior is intentional. You probably want to

start definitionInBytes with 0
assert that the key in the map is >= 0

or

assert that the key in the map is >= 10

This would also be useful for testing. You would want that the sum of values (total number of users) in the map and the buckets is same, that's an invariant.

You could assert that invariant:

assert userHistogramInfo.values().mapToInt(Integer::intValue).sum() == Arrays.stream(buckets).sum();

If it's actually intentional that it doesn't start with zero, you might consider documenting that. Right now, it too much looks like programming by coincidence and as if it might fall over sooner or later.

Besides, depending on your user base, you actually might want to go for Long instead of Integer. With Integer you can use only 2 Billion users. Given a world population of soon 8 Billion, of which soon everyone has internet access, if you manage to get everyone as user, your code might overflow. Not yet very likely, but think of the COBOL (and other) code that was written with 2-digit dates (sometimes for good reason like space) and then had to be changed later to prevent Y2K trouble. Our code often lives quite long. Why not prepare it for the future if the cost for that in the present is almost nothing.

Understandable source code

This is mostly met. I had no trouble whatsoever understanding your source code.  A few things how I could have understood your source code even faster are mentioned in different points. Just there's one thing in the source code which completely puzzles me. It's this comment:

//This is for database select

I do not see any database or query in context, and I also do not see how the loop that follows this comment would be related to a database query. I'm left puzzled by that comment. Puzzlement without resolution.

Good symbol names

I find the name userHistogramInfo ambiguous and therefore confusing, because the method actually deals with two histograms.

Avoid info, data and such stuff in symbol names, it does not communicate intent, it just adds noise.
For Maps communicate the key and value in the symbol name.

Maybe usersPerSize could be a good name for the input histogram. And usersPerSizeRange could be a good name for the second histogram which is generated from the first.

Use smaller methods

Your method generateHistogram() is too long. Ideally, methods do just one thing, they do it well, and they do it only (Robert C. Martin, Clean Code). It's also known as the SRP - Single Responsibility Principle applied to methods, or "extract 'till you drop" (Robert C. Martin, Clean Code).

Let's see what this method does:

It formats the original histogram.
It prints the original histogram.
It generates a new histogram, based on ranges instead of individual sizes.
It formats the new histogram.
It prints the new histogram.

That's far too much for just one method.
Each of these things should go in a separate method.

Don't be afraid of small methods in Java. The JIT of the JVM is taking care of optimization very very well.

Use Formatter

Formatter is a very useful class for formatting Strings. You don't need to care about the platform's line ending, you use "%n" and Java takes care of the rest. And you have all the usual formatting options, like auto-filling with spaces or leading zero. Hosch250 already explained this nicely in his answer.

Use smarter algorithms

Your array definitionInBytes is sorted. The expectation of most programmers would probably be that if you have a sorted array, you use binary search to identify the location, however, you use linear search.

You could use Arrays.binarySearch() for your use case.

Here's a method which gets the bucket index for a value:

public static int getBucketIndex(final int value) {
    // assert isSorted(definitionInBytes);
    final int searchResult = Arrays.binarySearch(definitionInBytes, value);
    if (searchResult >= 0)
        return searchResult;
    // Warning: returns -1 in case value is less than definitionInBytes[0];
    return -searchResult - 2;
}

Use Stream.collect() to create the second histogram.

This is not really a must. It just might be interesting for you to see what Java can do for you.

Actually, your two arrays definitionInBytes and buckets are a histogram, a Map<Integer, Integer>, just like the input data.
You're grouping the input keys by ranges, summing the values. You could let Java do that for you.

Here's a method which creates a Map<Integer, Integer> from a Map<Integer, Integer> based on specified floors (your definitionInBytes):

import static java.util.stream.Collectors.*;

public class Histogram {
    // ...
    private static final int[] definitionInBytes = { 10, 20, 40, 80, 160, 320, 640, 1280, 2560, 5120, 10240, 20480, 40960, 81920 };

    public static Map<Integer, Integer> groupByRange(final Map<Integer, Integer> usersPerSize) {
        return usersPerSize
            .entrySet()
            .parallelStream()
            .collect(groupingByConcurrent(Histogram::getBucketFromEntry, summingInt(Entry::getValue)));
    }

    public static Integer getBucketFromEntry(final Entry<Integer, Integer> entry) {
        return getFloor(entry.getKey(), definitionInBytes);
    }

    public static int getFloor(final int value, final int[] floors) {
        final int searchResult = Arrays.binarySearch(floors, value);
        if (searchResult >= 0) return floors[searchResult];
        if (-searchResult - 2 < 0) return Integer.MIN_VALUE;
        return floors[-searchResult - 2];
    }
}

Unit Test

When you have everything in one big method, that's of course difficult. If that method is not returning any data but just printing the data, unit testing is even more difficult.

When you write small methods, and ensure that presenting the data is only done at the top-most levels of your call hierarchy, you can unit test all the methods that contain the business logic.

Also, the methods then can be reused.

For example, getFloor() above can be tested individually, whereas the parts of your big method cannot be tested individually.

Ideally, even write the tests first.

Here's an example how to test getFloor():

private static final int[] floors = { 10, 100, 1000 };

@Test
public void testGetFloor() {
    assertEquals(Integer.MIN_VALUE, getFloor(Integer.MIN_VALUE, floors));
    assertEquals(Integer.MIN_VALUE, getFloor(0, floors));
    assertEquals(Integer.MIN_VALUE, getFloor(9, floors));
    assertEquals(10, getFloor(10, floors));
    assertEquals(10, getFloor(99, floors));
    assertEquals(100, getFloor(100, floors));
    assertEquals(100, getFloor(999, floors));
    assertEquals(1000, getFloor(1000, floors));
    assertEquals(1000, getFloor(Integer.MAX_VALUE, floors));
}