Shortest pathway across a snakes and ladders board (Update) Earlier I posted a fairly inefficient recursive solution to the problem of getting across a snakes and ladders board in the smallest number of moves.

I have created a much faster solution to this using Dijkstra's algorithm and I believe it is correct.

Each square on the board is linked to any square that is between 1-6 larger than it with a weight of one (equivalent of rolling a 1-6 on a dice). All snakes and ladders link squares with a weight of 1. The aim was to have the smallest total cost for the pathway between 1 and 100 (0 and 99 here as I have used list indexes).

This is the first time I have implemented Dijkstra's algorithm and the first time I have used namedtuples. I am not sure if using namedtuples was appropriate, but it made it clearer in my head.

I think I have massively over-complicated bits of code especially within the for loop under the condition if edge.start == next_item:. I seem to be using list comprehensions far too much and I know this makes the solution slower than it could be. Please could someone help me work out better ways to access the variables in my queue of named-tuples.

"""Calculate the shortest path across a snakes and ladders board using Dijkstra's shortest path"""
from collections import namedtuple

Edge = namedtuple("Edge", ("start", "end", "cost"))
Stack = namedtuple("Stack", ("start", "pathcost", "totalcost"))

class Graph:
    """Class generates graph and calculates shortest path"""
    def __init__(self, edges):
        """Generate edges in graph"""
        self.edges = [Edge(*edge) for edge in edges]

    def dijkstra_path(self, start, end):
        """Function that calculates the shortest path"""
        if start >= end or start < 0 or end > 99:
            return -1
        queue = sorted(
            (
                Stack(edge.end, edge.cost, edge.cost)
                for edge in self.edges
                if edge.start == start
            ),
            key=lambda x: x[2],
        )
        while queue:
            next_item, _, current_total = queue.pop(0)
            if next_item == end:
                return current_total
            for edge in self.edges:
                if edge.start == next_item:
                    if edge.end in [item.start for item in queue]:
                        current_cost = [
                            item.totalcost for item in queue if item.start == edge.end
                        ][0]
                        if not current_cost < edge.cost + current_total:
                            queue = [item for item in queue if item.start != edge.end]
                            queue.append(
                                Stack(edge.end, edge.cost, edge.cost + current_total)
                            )
                    else:
                        queue.append(
                            Stack(edge.end, edge.cost, edge.cost + current_total)
                        )
            queue = sorted(queue, key=lambda x: x[2])

def build_graph():
    """Chess Board"""
    list_board = [[i, i + j, 1] for i in range(100) for j in range(1, 7)]
    # Ladders
    list_board.append([1, 37, 1])
    list_board.append([19, 59, 1])
    list_board.append([28, 63, 1])
    list_board.append([55, 99, 1])
    # Snakes
    list_board.append([91, 13, 1])
    list_board.append([86, 11, 1])
    list_board.append([53, 2, 1])
    list_board.append([41, 13, 1])
    return list_board

if __name__ == "__main__":
    GRAPH = Graph(build_graph())
    FROM = 0
    TO = 100
    NUMBER_STEPS = GRAPH.dijkstra_path(FROM, TO)
    if not NUMBER_STEPS == -1:
        print(f"Can complete game in a minimum of {NUMBER_STEPS} rolls")
    else:
        print("Error. Make sure the starting point is between 0 and 99 and less than the end point",
              "which itself must be than or equal to 99")

Named tuples are a good idea here. If you need to mutate the data you should instead use dataclasses.dataclass. But as there is no need here it's good.
I'd suggest changing your nodes to contain snakes, ladders, and the start and end of the board. This is as you don't need to build the empty spaces as they are only used as distances.

Since you need the distance between two nodes, then you need to calculate the distance at creation, not during the calculations.
I've simplified calculating the distance between each node. You should take into account the following special cases:

If the distance between two nodes is 12, and there is a snake or ladder halfway between them, it takes 3 rather than 2 turns to travel between the nodes.
If there are 6 snakes or ladders after a node it's impossible to pass them without visiting those nodes.

Currently you have Edge with a start, end and cost. I would suggest you instead split this into a Node with a start, end and edges. And an Edge that has a distance and a node.

It should be noted that the start and end should only be used when creating the graph. The node will then just turn into glorified list.
It should be noted that Node should be called Vertex if you want to keep with 'pure' graph theory naming.
Please ignore my implementation of Dijkstra's algorithm, it's not great. And half way I came across some hashing problems, hence the path[node[:2]] ickyness. I don't recommend you take inspiration from it - unless it's how not to code.

This means if the graph is setup, incorrectly, to have two ladders over the same space, then some bugs may appear.

All of this together can look like:

from collections import namedtuple
from dataclasses import dataclass, field
from typing import *  # Against best-practice but shhh
import math

Edge = namedtuple('Edge', 'distance node'.split())

class Node(namedtuple('Node', 'start end edges'.split())):
    def __str__(self):
        return f'{self.start} -> {self.end}'

@dataclass(order=True)
class Path:
    distance: int
    current: Node=field(compare=False)
    previous: Node=field(compare=False)

@dataclass
class Graph:
    nodes: List[Node]

    def shortest_paths(self, start: Node) -> Dict[Node, Path]:
        if start not in self.nodes:
            raise ValueError("Graph doesn't contain start node.")
        paths = {}
        queue = []
        for node in self.nodes:
            path = Path(float('inf'), node, None)
            paths[node[:2]] = path
            queue.append(path)

        paths[start[:2]].distance = 0
        queue.sort(reverse=True)
        while queue:
            node = queue.pop()
            for neighbor in node.current.edges:
                alt = node.distance + neighbor.distance
                path = paths[neighbor.node[:2]]
                if alt < path.distance:
                    path.distance = alt
                    path.previous = node
            queue.sort(reverse=True)
        return paths

    def shortest_path(self, start: Node, end: Node) -> List[Tuple[int, Node]]:
        if end not in self.nodes:
            raise ValueError("Graph doesn't contain end node.")
        paths = self.shortest_paths(start)
        node = paths[end[:2]]
        output = []
        while node is not None:
            output.append((node.distance, node.current))
            node = node.previous
        return list(reversed(output))

def build_nodes(snakes: List[Tuple[int, int]], size: int) -> List[Node]:
    return [
        Node(1, 1, []),
        Node(size, size, [])
    ] + [
        Node(start, end, [])
        for start, end in snakes
    ]

# There are some edgecases that will need to be handled.
def calculate_distance(start: Node, end: Node, nodes: List[Node]):
    distance = int(math.ceil((end.start - start.end) / 6))
    start.edges.append(Edge(distance, end))

def add_edges(nodes: List[Node]):
    for start in nodes:
        for end in nodes:
            if end.start > start.end:
                calculate_distance(start, end, nodes)

def build_graph(edges: List[Tuple[int, int]], size: int):
    nodes = build_nodes(edges, size)
    add_edges(nodes)
    start, end = nodes[:2]
    return Graph(nodes), start, end

if __name__ == '__main__':
    graph, start, end = build_graph(
        [
            (  2, 520),
            (530, 500),
            (510, 999)
        ],
        1000,
    )
    for dist, node in graph.shortest_path(start, end):
        print(dist, node)

This outputs:

0 1 -> 1
1 2 -> 520
3 530 -> 500
5 510 -> 999
6 1000 -> 1000


Should I use an object or type as trait? Edit: Disclaimer: I am new to templates and traits.
I apologize in advance if this is a bad question.

I am refactoring two similar classes with small differences strewn all over them into one template class (or I am trying to anyway).
This is something I just wrote. (I hope I didn't make any mistakes renaming everything.)

// declaration - empty
template <typename SETTINGS>
struct SettingsTraits;

// specialization for type CFoo
template <>
struct SettingsTraits<CFoo>
{
  typedef CFooRelated RelatedType;
  typedef CAttribute<CFooRelated > & (CDataSource::*AttrGetter)();
  static CAttribute<CFooRelated > & (CDataSource::*GetAttr)();
};

SettingsTraits<CFoo>::AttrGetter SettingsTraits<CFoo>::GetAttr = &CDataSource::GetFooAttr;

// specialization for type CBar
template <>
struct SettingsTraits<CBar>
{
  typedef CBarRelated RelatedType;
  typedef CAttribute<CBarRelated> & (CDataSource::*AttrGetter)();
  static CAttribute<CBarRelated> & (CDataSource::*GetAttr)();
};

SettingsTraits<CBar>::AttrGetter SettingsTraits<CBar>::GetAttr = &CDataSource::GetBarAttr;

So that I can write something like this

CAttribute<SettingsTraits<SETTINGS>::RelatedType> attr = (datasource.*SettingsTraits<SETTINGS>::GetAttr)();

But now I am wondering if I shouldn't just have created two different structs and passed those as template parameters instead.

I imagine the calling code would end up looking like this:

CAttribute<FooTraits::RelatedType> attr = FooTraits::GetAttr(datasource);

So probably somewhat cleaner but I haven't tried it so I don't know if it will actually work.
(I should probably replace the function pointer with a simple one line function that calls the appropriate function for each type in any case.)

Which alternative should I chose? What are the benefits/drawbacks of each?

Does it even make sense to have functions in traits? Should I maybe just use free template functions instead?

Yes, the std way of doing things in C++ would be to provide static member functions of your traits type. Consider:

// declaration - empty
template <typename SETTINGS>
struct SettingsTraits;

// specialization for type CFoo
template <>
struct SettingsTraits<CFoo>
{
  typedef CFooRelated RelatedType;
  static CAttribute<RelatedType>& GetAttrFrom(CDataSource& ds) { return ds.GetFooAttr(); }
};

// specialization for type CBar
template <>
struct SettingsTraits<CBar>
{
  typedef CBarRelated RelatedType;
  static CAttribute<RelatedType>& GetAttrFrom(CDataSource& ds) { return ds.GetBarAttr(); }
};

typedef SettingsTraits<SETTINGS> TRAITS;
CAttribute<TRAITS::RelatedType> attr = TRAITS::GetAttrFrom(datasource);

Notice that this way of doing things doesn't involve global objects or values at all; it's all done entirely within the type system. The compiler can look at the line TRAITS::GetAttrFrom(datasource) and statically know that it's equivalent to datasource.GetFooAttr(), which means the whole thing compiles down to a single call instruction. Your old code would have required a trip to memory in order to load the value of the pointer SettingsTraits<CFoo>::GetAttr.

Rule of thumb: you should never need to construct a type-traits object at all. If you do, you're probably doing it wrong.


Writing cubic sums a better way I was asked the following problem:

  What is the smallest positive integer n that can be written as a sum of three positive cubes in at least 12 ways?

Can this be coded better way such that it is easy to compute the smallest positive integer that is sum of m nth powers? I'm not a professional programmer, so I managed only to write one solution that works.

l = list()
for i in range(1,500):
    for j in range(i,500):
        for k in range(j,500):
            l.append(i**3+j**3+k**3)
l.sort()
for i in range(0,len(l)-12):
    if l[i] == l[i+12]:
        number = l[i]
for i in range(1,500):
    for j in range(i,500):
        for k in range(j,500):
            if i**3+j**3+k**3 == number:
                print(str(number)+"="+str(i)+"^3+"+str(j)+"^3+"+str(k)+"^3")

Instead of three nested for loops, you can use itertools to enumerate combinations. Also, I would personally prefer to keep a count in a dictionary, as opposed to storing the same numbers over and over.

import itertools

cube_counts = {}

for i, j, k in itertools.combinations(range(1, 500), 3):
    cube = i**3 + j**3 + k**3
    cube_counts[cube] = cube_counts.get(cube, 0) + 1

print(min([cube for cube, count in cube_counts.items() if count >= 12]))


Computing cable bills using F(x)'s This is a school project that I am turning in late. The prof suggested that i somehow prove myself by improving upon the basic code. This is really tough to do for someone who is just starting to learn to code.

Is there something I can do to:

add onto the code (have it do or ask something different)?
make it even more simple than it probably already is (unlikely)?
though it's counter intuitive, make it more complicated by adding a function or something similar?

I have really looked at this and now that I've actually coded it, I don't know how to improve it.

print("This program computes a cable bill")
account = input("Enter account number: ")
cust_type = input("Enter customer type: R (Residental) or B (Business): ")

def residential():
 premium_channels = int(input("Enter number of premium channels used: "))
 processing_fee = 4.50
 basic_service_fee = 20.50
 premium_channel_fee = 7.50
 amount = processing_fee + basic_service_fee + (premium_channels * premium_channel_fee)
 return (amount)

def business():
 basic_connections = int(input("Enter number of basic service connections: "))
 premium_channels = int(input("Enter number of premium channels used: "))
 processing_fee = 15.0
 basic_service_fee = 75.0
 preimum_channel_fee = 50.0
 if (basic_connections <= 10):
     amount = processing_fee + basic_service_fee + (premium_channels * preimum_channel_fee)
 else:
     amount = processing_fee + basic_service_fee + ((basic_connections - 10) * 5) + (premium_channels *
preimum_channel_fee)
 return (amount)

if (cust_type == "R"):
    bill_amount = residential()
else:
    bill_amount = business()

print("Account number: ", account)
print("Amount due: ", bill_amount)

Placing all the input and output in a separate function would improve the structure of program.
Computing the bill follows the exact same logic for both types of connection; they are just using a different price list. You could use a data structure to represent the price list, and pass it to the billing function as a parameter.


Concat a collection's sub arrays and merge their parent I have an array like this:

var arr = [
    {
        revision: 19,
        text: 'hello',
        details: [
            {
                id: 5
            }
        ]
    },
    {
        revision: 19,
        text: 'hello',
        details: [
            {
                id: 6
            }
        ]
    },
    {
        revision: 17,
        text: 'world',
        details: [
            {
                id: 7
            }
        ]
    }
];

And I'd like to concat its details arrays if the revision is the same (the text values are the same if they got the same revision value):

var expected = [
    {
        revision: 19,
        text: 'hello',
        details: [
            {
                id: 5
            },
            {
                id: 6
            }
        ]
    },
    {
        revision: 17,
        text: 'world',
        details: [
            {
                id: 7
            }
        ]
    }
];

Here is what I came up with:

constructArray(groupByRevisionNb(arr));

function groupByRevisionNb(arr) {
    return _.groupBy(arr, 'revision');
}

function constructArray(obj) {
    return _.map(obj, function (arr) {
        return concatRevisions(arr);
    });
}

function concatRevisions(arr) {
    return _.reduce(arr, function (obj1, obj2) {
        if (_.isEmpty(obj1)) {
            obj1 = _.clone(obj2);
        } else {
            obj1.details = obj1.details.concat(obj2.details);
        }
        return obj1;
    }, {});
}

But I'm not 100% satisfied with it. What could be improved?

I'm not using ES6.

This can be done natively:

var revisionMap = arr.reduce((map, entry) => {
  var revision = entry.revision;

  if(map.hasOwnProperty(revision))
    map[revision].details.push(...entry.details);
  else
    map[revision] = _.clone(entry);

  return map;
}, {});

/*
{
  1: { revision: REV, text: TEXT, details: [] },
  2: { revision: REV, text: TEXT, details: [] },
}
*/

var expected = Object.keys(revisionMap).map( key => revisionMap[key] );

/*
[
  { revision: REV, text: TEXT, details: [] },
  { revision: REV, text: TEXT, details: [] },
]
*/

First, make a mapping of ID to revision data. This way, we can easily check the existence of an already visited revision and know when to append rather than assign. Then, we convert that object into an array.

The only thing not native is the use of _.clone as we don't want to be modifying the original data when we're be appending details.


Idiomatic input parsing in clojure I've been playing around with Clojure for a while now, and one of the tasks I occasionally find awkward is parsing input.  For example, I took part in the facebook hacker cup recently, where part of the challenge was to read input like this:

5
4
..##
..##
....
....
4
..##
..##
#...
....
4
####
#..#
#..#
####
5
#####
#####
#####
#####
.....
5
#####
#####
#####
#####
#####

Where the first line is the number of "cases" to follow and the first line of each case is the number of lines in that case.  It seems fairly obvious to me that the output of parsing this should be a list of "cases", but there's no obvious way to know when to split the data without reading part of the data first.  (In this particular case I know I could look for lines with numbers vs. lines with #s, but I'm more interested in a general solution)

I ended up implementing this like:

(defn read-stdin
  []
  (line-seq (java.io.BufferedReader. *in*)))

(defn my-iterate
  [f n input]
  "Calls iterate on input and returns the final result"
  (nth (iterate f input) n))

(defn parse-grid
  "Parses a text grid of .s and #s and returns a set of coord
   pairs for each # element"
  [lines]
  (set (filter identity
      (for [[row line] (map-indexed vector lines)
            [col c] (map-indexed vector line)]
        (if (= c \#) [row col] nil)))))

(defn parse-case
  "Takes a case-array and some lines, parses out a single case,
   adds it to the case-array and returns along with the remainder of the lines.
   Meant to be used with iterate"
  [[cases [first-line & lines]]]
  (let [problem-size (read-string first-line)]
    [(conj cases {:size problem-size
                   :blacks (parse-grid (take problem-size lines))}),
     (drop problem-size lines)]))

(defn parse-cases
  [[first-line & rest-lines]]
  (let [num-cases (read-string first-line)]
    (first (my-iterate parse-case num-cases [[] rest-lines]))))

(def read-cases-stdin (comp parse-cases read-stdin))

The parse-case function is my main worry here - it feels really awkward to have to return a vector of the current case list and the rest of the input, and have to deal with unpacking it again on the next iteration of the function.  Is there a more idiomatic way of going about doing this?

Any other general feedback on my code would also be welcome.

I think your main problem is that you're using iterate where partitioning and mapping would work just fine.

I would create a simple helper function that splits the input line-seq into cases. Normally you could use partition for this, but since the number of lines can vary from case to case, you would have to use loop/recur. (See my split-cases function below. Also, note that we don't really care about the number of cases on the first line; we completely ignore it via destructuring.)

Now, believe it or not, all you have to do is map your parse-grid function over the result of (split-cases ...) and you'll end up with a list of sets of coordinates, one for each case.

I think your parse-grid function is very nice. It's concise and it makes good use of map-indexed and destructuring. The only thing I would tweak is that I would leverage the built-in :when syntax of Clojure's for macro to filter out the coordinates of the non-# characters. Then you can take out the filter identity part because there are no longer any nil values to remove.

So, here is how I would revise your code:

(defn split-cases [[_ & lines]]
  "Returns a seq of cases, each of which is a seq of #/. lines."
  (loop [result []
         [number-of-lines & lines] lines]
    (if (seq lines)
      (recur (conj result (take number-of-lines lines))
             (drop number-of-lines lines))
      result)))

(defn parse-grid [grid]
  "Parses a text grid of .s and #s and returns a set of coord
   pairs for each # element"
  (set (for [[row line] (map-indexed vector grid)
             [col c]    (map-indexed vector line)]
             :when (= c \#)]
         [row col])))

(defn read-stdin []
  (line-seq (java.io.BufferedReader. *in*)))

(defn parse-cases-stdin []
  (map parse-grid (split-cases (read-stdin))))


My C code for Fibonacci Sequence I've been learning more programming in the past few years (C, Java, Arduino) for my hobbies. I read the "C for Dummies" book and found that it was good, but did not cover much - especially pointers... One of my friends from college (CS major) suggested a simple program exercise to see if I could figure out how to use pointers.  Well, I had to refer to a pdf of Ritchie's text "The C Programming Language" to learn about pointers. He asked me to make a program that returns the Fibonacci sequence. A month or so ago, I sent him my program, but he hasn't had time to review it since he is very busy with work these days. Basically what I am wondering about my program is if it is redundant or "too verbose" - basically is it written efficiently?  IMO, as a hobbyist programmer, it works just fine.

Below is the source code file he initially sent to me:

/**

* @file  fibonacci.cpp
*
* @brief Prints out a partial Fibonacci sequence.

*/

#include <stdio.h>

// Function prototype for fibonacci()
unsigned int Fibonacci(unsigned int);

int main(int argc, char** argv)
{
    printf("Fibonacci Sequence:\n");

    for (unsigned int n = 0; n < 10; ++n)
    {
        unsigned int fib = Fibonacci(n);
        printf("%d ", fib);
    }

    return 0;
}

// Computes the Nth Fibonacci number.
unsigned int Fibonacci(unsigned int N)
{
    // Actual implementation goes here!!
}

Below is my version of the program that I sent to him for review:

/**

* @file  fibonacci.cpp
*
* @brief Prints out a partial Fibonacci sequence.

*/

#include <stdio.h>
unsigned int n;
char nMax[3];  //user input for sequence up to and including value @ this index
int Nmax;

// Function prototype for fibonacci()
unsigned int Fibonacci(unsigned int);

int main(int argc, char** argv)
{
    printf("Please input the Nth index of the last sequence value.\n");
    printf("Program will return entire Fibonacci Sequence up to and including Nth value.\n");

    gets(nMax);
    Nmax =atoi(nMax);

    for (n = 0; n < Nmax; ++n)
    {
        unsigned int fib = Fibonacci(n);
        printf("%d ", fib);
    }

    return 0;
}

// Computes the Nth Fibonacci number.
unsigned int Fibonacci(unsigned int n)
{
    // Actual implementation goes here!!

    unsigned int N;  //function output value

    //declare a temp array of sequence values
    unsigned int tempseq[100];

    // declare two ptrs
    unsigned int *prev;  //pointer to previous or (n-1) value in sequence
    unsigned int *prevprev;  //pointer to previous previous or (n-2) value in sequence

    if (n==0)
    {   N = 1;  //1st seed value
        tempseq[n] = N;
    }
    else if(n==1)
    {
        N = 1;  //2nd seed value
        tempseq[n] = N;
    }
    else
    {
        prev = &tempseq[n-1];        //prev ptr points to n-1 array element
        prevprev = &tempseq[n-2];    //prevprev ptr points to n-2 array element

        N = *prev + *prevprev;       //fibonacci recurrence relation
        tempseq[n] = N;              //add current value of N to the temp array

    }

    return N;

}

EDIT:  Not to step on the toes of JS1 who recently replied, but I wanted to add an image here of my cmd line execution of my fibonacci.exe program to show that at least on my pc, it seems to work....

Doesn't work
The program doesn't work for me.  Example:
$ ./fibonacci
Please input the Nth index of the last sequence value.
Program will return entire Fibonacci Sequence up to and including Nth value.
10
1 1 2 3 1 0 1 1 2 3

The problem is that the array tempseq is a temporary local variable, which means that every time you call the function Fibonacci(), that variable becomes unitialized.  Your code is doing two things wrong:

It assumes that tempseq will contain the values that it had from previous invocations of the function Fibonacci().
It assumes that Fibonacci() will be called in a loop from 0 to n.  If you just called Fibonacci(10) directly, it would not work because it depends on the answers for 8 and 9 being precomputed.

Fixes

To fix problem #1, you can declare your tempseq array using the static keyword.  This makes the array persist across invocations.  You can think of it like if the static keyword made the tempseq variable into a global variable.

To fix problem #2, you should not assume that an entry in tempseq is necessarily valid.  You can do something like check if the entry is 0.  If it is zero, then the entry must be computed, otherwise it is valid to use.

Rewrite
I have rewritten your function with the two problems fixed:
#define FIBMAX        100

// Computes the Nth Fibonacci number.
unsigned int Fibonacci(unsigned int n)
{
    // FibResult is a persistent array of already computed values.
    // We initialize fibResult[0] and [1] to take care of special cases.
    static unsigned int fibResult[FIBMAX] = { 1, 1 };

    // Guard against bad input.
    if (n >= FIBMAX)
        return 0;

    if (fibResult[n] == 0) {
        // We have not previously computed this entry, so compute it now
        // using the recurrence relation.
        //
        // Note: calling Fibonacci(n-1) guarantees that fibResult[n-2] will
        // be set, so we just use that to avoid one function call.  We
        // could have written this and it would have worked just fine:
        //
        // fibResult[n] = Fibonacci(n-1) + Fibonacci(n-2);
        //
        fibResult[n] = Fibonacci(n-1) + fibResult[n-2];
    }

    return fibResult[n];
}

This rewrite also:

Replaces the magic number 100 with a defined constant and checks the input so that we don't have any buffer overflow problems.
Takes care of the special cases of n=0 and n=1 by filling in those entries in the fibResult array.

Simpler way
You might also want to consider a simpler function that doesn't use an array of precomputed values.  Calculating a fibonacci number can be done in a simple loop like this:
unsigned int Fibonacci(unsigned int n)
{
    unsigned int f1 = 1;
    unsigned int f2 = 1;

    for (unsigned int i = 0; i < n; i++) {
        f2 = f1 + f2;
        f1 = f2 - f1;
    }
    return f1;
}


Download and save bulk URL concurrently I am new to Go and wrote program to download and save bulk URLs concurrently. It is working correctly, but I would like to make it more efficient and follow best practices.

package main

import (...)

// Connection String
const connectionString = "connectionString"

// Declare Channels
var wg sync.WaitGroup
var ch = make(chan *item) // For success item(s)
var errItems = make(chan *item) // For error item(s)

type item struct {
    id      int
    url     string
    path    string
    content []byte
    err     error
}

func main() {
    start := time.Now()
    var basePath = os.Args[1]
    var from, to = os.Args[2], os.Args[3]
    db, err := sql.Open("sqlserver", connectionString)
    if err != nil {
        log.Fatal(err.Error())
    }
    rows, err := db.Query("SELECT [ImageID],[AccommodationlID],[Link],[FileName] FROM [dbo].[AccomodationImage] where AccommodationlID between @from and @to and FileName not like '%NoHotel.png%'", sql.Named("from", from), sql.Named("to", to))
    if err != nil {
        log.Fatal(err.Error())
    }
    defer db.Close()
    for rows.Next() {
        var item = item{}
        var accId, name string
        _ = rows.Scan(&item.id, &accId, &item.url, &name)
        item.path = fmt.Sprintf("%s\\%s\\%s", basePath, accId, name)
        wg.Add(1)
        go downloadFile(&item)
        go func() {
            select {
            case done := <-ch:
                wg.Add(1)
                go saveAndUpdateFile(db, done)
            case errorItem := <-errItems:
                wg.Add(1)
                go printResult(errorItem)
            }
        }()
    }
    wg.Wait()
    fmt.Printf("%.2fs elapsed\n", time.Since(start).Seconds())
}
func downloadFile(item *item) {
    defer wg.Done()
    resp, err := http.Get(item.url)
    if err != nil {
        item.content, item.err = nil, err
    } else if resp.StatusCode != http.StatusOK {
        item.content, item.err = nil, errors.New(resp.Status)
    } else {
        item.content, item.err = ioutil.ReadAll(resp.Body)
    }
    if item.err != nil {
        errItems <- item
    } else {
        ch <- item
    }
}
func saveAndUpdateFile(db *sql.DB, item *item) {
    defer wg.Done()
    if item.content == nil {
        item.err = errors.New("Content is empty.")
    } else {
        dir := filepath.Dir(item.path)
        err := os.MkdirAll(dir, os.ModePerm)
        if err != nil {
            item.err = err
        } else {
            item.err = ioutil.WriteFile(item.path, item.content, 0644)
        }
    }
    if item.err == nil {
        result, err := db.Exec("UPDATE [dbo].[AccomodationImage] SET IsRead = 1 WHERE ImageID = @id", sql.Named("id", item.id))
        if rows, _ := result.RowsAffected(); rows <= 0 || err != nil {
            item.err = errors.New("Update status failed.")
        }
    }
    if item.err != nil {
        errItems <- item
    }
}
func printResult(item *item) {
    defer wg.Done()
    fmt.Println(item.toString())
}

I use a select statement to implement event so that when errItems or ch channels receive, save or print items. I also surrounded select statement with goroutine to prevent blocking.

Full code

import (...)

Please include your full code next time. This should instead be:

import (
    "database/sql"
    "errors"
    "fmt"
    "io/ioutil"
    "log"
    "net/http"
    "os"
    "path/filepath"
    "sync"
    "time"
)

You also have item.toString(), but this is nowhere defined. I changed it to:

fmt.Println(item)

Scope

connectionString doesn't need to be in the global scope, whether it's const or not. The same could be argued about wg, ch, and errItems -- but moving those to a lower scope would involve adding more function arguments, which is a choice you can make.

In terms of security, I recommend against hard-coding the connection string in the source code. Instead, as one of many options, you can have the connection credentials as a separate file that you read.

Simplify query

You use named arguments in your query. But here, the query is so short that it's clear from the context. You can split the query across multiple lines to make it easier to read. You also have some keywords in all uppercase and some in all lowercase.

q := `
SELECT [ImageID],
       [AccommodationlID],
       [Link],
       [FileName]
FROM [dbo]. [AccomodationImage]
WHERE AccommodationlID BETWEEN ? AND ?
      AND FileName NOT LIKE '%NoHotel.png%'`

rows, err := db.Query(q, from, to)

(Notice that you also have a misspelling in AccomodationImage.)

This kind of formatting for query strings is also how the Go documentation does it.

Vertical spacing

All of the code is compressed together and has no room to breathe. I recommend adding the occasional empty lines, such as between if-statements, for loops, goroutines, etc.

Validate os.Args

Your code assumes the program will always have three arguments. While this makes sense if only you plan to use it, it's generally not good practice.

Without bounds checking, you will get a runtime panic "index out of range" -- no a particularly useful message for those running the program.

if len(os.Args) < 4 {
    log.Fatal("missing arguments")
}

You can also use short variable declaration when declaring basePath, from, and to.

basePath := os.Args[1]
from, to := os.Args[2], os.Args[3]

Default values

var item = item{}

This is redundant. You should be able to simply use var item -- please correct my if I'm wrong here and Rows.Scan() produces an error if item is nil. Since we do not need to initialize item, we can combine our declarations. Notice I rename the variable item to avoid confusion with the type item.

var (
    i     item
    accID string
    name  string
)

Conclusion

Here's the code I ended up with:

package main

import (
    "database/sql"
    "errors"
    "fmt"
    "io/ioutil"
    "log"
    "net/http"
    "os"
    "path/filepath"
    "sync"
    "time"
)

type item struct {
    id      int
    url     string
    path    string
    content []byte
    err     error
}

var wg sync.WaitGroup
var ch = make(chan *item)       // For success items
var errItems = make(chan *item) // For error items

func main() {
    const connectionString = "connectionString"

    if len(os.Args) < 4 {
        log.Fatal("missing arguments")
    }

    start := time.Now()

    basePath := os.Args[1]
    from, to := os.Args[2], os.Args[3]

    db, err := sql.Open("sqlserver", connectionString)

    if err != nil {
        log.Fatal(err.Error())
    }

    q := `
SELECT [ImageID],
       [AccommodationlID],
       [Link],
       [FileName]
FROM [dbo]. [AccomodationImage]
WHERE AccommodationlID BETWEEN ? AND ?
      AND FileName NOT LIKE '%NoHotel.png%'`

    rows, err := db.Query(q, from, to)

    if err != nil {
        log.Fatal(err.Error())
    }

    defer db.Close()

    for rows.Next() {
        var (
            i     item
            accID string
            name  string
        )

        _ = rows.Scan(&i.id, &accID, &i.url, &name)

        i.path = fmt.Sprintf("%s\\%s\\%s", basePath, accID, name)

        wg.Add(1)

        go downloadFile(&i)

        go func() {
            select {
            case done := <-ch:
                wg.Add(1)
                go saveAndUpdateFile(db, done)
            case errorItem := <-errItems:
                wg.Add(1)
                go printResult(errorItem)
            }
        }()
    }

    wg.Wait()

    fmt.Printf("%.2fs elapsed\n", time.Since(start).Seconds())
}

func downloadFile(i *item) {
    defer wg.Done()

    resp, err := http.Get(i.url)

    if err != nil {
        i.content, i.err = nil, err
    } else if resp.StatusCode != http.StatusOK {
        i.content, i.err = nil, errors.New(resp.Status)
    } else {
        i.content, i.err = ioutil.ReadAll(resp.Body)
    }

    if i.err != nil {
        errItems <- i
    } else {
        ch <- i
    }
}

func saveAndUpdateFile(db *sql.DB, i *item) {
    defer wg.Done()

    if i.content == nil {
        i.err = errors.New("Content is empty.")
    } else {
        dir := filepath.Dir(i.path)
        err := os.MkdirAll(dir, os.ModePerm)

        if err != nil {
            i.err = err
        } else {
            i.err = ioutil.WriteFile(i.path, i.content, 0644)
        }
    }

    q := `
UPDATE [dbo].[AccomodationImage]
SET    IsRead = 1
WHERE  ImageID = ?`

    if i.err == nil {
        result, err := db.Exec(q, i.id)

        if rows, _ := result.RowsAffected(); rows <= 0 || err != nil {
            i.err = errors.New("Update status failed.")
        }
    }

    if i.err != nil {
        errItems <- i
    }
}

func printResult(i *item) {
    defer wg.Done()

    fmt.Println(i)
}

Unfortunately I cannot test this code and suggest algorithmic or more in-depth changes.

Hope this helps!


Finding the most common character in a string I have started to try and work out the TopCoder problems. The "StringDup" problem asks us to:

  Create a class called StringDup. Given a string made up of ONLY letters and
  digits, determine which character is repeated the most in the string ('A' is
  different than 'a'). If there is a tie, the character which appears first in
  the string (from left to right) should be returned.

Is there any way I can make my solution better or more efficient?  Is there something that I have missed? I'm new to Java and I thought this would be a good way to learn.

public class StringDup {

    public char getMax(String word)
    {
        int characterCount = 0;
        int maxCharacter = 0;
        char maxCharacterChar = '.';

        char[] cArray = word.toCharArray();

        for(int i =0; i < cArray.length; i++)
        {
            int characterASCII = (int)cArray[i];
            characterCount = 0;

            //System.out.print("Chracter ASCII: " + characterASCII + "\n");
            for(int x = 0; x < cArray.length; x++)
            {
                if(characterASCII == (int)cArray[x])
                {
                    characterCount ++;
                    //System.out.print("Character Count for " + characterASCII + " " +  characterCount  + "\n");
                    if(characterCount > maxCharacter)
                    {
                        maxCharacter = characterCount;
                        maxCharacterChar = cArray[i];
                    }
                }
            }
        }
        return maxCharacterChar;
    }
}

It is called using the main method.

StringDup frequentChar = new StringDup();
System.out.print(frequentChar.getMax("sssdkljgh"));

Your algorithm has two loops, and that's really not necessary.

This question is looking for a special 'trick' to be done.... and that trick is to process the data in reverse order....

Consider this loop:

public static char getMax(String word) {
    if (word == null || word.isEmpty()) {
        throw new IllegalArgumentException("input word must have non-empty value.");
    }
    char maxchar = ' ';
    int maxcnt = 0;
    // if you are confident that your input will be only ascii, then this array can be size 128.
    int[] charcnt = new int[Character.MAX_VALUE + 1];
    for (int i = word.length() - 1; i >= 0; i--) {
        char ch = word.charAt(i);
        // increment this character's cnt and compare it to our max.
        if (++charcnt[ch] >= maxcnt) {
            maxcnt = charcnt[ch];
            maxchar = ch;
        }
    }
    return maxchar;
}

note the following things:

validate the input.
create an array of counters for each character.
work backwards... if this is the max count (or equal to it) then we have a new winner.
there is no validation on the input characters .... you should probably limit the chars to the a-zA-Z0-9 set that is specified.... the solution above will work for any String.

Some additional notes....

in Java, the standard code style puts the opening { brace on the same line as the block declaration (if condition, method declaration, etc.) Putting the brace on a new line is a C thing to do, and is frowned upon.
x is not a good variable name for anything unless it is a co-ordinate in a multidimensional array (normally linked with (x, y, ...) ). In your use case, you should use the variable j because that is the one that by convention is used inside the i loop.


Serialize and deserialize n dimensional int array I've made two symmetric methods which serialize an int array of any number of dimensions into an OutputStream, and read such an array from an InputStream. I was wondering if there is ways this code could be improved. (I have tested this code and it works correctly.) Thanks!

import java.io.ByteArrayInputStream;
import java.io.ByteArrayOutputStream;
import java.io.IOException;
import java.io.InputStream;
import java.io.OutputStream;
import java.lang.reflect.Array;

public class IntArraySerializer {

    private IntArraySerializer() {}

    /**
     * Writes arr, which is assumed to be an n dimensional int array, to out.
     */
    public static void writeIntArr(Object arr, OutputStream out) throws IOException {
        writeInt(dimensions(arr.getClass()), out);
        writeIntArrRaw(arr, out);
    }

    /**
     * Reads an n dimensional int array from in. Symmetrical to writeIntArr.
     */
    public static Object readIntArr(InputStream in) throws IOException {
        int dimensions = readInt(in);
        return readIntArrRaw(in, dimensions - 1);
    }

    private static void writeIntArrRaw(Object obj, OutputStream out) throws IOException {
        if (obj.getClass().isArray()) {
            writeInt(Array.getLength(obj), out);
            for (int i = 0; i < Array.getLength(obj); i++) {
                writeIntArrRaw(Array.get(obj, i), out);
            }
        } else {
            writeInt((Integer) obj, out);
        }
    }

    private static Object readIntArrRaw(InputStream in, int dimensionsRemaining) throws IOException {
        int length = readInt(in);
        Object[] out = new Object[length];
        if (dimensionsRemaining == 0) {
            for (int i = 0; i < length; i++) {
                out[i] = readInt(in);
            }
        } else {
            for (int i = 0; i < length; i++) {
                out[i] = readIntArrRaw(in, dimensionsRemaining - 1);
            }
        }
        return out;
    }

    public static void writeInt(int n, OutputStream out) throws IOException {
        out.write(new byte[] {
                (byte) ((n & 0xFF000000) >> 24),
                (byte) ((n & 0x00FF0000) >> 16),
                (byte) ((n & 0x0000FF00) >> 8),
                (byte) (n & 0x000000FF)
                });
    }

    public static int readInt(InputStream in) throws IOException {
        byte[] buffer = new byte[4];
        in.read(buffer);
        return (buffer[0] << 24) | (buffer[1] << 16) | (buffer[2] << 8) | buffer[3];
    }

    /**
     * @param clazz a class of array
     * @return how many dimensions clazz represents, or 0 if clazz doesn't represent an array.
     */
    public static int dimensions(Class<?> clazz) {
        if (clazz.isArray())
            return dimensions(clazz.getComponentType()) + 1;
        else
            return 0;
    }

}

1. writing an int to an OutputStream instance

Instead of splitting your int to bytes and putting them in a byte array, directly consume your bytes like this:

out.write((n >>> 24) & 0xFF);
out.write((n >>> 16) & 0xFF);
out.write((n >>>  8) & 0xFF);
out.write((n >>>  0) & 0xFF);

This is taken from the source code of FileOutputStream, which offers methods to write primitives to a stream.

This offers higher performance compared to packing your int in a byte array. Internally, the OutputStream.write(byte[] b) method is iterating over your byte array and calling out.write(int b) anyways. The same goes for the reading part as well I guess.

2. public helper methods

I would make readInt and writeInt private/protected. There is no need to expose them. If you plan to call them from other classes, a class called "IntArraySerializer" doesn't seem to be the correct place for them. Instead you should put them in a new class with a more expressive name like "StreamUtilities".

3. argument checking

You might want to check each argument of your public methods for null and throw an IllegalArgumentException if it is null, specifying which argument is null. This makes future debugging easier: You or other programmers won't have to look at the source code in order to debug a NullPointerException...

Throw and IllegalArgumentException in writeIntArr if the object is not an array.


Merging two lists of dicts based on the value of a specific key I'm combining two lists of dicts, based on the value of the key 'time'.

If have two lists, for example:

a=[{'time': '25 APR', 'total': 10, 'high': 10},
   {'time': '26 APR', 'total': 5, 'high': 5}]

b=[{'time': '24 APR', 'total': 10, 'high': 10},
   {'time': '26 APR', 'total': 15, 'high': 5}]

These lists contain values per day, for a specific source, say a and b.

I would like to add the dicts in the lists, based on the value of the key 'time'. So the end result would be:

c=[{'time': '24 APR', 'total': 10, 'high': 10},
   {'time': '25 APR', 'total': 10, 'high': 10},
   {'time': '26 APR', 'total': 20, 'high': 10}]

Notice that the results for 26 APR are added.

The way I do this now, is as follows:

from collections import Counter
import itertools

lst = sorted(itertools.chain(totals_30_days, past_30_days), key=lambda x: x['time'])

f = []

for k,v in itertools.groupby(lst, key=lambda x:x['time']):
    v = list(v)

    # Check if there are more than one item, because adding an empty Counter()
    # will delete any keys with zero or negative values.
    if len(v) > 1:
        e = Counter()
        for i in v:
            c = Counter(i)
            time = c.pop('time', None)
            e = e + c
        e['time'] = time
        f.append(dict(e))
    else:
        f.append(v[0])
print(f)

The result is correct:

[{'high': 10, 'total': 10, 'time': '24 APR'}, {'high': 10, 'total': 10, 'time': '25 APR'}, {'high': 10, 'total': 20, 'time': '26 APR26 APR'}]

But I wonder if it could be more efficient. Any ideas?

itertools.groupby() is a good start.  To process each group, you want to take advantage of functools.reduce().  reduce() does the right thing, whether there is just one record in a group or multiple records.

Your code suffers from readability problems.  One obvious issue is that a, b, c, d, e, and f are horribly meaningless variable names.  Another problem is that the only way to see what it does is to trace through the code.  (Well, it would help if you wrote the entire introductory text to this question as a giant comment, but ideally the code should be eloquent enough to speak for itself.)

Let's start with the goal of being able to write this:

a=[{'time': '25 APR', 'total': 10, 'high': 10},
   {'time': '26 APR', 'total': 5, 'high': 5}]

b=[{'time': '24 APR', 'total': 10, 'high': 10},
   {'time': '26 APR', 'total': 15, 'high': 5}]

merger = merge_list_of_records_by('time', add)
print(merger(a + b))

Then, it's a matter of writing a merge_list_of_records_by() function to make that happen.

from functools import reduce
from itertools import groupby
from operator import add, itemgetter

def merge_records_by(key, combine):
    """Returns a function that merges two records rec_a and rec_b.
       The records are assumed to have the same value for rec_a[key]
       and rec_b[key].  For all other keys, the values are combined
       using the specified binary operator.
    """
    return lambda rec_a, rec_b: {
        k: rec_a[k] if k == key else combine(rec_a[k], rec_b[k])
        for k in rec_a
    }

def merge_list_of_records_by(key, combine):
    """Returns a function that merges a list of records, grouped by
       the specified key, with values combined using the specified
       binary operator."""
    keyprop = itemgetter(key)
    return lambda lst: [
        reduce(merge_records_by(key, combine), records)
        for _, records in groupby(sorted(lst, key=keyprop), keyprop)
    ]


Haskell Arithmetic Quiz I'm currently learning Haskell, and as one of my first "proper" programs have created a mathematics quiz. It works like this:

A question is generated, with two numbers from 1 to 10 which need to be either added, multiplied or subtracted, for example 3 * 6.
The question is presented to the user, and they can write their answer and press Enter.
If the user's answer is the same as the actual answer to the question, they get one point towards their score. If the answer is incorrect their points stay the same.
This repeats 10 times, and the user's points are shown to them at the end.

I've implemented the operator part of question generation using an Operator data type with a Random instance. The numbers are generated with a randomSensibleInts x gen function, which generates x random numbers between 1 and 10.

The askQuestion function uses IO to get the user's answer, returning True if the answer was correct and False otherwise. Finally, in main, the askQuestion function is called 10 times, returning a [Bool]. fromEnum is then used to count True as one point and False as zero, which combined with sum creates a point total.

My code is as follows:

import System.Random

randomSensibleInts :: Int -> StdGen -> [Int]
randomSensibleInts quantity = take quantity . randomRs (1, 10)

data Operator = Add | Subtract | Multiply deriving (Eq, Enum)

instance Show Operator where
  show op = case op of
             Add -> "+"
             Subtract -> "-"
             Multiply -> "*"

instance Random Operator where
  random gen = case randomR (0, 2) gen of
                 (item, gen') -> (toEnum item, gen')

  randomR (lo, hi) gen = case randomR (fromEnum lo, fromEnum hi) gen of
                           (item, gen') -> (toEnum item, gen')

genQuestion :: StdGen -> (Int, Operator, Int, StdGen)
genQuestion gen = (numbers !! 0, operator, numbers !! 1, gen')
                  where numbers = randomSensibleInts 2 gen
                        (operator, gen') = random gen :: (Operator, StdGen)

askQuestion :: (Int, Operator, Int, StdGen) -> IO Bool
askQuestion (i1, o, i2, gen) = do
  putStrLn $ unwords ["Enter the answer to", show i1, show o, show i2] ++ ": "
  userAnswer <- getLine
  let trueAnswer = case o of
                     Add -> i1 + i2
                     Subtract -> i1 - i2
                     Multiply -> i1 * i2
  if (show trueAnswer) == userAnswer then do
    putStrLn "Well done!"
    return True
  else do
    putStrLn $ "Wrong - the answer was " ++ show trueAnswer
    return False

main = do
  gens <- mapM (const newStdGen) [1..10]
  let questions = map genQuestion gens
  answers <- mapM askQuestion questions
  putStrLn $ "Your score was " ++ show (sum $ map fromEnum answers)

Please let me know if my code can be made more "Haskell-like", for example if there are builtins to replace any of my custom functions. I'm also interested in pointfree style, so please let me know if any of my functions can be improved to use this. I'm also fairly certain this code can be shortened drastically.

The Show typeclass is for debugging

The Show typeclass is really meant to be derived, not implemented yourself. The purpose of Show is mostly for debugging, and the closest thing to a law for Show is that it should be the inverse of Read. The result of show should usually be a string that represents a Haskell expression that would produce the value itself.

While it is tempting to create a custom instance of Show for your own purposes, I would just convert your instance into a separate function, showOperator (which can also be implemented more simply with direct pattern matching rather that explicitly using case):

showOperator :: Operator -> String
showOperator Add = "+"
showOperator Subtract = "-"
showOperator Multiply = "-"

randomSensibleInts is poorly-named

What makes integers in the range [1, 10] any more “sensible” than any other integers? Given how incredibly simple it is, and given it’s only used in one place, I would honestly just inline it into the place it’s used. I think it’s clearer that way than any name you could give it.

Avoid !!

The !! function is unsafe, since it will crash if the provided index is out of bounds. Admittedly, in your case, you know the length of the list will always be exactly 2, so it’s not dangerous, but in that case, you should just use pattern-matching instead:

genQuestion :: StdGen -> (Int, Operator, Int, StdGen)
genQuestion gen = (x, operator, y, gen')
  where [x, y] = take 2 $ randomRs (1, 10) gen
        (operator, gen') = random gen

(You also don’t need the type annotation on random gen, so I’ve removed it.)

Package tuples into named datatypes

The long tuple return type for genQuestion is a little bit suspicious. The name even provides a good suggestion for naming the datatype: Question. Create a datatype for that instead of having such a large tuple:

data Question = Question Int Operator Int

genQuestion :: StdGen -> (Question, StdGen)
genQuestion gen = (Question x operator y, gen')
  where ...

askQuestion :: (Question, StdGen) -> IO Bool
askQuestion (Question i1 o i2, gen) = ...

Avoid using booleans directly

Using Bool as a return type of a non-predicate function can be confusing (this is referred to as “boolean blindness”). Using a separate, named type can be both more readable and more scalable if the number of cases ever changes:

data AnswerResult = Correct | Incorrect
  deriving (Eq)

askQuestion :: (Question, StdGen) -> IO AnswerResult
askQuestion = ...

This also allows adjusting the score summing logic to be a bit more reliable. Using toEnum and assigning significance to the indices is both unnecessary and mixing concerns.

putStrLn $ "Your score was " ++ show (length $ filter (== Correct) answers)

Avoid mixing pure and impure logic

The askQuestion function has too many responsibilities: formats a prompt for input, displays it to the user, calculates the answer to the question, validates the answer to the question, and prints out a response based on the answer. Three of those responsibilities are totally pure, and they should ideally be pulled into their own functions:

questionPrompt :: Question -> String
questionPrompt (Question x op y) = "Enter the answer to " ++ show x ++ showOperator op ++ show y ++ ": "

questionAnswer :: Question -> Int
questionAnswer (Question x op y) = operatorFunction op x y
  where operatorFunction Add = (+)
        operatorFunction Subtract = (-)
        operatorFunction Multiply = (*)

validateAnswer :: Question -> Int -> AnswerResult
validateAnswer question answer =
  if questionAnswer question == answer
  then Correct
  else Incorrect

askQuestion :: (Question, StdGen) -> IO AnswerResult
askQuestion (question, gen) = do
  putStrLn $ questionPrompt question
  userAnswer <- getLine
  let answerResult = validateAnswer question (read userAnswer)
  case answerResult of
    Correct -> putStrLn "Well done!"
    Incorrect -> putStrLn $ "Wrong - the answer was " ++ show (questionAnswer question)
  return answerResult

Eliminate unused variables

If you turn on -Wall, GHC will tell you that gen is never used in askQuestion at all, so it can be removed:

genQuestion :: StdGen -> Question
genQuestion gen = Question x operator y
  where [x, y] = take 2 $ randomRs (1, 10) gen
        (operator, _) = random gen

askQuestion :: Question -> IO AnswerResult
askQuestion question = do
  putStrLn $ questionPrompt question
  userAnswer <- getLine
  let answerResult = validateAnswer question (read userAnswer)
  case answerResult of
    Correct -> putStrLn "Well done!"
    Incorrect -> putStrLn $ "Wrong - the answer was " ++ show (questionAnswer question)
  return answerResult

main :: IO ()
main = do
  gens <- mapM (const newStdGen) [1..10]
  let questions = map genQuestion gens
  answers <- mapM askQuestion questions
  putStrLn $ "Your score was " ++ show (length $ filter (== Correct) answers)

Use where or let instead of case when there is only one pattern

In the Random instance for Operator, it’s possible to avoid case if you’re simply destructuring rather than matching against multiple patterns:

instance Random Operator where
  random gen = (toEnum item, gen')
    where (item, gen') = randomR (0, 2) gen

  randomR (lo, hi) gen = (toEnum item, gen')
    where (item, gen') = randomR (fromEnum lo, fromEnum hi) gen

Final result

import System.Random

data Operator = Add | Subtract | Multiply
  deriving (Eq, Enum)

data Question = Question Int Operator Int

data AnswerResult = Correct | Incorrect
  deriving (Eq)

instance Random Operator where
  random gen = (toEnum item, gen')
    where (item, gen') = randomR (0, 2) gen

  randomR (lo, hi) gen = (toEnum item, gen')
    where (item, gen') = randomR (fromEnum lo, fromEnum hi) gen

showOperator :: Operator -> String
showOperator Add = "+"
showOperator Subtract = "-"
showOperator Multiply = "-"

questionPrompt :: Question -> String
questionPrompt (Question x op y) = "Enter the answer to " ++ show x ++ showOperator op ++ show y ++ ": "

questionAnswer :: Question -> Int
questionAnswer (Question x op y) = operatorFunction op x y
  where operatorFunction Add = (+)
        operatorFunction Subtract = (-)
        operatorFunction Multiply = (*)

validateAnswer :: Question -> Int -> AnswerResult
validateAnswer question answer =
  if questionAnswer question == answer
  then Correct
  else Incorrect

genQuestion :: StdGen -> Question
genQuestion gen = Question x operator y
  where [x, y] = take 2 $ randomRs (1, 10) gen
        (operator, _) = random gen

askQuestion :: Question -> IO AnswerResult
askQuestion question = do
  putStrLn $ questionPrompt question
  userAnswer <- getLine
  let answerResult = validateAnswer question (read userAnswer)
  case answerResult of
    Correct -> putStrLn "Well done!"
    Incorrect -> putStrLn $ "Wrong - the answer was " ++ show (questionAnswer question)
  return answerResult

main :: IO ()
main = do
  gens <- mapM (const newStdGen) [1..10]
  let questions = map genQuestion gens
  answers <- mapM askQuestion questions
  putStrLn $ "Your score was " ++ show (length $ filter (== Correct) answers)


Paragraph Matching in Python So I developed some code as part of a larger project.  I came upon a problem with how to match paragraphs, and wasn't sure how to proceed, so I asked on Stack Overflow here.  You can find an in-depth description of my problem there if you're curious.

Just to be clear, I am not reposting the same question here to get an answer.

I came up with a solution to my own problem, but I'm unsure of limits/pitfalls, and here seems like the perfect place for that.

The short version on the explanation is this:  I have two strings, one is the revised version of the other.  I want to generate markups and preserve the paragraph spacing, thus, I need to correlate the list of paragraphs in each, match them, and then mark the remaining as either new or deleted.

So I have a function (paraMatcher()) which matches paragraphs and returns a list of tuples as follows:

(num1, num2) means that the best match for revised paragraph num1 is original paragraph num2
(num, '+') means that there is no match for revised paragraph num, so it must be new (designated by the '+')
(num, '-') means that no revised paragraph was matched to original paragraph num so it must have been deleted (designated by the '-')

So without further adiue, here is my function:

def paraMatcher(orParas, revParas):
    THRESHOLD = 0.75
    matchSet = []
    shifter = 0

    for revPara in revParas:
        print "Checking revPara ", revParas.index(revPara)
        matchTuples = [(difflib.SequenceMatcher(a=orPara,b=revPara).ratio(), orParas.index(orPara)) for orPara in orParas]
        print "MatchTuples: ", matchTuples
        if matchTuples:
            bestMatch = sorted(matchTuples, key = lambda tup: tup[0])[-1]
            print "Best Match: ", bestMatch
            if bestMatch[0] > THRESHOLD:
                orParas.pop(bestMatch[1])
                print orParas
                matchSet.append((revParas.index(revPara), bestMatch[1] + shifter))
                shifter += 1
            else:
                matchSet.append((revParas.index(revPara), "+"))

        print ""
        print "---------------"
        print ""

    if orParas:
        print "++++++++++++++dealing with extra paragraphs++++++++++++++"
        print orParas
        for orPara in orParas:
            matchSet.insert(orParas.index(orPara) + shifter, (orParas.index(orPara) + shifter, "-"))

    return matchSet

While I definitely want review of general coding style, etc, here are a few issues that I'm really interested in getting feedback on:

The function needs to be called with copies of the lists (paraMatcher(lst1[:], lst2[:]))
How might this fail?
How do I determine an appropriate value for THRESHOLD

Some Extra Notes:

I've left the diagnostic printing in there in case any of you want to test it
Due to other parts of the code, its more convenient that this function take lists of paragraphs as arguments, rather than the strings
I don't think it matters, but this is 32-bit Python 2.7.6 running on 64-bit Windows 7

A few tips for leaner code:

Use enumerate while iterating when you need the index, to avoid the sequential search and extra verbosity of orParas.index(orPara). For example the last loop becomes

for orIndex, orPara in enumerate(orParas):
    matchSet.insert(orIndex + shifter, (orIndex + shifter, "-"))

max(matchTuples) achieves the same as sorted(matchTuples, key = lambda tup: tup[0])[-1] and avoids sorting. You could even give a key argument to max, but tuples are sorted item by item anyway, and here the second item is an ascending integer, so including it in the sort key does not change the order.
Unpacking bestRatio, bestIndex = max(matchTuples) improves readability as you can use bestRatio instead of bestMatch[0] in the code that follows.


Processing a string containing product variants I have a variable containing a string in this format "Size-XS|Size-XL|Color-Red|Color-Green". When i process it the final result is "Variations: Size: XS, XL, Color: Green, Red. The code for this is like below.

$variations = explode("|", $search_db_row['variations']);
 foreach ($variations as $variations_arr1) {
  $variations_arr1_exp = explode("-", $variations_arr1);
  $variations_arr[$variations_arr1_exp[0]][] = $variations_arr1_exp[1];
 }
foreach ($variations_arr as $variations_arr2k => $variations_arr2v) {
 $variation.= $variations_arr2k.": ";
 foreach ($variations_arr2v as $variations_arr2_value) {
  $variation.= mb_strtolower($variations_arr2_value, "utf-8").", ";
 }
}

Is there a simpler way to achieve this? I think there are too much foreach's but I'm not sure how can I do it with less code.

This code is not very easy to follow.

(I am going to assume that the variables $search_db_row['variations'] and $variation where previously initialized)

Firstly, Your spacing is not very clear:
This way:

$variations = explode("|", $search_db_row['variations']);

foreach ($variations as $variations_arr1) {
    $variations_arr1_exp = explode("-", $variations_arr1);
    $variations_arr[$variations_arr1_exp[0]][] = $variations_arr1_exp[1];
}

foreach ($variations_arr as $variations_arr2k => $variations_arr2v) {
   $variation.= $variations_arr2k.": ";
   foreach ($variations_arr2v as $variations_arr2_value) {
      $variation.= mb_strtolower($variations_arr2_value, "utf-8").", ";
   }
}

makes it easier to see what is connected to what.

Secondly, consider changing the variable names.
Something like this:

$initial_variations = explode("|", $search_db_row['variations']);

foreach ($initial_variations as $initial_variations) {
    $each_variation = explode("-", $initial_variations);
    $variations_arr[$each_variation[0]][] = $each_variation[1];
}

foreach ($variations_arr as $variation_types => $variations_values) {
   $variation.= $variations_types.": ";
   foreach ($variations_values as $variations_value) {
      $variation.= mb_strtolower($variations_value, "utf-8").", ";
   }
}

Makes it easier to follow because the variable names themselves tell me what is happening.

As far as reducing the code/getting rid of the foreach's, you could try like this:

foreach ($initial_variations as $initial_variations) {
    $each_variation = explode("-", $initial_variations);
    if ($previous_type != $each_variation[0]){
        $variation .= $each_variation[0].": ";
    }
    $variation .= mb_strtolower($each_variation[1], "utf-8").", ";
    $previous_type = $each_variation[0];
}

two more points (homework):

you have a trailing comma at the end of each group (this prints Size: xs, xl, Color: red, green, instead of Size: xs, xl Color: red, green)
this could use a comment (or two)


Shell script to copy video files to another folder I recently started running but hate running both in the quiet and with music, so i've been listening to podcasts.  Unfortunately, I've caught up with all my favorites and so I've started listening to some iTunesU courses, but unfortunately (again) very few of them have audio-only versions.

I wrote a shell script to copy any recently added iTunesU video files to another folder as m4a files (via ffmpeg) but since it's the first such script that i've ever written I am slightly worried to set it to run every morning ...

Anyway, I would appreciate some feedback from you expert types of what potential problems there might be with how I have it working now or suggestions of how I might improve part of it.  Also, as you'll see, I tried to set up a function to do the extension check but couldn't get it to work that way, is there a way to do that?

#!/bin/bash

oldIFS="$IFS" # allow for files/directories with spaces in the names
IFS=$'\n'

is_video_file ()
{
    ext=$1

    if ([ $ext = "m4v" ] || [ $ext = "mp4" ]);
        then
            return 1
        else
            return 0
    fi
} # couldn't get this to work right...

iTunesU_Source=~/Music/iTunes_U/
iTunesU_Destination=~/Music/iTunesU_audio/

cd $iTunesU_Source

for sub in */; do
    echo;
    echo $sub

    dest_sub=${iTunesU_Destination}/${sub}

    # create the sub folder if it doesn't exist
    does_not_exist=false
    [ -d $dest_sub ] || does_not_exist=true

    if $does_not_exist;
        then
            mkdir $dest_sub
    fi

    # get the modified times for the two subfolders
    source_sub_time=$(stat -f "%m" -t "%s" ${sub})
    dest_sub_time=$(stat -f "%m" -t "%s" ${dest_sub})

    if $does_not_exist || (( $source_sub_time > $dest_sub_time ));
        then
            cd $sub

            for file in *; do
                filename=$(basename $file)
                extension="${filename##*.}"
                filename="${filename%.*}"

                dest_file=$dest_sub$filename".m4a"

#               if ! [[ -f $dest_file ]] && (is_video_file $extension = 1);
                if ! [[ -f $dest_file ]] && ([ $extension = "m4v" ] || [ $extension = "mp4" ]);
                    then

                    ffmpeg -i $file -vn -acodec copy $dest_file
            fi
            done; # each file in source sub_folder

            cd ..
    fi # source sub_folder is new or recently modified

done

IFS="$oldIFS"

Function

The command

is_video_file $extension = 1

just passes the arguments $extension , = and 1 to the function is_video_file.

To check if the return value of is_video_file is 1, do the following:

is_video_file "$extension"

if [ $? = 1 ]; then ...

However, making a function return 1 usually indicates an error; the return value 0 indicates success.

If you swap 1 and 0 in the definition of is_video_file, you can use

if is_video_file "$extension"; then ...

Further problems

Every time you do not know the value of a variable with certainty, you should surround it with double quotes.

Most of your variables are unquoted, while the strings are quoted. The latter is unnecessary.

For example, you wrote

[ $ext = "m4v" ]

which might fail if $ext contains unanticipated characters.

The commands

[ "$ext" = "m4v" ]

and

[ "$ext" = m4v ]

will both work fine.

Suggestions

There is not need to restore the IFS and the end of the script. Modifying the IFS will only affect the script itself.
Setting IFS=$'\n' gives a false sense of security. It's still necessary to quote all variables if their values are unknown.


Finding powers of multiple array pairs I'm doing a challenge on TalentBuddy that requires you to find the powers of multiple array pairs. Since I got the solution right, but not fast enough, I decided I'd ask for help here.

def fast_power(a, b)
  pairs = a.zip(b)
  pairs.each {|p| puts (p[0] ** p[1]) % 4211}
end

a1 = [long array with 5000 elements]
b1 = [Long array with 5000 elements]

Each element is a number between 0 and 50000.

I have to calculate the value of every element in the first array to the power of every corresponding element in the second array and then modulo that by 4211.

I ran benchmark tests on the data set and here is what I got

14.907000    0.734000    15.641000  (16.818161)

It was supposed to finish in under 2 seconds, so perhaps my algorithm is the issue.

Problem

  Searching for a string in a large data store by simply comparing
  characters would take ages or a huge amount of computing power. We
  need a way to do it fast!

  The first step to building a fast search algorithm is to encode the
  data in a format that makes search efficient.

  A critical operation in the encoding process which we’ll get familiar
  with a bit later is the exponentiation of integer numbers.

  Given a list of pairs of integers (a,b)

  Your task is to

  Write a function that prints to the standard output (stdout) for each
  pair the result of (ab) modulo 4211 (one per line)

  Note that your function will receive the following arguments:

  a - which is an array of integers giving the first element (a) of each pair

  b - which is an array of integers giving the second element (b) of each pair

  The ith pair is defined by a[i] and b[i].

  Data constraints

  The length of a, b arrays will not exceed 10000 elements of a, b
  arrays are integer numbers in the [0, 50000]

  Efficiency constraints

  Your function is expected to print the requested result and return in
  less than 2 seconds make sure you don't use any methods that do
  significant work for you (e.g. pow() in Python)

General Design

The pattern foreach...puts... means that each iteration performs IO. Because IO is slow and blocks writing the results of each multiplication to an output array and then batching the IO by writing the output array all at once is likely to improve performance.

Algorithm

From a pure algorithmic standpoint, using modular powers may significantly reduce computation. This website provides this example:

Let us compute 1143 (mod 13). As we saw before we start with squaring this number:

112 (mod 13) = 121 (mod 13) = 4 (mod 13)

114 (mod 13) = (112)2 (mod 13) = 42 (mod 13) = 16 (mod 13) = 3 (mod 13)

118 (mod 13) = (114)2 (mod 13) = 32 (mod 13) = 9 (mod 13)

1116 (mod 13) = (118)2 (mod 13) = 92 (mod 13) = 81 (mod 13) = 3 (mod 13)

1132 (mod 13) = (1116)2 (mod 13) = 32 (mod 13) =  9 (mod 13)

Putting this together (remember that 43 = 32 + 8 + 2 +1), we get

 1143 (mod 13) = 11 * 4 * 9 * 9 (mod 13)

Now we do our usual modular multiplication:
11 * 4 * 9 * 9 (mod 13) = 44 * 81 (mod 13) = 5 * 3 (mod 13) = 2 (mod 13).

When tackling a numeric problem and throughput matters, a good place to start is with assuming that the fundamental problem has already been solved and attempting to reduce the specific problem to an existing numeric algorithm. It is hard to embody performant mathematics into code without first knowing what is performant.


Longest Substring in Java This is my function for determining the Longest Substring between two strings in Java.

public class Main {
    public static String longestSubstring(String str1, String str2){
        int row = str1.length();
        int col = str2.length();

        int[][] checker = new int[row][col];

        int max = 0;
        int r = 0;
        int c = 0;
        for(int i=0; i < row; i++){
            for(int j=0; j < col; j++){
                if(str1.charAt(i) == str2.charAt(j)){
                    if((i -1 > 0) && (j -1 >0)){
                        checker[i][j] = checker[i-1][j-1] +1;

                    }else{
                        checker[i][j]++;
                    }
                    max = checker[i][j];
                    r = i;
                    c = j;

                }else continue;
            }
        }
        StringBuilder sb = new StringBuilder();
        while(true){
            if(checker[r][c] == 0) break;
            sb.append(str1.charAt(r));
            r--;
            c--;
        }
        return sb.reverse().toString();

    }

    public static void main(String[] args) {
    // write your code here
        String str1 = "adedfrgyu";
        String str2 = "arvbfrgykset";

       String str3 = "aaaaaaatyuieo";
       String str4 = "aaaaaklui";

        System.out.println(longestSubstring(str1, str2));
    }
}

The code compiles, but wondering is the algorithm is correct. It passes all the test cases that I could make up.

I liked your DP approach and use of stringbuider instead of string.

But, the code gives null pointer exception :

 Exception in thread "main" java.lang.ArrayIndexOutOfBoundsException: -1
at Test.longestSubstring(Test.java:157)
at Test.main(Test.java:22)

for the input System.out.println(longestSubstring("aabb", "akk"));

The reason is that whenever the resultant substring starts from index 0, this code will throw a null pointer exception because  checker[0][0] is 1(instead of 0), if the first characters match. Because, you're using if(checker[r][c] == 0) break; as the criteria to end the while loop. One solution is to have the while loop run for max number of times. Other approach is to use an array of 1 greater size i.e. int[][] checker = new int[row+1][col+1];

Moreover, there is a small bug in this code :-

For input System.out.println(longestSubstring("aabb", "abbbb"));, the output is "bb". But, the right answer is "abb". Even for the given test cases,

  String str3 = "aaaaaaatyuieo";
   String str4 = "aaaaaklui";

    System.out.println(longestSubstring(str1, str2));

The answer should be "aaaaa". But, it returns "ui". The reason for that is that you should check if checker[i][j] is greater than max before assigning

max = checker[i][j];
r = i;
c = j;

You need not write else continue; as the last statement in the loop.

Here is the complete code:-

    public static String longestSubstring(String str1, String str2){
    int row = str1.length();
    int col = str2.length();

    int[][] checker = new int[row+1][col+1];

    int max = 0;
    int r = 0;
    int c = 0;
    for(int i = 1; i <= row; i++){
        for(int j = 1; j <= col; j++){
            if(str1.charAt(i-1) == str2.charAt(j-1)){
                checker[i][j] = checker[i-1][j-1] +1;
                if(checker[i][j] > max){
                    max = checker[i][j];
                    r = i;
                    c = j;
                }
            }
        }
    }
    StringBuilder sb = new StringBuilder();
    while(true){
        if(checker[r][c] == 0) break;
        sb.append(str1.charAt(r-1));
        r--;
        c--;
    }
    return sb.reverse().toString();
}


Morse code translator in Java I'm writing an application called "Learn Morse" in Java using Maven. A Morse code translator is a part of this app. I created encode and decode methods to translate in both sides Morse code and I moved the HashMap to the properties file. I also created a helper method to get keys by value.

pl/hubot/dev/learn_morse/model/Encoder.java:

package pl.hubot.dev.learn_morse.model;

import java.io.FileNotFoundException;
import java.io.IOException;
import java.io.InputStream;
import java.util.HashMap;
import java.util.Map;
import java.util.Objects;
import java.util.Properties;

public class Encoder {
    public final String encode(final String input)
            throws IOException,
            NoSuchFieldException {
        StringBuilder encoded = new StringBuilder();
        String lowerCaseInput = input.toLowerCase();
        for (int i = 0; i < lowerCaseInput.length(); i++) {
            char current = lowerCaseInput.charAt(i);
            encoded.append(getMorseCode().getOrDefault(
                    Character.toString(current), " "));
            encoded.append("   ");
        }
        return encoded.toString();
    }

    public final String decode(final String input)
            throws IOException,
            NoSuchFieldException {
        StringBuilder decoded = new StringBuilder();
        String lowerCaseInput = input.toLowerCase();
        for (String current : lowerCaseInput.split(" ")) {
            String key = getKeyByValue(getMorseCode(), current);
            if (key != null) {
                decoded.append(key);
            }
            decoded.append(" ");
        }
        return decoded.toString();
    }

    final Map<String, String> getMorseCode()
            throws IOException,
            NoSuchFieldException {
        Class<?> aClass = Encoder.class;
        ClassLoader classLoader = aClass.getClassLoader();
        String filename = "morse_code.properties";
        try (InputStream input = classLoader
                .getResourceAsStream(filename)) {
            if (input == null) {
                throw new FileNotFoundException(
                        "Sorry, unable to find "
                                + filename);
            }

            Properties properties = new Properties();
            properties.load(input);

            Map<String, String> translations = new HashMap<>();
            for (String key : properties.stringPropertyNames()) {
                String value = properties.getProperty(key);
                translations.put(key, value);
            }

            return translations;
        }
    }

    private <T, E> T getKeyByValue(final Map<T, E> map, final E value) {
        for (Map.Entry<T, E> entry : map.entrySet()) {
            if (Objects.equals(value, entry.getValue())) {
                return entry.getKey();
            }
        }
        return null;
    }
}

resources/morse_code.properties:

a = ._
b = _...
c = _._.
d = _..
e = .
f = .._.
g = __.
h = ....
i = ..
j = .____
k = _._
l = ._..
m = __
n = _.
o = ___
p = .__.
q = __._
r = ._.
s = ...
t = _
u = .._
v = ..._
w = .__
x = _.._
y = _.__
z = __..
\u0105 = ._._
\u0107 = _._..
\u0119 = .._..
\u00E9 = .._..
ch = ____
\u0142 = ._.._
\u0144 = __.__
\u00F3 = ___.
\u015B = ..._...
\u017A = __.._
\u017C = __.._.
0 = _____
1 = .____
2 = ..___
3 = ...__
4 = ...._
5 = .....
6 = _....
7 = __...
8 = ___..
9 = ____.
. = ._._._
, = __..__
' = .____.
" = ._.._.
_ = ..__._
\: = ___...
; = _._._.
? = ..__..
\! = _._.__
- = _...._
+ = ._._.
/ = _.._.
( = _.__.
) = _.__._
\= = _..._
@ = .__._.

My main complaint about your code is that you rebuild the table used for translation for each and every character you are encoding or decoding. This is a major waste of computational power. In addition I've got some other minor comments:

Why the throws SomeException?  – Do you really need these, as you don't explicitly throw them from your code? I'm not entirely sure on common or best practices here, so this might be a moot point, but couldn't these be removed in your code?
Add more space – The density of your code, makes it harder to read. Add a little vertical space here and there, and it will look a lot better. Consider opening your methods with a newline, so that the signature doesn't interfere with the start of the method.
How to access the ch from the properties – In the properties file, you have an entry for ch. How do get to this one? Or is this at typo?

Also adding space before for loop or if blocks, helps structuring the code within a method. Finally, adding an extra newline so that it is two newlines between methods helps separate the methods.
Most variables are named well – Mostly good naming, but input is kind of vague. I would prefer text in encode() and morseCode in decode(), and avoid the lowerCaseInput. Having a variable name describing what has happened to it, seems a little strange.

Avoiding the getMorseCode() rebuild

You're in a class, and a class can have a constructor. Utilize this to build the needed HashMap. And instead of having a rather costly getKeyByValue() I would advice in this case duplicate the reverse map, as well. (Another option would be to use some external library to implement some bidirectional map, like the BiMap from the Guava library). This would lead to code resembling this untested code:

public class Coder {
  private HashMap <String, String> encodeMap;
  private HashMap <String, String> decodeMap;

  Coder() {
      this("morse_code.properties");
  }

  Coder(String filename) {
    InputStream input = null;
    Properties properties = new Properties();
    try {
      input = Morse.class.getClassLoader().getResourceAsStream(filename);
      if (input == null) {
        System.out.println("Sorry, unable to find " + filename);
        return; // Or throw bad exception... :-)
      }

      encodeMap = new HashMap<>();
      decodeMap = new HashMap<>();

      properties.load(input);

      for (String text : properties.stringPropertyNames()) {
        String morseCode = properties.getProperty(text);
        encodeMap.put(text, morseCode);
        decodeMap.put(morseCode, text);
      }

    // Catch and cleanup, if needed...
    } catch (IOException ex) {
        ex.printStackTrace();

    } finally{
      if (input!=null) {
        try {
            input.close();
              } catch (IOException e) {
                  e.printStackTrace();
            }
      }
    }
  }

  public final String encode(final String textInput) {

    StringBuilder encoded = new StringBuilder();
    int textLength = textInput.toLowerCase().length();

    for (int i = 0; i < textLength; i++) {
      String lookup = Character.toString(textInput.charAt(i));
      encoded.append(encodeMap.getOrDefault(lookup, " "));
      encoded.append("   ");
    }

    return encoded.toString();
  }

  public final String decode(final String morseInput) {

    StringBuilder decoded = new StringBuilder();

    for (String morseCode : morseInput.toLowerCase().split(" ")) {
      decoded.append(decodeMap.getOrDefault(morseCode, " "));
      decoded.append(" ");
    }
    return decoded.toString();
  }
}

Here I've also renamed the class to Coder, as it seemed kind of strange to use the Encode class to decode stuff... I've also removed some temporary variables to avoid clutter.

I also added a second constructor allowing for providing a different filename to the constructor. This way you could change the entire functionality of the encode/decode by providing a different properties file.

Code is untested as I don't have access to do the resources thingy using my online java compiler, but you'll get the gist of the idea.

PS! Using the try-with-resources as Vogel612 exemplifies in his answer, is most likely a better, more up to date way to do the properties reading.


Invert bits of a number in clojurescript Doing a challenge to print the number that you would get if you convert a given number to binary, invert its bits, then convert back to decimal.

I decided to give it a run in clojurescript (which I don't really know) and while all the below makes sense to me, it has to be possible to do it better and without dropping to js functions as often as I felt I had to.

(def invert-number [n] (+ 1 (* -1 n)))
(defn number-complement [num]
 (let [numstr (.toString num 2)
       bits (map (comp invert-number js/parseInt) numstr)
       complement-bits (reduce str bits)]
      js/parseInt complement-bits 2)))

First, fixing a few of the errors that I noted in the comments and altering the formatting a bit, I have:

(defn invert-number [n]
  (+ 1 (* -1 n)))

(defn number-complement [num]
 (let [numstr (.toString num 2)
       bits (map (comp invert-number js/parseInt) numstr)
       complement-bits (reduce str bits)]

   (js/parseInt complement-bits 2)))

(println (number-complement 10)
         (number-complement 12)
         (number-complement 15)
         (number-complement 993061001)
         (number-complement 123))

; 5 3 0 80680822 4

I prefer to have defn function definitions on a separate line, and have at least one empty line between function definitions.

invert-number can be fixed up a bit. - can actually be used as an unary operator to do a negation, and inc is arguably more idiomatic than + 1 unless you think you may need to add to the equation later. I changed it to:

(defn invert-number2 [n]
  (inc (- n)))

Since the entire purpose of the function though seems to be just toggling between 0 and 1, I'd just write it as a more explicit toggle:

(defn invert-number2 [n]
  (if (zero? n) 1 0))

I feel like that conveys the purpose much clearer. I'd expect it to perform similarly too.

You can get rid of the first call to js/parseInt by just using int:

(map (comp invert-number2 int) numstr)

Honestly, I don't know why this works. int here is not acting like it does in Clojure (which is what I'm familiar with; I don't actually know Cljs). I'm guessing this is due to some weirdness on Javascript's end. If I plug that into a Cljs transpiler, I get some weird clues:

(int "192837465")

becomes

("192837465" | (0));

Because... Javascript happened?

The second js/parseInt is a little harder to deal with though because of the radix argument. I'd just stick with what you have.

(reduce str bits) can be changed to (apply str bits). Many variadic functions automatically manually reduce over their arguments (like str and +), so you you can alternatively apply the list directly to the function. (apply str makes a little more sense to me, but that's likely because I've written that many times before.

In the end, I ended up with:

(defn invert-number2 [n]
  (if (zero? n) 1 0))

(defn number-complement2 [num]
 (let [numstr (.toString num 2)
       bits (map (comp invert-number2 int) numstr)

       complement-bits (apply str bits)]

   (js/parseInt complement-bits 2)))

(println (number-complement2 10)
         (number-complement2 12)
         (number-complement2 15)
         (number-complement2 993061001)
         (number-complement2 123))

; 5 3 0 80680822 4


Remove all adjacent duplicates in a string using a Stack Input: careermonk
     Output:    camonk

  Input: mississippi
     Output:    m

I want to improve this code.

#include <iostream>
#include <string>

#define max 1000

class Stack
{
  int top;
public:
  char stk[max];
  Stack()
  {
    top = -1;
  }
  bool isStackEmpty();
  void push(char);
  char pop();
  std::string modifyString(std::string);
private:
  void removeAdjacentDuplicate(std::string);
};

bool Stack::isStackEmpty()
{
   if (top == -1)
      return true;
   else
      return false;
}

void Stack::push(char val)
{
   int s = max - 1;
   if (top < s)
   {
     top = top + 1;
     stk[top] = val;
   }
   else
    std::cerr << "Stack Overflow \n";
}

char Stack::pop()
{
   if (isStackEmpty() == true)
       std::cerr << "Stack Underflow \n";
   else
   {
      --top;
      return stk[top + 1];
   }
}

void Stack::removeAdjacentDuplicate(std::string str)
{
   int len = str.length();
   for (int i = 0; i < len; i++)
   {
      if (isStackEmpty() == true)
      {
         push(str[i]);
      }
      else if (str[i] == stk[top])
      {
         int discard = pop();
      }
      else
      {
         push(str[i]);
      }
   }
}

std::string Stack::modifyString(std::string str)
{
   removeAdjacentDuplicate(str);
   str.resize(top + 1);
   for (int i = 0; i <= top; i++)
   {
     str[i] = stk[i];
   }
   str[top + 1] = '\0';
   return str;
}

int main()
{
   Stack stack1;
   std::string str = "careermonk";
   std::cout << "Orignal string : " << str << "\n";
   str = stack1.modifyString(str);
   std::cout << "New string     : " << str << "\n";

   Stack stack2;
   std::string str2 = "mississippi";
   std::cout << "Original string : " << str2 << "\n";
   str2 = stack2.modifyString(str2);
   std::cout << "New string      : " << str2 << "\n";
}

std::array

You could use std::array and get rid of max define. In general, I'd like to suggest avoiding defines which can conflict with any stl names by any cost.

One of Windows header has its own #define max and this is horrible when your code wants std::numeric_limits<int>::max(), std::max({a, b, c}), etc.

Things are even worse if you're writing header only library:

#if defined max
#  undef max
#  define HAD_MAX
#endif

// ... C++ code ...

#if defined HAD_MAX
#  define max(a, b) ... // copy-paste from Windows.h
#endif

Stack::removeAdjacentDuplicate signature

Signature of Stack::removeAdjacentDuplicate(std::string) can be improved:

At least, it doesn't actually removes anything. It perform some kind of initialization from given string, so consider choosing more intuitive name.

Additionally, avoid copying parameter - you can use const std::string&

Stack::modifyString

Stack::modifyString also says nothing about its purpose and it also constructs another string instead of modifying something. Consider better naming.

Use standard library

When copying N characters, consider using standard library functions instead of for loop: strncpy, memcpy, std::copy_n and others works fine, looks clear and may perform some performance optimizations.

Use standard library (alternative)

As alternative, you can construct new string directly from Stack::stk. There is corresponding constructor: std::string(const char* s, size_t n);. In this case you can pass all std::string parameters by const reference.

BTW, I'm assure you're already bored with my variable naming suggestions, so I don't need to add another one here :)

Non-const public members are dangerous

Consider using some naming conventions

It's hard to determine whether some variable is a class member or not. In order to simplify your work, make member variables somehow. You can use m_member, member_, etc.


Sum all numbers in text using regexp I would like to improve everything that can be improved. I don't understand regex a lot and what could be wrong. The main improve that I am looking for is related with errors. I want make the code robust to deal well with all kinds of errors. I could try to run in production and wait to see the errors, but I can't. At my company, I need to try to imagine all possible scenarios, good or bad.

Codepen

'use strict';

var sum = function(text, regex) {

  var match;
  var regex = regex || /(\d+)/g;

  var result = []
  while (match = regex.exec(text)) {
    result.push(match[1]);
  }

  return result.reduce(function(prev, curr) {
    return parseInt(prev) + parseInt(curr);
  }, 0);
};

console.log(sum('1 12 40'));
console.log(sum('10 12 40'));
console.log(sum('1 12 510'));
console.log(sum('LLLLLLLL'));
console.log(sum('10 1'));

Numbers or integers ?

First of all, I notice that your question title talks about numbers, while your code looks only for positive integers.

In other words, it's currently not intended to take in account numbers such as 12.34 or -123.
To also detect and use this kind of numbers, your function would need a much more complex process.

Let's say you really want to remain limited to integers (I think so especially because of your parseInts). Then you may simplify your code in several ways.

Utility of regexargument ?

First I don't see why you're using a regex argument: for the purpose of your function, seems that it can't be anything else than the default you gave to it.
But this is up to you, and I'll keep this argument in what follows.

Declaring regex variable.

So just regarding it, you don't need to declare a new variable to use it: the regex argument your received has its scope limited to the function, and you may modify it directly. So you can simply write:

regex || /(\d+)/g;

Choosing the more efficient regex expression

Now regarding the way you expressed the regex: with its capturing parentheses, /(\d+)/g it makes your regex.exec(text) to return only one matching block each time, forcing you to the while() where you extract it.

At the opposite, using /\d+/g, you can merely do text.match(regex) and it'll return all matching blocks at once. Then you can directly reduce() this resulting array, though having to take care of the case it's null (like with your 'LLLLL').

Casting to numbers

While reduce() works, passed blocks contain only digits but are strings, so you're right to ensure they're processed as number instead.
But rather than parseInt (really useful only to extract integer part from a string where there are also other characters), you can merely cast curr to a number with +curr.

Using reduce().

You can take advantage of this reduce() capability: if you omit its 2nd argument (initial value for prev), then the 1st step is that curr is automatically affected to prev.
Note that in this case you must alos cast +prev!

Final reduced suggestion.

Last point, you can reduce the whole code by directly using the match() result in the reduce() process, without having to use other intermediate variables, assumed you take care of the possible null result from match().

Here is the suggested version:

'use strict';

var sum = function(text, regex) {
  var match;
  return !!(match = text.match(regex || /\d+/g)) ?
    match.reduce(function(prev, curr) {
      return +prev + +curr;
    })
  : 0;
}

console.log(sum('1 12 40'));
console.log(sum('10 12 40'));
console.log(sum('1 12 510'));
console.log(sum('LLLLLLLL'));
console.log(sum('10 1'));


Rolling Dice Simulation using Tkinter I just wanted to post my code to request for any improvement. Originally, I did NOT come up with this code, but rather I 'improved' it visually (i.e. not performance). This code originally had several if statements that I wanted to improvise with a dictionary.

In summary, this will generate a canvas named "c" that will start blank. As the click() function is performed, it will randomly select a side on a dice while also animating it. There is an actual 'dice roll' feature to this using Tkinter's grid.

enter code here
import tkinter as tk
from random import randint

def create_dice():
    """
    Creates the dice with all the sides drawn in a list.
    The sides are drawn by the draw_dice function.

    :return: dice list
    """
    dice = list()
    dice.append(draw_dice('dot0'))  # empty
    dice.append(draw_dice('dot4'))  # center dot --> 1
    dice.append(draw_dice('dot3', 'dot5'))  # dice head 2
    dice.append(draw_dice('dot2', 'dot4', 'dot6'))  # dice head 3
    dice.append(draw_dice('dot1', 'dot2', 'dot6', 'dot9'))  # dice head 4
    dice.append(draw_dice('dot1', 'dot2', 'dot4', 'dot6', 'dot9'))  # dice head 5
    dice.append(draw_dice('dot1', 'dot2', 'dot3', 'dot5', 'dot6', 'dot9'))  # dice head 6
    return dice

def draw_dice(*args):
    """
    Creates the individual heads passed in through the
    create_dice function.

    :param args: string(s) for certain dots for certain heads
    :return: c canvas
    """
    w, h = 23, 23 # sets width and height
    x, y, r = 2, 2, 5 # sets x, y, and radius
    c = tk.Canvas(root, width=w, height=h, bg='white') # creates canvas c

    #Dictionary containing lambda functions to draw dots on canvas c
    dots = {
        'dot0': lambda x, y, r: c,
        'dot1': lambda x, y, r: c.create_oval(x, y, x + r, y + r, fill='black'),
        'dot2': lambda x, y, r: c.create_oval(x + 16, y, (x + 16) + r, y + r, fill='black'),
        'dot3': lambda x, y, r: c.create_oval(x, y + 8, x + r, (y + 8) + r, fill='black'),
        'dot4': lambda x, y, r: c.create_oval(x + 8, (y + 8), (x + 8) + r, (y + 8) + r, fill='black'),
        'dot5': lambda x, y, r: c.create_oval(x + 16, (y + 8), (x + 16) + r, (y + 8) + r, fill='black'),
        'dot6': lambda x, y, r: c.create_oval(x, y + 16, x + r, (y + 16) + r, fill='black'),
        'dot9': lambda x, y, r: c.create_oval(x + 16, y + 16, (x + 16) + r, (y + 16) + r, fill='black')
    }

    for arg in args:
        dots.get(arg)(x, y, r) # Gets the dictionary keys while passing in x, y, and r values

    return c

def click():
    """
    Performs the operation of clicking the button. This will roll through
    the different dice heads

    :return: None
    """
    t = 100 # start with a time delay of 100 ms and increase it as the dice rolls
    stop = randint(13, 18) # chooses random number between 13 - 17
    for x in range(stop):
        dice_index = x % 6 + 1 # gets the randomly selected dice head by modulo
        dice_list[dice_index].grid(row=1, column=0, columnspan=3)
        root.update()
        if x == stop - 1:
            # set text to the selected result
            text.set(str(x % 6 + 1))
            break
        root.after(t, dice_list[dice_index].grid_forget()) # forgets the grid and restarts
        t += 25

# create the window form
root = tk.Tk()
root.title("Dice Roll")

# StringVar() updates result label automatically
text = tk.StringVar()

# set initial value of text
text.set("")

# create the result label
result = tk.Label(root, textvariable=text, fg='black')
result.grid(row=3, column=0, columnspan=3)
dice_list = create_dice()

# start with an empty canvas
dice_list[0].grid(row=1, column=0, columnspan=3)
button1 = tk.Button(root, text="Roll", command=click)
button1.grid(row=2, column=0, padx=3, pady=3)
button2 = tk.Button(root, text="Quit", command=root.destroy)
button2.grid(row=2, column=1, pady=3)

# start of program event loop
root.mainloop()

Magic number elimination:
make offsets 8, 16 of something constant in places like this:

'dot2': lambda x, y, r: c.create_oval(x + 16, y, (x + 16) + r, y + r, fill='black'),

you probably should make one function inside draw_dice instead of lambdas and use partial for partial application

def make_dot(x,y,r, color='black'):
    # actual code here
    pass

and then

'dot': partial(make_dot, x, y, x+8, y+8, r)