Optimizing Codegeneration from Ast I have been writing a compiler. While profiling the compiler, one method takes 5 seconds to compile 2k lines of code, my profiler tells me that this is the bottleneck.

   def toLLVMHelp(self, tree= None, isGlobal= True):
    if tree == None:
        tree = self.tree #tree is the Ast

    for i in tree: #iterate through the top node
        g = not type(i) is Tree.FuncStart and not type(i) is Tree.FuncBraceOpen and not type(i) is Tree.FuncBody and isGlobal
        #for code that is not inside a function, it must be put somewhere else, g is a boolean for checking if outside a function
        if g:
            if i.isEnd(): #is the end node
                self.main += i.compile(self)
            else:
                self.toLLVMHelp(i, g) #recurse for further nodes
                self.main += i.compile(self) #compile turns the node into 'llvm-ir'
        else:
            if i.isEnd():
                self.out += i.compile(self)
            else:
                self.toLLVMHelp(i, g)
                self.out += i.compile(self)

I am guessing, that it is slow because of object dispatch. Any suggestion, would be greatly appreciated.

Ideally, you'd profile it on a line-based manner, seeing where the cost is.

But, I might have some suggestions.

The following if-statement gets run every time the function is called:

    if tree == None:
        tree = self.tree #tree is the Ast

In most cases, it would miss, so maybe separate the rest of the code out to another function? (Also, try tree is None instead of tree == None first).

Now, the following line can also be optimised:

        g = not type(i) is Tree.FuncStart and not type(i) is Tree.FuncBraceOpen and not type(i) is Tree.FuncBody and isGlobal
        #for code that is not inside a function, it must be put somewhere else, g is a boolean for checking if outside a function

I think it might as well be written

        g = isGlobal and type(i) not in (Tree.FuncStart, Tree.FuncBraceOpen, Tree.FuncBody)

The reason for this re-arrangement is the following: checking isGlobal is a simple boolean check: really fast. Also, computing type(i) is a function call. Not that expensive, but not cheap either. Better to do that just once. On the other hand, looking up Tree.* is also expensive. There are tricks to make that cheaper as well. I'll get back to that.

        if g:
            if i.isEnd(): #is the end node
                self.main += i.compile(self)
            else:
                self.toLLVMHelp(i, g) #recurse for further nodes
                self.main += i.compile(self) #compile turns the node into 'llvm-ir'
        else:
            if i.isEnd():
                self.out += i.compile(self)
            else:
                self.toLLVMHelp(i, g)
                self.out += i.compile(self)

Not sure if the following will be faster, but I'd like to hope so. That is: profile! I see that in the nested if/else, they share code.

if g:
    if not i.isEnd():
        self.main += self.toLLVMHelp(i, g)
    self.main += i.compile(self)
else:
    if not i.isEnd():
        self.out += self.toLLVMHelp(i, g)
    self.out += i.compile(self)

Again: profile! But I think it will either make it faster, or not be slower, so you should be good. Adding to self.main or self.out is done, but I don't see any nice way to make that better without losing performance.

After thinking about it some more, I see you doing string-appends. Depending on the situation, this can be very expensive, as it must do a full copy.

Maybe replace that logic with

self.main_parts.append(self.toLLVMHelp(i, g))
self.main_parts.append(i.compile(self))

and in the end, replace

self.main

with

''.join(self.main_parts)


Displaying styled validation messages to user I'm new to AJAX and jQuery, and just for testing, I created this simple code. When the user enters their First Name and their Last Name it displays a message that says

  Thanks for entering everything correctly!

in Bluish color, and when the user forgets to enter their First Name and their Last Name it displays an error that says

  You didn't entered anything

in Red color or something similar.  I know it sounds stupid, but this is just testing.

change_name.php

function displayError($error)
{
    // Display a div with a class of error, We style it using CSS
    echo "
$error

";
}

function displayMessage($message)
{
    // Display a div with a class of message, We style it using CSS
    echo "
$message

";
}

if (isset($_POST['first_name']) && isset($_POST['last_name'])) {
    if (empty($_POST['first_name']) && empty($_POST['last_name'])) {
        displayError('You didn\'t entered anything!');
    } else if (empty($_POST['first_name'])) {
        displayError('You didn\'t entered your first name!');
    } else if (empty($_POST['last_name'])) {
        displayError('You didn\'t entered your last name!');
    } else {
        displayMessage('Thanks for entering everything correctly!');
    }
}

JavaScript

$(document).ready(function(){
    $('#nameChange').on('submit', function (e) {

        $.ajax({
            type: 'POST',
            url: 'ajax/change_name.php',
            data: $(this).serialize(),
            success: function(data) {
               $('.result').empty().append(data);
            }
        });

        e.preventDefault();
    });
});

index.php

<!DOCTYPE html>

    <meta charset="UTF-8">
    <title>jQuery AJAX</title>

    <form action="ajax/change_name.php" method="POST" id="nameChange">

        Result:

        First Name:
        <input type="text" name="first_name" id="first_name">

        Last Name:
        <input type="text" name="last_name" id="last_name">

        <input type="submit" name="change_name" value="Change" id="change_name">
    </form>

Is it a good practice to display a
 and then add a class to it, so you can then style it using CSS? Is there a better way for doing this?

Why wait until the data has gone to the server before you inform the user that their name is empty?  You should be doing any processing up front that you can for performance reasons (though you'll still need to do validating from the server side for security reasons).  I recommend the addition of the required attribute:

<input type="text" name="first_name" required />

You can even use CSS to inform the user that the field contains invalid data.

input:invalid {
    border: 1px solid red;
}

Learn more: http://www.html5rocks.com/en/tutorials/forms/html5forms/


Login system security Here is a login system I am working on.  I just want an opinion on it and whether I am going in the right direction or am just completely missing something.

The main thing I would like to know is: am I initializing my user session IDs correctly? Basically once the user enters their credentials, the system goes into the database to see if there is a match. If there is, then it adds the session_id variable if it's not already set.

Now if it is set, then it checks your value against the one in the database.  If they match, then you are entered into the system and then the session_ids are changed on both sides (server/client), otherwise it returns false.  Is this a correct or incorrect method?

function userIsLoggedIn()
{
if ($_SERVER["REQUEST_METHOD"] == "POST") {
  if (isset($_POST['action']) and $_POST['action'] == 'login') {
  if (!isset($_POST['user']) or $_POST['user'] == '' or !isset($_POST['password']) or $_POST['password'] == '') {
        return FALSE;
  }

  array_push($_SESSION['mintime'], time());
  array_shift($_SESSION['mintime']);

  // minimum time (in seconds) between valid form submissions
  if ($_SESSION['mintime'][1] - $_SESSION['mintime'][0] < 2) {

        echo "to quick";
        return FALSE;

  } else {

        $user = test_input($_POST['user']);

        if ($GLOBALS['adminPage'] == 1 and $user != 'admin') {
        return FALSE;
        }

        $passtemp = test_input($_POST['password']);

        $password = hash('sha512', $passtemp . $user);

        if ($user == 'admin') {
        $GLOBALS["isAdmin"] = TRUE;
        }

        if (databaseContainsUser($user, $password)) {

            if (!isset($_SESSION['session_id'])) {
            setSessionID($user);
        $_SESSION['loggedIn'] = TRUE;
        $_SESSION['user']     = $user;
        $_SESSION['password'] = $password;
        return TRUE;
        }
        if ((isset($_SESSION['session_id'])) and (checkSessionID($user))) {

        updateSessionID($user);
        setSessionID($user);
        $_SESSION['loggedIn'] = TRUE;
        $_SESSION['user']     = $user;
        $_SESSION['password'] = $password;
        return TRUE;
        } else {
                unset($_SESSION['session_id']);
            unset($_SESSION['loggedIn']);
                unset($_SESSION['user']);
                unset($_SESSION['password']);
                return FALSE;
        }

        } else {
        unset($_SESSION['loggedIn']);
        unset($_SESSION['user']);
        unset($_SESSION['password']);
        return FALSE;
        }
  } //end else time check
  } //end if login check
  if (isset($_POST['action']) and $_POST['action'] == 'logout') {
  unset($_SESSION['loggedIn']);
  unset($_SESSION['user']);
  unset($_SESSION['password']);
  header('Location: ' . $_POST['goto']);
  exit();
  }

  if (isset($_POST['action']) and $_POST['action'] == 'admin') {
  header('Location: ' . $_POST['gotoadmin']); // gotoadmin = '..'
  exit();
  }
 } //end if POST check

if (isset($_SESSION['loggedIn'])) {
  return databaseContainsUser($_SESSION['user'], $_SESSION['password']);
}

} //end function

//validate input
function test_input($data)
{
$data = trim($data);
$data = stripslashes($data);
$data = htmlspecialchars($data, ENT_QUOTES, 'UTF-8');
return $data;
}

I store all passwords like this:

sha($password + salt, 512)

and the session IDs are made like this:

UUID()

First off, when posting a question, try to give it some natural IDE formatting if you can. this improves readability and you're more likely to get more/better answers!
Code
Technically, you can call
$_SERVER["REQUEST_METHOD"] == "POST"

however, my personal preference is just
isset($_POST)

I find that's it's easier to understand and is more intuitive.
This first two conditions encompass basically the entire function. Why not just reduce them to be negative and return false?
Such as
if (!isset($_POST['action']) or $_POST['action'] != 'login') {
    return FALSE;
}

It's reduces unnecessary nesting. Apply the same for if ($_SERVER["REQUEST_METHOD"] == "POST").

if (!isset($_POST['user']) or $_POST['user'] == '' or !isset($_POST['password']) or $_POST['password'] == '')

could be simplified to
if (!isset($_POST['user'], $_POST['password']) or empty($_POST['user']) or empty($_POST['password']))

It's just easier to read.

In your minimum time check between form submissions, you've added an else, but because the initial condition would return false, an else is completely unnecessary. It also nests your code one more layer, which isn't too great if it can be avoided.

$user = test_input($_POST['user']);

The variable here is very ambiguous. I suggest calling something more appropriate such as $username or something that describes what the value is.

hash('sha512', $passtemp . $user);

This not as secure as it could be! First off, if your PHP version supports it, I suggest you use bcrypt instead of hash. It take scares of the salting for you, so that you don't use bad salts such as you have. The salt should typically be a long string composed of random characters. Not just a username.

if ((isset($_SESSION['session_id'])) and (checkSessionID($user)))

Why so many parentheses! It just makes things unclear!

$_SESSION['password'] = $password;

It's generally bad practice to store a password in any place but the database.
This condition returns true, again the else after this is not needed. Remove it to reduce nesting.

Overall, you're on your way, but there's a lot that could be improved.

Your code is strictly procedural which is becoming less and less
neccessary.

Your code is reinventing something that doesn't need to be made
again.

Your code is hard to read and understand. Partly because of all the nesting you have (which is easy to avoid with an OO design). Here's an article about arrow code written by StackExchange's very own Jeff Atwood.

There're hundreds of frameworks and community run scripts that do the logging in/out for you. Don't think that using one of these is limiting you or making your coding skills worse.
If you insist on using this code, it will suffice, but it will be hard to maintain and expand...


Performs calculation with operators given in an unknown order Problem Statement

Write a simple interpreter which understands "+", "-", and "*" operations. Apply the operations with the correct order of operations (i.e. multiply, then add or subtract) using command/arg pairs starting with the initial value of value. The commands will not necessarily be given in the correct order of operations.

Arguments given: int value, vector<char> operations, vector<int> args

My Solution (C++)

int simpleInterpreter(int value, vector<char> operations, vector<int> args) {
    int index = 0;

    for (char operation : operations) {
        if (operation == '*') {
            value *= args[index];
        index++;
        }
    }

    for (char operation : operations) {
        if (operation == '+') {
            value += args[index];
        index++;
        } else if (operation == '-') {
            value -= args[index];
            index++;
        }
    }

    return value;
}

Example cases:

simpleInterpreter(0, {'+', '+', '*'}, {5000000, 1, 2}) => 3
simpleInterpreter(1, {'*', '-', '*', '+'}, {-1, -1, 1, 1) => 1
simpleInterpreter(45, {'+', '*', '-'}, {2, 90, 180}) => 0
simpleInterpreter(100, {'+', '+', '*'}, {0, 5, 2}) => 7

I'm not really concerned about having safe input / I don't care about bad input. I'm mostly focused on readability and performance here.

Is this a good solution? What can be improved? Another solution I thought of would be to build a custom comparator and store the operations into a set of pairs (operator, how many times it appears), however this would increase space complexity without reducing time complexity so I didn't do that.

I find this task rather bizarre, but anyway…

The function signature should probably be

int simpleInterpreter(int value, const std::vector<char> &operations, const std::vector<int> &args)

Instead of writing separate index++; statements, I'd roll them into a post-increment operator:

value *= args[index++];

To be clear that index refers to the args vector, I'd rename index to argIndex.


Storing words from an input stream into a vector I'm extremely new to C++ and am doing the exercises on the book Accelerated C++. Here is one of the exercises:

  4-5. Write a function that reads words from an input stream and stores
  them in a vector. Use that function both to write programs that count
  the number of words in the input, and to count how many times each
  word occurred.

How can I make this program better by only using vectors?

#include <iostream>
#include <algorithm>
#include <vector>
#include<string>
#include <stdexcept>
using std::cout; using std::cin; using std::vector; using std::sort; using std::string; using std::endl; using std::domain_error; using std::cerr;

//it said "count the number of words in the input, and to count how many times each word occurred". I cannot think of any return value if a function do two things, so I used void.
void run(const vector<string>& v) {
    if (v.size() == 0) throw domain_error("Nothing to count");
    cout << "words count: " << v.size() << endl;
    string unique = v[0];
    int count = 1;
    for (vector<string>::size_type i = 1; i < v.size(); ++i) {
        if (v[i-1] == v[i]) {
            count++;
        } else {
            cout << unique << ": " << count << endl;
            unique = v[i];
            count = 1;
        }
    }
    cout << unique << ": " << count << endl;
}

int main() {
    cout << "Enter all words followed by end of file." << endl;
    vector<string> words;
    string word;
    while (cin >> word) {
        words.push_back(word);
    }
    sort(words.begin(), words.end());
    try {
        run(words);
    } catch (domain_error e) {
        std::cerr << e.what() << endl;
    }
    return 0;
}

Minor inconsistency here:

#include<string>

Add a space in between them, like all the others:

#include <string>

Just get rid of this:

using std::cout; using std::cin; using std::vector; using std::sort; using std::string; using std::endl; using std::domain_error; using std::cerr;

Even if you put them onto separate lines, it'll still be lengthy.  Just put std:: where necessary and try to keep it consistent.
Exception objects usually should not be passed by value:

catch (domain_error e)

They should be passed by const&:

catch (domain_error const& e)

Consider separating this into additional lines:

if (v.size() == 0) throw domain_error("Nothing to count");

While it's okay to put short statements on a single line, it may be a little harder to read and maintain longer lines, also with the lack of curly braces.

In addition, just call empty() instead of comparing size() to 0.

if (v.empty())
{
    throw domain_error("Nothing to count");
}

Instead of getting the first element via []:

string unique = v[0];

use front():

string unique = v.front();

I'd also like to add a compliment:

Thank you for using std::size_type instead of int.  I hardly see that done by others.  It's good to see that you're paying attention to return types and compiler mismatch warnings.


Storing data about food I'm trying to make a simple app to help me store data about food to use this database in a future diet application. What can I do to improve the code? What part of code should I make a class etc.?

    #include <iostream>
    #include <vector>
    #include <string>
    #include <map>
    #include <iterator>

    using namespace std;

    struct food
    {
        int mass;
        double kcal;
        double prot;
        double carb;
        double fat;
    };

    map<string, food> foodMap_prot;
    map<string, food> foodMap_carb;
    map<string, food> foodMap_fat;
    int main()
    {

        int mass;
        double kcal;
        double prot;
        double carb;
        double fat;
        string name;
        cout << "Enter a name for a product: ";
        getline(cin,name,'\n');
        cout << "\nmass of [" << name << "]: ";
        cin >> mass;
        cout << "\ncalories of [" << name << "]: ";
        cin >> kcal;
        cout << "\nprotein in [" << name << "]: ";
        cin >> prot;
        cout << "\ncarbs in [" << name << "]: ";
        cin >> carb;
        cout << "\nfat in [" << name << "]: ";
        cin >> fat;
        string kategory;
        double factor;
        kcal /= 100;
        prot /=100;
        carb /=100;
        fat /=100;
        food temp1 = {mass, mass*kcal, mass*prot, mass*carb, mass*fat};
        map<string, food>::iterator itr, koniec;
        if(prot>carb && prot>fat)
        {
            factor = prot*100/7;
            mass = mass/factor;
            temp1 = {mass, mass*kcal, mass*prot, mass*carb, mass*fat};
            foodMap_prot[name] = temp1;
            itr = foodMap_prot.find(name);
            koniec = foodMap_prot.end();
            kategory = "Protein";
}
else if(carb>prot&&carb>fat)
{
    factor = carb*100/9;
    mass = mass/factor;
    temp1 = {mass, mass*kcal, mass*prot, mass*carb, mass*fat};
    foodMap_carb[name] = temp1;
    itr = foodMap_carb.find(name);
    koniec = foodMap_carb.end();
    kategory = "Carbs";
}
else if(fat>prot&&fat>carb)
{
    factor = fat*100/1.5;
    mass = mass/factor;
    temp1 = {mass, mass*kcal, mass*prot, mass*carb, mass*fat};
    foodMap_fat[name] = temp1;
    itr = foodMap_fat.find(name);
    koniec = foodMap_fat.end();
    kategory = "Fat";
}

for(;itr!=koniec; ++itr)
{
    cout <<"\n\nyou added a new product \t[" << name <<"]";
    cout <<"\n\ncategory: \t\t\t" << kategory;
    cout <<"\n\nmass of one block of [" << name << "]: \t" << itr->second.mass<<"g";
    cout <<"\n\ncalories in [" << name << "]: \t" << itr->second.kcal<<" kcal";
    cout << "\n\nprotein [" << name << "]: \t"<< itr->second.prot<<"g";
    cout <<"\n\ncarbs [" << name << "]: \t" << itr->second.carb<<"g";
    cout << "\n\nfat [" << name << "]: \t" << itr->second.fat<<"g";
}

    }

Namespace std
Stop doing this:
using namespace std;

See every other C++ code review and Why is “using namespace std;” considered bad practice?
Monolithic Functions
Break your code into multiple smaller functions. A good rule of thumb is no more than a dozen lines. If you give your function meaningful names then your code becomes self documenting.
Global Variables
Global variables are a bad idea. It makes testing hard. It makes debugging hard. You don't the result of the function biased on its inputs (it also depends on some other external global state that other functions may mutate while you are not watching.
Pass parameters to functions that should be the only state the worry about.
Objects
This looks like it should be an object:
map<string, food> foodMap_prot;
map<string, food> foodMap_carb;
map<string, food> foodMap_fat;

How about
class FoodMap
{
    typedef  std::map<std::string, Food>  FoodMap;

    FoodMap   protein;     // avoid the temptation to shorten names
    FoodMap   carbs;       // unless like this they are hard to spell.
    FoodMap   fat;
};

// Note on naming conventions.
// User defined types (usually) start with an Uppercase letters.
// Object usually start with a lowercase letter.
// This makes it easy to spot types over objects.

Looks like all the code in your main is designed to correctly fill up the map. So this should really be part of the FoodMap class. The class should be the only thing defining where things go internally. If you let functions (like main) fill up the map you have no control to validate input.


multiple if statements check optimize My code has mutiple checks for if staments which I think could be optimized into few lines if I take id into varibable. To give some background I want to disable a button based on length of characters in input box. But the thing is out of two buttons only one will be rendered at a time.
Below is my working version of code which I am hoping to optimize.

  var sortcodeError = document.getElementById("sortcodeError")
         var accountnameError = document.getElementById("accountnameError")
          if (sortcodeError != null) {
                  document.getElementById("sortcodeError").disabled = false;
               }
               if (accountnameError != null) {
                  document.getElementById("accountnameError").disabled = false;
               }
         if ($scope.data.branchTransitNumber && $scope.data.depositAccountNumber) {
            if ($scope.data.branchTransitNumber.length < 6) {
               if (sortcodeError != null) {
                  document.getElementById("sortcodeError").disabled = true;
               }
               if (accountnameError != null) {
                  document.getElementById("accountnameError").disabled = true;
               }

            }
            if ($scope.data.depositAccountNumber.length < 8) {
               if (sortcodeError != null) {
                  document.getElementById("sortcodeError").disabled = true;
               }
               if (accountnameError != null) {
                  document.getElementById("accountnameError").disabled = true;
                  // angular.element($('#btnSubmi')).addClass("gray");
               }

            }
         }

My initial thought is if I could take id as varibale but then I don't think it is the best way

Since exactly one element will exist at a time, you can retrieve it by alternating with || between both getElementById calls. Then, there's no need for any existence test later.

Since you set disabled to true in both branchTransitNumber.length < 6 and depositAccountNumber.length < 8, you can combine those blocks together:

const button = document.getElementById('sortcodeError') || document.getElementById('accountnameError');
button.disabled = false;
if ($scope.data.branchTransitNumber && $scope.data.depositAccountNumber) {
    if ($scope.data.branchTransitNumber.length < 6 || $scope.data.depositAccountNumber.length < 8) {
        button.disabled = true;
    }
}

(The above can be made even more concise via destructuring or optional chaining to avoid the double ifs, but I think it'd look too confusing.)

Or, if you need the addClass call only in the case of $scope.data.depositAccountNumber.length < 8 and accountnameError existing:

const button = document.getElementById('sortcodeError') || document.getElementById('accountnameError');
button.disabled = false;
if ($scope.data.branchTransitNumber && $scope.data.depositAccountNumber) {
    const depositNumberProblem = $scope.data.depositAccountNumber.length < 8;
    if ($scope.data.branchTransitNumber.length < 6 || depositNumberProblem) {
        button.disabled = true;
        if (depositNumberProblem && button.id === 'accountnameError') {
            angular.element($('#btnSubmi')).addClass("gray");
        }
    }
}

Depending on the logic of your script, the above may be simplifiable. For example, you may wish to do .addClass("gray") regardless when you disable the button.

Other notes:

It's 2020, best to at least use ES2015 syntax in new code by declaring variables with const, not var (which has too many gotchas)
Whenever you do have to compare two different values, use strict equality with === or !== - avoid loose equality == and !=, since loose equality has too many strange rules that a developer should not have to memorize.
If the alternation trick above weren't possible, you'd still be able to reference the variable names you saved the elements into to keep things concise. Eg, with const sortcodeError = document.getElementById("sortcodeError"), you should never have to call document.getElementById("sortcodeError") later - instead, just reference the sortcodeError variable.
Use consistent indentation. If one statement is in the same block as another, it should begin at the same indentation level. This significantly improves a code's readability.
Either use semicolons wherever appropriate, or don't. (If you're not an expert, I'd recommend using them, otherwise you'll occasionally run into strange bugs.) Consider using a linter to automatically prompt you to fix potential mistakes (and to make the code nicer-looking).
The overall logic looks a bit weird. Do you really need to check that both branchTransitNumber and depositAccountNumber exist, but disable a button if either of them fail a length test? In a validation situation, I'd expect to disable a button if either fail, rather than if both exist and either fail. Double-check the logical paths your code requires and results in, and consider what sort of effect you really want.


Read one million records per second The following code ingests 10k-20k records per second and I want to improve the performance of it. I'm reading json and ingesting it into the database using Kafka. I'm running it on the cluster of five nodes with zookeeper and Kafka installed on it.

Can you give me some tips to improve?

import os
import json
from multiprocessing import Pool
from kafka.client import KafkaClient
from kafka.producer import SimpleProducer

def process_line(line):
    producer = SimpleProducer(client)
    try:
       jrec = json.loads(line.strip())
       producer.send_messages('twitter2613',json.dumps(jrec))
    except ValueError, e:
                {}

if __name__ == "__main__":
    client = KafkaClient('10.62.84.35:9092')
    myloop=True
    pool = Pool(30)

    direcToData = os.listdir("/FullData/RowData")
    for loop in direcToData:
        mydir2=os.listdir("/FullData/RowData/"+loop)

        for i in mydir2:
            if  myloop:
                 with open("/FullData/RowData/"+loop+"/"+i) as source_file:
                     # chunk the work into batches of 4 lines at a time
                     results = pool.map(process_line, source_file, 30)

Don't split reading from same file into different processes

I haven't used Pool my self, but it seems like you're a splitting the file read over 30 different processes, where each reads 30 lines each? If that is correct, you should seriously consider a different split tactic, as that will throttle your IO seriously. You'll have 30 different processess trying to read from 30 different places in the file at the same time.

A better tactic would be to send each file to a different process, and then let that process handle that file completely.

Choose number of processes wisely

Another caveat would be the number of processes you use. You create 30 processes, but as long as you don't have a 30 actual processors available you wont see any major performance gain using this number of processes.

Back in the days using various Unix based operating systems, we did compilations in batches and the general rule was that we would aim for approx 4 times the number of processors we had available. In other words on a quad-processor, we would aim for 16 processes. Any more and we started seeing congestion due to interprocess issues and IO related performance bottlenecks.

Shorten the distance from file to server

Another speedup can be found if you are able to avoid network traffic. That is if you are able to run this script directly on the Kafka server, so that you can use loopback addresses and local connections instead of using the IP network.

Establish a baseline

Not so much a performance suggestion, but do you have good baselines for how long it takes to do a typical run using only a single process? This can be helpful, when you start dividing the load according to other metrics, to compare when you reach a threshold regarding what each server/client should do.


Using a decorator to apply an inherited method to all child objects of the inherited type I wanted to share this and get some feedback.  My goal was to be able to have an object with methods:

class Thing(object):
    def __init__(self, data=123):
        self.data = data

    def add_one(self):
        self.data += 1

    def add_number(self, number):
        self.data += number

And then a collection object, MetaThing that can hold several Thing objects, which inherits from Thing like this:

class MetaThing(Thing):

    def __init__(self):
        super(MetaThing, self).__init__()

        self.list = [1, 2, 3, 4, 5]
        self.things = []
        self.setupThings()

    def setupThings(self):

        for i in self.list:
            thing = Thing(data=i)
            self.things.append(thing)

Finally, I want to be able to apply the methods in Thing to all the Thing instances stored in MetaThing.things when those same methods are called on MetaThing.

Doing this explicitly creates methods like this:

    def add_one(self):
        for t in self.things:
            t.add_one()

    def add_number(self, number):
        for t in self.things:
            t.add_number(number)

So I started wondering if this would be a good situation for a decorator, so I wrote one that I think does what I want:

def allthings(func):
    def new_func(*args, **kwargs):
        self = args[0]
        for thing in self.things:
            arglist = list(args)
            arglist[0] = thing
            func(*arglist, **kwargs)

    return new_func

So now, the MetaThing method can use the @allthings decorator to call an inherited method on all child objects without rewriting the for loop every time:

class MetaThing(Thing):

    def __init__(self):
        super(MetaThing, self).__init__()

        self.list = [1, 2, 3, 4, 5]
        self.things = []
        self.setupThings()

    def setupThings(self):

        for i in self.list:
            thing = Thing(data=i)
            self.things.append(thing)

    @allthings
    def add_one(self):
        self.add_one()

    @allthings
    def add_number(self, number):
        self.add_number(number)

Finally this block verifies that it's working:

if __name__ == '__main__':

    meta = MetaThing()

    meta.add_one()

    print [t.data for t in meta.things]

    meta.add_number(5)

    print [t.data for t in meta.things]

Is this a valid approach? Is there a better way to achieve this? Am I crazy for using decorators in this way?

My actual scenario is a PlottedShape with lots of methods like plot, delete, update, select, apply_opacity, apply_color, apply_wireframe, etc. and I also have a PlottedMultiShape made up of multiple shapes (like a snowman shape made of multiple circles).  If PlottedMultiShape.select() is called I want to call select on all the child PlottedShape objects -- and I'd like this same behavior for all of the other methods as well.  Using a decorator seemed like a good "Don't Repeat Yourself" tactic.

Another way to do this would be to use __getattr__().

class Wraps(object):
  def __init__(self, *args):
    self.things = args
  def __getattr__(self, item):
    def each(*args, **kwargs):
      for thing in self.things:
        getattr(thing, item)(*args, **kwargs)
    return each
  def some_real_wraps_method(self):
    print "real method"

class Thing(object):
  def __init__(self, name):
    self.name = name
  def a(self):
    print self.name, "a"
  def b(self, *args, **kwargs):
    print self.name, "b", args, kwargs

w = Wraps(Thing("1"), Thing("2"))
w.a()
w.b()
w.b("arg1", "arg2", named=True)
w.some_real_wraps_method()

Running this will output:

1 a
2 a
1 b () {}
2 b () {}
1 b ('arg1', 'arg2') {'named': True}
2 b ('arg1', 'arg2') {'named': True}
real method

The upside is that you don't have to define an methods in order for them to be proxied. The downside is that it will try to proxy any attribute not defined by Wraps (this can be counteracted with a white list).


First Python script: number-guessing game with a global variable This is my first Python script, and I was hoping to get some feedback.  I do have one specific question: is the use of global variables considered a bad practice?  When I used them below, my programmer's instincts immediately went off as though I was creating a code-smell, mostly because it seems much less elegant than just having a private String myVariable at the top of a Java class.  The use of global myVariable to force the scope seemed a bit hacky.  But I don't know a single thing about Python except what I wrote below, so I could be way off.

I also realize that the below code is more complex than it needs to be.  I know that it could be written in much fewer lines of code by removing function definitions and such (in fact, my first version was only like 15 lines).  But I wrote this more complicated version in order to get a handle on some of the basic concepts like sharing variables, returning from functions, etc.

Any tips or glaring style/best-practices issues are appreciated!  (I'm a style-Nazi with Java, so please, don't hold back so that I can learn!)

#!/usr/bin/python

from random import randint

# GLOBALS
guesses = 0
the_number = 0

def checkGuess(guess):
    global the_number

    if guess <= 0:
        return False
    elif guess == the_number:
        return True
    elif guess < the_number:
        print "The number is HIGHER."
    elif guess > the_number:
        print "The number is LOWER."

    return False

def isValid(guess):
    return len(guess) > 0 and guess.isdigit()

def getInput():
    global guesses
    input = ""

    while not isValid(input):
        input = raw_input("(#" + str(guesses) + ") Guess a number: ")
        if not isValid(input):
            print "Please guess an integer!"

    return int(input)

def runGame(min, max):
    global guesses
    global the_number

    the_number = randint(min, max)
    guess = 0

    print "I'm thinking of a number between " + str(min) + " and " + str(max) + " ..."

    while not checkGuess(guess):
        guesses = guesses + 1
        guess = getInput()

    print "YOU WON!"

def printTitle():
    print "----------------------"
    print "----- MASTERMIND -----"
    print "----------------------"
    print ""

def main():
    printTitle()
    runGame(1, 100)

main()

Some notes:

Mutable global state is a bad programming practice. Instead, pass values as arguments so functions are black boxes that take values (as arguments) and return values. If such a function does not perform any side-effect (print to console, write a file, ...), then it's a "pure function". Try to write pure functions whenever possible.
Conditionals: Don't write a fallback (return False) where some branches get and others don't. Non-overlapping conditionals are more clear.
Use name_of_variable and name_of_function.
Try to use Python 3 whenever possible.

I'd write:

from random import randint
from itertools import count

def is_guess_correct(number, guess):
    if guess == number:
        return True
    elif guess < number:
        print("The number is HIGHER.")
        return False
    else:
        print("The number is LOWER.")
        return False

def is_valid_guess(number_string):
    return number_string.isdigit()

def get_number(guess_iteration):
    while 1:
        number_string = input("({0}) Guess a number: ".format(guess_iteration))
        if is_valid_guess(number_string):
            return int(number_string)
        else:
            print("Please enter a valid integer!")

def run_game(nmin, nmax):
    number = randint(nmin, nmax)
    print("I'm thinking of a number between {0} and {1}...".format(nmin, nmax))

    for guess_iteration in count(1):
        guess = get_number(guess_iteration)
        if is_guess_correct(number, guess):
            print("YOU WON!")
            break

if __name__ == '__main__':
    run_game(1, 100)


Function to calculate the ordinal day I just handed in my first assessed coursework for my Computer Science degree. Part of it was to take in a date (dd mm yyyy) between 1900 and 2100 and output the ordinal date. My function to do that is below.

I’m new to C - I’ve used Python, JavaScript and more recently Pascal, so I might be approaching this from the wrong direction, and there could be a more traditional method I should be using.

Any suggestions to make this more standard with C guidelines would be great - in addition to other algorithms I could have used.

int monthToDays(int day, int month, int year) {
  int monthLengths[11] = {31, 28, 31, 30, 31, 30, 31, 31, 30, 31, 30}; // only 11 long because january -> 0 not 31

  monthLengths[1] = monthLengths[1] + checkIsLeap(year); //increase febuaray by 1 if it's a leap year

  int iMonth = 0;
  int totalDays = day;
                          // reduce by 1 because array is 0 indexed
  for (iMonth = 0; iMonth < month - 1; iMonth++) {
    totalDays = totalDays + monthLengths[iMonth];
  }

  return totalDays;
}

int checkIsLeap(int year) {

  int leap = 0;

  if (year % 4 == 0) {
    leap = 1;
    if (year % 100 == 0) {
      leap = 0;
      if (year % 400 == 0) {
        leap = 1;
      }
    }
  }

  return leap;
}

Remarks about your code

I, too, think that you should change the signature and not pass the year into that function, since basically it doesn't need to know about the year.  What it needs to know is whether it's a leap year or not (thus a bool from stdbool.h).  So pass that in. (A bit like Tell, don't ask in OOP)
Modifying the months length of February seems like the most "real world like" approach, but I wouldn't do it, because then you can make the months lengths const (and also static).  Do a correction (+ 1) for leap years after your calculation, then.
Going further, are you really interested in the lengths of a month? You could as well just store the number of days (in a non leap year) that precede a month (January -> 0, February -> 31, March -> 31 + 28, ...).  Then you can get rid of that loop all together.
Lack of assertments: When you make an assumption about the value of an function argument (1 <= months <= 12), then assert for that (even if the specification of your function does not include argument validation!) such that if an invalid value gets passed, you fail fast and loud. (Ooops .. an advise I should have implemented myself in the code below ...)
naming. monthToDays ... sounds like "convert a month into days" ... I would expect this to accept a month (number) and get the number of days in that month. ordinal_date ? Or a bit more detailed ordinal_date_day ?

Test, test, test!

Next, I'd like to introduce you to TDD (test driven development): Instead of first writing your code and then test it (as all - or almost all - code should be tested!) you start off writing tests, specifying the desired behavior, and then make these tests pass by actually implementing the logic to achieve that behavior.

Normally, you'd look into some testing framework / library, but we can go with a simple macro here for now:

#define check(expr) \
  if(!(expr)) { \
    printf("[FAILED] " #expr " in %s\n", __func__); \
  }

Note that this is the bare minimum of information we need out of a test case: An indication when it fails and which test case failed.  Testing frameworks can give you far better information, for example a comparison of the expected value of a function call and it's actual value.

Start with a simple case:

void ordinal_date_in_january_should_be_day_number() {
  check(ordinal_date( 1, 1, false) ==  1);
  check(ordinal_date( 1, 1,  true) ==  1);
  check(ordinal_date(11, 1, false) == 11);
  check(ordinal_date(11, 1,  true) == 11);
}

That's simple to implement:

int ordinal_date(int day, int month, bool leap_year) {
  return day;
}

Going to a slightly more complex case, days in February:

void ordinal_date_in_february_should_be_day_number_plus_days_in_january() {
  check(ordinal_date( 1, 2, false) == 32);
  check(ordinal_date( 1, 2,  true) == 32);
  check(ordinal_date(11, 2, false) == 42);
  check(ordinal_date(11, 2,  true) == 42);
}

Well, KISS:

int ordinal_date(int day, int month, bool leap_year) {
  if (month == 1) {
    return day;
  }
  if (month == 2) {
    return day + 31;
  }
  return -1; // Fall-through, always code defensively
}

Fine, tests pass again.  Time for new tests.  Check against known values:

void ordinal_date_should_match_known_values_for_non_leap_year_march_to_dezember() {
  check(ordinal_date( 1,  3, false) == ( 59 + 1));
  check(ordinal_date( 1,  4, false) == ( 90 + 1));
  check(ordinal_date( 1,  5, false) == (120 + 1));
  check(ordinal_date( 1,  6, false) == (151 + 1));
  check(ordinal_date( 1,  7, false) == (181 + 1));
  check(ordinal_date( 1,  8, false) == (212 + 1));
  check(ordinal_date( 1,  9, false) == (243 + 1));
  check(ordinal_date( 1, 10, false) == (273 + 1));
  check(ordinal_date( 1, 11, false) == (304 + 1));
  check(ordinal_date( 1, 12, false) == (334 + 1));
}

Ok, we can go on with these if .. but this starts to look like a lookup table to me, so better implement that instead:

int ordinal_date(int day, int month, bool leap_year) {
  static const int days_up_to_month[] = {
    0, 31, 59, 90, 120, 151, 181, 212, 243, 273, 304, 334};
  return days_up_to_month[month - 1] + day;
}

Note that we can make this change with confidence of not breaking existing requirements due to the previous tests we wrote.

Finally, let's include the known values for a leap year:

void ordinal_date_should_match_known_values_for_leap_year_march_to_dezember() {
  check(ordinal_date( 1,  3, true) == ( 60 + 1));
  check(ordinal_date( 1,  4, true) == ( 91 + 1));
  check(ordinal_date( 1,  5, true) == (121 + 1));
  check(ordinal_date( 1,  6, true) == (152 + 1));
  check(ordinal_date( 1,  7, true) == (182 + 1));
  check(ordinal_date( 1,  8, true) == (213 + 1));
  check(ordinal_date( 1,  9, true) == (244 + 1));
  check(ordinal_date( 1, 10, true) == (274 + 1));
  check(ordinal_date( 1, 11, true) == (305 + 1));
  check(ordinal_date( 1, 12, true) == (335 + 1));
}

Of course this fails spectacularly, but is easily fixed with some "domain knowledge" (namely that in a leap year February is a day longer):

int ordinal_date(int day, int month, bool leap_year) {
  static const int days_up_to_month[] = {
    0, 31, 59, 90, 120, 151, 181, 212, 243, 273, 304, 334};
  return days_up_to_month[month - 1] + day
         + ((leap_year && (month > 2)) ? 1 : 0);
}

Note that our tests would have revealed this domain knowledge, too.  Try the code without the month > 2 condition and see which tests fail.

Finally: Tests pass.  Fine.  I hope this is of potential use for you.

(Ideone link with above code)

That 2 is a naked magic number, which is not good.  Better define a constant FEB or something like that.
You wrote that the code is tested with 65000 random values.  Sorry, but that's useless.  For this example, it will - of course (with very high probability) - spot any errors in the implementation as there are only 365 + 366 different inputs that the function might take.  But what it won't help with is clearly documenting the desired behavior.  Moreover, running your test will take a lot more time than running the simpler tests here.  That's not an issue for just one function, but within a mid size to large size project these run times will build up quickly.  And the longer the test, the less often you will run it!  The less often you run it, the more code you write without testing it.


Very basic calculator using methods I have made a basic calculator using methods. This is my first attempt at using methods and would like to see if I can improve on this as there is a lot of repeated code.

import java.util.Scanner;

public class Calculator {

    public static void main(String[] args) {

        Scanner kb = new Scanner(System.in);

        System.out.println("Simple Calculator");

        System.out.println("\nHere are your options:");
        System.out.println("\n1. Addition");
        System.out.println("2. Subtraction");
        System.out.println("3. Division");
        System.out.println("4. Multiplication");

        System.out.print("\nWhat would you like to do?: ");
        int choice = kb.nextInt();
        System.out.println();

        if (choice == 1){
            addition();
        }
        else if (choice == 2){
            subtraction();
        }
        else if (choice == 3){
            division();
        }
        else if (choice == 4){
            multiplication();
        }

        System.out.println();
        kb.close();
    }

    public static void addition(){

        int nOne, nTwo;
        Scanner kb = new Scanner(System.in);

        System.out.println("Addition");

        System.out.print("\nFirst Number: ");
        nOne = kb.nextInt();

        System.out.print("\nSecond Number: ");
        nTwo = kb.nextInt();

        kb.close();
        System.out.println("\nSum: " + nOne + " + " + nTwo + " = " + (nOne + nTwo));
    }

    public static void subtraction(){
        int nOne, nTwo;
        Scanner kb = new Scanner(System.in);

        System.out.println("Subtraction");

        System.out.print("\nFirst Number: ");
        nOne = kb.nextInt();

        System.out.print("\nSecond Number: ");
        nTwo = kb.nextInt();

        kb.close();
        System.out.println("\nSum: " + nOne + " - " + nTwo + " = " + (nOne - nTwo));
    }

    public static void division(){
        int nOne, nTwo;
        Scanner kb = new Scanner(System.in);

        System.out.println("Division");

        System.out.print("\nFirst Number: ");
        nOne = kb.nextInt();

        System.out.print("\nSecond Number: ");
        nTwo = kb.nextInt();

        kb.close();
        System.out.println("\nSum: " + nOne + " / " + nTwo + " = " + (nOne / nTwo));
    }

    public static void multiplication(){
        int nOne, nTwo;
        Scanner kb = new Scanner(System.in);

        System.out.println("Multiplication");

        System.out.print("\nFirst Number: ");
        nOne = kb.nextInt();

        System.out.print("\nSecond Number: ");
        nTwo = kb.nextInt();

        kb.close();
        System.out.println("\nSum: " + nOne + " x " + nTwo + " = " + (nOne * nTwo));
    }
}

Nice job!  I hope you find the comments below useful as you progress.

Variable names: Try to have variable names which describe what the variable actually represents or holds.  For example, I have no idea why you named your Scanner "kb".
Method names: Method names should always be verb phrases (actions) which describe what the method does.  So for example, your addition() method might be better named performAddition() or some such.  Even just add() might work, though that might be a bit misleading and lead someone reading your code to believe that all the method does is add two numbers, when really the core functionality is user interface.
Redundancy: Any time you are copy-pasting code, alarm bells should go off in your head.  You do this in multiple places.  Most notable is:

System.out.print("\nFirst Number: ");
nOne = kb.nextInt();

System.out.print("\nSecond Number: ");
nTwo = kb.nextInt();

What that means is that this is a prime candidate for encapsulation into its own method.  Something like this:

private static int[] getNumbers() {
    int[] numbers = new int[2];
    System.out.print("\nFirst Number: ")
    numbers[0] = scanner.nextInt();
    System.out.print("\nSecond Number: ");
    numbers[1] = scanner.nextInt();
    return numbers;
}

Now we can simply call this method whenever we need to get numbers from the user.  Much easier, and we've reduced the complexity of our code.

Another thing that's redundant is your constant re-declaration of kb = new Scanner(System.in).  You really only need to do this once.  If it were me, I'd have it as a static field for the entire class, e.g.,

private static final Scanner STDIN = new Scanner(System.in);

This is the basic rule for how constants are defined in Java.  I named the variable STDIN because this is a convention for referring to the "standard input" stream, but SCANNER or something would be just as good.

Some other small things

You should consider using a switch statement on your choice variable:

switch(choice) {
case 1:
    performAddition();
    break;
case 2:
    performSubtraction();
    break;
case 3:
    performDivision();
    break;
case 4:
    performMultiplication();
    break;
default:
    System.out.println("Invalid choice!");
    break;
}

... or you could write it all in-line, if it looks better to you, since each block only really does one thing:

switch(choice) {
case 1: performAddition(); break;
case 2: performSubtraction(); break;
// etc.
}

This would also be a perfect use-case for enums, but that my be getting a bit too complicated for a starting project.

Small final note: I think you have a bug.  The line where you display the results always says "sum", even when you're doing division, multiplication, or subtraction. :)


C# Logical and Math expression parser I needed a simple parser that could do both logical and math operations expressed in a string, as well as being able to use variables stored in json.
None of what I found online seemed to do all of the above, so I came up with my own. Below is the source code for my ExpressionParser as well as my JExpressionParser, in case your variables are stored in a JSON.
Hopefully this post will help someone, as well as allow others to improve my code.
Here is the base parser:
using System.Collections.Generic;
using System.Data;
using System.Linq;
using System.Text;

public class ExpressionParser
{
    public virtual IDictionary<string, object> Variables { get; }
    protected ExpressionParser() { }
    protected virtual DataTable GetComputer()
    {
        var computer = new DataTable();
        if (Variables != null)
        {
            computer.Columns.AddRange(Variables.Select(v => new DataColumn(v.Key) { DataType = v.Value.GetType() }).ToArray());
            computer.Rows.Add(Variables.Values.ToArray());
        }
        return computer;
    }
    public ExpressionParser(IDictionary<string, object> variables = null)
    {
        Variables = variables ?? new Dictionary<string, object>();
    }
    public object Compute(string expression)
    {
        StringBuilder sb = new StringBuilder(expression);
        foreach (var key in Variables.Keys)
            sb.Replace(key, $"Sum({key})");
        sb.Replace("==", "=");
        using (var computer = GetComputer())
            return computer.Compute(sb.ToString(), null);
    }
}

And here is one that works with json objects:
using Newtonsoft.Json.Linq;
using System;
using System.Collections.Generic;
using System.Linq;

public class JExpressionParser : ExpressionParser
{
    public JObject JVariables { get; set; }
    public override IDictionary<string, object> Variables =>
        JVariables.Properties().ToDictionary(p => p.Name, p => p.Value.ToObject(conversions[p.Value.Type]));
    static readonly Dictionary<JTokenType, Type> conversions = new Dictionary<JTokenType, Type>()
    {
        [JTokenType.Integer] = typeof(int),
        [JTokenType.Float] = typeof(float),
        [JTokenType.Boolean] = typeof(bool),
    };
    public JExpressionParser(JObject jVariables = null)
    {
        JVariables = jVariables ?? new JObject();
    }
}

Usage:
var variables = new Dictionary<string, object>() { ["budget"] = 1000, ["cost"] = 965 };
var parser = new ExpressionParser(variables);
Console.WriteLine("We have $" + parser.Compute("budget - cost") + " of budget left.");

I can see three small improvement areas:
Use ctor instead of object initializer
Instead of this:
new DataColumn(v.Key) { DataType = v.Value.GetType() }

Use this:
new DataColumn(v.Key, v.Value.GetType())

Object initializer will run after ctor. If ctor accepts that parameter then prefer to provide it in the ctor.
Use early exist instead of guard expression
Instead of this:
if (Variables != null)
{
    computer.Columns.AddRange(Variables.Select(v => new DataColumn(v.Key, v.Value.GetType())).ToArray());
    computer.Rows.Add(Variables.Values.ToArray());
}
return computer;

Use this:
if ((Variables?.Any() ?? true)) return computer;

computer.Columns.AddRange(Variables.Select(v => new DataColumn(v.Key, v.Value.GetType())).ToArray());
computer.Rows.Add(Variables.Values.ToArray());
return computer;

Yes, I know it's against the single return statement. But with this approach you streamline your logic.
Use immutable collection instead of just readonly access modifier
Instead of this:
static readonly Dictionary<JTokenType, Type> conversions = new Dictionary<JTokenType, Type>()
{
    [JTokenType.Integer] = typeof(int),
    [JTokenType.Float] = typeof(float),
    [JTokenType.Boolean] = typeof(bool),
};

Use this:
static readonly ImmutableDictionary<JTokenType, Type> conversions = new Dictionary<JTokenType, Type>()
{
    [JTokenType.Integer] = typeof(int),
    [JTokenType.Float] = typeof(float),
    [JTokenType.Boolean] = typeof(bool),
}.ToImmutableDictionary();

With readonly you only prevent to replace the whole object. But you can still add and remove new items to the dictionary:
//Invalid operations
conversions = null;
conversions = new Dictionary<string, JTokenType> {};

//Valid operations
conversions.Add(JTokenType.Array, typeof(Array));
conversions.Remove(JTokenType.Integer);

With ImmutableDictionary the Add and Remove will return a new ImmutableDictionary so it will not have any effect on the original collection. Visual Studio will even worn you about it:


LeetCode: Remove Duplicates from Sorted Array Please review for performance

https://leetcode.com/explore/featured/card/top-interview-questions-easy/92/array/727/

  Given a sorted array nums, remove the duplicates in-place such that
  each element appear only once and return the new length.

  Do not allocate extra space for another array, you must do this by
  modifying the input array in-place with O(1) extra memory.

  Example 1:

  Given nums = [1,1,2],

  Your function should return length = 2, with the first two elements of
  nums being 1 and 2 respectively.

  It doesn't matter what you leave beyond the returned length. Example
  2:

  Given nums = [0,0,1,1,1,2,2,3,3,4],

  Your function should return length = 5, with the first five elements
  of nums being modified to 0, 1, 2, 3, and 4 respectively.

  It doesn't matter what values are set beyond the returned length.

using Microsoft.VisualStudio.TestTools.UnitTesting;

namespace ArrayQuestions
{
    /// <summary>
    /// Remove Duplicates from Sorted Array
    /// https://leetcode.com/explore/featured/card/top-interview-questions-easy/92/array/727/
    /// </summary>
    [TestClass]
    public class RemoveDuplicatesfromSortedArray
    {
        [TestMethod]
        public void RemoveDuplicates5()
        {
            int[] nums = {0, 0, 1, 1, 1, 2, 2, 3, 3, 4};
            int[] excpected = {0, 1, 2, 3, 4};
            int res = RemoveDuplicates(nums);
            Assert.AreEqual(5,res);
            for (int i = 0; i < res; i++)
            {
                Assert.AreEqual(excpected[i], nums[i]);
            }
        }

        public int RemoveDuplicates(int[] nums)
        {
            if (nums.Length <= 1)
            {
                return nums.Length;
            }

            int current = nums[0];   //1
            int newLength = 1;          //2
            for (int i = 1; i < nums.Length; i++) // i = 5
            {
                if (nums[i] != current)
                {
                    nums[newLength] = nums[i];  //0,1,2
                    newLength++;
                    current = nums[i];
                }
                //else continue to next item
            }

            return newLength;
        }
    }
}

There is not much to review here. This algorithm looks to be as good as it gets. I don't know how much value there is in nitpicking, but here I go:

Review

The summary tag should be used to describe a type or a type member. Use remarks to add supplemental information (xmldoc summary) -> put the challenge URL in a remarks tag, not in the summary.
The challenge provides an input parameter nums but C# naming conventions don't invite you to use abbreviations. You could/should change its name to numbers.
Your unit test contains a variable with a typo excpected.
Your method is declared public, meaning you should check arguments against null to avoid the nasty NullReferenceException.
You've added alot of inline comments to show how the algorithm works. Considering the simplicity of the algorithm and the presence of unit tests, I feel these comments don't add much value to the code, if any at all //else continue to next item etc.


A replacement for OSX's path_helper I was having problems setting my path the way I wanted, I turned to Apple's path_helper but found it appends system dirs to the beginning of the path. I found someone had made a start on a replacement and I forked it and made some updates.

I don't write a lot of shell scripts, and less so to replace system supplied programs. As such, I'm wondering 2 main things:

Does this follow best practices for a shell script?
Is there an obvious security flaw? I'm most reticent about the line that isn't in the script itself but that calls it

The suggestion is to put this in ~/.bash_profile or ~/.zshenv:

if [ -x /usr/local/libexec/path_helper ]; then
  eval `/usr/local/libexec/path_helper`
fi

Evals make me nervous. Is that okay in this context?

From https://github.com/yb66/path_helper

#!/bin/bash -

## A better path helper - don't put the standard bins first.

function path_helper {
  local -a path_dirs
  local envvar=$1
  local target=`echo $1 | /usr/bin/awk '{print tolower($1)}'`
  local etcpaths="/etc/${target}s"
  local etcpathsd="${etcpaths}.d/*"

  while read -r dir ; do
      if [ "${#path_dirs[@]}" == "0" ]; then
          path_dirs=${dir}
      else
          path_dirs+=":${dir}"
      fi
  done < <(/bin/cat ${etcpathsd} ${etcpaths})

  path_dirs=($(echo ${path_dirs[@]} | /usr/bin/tr [:space:] '\n' | /usr/bin/awk '!a[$0]++'))
  echo "${envvar}=\"\${${envvar}}:${path_dirs[@]}\"; export ${envvar};"
}

path_helper "PATH"

# Only handle MANPATH if the variable hasn't already been set.
if [ -z "$MANPATH" ]; then
  path_helper "MANPATH"
fi
# EOF

As to the security of this:

if [ -x /usr/local/libexec/path_helper ]; then
  eval `/usr/local/libexec/path_helper`
fi

This runs the path_helper script, which presumably outputs a string, and then the command represented by the output string is run in the local context.  So this initially presumes you trust path_helper to run anything it wants.  I don't think path_helper could much more damage by running something in your shell instead of what it already ran in a sub-shell.


Thread-safe memoizer I have searched around but I was not able to find a complete implementation of the memoize pattern in a multithreaded context. It's the first time playing with thread synchronization. The code seems to works, but I am not able to test it extensively, in particular to find if there are race conditions. Do you think there is a better implementation, maybe using more advanced synchronization mechanism?

from threading import Thread, Lock, current_thread
import thread

def memoize(obj):
    import functools
    cache = obj.cache = {}
    lock_cache = obj.lock_cache = Lock()

    @functools.wraps(obj)
    def memoizer(*args, **kwargs):
        key = str(args) + str(kwargs)
        cached_result = None
        item_lock = None
        with lock_cache:
            # do the minimal needed things here: just look inside the cache,
            # return if present, otherwise create and acquire lock  specific to a
            # particular computation. Postpone the actual computation, do  not
            # block other computation.
             cached_result = cache.get(key, None)
            if cached_result is not None:
                 if type(cached_result) is not thread.LockType:
                     return cached_result
            else:
                item_lock = Lock()
                item_lock.acquire()
                cache[key] = item_lock

        if item_lock is not None:
            # if we are here it means we have created the lock: do the
            # computation, return and release.
            result = obj(*args, **kwargs)
            with lock_cache:
                cache[key] = result
            item_lock.release()
            return result

        if type(cached_result) is thread.LockType:
            # if we are here it means that other thread have created the
            # locks and are doing the computation, just wait
            with cached_result:
                return cache[key]
    return memoizer

Here a little test:

import time

@memoize
def f(x):
    time.sleep(2)
    return x * 10

lock_write = Lock()
def worker(x):
    result = f(x)
    with lock_write:
        print current_thread(), x, result

threads = []

start = time.time()
for i in (0, 1, 1, 2, 0, 0, 0, 1, 3):
    t = Thread(target=worker, args=(i,))
    threads.append(t)
    t.start()

time.sleep(1)

for i in (0, 1, 1, 2, 0, 0, 0, 1, 3):
    t = Thread(target=worker, args=(i,))
    threads.append(t)
    t.start()

for t in threads:
    t.join()

print time.time() - start

I get the correct results and it runs in 2 seconds, as expected.

1. Review

There's no docstring. What does memoize do, and how do I use it?
The code uses str(args) + str(kwargs) as the cache key. But this means that, for example, the number 1 and the string "1" will collide.
The code is not portable to Python 3, where there's no thread module.
It's not clear to me why functools is only imported inside memoize, but threading is imported at top level.
It's not clear to me why the code assigns to obj.cache and obj.lock_cache. What if obj already has a cache property?
The locking logic is hard to follow and needs comments. As far as I can see, lock_cache protects the cache dictionary; and item_cache protects the cache slot that is currently being computed. Its necessary to have two locks because the call to obj might be recursive and so need to re-enter memoizer.
The code uses three states for each key: (i) not present in the cache; (ii) currently being computed; (iii) present in the cache. The way it distinguishes between (ii) and (iii) is by checking to see if the type is thread.LockType. But this means that you can't use memoize on a function that might return a lock. It would be better to have a more robust mechanism for distinguishing these cases.
The naming is inconsistent: lock_cache versus item_lock.
key is looked up in cache multiple times on some code paths.

2. Revised code

from functools import wraps
from threading import Lock

class CacheEntry: pass

def memoized(f):
    """Decorator that caches a function's result each time it is called.
    If called later with the same arguments, the cached value is
    returned, and not re-evaluated. Access to the cache is
    thread-safe.

    """
    cache = {}                  # Map from key to CacheEntry
    cache_lock = Lock()         # Guards cache

    @wraps(f)
    def memoizer(*args, **kwargs):
        key = args, tuple(kwargs.items())
        result_lock = None

        with cache_lock:
            try:
                entry = cache[key]
            except KeyError:
                entry = cache[key] = CacheEntry()
                result_lock = entry.lock = Lock() # Guards entry.result
                result_lock.acquire()

        if result_lock:
            # Compute the new entry without holding cache_lock, to avoid
            # deadlock if the call to f is recursive (in which case
            # memoizer is re-entered).
            result = entry.result = f(*args, **kwargs)
            result_lock.release()
            return result
        else:
            # Wait on entry.lock without holding cache_lock, to avoid
            # blocking other threads that need to use the cache.
            with entry.lock:
                return entry.result

    return memoizer

Notes:

This is untested.
This doesn't delete result_lock after the result is computed. This simplifies the logic (for example, there's no need to take cache_lock again and no need for a type check) at the cost of 40 bytes to keep the Lock object around.


Thread-safe LFU Cache I am trying to implement a thread-safe LFU cache algorithm, please review my code and would really appreciate some criticism or suggestion. (on naming, design, data structures used, anything)
Some assumptions:

capacity will be larger than 0.
If the cache is full and multiple keys have the same frequency, the least recently used will be evicted.

Here is the code:
import java.util.*;
import java.util.concurrent.locks.ReentrantLock;

public interface CacheAlgorithm<K, V> {
    public V get(K key);
    public void put(K key, V value);
}

public class LFU<K, V> implements CacheAlgorithm<K, V> {

    private final ReentrantLock lock;
    private final int capacity;
    private final TreeMap<Integer, Set<K>> frequencyToKeys;
    private final Map<K, V> entries;
    private final Map<K, Integer> frequencies;

    public LFU(int capacity) {
        this.capacity = capacity;
        lock = new ReentrantLock();
        frequencyToKeys = new TreeMap<>();
        entries = new HashMap<>();
        frequencies = new HashMap<>();
    }

    @Override
    public V get(K key) {
        try {
            V value = entries.get(key);
            if (value != null) {
                incrementFrequency(key);
            }
            return value;
        } finally {
            lock.unlock();
        }
    }

    @Override
    public void put(K key, V value) {
        try {
            if (entries.containsKey(key)) {
                incrementFrequency(key);
                entries.put(key, value);
            } else {
                if (capacity == entries.size()) {
                    removeLeastFrequentEntry();
                }
                addNewEntry(key, value);
            }
        } finally {
            lock.unlock();
        }
    }

    private void addNewEntry(K key, V value) {
        entries.put(key, value);
        setKeyFrequency(key, 1);
    }

    private void incrementFrequency(K key) {
        int frequency = frequencies.get(key);
        Set<K> keys = frequencyToKeys.get(frequency);
        keys.remove(key);
        if (keys.isEmpty()) {
            frequencyToKeys.remove(frequency);
        }
        setKeyFrequency(key, frequency + 1);
    }

    private void setKeyFrequency(K key, int frequency) {
        frequencies.put(key, frequency);
        frequencyToKeys.putIfAbsent(frequency, new LinkedHashSet<>());
        frequencyToKeys.get(frequency).add(key);
    }

    private void removeLeastFrequentEntry() {
        Map.Entry<Integer, Set<K>> entry = frequencyToKeys.firstEntry();
        Integer frequency = entry.getKey();
        Set<K> keys = entry.getValue();
        K keyToEvict = keys.iterator().next();
        keys.remove(keyToEvict);
        if (keys.isEmpty()) {
            frequencyToKeys.remove(frequency);
        }
        frequencies.remove(keyToEvict);
        entries.remove(keyToEvict);
    }
}

Few comments from my side:
Looks like a code bug:
I think you want to use ReentrantLock to manage concurrent operations in multithreaded environment. But in your code I don't see any place where you have made a call to lock (neither lock() nor tryLock(long timeout, TimeUnit unit)). While unlock() is used couple of time, which is of no use in this case.
Other suggested updated:

CacheAlgorithm interface should be rather renamed to Cache only. Because it doesn't make sense in any class to make use of word Algorithm.

Code should prevent using abbreviations as much as possible. So LFU should be renamed to LeastFrequentlyUsed. Which is not a very big name. Along that it should be ended with Cache to make it more clear to user. So the better name would be LeastFrequentlyUsedCache.

It would be better to have a default constructor with default capacity, as Java API has for Collection classes.

Avoid auto-boxing and auto-unboxing in code. As these are heavy operations. Rather do custom boxing and unboxing as below:

final int frequency = frequencies.get(key).intValue();
final Set<K> keys = frequencyToKeys.get(Integer.valueOf(frequency));


How to make Print() method memory & CPU efficient? Problem Statement
You are receiving n objects in a random order, and you need to print them to stdout correctly ordered by sequence number.
The sequence numbers start from 0 (zero) and you have to wait until you get a complete, unbroken sequence batch of j objects before you output them.
You have to process all objects without loss. The program should exit once it completes outputting the first 50000 objects Batch size j = 100
The object is defined as such:
    {
    "id" : "object_id", // object ID (string)
    "seq" : 0, // object sequence number (int64, 0-49999)
    "data" : "" // []bytes
    }

Example Output Statement
    Step                Input Value                Output State j = 1                  Output state j = 3
    0                       6
    1                       0                           0
    2                       4                           0
    3                       2                           0
    4                       1                           0,1,2                               0,1,2
    5                       3                           0,1,2,3,4                           0,1,2
    6                       9                           0,1,2,3,4                           0,1,2
    7                       5                           0,1,2,3,4,5,6                       0,1,2,3,4,5

Solution
func (receiver *Receiver) Print(seqNumber uint64, batchSize uint64, outputFile io.Writer) (error, bool) {

    fmt.Fprintf(outputFile, "[ ")
    if seqNumber >= receiver.outputSequence.length {
        receiver.outputSequence.bufferSizeIncrease(seqNumber)
    }
    receiver.outputSequence.sequence[seqNumber] = true

    printedCount := uint64(0) // check for MAX_OBJECTS_TO_PRINT
    var nthBatchStartingIndex uint64
    MaxObjectsToPrint := config.GetMaxPrintSize()
Loop:
    for nthBatchStartingIndex < receiver.outputSequence.length { // check unbroken sequence
        var assessIndex = nthBatchStartingIndex
        for j := assessIndex; j < nthBatchStartingIndex+batchSize; j++ { // Assess nth batch
            if j >= receiver.outputSequence.length { //index out of range - edge case
                break Loop
            }
            if receiver.outputSequence.sequence[j] == false {
                break Loop
            }
        }

        count, printThresholdReached := receiver.printAssessedBatchIndexes(assessIndex, printedCount, batchSize, MaxObjectsToPrint, outputFile)
        if printThresholdReached { // print sequence threshold reached MAX_OBJECTS_TO_PRINT
            fmt.Fprintf(outputFile, " ]  ")
            fmt.Fprintf(outputFile, " ----for input value %d\n", seqNumber)
            return nil, false
        }
        printedCount += count
        if printedCount >= MaxObjectsToPrint { // print sequence threshold reached MAX_OBJECTS_TO_PRINT
            fmt.Fprintf(outputFile, " ]  ")
            fmt.Fprintf(outputFile, " ----for input value %d\n", seqNumber)
            receiver.Log.Printf("****MaxObjectsToPrint threshold(%d) reached \n", MaxObjectsToPrint)
            return nil, false
        }
        nthBatchStartingIndex = assessIndex + batchSize // next batch
    }
    fmt.Fprintf(outputFile, " ]  ")
    fmt.Fprintf(outputFile, " ----for input value %d\n", seqNumber)
    return nil, true
}

Here is the complete solution, written for this problem.

Print() is the method that does heavy lifting in this code, with varying size of memory & heavy CPU usage:

How to make receiver.outputSequence memory effective by using datastructure other than array? because newBufferSize := 2 * seqNumber is doubling memory...

How to make Print method have effective CPU usage? On some goroutine

First, observe that if you receive an out-of-sequence object, you have to potentially keep all the objects from the latest printed sequence number to the received number. Worst case, you receive the object in reverse order and have to keep them all in memory. There is no way around this. So, what you can optimize is first, when you are printing objects you can find them quickly, and second, you don't want to move them around, which will happen if you use a slice and grow it as necessary.
So, a sketch of an algorithm that will use CPU and memory better than what you have is as follows:
var nextInSequence=0 // The next item you are expecting in the sequence
var storedObjects=map[int]SomeStruct{}

func Print(seqNumber int, obj SomeStruct) {
   if seqNumber==nextInSequence {
     output obj
     nextInSequence++
   } else {
      storedObjects[seqNumber]=obj
   }
   for {
     if stored, ok:=storedObjects[nextInSequence] ; ok {
        output stored
        delete(storedObjects,nextInSequence)
        nextInSequence ++
     } else {
        break
     }
  }
}


Print out all possible sizes of subsequences of string in C For example, given a string "abcdefghijk", I want my function to print out:

a, b, c, d, e, f, g, h, i, j, k.

ab, bc, cd, de, ef, fg, gh, hi, ij, jk

abc, bcd, cde, def, efg, fgh, ghi, hij, ijk

abcd, bcde

I'm sure there's a better way to do this than what I currently have. What are some optimizations I could do? Mostly in terms of speed performance, but any kind of optimization would be great; time, space, design, etc...

The commas don't matter much, it's the order that I worry about at this point, so that a, b, b, c, c, d, d, e, e, f, f, g, g, h, h, i, i, j, j, k is fine for the second line.

int main(int argc, char const *argv[])
{

    char* s = "abcdefghijk";
    int n = 11;

    for (int k = 0; k < n; k++)
    {
        for (int i = 0; i < n-k; i++)
        {
            for (int j = i; j <= i+k; j++)
                printf("%c, ",s[j]);
        }
        printf("\n");
    }

    return 0;
}

Here's a way that (at least IMO) at least marginally cleaner:

#include <stdio.h>

int main(int argc, char const *argv[])
{

    char s[] = "abcdefghijk";
    int n = sizeof(s);

    for (int k = 1; k < n; k++)
    {
        for (int i = 0; i < n - k; i++)
        {
            printf("%*.*s, ", k, k, s + i);
        }
        printf("\n");
    }

    return 0;
}

As to things about the code that can be improved, the most obvious would be using sizeof (or strlen, if necessary) to find the length of the input array, instead of hard-coding it.

Realistically, there's probably not a lot you can do to improve speed/performance--it's virtually guaranteed to be heavily I/O bound (though Tom Ernack's suggestion of using fwrite instead of printf might help--then again, depending on the standard library you're using, it might not).


Archiving data while running update statement I'm using sql server and I'm  trying to update certain columns in a table but before it updates the data, I want to insert the data into another table.

This is my code

declare @table1 table (Id int, Name varchar(100))
declare @temp table (Id int, Name varchar(100), CreatedOn datetime default getdate())

insert into @table1 values (1, 'Albert')
insert into @table1 values (2, 'Alex')
insert into @table1 values (3, 'Alas')
insert into @table1 values (4, 'Bob')

update @table1
set Name = 'B' + Name
output Deleted.Id, Deleted.Name, getdate() into @temp
where Name like 'A%'

select * from @table1
select * from @temp

Above code seems to be working correctly. Please let me know if I have to change anything.

You don't need to change anything as long as the code performs what you need. If you want to insert records to another table on every update (and not only in this place) you should probably take a look at triggers


Decoding custom packets with Python using Twisted This server require fast decoding of packets from clients, as I am expecting 5000 concurrent clients that is sending packets possibly every second. This is a game server (5000 players walking send packets each n every step).

I want to see if my implementation is fast enough or there are still room for improvements. The encryption isn't on this code as I only want to tackle the decryption for now.

The decode function is taken from a JavaScript implementation, This is the best output I got after trying to translate it. (though the translation is correct because the results are same from JavaScript and now Python)

def dataReceived(self, data):
    packet = decode(data)
    (counter, packet_id, username, password, unknown) = unpack('<B H 16s 16s 16s', packet)
    print counter
    print packet_id
    print username
    print password
    print unknown

For example the above implementation is pretty straight forward but the concern here is the decode(data) part.

I am new to Python and Twisted so I had to do some maybe absurd way on implementing this.

This is my decode function:

def decode(packet_data):
    cl = 0x6e
    internal_rounds = 2
    i = 0

    seqX = bytearray.fromhex('HEXSTRINGREMOVED')
    seqY = bytearray.fromhex('HEXSTRINGREMOVED')
    result = []

    while i < (len(packet_data) - 4):
        dl = ord(packet_data[i])
        bl = seqX[(i * 2) + 1]
        al = seqX[i * 2]
        bl2 = seqY[(i * 2) + 1]
        dl2 = seqY[i * 2]
        j = internal_rounds - 1

        while j > 0:
            if bl <= dl:
                dl -= bl
            else:
                bl = (~bl) & 0xFF
                bl += 1
                dl += bl

            dl ^= bl2
            j -= 1
        if al <= dl:
            dl -= al
        else:
            al = (~al) & 0xFF
            al += 1
            dl += al
        dl ^= dl2
        dl ^= cl
        result.append(dl)
        cl ^= dl
        i += 1

    return "".join(map(chr, result))

I did this dl = ord(packet_data[i]) because I keep getting int -= char error.

Also did this return "".join(map(chr, result)) because I kept getting errors with unpack because the return was previously return result which result is an array.

So what do you think?  Should I also have used unpack_from instead? But I don't know how to make a buffer in Python. Is the data received already a buffer?

It is hard to review something when you can't test whether your reviewed code actually works, so with that disclaimer lets see what we can do with your code.

Regarding style issues, your variable names are not good, and doesn't convey any useful informations. But leave that as it is...

Here are some elements I've incorporated in the refactoring below:

Your code sets j=internal_rounds - 1 which in effect sets j=1, so that the while loop never executes more than once. In other words it can be removed
You calculate i*2 and i*2 +1 twice for each loop, which could be simplified to some addition if we add a few more variables. That is i is expanded into i2 and i2_1, in the hope that 3 additions on those is cheaper than 3 additions and 4 multiplications. :)
Several of your steps change a variable temporary before using the result to change the dl variable, so I've compressed those a little
Remove the calculation of the loop limit for every loop

Is my refactored code faster than yours? You've got to test, as I can't. The same applies for the correctness, but I think it should be the same.

def decode(packet_data):
    i = 0
    i2 = 0
    i2_1 = 1
    packet_length = len(packet_data) - 4

    seqX = bytearray.fromhex('HEXSTRINGREMOVED')
    seqY = bytearray.fromhex('HEXSTRINGREMOVED')
    result = []

    cl = 0x6e

    while i < packet_length:
        dl = ord(packet_data[i])
        bl = seqX[i2_1]
        al = seqX[i2]
        bl2 = seqY[i2_1]
        dl2 = seqY[i2]

        dl += -bl if bl <= dl else (~bl & 0xFF + 1)
        dl ^= bl2

        dl = -al if al <= dl else (~al & 0xFF + 1)
        dl ^= dl2 ^ cl

        result.append(dl)
        cl ^= dl

        i += 1
        i2 += 2
        i2_1 += 2

    return "".join(map(chr, result))

Another micro-change you could try out, is to change the handling of result to:

result = ''
...
result += chr(dl)
...
return result

It could be faster (or slower), as it skips the array handling and the join/map operations, but it uses a string concatenation instead...


C++ Class wrapper on top of WinMain/DllMain I've written some code to do the job for me. The final results seems to work fine, but I would like to know if this is how I should Handle classes. And where I can improve or things which are just plain stupid.

class CInit
{
public:
    CInit();
    ~CInit();

    void Check(bool);

    virtual bool Init();
    virtual bool ExInit();

private:
    static CInit* _ThisInit;
};
CInit* CInit::_ThisInit;

CInit::CInit()
{
    if(_ThisInit == 0)
        _ThisInit = this;
}

CInit::~CInit()
{

}

void CInit::Check(bool bStart)
{
    if(bStart)
        _ThisInit->Init();
    else
        _ThisInit->ExInit();
}

bool CInit::Init()
{
    return true;
}

bool CInit::ExInit()
{
    return true;
}

This is my cInit class, as you may see this has a static value, because in when the program execetues it will first run the global class instance which will be used like this. It will overload Init() and there the program starts.

    CRaPE mycp;

bool CRaPE::Init()
{
    return true;
}

bool CRaPE::ExInit()
{
    return true;
}

The program runs from here:

CInit* _Init;

void HandleStartUp()
{
     _Init = new CInit();
    _Init->Check(true); // So Init gets called
}

void HandleExit()
{
    _Init->Check(false); // so ExInit gets called
    delete _Init;
}

BOOL WINAPI DllMain(
    HINSTANCE hinstDLL,
    DWORD fdwReason,
    LPVOID lpReserved )
{
    switch( fdwReason )
    {
    case DLL_PROCESS_ATTACH:
        {
            HANDLE hThread = CreateThread(0,0,(LPTHREAD_START_ROUTINE)HandleStartUp,0,0,0);
            if(hThread == 0)
                return false;
        }
        break;

    case DLL_THREAD_ATTACH:
        break;

    case DLL_THREAD_DETACH:
        break;

    case DLL_PROCESS_DETACH:
        HandleExit();
        break;
    }
    return TRUE;
}

I hope I explained well, thanks for reading.

Gz

it's better to explicitly call init and exinit than pass a bool to "Check" to say which will one will get called.  maybe call them "Start" and "Exit"

Also, I'd suggest not making them virtual, if you want to be able to extend it...use a protected virtual

public: bool Start();
protected: virtual bool StartImpl();

void CInit::Start()
{
   StartImpl();
}

This gives you a bit more control over the calling in to your API.  (Also a mostly recommended best practice for C++).

It's called "Non Virtual Interface" or NVI.  It does have pros and cons.  But in this case I think it would be quite useful.


A safer way to return a generic indicator object indicating failure I'm writing a singly-linked list that implements the List interface. I decided going in that I'd try to implement every method as an exercise; even the optional ones.

I started writing the ListIterator, and found the need to return the node previous to a given node for operations like add. Unfortunately, I didn't anticipate this scenario prior to writing the rest of the list, and have no way of indicating failure (failure being that the given node doesn't exist in the list), since null is a valid return if the root node is supplied. I decided to allow the caller to pass in an indicator node that would be returned if the node wasn't found. I wasn't sure the best way to handle this thought, since Node is generic.

I ended up making the indicator of type Node<Object>, and casting it prior to returning. I know this is dangerous though, so I made it very clear in the documentation that the method isn't safe (plus, of course, it's private).

I'd like to know if there's a less hack-ish way of dealing with this scenario though.

I didn't think including the full implementation was necessary. The names should be obvious enough (I hope).

/**
 * Dangerous.
 * Returns the node that comes before the given node, or badNode if the
 *  node doesn't exist in the list.
 * The return must be checked against badNode prior to use!
 *
 * @param node The node to find the predecessor of
 * @param badNode The indicator that will be returned if node isn't in the list
 * @return The predecessor of node, or badNode if node doesn't exist.
 */
@SuppressWarnings("unchecked")
private Node<E> getNodeBefore(Node<E> node, Node<Object> badNode) {
    Node<E> curNode = getRootNode();
    Node<E> prevNode = null;

    while (curNode != null) {
        if (curNode == node) {
            return prevNode;
        }

    prevNode = curNode;
    curNode = curNode.getNextNode();
    }

    return (Node<E>)badNode;
}

A fairly clean way is to use Optionals, like these or those. So you would make getNodeBefore return an Optional<Node<E>>.

The badNode idea is, hm, exotic to me.


Does iterating through a large data set in a Map each time i click impact the performance? I need to get values from a Map by Adapter position i get from a RecyclerView.

As you can see each time i click on an Album Art i create a new array of album objects.

final Album[] albums = new Album[albumMap.size()];
      for (Map.Entry<Integer,Album> e : albumMap.entrySet()){
      albums[i++] = e.getValue();
 }

Then i get the album like this: String selectedAlbum = albums[position].getAlbum();

But what if someone has like 10000+ albums on his device and each time an album is clicked a new array of album objects is created and i iterate through it just to get the album name.

Would this have an impact on performance if there are alot of albums present?

TL;DR Is this code bad?

Full code

@Override
    public void onClickAlbum(int position, Map<Integer,Album> albumMap) {
        if (getActivity() != null) {
            int i = 0;
            final Album[] albums = new Album[albumMap.size()];
            for (Map.Entry<Integer,Album> e : albumMap.entrySet()){
                albums[i++] = e.getValue();
            }
            String selectedAlbum = albums[position].getAlbum();
            Main.getInstance().setSongsFilteredBy(SongsLibrary.getInstance().getSongsByAlbum(selectedAlbum));
            Intent intent = new Intent(getActivity(), ListSongsActivity.class);
            intent.putExtra("title", selectedAlbum);
            startActivity(intent);
            Toast.makeText(getActivity(), "test: " + selectedAlbum, Toast.LENGTH_SHORT).show();
        }
}

Technical improvements

use albumMap.values() to get a Collection<Album> directly instantiated as list, which supports positional access get(index). So no need for array and filling loop. See Listing 1.
use some of Android's ListView interfaces which usually provide a convenient method on their underlying data model like getItemAtPosition(position). See Listing 2.

Listing 1: Q: How to convert map to list

List<Album> albums = new ArrayList<Album>(albumMap.values());

Listing 2: Vogella's Tutorial on Android's ListView

ListView albumsView = (ListView) findViewById(R.id.listview);

// fill albums data to model (adapter implementation)
albumsView.setAdapter(adapter)

albumsView.setOnItemClickListener(new OnItemClickListener() {

  @Override
  public void onItemClick(AdapterView<?> parent, final View view, int position, long id) {
    final Album album = (Album) parent.getItemAtPosition(position);
    // your implementation
  }
}

Design improvements

album.getAlbum() gets the title of an album, so rename property|getter to title|getTitle()
keep the event-handler short and extract all action/intend creation into separate method

References

Answer on using RecycleView with OnItemClickListener
Tutorial: Android working with RecyclerView
Android Official Guide on AdapterView


Replacing items in a list I was wondering why replace is not in base. I thought, it's because it must be easy to implement with standard combinators. Although I do not like prefix syntax of Applicative I wrote it this way:

replace a b = map (bool <$> id <*> (const b) <*> (== a))

And was slightly surprised after seeing replace in Data.List.Utils:

replace old new l = join new . split old $ l

Are there non-opinion based reasons to prefer one than the other in public projects?

Your code is not a replacement for replace (pun intended).  Its type signature is wrong.  Your code has type Eq b => b -> b -> [b] -> [b], but replace should have type Eq a => [a] -> [a] -> [a] -> [a].

<$> and <*> are infix, not prefix.

I'd prefer this to your implementation:

replace' :: Eq b => b -> b -> [b] -> [b]
replace' a b = map (\x -> if (a == x) then b else x)

It does the exact same thing in the exact same way, but is simpler and more readable (even a novice Haskell programmer who has never heard of bool or <$> or <*> can read it).  Even more important: it has a type signature.

If you'd decided to cut-and-paste the type signature of the function you wanted to replicate before starting, you would have found out that your version doesn't work as a replacement at compile time -- and if you'd looked at and thought about the type signature, you probably would have wondered why the extra []s were there, and figured it out even earlier.


Batch file-download script - is it necessary to re-phrase commands? I run a script to download a file and unzip it. An import process then runs and leaves the files in their source location. I wrote the fully functioning code below to clean up that directory to be ready to use the next night.

I streamlined as much as I believe possible. I realize ~15 lines of code isn't exactly verbose or complicated and I've included the full script for reference. I feel, however, that specifically the second block - line SET DATE= through line DIR - can be better written but I don't know in what way. Am I simply seeing messy code where none exists?  It just doesn't seem to 'read' well.

A note - the index.txt file is the same despite having two paths and is located in one or the other depending on the outcome (errors or not) of the import.

SET PATH="\\server\path\path2"
IF NOT EXIST %PATH%\Upload.zip GOTO Done

SET DATE=%date:~4,2%-%date:~7,2%-%date:~10,4%
MKDIR %PATH%\Archives\%date%
SET ARCHIVE=%PATH%\Archives\%date%\
DIR %PATH% /B > %ARCHIVE%log-%date%.txt

MOVE %PATH%\Upload.zip %ARCHIVE%Upload-%date%.zip
MOVE %PATH%\index.txt %ARCHIVE%index-%date%.txt ||(
MOVE %PATH%\error_files\index.txt %ARCHIVE%index-%date%.txt
)
DEL %PATH%\*.tif
RMDIR %PATH%\error_files

PUSHD %PATH%\Archives &&(
FORFILES /S /D -15 /C "CMD /C IF @isdir == TRUE RD /S /Q @path"
) & POPD

:Done

I don't think there's a better way to format your %DATE% variable. And be careful, because I think this won't work in all locales. I had this problem a few years ago, and I remember that there was no good solution to format dates using native commands. I had to make a simple dateformat utility myself to generate date formats easily and in a locale independent way.

Just a small thing, I recommend NOT to put the trailing \ in the %ARCHIVE% variable. So instead of this:

SET ARCHIVE=%PATH%\Archives\%date%\
DIR %PATH% /B > %ARCHIVE%log-%date%.txt

MOVE %PATH%\Upload.zip %ARCHIVE%Upload-%date%.zip

Write like this:

SET ARCHIVE=%PATH%\Archives\%date%
DIR %PATH% /B > %ARCHIVE%\log-%date%.txt

MOVE %PATH%\Upload.zip %ARCHIVE%\Upload-%date%.zip

This may be a matter of taste, but I like to see explicitly that it's a directory there, not just a prefix.

Another formatting tip about this:

MOVE %PATH%\index.txt %ARCHIVE%\index-%date%.txt ||(
MOVE %PATH%\error_files\index.txt %ARCHIVE%\index-%date%.txt
)

I find it easier to read when the commands inside (...) are indented, like this:

MOVE %PATH%\index.txt %ARCHIVE%\index-%date%.txt || (
    MOVE %PATH%\error_files\index.txt %ARCHIVE%\index-%date%.txt
)


Finding nth Prime using Python and Sieve of Eratosthenes I'm currently working through the Project Euler Problems using HackerRank to evaluate the code and I'm stuck on the 7th Problem.

"""
    By listing the first six prime numbers: 2, 3, 5, 7, 11, and 13, we can see that the 6th prime is 13.

    What is the 10 001st prime number?

"""
import pytest

def find_primes(limit):
    nums = [True] * (limit+1)
    nums[0] = nums[1] = False

    for (i, is_prime) in enumerate(nums):
        if is_prime:
            yield i
            for n in range(i*i, limit+1, i):
                nums[n] = False

def find_n_prime(n):
    for i in range(n, (n*n)+1, n):
        primes = list(find_primes(i))
        if len(primes) >= n:
            return primes[n-1]

def test_find_n_primes():
    assert find_n_prime(2) == 3
    assert find_n_prime(3) == 5

    assert find_n_prime(10) == 29
    assert find_n_prime(15) == 47
    assert find_n_prime(81) == 419

    assert find_n_prime(941) == 7417
    assert find_n_prime(1000) == 7919

    assert find_n_prime(10001) == 104743

The code only completes the first test case on hackerRank and fails for #2 test case and times out for the rest.

How can I improve the Code ?

Upper bound for p_n

There is a known upper bound for the n-th prime.

It means that you don't need any loop inside find_n_prime, and you don't need to check if len(primes) >= n either.

import pytest
from math import log, ceil

def find_primes(limit):
    nums = [True] * (limit + 1)
    nums[0] = nums[1] = False

    for (i, is_prime) in enumerate(nums):
        if is_prime:
            yield i
            for n in range(i * i, limit + 1, i):
                nums[n] = False

def upper_bound_for_p_n(n):
    if n < 6:
        return 100
    return ceil(n * (log(n) + log(log(n))))

def find_n_prime(n):
    primes = list(find_primes(upper_bound_for_p_n(n)))
    return primes[n - 1]

It calculates the 100000th prime in less than 230ms on my computer, compared to 1.5s for your code.

itertools.islice

Another possible optimization would be to use itertools.isclice to get the n-th prime out of the find_primes generator, without converting it to a list.

from itertools import islice

def find_n_prime(n):
    primes = find_primes(upper_bound_for_p_n(n))
    return next(islice(primes, n - 1, n))


How can this rails controller test be cleaner refactored? I tend to write quite explicit tests for my rails controllers exposing APIs with two seperate concerns:

meeting the defined API-Response with headers headers and the response itself
ensuring the right calls to underlaying methods/objects are made

However when the calls are not simply executable in a test the controller tends to get bloated.
This is due to the fact that when the calls are stubbed out the execution order of the two test concerns is reversed:

for the api calls:

first stub the call method (since we don't want this to be run)
make the request
test the response

for the method calls

define the receive-expectations
make the request

Example Code:

require 'spec_helper'

describe ItemsController do
  render_views

  shared_examples :empty_success_response do
    it { expect(response.status).to eql(202) }
    it { expect(response.content_type).to eql('application/json')}

    it 'returns error message' do
      json = JSON.parse(response.body)
      expect(json).to be_blank
    end
  end

  context 'a single item' do
    subject { FactoryGirl.create(:item) }

    describe '#destroy' do
      let(:do_action) do
        delete :destroy, id: subject.id.to_s
      end

      describe 'response' do
        before do
          controller.stub!(:destroy_item)
          do_action
        end

        it_behaves_like :empty_success_response
      end

      it 'makes the call' do
        expect(controller).to receive(:destroy_item).once.times.with(subject.id.to_s)
        do_action
      end
    end

    # ...

  end

end

Note: The destroy_item method is just a stand in, the actual method makes more sense ;)

The code in the example works nicely, and is even fairly dry (mostly because the request is moved into a let), but feels still very chunky and bloated - especially if we consider this is just one simple actions, with a usual controller having quite a few more real logic to test.

Does anyone have an idea how to make this code more readable?

Or does it maybe make sense to split the 2 kinds of tests completely? This would however mean quite a bit of duplication since we have the same caller-code in 2 places.

shared examples share the let scope, so you could further dry up your code (assuming you use do_action for every action) to this:

shared_examples :empty_success_response do

  before do
    do_action
  end

 ...
end

Also, if you are using do_action, I also like to add to my scopes:

describe '#destroy' do
  let(:do_action) do
    delete :destroy, id: subject.id.to_s
  end

  after do
    do_action
  end

  ...
end

This way, you don't have to call do_action if all you do in the test is set expectations.

it 'makes the call' do
  expect(controller).to receive(:destroy_item).once.times.with(subject.id.to_s)
end

If in some tests you want do_action to run before the end of the test - that's no problem, since let runs the code only once - so it won't run again at the end of that specific test!