Project Euler #4 in C++: largest palindrome product of two 3-digit numbers Project Euler #4

  A palindromic number reads the same both ways. The largest palindrome
  made from the product of two 2-digit numbers is 9009 = 91 × 99.

  Find the largest palindrome made from the product of two 3-digit
  numbers.

Any reviews are accepted.

#include <iostream>
#include <algorithm>
#include <string>
bool isPalindrome(std::string num);
int findLargestProduct();
int main(){
        std::cout << findLargestProduct() << std::endl;
        return 0;
}

bool isPalindrome(std::string  num){
        std::string oldnum = num;
        reverse(oldnum.begin(),oldnum.end());
        if(oldnum == num)
                return true;
        else
                return false;

}

int findLargestProduct(){
        int largest,prod =0;
        for(int i =100;i<=999;i++){
                for(int j =100; j<=999;j++){
                        prod = i*j;
                        if(isPalindrome(std::to_string(prod)) && prod > largest){
                                largest = prod;
                        }
                }

        }
        return largest;
}

Excessive spacing and main()

To start with, 4-space tabs please. 8 is tooooo much, you're using up all the horizontal space!

Additionally, return 0; from main() is optional. If you omit it, the compiler will provide it for you - so you don't have to write it.

isPalindrome()

This function doesn't modify its input - you take a copy, reverse it, and then compare the two. But actually you're making two copies of the input - one into num and the other into oldnum. If you took the argument by reference-to-const, you save yourself a copy for free:

bool isPalindrome(std::string const& num) { .. }

Additionally, the argument should probably be named oldnum - since that's the one that doesn't change.

Avoid code that looks like: if (expr) return true; else return false; That's an extremely verbose way of writing return expr; Altogether, this becomes:

bool isPalindrome(std::string const& oldnum) {
    std::string revnum = oldnum;
    std::reverse(revnum.begin(), revnum.end());
    return oldnum == revnum;
}

However, this is pretty inefficient - we have to make another copy of oldnum just to check if it's reversible? We can do that directly in one go. Just compare the ith element to the N-ith element and make sure they all line up:

bool isPalindrome(std::string const& num) {
    for (size_t i = 0; i < num.size()/2; ++i) {
        if (num[i] != num[num.size()-i-1]) {
            return false;
        }
    }

    // still here? must be a palindrome
    return true;
}

This saves you a copy, so will be quite a bit faster.

findLargestProduct()

There is undefined behavior here. When you write:

int largest, prod = 0;

That isn't initializing largest, only prod. Which is unfortunate since it's only largest that needs to be initialized. Avoid this kind of multiple initialization, and simply do:

int largest = 0;

prod you can simply declare inside the inner-most loop.

Now, the slowest part of this function is isPalindrome(). That's an expensive operation to do - so we want to do it as rarely as possible! As an optimization, we can check if prod > largest first - so you avoid the expensive logic in all the cases where the outcome doesn't matter. If the product isn't the new max, who cares, right? That is:

int prod = i*j;
if (prod > largest && isPalindrome(std::to_string(prod))) {
    largest = prod;
}

Once we're there, let's really make sure we never have to do it by iterating in the opposite order:

for (int i=999; i>=100; --i) {
    for (int j=999; j>=100; --j) {
        ...
    }
}

Math is cool

Assuming the answer will be 6 digits (seems safe), we know it's divisible by 11. We know this because the digits of the number will be \$abccba\$, and the handy rule for checking divisibility by 11 is to sum the even digits and the odd digits and check if the difference is divisible by 11 - and both the even digits (\$b+c+a\$) and the odd digits (\$a+c+b\$) have the same sum. Since we know it's divisible by 11, we know one of the factors must be divisible by 11. Let's pick j. This let's us reduce the loop to:

for (int i=999; i>=100; --i) {
    for (int j=990; j>=100; j-=11) { // largest 3-digit multiple of 11
       ...
    }
}

Breakdown of timing

Here's a breakdown of all the things I just suggested:

as-is                  171.5ms
better isPalindrome    138.0ms
flip ordering            9.9ms
iterate backwards        4.8ms
count by 11s             0.2ms


Simple chart class for SVG path generation Here is a class I have:

class Chart {
    constructor (options) {
        this.cx = options.center[0]
        this.cy = options.center[1]
        this.radius = options.radius
    }

    getTopLeftSegmentPath (perc = 1) {
        const point = (this.radius / 100) * perc
        const start = [this.cx - point, this.cy]
        const end = [this.cx, this.cy - point]

        return getPath(point, start, end)
    }

    getTopRightSegmentPath (perc = 1) {
        const point = (this.radius / 100) * perc
        const start = [this.cx, this.cy - point]
        const end = [this.cx + point, this.cy]

        return getPath(point, start, end)
    }

    getBottomLeftSegmentPath (perc = 1) {
        const point = (this.radius / 100) * perc
        const start = [this.cx, this.cy + point]
        const end = [this.cx - point, this.cy]

        return getPath(point, start, end)
    }

    getBottomRightSegmentPath (perc = 1) {
        const point = (this.radius / 100) * perc
        const start = [this.cx + point, this.cy]
        const end = [this.cx, this.cy + point]

        return getPath(point, start, end)
    }

    getPath (point, start, end) {
        const tc = xy => xy.join(',')

        return `M${tc(start)} A${point}, ${point} 0 0 1 ${tc(end)} L${this.cx}, ${this.cy} Z`
    }
}

Usage:

const chart = new Chart({ center: [200, 200], radius: 100 })

chart.getTopLeftSegmentPath(50)
chart.getTopRightSegmentPath(13)
chart.getBottomLeftSegmentPath(20)
chart.getBottomRightSegmentPath(100)

I am not sure what is the best way to optimize or simplify it.

Maybe there is a chance to have a single getPath method instead of all these getTopRightSegmentPath, getTopLeftSegmentPath, etc.

Repetition, similarities, and redundancies

To simplify code you start by looking for repetition and similarities. In most cases you can remove the repeating/similar code by adding variables to take care of the minor differences.

Count how often you use a variable name, eg end and start are defined 5 times, and you have had to type them in 10 times each, and point has even more.

Variable names also should not have repetition. If you find yourself creating a pile of variables that all share some prefix or postfix, it's a good sign that you can optimize and simplify.

Don't do something twice when once is enough. A common practice is

const blah = foo + bar;
return blah;

The blah is redundant return foo + bar; does the same with less complexity.

see note (*1)

Functional similarities

You have four almost identical functions that contain the following

    const point = (this.radius / 100) * perc
    const start = [this.cx, this.cy + point]
    const end = [this.cx - point, this.cy]
    return getPath(point, start, end)

The only difference is the addition or subtraction of point to the coordinates. For the four functions there are only 4 different combinations which you can define as an array of values, 0 for no change, -1 for subtraction and 1 for addition.

For the above snippet that would be [0, 1, -1, 0] so if you had a generic function that accepted an array

getCorner(perc, cornerDesc) {
    const point = (this.radius / 100) * perc;
    const start = [this.cx + cornerDesc[0] * point, this.cy + cornerDesc[0] * point];
    const end = [this.cx + cornerDesc[2] * point, this.cy + cornerDesc[3] * point];
    return getPath(point, start, end);
}

But this is a bit obscure and not very intuitive. We still want to to use named function calls to get a corner, "topleft..." rather than a non descriptive array to define a corner. (see below)

Variable repetition

If we look further there is still repetition of the arguments passed to getPath. We can define  point, start, and end in the getPath function and pass it the perc argument and array of offsets.

Looking at the functions named getTopRightSegmentPath, getTopLeftSegmentPath ... etc, each contains SegmentPath which is a good clue in what to remove and create once.

A good rule of thumb. If adding the same name to a bunch of variables, that similarity should be an object, eg getTopRightSegmentPath becomes segmentPath.topRight (the get is redundant in lieu of more info about Path's functionality)

Though Javascript class syntax is not conducive to complicated Objects (*), so it is best to redefine the whole Path object as a factory function (see rewrite)

So we can create something like

segmentPath = {
    topLeft(perc = 1) { return getPath(perc, [-1,0,0,-1]) },
    topRight ... // and so on
}

Redundency

Looking at the getPath function we can remove some redundancy now that we have moved the defining of end and start into that function. The function xy can be removed as we can directly define start and end as strings skipping the need for the function xy and creating arrays that we immediately join.

this is redundant.

My favorite redundancy removal is this. When I see code that is full of this I see bad practice. The token this should only ever be used for properties that are to be accessed outside the object. The (so popular) class token encourages the exposure of everything, if you see the token class you can guarantee that by word count this is the most used (eg you use this 25 times) and mostly incorrectly.

Personalty, if you put effort in you can actually write good Javascript without ever needing this

The rewrite.

So with all that said let's put it together, (I can only guess at the overall requirements of the Path object so the rewrite may not suit additional behavioural and state management needs)

The variable cx, cy, and radius dont need to be exposed so we can close over them, the function getPath look like it is only used internally so again I will close over it.

function Chart(options) {
    var cx = options.center[0];
    var cy = options.center[1];
    var radius = options.radius;
    function getPath(offsets, perc = 1) {
        const rad = (radius / 100) * perc;
        const start = `${cx + offsets[0] * rad},${cy + offsets[1] * rad}`;
        const end = `${cx + offsets[2] * rad},${cy + offsets[3] * rad}`;
        return `M${start} A${rad}, ${rad} 0 0 1 ${end} L${cx}, ${cy} Z`;
    }
    return {
        segmentPath : {
            topLeft(perc) { return getPath([-1, 0, 0, 1], perc) },
            topRight(perc) { return getPath([0, -1, 1, 0], perc) },
            bottomLeft(perc) { return getPath([0, 1, -1, 0], perc) },
            bottomRight(perc) { return getPath([1, 0, 0, 1], perc) },
        }
    };
}

Which is used as follows

const chart = Chart({center : [x,y], radius : 10});
const svgPath = chart.pathSegment.topLeft(2);

Alternative

I did consider the following...

    return {
        topLeft(perc = 1) { return getPath(prec, -1, 0, 0, 1) },
        topRight(perc = 1) { return getPath(prec, 0, -1, 1, 0) },
        bottomLeft(perc = 1) { return getPath(prec, 0, 1, -1, 0) },
        bottomRight(perc = 1) { return getPath(prec, 1, 0, 0, 1) },
    };

...with the function getPath defined as

    function getPath(perc, ...offsets) {

But the repetition of perc = 1 annoyed me

Access properties via getters and setters

If you need to change the properties it is better to use getters and setters, though only as example, you can include some vetting on the setters

    return {
        segmentPath : {
            topLeft(perc) { return getPath([-1, 0, 0, 1], perc) },
            topRight(perc) { return getPath([0, -1, 1, 0], perc) },
            bottomLeft(perc) { return getPath([0, 1, -1, 0], perc) },
            bottomRight(perc) { return getPath([1, 0, 0, 1], perc) },
        }
        get center() { return [cx, cy] },
        get radius() { return radius },
        set center(cent) { [cx, cy] = cent },
        set radius(r) { radius = r },
    }

Usage

const chart = Chart({center : [x,y], radius : 10});
chart.radius *= 2;
console.log(chart.radius); // >> 20
chart.center = [x * 2, y * 2];
const svgPath = chart.pathSegment.topLeft(2);

Look mum no this

Some notes

(*) Called Object/s because in Javascript there are no "classes" only Objects. The class token is just a way of defining an object.

(*1) Note that in some cases to such as conditional and loop statements (where you use conditional operators) it is better to define befor the statement variables that require operations.

if (foo & bar === something) {

is better written as

const blah = foo & bar;
if (blah === something) {

This is because it can be a source of bug when precedence can give non intuitive results.

if(10 & 0 === 0) { // false
if((10 & 0) === 0){ // true
return 10 & 0 === 0 // returns 0 not a boolean
return (10 & 0) === 0 // returns a boolean true

Unless you have very familiar with all off javascript's operator precedence dont add expressions to statements.

Also if the following line becomes too long, dont remove

const blah = foo + bar + apple + sauce;
const poo = blah + " something else is " + value * 2 + " when " + num + count;

is better than

const poo = (foo + bar + apple + sauce) + " something else is " + value * 2 + " when " + num + count;

^^ scroll bar bad :(


Number of anagrams in an array of words (Codewars) Codewars Kata in Question.

  An anagram is a word, a phrase, or a sentence formed from another by
  rearranging its letters. An example of this is "angel", which is an
  anagram of "glean".

  Write a function that receives an array of words, and returns the
  total number of distinct pairs of anagramic words inside it.

  Some examples:

  There are 2 anagrams in the array ["dell", "ledl", "abc", "cba"]

  There are 7 anagrams in the array ["dell", "ledl", "abc", "cba", "bca", "bac"]

My code is usually not optimal, and I would like to improve my coding practices, and coding sense which is why I try to put up for review all nontrivial code I write.

My Code

from collections import Counter
import math

def choose(n, k):   #Each distinct anagram pair is a selection from the set of all words that have the same count.
    f = math.factorial
    return (f(n)//(f(n-k)*f(k)))

def anagram_counter(words):
    words = list(set(words))    #Suppress duplicates.
    unique = set()  #Set of unique words.
    count = {}  #Dictionary that stores the count for each word.
    unique = set()
    for word in words:
        #The current word is not an anagram of any word already in the set.
        wordID = Counter(word)
        if not unique or all((wordID != count[distinct][1] for distinct in unique)):
            unique.add(word)
            count[word] = [1,wordID]    #A tuple containing number of anagrams of a word and its `wordID`.
        else:   #If the current word is an anagram of a word already in the set.
            for distinct in list(unique):
                if count[distinct][1] == wordID:    #If the word is an anagram of a preexisting word.
                    count[distinct][0] += 1     #Increment the counter.
                    break
    return 0 if count == {} else sum((choose(itm[0], 2) for itm in count.values() if itm[0] > 1))

You are making your life too difficult, IMO. Whenever you iterate over all members of a set to see if some element is in it or write list(unique), you are probably doing something wrong.

I would just transform each word into a canonical form (you could choose a frozenset of the Counter items or just a sorted string). Then just count how often each appears:

def anagram_counter(words):
    count = Counter(frozenset(Counter(word).items()) for word in words)
    return sum(choose(x, 2) for x in count.values() if x > 1)

def anagram_counter2(words):
    count = Counter("".join(sorted(word)) for word in words)
    return sum(choose(x, 2) for x in count.values() if x > 1)

You could optimize the last line by using Counter.most_common and stopping as soon as you get to the elements that appeared only once:

from itertools import takewhile

def anagram_counter3(words):
    count = Counter("".join(sorted(word)) for word in words)
    return sum(choose(x[1], 2)
               for x in takewhile(lambda t: t[1] > 1, count.most_common()))

Comparing the timings for some small input:

x = ["foo", "bar", "oof", "rab", "foobar"]
%timeit anagram_counter(x)
# 27.2 µs ± 1.4 µs per loop (mean ± std. dev. of 7 runs, 10000 loops each)
%timeit anagram_counter2(x)
# 9.71 µs ± 656 ns per loop (mean ± std. dev. of 7 runs, 100000 loops each)
%timeit anagram_counter3(x)
# 11.9 µs ± 492 ns per loop (mean ± std. dev. of 7 runs, 100000 loops each)
%timeit anagram_counter_op(x)
# 25.6 µs ± 472 ns per loop (mean ± std. dev. of 7 runs, 10000 loops each)

And for some larger inputs:

import random
import string
import numpy as np

# increasing number of words, always 5 letters
x1 = [["".join(random.choices(string.ascii_lowercase, k=5)) for _ in range(n)]
     for n in np.logspace(1, 4, num=10, dtype=int)]
# increasing length of words, always 500 words
x2 = [["".join(random.choices(string.ascii_lowercase, k=n)) for _ in range(500)]
     for n in np.logspace(1, 4, num=10, dtype=int)]

(Note that both axis are logarithmic on both plots.)


C++ algorithm to implement multiple operators in one I have a class called IntMatrix which has 2 fields:
Dimensions dimensions;//To save height and width of my matrix
int *data;//For saving data

and I need to override the operators >,>=,<,<=,==,!= which take a scalar and compare each single value in my matrix with that scalar accordingly, and returns a new matrix with the same size that contains 1 if the comparison returned true and 0 else.
For example:
mat: {1,2,3;4,5,6}
mat_2 = mat > 3;//Should return {0,0,0;1,1,1}
So, you may notice that all my functions have the exact lines of code except whenever I wrote != it should be > or < or == or etc....
So, in order to remove duplication here's how I implemented them:
IntMatrix IntMatrix::operator<(int num) const {
    return filter(*this,Between(INT_MIN,num-1));
}

IntMatrix IntMatrix::operator>(int num) const {
    return filter(*this,Between(num+1,INT_MAX));
}

IntMatrix IntMatrix::operator!=(int num) const {
    return filter(*this,Between(num,num), true);
}

where Between is a functor (to replace a pointer to a function).
So, when I reviewed my code I strongly believe this is not the best solution to give to this kind of problem, since filter() won't be well understandable by other programmers.
From your experience is there anyway to improve this code?
(I know that macros would be perfect here but don't want to use them either since they are a bad practice)
Note: I am working with C++11 and want only to use standard libraries.

Make use of the standard library

This looks like a job for std::transform(), which in many other languages would be the equivalent of the map function. It works on iterators, so it would help if your class provides begin() and end() functions, or if internally you store values in a std::vector. However, the data pointer can be used as well.

Then, it's possible to write your operators like so:

#include <algorithm>

IntMatrix IntMatrix::operator<(int num) const {
    IntMatrix result(width(), height()); // assuming this creates a matrix with the same dimensions
    const auto count = width() * height();
    std::transform(data, data + count, result.data,
                   [num](int value){return value < num;});
    return result;
}

With this, there is no need to write your own filter() function.


C-style va_args replacement I inherited a lot of C code with many ellipsis (variadic) functions.

I have a lots of API with the following signature:

void getXY(int foo, ...) // many parameters

and this is used in this way as usual:

getXY(1, "sizex", 12, "sizey", 24, 0);

Now I started to think about how I can replace it with a typesafe C++ API, and I came up with the following:

#include <string>
#include <iostream>
#include <cstdarg>
#include <vector>

class Test
{
  struct GetParam
    {
      std::string name;
      int id;
    };
  struct Attr
    {
      Attr(Test& test) : test(test)
        {
          std::cout << "Attr()" << std::endl;
          test.get_params.clear();
        }
      ~Attr()
        {
          std::cout << "~Attr()" << std::endl;
          test.end_get();
        }
      Attr& add(const std::string& name, int id)
        {
          GetParam param = {name, id};
          test.get_params.push_back(param);
          return *this;
        }
      Test& test;
    };
  void end_get()
    {
      for (auto get_param : get_params)
        std::cout << "name:" << get_param.name << ", id:" << get_param.id << std::endl;
    }
  std::vector<GetParam> get_params;

public:
  // old code
  void get1(int foo, ...)
    {
      va_list args;
      va_start(args, foo);
      const char* name = va_arg(args, const char *);
      for (; name != NULL; name = va_arg(args, const char *))
        {
          int id = va_arg(args, int);
          std::cout << "name:" << name << ", id:" << id << std::endl;
        }
      va_end(args);
    }

  // new code plan
  Attr get2(int foo)
    {
      Attr attr(*this);
      return attr;
    }
private:
};

int main()
{
  Test test;
  test.get1(1, "sizex", 12, "sizey", 24, 0); // old style
  test.get2(1).add("sizex", 12).add("sizey", 24); // new style
  return 0;
}

What do you think? Is there a simpler solution? How can I improve this?

If the arguments are that simple, what's wrong with a locally defined array?

Test t;
const Test args[] = { {"sizex", 12}, {"sizey", 24} };
t.get(1, args, sizeof(args)/sizeof(*args));


Create a cache object in javascript My intent is to use this object to cache data from web responses. The cache object should only be valid for the current data.

var myCache = {

        Cache: { Date: '', Data: [] },

        AddData: function(Name, Data){
            myCache.CheckCache();
            myCache.Cache.Data[Name] = Data;
        },

        GetData: function(Name){
            myCache.CheckCache();
            return myCache.Cache.Data[Name];
        },

        HasData: function(Name){
            myCache.CheckCache();
            var data = myCache.Cache.Data[Name];
            var val = true;
            if(typeof data === "undefined" || data == null || data === '') {
                val = false;
            }
            return val;
        },

        CheckCache: function(){

            var currentDate = myCache.GetCurrentDate();
            var CacheDate = myCache.Cache.Date;
            if(CacheDate != currentDate){
                console.log('Reset Cache');
                myCache.Cache.Data = [];
                myCache.Cache.Date = currentDate;
            }
        },

        GetCurrentDate: function(){
            var today = new Date();
            var dd = today.getDate();
            var mm = today.getMonth() + 1;
            var yyyy = today.getFullYear();
            if(dd < 10) {
                dd = '0' + dd;
            }
            if(mm < 10) {
                mm = '0' + mm;
            }
            var returnDate = yyyy + '-' + mm + '-' + dd;
            return returnDate;
        }
    };

When the main page loads I call

myCache.CheckCache();

When I get data in the application I use the cache object as

   var data;
   if(myCache.HasData('UserData')){
       data = myCache.GetData('UserData');
   } else{
       data = myWebApi.Get(Url);
   }

Is there a better way of creating a cache object? Or any suggestions of improvment

There are two hard things in computer science: cache invalidation, naming things, and off-by-one errors.

  Phil Karlton

I'd suggest leaving the job to the browser and to the experts that make it. Additionally, peppering your code with a lot of if-else just to use your cache is too much unnecessary effort really.

Now if you insist going in this route...

First, your function names. They're like from C# land. JavaScript usually uses camel case, with the very first letter in lower case.
There's no sense storing date in a formatted way. You can store it in milliseconds instead. You can get the current timestamp using Date.now().
Your cache tracks time globally, not on a per-item basis. This means there will be a period after wiping that you'll be loading stuff fresh instead of the cache.
An better way to clear an array is to set its length property to 0 instead of assigning another array.
You can take advantage of localStorage to cache content more persistently than just in memory. Note that localStorage has a size limit of 5-10Mb depending on the browser and it only stores strings (objects need be serialized first).
Instead of peppering your code with a lot of if-else operations, consider making a wrapper function for data-fetching mechanisms. Sort of like a decorator.

function cacheWrapper(fn){
  var sourceId = generateIdForFn(fn);
  var isCached = MyCache.hasData(sourceId);
  return isCached ? MyCache.getData(sourceId) : fn();
}

function() originalGetter{ return someRemoteApi.getData(); }

var cachingGetter = cacheWrapper(originalGetter);

var potentiallyCachedData = cachingGetter();

undefined == null. It's always best to use strict comparison to avoid ambiguity.


Populate DataTable with API results I am querying an API and populating a C# Datatable with the results, then using a Table Value Parameter to populate a SQL Server table with the results.  This is my solution, but it is slow and takes quite some time to process. Is there any optimization that can be done to this syntax to speed up the process? Sorry for the length of code provided, just trying to provide full C# procedure so all transparency is seen.

namespace Test
{
    class Program
    {
        public static string gg;
        public static string ff;
        public static string ee;
        public static string dd;

        static void Main(string[] args)
        {
            DataTable SQLHelperTable = GenerateDataTable();
            var response = syncClient.DownloadString(url);
            var o = JsonConvert.DeserializeObject<PrimaryObject[]>(response);

            foreach (PrimaryObject ro in o)
            {
                if (ro.dataPull != null)
                {
                    foreach (dataPull data in ro.dataPull)
                    {
                        var secondaryData = data.DP.FirstOrDefault();

                        if (secondaryData != null)
                        {
                            gg = secondaryData["gg"];
                            ff = secondaryData["ff"];
                            ee = secondaryData["ee"];
                            dd = secondaryData["dd"];
                        }

                        SQLHelperTable.Rows.Add(ro.Alpha, data.a data.b data.c data.d data.e data.f data.g data.h data.i data.j data.k data.l data.m data.n data.o data.p data.q data.r data.s data.t data.u data.v data.w data.x data.y data.z data.aa data.bb, data.cc, gg,ff,ee,dd);
                    }
                }
            }
        }
        public static DataTable GenerateDataTable()
        {
            DataTable dt = new DataTable();
            \\Generate DataTable
            return dt;
        }
    }
    public class PrimaryObject
    {
        public string Alpha { get; set; }
        public List<dataPull> dataPull { get; set; }
    }
    public class dataPull
    {
        public int a { get; set; }
        //Remaining get set statements
        public List<Dictionary<string, string>> cc { get; set; }
    }
}

You can pass all of the properties of your dataPull class in a single line which will also pass any future properties you decide to add to your object, using Reflection:

var values = data.GetType().GetProperties().Select(p => p.GetValue(data))
    .Concat(new object[] {gg, ff, ee, dd});
SQLHelperTable.Rows.Add(ro.Alpha, values);


LinkedList Implementation I have created a LinkedList in Ruby and was wondering if anyone had any input on efficiencies that I could add or deficiencies I could remove.

module LinkedList

    class List
        attr_accessor :node

        def initialize
            self.node = nil
        end

        def add(node)
            if self.node.nil?
                self.node = node
            else
                current_node = self.node
                while ! current_node.getPointer.nil? do
                    current_node = current_node.getPointer
                end
                current_node.setPointer node
            end
        end

        def get(node)
            current_node = self.node
            data_match = nil
            while !current_node.getPointer.nil? and ! data_match.nil? do
                data_match = current_node.getData if node.getData == current_node.getData
                current_node = current_node.getPointer
            end
            return current_node
        end

        def remove(node)
            previous_node = nil
            current_node = self.node
            next_node = current_node.getPointer
            while ! current_node.nil? do
                if current_node.getData == node.getData and current_node.getData == self.node.getData
                    self.node = next_node
                    return true
                end
                if current_node.getData == node.getData
                    previous_node.setPointer next_node
                    return true
                end
                previous_node = current_node
                current_node = next_node
                next_node = current_node.getPointer

            end
            return false
        end

        def print
            current_node = self.node
            while ! current_node.nil? do
                pointerData = current_node.getPointer.nil? ? nil : current_node.getPointer.getData
                puts "data=#{current_node.getData}, pointer=#{pointerData}"
                current_node = current_node.getPointer
            end
            puts
        end

    end

    class Node
        attr_accessor :data
        attr_accessor :pointer

        def initialize(data = nil, pointer = nil)
            self.data = data
            self.pointer = pointer
        end

        def getData
            return self.data
        end

        def getPointer
            return self.pointer
        end

        def setData(data)
            self.data = date
        end

        def setPointer(node)
            self.pointer = node
        end

    end

end

Some things that immediately hit me were not about the implementation of the algorithm but the look of the code, it's not idiomatic Ruby, it looks like Java.

Most Rubyists use 2 spaces instead of 4.
Why all the getters and setters? Setters are quite often a code smell, make sure you really need them (you may do but if you can draw a line under an object's period of mutability - hopefully by the end of initialization - you'll do yourself a favour).
Camel case is the Java-y bit, so no setPointer and friends. pointerData would be pointer_data in idiomatic Ruby.
You only need def pointer for the getter and def pointer= val for the setter, or to use the attr_… helpers.
… and you've used the attr_ helpers, so why is there even a setPointer at all? Just use previous_node.pointer = next_node (to give one example) and cut out the middle man. Same goes for all the others, why write a getter/setter to wrap another getter/setter?
self.node = nil in the intializer is unnecessary and if you were going to do it you'd probably use @node = nil.
self.node = node, again, why not access the instance variable directly if you know there's no extra processing going on? @node = node is more idiomatic.
No need to write return unless you want an early return, everything's an expression so the last expression in a method will become its return value.
pointer_data = current_node.pointer.nil? ? nil : current_node.pointer.data - why use a ternary where an && or || will do? pointer_data = current_node.pointer && current_node.pointer.data is terser.
To be even more up to date and terse you could use the new safe-navigation operator, &., e.g. pointer_data = current_node.pointer&.data
You're returning true and false from methods that are changing state. Return either self or the particular state changed (take a look at what happens when you do [] << 1 or [1,2,3].delete 2 in a REPL). Methods that return true or false are usually suffixed with ? and side effects will probably surprise the consumer.

Once you've cleaned up all of that then the algorithm will be clearer. Some more thoughts on the code that are stylistic but not about Ruby per se.

Firstly, I'd allow the head node to be set in the initialize method - why make a 1 line operation into 2? Secondly, I'd be clearer in my naming, node is a bit abstract, if you're really talking about the position of a node - the head, then call it head:

def initialize head=nil # <- this makes it optional
  @head = head # <- this takes care of setting to nil anyway

while ! current_node.pointer.nil? is the same as while current_node.pointer but the latter is much easier to understand. No one likes a double negative.

def initialize(data = nil, pointer = nil) Optional arguments are nice until you start using them in multiple positions, something like def initialize(data = nil, pointer: nil) is better, or def initialize(data: nil, pointer: nil), it depends on what you think is most likely to be used / easier on the consumer of the library - think about the interface all the time.

This brings me to my last point - where are the examples for running it? Where are the tests? I've pasted my own version using the advice I've given but does it work? I don't know. Any refactoring can (will!) break things which is why you need examples/tests to fall back on and check your work. It's also where you I'd advise you should start in future. If you don't know how you want the code to be called then you won't do a good job writing it.

I hope that helps.

module LinkedList

  class List
    attr_accessor :node

    def initialize head=nil
      @head = head
    end

    def add(node)
      if @head.nil?
        @head = node
      else
        current_node = @head
        while current_node.pointer do
          current_node = current_node.pointer
        end
        current_node.pointer = node
      end
      self
    end

    def get(node)
      current_node = @head
      data_match = nil
      while current_node.pointer and data_match do
        data_match = current_node.data if node.data == current_node.data
        current_node = current_node.pointer
      end
      current_node
    end

    def remove(node)
      previous_node = nil
      current_node = @head
      next_node = current_node.pointer
      while current_node do
        if current_node.data == node.data and current_node.data == @head.data
          @head = next_node
          return node
        end
        if current_node.data == node.data
          # This looks like it will fail as previous_node is nil afaics
          previous_node.pointer = next_node
          return node
        end
        previous_node = current_node
        current_node = next_node
        next_node = current_node.pointer
      end
      nil
    end

    def print
      current_node = @head
      while current_node do
        pointer_data = current_node.pointer&.data
        puts "data=#{current_node.data}, pointer=#{pointer_data}"
        current_node = current_node.pointer
      end
      puts # puts *what*?
    end

  end

  class Node
    attr_accessor :data
    attr_accessor :pointer

    def initialize(data: nil, pointer: nil)
      @data = data
      @pointer = pointer
    end
  end

end


Finding the elements with the most neighbors I have a sample list which contains nodes and their neighbors, if they have any. The task is to find the node which have the highest number of neighbors.

neigh = [['A'],['A','B'],['A','C'],['B','D'],['C','A']]

The input is undirected. Meaning that ['A', 'B'] are neighbors. While ['A'] doesn't have any neighbors since it has no other value within its own list. If a sublist has 2 elements, then they are neighbors. Otherwise they are not neighbors.

My solution is below, it has a lot of if-else statements. Is there a more elegant way to solve this?

def find_neighbors(neigh):
    d = {}
    for i in neigh:
        if i:
                if len(i)==2:
                    if i[0] in d.keys():
                        d[i[0]] +=1.0
                    else:
                        d[i[0]] = 1.0

                    if i[1] in d.keys():
                        d[i[1]] +=1.0
                    else:
                        d[i[1]] = 1.0

    max = 0
    for i,j in d.items():
        if j > max:
            max = j
    return [i for i, j in d.items() if j == max]

find_neighbors(neigh)

This checks if there is a neighbor of an element and if so, adds it to the count.

import operator
def find_max_letter_in_array(neighborhood):
  all_neighbors_with_count = {}
  for neighbor_set in [pair_of_neighbors for pair_of_neighbors in neighborhood if len(pair_of_neighbors) > 1]:
    for neighbor in neighbor_set:
      all_neighbors_with_count.update({neighbor[0]: 1 if neighbor[0] not in all_neighbors_with_count else all_neighbors_with_count[neighbor[0]]+ 1})

  #return all_neighbors_with_count
  return max(all_neighbors_with_count.items(), key=operator.itemgetter(1))[0]

neigh = [['A'],['A','B'],['A','C'],['B','D'],['C','A']]
print(find_max_letter_in_array(neigh))

output:
#{'A': 3, 'B': 2, 'C': 2, 'D': 1}
A

You can loop over lists with conditions in them so that you only iterate over items you actually need

for each_element in [element for element in list if len(element) > 1]:

Another important part is, you can use dict.update to do both insert and update.
You can use the if statement while doing the assignment of a value  as well

x = y if something_is_true else

At the end, using the operator.max, you can simply your code to get the max element key from dictionary based on all keys' values.

Hope it helps in how you can compress your code.


Doge meme generator in Racket I wrote a doge meme generator in racket some time ago, mainly as a joke, and I would like some criticism about the general style and what could be improved.

Here is how the program output looks :

plainDoge.jpg refers to a plain image for the doge meme (700 * 525)

#lang racket
(require racket/gui
         racket/draw)

;keywords definitions
;TODO random capitalisation for first letter
(define doge-keywords
  '("such" "much" "so" "very"))
(define wow
  "Wow")

(define user-keywords
  '())
(define colors
  '("red" "magenta" "blue" "green" "yellow" "cyan" "white"))

(define (random-member my-list)
  ;gives a random member from a list
  (car
   (shuffle my-list)))

(define (make-sentence keyword)
  ;make a sentence
  (string-append
   (random-member doge-keywords)
   " "
   keyword))

;TODO add max count
(define (make-sentences my-list)
  ;make a list of sentences
  (if (null? my-list)
      ;return a list containing this message
      '()
      ;else
      (cons wow
            (map make-sentence
                 (shuffle my-list)))))

;frame
(define (create-parent-frame)
  (new frame%
       [label "racket-doge"]
       [width 700]
       [height 600]
       [style (list 'no-resize-border)]))

;canvas
(define (main-canvas parent-frame to-draw)
  (new canvas% [parent parent-frame]
       [min-height 500]
       [paint-callback
        (lambda (canvas dc)
          (send dc draw-bitmap to-draw 0 0))]))

;horiz panel
(define (create-horiz-panel parent-frame)
  (new horizontal-panel% [parent parent-frame]))

;font management
(define (set-font my-dc)
  (send my-dc set-font
        (make-font #:size (+ 20 (random 5))
                   #:family 'decorative
                   #:face "Comic Sans MS"
                   #:weight 'bold)))

;draw on doge
(define (draw-text-with-keywords kwd my-dc)
  ;draw the text on my-dc
  (for-each
   (lambda (kw)
     (set-font my-dc)
     (send my-dc set-text-foreground (random-member colors))
     (send my-dc draw-text kw (random 700) (random 500)))
   (make-sentences kwd)))

;text edit
(define (create-user-entries horiz-panel)
  (new text-field%
       ;[enabled #f]
       [label "user-keywords :"]
       [init-value (string-join user-keywords ", ")]
       [parent horiz-panel]))

(define (parse-user-input user-entries)
  (string-split (send user-entries get-value) ", "))

;buttons
(define (gen-again-but horiz-panel user-entries my-dc p-frame)
  (new button% [label "Wow !"]
       ;generate again
       [parent horiz-panel]
       ;[enabled #f]
       [callback
        (lambda (b e)
          (send my-dc draw-bitmap (read-bitmap "plainDoge.jpg") 0 0)
          (draw-text-with-keywords
           (parse-user-input user-entries) my-dc)
          (send p-frame refresh))]))

(define (save-but doge horiz-panel)
  (new button% [label "Save"]
       ;Save the bitmap to a file
       [parent horiz-panel]
       [callback
        (lambda (b e)
          (send doge save-file
                "./generated_doge.png"
                'png))]))

;let's make the GUI and run the whole program
(define (make-gui)
  (let
      ([p-frame (create-parent-frame)])
    (let
        ([doge (read-bitmap "plainDoge.jpg")])
      (main-canvas p-frame doge)
      (let
          ([horiz-panel (create-horiz-panel p-frame)])
        ;let's create a drawing context and draw on it
        (let
            ([my-dc (new bitmap-dc% [bitmap doge])])
          (let
              ([user-entries (create-user-entries horiz-panel)])
            (draw-text-with-keywords
             (parse-user-input user-entries) my-dc)
            (gen-again-but horiz-panel user-entries my-dc p-frame)
            (save-but doge horiz-panel)))))
    (send p-frame show #t)))

(make-gui)

There's lots of indentation that should be on a single line instead.
Including most of the lets.  There's also no reason to keep nesting
lets when you can just add additional definitions in a single let.
E.g. for make-gui it's entirely feasible just to have a single let
instead of four.

The save-but function doesn't need "./" in there, that's already
what will happen there's no additional path specified.

The bitmap should be only read once from disk; the additional read when
pressing the button is not necessary.  Hard coding the 700x500
dimensions into the code is also something that could be fixed by
querying the image (or the thing it's drawing on) for its size instead,
making it way more flexible in the process.  You should probably also
add a way to specify the output (and input) filename while you're at it.

random-member is not efficient, see
Rosetta Code,
i.e. it should be something like (list-ref my-list (random (length my-list))) instead, while not shuffling the entire list.

Otherwise I don't see too much to complain about; the variable names
could be more descriptive (save-but) and less truncated (horiz),
also the overuse of my- is a bit annoying (by which I mean that the
prefix doesn't provide the reader with any additional information).


Ranking and storing scores to a database I am developing a small website with PHP and am trying to receive user exam scores from the database. I rank it based on the highest score and then store the ranking to the database based on the user_id returned from the database with the score.

I have finished the code but it's too long. I need optimizations and reviews.

//=============init connection
$dbc = new mysqli('localhost','root','','test');

//==================database========================
CREATE TABLE IF NOT EXISTS `users` (
`uid` int(11) NOT NULL AUTO_INCREMENT,
`user` varchar(60) NOT NULL,
`score` int(11) NOT NULL,
`subposition` int(11) DEFAULT NULL,
PRIMARY KEY (`uid`)
) ENGINE=InnoDB  DEFAULT CHARSET=latin1 AUTO_INCREMENT=6 ;

//=========================functions=======================
function gradeScoreFormula($score1, $score2)
{
if ($score1 > $score2)
{
    return true;
}else{
    return false;
}
}

function gradeScore(array $values,$functionName, $dbc)
{
$newArr = array();
$dbc->autocommit(false);

uasort($values, $functionName);
$size = count($values);

foreach ($values as $k => $v)
{
    $newArr[] = array($k  => $v);
}

$reversedArr = array_reverse($newArr);
$complete = 0;
for($i=0; $i < $size; $i++)
{
    //echo 'POS = '.$a = $i + 1 .' --> ';
    $position = $a = $i + 1;
    foreach($reversedArr[$i] as $k => $v)
    {
        //echo "UID = ".$k."----------> score = ".$v.'<br>';
        $q = "UPDATE USERS set subposition = $position WHERE uid = $k AND score = $v";
        $r = $dbc->query($q);
        if ($dbc->affected_rows)
        {
            $complete +=1;
        }
    }
}

if ($complete == $size)
{
    $dbc->commit();
    echo 'succesfully stored';
}else{
    $dbc->rollback();
    echo 'not stored try again';
}
$dbc->autocommit(true);
}

//===============================program start========================
$q1 = "SELECT uid, score FROM users";
$r1 = $dbc->query($q1);
$returnedVal = [];

while(list($id, $scores) = $r1->fetch_array())
{
$returnedVal[$id] = $scores;
}

print_r($returnedVal);

gradeScore($returnedVal,'gradeScoreFormula', $dbc);

Formatting

your indentation is off, making your code hard to read.
it is customary to put the opening curly bracket on the same line as the statement it belongs to (except for functions and classes).
if (cond) return true; else return false; can be written as return cond.
Your spacing is not consistent (use an IDE to fix this - and most other formatting issues - easily).

If we fix these issues, the first function for example would look like this:

function gradeScoreFormula($score1, $score2)
{
    return $score1 > $score2;
}

Naming

gradeScoreFormula is quite specific for such a generic function, yet it doesn't really tell me anything about what the function actually does. isLargerThan might be better. [Although the function seems all wrong anyways. All the sort functions expect the function to return an integer; in that case, you could simply call the function compare]

Security

You should use prepared statements, as you may otherwise be vulnerable to second order SQL injection.

Even if the score and/or uid values in the users table are not currently user input, they may be in the future.

Additionally, prepared statements may increase performance in this case, as the query would only need to be prepared once.

Misc

WHERE uid = $k AND score = $v seems odd to me. Why are you checking for the score as well? If there is a reason, I would add a comment about it. If there is no reason, I would remove that check; the primary id should be enough, as far as I can tell.
why have position and a? It doesn't seem necessary to have two variables here. For that matter, it doesn't seem necessary to have either one, you could simply use i + 1 directly in the query.
Your foreach($reversedArr[$i] as $k => $v) is confusing and unnecessary. You only have it to access the key and value of a one-entry array. If you need the key, you can use the key function.

Complexity

Your code is indeed overly complex, mainly because you perform unnecessary and complex transformations between data structures and because you are not using the correct functions.

The core of your program could be as simple as this:

function gradeScore(array $values, $dbc)
{
    arsort($values);

    $position = 0;
    $query = "UPDATE users set subposition = ? WHERE uid = ?";
    $statement = $dbc->prepare($query);
    foreach($values as $id => $score) {
        $position++;
        $statement->bind_param("ss", $position, $id);
        $result = $statement->execute();
    }
}


Laravel voting system I have written a voting class in Laravel. It uses the voting sites API to check if the user has voted today. A user is only allowed to vote once every day.

All votes reset at midnight (i.e. 12:00 AM Chicago time) and I create a cookie to not bother wasting a curl response till then, so the cookie expires when a new vote is allowed.

The API returns response codes: 2 means the user hasn't voted for them in the past 24 hours, and 3 means they have voted for them in the past 24 hours. Just think of 2 as 0 and 3 as 1 in a false/true situation.

The reason I am asking this question is I don't think I am handling cookies the best way I can. Could anyone advise a better way to create them? I wasn't sure how to do it without a response, as Laravel says you have to send it with a response.

If the user needs to vote, they are sent to the voting page. For now I've just put a temp link as http://need2vote.com until I finish the class.

<?php

namespace App\Http\Controllers;

use App\Http\Controllers\Controller;
use Auth;
use Redirect;
use Illuminate\Support\Facades\Route;
use Cookie;
use Illuminate\Http\Request;
use Config;

class VotingController extends Controller
{
    protected $pageUsername;
    protected $requestTimeout;
    protected $usingCloudflare;
    protected $apiUrl;

    public function __construct()
    {
        $this->pageUsername = Config::get('voting.username');
        $this->requestTimeout = Config::get('voting.timeout');
        $this->usingCloudflare = true;
        $this->apiUrl = Config::get('voting.api_url');
    }

    public function checkVote(Request $request)
    {
        if (Config::get('voting.enabled') == false) {
            return;
        }

        if ($this->isVoteCookieSet()) {
            return;
        }

        $urlRequest = $this->apiUrl . 'user=' . $this->pageUsername . '&ip=' . $request->ip();

        $result = $this->makeCurlRequest($urlRequest, $this->requestTimeout);

        if ($result == 3) {
            $this->redirectToVote();
        }
        else {
            $this->setVoteCookie();
        }
    }

    private function makeCurlRequest($url, $timeout)
    {
        if (function_exists('curl_version'))
        {
            $curl = curl_init();

            curl_setopt($curl, CURLOPT_URL, $url);
            curl_setopt($curl, CURLOPT_RETURNTRANSFER, 1);
            curl_setopt($curl, CURLOPT_SSL_VERIFYPEER, 0);
            curl_setopt($curl, CURLOPT_SSL_VERIFYHOST, 0);
            curl_setopt($curl, CURLOPT_CONNECTTIMEOUT, 10);
            curl_setopt($curl, CURLOPT_TIMEOUT, $timeout);

            $requestData = curl_exec($curl);

            curl_close($curl);
        }
        else
        {
            $requestData = stream_context_create(array('http' => array('timeout' => $timeout)));
            return file_get_contents($url, 0, $requestData);
        }

        return $requestData;
    }

    public function redirectToVote() {
        header('Location: http://needtovote.com');
        exit();
    }

    private function setVoteCookie() {
        $rankingsResetTime = $this->getVoteResetTime();
        setcookie('vote_timestamp', $rankingsResetTime, $rankingsResetTime);
    }

    private function isVoteCookieSet() {
        if (isset($_COOKIE['voting_timestamp'])) {
            if ($_COOKIE['voting_timestamp'] == $this->getVoteResetTime()) {
                return true;
            }
            else{
                setcookie('voting_timestamp', '');
                return false;
            }
        }

        return false;
    }

    private function getVoteResetTime() {
        $serverDefaultTime = date_default_timezone_get();
        date_default_timezone_set('America/Chicago');

        $rankingsResetTime = mktime(0, 0, 0, date('n'), date('j') + 1);
        date_default_timezone_set($serverDefaultTime);

        return $rankingsResetTime;
    }
}

The Question

Could anyone advise a better way to create [cookies]?

Like your code already uses, setcookie() is the traditional way of setting cookies. And yes, since the cookie data is sent in a header, there must be a response, otherwise the browser/user won't really be able to receive the cookie.
General Feedback
Passing instance/member variables to method
Why pass $this->timeout to makeCurlRequest from the checkVote method? If the method was called outside this class (and the scope of the method changed to  protected for sub-classes or public for anywhere else in the code) then it might make sense to accept that parameter.

$result = $this->makeCurlRequest($urlRequest, $this->requestTimeout);

Since that method is not static, it could simply refer to $this->requestTimeout instead of accepting the parameter for it.
Misleading Method Name
The method makeCurlRequest appears to check if the curl function curl_version() exists and then either makes a cURL request or utilizes file_get_contents(). Thus the method might not always make a cURL request and a better name might be makeRequest. The code in the block when the curl_version() function does exist could be moved to a new method called makeCurlRequest.
return from makeCurlRequest()
The method makeCurlRequest has two return statements. One is in the else block, and the other is at the end of the method - i.e. return $requestData;. That variable,$requestData, appears to be the response from the API when the curl_version() function exists (and thus the cURL request is made), yet in the else block, $requestData is assigned a resource from stream_context_create(). While this code likely functions correctly, it could be confusing to a teammate who had to update it. A more appropriate name for the return value from the call to stream_context_create() might be something like $streamContext. Then it might be simpler to either assign the return value from the call to file_get_contents() to $requestData and utilize the return of that variable at the end of the method, or when making the curl request, return the response at the end of that block.
Redundant return false
In isVoteCookieSet() There is an else block to the nested if statement that contains return false;. That could be removed, since the last line of the method does the same thing.
Constants for Response codes
It would be wise to define (class) constants for the response codes, like the ones below. While there currently appears to only be one place in your code where that value appears, there may arise a need to have it appear in other logic and thus it would be useful to reuse the constant(s). Then if the value would ever need to be updated (e.g. if the API ever changes) then it can be updated in your code in one spot.
const RESPONSE_VOTE_PLACED_IN_PAST_24_HOURS = 3;
const RESPONSE_VOTE_NOT_PLACED_IN_PAST_24_HOURS = 2;

That way, the code lines like below:

if ($result == 3) {

Can be updated like this:
if ($result == self::RESPONSE_VOTE_PLACED_IN_PAST_24_HOURS) {

Which would hopefully allow somebody else reading your code to have a better idea of what that logic means. Though if those names are too long, feel free to shorten them.


Validate whether a ransom note can be made from a dictionary of words For simplicity, it is checking whether the characters in a string (that needs to be created) are present in a larger string (dictionary). Would a character array of the letters make sense here, or would it be better with a Map<Character, Integer>?

public class RansomNoteProblem {
public static void main (String args[]){

    String blob = "thisisanothwllowllqend";
    String note = "thisisanotex";

    System.out.println(validateLetterCreation(blob,note));
}

public static boolean validateLetterCreation(String dict, String note){
    int charArray[] = new int[256];
    for(int i=0;i<dict.length();i++){
        int charValue = (int)dict.charAt(i);
        charArray[charValue]++;
    }

    for(int i=0;i<note.length();i++){
        int charValue = note.charAt(i);
        if(charArray[charValue]<=0){
            return false;
        }
        else {
            charArray[charValue]--;
        }
    }
    return true;

}
}

Algorithm

You build an array with the counts of letters in the newspaper. Then you iterate over the letters of the ransom note, and short circuit when a letter is missing (count goes below zero).

Can this be done the other way around? That is, build an array of counts from the note, then iterate over the newspaper and short circuit when the letter is completed. Yes that would work too, but using a map of counts instead of an array, deleting letters that were completed, and an empty map marking the ransom note competed.

So which way is better? Consider these questions:

Which tends to be bigger: the ransom note or the newspaper? To complete the task of making the ransom note, which one must be inevitably finite, and which one may be infinite, the ransom note or the newspaper?
What if the newspaper contains billions of letters?
What if you receive the available letters (of the newspaper) as a (possibly infinite) stream of characters?
How would you make a ransom note in real life? Cut out letter by letter from a newspaper and build neat little stacks, ready for the ransom note? Probably not ;-)

Naming

The name validateSomething sounds like it might throw an exception is invalid, or mutate state (return void). More common prefixes for boolean methods are "is", "has" and "can". In this example, instead of validateLetterCreation, I would recommend canCreateRansomNote.

The name charArray is misleading. A reader might think it's an array of characters. When it's actually counts of letters, so I would call it just like that: countsOfLetters.

Testing

One example case with a sample newspaper and a sample ransom note executed in a main method is far from enough to verify the correctness of such program. Consider looking into a proper unit testing framework, such as JUnit.


Basic arithmetic calculator The calculator supports the basic operators (Add,Substract,Divide,Multiply).
At the moment,

It only works with positive integers
I do not validate my user input
My division rounds up if it isn't an integer

This is a work in progress. It is my first Python application so I want to get feedback before I keep on working on it.
Here is an example of how my calculator parses/processes an equation :

Given 3+3+9/3-4*2

Computing multiplications : 3+3+9/3-8 (Note that 4*2 is now 8)

Then divisions : 3+3+3-8 (9/3 = 3)

Then additions : 9-8 (3+3+3 = 9)

Finally substractions : 1 (9-8 = 1)

import operator

priorizedOperators = operator.getPriorizedOperators()

def calculate(args):
    return _calculate(args,0)

def _calculate(equation, operatorArrayIndex):

    if len(priorizedOperators) == operatorArrayIndex:
        return equation;

    arithmeticOperator = priorizedOperators[operatorArrayIndex]
    copiedEquation = equation

    try:
        operatorLocation = copiedEquation.index(arithmeticOperator.getStringSign())

        while True:
            leftBound = _findLeftBound(equation,operatorLocation)
            rightBound = _findRightBound(equation,operatorLocation)

            leftValue = int(copiedEquation[leftBound:operatorLocation])
            rightValue = int(copiedEquation[operatorLocation+1:rightBound])

            temp = arithmeticOperator.operate(leftValue,rightValue)

            #Replaces the part of the equation we just computed by the result
            copiedEquation = copiedEquation[:leftBound] + str(temp) + copiedEquation[rightBound:]

            #This will throw an exception if index doesn't find the operator, which ends the while loop
            operatorLocation = copiedEquation.index(arithmeticOperator.getStringSign())
    finally:
        return _calculate(copiedEquation, operatorArrayIndex + 1)

def _findLeftBound(equation ,operatorIndex):
    for leftBoundSearchIndex in reversed(range(0,operatorIndex)):
        if not _isNumber(equation[leftBoundSearchIndex]):
            return leftBoundSearchIndex + 1

    return 0

def _findRightBound(equation, operatorIndex):
    for rightBoundSearchIndex in range(operatorIndex+1, len(equation)):
        if not _isNumber(equation[rightBoundSearchIndex]):
            return rightBoundSearchIndex

    return len(equation)

def _isNumber(char):
    return ord("0") <= ord(char) <= ord("9")

def main():
    equation = input("Enter equation : ")
    print(calculate(equation))

if __name__ == "__main__": main()

Content of the operator.py
class PlusOperator():
    def getStringSign(self):
        return "+"

    def operate(self,left,right):
        return left + right

class MinusOperator():
    def getStringSign(self):
        return "-"

    def operate(self,left,right):
        return left - right

class MultiplyOperator():
    def getStringSign(self):
        return "*"

    def operate(self,left,right):
        return left * right

class DivideOperator():
    def getStringSign(self):
        return "/"

    def operate(self,left,right):
        return left // right

def getPriorizedOperators():
    return [MultiplyOperator(),DivideOperator(),PlusOperator(),MinusOperator()]

I'm looking to know if my python is okay and if my algorithm makes sense.

A few quick comments:

Read PEP 8, the Python style guide. In particular, Python uses lowercase_with_underscore variable names, and put commas after spaces.
Add some docstrings to your functions, so that it’s clear what purpose they serve in the context of your calculator. This will become useful as you extend the calculator, and you want to change existing function: you may remember what each function is for at the moment, but you may not in six months time.
Add more comments. Tell me why you’re doing certain operations. Anybody can read the code and see what it’s doing, but why is it doing it?

For example: after some inspection, I see that you iterate through prioritizedOperators to apply each operation in order (multiplication and division, then subtraction and addition).
You have no error handling, which means I can enter something silly like 1/0 and get a ZeroDivisionError. You also claim that it only works for positive integers, but the simple examples I tried with negative integers seemed to work as well.

If you only want it to work with positive integers, then you should spit out any input that isn’t positive.
You claim that

  My division rounds up if it isn't an integer

but this doesn’t seem to be true. For example, if I enter 2/3 then I get output 0.
There is no need for getPriorizedOperators to be a function. Just declare the list as a variable in operator.py, then import it in calculator.py. For example:

operator.py:

prioritized_operators =  [
    multiply_operator(),
    divide_operator(),
    plus_operator(),
    minus_operator()
]

calculator.py:

from operator import prioritized_operators

I would rename your _isNumber() function. It only checks if a single character is a numeral, but the name implies I might be able to pass in arbitrary-length strings. Perhaps _is_digit() would be better.

You can tidy up the function logic a little. Rather than calling out to ord(), you can also compare strings directly for the same result. For example:

>>> "0" < "5" < "9"
True
>>> "0" < "x" < "9"
False

Or you could just use the builtin function isdigit(), and do away with your function entirely:

>>> "5".isdigit()
True
>>> "x".isdigit()
False

(Whether you use the latter depends on how much you want to reinvent the wheel.)
Having two functions called calculate() and _calculate() is prone for confusion. Rename one or both of them to have a more specific name.


Does this nodeunit test conform to best practices? I have inherited a node-js project, and I am completely new to it. Fortunately, it is already covered by unit tests.

The class under test, FmsRescuers, has the responsibility of sending HTTP requests to a remote server.

My first refactoring was to extract the HTTP behavior into the HttpRequest class, to allow testing that an operation requests the correct url with the correct data, without actually performing the http request. The HttpRequest class did not previously exist.

I am looking for feedback on the unit test from a perspective of best practices. In particular, how do I best avoid the duplication of calling the checkIn function through the different tests?

require('../server/extensions');
var FmsRescuers = require('../server/fms_rescuers');
var Rescuer = require('./stubs/helper').Rescuer;

var requestedPath;
var requestedStationNumber;
var requestedRescuerNumber;
var httpRequest = {
    post: function(path, requestData, callback) {
              requestedPath = path;
              requestedStationNumber = requestData.stationNumber;
              requestedRescuerNumber = requestData.rescuerNumber;
              callback('', 200);
          }
};
var rescuers = new FmsRescuers(httpRequest);
var rescuer = new Rescuer(42, 'Name', new Date().addHours(-2).toTimeZonedString(), new Date().addHours(-1).toTimeZonedString());

exports.testCheckIn = {
    setUp: function(callback) {
               requestedPath = undefined;
               requestedStationNumber = undefined;
               requestedRescuerNumber = undefined;
               callback();
           },
    'check in requests correct path' : function(test) {
        rescuers.checkIn(rescuer, 123, 3, function(result, status) {
            test.equal(requestedPath, '/attendance/checkin');
            test.done();
        });
    },

    'check in sends correct station number' : function(test) {
        rescuers.checkIn(rescuer, 123, 3, function(result, status) {
            test.equal(requestedStationNumber, 123);
            test.done();
        });
    },

    'check in sends correct rescuer number' : function(test) {
        rescuers.checkIn(rescuer, 123, 3, function(result, status) {
            test.equal(requestedRescuerNumber, 42);
            test.done();
        });
    }
};

You could use sinon.js to have the power of spies, stubs and mocks. http://sinonjs.org/

That way you could create a stub of httpRequest.

Look for stub.yield([arg1, arg2, ...]).

Edit I added a code example. I did not fully tested the code, but it's how it would look like.

var sinon = require('sinon');
require('../server/extensions');
var FmsRescuers = require('../server/fms_rescuers');
var Rescuer = require('./stubs/helper').Rescuer;

var mockedHttpRequest;
var rescuers;
var rescuer;

exports.testCheckIn = {
    setUp: function(done) {
        var httpRequest { post: function () {} };
        mockedHttpRequest = sinon.mock(httpRequest);
        rescuers = new FmsRescuers(mockedHttpRequest);
        rescuer = new Rescuer(42, 'Name', new Date().addHours(-2).toTimeZonedString(), new Date().addHours(-1).toTimeZonedString());

    },

    'check in requests correct path' : function(test) {
        var expectedPath = '/attendance/checkin'
        mockedHttpRequest
                .expects('post')
                .once()
                .withArgs(expectedPath)
                .yields('', 200);

        rescuers.checkIn(rescuer, 123, 3, function(result, status) {
            mockedHttpRequest.verify();
            test.equal(result, '');
            test.equal(status, 200);
            test.done();
        });
    },
};


More Efficient Javascript indexOf query There is an e-commerce site that I'm working on that has thousands of pages in it (mostly product pages). I've got some jQuery that I'm running on about 1300 of these products. Only specific products need the code to run so what I've done is set up the product codes (which display in the URL of the product) in a JSON data sheet and I'm looping through the JSON data like this:

$(document).ready(function(){

$.getJSON('js/round.js', function(data) {

    var location_var = "location.pathname.indexOf('/product-p/main-product.htm')"

    for(i=0; i<data.records.length-1; i++){
        var location_var = location_var + " || location.pathname.indexOf('" + data.records[i].productcode + "')";
    }

    if (location_var != -1){
        //DO SOMETHING TO THESE SPECIFIC PAGES
    }

});
});

Now this functions but obviously it has to handle the 1300 hundred rows, check and then run my code. This causes a slight lag and unfortunately that lag will run on all of the pages because it has to determine if the page is correct for every page that it goes through.

This is the only way I could think of doing this (my Javascript is a bit limited at this point) so I'm curious if anybody knows of a way to do this more efficiently and to make the query faster.

Thanks for your help!

What exactly do you have in the javascript (json) file?  Can you get the product code from anywhere other than the the path?

Here is what I suggest:

Just include the javascript file.  Don't bother pulling it via JSON.  Doing so just adds overhead you don't need since you are checking this file on every page anyway.
Within the javascript file store the product codes in an array.

var productCodes = ['code1', 'code2', 'etc...']

Put the test in a separate function.

function pageNeedsProcessing() {
    // add any special rules here to avoid checks when not needed.
    if (location.pathname.indexOf('/product-p/main-product.htm')) {
        return true;
    }

    // do whatever you need to get the product code.  This may require parsing the path.
    var prodCode = ???;

    // if the product code is in array (established in #2) then return true.
    return $.inArray(prodCode, productCodes)
}

Add your code to be processed.

if (pageNeedsProcessing()) {
      //DO SOMETHING TO THESE SPECIFIC PAGES
}

One caveat here is that doing it exactly like this will introduce some variables into global scope.  But there are ways around that.


Vehicle, Components and Car relationship I am writing Vehicle - Components - Car relationship.
I have to separate cars that use the diesel, petrol and electric engines because each of them produce their own results (but methods of each are the same).

For example, if car is using diesel engine (which obviously should have only diesel exhaust system) then it produces higher pollution than electric car's engine.

It is very important that after I could set the engine, for example my Car has diesel engine and now I want to use electric instead because of lower pollution.

Right now, I have an abstract class Vehicle. Vehicle has-a components such as Engine and Tires. Car is-a (extends) Vehicle.

Vehicle.class

public abstract class Vehicle {
    private Engine engine;
    private Tire tires;

    public Engine getEngine() {
        return engine;
    }

    public void setEngine(Engine engine) {
        this.engine = engine;
    }

    public Tire getTires() {
        return tires;
    }

    public void setTires(Tire tires) {
        this.tires = tires;
    }

    public MotorVehicle() {
        super();
    }

    public Vehicle(Engine engine, Tire tires) {
        this.engine = engine;
        this.tires = tires;
    }
}

Car.class

public class Car extends Vehicle {
    private Engine engine;
    private Tire tires;
    private ExhaustSystem system;

    public Car() {
        super();
    }

    public Car(Engine engine, Tire tires) {
        super(engine, tires);
    }
}

Where Engine.class is an interface

public interface Engine {
    String getType();
    ExhaustSystem getExhaustSystem();
}

and, for example, DieselEngine.class implements it

public class DieselEngine implements Engine {
    private String type;
    private DieselExhaustSystem system;

    public DieselEngine(String type, DieselExhaustSystem system) {
        this.type = type;
        this.system = system;
    }

    @Override
    public String getType() {
        return type;
    }
    @Override
    public ExhaustSystem getExhaustSystem() {
        return system;
    }
}

Similar to Engine - ExhaustSystem.class is an interface

public interface ExhaustSystem {
    double getGasExits();
}

and DieselExhaustSystem.class implements it.

public class DieselExhaustSystem implements ExhaustSystem {

    /**
     *
     * @return CO2 emission units.
     */
    @Override
    public double getGasExits() {
        return 3;
    }
}

I also will have Truck which will extend Vehicle.

After I wrote this, I have some doubts about its correctness.

The main question is:

Is relationship and classes logic correct?

If not or partially not, then:

What can be done better?
Maybe I do not understand polymorphism?
Maybe I do not understand has-a / is-a relationships?
Maybe I should use another desing pattern for my task (for example, bridge pattern)?
Perhaps it is better to make Engine abstract and other engines extend it?

Thanks for sharing your code.

OOP doesn't mean to "split up" code into random classes.

The ultimate goal of OOP is to reduce code duplication, improve readability and support reuse as well as extending the code.

Doing OOP means that you follow certain principles which are (among others):

information hiding / encapsulation
single responsibility
separation of concerns
KISS (Keep it simple (and) stupid.)
DRY (Don't repeat yourself.)
"Tell! Don't ask."
Law of demeter ("Don't talk to strangers!")

We create a new class if we need to implement new behavior not yet present nor belonging to any existing class.

In your example the class Car does not (yet) add any new behavior neither to the class Vehicle which it extends nor to the program as a whole. Therefore it is not needed (as well as your planned class Truck) and Vehicle should not be abstract. If you need to identify different vehicle types you might introduce a vehicleType property of type String...


"When to take medicine" CodeChef Challenge MEDIC I just submitted this code to solve the "When to take medicine" challenge on CodeChef:

#include <iostream>
#include <string>
#include <sstream>

int countDays(int dd, int subtract)
{
    return ((subtract - dd) / 2) + 1;
}

int main()
{
    int t {0};
    std::cin >> t;
    std::cin.ignore();
    while (t--)
    {
        std::string date {};
        std::getline(std::cin, date);
        std::stringstream ss {date};
        int yy {0}, mm {0}, dd {0};
        std::string temp {};
        std::getline(ss, temp, ':');
        yy = std::stoi(temp);
        std::getline(ss, temp, ':');
        mm = std::stoi(temp);
        std::getline(ss, temp, ':');
        dd = std::stoi(temp);
        int subtract {0};
        bool leap {false};
        if (yy % 4 == 0 && (!(yy % 100 == 0) || yy % 400 == 0))
        leap = true;
        switch (mm)
        {
        case 2:
            if (leap)
                subtract = 29;
            else
                subtract = 59;
                break;
        case 4: case 6: case 9: case 11:
            subtract = 61;
        break;
        default:
            subtract = 31;
        }
        std::cout << countDays(dd, subtract) <<'\n';
    }
}

In short, it is a program to tell how many days a guy has took his pill correctly, considering that he has to take a pill every 2 days and only takes it in odd or even days, depending on the day he started to take the pill. We provide a number of test cases in the first line and then we provide a date in a single line in the format yyyy:mm:dd. Check the link above if you're interested in reading the whole problem.

The thing is that I am not really satisfied with the way I am parsing the string separated by the char :

I also want to mention that I want to stay away from any C code like the scanf() function. It would make things a lot easier, but I don't want to use C in C++.

So, how could I redesign my string parse code to make it look nicer? I am just a newbie with C++ so all feedback / criticism is greatly appreciated.

You don't check whether the input is invalid.
Try to extract useful well-named functions, like int days(int month, int year) and bool is_leap_year(int year).
You can extract the numbers directly from std::cin, the colon will be left behind.
And the colon can thereafter be extracted like any other single character.
As an aside, using scanf() would probably be easier than juggling streams.
break should be indented like any other Statement. It does not deserve more indentation (first occurrence), nor less (second occurrence).


LeetCode 652: Find Duplicate Subtrees I'm posting my code for a LeetCode problem copied here. If you have time and would like to review, please do so. Thank you!
Problem

Given a binary tree, return all duplicate subtrees. For each kind of
duplicate subtrees, you only need to return the root node of any one
of them.
Two trees are duplicate if they have the same structure with same node
values.
Example 1:
        1
       / \
      2   3
     /   / \
    4   2   4
       /
      4

The following are two duplicate subtrees:
      2
     /
    4

and
    4

Therefore, you need to return above trees' root in the form of a list.

Code
#include <vector>
#include <unordered_map>
#include <string>

class Solution {
public:
    std::vector<TreeNode *> findDuplicateSubtrees(TreeNode *root) {
        std::unordered_map<string, std::vector<TreeNode *>> nodes_map;
        std::vector<TreeNode *> duplicates;
        this->serialize(root, nodes_map);

        for (auto iter = nodes_map.begin(); iter != nodes_map.end(); iter++) {
            if (iter->second.size() > 1) {
                duplicates.push_back(iter->second[0]);
            }
        }

        return duplicates;
    }

private:
    std::string serialize(TreeNode *node, std::unordered_map<std::string, std::vector<TreeNode *>> &nodes_map) {
        if (!node) {
            return "";
        }

        std::string serialized = "[" + serialize(node->left, nodes_map) + std::to_string(node->val) + serialize(node->right, nodes_map) + "]";
        nodes_map[serialized].push_back(node);
        return serialized;
    }

};

Reference
LeetCode has a template for answering questions. There is usually a class named Solution with one or more public functions which we are not allowed to rename. For this question, the template is:
/**
 * Definition for a binary tree node.
 * struct TreeNode {
 *     int val;
 *     TreeNode *left;
 *     TreeNode *right;
 *     TreeNode() : val(0), left(nullptr), right(nullptr) {}
 *     TreeNode(int x) : val(x), left(nullptr), right(nullptr) {}
 *     TreeNode(int x, TreeNode *left, TreeNode *right) : val(x), left(left), right(right) {}
 * };
 */
class Solution {
public:
    vector<TreeNode*> findDuplicateSubtrees(TreeNode* root) {

    }
};

Problem

Solution

Discuss

Tree Data Structure

You forgot a std::
There's a string without std:: in front of it. I guess you secretly used using namespace std and/or #include <bits/stdc++.h> before submitting the result. The rest looks fine though.
Consider using const TreeNode * everywhere
I know the public API of the LeetCode problem explicitly takes non-const pointers to TreeNode, so you shouldn't change this. But consider that your functions, quite rightly so, don't modify the TreeNodes themselves. So you could write const TreeNode * everywhere you now have TreeNode *, and it would still compile correctly. So in real production code, that is what you should do.
Don't use this-> unnecessarily
In C++, there is rarely a need to write this->. So in findDuplicateSubtrees(), you can just write serialize(root, nodes_map).
Use range-for and structured bindings where appropriate
You can replace the for-loop in findDuplicateSubtrees() using range-for and structured bindings:
for (auto &[serialization, nodes]: nodes_map) {
    if (nodes.size() > 1) {
        duplicates.push_back(nodes[0]);
    }
}

Consider using a binary serialization format
There are many ways you could serialize trees. Making human-readable strings has some advantages: it is easy to reason about, and it is nice when having to put them in textual formats like XML or JSON files. But it can be inefficient to convert values to strings. In your case, you are only using the serialization internally, so a binary format might be more efficient.
In the case of trees of integers, an obvious representation is just a vector of ints. However, a node might have only one child, and you somehow have to encode that as well. You could use a std::optional<int>, and use std::nullopt to represent an unpopulated leaf node. The serialization format would then be:
std::vector<std::optional<int>> serialization;

The serialization function would then look like:
std::vector<std::optional<int>> serialize(TreeNode *node, std::unordered_map<std::string, std::vector<TreeNode *>> &nodes_map) {
    if (!node) {
        return {std::nullopt};
    }

    auto left = serialize(node->left, nodes_map);
    auto right = serialize(nodes->right, nodes_map);
    left.append(node->val);
    left.insert(left.end(), right.begin(), right.end()); // append right
    return left;
}

For the LeetCode problem, it's not worth doing this though; the algorithmic complexity is the same, and the string representation is smaller for trees with small values. Also, you can not use a std::unordered_map anymore for nodes_map, unless you write a custom hash function. You could use a std::map though, since optionals and vectors have well-defined ordering.


String encryption function in admin section I have been using this function locally to encrypt strings for example a username.

If the user successfully logged in, place the encrypted username into a session, decrypt the username in the admin section and use for various things.

Is it a strong method and are there any security concerns with the way I'm using it?

Cipher Function:

    public function cipher($string, $method) {
        $key = pack('H*', "bcb04b7e103a0cd8b54763051cef08bc55abe029fdebae5e1d417e2ffb2a00a3");
        $iv_size = mcrypt_get_iv_size(MCRYPT_RIJNDAEL_128, MCRYPT_MODE_CBC);
        $iv = mcrypt_create_iv($iv_size, MCRYPT_RAND);

        if($method === 1) {
            $string = mcrypt_encrypt(MCRYPT_RIJNDAEL_128, $key, $string, MCRYPT_MODE_CBC, $iv);
            $string = $iv . $string;
            $encryptedString = base64_encode($string);
    return trim($encryptedString, "\0");
    }
        if($method === 2) {
            $string = base64_decode($string);
            $iv_dec = substr($string, 0, $iv_size);
            $ciphertext_dec = substr($string, $iv_size);
            $decryptedString = mcrypt_decrypt(MCRYPT_RIJNDAEL_128, $key, $ciphertext_dec, MCRYPT_MODE_CBC, $iv_dec);
    return trim($decryptedString, "\0");
        }
    }

Function Usage:

//Encrypt
$_SESSION['username'] = $backend->cipher($username, 1)

// Decrypt
echo $backend->cipher($_SESSION['username'], 2)

It looks great, however, you can improve it as follows:

Break out the function into two - encrypt and decrypt. Your functions should do the least amount of things.
return on trim - no need to set $encryptedString.
In the encrypt, no need to have the second $string when you concat $iv. Just concat on the initial mcrypt call.

In the end, we're left with three functions. One setupCipher which creates the key, iv_size and iv. One encrypt and one decrypt to encrypt and decrypt, as follows:

private function setupCipher(){
    $iv_size = mcrypt_get_iv_size(MCRYPT_RIJNDAEL_128, MCRYPT_MODE_CBC);

    return array(
        pack('H*', "bcb04b7e103a0cd8b54763051cef08bc55abe029fdebae5e1d417e2ffb2a00a3"),
        $iv_size,
        mcrypt_create_iv($iv_size, MCRYPT_RAND)
    );
}

public function decrypt($string){
    list($key, $iv_size) = $this->setupCipher();

    $string = base64_decode($string);
    $iv_dec = substr($string, 0, $iv_size);
    $ciphertext_dec = substr($string, $iv_size);
    return trim(mcrypt_decrypt(MCRYPT_RIJNDAEL_128, $key, $ciphertext_dec, MCRYPT_MODE_CBC, $iv_dec), "\0");
}

public function encrypt($string) {
    list($key, $iv_size, $iv) = $this->setupCipher();

    $string = $iv . mcrypt_encrypt(MCRYPT_RIJNDAEL_128, $key, $string, MCRYPT_MODE_CBC, $iv);
    return trim(base64_encode($string), "\0");
}

You would use them nearly identically to how you used them before, except no need to pass in the $method:

# Encrypt
$_SESSION['username'] = $backend->encrypt($username);

# Decrypt
echo $backend->decrypt($_SESSION['username']);


Fibonacci sequence methods Decided to get a little JDK 8 practice in today, so I built two methods to print out the golden ratio, one uses the Stream API the other does not. Wanted to request general feedback on how well I implemented these methods (mainly the stream generator,) considering the given task and if there is anything I could have done better.

import java.util.ArrayList;
import java.util.Arrays;
import java.util.List;
import java.util.function.Supplier;
import java.util.stream.Collectors;
import java.util.stream.Stream;

/**
 * Created on 7/5/2016.
 *
 */

public class Fibonacci {
    public static void main(String[] args) {
        System.out.printf("Fibonacci Old: %s%n", String.join(", " , Fibonacci.calculateFib(10).stream()
                .map(Object::toString)
                .collect(Collectors.toList())));

        System.out.printf("Fibonacci New: %s%n", String.join(", " , Fibonacci.generateFib()
                .limit(10)
                .map(Object::toString)
                .collect(Collectors.toList())));
    }

    private static List<Integer> calculateFib(int fibCount){
        List<Integer> fibSequence = new ArrayList<>();
        fibSequence.add(0);
        fibSequence.add(1);
        for(int i = 2; i < fibCount; i++){
            fibSequence.add(fibSequence.get(i-1) + fibSequence.get(i-2));
        }
        return fibSequence;
    }

    private static Stream<Integer> generateFib() {
        return Stream.generate(new Supplier<Integer>() {
            List<Integer> list = new ArrayList<>(Arrays.asList(0, 1));
            int i = 0;

            @Override
            public Integer get() {
                list.add(list.get(i) + list.get(i+1));
                return list.get(i++);
            }
        });
    }
}

Collectors.joining

In the two parts of your code, you are using the pattern:

String.join(", " , someStream.map(Object::toString).collect(Collectors.toList()));

to join all the String of a list and separate them with a comma. This has the disadvantage that it requires to construct the whole list in memory only to build the wanted String from it. It would be preferable to use the built-in joining(delimiter) for this task: it will collect the String directly into a String delimited by the given delimiter, without building an intermediate list:

someStream.map(Object::toString).collect(Collectors.joining(", ")));

Stream solution

Your Stream solution has 2 major flaws:

When run in parallel, it is broken. It is simple to test it: add .parallel() just before returning the Stream inside generateFib() and you will not have the correct result; you may even have an ArrayIndexOutOfBoundsException. Why? generate takes here a Supplier<Integer> that will return the next Integer in the Stream, and in parallel, get() will be invoked concurrently. Unfortunately, its code is not thread-safe, leading to unexpected results / exceptions. You might wonder why this is important: the whole Stream API has been built with the idea that running a pipeline in parallel can be done without effort, by just adding .parallel(), so it would simply be inappropriate to write code that doesn't work in parallel.
The supplier is storing itself the whole List<Integer> in memory, when in fact, it would only need the last 2 elements to generate the next one.

Using Stream.generate won't do it: it needs state (last 2 values) so we'll never be able to make a parallel-friendly get(): this method is useful to generate inifnite Stream of uncorrelated values, which isn't our case here.

In this case, we can turn to the Stream.iterate method: this method gives us the last element produced to return the next element. So we could have:

private static LongStream generateFib() {
    return Stream.iterate(new long[]{ 0, 1 }, a -> new long[] { a[1], a[0]+a[1] })
                 .mapToLong(a -> a[0]);
}

This creates a Stream of tuples where each tupes is in fact a 2 element array containing the two previous values. It is initialized to the seed of { 0, 1 } and each next value is computed by retuning { p[1], p[0]+p[1] } from an existing tuple. Finally, this Stream is mapped to a LongStream by only keeping the first element of the tuple. (Note that this uses a primitive Stream of long value. It will quickly overflow: using a BigInteger in place of a long is left as exercise for the reader).

The big advantage is that now, you can run this in parallel, and it won't break (not that it would bring any real advantage, one would have to measure for that; a solution that would probably offer better parallel performance would be implementing a custom Spliterator like done on this Stack Overflow answer).

Then

Main.generateFib().limit(10).mapToObj(Long::toString).collect(Collectors.joining(", "))

gives the same output as the initial code.


decode `IEnumerable` to string made of 1 byte characters (eg., ASCII) I'm looking for a more elegant (eg., with less many functions, eg., "copy into contiguous array then reinterpret") or more efficient way (with less complexity) to transcode a IEnumerable<uint> (~ array of 32bits int), which is a compact way to represent a byte[4] array, into a string as array of 8bits char:
public static string UintArraytoString(this IEnumerable<uint> x) =>
    string.Concat(x.Select(o =>
        new string(BitConverter.GetBytes(o).Select(Convert.ToChar).ToArray())));

I'm using .net 5.0. I'm open to use unsafe methods too. I think that one may do something like a reinterpretcast, but the IEnumerable is not necessarily contiguous.

Well, I could propose few improvements for you:

Try to use SelectMany instead of just Select;
No need to call ToArray since the string.Concat could consume IEnumerable;

public static string UintArrayToString(this IEnumerable<uint> x) =>
    string.Concat(x.SelectMany(BitConverter.GetBytes).Select(Convert.ToChar));


Setting up a database class I am wondering the best way to setup a database class, to remove repeat code by using OOP. This way if I changed how I connect to the database (switch from PDO to sqlsrv driver) I can just change it in 1 file, instead of all the files that use PDO. I've setup a class that I will show below, but I feel like it may be hard to understand if I hired a developer in the future.

Also, would it be better to have pre-built functions for returning specific data (.getFriends())? I also am going to be using mainly stored procedures, which means, should I create specific functions for doing that? Just to make it more clear to a developer? As you can see, I understand how to do all of this, I am just not sure what a good, clean way is.

<?php

$database = new Database;

class Database
{
public $database;

public function __construct()
{
    $this->database = new PDO("sqlsrv:Server=$IP;Database=$DBNAME", "$UsrName", "$Password");
}

// get rows
//Returns rows from specified query. If no bindParams, then pass empty or       undefined variable.
public function selectRows($query, $bindParams, $fetchMode)
{
    $stmt = $this->database->prepare($query);
    $stmt->execute($bindParams);
    return $stmt->fetchAll($fetchMode);
}

// insert row
public function insertRows($query, $bindParams)
{
    try {
        $stmt = $this->datab->prepare($query);
        $stmt->execute();
        return TRUE;
    } catch (PDOException $e) {
        throw new Exception($e->getMessage());
    }
}

// update row
public function updateRows($query, $bindParams)
{
    $this->insertRow($query);
}

// delete row
public function deleteRows($query, $bindParams)
{
    $this->insertRow($query);
}

}

?>

Unless I'm missing something, your selectRows and insertRows can be used for other operations as well, because you don't actually check that the queries are SELECT or INSERT respectively.  In fact you call insertRows when the consuming code calls deleteRows or updateRows so why have all these functions?

In deleteRows and updateRows, you accept an argument ($bindParams) that you don't use. Why?

  would it be better to have pre-built functions for returning specific
  data (Eg..getFriends())?

In my own work, I put such functions in the User class which can in turn call selectRows with the right parameters.  It still meets your requirement that only the Database class interacts directly with the DB engine. The Database class has no information about other objects. It just executes queries and returns results. The core of my class is similar to:

public $error; //holds text of error if a query fails
private $results; //holds results of most recent query
private $conn; //holds db connection used by all queries

//connects to the database and saves the connection in $conn property
public function __construct($host, $port, $user, $pwd, $db_name)

//disconnect from the database
public function __destruct()

//executes query after binding params. Return TRUE on success or FALSE on error
//stores results in $results property so it can be accessed by other functions
public function query($sql, array $params = null)

//executes a query and returns a row. If no sql is provided,
// returns row from $results property. Return FALSE on error
public function fetch_row($sql = null, array $params = null)

//executes a query and returns the full results table. If no sql is provided,
// returns table from $results property. Return FALSE on error
public function fetch_array($sql = null, array $params = null)

//executes a query and returns the column of interest. If no sql is provided,
// returns column from $results property. Return FALSE on error
 public function fetch_column($column, $sql = null, array $params = null)

At the start of each function, I set $error to null. If there is an error, I fill $error with the details. So the consuming code can check this variable after executing a query to check for errors:

$result = $db->query(...); // or $db->fetch_array(...)

if($error_msg = $db->error):
  //query failed. Details are in $error_msg
else:
  // query succeeded
endif;

Does that help?


Array-based queue implementation Here I have implemented a queue. Is this a correct, efficient queue implementation? what issues are there if any and what can be done to make it better?

import java.util.*;

public class Queue2{
    private int amountInQueue;
    private int[] queue;
    private int rearIndex;
    private int frontIndex;

    public Queue2(){
        amountInQueue = 0;
        queue = new int[10];
        rearIndex = 0;
        frontIndex = 0;
    }

    public Queue2(int queueCapacity){
        amountInQueue = 0;
        queue = new int[queueCapacity];
        rearIndex = 0;
        frontIndex = 0;
    }

    public void enqueue(int valueToAdd){
        if(amountInQueue == queue.length){
            throw new FullQueueException();
        }
        else if(rearIndex == queue.length -1){
            rearIndex = 0;
            queue[rearIndex] = valueToAdd;
            amountInQueue++;
        }
        else if(amountInQueue == 0){
            queue[rearIndex] = valueToAdd;
            amountInQueue++;
        }
        else{
            rearIndex++;
            queue[rearIndex] = valueToAdd;
            amountInQueue++;
        }
    }

    public int dequeue(){
        int frontValue = frontIndex;
        if(amountInQueue == 0){
            throw new EmptyQueueException();
        }
        if(frontIndex == queue.length -1){
            frontValue = queue[frontIndex];
            frontIndex = 0;
            amountInQueue--;
        }
        else{
            frontValue = queue[frontIndex];
            frontIndex++;
            amountInQueue--;
        }
        return frontValue;
    }

    public boolean isEmpty(){
        return amountInQueue == 0;
    }

    public int valueAtRear(){
        return queue[rearIndex];
    }

    public int valueAtFront(){
        return queue[frontIndex];
    }

    public int currentSize(){
      return amountInQueue;
    }

  class FullQueueException extends RuntimeException{
      public FullQueueException(){
          super("You cannot add values to a queue that is full");
      }
  }

  class EmptyQueueException extends RuntimeException{
      public EmptyQueueException(){
          super("You cannot remove values from a queue that is empty");
      }
  }
}

Don't Repeat Yourself (DRY)

    public void enqueue(int valueToAdd){
        if(amountInQueue == queue.length){
            throw new FullQueueException();
        }
        else if(rearIndex == queue.length -1){
            rearIndex = 0;
            queue[rearIndex] = valueToAdd;
            amountInQueue++;
        }
        else if(amountInQueue == 0){
            queue[rearIndex] = valueToAdd;
            amountInQueue++;
        }
        else{
            rearIndex++;
            queue[rearIndex] = valueToAdd;
            amountInQueue++;
        }
    }

There's some duplicate code here.  Consider

    public void enqueue(int valueToAdd) {
        if(amountInQueue == queue.length) {
            throw new FullQueueException();
        }

        if (rearIndex == queue.length -1) {
            rearIndex = 0;
        }
        else if (amountInQueue == 0) {
            frontIndex = 0;
            rearIndex = 0;
        }
        else{
            rearIndex++;
        }

        queue[rearIndex] = valueToAdd;
        amountInQueue++;
    }

You don't need to use an else after you throw an exception, as it will leave the function immediately.

More importantly, other than the exceptional case, all the cases have the same two lines.  So take them out of the if/else construct.  In the empty queue case, there's nothing left.  You can clear the front and rear indexes if you want.  Or you can remove that case entirely.

    public int dequeue(){
        int frontValue = frontIndex;
        if(amountInQueue == 0){
            throw new EmptyQueueException();
        }
        if(frontIndex == queue.length -1){
            frontValue = queue[frontIndex];
            frontIndex = 0;
            amountInQueue--;
        }
        else{
            frontValue = queue[frontIndex];
            frontIndex++;
            amountInQueue--;
        }
        return frontValue;
    }

Similarly, this could just be

    public int dequeue(){
        if (amountInQueue == 0) {
            throw new EmptyQueueException();
        }

        frontValue = queue[frontIndex];
        amountInQueue--;
        frontIndex++;
        if (frontIndex == queue.length) {
            frontIndex = 0;
        }

        return frontValue;
    }

You don't have to set the return value before the exceptional case, as it won't be used if the exception is thrown.

Only the modifications of frontIndex need to be in the if/else.

If you always increment, then you don't have to subtract in the comparison.  Saving one math operation in the common case and using the same number in the uncommon case.

Interfaces

There is a Queue interface.  You might consider having this code implement that interface.  That will force you to name the operations as expected.

public class Queue2 implements Queue {

As is, someone used to the other queue might have trouble finding the equivalents of add, remove, and peek.

Bug?

In your valueAtFront and valueAtRear methods, you don't verify that there are elements in the queue.  So these may return garbage.  They should either throw an exception or return something like null when the queue is empty.

Resize

It's pretty common to allow queues to resize.  Something to consider for future development.