Multi threading with webrequests/responses I have been working with multi threading and would like some feedback on the code, i generally suck at class design/OO which i am trying to improve. Im sure there is a better way of making it neater and maximizing readability.

using System;
using System.Collections.Generic;
using System.IO;
using System.Linq;
using System.Net;
using System.Text;
using System.Threading;
using System.Threading.Tasks;
using System.Xml;

namespace XMLSender
{
    class Program
    {
        private static string serverUrl;
        static void Main(string[] args)
        {
            Console.WriteLine("Please enter the URL to send the XML File");
            serverUrl = Console.ReadLine();

            List<Thread> threads = new List<Thread>();
            List<string> names = new List<string>();
            string fileName = "";

            do
            {
                Console.WriteLine("Please enter the XML File you Wish to send, To start simulation type 'start'");
                fileName = Console.ReadLine();
                if (fileName != "start")
                {
                    Thread t = new Thread(new ParameterizedThreadStart(send));
                    threads.Add(t);
                    names.Add(fileName);
                }
            }
            while (fileName != "start");

            for (int i = 0; i < threads.Count; i++)
            {
                var t = threads[i];
                var name = names[i];
                t.Start(name);
            }

            foreach (Thread t in threads)
            {
                t.Join();
            }

        }
        static private void send(object data)
        {
            try
            {
                //Set the connection limit of HTTP
                System.Net.ServicePointManager.DefaultConnectionLimit = 10000;

                //ServicePointManager.ServerCertificateValidationCallback = delegate { return true; };
                HttpWebRequest request = (HttpWebRequest)WebRequest.Create(http://www.contoso.com/example.aspx);

                byte[] bytes;

                //Send XML data to Webserver
                bytes = File.ReadAllBytes((string)data);
                request.ContentType = "text/xml; encoding='utf-8'";
                request.ContentLength = bytes.Length;
                request.Method = "POST";
                //request.Timeout = 20000;
                request.KeepAlive = false;

                using (Stream requestStream = request.GetRequestStream())
                {
                    requestStream.Write(bytes, 0, bytes.Length);
                    requestStream.Close();
                }

                // Get response from Webserver
                using (HttpWebResponse response = (HttpWebResponse)request.GetResponse())
                {
                    Stream responseStream = response.GetResponseStream();
                    string responseStr = new StreamReader(responseStream).ReadToEnd();
                    Console.WriteLine(responseStr + Environment.NewLine);
                    responseStream.Close();
                }

            }
            catch (Exception e)
            {
                Console.WriteLine("An Error Occured" + Environment.NewLine + e);
                Console.ReadLine();
            }
        }
    }
}

You say you suck at class design but there's only one class in your code and everything is very "linear". I'm no expert anyway, but I'll try to help.

First of all you should separate the Main() function from the code you're executing (unlesss the code is short), this will improve readbility at it will be easier to update the code. To do this you can create a class where the main work will happen.

On the Main() function:

serverUrl = Console.ReadLine();

If you have many variables it might be better to declare this at the start of the function, but it isn't always necessary. In my opinion on the top you should put the variables that will be used many times or that are important.

while (fileName != "start");

I would do this with an if but this works fine. What I would also do is to save all the inputs in a list, either a List<> or a string[].
Also you're doing the "start" checking twice, one on the loop, and one on the code inside the loop.

It's simpler like this:

List<string> fileNames = new List<string>();
while (currentLine != "start")
    fileNames.Add(currentLine);

After this you iterate thorugh the items 2 times, I will go on that later.

Now we can create a class that does the "heavy work". Let's ask ourselves: what do we want this class to do? And what does it need to do that?

Well, now you have the class functions and its params.
We want the class to send data on, it needs the "XML file path", and we want to do it on a  thread.

 class TestClass
{
    private string xmlPath;
    public TestClass(string _xmlPath)
    {
        xmlPath = _xmlPath;
    }
    public void SendDataAsync(object data)
    {

    }
    public void SendData(object data)
    {

    }
}

We create a local class variable that's asked when we create the class and that will be accesed later: xmlPath.

Sometimes, using Threads it's nasty and can cause problems and performance issues (if using many of them, so I like using Tasks. This is what will be happening on the SendDataAsyncFunction:

 public void SendDataAsync(object data)
        => Task.Run(() => SendData(data));

If we only have one line of code using => makes it simpler than using brackets.

Then on the SendData function:

On the first line we see a try block, then is it really necessary? What could throw an exception?
Things that could throw and exception are File.ReadAllBytes , request.Write and the response, in this case it's ok to have a try block on the whole function, but normally you shouldn't do that.

 System.Net.ServicePointManager.DefaultConnectionLimit = 10000;

Is it necessary to do this every time? Why not take this to the main function? Since it's only executed once.

Then the bytes are read, to reduce code lines we can do this:

 byte[] bytes = File.ReadAllBytes(xmlPath);

Next the HttpResquest is created, if we take this to another function (ideally each function should do only 1 thing), the code will be easier to read and if something has to be change less problems will occur.

 private HttpWebRequest CreateRequest(int length)
    {
        WebRequest request = WebRequest.Create(ATMSimulator.Properties.Settings.Default.KTHBaseUri);
        request.ContentType = "text/xml; encoding='utf-8'";
        request.ContentLength = length;
        request.Method = "POST";
        return (HttpWebRequest)request;
    }

We take the cast to the end of the function, since it's not necessary to do it earlier.

Then we want to write the request. If you use using the stream will be closed itelf. We can change the code to this:

using (Stream requestStream = request.GetRequestStream())
            requestStream.Write(bytes, 0, bytes.Length);

Then the function receives the response, we can take that to a different function too, like this:

 private string ReceiveResponse(HttpWebRequest request)
    {
        using (HttpWebResponse response = (HttpWebResponse)request.GetResponse())
            return new StreamReader(response.GetResponseStream()).ReadToEnd();
    }

In this function we send Data and then we get the response, so the mame must be changed to something like SendDataAndGetResponse.

Now we go back to the top, and we see that we change some things that we had written before, like instead of using a string[] we can use a List of the class itelf.(I'm tired from explaining everything, so I just post the final code here):

   using System;
   using System.Collections.Generic;
   using System.IO;
   using System.Net;
   using System.Threading.Tasks;

    namespace test
{
    internal class Program
    {
        private static void Main(string[] args)
        {
            ServicePointManager.DefaultConnectionLimit = 10000;
            string serverURL;
            List<TestClass> fileNames = new List<TestClass>();

            Console.WriteLine("Please enter the URL to send the XML File");
            serverURL = Console.ReadLine();

            string currentLine = "";
            while (currentLine != "start")
                fileNames.Add(new TestClass(currentLine));
            foreach (TestClass tClass in fileNames)
                tClass.SendDataAndGetResponseAsync();
            Console.ReadLine();
        }
    }

    internal class TestClass
    {
        private string xmlPath;

        public TestClass(string _xmlPath)
        {
            xmlPath = _xmlPath;
        }

        public void SendDataAndGetResponseAsync()
            => new Task(SendDataAndGetResponse).Start();

        public void SendDataAndGetResponse()
        {
            try
            {
                ServicePointManager.DefaultConnectionLimit = 10000;
                byte[] bytes = File.ReadAllBytes(xmlPath);
                HttpWebRequest request = CreateRequest(bytes.Length);
                using (Stream requestStream = request.GetRequestStream())
                    requestStream.Write(bytes, 0, bytes.Length);
                Console.WriteLine(ReceiveResponse(request));
            }
            catch (Exception ex)
            {
                Console.WriteLine(ex.Message);
            }
        }

        private HttpWebRequest CreateRequest(int length)
        {
            WebRequest request = WebRequest.Create(ATMSimulator.Properties.Settings.Default.KTHBaseUri);
            request.ContentType = "text/xml; encoding='utf-8'";
            request.ContentLength = length;
            request.Method = "POST";
            return (HttpWebRequest)request;
        }

           private string ReceiveResponse(HttpWebRequest request)
        {
            using (HttpWebResponse response = (HttpWebResponse)request.GetResponse())
                return new    StreamReader(response.GetResponseStream()).ReadToEnd();
            }
       }
   }

I hope this helps you :). I had some free time so I wrote the long answer.


Loose coupling, accessing class properties I'm trying to get a better understanding of decoupling methods. Right now, I have this method:

private bool ContainsLegalFirstName(DataRow row, string legalFirstNameColumn)
{
    return row.Table.Columns.Contains(legalFirstNameColumn)
        && !String.IsNullOrEmpty(row[legalFirstNameColumn].ToString());
}

I was thinking about changing the DataRow to an IDataRecord and adding an IListSource parameter and passing in the DataTable, but then the problem is I can't access the columns of the DataTable from the IListSource. Any suggestions or is what I have good enough?

As DataRow does not implement IDataRecord, you would have to pass an adapter that wraps the row and implements the interface. Also, passing an IListSource would only help if it always returned a ITypedList (which is not guaranteed) which you could then query for available properties by name. But what would be the advantage of all that?

In my opinion, the parameter types are alright. We are still talking about a private helper method, not one that needs to be accessible using all kinds of interfaces. There are, however, some things that I might change:

Either: Rename the method to represent that it actually does not care about the legal first name. You could currently rename it to ContainsLegalLastName without any change in behaviour.
Or: Remove the legalFirstNameColumn and get that value from somewhere else (some configuration, const, …).

Unless you decide to remove the column name parameter and get the column name from an instance variable, make the method static. It currently does not use any state at all.


Counting letter frequencies from an input file Out of pure curiosity, I decided to create a short F# script to calculate the letter frequencies based on the input file. Everything works smoothly and as I would expect. My questions regarding my code are as follows:

Does it conform to formal F# standards?

I have done my best to keep it as functional as possible while maintaining a style similar to the most common one I've seen in example programs, but I am unsure if it is up to par.
Does it maximize F#'s capabilities while maintaining readability?

This is as it seems, I am curious as to whether or not I have used all relevant and applicable F# functions inside the program (e.g. map). The portion I am most concerned about is this bit:

Array.map (fun z ->
               let e, f = z
               match count |> Array.tryFind (fun a -> let f,s = a
                                                      (e = f)) with
               | Some x -> (e, Convert.ToInt64(snd x) + f)
               | None -> z)

I am unsure if my approach is proper, that Array.tryFind bit is especially horrendous, but I can't think of a better way to approach this. In addition, instead of the tuple type, should I  instead go for records or is there some other, more appropriate, solution that I am missing?
Are there any ways to optimize or simplify the program I have written?

I have a decently powerful machine (i5 6600, 16GB RAM) and per FSI's #time directive, the program has the following footprint when processing ~10.1gb of text Real: 00:14:30.479, CPU: 00:14:21.421, GC gen0: 208720, gen1: 96, gen2: 8. I have had very little optimization experience whatsoever, so I am unsure if (by my calculations) 11.89mb/s is a good speed for such a relatively simple program.

#if INTERACTIVE
#time
#else
module LetterFreqCalc
#endif

open System
open System.IO

let explode (x:string) = [|for c in x -> c|]

let ToUpper (x:string) = x.ToUpper()

let numLetterInWord word =
    word
    |> ToUpper
    |> explode
    |> Array.countBy id

let sumTupArray (tupArr:(char*int64)[]) =
    tupArr
    |> Array.sumBy snd

let div (x:float) (y:float) = y/x

let mul (x:float) (y:float) = x*y

let rec CalcNextLine (reader:StreamReader) accumulator =
    match reader.EndOfStream with
    | true -> accumulator
    | false ->
               let line = reader.ReadLine()
               let count = numLetterInWord line
               accumulator
               |> Array.map (fun z ->
                                     let e, f = z
                                     match count |> Array.tryFind (fun a -> let f,s = a
                                                                            (e = f)) with
                                     | Some x -> (e, Convert.ToInt64(snd x) + f)
                                     | None -> z)
               |> CalcNextLine reader

let CalcLetterFreqOfFile (path:string) =
    let reader = new StreamReader(path)
    let start = [|for c in [|'A'..'Z'|] -> (c,0L)|]
    CalcNextLine reader start

[<EntryPoint>]
let main argv =
    let occurences = @"dictionary.txt"
                     |> CalcLetterFreqOfFile

    let total = occurences
                |> Array.sumBy snd
                |> float

    let freqs =  occurences
                 |> Array.map (fun x -> (fst x, (let percentage = snd x
                                                                  |> float
                                                                  |> div total
                                                                  |> mul 100.0
                                                 Math.Round(percentage, 2))))

    printfn "%A" freqs
    printfn "%A" (freqs |> Array.sortByDescending snd)
    printfn "%A" argv
    0 // return an integer exit code

Your explode function is almost just equivalent to Seq.toArray<char>. There's no need to have a ToUpper function just to call a method, especially if you're using it just once. And there is already a .ToCharArray() method anyway. I would recommend getting rid of both of those functions and using method calls instead: let numLetterInWord (word:string) = word.ToUpper().ToCharArray() |> Array.countBy id
Similarly, it seems like overkill to have div and mul functions just so they can be used in a pipeline. Their usage later on can be replaced by a single lambda: fun x -> x / total * 100.0
The sumTupArray isn't used and not really necessary. At one point you just do an inline Array.sumBy snd instead and that's fine.
Regarding the portion you're concerned about: Note that you can do pattern matching in function arguments just like you can in a let binding. So this pattern...

|> Array.map (fun z ->
     let e, f = z

... can be replaced with |> Array.map (fun (e, f) ->

The Array.tryFind can be replaced with Array.tryFind (fst >> (=) e) (using the function composition operator >> and the curried prefix version of =). If you don't like that you can still just use a lambda: Array.tryFind (fun (f,_) -> e = f)
F# has its own more concise version of Convert.ToInt64 called int64. Ironically, it's less type-safe than Convert.ToInt64 because it also tries to convert other things like strings and can throw an exception if they're not valid. I would still use it in this case since we know an int can always be converted to int64.
I think that most people don't use the style of indentation that you have used because it quickly uses of a lot of horizontal space. Instead of this, I would suggest starting a new line and indenting by four spaces. This can mean using a bit more vertical space, but it's worth it in my opinion.

With these suggestions applied, we end up with something a bit more concise and readable:

open System
open System.IO

let numLetterInWord (word:string) = word.ToUpper().ToCharArray() |> Array.countBy id

let rec CalcNextLine (reader:StreamReader) accumulator =
    match reader.EndOfStream with
    | true -> accumulator
    | false ->
        let line = reader.ReadLine()
        let count = numLetterInWord line
        accumulator
        |> Array.map (fun ((e, f) as z) ->
            match count |> Array.tryFind (fst >> (=) e) with
            | Some x -> (e, int64 (snd x) + f)
            | None -> z)
        |> CalcNextLine reader

let CalcLetterFreqOfFile (path:string) =
    let reader = new StreamReader(path)
    let start = [| for c in [|'A'..'Z'|] -> (c,0L) |]
    CalcNextLine reader start

[<EntryPoint>]
let main _ =
    let occurences = @"dictionary.txt" |> CalcLetterFreqOfFile

    let total = occurences |> Array.sumBy snd

    let freqs = occurences |> Array.map (fun (e, f) ->
        let percentage = float f / float total * 100.0
        (e, Math.Round(percentage, 2)))

    printfn "%A" freqs
    printfn "%A" (freqs |> Array.sortByDescending snd)
    0

I just realised your original time was 14 minutes, not 14 seconds! Which explains a lot. I found a solution that I think is a good balance of fast and simple:

open System
open System.IO

let countByIdBig xs =
    let counts = Collections.Generic.Dictionary<_,_>()
    for x in xs do
        match counts.TryGetValue x with
        | true, c -> counts.[x] <- c + 1L
        | false, _ -> counts.[x] <- 1L
    counts
    |> Seq.map (fun (KeyValue kv) -> kv)

let CalcLetterFreqOfFile (path:string) =
    seq {
        for line in File.ReadLines path do
        for c in line do
            if Char.IsLetter c then yield Char.ToUpper c }
    |> countByIdBig

The countByIdBig is similar to Seq.countBy id but it counts using int64 instead of int32. It uses a mutable dictionary for performance in a very contained way.

Then it's just a matter of getting a seq of characters to count and passing them to countByIdBig.


CSMR for large-scale text-prcessing I'm working on a project for large-scale text-processing, which is a first implementation of the basic idea of CSMR. CSMR is an algorithm that measures the similarity between documents by calculating their cosine in the vector space in parallel manner, using MapReduce model.

Here is my implementation on GitHub.

Some review on the code, the algorithm, corrections and maybe proposals for changes would be gratefully appreciated. Below are the 3 most important classes:

Mapper

public class CSMRMapper extends Mapper<Text,VectorWritable,IntWritable,
        DocumentWritable >{

    @Override
    public void map(Text key, VectorWritable value, Context context)
            throws IOException, InterruptedException{
            DocumentWritable p = new DocumentWritable(new Text(key.toString())
                    ,new VectorWritable(value.get()));
            context.write(new IntWritable(1),p);

    }
}

Reducer (Pair Generator)

public class CSMRReducer extends Reducer<IntWritable, DocumentWritable ,Text,
        VectorArrayWritable> {

    private ArrayList<DocumentWritable> al;
    private VectorWritable[] val;
    private VectorArrayWritable vaw;

    @Override
    public void reduce(IntWritable key, Iterable<DocumentWritable> values,
            Context context) throws IOException, InterruptedException{
        al = new ArrayList();
        vaw = new VectorArrayWritable();

        /* Storing each key-value pair (document) in a java.util.ArrayList */
        for (DocumentWritable v : values){
            al.add(new DocumentWritable(v.getKey(),v.getValue()));
        }

        /* Generating all the possible combinations of documents */
        if (al.size()>0){
            for (int i=0;i<al.size();++i){
                for (int j=i+1;j<al.size();++j){
                    val = new VectorWritable[2];

                    /* Generating the key for the current document pair with
                        the format "doci_name@docj_name" */
                    String k = al.get(i).getKey().toString()+
                            "@"+al.get(j).getKey().toString();

                    //First Document (doci)
                    val[0] = new VectorWritable(al.get(i).getValue().get());
                    //Second Document (docj)
                    val[1] = new VectorWritable(al.get(j).getValue().get());
                    vaw.set(val);

                    context.write(new Text(k), vaw);
                }
            }
        }
    }

}

The Final Reducer (Cosine Calculator)

public class CosineSimilarityReducer
    extends Reducer<Text, VectorArrayWritable, Text, DoubleWritable>{

    @Override
    public void reduce(Text key, Iterable<VectorArrayWritable> value, Context context)
    throws IOException, InterruptedException{

        CosineDistanceMeasure cdm = new CosineDistanceMeasure();
        VectorWritable docX,docY;
        double cosine;
        for (VectorArrayWritable v : value){
            docX = (VectorWritable)v.get()[0];
            docY = (VectorWritable)v.get()[1];
            cosine = cdm.distance(docX.get(), docY.get());
            context.write(key, new DoubleWritable(cosine));
        }

    }
}

public class CSMRReducer extends Reducer<IntWritable, DocumentWritable ,Text,
        VectorArrayWritable> {

    private ArrayList<DocumentWritable> al;
    private VectorWritable[] val;
    private VectorArrayWritable vaw;

    @Override
    public void reduce(IntWritable key, Iterable<DocumentWritable> values,
            Context context) throws IOException, InterruptedException{
        al = new ArrayList();
        vaw = new VectorArrayWritable();

        /* Storing each key-value pair (document) in a java.util.ArrayList */
        for (DocumentWritable v : values){
            al.add(new DocumentWritable(v.getKey(),v.getValue()));
        }

        /* Generating all the possible combinations of documents */
        if (al.size()>0){
            for (int i=0;i<al.size();++i){
                for (int j=i+1;j<al.size();++j){
                    val = new VectorWritable[2];

                    /* Generating the key for the current document pair with
                        the format "doci_name@docj_name" */
                    String k = al.get(i).getKey().toString()+
                            "@"+al.get(j).getKey().toString();

                    //First Document (doci)
                    val[0] = new VectorWritable(al.get(i).getValue().get());
                    //Second Document (docj)
                    val[1] = new VectorWritable(al.get(j).getValue().get());
                    vaw.set(val);

                    context.write(new Text(k), vaw);
                }
            }
        }
    }

}

You can remove the fields and just declare the variables in the function. There's no need for class variables here.

You can also remove the al.size()>0 check, because that's automatically checked by the for loop.

On the performance end, you could prepare al.get(i) and al.get(i).getKey().toString()+"@" before going into the j loop.


Is this C++11 seqlock implementation correct? My code: https://ideone.com/DZeIZv

#include <atomic>

template <class T> class Seqlock {
    std::atomic<int> seq_;
    T val_;

public:
    Seqlock(T value = T())
        : val_(value) {
    }

    // concurrent calls are NOT allowed
    void store(T value) {
        const int seq0 = seq_.load(std::memory_order_relaxed);
        seq_.store(seq0 + 1, std::memory_order_relaxed);

        std::atomic_thread_fence(std::memory_order_release);

        val_ = value;
        std::atomic_thread_fence(std::memory_order_release);

        seq_.store(seq0 + 2, std::memory_order_relaxed);
    }

    // concurrent calls are allowed
    T load() const {
        for (;;) {
            const int seq0 = seq_.load(std::memory_order_relaxed);
            if (seq0 & 1) {
                // cpu_relax()
                continue;
            }

            std::atomic_thread_fence(std::memory_order_acquire);

            T ret = val_;
            std::atomic_thread_fence(std::memory_order_acquire);

            const int seq1 = seq_.load(std::memory_order_relaxed);
            if (seq0 == seq1) {
                return ret;
            }
        }
    }
};

Is this seqlock implementation correct across all architectures, at least when T is an integral type? Can it be improved?

References I was following:

https://elixir.bootlin.com/linux/latest/source/include/linux/seqlock.h
https://www.hpl.hp.com/techreports/2012/HPL-2012-68.pdf
https://github.com/rigtorp/Seqlock/

My guideline on memory orders is "Just say no"; I guarantee that if you're using them, you're using them wrong. :)  So I won't attempt to find the exact bug, I'll just assume they all say seq_cst. The only thing I'll say about your memory orders is, when you write

    seq_.store(seq0 + 1, std::memory_order_relaxed);
    std::atomic_thread_fence(std::memory_order_release);

can you explain how that's different from

    seq_.store(seq0 + 1, std::memory_order_release);

?

Seqlock(T value = T())

This constructor should be explicit; otherwise you're accidentally permitting

Seqlock<int> s = 42;

and in fact because of C++17 CTAD you're also permitting

Seqlock s = 42;

Rule of thumb: make all constructors explicit except for those you want the compiler to be able to call implicitly (i.e., copy and move constructors).

In T load() const, you load seq1 at the bottom of the loop, and then go around again and immediately load seq0 from the same location. You could just have set seq0 = seq1; i.e.

T load() const {
    int seq1 = seq_.load(std::memory_order_relaxed);
    while (true) {
        int seq0 = seq1;
        if (seq0 & 1) {
            // cpu_relax();
        } else {
            std::atomic_thread_fence(std::memory_order_acquire);
            T ret = val_;
            std::atomic_thread_fence(std::memory_order_acquire);
            seq1 = seq_.load(std::memory_order_relaxed);
            if (seq0 == seq1) {
                return ret;
            }
        }
    }
}


Tennis-Refactoring-Kata through OOP I was trying to solve Tennis-Refactoring-Kata. This seems quite hard at startup but, through use os OOP it can be deeply refactoring using just a single if.

My first understanding was about the fact that TennisGame (the main class) has a Score. Or at least should have one.

Furthermore score possibilities are just twenty (Love-All, Fifteen-All and so on...) and each one leads further Scores based on who is the last player who scores a point (e.g.: Love-All leads to Fifteen-Love if last point has been scored by first player, Love-Fifteen otherwise). From this observation I wrote 20 classes like this:

public class LoveAll implements IScore {

    public IScore Player1Scored() {
        return new FifteenLove();
    }

    public IScore Player2Scored() {
        return new LoveFifteen();
    }

    public String Label() {
        return "Love-All";
    }
}

This should seems a loss of time but this is not bad as it seems since each class is simple and concise. This give me the possibilities to write a TennisGame4 like this:

public class TennisGame4 implements TennisGame {
    private IScore score = null; // Composition here - TennisGame has a Score
    private String player1Name;
    private String player2Name;

    public TennisGame4(String player1Name, String player2Name) {
        this.player1Name = player1Name;
        this.player2Name = player2Name;
        score = new LoveAll();
    }

    public void wonPoint(String playerName) {
        if (playerName.equals("player1"))
            score = score.Player1Scored();
        else
            score = score.Player2Scored();
    }

    public String getScore() {
        return score.Label();
    }
}

Just a single if to distinguish who is the last player who score a point.
What do you think about this solution? Does this solution respect the kata constraint?

Consider that:

Code surely can be enhanced (Player should be an object too);
To stay in the Kata I have do everything in 90 minutes or so;

As you say, there are only a limited possibilities for a Score, which I would call GameScore to be more precise. This calls for an enum.

public enum GameScore {
    S0000(0, 0), S1500(15, 0), S3000(30, 0), ...;

    private static final GameScore[] SWIN = { S1500, ... };
    private static final GameScore[] RWIN = { S0015, ... };
    private static final String[] NAME = { "Love-All", ... };

    ...

    public GameScore serverWins() {
        return SWIN[ordinal()];
    }

    public GameScore returnerWins() {
        return RWIN[ordinal()];
    }

    public String getDisplayText() {
        return NAME[ordinal()];
    }
}

That's all. All in one class, ready for testing.

By the way, in Java all method names start with a lowercase letter, not an uppercase. (That would be C# or Ada.)

Using an enum and predefined string literals has the benefit of generating zero memory allocations at runtime.


Creating a minimax tree recursively I am currently working on creating a tree (this is my learning process of creating a minimax tree and am starting towards minimax tree from here) with multiple nodes recursively.

Can anyone please determine if the algorithm is correct? From my perspective and couple of test (using print, count and gdb), I think I am creating the tree that I want, which looks like this:

The code for this tree:

#include <iostream>
#include <vector>
#define SIZE 3 //  this is number of child nodes a node will have

using namespace std;

struct Node{

    std::vector<Node*> v_node;
    std::vector<int> my_vec;
    int val;
};

class Tree{

    Node *root;
    int size;
    int count;

    void create_tree(Node *n, int size){
        if(size == 0) return;

        count++;
        for(int i = 0; i < SIZE; i++){
            Node * node = new Node;
            node->v_node.resize(SIZE, nullptr);
            node->my_vec.resize((size-1), n->my_vec[0]-1); // decrease val of vector by 1 in each level
            node->val   = i;
            n->v_node[i] = node;
            create_tree(node, size-1); // recursively call
        }
    }

public:

    Tree(int s){
        size = s;
        root = new Node;
        root->v_node.resize(SIZE, nullptr);
        root->my_vec.resize(s, 36);
        root->val = 0;
        count = 0;
    }
    void ct(){
        create_tree(root, size);
    }
    int get_node_count(){
        return count;
    }
};

int main(){

    Tree t(4); //
    t.ct();

    std::cout << "Node count = " << t.get_node_count() << std::endl;

}

My core questions are:

Do you think using for loop inside recursive function a good idea?
I know that recursion have limits. When I do ulimit -s it gives me around 8000. This means that if I am using minimax for complex games like chess this might cause stack overflow. This gives me hint that this algorithm that I wrote is not good. Can you comment on this?
I know I need to do the delete for releasing the resource, and for that I am planning to use unique_pointer later.

Here are some things that, in addition to the other review, may help you improve your code.

Don't abuse using namespace std

Putting using namespace std at the top of every program is a bad habit that you'd do well to avoid.  In this program, especially, it's completely simply to eliminate because the namespace is already explicitly specified every place it's needed.

Prefer constexpr to old-style #define

Rather than using a #define for SIZE the code could use a constexpr:

constexpr size_t SIZE{3}; //  this is number of child nodes a node will have

It doesn't make a huge difference here, but generally the advantage is that the value has an associated type.

Use appropriate data structures

The code implies that there are only and exactly 3 children nodes for each node, but the data structure is a std::vector<Node*> with a #define SIZE 3.  This means that the size is set at compile time, but that there is no explicit prohibition to having 2 or 5 or more children.  Better would be to define the variable as std::array<Node*, 3> v_node;.  This would also eliminate both of the resize calls associated with the v_node variable.

Be careful with signed versus unsigned

The code currently contains this line:

    for(int i = 0; i < SIZE; i++){

But would it ever make sense for either i or SIZE to be negative?  Probably not, in my view, which means that it would likely make more sense, considering how they're used, to have both declared as size_t types

Prefer modern initializers for constructors

The constructor could use the more modern initializer style rather than the old style you're currently using.  Instead of this:

Tree(int s){
    size = s;
    root = new Node;
    root->my_vec.resize(s, 36);
    root->val = 0;
    count = 0;
}

You could write this:

Tree(int s) :
    root{new Node},
    size{s},
    count{0}
{
    root->my_vec.resize(s, 36);
    root->val = 0;
}

This could be even more simplified by implementing the following suggestion.

Rely on constructors to always create valid objects

By defining constructors for classes such as Node, you can assure that any Node object is actually valid; that is, it has a consistent internal state, according to whatever that might mean for your code.  In this particular case, defining a constructor for Node makes much of the rest of the code much simpler.  For example, we might move from a struct to a class definition like this:

class Tree;

class Node{
    std::array<Node*, 3> v_node;
    std::vector<int> my_vec;
    int val;
public:
    friend class Tree;
    Node(size_t count, size_t vecsize, int myval) :
        v_node{},
        my_vec(count, vecsize),
        val{myval}
    {}
};

Now the Tree constructor could be this:

Tree(int s) :
    root{new Node{s, 36, 0}},
    size{s},
    count{0}
{ }

And the loop within create_tree reduces to just a few lines:

for(size_t i = 0; i < SIZE; i++){
    n->v_node[i] = new Node{size-1, n->my_vec[0]-1, i};
    create_tree(n->v_node[i], size-1);
}

This same general principle also means that it makes little sense to have separate constructor and initialization calls.  So instead of this in main:

Tree t(4);
t.ct();

It should just be this:

Tree t(4);

And the Tree constructor would be this:

Tree(int s) :
    root{new Node{s, 36, 0}},
    size{s},
    count{0}
{
    create_tree(root, size);
}

The ct function would simply be eliminated.

Avoid memory leaks

You've already mentioned this in your problem statement, but for the benefit of other readers, the current version of the program leaks memory.  Two approaches would be to either add a destructor to each of the classes, or better, to convert from raw pointers to std::shared_ptr.

Algorithm correctness

Because the code is not really complete, (that is, the tree contains no meaningful data), it's difficult to say.  It constructs some tree, but whether it's a minimax tree is difficult to say outside of the context in which it will be used.


Moving MP3 files from one directory to another using regex This is a simple script for moving my MP3 files from one directory to another. I'd like to know if there is a more efficient way to handle my task in the script. I analyzed the algorithm to be \$O(n^2)\$.

import os
import shutil
import re

files = os.listdir(os.path.dirname('/home/rich/Downloads/'))

regex = r"(.mp3)"

for f in files:
    if (re.search(regex, f)):
        f = os.path.join('/home/rich/Downloads/', f)
        shutil.move(f, '/home/rich/Music/Songs/')

Constants like '/home/rich/Downloads/' could be made into variables, for example:

src_dir = '/home/rich/Downloads/'
dest_dir = '/home/rich/Music/Songs/'

This would make it clear what they mean, and would avoid duplication if you need to use a constant more than once.
There are three problem in the regular expression.

First, the parentheses are unnecessary — they capture whatever they match for future use, but there's no future use here.

Second, the dot has a special meaning in regular expressions — it matches any character. If you want to match a literal dot, then you need to escape it.

Third, the regular expression is not anchored and that means it can match anywhere in the string. For example, it will match the filename "not.actually.an.mp3.wav".

The regular expression should be r"\.mp3$".
There's no need for a regular expression at all, you could just call os.path.splitext to get the extension:

for f in files:
    _, ext = os.path.splitext(f)
    if ext == '.mp3':
        shutil.move(f, dest_dir)

In fact there's no need to do that, because you could use glob.glob instead:

for f in glob.glob(os.path.join(src_dir, '*.mp3')):
    shutil.move(f, dest_dir)

It seems like overkill to use Python for this, because it would be a one-liner in shell:

mv ~/Downloads/*.mp3 ~/Music/Songs/


JavaScript app to search GitHub using JSON API I've written this simple JavaScript application for a homework assignment and I've received feedback saying it could be written better.

Can I get some feedback on how to write this differently?  It works for me and returns the results I need, but I would like some more specific feedback. It was meant to be simple and not meant to require more than 2 hours on it.

The requirements were...

  You can only edit app.js, you cannot touch index.html. jQuery is provided.
  index.html contains two elements: one for the search term and one for the results.
  The results should show as a list with each item in an "owner/name" format.
  When a result is clicked, display an alert with the repo's language, followers, url and description.
  The search term should be cached so duplicate searches do not trigger further requests.
  Solution does not need to support older browsers.

Here are the app.js I submitted and the index.html.

(function(window){
 // Setup the variables for the application
 var search,
 results,
 textFieldValue,
 requestBaseUrl = 'https://api.github.com/legacy/repos/search/';

 // Enable browser caching for the application
 $.ajaxSetup({ cache: true});

 function applyPageStyling(){
  $( "" ).appendTo( "head" );
  setTimeout(function(){alertInstructions();},1000);
 };

 // Alert the user what to do
 function alertInstructions(){
  alert("Please type the term to search GitHub for in the text field and hit the ENTER key.");
 };

 // Function to query the github API
 function gitSearch(){
  // Get the value from the search field
  textFieldValue = $('#search').val();

  // Error handling for search box
  if (textFieldValue == ""){
   alert("Please enter a term to search for");
  }
  else {
   // Make the ajax call with the text field value appended to the url. The callback will
   // output the results or alert the error message
   $("
").appendTo('#results');
   $.getJSON(requestBaseUrl+textFieldValue, function(json, status){})
    // Success then append items to #results div
    .done(function(json){
     $.each(json.repositories, function(i, repositories){
      $("
"+json.repositories[i].owner+" / "+json.repositories[i].name+"
")
       .appendTo('#results ul')
       // Add a little styling, just a little.
       .css({'cursor': 'pointer','list-style-type':'none', 'width':'500px'})
       .hover(
        function(event) {
         $(this).css({'color':'#ffffff'});
        },
        function(event) {
         $(this).css({'color':'#000000'});
        }
       )
       .click(function(event){
        alert("Repo Lanuage is : "+json.repositories[i].language +"\n"+
        "There are "+ json.repositories[i].followers +" followers of this repo.\n"+
        "The URL of the repo is : "+ json.repositories[i].url +"\n"+
        "The description of the repo : "+ json.repositories[i].description);
       });
     });
   })
   // Log the error message
   .fail(function(status){
    console.log(status);
   });
  }
 };

 // Enter key event handler
 $(document).keypress(function(e) {
  if(e.which == 13) {
   gitSearch();
  }
 });
 applyPageStyling();

}(window));
<!DOCTYPE html>

  <meta http-equiv="Content-type" content="text/html; charset=utf-8">
  <title>Search</title>

  <input id="search" type="text"/>

Looks good to me. You could cache the ul:

var repoList = $("
").appendTo('#results');

So you don´t have to query the dom again when appending the list items.
Also it is better to build the list items in memory and only append the complete list to the dom. This way you only trigger one reflow.

So in the $.each loop I would

repoList.append($("
somerepo
"))

and then outside the loop

$("#results").html(repoList);


How to speed up ordering of output file I have a process where I take several .lst flatfiles, apply filters and reformat the data, then append the results to a single file. I loop through every file in a subdirectory, applying the reformatting, then I append it to a file called Myoutput. I also strip out some rows of the dataframe and write this to a different .lst called Myoutput2.

My code:

fileNames = next(os.walk('subdirectory'))[2]

for filename in fileNames:
    headers = ['My', 'list', 'of', 'headers']
    columns = [0,2,3,4]
    filePath = r'subdirectory\\' + filename

    df = pd.read_csv(filePath, sep='\t', names=headers, usecols=columns)

    #My reformatting code...

    with open('output/Myoutput_{}_{}_{}.lst'.format(year,month,day), 'a') as f:
        df.to_csv(f, header=False, index=False, sep='\t')

    df = df.loc[df['Type']!='UNKNOWN'] #removes UNKNOWN types for paragon

    paragonCount += len(df)
    with open('output/Myoutput2_{}_{}_{}.lst'.format(year,month,day), 'a') as f:
        df.to_csv(f, header=False, index=False, sep='\t')

    os.remove('download/'+filename)

This process has worked so far but I now have to change it so that the two output files are ordered by a datetime field in the files. All the datetime values in the field are on the same day. The input files are not chunked by datetime, so it wouldn't be enough to just change the order I load the files.

I appended this code to the end:

df = pd.read_csv('output/Myoutput_{}_{}_{}.lst'.format(year,month,day),header=None,sep='\t')
df = df.sort_values(by=1)

df.to_csv('output/Myoutput_{}_{}_{}_ordered.lst'.format(year,month,day),index=None,headers=None)

df = pd.read_csv('output/Myoutput2_{}_{}_{}.lst'.format(year,month,day),header=None,sep='\t')
df = df.sort_values(by=1)

df.to_csv('output/Myoutput2_{}_{}_{}_ordered.lst'.format(year,month,day),index=None,headers=None)

The output files tend to be around 1.5GB, so this doubles the runtime of my script, and it feels inefficient re-loading the data into memory. Is there any way to speed this up, perhaps have the loop appending the results based on their datetime field instead of on the end of the file?

With the output DataFrames being about 1.5Gb in file, the reason your performance is slow may be that you have insufficient memory to perform these operations quickly.

One way to reduce the amount of memory used is to is to use the inplace keyword argument common to some of the pandas.DataFrame methods. Setting inplace=True, when available, makes the operation augment the DataFrame in place; otherwise, inplace=False will create a copy (which can consume more memory).

One quick suggestion, therefore, might be to replace:
df = df.sort_values(by=1)
with df.sort_values(by=1, inplace=True).

This answer on StackOverflow may also be of interest (it talks about different optimisations for holding data in DataFrames; including inplace): https://stackoverflow.com/a/39377643/6852391


Computing the Mean Average Precision I've read this article about Mean Average Precision (MAP).

Now, in my C++ code I have an std::vector<std::string> queries, where queries[i] is the identifier of the i-th query.

In addition, I have a std::vector<std::vector<std::string> truePositives where truePositives[i][z] is the z-th true positive correspondent to the i-th query. Since the actual order in truePositives[i] isn't important, I could have used std::vector<std::set<std::string>>, but whatever...

Finally, I have a std::vector<std::string> topkTest where topkTest[j] is the j-th element in the top-k ranked list returned by the system for the given query, where k = min(data set size, 10k) (following the suggestion in this question).

Here you can find my code to compute the MAP. I post it here because there is no actual way to say if the returned result (the map value) is correct or not.

    float map = 0;
    for(size_t i=0; i<queries.size(); i++){
        std::vector<std::string> topkTest;
        //populate topkTest somehow using k
        float correct = 0;
        float ap = 0;
        for(size_t j=0; j<topkTest.size(); j++){
            //if topkTest[j] belongs to the true positives, increment the number of correct images
            if(std::find(truePositives.begin(), truePositives.end(), topkTest[j]) != queries.end())
                ap += ++correct / (j+1);
        map += ap / topkTest.size();
    }
    map /= queries.size(),

What do you think about this?

The equation for computing the mean average precision (MAP) is shown below:

$$
\text{MAP}(Q) = \frac{1}{\lvert Q \rvert} \sum_{j=1}^{\lvert Q \rvert}\frac{1}{m_j} \sum_{k=1}^{m_j} \text{Precision}(R_{jk})
$$

The inner summation is something that we need to focus on. The outer summation is simply an average over the queries of the inner sum. The inner summation calculates point-wise precision values at the recall points. A recall point is defined as a position in the ranked list which retrieves a relevant document. Note that this quantity is divided by the total number of relevant documents for the jth query denoted by \$m_j\$, which adds the recall factor in an otherwise precision oriented metric.

Coming back to your code, you should thus change the following line

map += ap / topkTest.size();

to

map += ap / truePositives.size();

The rest of the code is okay.


Token generation system in py3.7 I've created a system to generate tokens that could potentially be used in some sort of program:

# _gen.py is simply to generate the tokens, not to check them.

import string, secrets, random

# String is for getting all ascii letters and digits.
# Secrets is for a cryptographically secure function to choose chars.
# Random is providing a random number generator.

# Base functions used to generate a token

def _tknFrag(length: int=32):
    """Generates a tknFrag, or Token Fragment."""
    return ''.join(secrets.choice(string.ascii_letters + string.digits) for _ in range(length))

def _tknFragLen():
    """Returns the tknFrag's tknFragLen, or Token Fragment Length, an int in between 30 and 34"""
    return int(random.randint(30, 34))

def _insDot(tknFrag: str):
    """Inserts a dot in the tknFrag at the end"""
    tknFrag += "."
    return tknFrag

def _insDash(tknFrag: str):
    """Inserts a dash in the tknFrag at the end"""
    tknFrag += "-"
    return tknFrag

def _ins(tknFrag: str):
    """Inserts a dash or a dot inside of the tknFrag"""
    return {
        "0" : lambda frag: _insDot(tknFrag=frag),
        "1" : lambda frag: _insDash(tknFrag=frag)
    }[str(random.randint(0, 1))](tknFrag)

# Now on to actually generating the tokens

# This is one last helper function, completely generating a fragment
def tokenFragment(repetition):
    """Generates a fragment with the random length, and the dot/dash. The repetition is to determine wether or not to actually put the dot/dash at the end of the token."""
    return _ins(_tknFrag(length=_tknFragLen())) if repetition != 2 else _tknFrag(length=_tknFragLen())

def token():
    """Generates a three-fragment-long token"""
    return "".join([tokenFragment(repetition) for repetition in range(3)])

# Gotta test it somehow

if __name__ == '__main__':
    print(token())

Here are five results from running the code:

2dD0ZYjV4AOGMzZ2lIg6wVPzwpB82Z.BsX34AaWpvJ7i6jtWzYI1zNRjz2pI0.GRW9Uhfl9P8Xu7pYzoSYutEwuhqjhQnN

AghhjknBMqgKLpD6rR90iXKj2yuT44B.CAyXZJtAE62L97SZUbadGBmXTTSflC9th.RbuKtUDWxQ9ROgwo2OkKYEozHc1ToRz8Q

5sFE1npNyEA7JCz9hHEJFwmP2aX4CY3p.RMiKkilzUp7kmJCigrB6HfOVPkWmsczJ-4DF3qLuQSgdwmRxiOMBLUw1ZLj1Al7n

6FCXlDGWjAKylt5rZFYxOfLecxyyL4Pj-0MGu0B2knedbR8HnFI16gHChWZ8uldQ.chCVuFycHPJXF2tj9wR3mi4W4yuRp3o

rEAi9Rn1lXNkMett8wHXLF0iJLQHbc6lI.ksViH35sOlIwFfvOjVHlyXfMS0Ye58diZB-jEX93wyHTRznkYUJvbADmOrdZZF2D0R0

Note that the dots and dashes are for looks only and serve no purpose.

Well, in the first place I have a question as to why you would need this specific pattern of tokens; it seems for a very specific use case.

Secondly, you use a pretty consistent naming convention in your code (which is very good), but the shortened names and abbreviations make it difficult for a person to understand your code quickly (in my opinion).

Lastly, if you are  looking for less code and more speed with the same functionality, I can propose this:

import random
import secrets
import string

FRAGMENT_ALPHABET = string.ascii_letters + string.digits
FRAGMENT_SEPARATOR_OPTIONS = ('.', '-')
FRAGMENT_MIN_LENGTH = 30
FRAGMENT_MAX_LENGTH = 34

def _generate_fragment(length: int, prepend_separator: bool):
    prefix = ''
    if prepend_separator:
        prefix = secrets.choice(FRAGMENT_SEPARATOR_OPTIONS)

    s = ''.join(
        secrets.choice(FRAGMENT_ALPHABET)
        for _ in range(length))

    return prefix + s

def token(num_parts: int = 3):
    return ''.join(
        _generate_fragment(
            length=random.randint(FRAGMENT_MIN_LENGTH, FRAGMENT_MAX_LENGTH),
            prepend_separator=i > 0)          # only False for the first fragment
        for i in range(num_parts))

In this code:

In my opinion (opinions can be very subjective) the code is still readable, but has less functions and those have explicit long names, which helps users to understand it faster.
I also put some general parameters as uppercase module variables at the top of the file; this makes it easier to modify and (in my opinion) also easier to read/understand in the code.
The execution time is less. Comparing the two versions using timeit like this for both versions (with Ubuntu and Python3.6):

>>> timeit.timeit('token()', 'from __main__ import token', number=10000)

I get these results, showing my version as faster using about 8 % less time:

Version    Time      Time comparisson
yours      2.222258   100.0 %
mine       2.060732    92.7 %


Managing web.config appSettings I wanted a pattern to manage web.config appsettings. I want to be able to have a default setting if the config setting was missing. I also wanted the check for the value in the config file to happen only once, not each time the the value is retrieved.

Public NotInheritable Class ExternalReportRequestBL

    ''' <summary>
    ''' The default value for max number of request attempts
    ''' </summary>
    ''' <remarks></remarks>
    Private Shared _ReportRequestMaxExecuteAttemptDefault As Integer

    Shared Sub New()
        If Not Integer.TryParse(ConfigurationManager.AppSettings("ReportRequestMaxExecuteAttempt"), ExternalReportRequestBL._ReportRequestMaxExecuteAttemptDefault) Then
            _ReportRequestMaxExecuteAttemptDefault = 10
        End If
    End Sub

    Friend Shared ReadOnly Property MaxNumberOfTries As Integer
        Get
            Return ExternalReportRequestBL._ReportRequestMaxExecuteAttemptDefault
        End Get
    End Property
End Class

Now, I can access the variable from anywhere in the assembly:

ExternalReportRequestBL.MaxNumberOfTries

What are your thoughts?

Integer.TryParse will overwrite the value of that shared property with 0 if the value is not there. Use something like this instead:

Shared Sub New()
    Dim parsed As Integer
    If Integer.TryParse(ConfigurationManager.AppSettings("ReportRequestMaxExecuteAttempt"), parsed) Then
        _ReportRequestMaxExecuteAttemptDefault = parsed
    End If
End Sub


Scala: drop elements in Sequence Can I reduce the loop inside my method with map or is there a shorter implementation of this code that's dropping elements in sequence?

// Method that drops one element in sequence
def drop_Val_In_List[String](ls: Seq[String], value: String):  Seq[String] = {
    val index = ls.indexOf(value)  //index is -1 if there is no matc
    if (index < 0) {
        ls
    } else if (index == 0) {
        ls.tail
    } else {
        // splitAt keeps the matching element in the second group
        val (a, b) = ls.splitAt(index)
        a ++ b.tail
    }
}

val KeepCols = Seq("id", "type", "month", "car", "road")

// Generalization of the above method to drop multiple elements
def drop_Val_List_In_List[String](ls: Seq[String], in_ls: Seq[String]): Seq[String] = {
    var tmp_ = ls
    //println(tmp.getClass)
    for(x <- in_ls ){ // should work without var x
        tmp_ = drop_Val_In_List(tmp_, x)
    }
    tmp_;
}

val tmp = drop_Val_List_In_List(KeepCols, List("id", "type", "month"))
```

Follow the naming convention of the language
For scala, this is here. Spell methods and variables in lower camel case:
drop_Val_In_List
drop_Val_List_In_List
in_ls

Use the most appropriate data structure
To me, it seems like you want a set, not a sequence/list. Set also has the method you want:
val KeepCols = Set("id", "type", "month", "car", "road")
val tmp = KeepCols.diff(Set("id", "type", "month"))

If you don't want to use a set, Seq also has diff.


JS arrays to simplify jQuery filtering Okay, I'm under the impression that my current jQuery can be simplified (don't worry! It's short!) and I'm curious to know how this can be achieved. I think a solution to this will help produce a lot less code.

I currently have a list menu set up, with class names referring to their titles (HTML below) and if you click a list item, the menu item slides out and it's relevant content fades in. If you click the main heading again, the content fades out and the menu item slides back in.

Here's an example of my HTML

Corporate

Commercial

Construction

Corporate

Lorem ipsum lorem ipsum lorem ipsum

Commercial

Lorem ipsum lorem ipsum lorem ipsum

Here's some example JS:

jQuery('.wwd-choices-container ul.wwd-filters li a.corporate').click(function () {
    jQuery('.wwd-content-container .wwd-content-each').fadeOut();
    jQuery('.wwd-content-container .wwd-content-each.corporate').fadeIn();
});

My problem is that I don't want to have to repeat the jQuery code for each list item and it's content (e.g. li a.corporate, li a.commercial, li a.li a.corporate). As the class names for both the list item anchor and each content, is there an easy way to simplify this?

Everything is built via a CMS, so new sections and menu items would be added as time goes on so it has to remain as dynamic as possible. I'm thinking some form of array would work... and if it matches each other then it connections.

I would recommend using the .on() for click event delegation:

$('.wwd-filters').on('click', 'a', function(e) {
    e.preventDefault();
    var id = $(this).data('id');
    jQuery('.wwd-content-container .wwd-content-each').fadeOut();
    jQuery('.wwd-content-container .wwd-content-each.' + id).fadeIn();
});

This implementation grabs the id of the section you wish to show via the data attribute in the a elements:

Corporate

Commercial

Construction


Entity Framework - updating two different arrays of child associations Exercises have Muscles and Tools.  When someone adds or updates an Exercise, they also send an array of Tools and Muscles.  Adds are pretty straightforward, but my update method feels a bit bulky.  How can I simplify this?

public void UpdateExercise(Exercise exercise) {
            var dbExercise = db.Exercises.First(s => s.ExerciseId == exercise.ExerciseId);
            var dbTools = db.Tools.ToList();
            var dbMuscles = db.Muscles.ToList();

            dbExercise.Title = exercise.Title;

            // handle tools
            var existingTools = dbExercise.Tools.ToList();
            foreach (var newTool in exercise.Tools) {
                // handle pre-existing
                var match = existingTools.FirstOrDefault(s => s.ToolId == newTool.ToolId);
                if (match != null) {
                    existingTools.Remove(match);
                }
                else {
                    // add newTool to dbExercise
                    dbExercise.Tools.Add(dbTools.First(s => s.ToolId == newTool.ToolId));
                }
            }
            foreach (var existingTool in existingTools) {
                dbExercise.Tools.Remove(dbTools.First(s => s.ToolId == existingTool.ToolId));
            }

            // handle exercises
            var existingMuscles = dbExercise.Muscles.ToList();
            foreach (var newMuscle in exercise.Muscles) {
                // handle pre-existing
                var match = existingMuscles.FirstOrDefault(s => s.MuscleId == newMuscle.MuscleId);
                if (match != null) {
                    existingMuscles.Remove(match);
                } else {
                    // add newMuscle to dbExercise
                    dbExercise.Muscles.Add(dbMuscles.First(s => s.MuscleId == newMuscle.MuscleId));
                }
            }
            foreach (var existingMuscle in existingMuscles) {
                dbExercise.Muscles.Remove(dbMuscles.First(s => s.MuscleId == existingMuscle.MuscleId));
            }

            db.ChangeTracker.Entries<Tool>().ToList().ForEach(p => p.State = EntityState.Unchanged);
            db.ChangeTracker.Entries<Muscle>().ToList().ForEach(p => p.State = EntityState.Unchanged);

            db.SaveChanges();
        }

Exercise:

public class Exercise {

        public Exercise() {
            Tools = new Collection<Tool>();
            Muscles = new Collection<Muscle>();
        }

        [Key]
        [Required]
        public int ExerciseId { get; set; }

        [StringLength(300, ErrorMessage = "The value cannot exceed 300 characters. ")]
        public string Title { get; set; }

        public virtual ICollection<Tool> Tools { get; set; }
        public virtual ICollection<Muscle> Muscles { get; set; }

    }

Use design patterns to simplfiy your Data Access Layer and the Separation of concerns.

This method UpdateExercise(Exercise exercise) is very complicated and is not SRP.

Your problem in the model architecture more than in code. Of course you can use some tricks to simplify the code syntax like LINQ queries but the core problem will still there!

I recommeded you to apply Repository pattern and UnitOfwork pattern then you will see how it easy can you do all wished changes in one Transaction.
Use eager loading to inclued the wished entities(Tools,Muscles, etc.)  and load all load all everywhere
Use Linq to simplify the queries
Use attached entities, if possible!! slower but better!


Implementation of DDA Line Algorithm I have tested all the cases of how a line could be

vertically
horizontally
has a slope that's positive or less than 1

The function works, but I would like it reviewed, such as if there are overflows, etc.

// Draw line using DDA Algorithm
void Graphics::DrawLine( int x1, int y1, int x2, int y2, Color&color )
{

    float xdiff = x1-x2;
    float ydiff = y1-y2;
    int slope  = 1;
    if ( y1  == y2  )
    {
        slope = 0;
    }
    else if (  x1 == x2 )
    {
        slope = 2; // vertical lines have no slopes...
    }
    else
    {
        slope = (int)xdiff/ydiff;
    }

    if ( slope <= 1 )
    {
        int startx = 0;
        int endx   = 0;
        if ( x1 > x2 )
        {
            startx = x2;
            endx   = x1;
        }
        else
        {
            startx = x1;
            endx   = x2;
        }

        float y = y1; // initial value
        for(int x = startx; x <= endx; x++)
        {
            y += slope;
            DrawPixel(x, (int)abs(y), color);
        }
    }

    else if ( slope > 1 )
    {
        float x = x1; // initial value
        for(int y = y1;y <= y2; y++)
        {
            x += 1/slope;
            DrawPixel((int)x, y, color);
        }

    }

}

Here are some flaws in the current algorithm.

xdiff will have a roundoff error if x1-x2 is large in magnitude, likewise for the y variables.
slope is set to 1 for no reason, and then immediately reinitialized to something else.
slope is restricted to an integer, when most slopes will not be integral.
It's impossible to justify setting the slope of a vertical line to 2. There are slopes greater than 2 that are not vertical lines.
In the line slope = (int)xdiff/ydiff;, casting is higher precedence than division, so this will first cast xdiff to an int, overflowing if xdiff > MAXINT, throwing away the fractional part if there is one, then dividing this int by ydiff. The odds of this being the actual slope are very small.
startx and endx are initialized to 0 for no reason, then immediately reinitialized to somethiing else.
Roundoff errors will accumulate as you repeatedly add slope to y (or 1/slope to x).
What's the justification for the abs(y)? If y changes sign in the range, this will cause a kink in the line. Similarly for x.


Return a string without the first two characters I'm new to Java and am trying to solve a scenario.  Even though I've succeeded and it works, I've just wondered if there was a more practical way of doing this task.  Is there a quicker or more efficient way of solving the task, or is this fine?

  Task:

  Given a string, return a version without the first 2 chars. Except
  keep the first char if it is 'a' and keep the second char if it is
  'b'. The string may be any length.

deFront("Hello") → "llo"
deFront("java") → "va"
deFront("away") → "aay"

Code:

public String deFront(String str) {
  if(str.length() > 0) {
    if(str.substring(0,1).equals("a") && !str.substring(1,2).equals("b")) {
      return str.substring(0,1) + str.substring(2,str.length());
    } else if(str.substring(1,2).equals("b") && !str.substring(0,1).equals("a")) {
      return str.substring(1,2) + str.substring(2,str.length());
    } else if(str.substring(0,1).equals("a") && str.substring(1,2).equals("b")) {
      return str;
    } else {
      return str.substring(2,str.length());
    }
  } else {
    return "";
  }
}

Yes, some of the things you are doing is redundant and quite messy. As this seems to be a homework assignment I will only provide hints for you here.

What you can do is:

If the string is empty, return an empty string immediately
Store the first character in a variable
Store the second character in a variable, if it exists
Store the rest of the string in a variable, if it exists
Check if the second character is 'b', if it is then add it to the beginning of the string
Check if the first character is 'a', if it is then add it to the beginning of the string
Return the string

A problem of your existing code is that it will fail for strings of length 1. So it is important to only use the first and second characters if they exist at all.


Is this an excessive abstraction using objects? We have two physical AWS S3s called in the code s3Legacy and s3Cache. They store the same data but with different S3 key structures.
The fists solution shared below uses objects, the second does not.
Is the solution using objects an excessive abstraction or there are benefits to it?
(There is no need to comment on the use of the callback.)
Here is a version using objects:
// not all code is shown here for the sake of brevity
module.exports.handle = (event, context, cb) => {

    const s3Sdk = new AWS.S3({ region: config.s3.region });
    const s3Legacy = new S3(s3Sdk, config.s3.legacyBucketName);
    const s3Cache = new S3Cache(s3Sdk, config.s3.cacheBucketName);

    s3Legacy.putObject(key, event.body, callback);
    s3Cache.putObject({
                unitId: event.pathParameters.unitId,
                timestamp: event.timestamp,
                eventId: event.id,
                eventFileName: event.id,
                body: event.body
            }, callback)'

}

class S3 {
    constructor(s3Sdk, bucketName) {
        this.s3Sdk = s3Sdk;
        this.bucketName = bucketName;
    }

    putObject(key, body, callback) {
        this.s3Sdk.putObject({ Bucket: this.bucketName, Key: key, Body: body }, (err, response) => {
            callback(err, response);
        });
    }
}

class S3Cache extends S3 {

    constructor(s3Sdk, bucketName) {
        super(s3Sdk, bucketName);
    }

    putObject(params, callback) {
        const key = this.toS3Key(
            params.unitId,
            this.toTimestampObject(params.timestamp),
            params.eventId,
            params.eventFileName
        );
        super.putObject(key, params.body, callback);
    }

    toS3Key(unitId, incidentTimestamp, incidentId, archiveIncidentObjectName) {
       return ....
    }

    toTimestampObject(timestamp) {
        return ....
    }
}

Here is a version without using objects:
// not all code is shown here for the sake of brevity
module.exports.handle = (event, context, cb) => {

    const s3 = new AWS.S3({ region: config.s3.region });
    await s3.putObject({ Bucket: config.s3.legacyBucketName, Key: key, Body: event.body }).promise();
    await s3.putObject({
            Bucket: config.s3.cacheBucketName,
            Key: toS3Key(event.unitId, toTimestampObject(event.timestamp), event.eventId, event.eventId),
            Body: event.body
        }).promise();
}

function toS3Key(unitId, incidentTimestamp, incidentId, archiveIncidentObjectName) {
       return ....
}

function toTimestampObject(timestamp) {
        return ....
}

In this version of the code, both the service orchestration code (write to s3Legacy and if successful write to s3Cache) and the s3Legacy, s3Cache, the code constructing the S3 key resides in one source file.
So this source file could change for multiple unrelated reasons.
For example when we want to change the orchestration, change the order of writing to S3s or if we want to write to DynamoDB for example.
The source file would also change if we need to change the S3 key schema.
We set a pattern here that every new addition would be added to this single source file which size would grow as a result and the list of reasons for a change would also grow.
While this example here is still small, calls only 2 services, I wonder whether extracting out the S3 code would set a better starting point for additional new development and enhancement.

I think the use of classes here is excessive because of complexity added by their structure and inheritance. Since your objects have only one method putObject you can make abstraction on the function level. So you can just use closures to save state by returning a function that will put objects. In this case s3Cache will use only one function of s3Legacy as a dependency.
Here is the code:
const s3Legacy = (sdk, bucket) => function putObject(key, body, callback) {
    sdk.putObject({ Bucket: bucket, Key: key, Body: body }, callback);
}

const s3Cache = (putObjectLegacy) => function putObject(params, callback) {
    const toS3Key = (unitId, incidentTimestamp, incidentId, archiveIncidentObjectName) => 'implementation';
    const toTimestampObject = (timestamp) => 'implementation';

    const key = toS3Key(
        params.unitId,
        toTimestampObject(params.timestamp),
        params.eventId,
        params.eventFileName
    );

    putObjectLegacy(key, params.body, callback);
}

module.exports.handle = (event, context, cb) => {
    const s3Sdk = new AWS.S3({ region: config.s3.region });
    const putObjectLegacy = s3Legacy(s3Sdk, config.s3.legacyBucketName);
    const putObjectCache = s3Cache(s3Legacy(s3Sdk, config.s3.cacheBucketName));

    putObjectLegacy(key, event.body, callback);
    putObjectCache({
        unitId: event.pathParameters.unitId,
        timestamp: event.timestamp,
        eventId: event.id,
        eventFileName: event.id,
        body: event.body
    }, callback);
}


Validating the difference in days between two dates I have some code that is used for validating a condition based on dates, which is always a bit tricky. Here is the code:

package rules

type valueInterval struct {
    min int
    max int
}

func (v valueInterval) containsClosed(x int) bool {
    return x >= v.min && x <= v.max
}

func permanentExpiryLimit(entityToCheck bla.Entity, intervalInDays valueInterval, compare time.Time) (RuleStatus, string) {
    if entityToCheck.PermanentExpiry == nil {
        return RULE_NOT_APPLICABLE, "... TODO ..."
    }

    diff := beginningOfDay(*entityToCheck.PermanentExpiry).Sub(beginningOfDay(compare))
    diffInDays := int(diff.Hours()) / 24

    if diffInDays < 0 {
        return RULE_NOT_APPLICABLE, "... TODO ..."
    }

    if !intervalInDays.containsClosed(diffInDays) {
        return RULE_FAILED, "... TODO ..."
    }

    return RULE_PASSED, "... TODO ..."
}

func beginningOfDay(datetime time.Time) time.Time {
    oneDay := 24 * time.Hour
    return datetime.Truncate(oneDay)
}

Client code:

   status, msg := permanentExpiry(entity, valueInterval{0, 100}, time.Now())

The idea is that this code check if the PermanentExpiry is at most 100 days in the future from a given compare date.

It can be assumed that both dates are in the same timezone. The dates should be compared only on day basis, that's why the time objects are reset to 00:00:00 time.

Still feels a bit clunky and not sure if it's clear that the comparision is done based on full days only. Any comments appreciated.

Client code:

   status, msg := permanentExpiry(entity, 100, time.Now())

  The idea is that this code check if the PermanentExpiry is at most 100
  days in the future from a given compare date.

  It can be assumed that both dates are in the same timezone. The dates
  should be compared only on day basis, that's why the time objects are
  reset to 00:00:00 time.

  Still feels a bit clunky and not sure if it's clear that the
  comparision is done based on full days only.

In Go, encapsulate that sort of logic in useful functions like isExpired. For example,

expire.go:

package main

import (
    "fmt"
    "time"
)

// isExpired returns true if date is not before the expiration date.
// The expiration date is start date plus expire days.
// Start is in the same time zone as date.
func isExpired(date, start time.Time, expire int) bool {
    loc := date.Location()
    y, m, d := start.In(loc).Date()
    x := time.Date(y, m, d+expire, 0, 0, 0, 0, loc)
    return !date.Before(x)
}

func main() {
    now := time.Now()
    starts := []time.Time{
        time.Date(now.Year(), now.Month(), now.Day(), 0, 0, 0, 0, now.Location()),
        now,
        time.Date(now.Year(), now.Month(), now.Day(), 23, 59, 59, 0, now.Location()),
    }
    expire := 10 // 100
    for _, start := range starts {
        for i := expire - 2; i <= expire+1; i++ {
            date := now.AddDate(0, 0, i)
            x := isExpired(date, start, expire)
            fmt.Println(
                x, "\t",
                date.Format("2006-01-02 15:04"),
                start.Format("2006-01-02 15:04"), expire,
            )
        }
        fmt.Println()
    }
}

Output:

false    2017-07-20 11:07 2017-07-12 00:00 10
false    2017-07-21 11:07 2017-07-12 00:00 10
true     2017-07-22 11:07 2017-07-12 00:00 10
true     2017-07-23 11:07 2017-07-12 00:00 10

false    2017-07-20 11:07 2017-07-12 11:07 10
false    2017-07-21 11:07 2017-07-12 11:07 10
true     2017-07-22 11:07 2017-07-12 11:07 10
true     2017-07-23 11:07 2017-07-12 11:07 10

false    2017-07-20 11:07 2017-07-12 23:59 10
false    2017-07-21 11:07 2017-07-12 23:59 10
true     2017-07-22 11:07 2017-07-12 23:59 10
true     2017-07-23 11:07 2017-07-12 23:59 10


Simple Caesar Cipher C++ I was surprised to learn that there are not many C++ Caesar ciphers on Code Review (3 others as of this question). And none that work in this fashion.

Here's the code:

// Caesar cipher

#include <iostream>

const char chars[] = {"abcdefghijklmnopqrstuvwxyz"};

// Find position in array
int findPosition(char x){
    int i;

    for(i = 0; i <= 25; i++){
        if(x == chars[i]){
            break;
        }
    }

    return i;

}

// Change char to next in array, loop back if at end
char nextChar(char x){
    if(findPosition(x) == 25){
        return chars[0];
    }

    return chars[findPosition(x) + 1];
}

// Rotate char desired amount
char rotChar(char x, int rotNum){
    for(int i = 1; i <= rotNum; i++){
        x = nextChar(x);
    }

    return x;
}

int main(){
    std::string str;
    std::getline(std::cin, str);

    std::cout << '\n';

    for(int i = 0; i <= 25; i++){
        std::cout << "ROT" << i << " - ";

        for(int k = 0; str[k] != '\0'; k++){
            std::cout << rotChar(str[k], i);
        }

        std::cout << '\n';
    }

    return 0;
}

This is a very simple Caesar cipher. It currently ignores anything uppercase or non-alphabetic.

Here's an example output:

ROT0 - helloworld
ROT1 - ifmmpxpsme
ROT2 - jgnnqyqtnf
ROT3 - khoorzruog
ROT4 - lippsasvph
ROT5 - mjqqtbtwqi
ROT6 - nkrrucuxrj
ROT7 - olssvdvysk
ROT8 - pmttwewztl
ROT9 - qnuuxfxaum
ROT10 - rovvygybvn
ROT11 - spwwzhzcwo
ROT12 - tqxxaiadxp
ROT13 - uryybjbeyq
ROT14 - vszzckcfzr
ROT15 - wtaadldgas
ROT16 - xubbemehbt
ROT17 - yvccfnficu
ROT18 - zwddgogjdv
ROT19 - axeehphkew
ROT20 - byffiqilfx
ROT21 - czggjrjmgy
ROT22 - dahhksknhz
ROT23 - ebiiltloia
ROT24 - fcjjmumpjb
ROT25 - gdkknvnqkc

How can this code be improved?

Function by function review

findPosition()

This function is inefficient because it searches an alphabetized string trying to find the index of a lowercase letter.  It would be faster to not even use the string and just compute the position directly like this:

if (x >= 'a' && x <= 'z')
    return x - 'a';

The other problem is what to return if the input character is not found.  Currently you return 26 and this causes the caller to print a null character (because chars[26] is the end of the string).  I think it would be preferable to return -1 and have the caller detect and deal with that.  So in total:

int findPosition(char x)
{
    if (x >= 'a' && x <= 'z')
        return x - 'a';
    return -1;
}

nextChar()

This function works fine, but it gets called n times, where n is the number of times you are rotating.  You should be able to delete this function if you modified rotChar() appropriately.

rotChar()

Instead of calling nextChar() rotNum times, you should be able to use the modulo operator to compute the next character in one step:

// Rotate char desired amount
char rotChar(char x, int rotNum)
{
    int pos = findPosition(x);

    if (pos == -1)
        return x;
    return 'a' + ((pos + rotNum) % 26);
}

Notice that this also returns the character unmodified if it wasn't a lowercase character.  At this point, you might want to just eliminate findPosition() and just do everything in rotChar() like this:

// Rotate char desired amount
char rotChar(char x, int rotNum)
{
    if (x >= 'a' && x <= 'z')
        return 'a' + (((x - 'a') + rotNum) % 26);
    return x;
}

Then you could easily extend this to handle uppercase letters as well:

// Rotate char desired amount
char rotChar(char x, int rotNum)
{
    if (x >= 'a' && x <= 'z')
        return 'a' + (((x - 'a') + rotNum) % 26);
    if (x >= 'A' && x <= 'Z')
        return 'A' + (((x - 'A') + rotNum) % 26);
    return x;
}


Cancellable UI loader I'm playing with async/await, but I have yet to discover a standard method to safely cancel an intensive task. I have tested the following, and it works exactly as intended, though I remain unsure if a standard method exists.

abstract class PageLoader<TPage, TElement> where TPage : Page
{
    private TPage page;
    private Task loader;
    private volatile bool mode;
    private volatile int token;

    public PageLoader(TPage page)
    {
        if (page == null)
        {
            throw new ArgumentNullException("page");
        }

        this.page = page;
        this.mode = true;
    }

    protected TPage Page => page;
    protected bool Mode => mode;

    public void Load(TElement[] items)
    {
        if (!page.Dispatcher.HasThreadAccess)
        {
            throw new InvalidOperationException("The current thread is not authorized.");
        }

        if (loader != null)
        {
            throw new InvalidOperationException("The loader requires reset.");
        }

        if (items == null)
        {
            throw new ArgumentNullException("items");
        }

        int value = token;

        mode = true;
        (loader = new Task(() => AutoLoad(value, items))).Start();
    }

    public async Task<bool> Set(bool retainMode)
    {
        if (!page.Dispatcher.HasThreadAccess)
        {
            throw new InvalidOperationException("The current thread is not authorized.");
        }

        if (mode)
        {
            mode = false;

            if (loader != null)
            {
                await loader;
                loader = null;
                token++;
            }

            if (retainMode)
            {
                mode = true;
            }

            return true;
        }

        return false;
    }

    protected void Commit(Entry entry)
    {
        if (entry == null)
        {
            throw new ArgumentNullException("entry");
        }

        DispatchedHandler handler = () =>
        {
            if (entry.Token == token)
            {
                AutoCommit(entry);
            }
        };

        var action = page.Dispatcher.RunAsync(CoreDispatcherPriority.Normal, handler);
    }

    protected abstract void AutoLoad(int token, TElement[] items);
    protected abstract void AutoCommit(Entry entry);

    protected class Entry
    {
        public readonly int Token;
        public readonly TElement Element;
        public readonly object Data;

        public Entry(int token, TElement element, object data)
        {
            Token = token;
            Element = element;
            Data = data;
        }
    }
}

Tested with:

class GridViewLoader : PageLoader<MainPage, int>
{
    public GridViewLoader(MainPage page) : base(page) { }

    protected override void AutoLoad(int token, int[] items)
    {
        foreach (int item in items)
        {
            if (!Mode)
            {
                break;
            }

            // (Intensive process per item..)

            Commit(new Entry(token, item, null));
        }
    }

    protected override void AutoCommit(Entry entry)
    {
        GridViewItem item = new GridViewItem()
        {
            Content = entry.Element.ToString()
        };

        Page.gridView.Items.Add(item);
    }
}

Usage:

private GridViewLoader gridViewLoader;

private async void queryBox_QuerySubmitted(AutoSuggestBox sender, AutoSuggestBoxQuerySubmittedEventArgs args)
{
    if (!await gridViewLoader.Set(false))
    {
        return;
    }

    gridView.Items.Clear();

    int[] selection;

    // (Do stuff..)
    selection = new int[] { 1, 2, 3 };

    if (selection != null)
    {
        gridViewLoader.Load(selection);
    }
}

You should be using a CancellationToken, which can be generated like:

CancellationTokenSource source = new CancellationTokenSource();
CancellationToken token = source.Token;

Pass this token to the method you wish to potentially cancel and keep a reference outside the method.  Then, from the calling method you can check token.IsCancellationRequested at periodic intervals and return, or if you wish to throw, you can call token.ThrowIfCancellationRequested().  To signal a cancellation, you call source.Cancel(), which propagates through to the token and signals a cancellation request.  You can also set it up to automatically cancel after a certain amount of time when you create the CancellationTokenSource.


Pretty way of keeping sensitive info out of a logged command string in Ruby? I have a long command that I am building with shovels (<<), with the intention of eventually running system(command). I'd like to log the command string, to make sure it is built properly, right before I execute the command. However, the command has to contain account credentials that I don't want to log. I'm looking for an elegant way to satisfy both requirements (1. log the meat of the command string, 2. don't log the auth credentials it contains).

This is what I have now:

command = ""
public = ""

[command, public].each {|str| str << "command_name"}
[command, public].each {|str| str << " -a a"}
[command, public].each {|str| str << " -b b"}
[command, public].each {|str| str << " -c c"}
command << " -d d"
command << " -e e"
[command, public].each {|str| str << " -f f"}
command << " -g g"
[command, public].each {|str| str << " -h h"}
[command, public].each {|str| str << " -i i"}

puts command
=> command_name -a a -b b -c c -d d -e e -f f -g g -h h -i i
puts public
=> command_name -a a -b b -c c -f f -h h -i i

system(command)

Original question from Stack Overflow

I see two more security issues with this code.

Executing a string rather than an array of command and parameters

When building a command line programmatically, it is dangerous to build it as a string, especially when any of the parameters comes from user input.  When calling Kernel#system with a string, a shell interprets the command line; when calling it with an array, the operating system kernel executes the command directly.

In the benign failure case, the password might contain a space, in which case the shell would interpret it as two parameters, probably causing your command to be invoked incorrectly.  In the worst case, the user could trigger an arbitrary command execution vulnerability.

As an illustration, the following code will do more than just set your password to "hello":

password_file = '.htpasswd'
username = 'CHK'
password = 'hello; touch me; grep; unzip; strip; finger; fsck -y; more; yes; yes; yes; umount; sleep'
command = "htpasswd -b -c #{password_file} #{username} #{password}"
system(command)

Practice safer computing!  This is not vulnerable to arbitrary command execution:

command = ['htpasswd', '-b', '-c', password_file, username, password]
system(*command)

Passing sensitive information on the command line

Even if you manage to censor the logger within Ruby, it's still considered dangerous practice to pass any sensitive information as a command-line parameter.  Command lines are visible to all users on a machine.

For that reason, all reasonable commands should offer an alternative mechanism to accept secret information.  For example, the recommended way to use htpasswd, since Apache 2.4, is to read the password from standard input.

command = ['htpasswd', '-i', '-c', password_file, username]
IO.popen(command, mode='w') do |htpasswd|
  htpasswd.puts(password)
end

Similarly, here is an example of how to securely specify a passphrase to GnuPG (in Python).

In summary, by passing the secret information through pipes instead of the command line, you solve both the log censorship problem and the machine-wide command-line visibility problem.


Repetitive code driving me crazy! Ok, So first I must say that everything I know about coding I have learned on my own in my spare time so bear with me if my code is primitive, but please, I am open to any comments to make me better...

Anyway, as for my question. I have an application I am building in C# for .NET Compact Framework (for an HP iPaq) the program's purpose is to act similarly to a Restaurant POS terminal to "ring up" food orders.  it has gotten to the point where some of my code is the same line copied god even knows how many times with only a numerical difference between them. here is an example:

    private void button9_Click(object sender, EventArgs e)
    {
        AddItem(buttonNames[0], prices[0]);
    }

    private void button14_Click(object sender, EventArgs e)
    {
        AddItem(buttonNames[1], prices[1]);
    }

    private void button5_Click(object sender, EventArgs e)
    {
        AddItem(buttonNames[2], prices[2]);
    }

    private void button10_Click(object sender, EventArgs e)
    {
        AddItem(buttonNames[3], prices[3]);
    }

    private void button13_Click(object sender, EventArgs e)
    {
        AddItem(buttonNames[4], prices[4]);
    }

    private void button4_Click(object sender, EventArgs e)
    {
        AddItem(buttonNames[5], prices[5]);
    }

or

                button9.Text = buttonNames[0];
                button14.Text = buttonNames[1];
                button5.Text = buttonNames[2];
                button10.Text = buttonNames[3];
                button13.Text = buttonNames[4];
                button4.Text = buttonNames[5];
                button11.Text = buttonNames[6];
                button15.Text = buttonNames[7];
                button7.Text = buttonNames[8];
                button12.Text = buttonNames[9];
                button16.Text = buttonNames[10];
                button8.Text = buttonNames[11];

I KNOW there are easier ways to do a lot of the code I have written, I just don't know how to do it.

Even though the difference is minimal, there IS a difference. So you'll need to make a minimal distinction, depending on the button that was pressed.

What you could do is create a general event for all the buttons and in that event you'll determine the index to be passed to the AddItem() method. The code might look like this:

private void ButtonClick(object sender, EventArgs e)
{
    Button b = (Button)sender;

    switch(b.Name)
    {
        case "button9" : AddItem(buttonNames[0], prices[0]); break;
        case "button14" : AddItem(buttonNames[1], prices[1]); break;
        case "button5" : AddItem(buttonNames[2], prices[2]); break;
        //other buttons...
    }
}

Another solution is to pre-assign the index to the Tag property of your button:

button9.Text = buttonNames[0];
button9.Tag = 0;
//Same for other buttons

This way you can also use a general method like before and use the tag to call the AddItem() method. Like this:

private void ButtonClick(object sender, EventArgs e)
{
    Button b = (Button)sender;
    int index = Convert.ToInt32(b.Tag);
    AddItem(buttonNames[index], prices[index]);
}

Lastly, if you name your buttons in a different way, it will become even more easy to do this. Place the index to be used in the name of the button and get it from the name in the general event. Example:

//Assignment of the buttons:
button0.Text = buttonNames[0];
button1.Text = buttonNames[1];
button2.Text = buttonNames[2];

//Event:
private void ButtonClick(object sender, EventArgs e)
{
    Button b = (Button)sender;
    int index = Convert.ToInt32(b.Name.Replace("button", ""));
    AddItem(buttonNames[index], prices[index]);
}

Hope this helps!


Allow one thread to execute section while others wait and resume I have a requirement that is summarized below.
void update()
{
  |   |   |   |
  |   |   |   |
  |   |   |   |
 \|  \|  \|  \|
  =============
      meat           <--- Only one thread must execute here, the rest should skip it.
  =============
  |   |   |   |  <--- Other threads wait here until the "first" thread is done w/ "meat"
  |   |   |   |
  |   |   |   |
  |/  |/  |/  |/
}

From an outsider perspective, "meat" should be executed just once (by the first thread that encounters it)
This post is similar but differs in that it also requires other threads to execute the meat code block one at a time.
// testExecutive.cpp
#include <atomic>
#include <condition_variable>
#include <iostream>
#include <mutex>
#include <thread>

struct Target {
  int nhits = 0;
  inline void hit() { ++nhits; }
};

struct Executive {
  Executive() = delete;
  Executive(int _Id, Target &_Tgt) : m_Id(_Id), m_Ready(false), m_Tgt(_Tgt) {}
  void update() {
    { // only one thread can execute this
      std::scoped_lock<std::mutex> lk(m_Mtx);
      // std::cout << "++Acquired lk " << m_Id << " | tid: " <<
      // std::this_thread::get_id() << "++\n";
      if (!m_Ready.load()) {
        // std::cout << "==Updating" << m_Id << " | tid: " <<
        // std::this_thread::get_id() << "==\n";
        m_Ready.store(true);
        m_Tgt.hit();
        m_Cv.notify_all();
      }
      // std::cout << "++Releasing lk " << m_Id << " | tid: " <<
      // std::this_thread::get_id() << "++\n";
    }

    // the rest should wait here
    std::mutex wait_mtx;
    std::unique_lock<std::mutex> lk(wait_mtx);
    m_Cv.wait(lk, [&]() -> bool { return m_Ready.load(); });
  }
  inline void reset() { m_Ready.store(false); }

private:
  int m_Id;
  Target &m_Tgt;
  std::condition_variable m_Cv;
  std::mutex m_Mtx;
  std::atomic_bool m_Ready;
};

int main(int argc, char *argv[]) {
  int n(2);
  if (argc >= 2) {
    n = atoi(argv[1]);
  }
  Target tgt;
  Executive e(1, tgt);
  auto updFn = [&e]() { e.update(); };

  // update same executive from 3 threads. Repeat n times.
  int i(n);
  while (i > 0) {
    e.reset();
    {
      std::thread t1(updFn), t2(updFn), t3(updFn);
      t1.join();
      t2.join();
      t3.join();
    }
    --i;
  }
  std::cout << (n == tgt.nhits ? "[OK]" : "[ERR]") << " Updates: " << n << " | "
            << "Hits: " << tgt.nhits << "\n";
  return 0;
}

I setup a simple method to verify that exactly one out of three threads executes the desired code. The method hit() is called in Executive::Update. Compile and test like so.
jaswant@HAL9000:~$ g++ testExecutive.cpp -lpthread --std=c++17
jaswant@HAL9000:~$ ./a.out 2
[OK] Updates: 2 | Hits: 2
jaswant@HAL9000:~$ ./a.out 100
[OK] Updates: 100 | Hits: 100
jaswant@HAL9000:~$ ./a.out 2
[OK] Updates: 2 | Hits: 2
jaswant@HAL9000:~$ ./a.out 4
[OK] Updates: 4 | Hits: 4
jaswant@HAL9000:~$ ./a.out 8
[OK] Updates: 8 | Hits: 8
jaswant@HAL9000:~$ ./a.out 16
[OK] Updates: 16 | Hits: 16
jaswant@HAL9000:~$ ./a.out 256
[OK] Updates: 256 | Hits: 256
jaswant@HAL9000:~$ ./a.out 1024
[OK] Updates: 1024 | Hits: 1024
jaswant@HAL9000:~$ ./a.out 8192
[OK] Updates: 8192 | Hits: 8192
jaswant@HAL9000:~$ ./a.out 100000
[OK] Updates: 100000 | Hits: 100000

So, it works. Can anyone please point out bad usage of mutex/condition variables/locks and perhaps a way to do this w/o an atomic variable in the predicate?

Use std::once_flag
The std::once_flag class basically does what you want. However, there is no way nice way to reset it, except by using a trick like placement new, or by allocating a flag dynamically, and using a std::unique_ptr to point to it for example. As indi mentioned, it's not really "once" anymore then, but it does work for your specific case. Here is a drop-in replacement for your struct Executive:
struct Executive {
  Executive(int _Id, Target &_Tgt) : m_Id(_Id), m_Tgt(_Tgt) {}
  void update() {
        std::call_once(flag, [this]{ m_Tgt.hit(); });
  }
  void reset() {
    flag.~once_flag();
    new (&flag) std::once_flag;
  }

private:
  int m_Id;
  Target &m_Tgt;
  std::once_flag flag;
};

No need to write inline
If you include the full definition of a member function in a class or struct definition, you don't need to explicitly use the inline specifier, as it will implicitly be an inline function.
Write idiomatic code
There are a few things in your code that are not incorrect in any way, but are a rather unusual way to write things. For example, when declaring an int and initializing it, I would expect int n = 2 or int n{2}, but not int n(2).
Also, when repeating something a number of times, use a for-loop, like so:
for (int i = 0; i < n; ++i) {
    e.reset();
    ...
}

This completely describes how often the loop is run in a single line, which makes it easier to read.


I have a query like this that seems complicated and I want to do it by using Java Stream Api I have method that fetches sorted and filtered data from Mysql Database, Firstly I had done It by using Java. Now I did it by Mysql but I don't like the way the code looks
I have entity that named Order and It looks like this
public class Order implements Serializable {
    private int orderId;
    private int userId;
    private int carId;
    private String userAddress;
    private String userDestination;
    private double orderCost;
    @JsonSerialize(using = CustomLocalDateTimeJsonSerializer.class)
    private LocalDateTime orderDate;
//getters, setters
}

Method that performs all work
@Override
    public List<Order> findFilSortOrders(Map<String, String> filters, String sortedColumn, boolean descending, int startRow, int rowsPerPage) {
    StringBuilder query = new StringBuilder("SELECT*FROM (SELECT*FROM user_order ");
    ResultSet resultSet = null;
    LinkedHashMap<String,String> filterToQuery;
    List<String> values = new ArrayList<>();
    List<String> keys = new ArrayList<>();
        List<Order> orders = new ArrayList<>();
        if(filters!=null){
            query.append("WHERE ");
            filterToQuery = new LinkedHashMap<>(filters);
            values = new ArrayList<>(filterToQuery.values());
            keys = new ArrayList<>(filterToQuery.keySet());
            keys.forEach(key->
        {
            if(key.equals("orderDate")){
                query.append("date_format(").append(key).append(",'%Y-%m-%d %H:%i') = ? AND ");
            }else{
                query.append(key).append(" = ? AND ");
            }
        });
            query.setLength(query.length()-4);
    }
        query.append(" LIMIT ?, ?) a").append(" ORDER BY ").append(sortedColumn).append(" ").append(descending?"DESC":"ASC;");
    try(Connection connection = MySQLDAOFactory.getConnection();
    PreparedStatement statement = connection.prepareStatement(query.toString())){
       for (int i=0;i<values.size();i++){
           switch (keys.get(i)){
               case "orderId":
               case "userId":
               case "carId":{
                   statement.setInt(i+1, Integer.parseInt(values.get(i)));
                   break;
               }
               case "orderDate":
               case "userAddress":
               case "userDestination":{
                  statement.setString(i+1,values.get(i));
                   break;
               }
               case "orderCost":{
                   statement.setDouble(i+1,Double.parseDouble(values.get(i)));
                   break;
               }
           }
       }
       statement.setInt(values.size()+1,startRow);
       statement.setInt(values.size()+2,rowsPerPage);
        resultSet = statement.executeQuery();
        while(resultSet.next()){
            Order order = new Order();
            order.setCarId(resultSet.getInt("carId"));
            order.setOrderId(resultSet.getInt("orderId"));
            order.setUserId(resultSet.getInt("userId"));
            order.setUserAddress(resultSet.getString("userAddress"));
            order.setUserDestination(resultSet.getString("userDestination"));
            order.setOrderCost(resultSet.getDouble("orderCost"));
            order.setOrderDate(resultSet.getTimestamp("orderDate").toLocalDateTime());
            orders.add(order);
        }
    }catch (SQLException e){
        LOGGER.error(e);
        try {
            if (resultSet != null) {
                resultSet.close();
            }
        } catch (SQLException throwables) {
            LOGGER.error(throwables);
        }
    }
     return orders;
    }

This code works but I dont like it looks If you could help me improve the code, I would be very grateful. Thanks for answering.

First off, the SQL query you are generating probably doesn't do what you want it to do. Because you using a subquery (a SELECT inside a SELECT) you are first limiting the unsorted data, and the sorting it, resulting in returning "random" rows instead of the first rows in sorted order. There is no reason to use a subquery, so instead of
SELECT * FROM (SELECT * FROM user_order WHERE ... LIMIT ?, ?) a ORDER BY ...

use
SELECT * FROM user_order WHERE ... ORDER BY ... LIMIT ?, ?

Then you really should format your code cleanly. The indentation is really bad and you should insert some empty lines between logical code blocks to separate them optically. Also put spaces in lines, specifically around operators, between keywords (if, while etc.) and (, after ; and before {. For example, the line:
for(int i=0;i<values.size();i++){

should look like this:
for (int i = 0; i < values.size(); i++) {

BTW, your editor/IDE should be able to do that for you. Look for a menu item "Format code".

Don't declare variables until you need them. Both resultSet and orders don't need to be declared until the loop at the end:
ResultSet resultSet = statement.executeQuery();
List<Order> orders = new ArrayList<>();
while (resultSet.next()){