Simple array operation using a Queue Question

We are given an array (indexed from 1) of N elements on which we make M queries:

add(Left, Right, X) - all the elements between the position Left and Right (1<=Left<=Right<=N) are raising their values with X.

After all the operations are completed, print the array.

example

Array: 1 1 1 4 5 6

operations: (1, 5, 2), (2, 3, 10)

Final array: 3 13 13 6 7 6

Discussion

I managed to produce the expected output but I was wondering if this code can be simplified using a queue data structure somehow?

The complexity of this code is M*N (please correct me if I am wrong) - This naive approach is looping all the operations for every element of the input array.

Naive approach

#include <iostream>

using namespace std;

int Queue[1000], backInd;

void push(int i)
{
    ++backInd;
    Queue[backInd] = i;
}

void applyOperations(int input[], int operations[], int N, int M)
{
    for (int x = 0; x < N; ++x){
        int item = input[x];
        int offset = 0;
        for (int op = 0; op < M; ++op)
        {
            if (x + 1 >= operations[op + offset] && x + 1 <= operations[op + offset + 1])
            {
                item += operations[op + offset + 2];
            }
            offset += 2;
        }
        push(item);
    }
}

int main()
{
    backInd = -1;
    frontInd = 0;
    int input[] = {1, 1, 1, 4, 5, 6};
    int operations[] = {1, 5, 2, 2, 3, 10};
    int N = 6;
    int M = 2;

    applyOperations(input, operations, N, M);

    for (int x = 0; x < N; ++x) {
        cout << Queue[x] << " ";
    }

    return 0;
}

I'm not really sure if your code matches the intention of the task, albeit you possibly might get the expected results.

At first, you are producing a new array, whereas the task is 'all elements raise their values', which I read that you should modify the array in place.

Then I'd rather assume that you should implement a single operation in a function which then gets called M times, example:

void applyOperation(int array[], size_t length, size_t begin, size_t end, int operator);

int array[] = { 1, 1, 1, 4, 5, 6 };
applyOperation (array, sizeof(array)/sizeof(*array), 1, 5, 2);
applyOperation (array, sizeof(array)/sizeof(*array), 2, 3, 10);
// print result

M is 2, function is called twice (I renamed N to length...).

Whether you now implement the function to execute one operation at a time or all at once, you shouldn't iterate over the entire array and then check if the index is in the range, instead you'd iterate over the range directly:

void applyOperation(int array[], size_t length, size_t begin, size_t end, int operator)
{
    --begin; --end; // they want one-based indices? bad idea for C++,
                    // but OK, then let's adjust them to zero-based...

    for(;; begin != end; ++begin)
    {
         array[begin] += operator;
    }
}

Looks much easier, doesn't it? What's actually lacking yet is checking the range of begin and end, assuming you manage this yourself...

Now if you want to have all operations in one single go, then it is much more comprehensive if you iterate over the operations in first place:

void applyOperations(int input[], int operations[], int N, int M)
{
    for(int i = 0; i < M; ++i, operations += 3)
    // this assumes: each operation occupies three subsequent values
    {
         // now using operations[0] as begin, operations[1] as end
         // and operations[2] as operator just like above

         // it might be unacceptable to modify the operations array, then you'd
         // need a separate loop variable compared to above; but IF it is,
         // then you should keep the operations const anyway:
         //int const operations[]
    }
}

Finally: You tagged the question C++ – your code could as well be good old C, though. In C++, you'd rather use std::array or std::vector than raw arrays. Whichever you use, you don't have to provide size/length separately, both come with their own member function size() giving the number of elements (not size in bytes as sizeof operator does, so you'd avoid the ugly sizeof(x)/sizeof(*x) at the same time – even more important: you could provide a bad value for length, this cannot happen with the C++ containers as these manage their length/size on their own).

void applyOperation(std::vector<int> values, size_t begin, size_t end, int operator)
{
    --begin; --end;
    for(;; begin != end; ++begin)
    {
         values[begin] += operator;
    }
}

The loop still looks similar, but range checking would now use values.size() instead of a length parameter.

One final note: Did you notice that I changed the parameter types to size_t? Two reasons for this:

size_t is an unsigned type. Negative indices are meaningless in the given task, so why allow them at all?
size_t mapped to the native type large enough to hold an object of maximum size on given system (which means at the same time that it will always be large enough to index any member in an array). This is obviously not relevant in given case, as your arrays' sizes most likely won't ever exceed the range of ordinary int - but you simply cannot fail this way, so if you get used to it, you'll be well prepared for the future...


Condorcet voting method in OOP Python For learning purposes, I made this little class which determines the Condorcet winner of a rank voting election (basically, a Condorcet winner is a candidate that would beat every other candidate in a head-to-head election).
This code does work, but I'd like to learn two-three things by posting it here:

Is this class written in a correct Pythonic way?
Do you see any ways to improve this code?
(bonus) If there are voting specialists among you, could you tell if that way of going through a Condorcet algorithm seems correct?

Condorcet.py:
import itertools

class Condorcet:
    """
    Gives Concorcet winner from a list of votes
    """
    def __init__(self, filename):
        self.candidates = set()
        self.scores = dict() # score for each permutation of two candidates
        self.results = dict() # winner of each pair of candidates
        self.winner = None # Condorcet winner of the election
        self.voters = list() # ranking of each voter
        self.filename = filename

    def process(self):
        self._get_data_from_file()
        self._build_dict()
        self._matches_candidates()
        self._elect_winner()
        return self.winner

    def _get_data_from_file(self):
        with open(self.filename, encoding='utf-8') as file:
            for lines in file:
                # currently hard-coding the number of candidates at 4
                # needs fixing but it's not my point here
                # (more a question for StackOverflow actually)
                (one, two, three, four) = lines.split(None, 4)
                self.voters.append((one, two, three, four))

    def _build_dict(self):
        """
        Builds a dictionary of scores
        for each permutation of two candidates
        """
        for voting in self.voters:
            for candidate in voting:
                self.candidates.add(candidate)
            for pair in list(itertools.permutations(voting, 2)):
                if pair not in self.scores:
                    self.scores[pair] = 0
                if voting.index(pair[0]) < voting.index(pair[1]):
                    self.scores[pair] += 1

    def _matches_candidates(self):
        """
        Analyses the dictionary of scores and
        gives the winner of every pair of candidates
        """
        for match in list(itertools.combinations(self.candidates, 2)):
            reverse = tuple(reversed(match))
            if self.scores[match] > self.scores[reverse]:
                self.results[match] = match[0]
            else:
                self.results[match] = match[1]

    def _elect_winner(self):
        """
        If a candidates is the winner against
        every other candidate, declares him the winner
        (Note: does not detect Condorcet cycles yet)
        """
        for candidate in self.candidates:
            candidate_score = 0
            for result in self.results:
                if candidate in result and self.results[result] == candidate:
                    candidate_score += 1
            if candidate_score == len(self.candidates) - 1:
                self.winner = candidate

execute_condorcet.py:
import Condorcet

condorcet = Condorcet.Condorcet('condorcet.txt')
winner = condorcet.process()
print(winner)

condorcet.txt:
# each line represents the ranking
# of candidates A, B, C and D
# by a unique voter

A B C D # this voter ranked A first, B second, C third, D fourth
C B D A
A B D C
C A D B
C B A D
C A B D

# and it goes on

and the winner could be C, beating :

A 4 votes to 2
B 4 votes to 2
D 5 votes to 1

The purpose of your class is to perform a single computation: given filename, determine winner. That's a task for a function; there is no need for a class. You can turn each method to an independent function that takes one or two arguments and returns one or two values instead of accessing instance attributes, and provide this function to orchestrate the whole computation:

def concordet_winner(filename):
    voters = _get_data_from_file(filename)
    candidates, scores = _build_dict(voters)
    results = _matches_candidates(candidates, scores)
    return _elect_winner(candidates, results)

Now, reading this code gives a clear picture how the data flows through the computation.

For the purpose of learning OOP, you could consider a slightly different exercise: instead of having all votes in a file, create a class with a vote method that takes in an individual voter's vote, and a winner property that updates automatically after every vote.


Concise null checking vs readability I've got a case where I need to call a couple of methods, checking if they return null, and return some property from the result of the second when neither returns null.

I've thought of two ways to do this - one "verbose" and one "concise". For the purposes of this question, I'll just call the return types 'dynamic' since I'm not interested in those details here.

private dynamic VerboseVersion()
{
     var a = CallMethodA();
     if ( a == null )
     {
         return null;
     }

     var b = CallMethodB( a );
     if ( b == null )
     {
         return null;
     }

     return b.SomeProperty;
}

private dynamic ConciseVersion()
{
    dynamic a;
    dynamic b;

    if ( ( a = CallMethodA() ) == null ||
         ( b = CallMethodB( a ) ) == null )
    {
        return null;
    }

    return b.SomeProperty;
}

I the first because the intent at each step is very obvious but I find it somewhat verbose; I like the second because the intent of the function seems more obvious and it is less verbose but it may be a bit harder to follow at first.

Thoughts?

Update: Corrected a typo and corrected the formatting of the "Verbose" version to what I would actually use in committed code.

I take great pleasure in the fact that your "concise" version is longer than your "verbose" version!

If you believe one version to be more readable and the other to be more obscure, I would argue going with readability over obscurity. One obscure detail is that you're relying on people understanding

(a = CallMethodA()) == null

Some people might think that to be an error, not realizing the expression on the left actually has a value.

If you want to use less lines of code, you could go with something like this

var a = CallMethodA();
var b = (a == null) ? null : CallMethodB(a);
return (b == null) ? null : b.SomeProperty;

Which isn't altogether different than your original if/else chain, but is a bit more compressed and has just the one return statement.


Web crawler that uses Task Parallel Library This is the first time using the Task Parallel Library and it seems to be performing much slower than I was expecting. The application is simply a web crawler that returns all URLs for a given URL.

Here is my method that, depending on the level param, will either download single page links (getSinglePageLinks()) or it will make use of my threaded method (getManyPageLinks()):

public static IEnumerable<string> getLinks(string url, bool hostMatch=true, bool validatePages=true, int level=0)
    {
        string formattedUrl = urlFormatValidator(url);
        if (string.IsNullOrEmpty(formattedUrl)) return Enumerable.Empty<string>();
        //download root url's
        IEnumerable<string> rootUrls = getSinglePageLinks(formattedUrl, hostMatch, validatePages);
        //download url's for each level
        for (int i=0; i<level; i++)
        {
            rootUrls = rootUrls.Union(getManyPageLinks(rootUrls, hostMatch, validatePages));
        }
        return rootUrls;
    }

getSinglePageLinks() makes use of the HTML agility pack, and it simply downloads and parses the given URL:

private static IEnumerable<string> getSinglePageLinks(string formattedUrl, bool hostMatch = true, bool validatePages = true)
    {
        try
        {
            HtmlDocument doc = new HtmlWeb().Load(formattedUrl);
            var linkedPages = doc.DocumentNode.Descendants("a")
                                              .Select(a => a.GetAttributeValue("href", null))
                                              .Where(u => !String.IsNullOrEmpty(u))
                                              .Distinct();

             //hostMatch and validatePages left out
             return linkedPages;
        }catch(...){...}

And my getManyPageLinks():

   private static IEnumerable<string> getManyPageLinks(IEnumerable<string> rootUrls, bool hostMatch, bool validatePages)
    {
        List<Task> tasks = new List<Task>();
        List<List<string>> allLinks = new List<List<string>>();

        foreach (string rootUrl in rootUrls)
        {
            string rootUrlCopy = rootUrl; //required
            var task = Task.Factory.StartNew(() =>
            {
                IEnumerable<string> taskResult = getSinglePageLinks(rootUrlCopy, hostMatch, validatePages);
                return taskResult;
            });

            tasks.Add(task);
            allLinks.Add(task.Result.ToList());
        }

        Task.WaitAll(tasks.ToArray());
        return allLinks.SelectMany(x => x).Distinct();
    }

The app works OK if the level is set to 0, but if I set the level to 1 so it gets all links for all of the roots URL, the CPU/Network usage does not go above 1-3%. How can I improve performance?

From MSDN on Task.Wait:

  If the current task has not started execution, the Wait method
  attempts to remove the task from the scheduler and execute it inline
  on the current thread.

I have a feeling that something similar might be happening with WaitAll, killing performances. I'll have a look and see if I can find documentation about it. As you are using StartNew some of the tasks might be started already so they won't be in-lined.

I would refactor it using async/await so you are sure you are not using and blocking only one thread from the pool:

async static Task<IEnumerable<string>> GetAllPagesLinks(IEnumerable<string> rootUrls, bool hostMatch, bool validatePages)
{
    var result = await Task.WhenAll(rootUrls.Select(url => GetPageLinks(url, hostMatch, validatePages)));

    return result.SelectMany(x => x).Distinct();
}

static async Task<IEnumerable<string>> GetPageLinks(string formattedUrl, bool hostMatch = true, bool validatePages = true)
{
    var htmlDocument = new HtmlDocument();

    try
    {
        using (var client = new HttpClient())
            htmlDocument.Load(await client.GetStringAsync(formattedUrl));

        return htmlDocument.DocumentNode
                           .Descendants("a")
                           .Select(a => a.GetAttributeValue("href", null))
                           .Where(u => !string.IsNullOrEmpty(u))
                           .Distinct();
    }
    catch
    {
        return Enumerable.Empty<string>();
    }
}

async static Task<IEnumerable<string>> GetLinks(string url, bool hostMatch = true, bool validatePages = true, int level = 0)
{
    if (level < 0)
        throw new ArgumentOutOfRangeException(nameof(level));

    string formattedUrl = FormatAndValidateUrl(url);

    if (string.IsNullOrEmpty(formattedUrl))
        return Enumerable.Empty<string>();

    var rootUrls = await GetPageLinks(formattedUrl, hostMatch, validatePages);

    if (level == 0)
        return rootUrls;

    var links = await GetAllPagesLinks(rootUrls, hostMatch, validatePages);

    var tasks = await Task.WhenAll(links.Select(link => GetLinks(link, hostMatch, validatePages, --level)));

    return tasks.SelectMany(l => l);
}

Haven't had a chance to test it, but just get the gist.


Simple bubblesort algorithm in C++ I have been learning C++ over the last couple of days. I know a few other programming languages well, and it hasn't been horribly difficult. I would appreciate any comments on my code so that I may improve:

Readability
CPP "style"
Efficiency
Vector vs array--which is better in this case?

Thanks in advance for any help you can give me.

//Bubblesort algorithm
#include <iostream>
#include <cstdlib>
#include <ctime>

#define pass ;

int main()
{
    // int unsorted[] = {1,2,3,4,5,6,7,8,9,10};
    int length;
    int max;
    srand(time(NULL));
    std::cout << "Please input how many numbers you want to sort and the max value on those numbers: " << std::endl;
    std::cin >> length;
    std::cin >> max;
    int unsorted[length];
    int count = -1;
    while(count++<length)
    {
        unsorted[count] = rand()%max;
    }
    //int length = sizeof( unsorted ) / sizeof( int );
    int i = 0;
    int num_changes_per_loop = 1; /* Should be set to zero at the beginning of each loop, but is nonzero here to prevent the loop from being cancelled on the first iteration */
    int general_loop_count = 0;
    while(general_loop_count++<length)
    {
        if(num_changes_per_loop==0) { break; }
        for(i=0;i<length-1;i++)
        {
            int current_value = unsorted[i];
            int adjacent_value = unsorted[i+1];
            if(adjacent_value > current_value) {pass}
            else if(current_value > adjacent_value) {
                std::cout << "These two values need to be switched!" << std::endl;
                int tmp = adjacent_value;
                unsorted[i+1] = current_value;
                unsorted[i] = tmp;
                num_changes_per_loop++;
            }
        }
    }
    std::cout << "Finished!" << std::endl;
    for(i=0;i<length;i++)
    {
        std::cout << unsorted[i] << ", ";
    }
}

Vector vs raw array

When dealing with arrays that have a length know at run-time, you usually use an std::vector, it is cleaner and safer w.r.t the memory allocation (for dynamicity). When the size of the array is known at compile-time, consider using an std::array, which is the C++ way of using raw arrays.

In this case, I'd use an std::vector and a .reserve(length) call

Array out of bound

Here is a concrete argument to why you should use a vector: you are actually writing/reading outside of you array, which can fail immediately (if you are lucky), or can fail way later in your code (if you're unlucky).

int count = -1;
while(count++<length)
{
    unsorted[count] = rand()%max;
}

if you have length == 1 for example, here is what will happen, 'unwrapping' the while:

int count = -1;
if (count++ < 1) // count == -1, so count < 1 is true ; after that we increment count by 1, so count == 0
{
    // accessint unsorted[0]
    unsorted[count] = rand()%max;
}
if (count++ < 1) // count == 0, so count < 1 is true ; after that we increment count by 1, so count == 1
{
    // accessint unsorted[1], which is outside of the array
    unsorted[count] = rand()%max;
}
// after that, the condition is false

For loops vs while loops

Twice in your code, you have a while loop looking like this one:

int count = -1;
while(count++<length)

When you iterate this way, you usually see a for loop instead:

for(unsigned int count = 0 ; count < length ; ++count)

which does (almost) the same thing, but is more readable.

Note that the starting value is 0, leading into one less iteration than previously. However, it is actually the last iteration that is removed

Pass

In your main loop, you are using your pass macro. As jvb mentioned, you usually want to avoid to use macros (as the faq iso cpp says). Moreover, in this particular case, you may consider two alternatives:

Using the continue keyword
Reverting your condition

In this case, the second alternative is way better, as it is easy to put into place (the keyword continue shouldn't be used unless other alternatives are awkward).

if(adjacent_value > current_value) { continue; }
else if(current_value > adjacent_value) {
    // ...
}

would become

if(adjacent_value < current_value) {
    // ...
}

because you are really doing something only if the above condition is fulfilled

Swapping values

To swap your values, you are using your custom code

int tmp = adjacent_value;
unsorted[i+1] = current_value;
unsorted[i] = tmp;

which works well.

However, you may consider using the function std::swap, that swaps two values:

std::swap(unsorted[i], unsorted[i+1]);


Retrieving objects from JSON URLs This is an accounting program (and my first attempt at a web app of any kind) where users need to get and retrieve objects from different JSON URLs and at different levels. Rails is used for the back.

I'd like to know if the following process is the best to use:

User clicks button to toggle some boolean value
Local JS boolean value is updated
PUT request is sent to JSON URL
Success or failure response is sent to browser

On success, "all changes saved" is sent to the view
On failure, a get request is sent to the SAME URL as the put request
On success of that get request, the local variable is updated

I also had the idea that I could use one function (putValue in the code below) for all put requests if I make the local javascript object use the same structure as the JSON I am retrieving. Is this a good idea?

After this, I will need to do the same thing for text inputs which I want to write to the server after each keystroke. Can I go about that in the same way as the code above?

This gives the current users account data:

www.example.com/accData.json:
{"deleteWarning":true,"displayHiddenFields":true,"groups":[{"id":2,"text":"My Company"}]}

A user has many microposts, which each have one template:

www.example.com/microposts/9/template.json
{"id":9,"name":"template","templateIndex":0,"shared":true,"micropost_id":9}

Users can get and put to these URLs.

A function in an Angular factory is used to put to these addresses.

//Put a single objValue into the parentKey  SQL table
var putValue = function(objValue, objKey, parentKey) {

  var putURL = '';
  var obj = {};

  //create the obj to be put
  var accObj = {};

  //tell the notification variable what's happening
  viewed.current.status = "saving...";

  //update local variable and define save location
  if (typeof parentKey === 'string') {
    putURL = '/microposts/' + viewed.sig.id + '/' + parentKey + '.json';
    viewed.sig[parentKey][objKey] = objValue;
    obj[parentKey] = accObj;
  } else {
    putURL = '/accData.json';
    viewed.accData[objKey] = objValue;
    obj = {acc_datum:accObj};
  }

  accObj[objKey] = objValue;

  //tell the console what's happening
  console.log("object sent to acc_datum is:");
  console.dir(obj);

  //do the put request
  return $http.put(putURL, obj).then(
    //on success do nothing
    function(response){},
    //on failure, get the data again
    function(response){

      //CHANGE BELOW - need this to update the correct local
      //variable not just viewed.accData

      //getData from putURL and put it into the local js object
      viewed.accData = getData(putURL, 'accData');
    }
  );
};

Just a note about your comments, you don't need so many of them. Sometimes the name of functions are clear enough that anyone with a passing knowledge of the language/tools can follow it.

//tell the console what's happening
console.log("object sent to acc_datum is:");

//do the put request
return $http.put(putURL, obj).then(

Those are pretty self explanatory and taking up space, you could remove them.

Also it's better not to split a comment about 2 possiblities like this:

//on success do nothing
function(response){},
//on failure, get the data again
function(response){

They're part of one thought, combine them:

//on success do nothing, on failure get the data again
function(response){},
function(response){

This keeps your comments and code separated more cleanly.


Saving images from a web page I'm just getting my feet on the ground with Python (maybe 3 days now). The only issue with teaching my self is that I have no one to critique my work.

Correct me if I'm wrong but I think my algorithm/method for solving this problem is quite promising; but, the code not so much.

The program basically strips a web-page and puts it in to the designated directory. My favorite part is the method for deciding the image's extensions :)

import os, re, urllib

def Read(url):
    uopen = urllib.urlopen(url)
    uread = ''
    for line in uopen: uread += line
    return uread

def Find(what, where):
    found = re.findall(what, where)
    match = []
    for i in found:
        i = i[9:-1]
        match.append(i)
    return match

def Retrieve(urls, loc):
    loc = os.path.realpath(loc)
    os.system('mkdir ' + loc +'/picretrieved')
    loc += '/picretrieved'
    x = 0
    exts = ['.jpeg', '.jpg', '.gif', '.png']
    for url in urls:
        x += 1
        for i in exts:
            ext = re.search(i, url)
            if ext != None:
                ext = ext.group()
                urllib.urlretrieve(url, loc + '/' + str(x) + ext)
            else:
                continue

    print 'Placed', str(x), 'pictures in:', loc

def main():
    url = raw_input('URL to PicRetrieve (google.com): ')
    url = 'http://' + url
    loc = raw_input('Location for PicRetrieve older ("." for here): ')
    html = Read(url)
    urls = Find('img src=".*?"',  html)
    print urls
    Retrieve(urls, loc)

if __name__ == '__main__':
    main()

import os, re, urllib

def Read(url):

Python convention is to name function lowercase_with_underscores

    uopen = urllib.urlopen(url)

I recommend against abbreviations

    uread = ''
    for line in uopen: uread += line
    return uread

Use uopen.read() it'll read the whole file

def Find(what, where):
    found = re.findall(what, where)
    match = []
    for i in found:

avoid single letter variable names

        i = i[9:-1]
        match.append(i)
    return match

Combine short lines of code

 for i in re.findall(what, where):
      match.append(i[9:-1])

I think its cleaner this way. Also use capturing groups rather then indexes.

def Retrieve(urls, loc):
    loc = os.path.realpath(loc)
    os.system('mkdir ' + loc +'/picretrieved')

Use os.mkdir rather than using system

    loc += '/picretrieved'

Use os.path.join to construct paths

    x = 0
    exts = ['.jpeg', '.jpg', '.gif', '.png']
    for url in urls:
        x += 1

use for x, url in enumerate(urls):

        for i in exts:
            ext = re.search(i, url)

Don't use regular expressions to search for simple strings. In this case I think you really want to use url.endswith(i). Also, stop using i everywhere.

            if ext != None:
                ext = ext.group()
                urllib.urlretrieve(url, loc + '/' + str(x) + ext)
            else:
                continue

This continue does nothing

    print 'Placed', str(x), 'pictures in:', loc

You don't need to use str when you are using print. It does it automatically.

def main():
    url = raw_input('URL to PicRetrieve (google.com): ')
    url = 'http://' + url
    loc = raw_input('Location for PicRetrieve older ("." for here): ')
    html = Read(url)
    urls = Find('img src=".*?"',  html)

Generally, using an html parser is preferred to using a regex on html. In this simple case you are ok.

    print urls
    Retrieve(urls, loc)

if __name__ == '__main__':
    main()


Sort the characters in a UnicodeString The task: sort the characters in a string, as provided by the ICU UnicodeString. This is because I want to be able to find anagrams using the suggestion from "Programming Pearls", which is, find "signatures" for each word in the dictionary, then sort according to these. It is enough if it works for the European languages and scripts.

The input file is for example the word list found in /usr/share/dict/words. For now, I just read from standard input, print them to standard output, sort the characters in the word, and print the sorted word:

$ cat sort-each-word.cpp
#include <iostream>
#include <algorithm>
#include "unicode/ustream.h"
#include "unicode/unistr.h"
#include "unicode/schriter.h"

int main()
{
    icu::UnicodeString word;
    while (std::cin >> word) {
        std::cout << word << '\t';
        auto n = word.length();
        UChar *begin = word.getBuffer(n+1);
        UChar *end = begin + n;
        std::sort(begin, end);
        *(begin+n) = 0;
        word.releaseBuffer(n);
        std::cout << word << '\n';
    }
}

To compile it:

g++  -pedantic -Wall -O2 --std=c++0x    -L/usr/lib -licui18n -licuuc -licudata   -licuio   sort-each-word.cpp   -o sort-each-word

Then, for example:

$ cat naivete
naiveté
$ < naivete ./sort-each-word
naiveté aeintvé

I feel most uncomfortable about getBuffer(n+1), where n is the current string length. I need the extra space to terminate the sorted string, but must I check that length() + 1 =< getCapacity()?

Other suggestions and comments are of course most welcome.

You should not need the n + 1, since the UnicodeString keeps track of the string's length, no matter whether it is null-terminated or not. It's in the documentation, you just need to read it.

So the line *(begin+n) = 0; is unnecessary. Especially since you tell icu that the new length is n, which means that begin[n] will be ignored anyway.

A general remark: maybe the code is even easier to read like follows:

auto len = word.length();
UChar *buf = word.getBuffer(len);
std::sort(buf, buf + len);
word.releaseBuffer(len);

I renamed n to len, renamed begin to buf and removed end altogether. Given the idiomatic C++ usage of the (begin, begin + len) pattern, it saves some code and makes the intention of the variables a little clearer.

If you want to support full unicode, you should not use UChar, since that is only a UTF-16 code unit and therefore not able to handle emojis, extended CJK ideographs, byzantine musical symbols and several more scripts starting at U+10000 and beyond.


Is `if( hidden ) return;` an antipattern? Sometimes, when you're doing something like displaying a bunch of elements, you can avoid running a function entirely if an object is hidden.

So you can either write the check from the loop:

for( Displayable * d : displayables )
    if( d->isShowing() )
        d->draw() ;

Or you can put an "early return" in d->draw() if d is hidden:

void Displayable::draw()
{
    if( !isShowing() )  return ;
    // continue with draw..
}

With this 2nd way, you don't have to check any condition when looping, so the loop "looks cleaner"

for( Displayable * d : displayables )
    d->draw() ;

But with the 1st way, you don't have to check isShowing() at the beginning of the function, so then that looks cleaner,

void Displayable::draw()
{
    // simply draw
}

Which is better?  Is the latter an antipattern of sorts?

There's a semantic difference between the two of these. In the first, the draw method always draws the object whether the object is visible or not. I.e. it is the caller's responsibility to check the visibility of the object. In the second, the object checks it's own visibility and recognises that it does not need to do anything if the object is hidden.

The difference in the loop, then, is the difference between saying "Draw all of the non-hidden elements", and "Draw all of the elements, (where draw means draw the element if it is visible)"

One focuses on the fact that we're drawing visible elements, the other adds the concept of visbility to the method draw.

In my opinion, the second of these better fits the semantic of what draw is supposed to do, and for that reason it is clearly better to perform the visibility check inside draw rather than inside the loop.


Calculate divisors using prime factorization For learning purpouses I wrote a class that is supposed to find all divisors for a given integer. My class makes use of primes and prime factorization to speed things up a bit.
What can I improve in my class?

Are there any microoptimiztations that could speed up the process? Any bad practices that should be avoided? I am by no means an expert at math, so feel free to be very nit-picky. :)

DivisorFinder Class

public class DivisorFinder
{
    /// <summary>
    /// The number that is searched for divisors
    /// </summary>
    private int _number;

    /// <summary>
    /// Search new divisors for a given integer
    /// </summary>
    /// <param name="number">Integer that is searched for divisors</param>
    public DivisorFinder(int number)
    {
        _number = number;
    }

    /// <summary>
    /// Find prime numbers up to a given maximum
    /// </summary>
    /// <param name="upperLimit">The upper limit</param>
    /// <returns>All prime numbers between 2 and upperLimit</returns>
    private IEnumerable<int> FindPrimes(int upperLimit)
    {
        var composite = new BitArray(upperLimit);
        var sqrt = (int) Math.Sqrt(upperLimit);

        for (int p = 2; p < sqrt; ++p)
        {
            if (composite[p])
                continue;

            yield return p;

            for (int i = p * p; i < upperLimit; i += p)
                composite[i] = true;
        }

        for (int p = sqrt; p < upperLimit; ++p)
            if (!composite[p])
                yield return p;
    }

    /// <summary>
    /// Search prime factors in a given IEnumerable of prime numbers
    /// </summary>
    /// <param name="primesList">The IEnumerable of previously calculated prime numbers</param>
    /// <returns>IEnumerable of prime factors</returns>
    private IEnumerable<(int, int)> FindPrimeFactors(IEnumerable<int> primesList)
    {
        foreach (var prime in primesList)
        {
            if (prime * prime > _number)
                break;
            int count = 0;
            while (_number % prime == 0)
            {
                _number = _number / prime;
                count++;
            }

            if (count > 0)
                yield return ((prime, count));
        }

        if (_number > 1)
            yield return (_number, 1);
    }

    /// <summary>
    /// Find all divisors of the target number
    /// </summary>
    /// <returns>IEnumerable of integers that divide the target without reminder</returns>
    public IEnumerable<int> FindDivisors()
    {
        var primes = FindPrimes((int) (Math.Sqrt(_number) + 1));
        var factors = FindPrimeFactors(primes);
        var divisors = new HashSet<int> {1};

        foreach (var factor in factors)
        {
            var set = new HashSet<int>();
            for (int i = 0; i < factor.Item2 + 1; i++)
            {
                foreach (int x in divisors)
                {
                    set.Add((int) Math.Pow(x * factor.Item1, i));
                }
            }

            divisors.UnionWith(set);
        }

        return divisors;
    }
}

Usage

var number = 2147483644;
var divisorFinder = new DivisorFinder(number);
var divisors = divisorFinder.FindDivisors();
Console.WriteLine($"Divisors: [{string.Join(", ", divisors)}]");

Performance for this example: ~6 milliseconds on a i5-6600K @ 3.50GHz

Credits:

The class was inspired by the python example from Bruno Astrolino.

Furthermore the prime sieve function was adapted from Dennis_E.

Bugs

FindPrimes is one of the better sieve implementations I've seen posted on this site, but it has an out-by-one bug. Here's some test code, which you can tidy up in a unit testing framework if you want to do things properly:

    var df = new DivisorFinder(72);
    int[] knownPrimes = new int[] { 2, 3, 5, 7, 11, 13, 17, 19, 23, 29, 31, 37, 41, 43, 47 };
    for (int limit = 1; limit <= 50; limit++)
    {
        var expected = knownPrimes.Where(p => p < limit);
        var observed = df.FindPrimes(limit);
        if (!expected.SequenceEqual(observed)) Console.WriteLine($"Error for limit {limit}");
    }

FindDivisors has a more alarming bug. What are the divisors of 72? According to this code, they include 144...

Structure

FindPrimes should be static: nothing in that method depends on instance fields or properties.

FindPrimeFactors modifies state, so it's not idempotent. Calling FindDivisors twice on the same object should give the same results, but it doesn't.

Frankly, this class is an example of OO purism at its worst. There's no good reason for any of the methods to be non-static.

IMO the dependency relationship between FindDivisors and FindPrimeFactors is wrong: FindPrimeFactors should be responsible for calculating which primes it needs to use. Consider that if you want to have multiple methods which use the prime factors in different ways, you shouldn't have to copy-paste the line

    var primes = FindPrimes((int) (Math.Sqrt(_number) + 1));

into every single one.


Sorting seems to be slower with 2 threads instead of 1 I'm implementing a simple merge sort to practice with Python's Threads. I've tried to split the job into two threads, that sort normally, then I can join them and merge the result. However, it takes almost twice as long with two threads than it does with the "normal" algorithm. Did I do something wrong or is it normal ? How can I improve this code to make it run faster with two threads ? What advice would you give me ? Thanks a lot.

CODE

# -*- encoding: utf-8 -*-

from collections import deque
from random import randint
from time import clock
from itertools import islice
from threading import Thread

sort_dict = dict()

def merge(da, db):
    res = deque()
    a = da.popleft()
    b = db.popleft()
    while True:
        if a < b:
            res.append(a)
            try:
                a = da.popleft()
            except IndexError:
                res.append(b)
                res.extend(db)
                break
        else:
            res.append(b)
            try:
                b = db.popleft()
            except IndexError:
                res.append(a)
                res.extend(da)
                break
    return res

def sort(d, put_in_global = False, nb = 0):
    l = len(d)
    if l <= 1:
        return d
    else:
        da = deque(islice(d, 0, l/2))
        db = deque(islice(d, l/2, l))
        sda = sort(da)
        sdb = sort(db)
        if put_in_global:
            sort_dict[nb] = merge(sda, sdb)
        else:
            return merge(sda, sdb)

def multi_sort(d):
    l = len(d)
    if l <= 1:
        return d
    else:
        da = deque(islice(d, 0, l/2))
        db = deque(islice(d, l/2, l))
        sda = Thread(target=sort, args=(da, True, 1))
        sdb = Thread(target=sort, args=(db, True, 2))
        sda.start()
        sdb.start()
        sda.join()
        sdb.join()
        return merge(sort_dict[1], sort_dict[2])

def check_sorted(d):
    if d:
        s = d.popleft()
    while True:
        try:
            f = s
            s = d.popleft()
            if f > s:
                return False
        except IndexError:
            return True

if __name__=='__main__':
    with open('results.txt', 'wb') as f:
        lengths = [2*10**i for i in xrange(1, 7)]
        f.writelines(['Length  One_time  Multi_time  Ratio\r\n'])
        for length in lengths:
            print '----------------------------'
            print 'List length : ' + str(length)
            lst = [randint(0, 10**4) for i in xrange(length)]
            # print 'List : ' + str(lst)
            print 'ONE THREAD'
            start = clock()
            res = sort(lst)
            one_time = clock() - start
            print 'Sorting time (s) : ' + str(one_time)
            print 'Sort checking : ' + str(check_sorted(res))
            print 'MULTIPLE THREAD'
            start = clock()
            res = multi_sort(lst)
            multi_time = clock() - start
            print 'Sorting time (s) : ' + str(multi_time)
            print 'Sort checking : ' + str(check_sorted(res))
            print 'CONCLUSION'
            print 'Time ratio multi/one : ' + str(multi_time/one_time)
            f.writelines([str(length)+'  '+
                          str(one_time)+'  '+
                          str(multi_time)+'  '+
                          str(multi_time/one_time)+'\r\n'
                          ])

results.txt (changed the display a little bit because it was ugly)

Length  One_time          Multi_time        Ratio
20      0.00010350001112  0.00121833484164  11.7713498623
200     0.00101618192736  0.00266847549331  2.62598204265
2000    0.0117950095317   0.0210672419328   1.7861148714
20000   0.134505249988    0.2356253311      1.7517928194
200000  1.46486474003     2.59513874989     1.77158933448
2000000 16.4877353912     31.8037152227     1.92893168577

This looks like a case of GIL. For actual multiprocessing you should use the multiprocessing module.

  multiprocessing is a package that supports spawning processes using an API similar to the threading module. The multiprocessing package offers both local and remote concurrency, effectively side-stepping the Global Interpreter Lock by using subprocesses instead of threads. Due to this, the multiprocessing module allows the programmer to fully leverage multiple processors on a given machine. It runs on both Unix and Windows.


Angular JSON weather app I'm pretty new to Angular. I wrote a weather app and need my code reviewed. I especially need help on best practices for handling edge cases on the response from the JSON endpoint.

Here's the HTML:

<!DOCTYPE html>

    <meta charset="utf-8">
    <meta name="viewport" content="initial-scale=1, maximum-scale=1, user-scalable=no, width=device-width">
    <title></title>

    <link href="lib/ionic/css/ionic.css" rel="stylesheet">
    <link href="css/style.css" rel="stylesheet">

    <ion-pane>
        <ion-header-bar class="bar-calm">

Kansas City Weather

        </ion-header-bar>
        <ion-content>

                        Day {{ $index + 1 }}

                                {{ x.temp.day }} &#8451;

                                Max : {{ x.temp.max }} &#8451;

                                Min : {{ x.temp.min }} &#8451;

        </ion-content>
    </ion-pane>

Here's the app.js :

// Ionic Starter App

// angular.module is a global place for creating, registering and retrieving Angular modules
// 'starter' is the name of this angular module example (also set in a  attribute in index.html)
// the 2nd parameter is an array of 'requires'
var app = angular.module('starter', ['ionic'])

    .run(function($ionicPlatform) {
    $ionicPlatform.ready(function() {
    // Hide the accessory bar by default (remove this to show the accessory bar                  above the keyboard
    // for form inputs)
    if(window.cordova && window.cordova.plugins.Keyboard) {
      cordova.plugins.Keyboard.hideKeyboardAccessoryBar(true);
    }
    if(window.StatusBar) {
      StatusBar.styleDefault();
    }
    });
    })

      app.controller('apiController', function($scope, $http) {
    $http.get("http://api.openweathermap.org/data/2.5/forecast/daly?q=Kansas+City&mode=json&units=metric&cnt=10&appid=f6b7081abd94e66273817ed6b5ce95c7").then(function(response) {
        $scope.myData = response.data.list;
    });
});

Your JS could need improvement in terms of indentation. Other than that, your app is pretty small for a full-blown review. However... we can make improvements.

In the long run, your app will be more than just one controller to display different parts of the site. Most of them will be consuming more or less the same data, just displayed a bit differently. You might want to consider using factories/services as data stores, and have controllers just pull them in as dependencies.

This also means your API calls will live there as well, freeing the controller to only be a means to expose data from the services/factories to the templates as well a communicate from template to services/factories.


Stopwatch exception implementation in C# I'm new to C# programming and has been following a certain intermediate tutorial that has asked me to write a stopwatch simulation code that includes an exception system:

  Exercise 1: Design a Stopwatch

  Design a class called Stopwatch. The job of this class is to simulate a stopwatch. It should provide two methods: Start and Stop. We call the start method first, and the stop method next. Then we ask the stopwatch about the duration between start and stop. Duration should be a value in Timespan. Display the duration on the console.

  We should also be able to use a stopwatch multiple times. So we may start and stop it and then start and stop it again. Make sure the duration value each time is calculated properly.

  We should not be able to start a stopwatch twice in a row (because that may overwrite the initial start time). So the class should throw an InvalidOperationException is it's started twice.

  Educational tip: The aim of this exercise is to make you understand that a class should be always in a valid state. We use encapsulation and information hiding to achieve that. The class should not reveal its implementation detail. It only reveals a little bit, like a black box. From the outside, you should not be able to misuse a class because you shouldn't be able to see the implementation detail.

Transcribed from:

I have implemented code that does this exactly, however, I want to learn how to optimize my code to use fewer lines and make it more readable.

using System;

namespace Stopwatch_Training
{
    class Program
    {
        public class Stopwatch
        {
            private DateTime _startTime;
            private DateTime _stopTime;
            private TimeSpan _duration;
            public DateTime StartTime()

            {
                _startTime = DateTime.Now;
                return _startTime;
            }

            public DateTime Stoptime()

            {
                _stopTime = DateTime.Now;
                return _stopTime;
            }
        }

        static void Main(string[] args)

        {
            var stopwatch = new Stopwatch();
            int input1;
            DateTime starTime;
            DateTime endTime;
            double duration;
            int counter = 0;

            while (true)

            {
                Console.Write("1-Start Timer\n2-Stop Timer\n");
                input1 = Convert.ToInt32(Console.ReadLine());
                if (input1 == 1)
                {
                    starTime = stopwatch.StartTime();
                    counter = 1;
                }
                else
                {
                    throw (new InvalidOperationException("Please Start the timer with 1"));
                }

                Console.WriteLine("Please enter '2' to stop the timer whenever you want");
                input1 = Convert.ToInt32(Console.ReadLine());
                if (input1 == 2)
                {
                    endTime = stopwatch.Stoptime();
                    counter = 0;

                }

                else
                {
                    throw (new InvalidOperationException("Please end the timer with 2"));
                }
                if (counter == 1)
                {
                    throw (new InvalidOperationException("stuff"));
                }
                duration = (endTime - starTime).TotalSeconds;
                if (duration > 60.0)
                {
                    duration = (endTime - starTime).TotalMinutes;
                    Console.WriteLine(duration + "m");
                }
                else
                {

                    Console.WriteLine(duration + "s");
                }

            }
        }
    }
}

You have the beginning of a stopwatch representation—you have its data. You still need to add its behavior.

Approach your design of the class as if the class should stand on its own. Input/output will be handled by something else, like the console, but the stopwatch should be the authority on its internal state.

The simplest stopwatch has a start/stop button and a display. We'll interpret "having a display" to mean "a way to get the stored elapsed time".

This means we'll need to provide three methods:

Start: Sets the stopwatch in a running state.
Stop: Sets the stopwatch in an idle state.
TimeElapsed: Reads out the currently elapsed time.

Additionally, Start and Stop have the requirement that they should throw InvalidOperationException when the stopwatch is not a compatible state. Our stopwatch will need a notion of running to do this.

Let's look how we can modify the stopwatch class to adopt this behavior. I've annotated some below:

    public class Stopwatch
    {
        private DateTime _startTime; // we need this
        private DateTime _stopTime; // ? either this or duration
        private TimeSpan _duration;
        public DateTime StartTime() // why do we return a value here?

        {
            // we still need to check whether we are already running
            _startTime = DateTime.Now;
            return _startTime;
        }

        public DateTime Stoptime() // why do we return a value here?

        {
            // we still need to check whether we are still running
            _stopTime = DateTime.Now;
            return _stopTime;
        }

        // We still need the notion of 'currently running'
    }

    // First attempt
    public class Stopwatch
    {
        private DateTime _startTime;
        private DateTime _stopTime;
        private Boolean _running;

        public void StartTime()
        {
            if ( _running )
            {
                throw new InvalidOperationException("Cannot start: already running");
            }
            _startTime = DateTime.Now;
            _running = true;
        }

        public void Stoptime()
        {
            if ( !_running )
            {
                throw new InvalidOperationException("Cannot stop: not running");
            }

            _stopTime = DateTime.Now;
            _running = false;
        }

        public TimeSpan ElapsedTime()
        {
            if ( _running )
            {
                return DateTime.Now - _startTime;
            }
            else
            {
                return _stopTime - _startTime;
            }
        }
    }

Note that we check if the stopwatch is currently running when asking for elapsed time: otherwise, we're going to get a very strange reading.

Now we can 'clean up' a little bit to follow conventions:

Class members are implicitly private, so we don't need to add the private modifier.
Members should start with a capital letter.

Which gives us:

    public class Stopwatch
    {
        DateTime StartTime;
        Boolean IsRunning;
        DateTime StopTime;

        public void Start()
        {
            if (IsRunning)
            {
                throw new InvalidOperationException("Cannot start: already running");
            }
            StartTime = DateTime.Now;
            IsRunning = true;
        }

        public void Stop()
        {
            if (!IsRunning)
            {
                throw new InvalidOperationException("Cannot stop: not running");
            }
            IsRunning = false;
            StopTime = DateTime.Now;
        }

        public TimeSpan ElaspedTime()
        {
            if (IsRunning)
            {
                return DateTime.Now - StartTime;
            }
            else
            {
                return StopTime - StartTime;
            }
        }
    }

And a quick Main to play around with it:

    static void Main(string[] args)
    {
        var stopwatch = new Stopwatch();
        int choice;

        do
        {
            Console.Write("1-Start Timer\n2-Stop Timer\n3-Read Timer\n0-Quit\n");
            choice = Convert.ToInt32(Console.ReadLine());
            switch (choice)
            {
                case 1: stopwatch.Start(); break;
                case 2: stopwatch.Stop(); break;
                case 3: Console.WriteLine(stopwatch.ElaspedTime()); break;
            }
        } while (choice != 0);
    }


Disabling child scaling, when parent scales with Math I think this code is only simple math, but I'm posting it, because almost everyone fakes the scaling with unparenting, then parenting again.

Vector3 originalParentScale = new Vector3;
Vector3 originalScale = new Vector3;
void Awake(){
    originalScale = transform.localScale;
    originalParentScale = transform.parent.localScale;
}
void Update(){
    transform.localScale = (originalParentScale / transform.parent.localScale) * originalScale;
}

I think it can be even improved, so I would be happy if someone can improve it. It's challenge too, so happy programming :-)

I'm not very familiar with Unity3D, but are you absolutely 100% certain that transform.parent.localScale will never be 0? Otherwise, you should check for that before dividing.


Multiplication in terms of addition Given the following task:

  Exercise 1.17

  The exponentiation
  algorithms in this section are based
  on performing exponentiation by means
  of repeated multiplication. In a
  similar way, one can perform integer
  multiplication by means of repeated
  addition. The following multiplication
  procedure (in which it is assumed that
  our language can only add, not
  multiply) is analogous to the expt
  procedure:

(define (* a b)
  (if (= b 0)
      0
      (+ a (* a (- b 1)))))

  This algorithm takes a number of steps
  that is linear in b. Now suppose we
  include, together with addition,
  operations double, which doubles an
  integer, and halve, which divides an
  (even) integer by 2. Using these,
  design a multiplication procedure
  analogous to fast-expt that uses a
  logarithmic number of steps.

I wrote this solution:

(define (double a) (* a 2))
(define (halve a) (/ a 2))
(define (even n) (= (remainder n 2) 0))

(define (times a b)
  (cond ((= 1 b) a)
        ((even b) (times (double a) (halve b)))
        (else (times (+ a a) (- b 1)))))

What do you think?

Your implementation will not produce correct results in general.

In the base case of b = 0, result should be 0.  In the case of even b, result is double a times half b (which you have done correctly).  In the case of odd b, result should be a + (double a times half (b - 1)).

(define (times a b)
  (cond ((= 0 b) 0)
        ((even b) (times (double a) (halve b)))
        (else (+ a (times (double a) (halve (- b 1)))))))

To make this definition iterative, add an accumulator, like so:

(define (times a b)
  (times-iter 0 a b))

(define (times-iter acc a b)
  (cond ((= 0 b) acc)
        ((even b) (times-iter acc (double a) (halve b)))
        (else (times-iter (+ a acc) (double a) (halve (- b 1))))))


Do I need to delete a vector? I want to make a graph from input from a text file in the format:

vertex1 vertex2 cost time

where vertex1 and vertex2 are strings that form an edge and cost and time are the properties of that edge. This information will be used to construct the Graph based on cost. I want to plan out the project before coding it so I have a good idea of what to do. So far I decided on the following classes:

class Edge {
private:
  int vertex1;
  int vertex1;
  int cost;
  int time;

public:
  Edge(int v1, int v2, int cost, int time) {
    this->vertex1 = v1;
    this->vertex2 = v2;
    this->cost = cost;
    this->time = time;
  }

  ~Edge();

  int getEdgeCost();

  int getEdgeTime();
};

class Graph {
private:
  vector<Edges> edge; //edges in graph

public:
  Graph() : edge(0) {}

  ~Graph(){}

  int getTotalCost();

  int getTotalTime();

  void addEdge(Edge e);

  Graph getMinSpanTree();
};

class Vertex {
private:
  string name;
  int number;
  bool visited;
  vector<Edges> connEdges;  // vector of all edges connected to this vertex

public:
  Vertex(string name, int number) {
    this->name = name;
    this->number = number;
    visited = true;
  }

  ~Vertex();

  string getName();

  int getNum();

  bool wasVisited();

  Graph getShortestPath();
};

int main(int argc, char* argv[]) {

  std::string ifileName = std::string(argv[1]);

  ifstream ifile;
  if(ifile)
    ifile.open(ifileName);

  Edge edge;
  Graph* p_graph;
  Vertex v;

  string vert1, vert2;
  int cost, time;

  int i = 1; // numbers each vertex

  while(ifile.good()) {
  // read in input, put it in vars above, skipped for brevity

  // make the vertices
  v1 = new Vertex(vert1, i);
  i++; // increments vertex number
  v2 = new Vertex(vert2, i);
  i++;

  // make the edge
  edge = new Edge(v1.getNum(), v2.getNum(), cost, time);

  // add it to graph
  p_graph->addEdge(edge);

}

Is my design correct or is it lacking something? Can it be improved?

Overall

Resolve all standard library entities with std::. Never put using namespace declarations in a header file, where most of his belongs.

Class Edge

You use initializer lists in some places (Graph), not in others. Be consistent. I advise using them always where they're needed, and not at all where they're not.  Also, there is no need for a destructor in this class unless you're expecting polymorphism and need virtual-dtor for a vtable. Finally, make your getters const

class Edge {
private:
  int vertex1;
  int vertex2;
  int cost;
  int time;

public:
  Edge(int v1, int v2, int cost, int time)
    : vertex1(v1), vertex2(v2), cost(cost), time(time)
  {
  }

  int getVertex1() const { return vertex1; }
  // etc...
};

Class Graph

There are outright compilation bugs in this class. There is no such entity as Edges, yet you have a vector of them. Further, properly named it should be edges are a vector of Edge. Also, the initializer list in this class is pointless, as is the destructor and, as-written, the constructor. Finally, you may (or may not) want to pass your incoming edge as a const-ref (lots of opinions on that, I know).

class Graph {
private:
  vector<Edge> edges; //edges in graph

public:
  int getTotalCost();
  int getTotalTime();
  void addEdge(const Edge& e);

  Graph getMST();   // get Minimum Spanning Tree
};

Class Vertex

As previously mentioned, use initializer lists and full std:: namespace resolution. Like Graph, this uses Edges as a typename, but no such type exists. Again, we use const qualifiers on all getters, here also returning the name by const-ref rather than value-copy. Finally, no chance of ever setting visited to true unless you provide a setter for it, nor accessing or modifying the edges vector without providing some means to do so.

class Vertex
{
private:
    std::string name;
    int number;
    bool visited;
    std::vector<Edge> connEdges;  // vector of all edges connected to this vertex

public:
    Vertex(const std::string& name, int number)
        : name(name), number(number), visited(false)
    {
    }

    // edge accessors
    void addEdge(const Edge& edge) { connEdges.push_back(edge); }
    const std::vector<Edge>& getEdges() const { return edges; }

    // properties
    const std::string& getName() const { return name; }
    int getNum() const { return number; }
    bool wasVisited() const { return visited; }
    void setVisited(bool val = true) { visited = val; }

    Graph getShortestPath();
};

main()

This function is simply loaded with errors. Among them:

edge  is not a pointer type, so that assignment won't event compile.
p_edge is derferenced though it is never initialized, so that will likely outright-fault due to undefined behavior
ifile is never opened before use, so nothing will be read there.
it leaks memory, but only because of an error previously mentioned above. Were that coded correctly this would be a non-issue.


Improving readability of non-recursive depth first search function in Lisp As a free-time activity in order to learn some Lisp I had to implement depth first directed graph search. Due to large graph size (800K nodes, 5M arcs) recursion-based approach I could devise didn't work.

Below is my stack-based implementation that I would like to improve in terms of readability if nothing else. Any comments on style, use of Lisp idioms, code smell are welcome.

(defun depth-first-search (adjacency-array visit-function &optional (init-function nil init-function-supplied-p))
  (let* ((num-nodes (array-dimension adjacency-array 0))
         (init most-negative-fixnum)
         (nodes-seen (make-array num-nodes :element-type 'fixnum :initial-element init))
         (results (make-array num-nodes :element-type 'fixnum :initial-element init)))
    (labels ((visited-p (node) (not (eql init (aref results (1- node)))))
             (seen-p (node) (not (eql init (aref nodes-seen (1- node)))))
             (mark-seen (node) (setf (aref nodes-seen (1- node)) 1))
             (mark-visited (node)  (setf (aref results (1- node)) (funcall visit-function)))
             (visit (node)
               (unless (visited-p node)
                 (when init-function-supplied-p (funcall init-function node))
                 (mark-seen node)
                 (loop with stack = (list node)
                       until (null stack)
                       for head = (first stack)
                       for tails = (aref adjacency-array (1- head))
                       for fresh-tails = (unless (visited-p head)
                                           (loop for tail in tails
                                                 unless (or (visited-p tail)
                                                            (seen-p tail))
                                                 collect tail))
                       do (if fresh-tails
                            (progn
                              (mapcar #'mark-seen fresh-tails)
                              (setq stack (append fresh-tails stack)))
                            (progn
                              (pop stack)
                              (unless (visited-p head) (mark-visited head))))))))
      (loop for node from 1 to num-nodes
            unless (or (null node)
                     (visited-p node))
            do (visit node)
            finally (return results)))))

Some details:

adjacency-array for a graph 1->2;1->3;3->1 would be #((2 3) nil (1)).
visit-function returns a "visited" value for a node.
init-function makes necessary actions when new subgraph search starts.

This usage of LOOP is not supported by ANSI Common Lisp. You can't have an UNTIL clause (only in a main clause in LOOP syntax) before a FOR clause (a variable clause).

See: Macro LOOP.


How to add forms dynamically in javascript? Example speaks for it self:

function writeSpecialTabContentForTtm(){
        document.write('
');
        document.write('<form  id="ttmsetpointrun" action="" style="text-align:right">');
        document.write('<input style="float:bottom;" type="submit" value="Move to Set Point" />');
        document.write('</form>');
        document.write('
');

How this should be done? There is a form that is chosen by user. To be precise, user choses a tab and when creating a tab the above function is called. This is something that is changed frequently, so it would be nice to write it in a separate file.

edit:

so, in my case it would be:

var newdiv = document.createElement('div');
newdiv.style = "float:right;width:250px;min-height:200px;display:block;border-width: 1px 1px 1px";
document.body.appendChild(newdiv);

var newform = document.createElement('form');
newform.id = "ttmsetpointrun";
newform.action="";
newform.style="text-align:right";
newdiv.appendChild(newform);

var newinput = document.createElement('input');
newinput.style = "float:bottom;";
newinput.type = "submit";
newinput.value = "Move to Set Point";
newform.appendChild(newinput);

To create dynamic forms and elements in javascript is easy.

var newdiv = document.createElement('div'); // Creates the div
document.body.appendChild(newdiv);          // Adds the div to body

After that, you can access newdiv and change attributes of it, such as

newdiv.className = "helloworld";


Simple python COM server to expose scipy integration to VBA I want to create a COM server to expose scipy to VBA code, as performing integration is kinda difficult and I don't know of any good VBA/COM libraries for this. My objectives are:

As portable as possible
Fast
Maintainable

Here's the python:
# Requires C:\Users\...\anaconda3\Library\bin on the PATH
from scipy import integrate
from win32com.client import Dispatch
from typing import Callable
import comtypes

class ScipyWrapper:
    # pythoncom.CreateGuid()
    _reg_clsid_ = "{A621AB64-9378-4B54-A7D3-20711C0B72DB}"
    _reg_desc_ = "Python Scipy COM Server"
    _reg_progid_ = "PythonCOM.ScipyWrapper"
    _reg_clsctx_ = comtypes.CLSCTX_LOCAL_SERVER # don't want inproc because conda not supported on 64 bit
                                                # see https://stackoverflow.com/q/67462529/6609896

    _public_methods_ = ["quad",]
    # _public_attrs_ = []
    # _readonly_attrs_ = []

    def quad(self, vbFunc, a: float, b: float) -> float:
        vb_dispatch = Dispatch(vbFunc)
        func: Callable[[float], float] = vb_dispatch.Evaluate  # Callable[..., float]?
        return integrate.quad(func, a, b)[0]  # only return val, not uncertainty

if __name__ == "__main__":
    import win32com.server.register

    win32com.server.register.UseCommandLine(ScipyWrapper)

Called from VBA (twinBasic) like:
Module Main
    Public Sub doStuff()
        Dim integrator as Object = CreateObject("PythonCOM.ScipyWrapper")
        Dim func as IOneDimensionalFunction = New XSquared 'this is what we'll integrate
        Debug.Print integrator.quad(func, 0., 1.) 'integrate x^2 from 0->1, returns 0.333333...
    End Sub
End Module

'@Description("Interface for a f(x) -> y function, with optional parameters")
Class IOneDimensionalFunction
    Public Function Evaluate(ByVal x As Double, ParamArray parameters() as Variant) As Double
    End Function
End Class

Class XSquared
    Implements IOneDimensionalFunction
    Private Function IOneDimensionalFunction_Evaluate(ByVal x As Double, ParamArray parameters() as Variant) As Double
        Return x^2
    End Function
End Class

Questions:

Not sure how to Type the python, vbFunc is a PyIDispatch instance but not sure what that means for typing/mypy.
I'd like to use strong typing to protect my python, right now it's duck typed and I could easily pass an Interface without the Evaluate method, or anything else for that matter.
No idea how to handle python errors and turn them into COM errors.
Is this ref-count safe?

OFC any other tips on general style, use of comments and naming etc. are welcome :)

Deferred Imports
Why is import win32com.server.register only imported during __main__? It should be inexpensive and clean to just leave it at the top with everything else. I'd also de-namespace from win32com.server.register import UseCommandLine but that's mostly a matter of taste.
Stubbing
You ask:

vbFunc is a PyIDispatch instance but not sure what that means for typing/mypy

If PyIDispatch is an importable type, then simply
def quad(self, vbFunc: PyIDispatch, a: float, b: float) -> float:

If it's not importable at all (this can happen for some libraries that are aggressively dynamic), then you basically have two options:

Declare an opaque, named NewType to make it clear to callers that this is not just "anything"
Do the work that the library should have done for you, and stub it yourself using a custom .pyi

Call protocols
# Callable[..., float]?

You're right to be confused, and mypy is still fairly terrible at annotating variadic callables. Here you should consider using Protocol:
class DispatchCall(typing.Protocol):
    def __call__(self, x: float, *parameters: COMCompatibleTypes) -> float:
        ... # literally an ellipsis

# Later:

func: DispatchCall = vb_dispatch.Evaluate

Unpacking
Re.
return integrate.quad(func, a, b)[0]  # only return val, not uncertainty

it's worth noting that uncertainty is not the only thing quad returns; there's actually y, abserr, infodict, message and explain.
It's minor, but I typically prefer to do at least a partial unpack to make it a little more clear what's happening:
y, *_ = integrate.quad(func, a, b)

Given the tuple length I don't think I'd do this, but you can if you think it adds clarity:
y, abserr, infodict, message, explain = integrate.quad(func, a, b)


Search strings within a list from strings within a dictionary with the least complexity I have the following input:

newValue = [['men', 'John Doe,Chris perry'], ['women', 'Mary,Anne,Lita']] - list within list

keywords_list = [('MEN', 'john,zack')]    - tuple in a list (Dictionary on which .items() is used.)

Expected Output is:

newValue = [['men', 'John Doe'],['women','']]

What I want to do here is extract those names for Men which is common in both newValue and keywords_list.

Below is my working implementation that gets the desired result:

      allValues = []
      for item in newValue:
            for key in keywords_list:
                if str(item[0])[1:].upper() == key[0]:
                    ValueSplit = item[1].split(',')
                    keyWordSplit = key[1].split(',')
                    for EverySplit in ValueSplit:
                        for everyKey in keyWordSplit:
                            if everyKey.lower() in EverySplit.lower():
                                pass
                            else:
                                ValueSplit.remove(EverySplit)
                    item[1]= ValueSplit
      allValues.append(item)

This isn't an efficient way of solving the problem. If anyone could recommend an efficient way with lesser complexity to achieve the desired result, it would be really helpful.

both your datastructures can be cast into a dictionary directly:

>>> dict(newValue)
{'men': 'John Doe,Chris perry', 'women': 'Mary,Anne,Lita'}

>>> dict(keywords_list)
{'MEN': 'john,zack'}

from there you can just do a split based on , and compare in lowercase.


Measuring execution time in C++ I had time measuring pretty much figured out when I ended up using this structure.

#include <iostream>
#include <chrono>

template<typename TimeT = std::chrono::milliseconds>
struct measure
{
    template<typename F>
    static typename TimeT::rep execution(F const &func)
    {
        auto start = std::chrono::system_clock::now();
        func();
        auto duration = std::chrono::duration_cast< TimeT>(
            std::chrono::system_clock::now() - start);
        return duration.count();
    }
};

a use case would be

struct functor
{
    int state;
    functor(int state) : state(state) {}
    void operator()() const
    {
        std::cout << "In functor run for ";
    }
};

void func()
{
    std::cout << "In function, run for " << std::endl;
}

void func(int arg) {}

int main()
{
    int dummy(1);

    std::cout << measure<>::execution( [&]() {
        func(dummy);
    }) << std::endl;

    std::cout << measure<>::execution(functor(dummy)) << std::endl;
std::cout << measure<>::execution(func);

    return 0;
}

But since I started using my struct in various posts on Stack Exchange sites, I've had remarks and comments about the code (like on func being a const ref or not etc).

I'd like a review on the above and your suggestions on how I can approve this code (or leave it alone at last).

Extend your timer function so it can take all the parameters needed by func()
Don bother to pass func by const reference.

What I would have done
template<typename TimeT = std::chrono::milliseconds>
struct measure
{
    template<typename F, typename ...Args>
    static typename TimeT::rep execution(F func, Args&&... args)
    {
        auto start = std::chrono::system_clock::now();

        // Now call the function with all the parameters you need.
        func(std::forward<Args>(args)...);

        auto duration = std::chrono::duration_cast< TimeT>(std::chrono::system_clock::now() - start);

        return duration.count();
    }
};


My iPod is slower than my iPad Many of the variables in my 2D mining game were declared as constants like so:

DWConstants.h

static const int kBlockSpriteWidth = 64;
static const float kBufferFactor = 2.5;
static const int kMaxBlockDamage = 3;

This was all well and good for initial testing, but I soon found out that there were quite a few variables that I wanted to be able to adjust based on what device the game was running on.  I thought about a couple approaches to this, and I decided on this one.  The values of the variables are set based on the memory of the device.  I would love to know what you think.

DWMemoryConstants.h

#import <Foundation/Foundation.h>

@interface DWMemoryConstants : NSObject

@property int startingChunks;
@property int chunkSize;
@property int chunksToGenerate;
@property float maxZoom;

@end

DWMemoryConstants.m

#import "DWMemoryConstants.h"

@implementation DWMemoryConstants

-(instancetype) init {
    self = [super init];
    if (self) {

        unsigned long long memorySize = [NSProcessInfo processInfo].physicalMemory;
        unsigned long long memSize = memorySize/1000/1000;

        if (memSize < 256) {
            _startingChunks = 0;
            _chunkSize = 0;
            _chunksToGenerate = 0;
            _maxZoom = 0.00;
        } else if (memSize >= 256 && memSize < 800) {
            //ipod 5 is in this range
            _startingChunks = 8;
            _chunkSize = 16;
            _chunksToGenerate = 4;
            _maxZoom = 0.45;
        } else if (memSize >= 800 && memSize < 1800) {
            //ipad 4 will fall in this range
            _startingChunks = 10;
            _chunkSize = 18;
            _chunksToGenerate = 6;
            _maxZoom = 0.25;
        } else if (memSize >= 1800 && memSize < 2800) {
            //previous typical settings
            _startingChunks = 10;
            _chunkSize = 24;
            _chunksToGenerate = 8;
            _maxZoom = 0.05;
        } else if (memSize >= 2800) {
            _startingChunks = 12;
            _chunkSize = 30;
            _chunksToGenerate = 10;
            _maxZoom = 0.04;
        }
    }
    return self;
}

@end

As always, I am open to any sort of feedback about the code.

I think using memory size is an interesting idea. You may want to define the values  with an expression (e.g. startingChunks = round(memSize / 600) * 2 + 8), rather than an else-if chain, tho if you want to target specific devices, an if-else chain is acceptable.

Old:

if (memSize < 256) {
    _startingChunks = 0;
    _chunkSize = 0;
    _chunksToGenerate = 0;
    _maxZoom = 0.00;
} else if (memSize >= 256 && memSize < 800) {
    //ipod 5 is in this range
    _startingChunks = 8;
    _chunkSize = 16;
    _chunksToGenerate = 4;
    _maxZoom = 0.45;
}

Underscores are annoying, is there any reason you use them in those variable names? If it's supposed to denote constants, doesn't the name work well enough? It shouldn't be doubted that chunkSize, for instance, won't change if you know how the app works. And if in a future update you want it to change, you could change it without renaming.
if (memSize < 256) {} else {} – in the else block, memSize must be >= 256. So there's really no need to add that to the else if.

With my changes:

if (memSize < 256) {
    startingChunks = 0;
    chunkSize = 0;
    chunksToGenerate = 0;
    maxZoom = 0.00;
} else if (memSize < 800) {
    //ipod 5 is in this range
    startingChunks = 8;
    chunkSize = 16;
    chunksToGenerate = 4;
    maxZoom = 0.45;
}


Reviewing the file download controller in spring rest In my project, so this controller is working perfectly, my objective is to read the log file from the server which is created on daily basis, so I have to read the log file which is heavier in size around maybe 2GB, so my question is that please review the below code and advise my is my code is smart enough to read such heavy log file, or should I need to do some modifications in it to read such file in chunks and also look if any modifications I need to introduce into from performance point of view or exceptional handling point of view and from REST practices point of view
@GetMapping("/download")
@ResponseStatus(HttpStatus.OK)
public ResponseEntity<Resource> download(@RequestParam String filename) {
    File file = new File(serverLocation + File.separator + filename + EXTENSION);

    HttpHeaders header = new HttpHeaders();
    header.add(HttpHeaders.CONTENT_DISPOSITION, "attachment; filename=" + filename + EXTENSION);
    header.add("Cache-Control", "no-cache, no-store, must-revalidate");
    header.add("Pragma", "no-cache");
    header.add("Expires", "0");

    Path path = get(file.getAbsolutePath());
    Resource resource = null;
    try {
        resource = new InputStreamResource(Files.newInputStream(path));
    } catch (IOException e) {
        LOGGER.error(String.format("Exception occurred while reading the log file%s", e));
    }
    return ResponseEntity.ok()
            .headers(header)            .contentLength(file.length())
            .contentType(MediaType.parseMediaType("application/octet-stream"))
            .body(resource);

}

In my opinion, there are two ways with Spring to do that; your way is one of them (using the Spring Resource), and the other is to inject the file bytes directly in the javax.servlet.http.HttpServletResponse.
Your version can be improved, since you can use the FileSystemResource instead of the InputStreamResource.
The InputStreamResource is good when there are no other options available, as stated in the Java documentation since there are lots of other implementations to use.
Should only be used if no specific Resource implementation is applicable[...]

The FileSystemResource will remove the need to open the file and make the code shorter.
HttpHeaders header = new HttpHeaders();
header.add(HttpHeaders.CONTENT_DISPOSITION, "attachment; filename=" + filename + EXTENSION);
header.add("Cache-Control", "no-cache, no-store, must-revalidate");
header.add("Pragma", "no-cache");
header.add("Expires", "0");

File file = new File(serverLocation + File.separator + filename + EXTENSION);
Resource resource = new FileSystemResource(file);

return ResponseEntity.ok()
.headers(header)
.contentLength(file.length())
.contentType(MediaType.parseMediaType("application/octet-stream"))
.body(resource);

The thing that concerns me with your code, is the potential Path Traversal vulnerability that you have when reading the file, from the parameter. If I pass the filename value of ..\..\..\system.log, this could potentially be a problem, because we could download file out of the scope of your application.
File file = new File(serverLocation + File.separator + filename + EXTENSION);

One way to prevent that is to compare the base path / location of the file with the serverLocation, if the path is different, you ignore the request.


Reuseable C++11 Thread I'm learning about threading support in C++. I've got a basic understanding of thread-pools and why creating and destroying threads on some systems can be expensive. As far as I'm aware C++11 doesn't have any built in support for worker/background threads. std::thread is only designed to perform one task which you set in the constructor; it then destroys the thread after the task is complete.

At the moment I have no use for a fully fledged thread-pool but I would like to be able to reuse a thread for different tasks after I create it. I've created a simple class that will accept any function or method wrapped in a lambda and will then execute it exactly once. The reusable thread object should not destruct until the work function is fully complete.

This is the implementation:

class Reusable_Thread
{
    public:
        Reusable_Thread()
            : m_thread_pause(true), m_thread_quit(false),
              m_thread(&Reusable_Thread::thread_worker, this)
        { }

        ~Reusable_Thread()
        {
            m_thread_quit = true;
            m_thread.join();
        }

        bool get_readiness() const { return m_thread_pause; }

        bool set_work(const std::function<void()>& work_func)
        {
            if (get_readiness())
            {
                m_work_func = work_func;
                return true;
            }
            else
            {
                return false;
            }
        }

    private:
        std::atomic<bool> m_thread_pause;
        std::atomic<bool> m_thread_quit;
        std::thread m_thread;
        std::function<void()> m_work_func;

        void thread_worker()
        {
            while (!m_thread_quit)
            {
                if (!m_thread_pause)
                {
                    m_work_func();
                    m_thread_pause = true;
                }
            }
        }
};

And this is some example usage:

int main()
{
    Reusable_Thread thread;

    auto f1 = [&] () { /* do some work */ };
    thread.set_work(f1);

    while (thread.get_readiness() == false)
    {
        // do some other stuff
    }

    // set some new work
    auto f2 = [&] () { /* do some new work */ };
    thread.set_work(f2);

    // maybe call f1 again, depends if f2 has finished yet
    if (!thread.set_work(f1))
    {
        // thread too busy to call f1, call it on the main thread
        f1();
    }

    // thread won't destruct until work has finished
    return 0;
}

Are there any problems with implementing a reusable thread this way?

A nice way to warm your cup of tea

  Are there any problems with implementing a reusable thread this way?

Yes. Let's have a look at a slightly different main:

int main()
{
    Reusable_Thread thread;

    std::this_thread::sleep_for(std::chrono::seconds(10));

    return 0;
}

This should just wait for 10 seconds and then exit, right?

$ g++ reusablethread.cc -std=c++11 -pthread -o demo
$ /usr/bin/time ./demo
10.00user 0.00system 0:10.00elapsed 99%CPU (0avgtext+0avgdata 3148maxresident)k
0inputs+0outputs (0major+129minor)pagefaults 0swaps

Nope. It actually runs full throttle. This is due to your choice of

void thread_worker()
{
    while (!m_thread_quit)
    {
        if (!m_thread_pause)
        {
            /* ... */
        }
    }
}

This isn't the fault of std::atomic<>, but your design. If we don't have something to do, we shouldn't check immediately afterwards. Instead, we should wait. More on that later.

howdoiturnthison

Your class is currently missing features to toggle m_thread_pause, which makes it hard to reason about other behaviour. I guess you're just missing m_thread_pause = false in set_work, e.g.

bool set_work(const std::function<void()>& work_func)
{
    if (get_readiness())
    {
        m_work_func = work_func;
        m_thread_pause = false;     // here, but wrong, see below
        return true;
    }
    else
    {
        return false;
    }
}

Furthermore, the name get_readiness is misleading. What is ready? The internal thread? Is the work slot free? is_paused is much better.

Race condition m_work_func and m_thread_pause

Assume we have our Reusable_Thread rthread and it's currently paused. We have access on our rthread from two threads A and B. Both call set_work. What happens?

void foo(){ puts("foo"); }
void bar(){ puts("bar"); }

void thread_Alice(Reusable_Thread & rthread) {
   rthread.set_work(foo);
}

void thread_Bob(Reusable_Thread & rthread) {
   rthread.set_work(bar);
}

int main(){
    Reusable_Thread rthread;
    std::thread A(thread_Alice, std::ref(rthread));
    std::thread B(thread_Alice, std::ref(rthread));
    A.join();
    B.join();
    std::this_thread::sleep_for(std::chrono::seconds(1));
    return 0;
}

It's certainly possible that the result is foo. It's also possible that the result is bar. But it's also possible that it's neither and your program crashes:

We create the std::thread in rthread, we call it R
We create the std::threads A and B. Neither run yet.
A gets scheduled. It calls set_work and:

checks m_thread_pause (get_readiness())
sets the worker to foo
yields due to scheduling

B gets scheduled. It calls set_work and

checks m_thread_pause (get_readiness(), still true)
yields

A gets scheduled. It resumes the execution and

sets m_thread_pause to false
returns

A's thread of execution has ended.
R gets scheduled. It

checks m_thread_quit (is false)
checks m_thread_pause (is false due to A)
starts reading the std::function<> m_work_fun.
yields while reading

B gets scheduled. It finishes its call to set_work:

sets the worker to bar, although R currently reads from it
yields

R gets scheduled. It finishes reading m_work_fun and ends up with a corrupt std::function<...> and uses operator(). Your program crashes.
Even if your program didn't crash, after R sets m_thread_pause to true, B gets scheduled and finishes set_work, thus setting m_thread_pause to false and the corrupt function gets called again.

So depending on where the threads get interrupted, you end up with either:

bar
foo
bar
bar
foo
foo
foo
bar
bar
foo
*crash*

So, how does one get rid of this? Usually, you would use std::atomic::exchange or std::atomic::compare_exchange on m_thread_pause:

bool set_work(const std::function<void()>& work_func)
{
    if (m_thread_pause.exchange(false)) // old value was true
    {
        m_work_func = work_func;
        return true;
    }
    else
    {
        return false;
    }
}

But that doesn't work either, because R could start using m_work_func before it's actually set. The underlying problem here is that m_work_func and m_thread_pause are intertwined.

Third wheel to the rescue

We can decouple this by using a third atomic<bool>, that checks whether the function can be set safely:

bool set_work(const std::function<void()>& work_func)
{
    if (!m_function_ready && m_thread_pause.exchange(false))
    {
        m_work_func = work_func;
        m_function_ready = true;
        return true;
    }
    ...
}

Remember, only one thread will see m_thread_pause as true, due to the atomic exchange, so we're save from the previous A/B race.

void thread_worker()
{
    while (!m_thread_quit)
    {
        if (!m_thread_pause && m_function_ready)
        {
            m_work_func();
            m_thread_pause = true;
            m_function_ready = false;
        }
    }
}

Here, both the m_thread_pause and m_function_ready are checked, and the checks make sure that our function is actually set.

The memory ordering std::memory_order_seq_cst could be slightly relaxed (not to be confused with memory_order_relaxed!), but it's not trivial to get this right.

However, remember the problem in the first section of this review? You're still using active polling. A small sleep can help here:

void thread_worker()
{
    while (!m_thread_quit)
    {
        if (!m_thread_pause && m_function_ready)
        {
            m_work_func();
            m_thread_pause = true;
            m_function_ready = false;
        } else {
            std::this_thread::sleep_for(std::chrono::milliseconds(1));
        }
    }
}

Note that still uses more CPU power than it has to. Which brings us to condition variables.

Wait until notified

Condition variables provide a way to get notified instead of actively checking conditions. They have to be used with a mutex, so they have a  overhead compared to atomic operations. Still, they have their use.

In this case, you would add a std::mutex and a std::condition_variable to your class:

private:
    std::mutex m_mtx;
    std::condition_variable m_cv;

In set_work, you notify all thread that are currently waiting:

bool set_work(const std::function<void()>& work_func)
{
    if (!m_function_ready && m_thread_pause.exchange(false))
    {
        m_work_func = work_func;
        m_function_ready = true;

        m_cv.notify_all();       // <- notify all waiting threads

        return true;
    }
    ...
}

And worker_thread, you wait if m_thread_pause is false:

void thread_worker()
{
    std::unique_lock<std::mutex> lock(m_mtx);  // <- lock the mutex

    while (!m_thread_quit)
    {
        if (!m_thread_pause && m_function_ready)
        {
            m_work_func();
            m_thread_pause = true;
            m_function_ready = false;
        } else {
            m_cv.wait(lock);  // <- wait
        }
    }
}

Note that the std::unique_lock is placed outside the while loop to keep the number of lock aquires/releases to a minimum.

However, we have to make sure that the thread wakes up when we want to stop it, so we have to change the destructor slightly:

    ~Reusable_Thread()
    {
        m_thread_quit = true;
        m_cv.notify_all();
        m_thread.join();
    }

TL;DR

Make sure that you don't introduce race conditions and avoid while(true){if(false){ … }}. Use proper names for methods and check the behavior on non-trivial examples.


Async HTTP client the right way I am using an CloseableHttpAsyncClient the following way:

public class AsyncHttpClient {

    private static final Logger logger = LogManager.getLogger(Main.class);

    private static CloseableHttpAsyncClient HTTP_ASYNC_CLIENT = null;
    private static HttpPost HTTP_POST = null;
    private static HttpContext HTTP_CONTEXT;

    private static PropertiesLoader PROP_LOADER = new PropertiesLoader();
    private static Properties PROPERTIES = PROP_LOADER.initProp("./config/config.properties");
    private static final int CON_REQUEST_TIMEOUT = Integer.valueOf(PROPERTIES.getProperty("connectionrequesttimeout"));
    private static final int CON_TIMEOUT = Integer.valueOf(PROPERTIES.getProperty("connecttimeout"));
    private static final int CON_SOCKET_TIMEOUT = Integer.valueOf(PROPERTIES.getProperty("sockettimeout"));
    private static final String CON_URL = PROPERTIES.getProperty("conurl");
    private static final int CONMANAGER_MAX_TOTAL = Integer.valueOf(PROPERTIES.getProperty("conmanagermaxtotal"));
    private static final int DEFAULT_MAX_PER_ROUTE = Integer.valueOf(PROPERTIES.getProperty("conmanagermaxdefaultperroute"));

    //Reusable connection
    private static RequestConfig createConnConfig() {

        return RequestConfig.custom()
                .setConnectionRequestTimeout(CON_REQUEST_TIMEOUT)
                .setConnectTimeout(CON_TIMEOUT)
                .setSocketTimeout(CON_SOCKET_TIMEOUT).build();
    }

    private static PoolingNHttpClientConnectionManager createPoolingConnManager() throws IOReactorException {
        IOReactorConfig config = IOReactorConfig.DEFAULT;
        ConnectingIOReactor ioReactor = new DefaultConnectingIOReactor(config);

        PoolingNHttpClientConnectionManager cm = new PoolingNHttpClientConnectionManager(ioReactor);
        cm.setMaxTotal(CONMANAGER_MAX_TOTAL);
        cm.setDefaultMaxPerRoute(DEFAULT_MAX_PER_ROUTE);

        return cm;
    }

    private static synchronized CloseableHttpAsyncClient getCloseableClient() throws IOReactorException {

        if (HTTP_ASYNC_CLIENT == null) {
            logger.info("New Async Client created ");
            HTTP_ASYNC_CLIENT = HttpAsyncClientBuilder.create()
                    .setDefaultRequestConfig(createConnConfig())
                    .setConnectionManager(createPoolingConnManager()).build();

            HTTP_ASYNC_CLIENT.start();
        }
        return HTTP_ASYNC_CLIENT;
    }

    private static synchronized HttpPost getPost() {
        if (HTTP_POST == null) {
            logger.info("New Post created ");
            HTTP_POST = new HttpPost(CON_URL);
            HTTP_POST.setHeader("Accept", "application/json");
            HTTP_POST.setHeader("Content-type", "application/json");
        }
        return HTTP_POST;
    }

    public static void postData(String data) throws Exception {
        //TODO - needs cleanup
        StringEntity entity = new StringEntity(data);
        HttpPost httpPost = getPost();
        httpPost.setEntity(entity);
        getCloseableClient().execute(httpPost, HTTP_CONTEXT, new FutureCallback<HttpResponse>() {
            @Override
            public void completed(HttpResponse httpResponse) {
                logger.info("Completed");

            }

            @Override
            public void failed(Exception e) {
                logger.info("Completed with exception " + e);
            }

            @Override
            public void cancelled() {
                logger.info("Cancelled ");
            }
        });
    }

}

Throughout the program I want only one instance to be alive, I am also using multiple threads to post data using the above class.

Thread safety

You want to have only one instance of CloseableHttpAsyncClient which is why you use static method and static variable, right ?

Sadly, using only static methods and global states is usually a bad idea (impossible to inject object... and thus you can't mock in your unit test, it also  increases the coupling between your objects)

If you still want to use static methods, the synchronized method for the HttpAsyncClient creation make it safe against thread race and you are guaranteed to have only one HttpAsyncClient instance so you have that guarantee but you should consider using Dependency Injection instead.

Code format

The indentation is good IMO, however...

private static CloseableHttpAsyncClient HTTP_ASYNC_CLIENT = null;
private static HttpPost HTTP_POST = null;
private static HttpContext HTTP_CONTEXT;

private static PropertiesLoader PROP_LOADER = new PropertiesLoader();
private static Properties PROPERTIES = PROP_LOADER.initProp("./config/config.properties");

You should only use SCREAMING_SNAKE_CASE in java when the field is static final and using the object don't have any side-effects. Here it's not only non-final, CloseableHttpAsyncClient and PropertiesLoader are full of side-effects so SCREAMING_SNAKE_CASE is better avoided.