Periodically run Bash Job A to conditionally restart Bash Job B based on output of Bash Job A Is there any neater way I can accomplish the task (as in title)? Also, I'm not sure why asyncio.Task.all_tasks() explodes and whether this is bad or not.

from concurrent.futures import ProcessPoolExecutor
import asyncio
from contextlib import contextmanager
import random

@contextmanager
def event_loop():
    loop = asyncio.get_event_loop()
    with ProcessPoolExecutor(3) as executor:
        loop.set_default_executor(executor)
        yield loop
    loop.close()

async def start_long_proc(delay=0, *, started_event):
    await asyncio.sleep(delay)
    cmd = 'while true; do date >> temp.log; sleep 0.1; done;'
    proc = await asyncio.create_subprocess_shell(cmd)
    started_event.set()
    return proc

async def periodic_monitor():
    while True:
        n = random.randrange(100)
        cmd = f'echo "{n}"'
        proc = await asyncio.create_subprocess_shell(
            cmd, stdout=asyncio.subprocess.PIPE)
        stdout, stderr = await proc.communicate()
        yield int(stdout.decode().rstrip())

async def f():
    started_event = asyncio.Event()

    # run Bash Job B
    long_proc = await start_long_proc(started_event=started_event)

    p_terminate = 10
    monitor_interval = 1

    async for n in periodic_monitor():  # periodically run Bash Job A
        print(f'n={n}')
        if n < p_terminate:
            print('terminated and reschedule long proc')
            long_proc.terminate()  # kill Bash Job B
            started_event.clear()
            long_proc = await start_long_proc(5, started_event=started_event)
        await asyncio.sleep(monitor_interval)

if __name__ == '__main__':
    with event_loop() as loop:
        asyncio.ensure_future(f())
        loop.run_forever()

Is there any neater way I can accomplish the task?

        long_proc.terminate()  # kill Bash Job B

Well, sending SIGTERM is effective, but since you're in control of what Job B runs you might prefer to have it do IPC so it gracefully shuts down after completing a processing phase, rather than being interrupted at some arbitrary point. Could be as simple as creating a sentinel /tmp file that B looks for.

A bigger issue is that you're not doing wait4() child, like this:

        long_proc.terminate()
        long_proc.wait()

Instead you wrote a race with a 5-second delay to encourage the child to win. That long sleep implies wasted idle time. You could run the CPU flat out by synchronizing with exiting child.


Scala type matching map I was working with some complicated generics system, and I found the need to make an abstraction for a typesafe mapping between K[T] and V[T] where T is the same for individual pairs.

In the case where the key or the value is T itself, we use a trick: type Identity[T] = T, and set the parameter to Identity. I have provided type aliases to make this usage more clear.

I would appreciate critiques on how I can make my code more robust, elegant, efficient, or idiomatic

/**
  * A map where the keys and values accept a type parameter, with a bound, and key-value pairs in the map have the same
  * type parameter.
  */
class TypeMatchingMap[K[_ <: B], V[_ <: B], B](private val contents: Map[Any, Any]) {
  def +[T <: B](kv: (K[T], V[T])): TypeMatchingMap[K, V, B] =
    new TypeMatchingMap[K, V, B](contents + kv)

  def -(k: K[_]): TypeMatchingMap[K, V, B] =
    new TypeMatchingMap[K, V, B](contents - k)

  def apply[T <: B](k: K[T]): V[T] =
    contents(k).asInstanceOf[V[T]]

  def get[T <: B](k: K[T]): Option[V[T]] =
    contents.get(k).map(_.asInstanceOf[V[T]])

  type Pair[T <: B] = (K[T], V[T])
  def toSeq: Seq[Pair[_]] =
    contents.toSeq.map(_.asInstanceOf[Pair[_]])

  override def toString: String = contents.toString

  override def equals(obj: scala.Any): Boolean =
    if (obj.isInstanceOf[TypeMatchingMap[Identity, Identity, _]])
      obj.asInstanceOf[TypeMatchingMap[Identity, Identity, Any]].contents == this.contents
    else false

  override def hashCode(): Int = contents.hashCode()
}

object TypeMatchingMap {
  type Identity[T] = T
  type Identified[K[_ <: B], B] = TypeMatchingMap[K, Identity, B]
  type Identifying[V[_ <: B], B] = TypeMatchingMap[Identity, V, B]

  private val _empty = new TypeMatchingMap[Identity, Identity, Any](Map.empty)
  def empty[K[_ <: B], V[_ <: B], B]: TypeMatchingMap[K, V, B] = _empty.asInstanceOf[TypeMatchingMap[K, V, B]]
}

object TypeMatchingMapTest extends App {
  sealed trait NumBox
  type BoxID[T <: NumBox] = UUID

  case class FloatBox(n: Float) extends NumBox
  case class DoubleBox(n: Double) extends NumBox
  case class IntBox(n: Int) extends NumBox

  var map: Identified[BoxID, NumBox] = TypeMatchingMap.empty[BoxID, Identity, NumBox]

  val id5f: BoxID[FloatBox] = UUID.randomUUID()
  map = map + (id5f -> FloatBox(5f))

  val id6d: BoxID[DoubleBox] = UUID.randomUUID()
  map = map + (id6d -> DoubleBox(6.0))

  val id7i: BoxID[IntBox] = UUID.randomUUID()
  map = map + (id7i -> IntBox(7))

  val f5: FloatBox = map(id5f)
  val d6: DoubleBox = map(id6d)
  val i7: IntBox = map(id7i)

  println(map)
  println(map.toSeq)
}

A use case? Say you have a map of entities, which are organized by UUIDs. The problem is that the type system does now acknowledge what kind of Entity an EntityID is pointing to. So, you can make a parametrized alias:

type EntityID[E <: Entity] = UUID

And store your entities in an Identified[EntityID, Entity]. Then, if you have, say an EntityID[Cow], you can pass it to the map and get your Cow.

Thank you, and for your troubles, a related image for your enjoyment:

   https://i.stack.imgur.com/ZT7o0.jpg

It's been a couple of months so maybe you've moved on, but I had a look through this today. I also may have read too much into your example use case, so apologies if this isn't helpful.

If like in your example we can restrict the scope down to cases where K is covariant, and we can also remove the higher-kindedness of the value type, then we can improve the type safety significantly. So much so that we can escape all of the asInstanceOf calls!

class TypeMatchingMap[K[+_ <: B], B](private val contents: Map[K[B], B]) {
  def +[T <: B](kv: (K[T], T)): TypeMatchingMap[K, B] =
    new TypeMatchingMap[K, B](contents + kv)

  def -[T <: B](k: K[T]): TypeMatchingMap[K, B] =
    new TypeMatchingMap[K, B](contents - k)

  def apply[T <: B : ClassTag](k: K[T]): T =
    contents(k) match {
      case t: T => t
      case _ => throw ??? // Insert throwable here
    }

  def get[T <: B : ClassTag](k: K[T]): Option[T] =
    contents.get(k).flatMap {
      case t: T => Some(t)
      case _ => None
    }

  def toSeq: Seq[(K[B], B)] = contents.toSeq

  override def toString: String = contents.toString()

  override def equals(obj: Any): Boolean =
    obj match {
      case m: TypeMatchingMap[_, _] => m.contents == this.contents
      case _ => false
    }

  override def hashCode(): Int = contents.hashCode()
}

object TypeMatchingMap {
  def empty[K[+_ <: B], B]: TypeMatchingMap[K, B] =
    new TypeMatchingMap[K, B](Map.empty)
}

object TypeMatchingMapTest extends App {
  sealed trait NumBox
  type BoxID[+T <: NumBox] = UUID

  case class FloatBox(n: Float) extends NumBox
  case class DoubleBox(n: Double) extends NumBox
  case class IntBox(n: Int) extends NumBox

  var map: TypeMatchingMap[BoxID, NumBox] = TypeMatchingMap.empty[BoxID, NumBox]

  val id5f: BoxID[FloatBox] = UUID.randomUUID()
  map = map + (id5f -> FloatBox(5f))

  val id6d: BoxID[DoubleBox] = UUID.randomUUID()
  map = map + (id6d -> DoubleBox(6.0))

  val id7i: BoxID[IntBox] = UUID.randomUUID()
  map = map + (id7i -> IntBox(7))

  val f5: FloatBox = map(id5f)
  val d6: DoubleBox = map(id6d)
  val i7: IntBox = map(id7i)

  println(map)
  println(map.toSeq)
}

If you can't make the assumptions I've made above, I'd still recommend changing the signature of - to def -[T <: B](k: K[T]): TypeMatchingMap[K, V, B]. Consider the following:

val map: TypeMatchingMap[K, V, Int] = ???
val k: K[String] = ???
map - k

We know that the last line won't ever do anything useful, so in my mind it would be kinder to prevent it from compiling so as to prevent any client of the API from making a mistake.

Happy to discuss the code more, but didn't want to go into too much detail if you're no longer interested.


Renaming numbered files for proper sorting I have designed a tool that generates some HTML files and a CSS file in a directory, which then feed into a tool called Jutoh that uses these to make eBooks.

However, my output files are of the form 1.htm, 2.htm and so on. Jutoh ranks 10.htm to preceed 2.htm and therefore, I wrote a method to convert 1.htm into 0001.htm. I would like to know your opinion on it and learn where there might be scope for speedup and other improvements.

var inputDir = _cssFile.Directory.FullName + "\\Output\\";
var outputDir = _cssFile.Directory.FullName + "\\Output";
if (!Directory.Exists(outputDir))
    Directory.CreateDirectory(outputDir);
DirectoryInfo di = new DirectoryInfo(inputDir);
foreach (FileInfo fi in di.GetFiles("*.htm"))
{
    string n = fi.Name.Replace(fi.Extension, ""); //remove extension
    int k = Int32.Parse(n); //Convert string to int
    string t = k.ToString("D4"); //convert it into 4 digit
    t = t + fi.Extension; // add extension

    string trgtfile = Path.Combine(fi.DirectoryName, t);
    File.Move(fi.FullName, trgtfile);
}

Here are several tips that might help write better/cleaner code. Note that my tips or help might not be perfect but it gives you an idea on how to write better code.

Naming you variables

Using variable names like k and n are very bad practice. Not only will other people not understand your code, if you haven't looked at your code for a while, you might have forgotten what each variable does/stands for. Give your variables a meaningful name. So after applying this, your code looks like this:

string currentName = fi.Name.Replace(fi.Extension, "");
int nameAsNumber = Int32.Parse(currentName);
string newName = nameAsNumber.ToString("D4");
newName = newName + fi.Extension;

string targetFile = Path.Combine(fi.DirectoryName, newName);
File.Move(fi.FullName, targetFile );

Over-creating variables

Storing operations/calculations on variables in new variables can be handy and even necessary in certain situations. But in this situation where you take a string, parse it to an int and make it a string again, I suggest you do this in one line. You can even add the extension in the same line:

string currentName = fi.Name.Replace(fi.Extension, "");
string newName = int.Parse(currentName).ToString("D4") + fi.Extension;

This does not mean you have to write all your code in one line, this an example of doing too much in one line:

string targetFile = Path.Combine(fi.DirectoryName, int.Parse(Path.GetFileNameWithoutExtension(fi.FullName)).ToString("D4") + fi.Extension);

The above line does exactly the same but it's not easy to read and/or maintain.

Re-inventing the wheel (part 1)

Why create methods/functions/lines of code that do something that's already been made for you. If you want to get the filename without the extension, use the Path.GetFileNameWithoutExtension() method.

string currentName = Path.GetFileNameWithoutExtension(fi.FullName);

Re-inventing the wheel (part 2)

Remember, the .NET framework contains classes and methods for almost anything. And this also goes for adding zeros (or other characters) to a string when they do not yet meet a desired length. This is done with the Pad() method, and in your case the String.PadLeft(Int32, Char) method. You simply provide the desired length and the character that will be put in place. Your code will become:

string currentName = Path.GetFileNameWithoutExtension(fi.FullName);
string newName = currentName.PadLeft(4, '0') + fi.Extension;

Your complete inside-loop code will now look like:

string currentName = Path.GetFileNameWithoutExtension(fi.FullName);
string newName = currentName .PadLeft(4, '0') + fi.Extension;
string targetFile = Path.Combine(fi.DirectoryName, newName);
File.Move(fi.FullName, targetFile);

I hope this helps! :)


A recursive_transform for std::array with various return type This is a follow-up question for A recursive_transform for std::vector with various return type. Thanks to G. Sliepen provide further review suggestions. After digging into the topic of achieving a more generic recursive_transform function in both various output type and various container type, I still have no simple solution. However, based on G. Sliepen's answer, the case of the std::vector, std::deque and std::list container types may be resolved. I am trying to implement additional another overload recursive_transform function for std::array. Here's my implementation.
template<class T, std::size_t S, class F>
auto recursive_transform(std::array<T, S>& input, const F& f)
{
    using TransformedValueType = decltype(recursive_transform(*input.cbegin(), f));

    std::array<TransformedValueType, S> output;
    std::transform(input.cbegin(), input.cend(), output.begin(),
        [f](auto& element)
        {
            return recursive_transform(element, f);
        }
    );
    return output;
}

The test case of std::array:
//  std::array<int, 10> -> std::array<std::string, 10>
std::array<int, 10> test_array;
for (int i = 0; i < 10; i++)
{
    test_array[i] = 1;
}
auto recursive_transform_result5 = recursive_transform(
    test_array,
    [](int x)->std::string { return std::to_string(x); });                          //  For testing
std::cout << "string: " + recursive_transform_result5.at(0) << std::endl;

Here's the Godbolt link. The code in this link including the test cases for std::vector, std::deque and std::list.
All suggestions are welcome.
The summary information:

Which question it is a follow-up to?
A recursive_transform for std::vector with various return type

What changes has been made in the code since last question?
The handleable container type in the previous version of recursive_transform function is std:vector. With G. Sliepen's answer, this handleable container type list is extended to std::vector, std::deque and std::list. Then, I am trying to deal with std::array here.

Why a new review is being asked for?
In this version of recursive_transform function, it seems that working well in the mentioned test case std::array<int, 10> -> std::array<std::string, 10>. However, there is some issues when it comes to the more complex case like std::array<std::array<int, 10>, 10> -> std::array<std::array<std::string, 10>, 10> (the scalability to be improved!) If there is any suggestion or possible idea about this, please let me know.

Ensure you pass by const reference consistently
The main problem with your code was that you didn't make the std::array overload take the input by const reference:
auto recursive_transform(std::array<T, S>& input, const F& f)

Just add const! See this Godbolt link with the nested std::arrays working.
Prefer using std::begin() and std::end()
It doesn't really matter if you only want to support containers from the standard library, but when writing templates, prefer using std::begin() and std::end() instead of ->begin() and ->end(). The advantage is that if you are using a non-standard container somewhere that doesn't provide begin() and end() member functions, it will still be possible to overload the out-of-class std::begin() and std::end() functions to add iterator support to that class. If you use it in your algorithm templates, then your algorithms will also support those non-standard classes.


It there a pattern used to process a same function but assign the result to a different variable regarding to a different condition? Say I have a code snippet like this

    if(string.Compare(isStandardFlag,"STANDARD",true)==0)
    {
        hearingID = centreSession.standardID;
        centreSession.standardOutcomeID
            = disciplineSvc.SaveResultA(hearingID,OffenceID, Outcome);
        centreSession.standardOutcomeNoteID
            = disciplineSvc.SaveResultB(hearingID, txt1.Text, "ResultHearing");
    }
    else{
        hearingID = centreSession.noneStandardID;
        centreSession.noneStandardHearingOutcomeID
            = disciplineSvc.SaveResultA(hearingID,OffenceID, Outcome);
        centreSession.noneStandardHearingOutcomeNoteID
            = disciplineSvc.SaveResultB(hearingID, txt1.Text, , "ResultHearing");
    }

You can see the 'if' and 'else' do the same thing but just assign the result to different variables.

Is there a pattern I can refer to refactor the code?

First, it looks like the ID, OutcomeID and OutcomeNoteID belong together, so I think it it makes sense to extract them into a separate class (though my naming of it is just a guess):

class Hearing
{
    public int ID { get; private set; }

    public int OutcomeID { get; private set; }

    public int OutcomeNoteID { get; private set; }

    public Hearing(int id)
    {
         ID = id;
    }

    public void SetOutcome(int outcomeId, int outcomeNoteId)
    {
        OutcomeID = outcomeId;
        OutcomeNoteID = outcomeNoteId;
    }
}

With that, your code could be changed to something like:

Hearing hearing;
if (string.Equals(isStandardFlag, "STANDARD", StringComparison.InvariantCultureIgnoreCase))
    hearing = centreSession.StandardHearing;
else
    hearing = centreSession.NonStandardHearing;

hearing.SetOutcome(
    disciplineSvc.SaveResultA(hearing.ID, OffenceID, Outcome),
    disciplineSvc.SaveResultB(hearing.ID, txt1.Text, "ResultHearing"));

Some additional notes:

I have also changed string.Compare() == 0 to string.Equals(), because I think it expresses the intent better.
Some of the names are quite bad (e.g. txt1 or SaveResultA), you should improve those.
Dealing with ids a lot is not very object-oriented. I think it would make more sense to deal directly with Outcome and OutcomeNote objects, not just their ids.


Minimalistic, self-contained class for reading a sectionless INI file When sections are ignored and no writing capability is needed, an INI reader becomes quite simple to implement. Here is my attempt at reading a sectionless INI file without any non-std dependencies.

The general idea is to store all key-value pairs in a <string, string> map, and use getters to convert the value string to desired type on demand.

#include <fstream>
#include <iostream>
#include <map>
#include <string>

class SectionlessINIReader {

private:
    std::map<std::string, std::string> dict;

public:

    // https://stackoverflow.com/a/1798170/3516684
    static std::string trim(const std::string& str,
                 const std::string& whitespace = " \t")
    {
        const auto strBegin = str.find_first_not_of(whitespace);
        if (strBegin == std::string::npos)
            return ""; // no content

        const auto strEnd = str.find_last_not_of(whitespace);
        const auto strRange = strEnd - strBegin + 1;

        return str.substr(strBegin, strRange);
    }

    SectionlessINIReader(const std::string& filename) {

        std::ifstream file(filename);
        std::string line;
        int idxEq;
        while (std::getline(file, line))
        {

            if ((line.find_first_not_of(";#")==0) &&
                ((idxEq = line.find("=")) != std::string::npos)) {

                    std::string key = trim(line.substr(0,idxEq));
                    std::string value = trim(line.substr(idxEq+1));

                    dict[key] = value;
                }
        }
        file.close();
    }

    std::string get(std::string key) {
        return dict[key];
    }

    bool getLong(std::string key, long* p_value) {

        std::string value = dict[key];

        if (value=="") {
            return false;

        } else {
            *p_value = std::stol(dict[key]);
            return true;
        }
    }

    // bool getDouble(...)

};

The intended usage is:

SectionlessINIReader ini("config.ini");
long age;
if (ini.getLong("age", &age))
    std::cout << "Name: " << ini.get("name") << "; age: " << age << std::endl;

Include (only) what you use

This class uses nothing from <iostream>, so it's a waste of the compiler's time to include it.

Control the public interface

Is trim() really a facility that should be a part of SectionlessINIReader?  I don't think it belongs in the public interface, and more likely belongs in a private or protected section.

Make it easier to use

The getLong() member function has an awkward interface, requiring the user to pass an lvalue (worse, a pointer).  Consider returning a std::optional<long> instead, to convey the result in a single value.

Pass strings by const reference

There's no need to make a copy of the key argument to the accessors - that should be declared as const std::string& key instead.

Accessors should be const

get() and getLong() both modify dict (they create a new empty mapping for any requested key that doesn't yet exist).  We don't want to do that, as it's wasteful.  We would have spotted this bug if we'd declared those members const (so that they could be used by code with only a const reference to the reader - that will be important if we later add writing functions).

We could write:

std::optional<std::string> get(std::string key) const
{
    auto const it = dict.find(key);
    if (it == dict.end())
        return {};

    return it->second;
}

And getLong() could use it:

std::optional<long> getLong(std::string key) const
{
    auto const value = get(key);
    if (!value.has_value())
        return {};

    return std::stol(value.value());
}

Open the code to extension

At present, the only way to add more types that can be read is to edit this class or to subclass it.  And it's not designed for subclassing (it has a non-virtual destructor), so it's closed to end-users.

One way to make it open for extension is to separate the string-to-value conversion from the string lookup, and allow users to specialize that part; our code could then use the appropriate specialization, or fail to compile if no conversion has been defined.

This would be the starting position:

namespace convert {

    // This function must be specialized for any extension types to
    // convert from string to value.
    template<typename T>
    std::optional<T> fromString(const std::string&);
}

It's used in our get<>() function quite simply:

template<typename T>
std::optional<T> get(const std::string& key) const
{
    auto const value = get(key);
    if (!value.has_value())
        return {};

    return convert::fromString<T>(value.value());
}

We could provide some common conversions:

template<>
std::optional<std::string> convert::fromString(const std::string& s)
{ return s; }

template<>
std::optional<long> convert::fromString(const std::string& s)
{ return std::stol(s); }

template<>
std::optional<double> convert::fromString(const std::string& s)
{ return std::stod(s); }

Users can add their own conversions in similar manner.

Variation

Instead of creating a free function to specialize, we could make fromString() be a private static member function of SectionlessINIReader.  The choice may well depend on whether you might want to use fromString() from anywhere else.

Think about exceptions

std::stol() will throw an exception if the value can't be parsed as a long.  Do we want to pass this straight to our callers, or do we want to treat malformed values the same as missing values, and simply return an empty std::optional?  (There's no right or wrong answer here; I just want to be sure that we've made a conscious choice.)

Avoid narrowing conversions

    int idxEq;
    ...
        if (((idxEq = line.find("=")) != std::string::npos)) {

Here, we assign the result of find() (a size type) to idxEq (a signed integer) and then compare idxEq to std::string::npos (a size type again).  That's a potentially narrowing conversion which could change the sign, and a comparison between types of different signedness.  I know we don't expect keys to be long enough to overflow int, but it would be nice to silence the compiler warning here, and it's very easy to make it consistent:

    std::string::size_type idxEq;

Check the result of filesystem operations

In the constructor, we call file.close(), but never check the result.  If we want to be sure that we read the complete file, we need to ensure that close() has succeeded.

If we really don't care about whether we missed some data, then we could simply omit the close() and let the file's destructor do that for us - but I don't advise ignoring a failed close.

Inefficient use of search

There's no need to search all the way through a string just to see what its first character is:

if (line.find_first_not_of(";#") == 0)

It's clearer to examine the first character directly (after checking that the line has at least one character, of course).

Check for duplicate keys

When reading, we simply overwrite any key that appears more than once.  Consider emitting a warning, or throwing an exception.

Remove dead code

There's a remnant of some old code still hanging around, which should be deleted:

// bool getDouble(...)

Modified code

#include <fstream>
#include <map>
#include <optional>
#include <stdexcept>
#include <string>

class SectionlessINIReader
{

private:
    std::map<std::string, std::string> dict = {};

public:
    SectionlessINIReader(const std::string& filename)
    {
        auto file = std::ifstream {filename};
        readStream(file);
    }

    SectionlessINIReader(std::istream& in)
    {
        readStream(in);
    }

    std::optional<std::string> get(const std::string& key) const
    {
        auto const it = dict.find(key);
        if (it == dict.end())
            return {};

        return it->second;
    }

    template<typename T>
    std::optional<T> get(const std::string& key) const
    {
        auto const value = get(key);
        if (!value.has_value())
            return {};

        return fromString<T>(value.value());
    }

private:

    // https://stackoverflow.com/a/1798170/3516684
    static std::string trim(const std::string& str,
                            const std::string& whitespace = " \t")
    {
        const auto strBegin = str.find_first_not_of(whitespace);
        if (strBegin == std::string::npos)
            return {}; // no content

        const auto strEnd = str.find_last_not_of(whitespace);
        const auto strRange = strEnd - strBegin + 1;

        return str.substr(strBegin, strRange);
    }

    template<typename T>
    static std::optional<T> fromString(const std::string&);

private:

    void readStream(std::istream& in)
    {
        std::string line;
        while (std::getline(in, line)) {
            if (line.empty() or line[0] == ';' or line[0] == '#') {
                continue;
            }
            auto const idxEq = line.find('=');
            if (idxEq == line.npos) {
                // consider reporting an error here
                continue;
            }

            auto const key = trim(line.substr(0,idxEq));
            auto const value = trim(line.substr(idxEq+1));
            if (!dict.emplace(key, value).second) {
                throw std::runtime_error("Duplicate key in INI file");
            }
        }
        if (!in.eof()) {
            throw std::runtime_error("Failed to read INI file");
        }
    }
};

// String converters
template<>
std::optional<std::string> SectionlessINIReader::fromString(const std::string& s)
{ return s; }

template<>
std::optional<long> SectionlessINIReader::fromString(const std::string& s)
{ return std::stol(s); }

template<>
std::optional<double> SectionlessINIReader::fromString(const std::string& s)
{ return std::stod(s); }

// we can add more specializations here, and user may also add
// specializations.

#include <iostream>
#include <sstream>
int main()
{
    std::istringstream contents("age = 30\n"
                                "favourite_drink = tea\n"
                                "name = Bob\n");

    const SectionlessINIReader ini(contents);
    auto age = ini.get<long>("age");
    if (age.has_value())
        std::cout << "Name: " << ini.get<std::string>("name").value_or("")
                  << "; age: " << age.value()
                  << std::endl;
}


Hybrid Lock Implementation I have an algorithm that is painfully slow if I use 'pure' mutexes. That's because most of the critical sections are short and far shorter than the work to sleep a thread. However, it is also slower to allow the occasional long critical section cause long spins in other thread(s). This class is intended to provide a balance. The balance here is compile-time fixed. What would be interesting is an adaptive strategy for tuning the spin time.

#include <atomic>
#include <condition_variable>
#include <mutex>
///A hybrid lock making a number of spin attempts
///and then entering a wait before retrying.
///<remarks>Implemented to be interface swappable
///with std::mutex.</remarks>
template<unsigned SPIN_LIMIT>
class hybrid_lock {
public:
    typedef unsigned spin_count_type;
    typedef void *native_handle_type;

    inline void lock() {
        spin_count_type limit(0);
        for (;;) {
            if (!this->mFlag.test_and_set(std::memory_order::memory_order_acquire)) {
                return;
            }
            if (limit >= SPIN_LIMIT) {
                break;
            }
            ++limit;
        }
        std::unique_lock<std::mutex> guard(this->mMutex);
        this->mCondition.wait(guard, [this]() {
            return !this>mFlag.test_and_set(
               std::memory_order::memory_order_acquire
            );
        });
    }

    inline bool try_lock() {
        return !this->mFlag.test_and_set(
            std::memory_order::memory_order_acquire
        );
    }

    inline void unlock() {
        this->mFlag.clear(std::memory_order::memory_order_release);
        this->mCondition.notify_one();
    }

    ///Returns the address of the flag backing the lock.
    ///<remarks>The return value may not be compatible with
    ///std::mutex.</remarks>
    ///<returns>The address of the flag
    ///backing the spin-lock.</returns>
    inline native_handle_type native_handle() {
        return static_cast<native_handle_type>(&this->mFlag);
    }

    inline hybrid_lock(void) {
        mFlag.clear();
    }
private:
    std::atomic_flag mFlag;//The actual lock object.
    std::mutex mMutex;//Used to synchronize sleeping threads.
    std::condition_variable mCondition;

    hybrid_lock(const hybrid_lock&) = delete;
    hybrid_lock& operator=(const hybrid_lock&) = delete;
};

I would expect the standard library's mutex (which is basically pthread_mutex) to already take care of this optimization behind the scenes, the same way these days I would expect the standard library's malloc to maintain its own thread-local arenas instead of taking a lock on every call.
Have you benchmarked the difference between what you wrote above and
template<unsigned SPIN_LIMIT>
class kiss_mutex {
    std::mutex mtx_;

  public:
    void lock() {
        for (unsigned i = 0; i < SPIN_LIMIT; ++i) {
            if (mtx_.try_lock()) return;
        }
        mtx_.lock();
    }

    bool try_lock() { return mtx_.try_lock(); }
    void unlock() { return mtx_.unlock(); }
};

? I haven't, but I'd be very curious to know whether kiss_mutex<N> performed comparably to std::mutex or comparably to hybrid_lock<N> (for tuneable values of N, of course).

Your benchmark might have been thrown off by a hilarious bug in hybrid_lock::lock():
    this->mCondition.wait(guard, [this]() {
        return !this>mFlag.test_and_set(
           std::memory_order::memory_order_acquire
        );
    });

When you see it...
This is the funniest argument I've seen yet for suffixing data members with _ and never writing out this-> except where the grammar requires it. :)

Stylistically, your code seems pretty good. My nits:

I would be extremely wary of declaring template non-type parameters of type unsigned int. My advice is to stick with int, because its name is short and because it's easy to write literals of that type. The only reason I stuck with unsigned in kiss_mutex is because if I didn't, it wouldn't have been a drop-in replacement for your code.

Here's the code showing why unsigned is worse than int: if someone sees hybrid_lock<100> they're probably going to assume that 100 represents an int (or at worst a size_t), and they're going to code accordingly, and their code simply will not work if you've used some other weird type. In other words, int is widely recognized as a vocabulary type (a "ubiquitous type used throughout the internal interfaces of a program"), whereas unsigned is not generally recognized as such. Of course, unsigned might be used as a vocabulary type in your codebase; it all depends on your employer's coding style guide. But I would hope it's not, for sanity's sake (again refer back to the code in the Wandbox above).
For another set of reasons that I mostly agree with, see Laurion Burchall's "unsigned considered harmful". Since you mentioned you've used Java, you might like this similarly titled blog post elsewhere, re Java, which makes some good points re "things C and C++ screwed up in their implementation of unsigned types."

The thing you wrote is actually a mutex in C++ terms, not a lock; so it should be called hybrid_mutex. A mutex exposes imperative lock and unlock methods; a lock (for example, std::unique_lock) is a wrapper that hides lock and unlock under a protective layer of RAII. (In other words: If you're designing a drop-in replacement for std::mutex, it should almost certainly be named something_something_mutex.)

In C++11, it is inappropriate to make any special member function private. If you want a special member function to go away, declare it =delete (which you did) but leave it public; this way, you're sure to get the error messages you want, instead of weird interference from the part of the compiler that checks access controls. "This constructor was explicitly deleted" is a much better error message than "This constructor is private and by the way it's been deleted too."

The Rule of Zero: std::mutex is a non-copyable type, so merely having a member of type std::mutex suffices to implicitly delete your copy operations. It's redundant to declare them deleted; you can just rip out those lines, as I did in kiss_mutex.

On the same note, I've never seen anyone use the inline keyword as you did, on member functions that are declared in-line and are thus implicitly inline anyway. Keep the code clean by removing redundant keywords. (Personally I make an exception for virtual, but my impression is that the C++ mainstream these days doesn't even make that exception. Indeed, as of 2021, I've completely converted to the Church of override, and no longer ever write virtual redundantly.)


Mockable clock meeting std::chrono TrivialClock requirement and interface I'm using the time for e.g. ping and timeout calculation as well as animations in a game. So my (big) codebase uses std::chrono::steady_clock::now() in many places. For testing I'd like to test those without having to resort to sleep and friends (timeout of >=5mins will be hard to test this way...)

For this I created a custom Clock class that uses a singleton-like instance which can be changed to provide a custom clock. A default of steady_clock is used for production code. This Clock can be used as a drop-in replacement of  std::chrono::steady_clock and can be mocked in test code.

I'd like a review with suggestions how to improve this or if it has any flaws. Pointers to similar/better implementations are also welcome. I'm mostly concerned about the multiple indirections (and related performance penalties) this causes: Lookup of a "constructed" flag, the impl and the vtable (3 pointers) although I only have 1 function. See godbold

#include <chrono>
#include <memory>

struct BaseClock
{
    using Clock = std::chrono::steady_clock;
    using time_point = Clock::time_point;
    virtual ~BaseClock() = default;
    virtual time_point now(){ return std::chrono::steady_clock::now(); }
};

class Clock
{
    static std::unique_ptr<BaseClock>& inst(){
        static std::unique_ptr<BaseClock> clock(new BaseClock);
        return clock;
    }
public:
    using rep = BaseClock::Clock::rep;
    using duration = BaseClock::Clock::duration;
    using time_point = std::chrono::time_point<Clock>;

    static time_point now(){ return time_point(inst()->now().time_since_epoch()); }
    static void set(std::unique_ptr<BaseClock> clock){inst() = std::move(clock);}
};

int main()
{
    return Clock::now().time_since_epoch().count();
}

Example usage in test code:

struct MockClock: public BaseClock{
    static time_point current;
    time_point now() override { return current; }
};

void test_method(){
    Clock::set(new MockClock);
    MockClock::current = MockClock::time_point(100);
    testClass.methodUsingClock();
    MockClock::current += std::chrono::seconds(10);
    REQUIRE(testClass.checkTimeout());
}

While I think the underlying idea is quite nice, the actual implementation and design has some issues.
Wrong Abstraction Level
Let's start with the less obvious one: How would you actually use Clock or BaseClock with std::chrono::high_resolution_clock or std::chrono::system_clock?
The simplest approach would be something akin to this:
struct HighResClock : BaseClock {
    time_point now() override { return std::chrono::high_resolution_clock::now(); }
};

It seems so clean, so simple, and so easy. Except that it doesn't compile! That's because std::chrono::high_resolution_clock::time_point is not the same as BaseClock::time_point (and cannot easily be converted, if at all possible).
But: Do we actually need time_points in the public interface?
The only reason to expose time_point values is to extract time differences between them. But that only matters if arbitrary time_points are to be compared.

Technically, the time_point could be stored in some form. However, for many clocks, like std::chrono::steady_clock or std::chrono::high_resolution_clock, the epoch from when the clock are measuring their time offset can change between different executions of the same program (e.g. because the computer got rebooted).
This makes storing time_points, especially those not obtained from std::chrono::system_clock, rather useless. In that case, you'll likely need a calendar library (or similar) to get points of time in a storable format.

But in most cases, a simple Timer abstraction can fulfill all clock needs (comparing some time_points with some relation). A simple Timer interface could look like this:
struct timer {
    // choose an appropriate duration type
    using duration = std::chrono::duration<double>;

    virtual ~timer() = default;

    virtual void start() = 0;
    virtual duration stop() = 0;
    virtual duration tick() = 0; // to obtain multiple measurements from the same baseline
};

class steady_timer : public timer {
    using clock = std::chrono::steady_clock;
    using time_point = clock::time_point;

    time_point start_time;
    bool running;

public:
    steady_timer() = default;

    void start() override
    {
        start_time = clock::now();
        running = true;
    }

    duration tick() override
    {
        return duration(clock::now() - start_time);
    }

    duration stop() override
    {
        auto elapsed = tick();
        running = false;
        return elapsed;
    }
};

Now the only exposed part of the interface is the duration. And it is easily extensible to other time sources (e.g. Win32 QueryPerformanceCounter) or mockable.
Singleton
I really don't like the Clock singleton. Yes, it is easy to just ask a global clock. Yes, it is also easy to screw all code depending on this clock by changing the underlying instance.
For example, a test setting Clock to a mock but not restoring the original clock breaks all other tests that assume the default clock implementation - making test failure dependent on test execution order.
Instead, take a reference or pointer to a timer as parameter. This allows you to pass in a clock where needed, without changing (or corrupting) everyone elses timer.
Rewriting your test case:
class mock_timer : public timer {
    std::vector<duration> measurements;
    int counter = 0;

public:
    mock_timer(std::initializer_list<duration> il) : measurements(il) {}

    void start() override {}

    duration tick() override
    {
        if(counter < measurements.size()) return measurements[counter++];
        return measurements.back();
    }

    duration stop() override
    {
        return measurements.back(); // just example
    }
};

void test_method(){
    using namespace std::literals::chrono_literals;
    mock_timer my_timer{ 10s };

    testClass.methodUsingClock(my_timer);

    // or, more likely:
    testclass inst{my_timer};
    inst.methodUsingClock();

    REQUIRE(testClass.checkTimeout());
}


Tcp server messenger awaiter in f# Hi I have a simple tcp server, and that server receives messages with specific length so basically i need to wait for TcpClient to fill buffer with that amount of data, and it works.

But I am not sure if this should look like it looks as I am writing this in f# and still learning:

let private WaitForData (client : TcpClient, dataLength : int) : Async<bool>=

    async {

        let mutable loopBreaker : bool = true
        let mutable result : bool = false

        while loopBreaker do
            let isConnectionAlive : bool = IsConnectionEstablished client && client <> null

            if isConnectionAlive && client.Available >= dataLength then
                result <- true
                loopBreaker <- false

            if isConnectionAlive = false then
                result <- false
                loopBreaker <- false

            Task.Delay(500)
            |> Async.AwaitTask
            |> ignore

        return result
    }

So as you see I use mutables to exit while loop and set result value, and have some if there.

Is this OK or maybe there is some more elegant way to write this

The first thing is when you use parenthesis it means that you pass a tuple to a function instead of single parameters. So you can ditch them.

The second thing is that bool supports pattern matching

The third thing is that you can use recursion instead of mutable variables. Note the usage of rec which enables tail call optimisation.

The code would look something like this.

let rec private WaitForData2 (client : TcpClient) (dataLength : int) (loopBreaker : bool) (result: bool) : Async<bool>=
    async {
        match loopBreaker with
        | true ->
            let isConnectionAlive : bool = IsConnectionEstablished client && client <> null
            match isConnectionAlive, client.Available >= dataLength with
            | true, true ->
                Task.Delay(500)
                |> Async.AwaitTask
                |> ignore
                return! WaitForData2 client dataLength false true
            | false, _ ->
                Task.Delay(500)
                |> Async.AwaitTask
                |> ignore
                return! WaitForData2 client dataLength false false
            | _, _ ->
                Task.Delay(500)
                |> Async.AwaitTask
                |> ignore
                return! WaitForData2 client dataLength true false
        | false -> return result


Check_sudoku in Python I have just started to learn programming with Python. I have been going through several online classes for the last month or two. Please bear with me if my questions are noobish.

One of the classes I am taking asked us to solve this problem.

# Define a procedure, check_sudoku,
# that takes as input a square list
# of lists representing an n x n
# sudoku puzzle solution and returns the boolean
# True if the input is a valid
# sudoku square and returns the boolean False
# otherwise.

# A valid sudoku square satisfies these
# two properties:

#   1. Each column of the square contains
#       each of the whole numbers from 1 to n exactly once.

#   2. Each row of the square contains each
#       of the whole numbers from 1 to n exactly once.

# You may assume the the input is square and contains at
# least one row and column.

After a while of trying to come up with the best code I can, this is what I ended up with.

def check_sudoku(sudlist):
    x = range(1, len(sudlist)+1) # makes a list of each number to be found
    rows = [[row[i] for row in sudlist] for i in range(len(sudlist))] # assigns all the rows to a flat list
    z = range(len(sudlist))
    for num in x:
        for pos in z:
            if num not in sudlist[pos] or num not in rows[pos]:
                return False
    return True

This code does work and passed all checks the class asked for.  I just wonder what problems could the code have or how would you improve it?

I am just trying to improve by getting input from others.  I appreciate any suggestions.

I would use set to do the checks, and you can build the columns using some zip magic:

def check_sudoku(sudlist) :
    numbers = set(range(1, len(sudlist) + 1))
    if (any(set(row) != numbers for row in sudlist) or
        any(set(col) != numbers for col in zip(*sudlist))) :
        return False
    return True

I have tested it with the following samples:

a = [[1, 2, 3, 4, 5],
     [2, 3, 5, 1, 4],
     [3, 5, 4, 2, 1],
     [4, 1, 2, 5, 3],
     [5, 4, 1, 3, 2]] # latin square

b = [[1, 2, 3, 4, 5],
     [2, 3, 5, 1, 4],
     [3, 5, 4, 2, 1],
     [4, 1, 2, 5, 3],
     [3, 4, 1, 5, 2]] # 1st and 4th elements of last row interchanged

c = [[1, 2, 3, 4, 5],
     [2, 4, 5, 1, 4],
     [3, 5, 4, 2, 1],
     [4, 1, 2, 5, 3],
     [5, 3, 1, 3, 2]] #1st and 5th elements of second column intechanged

>>> check_sudoku(a)
True
>>> check_sudoku(b)
False
>>> check_sudoku(c)
False


The most elegant way to locate two enums I have situation where user has got options to select, and after selected any option will appear next options. For example :
1 - Add car
2 - Add motorcycle.
3 - Print cars
4 - Print motorcycles

And after selected for example "1" console will show options like :
1 - Add passenger car,
2 - Add light commercial car.
At this moment i have two enum classes in two separated files that has got almost the same code inside, and same methods createFromInt() which returns Options according to int provided by the user, and toString().Exmple one of two enums(The second one is almost the same except the fields) :

enum Option {
ADD_CAR(0,"Add car"),
ADD_MOTORCYCLE(1,"Add motorcycle"),
PRINT_CARS(2,"Print cars"),
PRINT_MOTORCYCLES(3, "Print motorcyces"),

private int value;
private String description;

Option(int value, String description) {
    this.value = value;
    this.description = description;
}

@Override
public String toString() {
    return value + "-" + description;
}

public static Option createFromInt(int option) throws NoSuchOptionException {
    try {
        return Option.values()[option];
    }catch (ArrayIndexOutOfBoundsException ex) {
        throw new NoSuchOptionException("Brak opcji o ID: " + option);
    }
}

}

I was just wondering is it corectly locate these enums in two files or exists any more elegant way for example one file with two enums , or nested enums (if something like that at all exists).

At first, I think you don't need the value field. Indeed, as shown in createFromInt, you refer to enum values with their position (Option.values()[option]).

I suggest the following refactoring.

enum Option {
  ADD_CAR("Add car"),
  ADD_MOTORCYCLE("Add motorcycle"),
  PRINT_CARS("Print cars"),
  PRINT_MOTORCYCLES("Print motorcycles");

  private String description;

  Option(String description) {
    this.description = description;
  }

  @Override
  public String toString() {
    return this.ordinal() + "-" + description;
  }

  public static Option getFromIndex(int optionIndex) throws NoSuchOptionException {
    if (optionIndex >= 0 && optionIndex < values().length) {
      return Option.values()[optionIndex];
    }
    throw new NoSuchOptionException("Brak opcji o ID: " + optionIndex);
  }
}

I prefer getFromIndex because the argument is an index or a value and you don't create new instances of the enum.


Paginating a Backbone collection This is part of a class for paginating a Backbone collection.

The paginateTo method is for paginating to a model id. It returns the model if it's already in the collection, otherwise it checks with the server that it exists. If so, it requests subsequent pages until it's loaded into the collection.

How would you refactor this method to make it clearer?

paginateTo: (id, dfd, check) ->
  dfd ?= new $.Deferred
  return dfd.resolve(model) if model = @get(id)
  check ?= @checkModelExists(id)
  check.done =>
    @once 'sync', => @paginateTo(id, dfd, check)
    @fetchNextPage()
  check.fail =>
    dfd.reject()
  dfd.promise()

checkModelExists: (id) ->
  url = @url() + '/' + id
  $.get url

fetchNextPage: ->
  @page += 1
  @fetch { update: true, remove: false, data: { page: @page }}

You could do something like this

paginateTo: (id) ->
  deferred   = new $.Deferred
  modelCheck = null

  modelExists = =>
    modelCheck or= @checkModelExists id

  fetchUntilFound = =>
    return deferred.resolve model if model = @get id
    modelExists().fail deferred.reject
    modelExists().done =>
      @fetchNextPage().then fetchUntilFound, deferred.reject

  fetchUntilFound()
  deferred.promise()


Setting Class Properties I am adding classes to some legacy code and wondering if there is a more appropriate way to be setting the properties. I am also trying to understand the trade offs, if there is another way to do this.
I'm trying to find out if there is a better way to do this? If there is, why? If there isn't, why is this better than the alternative?

class Foo
{
    public $id;

    public function __construct($id)
    {
        $this->id   = $id;
        $this->getDetails();
    }

    /**
     *
     * Assigns all properties
     *
     */
    public function getDetails()
    {
        $db = Database::getDatabase();

        $sql = "SELECT * FROM foos WHERE id = '{$this->id}' ";
        $result = $db->getRow($sql);

        foreach($result as $key => $value) {
            $this->$key = $value;
        }
    }

}

Security

Use prepared statements. Classes are reusable, so even if the id is not user provided right now, this may very well change in the future, making your code vulnerable to SQL injection.

Naming

getDetails is rather vague. What details? Additionally, getX usually signals that the method returns X, which isn't the case here.

Your comment is a bit better, something like assignProperties might work. I would prefer something like loadById, populateById, or more explicit populateFieldsById.

General Structur

getDetails should be private, right?

As for the general structure, it's fine.

I would probably separate the Foo object from the code retrieving it from the database like this (pseudo code):

// just setters, getters, and business logic
class Foo {

    constructor(...) {...}
    getPropertyX() {...}
    setPropertyX(...) {...}
    getPropertyY() {...}
    setPropertyY(...) {...}
    performSomeBusinesslogicOnObject() {...}

}

// performing database interactions and populating Foo objects
class FooDAO {

    Foo getById(id) {...}
    Foo getByPropertyZ(z) {...}
    save(Foo foo) {...}

}

My main reason for doing it this way is that it separates the database operation from the model.

Now, you can use the model without being forced to have a database interaction (maybe one isn't needed right now, maybe you want to run tests, maybe the current instance of the object comes from somewhere else than the database, etc).

On the other hand, if most of your business logic happens directly on the database, not the object, this might get confusing. So it really also depends on what Foo is exactly. But for simple objects which are mainly retrieved, updated, saved, etc, this is the way I would go.


Dynamic unpacking in Python Python lacks dynamic unpacking. Example: You want to unpack a list, let's say coordinates, but don't know whether it contains 3 items or just 2.

x, y, z = [1,2,3] works only if len([x,y,z]) == len([1,2,3]).

x, y, z = [1,2] results in an error. You could add try and except blocks but that could be complicated.

The best option is z being None, as you can simply check using if n is None without any excessive try/except.

So expected result:

>>> x, y, z = unpack([1,2])
>>> print(x)
1
>>> print(y)
2
>>> print(z)
None

My code

def unpack(num, list):
    return_arr = [None] * num
    for i,elem in enumerate(list):
        return_arr[i] = elem
        if i+1 == num:
            return return_arr
    return return_arr

And usage examples:

a,b,c,d = unpack(4, [1,2,3])
print(a),
print(b),
print(c),
print(d),
print("\n")

e,f,g = unpack(3, [1,2,3])
print(e),
print(f),
print(g)

resulting in

1 2 3 None

1 2 3

You basically have to specify the amount of variables you're unpacking the list to, since the function can't know that.

It is generaly a bad idea to shadow a builtin (like list) by using a variable named after it.
You can use slices and array extension to simplify a bit your algorithm:

def unpack(n, lst):
    result = lst[:n]
    return result + [None] * (n - len(result))

You can use the itertools module to improve memory management and allow for any iterable:

import itertools

def unpack(n, iterable):
    infinite = itertools.chain(iterable, itertools.repeat(None))
    return itertools.islice(infinite, n)

Python 3 has an extended unpacking capability that is closer to your needs:

>>> x, y, *z = [1, 2]
>>> print(x, y, z)
1, 2, []


Finding next perfect number - brute force A “perfect number” is defined as a number equal to the sum of all its factors less than itself. For example, the first perfect number is 6, because its factors are 1, 2, 3, and 6, and 1+2+3=6. The second perfect number is 28, because 1+2+4+7+14=28. What is the third perfect number? Write a procedure (next-perf n) that tests numbers starting with n and continuing with n+1, n+2, etc. until a perfect number is found. Then you can evaluate (next-perf 29) to solve the problem. Hint: you’ll need a sum-of-factors subprocedure.
[Note: If you run this program when the system is heavily loaded, it may take half an hour to compute the answer! Try tracing helper procedures to make sure your program is on track, or start by computing (next-perf 1) and see if you get 6.]

Source
I am aware of Euclid's algorithm for solving this problem, but with the "n+1,n+2" part of the problem statement I think I shouldn't do that. So I did it in a brute force way.
(define (nextPerf n)
    (define (sumOfFactors a)
        (cond ((= n a) 0)
            ((= (remainder n a) 0) (+ a (sumOfFactors (+ a 1))))
        (else (sumOfFactors (+ a 1)))))
    (if (= (sumOfFactors 1) n) n
        (nextPerf (+ n 1))))

Is this good code? How can I make it better?

You get:

  Hint: you’ll need a sum-of-factors subprocedure.

So I would make sum-of-factors a separate procedure. It's a little confusing as is, since sumOfFactors takes a single argument - and that argument isn't the number whose factors you're summing. That is:

(define (sum-of-factors n)
    ...)

(define (next-perf n)
    (if (= (sum-of-factors n) n)
        n
        (next-perf (+ n 1)))

Note that hyphenated-name is the preferred syntax for function names, as opposed to camelCase.

Once we're there, you can write sum-of-factors to do the looping:

(define (sum-of-factors n)
    (define (add-factor fac) (if (= (remainder n fac) 0) fac 0))
    (define (accum fac) (if (= fac n) 0 (+ (add-factor fac) (accum (+ 1 fac)))
    (accum 1))

I split it up into a few more bite-size functions. I find that helps readability.

In Racket (which I guess isn't up for discussion), you can do the latter way better since there's an in-range procedure that gives you want you need:

(define (sum-of-factors n)
    (for/sum ([fac (in-range 1 (- n 1))])
        (if (= (remainder n fac) 0) fac 0))))

Don't think there's a Scheme equivalent unfortunately, so maybe this is more of a tease than an answer.


Convert an array to a binary tree I wrote this code in C++ to convert an array to a binary tree.

For instance, int nums[] = {7,5,9,1,7,3} to a binary tree (root-7, root's left child-5, root's right child-9...).

( here, I made a function named 'create_tree()' which only take one argument )

Is there any shorter way to do this?

#include <iostream>
#include <cstdlib>
#include <cstdio>

template<class E>
struct TNode{
    E value;
    TNode<E>* leftChild;
    TNode<E>* rightChild;

    TNode<E>(){
        this->value = NULL;
        this->leftChild = NULL;
        this->rightChild = NULL;
    }

    TNode<E>(E value){
        this->value = value;
        this->leftChild = NULL;
        this->rightChild = NULL;
    }
};

template<class E>
class BTree{
private:
    TNode<E>* root;

    /**
        build and return the pointer to a binary tree for given array 'arr' and array         size 'len'
    */
    TNode<E>* build(E* arr, int len){

        TNode<E> *subRoot = new TNode<E>(arr[0]);
        if (len == 1){
            return subRoot;  // terminating function - at the leaf
        }
        E* elementsToLeft = new E[len];  // array to store the left subtree children
        E* elementsToRight = new E[len];  // array to store the right sub tree     children

        int counterLeft = 0;
        int counterRight = 0;

        for (int i = 1; i < len; i = (i * 2) + 1){
            for (int j = i; j < i + ((i + 1) / 2); j++){
                elementsToLeft[counterLeft] = arr[j]; // appending the array which     has children of left sub tree
                counterLeft++;
                if ((j + (i + 1) / 2)<len){  // check whether there are childs to     store in the right sub tree....
                    elementsToRight[counterRight] = arr[j + (i + 1) / 2]; //   appending the array which has children of left sub tree
                    counterRight++;
                }

            }
        }
        subRoot->leftChild = build(elementsToLeft, counterLeft);  // recursive call>     array- left sub tree's children and size
        subRoot->rightChild = build(elementsToRight, counterRight); // recursive     call> array- right sub tree's children and size
        return subRoot;
    }

public:

    /**
    add the value specified by newvalueas the left child of the tree node specified     by parent,if the node does not have a left childalready.
    returns 1 if addition is successful and returns -1 in failure.
    */
    template <class E>
    int intaddLeftChild(TNode<E>* parent, E newvalue){
        if (parent->leftChild != NULL){ return -1; }
        TNode<E> *temp = new TNode<E>(newvalue);
        parent->leftChild = temp;
        return 1;
    }

    /**add the value specified by newvalueas the right child of the tree node     specified by parent, if the node does not have a right childalready.
    returns 1 if addition is successful and returns -1 in failure.
    */
    template <class E>
    int intaddRightChild(TNode<E>* parent, E newvalue){
        if (parent->rightChild != NULL){ return -1; }
        TNode<E> *temp = new TNode<E>(newvalue);
        parent->rightChild = temp;
        return 1;
    }

    /**
    function to create and return the pointer to a Binary tree from an array
    input an array. eg:- int i[] = {3,5,7} ; create_tree(i);
    */
    template<class E, int N>
    TNode<E>* create_tree(E(&elements)[N]){
        int length = N;
        return build(elements, N);
    }
};

int main(){
    //to demonstrate
    int nums[] = { 7,5,9,1,7,3};
    BTree<int> mal;

    TNode<int>* BinaryTree = mal.create_tree(nums); // store the made binary tree     from nums and point it by the pointer, BinaryTree.

}

Main points to improve:

Memory leaks: C++ is not garbage collected. Every new must be deleted at some point. This is one of the reasons why C++ has destructors, so that we can perform this and other kinds of cleanup. In modern C++, smart pointers and standard containers are preferable, over manual memory management, since forgetting to delete a pointer is a very easy to make mistake, thus manual memory management being a major source of bugs and memory leaks.
Unusual data initialization style in constructors:

TNode<E>(){
    this->value = NULL;
    this->leftChild = NULL;
    this->rightChild = NULL;
}

The following is the correct way to initialize members. You should use the constructor initialization list. Also, prefixing the variables with a this-> in something we rarely do in C++:

TNode()
    : value(NULL)
    , leftChild(NULL)
    , rightChild(NULL)
{ }

Other tidbits:

Inside the body of a template class, such as in TNode, you should omit the template parameter in the name. Writing TNode<E> everywhere is too verbose and unnecessary.
Template parameter name for types is by convention called T instead of E.
NULL is deprecated since C++11 in favor of the better nullptr.
Integer return codes are quite error prone. Returning a bool true or false, when the function has only two possible outcomes, is a lot nicer.
You didn't follow your camelCase notation in the create_tree() method.


Process files in all subdirectories and save output to new files based on their current path Sample files:

./data/foo/file1.xml
./data/foo/bar/file2.xml
./data/baz/file3.xml

Corresponding output:

./out/foo/file1.xml.xml2
./out/foo/bar/file2.xml.xml2
./out/baz/file3.xml.xml2

The subproblems are:

Find all xml files in data/
Construct the output path: replace data/ with out/ and add an additional extension
Create the output path if it doesn't exist
Execute a script, redirect to output file in new path

I've been experimenting with find and I have come up with a solution that works on my test files:

find data/ -name "*.xml" -exec bash -c \
    'out=${1/data/out}.xml2; mkdir -p $(dirname "$out"); ./testscript.sh > $out' - {} \;

(testscript.sh contains echo "Hello World".)

But I'm not that good with bash, and this seems overly verbose, and might contain hidden errors.

Things I've thought about:

mkdir silently fails if the dir exists, so it'll work for multiple
files in the same dir, but maybe this entire script should be done in a loop in order to only call mkdir once per new dir.
${1/data/out} only replaces the first occurence of data, which should always exist and be the outermost directory in the variable (since relative paths are used). I'd prefer it if this was done in a more automatic and safe way though, if possible.

Side-note: I have full permissions in the current dir and all subdirs, but not on the entire machine.

(...) this seems overly verbose, and might contain hidden errors.

This is pretty good as it is.
I cannot think of a substantially shorter and more readable way.
I don't think there are hidden errors either.

  mkdir silently fails if the dir exists, so it'll work for multiple files in the same dir, but maybe this entire script should be done in a loop in order to only call mkdir once per new dir.

mkdir doesn't fail. The -p flag makes it exit with success if the directory already exists, so technically, it succeeds.
It's true that strictly speaking this is not the most optimal way.
It would be ideal to make the script in a way that it doesn't repeatedly call  mkdir -p for the same directory.
But it's just not worth it.
It will make the script more complicated,
for benefit so negligible that probably you cannot even measure it.
Very often, the simplest solution is the best, even it's not 100% optimal in terms of "performance".
If you consider readability and complexity,
then it might actually be optimal overall.

  ${1/data/out} only replaces the first occurence of data, which should always exist and be the outermost directory in the variable (since relative paths are used). I'd prefer it if this was done in a more automatic and safe way though, if possible.

If you always want to replace the first path element with out,
whatever it is, not only data,
then you can write this way:

find data/ -name "*.xml" -exec bash -c \
    'out=out/${1#*/}.xml2; mkdir -p $(dirname "$out"); ./testscript.sh > $out' - {} \;

The output will be the same as in your example,
and will still be the same for find other/ or find blah/.

It might be useful to add the -v flag for the mkdir.
This is for verbose mode,
it will print the directories it creates (if your system supports the -v flag).

Finally, it's probably good to chain the mkdir and the script following it with &&. So that in case you don't have permission to create directories the script doesn't need to run and double the errors printed on the script.

find data/ -name "*.xml" -exec bash -c \
    'out=out/${1#*/}.xml2; mkdir -vp $(dirname "$out") && ./testscript.sh > "$out"' - {} \;

Finally++, as you properly quoted $out in $(dirname "$out"),
you should do the same when redirecting to it: > "$out"


Naive lock free work stealing queue Recently, I read a article about work stealing queue  Job System 2.0: Lock-Free Work Stealing – Part 3: Going lock-free, and this is my c++11 naive implementation based on my understand of c++11 memory order model. This works on x86 , as x86 has a strong memory model, is there any issue running on other architecture (weak memory order)?

#include <atomic>

class Job;

struct WorkStealingQueue
{
    // only called by owner work thread
    void Push(Job* job) noexcept
    {
        // m_bottom -> stealing thread read, owner thread read, write.
        auto bottom = m_bottom.load(std::memory_order_relaxed);

        m_jobs[bottom & MASK] = job;

        // need  let stealing thread see the new job.
        m_bottom.fetch_add(1, std::memory_order_release);
    }

    // only called by owner worker thread
    Job* Pop(void) noexcept
    {
        auto bottom = m_bottom.load(std::memory_order_relaxed);

        auto top = m_top.load(std::memory_order_acquire);

        if (bottom > top)
        {
            auto job = m_jobs[(bottom - 1) & MASK];

            if(top < (bottom - 1))
            {
                m_bottom.fetch_sub(1, std::memory_order_release);
                return job;
            }
            if (m_top.compare_exchange_weak(
                top, top + 1,
                std::memory_order_release,
                std::memory_order_relaxed))
            {
                return job;
            }
        }

        return nullptr;
    }

    // called by stealing thread, not owner thread
    Job* Steal(void) noexcept
    {
        auto top = m_top.load(std::memory_order_acquire);

        // Release-Acquire ordering, so the stealing thread see new job
        auto bottom = m_bottom.load(std::memory_order_acquire);

        if (bottom > top)
        {
            auto job = m_jobs[top & MASK];
            // check if other stealing thread stealing this work
            // or owner thread pop this job.
            // no data should do sync, so use relaxed oreder
            if (m_top.compare_exchange_weak(
                top, top + 1,
                std::memory_order_release,
                std::memory_order_relaxed))
            {
                return job;
            }
        }

        return nullptr;
    }

private:

    static constexpr auto MAX_COUNT = 4096u;
    static constexpr auto MASK = MAX_COUNT - 1u;
    static_assert((MAX_COUNT & MASK) == 0, "the max number of job must be power of two.");

    Job* m_jobs[MAX_COUNT];
    std::atomic<unsigned int> m_bottom{ 0 }, m_top{ 0 };
};

Pop() has problem
Consider what happens if your queue is in the following state with two jobs:
m_top    = 0
m_bottom = 2

The owner thread calls pop(), and loads in to local variables:
top    = 0
bottom = 2

Now, before the owner thread does anything else, two stealer threads call steal(), leaving the state of the queue like this:
m_top    = 2
m_bottom = 2

When the owner thread continues inside of pop(), it will go into this case and return a job that no longer exists:

    if(top < (bottom - 1))
    {
        m_bottom.fetch_sub(1, std::memory_order_release);
        return job;
    }

Your code deviated from the code you linked.  In the linked code, the first thing that pop() did was to decrement m_bottom so that the stealer threads could not steal the bottom job.


Handling login using promises We've got this node.js code which is called by an Express route:

routes.login = (req, res) => {

  let inpEmail = req.body.email.toLowerCase(),
    inpPassword = req.body.password,
    loginFailureCount = 0;

  // find the user
  User.findOne({
      email: inpEmail
    }).exec()
    .then((user) => {

      //get existing login failure count
      loginFailureCount = user.loginFailureCount;

      if (user.isLocked && Date.now < user.lockExpiry) {
        //some logic here
      } else if (!user.emailVerified) {
        //some logic here
      } else {

        bcrypt.compare(inpPassword, user.passwordHash).then((compareResult) => {

          // check if password matches
          if (compareResult) {

            //some logic here
            setLoginFailureCount(user, loginFailureCount).then(() => {

                setLastLogin(user)
                  .then(() => {
                    //some logic here
                  })
                  .catch((err) => {
                    //some logic here
                  });
              })
              .catch((err) => {
                //some logic here
              });

          } else {

            //login failed
            loginFailureCount = loginFailureCount + 1;
            setLoginFailureCount(user, loginFailureCount)
              .then(() => {
                //some logic here
              })
              .catch((err) => {
                //some logic here
              });
          }
        });
      }
    })
    .catch((err) => {
      //some logic here
    });
}

All looks good, and we feel that the promises are not used correctly; there are multiple catch statements.

One of my colleague said that:

  Ideally, the code structure with promises shall look like this:

someFunction()
.then()
.then()
.then()
.catch();

Since, there are multiple if-else statements. We were trying to brainstorm if the above kind of chaining can be implemented or not.

Is there a better way to implement promises in this code?

This is missing the point. It is not about promises. You are right that if done correctly, you'll get a nice flat structure, but not if you inline all your code. This would be true if you weren't using promises as well. What you should do is split your code into smaller building blocks, and the nice structure will follow.

Also, be wary of your variable names:

// check if password matches
if (compareResult) {

Why not rename "compareResult" "passwordIsCorrect" and get rid of the comment?

// find the user
User.findOne({

That comment is useless.

Basically all the comments can be removed. Try to explain yourself with code instead, and only comment what can't be explained with code.

Just a quick example of the type of refactoring I would do:

routes.login = (req, res) => {
    let inpEmail = req.body.email.toLowerCase();
    let inpPassword = req.body.password;

    User.findOne({email: inpEmail})
        .exec()
        .then((user) => {
            if(user.isLocked && Date.now < user.lockExpiry){
                return blabla();
            }else if(!user.emailVerified){
                return requireEmailVerification();
            }else{
                return login(user, inpPassword);
            }
        })
        .then(() => {
            // Probably res.send() here
        })
        .catch((err) => {
            //some logic here
        });
};

function login(user, pw){
    return bcrypt.compare(pw, user.passwordHash)
        .then(passwordIsCorrect => {
            if(passwordIsCorrect){
                return setLastLoginAndDoSomeLogic(user);
            }

            return incrementLoginFailureCountAndDoSomeLogic(user);
        });
}

function setLastLoginAndDoSomeLogic(user){
    return setLastLogin(user)
        .then(() => {
            //some logic here
        })
        .catch((err) => {
            //some logic here
        });
}

function incrementLoginFailureCountAndDoSomeLogic(user){
    return setLoginFailureCount(user, user.loginFailureCount + 1)
        .then(() => {
            //some logic here
        })
        .catch((err) => {
            //some logic here
        });
}


Can I improve this code for readability and/or performance? I have some data that logs kWh. I want to be able to collect the data and produce a bar chart (I'm using the Microsoft ASP.Net Chart control). I have written code that works, but it looks a little clunky to me and I wondered if somebody could point out a method that may be easier to read and/or more efficient.

The data should normally constantly increase in value though it can be reset. As it is an accumulative value if the values from one period are the same as the previous period then I need to display zero for that time period. Then, when the value does increase, I need to display it as the difference between this period and the last period with a value.

I have the following code to collect the data at hourly intervals only:

      myValues = (from values in myEntities.PointValues
                      where
                        values.PointID == dataValue &&
                        SqlFunctions.DatePart("n", values.DataTime) == 0
                      orderby values.DataTime
                      select new BarChart
                        {
                          Time = values.DataTime,
                          Value = values.DataValue
                        }).ToList();

I then use the following code to set the values that are identical in consecutive periods to 0, though, of course, maintaining the first occurrence at its original value:

for (var i = 0; i < myValues.Count - 1; i++)
      {
        var j = 0;
        while (Math.Abs(myValues[i].Value - myValues[i + 1].Value) < double.Epsilon)
        {
          i++;
          j++;
          if (i == myValues.Count - 1) break;
        }
        while (j > 0)
        {
          myValues[i + 1 - j].Value = 0;
          j--;
        }
      }

Finally, I apply the following code to get the difference in values (difference between value this period and the last period with a positive value).

var subValue = double.MaxValue;
foreach (var t in myValues)
{
  if (t.Value > 0 && t.Value < subValue) subValue = t.Value;
  if (Math.Abs(t.Value - subValue) > double.Epsilon && Math.Abs(t.Value - 0) > double.Epsilon)
  {
    var j = t.Value;
    t.Value = t.Value - subValue;
    subValue = j;
  }
}

Which produces my final data.

As I say, this code works fine but if anyone can help with some optimizing/readability, it would be greatly appreciated.

EDIT:
For some images see:

https://stackoverflow.com/questions/14425230/can-anyone-assist-with-some-code-optimization

I think that your code to “remove the values that are identical” is quite confusing.

First, it doesn't remove anything, it just sets the values to zero. And unless you filter that out later, it will cause problems in your final calculation.

Second, modifying the indexing variable of a for is usually not a good idea. It's unexpected and creates code that is hard to understand.

Third, I think you don't actually need this. It would be much easier to remove zero values after you compute the differences.

Now, in your code to get the differences, I find the logic hard to follow. (One reason is probably non-descriptive variable names.) And I don't understand why do you have all those conditions there, computing a difference (even with the possibility of a reset) should be simpler. Also, you seem to ignore the fact that if you compute the difference between n values, you will get only n - 1 results.

I would rewrite this code using Zip() and Skip() like this:

var result = Enumerable.Zip(
    data, data.Skip(1),
    (first, second) =>
    second.Value < first.Value
        ? second
        : new BarChart { second.Time, Value = second.Value - first.Value });

Also, naming a single value in a chart BarChart is confusing, a better name would be something like BarChartValue.

You also asked for performance optimizations. Is your code actually causing performance problems? Have you profiled your application and has this code been identified as bottleneck? If not, don't worry (much) about performance, it won't make a difference anyway.


Pascal's Triangle Java (performance) I need to get some information about the Pascal Triangle.

What is a pascal triangle ?

  It is a triangle of integers with 1 at the top and at the sides. Any number inside is equal to the sum of the two numbers above his. For
  example, here are the first 5 lines of the triangle:

So I need to figure out the number of numbers
pairs that exist in the first thousand lines of the triangle.

I would like to know how I can improve the performance of my code or rewrite some part that is poorly structured

import java.sql.Date;
import java.text.DecimalFormat;
import java.text.SimpleDateFormat;
import java.util.Vector;

public class PascalTriangle {

    private static int [][] matrix;
    private static int i, j, n;

    public static void main(String[] args)
    {

        long start_time = System.currentTimeMillis();

        n = 0; // Number of even numbers;

        matrix = new int[1000][1000];

        for (i = 0; i < 1000; i++) {
            for (j = 0; j < 1000; j++) {
                matrix[i][j] = 0;
            }
        }

        matrix[0][0] = 1;
        matrix[1][0] = 1;
        matrix[1][1] = 1;

        for (i = 2; i < 1000; i++) {
            matrix[i][0] = 1;
            matrix[i][i] = 1;
            for (j = 1; j < i; j++) {
                matrix[i][j] = matrix[i-1][j-1]+matrix[i-1][j];
                if ( matrix[i][j] % 2 == 0 ) {
                    n++;
                }
            }
        }

        System.out.println( "Amount of even numbers = " + n );

        long end_time  = System.currentTimeMillis();
        System.out.println(new SimpleDateFormat("ss.SSS").format(new Date(end_time - start_time)));

    }
}

Expected result:

Amount of even numbers = 448363

Currently taking a time of 00.011 seconds

One array solution
The following algorithm is an improvement on the two array system stated bellow it.  Basically, we only need the context of the old previous value (as well as the next value, which goes unmodified), so we copy this value.  From there we can set the new end to 1 then start over.  This cuts down on the memory overhead.  I think you'll see minimal computational improvements as yours was already pretty efficient, but this one takes less memory.  I suspect there will be a performance increase where you are dealing with pretty large arrays.
import java.util.Arrays;

class Pascal{

     public static void main(String []args) {
        int toNthLine = 1000;
        int[] row = new int[toNthLine+1];
        int counter = 0;
        int previous;
        row[0] = 1;
        for (int i = 0; i < toNthLine; i++) {
            previous = 1;
            for (int j = 0; row[j+1] != 0; j++) {
                int temp = row[j+1];
                row[j+1] = previous + row[j+1];
                previous = temp;
                if (row[j+1] % 2 == 0)counter++;
            }
            row[i] = 1;
        }
        System.out.println(counter);
     }
}


Roll a die according to the number of sides that the user enters I have created a program which rolls a die according to the number of sides that the user enters:

def var():

    import random

    dicesides = int(input("Please enter the amount of sides you want the dice that is being thrown to have. The dice sides available are: 4, 6 and 12: "))
    script(dicesides, random)

def script(dicesides, random):

    if dicesides == 4:
        dice4 = int(random.randrange(1, dicesides))
        print(dicesides, " sided dice, score", dice4)

    elif dicesides == 6:
        dice6 = int(random.randrange(1, dicesides))
        print(dicesides, " sided dice, score", dice6)

    elif dicesides == 12:
        dice12 = int(random.randrange(1, dicesides))
        print(dicesides, " sided dice, score", dice12)

    elif dicesides != 4 or dicesides != 6 or dicesides != 12:
        print("That number is invalid. Please try again.")
        var()

    repeat = str(input("Repeat? Simply put yes or no : "))

    if repeat == "yes":
        var()
    else:
        quit()

var()

Is there a way to shorten this?

Your variables dice4. dice6 and dice12 are essentially the same variables, so let's give them the same name. It now happens that three of the cases are identical, and can be shortened to:

if dicesides != 4 or dicesides != 6 or dicesides != 12:
    print("That number is invalid. Please try again.")
    var()
else:
    score = int(random.randrange(1, dicesides))
    print(dicesides, " sided dice, score", score)

The test for allowed numbers of sides can be shortened to dicesides not in [4, 6, 12], which more clearly shows what you are trying to express. This would allow you to include the number of sides as an optional parameter to the function – some people use 20-sided dices a lot. And actually, there is no reason why you would constrain the user to a certain number of sides anyway.

It is not clear to me what advantage recursion via the var method has over an ordinary loop. One disadvantage is that an avid player could theoretically overflow the stack….

It is also not clear why random is a parameter to the script.

You are using random.randrange wrong. The help() output on my system gives me:

  Choose a random item from range(start, stop[, step]).

  This fixes the problem with randint() which includes the endpoint; in Python this is usually not what you want.

This means that rolling a 6-sided die, you will invoke random.randrange(1, 6), which returns a random int in the range [1, 6) – this excludes the number 6, and you actually pick from 1, 2, 3, 4, 5. Fix this either by 1 + random.randrange(0, dicesides) or preferably random.randint(1, dicesides).


Modifying a Word document template with OpenXML I have the following program that opens a Word document template using the OpenXML library and replaces a couple of phrases with their counterparts from what will be a database (right now its just dummy data). Something about the if-else struture in the nested  foreach loops bothers me. Is there a better way I can accomplish this task? Would regular expressions be a more viable option?

using DocumentFormat.OpenXml.Packaging;
using DocumentFormat.OpenXml.Wordprocessing;
using System;

namespace OpenXmlSearchExample
{
    class Program
    {
        static void Main(string[] args)
        {
            string templatePath = @"C:\users\example\desktop\template.dotx";
            string resultPath = @"C:\users\example\desktop\OpenXmlExample.docx";

            using (WordprocessingDocument document = WordprocessingDocument.CreateFromTemplate(templatePath))
            {
                var body = document.MainDocumentPart.Document.Body;
                var paragraphs = body.Elements<Paragraph>();

                // Iterate through paragraphs, runs, and text, finding the text we want and replacing it
                foreach (Paragraph paragraph in paragraphs)
                {
                    foreach (Run run in paragraph.Elements<Run>())
                    {
                        foreach (Text text in run.Elements<Text>())
                        {
                            if (text.Text == "Plan")
                            {
                                text.Text = string.Format("{0} {1} Plan", DateTime.Now.Year, "Q2");
                            }
                            else if (text.Text == "Project Name")
                            {
                                text.Text = "SUPER SECRET CODE NAME";
                            }
                            else if (text.Text == "WO-nnnn Name")
                            {
                                text.Text = "Maintenance";
                            }
                            else
                            {
                                Console.WriteLine(text.Text);
                                Console.ReadKey();
                            }
                        }
                    }
                }

                // Save result document, not modifying the template
                document.SaveAs(resultPath);
            }
        }
    }
}

Using case-sensitive string matching like that is in my opinion the first nail in the coffin. Instead a case-insensitive dictionary would be a much better option.

Example:

var replacements = new Dictionary<string, Func<string>>(StringComparer.OrdinalIgnoreCase)
{
    ["Plan"] = () => $"{DateTime.Now.Year} Q2 Plan",
    ["Project Name"] = () => $"SUPER SECRET CODE NAME",
};

text.Text = replacements[text.Text]();

This is also much easier to extend/maintain.

You shouldn't mix full type names with var. You can use var not only for normal variables but also inside using statements or foreach loops. Keep it consistent.


Callback for job applicant This is on a job listing site and it has a Linkedin apply button plugin - this provides for a callback the someone has applied for a job.  I've written the callback function which extracts the Job ID (a primary key in my database) and makes an AJX call to indcate that this Job ID has been applied for.

The Linkedin plugin is described here.

It essentially passes this JSON object to the callback:

{
   "event":"success",
   "job":{
      "companyID":"12345",
      "jobID":"9876",
      "jobTitle":"Chief Cat Herder"
   }
}

This is the code placed within the head of the page:

<script type="text/javascript">
function linkedInApplySuccess(jsonReturnObj)
    {
    // no need to use any jquery json functions as already jabe a json object from linkedin (instead os a string)
    // see second answer here http://stackoverflow.com/questions/3569416/parsing-json-string-with-jquery-parsejson

    var job_id = jsonReturnObj.job.jobId;

    $.ajax({
        url: "/php/cv_db/ajax_log_linkedin_application.php",
        data: {job_id: job_id},
        type: "POST"
        });

    };
</script>

Regarding the AJAX, I am not looking for any return data and nor do I care about success or failure as should it fail.  I'm not going to ask someone to apply for the job again. I do have error logging in my PHP file, though.

This looks pretty good, not much room for improvement I think. LinkedIn's JavaScript is calling linkedInApplySuccess, correct? All you have to do is specify data-success-callback, or something similar, in your HTML?

The only minor thing I would do is instead of sending just the job ID, send the entire JSON object. As long as resources aren't an issue, it's always best to capture all data and disregard the stuff you don't use rather than only collect the stuff you're currently using; this is because requirements change, and in the future you may want to do something with the data you've been ignoring. For instance, you may want to calculate how many people are applying for jobs with the position "Chief Weasel Wrangler".

You can alter your code to something like this:

$.post("/php/cv_db/ajax_log_linkedin_application.php", {application: jsonReturnObj});

(application refers to "job application" not "computer application" – maybe too ambiguous)


Max function with variable number of arguments I find it irritating that in standard C++ I can't do std::max(a, b) = x when it's possible and that it can't handle more than 2 arguments. For the second concern I found this std::min tutorial and it looks interesting but the usage of pointers confuses me because I don't understand why they are needed here. Since I want to learn more about c++ I wanted to try to create my own version of std::max while making use of the latest c++ features that I know of.

First I want to make use of concepts to let the user know when the types of arguments are invalid at compilation time:

namespace std1 {

// Helper concept for static_assert
template<typename>
concept False = false;

template<typename T>
concept Boolean = std::is_same_v<T, bool>;

template<typename T>
concept TypeLessThanComparable = requires(T a, T b) {
    { a < b } -> Boolean;
};

template<typename T>
concept TypeLessThanEqComparable = requires(T a, T b) {
    { a <= b } -> Boolean;
};

template<typename T>
concept TypeGreaterThanComparable = requires(T a, T b) {
    { a > b } -> Boolean;
};

template<typename T>
concept TypeGreaterThanEqComparable = requires(T a, T b) {
    { a >= b } -> Boolean;
};

These are the functions themselves:

template<typename T>
constexpr decltype(auto) max(T&& a) noexcept
{
    return std::forward<T>(a);
}

template<typename T>
constexpr decltype(auto) max(T&& a, T&& b) noexcept
{
    if constexpr(TypeLessThanComparable<T>) {
        return a < b ? std::forward<T>(b) : std::forward<T>(a);
    }
    else if constexpr(TypeLessThanEqComparable<T>) {
        return a <= b ? std::forward<T>(b) : std::forward<T>(a);
    }
    else if constexpr(TypeGreaterThanComparable<T>){
        return a > b ? std::forward<T>(a) : std::forward<T>(b);
    }
    else if constexpr(TypeGreaterThanEqComparable<T>) {
        return a >= b ? std::forward<T>(a) : std::forward<T>(b);
    }
    else {
        // if I just put false in static_assert it gives a compilation error no matter what
        static_assert(False<void>, "You called max with invalid arguments, cannot find comparison operators for their type");
    }
}

template<typename T, typename...Ts>
constexpr decltype(auto) max(T&& a, T&& b, T&& c, Ts&&...d) noexcept
{
    return max(a, max(b, max(c, d...)));
}

} // namespace std1

And some tests:

struct A{};

struct B
{
    bool operator<(B const&) const noexcept = delete;
    bool operator<=(B const&) const noexcept = delete;
    bool operator>(B const&) const noexcept = delete;
    bool operator>=(B const&) const noexcept;
};

int main()
{
    static_assert(std1::max(1, 2) == 2);

    int a = 1;
    int b = 5;
    int c = 3;
    int d = 2;

    assert(std1::max(a, b, c, d) == b);

    std1::max(b, c, d) = 4;

    assert(b == 4);

    // This gives a compilation error because the static assertion failed
    // (void)std1::max(A{}, A{});

    // This works
    std1::max(B{}, B{}, B{}, B{}, B{});
}

I want to know if the code is well written and could replace std::max in c++20, maybe it has bugs that I'm not aware of since I am inexperienced in c++. You could also check the compiler explorer link: https://godbolt.org/z/5urgqR.

I think this function is unnecessary.

We can deal with defective classes (that don't properly implement the standard LessThanComparable concept) by either fixing them (preferable) or by providing a comparator argument to std::max:

auto const b_lessthan = [](const B& a, const B& b){ return !(a>=b); };

std::max({B{}, B{}, B{}, B{}, B{}}, b_lessthan);

Sure, you could make a generic adapter using the same if constexpr chain as in this code, but are the defective types really that common?

We can arrange for std::max() to return an lvalue by passing it an initialiser list of std::reference_wrapper for its arguments:

template<typename... T>
constexpr auto& ref_max(T... args)
{
    return std::max({std::ref(args)...,}).get();
}

We now have

#include <algorithm>
#include <functional>
#include <cassert>

int main()
{
    static_assert(std::max(1, 2) == 2);

    int a = 1;
    int b = 5;
    int c = 3;
    int d = 2;

    assert(std::max({a, b, c, d}) == b);

    ref_max(b, c, d) = 4;

    assert(b == 4);

    // This gives a compilation error because the static assertion failed
    // (void)std1::max(A{}, A{});

    // This works
    auto const b_lessthan = [](const B& a, const B& b){ return !(a>=b); };

    std::max({B{}, B{}, B{}, B{}, B{}}, b_lessthan);
}

Which isn't so very different than the main() in the question.


A static array implementation in C++ I'm implementing a basic array data structure with basic functionalities.

#include <iostream>
#include "barray.h"

BArray::BArray(int init_size)
{
    b_array = new int[init_size]();
    array_size = init_size;
}

BArray::BArray(int init_size, int init_val)
{
    b_array = new int[init_size];
    array_size = init_size;

    for(int i = 0; i < init_size; ++i)
        b_array[i] = init_val;
}

BArray::BArray(const BArray & rhs)
{
    array_size = rhs.array_size;
    b_array = new int[array_size];

    for(int i = 0; i < array_size; ++i)
        b_array[i] = rhs[i];
}

BArray::~BArray()
{
    delete [] b_array;
}

int BArray::getSize() const
{
    return array_size;
}

int BArray::operator[](int index) const
{
    return *(b_array + index);
}

int& BArray::operator[](int index)
{
    return *(b_array + index);
}

BArray& BArray::operator=(const BArray& rhs)
{
    if(this == &rhs)
        return *this;

    array_size = rhs.array_size;

    delete [] b_array;

    b_array = new int[array_size];

    for(int i = 0; i < array_size; ++i)
        b_array[i] = rhs[i];

    return *this;
}

std::ostream& operator<< (std::ostream& out, const BArray& arr)
{
    for(int i = 0; i < arr.array_size; ++i)
        out << arr[i] << " ";

    return out;
}

And the header file

#ifndef BARRAY_H
#define BARRAY_H

class BArray
{
public:
    BArray() = delete;                    //Declare the default constructor as deleted to avoid
                                          //declaring an array without specifying its size.
    BArray(const int init_size);
    BArray(int init_size, int init_val);  //Constructor that initializes the array with init_val.
    BArray(const BArray & rhs);           //Copy constructor.
    ~BArray();                            //Destructor.

    int operator[](int index) const;      //[] operator overloading for "reading" index value.
    int& operator[](int index);           //[] operator overloading for "setting" index value.
    BArray& operator=(const BArray& rhs); //Copy assignment operator.

    //Utility functions.
    int getSize() const;

    //Friend functions.
    friend std::ostream& operator<< (std::ostream& out, const BArray& arr);

private:
    int* b_array;
    int array_size;
};

#endif // BARRAY_H

Can you please give me a feedback on what is missing, what is wrong and what is good?
I mean in terms of memory allocation, operators overloading, etc...

Is this is the best way this class can be implemented?

Edit:
This is how I tested the code

#include <iostream>

#include "barray.h"

int main()
{

    //BArray invalid_instance; //Default constructor is deleted.

    BArray barr(5); //Declaring an array of size 5, this initializes all values to zeros.

    std::cout << barr[2] << std::endl;

    barr[2] = 15;   //Setting index 2 to 15.

    std::cout << barr[2] << std::endl; //Reading out value of index 2.

    BArray anotherArray(barr); //Copy constructor.

    std::cout << anotherArray[2] << std::endl;

    anotherArray[3] = 8;

    BArray assignArray = anotherArray; //Copy assignment operator.

    std::cout << assignArray[2] << std::endl;

    std::cout << assignArray; //Printing out array values.

    return 0;
}

I would allow the default constructor and make it default to a 0 size array. with the optimization that b_array is a nullptr.

Then you can also add move constructors/assignment. Where the moved-from object gets a b_array = nullptr;

On of the basic functionalities of a container is iteration. So you should add a begin() and end() that return the b_array and b_array+array_size resp. Don't forget the const versions.


Intended to return account address using account ID, which uses Spring Stored Procedure class I have not used Springs stored procedure class before to return data and I was hoping someone would be able to check this for me..

If it will A. Return data, B. Can be improved. C. If I wanted to return multiple rows how would this need to be changed?

public class AccountAddress extends StoredProcedure implements DAOConstants{

    private static final String STOREDPROC = "storedProc";
    private static final String CURSOR = "cursor";

    public AccountAddress() {
        super(Config.getJdbcTemplate, STOREDPROC);
        declareParameter(new SqlParameter("accountID", OracleTypes.VARCHAR);
        declareParameter(new SqlOutParameter(CURSOR,OracleTypes.CURSOR, new AccountAddressMapper());
        setFunction(false);
        compile();
    }

    public List<String> getAccountAddress(String accountId) {
        Map<String,Object> params = new HashMap<~>();
        Map<String,Object> results = new HashMap<~>();
        List<String> data = new LinkedList<~>();
        try{
            params.put("accountId",accountId);
            results = execute(params);
            data = (List<String>) results.get(0);
        } catch (DataAccessException e) {
            // log errors
        }
        return data;
    }

    public class AccountAddressMapper implements RowMapper<List<String>> {
        @Override
        public List<String> mapRow(Resultset rs, int i) throws SQLException{
            List<String> data = new ArrayList<~>();
            data.add(rs.getString(4));
            data.add(rs.getString(5));
            data.add(rs.getString(6));
            data.add(rs.getString(7));
            data.add(rs.getString(8));
            return data;
        }
    }
}

As @Pimgd mentioned in his answer: You are setting defaults just to overwrite them. this sounds like you are not certain what is going to happen in your code and you want to 'make sure'. It's better to read your code over a few times and make sure you know what will happen, rather than set values you won't need. To another developer it will sound like there is a situation where you will need the results map you assigned.

Don't declare parameters (with defaults) if you don't use them outside of the scope you need them in. when a method grows, the distance between the parameter (at top of try) and the place it gets used grows, making it more likely that it will get reused (bad) or that it gets left behind because developers don't notice it isn't used anymore.
Use the exception, not just to report an error. either return null or return an empty List, whichever you want. (or if you dislike return statements in a catch, set the data field to null/empty list)
If you are going to return the dataset without mutation, why not return it straight away?

I would suggest:

public List<String> getAccountAddress(String accountId) {
    try{
        Map<String,Object> params.put("accountId",accountId);
        Map<String,Object> results = execute(params);
        return (List<String>) results.get(0);
    } catch (DataAccessException e) {
        // log errors
        return new LinkedList<String>();
    }
}