Send Response To Browser without DB save confirmation Just reviewing my own code and noticed that I'm saving info in the DB and WAITING for that to respond before I send info to the browser, although I'm wondering if it would be more performant to not wait...

For example, let's update a user's email address.

Currently, I get a user, update their info, save and wait for confirmation, then send the browser the data:

UserModel.findById(user._id)
  .then(function(user){
    user.email = 'newemail@google.com';
    return user.save();
  })
  .then(function(user){
    res.send(user.email)
  })

But the user data is already edited, and I already have it, so can't I send the browser the data and save to MongoDB at the same time...?

UserModel.findById(user._id)
  .then(function(user){
    user.email = 'newemail@google.com';
    user.save();
    res.send(user.email)
  })

I guess, now that I'm typing it out, the only issue (I can imagine) would be if there was an error with the saving operation at the DB level... How often does that even happen? It's really only a matter of milliseconds, but just wondering if anyone has done this or can share a horror store of why I definitely should not do this.

The big question to ask is: what happens when it doesn’t work? (For whatever reason)

In your case, the user will probably think it worked, only to find out later that it didn’t and will most likely think “stupid app, I just changed this”. You may be okay with it. I wouldn’t. The people paying me probably wouldn’t either.

And what a blessed career you’ve had as a developer if you believe a DB operation failing is such a very rare occurrence. Specially when deployed to production.


Is this an ugly/incorrect usage of subscriptions? Little backstory: I'm trying to write code that will read messages from INetworkConnection that the INetworkConnection publishes and then send those up to the NetworkController which should do some processing and then send that off to an injected IObserver<IMessage> (which is a single observer because there's a global listener interested in network messages).

In other words, I'm trying to read messages, transform them, and then push them off to some listener. The reason why the controller is not subscribable is because there is only meant to be one listener (which will then distribute messages as necessary).

Now, right now I haven't added the transformation, so all the network controller does is that it pipes the messages from every connection into the injected subscriber like so:

    private readonly IDisposable _networkConnectionStreamSubscription;
    private readonly IObserver<INetworkMessage> _networkMessageStream;

    public NetworkController(IObservable<INetworkConnection> networkConnectionStream,
        IObserver<INetworkMessage> networkMessageStream)
    {
        _networkConnectionStreamSubscription = networkConnectionStream.Subscribe(
            x => LinkWith(x),
            () => Close());
        _networkMessageStream = networkMessageStream;
    }

    /// <summary>
    /// Link the Controller with the given Connection, allowing the network
    /// controller to start listening for network messages.
    /// </summary>
    /// <param name="networkConnection"></param>
    public void LinkWith(INetworkConnection networkConnection)
    {
        // Subscribe to the network message, and pipe the messages that have been parsed
        // out through our network message stream.
        // TODO: Do something with the subscription disposable
        networkConnection.Subscribe(
            x => _networkMessageStream.OnNext(x),
            exception => _NetworkMessageError(networkConnection, exception),
            () => _NetworkConnectionClosed(networkConnection)
        );
    }

My question is, simply speaking, am I

going the wrong way by thinking of Rx as streams of T and
is this considered an anti pattern/bad approach to what I am trying to do?

I don't think your approach is fundamentally wrong: Rx is all about exposing and responding to streams of data. I do wonder though if any of this is necessary; doesn't the Observable.Select method do essentially what you are trying to do?


Calculating luma of a rect in an image of .yuv format I'm trying to get the average luma of an area in an YUV image in the Android camera. I have come up with the following code which I think is correct and produces consistent results.

private static final int AVERAGE_LUMA_SAMPLE_SIZE = 2;

// sample the average luma
// yuvImage is the byte data return from a camera's previewFrame
public static int getAverageLuma(byte[] yuvImage, int imageWidth, int imageHeight, RectF sampleArea) {
    int totalLuma = 0;
    int totalSampled = 0;

    int top = (int)Math.max(sampleArea.top, 0);
    int left = (int)Math.max(sampleArea.left, 0);
    int right = (int)Math.min(sampleArea.right, imageWidth);
    int bottom = (int)Math.min(sampleArea.bottom, imageHeight);

    for(int i = top; i < bottom; i += AVERAGE_LUMA_SAMPLE_SIZE) {
        int rowStartPosition = (i * imageWidth) + left;

        for (int j = 0; j < right; j += AVERAGE_LUMA_SAMPLE_SIZE) {
            int bytePostion = rowStartPosition + j;
            int y = (0xff & ((int) yuvImage[bytePostion])) - 16;

            totalLuma += Math.max(y, 0);
            totalSampled++;
        }
    }

    return Math.round((float)totalLuma / totalSampled);
}

One of the reasons that large 2d data sets are often reduced to a single 1D array is so that you can process the data in a single loop and the memory is contiguous. The trick to processing this data efficiently in a single loop is to be able to fiddle the single pointer in a way that makes sense...... and to do that, we need some smart pointer skips...

Consider the following single loop. It starts at the right place, and then skips the areas outside the sample rectangle....

int startpos = top * imageWidth + left;
int lastpos = bottom * imageWidth + right;
// skip is the amount of unsampled data from the right-side of the sample
// rectangle to the left side of the next row.
int skip = imageWidth - right + left;

// limit is a moving limit that we use to break-process the samples.
// limit only applies if we actually need to skip data.
int limit = skip == 0 ? lastpos + 1 : (top * imageWidth) + right;
for (int i = startpos; i <= lastpos; i+= AVERAGE_LUMA_SAMPLE_SIZE ) {
    if (i > limit) {
        // take the loop-increment in to consideration.
        i += skip - AVERAGE_LUMA_SAMPLE_SIZE;
        limit += imageWidth;
    } else {

        // Do the sampling work here.....

    }
}

Normally I am against updating a loop-counter index inside the loop, but there are times when it makes sense. This is one of those times.

Note, the skip variable is being calculated naively in this code. It should be smarter about taking the sample-size into consideration, and making sure it hits the beginning of the sample rectangle properly on the next line.... as it stands, if the rectangle is an odd-size, it may cause the start position of the next row to not be on the rectangle boundary. On average, I don't think this will make a difference, but that is your call.


Check Digit Calculator & GTIN-8 Validator This program calculates the 7th digit of a GTIN-8 barcode and calculates the validity of an 8 digit GTIN-8 barcode. Could I simplify my code any further?

import math

def roundup(x):
    return int(math.ceil(x / 10.0)) * 10

def doMaths(number):
     count = 0
     total = 0
     for n in number:
          n = int(n)
          if count %2 == 0:
               total += n * 3
          else:
               total += n
          count += 1
     if len(number) == 8:
          if total %10 == 0:
               print("Valid")
          else:
               print("Invalid")
     else:
          print(number + str(roundup(total)  - total))

while True:
     try:
          number = input("Enter a 7 or 8 digit number for testing: ")
          i = int(number)
          if len(number) == 7 or len(number) == 8:
               doMaths(number)
               break
          else:
               print("Length of number must be 7 or 8")
               continue
     except ValueError:
          print("Only enter numbers.")
          continue

The doMaths function has no clear purpose — hence its weird name.  It acts as both to calculate the expected last digit of a 7-digit input and to validate the last digit of an 8-digit input.  Furthermore, it prints its results instead of returning them, hindering reuse of the function.

What you really want to do is share the code that calculates total.  You could write a function like this, using slices:

def gtin8_checksum_digit(number):
    """
    Given a 7- or 8-digit string, calculate the expected GTIN-8 checksum.

    >>> gtin8_checksum_digit('3141592')
    '7'
    >>> gtin8_checksum_digit('31415927')
    '7'
    """
    total = sum(3 * int(d) for d in number[0:7:2]) + \
            sum(int(d) for d in number[1:6:2])
    return str(10 - (total % 10))

No floating-point arithmetic should be necessary.

Note the docstring, and in particular, the doctests that make it clear how the function behaves.

To use that function…

if __name__ == '__main__':
    while True:
        try:
            number = input("Enter a 7 or 8 digit number for testing: ")
            int(number)    # Trigger possible ValueError
            if 7 <= len(number) <= 8: break
            print("Length of number must be 7 or 8")
        except ValueError:
            print("Only enter numbers.")
    if len(number) == 8:
        print("Valid" if gtin8_checksum_digit(number) == number[7] else "Invalid")
    else:
        print(gtin8_checksum_digit(number))


Database classes for job offers I've recently started using PHP OOP, and I'm now working on an application where different company representatives can create job offers that candidates can look at.

I have a variety of objects for most entities such as Company, Candidate, Offer.

The problem that I have is that these objects all need to be stored in a database, and I need methods to retrieve the information, create the objects and return them. Since it does not make sense - at least to me with my limited experience in OOP - for an object to have access to the database and be able to fetch itself, I'm using a Database class for each of my main objects that are stored in the database : CompanyDatabase, CandidateDatabase, OfferDatabase.

Each of the aforementionned classes have static methods that retrieve one or multiple "objects" from the database - this is a three-step process, they fetch the data, create the objects and return them.

Here's an example of such a xDatabase class (where x  is the name of the class)

class CompanyDatabase{

    public static function getFromID($id)
    {
         $company = Database::query("SELECT * FROM companies WHERE id = ?", [$id]);
         return new Company($company->id, $company->name);
    }

    public static function save($company)
    {
        Database::update("UPDATE compagnies SET name = ? WHERE id = ?", [$company->name, $company-id]);
    }
}

And the Company class in this case :

class Company{

   public $id;
   public $name;

   public function __construct($id, $name)
   {
       $this->id = $id;
       $this->name = $name;
   }
}

I've excluded some code, like the validation to check wether or not data was fetched from the database, because I felt it wasn't relevant to my concerns here.

Is this the proper way of doing it? I end up with two seperate classes for just about every object type that I have, and I'm wondering if I'm doing something wrong and should rather keep everything in one class.

Using DAOs

  The problem that I have is that these objects all need to be stored in a database, and I need methods to retrieve the information, create the objects and return them. Since it does not make sense - at least to me with my limited experience in OOP - for an object to have access to the database and be able to fetch itself, I'm using a Database class for each of my main objects that are stored in the database : CompanyDatabase, CandidateDatabase, OfferDatabase.

You are right that it would not make sense for an object to have access to the database and fetch itself, and you have more or less the right approach.
The classes that perform the database operations are commonly called "DAO" -- data access object. (I recommend to rename those classes to CompanyDao, CandidateDao, OfferDao.)

It's not common to make the data accessor methods static.
If you use DAO instances instead of static method references,
then you get a very flexible design where you could change the backend of your operations by simply passing a DAO instance that implements the same interface, but instead of a database, it transfers data from a web service, or a csv file,
or via radio transmissions from the moon.

Class design

Avoid public fields in classes, like $id and $name in Company.
Fields are an implementation detail of a class,
and should not be easily manipulable from outside,
but strictly through dedicated accessor methods.

Working with SQL

Avoid SELECT *.
It's recommended to name the columns you need explicitly,
for two reasons:

Prevent wasted bandwidth between your program and database fetching unnecessary columns
Make sure you receive the columns in the right order, regardless of their ordering in the table

The SELECT query is from a table called companies,
but the UPDATE query is into a table called compagnies.
I suspect one of them is a typo.
In any case,
some frameworks recommend naming tables with singular words,
for example in this case I'd go for "company" instead.


Project Euler #15 in haskell I've solved Project Euler #15 in Haskell, but I feel like there might be a better way to express my solution that I'm not seeing. I start out defining my types:

import qualified Data.Map.Strict as Map
import Data.List

data Position = Position Int Int
                deriving (Show, Eq, Ord)

start :: Position
start = Position 20 20

I probably should have used newtype on a tuple of Ints, but I think this is ok.  I can then generate the list of moves available from a given position via:

moves :: Position -> [Position]
moves (Position x y) | x == 0 && y == 0 = []
                     | x == 0 = [Position x (y - 1)]
                     | y == 0 = [Position (x - 1) y]
                     | otherwise = Position (x - 1) y : Position x (y - 1) : []

From there a naive implementation to solve for the number of paths would simply look like:

naiveWays :: Position -> Int
naiveWays (Position 0 0) = 1
naiveWays pos = sum $ map naiveWays $ moves pos

Of course this is highly inefficient, and takes much to long to complete, so I tried to improve it using memoization. This is where I feel like I could probably use the most improvement:

ways :: Position -> Int
ways = snd . memWays Map.empty

memWays :: Map.Map Position Int -> Position -> (Map.Map Position Int, Int)
memWays mem (Position 0 0) = (mem, 1)
memWays mem pos = loop lookupWays
  where
    lookupWays = Map.lookup pos mem
    computeWays = mapAccumR memWays mem (moves pos)
    mapWays = fst computeWays
    sumWays = sum $ snd computeWays
    loop (Just w) = (mem, w)
    loop (Nothing) = (Map.insert pos sumWays mapWays, sumWays)

Basically memWays takes a Map of known solutions for the subproblems (number of paths from 2,2, etc), and a position and attempts to use it to find the shortest path. If the position is already in the map, it can simply look it up. Otherwise, it maps over the available moves, using the returned Map as an accumulator.

There actually is an even more efficient way to solve it, but I won't spoil that, and since I'm looking for Haskell practice, not math, I'm more interested in figuring out how I can make this approach cleaner.

Edit: Thanks to 200_success for pointing out that I don't need to enumerate moves.  memWays now looks like:

memWays :: Map.Map Position Int -> Position -> (Map.Map Position Int, Int)
memWays mem (Position 0 _) = (mem, 1)
memWays mem (Position _ 0) = (mem, 1)
memWays mem pos@(Position x y) = loop $ Map.lookup pos mem
  where right = memWays mem $ Position (x - 1) y
        down = memWays (fst right) $ Position x (y - 1)
        both = snd right + snd down
        loop (Just paths) = (mem, paths)
        loop Nothing = (Map.insert pos both $ fst down, both)

I feel like this looks a lot cleaner, though if anyone has suggestions for further improvement, I'd love to hear it.

There's no need to list all the moves.  Here is an unmemoized solution:

naiveWays :: Position -> Int
naiveWays (Position 0 _) = 1
naiveWays (Position _ 0) = 1
naiveWays (Position x y) = (naiveWays (Position (x - 1) y)) +
                           (naiveWays (Position x (y - 1)))

Note that it also takes better advantage of pattern matching.


Retrieving user details from a custom form I have been working on a Magento module for sometime now (not because it is a large module, it's my first and I really struggled with it). I now have it working, which I was really pleased with at first, but now I would like to improve it by increasing the reusability of the code.

While making my module I have looked at other modules to learn from them and have seen that many actions are only a line or two and I have tried keeping in with the skinny controller approach but failed, my example is as follows:

I have a function that get a users details that they have inputted into a custom form.

protected function _setPostData()
{
    $this->_salutation = $this->getRequest()->getPost('salutation');
    $this->_f_name = $this->getRequest()->getPost('f_name');
    $this->_l_name = $this->getRequest()->getPost('l_name');
    $this->_email = $this->getRequest()->getPost('email');
    $this->_emailAddressCheck = $this->getRequest()->getPost('emailAddressCheck');
    $this->_gender = $this->getRequest()->getPost('gender');
    $this->_country = $this->getRequest()->getPost('country');
    $this->_pref_lang = $this->getRequest()->getPost('pref_lang');
}

I feel that this is not the correct way to do it and that there is a better way of achieving the same goal. As you can see, this function gets the posts data and assigns it to attributes that I've set at the start of the class. I have several other examples that are very similar to the above and if someone could please offer some guidance on this one I am sure I will be able to work out the others.

This example is held within the index action. Should I put it in a helper? Once it's created correctly, will there be a few occasions in which I will be able to use it again.

First, I would apply self-encapsulation by changing:

$this->_salutation = $this->getRequest()->getPost('salutation');

to:

$this->set_salutation( $this->get_parameter( "salutation" ) );

Since PHP can call functions using a string, it can be simplified further:

function set( $p ) {
  $f = '$this->set_' . $p;
  $f( $this->get_parameter( $p ) );
}

function get_parameter( $p ) {
  return $this->get_request()->get_post( $p );
}

Then:

$this->set( "salutation" );
$this->set( "first_name" );
$this->set( "last_name" );
$this->set( "email" );
// ... etc.

You can take that one step further by iterating over all HTML FORM variables, which removes the repetition altogether:

foreach( $this->get_form_parameters() as $param_name => $param_value ) {
  $this->set( $param_name );
}

However, that could lead to attempts on making arbitrary function calls by the client, so you'd have to be careful.

What I like about removing the repetition is that it sparks of being consistent with the DRY principle. The HTML form has the list of variables that need to be set -- now the developer need only change the HTML form variable list to change what variables are set by the object. This leads to a single source for the variable list.


Skill system optimization This code feels long and clunky to me. How can I make it shorter, if possible?

I am writing for a libGDX game, and I need a skill algorithm.
I want it so once the player picks up something (which I already have checked for) I set a, b or c to a certain SkillType, the enum down below. Then, I want to get the value of the specific SkillType, by using the enum's methods to get cooldown and cost.

public SkillType[] skills;
public SkillType a, b, c;
public int acost, bcost, ccost;
public int acooldown, bcooldown, ccooldown;

skills = new SkillType[3];
skills[0] = a; skills[1] = b; skills[2] = c;
acost = a.getCost(); bcost = b.getCost(); ccost = c.getCost();
acooldown = a.getCooldown(); bcooldown = b.getCooldown(); ccooldown = c.getCooldown();

public SkillType getSkill(int index) { return skills[index]; }
public void setSkill(int index, SkillType skill) { skills[index] = skill; }

This code declares the enum used above.  It has all the skills that I want to use.  It declares, inside the SkillType, its cost and cooldown.  I do not want to change these values, and that's why I don't have setters.

public static enum SkillType {
        ROAR(1,3),
        DUST_CLOAK(2,10),
        THORN_CLOAK(2,10),
        SPARK(2,5),
        FLUTTER(0,0),
        EMBER(2,2),
        GLOW(0,0),
        SCRATCH(0,0),
        BITE(0,0);

        int cost;
        int cooldown;
        SkillType(int cost, int cooldown) {
                this.cost = cost;
                this.cooldown = cooldown;
        }
        public int getCooldown() { return cooldown; }
        public int getCost() { return cost; }
}

EDIT:

I have updated the enum class to include more items.

public class SkillData {

public SkillType[] skills;
public static final int MAX_SKILLS = 3;
public static final int SLOT_PRIMARY = 0;
public static final int SLOT_SECONDARY = 1;
public static final int SLOT_SECONDARY_TWO = 2;

public static enum SkillType {
    ROAR(1,3),
    DUST_CLOAK(2,10),
    THORN_CLOAK(2,10),
    SPARK(2,5),
    FLUTTER(0,0),
    EMBER(2,2),
    GLOW(0,0),
    SCRATCH(0,0),
    BITE(0,0);

    int cost;
    int cooldown;
    SkillType(int cost, int cooldown) {
        this.cost = cost;
        this.cooldown = cooldown;
    }
    public int getCooldown() { return cooldown; }
    public int getCost() { return cost; }
}

public SkillData() {
    skills = new SkillType[MAX_SKILLS];
}

public int getCost(int index) { return skills[index].getCost(); }
public int getCooldown(int index) { return skills[index].getCost(); }
public void setSkill(int index, SkillType skill) { skills[index] = skill; }
public SkillType getSkill(int index) { return skills[index]; }
public SkillType[] getSkills() { return skills; }

}

skills = new SkillType[2];
skills[0] = a; skills[1] = b; skills[2] = c;

This is simply wrong.  You make an array of size 2 and then put three things in it.

Also, you never set a, b, or c, so you might as well leave off the second line.

public int acost, bcost, ccost;
public int acooldown, bcooldown, ccooldown;

These are never read, so you could delete them.

In general, this code feels incomplete.  Your enum looks about as short as it can be for the functionality.  The rest of the code is unclear about what it is doing.  You could probably delete almost all of it.


Utility Method to find the Square root of a number Utility to calculate the square root of a number. The method also accept an epsilon value, which controls the precision. The epsilon value could range to any number including zero. I am expecting a general code review or if there is a better way to write the code.

/**
 * The class MathUtil contains methods for performing basic numeric operations
    such as the squareRoot functions. Square root of 2.0: 1.4142135623746899
 */
public class MathUtil {

    /**
     * Returns the correctly rounded positive square root of a double value.
     * Special cases: If the argument is NaN or less than zero, then the result
     * is NaN. If the argument is positive zero or negative zero, then the
     * result is the same as the argument. Otherwise, the result is the double
     * value closest to the epsilon.
     *
     * Function takes a non-negative real number             *
     * @param a double
     * @param epsilon double
     * @return double
     */
    public static double squareRoot(final double a, final double epsilon) {
        if (a == 0)
            return a;
        else
            return internalSqrRoot(a, a / 2, epsilon);

    }

    /**
     * Method internalSqrRoot.
     *
     * @param a double
     * @param x double
     * @param epsilon double
     * @return double
     */
    public static double internalSqrRoot(final double a, final double x,
            final double epsilon) {
        if (closeEnough(a, x, epsilon)) {
            return x;
        } else {
            return internalSqrRoot(a, betterGuess(a, x), epsilon);
        }

    }

    /*
     * return true if the current guess is close enought o accepted value

     * Method closeEnough.
     *
     * @param a double
     * @param x double
     * @param epsilon double
     * @return boolean
     */
    public static boolean closeEnough(double a, double x, double epsilon) {

        return (Math.abs(x - ((a / x) + x) / 2)) <= epsilon;

    }

    /*
     * perform a simple manipulation to get a better guess (one closer to the
     * actual square root) by averaging y with x/y

     * Method betterGuess.
     * @param a double
     * @param x double
     * @return double
     */
    public static double betterGuess(double a, double x) {
        return ((a / x) + x) / 2;
    }
}

Duplicated logic

Avoid duplicated logic like this:

public static boolean closeEnough(double a, double x, double epsilon) {
    return (Math.abs(x - ((a / x) + x) / 2)) <= epsilon;
}

public static double betterGuess(double a, double x) {
    return ((a / x) + x) / 2;
}

The closeEnough method includes the exact same logic as betterGuess.
Make it a habit to look at duplicated code fragments with suspicion.

If you eliminate the duplication, the code becomes:

public static boolean closeEnough(double a, double x, double epsilon) {
    return (Math.abs(x - betterGuess(a, x))) <= epsilon;
}

public static double betterGuess(double a, double x) {
    return ((a / x) + x) / 2;
}

... but then, does this actually make sense? Calling betterGuess from closeEnough? Not really. How can you check if a candidate square root is close enough? You cannot compare with the real square root, because that's the target unknown. What you can compare with, is the original number, which you should be able to get by squaring:

public static boolean closeEnough(double a, double x, double epsilon) {
    return Math.abs(a - x * x) <= epsilon;
}

This implementation is more logical, and has another positive side effect:
it makes it possible to clean up the squareRoot method with the suspicious 0-check:

public static double squareRoot(final double a, final double epsilon) {
    if (a == 0)
        return a;
    else
        return internalSqrRoot(a, a / 2, epsilon);
}

The method can become simply:

public static double squareRoot(final double a, final double epsilon) {
    return internalSqrRoot(a, a / 2, epsilon);
}

It would seem the 0-check was there to prevent a division by zero problem in Math.abs(x - ((a / x) + x) / 2))

Method visibility

The MathUtil class name suggests it's a utility class,
but the public MathUtil.betterGuess method (to name just one) doesn't seem very useful.
It's an implementation detail that should be hidden, so make it private.
Question the other methods too, and change their visibility as appropriate.

The variable names are quite poor.
For example in closeEnough it's impossible to tell if the target number is a or x.
target and candidate might have been better names.

Input validation

More than just commenting "Function takes a non-negative real number ",
it would be better to enforce that by throwing an IllegalArgumentException.

Poor JavaDoc

This kind of JavaDoc is worse than no JavaDoc at all:

/**
 * Method internalSqrRoot.
 *
 * @param a double
 * @param x double
 * @param epsilon double
 * @return double
 */

It's worse, because tells nothing new about the method,
but since it's there, I've read it, in vain.

Minor things

It's recommended to use braces even with single-line if statements
Redundant parentheses in the expression (Math.abs(x - ((a / x) + x) / 2))
Perhaps instead of internalSqrRoot, squareRootHelper might be a better name. Or, since the method has a different signature than squareRoot, it can just as well be squareRoot (overloading)


Stitching Linq with if blocks I recently learned about Linq's deferred execution and a little clearer understanding of IEnumerable. I came back to some code that I had written and decided to change with Linq extension methods. (The method is a helper function for some custom Excel functions that I use.)

Overall the code kind of feels wrong readability-wise having to tack on values = values.exension like that. When I step through the debugger it seems to be behaving like I expect.

Is there a more readable way to express this function with or without linq extension methods? I fear I'm making things less readable and possibly micro-optimizing.

private static string Concatenate(IEnumerable<string> values
    , string between, string begin, string end
    , bool excludeDuplicates, bool includeBlanks)
{
    if (!includeBlanks)
        values = values.Where(v => v != string.Empty);

    if (excludeDuplicates)
        values = values.Distinct();

    values = values.Select(v => begin + v + end);

    return string.Join(between, values);
}

Here is what I previously used in case it is of interest.

private static string Concatenate(IEnumerable<string> Values, string BetweenString
    , string BeginString, string EndString, bool ExcludeDuplicates, bool IncludeBlanks)
{
    IEnumerable<string> IncludedValues = (
        from Value in Values
        where IncludeBlanks || Value.Length > 0
        select BeginString + Value + EndString
        );

    IEnumerable<string> StringValues;

    if (ExcludeDuplicates)
        StringValues = new HashSet<string>(IncludedValues);
    else
        StringValues = new List<string>(IncludedValues);

    return string.Join(BetweenString, StringValues);
}

EDIT:

To give a little more context for this function (I'll hedge a little here and say that I haven't changed these functions to be more readable yet, so I beg forgiveness now. :) )

The purpose of this function is that it gets used by two other functions RA.VCONCATENATE and RA.HCONCATENATE that will be Excel functions (using ExcelDNA). (I do plan on changing this to be more readable as well and make Values IEnumerable instead of IList). Note: I can't have optional "missing" parameters in Excel or overloading so I made extension methods for string, int, double and bool that convert unspecified parameters to a default.

[ExcelFunction(Category = "Extra Range Functions", Name = "RA.VCONCATENATE",
    Description = "Vertically Concatenates all values in the Range starting from Top to Bottom, then Left to Right.")]
public static string VerticalConcatenate(object[,] Range, object BetweenString, object BeginString, object EndString, object ExcludeDuplicates, object IncludeBlanks)
{

    List<string> Values = RangeValuesToList(Range, RangeEnumerateFirstDirection.Down); //Empty cells replaced with String.Empty
    string sBetweenString = BetweenString.Optional("");
    string sBeginString = BeginString.Optional("");
    string sEndString = EndString.Optional("");
    bool bExcludeDuplicates = ExcludeDuplicates.Optional(false);
    bool bIncludeBlanks = IncludeBlanks.Optional(false);

    return Concatenate(Values, sBetweenString, sBeginString, sEndString, bExcludeDuplicates, bIncludeBlanks);
}

RA.HCONCATENATE is the same except is uses RangeEnumerateFirstDirection.Right. The expected output of =RA.VCONCATENATE(A1:A5, ",", "^", "$", TRUE, FALSE) (if Range [A1:B3] values are { 1, 2, 2 }, {3, EMPTY, EMPTY} ) is: "^1$,^2$,^3$". RA.HCONCATENATE output is: "^1$,^3$,^2$".

It's not bad already (save for the weird formatting of parameters, what's with commas at the beginning of the line? : ) But that's a matter of taste, and beyond the point).

You could return to the trick you used in your previous implementation to avoid breaking the chain and get rid of value reassignments by pushing includeBlanks into the predicate, like so:

values = values.Where(v => includeBlanks || v != string.Empty);

We've cut on verbosity, but it's debatable to me whether it improves readability.

It's harder to achieve the same thing for Distinct, although you could pull it through if you added a helper extension:

private static IEnumerable<T> If(this IEnumerable<T> enumerable, bool condition, IEnumerable<T> ifMet)
{
    return condition ? ifMet : enumerable;
}

Then you could turn all of it into a single statement:

return String.Join(
    between,
    values
        .Where(v => includeBlanks || v != string.Empty)
        .If(excludeDuplicates, values.Distinct())
        .Select(v => begin + v + end);

Is it worth it, or over-engineered? This is subjective and a matter of taste. Some find verbosity more readable, others prefer terseness/expressiveness.

I would let both solutions slide in a peer review, although creating a single-use extension method (If) looks like a minor code smell.

Another way to avoid repetitive overwriting of values, which you're uncomfortable with, would be to introduce temporary variables:

var blanksHandled = includeBlanks ? values : values.Where(v => v != String.Empty);
var duplicatesHandled = excludeDuplicates ? blanksHandled.Distinct() : blanksHandled;
var withAffixes = duplicatesHandled.Select(v => begin + v + end);

We're not overwriting values now, but when I look at it, I feel it sacrificed too much readability just for that, and what's worse, it got error-prone in process: you now have to remember to always use the last variable so as not to skip a step.

If another parameter was to be added, there's a risk of a bug slipping in:

var blanksHandled = includeBlanks ? values : values.Where(v => v != String.Empty);
var duplicatesHandled = excludeDuplicates ? blanksHandled.Distinct() : blanksHandled;
// oops! will everyone spot right away what went wrong here?
var dirtyWordsHandled = safeMode ? blanksHandled.Except(dirtyWords) : blanksHandled;
var withAffixes = duplicatesHandled.Select(v => begin + v + end);

The last trick I can think of would be to convert these conditionals into Func<IEnumerable<T>, IEnumerable<T>> instances, and pass them to the LINQ chain.

I haven't got a C# IDE on this computer, so this code could be syntactically incorrect, but you get the idea:

Func<IEnumerable<String>, IEnumerable<String>> handleDuplicates = (entries) => excludeDuplicates ? entries.Distinct() : entries;
// etc.

And use them as selectors:

values
   .Select(handleBlanks)
   .Select(handleDuplicates)
   // ...

Again, is it worth it? Sometimes the clever thing is not to be too clever, and keeping it simple beats trying to make it pretty.


Switch-Case pattern for non-constant types Edit: Following answers, I modified my code, and wrote an other question.

History

Recently, I (stupidly) tried to make a switch on myObj.GetType(). Of course, it didn't work. Then I made an ugly (this is subjective) if ... else if ... else if ... else ... list.
I ask myself if I couldn't make something to keep the switch-case architecture, and then I started experienced things.

Expected

To keep the structure of a real switch-case, I want to use it in this way:

Switch(myInt)
    .Case(1, EqualsOneMethod)
    .Case(2, EqualsTwoMethod)
    .CaseWhen(i => i <= 0, NullOrNegativeMethod)
    .Default(DefaultMethod);

static void EqualsOneMethod() { ... }
static void EqualsTwoMethod() { ... }
static void NullOrNegativeMethod() { ... }
static void DefaultMethod() { ... }

Code

Everything starts with this interface:

public interface ISwitchCase<T>
{
    ISwitchCase<T> Case(T value, Action action);
    ISwitchCase<T> Case(T value, IEqualityComparer<T> comparer, Action action);
    ISwitchCase<T> CaseWhen(Predicate<T> predicate, Action action);
    void Default(Action action);
}

The normal implementation is this one:

internal class SwitchCase<T> : ISwitchCase<T>
{
    private T Value { get; }

    public SwitchCase(T value) { Value = value; }

    public ISwitchCase<T> Case(T value, Action action)
    {
        if (Value is IEquatable<T>)
            return CaseWhen(t => ((IEquatable<T>)t).Equals(value), action);

        if (value is IEquatable<T>)
            return CaseWhen(t => ((IEquatable<T>)value).Equals(t), action);

        return Case(value, EqualityComparer<T>.Default, action);
    }

    public ISwitchCase<T> Case(T value, IEqualityComparer<T> comparer, Action action)
    {
        return CaseWhen(t => comparer.Equals(Value, value), action);
    }

    public ISwitchCase<T> CaseWhen(Predicate<T> predicate, Action action)
    {
        if (predicate(Value))
        {
            action();
            return EmptySwitchCase<T>.Instance;
        }

        return this;
    }

    public void Default(Action action) =>
        action();
}

Whenever a case fullfils its predicate, it returns an EmptySwitchCase, which is the following:

internal class EmptySwitchCase<T> : ISwitchCase<T>
{
    public static EmptySwitchCase<T> Instance { get; }

    static EmptySwitchCase() { Instance = new EmptySwitchCase<T>(); }

    private EmptySwitchCase() { }

    public ISwitchCase<T> Case(T value, Action action) => this;

    public ISwitchCase<T> Case(T value, IEqualityComparer<T> comparer, Action action) => this;

    public ISwitchCase<T> CaseWhen(Predicate<T> predicate, Action action) => this;

    public void Default(Action action) { }
}

Thanks to this EmptySwitchCase, once a case is executed, no other case / default will be triggered.

Finally, as all these classes are internal, a simple static class is used as a facade:

public static class Bar // couldn't find a correct name...
{
    public static ISwitchCase<T> Switch<T>(T value) => new SwitchCase<T>(value);
}

The co-occurrence of Value and value in the same methods is irritating and likely to lead to bugs: I would rename value to query or something like that (and shall call value by query for the rest of this answer in the interests of not making any mistakes myself).

I was initially dubious of Flater's suggestion that creating the SwitchCase objects would be a problem given all the predicates flying around, so I looked at the memory characteristics of his example loop (only with a few more zeros on the end), and saw that I was completely wrong:

Note all the predicates and boxed ints (always a bad sign). All those predicates are not the ones the caller wrote: those are predicates created by the Case(T, ...) methods, and all the Int32s are integers being boxed so they can be cast as IEquatable<int>. If you change Flater's code to one which only uses the Case(Predicate<T>, ...) overload, everything disappears with the exception of the SwitchCases (the caller's predicates are reused), and as Flater says, you can remove those too by caching and improve the method memory characteristics dramatically.

Now to diverge from the code itself... there may be little point in your if (Value is IEquatable<T>) condition, since the EqualityComparer will do that for you. The only thing it gives you, is that it will use T's implementation of IEquatable<T> by default rather than query's: this sort of thing needs to carefully documented, because it could cause someone down the line no-end of confusion when they try to 'override' the comparison by implementing IEquatable<T> in a super-class of T, only to have it ignored. This feels the wrong way round to me; however, personally I wouldn't do the check for either, and would solely depend on the EqualityComparer (and maybe a comparer passed to the constructor). If I did want to use query's implementation, then I probably ought to know either that it has an implementation to use (and when it will be used), in which case the caller can decide if they want to use it or not. I'd rather provide a performant version with statically determinable behaviour:

public ISwitchCase<T> Case<S>(S query, Action action) where S : IEquatable<T>
{
    if (query.Equals(Value))
    {
        action();
        return EmptySwitchCase<T>.Instance;
    }

    return this;
}

Such a method also has the advantage of being efficient when handling structs, as it doesn't involve any boxing. I've also in-lined the conditional logic, because it avoids the allocation of the Predicate<T> and its body (which can't be reused).

This doesn't solve the boxing int problem in Flater's example, because the Case(T, etc.) overloads are better, but if you replace those with these, you can prevent any repetitive allocations occurring when these methods are called.

/// <summary>
/// Compares the query and internal value using the DefaultEqualityComparer, and invokes the given action if they match
/// </summary>
public ISwitchCase<T> Case(T query, Action action)
{
    return Case(query, EqualityComparer<T>.Default, action);
}

/// <summary>
/// Compares the query and internal value using the given comparer, and invokes the given action if they match
/// </summary>
public ISwitchCase<T> Case(T query, IEqualityComparer<T> comparer, Action action)
{
    if (comparer.Equals(Value, query))
    {
        action();
        return EmptySwitchCase<T>.Instance;
    }

    return this;
}

Obviously it's your decision what your API does, and it is important to stress that the behaviour is different like this, but that the behaviour was unclear before. With the added inline documentation, the new behaviour is clear and completely transparent at compile time. The difference is that that a value which provides an alternative implementation of IEquatable<T> will only be compared using this implementation if it is not a T and it can be determined at compile time that it is IEquatable<T>. I'd make a case for this behaviour being more 'intuitive', because it become equivalent to using a predicate of the form v => query.Equals(v) when query is not of type T. Without a : T constraint, this means that stuff which is comparable to T without necessarily being a T can also be used, which may or may nor be desirable.

Naturally, it is not nice having many methods implementing the if (predicate(Value)) logic, so I'd be inclined to put this login in a simple private ISwitchCase<T> Case(bool condition, Action action), essentially replacing your current Case(Predicate<T>, etc.) in this role. If you want to keep the dynamic IEquatable<T> behaviour, then you can still rid yourself of all the anonymous methods by using such a method instead of deferring everything to Case(Predicate<T>, etc.). For example:

/// <summary>
/// Compares the query with the internal value, first trying to use implementation of `IEquatable&lt;t&gt;` provided by the internal value, then that provided by the query, falling back to the DefaultComparer otherwise
/// </summary>
public ISwitchCase<T> Case(T query, Action action)
{
    if (Value is IEquatable<T>)
        return Case(((IEquatable<T>)Value).Equals(query), action);

    if (query is IEquatable<T>)
        return Case(((IEquatable<T>)query).Equals(Value), action);

    return Case(query, EqualityComparer<T>.Default, action);
}

/// <summary>
/// Compares the query and internal value using the given comparer, and calls the given action if they match
/// </summary>
public ISwitchCase<T> Case(T query, IEqualityComparer<T> comparer, Action action)
{
        return Case(comparer.Equals(Value, query), action);
}

/// <summary>
/// Invokes the given action if the condition is true
/// </summary>
private ISwitchCase<T> Case(bool condition, Action action)
{
    if (condition)
    {
        action();
        return EmptySwitchCase<T>.Instance;
    }

    return this;
}

Or, more tidily (using pattern matching):

/// <summary>
/// Compares the query with the internal value, first trying to use implementation of `IEquatable&lt;t&gt;` provided by the internal value, then that provided by the query, falling back to the DefaultComparer otherwise
/// </summary>
public ISwitchCase<T> Case(T query, Action action)
{
    if (Value is IEquatable<T> v)
        return Case(v.Equals(query), action);

    if (query is IEquatable<T> q)
        return Case(q.Equals(Value), action);

    return Case(query, EqualityComparer<T>.Default, action);
}

Note the inline documentation, which should appear on the interface.


Core Location - Practices for Supporting Common iOS Devices Typical tutorials suggest simple CoreLocation code similar to the following:

Header

#import<CoreLocation/CoreLocation.h>
@interface MyCLController : NSObject <CLLocationManagerDelegate> {
    CLLocationManager *locManager;
}
@property (nonatomic, retain) CLLocationManager *locManager;
@end

Setup

....
   self.locManager = [[[CLLocationManager alloc] init] autorelease];
   if(!self.locManager.locationServicesEnabled)
   {
       NSLog(@"User has opted out of location services");
   }

   self.locManager.delegate = self;
   self.locManager.desiredAccuracy = kCLLocationAccuracyBest;
   self.locManager.distanceFilter = 880.0f; // About 1/10th mile, in meters
   [self.locManager startUpdatingLocation];
....

Methods

- (void)locationManager:(CLLocationManager *)manager
didFailWithError:(NSError *)error
{
    // Respond to errors
    NSLog(@"Location manager error: %@", [error description]);
    return;
}

- (void)locationManager:(CLLocationManager *)manager
didUpdateToLocation:(CLLocation *)newLocation
fromLocation:(CLLocation *)oldLocation
{
    NSLog(@"%@\n", [newLocation description]);
}

Deallocate

....
   [self.locManager release]
....

The above code works, but what common problems should I expect once I start testing on the most common iOS devices and versions? (assuming target of iOS 3.2 and above, building on iOS 4.3 SDK, code should work on all iOS devices that support at least 3.2 and any devices that support above 3.2)

Many simple examples online are 1-2 years old - are there improvements to the above that should be made due to changes in CoreLocation and the new devices released since 3.2?

What other suggestions would make the above more robust, and are there any other pitfalls developers should be aware of as they develop for CoreLocation?

Apple's LocateMe sample has been kept up to date and shows how to properly handle getting a fix with accuracy evalutation and timeout. It's quite an excellent sample so why not start there?

Devices without cellular won't have the benefit of cell triangulation but that's all abstracted anyway.


Mapping ExpandoObject to another object type I am working on a helper method that maps properties from an ExpandoObject to a user supplied object and was wondering if the code could be cleaned up or made any more efficient. It currently has the correct behaviour from a simple test.
public static class Mapper
{
    public static void Map<T>(ExpandoObject source, T destination)
    {
        IDictionary<string, object> dict = source;
        var type = destination.GetType();

        foreach (var prop in type.GetProperties())
        {
            var lower = prop.Name.ToLower();
            var key = dict.Keys.SingleOrDefault(k => k.ToLower() == lower);

            if (key != null)
            {
                prop.SetValue(destination, dict[key], null);
            }
        }
    }
}

Full test can be seen here. There is currently no type checking. Would this be next to add?

I've come up with a few changes that should actually speed it up.

// By using a generic class we can take advantage
// of the fact that .NET will create a new generic type
// for each type T. This allows us to avoid creating
// a dictionary of Dictionary<string, PropertyInfo>
// for each type T. We also avoid the need for the
// lock statement with every call to Map.
public static class Mapper<T>
    // We can only use reference types
    where T : class
{
    private static readonly Dictionary<string, PropertyInfo> _propertyMap;

    static Mapper()
    {
        // At this point we can convert each
        // property name to lower case so we avoid
        // creating a new string more than once.
        _propertyMap =
            typeof(T)
            .GetProperties()
            .ToDictionary(
                p => p.Name.ToLower(),
                p => p
            );
    }

    public static void Map(ExpandoObject source, T destination)
    {
        // Might as well take care of null references early.
        if (source == null)
            throw new ArgumentNullException("source");
        if (destination == null)
            throw new ArgumentNullException("destination");

        // By iterating the KeyValuePair<string, object> of
        // source we can avoid manually searching the keys of
        // source as we see in your original code.
        foreach (var kv in source)
        {
            PropertyInfo p;
            if (_propertyMap.TryGetValue(kv.Key.ToLower(), out p))
            {
                var propType = p.PropertyType;
                if (kv.Value == null)
                {
                    if (!propType.IsByRef && propType.Name != "Nullable`1")
                    {
                        // Throw if type is a value type
                        // but not Nullable<>
                        throw new ArgumentException("not nullable");
                    }
                }
                else if (kv.Value.GetType() != propType)
                {
                    // You could make this a bit less strict
                    // but I don't recommend it.
                    throw new ArgumentException("type mismatch");
                }
                p.SetValue(destination, kv.Value, null);
            }
        }
    }
}


Bracket matcher in Python I would like to receive feedback on my coding interview for the following problem:

  Bracket Match

  A string of brackets is considered correctly matched if every opening
  bracket in the string can be paired up with a later closing bracket,
  and vice versa. For instance, “(())()” is correctly matched, whereas
  “)(“ and “((” aren’t. For instance, “((” could become correctly
  matched by adding two closing brackets at the end, so you’d return 2.

  Given a string that consists of brackets, write a function
  bracketMatch that takes a bracket string as an input and returns the
  minimum number of brackets you’d need to add to the input in order to
  make it correctly matched.

  Explain the correctness of your code, and analyze its time and space
  complexities.

    Examples:

    input:  text = “(()” output: 1

    input:  text = “(())” output: 0

    input:  text = “())(” output: 2 Constraints:

    [time limit] 5000ms

    [input] string text

    1 ≤ text.length ≤ 5000 [output] integer

def bracket_match(text):
    diffCounter = 0
    answer = 0
    length = len(text)

    for i in range(length):
        if text[i] == '(':
            diffCounter += 1
        elif text[i] == ')':
            diffCounter -= 1
        if diffCounter < 0:
            diffCounter += 1
            answer +=1

    return answer + diffCounter

text1=")))("
print(bracket_match(text1))

Doesn't look like you've done this:

  Explain the correctness of your code, and analyze its time and space complexities.

This is an anti-pattern:

    length = len(text)

    for i in range(length):
        # code using only `text[i]`, but never using `i`

You are computing the length of text, then using a for loop of over the range(length), assigning the index to i, but then never using i for anything other than fetching the character text[i].

Far, far better is looping over the letters of text:

    for ch in text:
        # use `ch` here.

Efficiency:

        if text[i] == '(':
           # ...
        elif text[i] == ')':
           diffCounter -= 1
        if diffCounter < 0:
           diffCounter += 1
           # ...

Will diffCounter ever be negative, without first subtracting 1 from it?  No?  Perhaps the next test belongs in the elif clause:

        if text[i] == '(':
           # ...
        elif text[i] == ')':
           diffCounter -= 1
           if diffCounter < 0:
              diffCounter += 1
              # ...

But that is still subtracting 1 and then (possibly) immediately adding 1 again.  Better: check for the underflow condition first.

        if text[i] == '(':
           # ...
        elif text[i] == ')':
           if diffCounter > 0:
              diffCounter -= 1
           else:
              # ...

While the diffCounter is an understandable variable name (barely), the variable named answer is completely mysterious.  Answer to what?  Why is it incremented when there is an underflow?  Why is it added to diffCounter at the end.  Not a comment in sight, and the code is anything but self documenting.

Follow PEP-8 coding standards, and use pylint or other style checker.  For instance, variables should not be camelCase, and answer +=1 needs an extra space.


Simple neural-network simulation in C++ (Round 4) You may want to take a look at Rounds 1, 2, and 3, though that isn't necessary for understanding what's below.

The major change since Round 3 is that my code is much cleaner and I'm including profiling information here.

Here's my script:

#include "hansel.h"
#include <deque>

int main()
{

  // Parameters.
  const double tau = 10;
  const double g_l = 0.1;
  const double I_0 = 2.3;
  const double theta = -40;
  const double V_l = -60;
  const double c = 0.5;
  const double N = 128;
  const double I_syn_bar = 0;
  const double tau_1 = 3;
  const double tau_2 = 1;
  const double C = 1;
  const double Delta_t = 0.25;

  // Simulation.
  std::deque<double> spike_times;
  double V = set_initial_V(tau, g_l, I_0, theta, V_l, c, N);
  for (double t = 0; t < 10000; t += Delta_t)
    {
      double I_syn = get_I_syn(I_syn_bar, N, tau_1, tau_2, t, spike_times);
      double dV_dt = get_dV_dt(g_l, V, V_l, I_syn, I_0, C);
      V += Delta_t * dV_dt;
      if (V > theta)
        {
          V = V_l;
          spike_times.push_back(t + Delta_t);
        }
    }

}

Here's the header file it includes:

#include <math.h>
#include <deque>

const double get_T(const double tau, const double g_l, const double I_0,
                   const double theta, const double V_l)
{
  return -tau * log(1 - g_l / I_0 * (theta - V_l));
}

double set_initial_V(const double tau, const double g_l, const double I_0,
                     const double theta, const double V_l, const double c,
                     const double N)
{
  const double T = get_T(tau, g_l, I_0, theta, V_l);
  const double i = 1;
  return V_l + I_0 / g_l * (1 - exp(-c * (i - 1) / N * T / tau));
}

double get_dV_dt(const double g_l, const double V, const double V_l,
                 const double I_syn, const double I_0, const double C)
{
  return (-g_l * (V - V_l) + I_syn + I_0) / C;
}

double get_f(const double tau_1, const double tau_2, const double t)
{
  return 1 / (tau_1 - tau_2) * (exp(-t / tau_1) - exp(-t / tau_2));
}

double get_I_syn(const double I_syn_bar, const double N, const double tau_1,
                 const double tau_2, const double t,
                 const std::deque<double> & spike_times)
{
  double sum_f = 0;
  const int n_spikes = spike_times.size();
  for (int i = 0; i < n_spikes; i += 1)
    {
      sum_f += get_f(tau_1, tau_2, t - spike_times[i]);
    }
  return I_syn_bar / N * sum_f;
}

Profiling the script, I see that ~100% of the time is spent in calls to get_I_syn.  Within that, time is split between calls to get_f (50.1%) and use of the deque [] operator (45.4%).

I'm hoping to speed up this script by a factor of ~10.

My questions are

1) Is there a way to refactor this line to increase speed:

return 1 / (tau_1 - tau_2) * (exp(-t / tau_1) - exp(-t / tau_2));

2) Is there a way to vectorize (or otherwise speed up) this loop:

for (int i = 0; i < n_spikes; i += 1)
  {
     sum_f += get_f(tau_1, tau_2, t - spike_times[i]);
  }

3) Is it likely to be faster to use an array or a vector instead of the deque object?

get_I_syn() currently always returns 0
I'm not sure if you are aware of this, but because I_syn_bar is a constant 0, this makes get_I_syn() always return 0.  This means you can just get rid of all calls to get_I_syn(), which is where 100% of your time is being spent.
However, assuming that there is some mixup here and I_syn_bar is supposed to be non-zero, I will explain how you can speed your program up by a factor of 50+.
Caching return values of get_f()
If you look at the get_f() function, everything is a constant except for t, which is passed in as t - spike_times[i] from get_I_syn().  If you look closely at what t and spike_times[i] are, they are all multiples of Delta_t.  In fact, t - spike_times[i] can only take on one of 10000 values, which are the first 10000 multiples of Delta_t.
Knowing this, you can precompute the 10000 possible values of get_f().  Then inside get_I_syn(), you can look up the precomputed values instead of doing the expensive computation.
I modified your program to do this and the program sped up by more than 50x.
Modified program
Here is the modified program.  I copied the header contents directly into the c++ file for my own convenience.  As you can see, I increased the number of iterations from 10000 to 300000 because otherwise the program ran too fast.
#include <deque>
#include <math.h>

const double get_T(const double tau, const double g_l, const double I_0,
                   const double theta, const double V_l)
{
  return -tau * log(1 - g_l / I_0 * (theta - V_l));
}

double set_initial_V(const double tau, const double g_l, const double I_0,
                     const double theta, const double V_l, const double c,
                     const double N)
{
  const double T = get_T(tau, g_l, I_0, theta, V_l);
  const double i = 1;
  return V_l + I_0 / g_l * (1 - exp(-c * (i - 1) / N * T / tau));
}

double get_dV_dt(const double g_l, const double V, const double V_l,
                 const double I_syn, const double I_0, const double C)
{
  return (-g_l * (V - V_l) + I_syn + I_0) / C;
}

double get_f(const double tau_1, const double tau_2, const double t)
{
  return 1 / (tau_1 - tau_2) * (exp(-t / tau_1) - exp(-t / tau_2));
}

double get_I_syn(const double I_syn_bar, const double N, const double *f_cache,
                 const int t_index, const int *spike_times, int n_spikes)
{
    double sum_f = 0;
    for (int i = 0; i < n_spikes; i++)
        sum_f += f_cache[t_index - spike_times[i]];
    return I_syn_bar / N * sum_f;
}

#define ITERATIONS        300000

// Declare in global scope to not overflow the stack.
static double f_cache[ITERATIONS];
static int spike_times[ITERATIONS];

int main()
{
    // Parameters.
    const double tau = 10;
    const double g_l = 0.1;
    const double I_0 = 2.3;
    const double theta = -40;
    const double V_l = -60;
    const double c = 0.5;
    const double N = 128;
    const double I_syn_bar = 2;
    const double tau_1 = 3;
    const double tau_2 = 1;
    const double C = 1;
    const double Delta_t = 0.25;
    int    i = 0;
    double t = 0;

    // Simulation.
    int numSpikes = 0;
    double V = set_initial_V(tau, g_l, I_0, theta, V_l, c, N);

    // Precompute all the possible values returned by get_f().
    for (i = 0, t = 0; i < ITERATIONS; i++, t += Delta_t)
        f_cache[i] = get_f(tau_1, tau_2, t);

    for (i = 0; i < ITERATIONS; i++)
    {
        double I_syn = get_I_syn(I_syn_bar, N, f_cache, i, spike_times,
                numSpikes);
        double dV_dt = get_dV_dt(g_l, V, V_l, I_syn, I_0, C);
        V += Delta_t * dV_dt;
        if (V > theta)
        {
            V = V_l;
            spike_times[numSpikes++] = i + 1;
        }
    }
}


Find a triangle in a graph represented as an adjacency list I have a graph represented as an adjacency list. I need to find a triangle in this graph. A triangle is a triple of vertices u, v and w, such that (u, v), (v, w) and (u, w) are edges of the graph. The graph does not necessarily needs to be undirected.

I found the following pseudocode for solving the problem:

function contains-cycle(g):
    for each edge (u, v) in g:
        for each vertex w in g:
            if (v, w) is an edge and (u, w) is an edge:
                return true
    return false

Now, this pseudocode is really easy and intuitive to understand, and I implemented very fast my "contains cycle" function using my adjacency list representation of a graph that I had implemented some time ago.

Note that checking if (v, w) is an edge in an adjacency matrix representation of a graph is a constant time operation, but not in an adjacency list, where the complexity of that operation is linear in the size of the adjacency list.

Thus, with an adjacency matrix representation of a graph, the above algorithm runs roughly in O(n3) time, since iterating through the edges requires O(|V|2) and through the vertices O(|V|) times.

On the other hand, using an adjacency list representation, the time complexity of the "contains cycle" algorithm is even worse, as far as I have understood, unless we can iterate through the edges in linear time with respect to the number of edges. We could do this, if we keep track of a list of all edges in the graph while constructing the graph, but I would like to avoid this, and try to find an alternative solution.

The following is my implementation using Python 3:

def is_an_edge(u, v):
    """Returns true if v is an adjacent node to u.

    Running time complexity: O(number of adjacent nodes to u).

    :type u : GraphNode
    :type v : GraphNode
    """
    for w in u.get_adjacent_nodes():
        if w == v:
            return True
    return False

def find_triangle(g):
    """Returns true if there's a triangle in g.

    A triangle in a graph is a triple of vertices u, v and w,
    such that all three edges  (u, v), (v, w) and (u, w) are in the graph.

    :type g : Graph
    """
    for u in g.get_nodes():
        for v in u.get_adjacent_nodes():
            for w in g.get_nodes():
                if is_an_edge(v, w) and is_an_edge(u, w):
                    return True
    return False

First of all, is my algorithm correct?

Its time complexity should be roughly O(n4), which is very high for a simple idea, algorithm.

Is there a better way to do what I want to achieve without keeping track of all edges of the graph?

Algorithm looks correct to me. The code is mostly okay, but I have a few suggestions below.

Improving the algorithm

For better algorithms, you could look at algorithms for finding triangle-free graphs. Wikipedia mentions an algorithm that’s O(n^35/27), where n is the number of vertices. However, most of that research is for undirected graphs only – the directed case is probably more complicated.

(Although you should clarify what you mean by a triangle in a directed graph. Consider the two graphs below: do they both contain a triangle, or is the first one disqualified because the triangle does not form a cycle?

If it’s the former, then I don’t see any different from the undirected case.

If it’s the latter, then I’m not sure your current implementation will spot the difference.)

Some alternative approaches that might be worth investigating:

In the undirected case, Turán’s theorem tells us that a triangle-free graph with n vertices can have at most n^2/4 edges. By counting edges, you might be able to very quickly prove that a graph is not triangle-free.
Vertices of lower degree are less likely to be in a triangle. (Vertices of degree 1 are never in a triangle.) You could work through the vertices in increasing order of degree. Once you’ve proved that a vertex of low degree isn’t in a triangle, you can delete it and ignore it for further calculations.

The complexity will probably be less than O(n^4), although I’m not sure exactly what it would be (or indeed whether we could find the complexity of this approach).
You might be able to do something clever with Ramsey numbers, although that’s only a half-baked thought – I’m not entirely sure if there’s something sensible or useful there, it’s just an inkling that there might be.

It’s worth noting that all of these approaches are quite complicated, and likely to add significant complexity to your code. It may that the simplicity of the code is more valuable than the speed increase – that depends on how and when this code will be used.

Improving the code.

Here are my thoughts:

There are lots of repeated function calls. For example, g.get_nodes() is going to be called n(n-1) times. I hope these results are fast and/or cached – otherwise those repeated calls are going to be a big drain on your code. (But also probably some low-hanging fruit for improved performance.)
I don’t know why is_an_edge isn’t defined as simply

def is_an_edge(u, v):
    return v in u.get_adjacent_nodes()

although note that this is going to make lots of calls to u.get_adjacent_nodes(), and you will call it with the same arguments multiple times.

I’d do it something like this:

already_checked_nodes = set()

for u in g.nodes():
    for v in u.neighbors - already_checked_nodes:
        if any(u.neighbors & v.neighbors):
            return True
    already_checked_nodes.add(u)

return False

These are the changes I’ve made:

g.nodes() is only looked up once. I don’t know how it’s implemented in your code, but it should probably be a generator.
.neighbors is a property of a node that is precomputed – there shouldn’t be any processing going on to retrieve this information.
Once I’ve looked at a vertex and found that it doesn’t have a triangle with any of its neighbours, I put it in already_checked_nodes and discard it from future processing. That will save a lot of repeat lookup operations.
I’ve done away with is_an_edge entirely, and am simply looking for anything in the set intersection of u and v’s neighbours. Using any() gets you the same short-circuit evaluation behaviour as before, but it’s Python doing it: as soon as it finds any element in that intersection, it returns True and goes to the next line.

You could probably make this faster by sorting the list of nodes from g. I suspect it might be better to start with nodes of high degree, which are more likely to be in triangles, and work your way down, but I’m guessing a bit there.


C++ Random Number Generation I am making a few wrapper classes for the C++ standard library and was wondering if the following code is good practice.

#pragma once

#include <random>

namespace LibraryName
{
    class RandomBase
    {
    protected:
        RandomBase()
            : m_RandomGenerator(m_RandomDevice()) {}

        std::random_device m_RandomDevice;
        std::mt19937 m_RandomGenerator;
    };

    template<typename T>
    class IntRandom : public RandomBase
    {
    public:
        IntRandom(T minValue, T maxValue)
            : m_Distribution(minValue, maxValue), m_MinValue(minValue), m_MaxValue(maxValue) {}

        T Next()
        {
            return m_Distribution(m_RandomGenerator);
        }

        inline T GetMinValue() const { return m_MinValue; }
        inline T GetMaxValue() const { return m_MaxValue; }

    private:
        std::uniform_int_distribution<T> m_Distribution;
        T m_MinValue;
        T m_MaxValue;
    };

    template<typename T>
    class FloatRandom : public RandomBase
    {
    public:
        FloatRandom(T minValue, T maxValue)
            : m_Distribution(minValue, maxValue), m_MinValue(minValue), m_MaxValue(maxValue) {}

        T Next()
        {
            return m_Distribution(m_RandomGenerator);
        }

        inline T GetMinValue() const { return m_MinValue; }
        inline T GetMaxValue() const { return m_MaxValue; }

    private:
        std::uniform_real_distribution<T> m_Distribution;
        T m_MinValue;
        T m_MaxValue;
    };

    class BoolRandom : public RandomBase
    {
    public:
        BoolRandom() : BoolRandom(0.5f) {}
        BoolRandom(float trueChance)
            : m_Distribution(trueChance), m_TrueChance(trueChance) {}

        bool Next()
        {
            return m_Distribution(m_RandomGenerator);
        }

        inline bool GetTrueChance() const { return m_TrueChance; }

    private:
        std::bernoulli_distribution m_Distribution;
        float m_TrueChance;
    };
}

That seems like a lot of code for very little purpose. It's allowing you to write

IntRandom<int> r(17, 42);
int i = r.Next();

instead of the no-helpers version,

std::mt19937 g(std::random_device{}());
std::uniform_int_distribution<int> dist(17, 42);
int i = dist(g);

Some people would say that that's not worth the 77 lines of code.

Two small problems with your code, related to your use of random_device: First, you're using only 32 bits of entropy to seed your entire Mersenne Twister; it would be better to use as many bits as the mt19937 has bits of state. However, doing this correctly is needlessly difficult in C++17, so you get a pass on it for now.

Second, your RandomBase class keeps a member of type random_device. This might be problematic, because a random_device is essentially an open file handle to /dev/urandom. If you create a lot of RandomBase objects at once, you might find yourself running out of file handles.

Oh, and a third problem is that random_device is non-copyable and non-movable; so your RandomBase is also non-movable. That could actually be pretty bad, depending on how you want to use it.

You could fix the random_device issues by removing the random_device data member, and instead providing a method like this:

void RandomBase::reseed() {
    std::random_device rd;
    m_RandomGenerator.seed(rd());  // TODO: better seeding
}

This keeps the random_device alive (and /dev/urandom open) only as long as it's needed; and it removes the data member from the class so that the class becomes movable and even copyable.

You could also reduce duplication by using templates and type-aliases, something like this:

template<class Distribution>
class dist_and_gen {
    std::mt19937 m_g;
    Distribution m_dist;

    template<class... Args>
    explicit constexpr dist_and_gen(Args&&...) :
        m_dist(std::forward<Args>(args)...) {}

    auto next() { return m_dist(m_g); }
    auto params() const { return m_dist.params(); }
};

template<class T> using IntRandom =
    dist_and_gen<std::uniform_int_distribution<T>>;
template<class T> using FloatRandom =
    dist_and_gen<std::uniform_float_distribution<T>>;
template<class T> using BoolRandom =
    dist_and_gen<std::bernoulli_distribution<T>>;

This version loses some of the details, such as your custom-named accessors for the parameters; but it preserves a lot of the details "accidentally". For example, I didn't try to preserve your BoolRandom::BoolRandom() default constructor; but it turns out that it just works anyway, thanks to the default constructor of the standard bernoulli_distribution.


Stripping time information in a DateTime object Is there any better way to strip the time information in a DateTime Object than creating a new DateTime Object? For example, if I just want today's date and not time, is the only way is to do the following?

DateTime now = DateTime.Now;
DateTime Today = new DateTime(now.Year, now.Month, now.Day);

Simple Google Search rendered this link DateTime.Date

this Returns only the date

DateTime now = DateTime.Now;
Console.WriteLine(now.Date.ToString("d"));


A faster way to compute largest prime factor (Again, but a lot better) I came up with this code after implementing the feedback given by @200_success. The previous one (A faster way to compute the largest prime factor) tested my patience, but still failed to give the answer on time. This one is an improvement as it gives the answer without much delay.

The question (Euler project problem #3)

  The prime factors of 13195 are 5, 7, 13 and 29.

  What is the largest prime factor of the number 600851475143 ?

Logic:

Keep dividing the number n by all the factors, starting from the smallest one. The value of n left in the end will be the largest prime factor of the original value of n.

My code:

function problem3(n){
    for(let i=2; i<n; i++){
        if(n%i === 0)n = n/i;
        }
    return n;
}
console.log(problem3(600851475143));

Improvement:

No need to loop through all the numbers below n
Remove the nested for loops, replacing them by only one
No need to check if a number is prime or not

My question: Even though it is a lot better*, but still, how can it be improved further?

* Really, it's a lot than the previous one as it used to be so slow that I never saw the output for 600851475143

It's buggy. Consider input 8.

Also, consider an input which is a very large prime. E.g. 2147483647. Your current code would take about 2147483645 trial divisions. It's possible to do it with only 46340 trial divisions while keeping the code very simple. Can you see how?

   Hint: 46340 is the square root, rounded down.

And that's easily optimised to 23171. Can you see how?

   Hint: only one prime is even.


Find the smallest positive integer that is absent from a given array I made the acquaintance of big-O a couple of weeks ago and am trying to get to grips with it, but although there's a lot of material out there about calculating time complexity, I can't seem to find out how to make algorithms more efficient.

I've been practising with the the demo challenge in Codility:

  Write a function that, given an array A of N integers, returns the
  smallest positive integer (greater than 0) that does not occur in A.
  For example, given A = [1, 3, 6, 4, 1, 2], the function should return
  5. The given array can have integers between -1 million and 1 million.

I started with a brute-force algorithm:

public int solution(int[] A)
{
    for ( int number = 1; number < 1000000; number ++)
    {
        if (doesContain(A, number)){}
        else return i;
    }
    return 0;
}

This passed all tests for correctness but scored low on performance because the running time was way past the limit, time complexity being \$O(N^2)\$.

I then tried putting the array into an arraylist, which reduces big-O since each object is "touched" only once, and I can use .Contains which is more efficient than iteration (not sure if that's true; I just sort of remember reading it somewhere).

public int solution(int[] A)
{
    ArrayList myArr = new ArrayList();
    for (int i=0; i<A.Length; i++)
    {
        myArr.Add(A[i]);
    }
    for ( int i = 1; i < 1000000; i++)
    {
        if (myArr.Contains(i)){}
        else return i;
    }
    return 0;
}

Alas, the time complexity is still at \$O(N^2)\$ and I can't find explanations of how to cut down time.

I know I shouldn't be using brute force, but can't seem to think of any other ways... Anyone have an explanation of how to make this algorithm more efficient?

I then tried putting the array into an arraylist, which reduces big-O since each object is "touched" only once, and I can use .Contains which is more efficient than iteration (not sure if that's true; I just sort of remember reading it somewhere).

As was mentioned in the comments, for your purpose, there is no significant difference in performance between int[] and ArrayList. Both store their data in a similar way, and both will have to iterate through the array to check for Contains. So that doesn't help anything.

Sorting

You mention sorting in the title of your question. But you don't actually do any sorting. Let's look at the challenge:

  Write a function that, given an array A of N integers, returns the smallest >positive integer (greater than 0) that does not occur in A. For example, given A = [1, 3, 6, 4, 1, 2], the function should return 5. The given array can have integers between -1 million and 1 million.

We need to know whether 1, 2, 3, etc are in the input. You can do this, by checking each and every option, one at a time. Since you don't know whether 1 would be at the start, or at the end of the array, you have to check the entire thing each and every time. That's inefficient.

However, if you sort the input, the array becomes somewhat predictable. You can loop through the sorted array until you find the first positive number. Is it higher than 1? Then we can return 1, since that is the smallest possible integer that's not in the list - the smallest positive integer is >1 after all. If it equals 1, we can go to the next element and check its value. If it is another 1, move on. If it is >2, we can return 2, else we must move on. And repeat.

In that way, what we have done, is sorting the array (\$O(n logn)\$), and then looping through it once. All in all, that's still \$O(n logn)\$.

public int solution(int[] a) {
  // sort
  Array.Sort(a);
  var answerCandidate = 1;

  // Find the first nonzero positive number.
  for(var index = 0; index < a.Length; index++) {
    if (a[index] < answerCandidate) {
      continue;
    } else if(a[index] == answerCandidate) {
      // Oops, we found our candidate in the list, so we move to the next one.
      // Since the list is sorted, we know the next one can only be farther
      // on in the list, so we don't need to restart the loop.
      answerCandidate++;
    } else {
      // In this case a[index] > answerCandidate. This means that we haven't found
      // the candidate in the list, so we can return answerCandidate. Break so
      // that we can handle the case where we pass the end of the array at the same time.
      break;
    }
  }

  return answerCandidate;
}

We can still make a marginal improvement by searching for the first positive number using a binary search on our sorted list.


pulling specific data from XML from inside 3rd party application Another script that is used as a token to extract information from a 3rd party application,  the application does other things in the background that I don't specifically know about.

This code does work inside the parent application and produces the expected output.

Is there a nicer way to write this code?

Public Function GetParameterXml()
    GetParameterXml = _
        "<Parameters>" &_
        "<Parameter Value='Age' Code='A' Description='Age' Type='Combo' Tooltip='Would you like the newest or oldest Reason?'>" &_
        "    <Options>" &_
        "   <Option Code='O' Description='Oldest' Value='OLD' />" &_
        "   <Option Code='N' Description='Newest' Value='NEW' />" &_
        "    </Options>" &_
        "</Parameter>" &_
        "</Parameters>"
End Function

'Parameter Variables
Dim Age : Set Age = Parameters.Item( Bookmark , "Age" )

' PlaceHolder Variables
Dim oNodes
Dim oNode

Set oNodes = XmlDoc.SelectNodes("Record/CelloXml/Integration/Case/ServiceEvent")

'''stop here and look around
stop

If (Age.Value = "OLD") Then
    For Each iNode in oNodes(0).childNodes
        If iNode.nodeName = "Service" Then
            For Each jNode in iNode.childNodes
                If jNode.nodeName = "Comment" Then
                    ReturnData = ReturnData & jNode.text & MD & ""
                End If
            Next
        End If
    Next
    If Len(ReturnData) > 0 Then
        ReturnData = Left(ReturnData, Len(ReturnData) - Len(MD))
    Else
        ReturnData = ""
    End if
ElseIf (Age.Value = "NEW") Then
    For Each iNode in oNodes(oNodes.length - 1).childNodes
        If iNode.nodeName = "Service" Then
            For Each jNode in iNode.childNodes
                If jNode.nodeName = "Comment" Then
                    ReturnData = ReturnData & jNode.text & MD & ""
                End If
            Next
        End If
    Next

    If Len(ReturnData) > 0 Then
        ReturnData = Left(ReturnData, Len(ReturnData) - Len(MD))
    Else
        ReturnData = ""
    End If
Else
    ReturnData = " Hit the Else Statement "
End If

Joined declaration & assignment
This isn't necessarily a bad use of the : instruction separator, but it did surprise me. Actually, I'd even give you a star on that one:

Dim Age : Set Age = Parameters.Item( Bookmark , "Age" )

Pretty much any other use I can think of of the : instruction separator is hindering readability. Here I find it's a clever way of joining declaration with assignment.
However this:

Dim oNodes
Dim oNode

Set oNodes = XmlDoc.SelectNodes("Record/CelloXml/Integration/Case/ServiceEvent")

Should then be written like this:
Dim xPath : xPath = "Record/CelloXml/Integration/Case/ServiceEvent"
Dim ServiceEventNodes : Set ServiceEventNodes = XmlDoc.SelectNodes(xPath)

I'm not going to mention I'm allergic to things like an "o" prefix to object variables' names (oops just did), but I think you'll agree with me that ServiceEventNodes is a much more descriptive name than oNodes can ever dream to be.
I've skipped oNode here because.. I'll get there in a moment.

'''stop here and look around

I'll skip the Stop keyword and hope this isn't production code. This isn't a necessary comment, but it's well-placed - stop right here and look below, you've got If...For...If...For...If blocks nicely (!) nested here. I'd say stop here and fill up your lungs with that horrible smell, you've got two blocks of code that are rigorously identical, except for one tiny little thing; how about first deciding what the child nodes are, and then iterate them, regardless of Age.Value?
Dim ChildNodes
Select Case Age.Value
    Case "OLD":
        Set ChildNodes = ServiceEventNodes(0).childNodes
    Case "NEW":
        Set ChildNodes = ServiceEventNodes(ServiceEventNodes.length - 1).childNodes
    Case Else:
        Err.Raise 5 'invalid procedure call or argument
End Select

Notice this is not allowing the code to run any further if there's an invalid input. Returning " Hit the Else Statement " is a dangerous thing to do, because it's a valid value that your function will return just as if everything went normal, and I don't know what that's used for, but I'm sure " Hit the Else Statement " doesn't quite belong in production data.
That's why I'd rather throw an error and blow up than return a string that's actually an error message.

That being settled, I believe both loops could be completely eliminated with a little XPath (untested; I think this would do it):
Dim CommentNodes : Set CommentNodes = ChildNodes.SelectNodes("Service/Comment")

Now that you've got the nodes you're after, you can iterate them:
Dim CommentNode
For Each CommentNode In CommentNodes
    ReturnData = ReturnData & CommentNode.Text & MD
Next

I left out the & "" part, because that's concatenating an empty string, which does strictly nothing but add confusing clutter to your code.
The last part:

If Len(ReturnData) > 0 Then
    ReturnData = Left(ReturnData, Len(ReturnData) - Len(MD))
Else
    ReturnData = ""
End If

Looks like it's working around the fact that you haven't properly declared and initialized ReturnData in the first place. If you put Dim ReturnData : ReturnData = vbNullString somewhere before the loop, then you don't need to assign it to an empty string if the loop resulted in a no-op.


Implement 'filtering' The NICTA Haskell Course presents the following function to implement on List:

data List t =
  Nil
  | t :. List t
  deriving (Eq, Ord)

And the function:

filtering :: Applicative f => (a -> f Bool) -> List a -> f (List a)
filtering _ Nil       = pure Nil
filtering f (x :. xs) = (++) <$> ((\y -> case y of True -> x :. Nil
                                                   False -> Nil) <$> f x) <*>
                                                         filtering f xs

Please review this implementation.

Taking a look at the course materials, I think you've missed the point. Applicative.hs has you define sequence :: Applicative f => List (f a) -> f (List a) before filtering, and looking briefly through the suggested progression it appears as though things are ordered such that there is always a “clever” answer to be built out of the things you've previously implemented.

That said, this question then gets both easier and harder. Easier because there is purpose built machinery to manipulate e.g. that weird non-Prelude List type. Harder because this isn't really standard Haskell anymore since all of our usual functions and typeclasses have been replaced by their evil twins.

I think the intended answer is that you map the predicate function over the list you're given, then use the sequence function you defined earlier to switch List (f Bool) to f (List Bool), then... I dunno. This is mirror world.

Here's how you'd write this function given the types from regular old base.

import Control.Applicative
import Data.Traversable

filtering :: (Applicative f) => (a -> f Bool) -> [a] -> f [a]
filtering f xs = (xs `filterBy`) <$> (sequenceA . map f $ xs)
    where
        filterBy :: [a] -> [Bool] -> [a]
        filterBy xs ps = map fst . filter snd $ zip xs ps

I assume the version you're looking for will be identical, but using sequence instead of sequenceA.

I think there is probably an even more clever solution—and I suspect it uses the reader instance (((->) a)) and a lifted filter—but I sure can't tease it out. Comments and edits welcome.


Printing all factorizations of a number For example, if we have 120, the answer should contain (2,2,2,3,5),(2,60),(4,30),...

Here is my not-so-good attempt

for (int[] x : getPossibleProducts(120)) {
    System.out.println(Arrays.toString(x));
}

public static List<String> getPossibleProducts2(int n) {
    List<String> possprod = new ArrayList<String>();
    possprod.add(n + "");
    if (isPrime(n)) {
        possprod.add(n + "");
        return possprod;
    }
    for (int i = 2; i * i <= n; i++) {
        if (n % i == 0) {
            for (String y : getPossibleProducts2(n / i)) {
                possprod.add(i + "x" + y);
            }
        }
    }
    return possprod;

}

public static List<int[]> getPossibleProducts(int n) {
    List<String> l = new ArrayList<String>();
    Set<int[]> set = new HashSet<int[]>();
    List<String> possprod = getPossibleProducts2(n);
    for (String x : possprod) {
        String[] y = x.split("x");
        Arrays.sort(y);
        if (!l.contains(Arrays.stream(y).reduce("", (a, b) -> a + b))) {
            l.add(Arrays.stream(y).reduce("", (a, b) -> a + b));
            int[] z = stringArraytoIntArray(y);
            set.add(z);
        }
    }
    return new ArrayList<int[]>(set);
}

public static int[] stringArraytoIntArray(String[] a) {
    int[] arr = new int[a.length];
    for (int i = 0; i < a.length; i++) {
        arr[i] = Integer.parseInt(a[i]);
    }
    return arr;
}

public static boolean isPrime(int i2) {
    if (i2 == 2)
        return true;
    if (i2 % 2 == 0)
        return false;
    for (int i = 3; i * i <= i2; i += 2)
        if (i2 % i == 0)
            return false;
    return true;
}

Here is the output:

[15, 8]
[10, 3, 4]
[30, 4]
[2, 3, 4, 5]
[12, 2, 5]
[10, 2, 2, 3]
[10, 12]
[120]
[15, 2, 2, 2]
[15, 2, 4]
[3, 40]
[4, 5, 6]
[2, 2, 5, 6]
[20, 6]
[2, 2, 30]
[2, 20, 3]
[2, 60]
[2, 2, 2, 3, 5]
[24, 5]
[10, 2, 6]
[3, 5, 8]

It is not immediately obvious that getPossibleProducts2() finds all
factorizations  and getPossibleProducts() removes the duplicates,
so the names could be chosen better. (And I would use "factorizations"
instead of "possibleProducts").

Your code is ineffective for two reasons:

The first method finds all factorizations with all possible permutations
(e.g. \$ 120 = 2 \cdot 60 = 60 \cdot 2 \$) so that you have to find the duplicates
in a second step.

As an example, for \$ n = 1024 \$, the first method computes
364176 factorizations, which are then reduced to 627 unique
factorizations in the second step.
You use strings to represents the factorizations (e.g. "2x60", "60x2") which then have to be split into integer arrays again.

To find duplicate factorizations, you split the string (e.g. "60x2")
to an integer array ["60", "2"], sort the factors (["2", "60"]) then concatenate the factors again ("260"). This is used as a hash key
to eliminate the duplicates.
This is quite computing intensive. In particular the hash key

Arrays.stream(y).reduce("", (a, b) -> a + b))

is compute twice, and StringBuilder might be a more effective way
to concatenate the array elements.

More important, it does not work: For \$ n = 130 \$,
the factorizations \$ 2 \cdot 65 \$ and  \$ 5 \cdot 26 \$ are both
mapped to the same hash key "265", so that the output of your
program is

[2, 65]
[13, 2, 5]
[10, 13]
[130]

without the factorization [5, 26].

Generally a better method would be to determine the factorizations
without duplicates in the first step. This can be done recursively
as follows:

Find the smallest factor f of the given number n.
Compute all factorizations of the n / f where the smallest
factor is at least f.

This automatically leads to factorizations with the factors in
increasing order and without duplicates.

As a side effect, the isPrime() function is not needed.

Here is a possible implementation. (Please take this as a demonstration of the algorithm
only. Java is not my first language, so there may be more idiomatic
ways to do implement this.)

public static List<int[]> factorizations(int n) {
    // Start with a minimum factor of 2:
    return factorizations(n, 2, new ArrayList<Integer>());
}

public static List<int[]> factorizations(int n, int minFactor, ArrayList<Integer> previousFactors) {
    List<int[]> result = new ArrayList<int[]>();

    if (n == 1) {
        // This is where the recursion terminates. Convert the list of
        // all previously found factors to an int[] array
        // (found here: http://stackoverflow.com/a/23945015/1187415).
        result.add(previousFactors.stream().mapToInt(i->i).toArray());
    } else {
        for (int factor = minFactor; factor <= n; factor++) {
            if (n % factor == 0) {
                // `factor` is a factor of `n`. Append it to (a clone of)
                // the previously found factors ...
                ArrayList<Integer> factors = new ArrayList<Integer>(previousFactors);
                factors.add(factor);

                // ... and (recursively) find all factorizations of `n / factor`
                // with factors greater or equal to `factor`:
                result.addAll( factorizations(n / factor, factor, factors) );
            }
        }
    }
    return result;
}

Example:

for (int[] f : factorizations(130)) {
    System.out.println(Arrays.toString(f));
}

produces the output

[2, 5, 13]
[2, 65]
[5, 26]
[10, 13]
[130]


How to reformat this code so I don't use 'Exit Try' and 'Exit For'? I use SonarQube to help me try and become a better programmer to program along with 'best-practice' standards.

There are business rules when a user is submitting data they enter in a datagridview that I encompass in a Try Catch block. I think the comments in the code describe fairly well what should be happening.

The problem: When I submit my project to SonarQube, it says "Exit Statements should not be used." I do not know what to use in lieu of the Exit statements, when they do exactly what I need. Can anyone point me in the right direction?

A visual representation of the Data Grid View:

Must be between 0-85             Must total to 100
+------------------+-------------+-----------------+--------------+
| Bonus Percentage | Fund Option | Fund Percentage | Election Date|
+------------------+-------------+-----------------+--------------+
|       85         |     2       |     50          |    3/20/2014 |
+------------------+-------------+-----------------+--------------+
|       85         |     5       |     50          |    3/20/2014 |
+------------------+-------------+-----------------+--------------+

Code:

tot = CInt(BonusDGV.Rows(0).Cells(0).Value)

'Business Rules Try/Catch block
Try
    'Ensure, for all rows, Fund amount totals 100 and Bonus Percentage is between 0-85%.
    For i As Integer = 0 To BonusDGV.Rows.Count - 1
        'don't count rows that contain no data
        If BonusDGV.Rows(i).Cells(0).Value = Nothing Then
            Exit For
        End If
        fundTot = fundTot + CInt(BonusDGV.Rows(i).Cells(2).Value)
        If tot <> CInt(BonusDGV.Rows(i).Cells(0).Value) Then
            MessageBox.Show("Percentage Must Be Between 0-85%.", "Invalid Entry", MessageBoxButtons.OK, MessageBoxIcon.Error)
            'Stop looping, there has been a violation in user entry (Bonus Percentage)
            Exit Try
        End If
    Next
    'Bonus Percentage must be between 0-85% // \\ Double check
    If tot > 85 Or tot < 0 Then
        MessageBox.Show("Percentage Must Be Between 0-85%.", "Invalid Entry", MessageBoxButtons.OK, MessageBoxIcon.Error)
        Exit Try
        'Total of fun amounts must = 100
    ElseIf fundTot <> 100 Then
        MessageBox.Show("Fund Total Must Add to 100%.", "Invalid Entry", MessageBoxButtons.OK, MessageBoxIcon.Error)
        Exit Try
        'Met all the business requirements, proceed to insert record
    Else
        For i As Integer = 0 To BonusDGV.Rows.Count - 1
            'Make sure there is actually data in the row
            If BonusDGV.Rows(i).Cells(0).Value = Nothing Then
                Exit For
            End If
            cmd.Parameters("@BonusPct").Value = BonusDGV.Rows(i).Cells(0).Value
            cmd.Parameters("@FundID").Value = CInt(BonusDGV.Rows(i).Cells(1).Value)
            cmd.Parameters("@DefPct").Value = BonusDGV.Rows(i).Cells(2).Value
            cmd.Parameters("@ElectDT").Value = BonusDGV.Rows(i).Cells(3).Value
            'nonquery because we don't expect a return value
            cmd.ExecuteNonQuery()
        Next
    End If
Catch ex As Exception
    MessageBox.Show("Invalid Entry. Make sure Bonus Percentage is between 0-85% and Fund Percentage total = 100.", "Invalid Entry", MessageBoxButtons.OK, MessageBoxIcon.Error)
End Try
'dispose of the connection
conn.Close()

All of your Exit Try statements follow an error condition. So why not throw an exception with the message and display it in the Catch block instead? That's the purpose of the Try..Catch.

Also, to get rid of the Exit For, just reverse the conditional. As it is, those two statements will end the loop completely on the first empty row, not just skip it.

(Ideally, you'd want to create a custom Exception class and throw it instead of a generic Exception, but this is just to give you an idea.)

So you end up with something like this

tot = CInt(BonusDGV.Rows(0).Cells(0).Value)

'Business Rules Try/Catch block
Try
    'Ensure, for all rows, Fund amount totals 100 and Bonus Percentage is between 0-85%.
    For i As Integer = 0 To BonusDGV.Rows.Count - 1
        'don't count rows that contain no data
        If BonusDGV.Rows(i).Cells(0).Value IsNot Nothing Then
          fundTot = fundTot + CInt(BonusDGV.Rows(i).Cells(2).Value)
          If tot <> CInt(BonusDGV.Rows(i).Cells(0).Value) Then
            Throw New Exception("Percentage Must Be Between 0-85%.")
          End If
        End If
    Next
    'Bonus Percentage must be between 0-85% // \\ Double check
    If tot > 85 Or tot < 0 Then
      Throw New Exception("Percentage Must Be Between 0-85%.")
    ElseIf fundTot <> 100 Then
      Throw New Exception("Fund Total Must Add to 100%.")
      'Met all the business requirements, proceed to insert record
    Else
        For i As Integer = 0 To BonusDGV.Rows.Count - 1
          'Make sure there is actually data in the row
          If BonusDGV.Rows(i).Cells(0).Value IsNot Nothing Then
            cmd.Parameters("@BonusPct").Value = BonusDGV.Rows(i).Cells(0).Value
            cmd.Parameters("@FundID").Value = CInt(BonusDGV.Rows(i).Cells(1).Value)
            cmd.Parameters("@DefPct").Value = BonusDGV.Rows(i).Cells(2).Value
            cmd.Parameters("@ElectDT").Value = BonusDGV.Rows(i).Cells(3).Value
            'nonquery because we don't expect a return value
            cmd.ExecuteNonQuery()
          End If
        Next
    End If
Catch ex As Exception
    MessageBox.Show(ex.Message, "Invalid Entry", MessageBoxButtons.OK, MessageBoxIcon.Error)
End Try
'dispose of the connection
conn.Close()


Kotlin function which counts standard deviation Here is the code. The code should counts the standard deviation for double array, but also for Int arrays.

fun calculateSD(numArray: DoubleArray): Double {
var sum = 0.0
var standardDeviation = 0.0

for (num in numArray) {
    sum += num
}

val mean = sum / numArray.size

for (num in numArray) {
    standardDeviation += Math.pow(num - mean, 2.0)
}

val divider = numArray.size - 1

return Math.sqrt(standardDeviation / divider )
}

You may want to be more idiomatic when working with collections, for an instance:
import kotlin.math.pow
import kotlin.math.sqrt

fun sd(data: DoubleArray): Double {
    val mean = data.average()
    return data
        .fold(0.0, { accumulator, next -> accumulator + (next - mean).pow(2.0) })
        .let { sqrt(it / data.size )
    }
}

BTW, I don't think that data.size - 1 is correct