Type hinting for scalar types As you're probably aware, PHP doesn't support type hinting for scalar types.
I feel this might be good practice to build into my framework, although I'm aware some of you might disagree seeing as PHP is a loosely-typed language.

But I'd like some feedback on the implementation of this.
Do you think it's efficient, pointless? Or could it be achieved in a better way?

This works by causing an error.
By specifying a typehint (integer), PHP is expecting a class/object called "integer", so it's giving an error as we are passing a "real integer"/scalar value".

Giving this error,

  Argument 1 passed to test() must be an instance of integer, integer given.

So with a custom error handler function, I can use preg_match() to find the expected & given types, and just compare them via the name.

The code

<?php

ini_set('display_errors', 'on');
error_reporting(E_ALL);

function typehint($level, $message){

    if($level == E_RECOVERABLE_ERROR){

        if(preg_match('/^Argument (\d)+ passed to (?:(\w+)::)?(\w+)\(\) must be an instance of (\w+), (\w+) given/', $message, $match)){
            if($match[4] == $match[5]){
                return true;
             }
        }
    }

    return false;
}

set_error_handler('typehint');

function test(integer $value){
    var_dump($value);
}

for($int = 0; $int < 1000; $int++){
    test(23);
}

?>

It seems to work ok, but I'm not sure if using preg_match() on every function argument is a good idea?

Here's the benchmarks, on 1000 function calls. (1 typeint per function)

Type-hinting        No Type-hinting
0.015               0.0017
0.0169              0.002
0.0172              0.002
0.0148              0.0019
0.0159              0.0029
0.0157              0.0019
0.0149              0.0018
0.0161              0.0029
0.0167              0.002
0.0173              0.0018
0.0149              0.0019
0.0156              0.0019
0.0153              0.0029
0.0182              0.0023
0.0151              0.0029
0.0163              0.0017
0.0179              0.0018
0.0159              0.0018
0.0154              0.0027
0.0149              0.0023

Average             Average
0.016               0.002155

So just after some thoughts on this really... good or bad?

EDIT:
Using strstr() seems to be a little quicker, although I have to specify what types to check for. Which is a small number anyway. Average time: 0.009595.

function typehint($level, $message){

    if($level == E_RECOVERABLE_ERROR){

        $integer = 'must be an instance of integer, integer given';
        $string = 'must be an instance of string, string given';

        if(strstr($message, $integer) !== false OR strstr($message, $string) !== false){
            return true;
        }
    }

    return false;
}

"So just after some thoughts on this really... good or bad?"

Bad, really bad, I'm afraid. You're trying to make the language do something it doesn't want to do, or even: can't do. You're gluing feathers to a fish's fins, and throwing it up into the air, so it can mimic a bird's behaviour.
Your test code shows a dramatic decrease of speed (after switching to strstr, still ~5 times slower than usual). And you're only testing your code with valid input. Besides, have you even checked if you can get this function to return what it needs to return, and to Where?

The simple fact of the matter is this: each function call with your improvised (hacked) type-hints will issue an error, even if the type is correct.
It will, however, issue a notice in these cases, too:

function test(integer $foo)
{
    return $foo;
}
test('123');

How will you recover from this error? Are you going to add:

if (settype($argument, $expected) == $argument)
{
    call_user_func_array($function, array($argument));
}

to your error handler? and how are you going to deal with functions that take arrays of ints, or what if the user passes null?
There's just too many things to consider here.

Your approach also poses problems when you scale things up a little: Have you thought about what would happen if you used this code in tandem with a framework, that uses namespaces, sets all sorts of handlers all over the place, and uses complicated autoloading trickery?
Even simple autoloading would mean that PHP sets out to look for this integer class. That means calling the autoloader, when that turns out to be unsuccessfull, PHP will of course use the include paths, and a lot of I/O disk access is the result. Disk I/O is a speed killer, as we all know.

Note that lookups of files that aren't found are not cached!, so each type-hint will result in disk access. Whomever is hosting this code, if it ever got used in even a medium traffic site, will not be happy.
Perhaps this has changed with PHP5.5, but most hosting services are yet to upgrade.

Each function call, in your simple example, is already approx. 5 times slower than a normal one. Add the overhead of a custom autoloader, several layers of error/exception handlers to that, and you'll see the avg time per function call drop even more (probably converging on the initial times you had). Simply because autoloaders often access the disk, too, with file_exists calls, using the include path to scan for files who's name resembles that of the class that needs to be loaded, only to fail, and have PHP do the same disk operations again.
Then, like the example I listed: getting ints from a db, or user input means that these numbers are all passed around as strings, but are perfectly castable/usable as integers.

Suppose you were able to get the called function, cast the arguments accordingly and call the function again, what would happen? Well, the function gets called again, so another error is triggered, and your handler, which is now the caller of the function will be called twice.
Of course, the second time, owing your intervention, the types will match this time, but that's 4 function calls (5 including call_user_func_array) + 2 errors being raised to complete a single call:

Original caller           /->fails, PHP====\\
 \\              //``!AUTOLOADER!``<=\\    ||
  \\             ||                   \\   \/
   \=======> function -------> error   \==>DISK I/O !!TWICE!! per call
    \|            /\              \\
    ??            ||               \==> handler, type checking + casts
    ||            ||                    ||   /\
    ||            ||call_user_func_array||    |
    ??            |======================|    |
    ||                 \\    error2           |
    ||                  \----------------------
    ||                         ||
    ??                         \/
    ||      suppose it returns, still disk I/O and
    ||                                         ||
    ||           good luck getting back there  //
    \\=========================================/

An that's just assuming no intern gets the brilliant brain-wave to actually create a string or integer class or interface!

Even if you manage to get all of this working, a recoverable error isn't guaranteed to have anything to do with your special-case type-hints. Your handler was created with a specific type of error in mind, which -logic dictates- should be an E_USER_NOTICE.

Of course, you can't force the language to emit such an error when a non-custom error is encountered.

Ah well, you could try to work on this a bit more, if you want to, but I'd say you're better of spending that time to refactoring your functions like so:

/**
 * my test func - EXPECTS INTEGER
 * Any decent IDE uses these doc-blocks
 * @param int $integer
 * @return int
 **/
function test($int)
{
    $int = (int) $int;//cast to be sure
    //code
    return ++$int;
}

That'd be a better way to spend your time, and just ensure people you work with have a decent IDE, that parses the doc-blocks.


Helper class for Fragments management A Helper class for Fragment Management.
The FragmentsManager class has methods to add and replace fragments.
The replace method checks if the given fragment is present in backstack or not and if the fragment is present it brings back the old fragment.
Need review for the following:

Code optimized.
Memory Efficiency
Overall code review
Can the replace method be improved in any way so as to avoid creation of additional fragments?
I've tried to use popbackstack() but couldn't how to use it. Can you help with that?

Thanks

import android.app.Activity;
import android.support.v4.app.Fragment;
import android.support.v4.app.FragmentManager;
import android.support.v4.app.FragmentTransaction;
import android.support.v7.app.AppCompatActivity;

class FragmentsManager {

    private static final String TAG = "FragmentsManager";
    private static FragmentTransaction transaction;
//    Return Fragment Transaction
    private static FragmentTransaction getTransaction(Activity activity){
//        if (transaction == null) {
//            return transaction;
//        }
        return getFragmentManager(activity).beginTransaction();
    }
//    Return Fragment Manager
    private static FragmentManager getFragmentManager(Activity activity){
        return ((AppCompatActivity)activity).getSupportFragmentManager();
    }

    /**
     * Add Fragment to the given ID
     * @param activity
     * @param fragment
     * @param id
     * @param add_to_backstack
     */
    static void addFragment(Activity activity, Fragment fragment, int id, boolean add_to_backstack){
        transaction = getTransaction(activity);
        transaction.add(id,fragment,fragment.getClass().getName());
        if (add_to_backstack)
            transaction.addToBackStack(fragment.getClass().getName());
        transaction.commit();
    }

     static void replaceFragment(Activity activity, Fragment fragment, int id, boolean add_to_backstack) {
        Fragment check_Fragment = getFragmentManager(activity).findFragmentByTag(fragment.getClass().getName());
        if (check_Fragment == null) {
            transaction = getTransaction(activity)
                    .replace(id,fragment,fragment.getClass().getName());
            if (add_to_backstack)
                transaction.addToBackStack(fragment.getClass().getName());
            transaction.commit();
        }
        else{
            transaction = getTransaction(activity);
            transaction.replace(id,check_Fragment,check_Fragment.getClass().getName())
                    .addToBackStack(null)
                    .commit();
        }
    }
}

your code looks very good, there are only minor issues:

1) naming

since you don't create any instances of FragmentsManager you should consider to rename it to FragmentsUtility. An Utility class provides methods to help you with your code while a Manager is an instance that does the work for you.

boolean should beginn with prefix is (or rarely as an alternative has/can/should) (in Java it's convention to use camelCase)
so rename your boolean add_to_backstack into boolean isAddedToBackstack rather (btw. in java it's convention to use camelCase)

2) comments

remove commented code!

3) javadoc

you provide some javadoc on the addFragment methode which is very crude - get things done and finish it!

the same applies to replaceFragment.


Get count of positive numbers and sum of negative numbers Write a function called manipulate_data which will act as follows:

  When given a list of integers, return a list, where the first element is the count of positives numbers and the second element is the sum of negative numbers.

Sample code

def manipulate_data(data):
    if isinstance(data, (list, tuple, set)):
        #didn't know where 0 stood so I excluded it
        return [len([n for n in data if isinstance(n, int) and n > 0]), sum(n for n in data if isinstance(n, int) and n < 0)]

The count of positive numbers can be better written as:

sum(n > 0 for n in data)

This will take a lot less memory (which is important if data becomes large), because it uses a generator expression.

Also, are all the checks of isinstance really needed? Is data sometimes not an iterable? And does it sometimes not contain integers? Because if not, then those checks will cost some time.

At the same time, it is usually better to ask for forgiveness than permission, so just use:

try:
    print len(data)
except AttributeError:  # will trigger if `data` has no `len` function
    pass

to be more inclusive and allow any custom iterable.


Making an iterative formula shorter in Excel I have just typed out very long code in Excel that just repeats itself 17 times, but each time with different cell references. Is there a more concise way of writing my code, perhaps by using another column of cells?

The code that is repeated is:

=IF(OR(ISBLANK(X2),ISBLANK(Y2)),0,X2/VLOOKUP(YEAR(Y2),$EU$3:$FH$57,14,FALSE))*$FH$57

This figure is then added on to other figures which are obtained from columns further along. I am finding the real value of the cost of alien clearing in 2014 prices over a number of years. In the first example, the column X2 holds the nominal value of the alien clearing in year YEAR(Y2), array $EU$3:$FH$57 holds the CPI data, with the year in column 1 (EU) and the average index for each year in column 14 (FH). $FH$57 holds the CPI index for 2014.

I copied the following formula down an entire column to get the real value of clearing for a number of different projects:

=IF(OR(ISBLANK(X2),ISBLANK(Y2)),0,X2/VLOOKUP(YEAR(Y2),$EU$3:$FH$57,14,FALSE))*$FH$57+IF(OR(ISBLANK(AE2),ISBLANK(AF2)),0,AE2/VLOOKUP(YEAR(AF2),$EU$3:$FH$57,14,FALSE))*$FH$57+IF(OR(ISBLANK(AL2),ISBLANK(AM2)),0,AL2/VLOOKUP(YEAR(AM2),$EU$3:$FH$57,14,FALSE))*$FH$57+IF(OR(ISBLANK(AS2),ISBLANK(AT2)),0,AS2/VLOOKUP(YEAR(AT2),$EU$3:$FH$57,14,FALSE))*$FH$57+IF(OR(ISBLANK(AZ2),ISBLANK(BA2)),0,AZ2/VLOOKUP(YEAR(BA2),$EU$3:$FH$57,14,FALSE))*$FH$57+IF(OR(ISBLANK(BG2),ISBLANK(BH2)),0,BG2/VLOOKUP(YEAR(BH2),$EU$3:$FH$57,14,FALSE))*$FH$57+IF(OR(ISBLANK(BN2),ISBLANK(BO2)),0,BN2/VLOOKUP(YEAR(BO2),$EU$3:$FH$57,14,FALSE))*$FH$57+IF(OR(ISBLANK(BU2),ISBLANK(BV2)),0,BU2/VLOOKUP(YEAR(BV2),$EU$3:$FH$57,14,FALSE))*$FH$57+IF(OR(ISBLANK(CB2),ISBLANK(CC2)),0,CB2/VLOOKUP(YEAR(CC2),$EU$3:$FH$57,14,FALSE))*$FH$57+IF(OR(ISBLANK(CI2),ISBLANK(CJ2)),0,CI2/VLOOKUP(YEAR(CJ2),$EU$3:$FH$57,14,FALSE))*$FH$57+IF(OR(ISBLANK(CP2),ISBLANK(CQ2)),0,CP2/VLOOKUP(YEAR(CQ2),$EU$3:$FH$57,14,FALSE))*$FH$57+IF(OR(ISBLANK(CW2),ISBLANK(CX2)),0,CW2/VLOOKUP(YEAR(CX2),$EU$3:$FH$57,14,FALSE))*$FH$57+IF(OR(ISBLANK(DD2),ISBLANK(DE2)),0,DD2/VLOOKUP(YEAR(DE2),$EU$3:$FH$57,14,FALSE))*$FH$57+IF(OR(ISBLANK(DK2),ISBLANK(DL2)),0,DK2/VLOOKUP(YEAR(DL2),$EU$3:$FH$57,14,FALSE))*$FH$57+IF(OR(ISBLANK(DR2),ISBLANK(DS2)),0,DR2/VLOOKUP(YEAR(DS2),$EU$3:$FH$57,14,FALSE))*$FH$57+IF(OR(ISBLANK(DY2),ISBLANK(DZ2)),0,DY2/VLOOKUP(YEAR(DZ2),$EU$3:$FH$57,14,FALSE))*$FH$57+IF(OR(ISBLANK(EF2),ISBLANK(EG2)),0,EF2/VLOOKUP(YEAR(EG2),$EU$3:$FH$57,14,FALSE))*$FH$57

Why not create user-defined functions? User-defined functions can be used in the same flavor as Excel functions (i.e. =CUSTOM_FUNC_FOO(A1:B2)), and be composed similarly in formulas. There are many advantages: better readability (because the logic is encapsulated), better algorithms (because you gain access to programming structures). The downside is that you need to write a bit of VBA, which you may not be familiar with (it's not complicated though).


Simple simulation of animals eating other animals I was given the exercise of refactoring the following code, thus ensuring it was following the SOLID design principles. It refers to an Animal Park, where there are a couple of different species of animals, and it defines the rules in which these animals can eat other animals. The Animal Park class processes a polymorphic array of the represented animals.

    public abstract class Animal
    {
        public Size Size { get; set; }
        public bool IsDead { get; set; }

        public Animal(Size size)
        {
            this.Size = size;
            this.IsDead = false;
        }

        public abstract void Eat(Animal obj);
    }

    public enum Size
    {
        Small,
        Medium,
        Big
    };

    public class Lion : Animal
    {
        public Lion(Size size) : base(size)
        {
        }

        public override void Eat(Animal animal)
        {
            if (animal is Lion && this.Size > animal.Size)
            {
                animal.IsDead = true;
            }

            if (animal is Hyena && this.Size >= animal.Size)
            {
                animal.IsDead = true;
            }
        }
    }

    public class Hyena : Animal
    {
        public Hyena(Size size) : base(size)
        {
        }

        public override void Eat(Animal animal)
        {
            if (animal is Hyena && this.Size > animal.Size)
            {
                animal.IsDead = true;
            }
        }
    }

    public static class AnimalPark
    {
        public static void Process(IList<Animal> animals)
        {
            int length = animals.Count;
            for (int i = 1; i < length; i++)
            {
                animals[i - 1].Eat(animals[i]);
            }
        }
    }

The code above breaks the Open/Closed Principle because every time I would need to add a new animal, I would not be able to do it just by extending the existing classes or by adding new ones: I would have to modify the existing classes. But I am struggling a lot to fix it properly. I came up with a solution (below), but not only I rely on Reflection, which I think can be avoided, it won't work in the cases where the Eat() method is not implemented, thus ending calling the Eat() method in the base class in a loop.

public interface IEater
{
    public void Eat(IEater obj);
}

public abstract class Animal : IEater
{
    public Size Size { get; set; }
    public bool IsDead { get; set; }

    public Animal(Size size)
    {
        this.Size = size;
        this.IsDead = false;
    }

    public void Eat(IEater obj)
    {
        if ( this.IsDead )
        {
            return;
        }
        var method = this.GetType().GetMethod("Eat", new Type[] { obj.GetType() });
        method.Invoke(this, new object[] { obj });
    }
}

public enum Size {
    Small,
    Medium,
    Big
};

public class Lion : Animal
{
    public Lion(Size size) : base(size)
    {
    }

    public void Eat(Hyena hyena)
    {
        if (this.Size >= hyena.Size)
        {
            hyena.IsDead = true;
        }
    }

    public void Eat(Lion lion)
    {
        if (this.Size > lion.Size)
        {
            lion.IsDead = true;
        }
    }
}

public class Hyena : Animal
{
    public Hyena(Size size) : base(size)
    {
    }

    public void Eat(Hyena hyena)
    {
        if (this.Size > hyena.Size)
        {
            hyena.IsDead = true;
        }
    }
}

public static class AnimalPark
{
    public static void Process(IList<IEater> animals)
    {
        int length = animals.Count;
        for(int i=1; i<length; i++)
        {
            animals[i - 1].Eat(animals[i]);
        }
    }
}

What are your thoughts on my solution? How would you handle this case?

every time I would need to add a new animal, I would not be able to do it just by extending the existing classes or by adding new ones: I would have to modify the existing classes.

The problem exists in both your approaches. It is because the eating preferences of individual species is mutualy dependent on all other existing animals. If you add new animal, you define what existing animals it likes to eat, but unless you modify existing animals, no existing animal will ever be interested in eating the new animal.

Generally relying on object being of a specific type is not polymorphism, it's a rape.

You need to abstract and centralize the eating preferences somehow.

public enum AnimalKind
{
  Lion,
  Hyena
}

public interface IDinner
{
  public AnimalKind Kind {get;}
  public Size Size {get;}
}

public interface IEater
{
  public bool Likes(IDinner dinner);
}

public interface IAnimal : IEater, IDinner
{
  bool IsDead {get;}
  void Kill();
}

class Animal : IAnimal
{
  public AnimalKind Kind {get;}
  public Size Size {get;}
  public bool IsDead {get;}

  private IDictionary<AnimalKind, Size> Preferences {get;}

  public Animal(AnimalKind kind, Size size, IDictionary<AnimalKind, Size> preferences)
  {
    this.Kind = kind;
    this.Size = size;
    this.Preferences = preferences;
  }

  public bool Likes(IDinner dinner)
  {
     var maxSize = this.Preferences[dinner.Kind];
     return maxSize != null && dinner.Size <= maxSize;
  }

  public void Kill()
  {
    IsDead = true;
  }
}

public interface IAnimalFactory
{
  public IAnimal CreateAnimal(AnimalKind kind, Size size);
}

public class AnimalFactory : IAnimalFactory
{
  // centralized place for all animals and their eating preferences
}

public static class AnimalPark
{
    public static void Process(IList<IAnimal> animals)
    {
        int length = animals.Count;
        for(int i=1; i<length; i++)
        {
            var eater = animals[i - 1];
            var dinner = animals[i];
            if (!dinner.IsDead && !eater.IsDead && eater.Likes(dinner)) {
                dinner.Kill();
            }
        }
    }
}

Adding new animal now means just extending AnimalKind enum and modifying AnimalFactory class to be aware of the new kind.

Although you might object that having the enum is also not nice. And I kinda agree, but I didn't want to invent an abstraction of the dinner description that was not provided by you. Maybe dinner description could instead be something like a description of how big teeth and paws the dinner has, how aggresive and vicious they look, if they have colors that imply being venomous, etc... If you impleent something like this and get rid of the AnimalKind enum, you will need to change the factory to contain a method per existing animal, but I would not consider it a problem, it may actually be so even with the enum existing...

Also notice I have moved the actual killing of the animal to the AnimalPark class, because that would be the same for all animals. An animal is always killed when eaten and stays alive when not eaten, right?


Finding the longest path, avoiding obstacles in a 2D plane The Scenario

You are given a matrix of size m x n (width x height) with m*n spots where there are a few obstacles. Spots with obstacles are marked as 1, and those without are marked as 0. You can move vertically or horizontally, but can visit each spot only once. The goal is to cover as much area/spots as possible, avoiding obstacles.

Input

A 2D Matrix consisting of 0s and 1s and the coordinates of starting point.

Example Matrix:

0 1 0
0 0 0
1 0 0

Starting Point (0,0).
The coordinates are nothing but array indices.

Output

Output should be a line containing comma separated moves. A move is a space separated direction (n,s,e,w) and number of units of movement in the direction (range 1-1000).

Example output for the above matrix would be,

s 1,e 1,s 1,e 1,n 2

(0,0) --> (1,0) --> (1,1) --> (2,1) --> (2,2) --> (1,2) --> (0,2)

which is the longest path, visiting each coordinate only once and avoiding those coordinates with obstacles.

My algorithm

  for every vertex/coordinate, recursively choose the neighbor which
  yields the longest path

Code

def getDirections(p1, p2):
    return {(1,0):"n", (-1,0):"s", (0,1):"w", (0,-1):"e"}[(p1[0]-p2[0], p1[1]-p2[1])]

def findPath(start, mat):
    m, paths = getMax(mat, getNeighbours(mat), start, list())
    print(paths)
    if len(paths) == 1:
        return ""
    directions = ""
    prev = getDirections(paths[0], paths[1])
    current = ""
    ctr = 1
    for i in range(1, len(paths)-1):
        current = getDirections(paths[i], paths[i+1])
        if current == prev:
            ctr += 1
        else:
            directions, prev, ctr = directions + prev + " " + str(ctr) + ",", current, 1
    return directions + current+ " " +str(ctr)

#Neighbor is (-1,-1) if indices are out of range. For top and bottom rows for example.
def getNeighbours(mat):
    neighbours = dict()
    for i in range(len(mat)):
        for j in range(len(mat[i])):
            n = (i-1, j) if i-1 >= 0 else (-1, -1)
            s = (i+1, j) if i+1 < len(mat) else (-1, -1)
            w = (i, j-1) if j-1 >= 0 else (-1, -1)
            e = (i, j+1) if j+1 < len(mat[i]) else (-1, -1)
            neighbours[(i,j)] = {'n':n, 's':s, 'w':w, 'e':e}
    return neighbours

def getMax(mat, neighbours, coordinates, visited):
    if coordinates == (-1,-1):
        return -1, []
    elif mat[coordinates[0]][coordinates[1]] == 1:
        return -1, []
    elif coordinates in visited:
        return -1, []
    else:
        visited.append(coordinates)
        n, nlist = getMax(mat, neighbours, neighbours[coordinates]['n'], visited[:])
        s, slist = getMax(mat, neighbours, neighbours[coordinates]['s'], visited[:])
        w, wlist = getMax(mat, neighbours, neighbours[coordinates]['w'], visited[:])
        e, elist = getMax(mat, neighbours, neighbours[coordinates]['e'], visited[:])
        if max(n,s,w,e) == -1:
            return 0, visited
        if max(n,s,w,e) == n:
            visited = nlist
        elif max(n,s,w,e) == s:
            visited = slist
        elif max(n,s,w,e) == w:
            visited = wlist
        elif max(n,s,w,e) == e:
            visited = elist
        return 1+max(n,s,w,e), visited

Well, first of all (as I said in my earlier comment); "Extended algorithmic discussion may be better on Programmers.SE, however performance improvement is definitely on-topic here!".  So, let me start with (1) a quick review of some more basic things, then maybe give you (2) some advice about your approach and (3) the performance of your code.

1:   A Quick Review

1.1) Avoid Magical Complicated One-Liners

The very first thing I see in your code (really, before anything else), is the second line (the implementation of getDirections):

return {(1,0):"n", (-1,0):"s", (0,1):"w", (0,-1):"e"}[(p1[0]-p2[0], p1[1]-p2[1])]

While Python is a particularly powerful language when it comes to doing a LOT of things in one line... This usually (almost always) is not the best idea.  Both for readability, as well as for maintainability.  After all, this isn't Code Golf.

Additionally, you are also constructing the dictionary in each call - this is certainly not best practise performance wise (although in this case Python has probably optimized this for you).
But in general, if you have a lookup data structure that will not change throughout your program, you would make it a global variable, or class variable if you were using OOP.

1.2) Function Naming, and other Coding Standards

Taken from PEP8 section on Function Names:

  Function names should be lowercase, with words separated by underscores as necessary to improve readability.
  mixedCase is allowed only in contexts where that's already the prevailing style (e.g. threading.py), to retain backwards compatibility

Your function names resemble Java methods, rather than Python functions.  This strikes me as particularly unusual since you seem to be coding in a procedural style. (Often Java programmers will name things in a Java way, but they also use OO).

Rather than:

getDirections
findPath
getNeighbours
getMax

You should use a more Pythonic naming scheme, like this:

path_direction
find_path
cell_neighbours
max_path_length

1.3) Argument and Variable Names - Meaning is Important

In a lot (almost all) of your functions, I see some argument names like p1, or mat.  As well as variable names like n, s, i, j, and wlist, etc.

Certainly, at a minimum typing matrix instead of mat is MUCH better.  p1 and p2 can be figured to mean path1 and path2, but I feel the latter is much more expressive and clear.
When it comes to iteration variables like i and j, you are okay using things like this.  But it is better to restructure you loops such that you can do things like for row in matrix:, rather than for i in range(len(mat)):.

2:   A Graphical Approach

2.1) The Idea

As I eluded to in my earlier comment; "have you looked into any of the Graph Search Algorithms, such as Dijkstra and A-star (A)?*" - I (personally) think they key to this answer lays in one or another graph search algorithm.

2.2) Transformation

Let's take the example 3x3 matrix from your question:

0 1 0
0 0 0
1 0 0

Now we want to represent each of these 9 matrix cells as a node in a graph.  Given that 0 represents an empty node, and 1 is a blocked node, such a representation may look like the following:

Note that as the cell at row 0, column 0 is always the start node, we can label it as such.  This is important as most graph search algorithms will require a starting node to work from.

My apologies for the lack of planarity, however a matrix does not lend itself easily to having no cross-overs for 15 vertices!

Note, giving vertices from the blocked nodes back to the end node is (probably, depending on which exact algorithm you use) optional.  I've included it here for completeness, but you may very well decide for performance reasons to just ignore blocked nodes all together.

2.3) Weighting the Graph

The key to this approach, is the different colours links (vertices) in the graph above.
Imagine if each green vertex has a zero (or negative) weight, and each red vertex had some positive weight (say 1).
then an algorithm seeking to find the "best" (*i.e. optimum, in terms of cost) path would naturally visit the most nodes (via green links).

Taking the path produced in your question as an example:

s 1,e 1,s 1,e 1,n 2

This would represent a path with a cost of -1 + -1 + -1 + -1 + -2 + 1= -5.
Which is  cheaper (in terms of aggregated weight cost), than the sub-optimal path:

 s 1,e 2,n 1

Which would have the cost: -1 + -2 + -1 + 1 = -3!

2.4) Finding the Algorithm

Obviously, there are many choices of Graph Search algorithms out there.  And going through and examining each one is certainly not within the scope of an answer on Rode Review.  But there is plenty of information on Google/Wikipedia regarding these.

Essentially, you'll want an algorithm that can avoid cycles (i.e. only visits nodes once), find the "best" (or shortest, or lowest score, etc) path - and (most importantly) can do this in a reasonable time.
Algorithms such as DFS come to mind, although you'll no doubt need to alter most implementations for your purposes.

3:   Performance of Your Code

3.1) Data Structures

The main reason for your codes slow performance is that you appear to not be using the correct data structures for your problem space.

Instead of using lists and tuples for everything, I suggest you take a look at something like Graph-tool and use graph based data structures.

3.2) Use Existing Algorithms

As I mentioned in  §2 (A Graphical Approach) above, your best bet is to find an existing algorithm to accomplish this task (i.e. change the data structures you're working with to fit an algorithm rather than the other way around as you're currently doing).


Python3 Health System I made a code for a health system for the game I'm creating. I made it myself and I wanted some criticism on how to make it better. Thanks for viewing.

## Health system
import time
SH = 100
PA = True
while PA == (True):
  DMG = input("How much damage would you like to take?" )
  Output = int(SH) - int(DMG)
  if Output == (80):
    print("You took 20 damage!")
    print(" --------------")
    print("|■■|■■|■■|■■|  |")
    print(" --------------")
  elif Output == (60):
    print("You took 40 damage!")
    print(" --------------")
    print("|■■|■■|■■|  |  |")
    print(" --------------")
  elif Output == (40):
    print("You took 60 damage!")
    print(" --------------")
    print("|■■|■■|  |  |  |")
    print(" --------------")
  elif Output == (20):
    print("You took 80 damage!")
    print(" --------------")
    print("|■■|  |  |  |  |")
    print(" --------------")
  elif Output == (0):
    print("You took 100 damage!")
    print(" --------------")
    print("|  |  |  |  |  |")
    print(" --------------")
    print("You died!")
  else:
    print("That isn't a command")

  PA = input("Would you like to play again? |Y/N| ")
  if PA == ("Y"):
    PA = True
  elif PA == ("N"):
    PA = False
    print("The program will now close")
    time.sleep(5)
  else:
    print("That is not a command...")

I gotta start by saying it's weird to ask how much damage you want to take, my answer would always be zero, you know!

Your variable names could be more descriptive.

I understand that DMG means damage, SH would be.. something health?

Well I guess you see my point, if we don't know what the variable name means, it's harder to figure out what it does.

Considering the PEP8 guidelines towards Python, your variable should never contain upper case.

So I would change :

DMG -> damage
SH -> total_health
Output -> remaining_health
PA -> play_again

In your input you ask how much damage should be dealt. There are much more options that are invalid than valid ones. I mean, I can enter 0,20,40,60,80,100 but I can't enter any other number, so there are much more chances I face the "That isn't a command" error message than anything else.

If the only options are the values from 0 to a 100 that are multiples of twenty, you should specify it in your message :

"How much damage would you like to take? [0,20,40,60,80,100]"

There is no real advantage for the Output variable at this moment. You want to print how much damage is dealt and an health bar that corresponds to the remaining life.

if damage == 20:
    print("You took 20 damage!")

Or, even better, use the .format method.

print("You took {0} damage!".format(str(damage))

Then you could print the health bar. I don't want to go deep in details but you could write a function like print_remaining_health_bar(total_health,remaining_health) that would print according to the parameters instead of repeating this piece of code every time.

All in all you could remove the whole if/else business (except for your input validation, which is important) and have :

print("You took {0} damage!".format(str(damage))
remaining_health = total_health - damage
print_remaining_health_bar(total_health,remaining_health)

There's also a small problem with your code. When asked if I want to play again, if I enter an invalid command, I'd just restart the game.


C program to count number of lines in a file Very simple code, works fine. I'm mainly interested in the method I used to count lines. I thought about using fgetc, but I'm not even sure if it'd read the newline character, and I think it's slower too.

#include <stdio.h>
#include <stdlib.h>
#define MAX_SIZE 1000

int main(void){
    FILE *in_file;
    char line[MAX_SIZE];

    in_file = fopen("test", "r");
    if(in_file == NULL){
        fprintf(stderr, "Unable to open file");
        exit(EXIT_FAILURE);
    }

    int counter = 0; /*Number of lines*/

    while(fgets(line, sizeof(line), in_file) != NULL){
        counter++;
    }

    printf("Number of lines in the file is %i", counter);

    return 0;
}

As you say, it works fine.  But I can nitpick.

The main flaw is that you could get a wrong count if any line is longer than 999 bytes.  (In general, you should stress-test your code by cranking down the buffer size to ridiculously small numbers and checking whether you obtain the same results.)

The performance could be improved.  Since fgets() has no way of knowing in advance where the newlines occur, it must read the file contents into a temporary buffer (which is invisible to you), then copy each line into the line buffer.  Since only care about counting '\n' characters, you could read fixed-size blocks to avoid this internal copying.

It's awkward to read 1000 bytes at a time.  You would be better off reading chunks that are aligned with the blocks on the disk.  A better choice would be 1024.

You could use the perror() function to report why I/O operations failed.  Technically, fgets() could fail too, so you should check for that.

It's a good habit to ensure that your fopen() is paired with fclose().

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

#define SIZE 1024

int main(void) {
    const char filename[] = "test";
    FILE *in_file;
    char buffer[SIZE + 1], lastchar = '\n';
    size_t bytes;
    int lines = 0;

    if (NULL == (in_file = fopen(filename, "r"))) {
        perror(filename);
        return EXIT_FAILURE;
    }

    while ((bytes = fread(buffer, 1, sizeof(buffer) - 1, in_file))) {
        lastchar = buffer[bytes - 1];
        for (char *c = buffer; (c = memchr(c, '\n', bytes - (c - buffer))); c++) {
            lines++;
        }
    }
    if (lastchar != '\n') {
        lines++;  /* Count the last line even if it lacks a newline */
    }
    if (ferror(in_file)) {
        perror(filename);
        fclose(in_file);
        return EXIT_FAILURE;
    }

    fclose(in_file);
    printf("Number of lines in the file is %i\n", lines);
}

The for loop above is a bit tricky for beginners, but should be acceptable for experienced C programmers.  The loop

for (char *c = buffer; (c = memchr(c, '\n', bytes - (c - buffer))); c++)

means:

Search for the next newline, starting from the pointer c, in the remaining bytes - (c - buffer) bytes that have been read but not examined yet.
If a newline is found, make c point to the position just after it.
If no newline is found, then we're done with this chunk.  Try reading more input then.

The extra set of parentheses around c = memchr(…) is an indication to the compiler and to other programmers that the = is indeed intentional, and is not supposed to be ==.


pull things from lazy source till timeout is reached I need to pull things from lazy source till timeout is reached in this fashion:

var results = lazySource.SelectMany(item =>
{
    //processing goes here
}).Pull(timeout: TimeSpan.FromSeconds(5), times: maxNumberOfIterations);

and I have this solution:

public static class IEnumerableExtensions
{
    public static IEnumerable<T> Pull<T>(this IEnumerable<T> enumerable, int? times = null)
    {
        if (times == null)
            return enumerable.ToArray();
        else
            return enumerable.Take(times.Value).ToArray();
    }

    public static IEnumerable<T> Pull<T>(this IEnumerable<T> enumerable, TimeSpan timeout, int? times = null)
    {
        var stopwatch = new Stopwatch();

        stopwatch.Start();

        if (times != null) enumerable = enumerable.Take(times.Value);

        using (var iterator = enumerable.GetEnumerator())
        {
            while (stopwatch.Elapsed < timeout && iterator.MoveNext())
                yield return iterator.Current;
        }
    }
}

Am I just reinvented the wheel and it can be done using some external libs? like Rx/Ix maybe ...

A better name for times would be count.
Your two Pull methods are inconsistent:

The first one actually returns a copy of the entire input sequence (ToArray()) while the second one doesn't.
To keep it consistent with the standard LINQ methods the first method should also just enumerate the input sequence rather than making a copy.

The first method can then be rewritten as:

public static IEnumerable<T> Pull<T>(this IEnumerable<T> enumerable, int? count = null)
{
    return (times == null) ? enumerable : enumerable.Take(count.Value);
}

Or if you prefer to create a new enumerator:

public static IEnumerable<T> Pull<T>(this IEnumerable<T> enumerable, int? count = null)
{
    var items = (times == null) ? enumerable : enumerable.Take(count);
    foreach (var item in items)
    {
        yield return item;
    }
}

When i first read this question I assumed the intend was to only wait a maximum time for an item to arrive (i.e. input sequence is possibly blocking). Reading the code however seems that you just want to consume as many items as you can in a given time span (specifically it doesn't deal with blocking input sequences). Assuming the latter is what you wanted then I'd change the code to not use the enumerator interface but stick on a slightly higher level:

public static IEnumerable<T> Pull<T>(this IEnumerable<T> enumerable, TimeSpan timeout, int? count = null)
{
    var stopwatch = new Stopwatch();
    stopwatch.Start();

    var items = (times == null) ? enumerable : enumerable.Take(count.Value);

    foreach (var item in items)
    {
        if (stopwatch.Elapsed >= timeout) { yield break; }
        yield return item;
    }
}


How can I improve this 31-bit SuperFastHash implementation in VBA? I'm implementing a variation of the SuperFastHash in VBA for use in Excel (32-bit version, so no LongLong available) to hash strings.

To get around the limitations of signed 32-bit Long values, I'm doing the addition and bit-shifting using Double types, and then converting from Double to Long in a way that truncates it at 31 bits (the maximum positive value -- don't want to deal with two's complement and signs).

I'm getting answers and avoiding overflows so far, but I have a suspicion I'm making some mistakes in translation, since most implementations use all 32 bits of a uint and also deal with individual bytes from an array rather than 16-bit values coming from AscW().

Any suggestions of improvement, especially of the string-handling, bit-shifting, or  the final avalanche?

Public Function shr(ByVal Value As Long, ByVal Shift As Byte) As Long
    shr = Value
    If Shift > 0 Then shr = shr \ (2 ^ Shift)
End Function

Public Function shl(ByVal Value As Long, ByVal Shift As Byte) As Long
    If Shift > 0 Then
        shl = LimitDouble(CDbl(Value) * (2& ^ Shift))
    Else
        shl = Value
    End If
End Function

Public Function LimitDouble(ByVal d As Double) As Long
    '' Prevent overflow by lopping off anything beyond 31 bits
    Const MaxNumber As Double = 2 ^ 31
    LimitDouble = CLng(d - (Fix(d / MaxNumber) * MaxNumber))
End Function

Public Function SuperFastHash(ByVal dataToHash As String) As Long
    Dim dataLength As Long
    dataLength = Len(dataToHash)
    If (dataLength = 0) Then
        SuperFastHash = 0
        Exit Function
    End If
    Dim hash As Long
    hash = dataLength
    Dim remainingBytes As Integer
    remainingBytes = dataLength Mod 2
    Dim numberOfLoops As Integer
    numberOfLoops = dataLength \ 2
    Dim currentIndex As Integer
    currentIndex = 0
    Dim tmp As Double
    Do While (numberOfLoops > 0)
        hash = LimitDouble(CDbl(hash) + AscW(Mid$(dataToHash, currentIndex + 1, 1)))
        tmp = shl(AscW(Mid$(dataToHash, currentIndex + 2, 1)), 11) Xor hash
        hash = shl(hash, 16) Xor tmp
        hash = LimitDouble(CDbl(hash) + shr(hash, 11))
        currentIndex = currentIndex + 2
        numberOfLoops = numberOfLoops - 1
    Loop
    If remainingBytes = 1 Then
        hash = LimitDouble(CDbl(hash) + AscW(Mid$(dataToHash, currentIndex + 1, 1)))
        hash = hash Xor shl(hash, 10)
        hash = LimitDouble(CDbl(hash) + shr(hash, 1))
    End If
    '' Final avalanche
    hash = hash Xor shl(hash, 3)
    hash = LimitDouble(CDbl(hash) + shr(hash, 5))
    hash = hash Xor shl(hash, 4)
    hash = LimitDouble(CDbl(hash) + shr(hash, 17))
    hash = hash Xor shl(hash, 25)
    hash = LimitDouble(CDbl(hash) + shr(hash, 6))
    SuperFastHash = hash
End Function

This is quite possibly the most clever VBA code I have ever seen.

There's not much room for improvement here, except a couple naming nitpicks:

If that's what they stand for, functions shr and shl could afford to be called ShiftRight and ShiftLeft.
I don't see a reason for shr and shl to not follow the PascalCasing naming convention.
Given the almost-flawlessly consistent camelCasing convention for locals & parameters, I don't see a reason for Value and Shift parameters to not stick to it. (except perhaps VBA/VB6's stubornness with casing?)

As for string handling, as per this source AscW is faster than Asc so again, good call:

  AscW(Mid$(..)) considerations

  Don't copy too many characters with Mid$. AscW only examines the first
  character anyway. AscW(Mid$(s, x, 1)) is the best call. Note that if
  you call AscW(Mid$(s, x)) without the third parameter, Mid$ executes
  slowly when s is a long string.

  Example of potentially slow code:

For x = 1 To Len(s)
    If AscW(Mid$(s, x)) = ... Then ...
Next

  The above is better written as:

For x = 1 To Len(s)
    If AscW(Mid$(s, x, 1)) = ... Then ...
Next


Simple raw buffer queue implementation I am trying to implement a simple raw queue in C++. This is what I have come up with

#include <queue>
#include <cstdint>
#include <array>
#include <cstring>

class simple_queue
{
private:
    static constexpr uint32_t max_msg_size = 4096;
    using char_msg = std::array<char, max_msg_size>;
    std::queue<char_msg> char_queue;

public:
    simple_queue() = default;
    ~simple_queue() = default;
    simple_queue(simple_queue&&) = default;
    simple_queue& operator=(simple_queue&&) = default;

    uint32_t write(const char* const buff, const uint32_t size) noexcept
    {
        const uint32_t size_to_copy = std::min(max_msg_size, size);
        char_msg tmp_msg;
        std::memcpy(&tmp_msg, buff, size_to_copy);
        char_queue.push(std::move(tmp_msg));
        return size_to_copy;
    }

    uint32_t read(char* const buff, const uint32_t size) noexcept
    {
        if (char_queue.empty())
        {
            return 0;
        }
        const uint32_t size_to_copy = std::min(max_msg_size, size);
        std::memcpy(buff, &char_queue.front(), size_to_copy);
        char_queue.pop();
        return size_to_copy;
    }
};

Note that I am trying to implement this queue just to store raw char buffers. As far as I understand making it a template does not make sense in this case.

I have played with it a bit and it seems to work as I expect.

What can I change with this implementation?
Does it make sense to use a custom allocator in here? I am allocating memory every time I write to the queue; how can I use a custom allocator to allocate some default chunk of memory when the queue is constructed?

You are not allocating per se when when writing to the queue. std::array is backed by a plain C-array on the stack, that gets moved.

So you only ever allocate depending on the underlying container the std::queue is based on. By default this is a std::deque.

I do not really understand the need for the std::array in the type though. Why not just use a std::string and limit the size of it to 4096? There is even a constructor that does explicitely that string (const char* s, size_t n)

Before I get to the Code there are some other things I would like to mention:

why are you not useing std::copy instead of memcpy. The former works better with C++ and will in the end almost always end up as memcpy?
You do not need to define the special member functions. In fact you have forgotten 2 of then, aka copy assignment and copy constructor. Why do I say forgotten? Because there is no way to tell. So If you want your queue to be move only then you should actually delete those special member functions you dont want.
In your read function you never check whether the size you want to read is actually valid. Is that intendend? If so why? A std::array is not initialized so the memory in it that is not written by you will be random. You are not writing to it but you are copying it around. So you should actually take the minimum out of size and queue.front().size()
std::string has a member function std::string::copy that copyies a certain amount of chars to a buffer (http://www.cplusplus.com/reference/string/string/copy/). I would suggest to use that for writing back to the buffer

That leads me to the following:

#include <string>
#include <queue>
#include <vector>

class simple_queue
{
private:
    static constexpr uint32_t max_msg_size = 4096;
    std::queue<std::string, std::vector<std::string>> char_queue;

public:
   simple_queue(const simple_queue&) = delete;
   simple_queue& operator=(const simple_queue&) = delete;

    uint32_t write(const char* const buff, const uint32_t size) noexcept
    {
        const uint32_t size_to_copy = std::min(max_msg_size, size);
        char_queue.emplace(buff, size_to_copy);
        return size_to_copy;
    }

    uint32_t read(char* const buff, const uint32_t size) noexcept
    {
        if (char_queue.empty())
        {
            return 0;
        }
        std::string& msg = char_queue.front();
        const uint32_t size_to_copy = std::min(msg, size);
        msg.copy(buff, size_to_copy, 0);

        char_queue.pop();
        return size_to_copy;
    }
};

EDIT:

I forgot to mention, that now you should use a std::vector as backing of the queue as your are not storing a gargantuan array but rather a mall std::string.

Note that it actually ends up being the same. The std::deque based std::array implementation is backed by a linked list, so each array ends up in a node of a linked list, which is kind of similar to the separate allocation of the std::string


PHP MVC simple controller design I am currently rewriting a legacy PHP4 application to a MVC-style object-oriented one.
As the most complex part is the models, I came up with the following controller-view scheme. I wanted to use only variables in my views, using only <? if ($foo === 'bar'): ?>-like blocks if necessary. The controllers instead generate the variables for the views from the data generated by my models.

Is the following a good approach to MVC code separation?

The controller code looks like this:

interface InterfaceController {
    public function setup();
    public function compile();
}

abstract class AbstractPageController implements InterfaceController
{
    public static $variables = array();
    public static $template = '';

    public function compile() {
        static::$variables['header'] = Controller::run('partials/header');
        static::$variables['footer'] = Controller::run('partials/footer');

        return (new View(static::$template, static::$variables))->render();
    }
}

class ExampleController extends AbstractPageController
{
    public function setup() {
        $variables['foo'] = 'bar';

        static::$template = 'examplepage';
        static::$variables = $variables;

        return $this;
    }
}

An example route would look like that:

/* ... */

Route::get('/example/supercoolfeature', function() {
    Controller::run('example/features/supercool');
});

/* ... */

So, the router calls the following Controller class:

class Controller
{
    /**
     * Executes a controller:
     * If called as "Controller::run('foo/bar')", the function will call the
     * class "App\Controllers\Foo\BarController" and execute the setup() and run()
     * methods.
     */
    public static function run($name) {
        $parts = array();

        foreach (explode('/', $name) as $part) {
            // uppercase each controller path part so you can
            // go lowercase in the actual code
            $parts[] = ucfirst($part);
        }

        // attach the parts together and append "Controller" at the end
        $name = '\\App\\Controllers\\' . implode('\\', $parts) . 'Controller';

        // run the controller, let it prepare a view and return the rendered code
        return (new $name())->setup()->compile();
    }
}

And finally, the respective view for the controller:

<?php echo $header; ?>

Hi there, <?php echo $foo; ?>!

<?php echo $footer; ?>

your code is not bad. You write comments that don't state the obvious, so thats good. You don't make stupid rookie mistakes like I used to make, so thats good. But... (such a lovely word)
Naming
You define an interface: InterfaceController. The name tells me that every class that implements this interface, is some kind of Controller of interfaces. It sounds fancy, I must give you that.
Maybe it's personal, but I just don't like writing implements RandomInterface. Ofcourse it's an interface, thats why I implement it. And if you have a problem where the interface and the class implementing it have the same name, one of the two should not exist.

AbstractPageController implements Controller: So we have a Controller for an AbstractPage ? What is an Abstract page? This might be nitpicking, but these kind of namings lead to confusion. Either use class AbstractController or interface PageController.

Every Controller class we create needs to have 2 methods, setup and compile. However, I'm not so sure these are good names, they even smell.
setup: Whats wrong with using __construct()? This was basically created for setup.
compile: It's PhP, why should I compile, is there some kind of .Net integration? Compiling a view, I understand, but a Controller? You lost me. maube run is better.
Especially given the fact that your comment reads:
// run the controller, let it prepare a view and return the rendered code

wouldn't it make more sense to have the following code?
return (new $controllerName())->run()->renderView();

Also note that the word 'run' does not really tell us anything about what is being returned. Making us guess. What are we calling renderView() on? Try to avoid chaining unless they really make sense (Again, mabe it's me, but I just hate 'unknown method on undefined' errors). Use the __construct instead of run()/setup() and we get the following code:
return (new $controllerName())->render()

This doesn't even need comments, saves you a few key strokes #lazyDev

I also see no way of passing arguments to a Controller. Planning on using $_GET there? IF so, here is my sad face :(

Minor remarks
Your Controller has some magic strings:
'\\App\\Controllers\\'

and
'Controller'

maybe you should replace it by some constant or similar. Simply because CONTROLLER_NAMESPACE makes more sense than '\App\Controllers\'. Also make sure that the namespace is only hardcoded in one place. I just don't like hard coded strings.


Memoized solution to Count number of ways to climb n steps Problem statement : A Child is climbing up n steps with either 1 , 2, 3 hops ..how many ways can the child climb up the stairs?

source: cracking the coding interview book.

I have two solutions the brute force and the memoized one.

Wanted to know how to improve my solution further, any issues you see with this code?

description of the code:
Brute force: basically count in a recursive manner, as there are 3 ways to reach the nth step so we count number of ways to reach each of n-1 , n-2 and n-3 steps and then sum them up to find the number of ways of reaching the nth step.

memoized solution:

we are now caching the solutions in a hashmap to reuse the solutions.

The Code:

public class CountNWaysforSteps {

    // running time O(3 ^ n) inefficient solution
    public static int numberOfWays(int n){

        if(n < 0 ) return 0;
        if(n == 0 ) return 1;

        return numberOfWays(n-1) + numberOfWays(n-2) + numberOfWays(n-3);
    }
    //memoized solution running time O(N)
    public static int numberOfWaysMemoized(int n, Map<Integer,Integer> cache){

        if(n < 0 ) return 0;
        if(n == 0 ) return 1;

        if(cache.containsKey(n)){
            return cache.get(n);
        }

        int calculatedNumberOFWays = numberOfWaysMemoized(n-1, cache) + numberOfWaysMemoized(n-2, cache) + numberOfWaysMemoized(n-3, cache);
        cache.put(n,calculatedNumberOFWays);
        return calculatedNumberOFWays;
    }

    public static void main(String...args){
        System.out.println(numberOfWays(5));
        System.out.println(numberOfWaysMemoized(5, new HashMap<>()));
    }
}

First impressions:

The dynamic approach is a good idea, but it can be improved
When a Map<Integer, T> is used, see if a T[] can be used instead
When doing a dynamic solution, see if you can remove the need for recursion

By applying these tricks, my suggested solution is this:

public static int numberOfWaysFast(int n) {
    if (n < 2) {
        return n >= 0 ? 1 : 0;
    }
    int[] cache = new int[n+1];
    cache[0] = 1;
    cache[1] = 1;
    cache[2] = 2;
    for (int i = 3; i <= n; i++) {
        cache[i] = cache[i-1] + cache[i-2] + cache[i-3];
    }
    return cache[n];
}

This solution is about 50-100 times faster on my computer, and doesn't run into StackOverflowErrors for large inputs.

If you want to improve even further, you can reuse the cache array for subsequent calls, and only calculate new values when you have to. This could be applicable if you knew the maximum bound for values to be calculated, and had to rerun the function multiple times. Otherwise you could use an ArrayList<Integer>, but that's a tiny bit slower.

EDIT: forgot to handle input <2, added some code for it.


PHP code structure for string manipulation tasks I need to perform a bunch of string manipulation tasks on an original string, then output the final string. Best I could think of is extracting some functionality into functions. Any better way to structure it? Also, are the variable/function names too long?

strip out unicode chars from original string A, reverse them into string B
in the remaining string A, remove duplicate words
append: A + B

function extract_unicode_characters_from_string($str) {
    preg_match_all('/\p{Han}+/u', $str, $unicode_characters_only, PREG_SET_ORDER);
    $squashed = array_column($unicode_characters_only, 0);
    return $squashed;
}

function strip_unicode_characters_from_string($str) {
    return preg_replace('/\p{Han}+/u', '', $str);
}

function strip_duplicates($str){
    $exploded_str = explode(' ', $str);
    $exploded_str_no_empties = array_filter($exploded_str, function($item) { return !is_null($item) && $item !== ''; });
    return implode(' ', array_unique($exploded_str_no_empties));
}

$original_string = 'one two three 喞 喝 four 刷囿 two 跏正 吁';

$array_of_unicode_characters = extract_unicode_characters_from_string($original_string);
$reversed_unicode_characters = implode(' ', array_reverse($array_of_unicode_characters));

$string_without_unicode_characters = strip_unicode_characters_from_string($original_string);
$english_words_no_dupes = strip_duplicates($string_without_unicode_characters);

echo $english_words_no_dupes . ' ' . $reversed_unicode_characters;

Yes, I think your variable names are a bit long.  While it is good to be expressive, you don't want to be pushing your line width beyond the recommended max width if avoidable.

I don't know if you need many custom functions here.  The shared calls which filter empty and duplicate strings can be a custom call.  Otherwise, everything else is single-use.

Consider this snippet which simplifies much of the script by grouping the multibyte and single-byte non-whitespace substrings from the start.  No extra exploding, and only one implode call.

Code: (Demo)

function uniqueNoEmpty($array) {
    return array_unique(array_filter($array, 'strlen'));
}

$original_string = 'one two three 喞 喝 four 刷囿 two 跏正 吁';

if (!preg_match_all('~(\p{Han}+)|(\S+)~u', $original_string, $out)) {
    echo 'no qualifying strings';
} else {
    $singleBytes = uniqueNoEmpty($out[2]) ?? [];
    $multiBytes = array_reverse(uniqueNoEmpty($out[1]));

    echo implode(' ', array_merge($singleBytes, $multiBytes));
}


Aligning fstab columns I know I can use the unix column function to replicate 90% of the functionality here, so this is mostly just to practice python. I find a misaligned fstab file really hard to read. This script parses the file and aligns the columns by determining the longest entry in each column and then padding each entry in the column to that length (plus an offset). Newlines are passed over and comment lines (starting with #) are formatted to ensure there is a space between the first '#' and the comment content.

Is there anything I'm missing that could make this code better or more readable?

from pathlib import Path
from typing import NamedTuple

FSTAB = Path('/etc/fstab')
OFFSET = 4 # number of spaces desired between columns

class FstabLine(NamedTuple):
    device_spec: str
    mount_point: str
    fs_type: str
    options: str
    dump: int
    pass_: int

def format_line(len_dict: dict, line: FstabLine) -> str:
    return (
        f"{line.device_spec:{len_dict['device_spec']}}"
        f"{line.mount_point:{len_dict['mount_point']}}"
        f"{line.fs_type:{len_dict['fs_type']}}"
        f"{line.options:{len_dict['options']}}{line.dump:{len_dict['dump']}}"
        f"{line.pass_}\n"
    )

def get_longest(lines: list) -> dict:
    d = {}
    for field in FstabLine._fields:
        if field == 'pass_':
            continue

        d[field] = max(
            [len(getattr(l, field)) for l in lines if isinstance(l, FstabLine)]
        ) + OFFSET

    return d

def parse(content: list) -> list:
    parsed_content = []
    for line in content:
        if line.startswith('#'):
            parsed_content.append(f'# {line.lstrip("#").strip()}\n')
        elif line.startswith('\n'):
            parsed_content.append(line)
        else:
            parsed_content.append(FstabLine(*line.split(maxsplit=6)))

    return parsed_content

def main() -> None:
    with FSTAB.open() as f:
        lines = f.readlines()

    lines = parse(lines)

    len_dict = get_longest(lines)

    with FSTAB.open('w+') as f:
        for line in lines:
            if isinstance(line, FstabLine):
                f.write(format_line(len_dict, line))
            else:
                f.write(line)

if __name__ == '__main__':
    main()

The type hints you're using for containers are too generic .i.e list, dict etc. You could make them more helpful using the types present in typing module.

from typing import Dict, List, Union

def format_line(len_dict: Dict[str, str], line: FstabLine) -> str:
def get_longest(lines: List[str]) -> Dict[str, str]:
def parse(content: List[str]) -> List[Union[str, FstabLine]]:

This could also be done for variables like parsed_content as well because Mypy currently throws error for it:

  error: Argument 1 to "append" of "list" has incompatible type
  "FstabLine"; expected "str"

parsed_content: List[Union[str, FstabLine]] = []


Calculating a table of deBroglie wavelengths for various electron energies Here is the formula for the deBroglie wavelength of an electron versus its kinetic energy:
$$ \lambda(E_k) = h\left/\sqrt{\frac{(E_k+m_eC^2)^2-m_e^2C^4}{C^2}}\right.$$
and here is simple script that prints a CSV with plot data:

#!/usr/bin/python
import csv
from scipy import constants as spc
import numpy as np

vev = np.arange(1,1e6,100);
vWavelength = map( lambda n: \
               spc.h/np.sqrt( (np.power(spc.eV*n+spc.m_e*np.power(spc.c,2),2)-np.power(spc.m_e,2)*np.power(spc.c,4))/np.power(spc.c,2) ), \
               vev );
np.savetxt('plotdata/1a.csv', np.transpose(np.array([vev, vWavelength])));

The vWavelength line is abysmal, in my opinion. How can I make this code better?

First, simplify: factor out \$C^4\$ out of square root:

\$\lambda(E_k) = h\left/C\sqrt{(\frac{E_k}{C^2}+m_e)^2-m_e^2}\right.\$

Simplify even more: factor out \$m_e^2\$:

\$\lambda(E_k) = h\left/C m_e\sqrt{(\frac{E_k}{(C m_e)^2}+1)^2-1}\right.\$

Do not put everything in lambda:

# These are all constants; no need to recompute them.
# Consider putting them into a namespace.
# I didn't think too hard to figure appropriate names; my physics is too rusty

CM = spc.c * spc.me
e1 = spc.ev/(CM * CM)
scale = spc.h / CM

# This is an actual computation.
# Can be converted to lambda, but I strongly advise against.

def deBroglieWaveLength(n):
    arg = e1 * n + 1.0
    return scale / np.sqrt(arg * arg - 1)


Simple counting fair semaphore implementation Here is my attempt to implement a simple counting semaphore in Java. Please share your comments.

/*
 * A simple counting semaphore implementation.
 * Acquire calls block until a resource is available.
 * Supports fairness option which serves Acquire calls
 * in FIFO order.
 */

import java.util.Deque;
import java.util.ArrayDeque;

public class MySemaphore {
   private final int capacity;
   private int usedCount;
   private final boolean isFair;
   private Deque<Long> waitList;

   public MySemaphore(int capacity, boolean isFair) {
      this.capacity = capacity;
      this.isFair = isFair;

      if(isFair) {
         waitList = new ArrayDeque<>();
      }
   }

   public synchronized void acquire() {
      if(isFair)
         waitList.addLast(Thread.currentThread().getId());

      while(!(isResourceAvailable() && isEligibleToAcquire())) {
         try {
            wait();
         } catch(InterruptedException e) {
         }
      }

      usedCount++;

      if(isFair)
         waitList.removeFirst();
   }

   public synchronized void release() {
      if (usedCount > 0) {
         usedCount--;
         notifyAll();
      }
   }

   private boolean isResourceAvailable() {
      return usedCount < capacity;
   }

   private boolean isEligibleToAcquire() {
      if(!isFair)
         return true;

      // True only if current thread is at the head of the queue.
      return Thread.currentThread().getId() == waitList.getFirst();
   }
}

What happens to the intrinsic lock when interrupt is caught in the below block? My understanding is, the thread which catches the exception will still hold the lock. Please clarify.

  while(!(isResourceAvailable() && isEligibleToAcquire())) {
     try {
        wait();
     } catch(InterruptedException e) {
     }
  }

As for your question of what happens to the intrinsic lock when the InterruptedException is thrown: As soon as wait() is called, the thread relinquishes the lock, which is stated in the documentation of Object.wait(). Continuing to hold the lock would defeat the purpose of wait(), which is to wait for some other thread to do something for which it needs the lock and be notified when that thread is finished with the task and no longer needs the lock, so that the current thread can start trying to re-acquire the lock.

This means that, in order to return from wait() and, if needed, execute the catch block, the thread has to acquire the lock again first. Once the thread does have the lock, however, entering the catch block will have no effect on the thread's ownership of the monitor.

I found this question, which, along with the answer given there, might contain what you're interested in.

Update

Here's a test to demonstrate that, even if the catch block is not synchronized, it will still only be executed after the thread has re-acquired the lock, because the thread must first return from wait(), which will only happen when it has the lock it was waiting on.

static void testControlFlow() {
    final Object lock = new Object();

    final Thread threadA = new Thread(new Runnable() {
        @Override
        public void run() {
            try {
                synchronized (lock) {
                    while (!Thread.currentThread().isInterrupted()) {
                        lock.wait();
                        /* Theoretically, it is possible that threadA will
                           be spuriously woken up AND be interrupted by
                           threadB between this wake-up and the next loop
                           iteration. If this unlikely scenario happens,
                           then the catch block will never be executed because
                           no InterruptedException will ever be thrown */
                    }
                }
            } catch (InterruptedException e) {
                System.out.println("threadA has caught an InterruptedException");
            }
        }
    });

    Thread threadB = new Thread(new Runnable() {
        @Override
        public void run() {
            while (threadA.getState() != Thread.State.WAITING) {
                //ensure that threadA acquires the lock before threadB
            }
            synchronized (lock) {
                threadA.interrupt();
                while (threadA.getState() == Thread.State.WAITING) {
                    //wait for threadA to wake up
                }
                System.out.println("threadA has woken up");

                while (threadA.getState() == Thread.State.RUNNABLE) {
                    //see if threadA can catch the exception without holding the lock
                }

                if (threadA.getState() == Thread.State.BLOCKED) {
                    System.out.println("threadA is now blocked and trying to acquire the lock");
                } else {
                    assert threadA.getState() == Thread.State.TERMINATED;
                    System.out.println("threadA has terminated while threadB was still holding the lock");
                }
            }
        }
    });

    threadA.start();
    threadB.start();
}

Output is:

threadA has woken up
threadA is now blocked and trying to acquire the lock
threadA has caught an InterruptedException


I created a sorting algorithm, called Grid Sort Index & Sorting

Index

• Counting: when the index, or the position, is dependent on the item. Count from one. (Zero-based index is mathematically wrong.)

• Grid: when the index, or the position, is independent on the item. (e.g. JavaScript array)

Counting arrays are used in bubble sort, merge sort, quick sort, etc. Because of the nature of the index, sorting techniques using counting arrays require comparing the items and moving the items around.

Grid arrays are better suited for sorting.

Grid Sort

• Assigning the number (or other value associated with the number, such as name) to the corresponding index’s item

• Filtering out the undefined for print, iterate, length, etc.

• Assigning is sorting

• No comparing. No swapping. No moving around items.

• No need for indexOf()

• Stable

• Limitations : whole numbers only, size of an array

• An example using JavaScript

• https://code.sololearn.com/WWuuXR9rO45U/?ref=app

var test = [57, 285, 0, 854, 0, 27, 5, 5, 2885, 497, 57, 16, 803, 653, 62, 905, 975, 367, 147, 99, 58, 207643, 73, 83, 95, 24, 3, 1986, 47, 954, 1, 99, 954, 36, 67, 2, 753, 97, 3689, 79864, 5788, 479, 9865, 8900, 73, 753, 588, 19537, 489964, 30674, 578, 55, 99999, 578, 1000000, 58, 279, 3, 75, 975, 46, 785, 6789, 164, 5538, 744, 85, 2, 579, 753, 5538, 975, 158, 689, 379, 965, 57, 803, 84, 653, 85, 73, 864, 8643, 9047, 864, 783, 7, 4, 932, 5, 80, 97, 79, 849, 7, 3, 5, 10, 0];

document.write(test+”
”);
document.write(“Count: “+test.length+”
”);
document.write(“
”);

var test_original = [];
var x = 0;
while (x < test.length){
    if (test_original[test[x]] === undefined){
        test_original[test[x]] = [];
        test_original[test[x]][0] = test[x];
    }
    else{
        test_original[test[x]].push(test[x]);
    }
    x = x + 1;
}

var test_defined = [];
var defined_from = 0;
var defined_to = test_original.length;
var x = defined_from;
while (x < defined_to){
    if (test_original[x] !== undefined){
        test_defined.push(x);
    }
    x = x + 1;
}

var definedIndex = 0;
while (definedIndex < test_defined.length){
    document.write(definedIndex + 1);
    document.write(“. “);
    document.write(test_defined[definedIndex]);
    document.write(“: “);
    document.write(test_original[test_defined[definedIndex]]);
    document.write(“
”);

    definedIndex = definedIndex + 1;
}

Zero based indexing

Counting: when the index, or the position, is dependent on the item. Count from one. (Zero-based index is mathematically wrong.)

I will disagree. The index does not represent a count but rather it is a position. When we measure something we start at 0. For example a length of 5 units. Unit 0 the 1st unit is at 0 to < 1, and the last unit, the 5th is at 4 to < 5. The set of 5 units does not contain the unit 5.
It is not mathematically wrong to use zero based indexing, what is wrong is using the index to represent a count.
Array V Grid AKA hash table

Grid: when the index, or the position, is independent on the item. (e.g. JavaScript array)

I think you understanding of how arrays work is leading to some bad assumptions.
Arrays
Computers use RAM that is a continuous array of addresses. We use arrays because they are very fast when we need to get or set a item at an index.
To locate an item we get the RAM address of the array and add the index multiplied by the size of each item (JS arrays hold references so all array items are the same size) . In one addition and multiplication (a shift if item size is a power of two) we have the memory address of the data we want.
Almost all modern CPUs have special instructions (indexed addressing) to speed up array use.
Hash table
A grid is a little more complicated.
As the index can be any value we can not just offset a RAM address to find an item. We need to know where the item associated with an index is in RAM. That means that another array is needed to hold all the indexes and RAM addresses.
So if you want the item of grid index 10000000, we need the address of the index 10000000 which must be found by searching the array of indexes one at a time. Then we can get the RAM offset of the item we are after.
However computers have been around for a long time and some cleaver people have come up with ways to make "grids" somewhat faster. In javascript we call them Map
in computer science we use the term hash table.
Hash tables avoid the need to search for the index by applying a hash function to the index (key). This provides an offset that we can use to get the RAM address of the data we are after.
Still slower than an array as the hash function needs to be applied each time we index into the table, its a lot faster than searching all indexes each time we want an item.
When JS arrays are not arrays.
On a side note. JavaScript arrays come in two flavors

Dense arrays, each item is held one after the other in a continuous section of memory.
Sparse arrays. Sparse arrays look from the code perspective like dense array, but under the hood sparse array are actually hash tables.

A dense array is converted to a sparse array if you add an item above the array length. Different engines have different rules but generally its a bad idea to add items to an array at random indexes (as you do in your grid sort)
Review
I am not going to review your code as I think it is moot if you consider the above information.
I will say, you saw some institutionalized flaws and you created a better solution (sadly your assumptions were off). You demonstrate an initiative and a healthy skepticism of the status quo.
Most people will just keep at the same old same old and nothing great ever comes from that.
Keep at it.


Convert a double to a string containing rational and surd First off, code:

string FracSurd(double value)
{
    //A C# 7 local function
    double GCD(double a, double b)
    {
        while (b > 0)
        {
            var rem = a % b;
            a = b;
            b = rem;
        }
        return a;
    }
    for (int i = 0; i <= 1e6; i++)
    {
        var SubjectToTest = value / Math.Sqrt(i);
        for (var denom = 1.0; denom <= 500; denom++)
        {
            var numer = Math.Round(SubjectToTest * denom);
            if (SubjectToTest - numer / denom == 0)
            {
                int Square = 1, a = i;
                for (int b = 2; a > 1; b++)
                    if (a % b == 0)
                    {
                        int x = 0;
                        while (a % b == 0)
                        {
                            a /= b;
                            x++;
                        }
                        //Console.WriteLine("{0} is a prime factor {1} times!", b, x);
                        for (int j = 2; j <= x; j += 2) Square *= b;
                    }
                var LCM = GCD(numer, denom);
                numer /= LCM;
                denom /= LCM;
                var Builder = new System.Text.StringBuilder(numer.ToString());
                Builder.Append(" / ").Append(denom).Append(" √");
                foreach (var Char in (i / Square).ToString())
                {
#if WINDOWS_UWP
                    Builder.Append("̅");
                    Builder.Append(Char);
#else
                    Builder.Append(Char);
                    Builder.Append("̅");
#endif
                }
                return Builder.ToString();
            }
        }
    }
    throw new ArithmeticException("Cannot find appropiate fraction and surd.");
}

The code contains a lot of loops. How can I increase its performance so I can expand its range of input?

This is not a good question because the objective is unrealistic. The code (or perhaps the problem statement) is also flawed since √1 is considered a surd, which - if I am not mistaken - is wrong. (A surd is an "irrational nth root of a positive integer (n > 1)", i.e. it has a non-recurring floating point representation.)

Here is why the objective needs sorting out

Reasonably performant algorithms exist to express floating point numbers as irrational numbers (all of which may be expressed as x / y * √1).  Here's an example (E&OE):

static string Frac(double value)
{
  const double compareTolerance = 1.0E-12;
  bool isNegative = value < 0;
  value = Math.Abs(value);

  long numerator = 1L;
  long denominator = 1L;
  double fraction = (double)numerator / denominator;

  while (Math.Abs(fraction - value) > compareTolerance)
  {
    if (fraction < value)
      numerator++;
    else
    {
      denominator++;
      numerator = (long)(value * denominator);
    }
    fraction = (double)numerator / denominator;
  }
  return $"{numerator} / {denominator} ~= {fraction}, error = {value - fraction}";
}

Given Console.WriteLine(Frac(2.0 / 3 * Math.Pow(7, 1.0/3)));

One gets 1238109 / 970847 ~= 1.27528745518089, error = 7.04103442217274E-13

Reducing the "tolerance" leads to finding bigger (numerator, denominator) pairs giving "increasingly accurate" rational equivalents (within the significant digit limit of the floating point type of course). Taking the floating point representation of any rational, multiplying (or dividing) by a (floating point approximation to a) surd, just gives another floating point number which can be (approximately) represented by a rational. Every floating point number therefore has virtually unlimited rational * surd representations, so you need to specify how you want to limit the search range and/or output possibilities.


Print input lines longer than 80 characters "The C Programming Language" (2nd Edition) by Kernighan and Richie has this exercise:

  Write a program to print all input lines that are longer than 80 characters

I have written the following code for this exercise:

#include <stdio.h>

#define MAXLEN 1000
#define MINLEN 80

int main(void)
{
    int c;
    int index = 0;
    char line[MAXLEN];

    while ((c = getchar()) != EOF)
    {
        if (c != '\n')
        {
            line[index] = c;
            ++index;
        }

        else
        {
            line[index] = '\0';

            if (index >= MINLEN)
            {
                printf("%s\n", line);
            }
            index = 0;
        }
    }

    return 0;
}

This code is shorter than many other suggestions I found on the Internet. The code I wrote seems to be working. Am I missing something, or can I suppose that this code is fine for the exercise?

Just to elaborate on @ratchet freak's answer, what you need is a two-state program.

In state 1, when fewer than 80 characters have been input on a single line, store these characters in a buffer and increment a counter to reflect the number of characters that have been stored. If you receive a newline character in this state, reset the counter and start again.

As soon as this counter reaches 80, print out the contents of this buffer and switch to state 2, where characters are output as soon as they are received. There is no need to continue buffering in this state. If you receive a newline character, go back to state 1.

With this approach, there's no need to store more than 80 characters in memory (plus one byte for the terminating '\0'), and no possibility of a buffer overflow.

#include <stdio.h>
#define MIN_LENGTH 80

int main(void) {
  char buffer[MIN_LENGTH + 1];
  int c, n=0;

  while (1) {

    c = getchar();
    if (c == EOF) break;

    if (n < MIN_LENGTH) {

      /* State 1: Fewer than MIN_LENGTH characters in line */

      if (c == '\n') n = 0;
      else {
        buffer[n++] = c;
        if (n == MIN_LENGTH) {
          buffer[n] = '\0';
          printf("%s",buffer);
        }
      }
    }

    else {

      /* State 2: At least MIN_LENGTH characters in line */

      putchar(c);
      if (c == '\n') n = 0;
    }
  }

  return 0;
}


Get array of pressed buttons using extension method In my game, I need to detect whether any buttons/keys are pressed at all before I try to process any input. This is easy enough for keys since the XNA library provides a GetPressedKeys method on the KeyboardState class:

private void ProcessKeyboard()
{
    keyboardState = Keyboard.GetState( PlayerIndex.One );

    if ( !keyboardState.GetPressedKeys().Any() )
    {
        DoAction( Actions.Idle );
        return;
    }

    foreach ( var binding in keyboardBindings )
    {
        if ( keyboardState.IsKeyDown( binding.Key ) )
        {
            DoAction( binding.Value );
        }
    }
}

However, it doesn't provide a corollary for the GamePadState class:

private void ProcessGamePad()
{
    gamePadState = GamePad.GetState( PlayerIndex.One );

    // detect no input?

    foreach ( var binding in gamePadBindings )
    {
        if ( gamePadState.IsButtonDown( binding.Key ) )
        {
            DoAction( binding.Value );
        }
    }
}

I tried whipping up my own extension method but, as I'm not really familiar with enums or arrays in C#, I'm afraid it's a little sloppy:

public static class GamePadStateExtensions
{
    public static Buttons[] GetPressedButtons( this GamePadState gamePadState )
    {
        // 25 is the number of items in Buttons
        var pressedButtons = new Buttons[25];

        foreach ( Buttons button in Enum.GetValues( typeof(Buttons) ) )
        {
            if ( gamePadState.IsButtonDown( button ) )
            {
                pressedButtons[pressedButtons.Count()] = button;
            }
        }

        return pressedButtons;
    }
}

Would I be better off doing something else with ProcessKeyboard and/or ProcessGamePad? How can I improve GetPressedButtons?

Rather than using an array to return you should use a List<T> which can be easily appended to:

public static List<Buttons> GetPressedButtons( this GamePadState gamePadState )
{
    var pressedButtons = List<Buttons>();

    foreach ( Buttons button in Enum.GetValues( typeof(Buttons) ) )
    {
        if ( gamePadState.IsButtonDown( button ) )
        {
            pressedButtons.Add(button);
        }
    }

    return pressedButtons;
}

However this can be simplified still with the use of LINQ. The for loop basically selects all buttons which are pressed:

public static List<Buttons> GetPressedButtons(this GamePadState gamePadState)
{
    return Enum.GetValues(typeof(Buttons))
               .Cast<Buttons>()
               .Where(b => gamePadState.IsButtonDown(b))
               .ToList();
}

The Cast<T> is required because GetValue returns a non-generic Array.


C# code - SNMP packet processing I have the following code, which does the following:

Reads from the packet from UDP (SNMP) into an array, check if this number is > 127, if yes then '&' this with 127, now pad it with leading zero's to make it 7 bits in total. Combine it with result.
Now check the next one in the packet and repeat the above.
It works for if 3 numbers > 127. Can this code be optimized check for unlimited throughout the length of the array

Please do let me know if you require any further information.

Thank you.

 int[] obj = new int[Objectlength];
 string result = string.Empty;
 for (int cnt = 0; cnt < Objectlength; cnt++)
 {
     obj[cnt] = Convert.ToInt16(packet[objectstart]);
     objectstart++;

     if (obj[cnt] >127)
     {

         int brush = 127;
         int bin = obj[cnt] & brush;
         string binR1 = Convert.ToString(bin, 2).PadLeft(7, '0');
         result = result + binR1;
         cnt++;

         if ((packet[objectstart] > 127) | (packet[objectstart]  < 127))
         {

             Console.WriteLine(packet[objectstart]);
             int bin1 = packet[objectstart] & brush ;
             binR1 = Convert.ToString(bin1, 2).PadLeft(7, '0');
             result = result + binR1;

             obj[cnt] = Convert.ToInt16(packet[objectstart]);
             cnt++;
             objectstart++;

             if ((packet[objectstart] > 127) | (packet[objectstart] < 127))
             {

                 Console.WriteLine(packet[objectstart]);
                 int bin2 = packet[objectstart] & brush;
                 binR1 = Convert.ToString(bin2, 2).PadLeft(7, '0');
                 result = result + binR1;

                 obj[cnt] = Convert.ToInt16(packet[objectstart]);
                 objectstart++;
             }

         }

     }

}

First things first.  Your variable names are not very well thought out.  Even after I decode it, cnt or count?? does not describe what it does.  I think a better name would be currentIndex.

Second, C# standards say to use camelCasing for variable names.  I would also read up on var, it is a very useful keyword for cleaning up code.

Third, brush should be a constant, if you follow my suggestion on the change to recursion, it should be made a class constant.

I also don't like that cnt is incremented outside of the loop.

Now onto the main question.

Just seeing if ((packet[objectstart] > 127) | (packet[objectstart]  < 127)) twice in the same structure (one inside the other one) is a huge code smell to me, especially when it is the same code inside the second, as it is the first.  This looks like a good case for recursion, a method calling itself.

It would go something like this:

private string ProcessSegment(int[] obj, int currentIndex, int objectStart, dynamic[] packet)
{
     if ((packet[objectstart] != 127) || currentIndex > Objectlength)
     {
         return string.Empty;
     }

     Console.WriteLine(packet[objectstart]);

     var bin2 = packet[objectstart] & brush;
     binR1 = Convert.ToString(bin2, 2).PadLeft(7, '0');

     obj[currentIndex] = Convert.ToInt16(packet[objectstart]);

     return binR1 + ProcessSegment(obj, currentIndex++, objectStart++, packet);
}

Your calling code will look something like:

obj[cnt] = Convert.ToInt16(packet[objectstart]);
objectstart++;

if (obj[cnt] >127)
{
    result += ProcessSegment(obj, cnt++, objectstart++, packet);
}

I have no way of testing this, but I think it should work.  Also note, please change the dynamic in the ProcessSegment call to be whatever the packet variable is


Critique & improve a file reading routine ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
(defun read-sim-file (filename)
  "Reads file.
File is presumed to be the output from a simulation.

Returns lines from the simulation as a list of strings

We throw an error if TRAP ET is detected.

Lines with DBG_INT are ignored."
  (let ((file-text-list (split "\\n"
                   (batteries:read-text-file filename)))
    (return_list '()))

    (loop for line in file-text-list do
     (cond ((scan "DBG_INT" line)
        )  ; intentional newline.
           ((scan "TRAP ET" line)
        (error "Trap ET detected"))
           (t
        ;; There's got to be a cleaner method here.
        (setf return_list (append return_list (list line))))))
    return_list))

This code is ugly.
In particular, the (setf X (append X Y)) idiom feels hideously clunky.

Comments on how to make this more beautiful?

Possibly REMOVE-IF?

At first glance, you're looking for loops collect clause.

(defun read-sim-file (filename)
  (let ((file-text-list (split "\\n" (batteries:read-text-file filename))))
    (loop for line in file-text-list
      when (scan "TRAP ET" line)
        do (error "Trap ET detected o_o")
      unless (scan "DBG_INT" line)
        collect line)))

At second glance, you can do this much more efficiently using with-open-file instead of breaking your input file up into a list (this should save you two traversals of said file)

(defun read-sim-file (filename)
  (with-open-file (stream filename)
    (loop for line = (read-line stream nil 'eof) until (eq line 'eof)
      when (scan "TRAP ET" line)
        do (error "Trap ET detected ಠ_ಠ")
      unless (scan "DBG_INT" line)
        collect line)))

The first one is a fairly basic loop directive, the second one is almost verbatum an example out of the CL Cookbook "files" section (the whole cookbook contains many other useful recipes too).

Just as a sidenote, do try to indent things properly. It was more difficult than it should have been to read your initial program. If you ever catch yourself leaving comments like ;; intentional newline, think hard about what you just wrote.

Now that I've had some proper sleep, lets take the new one apart for educational purposes.

  (with-open-file (stream filename)

This line opens up a file and creates a handle for it named stream. Nothing has actually been read yet, you've just got a stream to pull from.

    (loop for line = (read-line stream nil 'eof) until (eq line 'eof)

loop has a couple of non-obvious directives. This is one of them (and coincidentally, the reason that some lispers prefer the non-standard iterate module; there's a feeling that infix notation isn't Lispy enough. I disagree, just like to point it out).

read-line takes a stream and a few options and returns the first line from that stream. (read-line stream nil 'eof) means "Read the next line from stream, don't error at the end of file marker (that's the nil) and return the symbol eof when you reach the end of the file.

The full line then reads "Loop through the lines in stream until you hit 'eof".

      when (scan "TRAP ET" line)
        do (error "Trap ET detected ಠ_ಠ")

loop has conditional directives too, so you don't need to resort to cond here. if, else, when and unless are all valid forms.

      unless (scan "DBG_INT" line)

Speaking of unless, you can use it to skip "DBG_INT" lines without devoting a separate clause to them.

        collect line))

This does the same thing as

     (setf tmp (append return_list (list line)))

which is to say, it creates a list where the elements are collected in order they're encountered (though there isn't an actual named variable that). The loop form also returns whatever you've collected if that's the last clause in the statement (so you don't need to explicitly return it).


Verifying the type of an AggregateException's inner exception in a unit test I've got the following code inside an integration test.

[TestMethod]
public void Integration_GetUser_WithoutLoggingIn_ThrowsInvalidOperationException()
{
    var myApi = new Api(new Session());

    try
    {
        var irrelevant = myApi.GetUserInfoAsync().AsTask().Result;
    }
    catch (AggregateException ae)
    {
        if (ae.InnerException.GetType() == typeof(InvalidOperationException))
        {
            Assert.IsTrue(true);
        }
        else
        {
            Assert.Fail("There was no InvalidOperationException.");
        }
    }
}

Some remarks about it:

I am writing a Windows Runtime Component so the return type has to be IAsyncOperation<T>, Task<T> is not an option.
I am blocking the call using AsTask().Result, is this the most appropriate method?
Exceptions thrown in an asynchronous chain return AggregateException so I want to inspect that it throws the exception which I defined as "you are not logged in".
I'm using MSTest because NUnit can't handle WinRT types (which stinks in combination with integration tests).

I am blocking the call using AsTask().Result, is this the most appropriate method?

Can't you use await? Most testing frameworks (apparently including MSTest) support async tests. That way, you could work directly with the inner exception instead of the AggregateException.

if (ae.InnerException.GetType() == typeof(InvalidOperationException))
{
    Assert.IsTrue(true);
}
else
{
    Assert.Fail("There was no InvalidOperationException.");
}

I think this should be shortened to just:

Assert.AreEqual(typeof(InvalidOperationException), ae.InnerException.GetType());

Or maybe:

Assert.AreEqual(
    typeof(InvalidOperationException), ae.InnerException.GetType(),
    "There was no InvalidOperationException.");

  Exceptions thrown in an asynchronous chain return AggregateException so I want to inspect that it throws the exception which I defined as "you are not logged in".

But you're not doing that, you're just checking that it's InvalidOperationException. If you want to test for this, maybe you should use a custom exception type?


Simple Python plugin loader I have written a simple system to load plugins from files in Python 2.7, and I'd like to see if any of it can be improved upon.

vedo.py

from __future__ import absolute_import

import os.path
import inspect

from vedo.event_emitter import EventEmitter
from vedo import VedoPlugin

def _get_module_name(directory, filename):
    return '{0}.{1}'.format(os.path.basename(directory), filename)

def _is_plugin_class(plugin_class):
    return (inspect.isclass(plugin_class) and
            issubclass(plugin_class, VedoPlugin) and
            plugin_class != VedoPlugin)

class Vedo(EventEmitter):
    def __init__(self, plugin_directory):
        self._plugin_directory = plugin_directory
        self._plugins = []

    @property
    def plugin_directory(self):
        return self._plugin_directory

    @property
    def plugins(self):
        return self._plugins

    def load_module(self, module):
        plugin_classes = [c for c in module.__dict__.values() if
                          _is_plugin_class(c)]

        for plugin_class in plugin_classes:
            self.plugins.append(plugin_class(self))

    def load_file(self, name):
        try:
            plugin_module = _get_module_name(self._plugin_directory, name)
            namespace = __import__(plugin_module)
        except ImportError:
            return

        self.load_module(getattr(namespace, name))

Each plugin must inherit from VedoPlugin, and is loaded by the Vedo class.

Here's an example plugin:

plugins/first.py

class MyFirstPlugin(VedoPlugin):
    def __init__(self, vedo):
        super(MyFirstPlugin, self).__init__(vedo)

And a quick example of how to use Vedo:

vedo = Vedo("./plugins")
vedo.load_file("first")

vedo.plugins[0] # This is an instance of `MyFirstPlugin'

Here's some things I'm not too fond of, but I'm not sure what I can do about them:

Should the private functions in the top of vedo.py be moved inside the Vedo class?
Should load_module be private? It's only really going to be used when loading files.
Is there a way to get around the ugliness that is the plugin __init__ function? Every single plugin will need to have that constructor, which I'm not too happy about.

Regarding your questions:

No, I think the functions as they are are fine, not everything needs
to be moved into a class.
If it's an implementation detail, sure.
AFAIK no, but if it's otherwise empty the method doesn't have to be
added.

Also:

In load_module the loop can be more compact by using a combination
of list.extend and filter.

def load_module(self, module):
    self.plugins.extend(filter(_is_plugin_class,
                            module.__dict__.values()))

The numbered arguments for str.format could be left out.

Looks good otherwise.  If you're really concerned with keeping the
internals safe you could also not expose the plugins list directly,
but provide a separate accessor instead.