Rock-Paper-Scissors game This is the first piece of code I wrote on my own.  I tried to use everything I know, like functions, loops and variables.  Is it good or could it be simpler?

Please be hard as you can be with me because I feel my programming knowledge is quite poor and I end up using Google for solving my problems.

    // RPS.cpp : Defines the entry point for the console application.
//

#include "stdafx.h"
#include <iostream>
#include <ctime>
#include <cstdlib>

using namespace std;

int nPlayerScore=0;
int nComputerScore=0;
int nPlayerMove=0;
int nComputerMove=0;

void DisplayScore()
{
    cout << "Player: " << nPlayerScore << endl;
    cout << "Computer: " << nComputerScore << endl << endl;
}

int GetMove()
{
    cout << "Please choose a move: \n1.Rock\n2.Paper\n3.Scissors\n";
    int x;
    cin >> x;
    cout << endl;
    return x;
}

void CalculateResults()
{
    if (nPlayerMove==1) //player is Rock
    {
        if (nComputerMove==1) {cout << "player Rock vs computer Rock: IT'S A TIE!";}
        if (nComputerMove==2) {cout << "player Rock vs computer Paper: COMPUTER WON!"; nComputerScore=nComputerScore+1;}
        if (nComputerMove==3) {cout << "player Rock vs computer Scissors: PLAYER WON!"; nPlayerScore=nPlayerScore+1;}
    }
    if (nPlayerMove==2) //player is Paper
    {
        if (nComputerMove==1) {cout << "player Paper vs computer Rock: PLAYER WON!"; nPlayerScore=nPlayerScore+1;}
        if (nComputerMove==2) {cout << "player Paper vs computer Paper: IT'S A TIE!";}
        if (nComputerMove==3) {cout << "player Paper vs computer Scissors: COMPUTER WON!"; nComputerScore=nComputerScore+1;}
    }
    if (nPlayerMove==3) //player is Scissors
    {
        if (nComputerMove==1) {cout << "player Scissors vs computer Rock: COMPUTER WON!"; nComputerScore=nComputerScore+1;}
        if (nComputerMove==2) {cout << "player Scissors vs computer Paper: PLAYER WON!"; nPlayerScore=nPlayerScore+1;}
        if (nComputerMove==3) {cout << "player Scissors vs computer Scissors: IT'S A TIE!";}
    }

cout << endl << endl;
}

int main()
{
    do
    {
    cout << "Welcome to RPS 1.0a \tby GeTAFIX \n\n";
    DisplayScore();
    nPlayerMove=GetMove();
    srand((unsigned)time(0));
    nComputerMove=rand()%(3)+1;
    CalculateResults();
    //cout << "Press any key to continue...";
    system("pause");
    system("cls");
    } while (1==1);
}

First of all, stop using system("pause"); and system("cls");.  Both of those are not portable, and are silly ways of achieving what you want.  Take a look here for an actual solution.

Secondly, you are seeding the random number generator every iteration.  This is not only unnecessary, it is harmful: if you do this in a program that calls rand more than once a second, you will get the same result each call.  Even if you are only accessing it once a second (as you may be here), it is bad practice in any case, and can lead to a decrease in randomness.

Thirdly, I think you're getting a little far with splitting things into functions.  It might make sense to split the move-reading part into a separate function if you're going to perform input validation, but I think it currently just complicates matters.

Your CalculateResults function is overcomplicated.  If 0 is rock, 1 is paper, and 2 is scissors, and x and y are the choices of the players, you can tell who won with (3+x-y)%3.  If the result is 1 then x has won; if it is 2, then y has won; otherwise (if it is 0) there has been a tie.

Further stylistic issues: you should fix your indentation, turn the do ... while loop into a while(true) or for(;;) loop, stop using using namespace std;, and disable precompiled headers (they offer no benefit for a single-file solution).  You should also output '\n' instead of std::endl unless you explicitly want to flush the stream; an expression like std::cout << std::endl << std::endl simply doesn't make sense.

By the way, if you're using Google to solve your C++ problems, you're likely picking up a lot of information that's very wrong.  Make sure you have a good book to learn from, such as Accelerated C++.

Your code could be simplified to this, although the behaviour isn't quite the same:

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

std::string numberToWeapon(int n) {
    switch (n) {
       case 0: return "rock";
       case 1: return "paper";
       case 2: return "scissors";
    }
    assert(!"Invalid parameter");
}

int main() {
    std::cout << "Rock-Paper-Scissors v1.0\n";

    int playerScore = 0;
    int ourScore = 0;
    std::srand(static_cast<unsigned int>(time(0)));

    while (true) {
         int playerMove;
         std::cout << "Please enter 1 for rock, 2 for paper, or 3 for scissors.\n";
         if (!(std::cin >> playerMove))
             return 0;
         --playerMove;
         if (playerMove < 0 || playerMove > 2) {
             std::cout << "Invalid input.\n";
             continue;
         }

         int ourMove = rand()%3;

         std::cout << "You played: " << numberToWeapon(playerMove) << ". ";
         std::cout << "We played: " << numberToWeapon(ourMove) << ".\n";
         switch ((3 + playerMove - ourMove) % 3) {
             case 0:
                 std::cout << "The game is a tie.\n";
                 break;
             case 1:
                 std::cout << "You won!\n";
                 ++playerScore;
                 break;
             case 2:
                 std::cout << "We won!\n";
                 ++ourScore;
                 break;
         }

         std::cout << "The score is currently:\n";
         std::cout << "You - " << playerScore << ".\n";
         std::cout << "We - " << ourScore << ".\n";
    }
}

This results in a larger main function, but no global variables.  You can argue on whether that's any better or not: personally, I think it's clear as it is, and I don't see any duplication, but you may want to experiment.

If you are using C++11, you can get even more awesome syntax than the numberToWeapon function, namely:

std::array<4, std::string> weapons = {"rock", "paper", "scissors"};

And then use weapons.at(num) (or weapons[num] if you're sure you're in-bounds).


Text interface for employee management in Rust I'm in the process of reading The Rust Programming Language, and decided to do the 3rd task at the end of the 8th chapter.
The description is as follows:

  Using a hash map and vectors, create a text interface to allow a user to add employee names to a department in a company. For example, “Add Sally to Engineering” or “Add Amir to Sales.” Then let the user retrieve a list of all people in a department or all people in the company by department, sorted alphabetically.

Please tell me if my code is any good.

use std::collections::HashMap;
use std::io;
use std::io::Write;

use regex::Regex;

fn main() {
    let add_regex = Regex::new(r"^(?i)Add\s(\w(?:\w|\s)*\w?)\sto\s(\w+)$").unwrap();
    let list_regex = Regex::new(r"^(?i)List\s(\w+)$").unwrap();
    let all_regex = Regex::new(r"^(?i)Show all$").unwrap();
    let mut departments: HashMap<String, Vec<String>> = HashMap::new();

    loop {
        let mut input = String::new();

        print!("Enter command: ");
        io::stdout().flush();
        io::stdin().read_line(&mut input);

        input = String::from(input.trim());

        let add_match = add_regex.captures(&input);
        let list_match = list_regex.captures(&input);
        let all_match = all_regex.captures(&input);

        if add_match.is_some() {
            let unwrapped_match = add_match.unwrap();
            let employee = &unwrapped_match[1];
            let dpt = &unwrapped_match[2];

            match departments.get_mut(dpt) {
                Some(v) => {
                    v.push(String::from(employee));
                    v.sort();
                },
                None => {
                    let emp_list = vec![String::from(employee)];
                    departments.insert(String::from(dpt), emp_list);
                }
            }
        } else if list_match.is_some() {
            let unwrapped_match = list_match.unwrap();
            let employees = departments.get(&unwrapped_match[1]);

            if !employees.is_some() {
                println!("No such department");
                continue;
            }

            println!("Department employees: {}", employees.unwrap().join(", "));
            io::stdout().flush();
        } else if all_match.is_some() {
            for (dpt, employees) in &departments {
                println!("{}: {}", dpt, employees.join(", "));
            }
        } else {
            println!("Please enter a valid command");
        }
    }
}

Sample output:

Enter command: add f to ff
Enter command: add b to ff
Enter command: list ff
Department employees: b, f
Enter command: add Darth Vader to ff
Enter command: list ff
Department employees: Darth Vader, b, f
Enter command: add d to zz
Enter command: add 1 to zz
Enter command: show all
ff: Darth Vader, b, f
zz: 1, d

At a glance: that function is way too long and deeply nested. Three levels of loop/branch nesting is, in my experience, the most that any function should have; and the more nested it is the shorter it should be.

Consider extracting branch and loop bodies to separate named functions.

Your code structure also seems to be “determine what command to execute” (add / list / etc), then “execute said command”. I’d make this explicit, have the regexp parsing of an input line in a separate function that returns the command type and parameters. Dispatching this would be a natural use for match over your own Command type. (That said, in a realistic CLI you might need the commands available to be extensible so don’t get too married to this pattern. I’m guessing you’ll get to dynamic polymorphism later in the book.)


Decimal to binary and binary to decimal convertor Please review this code. How can I optimise this code?

#include <iostream>
#include <vector>
#include <algorithm> // std::reverse
#include <cmath> // pow()

typedef unsigned long long int ulli;

bool is_binary(ulli num)
{
    bool status = true;
    while (true)
    {
        if (num == 0)
        {
            break;
        }
        else
        {
            int temp = num % 10;
            if (temp > 1)
            {
                status = false;
                break;
            }
            num = num / 10;
        }
    }
    return status;
}

std::vector<ulli> decimal_to_binary(int val)
{
    std::vector<ulli> result;
    int rem;
    if (val == 0)
    {
        result.push_back(0);
        return result;
    }
    while (val != 0)
    {
        rem = val % 2;
        result.push_back(rem);
        val = val / 2;
    }
    std::reverse(result.begin(), result.end());
    return result;
}

int binary_to_decimal(ulli val)
{
    int sum = 0;
    int rem, i = 0;
    while (val > 0)
    {
        rem = val % 10;
        sum += pow(2, i) * rem;
        val = val / 10;
        i++;
    }
    return sum;
}

void display(std::vector<ulli>& vec)
{
    for (int i = 0; i < vec.size(); i++)
    {
        std::cout << vec[i];
    }
    std::cout << '\n';
}

int main()
{
    int option;
    std::cout << "1. Decimal to Binary \n2. Binary to Decimal \n";
    std::cout << "Enter your option\n";
    std::cin >> option;

    switch(option)
    {
        case 1:
          {
             int num;
             std::cout <<"\nEnter Decimal number\n";
             std::cin >> num;
             std::vector<ulli> binary = decimal_to_binary(num);
             std::cout << "The binary equivalent of " << num <<" is :\n";
             display(binary);
             break;
          }
        case 2:
          {
             ulli binary_num;
             x:
             std::cout << "\nEnter Binary number\n";
             std::cin >> binary_num;
             bool flag = is_binary(binary_num);
             if (!flag)
             {
                std::cout << "The number is not binary\n";
                goto x;
             }
             int decimal = binary_to_decimal(binary_num);
             std::cout << "The decimal equivalent of " << binary_num << " is :\n";
             std::cout << decimal << '\n';
             break;
          }
        default: std::cout << "Enter option 1 or 2\n";
    }
}

Validate User Input:

Always check user input is what you expect. The user may enter a valid number, or they may enter a number too long to be stored in the given type, or they might enter "fifty-three".

The code should handle these cases, rather than crashing, entering an infinite loop (option 2) or producing incorrect output (option 1). These are bugs in your program that need to be fixed. Check the answers here for ideas.

One way to do this is to loop and re-request data until the user enters an expected value. This pattern can be applied to significantly improve the overall structure of your program, e.g.:

while (true)
{
    auto conversionType = ask_conversion_type();

    if (conversionType == ConversionType::BinaryToDecimal)
    {
        // ... ask user for binary input with another loop
    }
    else if (conversionType == ConversionType::DecimalToBinary)
    {
        // ... ask user for decimal input with another loop
    }
    else
    {
        // error...
    }

    // maybe we also need an exit option?
}

Testing

Write unit tests, so you can check that the various components of the code work. In the simplest case, this can be done by adding a little function like so:

void test(bool condition)
{
    if (condition)
        std::cout << "pass" << std::endl;
    else
        std::cout << "FAIL" << std::endl;
}

and then writing some tests at the start of main() to make sure you get the output you expect for each function:

test(decimal_to_binary(0) == std::vector<ulli>{ 0 });
test(decimal_to_binary(2) == std::vector<ulli>{ 1, 0 });
//test(decimal_to_binary(-5) == um??? );

This will also help with thinking about edge cases!

Types

If each number in the vector will only store 0 or 1, maybe it doesn't need to be a vector<ulli>...

Context

What is the purpose of this code? If it's an excercise doing a particular thing (i.e. you have a special definition of a "binary number" you must adhere to, or you have to write your own functions), then you need to share that with us, lest it all be "optimised" away.

For example, we could just use std::bitset:

#include <bitset>
...

case 1:
{
    std::cout << "\nEnter Decimal number\n";
    unsigned long long num;
    std::cin >> num;
    // check input... then:
    std::bitset<64> bits(num);
    std::cout << "The binary equivalent of " << num << " is :\n" << bits << "\n";
    break;
}
case 2:
{
    std::cout << "\nEnter Binary number\n";
    std::bitset<64> bits;
    std::cin >> bits;
    // check input... then:
    std::cout << "The decimal equivalent of " << bits << " is :\n" << bits.to_ullong() << "\n";
    break;
}

The code is certainly shorter! But maybe doesn't fulfill some other requirements?


Image type convertion So I have a piece a code that, altough it does what is expected, it doesn't seam very performant to me. I just wanted to know if there's a better way to do this or if my code is alright as it is.

The objective of the code is to recieve an image path, check if the image exists or if it isn't png, if the image isn't png convert it to png and return a byte[] of the stream.

the code:

    // the speed of the method, on my computer, as is, right now, is of 40ms per cycle, this means 10 cycles are around 400ms
    public static byte[] GetBytesFromImage(string imageName)
    {
        byte[] bytes = null;

        string imagePath = "..\\..\\Imagens";
        imageName = imageName == "" ? "\\logotipos-prototipo_logo_prototipo_print.jpg" : imageName;
        string fpath = imagePath + imageName;

            if (File.Exists(fpath) && !imageName.EndsWith("png"))
            {
                using (MemoryStream image = new MemoryStream())
                using (System.Drawing.Image bmpImageToConvert = System.Drawing.Image.FromFile(fpath))
                using (System.Drawing.Image bmpNewImage = new Bitmap(bmpImageToConvert.Width, bmpImageToConvert.Height))
                using (Graphics gfxNewImage = Graphics.FromImage(bmpNewImage))
                {
                    gfxNewImage.DrawImage(bmpImageToConvert, new Rectangle(0, 0, bmpNewImage.Width, bmpNewImage.Height), 0, 0, bmpImageToConvert.Width, bmpImageToConvert.Height, GraphicsUnit.Pixel);

                    bmpNewImage.Save(image, ImageFormat.Png);
                    bytes = image.ToArray();
                }
            }

        return bytes;
    }

1st Update:
As the @Nkosi's answer mentioned, I forgot to make a case for when the image is PNG, and also to deal with the method returning a potencial null byte[].
Also in his change now the MemoryStream is shared in both cases, and the string comparison now ignores the string case.

    readonly string BASEIMAGEPATH = "..\\..\\Imagens\\";
    readonly string DEFAULTIMAGENAME = "logotipos-prototipo_logo_prototipo_print.jpg";

    // the speed of the method, on my computer, as is, right now, is of 40ms per cycle, this means 10 cycles are around 400ms
    public static byte[] GetBytesFromImage(string imageName)
    {
        byte[] bytes = null;
        imageName = imageName == "" ? DEFAULTIMAGENAME : imageName;
        string path = Path.Combine(BASEIMAGEPATH, imageName);
        if (File.Exists(path))
        {
            // Now both cases use the same MemoryStream
            using (MemoryStream image = new MemoryStream())
            {
                // The comparison now ignores case
                if (imageName.EndsWith("png", StringComparison.InvariantCultureIgnoreCase))
                {
                    using (FileStream imageStream = new FileStream(path, FileMode.Open, FileAccess.Read))
                    {
                        imageStream.CopyTo(image);
                        bytes = image.ToArray();
                    }
                }
                else
                {
                    using (System.Drawing.Image bmpImageToConvert = System.Drawing.Image.FromFile(path))
                    using (System.Drawing.Image bmpNewImage = new Bitmap(bmpImageToConvert.Width, bmpImageToConvert.Height))
                    using (Graphics gfxNewImage = Graphics.FromImage(bmpNewImage))
                    {
                        gfxNewImage.DrawImage(bmpImageToConvert, new Rectangle(0, 0, bmpNewImage.Width, bmpNewImage.Height), 0, 0, bmpImageToConvert.Width, bmpImageToConvert.Height, GraphicsUnit.Pixel);
                        bmpNewImage.Save(image, ImageFormat.Png);
                        bytes = image.ToArray();
                    }
                }
            }
        }
        // I changed his code here because `Array.Empty<byte>()` didn't exist.
        return bytes ?? new byte[0];
    }

There is a lot of unnecessary noise in the function.

If the base path is not expected to change it can be moved out into a constant.

System.IO.Path can be used to construct the path to the desired image.

To avoid null reference errors when consuming this function, try to avoid returning null. An empty array should be returned.

const string BASEIMAGEPATH = "..\\..\\Imagens";
const string DEFAULTIMAGENAME = "\\logotipos-prototipo_logo_prototipo_print.jpg";

public static byte[] GetBytesFromImage(string imageName) {
    byte[] bytes = null;
    imageName = imageName == "" ? DEFAULTIMAGENAME : imageName;
    string path = Path.Combine(BASEIMAGEPATH, imageName);
    if (File.Exists(path) && !imageName.EndsWith("png", StringComparison.InvariantCultureIgnoreCase)) {
        using (MemoryStream image = new MemoryStream())
        using (System.Drawing.Image bmpImageToConvert = System.Drawing.Image.FromFile(path))
        using (System.Drawing.Image bmpNewImage = new Bitmap(bmpImageToConvert.Width, bmpImageToConvert.Height))
        using (Graphics gfxNewImage = Graphics.FromImage(bmpNewImage)) {
            gfxNewImage.DrawImage(bmpImageToConvert, new Rectangle(0, 0, bmpNewImage.Width, bmpNewImage.Height), 0, 0, bmpImageToConvert.Width, bmpImageToConvert.Height, GraphicsUnit.Pixel);
            bmpNewImage.Save(image, ImageFormat.Png);
            bytes = image.ToArray();
        }
    }
    return bytes ?? Array.Empty<byte>();
}

The logic should also be reviewed as it was stated

  The objective of the code is to receive an image path, check if the image exists or if it isn't png, if the image isn't png convert it to png and return a byte[] of the stream.

but currently, if the image exists and is already a PNG, it wont return the contents of the file.

const string BASEIMAGEPATH = "..\\..\\Imagens";
const string DEFAULTIMAGENAME = "\\logotipos-prototipo_logo_prototipo_print.jpg";

public static byte[] GetBytesFromImage(string imageName) {
    byte[] bytes = null;
    imageName = imageName == "" ? DEFAULTIMAGENAME : imageName;
    string path = Path.Combine(BASEIMAGEPATH, imageName);
    if (File.Exists(path)) {
        using (MemoryStream image = new MemoryStream()) {
            if (imageName.EndsWith("png", StringComparison.InvariantCultureIgnoreCase)) {
                using (FileStream imageStream = new FileStream(path, FileMode.Open, FileAccess.Read)) {
                    imageStream.CopyTo(image);
                    bytes = image.ToArray();
                }
            } else {
                using (System.Drawing.Image bmpImageToConvert = System.Drawing.Image.FromFile(path))
                using (System.Drawing.Image bmpNewImage = new Bitmap(bmpImageToConvert.Width, bmpImageToConvert.Height))
                using (Graphics gfxNewImage = Graphics.FromImage(bmpNewImage)) {
                    gfxNewImage.DrawImage(bmpImageToConvert, new Rectangle(0, 0, bmpNewImage.Width, bmpNewImage.Height), 0, 0, bmpImageToConvert.Width, bmpImageToConvert.Height, GraphicsUnit.Pixel);
                    bmpNewImage.Save(image, ImageFormat.Png);
                    bytes = image.ToArray();
                }
            }
        }
    }
    return bytes ?? Array.Empty<byte>();
}


FIrst time trying tree classifier in python Recentely I have started teaching myself some basic AI concepts and this was my first attemp at using a random forest in order to classify the iris data set. It would be great if anyone could give me any sort of feedback on the code, specifically the style and readability. I apologize if the code block is formatted incorrectly (this is my first time using stack exchange). Thank you!

import numpy as np
from sklearn.model_selection import cross_validate
from sklearn.ensemble import ExtraTreesClassifier
from sklearn.metrics import classification_report
from sklearn.model_selection import train_test_split

# Data goes through preprocessing, Label encoding is used to map
# String identifiers to float

def data_processor(input_file):
    data = []
    with open(input_file, 'r') as f:
        for line in f:
            line = line.strip().split(',')
            if line[-1] == 'Iris-setosa':
                line[-1] = 0
            elif line[-1] == 'Iris-versicolor':
                line[-1] = 1
            elif line[-1] == 'Iris-virginica':
                line[-1] = 2
            data.append(line)

    return np.array(data, dtype=float)

# create array of data and array of index
x, y = data_processor('iris.txt')[:, :-1], data_processor('iris.txt')[:, -1]

# create training and test data set, 80% training 20% test
x_train, x_test, y_train, y_test = train_test_split(x, y, test_size=0.20,
                                                random_state=5)

# params used to create classifier
# number of trees - 150, node depth - 5, random seed
params = {'n_estimators': 150, 'max_depth': 5, 'random_state': 10}
classifier = ExtraTreesClassifier(**params)
classifier.fit(x_train, y_train)
# make prediction
y_test_pred = classifier.predict(x_test)

# output info on model performance
class_names = ['Setosa', 'Versicolor', 'Virginica']
print("Training dataset results\n")
print(classification_report(y_train, classifier.predict(x_train),
                            target_names=class_names))
print("Testing dataset results\n")
print(classification_report(y_test, y_test_pred, target_names=class_names))

First of all, pandas, scikit-learn, numpy, keras and tf come with a lot of data loading utilities.

Your data_processor(input_file) can be replaced with pandas.read_csv().
Alternatively, IRIS dataset is avaiable at sklearn.datasets.load_iris()

I would advise to have a look at Scikit-learn's example of analyzing IRIS


Alternate way for comparison call back function? I'm doing a programming practice problem and was wondering if someone can suggest a better to create/setup the call back for the comparison function. I have the following classes:

public class myTreeItem<T> {
    private T item;
    private myTreeItem<T> left;
    private myTreeItem<T> right;
    private myTreeItem<T> parent;
}

I have the usual get/set functions. This item go into a tree, whose definition is:

public class myTree<T> {
    myTreeItem<T> root;
    protected comparableCallBack<T> callback; // call back function for comparisons, to be provided by the creator of the tree

    public interface comparableCallBack<T> {
        int onCompare(myTreeItem<T> item1, myTreeItem<T> item2);
    }
}

I omitted all the other functions to help with readability. I am creating the tree in the following way:

    // create a regular tree
    myTree<Integer> tree1 = new myTree<Integer>();
    tree1.setCallback(new comparableCallBack<Integer>() {
        public int onCompare(myTreeItem<Integer> item1,
                myTreeItem<Integer> item2) {
            // Do the actual comparison here.
            if (item1.getItem().intValue() < item2.getItem().intValue())
                return -1;
            else if (item1.getItem().intValue() == item2.getItem().intValue())
                return 0;
            else
                return 1;
            }
    });

Are there any other options/ideas for the call back?

Comparator

There is no need for your interface.... what's wrong with Comparator? Why do you need to have your own?

The reason you appear to have it is because you need to compare the myTreeItem against another myTreeItem and the generic type is not right for you, but you can solve it easily with wrapping the Comparator in your myTree class when you need to....

So, your code has:

tree1.setCallback(new comparableCallBack<Integer>() {
        public int onCompare(myTreeItem<Integer> item1,
                myTreeItem<Integer> item2) {
            .......
        }
    });

but it should instead have:

tree1.setCallback(new Comparator<Integer>() {
        public int compare(Integer item1, Integer item2) {
            .......
        }
    });

and then, your library should have it's own private Comparator that looks something like:

class TreeComparator <T> implements Comparator<myTreeItem<T>> {

     private final Comparator<T> delegate;
     TreeComparator(Comparator<T> delegate) {
         this.delegate = delegate;
     }
     public int compare(myTreeItem<T> a, myTreeItem<T> b) {
         return delegate.compare(a.getItem(), b.getItem());
     }
}

Code Style

Read the Java Code-style guidelines.

Java class names should start with a capital letter.


Globally accessible helper class I would like to get opinion if I am doing this right. I am creating a helper class that is globally accessible to my whole project. I want it to be easily called like Facebook.loginUser()

My code is like this:

class Facebook: NSObject {
  class func LoginUser() {
    println("Login user...")
  }
}

And on usage just call the class name and method:

Facebook.loginUser()

instead of:

var FB: Facebook = Facebook()
FB.loginUser()

This design pattern is known as a Singleton, and you can read more about them here.

It's super easy to set up a singleton in Swift, and when I use this pattern I forward all method calls to the shared instance of the class.

class Singleton {
    static let sharedInstance = Singleton()

    // type method (aka class method), allows for easy-access code
    class func doSomething() {
        // forward the method and any parameters to the shared instance
        sharedInstance.doSomething()
    }

    // instance method, visible through Singleton.sharedInstance
    func doSomething() {
        println("Did something.")
    }
}

And the usage would be like so:

Singleton.doSomething()
Singleton.sharedInstance.doSomething()

(Both call the same method, just the first one is more concise)

Notes:

Apple is calling class methods in swift type methods
I use camelCaseForMethodNames because it is a convention (instead of CapitalizingEveryWord like class names)
People tend to have mixed opinions about Singletons (read more about that here, if you desire).


Autoloader functions Every time I write a new PHP page, I usually need to include this at the top:

<?php

    require_once(__DIR__ . '/../libraries/global.lib.php');

    function load_classes($class) { // appears to get all the names of the classes that are needed in this script...
        $file_name = './classes/' . $class . '.class.php';
        if (file_exists($file_name)) {
            require_once($file_name);
        }
    }

    function load_interfaces($interface) {
        $file_name = './classes/' . $interface . '.interface.php';
        if (file_exists($file_name)) {
            require_once($file_name);
        }
    }

    spl_autoload_register('load_interfaces');
    spl_autoload_register('load_classes');

?>

Is there any way to condense this? Would putting this in a separate PHP file work?

Yes, you could put that into a separate file and include_once('header_file.php');.

You try something such as:

function loadFile($name, $isInterface = false) {
    $type = ($isInterface == true) ? 'interface' : 'class'
    $path = sprintf('./classes/%s.%s.php',$name,$type);
    if (file_exists($path)) {
        require_once($path);
    }
}


Injecting the Repository into the business layer I am developing an application using Asp.NET core with MVC framework. I would like to ask your opinion about whether I inject my repository in a good way into my business layer.

First of all, lets see my IRepository interface:

public interface IRepository<T>
{
    IEnumerable<T> GetAll();
    T Get(long id);
    void Insert(T entity);
    void Update(T entity);
    void Delete(T entity);
}

Lets see one of my service class:

public interface IContainersGridService
{
    IList<ContainerGridViewModel> GetGridViewModels(string searchText);
}

public class ContainersGridService : IContainersGridService
{
    private readonly IRepository<Container> _containersRepository; //TODO: when write test, test when throws same db exception
    private readonly IContainerGridViewModelsGetter _containerGridViewModelsGetter;

    public ContainersGridService(IRepository<Container> containersRepository, IContainerGridViewModelsGetter containerGridViewModelsGetter)
    {
        _containersRepository = containersRepository;
        _containerGridViewModelsGetter = containerGridViewModelsGetter;
    }

    public IList<ContainerGridViewModel> GetGridViewModels(string searchText)
    {
        var containers = _containersRepository.GetAll();
        return _containerGridViewModelsGetter.Get(containers, searchText);
    }
}

As you can see I have an IRepository interface with the basic CRUD method signatures. I also have a ContainersGridService which is responsible to get some containers (domain: dump containers) from the database.

Furthermore, I have an ContainersGridService injected which is responsible for mapping and sorting the data coming from the database:

public interface IContainerGridViewModelsGetter
{
    IList<ContainerGridViewModel> Get(IEnumerable<Container> containers, string searchText);
}

public class ContainerGridViewModelsGetter : IContainerGridViewModelsGetter
{
    private readonly IContainerMappingService<ContainerGridViewModel> _containerMappingService;
    private readonly IContainersGridGlobalSearcher _containersGridGlobalSearcher;

    public ContainerGridViewModelsGetter(IContainerMappingService<ContainerGridViewModel> containerContainerMappingService, IContainersGridGlobalSearcher containersGridGlobalSearcher)
    {
        _containerMappingService = containerContainerMappingService;
        _containersGridGlobalSearcher = containersGridGlobalSearcher;
    }

    public IList<ContainerGridViewModel> Get(IEnumerable<Container> containers, string searchText)
    {
        var containerGridViewModels = GetContainerGridViewModels(GetSortedContainers(containers));

        return _containersGridGlobalSearcher.Search(containerGridViewModels,searchText);
    }

    private List<ContainerGridViewModel> GetContainerGridViewModels(IEnumerable<Container> sortedContainers)
    {
        return sortedContainers
            .Select(c => _containerMappingService.Map(c))
            .ToList();
    }

    private IEnumerable<Container> GetSortedContainers(IEnumerable<Container> containers)
    {
        return containers.OrderBy(c => c.DesiredTakeUpDate);
    }
}

As you can see, I have injected two additional (one mapping and one searching) logic class here. At this point I am wondering if it is the best way to implement my business layer like this, with the repository.

First I wanted to inject my repository into the ContainerGridViewModelsGetter but later I realized that it is (maybe) better if I separate the my repository from this logic class and I only pass an IList to my ContainerGridViewModelsGetter.Get(...) method. Using this way, I separated the data from this class, so the corresponding tests are also cleaner and clearer.

My questions are the following: Is it a good way to inject my repository to my service layer? Or maybe I would have injected the IRepository into my ContainerGridViewModelsGetter? Maybe do you have any other remarks regarding my code generally?

Injecting Repositories or anything else into the business layer is an overall bad idea. You business layer should be free from framework mechanisms. You will pollute your most important parts with unneccessary complexity.

What you can do is to place DI in certain objects that will initialize the dependencies around your business layer. So you may use a "RepositoryProvider" as an DI dependent artefact that will initialize a "RepositoryRegistry" which is DI independent and is used by the business layer.

Keep your your business layer clean.


Converting std::string to int without Boost I'm trying to convert an std::string to an int with a default value if the conversion fails. C's atoi() is way too forgiving and I've found that boost::lexical_cast is quite slow in the case where the cast fails.  I imagine it's because an exception is raised. I don't have C++11, so stoi() is out.

The Delphi function StrToIntDef is the ideal, and it's also available in C-Builder (where I'm working currently).  But I want something more portable that works with std::string.

I've created the following function, which uses atoi() but first tests for error conditions. It also allows for leading and trailing spaces, which are harmless in my situation.

int stringtoIntDef(const std::string & sValue, const int & DefaultValue) {
// convert a std::string to integer with a default value returned
// in the case where the string doesn't represent a valid integer
// - accepts leading or trailing spaces as valid

   bool hasDigits = false;
   bool TrailingSpace = false;
   for (std::string::size_type k = 0; k < sValue.size(); ++k) {
      if ((sValue[k] == ' ') || (sValue[k] == '\t')) {
         TrailingSpace = hasDigits;
      } else if ((sValue[k] == '0') || (sValue[k] == '1') || (sValue[k] == '2') || (sValue[k] == '3') ||
                  (sValue[k] == '4') || (sValue[k] == '5') || (sValue[k] == '6') || (sValue[k] == '7') ||
                  (sValue[k] == '8') || (sValue[k] == '9')) {
         if (TrailingSpace) {
            return DefaultValue;
         } else {
            hasDigits = true;
         }
      } else if ((sValue[k] == '-') && !hasDigits) {
         hasDigits = true; // this protects against "--"
      } else {
         return DefaultValue;
      }
   }
   return atoi(sValue.c_str());
}

In my testing, I've compared it against StrToIntDef and it is just as fast, but lexical_cast is much slower in the case where the default is returned. For 1000 iterations, lexical_cast took 5 seconds while the other 2 weren't measurable.

For my lexical_cast function, I've used this:

try {
    return boost::lexical_cast<int>(sValue);
}
catch (boost::bad_lexical_cast &) {
    return DefaultValue;
}

Are there any gotchas I might have missed here?

There's a few things that jump out at me both in your implementation and standard options you missed. A few (scattered) thoughts follow.

Returning a default if you care about error checking doesn't make sense. How would you determine if conversion failed if there's not an integer available for use as a sentinel? You are basically doing the same thing atoi does except your default is flexible instead of 0.

Have you considered [strtol][1]? It's basically atoi with error checking capabilities. It's the fastest string to int converter you're likely going to get:

long string_to_long(const std::string& str, long default_value, int base = 10) {
    char* parse_end = NULL; //nullptr if C++11, though really you don't have to initialize this
                            //since strol is guaranteed to initialize it. I just like to avoid
                            //unitialized variables.
    long val = strtol(str.c_str(), &parse_end, base);
    if (parse_end == str.c_str() + str.size()) {
        return val;
    } else {
        return default_value;
    }
}

Note: strtol ignores leading whitespace. The function I've written above errors for trailing whitespace (or trailing anything). In other words, " 3" is fine, but "3 " would error.

The idiomatic way to do this in C++ would be to use a string stream:

int string_to_int(const std::string& str, int def)
{
    std::istringstream ss(str);
    int i;
    if (!(ss >> std::noskipws >> i)) {
        //Extracting an int failed
        return def;
    }
    char c;
    if (ss >> c) {
        //There was something after the int
        return def;
    }
    return i;
}

In terms of performance, strtol will likely be faster than this. This does have a useful advantage though that it can be made highly generic with simple templating (this is the essential idea behind boost::lexical_cast actually, but this doesn't use exceptions):

template<typename ResultType>
ResultType lexical_cast(const std::string& str, const ResultType& default_value = ResultType()) {
    std::istringstream ss(str);
    ResultType result;
    if (!(ss >> std::noskipws >> result)) {
        return default_value;
    }
    char c;
    if (ss >> c) {
        return default_value;
    }
    return result;
}

If you don't want to use exceptions, but you want to be able to detect errors, you're going to have to use a flag. You basically have two options of how to handle that. You can use a boolean flag and return the value, or you can use a boolean reference parameter and return the value. Which route you take really depends on how you want to use the code, but I would probably return the value. That lets you ignore errors if you want to, but it still doesn't force you to. I might do something like this:

template<typename ResultType>
ResultType lexical_cast(const std::string& str, bool& success) {
    std::istringstream ss(str);
    ResultType result;
    success = true;
    if (!(ss >> std::noskipws >> result)) {
        success = false;
    }
    char c;
    if (ss >> c) {
        success = false;
    }
    return result;
}

template<typename ResultType>
ResultType lexical_cast(const std::string& str) {
    bool ignore;
    return lexical_cast<ResultType>(str, ignore);
}

This gives you both options of detecting an error or ignoring one. You could even build more on top of this to have one that returns a default value:

template<typename ResultType>
ResultType lexical_cast(const std::string& str, const ResultType& default_value = ResultType()) {
    bool success;
    ResultType val(lexical_cast<ResultType>(str, ignore));
    if (success) {
        return val;
    } else {
        return default_value;
    }
}

Don't check for classes of characters manually. Just use the cctype header's functions (isspace, isdigit, etc).

You loop is way over complicated:

bool all_digit = true;
for (std::string::size_type i = 0, l = str.size(); i < l; ++i) {
    if (!std::isdigit(str[i])) {
       all_digit = false;
        break;
    }
}

if (all_digit) { ... }

Or, allowing leading/trailing whitespace:

std::string::size_t i = 0;
const std::string::size_t len = str.size();
for (; i < len && std::isspace(str[i]); ++i) { /* empty */ }
for (; i < len && std::isdigit(str[i]); ++i) { /* empty */ }
for (; i < len && std::isspace(str[i]); ++i) { /* empty */ }
const bool all_digits = (i == len);


Function to prompt user for input The following program calls a function, which prompts the user for input.  The function then returns one of three enum constants; INPUT_SUCCESSFUL, INPUT_TOOLONG, or INPUT_FAILED.  If successful, it prints the text entered by the user.  Otherwise, it prints an error message, and takes the appropriate action.

The function contains three parameters.  The first one is a constant character array, which gets passed the text that will be used to prompt the user.  The second is a character array, in which the text entered by the user will be placed.  And the third is the size of the character array.

I have the following questions...

1) Would it have been better to let the called function handle the printing of error messages, exiting the program, etc, instead of the main function?

2) Would it have been better to pass a pointer to the function so that it could allocate the appropriate memory, instead of passing it a fixed array?

3) Any potential issues with the program or improvements that can be made to the code?

The program was compiled as follows...

gcc -std=c99 -Wall -Wextra -Wpedantic main.c user_input.c

...and received only warnings about unused parameters argc and argv.  Here are the files...

main.c

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include "user_input.h"

#define MAX_STR_LEN 50

int main(int argc, char *argv[])
{
    char user_input[MAX_STR_LEN + 2]; /* +2 for newline character (\n) and terminating null character (\0) */
    input_status status;

    while (1)
    {
        status = get_user_input("Enter your name: ", user_input, sizeof(user_input));

        if (status == INPUT_FAILED)
        {
            fprintf(stderr, "Unable to read user input\n");
            exit(EXIT_FAILURE);
        }

        if (status == INPUT_TOOLONG)
        {
            fprintf(stderr, "Entry exceeds %d characters\n", MAX_STR_LEN);
            continue;
        }

        if (strlen(user_input) == 0)
        {
            fprintf(stderr, "Entry is missing\n");
            continue;
        }

        if (user_input[0] == 0x18) /* user entered Ctrl+X (^x) to cancel */
        {
            fprintf(stderr, "User cancelled\n");
            return 0;
        }

        break;
    }

    printf("User entered:  %s\n", user_input);

    return 0;
}

user_input.h

#ifndef USER_INPUT_H
    #define USER_INPUT_H

    typedef enum
    {
        INPUT_SUCCESSFUL,
        INPUT_TOOLONG,
        INPUT_FAILED
    } input_status;

    void flush_input();

    input_status get_user_input(const char *prompt, char *user_input, size_t size);

#endif

user_input.c

#include <stdio.h>
#include <string.h>
#include "user_input.h"

void flush_input()
{
    int c;

    while ((c = getchar()) != '\n' && c != EOF)
        /* skip it */ ;
}

input_status get_user_input(const char *prompt, char *user_input, size_t size)
{
    printf("%s", prompt);

    if (fgets(user_input, size, stdin) == NULL)
        return INPUT_FAILED;

    char *pNewline;
    if ((pNewline = strchr(user_input, '\n')) == NULL)
    {
        flush_input();
        return INPUT_TOOLONG;
    }

    *pNewline = '\0';

    return INPUT_SUCCESSFUL;
}

It seems fairly sane; I only picked up a couple of things:

Don't ignore errno

You do check for failure, which is good; but you ignore errno. You should use it, and/or perror, to get more detailed failure information.

Assign-in-condition

There's not a compelling reason to do this:

char *pNewline;
if ((pNewline = strchr(user_input, '\n')) == NULL)

Just assign pNewLine where it's declared, not where it's used in the condition.


Project Euler #4: Finding the largest palindrome that is a product of two 3-digit numbers Amateur c++ programmer here, working my way through the Project Euler problems (problem 4). I would love for someone to take a look at my ~40 lines of code and give suggestions on improving the effectiveness of the code, and how/what to change to better follow the general guidelines for the flow of a c++ program.
The task states as such: "A palindromic number reads the same both ways. The largest palindrome made from the product of two 2-digit numbers is 9009 = 91 × 99.
Find the largest palindrome made from the product of two 3-digit numbers."
// Largest palindrome product (4)
#include <iostream>
#include <time.h>
bool is_palindrome(int &num) { // Determine if a given number is a palindrome or not
    int reverse=0, copy=num, digit;
    do {
        digit = copy % 10;
        reverse = reverse*10 + digit;
        copy /= 10;
    } while (copy != 0);
    return (reverse == num);
}
int main(void) {
    double time_spent = 0.0;
    clock_t begin = clock();

    int largest_palindrome = 0;
    for (int i=999; i>99; i--) {
        for (int j=999; j>99; j--) {
            if (i < largest_palindrome/1000) { // Optimalization
                // std::cout << "Any value lower than " << i << " will, when multiplied by 999, never exceed the largest palindrome (" << largest_palindrome << ")."<< '\n';
                std::cout << "No larger palindromes possible." << '\n';
                i = 0;
                j = 0;
            } else {
                int product = i*j;
                if (is_palindrome(product) && product>largest_palindrome) {
                    std::cout << "Found palindrome! " << i << " * " << j << " == " << product << '\n';
                    largest_palindrome = product;

                    // More optimalization (no product of the current iteration of i can be larger than the current palindrome, so skip to next iteration)
                    j = 0;
                }
            }
        }
    }

    // Program execution time
    clock_t end = clock();
    time_spent += (double)(end - begin) / CLOCKS_PER_SEC;
    std::cout << "Elapsed time is " << time_spent << " seconds." << std::endl;
    return 0;
}

Here are some suggestions in addition to the existing answers.
<chrono>
In general, the C++ std::chrono API is more flexible and
type-safe than the C functions in <ctime>, so consider using
std::chrono to time the function.
is_palindrome
In C++, it is usually not advised to declare multiple variables on one
line.  Taking small types like int by value is more efficient than
by reference and gives you a free copy to use.  To prevent errors, I
would also add an assertion to ensure that the input is a nonnegative
integer:
bool is_palindrome(int number) {
    assert(number >= 0);

    int original = number;
    int reversed = 0;

    while (number > 0) {
        reversed *= 10;
        reversed += number % 10;
        number /= 10;
    }

    return reversed == original;
}

Note that your version suffers from overflow problems for certain
large values of number, which this version does not fix.
Mathematical optimizations
By analyzing the problem mathematically, some optimizations can be
achieved.  The largest product of two 3-digit numbers is \$ 999 \times
999 = 998001 \$, so the answer will be at most a six-digit number.
For now, let's just assume that the answer is \$ \ge 900000 \$.  Thus,
the palindromes are restricted to the form \$ 9abba9 \$, where \$a\$
and \$b\$ are single-digit numbers.
By applying the divisibility rule for \$11\$, we see that \$
9abba9 \$ is a multiple of \$11\$.  Therefore, at least one of the
3-digit factors is a multiple of \$11\$ — we'll call this factor
the primary factor.  Since the product is an odd number, the primary
factor is odd as well, so we can start from \$979\$, the largest odd
3-digit multiple of \$11\$, and subtract \$22\$ at a time.  Our search
would stop if the primary factor becomes lower than \$900\$, since
\$899 \times 999 = 898101 < 900000\$, meaning our assumption would be
invalid.
Here's my result:
#include <algorithm>
#include <cassert>
#include <climits>
#include <iostream>

bool is_palindrome(int number) {
    assert(number >= 0);

    int original = number;
    int reversed = 0;

    while (number > 0) {
        reversed *= 10;
        reversed += number % 10;
        number /= 10;
    }

    return reversed == original;
}

int main() {
    int max_product = INT_MIN;

    for (int primary = 979; primary >= 900; primary -= 22) {
        for (int secondary = 999; secondary >= 900; secondary -= 2) {
            int product = primary * secondary;

            if (is_palindrome(product)) {
                max_product = std::max(product, max_product);
                break; // break out of the inner loop
            }
        }
    }

    if (max_product < 900000) {
        std::cout << "No result >= 900000\n";
    } else {
        std::cout << "Found: " << max_product << '\n';
    }
}

(wandbox)
The result is 906609, which is correct.


Name/word generator using DTMC in Ruby I just wrote a basic DTMC algorithm focused on generating names, though it could be used to generate lots of other things. It's designed to be run from the command line, so input is taken from a file, with a couple of options specified on the command line.

I'm specifically looking for these things, though any other advice is also of course welcome:

Ways to make it faster or more memory-efficient
Ways to make it more idiomatic for Ruby

I'm perfectly fine with changing the input format, though I'd prefer if it stayed the same. I'm also fine with making the code a bit more unreadable if it helps make it more efficient or shorter, as long as it's still somewhat understandable and easy to maintain.

This is my full code (version with -h help available here):

#Constants!
LINE_PART_DELIMITER = '|'
LINK_DELIMITER = ';'
LINK_HALF_MARK = ','
DEFAULT_NAME_COUNT = 10

# End with the standard error message format -- makes it easy to stay consistent
def error(code, message)
  puts "Error #{code}: #{message}"
  puts 'Run this script with -e to see a list of error codes.'
  abort "name_gen.rb: Error #{code}: #{message}"
end

# Keys are candidates, values are probabilities
def weighted_random_choice(picking_from)
  current = 0
  max = picking_from.values.inject :+
  r_val = rand max
  picking_from.each { |candidate, probability|
    current += probability
    return candidate if r_val < current
  }
  raise "r_val>max? #{r_val>max}. Error while picking a weighted random value from #{picking_from}"
end

#Parsing the commandline arguments and suchlike
syllable_separator = (/-d./ === ARGV[-1]) ? ARGV.pop[2..-1] : ''
name_count = (/\d+/ === ARGV[-1]) ? ARGV.pop.to_i : DEFAULT_NAME_COUNT
file = ARGV.join ' '

#Parsing the file
syllables = Hash.new false
start = Hash.new 0
begin
  IO.foreach(file) { |line|
    name, start_prob, end_prob, links = line.split LINE_PART_DELIMITER
    error 2, "`#{line}`" if links.nil? || end_prob.nil? || start_prob.nil? || name.nil?
    links = links.split(LINK_DELIMITER).inject(Hash.new 0) { |memo, current_pair|
      syl, prob = current_pair.split LINK_HALF_MARK
      (prob = Integer prob) rescue error 3, "`#{name}`: `#{syl}`, `#{prob}`"
      memo[syl.to_sym] += prob
      memo
    }
    links[false] = end_prob.to_i
    syllables[name.to_sym] = links
    start[name.to_sym] += start_prob.to_i
  }
rescue Exception => message
  puts message
  error 1, "`#{file}`"
end

#Validate that all syllables referenced actually exist!
syllables.each { |syllable, links|
  links.each { |link, _|
    error(4, "`#{link}` in `#{syllable}`") if !!link && syllables[link.to_sym].nil?
  }
}

#Generating and printing the names
name_count.times {
  current_syllable = weighted_random_choice start
  name_so_far = [current_syllable.to_s]
  while (current_syllable = weighted_random_choice syllables[current_syllable.to_sym])
    name_so_far << current_syllable.to_s
  end
  puts name_so_far.join syllable_separator
}

And this is an example 'dictionary' file:

a|1|1|b,2;c,2
b|0|3|a,0;c,2
c|0|0|a,1;b,1

And here's a sample of ten names it can generate (with the above dictionary file):

ab
acb
acabcacacb
abca
acacaca
abcacacb
acb
a
acbcacb
a

I don't like how you've mixed the concerns of calculating with output. I know that right now you only want to output to a file, but what if you decide later that you want to work with this data in some other program? Writing to the file system is expensive and slow. Why write to a file and then read it back in.

I would modify this to be in two parts. One to generate the names from the dictionary and one that uses that class to output to a file. This leaves things open to writing the output to Standard IO, some other UI, or for another program to simply work with the data. The idea is that each class should do one thing and do it well.

On this note, each of your comments indicates a missed opportunity to extract a well named method that does one and only one thing.

#Parsing the commandline arguments and suchlike
syllable_separator = (/-d./ === ARGV[-1]) ? ARGV.pop[2..-1] : ''
name_count = (/\d+/ === ARGV[-1]) ? ARGV.pop.to_i : DEFAULT_NAME_COUNT
file = ARGV.join ' '

This is just begging to be a method called parse_cmd_args. It should return some object that represents those three values in some sensible and well named way.

I apologize that I've left s critique without any code examples. Normally I would provide some, but it's been a while since I've written any Ruby. It's better in this case that I leave you to attempt a clean up yourself.

Try to:

Write methods that do one and only one thing.
Create useful abstractions by the way of classes, even if they're just simple data structures to hold information.
Add some vertical whitespace around logic that's too trivial or too intertwined to be its own method.


Simplified Caesars Cipher I'm studying functional programming, and I have the following problem to solve. It's an simplified version of Caesars Cipher:

You need to write a function, which will take string encoded with Caesar cipher as a parameter and decode it.
The one used here is ROT13 where the value of the letter is shifted by 13 places. e.g. 'A' ↔ 'N', 'T' ↔ 'G'.
You have to shift it back 13 positions, such that 'N' ↔ 'A'.

I made the following implementation:

const head = (str) => str[0];
const tail = (str) => str.slice(1);
const code = (str) => str.charCodeAt(0);

const rot13 = (str) => str ? (code(str) < 65 || code(str) > 90) ? head(str).concat(rot13(tail(str))) : String.fromCharCode((((code(str) - 65)) + 13) % 26 + 65).concat(rot13(tail(str))) : '';

It works fine, I only want to know if there is performance issues, and what can I do to improve my code?

I don't know about the performance, but readability-wise, cramming that double-nested ternary on a single line of code isn't, well, ideal.

Give it some [vertical] air:

const rot13 = (str) => str
   ? (code(str) < 65 || code(str) > 90)
       ? head(str).concat(rot13(tail(str)))
       : String.fromCharCode((((code(str) - 65)) + 13) % 26 + 65).concat(rot13(tail(str)))
   : '';

Ah, already better.

I'm no javascript expert, but being all inline, it probably performs better than if you further improved readability by introducing a function for each of the two outcomes of the inner condition... on the other hand, that could just as well be premature optimization - so I'd still go for readability and extract the logic into its own function.


Constructing file paths for each row of a datatable in C# I need to iterate through a DataTable and columns in each rows and do some data manipulation.

Below given is an extract from my code, I use c#.net. I expect maximum rows in datatable will be less than 25K.

How I can improve the code for better performance?

DataTable metadaDataTable = GetMetaDataTable();
foreach (DataRow row in metadaDataTable.Rows)
{
    string newfileName = string.Empty;
    foreach (DataColumn col in metadaDataTable.Columns)
    {
       if (col.ColumnName != "path_variable")
       {
           newfileName += row[col].ToString() + "_";
       }
     }
       newPath = CleanFileName(newfileName);
   }

One simple trick here would be to take the column processing outside of the row processing (the column metadata should not change while the rows probably will):

using (var metadaDataTable = GetMetaDataTable())
{
    var columns = metadaDataTable.Columns
        .Cast<DataColumn>()
        .Where(col => col.ColumnName != "path_variable")
        .ToList();
    var path_variable = metadaDataTable.Columns
        .Cast<DataColumn>()
        .SingleOrDefault(col => col.ColumnName == "path_variable");
    string path_variable_value;

    foreach (var row in metadaDataTable.Rows.Cast<DataRow>())
    {
        var newfileName = columns.Aggregate(string.Empty, (current, col) => current + (row[col] + "_"));

        newPath = CleanFileName(newfileName);
        path_variable_value = path_variable == null ? null : row[path_variable].ToString();
        // TODO: do something with the path_variable_value before continuing the loop.
    }
}

Note also I've sprinkled a liberal amount of LINQ in there to signify intent rather than mechanism.

Finally, Also surrounded the lifetime of metaDataTable with a using construct as DataTable implements the IDisposable interface.


Create a Tree Node from JavaScript Array I'm trying to create a list of subpaths from a flattened array.

Here's the starting point:

var flat = [
  {path: "/"  },
  {path: "A"  },
  {path: "A.A"},
  {path: "A.B"},
  {path: "A.C"},
  {path: "B"  },
  {path: "C"  }
]

The goal is that anything under a parent would be a subpath of that.  So the goal would look like this:

var goal = [
  {path: "/"  },
  {path: "A",
   subpaths: [
    {path: "A.A"},
    {path: "A.B"},
    {path: "A.C"}
   ]
  },
  {path: "B"  },
  {path: "C"  }
]

I've hobbled together a solution, but it doesn't feel very clean and is probably prone to breakage:

var nested = [];
for (var i=0; i < flat.length; i++) {
  // split components up
  var routes = flat[i].path.split(".");

  // get parent key
  var key = routes[0];

  // check if we've already added the key
  var index = -1;
  for (var j=0; j < nested.length; j++) {
    if (nested[j].path == key) {
      index = j;
      break;
    }
  }

  if (index===-1) {
    // if we have a new parent add it
    nested.push(flat[i])

  } else {
    // create subpaths property on new object
    if (!nested[index].subpaths) { nested[index].subpaths = [] }

    // add child paths to existing parent
    nested[index].subpaths.push(flat[i])
  }
}

Here's a working Demo in Stack Snippets

var flat = [
  {path: "/"  },
  {path: "A"  },
  {path: "A.A"},
  {path: "A.B"},
  {path: "A.C"},
  {path: "B"  },
  {path: "C"  }
]

var nested = [];
for (var i=0; i < flat.length; i++) {
  // split components up
  var routes = flat[i].path.split(".");

  // get parent key
  var key = routes[0];

  // check if we've already added the key
  var index = -1;
  for (var j=0; j < nested.length; j++) {
    if (nested[j].path == key) {
      index = j;
      break;
    }
  }

  if (index===-1) {
    // if we have a new parent add it
    nested.push(flat[i])

  } else {
    // create subpaths property on new object
    if (!nested[index].subpaths) { nested[index].subpaths = [] }

    // add child paths to existing parent
    nested[index].subpaths.push(flat[i])
  }
}

console.log(nested);

I'm also open to using jQuery or Underscore if they expose any functionality that would tidy up the source code. I'd also like to modify the code so it could handle this recursively with an unspecified level of depth on each node.

Your code seems to only support 2 levels down. Also, the second loop is costly because it has to search through the array if the path already exists.

I believe your structure can be better represented using an object. Lookups won't need searching through, and there's still room for path metadata.

var goal = {
  // Room for "goal" metadata.
  "/": {},
  "A": {
    // Room for "A" metadata.
    subpaths: {
      "A": {},
      "B": {},
      "C": {},
    }
  },
  "B": {},
  "C": {},
}

Here's an example

var paths = [
  {path: "/"  },
  {path: "A"  },
  {path: "A.A"},
  {path: "A.B"},
  {path: "A.C"},
  {path: "A.D.C"},
  {path: "B"  },
  {path: "C"  }
];

// Move out or template into a creator function.
function createPath(){
  return {
    subpaths: {}
  };
}

// Resolves the path into objects iteratively (but looks eerily like recursion).
function resolvePath(root, path){
  path.split('.').reduce(function(pathObject, pathName){
    // For each path name we come across, use the existing or create a subpath
    pathObject.subpaths[pathName] = pathObject.subpaths[pathName] || createPath();
    // Then return that subpath for the next operation
    return pathObject.subpaths[pathName];
  // Use the passed in base object to attach our resolutions
  }, root);
}

var goal = paths.reduce(function(carry, pathEntry){
  // On every path entry, resolve using the base object
  resolvePath(carry, pathEntry.path);
  // Return the base object for suceeding paths, or for our final value
  return carry;
// Create our base object
}, createPath());

document.write(JSON.stringify(goal));


Getting messages asynchronously using MailKit I'm writing a prototype for an application that uses MailKit and imap to connect to gmail. It's the first time I've used MailKit and I'm a bit stumped around the best way to do this.

Essentially, I want to write an async method hosted in a class library that downloads emails based on a simple header text search and invokes a callback each time so I can update the GUI. My attempt to do this is given in the following method.

public async Task<IList<IEmail>> EmailQueryAsync(string query, Action<IEmail> callBack)
{
    IList<IEmail> result = new List<IEmail>();

    using (ImapClient client = new ImapClient())
    {
        await client.ConnectAsync("imap.gmail.com", 993, true);

        client.AuthenticationMechanisms.Remove("XOAUTH2");

        await client.AuthenticateAsync(this.userName, this.password);

        IMailFolder folder = client.Inbox;

        await folder.OpenAsync(FolderAccess.ReadOnly);

        IList<UniqueId> uniqueIds = await client.Inbox.SearchAsync(SearchQuery.FromContains(query));

        IList<Task<MimeMessage>> messageTasks = uniqueIds.Select(m => client.Inbox.GetMessageAsync(m)).ToList();

        while (messageTasks.Count > 0)
        {
            Task<MimeMessage> messageTask = await Task.WhenAny(messageTasks);

            messageTasks.Remove(messageTask);

            MimeMessage message = await messageTask;

            IEmail email = new Email(date: message.Date, id: message.MessageId, messageBody: message.TextBody, references: message.References);

            callBack(email);

            result.Add(email);
        }

        return result;
    }
}

However, I feel there is a lot of code for what I want to do - do I really need to call so many async methods to get the messages, is it essential to have everything inside the using statement, or are there any obvious objects that I can cache. Is there a way that I can rewrite this method so I do not have to include the callBack action? Also, I'm a bit unsure about the types of exceptions I can sensibly expect to handle, timeouts etc...

It's hard to say how to write the ideal method for your use-case because you don't specify some of the details that would be necessary to know, such as:

How often will your code be making this query?

Is the connection always to the same IMAP server, username and password?

(if it'll be called fairly frequently with the same server/username/password combo, you could try to re-use the same ImapClient connection)

That said, I'll start with trying to at least clean up the code a bit.

public async Task<IList<IEmail>> EmailQueryAsync(string query, Action<IEmail> callBack)
{
    return Task.Factory.StartNew (() => {
        var result = new List<IEmail>();

        using (var client = new ImapClient ()) {
            client.Connect ("imap.gmail.com", 993, true);

            client.AuthenticationMechanisms.Remove ("XOAUTH2");

            client.Authenticate (this.userName, this.password);

            var folder = client.Inbox;

            folder.Open (FolderAccess.ReadOnly);

            var uids = client.Inbox.Search (SearchQuery.FromContains (query));

            foreach (var uid in uids) {
                var message = folder.GetMessage (uid);

                var email = new Email (date: message.Date, id: message.MessageId, messageBody: message.TextBody, references: message.References);

                callBack (email);

                result.Add (email);
            }

            return result;
        }
    }
}

I changed your code to put the entire block of logic into a single task which will all run on the same thread rather than your previous block which would effectively just run synchronously anyway since it didn't actually parallelize anything (it can't, really, since you can't search a folder until you've opened the folder - which you can't do until you've authenticated - which you can't do until you've connected, etc).

As far replacing your GetMessageAsync() logic goes, since the ImapClient can only process a single command at a time anyway, it doesn't make sense to spawn potentially thousands of command tasks asynchronously so that they can all just hurry up and wait for the previously queued tasks. Better to just write a synchronous loop in this scenario.

Update 2017-12-14: This will no longer be the case in MailKit 2.0. 2.0 will no longer wrap the synchronous calls using Task.Run() which means that they will no longer spawn threads.


Layout for a Tkinter form I am making a GUI application for a class. I have made the form layout (make_form()) using both Frame and grid, but I am not exactly sure that one should be mixing those two together. What is the best way to make a layout, using grid to incorporate not just the labels and Entry fields, but also the buttons?

import tkinter as tk

fields = "Dimensions", "Parent population size", "Children populatoin size", \
            "Recombination population size", "Maximum iterations", "Low boundary domain", \
            "High boundary domain", "Parameter size"

def quit():
    root.destroy()

def clear_initial_conditions(initial_conditions):
    for ic in initial_conditions:
        ic[1].delete(0, "end")

def get_initial_conditions(initial_conditions):
    for ic in initial_conditions:
        field = ic[0]
        text = ic[1].get()
        if text.isdigit():
            print("%s: %s" % (field, text))
        else:
            print("Field \"%s\" does not have a valid number, correct to integer" % field)

def make_form(root, fields):
    initial_conditions = []
    for field in fields:
        row = tk.Frame(root)
        row.pack(side = tk.TOP, fill = tk.X, padx = 10, pady = 10)
        lab = tk.Label(row, width = 30, text = field, anchor = tk.W)
        lab.grid(row = 0, column = 0)
        ent = tk.Entry(row)
        ent.grid(row = 0, column = 1, sticky = tk.W)
        initial_conditions.append((field, ent))
    return initial_conditions

if __name__ == "__main__":
    root = tk.Tk()
    root.option_add('*font', ('verdana', 8))
    root.title("Pack - Example 12")
    ents = make_form(root, fields)

    root.bind("<Return>", (lambda event, e = ents: get_initial_conditions(e)))
    b1 = tk.Button(root, text = "Find minimum", command = (lambda e = ents: get_initial_conditions(e)))
    b1.pack(side = tk.LEFT, padx = 10, pady = 10)
    b2 = tk.Button(root, text = "Clear", command = (lambda e = ents: clear_initial_conditions(e)))
    b2.pack(side = tk.LEFT, padx = 10, pady = 10)

    root.mainloop()

Frames are specifically designed to be a tool in organizing the widgets in your GUI.

In the case of a simple form like this, I recommend using one frame for the labels and entries, and one for the buttons. grid is the natural choice for the form, and it looks like pack would be best for the buttons. pack is also best to put the form on top and the buttons on the bottom.

For the form, I would not use a frame for each row.

Example:

# the root window and its children
form = tk.Frame(root)
buttons = tk.Frame(root)

form.pack(side="top", fill="both", expand=True)
buttons.pack(side="bottom", fill="x")

# the buttons
b1 = tk.Button(buttons, ...)
b2 = tk.Button(buttons, ...)

b1.pack(side="left", ...)
b2.pack(side="left", ...)

# the form
for row, field in enumerate(fields):
    lab = tk.Label(form, ...)
    ent = tk.Entry(form, ...)

    lab.grid(row=row, column=0, ...)
    ent.grid(row=row, column=1, ...)

The form can be thought of as a single complex widget. You can do that by creating a class for the form, based off of the Frame widget:

class Form(tk.Frame):
    def __init__(self, parent, fields):
        tk.Frame.__init__(self, parent)
        for row, field in enumerate(fields):
            ...

You would then use it like a normal frame when adding it to the window:

form = Form(root, fields=fields)
...
form.pack(side="top", fill="both", expand=True)

With that, you now have a general purpose form that could be reused for different purposes with different sets of fields.


JavaScript function to create 2 soccer teams as even as possible, from a list of players ? Given a list of players with their respective scores (how well does a player play soccer, from 1 to 5 points), return two teams that are as even as possible.

I've created this code, which is a first version, but I really would like to hear about ideas of how can I improve it.

exports.TeamCreator = (function () {

    this.players = [];

    function setPlayers(players){
        this.players = Object.assign([], players);
    }

    function createTeams() {

        var input = this.players;

        var teams = [],
            i = 0,
            scores = [],
            max = input.length;

        scores["A"]=0; scores["B"]=0;
        teams["A"]=[]; teams["B"]=[];

        input.sort(function(a, b){ return b.score - a.score;});

        var nextPlayer;
        var teamForNextPlayer;

        for (i = 0; i < max; i++){

            next();
            teams[teamForNextPlayer].push(nextPlayer);
            scores[teamForNextPlayer] += nextPlayer.score;
        }

        function getBest() {
            nextPlayer = input.splice(0, 1)[0]; //Best in list
        }
        function getWorst() {
            nextPlayer =  input.splice(input.length - 1, 1)[0]; //Worst in list
        }
        function next(){
            var counterA = teams["A"],
                counterB = teams["B"],
                scoreA = scores["A"],
                scoreB = scores["B"];

            if(counterA > counterB) { //B has less players than A
                teamForNextPlayer = "B";

                if(scoreB >= scoreA){
                    getWorst();
                }else{
                    getBest();
                }
            }
            else if(counterA < counterB) { //A has less players than B
                teamForNextPlayer = "A";
                if(scoreA >= scoreB){
                    getWorst();
                }else{
                    getBest();
                }
            }
            else //Same amount of players
            {
                getBest();
                teamForNextPlayer = (scoreA >= scoreB) ? "B" : "A";
            }
        }

        return {
            teamA: teams["A"],
            teamB: teams["B"],
            diff: Math.abs(scores["A"] - scores["B"]),
            scoreA: scores["A"],
            scoreB: scores["B"]
        };

    }
    return {
        setPlayers : setPlayers,
        createTeams: createTeams
    }
}());

If it's easier, you can pull the code from here.

First I would make each team an object with a score and players rather than an object for scores and one for players:

var teams = [ { name:'A'; score: 0, players:[] },
              { name:'B', score: 0, players:[] };

Once you have sorted the players by score it might be simpler to pick them off the list two at a time and assign the one with the higher score to the lower scoring team:

// Note: slice is probably better than Object.assign
let input = this.players.slice();
assert(input.length % 2 == 0, 'Need an equal number of players');

while (input.length > 0) {
  let [strongerTeam,   weakerTeam]   = teams;
  let [strongerPlayer, weakerPlayer] = input.slice(0,2);
  addPlayer(strongerTeam, weakerPlayer);
  addPlayer(weakerTeam,   strongerPlayer);
  sortTeams(teams);
}

function addPlayer(team, player) {
  team.players.push(player);
  team.score += player.score;
}

// Sort highest scoring team first
function sortTeams(teams) {
  teams.sort(function(a, b) { return b.score - a.score;});
}

return teams;

One advantage is this always returns the more powerful team first.

This algorithm is not ideal in that it doesn't always choose the most balanced team, for example given players with scores of 8, 4, 3, 3, 1 and 1, Team A will have players 8, 3 and 1 (score of 12) while team b will have players with scores of 4, 3 and 1 (totaling 8). It might be more reasonable to balance them as Team A: 8, 1, 1 and Team B 4, 3, 3 so that both teams have a score of 10. The logic for that might be:

while (input.length > 0) {
  let [strongerTeam,   weakerTeam]   = teams;
  let nextPlayer = input.slice(0,1)[0];
  addPlayer(weakerTeam,   nextPlayer);
  if (strongerTeam.length - weakerTeam.length == input.length) {
    addPlayers(strongerTeam, input);
    input = [];
  }

  sortTeams(teams);
}

function addPlayers(team, players) {
  for (let player of players) {
    addPlayer(team, player);
  }
}


Handling ajax responses in Rails I have the following method which creates a like for a post:

def create
  post.likes.create(user: current_user)
  respond_to do |format|
    format.html { redirect_to post_redirect(post) }
    format.js
  end
end

We don't really care whether the create was successful or not and either redirect the user to the post (the page where the method is called) or return some js if done via AJAX (usually).

The js partial looks like:

// create.js.erb
$("#content").html("<%= j render(:partial => 'likes/like_button') %>");

However I didn't like that it expects the AJAX request to request JS and then having a partial render inside a jQuery method that then makes an assumption about #content existing to render it inside. So effectively the response is also handling how it should be used.

My alternative approach was:

def create
  post.likes.create(user: current_user)
  request.xhr? ? (render :partial => 'likes/like_button') : (redirect_to post_redirect(post))
end

This way we just return the partial or redirect depending on if the request was via AJAX.

The former seems to be the standard Rails approach (according to most docs) but I find the latter far cleaner as it means I can handle how the response is handled as it just returns HTML rather than having the actual handling of the response being returned with the response.

Is there a better way to handle AJAX responses? Or reasons why my approach would be considered bad practice? Of course using the first approach means using Rails UJS and my own uses custom JS to make the request and handle the response.

However I didn't like that it expects the AJAX request to request JS and then having a partial render inside a jQuery method that then makes an assumption about #content existing to render it inside. So effectively the response is also handling how it should be used.

I actually think this is best practice for a non-SPA solution. It's a great approach because you will know how the page is structured. It works even if there's multiple posts because you can assign object IDs to the posts in the HTML, which then lets you target those objects in your JQuery selector.

So you'd be looking at something like:

$('.post[data-object-id=<%= post.id %> .likes').html('<%= j render(partial: 'likes/like_button', locals: {likes: post.likes}) %> ')

Now if you want to do something fancy with the HTML beyond just inserting it at the appropriate location, then you'd indeed make an AJAX request for the HTML, and attach a handler to the promise generated from the AJAX request which would handle the HTML response appropriately.


Python: Astar algorithm implementation I have implemented Astar algorithm for a problem on an online judge relating to maze given start and end positions along with a grid representing the maze. I output the length of the path along with the path itself. The following is the implementation in Python using the Euclidean distance:
import heapq, math, sys

infinity = float('inf')

class AStar():

    def __init__(self, start, grid, height, width):
        self.start, self.grid, self.height, self.width = start, grid, height, width

    class Node():
        def __init__(self, position, fscore=infinity, gscore=infinity, parent = None):
            self.fscore, self.gscore, self.position, self.parent = fscore, gscore, position, parent

        def __lt__(self, comparator):
            return self.fscore < comparator.fscore

    def heuristic(self, end, distance = "Euclidean"):
        (x1, y1), (x2, y2) = self.start, end
        if (distance == "Manhattan"):
            return abs(x1 - x2) + abs(y1 - y2)
        return math.sqrt((x2 - x1)**2 + (y2 - y1)**2)

    def nodeNeighbours(self, pos):
        (x, y) = pos
        return [(dx, dy) for (dx, dy) in [(x + 1, y), (x - 1, y), (x, y + 1), (x, y - 1)] if 0 <= dx < self.width and 0 <= dy < self.height and self.grid[dy][dx] == 0]

    def getPath(self, endPoint):
        current, path = endPoint, []
        while current.position != self.start:
            path.append(current.position)
            current = current.parent
        path.append(self.start)
        return list(reversed(path))

    def computePath(self, end):
        openList, closedList, nodeDict = [], [], {}
        currentNode = AStar.Node(self.start, fscore=self.heuristic(end), gscore = 0)
        heapq.heappush(openList, currentNode)
        while openList:
            currentNode = heapq.heappop(openList)
            if currentNode.position == end:
                return self.getPath(currentNode)
            else:
                closedList.append(currentNode)
                neighbours = []
                for toCheck in self.nodeNeighbours(currentNode.position):
                    if toCheck not in nodeDict.keys():
                        nodeDict[toCheck] = AStar.Node(toCheck)
                        neighbours.append(nodeDict[toCheck])

                for neighbour in neighbours:
                    newGscore = currentNode.gscore + 1
                    if neighbour in openList and newGscore < neighbour.gscore:
                        openList.remove(neighbour)
                    if newGscore < neighbour.gscore and neighbour in closedList:
                        closedList.remove(neighbour)
                    if neighbour not in openList and neighbour not in closedList:
                        neighbour.gscore = newGscore
                        neighbour.fscore = neighbour.gscore + self.heuristic(neighbour.position)
                        neighbour.parent = currentNode
                        heapq.heappush(openList, neighbour)
                    heapq.heapify(openList)
        return None

if __name__ == '__main__':

    sys.stdin = open('input.txt', 'r')
    sys.stdout = open('output.txt', 'w')

    matrix = [[int(num) for num in line.split()] for line in sys.stdin]
    size = matrix.pop(0)
    coordinates = matrix.pop(0)
    n, m = size[0], size[1]
    x1, y1, y2, x2 = coordinates[0], coordinates[1], coordinates[2], coordinates[3]
    path = AStar((x1-1, y1-1), matrix, n, m).computePath((y2-1, x2-1))
    print(len(path))
    for pos in path:
        print(pos[0] + 1, pos[1] + 1)

self.start, self.grid, self.height, self.width = start, grid, height, width

I would not put these all on the same line like that. I think it would be much easier to read spread over multiple lines:
self.start = start
self.grid = grid
self.height = height
self.width = width

I would probably have the Node class as toplevel instead of nested. I don't think you're gaining much by having it inside AStar. You could name it _Node to make it "module-private" so that attempting to import it to another file will potentially raise warnings.
In Node's __lt__ implementation, I wouldn't call the second parameter comparator. A comparator is something that compares, whereas in this case, that's just another node. other_node or something would be more appropriate.

In heuristic, I'd personally make use of an else there:
if (distance == "Manhattan"):
    return abs((x1 - x2) + abs(y1 - y2))
else:
    return math.sqrt((x2 - x1)**2 + (y2 - y1)**2)

It makes it clearer that only one of the lines will be executed. Personally, I only neglect the else in a case like that if the if was an "early exit" precondition check, and I want to avoid nesting the entire rest of the function inside a block. That's not a problem here though.

nodeNeighbors (which should be node_neighbors) would be cleaner broken over several lines:
def nodeNeighbours(self, pos):
    (x, y) = pos
    return [(dx, dy)
            for (dx, dy) in [(x + 1, y), (x - 1, y), (x, y + 1), (x, y - 1)]
            if 0 <= dx < self.width and 0 <= dy < self.height and self.grid[dy][dx] == 0]

I think that makes it a lot easier to see what's going on in it.

Again, in many places you're assigning two or more variables on one line:
(x1, y1), (x2, y2) = self.start, end
current, path = endPoint, []
openList, closedList, nodeDict = [], [], {}
x1, y1, y2, x2 = coordinates[0], coordinates[1], coordinates[2], coordinates[3]

I would break those up. Especially once you get to 3+ on a line, for the reader to see what variable matches up with what value, they'll need to count from the left instead of just checking what's on each side of a =.

In computePath, it seems like  closedList should be a set. It doesn't appear as though order matters with it, and neighbour in closedList will be faster with a set than it will with a list. It looks though like openList is required to be a list though due to it being passed to heapify.

I don't think I'd reassign stdin and stdout. The reassignment of stdin seems completely unnecessary, and changing stdout will make it harder to debug later using print statements. You don't necessarily want all printed text to be sent to the file.
If need-be, you can specify what file you want printed to when printing:
with open('output.txt', 'w') as out_f:
    print("To file!", file=out_f)


Caesar Cipher improvement Currently, I have this and it's working for all cases:

import math
import os
import random
import re
import sys

def caesarCipher(s, k):
    st = []
    for i in range(0,len(s)):
        if 65<=ord(s[i])<=90:
            temp = ord(s[i])+k%26
            if (temp>90):
                temp-=26
            st.append(chr(temp))
        elif 97<=ord(s[i])<=122:
            temp = ord(s[i])+k%26
            if (temp>122):
                temp-=26
            st.append(chr(temp))
        else: st.append(s[i])
    return ''.join(st)

if __name__ == '__main__':

    s = input()

    k = int(input())

    result = caesarCipher(s, k)

I'm not sure if the multiple if loops reduce readability. Is there a cleaner way to do this?

Sample I/O:

Input: s = middle-Outz , k=2
Output: s = okffng-Qwvb

Input: s = Always-Look-on-the-Bright-Side-of-Life ; k=5
Output: s = Fqbfdx-Qttp-ts-ymj-Gwnlmy-Xnij-tk-Qnkj

Review

Remove unused imports

You don't use any of these imports, just remove them to reduce clutter
Read PEP8 the python style guide

Functions and variables should be snake_case
There should be a <space> in between operators

Instead of appending to a list, you could create the string for the start

So instead of making st = [], do st = '' and st += char, no need to join then

However you could make the shifting a separate function, and then ''.join() becomes needed again
When looping, loop over the item not the index

Instead of for i in range(len(s)) you could directly loop over the char --> for i in s
There is a function to determine if we encounter an uppercase or lowercase

You could use char.isupper() or char.islower()
Simplify the shifting

You could use the fact that if you subtract the start ord('a') or ord('A') and then modulo, the extra check of if temp > x becomes not needed
Notice how the operations for each case (upper, lower) are mostly similar

Putting it all together

import string
APLHA_LEN = len(string.ascii_lowercase)

def shift_letter(letter, shift):
    start = ''
    if letter.isupper():
        start = ord('A')
    elif letter.islower():
        start = ord('a')
    return chr((ord(letter) - start + k) % APLHA_LEN + start) if start else letter

def ceasar_cipher(st, shift):
    return ''.join(shift_letter(letter, shift) for letter in st)

BUT, there is a better way...

As mentioned by someone already you could use the maketrans function very nicely for ceasar ciphers.

Giving you something like this

from string import ascii_lowercase as lc, ascii_uppercase as uc

def ceasar_cipher_translate(st, k):
    trans = str.maketrans(lc + uc, lc[k:] + lc[:k] + uc[k:] + uc[:k])
    return str.translate(st, trans)

NOTE That this code assumes Python3.x


Extracting keywords from 3 billion CSV records I am processing around 3 billion records using this piece of code. It's pretty slow; it would be really helpful if you could suggest better ways to do this.

'''
Created on 27-Mar-2015
@author: siddarth
'''

import rake
import multiprocessing as mp
import time
import os
import csv

filename = 'Large_Input_File.csv'
outName = 'Large_Output_File.csv'
BYTES_PER_MB = 1048576

start = time.time()
def elapsed():
    return time.time() - start

''' Worker function used to call rake
    This method basically calls the rake class which returns a
    list of keywords, also each process
    writes to a different output file'''

def fileparser_worker(filename, start, end, c):
    outFileName = outName + str(c)
    outFile = open(outFileName,'w')
    with open(filename) as inFile:
        inFile.seek(start)
        lines = inFile.readlines(end-start)
        for line in lines:
            title = line.split('\t')
            try:
                keywords = rake.execute_rake(title[2])
            except:
                print('Index out of bound error due to NULL Values')
                continue
            for keyword in range(0,len(keywords)):
                outFile.write(keywords[keyword][0] + '\n')
    outFile.close()

if __name__ == '__main__':
    start = time.time()
    chunk_start = 0
    chunk_size =  512 * BYTES_PER_MB
    chunk_end = 512 * BYTES_PER_MB

    filesize = os.path.getsize(filename)

    print '\n%.3fs: file has %s rows' % (elapsed(), filesize)

    c = 0
    pause = 0
    iterations = (filesize / chunk_size) + 1
    ''' Chunk the file '''
    print(iterations)
    with open(filename) as inFile:
        while c < iterations:
            if chunk_start + chunk_size > filesize:
                chunk_end = filesize
            else:
                chunk_end = chunk_start + chunk_size

            inFile.seek(chunk_end)
            line = inFile.readline()

            if line == '':
                continue
            else:
                chunk_end = inFile.tell()

            print("Start chunk",chunk_start)
            ''' Initializing Processes '''
            proc = mp.Process(target=fileparser_worker, args = (filename,chunk_start,chunk_end, c) )
            proc.start()
            pause += 1
            print("End chunk",chunk_end)
            chunk_start = chunk_end
            c += 1

            ''' Making sure that there are atmost 4 processes
                running at a single point of time,
                cause I have a 4 core machine'''

            if pause == 4:
                proc.join() # Waits for processes to close
                pause = 0

    proc.join() # Close off any running processes
    print("TOTAL TIME TAKEN",elapsed())

Below are some metrics which may help you to understand what's going on under the hood.

File Size = 18GB

Usage Metrics:
CPU Usage = 99% ( Using 4 Processes )
RAM Usage = 30% ( May be I can use more RAM to improve performance ? )

I am not familiar with multiprocessing. I've learned about it today and wrote some sample code based on what I understood.

def fileparser_worker(filename, start, end, c):
    with open(filename) as inFile, open(outName + str(c),'w') as outFile:
            inFile.seek(start)
            #lines = inFile.readlines(end-start)

because readlines calls readline multiple times it can be replaced with
read.

If the text should be split by newline and each split line should be splitted by a tab character, there are two ways to do it depending on the input text.

    for title in (line.split('\t') for line inFile.read(end-start).splitlines()):

Here title is list of strings.

Another one is to call str.split() or str.split(None) in which case the string is split by white spaces (space, tab or newline), this is best if it's ok to split by space character or the text has no space character.

    for title in inFile.read(end-start).split():

Here the title is string.

If the read().split() becomes a memory hog, I think a generator will give
better result.

    #def get_title():
    #   yield next((line.split('\t') for line inFile.read(end-start).splitlines()))

    for title in (line.split('\t') for line inFile.read(end-start).splitlines()):
        try:
            keywords = rake.execute_rake(title[2])
        except:
            print('Index out of bound error due to NULL Values')
            continue

If we could build a sequence of strings out of keywords we can call
writelines straight.

        else:
            outFile.writelines((keyword[0] + '\n' for keyword in keywords))

~


Computation of concordant and discordant pairs I would like to compute the number of concordant and discordant pairs of two large vectors. This function is a nice try, and works quite efficiently.

pairs <- function(x,y){
    n <- length(x)
    ix <- order(x)
    x <- x[ix]
    y <- y[ix]
    Nc <- sum(sapply(1:(n-1),function(i) sum(x[i]<x[(i+1):n] & y[i]<y[(i+1):n])))
    Nd <- sum(sapply(1:(n-1),function(i) sum(x[i]<x[(i+1):n] & y[i]>y[(i+1):n])))
    return(list(Nc=Nc,Nd=Nd))
}

x <- runif(10000)
y <- runif(10000)
system.time(pairs(x,y))

Do you have any idea on how it would be possible to boost the function?

These sorts of operations are tough to vectorize because you need to compare every element to all later elements. If the counting operation is a performance bottleneck in your code, then one possibility would be to implement it in C++ using the Rcpp package:

library(Rcpp)
cppFunction(
  "IntegerVector CDcount(NumericVector x, NumericVector y) {
    IntegerVector counts(2, 0);
    int n = x.size();
    for (int i=0; i < n; ++i) {
      for (int j=i+1; j < n; ++j) {
        counts[0] += (x[i] < x[j]) && (y[i] < y[j]);
        counts[1] += (x[i] < x[j]) && (y[i] > y[j]);
      }
    }
    return counts;
  }")

pairs2 <- function(x, y) {
  n <- length(x)
  ix <- order(x)
  x <- x[ix]
  y <- y[ix]
  counts <- CDcount(x, y)
  return(list(Nc=counts[1], Nd=counts[2]))
}

For your test data, this results in a 50 times speedup on my system.

x <- runif(10000)
y <- runif(10000)
system.time(pairs(x,y))
#    user  system elapsed
#   4.892   1.447   6.584
system.time(pairs2(x, y))
#    user  system elapsed
#   0.119   0.001   0.120
identical(pairs(x, y), pairs2(x, y))
# [1] TRUE

The Rcpp package takes some time to compile the CDcount function, so this option is probably only helpful if you are operating on very large vectors or if you are repeating the operation many times.


Program that prints out repetitions of 3 and 5 This works in my Eclipse, but I think there is a better way to do it.

public static void main(String args[]) {

       for(int i = 1; i < 101; i++)
       {
           if(i%3 == 0)
           {
           System.out.println("repetitive of 3");
           }

           else if(i%5 == 0)
           {
           System.out.println("repetitive of 5");
           }

           else{
               System.out.println(i);
           }

       }

   }

}

Moderation Note: This answer was being typed at the same time as the asker changed his code, to avoid this bug. Here's the code, as it was, when this answer was typed:

  Program that prints out repetitives of 3,5 and both of them

public static void main(String args[]) {

       for(int i = 1; i < 101; i++)
       {
           if(i%3 == 0)
           {
           System.out.println("repetitive of 3");
           }

           else if(i%5 == 0)
           {
           System.out.println("repetitive of 5");
           }

           else if(i%5 == 0 && i%3 == 0)
           {
           System.out.println("repetitive of 3 and 5");
           }
           else{
               System.out.println(i);
           }
       }
   }
}

First of please format your code properly as it helps to read it. I've formatted the code as it is suggested for java:

public static void main(String args[]) {
    for (int i = 1; i < 101; i++) {
        if (i%3 == 0) {
            System.out.println("repetitive of 3");
        } else if (i%5 == 0) {
            System.out.println("repetitive of 5");
        } else if (i%5 == 0 && i%3 == 0) {
            System.out.println("repetitive of 3 and 5");
        } else {
            System.out.println(i);
        }
    }
}

Now for the review:

The third if will never be reached as the first if already matches.

So to get the result wanted you have to change the first and third if branch:

public static void main(String args[]) {
    for (int i = 1; i < 101; i++) {
        if (i%5 == 0 && i%3 == 0) {
            System.out.println("repetitive of 3 and 5");
        } else if (i%5 == 0) {
            System.out.println("repetitive of 5");
        } else if (i%3 == 0) {
            System.out.println("repetitive of 3");
        } else {
            System.out.println(i);
        }
    }
}

Using the method main() for small tests is fine but it is better to create objects and use them from the beginning.

Else the program look good so far.


Zend Framework getAction method I am new to MVC and Zend, so I have little idea what I am doing at the moment, but here is my question.

The following code is my get method for an API I am attempting to build in order to learn both. It works as is, but given the sparsity of code I have seen in example methods, and the fact that Zend seems to do a lot for you, is there a better way of doing this? The code is something I cobbled together because the tutorial that I was walking through forgot to add the get method code.

public function getAction()
{
    // action body
    // Is there a better way to get the id from the URL?
    $this->_getAllParams();
    $request = $this->_request->getParams();

    if (array_key_exists($request['id'], $this->_todo)){
        $return[] = array($request['id'] => $this->_todo[$request['id']]);
    }else{
        // error code here or call some error method?
        // do I do error coding here or send a message to some handler/helper?
    }
    echo Zend_Json::encode($return);
}

Essentially this will take a URL and pass json encode information that is currently stored in an array, i.e:

http://restapi.local/api/1

Will display:

[{"1":"Buy milk"}]

The routing takes place in the bootstrap:

class Bootstrap extends Zend_Application_Bootstrap_Bootstrap
{
    public function _initRoutes()
    {
        $front = Zend_Controller_Front::getInstance();
        $router = $front->getRouter();
        $restRoute = new Zend_Rest_Route($front);
        $router->addRoute('default', $restRoute);
    }

}

Below is the full ApiController.php:

class ApiController extends Zend_Controller_Action
{
    public function init()
    {
        /* Initialize action controller here */
        $this->_helper->viewRenderer->setNoRender(true);
        $this->_todo = array (
                    "1" => "Buy milk",
                    "2" => "Pour glass of milk",
                     "3" => "Eat cookies"
        );
    }

    public function indexAction()
    {
        // action body
        echo Zend_Json::encode($this->_todo);
    }

    public function getAction()
    {
        // action body
        $this->_getAllParams();
        $request = $this->_request->getParams();
        if (array_key_exists($request['id'], $this->_todo)){
            $return[] = array($request['id'] => $this->_todo[$request['id']]);
        }
        echo Zend_Json::encode($return);
    }

    public function postAction()
    {
        // action body
        $item = $this->_request->getPost('item');

        $this->_todo[count($this->_todo) + 1] = $item;

        echo Zend_Json::encode($this->_todo);
    }

    public function putAction()
    {
        // action body
    }

    public function deleteAction()
    {
        // action body
    }
}

By using Zend_Rest_Route you are on a good way. I wouldn't recommend making it the default route though, as I had trouble when using MVC-stuff which required additional params not specified in the route, e.g. for a paginator. You could also move the routing-configuration to your application.ini to keep your Bootstrap-class clean.

I recommend simplifying your getAction() by using:

$id = $this->getRequest()->getParam('id', null);
$id = $this->_request->getParam('id');

Both are identical, but I prefer using the get-methods in case I use them to override the handling of the underlying class-property (in this case $this->_request). The second argument specifies the default value to be used, if the param is not set (by default this is null).

edit: Also, instead of

$this->_todo[count($this->_todo) + 1] = $item;

$this->_todo[] = $item;

this will automatically append $item to $this->_todo, even if the variable is not yet initialized.