Entering pupil information I have the following code which is located within a case, in a switch statement. Its goal is to add a Pupil to a Subject using the Collections in the respective class.

I've tried to make the code as readable as possible, but I'm sure there's a better approach.

  System.out.println("Adding a Pupil...");

  String AddPupilSubj;
  do {

      UserInput.prompt("\nEnter the Subject Code to add the Pupil to, or enter 'list' to list all Subjects: ");
      AddPupilSubj = UserInput.readString();

      if(AddPupilSubj.equals("list"))
        school.listSubjects();

  } while(AddPupilSubj.isEmpty() || AddPupilSubj.equals("list"));

  Boolean found = false;

  // Locate the Subject
  for(Object obj : school.getSubjects()) {
      Subject subj = (Subject) obj;

      if(subj.getCode().equals(AddPupilSubj)) {

        found = true;

        // Get the ID of the Pupil
        UserInput.prompt("Enter Pupil ID: ");
        int id = UserInput.readInt();

        // Get the Pupil's Name
        UserInput.prompt("Enter Pupil Name: ");
        String name = UserInput.readString();

        // Get the Address of the Pupil
        UserInput.prompt("Enter Pupil Address: ");
        String address = UserInput.readString();

        String date; Boolean valid;
        do {

             // Get the Pupil's Date of Birth
            UserInput.prompt("Enter Pupil Date of Birth (dd/mm/yyyy): ");
            date = UserInput.readString();

            valid = validDOB(date);

        } while(!valid);

        // Get the Pupil's email address
        UserInput.prompt("Enter Pupil Email: ");
        String email = UserInput.readString();

        DateTime dob = DateTime.forDateOnly(Integer.parseInt(date.substring(6, 10)),
                  Integer.parseInt(date.substring(3, 5)),
                  Integer.parseInt(date.substring(0, 2)));

        DateTime admitted = DateTime.today(TimeZone.getDefault());

        Pupil Pupil = new Pupil(id, name, address, dob, email, admitted);
        if(subj.addPupil(Pupil)) {

             System.out.println(
                 String.format("\nPupil, %s, has been added to the %s Subject.",
                               Pupil.getName(),
                               subj.getName()));

        } else {

             System.out.println(
                 String.format("Pupil #%s is already in the Subject!",
                               Pupil.getID()));

        }

        break;
      }

  }

  if(!found) {
      System.out.println(
        String.format("A Subject with code, %s, doesn't exist!",
          AddPupilSubj));
  }

The function is very long.

Functions should do one thing, do it completely, and do it well. Long functions are difficult to reason about.

Move the subject and pupil data entry into separate functions to isolate the functionality, and streamline the mainline code.

I'm assuming the presence of String getSubjectCode() and Pupil getPupilInformation() methods, each containing the code currently in the mega-function.

Naming conventions

Minor, but you call an instance of the Pupil class Pupil, which is confusing at best, misleading at worst. Non-static-final variables should always begin with lowercase letters.

Use typed collections

School.getSubjects() should return a List<Subject> (or Collection). (Or at least a properly-typed array; nothing with Objects.)

Reduce the amount of code in conditionals

There's a lot of code in block that loops over the school's subjects, most of which is the block for when the subject is found.

In order to understand what happens if the subject isn't found, I have to scroll down to the end of that block, make sure I'm reading the indentation correctly (assuming the indentation is correct), only to find there's nothing there.

In cases like that, might as well do a negative test and continue, slightly "flattening" the loop's guts. (Code turned on its side is not a graph of how awesome it is ;)

That said, this functionality is available in a contains method--looping in the mainline code isn't necessary. See next.

Localize functionality

Deciding if a school has a given subject should be moved into the School class, for example:

public class School {
    // ... existing functionality elided ...

    /** Returns subject with given code, or null if not found. */
    public Subject findSubjectByCode(String code) {
        for (Subject s : subjects) {
            if (s.getCode().equals(code)) {
                return s;
            }
        }
        return null;
    }
}

The refactored mainline code

All the above turns the mainline code into the following:

System.out.println("Adding a Pupil...");

String subjectCode = getSubjectCode();
Subject schoolSubject = school.findSubjectByCode(subjectCode);
if (schoolSubject == null) {
    System.out.printf("A Subject with code, %s, doesn't exist!%n", subjectCode);
    return;
}

Pupil pupil = getPupilInformation();
if (schoolSubject.addPupil(pupil)) {
    System.out.printf("%nPupil, %s, has been added to the %s Subject.%n", pupil.getName(), subj.getName());
} else {
    System.out.printf("Pupil #%s is already in the Subject!%n", pupil.getID());
}

IMO this is more communicative, cleaner, and easier to understand.


C++ Tree Class implementation I am trying to implement a class for the Node of a Tree in C++ in order to represent an HTML structure.
It is not complete by a shot, but i'd still like an opinion.

TreeNode.h :

#ifndef TreeNode_H
#define TreeNode_H

#include <string>
#include <vector>

class TreeNode
{
    private:
        std::string textContent;
        std::string tagName;

        TreeNode *parent;

        std::vector<TreeNode *> children;

        int countNodesRec(TreeNode *root, int& count);

    public:
        TreeNode();
        TreeNode(std::string iTextContent, std::string iTagName);

        void appendChild(TreeNode *child);
        void setParent(TreeNode *parent);

        void popBackChild();
        void removeChild(int pos);

        bool hasChildren();
        bool hasParent();

        TreeNode* getParent();
        TreeNode* getChild(int pos);

        int childrenNumber();
        int grandChildrenNum();

        std::string getTextContent();
        std::string getTagName();
};

#endif

TreeNode.cpp :

#include "TreeNode.h"
#include <string>
#include <vector>
#include <iostream>

TreeNode::TreeNode() {};

TreeNode::TreeNode(std::string iTextContent, std::string iTagName) :
    textContent(iTextContent),
    tagName(iTagName),
    parent(NULL)
{}

int TreeNode::countNodesRec(TreeNode *root, int& count)
{
    TreeNode *parent = root;
    TreeNode *child = NULL;

    for(int it = 0; it < parent->childrenNumber(); it++)
    {
        child = parent->getChild(it);
        count++;
        //std::cout<<child->getTextContent()<<" Number : "<<count<<std::endl;
        if(child->childrenNumber() > 0)
        {
            countNodesRec(child, count);
        }
    }

    return count;
}

void TreeNode::appendChild(TreeNode *child)
{
    child->setParent(this);
    children.push_back(child);
}

void TreeNode::setParent(TreeNode *theParent)
{
    parent = theParent;
}

void TreeNode::popBackChild()
{
    children.pop_back();
}

void TreeNode::removeChild(int pos)
{
    if(children.size() > 0) {
        children.erase(children.begin()+ pos);
    }
    else {
        children.pop_back();
    }
}

bool TreeNode::hasChildren()
{
    if(children.size() > 0)
        return true;
    else
        return false;
}

bool TreeNode::hasParent()
{
    if(parent != NULL)
        return true;
    else
        return false;
}

TreeNode * TreeNode::getParent()
{
    return parent;
}

TreeNode* TreeNode::getChild(int pos)
{
    if(children.size() < pos)
        return NULL;
    else
        return children[pos];
}

int TreeNode::childrenNumber()
{
    return children.size();
}

int TreeNode::grandChildrenNum()
{
    int t = 0;

    if(children.size() < 1)
    {
        return 0;
    }

    countNodesRec(this, t);

    return t;
}

std::string TreeNode::getTextContent()
{
    return textContent;
}

std::string TreeNode::getTagName()
{
    return tagName;
}

Sample code :

#include <iostream>
#include <vector>
#include "TreeNode.h"

/* run this program using the console pauser or add your own getch, system("pause") or input loop */

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

    TreeNode *itr = NULL;

    TreeNode *node = new TreeNode("k", "p");
    node->appendChild(new TreeNode("test1", "testag"));
    node->appendChild(new TreeNode("test2", "testag"));
    node->appendChild(new TreeNode("test3", "testag"));

    itr = node->getChild(0);

    itr->appendChild(new TreeNode("test1a", "testtag"));
    itr->appendChild(new TreeNode("test1b", "testtag"));
    itr->getChild(0)->appendChild(new TreeNode("test1aa", "testtag"));

    itr = node->getChild(1);
    itr->appendChild(new TreeNode("test2a", "testtag"));
    itr->appendChild(new TreeNode("test2b", "testtag"));

    itr->getChild(0)->appendChild(new TreeNode("test2aa", "testtag"));

    std::cout<<node->grandChildrenNum();

    return 0;
}

Some quick comments:

Const correctness

You really should make your code const correct.

For example at least these functions could be const declared:

    bool hasChildren();
    bool hasParent();

    TreeNode* getParent();
    TreeNode* getChild(int pos);

    int childrenNumber();
    int grandChildrenNum();

    std::string getTextContent();
    std::string getTagName();

Use references to avoid copies

This:

TreeNode(std::string iTextContent, std::string iTagName);

should be:

TreeNode(const std::string& iTextContent, const std::string& iTagName);

otherwise you are creating temporary copies of the strings when you pass them in.

These functions should return by const reference:

    std::string getTextContent();
    std::string getTagName();

Like so:

    const std::string& getTextContent() const;
    const std::string& getTagName() const;

Otherwise you will be creating unnecessary memory copies each time the function is called even if you just want to look at the value.

Memory leak galore?

Your API requires that you pass in raw pointers and store them in the node. There is no documentation of the ownership of the pointers. Who is responsible for freeing them? Right now all I see is a memory leak waiting to happen.

In your main code you allocate the nodes with new but C++ is NOT a garbage collected language. You will need to pair each new with a delete to free the memory otherwise you will run out of memory faster than you can say "Holy memory management batman!".

Now, this keeping track of new/delete gets error prone very quickly so since C++11 you are recommended to use smart pointers. A.k.a. std::unique_ptr and std::shared_ptr for just about everything. See also std::make_unique and std::make_shared. There are places where you could/should use pointers, but as I gather that you are new to C++ I will not go into those detail because you can get away with just using smart pointers everywhere for now.

Also you might be interested in reading up on Resource Acquisition Is Initialisation (RAII). We ❤❤❤ RAII in C++.


Promise implementation in Python Recently I made an basic port of the JavaScript Promise construct to Python. Here's the basic code:

class Promise:
    def __init__(self, fn):
        self.value = None
        self.errors = []
        self._state = "unfullfilled"

        def resolve(val):
            if self._state == "settled":
                return
            self.value = val
            self._state = "settled"
        def reject(err):
            if self._state == "settled":
               return
            self.errors.append(err)
            self._state = "settled"
        fn(resolve, reject)

    def catch(self, fn):
        if self.errors:
            for i in self.errors:
                fn(i)
                self.errors.remove(i)
        return self

    def then(self, fn):
        if not self.errors:
            fn(self.value)
        return self

    @staticmethod
    def resolve(val):
        def g(resolve, reject):
            resolve(val)
        return Promise(g)

    @staticmethod
    def reject(val):
        def g(resolve, reject):
            reject(val)
        return Promise(g)

What could I improve in this code?
Besides that, are there any bugs in this code?

Thank you for showing your code. You really should show us how to use it, by including unit tests, a reference URL, or with comments. At a minimum the class needs a docstring.

self._state = "unfullfilled"

Typo. Consider using enum.Enum instead of str.

self.value = None

This appears to be just wrong. (Hard to tell, since no docstring describes what valid callers may do.) A fn could choose to return None, yes? The usual approach for such a sentinel is to invoke object() to create a private object, essentially an allocated memory location that other code doesn't know about. Then test equality against that. If, on the other hand, you want your API to prohibit passing None, then you'll want to add some checks for that.

self.errors.remove(i)

That's an odd way to phrase it. The usual idiom for the loop you wrote would be to .pop() the initial element. Consider switching from list to deque if lots of errors might accumulate.


Runge Kutta 2 Python ODE fluid flow I am trying to solve a set of differential equations in x,y,z, I have made simple kutta 2 code in python before but not with 3 variables. I am not sure if what i have done is an acceptable way to adjust for 3.
Here is what i have so far: (i have not entered the exact solution as i do not know it)
import numpy as np
import matplotlib.pyplot as plt
from mpl_toolkits.mplot3d import Axes3D

# Define function to compute velocity field

A = 0.2
B = 1.0
C = 0.20

def velfun(x,y,z,t):

    xvelocity =  (A*np.sin(z) +  C*np.cos(x))
    yvelocity =  (B*np.sin(x) + A*np.cos(z))
    zvelocity =  (C*np.sin(y) + B*np.cos(x))
    return xvelocity, yvelocity, zvelocity

# Set stopping time
# and the step length

Nstep = 10000
h     = 0.01

# Create arrays to store x and t values

x     = np.zeros(Nstep);
y     = np.zeros(Nstep);
z     = np.zeros(Nstep);

# Set the initial condition

x[0] = 1
y[0] = 0
z[0] = 0

# Carry out steps in a loop

for k in range(1,Nstep):

    # Provisional Euler step

    xx    = x[k-1]
    yy    = y[k-1]
    zz    = z[k-1]
    ux,uy,uz = velfun(xx,yy,zz)
    xp    = x[k-1] + h*ux
    yp    = y[k-1] + h*uy
    zp    = z[k-1] + h*uz
    # Compute velocity at provisional point

    uxp,uyp,uzp = velfun(xp,yp,zp)

    # Compute average velocity

    uxa = 0.5*(ux + uxp)
    uya = 0.5*(uy + uyp)
    uza = 0.5*(uz + uzp)

    # Make final Euler step
    # using average velocity

    x[k] = x[k-1] + h*uxa
    y[k] = y[k-1] + h*uya
    z[k] = z[k-1] + h*uza

# Exact solution

# Plot results
fig = plt.figure()
ax = plt.axes(projection='3d')
ax = plt.axes(projection='3d')
ax.scatter3D(x,y,z,'b',label='x (with RK2)')

plt.show()

This is an interesting problem but I'm afraid that my feedback will be somewhat limited:

Consider vectorizing your velfun to be represented with matrix multiplications
Your x/y/z should be combined into one matrix
You should add axis labels, and some form of colour indicator to make your graph more legible - here I've added an arbitrary "start-to-finish progress" colour
Rather than label, you likely meant to use title

The code I'm suggesting produces exactly the same results as yours. The problem is that I have no idea whether those are correct. I suggest that you re-post on https://physics.stackexchange.com/ with more details on your physics problem to get confirmation as to whether your RK is implemented correctly. You should include a picture of your graph.
Suggested
from typing import Tuple

import numpy as np
import matplotlib.pyplot as plt

A = 0.2
B = 1.0
C = 0.20

N_STEP = 10_000  # stopping time
H = 0.01         # step length

def new() -> np.ndarray:
    M = np.array([
        [0, 0, A],
        [B, 0, 0],
        [0, C, 0],
    ])
    N = np.array([
        [C, 0, 0],
        [0, 0, A],
        [B, 0, 0],
    ])

    # Define function to compute velocity field
    def velfun(p: np.ndarray) -> np.ndarray:
        return M@np.sin(p) + N@np.cos(p)

    X = np.empty((3, N_STEP))
    a = np.array((1, 0, 0), dtype=np.float64)
    X[:, 0] = a

    for k in range(1, N_STEP):
        # Provisional Euler step
        u = velfun(a)
        p = a + H*u

        # Compute velocity at provisional point
        up = velfun(p)

        # Compute average velocity
        ua = 0.5*(u + up)

        # Make final Euler step
        # using average velocity
        a += H*ua
        X[:, k] = a

    return X

def plot(x, y, z):
    # Plot results
    fig = plt.figure()
    ax = plt.axes(projection='3d')
    ax.set_title('x (with RK2)')
    ax.set_xlabel('x')
    ax.set_ylabel('y')
    ax.set_zlabel('z')
    colour = np.linspace(0, 1, len(x))

    ax.scatter3D(x, y, z, s=0.5, c=colour)

def main():
    # plot(*old())
    plot(*new())
    plt.show()

if __name__ == '__main__':
    main()

Be warned: I have very little understanding of what this code is actually doing, so my variable names are bad.


Restaurant booking subquery I have two database table name called "tablestr table" and "restbookingtable":

tablestr:

restbooking:

I want to retrieve bookingsection_id from the booking table with a few conditiona and need to remove from tablestr. I have a query for this, but I could achieve it by using subquery, which executes very slowly.

SELECT `str_id` FROM (`rest_tablestr`) WHERE str_id NOT IN (
SELECT SUBSTRING_INDEX(SUBSTRING_INDEX(t.bookingsection_id, ",", n.n), ",", -1) value FROM rest_restaurantbooking t
CROSS JOIN (
SELECT a.N + b.N * 10 + 1 n FROM (
SELECT 0 AS N
UNION ALL
SELECT 1
UNION ALL
SELECT 2
UNION ALL
SELECT 3
UNION ALL
SELECT 4
UNION ALL
SELECT 5
UNION ALL
SELECT 6
UNION ALL
SELECT 7
UNION ALL
SELECT 8
UNION ALL
SELECT 9) a , (
SELECT 0 AS N
UNION ALL
SELECT 1
UNION ALL
SELECT 2
UNION ALL
SELECT 3
UNION ALL
SELECT 4
UNION ALL
SELECT 5
UNION ALL
SELECT 6
UNION ALL
SELECT 7
UNION ALL
SELECT 8
UNION ALL SELECT 9)
b ORDER BY n ) n
WHERE n.n <= 1 + (LENGTH(t.bookingsection_id) -
LENGTH(REPLACE(t.bookingsection_id, ",", ""))) AND
t.res_id = 21 AND
t.booking_status not in ("cancelled","departed","noshow") AND
((t.bookingstart_time <= "2015-06-12 19:45:00" AND t.bookingend_time >= "2015-06-12 22:15:00") OR
(t.bookingend_time >= "2015-06-12 19:45:00" AND t.bookingend_time <= "2015-06-12 22:15:00") OR
(t.bookingstart_time >= "2015-06-12 19:45:00" AND t.bookingstart_time <= "2015-06-12 22:15:00") OR
(t.bookingstart_time >= "2015-06-12 19:45:00" AND t.bookingend_time <= "2015-06-12 22:15:00")) ) AND
`res_id` = '21' AND
`area_id` = '28' AND
`wait_table` = 'no' AND
`availability` = 'yes';

Result Set:

When I execute the subquery and main query, the separate execution time is just 0.1 sec. But while executing both, execution time is takes more than 5 to 7 seconds. How can I optimize this query?

You could start with Formatting your code so that it is readable, I got this far

SELECT `str_id`
    FROM (`rest_tablestr`)
    WHERE str_id NOT IN (
        SELECT SUBSTRING_INDEX(SUBSTRING_INDEX(t.bookingsection_id, ",", n.n), ",", -1) value
            FROM rest_restaurantbooking t
                CROSS JOIN (
                    SELECT a.N + b.N * 10 + 1 n
                        FROM (
                            SELECT 0 AS N
                            UNION ALL
                            SELECT 1
                            UNION ALL
                            SELECT 2
                            UNION ALL
                            SELECT 3
                            UNION ALL
                            SELECT 4
                            UNION ALL
                            SELECT 5
                            UNION ALL
                            SELECT 6
                            UNION ALL
                            SELECT 7
                            UNION ALL
                            SELECT 8
                            UNION ALL
                            SELECT 9) a
                            ,
                        (
                            SELECT 0 AS N
                            UNION ALL
                            SELECT 1
                            UNION ALL
                            SELECT 2
                            UNION ALL
                            SELECT 3
                            UNION ALL
                            SELECT 4
                            UNION ALL
                            SELECT 5
                            UNION ALL
                            SELECT 6
                            UNION ALL
                            SELECT 7
                            UNION ALL
                            SELECT 8
                            UNION ALL SELECT 9) b
                        ORDER BY n ) n
                WHERE n.n <= 1 + (LENGTH(t.bookingsection_id) - LENGTH(REPLACE(t.bookingsection_id, ",", "")))
                    AND t.res_id = 21
                    AND t.booking_status not in ("cancelled","departed","noshow")
                    AND ((t.bookingstart_time <= "2015-06-12 19:45:00" AND t.bookingend_time >= "2015-06-12 22:15:00")
                        OR (t.bookingend_time >= "2015-06-12 19:45:00" AND t.bookingend_time <= "2015-06-12 22:15:00")
                        OR (t.bookingstart_time >= "2015-06-12 19:45:00" AND t.bookingstart_time <= "2015-06-12 22:15:00")
                        OR (t.bookingstart_time >= "2015-06-12 19:45:00" AND t.bookingend_time <= "2015-06-12 22:15:00"))
    ) AND `res_id` = '21'
    AND `area_id` = '28'
    AND  `wait_table` = 'no'
    AND `availability` = 'yes';

Indentation in SQL is important so that you know where you are when you are debugging the code.


Construct list from a dataframe and a formula A dataframe and a right-sided formula are given:
dat <- data.frame(
  A = c("a", "b", "c"),
  B = c("x", "y", "z"),
  NotUsed = c(1, 2, 3)
)

frml <- ~ A + B + A:B

From them, I want to get this list:
# [[1]]
# [1] a b c
# Levels: a b c
#
# [[2]]
# [1] x y z
# Levels: x y z
#
# [[3]]
# [1] a:x b:y c:z
# Levels: a:x b:y c:z

Here is how I get this list:
library(lazyeval) # to use 'as.lazy' and 'lazy_eval'
tf <- terms.formula(frml)
factors <- rownames(attr(tf, "factors"))
tvars <- attr(tf, "variables")
tlabs <- attr(tf, "term.labels")
used <- lapply(eval(tvars, envir = dat), as.factor)
names(used) <- factors
lapply(tlabs, function(tlab){
  droplevels(lazy_eval(as.lazy(tlab), data = used))
})

Do you have a better way to propose?

Code Quality
Blank Lines
Generally, your code quality needs some improvement. For example, insert blank lines. So instead of
factors <- rownames(attr(tf, "factors"))
tvars <- attr(tf, "variables")
tlabs <- attr(tf, "term.labels")

I would suggest
factors <- rownames(attr(tf, "factors"))

tvars <- attr(tf, "variables")

tlabs <- attr(tf, "term.labels")

Order Within A Script or Source Code
Generally, one puts package import statements at the top of a file. Which you are not doing, and you also give no reason why you are doing it. So I would put library(lazyeval) at the top.
Further, you set variables before you use them. For example, factors <- rownames(attr(tf, "factors")).
These observations lead to the following script.
# to use 'as.lazy' and 'lazy_eval'
library(lazyeval)

dat <- data.frame(
  A = c("a", "b", "c"),
  B = c("x", "y", "z"),
  NotUsed = c(1, 2, 3))

frml <- ~ A + B + A:B
tf <- terms.formula(frml)

tvars <- attr(tf, "variables")
used <- lapply(eval(tvars, envir = dat), as.factor)
names(used) <- rownames(attr(tf, "factors"))

tlabs <- attr(tf, "term.labels")

lapply(tlabs, function(tlab){
  droplevels(lazy_eval(as.lazy(tlab), data = used))
})

Please pay attention to how a grouped the code lines.
A Better Way
Regarding a better way, for which you have asked, I would propose the following.
dat <- data.frame(A = c("a", "b", "c"),
                  B = c("x", "y", "z"),
                  NotUsed = c(1, 2, 3))

dat <- subset(x = dat, select = -NotUsed)

dat$c <- as.factor(paste(dat$A, dat$B, sep = ":"))

my.list <- lapply(dat,
                  FUN = function(column)
                  {
                      return(column)
                  })

In my view, this is a better way because it expresses your intention better. If you would have given me your code without any comments I would have a hard time explaining what it does or what it's purpose is. In addition, it does not require an additional package.
HTH!


Custom concurrent stack implementation in Java I don't want to use synchronized for sizeof method, any suggestion to implement the method thread safe?

package com.r.collection.adt;

import java.util.concurrent.atomic.AtomicReference;

public class ConcurrentStack<E> implements Stack<E>{

    @SuppressWarnings("hiding")
    private class Node<E>{

        public E item;
        public Node<E> next;

        public Node(E item){
            this.item=item;
        }

    }

    @SuppressWarnings("rawtypes")
    private AtomicReference<Node> head;

    @SuppressWarnings("rawtypes")
    ConcurrentStack(){
        head = new AtomicReference<Node>();
    }

    @SuppressWarnings("unchecked")
    @Override
    public void push(E item) {
        Node<E> newHead = new Node<E>(item);
        Node<E> headNode = null;
        do
        {
            headNode = head.get();
            newHead.next = headNode;
        }while(!head.compareAndSet(headNode, newHead));

    }

    @SuppressWarnings("unchecked")
    @Override
    public E pop() {
        Node<E> headNode = head.get();
        do
        {
            headNode = head.get();
            if(headNode == null)
                return null;
        }while(!head.compareAndSet(headNode, headNode.next));

        return headNode.item;
    }

    @SuppressWarnings("unchecked")
    @Override
    public E peek() {
        Node<E> headNode = head.get();
        if(headNode == null){
            return null;
        }

        return headNode.item;
    }

    @Override
    public boolean isEmpty() {

        return head.get() == null;
    }

    @SuppressWarnings("unchecked")
    @Override
    public synchronized int  sizeOf() {
        int size=0;
        for(Node<E> node=head.get();node != null; node=node.next){
            size++;
        }
        return size;
    }

}

The synchronization you are currently using is not even working the way you intend to: as nothing else is synchronizing on this (like the sizeOf() method) you might as well not make the method synchronized.

You could stick a size field into your Node which gets set on inserting, something along the lines of this:

do {
    headNode = head.get();
    newHead.next = headNode;
    newHead.size = (headNode == null) ? 1 : headNode.size + 1;
} while (!head.compareAndSet(headNode, newHead));

This should allow reduction of the sizeOf() method to:

public int sizeOf() {
    return head.get().size;
}


Removing everything after a certain character I was just writing code, and wanted to make a piece that removes everything after the character ? if it's found within the URL.

if (strstr($url, "?")) {
    $url = strstr($url, "?", true);
}

Can I make this any shorter? It doesn't feel right to type the "same thing" twice with strstr.

You're using the same variable for two different purpose. It would be readable with separate variables:

$urlWithoutParameters = strstr($url, "?", true);
if ($urlWithoutParameters !== FALSE) {
    // if you need additional processing
}

It might be safer to use PHP's built-in parse_url function which could help handling invalid inputs too:

  On seriously malformed URLs, parse_url() may return FALSE.

From Effective Java, 2nd edition, Item 47: Know and use the libraries:

  By using a standard library,
  you take advantage of the knowledge of the experts who wrote it and the
  experience of those who used it before you.

(It's mostly a Java book but it applies here.)


Checking if vector is normalized I tried to check if my vector struct is normalized, and I ended up with this code:

public bool IsNormalized
{
    get
    {
        double len = Length; // Math.Sqrt ((X * X) + (Y * Y) + (Z * Z)); -- X, Y, Z are in double format

        const double min = 1 - 1e-14;
        const double max = 1 + 1e-14;

        return (len >= min && len <= max);
    }
}

Is this solution OK? I read double has 15 digits precision, but 1+1E-15 give 1, so I changed to E14.

Is this all good? I need best accuracy.

This seems ok, depending on how precise you want it to be (some margin of error will always be needed since this is floating-point, but how much will depend on your needs)

Alternatively, you could check Math.Abs(1 - len) < 1e-14, but I suspect the same precision problems will remain.

I was somewhat surprised that 1 + 1e-15 equals 1, though, because doubles have 52 bits of precision, which should be more than enough to store a difference of 15 digits (-log2(1/10^15) gives me around 49.8, which is less than 52 bits).

So I double checked and 1 - 1.0e-15 does not give me 1.

class Program
{
    static void Main(string[] args)
    {
        Console.WriteLine(1 - 1.0e-15); //Does NOT print 1
        Console.Read();
    }
}

Same thing for 1 + 1.0e-15 (though this one is trickier)

class Program
{
    static void Main(string[] args)
    {
        Console.WriteLine(1 + 1.0e-15); //Prints 1
        Console.WriteLine(1 + 1.0e-15 == 1); //...but this prints False
        Console.Read();
    }
}

I suspect Console.WriteLine is only printing a limited number of digits: less than the ones double can represent.

As a final note, Sqrt(1) == 1, so instead of using the Length you could use the SquaredLength and save a (potentially expensive) Sqrt operation.


Keyboard handler for selecting radio buttons I have got my JavaScript working as seen here:
https://jsfiddle.net/yLg1qs04/3/

But how do I condense/shorten it so that it's not repeating itself so often and taking up so many lines?

$( '#mcnumber' ).keyup(function(){ //when a user types in input box
    var mcnum = this.value;
    if ( mcnum.charAt( 0 ) == '1' ) {
       $("#1mcskel1").prop("checked", "checked");
    } else if ( mcnum.charAt( 0 ) == '2' ) {
       $("#1mcskel2").prop("checked", "checked");
    } else if ( mcnum.charAt( 0 ) == '3' ) {
       $("#1mcskel3").prop("checked", "checked");
    } else if ( mcnum.charAt( 0 ) == '4' ) {
       $("#1mcskel4").prop("checked", "checked");
    } else if ( mcnum.charAt( 0 ) == '5' ) {
       $("#1mcskel5").prop("checked", "checked");
    } else if ( mcnum.charAt( 0 ) == '6' ) {
       $("#1mcskel6").prop("checked", "checked");
    } else if ( mcnum.charAt( 0 ) == '7' ) {
       $("#1mcskel7").prop("checked", "checked");
    } else if ( mcnum.charAt( 0 ) == '8' ) {
       $("#1mcskel8").prop("checked", "checked");
    } else if ( mcnum.charAt( 0 ) == '9' ) {
       $("#1mcskel9").prop("checked", "checked");
    } else if ( mcnum.charAt( 0 ) == '0' ) {
       $("#1mcskel0").prop("checked", "checked");
    }
})

If you are sure it will always have a value just use the charAt value to build the id you try to select in each if block. Something like:

$( '#mcnumber' ).keyup(function() {
    var mcnum = this.value;
    var id = "#1mcskel" + mcnum.charAt( 0 );
    $(id).prop("checked", "checked");
})

In the future just try to find patterns in the code. Every if block you have has the same code except for the number at the end of the id selector and the number you checked against charAt in the condition. And they are the same number in both places so try to merge the relevant parts of the code.

So seeing those patterns can help you write more versatile code. Of course the code above assumes you will have a definite value. Could be it needs some error handling added but hopefully this helps you get the idea.

EDIT

So I have taken what you added above and looked for a pattern, taken the relevant pieces of that pattern and shortened the code, just as I advised priorly in the answer. I would like to add as a comment that I have no context to this code only what you have shown me so I am simply applying my pattern searching/combining example here. Take a look:

$( '#mcnumber' ).keyup(function() {
    var mcnum = this.value, index = 0, id;
    for ( ; index < 3; index++) {
        id = "#" + (index + 1) + "mcskel" + mcnum.charAt( index );
        $(id).prop("checked", "checked");
    }
})

It seems odd to me that you were attaching the multiple anonymous functions to the same element and the same event so I made it a single one. As you can see the pattern I saw was you were attaching the same event (keyup) to the same element (mcnumber). So I took those relevant pieces and combined them.

Then I looked at the code inside the function and searched for a pattern. Again I saw most lines were exactly the same except for the middle line of code. What were the differences? The number at the beginning of the id selector and the number used in the charAt. OK, so can we find a pattern with those numbers? Well with the id the range goes from 1-3 and with charAt the range goes from 0-2. And looking at both those ranges they are the same length except for each number in the range id selector is +1 larger.

So lets loop over that range from 0-2 using 0-2 on the charAt and using 0-2 to build the id selector but add one every time so we get 1-3.


Repository Pattern universal application When I started learning Repository Pattern with Unity few days ago I was under impression that the main benefit of this pattern is the separation of data layer from the business layer.

In other words, if there is a need to change the way, how application stores the data, it's very easy as only one main model takes care of the communication.

This means, that if application currently saves data into a serialized XML files, it would not be very difficult to change this logic to connect to database instead.

I have found few nice demos that are also using Unit Of Work layer, which seemed very handy. Let me show you a bit of the code I have.

public class UnitOfWork : IUnitOfWork
{
    private readonly RepositoryContext _context;
    public IEmployeeRepository Employees { get; set; }

    public UnitOfWork(RepositoryContext context)
    {
        _context = context;
        Employees = new EmployeeRepository(_context);
    }

    public int Complete()
    {
        return _context.SaveChanges();
    }

    public void Dispose()
    {
        _context.Dispose();
    }
}

Main Repository Context:

public class RepositoryContext : DbContext
{
    public RepositoryContext() : base("name=RepositoryContext")
    {
    }

    public virtual DbSet<Employee> Employees { get; set; }
    public virtual DbSet<Equipment> Furniture { get; set; }
}

And here is the demo EmployeeRepository:

public class EmployeeRepository:Repository<Employee>, IEmployeeRepository
{
    public EmployeeRepository(RepositoryContext context) : base(context) { }

    public Employee GetEmployeeByName(string sName)
    {
        return MyContext.Employees.FirstOrDefault(n => n.Name == sName);
    }

    public RepositoryContext MyContext
    {
        get { return Context as RepositoryContext; }
    }
}

Employee Repository derives from a generic Repository which looks like this:

public class Repository<T> : Interfaces.Repositories.IRepository<T> where T : class
{
    protected readonly DbContext Context;

    public Repository(DbContext context)
    {
        Context = context;
    }

    public void Add(T item)
    {
        Context.Set<T>().Add(item);
    }

    public IEnumerable<T> Find(Expression<Func<T, bool>> predicate)
    {
        return Context.Set<T>().Where(predicate);
    }

    public T Get(int ID)
    {
        return Context.Set<T>().Find(ID);
    }

    public IEnumerable<T> GetAll()
    {
        return Context.Set<T>().ToList();
    }

    public void Remove(T item)
    {
        Context.Set<T>().Remove(item);
    }
}

Here is the question:

As far as my understanding goes, we are directly declaring, that under our Repository expects in it's constructor DbContext, which is afterwards used under all Add / Remove / Find functions under that particular class.

Currently this model is communicating with the database, but if I wanted (for whatever reason) to change this model to save data in the XML file, I would have to completely rewrite all my Repository classes? Or am I missing something here?

If I am wrong and it is easily doable, could anyone show me how to change the code so that we are serializing values into the XML files, please? I am trying to better understand this Repository Pattern, yet for now it's one big chaos for me.

Any help / suggestions regarding this matter would be highly appreciated.

What we are looking at is a long tutorial on Inversion of Control and dependency injection. Basically at the most simpilest form you will be looking to abstract your data layer from you application layer and provide interfaces for your IoC container to deliver to the application.

This can get quite confusing however all we have to do is remeber our application will be using the interfaces to work with the data layer and not the actual implementation.

So for your Unit of work you would resolve IUnityOfWork interface instead. Now here is a quick example I put together. It is by no means a finished or tested product (and the xml side is very fragile) but shows how this can work.

First off I am using the interface IRepository<T> to access the data layer. This can be connected database through EntityFramework or Xml through serialization. This repository is simple.

public partial interface IRepository<T>
    where T : BaseEntity
{
    T GetById(int id);

    T Insert(T item);

    T Update(T item);

    void Delete(T item);

    IQueryable<T> Entities { get; }
}

As simple interface the has the standard insert\update\delete and queryable methods. It is worth noting that I am referencing BaseEntity which is a simple entity with an public int Id { get; set; } property.

Now for Database connections I have created an implementation called EfRepository<T>

public class EfRepository<T> : IRepository<T>
    where T : BaseEntity
{

    public EfRepository(DbContext dbContext)
    {
        if (dbContext == null)
            throw new ArgumentNullException("dbContext");
        _dbContext = dbContext;
    }

    readonly DbContext _dbContext;
    DbSet<T> _entities;

    /// <summary>
    /// Gets the entities as a querably
    /// </summary>
    public IQueryable<T> Entities { get { return DbEntities; } }

    protected virtual DbSet<T> DbEntities { get { return _entities ?? (_entities = _dbContext.Set<T>()); } }

    public void Delete(T item)
    {
        DbEntities.Remove(item);

        _dbContext.SaveChanges();
    }

    public T GetById(int id)
    {
        return DbEntities.Find(id);
    }

    public T Insert(T item)
    {
        DbEntities.Add(item);
        _dbContext.SaveChanges();
        return item;
    }

    public T Update(T item)
    {
        _dbContext.SaveChanges();
        return item;
    }
}

Now this is a very simple class that connects to my Data layer via entity framework. (We will talk about the constructor shortly).

Next I have an Xml Repository named XmlRepository<T>

public class XmlRepository<T> : IRepository<T>
    where T : BaseEntity
{
    public XmlRepository(XmlContext xmlContext)
    {
        _xmlContext = xmlContext;
    }

    readonly XmlContext _xmlContext;
    IList<T> _entities;

    public IQueryable<T> Entities
    {
        get
        {
            return XmlEntities.AsQueryable();
        }
    }

    protected IList<T> XmlEntities
    {
        get
        {
            return _entities ?? (_entities = _xmlContext.Set<T>());
        }
    }

    public void Delete(T item)
    {
        if (XmlEntities.Any(i => i.Id == item.Id))
        {
            XmlEntities.Remove(item);
            _xmlContext.SaveChanges(XmlEntities);
        }
    }

    public T GetById(int id)
    {
        return Entities.FirstOrDefault(x => x.Id == id);
    }

    public T Insert(T item)
    {
        if (item.Id == default(int)) //only add if id = default(int)
        {
            var lastEntity = Entities.LastOrDefault();
            if (lastEntity != null)
                item.Id = lastEntity.Id + 1;
            else
                item.Id = 1;

            XmlEntities.Add(item);
            _xmlContext.SaveChanges(XmlEntities);
        }
        return item;
    }

    public T Update(T item)
    {
        _xmlContext.SaveChanges((IList<T>)Entities);
        return item;
    }
}

Again really simple implementation and also has another constructor reference (XmlContext).

We will get to resolution and setup of Unity shortly but given this we now have a two generic data access repositories. One for EntityFramework and one for Xml File Store.

Now back to the EfRepository it has a constructor parameter DbContext dbContext this is required as we need access to the underlying data context and connections. Now this is actually an injected class of type EfDataContext which has an initializer and a few overloads (plus dependency injection).

public class EfDataContext : DbContext
{
    public static void Initialize()
    {
        var connectionFactory = new SqlConnectionFactory();
#pragma warning disable CS0618
        Database.DefaultConnectionFactory = connectionFactory;
#pragma warning restore CS0618
    }

    public EfDataContext(ApplicationSettings applicationSettings)
        : base(applicationSettings.DataConnectionString)
    {
    }

    protected override void OnModelCreating(DbModelBuilder modelBuilder)
    {
        modelBuilder.Configurations.Add(new TestEntityMap());
        base.OnModelCreating(modelBuilder);
    }
}

The class above is really used to override the OnModelCreating method to add my Fluent configurations for entity mapping (out of scope here) and to Initialize my connection Factory. You will also note the ApplicationSettings constructor parameter.

Now for the XmlRepository has its own context that is used to load the xml files and save the entities to xml files. This is a very basic (not thread safe) example.

public partial class XmlContext
{
    public XmlContext(ApplicationSettings appSettings)
    {
        _xmlFileLocation = appSettings.DataConnectionString;
    }

    readonly string _xmlFileLocation;

    public void SaveChanges<T>(IList<T> entities)
        where T : BaseEntity
    {
        if (entities == null)
            return;

        string filePath = Path.Combine(_xmlFileLocation, $"{typeof(T).Name}.xml");
        using (var writer = new StreamWriter(filePath))
        {
            var serializer = new XmlSerializer(typeof(List<T>));
            serializer.Serialize(writer, entities);
            writer.Flush();
        }
    }

    public IList<T> Set<T>()
        where T : BaseEntity
    {
        string filePath = Path.Combine(_xmlFileLocation, $"{typeof(T).Name}.xml");
        IList<T> entities = new List<T>();
        if (File.Exists(filePath))
        {
            //XDocument doc = XDocument.Load(filePath);
            using (var stream = File.OpenRead(filePath))
            {
                var serializer = new XmlSerializer(typeof(List<T>));
                entities = serializer.Deserialize(stream) as List<T>;
            }
        }
        return entities.OrderBy(e => e.Id).ToList();
    }
}

This simple class uses the built in XmlSerializer to read\save files to disk by naming the files {Type.Name}.xml. You will also notice it has the ApplicationSettings constructor parameter.

So far we have setup our Contexts and Repositories for access both connected databases (via EntityFramework) and Xml file storage. The next step is to put it together and register everything in our Unity container. This is where the application settings come in handy.

public class ApplicationSettings
{
    [JsonProperty("dataProvider")]
    public DataProviderType DataProvider { get; set; }

    [JsonProperty("dataConnectionString")]
    public string DataConnectionString { get; set; }
}

public enum DataProviderType
{
    SqlServer,
    XmlFileStore
}

This is a simple Json text file that we can parse and load our settings (however I just did it manually in the example). Your implementation can be done any way. The important thing to note for the DataProvider.SqlServer the DataConnectionString is the database connection string and DataProvider.XmlFileStore is the root location to store the xml files.

static void Main(string[] args)
{
    var appSettings = new ApplicationSettings
    {
        DataConnectionString = @"D:\xmltest\",
        DataProvider = DataProviderType.XmlFileStore
    };

    RegisterContainer(appSettings);

    var repo = _container.Resolve<IRepository<TestEntity>>();
    var entity = new TestEntity
    {
        EntityName = "Hello Stack Oveflow"
    };

    repo.Insert(entity);
}

static UnityContainer _container;

public static void RegisterContainer(ApplicationSettings settings)
{
    _container = new UnityContainer();

    _container.RegisterInstance(settings);

    switch (settings.DataProvider)
    {
        case DataProviderType.SqlServer:
            EfDataContext.Initialize();
            _container.RegisterType<DbContext, EfDataContext>();
            _container.RegisterType(typeof(IRepository<>), typeof(EfRepository<>));

            break;

        case DataProviderType.XmlFileStore:
            _container.RegisterType<XmlContext>();
            _container.RegisterType(typeof(IRepository<>), typeof(XmlRepository<>));
            break;

        default:
            throw new Exception($"The DataProvider Type {settings.DataProvider} is unknown");
    }
}

Now the fun begins. Basically at this point we know our data access will be through the use of the generic interface IRepository<T>. We will use our ApplicationSettings class to dicate the provider and the connection string. From here we adapt our Unity Registration methods to register one instance of the other.

To walk through the RegisterContainer method we simply pass in the instance of the ApplicationSettings. We then register the instance of the ApplicationSettings (essentially a singleton). Next we then determine which type of provider we are going to load.

If the type is DataProvider.SqlServer we simply setup the EfDataContext with the Initialize() method and then register the DbContext dependency as EfDataContext (which requires the ApplicationSettings dependency already registered. Next we register the IRepository<T> dependency as EfRepository<T>. Where EfRepository<T> has the dependency DbContext which has been registered as EfDataContext.

Alternativly for the DataProvider.XmlFileStore type we register the dependency type XmlContext and then register the IRepository<T> dependency as XmlRepository<T>. Where XmlRepository<T> has the XmlContext dependency registered earlier.

Now that we have gotten this far we can answer the question. Finally moving back up to the Main method we can now call _container.Resolve<IRepository<TestEntity>>() which regardless of our data storage type gives us the same common interface.

Back to the original question

  Currently this model is communicating with the database, but if I
  wanted (for whatever reason) to change this model to save data in the
  XML file, I would have to completely rewrite all my Repository
  classes? Or am I missing something here?

Following the strategy above and leveraging your IoC container all you need to rewrite is your IRepository<T> implementation to your different storage type. For example you could easily write a JSON serializer similar to the XmlRepository<T> and XmlContext. You could change your EfRepository<T> to directly call sql queries.


Checking if an array can be rotated to equal another in JavaScript I needed to write a function in JavaScript which checks if any rotation of an array is equal to another array.

By rotation, I mean:

rotate [1, 2, 3] --> [3, 1, 2]
rotate [3, 1, 2] --> [2, 3, 1]
rotate [2, 3, 1] --> [1, 2, 3]

Here's the code for my function. It feels pretty verbose, especially since the array functions added by ES6 seem perfect for this sort of thing, yet I've used very few of them. In particular, using a label on the outer loop feels wrong.

Is there any way I can make this function shorter and/or faster?

/**
 * Taking two arrays, checks whether one may be rotated so that it equals
 * the other.
 */
function arraysEqualByRotation(first, second) {
  if (first.length !== second.length) {
    return false;
  }

  // We'll rotate the first continually and compare it to the second
  var rotatedFirst = first.slice(0);

  outer:
  for (var rotateCounter = 0; rotateCounter < rotatedFirst.length; rotateCounter++) {
    // Rotate the array
    rotatedFirst.unshift(rotatedFirst.pop());

    // Check if their elements are equal
    for (var i = 0; i < rotatedFirst.length; i++) {
      if (rotatedFirst[i] !== second[i]) {
        // Try another rotation
        continue outer;
      }
    }

    // At this stage, they're equal
    return true;
  }

  // At this stage, a matching combination was never found
  return false;
}

Example inputs and outputs:

first      | second     | result
-----------------------------------------------
[1, 2, 3]  | [1, 2, 3]  | true (same array)
[1, 2, 3]  | [3, 1, 2]  | true (rotate once)
[1, 2, 3]  | [2, 3, 1]  | true (rotate twice)
[1, 2]     | [1, 2, 3]  | false (different lengths)
[1, 2, 3]  | [1, 3, 2]  | false (no valid rotation)

I have not looked too deeply into the problem and I am guessing that there may be some function that produces a unique hash based on the sequence and independent of starting position. That would then only require that you iterate each array to create the hash and then compare to see if they match. That would give you a O(2n) solution where n is the size of one of the arrays.

If such hashing function does not exist, the brute force approch can easily be divided up and done in parallel. In javascript you would use webWorkers

There is also the approch that can use the GPU and the basic canvas 2D API but that has some limits on the size of the input arrays and the integer size of the values. Or via WebGL which is a little more flexible, but again the bottle neck is converting the arrays to an appropriate data format that the GPU can handle. The are some libraries out there that simplify the process, but you would only start to see any gains for very large arrays.

So to make some minor changes to your solution. To remove the pop and unshift  you can use the % operator to cycle past the end back to the start (see inner loop)

And the using continue to jump out of the loops is hacky to say the least. A little smarter logic and you don't need the goto in disguise.

So the function below is not an improvement in complexity, but rather a improvement of efficiency.

function canRotate(a, b) {
    var pos, count;
    const len = a.length;
    if (len === b.length) {
        pos = 0;
        while (pos < len) {
            count = 0;
            while (count < len && a[count] === b[(pos + count) % len]) {
                count ++;
            }
            if (count === len) { return true }
            pos ++;
        }
    }
    return false;
}

You could also use a regExp to do the search but again its not an improvement in complexity, just less code, and may be a lot faster in many situations (however the joins may need a separator to work on all cases)

Just join the second array and double it by add to its self, then join the first and convert to a regExp (or use String.API ) to test if the first array has a match in the second.

function canRotate(a,b){
    if(a.length === b.length) {
        const strB = b.join("");
        return new RegExp(a.join(""),"").test(strB+strB);
    }
    return false;
}

Or using a asymmetrical separator ie has two different characters.

function canRotate(a,b){
    if(a.length === b.length) {
        const jStr = "=>";
        const strB = b.join(jStr);
        return new RegExp(a.join(jStr),"").test(strB + jStr + strB);
    }
    return false;
}

Update
This answer assumes that the arrays contains positive values of the JS 32bit signed int.

Obliviously you should take care with the regExp solutions and only use it if array content can be joined without breaking the sequence uniqueness, and does not contain regExp tokens.


Grab the first 4 items that meet the condition from a data set with an unknown size I was turned down for a job after a coding take-home test, and some of the feedback I received was:

  would have preferred to see more functional programming instead of
   while loops - and the while loops that were implemented weren't ideal.

Here is the while loop (there was only one) in question:

getDetails (id) {

    return this.$http.get(this.baseUrl + (this.detailPath.replace('{ID}', id)))
    .then((json) => {

        let i = 0;
        let result = [];
        while (i < json.data.length && result.length < 4) {

            if (json.data[i].profile_path) {

                result.push(json.data[i]);

            }
            i++;

        }
        return result;

    });

}

I actually considered using a reducer function instead of the while loop, but there is no way to break out of the reducer once the result length condition (i.e. result.length < 4) is no longer true. So that would mean that very large datasets would increase computation time. I created a jsperf to test this theory.

So, my question is this: am I missing something here? Is there a better, more efficient way to do this that I am overlooking?

So that would mean that very large datasets would increase computation time.

You should ask if very large datasets are possible.  Because if they're not, clean code takes precedence over what would be a premature optimization.  All the code inside your then block can be replaced with a single line:

json.data.filter(x => x.profile_path).slice(0, 4)

If performance is truly an issue, your basic thinking is correct, but I'd just use a for loop:

let result = [];
let data = json.data
for (var i = 0; i < data.length; i++) {
  if (data[i].profile_path) result.push(data[i])
  if (result.length == 4) return result
}
// ask interviewer what the behavior should be for incomplete results...

Note that returning inside a for loop will exit the function immediately.

Fwiw, it sounds like the problem you faced was mostly about communication with the interviewer. Next time, politely probe them about what they're looking for, what their assumptions are about the problem, and so on.


Execute a list of Threads in parallel I have a list of Script that i want to execute them in parallel, i already find this way in Creating dynamic number of threads concurrently, so i use this piece of code here :

private int i;//<---------------------------------(1)
private String[] listScript = {"Script1", "Script2", "Script3", "Script4"};

private void executeQueries() throws InterruptedException {

    ExecutorService threadPool = Executors.newFixedThreadPool(listScript.length);
    // submit jobs to be executing by the pool
    for (i = 0; i < listScript.length; i++) {
        threadPool.submit(new Runnable() {
            @Override
            public void run() {
                executeQuerie();
            }
        });
        Thread.sleep(500);//<---------------------(2)
    }
    // once you've submitted your last job to the service it should be shut down
    threadPool.shutdown();
    // wait for the threads to finish if necessary
    threadPool.awaitTermination(Long.MAX_VALUE, TimeUnit.MILLISECONDS);

}

private void executeQuerie() {
    //execute scripts action
    System.out.println(listScript[i]);
}

I'm really not happy with this code, i will explain a little my code so you can understand more :

(1). I use this i variable outside the method so i can know which script i should to execute
  (2). I use Thread.sleep(500); to make sure that i execute the right script, without this i can execute the same script multiple time for example it can gives me:

Script1
Script1
Script2
Script2

My question is : how can i make sure that my scripts executed safely in parallel, should i improve this piece of code, or do you have any other safe way?

You should make i a parameter of executeQuery :

private String[] listScript = {"Script1", "Script2", "Script3", "Script4"};

private void executeQueries() throws InterruptedException {

    ExecutorService threadPool = Executors.newFixedThreadPool(listScript.length);
    // submit jobs to be executing by the pool
    for (int i = 0; i < listScript.length; i++) {
        final int j = i;
        threadPool.submit(() -> executeQuery(j));
    }
    // once you've submitted your last job to the service it should be shut down
    threadPool.shutdown();
    // wait for the threads to finish if necessary
    threadPool.awaitTermination(Long.MAX_VALUE, TimeUnit.MILLISECONDS);

}

private void executeQuery(final int index) {
    System.out.println(listScript[index]);
}

I used lambda here as it's simply less verbose. j is needed as i must be final for the closure to work ;)

Also awaitTermination(Long.MAX_VALUE... isn't a very good idea.... It's over 10 millions years you know ? ^^


Should a User class only contain attributes and no methods apart from getters/setters? I'm trying to improve my OOP code and I think my User class is becoming way too fat.

In my program a user has rights over "lists". Read, Write, Update, Delete.
So I made a User class

class User
{
protected $_id;
protected $_email;
protected $_username;
protected $_hashedPassword;

//...Various setters/getters

public function canRead(List $list){
    //Database query verifies if user has READ rights
}

public function canUpdate(List $list){
    //Database query verifies if user has UPDATE rights
}
//etc...
}

Should canRead, canUpdate, canWrite, canDelete methods be moved to another class (UserAccessCheck or something...)?
If not, should the actual SQL be moved into the List object (listCanBeReadByUser()) ?

If you have the feeling your classes are too big, you are usually right. Of course there are always different approaches how you could split classes and your idea seems to be reasonable.

So you will have some plain Value Objects (the User), some UserRepository class for getting/updating the data in the database and some kind of UserAccessCheck/PermissionManager/... class for checking the actual user rights.

In case you store your permission also in the database, you can of course extract a PermissionRepository to collect all the SQL statements in one location and don't mix your level of abstractions.

To sum this up, the decision is up to you. A class with 4 properties, some Getters and 4 can...()-methods might be suitable in one scenario and splitting it up into 5 classes might be suitable in an other scenario. In general "good" (tm) developer tend to make there code more general/abstract than necessary and wasting there time or there clients money in premature optimization. "Bad" (tm) developer will never feel the need to split a class. Find the right way in between.


Class simulating a connection among two pins Following this post and the one before that, here I am asking for a review on another piece of code: the connection among two pins. As usual, be merciless in your review. Here it goes:

public class PinConnection : IObserver<Pin>
{
    #region variables
    private Pin _from;
    private Pin _to;
    private IDisposable _fromUnsubscriber;
    private IDisposable _toUnsubscriber;
    #endregion

    #region properties
    /// <summary>
    /// Get or Set the label of this PinConnection object.
    /// </summary>
    public string Label { get; set; }

    public Pin From
    {
        get
        {
            return _from;
        }
        set
        {
            _from = value;

            if (_fromUnsubscriber != null)
            {
                _fromUnsubscriber.Dispose();
            }

            if (_from != null)
            {
                _fromUnsubscriber = _from.Subscribe(this);
                _from.Notify(_from);
            }
        }
    }

    public Pin To
    {
        get
        {
            return _to;
        }
        set
        {
            _to = value;

            if (_toUnsubscriber != null)
            {
                _toUnsubscriber.Dispose();
            }

            if (_to != null)
            {
                _toUnsubscriber = _to.Subscribe(this);
                _to.Notify(_to);
            }
        }
    }
    #endregion

    #region constructors
    public PinConnection()
    {
        Code = string.Empty;
        Label = string.Empty;
    }

    public PinConnection(Pin from)
        : this()
    {
        From = from;
    }

    public PinConnection(Pin from, Pin to)
        : this(from)
    {
        To = to;
    }
    #endregion

    public void OnCompleted()
    {
        throw new NotImplementedException();
    }

    public void OnError(Exception error)
    {
        throw error;
    }

    public void OnNext(Pin value)
    {
        if (value.Equals(From) && To != null)
        {
            To.Value = value.Value;
        }
    }
}

I'm not sure how I feel about this.

public void OnCompleted()
{
    throw new NotImplementedException();
}

In my opinion, if you're going to implement a contract, you should fulfill the contract, even if that means doing nothing at all. There's little more frustrating than getting an object that you believe implements an interface only to find out that it doesn't really implement it. Oh no, this implementation throws an exception when I call that method. Now, my nice clean code that, previously, didn't care what concrete type it was passed has to type check it's args to make sure it's not an instance of this class you've created.

Ok. So I do know how I feel about it. I hate it. I really really hate it. I'd much rather that you fulfill the contract by truly not implementing it.

/// <summary>
/// Does nothing. Not implemented for reasons.
/// </summary>
public void OnCompleted() { }

Now my code doesn't have to type check for your class anymore. Even better, I know why you've not implemented this part of the contract and decided that this method should just do nothing at all when called.

Of course, if it's not going to do anything, then we can hide it from the public interface of the concrete type by explicitly implementing the interface.

/// <summary>
/// Does nothing. Not implemented for reasons.
/// </summary>
public void IObserver.OnCompleted() { }


Simple octal reader macro in Common Lisp As an exercise, I've implemented a simple reader macro in Common Lisp (using SBCL).  It converts Octal (unsigned integers only) into numbers.

Usage:

* #z1234

668

The code:

(defun oct-string-to-number
  (string)
  "Converts an octal string to a number.  Only digits from 0 - 7 are accepted; sign or decimal point symbols will cause oct-to-number to fail"
  (setq place 1)
  (setq result 0)
  (setq digits '(#\0 #\1 #\2 #\3 #\4 #\5 #\6 #\7))
  (loop for char across (reverse string)
    do
    (setq pos (position char digits))
    (setq result (+ result (* pos place)))
    (setq place (* 8 place)))
  result)

(defun slurp-octal-digits
  (stream)
  (setq string (make-array 0 :element-type 'character :fill-pointer 0 :adjustable t))
  "Slurps all digits from 0 - 7 from a stream into a string, stopping at EOF, no data, or a non-digit character."
  (setq digits '(#\0 #\1 #\2 #\3 #\4 #\5 #\6 #\7))
  (with-output-to-string (out)
             (loop do
                   (setq char (read-char stream))
                   (setq isnum nil)
                   (if char
                   (progn
                     (setq isnum (find char digits))
                     (if isnum
                     (vector-push-extend char string)
                       (unread-char char stream))))
                   while (not (eq nil isnum))))
  string)

(defun octal-string-transformer
  (stream subchar args)
  "Slurps an octal number from stream, and converts it to a number.  Number must be an unsigned integer."
  (setq oct-string (slurp-octal-digits stream))
  (oct-string-to-number oct-string))

;; Sets #z to call octal-string-transformer, so e.g. #z1234 will evaluate to 668.  Use #z as SBCL has #o already :-)
(set-dispatch-macro-character
 #\# #\z
 #'octal-string-transformer)

I'm quite new to Common Lisp, so I'd greatly appreciate feedback on everything: formatting, style, idiom, correctness :-)

Edited to add: Actually, the formatting in the pasted code snippet is rendered oddly; indentation that is present when I edit vanishes when I submit.  So maybe be a bit gentle with feedback about the formatting ;-)

First of all a note about variables: You're using setq on previously undefined and undeclared variables. This is actually undefined behavior and in most implementations will create global dynamic/special variables. To create local variables (which you presumably intended) use let.

(defun oct-string-to-number
  (string)
  "Converts an octal string to a number.  Only digits from 0 - 7 are accepted; sign or decimal point symbols will cause oct-to-number to fail"

This whole function can be implemented as (parse-integer string :radix 8). However for the sake of learning let's go through the function anyway:

(setq digits '(#\0 #\1 #\2 #\3 #\4 #\5 #\6 #\7))
...
(setq pos (position char digits))

Iterating through the list of digits to convert a digit to an integer, is neither the most efficient nor the most succinct to do it. You can use the digit-char-p function which returns the digit's int value, if the given char is a digit and nil otherwise. (The same applies in slurp-octal-digits where you do the same thing).

(loop for char across (reverse string)
    do
    (setq pos (position char digits))
    (setq result (+ result (* pos place)))
    (setq place (* 8 place)))

Instead of reversing the string and keeping track of a place variable, you can just iterate over the string from the beginning and multiply the result by 8 each step:

(loop for char across string
    do
    (setq result (+ (* 8 result) (digit-char-p char))))

That being said, I'd rather use reduce then loop here (that is, if I weren't using parse-integer) - especially as higher order functions are one of the most useful things in lisp that a newcomer should get acquainted to. Using reduce the function would be written like this:

(defun oct-string-to-number (string)
  "Converts an octal string to a number.  Only digits from 0 - 7 are accepted; sign, non-octal digit or decimal point will signal error."
  (reduce
    (lambda (sum digit)
      (+ (* 8 sum) (digit-char-p digit 8)))
    string :initial-value 0))


Vue GitHub user lookup I have made a small Vue app which fulfills the following user stories:

Given I make a search for an existing GitHub user, then:

The user's username is shown
The user's avatar is shown
The user's most commonly used language, if available, in their repositories is shown as a 'favourite language'
The user's followers, if there are any, are shown alongside their amount

Here is a working demo.

Here is my single file component:

<template>

    <form @submit.prevent="submit">
      <input v-model="inputValue" type="text" placeholder="Enter a GitHub username...">
      <button>Go!</button>
    </form>

 Favourite Language: {{faveLang}}

 0">Followers ({{followers.length}}):

 0">

        {{follower}}

</template>

My questions:

Is this a good way of managing changing values, ie by mutating the data object? Or should this be represented in state?
How can I better structure this app? It could be split up into components but should it?
Is there a better way to make API calls like this, and should they go in the methods section?
Should the API calls be placed inside the mounted method?

This works. State works as well but you don't need it until your app gets more complex.
There are different ways to structure this app, but it doesn't necessarily make them better. I was able to grok this in just a few minutes, which is great. I was able to read it from top to bottom without jumping around the file, which is great. Your style is largely consistent throughout the app, which is great.

Points 3 and 4 are mostly the same as above. You can structure it differently, but it's not necessary for a small app like this. As the app grows you will notice some areas that can easily be refactored into their own pieces, and I think you've mentioned most of them.

Here are some ideas you can use as a learning exercise. Understanding them now will help you reason about larger apps in the future. They're not necessarily the correct way, just learning exercises:

Try using a component for the form and another for the stats. Think about how you would communicate the data between these two components.
Write unit tests to prove that your component(s) are working as expected. Currently you have a lot of logic in your component methods. Is it easy to test this logic with unit tests? Would it be easier to test this logic if it were separated from the controller?
Try extracting the API logic into another class that you can import. Currently your controller has data and methods that can be used by your template, but they are not. Think of these as 'public' properties and methods. Do they need to be public? Why would you not want to make them public?
Currently your methods are fetching data and then mutating the results. Try storing the raw data, and then use computed properties to alter the data for display. Does this approach make it easier or harder to reason about the logic? How does it affect the testability of your component?


More efficient jQuery scripting when manipulating multiple elements with multiple CSS attributes I'm relatively new to JavaScript and jQuery so go easy on me. I'm creating a website where upon jQuery document.ready a set of basic animations are performed on different divs on the HTML markup. All divs have separate IDs and I am storing all divs with same CSS property change in the same variable. Using these variables I run the function after. This code works fine but what would be a more effective manner of writing it?

<script src="jquery-1.8.3.js" type="text/javascript" ></script>

<script type="text/javascript">
    $(document).ready(function() {
        function fader(){
            var logofade = $('#portlogo, #toolslogo, #contactlogo, #portfoliolblw, #toolslblw, #contactlblw'),
                homefade = $('#homelogo'),
                homeline = $('#hline'),
                uline = $('#upline'),
                acrossline = $('#acrossline'),
                glow = $('#logoglow');

            logofade.fadeOut(0)
            homefade.fadeOut(0).delay(300).fadeIn(100)
            homeline.delay(100).animate({'width': '150px'}, 100)
            uline.delay(200).animate({'height': '41px', 'top':'-30px'}, 100)
            acrossline.delay(300).animate({'width': '825px'}, 100)
            glow.fadeOut(0).delay(600).fadeIn(600);
        }

        fader()
    });

    function logochange() { $('#homelogo').delay(300).fadeIn(100);}
    function logochange1() { $('#portlogo, #toolslogo, #contactlogo').fadeOut(100);}

    function logochange2() { $('#portlogo').delay(300).fadeIn(100);}
    function logochange3() { $('#toolslogo, #homelogo,  #contactlogo').fadeOut(100);}

    function logochange4() { $('#toolslogo').delay(300).fadeIn(100);}
    function logochange5() { $('#portlogo, #homelogo, #contactlogo').fadeOut(100);}

    function logochange6() { $('#contactlogo').delay(300).fadeIn(100);}
    function logochange7() { $('#portlogo, #homelogo, #toolslogo').fadeOut(100);}

    function homebtn() { $('#homelblw').fadeIn(0);}
    function homebtn1() { $('#homelblw').fadeOut(0);}

    function portbtn() { $('#portfoliolblw').fadeIn(0);}
    function portbtn1() { $('#portfoliolblw').fadeOut(0);}

    function toolsbtn() { $('#toolslblw').fadeIn(0);}
    function toolsbtn1() { $('#toolslblw').fadeOut(0);}

    function contactbtn() { $('#contactlblw').fadeIn(0);}
    function contactbtn1() { $('#contactlblw').fadeOut(0);}

    function hline1() {$('#hline').animate({'width': '150px'}, 100);}
    function hline2() {$('#hline').animate({'width': '0px'}, 100);}

    function pline1() {$('#pline').animate({'width': '150px'}, 100);}
    function pline2() {$('#pline').animate({'width': '0px'}, 100);}

    function tline1() {$('#tline').animate({'width': '150px'}, 100);}
    function tline2() {$('#tline').animate({'width': '0px'}, 100);}

    function cline1() {$('#cline').animate({'width': '150px'}, 100);}
    function cline2() {$('#cline').animate({'width': '0px'}, 100);}

    function upline1() {
        $('#upline').animate({
            'height': '-41px', 'top':'0px'
        }, 0).delay(100).animate({
            'height': '41px', 'top':'-30px'
        }, 100);
    }

    function acrossline1() {
        $('#acrossline').animate({
            'width': '0px'
        }, 0).delay(200).animate({
            'width': '825px'
        }, 100);
    }
</script>

I would say two things could improve this considerably:

Use CSS classes elements that share the same animations. This way you can just fetch all the elements that need to be animated with a single $(). e.g. $('.animate')
Instead of using jQuery's animation methods, use CSS transitions. This will make your code simpler, and you know you're using the browser's native animation rendering.

Here's an example:

Logo 1

Logo 2

Some text

You could simplify this even further by using a single CSS class for all elements that need animating. e.g. $('.needs-animation').addClass('animate');

Also, if there are any animations that are triggered by mousehover, you could do all the animation in CSS with the :hover pseudo-selector.

Finally, make sure the CSS transitions you use are compatible with all the browsers you're supporting.


Using an extension method for a small helper task that (ab)uses the fact that the object is not dereferenced I've recently got into an habit where I've used extension methods for giving things fluent-like properties - such as the below example or as another example entity.AssertNotNull()

public static class GenericHelper
{
    public static void AssignOrThrowIfNull<T>(this T obj, ref T assignTo, string paramName)
    {
        if (obj == null)
            throw new ArgumentNullException(paramName, "Unallowed null argument of type: " + typeof(T).FullName);
        assignTo = obj;
    }
}

This is then used as follows:

public class SmtpClient

public SmtpServerAddress Server
{
    get { return smtpServer; }
}

private SmtpServerAddress smtpServer;

public SmtpClient(SmtpServerAddress smtpServer)
{
    smtpServer.AssignToOrThrow(ref this.smtpServer, "smtpServer");
}

as opposed to

public SmtpClient(SmtpServerAddress smtpServer)
{
    if(smtpServer == null)
        throw new ArgumentNullException("smtpServer");
}

Do you think this is readable? Is it in any way preferable to regular checks everywhere? The fact that I can't really decide if I like it or not makes me nervous, so I'd like some external input!

UPDATE:

Yes, you made me open my eyes. Hopefully, you will agree with me that the following is a bit more agreeable. I used expressions because I want to include as much information as possible in the exception.

public static class Require
{
    public static void That(Expression<Func<bool>> func)
    {
        if (!func.Compile()())
            throw new ContractException("Contract not fulfilled: " + func.Body, func);
    }
}

Atleast I think this makes it a bit clearer, even though the lambda syntax is not very nice (I don't think smtpServer != null can be inferred to an Expression).

Require.That(() => smtpServer != null);
SmtpServer = smtpServer;

Since I compile the expression each time, there will surely be some performance penalty.

You might want to look into code contracts instead, which allows you to write:

Contract.Requires<ArgumentNullException>(smtpServer != null);

This expresses clearly what you are trying to do, and has the added advantage of allowing additional static checking by the code contracts tool to check the code that calls your method for possible violations of the contract.


Split string by list of separators and return used separator in the correct order I have built a function that splits a string by an list of separators and returns the used separator in order of usage.

This is the header of the function:

vector<string>* split(const string* value, const string* seperators, string* split_by)

The function is used like the following:

const string value_to_split = "000-000:000-000";
const string seperators = "-:";
string split_by;

auto parts = split(&value_to_split, &seperators, &split_by);
delete parts;

  Example:

  Input

  value => 000-000:000-000
  seperators => "-:"

  Output

  return value => [ '000', '000', '000', '000' ]
  split_by => "-:-"

This is the functions body:

#include <string>
#include <vector>

using std::string;
using std::vector;

vector<string>* split(const string* value, const string* seperators, string* split_by) {
    vector<string>* splited_string = new vector<string>;
    string current_item = "";
    for (int charIndex = 0; charIndex < value->length(); charIndex++) {
        auto current_char = (*value)[charIndex];
        auto seperator_search = seperators->find(current_char);
        if (seperator_search != string::npos) {
            auto current_seperator = (*seperators)[seperator_search];
            splited_string->push_back(current_item);
            *split_by += current_seperator;
            current_item = "";
        }
        else {
            current_item += current_char;
        }
    }
    splited_string->push_back(current_item);
    return splited_string;
};

I mainly want to know if I'm using the language correctly (I'm a C# developer learning C++). But pleace also dont hestitate to tell me about anything that you would do different.

The standard library offers strstr.
It seems more natural to look for separators in the string, rather for string characters in separators.
value and separators better be passed as const std::string& to make sure they are not attempted to be mutated.
Out parameters are typically frowned upon. Consider returning an std::pair<std::vector<string>, std::vector<char>>.


Game passive skill system I'm trying to create some easily accessible database of different skills. General idea is that every skill should do something different and be able to act on different things (ex. one skill which icreases weapon attack by 'x' [it acts on weapon] and other one which icreases chances to drop item by 'z'% [it acts on monster]).

I see it that way:

There is static database of skills accessible from everywhere
Skills have effects(methods with different return types, different parameters and calculations)
To use skill we add it's effect in particular calculations;

Example: If we want to increase drop chance of monster by 'z'% then, in place where we calculate that drop chance (in this case in Monster class), lets say (Monster.DropChance = 0.5f) we add skill effect : after skill implementation (Monster.DropChance = 0.5f + SkillDatabase.IncreaseDropChance.Effect (this))

I came up with a code like this:

public static class SkillData   // Static class acting as database for keeping skills in one place
{
    // Names of skills
    public enum Name
    {
        IncreaseDropChance = 1,
        IncreaseWeaponAtk = 2
    }

    // T - return type of skill effect, U - type of object on which effect works
    public class Skill<T, U>
    {
        public int Level { get; set; }
        public float ExperienceCost { get; set; }
        public Name SName;

        // Delegate for creating unique effects for each skill
        public delegate T EffectDel (U onWhat);
        public EffectDel Effect;

        public Skill (Name name)
        {
            SName = name;
        }
    }

    // Custom dictionary for generic skill
    public class SkillDictionary
    {
        private Dictionary<Name, object> _dict = new Dictionary<Name, object>();

        public void Add<T, U>(Name key, Skill<T, U> skill)
        {
            _dict.Add(key, skill);
        }

        public Skill<T, U> GetValue<T, U>(Name key)
        {
            return _dict[key] as Skill<T,U>;
        }
    }

    // Static for easy access
    public static SkillDictionary DB = new SkillDictionary ();

    static SkillData ()
    {
        // Here I want to create skills one after another and add them to DB
        // This is example skill creation
        Skill <float, Player> IncrWpnAtk = new Skill<float, Player> (SName.IncreaseWeaponAtk);
        IncrWpnAtk.Level = 5;
        // Let's say we want to increase weapon attack by 100% * skillLvl
        IncrWpnAtk.Effect = (player) => { float additionalAtk = player.Eq.Weapon.Atk * IncrWpnAtk.Level;
                                          return additionalAtk; };
        DB.Add <float, Player> (Name.IncreaseWeaponAtk, IncrWpnAtk);
    }
}

Now in weapon attack calculation we add SkillData.DB.GetValue<float, Player> (IncreaseWeaponAtk).Effect (player) .
Yeah... It's terrible. Load of text for such a tiny effect. It's my second time using delegates and third time creating generic classes, I just don't know any other solution for creating same class with different methods.

How would you solve it guys? Do you know any way to maintain organized, easily accessible database of skills? Or maby this approach is generaly wrong?

Hmmm,

Well I think the way you are going about it, while functionally correct won't scale well.

Ideally you want to try more of an aggregation. Off the top of my head I would approach this like:

interface Skill
{
   string Name{get;}
   double value{get;}
}

now with this base object you can write the rest of your code doing calculations based on your skills value.

As for the modifying, well nothing else needs to know if a Skill is modified to use it.
To handle that I would make a ModifiedSkill agnostic from everything else:

public interface ModifiedSkill : Skill
{
    IEnumerable<Modifier> Modifiers { get; }

    double BaseValue { get; }
    double Value { get; }

}

Now, what is a modifier you ask? well:

public interface Modifier
{
    string Name { get; }
    string Cause { get; }
    double Apply(double skill);
    Boolean Active { get; set; }
}

So that is all the interfaces you need to cover pretty much all eventualities, allowing for some potentially cool things down the line like timed modifiers or conditional modifiers or whatever.

Now, how to actually implement this?

Public class ModifiedSkillImpl : ModifiedSkill
    {
        private readonly List<Modifier> _modifiers;
        private readonly double _baseValue;

        public ModifiedSkillImpl(string name, double baseValue, IEnumerable<Modifier> modifiers)
        {
        if (name == null) throw new ArgumentNullException("name");
        if (modifiers == null) throw new ArgumentNullException("modifiers");
        Name = name;
        _baseValue = baseValue;
        _modifiers = modifiers.ToList();
    }

    public string Name { get; private set; }

    public IEnumerable<Modifier> Modifiers
    {
        get
        {
            return _modifiers;
        }
    }

    public double BaseValue
    {
        get
        {
            return _baseValue;
        }
    }

    public double Value
    {
        get
        {
            return Modifiers.Aggregate(_baseValue, (current, modifier) => modifier.Apply(current));
        }
    }
}

Now, this is only one example of course, you could do a lot of alternate skill implementations.
The final step would be i suppose to create PoisonModifier/ItchyLeftEarModifier etc
and then apply them with some effect like +5% -20% etc, or whatever.

This separation also means that while most things will be using the Skill.Value which in the case of a modified skill will be the modified version some special something could check if the skill is a Modified skill, if so cast it and use the base stat instead!

(one final note)
I was a bit wasteful with the Value aggregation in ModifiedSkillImpl, tbh I would probably have a private variable for modified Skill and only recalculate if the modifiers had changed since last call but you know, this is just a quick POC.


Algorithm to transform a string according to occurrence count of each character I have this algorithm that counts the frequency of character occurrence in a string, and outputs a new string based on that.

For example,

input = 'aabbcccaaa'
output = 'a5b2c2'

Here is my implementation in python

def compression(string):
    string = string.lower()
    freq_count = {}

    for index, char in enumerate(string):
        if char not in freq_count:
            freq_count[char] = 1
        else:
            freq_count[char] += 1

    return_string = ''
    for key in freq_count:
        return_string += key + str(freq_count[key])
    print(return_string)

    return return_string

compression('aabccccaaa')

My question is, am I making this algorithm less efficient by using dict to memoize values.

Also, I know that creating a new string takes up memory allocation, so is there a way to improve on that?

def compression(string):

Naming can be hard, but it is important to get right.  If I were to call compression('abcd'), I would expect the result length to be at-most the length of the input string.  "compression" doesn't really describe what is happening within the function.  So what exactly is your function doing?  From your description:

  I have this algorithm that counts the frequency of character occurrence in a string, and outputs a new string based on that.

A lot of nice verbs in that description you can use for a function name (serialize_frequencies?).

string = string.lower()

Does case-sensitivity have anything to do with your stated goals of calculating and serializing the frequency of characters?  It depends on the context in which this function is used.  Case-sensitivity isn't always required.  If you really want to provide a mechanism for case-insensitive frequency generation, consider a toggle parameter or another function that transforms the input then calls this function.

serialize_frequencies(string, case_insensitive = False):
    if case_insensitive:
        string = string.lower()

    freq_count = {}

    for index, char in enumerate(string):
        if char not in freq_count:
            freq_count[char] = 1
        else:
            freq_count[char] += 1

A function that performs a single operation is simpler to understand, test, and reuse.  Don't be afraid to break functions up into suitable logical parts and parameterize.

enumerate is a nice utility when you need to iterate through a sequence but also want to know the index.  Since you don't need the index, you can just iterate through the string itself.

    for char in string:
        if char not in freq_count:
            freq_count[char] = 1
        else:
            freq_count[char] += 1

With that said, Python's collections includes a dictionary sub-class to count frequencies (Counter).

    freq_count = Counter(string)

    return_string = ''
    for key in freq_count:
        return_string += key + str(freq_count[key])

If you want to iterate a dictionary by its key-value pair, Python's built-in dictionary includes the method items().

    return_string = ''
    for key, value in freq_count.items():
        return_string += key + str(value)

You can write the loop that appends each pair using the string method join.

    return_string = ''.join(k+str(v) for k,v in freq_count.items())

    print(return_string)

Debugging artifact?

  My question is, am I making this algorithm less efficient by using dict to memoize values.

No.  But as 200_success has noted, calling compression('abcd') might result in 'a1b1c1d1' or 'c1d1b1a1' depending on the implementation.  Ordering for the built-in dictionary is arbitrary and could change between implementations, versions, or possibly application executions.  If ordering matters, then you should use a sorted container (OrderedDict, SortedDict) or manually sort the resulting dictionary before serializing.


Elegantly check for null before method call How do I elegantly check for null before calling a method on an object? This is how I do it right now:

var title = document.querySelector('title');
title = title ? title.text : '';

Null Object pattern would be nice in this case but I don't own the document.querySelector code.

You can do this:

var title = document.querySelector('title') || {text:''};

then just use title.text in-line.

Alternatively, you could use this one-liner:

var title = (document.querySelector('title') || {text:''}).text;


SpamAssassin spam analyzer script in PHP based on sa-learn command I wrote a small script to analyse spam messages that are spam false negative; meaning that they are spam messages in nature but that happened to be in your INBOX folder because your spam filter failed to detect it correctly (I personally use SpamAssassin and unfortunately it rarely happens).

The goal is to have a chance to analyse spam messages that are put in spam folder manually by running a script in cron jobs.

Obviously, all spam messages will be analysed regardless of which email client you're using (Thunderbird, Kaiten Mail or Roundcube), because all you need is to have a message moved in spam folder.

Example of my well working PHP script:

<?php
// Check if there is messages in SPAM folder that don't have [SPAM] lable (e.g.: manually moved from INBOX)
exec("grep -L '\[SPAM\]' /home/domainexample.ru/Maildir/.Junk/cur/* 2> /dev/null", $spam_messages);

if (!empty($spam_messages)) {

    $sa_learn = '/usr/bin/sa-learn --spam';
    foreach ($spam_messages as $spam_message) {
        //Learn a message that we believe is spam
        $sa_learn .= ' ' . $spam_message;
        $marked_as_spam = file_get_contents($spam_message);
        // Adding [SPAM] flag to message's subject of analyzed message
        $marked_as_spam = str_replace("Subject:", "Subject: [SPAM] ", $marked_as_spam);
        file_put_contents($spam_message, utf8_encode($marked_as_spam));
    }
    // Logging the results
    $sa_learn .= ' > ' . '/home/domainexample.ru/.spamassassin/logs/' . date('d.m.Y-G:i') . '_analyzer.log';
    //Executing analyzer in background
    shell_exec("nohup $sa_learn 2> /dev/null & echo $!");
    // Cleaning cached messages in Dovecot
    shell_exec("rm -f /var/lib/dovecot/index/domainexample.ru/.Junk/*");
}
?>

I would like to have (in case there are) few ideas for improving this already working PHP script but most importantly, I would like to learn of how to write the same script using PERL or/and BASH scripting.

Could you please suggest some ideas for improving current PHP script, along with providing pure and fully working examples of the same scenarios in PERL or/and BASH?

Here's an untested Bash version:

#!/usr/bin/env bash
set -o errexit -o noclobber -o nounset

while IFS= read -r -u 9 path
do
    /usr/bin/sa-learn --spam "$path" \
        > "/home/domainexample.ru/.spamassassin/logs/$(date +%d.%m.%Y-%G:%S)_analyzer.log" \
        2>&1 &
    sed -i -e 's/^Subject:/Subject: [SPAM] /' "$path"
    rm "$path"
done 9< <(grep -FL '[SPAM]' /home/domainexample.ru/Maildir/.Junk/cur/* 2> /dev/null)

Some changes from the original:

Uses grep's -F option to speed up search.
Runs sa-learn repeatedly instead of once to avoid having to accumulate the data. Shouldn't slow down the execution since the processes are backgrounded.
Deletes files as soon as they are processed, to avoid reprocessing if the previous run failed.


Find power of a number Looking for optimization, smart tips and verification of complexity:  O (log (base 2) power).

NOTE: System.out.println("Expected 16, Actual: " + Power.pow(2, 7)); is a typo. It correctly returns 128.

/**
 * Find power of a number.
 *
 * Complexity: O (log (base 2) power)
 */
public final class Power {

    private Power() { }

    /**
     * Finds the power of the input number.
     * @param x     the number whose power needs to be found
     * @param pow   the power
     * @return      the value of the number raised to the power.
     */
    public static double pow(double x, int pow) {
        if (x == 0) return 1;
        return pow > 0 ? getPositivePower(x, pow) : 1 / getPositivePower(x,  -pow);
    }

    private static double getPositivePower(double x, int pow) {
            assert x != 0;
        if (pow == 0) return 1;

        int currentPow = 1;
        double value = x;
        while (currentPow <= pow/2) {
            value = value * value;
            currentPow = currentPow * 2;
        }

        return value * getPositivePower(x, pow - currentPow);
    }

    public static void main(String[] args) {
        System.out.println("Expected 6.25, Actual: " + Power.pow(2.5, 2));
        System.out.println("Expected 16, Actual: " + Power.pow(2, 7));
        System.out.println("Expected 0.25, Actual: " + Power.pow(2, -2));
        System.out.println("Expected -27, Actual: " + Power.pow(-3, 3));
    }
}

This can get worse than \$\operatorname{O}(\log \verb~pow~)\$. [Remark: in \$\operatorname{O}\$-notation, the base of an logarithm is irrelevant, since it represents only a multiplication with a constant]

Let \$\verb~pow~ = 2^n - 1\$ for some \$n\$. Then you're computing the following powers:

\$1, 2, 4,\dots, 2^{n-1}\$, followed by a function call for \$\verb~pow~ = (2^n-1) - 2^{n-1} = 2^{n-1} - 1\$;
\$1, 2, 4,\dots, 2^{n-2}\$, followed by a function call for \$\verb~pow~ = (2^{n-1}-1) - 2^{n-2} = 2^{n-2} - 1\$;
\$1, 2, 4,\dots, 2^{n-3}\$, followed by a function call for \$\verb~pow~ = (2^{n-2}-1) - 2^{n-3} = 2^{n-3} - 1\$;
...

All in all, \$n + (n-1) + ... + 1 = n(n+1)/2\$ calls of the inner loop. In terms of pow, this is \$\operatorname{O}(\log^2 \verb~pow~)\$.

Instead, observe pow as a binary number. I don't do Java, so I'll just sketch this in C, which is quite like it.

double px = x; // current power of x
double result = 1;
while (pow > 0) {
    if (pow % 2) result *= px;
    pow /= 2;
    px *= px;
}

So, you are observing \$x, x^2, x^4, x^8,...\$ and multiplying with them if the corresponding binary digit of pow is equal to \$1\$. This has \$\operatorname{O}(\log \verb~pow~)\$ steps.

By the way, I don't think \$2^7\$ is expected to be \$16\$. ;-)


JavaScript for toggling appearance I have a dedicated stylesheet <style id="dynamicStyles"></style> for my page's dynamic styles. They are controlled by this JavaScript:

function style(className, sty) {
    var sheet = document.getElementById('dynamicStyles').sheet;
    sheet.insertRule('.' + className + ' {' + sty + '}');
}

// No validation, do not pass tainted content.
function unstyle(className, sty) {
    var sheet = document.getElementById('dynamicStyles').sheet;
    var rules = sheet.cssRules;
    var regex = new RegExp('\\.' + className);
    for (var i = 0; i < rules.length; i++) {
        if (rules[i].cssText.match(regex)) {
            sheet.deleteRule(i);
            return;
        }
    }
}

function styleRule(className, that, sty) {
    if (that.checked) {
        style(className, sty);
    } else {
        unstyle(className);
    }
}

This is called, for example, like this:

<input type=checkbox onClick="styleRule('someClass', this, 'border: 1px #a00 dashed')">

There are maybe 10 or 20 inputs with similar code. The number of elements to be styled is large, hundreds or thousands, so I don't want to re-style each of them.

In addition to a general style review, I'm looking to see if there is a more elegant way to do the lookup in unstyle.

Moar built-ins... MOAR (and a look-up table)!

It is great that you are using the built-in insertRule method for style sheets here:

sheet.insertRule('.' + className + ' {' + sty + '}');

It is not great that you aren't using the built in deleteRule for style sheets here:

function unstyle(className, sty) {

Reading the docs, it says that the deleteRule method accepts an index. This index comes from the second parameter of insertRule which you are currently not using. That means all you need to do is start keeping track of the indexes.

In order to keep track of the indexes, class names, and CSS rules, simply create a look up table and use a counter for the indexes:

var ruleToIndexTable = {};
var indexCounter = 0;

Now, when you are styling a rule, simply add it to the table:

indexCounter++;
ruleToIndexTable[rule] = indexCounter;

Note that this will require you to compute the rule somehow, which are technically already doing in the earlier part of the style function:

sheet.insertRule('.' + className + ' {' + sty + '}');

We'll be using this logic again later, so it'll be best to extract this into it's own function computeRule(className, sty) and have both your style and unstyle functions take the single rule as a parameter.

Then later, in your unstyle function, you can simply use the lookup table to find out which table to delete:

function unstyle(rule) {
    sheet.removeRule(ruleToIndexTable[rule]);
}

This assumes that unstyle can access sheet, too, which it now should be able to so you don't have to get the sheet element over and over again (which is inefficient).

With these changes, your code is much more efficient now. With the addition of this look-up table and the new use of the removeRule function, your code now runs in \$O(1)\$ time (I think).