Trie SPOJ Problem - PhoneList TLE In this problem you are given input of phone numbers and you are supposed to determine whether any number is the prefix of another. I have an addchild method that returns true or false if a given string can be inserted and is not a prefix of another string. How could I improve the efficiency of my code so that I could pass the time constraints?

http://www.spoj.com/problems/PHONELST/

import java.util.Arrays;
import java.util.Scanner;

class PhoneList {

static class Node{
    Node [] children;

    int value;

    public Node(){
        children=new Node[11];
    }

    public Node(int value){
        children=new Node[11];
        this.value=value;
    }

}

static class  Trie{
    Node root=null;

    public Trie(){
        root=new Node();
    }

    boolean addChild(String s){

        Node currentNode=root;
        int number=0;

        for(int i=0;i<s.length();i++){
            number=s.charAt(i)-'0';
            if(currentNode.children[currentNode.children.length-1]!=null){
                return false;
            }
            if(currentNode.children[number]==null){
                currentNode.children[number]=new Node(number);
            }
            currentNode=currentNode.children[number];
        }

        if(currentNode.children[currentNode.children.length-1]==null){
            currentNode.children[currentNode.children.length-1]=new Node(10);
        }else{
            return false;
        }

        return true;
    }

}

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

    int number_of_test_cases=input.nextInt();

    boolean istrue;
    for(int i=0;i<number_of_test_cases;i++){

        Trie tree=new Trie();
        int length=input.nextInt();
        String numbers[]=new String[length];

        for(int j=0;j<numbers.length;j++){
            numbers[j]=input.next();
        }

        Arrays.sort(numbers);
        istrue=true;

       for(int k=0;k<numbers.length;k++){
        if(!tree.addChild(numbers[k])){
            System.out.println("NO");
            istrue=false;
            break;
        }
        }

        if(istrue){
            System.out.println("YES");
        }
    }

}

}

Instead of children = new Node[11], I would use a Map<Integer, Node>.  I'm not sure how you came up with 11.
Sorting is quite expensive.  Maybe you can solve it with a trie alone without sorting, or with sorting alone without a trie.
Small detail: you have random gaps of 2, 3 or 4 empty lines all over the place.

EDIT:
So, instead of arguing, here is my implementation, without sorting and without a terminal value:

    public static class Node<T> {
        private T value;
        private Map<T, Node<T>> children = new HashMap<>();

        public Node(T value) {
            this.value = value;
        }

        public Node<T> getChild(T t) {
            return children.get(t);
        }

        public boolean hasChildren() {
            return !children.isEmpty();
        }

        public void addChild(T t, Node<T> child) {
            children.put(t, child);
        }
    }

    public static boolean hasConflictingPrefixes(String[] numbers) {
        Node<Byte> trie = new Node<>(null);
        for (String number : numbers) {
            Node<Byte> currentNode = trie;
            for (Byte byt : number.getBytes(Charset.forName("UTF-8"))) {
                Node<Byte> childNode = currentNode.getChild(byt);
                if (childNode == null) {
                    childNode = new Node(byt);
                    currentNode.addChild(byt, childNode);
                } else {
                    // current number is longer (or equal) to existing word
                    if (!childNode.hasChildren())
                        return true;
                }
                currentNode = childNode;
            }
            // current number is a prefix to another one
            if (currentNode.hasChildren())
                return true;
        }
        return false;
    }

I'm not necessarily proud of this code, but it solves this problem.

Note that duplicate numbers are considered as conflicting prefixes, ie. {"911", "911"} has a conflict just as {"911", "9112"}.


Inheritance: declare a method that returns the current instance class I have a Vector and 2-dimensional Vector, defined as follows:

    // A general vector
    class Vector {
        values: number[];

        constructor (...values: number[]) {
            this.values = values;
        }

        opposite(): Vector {
            return new Vector(...this.values.map((value, index) => -value));
        }
    }

    // Two-dimensional vector
    class Vector2 extends Vector {
        constructor(x: number, y: number) {
            super(x, y);
        }

        get x() {
            return this.values[0];
        }

        get y() {
            return this.values[1];
        }
    }

And I have the following code:

    let v = new Vector2(1, 2);
    let w = v.opposite();

    // this is wrong, since w is not a 2-dimensional vector
    console.log(w.x);

How could I solve this inheritance problem elegantly? For me, there are two alternatives:

Override the "opposite" method. I think that this is not a good idea, since currently there is only one method, but there could be a lot of them.

// Two-dimensional vector
class Vector2 extends Vector {
    // ... more methods ... blah blah blah ...
    // overrides the super class method
    opposite(): Vector2 {
        let [x, y] = super.opposite().values;
        return new Vector2(x, y);
    }
}

Create a named constructor. I think this is a good solution, but a bit tedious:

// Two-dimensional vector
class Vector2 extends Vector {
    constructor(x: number, y: number) {
        super(x, y);
    }

    // declare a "named constructor"
    static createFromVector(v: Vector): Vector2 {
        let [x, y] = v.values;
        return new Vector2(x, y);
    }

    // ... more methods ... blah blah blah ...
}

let v = new Vector2(1, 2);
// It's good, but a bit tedious writing constantly "createFromVector"
let w = Vector2.createFromVector(v.opposite());

How do you address this problem?

let v = new Vector2(1, 2);
let w = v.opposite();
// this is wrong, since w is not a 2-dimensional vector
console.log(w.x);

The fact that the .opposite() returns a result that does not fit into the expected use cases, suggests that there is a design issue. In other words, it is a "hint" that Vector2 is not really a Vector in terms of OOP. Why so? Because it does not follow the intuitive contract in which the .opposite() method returns same exact type of the vector as the one on which the method is being invoked. Things will only get worse if one will need to work against a Vector3, Vector4, etc.

There's whole bunch of sources that explain how inheritance is very often not the best way to create new types nor reuse code.

If I was to design the VectorOf2, I would make it wrap the Vector class rather than inherit from it.

class VectorOf2 {
    private _vector: Vector;

    constructor(x: number, y: number) {
        this._vector = new Vector(x, y);
    }

    get x(): number {
        return this._vector[0];
    }

    get y(): number {
        return this._vector[1];
    }

    opposite(): VectorOf2 {
        return new VectorOf2( ... );
    }
}

With this design, each method added to Vector2 is always Vector2-specific and there's no confusion about it. The regular Vector's method can still be reused by VectorOf2 when a new method is implemented.


Generating prime candidates I'd like to generate an infinite list of prime candidates of the form 6k±1, but I'm looking for the fastest possible solution.

Currently I have this:

primeCands :: [Integer]
primeCands = concatMap (\i -> [6*i-1, 6*i+1]) [1..]

but I feel that concat is wasting too much time. Using all the odd numbers is almost the same thing in terms of performance. Maybe a list is not the right datatype for this task?

Haskell has a great package for testing performance called Criterion. When you're trying to compare the performance between variations of the same function, head right for it.

Here's a small example with a few other versions of primeCands I dreamed up.

module Main where

import Criterion
import Criterion.Main

import Data.Ratio

primeCands_cmap :: [Integer]
primeCands_cmap = concatMap (\i -> [6*i-1, 6*i+1]) [1..]

dropThirds :: [a] -> [a]
dropThirds (a:b:_:ds) = a : b : dropThirds ds
dropThirds xs         = xs

primeCands_dropOdds :: [Integer]
primeCands_dropOdds = dropThirds [5,7..]

interleave :: [a] -> [a] -> [a]
interleave (a:as) (b:bs) = a : b : interleave as bs
interleave as     bs     = as ++ bs

primeCands_interleave :: [Integer]
primeCands_interleave = interleave [5,11..] [7,13..]

primeCands_allOdds :: [Integer]
primeCands_allOdds = [5,7..]

main :: IO ()
main = defaultMain [
         bgroup "prime candidates"
                   [ bench "concatMap"  $ nf distantElement primeCands_cmap
                   , bench "dropThirds" $ nf distantElement primeCands_dropOdds
                   , bench "interleave" $ nf distantElement primeCands_interleave
                   ]
       , bgroup "odds" [ bench "odds" $ nf furtherElement primeCands_allOdds ]
                   ]
  where
    index = 1000000
    distantElement = (!! index)
    furtherElement = (!! (ceiling $ (fromIntegral index) * (4 % 3)))

And the results of the criterion benchmark (compiling with -O2)—

benchmarking prime candidates/concatMap
time                 6.918 ms   (6.631 ms .. 7.160 ms)
                     0.995 R²   (0.992 R² .. 0.999 R²)
mean                 6.668 ms   (6.615 ms .. 6.753 ms)
std dev              193.5 μs   (107.1 μs .. 297.1 μs)
variance introduced by outliers: 10% (moderately inflated)

benchmarking prime candidates/dropThirds
time                 4.548 ms   (4.463 ms .. 4.639 ms)
                     0.996 R²   (0.993 R² .. 0.998 R²)
mean                 4.488 ms   (4.430 ms .. 4.546 ms)
std dev              183.5 μs   (153.8 μs .. 216.7 μs)
variance introduced by outliers: 22% (moderately inflated)

benchmarking prime candidates/interleave
time                 4.100 ms   (4.030 ms .. 4.172 ms)
                     0.996 R²   (0.993 R² .. 0.998 R²)
mean                 4.484 ms   (4.399 ms .. 4.558 ms)
std dev              241.0 μs   (215.5 μs .. 271.9 μs)
variance introduced by outliers: 31% (moderately inflated)

benchmarking odds/odds
time                 5.529 ms   (5.500 ms .. 5.568 ms)
                     1.000 R²   (1.000 R² .. 1.000 R²)
mean                 5.521 ms   (5.506 ms .. 5.546 ms)
std dev              54.27 μs   (34.21 μs .. 93.68 μs)

In this case you can see that the concatMap version runs a bit slower (about ⅓) than the versions that don't flatten lists. I question whether you are prematurely optimizing however, I would imagine that the slight difference in prime candidate generation timings are dwarfed by the actual prime checking function you've written.


A heap queue in Dyalog APL I am learning Dyalog APL. I implemented a binary heap, which seems to work. How could I make it look more like APL (and less like Python)?

⎕io←0
heappush←{(⍺,⍵)siftdown 0(≢⍺)}

heappop←{
 heap←⍵
 last←⊃¯1↑heap ⋄ rest←¯1↓heap
 0=≢rest:rest last
 r←heap[0]
 (((last@0)rest)siftup 0)r
}

siftdown←{
 heap←⍺
 start pos←⍵
 item←pos⌷heap
 newpos←{
  ⍵≤start:⍵
  parentpos←⌊(⍵-1)÷2
  parent←parentpos⌷heap
  item<parent:∇ parentpos⊣heap[⍵]←parent
  ⍵
 }pos
 (item@newpos)heap
}

siftup←{
  heap←⍺
  p←{
   pos←⍵
   chp←1+2×pos
   chp≥≢heap:pos
   rpos←1+chp
   chp←((rpos<≢heap)∧~heap[chp]<heap[rpos])⊃chp rpos
   heap[pos]←heap[chp]
   ∇ chp
  }⍵
  heap[p]←⍺[⍵]
  heap siftdown ⍵ p
 }

heap←0 1 2 5 6 8 9
heappop heap

┌→────────────────┐
│ ┌→──────────┐   │
│ │1 5 2 9 6 8│ 0 │
│ └~──────────┘   │
└∊────────────────┘

heap heappush 3
┌→──────────────┐
│0 1 2 3 6 8 9 5│
└~──────────────┘

How could I make it look more like APL (and less like Python)?

Assuming you mean "in a more functional and less imperative way" by this line,

I don't think you can largely achieve that, for a good reason.

Basically, the array-based heap (and other common algorithms you see on algorithm textbooks) is designed for imperative languages. Translating it into a language whose main strength isn't imperative makes the code feel awkward and unfitting. It may also lead to code whose time complexity is actually worse than designed. See how it looks like when a similar algorithm is written in Haskell.

APL is not 100% functional, but definitely is more functional than imperative (especially when you mainly use dfns). If you want, search for "functional algorithms" and try implementing those. In the case of a heap, leftist tree isn't too complex, and supports one more \$O(\log n)\$ operation (heap merge) compared to an imperative binary heap. You can check out a nice illustration too.

But you can still improve some parts of the code.

Improvement in the algorithm

Use ⎕IO←1 instead.

Array-based heap uses 0-based indexing by default, so the parent-child relationship is slightly awkward:

$$
\begin{align}
\text{left child}&=1+2\times\text{parent} \\
\text{right child}&=2+2\times\text{parent} \\
\text{parent}&=\Bigl\lfloor \frac{\text{child} - 1}2 \Bigr\rfloor
\end{align}
$$

If you use 1-based indexing instead, it becomes slightly cleaner:

$$
\begin{align}
\text{left child}&=2\times\text{parent} \\
\text{right child}&=1+2\times\text{parent} \\
\text{parent}&=\Bigl\lfloor \frac{\text{child}}2 \Bigr\rfloor
\end{align}
$$

I don't have other better ideas to utilize the strengths of APL, due to the underlying algorithm being purely imperative.

General tips for writing APL code

Let the right argument of dyadic functions be the primary one (i.e. the heap).
If you see a negation of a comparison (e.g. ~heap[chp]<heap[rpos]), use a single equivalent function (e.g. heap[chp]≥heap[rpos]).
Prefer concatenation (e.g. 0,≢⍺) over stranding (e.g. 0(≢⍺)) when you concatenate two scalars.
Try not to modify existing variable's contents (e.g. avoid chp←((rpos<≢heap)∧~heap[chp]<heap[rpos])⊃chp rpos which refers to chp and then modifies it) when it isn't essential in implementing the algorithm. Try to choose a separate and meaningful name instead.
Parenthesize stranding assignments (e.g. (start pos)←⍵ instead of start pos←⍵).
Consider following a naming convention, and a little more descriptive names. (e.g. I can't easily see what chp stands for.)
Consider adding comments to each function which briefly describe the input(s) and the output.


Express server for an artists website I am currently creating a website for an artist (my grandfather). My main concern with the code below is readability, as for the most part I have not worked with others on a (programming) project. I use a few external modules but hopefully their purpose is obvious/irrelevant.

I have been programming with javascript as a hobby for about two years.

Here is my code:

const express = require('express')
const app = express()
const fs = require('fs')
const gzip = require('express-gzip')
const handleRequest = require('./handleRequest')
const verify = require('../Admin/login').verify

app.use(gzip)
app.use(express.json())

app.get(['/bio', '/contact', '/login', '/works', '/404', '/bug', '/edit-bio', '/edit-works'], (req, res) => verify(req.query.token).then(result => fs.readFile((req.path === '/edit-bio' || req.path === '/edit-works' ? result : true) ? ((result && req.path === '/login') ? '../../Frontend/html/edit-works.html' : `../../Frontend/html${req.path}.html`) : '../../Frontend/html/login.html', (err, data) => err ? res.status(500).end() : res.end(data))))
app.get('/', (_req, res) => fs.readFile('../../Frontend/index.html', (err, data) => err ? res.status(500).end() : res.end(data)))
app.post(/.+/, (req, res) => handleRequest[req.body.type](req.body).then(result => result.err ? res.status(500).end(JSON.stringify(result.data)) : res.end(JSON.stringify(result.data))))

app.use('/html', (_req, res) => res.status(404).redirect('/404'))
app.use(express.static('../../Frontend'))
app.use((_req, res) => (res.status(404).redirect('/404'))).listen(8080)

This is a simple express server that answers get requests on: '/bio', '/contact', '/login', etc. If the request is to '/edit-bio' or '/edit-works' it verifies a jwt provided in the query string. If it receives a post request of any kind it runs a function corresponding to a type property in that request. It also has a simple 404 page. Finally it uses express.static on a Frontend folder containing the css, js, html, etc.

Some suggestions:

Rewrite the line of code app.get(['/bio', '/contact', '/login', '/works', '/404', '/bug', '/edit-bio', '/edit-works'] ... to be readable and maintainable and debuggable.  It should likely be broken into at least two separate routes.
Separate into different route definitions the paths that care about calling verify() and the ones that don't.
Use express.static for routes that don't appear to have any special processing such as ['/bio', '/contact', '/login', '/works', '/404', '/bug'].
Use res.sendFile() instead of repeating this err ? res.status(500).end() : res.end(data) over and over again.
Put your verify logic into a middleware format so it can easily be attached to any route.


Class that defines a person I have been programming for a while, but I've only made games and less serious stuff. I want to get more serious. I would like some feedback on this class that defines a person. Structure, readability, use of static variables, etc. Please point out everything that can be improved.

public class Person {

    private String firstName, lastName, address;
    private int age;
    private boolean hasAddress = false;
    private boolean hasAge = false;
    private static final String NO_ADDRESS = "No address defined";

    //Constructors

    //Minimum information required
    public Person(String firstName, String lastName) {
        this.firstName = firstName;
        this.lastName = lastName;
    }

    //All possible info
    public Person(String firstName, String lastName, int age, String address) {
        this(firstName, lastName);
        this.age = age;
        this.address = address;
        hasAge = true;
        hasAddress = true;
    }

    //No address defined
    public Person(String firstName, String lastName, int age) {
        this(firstName, lastName);
        this.age = age;
        hasAge = true;
    }

    //No age defined
    public Person(String firstName, String lastName, String address) {
        this(firstName, lastName);
        this.address = address;
        hasAddress = true;
    }

    //Methods

    //Get-methods
    public String getName() {
        return firstName + lastName;
    }

    public String getFirstName() {
        return firstName;
    }

    public String getLastName() {
        return lastName;
    }

    public int getAge() {
        if (hasAge)
            return age;
        else
            return 0; //Return 0 if no age is defined
    }

    public String getAddress() {
        if (hasAddress)
            return address;
        else
            return NO_ADDRESS;
    }

    //Set-methods
    public void setFirstName(String firstName) {
        this.firstName = firstName;
    }

    public void setLastName(String lastName) {
        this.lastName = lastName;
    }

    public void setAge(int age) {
        this.age = age;
    }

    public void setAddress(String address) {
        this.address = address;
    }

}

Design

The problem with getName is already mentioned in the comments. firstName + lastName is at least missing a space. Note that there are multiple ways of creating a full name - you may want to leave the construction of a full name to the caller. For instance, Germans are a bit more formal and may prefer "lastname, firstname". Usually initials are also an important part of a name (and then there is title, junior/senior etc.).

Second is the age. An age is a property of a person, but it is not one that keeps constant in time. You generally store the birthday of a person. Usually the precise day is stored (YYYY-MM-DD), or in more complex systems just the month (YYYY-MM) or even just the year (YYYY) if the other information is unknown. This may seem strange, but not everybody knows their exact birthday. The age can then be calculated from that using comparison with the current date.

Related, using 0 to signify that no age is known is choosing a bad magic value (or sentinel). Magic values are already not recommended, but choosing a valid age for a baby as magic value is not a good idea at all.

Returning return NO_ADDRESS; is dangerous, especially if you don't have any method to indicate that NO_ADDRESS is indeed not an address. It is generally up to the caller to choose what to insert as string if no address is known, otherwise you may get packages posted to NO_ADDRESS. A hasAddress method would solve this. If you have an unknown address then you may use an Optional value instead. Accepting and returning an Optional<String> would remove the issue altogether and may bring down the number of methods and constructors. Storing the address as optional is not recommended because it cannot be serialized; probably best to use null, but make sure that you don't return null.

As you may notice, currently your Person instances can change completely by calling the setters. That's no good if you want to keep track of a person. This is why almost all organizations will create a unique ID for each person. That way any property can change without them becoming someone else. The ID can then represent the person in e.g. a transaction system.

Code

The code is generally well formed.

However there are some remarks to be made:

fields default to false so there is no need to initialize them to that value; it might be a good idea to set all fields in the various constructors so that they are not forgotten;
the if statement does not use braces; always use braces to subsequent changes do not result in unmaintainable code;
similarly, you will probably use class instances in collections, so you need to implement the hashCode and equals methods;
implementing toString is recommended for any class really.

Ideas

Using set methods is generally not a good idea. Having immutable objects often makes more sense. Other variants you may consider is having a Map of properties or having a factory / factory methods to create Person instances.

In Java, you may want to implement Serializable for data classes (and the static serializable UID - Eclipse will warn you about this) - that way you can more easily communicate data over a binary connection / stream. This is contended in the comments. In general, I would however try and keep in mind that you may want to be able to serialize a class in the future and - therefore - try and use serializable fields.


Downloading images from a forum I want to improve this program so I want you to criticize the code, and tell me what else can I do, and where can I find information about what I have to do.

TODO:

Fix wget timeout
The code should always check the returned value from any of the malloc family of functions to assure to memory allocation was successful
the code needs to check the returned value from fopen() to assure the operation was successful
The wget command can fail, so the wget line should route stderrto a file, then after system(cmd) should open/read that file to assure the wget was successful
The main() function will not compile cleanly because the declaration (as it should) indicates are int returned value, but no return (someint) line exists in the execution paths
In the download() function, what is the magic number '151' about? Better to #define that number, with a descriptive name and a comment
Eegarding the makefile:

cc := /usr/lib/gcc
Globbing all the compiles and the link into a single line is not a good nor flexible plan. Suggest breaking out into a compile rule from one .c to one .o and then using the list of .o files on a separate rule that performs the link.
The 'all' and 'clean' do not produce a file with those names. It suggests adding the line: .PHONY: all clean

#include <stdio.h>
#include <stdlib.h> // for using system calls
#include <stdbool.h>
#include <string.h> // for strlen

char  postBegin[] = "forum-post-body-content", postEnd[] = "p-comment-notes", img[] = "img src=";
int length1 = 23, length2 = 15, length3 = 8;
int pos1 = 0, pos2 = 0, pos3 = 0;

void downloadAndOpen (FILE **fp, int i, char *file);
bool search (char needle[], int length, char c, int *pos);
void download (FILE* *fp);

int main ()
{
    bool inPost = false;
    FILE *fp;
    int c;
    char file[20];

    for ( int i = 22; i <= 151; i++ )
    {
        downloadAndOpen (&fp, i, file);
        while ( (c = getc (fp)) != EOF ) {
            if ( search (postBegin, length1, c, &pos1) )
                inPost = true;
            if (inPost) {
                if ( search (postEnd, length2, c, &pos2) )
                    inPost = false;
                if ( search (img, length3, c, &pos3) )
                    download (&fp);
            }
        }
        fclose (fp);
        remove (file);
    }
}

void downloadAndOpen (FILE **fp, int i, char *file)
{
    char cmd[200]={0};
    // download web page
    sprintf (cmd, "wget -q -O page%d.txt 'http://www.mtgsalvation.com/forums/creativity/artwork/340782-official-digital-rendering-thread?page=%d'", i, i);
    system (cmd);

    // open text file
    sprintf (file, "page%d.txt", i);
    *fp = fopen (file, "r");
}

bool search (char needle[], int length, char c, int *pos)
{
    if (needle[*pos] == c)
    {
        if (*pos == length - 1)
        {
            return true;
            *pos = -1;
        }
        (*pos)++;
    }
    else
    {
        if(*pos > 0)
        *pos = 0;
    }
    return false;
}

void download (FILE **fp)
{
    char url[300], cmd[300];
    static int imageNumber = 496; // The image number where I left last time
    int pos = 0, c, j;
    bool found = false;

    while (!found)
    {
        if ( (c = getc (*fp)) == EOF ) {
            printf ("Image not found\n");
            return;
        }
        printf ("%c", (char) c);
        url[pos] = (char) c;
        if ( url[pos-1] == '\"' && url[pos] == '\"' )
            break;
        if ( url[pos-1] == '\"' && url[pos] == '>' )
        {
            printf ("\n");
            found = true;
        }
        ++pos;
    }
    pos -= 2;
    char url2[pos];
    for ( j = 1; j < pos; j++ )
    {
        url2[j - 1] = url[j];
    }
    url2[j - 1] = '\0';
    //http://joequery.me/code/snprintf-c/
    // wget -q for quiet -nc, --no-clobber   skip downloads that would download to existing files (no sobreescribir)
    snprintf(cmd, 300, "wget -q -nc --timeout=10 -O /home/arturo/Dropbox/Digital_Renders/%d \'%s\'", imageNumber++, url2);
    system(cmd);
    pos = 0;
}

Initialize all variables: fp, c, file etc.

your downloadAndOpen function has some issues

You do a system call to download (asynchronous call), but you do no attempt to wait for it to finish - instead you directly try to open it. If for some reason the download takes longer then it will fail. If your function fails you get a crash since you do not handle the return value of fopen.

I think the better approach would be to split it in two functions, one which does the download and returns when the file is there, alt. timeout with error code. Second part to open the file and returning the file pointer.

E.g.

int downloadFile( int pageNumber ); // return 1 - file downloaded 0 - failed
FILE* openFile( int pageNumber ); // NULL - file not found

Then add some checks to make sure they succeeded before you proceed.

if (downloadFile(pageNumber))
{
  FILE* fp = openFile(pageNumber);
  if (fp != NULL)
  {
   ...

You are a bit inconsistent with your if statements, sometimes you use braces sometimes not, sometimes the starting brace is on the same line as the if statement sometimes under the if. It makes the code harder to read. Choose one way and stick with that.

I am also not sure why you pass FILE** fp to download(), since you do not open some other file in there it is enough to pass the file pointer itself download(FILE* fp)

Global variables; bad, especially if they are not really used by several functions e.g. length1 is only used in main().

Comments; good to have. especially when I read such a line

for ( int i = 22; i <= 151; i++ )

It would be interesting to know why you start at 22.

EDIT:

I see you have some of my comments already in your todo list, anyway I will keep them here as well.


created a sql helper class using c# using System;
 using System.Collections.Generic;
 using System.Configuration;
 using System.Data;
 using System.Data.SqlClient;
 using System.Linq;
 using System.Web;

 namespace WebApplication1.App_Code
{
public class SqlHelper
{

    public string connstring;
    public SqlConnection con;
    public SqlCommand cmd;

    public SqlHelper(){}

    public SqlHelper(string sp)
    {
        connstring = ConfigurationManager.AppSettings["constr"];
        con = new SqlConnection(connstring);
        cmd = new SqlCommand(sp, con);
        cmd.CommandType = CommandType.StoredProcedure;
    }

}

public class Result : SqlHelper
{
    public Result(string sp) : base(sp)
    {
    }

    public bool ExecuteResultWithParameter(Dictionary<string, string> parameter)
    {

        bool result = true;
        try
        {
            foreach (var item in parameter)
            {
              cmd.Parameters.AddWithValue("@" + item.Key, item.Value);
            }
            con.Open();
            cmd.ExecuteNonQuery();
        }
        catch (Exception Ex)
        {
            result = false;
        }
        finally
        {
            cmd.Dispose();
            con.Close();
            con.Dispose();
        }
        return result;
    }

    public DataSet ReturnResultWithParameter(string sp, Dictionary<string, string> parameter)
    {

        DataSet dt = new DataSet();
        try
        {

         foreach (var item in parameter)
        {
            cmd.Parameters.AddWithValue("@" + item.Key, item.Value);
        }
        using (SqlDataAdapter sda = new SqlDataAdapter(cmd))
        {
            sda.Fill(dt);
        }

        }
        catch (Exception Ex)
        {

        }
        finally
        {
            cmd.Dispose();
            con.Close();
            con.Dispose();
        }
        return dt;
    }

    public DataSet ReturnResultWithoutParameter()
    {
        DataSet dt = new DataSet();
        try
        {

            using (SqlDataAdapter sda = new SqlDataAdapter(cmd))
            {

                sda.Fill(dt);
            }
        }
        catch (Exception Ex)
        {

        }
        finally
        {
            cmd.Dispose();
            con.Close();
            con.Dispose();
        }

        return dt;
    }

}

 }

I have written SQL helper class for my project and I want to improve the above code.also can someone suggest a suitable design pattern for the same.

Don't use public fields. Instead, make them public properties.
Use proper naming.
What is sp? storedProcedureName will be better.
Dictionary is a collection of parameters. Therefore, use the plural name: parameters.
Using the AddWithValue method may result in poor query performance. Use the Add method with the exact SqlDbType type specified.
The ExecuteNonQuery method returns the number of rows affected. Therefore, your ExecuteResultWithParameter method should return this value.
You can get rid of the bool result variable. The failure of the method should be signaled by an exception that is re-thrown in the catch block: throw ex; (please note: the name starts with a small letter).
The ReturnResultWithParameter method has an unused sp parameter. Remove it.
You swallow exceptions with empty catch blocks. Therefore, there is no way to find out about query errors.
In all methods, you dispose of the connection and the command in the finally blocks. After that, the methods cannot be called again without re-instantiating the class.
There are extra empty lines in your code - this is also an error.
Many of the errors I described will catch Visual Studio extensions like FxCop, StyleCop, and others. They will not only find them, but also suggest ways to fix them. Start using these extensions.


Find non duplicate Triangle Triplets from a list of given numbers- version 3 It is a continuation of my previous question:

Find non duplicateTriangle Triplets from a list of given numbers

After I realized triplet structure using less space than std:vector<int> and there is a simple way to remove duplicates from the triplets list by modifying loops index I rewrote the code again. On top of that it may be impossible to write a \$O(N^2)\$ solution for a three dimensional problem in this case. There are a few additional changes also: using lambda to avoid a function call and a few direct initialization attempts instead depending on assignment operations.

#include<vector>
#include<iostream>

struct Triplet
{
    int first;
    int second;
    int third;
};

std::vector<Triplet> findTriangleTriplets( std::vector<int>& vec_input )
{
    std::vector<Triplet> triplets;
    unsigned int size = vec_input.size();

    for( unsigned int i=0; i < size; ++i )
    {
        for( unsigned int j = i; j < size; ++j )
        {
            for( unsigned int k = j; k < size; ++k)
            {
                int first  ( vec_input.at( i ) );
                int second ( vec_input.at( j ) );
                int third  ( vec_input.at( k ) );

                if( [ &first, &second, &third ] {
                       return ( ( second + third  ) > first   )&&
                              ( ( first + third   ) > second  )&&
                              ( ( first + second  ) > third  );
                    }())
                {
                    triplets.emplace_back( Triplet{ first, second, third } );
                }
            }
        }
    }
    return triplets;
}

int main()
{
    std::vector<int> vec_input {  2, 4,  5, };

    std::vector<Triplet> triplets = findTriangleTriplets( vec_input );
    for ( auto &x : triplets )
    {
        std::cout<<x.first<<" "<<x.second<<" "<<x.third<<"\n";
    }
    return 0;
}

I have followed your previous implementations and I don't think there is much else you can change, code-wise. The next step is probably optimizing the algorithm, just like you've said.

Some nitpicking:

I personally didn't like your in-place lambda. It looks basically like a raw conditional, but with more boilerplate around it. Either make it a proper function or just leave the conditional as it was in the first iteration. Your hybrid approach does not appeal to me.
If you use push_back() instead of emplace_back(), you'll be able to omit repeating the type Triplet. Effectively, the results will be the same, since we are dealing with plain integers, so there's no way to avoid copying them. Use the DRYer version:

triplets.push_back( { first, second, third } );

unsigned int is likely smaller than vector::size_type on 64bit platforms. Realistically speaking, you probably don't need 64bit indexing, but a safer approach would be to use std::size_t or vector::size_type itself.
You can sprinkle a few consts here and there, such as for size, first, second and third. Right now the function is small, so it is easy to track the variables and see that they are never changed. However, if it were to be expanded in the future, it would aid the reader a lot to have an immutability guarantee when first spotting the variable declaration. Very important though, vec_input, as the name suggests, is input/read-only. So it must not be changed. Definitely make it const!
Any chance you can "guesstimate" the size of triplets when entering the function to reserve() some memory? If you could, that would potentially save a lot of time wasted with reallocations and copies done by [push|emplace]_back.


Tidying scaffolded Rails controller spec with subject/let I was just looking at a standard scaffolded Rails controller spec, and tried to get it to use subject or let blocks, and failed miserably ... can subject or let tidy controller specs the same as it would a model spec?

require 'spec_helper'

describe ProjectsController do

  def valid_attributes
    {}
  end

  def valid_session
    {}
  end

  before(:each) do
    @project = Project.create! valid_attributes
  end

  describe "GET index" do
    it "assigns all projects as @projects" do
      get :index, {}, valid_session
      assigns(:projects).should eq([@project])
    end
  end

  describe "GET show" do
    it "assigns the requested project as @project" do
      get :show, {:id => @project.to_param}, valid_session
      assigns(:project).should eq(@project)
    end
  end

  #etc
end

Check the answer from Pavel Druzyak for a different question. It answers your question as well.

Your code can be refactored to this;

describe ProjectsController do
    let(:project) { Project.create! }

    describe "GET index" do
      before(:each) { get :index }
      it { should respond_with(:success) }
      it { should assign_to(:projects).with([project]) }
    end

    describe "GET show" do
      before(:each) { get :show, {:id => @project.to_param} }
      it { should respond_with(:success) }
      it { should assign_to(:projects).with(project) }
    end
end


ECOO contest 2015: Problem 1 This is my solution to the ECOO 2015 contest, problem 1, Fractorials. I'd appreciate any advice to improve readability, style, organization, etc.

def main():
    number = int(input())
    fractorial = {}
    for i in range(2, number + 1):
        prime_factorization = prime_factor(i)
        update_fractorial(prime_factorization, fractorial)

    print(compute_fractorial_value(fractorial))

def prime_factor(number):
    primes = [2, 3, 5, 7, 11, 13, 17, 19]
    dict = {}
    i = 0
    while i < len(primes):
        p = 0
        while number % primes[i] == 0:
            number = number // primes[i]
            p += 1

        if p != 0:
            dict[primes[i]] = p

        i += 1

    return dict

def update_fractorial(prime_factorization, fractorial):
    for key in prime_factorization:
        if key in fractorial:
            if prime_factorization[key] > fractorial[key]:
                fractorial[key] = prime_factorization[key]
        else:
            fractorial[key] = prime_factorization[key]

        # Or should I replace the if/else statements with:
        #
        # if key in fractorial:
        #   if prime_factorization[key] < fractorial[key]:
        #      break
        #
        # fractorial[key] = prime_factorization[key]
        #
        # since it is shorter.

def compute_fractorial_value(fractorial):
    value = 1
    for key in fractorial:
        value *= key ** fractorial[key]

    return value

main()

Overall the code is quite easy to read, but it is more complicated than it needs to be.

    fractorial = {}
    for i in range(2, number + 1):
        prime_factorization = prime_factor(i)
        update_fractorial(prime_factorization, fractorial)

    print(compute_fractorial_value(fractorial))

I would expect the name fractorial to be either the final integer or the function which computes it. Perhaps fractorial_factorization would be a clearer name.

compute_fractorial_value doesn't actually do anything specific to fractorials. Perhaps compose_factorization or multiply_powers?

The algorithm used to compute the prime factorisation of the fractorial is correct, but overkill. If \$p\$ is a prime and \$a\$ is a positive integer, under what condition is \$p^a\$ a factor of the \$n\$th fractorial?

def prime_factor(number):
    primes = [2, 3, 5, 7, 11, 13, 17, 19]
    dict = {}
    i = 0
    while i < len(primes):
        p = 0
        while number % primes[i] == 0:
            number = number // primes[i]
            p += 1

        if p != 0:
            dict[primes[i]] = p

        i += 1

    return dict

Apart from the hard-coded primes, this is a standard implementation. I would suggest that p is more conventionally used to denote primes[i], so perhaps power would be a less surprising name.

def update_fractorial(prime_factorization, fractorial):
    for key in prime_factorization:
        if key in fractorial:
            if prime_factorization[key] > fractorial[key]:
                fractorial[key] = prime_factorization[key]
        else:
            fractorial[key] = prime_factorization[key]

        # Or should I replace the if/else statements with:
        #
        # if key in fractorial:
        #   if prime_factorization[key] < fractorial[key]:
        #      break
        #
        # fractorial[key] = prime_factorization[key]
        #
        # since it is shorter.

Shorter still, and more explicit:

def update_fractorial(prime_factorization, fractorial):
    for key in prime_factorization:
        if key not in fractorial or prime_factorization[key] > fractorial[key]:
            fractorial[key] = prime_factorization[key]

Although it's preferable to avoid repeating the lookup. The efficient way of doing this is

def update_fractorial(prime_factorization, fractorial):
    for key, val in prime_factorization.items():
        if key not in fractorial or val > fractorial[key]:
            fractorial[key] = val

def compute_fractorial_value(fractorial):
    value = 1
    for key in fractorial:
        value *= key ** fractorial[key]

    return value

Apart from the previous comment about the name, the only thing I'd say here is that if you really want to scale this up, multiplying lots of small numbers is best done on a tree-like basis. I.e. you pair up the numbers, multiply each pair, pair up again, multiply again, ... But that's overkill for this problem.

main()

This is half-way between the naïve approach of not defining main() at all and just dumping its contents at the end, and the "best practice" approach of

if __name__ == "__main__":
    main()

Might as well get used to the best practice approach now.


Find longest Palindromic substring technical interview question i got from daily interview pro
A palindrome is a sequence of characters that reads the same backwards and forwards. Given a string, s, find the longest palindromic substring in s.
Example:

Input: "banana"
Output: "anana"

Input: "million"
Output: "illi"

i was wondering if there is any way to optimize the code more
Also is using built in functions such as join(),index() and so on not preferred for a coding interview ?Because by using it it makes it so much easier in python compared to languages like java
class Solution:

    def checkOddChars(self,lst):
        ls = []
        for i in lst:
            if lst.count(i) == 1:
                ls.append(i)
        return ls

    def checkPalindrome(self,lst):
        return lst[:] == lst[::-1]

    def longestPalindrome(self, s):
      lst = list(s)

      while lst:
        if self.checkPalindrome(lst) :
            return(''.join(lst))

        oddChars = self.checkOddChars(lst)

        if lst[0] in oddChars:
            del lst[0]
        if lst[len(lst)-1] in oddChars:
            del lst[len(lst)-1]

      return('List is empty')

# Test program
s = "tracecars"
print(str(Solution().longestPalindrome(s)))
# racecar
```

def checkPalindrome(self,lst):
    return lst[:] == lst[::-1]

This is an extremely expensive implementation. lst[:] is needlessly creating a copy of lst, then lst[::-1] creates a second complete copy of lst (but reversed). There are many ways of approaching this, but I would go for something like this:
def checkPalindrome(self, lst):
    return all(start == end
               for start, end in zip(lst, reversed(lst)))

It zips lst with a reversed list iterator:
>>> lst = "abcd"
>>> list(zip(lst, reversed(lst)))
[('a', 'd'), ('b', 'c'), ('c', 'b'), ('d', 'a')]

Then checks to see if all the pairs are equal. If they are, the list is a palindrome. This method is still inefficient though since only half of each needs to be checked. It can be further improved by introducing itertools.islice to get a "view" of the first half of the zipped lists:
from itertools import islice

def checkPalindrome(self, lst):
    half_pair_view = islice(zip(lst, reversed(lst)), len(lst) // 2 + 1)
    return all(start == end for start, end in half_pair_view)

islice is like normal list slicing except instead of creating a copy, it just allows you to iterate over a limited portion of the original iterable.
This code is more efficient because every function involved here is "lazy": they do only as much work as they need to. reversed, zip, islice, and the generator expression all return an iterator that can produce elements (but do little work up front). all also exits as soon as it gets a Falsey result, so it's comparable to a for loop that contains a break in some branches. This is key here because we only want to do as much work as is necessary to determine whether not they're palindromes. Making two full copies of the list does a large amount of work; far more than is required to check if the string is a palindrome.

checkOddChars is a textbook use-case for a list comprehension:
def checkOddChars(self, lst):
    return [i for i in lst if lst.count(i) == 1]

If you ever find yourself initializing an empty list, then iterating over another iterable and adding to the list, you likely want a comprehension.
This is quite an expensive function too. count needs to iterate the entire list each time; once for every element in lst. This also double counts any repeated elements. I'm not sure off the top of my head what a better solution is though.

 del lst[0]

This is also quite expensive. Ideally, you shouldn't delete from a list except for at the very end. Lists don't support efficient deletes, and the closer to the beginning of the list you delete from, the worse it is. I'd switch to using a dequeue instead of a list, which avoids the overhead of popping from the beginning.


Lissajous pattern simulator I had seen a YouTube video making actual Lissajous patterns with sand falling from a pendulum with 2 different periods and looking in the Wikipedia page the looked very easy to simulate.

I made a Processing sketch simulating such a pattern. Here is an example output with a = 3, b = 4 (the same as the third Wikipedia example).

// Lissajous patterns simulator
// https://en.wikipedia.org/wiki/Lissajous_curve

void setup() {
  size(600, 600);
  background(255);
}

int A = 300; // X stretching factor
int B = 300; // Y stretching factor
float a = 3; // x "relative importance"
float b = 4; // y "relative importance"
float time = 0;
int offset = 0;
final float TIMESTEP = 0.001;

void draw() {
  int x = (int) ( A * sin(a*time + offset) );
  int y = (int) ( B * sin(b*time) );

  point(x + A, y + B);
  time += TIMESTEP;
}

I have two main questions:

I kept the names short to match closely the Wikipedia specification, and added a short comment near them. Is this best practice?
I have quite a few related variables. Should I encapsulate them into a class (maybe LissajousSimulator, with a single function updateState)? Or would that just be over-engineering?

About point (1)
If it's obviously copied from somewhere without changing anything then it's perfectly fine to not change anything to the code itself. Given that you also have the link to the (pseudo-)code. In this case I wouldn't even add comments other than: look at this page to know how it works.

If however you want to integrate it better into your own code to treat it like it's actually part of it then there's no reason to keep the code as close to the original as possible.
Instead you should refactor it until it fits the general java standards. Meaning making variables private, making constants final (static), following naming conventions (both for variables and methods), ...

question (2)
(see above if you want to stick to the structure from the source without changing anything. If you start refactoring following coding standards read on)

If like you say there are a number of variables and methods that are forming a coherent group then you should put them into their own class.

For your new class you'll have to find out what to put in the class and what methods to provide.

My first thought is something like this:

public class LissajousPointCalculator{
    private static final int HORIZONTAL_FACTOR = 300;
    private static final int VERTICAL_FACTOR = 300;
    private float xTimeScale;
    private float yTimeScale;
    private float xTimeOffset;

    public LissajousPointCalculator(float xTimeScale, yTimeScale, xTimeOffset){
        this.xTimeScale = xTimeScale;
        this.yTimeScale = yTimeScale;
        this.xTimeOffset = xTimeOffset;
    }

    public Point getPointAt(float time) {
        int x = (int) ( HORIZONTAL_FACTOR  * Math.sin(xTimeScale*time + xTimeOffset) );
        int y = (int) ( VERTICAL_FACTOR  * Math.sin(yTimeScale*time) );

        return new Point(x + HORIZONTAL_FACTOR , y + VERTICAL_FACTOR );
    }
}

Note that I did not include any timetracking inside this class. My reasoning is that the other class that uses this one is responsible for deciding which points to draw. So that class is responsible for staring over, deciding the step size, deciding how many points to add before drawing it on the screen etc...

I also made the horizontal and vertical scaling factors constant inside the class. This was just as an example. If you want to be able to change the size of the figure as well it's obvious that you want these as variables that you can pass through constructor or setters instead.


Creating a description of an item from an inventory I'm really new to Python (like only a month of study) and I was testing out dictionaries, so I tried to make an 'inventory' where you can pick an item from your inventory and examine it. But I can tell that I'm not doing this in the most efficient or elegant way.

Originally I thought I could make each item its separate dictionary, kind of like this:

#define what's in your inventory using a list
inventory = ["apple", "tire"]

#create a list of items with attributes using dictionaries
apple = {
    "name":"apple",
    "type":"fruit",
    "edible":"edible",
    "color":"red",
}

tire = {
    "name":"tire",
    "type":"object",
    "edible":"not edible",
    "color":"grey"
}

#explain the program to the user
print("Choose an item to examine. Items in your inventory are:")
print(*inventory, sep = ", ")

#set the loop to true
itemsloop = True

#begin the loop
while itemsloop == True:
    #ask for user input
    x = input()

But then I got stuck at the part where it's time to take the input and match it to the name of a dictionary, because one is a string and one isn't. The following code is working, but feels... inelegant and unnecessary to type out (prefix)-name, (prefix)-type, (prefix)-edible for every entry. I feel like there must be a simpler way to accomplish this that I'm just not seeing because I don't think like a programmer yet. I don't know.

If you can explain any more efficient methods with simple terminology (again, huge newbie not just to Python but programming in general), I'd really appreciate it!

#define what's in your inventory using a list
inventory = ["apple", "tire"]

#create a list of items with attributes using a dictionary
items = {
    "apple-name":"apple",
    "apple-type":"fruit",
    "apple-edible":"edible",
    "apple-color":"red",

    "tire-name":"tire",
    "tire-type":"object",
    "tire-edible":"not edible",
    "tire-color":"grey"
}

#explain the program to the user
print("Choose an item to examine. Items in your inventory are:")
print(*inventory, sep = ", ")

#set the loop to true
itemsloop = True

#begin the loop
while itemsloop == True:
    #ask for user input
    x = input()

    if x in inventory:
        #if the input matches something you have, find the attributes of
        #the item in the inventory and print them.
        print("This is a " + items[x + "-color"] + " " +
        items[x + "-name"] + ", a type of " + items[x + "-type"]
        + ". It is " + items[x + "-edible"] + ".")
    elif x == "quit":
        #if the player wishes to quit, end the program.
        itemsloop == False
        break
    else:
        #if the item is not recognized, let the player know.
        print("You don't have that in your inventory.")

(The sample output would be something like, "This is a red apple, a type of fruit. It is edible.")

As you use item's name to search the item information, so you can design the name as the key of items hash table.

items = {
    "apple":
        {
            #type, edible,color information
        },
    ...
}


Rust shortest way to find the maximum product of a fixed length substring I'm trying to code in a Rust a function, that gives the maximum product of adjacent digits.
So given the string 123456789, with 3 adjacent digits, the maximum is 7 * 8 * 9 = 504.
I've tried to come up with this current functional solution (see previous edit for longer solutions). Could it be shortened? Comments are welcome.
The log is here, and desired, so the code runs quickly.
use std::iter::Map;
use std::str::Chars;

fn iter_log_digits(str: &str) -> Map<Chars<'_>, fn(char) -> f64> {
  str.chars().map(|c: char| (c.to_digit(10).unwrap() as f64).ln())
}

/// Returns the maximum log sum of adjacent digit characters.
/// # Arguments
/// - `str`: Assumes there are no 0s.
/// - `adj_count`: The number of adjacent digit characters.
fn max_adj_log_sum_no_zeros(str: &str, adj_count: usize) -> f64 {
  if str.len() < adj_count {
    return 0.;
  }
  let log_first: f64 = iter_log_digits(&str[..adj_count]).sum();
  iter_log_digits(&str).zip(iter_log_digits(&str[adj_count..])).fold(
    (log_first, log_first), // `(log_cur, log_max)`
    |(log_cur, log_max), (x_left, x_right)| {
      let log_next = log_cur - x_left + x_right;
      (log_next, log_max.max(log_next))
    }
  ).1
}

fn max_product_no_zeros(str: &str, adj_count: usize) -> u64 {
  max_adj_log_sum_no_zeros(str, adj_count).exp().round() as u64
}

max_product_no_zeros("123456789", 3) should return 504.

General observations:

An indentation of two spaces is quite small.  The standard formatting uses four spaces.

Names like Map and Chars read better with their modules: iter::Map, str::Chars.

str: &str — don't do that.

When str is not entirely made of decimal digits, your function panics by unwrapping.  Reporting an error via Result is generally preferred.  Magic also happens when str contains zeros.

Using fold here makes the semantics harder to understand.  A simple for loop might be more suitable.

Now, my intuition tells me floating point calculations are less efficient than just doing the operations on integers, so I wrote a benchmark.  Here's the result that I got:
running 4 tests
test l_f::tests::test ... ignored
test simonzack::tests::test ... ignored
test l_f::tests::bench       ... bench:  10,189,230 ns/iter (+/- 278,958)
test simonzack::tests::bench ... bench:  16,865,990 ns/iter (+/- 440,350)

test result: ok. 0 passed; 0 failed; 2 ignored; 2 measured; 0 filtered out

Using integers resulted in a ~1.7x speedup.  Do note that this benchmark is very crude, and you might want to measure your own use cases.

For reference, here's the code for the benchmark.  (Error checking is omitted for simplicity.)
#![feature(test)]

extern crate test;

mod l_f {
    fn to_digits(digits: &str) -> impl Iterator<Item = u64> + '_ {
        digits.chars().map(|c| c.to_digit(10).unwrap().into())
    }

    pub fn max_product_no_zeros(digits: &str, length: usize) -> u64 {
        if digits.len() < length {
            return 0;
        }

        let mut current_product: u64 = to_digits(&digits[..length]).product();
        let mut max_product = current_product;

        for (left, right) in to_digits(digits).zip(to_digits(&digits[length..])) {
            current_product = current_product / left * right;
            max_product = max_product.max(current_product);
        }

        max_product
    }

    #[cfg(test)]
    mod tests {
        use test::Bencher;

        #[test]
        fn test() {
            assert_eq!(super::max_product_no_zeros("31415926", 3), 108);
        }

        #[bench]
        fn bench(b: &mut Bencher) {
            let digits = test::black_box("9".repeat(1_000_000));

            b.iter(|| super::max_product_no_zeros(&digits, 10));
        }
    }
}

mod simonzack {
    use std::iter::Map;
    use std::str::Chars;

    fn iter_log_digits(str: &str) -> Map<Chars<'_>, fn(char) -> f64> {
        str.chars()
            .map(|c: char| (c.to_digit(10).unwrap() as f64).ln())
    }

    /// Returns the maximum log sum of adjacent digit characters.
    /// # Arguments
    /// - `str`: Assumes there are no 0s.
    /// - `adj_count`: The number of adjacent digit characters.
    fn max_adj_log_sum_no_zeros(str: &str, adj_count: usize) -> f64 {
        if str.len() < adj_count {
            return 0.;
        }
        let log_first: f64 = iter_log_digits(&str[..adj_count]).sum();
        iter_log_digits(&str)
            .zip(iter_log_digits(&str[adj_count..]))
            .fold(
                (log_first, log_first), // `(log_cur, log_max)`
                |(log_cur, log_max), (x_left, x_right)| {
                    let log_next = log_cur - x_left + x_right;
                    (log_next, log_max.max(log_next))
                },
            )
            .1
    }

    pub fn max_product_no_zeros(str: &str, adj_count: usize) -> u64 {
        max_adj_log_sum_no_zeros(str, adj_count).exp().round() as u64
    }

    #[cfg(test)]
    mod tests {
        use test::Bencher;

        #[test]
        fn test() {
            assert_eq!(super::max_product_no_zeros("31415926", 3), 108);
        }

        #[bench]
        fn bench(b: &mut Bencher) {
            let digits = test::black_box("9".repeat(1_000_000));

            b.iter(|| super::max_product_no_zeros(&digits, 10));
        }
    }
}


Reusability vs simplicity in a small game with a set of interfaces I have a small game I'm working on with a set of interfaces:

IHavePosition:

public interface IHavePosition<T>
{
    T Position { get; set; }
}

ICanMove:

public interface ICanMove<T> : IHavePosition<T>
    {
        IEnumerable<IMotor<T>> Motors { get; set; }
    }

ICharacter:

public interface ICharacter<TPos> : IHavePosition<TPos>, ICanMove<TPos>
{
    /// <summary>
    /// Gets or sets the action points.
    /// </summary>
    /// <value>
    /// The action points.
    /// </value>
    int ActionPoints { get; set; }

    /// <summary>
    /// Gets or sets the maximum action points.
    /// </summary>
    /// <value>
    /// The maximum action points.
    /// </value>
    int MaxActionPoints { get; set; }
}

IPlayer:

public interface IPlayer<TPos> : ICharacter<TPos>, IResetable
{
    /// <summary>
    /// Gets or sets the gold in the player's inventory.
    /// </summary>
    /// <value>
    /// The gold.
    /// </value>
    int Gold { get; set; }
}

IEnemy:

public interface IEnemy<TPos> : ICharacter<TPos>
    {

    }

As you can see, they all share a generic parameter TPos, which is the data type used to store an object's position. I keep this generic because I hope to reuse these interfaces in later projects which might have different position storage types (e.g. floating point coordinates versus an integer grid. 3D coordinates versus 2D, etc). The problem, however, is that this means when creating my classes, I need to ferry around this TPos generic construct to places where it doesn't make sense to define it. I'm left creating new Player<Vector3> or Player<Int2> instances all over the place when really I just want to be creating a Player.

An additional problem is that this might not (conceivably) be the only time I need to do this. What if I want to add a Health field that could be an int, or a float, etc? I'm now creating Player<Vector3,int> or Enemy<Int2, float> instances all over the place.

Since TPos is the same project-wide, is there a better way of doing this than lugging around references to TPos everywhere?

The problem, however, is that this means when creating my classes, I need to ferry around this TPos generic construct to places where it doesn't make sense to define it. I'm left creating new Player<Vector3> or Player<Int2> instances all over the place when really I just want to be creating a Player.

Yes, specifying Player<Vector3> each time can be quite frustrating. I'm not sure though if you just want to create just a Player or if you want to create a specific kind of player

You could create a GameOnePlayer class which extends Player<Vector3>, so then you create a GameOnePlayer and not a Player<Vector3>. You might have use for the Abstract Factory Pattern for the creation of these objects.

I think however that your class inheritance will become quite complex, if it isn't complex enough already. Which you state yourself with this:

  An additional problem is that this might not (conceivably) be the only time I need to do this. What if I want to add a Health field that could be an int, or a float, etc? I'm now creating Player<Vector3,int> or Enemy<Int2, float> instances all over the place.

It sounds like what you need is to use composition over inheritance. A common way to do this in modern games is to use the Entity Component System approach.

If you go with this approach, both a Player and an Enemy should both just be an Entity. Then you can add whatever components they need, such as a Vector3Position component or a Int2Position component. Also IntHealthComponent and FloatHealthComponent.

There is a Entity Component System library named Artemis which has been ported to C# (I haven't used this myself, but I believe it's suitable enough for your needs). You can also make a Entity Component System implementation yourself, which many others have done on Code Review already (see entity-component-system)

To get an additional understanding of ECS, there is also an excellent question on GameDev StackExchange

Even though ECS is mostly used in real-time games, it is also possible to use it in turn-based games, which I have started doing myself in my Trading Card Game code.


Sending activation email for sqlmembership private void SendActivationEmail(MembershipUser user)
{
    var message = new MailMessage();
    message.To.Add(user.Email);
    message.Subject = "Attorney Event Notification Activation";

    string urlBase = Request.Url.GetLeftPart(UriPartial.Authority) + Request.ApplicationPath;
    string ActivationUrl = "/ActivateUser.aspx?ID=" + GetUserID(user.Email);
    var fullPath = urlBase + ActivationUrl;

    using (StreamReader sr = new StreamReader(Request.PhysicalApplicationPath + "ActivationTemplate.txt"))
    {
        message.Body = sr.ReadToEnd();
    }

    message.Body = message.Body.Replace("<%UserName%>", user.UserName);
    message.Body = message.Body.Replace("<%ActivationURL%>", fullPath);
    message.IsBodyHtml = true;

    using (var smtpClient = new SmtpClient())
    {
        smtpClient.Send(message);
    }
}

How can I clean up this activation email sender?

And is it relatively safe?

I think there is a problem with encapsulation and the single responsibility principle here. The method depends on too many external factors, it makes too many external references, and it includes logic of multiple independent responsibilities that would make sense to extract elsewhere.

These are good and belong to this method:

Creating and filling a MailMessage object
The constant strings for the subject
The SmtpClient object that sends the message

If the method contained only the above,
it would be perfectly reasonable.

But it contains some extra stuff that don't seem to belong:

The construction logic of fullPath. It seems this logic should be somewhere else, for example a centralized router component
The construction logic of the template file path to read
The replacement of the template parameters UserName and ActivationURL would also be good to extract to a component. There should be a component in charge of user templates and the template parameters, which naturally takes a user template and returns properly formatted text. That way, if the template changes, the logic of the template parameters will be closer to the logic that fills those parameters, making the code more cohesive.

Lastly, as a minor remark, the variable naming is not consistent.
Most of the local variables are camelCase, except ActivationUrl:

string urlBase = Request.Url.GetLeftPart(UriPartial.Authority) + Request.ApplicationPath;
string ActivationUrl = "/ActivateUser.aspx?ID=" + GetUserID(user.Email);

It would be better to use camelCase consistently.


Build Excel formulas with string replacements for (long i = 2; i <= numberOfRows; i++)
{
    string quarry =
        "=IF(J"+i+"<10000,J"+i+"*0.05,IF(J"+i+ "<30000,(J" + i + "-10000)*0.1+500,IF(J" + i + "<70000,(J" + i + "-30000)*0.15+2500,IF(J" + i + "<140000,(J" + i + "-70000)*0.2+8500,IF(J" + i + "<250000,(J" + i + "-140000)*0.25+22500,IF(J" + i + "<500000,(J" + i + "-250000)*0.3+50000,IF(J" + i + ">500000,(J" + i + "-500000)*0.32+125000)))))))";
    WS.Cells[i, 11].Formula = quarry;
}

I had originally intended to make this alot neater by adding the base string into a StringBuilder and then doing a string.Replace call on the column value with the long variable i, but it did not work saying i cannot be converted to char.

Not limited to a string builder, how else can I improve this statement where I could easily change the value of a very long string by replacing the concerned elements?

(i.e: this is a pretty long Excel formula C# query that gets inserted into the worksheet as the for loop rolls)

I'm not even going to review the C#, because there's a far better way to do the Excel portion.

Instead of creating this enormous formula (which I hope you don't need anymore nested IF portions, you've hit Excel's limit of 7), you can create a table that has the formula properties in it, then lookup what's in that table.

So, let's create our formula table/area:

---L---+--M--+---N---
Level  |Rate |Add
      0| 0.05|      0
  10000|  0.1|    500
  30000| 0.15|   2500
  70000|  0.2|   8500
 140000| 0.25|  22500
 250000|  0.3|  50000
 500000| 0.32| 125000

So, with that, we can build our actual formula:

=(J1-INDEX(L:L,MATCH(J1,L:L,1)))*INDEX(M:M,MATCH(J1,L:L,1))+INDEX(N:N,MATCH(J1,L:L,1))

If you're not an Excel whiz, here's what's happening:

The INDEX(...) function in a formula will do a lookup on the first value in a column that matches a specific location. So, INDEX(L:L,1) will return the value in L1. Simple enough.

Next, MATCH(...) is a function that will find the first value in a column that is less than, equal to or greater than the provided value. So MATCH(J1,L:L,1) will find the index of the cell that is the last cell found which is less than (the final 1 parameter) our current cell. So from our L column, for 1 it finds 0, etc. It then returns the row that cell is on, which can be piped to INDEX to find the actual value in that cell (or a different column).

So when we put it together, assuming J1 is 100000, we're replacing our formula with:

=(100000-70000)*0.2+8500

Which gives us 14,500, the exact value your formula gives us.

Some bonuses of this version:

Shorter, more concise formula. Easy copy/paste, easy to change the values later. (Suppose the criteria for 70000 become 65000, easy to change.)
More descriptive information, adding values is a piece of cake (and requires 0 work from the programmer).
Doesn't require a C# programme to pre-fill the spreadsheet. (That's a huge bonus right there.)
Makes you look like a boss being able to show the business guys how this stuff works.
Less typing in the future.

These are not mutually exclusive items.


Smalltalk (Squeak) - String parsing I'm learning Smalltalk programming as I think its way ahead as an OOP programming tool. I would appreciate feedback regarding my coding of the following problem. I have a sequence of letters representing bases in a DNA and want to change the T to a U in the string. I have done this in two ways. The first uses the copyReplaceAll:with: message to the string object which changes every occurrence of 'T' to a 'U':

string := 'AGCTTTTCATTCTGACTGCAACGGGCAATATGTCTCTGTGTGGATTAAAAAAAGAGTGTCTGATAGCAGC'.
string copyReplaceAll: 'T' with: 'U'.

My second method uses iteration and boolean conditions to test whether a 'T' is identified. I learned that I have to copy the string to an Array object, then iterate over that object and copy the elements to an empty array. Ideally I would like the original string object to be mutable. Am I going at this the wrong way? Here's my code:

string := 'AGCTTTTCATTCTGACTGCAACGGGCAATATGTCTCTGTGTGGATTAAAAAAAGAGTGTCTGATAGCAGC'.
strCopy := string asArray.
empty := Array new: strCopy size.
cntr := 1.

strCopy do: [:char |
     (char = $T) ifTrue: [empty at:cntr put: $U ]
    ifFalse: [empty at:cntr put: char].
    cntr := cntr + 1
    ].

Since Smalltalk is super interactive also try browsing through the methods of e.g. ByteString and other classes and the different ways to search for methods and their implementors:

You can actually run do: and at:put: on it directly, but there's also doWithIndex: for example, which looks like it will make your second bit even simpler:

string := 'AGCTTTTCATTCTGACTGCAACGGGCAATATGTCTCTGTGTGGATTAAAAAAAGAGTGTCTGATAGCAGC'.
string doWithIndex: [ :char :i | (char = $T) ifTrue: [ string at: i put: $U ] ].

Now, in terms of readability, the first one is of course much easier to understand.  It's to the point and obvious even for readers with no knowledge of Smalltalk.  The second one and the example I posted aren't (or are less so at least) - they use more complex syntax and several new concepts.

If you restrict yourself to just String I think your second approach is fine though.


Function that tests and mutates I am a huge proponent of self-documenting code (when at all possible).  I prefer to have code be self evident by the very nature of the names of the variables, methods, etc.

For instance:

if (TheSky.IsBlue && TheGrass.IsGreen)
    BeHappy();

I have run into a situation that has defied all attempts to name the method in a way that illustrates what it does.  The problem is that the method is required to do two things.

I have a class that must maintain its own state, however it must have a method that both verifies a condition, and changes the state via a thread safe lock to prevent race conditions.

It basically looks like the code below:

public class TimeWindow
{
    private static object lockObject = new object();
    private DateTime Timestamp;
    private int WindowCounter;

    // What do I call this?  CanWeDoSomething() implies that it is merely
    // calculating a true/false state, not mutating state.
    //
    // I don't want to call it
    // CanWeDoSomethingAndDecrementItIfWeActuallyDoIt() as that is
    // just too annoying, and it messes up the self-documenting nature
    // since if (CanWeDoSomethingAndDecrementItIfWeAcutallyDoIt()) describes
    // two actions, but only one of them is a Boolean condition, so it doesn't
    // really fit in with the if statement.
    public bool CanWeDoSomething(int WindowSeconds) {
        lock(lockObject) {
            if ((DateTime.Now - TimeStamp).TotalSeconds > WindowSeconds) {
               Timestamp = DateTime.Now;
               WindowCounter = 0;
            }
            if (WindowsCounter < 10) {
               ++WindowCounter;
               return true;
            }
            return false;
        }
    }
}

As you can see, I have to perform the test (to retrieve the Boolean) and modify state at the same time.  If I didn't do this, then a race condition is possible where the value can be changed by another thread in between checking it's value and mutating its state.

The naming is important because if it implies no state change, then people call the method multiple times without realizing that it also changes state.

Can anyone suggest a name that adequately documents what the method is doing, without making it overly complex?  The name should indicate that it's mutating the object as well as performing an action that is a Boolean state.

Alternatively, if you can suggest a better way to accomplish this in a thread safe manner that would work better, that would be good too.

I would call this method AcquireTimeSlot or similar, since the main action here is actually capturing a timely-limited resource. Result of the method specifies whether resource has been acquired or not. You may also call it TryAcquireTimeSlot to highlight the fact that acquisition may fail.

As a side note - you can rewrite the code without locks in case if that's the only place you use the lockObject. It will be a bit more verbose though...

public class TimeWindow
{
    private static readonly object _lockObject = new object();
    private long _timestampTicks;
    private int _windowCounter;

    private void CheckWindowExpiration(int windowSeconds)
    {
        long timestampTicks = _timestampTicks;
        while (TimeSpan.FromTicks(DateTime.Now.Ticks - timestampTicks).TotalSeconds > windowSeconds)
        {
            if (Interlocked.CompareExchange(ref _timestampTicks, DateTime.Now.Ticks, timestampTicks) == timestampTicks)
            {
                _windowCounter = 0;
                break;
            }

            timestampTicks = _timestampTicks;
        }
    }

    public bool TryAcquireTimeSlot(int windowSeconds)
    {
        CheckWindowExpiration(windowSeconds);

        int windowCounter = _windowCounter;
        while (windowCounter < 10)
        {
            if (Interlocked.CompareExchange(ref _windowCounter, windowCounter + 1, windowCounter) == windowCounter)
                return true;

            windowCounter = _windowCounter;
        }

        return false;
    }
}


Speeding up thousands of string parses I have a mapping application which takes string arguments in the form of string arrays. I parse these and then perform an action on the map. One of these is MoveVertex which lets the user pass in the name of an existing shape/feature on the map, the index of the vertex to move, and a new x,y location to move that vertex to.

I'm currently getting thousands of these in a row, all for the same shape. It takes anywhere from 3-10ish seconds to parse them all and complete the action. I'm looking for any improvements I could make.

One thing I did so far was stop the map from refreshing every time a vertex is moved. This helped a bit, but it's still slow.

Example of the string[] commands being sent:

command[0] = "move" //command type
command[1] = "vertex" //what to move
command[2] = "testLine[10]" //move the vertex at index 10 in testLine's list of vertices
command[3] = "x" //new x value for the vertex
command[4] = "y" //new y value for the vertex

Here's the function that does the work:

public static void MoveVertex(string[] command, Layer inMemoryFeatureLayer, Map map)
{
    int index; //this will be the index of the vertex in the collection of vertices of the shape
    string featureName; //name of the shape/feature that has the vertex to move

    if (command.Length < 5) //can't be more than 5 parameters
    {
        return;
    }

    try
    {
        if (!command[2].Contains('[')) //if it doesn't contain brackets which contain an index of the vertices
        {
            return;
        }

        const string pattern = @"\[(.*?)\]";
        var query = command[2];
        var matches = Regex.Matches(query, pattern); //Gets anything inside the brackets
        index = Convert.ToInt32(matches[0].Groups[1].Value); //should be an int

        featureName = command[2].Substring(0, command[2].IndexOf('[')).ToUpper(); //everything before the bracket is the name of the object
    }
    catch (Exception ex)
    {
        return;
    }

    if (!double.TryParse(command[3], out double longitude) || !double.TryParse(command[4], out double latitude))
    {
        return;
    }

    try
    {
        BaseShape shape;

        inMemoryFeatureLayer.Open(); //make sure the layer is open
        if (inMemoryFeatureLayer.Name.Contains(GlobalVars.LineType)) if it's the layer holding line shapes
        {
            shape = (LineShape)inMemoryFeatureLayer.FeatureSource.GetFeatureById(featureName, ReturningColumnsType.NoColumns).GetShape();
            ((LineShape)shape).Vertices[index] = new Vertex(longitude, latitude); //set the vertex to a new location
        }
        else //it's the layer holdilng polygone shapes
        {
            shape = (PolygonShape)inMemoryFeatureLayer.FeatureSource.GetFeatureById(featureName, ReturningColumnsType.NoColumns).GetShape();
            ((PolygonShape)shape).OuterRing.Vertices[index] = new Vertex(longitude, latitude); //set the vertex to a new location
        }

        inMemoryFeatureLayer.EditTools.BeginTransaction();
        inMemoryFeatureLayer.EditTools.Update(shape);
        inMemoryFeatureLayer.EditTools.CommitTransaction();
        map.Refresh(); //this won't happen because I suspend refreshes beforehand
    }
    catch (Exception ex)
    {
        //log it
    }
}

You should take a look at the possibility to name groups in regex patterns; your match pattern can then be a oneliner:

  const string pattern = @"^(?<name>\w+)\[(?<index>\d+)\]$";

  Match match = Regex.Match(command[2], pattern);

  if (match.Success)
  {
    string featureName = match.Groups["name"].Value;
    int index = int.Parse(match.Groups["index"].Value);

    try
    {
      // TODO: use the information
    }
    catch (Exception ex)
    {
       // TODO: Notify consumer
    }
  }
  else
  {
    // TODO: notify consumer
  }

As shown, the pattern for the index is changed from .*? to \d+ which is more precise and hence more efficient because it doesn't need the non-greedy operator ?.

if (command.Length < 5) //can't be more than 5 parameters
{
    return;
}

When meeting an error you just return from the method without doing anything. Shouldn't you notify the consumer/log the situation/condition?

You don't write from where the command information comes from, but if you read it from a text file, I think I would parse the string information while reading the file and then create a set of command objects defined as something like:

public class CommandInfo
{
  public string Action { get; set; }
  public string Type { get; set; }
  public string ListName { get; set; }
  public int Index { get; set; }
  public Point Vertex { get; set; }
}

Or whatever is suitable for your needs

This may not improve performance but it is more "type safe" than just dispatching an array of strings around.

According to efficiency:

You write, that you have thousands of commands for the same shape, but it seems that you requery a set of shapes inMemoryFeatureLayer.FeatureSource for every command. Whouldn't it be more efficient if you group the commands for each shape, so that you only have to query for the shape once and then can execute all the related commands?

TIP: If you are using Visual Studio 2019, you get help from intellisense and color coding if you compose the regex pattern directly in the call to Match or Matches:

Match match = Regex.Match(command[2], @"^(?<name>\w+)\[(?<index>\d+)\]$");

UPDATE

FYI: there has been some discussion in the comments to various answers about which approach is the most efficient, when extracting the name of the list of vertices and the index from a string like command[2] = "testLine[10]" => "testLine" and 10. So I ran some comparison tests for the following methods:

const string pattern = @"^(?<name>\w+)\[(?<index>\d+)\]$";

Regex regex = new Regex(pattern, RegexOptions.Compiled | RegexOptions.Singleline);
void DoRegex()
{
  foreach (string item in data)
  {
    Match match = regex.Match(item);
    if (match.Success)
    {
      string featureName = match.Groups["name"].Value;
      int index = int.Parse(match.Groups["index"].Value);
      testResult += index ^ featureName.GetHashCode();
    }
    else
    {
      throw new InvalidOperationException("DoRegex match");
    }
  }
}

void DoSplitString()
{
  foreach (string item in data)
  {
    string[] split = item.Split('[', ']');
    string featureName = split[0];
    if (int.TryParse(split[1], out int index))
      testResult += index ^ featureName.GetHashCode();
    else
      throw new InvalidOperationException("DoSplitString TryParse");
  }
}

void DoIndexOf()
{
  foreach (string item in data)
  {
    int endIndex = item.IndexOf('[');
    string featureName = item.Substring(0, endIndex);
    if (int.TryParse(item.Substring(endIndex + 1, item.Length - endIndex - 2), out int index))
      testResult += index ^ featureName.GetHashCode();
    else
      throw new InvalidOperationException("DoIndexOf TryParse");
  }
}

void DoStringBuilder()
{
  foreach (string item in data)
  {
    string featureName = "";
    int index = 0;
    char stop = '[';

    StringBuilder builder = new StringBuilder();
    foreach (char ch in item)
    {
      if (ch == stop)
      {
        if (stop == '[')
        {
          featureName = builder.ToString();
          stop = ']';
        }
        else
        {
          if (int.TryParse(builder.ToString(), out index))
            testResult += index ^ featureName.GetHashCode();
          else
            throw new InvalidOperationException("DoStringBuilder TryParse");
          break;
        }
        builder.Clear();
      }
      else
      {
        builder.Append(ch);
      }
    }
  }
}

I use a compiled instance for Regex, which reduces the duration for this test with about 600 ms compared to an uncompiled version - when it is compiled once and the same instance is reused for all tests. testResult serves as a simple validation.

The dataset is generated as (the data creation is not part of the measures):

const int testCount = 2000;
const int randSeed = 5;
Random rand = new Random(randSeed);
IList<string> data;
int testResult = 0;

IEnumerable<string> CreateData(int count)
{
  string chars = "abcdefghijklmnopqrstuwxyz";

  for (int i = 0; i < count; i++)
  {
    yield return $"{(string.Join("", Enumerable.Range(1, rand.Next(10, 30)).Select(n => chars[rand.Next(0, chars.Length)])))}[{rand.Next(0, 100)}]";
  }
}

public void Run()
{
  data = CreateData(1000).ToList();
  ...

I ran each method 2000 times in order to measure some realistic averages with the same dataset with 1000 strings for all measures. The result is rather disappointing for the Regex approach:

testResult: 2052366768
Result for test DoSplitString:
Iterations:                      2000
Average:                      0,34116 Milliseconds
Min:                          0,29660 Milliseconds
Max:                          1,17520 Milliseconds
Total:                      682,31420 Milliseconds

testResult: 2052366768
Result for test DoRegex:
Iterations:                      2000
Average:                      1,69160 Milliseconds
Min:                          1,48380 Milliseconds
Max:                          6,16170 Milliseconds
Total:                     3383,19930 Milliseconds

testResult: 2052366768
Result for test DoIndexOf:
Iterations:                      2000
Average:                      0,22634 Milliseconds
Min:                          0,19200 Milliseconds
Max:                          0,93010 Milliseconds
Total:                      452,68460 Milliseconds

testResult: 2052366768
Result for test DoStringBuilder:
Iterations:                      2000
Average:                      0,36898 Milliseconds
Min:                          0,33100 Milliseconds
Max:                          1,36560 Milliseconds
Total:                      737,96910 Milliseconds

IndexOf is by far the most efficient

As t3chb0t writes in his comments, the regex pattern can be optimized significantly to

const string pattern = @"^(?<name>.+)\[(?<index>.+)\]$";

static readonly Regex regex = new Regex(pattern, RegexOptions.Compiled);

Running the test with this pattern gives this result:

testResult: 2052366768
Result for test DoRegex:
Iterations:                      2000
Average:                      0,97967 Milliseconds
Truncated Average:            0,97777 Milliseconds
Min:                          0,84020 Milliseconds
Max:                          4,91550 Milliseconds
Total:                     1959,33570 Milliseconds


Dictionary Structure I am brand new to dictionaries and coding  and am hoping to get some suggestions on how I should structure my dictionary.

I have a collector number which has meters connected to them. Each of the meters have a meter number and a ert number and would like to store both the meter and ert number. There will be a list of meter reading timestamps and values for each of the meters. Also, I would like to be able to select the values by either the ert number or meter number.  I have attached the current structure I have for the dictionary but was wondering if this was the best way to format the dictionary. Someone had suggested objects but I have never worked with objects before.

{
Dictionary<Int32, Dictionary<string, Meter>> dictionary = new Dictionary<int, Dictionary<string, Meter>>();

if (!dictionary.ContainsKey(objectID))
{
    dictionary.Add(objectID, new Dictionary<string, Meter>());
}

Meter meter = new Meter();
meter.ertNumber = "9480:1";
meter.meterNumber = "fsadfa";
meter.Reading = new Dictionary<Int32, double>();
meter.Reading.Add(0, 6);

dictionary[objectID].Add("Test", meter);
}

class Meter
{
    public String ertNumber { get; set; }
    public String meterNumber { get; set; }
    public Dictionary<Int32, double> Reading { get; set; }
}

If you have a collection of Meters and you want to be able to find the ONE based on a property, this is how I would do it:

Create a List of Meters, say l..

List<Meter> l = new List<Meter>();

Then implement Extension methods like so..

public static class MeterExt
{
    public static Meter GetByErm(this List<Meter> l, String sErm)
    {
        Meter r = l.Find(s => s.ertNumber == sErm);

        return r;
    }

    public static Meter Get(this List<Meter> l, String sMeter)
    {
        Meter r = l.Find(s => s.meterNumber == sMeter);

        return r;
    }
}

Then you can find your meter like so..

Meter m = l.Get("fsadfa");


Excerpt from a brute force Rubik's Cube algorithm I have a program that brute force searches for a combination of inputted array elements by the user and when the program is searching all the for loops are nested for the brute search then I have this test() which looks is responsible for testing out all the possible combinations of the nested for loop to find the the combination entered by the user of the program, how ever the code is super redundant, here is a sample.

public static boolean test(int a, int b, int c, int d, int e, int f, int g, int h, int i, int j, int k){
    boolean result1 = true;

    if(a == 0){
        cube = turn_U(cube);
    }
    else if(a == 1){
        cube = turn_Ui(cube);
    }
    else if(a == 2){
        cube = turn_U2(cube);
    }
    else if(a == 3){
        cube = turn_L(cube);
    }
    else if(a == 4){
        cube = turn_Li(cube);
    }
    else if(a == 5){
        cube = turn_L2(cube);
    }
    else if(a == 6){
        cube = turn_F(cube);
    }
    else if(a == 7){
        cube = turn_Fi(cube);
    }
    else if(a == 8){
        cube = turn_F2(cube);
    }
    else if(a == 9){
        cube = turn_R(cube);
    }
    else if(a == 10){
        cube = turn_Ri(cube);
    }
    else if(a == 11){
        cube = turn_R2(cube);
    }
    else if(a == 12){
        cube = turn_B(cube);
    }
    else if(a == 13){
        cube = turn_Bi(cube);
    }
    else if(a == 14){
        cube = turn_B2(cube);
    }
    else if(a == 15){
        cube = turn_D(cube);
    }
    else if(a == 16){
        cube = turn_Di(cube);
    }
    else if(a == 17){
        cube = turn_D2(cube);
    }

This is what happens for letters a through k and I was wondering how I could shorten this, I had an idea where I could make a method that selects a turn method based on the letter in the test() , this would reduce so that test has 11 calls to the method instead of the all of this repeated ones.

when the program is searching all the for loops are nested for the brute search

That sounds like the first thing to fix. The brute force search should use recursion or an iterative approach which builds up a data structure (a stack or queue).

  then I have this test() which looks is responsible for testing out all the possible combinations of the nested for loop to find the the combination entered by the user of the program

Each of the six basic moves is a permutation. They could be represented as a two-dimensional array, and then a single method could apply an appropriate one by selecting the appropriate permutation from the array.

Moreover, the expanded moves are just iteration, and implementing them as iteration will give you much cleaner code.

public void applyMove(int move) {
    int iterations = (move % 3) + 1;
    int basicMove = move / 3;
    while (iterations-- > 0) {
        applyBasicMove(basicMove);
    }
}

public void applyBasicMove(int basicMove) {
    int[] permutation = basicPermutations[basicMove];
    ...
}

If you were worried about performance (which you clearly aren't, given that you're using a brute force search across a group of about \$4 \times 10^{19}\$ elements), you could optimise slightly by pre-computing the powers \$F^2\$ and \$F^3\$ (i.e. \$F^{-1}\$, or Fi if you prefer).


Python RESTful image handler This is a view as part of a RESTful image handler module, written for Flask, I am sure there is a better/more pythonic way of doing parts of this, maybe using List Comprehensions or Generator Functions. But I am a bit of a Python newbie and cannot spot it. Can any experienced pythonistas take a look?

@__mod__.route('/', methods=['GET'])
@__mod__.route('/<filename>', methods=['GET'])
def get(filename=None):
    """ Get a list of all Images or a single Image """

    couchdb.init_app(current_app)
    image = Image()

    if filename is None:
        results = image.view('images/all')
        filenames = []
        for result in results.all():

            if result._attachments:
                for attachment in result._attachments.keys():
                    filenames.append(url_for('.get', filename=attachment))
        response = make_response(simplejson.dumps(filenames), 200)
        response.headers['Content-Type'] = 'application/json'
        return response
    else:
        results = image.view('images/all', key=filename)

        if results.first() is not None:
            try:
                image_file = image.get_db().fetch_attachment(results.first(), filename)
                image_doc = results.first()._attachments[filename]
                response = make_response(image_file, 200)
                response.headers['Content-Type'] = image_doc['content_type']
                return response
            except ResourceNotFound:
                abort(404)

        else:
            abort(404)

I've managed this on my own, using a list comprehension instead of the ugly for-if structures:

if filename is None:
    results = image.view('images/all')
    filenames = [url_for('.get', filename=attachment)
                 for result in results
                 if result._attachments
                 for attachment in result._attachments]
    response = make_response(simplejson.dumps(filenames), 200)
    response.headers['Content-Type'] = 'application/json'
    return response
else:
    results = image.view('images/all', key=filename)

    if results.first():
        try:
            image_file = image.get_db().fetch_attachment(results.first(), filename)
            image_doc = results.first()._attachments[filename]
            response = make_response(image_file, 200)
            response.headers['Content-Type'] = image_doc['content_type']
            return response
        except ResourceNotFound:
            abort(404)
    else:
        abort(404)

@__mod__.route('/', methods=['GET'])
@__mod__.route('/<filename>', methods=['GET'])

Seeing as you end up handling these two routes in completely different ways, why don't you just implement two functions?

def get(filename=None):
    """ Get a list of all Images or a single Image """

    couchdb.init_app(current_app)
    image = Image()

I don't know what these class does, but I suspect its not really an Image given the methods called on it. It seems more like an image library or something.

    if filename is None:
        results = image.view('images/all')
        filenames = []
        for result in results.all():

            if result._attachments:

Attributes starting with underscores are usually intended to be private. You usually shouldn't be accessing them.

                for attachment in result._attachments.keys():

If you are checking for an empty _attachments above (as opposed to None), you don't need to.  The loop will execute zero times for an empty dictionary. Also, you don't need keys() because the default iteration for a dictionary is keys

                    filenames.append(url_for('.get', filename=attachment))

You did post an edit with a comprehensionified version.

filenames = [url_for('.get', filename=attachment)
             for result in results
             if result._attachments
             for attachment in result._attachments]

The same issues exist here as with the explicit for loop. Its also a bit long and complex I might do the following

attachments = [result._attachments for result in results if result._attachments]
attachments = [attachment for attachment for attachments in attachments]
attachments = [url_for('.get', filename = attachment) for attachment in attachments]

Better? maybe. maybe not.

        response = make_response(simplejson.dumps(filenames), 200)
        response.headers['Content-Type'] = 'application/json'
        return response
    else:
        results = image.view('images/all', key=filename)

        if results.first() is not None:
            try:
                image_file = image.get_db().fetch_attachment(results.first(), filename)
                image_doc = results.first()._attachments[filename]
                response = make_response(image_file, 200)
                response.headers['Content-Type'] = image_doc['content_type']
                return response

I'd consider a utility function that calls make_response and sets a content_type.

            except ResourceNotFound:
                abort(404)

It is recommended that you put as little as possible in your try blocks. You only want to catch exceptions from where it is expected to be thrown. I'm not sure where you it is coming from in your code.

        else:
            abort(404)