Searching for selectors not using document.querySelector/document.querySelectorAll I had a technical interview the other day and failed a certain question:

    Given a CSS selector (can just be a class name, id or an arbitrary
    number of selectors), return the parent DOM element in an array. The caveat is that you cannot use external libraries or use
    document.querySelector or document.querySelectorAll. Use
    the HTML code below:

For example, if given a CSS selector of img.ClassTwo it would return the img tag, where if the given CSS selector was div#idOne.classTwo it would return 1 div, whereas if the selector was just div, it would return 2 divs. Whereas if I was given img#One it would return an empty array.

My code below works, but I feel that it is too 'cheeky' as it doesn't use either document.querySelector or document.querySelectorAll directly, but uses a variation of document.querySelectorAll - as I can't think of an alternative method that can cater for an arbitrary number of CSS selectors unlike how document.querySelector or document.querySelectorAll can.

Is my code correct? Or can it be improved anyway?

var theFunction = function (input) {

    let arr = [];

    let body = document.getElementsByTagName("BODY")[0];

    let selectors = body.querySelectorAll(input);

    for (i = 0; i < selectors.length; ++i) {
        arr.push(selectors[i].nodeName);
    }

    console.log(arr);

}

Your method doesn't work because it fails to select elements outside the body.

I think what you were supposed to do is:

split the input into class / ID / tagName (using REGEX or whatever)
request elements using document.getElementByID() / document.getElementsByClassName() / document.getElementsByTagName()
return only the elements that match all 3 requests.

Essentially, you code your own querySelectorAll function.


Construct: Rebuffered stream This is part of the Construct library. Rebuffered is used to turn a seekless/tellless stream like from a socket or pipe into a seekable/tellable one like a BytesIO or a file.

If tailcutoff is provided, only that amount of bytes is kept counting from the end of last read write operation backwards. Otherwise entire stream is cached.

class RebufferedBytesIO:
    __slots__ = ["substream","offset","rwbuffer","moved","tailcutoff"]

    def __init__(self, substream, tailcutoff=None):
        self.substream = substream
        self.offset = 0
        self.rwbuffer = b""
        self.moved = 0
        self.tailcutoff = tailcutoff

    def read(self, count):
        startsat = self.offset
        endsat = startsat + count
        if startsat < self.moved:
            raise IOError("could not read because tail was cut off")
        while self.moved + len(self.rwbuffer) < endsat:
            newdata = self.substream.read(128*1024)
            self.rwbuffer += newdata
            if not newdata:
                sleep(0)
        data = self.rwbuffer[startsat-self.moved:endsat-self.moved]
        self.offset += count
        if self.tailcutoff is not None and self.moved < self.offset - self.tailcutoff:
            removed = self.offset - self.tailcutoff - self.moved
            self.moved += removed
            self.rwbuffer = self.rwbuffer[removed:]
        if len(data) < count:
            raise IOError("could not read enough bytes, something went wrong")
        return data

    def write(self, data):
        startsat = self.offset
        endsat = startsat + len(data)
        while self.moved + len(self.rwbuffer) < startsat:
            newdata = self.substream.read(128*1024)
            self.rwbuffer += newdata
            if not newdata:
                sleep(0)
        self.rwbuffer = self.rwbuffer[:startsat-self.moved] + data + self.rwbuffer[endsat-self.moved:]
        self.offset = endsat
        if self.tailcutoff is not None and self.moved < self.offset - self.tailcutoff:
            removed = self.offset - self.tailcutoff - self.moved
            self.moved += removed
            self.rwbuffer = self.rwbuffer[removed:]
        return len(data)

    def seek(self, at, whence=0):
        if whence == 0:
            self.offset = at
            return self.offset
        elif whence == 1:
            self.offset += at
            return self.offset
        else:
            raise ValueError("seeks only with whence 0 and 1")

    def seekable(self):
        return True

    def tell(self):
        return self.offset

    def tellable(self):
        return True

    def cachedfrom(self):
        return self.moved

    def cachedto(self):
        return self.moved + len(self.rwbuffer)

There's no docstring for the class. What kind of thing does an object belonging to this class represent? The text from your post would make a good start.
There's no docstring for the __init__ method. What do I pass for substream? What does tailcutoff mean? Again, the text from your post would make a good start.
There are no docstrings for the cachedfrom and cachedto methods. What do these return?
The cachedfrom and cachedto methods do not take any arguments. Would it make sense for these to be properties instead of methods?
Attributes like substream and rwbuffer do not appear to be intended to be used outside of the class implementation. It is conventional to give these names starting with _. See PEP8:

  Use one leading underscore only for non-public methods and instance variables.

The idea seems to be to create an object that acts like Python's built-in buffered I/O objects. But it doesn't implement the whole interface — for example, there's no close method, no closed property, and so on.

It would make sense to inherit from the abstract base class io.BufferedIOBase so that you get implementations of all the methods (even if some of them are only stubs).
The number 128*1024 appears in two places without explanation. It should have a name, something like _chunk_size, and it might make sense for it to be a keyword argument to the constructor.
In the seek method I would use the constants io.SEEK_SET and io.SEEK_CUR instead of the numbers 0 and 1. That's because the named constants make the intention clear.
BufferedIOBase.read has this behaviour:

  If the argument is omitted, None, or negative, data is read and returned until EOF is reached.

This behaviour is not implemented by RebufferedBytesIO.read. This kind of incompatibility might prevent someone from being able to pass a RebufferedBytesIO object to a function that was expecting an io.BufferedIOBase object.
RebufferedBytesIO.read raises an exception on a short read:

if len(data) < count:
    raise IOError("could not read enough bytes, something went wrong")

But for BufferedIOBase streams, short reads are possible.
The code for reading more chunks from the substream until the target position is reached is duplicated between the read and write methods. This should be refactored into its own method.
The code for doing the tail cut-off is duplicated between the read and write methods. This should be refactored into its own method.
Cutting off the tail using:

 self.rwbuffer = self.rwbuffer[removed:]

is inefficient. In the worst case this code ends up copying the whole of rwbuffer on each read or write, which would be disastrous for performance.
The logic for reading a single chunk from the underlying stream looks like this:

newdata = self.substream.read(128*1024)
self.rwbuffer += newdata

but the documentation for RawIOBase.read says:

  If the object is in non-blocking mode and no bytes are available, None is returned.

If this happens, the += will fail with TypeError: can't concat bytes to NoneType.
The logic for reading chunks from the underlying stream looks like this:

while self.moved + len(self.rwbuffer) < endsat:
    newdata = self.substream.read(128*1024)
    self.rwbuffer += newdata
    if not newdata:
        sleep(0)

If the underlying stream is blocking, the sleep(0) is not necessary (because the read blocks if there is nothing to return. But if the underlying stream is non-blocking, then:

  A BlockingIOError is raised if the underlying raw stream is in non blocking-mode, and has no data available at the moment.

So in neither the blocking case nor the non-blocking case is there any obvious reason to have a sleep here.


Most efficient way to iterate through arrays/can I use .times method in place of while method? I created a quick program to test the precision of randomness generated by the .rand method. The primary question is whether I can use the .times method in place of the while code blocks to increase efficiency and:

Whether such a practice would reduce the amount of processing required/if this would be at all significant.
Whether it's a more common approach, or alternatively is it an infeasible/awkward approach.
Even if it is a less acceptable approach than the one I've taken, how would I use the .times method to execute the same task. If it is inappropriate when should I use .times?

# Initializes array and iterator.
x = []
i = 0

# Stores 4000 random numbers 0..1 in array 'x'
while i < 4000
  x << rand(2)
  i += 1
end

# Initializes array for the purpose of storing zeros and ones in separate
#  arrays for the sake of counting how many instances of each occur in the sample
count_one = []
count_zero = []

# Resets iterator to 0
i = 0

# Stores instances of zero in `count_zero` and instances of one in `count_one`
while i < x.length
  if x[i] == 1
    count_zero << x[i]
    i += 1
  else
    count_one << x[i]
    i += 1
  end
end

# Calculates final averages
zero_average = count_zero.length.to_f/x.length.to_f * 100.0
one_average = count_one.length.to_f/x.length.to_f * 100.0

Additionally, I am curious as to:

How I could possibly have coded this for flexibility to better anticipate future needs. Example: If I later needed to perform the same operations on a larger range of numbers.
If I am using extraneous/obvious facts in my commenting or if my commenting is otherwise not in good practice.
What more insight I might be able to gain in general regarding my current coding practices. Thank you.

If you're interested in more idiomatic Ruby, my advice would be to thoroughly understand Array, Enumerable, and the functional style enabled by using blocks.

The functional style allows you to use the output of one method directly as input to another method without intermediate variables. This is known as method chaining, and it reduces the number of intermediate variables you need to create.

Let's start with your array creation:

# Initializes array and iterator.
x = []
i = 0

# Stores 4000 random numbers 0..1 in array 'x'
while i < 4000
  x << rand(2)
  i += 1
end

First, a bit about code comments. These are a bit gratuitous, as these repeat what the code says. Good comments should explain why something was done, not what is being done. If you feel that the code is non-obvious enough that you'll need a what comment, then it's time to think about refactoring so the code is a bit more self-explanatory.

I've been programming in Ruby for years and have never needed an iteration variable or a while loop. Why? Because the iteration methods such as Array's each or Enumerable's each_with_index are so powerful. So, as you mentioned, you could use times and do this:

x = []
4000.times do
  x << rand(2)
end

This is an improvement, but in this case we can do better. If you look at the constructor options for Array, you'll notice that you can specify the size and also pass a block for an initial value. Therefore, this can be written as:

Array.new(4000){ rand(2) }

#=> [1, 0, 0, 1, 0, 1, 0, 0, 1, 1, 1, 0, 1, 0...]

The output is the same: a 4000-element array consisting of random zeros and ones.

Now let's look at what you want to do next, according to your comment:

# Initializes array for the purpose of storing zeros and ones in separate
#  arrays for the sake of counting how many instances of each occur in the sample

So we want to separate the array into two buckets, based on their value. Thinking functionally, we should ask if, instead of iterating over intermediate output and creating a new data structure (which is also intermediate output because it will be thrown away once you count them), is there something we can do directly to the array? It turns out that the Enumerable module contains partition which can take a block that does exactly that.

Array.new(4000){ rand(2) }.partition{ |digit| digit.zero? }

#=> [[0, 0, 0, 0, 0, 0, 0, 0, ...], [1, 1, 1, 1, 1, 1, 1, 1, ...]]

Partition will separate the array into two sub-arrays: one for which the block returns true; and the other for which the block returns false. Note that I also used the zero? method which is available for numbers (see Fixnum#zero?) where I could have just said digit == 0. Either one is fine, but using zero? allows me to use a shortcut. This is equivalent:

Array.new(4000){ rand(2) }.partition(&:zero?)

#=> [[0, 0, 0, 0, 0, 0, 0, 0, ...], [1, 1, 1, 1, 1, 1, 1, 1, ...]]

This is known as the Symbol#to_proc trick which I won't go into detail here, but it basically allows you to shorten a block in the form {|x| x.method} to &:method. Whenever you want to call the same method on every item in an array, it is useful. You'll see this quite often in Ruby code these days.

Now, you don't really want these sub-arrays, you just want to know how many zeros and ones there are. Again thinking functionally, for each element in the array, you'd like its size. Enumerable's map is useful for transforming each element of an array.

Array.new(4000){ rand(2) }.partition(&:zero?).map{ |subarray| subarray.size }

#=> [2038, 1962]

Which, using the Symbol#to_proc trick can be shortened to

Array.new(4000){ rand(2) }.partition(&:zero?).map(&:size)

#=> [1994, 2006]

So there you have it: using idiomatic Ruby and functional style, you can reduce the first 28 lines down to a single short, yet readable line. To answer question #1, any speed difference would be insignificant with arrays of this size. (Thought it would be interesting to benchmark the two approaches with huge arrays)


Using  instead of other tag types (, , etc) Is it considered bad style to always use
 instead of using, say, ? I know that using specific tags like span better convey the purpose of the node, but is it actually frowned upon to always use div instead?

For example, if I were to have an inline style:

Hello, my name is
John Doe

Hello, my name is John Doe

Where the name-div class would have display: inline set. Is either way considered fine or is there a commonly accepted preference?

Edit:

Here is an actual example so that maybe this won't be off-topic:

You have been online for 30 days.

Where the desired output is something like:

You have been online for 30 days.

(i.e. the 30 has some special styling).

In this case, is it really considered "bad" to use a div with inline styling rather than span? I feel that arguments can be made either way in that span is obviously more accurately describing the function of that node, but you could also say it's preferable to minimize the variety of tags for the sake of consistency across browsers (and choose not to use things like em or strong in lieu of special styling on a div).

The div element, short for division, is a block element mostly used to group larger chunks of HTML, to create CSS-styled layouts.

On the other hand, the span element is an inline element, mostly used to group and stylize text/images. It makes it easy to apply custom styles without changing the general layout.

Also, a div element takes all available width while the span only takes as much width as necessary.

It is certainly not illegal (since HTML5) what you're doing, but to follow semantics I'd use span for what you are trying to achieve.


Project Euler #5 - Smallest Multiple JavaScript is my first programming language, and I'm still pretty new. I'm just looking for feedback. What can I do to make this more efficient and handle larger numbers?

// Project Euler - Smallest Multiple
// This program finds the smallest positive number
// that is evenly divisible by all numbers between 1 and n.

function smallestMult(n){
    var dividends = []; // the numbers by which the program must divide
    for (var i = 1; i <= n; i ++){
        dividends.push(i);
    }
    var result = n; // will increase in increments of n
    var count = 0; //  result has been found when count == n
    while (count < n){
        for (var x = 0; x < dividends.length; x ++){
            if (result % dividends[x] === 0){
                count += 1; // increases count for every successful division
            }
            else {
                count = 0; // if a division fails, count returns to 0
                result += n; // and the result is increased by n
            }
        }
    }
    return result;
}
console.log(smallestMult(12));

"Smallest Multiple" is, in other words, the least common multiple of a range of numbers. The function below implements the least common multiple, lcm:

var lcm = function (a, b) {
    // lcm is defined as:
    // lcm(a, b) = abs(a * b) / gcd(a, b)
    if (isNumber(a) && isNumber(b)) {
        if (!isInteger(a) || !isInteger(b)) {
            throw new Error('Parameters in function lcm must be integer numbers');
        }
        return (Math.abs(a) / gcd(a, b)) * Math.abs(b);
    }
};

The helper functions isNumber and isInteger are as follows:

var isNumber = function(value) {
    return (value instanceof Number) || (typeof value == 'number');
};

var isInteger = function(value) {
  return (value === Math.round(value));
};

Ok, so now we have to implement gcd, the greatest common divisor. Here we can use Euclidean Algorithm:

var gcd = function (a, b) {
    if (isNumber(a) && isNumber(b)) {
        if (!isInteger(a) || !isInteger(b)) {
            throw new Error('Parameters in function gcd must be integer numbers');
        }

        // http://en.wikipedia.org/wiki/Euclidean_algorithm
        while (b != 0) {
          r = a % b;
          a = b;
          b = r;
        }
        return (a < 0) ? -a : a;
    }
};

Now that we have lcm, we'll use the fact that lcm(a,b,c) ≡ lcm(lcm(a,b),c) to make the smallestMultiple function:

var smallestMultiple = function (endOfRange) {
    if (isNumber(endOfRange)) {
        if (endOfRange <= 0) {
            throw new Error('Parameter in function smallestMultiple must be greater than zero');
        }
        if (!isInteger(endOfRange)) {
            throw new Error('Parameter in function smallestMultiple must be a positive integer');
        }
        // Observing that lcm(a,b,c) ≡ lcm(lcm(a,b),c)
        var a = 1;
        for (var i = 1; i <= endOfRange; i++) {
            a = lcm(a, i);
        }
        return a;
    }
};

The full code is as follows:

var isNumber = function(value) {
    return (value instanceof Number) || (typeof value == 'number');
};

var isInteger = function(value) {
  return (value === Math.round(value));
};

var gcd = function (a, b) {
    if (isNumber(a) && isNumber(b)) {
        if (!isInteger(a) || !isInteger(b)) {
            throw new Error('Parameters in function gcd must be integer numbers');
        }

        // http://en.wikipedia.org/wiki/Euclidean_algorithm
        while (b != 0) {
          r = a % b;
          a = b;
          b = r;
        }
        return (a < 0) ? -a : a;
    }
};

var lcm = function (a, b) {
    // lcm is defined as:
    // lcm(a, b) = abs(a * b) / gcd(a, b)
    if (isNumber(a) && isNumber(b)) {
        if (!isInteger(a) || !isInteger(b)) {
            throw new Error('Parameters in function lcm must be integer numbers');
        }
        return (Math.abs(a) / gcd(a, b)) * Math.abs(b);
    }
};

var smallestMultiple = function (endOfRange) {
    if (isNumber(endOfRange)) {
        if (endOfRange <= 0) {
            throw new Error('Parameter in function smallestMultiple must be greater than zero');
        }
        if (!isInteger(endOfRange)) {
            throw new Error('Parameter in function smallestMultiple must be a positive integer');
        }
        // Observing that lcm(a,b,c) ≡ lcm(lcm(a,b),c)
        var a = 1;
        for (var i = 1; i <= endOfRange; i++) {
            a = lcm(a, i);
        }
        return a;
    }
};

The main point of everything above is not to talk to you about efficiency of how to handle larger numbers, but it's about something much more important, especially for beginners such as you and I: abstractions.

To abstract means to give names to things, so you can refer to them as a unit. For example, "stir", "fry" and "boil" are abstractions. Which one is better: "stir for 10 minutes, fry the steak and boil the water" or "move the spoon inside the pot for 10 minutes, heat a little oil on a fridge and put the steak on it, and bring the water to a temperature of a 100 degrees Celsius"? Note that the former is more descriptive even though it's less detailed. Similarly, defining smallestMultiple as the lcm of a range of numbers is more helpful than your ad hoc procedure. When you get a job as a programmer, remember that abstractions are king, and that premature optimizations are the root of all evil.

Also, sorry if the validations were excessive or unclear, as javascript is not my primary language and it's combination of dynamic and weak typing makes me a little afraid of it.


Matrix2x2 struct operators implementation in C++ Will be grateful if anyone would reasonably comment on my code. I am interested in the style only. If there will be any comments which are not related to the style I am glad to read them as well.

Main.cpp

#include "Matrix.h"

int main(void)
{
    Matrix2x2 m;
    std::cout << m << std::endl;
    std::cin >> m;
    std::cout << m << std::endl;

    Matrix2x2 m1{1, 2, 3, 4};
    Matrix2x2 res1{ 5 * m1 };
    std::cout << res1 << std::endl;

    Matrix2x2 m2{ 1, 2, 3, 4 };
    Matrix2x2 res2{ 5+m2 };
    std::cout << res2 << std::endl;

    Matrix2x2 m3{ 1, 1, 1, 1 };
    Matrix2x2 m4{ 1, 2, -1, -1 };
    Matrix2x2 m5 = m3 + m4;
    std::cout << m5 << std::endl;

    m4 += m5;
    std::cout << m4 << std::endl;

    return 0;
}

Matrix2x2.h

#pragma once

#include <iostream>

struct Matrix2x2
{double _00, _01, _10, _11;};

std::istream& operator>>(std::istream&, Matrix2x2&);
std::ostream& operator<<(std::ostream&, Matrix2x2&);
const Matrix2x2 operator*(const double&, const Matrix2x2&);
const Matrix2x2 operator+(const double&, const Matrix2x2&);
const Matrix2x2 operator+(const Matrix2x2&, const Matrix2x2&);
Matrix2x2& operator+=(Matrix2x2&, const Matrix2x2&);

Matrix2x2.cpp

#include "Matrix.h"

std::istream& operator>>(std::istream& is, Matrix2x2& m)
{
    return is >> m._00 >> m._01 >> m._10 >> m._11;
}

std::ostream& operator<<(std::ostream& os, Matrix2x2& m)
{
    return os << m._00 << ' ' << m._01 << std::endl
        << m._10 << ' ' << m._11 << std::endl;
}

const Matrix2x2 operator*(const double& c, const Matrix2x2& m)
{
    Matrix2x2 res{c*m._00, c*m._01, c*m._10, c*m._11};
    return res;
}

const Matrix2x2 operator+(const double& c, const Matrix2x2& m)
{
    Matrix2x2 res{ c+m._00, c+m._01, c+m._10, c+m._11 };
    return res;
}

const Matrix2x2 operator+(const Matrix2x2& m1, const Matrix2x2& m2)
{
    Matrix2x2 almostRes{m1};
    return almostRes+=m2;
}

Matrix2x2& operator+=(Matrix2x2& trg, const Matrix2x2& src)
{
    trg._00 += src._00;
    trg._01 += src._01;
    trg._10 += src._10;
    trg._11 += src._11;
    return trg;
}

Leading _

Some combinations of leading underscore are reserved specifically __ and _<CapitalLetter>, you're one character away from invalid code it's easier to stay away. See this answer.

Document Datatypes

The only way to discern what values are where in your matrix is to assume that the indices of your member variables are row/column, there is no documentation to that effect. I have to look at the out stream operator to figure that out. Always document assumptions or implicit behavior.

Scalar operations missing reverse operator

Matrix m1, m2;
double d;
m1 = m2 * d;

won't compile, you need to implement both version of asymmetric operators somehow.

Various matrix operations missing

For completeness there are quite a few operations missing, multiplication, inverse, determinant ...

const qualifier on returns will cause extra copy

Matrix m1, m2, m3;
m1 = m2 + m3;

will create an extra copy of the result matrix on the assignment. You marked the return value of + as const this means that when it's assigned to a non-const variable a copy will be created. You are doing this on all the binary operators is there a specific reason you did this ?

Fixed Matrices

There should be an easy way to get an identity, or a zero matrix it's not necessary but a nice to have.

Operator + might be inefficient

While delegating the addition to the += operator saves you from repeating some code it could be better to construct the correct matrix in place and return it

const Matrix2x2 operator+(const Matrix2x2& m1, const Matrix2x2& m2)
{
    return Matrix2x2{m1._00 + m2._00, m1._01 + m2._01,
                     m1._10 + m2._10, m1._11 + m2._11};
}

Btw you could use this form for all you operations.

Edit 1

const return value

You responded that this was something the lecturer advised you to do to protect against f() = x being written, if that is what he asks you should do it in that class. They are correct, having a const return value will prevent the compilation. This is a good thing as a statement like that most likely has no effect and the result of f() will immediately go out of scope.

struct X;

X function() {};
const X constFunction() {};

X newValue;
// Compiles correctly but there is no effect (in most cases)
// as the result of function() will be discarded immediately
// There would be an effect if operator= is overridden and has
// side effects. I would consider that a really bad idea.
function() = newValue;

// The following line won't compile, therefor telling you
// "Don't do this"
constFunction() = newValue;

In the code I usually work with i would definitely advise against it. In general I would be suspicious of any code that assigned directly to the result of a free function. I'd rather go with "Don't assign to the result of a free function" as a style rule. In modern c++ doing this prevents the use of move semantics on the result of your function.

With member functions this would be different returning a reference or a const references is something that may happen a lot. So there are cases where you might actually write object.getter() = x. But it's dealing with references as results and not values.

Operator efficiency

I changed the wording to reflect this being an option, it's great that you went and checked the assembly, i did the same (https://godbolt.org/g/Kh3BsJ). For me it looks like that for clang and gcc the solutions are equivalent except for a different use of registers, I am not qualified to say how this affects the actual performance. For MSVC the case looks different the while in place solution is almost the same as what clang/gcc do, the two call path is definitely longer. As a general thought, the simpler your code is the easier it will be to optimize for the compiler. This is a discussion to be had when the need for optimization arises. You're doing great, keep going.


measuring the time taken to copy a std stl container I'm trying to measure the time taken to copy a std::vector<someObject>. Would like to know if this is a correct approach. I tried to do some trivial computations on the copy to prevent the compiler from optimizing out the copy. Should I explicitly write a copy-constructor?

#include <iostream>
#include <new>
#include <chrono>
#include <algorithm>
#include <random>
#include <vector>
using namespace std;

int NUM_OF_ELEMENTS;
int NUM_OF_TRIALS;
char CONTAINER_TYPE;

template<size_t OBJECT_SIZE>
class NonPrimitiveType
{
public:
    int x[OBJECT_SIZE/4];
    NonPrimitiveType()
    {
        for(int i=0;i<OBJECT_SIZE/4;i++)
        {
            x[i] = i;
        }
    }
};

template <typename T, size_t OBJECT_SIZE>
void copyContainer()
{
    T container;
    container.reserve(NUM_OF_ELEMENTS);
    for (int i=0;i<NUM_OF_ELEMENTS;i++)
    {
        container.push_back(*(new NonPrimitiveType<OBJECT_SIZE>));
    }

    long long hash = 0;
    auto begin = chrono::high_resolution_clock::now();
    for (int i=0;i<NUM_OF_TRIALS;i++)
    {
        T copy = container;
        hash = hash + copy.at(0).x[1];
    }
    auto end = chrono::high_resolution_clock::now();
    long long totalTime = chrono::duration_cast<chrono::nanoseconds>(end - begin).count() / NUM_OF_TRIALS;
    printf("%c;%d;%d;%lu;%llu\n",CONTAINER_TYPE, NUM_OF_ELEMENTS, (int)OBJECT_SIZE, totalTime, hash);
}

int main(int argc, char *argv[])
{
    NUM_OF_ELEMENTS = atoi(argv[1]);
    CONTAINER_TYPE = *argv[2];
    NUM_OF_TRIALS = atoi(argv[3]);

    copyContainer<vector<NonPrimitiveType<16>>, 16>();
    copyContainer<vector<NonPrimitiveType<64>>, 64>();
    copyContainer<vector<NonPrimitiveType<256>>, 256>();
    copyContainer<vector<NonPrimitiveType<1024>>, 1024>();
    copyContainer<vector<NonPrimitiveType<4096>>, 4096>();

    return 0;
}

First regarding the correct approach.

printf usage: My output of hash for every case after 200000 trials was always 85899345920000, which is incorrect - due to wrong printf specifiers  - "%c;%d;%d;%lu;%llu\n". Correct ones are  "%c;%d;%d;%lld;%lld\n" if you declare totalTime and hash as long long. Why not use cout i wonder?
Using float for division results. Then again after fixing this in some cases of a small container with few elements i've kept getting totalTime = 0. Thats not good, so you'd want to use a floating variable to store the duration divided by NUM_OF_TRIALS. Otherwise the precision is reduced due to implicit conversion of the division result to integer.
Preparing for measurement. Also as this part goes:

T container;
container.reserve(NUM_OF_ELEMENTS);
for (int i=0;i<NUM_OF_ELEMENTS;i++)
{
    container.push_back(*(new NonPrimitiveType<OBJECT_SIZE>));
}

You're just wasting time here by searching memory to reallocate. Since stl container call default ctor your above code could be replaced with:

T container(NUM_OF_ELEMENTS);

And this will save you lots of time for redundant calls to new here.
sizeof(int) == 4 is true only generally. Use int32_t which gives that guarantee, and then OBJECT_SIZE/4 makes sense. Or better yet divide OBJECT_SIZE by sizeof(int32_t). That way nothing is going to surprise you in the future.
I'd also avoid calling every test together:

copyContainer<vector<NonPrimitiveType<16>>, 16>();
copyContainer<vector<NonPrimitiveType<64>>, 64>();
copyContainer<vector<NonPrimitiveType<256>>, 256>();
copyContainer<vector<NonPrimitiveType<1024>>, 1024>();
copyContainer<vector<NonPrimitiveType<4096>>, 4096>();

Make a switch statement and call only one of them per run, so as to avoid compiler further optimizing the switches between those statements.
If you turn off optimization flags, then you'll get quite an interesting picture of what's happening but your insights wont be applicable once the optimizations kick in. I think its true that measuring timings without optimization flags set to true is quite pointless.

Your code with -O3 optimization:

auto begin = chrono::high_resolution_clock::now();
for (int i=0;i<NUM_OF_TRIALS;i++)
{
    T copy = container;
    hash = hash + copy.at(0).x[1];
}
auto end = chrono::high_resolution_clock::now();

transforms to:

92 [2]      for (int i=0;i<NUM_OF_TRIALS;i++)
0x407d06  <+0x0046>        85 c0                    test   eax,eax
0x407d08  <+0x0048>        0f 8e 02 02 00 00        jle    0x407f10 <copyContainer<std::vector<NonPrimitiveType<16u>, std::allocator<NonPrimitiveType<16u> > >, 16u>()+592>
0x407d0e  <+0x004e>        31 f6                    xor    esi,esi
0x407d10  <+0x0050>        c7 45 c8 00 00 00 00     mov    DWORD PTR [ebp-0x38],0x0
0x407d17  <+0x0057>        c7 45 cc 00 00 00 00     mov    DWORD PTR [ebp-0x34],0x0
0x407d1e  <+0x005e>        66 90                    xchg   ax,ax
95 [1]          hash = hash + copy.at(0).x[1];
0x407daa  <+0x00ea>        8b 43 04                 mov    eax,DWORD PTR [ebx+0x4]
0x407db0  <+0x00f0>        99                       cdq
0x407db1  <+0x00f1>        01 45 c8                 add    DWORD PTR [ebp-0x38],eax
0x407db4  <+0x00f4>        11 55 cc                 adc    DWORD PTR [ebp-0x34],edx
92 [3]      for (int i=0;i<NUM_OF_TRIALS;i++)
0x407db7  <+0x00f7>        83 c6 01                 add    esi,0x1
0x407dbf  <+0x00ff>        39 35 3c f0 40 00        cmp    DWORD PTR ds:0x40f03c,esi
0x407dc5  <+0x0105>        0f 8f 55 ff ff ff        jg     0x407d20 <copyContainer<std::vector<NonPrimitiveType<16u>, std::allocator<NonPrimitiveType<16u> > >, 16u>()+96>
97 [1]      auto end = chrono::high_resolution_clock::now();

So - you're correct that compiler optimizes out the actual copy and leaves the sequential increment of  hash by 1 up to NUM_OF_TRIALS times.

Now, when you write  T copy = container; in a loop you're actually calling Ctor of T, copy assignment operator of T, copy ctor, and then dtor. Then loop counter increments and again 4 calls. Also since nothing is happening to 'T copy', you're getting the same address on the stack reused for the copy and have the stack top pointer moving up and down, while the memory for the elements of the container on the heap also get reused.

If you declare a set outside the loop:

unordered_set<int *> vAddr;//(NUM_OF_TRIALS + 1);
vAddr.insert(&(container[0].x[1]));

and inside the loop add vAddr.insert(&(copy[0].x[1])); then by calling vAddr.size() after the loop you should get the actual number of distinct memory location used by first element. Answer is 2. One for container and one for copy.

In a real life application you'd probably copy the container and let it be consumed by some other function, meaning that a call to T copy = container would probably have to allocate a new space for copy's elements on the heap. So while moving the stack pointer up down is a single instruction, what can become a bottleneck is allocating new memory on the heap.

So with this in mind, if you provide a copy ctor that wont really help measuring imo. I'd try to narrow down your request. What do you actually want to measure? Copy of arrays with dtor? Without dtor? Memory reallocation on the heap?

If you declare T copy outside the loop

T copy;
auto begin = chrono::high_resolution_clock::now();
for (int i=0;i<NUM_OF_TRIALS;i++)
{
    /*T*/ copy = container;
}

then your code with -O3 flag produces the following disassembly:

96 [1]      auto begin = chrono::high_resolution_clock::now();
0x407c11  <+0x0091>        e8 7a 9a ff ff           call   0x401690 <_ZNSt6chrono3_V212system_clock3nowEv>
0x407c16  <+0x0096>        89 c6                    mov    esi,eax
        97 [2]      for (int i=0;i<NUM_OF_TRIALS;i++)
0x407c18  <+0x0098>        a1 3c e0 40 00           mov    eax,ds:0x40e03c
        96 [2]      auto begin = chrono::high_resolution_clock::now();
0x407c1d  <+0x009d>        89 d7                    mov    edi,edx
        97 [3]      for (int i=0;i<NUM_OF_TRIALS;i++)
0x407c1f  <+0x009f>        85 c0                    test   eax,eax
0x407c21  <+0x00a1>        7e 1c                    jle    0x407c3f <copyContainer<std::vector<NonPrimitiveType<16u>, std::allocator<NonPrimitiveType<16u> > >, 16u>()+191>
        99 [1]          /*T*/ copy = container;
0x407c23  <+0x00a3>        8d 45 d0                 lea    eax,[ebp-0x30]
0x407c26  <+0x00a6>        8d 4d dc                 lea    ecx,[ebp-0x24]
0x407c29  <+0x00a9>        89 04 24                 mov    DWORD PTR [esp],eax
0x407c2c  <+0x00ac>        e8 8f 07 00 00           call   0x4083c0 <std::vector<NonPrimitiveType<16u>, std::allocator<NonPrimitiveType<16u> > >::operator=(std::vector<NonPrimitiveType<16u>, std::allocator<NonPrimitiveType<16u> > > const&)>
0x407c31  <+0x00b1>        83 ec 04                 sub    esp,0x4
        97 [4]      for (int i=0;i<NUM_OF_TRIALS;i++)
0x407c34  <+0x00b4>        83 c3 01                 add    ebx,0x1
0x407c37  <+0x00b7>        39 1d 3c e0 40 00        cmp    DWORD PTR ds:0x40e03c,ebx
0x407c3d  <+0x00bd>        7f e4                    jg     0x407c23 <copyContainer<std::vector<NonPrimitiveType<16u>, std::allocator<NonPrimitiveType<16u> > >, 16u>()+163>
        103 [1]     auto end = chrono::high_resolution_clock::now();

It calls the copy assignment operator and copy ctor without dtor. And you're spared the calls to .at operator and incrementation of hash. Though even if you leave it the timings will be quite close. Such alteration seems more or less close to your broadly stated purpose.


My simple PDO wrapper class I made a simple database class and I wanted to know if there are any improvements I could work on as far as readability, efficiency, methods and making it modular goes.

Any other suggestions are also welcome!

Database.php

class Database
{
    private $config;
    private $pdo;

    public function __construct(Config $config)
    {
        $this->config = $config;
    }

    public function query($sql, $params = null)
    {
        $this->connect();

        $sth    = $this->pdo->prepare($sql);
        $params = is_array($params) ? $params : is_null($params) ? null : [$params];

        if ($params && $sth->execute($params)) {
            return $sth;
        } elseif ($sth->execute()) {
            return $sth;
        }

        return false;
    }

    private function connect()
    {
        if (!$this->pdo) {
            try {
                $dsn       = 'mysql:host=' . $this->config->get('mysql/host') . ';dbname=' . $this->config->get('mysql/database') . ';charset=' . $this->config->get('mysql/charset');
                $this->pdo = new PDO($dsn, $config->get('mysql/username'), $config->get('mysql/password'));

                $this->pdo->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
                $this->pdo->setAttribute(PDO::ATTR_DEFAULT_FETCH_MODE, PDO::FETCH_OBJ);
            } catch (Exception $e) {
                die($e->getMessage());
            }
        }
    }
}

I see several things up for improvements:

What problem are you solving? PDO on itself is an abstraction. Your abstraction over it is, on most cases, redundant.

A abstraction is a Mapper for example, which uses the PDO connection to map an object to the database (or whatever form of storage). See The DataMapper pattern

Registry is an antipattern. Why does your database class need all of the config? Just for the 4 details it needs? Just pass those in to the constructor/connect method.

public function __construct($dsn) {

Stay consistent with return types. What does your method return? Is it a resultset? A PDOStatement object? A boolean? Pick one and stick with it. Utilize Exceptions for errors.
Don't catch the (PDO)Exception inside the method. You don't know what you want to do yet! Maybe I want to try to connect, and fallback to files if it fails. Maybe I want to log? Maybe I want to send an email? Your Database object doesn't know the context in which it's called, and so it doesn't know the context in which to deal with errors.
Don't die(). When you die, the entire script dies, just like that. That can be very undesirable in cases where you instantiate a database connection in the middle of the application and it fails. Allow the exception to bubble up, breaking execution when needed, until it's being handled. (And if it isn't, it'll fatal the script anyway!)


Facebook helper class This is my initial Facebook helper class. Is this code efficient or is there a way to improve this code?

using UnityEngine;
using System.Collections;
using Newtonsoft.Json;
using Facebook.MiniJSON;
using System.Linq;

public class FacebookManager : SingletonEternal<FacebookManager>
{
    public string accessToken;
    public FacebookFriends fbFriendsResult;
    public MyFacebookInfo myInfo = new MyFacebookInfo();
    public Texture2D pic;

    IEnumerator Start ()
    {
        if (!FB.IsInitialized)
            FB.Init (null);

        while (!FB.IsInitialized)
            yield return null;

        #if UNITY_EDITOR
        FacebookManager.instance.Login();
        #endif

        if(FB.IsLoggedIn)
        {
            GetMyInformation();
            GetMyFriendsList();
        }
    }

    void SetMyPicture(WWW www)
    {
        pic = www.texture;
    }

    public void Login()
    {
        FB.Login ("user_friends, email, public_profile",LoginCallback);
    }

    void LoginCallback(FBResult result)
    {
        if(result.Error!=null)
        {

        }
        else
        {
            GetMyInformation();
            GetMyFriendsList();
        }
    }

    public void Logout()
    {
        FB.Logout ();
    }

    void GetMyInformation()
    {
        FB.API ("me?fields=id,name,picture",Facebook.HttpMethod.GET,GotMyInformationCallback);
    }

    void GotMyInformationCallback(FBResult result)
    {
        if(result.Error!=null)
        {

        }
        else
        {
            myInfo = JsonConvert.DeserializeObject<MyFacebookInfo>(result.Text);
            MyRequest.CreateRequest(myInfo.picture.data.url,SetMyPicture);
        }
    }

    void GetMyFriendsList()
    {
        FB.API ("me/friends?fields=name,picture",Facebook.HttpMethod.GET,GotMyFriendsListCallback);
    }

    void GotMyFriendsListCallback(FBResult result)
    {
        if(result.Error!=null)
        {

        }
        else
        {
            fbFriendsResult = JsonConvert.DeserializeObject<FacebookFriends>(result.Text);
            fbFriendsResult.data = fbFriendsResult.data.OrderBy(a=>a.name).ToList();
        }
    }

    public void Invite()
    {
        FB.AppRequest ("Invite friends!");
    }
}

I'll refer you to this excellent blog post on Coding Horror in regard to the name of your class. XyzManager in general is never going to be a good name for a class.

You should sort your usings alphabetically and get rid of any that aren't used. Visual Studio (or resharper) can both do that automatically for you (using 'organise usings' or 'clean up code' respectively).

What is your base class? SingletonEternal<FacebookManager>. I could argue that singletons are always 'eternal' - at least for the lifetime of the application... so the name is a bit off too.

As already mentioned - you shouldn't normally use public fields. Properties are better for encapsulation as you can control who sets the value as well as who can read it. At the moment you're letting any consuming code update the values. Auto properties aren't even much more code:

public string AccessCode { get; set;}

This might sound a bit mean, but your code looks ugly to me... Let me explain.

Which is easier to read?

thisfirstone

or

the second one

Hopefully you'd agree that the second one is easier to read, the same is true for your code. Separate parameters with a space, do the same for operators:

FB.API ("me?fields=id,name,picture",Facebook.HttpMethod.GET,GotMyInformationCallback);

Becomes:

FB.API("me?fields=id,name,picture", Facebook.HttpMethod.GET, GotMyInformationCallback);

and result.Error!=null should be result.Error != null. The same applies here a=>a.name change it to a => a.name. It would be even nicer if you changed a to something meaningful.

I know it all seems really trivial but code will be read far more often than it's written/edited so you should be optimizing for read time not saving the odd space characters when you're typing it.


Interview solutions to reverse a string and reverse a sentence Recently in an interview I was asked to

Write a method to reverse a string. I used StringBuilder but was asked not to use reverse method of builder but iterate over the string. Sample input/output provided by interviewer : "abc" --> "cba"
Next was to reverse a sentence. Sample input/output provided by interviewer : "hello world" --> "world hello"

Total time spent on both the solutions : around 10-12 minutes to write and run

Following is the code I wrote and I got rejected from interview. Would appreciate any helpful feedback.

class Solution {

  public static void main(String[] args) {
    String input = "hello world";
    System.out.println(reverseString(input));
    System.out.println(reverseSentence(input));
  }

  private static String reverseString(final String input){
    if(input == null || input.length() == 0){
      return null;
    }
    StringBuilder sb = new StringBuilder();
      for(int i =input.length()-1; i  >= 0; i--){
      sb.append(input.charAt(i));
    }
    return sb.toString();
  }

  private static String reverseSentence(final String input){

    final String[] wordsInInput = input.split(" ");
    StringBuilder sb = new StringBuilder();
    for(int i = wordsInInput.length-1; i >=0; i--){
      sb.append(wordsInInput[i]);
      sb.append(" ");
    }
    final String reversed = sb.toString();
    return reversed.trim();
  }
}

The reverse of an empty string should be an empty string, not null.

In reverseSentence(), the reversed.trim() call inefficiently copies the entire string, less one final space.  You would have been better off shortening the StringBuilder instead of trimming the string.

All of your uses of final are excessive, I think.  The final keyword merely prevents reassignment, and doesn't make anything more immutable than it already is.  For short functions like this, writing final just adds noise.


Calculate the weekday from a date (M-D-Y) This Common Lisp exercise is to write a program that can calculate the weekday given a string of the format "M-D-Y."  It was more challenging than I expected.  If you have suggestions about how to simplify this code, I will be very grateful.

;;  2.7 Day of Week. A program for converting Gregorian dates in the form month-day-year to day of the
;;  week is the following. Let the Gregorian date be represented by M-D-Y where we have the following
;;  definitions:
;;    M is the month of the year. Let m be the month number derived from M by the rule m is M - 2 if M >= 3
;;    and m is M + 10 otherwise.
;;    d is the day of the month.
;;    y is the year of the century.
;;    c is the number of the previous century.
;;  The algorithm is as follows:
;;    (a) A is the integer part of (13m - 1)/5.
;;    (b) B is the integer parg of y/4.
;;    (c) C is the integer part of c/4.
;;    (d) D = A + B + C + d + y - 2c.
;;    (e) Make R equal to the remainder of D/7.
;;    (f ) Interpret R as Sunday if R = 0, Monday if R is 1, etc

(defun read-date () (format t "Enter the gregorian date in the format (M-D-Y): ~%") (read))

(defun flatten (the-list)
  (cond ((null the-list) nil)
    ((atom the-list) (list the-list))
    (t (concatenate 'list (flatten (car the-list)) (flatten (cdr the-list))))))

(defun split (split-char string-to-split)
  (let ((next-split-char (position split-char string-to-split)))
    (if next-split-char (flatten (list (safely-read-from-string (subseq string-to-split 0 next-split-char))
                  (split split-char (subseq string-to-split (+ 1 next-split-char)))))
      (list (safely-read-from-string string-to-split)))))

(defun weekday-code (bigM bigD bigY)
    (let* ((m (if (>= bigM 3) (- bigM 2) (+ bigM 10)))
          (y (mod bigY 100))
          (littleC (/ (- bigY y) 100))
          (A (floor (- (* 13 m) 1) 5))
          (B (floor y 4))
          (C (floor littleC 4))
          (D (- (+ A B C bigD y) (* 2 littleC)))
          (R (mod D 7)))
      (format t "m: ~a y: ~a c: ~a A: ~a B: ~a C: ~a D: ~a R: ~a ~%" m y littleC A B C D R)
      R))

(defun check-date-format (bigM bigD bigY)
  (cond ((or (< bigM 1) (> bigM 12)) (error "The month (~a) is not in the range 1-12." bigM))
    ((< bigY 0) (error "The year (~a) is less than 0." bigY))
    ((or (< bigD 1) (> bigD 31)) (error "The day (~a) is not in the range 1-31." bigD))))

(let ((gregorian-date (read-date)))
      (destructuring-bind (bigM bigD bigY) (split #\- gregorian-date)
    (check-date-format bigM bigD bigY)
    (format t "~a was (or will be) a ~[Sunday~;Monday~;Tuesday~;Wednesday~;Thursday~;Friday~;Saturday~]." gregorian-date (weekday-code bigM bigD bigY))))

If you're allowed to use library functions, split already exists as part of cl-ppcre.

There's a pretty rich set of date/time functions in the Lisp core that make this trivial. If it's for learning purposes, by all means, go ahead and do it by hand. But if you're thinking "What's the best way to get the day-of-week in Lisp?", that's actually ridiculously simple.

(defun day-of-week (day month year)
   (nth-value 6 (decode-universal-time (encode-universal-time 0 0 0 day month year 0))))

Re-arrange order of argumnents to taste. Monday is 0 by default, so you'd need to re-jig your format directive. If you need to be able to handle dates before 1900, there's a more robust version at the Common Lisp Cookbook.


Tetris random piece generator I was inspired by this Reddit /r/dailyprogrammer challenge:

  Contrary to popular belief, the tetromino pieces you are given in a game of Tetris are not randomly selected. Instead, all seven pieces are placed into a "bag." A piece is randomly removed from the bag and presented to the player until the bag is empty. When the bag is empty, it is refilled and the process is repeated for any additional pieces that are needed.

  The pieces are as follows: O, I, S, Z, L, J, T

I was just wanting some general feedback on the code regarding readability, structure, and efficiency.

import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collections;
import java.util.Scanner;

/**
 * Created on 6/23/2016.
 *
 * Inspired by r/dailyprogrammer
 * https://www.reddit.com/r/dailyprogrammer/comments/3ofsyb/20151012_challenge_236_easy_random_bag_system/
 */

public class RandomBag {

    /**
     * This is the main method. The main acquires input from the user
     * then generates a combination of pieces in a particular order.
     * @param args Unused.
     */
    public static void main(String args[]){
        Scanner scanner = new Scanner(System.in); // Used to obtain user input.
        loop : while (true) {
            System.out.print(">>> ");
            String input = scanner.nextLine().replace(" ", "");

            // Checks for special cases.
            switch (input) {
                case "exit" : break loop;
                case "" : continue loop;
            }

            // The generator class will return a string of its results.
            System.out.println(new Generator(Integer.parseInt(input)));
        }
    }

    /**
     * Created on 6/23/2016.
     * Used to create a bag that generates a set amount of pieces in a combination
     * that will never have the same piece displayed 3 times in a row. This is
     * accomplished by placing all pieces in a list and removing them one by one.
     * Once the list has been depleted the list is renewed with the same pieces.
     */

    private static class Generator{
        private int pieceNumber; // Number of pieces that will be generated.
        private String generation = ""; // Used to store the piece generation.
        private final String[] pieces = {"O", "I", "J", "L", "S", "Z", "T"}; // Pieces used in the generation.

        /**
         * Constructor used to store the number of pieces that need to be
         * generated and execute the piece generation method.
         * @param pieceNumber Used to determine how long the generation should be.
         */
        Generator(int pieceNumber){
            this.pieceNumber = pieceNumber; // Initializes number of pieces.
            generatePieces(); // Generates piece combination.
        }

        /**
         * Used to generate the pieces in the specific order. Manipulates class instanced variables.
         */
        private void generatePieces(){
            ArrayList<String> temp = new ArrayList<>(Arrays.asList(pieces)); // Used to store all pieces.
            while(generation.length() <= pieceNumber){ // Generates string until it is the specified size.
                if(temp.isEmpty()) // Checks if list is empty.
                    temp = new ArrayList<>(Arrays.asList(pieces)); // Replaces the pieces in the list.
                Collections.shuffle(temp); // Shuffles the list
                generation += temp.get(0); // Places the first element of the list onto the string.
                temp.remove(0); // Removes the element that was added from the list.
            }
        }

        /**
         * Returns the generation constructed by the generatePieces method.
         * @return The string generated.
         */
        @Override
        public String toString(){
            return generation; // Returns piece generation once generated.
        }
    }
}

The hard part of the challenge is already done for you by Collections.shuffle().  I'll focus on style, efficiency, and elegance.

In the main() function, scanner.nextLine().replace(" ", "") is a weird thing to do.  Would it be acceptable to interpret 5  0 as 50?  Perhaps you meant to do String.trim()?

Style

You have written a comment on every single line, which is too much.  Only beginner programmers are likely to want to state everything twice.  This comment, for example, is silly:

Collections.shuffle(temp); // Shuffles the list

The two lines above it aren't good either.  The // Checks if list is empty comment is noise.  You should not omit the "optional" braces.  You should pick a better variable name than temp.

if(temp.isEmpty()) // Checks if list is empty.
    temp = new ArrayList<>(Arrays.asList(pieces)); // Replaces the pieces in the list.

Efficiency

Repeated concatenation of strings using += is not recommended.  Concatenating n characters that way is O(n2), since each result has to be rebuilt from scratch.  Instead, use a StringBuilder (if you don't know the length of the final result) or a char[] (if you know exactly how long the final string will be).

Calling .remove(0) on an ArrayList is also not recommended.  To fill in the gap after removal, it has to copy over all of the subsequent elements.  Emptying an n-element ArrayList by repeatedly calling .remove(0) is O(n2).  Instead of removing the first element, you could remove the last element.  Better yet, just keep track of the index yourself, so that you don't have to remove anything, and you don't have to keep re-creating the list.

Elegance

A more elegant solution, I think, would be to generate an infinite stream, then take an excerpt from it of the desired length.  Such an infinite stream could actually be used as part of a real Tetris implementation.  Your Generator, on the other hand, has to be instantiated with a length limit (awkwardly named pieceNumber).

Suggested solution

This solution, using streams, produces an infinite sequence, then limits it to length 50.

You can easily tweak it to work like your interactive loop.

import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collections;
import java.util.List;
import java.util.stream.Collectors;
import java.util.stream.Stream;

public class ShuffledStream {
    private ShuffledStream() {}   // Suppress the default constructor

    /**
     * Produces an infinite stream of the elements, reshuffling when
     * each batch is exhausted.
     */
    public static <T> Stream<T> stream(T[] elements) {
        return Stream.iterate(Arrays.asList(elements), list -> {
            Collections.shuffle(list = new ArrayList<T>(list));
            return list;
        }).skip(1)              // First batch is unshuffled; skip it
          .flatMap(List::stream);
    }

    public static void main(String[] args) {
        String fifty = ShuffledStream.stream("OISZLJT".split(""))
                                     .limit(50)
                                     .collect(Collectors.joining());
        System.out.println(fifty);
    }
}

Note that since this solution uses generics, you can reuse the same code to deal from 52-card decks.


Find the sum of all digits that match the following digit in a circular list The challenge

  The captcha requires you to review a sequence of digits (your puzzle input) and find the sum of all digits that match the next digit in the list. The list is circular, so the digit after the last digit is the first digit in the list.

  For example:

  1122 produces a sum of 3 (1 + 2) because the first digit (1) matches the second digit and the third digit (2) matches the fourth digit.
  1111 produces 4 because each digit (all 1) matches the next.
  1234 produces 0 because no digit matches the next.
  91212129 produces 9 because the only digit that matches the next one is the last digit, 9.

My Logic

I keep track of the first item, the last seen item and the total.
I pass over the list, adding the items that match the last seen item.
I handle the first digit by adding the first to the total if it matches the last of the list.

import kotlinx.cinterop.*
import platform.posix.*

const val FILEPATH = "data.dat"
const val ZERO_CODEPOINT = '0'.toInt()

fun main(args: Array<String>) {
    withFile(FILEPATH) { fd ->
        val (first, last, total) = generateSequence {
            val value = fgetc(fd)
            when (value) {
                -1 -> null
                else -> value
            }
        }
            .map { it - ZERO_CODEPOINT }
            .foldIndexed(listOf(-1, -1, 0)) { index, (first, last, total), value ->
                val newTotal = if (last == value) total + value else total
                val newFirst = if (first == -1) value else first
                listOf(newFirst, value, newTotal)
            }

        val result = if (first == last) total + first else total
        println(result)
    }
}
fun <T> withFile(filename: String, block: (CPointer<FILE>) -> T): T {
    val fd = fopen(filename, "r")
    if (fd != null) {
        val ret = block(fd)
        fclose(fd)
        return ret
    } else {
        throw AssertionError("File not found: $filename")
    }
}

foldIndexed -> fold

The foldIndexed line can be replaced with:

.fold(Carry(-1, -1, 0)) { (first, last, total), value ->

This is because the indexed part is unused

Using a data class

A data class can be used to make the values passed along the fold operation more obvious.


Pattern in string count for C I am trying to write a function that will count the occurrences of a pattern in a string without overlap. This is what I have right now.

size_t count(char *string, char *pat) {
    size_t patternLength = strlen(pat);
    size_t stringLength = strlen(string);
    size_t count = 0;
    char *compareString = malloc(patternLength + 1);

    for (int i = 0; i < stringLength; i++) {
        if (i + patternLength > stringLength) return count;
        strcpy(compareString, &string[i], i+patternLength);
        if (strcmp(compareString, pat) == 0) {
            count++;
            i--;
            i+=patternLength; /* non overlapping find */
        }

    }
    free(compareString);
    return count;
}

Drop the Intermediate String

You're allocating compareString and strcpying into it just so that you can use strcmp. But instead, there's also memcmp, which lets you compare from the original string directly:

Fix the loop condition

You're not really looping from 0 to stringLength, you're looping from 0 to stringLength - patternLength. Splitting up those concerns in two is confusing.

Adjusting i

You have:

i--;
i+=patternLength; /* non overlapping find */

We can do both in one:

i += patternLength - 1;

Better solution:

size_t count(char *string, char *pat) {
    size_t patternLength = strlen(pat);
    size_t stringLength = strlen(string);
    size_t count = 0;

    for (int i = 0; i < stringLength - patternLength; i++) {
        if (memcmp(string+i, pat, patternLength) == 0) {
            count++;
            i += patternLength - 1;
        }

    }

    return count;
}


Rock, Paper, Scissors. C++ I'm a beginner in programming and I would like to have feedback and tips to improve. He's a simple command line program. It's a Rock, Paper, Scissors game.

I guess I could use an enum for the "Attack", but I don't know how to use it with cout after that...

Thanks!

#include <iostream>
#include <string>
#include <cstdlib>
#include <ctime>

using namespace std;

enum class Result
{
    win = 1,
    loss = -1,
    tie = 0
};

string ResultToString(Result result)
{
    switch (result)
    {
    case Result::win:
        return "WIN!";
    case Result::loss:
        return "LOSS!";
    default:
        return "TIE!";
    }
}

string DeterminePlay(char playAbbreviation)
{
    if (playAbbreviation == 'r')
    {
        return "Rock";
    }
    else if (playAbbreviation == 'p')
    {
        return "Paper";
    }
    else
    {
        return "Scissors";
    }
}

Result DetermineResult(char playerPlay, char computerPlay)
{
    if (playerPlay == 'r' && computerPlay == 's')
    {
        return Result::win;
    }
    else if (playerPlay == 'p' && computerPlay == 'r')
    {
        return Result::win;
    }
    else if (playerPlay == 's' && computerPlay == 'p')
    {
        return Result::win;
    }
    else if (playerPlay == 's' && computerPlay == 'r')
    {
        return Result::loss;
    }
    else if (playerPlay == 'r' && computerPlay == 'p')
    {
        return Result::loss;
    }
    else if (playerPlay == 'p' && computerPlay == 's')
    {
        return Result::loss;
    }
    else
    {
        return Result::tie;
    }
}

int GenerateRandomNumber(int min, int max)
{
    srand((int)time(0));
    int number{(rand() % max) + min};
    return number;
}

char GenerateComputerPlay()
{
    char computerPlay{};
    int number{GenerateRandomNumber(1, 3)};
    switch (number)
    {
    case 1:
        computerPlay = 'r';
        break;
    case 2:
        computerPlay = 'p';
        break;
    default:
        computerPlay = 's';
    }
    return computerPlay;
}

int main()
{
    setlocale(LC_ALL, "");
    char play{'y'};
    while (play == 'y')
    {
        char computerPlay{};
        char playerPlay{};

        cout << "--- Rock, Paper, Scissors ---" << endl << endl;

        while (playerPlay != 'r' && playerPlay != 'p' && playerPlay != 's')
        {
            cout << "Enter a choice : (R)ock, (P)aper, (S)cissors : ";
            cin >> playerPlay;
            playerPlay = tolower(playerPlay);
        }

        cout << endl << "You played " << DeterminePlay(playerPlay) << endl;

        computerPlay = GenerateComputerPlay();
        cout << "The computer played " << DeterminePlay(computerPlay) << endl;

        Result result{DetermineResult(playerPlay, computerPlay)};
        cout << "It's a " << ResultToString(result) << endl << endl;

        cout << "Play again?" << endl << "(Y)es" << endl << "(N)o" << endl;
        cin >> play;
        play = tolower(play);
    }
    return 0;
}

This is a good exercise in doing things in a more data-driven way. This will result in clearer separation of concepts (i.e., game logic) and allow for more customization easily.

So consider (skipping updating the main program):

#include <iostream>
#include <string>
#include <cstdlib>
#include <ctime>
#include <cctype>
#include <set>
#include <map>

enum Choice
{
    Rock = 1,
    Paper = 2,
    Scissors = 3
};

enum class Result
{
    Win,
    Loss,
    Tie
};

const std::set<std::pair<Choice, Choice> > wins =
{
    { Rock, Scissors },
    { Scissors, Paper},
    { Paper, Rock }
};

const std::map<Result, std::string> result_strings =
{
    { Result::Win, "WIN!" },
    { Result::Loss, "LOSS!" },
    { Result::Tie, "TIE!" }
};

const std::map<char, std::string> play_abbreviations =
{
    { 'r', "Rock" },
    { 'p', "Paper" },
    { 's', "Scissors" }
};

std::string ResultToString(Result result)
{
    // TODO: Decide how to handle the case of input being unrecognized.
    return result_strings[result];
}

std::string DeterminePlay(char playAbbreviation)
{
    // TODO: Decide how to handle the case of input being unrecognized.
    return play_abbreviations[playAbbreviation];
}

Result DetermineResult(Choice playerPlay, Choice computerPlay)
{
    if (playerPlay == computerPlay)
    {
        return Result::Tie;
    }
    else
    {
        return wins.find(std::make_pair(playerPlay, computerPlay)) != wins.cend() ? Result::Win : Result::Loss;
    }
}

int GenerateRandomNumber(int min, int max)
{
    // TODO: Look at <random> to see how this is done better.
    srand(std::time(0));
    return (rand() % max) + min;
}

Choice GenerateComputerPlay()
{
    const int number = GenerateRandomNumber(1, 3);
    return static_cast<Choice>(number);
}

What we did here was that built the logic of who wins into a data structure, which is essentially a directed graph (more precisely, it's a directed 3-cycle). The winner determination now consist of checking whether the input is an arc in our graph; otherwise it's a loss if no such arc exists and a draw if the arc is a self-loop.

Notice also the possibilities you obtain by a having a result_strings: you could put in more "output texts" for a win for instance, so maybe sometimes you'd like to tell the user "WIN!", but other times you could print "GREAT JOB!" or "NICE!"; that'll end up a nightmare from a maintenance and logic point of view if the number of possible strings grows large.


Subclassing pathlib.Path This arises from the SO Question subclassing pathlib.Path.

The pathlib sources  show that Path, when invoked, selects one of its subclasses, WindowsPath or PosixPath, to invoke in place of itself, within it's __new__ method. The following diagram shows a rough idea, with black lines indicating inheritance, red the constructor and green the instance returned.

Kevin points out that while this works for Path itself it prevents any subclass from having the same functionality. Now I'm not entirely clear as to why this is and Kevin does not offer an explanation. Presumably the __new__ method is called before the MRO is setup and one can not rely on super to do the right thing just yet, namely bumping up to the super classes __new__ to select the appropriate subclass.

The answers provided for the question itself include the following alternatives and I'm curious as to which is the best method/better practice.

Since everyday should be Christmas let's Wrap the class (a.k.a. Encapsulation/Clojures). I find quite often this is actually the go to answer for a lot of inheritance questions which I feel kinda defeats the point of subclassing as one then has to manually expose the wrapped classes methods.

from pathlib import Path as _Path_

class Path():
 def __init__(*args, **kvps):
  super().__init__(*args, **kvps)
  self._path_ = _Path_(*args, **kvps)

Instantiate the type of an instantiated class. This is a bit of trickery as you're invoking the class you plan to subclass. It selects the final class for you and you then determine the type it selected and subclass that.

from pathlib import Path

class Path(type(pathlib.Path())):
 pass

Tack on the required features to the selected class. This is roughly what projetmbc did.

from pathlib import Path as _Path_, PosixPath, WindowsPath

class Path(_Path_):
 def __new__(cls, *args):
  if cls is Path:
    cls = WindowsPath if os.name == 'nt' else PosixPath
setattr(cls, "extramethod", _extramethod)
return cls._from_parts(args)

The following is probably a simpler means of achieving the same goal.

Path.extramethod = _extramethod
# Or possibly
WindowsPath.extramethod = _extramethod
PosixPath.extramethod = _extramethod

In the same vane we could simply copy the __new__ method from Path the pathlib sources but that just screams bad practice.
There is one solution missing from the suite of answers there that Kevin hints towards which is to specify the missing attribute reported in the original error message.

from pathlib import Path as _Path_, _windows_flavour, _posix_flavour
import os

class Path(_Path_):
 _flavour = _windows_flavour if os.name == 'nt' else _posix_flavour

BKSpurgeon prompted the following strategy. This sweeps the underlying API details away which is quite nice.

from pathlib import Path as _Path_, PosixPath as _PosixPath_, WindowsPath  as _WindowsPath_
import os

class Path(_Path_) :
 def __new__(cls, *args, **kvps):
  print("Path*")
  return super().__new__(WindowsPath if os.name == 'nt' else PosixPath, *args, **kvps)

class WindowsPath(_WindowsPath_, Path) :
 pass

class PosixPath(_PosixPath_, Path) :
 pass

The figure below illustrates how the interaction plays out in this case. Basically the constructor in Path* selects one of it's sub-classes, WindowsPath* or PosixPath*, whose MRO map back to Path* and Path and the respective intermediate subclass, WindowsPath or PosixPath.

I feel (1) is kind of cop out, (2) is a bit of a hack, (3)
 and (4) seem the better approaches but require peaking under the hood (5) is probably the best as it is quite clean.

If it's possible could one elaborate upon which method 1-5, or if applicable their sub-methods, should one really employ here. The real test is probably checking the MRO in each case and that the final instance includes all the subclasses methods and attributes. I'm quite convinced that pathlib should really be using Metaclasses under the hood and would be keen to hear any commentary upon this aswell.

@Danninno asked us to close off the question, to be honest I forgot about the request for a bit until recently (I've been using the wrapper quite a lot).

Answer

I see that within my own code I went with Option 4. Mostly for the reasons stated in the question. It turned out to be the better fit of the 5 possible options. Option 5 was/is a close contender as it's more OOP'y but it does generate a crazy class structure to simply hide an implementation detail.

Motivation/Discussion

When I originally asked the question I was hoping for bit of a fight/haggle/explanatory answer (Even if I received an opinionated one i.r.o. the forum rules) or atleast a good argument as to which method was the better/worse one. Mostly so that I could improve my decision making process. I see now that some classes in Python are done by it's developers to showcase certain features (After all they like to play/push the bounds somewhat). In this instance Path is constructed in such a way as to avoid the use of Metaclasses (I might be a bit off here, as it's been a while since I looked at the source for pathlib). Another example of this "playing" is seen in the NamedTuple implementation which essentially evals a string to implement it's class, which eliminates any implied inheritance.


Max-digits of digit summing As a challenge I decided to do the following problem:

  How many different numbers with n digits are there whose digits add up to k?

As this is an embarrassingly parallel problem I decided to use a bit of GPGPU programming (partly for learning purposes) using C++AMP and came up with the following solution:

#include <iostream>
#include <vector>
#include <assert.h>
#include <amp.h>
#include <chrono>
#include <ctime>
#include <numeric>

inline unsigned int AddDigits( unsigned int n ) restrict(amp, cpu)
{
        unsigned int sum = 0;
        while( n > 0 )
        {
                sum += n % 10;
                n /= 10;
        }

        return sum;
}

int main()
{
        std::chrono::steady_clock clock;
        unsigned int iSize, iSumRequired;

        std::cout << "Please enter the number of digits: ";
        std::cin >> iSize;
        std::cout << std::endl << "Please enter the sum required: ";
        std::cin >> iSumRequired;
        std::cout << std::endl;

        unsigned long long iMaxNum = std::pow( 10, iSize );

        assert( vecData.max_size() > iMaxNum );

        std::vector<unsigned int> vecData( iMaxNum );
        std::vector<int> vecNumValid( iMaxNum );

        auto tpBegin = clock.now();
        std::iota( vecData.begin(), vecData.end(), 1 );

        concurrency::array_view<const unsigned int, 1> arrayView( iMaxNum, vecData );
        concurrency::array_view<int, 1> numValid( iMaxNum, vecNumValid );

        concurrency::parallel_for_each( numValid.extent, [=]( concurrency::index<1> idx ) restrict( amp ) {
                numValid[idx] = (AddDigits( arrayView[idx] ) == iSumRequired ? 1 : 0);
        } );

        numValid.synchronize();

        int iNumValid = concurrency::parallel_reduce( vecNumValid.begin(), vecNumValid.end(), 0 );

        std::cout << "The number of valid numbers are: " << iNumValid << std::endl;
        auto tpTimeTaken = clock.now() - tpBegin;

        std::cout << "Time Taken: " << std::chrono::duration_cast<std::chrono::milliseconds>(tpTimeTaken).count() << std::endl;

        return 0;
}

This offers a significant improvement over parallel brute-forcing on the CPU (2x speedup versus 8 threads on an 8-core CPU), however out of curiosity I was wondering whether any further speedup could be gained, perhaps through somehow running the parallel_reduce on the GPU rather than on the CPU; removing the necessity to synchronize the array_view?

Moreover, this only allows for up to 8-digits (because of the restriction of the maximum value being less than 231-1) because the GPU only supports types of float, double, int, unsigned int and bool; is there a way to circumvent this and improve the maximum number of digits allowed?

I understand that you are asking from a parallel point of view, but I am unable to provide feedback on that, as I have never worked with this system before. But from my point of view, creating a parallel solution is has been done premature.

Prior to creating a parallel solution to speed up your solution, you should improve the efficiency of the algorithm. At present you consider all numbers in the range of [1, 10N), whereas you only need to evaluate numbers where the number of digits in the number is equal to N. In which case you only need to evaluate numbers in the range of [10N-1, 10N).

You could further improve this via considering permutations and combinations. This would allow you to calculate all the different combinations of the digits for the given range. Once you have these you can then check which of the combinations digits sum to the required total.

If X combinations sum to a given total then you can quickly calculate how many numbers within the require range have N digits and sum to the goal. By computing X * (N Perm N) where N is the number of digits and where permutations are calculated without replacement. Representing just the N digits in the solution.

At this point you would be using a parallel solution to calculate the combination hits, but you may find the bottle neck of data transfer to a GPU causes the parallel solution to perform slower than the non-GPU enabled solution.


Console HangMan game #include <iostream>
#include <string>
#include <algorithm>
#include <time.h>
int faults = 0;
int hangman();
int i = hangman();
int main(){

srand(time(NULL));
while (i == 1){
    std::string words[7]{"Alpha", "Cornwall", "Crepuscular", "Blind", "Steroid", "Plunder", "Talisman"};
    std::string word = words[rand() % 7];
    int n;

    n = rand() % word.length();
    char underscore = '_';
    char checkAnswer = word.at(n);
    word.at(n) = underscore;

    std::cout << word;
    char answer;
    std::cin >> answer;

    if (answer == checkAnswer){
        std::cout << "Correct!";
    }
    else{
        faults++;
        hangman();
    }
}

}
      int hangman(){
       if (faults == 0){
        std::cout << "|----------------------" << std::endl;
        std::cout << "|                      |" << std::endl;
        std::cout << "|" << std::endl;
        std::cout << "|" << std::endl;
    std::cout << "|" << std::endl;
    return 1;
}
else if (faults == 1){
    std::cout << "|----------------------" << std::endl;
    std::cout << "|                      |" << std::endl;
    std::cout << "|                      O" << std::endl;
    std::cout << "|" << std::endl;
    std::cout<<"|" << std::endl;
    return 1;
}
else if (faults == 2){
    std::cout << "|----------------------" << std::endl;
    std::cout << "|                      |"<<std::endl;
    std::cout << "|                      O" << std::endl;
    std::cout << "|                      |" << std::endl;
    std::cout << "|" << std::endl;
        ;
    return 1;
}

else if (faults == 3){
    std::cout << "|----------------------" << std::endl;
    std::cout << "|                      |" << std::endl;
    std::cout << "|                      O" << std::endl;
    std::cout << "|                      |" << std::endl;
    std::cout << "|                     /" << std::endl;
        ;
    return 1;
}
else if (faults == 4){
    std::cout << "|----------------------" << std::endl;
    std::cout << "|                      |" << std::endl;
    std::cout << "|                      O" << std::endl;
    std::cout << "|                      |" << std::endl;
    std::cout << "|                      X" << std::endl;
    return 1;
}
else if (faults == 5){
    std::cout << "|----------------------" << std::endl;
        std::cout << "|                      |" << std::endl;
        std::cout << "|                      O" << std::endl;
        std::cout << "|                      |" << std::endl;
        std::cout << "|                      X" << std::endl;
        std::cout << "                       -" << std::endl;
    return 1;
}
else if (faults == 6){
    std::cout << "|----------------------" << std::endl;
    std::cout << "|                      |" << std::endl;
    std::cout << "|                      O" << std::endl;
    std::cout << "|                      |" << std::endl;
    std::cout << "|                      X" << std::endl;
    std::cout << "                      - -" << std::endl;
    return 1;
}
else if (faults == 7){
    std::cout << "|----------------------" << std::endl;
    std::cout << "|                      |" << std::endl;
    std::cout << "|                      O" << std::endl;
    std::cout << "|                      |" << std::endl;
    std::cout << "|                      X" << std::endl;
    std::cout << "                      - -" << std::endl;
    std::cout << "Dead.Dead.The hangman is dead!You lose!" << std::endl;
    return 0;
}
}

First of all, that's not how hangman is usually implimented, but I won't question it, I'll only review your code.

1. Make sure your code actually works

You currently have an endless while loop. Your hangman() function does not do what it's supposed to, it does not update the value of i. Remove the 'i' variable entirely and change the main loop to "while(hangman()) { ... }". You should also remove the hangman() call at the end of your while loop (in the 'else' block), because it's not needed after this adjustment.

(note: that's the least  you should do. I'd suggest avoiding global variables entirely (you'll still have a global variable 'faults'), but changing that would require redesigning the whole code. You should at least change 'faults' to 'g_faults' to indicate that it's a global variable)

2. Adjust the output of your program

This is sort of a minor nitpick, but your output looks ugly. A space before user input and a newline char after 'correct!' should fix it.

3. Properly indent your code

Your code as it currently is is hard to read and understand, because it does not follow the common identation rules. I'll post your code with proper indentation later.

4. Don't use the NULL macro

Especialy in this situation, there is no need for it (correct me if I'm wrong). Simply write 'time(0)'.

5. Name your variables properly

The name of your variable should make clear what the varable does. Thus, your 'word' string should be 'current_word' or at least 'cWord', the 'checkAnswer' char should be 'currentLetter' or 'missingLetter' ect.

Sorta fixed code

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

int faults = 0;
int hangman();

// No need for 'i' variable

int main(){
    // No need for NULL macro
    srand(time(0));

    // If you declare words[] inside the while loop, it gets created and destroyed every time the loop
    // executes. There is no need for that, thus you should declare the array outside of the loop
    std::string words[7]{"Alpha", "Cornwall", "Crepuscular", "Blind", "Steroid", "Plunder", "Talisman"};

    while (hangman()){
        std::string word = words[rand() % 7];

        // No need to separately declare and initialize the 'n' variable
        int n = rand() % word.length();
        char checkAnswer = word.at(n);

        // No need for an underscore variable, because you only use it once in your code.
        // One could argue that you should still keep the variable in case you wanted to change the
        // symbol, but your variable name indicates that it equals to '_', so you wouldn't use it
        // for that.(What I meant is if you wanted to change "A_pha" to, let's say, "A*pha")
        word.at(n) = '_';

        std::cout << word << ' ';
        char answer;
        std::cin >> answer;

        if (answer == checkAnswer){
            std::cout << "Correct!\n";
        }
        else{
            // Prefer ++a to a++. It doesn't matter in this case, but it's a good practice
            ++faults;
        }
    }
}

int hangman(){
    // Switch statement may have been more fitting here, but I'm too lazy to rewrite
    // your code, so do it yourself if you want to :P
    if (faults == 0){
    std::cout << "|----------------------" << std::endl;
    std::cout << "|                      |" << std::endl;
    std::cout << "|" << std::endl;
    std::cout << "|" << std::endl;
    std::cout << "|" << std::endl;
    return 1;
    }
    else if (faults == 1){
        std::cout << "|----------------------" << std::endl;
        std::cout << "|                      |" << std::endl;
        std::cout << "|                      O" << std::endl;
        std::cout << "|" << std::endl;
        std::cout<<"|" << std::endl;
        return 1;
    }
    else if (faults == 2){
        std::cout << "|----------------------" << std::endl;
        std::cout << "|                      |"<<std::endl;
        std::cout << "|                      O" << std::endl;
        std::cout << "|                      |" << std::endl;
        std::cout << "|" << std::endl;
            ;
        return 1;
    }

    else if (faults == 3){
        std::cout << "|----------------------" << std::endl;
        std::cout << "|                      |" << std::endl;
        std::cout << "|                      O" << std::endl;
        std::cout << "|                      |" << std::endl;
        std::cout << "|                     /" << std::endl;
            ;
        return 1;
    }
    else if (faults == 4){
        std::cout << "|----------------------" << std::endl;
        std::cout << "|                      |" << std::endl;
        std::cout << "|                      O" << std::endl;
        std::cout << "|                      |" << std::endl;
        std::cout << "|                      X" << std::endl;
        return 1;
    }
    else if (faults == 5){
        std::cout << "|----------------------" << std::endl;
            std::cout << "|                      |" << std::endl;
            std::cout << "|                      O" << std::endl;
            std::cout << "|                      |" << std::endl;
            std::cout << "|                      X" << std::endl;
            std::cout << "                       -" << std::endl;
        return 1;
    }
    else if (faults == 6){
        std::cout << "|----------------------" << std::endl;
        std::cout << "|                      |" << std::endl;
        std::cout << "|                      O" << std::endl;
        std::cout << "|                      |" << std::endl;
        std::cout << "|                      X" << std::endl;
        std::cout << "                      - -" << std::endl;
        return 1;
    }
    else if (faults == 7){
        std::cout << "|----------------------" << std::endl;
        std::cout << "|                      |" << std::endl;
        std::cout << "|                      O" << std::endl;
        std::cout << "|                      |" << std::endl;
        std::cout << "|                      X" << std::endl;
        std::cout << "                      - -" << std::endl;
        std::cout << "Dead.Dead.The hangman is dead!You lose!" << std::endl;
        return 0;
    }
    // Again, just a design nitpick, but your images of hangman with 6 and 7 faults are identical.
    // Shouldn't the hangman be dead after 6 faults then?
}


SICP - exercise 2.27 - reversing elements of a list and sublists From SICP

  Exercise 2.27: Modify your deep-reverse procedure of Exercise 2.18 to produce a deep-deep-reverse procedure that takes a list as argument and returns as its value the list with its elements deep-reversed and with all sublists deep-deep-reversed as well. For example,

(define x (list (list 1 2) (list 3 4)))

x
((1 2) (3 4))

(deep-reverse x)
((3 4) (1 2))

(deep-deep-reverse x)
((4 3) (2 1))

Please review my code.

(define (deep-deep-reverse lst)
    (cond ((null? lst) '())
        ((list? lst) (append (deep-deep-reverse (cdr lst)) (list (deep-deep-reverse (car lst)))))
    (else lst)))

I spent an hour doing this, and I am actually extremely surprise on how small this code is in the end. How can I improve this code? Perhaps make it faster?

If you actually indented it that way please use an editor that does
automatically - usually the individual cases of the cond should align,
e.g. like so:

(define (deep-deep-reverse lst)
  (cond
   ((null? lst) '())
   ((list? lst) (append (deep-deep-reverse (cdr lst))
                        (list (deep-deep-reverse (car lst)))))
   (else lst)))

The function looks good.  For clarity it might make sense to move the
middle case to the end, but it's not like that changes much.

However consider that append used in this way is quite expensive
because it repeatedly recreates a "long" list (from the cdr recursion)
to stick the "short" list (from the car part) at the end.

(As an exercise for the reader) you can use an accumulator instead to
avoid append completely (instead cons is enough).  The function
would look pretty similar:

(define (deep-deep-reverse2 lst)
  (define (aux lst acc)
    ...)
  (aux lst '()))


Binary HeapSort and Ternary HeapSort implementation This is my take on binary and ternary heapsort implementation for a university assignment. The code works but I wonder if there are any mistakes or things to improve.

class HeapSorter {

    static int[] array;
    static int heapSize;

    //=============================BINARY HEAPSORT=============================//

    static void maxHeapify(int i){

        int leftChild = left(i);
        int rightChild = right(i);
        int largest;

        largest = leftChild <= heapSize && array[leftChild] > array[i] ? leftChild : i ;

        if(rightChild <= heapSize && array[rightChild] > array[largest])
            largest = rightChild;

        if(largest != i){
            swap(i, largest);

            //Recursively heapify the subtree
            maxHeapify(largest);
        }

    }

    static void binaryHeapSort(int[] toSort){
        array = toSort;
        buildMaxHeap();

        //testing i >= 0 will be executed for i= array.length - 1, array.length - 2,......0,-1. So array.length + 1 times (n + 1)
        //i-- will be executed array.length times (n)
        for(int i = array.length - 1; i >= 0; i--){

            //swap executed n times (n)
            swap(0,i);

            // heapSize - 1 executed n times (n)
            // assignment executed n times (n)
            heapSize = heapSize - 1;

            //maxHeapify executed n times (n)
            maxHeapify(0);
        }

    }

    static void buildMaxHeap(){
        heapSize = array.length - 1;
        for(int i = (array.length - 1) / 2; i >= 0; i--)
            maxHeapify(i);

    }

    static int left(int i){
        return i << 1;
    }

    static int right(int i){
        return (i << 1) + 1;
    }

    //==========================================================================//

    //=============================TERNARY HEAPSORT=============================//

    static void buildMaxHeapT(){
        heapSize = array.length - 1 ;
        for(int i =array.length - 1  / 3; i >= 0; i--)
            maxHeapifyT(i);

    }

    static void maxHeapifyT(int i){

        int leftChild = leftT(i);
        int rightChild = rightT(i);
        int middleChild = middleT(i);
        int largest;

        largest = leftChild <= heapSize && array[leftChild] > array[i] ? leftChild : i;

        if(rightChild <= heapSize && array[rightChild] > array[largest])
            largest = rightChild;

        if(middleChild <= heapSize && array[middleChild] > array[largest])
            largest = middleChild;

        if(largest != i){
            swap(i, largest);
            maxHeapifyT(largest);
        }
    }

    static void ternaryHeapSort(int[] toSort){
        array = toSort;
        buildMaxHeapT();

        for(int i = array.length - 1; i >= 0; i--){
            swap(0,i); //add last element on array, i.e heap root

            heapSize = heapSize - 1; //shrink heap by 1
            maxHeapifyT(0);
        }

    }

    static int leftT(int i){
        return 3 * i + 1;
    }

    static int middleT(int i){
        return 3 * i + 2;
    }

    static int rightT(int i){
        return 3 * i + 3;
    }

    //==========================================================================//

    static void swap(int i, int j){
        int temp = array[i];
        array[i] = array[j];
        array[j] = temp;
    }

    static int[] start (int[] toSort, boolean binaryHeap) {
        if (binaryHeap) {
            binaryHeapSort(toSort);
        } else {
            ternaryHeapSort(toSort);
        }

        return toSort;
    }
}

Also, should I use bit shifting like this:

static int leftT(int i){
    return (i << 1) + i + 1;
 }

instead of

static int leftT(int i){
    return 3 * i + 1;
}

It's not bad. And I'm lazy, so just two comments:

static int[] array;

No statics should be ever used except for constants. Please always create an object, ideally assign everything in the constructor, and let it work on its data.

It's just a few lines more and it makes your class much more usable. Statics are sort of global variables, which means bad. The way you did it, it works... but try to start more threads and call start concurrently.

  Also should i use bit shifting like this:

No, except maybe on Android. Let the JVM do its work. It probably knows much more tricks than you (e.g., doing x / 10 without division instruction, i.e., maybe 3 times faster).


Go implementation of dining philosophers Follow-up of this question.

Questions:

is this code a correct implementation of the Dining Philosophers problem?
what is there to improve regarding Go Best Practices?

package main

import (
    "fmt"
    "sync"
)

func philos(id int, left, right chan bool, wg *sync.WaitGroup) {
    fmt.Printf("Philosopher # %d wants to eat\n", id)
    <-left
    <-right
    left <- true
    right <- true
    fmt.Printf("Philosopher # %d finished eating\n", id)
    wg.Done()
}

func main() {
    const numPhilos = 5
    var forks [numPhilos]chan bool
    for i := 0; i < numPhilos; i++ {
        forks[i] = make(chan bool, 1)
        forks[i] <- true
    }
    var wg sync.WaitGroup
    for i := 0; i < numPhilos; i++ {
        wg.Add(1)
        go philos(i, forks[(i-1+numPhilos)%numPhilos], forks[(i+numPhilos)%numPhilos], &wg)
    }
    wg.Wait()
    fmt.Println("Everybody finished eating")
}

All in all, it's not a bad implementation at all. The bulk of my comments will focus on idiomatic golang stuff, and some small tweaks you can make to the main function. As I usually do here, I'll go through it line by line

func main() {
    const numPhilos = 5

OK, so you're starting out defining an untyped constant in your main. That's perfectly valid, and it doesn't make much of a difference, but generally speaking, I'd define my constants outside of functions. This makes it easier to centralise your constants, see what constants are used in the file/package (if you're exporting them), and makes it easier to break up your code into smaller functions further down the line. Moving on:

var forks [numPhilos]chan bool

OK, so arrays can be used in go, but it's generally recommended you don't. The rule of thumb is: use slices if you can. Next:

for i := 0; i < numPhilos; i++ {
    forks[i] = make(chan bool, 1)
    forks[i] <- true
}

Again, no real issues here, only, you're assigning a channel to an index in an array, and then writing to it, accessing the array again. I'd use a scoped variable instead. Next:

var wg sync.WaitGroup
for i := 0; i < numPhilos; i++ {
    wg.Add(1)
    go philos(i, forks[(i-1+numPhilos)%numPhilos], forks[(i+numPhilos)%numPhilos], &wg)
}
wg.Wait()

Right, short of what I pointed out in the comment about forks[(i+numPhilos)%numPhilos] being the same as forks[i], this all works, but there's quite a few things we can improve:

you're creating a var wg sync.WaitGroup, and passing pointers to it. Good, but why not create a pointer literal. It's safer (less likely to pass by value accidentally), and code is easier to read IMO
You're incrementing i, and accessing forks, knowing full well that the len(forks) won't be exceeded. After all, your loop is the same as the one you used to initialise forks. So why not loop over forks to begin with?
wg.Add(1) is incrementing the waitgroup for each routine, but you clearly know beforehand how many routines you're going to spin up. You can add that total number of routines to your waitgroup outside of the loop.
I don't like the names numPhilos and philos for a func.
You're passing the waitgroup as a last argument. It's more common to see context.Context as the first argument, and things like a waitgroup (controlling the runtime and routines) as first arguments, rather than last

Last line:

fmt.Println("Everybody finished eating")

This should not be the end of your program. You should close the channels!

Now, let's put all of this together:

const numPhilos = 5

func main() {
    // create slice, not an array - set capacity to numPhilos
    forks := make([]chan bool, 0, numPhilos)
    for i := 0; i < numPhilos; i++ {
        // create channel in local scope
        ch := make(chan bool, 1)
        ch <- true // write to channel directly
        forks = append(forks, ch) // append to forks slice
    }
    // I prefer literals, because I can create a pointer type directly
    wg := &sync.WaitGroup{}
    // add 1 for each channel in forks
    wg.Add(len(forks))
    for i, ch := range forks {
        // forks[i] is now ch, get the left one using the method you are using already
        go philos(wg, i, forks[(i+numPhilos-1)%numPhilos], ch)
    }
    wg.Wait()
    // close channels
    for _, ch := range forks {
        close(ch)
    }
    // done
    fmt.Println("Everybody finished eating")
}


ReactJS paginated component with React Router I've recently started journey with ReactJS and to begin with I wanted to try something a bit more complex - paginated list of elements with back functionality enabled.
Below are two main components that I'd like to get feedback about. Especially PagedListings.
import './App.css';
import PagedListings from './components/PagedListings';
import ScrollToTop from './components/ScrollToTop';
import React from "react";
import { Route } from 'react-router';
import { BrowserRouter as Router } from 'react-router-dom';

function App() {
  return (
    <Router>
    <ScrollToTop>
       <Route path={["/", "/:page"]} component={PagedListings}/>
    </ScrollToTop>
    </Router>
  );
}

export default App;

And the actual component executing planned task
import React, { useMemo } from "react";
import ListingsContainer from "./ListingsContainer";
import Pagination from "@material-ui/lab/Pagination";
import { useHistory, useLocation } from "react-router-dom";

const LISTINGS = ["Rec1", "Rec2", "Rec3",
  "Rec4", "Rec5", "Rec6", "Rec7", "Rec8",
  "Rec9", "Rec10",];
const DEFAULT_PAGE = 1;
const MAX_PAGE_COUNT = 10;

function PagedListings(props) {
  const history = useHistory();
  const location = useLocation();
  const urlParams = useMemo(() => {
    return new URLSearchParams(location.search);
  }, [location.search]);

  const handleChange = (
    event: React.MouseEvent<HTMLButtonElement> | null,
    newPage: number
  ) => {
    let oneBasedNewPage = newPage;

    urlParams.set("page", oneBasedNewPage.toString());
    updateURL();
  };

  const updateURL = () => {
    history.push({
      pathname: location.pathname,
      search: `?${urlParams}`,
    });
  };

  function getPageNumber() {
    let newPage = parseInt(urlParams.get("page")) || DEFAULT_PAGE;
    return newPage <= MAX_PAGE_COUNT ? newPage : MAX_PAGE_COUNT;
  }

  return (
    <>
      <ListingsContainer listings={LISTINGS.slice(0, getPageNumber())} />
      <Pagination
        count={MAX_PAGE_COUNT}
        defaultPage={DEFAULT_PAGE}
        page={getPageNumber()}
        onChange={handleChange}
      />
    </>
  );
}

export default PagedListings;

Your code looks quite reasonable. There are only a few small things I notice:

Indent the JSX in App a bit more: ScrollToTop is a child of Router, so it should be indented further, not on the same level as Router. (It doesn't really make a difference here, but it's good to be consistent, and could be an improvement if that section ever got more complicated)

Avoid let, prefer const - ESLint rule here.

Or, in the case of oneBasedNewPage, consider defining the argument with that name instead of creating a new variable - and maybe call it oneIndexed, not oneBased:
const handleChange = (
  event: React.MouseEvent<HTMLButtonElement> | null,
  oneIndexedNewPage: number
) => {

URLSearchParams does not differentiate between numbers and strings.
urlParams.set("page", oneBasedNewPage.toString());

can be
urlParams.set("page", oneIndexedNewPage);

if you wish.

Consider if using Math.min might be clearer than the conditional operator:
function getPageNumber() {
  const newPage = parseInt(urlParams.get("page")) || DEFAULT_PAGE;
  return Math.min(newPage, MAX_PAGE_COUNT);
}