Creating a ES6 class for every API endpoint's response Background
I am writing a Node app that queries a third-party Web API.
The API returns JSON data in various formats, each endpoint in a different format. My app later processes this data.
I have decided to create a separate class representing each endpoint's response data, and convert the JSON received from the server into an instance of the class, rather than keeping it as a plain old object.
Goal
My goal is to increase visibility of what fields are available inside that JSON from server, and simplify coding by using auto-completion that my IDE (VS2017) will now provide.
However, the code looks a bit redundant - I am basically copying all properties from one object into another.
Question
I want to know your thoughts whether this approach makes sense, is an overkill or if there's a better way to do it.
Code
Here's some basic code how I query the server:
Api.prototype.publishApp = async function (appId, versionId) {
  let url = config.luis.paths.publishApp.replace('{appId}', appId);
  let serverResponse = await queryEndpoint(this.key, url, HttpMethod.POST, HttpStatus.CREATED);
  return new AppPublishResponse(serverResponse); // convert object to class instance
};

And that's the class - I create one class for each endpoint:
/**
 * Represents a response from LUIS when publishing an app
 */
class AppPublishResponse {
  constructor(serverResponse) {
    this.versionId = serverResponse.versionId;
    this.isStaging = serverResponse.isStaging;
    this.endpointUrl = serverResponse.endpointUrl;
    this.region = serverResponse.region;
    this.assignedEndpointKey = serverResponse.assignedEndpointKey;
    this.publishedDateTime = serverResponse.publishedDateTime;
  }
}

Just a note, the queryEndpoint() function above makes sure that the server response code matches the expected one (e.g. 201 Created), so I can be sure that the data passed to the constructor of AppPublishResponse is always a valid server response.

Hard coding outside data source should be avoided.
Hard coding outside data sources will present an ongoing workload. This will be compounded by the fact there is no validation of data.
At anytime the data source may change, add, or remove properties. You will not know about the change (if you don't personally follow the source doc feed) until it fails or a customer/consumer reports the problem, or worse just stops using it.
Testing for a valid response will not protect against modified data.
Even if you validate the data it will just improve response time for the fix / update but not the underlying problem.
The cost.
Every change will require an update, test, documentation, deploy, monitor, release, and pray cycle for the life of the service. As a business I would have to pass this cost/ risk on to the client or revenue source, and having a programmer on call is very expensive. In other words this type of design can kill a service.
For the sake of auto complete it is not worth it.
You are best of generating something that Vs2017 can read (I don't use it so you will have to look in the docs) and hard coding as little as possible that is outside your direct control.


Add item after every nth step The purpose of the method is to add a list of items after every nth step (5 for example) where the input items are repeated if needed. I'm looking for an overall review and possible how to remove the use of the while and replace it with a for.

public static List<string> AddEveryNthStep(List<string> items, List<string> itemsToAdd, int nthStep)
{
    List<string> output = new List<string>();

    int itemIndex = 0;
    int itemsToAddIndex = 0;
    int index = 1;

    while (itemsToAddIndex < itemsToAdd.Count)
    {
        output.Add(items[itemIndex++]);

        if (itemIndex == items.Count)
        {
            itemIndex = 0;
        }

        // We are at at point where we want to add a item
        if (index % nthStep == 0)
        {
            output.Add(itemsToAdd[itemsToAddIndex++]);
        }

        index++;
    }

    return output;
}

Usage:

List<string> items = new List<string> { "Item 1", "Item 2", "Item 3", "Item 4" };
List<string> itemsToAdd = new List<string> { "Item to add 1", "Item to add 2", "Item to add 3" };

Console.WriteLine(String.Join(Environment.NewLine, AddEveryNthStep(items, itemsToAdd, 5)));

Output:

Loops and Indexes

Why are you using a while loop? A for loop keeps track of indexes for you. Admittedly you do still need to track one index yourself, but you can eliminate one to loop processing:

int addIndex = 0;
for (int i = 0; i < items.Count; i++) {
    output.Add(items[i]);

    if (i % nthStep == 0) {
        output.Add(itemsToAdd[addIndex++]);
    }

    if (i == (items.Count - 1) && addIndex < (itemsToAdd.Count - 1)) {
        i = 0;
    }
}

In words:

define an index for the items you're adding
loop through the items list, on every pass:

add the current item to the output list
if the current step is the nth step, add the next item to add and increase the index
if we've run out of items but have more left to add, reset the items index so we loop back through

Now you're only tracking and handling one index yourself, instead of the three that your while loop requires.


Find number of neighbour pairs in the input tree My code works with small binary tree but with big ones (around 10000 nodes) it takes 15 sec to process.

For small binary tree I'm always getting correct answer and it is fast.

How can I make my program faster for big binary tree?

  (x, y) is a neighbour pair when the depth of x and y is the same, the
  node color of x and y is the same, the node key of x and y are equal
  and there is no other node of the same node color between x and y on the horizontal line connecting x and y in standard drawing..

  Input

  The first line of input contains number of nodes and root node. Next,
  each line describes each node.

  The first value is the node label, the second value is the node key,
  the third and the fourth values represent the labels of the left and
  right child respectively, and the fifth value represents the node
  color, white is 0, black is 1. If any of the children does not exist
  there is value 0 instead of the child label at the corresponding place

  Output

  number of neighbour pairs in the input tree

My code

from collections import defaultdict
def read_input(inputstring):
    inputs = inputstring.split(" ")
    nodes_number = int(inputs[0])
    root_node = int(inputs[1])
    input_tree = [list(map(int, input().split())) for _ in range(nodes_number)]
    tree = {}
    result = {}
    def depth(node, count):
        for i in input_tree:
            node_label = i[0]
            if node_label == node:
                tree.setdefault(count, []).append(i)
                l_node, r_node = i[2], i[3]
                if l_node != 0 and r_node != 0:
                    depth(l_node, count+1)
                    depth(r_node, count+1)

                elif l_node != 0:
                    depth(l_node, count+1)

                elif r_node != 0:
                    depth(r_node, count+1)
    depth(root_node, 0)

    def neighbour_pair(input_tree):
        for value in input_tree.values():
            for i in range(len(value)):
                for j in range(len(value)):
                    if j > i:
                        if value[i][1] == value[j][1] and value[i][4] == value[j][4]:
                            result.setdefault(value[i][0], []).append(value[j][0])
    neighbour_pair(tree)
    return len(result)
if __name__ == '__main__':
    print(read_input(input()))

Example of small input

13 5
7 50 0 0 1
2 70 10 11 0
4 30 9 0 0
6 70 0 0 0
1 90 8 12 0
9 90 0 2 1
13 90 0 6 0
5 30 4 3 0
12 80 0 0 1
10 50 0 0 1
11 50 0 0 0
3 80 1 13 0
8 70 7 0 1

Its associated representation:

The labels are written in the yellow rectangle at the corresponding nodes. Their key is the number in the black or white oval. For this example, the neighbour pairs are (1, 13), (2, 6), (10, 7).

As such, the output of the program for this tree is:

3

Readability counts

Overall, conventions are respected: case, spaces… But there are still some things missing to make your code feel readable:

vertical spacing: you should add blank lines before functions definition and important logical sections of the code so we can grasp the structure at a glance.
naming: one letter variable are not great, especially when conventions dictates an other use of that letter than the one you make (I’m looking at you for i in input_tree:).
namedtuples can help give meaning to your lines of input:

from collections import namedtuple

Node = namedtuple('Node', 'id key left_child right_child color')

def read_input(inputstring):
    …
    input_tree = [Node(*map(int, input().split())) for _ in range(nodes_number)]
    …
        for node in input_tree:
            node_label = node.id
            …
                l_node, r_node = node.left_child, node.right_child

and so on.
You don't need to define nested functions, you can pass them parameters. It will be easier to test (for correctness or speed). In fact, you should separate further the computation bits from the parsing of the input.

You also import defaultdict but never use it. Instead, your code contains a few setdefault(…, []) on regular dictionaries. You should declare tree = defaultdict(list) and result = defaultdict(list) instead.

Building the depth of nodes

For each node (technically each node id), you iterate over the whole input_tree and then recurse, iterating over the whole input_tree for the two children.

This is a huge waste of time. Instead of iterating over a list to find a node by its id, you should use a datastructure that let you access a node by its id directly. A simple dictionary where keys are ids and values are node should suffice.

Computing neighbour pairs

First of, you could start your last loop at i+1 so you don't need the if j > i: test. But still, you don't even need that second loop at all: by keeping track of the last white node and the last black node you could only iterate once for each depth.

Proposed improvements

from collections import defaultdict, namedtuple

Node = namedtuple('Node', 'id key left_child right_child color')

def read_nodes(nodes_count):
    for _ in range(nodes_count):
        yield Node(*map(int, input().split()))

def parse_input():
    nodes_count, root_node = map(int, input().split())
    tree = {node.id: node for node in read_nodes(nodes_count)}
    return root_node, tree

def build_depth(node_id, tree, storage, current_depth=0):
    node = tree[node_id]
    storage[current_depth].append(node)
    for child in (node.left_child, node.right_child):
        if child:
            build_depth(child, tree, storage, current_depth + 1)

def neighbour_pairs(layered_tree):
    neighbours_count = 0 # neighbours = defaultdict(list)
    for nodes in layered_tree.values():
        last_black = Node(0, None, 0, 0, 1)
        last_white = Node(0, None, 0, 0, 0)
        for node in nodes:
            if node.color == 1:
                last_node, last_black = last_black, node
            else:
                last_node, last_white = last_white, node
            if last_node.key == node.key:
                neighbours_count += 1 # neighbours[last_node.id] = node.id
    return neighbours_count # return neighbours

if __name__ == '__main__':
    root, tree = parse_input()
    nodes_by_depth = defaultdict(list)
    build_depth(root, tree, nodes_by_depth)
    neighbours = neighbour_pair(nodes_by_depth)
    print(neighbours) # print(len(neighbours))

You can see that I’m not storing the pairs of neighbours, it will help speed things up. But I left comments if you still want to be able to retrieve them to test for correctness.


LeetCode: Sliding Window Median C# https://leetcode.com/problems/sliding-window-median/

  Median is the middle value in an ordered integer list. If the size of
  the list is even, there is no middle value. So the median is the mean
  of the two middle value.

  Examples: [2,3,4] , the median is 3

  [2,3], the median is (2 + 3) / 2 = 2.5

  Given an array nums, there is a sliding window of size k which is
  moving from the very left of the array to the very right. You can only
  see the k numbers in the window. Each time the sliding window moves
  right by one position. Your job is to output the median array for each
  window in the original array.

  For example, Given nums = [1,3,-1,-3,5,3,6,7], and k = 3.

  Therefore, return the median sliding window as [1,-1,-1,3,5,6].

  Note: You may assume k is always valid, ie: k is always smaller than
  input array's size for non-empty array. Answers within 10^-5 of the
  actual value will be accepted as correct.

Please review for performance. also I am not sure about GetID function is there a smarter way to get the same result of unique number and sorting for median?

using System;
using System.Collections.Generic;
using Microsoft.VisualStudio.TestTools.UnitTesting;

namespace TreeQuestions
{
    /// <summary>
    /// https://leetcode.com/problems/sliding-window-median/
    /// </summary>
    [TestClass]
    public class SlidingWindowMedianTest
    {
        [TestMethod]
        public void ExampleTest()
        {
            int[] nums = { 1, 3, -1, -3, 5, 3, 6, 7 };
            int k = 3;
            double[] expected = { 1, -1, -1, 3, 5, 6 };
            double[] res = SlidingWindowMedian.MedianSlidingWindow(nums, k);
            CollectionAssert.AreEqual(expected, res);
        }

        [TestMethod]
        public void ExampleTestFailed()
        {
            int[] nums = { 1, 4, 2, 3 };
            int k = 4;
            double[] expected = { 2.5 };
            double[] res = SlidingWindowMedian.MedianSlidingWindow(nums, k);
            CollectionAssert.AreEqual(expected, res);
        }

        [TestMethod]
        public void ExampleTestOverFlow()
        {
            int[] nums = { 2147483647, 2147483647 };
            int k = 2;
            double[] expected = { 2147483647.0 };
            double[] res = SlidingWindowMedian.MedianSlidingWindow(nums, k);
            CollectionAssert.AreEqual(expected, res);
        }

        [TestMethod]
        public void ExampleFailed2()
        {
            int[] nums = { 5, 2, 2, 7, 3, 7, 9, 0, 2, 3 };
            int k = 9;
            double[] expected = { 3.0, 3.0 };
            double[] res = SlidingWindowMedian.MedianSlidingWindow(nums, k);
            CollectionAssert.AreEqual(expected, res);
        }
    }

    public class SlidingWindowMedian
    {
        public static double[] MedianSlidingWindow(int[] nums, int k)
        {
            var res = new List<double>();
            var med = new SortedList<long, int>();
            for (int i = 0; i < nums.Length; i++)
            {
                med.Add(GetId(i, nums), nums[i]);
                if (med.Count > k)
                {
                    med.Remove(GetId(i - k, nums));
                }

                if (med.Count == k)
                {
                    if (k % 2 == 0)
                    {
                        res.Add( ((long)med[med.Keys[k / 2 - 1]]  + med[med.Keys[k / 2]])/2.0 );
                    }
                    else
                    {
                        res.Add(med[med.Keys[k / 2]]);
                    }
                }
            }
            return res.ToArray();

        }

        private static long GetId(int i, int[] nums)
        {
            return Convert.ToInt64(nums[i]) * nums.Length + i;
        }
    }

}

Your ID is a long created from 2 ints, so you can fit both ints inside this one long side by side:

long key = (long)nums[i] << 32 | i;

You have a lot of unnecessary if statement evaluations. You could get rid of them by adding the first k elements from the array into the list in a separate loop, and then starting a second loop from i = k that can safely do med.Remove and med.Add without ifs.

k never changes, so instead of evaluating k % 2 == 0 and k / 2 and k / 2 - 1 in every iteration, you can store the final value in a variable before the loop begins. Then in the loop you only need to call

res.Add(med[med.Keys[medianIndex]]);

or

res.Add( ((long)med[med.Keys[medianIndex]] + med[med.Keys[medianIndex2]])/2.0 );

Making res a list and in the end converting it into an array slows your code down a lot. You can make it an array (of size nums.Length - k + 1). Lists come with the overhead of their extra functionality, so you waste time not only on the conversion to an array but also on every time you add a number to the list.


C++ Read File line by line I wanted to have a good review on my code I wrote today to read files in C++ line by line. The file can look something like this:

# Program configuration

network.port=30000 # The port the server uses
network.ip=127.0.0.1 # The IP the server uses

Notes:

Lines that start with # won't be processed
Lines that are empty won't be processed
All the data before the # on a line will be processed
The data will be split by =, before the = the key and after it the value

configuration.h:

#pragma once
#include <fstream>
#include <map>
#include <sstream>
class configuration
{
public:
    configuration(const char* path);
    ~configuration();
    std::string get_value(const char* key);
private:
    std::map<std::string, std::string> values;
    std::ifstream file;
};

configuration.cpp:

#include "configuration.h"
#include <Windows.h>
#include <iostream>
#include "logger.h"

configuration::configuration(const char* path)
{
    try
    {
        file.open(std::string(path), std::ios::in);
        values = std::map<std::string, std::string>();

        if (file.is_open())
        {
            std::string line;
            while (std::getline(file, line))
            {
                if (line.find("#") != 0 && !line.empty() && line.find("="))
                {
                    std::string part = line.substr(0, line.find("#"));
                    std::string key = part.substr(0, line.find("="));
                    std::string value = part.substr(line.find("=") + 1);
                    values.insert_or_assign(key, value);
                }
            }
        }
        else
        {
            logger::log_error("File %s is not open!", path);
        }
    }
    catch (const std::ios_base::failure& ex)
    {
        logger::log_error(ex.code().message().data());
    }
}

std::string configuration::get_value(const char* key)
{
    return values[key];
}

configuration::~configuration()
{
    file.close();
}

Usage would be:

configuration* config = new configuration("file.cnf");
config->get_value("network.port");

Regarding the API:

consider using std::string const & arguments instead of char const*. They are less error prone.
your configuration uses a std::ifstream input that is only used in the constructor; the file should not be a member of the configuration.
the code to read the configuration from a stream should (probably) be put into an std::istream& operator>>(std::istream&, configuration&);. This would:

integrate well with existing APIs on streams
abstract away the concrete type of the stream (your configuration would be able to read a string in memory - using a std::istringstream just as easily as a file on the disk)
separate concerns
allow you to test the configuration class in separation

Consider this public API instead:

class configuration final
{
public:
    using storage_type = std::map<std::string,std::string>;

    /// \b instantiate from set of values (that can be empty)
    explicit configuration(storage_type values = {}) noexcept;

    ///. \note default destructor is enough to have - so don't declare it here

    /// \return a const reference instead of an object
    ///
    /// we are not modifying the class so we can be const and noexcept
    const std::string&
    get_value(const std::string& key) const noexcept
    {
        using namespace std::string_literals;
        try
        {
            return values.at(key);
        }
        catch(std::out_of_bounds const&)
        {
            static const auto empty = ""_s;
            return empty;
        }
    }

private:
    storage_type data_;
};

Regarding the implementation (general notes):

try
{
    file.open(std::string(path), std::ios::in);

consider instantiating a file with a file name (opening directly from the construction instead of explicitly calling open)

if (file.is_open())

this is bad: the file could be open, but the stream could be in an invalid state; when you want to check on a stream's state for reading, simply rely on it's boolean conversion (use if(file) instead).
this could be a simple assignment: values.insert_or_assign(key, value);: values[key] = value;
logging messages in case of errors is very very bad:

it adds a dependency to a logging module (every time you want to reuse the class somewhere else, you will also have to add the logging dependencies)
it does too much: when you want to reuse the code you will want to do that for reading a configuration, not for logging. Logging should not be in here at all.
it gives you no possibility for checking for an error in client code, unless your logging framework has extra functionality to give you the last of it's error messages and you want to parse those (and all of a sudden you are solving the x-y problem)
you should either catch the errors here (like you already do) and throw a higher abstraction exception instead, or simply remove the try-catch block and allow client code to catch exceptions - or not. One of the best features of exceptions is that you can choose not to handle anything at the current level and you have to write no explicit code for that.

Final reading code for this:

std::istream& operator>>(std::istream& in, configuration& c)
{
    auto storage = storage_type{};
    auto line = std::string{};
    while(getline(in, line))
    {
        if(line.empty())
            continue;
        auto split = line.find_first_of("#=");
        if (split == std::string::npos)
            throw std::runtime_error{ "bad data format" };

        // strip comment
        if(line[split] == '#')
            line = line.substr(0, split);
        ...
    }

    c = configuration{ std::move(storage) };
    return in;
}

Edit: here's some client code, that is possible due to the separation of the parsing and the construction:

#include <sstream>
#include <fstream>

// test code
std::istringstream test_input{ R"(
# Program configuration

network.port=30000 # The port the server uses
network.ip=127.0.0.1 # The IP the server uses)"};

auto c1 = configuration{};
assert(test_input >> c1);

// useful code:
std::ifstream in{ "file.cnf" };
auto c2 = configuration{};
if!(in >> c2)
    std::cerr << "Error reading file.cnf\n";


Design a Tips class that calculates the gratuity on a restaurant meal Design a Tips class that calculates the gratuity on a restaurant meal.

  Its only class member variable, taxRate, should be set by a
  one-parameter constructor to whatever rate is passed to it when a Tips
  object is created. If no argument is passed, a default tax rate of
  .065 should be used.

  The class should have just one public function, computeTip. This
  function needs to accept two arguments, the total bill amount and the
  tip rate. It should use this information to compute what the cost of
  the meal was before the tax was added. It should then apply the tip
  rate to just the meal cost portion of the bill to compute and return
  the tip amount.

  Demonstrate the class by creating a program that creates a single Tips
  object, and then allows the program user to retrieve the correct tip
  amount using various bill totals and desired tip rates.

What can I do better?

#include<iostream>
#include <string>

using namespace std;

class Tips
{
       private :
           double taxRate;

       public:
          Tips()
          {
                taxRate = .65;
                cout << "Tax rate is "<<taxRate<<endl;
          }

          Tips(double tax)
          {
                     taxRate = tax;
                     cout << "Tax rate is"<<taxRate<<endl;
          }

          double computeTip(double billAmount,double tipRate)
          {
                double mealCost = billAmount - billAmount*(taxRate);

                cout << "Meal cost is with no tax "<<mealCost<<endl;
                return mealCost+tipRate;
          }
};

int main()
{
    Tips tip ;
    cout << "Meal cost + tip rate is "<<tip.computeTip(100,20)<<endl;

    return 0;
}

Inconsistant layout:

#include<iostream>   // No space
#include <string>    // Space

Be consistent in your code. I prefer the version with the space.

Using namespace std

Arrrrr. Don't do that. see: Why is “using namespace std;” considered bad practice?.

Definately never do it in a header file. You are pulliting the namesapce for anybody that includes your code (which can change behavior) and break other peoples code. Don't do it in source files because of the reasons you read in the linked article.

Default argument

      Tips()
      {
            taxRate = .65;
            cout << "Tax rate is "<<taxRate<<endl;
      }

      Tips(double tax)
      {
                 taxRate = tax;
                 cout << "Tax rate is"<<taxRate<<endl;
      }

When you see code like this you can usually replace it with a single constructor that has a parameter with a default argument (as they are basically doing the same thing).

      Tips(double tax = 0.065) // Corrected you constant.
      {
           taxRate = tax;
           cout << "Tax rate is"<<taxRate<<endl;
      }

Be consistent with spacing (white space is your friend)

           cout << "Tax rate is"<<taxRate<<endl;
                ///            ^^^^^^  ^^^^^        Looking very tight here.

           cout << "Meal cost + tip rate is "<<tip.computeTip(100,20)<<endl;
                ////                        ^^^^^                   ^^^^

Prefer '\n' over std::endl

std::end forces a flush of the stream. This is never what you actually want. The automated flushing of the stream will work much better than manual flushing.

Prefer to use the initializer list.

      Tips(double tax = 0.065) // Corrected you constant.
           :   taxRate(tax)
      {
           cout << "Tax rate is"<< taxRate << endl;
      }

Arithmetic issue

If the billAmount is mealCost * (1 + taxRate) then this is not the formula to calculate meal cost.

            double mealCost = billAmount - billAmount*(taxRate);


Multiplying corresponding integers using streams While reviewing Multiplying Lists, I got the impression that the problem was naturally suited to Java 8 streams.  Unfortunately, since there is no convenient way to zip two streams, the solution ended up being uglier than I expected.

The task is to take lines of input, in the form a0 a1 a2 … an | b0 b1 b2 … bn, where the ai and bi are integers, and output the products a0b0 a1b1 a2b2 … anbn.

import java.io.File;
import java.io.FileNotFoundException;
import java.util.*;
import java.util.stream.Collectors;
import java.util.stream.IntStream;
import java.util.stream.StreamSupport;

public class StreamMultiplier {
    public static IntStream multiply(IntStream aStream, IntStream bStream) {
        PrimitiveIterator.OfInt a = aStream.iterator(),
                                b = bStream.iterator();
        PrimitiveIterator.OfInt product = new PrimitiveIterator.OfInt() {
            public boolean hasNext() { return a.hasNext() && b.hasNext(); }
            public int nextInt() { return a.next() * b.next(); }
        };
        return StreamSupport.intStream(
            Spliterators.spliteratorUnknownSize(product, Spliterator.ORDERED),
            false
        );
    }

    public static IntStream toIntStream(String spaceDelimitedInts) {
        return Arrays.stream(spaceDelimitedInts.trim().split("\\s+"))
                     .mapToInt(Integer::parseInt);
    }

    public static void main(String[] args) throws FileNotFoundException {
        Scanner input = (args.length > 0) ? new Scanner(new File(args[0]))
                                          : new Scanner(System.in);

        while (input.hasNextLine()) {
            String[] halves = input.nextLine().split("\\|", 2);
            System.out.println(
                multiply(toIntStream(halves[0]), toIntStream(halves[1]))
                .mapToObj(String::valueOf)
                .collect(Collectors.joining(" "))
            );
        }
    }
}

Is this use of Spliterators correct, and can it be improved?

Streams are not designed to naturally fit every computational situation in Java. As you have found, you have needed to perform a number of compound/stream/Iterator/array state changes.

Your data starts as a compound structure in a single file
you scan that using a Scanner, using the Scanner's next structures.
You split the lines in to a 2-element array
you split each element using regex mapped to a parser to get two int arrays
you use the array-stream to convert the two arrays to int streams
you convert the streams to lock-step primitive int iterators
you re-combine the lock-step product result in to another primitive int iterator
you convert that iterator in to a stream
you map the product stream to strings
you collect-and-print the results as a single line.

Note that your work contains far more data state changes than would be necessary using non-stream functionality. The fact that you have to scan the entire line and convert all values to ints means that all the data is in int[] arrays already, and there is no real benefit for any optimization in the streams using short circuiting.
Additionally, your result is not able to be run on a parallel stream because it would not survive in the Iterator stage.
Correlating two active streams is not an operation that is supported using any 'natural' mechanism in Java8.
But, there are ways to do it using non-stream mechanisms, and also much more natural Java8 ways too.
Stream Solution
First up, use a stream for the file IO. Files.lines(...) provides a stream of the lines in a file. Next up, instead of streaming over the values in the source arrays, use a stream of what the two arrays have in common, the indices. Stream over the indices.
By streaming the indices you can process the data in a natural stream way and pull the data by index from the corresponding positions in the sources.
Finally, the product operator should be supplied as an input lambda, and not built in to the stream-iterator-stream translation.
The solution I would use, if forced to use streams for as much as possible, would be:
public static void main(String[] args) throws IOException {
    try (Stream<String> lineStream = args.length > 0
            ? Files.lines(Paths.get(args[0]), StandardCharsets.UTF_8)
            : new BufferedReader(new InputStreamReader(System.in)).lines()) {

        lineStream.map(line -> processLineWithOperator(line, (a, b) -> a * b))
            .forEach(outLine -> System.out.println(outLine));

    }
}

private static String processLineWithOperator(String line, IntBinaryOperator operator) {
    String[] halves = line.split("\\|");
    int[] halfA = toInts(halves[0]);
    int[] halfB = toInts(halves[1]);

    return IntStream.range(0, halfA.length)
            .map(index -> operator.applyAsInt(halfA[index], halfB[index]))
            .mapToObj(String::valueOf)
            .collect(Collectors.joining(" "));

}

private static int[] toInts(String spaceDelimitedInts) {
    return Arrays.stream(spaceDelimitedInts.split("\\s+"))
              .mapToInt(Integer::parseInt)
              .toArray();
}


Improving the efficiency of my FizzBuzz algorithm I worked on an exercise from codewars, which is a variation of the popular fizzbuzz game.  (If the link doesn't work, you may need to reload the page.)

In this variation, I'm supposed to find the sum of all the multiples of 3 and 5 that are less than the passed in number.

My code works, but it's horribly inefficient. It only works for smaller numbers and can't work for really large numbers

def fizz_buzz_sum(number)
  (1..number).to_a.select{|x| x % 3 == 0 && x % 5 == 0}.reduce(:+)
end

I think my problem is that I'm iterating over all the numbers in a large array and I might not have to.

Can anyone help reduce the complexity of my algorithm?

Your problem is not really a fizz-buzz problem at all, it is a sum-of-sequence problems, and is commonly based/equivalent to the first "Project Euler" challenge

Note: your code has a bug, you are supposed to sum values that are a multiple of either 3, or 5, but you are summing only the multiples of both 3 and 5 (i.e. 15) that are less than the number...

There is an algorithmic/mathematical solution to this which completely removes the need for the iteration at all.

Consider the sum of all the multiples of 3 less than n....:

3 + 6 + 9 + .... + t

where t < n.

The sum of that sequence is called the sum of the Arithmetic Progression.

if you change the sequence to be:

(1 x 3) + (2 x 3) + (3 x 3) + .... + (m * 3)

then you can factor out the 3's, and have:

3 * (1 + 2 + 3 + .... + m)

Now, the '1 + 2 + 3 + .... + m` sequence is what's called a triangular number:

*
* *
* * *
....

* * * .... *

which you can visualize as being half the area of a rectangle, where one side of the rectangle is m, and the other side is m + 1

What is the size of m, well, it is the number of 3 multiples there are:

m = (n - 1) / 3

And, your arithmetic progression sum becomes:

count3s = (n - 1) / 3
sum3s = 3 * count3s * (count3s + 1) / 2

Similarly, the sum of 5's less than n are:

count5s = (n - 1) / 5
sum5s = 5 * count5s * (count5s + 1) / 2

So, the sum of all 3 and 5 multiples less than n is easy, but there's a probem.... if we add the two sums together, it will count the multiples of 15 twice.....

so, the sum of one 15's series is:

count15s = (n - 1) / 15
sum15s = 15 * count15s * (count15s + 1) / 2

if we subtract that, you get the actual sum....

In ruby terms, this can all be reduced to a simple function, and then simple arithmetic... (see ideone ):

def sum_series(base, limit)
    count = (limit - 1) / base
    sum = base * (count * (count + 1)) / 2
end

def fizz_buzz_sum(number)
  sum_series(3, number) + sum_series(5, number) - sum_series(15, number)
end


Approximating π via Monte Carlo simulation Inspired by a tweet linked to me by a friend and a Haskell implementation by her for the same problem, I decided to try my hand at approximating the value of π using everything in the Haskell standard library I could find for the job. Here’s what I came up with:

module Pi where

import Data.List (genericLength)
import Control.Arrow (Arrow, (<<<), (***), arr)
import System.Random (newStdGen, randoms)

type Point a = (a, a)

chunk2 :: [a] -> [(a, a)]
chunk2 []      = []
chunk2 [_]     = error "list of uneven length"
chunk2 (x:y:r) = (x, y) : chunk2 r

both :: Arrow arr => arr a b -> arr (a, a) (b, b)
both f = f *** f

unsplit :: Arrow arr => (a -> b -> c) -> arr (a, b) c
unsplit = arr . uncurry

randomFloats :: IO [Float]
randomFloats = randoms <$> newStdGen

randomPoints :: IO [Point Float]
randomPoints = chunk2 <$> randomFloats

isInUnitCircle :: (Floating a, Ord a) => Point a -> Bool
isInUnitCircle (x, y) = x' + y' < 0.25
  where x' = (x - 0.5) ** 2
        y' = (y - 0.5) ** 2

lengthRatio :: (Fractional c) => [b] -> [b] -> c
lengthRatio = curry (unsplit (/) <<< both genericLength)

approximatePi :: [Point Float] -> Float
approximatePi points = circleRatio * 4.0
  where circlePoints = filter isInUnitCircle points
        circleRatio = circlePoints `lengthRatio` points

main :: IO ()
main = do
  putStrLn "How many points do you want to generate to approximate π?"
  numPoints <- read <$> getLine
  points <- take numPoints <$> randomPoints
  print $ approximatePi points

I’m interested in a general review, but I’m especially curious about my use of arrows: is there a better way to write lengthRatio? Are anything like both and unsplit provided anywhere in the standard library? If not, do any packages help?

About those arrows

  I’m interested in a general review, but I’m especially curious about my use of arrows: is there a better way to write lengthRatio?

Compare the following two lines. Both do the same, but which one would you rather see if you need to change your code drunk in three months, with only 5% battery left?

lengthRatio = curry (unsplit (/) <<< both genericLength)

lengthRatio xs ys = genericLength xs / genericLength ys

Also, which one has which type, and which one is more general?

Arrows are great if you want to abstract functions. But throughout your small script, you're still just working with (->), not any other instance of Arrow. For a small script like this, Arrow is too much. For example the pointwise definition above is actually a character shorter than the pointfree one. Sure, the pointfree one is clever, but it's also very beginner-unfriendly.

About randomness

randomPoints introduces a dependency between your point coordinates \$x\$ and \$y\$, since both draw from the same sequence. This usually leads to points on hyperplanes (see disadvantages of LCG and spectral test). Your friends variant doesn't have this immediate problem:

randomTuples :: Int -> IO [(Float, Float)]
randomTuples n = do
  seed1 <- newStdGen
  seed2 <- newStdGen
  let xs = randoms seed1 :: [Float] -- two different
      ys = randoms seed2 :: [Float] -- generators being used
  return $ take n $ zipWith (,) xs ys

However, since newStdGen is merely a split, it's more or less hiding the dependency at another place. Still, it's something to keep in mind, if you don't want to end up with something like this.

But how would you check this? Well, you would run tests, over and over. Here's the second design critique on randomPoints, it doesn't take a RandomGen. Truth be told, if I say that Arrow is too much for a small script, then

randomPoints :: RandomGen g => g -> [Point Float]

is too much either.

Also, if you know you're going to generate Points, a newtype Point a together with

instance Random a => Random (Point a) where

is feasible and doesn't introduce a potential error via chunk2. Keep possible problems with Random in mind, though.

About names

The function isInUnitCircle lies. It's not testing whether the point \$(x,y)\$ lies in the circle with radius \$r = 1\$ with center in the origin, e.g.
$$
\sqrt{x^2 + y^2} \le 1^2 \Leftrightarrow x^2 + y^2 \le 1
$$
but in the circle with diameter \$d = 2r = 1\$ with center in \$(0.5, 0.5)\$. In the following picture, the green region is where you generate your random values. In the left one, you see the regular unit circle, in the right one, you see the circle size you're actually testing (after shifting your values from the green square into the red one):

Therefore, you're not calculating the "usual" fourth of a circle, but instead a circle with a fourth of the original size (\$\pi(\frac{1}{2})^2 = \frac{\pi}{4}\$)). Luckily, it doesn't matter for the convergence.

A real test that checks whether a point is in the unit circle is tremendously easier:

isInUnitCircle :: (Num a, Ord a) => Point a -> Bool
isInUnitCircle (x, y) = x ^ 2 + y ^ 2 <= 1

About optimization

Last, but not least, there's an issue with approximatePi, or rather the use of lengthRatio on the same list twice. Actually, taking the length of the list again is a litle bit strange, since you know how large the sample is:

numPoints <- read <$> getLine                -- sample size
points    <- take numPoints <$> randomPoints
print $ approximatePi points                 -- sample size still known (?)

But let's say that you don't actually know how many points you have. Let's assume that someone wants to check a many points. Suddenly, the memory usage of your program explodes:

$ echo 10000000 | ./CalcPi +RTS -s
How many points do you want to generate to approximate π?
3.141744
  33,724,505,920 bytes allocated in the heap
   5,288,250,096 bytes copied during GC
   1,319,621,976 bytes maximum residency (17 sample(s))
       5,554,344 bytes maximum slop
            2587 MB total memory in use (0 MB lost due to fragmentation)

                                     Tot time (elapsed)  Avg pause  Max pause
  Gen  0     63626 colls,     0 par    1.606s   3.155s     0.0000s    0.0006s
  Gen  1        17 colls,     0 par    2.732s   3.373s     0.1984s    1.2720s

  INIT    time    0.000s  (  0.000s elapsed)
  MUT     time   17.158s  ( 15.542s elapsed)
  GC      time    4.337s  (  6.528s elapsed)
  EXIT    time    0.019s  (  0.166s elapsed)
  Total   time   21.514s  ( 22.236s elapsed)

  %GC     time      20.2%  (29.4% elapsed)

  Alloc rate    1,965,530,983 bytes per MUT second

  Productivity  79.8% of total user, 77.2% of total elapsed

Even though randoms generates a lazy list, approximatePi needs to hold onto it completely due to lengthRatio. A classic space leak. The altnerative version of lengthRatio won't save you from that. Instead, provide a function to check the ratio of filtered elements:

-- Rational from Data.Ratio
filterRatio :: (a -> Bool) -> [a] -> Rational
filterRatio p xs = -- exercise

That way, you can define a version of approximatePi that works for large lists:

approximatePi :: [Points Float] -> Double
approximatePi points = circleRatio * 4
  where
    circleRatio = fromRational $ filterRatio isInUnitCircle points

$ echo 10000000 | ./GenPIRatio +RTS -s
How many points do you want to generate to approximate π?
3.1421592
  24,445,866,792 bytes allocated in the heap
      15,555,552 bytes copied during GC
          77,896 bytes maximum residency (2 sample(s))
          21,224 bytes maximum slop
               1 MB total memory in use (0 MB lost due to fragmentation)

                                     Tot time (elapsed)  Avg pause  Max pause
  Gen  0     46874 colls,     0 par    0.012s   0.113s     0.0000s    0.0001s
  Gen  1         2 colls,     0 par    0.000s   0.000s     0.0001s    0.0001s

  INIT    time    0.000s  (  0.000s elapsed)
  MUT     time   10.809s  ( 10.746s elapsed)
  GC      time    0.012s  (  0.113s elapsed)
  EXIT    time    0.000s  (  0.000s elapsed)
  Total   time   10.821s  ( 10.859s elapsed)

  %GC     time       0.1%  (1.0% elapsed)

  Alloc rate    2,261,657,279 bytes per MUT second

  Productivity  99.9% of total user, 99.5% of total elapsed

Summary

Food for thought:

Use the right level of abstraction for your problem. Arrow is an overkill for such a small script, but alright for learning.
Try to decrease the amount of IO wherever possible, but again, that might be too abstract for a small script.

Bad:

Don't lie, give things the right name.
Don't overcomplicate, keep pointfree to a sane minimum.
Major space leak in approximatePi. Read the linked section of RWH and try to define filterRatio or a similar function.

Good:

Type signatures! Yay!
Explicit imports!
Type synonym instead of (a, a) everywhere!

So beside the slight arrow-overkill, well done.


DOS batch script to open file explorer I wrote a script that you can use to open the file explorer from command line like

e db --> opens my Dropbox folder (e.cmd is the name of the script)
e bin --> opens my script folder

(I’m not very happy with the way I define the relationship mnemonic <--> folder.)
Remarks, anyone?

@ECHO OFF

SET "target=%~1"

if "%target%" EQU "" (
    ECHO No target specified.
    EXIT /B
)

SET folder=""
CALL :set_folder_%target% 2> nul
if %folder% EQU "" (
    ECHO Invalid target %target%.
    EXIT /B
)

explorer %folder%
EXIT /B

:set_folder_db
SET folder="D:\Alex\Dropbox" & EXIT /B

:set_folder_bin
SET folder="D:\Alex\Scripts\Bin" & EXIT /B

...

It's not terrible as it is, but you're right to wonder if there's a better way.  As you noticed already, it would be nice to keep the associations between the abbreviation and the corresponding directory in one place.

Fortunately, there's a very slick way to to that.  First, create a text file file that contains just the file associations:

e.txt

db      D:\Alex\Dropbox
bin     D:\Alex\Scripts\Bin

e.cmd

@echo off
FOR /F "tokens=1,2" %%i in (e.txt) do if "%~1" EQU "%%i" ( explorer "%%j" & EXIT /B )
ECHO Invalid target %target%.

This uses a relatively obscure feature of the FOR command that allows us to parse a file.  You can read more about that by executing HELP FOR from the command line.

Naturally, in the real version, you'd want to specify the full path to the e.txt file.

Allowing spaces in directory names

If you might have spaces in the directory names, you can do so with little additional effort.  For example, we can use = for the delimiter in the text file so that each line now looks like this:

sp=D:\Alex\Scripts\Advanced Scripting

Now just modify the script so that we specify the delimiter.  To do that change the part of the line that says "tokens=1,2" to read "tokens=1,2 delims==" and that's it.


Java array extension/reduction I work with the Oracle JGeometry method which returns (or takes as method argument) 2D segments as double array (x1,y1,x2,y2 ... xn,yn). I need to extend this array to contain 3D segments as double array (x1,y1,z1,x2,y2,z2 ... xn,yn,zn) by default Z value or reduce to 2D (by removing of all Z coordinate). I wrote simple utility methods for making this. Is there any easier or smarter way to do this?
The conversion from 2D to 3D:
public static double[] to3D(double z, double[] inputArray) {
    List<Double> convertedItems = new ArrayList<>();
    for (int i = 0; i < inputArray.length; i++) {
        convertedItems.add(inputArray[i]);
        if ((i + 1) % 2 == 0) {
            convertedItems.add(z);
        }
    }
    return convertedItems.stream().mapToDouble(Double::doubleValue).toArray();
}

The conversion from 3D to 2D:
public static double[] to2D(double[] inputArray) {
    List<Double> convertedItems = new ArrayList<>();
    for (int i = 0; i < inputArray.length; i++) {
       if ((i + 1) % 3 == 0) {
           continue;
       }
       convertedItems.add(inputArray[i]);
    }
    return convertedItems.stream().mapToDouble(Double::doubleValue).toArray();
}

I would go with primitive Arrays, instead of using Collections and Streams in this case.
public static double[] to3D(double z, double[] in) {
    // Pre-allocated array 1.5 size of the original array
    double[] out = new double[in.length * 3 / 2];
    //loop with a step 2
    for (int i = 0, j = 0; i < in.length; i += 2) {
        out[j++] = in[i];
        out[j++] = in[i + 1];
        out[j++] = z;
    }
    return out;
}

public static double[] to2D(double[] in) {
    // Pre-allocated array 2/3 size of the original array
    double[] out = new double[in.length * 2 / 3];
    //loop with a step 3
    for (int i = 0, j = 0; i < in.length; i += 3) {
        out[j++] = in[i];
        out[j++] = in[i + 1];
    }
    return out;
}

Reasons for using arrays instead of lists: Input and output both are arrays, the target size is known, we only need basic operations and they would be faster in this case.
I hope it helps.


A Simple Interactive Program that Can Solve A Linear System of Equations So I was just reading my algebra-precalculus textbook, and learned that matrices can be used to solve systems of equations. Since the whole process seemed so algorithmic, I wondered if I could implement it as a program. The result is the following:

/*
 *
 * Solve a system of equations in two variables, x and y
 *
 */

#include <iostream>

struct LinearEquation
{
    double xCoefficient;
    double yCoefficient;
    double constantTerm;
};

struct Solution
{
    double x;
    double y;
};

LinearEquation readEquation();
double findDeterminant(const LinearEquation& a, const LinearEquation& b);
Solution solveEquation(const LinearEquation& a, const LinearEquation& b, double determinant);

int main()
{
    std::cout << "Welcome to the linear equation solver! Enter the following inputs for two equations of the form ax + by = c.\n\n";

    LinearEquation one, two;

    std::cout << "Equation #1\n";
    one = readEquation();

    std::cout << "Equation #2\n";
    two = readEquation();

    double det = findDeterminant(one, two);
    if (det != 0)
    {
        Solution solution = solveEquation(one, two, det);
        std::cout << "The solution:\n";
        std::cout << "x = " << solution.x << "\n";
        std::cout << "y = " << solution.y << "\n";
    }
    else
    {
        std::cout << "This linear equation has no unique solutions!\n";
    }
}

LinearEquation readEquation()
{
    LinearEquation result;
    std::cout << "X coefficient: ";
    std::cin >> result.xCoefficient;
    std::cout << "Y coefficient: ";
    std::cin >> result.yCoefficient;
    std::cout << "Constant term: ";
    std::cin >> result.constantTerm;

    return result;
}

double findDeterminant(const LinearEquation& a, const LinearEquation& b)
{
    return a.xCoefficient * b.yCoefficient - a.yCoefficient * b.xCoefficient;
}

Solution solveEquation(const LinearEquation& a, const LinearEquation& b, double determinant)
{
    // Calculating the inverse
    double matrixTL = b.yCoefficient / determinant;
    double matrixTR = -1 * (a.yCoefficient / determinant);
    double matrixBL = -1 * (b.xCoefficient / determinant);
    double matrixBR = a.xCoefficient / determinant;

    Solution result;
    // Matrix multiplication
    result.x = matrixTL * a.constantTerm + matrixTR * b.constantTerm;
    result.y = matrixBL * a.constantTerm + matrixBR * b.constantTerm;

    return result;
}

This program simply goes through each coefficient and constant term and basically applies a "formula" to solve the equation. It works as far, but it could have minor bugs. I am looking for ways this program can be improved in terms of program size or efficiency. Another problem I want to solve is in terms to making this program handle more equations/variables, which would currently require manually adding parts of the matrix.

Why not do the arithmetic in parentheses? That way, it will be clearer.
The rest of the code is very clear and understandable, I didn't find any bugs.


Implementation of MATLAB's `im2col` The function im2col in MATLAB is very useful to vectorize Patch based Image Processing algorithms. The problem is the function isn't optimized and doesn't use C Code. I'm trying to build efficient C code for that.

This is the code I created:

function [ mColumnImage ] = ImageToColumns( mInputImage, blockRadius )
% ----------------------------------------------------------------------------------------------- %
% [ mColumnImage ] = ImageToColumns( mInputImage, blockRadius )
%   Creates an column image from the sliding neighborhood in mInpuImage
% Input:
%   - mInputImage           -   Input image.
%                               Matrix, 1 Channels, Floating Point
%   - blockRadius        -      Local Window Radius.
%                               Scalar, Floating Point, {1, 2, ..., inf}.
% Output:
%   - mColumnImage          -   Input image.
%                               Matrix, 1 Channels, Floating Point, [0, 1]
% Remarks:
%   1.  Prefixes:
%       -   'm' - Matrix.
%       -   'v' - Vector.
%   2.  C
% TODO:
%   1.  I
%   Release Notes:
%   -   1.0.000     22/08/2014  R
%       *   First release version.
% ----------------------------------------------------------------------------------------------- %

numRows = size(mInputImage, 1);
numCols = size(mInputImage, 2);

blockSize = (2 * blockRadius) + 1;

numPixelsToProcess = (numRows - blockSize + 1) * (numCols - blockSize + 1);

mColumnImage = zeros((blockSize * blockSize), numPixelsToProcess);

colImageColIdx = 0;

for iColIdx = (blockRadius + 1):(numCols - blockRadius)
    for jRowIdx = (blockRadius + 1):(numRows - blockRadius)
        colImageColIdx = colImageColIdx + 1;
        colImageRowIdx = 0;
        for kPixelColIdx = -blockRadius:blockRadius
            for lPixelRowIdx = -blockRadius:blockRadius
                colImageRowIdx = colImageRowIdx + 1;
                mColumnImage(colImageRowIdx, colImageColIdx) = mInputImage((jRowIdx + lPixelRowIdx), (iColIdx + kPixelColIdx));
            end
        end
    end
end

end

It is MATLAB code written in C style. Imagine it was C code: what would you do make it faster? What would you do to allow compiler optimizations (loop unrolling, vectorization, parallelization, etc...)?

I posted vanilla C code I created and have a few questions about it:

How can I make the memory access more efficient (hence make the code faster)?
Any place to utilize vectorization?
How can I assist the compiler and tell it no aliasing will happen?
What would be the right way to utilize OpenMP?

This is a valid C code which implements the above:

void ImageToColumns(float* mO, float* mI, int numRows, int numCols, int blockRadius)
{
    int blockSize, blockNumElements, colImageColIdx, colImageRowIdx;
    int ii, jj, kk, ll;

    blockSize           = (2 * blockRadius) + 1;
    blockNumElements    = blockSize * blockSize;
    colImageRowIdx      = -1;

    for (ii = blockRadius; ii < (numRows - blockRadius); ii++)
    {
        for (jj = blockRadius; jj < (numCols - blockRadius); jj++)
        {
            colImageRowIdx = colImageRowIdx + 1;
            colImageColIdx = -1;

            for (kk = -blockRadius; kk <= blockRadius; kk++)
            {
                for (ll = -blockRadius; ll <= blockRadius; ll++)
                {
                    colImageColIdx = colImageColIdx + 1;
                    mO[(colImageRowIdx * blockNumElements) + colImageColIdx] = mI[((ii + kk) * numCols) + jj + ll];
                }

            }
        }
    }

}

Clearly this a memory bounded algorithm.


Calculating percentage of 3 test forms Please help me to simplify the script below. It has a lot of variables and a lot of values are getting assigned to them. Can I make it simpler?

It has to calculate the percentage of 3 test forms: pre, post and final.

DECLARE @tablefinal TABLE
(
    RowID1 INT IDENTITY(1,1),
    _CourseIDD int null,
    _courseName varchar(1000) null,
    _PRE INT NULL,
    _POST int null,
    _Feedback int null,
    _final int NULL,
    _Per decimal(18,2) null
)

DECLARE @rowCount INT,
        @currentRow INT,
        @@@CourseID INT,
        @@@CourseName varchar (1000)  --,  @STUDENT_ID int = 1078

DECLARE @tableCourse TABLE
(
    RowID INT IDENTITY(1,1),
    _COURSE_ID INT NULL,
    _COURSE_NAME varchar (1000) NULL
)

INSERT INTO @tableCourse (_COURSE_ID , _COURSE_NAME)
SELECT  COURSE_ID,
        COURSE_NAME
FROM DLC_COURSE C
WHERE COURSE_ID IN
(
    SELECT CS.COURSESHD_COURSE_ID
    FROM DLC_COURSE_SCHEDULE CS
    INNER JOIN DLC_STUDENT_PAYMENT SP ON
        CS.COURSESHD_ID = SP.STUDENT_PAYMENT_COURSESHD_ID
    WHERE SP.STUDENT_PAYMENT_STUDENT_ID = @STUDENT_ID
    AND CS.COURSESHD_STATUS=1
)
AND C.COURSE_STATUS=1

SELECT  @rowCount = @@RowCount,
        @currentRow = 1

WHILE @currentRow<=@rowCount
BEGIN
    SELECT  @@@CourseID = _COURSE_ID,
            @@@CourseName = _COURSE_NAME
    FROM @tableCourse
    WHERE RowID = @currentRow

    --do activity
    --select @@@CourseID as CID, @@@CourseName as CN
    SET @currentRow = @currentRow + 1

    DECLARE @@Pre INT = 0,
            @@Course INT = 0,
            @@Post INT = 0,
            @@Feedback INT = 0,
            @@Final INT = 0

    SELECT @@Pre = Count(*)
    FROM DLC_ASSESSMENT_RESULT
    WHERE RESULT_STUDENT_ID = @STUDENT_ID
    AND RESULT_COURSE_ID = @@@CourseID
    AND RESULT_STATUS='Pass'
    AND RESULT_ASSESSMENT_TYPE=2

    SELECT @@Course = ISNULL(COUNT(C.COURSE_ID),0)
    FROM DLC_COURSE C
    WHERE COURSE_ID IN
    (
        SELECT CS.COURSESHD_COURSE_ID
        FROM DLC_COURSE_SCHEDULE CS
        INNER JOIN DLC_STUDENT_PAYMENT SP
            ON CS.COURSESHD_ID = SP.STUDENT_PAYMENT_COURSESHD_ID
        WHERE SP.STUDENT_PAYMENT_STUDENT_ID = @STUDENT_ID
        AND CS.COURSESHD_STATUS = 1
        AND CS.COURSESHD_ENDDATE < GETDATE()
        AND CS.COURSESHD_COURSE_ID = @@@CourseID
    )
    AND C.COURSE_STATUS=1

    SELECT @@Post = Count(*)
    FROM DLC_ASSESSMENT_RESULT
    WHERE RESULT_STUDENT_ID = @STUDENT_ID
    AND RESULT_COURSE_ID = @@@CourseID
    AND RESULT_STATUS = 'Pass'
    AND RESULT_ASSESSMENT_TYPE = 1

    SELECT @@Feedback = Count(*)
    FROM FEEDBACK_RESULTS
    WHERE QUESTION_ID IN
    (
        SELECT QUESTION_ID
        FROM FEEDBACK_QUESTIONS
        WHERE QUESTION_COURSE = @@@CourseID
        AND STATUS_ID = 1
    )
    AND EMP_ID = @STUDENT_ID

    SELECT @@Final = COUNT(*)
    FROM DLC_ASSESSMENT_RESULT
    WHERE RESULT_STUDENT_ID = @STUDENT_ID
    AND RESULT_COURSE_ID = @@@CourseID
    AND RESULT_STATUS = 'Pass'
    AND RESULT_ASSESSMENT_TYPE = 3

    --declare @@Pre int= 0, @@Course int = 0 , @@Post int  = 0, @@Feedback int = 0  , @@Final  int  = 0

    DECLARE @per decimal (18,2)

    --select isnull(@@Pre,0), isnull(@@Post,0) , isnull(@@Final,0)
    SET @per = ((ISNULL(@@Pre,0) + ISNULL(@@Post,0) + ISNULL(@@Final,0) ) / 3.00   * 100)

    INSERT INTO @tablefinal
    SELECT  ISNULL(@@Course,0) AS COURSE,
            @@@CourseName,
            ISNULL(@@Pre,0) AS PRE,
            ISNULL(@@Post,0) AS Post,
            ISNULL(@@Feedback,0) AS Feedback,
            ISNULL(@@Final,0) AS Final,
            @per
  END

SELECT *
FROM @TABLEFINAL

To sum up what this script does, just for the sake of simplicity:

Declare a result set table variable @tablefinal.
Declare variables for a counter, and for courses, and a commented out @STUDENT_ID (likely for testing purposes).
Declare another result set table variable @tableCourse to hold course IDs and course names during the transaction. This seems to be to filter out courses with no students or not on the schedule.
Set the counter start = 1 and end = total number of rows.
Begin to cycle through @tableCourse at row 1 using WHILE. I noticed that @student_id is not declared or set (other than the instance which is commented out) even though it's being used in multiple WHERE clauses. May be worth your while to look into why it is that way.
Count records for @@pre
Count records for @@courses
Count records for @@post
Count records for @@feedback
Count records for @@final
Declare @per for precentage
Calculate @per with a simple arithmetic operation
Shove everything into your results table @tablefinal and then select from it to see the result set.

To me this really seems like overkill, like a SQL programmer trying to show that he knows how to use variables. I think this could be much more simple, I won't rewrite the whole script but here are some ideas. If you decide to try that approach you will need to start from the ground up, you are welcome to post your new script in a new Code Review if you would like.

DECLARE @pre INT = (Count(*) FROM DLC_ASSESSMENT_RESULT WHERE RESULT_STATUS='Pass'  AND RESULT_ASSESSMENT_TYPE=2);
-- Repeat DECLARE (Count(*)) for @post, @feedback, @final

SELECT
    C.COURSE_ID,
    C.COURSE_NAME,
    @pre AS 'Pre',
    @post AS 'Post',
    @feedback AS 'Feedback',
    @final AS 'Final',
    ((ISNULL(@Pre,0) + ISNULL(@@post,0) + ISNULL(@Final,0) ) / 3.00   * 100) AS 'Pre'

FROM DLC_COURSE C
WHERE COURSE_ID IN
-- Taken from the script, seems to be the only important JOIN
(
    SELECT CS.COURSESHD_COURSE_ID
    FROM DLC_COURSE_SCHEDULE CS
    INNER JOIN DLC_STUDENT_PAYMENT SP ON
        CS.COURSESHD_ID = SP.STUDENT_PAYMENT_COURSESHD_ID
    WHERE CS.COURSESHD_STATUS=1
)
AND C.COURSE_STATUS=1
-- This will aggregate the result set by course
GROUP BY C.COURSE_ID;


Job pipeline application We have a console application which runs to perform an incremental load of our data warehouse staging area, with data from a number of production databases.
The source database don't necessarily have a rowversion/timestamp/etc. column so we are currently using change data tracking in SQL to load changes to the tables. This all works fine and I think most of the code is okay, however, I don't like the way the 'main' pipeline works and was wondering if anyone had any better suggestions.

At the moment I have each 'step' as an instance of a class like this. This is so I can show a status of each step and if errors happen we have a descriptive name of the action it was performing. I was also thinking of adding pre and post conditions to each step so we can more easily tell the reason it fails.

public abstract class PipeLineStep
{
    public abstract String StepName { get; }
    public bool WasSuccessful { get; set; }
    internal abstract void Run();
}

These steps are run by the following method to enable a common type of logging. If an error is detected in the run, and we are not performing a full load, then the process will run again but perform a full load.

private bool RunStep(PipeLineStep step, TableContext context, Boolean isFullLoad, Boolean errorRerun)
{
    try
    {
        step.Run();
        return true;
    }
    catch (Exception ex)
    {
        Logging.Log(false, String.Format("Error during Pipeline Step [{0}]. {1}", step.StepName, ex.Message), context.TaskId, context.TableId, step.StepName, true, CurrentRunNumber);
        Logging.Log(false, String.Format("{0}", !isFullLoad && !errorRerun ? "Attempting Full Load of Table." : "Aborting Table Load."), context.TaskId, context.TableId, step.StepName, true, CurrentRunNumber);

        // If not currently doing a full load and we have an error - trying doing full load to refresh schema
        if (!isFullLoad && !errorRerun)
        {
            Run(context, CurrentRunNumber, isFullLoad, true);
            return false;
        }

        UpdateProgress(context.TableId, "Finished with Error", true, true);
        return false;
    }
}

In the main loop this method is called for each step, with control flow logic:

var step1 = new CheckChangeTrackingEnabledOnDatabaseStep(...);
if (!RunStep(step1, context, isFullLoad, errorRerun)) return false;

I don't see the actual order or control flow changing too much because each step is fairly well defined and we would handle any additional processing after the data is loaded into the staging area. So this part does not need to be too flexible.

I'd need more context so please consider some suggestions as implicit questions.

In PipeLineStep class StepName property is a read-only property. If you just return a constant value you may want to make it a constructor parameter instead (where you may also include some sort of validation):

protected PipeLineStep(string name) {
    if (String.IsNullOrWhiteSpace(name))
        throw new ArgumentException("Step name must not be nor empty.", "name");

    StepName = name;
}

public string StepName {
    get;
    private set;
}

Property name itself is little bit redundant, you do not need to repeat Step* for a property of a class named *Step:

public string Name {
    get;
    private set;
}

In the same class WasSuccessful is a public read/write property. Does anyone (inside or outside its assembly) has right to both read and write this property? Assuming (because Run() is internal) that also WasSuccessful is set from within same assembly then I'd make its setter internal.

In the same class Run() method is internal. Because it's also abstract it seems that it's used only from the same assembly where class is declared and also that all derived classes are located in the same assembly. This arises a question: does it make sense to have a public abstract class that can be used and extended only from the same assembly? It may be - if other assemblies will access Name ans WasSuccessful properties - but if it's not then don't make this class public.

In general visibility issues won't make your code broken but they will help to make it safer (from usage point of view) and they communicate intent.

In RunStep method you're catching a generic Exception. You should formalize which exceptions each step will throw to possibly narrow down to a smaller set of exceptions, catching Exception you get everything, from OutOfMemoryException to ArgumentException and it's bad because they're exceptions that need a special handling. Also, knowing what you're catching will help you to determine corner cases and error scenarios: a very good exercise for you and for your tests.

You're mixing bool and Boolean (for example), they're equivalent then I'd suggest to use the style you prefer and be consistent, reader won't repeatedly ask himself why (variations are eye-catching, they must be justified).

Parameter isFullLoad is not meaningful without more context, I'd rename it to something else but I actually don't know what it means (and your future-you won't know either without re-reading surrounding code...)

You're using a parameter errorRerun to know if you're retrying because of a previous error. While it's not an issue in such simple scenario it may be confusing (see your handling logic) and it adds more parameters to a function that should be as simple (you already have four parameters, two/three more than what's good). I can't suggest anything without much more context and knowledge but it's a point I'd investigate further.

Expression is !isFullLoad && !errorRerun is used twice, it's a good chance to extract a local variable with a meaningful name (again, rename it according to its exact meaning):

bool canRunFullPipelineAgain = !fullLoad && !errorRerun;

In general you're using too many function arguments and too many of them are bool. Pick any line where you use them and you can't understand their meaning without jumping back and forward to called function prototype (see RunStep(), Run(), UpdateProgress() and Logging.Log().)

To reduce number of arguments you have various techniques for example (again it depends on surrounding code) errorRerun may be a class member property as context and isFullLoaded (you have classes, containers of methods and state, you don't need to pass everything as parameters). Your logging function is a good example of this: its prototype is waaaaay too complex and convoluted. If you really can't change it - at least - use named parameters.

Boolean parameters are easier to remove, you may use enum instead of them (eventually - when it makes sense - combined as Flags). For example:

var options = RunStepOptions.TableIsFullyLoaded | RunStepOptions.RetryOnError);
if (!RunStep(step1, context, options))
    return false;

Minor question: shouldn't PipeLine be Pipeline?

Free thinking: if Run() executes all steps in pipeline then why you're putting error handling in RunStep()? Isn't easier to write a method Run() without error handling and an outer method RunWithErrorHandling() which takes care of this? You may also introduce a StepExecutionException, almost pseudo-code:

private void RunWithErrorHandling() {
    try {
       Run();
    }
    catch (StepExecutionException e) {
        // Try again? I don't know your full logic here
        Run();
    }
}

private void Run() {
    foreach (var step in Steps)
        RunStep(step);
}

private void RunStep(PipelineStep step) {
    try {
    }
    catch (Exception e) { // !!!
        // Logging?
        throw new StepExecutionException("...", e);
    }
}

Without surrounding code and better understanding of its context I can't suggest another design, just take this as something to think about.


Greasemonkey script to refresh page until target string is found I've been working on this user script to hunt for a specific string in an HTML webpage with the goal of finding the time at which it appeared.  This feels needlessly complicated and I would like advice on how too condense and upgrade the code to more simply find the time.

// ==UserScript==
// @name           Magma Time Finder
// @namespace      Will.GG
// @description    Refreshes the page until target string is found
// @include        *www.website.com
// @include http://www.website.com
// @grant metadata
// ==/UserScript==
var strHTML = document.body.innerHTML;
var yourTime = new Date();
var count = 1;
if(strHTML.indexOf("Text to find here") < 0){
  alert(yourTime);

} else {

  var lastCheck = document.createElement("div");
  lastCheck.innerHTML = "Last Check: " + yourTime.toString();
  console.log('Iteration: ' + count);
  count++;
  var parent = document.getElementsByClassName("content")[0];
  parent.insertBefore(lastCheck, parent.children[1]);

  window.setTimeout(function(){window.location.reload() ;},300000) ;

}
return;

Right now, your code grabs everything HTML related in the document. However, from what I can tell, you only want to search the text on a webpage.

That being said, this code currently could potentially pick up a string that is embedded in the HTML code, but is not actually visible to the user on the screen.

To only grab the text, you can use either the innerText property of the textContent of the body:

var text = document.body.innerText || document.body.textContent;

Now, the text variable will contain all the text in the document, excluding the actual HTML code.

Now, I may be mistaken, but after some testing, it seems that count does not exist after the page resets. Therefore, you can just remove this variable and all interactions with it.

window.setTimeout(function(){window.location.reload() ;},300000) ;

Where did that 300000 come from? You should declare a constant at the top of your code.

return;

This is unnecessary and is confusing. This should be removed.

The console.logs seem to me that they were used for debugging, since they don't seem to be showing any important message to the user, and why would the user be looking in the console anyway?. These can be removed.

Now, this is what your code looks like:

// ==UserScript==
// @name           Magma Time Finder
// @namespace      Will.GG
// @description    Refreshes the page until target string is found
// @include        *www.website.com
// @include http://www.website.com
// @grant metadata
// ==/UserScript==

var FREQUENCY = 300000;

var text = document.body.innerHTMl || document.body.textContent;
var yourTime = new Date();

if (text.indexOf("Text to find here") < 0) {
    alert(yourTime);
} else {
    var lastCheck = document.createElement("div");
    lastCheck.innerHTML = "Last Check: " + yourTime.toString();
    var parent = document.getElementsByClassName("content")[0];
    parent.insertBefore(lastCheck, parent.children[1]);

    window.setTimeout(window.location.reload, FREQUENCY);
}


Extracting and mangling a parameter from a MySQL configuration file Is there a better way of doing this?

File.open( "/etc/my.cnf" ) do |f|
  f.grep( /^testdir/ ) do |line|
    test1 = line.chop.gsub(/ /,"")
    test2 = test1.sub(/.*=/, "")
    RETURN_PATH = "#{test2}/test_ups"
  end
end

Aside from not using a constant for RETURN_PATH (which is obviously wrong and will cause Ruby to warn you), it seems reasonable enough to me.


KeyValuePair implementation In order to implement my own Dictionary<TKey, TValue> in VB6, I've implemented a KeyValuePair class, which can accomodate any type as its key, or value.
This KeyValuePair class implements the IComparable and IEquatable interfaces which I'm also using in this custom List<T> implementation

KeyValuePair.cls
Private Type tKeyValuePair
    Key As Variant
    value As Variant
End Type

Private this As tKeyValuePair
Implements IComparable
Implements IEquatable
Option Explicit

Private Function IsComparable() As Boolean
    IsComparable = Not IsObject(this.Key) Or TypeOf this.Key Is IComparable
End Function

Public Property Get Key() As Variant
    If IsObject(this.Key) Then
        Set Key = this.Key
    Else
        Key = this.Key
    End If
End Property

Public Property Let Key(k As Variant)
    If IsEmpty(k) Then Err.Raise 5
    this.Key = k
End Property

Public Property Set Key(k As Variant)
    If IsEmpty(k) Then Err.Raise 5
    Set this.Key = k
End Property

Public Property Get value() As Variant
    If IsObject(this.value) Then
        Set value = this.value
    Else
        value = this.value
    End If
End Property

Public Property Let value(v As Variant)
    this.value = v
End Property

Public Property Set value(v As Variant)
    Set this.value = v
End Property

Public Function ToString() As String
    ToString = TypeName(Me) & "<" & TypeName(this.Key) & ", " & TypeName(this.value) & ">"
End Function

Private Function IComparable_CompareTo(other As Variant) As Integer

    Dim kvp As KeyValuePair
    Set kvp = other

    IComparable_CompareTo = ToString = kvp.ToString
    If Not IComparable_CompareTo Then Exit Function

    If IsObject(kvp.Key) Then
        IComparable_CompareTo = CompareReferenceTypes(this.Key, kvp.Key)
    Else
        IComparable_CompareTo = CompareValueTypes(this.Key, kvp.Key)
    End If

End Function

Private Function IEquatable_Equals(other As Variant) As Boolean

    Dim kvp As KeyValuePair
    Set kvp = other

    IEquatable_Equals = ToString = kvp.ToString
    If Not IEquatable_Equals Then Exit Function

    If IsObject(kvp.Key) Then
        IEquatable_Equals = EquateReferenceTypes(this.Key, kvp.Key)
    Else
        IEquatable_Equals = EquateValueTypes(this.Key, kvp.Key)
    End If

End Function

Private Function EquateReferenceTypes(value As Variant, other As Variant) As Boolean

    Dim equatable As IEquatable
    If IsObject(this.Key) And TypeOf this.Key Is IEquatable Then

        Set equatable = value
        EquateReferenceTypes = equatable.Equals(other)

    Else

        Debug.Print "WARNING: Reference type doesn't implement IEquatable, using reference equality."
        EquateReferenceTypes = (ObjPtr(value) = ObjPtr(other))

    End If

End Function

Private Function EquateValueTypes(value As Variant, other As Variant) As Boolean

    EquateValueTypes = (value = other)

End Function

Private Function CompareReferenceTypes(value As Variant, other As Variant) As Integer

    Dim comparable As IComparable

    If IsComparable Then

        Set comparable = value
        CompareReferenceTypes = comparable.CompareTo(other)

    Else

        Err.Raise 9 'object required
        Exit Function

    End If

End Function

Private Function CompareValueTypes(value As Variant, other As Variant) As Integer

    If value < other Then

        CompareValueTypes = -1

    ElseIf value > other Then

        CompareValueTypes = 1

    End If

End Function

In C#, a KeyValuePair is a struct, a value type - this implementation being a class, makes it analoguous to a reference type, are there any major drawbacks to this? If so, are there any viable alternatives?

That has been said before in previous reviews, there's not really a gain in having a Private Type that lets you define a Private this As tKeyValuePair, which forces you to use this. to access what would otherwise be private fields.

Actually there is one gain: doing that allows having a Key public member that encapsulates a Key private member, all without confusing poor old VB6 compiler.

Other than that, one has to see the List and possibly also the Dictionary implementation to see the benefits of the additional complexity introduced by IEquatable and IComparable interface implementations.

The code is fairly clean, straightforward and readable, breaking IEquatable and IComparable implementations into sub-functions respectively for reference and value types might be a little overboard, but it does make a nice abstraction level when one reads the implementation code.

One nitpick, the interface implementations and the corresponding sub-functions could declare a local result variable and assign the function's return value in only a single location:

Dim result As Boolean

If IsObject(kvp.Key) Then
    result = CompareReferenceTypes(this.Key, kvp.Key)
Else
    result = CompareValueTypes(this.Key, kvp.Key)
End If

IComparable_CompareTo = result

Dim result As Boolean

If IsObject(kvp.Key) Then
    result = EquateReferenceTypes(this.Key, kvp.Key)
Else
    result = EquateValueTypes(this.Key, kvp.Key)
End If

IEquatable_Equals = result

Dim result As Boolean
Dim equatable As IEquatable

If IsObject(this.Key) And TypeOf this.Key Is IEquatable Then
    Set equatable = value
    result = equatable.Equals(other)
Else
    Debug.Print "WARNING: Reference type doesn't implement IEquatable, using reference equality."
    result = (ObjPtr(value) = ObjPtr(other))
End If

EquateReferenceTypes = result

Looking more closely at the shape of the code, vertical whitespaces aren't all that consistent: there's more vertical whitespaces near the bottom of the code module, while the code near the top doesn't seem to follow the same spacing style. Minor nitpick, but for a small specialized class like this, it's an easy fix.


Generic Makefile handling unit testing and folder structure I'm going back to C++ and, even though I'm not a beginner in programming and OOP, I'm quite new in the C++ world and I've not mastered the art of Makefiles.

I'd like to know what you think about my third Makefile and the folder structure that I use. Don't hesitate to make any comments you think necessary.

Here is the folder structure of the "project":

out/   ---   *.exe and *.zip
tmp/   ---   *.o
lib/
|   catch/   ---   Unit testing framework
|   |   inc/
|   |   |   catch.hpp
inc/   ---   Headers
|   Factorial.hpp
src/   ---   Source files
|   Factorial.cpp
|   main.cpp
tst/   ---   Tests specific source files
|   FactorialTest.cpp
|   MathTest.cpp
|   mainTest.cpp
Makefile

It is designed to support unit testing (here using Catch) with the tst folder.

I didn't know how to use external libraries with C++, so I created a lib folder to put Catch in. I thought libraries could be downloaded from the configure.sh script in the future. Does it sound like a "professional" solution for larger projects?

The Makefile :

CXX           := g++
LD            := g++

EXEC          := factorial
FLAGS         := -Wall
CXXFLAGS      := $(FLAGS)
LDFLAGS       := $(FLAGS)
INC           := -I inc
SRC           := $(wildcard src/*.cpp)
OBJ           := $(SRC:src/%.cpp=tmp/%.o)

EXEC_TEST     := test
FLAGS_TEST    := $(FLAGS)
CXXFLAGS_TEST := $(CXXFLAGS)
LDFLAGS_TEST  := $(LDFLAGS)
INC_TEST      := $(INC) -I lib/catch/inc
SRC_TEST      := $(wildcard tst/*.cpp)
OBJ_TEST      := $(filter-out tmp/main.o, $(OBJ)) $(SRC_TEST:tst/%.cpp=tmp/%.o)

.SUFFIXES:

# --------------------------------------------------------------

.PHONY: all
all: out/$(EXEC)

out/$(EXEC): $(OBJ)
    @$(LD) $(LDFLAGS) $^ -o $@ && echo "[OK]  $@"

# --------------------------------------------------------------

.PHONY: test
test: out/$(EXEC_TEST)

out/$(EXEC_TEST): $(OBJ_TEST)
    @$(LD) $(LDFLAGS_TEST) $^ -o $@ && echo "[OK]  $@"

# --------------------------------------------------------------

tmp/%.o: src/%.cpp
    @$(CXX) $(CXXFLAGS) -c $< $(INC) -o $@ && echo "[OK]  $@"

tmp/%.o: tst/%.cpp
    @$(CXX) $(CXXFLAGS_TEST) -c $< $(INC_TEST) -o $@ && echo "[OK]  $@"

# --------------------------------------------------------------

.PHONY: clean, clear
clean clear:
    @rm -f out/* && echo "[CL]  out/"
    @rm -f tmp/* && echo "[CL]  tmp/"

.PHONY: archive, zip
archive zip:
    @zip -x out/* tmp/* lib/* -q -r out/$(EXEC)-$(shell date '+%F').zip . && echo "[OK]  out/$(EXEC)-$(shell date '+%F').zip"

The Makefile is to be used via all and test targets, which build the final executable and the unit testing executable respectively.

I tried to avoid duplicating tmp/%.o: src/%.cpp and tmp/%.o: tst/%.cpp recipes with test target-specific variables buy it didn't work because there are variables in targets' and dependencies' names.

PS: It's also my third post on the Stack Exchange Network, so please feel free to make remarks about anything that is not "StackExchange-compliant".

I'll assume throughout this answer that you're targeting GNU Make.  It's available on all targets that matter, and it simplifies your world no end if you can avoid pandering to the idiosyncrasies of the manifold implementations of Make supplied with operating systems.
Object files depend on headers
This is the biggest thing that I think could be improved; I think it's probably a whole question in itself.  Your object files naturally depend on the corresponding C++ source files, but they also need rebuilding when their headers change.  Consider including generated makefiles that tell make how the object files depend on the headers; for a suitable recipe, see the Stack Overflow question "Makefile, header dependencies", and for a longer exposition, read "Auto-Dependency Generation" by Paul D. Smith.  I normally use something like
-include $(OBJS:.o=.d)

%.d: %.c
    $(CC) $(CFLAGS) -MM -MT $@ -MF $@ $<

More warnings
It's good to see that you're using -Wall.  I also recommend
CXXFLAGS += -Wextra -Wwrite-strings -Wno-parentheses
CXXFLAGS += -Wpedantic -Warray-bounds -Weffc++

The first line above always helps; the second does sometimes improve my code (you'll want to make use of -isystem to quieten your library includes, if you want to  make the latter bearable).
Output directory
Rules of makefiles advises that

Life is simplest if the targets are built in the current working directory.
Use VPATH to locate the sources from the objects directory, not to locate the objects from the sources directory.

That's certainly something to consider, though it might already be too painful to change.  I certainly prefer out-of-tree builds, so I can clean up with rm -rf build/ and be left with just my sources.
Hiding commands
I appreciate that you don't want a wall of text, but when you're debugging (or on an automated build host where you want a complete record of the build), then you might want to override this, and actually see which commands were run.  You can make the behaviour adjustable, like this:
QUIET := @
ECHO := @echo
ifneq ($(QUIET),@)
ECHO := @true
endif

Use it for your commands:
clean clear:
    $(ECHO) "[CL]  out/"
    $(QUIET)$(RM) out/*
    $(ECHO) "[CL]  tmp/"
    $(QUIET)$(RM) tmp/*

I changed the order to print first, like Make does, according to the Principle of Least Surprise.  It makes it more obvious what action was in progress when any error occurs.
Now, when you need to see the actual commands, you can make QUIET= to override the value and get the full output again.  IIRC, the Linux kernel uses a similar system (toggled by a KConfig setting), as does Qt's qmake (toggled with CONFIG += silent in the .pro file).
Re-use Make's pre-defined commands
Instead of writing
%.o: %.cpp
    $(CXX) $(CXXFLAGS) -c $< $(INC) -o $@

%: $(OBJ)
    $(LD) $(LDFLAGS_TEST) $^ -o $@

you can make the rules more like the built-in versions (that you can see with make --print-database):
%.o: %.cpp
    $(COMPILE.cpp) $(OUTPUT_OPTION) $<

%: $(OBJ)
    $(LINK.cpp) $(OUTPUT_OPTION) $^ $(LOADLIBES) $(LDLIBS)

Adapted to use QUIET and ECHO, that's
%.o: %.cpp
    $(ECHO) "[OK]  $@"
    $(QUIET)$(COMPILE.cpp) $(OUTPUT_OPTION) $<

%: $(OBJ)
    $(ECHO) "[OK]  $@"
    $(QUIET)$(LINK.cpp) $(OUTPUT_OPTION) $^ $(LOADLIBES) $(LDLIBS)

You'll need to ensure that your include path is in CXXFLAGS, and you might need target-dependent flags:
CXXFLAGS += $(INC)

tmp/%: CXXFLAGS=$(CXXFLAGS_TEST)

All makefiles should have...
.DELETE_ON_ERROR:

Without this, a failed command could leave a part-written output that a subsequent make believes is up-to-date.
Finally
Here's a tyni tyop:
.PHONY: clean, clear

Lose that , - you've said that clean, is a phony target, not clean as you intended.


Calculator for finding mass I've been trying to create a calculator to find mass using any and all known equations.  The program will also return the steps used to find mass so the user can 'check their work' on physics assignments.

My biggest problem so far (and was solved by much copy-pasta) is the equations themselves. I'd like to be able to create an equation variable that contains other variables inside itself, if possible, I want the variable to state:

var.mass = var.force / var.acceleration && var.energy / var.c^2 && var.density / var.volume

How, if possible, would I go about creating this variable? I could certainly define a different variable for each equation, but that would be extremely time-consuming and I'm sure Python has something that could help! I haven't tried an array, and would prefer not to as I have very little experience with them.  However, if someone could give me an idea, even just the command name I would be very appreciative!

Feel free to use the code however and with whatever you want.

#this script is for solving for mass
#Define variables
g = 9.8
c = 3.00 * 10 * 10 * 10 * 10 * 10 * 10 * 10 * 10
c2 = c * c
E = mass * c * c
#Ask if the user wants to see how problem was solved
show_work = input('Show work? ')
#Asks for variable input
force_val = float(input('Force? (In Newtons) '))
mass_val = float(input('Mass? (In kg) '))
acceleration_val = float(input('acceleration? (In m/s^2) '))
p_val = float(input('density? (In Kg/m^3) '))
volume_val = float(input('Volume? (m^3) '))
weight_val = float(input('Weight? (Normal Force) '))
energy_val = float(input('Energy? '))
kinetic_val = float(input('Kinetic Energy? '))
velocity_val = float(input('Velocity? (In m/s) '))

#Solve for mass based on known values
#show work off

if show_work == 'no':
    if mass_val > 0.00:
        print(mass_val)
    elif force_val > 0:
        if acceleration_val > 0:
            mass_val = force_val / acceleration_val
            print(mass_val)
    elif p_val > 0:
        if volume_val > 0:
            mass_val = p_val / volume_val
            print(mass_val)
    elif weight_val > 0:
        mass_val = weight_val / g
        print(mass_val)
    elif energy_val > 0:
        mass_val = energy_val / c2
        print(mass_val)
    elif kinetic_val > 0:
        if velocity_val > 0:
            mass_val = 1/2 * kinetic_val / velocity_val / velocity_val
            print(mass_val)

#show work on

if show_work == 'yes':
    if mass_val > 0.00:
        print(mass_val)
    elif force_val > 0:
        if acceleration_val > 0:
            mass_val = force_val / acceleration_val
            print('Force = Mass * Acceleration ')
            print('Mass = Force / Acceleration ')
            print('Mass =', force_val, '/', acceleration_val)
            print('Mass =', mass_val)
    elif p_val > 0:
        if volume_val > 0:
            mass_val = p_val / volume_val
            print('Mass = Density / Volume ')
            print('Mass =', p_val, '/', volume_val)
            print('Mass =', mass_val)
    elif weight_val > 0:
        mass_val = weight_val / g
        print('Mass = Weight / Gravity ')
        print('Mass =', weight_val, '/', g)
        print('Mass =', mass_val)
    elif energy_val > 0:
        mass_val = energy_val / c2

        print('E=mc^2')
        print('m=E/c^2')
        print('Mass =', energy_val, '/', 'c^2')
        print('Mass =', mass_val)
    elif kinetic_val > 0:
        if velocity_val > 0:
            mass_val = 2 * kinetic_val / velocity_val / velocity_val
            print('KE = 1/2mv^2')
            print('Mass = 2(KE/v^2)')
            print('Mass =', kinetic_val, '/', velocity_val, '^2')
            print('Mass =', mass_val)
            print(mass_val)

I’m not going to have a go at the big problem of “equation variable” (which might be better suited to Stack Overflow), but I do have some comments on your existing code:

Python uses the operator ** for powers; i.e., a ** b means “a to the power b”. You can use this to tidy up your constants at the top of the file:

c = 3.00 * 10 ** 8
E = mass * c ** 2

I think that’s easier to read, and makes it clear where they’re coming from.

I see no reason to define a c2 variable, especially when you use it inconsistently (sometimes you have c2, sometimes c * c.

The scipy.constants module contains lots of these constants built-in to high levels of precision, but that may be inappropriate for your work.
At the point when E is defined, the variable mass hasn’t been defined. In fact, I can’t find any mention of the variable in the script. What’s it doing here?
Something only happens if the user types exactly yes or no when asked “Show work?” You might be better off defining a list of “acceptable” responses which means the user wants their work shown.

For example:

yes_list = ['yes', 'y', 'true', 't', 'yep']
if input('Show work?').lower() in yes_list:
    show_input = True
else:
    show_input = False

Most of the code is duplicated: once in the branch of the if statement which shows the working, and again in the branch which doesn’t. You’d be better off defining a working_print function which only prints anything if the user wants it. Further, you can skip defining a show_input variable, and get it directly from the user input:

yes_list = ['yes', 'y', 'true', 't', 'yep']
if input('Show work?').lower() in yes_list:
    def working_print(x):
        print(x)
        return None
else:
    def working_print(x)
        return None

Then you can replace instances of print() which are optional with working_print(). For example, the branch

print('E=mc^2')
print('m=E/c^2')
print('Mass =', energy_val, '/', 'c^2')
print('Mass =', mass_val)

would become

working_print('E=mc^2')
working_print('m=E/c^2')
working_print('Mass =', energy_val, '/', 'c^2')
print('Mass =', mass_val)

You have lots of nested if statements of the form

elif p_val > 0:
    if volume_val > 0:
        # code goes here

elif something_else

I think you’d be better off combining the elif and the if into a single line, e.g. something like

if p_val > 0 and volume_val > 0
if min(p_val, volume_val) > 0

It makes it easier to see what the conditions for each branch are. It also leaves you open to less unexpected bugs if none of the branches get run correctly.

And why p_val instead of density_val?
Add an else block at the end of the code which lets you know when it got there, even if it prints something as silly as “Uh oh, don’t know how I got here.” It makes life easier when debugging – you can tell whether none of your branches ran, or whether one of the branches ran but aren’t printing anything.
In several places, you’re checking that variables are positive where it doesn’t seem appropriate.

For example, consider the F = ma branch. Both force and acceleration are vectors, so it makes sense for them both to be negative (corresponding to force/acceleration in the opposite direction). When computing mass, all you require is that they both have the same sign. So your if statement should become

if force_val * acceleration_val > 0

Velocity is also a vector, so it can be signed, and I see no reason why energy (or several other quantities) have to be non-zero.
There’s a great app called Pythonista for iOS which can edit and run Python scripts. (It doesn’t do Python 3, but your script is sufficiently simple that converting to run on 2.x shouldn’t cause any problems.) I don’t know enough about other platforms to suggest anything else.


Searching a string of consecutive characters in a binary bar code I am writing a program that will take a binary bar code and convert it to a zip-code. I need to validate the bar code. A valid binary bar code contains no more and no less than two 1s in any position per five digits. This is what I have for the validation part so far and would like simplify this code.

#include <iostream>
#include <string>

using std::cout;
using std::string;

int main()
{
    const int SIZE = 5;//number of characters to test in a set
    string bar = "100101010010001";

    string tempstring;
    int count = 0;
    while (count < bar.length())
    {
        int test = 0;
        tempstring = bar.substr(count, SIZE);//split bar string into next five characters

        for (int index = 0; index < SIZE; index++)
        {
            if (tempstring[index] == '1')
            {
                ++test;
            }
        }

        if (test != 2)
        {
            cout << "Invalid bar code!\n";
            exit(1);
        }

        count += 5;
    }

    cout << "Valid bar code!\n";
}

The constant SIZE and its comment don't quite seem consistent.  A name such as size would correspond to an actual size of, say, a structure.  But with such a comment, it doesn't seem to be for that.  Consider renaming the constant to something more accurate.
bar should be const since it's not modified.
test seems like an ambiguous name for an integer variable.  It sounds more like a Boolean test flag, but that's not the case here.  Consider renaming it to something more accurate.
Since tempstring is only used within the while loop, it can just be initialized there.  It doesn't need to be declared before it.
You shouldn't mismatch integer types when doing comparisons:

while (count < bar.length())

Since length() returns an std::size_t, which is an unsigned type, have count be of type std::size_t as well.  If you had compiler warnings up, then this should've been reported.
This may be a small nit-pick, but you can just return 1 instead of calling exit() in main().


Account class design using OOP paradigm I have a class Accounts with an array of account numbers and an array of balances I have a member function add_acc that adds an account. But before adding an account, I want to check if the account no. to be added, already exists or not. If it exists then an error message is displayed else the new account is added. In my following class design, the compiler is not able to check for the uniqueness of the account no. when more than 1 accounts are being added. Following is my approach.
#include <iostream>
#include <stdlib.h>
using namespace std;
#define MAX 100
class Account
{
    long long int acc_no[MAX];
    long long int balance[MAX];
    int count;
    public :
        void CNT()
        {
            count=0;
        }
        void add_acc();

};
void Account::add_acc()
{
    if(count==0)
    {
        cout<<"Enter the account number"<<" : ";
        cin>>acc_no[count];
        cout<<"Enter the balance"<<" : ";
        cin>>balance[count];
    }
    else
    {
        long long int temp;
        cout<<"Enter the account no. to be added : ";
        cin>>temp;
        for(int i=0;i<count;i++)
        {
            if(temp==acc_no[i])
            {
                cout<<"Account no. already exists"<<"\n";
                return;
            }
            else
            {
                continue;
            }

        }
        temp=acc_no[count];
        cout<<"Enter the balance : ";
        cin>>balance[count];

    }
    count++;
}
int main()
{
    Account A;
    A.CNT();
    char ch;
    while(ch)
    {
        A.add_acc();
        cout<<"Want to add another account? : ";
        cin>>ch;
        if(ch=='Y' || ch=='y')
            continue;
        else
            break;
     }
    return 0;
}

The problem with my output -:
In the first run I entered the account no. as 123, int the second run I enter the acc. no. as 345 and when in the third run again I entered the acc. no. as 345, the error message was not displayed.

Use unordered_map to maintain account number and balance.
You can use unordered_map(hash table) for checking if an acc_num already exists or not and the membership check in unordered_map is O(1).
You can use int64_t for long long int
You can int64_t for long long int, uint64_t for unsigned long long int and since bank balance can't be less than 0 use uint64_t for balance.
count is unnecessary, use unordered_map::find.
You can eliminate count when you are using unordered_map to check if acc_num exists use unordered_map::find.
Create alias using using.
using account_table not only allows you to give a short name to a complex type, but it puts the type definition in a single place. Then if you change the underlying type you only need to change it in that one place.
Avoid using using namespace std;
Find more details here why using namespace std; is considered bad praactice
I added Account::print_details, it prints acc_num --> balance
Refactored code
#include<iostream>
#include<unordered_map>
#include<string>

class Account{
    using account_table = std::unordered_map<std::string, uint64_t>;
    private:
        account_table table;
    public:
        void add_acc();
        void print_details();
};

void Account::add_acc(){
    std::cout<<"Enter account number : ";
    std::string acc_num;
    std::cin>>acc_num;
    if (table.find(acc_num)==table.end()){
        uint64_t balance;
        std::cout<<"Enter balance : ";
        std::cin>>balance;
        table.insert({acc_num, balance});
    }
    else{
        std::cout<<"Account number already exists\n";
    }
}

void Account::print_details(){
    for(auto& d: table){
        std::cout<<d.first<<"-->"<<d.second<<std::endl;
    }
}

int main(){
    Account A;
    char ch{'Y'};

    while(ch){
        A.add_acc();
        std::cout<<"Want to add another account : ";
        std::cin>>ch;
        if (ch!='Y' && ch!='y'){
            break;
        }
    }
    A.print_details();
    return 0;
}

Example run
Enter account number : 1234
Enter balance : 50
Want to add another account : y
Enter account number : 1234
Account number already exists
Want to add another account : Y
Enter account number : 12345
Enter balance : 12
Want to add another account : n
12345-->12
1234-->50

You can use clang-format-9(code formatting tool)
Example:
clang-format-9 -i -style=Google your_file.cpp