Looping over files in C++ and changing their names to store data into a single file I have a lot of data in separate files generated from a simulation program. All data is clear and formatted via a single space to read in 4 columns. I can open and store data from each file one at a time, but that will get so impossible once I hit more than, say, 10 files of data with the same simulation data columns.

I am just trying to find a better way to open files and store the data using an iterative method instead of typing, for example, "open file, read file data, append to file" over 1000 times.

#include <iostream>
#include <sstream>
#include <fstream>
#include <vector>

using namespace std;

int main ()
{
// INITIALIZE
    int tmax = 80000;

    float time_step[tmax];

    float LEx[tmax];
    float LEy[tmax];
    float LEz[tmax];
    float LE[tmax];

    float AvgLEx;
    float AvgLEy;
    float AvgLEz;
    float AvgLE;

    float Function[tmax];

// FILES
    ifstream infile;
    infile.open("File_1.prn");
// THIS INFILE is where I want to put a loop to change File_1 to File_2 to File_3 and so on and so on to append data from all those files into ONE single file
    if(infile.fail())
    {
        cout << "error reading file" << endl;
        return 1;
    }

    for (int i = 0; i < tmax+1; ++i)
    {
        infile >> time_step[i] >> LEx[i] >> LEy[i] >> LEz[i] >> LE[i];
        AvgLEx = AvgLEx + LEx[i];
        AvgLEy = AvgLEy + LEy[i];
        AvgLEz = AvgLEz + LEz[i];
        AvgLE = AvgLE + LE[i];

    }

    AvgLEx = AvgLEx/tmax;
    AvgLEy = AvgLEy/tmax;
    AvgLEz = AvgLEz/tmax;
    AvgLE = AvgLE/tmax;

    cout << "AvgLE: " << AvgLE << "\n";
    cout << time_step[tmax] << " LEx " << LEx[tmax] << " LEy " << LEy[tmax] << " LEz: " << LEz[tmax] << " LE: " << LE[tmax] << endl;

    cout << "\n";

    infile.close();
    cout << "\nFile read! \n";

// FUNCTIONS

    for (int i = 0; i < tmax+1; ++i)
    {
        Function[i] = LE[i] - AvgLE;
        cout << LE[i] << " " << AvgLE << " " << Function[i] <<  "\n";
    }

    cout << "\n Function set up \n";

// // CONVOLUTION
    float A_t;
    float A_dt;
    int dt, t;

    float max=0;

        for (int dt = 0; dt < 1 + tmax/10; ++dt)
        {
            //dt = 1;
            cout << "dt: " << dt << "\n";
            //A_dt = 0;
            int window = tmax-dt;
            for (int t = 0; t < window; ++t)
            {
                if (t==0)
                {
                    A_t = 0;
                    cout << "A_t was set to zero \n";
                }

                A_t+= Function[t] * Function[t + dt];
                cout << "A_t is accumulation ...: " << A_t << "\n" ;
            }

            A_dt = A_t / window;

            if (A_dt > max)
            {
                max = A_dt;
            }

// OUTPUT
            ofstream AutoCor;
            AutoCor.open ("AutoCor_25.prn", ios::app);
            AutoCor << dt << " " << A_dt << "\n";
            AutoCor.close();
            cout << "A_dt: " << A_dt << " " << "window used: " << window << "\n";

        }

        cout << "max used: " << max << "\n";

// RETURN
    return A_dt;
    return 0;

}

As you're new to C++, you should be aware that implementing an entire program in main() can make things difficult, especially when the code is already complex.

As it appears, you have declare and initialize variables at the top, open the file, perform the operations, then output things.  Even with these comments in place, you should still split this up into functions.

Do not maintain a list of variables as it could make maintenance more difficult, such as if variables are no longer being used.  Instead. you should only declare or initialize variables as close to their intended use as possible.  That way, if you no longer use that variable, you won't have to search for it at the top.  This will also make your code easier to follow.
Consider just having main() do two things: attempt to open the file and call all the necessary functions afterwards.  You can also open the file in an other function, but if you do it in main(), then you can easily return from there if the file cannot be opened.
Everything else, especially the file operations, should be in separate functions.  Also keep in mind that a function should just have one primary purpose, in order to maintain the Single Responsible Principle (SRP).  If a function needs to do something else that another function already does, then it should call that function.  You should also name functions, in the verb form, based on what they're supposed to do.  This will make the function's intent very clear to others.

Example:

void openFile() {}

Miscellaneous:

Try not to use using namespace std in global scope as it could cause name-clashing bugs, especially in larger programs.  See this post for more info on this.
This variable appears to be a constant:

int tmax = 80000;

If so, initialize it with const so that it cannot be modified:

const int tmax = 80000;

For this approach at checking the file:

infile.open("File_1.prn");

if(infile.fail())
{
    cout << "error reading file" << endl;
    return 1;
}

you should use std::ifstream, check for !infile(), print the error message to std::cerr, and then return EXIT_FAILURE:

std::ifstream infile("File_1.prn");

if (!infile)
{
    std::cerr << "error reading file";
    return EXIT_FAILURE;
}


An algorithm that finds the number of "lined up columns" I found this question in an old exam in computer science. I solved it in a good way, but I'm not convinced that my solution is the best solution. I believe that there's a better way to solve it.

  Given N Columns that are lined up, The horizontal distance
  between two adjacent columns (The lower edges) is exactly 1 meter. The
  columns heights are different, and they can only be positive integers
  (In meters). Let's call a Pair of columns (not necessarily adjacent)
  "A Good Pair" if the imaginary line that connects their upper edges creates a gradient of 45 degrees relatively with the ground. The
  slope can go from Left to Right, or from Right to Left. I have to
  write an algorithm that will count all the "Good Pairs" of columns
  that exist. Notice that 1 column could participate in more than 1
  "Good Pair".

  The algorithm must be written in the most efficient way, and have the
  best performance (The less Run-Time possible).

  It's input:

  N, Which represents the number of columns, 5 < N < 500,000 followed by a list of N positive integers that
  represent the heights of the columns, as the 1st number in the list is
  the 1st column's height and the 2nd is for the 2nd column, and so on.
  The algorithm has to print the number of "Good Pairs" that exist
  in the list.

  Example:

  N = 6, and the list of numbers: 3, 5, 5, 3, 4, 1. (The most left number represents the 1st columns height).

  The algorithm should print 5 because there are 5

  "Good Pairs":

  Column 1 and column 3
  Column 2 and column 4
  Column 2 and column 6
  Column 4 and column 5
  Column 4 and column 6

Here's my solution:

static void Main(string[] args)
{
    int n, count = 0; int[] heightList;
    Console.WriteLine("Enter the number of columns");
    n = int.Parse(Console.ReadLine());
    heightList = new int[n];

    Console.WriteLine("Enter the list of heights");
    for (int i = 0; i < heightList.Length; i++)
    {
        heightList[i] = int.Parse(Console.ReadLine());
    }

    for (int i = 0; i < heightList.Length; i++)
    {
        for (int j = i+1; j < heightList.Length; j++)
        {
            //Checking if the pair creates a gradient of 45 degrees with the ground.
            if (Math.Abs(heightList[j] - heightList[i]) == Math.Abs(j - i))
            {
                count++;
            }
        }
    }

    Console.WriteLine(count);
    Console.ReadLine();
}

Is there any better way to solve this question?

You loop over all pairs, which costs O(n^2) time. This is not necessary.

You can partition your input in "good sets": such a set consists of all columns that are on a certain 45-degree line. Every column is in exactly two such sets. You can identify the set by - for instance - its intersection with the x-axis (which is always an integer) plus wether it is upward or downward. As an example, column 3, which has height 5, is in set -2 (upward diagonal) and set 8 (downward diagonal).

Create two hashmaps (one for upward, one for downward) with set identifier as key and number of columns in there as value. You can create these maps in linear time. Now, a set consisting of n columns has n choose 2 = n(n-1)/2 good pairs. Adding them all up gives your answer, still in linear time.

In code, that would be something like:

public static int CountGoodPairs(params int[] heights)
{
    var upCounts = new Dictionary<int, int>();
    var downCounts = new Dictionary<int, int>();

    for (var i = 0; i < heights.Length; i++)
    {
        var height = heights[i];
        IncrementCount(upCounts, i - height);
        IncrementCount(downCounts, i + height);
    }

    return upCounts.Values
                   .Concat(downCounts.Values)
                   .Sum(c => c * (c - 1) / 2);
}

private static void IncrementCount(Dictionary<int, int> counts, int key)
{
    var count = 0;
    counts.TryGetValue(key, out count);
    counts[key] = count+1;
}


Return combined DTO from async controller I have an asynchronous controller.
It is supposed to make a couple async calls then return combined data from then as one DTO.

[HttpGet("[action]/{name}")]
public async Task<CombinedDTO> GetSummonerData(string name)
{
    SummonerEndPoint endPoint = new SummonerEndPoint();
    SummonerDTO summonerData=  await endPoint.GetSummonerData(name);
    MatchesEndPoint endPoint2 = new MatchesEndPoint();
    MatchListDto matchesData =  await endPoint2.GetMatches(summonerData.accountId.ToString());
    return await TestMethod(summonerData, matchesData);
}
public async Task<CombinedDTO> TestMethod(SummonerDTO summonerData, MatchListDto matchData)
    {
         return await Task.Run(() => {
               return new CombinedDTO {
                    summoner = summonerData,
                    matches = matchData }; });
    }

Code above seems to be working but I am not sure is this is the best way to combining two DTOs into one.

for context endpoints:

public class SummonerEndPoint : BaseEndPoint
{
    public SummonerEndPoint() => Url = @"https://eun1.api.riotgames.com/lol/summoner/v3/summoners/by-name/";

    public async Task<SummonerDTO> GetSummonerData(string summonerName)
    {
        var data = await client.MakeAsyncRequest(url + summonerName).ConfigureAwait(false);
        return JsonConvert.DeserializeObject<SummonerDTO>(data);
    }
}

public class MatchesEndPoint : BaseEndPoint
    {
        public MatchesEndPoint() => Url = @"https://eun1.api.riotgames.com/lol/match/v3/matchlists/by-account/";
        public async Task<MatchListDto> GetMatches(string accountId)
        {
            var data = await client.MakeAsyncRequest(Url + accountId + "?beginIndex=0&endIndex=10").ConfigureAwait(false);
            return JsonConvert.DeserializeObject<MatchListDto>(data);
        }
    }

public class BaseEndPoint
    {
        protected string url;
        protected string Url { get => url; set => url = value; }
        protected RiotApiRequest client = new RiotApiRequest();
    }

public class RiotApiRequest : RiotApiRequestBase
    {
        public async Task<string> MakeAsyncRequest(string url)
        {
            using (var response = await MakeRequest(url).ConfigureAwait(false))
            {
                return await GetResponseContentAsync(response).ConfigureAwait(false);
            }
        }

        protected async Task<string> GetResponseContentAsync(HttpResponseMessage response)
        {
            using (response)
            using (var content = response.Content)
            {
                return await content.ReadAsStringAsync().ConfigureAwait(false);
            }
        }
    }

public class RiotApiRequestBase
    {
        protected async Task<HttpResponseMessage> MakeRequest(string url)
        {
            using( HttpClient client = new HttpClient())
            {
                //later this may be moved elsewhere ;)
                client.DefaultRequestHeaders.Add("X-Riot-Token", "Token");
                return await client.GetAsync(url).ConfigureAwait(false);

            }
        }
    }

Short answer would be yes. However you have a few other issues with the code shown.

There is no need for the Task.Run in the example TestMethod.

public async Task<CombinedDTO> TestMethod(SummonerDTO summonerData, MatchListDto matchData) {
    return await Task.Run(() => {
        return new CombinedDTO {
            summoner = summonerData,
            matches = matchData
        };
    });
}

For a simple object like your model there really is no need to throw that on to a new thread. The DTO can be constructed inline.

[HttpGet("[action]/{name}")]
public async Task<CombinedDTO> GetSummonerData(string name) {
    SummonerEndPoint endPoint = new SummonerEndPoint();
    SummonerDTO summonerData =  await endPoint.GetSummonerData(name);
    MatchesEndPoint endPoint2 = new MatchesEndPoint();
    MatchListDto matchesData =  await endPoint2.GetMatches(summonerData.accountId.ToString());
    return new CombinedDTO {
        summoner = summonerData,
        matches = matchData
    };
}

With that out of the way, this code is ripe for refactoring.

Starting from the bottom RiotApiRequestBase should not constantly create and dispose of HttpClient as this can have adverse effects (socket exhaustion).

public interface IRiotApiClient {
    Task<HttpResponseMessage> GetAsync(string url);
}

public class RiotApiClient : IRiotApiClient {
    static Lazy<HttpClient> client = new Lazy<HttpClient>(() => {
        //later this may be moved elsewhere
        var client = new HttpClient();
        client.DefaultRequestHeaders.Add("X-Riot-Token", "Token");
        return client;
    });

    protected Task<HttpResponseMessage> GetAsync(string url) {
        return client.Value.GetAsync(url);
    }
}

Keep an instance for the lifetime of the application. Alternatively, if you are using a more recent version of .net core consider using IHttpClientFactory.

Note the abstraction of the client, it will come into play later on.

The response in MakeAsyncRequest is already going to be disposed by its using. No need to wrap it again in GetResponseContentAsync. This also breaks Single Responsibility Principle as GetResponseContentAsync has no business disposing of the response.

Based on the abstraction of the client, that functionality can be refactored to extension methods.

public static class RiotApiClientExtensions {

    public static async Task<TResult> GetAsAsync<TResult>(this IRiotApiClient client, string url) {
        var data = await client.GetStringAsync(url);
        return JsonConvert.DeserializeObject<TResult>(data);
    }

    public static async Task<string> GetStringAsync(this IRiotApiClient client, string url) {
        using (var response = await client.GetAsync(url).ConfigureAwait(false)) {
            return await ReadContentAsStringAsync(response).ConfigureAwait(false);
        }
    }

    protected static async Task<string> ReadContentAsStringAsync(HttpResponseMessage response) {
        using (var content = response.Content) {
            return await content.ReadAsStringAsync().ConfigureAwait(false);
        }
    }
}

You appear to be following a format similar to CQRS (Command Query Responsibility Segregation), with your end points acting as queries.

I've used the following abstractions to help standardize queries.

using System.Threading.Tasks;

public interface IAsyncFunction<TResponse> {
    Task<TResponse> ExecuteAsync();
}

public interface IAsyncFunction<T, TResponse> {
    Task<TResponse> ExecuteAsync(T parameter);
}

public interface IAsyncFunction<T1, T2, TResponse> {
    Task<TResponse> ExecuteAsync(T1 parameter1, T2 parameter2);
}

public interface IAsyncFunction<T1, T2, T3, TResponse> {
    Task<TResponse> ExecuteAsync(T1 parameter1, T2 parameter2, T3 parameter3);
}

A similar format could be used for Actions/Commands

Using the above, look at the transformation of SummonerEndPoint and MatchesEndPoint

public interface ISummonerEndPoint : IAsyncFunction<string, SummonerDTO> { }

public class SummonerEndPoint : ISummonerEndPoint {
    private readonly string baseUrl;
    private readonly IRiotApiClient client;

    public SummonerEndPoint(IRiotApiClient client) {
        this.client = client;
        baseUrl = @"https://eun1.api.riotgames.com/lol/summoner/v3/summoners/by-name/";
    }

    public Task<SummonerDTO> ExecuteAsync(string summonerName) {
        return client.GetAsAsync<SummonerDTO>(baseUrl + summonerName);
    }
}

public interface IMatchesEndPoint : IAsyncFunction<string, MatchListDto> { }

public class MatchesEndPoint : IMatchesEndPoint {
    private readonly string baseUrl;
    private readonly IRiotApiClient client;

    public MatchesEndPoint(IRiotApiClient client) {
        this.client = client;
        baseUrl = @"https://eun1.api.riotgames.com/lol/match/v3/matchlists/by-account/";
    }
    public Task<MatchListDto> ExecuteAsync(string accountId) {
        return client.GetAsAsync<MatchListDto>(baseUrl + accountId + "?beginIndex=0&endIndex=10");
    }
}

Controller actions should be kept as lean as possible. It should really have only one purpose (SRP).

The initialization of the end points can now be inverted out of the controller and explicitly injected. This reduces the responsibilities of the controller and simplifies the action.

[Route("[controller]")]
public class MyController : Controller {
    private readonly IMatchesEndPoint getMatches;
    private readonly ISummonerEndPoint getSummonerData;

    public MyController(ISummonerEndPoint getSummonerData, IMatchesEndPoint getMatches) {
        this.getMatches = getMatches;
        this.getSummonerData = getSummonerData;
    }

    [HttpGet("[action]/{name}")]
    public async Task<IActionResult> GetSummonerData(string name) {
        SummonerDTO summonerData =  await getSummonerData.ExecuteAsync(name);
        MatchListDto matchesData =  await getMatches.ExecuteAsync(summonerData.accountId.ToString());
        var combinedDto = new {
            summoner = summonerData,
            matches = matchData
        };
        return Ok(combinedDto);
    }
}

Ideally the controller could have been simplified even more with a service that encapsulates the combining of the DTO for reuse but that is totally up to you.

With that all abstractions and their implementations should be registered at the composition root on startup


Merge sort in one Single Function in Scala I have implemented a sortmerge in Scala in one single function. I would like to have it reviewed, so that I can have it well organized, and it is then doing what needs to do.

val a = Array(5, 4, 1, 8, 7, 2, 6, 3)

def mergeSort(a: Array[Int]): Array[Int] = {
  val size = a.length

  if (size == 1) {
    return a
  }
  val a1 = new Array[Int](size / 2)
  val a2 = new Array[Int](size / 2)

  val upTo = (a.length / 2) - 1
  for (i <- 0 to upTo) {
    a1(i) = a(i)
  }
  var pos = 0
  for (i <- upTo+1 to a.length - 1) {
    a2(pos) = a(i)
    pos += 1
  }

  val aR1 = mergeSort(a1)
  val aR2 = mergeSort(a2)
  val aFinalR = new Array[Int](size)

  var i = 0
  var j = 0
  var upBoundR1 = false;
  var upBoundR2 = false;
  for (k <- 0 to aFinalR.length - 1) {
    if (aR1(i) < aR2(j) && !upBoundR1) {
      aFinalR(k) = aR1(i)
      if (i == aR1.length - 1) {
        upBoundR1 = true
      }
      else{
        i = i+1
      }
    } else if (aR2(j) < aR1(i) && !upBoundR2) {
      aFinalR(k) = aR2(j)
      if(j == aR2.length  -1) {
        upBoundR2 = true
      }
      else{
        j = j+1
      }
    } else {
      if(upBoundR1) {
        aFinalR(k) = aR2(j)
        j = if (j == aR2.length - 1) j else j + 1
      } else {
        aFinalR(k) = aR1(i)
        i = if (i == aR1.length - 1) j else i + 1
      }
    }
  }
  return aFinalR
}

val r = mergeSort(Array(1,2,5,4,3,7))

How can I use this function for arrays of different sizes?

And how can I clean up this code a bit more, I feel like the code is a little too big.

I would like to avoid the use of pattern matching, but rather, intentionally sort Arrays by iterating through them, at the same time, I would like to do a canonical merge sort, how can I get there?

Let me very clear here: I don't like what you have done and I don't like what you want to do.

With that out of the way, however, let me explain myself:

Scala is heavily inspired by the functional paradigm, which favors logic development by composition of smaller functions into larger ones and using a more declarative style, where you tell the computer "what" you want to do, not "how" you want to do it., unlike primarily imperative languages like C/C++/C#/Java.

Following the functional paradigm leads to more concise, verifiable and readable code (in most cases) over equivalent imperative implementations.

Pattern matching and recursion are especially important in the functional paradigm, where uncontrolled global mutable state is an anathema.

Yet, in Scala, you want to to do it the imperative way. Very well, Scala allows you to do it. However, should you? Most wouldn't think so.

Now on to the code review.

Observations

All you have done is hard-inlined the merge of mergeSort into the mergeSort() function itself.

If function call overheads are your primary concern for doing this, you probably shouldn't be using Scala. You should be using C/C++.

However, you're using Scala. So split it up. Write the merging function as a separate function, following the Single Responsibility Principle (SRP).

You have said that you don't want to pattern match and want to loop over the elements of the array, when you very well know merge sort in Scala can be implemented as a one-liner. That is kind of like taking the engine out of your car because you want to ride it like it's a quadricycle (or motorbike to bicycle, whichever makes more sense).

Horrible variable naming. aR1 and aR2 mean what, exactly? Why not leftSubArray and rightSubArray? More typing, yes, but that much clearer.

Suggestions

Write Scala like Scala should be written. You will be benefiting any other who will have to work on your code later. You will be benefiting yourself if you ever look at this code after you've gained more experience.
Use an IndexedSeq instead of an Array. Updates will not be in-place if you use the IndexedSeq object's apply() method (which returns a scala.Vector in Scala 2.11), as this is an immutable data structure unlike an Array. However, a Vector gets you type covariance. To get around the immutability in your design, keep the Vector as a var and reassign the result of any calls to updated() on it to itself. This is a first step on the path of functional-ness.
The Scala Predef and standard libraries have a lot of nice functions for you to exploit instead of explicitly looping over everything. You could try foreach, map and flatMap to help. There's also the handly sequence concatenation operator ++. There's also handy comparison functions like orderBy, and something which invalidates the need for what you are trying to do, sorted and sortBy
Try until instead of to for ranges. until will return a half-open interval in comparison to the closed interval returned by to. That is:

0 to 10 = [0,1,2,3,4,5,6,7,8,9,10]
0 until 10 = [0,1,2,3,4,5,6,7,8,9]

That should help get rid of some of the ugly -1s.
Even imperative merging isn't that complicated. Try the following, it should help:

def merge(arr: Array[Int], low: Int, mid: Int, high: Int): Array[Int] = {
    /*[low..mid..high), high is exclusive*/
    var (i, j, k) = (0, 0, 0)
    val (leftLength, rightLength) = (mid - low + 1, high - mid)
    /* create temp arrays */
    val (leftArr, rightArr) = Array(leftLength), Array(rightLength)
    /* Copy data to temp arrays leftArr and rightArr */
    arr.copyToArray(leftArr, low, leftLength)
    arr.copyToArray(rightArr, mid, rightLength)
    /* Merge the temp arrays back into arr(l..r)*/
    i = 0; // Initial index of first subarray
    j = 0; // Initial index of second subarray
    k = l; // Initial index of merged subarray
    while (i < leftLength && j < rightLength){
        if (leftArr(i) <= rightArr(j)){
            arr(k) = leftArr(i);
            i+=1;
        }
        else{
            arr(k) = rightArr(j);
            j+=1;
        }
        k+=1;
    }
    /* Copy the remaining elements of leftArr, if there
       are any */
    while (i < leftLength){
        arr(k) = leftArr(i);
        i+=1;
        k+=1;
    }
    /* Copy the remaining elements of rightArr, if there
       are any */
    while (j < rightLength){
        arr(k) = rightArr(j);
        j+=1;
        k+=1;
    }
    arr
}

I use Array instead of IndexedSeq in the above example as I need the intrinsic mutability of arrays in the presented method. It could have been written using IndexedSeq and assignment updates instead, but that would make the intent of the code a little less clear IMHO.

The functional one can be as simple as:

    def merge(left: IndexedSeq[Int], right: IndexedSeq[Int]): IndexedSeq[Int] =
        (left ++ right).sorted

The implementation without sorted() involves pattern matching, take a look here for details (especially @Shadowlands' answer for one without explicit pattern matching).

This was all assuming you've made merge a separate function. You don't need that many Boolean flags to maintain state if you do sequential merging. If you haven't split them up already, then do it now.
Splitting the array into parts can be handles by the splitAt(Int)function of Array, which takes as a parameter the index to split the array at, and returns a tuple containing the left and right parts. So, we could have the following:

 (leftArr, rightArr) = arr splitAt (arr.length / 2)

You sure you still don't want pattern matching?

Just to show off pattern matching, the canonical Scala solution (adapted to work on Seqs for Scala 2.10+, is the following:

    def mergeSort[T <: Ordered[T]](xs: Seq[T]): Seq[T] = {
      val n = xs.length / 2
      if (n == 0) xs
      else {
        def merge(xs: Seq[T], ys: Seq[T]): Seq[T] =
            (xs, ys) match {
            case(Nil, ys) => ys
            case(xs, Nil) => xs
            case(x :: xs1, y :: ys1) =>
              if (x < y) x::merge(xs1, ys)
              else y :: merge(xs, ys1)
        }
        val (left, right) = xs splitAt(n)
        merge(mergeSort(left), mergeSort(right))
      }
    }

Enriched with parameterized types (generics for a non-math guy)!

And for a code-golf answer, which doesn't really handle empty lists, see @lambruscoAcido's answer on PPCGSE.


Return early with Error from Sequence mapping I have put together this f# code in an attempt to solve the following problem

Given a mapping operation that may not be able to return a value
Cease mapping over the remaining items of a sequence if the mapping operation fails
Append the error to items already mapped and return early

This cannot be achieved with the inbuilt Seq.map and I could not locate any other existing implementations
type OptionFunction<'a, 'b> = 'a -> 'b option
type ResultFunction<'a, 'b, 'c> = 'a -> Result<'b, 'c>

type MaybeProjection<'a,'b,'c> =
    | OptionProjection of OptionFunction<'a,'b>
    | ResultProjection of ResultFunction<'a,'b,'c>

type OptionResultBuilder<'e>() =
    member _.Return(x) = Result.Ok x
    member _.Bind(m,f) =
             let r = match m with
                        | Some a -> Ok a
                        | None -> Error Unchecked.defaultof<'e>
             Result.bind f r
    member _.Bind(m,f) = Result.bind f m

let orb = OptionResultBuilder()

let mapTillFail mapper s =
    let rec inner sq =
        seq {
            let mapped = orb {
                let! head = Seq.tryHead sq
                match mapper with
                | OptionProjection a ->
                    let! m = a head
                    return m
                | ResultProjection a ->
                    let! m = a head
                    return m
                }

            match mapped with
                | Error _ as err-> yield err
                | Ok _ ->
                    yield mapped
                    yield! sq |> Seq.skip 1 |> inner
        }
    inner s

//code to test it, not part of the implementation
let r = Seq.empty
let s = seq{1..10}

let op = OptionProjection (fun x -> if x < 5 then Some x else None)
let rp = ResultProjection (fun x -> if x < 5 then Ok x else Error "if >= 5")

r|> mapTillFail op
s|> mapTillFail op
r|> mapTillFail rp
s|> mapTillFail rp

The inbuilt Option and Result types are a natural fit, and I wanted to be able to use a mapping function that returned either of these types.
Despite several attempts to refactor the duplicate statements inside the match mapper with, I just could not not work out how to satisfy the compiler. Every attempt results in the compiler complaining that it can't find a suitable overload for Bind. Suggestions ?
Beyond that I'm largely happy with it for my purposes. But I welcome any other improvements, or comments on issues like performance (at this stage I've not stress tested it).

I can't say, I have the full understanding of computation expressions, so here are some comments that may go in another direction than your purpose with the post:

sq |> Seq.skip 1

can be replaced with
sq |> Seq.tail

I'm not sure, I like that the function returns [ Error null ] for an empty sequence, but that again may be needed in the context? I would expect an empty sequence to return - an empty sequence.

I don't understand the Projection layer, but that may be dictated by the original context.
You could define the type at the function level:
type MaybeFunction<'a, 'b, 'c> =
    | OptionFunction of ('a -> 'b option)
    | ResultFunction of ('a -> Result<'b, 'c>)

and then create the functions as:
let mbopFunction = OptionFunction (fun x -> if x < 5 then Some x else None)
let mbrsFunction = ResultFunction (fun x -> if x < 5 then Ok x else Error "if >= 5")

Your main function will then looks like:
let mapTillFail mapper s =
    let rec inner sq =
        seq {
            let mapped = orb {
                let! head = Seq.tryHead sq
                match mapper with
                | OptionFunction a ->
                  let! m = a head
                  return m
                | ResultFunction a ->
                    let! m = a head
                    return m
                }

            match mapped with
                | Error _ as err-> yield err
                | Ok _ ->
                    yield mapped
                    yield! sq |> Seq.tail |> inner
        }
    inner s

which tells that your monad can handle that level without modifications.
Maybe it would be fair to call these function Projections as well as that is what they do?

That said, you can go without the computation expression:
let mapTillFail projection data =

    let test x =
        let result =
            match projection with
            | OptionProjection a ->
                match a x with
                | Some d -> Ok d
                | None -> Error Unchecked.defaultof<'c>
            | ResultProjection a -> a x
        result

    let rec inner lst =
        seq {
            match lst |> Seq.isEmpty with
            | true -> yield Error Unchecked.defaultof<'c>
            | _ ->
                match test (lst |> Seq.head) with
                | Error e -> yield Error e
                | Ok o ->
                    yield Ok o
                    yield! inner (lst |> Seq.tail)
        }

    inner data

Another alternative is to extent the MaybeProjection or MaybeFunction with a member called calculate:
type MaybeFunction<'a, 'b, 'c> =
    | OptionFunction of ('a -> 'b option)
    | ResultFunction of ('a -> Result<'b, 'c>)
with
member this.Calculate (a) =
    match this with
    | OptionFunction f ->
        match f a with
        | Some d -> Ok d
        | None -> Error Unchecked.defaultof<'c>
    | ResultFunction f -> f a

used as:
module Seq =
    let mapTillFail (func: MaybeFunction<'a, 'b, 'c>) data =

        let rec inner lst =
            seq {
                match lst |> Seq.isEmpty with
                | true -> ()
                | _ ->
                    match func.Calculate (lst |> Seq.head) with
                    | Error e -> yield Error e
                    | Ok o ->
                        yield Ok o
                        yield! inner (lst |> Seq.tail)
            }

        inner data

Here I've made an extension to Seq so you can call it like any other Seq functions:
printfn "%A" ((r |> Seq.mapTillFail mbopFunction) |> Seq.toList)
printfn "%A" ((s |> Seq.mapTillFail mbopFunction) |> Seq.toList)
printfn "%A" ((r |> Seq.mapTillFail mbrsFunction) |> Seq.toList)
printfn "%A" ((s |> Seq.mapTillFail mbrsFunction) |> Seq.toList)


Effective Python 2/3 two-dimensional array initialisation Recently I noticed that the idiomatic python two-dimensional array initialisation is extremely slow. I am wondering, is there a good, proper way of doing this simple task fast? Also, are those two variants equivalent?

Here are some code snippets using timeit

import timeit

A = 5000
B = 7000

N = 10

def list_comprehension_xrange():
    matrix = [[0 for j in xrange(A)] for i in xrange(B)]
def list_comprehension_range():
    matrix = [[0 for j in range(A)] for i in range(B)]
def multiplication():
    matrix = [[0] * A] * B

print "list_comprehension_xrange:", timeit.timeit(list_comprehension_xrange, number=N)
print "list_comprehension_range:", timeit.timeit(list_comprehension_range, number=N)
print "multiplication:", timeit.timeit(multiplication, number=N)

list_comprehension_xrange: 11.4952278137
list_comprehension_range: 13.5112810135
multiplication: 0.00100612640381

First off don't use multiplication.
It's creates 2 lists, not 7001.

To better show this, hop into IDLE:

>>> a = [[0] * 3] * 3
>>> a
[[0, 0, 0], [0, 0, 0], [0, 0, 0]]
>>> a[0][0] = 1
>>> a
[[1, 0, 0], [1, 0, 0], [1, 0, 0]]

No this is not what you want.

In both your other functions you should use _,
this is a convention that says you don't use the result, it's thrown away.

This results in:

def list_comprehension_xrange():
    matrix = [[0 for _ in xrange(A)] for _ in xrange(B)]

If speed is highly important, then you can use a hybrid of multiplication and range or xrange.
From the benchmarks I'd lean towards xrange, but I don't think there is too much difference.

I can't say exactly why these are so fast, it may be because the multiplication doesn't build and destroy an intermarry list.
Or it's not creating a list in Python code but in C. Or it's not building ~A*B amount of objects only ~B amount. But I don't.

I added the following functions, with timers:

def multiplication_range():
    matrix = [[0] * A for _ in range(B)]
def multiplication_xrange():
    matrix = [[0] * A for _ in xrange(B)]

And got the following results:

list_comprehension_xrange: 23.0122457496
list_comprehension_range: 24.9418833563
multiplication: 0.00104575910762
multiplication_range: 3.35667382897
multiplication_xrange: 3.35528033683


Enigma simulator performance Here is my implementation of a simple 3 rotor Enigma machine in C++:

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

char alpha[] = "ABCDEFGHIJKLMNOPQRSTUVWXYZ";
char rotors[3][27] =
{
  "EKMFLGDQVZNTOWYHXUSPAIBRCJ",
  "AJDKSIRUXBLHWTMCQGZNPYFVOE",
  "BDFHJLCPRTXVZNYEIWGAKMUSQO"
};
char reflector[] = "YRUHQSLDPXNGOKMIEBFZCWVJAT";
char key[] = "ABC";

long mod26(long a)
{
  return (a%26+26)%26;
}

int li (char l)
{
  // Letter index
  return l - 'A';
}

int indexof (char* array, int find)
{
  return strchr(array, find) - array;
}

string crypt (const char *ct)
{
  // Sets initial permutation
  int L = li(key[0]);
  int M = li(key[1]);
  int R = li(key[2]);

  string output;

  for ( int x = 0; x < strlen(ct) ; x++ ) {
    int ct_letter = li(ct[x]);

    // Step right rotor on every iteration
    R = mod26(R + 1);

    // Pass through rotors
    char a = rotors[2][mod26(R + ct_letter)];
    char b = rotors[1][mod26(M + li(a) - R)];
    char c = rotors[0][mod26(L + li(b) - M)];

    // Pass through reflector
    char ref = reflector[mod26(li(c) - L)];

    // Inverse rotor pass
    int d = mod26(indexof(rotors[0], alpha[mod26(li(ref) + L)]) - L);
    int e = mod26(indexof(rotors[1], alpha[mod26(d + M)]) - M);
    char f = alpha[mod26(indexof(rotors[2], alpha[mod26(e + R)]) - R)];

    output += f;
  }

  return output;
}

int main ()
{
  for ( int i = 0; i < 1000000; i++) {
    crypt ("PZUFWDSASJGQGNRMAEODZJXQQKHSYGVUSGSU");
  }

  return 0;
}

Benchmark ran on a 2.66ghz Core 2 Duo for 1 million decrypts:

C++ Version:

11.64s user 0.02s system 97% cpu 11.963 total

I'm not a C++ programmer, I first wrote this in Javascript and then translated it into C++.

Whilst 1 million in ~10 seconds is OK, I feel that my implementation isn't optimal. Are there any optimisations, or micro optimisations that you could recommend to my C++ code to make it faster? Maybe I should try a different language? Any suggestions are welcome.

Let's profile this a little bit (this is on a dual-core i7, so a bit quicker):

  Running Time  Self        Symbol Name

  3984.0ms  100.0%  0.0     Main Thread  0x2856c

  3982.0ms   99.9%  0.0      start

  3982.0ms   99.9%  1.0       main

  3936.0ms   98.7%  3081.0         crypt(char const*)

  640.0ms   16.0%   640.0           _platform_strchr

  126.0ms    3.1%   102.0           std::__1::basic_string, std::__1::allocator >::push_back(char)

  89.0ms    2.2%    89.0            strlen

  14.0ms    0.3%    14.0           DYLD-STUB$$strchr

  14.0ms    0.3%    4.0        free

  8.0ms    0.2% 1.0        szone_free_definite_size

  7.0ms    0.1% 7.0        DYLD-STUB$$strlen

  2.0ms    0.0% 2.0        operator delete(void*)

  2.0ms    0.0% 0.0      _dyld_start

Ok, so more than 98% of the time is spent in the crypt function (which isn't surprising really). Almost 16% of the time is spent in strchr, with another 3.1% spent in push_back. There's also a few percent (2.2) spent doing strlen.

Ok, so how do we cut down on these:

Firstly, we'll switch over to using std::array<char> instead of char *. These are both stack-allocated (and so we don't incur the dynamic allocation overhead of std::string or std::vector), but std::array knows its size, and will allow us to remove the strlen in the crypt function:

#include <array>

const std::array<char, 27> alpha = {"ABCDEFGHIJKLMNOPQRSTUVWXYZ"};
const std::array<std::array<char, 27>, 3> rotors
{
    {{"EKMFLGDQVZNTOWYHXUSPAIBRCJ"},
    {"AJDKSIRUXBLHWTMCQGZNPYFVOE"},
    {"BDFHJLCPRTXVZNYEIWGAKMUSQO"}}
};

const std::array<char, 27> reflectors = {"YRUHQSLDPXNGOKMIEBFZCWVJAT"};
const std::array<char, 4> key = {"ABC"};

The indexof function then needs to change as well (and I'm going to rename it to index_of, which is easier to read):

template <size_t N>
std::size_t index_of (const std::array<char, N>& str, int find)
{
    for(std::size_t i = 0; i < N; ++i) {
        if(str[i] == find) return i;
    }
    return -1;
}

crypt now looks like:

template <size_t N>
std::array<char, N> crypt (const std::array<char, N>& ct)
{
    // Sets initial permutation
    int L = li(key[0]);
    int M = li(key[1]);
    int R = li(key[2]);

    std::array<char, N> output;

    for ( unsigned x = 0; x < N ; x++ ) {
        int ct_letter = li(ct[x]);

        // Step right rotor on every iteration
        R = static_cast<int>(mod26(R + 1));

        // Pass through rotors
        char a = rotors[2][mod26(R + ct_letter)];
        char b = rotors[1][mod26(M + li(a) - R)];
        char c = rotors[0][mod26(L + li(b) - M)];

        // Pass through reflector
        char ref = reflectors[mod26(li(c) - L)];

        // Inverse rotor pass
        long d = mod26(index_of(rotors[0], alpha[mod26(li(ref) + L)]) - L);
        long e = mod26(index_of(rotors[1], alpha[mod26(d + M)]) - M);
        char f = mod26(index_of(rotors[2], alpha[mod26(e + R)]) - R);

        output[x] = alpha[f];
    }

    return output;
}

which is close to what it was, but with a few minor changes (no += on a string, as we know already how large it will be and simple assign directly into that index).

And we now call it like so:

int main ()
{
    for ( int i = 0; i < 1000000; i++) {
        crypt(std::array<char, 37>{"PZUFWDSASJGQGNRMAEODZJXQQKHSYGVUSGSU"});
    }

    return 0;
}

Let's profile this again:

  Running Time  Self        Symbol Name

  2873.0ms  100.0%  0.0     Main Thread  0x2c7ff

  2872.0ms   99.9%  0.0      start

  2872.0ms   99.9%  2.0       main

  2870.0ms   99.8%  2870.0         std::__1::array crypt<37ul>
  (std::__1::array const&)

  1.0ms    0.0% 0.0      _dyld_start

Ok, so everything except crypt has now been inlined. If we apply the modification from @MrSmith42, we shave even more time off:

  Running Time   Self        Symbol Name

  1855.0ms  100.0%  0.0     Main Thread  0x2ca5c

  1854.0ms   99.9%  0.0      start

  1854.0ms   99.9%  1.0       main

  1853.0ms   99.8%  1853.0         std::__1::array crypt<37ul>
  (std::__1::array const&)

  1.0ms    0.0% 0.0      _dyld_start

We have imposed a fairly large constraint however: we need to know the size of the string to be encrypted at compile-time (so no reading it from a file or stdin or the like). If this will always be the case, then this provides a fairly significant speed-up on my hardware.


Calculate compounded interest My job for now is to teach a trainee how to program. I told him to solve the following task, given by a book (I try to translate it):

  Write a program for compound interest calculation. The user has to
  enter the amount of money, the years, and the interest rate. The
  program should look like this:

Money in Euro: 100
Annual percentage rate: 6
Term in years: 4
Money after 1 years: 106.0
Money after 2 years: 112.36
Money after 3 years: 119.1016
Money after 4 years: 126.247696

Important Note

He hasn't learned anything about OOP, exception handling or using objects and methods yet. He now learns how to design simple algorithms. But he has gotten a hint how to use the scanner class to get the user input.

I want to show him my own example solution, so that he can learn. I want to ask you, if my example solution is okay for showing it to a beginner. Is it well written or have I ignored any good practices?

// use "int variable = scanner.nextInt()" for getting int values
// use "double variable = scanner.nextDouble()" for getting double values

import java.util.Scanner;

public class Main {
    private static Scanner scanner = new Scanner(System.in);

    public static void main(String args[]) {
        // write your solution here:
        double money;
        int years;
        double rate;

        System.out.print("Money in Euro: ");
        money = scanner.nextDouble();
        System.out.print("Annual percentage rate: ");
        rate = scanner.nextDouble();
        System.out.print("Term in years: ");
        years = scanner.nextInt();

        for (int i = 1; i <= years; i++) {
            money += money * (rate / 100);
            System.out.println("Money after " + i + ". years: " + money);
        }
    }
}

I have some suggestion for you.

1) Constant name should always be uppercase.

private static final Scanner SCANNER = new Scanner(System.in);

2) When creating arrays, I suggest that you use the java style instead of the c style, since it's the more used and less error-prone, in my opinion.

Before

public static void main(String args[]) {
   //[...]
}

After

public static void main(String[] args) {
   //[...]
}

3) For the variables, I suggest that you create them on the same line as the initialization, you will save 3 lines in the method and make the code more readable.

        System.out.print("Money in Euro: ");
        double money = SCANNER.nextDouble();
        System.out.print("Annual percentage rate: ");
        double rate = SCANNER.nextDouble();
        System.out.print("Term in years: ");
        int years = SCANNER.nextInt();

4) You can extract the questions in methods, to separate the logic and it will be easier to read, if you want; but will have to explain to him :)

public static void main(String args[]) {
    // write your solution here:

    double money = askForAmountInEuro();
    double rate = askForAnnualPercRate();
    int years = askForTerm();

    for (int i = 1; i <= years; i++) {
        money += money * (rate / 100);
        System.out.println("Money after " + i + ". years: " + money);
    }
}

private static int askForTerm() {
    System.out.print("Term in years: ");
    return SCANNER.nextInt();
}

private static double askForAnnualPercRate() {
    System.out.print("Annual percentage rate: ");
    return SCANNER.nextDouble();
}

private static double askForAmountInEuro() {
    System.out.print("Money in Euro: ");
    return SCANNER.nextDouble();
}
```


Facial recognition tool I created a small library and an example application in Python for learning about facial recognition and experimenting with it. Right now though, I am loading a list of file names into memory and then allowing the code to iterate over those names in order to do comparisons or to find the faces.

I think my example and the library is pretty inefficient but I am unsure of how best to handle large amounts of photos using my library. Also, it only works on one photo at a time but my understanding is that the facial recognition library uses all cores and I should only look at one photo at a time.

How can I make my code more efficient?

The original facial recognition library I am using is found at https://github.com/ageitgey/face_recognition

My Library:

import face_recognition
import os, os.path
import ntpath
from PIL import Image

def get_image_directory(path):
  imgs = []
  valid_images = [".jpg",".gif",".png",".tga"]
  for f in os.listdir(path):
    ext = os.path.splitext(f)[1]
    if ext.lower() not in valid_images:
      continue
    imgs.append(os.path.join(path,f))
  return imgs

def recognize_faces(path):
  file_name = path_leaf(path)
  image = face_recognition.load_image_file(path)
  face_locations = face_recognition.face_locations(image)
  i = 0
  for face_location in face_locations:
    top, right, bottom, left = face_location
    face_image = image[top:bottom, left:right]
    pil_image = Image.fromarray(face_image)
    pil_image.save( 'output/' + str(file_name) + '_' +str(i)+".png")
    i+=1

def compare_faces(unknown_images, known_images):
  for image in unknown_images:
    unknown_comparison_image = create_face_comparison_encoding(image)
    for k_image in known_images:
      results = face_recognition.compare_faces([k_image],unknown_comparison_image)
      if results[0] == True:
        print("This person appears familiar!")
        print("Writing the faces to output folder!")
        recognize_faces(image)
      else:
        print("This doesn't appear to be familiar!")

def path_leaf(path):
  head, tail = ntpath.split(path)
  return tail or ntpath.basename(head)

def create_face_comparison_encoding(image):
  known_image = face_recognition.load_image_file(image)
  known_image_encoding = face_recognition.face_encodings(known_image)[0]
  return known_image_encoding

Application:

from ml_face import *

##
# The Program
##
unknown_persons = get_image_directory('images/') # Unknown Persons Directory
known_persons = get_image_directory('known_person/') # Known Persons Directory
known_list = [] # Empty List
for k_per in known_persons:
  known_comparison_image = create_face_comparison_encoding(k_per)
  known_list.append(known_comparison_image)
compare_faces(unknown_persons,known_list)

I noticed that the face_recognition library itself is quite optimized to run on both multiple cpus and gpu as well. But I will try to give my two cents for your specific code here.

Typically, there are two steps when it comes to such applications.

There is the training step where you parse all the known_person images once, and build a data of face_encodings, and store them on the disk. Typically these encodings are much smaller than the image data itself in size and it is something you do once for all the known images. (if you add new known images, consider this as a recalibration step where you need to regenerate your encodings). This step is usually accepted to be time-consuming and not done very frequently.

Then there is the inference step, which is the compare_faces function. This is what you want to do several times. For this specific step you just need the new image and known_list(which is the trained data that you stored on disk ahead of time).

So to improve your library, I would suggest adding a 'train' function which allows a list of images. This list should then store the encodings in a format (compressed if you have several images).

Then the compare_faces function can be modified to take in the encodings data. You can split this into load step (in case you compressed the data) and then compare step. This allows application to load the data once, but infer several times.

In your code itself, the compare_faces can be improved already. It can be changed to:

def compare_faces(unknown_images, image_encodings_data):
  for image in unknown_images:
    unknown_data = create_face_comparison_encoding(image)
    # Note how compare_faces takes multiple encodings.
    results = face_recognition.compare_faces(image_encodings_data,unknown_data)
    if results.count(True) > 0:
      print("This person appears familiar!")
      print("Writing the faces to output folder!")
      recognize_faces(image)
    else:
      print("This doesn't appear to be familiar!")

Also, I find it strange that you do not return which face it actually matched to.

Please feel free to ignore me if this is no longer your concern :)


Open text file and read data to an arrayfire array using Qt and C++ I'm currently writing a C++ project with a Qt GUI and would like to read numerical data from user-selected text files through QtFileDialog::getOpenFileName. There are quite a few methods around which use std::vector, but I will be using the arrayfire library and it generally seems to be a bit more convenient to either use C-style arrays instead of std::vector for storing the data in host memory, or to send the data to an arrayfire array directly. This is what I've done here.

The idea is to pull the data into arrayfire arrays that are part of a struct (dataStruct) which is defined as follows:

struct dataStruct{
    af::array d1, d2, d3;
};

To get the filename from the user I call this in the MainWindow instance after a signal from a pushButton in the GUI:

dataStruct dS;
QString tmp =
    QFileDialog::getOpenFileName(this, tr("Open data file"), "C:\\",
                                 tr("Text Files (*.txt)"));
std::string filename = tmp.toStdString();
loadData(filename, dS);

loadData is the function which extracts the data from the text file, and to do so needs to count the number of rows of data. This is done using getNumberOfLines below:

void getNumberOfLines(std::string filename, int &numLines)
{
    std::ifstream in(filename);
    std::string line;

    while(std::getline(in, line))
        numLines  = numLines + 1;
}

And finally, loading the data into the struct arrayfire arrays:

void loadData(std::string filename, structType &dS)
{
    int numLines = 0;
    getNumberOfLines(filename, numLines);

    std::ifstream in(filename);
    std::string line;
    int i = 0;

    af::array data1 = af::constant(0, numLines, f32);
    af::array data2 = af::constant(0, numLines, f32);
    af::array data3 = af::constant(0, numLines, f32);

    while (std::getline(in, line))
    {
        std::istringstream is(line);
        std::string 1_tmp, 2_tmp, 3_tmp;

        is >> 1_tmp >> 2_tmp >> 3_tmp;
        data1(i) = std::stof(1_tmp) * pow(10, -9);
        data2(i) = std::stof(2_tmp);
        data3(i) = std::stof(3_tmp);
        i++;
    }
    dS.d1 = data1;
    dS.d2 = data2;
    dS.d3 = data3;
}

I'm not so happy with getNumberOfLines and loadData because I end up reading the file data twice: First to get the number of rows/lines, the second to extract the data. I think it would be much better if this process could be reduced to a single step somehow.

One thing that I would like to implement is a function similar to getNumberOfLines or extension to loadData that allows the user to select data files with an arbitrary number of columns too. This could involve assigning all data to a single arrayfire array with dimensions given by (numRows, numCols, 1, 1) or similar, and then to split this array by columns later on to get the relevant data. Not sure how to implement this yet, so ideas are welcome!

If there are improvements to the code that other people can spot I'd really appreciate it if they could let me know. This will make up a small part of my first "big" project in C++, so please flag up any screamers that I've adopted when handling data or structuring the code.

Prefer return over Output parameter

A result as an out parameter seems like a smell to me:

void getNumberOfLines(std::string filename, int &numLines)
{
    std::ifstream in(filename);
    std::string line;

    while(std::getline(in, line))
        numLines  = numLines + 1;
}

Why not return the number of lines as a result of the function?

Also when counting the lines you don't actually need to read the strings out. You just need to find the end of line marker. To avoid the cost of copying and re-sizing lines for data you don't need, you can use the std::istream::ignore() method.

Lets also pass the filename by reference to avoid a copy (copying strings is relatively expensive (its const as you don't modify it)).

std::size_t getNumberOfLines(std::string const& filename)
{
    std::size_t   result = 0;
    std::ifstream in(filename);

    while(in.ignore(std::numeric_limits<streamsize>::max(), '\n') {
        ++result;
    }
    return result;
}

You can now change the call point to:

// From
int numLines = 0;
getNumberOfLines(filename, numLines);

// To
int numLines = getNumberOfLines(filename);

Illegal identifiers:

    std::string 1_tmp, 2_tmp, 3_tmp;

Those are not legal variable names. Not sure how you think this is compiling!

Check stream reads/writes

You should always check the result of a stream input/output operation to make sure it worked. Streams are the one place in the standard where exceptions are turned off so you need to check the error codes (luckily this is easy as the stream used in a boolean context will convert itself into the current error code state).

    // Here you read value from a stream
    // But don't check that it worked.
    is >> 1_tmp >> 2_tmp >> 3_tmp;
    data1(i) = std::stof(1_tmp) * pow(10, -9);
    data2(i) = std::stof(2_tmp);
    data3(i) = std::stof(3_tmp);
    i++;

Should look like this:

    if (is >> 1_tmp >> 2_tmp >> 3_tmp)
    {
        data1(i) = std::stof(1_tmp) * pow(10, -9);
        data2(i) = std::stof(2_tmp);
        data3(i) = std::stof(3_tmp);
        i++;
    }
    else
    {
        // If something went wrong with one line.
        // Then do you trust the rest of the stream?
        // I think not. So you should also mark the input stream
        // bad at this point.
        in.setstate(std::ios::failbit);

        // This will cause the outer loop reading the file to stop.
    }

Use the stream read ability

Why read a string when the data is a float?

Why go to the effort of parsing a string when the data could be a float? In your code you are effectively scanning each value twice, which seems like a lot of extra work.

 std::string 1_tmp;
 in >> 1_tmp;
 double 1_val = std::stof(1_tmp);

Instead, use the existing read float capabilities of the stream input operator.

 double 1_val;
 int >> 1_val;

Copy ss Move

This is a copy:

dS.d1 = data1;
dS.d2 = data2;
dS.d3 = data3;

But you never use data[123] again. So you may as well try and move the data.

dS.d1 = std::move(data1);
dS.d2 = std::move(data2);
dS.d3 = std::move(data3);


Maximum subarray problem solved with divide and conquer also for the all negative element case Solving the maximum subarray problem with added cases of finding non maximum non contiguous subarray as well. I used divide and conquer approach and tackled the 'all negative' case by keeping a counter for that and branching it.

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

// finding maximum non contiguous  subarray by adding up all the non negative elements in array

int max_noncont_sub(int a[],int *lower, int *upper)
{
    int sum=0, i ;
for(i = *lower; i <= *upper; i++)
{
    if(a[i]>0)
        sum += a[i];
}
return sum;
}

// finding max subarray which include the middle term, starting from mid and branching both sides till we encounter negative element

int max_cross_sub(int A[], int *lower, int mid, int *upper)
{
int left_sum = INT_MIN;
int sum = 0;
int i, max_left;

for(i = mid; i>=*lower; i--)
{
    sum = sum + A[i];
    if(sum>left_sum)
        left_sum = sum;
    max_left = i;
}

int right_sum = INT_MIN;
sum = 0;
int max_right;

for(i = mid+1; i<=*upper; i++)
{
    sum = sum + A[i];
    if(sum>right_sum)
        right_sum = sum;
    max_right = i;
}
*lower = max_left;
*upper = max_right;
return left_sum + right_sum;

}

//dividing the problem into 3 parts, left subarray and right have same substructure, 3rd one is solved by max_noncont_sub().

int max_sub(int A[], int *lower, int *upper)
{
    if(*lower == *upper)
    {
        return A[*lower];
    }   else
          {
               int mid = *lower + (*upper - *lower)/2;
               int left_low = *lower;
               int left_high = mid;
               int left_sum = max_sub(A, &left_low, &left_high);

               int right_low = mid+1;
               int right_high = *upper;
               int right_sum = max_sub(A, &right_low, &right_high);

               int cross_low = *lower;
               int cross_high = *upper;
               int cross_sum = max_cross_sub(A, &cross_low, mid, &cross_high);

               if(left_sum >=right_sum && left_sum>= cross_sum)
               {
                   *lower = left_low;
                    *upper = left_high;
                  return left_sum;
               } else if(right_sum >=left_sum && right_sum>= cross_sum)
                   {
                        *lower = right_low;
                        *upper = right_high;
                        return right_sum;
                   } else
                       {
                            return cross_sum;
                        }

        }
 }

int main()
{
    int T, N, nc=0;
     scanf("%d",&T);
    while(T--)
    {
        scanf("%d",&N);
        int *a = (int*)malloc(N*sizeof(int));
        int i;

        nc=0;
        for(i = 0; i<N; i++)
       {
            scanf("%d",&a[i]);
            if(a[i]<0)
                nc++; // counting the negative numbers for later tackling with all negative special case
       }

    int lower = 0;
    int upper = N-1;

    if(nc == N)  // if all elements are negative, we just get the least negative and its the answer.
    {
        int max=INT_MIN;
        for(i =0;i<N;i++)
        {
            if(a[i]>max)
                max = a[i];
        }
        printf("%d %d\n",max, max);

    }
    else
    {
        printf("%d %d\n",max_sub(a, &lower, &upper), max_noncont_sub(a, &lower, &upper));
    }
 }
   return 0;
}

Any review and suggestions are welcome.
Is there a better way to do it? I know there's kadane's algorithm with O(n) running time.

Bugs

Your sum in max_cross_sub() could overflow an int, causing a change in sign.  Therefore, you could either miss the maximum subarray if your sum erroneously became negative, or get the wrong subarray if your sum erroneously became positive.
It seems that you want lower and upper to return the bounds of the maximum subarray.  Otherwise you could just pass those arguments as regular integers instead of pointers.  However, max_sub() doesn't set the correct bounds in the case of cross_sum being the max sum.
In addition to #2, max_cross_sub() doesn't set *lower and *upper to the correct bounds.  The problem is that you don't set max_left and max_right correctly, so you end up leaving the bounds unchanged from whatever was passed in.
This line isn't right:

printf("%d %d\n",max_sub(a, &lower, &upper), max_noncont_sub(a, &lower, &upper));

The problem with it is that the call to max_sub() changes lower and upper, so that the call to max_noncont_sub() has the wrong bounds passed in.  You should use two separate copies of the lower and upper bounds instead.

Better way

Clearly you already knew about Kadane's algorithm, because you mentioned it.  The question is why you didn't use it?  Not only is it faster, it's also only 6 lines of code compared to the roughly 60 lines you used to do it the divide and conquer way.


Get all members from country depending on multiple tables I have 4 tables, filled with some example data

user table

id | name  | idAddress |
------------------------
1  | Bernd | 1         |
2  | Max   | 2         |
3  | Tom   | 3         |
4  | Bob   | 4         |
5  | Alice | 5         |

address

id | country |  zip  |
----------------------
1  | DE      | 39213 |
2  | CH      | 13847 |
3  | NL      | 38472 |
4  | DE      | 94872 |
5  | MT      | 54682 |
6  | US      |  4682 |
7  | DE      | 45853 |
8  | NA      | 12144 |

orga2Address

idOrga   | idAddress |
------------------
1        | 6         |
1        | 7         |
2        | 7         |
2        | 8         |

orga2User

idUser | idOrga |
-----------------
1      | 1      |
1      | 2      |
2      | 1      |
3      | 2      |
4      | 2      |

I want to select all user names that have address in 'DE' in the user table or that are in an orga which has an address in 'DE' in orga2Address.

I was able to get the result with the UNION command like this (see this SQLFiddle):

SELECT `name`, `country` FROM (SELECT `user`.`name`, `address`.`country` from `user`
INNER JOIN `address` ON `user`.`idAddress` = `address`.`id`
UNION
SELECT  `user`.`name` from `user`
INNER JOIN `orga2User` `o2U` ON `o2U`.`idUser` = `user`.`id`
INNER JOIN `orga2Address` `o2A` ON `o2A`.`idOrga` = `o2U`.`idOrga`
INNER JOIN `address` `a` ON `a`.`id` = `o2A`.`idAddress`) as `U` WHERE `U`.`country`="DE";

The result

name   |
--------
Bernd  |
Max    |
Tom    |
Bob    |

Is there a quicker and better way to get the wanted result?

Here's a more compact formulation (SQL Fiddle):

SELECT DISTINCT user.name, address.country
    FROM orga2Address
        INNER JOIN orga2User
            ON orga2User.idOrga = orga2Address.idOrga
        RIGHT OUTER JOIN user
            ON user.id = orga2User.idUser
        INNER JOIN address
            ON address.id IN (user.idAddress, orga2Address.idAddress)
    WHERE address.country = 'DE';

The trick is, instead of UNION, perform a JOIN with more than one join path.  (We unfortunately need to use DISTINCT, but that is no worse than your UNION, which also de-duplicates results.)

In addition, I suggest:

formatting your code with better indentation.  In this example, the SELECT statement has a FROM clause and a WHERE clause.  The FROM clause has multiple JOINs, each of which has an ON clause.
using single quotes for string literals like 'DE', which is standard ANSI SQL.  Using double quotes for strings is a MySQLism, and it fails if SQL_MODE = 'ANSI_QUOTES' is in force.
avoid overdoing the `quotes`.  If you didn't need them in your CREATE TABLE statements, you don't need them in your query either.


OOP demonstration in C++17 using a Pokémon I want to showcase with as minimal code as possible the basic Object-oriented programming (OOP) principles of Polymorphism, Inheritance, and Encapsulation. I know there are many more principles than just these 3 and even different types of the 3 I've mentioned there. I attempted to implement them all below. I think using Pokémon as an example is a good way to show this, so I've used Castform as my buddy of choice for these examples.

If you believe showcasing more than just Polymorphism, Inheritance, and Encapsulation to evaluate a developers understanding of OOP, please provide comments around this and which additional key principles I should add to my Pokémon example.

Main.cpp

#include <iostream>
#include <string>
#include <time.h>

class Pokemon
{
    protected:
        int m_dex_num;
        float m_catch_rate;
        std::string m_type;
    public:
        Pokemon(int dex_num, int catch_rate, std::string type)
            : m_dex_num(dex_num), m_catch_rate(catch_rate), m_type(type) {}

        int dex_num() const { return m_dex_num; }
        float catch_rate() const { return m_catch_rate; }
        std::string type() const { return m_type; }

        virtual bool attempt_catch() = 0;
};

class Castform : public Pokemon
{
    private:
        std::string m_forms[3] = { "sunny", "rainy", "snowy" };
    public:
        Castform() : Pokemon(351, 11.9, "normal") {}

        bool attempt_catch() {
            srand(time(NULL));
            return (rand() % 100) < m_catch_rate;
        }
};

int main()
{
    auto wild_castform = Castform();

    std::cout << "Wild Castform appeared!\n";
    std::cout << "Dex number " << wild_castform.dex_num() << '\n';
    std::cout << "It's a " << wild_castform.type() << " type\n";

    for(int i=0; i < 5; ++i) {
            std::cout << "Catch attempt resulted in " << wild_castform.attempt_catch() << '\n';
    }

    return 0;
}

It looks like all the Pokemon member variables should be private, not public.
The Pokemon constructor takes catch_rate as an int, where it should be a float.
The Pokemon constructor could std::move the string argument into m_type.
Pokemon::type() could return by const& to avoid an unnecessary copy.
Castform::m_forms is not used anywhere, but should probably also be static and const.
When overriding a virtual function, we should always use the override keyword (and arguably also the virtual keyword):

virtual bool attempt_catch() override { ... }

Use the C++11 <random> functionality, not rand().

Run-time polymorphism with virtual functions in C++ is driven by a need to treat objects of different types with the same interface. Unfortunately, this example lacks the motivation for it:

Currently the attempt_catch() function could be implemented in the Pokemon base-class with no problem.
There is no example of differing behavior (i.e. there should be a second pokemon type implementing a different attempt_catch() function). Maybe an attack function would work better?
There is no demonstration of using the different types through the same interface. In C++ this usually boils down to storing different types in the same container (e.g. iterating a std::vector<std::unique_ptr<Pokemon>> and calling the virtual function). However, it might be simpler to pass two different pokemon types to a function taking a Pokemon reference:

void throw_pokeball(Pokemon const& target) { target.attempt_catch() } // or something

Depending on what this is actually for, I'd suggest approaching things in a "problem -> solution" way, both in terms of what the code does (see above), but also in terms of language development and why these features exist. i.e.

"Here's what programmers used to have to do without this language feature [...]. It sucks because [...]"
"This language feature allows us to do [...] safely and easily because [...]".

Note that there are many different types of both static and dynamic polymorphism in C++ (e.g. function overloading, implicit conversions, function objects, template parameters (traits, tags, etc.)), not just inheritance and virtual functions.


Tail Recursive Fibonacci Please critique my implementation of a tail-recursive method for generating the Fibonacci sequence:

def fib(n: Int): Option[Int] = {
    @tailrec
    def go(count: Int, prev: Int, acc: Int): Option[Int] = {
        if   (count == n) Some( acc )
        else              go(count+1, acc, acc + prev)
    }
    if      ( n < 0)   None
    else if ( n == 0 ) Some( 0 )
    else if ( n == 1)  Some ( 1 )
    else               go(2, 1, 1)
}

go is under your control, you can guarantee that it will always have a result, so you don't need to use Option for it. Also, the special cases for 0 and 1 are not neccessary. Counting backwards instead of forwards is more a matter of taste here, but it does reduce the dependency on the outer context.

def fib(n: Int): Option[Int] = {
    @tailrec
    def go(count: Int, prev: Int, acc: Int): Int = {
        if (count == 0) acc else go(count-1, acc, acc + prev)
    }
    if ( n < 0) None else Some(go(n, 1, 0))
}

However, if you want a really fast implementation (only interesting when you use BigInt), you should use these formulas, where you need only ~log(n) iterations instead of n:

$$
\begin{align}
F_{2n-1} &= F_n^2 + F_{n-1}^2 \\
F_{2n}   &= (F_{n-1} + F_{n+1}) F_n \\
         &= (2F_{n-1} + F_n) F_n
\end{align}
$$


Javascript function to return an array that needs to be in a specific order, depending on the order of a different array I have an array of objects, reviews which I get from an API endpoint. This object looks like this:
[
  { name: "Facebook", count: 50 },
  { name: "Google", count: 43 },
  { name: "TripAdvisor", count: 67 },
  { name: "Other", count: 130 }
]

I have a chart.js Pie chart that I'm feeding this data to. I'm using the name for the labels and the count for the size of each slice of the pie chart. Now, due to how chart.js works, I have to supply some hexadecimal colors to use as background colors, and the order in which the background color is being defined matters if I want the labels to match the colors properly. For example, if the order of the labels is [A, B, C], an array of colors like [red, green, blue] means that A = red, B = green and C = blue. The problem is that the reviews array comes in a random order every single time from the API (I don't have control over the API and the data it gives me), so I can't just have a simple, predetermined array.
As far as I see, I have two options, I either sort the reviews array in some way, or I iterate over the data I get from the API and return an array based on some simple switch statements. I opted for the 2nd option, as it seems simpler for my usecase.
My current solution looks like this
const chartBackgroundColor = collection => {
  let colorArray = [];
  const itemArray = collection.map(data => data.name);
  itemArray.forEach(item => {
      switch (item) {
        case "TripAdvisor":
          colorArray.push("#00B98B");
          break;
        case "Google":
          colorArray.push("#FFBD00");
          break;
        case "Facebook":
          colorArray.push("#4B6DAA");
          break;
        default:
          colorArray.push("#D9D9D9");
          break;
      }
    }
  )
  return colorArray;
}

The method works fine, but I was just curious whether there was some more elegant way of handling this issue, and whether a switch case is appropriate in this situation rather than a if/ifel/else tree.

I would

Encapsulate the names and colors in a data structure
Use .map instead of .forEach

So something like this;
function chartBackgroundColor(collection){
  const nameColorMap = {
    "TripAdvisor": "#00B98B",
    "Google": "#FFBD00",
    "Facebook": "#4B6DAA",
  };
  return collection.map(data => nameColorMap[data.name] || "#D9D9D9");
}


Custom key class for .NET 3.5 framework I use .NET 3.5 framework, so no tuples. I have many use cases where I have to create a custom key for a dictionary. How can I make this better?

public class CompositeKey<T>
{
    public T Content { get; set; }
    public Func<T, object>[] Lambdas { get; set; }

    public CompositeKey(T obj, params Func<T, object>[] propLambdas)
    {
        Content = obj;
        Lambdas = propLambdas;
    }

    public override int GetHashCode()
    {
        int hash = 0;
        foreach (var l in Lambdas)
        {
            hash ^= l(Content).GetHashCode();
        }
        return hash;
    }

    public override bool Equals(object obj)
    {
        bool isEqual = true;
        if (obj is T)
        {
            T o = (T)obj;
            foreach (var l in Lambdas)
            {
                isEqual &= (l(Content) == l(o));
            }
            return isEqual;
        }
        return false;
    }
}

public class TestCode
{
    public void somemethod()
    {
        Dictionary<CompositeKey<TestPOCO>, string> dict = new Dictionary<CompositeKey<TestPOCO>, string>();
        var t1 = new TestPOCO() { ID=1, Name="A" };
        var t2 = new TestPOCO() { ID = 2, Name = "B" };
        dict.Add(new CompositeKey<TestPOCO>(t1, x => x.ID, x => x.Name) , t1.Name);
        dict.Add(new CompositeKey<TestPOCO>(t2, x => x.ID, x => x.Name), t2.Name);
    }
}

public class TestPOCO
{
    public int ID;
    public string Name;
}

Especially this part:

        dict.Add(new CompositeKey<TestPOCO>(t1, x => x.ID, x => x.Name) , t1.Name);
        dict.Add(new CompositeKey<TestPOCO>(t2, x => x.ID, x => x.Name), t2.Name);

The challenge is to have lambda defined only once, but have different key objects based on the POCO.

Mutable keys are a very bad idea, try this:

public void Foo()
{
    var mutable = new Mutable { Value = 1 };
    var dictionary = new Dictionary<Mutable, string> { { mutable, "one" } };
    mutable.Value = 2;
    var explodesHere = dictionary[mutable];
}

public class Mutable
{
    public int Value;

    public override int GetHashCode()
    {
        return Value;
    }
}

GetHashCode must not change while the instance is used in a collection that relies on the value. There are two ways to solve this:

Calculate the hash from values that cannot change. Prefer this wherever possible.
Make sure that the values affecting the hash are not changed while the instance is used in a collection. This is a pit of failure.

You can create a dictionary passing in an IEqualityComparer<TKey>
About your example:

ID reads like a property that uniquely identifies an entity and does not change.
If this is the case it is a nice candidate to use as a key. I'm guessing a bit here but I'm getting a feeling you really want:

var idNameMap = new Dictionary<int, string>();
idNameMap.Add(t1.ID, t1.Name);


MS Access SQL query for active assigned cases I've inherited an Access database and am looking to start using it to do reporting. I've written a query that gets me the information I'm looking for, I'm just hoping someone can have a look over it and tell me if I'm doing it the wrong way or not.

Anyway, the query takes information from 3 tables:

sites - more detailed information on each workplace
employees - list of employees and their associated workplace
cases - holds the bulk of the information needed for the report

I want most fields from the cases table as this is how work is tracked, but I want to also group by the city which is in the sites table and by employee name from the employees table.

SELECT
    sites.city,
    employees.[first name],
    employees.[last name],
    cases.[assigned to],
    cases.ref,
    cases.status,
    cases.priority,
    cases.[opened date],
    Round(Now()-cases.[opened date],0) AS Age
FROM
    cases
LEFT JOIN
    (employees LEFT JOIN sites ON employees.site = sites.site)
ON
    cases.[assigned to] = employees.username
WHERE
    cases.[assigned to]<>"Not Assigned"
AND
    cases.status="Active"
AND
    cases.priority<>"first contact"
ORDER BY
    sites.city,
    employees.[first name],
    employees.[last name],
    cases.[opened date] desc;

Is there any stupid rookie mistakes in here that I've fallen prey to? Could that be done any better? Have I not given enough information in my question?

I can't see any major flaw in this query.

I would just make the Jointures like this, as it's more clear :

SELECT
    sites.city,
    employees.[first name],
    employees.[last name],
    cases.[assigned to],
    cases.ref,
    cases.status,
    cases.priority,
    cases.[opened date],
    Round(Now()-cases.[opened date],0) AS Age
FROM ((cases
LEFT JOIN employees ON cases.[assigned to] = employees.username)
LEFT JOIN sites ON employees.site = sites.site)
WHERE cases.[assigned to]<>"Not Assigned"
  AND cases.status="Active"
  AND cases.priority<>"first contact"
ORDER BY
    sites.city,
    employees.[first name],
    employees.[last name],
    cases.[opened date] desc;

If you can't have any case with non existing employees and sites, it's better to change your 2 LEFT JOIN into INNER JOIN. Just throw the 2 queries and compare the results.

In your WHERE clause, having some <> is generally inefficient on large tables. It's always better to make a IN() and list all the possibilities excluding "Not assigned" or "first contact" :

If  cases.[assigned to] can only contain an employee username or "Not Assigned", then just doing an INNER JOIN on the employee table will automatically filter the not assigned and you don't need to specify it in the WHERE clause.
if you have a limited number of case.priority values, list them all but "first contact" : AND cases.priority IN ("priority1", "super urgent", "can wait")

For the Age, if you want to compute the days from your open_date, it's better to do :

dateDiff("d",Now(),cases.[opened date]) AS Age

you can also concatenate the first and last name in one column:

 (employees.[last name] & ' ' & employees.[first name]) AS EmpName,

That's all what comes to my mind.


Type for each value and naming, anonymous structs inside struct Here is little snippet from my types.h code:

/**
 * tick_t: is used to store and manipulate with time (tick count)
 * Unsigned, at least 32bits
 * 32bit number gives us 71 minutes resolution
 * 64bit number gives us 584'942 years resolution
 * Use OMFW_TIME_ISELAPSED(targetTime, currentTime) for time comparing
 */
typedef unsigned long tick_t;

/**
 * pos_t: Signed integer which holds position
 * In most cases even 16bit number is enough
 */
typedef int pos_t;

/**
 * num_t: Floating point number used to calculate
 * Please note, if you want use fixed point or any custom
 * number, you should use operator overoading feature of C++
 * and also implement following functions for it in omfw/math.h
 */
typedef double num_t;

/**
 * step_t: Hold current step value
 * Only increased / decreased by one
 * May be signed, unsigned, floating point
 */
#ifdef OMFW_PRECOMPUTE_TABLE
typedef uint32_t step_t;
#else
typedef num_t step_t;
#endif

and here definition of struct in different file:

typedef struct {
    num_t acceleration;
    num_t speed;
    num_t pulseLength;

    int id; //index 0,1,2...
    int32_t letter; //unicode letter

    /** total resolution = motor resolution * gearbox */
    pos_t motorResolution;
    num_t gearbox;

    int pinSignal;
    int pinDirection;
    bool invertSignal;
    bool invertDirection;
} MotorSettings;

typedef struct {

    /** Motor settings */
    MotorSettings set;

    /** Maximum achievable limits
     * Computed from "in" variables */
    struct {
        num_t speed; //v-max; max steps/sec
        num_t acceleration; //a-max; Max accceleration that can be used
    } max;

    /** Position data stored here */
    struct {
        pos_t resolution;
        pos_t curPosition;
        pos_t tarPosition;

        //TODO impl - when motor lose step, increment this
        pos_t curOffset;

        dir_t curDirection;
        dir_t tarDirection;
    } pos;

    /** Calculated and changed by program
     * Used as indications to calculate realtime data */
    struct {
        num_t speed; //v = steps/sec
        num_t acceleration; //a = steps/sec.sec
    } cur;

    /** Used for Realtime caclulations and timing
     * Should not be used in other parts of program than realtime */
    struct {
        //tick count to turn off and next tick
        //cannot overflow
        tick_t timeTurnOff;
        tick_t timeNextMove;

        num_t firstDelay; //c0; depending on TIME_RESOLUTION
        num_t minimalDelay; //clast; TIME_RESOLUTION / maxSpeed

        step_t stepsToAccelerate; //nmax
        step_t currentStep; //n
    } rt;

} Motor;

I am doing one little project that I would like to publish soon, but I have few questions about naming:

Is having multiple types for each value OK in way I have them? I hope yes, I tried keep it as simple as I knew, but we are arguing with friend about that - he thinks that we should add omfw_ prefix to them and make them more general (eg. make tick_t just 64bits), but I want to make this code run fast on 16bit microcontrollers too and having 32bit timing is working good.
Do they have correct naming? I added _t prefix as uintx_t have.
Is it OK to have multiple anonymous structs inside main struct? There is lot of data in here and I want to organize them. Should I declare then as normal structs, even when I dont want to serialize them or access as single ones (except MotorSettings)?
Do the structs and variables in them have correct naming?
And last one. I am not native English speaker, so if there are any errors in text or code, I would appreciate if you let me know. Thanks

typedef unsigned long tick_t;

  we are arguing with friend about that - he thinks that we should [...] make tick_t just 64bits [...] but I want to make this code run fast on 16bit microcontrollers too

Personally, I would never use the long or unsigned long types in production code, because they vary in size even between different 32-bit platforms. On some platforms, int is 32 and long is 32 and long long is 64; on other platforms, int is 32 and long is 64 and long long is 64. Rather than play a guessing game with my program's future portability requirements, I would write explicitly

#if OMFW_USE_32_BIT_TIMES
typedef uint32_t tick_t;
#else
typedef uint64_t tick_t;
#endif // OMFW_USE_32_BIT_TIMES

This is much more typing, but it conveys the intended semantics unambiguously: that is, by default I want 64-bit tick_t, but I also want to give the builder the option of using 32-bit tick_t in case it's helpful to him. In fact, this lets the builder pass -DOMFW_USE_32_BIT_TIMES on his 32-bit platform, or -DOMFW_USE_32_BIT_TIMES=0 on his 16-bit platform if he really wants to.

  Do they have correct naming? I added _t prefix as uintx_t have.

FYI, that's a _t suffix (appended), not a prefix (prepended).

This is fine; but you are courting collisions with the standard library. For example, the standard library already defines time_t and off_t; if you get in the habit of using the exact same naming conventions as the standard library, then you might one day run into trouble when you need a name for an entity representing a "time" type or an "offset" type.

Therefore I would recommend at least namespacing the identifiers as omfw_time_t, omfw_offset_t, etc. (Also notice that I wrote out "offset" instead of "off", because it's clearer and the compiler certainly won't care that we're being a little more verbose.)

int pinSignal;
int pinDirection;

Surely pinDirection should be a bool. Also, in all cases where you're using int, I suggest being explicit about the expected range of the value: is an int16_t good enough? Could we get away with int8_t? I would definitely use unadorned int for computations, but here you're defining a storage format, which means it would be nice to clarify exactly how many bytes each field is expected to have.

  Is it OK to have multiple anonymous structs inside main struct?

Yes. But I do think it's confusing to sometimes have a multi-level access path and sometimes not. For example, you access one field as

myMotor.cur.speed

but another as

myMotor.pos.curDirection

Why is it not myMotor.cur.direction?  And why is myMotor.rt.currentStep not myMotor.cur.step or at least myMotor.rt.curStep, given that you're abbreviating current to cur on every other line of the program?

(Again, personally I would write out current and especially target; abbreviating them as cur and tar is more confusing than helpful IMHO. With current you have to worry about whether the proper abbreviation is cur or curr (I've seen both, particularly since curr is the same number of characters as prev and next and last). I've never seen target abbreviated as tar (although I have seen targ and tgt).

/**
 * num_t: Floating point number used to calculate
 * Please note, if you want [to] use fixed point or any custom
 * number, you should use operator over[l]oading feature of C++
 * and also implement following functions for it in omfw/math.h
 */
typedef double num_t;

You don't explain which functions are "the following functions." Also, it might be more appropriate to write this as

#ifndef OMFW_NUM_T
#define OMFW_NUM_T double
#endif // OMFW_NUM_T

typedef OMFW_NUM_T num_t;

so that it can be specified by the user at build time as e.g. -DOMFW_NUM_T=float.

/**
 * step_t: Hold current step value
 * Only increased / decreased by one
 * May be signed, unsigned, floating point
 */
#ifdef OMFW_PRECOMPUTE_TABLE
typedef uint32_t step_t;
#else
typedef num_t step_t;
#endif

I'm suspicious of this code. Why is it okay for the step number to be a double (which is what num_t is)? At the very least, you should replace the word num_t with double here for clarity. But also, maintaining this code is going to be a nightmare without a very thorough test suite. For example, you'll have to be very careful never to use ++ or % with a step number, because those don't work with double.

Why not just let the step number be int and be done with it?

Incidentally, I don't know what the initialism omfw_ stands for, but you should be aware that to a native English speaker it bears an amusing resemblance to "omfg". :)


Elixir generate inverted / reversed map I would like to transform the input
elems = [a, b, c]
terms = [t1, t2]

@spec contains?(term, elem) :: boolean
def contains?(term, elem), do: #implementation

to either of
%{t1 => [a, b], t2 => [c]}

or
%{t1 => [a], t2 => [b, c]}

where
t1 |> contains?(a) #=> true
t1 |> contains?(b) #=> true
t1 |> contains?(c) #=> false

t2 |> contains?(a) #=> false
t2 |> contains?(b) #=> true
t2 |> contains?(c) #=> true

My current solution is as follows
defmodule Test do
  def contains?(term, elem) do
    elem in term
  end

  def test do
    elems = [1,2,3]
    terms = [[1,2], [2,3]]

    elems
    |> Enum.into(%{}, fn elem ->
      {elem,
       terms |> Enum.find(&contains?(&1, elem))}
    end)
    |> reverse_map()
  end

  defp reverse_map(map, reversed \\ %{})
  defp reverse_map(map, reversed) when map_size(map) == 0, do: reversed

  defp reverse_map(map, reversed) do
    [key | _] = Map.keys(map)
    {value, map} = Map.pop!(map, key)
    reversed = Map.update(reversed, value, [key], &[key | &1])
    reverse_map(map, reversed)
  end
end

With this solution I'm generating the map
%{a => t1, b => t1, c => t2}

then reversing it and collecting collisions in a list.
But I feel this intermediate map is unnecessary and a solution could exist without it.
In addition I'm not sure my implementation of reverse_map is as elegant as it could be.

defmodule Test do
  def contains?(term, elem) do
    elem in term
  end

  def test do
    elems = [1,2,3]
    terms = [[1,2], [2,3]]

    for e <- elems, reduce: %{} do
      acc ->
        key = Enum.find(terms, &contains?(&1, e))
        Map.update(acc, key, [e], &[e|&1])
    end
    #=> %{[1, 2] => [2, 1], [2, 3] => [3]}
  end
end

Found a simple and elegant solution using a reduce comprehension.
Here, we iterate over each elem in elems, find the first term which contains it, then update our map with either term => [elem] or term => [elem|previous_elems] depending on if that key exists in the map yet.

Side note:
If I needed the elems in the same as order they were originally I could use & &1 ++ [e] instead of &[e|&1], but as I don't have that requirement it is more efficient to prepend the list.


Is there a better flow for returning the response? So I have the getSessionNamespace method in the below class in which I am trying to return different scenarios but one json response. So my solution is to store the required message and status code in a response array and vary it depending on the situation.

Is there a cleaner more obvious method?

<?php

namespace Drupal\auth\Controller;

use Drupal\Core\Controller\ControllerBase;
use Drupal\auth\Service\AuthGroupService;
use Symfony\Component\HttpFoundation\Cookie;
use Symfony\Component\HttpFoundation\Request;
use Symfony\Component\DependencyInjection\ContainerInterface;
use Symfony\Component\HttpFoundation\JsonResponse;
use GuzzleHttp\Exception\ClientException;
use Symfony\Component\HttpKernel\Exception\HttpException;

/**
 * Class SessionController
 * @package Drupal\my_moduke\Controller
 */
class SessionController extends ControllerBase
{
    /**
     * @var AuthGroupService
     */
    protected static $authGroupService;

    /**
     * @param ContainerInterface $container
     *
     * @return static
     */
    public static function create(ContainerInterface $container)
    {
        self::$authGroupService = $container->get('auth.auth_group');
        return parent::create($container);
    }

    /**
     * @param Request $request
     * @param bool    $internal
     *
     * @return mixed|JsonResponse
     */
    public function getSessionNamespace(Request $request, $internal = false)
    {
        try {
            if (!$request->headers->has('host')) {
                throw new \HttpHeaderException('Header \'host\' not found.');
            }

            $host = $request->headers->get('host');

            // Query datastore for the authGroup data for the given host.
            $result = $this->getAuthGroupData($host);
            $configId = \Drupal::config('auth.settings')->get('DS5.auth_group_id');

            if (!$this->allowedAccess($configId, $result['authGroupId'])){
                throw new HttpException(403, 'Not allowed.');
            }

            $response = [
                'result' => $result,
                'status' => 200
            ];
        } catch (ClientException $e) {
            $response = [
                'result' => 'The endpoint responded with a ' . $e->getCode(),
                'status' => $e->getCode()
            ];
        } catch (\Exception $e) {
            $response = [
                'result' => $e->getMessage(),
                'status' => $e->getStatusCode()
            ];
        }

        if ($internal) {
            return $response['result'];
        }

        $jsonResponse = new JsonResponse($response['result'], $response['status']);

        if (!$_COOKIE[$response['result']['cookieName']] && $response['status'] === 200) {
            $this->createSessionCookie($jsonResponse, $response['result']);
        }

        return $jsonResponse;
    }

    /**
     * @param JsonResponse $response
     * @param array        $values
     *
     * @return JsonResponse
     */
    private function createSessionCookie(JsonResponse $response, array $values)
    {
        $dateTime = new \DateTime('+'. $values['sessionTimeToLiveSeconds'] .' seconds');

        $sessionCookie = new Cookie(
            $values['cookieName'],
            session_id(),
            $dateTime,
            '/',
            $values['domain']
        );

        $response->headers->setCookie($sessionCookie);
    }

    /**
     * @param $configId
     * @param $authGroupId
     *
     * @return bool
     */
    private function allowedAccess($configId, $authGroupId)
    {
        return $configId === $authGroupId;
    }

    /**
     * @param $host
     *
     * @return mixed
     */
    private function getAuthGroupData($host)
    {
        // Fetch from authGroup datastore endpoint.
        return self::$authGroupService->fetchAuthGroupData($host);
    }
}

I think this generally looks pretty good.  I would consider two things:

First I don't like the "internal" option here which leads to mixed return values.  Have you considered having getSessionNameSpaceInternal() which would call this method and extract the desired return value from the JsonResponse?
Doing the above would allow you to directly return from catch block s like this:

} catch (ClientException $e) {
    return new JsonResponse(
        'The endpoint responded with a ' . $e->getCode(),
        $e->getCode()
    );
} catch (\Exception $e) {
    return new JsonResponse( $e->getMessage(), $e->getStatusCode() );
}


Convert hex color string to SDL Color I am writing a small game and as a part of it I load json config with colors definitions in a format of strings #00ff00.

Then I am using this function to convert these strings to SDL Color structure. I am assuming that all color strings are a valid hex colors (between 6-7 chars, depending if # (hash) is provided at the beginning).

template <class T>
inline unsigned int toIntFromHexString(const T& t) {

    unsigned int x;
    std::stringstream ss;
    ss << std::hex << t;
    ss >> x;
    return x;

}

inline SDL_Color stringToSDLColor(std::string colorString) {

    SDL_Color color;

    if (colorString[0]=='#') {
        colorString.erase(0,1);
    }

    std::string stringR = colorString.substr(0,2);
    std::string stringG = colorString.substr(2,2);
    std::string stringB = colorString.substr(4,2);
    std::string stringA = colorString.substr(6,2);

    color.r = toIntFromHexString(stringR);
    color.g = toIntFromHexString(stringG);
    color.b = toIntFromHexString(stringB);
    if (stringA.length()>0) {
        color.a = toIntFromHexString(stringA);
    }

    return color;

}

What can be adjusted in the implementation?

Rather than breaking the string into substrings, then converting each individually from a hex string to an integer, I'd probably convert the entire string at once, then use bit shifting to get the pieces you care about.

SDL_Color hex2sdl(std::string input) {

    if (input[0] == '#')
        input.erase(0, 1);

    unsigned long value = stoul(input, nullptr, 16);

    SDL_Color color;

    color.a = (value >> 24) & 0xff;
    color.r = (value >> 16) & 0xff;
    color.g = (value >> 8) & 0xff;
    color.b = (value >> 0) & 0xff;
    return color;
}

This simplifies the code quite a bit, and at least in my mind makes it rather easier to read.

Another point to consider (when/if your compiler supports it) would be to use an std::string_view instead of a std::string for the input. A string_view is something like a pointer/length, so (when needed) it can trim the # from the beginning without copying all the data like an actual string normally will. Even if your compiler supports string_view itself, however, it may easily not support passing a string_view to std::stoul, which kind of ruins the idea (for now).


Random Sentences generator This program use random number generator to create sentences. It prints 20 sentences randomly.
Here is the code:

#include <stdio.h>
#include <string.h>
#include <time.h>
#include <ctype.h>

#define STR_LEN 80
#define MAX_SEN 20

int main(void) {
    char *article[] = {"the", "a", "one", "some", "any"};
    char *noun[] = {"boy", "girl", "dog", "town", "car"};
    char *verb[] = {"drove", "jumped", "ran", "walked", "skipped"};
    char *preposition[] = {"to", "from", "over", "under", "on"};
    int num;
    char sentence[MAX_SEN][STR_LEN];
    char (*i)[STR_LEN];
    srand((unsigned) time(NULL));
    for(i = sentence; i < sentence + MAX_SEN; i++) {
        num = rand() % (sizeof(article)/sizeof(article[0]));
        strcpy(*i, article[num]);
        num = rand() % (sizeof(noun)/sizeof(noun[0]));
        strcat(strcat(*i, " "), noun[num]);
        num = rand() % (sizeof(preposition)/sizeof(preposition[0]));
        strcat(strcat(*i, " "), preposition[num]);
        printf("%s.\n", *i);

    }

    return 0;
}

Can you make some improvements on the code especially on the sizeof operator. You can use any things functions, arrays, strings, pointers etc.
Also, I don't know how to make first letter capital of each sentence using toupper() function.

Sorry for no explanation. Not really sure why I wrote this. Just saw your post, got in the zone, and suddenly I have a chunk of code! Hope it is insightful.

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

const int SEN_LEN = 80;
const int MAX_SEN = 20;

const char* ARTICLES[] = {"the", "a", "one", "some", "any"};
const char* NOUNS[] =  {"boy", "girl", "dog", "town", "car"};
const char* VERBS[] =  {"drove", "jumped", "ran", "walked", "skipped"};
const char* PREPOSITIONS[] =  {"to", "from", "over", "under", "on"};
const int ARTICLES_SIZE = sizeof(ARTICLES)/sizeof(ARTICLES[0]);
const int NOUNS_SIZE = sizeof(NOUNS)/sizeof(NOUNS[0]);
const int VERBS_SIZE = sizeof(VERBS)/sizeof(VERBS[0]);
const int PREPOSITIONS_SIZE = sizeof(PREPOSITIONS)/sizeof(PREPOSITIONS[0]);

char* generateSentence() {
  char* sentence = calloc((SEN_LEN+1), sizeof(char));

  //Build Sentence
  strcat(sentence, ARTICLES[rand()%ARTICLES_SIZE]);

  strcat(sentence, " ");
  strcat(sentence, NOUNS[rand()%NOUNS_SIZE]);

  strcat(sentence, " ");
  strcat(sentence, VERBS[rand()%VERBS_SIZE]);

  strcat(sentence, " ");
  strcat(sentence, PREPOSITIONS[rand()%PREPOSITIONS_SIZE]);

  //Capitalize first letter
  sentence[0] = toupper(sentence[0]);

  return sentence;
}

int main(int argc, char* argv[]) {
    srand(time(NULL));

    for(int i = 0; i < MAX_SEN; i++) {
        char* sentence = generateSentence();
        printf("%s.\n", sentence);
        free(sentence);
    }

    return 0;
}

Requires C99. (-std=c99 on gcc)