Custom Authentication Attribute I was trying to find a way to redirect to different pages on authorization and authentication failure. I found this to be a possible solution.

However, I ended with a different solution by myself. It seems to work fine, however, I am not sure if it is the right thing to do.

I created a custom Authorize Attribute that redirects to an action if the request is Authenticated but not Authorized.

public class HandleAuthorizeAttribute : AuthorizeAttribute {
        public static string GlobalUnAuthorizationUrl { get; set; }

        private const string DefaultUnAuthorizationUrl = "~/Account/UnAuthorized";
        private static readonly char[] RolesSeparator = { ',' };

        public string UnAuthorizedUrl { get; set; }

        protected override bool AuthorizeCore(HttpContextBase httpContext) {
            if(httpContext.User.Identity.IsAuthenticated) {
                if(string.IsNullOrEmpty(Roles)) {
                    return true;
                } else {
                    var rolesOfUser = System.Web.Security.Roles.GetRolesForUser(httpContext.User.Identity.Name);
                    var authorizedRoles = Roles.Split(RolesSeparator);

                    var common = rolesOfUser.Intersect(authenticatedRoles);

                    if(common.Count() == 0) {
                        httpContext.Response.Redirect(
                            string.Format("{0}?{1}={2}", ActiveUnAuthorizedUrl, "requestUrl", httpContext.Request.Url.AbsoluteUri));
                        return false;
                    }

                    return true;
                }
            } else {
                return false;
            }
        }

        private string ActiveUnAuthorizedUrl {
            get {
                if(!string.IsNullOrEmpty(UnAuthorizedUrl)) {
                    return UnAuthorizedUrl;
                }
                if(!string.IsNullOrEmpty(GlobalUnAuthorizationUrl)) {
                    return GlobalUnAuthorizationUrl;
                }
                return DefaultUnAuthorizationUrl;
            }
        }
    }

Is it alright to redirect to a URL in the middle of a non-action method? Does it have any potential drawbacks?

There is something wrong with this Method that makes it a bit confusing.

    protected override bool AuthorizeCore(HttpContextBase httpContext) {
        if(httpContext.User.Identity.IsAuthenticated) {
            if(string.IsNullOrEmpty(Roles)) {
                return true;
            } else {
                var rolesOfUser = System.Web.Security.Roles.GetRolesForUser(httpContext.User.Identity.Name);
                var authorizedRoles = Roles.Split(RolesSeparator);

                var common = rolesOfUser.Intersect(authenticatedRoles);

                if(common.Count() == 0) {
                    httpContext.Response.Redirect(
                        string.Format("{0}?{1}={2}", ActiveUnAuthorizedUrl, "requestUrl", httpContext.Request.Url.AbsoluteUri));
                    return false;
                }

                return true;
            }
        } else {
            return false;
        }
    }

first thing that I noticed is that the authorizedRoles variable isn't being used, then I saw that you use a variable that isn't declared or initialized anywhere in your code, authenticatedRoles, I am assuming that these are supposed to be the same variable when I removed the common variable like this

    protected override bool AuthorizeCore(HttpContextBase httpContext) {
        if(httpContext.User.Identity.IsAuthenticated) {
            if(string.IsNullOrEmpty(Roles)) {
                return true;
            } else {
                var rolesOfUser = System.Web.Security.Roles.GetRolesForUser(httpContext.User.Identity.Name);
                var authorizedRoles = Roles.Split(RolesSeparator);

                if((rolesOfUser.Intersect(authorizedRoles)).Count == 0) {
                    httpContext.Response.Redirect(
                        string.Format("{0}?{1}={2}", ActiveUnAuthorizedUrl, "requestUrl", httpContext.Request.Url.AbsoluteUri));
                    return false;
                }
                return true;
            }
        } else {
            return false;
        }
    }

I removed some of the else statements that were making this code a little cluttered as well, I was tempted to remove some of these return statements and replace it with a single bool and then just return at the end of the method, but I think that would have made this code messier and harder to read, so this is what I came up with, and it should do the same thing as your code assuming that those two variables I mentioned earlier are the same variable.

protected override bool AuthorizeCore(HttpContextBase httpContext) {
    if(httpContext.User.Identity.IsAuthenticated) {
        if(string.IsNullOrEmpty(Roles)) {
            return true;
        }
        var rolesOfUser = System.Web.Security.Roles.GetRolesForUser(httpContext.User.Identity.Name);
        var authorizedRoles = Roles.Split(RolesSeparator);

        if((rolesOfUser.Intersect(authorizedRoles)).Count == 0) {
            httpContext.Response.Redirect(
                string.Format("{0}?{1}={2}", ActiveUnAuthorizedUrl, "requestUrl", httpContext.Request.Url.AbsoluteUri));
            return false;
        }
        return true;
    }
    return false;
}


Calculating delta in SpriteKit using Swift I am attempting to learn swift by refactoring one of my old games and I need to rewrite my update method which calculates a delta time. This code works but is ugly. How would I go about properly rewriting this?

import SpriteKit

class GameScene: SKScene {
    var lastUpdateTimeInterval: CFTimeInterval?

    override func update(currentTime: CFTimeInterval) {

        var delta: CFTimeInterval?
        if let luti = lastUpdateTimeInterval {
            delta = currentTime - luti
        } else {
            delta = currentTime
        }

        lastUpdateTimeInterval = currentTime;

        if (delta > 1.0) {
            delta = minTimeInterval;
            lastUpdateTimeInterval = currentTime;
        }

        updateWithTimeSinceLastUpdate(delta!)
    }
}

You are inconsistent with the optional semicolons.  It's optional in Swift, but all the example code I've seen from Apple excludes them.  They may be excluding them simply to drive home the point that they're not necessary, but it will probably end up being standard to exclude them.  Either way, you've included them in 3 lines and excluded them in 5.  Consistency is actually more important then what "best practice" says to do, and your code isn't consistent in this regard.
In Swift, the parenthesis around if statements are optional.  Like the semicolons, you've been inconsistent in this.  You include the parenthesis once and omit them once.  Again, all the example code I've seen from Apple omits them and that will probably become the Swift standard, but again, what's most important is consistency.
Perhaps in an effort to force some Swift into your code, you've needlessly declared delta as an optional.  There's no possible path through this method which results in delta being uninitialized, and that becomes pretty evident when we force unwrap it as we pass it to an argument expecting a non-optional.
updateWithTimeSinceLastUpdate() is probably copied over from a method that looks like this in ObjC: updateWithTimeSinceLastUpdate:, which might be okay.  It's a method called update that takes a variable which describes the timeSinceLastUpdate, right?  In Swift, the method might be a bit Swift-ier if timeSinceLastUpdate is actually the name of the argument: update(timeSinceLastUpdate:delta).  This makes it more clear.  The part outside the parenthesis describes what the method actually does, while the part inside the method describes what sort of arguments to send.


creating a concreate implemetation inside a method I have this code:

public bool IsWorkAddedWIthConcreteImplementation()
{
    SomeAgentClass agentClass = new SomeAgentClass();
    bool param1 = true;
    string param2 = "Hello";
    DataTable dt1 = agentClass.ProvideData(param1, param2);
    SimpleDAl da = new SimpleDAl();
    DataTable dt = da.ProvideData(dt1);
    bool isDataSentSuccessfully = agentClass.SendDataToClient(dt);
    IUnImplementedInterface dalInterface = new SimpleDAl();
    if (dalInterface.ReturnMeTrue(dt1))
    {
        return true;
    }
    else
    {
        return false;
    }

}

And the implementation

 IUnImplementedInterface dalInterface = new SimpleDAl();

Should I have passed the interface as a parameter to method or in the class constructor?
What will happen to the method signature or class constructor signature if more interfaces are introduced ?

Update:

In the actual implementation new SimpleDal() class is replaced by DaFactory.GetUnImplementedInterface(), which is returning me the concrete instance of IUnImplementedInterface.

Now the DaFactory is using Activator.CreateInstance() in order to give me correct instantiation.

Update2

One thing I forgot to tell was, that I am passing the DaFactory instance in the constructor of this class, and later using this factory instance to create concrete IUnImplementedInterface instance.

Update3 and Solution

Finally my code structure will now be having a IUnImplementedInterface instance in the class rather than the Factory instance.So now the caller of this class will have to pass the correct concrete implementation of this interface in its constructor.

This article is a great overview of Inversion of Control (IoC), and why it makes code easier to work with. But here's an example in the context of the code you posted. Imagine you are working on the SimpleDal class and decide that you have some configuration setting that you need to pass in. So now your constructor looks like this.

public class SimpleDal:IUnImplementedInterface
{
    public SimpleDal(string configSetting)
    {
        ...do something with it
    }

    ...

    public bool ReturnMeTrue(DataTable dt1){...}
}

This is not an issue in itself, except for now your line

IUnImplementedInterface dalInterface = new SimpleDAl();

needs to have that string in the constructor call (or you could add another constructor that called it with a default value). The point here is that in the end all your calling code really cares about is the IUnImplementedInterface.ReturnMeTrue() method. You "should" be able to replace SimpleDal with any other class that implements that interface (polymorphism). So, by deferring the creation of the IUnImplementedInterface instance to a level above the calling code or by using a factory method, you separate the concern of creating a dependency and using that dependency. The two most popular methods of achieving that are Dependency Injection, or Factory methods.


Sequence processing problems Problem 1:

  Sum the even numbers for the first n fibonacci numbers

Solution:

def fib(k):
    """Compute kth fibonacci number.

    >>> fib(1)
    0
    >>> fib(2)
    1
    >>> fib(11)
    55
    """
    def recursive_fib(prev, curr, k):
        if k - 1 == 0:
             return curr
        else:
             return recursive_fib(curr, prev + curr, k - 1)
    return recursive_fib(1, 0, k)

def iseven(n):
    if n % 2 == 0:
       return True

def sum_even_fibs(n):
    """Sum the first even fibonacci numbers

    >>> sum_even_fibs(11)
    44
    """
    return  sum(filter(iseven, map(fib, range(1, n + 1))))

def sum_even_fibs_gen(n):
    """Sum the first even fibonacci numbers

    >>> sum_even_fibs_gen(11)
    44
    """
    return sum(fib(k) for k in range(1, n+1) if fib(k) % 2 == 0)

Problem 2:

  List the letters in the acronym for a name, which includes the first letter of each capitalized word.

Solution:

def first(s):
    return s[0]

def iscap(s):
    return len(s) > 0 and s[0].isupper()

def acronym(name):
    """Return a tuple of the letters that form the acronym for name.

    >>> acronym('University of California Berkeley')
    ('U', 'C', 'B')
    """
    return tuple(map(first, filter(iscap,name.split())))

def acronym_gen(name):
    """Return a tuple of the letters that form the acronym for name.

    >>> acronym_gen('University of California Berkeley')
    ('U', 'C', 'B')
    """
    return tuple(w[0] for w in name.split() if iscap(w))

Functions fib, acronym and sum_even_fibs are written in functional paradigm and tested.

Functions acronym_gen and sum_even_fibs_gen are written in imperative paradigm and tested.

My question:

After using predefined functions split filter map in above 5 user defined functions, are the aforementioned paradigms being implemented correctly? If no, please let me know of the improvements.

Your fib function would benefit strongly from memoization, as currently you don't reuse the values you've already calculated to save on computations, resulting in O(n^2) computation time, as each fib(k) takes O(n) on average and you do O(n) calls.

By storing the results of previous calls, you can bring total complexity down to O(n) as each consecutive call will take O(1) and you do O(n) calls.

An easy way to do this with Python 3 is to use functools lru_cache decorator as so:

from functools import lru_cache

@lru_cache(maxsize=None)
def function_to_memoize(n):

As a side note, there is a fun fact about fibonacci numbers that every 3rd one is even and the rest are odd, so you can use that to avoid checking parity. See my answer for more details.

As for your other answer, while you seem to have the correct code, I just would like to present an alternative solution for the sake of completeness which first extracts the first letter of every word and then filters by capitalization. I also made use of lambdas for such simple functions.

def acronym(name):
    return tuple(filter(lambda x: x.isupper(), map(lambda x: x[0], name.split())))

I would warrant that your code runs faster though as it does a map on a filtered result rather than a filter on a mapped result, as filter usually reduces the search space while map does not


Draw a directory tree My goal was to create a simple-to-use CLI program for drawing directory trees (what tree does on most platforms, basically). I'm submitting my 'backend' program for review because I think it makes more sense to review the actual algorithm, rather than reviewing an interface on top of it.

import os
import re

def tree(path, indentation=4, ignore_hidden=True, regex=None):
    """Build a directory tree starting at `path`. See also
    https://en.wikipedia.org/wiki/File:Files11_directory_hierarchy.svg

    Arguments
    * path: A relative or absolute path to use as starting point
            for the tree. [type: str]

    * indentation: The amount of spaces to indent each level with.
                   Defaults to 4. [type: int]

    * ignore_hidden: Ignore hidden files (starting with a period).
                     Defaults to True. [type: boolean]

    * regex: If set, only matching files will be shown.
             Set to None to show all files. Defaults to None.
             [type: str / None]

    Returns
    * A directory tree [type: str]

    Example
    Assuming we're in the 'home' directory:

        >>> import tree
        >>> t = tree.tree(path="./")
        >>> t

        ./images
            foo.jpg
            bar.jpg
            ./images/personal
                spam.png
        ./documents
            eggs.docx

        >>> t = tree.tree(path="./", ignore_hidden=False)
        >>> t

        .hidden.txt
        ./images
            foo.jpg
            bar.jpg
            .hidden.jpg
            ./images/personal
                spam.png
        ./documents
            eggs.docx

        >>> t = tree.tree("./", 8, regex="(.*\.png)")
        >>> t

        ./images
                ./images/personal
                        spam.png
        ./documents
    """
    structure = "\n"
    tab = " "*indentation
    if regex is None:
        for root, directories, files in os.walk(path):
            try:
                depth = root.count(os.sep)
                offset = tab * depth
                structure += offset + root + "\n"
            except OSError:
                continue
            for f in files:
                if ignore_hidden and f.startswith("."):
                    continue
                try:
                    depth = os.path.join(root, f).count(os.sep)
                    offset = tab * depth
                    structure += offset + f + "\n"
                except OSError:
                    continue
    else:
        restriction = re.compile(regex)
        for root, directories, files in os.walk(path):
            try:
                depth = root.count(os.sep)
                offset = tab * depth
                structure += offset + root + "\n"
            except OSError:
                continue
            for f in files:
                if ignore_hidden and f.startswith("."):
                    continue
                if not re.match(restriction, f):
                    continue
                try:
                    depth = os.path.join(root, f).count(os.sep)
                    offset = tab * depth
                    structure += offset + f + "\n"
                except OSError:
                    continue
    structure = structure.split("\n")
    dedented = list(map(lambda e: e.replace(tab, "", 1), structure))[1:]
    return "\n".join(dedented)

I believe my code runs in \$O(n)\$ complexity and running it on my Android phone on / takes about 9 seconds:

time python -c "import tree; tree.tree('/')"

real    0m9.365s
user    0m7.390s
sys     0m1.630s

I'd appreciate any feedback regarding performance, usability, coding style, documentation, or anything else!

According to whether regex is None, you run code segment A or B. This feels like a copy-n-paste "bolt-on" to add the regex feature.

I suggest that you may be able to refactor A & B into a common piece of code that pays attention to regex and Does The Right Thing.

You didn't publish any automated unit tests that exercise this code. Putting together some unit tests would help you to refactor while verifying that functionality is unchanged.

You have some KPIs, and you mentioned 9s wall clock time, but did not mention how many directory entries were parsed in those 9 seconds. It would be interesting to know what sort of inputs this code expects, and where a profiler says the code spends most of its time.


Car Showroom Database: Taking user inputs and manipulating arrays I've been learning C# for around 2 weeks and I'm feeling a bit more confident. I wrote this for general practice and am quite proud but hoping to make it faster and more efficient as I progress and learn more.

This program allows the user to input car data i.e. "Model and Price" and then search for the stored data.
(Next step is to add more values to the arrays).

class Program
{
    static void Main(string[] args)
    {

        Console.WriteLine("How many cars are in the workshop?");
        int amountofcars = int.Parse(Console.ReadLine());

        int[] cars = new int[amountofcars];
        double[] carprice = new double[amountofcars];
        string[] models = new string[amountofcars];

        for(int i = 0; i < amountofcars; i++)
        {
            Console.WriteLine("List the model of car #{0}", i + 1);
            string carmodels = Console.ReadLine();
            models[i] = carmodels;
            Console.WriteLine("Input car price");
            double price = double.Parse(Console.ReadLine());
            carprice[i] = price;

        }

            Console.WriteLine("Data stored");

            Console.WriteLine();
            Console.WriteLine("What do you want to do next?");
            Console.WriteLine("Type \"Search\" to start a new search or any key to leave program");
            string responce = Console.ReadLine();
            char searchagain = 'Y';

        do
        {
            if (responce == "Search")
            {
                bool contains = false;
                Console.WriteLine("What car did you want to search for?");
                string modelsearch = Console.ReadLine();

                for (int i = 0; i < amountofcars; i++)
                {
                    if (modelsearch == models[i])
                    {
                        contains = true;
                        Console.WriteLine("{0} {1} ", models[i], carprice[i]);
                        Console.WriteLine();
                        Console.WriteLine("What do you want to do next?");
                        Console.WriteLine("Search again? \"Y\" or any key to exit");
                        searchagain = char.Parse(Console.ReadLine());

                    }
                    if (modelsearch == "All")
                    {
                        for (i = 0; i < amountofcars; i++)
                        {
                            contains = true;
                            Console.WriteLine("{0} {1} ", models[i] , carprice[i]);
                            Console.WriteLine();
                        }
                        Console.WriteLine("What do you want to do next?");
                        Console.WriteLine("Search again? \"Y\" or any key to exit");
                        searchagain = char.Parse(Console.ReadLine());
                    }

                }

            }

        } while (searchagain == 'Y');
    }
}

first, using Parse for user input it would be a bad practice, as you're parsing the input without any validation. thus, you'll need to use TryParse instead, which will validate the input to the specific type, if it's valid input that would be acceptable by the targeted type, then it would return true and output the parsed value.

Example direct parsing :

// this would pass the value without validation
int amountofcars = int.Parse(Console.ReadLine());

Example of validated parsing :

// this validates the input value, then process :
bool isValid = int.TryParse(Console.ReadLine(), out int amountofcars);
// isValid == true, then it's an interger.

if(isValid) // if it's a valid integer
{
    // process the amountofcars
}

you'll find a lot of System types that already has TryParse like int.TryParse, double.TryParse and so on. You should always use it when parsing user inputs. You can still use Parse but use it whenever you see that you're diffidently sure a hundred percent that the value will always be compatible with the targeted type.

Since you have cars, carprice and models arrays, I assume you need one class model, that would hold car information. something like :

public class Car
{
    public double Price { get; set; }

    public string Model { get; set; }
}

now you can initiate a car array :

Car[] cars = new Car[amountofcars];

then inside your loop you can simply do this :

for(int i = 0; i < amountofcars; i++)
{
    Console.WriteLine("List the model of car #{0}", i + 1);
    string carmodels = Console.ReadLine();
    cars[i].Model = carmodels;
    Console.WriteLine("Input car price");
    double price = double.Parse(Console.ReadLine()); // use TryParse instead;
    cars[i].Price = price;
}

doing this, would let you have and process one array which  hold the car information in each element.

I suggest you use List instead of array. It would make things easier for you to process like adding and removing and also you don't need to specify its length. Also, it has more functionality than a system array.

Here is a modified version with some comments that would help you throughout the code, if you feel you need a clarification just let me know.

public class Car
{
    public double Price { get; set; }

    public string Model { get; set; }
}

public static class Program
{
    public static void Main(string[] args)
    {
        List<Car> cars = new List<Car>();

        int amountofcars = 0;

        do
        {
            // this should forced the user to input a valid integer
            // it won't break this loop until it gets a valid integer.
            Console.WriteLine("How many cars are in the workshop?");
        }
        while (int.TryParse(Console.ReadLine(), out amountofcars) == false);

        // now we can process
        for (int i = 0; i < amountofcars; i++)
        {

            Console.WriteLine("List the model of car #{0}", i + 1);
            // since it's string, there is no need for type validation, unless you need to include business validation.
            string carmodels = Console.ReadLine();

            // repeat the same validation :
            double price = 0;
            do
            {
                // this should forced the user to input a valid integer
                // it won't break this loop until it gets a valid integer.
                Console.WriteLine("Input car price");
            }
            while (double.TryParse(Console.ReadLine(), out price) == false);

            cars.Add(new Car { Model = carmodels, Price = price });
        }

        Console.WriteLine("Data stored");
        Console.WriteLine();
        Console.WriteLine("What do you want to do next?");
        Console.WriteLine("Type \"Search\" to start a new search or any key to leave program");
        string responce = Console.ReadLine();

        while (true)
        {
            //using StringComparison.InvariantCultureIgnoreCase
            // this would make it case-insensitive
            if (responce.Equals("Search", StringComparison.InvariantCultureIgnoreCase))
            {
                Car found = null;

                Console.WriteLine("What car did you want to search for?");

                string modelsearch = Console.ReadLine();

                foreach (var car in cars)
                {
                    if (car.Model.Equals(modelsearch, StringComparison.InvariantCultureIgnoreCase))
                    {
                        found = car;
                    }
                }

                if (found != null)
                {
                    Console.WriteLine($"{found.Model} {found.Price}");
                    Console.WriteLine();
                }
                else
                {
                    Console.WriteLine($"Model {modelsearch} not found.");
                    Console.WriteLine();
                }

                Console.WriteLine("What do you want to do next?");
                Console.WriteLine("Search again? \"Y\" or any key to exit");

                // this line would break if it's not a char.
                //searchagain = char.Parse(Console.ReadLine());

                var whatnext = Console.ReadLine();

                if (whatnext.Equals("Y", StringComparison.InvariantCultureIgnoreCase) == false)
                {
                    // if user input anything else will break this loop.
                    break;
                }
            }
            else
            {
                // in case if pressed any key, it'll break this loop.
                break;
            }

        }

    }
}


Saturated arithmetic For project Euler, I need quite often to test things like is x*y*z < 1e18. Since BigInteger is usually too slow, long overflows, and double may return incorrect result due to rounding, I wanted to use long with appropriate checks. As Guava's LongMath.checkedMultiply throws, it can't be used as exception handling is too slow, too. So I decided to write a saturated arithmetic class.

As usual, feel free to ignore my slightly deviating coding conventions. The code has been optimized, so it's a bit hacky. There should be no bugs as it's thoroughly tested. Simplifications and optimizations are most welcome.

@UtilityClass public class SaturatedMath {
    public static long pow(long base, int exp) {
        if (exp==0) return 1;
        checkArgument(exp>=0);

        // Handle small bases and also Long.MIN_VALUE
        if (Math.abs(base) <= 2) return powSpecial(base, exp);

        switch (exp) {
            case 1: return base;
            case 2: return square(base);
            case 3: return cube(base);
        }

        // Now base is at least 3, so the exponent can't be big.
        if (exp >= FLOOR_ROOTS_MAX_LONG.length) return saturate(base < 0 && (exp&1) != 0);

        final long limit = FLOOR_ROOTS_MAX_LONG[exp];
        if (base > +limit) return Long.MAX_VALUE;
        if (base < -limit) return saturate((exp&1) != 0);

        // No more overflow.
        return powUnsaturated(base, exp);
    }

    private static long powSpecial(long base, int exp) {
        assert -2 <= base && base <= 2 || base == Long.MIN_VALUE;
        final long sign = 1 - ((exp&1) << 1);

        switch ((int) base) {
            case -2: return exp>=63 ? saturate(sign) : sign << exp;
            case -1: return sign;
            case 0: return base<0 ? saturate(sign) : 0;
            case 1: return base;
            case 2: return exp>=63 ? Long.MAX_VALUE : 1L << exp;
        }

        throw new RuntimeException("impossible");
    }

    private static long powUnsaturated(long base, int exp) {
        long result = 1;
        for (long x=base; exp>0; exp>>=1) {
            if ((exp&1) != 0) result *= x;
            x *= x;
        }
        return result;
    }

    public static long mul(long x, long y) {
        final long result = x * y;
        // See https://goo.gl/ZMEZEa
        final int nlz = Long.numberOfLeadingZeros(x) + Long.numberOfLeadingZeros(~x)
                + Long.numberOfLeadingZeros(y) + Long.numberOfLeadingZeros(~y);
        if (nlz > 65) return result;
        if (nlz < 64) return saturate(x^y);
        if (x==Long.MIN_VALUE & y<0) return Long.MAX_VALUE;
        if (y != 0 && result / y != x) return saturate(x^y);
        return result;
    }

    public static long square(long x) {
        if (x > +FLOOR_SQRT_MAX_LONG) return Long.MAX_VALUE;
        if (x < -FLOOR_SQRT_MAX_LONG) return Long.MAX_VALUE;
        return x * x;
    }

    public static long cube(long x) {
        if (x > +FLOOR_CBRT_MAX_LONG) return Long.MAX_VALUE;
        if (x < -FLOOR_CBRT_MAX_LONG) return Long.MIN_VALUE;
        return x * x * x;
    }

    private static long saturate(long sign) {
        return saturate(sign < 0);
    }

    private static long saturate(boolean negative) {
        return negative ? Long.MIN_VALUE : Long.MAX_VALUE;
    }

    // (long) Math.sqrt(Long.MAX_VALUE)
    private static final long FLOOR_SQRT_MAX_LONG = 3037000499L;
    // (long) Math.cbrt(Long.MAX_VALUE)
    private static final long FLOOR_CBRT_MAX_LONG = (1L << 21) - 1;

    // For n>=3, the n-th item is the n-th root of Long.MAX_VALUE.
    // The first three values are unused.
    private static final int[] FLOOR_ROOTS_MAX_LONG = {
        0, 0, 0, 2097151, 55108, 6208, 1448, 511,
        234, 127, 78, 52, 38, 28, 22, 18,
        15, 13, 11, 9, 8, 7, 7, 6,
        6, 5, 5, 5, 4, 4, 4, 4,
        3, 3, 3, 3, 3, 3, 3, 3,
        // The next value would be 2, so we stop here.
    };
}

It looks good to me, except for a couple of things:

private static long saturate(boolean negative) {
    return negative ? Long.MIN_VALUE : Long.MAX_VALUE;
}

Is there a particular reason you did negative instead of positive? It feels like an arbitrary choice to me. If it's not, add a brief comment explaining why. If it is, the convention (well, that I've seen, which means that it might well be the opposite) is to have true mean positive, not negative.

private static long powSpecial(long base, int exp) {

How is it special? Explain in the method name or a Javadoc comment (/** */). Here, it seems like it's for small numbers, so a better method name would be powSmallBases. It may only be used here, but it's rather convenient to be able to read your own code later, if you want to change it later.

Anyway, aside from that (and the things you mention in your coding style page thingamabobber) it looks good! Nicely done.


PHP Login Authentication with BCrypt I have a php authentication script and everything works fine, but I'm very unsure about the way I programmed it (I hardcoded some things). I was hoping stack could look through this and point out any potential problems. Here's the script:

<?php
require_once 'Bcrypt.php';
class Mysql {
    private $conn;

    function __construct() {
        $this->conn =  new PDO('mysql:host=***;dbname=***;charset=UTF-8','***','***') or
                      die('There was a problem connecting to the database.');
    }

    function verify_Username_and_Pass($un, $pwd) {
        ini_set('display_errors', 'On');
        error_reporting(E_ALL | E_STRICT);
        $query = "SELECT *
                FROM Conference
                WHERE Username = :un";

        $stmt = $this->conn->prepare($query);

        $stmt->bindParam(':un', $un);
        //$stmt->bindParam(':pwd', $pwd);
        $stmt->execute();
        $row = $stmt->fetchAll();
        $hash = $row[0]["Password"];
        $is_correct = Bcrypt::check($pwd, $hash);

        if ($is_correct) {
            // User exist
            $firstName = $row[0]["First Name"];
            $_SESSION["FirstName"] = $firstName;
            return true;
            $stmt->close();
        }
        else {
            // User doesn't exist
            return false;
            $stmt->close();
        }
    }
}
?>

So how does it look?

I've tried cleaning it up a bit here, removed instances of setting a variable once and using it directly below where it was set (yes, improves readability, but still).

require_once 'Bcrypt.php';

class Mysql {
    private $conn;
    private $host    = 'host';
    private $dbName  = 'dbname';
    private $charset = 'UTF-8';
    private $checkUserQuery = 'SELECT * FROM Conference WHERE Username = :un';

    function __construct() {
        $this->conn =  new PDO('mysql:host=' . $this->host . ';dbname=' . $this->dbname . ';charset=' . $this->charset)
                       or die('There was a problem connecting to the database.');
    }

    function verifyUsernameAndPass($un, $pwd) {
        $stmt = $this->conn->prepare($this->checkUserQuery);

        $stmt->bindParam(':un', $un);
        #$stmt->bindParam(':pwd', $pwd);
        $stmt->execute();
        $row = $stmt->fetchAll();
        $stmt->close(); # Moved up, so closes before we return/exit function

        # You're setting $hash here, and using it directly below.
        # See new code below
        //$hash = $row[0]['Password'];
        //$is_correct = Bcrypt::check($pwd, $hash);

        //if ($is_correct) {
        if( Bcrypt::check($pwd, $row[0]['Password']) ){
            # User exist

            # You're setting $firstName once, and using it directly below it.
            # See new code below
            //$firstName = $row[0]['First Name'];
            //$_SESSION['FirstName'] = $firstName;

            $_SESSION['FirstName'] = $row[0]['First Name'];

            return true;
        } else {
            # User does not exist
            return false;
        }
    }
}

And here's the same code, without my comments:

require_once 'Bcrypt.php';

class Mysql {
    private $conn;
    private $host    = 'host';
    private $dbName  = 'dbname';
    private $charset = 'UTF-8';
    private $checkUserQuery = 'SELECT * FROM Conference WHERE Username = :un';

    function __construct() {
        $this->conn =  new PDO('mysql:host=' . $this->host . ';dbname=' . $this->dbname . ';charset=' . $this->charset)
                       or die('There was a problem connecting to the database.');
    }

    function verifyUsernameAndPass($un, $pwd) {
        $stmt = $this->conn->prepare($this->$checkUserQuery);

        $stmt->bindParam(':un', $un);
        $stmt->execute();
        $row = $stmt->fetchAll();
        $stmt->close();

        if( Bcrypt::check($pwd, $row[0]['Password']) ){ # User exists
            $_SESSION['FirstName'] = $row[0]['First Name'];
            return true;
        } else { # User does not exist
            return false;
        }
    }
}


Ohms law calculator + whitelist of user. Learning use I've been learning a ton of c++ lately on Codecademy and I'm soon done with the course of c++.

(Still need to learn classes, object-oriented programming and harder projects.)

But I wanted to get a review of my recent program.

And I also wanted to know if its possible to make an Unique ID, that only fits me, kinda like the cicada.

Psst, the program is to solve Ohm's Formular in physics and actively takes input to solve exactly your case.

Leave suggestions on what I could do better in this project + what I should try to program next too.

//Include
#include <iostream>
#include <string>
using namespace std;

 int main(){

    //Declare variables
    double Wolt;
    double Ohm;
    double Amp;
    std::string Formular;
    std::string Username;
    std::string Whitelist;

    //Login sequence:
    std::cout << "Hvad er dit username?: ";
    cin >> Username;

    //Whitelist
    Whitelist = "Fred267s", "Seb4572s", "Popp4593s";

    //Check if username is on whitelist
    if (Username == Whitelist){

        std::cout << "Velkommen til fysik formel udregner til strøm " << Username << " \n";
        std::cout << "Lavet af Frederik Schmidt 1.S \n";
    }

    //If username is NOT on the whitelist
    else if (Username != "Fred267s", "Seb4572s", "Popp4593s" ){

        std::cout << "Sorry, you do not have access! \n";
        return 0;
    }

    //Questions
    std::cout << "Hvad er det du skal udregne? ";
    cin >> Formular;

    //Wolt formular

    if (Formular == "Wolt"){

        //Questions
        std::cout << "Hvad er Ohmen?: ";
        cin >>Ohm;
        std::cout << "Hvad er Amperen?: ";
        cin >> Amp;

        //Formular
        Wolt = Ohm*Amp;

        //Return
        std::cout << "Volten er: " << Wolt << " \n";
    }

    //Amp formular

    else if (Formular == "Amp"){

        //Questions
        std::cout << "Hvad er Volten?: ";
        cin >>Wolt;
        std::cout << "Hvad er Ohmen?: ";
        cin >>Ohm;

        //Formular
        Amp = Wolt/Ohm;

        //Return
        std::cout << "Amperen er: " << Amp << " \n";
    }

    //Ohm formular

    else if (Formular == "Ohm"){

        //Questions
        std::cout << "Hvad er Volten?: ";
        cin >>Wolt;
        std::cout << "Hvad er Amperen?: ";
        cin >>Amp;

        //Formular
        Ohm = Wolt/Amp;

        //Return
        cout << "Ohmen er: " << Ohm << " \n";
    }

    //If the Formular inputtet is NOT on the list.

    else {

        cout << "Error in the formular, please try again!";

    }
}

Don't

using namespace std;

This doesn't do what you want (only first one works):

    //Whitelist
    Whitelist = "Fred267s", "Seb4572s", "Popp4593s";

    //Check if username is on whitelist
    if (Username == Whitelist){

[It's also a VERY dodgy access control mechanism!]

I don't like asking user for a string describing the "Formula". How does the user know they should type "Wolt"? What if they type "wolt" instead? I think it would be more reasonable to show 3 choices:

1 - Wolt    2 - Amp    3 - Ohm

Please Enter (1-3):

And then clean the user input checking if it is a valid integer between 1-3 and then represent the choice internally using a Class enum in your code.

This will open up a range of oppotunities to use functions in your code.

enum class Formula ( wolt = 1, amp = 2, ohm = 3 );

...

Formula prompt_formula() {
  // code to read from std::cin clean input and cast to enum class
}

Then try to find a way to add even more structure to your program rather than one huge if/elseif/else statement. I am not going to give you the code, because that would defeat the purpose, but your main function should be more like this:

int main() {
  explain_usage();
  Formular form = prompt_formular();
  auto [label, answer] = compute(form);
  print_answer(label, answer);
}

I could go on, but that's probably plenty to get on with for now.


Basic terrain generator I've decided to try and start programming using Object-Oriented-Programming, so I built a small terrain generator that could be done using something as simple as this:

import random
for _ in range(1000):
    print random.choice('*#~o'),

Instead, I decided to have a go at OOP and made it like this instead:

# Terrain generator
from random import choice
from time import sleep

# Class containg world attr.
class Attr(object):
    def __init__(self):
        self.TILES = '#*~o'
        self.WORLD_SIZE = 1000
        self.Y_LENGTH = 1000
        self.X_LENGTH = 1000

# Generator class
class Generator(object):
    def __init__(self):
        self.world = []
        self.attr = Attr()

    # Create an empty row
    def create_row(self):
        for _ in range(self.attr.X_LENGTH):
            self.world.append(choice(self.attr.TILES))

# Main generator class
class MainGen(object):
    def __init__(self):
        self.gen = Generator()
        self.attr = Attr()

    # Create the world
    def create_world(self):
        for _ in range(self.attr.WORLD_SIZE):
            self.gen.create_row()

    # Render the world
    def render_world(self):
        for tile in self.gen.world:
            print tile,
            sleep(0.05)

# Main game class
class Game(object):
    def __init__(self):
        self.main_gen = MainGen()

    # Run the functions
    def run(self):
        self.main_gen.create_world()
        self.main_gen.render_world()

# Start the program
if __name__ == "__main__":
    game = Game()
    game.run()

All I'm really looking for are these things:

What OOP mistakes did I make?
What general mistakes did I make?
How can I improve in general?

You've gone from two easy-to-understand lines to six methods across three classes; hardly an efficient use of OOP. There is really no need to have the three classes Attr, Generator and MainGen, and they're so heavily interconnected as to suggest single class.

If you do want to use classes, I think you should have a World class, in which generate (covering Generator and MainGen) is a class method for creating a new world and the attributes (covering Attr) are a mix of class attributes and parameters to the class method. This would look like:

import random

class World(object):
    """The world in which the game takes place.

    Args:
      map (list of list of string): a representation
        of the world, a 2-D structure of single-
        character tiles.

    Attributes:
      map (list of list of string): the map supplied
        in the argument to __init__
      TILES (string): the characters to use for tiles.

    """

    TILES = '#*~o'

    def __init__(self, map):
        self.map = map

    def __str__(self):
        """Display whole map as 2-D grid."""
        return "\n".join("".join(row) for row in self.map)

    def render(self):
        """Generator to provide one tile at a time."""
        for row in self.map:
            for tile in row:
                yield tile

    @classmethod
    def generate(cls, shape):
        """Generate a new, random map.

        Args:
          shape (int or tuple of int): Either a two-tuple
            (x, y) or a single integer to use for both x
            and y defining the shape of the map to create.

        """
        try:
            x, y = shape
        except TypeError:
            x = y = shape
        return cls([[random.choice(cls.TILES)
                     for _ in range(x)]
                    for _ in range(y)])

Then when you play the game:

world = World.generate(100)
for tile in world.render():
    ...

This separates the world itself out from how the game needs it displayed. As a simple example:

>>> world = World.generate(5)
>>> print(str(world))
oo#~#
*oo~#
#o#o~
*o*#o
**#*o


Scraping with asyncio/aiohttp and retrying the request on error I am doing some practicing with back-connect proxies and asyncio/aiohttp. The IP changes on every request to the proxy and I am allowed up to 1500 threads. The big issue with back-connect proxies, is you do not always get a proxy that works. This forces me to keep requesting the URL until there are no errors so I can return the HTML.

I am mainly asking for a code review to see if using a while loop is the best option to keep retrying the request. And how I can speed up the script overall? I have been looking into different HTML parsers, using multiprocessing, etc... but just not sure what direction to go in. The code below does work fine, and it processes 12,000 URLs in about 30 minutes. I really appreciate you looking at my code and let me know if you have any questions! Thank you for your time.

import asyncio
import aiohttp

from bs4 import BeautifulSoup
from datetime import datetime
from aiohttp.resolver import AsyncResolver

class Proxy:
    def __init__(self, headers, proxy):
        self.headers = headers
        self.proxy = proxy

    async def build(self, urls):
        tasks = []
        resolver = AsyncResolver(nameservers=["8.8.8.8", "8.8.4.4"])
        connector = aiohttp.TCPConnector(limit=1500, limit_per_host=0, resolver=resolver, use_dns_cache=False)
        async with aiohttp.ClientSession(connector=connector) as session:
            for url in urls:
                task = self.fetch(session, url)
                tasks.append(task)
            r = await asyncio.gather(*tasks)
            return r

    async def fetch(self, session, url):
        while True:
            try:
                async with session.get(url, headers=self.headers, proxy=self.proxy) as resp:
                    assert resp.status == 200
                    r = await resp.read()
                    soup = BeautifulSoup(r, 'lxml')
                    html_title = soup.title.string
                    print (html_title)
                    return html_title
            except Exception as e:
                print (e)

if __name__ == '__main__':
    headers = {'User-Agent': 'Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/64.0.3282.186 Safari/537.36'}
    proxy = 'XXX.XX.X.XXX:XXXXX'
    proxy = "http://{}".format(proxy)

    px = Proxy(headers, proxy)

    urls= []
    with open('urls.txt') as f:
        for line in f:
            urls.append(line.strip())

    startTime = datetime.now()
    loop = asyncio.new_event_loop()
    asyncio.set_event_loop(loop)
    future = asyncio.ensure_future(px.build(urls))
    html_title = loop.run_until_complete(future)
    loop.close()
    print (datetime.now() - startTime)

with back-connect proxies, is you do not always get a proxy that works.

There are several possibilities here. The origin webserver could be responsive, slow (slower than your timeout), or dead, and the proxy could be responsive, slow, or dead. Call those "health" details. It doesn't look like you are tracking health details of either one, so you don't adapt to such details. For faster processing, use adaptive timeouts and don't re-submit requests to dead hosts (don't contact known-dead proxies, and don't contact known-dead webservers).

EDIT: An example of an adaptive timeout would be triple the observed median response time of a host, which attempts to capture a large fraction of the pdf (probability distribution function) of its observed response times, conditioned on actually getting a response.

class Proxy:
    def __init__(self, headers, proxy):

I'm not crazy about that last identifier. Call it proxy_url, please. It clearly isn't an instance of the Proxy class.


Download, unzip and parse xml into pandas dataframe I have a python script that downloads, unzip and parses an XML file published by a Canadian institution. Only some very specific tags are extracted and then all put into a pandas dataframe for later processing.

Everything works well. I just wonder if there is room for improvement here, specially in the parsing part. I am not sure if the nested for I use are a good idea or there is a better and cleaner way to parse.

import requests
import zipfile
import os
import glob
from lxml import etree
from io import StringIO, BytesIO
import pandas as pd
import xml.etree.ElementTree as ET

def download_file(url,filename):
    r = requests.get(url, allow_redirects=True)
    open(filename, 'wb').write(r.content)

def unzip_and_delete(filename):
    zip_file = zipfile.ZipFile(filename,'r')
    zip_file.extractall()
    zip_file.close()
    os.remove(filename)

def parse_xml_fields(file, base_tag, tag_list,final_list):
    root = etree.parse(file)
    nodes = root.findall("//{}".format(base_tag))
    for node in nodes:
        item = {}
        for tag in tag_list:
            if node.find(".//{}".format(tag)) is not None:
                item[tag] = node.find(".//{}".format(tag)).text.strip()
        final_list.append(item)

# My variables
field_list = ["MsbRegistrationNumber","StatusDescriptionEnglish","Surname","GivenName","MiddleName","Name","StreetAddress"]
entities_list = []

download_file('http://www10.fintrac-canafe.gc.ca/msb-esm/public/msb-search/zipdownload-eng/', 'fintrac.zip')

unzip_and_delete('fintrac.zip')

parse_xml_fields("MsbRegistryPublicDataFile.xml", "MsbInformation", field_list, entities_list)

df = pd.DataFrame(entities_list, columns=field_list)

df.to_excel("Canada_MSB_List.xlsx")

You may import itertools and then replace the part

for node in nodes:
    item = {}
    for tag in tag_list:
        if node.find(".//{}".format(tag)) is not None:
            item[tag] = node.find(".//{}".format(tag)).text.strip()
    final_list.append(item)

with

for node, tag in itertools.product(nodes, tag_list):
    if node.find(".//{}".format(tag)) is not None:
        final_list.append({tag: node.find(".//{}".format(tag)).text.strip()})

as itertools.product() creates a cartesian product of your two lists (and so the temporary dictionary item is no longer needed).


Find all anagrams to a given word from a text file This program I made takes a word as an input and prints all the anagrams of the word in a given word base from a txt file. I would like advice for code style, Java specific practices etc as I'm new to to the Java language (about 2 months).

import java.io.File;
import java.io.FileNotFoundException;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.HashMap;
import java.util.Scanner;

public class Anagrams {
    public static void main(String[] args) throws FileNotFoundException {
        Scanner s = new Scanner(new File("bg.txt"));

        ArrayList<String> words = new ArrayList<String>();
        while (s.hasNext()) {
            words.add(s.next());
        }

        HashMap<String, ArrayList<String>> map = new HashMap<>();

        for (String str : words) {
            char[] temp = str.toCharArray();
            Arrays.sort(temp);
            String key = new String(temp).toLowerCase();
            if (map.get(key) != null) {
                map.get(key).add(str.toLowerCase());
            } else {
                ArrayList<String> anagramList = new ArrayList<>();
                anagramList.add(str);
                map.put(key, anagramList);
            }
        }
        Scanner input = new Scanner(System.in);
        String str = input.next();
        char[] key = str.toCharArray();
        Arrays.sort(key);
        str = new String(key).toLowerCase();
        if (!map.containsKey(str)) {
            System.out.print("word not found");
        } else if (map.get(str).size() != 1) {
            for (String p : map.get(str)) {
                System.out.print(p + " ");
            }
        } else {
            System.out.print("No anagrams found");
        }
        s.close();
        input.close();
    }
}

Use more functions

The current implementation is all in the main method.
Break the problem down to smaller pieces,
so that each function has a single responsibility, for example:

Read words from a text file into a List and return it
Convert a word to "normalized" form (sorted and lowercased)
Convert a list of words to a map of normalized form to list of anagrams
Read input and respond with the list of anagrams

Use interface types instead implementations

Instead of these:

ArrayList<String> words = new ArrayList<String>();
// ...

HashMap<String, ArrayList<String>> map = new HashMap<>();

It's better to use interface types in declarations, like this:

List<String> words = new ArrayList<>();
// ...

Map<String, List<String>> map = new HashMap<>();

And on the right-hand side, use the diamond operator <> when possible.

Use try-with-resources when working with resources

Instead of this:

Scanner s = new Scanner(new File("bg.txt"));

ArrayList<String> words = new ArrayList<String>();
while (s.hasNext()) {
    words.add(s.next());
}

// much much later...
s.close();

Write like this:

List<String> words = new ArrayList<>();
try (Scanner s = new Scanner(new File("bg.txt"))) {
    while (s.hasNext()) {
        words.add(s.next());
    }
}

Notice that s.close() is no longer necessary.
That also means you cannot "forget" to close a resource manually.

You can read more about this here: https://docs.oracle.com/javase/tutorial/essential/exceptions/tryResourceClose.html


Module to validate and sum a string containing no negative numbers To learn the language of JavaScript, I did a dojo exercise by Roy Osherove. Is there a better way to organize this so that the only method exported is add while keeping it in one single module?

module.exports = {
  add: function (string) {
    this._checkForError(string);
    this._checkForNegatives(string);
    return this._result(string);
  },

  _result: function (string) {
    var numbers = this._numbers(string);
    var result = numbers.reduce(function (sum, number) {
      return sum + number;
    });
    return result;
  },

  _numbers: function (string) {
    var modded = this._modded(string);
    var numbers = modded.map(function (number) {
      return parseInt(number, 10) || 0;
    });
    return numbers;
  },

  _checkForError: function (string) {
    if (string.match(/\n$/)) {
      throw 'Expression ending in newline!';
    }
  },

  _negatives: function (string) {
    var numbers = this._numbers (string);
    var negatives = numbers.filter(function (number) {
      return number < 0;
    });
    return negatives;
  },

  _delimiter: function (string) {
    return (string.substr(0, 2) === '//') ? string[2] : ',';
  },

  _checkForNegatives: function (string) {
    var negatives = this._negatives(string);
    if (negatives.length > 0) {
      throw 'Negatives not allowed: ' + negatives.join(', ');
    }
  },

  _modded: function (string) {
    var delimiter = this._delimiter(string);
    return string.replace(/\n/, delimiter)
      .split(delimiter);
  }
};

module.exports is just an object. Set properties on it that you want to expose.

exports.add = add

This is also equivalent in this case.

module.exports = {add: add}

Now, when you require this module, you'll get an object with one property: add.

There are a few other small issues:

Nothing about your design depends on accessing methods via this. This module
is clearer without bringing this into the mix.

function add (string) {
  _checkForError(string)
  _checkForNegatives(string)
  return _result(string)
}

This code is quite easy to follow already, but It would be even better if you
had your methods in the order they are called so the code reads like an article:
broad info at the top, specific details later on.

I'd also leave out semicolons where they will be handled by automatic semicolon
insertion, but that's a style choice.

function _checkForError (string) {
  if (string.match(/\n$/)) {
    throw 'Expression ending in newline!'
  }
}

function _checkForNegatives (string) {
  var negatives = _negatives(string)
  if (negatives.length > 0) {
    throw 'Negatives not allowed: ' + negatives.join(', ')
  }
}

function _negatives (string) {
  var numbers = _numbers(string)
  var negatives = numbers.filter(function (number) {
    return number < 0
  })
  return negatives
}

function _result (string) {
  var numbers = _numbers(string)
  var result = numbers.reduce(function (sum, number) {
    return sum + number
  })
  return result
}

function _numbers (string) {
  var modded = _modded(string)
  var numbers = modded.map(function (number) {
    return parseInt(number, 10) || 0
  })
  return numbers
}

function _modded (string) {
  var delimiter = _delimiter(string)
  return string.replace(/\n/, delimiter).split(delimiter)
}

function _delimiter (string) {
  return (string.substr(0, 2) === '//') ? string[2] : ','
}


Removing list of words from a text file in Ruby I have two files.

File 1. Has a list of all the dictionary words
File 2. Has a list of
all prepositions.

I want to remove all the prepositions from the dictionary.
I want to reduce the number of lines in my code and also make it more elegant, idiomatic and readable.

    #!/usr/bin/env ruby

    path = "/Users/../Desktop/";

    file_original_wordlist = File.open("#{path}"  + "dictionary.txt",  "r")
    file_remove_wordlist = File.open("#{path}"  + "prepositions.txt", "r")

    # Need to initialize the variables else I get errors
    delete_word = false
    word_orig =  ''
    word_rem  =  ''
    count = 0
    file_original_wordlist.each_line do |line1|
      file_remove_wordlist.each_line do |line2|
        word_orig =  line1
        word_rem  =  line2
        if word_orig.eql?(word_rem)
          puts "Deleting the word " + word_rem
          delete_word = true
          count++
        end
      end
      if delete_word == false
        File.open(path + "scrubbed_list.txt", "a") {|f| f.write(word_orig) }
      end
  # Need to reopen the file otherwise after the first iteration to start from the beginning
      file_remove_wordlist = File.open("#{path}"  + "prepositions.txt", "r")
      delete_word = false
    end

    puts "Deleted " + count + " words in total"

Some notes:

I guess you come from imperative languages. Try to write in a more functional style (more expressions, less statements).
Use libraries (File) to manipulate paths.
This double each_line is bad news for performance: O(n*m). Avoid it by building a data structure that has O(1) checks for inclusion. I'd create a set of the prepositions (it's the smaller set). The overall performance is now O(n).

I'd write:

prepositions = open(File.join(path, "prepositions.txt")).lines.to_a
words = open(File.join(path, "dictionary.txt")).lines.to_a
filtered_words = words - prepositions
File.write("dictionary_without_prepositions.txt", filtered_words.join)

If the input file dictionary.txt is very, very large, this is a more lazy aproach:

require 'set'
prepositions = open(File.join(path, "prepositions.txt")).lines.to_set

open("dictionary_without_prepositions.txt", "w") do |output|
  open(File.join(path, "dictionary.txt")).lines.each do |line|
    unless prepositions.include?(line)
      output.write(line)
    end
  end
end


Split a Map into sublists I may get a Map of many many items that needs to be requested to an API.

In order to improve performance, I thought about splitting that list into smaller sublists, and querying the API with this small subset.

For that, I came to this code and was wondering what was your opinion on it.

This is some banal initiator I used to test my code:

Map<String, Integer> map = new LinkedHashMap<String, Integer>();
for (int i = 0; i < 550;i ++) {
    map.put(String.valueOf(i), i);
}

Now the real code:

    int length = 100;
    List<Integer> items = new LinkedList<Integer>(map.values());
    int pages = (int) Math.ceil((double) items.size() / length);

    for (int i = 0; i < pages; i++) {
        List<Integer> sub = items.subList(i * length, ((i+1) * length > items.size() ? items.size() : (i+1) * length));
        System.out.println(sub.get(0));
        System.out.println(sub.get(sub.size() - 1));
        System.out.println("--");

        // In my case, I would call the API here, at that place, using the sublist
    }

The final output is:

0
99
--
100
199
--
200
299
--
300
399
--
400
499
--
500
549
--

This works great. What's your opinion on it?

The implementation is simple enough to require any comments. Just a few things:

Is LinkedList a requirement? Can't it be done by an ArrayList?
Use Math.min while making the subList.

An alternative for simpler solution
You can add the Guava library to your project and use the Lists.partition method, e.g.

List<Integer> items = ...
List<List<Integer>> subLists= Lists.partition(items, pageSize);


Database of students At first, I had Database to contain std::list<Student> students, but because there are over 2000 students, searching by students should not be done in linear time.  However, searching for students can be by name or by student number.  To enable both searches in logarithmic time, I decided to give Database two maps that hold the same 2000+ students.  Inserting and removing a student will have to update both maps.  This doubles the time, but I figured that \$2*O(logN)\$ is still much better than \$O(N)\$.  If there are to be k different search criteria later on, then \$k*O(logN)\$ is still better than \$O(N)\$ up to, say, k = 20 or so.  Agree?

Space complexity is a different matter though.  Is the doubling of memory usage bad design here, for 2000+ students?  Tripling?  Quadrupling?

Is there a better solution altogether?

#include <iostream>
#include <string>
#include <memory>
#include <map>

class Student {
    std::string surname, firstName;
    long studentNumber;
public:
    std::string getSurname() const {return surname;}
    std::string getFirstName() const {return firstName;}
    std::string fullName() const {return firstName + ' ' + surname;}
    long getStudentNumber() const {return studentNumber;}
};

class Database {
    std::multimap<std::string, std::shared_ptr<Student>> mapByName;
    std::map<long, std::shared_ptr<Student>> mapByStudentNumber;
public:
    void insertStudent (const std::shared_ptr<Student>& p) {
        mapByName.emplace (getName(p), p);
        mapByStudentNumber.emplace (p->getStudentNumber(), p);
        // 2 insertions needed.
    }
    inline void removeStudentByStudentNumber();
    inline void removeStudentByName();
private:
    void remove (const std::shared_ptr<Student>& p) {
        mapByName.erase(getName(p));
        mapByStudentNumber.erase(p->getStudentNumber());
        // 2 removals needed.
    }
    std::string getName (const std::shared_ptr<Student>& p) const {return p->fullName();}
};

inline void Database::removeStudentByStudentNumber() {
    long studentNumber;
    std::cout << "Student number? ";
    std::cin >> studentNumber;
    const auto it = mapByStudentNumber.find(studentNumber);
    if (it == mapByStudentNumber.end()) {
        std::cout << "No student with that student number exists.\n\n";
        return;
    }
    remove(it->second);
}

inline void Database::removeStudentByName() {
    // Similar to above
}

int main() {}

This code is not complete so difficult to give a full review, but here are my thoughts.

Student

Put public methods first

This point is debatable, but I generally think it's better to have public methods before private variables. This is the only thing users of your class should care about, so make it easier to find.

Use variable name forename

forename is a better variable name than firstName, and fits better with surname (as apposed to lastName).

Type alias std::string

I think it would be clearer to type alias std::string for the names, if you make this public, the database class can use this alias to make the map definition clearer too:

using NameType = std::string;

Add missing constructors

Not sure if these were left out intentionally, but a simple forwarding constructor here will suffice:

template <typename T1, typename T2>
Student(T1&& forename, T2&& surname, long studentNumber);

Make fullName a non-member

It's generally preferable to make your class interface as minimal as possible, and you really gain nothing here by having this as a member method, so make it a non-member:

Student::NameType getFullName(const Student& student);

Return fields by const &

You are making a copy on every call to getFirstName and getSurname, better to return a const reference - the caller can always make a copy if needed.

Database

Don't make Database responsible for reading user input

Your remove functions should not read anything from the user, it obscures the interface and purpose of the class. If you need this functionality, put it into a non-member method:

long getStudentNumberToRemoveFromUser();

Consider a better design

I'd say your design is flawed; you shouldn't be removing Student objects by name (not even full name), as you could have duplicates. Presumably the student number is unique, so this should be the only identifier used for removal. Instead you could offer a public method to gain access for student numbers. So you just have:

std::vector<Student> getMatchingStudents(Student::NameType surname forename, Student::NameType surname) const;
bool remove(const Student& student);

Where the return bool indicates if the record was removed.

Consider Boost.Bimap

Boost.Bimap already solves the two way access problem you are having.

Remove shared_ptr

Assuming you don't want to use Boost.Bimap.

There's really no need to use shared_ptr here; Database has complete control over the lifetime of the Student objects. Introducing the shared_ptr only adds code complexity, additional memory consumption, and confusion. Instead you can either store everything by value (completely feasible for just 2000 records), or store values in mapByStudentNumber and store a raw pointer, or std::reference_wrapper<const Student::NameType in mapByName.

Use better variable names

There's some debate about how to best name maps (xToY, ys, xToYMap etc), but I don't like mapByName and mapByStudentNumber. I'd probably go with namesToNumbers and numbersToNames.


Split CSV file into a text file per row, with whitespace normalization I have a CSV file that I would like to split up by row, do some whitespace processing on the string data in each row of the document column, and then output the processed data into separate TXT files in a new directory. The whitespace processing is to standardize the documents by removing newlines, carriage returns, and replacing all empty strings with " ".

The thing is, I will be doing this on corpora of 3+ million documents, so was wondering if there is a better way to do this instead of iterating through each one.

with open(csv_file) as file:
    reader = csv.reader(file, delimiter=',')
    count = -1
    for row in reader:
        row[1] = str(row[1]).replace(r'\n', '')
        row[1] = str(row[1]).replace(r'\r', '')
        if not row[1]:
            row[1] = ' '
        with open('corpus' + str(count) + '.txt', 'w') as output:
            output.write(str(row[1]))
        count += 1

Input CSV data:

document_id,document
0,"Bacon ipsum dolor amet kevin jerky sausage filet mignon landjaeger, turducken drumstick burgdoggen kielbasa frankfurter doner tongue meatloaf."
1,"Beef ribs jerky biltong fatback."
2,"Short loin capicola pastrami meatball. Brisket meatloaf jowl salami porchetta jerky hamburger t-bone meatball turkey."
3,"Cow ham strip steak pastrami venison."
4,"Landjaeger fatback pork loin pig sausage."

Output individual TXT files with contents of processed document column:

corpus_0 "Bacon ipsum dolor amet kevin jerky sausage filet mignon landjaeger, turducken drumstick burgdoggen kielbasa frankfurter doner tongue meatloaf."
corpus_1 "Beef ribs jerky biltong fatback."
corpus_2 "Short loin capicola pastrami meatball. Brisket meatloaf jowl salami porchetta jerky hamburger t-bone meatball turkey."
corpus_3 "Cow ham strip steak pastrami venison."
corpus_4 "Landjaeger fatback pork loin pig sausage."

It seems odd, right now, that you throw away the document_id, and that's at least worth an explanatory comment.

The code that transforms each document — the whitespace processing — should be moved into a separate function. This because it is a distinct operation from the CSV reading, and allows it to be separately unit tested.

The code needs to handle the header row from the input.

If by "better" you are asking for faster, well for that you could split up the input file and run multiple copies of your program (and changing how output filenames are generated, since the line numbers wouldn't be the same).


Optimize Scipy Sparse Matrix Factorization code for SGD I'm working on implementing the stochastic gradient descent algorithm for recommender systems (see Funk) using sparse matrices with Scipy.

This is how a first basic implementation looks like:

N = self.model.shape[0] #no of users
M = self.model.shape[1] #no of items
self.p = np.random.rand(N, K)
self.q = np.random.rand(M, K)
rows,cols = self.model.nonzero()
for step in xrange(steps):
    for u, i in zip(rows,cols):
        e=self.model-np.dot(self.p,self.q.T) #calculate error for gradient
        p_temp = learning_rate * ( e[u,i] * self.q[i,:] - regularization * self.p[u,:])
        self.q[i,:]+= learning_rate * ( e[u,i] * self.p[u,:] - regularization * self.q[i,:])
        self.p[u,:] += p_temp

I have two questions and I am hoping some seasoned python coders / recommender systems experts here could provide me with some insight into this:

1) How often should the error e be adjusted? It is not clear from Funk's and Koren's papers if it should be once for every rating or if it should be once per every factor. Meaning, should i take it out of the for loop, leave it there, or put it even deeper (over the k iterations) ?

2) Unfortunately, my code is pretty slow, taking about 20 minutes on ONE iteration over a 100k ratings dataset. This means that computing the factorization over the whole dataset takes about 10 hours. I was thinking that this is probably due to the sparse matrix for loop. I've tried expressing the q and p changes using fancy indexing but since I'm still pretty new at scipy and numpy, I couldn't figure a better way to do it. Do you have any pointers on how i could avoid iterating over the rows and columns of the sparse matrix explicitly?

There may be other optimizations available, but this one alone should go a long way...

When you do e = self.model - np.dot(self.p,self.q.T), the resulting e is a dense matrix the same size as your model. You later only use e[u, i] in your loop, so you are throwing away what probably are millions of computed values. If you simply replace that line with:

e = self.model[u, i] - np.dot(self.p[u, :], self.q[i, :])

and then replace e[u, i] with e, you will spare yourself a huge amount both of computations and memory accesses, which should boost performance by a lot.


Safe stack operations with lens I wrote this code to learn how to work with State monad. Then I added Either result to gracefully handle error condition (stack underflow).

I believe most of this code is actually unnecessary. I know there are stateful and monadic lens out there, there is EitherT transformer and also StateStack monad. How can I go about incorporating those libraries into this example?

For example, I imagine that it is possible to replace safely with %= or mapMOf from lens library once appropriate EitherT monad stack is constructed but I'm not sure how to get there.

{-# LANGUAGE LambdaCase #-}

module Main where

import           Control.Lens
import           Control.Monad.State

push :: a -> State [a] ()
push a = get >>= put . (a:)

pop :: State [a] (Either String a)
pop = get >>= \case (x:xs) -> liftM (const $ Right x) $ put xs
                    _      -> return $ Left "Stack underflow"

safely :: (a -> a) -> (Either String a) -> State [a] ()
safely f = either (const $ return ()) (push . f)

-- Stack operations that cannot crash program.
--
-- To avoid passing stack back and forth in pure language, use State monad.
-- To avoid `if` statements for overflow checking, use Either monad.
-- To avoid code duplication use parameterized and high-order types.
main :: IO ()
main = do
    print $ runState (push "Hello" >> pop >> pop >>= safely (++" World!") >> pop) []
    print $ runState (push 2 >> pop >>= safely (**7) >> pop) []

I don't really see how lens will help you at all here, but you can certainly take advantage of monad transformers to handle part of your calculations, and make it so that you can layer you stack operation language on top of any monad. (so that you can, as I show below, print out the stack in the middle of the calculation)

{-# LANGUAGE LambdaCase #-}

module Main where

import           Control.Monad.State
import           Control.Monad.Except
import           Control.Monad.Identity

type StackMT s m a = ExceptT String (StateT [s] m) a
type StackM s a = StackMT s Identity a

push :: (Monad m) => s -> StackMT s m ()
push a = modify (a:)

pop :: (Monad m) => StackMT s m s
pop = get >>= \case (x:xs) -> put xs >> return x
                    _      -> throwError "Stack underflow"

safely :: (Monad m) => StackMT s m a -> StackMT s m (Either String a)
safely act = liftM Right act `catchError` (return . Left)

safely_ :: (Monad m) => StackMT s m a -> StackMT s m ()
safely_ act = (act >> return ()) `catchError` (\_ -> return ())

runStackT :: (Monad m) => StackMT s m a -> [s] -> m (Either String a, [s])
runStackT = runStateT . runExceptT

runStack :: StackM s a -> [s] -> (Either String a, [s])
runStack act stack = runIdentity $ runStackT act stack

runOnEmpty :: StackM s a -> (Either String a, [s])
runOnEmpty = flip runStack []

runOnEmptyIO :: StackMT s IO a -> IO (Either String a, [s])
runOnEmptyIO = flip runStackT []

-- Stack operations that cannot crash program.
--
-- To avoid passing stack back and forth in pure language, use State monad.
-- To avoid `if` statements for overflow checking, use Either monad.
-- To avoid code duplication use parameterized and high-order types.
main :: IO ()
main = do
    print =<< runOnEmptyIO (do push "Hello"
                               _ <- pop
                               safely_ $ pop >>= push . (++" World!")
                               get >>= liftIO . putStrLn . ("Stack: " ++) . show
                               pop)
    print $ runOnEmpty $ do push (2::Double)
                            safely_ (pop >>= push . (**7))
                            pop

As you can see, I also added a few convenience methods.

Note that the way this is set up, the StackMT monad transformer has to be the outermost in the stack; that is, if you attempt to apply ListT or ReaderT to something of the form StackMT s m, then in the resulting monad push and pop won't work.


An alternative algorithm for sorting a stack using only one additional stack From "Cracking the Coding Interview":

  Write a program to sort a stack in ascending order (with biggest items on top). You may use at most one additional stack to hold items, but you may not copy the elements into any other data structure (such as an array). The stack supports push, pop, is_empty, and peek

The classic solution I found online to this (and the one in the book) is something like this:

Algo #1 (Classic)

 def sort_stack(primary):
   secondary = []
   while primary:
     tmp = primary.pop()
     while (secondary and secondary[-1] > tmp):
       primary.append(secondary.pop())
     secondary.append(tmp)
   return secondary

The gist of this being that we will return our secondary/auxiliary stack after sorting via \$O(n^2)\$ time.

This is not what my initial approach was, however, and I do think my approach has some interesting qualities:

Algo #2 (Mine)

def sort_stack(primary):
  did_sort = False
  secondary = []
  while not did_sort:
    # move from primary to secondary, pushing larger elements first when possible
    desc_swap(primary, secondary)
    # move from secondary back to primary, pushing smaller elements first when possible. Set did_sort = True if we're done and can exit.
    did_sort = asc_swap(secondary, primary)
  return primary

def asc_swap(full, empty):
  temp = None
  did_sort = True
  yet_max = None

  while full:
    if not temp:
      temp = full.pop()
    if full:
      if full[-1] < temp:
        insert = full.pop()
        if insert < yet_max:
          did_sort = False
        yet_max = insert
        empty.append(insert)
      else:
        empty.append(temp)
        temp = None
  if temp:
    empty.append(temp)
  return did_sort

def desc_swap(full, empty):
  temp = None
  while full:
    if not temp:
      temp = full.pop()
    if full:
      if full[-1] > temp:
        empty.append(full.pop())
      else:
        empty.append(temp)
        temp = None
  if temp:
    empty.append(temp)

Now obviously it is not nearly as clean or elegant, but it could be with some helper functions that dynamically choose our comparator and choose which element to push, etc.

Basically what it is doing is this:

# Start with stack in wrong order (worst case)
primary:   4 3 2 1
secondary:

# Swap to secondary, pushing larger elements first (1 is held in temp until the end because it is smaller than the following elements)
primary:
secondary: 2 3 4 1

# Swap back to primary, pushing smaller elements first
primary:   1 3 2 4
secondary:

# back to secondary
primary:
secondary: 4 3 2 1

# Back to primary, finished
primary:   1 2 3 4
secondary:

This strategy has a best-case/worst-case tradeoff.
Algo #1 actually performs worst when the stack is already sorted and best when the stack is sorted in the wrong order, and algo #2 does the opposite.

Questions

What are your thoughts? I think it is just an interesting way to sort that I haven't seen before.
Is there a name for this kind of sorting? I couldn't find similar algos but I'm sure theyre out there and would love to be able to describe it/recognize it better.

I really don't recommend trying to write anything that is more complicated than necessary during an interview, especially if it's on a whiteboard. Interviews are stressful enough for you; introducing more opportunities for errors is not a good idea. As for the interviewer, their likely thoughts are that your code lacks elegance and is hard to verify.

In this case, your code is buggy and crashes for sort_stack([3, 1, 4, 1]) on line 22, in asc_swap:

    if insert < yet_max:
TypeError: '<' not supported between instances of 'int' and 'NoneType' `


how to optimize vba looping code using variables? This looping has no problem if I have under 100 rows in the looping table sheet. However, If it go over 100 rows, looping takes a while to filtering and paste value in looping sheet. Is there any way to optimize this VBA code to run faster?

Sub Testingloop()
Dim endrown As String
Dim ex As String
Dim ez As String
Dim eh As String
Dim eg As String
Dim el As String
Dim ee As String
Dim es As String
Dim ef As String
Dim ei As String
Dim i As Integer
Dim LastRowColumnA As Long: LastRowColumnA = Sheets("looping").Cells(Rows.Count, 1).End(xlUp).Row

Application.ScreenUpdating = False

Sheets("looping table").Select
endrown = Sheets("looping table").Range("I1000").End(xlUp).Row
    For i = 3 To endrown

        ee= Cells(i, 9).Value
        ex= Cells(i, 10).Value
        ez= Cells(i, 11).Value
        es = Cells(i, 12).Value
        ef = Cells(i, 13).Value
        ei = Cells(i, 14).Value
         eh = Cells(i, 15).Value
        eg= Cells(i, 16)
        el= Cells(i, 17)

        Sheets("looping").Select
             ActiveSheet.UsedRange.AutoFilter Field:=1, Criteria1:=ee
             ActiveSheet.UsedRange.AutoFilter Field:=2, Criteria1:=ex
             ActiveSheet.UsedRange.AutoFilter Field:=3, Criteria1:=ez
            ActiveSheet.UsedRange.AutoFilter Field:=4, Criteria1:=es
            ActiveSheet.UsedRange.AutoFilter Field:=5, Criteria1:=ef
             ActiveSheet.UsedRange.AutoFilter Field:=6, Criteria1:=ei
            ActiveSheet.UsedRange.AutoFilter Field:=7, Criteria1:=eh
        On Error Resume Next
        Range("H2:H" & LastRowColumnA).SpecialCells(xlCellTypeVisible).Value = eg
        Range("I2:I" & LastRowColumnA).SpecialCells(xlCellTypeVisible).Value = el
            ActiveSheet.ShowAllData
                Sheets("looping table").Select

                   Next i

End Sub

For a start:

Sub Testingloop()
' Move this to the top. It doesn't affect your code, but it is good practice
' to put statements like these at the top and bottom of your code to make them
' easy to find.
Application.ScreenUpdating = False

' This should be a number type and not a string
Dim endrown As Long

Dim ex As String
Dim ez As String
Dim eh As String
Dim eg As String
Dim el As String
Dim ee As String
Dim es As String
Dim ef As String
Dim ei As String
Dim i As Integer
Dim LastRowColumnA As Long

' Better method of loading in multiple args, this will allow you to get
' all of your args in one go, and thus will speed up performance a bit.
' Practicing this early will also make it easier to develop more complex
' projects in the future.
Dim arrArgs as Variant

' Create objects to hold the main workbook and worksheet you reference
' This allows for accurate range references.
Dim wb as Workbook, Dim ws as Worksheet

' Thisworkbook refers to the workbook containing the code.
Set wb = ThisWorkbook
Set ws = wb.Sheets("looping")

' Create a variable to hold the usedrange.
Dim rUsed as Range

' Qualified your cells reference with 'ws' to ensure the proper range is set
LastRowColumnA = ws.Cells(ws.Rows.Count, 1).End(xlUp).Row

ws.Activate ' Use Activate over select, but it is best to avoid either

endrown = ws.Range("I1000").End(xlUp).Row
    For i = 3 To endrown
        ' When referring to the same object repeatedly, use a with block
        ' This does a few things, first it allows your code to skip evaluation
        ' of your 'ActiveSheet' reference since it is already told where to look.
        ' It also makes your code easier to read, cleaner, and more efficient.
        With ws
            ee = .Cells(i, 9).Value ' Be sure to qualify your cells references
            ex = .Cells(i, 10).Value
            ez = .Cells(i, 11).Value
            es = .Cells(i, 12).Value
            ef = .Cells(i, 13).Value
            ei = .Cells(i, 14).Value
            eh = .Cells(i, 15).Value
            eg = .Cells(i, 16).Value ' I am assuming you intended to get the value here
            el = .Cells(i, 17).Value
        End With

        ' Instead of referring to the different ranges you could do something like
        ' arrArgs = ws.Cells(i, 9).Resize(8, 1).Value

        ' Using a declared variable is good practice, and may improve performance slightly
        Set rUsed = ws.UsedRange
        With rUsed
             .AutoFilter Field:=1, Criteria1:=ee
             .AutoFilter Field:=2, Criteria1:=ex
             .AutoFilter Field:=3, Criteria1:=ez
             .AutoFilter Field:=4, Criteria1:=es
             .AutoFilter Field:=5, Criteria1:=ef
             .AutoFilter Field:=6, Criteria1:=ei
             .AutoFilter Field:=7, Criteria1:=eh
        End With

        On Error Resume Next
        With ws
            .Range("H2:H" & LastRowColumnA).SpecialCells(xlCellTypeVisible).Value = eg
            .Range("I2:I" & LastRowColumnA).SpecialCells(xlCellTypeVisible).Value = el
            .ShowAllData
        End With
    Next i
End Sub

This won't make a tremendous impact on performance, but it will make your code easier to work with. I also strongly encourage you to use the array instead of creating a separate string for each value you need to filter by. Again, not a big performance impact, but a good start on improving your coding.

Finally, depending on what your code is attempting to do you could try performing the operations in an array. You may also want to consider changing the calculation mode of the worksheet to xlCalculationManual (or something like that).


Optimizing database access using LINQ I have the following method that I am trying to optimize. Currently, the method works great, however, it takes a little over 3 hours to complete. I feel like, using some nifty LINQ joins, it should be able to run in minutes. Here is the method:
    public async Task UpdateEmailTable(IEnumerable<StudentEmail> students)
    {
        Console.WriteLine("Updating Email table");
        var studentEmails = students.ToList();
        int totalStudents = studentEmails.Count;
        int currentCount = 0;
        float lastPercentage = -1;

        foreach (var student in studentEmails)
        {
            var isIn = await _context.BarcDemoGraphicEmail.AnyAsync(z => z.Sridentifier == student.StudentId);

            if (!isIn)
            {
                var ar = await _context.Barcaccount.FirstOrDefaultAsync(z => z.SrIdentifier == student.StudentId);

                await _context.BarcDemoGraphicEmail.AddAsync(new BarcDemoGraphicEmail
                {
                    Sridentifier = student.StudentId,
                    Email = student.EmailAddress,
                    Updatedate = DateTime.Now,
                    Aridentifier = ar == null ? "" : ar.ArIdentifier
                });
            }
            else
            {
                var rec = await _context.BarcDemoGraphicEmail.Where(z => z.Sridentifier == student.StudentId)
                    .FirstAsync();
                rec.Email = student.EmailAddress;
                rec.Updatedate = DateTime.Now;
            }

            await _context.SaveChangesAsync();

            int percentage = (int)Math.Round((float)currentCount / totalStudents * 100);
            if (lastPercentage != percentage) Console.Write("\r" + percentage + "%");
            lastPercentage = percentage;
            currentCount++;
        }
    }

So this method is called with a list of student ids and emails. This list does not come from the database. This list comes from a web call and must be passed into the method. It then iterates through the list, one at a time. It takes the student's id and checks to see if it exists in the BarcDemoGraphicEmail table, there by setting the isIn flag.
If the student ID is NOT in the BarcDemoGraphicEmail table, it pulls the account information from another table called BarcAccount. It then inserts this student's email into the BarcDemoGraphicEmail using the information from the BarcAccount table and the email from the students current iteration.
If the student ID IS in the BarcDemoGraphicEmail table, it simply pulls that record and updates the UpdateDate field and the Email field from the current iteration.
As I said earlier, this method currently works fine, however it takes way too long. How can I optimize my code (or possibly optimize the database) so this executes faster?
This is a console .NET Core 3.1 C# application using Entity Framework Core 3.1. The database is a SQL Server database.

Some quick remarks:

DemoGraphic isn't a compound word so there shouldn't be a capital letter in the middle. However, Barcaccount is so it should be called BarcAccount. Same for Sridentifier, Updatedate, Aridentifier.

Use meaningful variable names. I have no idea what ar is, or sr.

UpdateEmailTable(IEnumerable<StudentEmail> students) and then var studentEmails = students.ToList(); Use properly named and typed parameters. Your method requires a List<StudentEmail> studentEmails (or perhaps ICollection<StudentEmail> studentEmails), write it like that and make the calling code to pass the correct argument.

This method is peppered with Console.WriteLine. This means it mixes business logic and UI. Bad idea. At least change this to use a logging system, and then configure that so it outputs to the command line.

You say this takes three hours, which suggests either a massive volume of data or particularly slow code (or perhaps your tables are not correctly indexed?). Perhaps implementing logic that uses SqlBulkInsert could solve the performance issue. Perhaps take the incoming data and store it in a table, and then apply one or two SQL queries to update the data in BarcDemoGraphicEmail.


Python genetic algorithm I haven't done any research into how you actually write a genetic algorithm, so this is my best guess. What I'm really curious to know is two-fold:

Have I created a genetic algorithm?
If I have, how can I continue to explore the subject more?

I hope this code and docstrings are enough explanation for how the code works.

from random import randint

import time

class Organism(object):

    def __init__(self, r=0, g=0, b=0):
        """Initialize the class with the RGB color values."""
        self.r = r
        self.g = g
        self.b = b

    @property
    def fitness(self):
        """The lower the fitness the better."""
        total = self.r + self.g + self.b
        return abs(765 - total)

    @classmethod
    def spawn(cls, parent):
        """Return a mutated generation of ten members."""
        generation = []
        for number in range(0, 10):
            r = cls.mutate(parent.r)
            g = cls.mutate(parent.g)
            b = cls.mutate(parent.b)
            generation.append(cls(r, g, b))
        return generation

    @staticmethod
    def mutate(value):
        """Mutate the value by 10 points in either direction."""
        min_, max_ = value - 10, value + 10
        return randint(min_, max_)

def breed_and_select_fittest(individual):
    """Return the fittest member of a generation."""
    generation = Organism.spawn(individual)
    fittest = generation[0]
    for member in generation:
        if member.fitness < fittest.fitness:
            fittest = member
    return fittest

if __name__ == '__main__':
    individual = Organism()  # abiogenesis!

    while True:
        individual = breed_and_select_fittest(individual)
        print individual.fitness

        time.sleep(0.2)

That is, more or less, a genetic algorithm, though it only implements mutation and not recombination:

@classmethod
def recombine(cls, parentA, parentB):
    traits = {}
    for t in ("r", "g", "b"):
        traits[t] = (getattr(parentA, t) + getattr(parentB, t))/2
    return cls(**traits)

There are, of course, many ways to implement recombination.  For example, instad of the simple averaging I do above, you could instead choose a random parent to take each trait from.  The best solution depends on your problem space.

To learn more, I'd suggest googling and finding tutorials or examples, and then trying to solve some more realistic problems with them.  Can you adapt them to solve any ProjectEuler problems?  Or find another application where they're useful?


More ergonomic / convenient STL algorithm calls -- sort order & member field extraction using std::invoke While we patiently wait for ranges in C++20, it can still get somewhat frustrating to write begin(), end() and micro-lambdas to specify "sort order" and "field extraction".

It is the latter, "field extraction" which I find is very commonly needed, but rarely discussed. The C++11 lambda has made it easier but the syntax is still very verbose for a very common task. There was even a recent slack/cpplang question about it. Inspired by a comment in Sean Parent's famous Seasoning talk, where he suggested using std::bind to abstract this syntax, I looked into modern alternatives. std::bind has a bad name for being large and slow. More modern is std::invoke from C++17.

So my proof of concept below tries to get to some terser syntax for these common operations using std::invoke.

I am looking for feedback on:

Coding style
Constraining that template - with / without c++20 concepts (it's wide open right now)
The new, terser calling syntax for STL algos which operate on some user defined class and require field/member extraction and potentially sorting on those.
std::sort is just one example. This could work for std::accumulate or anything really. Would I need up to 115 such wrapper templates? is there a better way.

Code size may still be a concern (even with std::invoke rather than std::bind) as shown by the the generated ASM here. The base case of the traditional lambda generates ~2000 lines of assembly code (LOA). Then each of the "new syntax" lines generates a further 2000 LOA -bringin it to ~8000 LOA in total for clang-9 -O3. Because the sortby template is stamped out for each type of lambda.

I haven't measured performance. Will all this code slow it down significantly with a lot of boilerplate and indirection, or is it just code size that suffers?

EDIT: Benchmark data has now been added below.

#include <algorithm>
#include <functional>
#include <iostream>
#include <ostream>
#include <string>
#include <vector>

struct Employee {
  int         id;
  std::string firstname;
  std::string lastname;
  int         yob;

  [[nodiscard]] int getAge() const noexcept { return 2020 - yob; }

  friend std::ostream& operator<<(std::ostream& stream, const Employee& e) {
    return stream << e.id << ", " << e.firstname << ", " << e.lastname << ", " << e.yob << ", "
                  << e.getAge();
  }
};

namespace os::algo {

// the following 6 lines make it possible
template <typename Container, typename Member, typename Comparison = std::less<>>
void sortby(Container& c, Member&& m, Comparison comp = Comparison()) {
  std::sort(c.begin(), c.end(), [&m, &comp](const auto& a, const auto& b) {
    return comp(std::invoke(m, a), std::invoke(m, b));
  });
}

} // namespace os::algo

int main() {

  using os::algo::sortby;

  auto employees = std::vector<Employee>{
      {1, "James", "Smith", 2008},
      {2, "John", "Jones", 1998},
      {3, "Sarah", "Evans", 1968},
      {4, "Michelle", "Harris", 1978},
  };

  // BEFORE: This is what's required with normal STL calls
  std::sort(employees.begin(), employees.end(),
            [](const auto& a, const auto& b) { return a.firstname < b.firstname; });

  // AFTER: and now we can do this...

  // the easy part -- no begin(), end()

  // trivial, terse sorting by any member field
  sortby(employees, &Employee::firstname);

  // trivially specify sort direction
  sortby(employees, &Employee::yob, std::greater<>());

  // can use getters too
  sortby(employees, &Employee::getAge, std::greater<>());

  for (const auto& e: employees) std::cout << e << "\n";

  // std::sort is but one example. The same technique applies to any STL algorithm

  return 0;
}

Indeed, your sortby when used in the style shown, with data members, will not be quite as fast as if you used the STL with a lambda. Look at the difference in assembly between

struct Date {
    int year, month, day;
};

void test1(std::array<Date, 100>& a) {
    sortby(a, &Date::month);
    sortby(a, &Date::year);
}

void test2(std::array<Date, 100>& a) {
    sortby(a, [](const auto& x){ return x.month; });
    sortby(a, [](const auto& x){ return x.year; });
}

In test1, the difference between &Date::month and &Date::year is encoded in the runtime value of the member pointer. In test2, the difference is encoded in the compile-time type of the lambda. So test2 instantiates two different template instantiations, whereas test1 calls the same instantiation twice with different inputs. (But if both calls are inlined, the optimizer might be able to hide the difference.)

However, this is not the fault of your sortby helper; it's the fault of using member pointers instead of custom lambdas. You can observe the same differences happening if you call std::sort with a function pointer, versus calling std::sort with an instance of some bespoke type such as std::less<> or std::greater<>.

You pass Member&& m by forwarding reference, but then you use it as an lvalue (without forwarding it again). I think I've mentioned before: if you don't intend to forward an argument, then you shouldn't be using a forwarding reference with it. In this case, you just need to look at the value of the member-pointer m without modifying it, so const Member& m would be appropriate. Or, since member data pointers and custom lambdas are both going to be small trivial types, and member function pointers are also trivial types (if slightly larger), we should seriously think about taking Member m directly by value.

Notice that you already take Comparison comp by value.

The comment about using std::begin(c) over c.begin() is accurate. If you want your sortby template to work for C-style arrays, then you should use std::begin(c). Remember, you can't pass arrays by value (they decay to pointers) — but you can certainly pass arrays by reference, and that's what you're doing here when you sortby an array "in place."

The vast majority of lambdas you write should just take [&]. I don't see any benefit to writing [&m, &comp] when it ends up meaning the same thing as [&].

So I'd write your helper like this:

template<class Container, class Member, class Comparison = std::less<>>
void sortby(Container& c, Member m, Comparison comp = Comparison()) {
    using std::begin; using std::end;
    std::sort(begin(c), end(c), [&](const auto& a, const auto& b) {
        return comp(std::invoke(m, a), std::invoke(m, b));
    });
}

using std::begin; is the "std::swap two-step," familiar to anyone who's used std::swap before. It means "consider using std::begin for unqualified calls, but if someone has provided a better-matching ADL version of begin, then we'll happily use that version instead." This is certainly overkill — I mean we could just write std::begin(c), std::end(c), — but the two-step is how all the standard customization points are meant to be used as far as I know, so we might as well stick to it.

Vice versa, I would personally shy away from using the two-step in main —

using os::algo::sortby;

This seems more confusing than helpful. Are you expecting that someone else might provide a better-matching ADL sortby? If not, then sortby(x,y,z) will always mean os::algo::sortby(x,y,z)... and if it is os::algo::sortby, why not say so?