Write a program that simulates a lottery I am taking a C++ course and we have received our final assignment, I was looking for some suggestions and or tips so I can continue to improve.

I am aware that the use of using namespace std is bad practice but my professor prefers that we use this way since it is a beginners course.

The assignment was a two part assignment I have commented out the piece of the code that was unneeded for the second part.

Part One:

  Write a program that simulates a lottery. The program should have an array of 7 integers
  named winningDigits, with a randomly generated number in the range of 0 through 9 for each element in the array. The program should ask the user to enter 7 digits and should store them in a second integer array named player. The program must compare the corresponding elements in the two arrays and count how many digits match.

  For example, the following shows the winningDigits array and the player array with sample numbers stored in each. There are two matching digits, elements 2 and 4.

  WinningDigits:    7    4    9    1    3    5    0

  Player: 4    2    9    7    3    4    2

Part Two:

  Modify the lab solution so that the player numbers are entered not from the keyboard, but from the file player.dat (you will need to create this file and add 7 numbers).

What can I do better?

#include <iostream>
#include <fstream>
using namespace std;

int main()
{
    const int size = 7;
    int winningDigits[size];
    int player[size];
    int count=0;
    ifstream datafile;

    datafile.open("player.dat");

    for(int i=0; i<size; i++)
    {
        winningDigits[i]=rand()%10;
        datafile >> player[i];
        if (winningDigits[i]==player[i])
        count++;
    }
/*
for(int i=0; i<size; i++)
{
    winningDigits[i]=rand()%10;
    cout << "Enter number " << i+1 << " ";
    cin >> player[i];

    if (winningDigits[i]==player[i])
        count++;
}
*/
    cout << "WinningDigits: ";
    for(int i=0; i<size; i++)
    cout << winningDigits[i] << " ";

    cout << "\nPlayer:        ";
    for(int i=0; i<size; i++)
    cout << player[i] << " ";

    cout << " \nThere are " << count << " matching elements ";
    for(int i=0; i<size; i++)
    if(winningDigits[i]==player[i])
    cout << i+1 << " ";
    cout << "\n";
}

There's quite a few things I would change here. Getting quality random number generation right is difficult, (these questions come up a lot so it's something I've blogged about), but thankfully with the newer c++11 standard things are a fair bit easier.
Program Correctness
Making sure that your program is correct is extremely important. As it stands this program is not correct (as in does not meet the specification):
cout << " \nThere are " << count << " matching elements ";
for(int i=0; i<size; i++){
    if(winningDigits[i]==player[i]){
        cout << i+1 << " ";
    }
}
cout << "\n";

This comparison assumes that the arrays are sorted, which is not the case. So matching elements could be completely missed here. Sort the arrays first before comparing them or consider a different data structure to store the results from generating the numbers.
Basic things:
There's commented out code in the source, this is almost always a bad sign. Keep code changes tracked by version control, not commented out blocks.
Loops like this are a nightmare formatting wise:
for(int i=0; i<size; i++)
cout << winningDigits[i] << " ";

I personally always use braces (as does the coding standard we go by at my work) but whatever you do you need a tab here:
for(int i=0; i<size; i++){
    cout << winningDigits[i] << " ";
}

Another thing is this:
using namespace std;

Other people have commented on this, but generally speaking the only time you would really want to do this is when you are making some small throwaway program for testing a concept or making an example or similar. You probably don't want to do it in any production code. This pollutes the namespace which is something you don't want, see this https://stackoverflow.com/questions/1452721/why-is-using-namespace-std-considered-bad-practice for more info on that.
If you want to cut down on on typing you can bring in just the names you need by doing:
using std::cout;
using std::cin;

an so on.
Standard library containers
Generally speaking with c++ you don't want to use raw arrays, the standard container classes are safer and allow you to be more productive. There's a c++ FAQ article that deals with just this, so give that a read.
int winningDigits[size];
int player[size];

Instead of these I would use std::array for both of these as the length is both fixed and known at compile time. Were then lengths not known then std::vector would most likely be the best choice. These become:
#include <array> //header needed for std::array

std::array<int, size> winningDigits;
std::array<int, size> player;

Now this ends up being the same type in a bunch of different places so a typedef will help make things easier:
typedef std::array<int, size> lottery_results_t;
lottery_results_t winningDigits;
lottery_results_t player;

An additional benefit of the typedef is that if you do change the type later it will be easier to do so.
Random number generation
rand() is a really bad random number generator, to the point where the standard committee actually made a report about depreciating it. Additionally rand() gets worse when you take the modulus of it (statistically speaking).
We can avoid all these problems by just using the newer c++ random number library in a manner like this:
std::default_random_engine generator;
generator.seed( time(0) );
std::uniform_int_distribution<int> distribution(0,9);//note the min and max parameters are inclusive here
while(true)
{
    std::cout << distribution(generator) << std::endl;
}

Demonstration here: http://ideone.com/xzgcZy
The output code definitely violates the "don't repeat yourself" principle. Success200 has posted a great answer that outlines some of the things you can do to reduce that duplication.


Test whether a type T is among the types of a tuple On SO, I asked for a way to do it without variadics. I also provided a typical "normal" implementation with variadics:

template <typename V, typename T>
struct Is_in_tuple;

template <typename V, typename T0, typename... T>
struct Is_in_tuple <V, tuple<T0, T...> >
{
  static const bool value = Is_in_tuple<V, tuple<T...> >::value;
};

template <typename V, typename... T>
struct Is_in_tuple <V, tuple<V, T...> >
{
  static const bool value = true;
};

template <typename V>
struct Is_in_tuple <V, tuple<> >
{
  static const bool value = false;
};

I'd like a review on the above code. Is it safe / OK / missing something, before I make it my standard way of querying type lists?

Since you're implementing a traits-like check, I would typically expect your traits class types to be derived from std::true_type and std::false_type. This is certainly not required, and overall this implementation doesn't bug me. I was slightly surprised to see std::tuple<> called out explicitly, but that doesn't seem to change the overall correctness in any way.

The biggest question I have is how exact you want your type matching to be. Do you require exact type matches like std::is_same<T, U> or usably similar types like std::is_convertible<T, U>? To be concrete, what should Is_in_tuple<decltype(0), std::tuple<unsigned int>>::value be? It would be great to have some unit tests that document this sort of edge case.

The only thing I see wrong with your implementation is by implication: either you must have included some form of using, either using namespace std; or using std::tuple;, or you are dependent on someone else having done so before including this header. While correct use of using is fine, particularly in implementation files or inside functions, I would strongly suggest avoiding it here where it will pollute (or require previous pollution of) the compilation unit of whatever code includes this.


Generating lists of sequences of specified lengths I'm writing a program that uses a Genetic Algorithm to "intelligently brute force" our schedule at work. We typically have issues with creating schedules, so I thought using my new GA library to tackle the problem would be a neat exercise.

One of the very first functions I wrote seems overly complex though. It works as intended, but I worry that it could be simplified by making use of higher-order looping constructs instead of a raw loop:

(defn cut-genes-into-day-blocks
  "Cuts the genes into blocks. Each block will have as many genes are there are shifts that day.
  shifts-per-day should be a list (whose length matches the period length) containing how many shifts exist for each day of the period.

  For example, if a period is 14 days, and there are 5 shifts on weekdays, and 2 shifts on weekends,
   shifts-per-day would be [5 5 5 5 5 2 2 5 5 5 5 5 2 2]."
  [genes shifts-per-day]
  (loop [rem-genes genes
         [n-shifts & rem-days] shifts-per-day
         acc []]

    (if n-shifts
      (let [[block rest-genes] (split-at n-shifts rem-genes)]
        (recur rest-genes
               rem-days
               (conj acc block)))

      acc)))

As the docs mention, the purpose of this function is to take a gene sequence, and cut it into blocks where each block has as many elements as there are shifts in that day. An example usage (using dummy genes created by range) is:

(let [shifts-per-day [5 5 5 5 5 2 2 5 5 5 5 5 2 2]
      total-shifts (reduce + 0 shifts-per-day)
      genes (vec (range total-shifts))]
  (cut-genes-into-day-blocks genes shifts-per-day))

[(0 1 2 3 4) ; <---- Each number represents a person working that day
 (5 6 7 8 9) ; <---- 5 different people/shifts on weekdays...
 (10 11 12 13 14)
 (15 16 17 18 19)
 (20 21 22 23 24)
 (25 26) ; <---- ... and 2 shifts on the weekends
 (27 28)
 (29 30 31 32 33)
 (34 35 36 37 38)
 (39 40 41 42 43)
 (44 45 46 47 48)
 (49 50 51 52 53)
 (54 55)
 (56 57)]

This function is overly chunky though and contains a few things that I've run into in the past that I'd like suggestions on:

The "deconstruct the head from tail, then check if the first element is valid" "pattern". I use it a lot in my code whenever I'm iterating over a list using loop. Use the rest operator to pop the first element off, then check if it's actually valid before using it.
The fact that the loop requires 3 "state variables" is unfortunate. It would be nice if the accumulator was implicit somehow. I know that using reduce over shifts-per-day would et rid of one of the bits of information needing to be maintained, but there are two accumulators in use here (acc and rem-genes), and I've found reducing when more than one accumulator is necessary to be very messy and not worth the trouble.
Any other tips that you think are worth mentioning would be appreciated.

Your function seems very well suited to the Clojure iterate function, which takes a function that turns state into state:

(def shifts [5 5 5 5 5 2 2 5 5 5 5 5 2 2])
(def total-shifts (apply + shifts))
(def genes (vec (range total-shifts)))

(defn transition [[answer shifts-remaining genes-remaining]]
  (let [[num-shifts & more-shifts] shifts-remaining
        [genes-in-day rest-of-genes] (split-at num-shifts genes-remaining)]
    [(conj answer genes-in-day) more-shifts rest-of-genes]))

(->> (iterate transition [[] shifts genes])
     (drop (count shifts))
     ffirst)


Command-line implementation of Mastermind I just finished this command-line implementation of the Mastermind board game that allows two players to play against each other. I'm fairly new to Ruby, but I think I did a decent job with this. However, I'm sure there are some things I could've done better, so please let me know if there are.

class Codebreaker
  attr_accessor :colors

  def initialize
    @colors = []
  end
end

require 'io/console'

class Codemaker
  attr_accessor :code_colors

  def make_code
    puts "\nCodemaker, select 4 colors! Please enter them separated by spaces, with no commas.  To prevent cheating, your input will be hidden.  Try to not make any typos :)"
    input = STDIN.noecho(&:gets).chomp
    @code_colors = input.split(" ")
    puts "Excellent!"
  end
end

class Game
  require_relative "codebreaker.rb"
  require_relative "codemaker.rb"

  def initialize
    @code_breaker = Codebreaker.new
    @code_maker = Codemaker.new
    @turn = 1
    start_game
  end

  def start_game
    puts "Welcome to Mastermind!
    \nRules:
    \nThe codemaker creates a code consisting of four colors chosen from the provided list.
    It is the goal of the codebreaker to crack the code in 12 turns, entering the same exact colors in the same order.  To attempt to crack the code, simply
    keep guessing.
    \nYou may choose from the following colors: red, blue, green, yellow, orange, purple, white, and black."
    @code_maker.make_code
    turn
  end

  def turn
    while @turn < 12
      code_breaker_input
      compare
      @turn += 1
      if @turn == 12
        puts "Unfortunately, you didn't guess the codemaker's code in the allotted 12 turns.  Better luck next time!"
        exit
      end
    end
  end

  def code_breaker_input
    puts "\nCodebreaker, please make your selection! Enter colors in lowercase, separated by spaces, with no commas."
    @code_breaker.colors = gets.chomp
  end

  def compare
    b_colors = @code_breaker.colors.downcase.split(" ")
    m_colors = @code_maker.code_colors
    correct_indexes = []
    correct_colors = []

    b_colors.zip(m_colors).map { |a, b|
      if a == b
        correct_indexes << "X"
      else correct_indexes << "O"       end
    }

    b_colors.each { |color|
      m_colors.any? { |c_color|
        correct_colors << color if c_color == color
      }
    }

    puts correct_indexes.join(" ")
    puts "Colors guessed correctly: #{correct_colors.join(", ")}"

    #checks win condition
    if correct_indexes.all? { |index| index == "X" }
      puts "The codemaker cracked the code!"
      exit
    end
  end
end

game = Game.new
```

Simple things you can improve:

12 is a magic number in your code, you can move it to a constant.
I think TURNS.times do |turn| ... is better than the while loop because you don't have to keep track of the counter.
When you're passing a simple space (" ") to the method split you can just call split instead, without passing any argument.
When the block has multiple lines it is a good practice to use do and end instead of brackets.
I think you can use the select method and assign the result directly to the correct_colors variable.
I think it would be better to call Game.new.start_game instead of Game.new, for me, it isn't intuitive that the game will begin just by instantiating the Game class.
You can make some methods private because you are not calling them on the instance.


Create individual files via Bash with MD5 hashes of all files in a directory recursively I have an archive of photos stored in a directory tree on my Mac like:

./2016/05/17/photo-312.jpg
./2016/05/19/photo-1234.jpg
./2016/05/19/photo-5678.jpg

I want to create MD5 hashes of each file that can be used to verify the photos have not been altered or corrupted. My goals are:

One MD5 file per photo
Store the MD5 files in the same directory their corresponding photos
Use the same base name as the photo, but switch the extension to .md5
Capture only the hash value (e.g. b1046abbe7bbf2a2473e9489599f38e0) without any trailing spaces or newlines

For example, the above directory structure would look like this after the process runs:

./2016/05/17/photo-312.jpg
./2016/05/17/photo-312.md5
./2016/05/19/photo-1234.jpg
./2016/05/19/photo-1234.md5
./2016/05/19/photo-5678.jpg
./2016/05/19/photo-5678.md5

(Note: I only need to run this process one time. The process I use to move photos into the archive will create the necessary MD5 files for new photos from this point forward.)

Here's the one-liner I came up with:

find . -type f -name "*.jpg" -exec bash -c 'printf "%s" $(md5 -q "$0") > "${0%.*}.md5"' {} \;

(Note: my machine has md5 instead of md5sum which I often see referenced. So, I'm using that.)

Here's a few details on how I understand this to work:

The first section runs a basic find command on the current directory (i.e. ".") looking for .jpg files and sends them to bash with -exec bash -c

find . -type f -name "*.jpg" -exec bash -c

Bash runs printf to setup for a string that doesn't have a newline:

printf "%s"

This section generates the hash that is used to feed the string into printf:

$(md5 -q "$0")

The -q flag tells md5 to output only the hash instead of the standard MD5 output which would look something line:

MD5 (photo-312.jpg) = b1046abbe7bbf2a2473e9489599f38e0

The value of $0 is the relative path to the source .jpg file that find sent to bash.
This section creates the file path to store the value in where the original extension is replaced by .md5:

"${0%.*}.md5"

More details about what's going on there can be found in the ${parameter%word} section of the Bash Manual.
The last little bit is:

{} \;

I'm not sure why, but the {} is necessary to make this run. (My understanding is that it's a reference to the file path. I don't know how that ties in, but md5: bash: No such file or directory errors pop up if it's not there.)

Finally, the \; identifies the end of find's -exec.

While I normally use other languages for this type of work, I decided to try this with bash to get some practice with it. I've done some basic testing and everything appears to work as expected. Given my infrequent use of bash, I'd like to make sure I'm not getting myself in trouble. So, my questions are:

Are there any gotchas in this code that are waiting to bite me?
Is there a more standard or efficient way to do this?

UPDATE: I modified my code based on the answers. In case it's useful, here's what I ended up with:

find . -type f \( -name '*.cr2' -or -name '*.jpg' \) -execdir sh -c 'sha1sum "{}" > "${1%.*}".sha1' -- {} \;

Which:

Allows for multiple file extension to be processed at the same time.
Uses -execdir instead of -exec so the default output of the hashing algorithm don't contain paths. (Which is one reasons I was trying to strip them originally).
Instead of md5 uses sh1sum which provides a sha1sum -c flag for verifying files and didn't require installation via homebrew.
Uses the more appropriate ${1%.*} (with the help of the -- at the end) instead of ${0%.*} to remove the initial file extension.

A gotcha, sort of...

Although the one-liner works, the use of $0 is inappropriate. From man bash:

   -c        If the -c option is present, then commands are read from  the
             first non-option argument command_string.  If there are argu-
             ments  after  the  command_string,  the  first  argument   is
             assigned  to  $0  and any remaining arguments are assigned to
             the positional parameters.  The assignment  to  $0  sets  the
             name  of  the  shell, which is used in warning and error mes-
             sages.

That is, the file names to compute MD5 for are not appropriate values as the "shell". Positional arguments are in $1, $2, and so on, that would be appropriate for this purpose. You can fix that using the -- special argument, that signals the end of options and disables further option processing:

find ... -exec bash -c 'printf $(md5 -q "$1") > "${1%.*}.md5"' -- {} \;

Simplify

Is it really important to strip the .jpg at the end of filenames?
Would it be terrible if the files looked like this?

./2016/05/17/photo-312.jpg
./2016/05/17/photo-312.jpg.md5
./2016/05/19/photo-1234.jpg
./2016/05/19/photo-1234.jpg.md5
./2016/05/19/photo-5678.jpg
./2016/05/19/photo-5678.jpg.md5

Because that would simplify the script a bit.

Is it really important to print only the MD5 digest without trailing newline?
That would simplify the script a bit more.
And that would get rid of the -q flag which is only supported by BSD's  md5 tool, and not by GNU's md5sum.
With this change, the script would be usable in Linux by defining md5 as an alias to md5sum.

Actually, it would be great to install GNU's md5sum (md5sha1sum package in Brew and MacPorts), because it has a -c flag to verify the file easily. For example, you would be able to verify the checksum of all files with:

find . -name '*.md5' -execdir md5sum -c {} \;

To create the files:

find . -type f -name "*.jpg" -execdir sh -c 'md5sum "{}" > "{}".md5' \;

Note that -execdir is necessary instead of -exec, so that the filenames don't have the directory part. Otherwise the .md5 files would contain the full path of files, and the md5sum -c verification would only work if invoked from the same relative path from where the digest file was created.

Use sh if good enough

The Bash script in -exec doesn't do anything Bash specific, so you could replace bash with sh.

Redundant "%s" in printf

Instead of printf "%s" something you could simply write printf something.


Loading and Refreshing DataGrid in WinForms using C# .Net Framework I am looking for some review on the code I am about to provide below.

I'd like to improve my coding in general and happy to hear any comments from the community.

The goal of this code is simply load dataGridView1 with some data from MySQL and Refresh it with a click of a button.

I have not yet tried using automatic refresh. But will look into it soon or later.

Code:

using System;
using System.Collections.Generic;
using System.ComponentModel;
using System.Data;
using System.Drawing;
using System.Linq;
using System.Text;
using System.Threading.Tasks;
using System.Windows.Forms;
using MySql.Data.MySqlClient;
using System.Configuration;

namespace Final_Version
{
    public partial class Form19 : Form
    {
        public Form19()
        {
            InitializeComponent();
        }

        private void Form19_Load(object sender, EventArgs e)
        {
            RefreshGridHistory();
        }

        string connString = ConfigurationManager.ConnectionStrings["Final_Version.Properties.Settings.technicalConnectionString"].ConnectionString;

        public void RefreshGridHistory()
        {
            // MySQL connection string
            using (var conn = new MySqlConnection(connString))
            {
                //Seelct query
                using (var mySqlDataAdapter = new MySqlDataAdapter("select r.date_requested, u.username, r.product_code, r.customer_code " +
                                                                   " from verify v join request r on v.request_id = r.id join users u on r.user_id = u.id join" +
                                                                    " users u2 on v.user_id = u2.id where v.status = 'Verified' or v.status = 'Rejected';", conn))
                {
                    using (var dataSet = new DataSet())
                    {
                        DataSet DS = new DataSet();
                        mySqlDataAdapter.Fill(DS);

                        //Assign headers to Data Grid View
                        dataGridView1.DataSource = DS.Tables[0];
                        dataGridView1.Columns[0].HeaderText = "Date Requested";
                        dataGridView1.Columns[1].HeaderText = "User Requested";
                        dataGridView1.Columns[2].HeaderText = "Product Code";
                        dataGridView1.Columns[3].HeaderText = "Customer Code";

                    }
                }
            }
        }

        private void button1_Click(object sender, EventArgs e)
        {
            RefreshGridHistory();
        }
    }
}

Single responsibility

Move the query logic to different class, let's call it DAL for example. In this implementation you are returning DataTable.  Returning a list of some class instead will make sure changes in the DB query/ schema will not affect the GUI but only the DAL class.

LINQ

You can write the sql query using linq and then if you have syntax errors they will be cache during completion

Grid

I wonder why  you set the columns headers multiple times?

Think about this scenario: when the form loads, If the query return 0 results or there is a connection problem, how the grid will look like?


Finding the top k elements from a file I was writing a program in C++11 to find top k words from a file. I think priority queues will give the optimal solution so I implemented using that. The code works and compiles. Could anyone please review this and suggest some more C++11/14-ish things to do?

I also have some specific questions:

Is there way in C++11 or C++14 to avoid using an iterator to find whether an element exists or not in a map?

I am talking about the condition:

if(it != word_map.end()) {
  word_map[word] = ++word_map[word];
}

How can I avoid using an iterator? Is there a "defaultdict" in C++ like we have in Python? I suppose here I can just remove it since default initialization will be 0, but what is the general way?
I saw a weird behavior for this:

while(input_file>>word) {
  //....
}

This loop gives the correct count. But if I do:

while(input_file.good()) {
   //..
}

this gives the count with an additional 1 added if the word is present on two different lines.

For example:

this is a test.
this.

while giving a count of this as 3.
I think I need to write a custom comparator for the priority queue to compare only the second part of the "pair". In that case, on what basis is the priority queue operating as of now?

/** top k words in a file
  */
#include <iostream>
#include <fstream>
#include <map>
#include <queue>
using namespace std;

void getTopKWords(const map<string, int>& word_map, const unsigned int k) {
  priority_queue<pair<string, int> > pq;
  for(auto item : word_map) {
    pq.push(make_pair(item.first, item.second));
  }

  for(unsigned int i=0;i<k;i++) {
    auto item = pq.top();
    pq.pop();
    cout << item.first << " " <<item.second << endl;
  }
}

void getWordFrequency(const string& filename, const unsigned int& k) {
  ifstream input_file;
  input_file.open(filename);
  if(!input_file.is_open()) {
    std::cout<< "Error opening file " << std::endl;
    return;
  }

  map<std::string, int> word_map;
  std::string word = "";
  while(input_file >> word) {
    auto it = word_map.find(word);
    if(it != word_map.end()) {
      word_map[word] = ++word_map[word];
    }
    else {
      word_map[word] = 1;
    }
  }
  input_file.close();
  getTopKWords(word_map, k);
}

int main() {
  getWordFrequency("test_file.txt", 5);
}

Answer to questions:

  I think priority queues will give the optimal solution so i implemented using that.

I would bet a heap is better for anything bigger than test data. Note that in a priority queue you are keeping all the data. So every time you add an item you are adding to its size and the length of time it takes to insert.

In real terms a heap would be better because you only keep the top 10 (or hover many you actually require). So each new value is only being compared against the top 10 items.

See:

http://en.cppreference.com/w/cpp/algorithm/make_heap
http://en.cppreference.com/w/cpp/algorithm/push_heap
http://en.cppreference.com/w/cpp/algorithm/pop_heap

  The code works and compiles. Could anyone please review this and suggest some more C++11/14-ish things to do?.

Covered in the next section:

  Is there way in c++11 or c++14 to avoid using an iterator to find whether an element exists or not in a map ?

if(it != word_map.end()) {
  word_map[word] = ++word_map[word];
}
else {
  word_map[word] = 1;
}

Yes. This can be replaced by

++word_map[word];

If the item does not exist in the map then using operator[] will insert the key and set the default value to zero. We then apply the ++ operator and increment it. So when a value is first inserted it gets the value 1, subsequent increments increase the stored value appropriately.

  I saw a weird behavior for this : while(input_file>>word) { //.... }

  this loop gives the correct count. But if i do while(input_file.good()) { //.. } this gives the count with an additional 1 added if the word is present on two different lines. For example :

This is correct. In all languages you test the result of the read operation (the behavior when you use while(input_file>>word)).

In all languages this is wrong while(input_file.good()) {.

The reason is that the EOF flag is not set until you read past the actual eof. BUT the last successful read reads up-to but not past the EOF. So when you read the last word, there is no more data on the input but the EOF flag is not set because you have not read past the end of file.

So if you use while(input_file.good()) you will re-enter the loop after you have read all the data. It is not until you try and read data from the stream that the EOF will be set.

You can do it this way but you must test the state of the read as well:

while(input_file.good())
{
    if (input_file >> word)
    {
        // read worked so you can processes the data
    }
}

It seems much easier to write:

    while(input_file >> word)
    {
        // Only enter the loop if the read worked.
    }

This is because operator>> reads a word from the stream and returns as a result a reference to the stream. So you are now using the stream as the condition of the while(). When used in this context the stream is converted to a boolean by calling good(). So in this case you have done the read and then tested the state of the stream to see if the read worked.

  while give count of this as 3.

Yep you entered the loop again when there was no data to read and counted the extra entry into the loop.

I think i need to write a custom comparator for the priority queue to compare only the second part of the "pair".

Yep

Code Review

Don't do this:

using namespace std;

See every other C++ reviewed question for why.
And Why is “using namespace std;” considered bad practice?.

Don't use unsigned int.

void getTopKWords(const map<string, int>& word_map, const unsigned int k) {

I know it seems logical that the your input should be a positive value. But unfortunately if somebody calls the function and accidentally passes a negative number it will still work; It just passes a very large positive value instead (stupid auto-conversions).

The general consensus is that you should always used signed integers. The only time that using unsigned is useful is when your integer is holding a set of bit flags.

To make this work as required you would need to define a custom ordering.

  priority_queue<pair<string, int> > pq;

Why not create and open the file in a single line:

  ifstream input_file;
  input_file.open(filename);

  // Easier to read as:
  ifstream input_file(filename);

String defaults to the empty string.

  std::string word = "";
  // Easier to write as:
  std::string word;

This we mentioned above:

    auto it = word_map.find(word);
    if(it != word_map.end()) {
      word_map[word] = ++word_map[word];
    }
    else {
      word_map[word] = 1;
    }
    // Can all be replaced with:
    ++word_map[word];

Don't manually close a file.

  input_file.close();

The exception is if you want to check that the close worked and potentially report a problem to the user. Since you are not doing this check then let the destructor of the file call close() for you. This way it caches all errors cleanly and discards them.

Code to use Heap to get top - N Items

struct CountTester
{
    bool operator()(std::pair<std::string, int> const& lhs, std::pair<std::string, int> const& rhs) const
    {
        return lhs.second > rhs.second;
    }
};

void topNFromMap(std::map<std::string, int>  const& wordData, int count)
{
    CountTester                                 countTester;
    std::vector<std::pair<std::string, int>>    heap;
    heap.reserve(count+1);                              // +1 is for the item we will add next.

    auto loop=std::begin(wordData);
    // Push the first `count` items into the vector.
    for(; loop != std::end(wordData) && heap.size() < count; ++loop)
    {
        heap.push_back(*loop);
    }

    // Make a heap so the smallest value is on top.
    std::make_heap(std::begin(heap), std::end(heap), countTester);

    // Add any more items.
    // Only need to keep the ten top 'count' items.
    // Note: the complexity is 2 x O(log(count)) => O(1) (since count is fixed)
    //       So this pushes the new item into the heap.
    //       Pops the smallest value of the top of the heap
    while (loop != std::end(wordData))
    {
        heap.push_back(*loop);
        std::push_heap(std::begin(heap), std::end(heap), countTester);
        std::pop_heap(std::begin(heap), std::end(heap), countTester);
        heap.pop_back();        // remove smallest item.
        ++loop;
    }

    // Pop Values from the heap.
    // This will put them in the correct order.
    for(auto popPos = std::end(heap); popPos != std::begin(heap); --popPos)
    {
        std::pop_heap(std::begin(heap), popPos, countTester);
    }

    // Now we can print all the items.
    for(auto const& item : heap)
    {
        std::cout << item.first << " " << item.second << "\n";
    }
}


Review of a generic request handler class (RestSharp) I wrote a class that uses RestSharp to access Rest and HTTP API web services. I tested it and it works however I was wondering if any changes could be made to it to make it better.

public class RequestHandler
{
    //Client to translate Rest Request intot HTTP request and process the result.
    private RestClient client;

    //this property stores the base URL
    const string baseUrl = "";

    //stores the account id and secret key if they will be needed for any authentication processes.
    readonly string account_id;
    readonly string secretKey;

    //this property stores the API key
    readonly string accessKey = "";

    //This constructor contains the account id and secret key that will be used for authorization when connecting to the API
    public RequestHandler(string account_id,string secretKey)
    {
        client = new RestClient(baseUrl);
        this.account_id = account_id;
        this.secretKey = secretKey;
    }

    //The default constructor
    public RequestHandler()
    {
        client = new RestClient(baseUrl);
    }

    public void Execute<T>(int id,Action<T> Success, Action<string> Failure) where T:new()
    {
        RestRequest request = new RestRequest();
        request.RequestFormat = DataFormat.Json;
        request.AddParameter("id",id,ParameterType.GetOrPost);

        client.ExecuteAsync<T>(request,(response) =>
        {
            if(response.ResponseStatus == ResponseStatus.Error)
            {
                Failure(response.ErrorMessage);
            }
            else
            {
                Success(response.Data);
            }
        });
    }
}

Depend on the IRestClient interface rather than the RestClient implementation and make the field readonly as it is only set via the constructor:

private readonly IRestClient client;

Invert your dependancy on RestClient, if you are not using an IOC container, at least implement poor mans injection so that the class is testable (e.g. you can supply a mock IRestClient to test RestHandler.

public RequestHandler() // If you are using an IOC container, remove the default constructor.
    : this(new RestClient(baseUrl))
{
}

public RequestHandler(IRestClient restClient)
{
    this.client = restClient;
}

As Ron mentioned above, you don't appear to be using secret key and account id so either remove them if they are not needed or use them.

Making the base url a constant inside the class makes it difficult to re-use, I would move that into the application configuration.

If you do need account_id, remove the underscore and camel case it accountId to adhere to  the standard naming conventions.

Adding a property for the DataFormat will make the class more re-usable if you want to call a web service which doesn't support Json:

public DataFormat DataFormat { get; set; }

// Default to Json in the constructor
this.client = restClient
this.DataForma = DataFormat.Json;

ExecuteAsync returns a RestRequestAsyncHandle which allows the request to be aborted, it might be worth looking at whether you need the ability to abort requests or not. You could update your Execute to return the wait handle.

The error checking here doesn't account for any of the other reasons for failure such as a timeout or abort.

if (response.ResponseStatus == ResponseStatus.Error)

Update it to something like this:

switch (response.ResponseStatus)
{
    ResponseStatus.Completed:
        Success(response.Data);
        break;

    ResponseStatus.Error:
    ResponseStatus.TimedOut:
        Failure(response.ErrorMessage);
        break;
}


Add buffer to hash table RetrieveData is a 3rd party API that takes 3 parameters: a buffer, number of bytes to retrieve, and actual bytes required for the buffer.

The buffer returned is a non-null terminated unicode string
99% of the time, the buffer is less than 256 bytes, but may be larger and needs to handle this scenario
RetrieveData(data) is guaranteed to return
The reason for using std::map is the ID must later be retrieved based on a string

std:map<std::wstring, unsigned int> map_columns;
unsigned int id;
unsigned long pcbActual;
char* data;

do
{
    RetrieveID(&id);
    // find out how large buffer we need to retrieve data
    RetrieveData(NULL, 0, &pcbActual);
    data = new char[pcbActual + 2];
    // actually retrieve data
    RetrieveData(data, pcbActual, &pcbActual);
    data[pcbActual] = '\0';
    data[pcbActual + 1] = '\0';
    std::wstring str((wchar_t*)data);
    map_columns[str] = id;
} while (MoveNext());

Jerry Coffin has already pointed out that your current code leaks memory all over the place, which could certainly be slowing it down (since the heap allocator will have to go get more and more and more memory to feed the leaky code, instead of just reusing the same memory every time).

Consider using std::unordered_map<std::wstring, unsigned int>, as a hash lookup is likely to be O(1) and faster than the O(lg N) lookup associated with a std::map (which is basically the same thing as Java's TreeMap).

Move semantics are your friend. Use them to eliminate all the expensive reallocations and memcpys that are happening in your code. I count three memcpys: once inside RetrieveData, then again from data into str, then again from str into map_columns.

Minor nit: Declare your variables in the smallest, tightest scopes possible; this will aid the optimizer as well as helping the human reader understand the code.

Putting it all together, we get something like this:

std::unordered_map<std::wstring, unsigned int> map_columns;

do {
    unsigned int id;
    RetrieveID(&id);
    unsigned long len;
    RetrieveData(nullptr, 0, &len);
    assert(len % 2 == 0);  // len is measured in 2-byte units
    std::wstring data((len/2)+1);
    RetrieveData(data.data(), len, &len);
    map_columns.emplace(std::move(data), id);
} while (MoveNext());

(You might need to cast data.data() to (void*) or (char*); I don't know. You might also want to see whether the third argument to RetrieveData can be nullptr; or, if not, maybe you should use a temporary variable and assert that the new value is exactly the same as the old value, since it would be Very Bad if that weren't true.)

Oh, and I strongly suspect that you're eventually going to run into problems where you're looking up a Unicode string in your map and you don't find it when you thought you would, because of Unicode normalization issues. So, be prepared for that, if you can.


Async ASP.NET MVC 5 controller method I'm attempting to correctly convert a synchronous controller method to asynchronous, given that the operation it performs is CPU-intensive.

private MyDbContext db = new MyDbContext();
public async Task<ActionResult> Index()
{
    List<DashboardItemViewModel> viewModels = new List<DashboardItemViewModel>();
    List<ProductModel> products = db.Products.ToList();
    await Task.Run(() =>
    {
        foreach (ClientModel client in db.Clients)
        {
            // Constructor uses lots of logic, mostly looping through the list
            //   many times
            viewModels.Add(new DashboardItemViewModel(client, products));
        }
    });
    return View(viewModels);
}

As a key area of concern with this code, you really should be passing a cancellation token here, as it is a long running operation.

You need to worry about the client giving up on the request, let's say they navigated to the URL and decided they didn't mean to. The operation will still continue to run until the thread operation is complete.

This kind of issue can also lead to all kinds of problems.

Note that this can also occur when there multiple requests (say an impatient user hits refresh frequently) or a lot of traffic.


Iterator class using a square list I am trying to implement squarelist by using iteration that points to the head element.  I've create this iterator class to make my iterator id directional.  I just want to see if I implemented it correctly.

class iterator : public std::iterator<std::bidirectional_iterator_tag , Object , int>
    {

    public:
        iterator(Node* p=nullptr) : _node(p){}
        ~iterator(){}

        Node* getNode(){return _node;}

        Object operator * () { return _node->data; }
        iterator & operator ++()
        {
            _node = _node->next;
            return *this;
        }

        iterator operator ++(int)
        {
            iterator  retVal = *this;
            ++this;
            return retVal;
        }
        bool operator < (iterator const& rhs) const
        {
            return _node < rhs._node;
        }
        bool operator != (iterator const& rhs) const
        {
            return _node != rhs._node;
        }
        bool operator == (iterator const& rhs) const
        {
            return _node == rhs._node;
        }
    };

A couple of real issues (rather than the imaginary ones previously pointed out):
Your code does not fully implement the requirements of iterator.

You have not implemented the operator-> as required by the iterator concept.
operator* should return by reference.
This is because *x = t is a requirement of iterator and without the reference this will not work as expected (or at all depending).

Without the container code its hard to tell if you can use the same iterator for both const and non-const iterator types. So sometimes its useful to provide const overloads of the above (but that will depend on usage).
see: http://www.sgi.com/tech/stl/trivial.html
You classify your iterator as
std::bidirectional_iterator_tag

This is an indication that you can go backwards and forwards with the iterator. In fact the concept means that your iterator must also support operator-- are well as operator++.
See: http://www.sgi.com/tech/stl/BidirectionalIterator.html
The link you provide indicates that a square list is circular. So moving forward all the time you may never reach the end. But without understanding (or seeing your implementation) it is hard to tell if the current implementation will work.
    iterator & operator ++()
    {
        _node = _node->next;   // Will this ever reach a NULL
                               // Or do you have a special version of
                               // iterator that marks the end.
                               //
                               // Note: end is one past the last element
                               //       Which makes this doubly hard to
                               //       implement with a circular list.
                               //       But can be done if you use a sentinel.
                               //
                               // The default constructor suggests that the
                               // end is marked by a NULL but hard to be sure.
        return *this;
    }

No point in defining destructor the default version will work perfectly. (As a side note: since it is not virtual there can be no derived types with alternative behaviors so making things symmetric is doubly useless).
    ~iterator(){}

This is perfectly fine.
        iterator  retVal = *this;

But just as a personal preference I prefer:
        iterator  retVal(*this);

As mentioned above (but without the container code it is hard to tell). Well this test work for the end iterator.
    bool operator != (iterator const& rhs) const
    {
        return _node != rhs._node;
    }
    bool operator == (iterator const& rhs) const
    {
        return _node == rhs._node;
    }

There is no requirement for bidirectional iterators to have a less than comparison. Unless you really want to store iterators in a sorted container I would leave this out.
Note: A random access iterator does have a requirement for a less than operator. But it has a precondition that it can only compare iterators that are reachable from each other.
see: http://www.sgi.com/tech/stl/RandomAccessIterator.html
    bool operator < (iterator const& rhs) const
    {
        return _node < rhs._node;
    }

The second thing to consider is the sort ordering correct?

You are defining the less than operator in term of pointer arithmetic. Unless the object are allocated by the same allocators from a known block this is actually undefined behavior. For pointer comparison to be valid they need to be in the same allocated block.
Is this a good ordering? Youre ordering is based on their address (not the position in the container). So things may be in a jumbled up state in relation to everything else. This may be perfectly fine but something you should keep in mind.

WHY The following answers is a complete waste of time and WRONG
So please stop voting for it.

I learnt about its importance following discussions on one of the C++ working groups. It's a good practice to follow smarter people.

Yes but you have to also understand when to apply these principles. Some ideas contradict other ideas if you just use them bluntly. You need to understand each idea but also when to apply it.
The concept of operator symmetry is an important concept when used in the correct situation (no argument there). The problem is that iterators are not the correct situation and it plays no part.
But we have two competing concepts at play here. One: is symmetry is good. Two: auto conversion of types by the compiler is bad.
You have to balance the two concepts. In terms of iterators auto converting iterators is a very bad idea. You want to make the user of the iterator be explicit about any conversions (as the STL designers did (see below)). As a result the symmetry concept can not be used (as it requires auto conversion).
Why it makes no sense for auto conversion
 Container    x;
 AltCont      y;

 // You can not compare iterators
 // From different containers.
 if (x.begin() < y.end()) {
     // At best this is undefined
     // At worst this is undefined behavior.
 }

 // There is no logic to that situation.
 // SO because you should not compare iterators from different
 // containers the type of the iterator when being tested will
 // ***ALWAYS*** be the same type.

 // The type of the iterator is always `Container::iterator`
 // Its a required property of a container.
 Container::iterator  begin = x.begin();
 Container::iterator  end   = x.end();

Q: A but you can create other types of iterator from your iterator.
A: Yes. But this new type is not comparable to the old type.
typedef std::reverse_iterator<Container::iterator> reverse_iterator;
reverse_iterator rbegin(end);
reverse_iterator rend(begin);

The types are still not comparable.
 if (begin < rbegin) {  // Compile time error
 }                      // For a good reason
                        // there is no logical reason for the ranges
                        // to be equivalent.

You will also note it does not work the other way around.
 if (rbegin < begin) {
 }

Why you ask is this second case a big deal. Well there is no constructor on Cont::iterator to create an object of type std::reverse_iterator<Cont::iterator>. So in the first test above there was no way to convert rbegin into the correct type.
BUT there is a constructor on std::reverse_iterator<Cont::iterator> that takes an object of Cont::iterator so in the second condition we could be in a situation where begin is auto converted into std::reverse_iterator<Cont::iterator> and the expression would compile. But luckily for us the designers of the STL decided that was a bad idea and made the constructor explicit to prevent auto conversion and thus prevent the code from compiling.
OK so why is in Dangerous
Its also hides errors.
The above code will generally generate a compiler error (so you spot it and correct it quickly). But lets dig in and build the code as suggested by @vinipsmaker.
For this to work there either has to be a conversion from one iterator to the other or as he suggests a base class and a derived type.
 class IterBase
 {  /* Define iterator */ }

 class IterDerivedForCont1: public IterBase
 {  /* Define iterator */ }

 class IterDerivedForCont2: public IterBase
 {  /* Define iterator */ }

 class Cont1 {
     typedef IterDerivedForCont1 iterator;
     /* Other Stuff */
 }

 class Cont2 {
     typedef IterDerivedForCont2 iterator;
     /* Other Stuff */
 }

 bool operator<(IterBase const& l, IterBase const& r)
 { return TEST(l,r);}

 Cont1   a;
 Cont2   b;

 if (a.begin() < b.end()) {
     // You want this to compile?
     // Not really. But it will
 }

 // So in this case you are hiding errors from the user.
 // That could have been easily spotted by the compiler.


Function that normalizes a vector of double I have written the following function in order to normalize a container of double, so that it sums up to 1.0:

/**
 * @brief This function normalizes a container so that it sums to 1.0.
 *
 * @param begin The beginning of the range to normalize.
 * @param end The end of the range to normalize.
 * @param out The beginning of the output range (can be the same as begin).
 */
template <typename InputIterator, typename OutputIterator>
void normalizeProbability(InputIterator begin, InputIterator end, OutputIterator out) {
    double norm = static_cast<double>(std::accumulate(begin, end, 0.0));
    std::transform(begin, end, out, [norm](decltype(*begin) t){ return t/norm; });
}

Is it possible to do better, or having a more precise result?

It looks pretty good as it is but I can offer the following:

template <typename InputIterator, typename OutputIterator>
void normalizeProbability(InputIterator begin, InputIterator end, OutputIterator out) {
    typedef typename std::iterator_traits<InputIterator>::reference ref_t;
    double norm = std::accumulate(begin, end, 0.0);
    std::transform(begin, end, out, [norm](const ref_t t){ return t/norm; });
}

I have removed the cast to double, which isn't necessary. The double return type is deduced by the 0.0 argument. I also changed to use std::iterator_traits instead of decltype which I think is a bit cleaner.

The biggest source of error you have is in the summation of norm. Adding a small floating point (FP) number (the dereferenced value) to a large floating point number (the current sum) will cause a truncation error. For example 1.0 + 1E-18 == 1.0 will return true due to the limited precision of FP-numbers. What you want to do is to add the numbers from the smallest to the biggest. But simply sorting isn't good enough, consider 10^20 entries (yeah I know fat chance but it's just for illustration) each with value 1E-18 you expect the norm to be 10^2, but in fact it will be close to 1.0 or less. Because after about 1E18 iterations the running sum will be so big that any remaining elements will truncate. So you need to have a heap, take the two smallest numbers, add them and put the result back into the heap. Then repeat until only one number is left, which will be the sum. This guarantees that you will always minimize the truncation error.

However I have no idea if this level of accuracy is necessary for you, but you stated precision as a question so I went for it. :)


Replacing names in text fields with aliases to help de-identify data I have a table with over 1,000,000 records. I need to replace any names in the text fields with aliases to help de-identify the data. For this example, let's assume the table is TemporaryTest and has two fields: Id (the key field) and IndexedXML (the text field).

I have a second table, AppellationSubstitution, that has the following columns: TextEntry (a name needing replacement), Length (length of TextEntry), Replacement (the replacement name, which may be of a different length). That table has about 110,000 rows.

The first step I use is (the regex matches words in the text field -- it looks a bit odd because of some odd characters that show up in this database):

SELECT id,
       matchindex,
       matchlength,
       replacement
FROM   TemporaryTest
       CROSS APPLY
master.dbo.Regexmatches('([Xx]-)?[\w-[0-9üÿ_]]{2,}(-[\w-[0-9üÿ_]]{2,})?(''[\w-[0-9üÿ_]])?', [IndexedXML],
master.dbo.Regexoptionenumeration(0, 0, 1, 1, 0, 0, 0, 0, 0))
       INNER JOIN dbo.appellationsubstitution
       ON match = textentry
       ORDER BY Id, MatchIndex DESC;--if replace in forward order, insertion point gets moved

This produces a table with over 100,000 rows, which the following shows a few lines:

Id matchindex matchlength replacement

99309 122 5 “Demarcus”
108639 106 5 “Demarcus”
109809 84 6 “Rehbein”
110373 89 7 “Reginald”
111156 105 5 “Demarcus”
112452 129 6 “Thie”
112896 113 6 “Diberardino”
112896 92 6 “Diberardino”
113503 119 3 “Rubin”

The full procedure I'm currently trying out is:

SET NOCOUNT ON;
SET XACT_ABORT ON;
BEGIN TRANSACTION;

DECLARE ReplaceCursor CURSOR LOCAL FOR
SELECT id,
       matchindex,
       matchlength,
       replacement
FROM   TemporaryTest
       CROSS APPLY
master.dbo.Regexmatches('([Xx]-)?[\w-[0-9üÿ_]]{2,}(-[\w-[0-9üÿ_]]{2,})?(''[\w-[0-9üÿ_]])?', [IndexedXML],
master.dbo.Regexoptionenumeration(0, 0, 1, 1, 0, 0, 0, 0, 0))
       INNER JOIN dbo.appellationsubstitution
       ON match = textentry
       ORDER BY Id, MatchIndex DESC;--if replace in forward order, insertion point gets moved
DECLARE @Rid int, @Rmi AS int, @Rml AS int, @Rrep AS nvarchar(255);
OPEN ReplaceCursor;
FETCH NEXT FROM ReplaceCursor INTO @Rid, @Rmi, @Rml, @Rrep;
WHILE @@FETCH_STATUS = 0
BEGIN
    UPDATE TemporaryTest
    Set IndexedXML =  STUFF([IndexedXML],@Rmi+1,@Rml,@Rrep)
        WHERE Id = @Rid;
    FETCH NEXT FROM ReplaceCursor INTO @Rid, @Rmi, @Rml, @Rrep;
END;
CLOSE ReplaceCursor;
DEALLOCATE ReplaceCursor;
COMMIT TRANSACTION

This works, but takes a very long time to run (over an hour and not yet completed), and IndexedXML is one of the smallest text fields I have in the production database.

I resorted to using a cursor as I didn't know any other way to manage sequential STUFF calls on the same cell, where subsequent STUFF calls use the result of the previous ones.

Am I taking the right course with this, or is there a faster/cleaner way of achieving this?

You seem to run a separate UPDATE command on each row of the cursor. That's inherently slow and will likely be a lot quicker when you run the UPDATE based on a SELECT statement that does the required projection for all matching rows at once.


SQL Sub-query with running total optimization The codes below get the rows of data until it reaches the total running quantity limit.

Notice that the SQL uses sub-query. select *, sum(Quantity) over (order by id) as RunningQty from #PurchaseOrderProducts. I'm worried that the sub-query would get all the data from the table. Is there a way to improve the performance?

CREATE TABLE #PurchaseOrderProducts
(
    Id int,
    Product varchar(10),
    Quantity int
)

INSERT INTO #PurchaseOrderProducts VALUES (1, 'Item A', 5)
INSERT INTO #PurchaseOrderProducts VALUES (2, 'Item B', 1)
INSERT INTO #PurchaseOrderProducts VALUES (3, 'Item C', 8)
INSERT INTO #PurchaseOrderProducts VALUES (4, 'Item D', 2)
INSERT INTO #PurchaseOrderProducts VALUES (5, 'Item E', 1)

SELECT P.*
FROM (select *, sum(Quantity) over (order by id) as RunningQty
      from #PurchaseOrderProducts
     ) P
WHERE P.RunningQty - Quantity < 12;

Assuming that #PurchaseOrderProducts is a realistic depiction of your real table but with less data (and that your real table is also a heap), then I get this plan.

This is a pretty reasonable plan, where I wouldn't necessarily expect it to be that bad.

There are a few things we could do to make this better, including:

Batch-mode for rowstore is enabled (via hacks) in SQL Server 2016 if you include a CCI in your query, even if it isn't used. This can likely make the window function better. This gets me a batch-mode operator for everything but the table scan, and cleans up a lot of the aggregation.
Creating an appropriate index that your window function can use will help as well by removing the sort (the index maintains a sorted order). A clustered index on the Id column moves all of the cost onto the index scan. A non-clustered index on Id that INCLUDEs Quantity and Product (order didn't matter in my testing) does the same. You'll have to INCLUDE any columns you want in the output, however - otherwise it will just table scan, or if you force it to use the index it'll key lookup. That won't be great for performance.

If we look at all of these side-by-side, we notice that the non-clustered without an INCLUDE is the worst by far, and the rest all look about the same.

You may notice that the batch-mode query is also using the non-clustered index; getting all of the plans side-by-side without indices is a pain. If you use the batch-mode hack without any indices then it will still have to sort the data, but it can do that in batch mode which will provide a nice performance bump. This is also good if you rely on the table being a heap elsewhere (e.g. parallel inserts, minimal logging, etc).

You may also notice that the plans using the clustered index vs non-clustered index have no apparent difference (besides the index they use). This is because all of the columns in the table are selected in both queries, and all indices include all columns. You can get substantial performance and memory savings if you only need a subset of columns and have a good non-clustered index; this might be helpful for your real data if the temp table isn't representative.

Query samples:

Batch mode w/ rowstore table

DROP TABLE IF EXISTS #FakeCciForRowstoreBatchMode;
CREATE TABLE #FakeCciForRowstoreBatchMode
(
  __zzDontUseMe bit NULL,
  INDEX CCI CLUSTERED COLUMNSTORE
);

DROP TABLE IF EXISTS #PurchaseOrderProducts;
CREATE TABLE #PurchaseOrderProducts
(
  Id       int,
  Product  varchar(10),
  Quantity int
);

INSERT INTO #PurchaseOrderProducts
VALUES
( 1, 'Item A', 5 ),
( 2, 'Item B', 1 ),
( 3, 'Item C', 8 ),
( 4, 'Item D', 2 ),
( 5, 'Item E', 1 );

SELECT P.Id,
       P.Product,
       P.RunningQty,
       P.Quantity
  FROM ( SELECT Id,
                Product,
                Quantity,
                SUM( Quantity ) OVER ( ORDER BY Id ) RunningQty
           FROM #PurchaseOrderProducts
             LEFT OUTER JOIN #FakeCciForRowstoreBatchMode
               ON 1 = 0 ) P
  WHERE P.RunningQty - P.Quantity < 12;

Indices I created for #2 (plans generated using INDEX hints)

CREATE CLUSTERED INDEX whatever ON #PurchaseOrderProducts( Id ASC )
CREATE NONCLUSTERED INDEX whatever2 ON #PurchaseOrderProducts( Id ASC ) INCLUDE( Quantity, Product )
CREATE NONCLUSTERED INDEX whatever3 ON #PurchaseOrderProducts( Id ASC ) INCLUDE( Product, Quantity )
CREATE NONCLUSTERED INDEX whatever4 ON #PurchaseOrderProducts( Id ASC )


DRY cursors: preventing T-SQL's FETCH statement from being repeated Please assume I've exhaustively tried to come up with a set-based solution to my T-SQL problem, and that I need to use a cursor. The following is the typical1 boilerplate for a T-SQL cursor:

DECLARE @myId AS INT;

DECLARE myCursor CURSOR FAST_FORWARD FOR
SELECT id FROM MyTable;

OPEN myCursor;

FETCH NEXT FROM myCursor INTO @myId; -- COPY...

WHILE @@FETCH_STATUS = 0
BEGIN
    PRINT 'Doing the cursor work!';

    FETCH NEXT FROM myCursor INTO @myId; -- ...PASTE! :'(
END;

CLOSE myCursor;
DEALLOCATE myCursor;

On Stack Overflow it's already been asked how to avoid the duplication. Please suppose this is quite important (e.g. it's somehow unavoidable to FETCH many columns into variables). While trying to come up with a solution to DRY the two FETCH statements into one, I ended up with boilerplate similar to the OP's answer to the mentioned question:

DECLARE @myId AS INT;

DECLARE myCursor CURSOR FAST_FORWARD FOR
SELECT id FROM MyTable;

OPEN myCursor;

WHILE 1=1 -- Not very pretty, but required to keep the FETCH statement DRY.
BEGIN
    FETCH NEXT FROM myCursor INTO @myId;
    IF @@FETCH_STATUS <> 0 BREAK;

    PRINT 'Doing the cursor work!';
END;

CLOSE myCursor;
DEALLOCATE myCursor;

Now, my code review question comes down to: is there any elegant way to DRY out duplicate FETCH statements from the "default" boilerplate (1st snippet)? If my second snippet is the easiest way, then I'd love to know if there is any way to get rid of the nasty WHILE 1=1 statement? Or is it a necessary sacrifice?

*Bonus (subjective) question:

Although the WHILE 1=1 bit from the second boilerplate isn't very pretty, I think I prefer it a lot over duplicating the FETCH statement. Any insights as to why the first boilerplate is much more prominent / widespread? Am I missing a major advantage of the first snippet?

1 Taken / adapted from the 70-461 exam training kit.

The infinite loop idiom in languages that have boolean literals is while ( true ) but it seems the boolean type and its literals (e.g., TRUE/FALSE) have gone the way of the DO ... WHILE as far as T-SQL is concerned. This means you're stuck with true-evaluating expressions of which 1=1 is the simplest.

In your case I'd replace the start of the loop with WHILE (1=1) -- Loop until BREAK..


Codility's count passing cars in opposite directions in C# I programmed a few things in java and now I'm learning C#. I've been doing exercises from Codility.

The first 3 or 4 were very difficult to me but now I guess I've got the hang of it. This site helped a lot at improving my style and the things I now think I should be looking at before contenting myself with the final code. I comment a lot less than previous posts because I guess the names and the constants are self explanatory.

Are there still things to improve in my code in easy exercises like this?

Task score: 100%

Detected time complexity: $O(N)\$

  Task description

  A non-empty array A consisting of N integers is given. The consecutive
  elements of array A represent consecutive cars on a road.

  Array A contains only 0s and/or 1s:

  0 represents a car traveling east, 1 represents a car traveling west.
  The goal is to count passing cars. We say that a pair of cars (P, Q),
  where 0 ≤ P < Q < N, is passing when P is traveling to the east and Q
  is traveling to the west.

  For example, consider array A such that:

  A[0] = 0
  A[1] = 1
  A[2] = 0
  A[3] = 1
  A[4] = 1
  We have five
  pairs of passing cars: (0, 1), (0, 3), (0, 4), (2, 3), (2, 4).

  Write a function:

  class Solution { public int solution(int[] A); }

  that, given a non-empty array A of N integers, returns the number of
  pairs of passing cars.

  The function should return −1 if the number of pairs of passing cars
  exceeds 1,000,000,000.

  For example, given:

  A[0] = 0
  A[1] = 1
  A[2] = 0
  A[3] = 1
  A[4] = 1
  the function should return 5, as explained above.

  Write an efficient algorithm for the following assumptions:

  N is an integer within the range [1..100,000]; each element of array A
  is an integer that can have one of the following values: 0, 1.

And this is my solution, please note that if 1000000000+ passing cars are counted, then the return value of the function should be -1. I had to check for overflows, because one of the tests returned a huge negative number if overflowing, also if overflow occurs before 1000000000 (I don't know if an int holds that number), then it would stop counting sooner, hence than try/catch block improves performance(?).

public static int GetNumberOfPassingCars( int[] passingCars )
{
    const int OVERFLOW = -1 ;
    const int EAST = 0 ;
    const int WEST = 1 ;
    int carTravelingEast = 0, pairOfPassingCars = 0;
    foreach( var passingCar in passingCars )
    {
        if ( passingCar == EAST )
        {
            ++carTravelingEast;
        }
        else if ( passingCar == WEST )
        {
            try
            {
                pairOfPassingCars = checked( pairOfPassingCars + carTravelingEast );
                if ( pairOfPassingCars > 1000000000 )
                {
                    return OVERFLOW;
                }
            }
            catch( OverflowException )
            {
                return OVERFLOW;
            }
        }
    }
    return pairOfPassingCars;
}

Overflow averted

Performance of your algorithm is optimal since you iterate the input just once in \$O(n)\$ time complexity. There is a way to avoid the checked overflow guard. Noone forces you to use int to perform the arithmetic in the method body.

int carTravelingEast = 0, pairOfPassingCars = 0;

Using uint with a max value of \$4,294,967,295\$ would never overflow, since:

carTravelingEast can never be more than int's max value \$2,147,483,647\$ (because of the length of an array is capped)
the maximum value \$3,147,483,647\$  as the sum of the custom threshold of one billion + int's max value could never exceed uint's max value (and we exit early on reaching one billion)

Code Conventions

Don't use UPPERCASE variable names for constants; use PascalCase instead.
Use white space according to Conventions.

GetNumberOfPassingCars( int[] passingCars ) -> GetNumberOfPassingCars(int[] passingCars)
foreach( var passingCar in passingCars ) -> foreach (var passingCar in passingCars)

Use plural name for a variable that represents multiple objects: pairOfPassingCars -> pairsOfPassingCars.
Use a constant to avoid magic numbers; const uint maxLimit = 1000000000;.
Throw ArgumentNullException when mandatory arguments are null in public methods.

Code Refactored

public static int GetNumberOfPassingCars(int[] passingCars)
{
    if (passingCars == null) throw new ArgumentNullException(nameof(passingCars));

    const int Overflow = -1;
    const int East = 0;
    const int West = 1;
    const uint maxLimit = 1000000000;
    uint carsTravelingEast = 0, pairsOfPassingCars = 0;

    foreach (var passingCar in passingCars)
    {
        if (passingCar == East)
        {
            ++carsTravelingEast;
        }
        else if (passingCar == West)
        {
            pairsOfPassingCars += carsTravelingEast;
            if (pairsOfPassingCars > maxLimit)
            {
                return Overflow;
            }
        }
    }

    return (int)pairsOfPassingCars;
}


Python array literal syntax sugar similar to Perl and Ruby Background

Python (version 2.7.2)

Problem

You want to specify an array in your code.
You want to use the 'syntax sugar' available in Ruby and Perl.
You are a fanatic about readable code but also saving keystrokes.

Solution

Python Triple-quoted string
Python splitlines
Python list comprehension

Pitfalls

The solution may look ugly or un-Python-ish

Question

Are there any refactorings, suggestions or comments on this approach?

### ********************
## setup an array using append
ary_names = []
ary_names.append('alpha')
ary_names.append('bravo')
ary_names.append('charlie')
ary_names.append('delta')
ary_names.append('echo')

### ********************
## setup an array using triple-quoted string
ary_names = [
  item.strip() for item in
  """

  alpha
  bravo
  charlie
  delta
  echo

  """
  .splitlines()
  if(not item.strip()=='')
]

"""
Here is a ruby example for comparison

ary_names = %w[
  alpha
  bravo
  charlie
  delta
  echo
]
"""

If you call split with no arguments, it will split on any whitespace and drop any empty elements. Knowing that, it's as simple as this:

ary_names = """
    alpha
    bravo
    charlie
    delta
    echo
""".split()

If you have only five short elements, you may want to put them all on one line:

ary_names = 'alpha bravo charlie delta echo'.split()

Of course, you'd only want to do that if it fits easily on one line.


Better way to have output code? When a user is created, deleted and update the system will give feed back. e.g. User Deleted/Updated/Create success or unsuccessfully, but I'm sure there is a better way to do it than what I have done because I repeat the same code in all functions within my class.

Code to create user

$userOne = new userActions($database, 'Greg', '324b2643243');//
echo $userOne->create();

Code to delete user

$userOne = new userActions($database, 'Abel'); //
echo $userOne->delete();

Code for user creation

public function create()
{//Works
    $outPut = "";
    if(!$this->user_exists)
        {
            list($password_hash, $salt) = $this->hash($this->password);
            echo 'Hash: ' . $password_hash . '
' . 'Salt: ' . $salt . '
';
            $result = $this->connection->query("INSERT INTO users VALUES( NULL, '{$this->username}' , '{$password_hash}' , '{$salt}' ) ");

            if($result)
                {
                    $outPut = "User has been created";
                }
                else
                {
                    $outPut = "User could not be created";
                }

        }
        else
        {
            $outPut = 'Username already exists';
        }

    return $outPut;
}

Is there a better way to display feedback to the user istead of having $outPut every time in all of my create, delete and update functions?

I recommend splitting your model (the database operation) from your view (the localization of your message).

Depending if you prefer a return code or a exception flow your code could look like:

public function create()
{
    if($this->user_exists) return ""; //do you really want to show nothing?
    $error=UserModel::create($this->password,$salt,$this->username); // don't have to be static! It's up to you.
    if ($error==-1) return "User could not be created";  //could be a constant UserModel::ERROR_GENERAL
    if ($error==0) return "Username already exists"; //UserModel::ERROR_EXISTS
    return "User has been created";
}

Translating this to exception is up to the reader :)

Actually I even would prefer to give the $error code to my template and let it handle the localization:

public function create()
{
    if($this->user_exists) return "";
    $error=UserModel::create($this->password,$salt,$this->username);
    $view=new View("templatefile.php");
    $view->assign('error',$error);
    return $view->render();
}

//"templatefile.php"
<?if ($error==1):?>...
<?elseif ($error==0):?>...
<?else:?>...
<?endif?>


Sorting algorithm, possibly a new one I'm a beginner who has never seen a sorting algorithm's implementation and have decided to try make one on my own (can someone tell me if this has been already used?).

The fact is I have never used iterators, but since I wanted to make this more generic I decided to quickly look them up.

Also, can anyone suggest what to do if someone passes the same iterator twice?

#include <iterator>
#include <algorithm>

template <typename container>
inline void
sort
(typename container::iterator beginning,
 typename container::iterator end       )
{
    typedef typename container::value_type type_contained;
    typename container::iterator beginning_original_state = beginning;
    while (beginning <= end)
    {
        if (*beginning > *(beginning + 1))
        {
            // Gotta change the name to something more meaningful
            auto check_back(beginning);

            while (check_back != beginning_original_state && *check_back < *(check_back - 1))
            {
                std::swap(check_back, --check_back);
            }
        }
        else
        {
            ++beginning;
        }
    }
}

Algorithm
What you have implemented is called (linear) insertion sort. It is very old, sorry your method is not new or novel ;) It has average case O(n^2) time complexity which is basically unusable for anything with more than a few thousand/tens of thousands elements. I recommend that you study the classic algorithms: Bubble Sort, (Cocktail) Shaker Sort, Insertion Sort, Shell Sort, Merge Sort, Heap Sort, Radix Sort and Quick Sort.
There is a classic video from 1981 that is part of many CS curriculums called Sorting out Sorting which is very illustrative and a recommended watch.
Code
You never use type_contained remove it.
Once the unused type is removed you can change your template parameter to the iterator type directly and get rid of a lot of typing. This also allows template parameter deduction so that you don't have to explicitly state the container type as a template parameter.
Please don't reuse the argument beginning as a variable. It is hard to read when your iteration variable is named begin with another being original begin. Simply make a copy of begin, call it it for iterator or current.
Typically when two iterators are used to represent a range we use the names: first and last. To not confuse with begin() and end() and avoid a name collision when some one uses using namespace std;  (which you shouldn't!).
In general I'm not fond of your naming. For example check_back could simply be previous.
You don't need the inline specifier. You need a better algorithm. The compiler will automatically inline if it makes sense.
Bug
In the code here:
while (beginning <= end)
{
    if (*beginning > *(beginning + 1))

you will dereference the end iterator + 1 which is beyond the end of the range. Also note that typically one expects the first iterator to be inclusive and the second to be exclusive (as is returned from begin() and end() on containers). So when used as expected your code will go 2 elements outside of the range. This results in undefined behaviour.
Also here:
std::swap(check_back, --check_back);

you are swapping the iterators, not the values they point to. Which means that the code doesn't even work at all. It gets stuck in an infinite loop, always. Not only that, but the order in which the arguments are evaluated is undefined so this type of code might not work if the check_back variable is decremented first.
You would have noticed all of the above if you tried to use this code in any way before posting to review. For future reference, we only review working code and this question is actually off-topic due to it not working at all.
Cleaning it all up and fixing the bugs we get:
#include <iterator>
#include <algorithm>

template <typename Iterator>
void sort(Iterator first, Iterator last){
    auto it = first+1;
    while (it < last){
        auto prev = it;
        while (prev != first && *(prev-1) > *prev){
            std::iter_swap(prev-1, prev);
            prev--;
        }
        it++;
    }
}


Password generator in Python 3.7 I am learning how to code in Python by myself and have never coded before, I just want to know if there is any way I can upgrade my PasswordGenerator.

#!/usr/bin/env python3
import random

def passwordgenerator():
    mypw = ""
    count = 0
    alphabet_number =    "abcdefghijklmnopqrstuvwxyz0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZ"
    symbols = "!?@%#$"
    running1 = True
    while running1:

        if len(symbols) == 1:

            #  Reset symbols
            symbols = "!?@%#$"

        if len(alphabet_number) == 1:

            #  Reset letters/numbers
            alphabet_number = "abcdefghijklmnopqrstuvwxyz0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZ"

        elif count == pw_length:

            if mypw.isupper() or mypw.islower():  # check if there is only upper     or only lower letter and rerun if True
                passwordgenerator()

            else:
                x = mypw
                y = list(x)  # creates a list and put the pw in to shuffle it
                random.shuffle(y)
                mypw = "".join(y)

                print(mypw)
                input("\nPress <Enter> to close the program")
                running1 = False

        elif count % 4 == 0:

            #  pick a random symbol every 3 loop and add it to the pw
            symbols_index = random.randrange(len(symbols))
            new_symbols = symbols[symbols_index]
            mypw = mypw + new_symbols

            # delete the symbols that is picked so there are no duplicate
            symbols_list_change = symbols.replace(new_symbols, "")
            symbols = symbols_list_change

        else:

            # pick a random number or letter and add it to the pw
            next_index = random.randrange(len(alphabet_number))
            new_alphabetnumber = alphabet_number[next_index]
            mypw = mypw + new_alphabetnumber

            # delete the symbols that is picked so there are no duplicate
            an_list_change = alphabet_number.replace(new_alphabetnumber, "")
            alphabet_number = an_list_change

        count += 1

if __name__ == '__main__':
    print("/!\ 12 Characters is a minimum for good security /!\ ")

    print("=" * 55)  # just to make it pretty

    running = True
    while running:
        pw_length = input("How many Characters do you want?\n")

        if pw_length.isdigit():  # verify if user input a number
            pw_length = int(pw_length)
            passwordgenerator()
            running = False
        else:
            print("A number is needed")

Not bad a all for a first Python program:

Good use of the line: if __name__ == '__main__': .
Good use of string methods (replace, isupper, islower etc...).
Good use of the random module methods.

But some things can be made better:

The function passwordgenerator could have pw_length as a parameter and return mypw. (The printing of the password moves to the main part in this case)
calling passwordgenerator from within passwordgenerator itself (a recursive call) can be tricky (what is the password accidentally is always lower...?)
The string constants "!?@%#$" and "abc...XYZ" occur both twice, they could be made constants on top of the program.
I think the line: elif count == pw_length should be different
a docstring could be added to passwordgenerator explaining the properties of the generated password.


Making currency rate requests in Java with CompletableFuture here is my code to get quotes for forex pairs, using the CurrencyLayer API, with CompletableFuture. We do an HTTP GET, parse the JSON response and map it into my data structures (see sample output at the bottom)

Does this look OK?
I'm worried about thread-safety !! :x

Currency.java:

public enum Currency {
    GBP("GBP", "Pound Sterling"),
    EUR("EUR", "Euros"),
    USD("USD", "US Dollars");

    public final String symbol;
    public final String name;
    Currency(String symbol, String name) {
        this.symbol = symbol;
        this.name = name;
    }
}

CurrencyPair.java:

public class CurrencyPair {
    final public Currency base;
    final public Currency counter;

    public CurrencyPair(Currency base, Currency counter) {
        this.base = base;
        this.counter = counter;
    }
}

CurrencyLayer.java:

import java.util.ArrayList;
import java.util.List;
import java.util.concurrent.CompletableFuture;
import javax.ws.rs.client.Client;
import javax.ws.rs.client.ClientBuilder;
import javax.ws.rs.core.Response;

import com.google.gson.JsonElement;
import com.google.gson.JsonObject;
import com.google.gson.JsonParser;
import models.Currency;

public class CurrencyLayer implements ForexQueryService {
    private final Client client = ClientBuilder.newClient();

    private CompletableFuture<Response> makeRequest(List<CurrencyPair> pairs) {
        final String url = "http://www.apilayer.net/api/live?access_key=[my key ^^]&format=1";
        return CompletableFuture.supplyAsync(() -> client.target(url).request().get());
    }

    private static double getUSDto(JsonObject quotes, Currency c) {
        if (c == Currency.USD) {
            return 1.0;
        } else {
            return quotes.get("USD" + c.symbol.replace("XBT", "BTC")).getAsDouble();
        }
    }

    private static List<ForexRate> convertResponse(Response r, List<CurrencyPair> pairs) {
        final String body = r.readEntity(String.class);

        // log this data
        System.out.println(body);

        JsonElement jelement = new JsonParser().parse(body);
        JsonObject jo = jelement.getAsJsonObject();
        JsonObject quotes = jo.get("quotes").getAsJsonObject();

        List<ForexRate> ret = new ArrayList<>();
        for (CurrencyPair p : pairs) {
            ret.add(new ForexRate(p.base, p.counter, getUSDto(quotes,p.counter)/getUSDto(quotes, p.base)));
        }

        return ret;
    }

    public CompletableFuture<List<ForexRate>> lookUpRates(List<CurrencyPair> pairs) {
        return makeRequest(pairs).thenApply((t) -> convertResponse(t, pairs));
    }
}

ForexRate.java

public class ForexRate {
    public final Currency from;
    public final Currency to;
    public final Double rate;

    public ForexRate(Currency from, Currency to, Double rate) {
        this.from = from;
        this.to = to;
        this.rate = rate;
    }
}

Main.java:

public class Main {
    static public void main(String[] args) {
        CurrencyLayer c = new CurrencyLayer();
        try {
            List<CurrencyPair> pairs = new ArrayList<>();
            pairs.add(new CurrencyPair(Currency.GBP,Currency.USD));
            pairs.add(new CurrencyPair(Currency.JPY,Currency.GBP));

            List<ForexRate> rates = c.lookUpRates(pairs).get();
            for (ForexRate rate : rates) {
                System.out.println(rate.from + " " + rate.to + " -> " + rate.rate);
            }

        } catch (Exception e) {
            e.printStackTrace();
        }
    }
}

Sample output:

{
  "success":true,
  "terms":"https:\/\/currencylayer.com\/terms",
  "privacy":"https:\/\/currencylayer.com\/privacy",
  "timestamp":1505384954,
  "source":"USD",
  "quotes":{
    "USDAUD":1.249706,
    "USDEUR":0.840197,
    "USDGBP":0.75723
  }
}
GBP USD -> 1.320602723082815
EUR GBP -> 0.9012529204460382

Thread-safety

  I'm worried about thread-safety !! :x

I don't see any shared data anywhere,
and as such there are no visible thread-safety issues.

If you called CurrencyLayer.lookUpRates from multiple threads,
then CurrencyLayer.client would be shared and so it would need to be thread-safe.

The current program as it is,
fetches and processes exchange rates practically sequentially,
so there's no visible need for multi-threading,
and doesn't really benefit at all from CompletableFuture.
There is no visible reason for CurrencyLayer.lookUpRates to return CompletableFuture<List<ForexRate>>,
it could as well return a simple List<ForexRate>.

Unused method parameter

private CompletableFuture<Response> makeRequest(List<CurrencyPair> pairs) {
    final String url = "http://www.apilayer.net/api/live?access_key=[my key ^^]&format=1";
    return CompletableFuture.supplyAsync(() -> client.target(url).request().get());
}

The pairs parameter is not used in this method.

Exception handling in main

In this code,
it would be better to remove the try-catch.
The outcome will look the same in case of failure,
but the exit code will be correctly non-zero,
which is currently not the case.

static public void main(String[] args) {
    CurrencyLayer c = new CurrencyLayer();
    try {
        // ...
    } catch (Exception e) {
        e.printStackTrace();
    }
}


C# function to execute queries in a SQL Server database public static DataSet execute_query(string query)
{
    DataSet ds = new DataSet();
    SqlConnection con = new SqlConnection();
    con.ConnectionString = DataAccessLayer.Properties.Settings.Default.cn;
    try
    {
        con.Open();
        SqlDataAdapter da = new SqlDataAdapter(query, con);
        da.Fill(ds);
    }
    catch (Exception ex)
    {
        ds = null;
    }
    finally
    {
        if (con.State == ConnectionState.Open) con.Close();
    }

    return ds;
}

SqlConnection implements the IDisposable interface, and so does the SqlDataAdapter class - any type that implements IDisposable should have its Dispose method called; by disposing the connection you don't need to explicitly close it.

The best way to ensure Dispose gets called is to wrap the object in a using block, which is basically compiler magic for a try/finally block.

using (SqlConnection connection = new SqlConnection(connectionString))
{
    connection.Open();
    using (SqlDataAdapter adapter = new SqlDataAdapter(sql, connection))
    {
        DataSet result = new DataSet();
        adapter.Fill(result);
        return result;
    }
}

The SqlAdapter+DataSet way will work on any version of .net - but it's also a pretty dated way to go about it, and since DataSet also implements IDisposable, returning it makes for awkward code, because normally it's the responsibility of the code that creates an IDisposable object to call its Dispose method - but if you do that (or wrap it in a using block), then the client code will run into an ObjectDisposedException when it tries to use the data set.

Slightly more advanced is the ADO.NET SqlCommand, which lets you create SqlParameter objects to send a parameterized command to the SQL Server. This puts the responsibility of dealing with parameters on the database server, where it belongs (Greg Burghardt's answer hints at this).

If you're not constrained to .net 2.0 then the next best thing would be LINQ-to-SQL, or Entity Framework (or NHibernate, or Dapper, depending on your needs): these more modern tools often eliminate the need to ever concatenate SQL strings, or even to explicitly create SqlParameter objects for a parameterized query - for example this:

public User GetById(int id)
{
    using (var context = new MyDbContext(connectionString))
    {
        return context.Users.SingleOrDefault(user => user.Id == id);
    }
}

Generates a SQL query similar to:

SELECT TOP 1 Id, FirstName, LastName, BirthDate, Gender
FROM dbo.Users
WHERE Id = @id

...without ever needing to explicitly create a parameter or concatenate an arbitrary string into a bit of SQL - and C# looks so much better without SQL string literals all over the place! And the best part, it returns a User object ("entity") instead of a DataTable.

But it's great that you're starting with the lower-level stuff: lots of people start with Entity Framework and don't quite understand what's happening under the hood.

Other issues:

The method being static hints at procedural-style. C# is multi-paradigm, but the overwhelmingly dominant one is the object-oriented one: by working with static methods, you're working with types, not objects. The difference will start showing when you start measuring things like coupling and testability.
Method names should be PascalCase, so execute_query should be ExecuteQuery.
query isn't an ideal name for a string containing an SQL statement, because starting in .NET 3.5 a query is a good name for an IQueryable<T> object that's actually building a query via a LINQ provider. Keep it simple: sql is a perfectly acceptable and descriptive name.


VBA script that copies data from one workbook to another I've written a perl script that creates a csv file consolidated of values from multiple files. This file that I've created is then used to update certain rows in a main excel spreadsheet my company uses. I've created a VBA script that scans the main file for values that match up with those from my perl generate file and it all works fine.

The problem is that it takes about four minutes to run. The main file is only about 1000 or so rows long, and the file I'm generating only comes out to about 100 rows top. My logic in my script is to scan through every row in the main file for each row in the generated file:

Sub TransferDataToForecast()
  Dim strPath As String  'File path
  Dim foreb As Workbook  'Workbook that the file path points to
  Dim copys As Worksheet 'Worksheet that contains the data to be copied
  Dim dests As Worksheet 'Worksheet that is being copied to
  Dim copysLastRow As Long 'Last row of data worksheet
  Dim copysLastCol As Long 'Last column of data worksheet
  Dim destsLastRow As Long 'Last row of target worksheet
  Dim counter As Long      'Counter for loop control

  Application.ScreenUpdating = False
  Set copys = ActiveWorkbook.ActiveSheet

  'Get user to select target workbook
  strPath = BrowseForFile("Select the DM Forecast file you want to update")
  If strPath = vbNullString Then Exit Sub

  'Debug.Print strPath
  Set foreb = Workbooks.Open(strPath)
  Set dests = foreb.Worksheets("Material_Usage_Prim")

  'Get last row of each worksheet and last column of data sheet in order to help with loop control
  copysLastRow = copys.Cells(Rows.Count, 3).End(xlUp).Row
  copysLastCol = copys.Cells(2, Columns.Count).End(xlToLeft).Column
  destsLastRow = dests.Cells(Rows.Count, 1).End(xlUp).Row
  counter = 0

  'Loop through each row of the data sheet
  For i = 2 To copysLastRow
  Debug.Print "i is "; i
  'For each row in the data sheet, loop through every row in the destination sheet in order
  'to find matching id's between each sheet
    For j = 2 To destsLastRow
      Debug.Print "j is "; j
      If Trim(copys.Cells(i, 3).Value2) = vbNullString Then Exit For   'This exits the loop if the data sheet has a blank row
      If dests.Cells(j, 4) <> copys.Cells(i, 4) Then GoTo NextDestLoop 'This skips the rest of the current j if values in col4 of each sheet don't match up

      'Have to use replace due to differing naming conventins among people i.e. _ vs - (this row can be changed for specific peoples needs)
      'This checks that columns 3 and 4 of each sheet match
      If Replace(copys.Cells(i, 3).Value2, "_", "") = Replace(dests.Cells(j, 3).Value2, "_", "") And copys.Cells(i, 4).Value2 = dests.Cells(j, 4).Value2 Then
        counter = 0
        For k = 5 To copysLastCol
            Debug.Print "k is "; k
            'Go to 14, because we want to keep the first 14 weeks (columns)
            If counter < 14 Then
                counter = counter + 1
                GoTo NextIteration
            End If
            counter = counter + 1
            dests.Cells(j, k).Value2 = copys.Cells(i, k).Value2
     NextIteration:
            Next k
      End If
      If counter = copysLastRow Then GoTo NextLoop
   NextDestLoop:
    Next j
NextLoop:
  Next i

  Application.ScreenUpdating = True
  MsgBox ("Data has been transferred.")
End Sub

'This function brings up a window for the user to pick an excel file they want to update
Private Function BrowseForFile(Optional strTitle As String) As String
  Dim fDialog As FileDialog
    On Error GoTo Err_handler
    Set fDialog = Application.FileDialog(msoFileDialogFilePicker)
    With fDialog
        .Title = strTitle
        .Filters.Add "Excel Files", "*.xlsx; *.xlsm; *.xls; *.xlsb", 1
        .AllowMultiSelect = False
        .InitialView = msoFileDialogViewList
        If .Show <> -1 Then GoTo Err_handler:
        BrowseForFile = fDialog.SelectedItems.Item(1)
    End With

lbl_Exit:
    Exit Function
Err_handler:
    BrowseForFile = vbNullString
    Resume lbl_Exit
End Function

If anyone can help me to make this more efficient I would be grateful!

Option Explicit

The counter variables were never declared.

MSDN Option Explicit Statement:

  Note Use Option Explicit to avoid incorrectly typing the name of an existing variable or to avoid confusion in code where the scope of the variable is not clear.

Goto Statement

Goto Statement are best reserved for error handling.

MSDN Goto Statement:

  Note Too many GoTo statements can make code difficult to read and debug. Use structured control statements ( Do...Loop, For...Next, If...Then...Else, Select Case ) whenever possible.

ID Lookups

Using nested loops over multiple lists comparing unique values (IDs) is very inefficient.  Use a Dictionary. They are optimised to reference IDs stored as Key/Value pairs.

K Loop

        Debug.Print "k is "; k
        'Go to 14, because we want to keep the first 14 weeks (columns)
        If counter < 14 Then
            counter = counter + 1
            GoTo NextIteration
        End If
        counter = counter + 1
        dests.Cells(j, k).Value2 = copys.Cells(i, k).Value2
 NextIteration:
        Next k

Why would you not just start K at 14?????

Refactored Code

Using a Dictionary to store value Arrays by key ID, this code took 11.27 seconds to transfer 10K rows x 12 Columns of data.  I better computer would easily cut that time in helf.  My IndexedArray Class takes about 0.6 seconds per 10K Rows x 7 Columns.

Sub TransferDataToForecast2()
    Dim r As Long, c As Long
    Dim Key As Variant, Values As Variant
    Dim strPath As String
    Dim dicID As Object
    Set dicID = CreateObject("Scripting.Dictionary")

    'Get user to select target workbook
    strPath = BrowseForFile("Select the DM Forecast file you want to update")
    If strPath = vbNullString Then Exit Sub

    Application.Calculation = xlCalculationManual
    Application.ScreenUpdating = False
    With ActiveWorkbook.ActiveSheet                   'Worksheet that contains the data to be copied

        For r = 2 To .Cells(Rows.Count, 3).End(xlUp).Row
            If Len(Trim(.Cells(r, 3).Value2)) > 0 Then
                Key = Replace(.Cells(r, 3).Value2, "_", "") & "|" & .Cells(r, 4).Value2
                If dicID.Exists(Key) Then
                    Debug.Print "Duplicate ID Found:"; .Name; " - "; .Cells(r, 3).Address
                Else
                    ' Add the ID and Array of Values
                    Values = .Range(.Cells(r, 14), .Cells(r, .Columns.Count).End(xlToLeft)).Value
                    dicID.Add Key, Values
                End If
            End If
        Next
    End With

    With Workbooks.Open(strPath).Worksheets(1)
        For r = 2 To .Cells(.Rows.Count, 3).End(xlUp).Row
            If Len(Trim(.Cells(r, 3).Value2)) > 0 Then
                Key = Replace(.Cells(r, 3).Value2, "_", "") & "|" & .Cells(r, 4).Value2
                If dicID.Exists(Key) Then
                    Values = dicID(Key)
                    .Cells(r, 14).Resize(1, UBound(Values, 2)).Value2 = Values
                End If
            End If
        Next
    End With

    Application.Calculation = xlCalculationAutomatic
    Application.ScreenUpdating = True

    MsgBox ("Data has been transferred.")
End Sub


Implementation of the binary search algorithm This is my first implementation the binary search algorithm. I have tested it and so far it is okay. But I need an experienced eye to review the code and recommend best practices.

bool search(int value, int values[], int n) {
  bool x = false;
  int *max;
  int *min;
  int *mid;
  min = values;
  max = min + n - 1;
  mid = min + n / 2;
  while (true) {
    if ((value == *mid || value == *min || value == *max)) {
      x = true;
      break;
    } else if (max - min <= 1) {
      x = false;
      break;
    } else if (value < *mid) {
      n = n / 2;
      mid = min + n / 2;
      max = min + n - 1;
    } else if (value > *mid) {
      n = n / 2;
      mid = max - n / 2;
      min = max - n - 1;
    }
  }

  return x;
}

Variable names

The name x is a very bad name in this case. You should change it to found or something. Even better instead of

x = true;
break;

just do:

return true;

and change the last: return x; to return true;. It is always good to eliminate variables if you can.

Don't modify parameters/arguments

It is confusing when you change the value of the input parameters for example with n = n/2;. This makes the code hard to follow. Instead choose mid as mid = min + (max - min + 1)/2. This makes your intent much clearer.

Avoid while(true) when possible

Some times it is impossible to avoid an infinite loop such as while(true) this is not the case. You should avoid formulating infinite loops like this because it makes it harder to analyze the termination condition of the loop to verify that it will terminate.

while(true)

should be:

while(max - min > 0 )

Simplify branches by realising invariants

Realize that mid is always defined by the min and max variables hence we can define it inside of the loop as a temporary. By doing so and keeping in mind that the array is sorted we can simplify the code as follows:

bool search(int value, int values[], int n) {
  int *min = values;
  int *max = min + n; // Max is exclusive

  while (max - min > 1) {
    int* mid = min + (max - min)/2;
    if(value < *mid){
        max = mid; // Max still exclusive as value < *mid
    }
    else if( value > *mid){
        min = mid;
    }
    else{ // Its not smaller and not larger. Must be equal.
        return true;
    }
  }
  return *min == value;
}