Haskell Sudoku solver using bactracking This solves Sudoku by using backtracking. Accepts one grid where 0 represents a missing number. Here are some examples: http://norvig.com/easy50.txt

The program is quite slow. Input is stdin.

import Data.Char (isDigit)
import Data.List (intersperse, delete)

printGrid [] = []
printGrid x = (intersperse ' ' $ take 9 x) ++ "\n" ++ (printGrid $ drop 9 x)

solve x = if isSolved x then x
          else
                firstSolved cands
             where
                firstSolved [] = []
                firstSolved (c:cs) = if isSolved trial then trial else firstSolved cs
                        where
                            trial = solve c
                cands = candidates x

isSolved x = (length x == 81) && (not $ elem '0' x) && (isValidCandidate x)

candidates x = filter isValidCandidate [(takeWhile (/='0') x) ++ z ++ (tail $ dropWhile (/='0') x) | z <- map show [1..9]]

isValidCandidate x = not $ any (isInvalidCand x) [0..80]

isInvalidCand x i = (x !! i /= '0') &&
                    any (elem ic) [row, col, unit]
    where
        ix = mod i 9
        iy = quot i 9
        ic = x !! i
        row = [x !! a | a <- [9*iy..(9*iy + 8)], a /= i]
        col = [x !! a | a <- [ix, (ix + 9)..(ix + 72)], a /= i]
        icx = (quot ix 3) * 3
        icy = (quot iy 3) * 3
        si = (9*icy) + icx
        unit = map (x !!) (delete i [si, si + 1, si + 2, si + 9, si + 10, si + 11, si + 18, si + 19, si + 20])

main :: IO ()
main = interact $ printGrid . solve . filter isDigit

Since you said you were looking for suggestions for coding style I will focus on that.

Comment your code! Preferably with Haddock style comments. Even if you aren't planning on sharing your code with others, having comments will still help you if you decide to come back to a project after a break.

Haddock is a system that will automatically generate documentation based on your comments. Formatting your comments to work with Haddock basically just mean adding a | as the first character of your top level comments. For example:

-- | Pretty prints a Sudoku board encoded as a list.
printGrid :: [Char]
printGrid [] = ...

All top level symbols should have type signatures. Types in Haskell are super expressive and often convey as much meaning or more than variable names.
Line wrap your code at 80 or 100 characters (80 seems to be more popular). Not everybody has as wide a monitor as you, and limiting the length of your lines forces you to break up code into logical units.
Haskell programs tend to have lots of short variable names. This makes sense for general functions like map where you can't really be specific about the values even if you wanted to:

map f (x:xs) = f x : map f xs

However, the more specific your code gets the less applicable (and more confusing) short variable names get. For example, the argument to all of your functions is x. This a missed opportunity to give the reader more information on what the parameter of this function actual is.
You are using a String to represent your Sudoku board. This is fine, but it makes your type signatures unhelpful. You can fix this by defining an alias for your board type:

-- | An entry in a Sudoku board is either the number as a
-- character, or '0' to indicate an unknown value.
type Entry = Char

-- | A Sudoku board is a list of Sudoku entries read from
-- left to right, top to bottom.
type Board = [Entry]

You have several take n/drop n and takeWhile/dropWhile pairs in your code. You can combine these into a single statement with splitAt and span.

front = take 9 x
back  = drop 9 x
(front, back) = splitAt 9 x

front = takeWhile (/= '0') x
back  = dropWhile (/= '0') x
(front, back) = span (/= '0') x

You have a lot of if-then-else blocks in your code. Often times these can be more succinctly expressed with pattern guards. For example, you can simplify your solve function as follows:

-- | Takes a board with possibly unknown entries and returns
-- Just a board with all of the entries containing valid numbers,
-- or Nothing if there is no solution.
solve :: Board -> Maybe Board
solve board | isSolved board = Just board
            | otherwise      = listToMaybe solutions
    where solutions = filter isSolved $ map solve (candidates board)

Also, your solve method returns the empty list if no solution can be found. This is confusing because the empty list is also a valid board. When your function has the possibility of failing, it is best to wrap the result in a Maybe or another type that is capable of representing an error.
Your candidates function is doing a lot of list traversals with your use of ++ and takeWhile and dropWhile. You can get rid of most of these traversals by using recursion to find the first element of the list and replacing it inline.

-- | Generate a list of possible solutions to a Sudoku board by
-- replacing the first unknown entry with all possible values.
candidates board = filter isValidCandidate [ replaceFirst guess board
                                           | guess <- ['1'..'9']]

-- | Replace the first unknown entry in a Sudoku board with
-- the given entry.
replaceFirst :: Entry -> Board -> Board
replaceFirst _ []       = []
replaceFirst a ('0':xs) = a : xs
replaceFirst a (x:xs)   = x : replaceFirst a xs

Another note about candidates,  you can incorporate the filter as part of the comprehension you are already doing:

candidates board = [ candidate | guess <- ['1'..'9']
                               , candidate <- [replace guess board]
                               , isValidCandidate candidate]

I stared at the isInvalidCand function for a while and I'm still having trouble groking all that is happening. Some comments and some better naming would go a long way in improving the readability this function.


Cleaner way to handle double pointer in C++ BST? I have an implementation for my first binary search tree in C++. I was wondering if there was some cleaner way to avoid using the double pointer in the way I have my code setup? Such as on one line I have:

(*node)->left = insert(&((*node)->left),value);

Which seems a bit "messy", but it almost seems necessary for the way I have implemented the BST. Maybe I am possibly missing a way I can change the syntax slightly to achieve the same result? I understand that I can have a double pointer as a parameter for my functions, but I have been told that it is not the standard in C++. I have my code posted below, along with how I am testing it.I am trying to prepare for technical interviews so any feedback is welcome.

#include<stdio.h>
#include<stdlib.h>
#include<iostream>
struct Node
{
    int data;
    Node *left, *right;
};

// A utility function to create a new BST node
Node* newNode(int data)
{
    Node *temp =  new Node();
    temp->data = data;
    temp->left = NULL;
    temp->right = NULL;
    return temp;
}

// A utility function to do inorder traversal of BST
void inorder(Node **root)
{
    if (*root != NULL)
    {
        inorder(&((*root)->left));
        printf("%d \n", (*root)->data);
        inorder(&((*root)->right));
    }
}

/* A utility function to insert a new node with given key in BST */
Node* insert(Node** node, int value)
{
    if(*node==NULL){
        return newNode(value);
    }
    if((*node)->data > value){
        (*node)->left = insert(&((*node)->left),value);
    }
    else if((*node)->data < value){
        (*node)->right = insert(&((*node)->right),value);
    }
    return *node;
}

// Driver Program to test above functions
int main()
{
    /* Let us create following BST
              50
           /     \
          30      70
         /  \    /  \
       20   40  60   80 */
    Node *root = NULL;
    root = insert(&root, 50);
    insert(&root, 30);
    insert(&root, 20);
    insert(&root, 40);
    insert(&root, 70);
    insert(&root, 60);
    insert(&root, 80);

    // print inoder traversal of the BST
    inorder(&root);

    return 0;
}

EDIT:
By changing " ** " in the parameters of the function to "*&" was able to make code much easier to read, with the same functionality.

#include<stdio.h>
#include<stdlib.h>
#include<iostream>
struct Node
{
    int data;
    Node *left, *right;
};

// A utility function to create a new BST node
Node* newNode(int data)
{
    Node *temp =  new Node();
    temp->data = data;
    temp->left = NULL;
    temp->right = NULL;
    return temp;
}

// A utility function to do inorder traversal of BST
void inorder(Node *&root)
{
    if (root != NULL)
    {
        inorder(((root)->left));
        printf("%d \n", (root)->data);
        inorder(((root)->right));
    }
}

/* A utility function to insert a new node with given key in BST */
Node* insert(Node*& node, int value)
{
    if(node==NULL){
        return newNode(value);
    }
    if((node)->data > value){
        node->left = insert(((node)->left),value);
    }
    else if((node)->data < value){
        (node)->right = insert(((node)->right),value);
    }
    return node;
}

// Driver Program to test above functions
int main()
{
    /* following BST
              50
           /     \
          30      70
         /  \    /  \
       20   40  60   80 */
    Node *root = NULL;
    root = insert(root, 50);
    insert(root, 30);
    insert(root, 20);
    insert(root, 40);
    insert(root, 70);
    insert(root, 60);
    insert(root, 80);

    // print inoder traversal of the BST
    inorder(root);

    return 0;
}

If you're trying to learn C++, you should get comfortable with constructors and destructors — they're what C++ is all about!

struct Node
{
    int data;
    Node *left, *right;
};

// A utility function to create a new BST node
Node* newNode(int data)
{
    Node *temp =  new Node();
    temp->data = data;
    temp->left = NULL;
    temp->right = NULL;
    return temp;
}

That's C style. C++ style would be:

struct Node {
    int data_;
    Node *left_ = nullptr;
    Node *right_ = nullptr;

    explicit Node(int data) : data_(data) {}
};

Then when you want a new heap-allocated node, you don't call newNode(42) — you call new Node(42)! Or, a good habit you should get into: call std::make_unique<Node>(42) to get back a smart pointer.

Notice that I added sigils to your data members (data_ etc) to distinguish them from non-member variables; and I declared no more than one variable per line to reduce reader confusion.

void inorder(Node *&root)
{
    if (root != NULL)
    {
        inorder(((root)->left));
        printf("%d \n", (root)->data);
        inorder(((root)->right));
    }
}

Several things weird here. First, you have a bunch of unnecessary parentheses. (root) is the same thing as root. Second, you're passing root by non-const reference, even though you don't intend to modify it. Third, very minor nit, you're using C-style NULL instead of nullptr. Fourth, why do you print a space before the newline? Fixed up:

void inorder(const Node *root)
{
    if (root != nullptr) {
        inorder(root->left);
        printf("%d\n", root->data);
        inorder(root->right);
    }
}

Remember to remove the redundant parentheses in places like insert(((node)->right),value). It's much easier to read as insert(node->right, value).


Calculating the point of intersection of two parabolas I recently wrote a function to calculate the intersection point(s) of two parabolas.  Working it out on paper was a nightmare, but I eventually did it (I think it's correct), and ended up with:

\$ x = \frac{\sqrt{c_2 - c_1 + \frac{b_1^2}{4a_1} - \frac{b_2^2}{4a_2} + E}}{\sqrt{a_2 - a_1}} - \frac{b1 - b2}{2(a1 - a2)} \$

with

\$ E = \frac{b_1^2a_2^3(a_2 - 2a_1^2) - a_1^3b_2^2(a_1 - 2a_2^2) - 2a_1^2a_2^2b_2(b_2 - b_1)}{(4a_1^2a_2^2)(a_1 - a_2)^2} \$

For any two parabolas \$ a_1x^2 + b_1x + c_1 = 0\$ and \$a_2x^2 + b_2x + c_2 = 0\$.

That can't be the fastest way to do it, I must be missing something. Does anyone know of any better ways I could implement parabola to parabola intersection point calculations?

The function I wrote is below:

def parabola_to_parabola_poi(a1, b1, c1, a2, b2, c2):
    """
    Calculate the intersection point(s) of two parabolas.
    """
    a1_sqrd, a2_sqrd = a1*a1, a2*a2
    b1_sqrd, b2_sqrd = b1*b1, b2*b2
    c1_sqrd, c2_sqrd = c1*c1, c2*c2

    E1 = b1_sqrd*a2_sqrd*a2*(a2 - 2*a1_sqrd)
    E2 = a1_sqrd*a1*b2_sqrd*(a1 - 2*a2_sqrd)
    E3 = 2*a1_sqrd*a2_sqrd*b2*(b2 - b1)
    E4 = (4*a1_sqrd*a2_sqrd)*(a1 - a2)*(a1 - a2)

    E = (E1 - E2 - E3)/E4

    F = (c2 - c1 + b1_sqrd/(4*a1) - b2_sqrd/(4*a2) + E)/(a2 - a1)
    G = (b1 - b2)/(2*(a1 - a2))

    if not F:
        px = math.sqrt(F) - G
        py = a1*px*px + b1*px + c1
        return [
            (px, py)
            ]
    elif F < 0:
        return []
    sqrt_F = math.sqrt(F)

    px1 = sqrt_F - G
    px2 = -sqrt_F - G

    py1 = a1*px1*px1 + b1*px1 + c1
    py2 = a1*px2*px2 + b1*px2 + c1

    return [
        (px1, py1),
        (px2, py2)
        ]

Well it seems as though I SIGNIFICANTLY overcomplicated the equation for some reason. I just did the equation it a second time and I got something much nicer:

For any two parabolas \$ ax^2 + bx + c = 0\$ and \$dx^2 + ex + f = 0\$, we get:

\$
ax^2 - dx^2 + bx - ex + c - f = 0 \\
x^2(a - d) + x(b - e) = f - c
\$

Now it's just a matter of completing the square.

\$
x^2(a - d) + x(b - e) + \frac{(b - e)^2}{4(a - d)} = f - c + \frac{(b - e)^2}{4(a - d)} \\
(x\sqrt{a - d} + \frac{b - e}{2\sqrt{a - d}})^2 = f - c + \frac{(b - e)^2}{4(a - d)} \\
(a - d)(x + \frac{b - e}{2(a - d)})^2 = f - c + \frac{(b - e)^2}{4(a - d)} \\
x + \frac{b - e}{2(a - d)} = \sqrt{\frac{f - c + \frac{(b - e)^2}{a - d}}{a - d}} \\
x = \sqrt{\frac{f - c + \frac{(b - e)^2}{a - d}}{a - d}} - \frac{b - e}{2(a - d)} \\
\$

I don't know how I ended up with the monster I originally posted, but this seems much more reasonable.


Creating a binary search tree I want you to pick my code apart and give me some feedback on how I could make it better or simpler. Although, agree that generics should be used, instead of integers, let's punt on generics as a review for now.

public class CreateBinarySearchTree {

    private TreeNode root;

    public CreateBinarySearchTree() {
    }

    /**
     * Will create a BST imperative on order of elements in the array
     */
    public CreateBinarySearchTree(int[] a) {
        this();
        for (int i : a) {
            add(i);
        }
    }

    private static class TreeNode {
        TreeNode left;
        int item;
        TreeNode right;

        TreeNode(TreeNode left, int item, TreeNode right) {
            this.left = left;
            this.right = right;
            this.item = item;
        }
    }

    public void add(int item) {
        if (root == null) {
            root = new TreeNode(null, item, null);
            return;
        }

        TreeNode node = root;
        while (true) {
            if (item < node.item) {
                if (node.left == null) {
                    node.left = new TreeNode(null, item, null);
                    break;
                }
                node = node.left;
            } else {
                if (node.right == null) {
                    node.right = new TreeNode(null, item, null);
                    break;
                }
                node = node.right;
            }
        }
    }
}

This class is too basic to be useful. You can only add nodes to it, but you don't provide a way to access those nodes. As such this is something incomplete.

Make it testable

At the minimum, make it testable. For example by implementing a toString method:

@Override
public String toString() {
    return toString(root);
}

private String toString(TreeNode node) {
    if (node == null) {
        return null;
    }
    return "[" + toString(node.left) + "," + node.item + "," + toString(node.right) + "]";
}

Now we can write some tests to verify it's actually working:

@Test
public void test345() {
    CreateBinarySearchTree tree = new CreateBinarySearchTree();
    tree.add(3);
    tree.add(4);
    tree.add(5);
    assertEquals("[null,3,[null,4,[null,5,null]]]", tree.toString());
}

@Test
public void test453() {
    CreateBinarySearchTree tree = new CreateBinarySearchTree();
    tree.add(4);
    tree.add(5);
    tree.add(3);
    assertEquals("[[null,3,null],4,[null,5,null]]", tree.toString());
}

NOT a Binary Search Tree

The implementation violates a requirement of a Binary Search Tree: it will add duplicate elements. Let's expose this bug by adding a unit test:

@Test
public void testNoDups() {
    CreateBinarySearchTree tree = new CreateBinarySearchTree();
    tree.add(4, 4);
    tree.add(4);
    assertEquals("[null,4,null]", tree.toString());
}

Easy enough to fix, by adding an else in the main loop:

    while (true) {
        if (item < node.item) {
            if (node.left == null) {
                node.left = new TreeNode(null, item, null);
                break;
            }
            node = node.left;
        } else if (item > node.item) {
            if (node.right == null) {
                node.right = new TreeNode(null, item, null);
                break;
            }
            node = node.right;
        } else {
            break;
        }
    }

Recursive add

Instead of a while loop, it would be more elegant to implement adding a node using recursion:

public void add(int item) {
    if (root == null) {
        root = new TreeNode(null, item, null);
    } else {
        add(root, item);
    }
}

public void add(TreeNode node, int item) {
    if (item < node.item) {
        if (node.left == null) {
            node.left = new TreeNode(null, item, null);
        } else {
            add(node.left, item);
        }
    } else if (item > node.item) {
        if (node.right == null) {
            node.right = new TreeNode(null, item, null);
        } else {
            add(node.right, item);
        }
    }
}

Minor things

public CreateBinarySearchTree() {}

public CreateBinarySearchTree(int[] a) {
    this();
    for (int i : a) {
        add(i);
    }
}

The constructor that takes an array calls this(), but since that other constructor does nothing, this is pointless. The variable names a and i are poor, it would be better to rename them to arr or items, and item, respectively.

As others have pointed out, it would make sense to move the adding logic outside of the constructor. In fact, how about removing all the constructors, and using this new method instead to add elements:

public void add(int... items) {
    for (int item : items) {
        add(item);
    }
}


Automate the Boring Stuff Chapter 8 Sandwich Maker I am currently learning Python by working my way through Automate the Boring Stuff with Python. This project is from Chapter 8 which is focused on validating input by using the PyInputPlus module. Whenever I finish, I like to look the problem up and compare to other's solutions to see what I could have done better. However, I can only find a couple of links to this problem. Here is the problem:

Sandwich Maker
Write a program that asks users for their sandwich preferences. The program should use PyInputPlus to ensure that they enter valid input, such as:

Using inputMenu() for a bread type: wheat, white, or sourdough.
Using inputMenu() for a protein type: chicken, turkey, ham, or tofu.
Using inputYesNo() to ask if they want cheese. If so, using inputMenu() to ask for a cheese type: cheddar, Swiss, or mozzarella.
Using inputYesNo() to ask if they want mayo, mustard, lettuce, or tomato.
Using inputInt() to ask how many sandwiches they want. Make sure this number is 1 or more.

Come up with prices for each of these options, and have your program display a total cost after the user enters their selection.

That being said, I'd like to hear what I can do better and how I can improve this code:
import pyinputplus as pyip

# store a dictionary of ingredients and their respective prices
optionPrices = {'white' : 2.00,
                'wheat' : 2.50,
                'sour dough' : 3.00,
                'chicken' : 2.50,
                'turkey' : 2.25,
                'ham' : 1.75,
                'tofu' : 4.00,
                'cheddar' : 1.00,
                'swiss' : 1.25,
                'mozzarella' : 2.00,
                'mayo' : 0.25,
                'mustard' : 0.25,
                'lettuce' : 0.30,
                'tomato' : 0.50
                }

customerOrder = [] # a list to store the current order
extras = ['mayo', 'mustard', 'lettuce', 'tomato']
sandwichTotal = 0.0

# ask the user for bread choice and append to order list
breadChoice = pyip.inputMenu(['white', 'wheat', 'sour dough'], 'Please choose your bread:\n', lettered=True)
customerOrder.append(breadChoice)

# ask the user for protein choice and append to order list
proteinChoice = pyip.inputMenu(['chicken', 'turkey', 'ham', 'tofu'], 'Please choose your protein:\n', lettered=True)
customerOrder.append(proteinChoice)

# ask if the user wants cheese, and if so, record cheese choice
cheeseResponse = pyip.inputYesNo('Would you like cheese?\n')
if cheeseResponse == 'yes':
    cheeseChoice = pyip.inputMenu(['cheddar', 'swiss', 'mozzarella'], 'Please choose your cheese:\n', lettered=True)
    customerOrder.append(cheeseChoice)
else:
    cheeseChoice = ''

# loop through 'extras' and ask if customer wants each one. If so, append it the order
choice = ''
for i in extras:
    choice = pyip.inputYesNo('Would you like ' + i +'?\n')
    if choice == 'yes':
        customerOrder.append(i)
    else:
        choice = ''

# get the number of sandwiches from the customer
numSandwiches = pyip.inputInt('How many sandwiches would you like?\n', min=1)

print('\nYour order: ')
# check if the item exists in the options, and get the price for each sandwich
for item in customerOrder:
    if item in optionPrices.keys():
        sandwichTotal += optionPrices.get(item)
        print('\t' + item + ' - $' + str(optionPrices.get(item)))

print('Total for your sandwich: $' + str('{:0.2f}'.format(sandwichTotal))) # per sandwhich total
print('Total for your order: (' + str(numSandwiches) + ' sandwiches @ $' +
      str('{:0.2f}'.format(sandwichTotal)) + ' each): ')
print('$' + str('{:0.2f}'.format(sandwichTotal * numSandwiches))) # give the total price of sandwiches

I know that there are likely a lot of things I could do better to optimize or make the code easier to read, but there are some things I just haven't learned yet. I realize this probably isn't the most efficient way to solve this problem and that is why I'd like to get some feedback.

PEP8
The "PEP 8 -- Style Guide for Python Code" has many recommendations on how programs should be written, for maximum understandability between programmers.
Variable Names
The one that you violate the most relates to variable naming.  PEP8 recommends snake_case for all variables.  So customerOrder should be customer_order, sandwichTotal should be sandwich_total, etc.
Constants
Constants, things which never change, should be named using UPPERCASE or UPPERCASE_WITH_UNDERSCORES.  Therefore, optionPrices should be OPTION_PRICES.
White Space
PEP-8 recommends no space between a diction key and the :.
Encapsulate Data into Containers
optionPrices contains breads, proteins, cheeses, and extras.  This seems like it is stuffing too much -- well, stuffings -- into one container.
I might write:
BREAD_PRICE = {'white':2.00, 'wheat': 2.50, 'sour dough': 3.00}
PROTEIN_PRICE = {'chicken': 2.50, 'turkey': 2.25, 'ham': 1.75, 'tofu': 4.00}
CHEESE_PRICE = {'cheddar': 1.00, 'swiss' : 1.25, 'mozzarella' : 2.00}
EXTRA_PRICE = {'mayo': 0.25, 'mustard': 0.25, 'lettuce': 0.30, 'tomato': 0.50}

With the bread, protein, and cheese options, you could now write a common function:
def get_sandwich_choice(category, options):
    prompt = "Please choose your " + category + ":\n"
    choices = list(options.keys())
    choice = pyip.inputMenu(choices, prompt, lettered=True)
    return choice

bread_choice = get_sandwich_choice('bread', BREAD_PRICE)
protein_choice = get_sandwich_choice('protein', PROTEIN_PRICE)
...

The price containers are all really the same kind of things, just in groups.  So one might instead ...
PRICE = {'bread': {'white':2.00, 'wheat': 2.50, 'sour dough': 3.00},
         'protein': {'chicken': 2.50, 'turkey': 2.25, 'ham': 1.75, 'tofu': 4.00},
         'cheese': {'cheddar': 1.00, 'swiss' : 1.25, 'mozzarella' : 2.00},
         'extras': {'mayo': 0.25, 'mustard': 0.25, 'lettuce': 0.30, 'tomato': 0.50},
         }

and the choices could be fetched with:
bread_choice = get_sandwich_choice('bread', PRICE['bread'])
protein_choice = get_sandwich_choice('protein', PRICE['protein'])
...

You could expand this further by adding more data.  bread is choice, protein is a choice, cheese is an optional choice, extras are optionals.  Based on the category type, you could call the appropriate user input functions.  The prompt for each category could be customized, and so on.  To add this extra data, I'd look into @dataclass.
Are all things in customerOrder the same?  The bread is kind of unique; you need two slices for the sandwich.  The extras might eventually have options like "light on the mayo" or "heavy on the mustard", where as you can't ask for extra bread.  So it might make more sense keeping them separate.  bread_choice, protein_choice, cheese_choice, and a list of extras.  This also helps our cost, since the prices are in different containers.
def sandwich_cost(bread, protein, cheese, extras):
    cost = PRICE['bread'][bread] + PRICE['protein'][protein]
    if cheese:
        cost += PRICE['cheese'][cheese]
    for extra in extras:
        cost += PRICE['extra'][extra]

sandwich_total = sandwich_cost(bread_choice, protein_choice, cheese_choice, extras)

Again, a @dataclass may be useful in construction a sandwich object.
By having a separate function for computing the sandwich cost, you make your code testable.  For example, you could check:
assert sandwich_cost('white', 'tofu', 'cheddar', ['tomato']) == 7.50
assert sandwich_cost('white', 'tofu', '', []) == 6.00

Look into unittest for better ways of writing test code.
Main Guard
It is a good habit to move your code into functions, and call it from inside a "main guard" at the end of the code.  Eg)
import pyinputplus as pyip

... helper functions here ...

def main():
    ... code here ...

if __name__ == '__main__':
    main()

Summary
Lots of ways to improve this code.  I've touched on a few.  Experiment, and once you've got something working, post a new followup question for further advice.


Making VBA generated dynamic SQL statements safe against injection I need to use SQL extensively in VBA macros that I write. Since the DB is from our enterprise application, I use vendor's library for this.

I generate dynamic queries, sometimes with many JOINs and UNIONs, and the queries can be quite complex.

The procedure that I coded screams "I'm open to SQL injection". The macros using this function are all internal but I need to make it safe anyway.

How can I make the below code only run SELECT statements and safe against injections?

Please note that it is not possible to make views for all possible SELECT statements and use them instead, according to the parameters, one or more DB's need to be queried and UNION'd sometimes.

How can I safely query an arbitrary number of columns with an arbitrary number of constraints from an arbitrary number of tables in an arbitrary number of DBs?

'***************************************************************************'
'Evaluates and executes the passed query and returns the results            '
'Parameters:                                                                '
'   Query: The main query body with optional placeholders like #0 starting  '
'               with 0                                                      '
'   Args : Optional arguments to replace placeholders                       '
'Returns:                                                                   '
'   The resulting recordset                                                 '
'***************************************************************************'
Public Function DirectQuery( _
    Query As String, _
    ParamArray Args() As Variant _
) As NetRS
'Declarations **************************************************************'
Dim ReturnRs As NetRS
Dim Params As Variant
'***************************************************************************'
    On Error GoTo ErrorHandler
    'This code shouldn't be used
    MsgBox "This code is dangerous. Don't copy-paste and use it."
    Exit Function
    Params = Args
    Set ReturnRs = 3rdPartyObject.NewRecordset(DBConnection)
    ReturnRs.Open EvaluateQuery(Query, Params)
    Set DirectQuery = ReturnRs
    Set ReturnRs = Nothing
Exit Function
ErrorHandler:
End Function
'***************************************************************************'

'***************************************************************************'
'Evaluates the arguments in a query string and returns the formed query     '
'Parameters:                                                                '
'   Query: The main query body with optional placeholders like #0 starting  '
'               with 0                                                      '
'   Args : Optional arguments to replace placeholders                       '
'Returns:                                                                   '
'   The formed query string                                                 '
'***************************************************************************'
Public Function EvaluateQuery( _
    Query As String, _
    ParamArray Args() As Variant _
) As String
'Declarations **************************************************************'
Dim i As Integer
Dim Params As Variant
'***************************************************************************'
    On Error GoTo ErrorHandler
    Params = Args
    EvaluateQuery = Query
    For i = LBound(Params(0)) To UBound(Params(0))
        EvaluateQuery = Replace(EvaluateQuery, "#" & i, Params(0)(i))
    Next i
Exit Function
ErrorHandler:
    EvaluateQuery = ""
End Function
'***************************************************************************'

Error Handling / Execution Flow: Beware of Raptors
I realize you haven't implemented it yet, but you've set yourself up for some twisted execution flow:
    Exit Function 'fixed indentation
ErrorHandler:
    EvaluateQuery = ""
End Function

When you do implement error handling, you'll likely be cleaning up resources (closing recordsets, connections, etc.) - you'll want to do that in case of error, and in normal execution paths too.
I'd remove Exit Function and rework the ErrorHandler: part:
ErrorHandler:
    'todo: cleanup
    If Err.Number <> 0 Then
        'todo: handle errors
    End If
End Function

This way you'll avoid introducing another label (Cleanup:?) and a GoTo Cleanup, and raptors won't get you.

SQL Injection
I know only 1 way of preventing SQL injection: let the server handle the parameters. I don't know what 3rd party you're using nor what a NetRs is (why not use ADODB?), but the code in EvaluateQuery seems to be concatenating the parameters into the query.
This means a String parameter would have to be surrounded by single quotes, and that a string that says "Robert'); DROP TABLE Students;--" would be concatenated - and executed as such. Thus you are correct, this code is screaming SQL injection vulnerability.
Don't even try "sanitizing" the strings before concatenating them into your query. It's the server's job to deal with the parameters. With ADODB you'd be sending the server a query like:
"SELECT SomeField FROM SomeTable WHERE SomeOtherField = ?"

Where ? is replaced by the server, by its parameter value - no need to worry about single quotes around string parameters, the parameter itself has all the information needed for the server to know how to handle.
On top of being more secure, such queries are more performant: if you execute them in a loop, the server receives the same query everytime, only with different parameters - it reuses the same query plan every time and only calculates it once. If you send the parameter values embedded in the query, the server receives a different query every time, and has to recalculate a query plan each time.
See MSDN for more info on parameterized ADODB queries.


Varnish - HttpClient Exception Handling I'm consuming some messages from a rabbit queue. Within the consumer/processor I need to invalidate a varnish cache and requeue the message if the purge failed.
I'm using a local purge proxy.
The consumer is a Dropwizard microservice but I'm using a BasicHttpClient because JerseyClient doesn't allow me (AFAIK) to set PURGE as http verb.

I'm trying to improve my error handling.. Is throwing an IllegalStateException fine or should I improve it?
Thanks

public void invalidateVarnishCache( String level, String idUrl ) throws IOException {
    try{
        String xPurgeRegex = level+"/"+idUrl+"$";
        Header header = new BasicHeader( "X-Purge-Regex", xPurgeRegex );
        BasicHttpRequest purgeRequest = new BasicHttpRequest("PURGE", "/" );
        purgeRequest.setHeader(header);

        LOGGER.log( LogEntries.VarnPurge, level, idUrl, host, xPurgeRegex );

        response = httpClient.execute(host, purgeRequest);

        entity = response.getEntity();

        int statusCode = response.getStatusLine().getStatusCode();

        if( !is2XXResponse( statusCode ) ){

            int respLength = entity.getContent().available();
            byte[] errorResp = new byte[ respLength ];
            entity.getContent().read(errorResp, 0, respLength);

            LOGGER.log( LogEntries.VarnPurgeErrResp, statusCode, new String(errorResp) );

            throw new IllegalStateException("Varnish local proxy returned response with status code "+statusCode+" from Varnish backend. Consumed message will be requeued.");
                      /////////////////////
        }else{
            LOGGER.log( LogEntries.VarnPurgeSuccessResp, statusCode );
        }

        EntityUtils.consume(entity);

    }catch( IOException ex ){
        LOGGER.log( LogEntries.VarnPurgeException, ex );
        throw ex;
    }
}

This is the consumer..

public MessageResult apply(UpdateMessage t) {

    if( !agentValidator.validMessage( t ) ){
        return MessageResult.ACK; // discard message
    }

    String urlRequest = t.getLink();

    String[] splitted = urlRequest.split( "/", -1 );
    String level = splitted[2];
    String idUrl = splitted[3];

    try{
        // Varnish cache purge. It will requeue if purge fails for any reason.
        try{
            pcsConnector.purgeVarnishBeforePcsRequest( level, idUrl );
        }catch(Exception e){

            LOGGER.log( LogEntries.PurgeError, host, urlRequest, e );
            return MessageResult.NACK_REQUEUE;
        }

        // retrieve resource from ohter microservice
        try{
            response = connector.getLatestStateOfResource( urlRequest );
        }catch(Exception e){

            LOGGER.log( LogEntries.DBServiceError, pcsHost, urlRequest, e );
            return MessageResult.NACK_REQUEUE;
        }

        // .....
        // more logic
        // ....

    }catch(Exception e){

        LOGGER.log( LogEntries.MsgProcessingError, t.toString(), e );
        return MessageResult.NACK_REQUEUE;
    }

    LOGGER.log( LogEntries.MsgConsumedSuccess, t.toString() );

    return MessageResult.ACK; // SUCCESS. Remove message from queue.

}

The Consumer

The nested try-catch blocks look weird and useless. Since purgeVarnishBeforePcsRequest and getLatestStateOfResource throw exceptions, they may be caught in the outer catch block.

try {
  pcsConnector.purgeVarnishBeforePcsRequest( level, idUrl );
  response = connector.getLatestStateOfResource( urlRequest );
  // .....
  return MessageResult.ACK;
} catch (Exception e) {
  // logger
  return MessageResult.NACK_REQUEUE;
}

By the way, catching of Exception type might be a bad coding practice, but I'd not discuss it here because the context is not enough to judge.

invalidateVarnishCache

The only purpose of the try-catch block in the original code is to trace the IOException using a hypothetical commented logger and then re-throw it. I doubt that the block itself is useful, because exactly the same exception is re-thrown and must be handled somewhere among the callers and even probably also logged by them. Wouldn't it be redundant?

Concerning the IllegalStateException, the problem is that it is a RuntimeException and the caller can miss to handle it properly, with hardly predictable consequences.

I don't know what is the range of response statuses that this sort of proxy can return, but your implementation does not make distinction between 4xx and 5xx codes, which may lead either to a client-side error (your request is invalid, 4xx) or to a server-side error (5xx), which is semantically close to an I/O exception. For the latter, I can suggest to wrap the response message into a IOexception and re-throw it. For the former, it's still up to you to decide how to handle it, probably just logging is enough.


Sending mail using nodemailer in Clojure I'm very new to Clojure/ClojureScript and have a question regarding a "proper" way to express something.

This code sends a mail using nodemailer on nodejs:

(defn send-mail [some-id]
  (try
    (let [clj-props (:mail (config/config-file)) ; get the mail server cofiguration
          mail-address (mail-adress-from-props clj-props) ; get the mail address
          mail (clj->js (create-mail mail-address some-id)) ; create a mail to send
          transporter (.createTransport nodemailer (clj->js clj-props))] ; create a transporter using nodemailer
      (.sendMail transporter mail on-mail-sent)) ; send mail using nodemailer
    (catch js/Error e
      (logging/error (.toString e)))))

It is working, but looks completely out of place in ClojureScript. I woud like for a more idiomatic way to do this, so any feedback is welcome.

In my opinion the code looks great. What in particular looks out of place? When dealing with interop, there is no need to get hung up style, you are going to have to match the model exposed to you. I don't think there is any more proper way than as you have done.

Just as a matter of style I would make these suggestions based on my personal tastes:

Lean toward functions that take arguments over looking things up from the environment. Consider passing the result of create-mail to send-mail as an argument. Consider passing on-mail-sent to send-mail as an argument. Your functions will be more flexible this way.
Consider putting the try catch log around the call to send-mail, instead of inside it. Functions tend to be more reusable when they allow the caller to handle errors either by returning a result or possibly throwing.
Don't bother with comments that repeat the code.
nodemailer appears to be a global; should this be called like (js/nodemailer.createTransport)? Or is it a def?
clj-props doesn't convey meaning. Consider mail-config?
What does mail-adress-from-props do? is it just a keyword lookup? If so consider just using the direct lookup instead.


Circular linked list of integers I'm looking for a review paying special attention as to whether the code can be optimized in the form of number of variables used, especially for the delete() method.

public class circularLL {

    Node start;
    Node last;

    circularLL()
    {
        start=null;
        last=null;
    }

    class Node
    {
        int data;
        Node next;

        Node(int datanew)
        {
            data=datanew;
        }
        public void setNext(Node n)
        {
            next=n;
        }
        public Node getNext()
        {
            return next;
        }
        public void setData(int datanew)
        {
            data=datanew;
        }
        public int getData()
        {
            return data;
        }
    }

    public void insert(int datanew)
    {
        Node p=new Node(datanew);
        if(start==null)
        {
            start=p;
            last=p;
            p.setNext(p);
        }
        else
        {
            last.setNext(p);
            last=last.getNext();
            last.setNext(start);
        }
    }
    public void delete(int datanew)
    {
        Node temp=start;
        Node temp1=start;
        Node previous=null;
        Node previous1=null;
        if(last.getData()==datanew)
        {
            while(temp1.getData()!=datanew)
            {
                previous1=temp1;
                temp1=temp1.getNext();
            }
            previous1.setNext(temp.getNext());
            last=previous1;
        }
        while(temp.getData()!=datanew)
        {
            previous=temp;
            temp=temp.getNext();
        }
        previous.setNext(temp.getNext());
    }

    public void display() {
        int count = 0;

        if(start == null) {
            System.out.println("\n List is empty !!");
        } else {
            Node temp = start;

            while(temp.getNext() != last) {
                System.out.println("count("+count+") , node value="+temp.getData());
                count++;
                temp = temp.getNext();
            }
            System.out.println("count("+count+") , node value="+temp.getData());
        }
    }

    public static void main(String args[])
    {
        circularLL ll=new circularLL();
        ll.insert(40);
        ll.insert(20);
        ll.insert(22);
        ll.insert(15);
        ll.insert(16);
        ll.insert(35);
        ll.insert(40);
        ll.insert(38);
        ll.delete(22);
        ll.display();
    }
}

I'm not a big algorithms/data structures guy, but the only thing that caught my attention was the delete function. Is there a reason that delete uses data to identify the node, as opposed to just taking in the Node object you want to delete? It seems like that could get hairy if two nodes have the same data (which happens in main).

Besides that, there are a few comments I have in general about the code. Most of the following is fairly standard code format type stuff, but I think it's worth saying it as many times as possible.

Formatting

It's fairly minor, it helps to make sure there's an empty line between methods.

Also, I know some feel differently, but my preference about operators is that they have spaces on either side. data=datanew vs data = datanew.

Naming

In Java, the convention is that class names start with a capital letter, so circularLL should be CircularLL. Even better, it should really be something more descriptive like CircularLinkedList.

Besides that, names of methods, members, variables, etc should follow lowerCamelCase. So, datanew becomes dataNew, but I would personally call it newData.

In delete, avoid variable names like temp1 and previous1. With the number of variables and how they're named, you're starting to enter mental-juggling territory. What I mean by that is that as I read through the code, I have to keep reading back through the code to remember what a variable does because its name isn't descriptive enough.

Etc

I don't have too much else to say, but I would consider breaking this code out into two or three files. I would make a new class with main to separate the linked list from what's using it, and I might put Node in its own class.

Sorry I couldn't help out more with the efficiency/number of variables issue, but you don't seem to be doing anything too crazy, and like I said before, data structures and algorithms aren't my strong suit. Just thought I'd throw in my two cents.


Notifying view controller of changes to any of five types of models I have a swift 3 project with a code that's look like very generic, but I don't know if I can reduce a method like this one :

var pages: [Page] // List of page object

func RefreshList(){
        guard let currentIndex = currentIndex else { return }

        if let listTableViewController: listTableViewController<ModelA> = pages[currentIndex].viewController as? listTableViewController<ModelA> {
            listTableViewController.didRefreshNodesList()
        }
        else if let listTableViewController: listTableViewController<ModelB> = pages[currentIndex].viewController as? listTableViewController<ModelB> {
            listTableViewController.didRefreshNodesList()
        }
        else if let listTableViewController: listTableViewController<ModelC> = pages[currentIndex].viewController as? listTableViewController<ModelC> {
            listTableViewController.didRefreshNodesList()
        }
        else if let listTableViewController: listTableViewController<ModelD> = pages[currentIndex].viewController as? listTableViewController<ModelD> {
            listTableViewController.didRefreshNodesList()
        }
        else if let listTableViewController: listTableViewController<ModelE> = pages[currentIndex].viewController as? listTableViewController<ModelE> {
            listTableViewController.didRefreshNodesList()
        }
    }

The only things that change is the Model(LETTER) is there a way to reduce that ?

The type annotation in

if let listTableViewController: listTableViewController<ModelA> = pages[currentIndex].viewController as? listTableViewController<ModelA>

is not needed because the type is automatically inferred from the
expression on the right-hand side:

if let listTableViewController = pages[currentIndex].viewController as? listTableViewController<ModelA>

According to the Swift API Design Guidelines

  Names of types and protocols are UpperCamelCase. Everything else is lowerCamelCase.

Therefore it should be func refreshList() and ListTableViewController.

To simplify your code, define a protocol for the common methods:

protocol RefreshController {
    func didRefreshNodesList()
    // ...
}

and make ListTableViewController conform to that protocol.
Then you can reduce it to a single test

if let refreshController = pages[currentIndex].viewController as? RefreshController {
    refreshController.didRefreshNodesList()
}


Applying gaussian blur on RGBA images I designed the code and it works quite well for images of lower res. However my program takes a lot of time and ram to display higher res images (occupies 2GB RAM for 4k images and takes 20 minutes). This program currently only processes files in .png format.

#------------------------------------
import matplotlib.pyplot as plt
import skimage
from skimage import color
import numpy as np
#------------------------------------
x=plt.imread("s2.png")    #the file name
#x=color.rgba2rgb(x)
y=x
n=int(input())     #"n" is the level of blurring(typical values=5,10,20,50)
le=y.shape[0]
bd=y.shape[1]
#------------------------------------
def frame(y):
    y2=np.ones(shape=(le+(2*n),bd+(2*n)))
    for i in range(0,le):
        for j in range(0,bd):
            y2[(i+n),(j+n)]=y[i,j]
    return(y2)
#------------------------------------
#print((frame(y[:,:,0])).shape)

p=(2*n)+1
def kernel(p):

    k=np.zeros(shape=(p,p))
    for i in range(0,p):
        for j in range(0,p):
            k[i,j]=1.00005**(-(n-i)**2-(n-j)**2)
    k=k/(np.sum(k))
    return(k)
#print(kernel(p).shape)
#------------------------------------

def blur(arr,k,y2):
    z=np.zeros(shape=(le,bd))
    for i in range(0,le):
        for j in range(0,bd):
            z[i,j]=np.sum(k*(y2[i:(i+n+2+n-1),j:(j+n+2+n-1)]))

    return(z)
#------------------------------------
r=blur(y[:,:,0],kernel(p),frame(y[:,:,0]))
g=blur(y[:,:,1],kernel(p),frame(y[:,:,1]))
b=blur(y[:,:,2],kernel(p),frame(y[:,:,2]))

x2=(np.array([r.transpose(),g.transpose(),b.transpose()])).transpose()
plt.imshow(x2)
plt.show()

Before we get into deep performance concerns, let's do a quality pass:

Global code

Lines like

x=plt.imread("s2.png")    #the file name
#x=color.rgba2rgb(x)
y=x
n=int(input())     #"n" is the level of blurring(typical values=5,10,20,50)
le=y.shape[0]
bd=y.shape[1]

should be in a function and not in global scope.

Mystery input

Pass a prompt to input, such as Please enter the level of blurring.

Extraneous parens

This:

y2=np.ones(shape=(le+(2*n),bd+(2*n)))
for i in range(0,le):
    for j in range(0,bd):
        y2[(i+n),(j+n)]=y[i,j]
return(y2)

can be

y2 = np.ones(shape=(le + 2*n, bd + 2*n))
for i in range(le):
    for j in range(bd):
        y2[i + n, j + n] = y[i, j]
return y2

Also note the default value for the start of range.

In-place division

This:

k=k/(np.sum(k))
return(k)

can be

k /= np.sum(k)
return k

Empty, not zero

Your blur method should definitely be vectorized, which I'll get to later, but for now, since it's clear that you're overwriting every entry: use empty rather than zeros.

Add another dimension

This:

r=blur(y[:,:,0],kernel(p),frame(y[:,:,0]))
g=blur(y[:,:,1],kernel(p),frame(y[:,:,1]))
b=blur(y[:,:,2],kernel(p),frame(y[:,:,2]))

should not be three separate calls. Your data should have a dimension (y already does) of length 3, and y should be passed in its entirety.

Variable names

le, bd and p mean nothing to me, and in three months they might not mean anything to you, either. Write these out with longer, more meaningful names. I promise that it will not slow the program down.


Computing the week for a certain date I have the following SQL query that computes for every date the week it falls in (a week begins on Sunday and ends on Saturday):

SELECT EntryDate
      ,CAST(DATEADD(DAY, 1-DATEPART(WEEKDAY, EntryDate), EntryDate) AS DATE) AS 'SundayDate'
      ,CAST(DATEADD(DAY ,7-DATEPART(WEEKDAY, EntryDate), EntryDate) AS DATE) AS 'SaturdayDate'
      ,CONVERT(VARCHAR, DATEADD(DAY ,1-DATEPART(WEEKDAY, EntryDate), EntryDate), 103) + ' - ' +
       CONVERT(VARCHAR, DATEADD(DAY ,7-DATEPART(WEEKDAY, EntryDate), EntryDate), 103) AS 'Week'
FROM MyTable
WHERE CAST(DATEADD(DAY, 1-DATEPART(WEEKDAY, EntryDate), EntryDate) AS DATE) <=
      CAST(DATEADD(DAY, 1-DATEPART(WEEKDAY, GETDATE()), GETDATE()) AS DATE)
ORDER BY CAST(DATEADD(DAY, 1-DATEPART(WEEKDAY, EntryDate), EntryDate) AS DATE)

It works fine, but I don't like the repeated function calls because they cluster the query and make it less readable.

How can I make the query cleaner and more readable? (I'm using SQL Server 2008.)

WITH Dates AS (
    SELECT EntryDate
         , DATEADD(DAY, 1-DATEPART(WEEKDAY, EntryDate), EntryDate) AS SundayDate
         , DATEADD(DAY, 7-DATEPART(WEEKDAY, EntryDate), EntryDate) AS SaturdayDate
        FROM MyTable
), Coming AS (
    SELECT DATEADD(DAY, 1-DATEPART(WEEKDAY, GETDATE()), GETDATE()) AS Sunday
)
SELECT EntryDate
     , SundayDate
     , SaturdayDate
     , CONVERT(VARCHAR, SundayDate, 103) + ' - ' + CONVERT(VARCHAR, SaturdayDate, 103) AS Week
    FROM Dates, Coming
    WHERE SundayDate <= Coming.Sunday
    ORDER BY 2;

The simplifications I've made are:

Extracted most of the query into a Common Table Expression named Dates to reduce redundancy.
Removed the pointless CAST(... AS DATE), since DATEADD() already produces dates.
Used a column number for ORDER BY.  With the Common Table Expression, though, we could just as easily ORDER BY SundayDate.
Extracted DATEADD(DAY, 1-DATEPART(WEEKDAY, GETDATE()), GETDATE()) into a Common Table Expression named Coming.  This doesn't reduce the complexity of the query, but helps make the WHERE-clause read more like English.

SQLFiddle

Edit

Since the benefits of the last two suggestions are debatable, you may prefer a milder approach that incorporates just the first two suggestions:

WITH Dates AS (
    SELECT EntryDate
         , DATEADD(DAY, 1-DATEPART(WEEKDAY, EntryDate), EntryDate) AS SundayDate
         , DATEADD(DAY, 7-DATEPART(WEEKDAY, EntryDate), EntryDate) AS SaturdayDate
        FROM MyTable
)
SELECT EntryDate
     , SundayDate
     , SaturdayDate
     , CONVERT(VARCHAR, SundayDate, 103) + ' - ' + CONVERT(VARCHAR, SaturdayDate, 103) AS Week
    FROM Dates
    WHERE SundayDate <= DATEADD(DAY, 1-DATEPART(WEEKDAY, GETDATE()), GETDATE())
    ORDER BY SundayDate;


Python Event Dispatcher This code creates a singleton event dispatcher. It's called like this:

eventDispatcher = Event()
eventDispatcher.on("event", handler)
eventDispatcher.trigger("event", 5)

I'm wondering if this is "the python way," or if it could be improved? Especially the class, I'm not sure about creating a new object for every use of the dispatcher.

class Event():
    events = {}

    def on(self, event, func):
        if event not in self.events.keys():
            self.events[event] = []

        self.events[event].append(func)

    def trigger(self, event, *args, **namedArgs):
        if event in self.events.keys():
            for func in self.events[event]:
                func(*args, **namedArgs)

Checking items in dictionnary
There is no need to call keys when checking for dictionnary membership. Much like there is no need of True in if my_bool == True.
on
Checking if a key exist and set it to a default value otherwise is so common that you have 2 standard ways of doing it: dict.setdefault(...) or using a collections.defaultdict.
trigger
Since KeyError should normaly not happen, you can use EAFP approach by using try...except instead of if. It's a tiny bit faster if exceptions are quite rare. Moreover, by using defaultdict you are guaranteed that every access to an item will be valid, thus eliminating the need to check membership of any key.
class
Since the only state you need is the one of the class, and in order to avoid instanciating to call methods, you can use @staticmethods and call them with Event.on(...) and Event.trigger(..).
Proposed alternative
from collections import defaultdict

class Event():
    __events = defaultdict(list)

    @staticmethod
    def on(event, func):
        Event.__events[event].append(func)

    @staticmethod
    def trigger(event, *args, **namedArgs):
        for func in Event.__events[event]:
            func(*args, **namedArgs)


std::function alternative with no dependencies An alternative to std::function for when compile time is more important than runtime performance. Doesn't pull in any headers.

(#include <functional> pulls in 19k lines of code.)

This satisfies my needs, but I'm curious how it could be improved (without adding any dependencies).

template<typename>
class func;

// Alternative to std::function for when compile time is
// more important than runtime performance. Doesn't pull in
// any headers.
template<typename Result, typename... Arguments>
class func<Result (Arguments...)> {

  struct holder {
    virtual ~holder() {}
    virtual Result call(Arguments...) = 0;
    virtual holder* clone() = 0;
  };

  template<typename Lambda>
  struct lambda_holder : public holder {
    virtual ~lambda_holder() {}
    Lambda lambda;
    lambda_holder(Lambda l) : lambda(l) { }
    Result call(Arguments... args) override {
      return lambda(args...);
    }
    holder* clone() override {
      return new lambda_holder<Lambda>(lambda);
    }
  };

public:

  func() { }

  func(const func& f) {
    holder = f.holder->clone();
  }

  func& operator=(const func& f) {
    if(holder) { delete holder; }
    holder = f.holder ? f.holder->clone() : nullptr;
    return *this;
  }

  // Create from a lambda.
  template<class F>
  func(F f) {
    holder = new lambda_holder<F>{f};
  }

  ~func() {
    if(holder) { delete holder; }
  }

  Result operator()(Arguments... args) const {
    assert(holder);
    return holder->call(args...);
  }

  operator bool() const {
    return holder;
  }

private:

  holder *holder = nullptr;

};

Self-assignment may be expensive, but it should be a no-op instead of UB. I suggest copy-and-swap.
Omitting move-semantics will most certainly cost you.
Your naming of the internal class suggests all callables are lambdas. Not true!
You cannot store any move-only callables. Admittedly std::function is also crippled in that respect. Just throw an exception if the non-copyable object needs to be copied.
If your function has return-type void, it should simply discard the callables return-value. You don't.
std::function throws an exception if empty when called. Using assert() is less than a pale imitation.
delete p; if p is a null pointer is a no-op. No need to double-check.
Conversion to bool should be explicit. Otherwise, conversion to any arithmetic type can happen.
As you said, yours is less efficient. That's especially the case when std::function benefits from mandatory small-object-optimization.
You don't use SFINAE when creating from a callable. That can be inconvenient.


I finally solved the Birthday Problem using Java I like to put ideas and mathematics into code whenever I am interested in the subject. Ever since I was introduced to The Birthday Problem I wanted to throw it into a Java program.

I started writing the program in several different ways, all of which in the end would return "Infinite" for one of the variables dealing with the factorial numbers.

I finally stumbled across "BigInteger" and "BigDecimal", I was annoyed at first of how hard it was to use them, but I stuck with it.

Others have built similar programs that generate random dates and run x amount of times ( like 1000) to find an average percentage rate at with birthdays collide, mine finds the mathematical statistic.

Check this out, how cool is this?

//Author: Joseph Kreifels II
//  Date: June 30, 2017
// Purpose: To find that percentage at which 2 students share a birthday.

import java.io.*;
import java.util.*;
import java.lang.Math;
import java.math.BigDecimal;
import java.math.RoundingMode;

public class Testing {

    public static void main(String[] args) {

        int n, r, i;
        double d;

        BigDecimal abd = new BigDecimal("0");

        Scanner sc = new Scanner(System.in);

        System.out.print("Enter Number of Students: ");
        i = sc.nextInt();

        n = 365;
        System.out.printf("Value of n: %d\n",n);
        r = 365 - i;
        System.out.printf("Value of r: %d\n",r);

        System.out.println("");
        System.out.println("Factorial of n = " + fact(n) );
        System.out.println("Factorial of r = " +  fact(r) );

        // First get the NPR
        abd = fact(n).divide(fact(n-(n-r))) ;

        // Display NPR for FUN.
        System.out.println("NPR of n and (n-r) = " + abd);

        // Now we must divide it by  x^y.. (e.g 365^5)
        abd = abd.divide(BigDecimal.valueOf( Math.pow(n,i) ),4, RoundingMode.HALF_UP);

        // I am lazy, So I turned it into a double to make the last 2 display statements easier to write.
        d = abd.doubleValue();

       System.out.printf("\nNot Same Birthday chance: %.0f%%\n", (d * 100) );
       System.out.printf("Same Birthday chance: %.0f%%\n", ((1 - d) * 100)  );

    } // main

        public static BigDecimal fact(int num)
    {

        BigDecimal abd = new BigDecimal("1");

        int i;
        for(i=0; i<num; i++)
        {
            abd = abd.add(abd.multiply(BigDecimal.valueOf(i)));
        }
        return abd;
    }

}

EDIT: I feel like the results could come out better. So Here are some changes.

Now 4 Decimal Places

abd = abd.divide(BigDecimal.valueOf( Math.pow(n,i) ),4, RoundingMode.HALF_UP);

Now outputs a percentage with 2 decimal places (e.g. 25.65%)

 System.out.printf("\nNot Same Birthday chance: %.2f\n", (d * 100 ) );
 System.out.printf("Same Birthday chance: %.2f\n", ((1 - d) * 100 )  );

Can be done without BigDecimals and factorials

You can compute the chance of shared birthdays using a simpler method, which avoids big numbers and factorials.  With one person, the chance of all people having different birthdays is 100% (obviously).  If you add a second person, that person has a 364/365 chance of also having a distinct birthday.  When you add a third person, that person has a 363/365 chance of having a birthday distinct from the previous two.  So as you keep adding more people, you keep track of the total chance of everyone having a distinct birthday by multiplying your current chance by (365-i)/365 for each new person.

Here is how you could do this in java:

import java.util.Scanner;

public class Testing {
    public static void main(String[] args)
    {
        Scanner sc = new Scanner(System.in);

        System.out.print("Enter Number of Students: ");
        int num = sc.nextInt();

        double notSameChance = 1.0;
        for (int i = 0; i < num; i++)
            notSameChance *= (365-i) / 365.0;

        System.out.printf("\nNot Same Birthday chance: %.0f%%\n",
                (notSameChance * 100) );
        System.out.printf("Same Birthday chance: %.0f%%\n",
                ((1 - notSameChance) * 100)  );
    }
}

Factorial function was strange

Looking at your factorial function, it does an addition and a multiplication on every loop.  Although it appears to work, you could make it simpler by doing just the multiplication, like this:

public static BigDecimal fact(int num)
{
    BigDecimal abd = new BigDecimal("1");

    for (int i=2; i<=num; i++) {
        abd = abd.multiply(BigDecimal.valueOf(i));
    }
    return abd;
}


Building an undirected graph from depth-first sorted node list in Python I have an array called nodes that contains a list of Node objects. The list is sorted in depth-first order. I have a function called build_undirected_tree that assigns the parent value and creates a list of children for each Node in the list.

I'm not satisfied with the need for using the idx array to circumvent Python's lack of pass-by-reference on primitive-type arguments and I'm wondering if there's a conceptually simpler way to accomplish the same thing.

class Node(object):
    def __init__(self, child_count):
        self.child_count = child_count

'''
This is a depth-first iterator. The weird `idx` array is used because
integers cannot be pass-by-reference.
'''
def node_iterator(nodes, idx=None, parent=None):
    if idx is None:
        idx = [0]
    node = nodes[idx[0]]
    yield (node, parent)
    for i in range(node.child_count):
        idx[0] += 1
        yield from node_iterator(nodes, idx, node)

def build_undirected_tree(nodes):
    for (node, parent) in node_iterator(nodes):
        node.parent = parent
        node.children = []
        if parent is not None:
            parent.children.append(node)

nodes = [...]
# Populate nodes from a file.
build_undirected_tree(nodes)

I believe you can get a similar result by creating and passing around an iterator on the nodes list:

def node_iterator(nodes, nit=None, parent=None):
    if nit is None:
        nit = iter(nodes)

    node = next(nit)
    yield (node, parent)

    for i in range(node.child_count):
        yield from node_iterator(nodes, nit, parent=node)


Mass build and save of docker images I would welcome feedback about this script to automate the building of docker images from a set of dockerfiles.

The dockerfiles are provided to the script via stdin.  The images are saved to the first (and only) argument of this script.  In normal use either one or two specific images are removed manually from the machine so that they get rebuilt or all images are removed (using a different script) for them all to be rebuilt.

Example invocation:

$ find dockerfiles -mindepth 1 -maxdepth 1 -type d | dockerfiles-build images

Actual output:

Reading dockerfile directories from std input.  Building can take a long time...
Building golang-build:2017-03-05T22-28GMT from dockerfiles/golang-build
Sending build context to Docker daemon 4.608 kB
<Lots of docker build process text>
Successfully built cc287d22a661
Saving golang-build:2017-03-05T22-28GMT.save.xz from cc287d22a661
Use images/run-golang-build to use the image
Reading next dockerfile...
$

My code (MIT license):

#!/bin/bash

# Script to build and save specific docker containers to the specified directory with the paths to the dockerfile directory arriving on the standard input

set -eu -o pipefail

if [ -z "${1-}" ] || ! [ -d "${1-}" ]; then
    echo "Correct invocation is: $(basename "$0") _out-directory_ <_dockerfile-directory-paths_" >&2
    exit 1
fi

outdir="${1-out}"
timestamp=$(date '+%Y-%m-%dT%H-%M%Z')

echo "Reading dockerfile directories from std input.  Building can take a long time..." >&2

while read dockerfilepath; do
    # Filter out empty input
    if [ -n "$dockerfilepath" ]; then

        # Process the path name
        name="$(basename $(readlink -f "$dockerfilepath"))"

        # Check that image does not already exist
        if ! docker images | grep "$name"; then

            # Get docker to build the image
            echo "Building $name:$timestamp from $dockerfilepath" >&2
            docker build -t "$name":$timestamp "$dockerfilepath"

            # Output the image (but only one if there are duplicates with different tags)
            image="$(docker images | grep "$name" | awk '{print $3}' | sort -u)"
            if [ "$(echo "$image" | wc -w)" -ne "1" ]; then
                echo "Multiple images detected for $name; aborting" >&2
                exit 1
            fi
            echo "Saving $name:$timestamp.save.xz from $image" >&2
            docker save "$image" | xz -z9 > "$outdir/$name:$timestamp.save.xz"

            # output the image specific run script, enforcing a naming convention
            cp "$dockerfilepath"/run* "$outdir/run-$name"
            echo "Use $outdir/run-$name to use the image" >&2
        else
            echo "A $name image already exists; remove it to build it again" >&2
        fi
    else
        echo "Skipping empty path" >&2
    fi
    echo "Reading next dockerfile..." >&2
done

Points of particular interest:

Are there any unsafe filepath manipulations
Are variables treated safely
Could any names break the script
Anything that could lead to the script being unreliable for unsupervised operation
Any general optimisations that you could suggest

This is a good shell script. You're checking for the existence of directories and files before taking any action and using set -eu and set -o pipefail.

In no particular order, here's what comes to mind.

1) In my opinion, the logic is getting slightly too complex for a shell script. I think you'd be better off in the long run with Python / Perl / Ruby.

2) reading from stdin and processing input from stdin in a loop.

See Stéphane Chazelas' answer to this question.

https://unix.stackexchange.com/questions/169716/why-is-using-a-shell-loop-to-process-text-considered-bad-practice

If you're going to do it anyway, you should probably to use IFS= read -r dockerfilepath.

3) name="$(basename $(readlink -f "$dockerfilepath"))" should have quotations around $(readlink ...), i.e.

name="$(basename "$(readlink -f "$dockerfilepath")")".

I don't think quotes around the whole expression are necessary in this case, but it's probably better to keep them there for peace of mind.

also readlink -f is not portable.

I'm also not sure why you're using it. Are you expecting lots of symlinks in your environment?

4) ! docker images | grep "$name"

You can use grep -q -F -- "$name" if you don't need the output (and just want the exit status) and want to treat "$name" as a fixed string.

I think there are a few other places where you're using grep with $name. Always use -F -- unless you want the regex behavior.

5) cp "$dockerfilepath"/run* "$outdir/run-$name"

This is actually fine from a whitespace perspective. (http://mywiki.wooledge.org/glob)

but you may want to set nullglob or failglob just to be safe.

Also should just check for the existence of the "$outdir/run-$name" directory earlier so that the operation fails before we attempt to build the container.

I'm not sure what it means if the directory doesn't exist, but a simple

[ -d "$outdir/run-$name" ] immediately after computing name would be useful.

6) At the beginning test for the existence of "$outdir"

outdir is just like $1 except it hasa default value of out. You can move the test for the existence of ${1-} until after outdir is set.

7) since you're using sort(1), set LC_ALL=C; export LC_ALL. Also, set LC_COLLATE=C; export LC_COLLATE for good measure.

8) If you aren't going to go the portable route, you can use [[ .. ]] instead of [ .. ] for tests.

9) use printf over echo

More explanation here, basically echo does weird things with backslashes. This isn't a huge deal since you're only using echo for diagnostic information, except in $(echo "$images" | wc -w) which should be changed to $(printf "%s" "$images" | wc -w).

https://unix.stackexchange.com/questions/65803/why-is-printf-better-than-echo

Here's one possible script with the changes incorporated. I changed the logic somewhat so it now says Reading next dockerfile at the very beginning and uses continue to skip the current iteration rather than having multiple levels of if-else in the main loop.

#!/bin/bash

# Script to build and save specific docker containers to the specified directory with the paths to the dockerfile directory arriving on the standard input

set -eu
set -o pipefail

shopt -s failglob

LC_ALL=C; export LC_ALL
LC_COLLATE=C; export LC_COLLATE

outdir="${1-out}"
timestamp=$(date '+%Y-%m-%dT%H-%M%Z')

if [[ ! -d $outdir ]]; then
    echo "Correct invocation is: $(basename "$0") _out-directory_ <_dockerfile-directory-paths_" >&2
    exit 1
fi

diag() {
    >&2 printf '%s\n' "$1"
}

diag "Reading dockerfile directories from std input.  Building can take a long time..."

while IFS= read -r dockerfilepath; do

    diag "Reading next dockerfile..."

    # Filter out empty input
    if [[ -z "$dockerfilepath" ]]; then
        diag "skipping empty path"
        continue
    fi

    # Process the path name
    name="$(basename "$(readlink -f "$dockerfilepath")")"
    outpath="$outdir/run-$name"

    # Check that image does not already exist
    if docker images | grep -q -F -- "$name"; then
        diag "A $name image already exists; remove it to build it again"
        continue
    fi

    # create output directory if it doesn't exist
    if [[ ! -d $outpath ]]; then
       mkdir -p "$outpath"
    fi

    # Get docker to build the image
    diag "Building $name:$timestamp from $dockerfilepath"
    docker build -t "$name":"$timestamp" "$dockerfilepath"

    # Output the image (but only one if there are duplicates with different tags)
    image="$(docker images | grep -F -- "$name" | awk '{print $3}' | sort -u)"
    if [[ "$(printf "%s" "$image" | wc -w)" -ne 1 ]]; then
        diag "Multiple images detected for $name; aborting"
        exit 1
    fi
    diag "Saving $name:$timestamp.save.xz from $image"
    docker save "$image" | xz -z9 > "$outdir/$name:$timestamp.save.xz"

    # output the image specific run script, enforcing a naming convention
    cp "$dockerfilepath"/run* "$outpath"
    diag "Use $outpath to use the image"
done


C++ custom exception class I am wondering, how can I improve this simple MissingResource exception class? I am using it in my simple game.
template <typename Resource>
class MissingResource : public std::exception {
    std::string_view fileName{};
    std::string_view msg{};

public:
    MissingResource() = default;
    MissingResource(std::string_view fileName)
    : fileName{fileName}
    {
        initMsg<Resource>();
    }

    template <typename T>
    void initMsg() {
        msg = "Failed loading resource: "
              "{ Type: "    + std::string(typeid(Resource).name()) + ", "
              "File name: " + fileName.data()                      + " }";
    }

    virtual const char* what() const noexcept override {
        return msg.data();
    }
};

Throw usage:
template <typename Key, typename Resource>
#if (__cplusplus == 202002L)
  requires Mappable<Key>
#endif
class ResourceHolder {
    std::unordered_map<Key, std::unique_ptr<Resource>> resources;
    std::string resourcesDir;

public:

    explicit ResourceHolder(std::string_view resourcesDir = "../resources/")
            : resourcesDir{std::move(resourcesDir)}
    {}

    template <typename... Args>
    void insert(const Key& key, std::string_view fileName, Args&&... args) {
        auto resPtr = std::make_unique<Resource>();

        bool loaded{};
        if constexpr (std::is_same<Resource, sf::Music>()) {
            loaded = resPtr->openFromFile(resourcesDir + fileName.data(), std::forward<Args>(args)...);
        } else {
            loaded = resPtr->loadFromFile(resourcesDir + fileName.data(), std::forward<Args>(args)...);
        }

        /** Called here */
        if (!loaded) {
            throw MissingResource<Resource>(fileName);
        }
        resources.emplace(key, std::move(resPtr));
}

Try-Catch usage:
ResourceManager::ResourceManager() {
    try {
        loadResources();
    } catch (const std::exception& e) {
        std::cerr << e.what() << std::endl;
        std::exit(EXIT_FAILURE);
    }
}

Exemplary error message:
what():  Failed loading resource: { Type: N2sf11SoundBufferE, File name: Death.wav }

P.S: If you see some smelly code or have doubts or found any kind of other mistakes, I would be looking forward to knowing them. Thanks!

std::string_view
Firstly, there seems to be some misusages of string_view. string_view is non-owning, and is a view to an allocated char array (such as string).
The usage
template <typename Resource>
class MissingResource : public std::exception {
    std::string_view fileName{};
    std::string_view msg{};

public:
    [...]
    template <typename T>
    void initMsg() {
        msg = "Failed loading resource: "
              "{ Type: "    + std::string(typeid(Resource).name()) + ", "
              "File name: " + fileName.data()                      + " }";
    }
    [...]
};

Is dangerous, since after calling initMsg(), msg will point to an destroyed object (the right hand string is temporary).
Better change it to be an owning string in this case: std::string_view msg => std::string msg. I assume you already have it like this, otherwise it would provide the output which you gave in the description.
Secondly, the other member variable std::string_view fileName does not seem necessary. Since you create the error message for msg with fileName and do not use alone, we can skip it, right?
General design of MissingResource
Applying the changes above, we have:
template <typename Resource>
class MissingResource : public std::exception {
    std::string msg{};

public:
    MissingResource() = default;
    MissingResource(std::string_view fileName)
    {
        initMsg<Resource>();
    }

    template <typename T>
    void initMsg(std::string_view fileName) {
        msg = "Failed loading resource: "
              "{ Type: "    + std::string(typeid(Resource).name()) + ", "
              "File name: " + fileName.data()                      + " }";
    }

    virtual const char* what() const noexcept override {
        return msg.data();
    }
};

initMsg is currently a public method, but from what I see, it's only used locally in the constructor. If this is the case, it should rather be a private method - you want to keep the public API for the class as simple and small as possible.
Another improvement would be to be able to create msg in the constructor list - so that it does not have to be created twice. We can do this by making the initMsg a static method.
Let's remove the extra unnecessary template for initMsg as well - the entire class already holds the template argument.
And note that virtual + override is unnecessary. If you mark a method as overrided, it is implicitly virtual. Remove virtual for unnecessary noise.
We now have something like this:
template <typename Resource>
class MissingResource : public std::exception {
    std::string msg{};

    static std::string initMsg(std::string_view fileName) {
        return "Failed loading resource: "
              "{ Type: "    + std::string(typeid(Resource).name()) + ", "
              "File name: " + fileName.data()                      + " }";
    }

public:
    MissingResource() = default;
    MissingResource(std::string_view fileName) : msg{initMsg<Resource>()}
    {}

    const char* what() const noexcept override {
        return msg.data();
    }
};

ResourceHolder
Since this review is already long, I will just point out one thing in ResourceHolder, namely in insert. We can avoid separating declaration from initialization for bool loaded (and make it const!) by encapsulating the logic into a separate method. This might be a bit preferential, but it is good practice to avoid separating declaration and initialization.
Something like this:
private:
    bool fromFile(std::string_view fileName, Args&&... args)
    {
    if constexpr (std::is_same<Resource, sf::Music>()) {
            return resPtr->openFromFile(resourcesDir + fileName.data(), std::forward<Args>(args)...);
        }
    else {
            return resPtr->loadFromFile(resourcesDir + fileName.data(), std::forward<Args>(args)...);
    }

public:
    template <typename... Args>
    void insert(const Key& key, std::string_view fileName, Args&&... args) {
        auto resPtr = std::make_unique<Resource>();

        const bool loaded = fromFile(fileName, std::forward<Args>(args)...)

        /** Called here */
        if (!loaded) {
            throw MissingResource<Resource>(fileName);
        }
        resources.emplace(key, std::move(resPtr));
    }


Return the last word of the url and capitalize the first letter Can the below code be done in one line? Do you think it should or is it ok like this?

function() {
var str = {{Click URL}};
var str1 = str.split('/').pop();
  return str1.charAt(0).toUpperCase() + str1.slice(1);
}

Seems alright, though the names str and str1 aren't very descriptive names. Also, indentation is inconsistent.

I'd also point out that your code obviously isn't real code (which technically makes it off-topic for Code Review) due to the {{Click URL}}. I can still tell what's going on, but in the future, post the real code. Or at least post a working piece of code where the function takes a URL as its argument or something, rather than having a hard-coded syntax error!

But beyond that: URLs can be complex. While I'm sure this is built for a very specific purpose, and you're expecting URLs that look a certain way, it'd be nice to make it more robust.

Firstly, the URL can have a trailing slash which would cause your code to find an empty string.

Also, a URL can have a query string like ?foo=bar or a hash/fragment like #heading-2 appended. Your code would treat such things as part of the last "word" in the URL (or maybe the query string contains slashes too, which would split the URL in the wrong place).

Lastly, URLs can contain percent-encoded characters, which won't be decoded by your function.

Some test cases:

http://example.com/something           => "Something"
http://example.com/something/          => ""
http://example.com/something?q=foo     => "Something?q=foo"
http://example.com/something#id123     => "Something#id123"
http://example.com/something?q=foo#123 => "Something?q=foo#123"
http://example.com/some%20thing        => "Some%20thing"
http://example.com/something/#id123    => "#id123"
http://example.com/some%20thing?q=foo  => "Some%20thing?q=foo"

I trust you can see the problem.

Something like this is more robust, though I don't guarantee it's bullet-proof:

function lastWordCapitalized(url) {
  return url.replace(/^.*\/([^?#\/]+).*$/, function (_, word) {
    var word = decodeURI(word);
    return word.charAt(0).toUpperCase() + word.slice(1);
  });
}

Which returns

http://example.com/something           => "Something"
http://example.com/something/          => "Something"
http://example.com/something?q=foo     => "Something"
http://example.com/something#id123     => "Something"
http://example.com/something?q=foo#123 => "Something"
http://example.com/some%20thing        => "Some thing"
http://example.com/something/#id123    => "Something"
http://example.com/some%20thing?q=foo  => "Some thing"

I should add, that all of this is assuming that

You're always dealing with absolute URLs. A link tag's href might just be "..", which is perfectly valid, but in which case there isn't much to display.
You always have some sort of path in the URL. If the URL just points to the domain's root (e.g. http://example.com/), you'll just end up upcasing the domain name.


CoffeeScript implementation of A* algorithm a_star =                        # returns the goal node (or null if path not found). To reconstruct path follow the node.__src till null
  (
    start,                       # starting node
    neighbors,                   # a function that takes in a node and returns list of neighbours
    h,                           # admissable A* heuristic distance from goal i.e. heurisitic(x) <= distance(x,y) + heuristic(y) for all x,y
    dist = (a,b) -> 1,           # takes two nodes and returns distance between them (optional - default is 1)
    isGoal = (x) -> h(x) is 0    # returns true iff node is goal (optional - assumes heurisitc(goal) = 0)
  ) ->

    closed = {}                # set of nodes already evaluated
    q = {}                     # set of tentative nodes to be evaluated, the value is the 'f_score' (F = G + H)
    g = []                     # 'g-score' - the exact cost to reach this node from start

    add = (node, parent) ->
      node.__src = parent
      g[node] = if parent? then g[parent] + dist parent, node else 0
      q[node] = g[node] + h(node)

    remove_best = ->
      best = null
      best = node for node,f of q when best is null or f < q[best]
      if best? then closed[best] = true; delete q[best]
      return best

     add start, null

     while true
      c = remove_best()
      if c is null or isGoal c then return c
      add n, c for n in neighbours c when not closed[n] and (q[n] is null or g[c] + dist(c, n) <= g[n])

I am not an expert on the A* algorithm. I'm suggesting changes based on the
Rules of Clarity, Simplicity, and Economy and the code you provided,
not on an extensive knowlege of graph traversal methods.

Extensive documentation inline is tricky to read. Consider using a standardized
docstring format like those in docco or codo for detailed
inline docs.

###
  returns the goal node or null if path not found. Reconstruct path by
  following the goalNode.src until null

  @param [Object] start The node to start searching from
  @param [Object] options The options object

  @option [Function] neighbors Takes node, returns array of neighbors
  @option [Function] heuristic Takes node, returns A* heuristic
  @option [Function] isGoal Takes node, returns true if node is goal
  @option [Function] distance Takes two nodes returns distance between them

  @returns {Object} Returns the goal node with a tracable path to the start
###

It's not clear which options you expect you be left as their default values
most frequently. If most options will be left as defaults most of the time,
and the function will "just work" without configuration, use an options
object and set sensible defaults.

aStar = (start, opts = {}) ->
  opts.neighbors or= (node) -> []
  opts.heuristic or= (node) -> 0
  opts.isGoal    or= (node) -> opts.heuristic(node) is 0
  opts.distance  or= (a, b) -> 1

If you want to find paths from many start points, you might
setup your function once and allow several calls, each providing a start point.

AStar = (opts = {}) ->
  opts.distance  or= (a, b) -> 1
  # ... and other defaults

  (start) ->
    # logic for calculating path from "start"

# use it like this
astar = AStar
  isGoal: -> yes
  neighbors: -> [1, 2, 3]

# goal is the destination node (as it was in the original logic)
goal = astar {x: 1, y: 2}

Inside the logic function, remember that "Clarity is better than cleverness".
Slightly longer, but much more descriptive names go a long way to making the
operations easier to understand.

(start) ->
  closed = {} # nodes already evaluated
  nodes  = {} # key: node to be evaluated, value: f-score (F = G + H)
  gscore = [] # the exact cost to reach this node from start

Breaking the operations in to related sections makes it easier to follow.

  # constructing path
  addNode = (node, src) ->
    node.src = src
    gscore[node] = if src? then gscore[src] + opts.distance src, node else 0
    nodes[node] = gscore[node] + opts.heuristic(node)

Naming functions with verbs and naming functions that return booleans with is
and has usually increases clarity. Verbs do things, so do functions. You
want the behavior of your code to be unsurprising to the person reading it.

  # selecting best path
  isBetter = (a, b) ->
    a is null or b < nodes[a]

  selectBest = (best = null) ->
    best = node for node, fscore of nodes when isBetter(best, fscore)
    closed[best] = yes if best?
    delete nodes[best]
    best

Break complex boolean questions into parts so the reader (which is also you,
later) can follow (with a minimal cognitive load) what you intend to be
determining with your comparisons.

  # answers about current node
  isOpen = (node) ->
    not closed[node]

  isShorter = (a, b) ->
    nodes[a] is null or gscore[b] + opts.distance(a, b) <= gscore[a]

  isAvailable = (neighbor, node) ->
    isOpen(neighbor) and isShorter(neighbor, node)

The big loop that does all the work would be clearer if you could quickly see
what situation would trigger the loop ending. while true is less communicative
than doneSearching.

  findGoal = (start) ->
    # insert start point
    addNode start, null

    # store the node in use
    node = null

    doneSearching = ->
      node = selectBest()
      return yes if node is null or opts.isGoal node
      no

    # find the shortest path in the current open set
    # stop looking when out of options or at the goal
    until doneSearching()
      for neighbor in getNeighbors node when isAvailable neighbor, node
        addNode neighbor, node
    node

Everything up to here defines the operations that will be used. This is the
single point that triggers action in this function. Reducing the number of
moving parts makes tracing the logic path easier.

  findGoal(start)


Simple class, solely for setting "globals" I'm aiming for the best and simplest way to access "global" variables set in the config file for my website. So I can use them anywhere, even inside functions

It's for file versioning, and I use htaccess to rewrite urls (trim out version part)

Right now, I'm at this:

// config.php:

class VERS {

  static $css = '_214';
  static $js = '_167';
  static $image = '_56';
  static $other = '_19';

}

// subpage.php:

require 'settings/config.php';
echo '<link type="text/css" rel="stylesheet" href="style'.VERS::$css.'.css" />';

Is this an ok solution? Are there downsides and/or better ways?

I want to avoid setting multiple DEFINE's, and I aim for a short version var call (like VERS::$css)

EDIT: This is not about alternatives to the filename method itself (like filename?queries etc.)

If these are constants, use const, not static:

class VERS {
    const css = '_214';
    const js = '_167';
    const image = '_56';
    const other = '_19';
}

and then

echo .....VERS::css....

As to whether this is a good idea in general, it would be helpful to show a bigger picture: what the rest of config looks like? how it's used? etc.


How do I clean up these Python decorators? I wrote some type checking decorators for Python. One checks the type that a function returns, the other checks the type that a function accepts.

import functools

def returns(t):
""" Calls a function, and if it returns something of the wrong type,
    throws a TypeError
"""
    def outer(func):
        @functools.wraps(func)
        def wrapper(*args, **kwargs):
            output = func(*args, **kwargs)
            if not isinstance(output, t):
                message = "function {} returned an invalid type: {} is a {}"
                raise TypeError(message.format(func.__name__,
                                               output,
                                               type(output)))
            else:
                return output
        return wrapper
    return outer

class accepts:
    def __init__(self, *artypes, **kwtypes):
        """ Pass it a list consisting of types or tuples of types.
        in other words, pass it things that will be valid second
        arguments to the isinstance(arg, type) method.
        """
        self.artypes = artypes
        self.kwtypes = kwtypes

    def checkTypes(self, args, kwargs):
        """ Checks each arg and kwarg for type validity, and if
            they fail, then it throws a TypeError. Not very descriptive,
            but it's just an example.
        """
        type_zip = zip(args, self.artypes)
        for arg, type_ in type_zip:
            if not isinstance(arg, type_):
                raise ValueError()

        for key, value in kwargs.items():
            if not isinstance(value, self.kwtypes[key]):
                raise ValueError()

    def __call__(self, func):
        @functools.wraps(func)
        def wrapper(*args, **kwargs):
            self.checkTypes(args, kwargs)
            return func(*args, **kwargs)
        return wrapper

@accepts(int)
def hitimes(times=1):
    return "hello " * times

@accepts(int, word=str)
def byetimes(times, word="bye"):
    return " ".join([word] * times)

print(hitimes(2)) # works
print(byetimes(5)) # works
print(byetimes(3, word="adios")) # works as expected
print(byetimes(3, word=[])) # fails as expected

For returns, I used nested functions, and for accepts I used the class style of decorator. But both of them get pretty unwieldy with respect to indentation levels. What can I do to make these cleaner?

Note that the wrappers are themselves pretty pointless.

I agree with @minitech that accepts shouldn't be a class. It can be written much more compactly using a nested decorator (like returns, in fact):

def accepts(*artypes, **kwtypes):
    """
    Pass it a list consisting of types or tuples of types.
    in other words, pass it things that will be valid second
    arguments to the isinstance(arg, type) method.
    """
    def outer(func):
        @functools.wraps(func)
        def wrapper(*args, **kwargs):
            type_zip = zip(args, artypes)
            for arg, type_ in type_zip:
                if not isinstance(arg, type_):
                    raise ValueError()

            for key, value in kwargs.items():
                if not isinstance(value, kwtypes[key]):
                    raise ValueError()
            return func(*args, **kwargs)
        return wrapper
    return accepts_wrapper

This then works equivalently to returns, and is much shorter (as you need less variable-passing code).


Using node bin/www with twits I use the following code which is working as expected, but I want to use it in production. I'm not sure that this is clean code since I'm new to node.js. It would be great if I can get some example how can I write cleaner code.

I mean how do I use the config to read the values, there is better way?
How do I use the onListening?

/**
 * Module dependencies.
 */
var app = require('../app');
var debug = require('debug')('blog-twitter:server');
var http = require('http');
var os=require('os');
var request=require('superagent')
/**
 * Get port from environment and store in Express.
 */

var port = normalizePort(process.env.PORT || '8062');
app.set('port', port);

/**
 * Create HTTP server.
 */
//var io=require('express-socket.io');

var server = http.createServer(app);
var io=require('socket.io').listen(server);
var Twitter=require('twitter');
var cfg = require('../config.json');
//console.log(cfg.twitter);
/**
*Create a twitter client to use AUTH credentials defined
*in config.json file
*/
var client=new Twitter({
  consumer_key: cfg.twitter.consumer_key,
  consumer_secret: cfg.twitter.consumer_secret,
  access_token_key: cfg.twitter.access_token_key,
  access_token_secret: cfg.twitter.access_token_secret
});
/**
Bind to twitter public streaming API
*/
var stream=client.stream('statuses/filter',{track:"Javascript"});
/**
*Create twitter stream, when a tweet is available,
*get its embeded format and emit that to the server.
*/
stream.on("data", function(data){
  console.log(data.id_str);
  var tweet_id="https://api.twitter.com/1.1/statuses/oembed.json?id="+data.id_str;
  //console.log(data);

  //console.log("ID:",tweet_id);
  request.get(tweet_id)
  .end(function(err,res){
      if(err){
        console.log("Error: "+err);
      }else{
        //console.log(res.body);
        io.emit('tweet',res.body);
      }
  });
  //console.log("Tweet: ",data);
});
stream.on('error', function(err){
  console.log("Error retrieving tweets: "+err);
});
io.on('connection', function(client){
  client.on("join", function(data){
    console.log(data);
  });
  client.emit("join",{"message":"Welcome"});
});
/**
 * Listen on provided port, on all network interfaces.
 */

server.listen(port);
server.on('error', onError);
server.on('listening', onListening);

/**
 * Normalize a port into a number, string, or false.
 */

function normalizePort(val) {
  var port = parseInt(val, 10);

  if (isNaN(port)) {
    // named pipe
    return val;
  }

  if (port >= 0) {
    // port number
    return port;
  }

  return false;
}

/**
 * Event listener for HTTP server "error" event.
 */

function onError(error) {
  if (error.syscall !== 'listen') {
    throw error;
  }

  var bind = typeof port === 'string'
    ? 'Pipe ' + port
    : 'Port ' + port;

  // handle specific listen errors with friendly messages
  switch (error.code) {
    case 'EACCES':
      console.error(bind + ' requires elevated privileges');
      process.exit(1);
      break;
    case 'EADDRINUSE':
      console.error(bind + ' is already in use');
      process.exit(1);
      break;
    default:
      throw error;
  }
}

/**
 * Event listener for HTTP server "listening" event.
 */

function onListening() {
  var addr = server.address();
  var serverName=cfg.server;
  var runServer=`http://localhost:8060/service/tweets/${server.address().port}`;
  if(cfg.hosted){
    runServer=`http://${serverName}/service/tweets/${server.address().port}`;
  }
  console.log(" Server on:",runServer)
  var bind = typeof addr === 'string'
    ? 'pipe ' + addr
    : 'port ' + addr.port;
  debug('Listening on ' + bind);
  var expose=()=>{
      request.put(blogServer,(err,res)=>{
        if(err){
          console.log("Error:",err);
        }
      });
  }
  setInterval(expose,15*1000);
}

If you want to write production-ready code using node, then you should use node modules to divide your code into independent, maintainable modules. Even better, use a framework (Express). It provides "development" and "productions" options for the application and it also solves the trivial common issues for you, like setting up a router. You can also use an scaffolding tool, to generate a standard folder structure for your app.

For reading values from a config file, create a new module and store all the application wide configuration in it, instead of reading directly from a .json file.

Coming to onListening() function, it's not a good practice to hardcode url's. It's better to store them in a config module and require it when needed.


JavaScript Queue Attempt #2 This is a follow-up review to JavaScript queue implementation using linked list

I've run the code through a linter, fixed every style error, added const where it was possible, removed extraneous templates and changed the return policy for calling dequeue() on an empty queue to make it fail fast. I also implemented an empty() method for the user's convenience.

I'm still undecided as to whether to use single or double quotes for strings. The linter suggests single quotes, but I've read reviews suggesting double quotes might be better because they look less like backticks, which are used for template literals.

Is there anything I've missed?

function makeQueue() {
  // data
  let head = null;
  let size = 0;

  // methods
  function enqueue(x) {
    let newNode = {next: head, data: x};
    head = newNode;
    size++;
  }

  function dequeue() {
    if (size === 0) {
      throw new Error('dequeue() on an empty queue');
    }

    const x = head.data;
    head = head.next;
    size--;

    return x;
  }

  function getSize() {
    return size;
  };

  function isEmpty() {
    return size === 0;
  };

  // public
  return {
    enqueue,
    dequeue,
    size: getSize,
    empty: isEmpty
  };
}

var q0 = makeQueue();
var q1 = makeQueue();
var q2 = makeQueue();

for (var i = 0; i < 16; i++) {
  q0.enqueue(i);
}

console.log(q0.size());
console.log(q1.empty());
console.log(q2.size());

Edit

The goal here was to improve the JavaScript, I only realized now that I actually wrote a LIFO stack.

Looks good!

I'm a bit surprised the linter didn't catch redundant semicolons at the end of some function declarations, for example here:

  function getSize() {
    return size;
  };

I would rename all the local variables and function parameters named x to data.

In enqueue, you could inline the newNode variable,
and assign the object literal {next: head, data: data} directly to head.

It would be good to test a bit more thoroughly,
for example dequeue everything from a non-empty queue,
and performing a mix of enqueue and dequeue operations.


Replace unicode character in the string A function which replaces an unicode character in a string:

void replaceAllOccurences(std::string& source,
                          const std::string& replaceFrom,
                          const std::string& replaceTo)
{
    std::vector<std::uint8_t> data(source.begin(), source.end());
    std::vector<std::uint8_t> pattern(replaceFrom.begin(), replaceFrom.end());
    std::vector<std::uint8_t> replaceData(replaceTo.begin(), replaceTo.end());

    std::vector<std::uint8_t>::iterator itr;
    while((itr = std::search(data.begin(), data.end(), pattern.begin(), pattern.end())) != data.end())
    {
        data.erase(itr, itr + pattern.size());
        data.insert(itr, replaceData.begin(), replaceData.end());
    }

    source = std::string(data.begin(), data.end());
}

Usage:

std::string source = "123€AAA€BBB";
std::string replaceFrom = "€";
std::string replaceTo = "\x80";

replaceAllOccurences(source, replaceFrom, replaceTo);

replaceTo may be get from some external conversion library, for example: iconvpp. Normally I would convert the whole source using iconvpp library but I have a case where I need to convert only particular character, for example "€".

If you're not worried about performance, you can replace all the manual work by using the <regex> facilities, which results in a considerable reduction of code to test and maintain.

#include <regex>

source = std::regex_replace(source, std::regex("€"), "\x80");

I would still keep it in a separate function to make it easy to change the implementation afterwards.