Drawing a triangle and some concentric circles For class, I had to use Java to draw a triangle and some concentric circles:

import javax.swing.*;
import java.awt.*;
import java.awt.geom.*;

public class JComponentDemo {

    private static final JComponent triangleComponent = new JComponent() {
        private static final long serialVersionUID = 1L;

        @Override
        public void paintComponent(Graphics g) {
            Graphics2D g2 = (Graphics2D) g;
            g2.draw(new Line2D.Double(180, 100, 100, 0));
            g2.draw(new Line2D.Double(20, 100, 100, 0));
            g2.draw(new Line2D.Double(180, 100, 20, 100));
        }
    };

    private static final JComponent ConcentricCircleMaker = new JComponent() {
        private static final long serialVersionUID = 1L;

        @Override
        public void paintComponent(Graphics g) {
            Graphics2D g2 = (Graphics2D) g;
            double[] circles = {40, 80, 120, 160, 200};
            Color[] colors = {Color.red, Color.green, Color.orange, Color.MAGENTA, Color.cyan};
            int count = 0;
            for (double i : circles) {
            g2.setColor(colors[count++]);
            g2.draw(Factory.get().makeCircle(200 - i / 2, 200 - i, i));
            }
        }
    };

    public static void main(String[] args) {
        JFrame frame = new JFrame("Drawing");
        frame.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
        frame.setBounds(50, 50, 500, 500);
        frame.setVisible(true);
        frame.setContentPane(triangleComponent);
        JOptionPane.showMessageDialog(null, "Click OK to Continue");
        frame.setVisible(false);
        frame.setContentPane(ConcentricCircleMaker);
        frame.setVisible(true);
    }

    private static class Factory {

        private static final FactorySingle factory;

        static {
            factory = new FactorySingle();
        }

        static FactorySingle get() {
            return factory;
        }

        private static class FactorySingle {

            public Ellipse2D.Double makeCircle(double x, double y, double length) {
            return new Ellipse2D.Double(x, y, length, length);
            }
        }
    }
}

I admit that I am terrible at commenting. I have some questions:

Is it a good practice just to import everything so I don't have to go back and add stuff?
Is it bad just to let NetBeans do serialVersionUID for me? (I don't like yellow squiggles!)
Are those anonymous classes smart or unnecessarily confusing?
Am I instantiating more than I need to?
Is this the best way to handle graphics/Graphics?
Do all those fields have to be final?
Should I make all of the objects final that don't need to change?
I tried to implement a Circle factory singleton in order not to repeat code. Are there problems? Is this worth it?
In general, are there ways to make my code more succinct?

Is it a good practice just to import everything so I don't have to go back and add stuff?

In a modern IDE (e.g. NetBeans) you can just type in the class you intend to use and quick-fix the import. I usually do this so that I don't have spare imports laying around inadvertently. Also, you can use the source cleanup tool to remove unused imports when you're done. I do this in Eclipse, but I'm exceptionally sure that a similar option is in NetBeans. Importing only what you need reduces compilation time.

  Is it bad just to let NetBeans do serialVersionUID for me? (I don't like yellow squiggles!)

Do you plan on "serializing" your class? Do you expect anyone else to? If so, you'll want that property to be set, so that different versions of your code can determine which version of the class you're deserializing to (or generate an error). You can use 1L for the first version, and increment it every time you add, remove, or change a member of the class. Or, you can just save that for last if you don't expect many changes when the class is done, and generate it then. If you really don't care about serializing the objects, just suppress the warning with an annotation.

  Are those anonymous classes smart or unnecessarily confusing?

The general rule of thumb is: if it's less than (approximately) 10 lines of code, and not frequently used (e.g. is used only once), than anonymous classes are perfectly acceptable, and in fact, encouraged. For code that is bound to be reused often, a named class should be preferred. The Java designers knew that there would often be small classes that only had a few lines of code that, in practice, wouldn't make sense to have in their own source files when one could simply inline the class definition and instantiation at once. Also, because anonymous classes share scope with the outer class, they can get access to the necessary variables and methods without having to accept an instance of the class, modifying member access modifiers, etc.

  Am I instantiating more than I need to?

There's no particular excess here, although this may not be the most efficient method you could have come up with. Unless you were under specific direction from a higher authority not to use certain constructs, such as BufferedImages, this almost qualifies as object abuse; components are generally meant to be a way to interact with users, not to just draw arbitrary objects. However, since we're only talking about two frames, it doesn't really matter, and I don't think it would be worth debating the merit of using components versus images.

  Is this the best way to handle graphics/Graphics?

Normally, one should create a sort of "canvas", and draw onto that. Here, you're changing the entire content of the window just to flop graphics around. Since there's only two frames, this works out okay, but in a game running at 60FPS, your players would get a headache and/or seizures. You could just as easily have made two BufferedImage elements, and flipped between the two, using a common component that you could draw to.

  Do all those fields have to be final?

Fields are final because you want to force certain behavior (e.g. a class cannot be inherited, a value can only be set once, etc). Assuming this was a module that you'd distribute to other developers to use, you'd want anything that should be locked down to be final. You might also use final to prevent yourself from writing code that accidentally modifies the variables. In short, it is a good practice, but it doesn't make sense logically in this example, because this program is sufficiently small enough that you wouldn't likely introduce such a logical error. I would recommend using final as frequently as necessary, but not necessarily here.

  Should I make all of the objects final that don't need to change?

Like before, it doesn't really matter in this example. You make values final so as to avoid accidental or intentional changes to the values. Also remember that final can affect inheritance, so if this were a module to a larger application, you might not want to lock everything down with final. In this case, most everything could be final just to prove to yourself that your code isn't making any unintentional changes. I would actually recommend having everything not be final, and for good measure, not static either.

  I tried to implement a Circle factory singleton in order not to repeat code. Are there problems? Is this worth it?

In a larger program, factories make sense. In this case, it makes no sense at all; the code needed to support the factory is larger than the savings of not having a factory at all. Consider this version (which does the same thing):

for (double i : circles) {
    g2.setColor(colors[count++]);
    g2.draw(new Ellipse2D.Double(200 - i / 2, 200 - i, i, i)));
}

It is perfectly legal, and in fact desirable, to instantiate anonymous, short-lived objects that are passed directly into the parameter list of the function, which is essentially what you're doing anyways. This lets people know, clearly, that you're simply creating a circle, instead of having to refer to the factory's source code to get an answer as to what is going on.

  In general, are there ways to make my code more succinct?

I believe I've outlined most of the problems in the prior answers, but let's go over them:

Drop the use of the factory, it's simply overkill in this case.
Drop the serial ids, especially in anonymous inner classes, as it is unlikely you'll ever serialize them. Warnings are okay, but you can use suppression annotations if you don't like the yellow squiggles (and I personally hate them; they're a guilt trip).
Use just one component to draw onto (or, one to manage the current view state, if you prefer).
Don't hide then show the frame. Instead, call invalidate() to force a repaint. No flickering.
Loading graphics and stuff as an initialization generally makes sense, but not here, because you're just wasting memory (although, not a ton, since this program only weighs in at a few KB, but, the principal remains). You should defer generating the two components until the last possible moment.
Generally speaking, your main app can be the JFrame; this is a normal design.

Without switching to images and all that, I can simply rearrange your code to the far simpler:

import javax.swing.*;
import java.awt.*;
import java.awt.geom.*;

public class JComponentDemo extends JFrame {

    public JComponentDemo() {
        super("Drawing");
    }

    public void start() {
        setDefaultCloseOperation(EXIT_ON_CLOSE);
        setBounds(50, 50, 500, 500);
        setVisible(true);
        setContentPane(
            new JComponent() {
            @Override
            public void paintComponent(Graphics g) {
                Graphics2D g2 = (Graphics2D) g;
                g2.draw(new Line2D.Double(180, 100, 100, 0));
                g2.draw(new Line2D.Double(20, 100, 100, 0));
                g2.draw(new Line2D.Double(180, 100, 20, 100));
            }
        });
        JOptionPane.showMessageDialog(null, "Click OK to Continue");
        setContentPane(new JComponent() {
            @Override
            public void paintComponent(Graphics g) {
                Graphics2D g2 = (Graphics2D) g;
                double[] circles = {40, 80, 120, 160, 200};
                Color[] colors = {Color.red, Color.green, Color.orange, Color.MAGENTA, Color.cyan};
                int count = 0;
                for (double i : circles) {
                    g2.setColor(colors[count++]);
                    g2.draw(new Ellipse2D.Double(200 - i / 2, 200 - i, i, i));
                }
            }
        });
        setVisible(true);
    }

    public static void main(String[] args) {
        new JComponentDemo().start();
    }
}

Note that anonymous inner classes are only good for "small" things, so this design wouldn't scale well (imagine a UI with hundreds of components). Regardless, you would still want to instantiate those elements either in a constructor, or in a thread that allocates the objects as the program is starting up or running so that the elements are available when needed.


Speeding up Dijkstra's algorithm I have Dijkstra's algorithm:

# ==========================================================================
# We will create a dictionary to represent the graph
# =============================================================================
graph = {
    'a' : {'b':3,'c':4, 'd':7},
    'b' : {'c':1,'f':5},
    'c' : {'f':6,'d':2},
    'd' : {'e':3, 'g':6},
    'e' : {'g':3, 'h':4},
    'f' : {'e':1, 'h':8},
    'g' : {'h':2},
    'h' : {'g':2}
}

def dijkstra(graph, start, goal):
    shortest_distance = {}     # dictionary to record the cost to reach to node. We will constantly update this dictionary as we move along the graph.
    track_predecessor = {}     # dictionary to keep track of path that led to that node.
    unseenNodes = graph.copy() # to iterate through all nodes
    infinity = 99999999999     # infinity can be considered a very large number
    track_path = []            # dictionary to record as we trace back our journey

    # Initially we want to assign 0 as the cost to reach to source node and infinity as cost to all other nodes
    for node in unseenNodes:
        shortest_distance[node] = infinity
    shortest_distance[start] = 0

    # The loop will keep running until we have entirely exhausted the graph, until we have seen all the nodes
    # To iterate through the graph, we need to determine the min_distance_node every time.
    while unseenNodes:
        min_distance_node = None
        for node in unseenNodes:
            if min_distance_node is None:
                min_distance_node = node
            elif shortest_distance[node] < shortest_distance[min_distance_node]:
                min_distance_node = node

        # From the minimum node, what are our possible paths
        path_options = graph[min_distance_node].items()

        # We have to calculate the cost each time for each path we take and only update it if it is lower than the existing cost
        for child_node, weight in path_options:
            if weight + shortest_distance[min_distance_node] < shortest_distance[child_node]:
                shortest_distance[child_node] = weight + shortest_distance[min_distance_node]
                track_predecessor[child_node] = min_distance_node

        # We want to pop out the nodes that we have just visited so that we dont iterate over them again.
        unseenNodes.pop(min_distance_node)

    # Once we have reached the destination node, we want trace back our path and calculate the total accumulated cost.
    currentNode = goal
    while currentNode != start:
        try:
            track_path.insert(0, currentNode)
            currentNode = track_predecessor[currentNode]
        except KeyError:
            print('Path not reachable')
            break
    track_path.insert(0, start)

    #  If the cost is infinity, the node had not been reached.
    if shortest_distance[goal] != infinity:
        print('Shortest distance is ' + str(shortest_distance[goal]))
        print('And the path is ' + str(track_path))

It works fine if I have a small amount of nodes (like in the code), but I have graph with around 480 000 nodes and by my approximation, in such a large graph, it will take 7.5 hours, and that is only 1 way! How could I make it work faster? In OSM, for instance it calculates in seconds!

PEP-8 Violations

Variables (unseenNodes, currentNode) should be snake_case, not camelCase.

Infinity

99999999999 is no where near infinity.  If you want to use infinity, use the real infinity: float("+inf"), or in Python 3.5+, math.inf

Conversion to strings

Python's print() statement automatically converts any non-string object into a string, for printing.  You don't have to.  This:

    print('Shortest distance is ' + str(shortest_distance[goal]))
    print('And the path is ' + str(track_path))

can be written as:

    print('Shortest distance is', shortest_distance[goal])
    print('And the path is', track_path)

which is slightly shorter to type.  As a bonus, it is slightly more efficient, as it does not need to create, and subsequently deallocate, a new string object which is the concatenation of the the strings.

If you are using Python 3.6+, you might want to use f-strings:

    print(f'Shortest distance is {shortest_distance[goal]}')
    print(f'And the path is {track_path}')

which interpolates values directly into the strings.

Return Value

Your function finds the path, prints the result, and does not return anything.  This is not useful if you wish to use the discovered path in any other fashion.  The function should compute the result and return it.  The caller should be responsible for printing the result.

Document the code

True, you have plenty of comments.  But the caller can only guess at what the functions arguments are supposed to be, and what the function returns.  You should document this with type-hints, and """doc-strings""".  Something like:

from typing import Any, Mapping, Tuple, List

Node = Any
Edges = Mapping[Node, float]
Graph = Mapping[Node, Edges]

def dijkstra(graph: Graph, start: Node, goal: Node) -> Tuple[float, List]:
    """
    Find the shortest distance between two nodes in a graph, and
    the path that produces that distance.

    The graph is defined as a mapping from Nodes to a Map of nodes which
    can be directly reached from that node, and the corresponding distance.

    Returns:
        A tuple containing
            - the distance between the start and goal nodes
            - the path as a list of nodes from the start to goal.

    If no path can be found, the distance is returned as infinite, and the
    path is an empty list.
    """

Avoid multiple lookups

In this code:

    min_distance_node = None
    for node in unseenNodes:
        if min_distance_node is None:
            min_distance_node = node
        elif shortest_distance[node] < shortest_distance[min_distance_node]:
            min_distance_node = node

you continuously look up shortest_distance[min_distance_node].  In compiled languages, the compiler may be able to perform data-flow analysis, and determine that the value only need to be looked up again if min_distance_node changes.  In an interpreted language like Python, where a lookup action can execute user-defined code and change the value, each and every lookup operation must be executed.  shortest_distance[min_distance_node] is two variable lookups plus a dictionary indexing operation.  Compare with:

    min_distance_node = None
    min_distance = infinity
    for node in unseenNodes:
        distance = shortest_distance[node]
        if min_distance_node is None:
            min_distance_node = node
            min_distance = distance
        elif distance < min_distance:
            min_distance_node = node
            min_distance = distance

This code will run faster, due to less lookups of shortest_distance[min_distance_node] and shortest_distance[node].

But finding the minimum of a list is such a common operation, that Python has a built-in function for doing this: min(iterable, *, key, default).  The key argument is used to specify an ordering function ... in this case, a mapping from node to distance.  The default can be used to prevent a ValueError if there are no nodes left, which is unnecessary in this case.

    min_distance_node = min(unseenNodes, key=lambda node: shortest_distance[node])

In the same vein:

    for child_node, weight in path_options:
        if weight + shortest_distance[min_distance_node] < shortest_distance[child_node]:
            shortest_distance[child_node] = weight + shortest_distance[min_distance_node]
            track_predecessor[child_node] = min_distance_node

repeatedly looks up shortest_distance[min_distance_node]; again, two variable lookups and a dictionary indexing operation.  Again, we can move this out of the loop:

    min_distance = shortest_distance[min_distance_node]
    for child_node, weight in path_options:
        if weight + min_distance < shortest_distance[child_node]:
            shortest_distance[child_node] = weight + min_distance
            track_predecessor[child_node] = min_distance_node

Reducing the Working Set

The code to find the min_distance_node: how many nodes does it check?  In your toy graph "a" to "h", on the first iteration, it needs to search 8 nodes.  With 480 000 nodes, it would need to search 480 000 nodes!  In the second iteration, one node has been removed from unseenNodes, so the it would search one node less.  7 nodes is fine, but 479 999 nodes is a huge number of nodes.

How many nodes does "a" connect to?  Only 3.  The min_distance_node will become one of those 3 nodes.  Searching the remaining nodes (with infinite distances) isn't necessary.  If you added to the unseenNodes only the nodes which can be reached at each step of the algorithm, your search space would reduce from several thousand nodes to a couple of hundred.

Moreover, if you maintained these unseenNodes in a sorted order by distance, the min_distance_node would always be the first node in this “priority queue”, and you wouldn’t need to search through the unseenNodes at all.

Maintaining the unseen nodes in a priority queue is easily done through a min-heap structure, which is built into Python (heapq):

from math import inf, isinf
from heapq import heappush, heappop
from typing import Any, Mapping, Tuple, List

Node = Any
Edges = Mapping[Node, float]
Graph = Mapping[Node, Edges]

def dijkstra(graph: Graph, start: Node, goal: Node) -> Tuple[float, List]:
    """
    Find the shortest distance between two nodes in a graph, and
    the path that produces that distance.

    The graph is defined as a mapping from Nodes to a Map of nodes which
    can be directly reached from that node, and the corresponding distance.

    Returns:
        A tuple containing
            - the distance between the start and goal nodes
            - the path as a list of nodes from the start to goal.

    If no path can be found, the distance is returned as infinite, and the
    path is an empty list.
    """

    shortest_distance = {}
    predecessor = {}
    heap = []

    heappush(heap, (0, start, None))

    while heap:

        distance, node, previous = heappop(heap)

        if node in shortest_distance:
            continue

        shortest_distance[node] = distance
        predecessor[node] = previous

        if node == goal:

            path = []
            while node:
                path.append(node)
                node = predecessor[node]

            return distance, path[::-1]

        else:
            for successor, dist in graph[node].items():
                heappush(heap, (distance + dist, successor, node))

    else:

        return inf, []

if __name__ == '__main__':
    graph = {
        'a' : {'b':3, 'c':4, 'd':7},
        'b' : {'c':1, 'f':5},
        'c' : {'f':6, 'd':2},
        'd' : {'e':3, 'g':6},
        'e' : {'g':3, 'h':4},
        'f' : {'e':1, 'h':8},
        'g' : {'h':2},
        'h' : {'g':2}
    }

    distance, path = dijkstra(graph, 'a', 'e')
    if isinf(distance):
        print("No path")
    else:
        print(f"Distance = {distance}, path={path}")

OSM

By 'OSM' do you mean "Open Street Maps"?  If so, you are using the wrong algorithm.  Map nodes have coordinates, which can be use as "hints", to direct the search in a given direction.  See A* Search Algorithm


Multiple calls to the GitHub API I have the following class:

public class IssueWithComments : Issue
{
    public IReadOnlyList<IssueComment> FullComments { get; set; }

    public IssueWithComments()
        : this(new List<IssueComment>())
    {
    }

    public IssueWithComments(IEnumerable<IssueComment> comments)
    {
        FullComments = comments.ToList();

    }

    public static IssueWithComments FromIssue(Issue issue, IEnumerable<IssueComment> comments)
    {
        return new IssueWithComments(comments)
        {
            Assignee = issue.Assignee,
            Body = issue.Body,
            ClosedAt = issue.ClosedAt,
            Comments = issue.Comments,
            CreatedAt = issue.CreatedAt,
            HtmlUrl = issue.HtmlUrl,
            Labels = issue.Labels,
            Milestone = issue.Milestone,
            Number = issue.Number,
            PullRequest = issue.PullRequest,
            State = issue.State,
            Title = issue.Title,
            UpdatedAt = issue.UpdatedAt,
            Url = issue.Url,
            User = issue.User
        };
    }
}

where Issue is a type defined in the Octokit.Net namespace.

I populate my objects by calling the following code:

public static async Task<IReadOnlyList<IssueWithComments>> GetAllForRepositoryWithComments(this IIssuesClient client, string owner, string name)
{
     var issues = await client.GetAllForRepository(owner, name);
     List<IssueWithComments> all = new List<IssueWithComments>();

     foreach (var i in issues)
     {
         all.Add(IssueWithComments.FromIssue(i, await client.Comment.GetAllForIssue(owner, name, i.Number)));
     }

     return all;
 }

This works, but just takes 7.640 seconds on average according to System.Diagnostics.Stopwatch (plus that I can see it taking a long time to load).

Task.WhenAll could work, but the way to find the Issue it is referring to is as follows:

x.FullComments.First().HtmlUrl.Segments[4]

Is there a better way to either run the loop or should I go with Task.WhenAll and drill into the URL?

Since you're loading all comments for all issues, I think

await client.Comment.GetAllForRepository(owner, name)

is what you're looking for. Otherwise, if you do need to use GetAllForIssue, try using Task.WhenAll to run comments retrieval in parallel:

public static async Task<IReadOnlyList<IssueWithComments>> GetAllForRepositoryWithComments(IIssuesClient client, string owner, string name)
{
    var issues = await client.GetAllForRepository(owner, name);

    return await Task.WhenAll(issues.Select(async i => IssueWithComments.FromIssue(i, await client.Comment.GetAllForIssue(owner, name, i.Number)));
}


Prim's Lazy Minimum Spanning Tree Algorithm in Haskell (Project Euler# 107) I have implemented Lazy version of Prim's Minimum Spanning Tree Algorithm. I want to improve the code structure, follow prevalent conventions and reduce code size. I am solving Project Euler #107.

Explanation:

I do as follows for MST:

Initialize an adjacency list, visited set with vertice #0, min-priority heap/queue for edges with shortest edge from vertice #0, sum of edges in MST as 0.
Find next edge, if it is not a crossing edge, recurse.
Otherwise I include that edge (by updating visited and heap/queue) and add to MST sum the edge's weight.

Note: Non-existent edges have been assigned value -1.

Code:

import           Data.Array
import qualified Data.Heap       as Heap
import           Data.List.Split
import           Data.Maybe
import qualified Data.Set        as Set

dim = 40 :: Int

main :: IO ()
main = print . maximumSaving . toAdjacencyMatrix . map (wordsBy (==',')) . lines
    =<< readFile "txt/107.txt"
    where
        toAdjacencyMatrix mat = array ((0,0),(dim-1,dim-1))
            [ ((i,j), if val == "-"  then -1 else read val)
                | i <- [0..dim-1], j <- [0..dim-1], let val = mat !! i !! j]
        totalWeight network = sum [network ! (i,j) | i <- [0..dim-1], j <- [0..dim-1], i < j, network ! (i, j) > 0]
        maximumSaving network = totalWeight network - minimumSpanningTreeEdgeSum network

minimumSpanningTreeEdgeSum :: Array (Int, Int) Int -> Int
minimumSpanningTreeEdgeSum adj =
    minimumSpanningTree'
        adj
        (Set.singleton 0)
        (Heap.fromList [(adj!(0,a),(0, a)) | a <- [1..dim-1], adj ! (0,a) > 0])
        0
    where
        minimumSpanningTree' :: Array (Int, Int) Int -> Set.Set Int -> Heap.MinPrioHeap Int (Int, Int) -> Int -> Int
        minimumSpanningTree' adj visited queue sm = case Heap.viewHead queue of
            Nothing          -> sm
            (Just (weight, edge)) ->
                if isCrossingEdge edge then
                    let sm' = sm + weight in
                    let nextVertice = if Set.notMember (fst edge) visited then fst edge else snd edge in
                    let visited' = Set.insert nextVertice visited in
                    let newEdges = [(i, j) | a <- [0..dim-1], let i = min nextVertice a, let j = max nextVertice a, Set.notMember i visited, adj ! (i,j) > 0 ] in
                    let queue'' = foldl (flip Heap.insert) queue' $ map (\e -> (adj ! e, e)) newEdges
                    in minimumSpanningTree' adj visited' queue'' sm'
                else
                    minimumSpanningTree' adj visited queue' sm
            where
                queue' = fromJust $ Heap.viewTail queue
                isCrossingEdge edge = Set.notMember (fst edge) visited ||
                    Set.notMember (snd edge) visited

I would separate the looping queue logic from the rest. A general implementation of unrollM necessitates a state monad here.

import Control.Monad.State
import Control.Monad.Loops

unrollM :: (Monad m, Ord a) => (a -> m [a]) -> [a] -> m [a]
unrollM f = step . (Heap.fromList :: Ord a => [a] -> Heap.MinHeap a) where
  step queue = case Heap.view queue of
    Nothing -> pure []
    Just (x, queue') -> do
      as <- f x
      (x:) <$> step (foldl (flip Heap.insert) queue' as)

minimumSpanningTreeEdgeSum :: Array (Int, Int) Int -> Int
minimumSpanningTreeEdgeSum adj = sum $ map fst $ (`evalState` Set.singleton 0) $ unrollM minimumSpanningTree'
  [(adj!(0,a),(0, a)) | a <- [1..dim-1], adj ! (0,a) > 0] where
  minimumSpanningTree' :: (Int, (Int, Int)) -> State (Set.Set Int) [(Int, (Int, Int))]
  minimumSpanningTree' (weight, (from, to)) = firstM (gets . Set.notMember) [from, to] >>= \case
    Nothing -> pure []
    Just nextVertex -> do
      modify $ Set.insert nextVertex
      filterM (gets . Set.notMember . fst . snd) $ filter ((>0) . fst)
        [ (adj ! (i,j), (i, j))
        | a <- [0..dim-1]
        , let [i,j] = sort [nextVertex, a]
        ]


Applying functions to decorated objects I have an interface Animal with a class that implements it, Cat.
public interface Animal {
    public int price();
    public Breed.BreedType breedType();
}

public class Cat implements Animal {

    private int price;
    private Breed.BreedType breedType;

    public Cat(int price) {
        this.price = price;
    }

    public Cat(int price, Breed.BreedType breedType) {
        this.price = price;
        this.breedType = breedType;
    }

    @Override
    public int price() {
        return price;
    }

    @Override
    public Breed.BreedType breedType() {
        return breedType;
    }
}

I have decorated the Animal class with the following interface, Breed.
import java.util.List;

public interface Breed {
    public enum BreedType {
        RARE,
        EXOTIC
    }

    public int calculatePrice(List<Animal> animals);
}

The concrete decorator class looks as follows:
import java.util.List;

public class Exotic implements Breed {

    @Override
    public int calculatePrice(List<Animal> animals) {
        return animals.stream()
                .mapToInt(Animal::price)
                .map(i -> i * 2)
                .sum();
    }
}

Finally I iterate over a collection of animals and apply the decorator to them to work out their total cost of special breed animals.
import java.util.List;
import java.util.Map;
import java.util.stream.Collectors;
import java.util.stream.Stream;

public class Main {

    public static void main(String[] args) {
        Cat cat1 = new Cat(100);
        Cat cat2 = new Cat(100, Breed.BreedType.EXOTIC);
        Cat cat3 = new Cat(100, Breed.BreedType.EXOTIC);

        Map<Breed.BreedType, List<Animal>> map = Stream.of(cat1, cat2, cat3)
                .filter(animal -> animal.breedType() != null)
                .collect(Collectors.groupingBy(Animal::breedType, Collectors.toList()));

        int total = 0;
        for(Breed.BreedType breedType : map.keySet()) {
            if(breedType.equals(Breed.BreedType.EXOTIC)) {
                total += new Exotic().calculatePrice(map.get(breedType));
            }
        }
        System.out.println(total);
    }
}

This does not seem particularly flexible as I will end up with a giant if statement as more breeds are added. Is there a better way to do this? My enum and the corresponding method for the Exotic breed seem a bit disconnected.

I'm not much of a stream fan and your code itself seems rather good, so I'll skip directly to your architecture.
One note, through:
total += new Exotic().calculatePrice(map.get(breedType))

a += b is not shorthand for a = a + b, it is shorthand for a = (TYPE_A)(a + b). This is something to keep in mind, as it might silently truncate data, for example if a is an int and b is a float, no warning will be emitted by the compiler.

Let's sum it up:
interface Breed
    enum BreedType

interface Animal

class Cat implements Animal

What is odd here is that you have a Breed type, then decide not to use it and instead use a specialized enum for representing the breed. So what I'd rather expect would be to have a Cat which has the property Breed. Additionally, your handling of the price of a cat is odd. The cat gets assigned a price, which is then changed afterwards based on the breed, which means it really is more of a "base price" then the actual price of this cat.
So what I'd expect is the following setup:
public interface Breed {}

public class Cat implements Animal {
    public Breed getBreed();
    public int getPrice();
}

With getPrice here returning the actual price of this single cat. Or, an alternative would be to have an extra price calculator:
public interface Breed {}

public class Cat implements Animal {
    public Breed getBreed();
}

public class CatPriceCalculator {
    public int calculatePrice(Cat cat);
}

Depending on what you're trying to achieve, both solutions might be viable. But let's go with the first for now. To mimic your original code, let's implement Breed:
public interface Breed {
    // TODO Note that we do not support fractions here.
    public int getPriceMultiplier();
}

public class RegularBreed implements Breed {
    public int getPriceMultiplier() {
        return 1;
    }
}
public class ExitocBreed implements Breed {
    public int getPriceMultiplier() {
        return 2;
    }
}

This will allow us to calculate the price of a single cat by doing cat.getBasePrice() * cat.getBreed().getPriceMultiplier(). We could also map breeds as enum, similar to how you've done it:
public enum Breed {
    EXOTIC(2),
    REGULAR(1);

    private int priceMultiplier;

    private Breed(int priceMultiplier) {
        this.priceMultiplier = priceMultiplier;
    }

    public int getPriceMultiplier() {
        return priceMultiplier;
    }
}

But this has the downside that it is not extensible from the outside, as enums cannot be extended. So I'd suggest to stick with the interface for this one.
Looking again at my second suggestion to use an extra price calculator, this would have the upside that you can map the price of a single cat base on more properties, for example weight, color and so forth:
public class Cat extends Animal {
    public int getAge();
    public Breed getBreed();
    public Color getFurColor();
    public Quality getFurQuality();
    public int getWeight();
}

Now if we have an extra calculator class, all these properties can be weighted in there. Having an extra class for this might for example be necessary if we plan to support different markets. For example, a market of a cat-show will assess fur and color higher than other properties. On the other side, a meat market will assess age and weight higher than other properties. So decoupling the price calculation from your model classes might be preferable.


Setting all possible combinations of values I have a circumstance where I need to create every single possible version of an object upon a particular magnitude. After this, a process occurs which determines the best combination of values at this magnitude and then sets these for the next iteration of the magnitude reduction loop.

const int magReductions = 2//As an example; would be user decided
int currentItem;
double bestVal1 = 5,
       bestVal2 = 5,
       bestVal3 = 5,
       bestVal4 = 5,
       bestVal5 = 5;

for (double magnitude = 1; magnitude > pow(10, -magReductions); magnitude /= 10) {
    for (int a = 0; a < 10; a++) {
        for (int b = 0; b < 10; b++) {
            for (int c = 0; c < 10; c++) {
                for (int d = 0; d < 10; d++) {
                    for (int e = 0; e < 10; e++) {
                        currentItem = (10000 * a) + (1000 * b) + (100 * c) + (10 * d) + e;
                        objectList[currentItem].val1 = ((a - 5) * magnitude) + bestVal1;
                        objectList[currentItem].val2 = ((b - 5) * magnitude) + bestVal2;
                        objectList[currentItem].val3 = ((c - 5) * magnitude) + bestVal3;
                        objectList[currentItem].val4 = ((d - 5) * magnitude) + bestVal4;
                        objectList[currentItem].val5 = ((e - 5) * magnitude) + bestVal5;
                    }
                }
            }
        }
    }
//function calculating best values
}

It is very hard to give a concise example of this in action due its nature of being very heavy on computing power.

Quite a while after writing this I look at it with quite a lot of suspicion that it is rather badly done, although no alternative method seems to present itself to me. I suppose I ask the very typical question of this site, 'Is there a better way?'.

Based on how you store objects at indices determined by the init values, and not stored in sequential order, I'm going to guess that you have a function somewhere that takes an object's index and returns the matching values of a,b,c,d,e.

Let's say it's std::tuple<int,int,int,int.int> get_values(int obj_index);

You could leverage that same function to clean up the generating loop:

for(int i=0; i < 10*10*10*10*10; ++i) {
  auto values = get_values(i);
  objectList[i].val1 = ((std::get<0>(values) - 5) * magnitude) + bestVal1;
  objectList[i].val2 = ((std::get<1>(values) - 5) * magnitude) + bestVal2;
  objectList[i].val3 = ((std::get<2>(values) - 5) * magnitude) + bestVal3;
  objectList[i].val4 = ((std::get<3>(values) - 5) * magnitude) + bestVal4;
  objectList[i].val5 = ((std::get<4>(values) - 5) * magnitude) + bestVal5;
}

If you don't have such a function, then indexing the objects in the way you do makes no sense, and you should just be storing them in order of generation.

Apart from that, my other recommendations would be too dependant on the context.

For instance, I would look for a way to not have to store the entire table of objects, and implement the evaluation algorithm in a way that at most a few objects have to be in existence at the same time.

I would also consider that brute-forcing an optimization problem like this is generally a last resort, so I would probably look for tackling this with something like a gradient descent if possible.


Console random password generator This program is a random password generator. It asks the user how many chars they want their password to be and how many passwords it should generate.
I used the rand() and srand() methods to generate random letters.

I used dynamic allocation to create a char array based on user input. The comment line is where I tried to dynamically allocate an array of pointers, where each pointer would point to a char array, but I couldn't figure out how to do it so I used a counter and a do while loop and a for loop to create passwords based on user input.

Please give me advice on how I can improve or what I can do differently!

#include <iostream>
#include <string>
#include <cstdlib>
#include <ctime>

const int MAX = 90;
const int MIN = 65;

char * createPassword();

int main()
{
    char * p = createPassword();

    return 0;
}

char * createPassword()
{
    unsigned seed = time(0);

    srand(seed);

    char x = ' ';
    int passwordLength = 0;
    int numOfPasswords = 0;

    std::cout << "How many chars in password: ";
    std::cin >> passwordLength;
    char * pwptr = new char[passwordLength];

    std::cout << "How many passwords should be generated?";
    std::cin >> numOfPasswords;
    //char * passwords = new char *pwptr[numOfPasswords];

    int passwordcount = 0;

    do{
        for(int cnt = 0; cnt < passwordLength; cnt++)
        {
            x = (rand() % (MAX - MIN + 1)) + MIN;
            pwptr[cnt] = x;
            std::cout << pwptr[cnt];
        }
        std::cout << std::endl;
        passwordcount++;
        } while(passwordcount != numOfPasswords);

        return pwptr;
}

A few things immediately jump out:

Use std::string instead of char*.
Your string is not null terminated (use of std::string fixes this)
Don't create two globals when they're only used in one place.
Don't create globals with an "uppercase" name. By common practice this is reserved for macros.
srand() and rand() are superseded by any RNG from the <random> header.
Don't use a do...while when a for-loop does the same job more succinctly.
Your createPassword function does two things:

It communicates with the user.
It creates a new password.

Split the logic into 2 functions.


Mixing Linq queries and Linq lambdas I have been given a change to rewrite an old framework we use, and to implement the repository and unit of work patterns, but in an attempt to not rewriting all queries I have ended up with a weird combination of Linq queries and Linq lambdas.

I was hoping someone could explain to me a better way of doing this, or advice what to do.

First off, the generic repository has a method Get() defined as

public virtual IEnumerable<TEntity> Get(
Expression<Func<TEntity, bool>> filter = null,
Func<IQueryable<TEntity>, IOrderedQueryable<TEntity>> orderBy = null,
string includeProperties = "")

and the old code would use a context directly like this:

Arrival arrival =
    (from a in DataContextPortX.Arrival
         where a.I_ARRIVALNO == arrivalNo
     select a).SingleOrDefault();

And now, since the class no longer has direct access to the context I can choose to keep the old code with the Get() method mixed in like this:

Arrival arrival =
    (from a in Get()
        where a.I_ARRIVALNO == arrivalNo
        select a).SingleOrDefault();

or I can rewrite the whole query to lambda like this:

var arrival = Get(a => a.I_ARRIVALNO == arrivalNo).SingleOrDefault();

I would of course prefer to rewrite all queries but there are a lot of them, and this was the shortest I could find, others are like really long.

But I am worried about first using Get() and then iterating of that, but for all I know it doesn't matter.

There is a reason why you can write LINQ queries using the query syntax or the method syntax: sometimes one is better and sometimes it's the other one.

So, when you're writing a new query, use the syntax that suits your query better. This usually means method syntax for simpler queries (especially if they're as simple as your example) and query syntax for more complicated queries (especially ones with multiple sources).

When you need to use an operator that's only available in method syntax (like SingleOrDefault()), I think that points towards using method syntax for the whole query. But using both syntaxes in the same query can sometimes make sense too.

When it comes to rewriting old code: if it works and it's reasonably readable, I would leave it be, I think the effort spent rewriting it isn't worth it.


Testing a simple guard clause in isolation I’m using decorator objects in a Rails app so that I don’t have to do nil? checks in views. Everything is working nicely and it’s easy to test, but I’m suspicious that my test:code ratio is a little high. Am I being too verbose? Is there a better way this could be done?

Test suite:

# spec/decorators/client_decorator_spec.rb
require_relative "../../app/decorators/client_decorator"

describe ClientDecorator do
  describe ".position" do
    context "with a recorded client position" do
      it "returns the name of the position" do
        occupation = double("Occupation", name: "CEO")
        client = double("Client", occupation: occupation)
        expect(ClientDecorator.new(client).position).to eq "CEO"
      end
    end

    context "with no recorded position" do
      it "returns an empty string" do
        client = double("Client", occupation: nil)
        expect(ClientDecorator.new(client).position).to eq ""
      end
    end
  end
end

Production code:

# app/decorators/client_decorator.rb
require "simple_decorator"

class ClientDecorator < SimpleDecorator
  def position
    return "" unless occupation
    occupation.name
  end
end

Yeah, the code/test ratio is probably a little high - especially if you have to do this for many attributes.

May I direct your attention to the #try method? Seems to me you that you might be able to skip a lot of code (and tests) by doing things like @client.try(:occupation).try(:name).

#try simply returns nil if the receiver doesn't respond to the whatever message you're sending. And since #try is also implemented on the nil object, you can chain "tries" together.

In you views, writing something like <%= @client.try(:occupation).try(:name) %> will just print nothing if the try-chain ends up returning nil - same as your current setup. And if you want a different fallback, you can do this:

<%= @client.try(:occupation).try(:name) || "N/A" %>

In other words, having an entire decorator seems a bit much just to absorb nils. But granted, the view ends up having to dig a bit further into the models.

You can of course also keep the decorator (at least so you current code doesn't break elsewhere), but simply rewrite it using #try to something like

# app/decorators/client_decorator.rb
require "simple_decorator"

class ClientDecorator < SimpleDecorator
  def position
    occupation.try(:name) || ""
  end
end

which I honestly don't know if I'd bother to test very rigorously (lest it breaks horribly).


Improving a filter array function, to match elements and conditions I have to do some check/match elements with conditions.
A have a list of conditions, each with a specific ID.
I've put for each of my elements an array with the ids of their respective affirmative conditions.
Some elements can share a condition. The goal is to, everytime a condition is "checked" by the user, the code filter the array with all elements, showing only the ones that attend the conditions.
I've made this function. But I don't know if its the "best" way..
this.selectedConditions is already maped by ID
Any help?

private filterElements(){

    let filtered: any[] = [];

    originalArray.forEach( item => {

      let isPossibility: boolean = true;

      for (let i = 0; i < this.selectedConditions.length; i++) {

        if( item.condition_matrix.indexOf( this.selectedConditions[i] ) === -1 ){
          isPossibility = false;
          break;
        }
      }

      if( isPossibility ){
        filtered.push(item);
      }

    });

    this.outputArray = filtered;

    if(this.outputArray.length === 0){
      this.outputArray.push( { <a_filler_object> } );
    }
}

Elements list example:
elements: [
    {
        name: 'el1',
        condition_matrix: [1,2]
    },
    {
        name: 'el2',
        condition_matrix: [2,3]
    },
    {
        name: 'el3',
        condition_matrix: [2,1]
    }
]

Conditions list example:
conditions: [
    {
        id: 1
        name: 'cond1'
    },
    {
        id: 2
        name: 'cond2'
    },
    {
        id: 3
        name: 'cond3'
    }
]

It looks like you're using TypeScript, so a couple pointers:
Avoid any - any effectively disables type-checking, which defeats the whole point of using TypeScript. Better to type variables properly:
let filtered: Array<{ name: string; condition_matrix: Array<number>; }> = [];

Or, even better, let TypeScript infer the types automatically when possible, by transforming the data all at once with array methods (see below). No need to denote types when TS can infer them automatically - eg let isPossibility: boolean = true; can be just let isPossibility = true;
Avoid let, prefer const - see here, using const indicates that the variable will never be reassigned, which reduces cognitive overhead.
Semantic array methods While for loops work, they should only be used as a last resort, when other array methods aren't appropriate. Here, rather than breaking inside a for loop to check if an item passes a condition, it would be better to use .some. Also, rather than .pushing to an outer array based on the condition, use .filter instead:
this.outputArray = originalArray.filter(
  item => this.selectedConditions.some(
    condition => item.condition_matrix.includes(condition)
  )
);

(Better to use .includes than to use an indexOf check against 0)
Or:
Time complexity If performance is an issue and you have a lot of conditions, rather than iterating all of the conditions each time, consider constructing a Set of all of the selectedConditions first; Set#has is O(1), which is an improvement over iterating over arrays with O(n).
const allConditions = new Set(this.selectedConditions);

Conditions list typo? The data structure in your code is an array of objects, but you're using item.condition_matrix.indexOf( this.selectedConditions[i] ). Did you mean:
item.condition_matrix.indexOf( this.selectedConditions[i].id )

?
(If so, and you want to use the Set method, map to IDs first:
const allConditions = new Set(
  this.selectedConditions.map(({ id ]) => id)
);

)
Side-effects? The result of the whole function is to set a value (to this.outputArray). Is this required? Code is generally easiest to understand and reason about when it does not contain side-effects. Imagine seeing some code you haven't looked at before, and compare:
this.calculate();
console.log('Result:', this.result);

vs
console.log('Result:', this.calculate());

The second is easier to make sense of because the method is used for its return value instead of for its side-effects (which, to understand, you'd have to read through the method to find).
Could your filterElements method return a value instead of assigning to this.outputArray?


Communicating events between entity components with minimal code overhead I'm working on a game which utilises a variant of the entity component system pattern. In my current code, I have been communicating between components using code of the form:

_entity.GetComponent<MovementComponent>().TargetPosition = targetPosition;

I wanted to remove this very strong coupling which my components have (as a result of specifying exactly which component should receive the message), and transition towards an event / message system.

As a result, I've written an implementation which seems ideal, however it uses run-time dynamic typing which has me concerned that there may be a better way to achieve my goals.

My code is as follows, currently:

Inside Entity:

public void SendMessage<T>(T message) {
    _components.ForEach(x => (x as dynamic).Message(message));
}

Inside Abstract Component class:

public virtual void Message(IComponentMessage componentMessage) { }

Inside an example component (MovementComponent) that receives a move message:

public void Message(MoveMessage message) {
    Console.Write("Received message");
}

Dispatching a command to move:

private void IssueMovement(TileCoordinate targetPosition) {
    Console.Write("Dispatching move message");
    _entity.SendMessage(new MoveMessage(targetPosition));
}

Example Message class - MoveMessage:

public class MoveMessage : IComponentMessage {
    public readonly TileCoordinate MoveTo;

    public MoveMessage(TileCoordinate coordinates) {
        MoveTo = coordinates;
    }
}

NB: IComponentInterface is just an empty interface at the current moment in time.

The way I see it, my current approach has some big advantages:

Adding a new message type does not require any modification elsewhere to be supported.
Subscribing to an event is as easy as creating a method with the correct event class.
There is practically no overhead for listening to a specific event - no need to instantiate a class, inherit some event-specific interface, register interest at run-time with a subscribe method, etc.
As messages are represented by classes (like a MoveMessage), they can contain all the relevant data without needing to be cast or coerced.

However, as mentioned at the beginning of my question, I am concerned about using run-time dynamic typing in this fashion (See my dispatch code in Entity - the second snippet in this post). This system will probably be handling a lot of messages, and I have some concerns about the overhead incurred (it also feels like a code smell in general)

I would really appreciate feedback on this approach; particularly whether it seems like an abuse of dynamic to more experienced C# developers than I, but would also appreciate alternative suggestions.

Alternative approaches I've considered, and why I did not go with them initially:

Exhaustive set of event properties which components can subscribe to manually - adds extra code overhead to each message type, which I'd like to avoid
Dictionary from event Type to handler in each component - have to subscribe at run-time, which feels undesirable.
Switching dynamically inside a non-overloaded handler - more viable than it sounds since most components will not listen to more than a few message types, but still has unpleasant code overhead

I am quite sure this code is okay, outside of the overhead induced by the run-time typing - my apologies if this makes it a bad fit. I am just hoping for a once-over review by someone more experienced to reassure me that this is a good design.

You have one big problem with this design. It's easiest to explain the problem with the example.

So, let's say you add a new feature where you can issue a speak message. You'll need to add SpeakMessage and SpeakComponent classes. SpeakComponent would have public void Message(SpeakMessage message) method.

In this scenario, when someone calls your IssueMovement method, it will call SendMessage method which in turn will iterate over _components (which now contains MoveComponent and SpeakComponent) and try to call their Message method that has one argument of type MovementMessage. Since SpeakComponent doesn't have that method, it will throw an exception. The only way to avoid that with your current design is to have your component classes each handle every message in the system which is not what you would want to do.

So, to improve your design, your entity will need to know which component handles which message. The best way to do it, IMO, is to have an interface IMessageComponent<T> where T: IComponentMessage with a single method void Message(T componentMessage). MoveComponent would then implement IMessageComponent<MoveMessage> and SpeakComponent would implement IMessageComponent<SpeakMessage>.

Your SendMessage method inside the entity would then become:

public void SendMessage<T>(T message) {
  _components.OfType<IMessageComponent<T>>().ForEach(x => x.Message(message));
}

If you want one component to handle more than one message type (although, that would violate SRP), your component could implement multiple IMessageComponent<T> interfaces with different message classes.


Traversing an array with a pointer to the array I was thinking about how to traverse an array without an int iterator, and I'm curious if my answer is good/bad.
The first for loop simply populates each element of the array.
The second for loop traverses through the array. The int pointer, curr, initially points to the first address of the array then goes on to momentarily hold the address of each element of the array. The int pointer, ptrLastElement, contains the address of the last element of the array arr.
The check condition is essentially the difference of the address contained in ptrLastElement and the address contained in curr. If the difference is less than 0, then we've ran out of elements to display.
My main questions are:

Would this idea ever be useful?
Is this a terrible idea?
Did I do anything incredibly stupid?
How can I improve this and/or make it more generic?

main.cpp
#include <iostream>

int main()
{
    const size_t SIZE = 10;
    int arr[SIZE];
    int * ptrLastElement = arr + (SIZE-1);

    // Populate the array, arr, using int iterator
    for (int i = 0; i < SIZE; ++i)
    {
        arr[i] = i;
    }

    // Traversing arr using a pointer
    for (int * curr = arr; (ptrLastElement - curr) >= 0 ; ++curr)
    {
        std::cout << curr << "\t" << *curr << std::endl;
    }

    return 0;
}

It's not terrible as it stands, but the code could be improved.  Here are some ideas on how to do so.

Minimize work within the loop

If we redefine ptrLastElement to something like this:

const int *end = &arr[SIZE];

Since the pointer starts from the beginning of the array, the for loop could instead look like this:

for (int * curr = arr; curr != end; ++curr)

This matches the use of real iterators in the standard library by having the end of the array be one beyond the last element.

However, modern C++ rarely uses pointers, so I would probably neither use nor advise anyone else to use such a construct.  There's little benefit and the use of pointers can lead to difficult-to-find errors.

Don't use std::endl if '\n' will do

Using std::endl emits a \n and flushes the stream.  Unless you really need the stream flushed, you can improve the performance of the code by simply emitting '\n' instead of using the potentially more computationally costly std::endl.

Omit return 0

When a C or C++ program reaches the end of main the compiler will automatically generate code to return 0, so there is no need to put return 0; explicitly at the end of main.

Note: when I make this suggestion, it's almost invariably followed by one of two kinds of comments:  "I didn't know that." or "That's bad advice!"  My rationale is that it's safe and useful to rely on compiler behavior explicitly supported by the standard.  For C, since C99; see ISO/IEC 9899:1999 section 5.1.2.2.3:

  [...] a return from the initial call to the main function is equivalent to calling the exit function with the value returned by the main function as its argument; reaching the } that terminates the main function returns a value of 0.

For C++, since the first standard in 1998; see ISO/IEC 14882:1998 section 3.6.1:

  If control reaches the end of main without encountering a return statement, the effect is that of executing return 0;

All versions of both standards since then (C99 and C++98) have maintained the same idea.  We rely on automatically generated member functions in C++, and few people write explicit return; statements at the end of a void function.  Reasons against omitting seem to boil down to "it looks weird".  If, like me, you're curious about the rationale for the change to the C standard read this question.  Also note that in the early 1990s this was considered "sloppy practice" because it was undefined behavior (although widely supported) at the time.

So I advocate omitting it; others disagree (often vehemently!)  In any case, if you encounter code that omits it, you'll know that it's explicitly supported by the standard and you'll know what it means.


Counting words, letters, average word length, and letter frequency I'm pretty sure my code is mostly correct.  I think I'm having formatting errors more than anything. I keep receiving warnings about "double to float and int to float,possible loss of data.  Here is what I am trying to accomplish exactly.

  Write a function that accepts a pointer to a C-
  string as an argument and calculates the number of
  words contained in the string as well as the number of letters in the string. Communicate (or send) both of these
  values back to the main function, but DO NOT use global variables
  (variables defined outside of a function).

  Write another function that accepts the number of letters and the number of words and sends the
  average number of letters per word (or average word
  size) back to the main function.

  Demonstrate the functions in a program that asks the user to input a string.
  First, store the input
  in a large array. The program should dynamically allocate just enough memory to store the
  contents of that array.
  Copy the contents of the large array into the dynamically allocated
  memory. Then the program should pass that new, dynamically allocated array to the first
  function.
  Both the number of words and the average word size should be displayed on the
  screen. Round the
  average word size to 2 decimal places.

  For instance, if the string argument is "Four score and seven years ago" the first function (word
  count) should calculate and send back a word count of 6 and a letter count of 25. The second
  function (average word size) should send back 4.17, or 25 / 6.

  Extra challenge: See if you can prevent the program from counting punctuation (such as quotes
  or periods) as part of the sentence. Also, see if you can prevent the program from counting extra
  spaces as new words. For in
  stance, 2 spaces will often follow a colon, such as the sentence:
  "There are 3 primary colors
  :
  red, blue, and green."
  In this example, the word count should be 9 (the number
  3
  does count as a 1-letter word), and the letter count should be 37, for an average of 4.11

#include<iostream>
#include<string>

using namespace std;

int CountWordsAndLetters(char* str, int& words, int& letters)
{
    words = 0;
    unsigned int i = 0;
    letters = 0;

    while (str[i] == ' ')
            i++;

    for (; str[i]; i++)
    {
            if (((str[i] >= 'a') && (str[i] <= 'z')) || ((str[i] >= 'A') && (str[i] <= 'Z')))
                    letters++;
            if (str[i] == ' ')
            {
                    words++;
                    while (1)
                            if (str[i] == ' ')
                                    i++;
                            else
                            {
                                    i--;
                                    break;
                            }

            }

    }
    words = words + 1;
    return (words);
}

float avg(float words, float letters)
{
    float a = (double)(letters / words);
    return a;
}

int main()
{
    char array[1000000];
    int words = 0;
    int letters = 0;

    cout << "enter the string\n\n";
    gets(array);
    int size;
    for (size = 0; array[size]; size++);
    char* str = new char[size];
    strcpy(str, array);
    CountWordsAndLetters(str, words, letters);

    cout << "\nword count= " << words << endl;
    cout << "\n letter count=" << letters << endl;
    cout << "\naverage number of letters per word=" << avg(words, letters);

    return 1;
}

It's gonna be a small review, but do you know strtok ? As said on the link I provided :

  A sequence of calls to this function split str into tokens, which are sequences of contiguous characters separated by any of the characters that are part of delimiters.

So if you want to split this into words, just put the delimiters to space and punctuation, and you already have most of the job done.

An other thing :

float avg(float words, float letters)
{
    float a = (double)(letters / words);
    return a;
}

If you want a float, why do you explicitly cast it to a double ? That's why you have warnings.

Same goes for the typing of words and letters : can it take a negative value ? If not, why does your type allows it to do so, instead of using an unsigned type ?

And last, I think you are re-inventing strlen with this piece of code : for (size = 0; array[size]; size++);. And of course you should not, just use the function instead.


Making a catalog of pictures Is there a way to make this code run faster? I have over 6000 pictures in .jpg optimized to a maximum of 40 kB. It starts quickly but after like 2000 pictures it runs way slower. The final .xlsm file is about 380 MB.

I'm using Excel 2016 with Windows 10 on a big server with 16 processors and 80 GiB RAM.

Sub Button5_Click()
With Excel.Application
.Calculation = xlCalculationManual
.ScreenUpdating = False
.DisplayStatusBar = False
.EnableEvents = False
End With
With Worksheets
DisplayPageBreaks = False
End With
ChargeTrombinoscope
With Excel.Application
.EnableEvents = True
.DisplayStatusBar = True
.ScreenUpdating = True
.Calculation = xlCalculationAutomatic
End With
With Worksheets
DisplayPageBreaks = True
End With
''''CheckImageName
End Sub

Sub ChargeTrombinoscope()
Dim Chemin As String, Fichier As String
Dim nom, nom As String
Dim splitArr() As String
Dim Ligne As Integer
Dim Largeur As Integer
Dim Hauteur As Integer
Dim h As Long, Rapport As Single
Const hDefaut = 97

Worksheets("Pix").Activate

  'Définit le répertoire contenant les fichiers
Chemin = "C:\IMAGES\"

  'Boucle sur tous les fichiers du répertoire (photos).
Ligne = 3
Columns("K:K").ColumnWidth = 40  ' défini la largeur de la colonne
Columns("H:H").ClearContents
Columns("I:I").ClearContents
For Each Sh In ActiveSheet.Shapes
   If Sh.Type = msoPicture Then 'msoPicture Then
      Sh.Delete
   End If
Next Sh

Fichier = Dir(Chemin & "*")
Do While Len(Fichier) > 0
  'Extraction nom
  splitArr = Split(Fichier, ".")
  nom = splitArr(0)
  Range("H" & Ligne).Value = CStr(nom)
  Range("H" & Ligne).NumberFormat = "@"
  Range("I" & Ligne) = "a" & Range("H" & Ligne)     ''' pour corriger le bogue des noms numériques
  'insertion de la photo dans la colonne K
  Range("K" & Ligne).Select
''  Largeur = Range("K" & Ligne).Width
''  Hauteur = Range("K" & Ligne).Height

 ActiveCell.RowHeight = 99  ' ajuste la hauteur de la ligne : 1 point = 0,035 cm

h = hDefaut
h = h - 4

  ActiveSheet.Shapes.AddPicture(Chemin & Fichier, False, True, ActiveCell.Left, ActiveCell.Top, Largeur, Hauteur).Select

With Selection.ShapeRange
    Rapport = h / Selection.Height
    AjusterImage Selection, Rapport
   .Name = Range("I" & Ligne)
End With

  'Fichier suivant
  Fichier = Dir()
  Ligne = Ligne + 1
Loop
  Range("H3").Select

With Worksheets("Pix")
   DerLig = .Range("H" & Rows.Count).End(xlUp).Row
   ActiveWorkbook.Names("PicTable").Delete
   ActiveWorkbook.Names.Add Name:="PicTable", RefersTo:="=Pix!$H$2:$H$" & DerLig
End With

End Sub

Function AjusterImage(Imag As Object, Rapport As Single)
Dim Largeur As Single
Dim Hauteur As Single

Largeur = Imag.Width
Hauteur = Imag.Height
Largeur = Largeur * Rapport
Hauteur = Hauteur * Rapport
Imag.Width = Largeur
Imag.Height = Hauteur
End Function

Avoid Selecting of Activating Object unless absolutely necessary Excel VBA Introduction Part 5 - Selecting Cells (Range, Cells, Activecell, End, Offset).

Writing an array of values to a worksheet is much faster than writing the values individually.

The Worksheet.Pictures method returns a collection of pictures on the worksheet.
You can get a subset of pictures by passing an Array of Indices as a parameter to the Worksheet.Pictures method:

 ActiveSheet.Pictures(Array(1,2,30))

Or an Array of names:

 ActiveSheet.Pictures(Array("Picture 1","Picture 2","Picture 3"))

You can also work on the whole collection or subset at once:

 ActiveSheet.Pictures.Delete

Formatting a range of cells is much faster than formatting each cell in a range.

Columns("H:I").ClearContents
Columns("H").NumberFormat = "@"

Short compact code is much easier to read.

Which is easier to read?

h = hDefaut
h = h - 4

Or

 h = hDefaut - 4

Largeur = Imag.Width
Hauteur = Imag.Height
Largeur = Largeur * Rapport
Hauteur = Hauteur * Rapport
Imag.Width = Largeur
Imag.Height = Hauteur

Or

Largeur = Imag.Width * Rapport
Hauteur = Imag.Height * Rapport

With Worksheets("Pix")
   DerLig = .Range("H" & Rows.Count).End(xlUp).Row
   ActiveWorkbook.Names("PicTable").Delete
   ActiveWorkbook.Names.Add Name:="PicTable", RefersTo:="=Pix!$H$2:$H$" & DerLig
End With

The code above can be simplified:

With Worksheets("Pix")
   .Range("H2",.Range("H" & Rows.Count).End(xlUp)).Name = "PicTable"
End With

MS Access is better suited for this type of thing.  I personally would create a webpage.  Whether I used Excel or a webpage I would create a paginator and avoid embedding the images.

I refactored your code here: Making a catalog of pictures.xlsm.  Note: I didn't make the Rapport size adjustments but it should be easy enough for you to implement it.


Improved RandomGenerator with nextPrime() method I am trying to implement a method that generates a random prime number between a given range.

I have tried using a simple method to find whether a number is prime or not, advanced simple method, Wilson's method and I ended up with sieve of Eratosthenes as soon as my code based on this algorithm is faster than all of previous methods for finding if a number is prime or not.

Although the efficiency is satisfying, I know that there is an implementation that works without creating an array and with the code run even faster, but I don't know how to do this, so my question is obvious.

import acm.program.*;
import acm.util.RandomGenerator;

public class exercise4 extends Program {
public int nextPrime(int n){                                          //sieve of Eratosthenes
        final RandomGenerator rgen = RandomGenerator.getInstance();
        boolean[] array = new boolean[n];
        for (int i=2; i<n;i++) {
            array[i]=true;
        }
        int i=2;
        while (i<Math.sqrt(n)) {
            if (array[i]==true) {
                int m=i*i;
                int j=0;
                while (m+j*i<n) {
                    array[m+j*i]=false;
                    j++;
                };
            };
            i++;
        };
        int random;
        do {
            random=rgen.nextInt(n);
        } while(array[random]==false);
        return random;
    }
    public void run(){
        //long start = System.currentTimeMillis();
        println(nextPrime(2_000_000));
        //long elapsedTimeMillis = System.currentTimeMillis()-start;
        //float elapsedTimeSec = elapsedTimeMillis/1000F;
        //println("Completed in: "+elapsedTimeSec+" seconds.");
    }
}

I'll concentrate on the syntactical issues in this review since the others have already commented on the other parts of the code.

import acm.program.*;
import acm.util.RandomGenerator;

public class exercise4 extends Program {
public int nextPrime(int n){                                          //sieve of Eratosthenes
        final RandomGenerator rgen = RandomGenerator.getInstance();
        boolean[] array = new boolean[n];

Your code is difficult to read for an experienced Java programmer because you violate all kinds of conventions:

Class names start with an uppercase letter, yours (exercise4) doesn't.
After every opening {, the code is written one more level to the right. So the line that declares nextPrime should be more to the right. Ideally you let your IDE format the code for you. For Eclipse, press Ctrl+Shift+F, for IntelliJ press Ctrl+Alt+L, for Netbeans Alt+Shift+F.
rgen is a final variable, but array isn't. That's inconsistent. Either both or none should be final.

Continuing:

        for (int i=2; i<n;i++) {

You should leave some space around the operators, so write for (int i = 2; i < n; i++) {.

            array[i]=true;
        }
        int i=2;
        while (i<Math.sqrt(n)) {
            if (array[i]==true) {

You can leave out the == true.

                int m=i*i;
                int j=0;
                while (m+j*i<n) {

You should never call this code with an n that is close to Integer.MAX_VALUE. Otherwise the expression m + j * i will overflow and become negative, and something negative is surely smaller than a large n.

                    array[m+j*i]=false;
                    j++;
                };
            };
            i++;
        };
        int random;
        do {
            random=rgen.nextInt(n);
        } while(array[random]==false);
        return random;
    }
    public void run(){
        //long start = System.currentTimeMillis();
        println(nextPrime(2_000_000));
        //long elapsedTimeMillis = System.currentTimeMillis()-start;
        //float elapsedTimeSec = elapsedTimeMillis/1000F;
        //println("Completed in: "+elapsedTimeSec+" seconds.");
    }
}


Test-Spy Implementation I have tried to write a test-spy myself. Just for getting more familar with the topic.

Here's the code:

// --------- SPY - Start -----------------------------------
class Spy {
  constructor(func) {
    this.func = func;
    this.returnValue = null;
    this.result = null;
    this.countFuncCalled = 0;
  }

  invoke(...givenArgs) {
    this.receivedArgs = givenArgs;
    this.returnValue = this.func(...givenArgs);
    this.countFuncCalled++;
  }
}
// --------- SPY - End ------------------------------------

const calc = {
  add: (a, b) => {
    return a + b;
  },
  sub: (a, b) => {
    return a - b;
  }
}

const addSpy = new Spy(calc.sub);

addSpy.invoke(9, 4);
console.log(`Used arguments: ${addSpy.receivedArgs.join(", ")}`);
console.log(`Return value: ${addSpy.returnValue}`);
console.log(`Count of function-calls: ${addSpy.countFuncCalled}`);

addSpy.invoke(8, 7);
console.log(`Used arguments: ${addSpy.receivedArgs.join(", ")}`);
console.log(`Return value: ${addSpy.returnValue}`);
console.log(`Count of function-calls: ${addSpy.countFuncCalled}`);

What do you think about my implementation? Is is done in a basically correct way?

I'm I using the ES6-features (Classes, Rest, Spread) right?

What would you have done differently and why?

Looking forward to reading your comments and answers.

What do you think about my implementation? Is is done in a basically correct way?

Yeah who is to say what is "correct"? I mostly have worked with the chaiJS spies API and it feels like a basic version of that library.

  I'm I using the ES6-features (Classes, Rest, Spread) right?

While there isn't much to the code, I would say it looks like those features are used correctly. You could consider using more class features like getters and setters

In the constructor, there exists this line:

this.result = null;

but the result property doesn't appear to be used anywhere. What is that for?


Using argparse with parameters defined in config file I understand that and why using eval is generally considered bad practice in most cases (see e.g. here). Related questions on config files and argparse don't use types. (1, 2)

I needed to parse a number of command line arguments and decided to store the arguments, defaults and help strings in a .json file. I read the .json and add every argument to an argparse.ArgumentParser. To specify a type I need to pass a callable like float and not the string "float".

To get from the string to the callable I thought of

using eval
using a dict to map from string to callable
using a if/else or switch

and decided to use to use eval to avoid hard coding all types.

I have no security concerns because the argument file is supplied by the user and the program that uses this code is targeted at scientists that will realistically want to change the file to add parameters or change defaults. (Also, it is a university project which will never be run except for grading. I handed in a version using eval.)

Is there a smart solution avoiding hardcoding all types and avoiding eval, or did I find a place where eval is a sensible choice? I was only allowed to use the standard library.

Minimal args.json:

{
    "dt": {
        "type": "float",
        "help": "Time step size",
        "default": 0.4},
    "T": {
        "type": "int",
        "help": "Time steps between outputs",
        "default": 50},
    "f": {
        "type": "str",
        "help": "Landscape file name",
        "default": "small.dat"}
}

Runnable code, put args.json above in same directory to run:

import json
import argparse
import pprint

def setup_parser(arguments, title):

    parser = argparse.ArgumentParser(description=title,
                                     formatter_class=argparse.ArgumentDefaultsHelpFormatter)

    for key, val in arguments.items():
        parser.add_argument('-%s' % key,
                            type=eval(val["type"]),
                            help=val["help"],
                            default=val["default"])

    return parser

def read_params(parser):

    parameters = vars(parser.parse_args())
    return parameters

def get_parameters(title=None):

    with open("args.json") as data_file:
        data = json.load(data_file)
    parser = setup_parser(data, title)
    parameters = read_params(parser)

    return parameters

if __name__ == "__main__":
    params = get_parameters()
    pprint.pprint(params)

The use of a json file/expression as a source for defining argparse arguments is clever, and I think sound.  ipython defines many of its commandline arguments from values in a config file.

I commented that we need to be careful about what type does.  It's a frequent point of confusion.  It is a callable, usually a function, that takes a string, and converts it to the kind of value you want in the args namespace.  Its relation to builtin types and the the Python type function is only tangential.

int and float work because those functions take a string and return a desired value (or raise an error).  bool does not because it does not take string and return a True/False (try bool('False').

argparse has an undocumentated registries mechanism that maps from strings to callables.  Mostly it is used for the action parameter, but can also be used with type.  Basically it's a dictionary.

Here's a quick session to illustrate its use.

In [2]: parser=argparse.ArgumentParser()

parser.register is an undocumented method, used internally, but available to the user (no _):

In [3]: parser.register?
Type:       instancemethod
...
File:       /usr/local/lib/python2.7/argparse.py
Definition: parser.register(self, registry_name, value, object)
Docstring:  <no docstring>

It sets an entry in a dictionary that the parser owns.  Notice all the familiar action values.  This maps from those strings to the Action subclasses.  There is only one type entry, the default case that does nothing (leaves a string unchanged).

In [4]: parser._registries
Out[4]:
{'action': {None: argparse._StoreAction,
  'append': argparse._AppendAction,
  'append_const': argparse._AppendConstAction,
  'count': argparse._CountAction,
  'help': argparse._HelpAction,
  ...
 'type': {None: <function argparse.identity>}}

So let's add some entries to the register:

In [5]: parser.register('type','float',float)
In [6]: parser.register('type','int',int)
In [7]: parser.register('type','str',None)

and create some Actions to use them:

In [8]: parser.add_argument('-i',type='int')
....
In [9]: parser.add_argument('-f',type='float')
...
In [10]: parser.add_argument('-s',type='str')
...
ValueError: None is not callable

Oops, it didn't like my None.  I was intending the None to map on to the existing None mapping in the registries, but it does not do that kind of double mapping.

I think the str() function will work.  It takes a string and returns it unchanged.  The argparse identity is more like a do nothing lambda x:x.

In [12]: str('astirng')
Out[12]: 'astirng'

In [13]: parser.register('type','str',str)

In [14]: parser.add_argument('-s',type='str')
Out[14]: _StoreAction(option_strings=['-s'], dest='s', nargs=None, const=None, default=None, type='str', choices=None, help=None, metavar=None)

Now let's test:

In [15]: parser.parse_args('-i 1 -f 23 -s 23'.split())
Out[15]: Namespace(f=23.0, i=1, s='23')

If you don't want to use register, I'd recommend a dictionary over eval.  As another answer notes there aren't that many types, and you have more control over the translation.

A recent SO question suggests on potential difficulty with your json-argparse mapping.  Not all Actions take a type parameter.  Your set of parameters is fine for the defaultstoreaction.  But astore_trueaction raises an error if you give it atype` parameter.

https://stackoverflow.com/questions/33574270/typeerror-init-got-an-unexpected-keyword-argument-type-in-argparse

I'd prefer calling add_argument with:

parser.add_argument(*args, **kwargs)

and build up args as a list, and kwargs as a dictionary. For example instead of :

for key, val in arguments.items():
    parser.add_argument('-%s' % key,
                        type=eval(val["type"]),
                        help=val["help"],
                        default=val["default"])

use something like:

for key, val in arguments.items():
    args = ['-%s'%key]
    kwargs = {<some defaults>}
    kwargs.update(val)
    parser.add_argument(*args, **kwargs)

This would allow me to use json strings like

'foo': {'action': 'store_true',
           'help': 'True/False values'},
   'bar': {'type': 'int',
           'default': 2},
   'baz': {'nargs': '+',
           'help': 'multiple values'}

I haven't tested this yet, but if I've got it right, it would give more flexibility.  Internally argparse uses **kwargs a lot to handle the large numbers of parameters that its methods take.


Removing hackery from pair of radix functions I like the radix function and found a really neat way to compute its inverse in Python with a lambda and reduce() (so I really feel like I'm learning my cool functional programming stuff):

def unradix(source,radix):
        # such beauty
        return reduce(lambda (x,y),s: (x*radix,y+s*x), source, (1,0))

where:

>>> unradix([1,0,1,0,1,0,1],2)
(128,85)

But its dizygotic twin is unable to use the same pattern without an ugly hack:

def radix(source,radix):
        import math
        hack = 1+int(math.floor(math.log(source)/math.log(radix)))
        # ^^^^ ... _nearly_ , such beauty
        return reduce(lambda (x,y),s: (x/radix,y+[x%radix]), xrange(hack), (source,[]))

where:

>>> radix(85,2)
(0, [1, 0, 1, 0, 1, 0, 1])

How can I remove the hack from this pattern?  Or failing that, how can I rewrite this pair of functions and inverse so they use the same pattern, and only differ in "mirror-symmetries" as much as possibly?

By mirror symmetry I mean something like the way increment and decrement are mirror images, or like the juxtaposition of x*radix versus x/radix in the same code location as above.

The reverse operation to folding is unfolding.  To be honest, I'm not very
fluent in python and I couldn't find if python has an unfolding function.
(In Haskell we have
foldr
and its complement
unfoldr.)

Nevertheless, we can easily construct one:

"""
f accepts a value and returns None or a pair.
The first element of the pair is the next seed,
the second element is the value to yield.
"""
def unfold(f, r):
    while True:
        t = f(r);
        if t:
            yield t[1];
            r = t[0];
        else:
            break;

It iterates a given function on a seed until it returns None, yielding intermediate results. It's dual to reduce, which takes a function and an iterable and produces a value. On the other hand, unfold takes a value and a generating function, and produces a generator. For example, to create a list of numbers from 0 to 10 we could write:

print list(unfold(lambda n: (n+1, n) if n <= 10 else None, 0));

(we call list to convert a generator into a list).

Now we can implement radix quite nicely:

def radix(source, radix):
    return unfold(lambda n: divmod(n, radix) if n != 0 else None, source);

print list(radix(12, 2));

prints [0, 0, 1, 1].

Edit: Folding (reduce) captures the pattern of consuming lists. Any function that sequentially consumes a list can be eventually written using reduce. The first argument to reduce represents the core computation and the third argument, the accumulator, the state of the loop. Similarly, unfolding (my unfold) represents the pattern of producing lists. Again, any function that sequentially produces a list can be eventually rewritten using unfold.

Note that there are functions that can be expressed using either of them, as we can look at them as functions that produce or consume lists. For example, we can implement map (disregarding any efficiency issues) as

def map1(f, xs):
    return reduce(lambda rs, x: rs + [f(x)], xs, [])

def map2(f, xs):
    return unfold(lambda xs: (xs[1:], f(xs[0])) if xs else None, xs)

The duality of reduce and unfold is nicely manifested in a technique called deforestation. If you know that you create a list using unfold only to consume it immediately with reduce, you can combine these two operations into a single, higher-order function that doesn't build the intermediate list:

def deforest(foldf, init, unfoldf, seed):
    while True:
        t = unfoldf(seed);
        if t:
            init = foldf(t[1], init);
            seed = t[0];
        else:
            return init;

For example, if we wanted to compute the sum of digits of 127 in base 2, we could call

print deforest(lambda x,y: x + y, 0,  # folding part
               lambda n: divmod(n, 2) if n != 0 else None, # unfolding
               127); # the seed


has extra blank 's and I have no idea why. please help me get them out This is the result of the code it has extra option tags no idea why.

I have no idea why It produces the extra option box.

Below is the result of the query. Correct number of rows correct response...

     ParentID   Parent
    1   Pipe
    2   Valve
    3   Control Valve
    4   Pump
    5   Chamber
    6   Meter
    7   Check
    8   Distribution Pipe

This is some php that makes the dropdown:

$qry = "select distinct ParentID, Parent from tbl
    where ChildID = '0' and Parent is not null";

$parentRslt = mysql_query($qry);

$count = mysql_num_rows($parentRslt);

$opt7 = null;

for($m=0;$m<$count;$m++)
    {
        $var = mysql_result($parentRslt,$m,'ParentID');
        $name = mysql_result($parentRslt,$m,'Parent');

        $var = trim($var);
        $name = trim($name);

        $opt7.="<option value='$var'>$m $name<option>";
    }

$form = "
    <form method='post'>

    Network Action: <input type='text' name='child' />
    <select name='parentID'>
            $opt7
    </select>

    <input type='submit' name='submit' value='submit' /></form>";

return $form;

You should close the option tag. Change

$opt7.="<option value='$var'>$m $name<option>";

to

$opt7.="<option value='$var'>$m $name</option>";


Simple backup script I haven't done much Python in the past, so I was wondering if you could look over my code and make sure everything is up to snuff?

I know programs that do this exist already, but I am having a challenge with my friend on who can build a simple backup system for our college files.

I have run the code through Pylint and a PEP8 code checker, and fixed the errors it produced.

"""
Basic Python Backup
Nat Zimmermann
"""

# SETTINGS
TIME_FORMAT = '%d-%m-%Y-%H:%M:%S'
MAX_DIRS = 5

import shutil
from datetime import datetime
from os import walk

def get_dirs(dirs):
    """Get all immidiate directories in the current directory"""

    converted_dirs = []

    if len(dirs) > MAX_DIRS:
        test_dir = None

        for test_dir in dirs:
            try:
                app_dir = datetime.strptime(test_dir, TIME_FORMAT)
                converted_dirs.append(app_dir)

            except ValueError:
                print("Directory %s does not match date format, ignoring..."
                      % test_dir)

    return converted_dirs

def remove_dirs():
    """Remove the oldest directories"""

    dirs = walk('.').__next__()[1]
    converted_dirs = get_dirs(dirs)

    if converted_dirs:
        try:
            oldest_dir = datetime.strftime(min(converted_dirs), TIME_FORMAT)
            shutil.rmtree(oldest_dir)

            print("Removed old directory %s" % oldest_dir)

            remove_dirs()

        except IOError as error:
            print(error)

def copy_directory():
    """Copiy a given directory to a new directory with the current date"""

    old_directory = input("Directory to copy: ")
    new_directory = datetime.now().strftime(TIME_FORMAT)

    print("Copying...")

    try:
        shutil.copytree(old_directory, new_directory)
        print("Successfully copied files to directory %s" % new_directory)

    except IOError as error:
        print(error)
        copy_directory()

copy_directory()
remove_dirs()

Add following header for *nix users

#!/usr/bin/env python
# ~*~ codding: utf-8 ~*~

Not required variable definition

test_dir = None

Because of loop

for test_dir in dirs:

Use native generator insted

def get_dirs(dirs):
    """Get all immidiate directories in the current directory"""
    if not len(dirs) > MAX_DIRS:
        return []

    for test_dir in dirs:
        try:
            yield datetime.strptime(test_dir, TIME_FORMAT)
        except ValueError:
            print("Directory %s does not match date format, ignoring..."
                  % test_dir)

If you will use it like module, add

if __name__ == '__main__':
    copy_directory()
    remove_dirs()

IMHO: don't use direct input with input(), try parse command line instead using sys.argv or with help of some more powerful libraries.


Implementation of the Access version DBLookup() in VB6 I've tried to implement the access function DBLookup() in VB6 for a project I'm doing. Does anyone have comments on how well done it is and what could be improved? Take note that the Database connection object cn is only valid for a single query, and it's being initialized whenever you call the other public function, dbConn.

   Public Function cDLookup(TargetField As String, TargetTable As String, cTCondition As String) As String
    'Eigene Implementation von DLookup
    Dim Result As String
    Dim rs As New ADODB.Recordset
    Dim SQL As String
    On Error GoTo Fehlerbehandlung

'Zusammenbauen der Query
SQL = "SELECT " & TargetField & " FROM " & TargetTable & " WHERE " & cTCondition

Call dbConn
'Initiate Database connection object cn

rs.Open SQL, cn
If (rs.RecordCount = 1) Then
Result = cleanString(rs.GetString)
Debug.Print ("[DLOOKUP] Erfolgreich Einen Datensatz gefunden und konvertiert. Output: " & Result)

ElseIf (rs.RecordCount > 1) Then
Result = "#ErrRC"
Debug.Print ("[DLOOKUP] Es wurden " & CStr(rs.RecordCount) & " Datensätze statt einem festgestellt. Dies ist nicht erlaubt")

Else
Result = "#ErrGen"
Debug.Print ("[DLOOKUP] Es ist ein Fehler in der Abfrage aufgetreten")
End If
rs.Close

cDLookup = Result
Exit Function
Fehlerbehandlung:
Debug.Print ("[DLOOKUP] Fehler im Ausführen der Prozedur cDLookup()]")
cDLookup = "#Fehler"
Exit Function
End Function

In addition to @RubberDuck's excellent input (which I completely agree with), I have the following concerns:
Global Connection

Take note that the Database connection object cn is only valid for a single query

Nope, that's not how ADODB.Connection works. The reality here is that you have a function that consumes a connection that it doesn't own, encapsulated by an object that is global. Why isn't dbConn a function that returns the initialized connection? And if it initializes a connection, then why isn't it called InitializeConnection or GetConnection? And if cn is "only valid for a signle query", then why would it ever need to be visible to anything other than the procedure that needs an opened connection?
You're closing the recordset, but not the connection: if dbConn starts by closing the previous connection before opening a new one, this means you're leaving a database connection opened for much, much longer than you actually need to, and that's dirty. If dbConn doesn't do that, then your connection object simply falls in limbo, and that's outright wrong.

Call

Call dbConn
'Initiate Database connection object cn

That's an obsolete statement. This instruction is completely equivalent:
dbConn
'Initiate Database connection object cn

It doesn't look like a procedure call, only because of the poor naming of the dbconn procedure: if the name started with a verb...
InitializeConnection
'Initiate Database connection object cn

Why is that comment not on top of the instruction it's commenting?
'Initiate Database connection object cn
InitializeConnection

Now, if cn was local as it should be, that comment wouldn't even be needed, as the code would speak for itself:
Dim cn As ADODB.Connection
Set cn = InitializeConnection

As New
This is bad:

Dim rs As New ADODB.Recordset

Why? You might think that this:
Dim foo As New Collection

Is exactly the same as that:
Dim foo As Collection
Set foo = New Collection

But it will bite you. What does this code do?
Public Sub TestMe()
    Dim foo As New Collection
    foo.Add "bar"
    Set foo = Nothing
    foo.Add "surprise!"
End Sub

If you were expecting a Runtime error 91 - Object reference not set, you've been bitten. This blows up as expected:
Public Sub TestMe()
    Dim foo As Collection
    Set foo = New Collection
    foo.Add "bar"
    Set foo = Nothing
    foo.Add "surprise!" 'error 91
End Sub

Instead of running the query off the rs object with rs.Open, get a recordset instance from the connection with cn.Execute instead:
Set rs = cn.Execute(SQL)

Magic Return Values
Your function has several ways of "failing". A number of them involve an actual runtime error that the client code must handle. And if my German is correct, 3 of them involve returning a valid string that the client code must check (or treat as valid, and propagate to the UI?). Don't use your return value for that, raise proper errors: the client code should already be handling runtime errors, all you need to do is come up with a way of telling it exactly what went wrong.
An enum works great for that:
Public Enum DLookupError
    ErrNoRecordsReturned = vbObjectError + 42
    ErrMultipleRecordsReturned
End Enum

Now you can call Err.Raise DLookupError.ErrNoRecordsReturned, "DLookup", "Es ist ein Fehler in der Abfrage aufgetreten." to raise a runtime error that can be handled as such, effectively only ever returning an actual string result to your client code when there's such a result to return.
I left out the general "failure" error, since that's simply an error that you should bubble up to the caller.

Responsibilities
Code that's sprinkled with rs.Open calls all over the place, is code with poorly separated concerns. If your application needs to access a database, then you need an object whose responsibility is do to exactly that, so that the rest of the code can work at a higher abstraction level and not be bothered with connections and recordsets.
I've done exactly that in a previous life, and put everything up right here on Code Review, so you can see if you want something similar:

Materializing any ADODB Query
Creating ADODB Parameters on the fly

The entire function could be pretty much replaced with this (error-handling omitted):
Dim sql As String
sql = "SELECT " & fieldName & " FROM " & tableName & " WHERE " & predicate

Dim result As String
result = SqlCommand.QuickSelectSingleValue(sql)

Some other considerations

Parameters are implicitly passed ByRef; they can/should be passed ByVal instead.
Prefer camelCase for locals (I know VB6 isn't case-sensitive and that's a royal PITA at times); I'd go with fieldName, tableName and predicate.
You're not validating any of the parameters; an empty string is a valid condition resulting in invalid SQL. Granted that's handled by the error-handling there, but you could fail early and avoid sending malformed SQL to the database altogether.


Polynomial equation solver in Ruby I've written a small function for solving simple quadratic equations:

class EquationSolver
  def solve(x, *args)
    args.reverse.map.with_index { |coefficient, index| coefficient * x ** index }.reduce { |result, element| result + element }
  end
end

To calculate  f(3) for f(x)=3x3−2x2−x+5, one would write:

puts EquationSolver.new.solve(3, 3, -2, -1, 5)

However, is there a more elegant version of my function, more like reduce.with_index or something similar?

Yes, there is a more elegant version. Here it is:

class EquationSolver
  def solve(x, *args)
    args.reverse.each_with_index.reduce(0) { |result, (coefficient, index)| result + coefficient * x ** index }
  end
end

It's possible to do this because iterator methods like each_with_index, when called without a block, return an Enumerator object on which you can call all the methods of the Enumerable.

In fact, an even shorter solution is obtained by using a variation of reduce to use a symbolic operator:

class EquationSolver
  def solve(x, *args)
    args.reverse.map.with_index { |coefficient, index| coefficient * x ** index }.inject(:+)
  end
end


Rails initializer to be more clean I need a little bit of refactor my initialize method because I think it will make initializer more flexible and readable.

class LogAdminData
  DEFAULT_EXCLUDED_PARAMS = %w[
    encrypted_password
    reset_password_token
    reset_password_sent_at
  ].freeze

  def initialize(admin_obj:, action_type:, old_data:, new_data:, excluded_params: %w[])
    excluded_params += DEFAULT_EXCLUDED_PARAMS
    @old_data = old_data.reject { |k, _v| excluded_params.include? k }
    @new_data = new_data.reject { |k, _v| excluded_params.include? k }
    @action_type = action_type
    @admin_email = admin_obj.email
    @admin_role = admin_obj.role
  end

  def call
    AdminPanelLog.create(
      admin_email: admin_email,
      admin_role: admin_role,
      action_type: action_type,
      new_data: new_data,
      old_data: old_data,
    )
  end

Maybe something like

  def cleanup_data(data)
    excluded_params += DEFAULT_EXCLUDED_PARAMS
    data.reject { |k, _v| excluded_params.include? k }
  end

But how to call it in initializer using old_data and new_data?

If this is a Rails project (or you use ActiveSupport) you can replace

@old_data = old_data.reject { |k, _v| excluded_params.include? k }
with
@old_data = old_data.except(excluded_params)