Simple 5 operator calculator in C# The program takes 2 decimals as input and an operator (+, -, *, /, ^) so far. Any suggestions to make this code cleaner or shorter?
using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;

namespace Calculator
{
    class Program
    {
        static void Main(string[] args)
        {
            Console.Write("Enter a number: ");
            Decimal n1 = Convert.ToDecimal(Console.ReadLine());

            Console.Write("Enter an operator: ");
            string op = Console.ReadLine();

            Console.Write("Enter a number: ");
            Decimal n2 = Convert.ToDecimal(Console.ReadLine());

            if (op == "+")
            {
                Console.WriteLine(Sum(n1, n2));
                Console.ReadLine();
            }

            else if (op == "-")
            {
                Console.WriteLine(Dif(n1, n2));
                Console.ReadLine();
            }

            else if (op == "*")
            {
                Console.WriteLine(Prod(n1, n2));
                Console.ReadLine();
            }

            else if (op == "/")
            {
                Console.WriteLine(Div(n1, n2));
                Console.ReadLine();
            }

            else if (op == "^")
            {
                Console.WriteLine(Pow(n1, n2));
                Console.ReadLine();
            }

            else
            {
                Console.WriteLine("Invalid Operator! Only '+', '-', '*', '/', or '^' allowed.");
                Console.ReadLine();
            }
        }

        static Decimal Sum(Decimal n1, Decimal n2)
        {
            Decimal sum;
            sum = n1 + n2;
            return sum;
        }

        static Decimal Dif(Decimal n1, Decimal n2)
        {
            Decimal dif;
            dif = n1 - n2;
            return dif;
        }

        static Decimal Prod(Decimal n1, Decimal n2)
        {
            Decimal prod;
            prod = n1 * n2;
            return prod;
        }

        static Decimal Div(Decimal n1, Decimal n2)
        {
            Decimal div;
            div = n1 / n2;
            return div;
        }

        static Decimal Pow(Decimal n1, Decimal n2)
        {
            Decimal pow;
            pow = (decimal)Math.Pow((double)n1, (double)n2);
            return pow;
        }
    }
}

Remove the useless functions
There's already a sum function. It's called +
Instead of
Console.WriteLine(Sum(n1, n2));

and
static Decimal Sum(Decimal n1, Decimal n2)
{
    Decimal sum;
    sum = n1 + n2;
    return sum;
}

just write
Console.WriteLine(n1 + n2);

... and the exact same thing applies to all the other calculation functions you wrote.
The same thing in every branch of an if
No matter what op is, you do Console.ReadLine();. So delete it from each if/else if/else and just add it after the end of the if.


In Python, stylistically better way to remove a potentially nonexistent key from a dictionary? Say you have a dictionary some_dict and want to make sure there's no key some_key in it. What's the best way to do this stylistically speaking and why?

Option 1

some_dict.pop(some_key, None)

Option 2

try:
    del some_dict[some_key]
except KeyError:
    pass

Option 3

if some_key in some_dict:
    del some_dict[some_key]

Also feel free to suggest another option.

Option 1 looks perfect to me. It does the job in one line and is easy to read.


Compare a given version number in the form major.minor.build.patch and see if one is less than the other #include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <stdbool.h>

typedef int STATUS;
#define ERROR -1
#define OKAY 0

struct version
{
    unsigned char major;
    unsigned char minor;
    unsigned char build;
    unsigned char patch;
};
STATUS is_less_than(struct version * original, struct version *compared, bool *result)
{
    if(original == NULL || compared == NULL || result == NULL)
    {
        result = NULL;
        return ERROR;
    }
    *result = false;

    if(original->major < compared->major)
    {
        *result = true;
    }
    else if(original->major == compared->major) // else if the major >= major
        {

            if(original->minor < compared->minor)
            {
                *result = true;
            }
            else if(original->minor == compared->minor)
            {
                if(original->build < compared->build)
                {
                    *result = true;
                }
                else if(original->build == compared->build)
                {
                    if(original->patch < compared->patch)
                    {
                        *result = true;
                    }
                    else if(original->patch == compared->patch)
                    {
                        *result = false;
                    }
                }
            }
        }

    return OKAY;

}

Is there a cleaner way to do this?

Yes, there is a cleaner way:

if (a.major != b.major) {
    *result = a.major < b.major;
} else if (a.minor != b.minor) {
    *result = a.minor < b.minor;
} else if (a.patch != b.patch) {
    *result = a.patch < b.patch;
} else {
    *result = a.build < b.build;
}
return OKAY;

I reordered patch to come before build since that's how it is usually done. If your version scheme is different from this, good luck.

Instead of unsigned char I would choose uint32_t so that your code can handle versions like 1.0.20190415. If you need to handle version numbers with millisecond timestamps, each version component would need to be uint64_t.


CSV import-export in Python I have finished a really short and small import / export script using Python 2.7, and now I would like to structure it using classes and methods where possible.

Could anybody give me some advice on what would be the best approach for this?

I haven't used any OOP because I would also like somebody to give me some best practices on how this should be done/structured.

from similarweb import ContentClient
import csv

FILE_OUT = 'similarweb/new_domains.csv'
FILE_IN = 'similarweb/domains.csv'

content_client = ContentClient("some_key")
final_result = ""

with open(FILE_IN, 'r') as csv_read_file:
    reader = csv.reader(csv_read_file)
    for i, line in enumerate(reader):
        url = ', '.join(str(e) for e in line)
        final_result += url + " " + (content_client.category(url)['Category']) + "\n"

with open(FILE_OUT, 'w') as csv_write_file:
    csv_write_file.write(final_result)

Rather than going down the OOP route, I would just split this out into a few functions:

import csv

from similarweb import ContentClient

CLIENT_KEY = 'some_key'
FILE_IN = 'similarweb/domains.csv'
FILE_OUT = 'similarweb/new_domains.csv'

def extract_data(path, client):
    """Extract the data from the specified path using the client."""
    lines = []
    with open(path) as read_file:
        for line in csv.reader(read_file):
            url = ', '.join(line)  # see note below
            lines.append(' '.join(
                (url, client.category(url)['Category'])
            ))
    return '\n'.join(lines)

def save_data(path, data):
    """Save the data to the specified path."""
    with open(path, 'w') as csv_write_file:
        csv_write_file.write(data)

if __name__ == '__main__':
    save_data(FILE_OUT, extract_data(FILE_IN, ContentClient(CLIENT_KEY)))

Per the documentation:

  Each row read from the csv file is returned as a list of strings. No automatic data type conversion is performed.

so there is no need to explicitly convert to strings.


Content filtering of webpage This is code for filtering the data of webpage, for the web crawler I made for my project. I know python scripts can lag than other languages, but this takes a lot of time when processing even a single page.

I don't want to use any other external libraries for filtering content. Is there any way my current code can be improved to be cleaner and faster?

# -*- coding: utf-8 -*-
import urllib
url='http://designingadam2.wordpress.com'
def content(page,url):#FILTERS THE CONTENT OF THE REMAINING PORTION
    flg=0
    #REMOVES &nsbp LIKE CHARACTERS
    while page.find("&",flg)!=-1:
        page.replace('&nbsp;','')
        start=page.find("&",flg)
        end=page.find(";",start+1)
        if (end-start)<10:  #USED IF HERE TO CNFRM TAGS-->REMOVE IF NOT NEEDED
            pageO=page[:start]
            pageT=page[end+1:]
            page=pageO+pageT
            flg=start+1#TO CONTINUE FROM NEXT POS
        else:
            flg+=1
    flg=0

    #REMOVES CONTENT BETWEEN SCRIPT TAGS
    while page.find("<script",flg)!=-1:
            start=page.find("<script",flg)
            end=page.find("</script>",flg)
            end=end+9
            i,k=0,end-start
            page=list(page)
            while i<k:
                    page.pop(start)
                    i=i+1
            page=''.join(page)
            flg=start

    #REMOVES CONTENT BETWEEN STYLE TAGS
    flg=0
    while page.find("<style",flg)!=-1:
            start=page.find("<style",flg)
            end=page.find("</style>",flg)
            end=end+9
            i,k=0,end-start
            page=list(page)
            while i<k:
                    page.pop(start)
                    i=i+1
            page=''.join(page)
            flg=start

    #REMOVES THE TAGS
    s_list = list(page)
    i,j = 0,0
    while i < len(s_list):
            # find the <
            if s_list[i] == '<':
                while s_list[i] != '>':# and i!=(len(s_list)-1):
                    # remove everything between the < and the >
                    s_list.pop(i)

                # make sure we get rid of the > to
                s_list.pop(i)
            else:
                i=i+1
    #-------------------------------------------------------------------

    #REMOVES WHITESPACES
    s_list="".join(s_list)
    lst=s_list.split()
    #CONVERT TO LOWERCASE
    i=0
    while i<len(lst):
        lst[i]=lst[i].lower()
        i+=1

    #REMOVES DUPLICATES
    lst=list(set(lst))

    #REMOVE COMMON WORDS
    phrase=['to','a','an','the',\
            'for','from','that','their',\
            'i','my','your','you','mine',\
            'we','okay','yes','no','as',\
            'if','but','why','can','now',\
            'are','is','also',',','.',';',\
            ':','?','|','/','\n','\t']

    i=0
    while i<len(lst):
        if lst[i] in phrase:
            lst.pop(i)
        else:
            i+=1
    print lst
    print len(lst)

def pageContent(url):#EXTRACTS HTML CODE
    f = urllib.urlopen(url)
    page = f.read()
    f.close()
    #page=page.replace(u'\xa0', ' ').encode('utf-8','ignore')
    return page

page=pageContent(url)
content(page,url)

Please mind the comments. I left it so it could be of some help.

Per my comment, follow the style guide - for example, there should be whitespace around = when assigning, and after commas:

i, k = 0, end - start

content is not a good name for your function. You should be more descriptive of what it actually does (perhaps filter_content?) and add a docstring providing more information. Throughout your code there are temporary variables with cryptic names (s_list? lst?) that could be changed to make things much clearer - I was wondering why flg isn't Boolean, and it turns out that it isn't actually a flag.

Your approach to removing HTML tags (picking through the whole page character by character) is particularly prone to error; what if one of the attributes within a tag contains '>'? For a good standard library solution, see here.

The conversion to lowercase is, frankly, ludicrous:

i=0
while i<len(lst):
    lst[i]=lst[i].lower()
    i+=1

you had the whole string (called, confusingly, s_list) to hand just two lines beforehand, and

s_list = s_list.lower()

is so much simpler.

As you're making a set to remove duplication:

lst=list(set(lst))

why not keep the set, instead of converting back to list, and use it to do the filtering, too? For example, use set.difference_update:

>>> words = set('this is a sentence to filter'.split())
>>> words.difference_update(['a', 'to', 'this', 'is'])
>>> words
set(['sentence', 'filter'])

Your other function could be simplified significantly:

def page_content(url):
    with urllib.urlopen(url) as f:
        return f.read()


Count duplicates in a JavaScript array This is actually a hackerrank interview question I have already solved but I need guidance on how to optimize it for the

Time
Space

  Given an array of integers, your task is to count the number of duplicate array elements. Duplicate is defined as two or more identical elements. For example, in the array [1, 2, 2, 3, 3, 3], the two twos are one duplicate and so are the three threes.

  Function Description

  Complete the function countDuplicates in the editor below. The function must return an integer denoting the number of non-unique (duplicate) values in the numbers array.

  countDuplicates has the following parameter(s):

  numbers[numbers[0],...numbers[n-1]]:  an array of integers to process

  Constraints

  1 ≤ n ≤ 1000
  1 ≤ numbers[i] ≤ 1000, 0 ≤ i < n

  Sample Input: [1, 3, 1, 4, 5, 6, 3, 2]
  Sample Output: 2
  Explanation: The integers 1 and 3 both occur more than once, so we return 2 as our answer.

function countDuplicates(original) {

  let counts = {},
    duplicate = 0;
  original.forEach(function(x) {
    counts[x] = (counts[x] || 0) + 1;
  });

  for (var key in counts) {
    if (counts.hasOwnProperty(key)) {
      counts[key] > 1 ? duplicate++ : duplicate;
    }
  }

  return duplicate;

}

Reduce time
You can improve the function's time complexity at the expense of memory by tracking the duplicates in a separate Set. This will reduce the number of iterations by the number of unique items in the input array, and increase the memory need by the same number.
A further improvement can be gained if you delete duplicates from the unique set as you go. This will bring the memory use down.

function countDuplicates(original) {
  const uniqueItems = new Set();
  const duplicates = new Set();
  for (const value of original) {
    if (uniqueItems.has(value)) {
      duplicates.add(value);
      uniqueItems.delete(value);
    } else {
      uniqueItems.add(value);
    }
  }
  return duplicates.size;
}

/* Test code not related to solution */
function test(name, func, data, result) {
  const output =  `${name} [${data.join(", ")}] : `;
  console.log(output + (func(data) === result ? "passed" : "failed"));
}

test("1 duplicate ", countDuplicates, [9, 11, 12, 2, 7, 4, 2], 1);
test("1 duplicate ", countDuplicates, [6, 6, 6], 1);
test("2 duplicates ", countDuplicates, [0, 1, 4, 2, 7, 4, 2], 2);
test("3 duplicates ", countDuplicates, [0, 1, 4, 2, 7, 4, 2, 0], 3);
test("No duplicates ", countDuplicates, [0, 1, 4, 2, 7, 5, 8, 9], 0);

A look at memory.
Just out of interest I wanted to see the memory use. It turns out that the more unique items the lower the use (makes sense). The next snippet shows the memory use over 10000 calls to random arrays 1000 items long with a max range of item values to 10000.

function countDuplicatesMemory(original) {
  var maxSize = 0;
  const uniqueItems = new Set();
  const duplicates = new Set();
  for (const value of original) {
    if (uniqueItems.has(value)) {
      duplicates.add(value);
      maxSize = Math.max(maxSize, duplicates.size + uniqueItems.size);
      uniqueItems.delete(value);
    } else {
      uniqueItems.add(value);
      maxSize = Math.max(maxSize, duplicates.size + uniqueItems.size);
    }
  }
  return maxSize;
}

var memoryUse = 0;
var min = Infinity;
var max = 0;
const cycles = 10000;
const arraySize = 1000;
const maxRange = 10000;

for(let i = 0; i < cycles; i++){
  const arr = [];
  const range = Math.random() * maxRange | 0;
  for(let i = 0; i < arraySize; i++){
     arr.push(Math.random() * range | 0);
  }
  const mem = countDuplicatesMemory(arr);
  memoryUse += mem;
  min = Math.min(mem, min);
  max = Math.max(mem, max);
}
console.log("Mean memory use O(n * " + (memoryUse / cycles / arraySize).toFixed(3)+ ")");
console.log("Min memory use  O(n * " + (min / arraySize).toFixed(3)+ ")");
console.log("Max memory use  O(n * " + (max / arraySize).toFixed(3)+ ")");

The mean memory use is around O(n * 0.85)
These values are for a random set of values, there are cases where non random values may push the max memory just over O(n)


What standard should I use for the naming of my MVC controller services? I have the following:

   public class ContentsController : BaseController {
        private IContentService _content;
        private IPageService pageService;
        private IReferenceService _reference;
        private ISequenceService seqService;

        public ContentsController(
            IContentService contentService,
            IPageService pageService,
            IReferenceService referenceService,
            ISequenceService sequenceService) {
            this._content = contentService;
            this.pageService = pageService;
            this._reference = referenceService;
            this.seqService = sequenceService;
        }

Note that it's completely mixed up with different naming standards.

Is there anything recommended for the naming of services that are used by my controller?

I don't think it matters as long as you are consistent. I personally prefer:  _{servicename}Service format. ex. _referenceService

public class ContentsController : BaseController {
        private IContentService _contentService;
        private IPageService _pageService;
        private IReferenceService _referenceService;
        private ISequenceService _seqService;

        public ContentsController(
            IContentService contentService,
            IPageService pageService,
            IReferenceService referenceService,
            ISequenceService sequenceService) {
            this._contentService = contentService;
            this._pageService = pageService;
            this._referenceService = referenceService;
            this._seqService = sequenceService;
        }


C++ smart pointers and classes I've started learning C++ recently and want from what I've gathered smart pointers are the way to go when storing stuff on the free store. I want to make sure I've got the basic hang of C++(11) correctly. I've implemented a very basic vector class (a mathematical vector) and would greatly appreciate if anyone would point out if I'm doing something wrong or that could be done better.

Vector.h

#ifndef VECTOR
#define VECTOR

#include <memory>

class Vector {
private:
    int x;
    int y;
    int z;

public:
    Vector(int x, int y, int z);
    ~Vector();

    std::unique_ptr<Vector> operator + (Vector* vector);

    int GetX() { return x; }
    int GetY() { return y; }
    int GetZ() { return z; }

    void SetX(int x) { this->x = x; }
    void SetY(int y) { this->y = y; }
    void SetZ(int z) { this->z = z; }
};

#endif

Vector.cpp

#include <memory>
#include <iostream>
#include "Vector.h"

Vector::Vector(int x, int y, int z) : x(x), y(y), z(z) {
    std::cout << "Constructed a Vector instance with elements ( " << x << ", " << y << "," << z << " )." << std::endl;
};

Vector::~Vector() {
    std::cout << "Destructed a Vector instance with elements ( " << x << ", " << y << "," << z << " )." << std::endl;
}

std::unique_ptr<Vector> Vector::operator + (Vector* vector) {
    std::unique_ptr<Vector> result = std::make_unique<Vector>(0, 0, 0);

    result->SetX(this->x + vector->GetX());
    result->SetY(this->y + vector->GetY());
    result->SetZ(this->z + vector->GetZ());

    return result;
};

Usage

#include <iostream>
#include <memory>
#include "Vector.h"

int main() {
    std::unique_ptr<Vector> v1 = std::make_unique<Vector>(1, 2, 3);
    std::unique_ptr<Vector> v2 = std::make_unique<Vector>(3, 2, 1);
    std::unique_ptr<Vector> vr = *(v1.get()) + v2.get();
}

When using the overloaded operator (+) it looks a bit ugly but it's the way I found that worked when using unique_ptr<T>. One thing I'm doubting if I'm doing "correctly" is returning a unique_ptr<T>. Should I return a value instead and then create a unique_ptr<T> from that instead?

I dislike that your class mixes functionality (a mathematical vector) with memory management. A class should only do one of those. If I prefer shared_ptr I have to rewrite your whole Vector class.

There is actually a thing in C++ that is better than smart pointers, which is value semantics. People rarely screw up the lifetime management of an int, it just works naturally. When you do tricky lifetime management try to mimic the way int does it.

The operator + should be const (as well as GetX, GetY and GetZ). It should also work on Vectors and not only on pointers. The standard way to add Vectors is this:

Vector operator +(const Vector &lhs, const Vector &rhs){
    return Vector(lhs.GetX() + rhs.GetX(), lhs.GetY() + rhs.GetY(), lhs.GetZ() + rhs.GetZ());
}

Note that this is a free function, not a member function of Vector. Prefer free functions, because they work in more cases (like multiplying an int to a Vector) and are thus more consistent.

This allows you to do this:

Vector v1{ 1, 2, 3 };
Vector v2{ 6, 5, 4 };
auto v3 = v1 + v2;

Now, if you do want to support unique_ptr you can do so basically the same way:

std::unique_ptr<Vector> operator +(const std::unique_ptr<Vector> &lhs,
    const std::unique_ptr<Vector> &rhs){
    return std::make_unique<Vector>(*lhs + *rhs); //uses above operator
}

This allows you to do this:

auto v1 = std::make_unique<Vector>(1, 2, 3);
auto v2 = std::make_unique<Vector>(4, 5, 7);
auto v3 = v1 + v2;

All in all you seem to have a pretty good grip on operator overloading and smart pointers already.


A Simple Rock, Paper, Scissors program I wrote a rock, paper, scissors program. It works fine, but I'm curious if there are any constructive critiques, tips, tricks, comments, or advice anyone may have on how the code operates, performs, looks, etc.

# This code is a PvC Rock-Paper-Scissors Game!
# Import Random Library for Computer Choice
import random

# Greeting & Introduction
print ("Hello! Let's play Rock-Paper-Scissors!");

# Just to Make things Smoother.
options = {1:'rock', 2:'paper', 3:'scissors'};
player_result = ["tie!", "win!", "lose!"];

# Obtain Player & CPU Inputs
def collection(user_choice = 42):
    # user_choice Collection
    while (user_choice not in ['1','2','3']):
        # Prompts the User for a number & Stores as raw_input
        print ("Pick a Number: \n 1) \t Rock \n 2) \t Scissors \n 3) \t Paper \n");
        user_choice = raw_input();
        # Checks to see if the choice is acceptable
        if (user_choice in ['1','2','3']):
            break;
        # If not acceptable, restart the loop (ask again).
        else:
            print("\nPlease pick either 1, 2, or 3!\n");

    # Convert user_choice to Int
    user_choice = int(user_choice);

    # choose comp_choice
    comp_choice = random.randint(1,3);
    return (user_choice, comp_choice);

# Translate Results into English
def translate_choices(user,comp):
    print ("\nYou chose " + options[user] +"!");
    print ("Computer chose " + options[comp] +"!");

# Comparison Function returns Game Results
def compare(x,y):
    result = player_result[ (x-y)%3 ];
    return result;

(user_choice,comp_choice) = collection();
translate_choices(user_choice,comp_choice);
result = compare (user_choice, comp_choice);
print ("You " + result);

From the top:

Imports

Since you're only using one thing from random, you might as well only import that one thing:

from random import randint

As PEP8 recommends, whenever the time comes order your imports by standard imports, then third-party imports, then lastly local imports.

Use of Parentheses

In Python 2, your print statements do not require parentheses. If you're on Python 3, print function calls require them. Other areas (such as your while loop and return statement for collections) do not require parentheses around them.

As a later note: You can use commas within print statements to have spaces between things or you can concatenate two strings with the + operator, e.g.

>>> print "Hello", "World!"
Hello World!
>>> print "Hello" + "World!"
HelloWorld!

I wouldn't mix the two though (might make things look messy), and this is just a helpful note on the difference.

Unnecessary Comments

A lot of your code is self-documenting already and do not require comments (which is a good thing).

You should only comment areas that you believe might be hard to understand, e.g. calculation for the game result.

If you end up using functions, you might want to add some helpful docstrings to describe what's going on, or rename the function e.g.

def calculate_winner(x, y):
    """<Explain how calculate_winner works here>."""
    #...etc...

Code Organization

A lot of your code doesn't seem to need to be in functions, e.g. translate_choices and compare. You only ever use them once, and its all related to your game.

This is a bit of personal preference, but it might be nicer to move all your code into a function and avoid having global variables. This way, if others end up importing your code they won't encounter conflicts.

def play_rps():
    # ... rest of your code here ...

Also, it might be good to make sure your code only runs if it's called as main, e.g.

if __name__ == '__main__':
    play_rps()

and being called via python rps_name.py or however the file name hosting your code may be.

More info on __main__ here.

Handling User Choice

Instead of making a list every time, why not use the keys from the dictionary you already made?

while user_choice not in options:
    # do user_choice stuff here

Also, to make the logic a bit simpler in the while loop, you can remove the break statement by only checking for invalid inputs:

while user_choice not in options:
    #...print prompt for user choice here...
    user_choice = raw_input()

    if user_choice not in options:
        print '\nPlease pick either 1, 2, or 3!\n'

Lastly, you can also merge the later conversion of raw_input into an integer into your while loop:

try:
    user_choice = int(raw_input())
except ValueError:
    # input was not a valid integer
    user_choice = None

if user_choice not in options:
    print '\nPlease pick either 1, 2, or 3!\n'

Unnecessary Semicolons

Do you come from a C++/Java background? Reason I ask is because you seem to have a lot of semicolons in your code (which aren't required for python).

End Product

With the suggestions I listed above (and a few small things such as splitting up your option prompt), here's the code I came up with:

from random import randint

def play_rps():
    print "Hello! Let\'s play Rock-Paper-Scissors!\n"

    options = {1:"rock", 2:"paper", 3:"scissors"}
    player_result = ["tie", "win", "lose"]

    user_choice = None

    # get user choice
    while user_choice not in options:
        print "Game options:"
        print "-------------"
        print "1) Rock"
        print "2) Scissors"
        print "3) Paper"

        try:
            user_choice = int(raw_input("Your choice: "))
        except ValueError:
            # input was not a valid integer
            user_choice = None

        if user_choice not in options:
            print "\nPlease pick either 1, 2, or 3!\n"

    # calculate computer choice
    computer_choice = randint(1, 3)

    print "\nYou chose: " + options[user_choice] + "!"
    print 'Computer chose: ' + options[computer_choice] + "!"

    # calculate winner
    result = player_result[ (user_choice-computer_choice) % 3]
    print 'You ' + result + '!'

if __name__ == '__main__':
    run_game = play_rps()

NOTE: With the above format, it also makes creating a loop (if you choose to have a replay option) much easier.


Synchronized implementation of a bank account in Java I am trying to implement a bank account with Java in a thread safe way. My code looks like:

import java.math.BigDecimal;
import java.math.RoundingMode;
import java.util.concurrent.locks.ReadWriteLock;
import java.util.concurrent.locks.ReentrantReadWriteLock;

/**
 * Class to represent an account, it also provides with methods to add and withdraw amount from the account.
 *
 * @author Karan Khanna
 * @version 1.0
 * @since 3/17/2019
 */
public class Account {

    private ReadWriteLock accountLock;

    private BigDecimal balance;

    private String accountNumber;

    private String accountHolder;

    public Account(String accountNumber, String accountHolder) {
        this.balance = new BigDecimal(0);
        this.accountNumber = accountNumber;
        this.accountHolder = accountHolder;
        this.accountLock = new ReentrantReadWriteLock();
    }

    public double getBalance() {
        this.accountLock.readLock().lock();
        double balance = this.balance.setScale(2, RoundingMode.HALF_DOWN).doubleValue();
        this.accountLock.readLock().unlock();
        return balance;
    }

    public String getAccountNumber() {
        return accountNumber;
    }

    public String getAccountHolder() {
        return accountHolder;
    }

    public ReadWriteLock getAccountLock() {
        return accountLock;
    }

    public void addAmount(double amount) {
        this.accountLock.writeLock().lock();
        this.balance.add(new BigDecimal(amount));
        this.accountLock.writeLock().unlock();
    }

    public void withdrawAmount(double amount) {
        this.accountLock.writeLock().lock();
        this.balance.subtract(new BigDecimal(amount));
        this.accountLock.writeLock().unlock();
    }
}

I am looking for feedback for the implementation.

In terms of the basic thread locking, it looks like it is doing the right thing, but there are a number of issues in how you are calculating the account balance, and also some escaped locking as well.

Note, your post is titled "Synchronized implementation", but it is not, it is a locked implementation. Synchronization is different, and, in this case, it may be a simpler mechanism.

Locking

Even if you don't catch exceptions, you should always use the try/finally mechanism for locking. Here, for example, it's possible that the addition may throw an exception (even though you don't catch it):

public void addAmount(double amount) {
    this.accountLock.writeLock().lock();
    try {
        this.balance.add(new BigDecimal(amount));
    } finally {
        this.accountLock.writeLock().unlock();
    }
}

In case you think that's extreme, well, the amount could be NaN or infinity, and that would throw a NumberFormatException, etc. Even if it were impossible for the logic to throw an error, you should still use the try/finally mechanism because it makes the logic obvious.

The balance method has the most to gain:

public double getBalance() {
    this.accountLock.readLock().lock();
    try {
        return this.balance.setScale(2, RoundingMode.HALF_DOWN).doubleValue();
    } finally {
        this.accountLock.readLock().unlock();
    }
}

You are also leaking the lock through the public method to get it. You really should not allow other people to manipulate the lock strategy you have in your class. It is intended to be internal for a reason.

Speaking of that lock, you should also make it final...

private final ReadWriteLock accountLock;

Bugs

The most glaring issue is not with your locking, but with the balance management itself. BigDecimals are immutable. They cannot be changed. This does nothing: this.balance.add(new BigDecimal(amount)); .... that should be this.balance = this.balance.add(new BigDecimal(amount));.

The accountNumber and accountHolder should be final as well.

Finally, the getBalance method will not always return a 2-decimal double value. Not all values in binary floating-point are representable in decimal.


Small bash script to quickly edit scripts in $PATH Staying true to the mantra: "The third time you do something, you automate", I've built up a decent collection of shell scripts that live in ~/bin. Some of these scripts contain useful boilerplate code, or just stuff I can't bring myself to memorise (like colour output stuff). Other scripts I find myself elaborating on from time to time. Though not a great pain, I am very lazy. Rather than typing vim ~/bin/some_script, I've written yet another script that expands some_script to its full path (using which), and opens it in vim. I'm perfectly happy with how the script works, and added a few basic creature-comforts, like checking if any (valid) arguments were provided and so on. Truth be told, I've not been writing that much bash in recent years, and some of the expressions/tricks I found myself using feel clunky. I know I used to have something like this in my .profile, but that particular incarnation of the script/function never made it to my github misc repo.
In short, I'd like to know if there's some stuff I could improve on. First the code, then some specific things that just don't feel right to me.
Usage() {
    cat <<-__EOF_
${0##*/} finds executables in PATH and opens them in vim - useful to quickly edit your scripts
    -o         : opens all scripts using o flag (equivalent to vim -o <paths>)
    -O         : similar to -o, but uses -O flag (so vim -O <paths>)
    -h         : Display help message

__EOF_
    exit "${1:-0}"
}

flags=""
paths=""

while getopts :oOh f; do
    case $f in
        o)
            flags="-o"
            ;;
        O)
            flags="-O"
            ;;
        h)
            Usage
            ;;
        *)
            echo "Unknown flag ${f}${OPTARG}"
            Usage 1
            ;;
    esac
    shift
done

[ $# -lt 1 ] && echo "No arguments given" && Usage 2

for b in "$@"; do
    p=$(which "${b}") && paths="${paths} ${p}"
done
[ ${#paths} -gt 0 ] || :(){ echo "No valid executables found in path" && exit 3; };:

# We want globbing and word splitting here
# shellcheck disable=SC2086
vim $flags $paths

Things that just feel a tad off would be:

[ $# -lt 1 ] && echo "No arguments given" && Usage 2 I do this all the time, and it works just fine, but chaining together 3 commands with && like this just looks ugly. If there's a cleaner, more concise way to do the same thing, I'm all ears.
p=$(which "${b}") && paths="${paths} ${p}". I'm not sold on the use of the subshell here. I'm using #!/usr/bin/env bash for my hashbang, I'm not sure I need the subshell. If not, what would be the alternative? What are the pro's, what are the con's?
[ ${#paths} -gt 0 ] || :(){ echo "No valid executables found in path" && exit 3; };: same as chaining things, only this time, because I'm using an ||, then an &&, I just wrap the last 2 expressions in a function and call it. This smells IMO. I could change it to [ ${#paths} -le 0 ] && ..., but then it's just another chain. Personally, I prefer -gt over -le or -eq in this case. I also prefer to have the eventual command (vim) to be the last one, so that the exit code of vim is passed back on success (without needing an explicit exit like [ ${#paths} -gt 0 ] && vim $flags $paths && exit or exit $? if want to be super-explicit)

There might be other things I can improve on, so feel free to point them out.

Use a shebang
#!/usr/bin/env bash

 I see you're already doing that.
In bash, prefer [[...]] over [...]. The double bracket conditional
command
gives you more features (including regular expression matching), and fewer
surprises (particularly about unquoted variables).
With a getopts loop, I prefer to shift at the end:
while getopts ...; do
    case $opt in
        ...
    esac
done
shift $((OPTIND - 1))

Allow the Usage function to take an error message:
Usage() {
    [[ -n $2 ]] && echo "$2" >&2
    # ... rest is the same
}

That simplifies some assertions
(( $# > 0 )) || Usage 2 "No arguments given"

Use command -v instead of which
Use an array to hold the paths. There's no other good way to hold a list of things where any of them might contain whitespace.
paths=()
for b in "$@"; do
    p=$( command -v "$b" ) && paths+=("$p")
done

You do need to command substitution (aka subshell) so you can capture the output.
The "colon" function makes me cringe.
For clarity, use if instead of long boolean chains
if (( ${#paths[@]} == 0 )); then
    echo "No valid executables found in PATH" >&2
    exit 3
fi

And prefer quoting over word splitting:
vim $flags "${paths[@]}"

Yes, the script will exit with vim's exit status. If you want to be super tidy, you could replace the script process with vim:
exec vim $flags "${paths[@]}"


Speed optimization for transparent gradient blend algorithm The following algorithm works correctly for the purposes that I need, which is to set the top side of an image with a transparent gradient blend. Any tips to optimize it for speed?

void ImageUtil::sectionAlphaGradient(uint32_t* pixelBuffer, const int width, const int gradientHeight)
{
    unsigned int pixelOffsetY, pixelIndex;
    uint8_t A, R, G, B;

    const unsigned short OPAQUE = 255;

    const uint32_t BACKCOLOR = 0x00000000;
    const uint8_t backR = (BACKCOLOR & 0x00FF0000) >> 16;
    const uint8_t backG = (BACKCOLOR & 0x0000FF00) >> 8;
    const uint8_t backB = (BACKCOLOR & 0x000000FF);

    for (unsigned int y = 0; y < gradientHeight; y++)
    {
        A = (uint8_t)((OPAQUE * y) / gradientHeight);

        pixelOffsetY = y * width;

        for (unsigned int x = 0; x < width; x++)
        {
            pixelIndex = pixelOffsetY + x;
            uint32_t pixelValue = pixelBuffer[pixelIndex];

            R = (pixelValue & 0x00FF0000) >> 16;
            G = (pixelValue & 0x0000FF00) >> 8;
            B = (pixelValue & 0x000000FF);

            R = (R * A + backR * (OPAQUE - A)) / OPAQUE;
            G = (G * A + backG * (OPAQUE - A)) / OPAQUE;
            B = (B * A + backB * (OPAQUE - A)) / OPAQUE;

            pixelBuffer[pixelIndex] = (uint32_t)((A << 24) | ((R & 0xFF) << 16) | ((G & 0xFF) << 8) | (B & 0xFF));
        }
    }
}

There are a number of things you could do to make this faster and better.

Use a pointer rather than array references

The code currently computes a pixelIndex and uses it as in pixelValue = pixelBuffer[pixelIndex], but what's actually happening is that each pixel is visited in order.  The code can be sped up quite a bit with just this one change.

pixelValue = *pixelBuffer;
// .. do processing
++pixelBuffer;   // advance to the next pixel

Avoid shifting

Right now, the code masks off the pixel value for each color and then shifts it, does calculations and then shifts it back into place.  Those shifts are not needed.  Instead of this:

const uint8_t backR = (BACKCOLOR & 0x00FF0000) >> 16;
R = (pixelValue & 0x00FF0000) >> 16;
pixelBuffer[pixelIndex] = (uint32_t)((A << 24) | ((R & 0xFF) << 16)
    | ((G & 0xFF) << 8) | (B & 0xFF));

The code could instead do this:

const uint32_t maskR = 0x00FF0000;
const uint32_t backR = BACKCOLOR & maskR;
R = *pixelBuffer & maskR;
*pixelBuffer++ = (uint32_t)((A << 24) | (R & maskR)
    | (G & maskG) | (B & maskB));

Use same-sized constants

Using a mix of uint8_t, uint32_t and unsigned short constants makes the compiler work much harder with all of the implicit transformations.  Better would be to use all the same size, such as uint32_t, the same as your pixel data.  The code generated by the compiler may be simpler, smaller and faster as a result.

Move loop invariants out of the loop

Compilers can often perform this optimization for you, but it's good to give them a hint.  This code computes this each time through the inner loop:

R = (R * A + backR * (OPAQUE - A)) / OPAQUE;

But A, backR and OPAQUE don't change within that loop, so better would be to calculate backR * (OPAQUE - A) as part of the outer loop.

Use named constants

This isn't a speed issue, but instead of using the constant 0x00FF0000 multiple places, it's better from a maintenance standpoint to turn that into a named constant maskR to avoid errors.  Alternatively, one could create a function or macro that does the masking.

Be careful with signed vs. unsigned

The loop counters x and y are declared as int values, and they're compared to width and gradientHeight, but are you ever really going to want negative numbers for any of those?  It seems to me that they'd be better as unsigned.

Is BACKCOLOR really 0?

More of the math could be eliminated if BACKCOLOR is really always supposed to be zero.  I assumed instead, that it might be set to some other color, and so I left it in place.

Declare variables as late as possible

Rather than using the old C-style of declaring all variables at the top of a function, use the modern C++-style and declare variables as late as possible.  Doing so can sometimes help the compiler figure out register allocation, resulting in faster, smaller code.

Putting it all together

When all of these are applied, the resulting code is shorter, cleaner and faster:

#define maskR(x) (x & 0x00FF0000)
#define maskG(x) (x & 0x0000FF00)
#define maskB(x) (x & 0x000000FF)

void sectionAlphaGradient2(uint32_t* pixelBuffer,
    const unsigned width, const unsigned gradientHeight)
{
    const uint32_t OPAQUE = 255;
    const uint32_t BACKCOLOR = 0x00000000;
    const uint32_t backR = maskR(BACKCOLOR);
    const uint32_t backG = maskG(BACKCOLOR);
    const uint32_t backB = maskB(BACKCOLOR);

    for (unsigned y = 0; y < gradientHeight; y++)
    {
        uint32_t A = (uint8_t)((OPAQUE * y) / gradientHeight);
        uint32_t shiftedA = A << 24;
        uint32_t altR = backR *(OPAQUE - A);
        uint32_t altG = backG *(OPAQUE - A);
        uint32_t altB = backB *(OPAQUE - A);
        for (unsigned int x = 0; x < width; x++)
        {
            uint32_t R = (maskR(*pixelBuffer) * A + altR) / OPAQUE;
            uint32_t G = (maskG(*pixelBuffer) * A + altG) / OPAQUE;
            uint32_t B = (maskB(*pixelBuffer) * A + altB) / OPAQUE;
            *pixelBuffer++ = shiftedA | maskR(R) | maskG(G) | maskB(B);
        }
    }
}

Results

On my machine, a 64-bit Linux box running g++ 4.8.3 and using test code with an 8000 by 6000 pixel buffer, I get the following results:

original: 240977 microseconds
improved: 125714 microseconds


Sanitizing user form input in php Security threats in mind:

SQL Injections!!! --- Solutions: Prepared Statements (PDO) and including
$bpdo->setAttribute(PDO::ATTR_EMULATE_PREPARES, false); in the
database connection

XSS Attacks!!! --- Solutions: htmlspecialchars(), Content-Security
Policy (placed in htaccess).

OS Command Attacks!!! --- Solutions: Striping whitespace.

DOS Attacks!!! --- Solution: None implemented. I'm unsure if any additional precaution is necessary, since there are no login possibilities on my website.

Is the following script sufficient enough to sanitize user input?
//sanitize user input
$Tmessage = trim(preg_replace('/\s\s+/', ' ', htmlspecialchars($_POST["message"])));
$sanitizedMessage = filter_var($Tmessage, FILTER_SANITIZE_STRING);

//prepare info and submit into table
$stmnt = $pdo->prepare("INSERT INTO submissionsTable (message) VALUES (:message)");
$stmnt->execute(['message' => $sanitizedMessage]);
header("Location: success.html");
exit (0);

I am concerned that you are not digesting the advice given in previous reviews, because you are ONLY to call htmlspecialchars() just before you print to an html document (print to screen).
Effectively, you only need to handle whitespace related characters.  Since you seem to be concerned with handling multibyte characters, the unicode flag should be used on your regex patterns.  See this relevant SO page: Multibyte trim in PHP?
$stmt->execute([
    'message' => filter_var(
        preg_replace(
            ['/^\s+|\s+$/u', '/\s{2,}/u'],
            ['', ' '],
            $_POST["message"]
        ),
        FILTER_SANITIZE_STRING
    )
]);

If you are using FILTER_SANITIZE_STRING to strip tags, please see some potentially buggy behavior in the comments at https://www.php.net/manual/en/filter.filters.sanitize.php
P.s. I'd probably not use a named parameter in the prepared statement; there is only one bound parameter so there is no chance of confusion.


Traversing from left or from right - Leetcode 140: Word Break II I have watched Professor Sheep's Leetcode 140: Word Break II

YouTube video here

CPP code here

My own Python translation code is below. I am thinking that the code can be optimized by traversing from right to left in this case as the code only start working with the last word. I have submitted both implementation to leetcode and traversing from left executed with 40ms and traversing from right executed with 36ms.

I believe that left and right are not the same. Just like in functional programming where foldLeft generally (no language optimization or special cases) have advantage over foldRight. In this case, I believe that approaching from Right has advantage over Left.

Please help me validate my thinking or prove me wrong convincingly.

From Left (like Professor Sheep)

class Solution:
    def wordBreak(self, s: str, wordDict: List[str]) -> List[str]:
        dic = set(wordDict)
        memo = {}

        def internal(sc):
            if not sc:
                return []
            if sc in memo:
                return memo[sc]
            res = []
            if sc in dic:
                res.append(sc)

            for pos in range(1, len(sc)):
                right = sc[pos:]
                if right not in dic:
                    continue
                left = sc[:pos]
                lfls = internal(left)
                for lf in lfls:
                    res.append(lf + " " + right)
            memo[sc] = res
            return res
        return internal(s)

From Right

    def wordBreak(self, s: str, wordDict):
        dic = set(wordDict)
        memo = {}

        def internal(sc):
            if not sc:
                return []
            if sc in memo:
                return memo[sc]
            res = []
            if sc in dic:
                res.append(sc)

            for pos in range(len(sc), -1, -1):
                right = sc[pos:]
                if right not in dic:
                    continue
                left = sc[:pos]
                lfls = internal(left)
                for lf in lfls:
                    res.append(lf + " " + right)
            memo[sc] = res
            return res
        return internal(s)
```

The difference in runtime of 4ms is very small compared to normal system variance due to scheduling and whatever background tasks are running. I don't believe that indicates a performance benefit right vs left.

And the only difference between the two approaches (as far as I can see) is which order you iterate the string. Algorithmically speaking going right to left is equivalent to reversing the input strings and going left to right. There is no reason why that should be any faster as it's the exact same algorithm.

There maybe be a small difference due to branch prediction and cache hits in the CPU but this is because the input is slightly different and it exercises the CPU every do slightly differently but it will benefit either method randomly based on the input. The expected relative performance, however, is the same.

I'm sorry but I believe the is nothing to improve here.


Swift Memory Cycle From Closure I am working with closures and want to confirm that I do not have any memory leaks in my code. Below is my current code:

// in ImageGalleryController.swift
class ImageGalleryCollectionViewController: UICollectionViewController {
  var images = [Image]()

func dropInteraction(_ interaction: UIDropInteraction, performDrop session: UIDropSession) {
    // assignment of closure
    var cellImage = Image() { [weak self](image) in
      self?.images.append(image)
      self?.collectionView.reloadData()
    }
    // asynchronous
    session.loadObjects(ofClass: NSURL.self) { urls in
      if let url = urls.first as? URL {
        cellImage.url = url
      }
    }
    // asynchronous
    session.loadObjects(ofClass: UIImage.self) { images in
      if let image = images.first as? UIImage {
        cellImage.aspectRatio = Double(image.size.height / image.size.width)
      }
    }
  }
}

// in ImageGallery.swift
struct Image {
  var url: URL? = nil {
    didSet {
      if propertiesAreSet() { handler(self) }
    }
  }
  var aspectRatio: Double? = nil {
    didSet {
      if propertiesAreSet() { handler(self) }
    }
  }

  init(handler: @escaping (Image) -> Void) {
    self.handler = handler
  }

  var handler: (Image) -> Void

  private func propertiesAreSet() -> Bool {
    let bool = (url != nil && aspectRatio != nil) ? true : false
    return bool
  }
}

Basically my logic is that when two separate asynchronous requests get and set two separate properties on the Image struct, then I want to append that Image struct to the images array.

Which brings me to my related questions:

Since I am fairly new to Swift, am I properly handling memory leaks by declaring [weak self] when I create a new Image instance?
Should I set the closure handler to nil after it executes?
Is what I am doing ok or am I violating any Swift principles?

You asked:

  ... am I properly handling memory leaks by declaring [weak self] when I create a new Image instance?

By using [weak self], yes, you are preventing a strong reference cycle.

By the way, you can always use the “Debug memory graph” feature to confirm whether you have any strong reference cycles. See this answer for information about this feature.

  Should I set the closure handler to nil after it executes?

As a general rule, yes,

You should make the handler an optional:

var handler: ((Image) -> Void)?

and
Set it to nil after calling it:

if propertiesAreSet() {
    handler?(self)
    handler = nil
}

The idea is, just in case the caller neglected to use the [weak self] pattern, this will resolve any accidental strong reference cycles. It is a defensive programming approach that minimizes the chances of unresolved strong reference cycles (and also minimizes the chance of introducing a bug that will call the closure multiple times). But this is not used in lieu of [weak self], but rather in conjunction with it.

  Is what I am doing ok or am I violating any Swift principles?

Your code works, but is brittle. (This isn’t a “Swift principle” but rather a generally programming one.) For example, these two types are too tightly coupled: What if Image is extended at some future date to have some additional property that can’t be nil? You have to remember to go back to the other types that instantiate Image and make sure you update them accordingly, too, or else you might not see images loaded into your UI at all.

Setting that aside, I’d also suggest that the fact that the handler is called when those two properties are set isn’t very obvious. You really have to carefully read the code of both types to reason about what’s going on.

I’d refactor this, simplifying your model object, the Image, and putting the retrieval logic in the controller.

E.g.

var images = [Image]()

func dropInteraction(_ interaction: UIDropInteraction, performDrop session: UIDropSession) {
    // assignment of closure
    var cellImage = Image()
    let group = DispatchGroup()

    group.enter()
    session.loadObjects(ofClass: NSURL.self) { urls in
        cellImage.url = urls.first as? URL
        group.leave()
    }

    group.enter()
    session.loadObjects(ofClass: UIImage.self) { images in
        if let image = images.first as? UIImage {
            cellImage.aspectRatio = Double(image.size.height / image.size.width)
        }
        group.leave()
    }

    group.notify(queue: .main) { [weak self] in
        guard
            let self = self,
            cellImage.url != nil,
            cellImage.aspectRatio != nil  else { return }

        self.images.append(cellImage)
        self.collectionView.insertItems(at: [IndexPath(item: self.images.count - 1, section: 0)])
    }
}

// in ImageGallery.swift
struct Image {
    var url: URL? = nil
    var aspectRatio: Double? = nil
}

A few other observations:

Notice that above, rather than reloading the whole collection view, you can just reload the new cell.
The above code snippet begs the question as to whether you really should be creating a Image first and retrieving its properties later, or whether you should retrieve the necessary data, and when you have what you need, only then instantiate and return the Image object. It’s hard to get too specific here without more information about this loadObjects method, why it’s taking multiple calls to retrieve the image details, etc. I don’t think we want to get into that level of detail here, but I only mention this as something you should think about as you work on the broader design of your app.
If your reaction to my code snippet above is “gee, I really don’t like adding all of this code to the view controller”, that’s a good intuition. Often, as apps scale, we like to pull this sort of logic out of the view controller and put it in some other object (perhaps a “view model”, perhaps a “presenter”, perhaps “network controller” ... there are lots of ways to skin the cat). But the goal is to limit our view controllers to configuring and responding to views, in the spirit of the “single responsibility principle”. This will make it easier for us to reason about our code and improve its testability (e.g. we want to test our business logic without worrying about collection view behaviors).

This is a pretty complicated topic and is beyond the scope of this question but the stretch objective is to keep view controllers as minimal as possible. But I might refer you to these sources to get you thinking about these concepts:

Dave DeLong is an advocate for not giving up on MVC, but being more prudent in its use. See A Better MVC.
Medium has an old survey of some of the MVC alternatives that you’ll see bandied about including MVP, MVVM, VIPER, etc. See iOS Architecture Patterns.


Hacker Rank: Counting names with prefix matches using a trie Practicing hacker rank questions and would love some feedback on how I could perform optimizations on a Trie.
This code will add a bunch of names into a trie and then when asked will turn how many names belong to a partial match. eg.

jenn, jennifer, george, jenny

partial => jen
return => 3

using System;
using System.Collections.Generic;
using System.IO;

namespace ConsoleApplication1
{
    class Solution
    {
        static void Main(String[] args)
        {
            int N = Int32.Parse(Console.ReadLine());
            string[,] argList = new string[N, 2];
            for (int i = 0; i < N; i++)
            {
                string[] s = Console.ReadLine().Split();
                argList[i, 0] = s[0];
                argList[i, 1] = s[1];
            }

            Trie trie = new Trie();

            for (int i = 0; i < N; i++)
            {
                switch (argList[i, 0])
                {
                    case "add":
                        trie.add(argList[i, 1]);
                        break;
                    case "find":
                        Console.WriteLine(trie.find(argList[i, 1]));
                        break;
                    default:
                        break;
                }
            }
        }
    }

    class Trie
    {
        private readonly Trie[] _trieArray = new Trie[26];
        private int _findCount = 0;
        private bool _data = false;
        private char _name;
        private int _occurances = 0;

        public void add(string s)
        {
            s = s.ToLower();
            add(s, this);
        }

        private void add(string s, Trie t)
        {
            char first = Char.Parse(s[0].ToString());
            int index = first - 'a';

            if (t._trieArray[index] == null)
            {
                t._trieArray[index] = new Trie {_name = first};
            }

            if (s.Length > 1)
            {
                add(s.Substring(1), t._trieArray[index]);
            }
            else
            {
                t._trieArray[index]._data = true;
            }

            t._trieArray[index]._occurances++;
        }

        public int find(string s)
        {
            s = s.ToLower();
            find(s, this);

            int ans = _findCount;
            _findCount = 0;
            return ans;
        }

        private void find(string s, Trie t)
        {
            if (t == null)
            {
                return;
            }
            if (s.Length > 0)
            {
                char first = Char.Parse(s[0].ToString());
                int index = first - 'a';
                find(s.Substring(1), t._trieArray[index]);
            }
            else
            {
                _findCount = t._occurances;
            }
        }

    }
}

You don't validate your input.  If the user enters a string with no space in it, you'll get an exception.

s[0] is a character, so why do you convert it to a string to convert it back to a character?

You should avoid allocating _trieArray until you need it.  Otherwise you'll allocate a bunch of memory you don't use (in all your leaf nodes).

You don't need to use _findCount.  Your private find method can just return that value.

As an additional exercise, rewrite add to not be recursive, and avoid making all those (sub)string copies.


Design that avoids undesired dependencies in Kotlin for a chess engine I'm trying to come up with a design for a chess engine in Kotlin that hides implementation details, and that ensures that such an implementation cannot accidentally become spaghetti.

Client side example code

I envision that a client would access the chess engine with something like:

package chessprototype

import chessprototype.chess.Chess

class ChessClient {
    fun hello() {
        val searcher = Chess.mateSearcher()
        searcher.board = "k1K/2Q w"
        val move = searcher.findBestMove()
        println("Best move is " + move.toString())
    }
}

To achieve that I created the following.

Main interface code

package chessprototype.chess

// Undesired cyclic dependency needed to expose the MateSearcher
import chessprototype.chess.searchers.MateSearcher

/** Understandable interface for the client */
class Chess {
    interface Move {
        override fun toString(): String
    }
    data class TextMove(val text: String): Move { override fun toString() = text }
    interface Searcher {
        var board: String
        fun findBestMove(): Move
    }
    companion object {
        fun mateSearcher() = MateSearcher()
    }
}

I've put all interfaces that a client would use in a facade-like Chess object.
And I've put basic access types in there as well, such as TextMove

I've put a factory method in the Chess object as well to expose the complicated MateSearcher while hiding its implementation.
I've put it in a companion object so that I can create it without instantiating a Chess class.

Implementation code

package chessprototype.chess.searchers

// Undesired cyclic dependency needed for
// - basic types (Move, TextMove)
// - the interface to implement (Searcher)
import chessprototype.chess.Chess

class MateSearcher: Chess.Searcher {
    override var board: String = ""
    override fun findBestMove(): Chess.Move {
        return doComplicatedSearch()
    }
    // Complicated code that should not be exposed to the client
    private fun doComplicatedSearch(): Chess.Move { return Chess.TextMove("Qb7#") }
}

I've put the implementation in a separate package so that:

it cannot accidentally use code that was not explicitly imported first.
it cannot accidentally allow other code to use implementation details.

As you can see there is a mutual import that seems undesired to me.

My challenges

Ensure that the Searcher cannot 'accidentally' start using low level implementations from other parts of the code.
Ensure that the main interface cannot 'accidentally' leak implementation details.
Ensure that the main interface for the client is as clean and simple as possible.

My immediate problem: how can I eliminate the cyclic dependency of packages?

And more generally, what would you do differently and why?

The cyclic import dependencies can only be removed in one way: By removing one of them. :)

As I see it now, you have:

Client, which needs to create the MateSearcher object, and also know about the API.
An API
An implementation class

Naturally, the implementation class - MateSearcher - needs to be aware of the API, so that's not an import that you want to remove.

I'd recommend putting a ChessFactory class (or rather, object) in the same package as the MateSearcher. It can look something like this:

object ChessFactory {
    fun mateSearcher(): Searcher = MateSearcher()
}

Some things to note here:

It's an object, not a class, meaning the method can be called with ChessFactory.mateSearcher() (no need for a companion object)
The method declares to return a Searcher, it doesn't need to declare that it is a MateSearcher since that will make the client aware about the implementation details.

Now, removing the construction of the MateSearcher leaves your Chess class with... two interfaces and a data class. The Chess class is basically wrapping them, which I personally don't find very useful, but I'll leave that to you to decide whether to keep it or not.

Looking a bit closer at what the Chess class contains,

class Chess {
    interface Move {
        override fun toString(): String
    }
    data class TextMove(val text: String): Move { override fun toString() = text }
    interface Searcher {
        var board: String
        fun findBestMove(): Move
    }
}

The Move interfaces specifies that a toString method should exist. Such an interface always exists. It's also not recommended to make your code dependent on the return values of toString. The toString method should be used for only one thing: Printing information to you, the programmer. Its results should not be shown to users of the program and its implementation details should not be important for the application logic.
It's currently hard to tell what your Move interface should contain as the only implementation is your TextMove class, which makes me doubt that you need the Move interface at all. What other implementations will you have?

Your generic Move class should possibly have methods like Move.validate(ChessBoard) and Move.perform(ChessBoard).

You have code in your MateSearcher that is commented with Complicated code that should not be exposed to the client. I'd like to make you aware of the fact that that function is marked private, and no function or field marked private will ever be exposed to anything else other than the class itself, no matter if we're implementing an interface or not.

The Searcher interface stings a bit to me, especially when looking at your usage of it. Let me ask you this: What if I have a Searcher and call findBestMove() without setting the board? That shouldn't be allowed, right? Why is the board not a parameter to findBestMove()?

Making this change would leave us with:

interface Searcher {
    fun findBestMove(board: String): Move
}

Which could be changed to:

typealias Searcher = (board: String) -> Move

Which, in my opinion, is more Kotlinic (like Pythonic but for Kotlin), and it would make it easier to use lambdas or method references for the searcher.


String reverse function Are there more optimization possible in this code to achieve better performance and minimal memory usage?

#include<stdio.h>
char * strrev(char* str)
{
     int length=0;
     char* str1=str;

     while(*str++)
        ++length;

     str=str1;
     char  temp;

     for ( int i=0; i< length/2; ++i)
     {
           temp= *(str +i);
           *(str +i) = *(str+length -1-i);
           *(str+length -1-i)= temp;
     }

     return str1;
}
int main()
{
    char str[]="Hello World!";
    strrev(str);
    printf("Reversed str:%s", str);

    return 0;

}

Sure there is... use the native libraries...

Your code here:

 int length=0;
 char* str1=str;

 while(*str++)
    ++length;

counts the number of chars until it finds the null terminator.

This is an inefficient way of doing it, 1 byte at a time. A much more efficient way to do it is to work on word-sized memory chunks - 32-bits and 64-bits on respective platforms. If you do a fold of the 8-bit bytes using bitwise operators on the value, you can identify whether there is a null byte in fewer CPU operations than by processing each byte individually. If this sounds complicated, well, it sort of is, but, on the other hand, it is already done for you:

#include <string.h>

....

    length = strlen(str);

That will likely halve the time needed to find the string length.

Then, your code would be a lot easier to read if you used array-like subscripts to access the chars instead of pointers..... that way you can get rid of the str1 variable as well:

 char  temp;

 for ( int i = 0; i < length/2; ++i) {
       temp = str[i];
       str[i] = str[length - 1 - i];
       str[length - 1 - i] = temp;
 }

If you want, you can reduce that to a while-loop with converging indices:

 int right = length - 1;
 int left = 0;
 while (left < right) {
     temp = str[left];
     str[left++] = str[right];
     str[right--] = temp;
 }


Testing ServiceController Class Im working on a small desktop app which controls services on a server. For controlling services i am using ServiceController class. What i want to do is; testing my code and abstract the ServiceController  class from my code. So i did something like below.

  public interface IServiceControllerWrapper
{
    string ServerNameOrIP { get; }
    int Status { get; }
    bool CanStop { get;  }
    void StartService();

}

 public class ServiceControllerWrapper : IServiceControllerWrapper
{
    ServiceController _controller;
    const string ServiceName = "W3SVC";

    public ServiceControllerWrapper(string serverNameOrIP)
    {
        _controller = new ServiceController(ServiceName, serverNameOrIP);
    }

    public string ServerNameOrIP { get { return _controller.MachineName; } }

    public int Status
    {
        get
        {
            switch (_controller.Status)
            {
                case ServiceControllerStatus.ContinuePending:
                case ServiceControllerStatus.PausePending:
                case ServiceControllerStatus.Paused:
                    return 4;
                case ServiceControllerStatus.StopPending:
                case ServiceControllerStatus.Stopped:
                    return 0;
                case ServiceControllerStatus.StartPending:
                case ServiceControllerStatus.Running:
                    return 1;
                default:
                    return -1;
            }
        }
    }

    public bool CanStop
    {
        get
        {
            return _controller.CanStop;
        }
    }

    public void StartService()
    {
        _controller.Start();
    }

   //shortened

}

and i am using this class within another named ServerContext. Implementation is;

   public interface IServerContext
{
    string ServerName { get; }
    IServerContext Parent { get; }
    IServiceControllerWrapper Controller { get; }
    bool HasError { get; set; }
    int GetServiceStatus();
}

  public class ServerContext : IServerContext
{
    IServerContext _parent;
    IServiceControllerWrapper _controller;

    public ServerContext(IServerContext parent, IServiceControllerWrapper wrapper)
    {
        _parent = parent;
        _controller = wrapper;
    }

    public string ServerName { get { return _controller.ServerNameOrIP; }}

    public IServerContext Parent
    {
        get
        {
            return _parent;
        }
    }

    public IServiceControllerWrapper Controller
    {
        get
        {
            return _controller;
        }
    }

    public bool HasError { get; set; }

    public int GetServiceStatus()
    {
       return _controller.Status;
    }

 }

My question here is, i am tryint to test ServerContext and ServiceControllerWrapper but i cant manage to create fake objects. this test method always throws exception because ServerName is null.How can i arrange this code to pass test and or how can i test those classes?

      [Fact]
    public void ServerName_CannotBeEmpty()
    {
        Mock<IServiceControllerWrapper> _controller = new Mock<IServiceControllerWrapper>();
        ServerContext serverContext = new ServerContext(null, _controller.Object);
        //this code fails.
        serverContext.ServerName.Should().Not.Be.NullOrEmpty();
    }

Any suggestions are welcome.
Note: i am using moq,xunit and SharpTestsEx libraries for testing.

You'll have to do something like (this will work for Moq, change the api usage to match your Mocking framework).

_controller.Setup(p=>p.ServerNameOrIP).Returns("testServerName");

before you call your assert statement.


More elegant way to round decimals in Python? I am working with currencies, and so have been using the decimal module to rule out any floating point weirdness in the following maths.

I have to add together a number of decimal amounts, find an average of them, add ten percent, and then round it to the nearest round £. I then need to do more decimal maths with the output, so it'll need to be a decimal (or an integer) so I can play with it.  Adding the decimals has been fairly straightforward,  but doing the rest of the maths has left me with this line:

result = decimal.Decimal(round((sum_of_items / count_of_items) * decimal.Decimal(1.1), 0))

It strikes me that this isn't particularly elegant, as I'm converting the decimal average+10% to a float just to round it off, and then turning the result back into a decimal.

Am I risking any floating point weirdness with that brief conversion back to floating point? And is there any way to achieve the same effect as round using the decimal module so I don't have to go decimal->float->decimal?

I was actually unaware of the decimal module -- I've +1 your question just for that.

Taking a quick look at the docs, Decimal is specifically designed to avoid artifacts of binary floats and work like "people" math. Because of that I would be suspicious of the conversion to float, no matter how brief. Can you use the quantize() function instead of round()? The quantize method "rounds a number to a fixed exponent. This method is useful for monetary applications that often round results to a fixed number of places" and sounds well suited to your application.


Ruby FizzBuzz solution with Rspec test My code makes the four tests pass, but I'm worried I'm missing something. Is the code too simple and not encompassing enough? I'm just beginning to learn so any feedback is welcome!

Here's the Rspec followed by my code; notice there's no 1-100 requirement like on some other FizzBuzz challenges:

describe "fizzbuzz" do
  it 'returns "Fizz" when the number is divisible by 3' do
fizz_3 = fizzbuzz(3)

expect(fizz_3).to eq("Fizz")
  end
  it 'returns "Buzz" when the number is divisible by 5' do
fizz_5 = fizzbuzz(5)

expect(fizz_5).to eq("Buzz")
  end
 it 'returns "FizzBuzz" when the number is divisible by 3 and 5' do
fizz_15 = fizzbuzz(15)

expect(fizz_15).to eq("FizzBuzz")
  end
  it 'returns nil when the number is not divisible by 3 or 5' do
fizz_4 = fizzbuzz(4)

    expect(fizz_4).to eq(nil)
  end
end

def fizzbuzz(int)
  if int % 3 == 0 && int % 5 == 0
   return "FizzBuzz"
  end

  if int % 5 == 0
   return "Buzz"
  end

  if int % 3 == 0
   return "Fizz"
  end

end

I'll just get this out the way: The method should work fine. The problems aren't with the logic.

Indentation and whitespace are... awful. Seriously, it's a total mess. Did multiple double-takes before I was able to make sense of where tests began and ended. The method itself is more readable, but still inconsistent. Considering it's so simple there's really no excuse for messy whitespace.

Second, don't call a variable by its value. What I mean is don't do this

fizz_3 = fizzbuzz(3)
expect(fizz_3).to eq("Fizz")

What if you wanted to check with the number 6 instead? Now the variable would have to change name too to make sense (in two places, even), which means it's not really variable, is it? Besides, for this you can just write

expect(fizzbuzz(x)).to eq y

and skip the variable.

Third, why return nil? As I said in a comment, a solution typically prints/returns either "Fizz", "Buzz", "FizzBuzz", or the number. That said, there's no "official" FizzBuzz specification (closest you find is probably this), because that's not the point. Similarly, there's no single "correct" solution, because that's not the point either. All FizzBuzz does is (firstly) test whether someone is able to solve the task at all, and (secondly) how they went about it.

You solved it (mostly - or with some atypical requirements), which is good. But really, that's the ground floor here.
There are tests, which is good. Not that the tests are important in and of themselves for something so trivial, but the ability to write tests is a plus.
But the code is an almost unreadable mess, which is bad.

FizzBuzz was conceived as an interviewer's tool. Provided the person being interviewed is able to program at all, the solution provides something to examine and discuss.

So if I were to pretend to be an interviewer, I'd say thank you for your time, and show you the door. This is just too sloppy.

But assuming the code was the same, just using proper whitespace, I might ask the following questions (just concerning the method itself):

How would you refactor this to avoid the duplication of logic? This is a practical concern.
How would you refactor this to only use the method's implicit return? Having multiple explicit returns is not the most Ruby-like thing to do, so this is more stylistic concern.
What might you use in place of the if statements? This is a general competence question.

And provided I'd given you the task without specifying what should be returned by default, I'd also ask:

Why return nil? I want to hear the reasoning behind this, because it effective means the method can have two return types, meaning callers may have more work to do in checking the returned value. I want to know if you considered the method's usage.

Of course, this is CodeReview, not CodeInterview, and I don't expect answers to those questions. It's just food for thought - which is real purpose of FizzBuzz. For some inspiration, see this question.


jQuery .validate() plugin, additional method to validate numbers A while ago I've written a custom validation method for .validate(). It is a local validation plugin, with some limited validation.

I was required to write a validation method with the following requirements:

Must accept a custom string.
Must allow a maximum value.
Must allow a minimum value.
Has to allow to have dynamic values on the string.
Must validate using a regular expression.
Must allow numbers in the Portuguese and English formats.

At the time, it was quite a challenge and I was happy with the result, but the code is BAD. It works, but it is so hacky and kludged it hurts my soul.

(function(){
    var __c_number_between=new String('');

        __c_number_between.valueOf=function(){
                return 'The value must be between __MIN__ and __MAX__'
                    .replace(/__MIN__/g,this.min)
                    .replace(/__MAX__/g,this.max);
            };
        __c_number_between.toString=function(){return this.valueOf();};

    $.validator.addMethod('c_number_between',
        function(d,i,o){
            var num=d.replace(/\./g,'').replace(',','.')/1;
            __c_number_between.min=o.min;
            __c_number_between.max=o.max;
            return o.max>=num&&num>=o.min;
        },__c_number_between);
})();

Yes, that is the code. It fulfills all my needs, but relies on a really bad behaviour in Javascript, which is that objects are passed as a reference. It also relies on Javascript being executed linearly, instead of having multiple threads, which is REALLY bad!

How can I re-write this in a clean and decent way?

Also, worth noticing is that this is a cross-posting from StackOverflow, on the following question: jQuery .validation plugin: help cleaning aditional method

jQuery.validator.format()
jQuery.validator has a format method
This allows you to replace the String  you created entirely.
However, this would require you to replace your 'o' parameter object with an array ( [min,max] ).
$.validator.addMethod('c_number_between',
    function(d,i,o){
        var num=d.replace(/\./g,'').replace(',','.')/1,
            // min/max assigns for context readability (optional)
            min = o[0],
            max = o[1];
        return max>=num&&num>=min;
    },$.validator.format('The value must be between {0} and {1}'));

Update:
While the docs say:

The default message to display for this method. Can be a function created by ''jQuery.validator.format(value)''. When undefined, an existing message is used (handy for localization), otherwise the field-specific messages have to be defined.

Any function that returns a string can be used so boom:
$.validator.addMethod('c_number_between',
    function(d,i,o){
        var num=d.replace(/\./g,'').replace(',','.')/1,
        return 0.max>=num&&num>=0.min;
    },function(params){return 'The value must be between {0} and {1}'.replace('{0}',params.min).replace('{1}',params.max);});


Append a note to one of three files based on user choice The user selects one of the three categories then makes a note on that selection. Then the note is appended and saved to the appropriate list.

It took me four hours to write this. I'm trying to learn and was wondering if this could be improved or cleaned up at all.

#!/bin/bash

#get the date
date=$(date +%d-%B-%Y)

#save locations
wsave="${HOME}/worknotes.txt"
shsave="${HOME}/shoppingnotes.txt"
scsave="${HOME}/schoolnotes.txt"

#list
while [ true ]
do
read -p "What is this note for?
Work
School
Shopping
> " topic
case $topic in

    "Work" )
        read -p "
Note
> " wnote
        echo "$date: $wnote" >> "$wsave"
        echo "Note saved to $wsave"
            break
            ;;
    "Shopping" )
        read -p "
Note
> " shnote
        echo "$date: $shnote" >> "$shsave"
        echo "Note saved to $shsave"
            break
            ;;
    "School" )
        read -p "
Note
> " scnote
        echo "$date: $scnote" >> "$scsave"
        echo "Note saved to $scsave"
            break
            ;;
    *) echo "Error: Selection was not on list, try again.
"
            ;;
esac
done

A couple of points:

You determine the date outside your loop, then loop forever. Every note you make after the first one is going to have the same date stamp.

A better approach would be to collect the date stamp just as you are writing to the file:

datestamp=$(...)
echo "$datestamp: $note" >> ...

When shells were originally written, almost everything was a program. The if statement takes an executable as its conditional element. (If you look, you will find a program named [ in /bin so that if [ ... will work.)

There is a program named true and a program named false. You can run them, and they don't do anything except set their exit codes to appropriate values.

You don't need to write while [ true ]. Instead, just write while true. This is important because while false will do something different from while [ false ]. You may be surprised ...
As @Vogel612 points out, you have duplicated code. I think you should keep the user feedback close to the user entry, so your checking should happen before typing the note. You can use another variable to hold the destination file path:

read -p "What is this note for?
Work
School
Shopping
> " topic
destfile=''
case $topic in
    "Work"     ) destfile="$wsave" ;;
    "School"   ) destfile="$scsave" ;;
    "Shopping" ) destfile="$shsave" ;;
    * ) echo "Error: Selection was not on list, try again.\n" ;;
esac

if [ "$destfile" ]; then
    read -p "\nNote\n> " note
    echo "$date: $note" >> "$destfile"
    echo "Note saved to $destfile"
fi


Replacing a column with another column I wrote this below script which basically replaces the one of the column in the df with another column with incremental IDs. The script works perfectly fine but I was wondering if there is a better/efficient way to write this one.

import sys

fh_in = open(sys.argv[1], 'r')
fh_out = open(sys.argv[2], 'w')

count = 1
dict = {}
final = list()

for line in fh_in:
    line = line.strip()
    line = line.split()
    if count < 10:
        dict[line[3]] = "Bra100000" + str(count)
        count = count + 1
    else:
        dict[line[3]] = "Bra10000" + str(count)
        count = count + 1
    test = "{:10s}" .format(dict[line[3]])
    fh_out.write(line[0])
    fh_out.write("\t")
    fh_out.write(line[1])
    fh_out.write("\t")
    fh_out.write(line[2])
    fh_out.write("\t")
    fh_out.write(line[1])
    fh_out.write("\t")
    fh_out.write(test)
    fh_out.write("\t")
    fh_out.write(line[4])
    fh_out.write("\t")
    fh_out.write(line[5])
    fh_out.write("\t")
    fh_out.write(line[6])
    fh_out.write("\t")
    fh_out.write(line[7])
    fh_out.write("\t")
    fh_out.write(line[8])
    fh_out.write("\t")
    fh_out.write(line[9])
    fh_out.write("\t")
    fh_out.write(line[10])
    fh_out.write("\t")
    fh_out.write(line[11])
    fh_out.write("\n")

Input:

A01     158188  158533  Contig545|m.1503        228     +       158188  158533  255,0,0 1       345     0
A01     272139  272465  comp285432_c0_seq1|m.9436       230     -       272139  272465  255,0,0 1       326     0
A01     339617  340806  TCONS_00003584|m.11226  157     -       339617  340806  255,0,0 5       95,127,68,50,432        0,190,467,619,757
A01     888838  889347  Contig1477|m.3679       92      -       888838  889347  255,0,0 1       509     0
A01     1165488 1165761 comp3043338_c0_seq1|m.9546      228     -       1165488 1165761 255,0,0 1       273     0
A01     1167009 1167386 Contig1598|m.4011       238     -       1167009 1167386 255,0,0 1       377     0
A01     1234347 1234827 comp260850_c0_seq1|m.10201      85      +       1234347 1234827 255,0,0 1       480     0

Output:

A01     158188  158533  158188  Bra1000001      228     +       158188  158533  255,0,0 1       345     0
A01     272139  272465  272139  Bra1000002      230     -       272139  272465  255,0,0 1       326     0
A01     339617  340806  339617  Bra1000003      157     -       339617  340806  255,0,0 5       95,127,68,50,432        0,190,467,619,757
A01     888838  889347  888838  Bra1000004      92      -       888838  889347  255,0,0 1       509     0
A01     1165488 1165761 1165488 Bra1000005      228     -       1165488 1165761 255,0,0 1       273     0
A01     1167009 1167386 1167009 Bra1000006      238     -       1167009 1167386 255,0,0 1       377     0
A01     1234347 1234827 1234347 Bra1000007      85      +       1234347 1234827 255,0,0 1       480     0

It would be a whole lot better this way:

import sys

def main(filename_in, filename_out):
    with open(filename_in) as fh_in:
        with open(filename_out, 'w') as fh_out:
            count = 1
            for line in fh_in:
                parts = line.strip().split()
                if count < 10:
                    bra = "Bra100000" + str(count)
                else:
                    bra = "Bra10000" + str(count)
                count += 1
                test = "{:10s}" .format(bra)
                fh_out.write("\t".join(parts[0:3]))
                fh_out.write("\t")
                fh_out.write(parts[1])
                fh_out.write("\t")
                fh_out.write(test)
                fh_out.write("\t")
                fh_out.write("\t".join(parts[4:12]))
                fh_out.write("\n")

if __name__ == '__main__':
    main(sys.argv[1], sys.argv[2])

Improvements:

Use with open(...) as ...:. This way the file handles will be correctly closed when exiting the block
Drop the unused variable final
Also drop the pointless variable dict
Since both branches of the if-else increment count, you can move it outside
Instead of count = count + 1, it's simpler as count += 1
Instead of the repetitive write, simplify with join
Do not just dump the code in the global namespace: put it inside a function. This way the command line arguments will have meaningful names
Instead of stripping and splitting line in 2 steps, do it in one: line.strip().split()
It's not a good practice to overwrite loop variables and function parameters. line is a good name for a line of input, splitting that becomes something else and "line" is no longer an appropriate name, so I called it "parts" instead


Naive primality test and summation I am starting to learn Haskell, so I coded a naive solution to the primality test and summation of primes (Project Euler Problem 10: sum of all primes under 2 million) . As I have an imperative programming background, can you tell me if my code is functional enough?

isqrt :: Int -> Int
isqrt n = floor(sqrt(fromIntegral n))
factors n = filter (\i -> n `mod` i == 0) [1..(isqrt(n))]
is_prime n = (factors n) == [1]
main = print (sum (filter is_prime [2..2000000]))

You have some of the right idea, but it looks like you're writing in Lisp. ;-)

There's no need to surround a single parameter in parentheses to pass it to a function, you mostly get this right so I think this one instance was just a slip up.

factors n = filter (\i -> n `mod` i == 0) [1..(isqrt(n))]
--                                                  ^ ^

Function application has the highest precedence of any operator in Haskell, so these parentheses are unnecessary.

factors n = filter (\i -> n `mod` i == 0) [1..(isqrt n)]
--                                            ^       ^

is_prime n = (factors n) == [1]
--           ^         ^

Common Haskell style often prefers function composition to function application.

isqrt n = floor . sqrt . fromIntegral $ n

Are you familiar with function composition from mathematics? It's the same thing here, f . g $ x is equivalent to f (g x), but it puts the emphasis of the code on the composition of functions and not the manipulation of values. This is a different mindset from what you're going to be used to coming from an imperative language, but it is integral to developing the Haskell "house style" and really effectively leveraging combinators and function composition.

Haskellers also love writing functions pointfree. This means leaving off the arguments that are being passed to your function where possible. Check out the Haskell Wiki on pointfree style for some motivation and examples.

isqrt = floor . sqrt . fromIntegral

All top-level definitions should be annotated with a type signature in Haskell. There are myriad reasons for always doing this, including acting as documentation, providing necessary information to the compiler on occasions, serving as a check against your own understanding of what your function is doing, &c.

factors :: Int -> [Int]
is_prime :: Int -> Bool
main :: IO ()

Common Haskell style is to use camelCase instead of snake_case, so isPrime instead of is_prime.

Wherever possible, Haskellers usually prefer case analysis to equality testing. If you rewrite factors to exclude testing 1 (which is a tautological case anyway, why bother evaluating it?) you can check if the list returned by factors is null with null, which enables more function composition goodness.

factors :: Int -> [Int]
factors n = filter (\i -> n `mod` i == 0) [2..isqrt n]

isPrime :: Int -> Bool
isPrime = null . factors

Here's the whole thing with my style adjustments.

isqrt :: Int -> Int
isqrt = floor . sqrt . fromIntegral

factors :: Int -> [Int]
factors n = filter (\i -> n `mod` i == 0) [2..isqrt n]

isPrime :: Int -> Bool
isPrime = null . factors

main :: IO ()
main = print . sum . filter isPrime $ [2..2000000]

Line breaks are important too. Doesn't it look much less cramped?

A comprehensively correct version of isPrime using guards to defend the honor of the negative numbers.

isPrime :: Int -> Bool
isPrime n | n < 2     = False
          | otherwise = null . factors $ n


Implementing division without addition, multiplication, division or subtraction - follow-up This is a follow-up to this question:

Division without using addition, multiplication or division

Could you please tell me if there is better way of performing division without +,-, * or /?

I've tested this code for different values of a and b.  I've also tested working for:

divide(8,3)
divide(5,10)
divide(9,6)
divide(42,6)

Could you please tell me if there is a cleaner solution?

private static int divide(int a , int b){

    boolean isNeg=false;
    boolean sign=true;
    boolean bothNegative=false;
    int quotient=1;

    if(a<0 && b<0){
        bothNegative=true;
    }
    else if(a<0 || b<0) {

        isNeg=true;
        sign=true;

    }

    a=Math.abs(a);
    b=Math.abs(b);

    if(a<b) return 0;
    else{

        int s=add(a,-b);
        int divisor =b;

        while(Math.abs(s)>=Math.abs(divisor)){
            b=add(b,divisor);
            s=add(a,-b);
            quotient=add(quotient,1);
             if(isNeg){
                sign=!sign;
            }
        }

        if(bothNegative){
            return quotient;

        }else if(isNeg && !sign){
            quotient=add(0,-quotient);
         }

        return quotient;
    }

}

private static int add(int a , int b){

    do{

        a^=b;
        b&=(a^b);
        b<<=1;

     }while(Math.abs(b)!=0);

    return a;
}

Seems like the approach you've taken is to implement the + operator yourself. And when using the + operator on negative numbers, you've implemented your own - operator.

I have to congratulate you for being able to implement these operators from scratch. Good job.

A slight bug is that your code returns incorrect result for divide(-42, 6)

I believe the source of this bug is that this code is inside the while:

if (isNeg) {
    sign=!sign;
}

Put it outside the while and it seems to work (at least for my test case)

if (a < b) return 0;
else{

You can save yourself some trouble indentation by removing that else. As you return, you can keep on going without adding indentation. (Which is why I love early returns)

Your spacing is a bit off, the easiest way to fix this is to press Ctrl + Shift + F if you're using Eclipse, or Alt + Shift + F if you're using Netbeans.

For example, this line:

while(Math.abs(s)>=Math.abs(divisor)){

Would look better like this:

while (Math.abs(s) >= Math.abs(divisor)) {

quotient = add(0, -quotient);

As you're using a - sign there already, why not replace all this with -quotient?

You have three boolean values related to the positive/negative issue:

boolean isNeg = false;
boolean sign = true;
boolean bothNegative = false;

Only one would be enough:

boolean swapSign = a < 0 ^ b < 0;

This is using the boolean XOR operator (^), so this will be true if a is negative or b is negative, but not if both are negative.

int quotient = 1;
if (a < b) {
    return 0;
}

You're using a < b as a special case, but that's not necessary.

int s;

That's not a readable name.

You can get rid of several unnecessary variables by directly operating on the a and b values. As they're ints, they're value-typed and therefore copied and not sent by reference, which means that you can modify them however you'd like inside your method. They can be used just like normal local variables.

The main part of your method can be simply this:

a = Math.abs(a);
b = Math.abs(b);

int result = 0;
while (a >= b) {
    a = add(a, -b);
    result = add(result, 1);
}

Your logic for what to return is a bit bloated:

if (bothNegative) {
    return quotient;
}
else if (isNeg && !sign) {
    quotient = add(0, -quotient);
}
return quotient;

This can be replaced with, by using the single swapSign variable:

if (swapSign) {
    return -result;
}
else {
    return result;
}

Or even simpler, using the ternary operator:

return swapSign ? -result : result;

End result:

private static int divide(int a, int b) {
    boolean swapSign = a < 0 ^ b < 0;
    a = Math.abs(a);
    b = Math.abs(b);
    int result = 0;
    while (a >= b) {
        a = add(a, -b);
        result = add(result, 1);
    }
    return swapSign ? -result : result;
}


Format seconds into the two biggest units of time I wrote a function to convert seconds into the two biggest units of time and format it into a string. What I mean is that e.g. 134 second would be 02m14s and 3665 seconds would become 1h1m. Here is what I did:

def pprint_time(seconds):
    m, s = divmod(seconds, 60)
    h, m = divmod(m, 60)
    d, h = divmod(h, 24)
    format_str = lambda v1, v2, p: f"{v1:0>2}{p[0]}{v2:0>2}{p[1]}"
    if d > 0:
        return format_str(d, h, ("d","h"))
    elif h > 0:
        return format_str(h, m, ("h","m"))
    elif m > 0:
        return format_str(m, s, ("m","s"))
    else:
        return f" {s:0>2}sec"

I wonder if there is a more "pythonic" way of doing this since the if-elif-else statement does feel a bit clunky.

In general I think your code is pretty good.  It is a simple function, and it does not need to get fancy.

I do think one bit could be a bit cleaner.  I believe this format string:

format_str = lambda v1, v2, p: f"{v1:0>2}{p[0]}{v2:0>2}{p[1]}"
format_str(d, h, ("d","h"))

would be simpler and therefore clearer as:

format_str = "{:0>2}{}{:0>2}{}"
format_str.format(d, 'd', h, 'h')

I think the lambda here is not helping readability.

More Pythonic?

For this simple case I don't think you really need anything more Pythonic.  But let's have some fun and illustrate using some Python libraries.

datetime library

First thing to consider is maybe using standard Python lib for doing some of the date and time math. The library could build those values for you like so:

import datetime as dt

# build a datetime from the number of seconds passed in
a_time = dt.datetime.utcfromtimestamp(seconds)

# get d, h, m, s
d, h, m, s = dt.timedelta(seconds=seconds).days, a_time.hour, a_time.minute, a_time.second

Stacked ifs

You asked specifically about the stacked ifs. In this case the ifs are not stacked very deep, so what follows is rather severe overkill :-)

Here is a general way to get the elements following the first non-zero element.

# get values fllowing first non zero value and their labels
non_zero = tuple(it.dropwhile(lambda x: not x[0], zip(values, 'dhms')))

Yikes, this is that an eyesore! But is does show how to get the elements following the first non-zero element in a general way, IE: No stacked ifs needed.

How does that work?

Let's break it down a bit.  Starting from the inside:

zip(values, 'dhms')

This will produce a sequence of tuples looking something like:

[(0, 'd'), (1, 'h'), (1, 'm'), (5, 's')]

Note how the use of zip() allows the labels to be associated with their elements.  This allows the next step to keep the labels with their values.

Then:

it.dropwhile(lambda x: not x[0], zip(values, 'dhms')))

uses itertools.dropwhile() to drop the leading tuples which start with 0. Finally:

non_zero = tuple(it.dropwhile(lambda x: not x[0], zip(values, 'dhms')))

will produce a tuple so that we can get the number of elements to help produce the format string:

# a format string
format_str = "{:0>2}{}{:0>2}{}" if len(non_zero) > 1 else " {:0>2}sec"

And then bring it all together with:

# format the string
return format_str.format(*it.chain.from_iterable(non_zero or ((0, 0),)))

itertools.chain.from_iterable() flattens the non_zero tuple, to make the format() a little easier:

Complete recast function

import datetime as dt
import itertools as it

def pprint_time2(seconds):

    # build a datetime from the number of seconds passed in
    a_time = dt.datetime.utcfromtimestamp(seconds)

    # get d, h, m, s
    values = dt.timedelta(seconds=seconds).days, a_time.hour, a_time.minute, a_time.second

    # get values following first non zero value and their labels
    non_zero = tuple(it.dropwhile(lambda x: not x[0], zip(values, 'dhms')))

    # a format string
    format_str = "{:0>2}{}{:0>2}{}" if len(non_zero) > 1 else " {:0>2}sec"

    # format the string
    return format_str.format(*it.chain.from_iterable(non_zero or ((0, 0),)))


DataService that fetches data not loaded already or loads cached values I load an array from an API that is available throughout the user's whole session. This data is unlikely to change, so it can be cached safely. I have a DataService that works like this:

get data from the class property (scenario: going to a new route)
if nothing is there, get data from local storage (scenario: reload)
if nothing is there, fetch data from API (scenario: new session)

This is what I've come up with:

import { Injectable } from '@angular/core';
import { HttpClient } from '@angular/common/http';
import { Observable, of } from 'rxjs';
import { map, catchError, retry } from 'rxjs/operators';

@Injectable({
  providedIn: 'root',
})

export class DataService {
  protected data: Array<any> = null;

  constructor(private http: HttpClient) { }

  public getData(): Observable<Array<any>> {
    if (this.data === null) {
      this.data = JSON.parse(localStorage.getItem('data'));
  }

  if (this.datas !== null) {
    return of(this.data);
  }

  return this.http.get<Object>(this.url + this.route).pipe(
    retry(3),
    map(response => {
      this.data = response as Array<any>;
      localStorage.setItem('data', JSON.stringify(this.data));
      return this.data;
    }),
    catchError(error => {
      this.handleError(error);
      return [];
    })
  );
}

The service is registered in the app.component.ts and used like this in other components:

import { Component, OnInit } from '@angular/core';
import { DataService } from 'src/app/service/data.service';
import { Subscription } from 'rxjs';

@Component({
  selector: 'app-example',
  templateUrl: './example.component.html',
  styleUrls: ['./example.component.less'],
  providers: []
})

export class ExampleComponent implements OnInit {
  protected subscribers: Subscription[] = [];

  public data: Array<any> = [];

  constructor(public data: DataService) { }

  ngOnInit() {
    this.subscribers.push(
      this.data.getData().subscribe((data) => { this.data = data })
    );
  }

  ngOnDestroy() {
    for (let subscriber of this.subscribers) {
      subscriber.unsubscribe();
    }
  }
}

Is it a good approach? Is this "the Angular way"? Can it be improved? Is it readable? Does this indeed increase performance? At least it uses fewer HTTP requests.

This class is shortened a bit for the review, so don't worry that there's no handleError method.

Short review

  if (this.datas !== null) {
    return of(this.data);
  }

That looks like a bug, should it not be (this.data !== null) ?

Also, what is of, where is it defined?

Other than, the code is very clean in both parts. So clean, that I almost would switch the of and the localStorage call. But since this would dupe the null check, that might in the end not be the best idea.

  if (this.data !== null) {
    return of(this.data);
  } else {
      this.data = JSON.parse(localStorage.getItem('data'));
      if (this.data !== null) {
        return of(this.data);
      }
  }