Checking For multiple null references So this question might be a bit opinion based but I am a bit confused, say i have these two code snippets -

if (eventArgs != null && eventArgs is OnZoomEventArgs)
        {
            var zoomEventArgs = eventArgs as OnZoomEventArgs;
            if (zoomEventArgs.focus != null)
            {
                var line = zoomEventArgs.focus.GetComponent<AssemblyLine>();
                if (line != null)
                {
                    SetLine(line);
                } else
                {
                    ResetLine();
                }
            }
            else
            {
                ResetLine();
            }
        }
        else
        {
            ResetLine();
        }

and

var zoomEventArgs_ = eventArgs as OnZoomEventArgs;
        if(zoomEventArgs_.focus != null)
        {
            SetLine(zoomEventArgs_.focus.GetComponent<AssemblyLine>());
        } else
        {
            ResetLine();
        }

Which one is better in the sense of readability and in terms of performance maybe.
My question is should I instead check for null reference this many times or should I instead try to not pass a null reference. Is there any other way I can make these code snippets more readable.
I tried to replace

if (line != null)
                {
                    SetLine(line);
                } else
                {
                    ResetLine();
                }

with ternaries but I cannot as both SetLine() and ResetLine() have void as return type.

The problem

Both examples you show have issues.

Example 1 violates DRY. You have three instances of the same "failure" outcome (in this case ResetLine()). Imagine if the failure outcome requires multiple steps. Now imagine you need to change the failure logic. You'd have to make the same change three times, or you'd end up with unexpected behavior.

Example 2 solves the DRY issue, but it actually fails to properly null check. If zoomEventArgs_ is null, you're going to run into a null reference exception when you try to access zoomEventArgs_.focus. Note that the as conversion returns null when it cannot convert the object to the chosen type.

There is an easy fix for this:

if(zoomEventArgs_?.focus != null)

Note the ?. operator. It is the null propagation operator that will prevent nullreferences from being thrown (and instead returns null). In other words, your if evaluation will be false if either zoomEventArgs_ or zoomEventArgs_.focus is null

The solution

The second example is closer to what you want, but it's not leveraging the best syntax. It can be cleaned up further, without introducing nesting:

if(eventArgs is OnZoomEventArgs zoomEventArgs && zoomEventArgs.focus != null)
{
    SetLine(zoomEventArgs.focus.GetComponent<AssemblyLine>());
} else
{
    ResetLine();
}

eventArgs is OnZoomEventArgs zoomEventArgs will cast the object when it can (and return true), and will pass false if it cannot (thus directing you to the else). Notice that you are able to use this casted object immediately in the same evaluation (zoomEventArgs.focus != null).

I also removed the _ suffix as it does not conform to any convention I've ever heard of.

Your question

  My question is should I instead check for null reference this many times?

Should you check for nulls? Of course, it's part of the validation logic. But you should avoid writing individual failure clauses when the failure logic remains the same in all cases.

  should I instead try to not pass a null reference

Never return null unless you intend to deal with it yourself.

When you intentionally throw a null, you are required to handle the potential nulls being returned. This is the honeytrap of developers. It's very easy to "just return null" and not write any proper handling, but it simply shifts the problem towards the caller, who now has to deal with nulls.
Dealing with nulls, as trivial as it may seem, requires the caller to know if and when nulls are being returned, and it burdens them with the task of dealing with it.

Avoiding wanton null returning as a good practice is further confirmed by the coming update of C#8, where nullable reference types are added specifically to address this "just return null" behavior. The name is a bit confusing, what it actually does is that it makes all reference types non-nullable, unless you explictly allow them to be nullable.

This is analogous to how you today can already choose to use int and int?, for example. This behavior is simply going to be extended to reference types.

  I tried to replace [...] with ternaries but I cannot as both SetLine() and ResetLine() have void as return type.

Don't abuse ternaries as "shorter ifs". If you want to shorten the line count of your if, simply remove the braces (I'm not opposed to that, but this may clash with some style guides who enforce the use of brackets).

Ternaries should only be used in cases where you are assigning a value and would end up with a needlessly bulky if block for what amounts to a simple assigning of a value.

Note that ternaries do not improve code readability on non-trivially readable code.

isActive ? "active" : "inactive" is readable because it is simple (and using an if structure here would be needlessly bulky.
(eventArgs is OnZoomEventArgs zoomEventArgs && zoomEventArgs.focus != null) ? "passed" : "failed" is considerably less readable because it's no longer a trivially readable piece of logic.


How can I simplify this JQuery validation process? How can I simplify this functioning JQuery validation process:
class UserEdit {
  addUser(uobj) {
    if(uobj.adduser == "" && uobj.addfullname == "" && uobj.addemail == "" && uobj.addlevel == "")
    {
        $('#adduserError').show();
        $('#addfullnameError').show();
        $('#addemailError').show();
        $('#adduserlevelError').show();
        return false;
    }
    if(uobj.adduser == "")
    {
        $('#adduserError').show();
        return false;
    }
    if(uobj.addfullname == "")
    {
        $('#addfullnameError').show();
        return false;
    }
    if(uobj.addemail == "" || (uobj.addemail.indexOf("@",0)) == -1 || (uobj.addemail.indexOf(".",0)) == -1 ||  uobj.addemail.length < 6)
    {
        $('#addemailError').show();
        return false;
    }
    else
    {
      $('#loadingDiv').show();
      $.post('process/editUser.php', {uobj:uobj}, function(data)
      {
        var modal = "#addUserModal";
        $('#loadingDiv').hide();
        dataCheck(data, modal);
      });
    }
  }
}

All of the above works as it should.  I just know there must be a way to simplify it.
Note (because I believe someone will ask):  I use the dataCheck function to check the data returned from the PHP process, and then close the modal if the process was successful.  There are a couple of other modules within the class that use the same dataCheck function.  That was my initial attempt to at least simplify how to handle the return data in each module.
Now I want to try to simply the code above.  Any help is appreciated.

I see you removed the calls to $('#loadingDiv').show(); from the many conditional blocks and moved that before the call to the AJAX call - I was going to suggest that.
I see the ecmascript-6 features like the Class syntax are used, so other ES-6 features could be used - like the object initializer shorthand notation - i.e. on this line:

 $.post('process/editUser.php', {uobj:uobj}, function(data)

The second argument can be simplified:
$.post('process/editUser.php', {uobj}, function(data)

There is no need to use the else keyword since all previous conditional blocks have a return statement.
The callback to the AJAX call has a variable modal:

var modal = "#addUserModal";

The variable is only used one time so the value can be used in the call to dataCheck and the variable eliminated. Additionally, unless there is a good reason like needing a global variable, don't use var. Use const and then if you determine re-assignment is necessary use let.
Instead of using .indexOf() to determine if a string does or does not contain a substring, String.prototype.includes() could be used unless browser compatibility is an issue.
I know that the PHP PSRs recommend having opening braces on a new line but this is less common in JavaScript.


Implementing a temporary data context manager I implemented a context manager to handle temporary files and directories.

"""Temporary data management."""

from contextlib import ExitStack
from tempfile import template, NamedTemporaryFile, TemporaryDirectory

__all__ = ['TemporaryDataContext']

class TemporaryDataContext(ExitStack):
    """A context manager for temporary files and folders."""

    def __init__(self, root=None, prefix=template):
        """Initializes the context manager with an
        optional temporary data root and prefix.
        """
        super().__init__()
        self.root = root
        self.prefix = prefix

    def mktemp(self, mode='w+b', encoding=None, suffix=''):
        """Creates and registers a named temporary
        file and returns the file object.
        """
        temp = self.enter_context(NamedTemporaryFile(
            suffix=suffix, prefix=self.prefix, dir=self.root))
        temp.file.close()
        return open(temp.name, mode=mode, encoding=encoding)

    def mkdtemp(self, suffix=''):
        """Creates and registers a temporary
        directory and returns its path.
        """
        return self.enter_context(TemporaryDirectory(
            suffix=suffix, prefix=self.prefix, dir=self.root))

It is being invoked by a data processing API to arbitrarily create temporary files and directories, that need to survive the entire processing chain and will be cleaned up at the end.

I am especially interested in optimizing the mktemp method, which seems awkward to me (though it does its job).

I just realized, that I can simplify mktemp by directly returning the tempfile._TemporaryFileWrapper's buffer, which can be used in a file context for open and closing without deleting the superordinate _TemporaryFileWrapper:

def mktemp(self, mode='w+b', encoding=None, suffix=''):
    """Creates and registers a named temporary
    file and returns the file object.
    """
    return self.enter_context(NamedTemporaryFile(
        mode=mode, encoding=encoding, suffix=suffix, prefix=self.prefix,
        dir=self.root)).file

Update
With the change above, the returned buffer does not behave as expected regarding its attribute name.
To fix that, I introduced a wrapper class:

class NamedFile:
    """Wraps a file buffer and its associated name."""

    def __init__(self, file, name):
        """Sets buffer and name."""
        self.file = file
        self.name = name

    def __enter__(self):
        self.file.__enter__()
        return self

    def __exit__(self, typ, value, traceback):
        return self.file.__exit__(typ, value, traceback)

    def __getattr__(self, attr):
        return getattr(self.file, attr)

    @classmethod
    def from_ntf(cls, named_temporary_file):
        """Creates a named file from the specified NamedTemporaryFile."""
        return cls(named_temporary_file.file, named_temporary_file.name)

And return it instead of the raw buffer accordingly:

def mktemp(self, mode='w+b', encoding=None, suffix=''):
    """Creates and registers a named temporary
    file and returns the file object.
    """
    return NamedFile.from_ntf(self.enter_context(NamedTemporaryFile(
        mode=mode, encoding=encoding, suffix=suffix, prefix=self.prefix,
        dir=self.root)))


"Acro Words" - Creating Acronyms of a Text that are Words I wrote a program that given an input string returns a list of tuples in which each element of the tuple is a word given by dropping some number of characters of the original string, preserving order.

For example:

'valley' -> [('alley',), ('valley',)]
'friction' -> [('fiction',), ('friction',), ('ricin',)]
'university' -> [('unvest',), ('nesty',), ('unsty',), ...]
'freetrade' -> [..., ('erade',), ..., ('free', 'tade'), ..., ('free', 'trade'), ...('feer',), ...]

The algorithm I've written works as follows

Find the powerset of the characters in the string.  This gives us a set of lists of characters, conceptually given by "dropping" some number of characters of the input string, preserving order.  powerset('abc') = {(),('a',),('a', 'b'),('a', 'b', 'c'),('a', 'c'),('b',),('b', 'c'),('c',)}
Next, for each element of the powerset, we perform a "powersplit" in which we find every possible split (or, inversely, concatenation).  powerset(('a', 'b', 'c')) = {('a', 'b', 'c'), ('a', 'bc'), ('ab', 'c'), ('abc',)}
Finally, for each tuple produced by the powersplit, we check if the contents of the tuple are in our dictionary.  contents_in_dictionary(dictionary, ('free', 'trade')) = True

The following is my code

from functools import reduce
from itertools import chain, combinations, islice, zip_longest

def powerset(data):
    return {z for z in chain.from_iterable((x for x in combinations(data, r)) for r in range(len(data)+1))}

def slices(data, indices):
    return [[x for x in islice(data, *list(y))] for y in zip_longest(chain([0], indices), chain(indices, [len(data)]))]

def powersplit(data):
    return {tuple([''.join(x) for x in slices(data, indices)]) for indices in powerset(range(1, len(data)))}

def get_dictionary(min_length):
    return {line.rstrip('\n').lower() for line in open('/usr/share/dict/words') if len(line.rstrip('\n')) >= min_length}

def contents_in_dictionary(dictionary, in_string):
    return all([y in dictionary for y in in_string])

def acro_word(in_string):
    candidates = reduce(set.union, [powersplit(partial) for partial in powerset(in_string)])
    dictionary = get_dictionary(4)
    return [x for x in candidates if contents_in_dictionary(dictionary, x)]

My questions are:

Am I missing any obvious / not complex improvements to my algorithm?
Am I missing any obvious pythonic data structures / functions that I can take advantage of?
How can I

Make this generally faster. As you add characters to the input string, it takes exponentially (I believe) longer to return.
Turn everything into a generator and stream results to the client.

1. Review

The lines are quite long, which means that it doesn't fit in the box here at Code Review. The Python style guide (PEP8) recommends sticking to 79 characters.
There are no docstrings. What do all these functions do? What am I supposed to pass as parameters?
Instead of:

[x for x in expression]

write:

list(expression)

and instead of:

{x for x in expression}

write:

set(expression)

This simplifies powerset to:

def powerset(seq):
    """Return a set whose elements are tuples of elements of seq."""
    return set(chain.from_iterable(combinations(seq, r)
                                   for r in range(len(seq) + 1)))

Instead of building a list:

return all([y in dictionary for y in in_string])

you can often just pass a generator expression to a function:

return all(y in dictionary for y in in_string)

This avoids constructing an intermediate list in memory.
Similarly, instead of:

tuple([''.join(x) for x in slices(data, indices)])

write:

tuple(map(''.join, slices(data, indices)))

In slices, I think it would be clearer to use tuple unpacking to give names to the elements of y, and so avoid the mysterious *list(y). Also, since the chains are the same length, you can use zip instead of zip_longest:

return [list(islice(data, start, stop)]
        for start, stop in zip(chain([0], indices),
                               chain(indices, [len(data)]))]

Consider using the pairwise recipe from the itertools documentation, and writing:

indices = chain([0], indices, [len(data)])
return [list(islice(data, start, stop)]
        for start, stop in pairwise(indices)]

The expression line.rstrip('\n') is repeated in get_dictionary.

2. Algorithm

The approach taken here is known as generate-and-test. The code generates all subsequences of the input, and then all ways to split the subsequences into words, and then tests whether each set of words is valid. The problem with this approach is that it takes time that's exponential in the length of the input.

What you need is some way to prune the search so that most of the exponentially many candidates subsequences are never considered (because they don't appear in your dictionary).

I'll sketch an algorithm. Take your dictionary:

BLACK
BLUE
BLUER
GREEN
GREY

and transform it into a prefix tree (in the diagram, nodes with a thicker border indicate ends of valid words in the dictionary):

We'll carry out a depth-first search over all the possible substrings of the input, but at each branch of the search in the word we'll also have a corresponding position in the prefix tree. At each step of the search we can either add the first letter of the input to the candidate subsequence and move downwards in the prefix tree, or discard the first letter of the input and remain at the same point in the prefix tree. This gives us two possible branches in the search.

Now suppose the input is the word ABSOLUTE. We start the search with input ABSOLUTE, an empty candidate subsequence, and the position at the root of the prefix tree:

Can we take the first branch of the search (adding the A to the candidate subsequence and moving down in the prefix tree)? No, because we can't move down in the prefix tree from our current position to an A. So this branch of the search goes nowhere.

In the other branch of the search (where we discard the A), we now have input BSOLUTE (with candidate subsequence and position in the prefix tree unchanged).

From this position, we can take the first branch, because there is a transition to B in the prefix tree, so we can add B to the candidate subsequence, discard the B from the input, and continue to here in the search:

And so on. You'll see that in this algorithm we immediately prune any step in the search that we know can never lead to a valid subsequence.


Balanced smileys check algorithm (part 4) This is a follow-up for here, for bug fixes and applied advice.

  Problem

  Your friend John uses a lot of emoticons when you talk to him on
  Messenger. In addition to being a person who likes to express himself
  through emoticons, he hates unbalanced parenthesis so much that it
  makes him go :(

  Sometimes he puts emoticons within parentheses, and you find it hard
  to tell if a parenthesis really is a parenthesis or part of an
  emoticon.

  A message has balanced parentheses if it consists of one of the
  following:

  An empty string ""
  One or more of the following characters: 'a' to 'z', ' ' (a space) or ':' (a colon)
  An open parenthesis '(', followed by a message with balanced parentheses, followed by a close parenthesis ')'.
  A message with balanced parentheses followed by another message with balanced parentheses.
  A smiley face ":)" or a frowny face ":("
  Write a program that determines if there is a way to interpret his message while leaving the parentheses balanced.

I'm working on this balanced smileys checking algorithm, and my current solution is very naive, with just two rules:

Do a forward scan, at any point, the number of ) (close) should be less than or equal to the number of ( (open) + number of :( (frown)
Do a backward scan, at any point, the number of ( (open) should be less than or equal to the number of ) (close) and :) (smile)

I'm wondering if there are any bugs in my checking logic. Any advice on algorithm time complexity improvement or code style advice is highly appreciated as well.

def check_balance(source):
    pre = ''
    left = 0
    right = 0
    frown = 0 # :(
    smile = 0 # :)
    # forward scan
    for c in source:
        if c == ')':
            if pre == ':':
                smile += 1
            else:
                right += 1
                if right > left + frown:
                    return False
        elif c == '(':
            if pre == ':':
                frown += 1
            else:
                left += 1
        pre = c
    # reverse scan
    for i in range(len(source)-1, -1, -1):
        if source[i] == ')':
            if i > 0 and source[i-1] == ':':
                smile += 1
            else:
                right += 1
        elif source[i] == '(':
            if i > 0 and source[i-1] == ":":
                frown += 1
            else:
                left += 1
                if right + smile < left:
                    return False
    return True

def verify(test_input, expected_result):
    actual_result = check_balance(test_input)
    assert actual_result == expected_result, "{} expected {} failed".format(test_input, expected_result)
    print(test_input, actual_result)

if __name__ == "__main__":
    verify(':)(', False)
    verify(':()', True)
    verify('abc(b:)cdef(:()', True)
    verify('a)', False)
    verify(')(' , False)

Correctness

I suggest you add a test to verify(':):)(():(:(', False).  Perhaps your counters need to be reset?

Algorithm

The great thing about your algorithm is its simplicity.  However the algorithm now requires scanning the loop twice. The problem can be evaluated in a single pass with a different strategy, should you wish to consider performance.

Coding

Consider whether every line in your algorithm is actually relevant to the result.  Specifically, if you don't use smile in the first loop or frown in the second loop, why count them?

Beyond that and the stylistic comments I made previously, you might sit back in your chair and scan your solution.  Where the meaning, intent and motivation are unclear you might consider augmenting it by variable name selection and with comments.

Alternate Strategy

As an example of an alternate strategy that only requires scanning once, you might

keep a count of unpaired parens, opt_open, opt_close
borrow from opt_open as needed to pair with excess close parens
ensure opt_close never exceeds unpaired parens count since those are out of scope
ensure final unpaired parens count does not exceed opt_close

def check_balance(src):
    opt_open = opt_close = 0
    count = 0
    has_eyes = False

    for char in src:
        if char == '(':
            if has_eyes:
                opt_open += 1
            else:
                count += 1

        elif char == ')':
            if has_eyes:
                opt_close += 1
            elif count > 0:
                count -= 1
            # if needed pair with an optional parens
            elif opt_open > 0:
                opt_open -= 1
            else:
                return False

        if opt_close > count:
            opt_close = count

        has_eyes = (char == ':')
    return count <= opt_close


Can MD5ing all the files in a table cause a memory leak? I'm running this code with the FastMD5 library. Could it lead to memory leaks, or does it seem OK?

File[] directory = new File("/PATH/TO/FOLDER/WITH/LOTS/OF/FILES").listFiles();

    for(int i = 0; i < directory.length;i++){

    System.out.println(MD5.asHex(MD5.getHash(directory[i])));

    }

In Java you have to do so much hard work to leak memory (someone will say not closing stream, but that's another story).

I think you have a C programming background. So you are thinking of memory leak. Forget it there is JVM for you.

And use enhanced for loop in Java as much as you can...

for(File file : directory) {
    System.out.println(MD5.asHex(MD5.getHash(file)));
}


Asynchronous database lookups in Node.JS using Promises Not confident enough if this would be the correct 'node way' of writing Promises:

Essentially, there are two async database calls, the result from both of these calls has to be collected and used in rendering the view:

Creating ...

exports.getIndustryById = function(industryId) {
   return new Promise(function(resolve, reject) {
    Industry.findOne({
      value: industryId
}, function(err, industry) {
  if (err) {
    return reject(err);
  }
  return resolve(industry);
});
})
}

exports.getCountryById = function(countryId) {
 return new Promise(function(resolve, reject) {
  Country.findOne({
   value: countryId
  }, function(err, country) {
  if (err) {
    return reject(err);
  }
   return resolve(country);
  });
 })
}

And using ...

  exports.renderEditProfile = function(req, res, next) {
  var i, c;
  ct.getIndustryById(req.user.industry)
    .then(function(industry) {
      i = industry;

      return ct.getCountryById(req.user.country);
    }).then(function(country) {
      c = country;

      res.render('profile_edit', {
        profile: req.user,
        industry: i,
        country: c
      });
    })
    .catch(function(err) {
      res.status(500).json({'message': err});
    });
};

This works, but am I getting Promises right?

Promises benefit from the new ES6 syntax, making callbacks a bit compact. If you can use Node.js 4+, then by all means take advantage of the new syntax.

Your indents could need a bit of work. You're mixing 2-space and 4-space. It doesn't really matter which one you chose as long as you are consistent.

var i, c;
ct.getIndustryById(req.user.industry)
  .then(function(industry) {
    i = industry;

Instead of putting an outsider i and c to store the values temporarily until the last then, why not pass it through as a resolved value? That way, your functions don't need to use temporary variables.

exports.renderEditProfile = function(req, res, next) {
  ct.getIndustryById(req.user.industry).then( industry => {
    return ct.getCountryById(req.user.country).then( country => ({ country, industry }) );
  }).then( data => {
    res.render('profile_edit', {
      profile: req.user,
      industry: data.industry,
      country: data.country
    });
  }).catch(function(err) {
    res.status(500).json({'message': err});
  });
};

Alternatively, both requests don't seem to depend on each other. Therefore, it makes no sense to fire them sequentially. Suggesting you fire them in parallel, and use Promise.all to listen for the response.

exports.renderEditProfile = function(req, res, next) {

  const industryPromise = ct.getIndustryById(req.user.industry);
  const countryPromise = ct.getCountryById(req.user.country)

  Promise.all([ industryPromise, countryPromise ]).then(resolution => {
    res.render('profile_edit', {
      profile: req.user,
      industry: resolution[0],
      country: resolution[1],
    });
  }).catch(function(err) {
    res.status(500).json({'message': err});
  });

};


use Identity value in entity after insert in Database i want to generate value using current date and ID of inserted entity
problem is that ID value not generated until insert object to  database,
so i use this code for generate value:

public ActionResult Create([Bind(Include = "ID,TrackingCode,FirstName,LastName,ProductName,CurrentStats,FailureReason,ReceiveDateTime,DeliveryDateTime,Price,Description,SerialNumber,ExtraItems,PhoneNumber")] Tracking tracking)
    {
        if (ModelState.IsValid)
        {
            db.Trackings.Add(tracking);
            db.SaveChanges();
            Tracking _tracking = db.Trackings.Find(tracking.ID);
            db.Entry(_tracking).State = EntityState.Modified;
            _tracking.TrackingCode = Convert.ToInt32(DateTime.Now.ToString("yyyyMMdd") + tracking.ID.ToString());
            db.SaveChanges();
            return RedirectToAction("Index");
        }

        return View(tracking);
    }

code work fine and do job that i want
but i think this is not standard way is there any better approach to improve this code?

I would suggest you change your database design.

Option 1)

Have SQL Server set a column to the current date/time when you insert a row  See https://stackoverflow.com/q/6594589 for how to do it.  Then have that column mapped on your Tracking entity but mark the property as [DatabaseGenerated(DatabaseGeneratedOption.Computed)]

Assuming you call the field AddDate

[DatabaseGenerated(DatabaseGeneratedOption.Computed)]
public datetime AddDate {get;set;}

Now you can create your get only TrackingCode property in code and use the [NotMapped] on it and define it as

[NotMapped]
public int TrackingCode
{
     get { return Convert.ToInt32(AddDate.ToString("yyyyMMdd") + tracking;
}

Option 2:

Create an after insert tigger and have SQL Server update the TrackingCode.  Again I would mark the property as DatabaseGenerated

In a nut shell it's better to offload this to the SQL Server


Demonstration of ChessBoard Traveling (CoderByte) Problem

  Have the function ChessboardTraveling(str) read str which will be a
  string consisting of the location of a space on a standard 8x8 chessboard with no pieces on the board along with another space on the
  chessboard.

  The structure of str will be the following:

  \$(x,y)(a,b)\$ where \$(x, y)\$ represents the position you are
  currently on with \$x\$ and \$y\$ ranging from 1 to 8 and \$(a, b)\$
  represents some other space on the chessboard with \$a\$ and \$b\$
  also ranging from 1 to 8 where \$a > x\$ and \$b > y\$. Your program
  should determine how many ways there are of traveling from \$(x, y)\$
  on the board to \$(a, b)\$ moving only up and to the right.

  Example: if str is \$(1,1)(2, 2)\$ then your program should output 2 because there are only two possible ways to travel from space
  \$(1, 1)\$ on a chessboard to space \$(2, 2)\$ while making only moves
  up and to the right.

Are there any improvements I can make?

#include <algorithm>
#include <functional>
#include <iostream>
#include <iterator>
#include <sstream>
#include <string>
#include <vector>

std::string delete_Punctuation(std::string& str)
{
    std::string noPunctString = "";

    str.insert(str.length()/2," ");

    for (auto character : str)
    {
        if (!ispunct(character))
        {
            noPunctString.push_back(character);
        }
    }

    return str = noPunctString;
}

bool check_If_PointXY_Is_Less_Than_PointAB(std::vector<int> v)
{
     return (v.at(2) > v.at(0) && v.at(3) > v.at(1));
}

long long int find_Factorial(unsigned int n)
{
    if (n == 0)
    {
       return 1;
    }

    return n * find_Factorial(n - 1);
}

int number_Of_Steps(std::string& str)
{
    str = delete_Punctuation( str );

    std::istringstream iss( str );

    std::vector<int> coll((std::istream_iterator<int>( iss )),std::istream_iterator<int>());

    int steps = 0;

    if (check_If_PointXY_Is_Less_Than_PointAB(coll))
    {
        int xDistance = coll.at(2) - coll.at(0);
        int yDistance = coll.at(3) - coll.at(1);

        int totalDistance = xDistance + yDistance;

        //combination formula
        steps = find_Factorial(totalDistance)/(find_Factorial(xDistance) * (find_Factorial(totalDistance - xDistance)));
    }

    return steps;
}

I see some things that may help you improve your code.

Fix the input parsing

The idea is sound but the implementation is flawed. Instead of the complex method currently used, I'd recommend using this single function:

std::replace_if(str.begin(), str.end(),
        [](char ch){ return !std::isdigit(ch); }, ' ');

Using this, we're guaranteed that the string will contain only spaces and digits.

Use objects

I'd suggest that one could make good use of a Point2D object to make calculations very clear:

struct Point2D {
    int x = 0;
    int y = 0;
    int dx(const Point2D &other) const {
        return x - other.x;
    }
    int dy(const Point2D &other) const {
        return y - other.y;
    }
    friend std::istream& operator>>(std::istream &in, Point2D &p) {
        return in >> p.x >> p.y;
    }
};

Reduce the numerical range for calculations

The current code uses a generic factorial which boils down to $$\frac{(dx + dy)!}{dx! dy!}$$  The maximum for \$dx\$ or \$dy\$ is 7, so the largest factorial required is \$14! = 87178291200\$.  ... Or is it?  In fact, we could consider instead how to generically calculate $$\frac{n!}{m!}$$ with \$m < n\$.  A simple way to do this is like so:

constexpr unsigned factDelta(unsigned n, unsigned m) {
    return n > m ? n * factDelta(n-1, m) : 1;
}

And the ordinary factorial function could be expressed as:

constexpr unsigned fact(unsigned n) {
    return factDelta(n, 1);
}

This can reduce the range of the multiplications somewhat, especially if we always choose the larger of \$dx\$ or \$dy\$ to divide.  This means that the return value will always be \$<= 7! = 5040\$ which easily fits into an unsigned.  Note, however, that these functions will overflow if they're given numbers outside that range.  For that reason, I'd recommend making them static functions and/or doing range checking.

Don't alter passed parameters

The current number_Of_Steps() routine takes a reference to a string and then alters the string.  It's not strictly wrong, but it's probably not the best interface design.  I would not expect that the passed string was altered.  Two ways to address this would be to pass a const reference or to simply declare the argument as std::string str which makes a local copy.  Because of the need to alter the string (by removing punctuation), I'd recommend the latter approach.

A worked example

Here's an alternative function that uses the functions and objects described above:

int alt(std::string str) {
    std::replace_if(str.begin(), str.end(),
            [](char ch){ return !std::isdigit(ch); }, ' ');
    std::istringstream in(str);
    Point2D a;
    Point2D b;
    in >> a >> b;
    int dx = b.dx(a);
    int dy = b.dy(a);
    if (dx <= 0 || dy <= 0) {
        return 0;
    }
    int m = dx + dy;
    if (dy > dx) {
        std::swap(dx, dy);
    }
    return factDelta(m, dx) / fact(dy);
}


Python XML - extracting information I am very new to Python, and also this is my first time trying to parse XML.
I am interested in information within str elements. I can identify that information using the str@name attribute value.

def get_cg_resources(pref_label, count=10):
    r = request_that_has_the_xml

    ns = {'ns':"http://www.loc.gov/zing/srw/"}

    tree = ET.ElementTree(ET.fromstring(r.text))
    records =[]
    for elem in tree.iter(tag='{http://www.loc.gov/zing/srw/}record'):
        record = {
        'title':'',
        'source': '',
        'snippet': '',
        'link':'',
        'image':'',
        'adapter':'CG'
        }
        for value in elem.iter(tag='str'):
            attr = value.attrib['name']
            if(attr == 'dc.title'):
                record['title'] = value.text
            elif(attr == 'authority_name'):
                record['source'] = value.text
            elif(attr == 'dc.description'):
                record['snippet'] = value.text
            elif(attr == 'dc.related.link' ):
                record['link'] = value.text
            elif(attr == 'cached_thumbnail'):
                img_part = value.text
                record['image'] = "http://urlbase%s" % img_part
        records.append(record)

    return  records

Is this approach correct/efficient for extracting the information I need? Should I be searching for the str elements differently?

Any suggestions for improvements are welcome.

def get_cg_resources(pref_label, count=10):
    r = request_that_has_the_xml

    ns = {'ns':"http://www.loc.gov/zing/srw/"}

    tree = ET.ElementTree(ET.fromstring(r.text))

You dont't need ElementTree to extract data from xml, Element is enough.

    root = ET.fromstring(r.text)

If 'str' tag is contained only in 'record' tag you don't have to find 'record'
tag first. You can simply look for 'str' tag. The iter method recursively
iterates over it's children.

There is a dict to represent 'namespace'. So you don't have to explicitly list
it's value, 'key:tag', dict is enough.

    for elem in root.iter('ns:str',ns):

If there are 'str' tags that are contained in other tags that you don't want,
then you have to first find 'record' tags.

    records =[]

    for elem in root.iter('ns:record',ns):

         record = {
        'title':'',
        'source': '',
        'snippet': '',
        'link':'',
        'image':'',
        'adapter':'CG'
        }

record can be initialized as follows,

        record =dict.fromkeys(['title','source','snippet','link','image'],'')
        record['adapter']='CG'

        for value in elem.iter('ns:str',ns):
            attr = value.attrib['name']
            if(attr == 'dc.title'):
                record['title'] = value.text
            elif(attr == 'authority_name'):
                record['source'] = value.text
            elif(attr == 'dc.description'):
                record['snippet'] = value.text
            elif(attr == 'dc.related.link' ):
                record['link'] = value.text
            elif(attr == 'cached_thumbnail'):
                img_part = value.text
                record['image'] = "http://urlbase%s" % img_part
        records.append(record)

    return  records

The above code means you want to extract value of the 'name' attribute of 'str'
tags which are contained in 'record' tags.

If you want a generator you can simply
replace records.append(record) wiht yield record and delete return records
and records = [] which will be efficient if the list is huge.


Image upload handler with random filename generator and postprocessing I have a function to handle image uploads.  Originally, it contained a goto:

restartA:
$name = $this->random_name() . $ext;
if(file_exists($this->img_path . $name)){
    unset($name);
    goto restartA;
}

It seems pretty clear to me what the code does, and I think anyone who reads it would immediately understand what's happening.  On the other hand, goto is considered evil, so I rewrote it like this:

while(true){
    $name = $this->random_name() . $ext;
    if(file_exists($this->img_path . $name)){
        unset($name);
    }else{
        break;  //weve got a unique name
    }
}

Here's the context in which the code excerpt above appears. In place of some code which I am not allowed to post, I have substituted an array of 3 static sizes.

public function original_img_upload()
{
    $img_name = 'original_image';
    $image_name = $_FILES[$img_name]['name'];
    $image_temp = $_FILES[$img_name]['tmp_name'];
    $image_type = $_FILES[$img_name]['type'];
    $image_info = getimagesize($image_temp);
    $valid = ['.jpg', '.jpeg', '.gif', '.png'];
    $exif = [IMAGETYPE_GIF,IMAGETYPE_JPEG,IMAGETYPE_PNG];
    $ext = strtolower(strrchr($image_name, "."));
    $names_to_return = []; //image names to return to main scope
    $sizes = [
        [1000,608],
        [657,400],
        [329,200]
    ]; //image target sizes

    if( !is_uploaded_file($_FILES[$img_name]['tmp_name']) ){
        $this->e('bad file input');
    }
    if( !in_array(exif_imagetype($image_temp),$exif) && in_array($ext,$valid) ){
        $this->e('file type not allowed');
    }
    if( !$image_info ){
        $this->e('invalid image size.');
    }

    switch($image_type){
        case 'image/png':
            $image_res = imagecreatefrompng($image_temp);
        break;
        case 'image/gif':
            $image_res = imagecreatefromgif($image_temp);
        break;
        case 'image/jpeg':
        case 'image/pjpeg':
            $image_res = imagecreatefromjpeg($image_temp);
        break;
        default:$this->e('bad file input');
    }

    for($i=0;$i <3; $i++){
        $canvas = imagecreatetruecolor($sizes[$i][0],$sizes[$i][1]);//width + height
        //( resource $dst_image , resource $src_image , int $dst_x , int $dst_y , int $src_x , int $src_y , int $dst_w , int $dst_h , int $src_w , int $src_h )
        imagecopyresampled(
            $canvas,
            $image_res,
            0,0,0,0,
            $sizes[$i][0],
            $sizes[$i][1],
            $image_info[0],
            $image_info[1]
        );

        //create random destination file name
        while(true){
            $name = $this->random_name() . $ext;
            if(file_exists($this->img_path . $name)){
                unset($name);
            }else{
                break;  //weve got a unique name
            }
        }

        $destination = $this->img_path . $name;

        //write image
        switch(strtolower($image_type))
        {
            case 'image/png':
                imagepng($canvas,$destination);
                break;
            case 'image/gif':
                imagegif($canvas,$destination);
                break;
            case 'image/jpeg':
            case 'image/pjpeg':
                imagejpeg($canvas,$destination,100);
                break;
        }

        //push image name to return images
        array_push($names_to_return,$this->args['base_url'].'img/'. $name);
        //unset resources and free memory
        unset($canvas,$name,$destination);
    }
    return $names_to_return;
}

Extract Code to Function and avoid goto

First of all, I would extract this code to it's own function, and then I would use a while loop looking something like this:

private function getUnusedRandomFileName($extention) {
    $name = $this->random_name() . $extention;

    while(file_exists($this->img_path . $name)){
        $name = $this->random_name() . $extention;
    }

    $return name;
}

To avoid the two duplicate lines, you could also use do-while:

private function getUnusedRandomFileName($extention) {
    do {
        $name = $this->random_name() . $ext;
    } while (file_exists($this->img_path . $name));
    return $name;
}

Or, since you now have a function, you could return:

private function getUnusedRandomFileName($extention) {
    while(true){
        $name = $this->random_name() . $ext;
        if(!file_exists($this->img_path . $name)){
            return $name;
        }
    }
}

Or use recursion:

private function getUnusedRandomFileName($extention) {
    $name = $this->random_name() . $extention;
    if (file_exists($this->img_path . $name)) {
        return getUnusedRandomFileName($extention);
    } else {
        return $name;
    }
}

So there are many methods to solve this without goto. I like the third best, but choose the one you like.

Misc

More functions: in addition to the getUnusedRandomFileName function, I would also add a createThumbnail function, and possibly createImageFrom and writeImageTo, for the first and second switch statement.
Paths: at one point you have $this->img_path . $name, and at another you have $this->args['base_url'].'img/'. $name. Do these mean the same thing? Is $this->img_path the same as img? In that case use it everywhere, otherwise it will cause bugs when changed.
The image type is completely user controlled, so $image_type = $_FILES[$img_name]['type']; will be whatever the user wants it to be; exif_imagetype is also bypassable. So it seems that the in_array($ext,$valid) is currently the only part that prevents attacks. I would add a comment stating this, just in case the code is changed later on.


C++ class to compute similarity coefficients I'm a complete beginner in C++. I wrote a class to compute similarity coefficients using Euclidean distance and Pearson coefficients on map data structure. I would like to know if it is possible to improve the code to make it more efficient in terms of memory and time it takes to execute the code.

Header:

#include <map>
#include <string>

extern "C" class DATAANALYTICS_API Similarity
{
    std::map<std::string, std::map<std::string, double>> data_;

public:
    Similarity(std::map<std::string, std::map<std::string, double>> &data);
    double EuclideanSimilarity(std::string a, std::string b);
    double PearsonSimilarity(std::string a, std::string b);
};

Implementation:

#include <iostream>
#include <map>
#include <string>
#include <cmath>
#include <vector>
#include "DataAnalytics.h"

Similarity::Similarity(std::map<std::string, std::map<std::string, double>> &data)
{
    data_ = data;
}

double Similarity::EuclideanSimilarity(std::string a, std::string b)
{
    double distance_squared = 0, aVal, bVal;
    for (auto const &entry : data_[a])
    {
        if (data_[b].count(entry.first) > 0)
        {
            aVal = entry.second;
            bVal = data_[b][entry.first];
            distance_squared += (aVal - bVal) * (aVal - bVal);
        }
    }
    return (1 / (1 + distance_squared));
}

double Similarity::PearsonSimilarity(std::string a, std::string b)
{
    double aVal, bVal, aExpectedValue = 0, bExpectedValue = 0, aSquaredExpectedValue = 0, bSquaredExpectedValue = 0, abExpectedValue = 0;
    std::vector<double> aValues, bValues, abValues;
    int commonItemCounter = 0;

    for (auto const &entry : data_[a])
    {
        if (data_[b].count(entry.first) > 0)
        {
            commonItemCounter++;
            aVal = entry.second;
            bVal = data_[b][entry.first];
            aValues.push_back(aVal);
            bValues.push_back(bVal);
            abValues.push_back(aVal * bVal);
        }
    }

    if (aValues.size() == 0)
    {
        return 0;
    }
    else
    {
        for (int i = 0; i < aValues.size(); i++)
        {
            aExpectedValue += aValues[i] / commonItemCounter;
            bExpectedValue += bValues[i] / commonItemCounter;
            aSquaredExpectedValue += pow(aValues[i], 2) / commonItemCounter;
            bSquaredExpectedValue += pow(bValues[i], 2) / commonItemCounter;
            abExpectedValue += abValues[i] / commonItemCounter;
        }
        double denominator = sqrt(aSquaredExpectedValue - pow(aExpectedValue, 2)) * sqrt(bSquaredExpectedValue - pow(bExpectedValue, 2));
        if (denominator != 0)
        {
            return (abExpectedValue - (aExpectedValue * bExpectedValue)) / denominator;
        }
        else
        {
            return 0;
        }
    }
}

When I run unit tests on the two methods EuclideanSimilarity and PearsonSimilarity, they both take about 130ms to complete which is a bit strange because I'm doing a lot more in PearsonSimilarity than in EuclideanSimilarity.

I have to admit that I don't have any familiarity with this particular problem, so I can't offer any great advice on it. That said, I do see a few things in your code that I think could be improved.
Scope
One thing I would recommend is moving variables closer to the place where they are used. You're currently defining all variables at the beginning of the function, and that makes it harder to tell what might have been changed when reading the body of the function. In PearsonSimilarity() you're using aVal, bVal, and the vectors in the first for loop. The expected values are not used until the second for loop (inside an if). I would move their declarations into the else clause of the if because of this. Further, I would move each declaration to its own line. Your lines are very long and hard to read because I have to scroll to see everything on them.
In fact, it might make sense to break up the function and call 2 smaller functions - one which calculates the commonItemCounter and the arrays, and another which processes the arrays. That's up to you.
Performance
One big thing I see that could be improved is to remove all calls to pow(). In each case you are squaring the input value. pow() is a very expensive function, and it's a single instruction to just multiply a value by itself. I would first pull the value out of the array, and then use it multiple times, like this:
    for (int i = 0; i < aValues.size(); i++)
    {
        double a = aValues [ i ];
        double b = bValues [ i ];
        double ab = abValues [ i ];
        aExpectedValue += a / commonItemCounter;
        bExpectedValue += b / commonItemCounter;
        aSquaredExpectedValue += (a * a) / commonItemCounter;
        bSquaredExpectedValue += (b * b) / commonItemCounter;
        abExpectedValue += ab / commonItemCounter;
    }

Another thing that I see that is likely to be slow is that commonItemCounter is defined as an int but is used to calculate floating point values. You should declare it as a double. And then, once you have, it might make sense (measure to be sure) to invert it and multiply by it instead of dividing by it. (This might be a non-issue on modern systems, but has been an issue in the past.)
Early Returns
Early returns are helpful when you have some "early out" conditions such as the input being out-of-range or degenerate in some way. That's not the case here, and it can be confusing to follow. In this case, I'd recommend having a return value and only a single return statement at the end like this:
double result = 0.0;
if (aValues.size() > 0)
{
    for (int i = 0; i < aValues.size(); i++)
    {
        // ... rest of loop here
    }
    double denominator = sqrt(...stuff...)
    if (denominator != 0.0)
    {
        result = (abExpectedValue - (aExpectedValue * bExpectedValue)) / denominator;
    }
}
return result;

It's fewer levels of ifs (less cyclomatic complexity) and easier to understand the flow.


Calculation of Reynolds number for a pipe I'm new to coding and want to pick up some good habits with regards to coding conventions and general readability (I am using PEP8 as a guideline). Below I added a very simple function. Feel free to give me some pointers on what to improve/change about the commenting. Should I be catching any other errors?

def reynolds(den, vel, dia, vis):
    """Calculate the Reynolds number for a given pipe flow.

    Arguments:
    den -- density of the fluid in [kg/m3]
    vel -- the mean velocity of the fluid in [m/s]
    vis -- dynamic viscosity of the fluid in [Pa*s]
    dia -- inside diameter of the pipe in [m]

    The Reynolds number itself is dimentionless.
    It is used to charakterize flow patterns.
    """
    try:
        return den*vel*dia/vis       # regular execution of the function
    except ZeroDivisionError:
        print ('divide by zero')     # catch devision by zero

There is no reason to abbreviate parameter names to three letters.  That just makes the code unnecessarily cryptic.  (Using names like rho instead of density could be an acceptable alternative, if brevity is essential.)  Furthermore, consider naming the function more specifically, so that it is obvious that it applies to cylindrical pipes.

As per PEP 8, binary operators should have have spaces for readability.

There is no good reason to catch ZeroDivisionError.  If the viscosity is 0, then this function would return None, and would likely cause other problems elsewhere in your calling code.  Just let the ZeroDivisionError propagate naturally, and your code would be easier to debug.  (Furthermore, your two comments are pointless and redundant.)

Watch your spelling ("dimentionless" → "dimensionless", "charakterize" → "characterize").

def reynolds_pipe(density, velocity, diameter, viscosity):
    """
    Calculate the Reynolds number for a given pipe flow.
    The resulting Reynolds number is dimensionless.
    It is used to characterize flow patterns.

    Arguments:
    density -- density of the fluid in [kg/m3]
    velocity -- the mean velocity of the fluid in [m/s]
    viscosity -- dynamic viscosity of the fluid in [Pa*s]
    diameter -- inside diameter of the pipe in [m]
    """
    return density * velocity * diameter / viscosity


Draft version of wind farm design software I'm currently working on a wind farm design software and basically I have to analyze the incidence of certain wind turbines on other downwind, in order to calculate the velocity drop in the wind once the downstream turbine "sees" the flow.

I have sorted out most of the code and it works, yet this particular piece of it is extremely slow to compute.

for wd in wind_direction_index:
        self.list_for_speed.append([])
        for ws in wind_speed_index:
            self.list_for_speed[wd].append([])
            wind_speed = self._wind_speeds[ws]
            #For every wind turbine, remember this list is still ordered from up to dow
            dummylist=copy.deepcopy(self.list_for_direction[wd])
            for WT in dummylist:
                #Replace No deficit with 0
                if WT[6]=='NO_deficit':
                    WT[6]=[0.0]
                else:
                    pass
                #For every WT that causes a wake effect on current one
                for upWT in WT[3]:
                    #Define current turbine and rotating upwind turbine
                    for ready in dummylist:
                        if upWT[0]==ready[0]:
                            upWT[6]=ready[6]
                        else:
                            pass
                    current_turbine=WT
                    upwind_turbine=upWT
                    #Calculate wind speeds deficits in every current WT due to every upwind WT to it only if here are upwind WTS
                    if len(current_turbine[3])!=0:
                        self.calculate_deficits(current_turbine,upwind_turbine,wind_speed)
                    else:
                        pass
                #Now we have appended all deficits in current_turbine(6) Time to add them up, find the speed and replace
                #First, I square the list of deficits
                WT[6]=[i**2 for i in WT[6]]
                #Now, i add the squares of the deficits
                deficit_squared=sum(WT[6])
                #Take the square root
                deficit_current=sqrt(deficit_squared)
                #Finally calculate wind speed at the rotor
                U=wind_speed*(1-deficit_current)
                #And I replace the list of wind speed deficits with the wind speed at the rotor
                WT[6]=U
                _Ct=self.team.rna_analysts.get_Ct(U)
                #Save results to local wind speeds array
                self.local_wind_speeds[WT[0]][wd][ws]=[U,_Ct]
            self.list_for_speed[wd][ws].append(dummylist)

I know it's a bit messy, but you can see more or less how my data structures are arranged. I have to calculate a wind speed "deficit" for every turbine, based on the wind direction and the wind speed (evidenced by local_wind_speeds). Can anyone spot how to make this piece of code much much faster?

I suspect copy.deepcopy is costing you much time; try to avoid it if possible.

Also you are only copying self.list_for_direction[wd], is it necessary to do it multiple time for ws in wind_speed_index?

I try at my best to guess what you are trying to achieve and do not guarantee my code would do exactly what you want.  Here the dummylist is no longer needed except for self.list_for_speed[wd][ws].append(dummylist).  Unless you really need self.list_for_speed later and it is important that those sharing the same wd are distinct object, I am afraid you can't save much time doing so many copy.deepcopy.

One thing that confuses me is that you have nested for WT in dummylist and for ready in dummylist.  And you change WT[6] when it is "NO_deficit" in the first for-loop.  Say you are at the i-th iteration in the first for loop and the j-th iteration in the second for-loop.  If j>i, it is possible that you have ready[6] being "NO_deficit". I assume this is not what you want.

def U_Ct(OBJ,WT6,wind_speed):
    if WT6 == 'NO_deficit':
        deficit_current = 0.0
    else:
        deficit_squared = sum([i**2 for i in WT6])
        deficit_current = sqrt(deficit_squared)
    U = wind_speed*(1-deficit_current)
    _Ct = OBJ.team.rna_analysts.get_Ct(U)
    return U,_Ct

def get_turbine(WT,upWT):
    '''  WT is a value in the dummylist
    upWT is the WT[3] where WT is from the dummylist
    '''
    if WT[6] == 'NO_deficit':
        WT[6] = [0.0]
    for ready in WT:
        if upWT[0] == ready[0]:
            upWT[6] = ready[6]
    current_turbine = WT
    upwind_turbine = upWT
    return current_turbine,upwind_turbine

for wd in wind_direction_index:
    for ws in wind_speed_index:
        dummylist = copy.deepcopy(self.list_for_direction[wd])
        self.list_for_speed[wd][ws].append(dummylist)
        wind_speed = self._wind_speeds[ws]
        for WT in self.list_for_direction[wd]:
            for upWT in WT[3]:
                current_turbine,upwind_turbine = get_turbine(WT,upWT)
                if len(current_turbine[3])!=0:
                    # What does this function do? It doesn't return anything
                    self.calculate_deficits(current_turbine,upwind_turbine,wind_speed)
            U,_Ct = U_Ct(self,WT[6],wind_speed)
            self.local_wind_speeds[WT[0]][wd][ws] = [U,_Ct]


Simple Single Linked List I'm currently studying data structures, I have implemented simple single linked list as shown below.

I would like to know how can I improve it?

#include <iostream>
#include <memory>
#include <cassert>

template <typename T>
class List
{
    struct node
    {
        T value;
        std::unique_ptr<node> next;
    };

public:
    List();

    void add(T value);

    T first() const;
    T value(std::size_t position) const;

    std::size_t length() const;
    void clear();

private:
    std::unique_ptr<node>   mFirst;
    std::size_t             mLength;
};

template <class T>
List<T>::List()
    : mFirst(nullptr)
    , mLength()
{
}

template <typename T>
std::size_t List<T>::length() const
{
    return mLength;
}

template <typename T>
T List<T>::first() const
{
    assert(mFirst != nullptr);
    return mFirst->value;
}

template <typename T>
void List<T>::add(T value)
{
    auto next = std::move(mFirst);
    mFirst = std::make_unique<node>();
    mFirst->value = value;
    mFirst->next = std::move(next);
    mLength++;
}

template <class T>
T List<T>::value(std::size_t position) const
{
    assert(mFirst != nullptr);

    auto current = mFirst.get();

    for (auto i = 0u; i < position; ++i)
    {
        current = current->next.get();
    }
    return current->value;
}

template <class T>
void List<T>::clear()
{
    assert(mFirst != nullptr);

    auto length = mLength;

    for (auto i = 0u; i < length; ++i)
    {
        mFirst = std::move(mFirst->next);
        mLength--;
    }
}

int main()
{
    List<int> list;

    for (int i = 0; i < 10; ++i)
        list.add(i);

    for (auto i = 0u; i < list.length(); ++i)
        std::cout << list.value(i) << ' ';

    list.clear();

    std::cout << "\n\n" << list.length() << "\n\n";
}

Well, there's already a good review by Barry, but I'll take a different tack:

In general, it's a good idea to use existing types (like smart-pointers) as building-blocks.
Still, you should not deform your code, causing loss of efficiency or clarity, just so you can use one, especially not in library-code (and such a basic container is that).

So, no smart-pointers for you, sometimes one really should get down to it.
As an aside, your clear has a bug: It tries to assign the deleter after already having deleted the source. See std::unique_ptr::operator=(std::unique_ptr&&).
There's a reason a node is generally a wholly-owned passive and dumb aggregate without any behavior: It would just get in the way of the List defining it properly.
You should use the same names for members all the containers in the standard-library use, so you can use standard algorithms.
For that same reason, and so you can efficiently iterate over all elements, you should provide iterators.
Sacrificing some space in the class itself for a size can be a reaonable idea. Similar arguments can be made for including a pointer (node**) to the end, so you can speed up push_back/emplace_back.
Consider putting the next-pointer first, that might allow the compiler to merge code for different template-arguments more often.

This is how you clear a list:

void List::clear() {
    for(node *head = this->head, *temp; head; head = temp) {
        temp = head->next;
        delete head;
    }
    this->head = nullptr;
}


plz help optimize the small code snippet, VS2010 profiling chart provided Profiling shows MoveNext takes most time, which I guess is true as it is lazy. But I don't know in what direction I could improve the code. (Open pic in new tab would allow u see bigger chart.)

    IEnumerable<string> MixedBlock(List<string> reelLayout, string thisBlock, int length)
    {
        List<string> recentBlock = RecentBlock(length - 1, reelLayout);
        var upper = Enumerable.Range(1, length - 1);
        var lower = Enumerable.Range(1, totalWeight[thisBlock].Num);
        var query = from x in upper
                    from y in lower
                    where x + y == length
                    select Tuple.Create(x, y);
        foreach (var t in query)
        {
            string upperBlock = string.Join(",", recentBlock.Skip(length - 1 - t.Item1));
            string lowerBlock = string.Join(",", thisBlock.Split(',').Take(t.Item2));
            yield return upperBlock + "," + lowerBlock;
        }

    }

You're doing way more iterations than are necessary.. you don't need 2 enumerables of ints just to do this, that's a lot more heap references when all your need are 2 integers right on the stack, and a nested for loop:

int upperLimit = length - 1;
int lowerLimit = totalWeight[thisBlock].Num;
List<string> recentBlock = RecentBlock(length - 1, reelLayout);
List<string> stringResults = new List<string>();

for(int upper = 1; upper < upperLimit; upper++)
{
  for(int lower = 1; lower < lowerLimit; lower++)
  {
    if (upper + lower = length)
    {
      stringResults.Add(
        string.Join(",", recentBlock.Skip(length - 1 - upper)) + "," +
        string.Join(",", splits.Take(lower)));
    }
  }
}

return stringResults;

also instead of doing the stringResults.Add you could just yield return the string right there if you wanted to allow delayed execution.

Remember, just because we now have tuples and all the happy linqiness, they can be used to simplify complex things, though sometimes they just make operations more complex. If performance is important, study up on what goes on underneath the linq before throwing it around too much.


Making additional subclasses at any time should be as painless as possible I'm writing a small text-based game in Java to learn the language. I'm concerned that I may be making some poor design decisions. I'll introduce 2 elements: A character and monsters. A singleton character should be able to fight a wide array of monsters.

Here is the monster class:

public abstract class Monster {

    protected static Random random = new Random();
    protected static int STATS_PER_LEVEL = 3;

    public int combatLvl;
    public int defense, strength, attack, hp;
    protected String name;

    protected Monster(int defense, int strength, int attack, int hp) {
        this.defense = defense;
        this.strength = strength;
        this.attack = attack;
        this.hp = hp;
        this.combatLvl = (int) (defense+strength+attack+Math.round((double)hp/10)-4)/STATS_PER_LEVEL;
    }

    public int attack() {
        //removed the body
    }

    public String toString(String name) {
        return this.name;
    }
}

Here's a subclass:

public class Bandit extends Monster {

    public static double averageLevelDouble = -1;
    public static String averageLevelString = "dummy";

    public static int minDefense = 1;
    public static int maxDefense = 1;
    public static int minStrength = 1;
    public static int maxStrength = 3;
    public static int minAttack = 1;
    public static int maxAttack = 3;
    public static int maxHp = 4;

    public Bandit() {
        super(random.nextInt(maxDefense-minDefense+1) + minDefense,
              random.nextInt(maxStrength-minStrength+1) + minStrength,
              random.nextInt(maxAttack-minAttack+1) + minAttack,
              random.nextInt(maxHp-minHp+1) + minHp);
        double min = minDefense+minStrength+minAttack+((double)minHp/10);
        double max = maxDefense+maxStrength+maxAttack+((double)maxHp/10);
        double avg = ((max+min)/2)-4;
        averageLevelDouble = (double)Math.round((avg/STATS_PER_LEVEL)*10)/10;
        averageLevelString = (Math.round(min)-4)/STATS_PER_LEVEL + "-" +
                     (Math.round(max)-4)/STATS_PER_LEVEL;
        name = "Bandit";
    }
}

My main concern is that I should be able to easily expand the game with arbitrarily many monsters. The way I'd do it now is to copy paste Bandit into a new class: Change the class name to f.i. Skeleton, initiate 'name' variable in the constructor, customize all the static ints. Is this a wrong approach?

Also a very obvious thing that comes to mind is that I should be able to display information on a particular monster without having to instantiate it. From a Swing.JComboBox I should be able to choose all monsters, display their information, and at the moment I decide to fight a monster it should be instantiated. I can't find a solution to represent monsters on a JList or JComboBox without having to manually add Bandit.name (pretend it's static) as a line of code in the JList/JComboBox class when I define the Bandit class.

I'm just assuming that I shouldn't be instantiating my subclasses. Again, that is the question: What am I doing wrong and right? I really don't how to ask but; What's wrong with my code?

You don't need a separate class for it!

I don't see any real difference between a Bandit and a Skeleton and a regular monster. You don't add or change any methods in the subclasses, and you don't add any new variables (other than the static ones).

The most important change you can make here is to only use one class. And then you'd want different methods to instanciate that class. This can be considered factory methods

A current smell of your code is that your call to the super constructor is a very long one and it's using many variables and some calculations on them. Factory methods will deal with this.

I also think that you are overusing static variables. There are a bunch of ways to deal with this but in this example I will just put them as local variables in a method. A future extension would be to create a MonsterConfiguration class that keeps these variables as fields, then you can create several MonsterConfigurations, and perhaps read some MonsterConfigurations from XML?

In your Monster class, add String name as a parameter to the constructor and remove the abstract keyword from it.

public Monster createBandit() {
    int minDefense = 1;
    int maxDefense = 1;
    int minStrength = 1;
    int maxStrength = 3;
    int minAttack = 1;
    int maxAttack = 3;
    int maxHp = 4;

    int defense = random.nextInt(maxDefense-minDefense+1) + minDefense;
    int strength = random.nextInt(maxStrength-minStrength+1) + minStrength;
    int attack = random.nextInt(maxAttack-minAttack+1) + minAttack;
    int hp = random.nextInt(maxHp-minHp+1) + minHp;
    return new Monster("Bandit", defense, strength, attack, hp);
}

I would suggest moving these things to fields in the class or perhaps preferably, methods of the Monster class:

    double min = minDefense+minStrength+minAttack+((double)minHp/10);
    double max = maxDefense+maxStrength+maxAttack+((double)maxHp/10);
    double avg = ((max+min)/2)-4;
    averageLevelDouble = (double)Math.round((avg/STATS_PER_LEVEL)*10)/10;
    averageLevelString = (Math.round(min)-4)/STATS_PER_LEVEL + "-" +
                 (Math.round(max)-4)/STATS_PER_LEVEL;

These methods could be called something like: getMin, getMax, getAvg, getAverageLevelDouble, getAverageLevelString.

Now if you want to have a Skeleton, just make a createSkeleton method.


Intersection of a Stream of Collections An API (I have no control over) gives me a bunch of Collections (Lists, to be precise,) and I need the elements that are common to all of them. In this concrete case, I'm trying to find Selenium WebElements that match several selectors. But I went off on a Yak-Shaving spree and tried to figure out how to make an intersection of a stream of Collections. This is what I came up with (and I think it works, at least my tests say so.)

/**
 * Given a stream of collections, intersect it.
 *
 * @param stream A stream of collections you want to intersect
 * @param <T> Any item that has a decent .equals()/.hashCode().
 *            Seriously, don't attempt this without a working .equals()/.hashCode().
 * @return The unique(!) elements present in *all* collections in the stream
 */
public static <T> Collection<T> intersect(Stream<Collection<T>> stream) {
  // Optimization: sorting by size so that the biggest constrainer
  // (smallest collection) comes first
  final Iterator<Collection<T>> allLists = stream.sorted(
        (l1, l2) -> l1.size() - l2.size()
  ).iterator();

  final Set<T> result = new HashSet<>(allLists.next());
  while(allLists.hasNext()) {
      result.retainAll(allLists.next());
  }
  return result;
}

I fiddled around with Collectors in the beginning, but that seemed to be too convoluted, since it'd require the unit element to be some special class.

Any better way of doing it? Is this woefully inefficient? I think its complexity is somewhere around O(n) where n should be the total number of elements in all the collections.

Using built-ins

You can use built-in comparators instead of creating your own. For example

final Iterator<Collection<T>> allLists = stream.sorted(
      (l1, l2) -> l1.size() - l2.size()
).iterator();

can be written more simply using the comparingInt comparator, which compares elements according to the result of applying the given function (returning an int) to each element. In this case, you could have:

final Iterator<Collection<T>> allLists =
    stream.sorted(Comparator.comparingInt(Collection::size)).iterator();

The function returning the integer to compare is written as a method-reference refering to Collection.size(). Also, it gets rid of the comparison by subtracting the two int values, which has a corner-case.

Bug if the stream is empty

There is a bug in the current method if the given stream is empty. This is because of

final Set<T> result = new HashSet<>(allLists.next());

which unconditionally invokes next() on the iterator of the stream. You can call intersect(Stream.empty()) to verify it; it will throw a NoSuchElementException. In the case of an empty stream, the method should return an empty collection instead.

Better generics

With the current signature of

<T> Collection<T> intersect(Stream<Collection<T>> stream)

the issue is that passing for example a Stream<List<Integer>> will not compile. To verify this, you can have

intersect(Arrays.asList(Arrays.asList(1)).stream())

Instead, we can introduce a second generic type C for the collection with the following signature:

<T, C extends Collection<T>> Collection<T> intersect(Stream<C> stream)

This makes sure that you can pass any collection for the elements of the input stream. With such a signature, you can verify that the above compilation error is not there anymore.

Huge performance improvement

If you're not dealing with sets, the retainAll process is very inefficient:

result.retainAll(allLists.next());

Since this method checks to see if the collection given as parameter doesn't contain the elements of this collection (in order to remove them), using this on a List is O(n), making the whole operation O(n²). Instead, pass a new HashSet:

result.retainAll(new HashSet<>(allLists.next()));

Since the contains operation is constant time for sets, this will always be O(n), and, thus, a lot faster (at the expense of more memory).

Why sort?

The comment in your code says that the sorting step is used as an optimization, to ensure that shorter collections comes first. Intrigued, I made a benchmark comparing the code with and without sorting. It applied the two methods to a stream having 1000 and 10.000 elements where each inner collections had 100 and 1.000 elements. The elements chosen were random integers. Here are the results (Windows 10 x64, JDK 1.8.0_102, i5, 2.90 GHz):

Benchmark                 (lengthOfEach)  (totalLength)  Mode  Cnt    Score   Error  Units
StreamTest.intersect                 100           1000  avgt   30    1,757 ± 0,069  ms/op
StreamTest.intersect                 100          10000  avgt   30   18,876 ± 0,954  ms/op
StreamTest.intersect                1000           1000  avgt   30   17,287 ± 0,378  ms/op
StreamTest.intersect                1000          10000  avgt   30  177,633 ± 7,043  ms/op
StreamTest.intersectSort             100           1000  avgt   30    1,805 ± 0,080  ms/op
StreamTest.intersectSort             100          10000  avgt   30   18,434 ± 0,621  ms/op
StreamTest.intersectSort            1000           1000  avgt   30   19,472 ± 0,981  ms/op
StreamTest.intersectSort            1000          10000  avgt   30  184,440 ± 5,380  ms/op

For the values tested, this shows that there is really no measurable difference between the two, so I'd just get rid of this sorting.

Code of benchmark for completeness:

@Warmup(iterations = 10, time = 700, timeUnit = TimeUnit.MILLISECONDS)
@Measurement(iterations = 10, time = 700, timeUnit = TimeUnit.MILLISECONDS)
@BenchmarkMode(Mode.AverageTime)
@OutputTimeUnit(TimeUnit.MILLISECONDS)
@Fork(3)
public class StreamTest {

    public static <T, U extends Collection<T>> Collection<T> intersectSort(Stream<U> stream) {
        final Iterator<U> allLists = stream.sorted(Comparator.comparingInt(Collection::size)).iterator();

        if (!allLists.hasNext()) return Collections.emptySet();

        final Set<T> result = new HashSet<>(allLists.next());
        while (allLists.hasNext()) {
            result.retainAll(new HashSet<>(allLists.next()));
        }
        return result;
    }

    public static <T, C extends Collection<T>> Collection<T> intersect(Stream<C> stream) {
        final Iterator<C> allLists = stream.iterator();

        if (!allLists.hasNext()) return Collections.emptySet();

        final Set<T> result = new HashSet<>(allLists.next());
        while (allLists.hasNext()) {
            result.retainAll(new HashSet<>(allLists.next()));
        }
        return result;
    }

    @State(Scope.Benchmark)
    public static class Container {

        @Param({ "100", "1000" })
        private int totalLength;

        @Param({ "1000", "5000" })
        private int lengthOfEach;

        private List<List<Integer>> list;
        private Stream<List<Integer>> stream;

        @Setup(Level.Trial)
        public void setUp() {
            ThreadLocalRandom rnd = ThreadLocalRandom.current();
            list = rnd.ints(totalLength).mapToObj(i -> rnd.ints(lengthOfEach).boxed().collect(Collectors.toList())).collect(Collectors.toList());
        }

        @Setup(Level.Invocation)
        public void makeStream() {
            stream = list.stream();
        }

    }

    @Benchmark
    public Collection<Integer> intersectSort(Container a) {
        return intersectSort(a.stream);
    }

    @Benchmark
    public Collection<Integer> intersect(Container a) {
        return intersect(a.stream);
    }

}


Are there any issues in this `Lazy` implementation? I have written following small utility to abstract away the lazy initialization logic. (I tend to use lazy variables quite often.)

Are there any issues in this implementation?

// File: F0.java
public abstract class F0<A> {
  public abstract A apply();
}

// File: Lazy.java
public class Lazy<A> {
  private boolean isInitialized = false;
  private A content;
  private final F0<A> init;

  private Lazy(F0<A> init) {
    this.init = init;
  }

  public static <A> Lazy<A> initializedAs(F0<A> init) {
    return new Lazy<A>(init);
  }

  @Override
  public String toString() {
    return this.get().toString();
  }

  public A get() {
    if (!isInitialized) {
      content = init.apply();
      isInitialized = true;
    }
    return content;
  }

  @Override
  public boolean equals(Object o) {
    if (this == o) {
      return true;
    }
    if (o == null || getClass() != o.getClass()) {
      return false;
    }
    Lazy lazy = (Lazy) o;
    Object c = this.get();
    if (c != null ? !c.equals(lazy.get()) : lazy.get() != null) {
      return false;
    }
    return true;
  }

  @Override
  public int hashCode() {
    return this.get().hashCode();
  }
}

Edit:

Here is an example usage from the current project I am working on.

final Prerequisite prerequisite = new Prerequisite() {

  private Lazy<String> minimumRequiredJavaVersion = Lazy.initializedAs(
    new F0<String>() {
      @Override
      public String apply() {
        return SystemProperties.isWindows() && SystemProperties.is64Bit() ? "1.7" : "1.6";
      }
    }
  );

  @Override
  public boolean isSatisfied() {
    return SystemProperties.javaVersionIsMinimum(minimumRequiredJavaVersion.get());
  }

  @Override
  public String failureMessage() {
    StringBuilder sb = new StringBuilder();
    sb.append("Minimum required Java version is: " + minimumRequiredJavaVersion.get() + "\n");
    sb.append("Your system has " + JAVA_SPECIFICATION_VERSION + ".");
    return sb.toString();
  }
};

Four notes:

It's not thread-safe. init.apply() could be called multiple times from different threads. (I don't know whether it is an issue or not.)
The constructor should check null inputs. (Effective Java 2nd Edition, Item 38: Check parameters for validity)
You should use append instead of String concatenation:

StringBuilder sb = new StringBuilder();
sb.append("Minimum required Java version is: ").append(minimumRequiredJavaVersion.get()).append("\n");
sb.append("Your system has ").append(JAVA_SPECIFICATION_VERSION).append(".");
return sb.toString();

Instead of the anonymous Prerequisite inner class I'd use a named top-level class (for example, MinimumRequiredJavaVersionPrerequisite). I think it's easier to read, naming it helps readers to understand the purpose of the class.


Convert an `int` to a C string with size limitation Creating a textual version of an int to save as a C string to a buffer that is all ready allocated and maximum allowable size is determined seems common.  Thought I'd make a function to fulfill that.

If the destination size is too small or destination is NULL, return NULL.

Your critique of functionality, style, use, etc. is requested.

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

char *int_strnull(int x, char *dest, size_t dest_size) {
  if (dest != NULL) {
    // Size local buffer to x needs.  Note 10/33 just greater than log(2)
    char buf[sizeof x * CHAR_BIT * 10 / 33 + 3];
    char *p = &buf[sizeof buf - 1];
    // Work with negative absolute value to cope with INT_MIN
    int xna = x < 0 ? x : -x;

    // Form string
    *p = '\0';
    do {
      *--p = '0' - xna % 10;
      xna /= 10;
    } while (xna);
    if (x < 0) {
      *--p = '-';
    }

    // Copy string
    size_t src_size = &buf[sizeof buf] - p;
    if (src_size > dest_size) {
      // Not enough room
      return NULL;
    }
    memcpy(dest, p, src_size);
  }
  return dest;
}

Similar to snprintf(buf, sizeof buf, "%d", some_int); but without the printf() overhead.

Use some constants:

This char buf[sizeof x * CHAR_BIT * 10 / 33 + 3];

Would be a lot better if written like this: char buf[MAX_CONV_CHARS]; (or any other name your might prefer for the constant).

Also, do you really care about being that exact with the size of buf? I usually just throw some large number that can't overflow, like 128.

Return early:

Instead of nesting the entire function under an if, return early:

char *int_strnull(int x, char *dest, size_t dest_size) {

    if ((dest == NULL) || (dest_size < MIN_SIZE)) {
        return NULL;
    }

Mind your naming:

xna is an abbreviation for what exactly? Not obvious to me. x is also very vague. Call it input, number or even value.

int_strnull() also seems odd. Why not just int_to_string()?

Local buffer actually needed?

Finally, I don't really get the necessity of the local buffer buf. Why not simply write to dest directly. That would also save a memcpy at the end. I guess you did that to first figure out the total length of the string. But since it is such a small string, I would suggest just requiring that the output buffer be at least the maximum size.


Improve Parallel Cache with Reactive Extensions & Unity Interception The requirement is to be able to cache method invocations made on a boundary layer (Services layer). I'm using Unity to inject the concrete implementation of the Service layer classes. The intercepting CacheCallHandler caches all the responses across this layer.

The twist is that this is a multi-threaded environment with multiple clients invoking the same boundary layer in the context of a single request. If a service layer call is already in "flight", it should wait for the original invokers return and use that.

I've used Reactive extensions to implement this.

/// <summary>
/// Intercepts the calls and tries to retrieve from the cache
/// </summary>
class CacheCallHandler : ICallHandler
{

    [Dependency]
    public ICache RequestCache { get; set; }

    public IMethodReturn Invoke(IMethodInvocation input, GetNextHandlerDelegate getNext)
    {
        IMethodReturn mesg = null;

        string cacheKey = CacheKeyGenerator.GetCacheKey(input);

        //create the task to retrieve the data
        var task = new Task<IMethodReturn>(() =>
        {
            return getNext()(input, getNext);
        });

        //make it observable
        var observableItem = task.ToObservable();

        //try to add it to the cache
        //we need to do this in the order of Add and then try to get, otherwise multiple thread might enter the same area
        if (RequestCache.TryAdd(cacheKey, observableItem))
        {
            //if the add succeeed, it means that we are responsible to starting this task
            task.Start();
        }
        else
        {
            if ( RequestCache.TryGetValue(cacheKey, out observableItem) )
            {
                //do nothing, the observable item is already updated with the requried reference
            }
            else
            {
                throw new CacheHandlerException("Could not add to cache AND could not retrieve from cache either. Something's wrong", input);
            }
        }

        //observe the return
        if ( observableItem != null )
            mesg = observableItem.FirstOrDefault();

        if (mesg == null)
            throw new CacheHandlerException("Not return value found. this should not happen", input);

        return mesg;
    }

    /// <summary>
    /// Should always be the first to execute on the boundary
    /// </summary>
    public int Order
    {
        get { return 1; }
        set { ; }
    }
}

Personally i believe that there is no need for the reactive framework in this scenario.
To handle the multithreaded nature of your problem i would first make sure that your RequestCache (Icache) uses the ConcurrentDictionary> and exposrts the method GetOrAdd

With this your code will look like this:

/// <summary>
/// Intercepts the calls and tries to retrieve from the cache
/// </summary>
class CacheCallHandler : ICallHandler
{

    [Dependency]
    public ICache RequestCache { get; set; }

    public IMethodReturn Invoke(IMethodInvocation input, GetNextHandlerDelegate getNext)
    {
        return InvokeAsync.Result;
    }

    public Task<IMethodReturn> InvokeAsync(IMethodInvocation input, GetNextHandlerDelegate getNext)
    {
        IMethodReturn mesg = null;

        string cacheKey = CacheKeyGenerator.GetCacheKey(input);

        //create the task to retrieve the data
        var task = RequestCache.GetOrAdd(
               cacheKey,
               key => new Task.Factory.StartNew(() => getNext()(input, getNext))
               );

        return task;
    }

    /// <summary>
    /// Should always be the first to execute on the boundary
    /// </summary>
    public int Order
    {
        get { return 1; }
        set { ; }
    }
}

The asynchronous version offers the option of to blocking the calling thread when invoking it.


Design issues, tendency to write custom methods I seem to have this tendency to write custom methods, i.e. code that isn't reusable. The following seems to be one example of trying to do too much in a single method.
But the point I'm not sure about is, in order to split it up, I'd have to redo the for loop each time. Is that acceptable?

Any general pointers on how this should be structured would really be appreciated.

public static void ShowInputs(DataTable dtSProcList, List<Label> inputLabels,
    ComboBox cbSProcList, List<TextBox> inputTxtboxes,
    List<TextBox> selectedSProcInputs,
    List<String> selectedSprocDataTypes, List<string> selectedSprocParamterNames)
{
    int counter = 0;

    for (int i = 0; i < dtSProcList.Rows.Count; i++)
    {
        if (dtSProcList.Rows[i][0].ToString() == cbSProcList.SelectedItem.ToString())
        {
            string temp = dtSProcList.Rows[i][0].ToString();
            if (dtSProcList.Rows[i][1] != DBNull.Value)
            {
                inputLabels[counter].Text = dtSProcList.Rows[i][1].ToString().Remove(0, 1);
                inputLabels[counter].Visible = true;
                inputLabels[counter].Enabled = true;
                inputTxtboxes[counter].Visible = true;
                inputTxtboxes[counter].Enabled = true;

                selectedSProcInputs.Add(inputTxtboxes[counter]);
                selectedSprocDataTypes.Add(dtSProcList.Rows[i][2].ToString());

                counter++;
            }
            else
            {
                //Returns control to calling method if the parameter name is null.
                return;
            }
        }
    }
}

EDIT: I had originally posted this on SO and was asked to report it here.

Just a few small notes:

The method has seven parameters which isn't a good smell. Maybe some of them could be class field.
First, I'd reverse some conditions and use guard clauses to make the code flatten.

for (int i = 0; i < dtSProcList.Rows.Count; i++)
{
    if (dtSProcList.Rows[i][0].ToString() != cbSProcList.SelectedItem.ToString())
    {
        continue;
    }

    string temp = dtSProcList.Rows[i][0].ToString();
    if (dtSProcList.Rows[i][1] == DBNull.Value)
    {
        //Returns control to calling method if the parameter name is null.
        return;
    }

    inputLabels[counter].Text = dtSProcList.Rows[i][1].ToString().Remove(0, 1);
    inputLabels[counter].Visible = true;
    inputLabels[counter].Enabled = true;
    inputTxtboxes[counter].Visible = true;
    inputTxtboxes[counter].Enabled = true;

    selectedSProcInputs.Add(inputTxtboxes[counter]);
    selectedSprocDataTypes.Add(dtSProcList.Rows[i][2].ToString());

    counter++;
}

References:

Replace Nested Conditional with Guard Clauses in Refactoring: Improving the Design of Existing Code;
Flattening Arrow Code

0, 1 and 2 are magic numbers. Using named constants instead of numbers would make the code more readable and less fragile.
What does the counter count? You should give it a name which shows the intent of the variable.
dtSProcList.Rows[i] repeats five times in the code. A named local variable could remove this duplication.
The temp variable seems to be unused. If it's superfluous remove it.


optimize integer validation I have some classes with properties, wich could only have numeric values in a given range (e.g. 0 to 8). This should be validated and wrong values prevented with an exception. The given behavior may be implemented without duplicating the given code.
There are some ValidatorAttributes in C# but I can't get them to running. They don't seem to prevent the assignment with wrong values in a normal class.
My current solution is to copy paste this snippet for every attribute. But copy paste is not perfect:
private short _index;
public short Index
{
    get => this._index;
    private set
    {
        const short min = 0;
        const short max = 8;
        if (value < min || max < value)
        {
            throw new ArgumentOutOfRangeException("The passed value is not between " + min + "  and " + max + " (values included)");
        }
        this._index = value;
    }
}

To reduce the lines of code to copy/paste, you could create a helper method:
public static class Helper
{
    public static void EnsureIsInRange(short value, short min, short max)
    {
        if (value < min || max < value)
        {
            throw new ArgumentOutOfRangeException("The passed value is not between " + min + "  and " + max + " (values included)");
        }
    }
}

or more generic:
public static class Helper
{
    public static void EnsureIsInRange<T>(T value, T min, T max) where T : IComparable<T>
    {
        if (value.CompareTo(min) != 1 || value.CompareTo(max) != -1)
        {
            throw new ArgumentOutOfRangeException("The passed value is not between " + min + "  and " + max + " (values included)");
        }
    }
}

// usage:
private short _index;
public short Index
{
    get => this._index;
    private set
    {
        Helper.EnsureIsInRange(value, 0, 8);
        this._index = value;
    }
}


Building a graph using dictionary I have an input text file which contains distance between 2 cities shown below

Luebeck Hamburg 63
Hamburg Bremen 116
Hamburg Hannover 153
Hamburg Berlin 291
Bremen Hannover 132
Bremen Dortmund 234
Hannover Magdeburg 148
Hannover Kassel 165
Magdeburg Berlin 166
Berlin Dresden 204
Dresden Leipzig 119
Leipzig Magdeburg 125
Dortmund Duesseldorf 69
Kassel Frankfurt 185
Frankfurt Dortmund 221
Frankfurt Nuremberg 222
Leipzig Nuremberg 263
Dortmund Saarbruecken 350
Saarbruecken Frankfurt 177
Saarbruecken Karlsruhe 143
Karlsruhe Stuttgart 71
Stuttgart Frankfurt 200
Stuttgart Munich 215
Stuttgart Nuremberg 207
Nuremberg Munich 171
Manchester Birmingham 84
Birmingham Bristol 85
Birmingham London 117
END OF INPUT

My dictionary should be like this:

{
 'luebeck': [('hamburg', '63')], 'dortmund': [('bremen', '234'), ('duesseldorf', '69')]
 'saarbruecken': [('dortmund', '350'), ('frankfurt', '177'), ('karlsruhe', '143')]
  ..
 }

My code is working but it is not efficient and I want to optimize it:

def read_file(filename):
    paths = []
    graph = {}
    f = open(filename, 'r')
    for line in f:
        route = line.lower().split()
        paths.append(route)

    # to remove space and end of input
    del paths[-1]
    del paths[-1]

    for path in paths:
        node = path[0]
        if node not in graph:
            graph[node] = {}
            graph[node][path[1]] = int(path[2])
        elif node in path:
            graph[node][path[1]] = int(path[2])
            node = path[1]
            if node not in graph:
                graph[node] = {}
        node = path[1]
        if node not in graph:
            graph[node] = {}
            graph[node][path[0]] = int(path[2])
        elif node in path:
            graph[node][path[0]] = int(path[2])
    return graph

Please tell me if any additional information is required.

To many accumulators, paths and graph, you just need one.
One for loop to many, you can do everything with one loop.
open with no close is a no no. Sure you could aruge that the codebase is small and you will just run the function once and then terminate... But it's good practice to always close your file handlers. Instead of using open() and close() you shuold just use with open(filename, 'r') as f:, because with clause take care of close() for you.
Route is not use anywhere else so no need for it.
You are doing del paths[-1] twice so you will actually remove one vaild path.
The for loop is stupidly complex for such a simple problem. I dont even know where to start in my quest of understading it.

graph[node][path[0]] doesn't do what you say it does; it makes a dict of dict, and not a list of tuples in a dict as you specified. eg. the code returns {birmingham : {'manchester': 84, 'bristol': 85}} instead of {birmingham : [('manchester', 84), ('bristol', 85)]}.
So theres a lot of if else statements in the loop to solve the problem of: "what to do when the dict/list isn't instantiated" . This can be done in a simpler way with dict.setdefault(), this function basically checks if the dict has an list (dict or whatever you want) as a child, and if it dosen't it will instantiate it for you.

I dont think that's a graph because not everything in that dataset is connected to eachother, its more like a set of graphs.

def new_sol(filename):
    graph = {}
    with open(filename, 'r') as f:
        for line in f:
            city1, city2, distance = line.lower().split()
            if distance == "input":
                break
            #what you say you want
            graph.setdefault(city1, []).append((city2,int(distance)))
            graph.setdefault(city2, []).append((city1,int(distance)))
            #same as your code
            #graph.setdefault(city1, {})[city2] = int(distance)
            #graph.setdefault(city2, {})[city1] = int(distance)
    return graph
if __name__ == "__main__":
    print(new_sol("test.txt"))

Time analysis

You wanted to optimize the code, so lets us do an time analysis to see if there is some thing we can optimize in our algorithms, or if the optimaztion is just in the implementaion.

askers code

read line: \$O(N)\$
put line in arr: \$O(1)\$
iterate over paths: \$O(N)\$
put the diffrent directions in a dict \$O(2)\$

\$O(N*1+N*2) = O(N*3) = O(N)\$

proposed code

read line: \$O(N)\$
put line in arr/dict: \$O(2*(1+1))\$, \$(1+1)\$ comes from lookup/insertion + insertion

\$O(N*4) = O(N)\$

So from a algorithmic standpoint both your code and my code preforme equally well, and that is the fastst we can do it.
From an implentaion standpoint my code is a bit faster but as \$N\$ goes toward infinity it dosent really matter.


log(n) "sort by value" Map? I've read so much arguing about "sort by value maps". Well, I wrote the following log(n) sort by value Map . And, by the way, a real Map , does not allow duplicate keys. Many of the maps I googled ignore that annoying problem. I assume this map is still log(n)...:

public static void sortByValue_logN() {

    class MyComp implements Comparator {
        Map<Object, Integer> sharedMap;

        public int compare(Object key1, Object key2) {
            if(key1.equals(key2)) { return 0; }

            Integer val1 = sharedMap.get(key1);
            Integer val2 = sharedMap.get(key2);

            if(val1 > val2) return -1;
            return 1;
        }

    }

    class TreeMapByValue<K> extends TreeMap<K, Integer> {
        Map<Object, Integer> sharedMap = new HashMap<Object, Integer>();
        TreeMapByValue(Comparator comp) {
            super(comp);
        }

        @Override
        public Integer put(K key, Integer val) {
            if(sharedMap.containsKey(key)) {
                super.remove(key);
                val += sharedMap.get(key);
            }
            sharedMap.put(key, val);

            super.put(key, val);

            return val;
        }
    }

    MyComp myComp = new MyComp();
    TreeMapByValue<Object> myMap = new TreeMapByValue(myComp);
    myComp.sharedMap = myMap.sharedMap;

    // all the rest is just testing the map
    Random rand = new Random();
    String[] story = { "The", "quick", "brown", "fox", "jumped" };

    for(int repeat = 0; repeat < 3; repeat++) {
        for(int i = 0; i < story.length; i++) {
            String key = story[i];
            int value = rand.nextInt(100);
            myMap.put(key, value);
            System.out.println(key + "-->" + value);
        }
    }

    for(Map.Entry<Object, Integer> entry : myMap.entrySet()) {
        System.out.println("key = " + entry.getKey() + "___value = " + entry.getValue());
    }
}

Surely (at least stylistically) something is bad about the above code.

I don't understand generics yet, so I am ok with ignoring that for now. I am also ignoring thread-safety issues as well.

My goal is to create a log(n) "sort by value" map that respects the property of not having duplicate keys. So, what is wrong with what I did?

put seems quite broken, which in turn breaks get.

From the documentation,

  V put(K key, V value)

  Associates the specified value with the specified key in this map
  (optional operation). If the map previously contained a mapping for
  the key, the old value is replaced by the specified value. (A map m is
  said to contain a mapping for a key k if and only if m.containsKey(k)
  would return true.)

  Returns:
  the previous value associated with key, or null if there was no mapping for key. (A null return can also indicate that the map previously associated null with key, if the implementation supports null values.)

Let's take this code

  myMap.put("x", 1);
  int oldValue = myMap.put("x", 2);
  System.out.printf("%d %d%n", oldValue, myMap.get("x"));

I would expect it to print 1 2, but it prints 3 3.