Improve performance of math function applied to arrays This block of code is already somewhat optimized thanks to some answers given over at Stack Overflow. In the last question I made (Improve performance of function without parallelization) the code was marginally improved but I later had to re-write a little bit of it and the changes proposed didn't apply anymore.

I'm looking at optimizing this block of code without resorting to parallelization, other than that I'm pretty much open to using any package out there.

I think this is perhaps the correct site to post this since it's really more of a question about how can I improve my code rather than something not working with it.

Here's the MWE (minimum working example):

import numpy as np
import timeit

def random_data(N):
    # Generate some random data.
    return np.random.uniform(0., 10., N)

# Data lists.
array1 = np.array([random_data(4) for _ in range(1000)])
array2 = np.array([random_data(4) for _ in range(2000)])

def func():
    lst = []

    for elem in array1:

        # Define factors.
        a_01, a_11 = max(elem[1], 1e-10), max(elem[3], 1e-10)
        a_02, a_12 = array2[:,1], array2[:,3]
        # Combine the factors defined above.
        a_0 = a_01**2 + a_02**2
        a_1 = a_11**2 + a_12**2
        Bs, Cs = -0.5/a_0, -0.5/a_1

        # Perform operations.
        A = 1./(np.sqrt(a_0*a_1))
        B = Bs*(elem[0]-array2[:,0])**2
        C = Cs*(elem[2]-array2[:,2])**2
        ABC = A*np.exp(B+C)

        lst.append(max(ABC.sum(), 1e-10))

    return lst

# Time function.
func_time = timeit.timeit(func, number=10000)
print func_time

Well, some things I noticed:

    # Define factors.
    a_01, a_11 = max(elem[1], 1e-10), max(elem[3], 1e-10)
    a_02, a_12 = array2[:,1], array2[:,3]

a_02 and a_12 are static across the loop, no need to redefine them every time. a_01 and a_12 could be defined array-wise using np.maximum:

a_01 = np.maximum(array1[:,1], np.array(1e-10 for _ in xrange(len(array1))
a_11 = np.maximum(array1[:,3], np.array(1e-10 for _ in xrange(len(array1))

However, these are trivial improvements; function call overhead is often blamed for much in the Python optimisation world, but there is no way that removing 2n calls to max will improve things here. With that said, following the static data path further, we come across an instance of

a_0 = a_01**2 + a_02**2
a_1 = a_11**2 + a_12**2

If we instead define

a_022 = a_02**2
a_122 = a_12**2

at the beginning of the function, we can avoid n multiplications by replacing later occurrences of a_02**2 with a_022 (and likewise for a_12), which reduces the running time of the program by about 5% on my machine. That isn't too impressive, though I'm sure you should be able to knock that down a fair bit by doing more thing using numpy functions. However, my numpy skills evaporate when it comes to 2d arrays, so I can't help.

It is worth mentioning that the algorithm you are using is O(n^2) at least, so wouldn't get your hopes up too high. Otherwise, I'd recommend looking at Cython as this seems like a problem it would excel at, though you've mentioned that parallelism isn't an option, so I assume using C extensions won't be either. However, if it is, I think you will find great time savings there.

This was the final code I came up with:

def func2():
    lst = []
    # Define factors.
    a_02, a_12 = array2[:,1], array2[:,3]
    a_022 = a_02 ** 2
    a_122 = a_12 ** 2
    e10 = np.array([1e-10 for _ in xrange(len(array1))])
    a_01 = np.maximum(array1[:,1], e10)
    a_11 = np.maximum(array1[:,3], e10)
    for i, elem in enumerate(array1):
        # Combine the factors defined above.
        a_0 = a_01[i]**2 + a_022
        a_1 = a_11[i]**2 + a_122
        Bs, Cs = -0.5/a_0, -0.5/a_1
        # Perform operations.
        A = 1./(np.sqrt(a_0*a_1))
        B = Bs*(elem[0]-array2[:,0])**2
        C = Cs*(elem[2]-array2[:,2])**2
        ABC = A*np.exp(B+C)
        lst.append(max(ABC.sum(), 1e-10))
    return lst

N = 100
func_time = timeit.timeit(func, number=N)
print "Original\t", func_time / N
print "-" * 50

func2_time = timeit.timeit(func2, number=N)
print "Revision 1\t", func2_time / N
print "Improvement\t", str(int((func_time - func2_time) / func_time * 100)) + "%"
print "-" * 50


How do I divide Dao and Dao-Cache into different modules? I want to improve performance for api. I design cache in dao layer. I use caffine cache. Basic code is:
CacheConfig.java
@Configuration
public class CacheConfig {

    @Autowired
    private AppDao appDao;

    @Bean(value = "appCache")
    public LoadingCache<String, App> appCache() {
        return Caffeine.newBuilder()
                .maximumSize(1000)
                .refreshAfterWrite(5, TimeUnit.MINUTES)
                .build(key -> appDao.getByKeyFromDB(key));
    }
}

AppDao.java
@Repository
public class AppDao {

    @Autowired
    @Qualifier(value = "appCache")
    private LoadingCache<String, App> appCache;

    @Autowired
    private AppMapper appMapper;

    // service call getByAppKey
    public App getByKey(String appKey) {
        App app = appCache.get(appKey);
        return app;
    }

    // appCache load data by getByKey
    public App getByKeyFromDB(String appKey) {
        LambdaQueryWrapper<App> queryWrapper = Wrappers.lambdaQuery();
        queryWrapper.eq(App::getAppKey, appKey);
        return appMapper.selectOne(queryWrapper);
    }
}

I maybe divide getByKey and getByKeyFromDB to two file, but I have no idea.
How do I divide getByKey and getByKeyFromDB to different modules?

Well if you want to distinct classes, you should create two classes:
class CahedDao {
  AppCache cache;
  getByKey(String)
}

class DbDao {
  getByKeyFromDb(String)
}

Notes:
A. Codereview is to review working code, if you want to solve a problem you should better ask on stackexchange.
B. There are some issues in your code. Thanks to the DI and (bad) field injection you did not got them.


Regex substitution using a variable replacement string containing backreferences in Perl I have written a program where the user inputs a regular expression and a replacement string, and the program will search through a set of files and do the replacements.

The user is allowed to use backreferences in the replacement string (to refer to capture groups in the regex).
The part of the program I would like you to consider is the sub routine substitute_regex_backref below that searches a input string (for example the contents of a file) and does the replacements:

#! /usr/bin/env perl

use feature qw(say);
use strict;
use warnings;

my $old_str = '$1B<$ <hello> $> aba B<$ <kk> $>$1';
my $str = $old_str;
my $regex = qr'B<\$ <(.*?)> \$>';
my $replace = 'I<\$1$1\$1>';

(my $cnt, $str) = substitute_regex_backref( $str, $regex, $replace );

say "Old string: '$old_str'";
say "Number of replacements: $cnt";
say "New string: '$str'";

exit;

#
#
# ($num_substitutions, $new_str) =
#     substitute_regex_backref( $str, $regex, $replace )
#
#
# This sub routine is based on a stackoverflow.com answer
# by username Kent Fredric, see http://stackoverflow.com/a/392649/2173773
#
# Replace all occurences of $regex in $str with $replace.
# Returns number of replacements and the new string.
#
# The $regex input is assumed to be a regex quoted string. For example:
#  my $regex = qr/A simple (\w+) example/;
#
# The replacement string $replace is allowed to have backreferences:
#   Example: $replace = "a $1 b"
# "$1" is here treated as a backreference. It corresponds to capture group number 1
# in the $regex string.
#
# The replacement string $replace can also have backslash escaped dollar signs,
#  Example:  $replace = "\$1 a $1 b"
# Such escaped dollar signs should be replaced by
#  a literal '$' (and not treated as a backreference)

# Note: This subroutine was written to avoid using the ee modifier technique:
#   $var =~ s/$find/$replace/ee;
# which has sequrity risks if the $replace string comes from user input.
#
sub substitute_regex_backref {
    my ( $str, $regex, $replace ) = @_;

    # First obtain an array @m of matches
    my @m = $str =~ /$regex/g;
    if (@m == 0) {
        return (0, $str);
    }
    my $special_character_seq = "\x41\x42\x43\x44";

    # Remove any dollar signs from $str
    $str =~ s/\$/$special_character_seq/g;

    # If $regex contain escaped dollar signs (to be treated as
    # literal dollar signs), we need to replace
    # them with $special_character_seq since we have removed all
    # dollar signs in the previous line
    $regex =~ s/\\\$/$special_character_seq/g;

    # Do the replacement, but dollar signs in the $replace variable
    # are left as literal dollar signs
    $str =~ s/$regex/$replace/g;

    # Replace backslash escaped dollar signs with special string
    $str =~ s/\\\$/$special_character_seq/g;

    # use "reverse" function to cope with mixed one- and two-digit back references
    # For example, $12 should be dealt with before $1, in order to avoid confusion.
    for( reverse 0 .. $#m ){
        my $n = $_ + 1;
        my $val = $m[$_];

        # Replace "$n" with the value of capture group
        $str =~ s/\$$n/$val/g;
        }
        # Reinsert all literal dollar signs:
        $str =~ s/$special_character_seq/\$/g ;
    return (@m + 0, $str);
}

Any comments and suggestions are appreciated. I am especially concerned with the use of the $special_character_seq variable, if it is necessary or how to avoid using it..

This is incredibly hackish and broken code. Input that contains the magic substitution string (ABCD) will lead to incorrect output, and no steps to guard against this were taken. Even worse, the documentation is misleading since it uses double-quoted strings.

If you perform some escape encoding, then you must also escape the escape mechanism whenever it occurs in the input. For example, in single-quoted or q()-style strings, the closing delimiter is a forbidden symbol and must be escaped with a backslash \. However, there must also be a way to escape the backslash itself so that we can express both the strings ' via '\'' and \' via '\\\''. An alternative encoding in languages where two string literals cannot be directly adjacent is to use the delimiter as an escape character. If ' is the delimiter then '' cannot occur naturally except when denoting the empty string, so ' can be encoded as '''' and \' can be encoded as '\'''.

In our case, dollar symbols are forbidden and are escaped by the sequence ABCD, but that sequence itself is not escaped.

Instead of figuring out a proper way to escape this so that you can cobble together a half-solution using successive substitutions, let's take a step back and treat your substitution syntax as a serious language of its own. And languages get a parser. In particular, your language is a concatenation of three elements:

literal parts that consist of anything that's not a backslash or a dollar sign,
escaped characters that must at least contain $ via \$ and \ via \\,
backrefs that are a dollar sign followed by a positive non-zero integer.

The point that backslashes need escaping as well follows directly from the discussion above.

The syntax for backrefs is problematic since there's no way to properly delimit the number. Assume that we want to surround the capture group 1 with the letter a, and we can use the replacement directive 'a$1a', but assume we want to surround it with the digit 4, and '4$14' would parse more sensibly as the digit 4 followed by the backref $14. Perl – following established shell syntax – offers a ${1} syntax for these cases, and we should as well. Your code tries to deal with this problem by looping through all possible backrefs starting with the largest number, but given ten backrefs we wouldn't be able to discern $10 from ${1}0!

Therefore, the backref syntax ought to be a dollar symbol followed by either a sequence of decimal digits not starting with zero or else by such a sequence enclosed in matching curly braces. Clearly specifying it like this allows us to die when there is an unknown backref such as $666 when we only have three matches.

If we want to parse this substitution language in Perl, m/\G.../gc-style parsing is a viable option since the language is regular. The below function takes a replacement string, parses it, and returns a function that given a list of captures returns the fully substituted string.

use Carp;
use Scalar::Util 'reftype';

sub parse_replacement {
  my ($replacement) = @_;
  my @tokens;  # literals are strings, backrefs are refs
  pos($replacement) = 0;
  while (pos($replacement) < length($replacement)) {
    # normal literals
    if ($replacement =~ /\G( [^\$\\]+ )/xgc) {
          # if the previous token was literal, concatenate rather than pushing
          if (@tokens and not defined reftype $tokens[-1]) {
            $tokens[-1] .= $1;
          }
          else {
            push @tokens, $1;
          }
        }
        # escapes
        elsif ($replacement =~ /\G [\\]/xgc) {
          if ($replacement =~ /\G ([\$\\])/xgc) {
            # if the previous token was literal, concatenate rather than pushing
            if (@tokens and not defined reftype $tokens[-1]) {
              $tokens[-1] .= $1;
            }
            else {
              push @tokens, $1;
            }
          }
          elsif ($replacement =~ /\G\z/xgc) {
            croak "Illegal trailing backslash";
          }
          else {
            $replacement =~ /\G (.)/smxgc;
            croak sprintf "Escape can only contain backslash or dollar sign, not U+%4X '%s'", ord $1, $1;
          }
        }
        # backrefs
        elsif ($replacement =~ /\G [\$]/xgc) {
      if ($replacement =~ /\G [{]/xgc) {
        if ($replacement =~ /\G( [1-9][0-9]* )/xgc) {
          my $n = $1;
          push @tokens, \($n - 1);
          if ($replacement =~ /\G [}]/xgc) {
            # all is OK
          }
          else {
            croak "Expected closing curly brace for \${$n} identifier";
          }
        }
        else {
          croak 'Expected ${123} style numeric identifier inside ${...}';
        }
      }
      elsif ($replacement =~ /\G( [1-9][0-9]* )/xgc) {
        my $n = $1;
        push @tokens, \($n - 1);
      }
      else {
        croak 'Expected $123 or ${123} style number after dollar sign';
      }
    }
    else {
      croak sprintf "Illegal state – expected literal, escape or backref at position %d", pos($replacement);
    }
  }

  return sub {
    my ($captures) = @_;
    my $buffer = '';
    for my $token (@tokens) {
      if (reftype $token) {
        my $i = $$token;
            if ($i < @$captures) {
              $buffer .= $captures->[$i];
            }
            else {
              croak sprintf 'Unknown backref $%d; there are only %d captures', $$token, 0+@$captures;
        }
      }
      else {
        $buffer .= $token;
      }
    }
    return $buffer;
  };
}

Note that this code handles any eventuality and is therefore clearly correct and tries to provide meaningful error messages. It could further be improved by including the exact position and context in the error message.

There are various other problems with your code – I immediately see minor stylistic problems and another large-scale logical problem – but I'll leave those to another answer to tackle.

Your development would benefit from using a stringent set of test cases that handles edge cases and deliberately difficult inputs in order to find out whether your code actually matches your implementation. The Test::More module is an excellent place to start testing with Perl.


Excessive number casting & conversion? I’m trying to improve my code’s "signal to noise ratio", hence would appreciate any tips on improving this, what appears to be smelly code. Perhaps there could also be potential performance improvements to be made here:

public class PricingInfoDto
{
    [DataMember]
    public decimal ListPrice { get; set; }

    [DataMember]
    public decimal Contribution
    {
        get
        {
            var contributionAmount = (1 + TaxRate) * (float)ListPrice * (ContributionPercentage / 100);
            return Convert.ToDecimal(Math.Round(Math.Min(ContributionMaximumAmount, contributionAmount), 2));
        }
        private set
        {
            // For NHibernate Map
        }
    }

    public float TaxRate { get; set; }

    public double ContributionMaximumAmount { get; set; }

    public float ContributionPercentage { get; set; }
}

Unfortunately, I don’t have control over the types of properties, as the types of ContributionMaximumAmount & ContributionPercentage are enforced by the ADO.NET and the public DataMember is expected to be Decimal.

You are ignoring the whole point of decimals. Which is to avoid rounding errors from floating point caclulations.

You must cast TaxRate, ListPrice, ContributionPercentage, etc. to Decimal and only then do your calculations using Decimal values. Casting at some point is unavoidable, if you have no control over signatures. You can, however, implement a wrapper, which would do the casting and expose Decimal properties. That will make your code more pretty.


Singleton implementation using generics I did this as an exercise just to practice/improve using generics.

Independent of how useful this implementation of a Singleton is, how is my coding in terms of using generics and any other aspect of class design of code style?

void Main()
{
    var a = Singleton<MyClass>.Value;
    var b = Singleton<MyClass, MyClassFactory>.Value;
    var c = Singleton<MyClass>.Value;
    var d = Singleton<MyClass, MyClassFactory>.Value;
    var e = Singleton<MyOtherClass>.Value;
    var f = Singleton<MyOtherClass>.Value;
    var g = Singleton<MyOtherClass, MyOtherFactory>.Value;
    var h = Singleton<MyOtherClass, MyOtherFactory>.Value;
}

class SingletonBase
{
    protected static object Locker = new LockerObject();
}

class Singleton<T> : SingletonBase where T : new()
{
    static T StaticT;

    public static T Value
    {
        get
        {
            lock (Locker)
            {
                if(StaticT == null)
                {
                    StaticT = Activator.CreateInstance<Factory<T>>().Create();
                }
                else
                {
                    Console.WriteLine ("Singleton<T>::Value" + typeof(T).Name + " is already created");
                }
            }
            return StaticT;
        }
    }
}

class Singleton<T, F> : SingletonBase where T : new() where F : IFactory<T>, new()
{
    static T StaticT;

    public static T Value
    {
        get
        {
            lock (Locker)
            {
                if(StaticT == null)
                {
                    StaticT = new F().Create();
                }
                else
                {
                    Console.WriteLine ("Singleton<T, F>::Value" + typeof(T).Name + " is already created");
                }
            }
            return StaticT;
        }
    }
}

class LockerObject
{
    Guid myGUID;
    public LockerObject()
    {
        this.myGUID = Guid.NewGuid();
        Console.WriteLine ("New LockerObject " + this.myGUID.ToString());
    }
}

interface IFactory<T>
{
    T Create();
}

class Factory<T> : IFactory<T> where T : new()
{
    public T Create()
    {
        Console.WriteLine ("Factory<T>::Create()");
        return new T();
    }
}

class MyClassFactory : IFactory<MyClass>
{
    public MyClass Create()
    {
        Console.WriteLine ("MyClassFactory::Create()");
        return new MyClass();
    }
}

class MyClass
{
    public MyClass()
    {
        Console.WriteLine ("MyClass created");
    }
}

class MyOtherClass
{
    public MyOtherClass()
    {
        Console.WriteLine ("MyOtherClass created");
    }
}

class MyOtherFactory : IFactory<MyOtherClass>
{
    public MyOtherClass Create()
    {
        Console.WriteLine ("MyOtherFactory::Create()");
        return new MyOtherClass();
    }
}

Output:

New LockerObject 36aa2282-d745-43ca-84d2-998a78e39d51
Factory<T>::Create()
MyClass created
MyClassFactory::Create()
MyClass created
Singleton<T>::ValueMyClass is already created
Singleton<T, F>::ValueMyClass is already created

About your locking strategy to create the singleton, I would use a double check to avoid too much locking:

if(x)
{
    lock(Locker)
        if(x) //again once we got the lock
            DoStuff();
}
else
{
    FooBar();
}

Like this, you will hopefully use the lock only once when you first call the instance.


Refactoring the Iffy Tractor task There is a test of OO knowledge called Iffy Tractor. The task is to refactor the code and make it as much flexible as possible for the future modifications.

I have placed the code here.

It was taken from here.

This is a kind of a part of the game with a tractor unit, which receives the commands from user. So the possible modifications are:

add different types of Tractors
add more types of movements, available only to selected types of tractors
add some logging for each movement of the tractor

How would you go about refactoring this code? I am not asking for new code of course, just some basic explanation of your idea with a scheme of classes.

Here is my code as a solution too, please criticize it in a most cruel manner possible:

An evident thing is to replace the orientation string with the vectors, thus removing the if-blocks. I will not dwell on this.

So my interfaces look like this:

public interface Unit {
}
public interface Land {
    void positionUnit(Unit unit, Location location, Orientation orientation);
    Orientation getUnitOrientation(Unit unit);
    Location getUnitLocation(Unit unit);
}
public interface Landscape {
    boolean isAvailable(Location location);
}
public interface GameOperator {
    public void runCommand(String command, Unit unit);
}

How can I implement the command-movement transition inside the GameOperator?

A simple if-block which calculates new position of the tractor based on command string and then changes it inside the Land via setPosition(..).
Implementations of the Action interface. Create a new one for each command or "stamps" the existing one, taken from the pool with actual values, if no multithreading is expected.

public class Move implements Action {
    private Land land;
    private Unit unit;
    public Move(Unit unit, Land land) {
        this.land = land;
        this.unit = unit;
    }
    @Override
    public void evaluate() {
        Location oldLocation = land.getUnitLocation(unit);
        Orientation oldOrientation = land.getUnitOrientation(unit);
        Location newLocation = new Location(oldLocation.getX() + oldOrientation.getX(), oldLocation.getY() + oldOrientation.getY());
        land.positionUnit(unit, newLocation, oldOrientation);
    }
}

Move as a classname sounds really weird. It's a verb. Verbs don't make good class names.

Try a noun like MoveAction instead.

Location also seems to be suffering from "Feature Envy" of sorts... you ask the Land about the Unit's location? And you ask the Land about the Unit's orientation too! And then when you need to reposition a Unit, you ask the Land to do this!

Here, take a look at your own move command.

public void evaluate() {
    Location oldLocation = land.getUnitLocation(unit);
    Orientation oldOrientation = land.getUnitOrientation(unit);
    Location newLocation = new Location(oldLocation.getX() + oldOrientation.getX(), oldLocation.getY() + oldOrientation.getY());
    land.positionUnit(unit, newLocation, oldOrientation);
}

You need to get the Location from the Land, then you need to get the Orientation from the Land, and then you need to make the Land reposition a Unit... in order to move it.

I don't have the source for Land or Orientation or Location but I assume the following:

Orientation is an Enum of NORTH, WEST, EAST, SOUTH with two Maps x and y that map an Orientation to a int value of -1, 0 or 1
Land has a 2D int[] array
Location has 2 ints, x and y
Unit is implemented by Tractor

So lets alter this so it makes sense again. Right now, you've simulated what a human does. Look at the board, get the position of the object, look at in what way it's going... Then pick it up, hit each tile it passes, and place it on the last tile it needs to be.

Welcome to the magical world of programming, where board pieces can move on their own.

Unit should contain a move function. I'm not sure how the game works, since there's no instructions anywhere, but I assume it's a game where you give a series of Actions that would do stuff (like moving a unit some place). moveForwards in the original source code reveals this to me.

So a tractor would only move forward.

I suggest a radically different design from what you've suggested.

Assign responsibilities to each class.

Land manages the Locations.

Location is a value class, it contains x, y. These can't be altered.

Unit has a Location and Orientation.

With this new design, if you want to move, you ask the Unit and give it the Land to do so.
Unit then queries the Land in regards to obstacles and such. Then it moves by requesting a new Location FROM Land.

So why is this important?

Because right now, you've defined Move to be a pretty rigid Action. There's only one way to move. And that way ignores all obstacles (I have the feeling it even ignores the Land's size!). If you were to add obstacles later this code would come crashing down as you have to ask the Land for ALL it's units, iterate over their locations (and possibly orientations, if they're moving at the same time), and somehow calculate if it's possible, and then do the move. And you have to do this for each move, because you can't store state in between.


Cycle through an IEnumerable I built an extension method to cycle through all items of an IEnumerable starting at some index:

public static IEnumerable<T> Circle<T>(this IEnumerable<T> list, int startIndex)
{
    if (list != null)
    {
        List<T> firstList = new List<T>();
        using (var enumerator = list.GetEnumerator())
        {
            int i = 0;
            while (enumerator.MoveNext())
            {
                if (i < startIndex)
                {
                    firstList.Add(enumerator.Current);
                    i++;
                }
                else
                {
                    yield return enumerator.Current;
                }
            }
        }
        foreach (var first in firstList)
        {
            yield return first;
        }
    }
    yield break;
}

So when you do

Enumerable.Range(1,10).Circle(5);

The result is 6,7,8,9,10,1,2,3,4,5.

What I don't like is the use of the firstList variable. Is there a way to do this with the enumerator only, without storing an intermediate result?

EDIT:
I use an enumerator to iterate the enumerable not more than once.

A shorter form of Trevor's answer (but essentially doing the same thing):

public static IEnumerable<T> Circle<T>(this IEnumerable<T> list, int startIndex)
{
    return list.Skip(startIndex).Concat(list.Take(startIndex));
}

This will still lazy evaluate just like Trevor's answer, because Concat lazily evaluates.

Further more, if you have written a method to cycle through an IEnumerable, why not call it Cycle?

Lastly, I'd actually recommend taking advantage of lazy evaluation to give you a more useful method. The one below will continue to cycle indefinitely, starting with an optional index:

public static IEnumerable<T> Cycle<T>(this IEnumerable<T> list, int index = 0)
{

    var count = list.Count();
    index = index % count;

    while(true)
    {
        yield return list.ElementAt(index);
        index = (index + 1) % count;
    }
}

Then you can do something like:

foreach(var num in Enumerable.Range(1, 10).Cycle(4).Take(30))
{
    Console.WriteLine(num.ToString());
}

And this lets you specify just how many repeated items you want.


transform string to decimal This feels wrong:

private decimal ConvertStringToDecimal(string decimalString, decimal defaultReturnValue)
{
    var returnDecimal = defaultReturnValue;

    try
    {
    returnDecimal = decimal.Parse(decimalString.Trim());
    }
    catch (Exception)
    {
    // naughty
    }

    return returnDecimal;
}

but I think this is the only way to transform a string from a third party system to a decimal whilst returning a default. Does anyone have any suggestions on how to improve this?

The easiest and most clean way would be to use decimal.TryParse() like so

private decimal ConvertStringToDecimal(string decimalString, decimal defaultReturnValue)
{
    decimal result;
    if (decimal.TryParse(decimalString.Trim(), out result))
    {
        return result;
    }

    return defaultReturnValue;
}

Using exceptions to control the returned value isn't a good way, especially if there are better methods to use.

Your indentation is off but I assume that thats a posting issue.


A NOT macro for negative conditions Motivation

I typically try to avoid using ! in if-statements, because (to me) it seems easy to overlook. So, I'll usually just spell it out instead, ex: if (done == false) instead of if (!done). While that makes the intention more obvious, it doesn't read very fluidly.

Macro code

For those cases I wrote the following macro:

#define NOT(x) ((x) == false)

The outer parentheses make it possible to use NOT(cond) without additional parentheses in if and make it less error prone:

if NOT(condition) {
    ...
}

Complete example of macro usage

#include <iostream>

#define NOT(x) ((x) == false)

bool successful() {return false;}

int main(int argc, char **argv)
{
    if NOT(successful()) {
        std::cout << "Warning! Self-destruction activated...\n";
    }

    const bool this_is_true = true;

    if NOT(this_is_true) {
        std::cout << "True is false.\n";
    }

    return EXIT_SUCCESS;
}

Concerns

While this works, I have some concerns:

From a best-practices or clean code perspective, is this frowned upon?
How are macros generally viewed, or reported on, in code analysis tools? We are considering employing a system called CAST-AIP for code analysis and, while I am strongly in favor of doing so, I am wondering how many volumes of practice violations my reports are going to be.

Something like if (foo == false) will always be frowned upon. If you find the negation hard to spot you might consider using not instead (see this for more info).
E.g. if (not foo)

From what I've seen macros are generally discouraged as they tend to obfuscate code and are not typesafe.


Arithmetic expression parsing, and converting infix to postfix notation I'm doing a infix to postfix conversion. The code works, but here are some points I want to improve

1) In the line of

while ((!operators.empty())&&(order(operators.top()) >= order(token[0])))

If the condition order changes ( check the stack empty later), there'll be a segmentation fault (because I'm calling operators.top() which is NULL)

2) Convert from char to string using:

 string s(1, operators.top());

which I think constantly create a new variable s and delete it.

How to improve this code?

#include "Parser.h"
#include <sstream>
#include <algorithm>
#include <iterator>

using namespace std;

Parser::Parser()
{
    Postfix="";
    _source="";
    operators=std::stack<char>();
}

string Parser::postfix()
{
    return Postfix;
}

string Parser::source()
{
    return _source;
}

Parser& Parser::parse(string s)
{
    Postfix="";
    _source="";
    operators=std::stack<char>();

    _source=s;
    string token;

    istringstream iss (_source);
    while (iss >> token)
    {
        if (isdigit(token))
            Postfix.append(token).append(" ");

        if (isoperator(token))
        {
            if (operators.empty())
            {
                operators.push(token[0]);
            }
            else
            {
                // Pop until get the same order operator
                while ((!operators.empty())&&(order(operators.top()) >= order(token[0])))
                {
                    string s(1, operators.top());
                    Postfix.append(s).append(" ");
                    operators.pop();
                }
                operators.push(token[0]);
            }
        }
        cout <<Postfix<<"\n";
    }
    while(!operators.empty())
    {
        string s(1, operators.top());
        Postfix.append(s).append(" ");
        operators.pop();
    }
    return *this;
}

bool Parser::isdigit(const string &str)
{
    return str.find_first_not_of("-0123456789") == std::string::npos;
}

bool Parser::isoperator(const string &str)
{
    return (str=="+")||(str=="-")||(str=="*")||(str=="/");
}
int Parser::order(const char& c)
{
    if (c=='+')
        return 1;
    else if (c=='-')
        return 1;
    else if (c=='*')
        return 2;
    else if (c=='/')
        return 2;
    else
        return 0;
}

If the condition order changes ( check the stack empty later), there'll be a segmentation fault (because I'm calling operators.top() which is NULL)

Calling top() on an empty queue is even worse then returning NULL. It is actually undefined behavior. Which means your code is broken.

This though:

while ((!operators.empty())&&(order(operators.top()) >= order(token[0])))

Is fine: The && operator evaluates the left hand side first and only if this evaluation returns true will it evaluates the right hand side. So the above expression is well formed and correct.

  Convert from char to string using: which I think constantly create a new variable s and delete it.

Yes it creates a new variable s.

s will be destroyed(and its destructor called) when it goes out of scope (just like all variables are destroyed when they go out of scope). This is not a big deal.

Personally I would have used Flex/Bison to implement the parser.


Simple Web Crawler for Small Sites This script is my web crawlerfor small sites. Can you possibly review it for best coding practices?
Code
import bs4 as bs
import urllib.request
from urllib.parse import urlparse,urljoin
import pprint

class Page(object):
    def __init__(self,base_url,url):
        self.url = url
        self.base_url = base_url
    def soup(self):
        sauce = urllib.request.urlopen(self.url).read()
        return (bs.BeautifulSoup(sauce,'lxml'))
    def title(self):
        soup = self.soup()
        return soup.title.string
    def links(self):
        urls = []
        soup = self.soup()
        href = [i.get('href') for i in soup.findAll('a') ]
        links = [i for i in (list(map((lambda url : url if bool(urlparse(url).netloc) == True else urljoin (self.base_url, url)),href))) if i.startswith(self.base_url)]
        return links
    def map_page(self):
        map = {self.url:{'title':self.title(),'links':set(self.links())}}
        return map

def site_map(base_url):
    map_pages = {}
    links_to_map = [base_url]

    def check_and_add(url):
        if url not in map_pages:
            [links_to_map.append(i) for i in Page(base_url,url).links()]
            (map_pages.update(Page(base_url,url).map_page()))
            links_to_map.remove(url)
        else:
            links_to_map.remove(url)

    while links_to_map != []:
        url = links_to_map[0]
        check_and_add(url)

    pprint.pprint(map_pages)

I have made some changes to make it neater and more efficient. See the end of this response for the final solution. But I'll start with a few comments.

There is no point making your class inherit the object class, especially if you're working with Python 3.

class Page(object):

The soup() method seems to be called indiscriminately causing BeautifulSoup to parse the contents of the page in concern too many times (see the map_page() method, for example). It is probably more efficient to parse once, afterwhich you can save whatever (e.g. title and links)you need as object properties.

Using list and map in this context seems unnecessary. It helps if you strive for readability --- your future self will thank you.

links = [i for i in (list(map((lambda url : url if bool(urlparse(url).netloc) == True else urljoin (self.base_url, url)),href))) if i.startswith(self.base_url)]

Also, checking the truthiness of urlparse(url).netloc doesn't require you to coerce it to a boolean result first. Simply replacing if bool(urlparse(url).netloc) with if urlparse(url).netloc would suffice.

In the site_map() function, you can replace the while loop with something simpler, e.g.:

while links_to_map:
    check_and_add(links_to_map.pop())

I have also made other subjective simplifications that hopefully should be obvious to you.

import pprint
import urllib.request
from pprint import pprint
from urllib.parse import urlparse, urljoin

import bs4 as bs

class Page:
    def __init__(self, base_url, url):
        self.url = url
        self.base_url = base_url
        self.souped = None
        self.title = None
        self.links = None

    def soup(self):
        def clean(url):
            return url if urlparse(url).netloc else urljoin(self.base_url, url)

    sauce = urllib.request.urlopen(self.url).read()
    self.souped = bs.BeautifulSoup(sauce, "lxml")
    self.title = self.souped.title.string
    hrefs = set([clean(i.get("href")) for i in self.souped.findAll("a")])
    self.links = [link for link in hrefs if link.startswith(self.base_url)]
    return self

    @property
    def map_page(self):
        lookup = {self.url: {"title": self.title, "links": self.links}}
        return lookup

def site_map(base_url):
    map_pages = {}
    links_to_map = [base_url]

    def check_and_add(url):
    if url not in map_pages:
        page = Page(base_url, url).soup()
        links_to_map.extend(page.links)
        map_pages.update(page.map_page)

    while links_to_map:
        check_and_add(links_to_map.pop())

    pprint(map_pages)


Checking if string contains other strings This is simply an extension method which checks if a string str contains all the values in values.

However, there are cases when str might be null, values might be null, or any item in values might be null. I am handling those cases by using the ternary ?:, null-conditional ?. and null-coalesce ?? operators.

Should this piece of code be refactored?

public static bool ContainsAll(this string str, string[] values)
{
    return str == null ? false : values?.All(v => str.Contains(v ?? "")) ?? false;
}

I like the ternary operator but in this case I wouldn't use it because a single expression that makes sure both variables are valid and then searching for the strings is easier to understand I think.

The v ?? "" isn't very pretty, especially the "" part which actually should be string.Empty. You could get rid of it by filtering the array and skipping the empty and null elements.

public static bool ContainsAll(this string str, params string[] values)
{
    return
        !string.IsNullOrEmpty(str) &&
        values != null &&
        values
            .Where(x => !string.IsNullOrEmpty(x))
            .All(v => str.Contains(v));
}

If you decorate the second parameter with params keyword then you can just type the values without an array (if you need to):

"text".ContainsAll("t", null);


PHP/Apache - Production/staging consts file path Is there anything wrong with having a file path as an env variable and using that as-is in a require_once?

<?
$CONSTANTS_FILE = getenv('CONSTANTS_FILE_PATH');
require_once($CONSTANTS_FILE);
echo DB_USER
?>

And here is how it is set in apache:

<VirtualHost *:80>
    #ServerName
    #DocumentRoot
    SetEnv CONSTANTS_FILE_PATH /path/to/file.php
    # Directory
</VirtualHost>

Is there a better way of having variable const files included in the project source code?

You are right, but there are better ways to do that.
One way is using dotenv. Trending way for managing configurations.

from dotenv project:

  Storing configuration in the environment is one of the tenets of a
  twelve-factor app. Anything that is likely to change between
  deployment environments – such as database credentials or credentials
  for 3rd party services – should be extracted from the code into
  environment variables.

So what you are doing is right, the way you are doing may not be best.

Since you are doing require_once(), you may want to check if file exists else it would be fatal error.

The problems I see with keeping that path in Apache:

You may need to modify apache config and restart / reload Apache for your new configurations
Not all developers in your team may have access to Apache config. If the config is in code itself, like .env, it would be easy for developers.
Debugging would be easy with .env
If you plan to unit test, mocking would be easy.


Performing an action which may return other actions to be performed, concurrently I have a program which, at its core, involves repeatedly performing an action on some value, and based on that action also returning a list of new values to perform that action on. The pure version of my function is:

recurse :: (a -> [a]) -> a -> [a]
recurse f a = a : concatMap (recurse f) (f a)

Which can be used for things like tree traversals, such as:

data Tree a = Node [Tree a] | Leaf a

f (Node ls) = ls
f (Leaf x) = []

recurse f (Node [Leaf 10, Node [Leaf 20, Leaf 3], Leaf 12])
  == [Node [..], Leaf 10, Node [..], Leaf 20, Leaf 3, Leaf 12]

This function becomes more useful if you generalize it to an arbitrary monad:

recurseM :: Monad m => (a -> m [a]) -> a -> m [a]
recurseM f a = do
  xs <- f a
  rests <- mapM (recurseM f) xs
  return $ a : concat rests

But what I really want to do with it is make it asynchronous, because (a) I do not care about the order the results come in, and (b) I anticipate the actions themselves taking a while waiting on network operations. Here is my first pass at writing the async implementation (without results) of the above functions:

recurseAsync_ :: (a -> IO [a]) -> a -> IO ()
recurseAsync_ f a = do
  xs <- f a
  mapM_ (forkIO . recurseAsync_ f) xs

While this works, I'm unfamiliar enough with concurrency in general and in Haskell specifically to be sure that this will actually run all of the code I want in parallel. Also, I'm unsure of how to make this code return the results, but that isn't really the important part of the code.

I think yours works, but async specializes in this stuff.

recurseAsync :: (a -> IO [a]) -> a -> IO [a]
recurseAsync f a = do
  xs <- f a
  fmap concat $ mapConcurrently (recurseAsync f) xs


Сlone image in pure js I wrote JS to clone image and attached this action to button.
It works but I'm not sure that this better way to do it.

My questions:

is it correct to use querySelector to return elements, or is it better to add additional classes or id for buttons and pictures?
use cloneNode is optimal or better use other ways in this case?

(function() {
  'use strict';
  document.querySelector('button[action="button"]').addEventListener('click', function() {
    let image = document.querySelector('img[alt="Dog"]');
    let cln = image.cloneNode(true);
    document.getElementById('image-section').appendChild(cln);
  });
})();

       <button action="button">Yeah, I want more dogs!</button>

Note: you're cloning a DOM element not an actual image..

About .querySelector

The great thing about .querySelector is that you can use any DOMString (CSS string selectors) to select the elements. However in your case you're using the action attribute on the button. So essentially you're already naming your buttons (here there's only one). You can actually get away with not naming them at all. If you have multiple buttons you could simply select them with .querySelectorAll which returns a NodeList that you can either:

directly iterate over with the built-in forEach

document.querySelectorAll('button').forEach(button => {
  // do something with `button`
});

or first convert the NodeList into an Array. You can use the ES6 spread syntax to destructure the iterable NodeList object into an actual object of type Array. This way you can then apply methods from the Array class, for example: .forEach, .map, .reduce, .filter, etc... Here's an example:

[...document.querySelectorAll('button').forEach(button => {
  // do something with `button`
}

I personally never go with .getElementById or .getElementbyClassName and always use .querySelector and .querySelectorAll.

About .nodeNode

If you're going to duplicate a DOM element on the page then I would say nodeList is the way to go. There are alternatives but they are slower.

copy the OuterHTML of the image (the actual image tag) and add it directly to the innerHTML of #image-section with .innerHTML +=. This is by far the worst idea as it forces the browser to re render the DOM when the property is changed. You can read more on this other Stack Overflow post.
copy the OuterHTML of the image, create a new element we will use as a wrapper for our duplicate then add the image element as the wrapper's child and append the wrapper's first child (the image) to #image-section.

Here is how it can be implemented:

const duplicate = () => {
  const image = document.querySelector('img[alt="Dog"]');
  const copy = document.createElement('div');
  copy.innerHTML = image.outerHTML;
  document.querySelector('#image-section').appendChild(copy.firstChild);
}

I have tested both version with a timer to make sure, here is the code. Results are below.

const timeit = fn => {
    const start= new Date();
    let i = 1000;

    while(i--) fn();
    return new Date()-start;
}

console.log('clone: ', timeit(clone), 'ms')
console.log('duplicate: ', timeit(duplicate), 'ms')

       <button action="button">Yeah, I want more dogs!</button>

Results are here, time to render 1000 dogs (in ms):

-----------------Chrome:----------------
clone:  50 | 30 | 44 | 38 | 50 | 37 | 42
copy:   86 | 50 | 55 | 50 | 57 | 45 | 56

-----------------Firefox:---------------
clone:  42 | 46 | 44 | 66 | 34 | 53 | 50
copy:   59 | 49 | 50 | 97 | 55 | 55 | 58

You should stick with nodeClone(true).

For further reference on this read: Deep cloning vs setting of innerHTML: what's faster?


grouping values in a list of tuples to a dictionary I've a list of tuples (comes in pairs only)

a = [(1,2),(1,4),(3,5),(5,7)]

which I'm trying to convert to a dictionary as below

d = {1: [2, 4], 3: 5, 5: 7}

Here's what I've come up with -

a= [(1,2),(1,4),(3,5),(5,7)]
d= {}

for k, v in a:
    if k not in d:
        d[k] = v
    else:
        d[k] = [d[k]]
        d[k].append(v)

Is there a better way to do this? Can this be achieved using something like defaultdict(list) without having to compromise on the desired output?

I can't think of a builtin that solves your problem, but there is an even simpler solution than the one you provided. Taking Gareth Rees' comment into consideration, I would solve your problem like this:

input_list = [(1,2),(1,4),(3,5),(5,7)]
output_dict = {}

for key, value in input_list:
    #Initialize empty list if key doesn't exist
    if key not in output_dict:
        output_dict[key] = []
    # Now we know that the key always exists
    output_dict[key].append(value)

print(output_dict)
# If you want single items to be integers instead of lists
for key, value in output_dict.items():
    if len(value) == 1:
        output_dict[key] = value[0]

print(output_dict)

Output:

{1: [2, 4], 3: [5], 5: [7]}
{1: [2, 4], 3: 5, 5: 7}

The first for loop creates a dictionary mapping ints to lists. The advantages of this structure compared to the solution you provided is that there are no type-checks required when parsing the data. Checking if a value is a duplicate is a simple if len(value) > 1.

Usually, I'd prefer this implementation in any application that is not extremely dependent on performance. If your application requires a lot of performance, I'd suggest:

Not using Python
Not using dicts

If this is the case, I could elaborate on more ways of improving performance.

In the more likely scenario that a coherent data structure is more important, I'd reduce your code to the first for-loop above, and wrapping it in a function:

def list_to_dict(input_list):
    output_dict = {}
    for key, value in input_list:
        if key not in output_dict:
            output_dict[key] = []
        output_dict[key].append(value)
    return output_dict


Sending a file over a socket with AES encryption I'm not sure what's the best way to deal with streams that need to be closed whenever I'm done using them? Usually the need to also catch the exception inside the finally statement is very, very annoying. Am I doing this right?

public class FileSender {

    public static final String KEY = "00000000000000000000000000000000";
    public static final String ALGORITHM = "AES";
    public static final String FILE = "C:\\Users\\Andres\\Desktop\\file.txt";
    public static final String HASH_ALG = "MD5";

    private String filePath;
    private String serverMachine;
    private int serverPort;

    public FileSender(String filePath, String serverMachine, int serverPort){
        this.filePath = filePath;
        this.serverMachine = serverMachine;
        this.serverPort = serverPort;
    }

    public void sendFile(){
        CipherOutputStream cipherOut = null;
        Socket client = null;
        String checkSum;
        try {
            System.out.println("Running client ...");
            client = connectToServer();
            checkSum = getHashChecksum();
            cipherOut = initCipherStream(client);
            transferData(cipherOut, checkSum);
        } catch (Exception e) {
            System.out.println(e.getMessage());
        } finally {

        }
    }

    private String getHashChecksum() {
        FileInputStream fis = null;
        try {
            MessageDigest md = MessageDigest.getInstance(HASH_ALG);
            fis = new FileInputStream(this.filePath);
            byte[] byteArray = new byte[1024];
            int bytesCount = 0;
            while ((bytesCount = fis.read(byteArray)) >= 0)
                md.update(byteArray, 0, bytesCount);
            byte[] hash = md.digest();
            fis.close();
            return String.format("%032x",new BigInteger(1, hash));
        } catch (Exception e) {
            e.printStackTrace();
            throw new RuntimeException("Error in checksum");
        } finally {
            try {
                fis.close();
            } catch (IOException e) {
                throw new RuntimeException("Error in checksum. Provided file can't be closed.");
            }
        }
    }

    private Socket connectToServer() throws Exception{
        try {
            Socket client = new Socket(this.serverMachine, this.serverPort);
            return client;
        } catch (IOException e) {
            throw new Exception("Server can't be reached");
        }
    }

    private CipherOutputStream initCipherStream(Socket client) {
        SecretKeySpec keySpec = new SecretKeySpec(hex2byte(KEY), ALGORITHM);
        Cipher cipher;
        try {
            cipher = Cipher.getInstance(ALGORITHM + "/ECB/PKCS5PADDING");
            cipher.init(Cipher.ENCRYPT_MODE, keySpec);
            return new CipherOutputStream(client.getOutputStream(), cipher);
        } catch (Exception  e) {
            e.printStackTrace();
            throw new RuntimeException("Error in cipher stream initialization.");
        }

    }

    private void transferData(CipherOutputStream cipherOut, String checksum) {
        FileInputStream fileReader = null;
        try {
            //Missing here: send MD5 checksum. TODO
            fileReader = new FileInputStream(this.filePath);
            byte[] fileBuffer = new byte[1024];
            int len;
            while ((len = fileReader.read(fileBuffer)) != -1)
                cipherOut.write(fileBuffer, 0, len);
        } catch (Exception e) {
            throw new RuntimeException("File " + this.filePath + " not found");
        } finally{
            try {
                if(fileReader != null) fileReader.close();
            } catch (IOException e) {
                throw new RuntimeException("Socket stream can't be closed");
            }
        }
    }

    private byte[] hex2byte(String s) {
        return DatatypeConverter.parseHexBinary(s);
    }

    public static void main(String [] args) throws Exception {
        FileSender fileSender = new FileSender(FILE, "localhost", 8081);
        fileSender.sendFile();
    }
}

try-with-resources

        FileInputStream fis = null;
        try {
            MessageDigest md = MessageDigest.getInstance(HASH_ALG);
            fis = new FileInputStream(this.filePath);
            byte[] byteArray = new byte[1024];
            int bytesCount = 0;
            while ((bytesCount = fis.read(byteArray)) >= 0)
                md.update(byteArray, 0, bytesCount);
            byte[] hash = md.digest();
            fis.close();
            return String.format("%032x",new BigInteger(1, hash));
        } catch (Exception e) {
            e.printStackTrace();
            throw new RuntimeException("Error in checksum");
        } finally {
            try {
                fis.close();
            } catch (IOException e) {
                throw new RuntimeException("Error in checksum. Provided file can't be closed.");
            }
        }

In a normal run, you try to close the stream twice.  You do not need to call close() before the return.  The finally block will take care of it.

There is also an issue.  If there is an exception in the getInstance call, you will attempt to dereference a null pointer in the finally block.

You can rewrite this

        try (FileInputStream fis = new FileInputStream(this.filePath)) {
            MessageDigest md = MessageDigest.getInstance(HASH_ALG);

            byte[] bytes = new byte[1024];
            for (int bytesCount = fis.read(bytes); bytesCount >= 0; bytesCount = fis.read(bytes)) {
                md.update(byteArray, 0, bytesCount);
            }

            return String.format("%032x", new BigInteger(1, md.digest()));
        } catch (Exception e) {
            e.printStackTrace();
            throw new RuntimeException("Error in checksum");
        }

This uses a try-with-resources so that you don't have to manually close the resource.

I prefer names like bytes to byteArray as being simpler and more focused on what the data means logically than how it is stored physically.  You can read more about this in discussions of Hungarian notation.  This is somewhat controversial.  I mostly mention it because the code was scrolling when it was named the Hungarian notation way.

Using the for loop to replace the while reduces the scope of the bytesCount variable.  Other than that, both do the the same thing.

I added some vertical whitespace to break things up.

It's not necessary to assign the result of md.digest() to a variable.  We can pass it directly to the BigInteger constructor.  Either way works though.


jQuery animation and rotation effects for a WordPress site What is the best way to tidy up JavaScript code I have written? It seems very long winded, any suggestions to shorten it?

Note: I'm running it on Wordpress which runs jQuery in nonconflict mode, hence the insane amounts of jQuery.().

jQuery("document").ready(function (jQuery) {

    if (jQuery(window).width() < 960) {
        jQuery(".main").animate({
            "top": "22%"
        });
        jQuery('#tblcontents').toggle(function () {
            jQuery(".main").fadeOut(1000);
        }, function () {
            jQuery(".main").fadeIn(1000);
            jQuery(".main").animate({
                "top": "22%"
            });

        });

    } else {}

    jQuery('.menu-button').click(function () {
        jQuery("#sb-container div").css("transform", "rotate(0deg)");
        jQuery("#sb-container div").css("-webkit-transform", "rotate(0deg)");
        jQuery("#sb-container div").css("-ms-transform", "rotate(0deg)");
        jQuery("#sb-container div").css("-moz-transform", "rotate(0deg)");
        jQuery("#sb-container div").css("-o-transform", "rotate(0deg)");
        jQuery(".main").animate({
            "right": "1%"
        }, "slow");
    });

    jQuery(function () {
        jQuery('.toggle_div').toggle(function () {
            jQuery(".main").animate({
                "right": "50%"
            }, "slow");
        }, function () {
            jQuery(".main").animate({
                "right": "1%"
            }, "slow");
        });
    });

    jQuery(".pulse").effect("pulsate", {
        times: 100
    }, 2000).click(function () {
        jQuery(".pulse").effect().stop(true, true);
        jQuery(".pulse").animate({
            "opacity": "1"
        }, "fast");
    });
    jQuery('.thumb').click(function () {
        jQuery("#sb-container div").css("transform", "rotate(0deg)");
        jQuery("#sb-container div").css("-webkit-transform", "rotate(0deg)");
        jQuery("#sb-container div").css("-ms-transform", "rotate(0deg)");
        jQuery("#sb-container div").css("-moz-transform", "rotate(0deg)");
        jQuery("#sb-container div").css("-o-transform", "rotate(0deg)");
        jQuery("#sb-container").animate({
            "left": "0px"
        }, "slow");
        jQuery(".main").animate({
            "right": "1%"
        }, "slow");
    });

    jQuery(window).scroll(function () {
        if (jQuery(this).scrollTop() > 20) {
            jQuery("#sb-container div").css("transform", "rotate(0deg)");
            jQuery("#sb-container div").css("-webkit-transform", "rotate(0deg)");
            jQuery("#sb-container div").css("-ms-transform", "rotate(0deg)");
            jQuery("#sb-container div").css("-moz-transform", "rotate(0deg)");
            jQuery("#sb-container div").css("-o-transform", "rotate(0deg)");
            jQuery(".main").animate({
                "right": "1%"
            }, "slow");

        } else {
            jQuery("#sb-container").draggable()
        }
    });

});

jQuery(function () {
    jQuery("#sb-container").draggable()
});

How do I go about reducing the amount of code shown here? What are the best practices and what am I missing? Also, do I need the else statements querying the window width since they don't have any functions in them for the most part?

I'm going to go through a few main points here about your code and at the bottom I'll have a "what I would change" section with inline comments. Do not be overwhelmed by the size of this book, I prefer to be on the side of too much information than too little.

- Cache your selectors: Probably the most important thing you can do for your code now. As a rule of thumb, if you use a selection more than once, you should cache it. What happens when you use $("someElem") is jQuery queries the DOM to try and find elements that match. So imagine that every time you do that it runs a search. Would it be better if you could save the search results? This way you can look and play with them whenever you want.  Ex.:

$("#sb-container div").css("transform", "rotate(0deg)"  );
$("#sb-container div").css("-webkit-transform", "rotate(0deg)"  );

//Should be like this:
var sbContainer = $("#sb-container div"); //Saved my search to a variable, which I use later.
sbContainer.css("transform", "rotate(0deg)"  );
sbContainer.css("-webkit-transform", "rotate(0deg)"  );

- You can use $ in Wordpress: If you have your code in the footer (which you should be doing) what you'll want to do is wrap it in a IIFE. "What the hell is that?" you may ask. A basic syntax looks like this:

(function () {
    //Code goes in here.
})();

You'll see several variations of this all over the internet as you read to find out more. The one I recommend you use in this case is like this:

;(function ($, window, document) {
    //Code goes here and the $ belongs to jQuery
})(jQuery, window, document);

So lets break this down. The ; in the front is a safety net against unclosed scripts, which can be common if you use plugins in Wordpress, or if you concat and minify your files it will protect you from your function becoming an argument.

Then we pass in $, window, and document. We pass in $ and at the bottom assign it to jQuery. So now in this function, no matter what value the $ carried outside, in here it's jQuery. Then we pass in window and document. These are optional in your case, but a good idea since you do use references to window in your code. It saves window as a local variable, and also will be good when you minify your code as the window reference can be changed to something like a or foo automatically.

Now keep in mind there might be times where you have to put your script in the header. Modernizr is an example of this. Then you'll probably end up using the document.ready. Don't sweat because all you have to do is this:

jQuery(document).ready(function( $ ) {
    //Code goes here and $ will belong to jQuery.
});

I wouldn't rely on $.noConflict

- Chaining: As pointed out in your question, you should be using something like that. jQuery is particularly good at this and you should make the most of it. Ex.:

$("#someElem").css("background-color", "#fff").animate({"right":"1%"}, "slow").draggable();

I apply all this stuff to this selector, one after the other. If I want to continue chaining but change my selectors, I can use stuff like .find() and continue with my methods.

- Save function calls: If you read the jQuery source code you'll see that shortcut methods like .click(), .scroll(), and etc. all reference the .on() method. All they do is basically call the .on() method with some variances. Well why not just go straight to the meat and potatoes? Check this out:

$("#foo").click(function() {
    //Do your awesomeness
});

//That does the same thing as this:
$("foo").on("click", function() {
    //Do your awesomeness even awesomener.
});

I highly recommend, if you're going to use jQuery on a regular basis, that you take some time and read through the source code of the methods you're using. To find them quickly just use CTRL+F and type method:. That should jump you straight to what you want to know. This way you can understand what and how you are doing things, and even find better ways to do them on your own.

-Playing with visible elements: You're if statements from what I can tell are trying to detect scroll position and etc. I highly recommend you check out this tiny plugin. It basically detects if an element is visible or not at any given time. This may or may not help you out but I just thought I'd throw that out there.

-Things I'd write differently: Here's an example of all of this summed up into your actual code. I've included comments so you can relate to the reading part.

;(function ($, window) { //Here's our IIFE giving us full $ power
    //Right here at the top we declare our selection variables to be used throught the code.
    //Don't forget to replace your selectors with the appropriate variable name.
    var main = $(".main"),
        sbContainer = $("#sb-container"),
        sbContainerDiv = sbContainer.find("div");

    function animate() { //Write your code only once, and call it when you need it.
        sbContainerDiv.css({ //You should apply a class instead of doing all of this in here like this.
            "transform": "rotate(0deg)",
            "-webkit-transform": "rotate(0deg)",
            "-ms-transform": "rotate(0deg)",
            "-moz-transform": "rotate(0deg)",
            "-o-transform": "rotate(0deg)"
        });

        main.animate({"right":"1%"}, "slow");
    }

    if ( window.width() < 960) {
        main.animate({"top":"22%"});

        //This use of .toggle() was deprecated in jQuery 1.8 and removed in 1.9
        //Consider doing this differently
        //If you provide me with more details on this I can help you come up with a different solution
        $('#tblcontents').toggle(function() {
            main.fadeOut(1000);
        }, function() {
            main.fadeIn(1000);
            main.animate({"top":"22%"});
        });
    } else {
        $('.menu-button').on('click', function() {
            animate(); //Here we call our previously set animate function
        });
    }

    //Refer to the previous toggle comment.
    $('.toggle_div').toggle(function() {
        main.animate({"right":"50%"}, "slow");
    }, function(){
        main.animate({"right":"1%"}, "slow");
    });

    $(".pulse").effect("pulsate", { times:100 }, 2000).on('click', function() {
        $(this).effect().stop(true, true); //In a callback function, 'this' refers to the element in question.
        $(this).animate({"opacity":"1"}, "fast");
    });

    $('.thumb').on('click', function() {
        animate();
    });

    window.on('scroll', function() {
        if ($(this).scrollTop() > 20) {
            animate();
        } else{
            sbContainer.draggable();
        }
    });
    //You only need to make this draggable once, either here or in your else statement.
    //Remove one depending on what you need.
    sbContainer.draggable()

})(jQuery, window); //Here we set our awesome stuff in the function

UPDATE

I've looked at your .js file and there's quite a bit I would change. But focus on the resize event for now. The way you have it now, the code runs hundreds of times since it is called each time the window size changes, even if the user isn't done resizing.

Now we don't want to do that since it can really slow down the browser, if not crash it all together. So what you want to do is only run when the user stops or is done resizing, then run the code. Here's an example of how I'd probably do it:

var $window = $(window); //Remember, if you use a selection more than once, you should cache it.
$window.resize(function() {
    //Check if there has been a call already
    //If the check is true that means the code was already called
    //So we cancel any previous calls, this way the code is only called once
    if(this.resizeTO) clearTimeout(this.resizeTO);

    this.resizeTO = setTimeout(function() {
        $(this).trigger('resizeEnd'); //Call the actual code with a 500ms delay
    }, 500);
});

$window.on('resizeEnd', function() {
//This is a custom event that is triggered in the timeout
//This will only run on the last time the resize method was called
    var winWidth = $window.width();

    if( winWidth < 960 ) {
        toggletwo();
        animate();
    } else {
        toggleone();
    }
});


Beginning C character array explanation I have been following along the book 'The C Programming Language' 2nd edition by Kernighan and Ritchie. The below code is an exercise from the book. What I am confused about in this code is the Character array 'line'. It is a local variable so I don't understand how the value is changing when it is used as an argument in the 'get_line' function.

/*******************************
 * Author:                     *
 * Date: 4/22/2019             *
 * Purpose: Book exercise      *
 *******************************/

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

#define MAX_LINE 1000

int get_line(char line[], int maxline);
void copy(char to[], char from[]);

/*Print longest input line*/
int main()
{
    int len;    //Current line length
    int max;    //Maximum length seen so far

    char line[MAX_LINE];        //Current input line
    char longest[MAX_LINE];     //Longest line saved here

    max = 0;

    //While input length is greater than 0
    while((len = get_line(line, MAX_LINE)) > 0)
    {
        //If current string is longer than max
        if (len > max)
        {
            max = len;              //Assign new max value
            copy(longest, line);    //Replace longest string value with current string
        }
    }

    if (max > 0)
    {
        printf("%s", longest);
    }
    return 0;
}

int get_line(char s[], int lim)
{
    int c, i;

    /*i = 0; i less than limit - 1 *and* input is not EOF *and* input is not enter key */
    for (i=0; i<lim - 1 && (c=getchar())!=EOF && c!='\n'; i++)
    {
        s[i] = c; // input array position i is = input
    }

    //Once enter key is hit
    if (c == '\n')
    {
        s[i] = c;   //Add enter key '\n' onto array
        i++;        //Incr i so that last char can be '\0' later
    }

    s[i] = '\0';    //The last spot on char array is '\0'
    return i;
}

void copy(char to[], char from[])
{
    int i;

    i = 0;

    //Assign from[i] to to[i] until null terminator
    while((to[i] = from[i]) != '\0')
    {
        i++;
    }
}

For example if I had a variable in the main routine

int add_num(int num1);
main()
{
    int x = 5;
    add_num(x);
    printf("%d", x);
}

int add_num(int num1)
{
    num1 += 5;
    return num1;
}

x would still remain equal to 5 unless I did:

x = add_num(x);

then x would equal 10.

So how does the line character array change when the function that it's used in returns 'i'?
Where I is the length of the string entered.

Sorry if this was a little long, but I wanted to explain myself clearly.

From the end of the chapter 1.8:

  "When the name of an array is used as an argument, the value passed to
  the function is the location or address of the beginning of the array
  - there is no copying of array elements. By subscripting this value, the function can access and alter any element of the array".

So, when you pass array as an argument to a function, you don't create a local copy of array. Instead, you create a pointer (Chapter 5) and work with it as with an array.


Calculating neuron outputs and derivatives This function runs very often.  cudaMemcpy is at the start and works very slowly. How can I change this function to avoid this? I already have inputs in device memory.

void OpenNNL::calculateNeuronsOutputsAndDerivatives(double * inputs, double * deviceOutputs, double * deviceDerivatives)
{
    int inputsCount = _inputsCount;

    double * deviceTemp;
    double * deviceInputs;

    cudaCall(cudaMalloc ( (void**)&deviceInputs, inputsCount*sizeof(double) ));

    cudaCall(cudaMemcpy ( deviceInputs, inputs, inputsCount*sizeof(double), cudaMemcpyDeviceToDevice ));

    for(int i=0;i<_layersCount;i++)
    {
        cudaCall(cudaMalloc((void**)&deviceTemp, _neuronsPerLayerCount[i]*inputsCount*sizeof(double)));

        dim3 threadsMul = dim3(BLOCK_SIZE, 1);
        int blocksCount = floor((double) _neuronsPerLayerCount[i]*inputsCount / threadsMul.x) + 1;
        dim3 blocksMul = dim3(blocksCount, 1);

        weighting<<<blocksMul, threadsMul>>>(deviceTemp, deviceInputs, _neuronsInputsWeights, _inputsInPreviousLayers[i], inputsCount, _neuronsPerLayerCount[i]);

        cudaCall(cudaFree(deviceInputs));

        cudaCall(cudaMalloc((void**)&deviceInputs, _neuronsPerLayerCount[i]*sizeof(double)));

        dim3 threadsSum = dim3(BLOCK_SIZE, 1);
        blocksCount = floor((double) _neuronsPerLayerCount[i] / threadsSum.x) + 1;
        dim3 blocksSum = dim3(blocksCount, 1);

        calculateOutputsAndDerivatives<<<blocksSum, threadsSum>>>(deviceOutputs, deviceDerivatives, deviceInputs, deviceTemp, _neuronsBiases, inputsCount, _neuronsPerLayerCount[i], _neuronsInPreviousLayers[i]);

        inputsCount = _neuronsPerLayerCount[i];

        cudaCall(cudaFree(deviceTemp));
    }

    cudaCall(cudaFree(deviceInputs));
}

Try to minimaze memory allocations.

Allocate memory for deviceTemp and deviceInputs only once (in the constructor, for example):

cudaCall(cudaMalloc ( (void**)&deviceInputs, some_big_value * sizeof(double) ));
cudaCall(cudaMalloc((void**)&deviceTemp, some_big_value * sizeof(double)));

And in calculateNeuronsOutputsAndDerivatives, reallocate memory only if needed:

if (cur_deviceInputs_size < inputsCount)
{
    cudaCall(cudaFree(deviceInputs));
    cudaCall(cudaMalloc ( (void**)&deviceInputs, inputsCount*sizeof(double) ));
    cur_deviceInputs_size = inputsCount;
}


Sherlock is going against the Beast with PowerShell I ran into this challenge a couple of times today and thought I could tackle it with PowerShell. I refused to look at other peoples approaches to this hoping to complete it on my own. I'm sure the general idea is the same in either case. I have included more comments than I would normally to help people not familiar with the language/challenge.

  A Decent Number has the following properties:

  Its digits can only be 3's and/or 5's.
  The number of 3's it contains is divisible by 5.
  The number of 5's it contains is divisible by 3.
  If there are more than one such number, we pick the largest one.

  Constraints

  1≤T≤20

  1≤N≤100000

  Input Format

  The first line is an integer, T, denoting the number of test cases.

  The T subsequent lines each contain an integer, N, detailing the number of digits in the number.

  Output Format

  Print the largest Decent Number having N digits; if no such number exists, tell Sherlock by printing -1.

# Split the lines in the string passed as string array. Cast each value as an integer
$inputNumbers = $args[0] -split "`r`n" | ForEach-Object{[int]$_}

# Cycle the numbers. Amount determined by the first line.
for($inputIndex=1;$inputIndex -le $inputNumbers[0];$inputIndex++){
    # Create a loop that runs for the number of passes in the current line of input.
    # Count down from the number until 0
    $highestNumber = $inputNumbers[$inputIndex]..0 | ForEach-Object{
        # Check to see if this digit combination is a decent number.
        # If it is then we pass it thru the pipeline and stop the loop.
        If(($_ % 3) -eq 0 -and (($inputNumbers[$inputIndex] - $_) % 5) -eq 0){
            ("5" * $_).PadRight($inputNumbers[$inputIndex],"3")
            # If we get a result then this is the highest. Exit the loop.
            continue
        }
    }

    # Check the value of $highestNumber. It is possible that for this pass there
    # are no decent numbers. Nulls evaluate to false.
    if($highestNumber){$highestNumber}else{-1}
}

Usage

Save the above code as a ps1 file. Run it passing a multi-line string parameter.

.\Get-DecentNumbers.ps1 "4
1
3
5
11"

I'm no powershell expert but if spaces sprinkled around here and there would make the code more readable. So instead of this:

for($inputIndex=1;$inputIndex -le $inputNumbers[0];$inputIndex++){

it would read like this:

for (inputIndex = 1; $inputIndex -le $inputNumbers[0]; $inputIndex++) {

Less crammed and easier to read I'd say.
Your outer for loop could be expresses as a ForEach which would yield cleaner code:

ForEach ($N in $inputNumbers) {
    # Create a loop that runs for the number of passes in the current line of input.
    # Count down from the number until 0
    $highestNumber = $N..0 | ForEach-Object {
        # Check to see if this digit combination is a decent number.
        # If it is then we pass it thru the pipeline and stop the loop.
        If (($_ % 3) -eq 0 -and (($N - $_) % 5) -eq 0) {
            ("5" * $_).PadRight($N, "3")
            # If we get a result then this is the highest. Exit the loop.
            continue
        }
    }

    # Check the value of $highestNumber. It is possible that for this pass there
    # are no decent numbers. Nulls evaluate to false.
    if ($highestNumber) { $highestNumber } else { -1 }
}

As for the algorithm: You kind of brute force it which is usually not the intend of these programming challenges. I have outlined a slightly better algorithm in my answer to another question trying to solve the same problem.


How would I unit test a method that gets data from an RSS feed? I have a method that gets an RSS feed using LINQ to XML and returns a collection of an object that grabs only the data I need. There's really only one other method in the class I'm concerned about that you'll see in the sample -- RemoveUrlNoise.

How do I test a method like this or change it to be more testable?

    public IEnumerable<PostDetails> GetPostDetails(string rssUrl)
    {
        XElement feed = XElement.Load(rssUrl);

        var items = from item in feed.Elements("channel").Elements("item")
                    select item;

        return items.Select(item => new PostDetails
            {
                Title = item.Element("title").Value,
                Url = RemoveUrlNoise(item.Element("guid").Value),
                PubDate = item.Element("pubDate").Value
            }).OrderBy(i => i.PubDateAsDateTime).ToList();
    }

Separate the concerns of fetching the feed XML from parsing it by extracting a class (e.g. FeedReader) to do the reading. Your IFeedReader would be generically reusable and look something like:

public interface IFeedReader
{
    XElement Load(string url);
}

with implementation

public class FeedReader : IFeedReader
{
    public XElement Load(string url)
    {
        return XElement.Load(url);
    }
}

You then inject an IFeedReader through the constructor and use it to fetch the data in your method. This lets you use a Fake or a Mock of the IFeedReader, depending on your preference, to return appropriate results for your unit test.

consider reading up on: inversion of control (IoC) and dependency injection (DI), and mock object frameworks.


Is this factory method a good approach? We are beginning to learn Node.js and we are not always sure if we do it the right way. We have a strong object oriented background so the adoption is not always easy. Right now we implemented a factory method, but we are not completely sure if our approach is good.

Factory Method:

var fs = require('fs');

module.exports.getHandler = function(type,callback) {
    if(!type) {
        throw new Error('Please specifiy a type');
    }
    var nodeModule = type.ucFirst()+'FieldHandler';
    var file = __dirname+'/'+nodeModule+'.js';
    fs.exists(file, function(exists) {
        if (exists) {
            var handler = require('./'+nodeModule);
            var instance = new handler;
            callback(instance);
        } else {
            throw new Error('Handler for "'+type+'" does not exist. ' +
                'Expect file of handler to have the name "'+file+'"');
        }
    });
};

Object which uses the factory method:

var FieldhandlerFactory = require('./fieldhandler/Factory');
...
...
...
var handleField = function (actualField) {
    var callbackHandler = function (handler) {
        handler.setJson(actualField);
        handler.check();
        fieldString += handler.toView() + '\n';
        actual++;
        if (amountFields == actual) {
            // WE ARE READY DO SOMETHING
        }
    };
    var handler = FieldhandlerFactory.getHandler(type, callbackHandler);
}
for (var index in fields) {
    var field = fields[index];
    type = field.type;
    if (!type) {
        throw new Error('Field on index "' + index + '" has no attribute type');
    }
    var string = handleField(field);
}
...
...
...

The code works, so we are quite happy.  Is this a good approach or do we stick too much to the classical OOP style? Especially with testing we face some problems. How to test the function handleField isolated? And with jasmine-node I'm not able to catch the exception in the factory method when the file is not found.

So we think there are tons of improvements. We are looking forward to learn from you and to exchange experiences.

When using callbacks, the first parameter is the error. So instead of throwing the error, you should pass it as the first argument. The signature of the callback should look like this function(err, all, other, arguments).

So instead of this

var fs = require('fs');

module.exports.getHandler = function(type,callback) {
    if(!type) {
        throw new Error('Please specifiy a type');
    }
    var nodeModule = type.ucFirst()+'FieldHandler';
    var file = __dirname+'/'+nodeModule+'.js';
    fs.exists(file, function(exists) {
        if (exists) {
            var handler = require('./'+nodeModule);
            var instance = new handler;
            callback(instance);
        } else {
            throw new Error('Handler for "'+type+'" does not exist. ' +
                'Expect file of handler to have the name "'+file+'"');
        }
    });
};

You should have something like

var fs = require('fs');

module.exports.getHandler = function(type,callback) {
    if(!type) {
        callback(new Error('Please specifiy a type');
    }
    var nodeModule = type.ucFirst()+'FieldHandler';
    var file = __dirname+'/'+nodeModule+'.js';
    fs.exists(file, function(exists) {
        if (exists) {
            var handler = require('./'+nodeModule);
            var instance = new handler;
            callback(null, instance);
        } else {
            callback(new Error('Handler for "'+type+'" does not exist. ' +
                'Expect file of handler to have the name "'+file+'"');
        }
    });
};

Then it should become pretty easy to test. As a bonus, you can have a look at https://github.com/cjohansen/Sinon.JS. It's a good module for mocks, stubs and spies.

Also, I realized that you are doing a synchronous for loop calling asynchronous functions. var aString = handleField(field); will never do what you are expected. handleField does not return anything. You should expect it in a callback. If you want a tip to handle the flow properly, use https://github.com/caolan/async. With this module, you will stay away of the mythical "callback-hell".

Enjoy, node.js!


onChange vs onKeyPress for input in React I have an input form, and when you press enter I want it to call a function. I was wondering what "the right" way of doing this is.

Right now I have an input looking like this:

<input
    type="text"
    placeholder="Enter key words"
    value={this.state.value}
    onChange={this.handleChange}
    onKeyPress={this.handleEnter}
/>

And two functions that look like this

handleChange(event) {
    this.setState({value: event.target.value});
}

handleEnter(event) {
    if (event.key === 'Enter') {
        this.setState({value: event.target.value},function(){
            this.props.theFunction(this.state.value);
        });
    }
}

Another solution was to point both onChange and onKeyPress to the same function and write it like

handleChangeAndEnter(event) {
    if (event.key === 'Enter') {
        this.setState({value: event.target.value},function(){
            this.props.theFunction(this.state.value);
        });
    } else {
        this.setState({value: event.target.value});
    }
}

If I remove the onChange-binding the input-field stops updating itself on input (not entirely sure why this is) and if I remove the onKeyPress-binding the event-object no longer has a key-property.

My current solution works, but it still feels like a hack.

Don't complicate your code; the first solution is just fine but needs some tweaking:
handleChange(event) {
    this.setState({value: event.target.value});
}

handleEnter(event) {
    if (event.key === 'Enter') {
        this.props.theFunction(this.state.value);
    }
}

Or, you can use what they call a guard clause and make it short and readable:
handleEnter(event) {
    if (event.key !== 'Enter') return;
    this.props.theFunction(this.state.value);
}

And if you're using react hooks:
const handleChange = (e) => {
    setInputValue(e.target.value);
}

const handleEnter = (e) => {
    if (e.key === "Enter") {
        if (value !== "") {
            console.log("Hello", name);
        }
        else {
            alert("can't pass empty string");
        }
        setInputValue("");
    }
}


Updating list of Dictionaries from other such list Basically I have created a function which takes two lists of dicts, for example:

oldList = [{'a':2}, {'v':2}]
newList = [{'a':4},{'c':4},{'e':5}]

My aim is to check for each dictionary key in the oldList and if it has the same dictionary key as in the newList update the dictionary, else append to the oldList.
So in this case the key 'a' from oldList will get updated with the value 4, also since the keys b and e from the newList don't exist in the oldList append the dictionary to the oldList. Therefore you get [{'a': 4}, {'v': 2}, {'b': 4}, {'e': 5}].

Is there a better way to do this?

def sortList(oldList, newList):
    for new in newList: #{'a':4},{'c':4},{'e':5}
        isAdd = True
        for old in oldList: #{'a':2}, {'v':2}
            if new.keys()[0] == old.keys()[0]: #a == a
                isAdd = False
                old.update(new) # update dict
        if isAdd:
            oldList.append(new) #if value not in oldList append to it
    return oldList

sortedDict = sortList([{'a':2}, {'v':2}],[{'a':4},{'b':4},{'e':5}])
print sortedDict

[{'a': 4}, {'v': 2}, {'b': 4}, {'e': 5}]

A list of 1-pair dictionaries makes no sense as data structure. You should join them to create a single dictionary. This way all your code reduces to:

d1 = {'a': 2, 'b': 2}
d2 = {'a': 4, 'c': 4, 'e': 5}
d3 = dict(d1, **d2)
# {'a': 4, 'b': 2, 'c': 4, 'e': 5}

If you are going to union/merge dicts a lot, you can create a handy abstraction, more general than dict(d1, **d2):

def merge_dicts(d1, d2):
    return dict(itertools.chain(d1.items(), d2.items()))

To convert list of dictionaries to a dictionary is simple:

d2 = dict(list(d.items())[0] for d in new_list)
# {'a': 4, 'c': 4, 'e': 5}


Fill a DataSet with nearly 20,0000 records using SqlDataAdapter I want to fill a DataSet with 20,0000 records using a SqlDataAdapter by using this code:

adapter.Fill(dataset);

If I fill a DataSet this way it takes a long time. Is there anything wrong with this?

This is my code:

connection = new SqlConnection(ConfigurationSettings.AppSettings["ConnectionString"]);
connection.Open();

SqlCommand command = new SqlCommand("sp_DMS_Report_Generate", connection);
command.CommandType = CommandType.StoredProcedure;
command.Parameters.AddWithValue("@XML", reportCriteria.XML);
command.CommandTimeout = 1000;

SqlDataAdapter adapter = new SqlDataAdapter(command);
adapter.Fill(resultSet);

return resultSet.Tables[0];

I would say the C# is fine, it's your database that is the issue.  Is the Stored procedure optimized?  Are the tables in the proper normalization?  Are your primary keys & foreign keys set up properly? (you should get the point by now)

Start there first since really everything is there.


Reading a line of text from a socket I tend to write code, with very few comments, and few line breaks. I'm currently just a student at a University. I've had professors tell me one way or another to comment, but I have a hard time with more ambiguous tasks.

I have a misc. snippet of code, written by me in C++. I'm not looking for revisions on it or anything. Just how, in your professional field, would you go about commenting it?

CString Socket::recvln()
{
    int size, count;
    char* result;

    OSFactory::lockMutex(recvLock);
    size = recvBuffer.size();
    result = new char[size+1];
    for (count = 0; count < size; count++)
        if (recvBuffer.at(count) == '\n')
            break;
        else
            result[count] = recvBuffer.at(count);

    if (count >= size) // Should never >= size if '\n' found
        count = 0;
    result[count] = '\0';
    recvBuffer.erase(recvBuffer.begin(), recvBuffer.begin()+count);
    OSFactory::unlockMutex(recvLock);

    std::string _result = result;
    delete[] result;
    return _result;
}

Again, this is just a misc. method I have. I'm just trying to understand how other people would go about commenting code. There are tons of tutorials out there, but many even have you comment getter/setter methods, and to me that's really really pointless. Many times it's very clear that setName(std::string), for example, sets the Name!

So, I'm just trying to have a better grasp on real life commenting, versus text-book answers. For some reason this is really hard for me, and I'm sure it should benefit other people as well.

I don't see anything in this code that needs to be commented. To echo what other people said, comments should clarify code and they should never say what the code already says.

I would recommend commenting code under these conditions:

You can't tell what the code is doing by reading it
You encountered a wonky bug that took you a long time to figure out, and the fix does not look clear or uses a programming construct or pattern that isn't normally used.

Really, the biggest problem I see is one of the biggest problems in Software Engineering. As Martin Fowler once said:

  ... there are two hard things in computer science: cache invalidation, naming things, and off-by-one errors.

The names of your variables and methods could use a little polishing.

For instance, does Socket::recvln() mean:

Receive Line
Receive Lane
Recover Line...

Even if names become a little long, make them obvious. Don't abbreviate. Some people say that abbreviating well known terms within your industry are OK. I'm from the camp that disagrees. Never assume that the next programmer is an expert in your industry.

When it comes to "reducing lines" I have mixed feelings about omitting optional curly brackets. It looks nicer, but becomes extra typing when you have to add one line of code. I go back and forth on that and have come to a compromise with myself.

If omitting the curly brackets makes the code a two-liner, that's OK

if (count >= size)
    count = 0;

If I would be omitting curly brackets for nested structures, I'll put the curly brackets in even if I don't need to

for (count = 0; count < size; count++)
{
    if (recvBuffer.at(count) == '\n')
    {
        break;
    }
    else
    {
        result[count] = recvBuffer.at(count);
    }
}

But sometimes within a nested structure, I'll omit the optional brackets if it is obvious:

for (count = 0; count < size; count++)
{
    if (recvBuffer.at(count) == '\n')
        break;
    else
        result[count] = recvBuffer.at(count);
}

If I have a structure with one-liners and multi liners, I never omit the curly brackets:

if (count > size)
{
    count = 0;
}
else
{
    foo = count + 10;
    bar = foo * -1;
}

Lastly, I recommend putting spaces around operators to make the code easier to read:

recvBuffer.erase(recvBuffer.begin(), recvBuffer.begin() + count);

(notice the spaces around the "+" sign)


Separating digits in an Integer I was reading a Java book and I came up with this kind of problem and I tried to solve it:

  Problem: Write an application that inputs one number consisting of five digits from the user, separates the number into its individual
  digits and prints the digits separated from one another by three
  spaces each. For example,

  Input:

42339

  Output:

4   2   3   3   9

Solution:

package practice;

import java.io.IOException;
import static java.lang.System.in;
import java.util.InputMismatchException;
import java.util.Scanner;

public class practice {

    public static void main(String[] args) throws IOException {
        try(Scanner input = new Scanner(System.in)){

            System.out.print("");
                int a = input.nextInt();

            char[] newString = Integer.toString(a).trim().toCharArray();

            if (newString.length < 5 || newString.length > 5) {

                System.out.println("Input should be exactly 5.");
                return;

            } else {

                for (int i = 0; i < newString.length; i++) {
                    System.out.print(newString[i] + "   ");
                }
            }

            System.out.println();

         } catch (InputMismatchException e){

             System.out.println("Error: Invalid input.");

        } finally {

            in.close();

        }
    }
}

My questions are:

Did I do something wrong?
Is there any way to solve this problem more efficiently?

Input and validation

Using a Scanner is a good idea, and using a try-with-resources block is the right thing to do.

You don't need to close the Scanner or the underlying System.in.  The whole point of the try-with-resources block is that resources will be automatically closed for you.

Note that neither the Scanner(Readable) constructor nor Scanner.nextInt() will ever throw an IOException.  The only exception you need to catch (but didn't) is NoSuchElementException.  (Well, also InputMismatchException, but that is just a subclass of NoSuchElementException.)  Your program will crash with a NoSuchElementException if you send it an EOF at the prompt.

The error messages aren't quite right.  The first message should read: "Input should be exactly 5 digits."  The second message is unhelpful: you should tell the user what would be considered valid input.

The validation is not quite strict enough: a negative four-digit integer will also be accepted.

Output

Strictly speaking, the output is not quite right, in that you also append three spaces after the fifth digit.  On screen, the output will look the same, but if you pipe the output to a program that examines the bytes, you'll see the trailing spaces.

You don't need to call String.trim().

newString is not a good name: it's not a string, and there is no oldString.  If I had to pick a name, I'd call it digits.

Suggested solution

With Java 8, the simplest solution is to use String.join().

public static void main(String[] args) {
    try (Scanner input = new Scanner(System.in)) {
        int n = input.nextInt();
        if (!(1e5 <= n && n < 1e6)) {
            System.out.println("Input should be exactly 5 digits.");
            return;
        }
        System.out.println(String.join("   ", String.valueOf(n).split("")));
    } catch (NoSuchElementException e) {
        System.out.println("Error: No integer was given.");
    }
}


Split every vector in a sequence into two parts: old elements that were contained in previous vectors and new elements never before seen Am I doing it right? Is there some function in the standard library I should be using? This is my first real(ish) world Clojure program…

Input

(def data
  [[{:id 5 :name "a"} {:id 1 :name "b"}], [{:id 4 :name "c"} {:id 1 :name "d"}]])

Code

The elements are maps and they are compared based on their :id.

(def precomputed
  (reductions
   (fn [global, v]
     (let [seen? (:seen? global)
           new   (remove #(-> % :id seen?) v)
           old   (filter #(-> % :id seen?) v)]
       {:seen? (clojure.set/union seen? (->> new (map :id) (set)))
        :new   new
        :old   old}))
   {:seen? #{} :new '() :old '()}
   data))

At this moment precomputed looks like

({:seen? #{}, :new (), :old ()}
{:seen? #{1 5}, :new ({:id 5, :name "a"} {:id 1, :name "b"}), :old ()}
{:seen? #{1 4 5}, :new ({:id 4, :name "c"}), :old ({:id 1, :name "d"})})

Print some sort of output

(doseq [i (rest precomputed)]
  (println "new: " (->> (i :new) (map :name) (interpose ", ") (reduce str))
           "old: " (->> (i :old) (map :name) (interpose ", ") (reduce str))))

Output

new:  a, b old:     ; first vector, every element is new
new:  c old:  d     ; we have seen element with :id 1 before

Concatenating a sequence of strings

You should use (apply str) instead of (reduce str) As str can take multiple arguments, and apply can take a sequence as a second argument. Intermediate results generated by reduce would cause time and space waste.

Since you are using a separator string, use clojure.string/join instead:

(->> (i :new) (map :name) (clojure.string/join ", "))

Naming loop vars

In the above code i is not a good name, I expect i to be some kind of index. If using meaningful names like user, order is not available; you can user curr for current m for map as temporary variables. Important point is principle of least surprise.

Map destructuring

You can also use map destructuring. It is easiest to show it by example:

> (doseq [{:keys [new old]} [{:new 1 :old 2} {:new 3 :old 4}]] (prn old new old))
2 1 2
4 3 4

So similarly here:

(doseq [{:keys [new old]} (rest precomputed)]
  (println "new: " (->> new (map :name) (clojure.string/join ", "))
           "old: " (->> old (map :name) (clojure.string/join ", "))))

Prefer functions over constants

Prefer functions over data. Functions are the basic unit of re-use in clojure. It will be obvious if you try to test precomputed you provided, with multiple example data (in REPL by hand or with run-tests). Other signs precomputed should be a function is:

it's name: precomputed implies a function by name precompute should exist (also/instead).
data is only referred to once, right at the end; and the rest of the form doesn't depend on it.

Decompose precomputed

See values in precomputed does not depend on the previous values of :new and :old but only those of :seen?.

It can be rewritten thus:

(defn seen-ids [data]
  (reductions
      #(clojure.set/union %1 (->> %2 (map :id) set))
    #{}
    data))

You don't need to put parens around set.

You can then get new and old elements separately:

(defn new-elts [seen-ids data]
  (map (fn [seen? v] (remove #(-> % :id seen?) v)) seen-ids data))

Terse function definition

Sometimes you may want to define a function as combination of other functions instead of using fn. Combinators clojure has for this purpose are comp, partial, and juxt.

For example #(-> % :id seen?) is equivalent to (comp %1 :id), which also allows the fn to be replaced by #. You can sometimes use partial to define functions w/o using parameter names, but I do not recommend it to use in top level functions, as parameter names are important developer documentation; but this style may be useful for shortening inline definitions (in let, for etc).

old-elts can be defined using this style, like this (compare with the definition of new-elts above):

(def old-elts (partial map #(filter (comp %1 :id) %2)))