Loading files or directories based on type I am building a config class for PHP which initially loads all given config files. To add files, users should be able to provide either a single file or a directory where files can be JSON or PHP.

Below is what I have so far, but it seems sluggish and inefficient to me. Is there a more elegant way to give users a wide choice of input? Should I split up the individual input cases into several private (or public?) methods? Are there any problems with this code apart from performance?

/**
* add function.
* merges the config data with another array
*
* @access public
* @param string|array $input  the data to add
* @return void
*/

public function add($input)
{
    // if we got a string, it can be either JSON, a filename or a folder path.
    if (is_string($input)) {

      // if we got a directory, add each file separately again (except . and ..).
      if (is_dir($input)) {
        if (! is_readable($input)) throw new \RuntimeException('Directory "' . $input . '" is not readable.');

        // add each file in a directory to the config
        foreach (array_diff(scandir($input), array('..', '.')) as $file) {
          $this->add(rtrim($input, DIRECTORY_SEPARATOR) . DIRECTORY_SEPARATOR . $file);
        }
        return;
      }

      // if we got a filename, decide how to handle it based on the extension, then add its parsed content again
      if (is_file($input)) {
        if (! is_readable($input)) throw new \RuntimeException('File "' . $input . '" is not readable.');

        switch(pathinfo($input, PATHINFO_EXTENSION)) {
            case 'php':
              $content = require($input);
            break;

            case 'json':
              $content = $this->parseJSON(file_get_contents($input));
            break;
          }

        $this->add($content);
        return;
      }

      // string input is generally treated as JSON
      $this->add($this->parseJSON($input));
      return;
    }

    // if the input is an array, it must be plain config data, ready to merge in.
    if (is_array($input)) {
      $this->data = array_replace_recursive($this->data, $input);
      return;
    }

    // if we have no match, throw an exception (this happens if neither a string nor an array was given).
    throw new \RuntimeException('Provided data could not be parsed ("' . substr($input, 0, 20) . '...").');
    return;
}

To give a little context to the file extension switch: I used it to be able to implement more file extensions later on. I also thought about creating a parser interface for each type to be more expandable, that would prohibit further use of a single class file tho.

PHP config files consist of the following:

<?php return array(
  'foo' => 'bar'
);

I don't see glaring performance issues that should make this "sluggish".

But you could improve performance by extracting specialized logic to specialized functions:

add_file
add_dir
add_array
add_json

Because,
in the current implementation,
for example,
when you scan a directory and call $this->add for each file,
the recursive call will do unnecessary is_string, is_dir, is_file checks on it.
Those checks don't need to be in the specialized add_file.

The same goes for the other input types,
when you separate to multiple functions,
there should be fewer unnecessary conditions to go through.

Users of the class,
when they know that the input is JSON data,
it will make sense that they should call add_json directly.
In the end,
I don't know what legitimate cases will be left for calling the generic add function.
At that point, you can delete the function.

The return statement is pointless here:

throw new \RuntimeException('Provided data could not be parsed ("' . substr($input, 0, 20) . '...").');
return;

Instead of this:

    case 'php':
      $content = require($input);
    break;

The common practice is to align the break statement with the content of the case, like this:

    case 'php':
      $content = require($input);
      break;

It's recommended to use braces { ... } with single-statement blocks too,
even they aren't strictly necessary.


Dynamic programming solution to "Climbing Stairs" This is the "Climbing Stairs" problem from leetcode.com:

  You are climbing a stair case. It takes \$n\$ steps to reach to the top.

  Each time you can either climb 1 or 2 steps. In how many distinct ways
  can you climb to the top?

  Note: Given \$n\$ will be a positive integer.

  Example 1:

  Input: 2 Output:  2 Explanation:  There are two ways to climb to the
  top.

  1 step + 1 step
  2 steps

  Example 2:

  Input: 3 Output:  3 Explanation:  There are three ways to climb to the
  top.

  1 step + 1 step + 1 step
  1 step + 2 steps
  2 steps + 1 step

I thought this question is very similar to Fibonacci question. I use dynamic programming,

dp[n] = dp[n - 1] + dp[n - 2]

class Solution:
    # @param n, an integer
    # @return an integer
    def climbStairs(self, n):
        dp = [1 for i in range(n+1)]
        for i in range(2, n+1):
            dp[i] = dp[i-1] + dp[i-2]
        return dp[n]

The code in the post has to compute the \$i\$th Fibonacci number, \$F_i\$, for every \$i \le n\$, in order to compute \$F_n\$. It's possible to do much better than that, by using the recurrence $$ \eqalign{F_{2n−1} &= F_{n}^2 + F_{n−1}^2 \\ F_{2n} &= (2F_{n−1} + F_{n}) F_{n}} $$ combined with memoization. For example, you could use the @functools.lru_cache decorator, like this:

from functools import lru_cache

@lru_cache(maxsize=None)
def fibonacci(n):
    """Return the nth Fibonacci number."""
    if n <= 1:
        return n
    elif n % 2:
        a = fibonacci(n // 2)
        b = fibonacci(n // 2 + 1)
        return a * a + b * b
    else:
        a = fibonacci(n // 2 - 1)
        b = fibonacci(n // 2)
        return (2 * a + b) * b

this computes the \$10^6\$th Fibonacci number, which has more than 200,000 decimal digits, in a fraction of a second:

>>> from timeit import timeit
>>> timeit(lambda:fibonacci(10**6), number=1)
0.06556476117111742
>>> len(str(fibonacci(10**6)))
208988

By contrast, the code in the post cannot compute Solution().climbStairs(10 ** 6) without running out of memory.


Jumping on the clouds I have started the Hackerrank interview preparation kit. The first problem I have solved with Swift is as follows:

  Emma is playing a new mobile game that starts with consecutively numbered clouds. Some of the clouds are thunderheads and others are cumulus. She can jump on any cumulus cloud having a number that is equal to the number of the current cloud plus 1 or 2. She must avoid the thunderheads. Determine the minimum number of jumps it will take Emma to jump from her starting position to the last cloud.

  For each game, Emma will get an array of clouds numbered 0 if they are safe or 1 if they must be avoided.

func jumpingOnClouds(c: [Int]) -> Int {
    var jump = 0
    var location = 0
    repeat {
        if (location + 2 < c.count) &&
        (c[location + 2] == 0) {
            location += 2
        } else {
            location += 1
        }
        if location >= c.count {
            break
        }
        jump += 1
    } while location < c.count
    return jump
}

It's time complexity is \$O(n)\$. Are there any ways I could reduce this? I am also interested in improving the code quality. Are there ways I can take better advantage of Swift to improve my code?

Your program is clearly written, and works correctly (to the best of my knowledge). I don't think that the time complexity is \$O(n)\$ can be improved, because the array must be traversed in order to locate the dangerous thunderheads. Your algorithm already optimizes that by checking only every second element (if that is a safe spot), relying on the fact that a solution is guaranteed.

The program can be improved a bit though. The while location < c.count condition of the main loop will always be true because the loop is always “early left” at

if location >= c.count {
    break
}

As a consequence, the program computes one location beyond the array bounds which is not needed (and not counted). This can be simplified by using a while-loop instead:

while location < c.count - 1 {
    if (location + 2 < c.count) &&
        (c[location + 2] == 0) {
        location += 2
    } else {
        location += 1
    }
    jump += 1
}

This is shorter and easier to understand because there is only one terminating condition for the loop instead of two.

A possible improvement might be to run the loop until one of the last two positions it reached, because then the test against the array count is done only once per iteration and not twice:

while location < c.count - 2 {
    if c[location + 2] == 0 {
        location += 2
    } else {
        location += 1
    }
    jump += 1
}
return location == c.count - 1 ? jump : jump + 1

A further optimization would be to use that if we do a size 1 jump because of a thunderhead, the following jump will always be of size 2:

while location < c.count - 2 {
    if c[location + 2] == 0 {
        // Jump two positions:
        location += 2
        jump += 1
    } else {
        // Jump one position and then two positions:
        location += 3
        jump += 2
    }
}
return location == c.count - 1 ? jump : jump + 1

But the performance increase will probably be negligible since the arrays have at most 100 elements.

Some minor points: The parentheses in

if (location + 2 < c.count) &&
    (c[location + 2] == 0)

are not needed because the comparison operators have a higher precedence than the logical operators:

if location + 2 < c.count && c[location + 2] == 0

Finally, I would name the counter variable jumps or jumpCount to better describe its purpose.


Creating enchantment objects based on configuration I'm searching a way to literally shorten my code.
For the moment my code is this;

    if(getDashConfig().getBoolean("enchantments.multifirearrow.enabled")){
        MultiFireArrow fire = new MultiFireArrow();
        DashEnchant.getEnchants().put(1, fire);
        DashEnchant.getStringEnchants().put("multifirearrow", fire);
        if(debug()){
            getLogger().info("MultiFireArrow enchant registred");
        }
    if(getDashConfig().getBoolean("enchantments.bolt.enabled")){
        Bolt bolt = new Bolt();
        DashEnchant.getEnchants().put(2, bolt);
        DashEnchant.getStringEnchants().put("bolt", bolt);
        if(debug()){ getLogger().info("Bolt enchant registred"); }
        }
    if(getDashConfig().getBoolean("enchantments.randomspeed.enabled")){
        SpeedRandom speed = new SpeedRandom();
        DashEnchant.getEnchants().put(3, speed);
        DashEnchant.getStringEnchants().put("randomspeed", speed);
        if(debug()){ getLogger().info("RandomSpeed enchant registred"); }

Not that presentable.. uh?
How i would literally shorten it? I don't want 350 lines like that.

Assuming you already have a class called Enchant (should be a superclass of your enchantments), you could use an enum to organize all of your enchantments:

public enum Enchantments {
    MULTI_FIRE(1, "multifirearrow") {
        Enchant getEnchant() { return new MultiFireArrow(); }
    }, BOLT(2, "bolt") {
        Enchant getEnchant() { return new Bolt(); }
    }, RANDOM_SPEED(3, "randomspeed") {
        Enchant getEnchant() { return new SpeedRandom(); }
    };
    //Add more enchantments here!
    abstract Enchant getEnchant();
    public void enchant() {
        Enchant enchant = getEnchant();
        DashEnchant.getEnchants().put(this.index, enchant);
        DashEnchant.getStringEnchants().put(this.name, enchant);
        if(debug()) getLogger().info(this.toString() + " enchant registered");
    }
    public boolean isEnabled() {
        return getDashConfig().getBoolean("enchantments." + this.name + ".enabled");
    }
    private String name;
    private int index;
    private Enchantment(int index, String name) {
        this.index = index;
        this.name = name;
    }
}

and then do something like:

for(Enchantment e : Enchantment.values())
    if(e.isEnabled()) e.enchant();

You could make the code even shorter using reflection and enum ordinals but there is a point where shortening the code doesn't really help anymore (plus the added speed penalty.) Nonetheless: (replace MY_PACKAGE_NAME with your package)

public enum Enchantment {
    MULTI_FIRE, BOLT, SPEED_RANDOM;
    //Add more enchantments here!
    public void enchant() {
        Enchant enchant = Class.forName(MY_PACKAGE_NAME +
                toUpperCamel(this.toString(), "_")).newInstance();
        DashEnchant.getEnchants().put(this.ordinal() + 1, enchant); //ordinal starts at 0
        DashEnchant.getStringEnchants().put(this.replaceAll("_", "").toLowerCase(), enchant);
        if(debug()) getLogger().info(this.toString() + " enchant registered");
    }
    public boolean isEnabled() {
        return getDashConfig().getBoolean("enchantments." +
                this.toString().replaceAll("_", "").toLowerCase() + ".enabled");
    }
    private static String toUpperCamel(String a, String spl) {
        StringBuilder b = new StringBuilder(a.length());
        for(String s : a.split(spl)) {
            b.append(Character.toUpperCase(s.charAt(0)));
            b.append(s.substring(1).toLowerCase());
        } return b.toString();
    }
}


Tree heap Haskell code I'd like a review of Haskell tree heap code in Turning a tree into a heap in Haskell.

module Heapify where

data Tree a = Leaf a | Node (Tree a) a (Tree a)
   deriving Show

ourTree = Node (Node (Leaf 8) 2 (Leaf 4))  3  (Node (Leaf 1) 7 (Leaf 9))

atTop :: Tree a -> a
atTop (Leaf a) = a
atTop (Node _ a _) = a

replaceTop :: Ord a => Tree a -> a -> Tree a
replaceTop (Leaf _) a = Leaf a
replaceTop (Node l _ r) a = heapify (Node l a r)

adjustLeft :: Ord a => Tree a -> Tree a
adjustLeft (Leaf a) = Leaf a   -- But we shouldn't ask to do this.
adjustLeft  node@(Node l a r)
     | topL <= a = node
     | otherwise = Node (replaceTop l a) topL r
         where topL = atTop l

adjustRight :: Ord a => Tree a -> Tree a
adjustRight (Leaf a) = Leaf a   -- But we shouldn't ask to do this.
adjustRight  node@(Node l a r)
     | topR <= a = node
     | otherwise = Node l topR (replaceTop r a)
         where topR = atTop r

doTop :: Ord a => Tree a -> Tree a
doTop (Leaf a) = Leaf a
doTop node@(Node l a r)
    | atTop l > atTop r = adjustLeft node
    | otherwise         = adjustRight node

heapify :: Ord a => Tree a -> Tree a
heapify (Leaf a) = Leaf a
heapify (Node l a r) = doTop (Node (heapify l) a (heapify r))

Correctness? Unanticipated cases?

module Heapify where

data Tree a = Leaf a | Node (Tree a) a (Tree a)
   deriving Show

I think this can't host an even number of elements. fromList [1,2] seems impossible.

ourTree = Node (Node (Leaf 8) 2 (Leaf 4))  3  (Node (Leaf 1) 7 (Leaf 9))

an example data.

atTop :: Tree a -> a
atTop (Leaf a) = a
atTop (Node _ a _) = a

ok.

replaceTop :: Ord a => Tree a -> a -> Tree a
replaceTop (Leaf _) a = Leaf a
replaceTop (Node l _ r) a = heapify (Node l a r)

produces a heap. doesn't know what data was put on top, must re-heapify. Does not use the knowledge whether l/r are in fact heaps already or not. If was called on a heap,  both must have been heaps already. The usual flow would be for heapify to be called on arbitrary trees, but replaceAtTop to be called on heaps only. This might have an impact on performance.

adjustLeft :: Ord a => Tree a -> Tree a
adjustLeft (Leaf a) = Leaf a   -- But we shouldn't ask to do this.
adjustLeft  node@(Node l a r)
     | topL <= a = node
     | otherwise = Node (replaceTop l a) topL r
         where topL = atTop l

assumes l was already a heap. If not, l might harbor some number yet bigger than topL and the otherwise clause could produce a non-heap (replaceTop fully heapifies its argument so its biggest number will get floated to its top).

adjustRight :: Ord a => Tree a -> Tree a
adjustRight (Leaf a) = Leaf a   -- But we shouldn't ask to do this.
adjustRight  node@(Node l a r)
     | topR <= a = node
     | otherwise = Node l topR (replaceTop r a)
         where topR = atTop r

similarly to the above, assumes r was a heap. Both functions assume both l and r were heaps actually, because r (corr., l) is kept unchanged. So both assume that only the top element can be out of place before the call, and produce a heap under that assumption. If the assumption does not hold, the produced value will not be a heap.

doTop :: Ord a => Tree a -> Tree a
doTop (Leaf a) = Leaf a
doTop node@(Node l a r)
    | atTop l > atTop r = adjustLeft node
    | otherwise         = adjustRight node

assuming both l and r were heaps before call, produce a heap.

heapify :: Ord a => Tree a -> Tree a
heapify (Leaf a) = Leaf a
heapify (Node l a r) = doTop (Node (heapify l) a (heapify r))

assuming heapify fulfills its promise, OK. Base case (Leaf a): OK.

So, OK (except for the data definition deficiency).

Call graph

 heapify                             (does not assume l/r were heaps)
    |
    |_____  heapify
    |
    |_____  doTop
            |
            |____ adjustLeft / adjustRight    (assume l/r were heaps)
                  |
                  |____ replaceTop   (does not assume l/r were heaps)
                        |
                        |_____ heapify

Performance?

So, heapify essentially is

heapify (Leaf a) = Leaf a
heapify (Node l a r)
   | a >= top    = Node lh a rh
   | ltop > rtop = Node (replaceTop lh a) ltop rh                 -- lh is a heap!
   | otherwise   = Node lh                rtop (replaceTop rh a)  -- rh is a heap!
  where
    lh   = heapify l    ----- superfluous, == id when called from (1)    -- (2)
    rh   = heapify r    ----- superfluous, == id when called from (1)    -- (2)
    ltop = atTop lh
    rtop = atTop rh
    top  = max ltop rtop
    replaceTop (Leaf _)     a = Leaf a
    replaceTop (Node l _ r) a = heapify (Node l a r)   ------- (1)

Looks a very heavy recursion.

One way to fix this is to assume that heapify will be called only on heaps, not on arbitrary trees. Then the fix is simply to eliminate the two (2) calls. Then it  becomes logarithmic.

Heaps would need to be created from lists. The usual way is fold/insertElem.
The data type definition needs to be adjusted to allow insertion of one element into a heap tree. The definitions will have to be readjusted. Probably easier done with small snippets, like in OP code. atTop would have to produce a Maybe value, as now empty heaps become a possibility (say, one of the children of a fromList [1,2] heap tree - now both subtrees are heaps, and one is empty).
The whole balance/depth issue is untouched here.

It's probably simpler to re-write replaceTop to essentially duplicate the above code, without the calls to heapify. It will now assume it's operating on heaps only, not on arbitrary trees. This will make replaceTop logarithmic. Leaves all the other problems unaddressed.

replaceTop (Leaf _)       a = Leaf a      -- operates on heaps!
replaceTop (Node lh _ rh) a               -- lh, rh are heaps!
   | a >= top       = Node lh a rh
   | ltop > rtop    = Node (replaceTop lh a) ltop rh
   | otherwise      = Node lh                rtop (replaceTop rh a)
  where
    ltop = atTop lh
    rtop = atTop rh
    top  = max ltop rtop

According to master theorem for 2T(n/2) + O(log(n)) case, heapify will be O(n) then, assuming a balanced tree. A balanced tree can be built from a list in O(n) time.

removeTop will most probably be O(log(n)).


Is this page dispatcher secure/fast? I'm using a index system to say:

On index i use:

<?php core::doPage(); ?>

Core code is:

<?php
class core
{
    public static function doPage()
    {
        if(!isset($_GET["page"]) || empty($_GET) || !is_string($_GET["page"]))
        {
            if(!include_once('./_pages/index.php')) die("404 - File not found.");
        }
        else
        {
            $_GET["page"] = substr($_GET["page"], 0, -4);
            $pages = array_map(function($x) { return str_replace(".php", "", basename($x)); }, glob("./pages/*.php"));
            if(in_array($_GET["page"], $pages))
            {
                if(!include_once("./_pages/{$_GET["page"]}.php")) die("404 - File not found.");
            }
            else
                if(!include_once('./_pages/404.php')) die("404 - File not found.");
        }
    }
}
?>

Any tweak suggestions or is this code fairly secure? Not exploitable?

There are no glaring security issues I can see in the original code, but I felt it could be improved.

<?php

core::doPage();

class core
{
    public static function doPage()
    {

        // i hate having $_GET littered through my code,
        // it is too easy to mishandle non-sanitized data
        $request_page = isset($_GET["page"]) ? $_GET["page"] : null;

        // why die here, why not use our standard page handling
        if(empty($request_page) || !is_string($request_page)) {
            $request_page = 'index.php';
        }

        // we take .php extenstion off, then tack it back on later, whats the point?,
        // just leave it on
        // if you have 1000's of files glob + in_array test will be a slow way to check
        // give your variables a more descriptive name other then $pages,
        // so that when someone else edits your code,
        // they can tell what you are trying to do
        $pages_whitelist = glob("*.php");

        // test and alter your request_page, just do one include at the end
        if(!in_array($request_page, $pages_whitelist)) {
            $request_page = '404.php';
        }

        // include_once triggers a php warning if the file doesn't exist,
        // why not use file_exists?
        $file = "./_pages/$request_page";

        if (!file_exists($file)) {
            die("404 - File not found.");
        }

        // yay, no more nested if statements
        // a lot cleaner and easier to read (i hope)
        include_once($file);
    }

}


Create a binary tree, code review request Ok, code reviewers, I want you to pick my code apart and give me some feedback on how I could make it better or more simple. ( Generics would be added a bit later).

public class CreateABinaryTree {
    private TreeNode root;

    public CreateABinaryTree() {
    }

    /**
     * Constructs a binary tree in order of elements in an array.
     * After the number of nodes in the level have maxed, the next
     * element in the array would be a child of leftmost node.
     *
     * http://codereview.stackexchange.com/questions/31334/least-common-ancestor-for-binary-search-tree/31394?noredirect=1#comment51044_31394
     */
    public CreateABinaryTree(List<Integer> items) {
        this();
        create(items);
    }

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

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

    private void create (List<Integer> items) {
        root = new TreeNode(null, items.get(0), null);

        final Queue<TreeNode> queue = new LinkedList<TreeNode>();
        queue.add(root);

        final int half = items.size() / 2;

        for (int i = 0; i < half; i++) {
            if (items.get(i) != null) {
                final TreeNode current = queue.poll();
                final int left = 2 * i + 1;
                final int right = 2 * i + 2;

                if (items.get(left) != null) {
                    current.left = new TreeNode(null, items.get(left), null);
                    queue.add(current.left);
                }
                if (right < items.size() && items.get(right) != null) {
                    current.right = new TreeNode(null, items.get(right), null);
                    queue.add(current.right);
                }
            }
        }
    }
}

I only see one really questionable thing in your code.  Why do you call this() in your constructor?  Your parameter-less constructor does absolutely nothing, so this line is irrelevant.  If anything, I would actually even remove that constructor entirely so that your API can only be used with your meaningful and useful one.

Also, as a side note, the following line opens you up to a NullPointerException since you never check to see if items is null or if it has any elements.

root = new TreeNode(null, items.get(0), null);


JSON parsing service I created the following service for the purpose of sending in a file for parsing. I'm using the generics so that when I call parse() I can pass a custom type conforming to Codable. I'm not a huge fan of the repetition of do {} catch {} but I don't see any other way to avoid this.
import Foundation

enum JSONError: Error {
    case read,
         parse
}

struct JSONService {
    let fileName: String

    func parse<T: Decodable>(type: T.Type) -> T? {
        do {
            let fileContents = try self.getJsonContents()
            return try self.parseJson(data: fileContents, type: T.self)
        } catch {
            fatalError("\(error.localizedDescription)")
        }
    }

    private func getJsonContents() throws -> Data {
        if let file = Bundle.main.path(forResource: self.fileName, ofType: "json") {
            if let data = try String(contentsOfFile: file).data(using: .utf8) {
                return data
            }
        }

        throw JSONError.read
    }

    private func parseJson<T: Decodable>(data: Data, type: T.Type) throws -> T {
        do {
            return try JSONDecoder().decode(type.self, from: data)
        } catch {
            throw JSONError.parse
        }
    }
}

Your method reads an object from a JSON file which is embedded in the application bundle, and aborts with a runtime error if that fails for any reason. That is fine: Bad embedded data are a programming error and should be detected early.
The return type of the parse() method should not be an optional because it never returns nil.
The custom error type is used to pass errors from the helper methods to the main method. That has two disadvantages:

It hides (possible more informative) errors thrown from other methods, e.g. the JSON decoder method.

The messages printed with fatalError() show only the error type and an integer, for example
Fatal error: The operation couldn’t be completed. (MyApp.JSONError error 0.)

The second issue can be improved (see for example How to provide a localized description with an Error type in Swift? on Stack OverFlow) but I would go another route and pass on existing errors instead, where possible:
In parseJson() you can pass the error which the JSON decoder already throws, instead of shadowing it by a custom error:
private func parseJson<T: Decodable>(data: Data, type: T.Type) throws -> T {
    try JSONDecoder().decode(type.self, from: data)
}

And in getJsonContents() you can use the (throwing) Data(contentsOf: url) method to read the data, that is also simpler than reading into a string and then converting the string to data.
It remains to handle an error in Bundle.main.url(forResource:withExtension:), that method does not throw an error but returns an optional.
Without the do-catching in the helper methods everything fits neatly into the main method:
func parse<T: Decodable>(type: T.Type) -> T {
    guard let url = Bundle.main.url(forResource: self.fileName, withExtension: "json") else {
        fatalError("file not found")
    }
    do {
        let data = try Data(contentsOf: url)
        return try JSONDecoder().decode(type.self, from: data)
    } catch {
        fatalError("\(error.localizedDescription)")
    }
}

You many also want to print the full error and not its localizedDescription because that can provide more information (and the string is not presented to the user of your program but only for diagnostic purposes).
Finally, I wonder if fileName should really be an instance variable. If it is only used in the parse() method and nowhere else then you can make it a parameter of a static  method:
struct JSONService {
    static func parse<T: Decodable>(type: T.Type, from fileName: String) -> T {
        guard let url = Bundle.main.url(forResource: fileName, withExtension: "json") else {
            fatalError("file not found")
        }
        do {
            let data = try Data(contentsOf: url)
            return try JSONDecoder().decode(type.self, from: data)
        } catch {
            fatalError("\(error)")
        }
    }
}

which would be called as
let value = JSONService.parse(type: MyType.self, from: "filename")

Embedding it into a struct JSONService is still useful as it provides a “name space“ for its static methods. If there are only static methods then you can embed them in an enum JSONService instead, because no instance of that type needs to be created (thanks to @Shadowrun for reminding me about that).


best way to write series of objects to a .ser file using ObjectOutputStream and read them back I create series of objects out of Student Class and store them in a vector. I write each object in to a .ser file once it created. Then I read them back. My code is working perfectly. What I want to know is, I do this in a correct optimized way or are there any easy and optimized ways to get this done?? And also how to replace or delete specific object in a .ser file without re-witting whole the file again.

Student Class

public class Student implements Comparable<Student>, Serializable{

    private String firstName = "";
    private int registrationNumber;
    private int coursework1;
    private int coursework2;
    private int finalExam;
    private double moduleMark;

    public String getFirstName() {
        return firstName;
    }
    public void setFirstName(String firstName) {
        this.firstName = firstName;
    }
    public int getRegistrationNumber() {
        return registrationNumber;
    }
    public void setRegistrationNumber(int registrationNumber) {
        this.registrationNumber = registrationNumber;
    }
    public int getCoursework1() {
        return coursework1;
    }
    public void setCoursework1(int coursework1) {
        this.coursework1 = coursework1;
    }
    public int getCoursework2() {
        return coursework2;
    }
    public void setCoursework2(int coursework2) {
        this.coursework2 = coursework2;
    }
    public int getFinalExam() {
        return finalExam;
    }
    public void setFinalExam(int finalExam) {
        this.finalExam = finalExam;
    }
    public double getModuleMark() {
        return moduleMark;
    }
    public void setModuleMark(double moduleMark) {
        this.moduleMark = moduleMark;
    }
    public int compareTo(Student s){
        if (this.moduleMark > s.moduleMark)
            return 1;
        else if (this.moduleMark == s.moduleMark)
            return 0;
        else
            return -1;
    }
}

File writing part

public static void Write(Student mm){
        try
          {
            FileOutputStream fileOut = new FileOutputStream("info.ser",true);
            ObjectOutputStream out = new ObjectOutputStream(new BufferedOutputStream(fileOut));

            out.writeObject(mm);
            out.close();
            fileOut.close();
            System.out.println("Serialized data is saved in info.ser");
          }catch(IOException i)
          {
              //i.printStackTrace();
          }

    }

Reading part

public static int Read() {
        int count=0;
        try{
            vector = new Vector<Student>();
            FileInputStream saveFile = new FileInputStream("info.ser");
            ObjectInputStream save;
            try{
                for(;;){
                    save = new ObjectInputStream(saveFile);
                    student = (Student) save.readObject();
                    vector.add(student);
                    count++;
                }
            }catch(EOFException e){
                //e.printStackTrace();
            }
            saveFile.close();

        }catch(Exception exc){
            exc.printStackTrace();
        }
        return count;
    }

Why didn't you provide a constructor for Student class? Was that on purpose? However...

Use serialVersionUID.
In Java it's recommended not to use Vector class, instead use ArrayList.
Write method is serializing a Student individually. But as you stated you are writing a whole list of student into the .ser file. This will take many time for a list of 10,000 students.

Because you are opening two streams for each student. So instead pass the whole list and serialize them after opening the stream only once.
Do not close the stream in the try block. close them into finally block. Even better if you are using Java7 or higher use try-with-resources block. It is easy to use and you don't have to manually close the stream.
In Read method

        for(;;){
            save = new ObjectInputStream(saveFile);

you are creating new stream for every object thats not necessary instead do this

        save = new ObjectInputStream(saveFile);
        for(;;){

Onwards Java7 there is multiple catch statement. Give it a try(pun intended).
In Java we use mixed-case convention for naming a method. So Write and Read would be write and read.


Minimum number of parentheses to be removed to make a string of parentheses balanced The task:

  Given a string of parentheses, write a function to compute the minimum
  number of parentheses to be removed to make the string valid (i.e.
  each open parenthesis is eventually closed).

  For example, given the string "()())()", you should return 1. Given
  the string ")(", you should return 2, since we must remove all of
  them.

const brackets = "()())()";

My functional solution:

const numberOfUnbalanced = brackets => Object.values(brackets
  .split("")
  .reduce((brackCounter, b) => {
    b === "(" ? brackCounter.openBrackets++ :
    brackCounter.openBrackets ? brackCounter.openBrackets-- :
    brackCounter.closedBrackets++;

    return brackCounter;
  }, {openBrackets: 0, closedBrackets: 0}))
.reduce((sum, b) => sum + b, 0);

console.log(numberOfUnbalanced(brackets));

My imperative solution:

function numberOfUnbalanced2(brackets) {
  let openBrackets = 0, closedBrackets = 0;
  for (let i in brackets) {
    brackets[i] === "(" ? openBrackets++ :
    openBrackets ? openBrackets-- :
    closedBrackets++;
  }
  return openBrackets + closedBrackets;
}

console.log(numberOfUnbalanced2(brackets));

Usually the functional approach is shorter and tend to be easier to understand in comparison to imperative approaches. However, in this case it doesn't have any advantage to the imperative approach. I guess it is due to the nested  structure in the functional solution.

for of not for in

I have noticed that on occasion you use the indexing for in Try to avoid using this loop as it can be problematic if the object you iterate has inherited properties not set as enumerable = false

Use the values iterator for of as it avoids the problems that come with for in. It also has the benefit of not needing to index the object for the value

Use const

When using either for of and for in and you don't intend to change the value or key use a const  eg for(const i in brackets) or for(const char of brackets)

Note That you can not use a const in for(;;) loops but let is allowed. for(let i=0;i<10;i++). The reason is that even though a new instance of i is created each iteration and assigned the value of the previous i, the last loop expression i++ is applied at the bottom of the loop block and thus does not work for constants.

Simplify

Strings are iterable objects so you can avoid the need to use String.split and use the more succinct spread ... operator. eg [...string] is the same as string.split("");

Complexity

Your imperative function is \$O(n)\$ time and \$O(1)\$ space.
Your declarative (you call functional) function is \$O(n)\$ time and \$O(n)\$ space.

Ambiguity

The question does not indicate if the string will contain characters other than "()", yet the example shows only "()" and your solutions count characters other than "()" as ")" so will assume that the input string contains only "()"

Solutions

Imperative

function balanced(str) {
    var open = 0, closed = 0;
    for (const char of str) { char === "(" ? open++ : (open ? open-- : closed++) }
    return open + closed ;
}

Declarative

function balanced(str) {
    const open = counts => (counts[0]++, counts);
    const close = counts => (counts[0] ? counts[0]-- : counts[1]++, counts);
    const counter = (counts, char) => char === "(" ? open(counts) : close(counts);
    const sum = arr => arr[0] + arr[1];
    const chars = str => [...str];
    return sum(chars(str).reduce(counter, [0, 0]));
}

Functional

function balanced(str) {
    const counter = ([open, closed], char) => {
        char === "(" ? open++ : (open ? open-- : closed++);
        return [open, closed];
    }
    const sum = (sum, val) => sum += val;
    return [...str]
       .reduce(counter, [0, 0])
       .reduce(sum, 0);
}

or

function balanced(str) {
    const counter = ([open, closed], char) =>
        (char === "(" ? open++ : (open ? open-- : closed++), [open, closed]);
    const res = [...str].reduce(counter, [0, 0])
    return res[0] + res[1];
}


Simple Stack implementation in C++ I've recently started teaching myself basic C++ and decided to implement a simple stack with pointers.

#include <iostream>

using namespace std;

struct StackElement {
    char value;
    StackElement* next;

    StackElement(char value, StackElement* next) : value(value), next(next) {}
};

struct Stack {
    StackElement* top = NULL;

    bool isEmpty() { return top == NULL; }

    void push(char value) {
        StackElement* newElement = new StackElement(value, top);

        top = newElement;
    }

    StackElement pop() {
        StackElement* toBeDeleted = top;
        StackElement toBeReturned = *top;

        top = top->next;
        delete toBeDeleted;
        return toBeReturned;
    }
};

int main() {
    Stack* stack = new Stack();
    cout << "Created a stack at " << &stack << endl;

    int number_of_inputs;
    cout << "Enter the number of elements you want to push at the stack: ";
    cin >> number_of_inputs;

    for (int i = 0; i < number_of_inputs; i++) {
        char input;
        cin >> input;
        stack->push(input);
    }

    cout << "- - - - - - - - - - - - - - - - " << endl;
    cout << "Displaying content of the stack: " << endl;

    while (!stack->isEmpty()) {
        cout << stack->pop().value << endl;
    }

    return 0;
}

My questions are:
- what could be generally done better here?
- is the pop() method written correctly? Does it create any memory leaks? Is there a way to write it shorter?

Thank you in advance! (And forgive use of using namespace std)

Your stack implementation is terrible, and so is @hc_dev: neither handles memory correctly.

Resource Handling

It is generally frowned upon to call new and delete directly, simply because doing it correctly is hard.

Proper resource handling requires:

Thinking about moves.
Thinking about copies.
Thinking about destruction.

This used to be called the Rule of 3 in C++03 (Copy Constructor, Copy Assignment Operator and Destructor) and is called the Rule of 5 since C++11 (+Move Constructor, +Move Assignment Operator).

Your current Stack implements neither of those 5 operations correctly -- it doesn't implement them at all, and the default generated operations are buggy due to your use of a raw pointer.

The best advice for resource handling, though, is to use the Rule of Zero: just delegate it to something that works!

In your case, look into std::unique_ptr and std::make_unique!

Corrected resource management:

struct StackElement {
    char value;
    std::unique_ptr<StackElement> next;

    StackElement(char value, std::unique_ptr<StackElement> next) :
        value(value), next(std::move(next)) {}
};

struct Stack {
    std::unique_ptr<StackElement> top = nullptr;

    bool isEmpty() { return top == nullptr; }

    void push(char value) {
        top = std::make_unique<StackElement>(value, std::move(top));
    }

    char pop() {
        assert(!isEmpty());

        char toBeReturned = top->value;

        top = std::move(top->next);

        return toBeReturned;
    }
};

This struct is no longer copiable, as std::unique_ptr is not copiable.

Limited stack depth.

The previous rewrite is good, but its destructor suffers from stack overflow (!).

That is, when the destructor is executed, you get:

Call destructor of Stack
Call destructor of Stack::top
Call destructor of StackElement (stack->top)
Call destructor of StackElement::next.
Call destructor of StackElement (stack->top->next)
...

To handle this, create a clear method, and manually write the destructor.

struct Stack {
    // ...

    Stack(Stack&&) = default; // automatic generation is disabled when
                              // the destructor is explicit, so explicitly
                              // ask for it.

    Stack& operator=(Stack&&) = default; // automatic generation...

    ~Stack() { clear(); }

    void clear() {
        while (!isEmpty()) {
            pop();
        }
    }
};

General

Once you have the memory part correct, further improvements:

Encapsulation: do not expose your privates.
Generalization: make it work for any type.

This yields:

//  No need for a class here, it's internal.
template <typename T>
struct StackElement {
    StackElement(T value, std::unique_ptr<StackElement> next):
        value(std::move(value)), next(std::move(next)) {}

    T value;
    std::unique_ptr<StackElement<T>> next;
};

template <typename T>
class Stack {
public:
    ~Stack() { this->clear(); }

    Stack() = default;

    Stack(Stack&&) = default;
    Stack& operator=(Stack&&) = default;

    bool isEmpty() const { return this->head == nullptr; }

    T const& top() const {
        assert(!this->isEmpty());

        return this->head->value;
    }

    void clear() {
        while (!isEmpty()) {
            this->pop();
        }
    }

    void push(T value) {
        //  Create empty node first, in case moving `value` throws an exception.
        auto neo = std::make_unique<StackElement<T>>(std::move(value), nullptr);

        neo->next = std::move(this->head);
        this->head = std::move(neo);
    }

    T pop() {
        assert(!isEmpty());

        //  Pop top first, in case moving `current->value` throws an exception.
        auto current = std::move(this->head);
        this->head = std::move(current->next);

        return std::move(current->value);
    }

private:
    std::unique_ptr<StackElement<T>> head;
};

Miscellaneous

There are few nits in your main:

There is no need to allocate Stack on the heap, just Stack stack; works.
Don't use std::endl, just use '\n' or "\n".

std::endl both appends \n and calls flush, the latter kills all performance benefit of internally buffering.

With that in mind, the rewritten main is:

int main() {
    Stack<char> stack;
    std::cout << "Created a stack at " << &stack << "\n";

    int number_of_inputs;
    std::cout << "Enter the number of elements you want to push at the stack: ";
    std::cin >> number_of_inputs;

    for (int i = 0; i < number_of_inputs; i++) {
        char input;
        std::cin >> input;
        stack.push(input);
    }

    std::cout << "- - - - - - - - - - - - - - - - " << "\n";
    std::cout << "Displaying content of the stack: " << "\n";

    while (!stack.isEmpty()) {
        std::cout << stack.pop() << "\n";
    }

    return 0;
}


Java - propagating a threadlocal by wrapping a runnable My aim is to use a thread pool together with a thread local. I cannot use InheritableThreadLocal because the thread data is different per user and there a couple of parallel users at any time.

So I need to push the thread local down from the mother thread to the children.

I use guava so I want to get listenable futures back. Below is my implementation. Please look at the execute function where the logic happens.

My question is what could go wrong here?

import static com.google.common.base.Preconditions.checkNotNull;

import java.util.List;
import java.util.concurrent.ExecutorService;
import java.util.concurrent.TimeUnit;

import myorg.MyService;
import myorg.ThreadData;

import com.google.common.util.concurrent.AbstractListeningExecutorService;

public class ListeningDecorator extends AbstractListeningExecutorService {
    private final ExecutorService delegate;

    ListeningDecorator(final ExecutorService delegate) {
        this.delegate = checkNotNull(delegate);
    }

    @Override
    public final boolean awaitTermination(final long timeout, final TimeUnit unit) throws InterruptedException {
        return this.delegate.awaitTermination(timeout, unit);
    }

    @Override
    public final boolean isShutdown() {
        return this.delegate.isShutdown();
    }

    @Override
    public final boolean isTerminated() {
        return this.delegate.isTerminated();
    }

    @Override
    public final void shutdown() {
        this.delegate.shutdown();
    }

    @Override
    public final List<Runnable> shutdownNow() {
        return this.delegate.shutdownNow();
    }

    @Override
    public final void execute(final Runnable command) {
        final ThreadData threadData = MyService.getThreadData();
        if (threadData == null) {
            throw new RuntimeException("threadData is null");
        }
        Runnable runnable = new Runnable() {

            @Override
            public void run() {
                MyService.setThreadData(threadData);
                command.run();

            }
        };

        this.delegate.execute(runnable);
    }
}

It might be good to revert the ThreadLocal changes in the worker thread as soon as command.run() finished (using try-finally). This would have the following advantages:

If your ExecutorService delegate (provided to the constructor) is also used somwhere else for different tasks, then currently these tasks would be affected by the changes you make to the ThreadLocals as part of ListeningDecorator.execute. For example:
ExecutorService delegateExecutor = Executors.newFixedThreadPool(10);
ListeningDecorator listeningExecutor = new ListeningDecorator(delegateExecutor);

// Sets MyService thread data
Future<?> future = listeningExecutor.submit(() -> {
    ...
});
future.get();

// In different thread: Directly use delegateExecutor
delegateExecutor.execute(() -> {
    // Would still have MyService thread data even though this was submitted
    // by different thread
    ...
});

If the MyService thread data changes were reverted as soon as command.run() finished, then this would not be an issue.
Reverting the changes might allow the garbage collector to collect MyService thread data earlier. Otherwise it might linger around indefinitely if ExecutorService delegate reuses threads.


Simplifying and optimizing factors and prime factorization generator My first algorithm for finding the factors of a number was pretty slow and horrible (it started at \$O(n^2)\$, where n was not even the inputted number), but I eventually came up with this new code.  I just want the first factors of a number (not the duplicates), so I used sqrt(n) instead of n for the loop.  As for the prime factorization, it appears to be efficient but it could probably be improved.

Could this program be written simpler and more efficiently?
Are all of these std::stringstreams even necessary?
Is there a better way of having findPrimeFactorization() output in the form of w^x * y^z?

#include <iostream>
#include <vector>
#include <cmath>
#include <string>
#include <sstream>

using std::cin;
using std::cout;
using std::vector;
using std::string;
using std::stringstream;

string findFactors(unsigned, vector<unsigned> &factors);
string findPrimeFactorization(unsigned posInt);

int main()
{
    unsigned positiveInteger;

    cout << "\n\n> Pos. Integer: ";
    cin >> positiveInteger;

    vector<unsigned> factors;

    cout << "\n\n  * Factors:\n\n";
    cout << findFactors(positiveInteger, factors);

    cout << "\n  * Prime Factorization:\n\n      ";
    cout << findPrimeFactorization(positiveInteger) << "\n\n\n";

    system("PAUSE"); // I use Visual C++
}

string findFactors(unsigned posInt, vector<unsigned> &factors)
{
    std::stringstream factorsStr;
    double sqrtInt = sqrt(static_cast<double>(posInt));

    for (unsigned i = 1; i <= sqrtInt; i++)
    {
        if (posInt % i == 0)
        {
            factors.push_back(i);
            unsigned x = posInt / i;
            factorsStr << "      " << i << " x " << x << "\n";
        }
    }

    return factorsStr.str();
}

string findPrimeFactorization(unsigned posInt)
{
    std::stringstream primesStr;

    for (unsigned i = 2; i <= posInt; i++)
    {
        int powerDegree = 0;

        while (posInt % i == 0)
        {
            posInt /= i;
            powerDegree++;
        }

        if (powerDegree >= 1)
        {
            primesStr << i;

            if (powerDegree > 1)
                primesStr << '^' << powerDegree;
            if (i <= posInt)
                primesStr << " x ";
        }
    }

    return primesStr.str();
}

I have a couple of suggestions that you may wish to consider:

Don't return a string with the complete statement, return a std::vector/std::map instead with the values, and later assemble the string. It will keep the values in a more usable form and separate out the concerns. You're half doing this with findFactors, go all the way!
In findPrimeFactorization, your main loop has a terminating condition that varies both sides (i <= posInt), which is a little confusing to reason about. Instead, rewrite it to a while ( posInt != 1 ), which is your actual termination.
In findPrimeFactorization, to speed things up (slightly) you can try testing 2 separately, then 3, 5, 7... within the loop (other more complex shortcuts are available)
In terms of writing out the prime factors, you can combine the ideas behind the questions infix_iterator code and std::copy to std::cout for std::pair to get something that's nice, if a little obscure.


Converting Reverse Polish to Infix Notation in Java I am trying to solve a programming challange that involves converting reverse polish notation to infix notation. For example: 1 3 + 2 4 5 - + / would be: ((1+3)/(2+(4-5))) My solution so far does work, but it's not fast enough. So I am looking for any optimization advice.

public class betteralgo {
    public static void main(String[] args) throws IOException {
        BufferedReader bi = new BufferedReader(new
        InputStreamReader(System.in));
        String line = bi.readLine();
        String[] input = line.split(" ");
        StringBuilder builder = new StringBuilder();
        Stack<String> stack = new Stack<String>();

        for(String e:input) {
            switch(e){
                case("+"):
                case("-"):
                case("*"):
                case("/"):
                    String i = stack.pop();
                    String k = stack.pop();
                    stack.push("(" + k + e + i + ")");
                break;
                default:
                    stack.push(e);
                }
            }
        System.out.println(stack.pop());
       }
    }

use a recursive function
I guess avoiding the stack-juggling (because of the recursivity of fromRPN) and the unnecessary creation of String objects (with StringJoiner) should make it fast enough.

fromRPN( new ArrayList<String>( Arrays.asList( "1 3 + 2 4 5 - +".split( " " ) ) ) );  // ((1+3)/(2+(4-5)))

public String fromRPN( ArrayList<String> rpn ) {
  for( int n = 0; rpn.size() > 1; n++ )
    switch( rpn.get( n ) ) {
    case "+":
    case "-":
    case "*":
    case "/":
      String s = new StringJoiner( "", "(", ")" ).add( rpn.remove( n - 2 ) ).add( rpn.remove( n - 1 ) ).add( rpn.remove( n - 2 ) ).toString();
      rpn.add( n - 2, s );
      if( rpn.size() > 1 )
        return( fromRPN( rpn ) );
      return( rpn.get( 0 ) );
    case "sqrt":  // insert Your unary operator(s) here…
      StringJoiner join = rpn.get( n ).startsWith( "(" ) ? new StringJoiner( "" ) : new StringJoiner( "",  "(", ")" );
      join.add( rpn.remove( n ) );
      if( rpn.get( n - 1  ).startsWith( "(" ) )
        join.add( rpn.remove( n - 1 ) );
      else
        join.add( "(" ).add( rpn.remove( n - 1 ) ).add( ")" );
      s = join.toString();
      rpn.add( n - 1, s );
      if( rpn.size() > 1 )
        return( fromRPN( rpn ) );
      return( rpn.get( 0 ) );
    }
  return( fromRPN( rpn ) );
}

You can test both types of expressions (RPN and algebraic) with 7th stone


Social Media Hashtag Splitting I decided to try out Python (3.x) two or so weeks ago, and this is my first real script using it. The program I've written below is slow, clunky, inefficient, inaccurate, and probably poorly coded! But it produces what I want most of the time, and it was really just a project to become accustomed to different Python techniques.

What it should do is, recieve a string, one that looks like a tag from a social media site (#socialmedia, #20000leaguesunder, etc.), and then based on a dictionary, split the string into logical words.

So "#volunteeringtoday" becomes "volunteering today" and so on.

I understand my algorithm is poor, so any help with the actual Python and formatting would be preferred.

import re

class Splitter():

    input = ''
    clean = ''
    words = []

    def __init__(self, string):
        '''
        Set the input string
        '''
        self.input = string

    def split(self):
        '''
        Split the input string. Get the clean version then start the separation loop.
        '''
        self.clean = self.cleanString(self.input)
        return self.separateString(self.clean)

    def separateString(self, inp):
        '''
        Separate the string input. Break apart and look for matches in a dictionary.
        '''
        # Index each character in the input string
        for ind in range(len(inp)):
        # Build a segment
        built = self.partition(inp, ind)
        #print('"'+built+'"') # Add for details
        # If only one letter remains, steal a letter from the previous match
        if len(built) == 1:
            built = self.words[-1][-1] + self.lastLarge
            self.words[-1] = self.words[-1][0:-1]
            return self.separateString(built)
        # Check if segment ends with a digit. Separate it if it does.
        if re.match('\d+$', built):
            built = re.sub("([^\d])\d", "", built)
            self.words.append(built)
            return self.separateString(inp.replace(built, '', 1))
        # It does not end with a digit
        else:
            # Iterate over each line in the dictionary
            for word in open('word.txt', 'r'):
                if self.cleanString(word) == built:
                    self.words.append(built)
                    # Check if list of separations joined together is equal to original
                    if ''.join(self.words) == self.clean:
                        return ' '.join(self.words)
                    else:
                        # Loop back through to separate more
                        self.lastLarge = inp.replace(built, '', 1)
                        return self.separateString(inp.replace(built, '', 1))

    def partition(self, value, index):
        '''
        Break apart string. Length from right
        '''
        return value[0:len(value) - index]

    def cleanString(self, string):
        '''
        Remove all non-alphabetic and non-digit characters.
        '''
        return re.sub("[^\d[a-z]]*", "", string.lower().strip())

And then you can sample it with something such as:

splitter = Splitter(input('Hashtag: '))
print('\nFinal:',splitter.split())

word.txt is a text file with roughly 60k words. One on each line. The list can be found here.

Any tips on making it more professional?

Bugs
Try some simple test cases to find bugs:
def test_split(s):
    splitter = Splitter(s)
    print(s, "⇒", splitter.split(s))

test_split('[')        #no word, just punctuation
test_split('cat')
test_split('cattree')
test_split('cathode')  #one word, not cat-hode
test_split('muscat')   #one word, not mus-cat
test_split('onetwothree')
test_split('one1two2') #multiple numbers
test_split('2014')     #multi-digit number
test_split('xyzzy')    #unknown word
test_split('cathodemuscat') #cathode-muscat, not cathodemus-cat or cat-hodemuscat

These reveal some bugs:

Only the first call to split works (even on separate instances). This is because Splitter.words is shared between instances but is modified by separateWords, and words remain in it after each call.

Unknown words are omitted. What should separateString return when it doesn't recognize a word? The whole input?

Words containing numbers are not handled in any reasonable way. Perhaps any sequence of digits should be parsed as a word?

Brackets are treated as letters. The regex in cleanString should be "[^\da-z]*", not "[^\d[a-z]]*", unless you want to keep square brackets.

When modifying separateString, be careful with the case where the whole input as one word — it's easy to break this or have it only work by accident.
The last test case ('cathodemuscat') is particularly hard. Hint: split the string, recurse on both halves, and check that both halves succeed.
Architecture and simplification
The Splitter class does nothing useful. Its methods should be top-level functions instead. (This contributes to the Splitter.words bug.)
It's not a good idea for separateString to communicate with its recursive call by modifying input and words. This is bug-prone (it contributes to the Splitter.words bug) and hard to read. Instead, separateString should pass the appropriate substring as an argument to the recursive call, and append its word to the result. It may look something like this:
return separateString(inp[:-ind]) + [word]

Callers probably want a list of words, rather than a space-separated string, so separateString should return the list without joining.
It would be clearer (and faster) to load the dictionary once, as a set, rather than reading it repeatedly:
dictionary = set([cleanString(w) for w in open('word.txt')])

partition is unnecessary. Just write inp[len(inp)-ind:], or even inp[-ind:] if ind is never 0. (Negative indexes count from the end.)


Is this node replacement code reasonable? JSFiddle: http://jsfiddle.net/yMSHa/ (Note: Real code uses JQuery 1.4.1)

HTML:

Do It A 1

Do It A 2

Do It A 3

Do It A 4

Do It A 5

Do It B

JS:

var RepDiv = $('#DoIt2');
var OldDiv = null;

$('div[data-name="action"]').each(function (i, f) {
    var actionid = $(f).attr('data-id');
    if (actionid) {
        $(f).css('cursor', 'pointer');
        var OnClick = function () {
            if (OldDiv) {
                RepDiv.replaceWith(OldDiv);
                OldDiv.click(OnClick);
            }
            OldDiv = $(this).replaceWith(RepDiv);
        };
        $(f).click(OnClick);
    }
});

Below is some simple JavaScript which replaces one node at a time when that node is clicked.  The idea behind this code is that a user can click on a node for that node to be expanded with more options.

I'm looking for a general review.  My main concern is whether this code is written properly in terms of good practices.

From a once over;

i and f, even for forEach are not very good names, pick something more meaningful.
if data-id is the action id, perhaps you could have gotten/set it with $().data('actionId') ?
The way your replacement function uses OldDiv and RepDiv smells like global variable abuse
Consider using smallCamelCase, actionid -> actionId, OldDiv -> oldDiv
Consider caching $(f), since you access it several times ( var $f = $(f); and then only access $f )
This : $(f).css('cursor', 'pointer'); really belongs in .css file

Other than that, your code looks okay to me.


Basic implementation of suffix in Go I'm learning string algorithms and I wanted to optimize the basic suffix algorithm which I implemented.
suffix.go
package main

import (
  "fmt"
)

func main(){
  str := "Hello"
  arr := []string{}
  for i:=len(str)-1; i >=0 ; i--{
    var s string
    s = s+str[i:]
    fmt.Println(s)
    arr = append(arr, s)
  }
}

The problem with the above code is, every time I'm creating a new string. I'm trying to avoid it but was not able to do.

There is no need to create a new string at each iteration. You can directly append the subslice to the array like this:

arr = append(arr, str[i:])

An other easy optimisation would be to create an array with the correct capacity to save an allocation at each iteration, as we know that arr will have the same length as str:

arr := make([]string, len(str))

so the code can now be rewritten like this:

func new() {
    str := "Hello"

    arr := make([]string, 0, len(str))
    for i := len(str) - 1; i >= 0; i-- {
        arr = append(arr, str[i:])
    }
}

Running a small benchmark comparing the old and the new implementation, we get this result:

goos: linux
goarch: amd64
BenchmarkOld-2           3000000               553 ns/op             240 B/op          4 allocs/op
BenchmarkNew-2          10000000               127 ns/op              80 B/op          1 allocs/op


Haskell creating a greedy zip function I am trying to implement a greedy zip method in Haskell where it does not drop the elements, instead, it will use some default value. I appreciate any feedback to make it more compact.

zip' [-1] [] r = r
zip' [] [-1] r = r

zip' [] b r  = zip' [-1] b r
zip' a [] r  = zip' a [-1] r

zip' a b r  = (xa, xb) : (zip' xsa xsb r)
  where
    (xa: xsa) = a
    (xb: xsb) = b

main = do
  let l1 = [1, 2, 3]
  let l2 = [3, 4]
  print (zip' l1 l2 [])
  --  expected: [(1,3),(2,4),(3,-1)]

r is always []. Simpler library functions can do some of the work.

zip' [] b = map (-1,) b
zip' a [] = map (,-1) a
zip' (x:xs) (y:ys) = (x, y) : zip' xs ys


Raspberry Pi Safe Clean Up - the decorator way Yesterday, I've come up with the idea of using python context manager to ensure cleanup available here. This time I am using that context manager to make a decorator.

The context manager

# SafeGPIO.py
# updated, warning silenced

from RPi import GPIO
from exceptions import RuntimeWarning
import warnings

class SafeGPIO(object):

    def __enter__(self):
        return GPIO

    def __exit__(self, *args, **kwargs):
        with warnings.catch_warnings():
            warnings.simplefilter("error") #turn warning into exceptions
            try:
                GPIO.cleanup()
            except RuntimeWarning:
                pass # silence it

The decorator

#decorators.py

from . import SafeGPIO
from RPi import GPIO
from functools import wraps

def safe_gpio(func):
    """
    This decorator ensure GPIO.cleanup() is called when function call ends,
    also it injects GPIO as first argument into your function
    """
    @wraps(func) # using wraps preservses doc string
    def wrapper(*args, **kwargs):
        with SafeGPIO() as GPIO:
            return func(GPIO, *args, **kwargs)
    return wrapper

def gpio(func):
    """
    This decorator injects GPIO as first argument into your function
    """
    @wraps(func)
    def wrapper(*args, **kwargs):
        return func(GPIO, *args, **kwargs)
    return wrapper

Use like this:

# decorator_test.py

from SafeGPIO.decorators import safe_gpio, gpio
from time import sleep
from random import choice, randint

GPIO_PINS = (3,5,7,8,10,11,12,13,15,16,18,19,21,22,23,24,26)
VALUES = (True, False)

@safe_gpio
def do_random_things_with_pins_for_ten_times(GPIO):
    GPIO.setmode(GPIO.BOARD)
    for pin in GPIO_PINS:
        GPIO.setup(pin, GPIO.OUT)
    for _ in xrange(10):
        pin = choice(GPIO_PINS)      # choose one of the GPIO pin
        value = choice(VALUES)       # output either true or false
        sleep_seconds = randint(1,3) # sleep from 1 to 3 seconds
        print "slected pin %d, output %r, sleep for %d seconds" %
                (pin, value, sleep_seconds)
        GPIO.output(pin, value)
        sleep(sleep_seconds)

@safe_gpio
def do_real_work(GPIO):
    GPIO.setmode(GPIO.BOARD)
    GPIO.setup(7, GPIO.OUT)
    print "doing real work for 5 seconds"
    GPIO.output(7, True)
    sleep(5)

@safe_gpio # guarantee to clean up on exit
def main(GPIO):
    do_random_things_with_pins_for_ten_times()
    do_real_work()

if __name__ == '__main__':
    main()

If you want to do gpio in one line, you can use functools.partial and either functools.wraps or functools.update_wrapper.

As you are using wraps already, you may as well use partial too.

def gpio(func):
    return update_wrapper(partial(func, GPIO), func)

Now it's a simple one-liner.
If you don't need @wraps, this can be further simplified to:

def gpio(func):
    return partial(func, GPIO)

Ways I would change the program:

Adding an option to SafeGPIO to have the option to handle errors.
Merging gpio and safe_gpio into one function would improve my user experience.
Less functions to remember, whilst keeping the option for it to be safe.
You don't use @gpio, so would you ever use it?

If your response is no, you should consider removing it.
As you could just import GPIO.
And it seems like a lot of overhead for importing GPIO.


Combinations of list elements It was written in Emacs Lisp and requires Common Lisp loop facility.

Can this code be improved? Did I hit any anti-patterns along the way?

(defun combos (list)
  (let* ((a (car list))
         (d (cdr list))
         (s (and d (combos d)))
         (v (loop for c in s collect (cons a c))))
    (cons (list a) (append s v))))

I would suggest handling a nil input explicitly. Then you can:

strip out the and (in s's binding), and just recurse directly
strip out the cons at the bottom, leaving only the append

Here's the code I tested with (in Scheme, since I have neither Emacs nor a CL implementation installed):

;; Requires SRFI 26
(define (combos lst)
  (if (null? lst) '(())
      (let* ((a (car lst))
             (d (cdr lst))
             (s (combos d))
             (v (map (cut cons a <>) s)))
        (append s v))))

And if you really can't bear to see the empty list as one of the combinations (though it really is), just take the cdr of the resulting list. It's always the first element.

Update: I installed Emacs now, so I was able to port my Scheme version to elisp and test it:

(defun combos (list)
  (if (null list) '(nil)
      (let* ((a (car list))
             (d (cdr list))
             (s (combos d))
             (v (mapcar (lambda (x) (cons a x)) s)))
        (append s v))))


Designing simple state machine in C++ I have designed a simple state machine for the following diagram:

I am seeking improvements from the community. I especially don't like two things:

When calling performAction I wan't to send only the action without the callback.
And maybe remove the Action enum from the Engine class.

For simplicity everything is added in one .cpp file:

#include <iostream>
#include <utility>
#include <map>

typedef enum {
    Idle,
    Start,
    Stop,
    ChangeSpeed
}State;

class Engine
{
public:
    typedef enum {
        SetSpeed,
        Halt
    }Action;

    typedef void (Engine::*DoAction)(void);

    Engine() {
        std::cout << "Engine()" << std::endl;
    }
    void setSpeed() {
        std::cout << "Engine::setSpeed()" << std::endl;
    }
    void halt() {
        std::cout << "Engine::halt()" << std::endl;
    }
};

struct ActionCallback
{
    Engine::Action         action;
    Engine::DoAction       callback;
    ActionCallback(Engine::Action a, Engine::DoAction c):action(a), callback(c){}
};

struct StateAction {
    State state_;
    Engine::Action action_;
    StateAction(State state, Engine::Action action):state_(state), action_(action){}
    friend bool operator<(const StateAction& sta1, const StateAction& sta2) {
        return (sta1.state_ < sta2.state_) ||
        (sta1.state_ == sta2.state_ &&
         sta1.action_ < sta2.action_);
    }
};

class StateMachine {
public:
    typedef std::map<StateAction, State> StateDiagram;

    StateMachine(Engine &engine):engine_(engine) {
        stateDiag_[StateAction(Idle, Engine::SetSpeed)] = Start;
        stateDiag_[StateAction(Start, Engine::SetSpeed)] = ChangeSpeed;
        stateDiag_[StateAction(ChangeSpeed, Engine::SetSpeed)] = ChangeSpeed;
        stateDiag_[StateAction(ChangeSpeed, Engine::Halt)] = Stop;
        stateDiag_[StateAction(Start, Engine::Halt)] = Stop;
        //
        //  current state is initialized to Idle
        //
        currentState_ = Idle;
        std::cout << "State machine initial state: " << currentState_ << std::endl;
    }
    void performAction(ActionCallback actionCallback) {
        //
        //  The state machine "state" is: currentState
        //
        StateAction stateAction(currentState_, actionCallback.action);

        if (stateDiag_.find(stateAction) == stateDiag_.end()) {
            //
            //  Invalid state transition
            //
            std::cout << "You cannot perform action: "
            << actionCallback.action
            << " from state: "
            << currentState_
            << ", invalid transition!!!"
            << std::endl;
            return;
        }
        State oldState = currentState_;
        //
        //  Execute the action and update the state machine
        //
        (engine_.*actionCallback.callback)();
        currentState_ = stateDiag_[stateAction];

        std::cout << "State changed:\n\tOld state: " << oldState << "\n\tNew state: " << currentState_ << std::endl;
        //
        //  If current state is: Stop change state to: Idle
        //
        if (Stop == currentState_) {
            oldState = currentState_;
            currentState_ = Idle;
            std::cout << "State changed:\n\tOld state: " << oldState << "\n\tNew state: " << currentState_ << std::endl;
        }

    }
    State getInitialState() {
        return Idle;
    }

private:
    StateDiagram stateDiag_;
    State currentState_;
    Engine engine_;
};
int main()
{
    Engine engine;
    StateMachine stateMachine(engine);
    stateMachine.performAction(ActionCallback(Engine::SetSpeed, &Engine::setSpeed));
    stateMachine.performAction(ActionCallback(Engine::Halt, &Engine::halt));
    stateMachine.performAction(ActionCallback(Engine::Halt, &Engine::halt));
    return 0;
}

That's typically how C enums are written, but it's done differently in C++.  The typedef keyword is no longer needed, and the name is given after the enum keyword.

enum State {
    Idle,
    Start,
    Stop,
    ChangeSpeed
};


Property Mapping with Reflection efficiency I have an object that I am populating via mapping and lookup with Reflection:

this.SearchResults = (from a in response.postings
                          select new SearchResponseModel
                          {
                              Id = a.id,
                              TimeStampDate = a.timestampDate,
                              Body = a.body,
                              Title = a.heading,
                              Status = a.status,
                              State = a.state,
                              Language = a.language,
                              Currency = a.currency,
                              CategoryGroup = a.category_group,
                              Source = a.source,
                              ExternalId = a.external_id,
                              ExternalUrl = a.external_url,
                              Price = a.price,
                              Location = PopulateLocation(a.location)
                          }
                          ).ToList();

And the method that does the mapping:

 private static List<LocationLookupModel> PopulateLocation(Location location)
{
    List<LocationLookupModel> allLocations = new List<LocationLookupModel>();
    if (HttpContext.Current.Session["LocationModel"] == null)
    {

        HttpContext.Current.Session["LocationModel"] = allLocations = new LocationModel().LocationList;
    }
    else
    {
        allLocations = (List<LocationLookupModel>)HttpContext.Current.Session["LocationModel"];
    }
    List<LocationLookupModel> modelList = new List<LocationLookupModel>();

    foreach (PropertyInfo propertyInfo in location.GetType().GetProperties())
    {
        var value = propertyInfo.GetValue(location);
        if (value != null)
        {
            LocationLookupModel model = (from a in allLocations
                                         where a.Code == propertyInfo.GetValue(location).ToString()
                                         select a).FirstOrDefault();
            if (model != null)
            {
                modelList.Add(model);
            }
        }
    }

    return modelList;
}

The issue that I run into is that an allLocations object has about 70k records (it represents a list of location lookup values for countries, states, zipcodes, etc), and populating about 100 instances of SearchResponseModel takes about 20 seconds. This is far too long for a UI call, and I have not been able to find a way to make it faster. I understand I am basically doing 3 nested loops (calling helper method for each population, looping over reflected properties, and finally the LINQ call over 70k records) so there are some time efficiency issues, but I am a bit lost on what tricks I can use to make this process more efficient.

Here is a screenshot of the location object:

Here is a screenshot of one of the allLocation objects in the list:

Well to start with reflection is incredibly slow.  And second I'm not even sure why you're using reflection to begin with since you have the strongly typed Location.  Don't use reflection.  I don't think you need to compare every single property.  Pick only the ones that could possibly match.

If you insist on using reflection here's a faster impl:

private static readonly locationProperties = typeof(Location).GetProperties();

foreach (PropertyInfo propertyInfo in locationProperties)
{
    var value = propertyInfo.GetValue(location);
    if (value != null)
    {
        LocationLookupModel model = allLocations.FirstOrDefault(a.Code == value.ToString());
        if (model != null)
        {
            modelList.Add(model);
        }
    }
}

Without reflection:

private static List<LocationLookupModel> PopulateLocation(Location location)
{
    List<LocationLookupModel> allLocations = new List<LocationLookupModel>();
    if (HttpContext.Current.Session["LocationModel"] == null)
        HttpContext.Current.Session["LocationModel"] = allLocations = new LocationModel().LocationList;
    else
        allLocations = List<LocationLookupModel>)HttpContext.Current.Session["LocationModel"];
    return allLocations.Select(x => x.Code == location.city || x.Code == location.country || x.Code == location.county || x.Code == location.locality || x.Code == location.metro || x.Code == location.region || x.Code == location.state || x.Code == location.zipcode).ToList();
}


iterator class using generator function It is sometimes necessary for a generator function to expose state. In those situations it is advised to wrap the generator function as method __iter__ in a class. Here is an example:

class C:
    def __iter__(self):
        for i in range(4):
            yield i
            self.state = i

It is convenient to use this in a for loop since the for loop will call iter() on the object, however, the object itself is not an iterator, so you cannot do this:

c = C()
print(next(c))

Which is inconvenient. To get around this inconvenience I have come up with the following class:

class C:
    def __init__(self):
        self.next = self.__iter__().__next__
    def __iter__(self):
        for i in range(4):
            yield i
            self.state = i
    def __next__(self):
        return self.next()

Usage example:

c = C()
while 1:
    try:
        num = next(c)
        print(num)
        if num == 2:
            print("state:", c.state)
    except Exception:
        break

Is this the best way to design the class? I haven't seen this technique used anywhere else so I'm wondering why everyone else isn't using it, since the C objects can be used both as iterators and as iterables.

Looks like a lot of extra code for very little gain, considering you can simply call iter() on an iterable object to return an iterator:

>>> c = C()
>>> i = iter(c)
>>> next(i)
0
>>> next(i)
1
>>> next(i)
2
>>> c.state
1

Also, note your new class C can have more than one iterator, with only 1 "state".  The first iterator is created automatically by the constructor, for use with the next(c) call.  Additional iterators are created each time you start looping over c, since iter(c) gets called and returns a new generator!

>>> c = C()
>>> next(c)
0
>>> print(next(c), c.state)
1 0
>>> print(next(c), c.state)
2 1
>>> for x in c: print(x, c.state)       # Start a new iteration
...
0 1
1 0
2 1
3 2
>>> print(next(c), c.state)             # Continue original iteration
3 2
>>> next(c)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "<stdin>", line 9, in __next__
StopIteration


Haskell markov text generator I'm new to Haskell, and here is my first not-totally-trivial program. It's the first program I tend to write in any language -- a Markov text generator. I'm wondering what I can change to make it more idiomatic, or what language features I could make better use of.

import Data.List
import System.Environment
import qualified Data.Map as Map
import Control.Monad.State
import System.Random

type MarkovMap = Map.Map String String
type MarkovState = (MarkovMap, StdGen, String)

transition :: State MarkovState Char
transition = do
    (m, gen, sofar) <- get
    let options = m Map.! sofar
        (index, newGen) = randomR (0, length options - 1) gen
        next = options !! index
    put (m, newGen, tail sofar ++ [next])
    return next

generateText :: State MarkovState Char -> State MarkovState String
generateText s = do
    x <- s
    xs <- generateText s
    return (x:xs)

getWords :: MarkovMap -> Int -> [String]
getWords m n =
    let keys        = filter ((==) ' ' . last) $ Map.keys m
        (r, gen)    = randomR (0, length keys - 1) $ mkStdGen 137
        startState  = keys !! r
        markovChars = evalState (generateText transition) (m, gen, startState)
    in  take n . words $ markovChars

printWords :: [String] -> IO ()
printWords ws = mapM_ putStrLn $ makeLines ws
    where makeLines [] = []
          makeLines ws = unwords (take 10 ws) : makeLines (drop 10 ws)

main :: IO ()
main = do
    (n:nwords:fileName:[]) <- getArgs
    contents <- readFile fileName
    let chainMap = chain (read n :: Int) . unwords . words $ contents
    printWords $ getWords chainMap (read nwords :: Int)

chain :: Int -> String -> Map.Map String String
chain n xs =
    let from = map (take n) . tails $ xs ++ " " ++ xs
        to   = drop n . map (:[]) $ xs ++ " " ++ take n xs
    in Map.fromListWith (++) $ zip from to

Example usage:

Keep track of the last 3 characters, take 100 words from tobeornottobe.txt

> runhaskell markov.hs 3 100 tobeornottobe.txt

delay, the law's count with who would bear things all;
and that make and sweary life; fortal shocks the hue
of returns tural consience of somethis againsolution. To die, thers
the he law's the have, or nobler retus resolence to
troublesh is noblesh is sicklied of regards of some will,
and arrows of? To beary from who would by a
life; for inst give spurns, and, but to sleep: perchan
flesh is heir thing afterprises us for no mortal shocks
turns of action devoutly to dreathe sleep: perchance thance the
ills we hue of time, to suffled of great pith

First of all, it seems like you never change the MarkovMap in your States. So, why don't you take it out of MarkovState and change the type of, say, transition, to MarkovMap -> State MarkovState Char?

Try to use more combinators and less pattern-matching. For example, the generate function is something like sequence . repeat.


How to speed up the code for LeetCode "Container with most water" task? I'm trying to solve the LeetCode question where you need to find out the area of the container with the most water. I have created the solution that seems to work in my testing but it fails for being too slow when I try to submit it on LeetCode.
My idea is that I create a dictionary of (x, y) tuples from the input list and then for every item I need to find the maximum distance to any of other lines that are equal or taller than it and from here I can calculate what the maximum area is possible for this line.
How else can I approach this to get it to run faster? (I can't submit solution successfully so can't see examples of answers by other users)
def max_area(height) -> int:
    areas = []
    coords = {x: (x, y) for x, y in enumerate(height)}
    for x in coords:
        higher = [k for k in coords if coords[k][1] >= coords[x][1]]
        area = max(abs(coords[j][0] - coords[x][0]) for j in higher) * coords[x][1]
        areas.append(area)
    return max(areas)

My suggestion about your current solution:
def max_area(height) -> int:
    areas = []
    coords = {x: (x, y) for x, y in enumerate(height)}
    for x in coords:
        higher = [k for k in coords if coords[k][1] >= coords[x][1]]
        area = max(abs(coords[j][0] - coords[x][0]) for j in higher) * coords[x][1]
        areas.append(area)
    return max(areas)

Keep track of the biggest area so far, instead of using the list areas.

def max_area(height) -> int:
    coords = {x: (x, y) for x, y in enumerate(height)}
    result = 0
    for x in coords:
        higher = [k for k in coords if coords[k][1] >= coords[x][1]]
        area = max(abs(coords[j][0] - coords[x][0]) for j in higher) * coords[x][1]
        result = max(result, area)
    return result

This reduces memory usage but is not enough to pass the challenge.
The solution can be considered a brute force approach which is typically not enough to solve medium/hard problems on LeetCode.
The constraint is \$n <= 3 * 10^4\$, where \$n\$ is the length of the input list. Generally, with such constraint, we should look at a solution with a time complexity less than \$O(n^2)\$.
Let's consider the example on LeetCode where the input list is:

height = [1,8,6,2,5,4,8,3,7]

For each two bars, the biggest area is given by the lowest bar. In the example, the two bars are 8 and 7, so the area is 7 * (8 - 1) = 49. Note that (8 - 1) is the difference between the indices of the bars. This is an \$O(n)\$ algorithm:
Initialize l to 0
Initialize r to the right most index
Initialize max_area to 0
while l is lower than r
    find the area as: lowest bar * (r - l)
    update max_area
    increment l if points to the lower bar, else decrement r
return max_area

Next time you can check the "hints" and the tab "Discuss" for help or alternative solutions.


Parse Numerals from a String in Golang I recently tried my hand at writing Go code for a project I'm working on. In order to help myself get a better grasp I decided to write a simple yet somewhat practical program for parsing numeral characters from strings based on the ascii values of the runes within, and appending them to a slice.

My primary concerns with this code are:

It may not be a good solution.
It may not be very performant.
It may not be very idiomatic or reflective of best practices.
It may not be very concise, clean, or readable.

Note: The code below contains the documentation comments. Since this is my first time using Stack Exchange, I'm not entirely certain whether or not this is reasonable and it makes the read a little long.

/*
parseNum package
This is a package which contains a function for converting characters
(charToInt), in the form of runes, into numerals. It also contains a
function for parsing numerals from strings (parseNum).
*/
package main

import (
    "errors"
    "fmt"
)

/*
CharToNum function
This is a function which recieves a rune and converts it to a numeral
based on its ascii code by matching it against the numeral ascii codes
in a for loop. It subtracts 48 from the code to get the actual numeral,
as the ascii codes for numerals are 48-57 and simple subtraction gives
you the actual numeral.
*/
func CharToNum(r rune) (int, error) {
    for i := 48; i <= 57; i++ {
        if int(r) == i {
            return (int(r) - 48), nil
        }
    }
    return -1, errors.New("type: rune was not int")
}

/*
ParseNum function
This is a function which serves the purpose of identifying numerals inside
strings and returning them in a slice. It loops over the string, passing each
character to the charToNum function and identifying whether it should append the
output to the array by testing whether or not the error evaluates to nil.
*/
func ParseNum(str string) []int {
    var nums []int
    for _, val := range str {
        num, err := CharToNum(val)
        if err != nil {
            continue
        }
        nums = append(nums, num)
    }
    return nums
}

func main() {
    fmt.Println(ParseNum("000123456789000"))
}

In your question, you wrote:

func CharToNum(r rune) (int, error) {
    for i := 48; i <= 57; i++ {
        if int(r) == i {
            return (int(r) - 48), nil
        }
    }
    return -1, errors.New("type: rune was not int")
}

In his answer, Janos wrote:

func CharToNum(r rune) (int, error) {
    intval := int(r) - '0'
    if 0 <= intval && intval <= 9 {
        return intval, nil
    }
    return -1, errors.New("type: rune was not int")
}

Using idiomatic Go, I wrote,

var ErrRuneNotInt = errors.New("type: rune was not int")

func CharToNum(r rune) (int, error) {
    if '0' <= r && r <= '9' {
        return int(r) - '0', nil
    }
    return 0, ErrRuneNotInt
}

It's simple and direct and it's fast. Benchmarking the numbers zero through nine:

BenchmarkCharToNumTschfld    5000000    323 ns/op
BenchmarkCharToNumPeter     50000000     37.7 ns/op

Benchmarking the invalid numeric character space (' '):

BenchmarkCharToNumErrTschfld    10000000      242 ns/op       16 B/op      1 allocs/op
BenchmarkCharToNumErrPeter      2000000000      1.26 ns/op     0 B/op      0 allocs/op

The error variable ErrRuneNotInt can be used to check for a specific error, for example, if err == ErrRuneNotInt {}.

In your question, you wrote:

func ParseNum(str string) []int {
    var nums []int
    for _, val := range str {
        num, err := CharToNum(val)
        if err != nil {
            continue
        }
        nums = append(nums, num)
    }
    return nums
}

Using idiomatic Go, I wrote,

func ParseNum(s string) []int {
    nLen := 0
    for i := 0; i < len(s); i++ {
        if b := s[i]; '0' <= b && b <= '9' {
            nLen++
        }
    }
    var n = make([]int, 0, nLen)
    for i := 0; i < len(s); i++ {
        if b := s[i]; '0' <= b && b <= '9' {
            n = append(n, int(b)-'0')
        }
    }
    return n
}

It minimizes both CPU time and memory allocations; it's fast. Benchmarking the string "000123456789000":

BenchmarkParseNumTschfld     1000000    2475 ns/op     248 B/op    5 allocs/op
BenchmarkParseNumPeter       2000000     610 ns/op     128 B/op    1 allocs/op

Unicode and UTF-8 characters corresponding to the ASCII set have the same byte values as ASCII.

See Benchmarks, Package testing.


Mysql PDO Wrapper that throws Unique Constraint Exception The main reasoning behind this PDO wrapper, is that I find myself using unique constraints quite frequently in my designs, and I have if ($ex->errorInfo[1] == 1062) littered throughout my code, and I thought there has to be a better way.
<?php

class PDOMysql extends \PDO {

    public function __construct($dsn, $username = null, $passwd = null, $options = null)
    {
        parent::__construct($dsn, $username, $passwd, $options);
        $this->setAttribute(\PDO::ATTR_ERRMODE, \PDO::ERRMODE_EXCEPTION);
        $this->setAttribute(\PDO::ATTR_DEFAULT_FETCH_MODE, \PDO::FETCH_OBJ);
    }

    public function prepareAndExecute($statement, array $params = []): \PDOStatement
    {
        try {
            $stmt = $this->prepare($statement);
            $stmt->execute($params);
            return $stmt;
        } catch (\PDOException $ex) {
            if ($ex->errorInfo[1] == 1062) {
                throw new PDOMysqlUniqueConstraintException($ex);
            } else {
                throw $ex;
            }
        }
    }

}

class PDOMysqlUniqueConstraintException extends \PDOException {

    public function __construct(\PDOException $ex)
    {
        parent::__construct($ex->getMessage(), $ex->getCode(), $ex->getPrevious());
    }

}

// example usage
$user = 'vps';
$pass = 'vps';
$db = new PDOMysql("mysql:host=localhost;dbname=testdb", $user, $pass);

// create table
$sql = "create table if not exists `mytable` (col1 varchar(100) null, constraint idx_u unique (col1) )";
$db->prepareAndExecute($sql);

// insert first row

$params = ['col1' => uniqid('')];
$sql = "insert into mytable (col1) values (:col1)";
$db->prepareAndExecute($sql, $params);

// select row
$sql = "select * from mytable where col1 = :col1";
$stmt = $db->prepareAndExecute($sql, $params);
var_dump($stmt->fetchAll());

// insert duplicate row
try {
    $sql = "insert into mytable (col1) values (:col1)";
    $db->prepareAndExecute($sql, $params);
} catch (\PDOMysqlUniqueConstraintException $ex) {
   die('Unique Constraint Detected');
}

This code is so good that it's the first time I have nothing to say.
Your approach to the problem is absolutely correct and you should keep using it.
There could be only microscopic nuances, such as one mentioned by Sam. Or, the fact that your constructor would override the configuration settings passed in the $options array. Say, if someone would want to use FETCH_ASSOC as a default fetch mode, your class won't let them. I would rather make it this way
$defaults = [
    PDO::ATTR_ERRMODE            => PDO::ERRMODE_EXCEPTION,
    PDO::ATTR_DEFAULT_FETCH_MODE => PDO::FETCH_OBJ,
    PDO::ATTR_EMULATE_PREPARES   => false, // I would throw in this one too
];
$options = array_merge($defaults,$options);
parent::__construct($dsn, $username, $passwd, $options);

So this way you would have your preferred defaults that, however, could be overwritten, making your class flexible, without artificial restrictions.


Generating Julia set So, I just wanted to post something on Rosetta Code, and I found this task of generating and plotting a Julia set: http://www.rosettacode.org/wiki/Julia_set. There was already one solution but it was quite inefficient and not Pythonic. Here is my attempt on this:

"""
This solution is an improved version of an efficient Julia set solver
from:
'Bauckhage C. NumPy/SciPy Recipes for Image Processing:
 Creating Fractal Images. researchgate. net, Feb. 2015.'
"""
import itertools
from functools import partial
from numbers import Complex
from typing import Callable

import matplotlib.pyplot as plt
import numpy as np

def douady_hubbard_polynomial(z: Complex,
                              *,
                              c: Complex):
    """
    Monic and centered quadratic complex polynomial
    https://en.wikipedia.org/wiki/Complex_quadratic_polynomial#Map
    """
    return z ** 2 + c

def julia_set(*,
              mapping: Callable[[Complex], Complex],
              min_coordinate: Complex,
              max_coordinate: Complex,
              width: int,
              height: int,
              iterations_count: int = 256,
              threshold: float = 2.) -> np.ndarray:
    """
    As described in https://en.wikipedia.org/wiki/Julia_set
    :param mapping: function defining Julia set
    :param min_coordinate: bottom-left complex plane coordinate
    :param max_coordinate: upper-right complex plane coordinate
    :param height: pixels in vertical axis
    :param width: pixels in horizontal axis
    :param iterations_count: number of iterations
    :param threshold: if the magnitude of z becomes greater
    than the threshold we assume that it will diverge to infinity
    :return: 2D pixels array of intensities
    """
    imaginary_axis, real_axis = np.ogrid[
                        min_coordinate.imag: max_coordinate.imag: height * 1j,
                        min_coordinate.real: max_coordinate.real: width * 1j]
    complex_plane = real_axis + 1j * imaginary_axis

    result = np.ones(complex_plane.shape)

    for _ in itertools.repeat(None, iterations_count):
        mask = np.abs(complex_plane) <= threshold
        if not mask.any():
            break
        complex_plane[mask] = mapping(complex_plane[mask])
        result[~mask] += 1

    return result

if __name__ == '__main__':
    mapping = partial(douady_hubbard_polynomial,
                      c=-0.7 + 0.27015j)  # type: Callable[[Complex], Complex]

    image = julia_set(mapping=mapping,
                      min_coordinate=-1.5 - 1j,
                      max_coordinate=1.5 + 1j,
                      width=800,
                      height=600)
    plt.axis('off')
    plt.imshow(image,
               cmap='nipy_spectral',
               origin='lower')
    plt.show()

I think it looks good, and it is definitely more efficient. There was just one thing that I was not sure about. I was thinking to take out creating a complex_plane to a separate function and pass it as a parameter to julia_set. But in this case the julia_set wouldn't be a pure function as it would mutate the complex_plane. And I prefer my functions not to have any side effects. So I decided to leave it as is.

Any comments on this matter or anything else are welcome.

Here are some examples of output:

1. Review

Some of the variable names could be improved:

complex_plane is an array of \$z\$ values for each pixel in the image, so naming it z would help the reader relate it to the z in douady_hubbard_polynomial.

imaginary_axis and real_axis are only used once in the very next line, so there is no need for them to have long and memorable names. I would use something short like im and re.

result is an array of iteration counts, so it could be named something like iterations.

mask is a Boolean array selecting pixels that have not yet diverged to infinity, so something like not_diverged or live would convey this better.
On each iteration, the iteration counts of the escaped pixels are incremented. This means that some pixels get incremented many times, for example a pixel that escapes on the first iteration gets its count incremented 256 times. It would be more efficient to set the iteration count for each pixel just once. A convenient time to do this is when it escapes.
As the number of iterations goes up, the number of pixels that have not escaped to infinity gets smaller and smaller. But the masking operations are always on the whole array. It would be more efficient to keep track of the indexes of the pixels that have not escaped, so that subsequent operations are on smaller and smaller arrays.

2. Revised code

im, re = np.ogrid[min_coordinate.imag: max_coordinate.imag: height * 1j,
                  min_coordinate.real: max_coordinate.real: width * 1j]
z = (re + 1j * im).flatten()
live, = np.indices(z.shape) # indexes of pixels that have not escaped
iterations = np.empty_like(z, dtype=int)
for i in range(iterations_count):
    z_live = z[live] = mapping(z[live])
    escaped = abs(z_live) > threshold
    iterations[live[escaped]] = i
    live = live[~escaped]
iterations[live] = iterations_count - 1
return (iterations_count - iterations).reshape((height, width))

Notes

This is about three times as fast as the code in the post.
Because we are maintaining an array of indexes, it is convenient to flatten the z array and then reshape iterations to two dimensions before returning it. If we left the array two-dimensional, there would need to be two arrays of indexes, live_i and live_j.
Pixels that don't escape are given the value iterations_count - 1 in order to match the code in the post. It would make more sense to use iterations_count or a larger value here.
The subtraction iterations_count - iterations is only there so that the returned values match the code in the post. The subtraction could be omitted if you reverse the colour map.