C++ string operators I'm trying to practice my C++ on my own;
At the moment I'm trying to implement the concept of operator overloading making my own string class/function.

I think I handled this correctly; the code runs successfully.
However I'm not so sure if my code is optimally written (do I need to make more methods/params const? Do I use to much excess code?) and did I do good memory management?

Thanks for all the feedback :)

#include <iostream>
#include <vector>
#include <string.h>
#include <stdio.h>
using namespace std;
class DynString {
private:
    char *c;

public:
    ~DynString() { delete[]c; }
    DynString() : c(NULL) {}
    //second constructor
    DynString(const char* input) {
        int x = strlen(input) + 1;
        c = new char[x];
        strcpy(c, input);
    }

    //(copyconstructor) TRIO //QUATRO
    DynString(const DynString& dynstring) {
        c = new char[strlen(dynstring.c) + 1];
        strcpy(this->c, dynstring.c);
    }
    //UNO
    friend ostream& operator<< (ostream& lhs, DynString& a) {
        lhs << a.c;
        return lhs;
    }
    //DUO
    char& operator[] (int i) {
        return c[i];
    }
    //TRIO
    DynString& operator+ (DynString& y) {
        char *newArray = new char[strlen(y.c) + strlen(this->c) + 1];
        strcpy(newArray, this->c);
        strcat(newArray, y.c);
        delete[] c;
        c = new char[strlen(newArray)];
        this->c = newArray;
        return *this;
    }

    //QUATRO
    friend DynString operator+ (char* x, DynString& y) {
        DynString newString;
        newString.c = new char[strlen(x) + strlen(y.c) + 1];
        strcpy(newString.c, x);
        strcat(newString.c, y.c);

        return newString;
    }
    //QUATRO
    DynString& operator+ (char* x) {
        char *newArray = new char[strlen(x) + strlen(this->c) + 1];
        strcpy(newArray, this->c);
        strcat(newArray, x);
        delete[] c;
        c = new char[strlen(newArray)];
        this->c = newArray;

        return *this;
    }
    //TRIO // QUATRO
    DynString& operator= (DynString& x) {
        char *newArray = new char[strlen(x.c) + 1];
        strcpy(newArray, x.c);
        delete[] c;
        c = new char[strlen(newArray)];
        this->c = newArray;
        return *this;
    }

};

int main()
{
    DynString stringA("Hello");
    DynString stringB(" Worlt");
    //UNO
    cout << stringA << stringB << std::endl;

    //DUO
    stringB[5] = 'd';

    //TRIO
    DynString stringC = stringA + stringB;
    cout << stringC << std::endl;

    DynString stringD;
    //QUATRO
    stringD = "The" + stringB + " Wide Web";
    cout << stringD << std::endl;

    system("pause");

    return 0;
}

Don't include unnecessary headers

We're not using <vector> or <stdio.h>, so drop them.  We only need <ostream> rather than <iostream> for the class, and we prefer <cstring> to <string.h> to place the standard functions in their proper namespace.

Avoid importing all of namespace std

It's a bad habit that will cause problems in bigger programs.  Just don't.

Add some compiler warnings

I get some useful diagnostics, worth heeding, with g++ -Weffc++:

165851.cpp:33:39: warning: ‘DynString& DynString::operator+(DynString&)’ should return by value [-Weffc++]
     DynString& operator+ (DynString& y) {
                                       ^
165851.cpp:54:34: warning: ‘DynString& DynString::operator+(char*)’ should return by value [-Weffc++]
     DynString& operator+ (char* x) {
                                  ^
165851.cpp: In constructor ‘DynString::DynString(const char*)’:
165851.cpp:12:5: warning: ‘DynString::c’ should be initialized in the member initialization list [-Weffc++]
     DynString(const char* input) {
     ^~~~~~~~~
165851.cpp: In copy constructor ‘DynString::DynString(const DynString&)’:
165851.cpp:19:5: warning: ‘DynString::c’ should be initialized in the member initialization list [-Weffc++]
     DynString(const DynString& dynstring) {
     ^~~~~~~~~
165851.cpp: In function ‘int main()’:
165851.cpp:95:23: warning: ISO C++ forbids converting a string constant to ‘char*’ [-Wpedantic]
     stringD = "The" + stringB + " Wide Web";
                       ^~~~~~~
165851.cpp:95:33: warning: ISO C++ forbids converting a string constant to ‘char*’ [-Wpedantic]
     stringD = "The" + stringB + " Wide Web";
                                 ^~~~~~~~~~~

The last two are easily fixed by changing operator+ to accept const char* instead of char*.

The usual way of fixing the warnings on operator+ is to implement operator+ in terms of operator+= (the latter returns a reference, but the former an object).

BUG - be careful using a null pointer as sentinel

What happens if you evaluate DynString() + DynString()?  In both cases, the internal pointer points to NULL, but you happily call strlen on both - that's Undefined Behaviour.  Here's some code that actually checks:

DynString(const DynString& other)
    : c(nullptr)
{
    if (other.c) {
        c = new char[std::strlen(other.c) + 1];
        std::strcpy(c, other.c);
    }
}

But it's probably easier (safer) to always allocate memory:

DynString(const char* input)
    : c(new char[(input ? std::strlen(input) : 0) + 1];)
{
    if (input) {
        std::strcpy(c, input);
    } else {
        *c = '\0';
    }
}

And then all methods can safely assume c isn't null.

Add a move constructor

This one's easy:

DynString(DynString&& other)
    : DynString()
{
    std::swap(c, other.c);
}

Overload operator[]

Allow for both constant and mutable string objects:

char& operator[](size_t i) {
    return c[i];
}

const char& operator[](size_t i) const {
    return c[i];
}

I believe size_t is more appropriate than int as the type of the index.

Eliminate the memory leaks

The example program leaks:

==13517== Memcheck, a memory error detector
==13517== Copyright (C) 2002-2015, and GNU GPL'd, by Julian Seward et al.
==13517== Using Valgrind-3.12.0.SVN and LibVEX; rerun with -h for copyright info
==13517== Command: ./165851
==13517==
Hello Worlt
Hello World
The World Wide Web
sh: 1: pause: not found
==13517==
==13517== HEAP SUMMARY:
==13517==     in use at exit: 47 bytes in 3 blocks
==13517==   total heap usage: 12 allocs, 9 frees, 73,860 bytes allocated
==13517==
==13517== 11 bytes in 1 blocks are definitely lost in loss record 1 of 3
==13517==    at 0x4C2C93F: operator new[](unsigned long) (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==13517==    by 0x108FF3: DynString::operator+(DynString&) (165851.cpp:38)
==13517==    by 0x108C62: main (165851.cpp:90)
==13517==
==13517== 18 bytes in 1 blocks are definitely lost in loss record 2 of 3
==13517==    at 0x4C2C93F: operator new[](unsigned long) (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==13517==    by 0x10915B: DynString::operator+(char*) (165851.cpp:59)
==13517==    by 0x108CD5: main (165851.cpp:95)
==13517==
==13517== 18 bytes in 1 blocks are definitely lost in loss record 3 of 3
==13517==    at 0x4C2C93F: operator new[](unsigned long) (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==13517==    by 0x1091EF: DynString::operator=(DynString&) (165851.cpp:69)
==13517==    by 0x108CE7: main (165851.cpp:95)
==13517==
==13517== LEAK SUMMARY:
==13517==    definitely lost: 47 bytes in 3 blocks
==13517==    indirectly lost: 0 bytes in 0 blocks
==13517==      possibly lost: 0 bytes in 0 blocks
==13517==    still reachable: 0 bytes in 0 blocks
==13517==         suppressed: 0 bytes in 0 blocks
==13517==
==13517== For counts of detected and suppressed errors, rerun with: -v
==13517== ERROR SUMMARY: 3 errors from 3 contexts (suppressed: 0 from 0)

The main cause is:

    char *newArray = new char[strlen(y.c) + strlen(this->c) + 1];
    strcpy(newArray, this->c);
    strcat(newArray, y.c);
    delete[] c;
    c = new char[strlen(newArray)];
    this->c = newArray;

The error is more visible if you remove the redundant this-> from the last line:

    delete[] c;
    c = new char[strlen(newArray)];
    c = newArray;

The first allocation is obviously leaked, and that line of code should be deleted.  The same error is present in several other methods.

Implementation of operator+:

I would write it like this, starting with the += that the other versions need:

public:
    DynString& operator+=(const DynString& other)
    {
        char *newArray = new char[strlen(c) + strlen(other.c) + 1];
        std::strcpy(newArray, c);
        std::strcat(newArray, other.c);
        delete[] c;
        c =  newArray;
        return *this;
    }

    DynString operator+(const DynString& other) const
    {
        DynString newString(*this);
        return newString += other;
    }

We don't need operator+(const char*), as a C-style string will be promoted to DynString using the (non-explicit) constructor.  We do need an overload to take a C-style string as the first argument - but it doesn't need to be a friend, as it can be an ordinary non-member function instead:

DynString operator+(const char* x, const DynString& other) {
    DynString newString(x);
    return newString += other;
}

Improved version

This code is tested and does not leak:

#include <ostream>
#include <cstring>

class DynString {
private:
    char *c;

public:
    ~DynString()
    {
        delete[] c;
    }

    DynString(const char* input = nullptr)
        : c(new char[(input ? std::strlen(input) : 0) + 1])
    {
        if (input) {
            std::strcpy(c, input);
        } else {
            *c = '\0';
        }
    }

    DynString(const DynString& other)
        : c(new char[std::strlen(other.c) + 1])
    {
        std::strcpy(c, other.c);
    }

    DynString(DynString&& other)
        : DynString()
    {
        std::swap(c, other.c);
    }

    friend std::ostream& operator<<(std::ostream& lhs, const DynString& a)
    {
        return lhs << a.c;
    }

    char& operator[](size_t i)
    {
        return c[i];
    }

    const char& operator[](size_t i) const
    {
        return c[i];
    }

    DynString& operator+=(const DynString& other)
    {
        char *newArray = new char[strlen(c) + strlen(other.c) + 1];
        std::strcpy(newArray, c);
        std::strcat(newArray, other.c);
        delete[] c;
        c =  newArray;
        return *this;
    }

    DynString operator+(const DynString& other) const
    {
        DynString newString(*this);
        return newString += other;
    }

   DynString& operator=(DynString other) {
       std::swap(c, other.c);
       return *this;
   }
};

DynString operator+(const char* x, DynString& other) {
    DynString newString(x);
    return newString += other;
}

#include <iostream>
int main()
{
    DynString stringA("Hello");
    DynString stringB(" Worlt");
    //UNO
    std::cout << stringA << stringB << std::endl;

    //DUO
    stringB[5] = 'd';

    //TRIO
    DynString stringC = stringA + stringB;
    std::cout << stringC << std::endl;

    DynString stringD;
    //QUATRO
    stringD = "The" + stringB + " Wide Web";
    std::cout << stringD << std::endl;
}

Make the concatenation more efficient

Implementing operator+ by copying and appending is inefficient because it allocates memory for the copy that is immediately released during the append.  So we can revisit that method and eliminate the copy step.  I'd do it using an extra constructor:

private:
    DynString(const DynString& a, const DynString& b)
        : c(new char[std::strlen(a.c) + std::strlen(b.c) + 1])
    {
        std::strcpy(c, a.c);
        std::strcat(c, b.c);
    }

public:
    DynString operator+(const DynString& other) const
    {
        return DynString(*this, other);
    }

We can now simplify operator+=, implementing that in terms of our new operator+:

void swap(DynString& other)
{
    std::swap(length, other.length);
    std::swap(c, other.c);
}

DynString& operator+=(const DynString& other)
{
    Dynstring newString(*this, other);
    swap(newString);
    return *this;
}

Remember the string length

We can make every method more efficient, and also allow for embedded NUL characters in strings, if we store the length as a member:

private:
    std::size_t length;         // not including terminator
    char *c;

    DynString(const DynString& a, const DynString& b)
        : length(a.length + b.length),
          c(new char[length + 1])
    {
        std::memcpy(c, a.c, a.length);
        std::memcpy(c+a.length, b.c, b.length+1); // copies terminator
    }

Lean on the standard library for memory management

Instead of managing our own memory, we could make c be a unique pointer:

private:
    std::size_t length;         // not including terminator
    std::unique_ptr<char[]> c;

    DynString(const DynString& a, const DynString& b)
        : length(a.length + b.length),
          c(std::make_unique<char[]>(length + 1))
    {
        std::memcpy(c.get(), a.c.get(), a.length);
        std::memcpy(c.get()+a.length, b.c.get(), b.length+1); // copies terminator
    }

You'll see that we do need to sprinkle calls to get() where we need pointers to the contents

Improved, efficient, smart-pointer version:

(just the class; everything else unchanged from first "Improved version" above, except an additional include)

#include <memory>

class DynString {
private:
    std::size_t length;         // not including terminator
    std::unique_ptr<char[]> c;

    // Helper constructor - allocates but does not initialize
    DynString(size_t length)
        : length(length),
          c(std::make_unique<char[]>(length + 1))
    {}

    DynString(const DynString& a, const DynString& b)
        : DynString(a.length + b.length)
    {
        std::memcpy(c.get(), a.c.get(), a.length);
        std::memcpy(c.get()+a.length, b.c.get(), b.length+1); // copies terminator
    }

public:
    DynString(const char* input = nullptr)
        : DynString(input ? std::strlen(input) : 0)
    {
        if (input) {
            std::memcpy(c.get(), input, length + 1);
        } else {
            c.get()[0] = '\0';
        }
    }

    DynString(const DynString& other)
        : DynString(other.length)
    {
        if (length != other.length)
            c = std::make_unique<char[]>(other.length+1);
        length = other.length;
        std::memcpy(c.get(), other.c.get(), length + 1);
    }

    DynString(DynString&& other) noexcept
        : DynString()
    {
        swap(other);
    }

    void swap(DynString& other) noexcept
    {
        std::swap(length, other.length);
        std::swap(c, other.c);
    }

    friend std::ostream& operator<<(std::ostream& lhs, const DynString& a)
    {
        return lhs << a.c.get();
    }

    char& operator[](size_t i) noexcept
    {
        return c.get()[i];
    }

    const char& operator[](size_t i) const noexcept
    {
        return c.get()[i];
    }

    DynString& operator+=(const DynString& other)
    {
        DynString newString(*this, other);
        swap(newString);
        return *this;
    }

    DynString operator+(const DynString& other) const
    {
        return DynString(*this, other);
    }

    DynString& operator=(DynString other) noexcept
    {
        swap(other);
        return *this;
    }
};


Partial Maven clean install after Git pull I had searched for a solution to mvn clean install on my changed projects after a git pull. I even asked a question over at SO about the same thing. The answer I got pointed me into creating my own script to achieve what I wanted to accomplish. So I went, I saw, and I conquered the script that I wanted. However, it is probably not efficient, and could use refactoring. I am new to bash scripting and learned a bit while doing this.

The structure I was targeting is a Maven POM Project. That project contains several sub-POM Projects, which in turn contain Maven Java Projects.

Example Structure:

-Parent
|-Sub_Parent_A
 |-Child_1
 |-Child_2
|-Sub_Parent_B
 |-Child_3
 |-Child_4

I wanted to only call mvn clean install if a project had files that changed in the child projects. This would reduce time on going to each project by hand and calling the command or by just calling the command on the parent project.

The code, along with revisions, can also be found on GitHub

################################################################################
#
# License:.....GNU General Public License v3.0
# Author:......CodeMonkey
# Date:........14 November 2018
# Title:.......GitMavenCleanInstall.sh
# Description: This script is designed to cd to a set Maven POM Project,
#   perform a git remote update and pull, and clean install the changed
#   files projects.
# Notice:......The project structure this script was originally set to target
#   is structured as a Maven POM Project that contains several sub-POM Projects.
#   The sub-POM Projects contain Maven Java Application projects. The targets
#   should be easy to change, and allow for others to target other structures.
#
################################################################################
#
# Change History: N/A
#
################################################################################

#!/bin/bash
#Function to check if array has element
containsElement () {
    local e match="$1"
    shift
    for e; do [[ "$e" == "$match" ]] && return 0; done
    return 1
}

#Navigate to the POM Project
cd PATH/TO/POM/PROJECT
#Remote update
git remote update -p
#Pull
git pull

#Get the current working branch
CURRENT_BRANCH="$(git branch | sed -n -e 's/^\* \(.*\)/\1/p')"
#Get the output of the command git diff
GIT_DIFF_OUTPUT="$(git diff --name-status HEAD@{1} ${CURRENT_BRANCH})"

#Split the diff output into an array
read -a GIT_DIFF_OUTPUT_ARY <<< $GIT_DIF_OUTPUT
#Declare empty array for root path
declare -a GIT_DIFF_OUTPUT_ARY_ROOT_PATH=()
FORWARD='/'
#Loop diff output array
for i in "${GIT_DIFF_OUTPUT_ARY[@]}"
do
    #Check that the string is not 1 Character
    if [[ "$(echo -n $1 | wc -m)" != 1 ]]
    then
        #Split the file path by /
        IFS='/' read -ra SPLIT <<< $i
        #Concatenate first path + / + second path
        path=${SPLIT[0]}$FORWARD${SPLIT[1]}
        #Call function to see if it already exists in the root path array
        containsElement "$path" "${GIT_DIFF_OUTPUT_ARY_ROOT_PATH[@]}"
        if [[ $? != 0 ]]
        then
            #Add the path since it was not found
            GIT_DIFF_OUTPUT_ARY_ROOT_PATH+=($path)
        fi
    fi
done

#Loop root path array
for val in ${GIT_DIFF_OUTPUT_ARY_ROOT_PATH[@]}
do
    #CD into root path
    cd $val
    #Maven call to clean install
    mvn -DskipTests=true --errors -T 8 -e clean install
    #CD back up before next project
    cd ../../
done

Take a cleaner approach if you can

The current approach is basically a dirty hack to work around Maven's inability to do incremental builds.
I recommend to start looking into another more modern build system that is able to re-build just what needs to be rebuilt.
Such as Gradle.

Always double-quote variables in arguments

As a rule of thumb, always put double-quotes around variables used as arguments. For example in all these places, there should be double-quotes around the variables:

read -a GIT_DIFF_OUTPUT_ARY <<< $GIT_DIFF_OUTPUT
...

GIT_DIFF_OUTPUT="$(git diff --name-status HEAD@{1} ${CURRENT_BRANCH})"
...

IFS='/' read -ra SPLIT <<< $i
...

GIT_DIFF_OUTPUT_ARY_ROOT_PATH+=($path)
...

for val in ${GIT_DIFF_OUTPUT_ARY_ROOT_PATH[@]}
...

cd $val
...

Avoid performance pitfalls

For each path in the diff,
the script builds the array GIT_DIFF_OUTPUT_ARY_ROOT_PATH with the first two path segments,
and keeps the elements unique by not adding a value if it's already in the array.
As the size of the array grows, this becomes increasingly inefficient,
as this is quadratic time complexity.

I believe the paths printed by git diff is sorted by default.
You can take advantage of that:
track the previous element,
and only add a new value when it's different from the previous.
You will achieve better performance and without using the containsElement function.

Use the exit code directly in conditional expressions

Instead of this:

containsElement "$path" "${GIT_DIFF_OUTPUT_ARY_ROOT_PATH[@]}"
if [[ $? != 0 ]]
then
    #Add the path since it was not found
    GIT_DIFF_OUTPUT_ARY_ROOT_PATH+=($path)
fi

You could use the exit code of the function directly in a conditional:

if ! containsElement "$path" "${GIT_DIFF_OUTPUT_ARY_ROOT_PATH[@]}"
then
    #Add the path since it was not found
    GIT_DIFF_OUTPUT_ARY_ROOT_PATH+=($path)
fi

Get current Git branch name using native Git commands

Instead of this:

CURRENT_BRANCH="$(git branch | sed -n -e 's/^\* \(.*\)/\1/p')"

This is a better, native way:

CURRENT_BRANCH="$(git rev-parse --abbrev-ref HEAD)"

Use native Bash to get string length

This line tries to check the length of a variable:

if [[ "$(echo -n $i | wc -m)" != 1 ]]

Although -m is used, the possible values will not be multibyte,
so the solution is equivalent to using -c.
In this case, a better solution exists with native Bash:

if [[ ${#i} != 1 ]]

Use better variable names

i is good for simple counters. It's not a good name for anything else.

What is FORWARD? path_separator or pathsep would have been better.


Graph implementation to be used for finding Cycles, Paths, closest path between two nodes, etc I am restudying graph theories and I want to write a Graph class that would allow me to implement those methods. Is there a better way of implementing a general Graph class?

Main function:

int main() {

    using namespace GraphUtils;

    Graph<char> graph;

    Node<char> a('a');
    Node<char> b('b');
    Node<char> c('c');
    Node<char> d('d');
    Node<char> e('e');
    Node<char> f('f');

    graph.AddNode(a).AddNode(b).AddNode(c).AddNode(d).AddNode(e).AddNode(f);

    a.AddChild(f).AddChild(d);
    b.AddChild(c).AddChild(a);
    c.AddChild(d);
    e.AddChild(f);
    f.AddChild(b);

    graph.Print();

    if( !PathExist(graph, a, b)) {
        cout << "ERR: a -> b exists" << endl;
    }

    if( !PathExist(graph, a, c)) {
        cout << "ERR: a -> c exists" << endl;
    }

    if( PathExist(graph, a, e)) {
        cout << "ERR: a -> c doesn't exist" << endl;
    }

    if( PathExist(graph, c, e)) {
        cout << "ERR: c -> e doesn't exist" << endl;
    }
    if( !PathExist(graph, e, c)) {
        cout << "ERR: e -> c exists" << endl;
    }
}

Class implementation, which is heavily pointer-oriented:

#include <iostream>
#include <map>
#include <vector>
#include <queue>

using std::map;
using std::vector;
using std::cout;
using std::endl;
using std::queue;

namespace GraphUtils {

template <typename T>
struct Node {
    T data;
    vector<Node*> children;

    Node() {}
    Node(T input): data(input) {}

    Node& AddChild(Node& input) {
        children.push_back(&input);
        return *this;
    }

};

template <typename T>
bool operator<(Node<T>& lhs, Node<T>& rhs) {
    return (lhs.data < rhs.data);
}

template <typename T>
bool operator==(Node<T>& lhs, Node<T>& rhs) {
    return !(lhs.data < rhs.data || lhs.data > rhs.data);
}

template<typename T>
struct Graph {
    vector<const Node<T>* > nodes;

    Graph& AddNode(const Node<T>& input) {
        nodes.push_back(&input);
        return *this;
    }

    void Print() {

        for(typename vector<const Node<T>* >::const_iterator iter = nodes.begin();
                iter != nodes.end();
                iter++)
        {
            cout << (*iter)->data << ":";

            for(typename vector<Node<T>* >::const_iterator child = (*iter)->children.begin();
                    child != (*iter)->children.end();
                    child++)
            {
                cout << (*child)->data << ", ";

            }

            cout << endl;
        }
    }
};

// using BFS to determine if path exists
template<typename T>
bool PathExist(Graph<T>& graph, Node<T>& a, Node<T>& b) {

    map<const Node<T>* , bool> t_visited;
    queue<Node<T>* > t_queue;

    for(typename vector<const Node<T>* >::const_iterator iter = graph.nodes.begin();
            iter != graph.nodes.end(); iter++)
    {
        t_visited[*iter] = false;
    }

    t_queue.push(&a);

    Node<T>* t_node;
    while(!t_queue.empty()) {

        // get the top object
        t_node = t_queue.front();
        t_queue.pop();

        // set visited
        t_visited[t_node] = true;

        // node doesn't have the == operator
        if(*t_node == b) return true;

        // for each ot it's children
        for(typename vector<Node<T>* >::iterator iter = t_node->children.begin();
                iter !=  t_node->children.end(); iter++)
        {
            // if item has not been visited
            if(!t_visited[(*iter)]) {
                t_queue.push((*iter));
            }
        }
    }

    return false;
}

};

That depends on what is good in some sense. It's a pretty standard adjacency list implementation, and it's nothing wrong with that. Whether it's good depends on what query you gonna ask the data structure. If doing path search or minimal spanning tree for sparse graph that is good. If you want to do some clustering, then maybe an adjacency matrix is better.

On the API level, I like that chaining add methods. It's nice and easy to use. Storing pointer to aliases avoided the problem of memory leakage, but it makes the programmer feel a bit silly to declare a variable for each node, with only one character in it. What if the number of nodes dynamically grows, which is one of the strength of using an adjacency list? Why can't I do like graph.AddNode('a')? And add child by graph.get('a').AddChild('b') or something.

Also if the node local variables goes out of scope, the graph must die or in danger of facing segmentation fault, or even some security issues. Not very flexible.


generate a file name with format Name(Number).extension I am writing a program which downloads a file from a URL. Because the name of the downloaded file depends on the url, there is a risk of duplication if the user downloads the same file twice.

For example if the url is : http://www.example.org/myfile.zip
The downloaded file name will be myfile.zip. and if the user downloads it again the name will be myfile(1).zip

To achieve this, I wrote the following code :

    //A file already exist, we use the usual name
    //but add a number before the extension like "Name(X).extension" X being a number

    //Append the number just before the file extension
    auto pos = name.find_last_of(".");
    std::string nameWithoutExt = name.substr(0, pos);
    std::string extension = name.substr(pos);

    std::ostringstream possibleName;
    int i = 1;
    do
    {
        //Clear the string stream
        possibleName.str("");
        possibleName.clear();
        possibleName<< nameWithoutExt << "(" << std::to_string(i) << ")" << extension;
        ++i;

      //Check if a file with the possible name exists
    } while (std::experimental::filesystem::exists(builder.str()));

    name = builder.str();

This solution does not look optimal to me, because it may require a lot of calls to std::experimental::filesystem::existsfunction. Are there any ways to improve this?

There are a couple of improvements that I think you might make to this code.

Provide complete code to reviewers

This is not so much a change to the code as a change in how you present it to other people.  Without the full context of the code and an example of how to use it, it takes more effort for other people to understand your code.  This affects not only code reviews, but also maintenance of the code in the future, by you or by others.  One good way to address that is by the use of comments.  Another good technique is to include test code showing how your code is intended to be used.

Use standard functions where appropriate

Since you're already using the experimental/filesystem routines, why not make better use of them?  Here is a function called uniqueName which shows one way to do that using your current strategy:

fs::path uniqueName(const std::string &name) {
    fs::path possibleName{name};
    auto stem = possibleName.stem().string();
    auto ext = possibleName.extension().string();
    for (int i=1; fs::exists(possibleName); ++i) {
        std::ostringstream fn;
        fn << stem << "(" << i << ")" << ext;
        possibleName.replace_filename(fn.str());
    }
    return possibleName;
}

As suggested by the name of the function, this routine takes the name of a file and either returns it unaltered if no such file exists in the current directory or returns an altered filename as per your schema.  Be aware that after this function is called, some other process could create a file with the same name.

Create a list and use it

As shown in the review by @yuri, you could create a list and then use that.  It has the same potential problem noted above in that another process could create additional files after the list is created.  This version does not require any regex:

std::unordered_set<std::string> create_file_list()
{
    std::unordered_set<std::string> m{};
    for (const auto item : fs::directory_iterator{fs::current_path()}) {
        m.emplace(item.path().filename().string());
    }
    return m;
}

This creates a list of just the filenames (stripping the path).  This can then be used in a minor variation of the routine shown above.  Note that only a single line is different.

fs::path uniqueName(const std::string &name, const std::unordered_set<std::string> files) {
    fs::path possibleName{name};
    auto stem = possibleName.stem().string();
    auto ext = possibleName.extension().string();
    for (int i=1; files.find(possibleName.string()) != files.end(); ++i) {
        std::ostringstream fn;
        fn << stem << "(" << i << ")" << ext;
        possibleName.replace_filename(fn.str());
    }
    return possibleName;
}


Apply smiley replacement filter to Pandoc but not in code segments I wrote a filter for pandas that converts :-) to ☺ but not in inline code or code blocks. Pandoc makes it so the applyExceptOnCodeBlock smileyfy function trickles through the whole tree. But since I operate on Blocks and Inlines I need to handle every data constructor for Block that takes Inlines.

import Text.Pandoc.JSON
import Text.Pandoc.Walk

import Data.List (intercalate)
import Data.List.Split (splitOn)

main :: IO ()
main = toJSONFilter $ applyExceptOnCodeBlock smileyfy

smileyfy :: String -> String
smileyfy = intercalate "☺" . splitOn ":-)"

applyExceptOnCodeInline :: (String -> String) -> Inline -> Inline
applyExceptOnCodeInline f (Str s)       = Str (f s)
applyExceptOnCodeInline f (Code attr s) = Code attr s -- for code nothing shall happen
applyExceptOnCodeInline _ x = x

applyExceptOnCodeBlock :: (String -> String) -> Block -> Block
applyExceptOnCodeBlock f (Plain is) = Plain $ applyExceptOnCodeInline f <$> is
applyExceptOnCodeBlock f (Para  is) = Para  $ applyExceptOnCodeInline f <$> is
applyExceptOnCodeBlock f (LineBlock iss) = LineBlock $ (fmap . fmap) (applyExceptOnCodeInline f) iss
applyExceptOnCodeBlock f (RawBlock format s)  = RawBlock format $ f s
applyExceptOnCodeBlock f (DefinitionList [(is, bss)]) = DefinitionList [(applyExceptOnCodeInline f <$> is
                                                                        ,fmap (fmap (applyExceptOnCodeBlock f)) bss)]
applyExceptOnCodeBlock f (Header i attr is) = Header i attr $ applyExceptOnCodeInline f <$> is
applyExceptOnCodeBlock f (Table is as ds tcs tcss) = Table (applyExceptOnCodeInline f <$> is) as ds tcs tcss
applyExceptOnCodeBlock _ x = x --this covers CodeBlock as well

And thus it looks very convoluted. Is there a clearer way to express this?

There's a much shorter way to express the same functionality, but I'm not sure you'll immediately find it to be clearer.

import Data.List (intercalate)
import Data.List.Split (splitOn)
import Text.Pandoc.JSON

main :: IO ()
main = toJSONFilter smile

smile :: Inline -> Inline
smile (Str s) = Str . intercalate "☺" . splitOn ":-)" $ s
smile x = x

I think the insight you missed is that the Block constructor CodeBlock contains no Inlines, so you can trust the Walkable typeclass machinery to ignore them if your smiley making code only works on Inlines. That eliminates the entire applyExceptOnCodeBlock function.

Similarly it isn't necessary to include a case for Inline's Code constructor, constructors like Code and Math contain their own raw strings and are safely ignored. Pandoc's datatypes are already structured to divide prose text from markup making it easy to define transformations by only matching on the things you do want to modify.


How to increase efficiency of matrix operation in C? I have a N*N upper triangular matrix with property such that, all its diagonal elements are a1,a2,a3,...,aN. I want that a[i][j] (for all j>i) should be

(a[i][j-1] + a[i+1][j]) / 2.

I have many test cases, and I have to apply this property every time to calculate the answer. What is the most optimal way to do this, so that for all test cases the overall running time is less? Test cases: Inputs are N and a1,a2,...,aN.

To calculate the answer, I need to do:

a[0][0] + a[0][2] + ... + a[0][n-1] + a[2][n-1] + a[4][n-1] + ... + a[n-1][n-1].

My solution (which keeps getting timed out):

#include<stdio.h>
double a[2000][2000];
int main(){
int test;
scanf("%d",&test);
//int arr[2000];
while(test--){
    int n,i,j;
    //scanf("%d",&n);
    scanf("%d",&n);
    for(i=0;i<n;i++){
         int num;
         scanf("%d",&num);
         if(n!=1)
            a[i][i] = num*0.5;
         else
            a[i][i] = num;
     }
    for(j=1;j<n;j++){
         int k=j;
         for(i=0;i<n-j;i++,k++){
             if(i==0 && k==n-1)
                 a[i][k] = (a[i+1][k]+a[i][k-1]);
             else
                 a[i][k] = (a[i+1][k]+a[i][k-1])*0.5;
         }
     }
     float sum=0.0;
     for(i=0;i<n;i+=2){
         if( i != n-1 )
         sum+=a[0][i]+a[n-1-i][n-1];
         else
         sum+=a[0][i];
     }
    printf("%.3f\n",sum);
}
getch();
}

Please provide some hints how to optimize the above code.

I suggest dropping the floating point and doing it fixed point.  To do this I
would make a[][] of type int or short (depending upon the input
constraints) and instead of storing values x and x * 0.5, store 2x and x
respectively.  At the end of the summation you can then divide by 2 to get the
true result.

The only issue this raises is that of integer overflow.  You don't state any
numerical constraints but the fixed size arrays (2000x2000) suggest overflow
will not occur with long long.  Alternatively the summation itself could be
floating point while the array values remain integers.  I notice that your
current summation uses float which might well be slower than double.

Another minor point where you can improve the speed is in the input loop.  You
have the conditional

if(n!=1)
    a[i][i] = num*0.5;
else
    a[i][i] = num;

As the loop variable i goes from 0 to n, if n is 1 then the loop only
fills a[0][0] and so this conditional can be removed from the loop.  But if n
is small this will make little difference.


Rename files to all lowercase letters I use this Bash script to rename specified files to all lowercase letters:

#!/bin/bash

usage() {
    test $# = 0 || echo "$@"
    echo "Usage: $0 [OPTION]... FILE..."
    echo
    echo "Rename files to all lowercase letters."
    echo
    echo "  -n, --dry-run         Dry run, show what would happen"
    echo
    echo "  -h, --help            Print this help"
    echo
    exit 1
}

args=
dryrun=off
while [ $# != 0 ]; do
    case $1 in
    -h|--help) usage ;;
    -n|--dry-run) dryrun=on ;;
    --) shift; while [ $# != 0 ]; do args="$args \"$1\""; shift; done; break ;;
    -?*) usage "Unknown option: $1" ;;
    *) args="$args \"$1\"" ;;
    esac
    shift
done

eval "set -- $args"

test $# -gt 0 || usage

for path; do
    test -e "$path" || continue
    origfile=$(basename "$path")
    newfile=$(tr '[:upper:]' '[:lower:]' <<< "$origfile")
    origdir=$(dirname "$path")
    origpath=$origdir/$origfile
    newpath=$origdir/$newfile
    test "$origpath" != "$newpath" || continue
    echo "$path -> $newpath"
    test $dryrun = on || mv -i -- "$path" "$newpath"
done

Is there a better way I missed? Anything to improve?

(This script, along with many other utility scripts I use are on GitHub.)

On case-preserving but case-insensitive filesystems, renaming like that might not work, as the source and destination paths are the "same".  You may need to rename to an intermediate filename first, using mktemp.


Is my N-drome (variation of palindrome) checking program efficient? An N-Drome is a string that is the same forwards and backwards when split into n-character units.
For example, a string would be a 1-drome if and only if it were a standard palindrome.
An example of a 2-drome would be abcdab since its 2-character units are ab, cd, and ab, and its first and last 2-character units are identical.

package ndrome;

import java.util.Scanner;

public class NDrome {
    public static void main(String[] args) {
        System.out.print("enter a string: ");
        Scanner in = new Scanner(System.in);
        String original = in.nextLine();
        System.out.print("enter the n value: ");
        int n = in.nextInt();
        if(n-1>=original.length()){
            System.out.println("Error:N value is greater than string length! exiting...");
            System.exit(0);
        }
        int y = 2;
        here: for (int i = 0, j = original.length() - 1;;) {
            int x = n - 1;
            String a = "", b = "";
            while (x >= 0) {
                if (i >= original.length()) {
                    break here;
                } else {
                    a = a + original.charAt(i);
                    i++;
                    b = b + original.charAt(j - x);
                    x--;
                }
            }
            j=j-n;
            if (a.equals(b)) {
                y = 1;
            } else {
                y = 0;
                break;
            }
        }
        if (y == 1) {
            System.out.print("this is a " + n + "-drome");
        } else if (y == 0) {
            System.out.print("this is not a " + n + "-drome");
        }
    }
}

I'm not a Java programmer, but I'll point out some familiar syntactical things I see:

You should separate the logic by putting the checking code into a separate function, leaving main() to call the function, handle the input/output, and terminate when needed.
What is the purpose of y?  From the looks of it, it's used to hold an "error condition" for indicating whether or not the inputted string is an N-drome.  If so, I think this should be a boolean type and given a relevant name.  Either way, y should be renamed to reflect its purpose.

What about strings a and b?  Are they the leftmost one and the rightmost one for the original string?  You should write out the purpose of these single-char variables to give them more meaning.
The lack of an increment value in the for-loop doesn't look too clear.  If someone wanted to see what the loop should do after each iteration, they would have to fish through the loop body.  If the loop won't be incremented in each control path, then it may not be the best loop type to use.
I'm not familiar with break here, but it looks like a goto in C++.  I don't think something like that should be done with a loop (or probably anything practical).  Again, you may need to rethink how you approach this loop type and execution.


Increasing visible text with on-click "read more" What can I do better?

$(function () {
    "use strict";

    /*
     *  Header text. Vergroot de tekst door op lees meer te klikken
     */
    var heading = $(".header .text h2").height(),
        firstP = $(".header p:first").height(),
        areaSmall = heading + firstP,
        areaBig = $(".header .text").height(),
        box = $(".header .text"),
        readmore = $('.header .text a[title*="meer"]'),
        closeBox = $('.header .text a[title*="Sluit"]');

    $(".header .text").css({ height: areaSmall });

    readmore.click(function () {
        $(".header .text").animate({ height: areaBig }, 1000, "easeInQuart", function () {
            $(".header .text p").css({ visibility: "visible" });
        });
    });

    closeBox.click(function () {
        $(".header .text").animate({ height: areaSmall }, 1000, "easeOutQuart", function () {
            $(".header .text p").css({ visibility: "hidden" });
            $(".header .text p:first").css({ visibility: "visible" });
        });
    });

});

You have lots of calls to $(".header .text"), which you can replace with calls to box:

var areaBig = box.height();

Similarly, you can simplify calls to selectors within box, such as:

// Better than: readmore = $('.header .text a[title*="meer"]')
var readmore = box.find('a[title*="meer"]');

// Better than: $(".header .text").animate(...
box.animate({ height: areaBig }, 1000, "easeInQuart", function () {
  box.find("p").css({ visibility: "visible" });
});

These changes will not only make the code easier to read but also increase execution speed.


Is this powershell code to install the linux sl command to PowerShell good? I have the following code in my project that installs linux sl on Windows using PowerShell. Is there anything you'd improve about this?

param([String]$Profile, [Switch]$Help, [Switch]$Force)

$ErrorMessages = @()

$Payload = "
# <Code inserted by Install-Steam-Locomotive>
function Steam-Locomotive {wsl sl -e}
function Steam-Locomotive-Force {wsl sl}
# </>
"

$HelpText = "This script helps you use the tremendous `"sl`" program in Windows PowerShell.
Simply download the .ps1 file and execute it.
If the script finishes successfully, you can type Steam-Locomotive in PS to start the interruptable `"sl -e`".
Use Steam-Locomotive-Force to prevent interruption (`"sl`").
Example:
PS C:\Users\User\Downloads\> .\Install-Steam-Locomotive.ps1 -Profile $PROFILE
"

if ($Help) {
  Write-Output $HelpText
  exit
}

if (!(Get-Command -Name "wsl" -CommandType "Application" -ErrorAction SilentlyContinue)) {
  $ErrorMessages += @("You don't have WSL installed. Cannot continue.")
}

if (!$Profile) {
  $ErrorMessages += @("Please supply your profile location under -Profile. Cannot continue.")
  if (Get-Content $Profile -ErrorAction SilentlyContinue | Select-String "Steam-Locomotive") {
    $ErrorMessages += @("Your profile seems to already contain something called `"Steam-Locomotive`". Use -Force to continue anyways.")
  }
}

if ((Get-Command -Name "Steam-Locomotive" -ErrorAction SilentlyContinue) -and !$Force) {
  $ErrorMessages += @("It seems a command named `"Steam-Locomotive`" is already installed. Use -Force to continue anyways.")
}

if ($ErrorMessages) {
  Write-Output $ErrorMessages
  exit
}

if (!(wsl command -v sl)) {
  Write-Output "First, install SL."
  wsl sudo apt install sl
}

if (wsl command -v sl) {
  Add-Content $Profile $Payload
  Write-Output "Done!"
}

if (!(wsl command -v sl)) {
  Write-Output "Failed installing wsl."
  exit
}

Write-Output "Success!"

EDIT: I had an error in this script. The fixed version is up on the project page.

Use the script template for documentation
Use a cmdlet binding for parameter
validation and help text
Personally, I run with $ErrorActionPreference = "Stop"


Skip by Id on large list This is in ASP.NET MVC, but that shouldn't make a difference. I have a large number of rows in my database for Songs.

var songIndex = Context.Songs.ToList().FindIndex(x => x.Id == 500);
var songs = Context.Songs.OrderBy(x => x.Id).Skip(songIndex).Take(10);

However, the ToList() and OrderBy seem pretty heavy just to take the next next 10 songs after the element with Id of 500.

Is there a better way to do this so I don't have to modify the entire list?

How about using a simple Where clause?

var songs = Context.Songs
    .OrderBy(x => x.Id)
    .Where(x => x.Id > 500)
    .Take(10);


Posting data via fetch and handling network errors I've built a login panel, which will submit an email and password via fetch/POST. And I wanted to know the best way to handle network status errors?
Reason being, as I want to inject error messages into my login panel for each state/status code.
I've written the code below, which works. But I wondered if there is a better way to write this?  For example, should my status code handling go in the catch() etc?
let url = 'www.MYAPI.com/xyz';

// post body data
let data = {
    "Username": emailAddress,
    "Password": Password,
  }

// create request object
let request = new Request(url, {
    "method": 'POST',
    "mode": 'cors',
    "credentials": 'include',
    "body": JSON.stringify(data),
    "referrerPolicy": "strict-origin-when-cross-origin",
    "headers": {
        "accept": "application/json, text/plain, */*",
        "content-type": "application/json;charset=UTF-8",
        "sec-fetch-mode": "cors",
        "sec-fetch-site": "same-origin",
        "x-requested-with": "XMLHttpRequest"
    }
});

fetch(request)
.then( async (response) => {
    let data = await response.json();

    if(response.status === 200){
     // LOGIN SUCCESSFUL HERE;
        console.log("Successful log in - status: ", data);
        document.getElementById("id01").style.display = "none"; // HIDE LOGIN PANEL
    } else{
     // Rest of status codes (400,500,303), can be handled here appropriately

        // IF EMAIL ADDRESS ERROR / STATUS 502
        if (data.KnownErrorDescription === "MemberEmailDoesNotExist") {
            document.getElementById("email-error").innerHTML = "Please enter a valid email address to sign in";
        };

        // IF PASSWORD IS EMPTY / STATUS 400
        if (data.Message === "The request is invalid.") {
            document.getElementById("password-error").innerHTML = data.ModelState["request.Password"][0];
        }

        // // IF ACCOUNT LOCKED / 502
        if (data.KnownErrorDescription === "MemberAccountLocked") {
            document.getElementById("failed-sign-in-error").innerHTML = "You need to reset your password";
            document.getElementById("password-reset").style.display = "block";
        }
    };

  })
  .catch((err) => {
      console.log(err);
  })

I personally think that You can easily handle part of the error handling logic in the catch block however at the moment You are listening for the specific response status do specify what went wrong during the API connection process.
MDN says:

You can use a catch block to handle all exceptions that may be generated in the try block.

try/catch works in a way that if anything fails on the try block, then the rest of the code (in the try block) is ignored and the script execution goes to the catch block. If something goes wrong then the error object is created and passed to the catch block as a parameter .catch(err). From this object You might be able to handle part of the error handling logic. Take a look at the below links:
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/try...catch
https://javascript.info/try-catch
Moreover:

data variable can be a const
Instead of writing .style.display = "none"; use YourElement.classList.add('YourNewSexyClass') and use CSS for styling :)
innerHTML is not a good practice. Take a look here -> https://developer.mozilla.org/en-US/docs/Web/API/Element/innerHTML#security_considerations
url variable should a const
data variable should be a const

let data = {
    "Username": emailAddress,
    "Password": Password,
  }

can be changed like so (get rid of capital letters here)
let data = {
    username: emailAddress,
    password: password
  }

so that You can change it to
let data = {
    username: emailAddress,
    password
  }


Java Trie Implementation I am creating a Trie class in Java, and am wondering what else can be done to make it even better. I am hoping to add concurrency to speed querying up.

public class Trie {
    private HashMap<Character, HashMap> root;
    private final Character END_CHARACTER = '$';

    public Trie() {
        initializeRoot();
    }

    public Trie(String s) {
        initializeRoot();
        add(s);
    }

    public Trie(Collection<String> collection) {
        initializeRoot();
        for (String s : collection) {
            add(s);
        }
    }

    private void initializeRoot() {
        root = new HashMap<>();
    }

    public void add(String s) {
        HashMap<Character, HashMap> node = root;
        for (int i = 0; i < s.length(); i++) {
            Character character = s.charAt(i);
            if (!node.containsKey(character)) {
                node.put(character, new HashMap<Character, HashMap>());
            }
            node = node.get(character);
        }
        node.put(END_CHARACTER, new HashMap<>());
    }

    public boolean contains(String s) {
        HashMap<Character, HashMap> node = root;
        for (int i = 0; i < s.length(); i++) {
            Character character = s.charAt(i);
            if (node.containsKey(character)) {
                node = node.get(character);
            } else {
                return false;
            }
        }
        return node.containsKey(END_CHARACTER);
    }
}

Initializing private HashMap<Character, HashMap> root; at declaration will simplify your code

public class Trie {
    private HashMap<Character, HashMap> root = new HashMap<>();
    private final Character END_CHARACTER = '$';

    public Trie() {}

    public Trie(String s) {
        add(s);
    }

    public Trie(Collection<String> collection) {
        for (String s : collection) {
            add(s);
        }
    }

    public void add(String s) {
        HashMap<Character, HashMap> node = root;
        for (int i = 0; i < s.length(); i++) {
            Character character = s.charAt(i);
            if (!node.containsKey(character)) {
                node.put(character, new HashMap<Character, HashMap>());
            }
            node = node.get(character);
        }
        node.put(END_CHARACTER, new HashMap<>());
    }

    public boolean contains(String s) {
        HashMap<Character, HashMap> node = root;
        for (int i = 0; i < s.length(); i++) {
            Character character = s.charAt(i);
            if (node.containsKey(character)) {
                node = node.get(character);
            } else {
                return false;
            }
        }
        return node.containsKey(END_CHARACTER);
    }
}

From whats-wrong-with-overridable-method-calls-in-constructors

  Simply put, this is wrong because it unnecessarily opens up possibilities to MANY bugs. When the @Override is invoked, the state of the object may be inconsistent and/or incomplete.

This having in mind we will refactor the class, to add a private internalAdd() method which we call from the constructor and the public add() methods

public class Trie {
    private HashMap<Character, HashMap> root = new HashMap<>();
    private final Character END_CHARACTER = '$';

    public Trie() {}

    public Trie(String s) {
        internalAdd(s);
    }

    public Trie(Collection<String> collection) {
        for (String s : collection) {
            internalAdd(s);
        }
    }

    public void add(String s) {
        internalAdd(s);
    }

    private void internalAdd(String s) {
        HashMap<Character, HashMap> node = root;
        for (int i = 0; i < s.length(); i++) {
            Character character = s.charAt(i);
            if (!node.containsKey(character)) {
                node.put(character, new HashMap<Character, HashMap>());
            }
            node = node.get(character);
        }
        node.put(END_CHARACTER, new HashMap<>());
    }

    public boolean contains(String s) {
        HashMap<Character, HashMap> node = root;
        for (int i = 0; i < s.length(); i++) {
            Character character = s.charAt(i);
            if (node.containsKey(character)) {
                node = node.get(character);
            } else {
                return false;
            }
        }
        return node.containsKey(END_CHARACTER);
    }
}

but we can do better.. let us take a look at internalAdd()

private void internalAdd(String s) {
    HashMap<Character, HashMap> node = root;
    for (int i = 0; i < s.length(); i++) {
        Character character = s.charAt(i);
        if (!node.containsKey(character)) {
            node.put(character, new HashMap<Character, HashMap>());
        }
        node = node.get(character);
    }
    node.put(END_CHARACTER, new HashMap<>());
}

If node.isEmpty() we won't need to check if (!node.containsKey(character)) anymore.
Also if (!node.containsKey(character)) evaluates one time to true, we won't need to check this anymore. Let us add a new method:

private void internalAdd(String s, HashMap<Character, HashMap> node) {
    for (int i = 0; i < s.length(); i++) {
        Character character = s.charAt(i);
        node.put(character, new HashMap<>());
        node = node.get(character);
    }
}

and call it

private void internalAdd(String s) {
    HashMap<Character, HashMap> node = root;
    for (int i = 0; i < s.length(); i++) {
        Character character = s.charAt(i);
        if (node.isEmpty() || !node.containsKey(character)) {
            internalAdd(s.substring(i), node);
            break;
        }
        node = node.get(character);
    }
    node.put(END_CHARACTER, new HashMap<>());
}

But wait, we can do even better, as node.isEmpty() should also be used in the contains() method

public boolean contains(String s) {
    HashMap<Character, HashMap> node = root;
    for (int i = 0; i < s.length(); i++) {
        if(node.isEmpty()){
            return false;
        }
        Character character = s.charAt(i);
        if (node.containsKey(character)) {
            node = node.get(character);
        } else {
            return false;
        }
    }
    return node.containsKey(END_CHARACTER);
}

Finished ? No, we still can do better. As the passed String parameters won't be changed, let us make them final the same is true for the root field. Also as janos has answered

  use interface types instead of the implementation

Putting altogether

public class Trie {

    private final Map<Character, HashMap> root = new HashMap<>();
    private final Character END_CHARACTER = '$';

    public Trie() {
    }

    public Trie(final String s) {
        internalAdd(s);
    }

    public Trie(final Collection<String> collection) {
        for (String s : collection) {
            internalAdd(s);
        }
    }

    public void add(final String s) {
        internalAdd(s);
    }

    private void internalAdd(final String s) {
        Map<Character, HashMap> node = root;

        for (int i = 0; i < s.length(); i++) {
            Character character = s.charAt(i);
            if (node.isEmpty() || !node.containsKey(character)) {
                internalAdd(s.substring(i), node);
                break;
            }
            node = node.get(character);
        }
        node.put(END_CHARACTER, new HashMap<>());
    }

    private void internalAdd(final String s, Map<Character, HashMap> node) {
        for (int i = 0; i < s.length(); i++) {
            Character character = s.charAt(i);
            node.put(character, new HashMap<>());
            node = node.get(character);
        }
    }

    public boolean contains(final String s) {
        Map<Character, HashMap> node = root;

        for (int i = 0; i < s.length(); i++) {
            if (node.isEmpty()) {
                return false;
            }
            Character character = s.charAt(i);
            if (node.containsKey(character)) {
                node = node.get(character);
            } else {
                return false;
            }
        }
        return node.containsKey(END_CHARACTER);
    }
}


Circular Buffer on a typed Float32Array I am sure there are better ways to do things but this is part of my first JavaScript project and I'm slowly getting accustomed with how things work in JavaScript. This is also why I ask for a code review, to find what I could not see.

Suggestions on idiomatic JavaScript would be welcome.

/* Circular buffer used to add values in blocks of constant size */
function IndexOutOfBounds(maxIndex, requestedIndex, method) {

  this.maxIndex       = maxIndex;
  this.requestedIndex = requestedIndex;
  this.method         = method;

  this.toString = function () {
    return "Invalid index in method ['" + this.method + "']\n" +
           "Requested index    : " + this.requestedIndex + "\n" +
           "Valid _buffer index : " + "[0.." + this.maxIndex + "]";
  };
}

function ImproperBlockLength(ringBlockLength, givenBlockLength) {
  this.ringBlockLength  = ringBlockLength;
  this.givenBlockLength = givenBlockLength;

  this.toString = function () {
    return "Block length mismatch.\n" +
           "Requeste block length : " + this.givenBlockLength  + "\n" +
           "Valid block length    : " + this.ringBlockLength;
  };
}

function Ring(length, blockLength) {
  this.length    = length;
  this._maxIndex = this.length - 1;
  this._start    = 0;
  this._buffer   = new Float32Array(this.length);

  /* blockLength should always be a factor of size.
   * An exception is thrown if it's not;
   */
  this._blockLength = 0;

  if (blockLength) {

    if (length % blockLength != 0) {
      throw "Block length must be a factor of length.";
    } else {
      this._blockLength = blockLength;
    }

  }
}

Ring.prototype.checkBounds = function (requested, callerName) {

  if (requested < 0 || requested > this._maxIndex)
    throw new IndexOutOfBounds(this._maxIndex, requested, callerName);
};

Ring.prototype.relativeIndex = function(index) {
  return (this._start + index) % this.length;
};

/* Should not be used when there is a set _blockLength */
Ring.prototype.push = function (element) {

  this._buffer[this._start] = element;

  var newStart = this._start + 1;
  this._start = newStart > this._maxIndex ? 0 : newStart;
};

Ring.prototype.get = function (index) {

  this.checkBounds(index, 'get');

  return this._buffer[this.relativeIndex(index)];
};

Ring.prototype.set = function (index, value) {

  this.checkBounds(index, 'set');

  this._buffer[this.relativeIndex(index)] = value;

};

Ring.prototype.concat = function(arr) {

  var alen = arr.length;
  var blen = this._blockLength;

  if (alen != blen) {
    throw new ImproperBlockLength(blen, alen);
  }

  this._buffer.set(arr, this._start);
  this._start = (this._start + alen) % this.length;

};

Ring.prototype.map = function (callback) {

  var relativeIndex;
  var value;

  for(var i = 0 ; i < this.length ; i++) {
    relativeIndex = this.relativeIndex(i);
    value = this._buffer[relativeIndex];

    this._buffer[relativeIndex] = callback(value, i, this.length);
  }
};

I haven't examined the actual functionality, but this is based on my comment about moving this.toString to a prototype method (which is, I feel, the main improvement) and then I've done a few micro optimisations (but nothing that is going to make any real noticeable difference). Other than that I performed a bit of personal styling, there was nothing particularly wrong with what you had.

Javascript

/*jslint maxerr: 50, indent: 4, browser: true, nomen: true */
/*global Float32Array */

(function () {
    "use strict";

    /* Circular buffer used to add values in blocks of constant size */

    function IndexOutOfBounds(maxIndex, requestedIndex, method) {
        this.maxIndex = maxIndex;
        this.requestedIndex = requestedIndex;
        this.method = method;
    }

    IndexOutOfBounds.prototype.toString = function () {
        return "Invalid index in method ['" + this.method + "']\nRequested index    : " + this.requestedIndex + "\nValid _buffer index : [0.." + this.maxIndex + "]";
    };

    function ImproperBlockLength(ringBlockLength, givenBlockLength) {
        this.ringBlockLength = ringBlockLength;
        this.givenBlockLength = givenBlockLength;
    }

    ImproperBlockLength.prototype.toString = function () {
        return "Block length mismatch.\nRequeste block length : " + this.givenBlockLength + "\nValid block length    : " + this.ringBlockLength;
    };

    function Ring(length, blockLength) {
        this.length = length;
        this._maxIndex = length - 1;
        this._start = 0;
        this._buffer = new Float32Array(length);

        /* blockLength should always be a factor of size.
         * An exception is thrown if it's not;
         */
        this._blockLength = 0;
        if (blockLength) {
            if (length % blockLength !== 0) {
                throw "Block length must be a factor of length.";
            }

            this._blockLength = blockLength;
        }
    }

    Ring.prototype.checkBounds = function (requested, callerName) {
        var maxIndex = this._maxIndex;

        if (requested < 0 || requested > maxIndex) {
            throw new IndexOutOfBounds(maxIndex, requested, callerName);
        }
    };

    Ring.prototype.relativeIndex = function (index) {
        return (this._start + index) % this.length;
    };

    /* Should not be used when there is a set _blockLength */
    Ring.prototype.push = function (element) {
        var start = this._start,
            newStart = start + 1;

        this._buffer[start] = element;
        this._start = 0;
        if (newStart <= this._maxIndex) {
            this._start = newStart;
        }
    };

    Ring.prototype.get = function (index) {
        this.checkBounds(index, 'get');

        return this._buffer[this.relativeIndex(index)];
    };

    Ring.prototype.set = function (index, value) {
        this.checkBounds(index, 'set');
        this._buffer[this.relativeIndex(index)] = value;

    };

    Ring.prototype.concat = function (arr) {
        var alen = arr.length,
            blen = this._blockLength,
            start;

        if (alen !== blen) {
            throw new ImproperBlockLength(blen, alen);
        }

        start = this._start;
        this._buffer.set(arr, start);
        this._start = (start + alen) % this.length;

    };

    Ring.prototype.map = function (callback) {
        var relativeIndex,
            length = this.length,
            i = 0;

        while (i < length) {
            relativeIndex = this.relativeIndex(i);
            this._buffer[relativeIndex] = callback(this._buffer[relativeIndex], i, length);
            i += 1;
        }
    };
}());

One other thing to consider is, because you will only be using newer browsers that support Float32Array then you could also define properties using Object.defineProperty

Update: Here is a refactoring using Object.defineProperty, I did it kind of quick so there are no guarantees that I did it correctly, it is more to give you a starter, hope it helps.

Javascript

/*jslint maxerr: 50, indent: 4, browser: true, nomen: true, bitwise: true */
/*global console, Float32Array */

(function () {
    "use strict";

    var Ring = (function () {
        /* Circular buffer used to add values in blocks of constant size */

        var minIndex = 0,
            maxIndex = 4294967294;

        function clamp(number, min, max) {
            return Math.min(Math.max(number, min), max);
        }

        function IndexOutOfBounds(maxIndex, requestedIndex, method) {
            Object.defineProperties(this, {
                "maxIndex": {
                    value: maxIndex
                },

                "requestedIndex": {
                    value: requestedIndex
                },

                "method": {
                    value: method
                }
            });
        }

        Object.defineProperty(IndexOutOfBounds.prototype, "toString", {
            get: function () {
                return "Invalid index in method ['" + this.method + "']\nRequested index    : " + this.requestedIndex + "\nValid _buffer index : [0.." + this.maxIndex + "]";
            }
        });

        function ImproperBlockLength(ringBlockLength, givenBlockLength) {
            Object.defineProperties(this, {
                "ringBlockLength": {
                    value: ringBlockLength
                },

                "givenBlockLength": {
                    value: givenBlockLength
                }
            });
        }

        Object.defineProperty(ImproperBlockLength.prototype, "toString", {
            get: function () {
                return "Block length mismatch.\nRequeste block length : " + this.givenBlockLength + "\nValid block length    : " + this.ringBlockLength;
            }
        });

        function Ring(length, blockLength) {
            length = clamp(length, minIndex, maxIndex + 1) >>> 0;

            /* blockLength should always be a factor of size.
             * An exception is thrown if it's not;
             */
            blockLength = clamp(blockLength, minIndex, maxIndex + 1) >>> 0;
            if (length % blockLength !== 0) {
                throw "Block length must be a factor of length.";
            }

            var privateStart = 0,
                privateBuffer = new Float32Array(length);

            Object.defineProperties(this, {
                "length": {
                    get: function () {
                        return length;
                    },

                    set: function (newLength) {
                        length = clamp(newLength, minIndex, maxIndex + 1) >>> 0;
                    }
                },

                "_start": {
                    get: function () {
                        return privateStart;
                    },

                    set: function (start) {
                        privateStart = clamp(start, minIndex, maxIndex) >>> 0;
                    }
                },

                "_buffer": {
                    get: function () {
                        return privateBuffer;
                    },

                    set: function (buffer) {
                        privateBuffer = buffer;
                    }
                },

                "_blockLength": {
                    value: blockLength
                }
            });
        }

        Object.defineProperties(Ring.prototype, {
            "_maxIndex": {
                value: function () {
                    return clamp(this.length - 1, minIndex, maxIndex) >>> 0;
                }
            },

            "checkBounds": {
                value: function (requested, callerName) {
                    var maxIndex = this._maxIndex;

                    if (requested < 0 || requested > maxIndex) {
                        throw new IndexOutOfBounds(maxIndex, requested, callerName);
                    }

                    return maxIndex;
                }
            },

            "relativeIndex": {
                value: function (index) {
                    return (this._start + index) % this.length;
                }
            },

            /* Should not be used when there is a set _blockLength */
            "push": {
                value: function (element) {
                    var start = this._start,
                        newStart = start + 1;

                    this.buffer = element;
                    if (newStart > this._maxIndex) {
                        this._start = 0;
                    } else {
                        this._start = newStart;
                    }
                }
            },

            "get": {
                value: function (index) {
                    this.checkBounds(index, 'get');

                    return this._buffer[this.relativeIndex(index)];
                }
            },

            "set": {
                value: function (index, value) {
                    this.checkBounds(index, 'set');
                    this._buffer[this.relativeIndex(index)] = value;
                }
            },

            "concat": {
                value: function (arr) {
                    var alen = arr.length,
                        blen = this._blockLength,
                        start;

                    if (alen !== blen) {
                        throw new ImproperBlockLength(blen, alen);
                    }

                    start = this._start;
                    this._buffer.set(arr, start);
                    this._start = (start + alen) % this.length;
                }
            },

            "map": {
                value: function (callback) {
                    var relativeIndex,
                        length = this.length,
                        i = 0;

                    while (i < length) {
                        relativeIndex = this.relativeIndex(i);
                        this._buffer[relativeIndex] = callback(this._buffer[relativeIndex], i, length);
                        i += 1;
                    }
                }
            }
        });

        return Ring;
    }());

    var ring = new Ring(10, 1);

    console.log({
        0: ring,
        1: ring.length
    });
}());

On jsfiddle


Family and vehicles example using the strategy pattern I have studied the strategy pattern and understood it w.r.t the example given in the PDF that I referred. I wanted to make sure that my understanding is correct, so I have written a simple program in C++ and designed the strategy pattern using the family and vehicles as the subject. Please provide corrections/suggestions for the program.

#include <iostream>

class Vehicle
{
public:
    virtual void useVehicle() = 0;
};

class FordFigo : public Vehicle
{
public:
    void useVehicle()
    {
        std::cout << "Driving FordFigo" << std::endl;
    }
};

class TataNano : public Vehicle
{
public:
    void useVehicle()
    {
        std::cout << "Driving TataNano" << std::endl;
    }
};

class BlackPulsar : public Vehicle
{
public:
    void useVehicle()
    {
        std::cout << "Riding BlackPulsar" << std::endl;
    }
};

class HondaActiva : public Vehicle
{
public:
    void useVehicle()
    {
        std::cout << "Riding HondaActiva" << std::endl;
    }
};

class Family
{
public:
    Vehicle *myVehicle;
    virtual void display() = 0;
    void setVehicle(Vehicle *v)
    {
        myVehicle = v;
    }
    void driveVehicle()
    {
        myVehicle->useVehicle();
    }
};

class Father : public Family
{
public:
    virtual void display()
    {
        std::cout << "I am Father" << std::endl;
    }
};

class Mother : public Family
{
public:
    virtual void display()
    {
        std::cout << "I am Mother" << std::endl;
    }
};

class Son : public Family
{
public:
    virtual void display()
    {
        std::cout << "I am Son" << std::endl;
    }
};

class Daughter : public Family
{
public:
    virtual void display()
    {
        std::cout << "I am Daughter" << std::endl;
    }
};

int main()
{
    Family *fam = new Father();
    fam->display();
    fam->setVehicle(new FordFigo());
    fam->driveVehicle();

    fam = new Daughter();
    fam->display();
    fam->setVehicle(new HondaActiva());
    fam->driveVehicle();
    return 0;
}

In "modern C++" it is unusual to see (and use) RAW pointers.

class Family
{
public:
    Vehicle *myVehicle;

Here you have a pointer to a vehicle. But there is no indication of who owns the vehicle. Does the family member own the vehicle or is a shared resource? Can a family member have no vehicle in which case what is the appropriate response to driveVehicle()? Maybe with no vehicle they default to walking?

Anyway. Don't use pointers. In terms of memory management they do not indicate who the owner of the pointed at object is; Note: it is the responsibility of the owner to delete the object. If you don't know who should delete it you have memory leaks (and there is a lot in your code).

Also by default pointers are not set to NULL. So if you do not explicitly initialize them in a constructor you have a random value (so initialize them).

What happens if myVehicle is NULL here?

void driveVehicle()
{
    myVehicle->useVehicle();
}

You should mark virtual implementations as override

void display() override  // indicates this is the implementation
                         // of a virtual function.
{
    std::cout << "I am Daughter" << std::endl;
}

Never use dynamic memory memory management when local variables will do.

Family *fam = new Father();

// This should be:

Father father;


Restart Services Automatically I have been working on a script that automatically checks on the state of 2 services that I require to be running 24/7 on a server that I manage.

The script works as I need, but I would really like to optimize it if possible.
Specifically, I find that I want a way to reduce the need to type the command:

$(ps -ef | grep -v grep | grep zabbix_agentd | wc -l)

and

$(ps -ef | grep -v grep | grep zabbix_server | wc -l)

Any other feedback is greatly appreciated, I am new to bash scripting and trying to find the best way to get this to be as optimal as possible!

#!/bin/bash
zabbix_server="service zabbix-server"
zabbix_agent="service zabbix-agent"
logfile=/etc/scripts/zabbix/zabbix_auto_restart.log
zabbix_server_running=0
zabbix_agent_running=0

check_zabbix_agentd (){
        if (( $(ps -ef | grep -v grep | grep zabbix_agentd | wc -l) <= 0 ))
        then
           $zabbix_agent start
           echo `date` "$zabbix_agent was stopped... Restarting" >> $logfile
           echo "************************************************" >> $logfile

           #Send email to notify that the script ran
           echo "$(date) $zabbix_agent was restarted from zabbix_restart.sh" | mutt -s "Zabbix Auto-restart Script Just Ran" <admin email address>

        else
           let zabbix_agent_running=1
        fi
}

check_zabbix_server (){
        if (( $(ps -ef | grep -v grep | grep zabbix_server | wc -l) <= 0 ))
        then
           $zabbix_server start
           echo `date` "$zabbix_server was stopped... Restarting" >> $logfile
           echo "************************************************" >> $logfile

           #Send email to notify that the script ran
           echo "$(date) $zabbix_server was restarted from zabbix_restart.sh" | mutt -s "Zabbix Auto-restart Script Just Ran" <admin email address>

        else
           let zabbix_server_running=1
        fi
}

main_loop (){
        until ((zabbix_server_running == 1 && zabbix_agent_running == 1));
        do
            check_zabbix_agentd
            check_zabbix_server
            sleep 1.5
        done
}

main_loop

As you and I both know, what follows is the answer I posted to your question at Unix and Linux. I'm not sure if this might be considered in bad taste or not - and if so, I'll happily delete it - but at the other site the question was apparently closed as a cross-post in favor of this. And so I thought - maybe it belonged? Anyway, here goes:

What you're really doing wrong is duplicating your effort - basically every hardcoded occurrence of _agent or _server appears to be completely redundant.

For example, if this is being run on a linux system, you can completely drop the grep_...() functions, and consolidate both check_...s into a single entity which might work like:

email(){
    mutt -s "Zabbix Auto-restart Script Just Ran" \<user email\>
}
prlog(){
    date +"%x %X:%tservice $1${2+%n************************}"
}
chk_run()
    while  [ "$#" -gt 0 ]
    do     if     ps -C zabbix_"$1"
           then   : "$(($1=1))"
           else   set zabbix_"$@"
                  service "$1" start || eval >&2 '
                  prlog   "$1 restart failed."  +; exit '"$?"
                  prlog   "$1 restarted."       +  >&2
                  prlog   "$1 restarted from $0." |email
           fi;    shift
    done

The key to that is you would just call chk_run with an argument list each member of which would indicate to it what it should be checking each iteration.

loop()
    until [ "$(($1&&$2))" -eq 1 ]
    do    chk_run "$@"
          sleep 2
    done  >/dev/null 2>>"$log"
agentd=0 server=0 loop agentd server

POSIXly the only thing that should need altering there is the ps command - because POSIX doesn't specify the -C switch. And so you could just change the if line to look like:

if    ps -eocomm= |
      grep -xqF zabbix_"$1"

Aside from mutt, service, and the ps optimization, it should all be standard command language. At least one advantage to that is the #!/bin/bash hash-bang is completely unnecessary - there is no anchor here to some shell-specific extension, and so it should work pretty much exactly the same in all shells which strive for POSIX-compliance. That means that #!/bin/dash is a very simple optimization in this case.


Is it fine to use my domain model as DTO (Entity Framework Core)? I was reading about anemic data models and rich domain models in DDD. I don't want to follow DDD completely but take rather pragmatic approach and just take some concepts out of it because clean ddd seems like an overkill. What I'm building is Web API (using ASP.NET Core and Entity Framework Core).

I don't want to maintain separate DTOs just for serializing and deserializing my models.

And I want to make most of the things like service layer and my controllers generic. Because it's mostly CRUD and logic is repeating - a lot of boilerplate.

I'm not planning to use any other ORM other than EF Core.

EF Core allows mapping to backing fields:

https://ardalis.com/encapsulated-collections-in-entity-framework-core
https://technet.microsoft.com/en-us/mt842503.aspx

Is it fine if I will design my models like this and at the same time use them as my DTOs for serialization and deserialization?

public class Player
{
    [JsonIgnore]
    public int Id { get; set; }

    [NotMapped, Required]
    public PlayerInfo ActiveInfo { get; set; } // setter must be public to allow deserialization (or use custom resolver for json net)

    [JsonIgnore]
    public ICollection<PlayerInfo> PlayerInfos { get; set; } = new HashSet<PlayerInfo>();

    private Player()
    {
    }

    public Player(PlayerInfo info)
    {
        info.IsActive = true;
        PlayerInfos.Add(info);
    }

    public void SetActiveInfo(PlayerInfo playerInfo)
    {
        var currentlyActiveInfo = PlayerInfos.SingleOrDefault(info => info.IsActive);
        if (currentlyActiveInfo != null)
        {
            currentlyActiveInfo.IsActive = false;
        }
        playerInfo.IsActive = true;
        PlayerInfos.Add(playerInfo);

        ActiveInfo = playerInfo;
    }
}

This is controller:

public class PlayerController
{
    private readonly DbContext _dbContext { get; set; }
    private readonly DbSet<Player> _players { get; set; }

    public PlayerController(DbContext dbContext)
    {
        _dbContext = dbContext;
        _players = dbContext.Set<Player>();
    }

    // Create new player
    [HttpPost]
    public ActionResult<Player> Post([FromBody] Player player)
    {
        if (!ModelState.IsValid)
        {
           //Handle validation error
        }
        // No service layer for the sake of simplicity.
        player.SetActiveInfo(player.ActiveInfo); // will use _activeInfo
        _players.Add(player); // begins tracking entity
        if (_dbContext.Save() == 0)
        {
            // handle error

        }
        return player;
    }
}

I was reading about anemic data models and rich domain models in DDD. I don't want to follow DDD completely but take rather pragmatic approach and just take some concepts out of it because clean ddd seems like an overkill.
I don't want to maintain separate DTOs just for serializing and deserializing my models.

I'm similarly pragmatic as you, and I agree with you in this situation.
KISS (keep it simple & stupid) applies here. If your application is not sufficiently large, a DTO layer abstraction is not necessary.
I have several colleagues who would disagree with this, this is an open discussion topic. Personally, I disagree with blindly implementing things just because it was needed in other projects. Before I implement something, I need to justify its existence in the current project.

I'm not planning to use any other ORM other than EF Core.

It's not so much about what you plan to use, but rather the chance of this changing in the future, and how prepared you want to be when it turns out you have to change it.

Is it fine if I will design my models like this and at the same time use them as my DTOs for serialization and deserialization?

Yes, as long as you're okay with the tighter coupling and don't start violating SRP by loading everything into your entity classes.
However, that is a separate discussion from whether or not you should use DTOs. You could just as well violate SRP on your DTO class.


JavaScript Code for image rotation on a web page How does this look for basic image rotation?  What is there to be improved here?

Three things I noticed:

1. Looking through the document with DOM methods is a slow process. There's often no way around it, and it won't crash anything here, but try to minimize it when you can. From page 183 of Stoyan Stefanov's JavaScript Patterns:

  DOM access is expensive; it's the most common bottleneck when it comes
  to JavaScript performance. This is because the DOM is usually
  implemented separately from the JavaScript engine...

  The bottom line is that DOM access should be reduced to a minimum.
  This means:

  Avoiding DOM access in loops
  Assigning DOM references to local variables and working with the locals
  Using selectors API where available
  Caching the length when iterating over HTML collections

I've done that here. I created one extra variable named images and assigned it an empty array. Then, using your counting function, I store the image references in it.

var currentImage,
    images = [],

      ...

    count = (function () {
      // Figure out how many images we have on the rotation
      var x = 0;
      while (document.getElementById('rotateImg' + (x + 1).toString())) {
        images[x] = document.getElementById('rotateImg' + x.toString());
        x++;
      }
      return images.length;
    }()),

(If you're able to change the HTML, you can avoid the loop altogether and collect these images in one line. Just add an id to the element that contains them...

  ...

...and collect them with...

var images = document.getElementById("container").getElementsByTagName("img");

But this doesn't help you if you can't change the markup.)

Now you can run hideImage and showImages on the array instead of having to walk the DOM every time.

2. When the first argument to setInterval is a string, the browser runs eval() -- an implementation of the JavaScript compiler -- on it before firing. That's also expensive and unnecessary here. From page 111 of Douglas Crockford's JavaScript: The Good Parts:

  The browser provides setTimeout and setInterval functions that can
  take string arguments or function arguments. When given string
  arguments, setTimeout and setInterval act as eval. The string
  argument form also should be avoided.

Getting rid of the quotes and the closing parentheses makes it a plain old function reference, which works fine.

So I changed this:

var interval = setInterval("rotation.advanceRotation()", 4000);

To this:

var interval = setInterval(rotation.advanceRotation, 4000);

3. Not crucial here. You can do all your variable declaration with one var statement per function. This reduces the chances of your variables being overwritten or unavailable due to hoisting. From JavaScript Patterns again:

  JavaScript enables you to have multiple var statements anywhere in a
  function, and they all act as if the variables were declared at the
  top of the function... This can lead to logical errors when you use a
  variable and then declare it further in the function.

So I took out the multiple var statements and declared them all at the top of the outer function.

The final code:

  <title>Rice Flour Cookies' image rotation test</title>

And you can see it at work here:
http://james.da.ydrea.ms/riceflourcookies/rotate.html


Find total number of paths in a matrix of 0's and 1's I am working on a program which states that:

You are given a 2-D matrix with M rows and N columns.You are initially positioned at (0,0) which is the top-left cell in the array. You are allowed to move either right or downwards. The array is filled with 1’s and 0’s. A 1 indicates that you can move through that cell, a 0 indicates that you cannot move through that cell. Return the number of paths from top-left cell to bottom-right cell.(i.e. (0,0)to(M-1,N-1)). Since answer can be large thus you have to return ans%(10^9+7).

static int count(int a[][], int i, int j) {
    int rows = a.length;
    int cols = a[0].length;
    if(a[i][j] == 0)  return 0;
    if (i == rows - 1 && j == cols - 1)
        return a[i][j];
    else if (i == rows - 1)
        return a[i][j + 1];
    else if (j == cols - 1)
        return a[i + 1][j];
    else if (a[i][j] == 1)
        return count(a, i + 1, j) + count(a, i, j + 1);
    else
        return 0;
}

How can we improve the performance of this program?

Recursion is not recommended.  First, you would likely end up repeating calculations for cells that are approached from multiple paths.  Second, the call stack would be as deep as the longest path, and you could easily crash with a StackOverflowError for a large maze.

Instead, I recommend using dynamic-programming, building a 2D table where each entry is the number of paths from the start to that location.  Every paths[r][c] is calculated exactly once, and in an order that is gentle to the cache.

public static int count(int[][] a) {
    int[][] paths = new int[a.length][a[0].length];
    if ((paths[0][0] = a[0][0]) == 0) {
        return 0;
    }
    for (int c = 1; c < a[0].length; c++) {
        paths[0][c] = a[0][c] * paths[0][c - 1];
    }
    for (int r = 1; r < a.length; r++) {
        paths[r][0] = a[r][0] * paths[r - 1][0];
        for (int c = 1; c < a[r].length; c++) {
            paths[r][c] = a[r][c] * (paths[r - 1][c] + paths[r][c - 1]);
        }
    }
    return paths[a.length - 1][a[0].length - 1];
}

You can even even build the paths one row at a time, while the maze a is being loaded. Furthermore, it is possible to modify the code to use less memory, since only the last two rows of paths are relevant at any given time. However, for simplicity, I haven't done that here: if you can afford to keep the entire a in memory, then you can probably also afford to keep another 2D array of the same size.


OOP implementation of in-memory file system I was working on this problem, where I am constructing a Object Oriented structure of file system where you can have Directories and Files. The Directories could have sub Directories and files.

I have the following layout and I am looking for feedback on improvements or if my approach is wrong.

My OOP design has three different classes Root, Directory and Files

class Root {
    constructor(name, size, writable, parent){
        this.name = name;
        this.size = size;
        this.writable = writable;
        this.parent = parent;
    }

    getName(){
        return this.name;
    };

    setName(val){
        this.name = val;
    }
}

class Directory extends Root{
    constructor(name, size, writable, parent){
        super(name, size, writable, parent);
        this.children = [];
    }

    addContent(obj){
        this.children.push(obj);
    }

    getContent(){
        return this.children;
    }
}

class File extends Root{
    constructor(name, size, writable, parent){
        super(name, size, writable, parent);
    }

    getSize(){
        return this.size;
    }
}

let root = new Root('head', 100, true);
let directory1 = new Directory('sample', 40, true, root);
let file1 = new File('test', 12, true, directory1);
let file2 = new File('test2', 14, true, directory1);
let subDirectory = new Directory('sample2', 40, true, directory1);
directory1.addContent(file1);
directory1.addContent(file2);
directory1.addContent(subDirectory);

console.log(directory1.getContent());

I'd say Root is a misnomer here, since at first glance I imagined it to represent the filesystem root (i.e. the / directory), not the root of your class tree. Moreover, instead of deriving File and Directory from a common (abstract) ancestor, I'd derive Directory from File instead, since that's what most real filesystems do anyway, i.e. they treat directories as special files.

size should not be a writable property; the File object should always calculate it based on its content. This can ensure there's no discrepancy between the stored size and the actual size of the file's content. Also, why doesn't Directory have a getSize() method? It could just call getSize() on all its children and add up the results.

You have a writable attribute on your objects, but you don't seem to be using it, e.g. in setName() you just replace the name of the object without checking if it's writable. Similarly, you have a parent argument in the constructor, but you don't use it; I'd expect the File to call addContent() on its parent automatically if the parent is specified in the constructor.

As for further features, I'd recommend making your Directory class iterable (so it can be used in for loops), as well as implementing the most common collection methods, such as forEach, has, length (i.e. the number of files in the directory), etc., instead of just exposing the array of children.


Accessing fields in LINQ query for customers with overdue balances Practicing my autodidactism using a free Kindle Unlimited book to get myself acquainted with the basics of C# in hopes of getting enough of a foundation to get into automation and I still feel a little wibbly on the best practices, etc and I just feel like I could've been a bit more elegant than what I enacted with the get; set; but I don't know what I don't know.

"C#: Learn C# in One Day and Learn it Well" in this instance. Which, I've exceeded a day by far... anyways, I've been challenging myself to use what I have learnt in prior chapters to unravel and actively enact theoretical applications in the book without any actual 'code-alongside' snippets when they rear their head.

I'm current learning about var and their usage in using LINQ queries which is fairly easy as I've prior experience in SQL but I feel like I've inelegantly accessed/set the values for what cust.Balance and cust.Name are pulling in the code with the get; set; in the snippet below and feel like there's some way to have accessed them without actively creating another set of public strings.

Is there a better way to have executed this?

    using System;
    using System.Collections.Generic;
    using System.Linq;
    using System.Text;
    using System.Threading.Tasks;

    namespace LinqTut2
    {
        class Program
        {
            static void Main(string[] args)
            {
                List<Customer> customers = new List<Customer>();

                customers.Add(new Customer("Alan", "80911291", "ABC Street", 25.60m));
                customers.Add(new Customer("Bill", "19872131", "DEF Street", -32.10m));
                customers.Add(new Customer("Carl", "29812371", "GHI Street", -12.2m));
                customers.Add(new Customer("David", "78612312", "JKL Street", 12.6m));

                var overdue =
                    from cust in customers
                    where cust.Balance < 0
                    orderby cust.Balance ascending
                    select new { cust.Name, cust.Balance };

                foreach (var cust in overdue)
                    Console.WriteLine("Name = {0}, Balance = {1}", cust.Name, cust.Balance);
                Console.ReadLine();
            }
        }

        class Customer
        {
            private string CustName;
            private string CustPhone;
            private string CustAddress;
            private decimal CustBalance;

            public decimal Balance
            {
                get
                {
                    return CustBalance;
                }
                set
                {
                    CustBalance = value;
                }
            }
            public string Name
            {
                get
                {
                    return CustName;
                }
                set
                {
                    CustName = value;
                }
            }

            public Customer(string name, string phn, string addr, decimal         bal)
            {
                CustName = name;
                CustPhone = phn;
                CustAddress = addr;
                CustBalance = bal;
            }

        }
    }

This is a pretty good attempt for someone just starting out. However, there is always a lot to learn and the books never seem to take advantage of all the latest language features.

The first thing I'm going to tell you to do is stop abbreviating everything. You don't pay extra for full naming.

I'll choose you constructor to highlight this:

public Customer(string name, string phn, string addr, decimal         bal)
{
    CustName = name;
    CustPhone = phn;
    CustAddress = addr;
    CustBalance = bal;
}

First, Customer is a great name, name is also good.

Now, string phn I'd guess at phoneNumber but it could also be the name of the Primary Health Network if this was an Australian healthcare customer. string addr - is address really that much harder to type? decimal is a great choice for the balance but bal is a terrible name and there's some extra whitespace which should be removed.

Also, your fields shouldn't start with Cust. They should aslo be camelCase. The old version of the guidelines stated this explicitly but the newer C# standards don't specify a convention for non public fields. Most people use camelCase in my experience. As a commenter has mentioned, you can use auto properties:

public string Name { get; set; }
public decimal Balance { get;set; }

You can set them directly in your constructor.

I'm assuming you're using the latest and greatest C# for the next few points. I can't remember when collection initializers were introduced but it's a while ago now.

This is a very common pattern:

List<Customer> customers = new List<Customer>();
customers.Add(new Customer("Alan", "80911291", "ABC Street", 25.60m));
customers.Add(new Customer("Bill", "19872131", "DEF Street", -32.10m));
customers.Add(new Customer("Carl", "29812371", "GHI Street", -12.2m));
customers.Add(new Customer("David", "78612312", "JKL Street", 12.6m));

It's so common, we have a construct to make it easier:

var customers = new List<Customer>
{
    new Customer("Alan", "80911291", "ABC Street", 25.60m),
    new Customer("Bill", "19872131", "DEF Street", -32.10m),
    new Customer("Carl", "29812371", "GHI Street", -12.2m),
    new Customer("David", "78612312", "JKL Street", 12.6m)
};

That's a lot less typing and a bit easier to scan IMO.

We also now have string interpolation (this one is much newer):

Console.WriteLine("Name = {0}, Balance = {1}", cust.Name, cust.Balance);

Can become:

Console.WriteLine($"Name = {cust.Name}, Balance = {cust.Balance}");

Which is a lot less scanning backwards and forwards to see what goes where.

For the sake of completeness, let's look at your naming here:

var overdue =
    from cust in customers
    where cust.Balance < 0
    orderby cust.Balance ascending
    select new { cust.Name, cust.Balance };

overdue is better as overdueCustomers.
from customer in customers is equally better than using cust which isn't a word.

You could then finally do a foreach (var overdueCustomer in overdueCustomers) to make it obvious everywhere in the code that the reason you're interested in this particular customer is that they are overdue.

Edit

As noted in a comment from Peter Taylor, overdrawnCustomers is an even better name. It doesn't have the date connotation that overdue does.