Alert about duplicates and remove them So I want ask if I'm doing it right way.

Im still learning..

I want check textarea duplicates and alert if duplicate and remove it.

$(document).ready(function(){
  $('#lol').keypress(function(){
    var data = $('#lol').val().split('\n');
    var result = data.filter((word, i, arr) => arr.indexOf(word) === i);
    $('#lol').val(result.join('\n'));
    data.unique();
  });
});

(function() {
    "use strict";

    Array.prototype.unique = function unique() {
        var self = this;
        return self.filter(function(a) {
            var that = this;
            // console.log(that);
            if(!that[a]) {
                return that[a] = true;
            } else {
                alert("duplicate");
                return that[a] = false;
            }
        }, {});
    }
})();

Take care extending built in objects

Extending the prototype of builtin objects should be done with care. The best way is via Object.defineProperty  and first check if something has not already done so. (See 2nd rewrite)

Why create two copies of this.. self and that. Why  not just use this and create a map object with a meaningful name. (see 2nd rewrite)

Don't add functionality outside the role of the function. Array.unique Defiantly should not be calling an alert (in fact you can not trust alerts (clients can turn them off) so you should never use them)

You have actually repeated the process of removing duplicates in the keypress handler. You could just have checked if the length of data and result are different and alerted the client from that. (see 1st rewrite)

Rewrites
First quick rewrite showing you don't need Array.unique
$(document).ready(function(){
  const lol = $('#lol');
  lol.keypress(function(){
    const data = lol.val().split('\n');
    const result = data.filter((word, i, arr) => arr.indexOf(word) === i);
    lol.val(result.join('\n'));
    result.length !== data.length && (alert("duplicate"));
  });
});

The second rewrite showing a safe way to add to inbuilt prototypes.
Note that the options configurable, enumerable, writable are set to false. Particularly you MUST set enumerable to be false or you could end up with all sorts of bugs appearing in existing code.
You should also learn to not use jQuery. It has had its day and will just set you back as it shelters you from learning modern JS and DOM interfaces.

"use strict";
lol.focus();
lol.addEventListener("keyup", event => {
    const data = lol.value.split("\n");
    const result = data.unique();
    info.textContent = result.length !== data.length ? "Duplicate removed" : "";
    lol.value = result.join('\n');
});

if (!Array.prototype.unique) {
    Object.defineProperty(Array.prototype, "unique", {
        configurable: false,  // these 3 properties default to false so not needed
        enumerable: false,    // But have added them just to show their availability.
        writable: false,
        value: function() {
            const existing = {};
            return this.filter(a => existing[a] = !existing[a] ? true : false);
        }
    });
} else {
    throw new Error("Array.prototype.unique already defined.");
}
<textarea id="lol"></textarea>


Simple Caesar shift and deshift I wrote this program to do a simple Caesar shift by a user inputted key, and then deshift.
I'm really enjoying it, but I've run out of ideas on improvements!  Can you think of anything?

def decrypt():
    a=raw_input("Give me the word to decrypt:")
    number=input("What was it shifted by?")
    b=list(a)
    str(b)
    c=[ord(x)for x in(b)]
    d=[]
    for i in c:
        d.append(i-number)
    e=[chr(i) for i in (d)]
    e="".join(e)
    print "Decryption Successful, your word is",e,"!"

def encrypt():
    a=raw_input("Give me a word:")
    number=input("Give me a number:")
    b=list(a)
    str(b)
    c=[ord(x)for x in(b)]
    d=[]
    for i in c:
        d.append(i+number)
    e=[chr(i) for i in (d)]
    e="".join(e)
    print "Your Caesar shifted result (ascii code) is:",e,"!"
    print "Your key is", number, ",remember that!"

def menu():
    print "\n\n\nWelcome to the Caesar Shifter."
    print "What would you like to do?"
    print "Option 1:Encrypt Word"
    print "Option 2:Decrypt Word"
    print "If you would like to quit, press 0."
    choice=input("Pick your selection:")
    if choice==1:
        run=encrypt()
        run
        menu()
    elif choice==2:
        derun=decrypt()
        derun
        menu()
    elif choice==0:
        quit
    else:
        print"That is not a correct selection, please pick either 1, or 2."

menu()

This might not be quite what you've got in mind, but one big improvement you could make would be to use meaningful variable names and insert whitespace.

def decrypt():
    cyphertext = raw_input('Give me the word to decrypt:')
    shift = input('What was it shifted by?')

    cypher_chars = list(cyphertext)
    str(b) # this does nothing; you should remove it
    cypher_ords = [ord(x) for x in cypher_list]
    plaintext_ords = []
    for i in cypher_ords:
        plaintext_ords.append(i - shift)

    plaintext_chars = [chr(i) for i in plaintext_ords]
    plaintext = ''.join(plaintext_chars)
    print 'Decryption Successful, your word is', plaintext, '!'

Of course you could actually compress much of this into a one-liner. But for a new programmer, I'd suggest sticking with readable variable names that make it clear what's going on.

Still, you could do that while compressing the code a bit:

def decrypt():
    cyphertext = raw_input('Give me the word to decrypt:')
    shift = input('What was it shifted by?')

    cypher_ords = [ord(x) for x in cyphertext]
    plaintext_ords = [o - shift for o in cypher_ords]
    plaintext_chars = [chr(i) for i in plaintext_ords]
    plaintext = ''.join(plaintext_chars)
    print 'Decryption Successful, your word is', plaintext, '!'


Net health monitoring This is a little net health monitoring project I made in NodeJS. It pings Google every second and stores the result (true/false) along with the date in a database (MongoDB). I've collected enough data and I want to represent it for the past week.

I created a 2x2 array of 7 days and 24 hours each. I go through each record and map it to the appropriate day and hour of the week. And I give that hour a little score: if true +1, if false -5. And it creates a little matrix like this:

[
    [2015, 1724, 1733, 1679, ...
    [1818, 1909, 1614, 1829, ...
    [1632, 1778, 1726, 1657, ...
    ...
]

The only problem is computation takes ~10-15 seconds and lots of CPU for going through ~200k records of just 3 days.

Is there some scope for improvement here?

days = days || 3;
var counter = 0
this.find({
    date: {
        // Date greater than (Now - X days)
        $gt: new Date(Date.now() - days * 24 * 3600 * 1000),
    },
}, function(err, datums) {
    var week = new Array(days);
    for (var i = 0; i < week.length; i++)
        week[i] = new Array(24);
    // Empty Week Matrix (7x24)
    datums.forEach(function(data) {
        counter++;
        var date1 = data.date;
        var date2 = new Date();
        var timeDiff = Math.abs(date2.getTime() - date1.getTime());
        var diffDays = Math.ceil(timeDiff / (1000 * 3600 * 24));
        //stackoverflow.com/q/3224834/1266650

        var day = days - diffDays;
        if (day < 0) return;
        var hour = data.date.getHours();

        if (!week[day][hour]) week[day][hour] = 0;
        if (data.isNetAlive)
            week[day][hour]++;
        else
            week[day][hour] -= 5;
    });
    console.debug(week);
    console.debug(counter, 'records');
});

You might want to move out 1000, 3600, 24 to "constants" so that they're properly described. Something like:

var HOURS_A_DAY = 24;
var MILLISECONDS_A_SECOND = 1000;
var SECONDS_AN_HOUR = 3600;

In addition, you can name your variables properly so that they're easily understood. timeDiff could be timeDifference, diffDays could be daysDifference.

Creating date objects per iteration is costly. Try operating using milliseconds instead. Make data.date return milliseconds for date1. Then use Date.now() on date2. Also, if possible, avoid doing getTime() and getHours(). Try calculating them manually.

Next, Math.abs can be done using a bitwise operation. The same could be done for Math.ceil, just look around the internet. Here's an example of a bitwise Math.abs, where n is the number to absolutify.

return (n < 0) ? (~n + 1) : n

Bitwise isn't the solution to all math performance problems and highly depends on the JS engine running. Some bitwise operations are faster than their high-level counterparts, some the other way around. Highly depends on the combination, so try checking which ones work for you.

if (!week[day][hour]) week[day][hour] = 0;

Property access and condition evaluation can be costly. Move this out to the for-loop that builds your matrix. This has nothing to do with the data crunching. It's just initializing a hole in the array to 0.

Lastly, for a more beautiful code, let's make everything less redundant and more consistent-looking.

week[day][hour] += data.isNetAlive ? 1 : -5;


Flashcard CLI app in Golang This is a simple flashcard app. With flags to control the sides of the flashcards and the number of flashcards in the session.

All the flashcards without any input at the prompt, are deleted from the map. And ones with an input, remain. The idea is that, the user marks the flashcard that they want to revise in case they had forgotten it, later in the session.

The outer loop, loops until the length of the map is >= 1

package main

import (
    "encoding/csv"
    "flag"
    "fmt"
    "log"
    "math/rand"
    "os"
    "os/exec"
    "runtime"
    "time"

    "github.com/fatih/color"
)

var (
    flagNoColor = flag.Bool("no-color", false, "disable color output")
    csvPath = flag.String("src", "src.csv", "source")
    side = flag.Int("s", -1, "side")
    num = flag.Int("n", -1, "number")
)

var clear map[string]func()

func init() {
    clear = make(map[string]func())
        clear["linux"] = func() {
        cmd := exec.Command("clear")
        cmd.Stdout = os.Stdout
        cmd.Run()
    }
    clear["windows"] = func() {
        cmd := exec.Command("cmd", "/c", "cls")
        cmd.Stdout = os.Stdout
        cmd.Run()
    }
}

func CallClear() {
    value, ok := clear[runtime.GOOS]
    if ok {
        value()
    } else {
        panic("Panic!")
    }
}

func random(min int, max int) int {
    return rand.Intn((max-min) + 1) + min
}

func main() {

    flag.Parse()

    file, err := os.Open(*csvPath)
    if err != nil {
        log.Println(err)
        return
    }
    defer file.Close()

    csvReader := csv.NewReader(file)

    csvData, err := csvReader.ReadAll()
    if err != nil {
        log.Println(err)
        return
    }

    qaRaw := make(map[string]string, len(csvData))
    for _, data := range csvData {
        qaRaw[data[0]] = data[1]
    }

    qaPair := make(map[string]string)

    if *num != -1 {
        i := 1
        for one, two := range qaRaw {
            if i <= *num {
                qaPair[one] = two
            }
            i++
        }
    } else {
        for one, two := range qaRaw {
            qaPair[one] = two
        }
    }

    done := make(chan bool)

    go func() {
        if *flagNoColor {
            color.NoColor = true
        }
        white := color.New(color.FgWhite)
        boldWhite := white.Add(color.Bold)

        qNum := 0

        for len(qaPair) >= 1 {
            for one, two := range qaPair {
                rand.Seed(time.Now().UnixNano())
                randomNum := random(1, 2)
                if *side != -1 {
                    randomNum = *side
                }
                var userInput string
                CallClear()
                boldWhite.Printf("\n #\t%d / %d\n", qNum+1, qNum+len(qaPair))
                qNum++
                if randomNum == 1 {
                    boldWhite.Printf("\n Q\t%s", one)
                    fmt.Scanln(&userInput)
                    boldWhite.Printf("\n A\t%s", two)
                    fmt.Scanln(&userInput)
                    if len(userInput) == 0 {
                        delete(qaPair, one)
                    }
                } else if randomNum == 2 {
                    boldWhite.Printf("\n Q\t%s", two)
                    fmt.Scanln(&userInput)
                    boldWhite.Printf("\n A\t%s", one)
                    fmt.Scanln(&userInput)
                    if len(userInput) == 0 {
                        delete(qaPair, one)
                    }
                }
                CallClear()
            }
        }
        done <- true
    }()

    select {
        case <-done:
            fmt.Println("blah blah blah")
    }

}

Flashcards example data (.csv file)

"hola","hi"
"sí","yes"
"¿qué pasa?","what's up?"
"¡vamos!","let's go!"
"¡salud!","cheers!; bless you!"
"no","no"
"por favor","please"
"lo siento","I'm sorry"
"buenos días","good morning"
"buenas noches","good night"
"gracias","thank you"
"vale","okay"
"hasta luego","see you later"
"adiós","goodbye"
"cómo","how (questions)"

var clear map[string]func()

To handle OS specific implementations you may consider using Golang tags feature. See it in action:

flashcard_windows.go

// +build windows

package main

import(
    "os"
    "os/exec"
)

func Clear() {
    cmd := exec.Command("cmd", "/c", "cls")
    cmd.Stdout = os.Stdout
    cmd.Run()
}

flashcard_unix.go

// +build linux

package main

import(
    "os"
    "os/exec"
)

func Clear() {
    cmd := exec.Command("clear")
    cmd.Stdout = os.Stdout
    cmd.Run()
}

To build run go build with -tags windows or -tags linux arguments.

Now there is no need call panic(). go build will exit if implementation of Clear function is missing.

See this for more information.


Modified Sieve of Eratosthenes has very long runtime I am slowly moving through Project Euler, and have reached problem #10:

  The sum of the primes below 10 is 2 + 3 + 5 + 7 = 17.

  Find the sum of all the primes below two million.

I did a naive implementation first, which was very slow (15+ minutes runtime).  But since I know the last number in the series, the Sieve of Eratosthenes should be far swifter, yet I still find it very slow.  I'm wondering whether I've mis-implemented a list somewhere so I go through it far more than I should.

Assuming I haven't messed up with the list, is there some other thing that might be the slow-down, or is this just a very intensive task that should take a long time?

#include <list>
#include <iterator>
#include <iostream>

int main(){

    std::list<long long> candidates;

    candidates.begin();

    for(long long i = 2; i<2000000; i++){

        candidates.push_back(i);
    }

    long long sieve      = 0;
    long long runningSum = 0;
    long long lastSieve  = 1415;

    std::list<long long>::iterator outer = candidates.begin();
    std::list<long long>::iterator inner = candidates.begin();

    while(outer != candidates.end()){

    sieve = *outer;
    outer++;
    inner = outer;

        while(sieve < lastSieve){

            if( *inner % sieve == 0){
                inner = candidates.erase(inner);
            }
            else{
                inner++;
            }
        }
    }

    for(std::list<long long>::iterator output = candidates.begin(); output != candidates.end(); output++){

    runningSum += *output;
    }

    std::cout << "\nTotal: " << runningSum << std::endl;
    return 0;
}

First of all some stylistic remarks:

Unnecessary vertical whitespace

You have many empty lines that don't improve readability (and actually hurt because the reader needs to do more scrolling)

Inconsistent indentation

You need to stick to one indentation style and not switch between several.
Make sure to always use the same depth of indentation.

This facilitates faster parsing of the code's structure while reading it.

Use C++11

While it might not be available to you or scares you of because of the many new features, I can only advice you to use the improvements that came with C++11.

Foremost you should go for the auto keyword and range based for loops.
The latter gives a much stronger hint at what a loop iterates:

for(std::list<long long>::iterator output = candidates.begin(); output != candidates.end(); output++){

    runningSum += *output;
    }

becomes

for(auto output : candidates) {
    runningSum += output;
}

Use standard algorithms

There are several instances in your code where you reimplemented standard algorithms to some extent:

The last loop could be replaced by:

long long runningSum += std::accumulate(candidates.begin(), candidates.end(), 0);

Performance

The first thing anyone should do regarding performance problems is profiling. Most of the time our guesses about what is slow are actually wrong.

Yet, I want to give some pointers on what could be hindering performance:

std::list provides poor cache locality, especially when it has been thinned out very much (and especially when it is so big as yours)
the original sieve works via scattering not gathering: in your algorithm each number in the list (after the current prime) has to be tested against each sieve value in the original there is no testing but only striking out values that are multiples of the current prime

Usually prime sieving is done with an array like interface that allows to strike out elements via random access. I am keen to know why you have chosen a list instead.

Random remarks

What is the line

candidates.begin();

good for?

Why is the value of lastSieve hardcoded? You should calculate it from the size of the sieve itself.

Your variables are declared too early:

runningSum is only needed at the end
sieve can be declared locally in the while loop
inner can be declared locally in the while loop

This hints at a more fundamental problem of your code:

Use more functions

Some parts of your code could gain from functions that give names to what you are doing there.

The inner while loop could be moved into a function removeMultiples.

Your whole problem code should be wrapped into a function to be reused later on (hint: project euler will require efficient prime calculation later on!)


Trim certain characters from a string in JavaScript I want to remove not only spaces, but certain characters, as well from the beginning or end of a JavaScript string.

function trim(str, characters) {
  var c_array = characters.split('');
  var result  = '';

  for (var i=0; i < characters.length; i++)
    result += '\\' + c_array[i];

  return str.replace(new RegExp('^[' + result + ']+|['+ result +']+$', 'g'), '');
}

The function can be used as follows:

trim(' - hello**   +', '+*- ');

The escaping is required as + * . etc. would be used in the regular expression. Is there a better way to trim any character in JavaScript? Or a better way to do the escaping, as not every character needs it?

There are many different ways to tackle trimming strings, a regex is probably the most popular, and there are lots of blogs and performance tests out there, (one such blog), that you can look at and decide which is best.

So continuing along the lines that you have demonstrated, then this may be of some help to you.

Javascript

/*jslint maxerr: 50, indent: 4, browser: true */

var trim = (function () {
    "use strict";

    function escapeRegex(string) {
        return string.replace(/[\[\](){}?*+\^$\\.|\-]/g, "\\$&");
    }

    return function trim(str, characters, flags) {
        flags = flags || "g";
        if (typeof str !== "string" || typeof characters !== "string" || typeof flags !== "string") {
            throw new TypeError("argument must be string");
        }

        if (!/^[gi]*$/.test(flags)) {
            throw new TypeError("Invalid flags supplied '" + flags.match(new RegExp("[^gi]*")) + "'");
        }

        characters = escapeRegex(characters);

        return str.replace(new RegExp("^[" + characters + "]+|[" + characters + "]+$", flags), '');
    };
}());

/*jslint devel: true */

console.log(trim(" - hello** +", "+*- "));

On jsfiddle

Of course, you can customise which characters you wish to escape, or flags to test for, as per your own needs. Regular Expressions


Safe navigating function for nested object properties I've made a function to safely traverse deep objects.

var foo = {
    bar: {
        baz: 1
    }
};

function getDeepProp(obj, properties) {

    if (typeof properties === 'string')
        properties = properties.split('.');

    if (typeof obj === 'undefined')
        return;

    if (!properties.length)
        return obj;

    return getDeepProp(obj[properties[0]], properties.slice(1));
}

console.log(getDeepProp(foo, 'bar.baz')); // 1
console.log(getDeepProp(foo, 'baz.baz')); // undefined

// The function was created to avoid exceptions like:
console.log(foo.baz.baz); // Uncaught TypeError: Cannot read property 'baz' of undefined

Please suggest possible improvements.

http://jsfiddle.net/rtkczasa/

It is a good exercise on recursion, but I'd suggest using built-in functions. Think that what you want to abstract is this syntax:

obj[a][b][c]

But doing some checking in between:

obj[a] && obj[a][b] && obj[a][b][c]

It seems to be a good job for a reducer, where obj is the accumulator, that gets passed along, and the properties are given as an array that you can reduce over, so p below will be each of the properties above:

function getDeepProp(obj, props) {
  return props.split('.').reduce(function(acc, p) {
    // if the accumulator is something
    // then lookup the next nested property
    // otherwise return undefined
    if (acc == null) return
    return acc[p]
  },obj) // the initial accumulator is the object
}

For example:

var obj = {
  a: {
    b: {
      c: 1000
    }
  }
}

console.log(getDeepProp(obj, 'a.b.c')) //=> 1000
console.log(getDeepProp(obj, 'a.b.foo')) //=> undefined

Note that acc can be an object, or an array, since arrays are objects, and the bracket syntax is the same:

console.log(getDeepProp([[[2,[3]]]], '0.0.1.0')) //=> 3

Also, I would worry about the function doing too much. If properties have not been passed, then the function shouldn't be used in the first place. That's why I'd suggest, among other reasons, to take the object (the receiver of the operation), always last:

function getDeepProp(props, obj)


Reverse-engineering with Filepicker API I have this script to pull data out of the Filepicker API internal. It's mostly reverse-engineering and the code seems to be ugly to me.  How can this be improved?

yesterday = (Time.now - 1.day).to_i
start_date = "08/08/2013".to_time.to_i
url = URI.parse("https://developers.inkfilepicker.com/login/")
http = Net::HTTP.new(url.host, url.port)
http.use_ssl = (url.scheme == "https")
request = Net::HTTP::Post.new(url.request_uri)
request.set_form_data({"email" => "my@email.com", "password" => "123456"})
response = http.request(request)
cookie = response.response['set-cookie'].split('; ')[0]
uri = URI("https://developers.inkfilepicker.com/apps/XXX/stats/file?startdate=#{start_date}&enddate=#{yesterday}")
req = Net::HTTP::Get.new(uri.request_uri)
req['Cookie'] = cookie
res = Net::HTTP.start(uri.hostname, uri.port, :use_ssl => uri.scheme == 'https') {|http|
  http.request(req)
}
stats = JSON.parse res.body
stats = stats['data']['links.created']

In case of some of you are against reverse engineering, I asked for a 'real' API to pull this data and they suggested me to do this until they provide an API.

Looks pretty good. Some notes:

(Time.now - 1.day) -> 1.day.ago
"08/08/2013".to_time: I'd be in doubt "is that day/month or month/day?". An alternative to avoid problems with the parsing format: Time.new(2013, 8, 8)
Net::HTTP: IMO this module is a pain to use for common tasks. I prefer something higher-level like the rest-client gem. It does a lot of boring things for you.
response.response['set-cookie'].split('; ')[0]. I wouldn't rely on this space after the semi-colon. Safer: response.response['set-cookie'].split(';')[0].strip
Use do/end for multiline blocks.
JSON.parse res.body Be consistent, use parentheses like you did in the rest of the code.
stats = stats['data']['links.created']: don't reuse variables: links_created = stats['data']['links.created']
request and later req, response and later res: Use more declarative names.


Implementation of warning storage I implemented type (UserRuleManager) that will apply list of rules to entity User. I need your code review and your advices.

Here is usage of that type

private void ProcessUser(User user)
{
    if (!_userRuleManager.ApplyGeneralRules(user))
    {
        SetWarning();
    } else if (!_userRuleManager.ApplySpecificRules(user)) {
        SetWarning();
    } else {
        CreateUser(user);
    }
}
private void SetWarning()
{
    foreach (var warningMessage in _userRuleManager.GetWarnings())
    {
        Warnings.Add(warningMessage);
    }
}

I have 2 types of rule: general and specific.  First, I'm trying to apply general rules (ApplyGeneralRules) and if it is false then get list of warnings. The same for specific rules (ApplySpecificRules).

In implementation of UserRuleManager I have:

private list of warnings:

   private readonly List<string> _warnings;

private property

    private bool IsVerified
    {
        get { return _warnings.Count == 0; }
    }

method to return list with warnings

public ICollection<string> GetWarnings()
{
    return _warnings;
}

method for general rules

public bool ApplyGeneralRules(User user)
{
    _warnings.Clear();
    ICollection<Rule> rules = _userRepository.GetActiveGeneralRules(user.CustomerId);

    foreach (Rule rule in rules.OrderBy(x => x.Order))
    {
        if (RuleChecker.IsVerified(rule, user))
        {
            _warnings.Add(string.Format("General rule : {0}", rule.TypeId));
        }
    }

    return IsVerified;
}

and method for specific rules

public bool ApplySpecificRules(User user)
{
    _warnings.Clear();
    var questions = GetQuestions(user);

    if (!questions.Any())
    {
        _warnings.Add(string.Format("No questions with code {0}", user.QuestionCodeId));
        return false;
    }
    foreach (var question in questions.OrderBy(q => q.Order))
    {
        var rules = _userRepository.GetSpecificRules(user.CustomerId, question.Id);
        foreach (var rule in rules)
        {
            if (!RuleChecker.IsVerified(rule, user))
            {
                _warnings.Add(string.Format("Specific rule: {0} -> {1} -> {2}", question.TypeId, question.CodeId, rule.TypeId));
            }
        }
    }

    return IsVerified;
}

Could you please give me advice how can I improve or maybe implement more proper relation between this entities. What the pattern can I use here?
 Thank you in advance and let me know if I should give more information

UPDATE

public static bool IsVerified(Rule rule, User user) {

RuleParameterValue daysParameter = rule.RuleParameterValues.SingleOrDefault(v => v.RuleParameterTypeId == Rule.Parameter.Days);

if (daysParameter == null) {
    return false;
}

int days = daysParameter.IntValue;
DateTime? lastLoginDateUtc = _userRepository.GetLastLoginDate(user.Id);
if (lastLoginDateUtc == null || lastLoginDateUtc.Value.AddDays(days) <= DateTime.UtcNow) {
    return true;
} else {
    return false;
}
}

The only criticism with the code is that your business rules are implemented in the third-party class UserRuleManager. This is OK if you are developing a a "rule system", but otherwise you should encapsulate the rules within the user class if they are about a user; the user factory if they are about how to create users; and the user repository (or data access class) if they are about how to persist and retrieve user objects.

Also, be wary if your class ends with "Manager"; it signals that the class will be responsible of too many things -- a God class.


Random Password Generator in C# Security is a huge complicated subject. What I'm looking for here is three things:

The password generating algorithm isn't reversible, meaning that when someone looks at the source code of this application, that won't help them to break passwords made by it other than the fact that they can see the possibilities as strings there.
Could I somehow remove those possibilities as strings to make it more secure?
Am I overlooking any potential memory issues e.g. should I be zeroing/wiping memory in areas where the password is stored after?

enum Password_Options
{
    ALPHANUM,
    ALL
}
private string CreatePassword(int length, Password_Options options)
{
    const string valid = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ1234567890";
    const string valid_all = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ1234567890!@#$%^&*()_-=+{}:;\\<>?|,./`~[]'";
    if (options == Password_Options.ALPHANUM){
        StringBuilder res = new StringBuilder();
        byte[] random = new byte[1];
        RNGCryptoServiceProvider rProvider = new RNGCryptoServiceProvider();
        while (0 < length--){
            rProvider.GetBytes(random);
            res.Append(valid[random[0] % (valid.Length - 1)]);
        }
        return res.ToString();
    }
    else{
        StringBuilder res = new StringBuilder();
        byte[] random = new byte[1];
        RNGCryptoServiceProvider rProvider = new RNGCryptoServiceProvider();
        while (0 < length--)
        {
            rProvider.GetBytes(random);
            res.Append(valid_all[random[0] % (valid_all.Length - 1)]);
        }
        return res.ToString();
    }
}

private void button1_Click(object sender, EventArgs e)
{
    Password_Options po;
    if (radioButton1.Checked){
        po = Password_Options.ALL;
    }
    else{
        po = Password_Options.ALPHANUM;
    }
    textBox1.Text = CreatePassword((int)numericUpDown1.Value,po);
}

One way to avoid using constant strings is to use the char.IsLetterOrDigit method, just restrict the values mod 92 and add to 33 to get all printable characters to 125:

enum Password_Options
{
    ALPHANUM,
    ALL
}
RNGCryptoServiceProvider rProvider = new RNGCryptoServiceProvider();
private string CreatePassword(int length, Password_Options options)
{
    StringBuilder res = new StringBuilder();
    byte[] random = new byte[1];
    using (rProvider)
    {
        while (0 < length--)
        {
            char rndChar = '\0';
            do
            {
                rProvider.GetBytes(random);
                rndChar = (char)((random[0] % 92) + 33);
            } while (options == Password_Options.ALPHANUM && !char.IsLetterOrDigit(rndChar));
            res.Append(rndChar);
        }
    }
    return res.ToString();
}

According to MSDN you should always dispose of the rng provider after using it.  One way is with a using block.


Speeding up and fixing phone numbers from CSVs with Regex I've hodgepodged together an attempt to extract all phone numbers from all CSVs in a directory, regardless of where they are and what format they're in.  I want all phone numbers to be printed to a compilation CSV with no repeat elements in the format 8885551234 with NO leading ones.  E.g., 1 (510) 533-2134 would become 5105332134, as would 510-5332134 (and the latter would count as a dupe and fail to be written).

My challenges are that the numbers are in different formats, in different columns (and sometimes in several columns), and that the files in the directory are, in total, many gigabytes in size.

What I came up with, first, fails to weed out numbers that start with 0 or 1 (though it does seem to "skip" the 1 in 11-digit numbers); second, I need checked by a regexpert to see if it is even doing what I want (identifying and printing phone numbers); and third, is far too slow:

import csv
import re
import glob
import string

with open('phonelist.csv', 'wb') as out:
    seen = set()
    output = []
    out_writer = csv.writer(out)
    csv_files = glob.glob('*.csv')
    #csv_files2 = glob.glob('*.csv')
    for filename in csv_files:
        with open(filename, 'rbU') as ifile:
            read = csv.reader(ifile)
            for row in read:
                for column in row:
                    s1 = column.strip()
                    result = re.match(
                        r'.*(\+?[2-9]?[0-9]?[0-9]?-?\(?[0-9][0-9][0-9]\)? ?[0-9][0-9][0-9]-?[0-9][0-9][0-9][0-9]).*', s1) or re.match(
                        r'.*(\+?[2-9]?[0-9]?[0-9]?-?\(?[0-9][0-9][0-9]\)?-?[0-9][0-9][0-9]-?[0-9][0-9][0-9][0-9]).*', s1)
                    if result:
                        tempStr = result.group(1)
                        for ch in ['(', ')', '-', ' ']:
                            tempStr = tempStr.replace(ch, '')
                        if tempStr not in seen:
                            seen.add(tempStr)
                            output.append(tempStr)
    for val in output:
        out_writer.writerow([val])

Use a compiled regular expression.

Since you are looking for the pattern anywhere within a string, use re.search() instead of re.match().  Actually, the behaviour is not exactly the same.  .search() will return the leftmost match, whereas .match() using your original pattern will return the rightmost match, since the initial .* is greedy.  The backtracking that is necessary to accomplish a rightmost match would be a source of inefficiency.

By using .search(), which performs a search not anchored to the start of the subject string, you can also eliminate the capturing parentheses and use .group(0) instead.

You can also shorten the regex by using \d (match any digit) and {n} (match the previous element n times).  Since your two regular expressions differ by only one character, combine them, using a [ -] character class to cover both cases.

In the canonicalization step, you want to eliminate all non-digits, so just do that instead.

regex = re.compile(r'\+?[2-9]?\d?\d?-?\(?\d{3}\)?[ -]?\d{3}-?\d{4})')
…

match = regex.search(s1)
if match:
    canonical_phone = re.sub(r'\D', '', match.group(0))
    if canonical_phone not in seen:
        seen.add(canonical_phone)
        output.append(canonical_phone)


Scraping all closed captions (subtitles) of a YouTube's creator's video library A script that scrapes all YouTube's videos closed captions(subtitles) of a single creator's video library. It scrapes either auto-generated or manually written subtitles. The script accepts a link to the creators video library, such as, "https://www.youtube.com/user/creator/videos" and outputs subtitles of each video to it own respective '.txt' file. I use Selenium to trigger the events and BeautifulSoup to scrape the content.

import time
from bs4 import BeautifulSoup
import urllib.request
from selenium import webdriver
from selenium.common.exceptions import TimeoutException
from selenium.webdriver.support.ui import WebDriverWait
from selenium.webdriver.support import expected_conditions as EC
from selenium.webdriver.common.by import By
import re
import sys

# Clicks on "Load More" button to display all users videos.
def display_all_videos(driver):
    while(True):
        try:
            element = WebDriverWait(driver, 10).until(EC.element_to_be_clickable((By.CLASS_NAME, "yt-uix-load-more")))
            element.click()
        except:
            break

# Creates a list of tuples (video_title, video_link) of all
# videos displayed on page
def video_list(driver):
    videos = driver.find_elements_by_class_name("yt-uix-tile-link")
    video_links = []
    for vid in videos:
        video_links.append((vid.text,vid.get_attribute("href")))
    return video_links

# Clicks on CC(Closed Caption) button in YouTube video
def enable_subtitles(driver):
    elem = WebDriverWait(driver, 10).until(EC.element_to_be_clickable((By.CLASS_NAME, "ytp-subtitles-button")))
    elem.click()

def subtitles_link(driver):
    time.sleep(1)
    timings = driver.execute_script("return window.performance.getEntries();")

    # Find string in timings that contains the substring 'srv3'
    # which is the subtitles link.
    link = ""
    for t in timings:
         for v in t.values():
             if "srv3" in str(v):
                 link = v
    return link

def create_file(title, link,subtitles):
    # remove illegal chars for file name
    title = "".join([c for c in filename if c.isalpha() or c.isdigit() or c==' ']).rstrip()

    try:
        file = open(title + '.txt', 'w')
        file.write('LINK: ' + link + '\n')
        file.write(subtitles)
        file.close()
    except:
        print("Can't create file for: " + title + " : " + link)

def scrape_subtitles(subtitle_link):
    r = urllib.request.urlopen(subtitle_link).read()
    soup = BeautifulSoup(r)

    # Remove tags (<*>), \n, and unecessary whitespace
    s = re.sub(r'<.+?>', '', soup.prettify())
    s = re.sub(r'\n', '', s)
    s = re.sub( '\s+', ' ', s ).strip()
    return s

def main(argv):
    driver = webdriver.Chrome('C:\Program Files\ChromeDriver\chromedriver.exe')

    # Visit page and load all videos to create a list of
    # tuples(video_name,video_link) of the videos
    driver.get(argv[1])
    display_all_videos(driver)
    videos = video_list(driver)

    # Visit video's page and enable 'CC' to scrape the subtitles and
    # save subtitles to '.txt' file.
    for v in videos:
        driver.get(v[1])
        try:
            enable_subtitles(driver)
            link = subtitles_link(driver)
            subtitles = scrape_subtitles(link)
        except:
            subtitles = "No Closed Caption"
        create_file(v[0],v[1],subtitles)

if __name__ == "__main__":
    main(sys.argv)

Readable?
Follows Python's best practices?

Bug

This was probably a posting or a copy/paste bug - in create_file() you meant to name the first parameter to be filename instead of title.

Code Style and PEP8 notes

use more descriptive variable names - r, s, c, v are not good variable names
remove unused TimeoutException import, organize imports in groups and alphabetically (PEP8 reference)
move the comments before the functions into proper docstrings
avoid catching broad exceptions via bare except
follow PEP8 whitespace and newline usage guidelines - specifically, whitespaces around the operators, two newlines between the function definitions

Third-party Library Usage notes

BeautifulSoup constructor accepts a file-like object as well - you don't have to explicitly call .read() method
it is a good idea to specify BeautifulSoup's parser explicitly:

soup = BeautifulSoup(r, "html.parser")  # or "html5lib", or "lxml"

Otherwise, BeautifulSoup would pick a parser automatically depending on what is available. This would mean that on your machine it may pick, say, lxml - on someone else's - html5lib, on other's html.parser. And, there might be different results since different parser behave differently.
a bigger problem is that you are not actually using BeautifulSoup capabilities at all - you are post-processing the "soup" with regular expressions. It looks like you can achieve the same by calling the .get_text() method on the "soup" object
for better reusability I would probably assume chromedriver is in PATH instead of hardcoding the path to it explicitly
you can reuse WebDriverWait instance instead of re-instantiating it multiple times
you never close the driver instance
time.sleep() is generally unreliable when used for waiting for something with selenium - recheck if you actually need it

Code Organization notes

You are passing driver to multiple methods - it may be a good idea to have a class with a driver and wait attributes for better reusability.

Some Syntax Sugar

you can construct a list of videos with a list comprehension:

[(video.text, video.get_attribute("href"))
 for video in driver.find_elements_by_class_name("yt-uix-tile-link")]

you can unpack the list of videos into filename and a link:

for filename, link in videos:

Modified Code:

import sys
import time
import urllib.request

from bs4 import BeautifulSoup
from selenium import webdriver
from selenium.common.exceptions import TimeoutException
from selenium.webdriver.common.by import By
from selenium.webdriver.support import expected_conditions as EC
from selenium.webdriver.support.ui import WebDriverWait

class YoutubeSubtitlesScraper:
    def __enter__(self):
        self.driver = webdriver.Chrome()

        self.wait = WebDriverWait(self.driver, 10)

        self.driver.get(start_url)
        self.display_all_videos()

    def __exit__(self, exc_type, exc_val, exc_tb):
        self.driver.close()

    def display_all_videos(self):
        """Clicks on "Load More" button to display all users videos."""
        while True:
            try:
                element = self.wait.until(EC.element_to_be_clickable((By.CLASS_NAME, "yt-uix-load-more")))
                element.click()
            except TimeoutException:
                break

    def subtitles(self):
        """Visits video's page, enables 'CC' to scrape the subtitles and generates filename, link and the subtitles content."""
        videos = [(video.text, video.get_attribute("href"))
                  for video in self.driver.find_elements_by_class_name("yt-uix-tile-link")]

        for filename, link in videos:
            self.driver.get(link)
            self.enable_subtitles()

            link = self.get_subtitles_link()
            yield filename, link, self.scrape_subtitles(link) if link else "No Closed Caption"

    def enable_subtitles(self):
        """Clicks on CC(Closed Caption) button in YouTube video."""
        show_subtitles_button = self.wait.until(EC.element_to_be_clickable((By.CLASS_NAME, "ytp-subtitles-button")))
        show_subtitles_button.click()

    def get_subtitles_link(self):
        """Finds string in performance timings that contains the substring 'srv3' which is the subtitles link."""
        time.sleep(1)
        timings = self.driver.execute_script("return window.performance.getEntries();")

        for timing in timings:
            for value in timing.values():
                if "srv3" in str(value):
                    return value
        return ""

    def scrape_subtitles(self, subtitle_link):
        """HTML parses subtitles."""
        response = urllib.request.urlopen(subtitle_link)
        soup = BeautifulSoup(response, "lxml")
        return soup.get_text(strip=True)

def create_file(filename, link, subtitles):
    """Creates file for the subtitle."""
    title = "".join([c for c in filename if c.isalpha() or c.isdigit() or c == ' ']).rstrip()

    with open(title + '.txt', 'w') as subtitles_file:
        subtitles_file.write('LINK: ' + link + '\n')
        subtitles_file.write(subtitles)

if __name__ == "__main__":
    start_url = sys.argv[1]

    with YoutubeSubtitlesScraper(start_url) as scraper:
        for filename, link, content in scraper.subtitles():
            try:
                create_file(filename, link, content)
            except:
                print("Can't create file for: " + filename + " : " + link)

(not tested)

Note how we made the class a context manager by defining the __enter__() and __exit__() magic methods.


Balanced delimiter method I was trying to solve the following Programming Praxis problem:

  Write a function that takes a string and determines if the delimiters
  in the string are balanced. The pairs of delimiters are (), [], {},
  and <>, and delimiters may be nested. In addition, determine that
  string delimiters ‘ and ” are properly matched; other delimiters lose
  their magical delimiter-ness property within quoted strings. Any
  delimiter is escaped if it follows a backslash.

Here's my attempt to solve the problem:

import java.util.*;
public class BalancedDelimiters{
    private Map<Character,Integer> delimitMap;
    private Map<Character,Character> charMap;
    public BalancedDelimiters(){
         delimitMap = new LinkedHashMap<Character,Integer>();
         charMap = new LinkedHashMap<Character,Character>();
         initDelimitDics(delimitMap);
         initCharMap(charMap);
    }

    private void initCharMap(Map<Character,Character> charMap){
        charMap.put('(',')');
        charMap.put(')','(');
        charMap.put('[',']');
        charMap.put(']','[');
        charMap.put('{','}');
        charMap.put('}','{');
        charMap.put('<','>');
        charMap.put('>','<');
    }

    private void initDelimitDics(Map<Character,Integer> delimitMap){
        delimitMap.put('(',0);
        delimitMap.put(')',0);
        delimitMap.put('[',0);
        delimitMap.put('{',0);
        delimitMap.put('<',0);
        delimitMap.put('>',0);
        delimitMap.put(']',0);
        delimitMap.put('}',0);
    }

    public boolean checkForBalance(String extractedString){
        int balancedIndicator = 0;
        char[] charArr = extractedString.toCharArray();
        for(char c:charArr){
            if(delimitMap.containsKey(c)){
                delimitMap.put(c,delimitMap.get(c)+1);
            }
        }
        for(Map.Entry<Character,Integer> entry:delimitMap.entrySet()){
            System.out.println("Entry Key:"+entry.getKey()+"Entry value:"+entry.getValue());
            System.out.println("Complementary delimiter:"+charMap.get(entry.getKey()));
            if(delimitMap.get(charMap.get(entry.getKey())) != entry.getValue())
                return false;
        }

        return true;
    }

}

Can somebody review my code and let me know how I would be able to improve this snippet?

In order to solve this problem you should either use a stack or recursion to keep track of the current nesting levels. The proposed solutions (including the linked one from assylias) will not do when you have multiple different delimiters and also string delimiters.

Here is a solution that seems to work based on recursion. It can probably be made a little shorter but i tried to keep it readable:

public class BalancedDelimiters {

    private static class NotBalancedException extends RuntimeException {
        private static final long serialVersionUID = 1L;
    }

    private static final Map<Character, Character> blockDelimiters = new HashMap<Character, Character>();
    static {
        blockDelimiters.put('(', ')');
        blockDelimiters.put('[', ']');
        blockDelimiters.put('{', '}');
        blockDelimiters.put('<', '>');
    }

    private static final Set<Character> stringDelimiters = new HashSet<Character>();
    static {
        stringDelimiters.add('\"');
        stringDelimiters.add('\'');
    }

    public boolean checkForBalance(String extractedString) {
        try {
            consume(extractedString, null);
            return true;
        } catch (NotBalancedException e) {
            return false;
        }
    }

    private String consume(String remainingString, Character currentDelimiter) {
        while (true) {
            int nextInterestingCharacter = getNextInterestingCharacter(remainingString);
            if (nextInterestingCharacter == -1) {
                if (currentDelimiter == null) {
                    return "";
                } else {
                    throw new NotBalancedException();
                }
            }

            char delimiter = remainingString.charAt(nextInterestingCharacter);
            remainingString = remainingString.substring(nextInterestingCharacter + 1);
            if (closes(delimiter, currentDelimiter)) {
                return remainingString;
            } else if (isBlockStartDelimiter(delimiter)) {
                remainingString = consume(remainingString, delimiter);
            } else if (isStringDelimiter(delimiter)) {
                remainingString = consumeToNextStringDelimiter(remainingString, delimiter);
            } else {
                throw new NotBalancedException(); // we found a delimiter that we could not use!
            }
        }
    }

    private int getNextInterestingCharacter(String remainingString) {
        for (int i = 0; i < remainingString.length(); i++) {
            if (remainingString.charAt(i) == '\\') {
                i++; // also skip next character
            } else if (isAnyKindOfDelimiter(remainingString.charAt(i))) {
                return i;
            }
        }
        return -1;
    }

    private String consumeToNextStringDelimiter(String remainingString, char delimiter) {
        for (int i = 0; i < remainingString.length(); i++) {
            if (remainingString.charAt(i) == '\\') {
                i++; // also skip next character
            } else if (remainingString.charAt(i) == delimiter) {
                return remainingString.substring(i + 1);
            }
        }
        throw new NotBalancedException();
    }

    private boolean closes(char delimiter, Character currentDelimiter) {
        if (currentDelimiter == null) {
            return false;
        }
        return blockDelimiters.get(currentDelimiter).equals(delimiter);
    }

    private boolean isAnyKindOfDelimiter(char c) {
        return isBlockDelimiter(c) || isStringDelimiter(c);
    }

    private boolean isBlockDelimiter(char c) {
        return isBlockStartDelimiter(c) || isBlockEndDelimiter(c);
    }

    private boolean isBlockStartDelimiter(char c) {
        return blockDelimiters.containsKey(c);
    }

    private boolean isBlockEndDelimiter(char c) {
        return blockDelimiters.containsValue(c);
    }

    private boolean isStringDelimiter(char c) {
        return stringDelimiters.contains(c);
    }
}

Note that I handle normal delimiters (i call them block delimiters) differently from string delimiters as they behave quite differently. I'm using an exception to control flow when the string is not balanced. Maybe some will find that a bit ugly but it saves a few lines of code (i think).


Fast integer handling Each data structure has its own time complexities.  The biggest one that jumps out at you is the hash table.  The average times for Insertion, Deletion and Searching are all O(1).  But it's really just constant time, since there can be multiple reps through each of these to find the right spot.  The real question is, can we do each of those operations in exactly one operation, if space wasn't an issue?

Recently I was working on the radix sort, and figuring out how to speed it up as much as possible.  The one thing I settled on was doing a 2-byte counting sort, thus you would only have to loop once through.  Then I got to thinking about the trie I used before, with integers where I traversed through by modding the number by 10 each time through.

This seemed to be faster than any other data structure that I've tried, but wanted to make it faster.  What I tried today was treating the trie as a guaranteed 2-level tree: each node with \$2^{16}\$ (65536) children.  That way I can reference each number like this: root->link[twoByte1]->link[twoByte2].  Where twoByte1 is the first 16 bits, and twoByte2 is the second 16-bit chunk.  Problem is, this is a lot of memory.  Instead, I decided to make an array of structures, with \$2^{16}\$ bool (uint8_t) values instead, and ran across the same issue.  To fix this, I made the following structure.

struct trie {
    unsigned int link[2048];
};

I decided to use 2048 unsigned integers (or signed, wasn't sure if there would be a difference) instead.  That way I can treat each bit as the true/false value during look up.  In order to reference each position, I would take the first 16 bytes, and reference the element he same way as before, but the 2nd I would divide by 32 to find out which element of the integer array to use, and mod the number again by 32 to find out which bit to look at.  The following is the code I used to reference it:

uint8_t getData (int data, uint16_t& dir, uint16_t& pos, uint8_t& x) {
    dir = (data >> 0)  & 0xffff;
    pos = (data >> 16) & 0xffff;
    x   =  pos / 32;
    return pos % 32;
}

The insert function was simply finding the spot and turning the bit on.  To do this, I would get the data from the function above, then OR the specific bit with 1.  Similarly with deletion, only I would invert the bit string, then AND it, to turn it off.  Then to search for that element, I would just return the specific bit.  Help with how to clear the bit was found in How do you set, clear, and toggle a single bit? on SO.

Here is the code for those three functions:

void insert(trie *root, int data) {
    uint16_t dir, pos;
    uint8_t x, y = getData(data, dir, pos, x);
    root[dir].link[x] |= 1 << y;
}
bool find(trie *root, int data) {
    uint16_t dir, pos;
    uint8_t x, y = getData(data, dir, pos, x);
    return root[dir].link[x] & (1 << y);
}
void remove(trie *root, int data) {
    uint16_t dir, pos;
    uint8_t x, y = getData(data, dir, pos, x);
    root[dir].link[x] &= ~(1 << y);
}

From here it was as simple as initializing the array of structs.  The great thing is that with the number of elements coming in, the data structure will never grow any farther out.  Also, in the case of threading, since I'm only turning the bits on during insertion, I don't think I would need to synchronize any of the threads.  Therefore it could be speed up with threading.  What are your thoughts on this? I made a test case on Ideone.  I ran a test on a Linux machine running on an i7, and was able to process 250 million integers every 2.5 seconds.

First there is no C++ here.
You are simply doing C (use class and member functions that way you will not be passing pointers around).

So what you have implemented is a chunked set.
Each chunk trie uses 2048 integers to hold existence information and somewhere outside the scope of code review you are creating arrays of theses to define your set.

Looking at the code:

uint16_t dir, pos;
uint8_t x, y = getData(data, dir, pos, x);

This is awful. The first think I am thinking is that x is undefined because you think that functions can return multiple values. BUT I read on and find that x is passed in and is assigned inside the function. This is just horrible in terms of style and readability.

Rues of thumb:

Declare one variable per line (and make their names meaningful).
Don't mix input/output parameters and return values all in the same function.
When you use output parameters the result is usually a success value.

What is trie.

bool find(trie *root, int data)

It looks like you have to pass an array of them through for this to work. From inside this function I can not tell how legal it is to access multiple elements. I am assuming the memory management is done somewhere else but who know.

You are making the assumption that an unsigned int is 32 bits.

return pos % 32; // assigned to y

// All your code then calculates bit offsets like this
(1 << y)

The standard only requires 16. https://stackoverflow.com/a/271132/14065

Overall I think this is marginal for C code as you can not do much more.
But for C++ code this absolutely terrible. There is no encapsulation, there is no protection from using the code wrong. There is no initialization to make sure it is all set to zero.


Replacing letters with numbers with its position in alphabet If anything in the text isn't a letter, ignore it and don't return it.

  a being 1, b being 2, etc.

  As an example:

  alphabet_position("The sunset sets at twelve o' clock.") Should return
  "20 8 5 19 21 14 19 5 20 19 5 20 19 1 20 20 23 5 12 22 5 15 3 12 15 3
  11" as a string.

It is my naive solution and my code is below in python 2.7

def alphabet_position(text):
    dictt = {'a':'1','b':'2','c':'3','d':'4','e':'5','f':'6','g':'7','h':'8',
    'i':'9','j':'10','k':'11','l':'12','m':'13','n':'14','o':'15','p':'16','q':'17',
    'r':'18','s':'19','t':'20','u':'21','v':'22','w':'23','x':'24','y':'25','z':'26'
    }
    arr = []
    new_text = text.lower()
    for i in list(new_text):
        for k, j in dictt.iteritems():
            if k == i:
                arr.append(j)
    return ' '.join(arr)

First of all, you don't need to hardcode the letters and their positions in the alphabet - you can use the string.ascii_lowercase.

Also, you don't have to call list() on a new_text - you can just iterate over it character by character.

Then, what if we would construct a mapping between letters and letter indexes in the alphabet (with the help of enumerate()). Then, use a list comprehension to create an array of numbers which we then join to produce a result:

from string import ascii_lowercase

LETTERS = {letter: str(index) for index, letter in enumerate(ascii_lowercase, start=1)}

def alphabet_position(text):
    text = text.lower()

    numbers = [LETTERS[character] for character in text if character in LETTERS]

    return ' '.join(numbers)


Printing random lines from a file in Python Is this the most efficient solution or can the performance be improved? Just opens a file and prints random words (lines with newlines stripped out).

import random

filename = "20k.txt"

def main(filename):
    words_file = open(filename, 'r')

    words = words_file.read().split("\n")
    temp_words = []
    for word in words:
        temp_words.append(word.strip("\r"))
    words = temp_words

    while True:
        length = int(raw_input("Words:"))
        for i in range(0,length):
            print(words[random.randrange(0,len(words)+1)]),
        print("\n")

if __name__ == "__main__":
    main(filename)

First, you should always close your files. Or, use the with keyword, which takes care of this for you.

Then, the 'r' mode is already the default mode for opening a file. Also, consider storing your words in a set, instead of a list (gets rid of duplicate words, which might be wanted, or not if you want a higher probability for some words).

You should consider using random.choice to get a random element from an iterable (actually an iterable with len defined).

I would also separate the file reading into a separate method. and make it more explicit what the prompt actually means (the number of words to generate).

I would use str.join and give it a generator expression rather than multiple calls to print.

import random

def get_words(file_name):
    with open(filename) as word_file:
        return list(set(word.strip() for word in word_file))

def random_words(n, words)
    return " ".join(random.choice(words) for _ in range(n))

def main(file_name):
    words = get_words(file_name)
    while True:
        n = int(raw_input("Number of words:"))
        print(random_words(n, words))

if __name__ == "__main__":
    file_name = "20k.txt"
    main(file_name)


Stable custom alphabetical order using List.Sort Background

  This is a problem from a programming contest.

  The Sharonians of planet Sharon, at the far end of our galaxy, have
  discovered various samples of English text from our electronic
  transmissions, but they did not find the order of our alphabet. Being
  a very organized and orderly species, they want to have a way of
  ordering words, even in the strange symbols of English. Hence they
  must determine their own order.

  For instance, if they agree on the alphabetical order:
  UVWXYZNOPQRSTHIJKLMABCDEFG

  Then the following words would be in sorted order based on the above alphabet order:

WHATEVER
ZONE
HOW
HOWEVER
HILL
ANY
ANTLER
COW

The input format is:

<N - number of words> <alphabet order>
<0>
..
<N>

What I Did

I intended to use List<T>.Sort and use a custom Comparator so I don't have to implement a sorting algorithm from scratch. However, the alphabet input and the order is case insensitive and since List<T>.Sort uses an unstable QuickSort, words like "go" and "Go" would be swapped.

My workaround is so dirty but it works by making a copy of the word list to serve as a look-up to preserve the order of equal words.

The Code As Is

using System;
using System.Collections.Generic;

class Program
{
    static void Main(string[] args)
    {
        List<string> words = new List<string>();
        List<string> wordsCopy = new List<string>();

        string firstLine = Console.ReadLine();

        //assume correct input
        int numOfWords = int.Parse(firstLine.Split(" ".ToCharArray())[0]);
        string alphabet = firstLine.Split(" ".ToCharArray())[1].ToUpper();

        for (int i = 1; i <= numOfWords; i++)
        {
            words.Add(Console.ReadLine());
            wordsCopy.Add(words[words.Count - 1]);//preserve index
        }

        Console.WriteLine();

        //Quicksort is unstable (meaning "equal" elements will still be swapped, e.g. "go" and "Go")
        words.Sort(delegate(String x, String y)
        {
            int max = (x.Length > y.Length) ? y.Length : x.Length;

            if (x.ToUpper().Equals(y.ToUpper()))
            {
                //dirty work-around to stablize the sort
                return wordsCopy.IndexOf(x) < wordsCopy.IndexOf(y) ? -1 : 1;
            }

            for (int i = 0; i < max; i++)
            {
                int indexX = alphabet.IndexOf(x[i].ToString(), StringComparison.InvariantCultureIgnoreCase);
                int indexY = alphabet.IndexOf(y[i].ToString(), StringComparison.InvariantCultureIgnoreCase);

                if (indexX > indexY)
                {
                    return 1;
                }
                else if (indexX < indexY)
                {
                    return -1;
                }
            }

            return 0;
        });

        Console.Write(String.Join("\n", words));
        Console.ReadLine();
    }
}

This code was good enough to win, but what could I have done better?

First of all, I would take advantage of the fact that the other commonly used implementation of sorting in .Net is stable: the OrderBy() extension method from LINQ. Though it requires IComparer interface for custom sorting, it won't accept the Comparer delegate you're using.

If you wanted to keep using List.Sort(), then I think your implementation is mostly okay, with the following caveats:

It has the potential to balloon the time complexity of the sort from \$\mathcal{O}(n \log n)\$ to \$\mathcal{O}(n^2 \log n)\$. This is becuse every call to your comparison delegate can take \$\mathcal{O}(n)\$ time, due to the calls to IndexOf().
It doesn't work correctly if some words in the list repeat themselves. For example “Good, good, Good” would be sorted as “Good, Good, good”.

Neither of these might matter to you, but I think it's good to be aware of them.

Your comparison doesn't work correctly for pairs where one word is the start of another (like “HOW” and “HOWEVER” from the sample list), it returns 0 for those, even though the words are not the same.

Some small specific comments about your code:

All your code is inside a single method. That's usually frowned upon, but I think it's probably okay when you have this little code. The same goes for separation of concerns.

int numOfWords = int.Parse(firstLine.Split(" ".ToCharArray())[0]);
string alphabet = firstLine.Split(" ".ToCharArray())[1].ToUpper();

I would avoid the repetition of firstLine.Split(" ".ToCharArray()) by extracting its result to a separate variable.

Also, you don't actually need that ToCharArray(), since Split() is a params method.

string[] firstLineParts = firstLine.Split(' ');

int numOfWords = int.Parse(firstLineParts[0]);
string alphabet = firstLineParts[1].ToUpper();

wordsCopy.Add(words[words.Count - 1]);

I think creating wordsCopy would be simpler if you used the constructor of List that can copy existing collection:

List<string> wordsCopy = new List<string>(words);

words.Sort(delegate(String x, String y)

You can use the lambda syntax here. That way, you don't have to (but can, if you want) specify the types of parameters:

words.Sort((x, y) =>

x.ToUpper().Equals(y.ToUpper())

ToUpper() is culture-specific, which can cause problems in some cultures (particularly Turkish).

But you probably don't need to care about that if you only ever expect to run this program on your computer.


Review Python script for getting the stats of codereview.SE into a file for analysis I wrote a script for getting the stats of codereview.SE from the front page into a file. Github link if someone prefers reading there. Here is  data_file.txt's current contents

23-08-2013
dd-mm-yyyy, questions, answers, %answered, users, visitors/day

22-08-2013,9079,15335,88,26119,7407,
23-08-2013,9094,15354,88,26167,7585,

The first sentence contains the last date at which the data is written into the file. It is a check to make sure that I don't write the data for the same date twice in the file. Also note that there is a newline at the end which isn't being shown above.

What I plan to do is to collect the data for many data and then use matplotlib to get graphs for finding out about the growth of this website. The part about plotting is currently under progress so I am not putting that here.

The python script that I am using is below. I would like a general review of this script and any suggestions about its design in particular.

#! python3
import urllib.request
import datetime

FILE_NAME = 'data_file.txt'
CURRENT_URL = 'http://codereview.stackexchange.com/'

def today_date():
    return datetime.date.today().strftime('%d-%m-%Y')

def already_written():
    with open(FILE_NAME, 'a+') as f:
        f.seek(0)
        first_line = f.readline()
        if today_date() == first_line[:-1]:
            return True
        return False

def parse(line):
    """This separates the stat-name and associated number"""
    temp = [0, '']
    braces = False
    for c in line:
        if c == '<':
            braces = True
        elif c == '>':
            braces = False
        elif braces is True or c in [' ', ',', '%']:
            continue
        elif c.isdigit():
            temp[0] *= 10
            temp[0] += int(c)
        else:
            temp[1] += c

    return temp

def write_stats():
    '''This writes the stats into the file'''
    with open(FILE_NAME, 'r') as f:
        data = f.readlines()

    with open(FILE_NAME, 'w') as f:
        f.write(today_date() + '\n')
        f.writelines(data[1:])

        url_handle = urllib.request.urlopen(CURRENT_URL)
        write_this = today_date() + ','
        for line in url_handle:
            temp_line = str(line)[2:-5]
            if 'stats-value' in temp_line and 'label' in temp_line:
                temp = parse(temp_line)
                write_this += str(temp[0]) + ','
        else:
            write_this += '\n'
            f.write(write_this)

def main():
    if not already_written():
        write_stats()

if __name__ == "__main__":
    main()

EDIT:

I think this line might need some explanation.

if 'stats-value' in temp_line and 'label' in temp_line:

I choose this for finding out the lines in the source code of the html of codereview.SE's front page. Only the 5 lines containing the stats have these 2 in it. So in front they tell me which line to parse.

Firstly, you don't need to store the last date at the beginning of the file since it's already at the end.

Secondly, you shouldn't compute today's date twice.

Thirdly, you should use Stack Exchange's API instead of trying to extract information from Code Review's home page. It has everything except visitors/day.

Fourthly, the date format %Y-%m-%d is better than %d-%m-%Y because it allows string comparison of dates.

So, here's what I came up with:

#!/usr/bin/env python3

import datetime
import gzip
import json
from urllib.request import Request, urlopen

FILE_NAME = 'data_file.txt'
API_URL = 'http://api.stackexchange.com/2.1/info?site=codereview'

def last(iterable, default):
    r = default
    for v in iterable:
        r = v
    return r

def main():
    today = datetime.date.today().strftime('%Y-%m-%d')
    with open(FILE_NAME, 'a+') as f:
        f.seek(0)
        last_date = last(f, '').split(',')[0]
        if last_date == today:
            return  # we already have the data for today
        elif last_date > today:
            raise Exception('last date is in the future')

        req = Request(API_URL, headers={'Accept-Encoding':'gzip'})
        r = gzip.decompress(urlopen(req).read()).decode('utf8')
        d = json.loads(r)['items'][0]
        print(today,
            d['total_questions'],
            d['total_answers'],
            d['total_unanswered'],
            d['total_users'],
            sep=',', file=f)

if __name__ == "__main__":
    main()

The gzip stuff is to work around a bug in Stack Exchange's API server: it always compresses the response even if you don't ask, which is a protocol violation. Using the requests library might make it easier.

The way I get the last line of the file isn't the most efficient but it should be good enough.


Gamblers ruin, parallel compute trials Context

The following script is a quick implementation of the gambler's ruin problem. For each given upper bound on the number of rounds in a game max_iter, simulate several games (trials) in parallel and summarize statistics.

Question

Is there a more efficient/neater way to parallelize? For example, the main method simulate_game only takes in one input parameter for convenience when calling Pool and it'd be nice to instead have e.g. simulate_game(max_iter, args).

import time
import numpy as np
from multiprocessing import Pool
from matplotlib import pyplot as plt
from random import random as unif_rand

def simulate_game(max_iter):
    """Gambler wins/loses with prob 0.5 and the game stops when either:
        i) the gambler runs out of money, or
        ii) house runs out of money, or
        iii) max_iter reached.

    Params:
        max_iter (int): upper bound on number of rounds in game.

    Returns:
        curr_iter (int): num iterations reached at end of game.

    Notes:
        "game" is synonymous with "trial".
    """
    # hardcoded vals to make parallelization easier...
    gambler_limit=100
    house_limit=200
    gambler_win_prob=0.5
    curr_iter = 0
    # simulate game
    while gambler_limit and house_limit and curr_iter < max_iter:
        curr_iter += 1
        u = unif_rand()
        payout = 1 if u > gambler_win_prob else -1
        gambler_limit += payout
        house_limit += -payout
    return curr_iter

class GamblerStats(object):
    """Compute and record stats."""

    def __init__(self, avg_duration_list=[], var_duration_list=[]):
        self.avg_duration_list = avg_duration_list
        self.var_duration_list = var_duration_list

    def compute_duration_stats(self, duration_list):
        self.avg_duration = np.mean(duration_list)
        self.var_duration = np.var(duration_list)
        return None

    def update_stats(self, duration_list):
        """Update stats based on new duration_list."""
        self.compute_duration_stats(duration_list)
        self.avg_duration_list.append(self.avg_duration)
        self.var_duration_list.append(self.var_duration)
        return None

if __name__ == "__main__":
    start_time = time.time()
    procs = 3  # num physical cores - 1
    max_range = 20
    scale_iter = 2e4
    max_iter_list = [n*scale_iter for n in range(1, max_range)]  # max iter per trial
    n_trials = 250  # num trials per experiment
    # Execute Simulated Experiments
    gambler_stats = GamblerStats()
    for max_iter in max_iter_list:
        # parallel compute trials
        jobs = [max_iter] * n_trials
        duration_list = Pool(procs).map(simulate_game, jobs)
        # Update Gambler Stats
        gambler_stats.update_stats(duration_list)
    # Summarize Time
    end_time = time.time()
    work_time = end_time - start_time
    print('Time taken for all simulations', work_time)
    # Example Plot
    plt.title('Avg Game Duration [iter]')
    plt.xlabel('max_iter x {:.0E}'.format(scale_iter))
    plt.ylabel('Avg Duration')
    plt.plot(gambler_stats.avg_duration_list, '.-')

Assigning number of processors

You currently set procs = 3, specifically for your 4-core machine. You can get the number of (logical) cores of your machine using the multiprocessing.cpu_count function:

from multiprocessing import Pool, cpu_count
procs = cpu_count() - 1

Create Pool just once

Rather than creating a new Pool every iteration of your for-loop, it is better to just create it once and keep using the same one. By using the with statement, it also makes sure to close it all neatly when you're done with it.

with Pool(procs) as p:
    for max_iter in max_iter_list:
        jobs = [max_iter] * n_trials
        duration_list = p.map(simulate_game, jobs)
        gambler_stats.update_stats(duration_list)

Hardcoded values in simulate_games function

First of all, you can define your default values in the function definition. This also makes the difference more clear between the hardcoded defaults and initializations like cur_iter = 0

def simulate_game(max_iter,
                  gambler_limit=100,
                  house_limit=200,
                  gambler_win_prob=0.5):

Create different fixed-value alternative using partial

To have a fixed different set of default values, you could use functools.partial:

from functools import partial
simulate_high_roller = partial(simulate_game, gambler_limit=1_000)
with Pool(procs) as p:
    duration_list = p.map(simulate_high_roller , jobs)

Mapping with multiple arguments: starmap

If instead you want to simulate using a whole range of parameter values, Pool.starmap is probably what you're looking for. Just like the 'regular' itertools.starmap, it allows you to pass in a list of tuples that it will unpack and pass to your function.

So if you have pre-defined lists of parameters you want to simulate, you can use zip to create the tuples.

# Example
max_iter_list = [10, 20, 50]
gambler_limits = [50, 100, 200]
house_limits = [150, 200, 250]
win_probabilities = [0.6, 0.5, 0.4]

with Pool(procs) as p:
    for arguments in zip(max_iter_list, gambler_limits, house_limits, win_probabilities):
        jobs = [arguments] * n_trials
        duration_list = p.starmap(simulate_game, jobs)

In case of a full grid-search of your parameters, this combines nicely with itertools.product, although that set of combinations will grow quite fast, so be deliberate about which ones you need to test.

>>> from itertools import product
>>> arguments = list(product(max_iter_list, gambler_limits,
...                          house_limits, win_probabilities))
>>> print(arguments)
[(10, 50, 150, 0.6),
 (10, 50, 150, 0.5),
 (10, 50, 150, 0.4),
 (10, 50, 200, 0.6),
 ...
 (50, 200, 200, 0.4),
 (50, 200, 250, 0.6),
 (50, 200, 250, 0.5),
 (50, 200, 250, 0.4)]
>>> len(arguments)
81


Update grid from source hierarchy In this question I answered with this Linq. While it does what I was looking for, I am not sure how easy the linq queries are for others to follow. So I am looking for feedback on formating, what comments would be helpful and other alternative approaches to moving through the children records.

    private void UpdateGridFromSourceHierarchy(int depth, IEnumerable<SourceFile> list)
    {

        //Get the initial set of sourcefiles
        var sourceFiles = list
            .SelectMany(current => current.getInvocations()
                                .Select(invocation => new {current = (SourceFile) null, invocation}))
            .ToList() //This ensures that getInvocations is only called once for each sourcefile
            .GroupBy(x => x.current, x => x.invocation);

        for (var currentDepth = 0; currentDepth <= depth; currentDepth++)
        {
            foreach (var currentGroup in sourceFiles)
            {
                int sourceFileCount = currentGroup.Count();
                int counter = 0;

                foreach (var invocation in currentGroup)
                {
                    /*
                     * Generalized grid code goes here
                     * In my code it was a call to:
                     * UpdateGridPosition(currentGroup,invocation,counter);
                     */
                    counter++;
                }
            }

            //Select the current sub source files
            sourceFiles = sourceFiles.SelectMany(current => current.Select(invocation => invocation))
                //Get all of the invocations paired with the new current level of source files
                .SelectMany(newCurrent => newCurrent.getInvocations()
                                     .Select(invocation => new { newCurrent, invocation }))
                //Group them so that we can loop through each set of invocations seperately
                .ToList().GroupBy(x => x.newCurrent, x => x.invocation);
        }
    }

Leaving aside the problem of deeply nested code for now, I think readability is greatly improved by the use of LINQ syntax:

var sourceFiles = from file in list
                  from invocation in file.getInvocations()
                  group invocation by (SourceFile)null into groupedByInvoker
                  select groupedByInvoker;

for (var currentDepth = 0; currentDepth < depth; currentDepth++)
{
    foreach (var invokerGroup in sourceFiles)
    {
        int sourceFileCount = currentGroup.Count();
        int counter = 0;

        foreach (var invocation in invokerGroup) {
            // Do stuff
            counter++;
        }
    }

    sourceFiles = from invokerGroup in sourceFiles
                  from file in invokerGroup
                  from invocation in file.getInvocations()
                  group invocation by file into groupedByInvoker
                  select groupedByInvoker;
}

For me, this makes the SelectMany a lot easier to follow. There are some other tweaks to the queries too:

You can just group by null directly, rather than projecting to an anonymous type and selecting that back out as the key.
You don't need to convert to a list if you follow it by a groupby. The groupby traverses and stores the result of the enumerable anyway. You can test this by removing the ToList and adding a break point/debug output inside getInvocations.
By forcing it to evaluate to to a list instead of allowing deferred execution you are actually calling getInvocation too many times. When currentDepth == depth - 1, you don't need to evaluate sourceFiles again and so you don't need to call getInvocations. With deferred execution, because the final sourceFiles is never read getInvocations is never called and everything is fine and dandy.


Checking if an integer is a palindrome using either a string or a dict I have solved the palindrome question on LeetCode today, but I wasn't happy with the original solution - I used strings, it worked slowly and used a lot of memory.
So I rewrote the answer using a dictionary approach instead knowing that looking up a value in a dictionary has lower time complexity than searching through a list, but the results were strange - my latest answer is much better in terms of memory use (better than 94.65% of other submissions), but it became significantly slower. As you can see in the image below, my last submission is much slower than the first, but memory use has improved a lot:

My original solution was this:
class Solution:
    def isPalindrome(self, x: int) -> bool:
        s = str(x)

        if len(s) == 1:
            palindrome = True

        i = int(len(s)/2)

        for p in range(i):
            if s[p] != s[-(p+1)]:
                return False
            else:
                palindrome = True

        return palindrome

And the updated version is:
class Solution:
    def isPalindrome(self, x: int) -> bool:
        s = {i:j for i,j in enumerate(str(x))}
        i = (max(s)+1)//2

        if max(s)+1 == 1:
            return True

        for p in range(i):
            if s[p] != s[max(s)-p]:
                return False
            else:
                palindrome = True

        return palindrome

Can anyone explain why the latest version is so much slower? I thought it should be faster...
What should I change to improve the speed?
EDIT:
New version based on @hjpotter comment. Now I only have 1 call to max() in the entire script and yet it's even worse - 168ms and 13.9 Mb. I don't understand what's happening here:
class Solution:
    def isPalindrome(self, x: int) -> bool:
        s = {i:j for i,j in enumerate(str(x))}
        m = max(s)
        i = (m+1)//2

        if m+1 == 1:
            return True

        for p in range(i):
            if s[p] != s[m-p]:
                return False
            else:
                palindrome = True

        return palindrome

Apart from performance, there are several other problems in your implementation which the other answers did not address.

This should not be a class. It should be a function, because it being a class offers no advantage whatsoever and it makes its usage more complex.
The variable palindrome is unnecessary and confuses the reader: this variable is always going to be True. So remove it.
Don’t cast the result of a division to int. Use integer division instead.
The if len(s) == 1 check is redundant: its logic is already subsumed in the subsequent code.
Use descriptive variable names: s for an arbitrary string is fine but i to denote the middle index of a sequence is cryptic.

An improved implementation (without changing the algorithm) would look as follows:
def is_palindrome(n: int) -> bool:
    s = str(n)
    mid = len(s) // 2
    for i in range(mid):
        if s[i] != s[-i - 1]:
             return False
    return True

Or, using a generator expression:
def is_palindrome(n: int) -> bool:
    s = str(n)
    mid = len(s) // 2
    return all(s[i] == s[-i - 1] for i in range(mid))

… and of course this whole loop could also be replaced with return s[:mid] == s[: -mid - 1 : -1] because Python gives us powerful subsetting.

I also want to address a misconception in your question:

looking up a value in a dictionary has lower time complexity than searching through a list,

That’s only true if the dictionary has the same size as the sequence (more or less …), and if looking up the key is constant. Neither is true in your case. But what’s more, you still need to construct the list/dictionary, and you completely omit the complexity of these operations.
In fact, the first thing your dictionary does is also construct the sequence — namely, by invoking str(x). It then goes on to traverse the whole list! Because that’s what i:j for i,j in enumerate(str(x)) does.
So before even starting your algorithm proper, it has already performed more work than your other implementation which, after all, only needs to traverse half the string. Even if you stopped at this point and returned, the second implementation would already be slower than the first, and you haven’t even finished constructing the dictionary yet (constructing the dictionary from key: value mappings also takes time).
As for the actual algorithm, you’re still traversing half the string, so once again you are doing the same work as your initial algorithm. This means you have the same asymptotic complexity. But even that part of the second algorithm is slower, because a dictionary is a much more complex data structure than a plain sequence, and each key access of the dictionary (s[p]) is substantially slower than the same access on a string or list would be.


Descriptive quantity (one, two, many) from number I just write a simple method which returns a string based on the number in accepts as a parameter. I used if/elif/esle here but I think for this simple scenario I wrote long code. I want to know is it possible to shorten this code in any way except using a lambda method. Also, I'm interested to know about the performance of if/elif/else. Is there any other solution which can make faster this method?

def oneTwoMany(n):
    if n == 1:
        return 'one'
    elif n == 2:
        return 'two'
    else:
        return 'many'

You can instead use a dictionary to define your n values:

nums = {1: 'one', 2: 'two'}

Then, when you want to use this, you can use .get(). .get() has a default argument which is returned when a dict doesn't have a key you've requested; here we use 'many' to be returned.

def oneTwoMany(n):
    nums = {1: 'one', 2: 'two'}
    return nums.get(n, 'many')

If you really want to be concise you can stop defining nums:

def oneTwoMany(n):
    return {1: 'one', 2: 'two'}.get(n, 'many')

As an aside to the downvoters of the question; it may be a simple question but I don't see how it breaks any rules.

EDIT: incorporating some other answers which (quite rightly) suggest catching certain invalid inputs.

def oneTwoMany(n):
    if type(n) is not int:
        raise TypeError('Invalid input, value must be integer')
    elif n < 1:
        raise ValueError('Invalid input, value is below 1')
    else:
        return {1: 'one', 2: 'two'}.get(n, 'many')


Stack data structure unit testing I have this contract:

public interface Stack<T> {
    void push(T object);
    T pop();
    int size();
}

I am curious what you think about my test of the size() and pop() in order to prevent bugs (underlying data structure in not being cleaned) in implementation like below:

Implementation:

public class StackImpl implements Stack<Object> {
    private int size;
    private Object data[];
    @Override
    public void push(Object object) {
        data[size] = object;
        size++;
    }
    @Override
    public Object pop() {
        --size;
        return data[size];
    }
    @Override
    public int size() {
        return size;
    }
}

Test:

public class StackTest {
    @Test
    public void pushTwoObjectsToEmtyStackCheckThatSizeIsTwo() {
        pushObjectsInOrder(OBJECT_A, OBJECT_B);
        assertEquals(2, stack.size());
    }
    @Test
    public void pushTwoObjectToEmptyStackAndPopTheSameObjectsInReversedOrder() {
        pushObjectsInOrder(OBJECT_A, OBJECT_B);
        assertEquals(OBJECT_B, stack.pop());
        assertEquals(OBJECT_A, stack.pop());
    }
    @Test(expected=IllegalArgumentException.class)
    public void stackDoesntAcceptNullAndThrowExcpetion() {
        stack.push(null);
    }
    @Before
    public void setUp() throws Exception {
        stack = new StackImpl();
    }
    private void pushObjectsInOrder(Object... objects) {
        for (Object object : objects) {
            stack.push(object);
        }
    }
    private static final Object OBJECT_A = new Object();
    private static final Object OBJECT_B = new Object();
    private Stack<Object> stack;
}

I'd like to not focus too much on the implementation itself, like whether it should be List or array etc. Assume that you defined interface and going to give to implement it to other guy. Since you defined the contract you are the one who writes a unit test.

Here are some tests I'd perform at the very least. There may be more you'd want:

Create an empty Stack. Test that its size is 0.
Push an element onto the stack. Test that its size is now 1.
Push another element onto the stack. Test that its size is now 2.
Pop an element from the stack. Test that it matches the 2nd pushed value. Check that the size of the stack is now 1.
Pop an element from the stack. Test that it matches the 1st pushed value. Check that the size of the stack is 0.
Attempt to pop an element from the stack. You should receive an ArrayIndexOutOfBounds exception.


Separating paging, sorting and filtering in my ASP.NET MVC application I have a service that had a method for paging, sorting and filtering of Vehicle Makes:

public class VehicleService : IVehicleService
{
    private readonly DbContext _context;

    public VehicleService(DbContext context)
    {
        _context = context;
    }

    public async Task<IPagedList<VehicleMake>> GetVehicleMakesWithPaginationAsync(string search, int? page, string sort)
    {
        var makes = _context.VehicleMakes.AsQueryable();

        switch (sort)
        {
            case "Name desc":
                makes = makes.OrderByDescending(x => x.Name);
                break;
            default:
                makes = makes.OrderBy(x => x.Name);
                break;
        }

        return await makes.Where(x => x.Name.StartsWith(search) || search == null).ToPagedListAsync(page ?? 1, 5);
    }
}

VehicleService interface:

public interface IVehicleService{
Task<X.PagedList.IPagedList<VehicleMake>> GetVehicleMakesWithPaginationAsync(string search, int? page, string sort);
}

After the review of my code, I was told that sorting, filtering and paging should be in separate classes that have interfaces. I implemented that in following way:

Sorting:

internal class Sorting : ISorting
{
    private readonly DbContext _context;

    public Sorting(DbContext context)
    {
        _context = context;
    }

    public IEnumerable<VehicleMake> SortMakes(string sort)
    {
        var makes = _context.VehicleMakes.AsQueryable();

        makes = sort == "Name desc" ? makes.OrderByDescending(x => x.Name) : makes.OrderBy(x => x.Name);
        return makes;
    }
}

Sorting interface:

interface ISorting
{
 IEnumerable<VehicleMake> SortMakes(string sort);
}

Paging:

class Paging : IPaging
{
    private readonly ISorting _sorting;

    public Paging(DbContext context)
    {
        _sorting = new Sorting(context);
    }

    public async Task<IPagedList<VehicleMake>> GetPagedListOfSortedMakes(string search, int? page, string sort)
    {
        var sortedMakes = _sorting.SortMakes(sort).AsQueryable();
        return await sortedMakes.Where(x => x.Name.StartsWith(search) || search == null).ToPagedListAsync(page ?? 1, 5);
    }
}

Paging interface:

interface IPaging
   {
    Task<IPagedList<VehicleMake>> GetPagedListOfSortedMakes(string search, int? page, string sort);
   }

And then in my service:

public class VehicleMakeService : IVehicleMakeService
{
    private readonly DbContext _context;
    private readonly IPaging _paging;

    public VehicleMakeService(DbContext context)
    {
        _context = context;
        _paging = new Paging(context);
    }

    public async Task<IPagedList<VehicleMake>> GetVehicleMakesWithPaginationAsync(string search, int? page, string sort)
    {
        return await _paging.GetPagedListOfSortedMakes(search, page, sort);
    }
}

This works well, but I'm not sure if I implemented this correctly. Is there a better (cleaner) way to do this?

For my money it would only be worthwhile having separate classes for Sorting/Filtering/Pagination if they were generic and able to work on multiple entity types. Even then they'd more likely be helper classes than a part of the top-level application interface.

If the current requirement of your application is purely to do what GetVehicleMakesWithPaginationAsync does then the proposed split into 3 classes doesn't strike me as justified. I'm more concerned about the nature/names of the parameters being passed.

Why not make "sort" a boolean flag called "sortDescending"? And I'd rename search to namePrefix or something that makes it clearer.

Lastly - is the AsQueryable() really needed?


How can I refactor the validation code to minimize it? I am using Django Rest Framework and below is the code of one of the API Endpoints. The code creates generates the PUBLIC and SECRET Values based on Stellar SDK.
def create(self, request):
        try:
            if 'app_id' not in request.data:
                response = {'status': 'FAIL', 'data': 'App ID field not found'}
                return Response(response, status=status.HTTP_400_BAD_REQUEST)

            if 'customer_id' not in request.data:
                response = {'status': 'FAIL', 'data': 'Customer ID field not found'}
                return Response(response, status=status.HTTP_400_BAD_REQUEST)

            customer_id = request.data['customer_id']
            app_id = request.data['app_id']
            app = App.objects.get(id=app_id)

            # Creating Stellar Account
            addr, secret = create_account()

            if addr is None and secret is None:
                response = {'status': 'FAIL', 'data': 'Could not generate customer Wallet'}
                return Response(response, status=status.HTTP_200_OK)

            # Make sure we do not add duplicate customers
            wallet_count = Wallet.objects.filter(customer_id=customer_id).count()
            if wallet_count > 0:
                response = {'status': 'FAIL', 'data': 'The customer ID already exist'}
                return Response(response, status=status.HTTP_200_OK)

            # The customer does not exist, hence create his wallet.
            wallet = Wallet(customer_id=customer_id, app=app, address=addr, secret=secret)
            wallet.save()

            if wallet.pk is None and wallet.pk > 0:
                response = {'status': 'OK', 'data': 'success'}
            else:
                response = {'status': 'FAIL', 'data': 'Messed up'}
        except App.DoesNotExist:
            response = {'status': 'FAIL', 'data': 'The App ID not found'}
            return Response(response, status=status.HTTP_404_NOT_FOUND)

        return Response({'status': 'OK', 'data': 'success'}, status=status.HTTP_200_OK)

As you can see there is repetition of codeblock like below:
response = {'status': 'FAIL', 'data': 'The customer ID already exist'}
                return Response(response, status=status.HTTP_200_OK)

It'd be nice if I can reduce all this code either by passing to a function or someway once and let it handle based on request dict.

Exception handling
You may want to consider reducing the try block to where the expected exception actually is raised.
try:
    app = App.objects.get(id=app_id)
except App.DoesNotExist:
    …

Possible bugs
This test does not consider the case, that only one of addr and secret is None. Is that permitted?
if addr is None and secret is None:
    …

These response dicts are never used:
if wallet.pk is None and wallet.pk > 0:
    response = {'status': 'OK', 'data': 'success'}
else:
    response = {'status': 'FAIL', 'data': 'Messed up'}

Deduplication
Regarding your concern of duplicated dicts: I see none.
Sure, they are similar, but there are no duplicates, since the error messages differ. Of course, you can outsource the generation to a function like
def make_response(data: str, success: bool) -> dict:
    """Returns a response dict."""
    return {'status': 'OK' if success else 'FAIL', 'data': data}

but it's up to you to decide if that's worth an extra function.


refactoring common behavior to js object I want to refactor this code into js object,
but not sure how. How should I do it?

I initialize an object for every item in the  DS.data_per_folder array.
how can I define it in a higher level? Where should I init it then?

   ..
    DS.data_per_folder[CurrentContextMgr.current.folder.attr('id')] = {
                allAttachments_vector: [],
                allAttachments_vector_counter : 0,
                attachment_position_dict : {},
                extraDetails_dict: {},
                getAttachmentsPerPage: function (pageNum) {
                    var result = this.allAttachments_vector.slice(global_vars.NUM_OF_ATTACH_PER_PAGE * (pageNum - 1), global_vars.NUM_OF_ATTACH_PER_PAGE * (pageNum));
                    return result.length == 0 ? null : result;
                },

                getAttachmentsMarkupPerPage: function (pageNum) {
                    var attachmentsMarkup = "";
                    var attachments = this.getAttachmentsPerPage(pageNum);
                    for (key in attachments) {
                        attachmentsMarkup += attachments[key].attachment_markup + " ";
                    }
                    return (attachmentsMarkup == "") ? "No attachments to display" : attachmentsMarkup;
                },

                pushAttachment: function (attachment, position) {

                    //not in dict yet
                    if (this.attachment_position_dict[attachment.attachmentId] == undefined)
                    {
                    if (position ==  null)
                    {
                    position = this.allAttachments_vector_counter;
                    }

                    this.allAttachments_vector[position] = attachment;

                this.allAttachments_vector_counter++;
                this.attachment_position_dict[attachment.attachmentId] = position;
                //this.extraDetails_dict[attachment.attachmentId] = {};
                }
            },       };
    }
..

I would recommend using a constructor function to create the objects:

function FolderData() {

    this.allAttachments_vector_counter = 0;
    this.allAttachments_vector = [];
    this.attachment_position_dict = {};
    this.extraDetails_dict = {};

}

FolderData.prototype = {

    constructor: FolderData,

    getAttachmentsPerPage: function (pageNum) {
        var result = this.allAttachments_vector.slice(
            global_vars.NUM_OF_ATTACH_PER_PAGE * (pageNum - 1),
            global_vars.NUM_OF_ATTACH_PER_PAGE * (pageNum)
        );
        return result.length == 0 ? null : result;
    },

    getAttachmentsMarkupPerPage: function (pageNum) {
        var attachmentsMarkup = "";
        var attachments = this.getAttachmentsPerPage(pageNum);
        for (key in attachments) {
            attachmentsMarkup += attachments[key].attachment_markup + " ";
        }
        return (attachmentsMarkup == "") ? "No attachments to display" : attachmentsMarkup;
    },

    pushAttachment: function (attachment, position) {
        //not in dict yet
        if (this.attachment_position_dict[attachment.attachmentId] == undefined) {
            if (position ==  null) {
              position = this.allAttachments_vector_counter;
            }
            this.allAttachments_vector[position] = attachment;
            this.allAttachments_vector_counter++;
            this.attachment_position_dict[attachment.attachmentId] = position;
            //this.extraDetails_dict[attachment.attachmentId] = {};
        }
    }

};

...

DS.data_per_folder[CurrentContextMgr.current.folder.attr('id')] = new FolderData();

The benefit of this, other than readability, is that you don't create a separate copy of every function property for each object. The functions are instead attached to the constructor's prototype, so there is only one instance of each function, and functions are shared by each constructed object instance.