Razor view to render simple scaffolding for complex type After several attempts to follow my intuition that this code can be refactored to something more DRY, I keep hitting road blocks.

Hoping someone can see a refactor that might get me going in a nicer direction.

I'm trying not to break anything out of this one code file, although, I suppose helper types could be placed in a top level code block if they help.

This razor view essentially presents all string string types in an arbitrarily complex type as a single form:

Member.cshtml

@model Tuple<object, Type, string>
@{
    var targetObj = Model.Item1;
    var targetType = Model.Item2;
    var path = Model.Item3;
}

    @foreach (var prop in
        from p in targetType.GetProperties()
        where p.Name != "SyncRoot"
        orderby p.Name
        select p)
    {
        var name = (path + "." + prop.Name).TrimStart('.');
        if (prop.PropertyType == typeof (string))
        {
            var value = "";
            if (targetObj != null)
            {
                var propValue = prop.GetValue(targetObj);
                if (propValue != null)
                {
                    value = propValue.ToString();
                }
            }

                @prop.Name
                <input class="form-control" value="@value" name="@name"/>

        }
        else if (prop.PropertyType.IsArray && prop.PropertyType.GetElementType() == typeof (string))
        {
            var value = "";
            if (targetObj != null)
            {
                var propValue = prop.GetValue(targetObj) as string[];
                if (propValue != null)
                {
                    value = propValue[0];
                }
            }

                @(prop.Name)[]
                <input class="form-control" value="@value" name="@(name)[0]"/>

        }
        else if (prop.PropertyType.IsArray)
        {
            object value = null;
            if (targetObj != null)
            {
                value = prop.GetValue(targetObj);
            }

                <fieldset>
                    <legend>@(prop.Name)[]</legend>
                    @Html.Partial("Member", Tuple.Create(value, prop.PropertyType.GetElementType(), name))
                </fieldset>

        }
        else if (!prop.PropertyType.Name.StartsWith("System."))
        {
            object value = null;
            if (targetObj != null)
            {
                value = prop.GetValue(targetObj);
            }

                <fieldset>
                    <legend>@prop.Name</legend>
                    @Html.Partial("Member", Tuple.Create(value, prop.PropertyType, name))
                </fieldset>

        }
    }

Having a chain of if...elseif s it is a  good place to start having a registry/resolver container instead.

Please take a look at my answer here.

After refactoring the code and removing the nested if....else s you will have all the logic and actions in one place to look at and you can refactor them in order to be DRY.


Parsing notification information from distribution lists I'm writing am application that I've previously posted questions on here, but I'm looking for similar advice if possible. I am looking for how best to write my code, as I have a working example, but to me it feels long winded.

The application first grabs 3 tables via DataContext:-

        var contacts = db.GetTable<Contact>();
        var distributionLists = db.GetTable<DistributionList>();
        var JunctionTable = db.GetTable<ContactsDistribution>();

I then get the selected items from my listbox, which are distribution lists. A contact can be in more than one distribution list, so I first get the raw data from my database:

var listBoxItems = listBox1.SelectedItems.Cast<object>().Select(t => t.ToString());

var initialList = (from j in JunctionTable
                      where listBoxItems.Contains(j.DistributionName)
                      join c in contacts on j.ContactID equals c.ContactID
                      select new { c.ContactID,c.Surname,j.DistributionName, j.EmailFlag, j.SMSFlag}).ToList();

I then need to search the initialList collection for users who require email notifications.  I also need to remove duplicate entries, as a Contact can appear multiple times. So the list is made Distinct:-

var email = (from l in initialList
                       where l.EmailFlag.Equals(true)
                       select new { l.ContactID }).Distinct().ToList();

I then do the same search for Contacts that require SMS Notification from the selected lists:

var sms = (from l in initialList
                       where l.SMSFlag.Equals(true)
                       select new { l.ContactID }).Distinct().ToList();

Now that I have lists for both SMS & Email Notifications I need get the required email address or mobile number by doing this:

var smsMobileNumbers = (from s in sms
                        join c in contacts on s.ContactID equals c.ContactID
                        select new { c.MobileNumber }).ToList();

var emailAddresses = (from m in email
                     join c in contacts on m.ContactID equals c.ContactID
                     elect new { c.EmailAddress }).ToList();

Is there a cleaner way of writing this code or is there a better, more efficient way of achieving this?

The only change I would make would be to remove the two statements where you create the lists sms and email.  They are redundant: by adding the EmailAddress and MobileNumber to the query where you are creating the initialList, you can do a select on the initialList.

var initialList = (from j in JunctionTable
                where listBoxItems.Contains(j.DistributionName)
                join c in contacts on j.ContactID equals c.ContactID
                select new { c.ContactID, c.Surname, c.EmailAddress, c.MobileNumber,
                                j.DistributionName, j.EmailFlag, j.SMSFlag }).ToList();

I changed these more because I like this syntax better than the SQL like Linq.

var emailAddresses = initialList
    .Where(l => l.EmailFlag)
    .Select(l => l.EmailAddress).Distinct().ToList();

var smsMobileNumbers = initialList
    .Where(l => l.SMSFlag)
    .Select(l => l.MobileNumber).Distinct().ToList();


Function to generate radom alphanumeric cookies I wrote this python function to generate random values to use as id for the cookies I have on my project.

These are some examples of what the return is:

'kck9874118vq{5470553602125615945684311372005'
'72195739g112py3472118v104h122z5038a100d118v912296336399110nu110n60041909'

That is the method:

def generateCookieID(userName):

    cookieId = ''
    for a in range(0,9):
        aux = randint(0,9999)
        if aux % 2==0:
            for i in range(1,4):
                letter = randint(97,123)
                if letter%2==1:
                    cookieId += chr(letter)
                else:
                    cookieId += str(letter)+chr(letter)
        cookieId += str(aux)
    cookieJar['name'] = userName
    cookieJar['id'] = cookieId
    return cookieId

It works, but is it appropriate to use this way?
What could I do better?

If you are on python 3.6, you can just use the secrets module: which you should be using, in case the random value should be secret.

From the documentation:

>>> token_urlsafe(16)
'Drmhze6EPcv0fN_81Bj-nA'

if you're below python 3.6, see here, how it's implemented. You  might just copy that.


How is my implementation of an immutable stack? Below is my implementation of an immutable stack class.  The reverse function is trying to return a stack with all elements reversed. Is my implementation good? Maybe the reverse function can be faster?

public class ImStack<T> {

    private final T head;
    private final ImStack<T> tail;

    public ImStack ( T head, ImStack<T> tail)
    {
        this.head = head;
        this.tail = tail;
    }

    public ImStack<T> pop()
    {
        return this.tail;
    }
    public ImStack<T> push( T e)
    {
        return new ImStack<T>( e, this);
    }
    public T peek()
    {
        return this.head;
    }
    public boolean isEmpty()
    {
        if ( head == null && tail == null)
            return true;
        else return false;
    }
    public int size()
    {
        if ( isEmpty())
            return 0;
        else return 1 + tail.size();
    }
    public ImStack<T> reverse()
    {
        ImStack<T> resultStack = new ImStack<T> ( null,null);
        ImStack<T> tempStack;
        tempStack = this;
        for ( int i = 0 ; i < this.size(); i++)
        {
            resultStack = resultStack.push(tempStack.peek());
            tempStack = tempStack.pop();
        }
        return resultStack;
    }
}

Writing an elegant immutable stack is difficult in Java, mainly due to the lack of multiple return values. But your code does look quite fine.

I am comparing your API with Scala's immutable stack. One interesting bit is that there the signature of push translates to:

public <T2 super T> ImStack<T2> push(T2 e)

which looks uncomfortable but makes a lot of sense too: Adding elements to a stack can widen the type but never narrow it down. Keeping the current type is a special case of this.

Your isEmpty method is unfortunately flawed. Assume I have created a stack of one element, like:

Object myObject = null;
ImStack<Object> stack = new ImStack<Object>(myObject, null);

Now stack.peek() is correctly null. On an empty stack, this should have created an exception, not returned a null.

One solution to solve this problem is to have isEmpty() always return false. We also create a private subclass¹ of ImStack called ImEmptyStack, where isEmpty() returns true, and peek() throws an exception. In your constructor, you create a new empty stack as tail when that argument would be null:

ImStack(T head, ImStack<? extends T> tail) {
  this.head = head;
  this.tail = tail != null ? tail : new ImEmptyStack<T>();
}

1: Actually, subclassing here leads to problems. The two classes should both be subtypes of a common interface. So basically, the Composite Pattern.

By overriding the size() in the ImEmptyStack, you can simplify that as well:

class ImStack<T> {
  public int size() {
    return 1 + tail.size();
  }
}

class ImEmptyStack<T> extends ImStack<T> {
  public int size() {
    return 0;
  }
}

Back to your isEmpty() method, code like this should never happen:

if ( head == null && tail == null)
    return true;
else return false;

because it is equivalent to return(head == null && tail == null). Every time you write code like return foo() ? true : false, you know you can simplify that.

I'd like to sketch your reverse() method in a more functional manner, using an accumulator argument:

public ... reverse() {
  return this.reverse(new ImEmptyStack<T>());
}

private ... reverse(ImStack<...> acc) {
  return tail.reverse(acc.push(head));
}

// Empty stack:

private ... reverse(... acc) {
  return acc;
}

This is as efficient as your solution, because your size call also traverses the whole stack recursively. The advantage of my solution is the simplicity. Of course, it is trivial to rewrite this to an iterative solution, because the above sketch is tail recursive:

reverse() {
  ImStack<T> accumulator = new ImEmptyStack();
  ImStack<T> current     = this;
  while(!( current instanceof ImEmptyStack<T> )) {
    accumulator = accumulator.push(current.peek());
    current     = current.pop();
  }
  return accumulator;
}

Note that .pop() can never return null in my adaption of your code (an empty stack should throw an exception).

Something minor that I noticed is your inconsistent spacing around the argument lists of your methods:

ImStack ( T head, ImStack<T> tail)  // left outer and inner space
pop()  // no space
push( T e) // left inner space

This is no problem in itself, but it would be better to settle for one style.


Ad-hoc analytics script to module/package I have a text analytics process that I complete for work. This process mines text for different Twitter accounts and finds patterns in the tweets. The actual machine learning pipeline is much more complicated than what's shown below, but this will have to suffice as my minimum reproducible example. In reality, there are a lot more instances when I asked for user input in the script, so I don't think things like user input should be stored in the same script as the functions.

Right now, I run this script on an ad-hoc basis, and a lot of the things are hard-coded. I want to be able to package the code I've written to make it more robust and stable.

How should the functions before __main__ and the procedures in __main__ be split up to best make this code a package? Should the functions be saved in a different script altogether, and then be imported when needed to a different script? Right now, I just run these scripts from the command line and am sort of lost as to where to go in terms of packaging it.

Text data cleaning script:

import pandas as pd

from nltk.corpus import stopwords

import re
import string

import os

def strip_punct(string, regex):
    return regex.sub(' ', string)

def clean_string(string, regex, stopwords):
    words_to_keep = []
    stripped_string = strip_punct(string, regex)
    string_words = stripped_string.split()
    for word in string_words:
        if word not in stopwords:
            words_to_keep.append(word)
    cleaned_string = " ".join(words_to_keep)
    return cleaned_string

def clean_and_map_column(column, df, regex, stopwords):
    text_set = set(df[column])
    text_dict = dict((string, clean_string(row, regex, stopwords)) for row in text_set)
    df[column] = df[column].map(text_dict)
    return df[column]

if __name__ == '__main__':

    os.chdir("/path/to/my/folder")

    # regular expression containing punctuation to be removed from strings
    regexp = re.compile('[%s]' % re.escape(''.join(p for p in string.punctuation)))

    # set of stopwords to remove from text from NLTK
    stops = stopwords.words('english')

    # my data frame, containing only columns of text
    df = pd.read_csv("file.csv")

    # clean each column
    for col in df.columns.values:
        df[col] = clean_and_map_column(col, df, regexp)

    df.to_csv("cleaned_file.csv")

KMeans clustering script:

from sklearn.feature_extraction.text import CountVectorizer
from sklearn.cluster import KMeans

import pandas as pd

import os

if __name__ == '__main__':

    os.chdir("/path/to/my/folder")

    df = pd.read_csv("cleaned_file.csv")

    corpus_column = 'twitter_data'

    corpus = df[corpus_column]

    vectorizer = CountVectorizer()

    X = vectorizer.fit_transform(corpus)

    model = KMeans(k = 5)

    model.fit(X)

    df['cluster_id'] = model.labels_

    df.to_csv('kmeans_file.csv")

You may want to start by converting the process under __main__ into a function called main and then have it take command line arguments. That way, you separate user-configurable items from the actual program.

For example:

import re
import string
import sys
import os

import pandas as pd
from nltk.corpus import stopwords

def main(cmd_line_args):
  path_to_data_folder = cmd_line_args[1]
  os.chdir(path_to_data_folder)

  # regular expression containing punctuation to be removed from strings
  regexp = re.compile('[%s]' % re.escape(''.join(p for p in   string.punctuation)))

  # set of stopwords to remove from text from NLTK
  stops = stopwords.words('english')

  # my data frame, containing only columns of text
  data_frame_filename =  cmd_line_args[2]
  df = pd.read_csv(data_frame_filename+".csv")

  # clean each column
  for col in df.columns.values:
      df[col] = clean_and_map_column(col, df, regexp)

  cleaned_filename = cmd_line_args[3]
  df.to_csv(cleaned_filename+".csv")

if __name__ == '__main__':
  main(sys.argv)


Given a 2D array of digits, try to find the occurrence of a given 2D pattern of digits My algorithm is quite simple. I simply iterate over the entire map arr and if I find the start of the combination I need to find, I begin exploring.

My combination data structure is simply the combination I need to find, column at a time.

If each column in arr matches, I return True. If at any point the value doesn't match the combination I need I return False.

I believe this is O(n+m) time with O(1) space. Code below...

def explore(arr, combination, i, j):
    """
    for each column in the combination we have to find
    compare it with what is present in the map (arr)
    """
    for row in combination:
        for count, item in enumerate(row):

            # compare the map with the combination value we're up to
            # if it doesn;t match, return False and stop
            if arr[i+count][j] != item:
                return False
        j+=1
    return True

def find_combination_in_arr(arr, combination, ):
    for i, row in enumerate(arr):
        for j, item in enumerate(row):

            # if we have found the start of the combination, then start exploring
            if item == combination[0][0]:
                if explore(arr, combination, i, j):
                    return "FOUND IT!"

# the map we need to search
arr = [
    [1, 1, 2, 3, 4, 1, 1],
    [1, 1, 5, 6, 7, 1, 1],
    [1, 1, 2, 7, 4, 1, 1],
    [1, 1, 7, 8, 6, 1, 1]
]

# the combination we need to find
combination = [
    [2, 5, 2, 7],
    [3, 6, 7, 8],
    [4, 7, 4, 6]
]

find_combination_in_arr(arr, combination)

Duplication

Your nested loops are almost equal, write a generator that contains the loops and use it twice.

Power

enumerate has more power then you think: you can specify a starting index, so you do not need manual increment.

all

You want all items to be equal, so use all.

Minimalism

Return a boolean not a string, a string is an unecessary complication.


Ensuring that attributes referenced by a mixin are present in the children class I have implemented the below _get_schema method to ensure that schema attribute is always present on the Todos instance, and can therefore be used in the mixin.
Is it OK to do it like this? As far as I know, mixins usually do not implement constructors.
Is there a better way to make sure that someone did not forget to explicitly set the schema attribute in the Todos class, especially that this attribute is also not inherited from View?
class Mixin:
    def __init__(self):
        self._get_schema()
        super().__init__()

    def _get_schema(self):
        if not hasattr(self, 'schema'):
            msg = f'No "schema" attribute provided in {self.__class__.__name__} class'
            raise Exception(msg)

class View:
    def __init__(self):
        print('view constructor')

class Todos(SchemaMixin, MethodView):
    schema = 'todo schema'

todos = Todos()

The point of a mixin, at least as I've always understood it, is to inject
methods into other classes -- and not to create instances of the mixin class
itself. So putting an __init__() in a mixin does seem like an odd pattern. Of
course, __init__() is just a method: why not inject that method too? Fair
enough, but it still feels off. And is it really necessary?
One option is to inject the schema checking method and then call it in a proper
__init__():
class SchemaMixin:

    def _check_schema(self):
        ...

class Todos(SchemaMixin):

    def __init__(self):
        ...
        self._check_schema()

Or just use regular functions and get out of the awkward business of
inheritance. Without knowing more details about your case, I would lean in this
direction.
def check_schema(obj):
    ...

class Todos():

    def __init__(self):
        check_schema(self)

I find myself almost never using inheritance these days -- regular or
mixin-style. I don't have a blanket policy against it, but I increasingly find
that I just don't need it. If a class needs something, I pass it in as an
argument. If multiple classes need the same operation, I use functions.


Copy directories while changing Unicode filenames to ASCII I created a short Perl 6 script copyfnameascii.pl to copy a file hierarchy I have, applying an URL Decoding to the names of folders and removing non-ASCII characters from the names of files.

$ cd workingdirectory
$ perl6-m ../copyfnameascii.pl project copy

It is best illustrated by the following diagram:

                             $ tree
                             .
                             ├── copy
                             │   ├── folder 1
$ tree                       │   │   ├── cr.txt
.                            │   │   └── es.txt
└── project                  │   └── folder 2
    ├── folder%201           │       ├── ai.txt
    │   ├── čř.txt           │       └── zy.txt
    │   └── ěš.txt   ~*>     └── project
    └── folder%202               ├── folder%201
        ├── áí.txt               │   ├── čř.txt
        └── žý.txt               │   └── ěš.txt
                                 └── folder%202
                                     ├── áí.txt
                                     └── žý.txt

I just started using Perl. I'd like to hear your advice how to make the code generally better; more readable, idiomatic, and so on. The script does work; there is just a few things I am not satisfied with.

use Inline::Perl5;
my $p5 = Inline::Perl5.new;
$p5.use('Text::Unidecode');

sub transformdirname($dirname is copy) {
  $dirname.=subst( /:i \%(<[0..9A..F]>**2) /, { chr(:16(~$0)) }, :g ); # undo URL encoding
  $dirname.=subst( /\//, '_', :g ); # make it a valid UNIX filename
  return $dirname;
}

sub transformfilename($filename) {
  return $p5.call('Text::Unidecode::unidecode', $filename); # make it ASCII at all costs
}

sub MAIN($fromdir, $todir) {
  for dir($fromdir) -> $subdir {
    unless $subdir ~~ :d {
      next;
    }
    my $tosubdir = $todir ~ '/' ~ transformdirname($subdir.basename);
    say $tosubdir;
    mkdir($tosubdir);
    for dir($subdir) -> $file {
      my $tofile = $tosubdir ~ '/' ~ transformfilename($file.basename);
      if $tofile.path ~~ :e {
        die("Will not overwrite $tofile");
      }
      copy($file, $tofile);
    }
  }
}

My overall take was "Nice!". :)

Given your exchange with Patrick J S above (not to mention my lack of familiarity with the wrinkles discussed in the relevant RFCs) I'm not going to comment at all on the technical-compliance-with-standards aspects of your decoding logic.

Simplify Inline::Perl5 code

use Inline::Perl5;
my $p5 = Inline::Perl5.new;
$p5.use('Text::Unidecode');
.
.
sub transformfilename($filename) {
  return $p5.call('Text::Unidecode::unidecode', $filename);
}

That was old style. Write this way instead:

use Text::Unidecode:from<Perl5>;

sub transformfilename($filename) {
  return Text::Unidecode::unidecode($filename);
}

Don't repeat yourself?

sub transformdirname($dirname is copy) {
  $dirname.=subst( /:i \%(<[0..9A..F]>**2) /, { chr(:16(~$0)) }, :g );
  $dirname.=subst( /\//, '_', :g );
  return $dirname;
}

This has 'dirname' in the sub's name plus four mentions of $dirname. Sometimes repeats are valuable, perhaps making code easier to read or change, but in this case I think the reverse applies. I'd write:

sub transformdirname($_ is copy) {
  .=subst( /:i \%(<[0..9A..F]>**2) /, { chr(:16(~$0)) }, :g )
  .=subst( /\//, '_', :g )
}

transformfilename has a similar issue:

sub transformfilename($filename) {
  return $p5.call('Text::Unidecode::unidecode', $filename);
}

('filename' is written three times.) In addition it's a one line routine introducing the abstract "transform" and I'm going to assume the transformation is unlikely to change, so abstracting it out to a sub is questionable. In addition there's another several 'file's in the call so instead I'd elide the sub altogether and eliminate all the repeats of 'file'. We'll see that later in the final version where I've made some additional changes.

Comments

Imo good code means good comments. You've written good comments like:

sub transformdirname($dirname is copy) {
  $dirname.=subst( /:i \%(<[0..9A..F]>**2) /, { chr(:16(~$0)) }, :g ); # undo URL encoding
  $dirname.=subst( /\//, '_', :g ); # make it a valid UNIX filename
  return $dirname;
}

In Raku you can effortlessly turn such ordinary end-of-line comments (that start with #) into Pod declarator comments (that start with #| or #= and appear immediately before or after a declaration):

#| undo URL encoding and make it a valid UNIX filename
sub transformdirname($_ is copy) {
  .=subst( /:i \%(<[0..9A..F]>**2) /, { chr(:16(~$0)) }, :g );
  .=subst( /\//, '_', :g );
}

Now documentation tools can automatically extract documentation of the sub because this:

say &transformdirname.WHY;

now displays:

undo URL encoding and make it a valid UNIX filename

(Why is the method that extracts such doc called WHY? It's to remind folk that by far the most important thing to explain in code comments is WHY -- why the code has been written and/or why it's written the way it is.)

I note you've used MAIN:

sub MAIN($fromdir, $todir) {

As you know, this automatically generates a nice usage message. What you may not know is that this can be combined with the Pod declarator blocks discussed above to great effect. Checkout The Cool subset of MAIN for examples.

Final version

I've made many more changes to be the way I write things. As I wrote at the start, your original code looks nice and many coding issues are subjective.

use Text::Unidecode:from<Perl5>;

sub MAIN ($fromdir, $todir) {

  for $fromdir.dir {

    next unless .d;

    say my $tosubdir = "$todir/&transformdirname(.basename)";
    mkdir $tosubdir;

    for .dir {

      # make filename ASCII at all costs:
      my $tofile = "$tosubdir/{.basename.&Text::Unidecode::unidecode}";

      if $tofile.path.e { die "Will not overwrite $tofile" }

      copy $_, $tofile;

    }
  }
}

# undo URL encoding and make it a valid UNIX filename
sub transformdirname($_ is copy) {
  .=subst( /'%' (<xdigit>**2)/, { chr(:16(~$0)) }, :g )
  .=subst( /\//, '_', :g )
}


Bresenham's line algorithm implementation May there be any performance and/or code standard improvements on the following LineOfSight code?

static List<Coordinate> getIntersects(Coordinate c1, Coordinate c2) {
    //  Bresenham's line algorithm from http://rosettacode.org/wiki/Bitmap/Bresenham's_line_algorithm#C.2B.2B

    List<Coordinate> returningVector = new List<Coordinate>();
    double y1 = c1.y, x1 = c1.x, y2 = c2.y, x2 = c2.x;

    bool steep = (Math.Abs(y2 - y1) > Math.Abs(x2 - x1));
    if(steep)
    {
        double _t;
        _t = x1;
        x1 = y1;
        y1 = _t;
        _t = x2;
        x2 = y2;
        y2 = _t;
    }

    if(x1 > x2)
    {
        double _t;
        _t = x1;
        x1 = x2;
        x2 = _t;
        _t = y1;
        y1 = y2;
        y2 = _t;
    }

    double dx = x2 - x1;
    double dy = Math.Abs(y2 - y1);

    double error = dx / 2.0f;
    int ystep = (y1 < y2) ? 1 : -1;
    int y = (int)y1;

    int maxX = (int)x2;

    for(int x=(int)x1; x<maxX; x++)
    {
        if(steep)
        {
            Coordinate tc = new Coordinate(y, x);
            returningVector.Add(tc);
        }
        else
        {
            Coordinate tc = new Coordinate(x,y);
            returningVector.Add(tc);
        }

        error -= dy;
        if(error < 0)
        {
            y += ystep;
            error += dx;
        }
    }
    if(steep)
    {
        Coordinate tc = new Coordinate(y2, x2);
        returningVector.Add(tc);
    }
    else
    {
        Coordinate tc = new Coordinate(x2,y2);
        returningVector.Add(tc);
    }
    return returningVector;
}

static bool isObstacle(Coordinate c1, Coordinate c2, Coordinate c3) {
    double heightOn_c3 = Math.Abs(((c1.height - c2.height) * Tools.distanceXY(c2, c3)) / Tools.distanceXY(c2, c1));
    return (heightOn_c3 + c2.height) < c3.height;
}

public static bool calculateLOS(Coordinate c1, Coordinate c2, Map map) {
    List<Coordinate> intersects = getIntersects(c1, c2);
    for(int i=0; i<intersects.Count; i++) {
        if(isObstacle(c1, c2, map.points[(int)intersects[i].x][(int)intersects[i].y].coordinates)){
            return false;
        }
    }
    return true;
}

The least you could do is move if(steep) out of the for loop, saving one conditional in every iteration.

In the initializations, you could use a swap function.

Why is the name of the function getIntersects?

I don't know C#, but if new Coordinate(x,y) is using the heap for every single point, this does not sound very efficient.


A simple thread-safe Deque in C++ I am trying to implement a thread-safe deque in C++.
ThreadSafeDeque will be used by a FileLogger class.
When threads call the log() function of FileLogger the messages will be push_back()ed to ThreadSafeDeque and return almost immediately. In a separate thread the FileLogger will pop_front() messages and write them to a file in its own pace.
Am I doing things correctly and efficient below?

#pragma once
#include <deque>
#include <mutex>
template<class T>
class ThreadSafeDeque {
public:
    void pop_front_waiting(T &t) {
        // unique_lock can be unlocked, lock_guard can not
        std::unique_lock<std::mutex> lock{ mutex }; // locks
        while(deque.empty()) {
            condition.wait(lock); // unlocks, sleeps and relocks when woken up
        }
        t = deque.front();
        deque.pop_front();
    } // unlocks as goes out of scope

    void push_back(const T &t) {
        std::unique_lock<std::mutex> lock{ mutex };
        deque.push_back(t);
        lock.unlock();
        condition.notify_one(); // wakes up pop_front_waiting
    }
private:
    std::deque<T>               deque;
    std::mutex                  mutex;
    std::condition_variable condition;
};

It's probably worth accepting a second template argument for an Allocator to be passed through to the std::deque:

template<class T, class Allocator = std::allocator<T>>
class ThreadSafeDeque {
    std::deque<T, Allocator> deque;
};

Technically, we do need to include <condition_variable>; we're not allowed to assume that <mutex> always pulls it in.

I don't like the interface to pop_front_waiting() - why does the caller have to pass an lvalue reference, instead of simply being returned the value?  Return-value optimisation will ensure there's no unnecessary copy.

The locking logic all looks good to me.  We can avoid the lock.unlock() by using a simple lock-guard with smaller scope:

void push_back(const T &t)
{
    {
        std::lock_guard<std::mutex> lock{ mutex };
        deque.push_back(t);
    }
    condition.notify_one(); // wakes up pop_front_waiting
}

From C++17, we can use constructor deduction and omit the template parameter from std::lock_guard and std::unique_lock, making the code a little easier to read.

It's helpful to show that members are intentionally default-constructed, and it pacifies g++ -Weffc++:

std::deque<T> deque = {};
std::mutex mutex = {};
std::condition_variable condition = {};


Is there a cleaner way to add DEBUG comments? I have a rather large class that needs to provide a reasonable amount of output for debugging purposes. I've done this with the following:

#if DEBUG
Console.WriteLine("Source Site Set to: {0}", archiveQueueEntity.SourceSite);
Console.WriteLine("Source List Set to: {0}", archiveQueueEntity.SourceUrl);
Console.WriteLine("Destination Site Set to {0}", archiveQueueEntity.DestinationSite);
Console.WriteLine("Destination List Set to: {0}", archiveQueueEntity.DestinationUrl);
#endif

Is there a better way to do this though?

After googling this I tried using Debug.WriteLine but it appears that this will only output to the 'output' window in Visual Studio, and not the console.

Am I missing something?

I think it would be much cleaner to utilize partial methods to create your logging statements.  That way you can log wherever you need it and can disable the code my omitting it the logging function definition.  By using partial methods, if the definition is omitted, no IL is generated for the method and calls to the partial method are ignored as if it was never there.

Just mark the class partial, define the signature of the partial method and call it like normal.  Then wrap the actual implementation in the conditional compilation blocks.

partial class MyClass
{
    // declare the partial method
    static partial void Log(string format, params object[] arguments);

    static void SomeMethod()
    {
        // call the log method like usual
        Log("Source Site Set to: {0}", archiveQueueEntity.SourceSite);
        Log("Source List Set to: {0}", archiveQueueEntity.SourceUrl);
        Log("Destination Site Set to {0}", archiveQueueEntity.DestinationSite);
        Log("Destination List Set to: {0}", archiveQueueEntity.DestinationUrl);
    }

#if DEBUG
    static partial void Log(string format, params object[] arguments)
    {
        Console.WriteLine(format, arguments);
    }
#endif
}

Otherwise if you're not able to change it, you should still create a separate logging method and disable to actual printing in the method there.  That way your method is called but does nothing.

static void Log(string format, params object[] arguments)
{
#if DEBUG
    Console.WriteLine(format, arguments);
#endif
}

Or alternatively, use the Trace class to do your logging.  As long as you have no listeners registered, you will not see any of the logging messages.  When debugging, add a ConsoleTraceListener to the Listeners collection.

#if DEBUG
    Trace.Listeners.Add(new ConsoleTraceListener());
#endif

Trace.WriteLine("Source Site Set to: {0}", archiveQueueEntity.SourceSite);
Trace.WriteLine("Source List Set to: {0}", archiveQueueEntity.SourceUrl);
Trace.WriteLine("Destination Site Set to {0}", archiveQueueEntity.DestinationSite);
Trace.WriteLine("Destination List Set to: {0}", archiveQueueEntity.DestinationUrl);

Something that eluded me until now, use the ConditionalAttribute attribute on the log function for both cases to achieve the same effect.

[Conditional("DEBUG")]
static void Log(string format, params object[] arguments)
{
    Console.WriteLine(format, arguments);
}


SharedMutex concept implementation I was curious to implement something similar to SharedMutex concept.
So far only 4 methods implemented. I decided to omit try_lock, try_lock_shared and native_handle for the sake of simplicity.

#ifndef READ_WRITE_MUTEX_H__
#define READ_WRITE_MUTEX_H__

#include <atomic>
#include <mutex>
#include <condition_variable>

struct ReadWriteMutex {

  void lock() { // block everyone
    std::unique_lock<std::mutex> lock(_mutex);

    if (_writing || _readers > 0) {
      // wait while writer and/or reader(s) have finished
      _cv_writing.wait(lock, [this] {return !_writing && _readers == 0;});
    }

    _writing = true;  // set flag to block everyone
                      // current thread continue run
  }

  void unlock() {
    std::lock_guard<std::mutex> lock(_mutex);
    _writing = false;
    _cv_writing.notify_all();
    _cv_reading.notify_all();
  }

  void lock_shared() { // block writers, allow readers
    std::unique_lock<std::mutex> lock(_mutex);
    if (_writing) { // we should wait : writer is working
      _cv_reading.wait(lock, [this] {return !_writing; });
    }

    ++_readers;
  }

  void unlock_shared() {
    std::lock_guard<std::mutex> lock(_mutex);
    if (--_readers == 0) {
      _cv_writing.notify_all();
    }

  }

private:
  mutable std::mutex _mutex;
  mutable std::condition_variable _cv_writing;
  mutable std::condition_variable _cv_reading;

  std::atomic<bool> _writing = false;
  std::atomic<int> _readers = 0;

};

#endif // READ_WRITE_MUTEX_H__

C++11 support assumed.

Just a quick heads up about your use of leading underscores. This is discouraged because they are reserved by the standard.
You should find another way to mark variables as members (if you find it's necessary). I've seen a lot of people using trailing underscores for this. In my opinion trailing indentifiers are smarter as otherwise you will have to type whatever prefix you picked everytime you want to address a member variable (which also slows down autocomplete).


Canonicalize URLs for static website I want to "canonicalize" URLs for my static (files and folders) website.

The 'Aims' describes what I want to accomplish. The 'Code' gives my current .htaccess.

Everything works right now, but I wonder if the code could be improved:

any changes for better performance?
can some rules be removed?
can some rules be merged?
can something be shortened?
are some explaining comments wrong?

In my .htaccess there shouldn't be anything else as what is described in 'Aims' and my examples. So if there should be something that has nothing to do with it, it is probably unneeded (if I don't miss an important part right now).

Aims

Strip the file ending .html (but keep all other endings).
if the file is index.html, strip "index", too, and don't keep a (folder) trailing slash
no trailing slashs for files or folders

if someone adds a trailing slash, redirect to variant without

Example 1

Physical file: example.com/foo/bar.html
Desired URL: example.com/foo/bar
URLs that should redirect (301) to desired URL:

example.com/foo/bar.html
example.com/foo/bar/

Example 2 (if index.html)

Physical file: example.com/foo/index.html
Desired URL: example.com/foo
URLs that should redirect (301) to desired URL:

example.com/foo/index.html
example.com/foo/index
example.com/foo/
example.com/foo.html

Example 3 (if non-HTML file)

Physical file: example.com/foo/bar.png
Desired URL: example.com/foo/bar.png (= same as physical)
URLs that should redirect (301) to desired URL:

none

Code (.htaccess)

# Turn MultiViews off. (MultiViews on causes /abc to go to /abc.ext.)
Options +FollowSymLinks -MultiViews

# It stops DirectorySlash from being processed if mod_rewrite isn't.
<IfModule mod_rewrite.c>

  # Disable mod_dir adding missing trailing slashes to directory requests.
  DirectorySlash Off

  RewriteEngine On

  # If it's a request to index(.html)
  RewriteCond %{THE_REQUEST} \ /(.+/)?index(\.html)?(\?.*)?\  [NC]
  # Remove it.
  RewriteRule ^(.+/)?index(\.html)?$ /%1 [R=301,L]

  # if request has a trailing slash
  RewriteCond %{REQUEST_URI} ^/(.*)/$
  # but it isn't a directory
  RewriteCond %{DOCUMENT_ROOT}/%1 !-d
  # and if the trailing slash is removed and a .html appended to the end, it IS a file
  RewriteCond %{DOCUMENT_ROOT}/%1.html -f
  # redirect without trailing slash
  RewriteRule ^ /%1 [L,R=301]

  # Add missing trailing slashes to directories if a matching .html does not exist.
  # If it's a request to a directory.
  RewriteCond %{REQUEST_FILENAME}/ -d
  # And a HTML file does not (!) exist.
  RewriteCond %{REQUEST_FILENAME}/index.html !-f
  # And there is not trailing slash redirect to add it.
  RewriteRule [^/]$ %{REQUEST_URI}/ [R=301,L]

  RewriteCond %{REQUEST_FILENAME} -d
  # And a HTML file exists.
  RewriteCond %{REQUEST_FILENAME}/index.html -f
  # And there is a trailing slash redirect to remove it.
  RewriteRule ^(.*?)/$ /$1 [R=301,L]

  RewriteCond %{REQUEST_FILENAME} -d
  # And a HTML file exists.
  RewriteCond %{REQUEST_FILENAME}/index.html -f
  # And there is no trailing slash show the index.html.
  RewriteRule [^/]$ %{REQUEST_URI}/index.html [L]

  # Remove HTML extensions.
  # If it's a request from a browser, not an internal request by Apache/mod_rewrite.
  RewriteCond %{ENV:REDIRECT_STATUS} ^$
  # And the request has a HTML extension. Redirect to remove it.
  RewriteRule ^(.+)\.html$ /$1 [R=301,L]

  # If the request exists with a .html extension.
  RewriteCond %{SCRIPT_FILENAME}.html -f
  # And there is no trailing slash, rewrite to add the .html extension.
  RewriteRule [^/]$ %{REQUEST_URI}.html [QSA,L]

</IfModule>

(some parts of this file are by Jon Lin over at Stack Overflow)

First, I'll start with an observation that applying these rewriting rules — in particular, removing trailing slashes in the URL — can break pages that reference relative URLs.  I assume you know that and want to proceed anyway.

In general, you should realize that every RewriteRule is conditional.  If the path does not match the pattern, the RewriteRule and all of its RewriteConds are skipped.  You should take advantage of that fact to eliminate a few superfluous RewriteConds.

You have seven RewriteRules:

Rule 1

# If it's a request to index(.html)
RewriteCond %{THE_REQUEST} \ /(.+/)?index(\.html)?(\?.*)?\  [NC]
# Remove it.
RewriteRule ^(.+/)?index(\.html)?$ /%1 [R=301,L]

It is highly unorthodox to use %{THE_REQUEST}, which works at the raw HTTP level (e.g., GET /index.html HTTP/1.1).  It contains the raw URL, whereas you usually care about the decoded path.  The fix is to remove the RewriteCond altogether, since it is redundant anyway.

I recommend incorporating Rule 6 into this rule as a simplification.
Rule 2

# if request has a trailing slash
RewriteCond %{REQUEST_URI} ^/(.*)/$
# but it isn't a directory
RewriteCond %{DOCUMENT_ROOT}/%1 !-d
# and if the trailing slash is removed and a .html appended to the end, it IS a file
RewriteCond %{DOCUMENT_ROOT}/%1.html -f
# redirect without trailing slash
RewriteRule ^ /%1 [L,R=301]

Again, the first RewriteCond should be incorporated into the RewriteRule instead.  There's no need to use %{REQUEST_URI}, since RewriteRule naturally works on paths.

Avoid hard-coding the assumption that the resources are relative to the document root.  They may have been remapped to another portion of the filesystem.
Rule 3

# Add missing trailing slashes to directories if a matching .html does not exist.
# If it's a request to a directory.
RewriteCond %{REQUEST_FILENAME}/ -d
# And a HTML file does not (!) exist.
RewriteCond %{REQUEST_FILENAME}/index.html !-f
# And there is not trailing slash redirect to add it.
RewriteRule [^/]$ %{REQUEST_URI}/ [R=301,L]

Avoid referencing %{REQUEST_URI} in the RewriteRule.  Instead, use regular expression capturing:

RewriteRule (.*[^/])$ $1/ [R=301,L]

Rule 4

RewriteCond %{REQUEST_FILENAME} -d
# And a HTML file exists.
RewriteCond %{REQUEST_FILENAME}/index.html -f
# And there is a trailing slash redirect to remove it.
RewriteRule ^(.*?)/$ /$1 [R=301,L]

The directory test is superfluous.  Also, since Rule 2 also strips trailing slashes, I would swap this with Rule 3 so that the rules to strip trailing slashes are placed together.
Rule 5

RewriteCond %{REQUEST_FILENAME} -d
# And a HTML file exists.
RewriteCond %{REQUEST_FILENAME}/index.html -f
# And there is no trailing slash show the index.html.
RewriteRule [^/]$ %{REQUEST_URI}/index.html [L]

Again, the directory test is superfluous, and you can avoid referencing %{REQUEST_URI} by using regular expression capturing.
Rule 6

# Remove HTML extensions.
# If it's a request from a browser, not an internal request by Apache/mod_rewrite.
RewriteCond %{ENV:REDIRECT_STATUS} ^$
# And the request has a HTML extension. Redirect to remove it.
RewriteRule ^(.+)\.html$ /$1 [R=301,L]

As previously mentioned, this can be incorporated into Rule 1.
Rule 7

# If the request exists with a .html extension.
RewriteCond %{SCRIPT_FILENAME}.html -f
# And there is no trailing slash, rewrite to add the .html extension.
RewriteRule [^/]$ %{REQUEST_URI}.html [QSA,L]

Use regular expression capturing.  Also, use of %{SCRIPT_FILENAME} is weird and inconsistent with the rest of the rules.

I found that the ruleset was prone to infinite redirects.  As you probably found, when the non-redirecting rules rewrite the URL, they trigger an internal subrequest in Apache, causing the entire ruleset to be evaluated again.  You probably encountered this problem and put in the check for %{ENV:REDIRECT_STATUS} as a workaround.  I would generalize that check by making it the very first rule.

Options +FollowSymLinks -MultiViews

<IfModule mod_rewrite.c>
  # Disable mod_dir adding missing trailing slashes to directory requests.
  DirectorySlash Off

  RewriteEngine On

  ######################################################################
  # Canonicalizing redirects
  ######################################################################

  # Skip all rewrites of internal subrequests (see below).
  RewriteCond %{ENV:REDIRECT_STATUS} .
  RewriteRule . - [L]

  # Strip .html or /index.html
  RewriteRule ^(.*?)(/index)?(\.html)$ $1 [NC,R=301,L]

  # Strip trailing slash if...
  # It isn't a directory, and if the trailing slash is removed and a .html
  # appended to the end, it IS a file.
  RewriteCond %{REQUEST_FILENAME} !-d
  RewriteCond %{REQUEST_FILENAME} (.*?)/?$
  RewriteCond %1.html -f [OR]
  RewriteRule (.*)/$ $1 [R=301,L]

  # Strip trailing slash if...
  # It is a directory that contains an index.html file.
  RewriteCond %{REQUEST_FILENAME} (.*?)/?$
  RewriteCond %1/index.html -f
  RewriteRule (.*)/$ $1 [R=301,L]

  # Add trailing slash if...
  # If is a directory and index.html does not exist.
  RewriteCond %{REQUEST_FILENAME} -d
  RewriteCond %{REQUEST_FILENAME}/index.html !-f
  RewriteRule (.*[^/])$ $1/ [R=301,L]

  ######################################################################
  # URL-to-filesystem mapping
  #
  # Even with the [L] flag, these mapping rules will trigger an internal
  # subrequest, causing the mod_rewrite ruleset to be re-evaluated.
  # Therefore, to prevent infinite recursion, the canonicalizing
  # redirect rules above need to skipped if %{ENV:REDIRECT_STATUS} is
  # set.
  ######################################################################

  # If there is no trailing slash, try appending /index.html
  RewriteCond %{REQUEST_FILENAME}/index.html -f
  RewriteRule (.*[^/])$ $1/index.html [L]

  # If there is no trailing slash, try appending .html
  RewriteCond %{REQUEST_FILENAME}.html -f
  RewriteRule (.*[^/])$ $1.html [QSA,L]
</IfModule>


String Trimmer and trimmed Input routine As I thought about this question on SO, I realized that I wanted to implement either a string trimmer or a trimmed string inputter.

This is what I came up with, but I have a nagging feeling I could have done better.

Would you critique this, please?

#include <string>
#include <iostream>
#include <sstream>
#include <cassert>

template <typename Ch, typename Tr, typename Alloc>
void
trim(std::basic_string<Ch, Tr, Alloc>& str, const std::locale& loc)
{
  // (b,e)=ltrim(str)
  typename std::basic_string<Ch, Tr, Alloc>::const_iterator b = str.begin();
  typename std::basic_string<Ch, Tr, Alloc>::const_iterator e = str.end();
  while( b != e && std::isspace(*b, loc) )
    b++;

  // (b,e)=rtrim(b,e)
  // Query: this loop looks u-g-l-y. Can it be less ugly?
  while( b != e ) {
    e--;
    if(!std::isspace(*e, loc)) {
      e++;
      break;
    }
  }

  // str=(b,e)
  // Query: this string self-assigns (sort of). Is this UB?
  str.assign(b, e);
}

template <typename Ch, typename Tr, typename Alloc>
std::basic_istream<Ch,Tr>&
getTrimmedLine(std::basic_istream<Ch,Tr>& is,
  std::basic_string<Ch, Tr, Alloc>& str)
{

  // Get raw data from input
  std::getline(is, str);

  // And trim it
  trim(str, is.getloc());

  return is;
}

#define TEST
#ifdef TEST
template<typename T, std::size_t N>
std::size_t countof( T (& a)[N] ) { return N; }

int main () {
  const char * const inputs[] = {
    "simple", " front", "back ", " both ",
    "  front2", "back2  ", "  both2  ",
    "\n", "", "\n\n\n",
    "  \n", "  ", "  \n\n\n"
  };
  const char * const outputs[] = {
    "simple", "front", "back", "both",
    "front2", "back2", "both2",
    "", "", "",
    "", "", ""
  };

  assert(countof(inputs) == countof(outputs));

  for(std::size_t i = 0; i < countof(inputs); i++) {
    std::istringstream is(inputs[i]);
    std::string result;
    getTrimmedLine(is, result);
    assert(result == outputs[i]);
  }
}
#endif

typename std::basic_string<Ch, Tr, Alloc>::const_iterator e = str.end();
...

// Query: this loop looks u-g-l-y. Can it be less ugly?
  while( b != e ) {
    e--;
    if(!std::isspace(*e, loc)) {
      e++;
      break;
    }
  }

This looks weird and tends to go UB.

1) e = str.end();

2) e--; /* it's better to use --e in this case */ you move one more far position after end()!!!

3) std::isspace(*e, loc) <--- UB, accessing the memory that haven't been allocated

If you need to find the first and the last spaces in the string, use this:

b = find_if(str.begin(),  str.end(),  isspace_predicate);
e = find_if(str.rbegin(), str.rend(), isspace_predicate);


The 3n + 1 algorithm for a range Essentially the Collatz Conjecture.

Challenge requirements are here.

And here is my implementation in Java:

/* User: koray@tugay.biz Date: 21/03/15 Time: 19:40 */

public class ThreeNOneForRangeSolver {

    public int solveFor(int inputOne, int inputTwo) {
        int maximumCycleLength = 0;

        do {
            if (threeNOneValueFor(inputTwo) > maximumCycleLength) {
                maximumCycleLength = threeNOneValueFor(inputTwo);
            }
            inputTwo = inputTwo - 1;
        } while (inputTwo >= inputOne);

        return maximumCycleLength;
    }

    private static int threeNOneValueFor(int inputNumber) {
        int counter = 1;
        while (inputNumber != 1) {
            if (inputNumber % 2 == 0)
                inputNumber = inputNumber / 2;
            else
                inputNumber = 3 * (inputNumber) + 1;
            counter++;
        }
        return counter;
    }
}

and the test class:

/* User: koray@tugay.biz Date: 21/03/15 Time: 20:54 */

public class TestClass {

    public static void main(String[] args) {
        int one = 1;
        int two = 10;

        int result = new ThreeNOneForRangeSolver().solveFor(one, two);

        System.out.println(one + " " + two + " " + result);
    }

}

Any feedback is welcome.

Looks pretty good over all, but I have a few suggestions.

Don't call threeNOneValueFor twice when you could store it in a variable instead (or you can use Math.max). There's a chance that the compiler will optimize that, but that's a risky thing to assume.

The do-while seems a bit odd and unintuitive. I would go with a for loop instead.

int maxCycleLength = 0;
for (; inputOne <= inputTwo; ++inputOne) {
    maxCycleLength = Math.max(threeNOneValueFor(inputOne), maxCycleLength);
}
return maxCycleLength;

inputOne and inputTwo are pretty bad names. If you didn't already know what they were, the names would really mean much to you. Something like minimumValue, rangeBegin, etc might be better.

inputNumber is a pretty meh name as well. Considering the problem domain, I'd probably just call it n since that has a well defined meaning in this context and inputNumber could mean pretty much anything (it's an int parameter -- of course it's an input number).

I would call solveFor maximumCycleLength. solveFor has the same problem as inputNumber. You could be solving for pretty much anything that takes two inputs.

public int maximumCycleLength(int minN, int maxN) {

Seems significantly clearer as to what the method does. Couple that with a javadoc and it becomes very well explained:

/**
 * Calculate the maximum cycle length for the {@code 3n + 1} sequence over every {@code n} such that
 * {@code minN <= n <= maxN}. Note that {@code maxN} must be greater than or equal to {@code minN}.
 * @param minN The beginning (inclusive) of the range over which to find the maximum cycle length.
 * @param maxN The end (inclusive) of the range over which to find the maximum cycle length.
 * @return The maximum cycle length of the {@code 3n + 1} sequence for all {@code n} in the provided range.
*/
public int maximumCycleLength(int minN, int maxN) {

(I would probably omit the @param and @return parts if I were to write this javadoc for my own code, but people tend to vehemently disagree with that.)

I'm probably getting pretty annoying at this point, but I don't much like the name threeNOneValueFor either. It doesn't find the valueFor; it finds the cycle length.

public int threeNOneCycleLength(int n) {

Considering it's on a class that already mentions the 3n + 1 problem, I would probably name it just cycleLength.

In it's current form, I would probably make solveFor static. There's no state, so there's no real reason in having it be an object. Typically I lean far more towards non-static than static, since it's much better to regret something being non-static than it is to regret having made it static, but in this case, since it's pretty much a mathematic function, I doubt it's ever going to manipulate state (hopefully not anyway).

In fact, I'd probably take this a step farther and have threeNOneCycleLength be public. It could be useful on it's own, and since both methods are static utility-like methods, there's not too much concern about having the class do too many things.

All in all (woo boring Saturday afternoons!), I might do something like this:

/**
 * Provides methods related to the {@code 3n + 1} sequence.
 *
 * @see Collatz conjecture.
*/
public class ThreeNPlusOne {

    /**
     * Calculate the maximum cycle length for the {@code 3n + 1} sequence over every {@code n} such that
     * {@code minN <= n <= maxN}. Note that {@code maxN} must be greater than or equal to {@code minN}.
     *
     * @param minN The beginning (inclusive) of the range over which to find the maximum cycle length.
     * @param maxN The end (inclusive) of the range over which to find the maximum cycle length.
     * @return The maximum cycle length of the {@code 3n + 1} sequence for all {@code n} in the provided range.
    */
    public static int maximumCycleLength(int minN, int maxN) {
        int maxCycleLength = 0;
        for (; minN <= maxN; ++minN) {
            maxCycleLength = Math.max(maxCycleLength, cycleLength(minN));
        }
        return maxCycleLength;
    }

    /**
     * Calculates the cycle length for the provided {@code n}.
     *
     * @param n The value of {@code n} for which the
     * @return The cycle length for the provided {@code n}.
    */
    public static int cycleLength(int n) {
        int cycles = 1;
        while (n != 1) {
            n = (n % 2 == 0) ? n / 2 : 3*n + 1;
            cycles += 1;
        }
        return cycles;
    }

}

I'm not very happy with some of my namings (is "Collatz sequence" a recognized thing? I don't know what to call all of this), and some of the javadocs could use some better wording and whatnot, but hopefully this shows the structure and approach well enough.


TeamCity TestComplete runner in Python I am new to using Python and the following is the first script that I have written. I will execute a test complete project and will be started via team city. Once test execute has been called - the log file needs to be processed for consumption through TeamCity.

import sys
import argparse
import subprocess
import os.path

parser = argparse.ArgumentParser()
parser.add_argument("-s", "--projectSuite", help=".pjs that you want to execute", type=str)
parser.add_argument("-p", "--project", help=".pj that you want to execute", type=str, default="ExploratoryTests")
parser.add_argument("-l", "--logDirectory", help="The directory to log the mht's too", type=str, default="C:\TestLogs")
parser.add_argument("-lf", "--logFile", help="The name of the log", type=str, default="TextExecuteLog.mht")
parser.add_argument("-ps", "--psexec", help="Absolute path to psexec", type=str, default="c:\Program Files (x86)\PsTools\PsExec.exe")
parser.add_argument("-te", "--testExecute", help="Absolute path to testExecute", type=str, default="c:\Program Files (x86)\SmartBear\TestExecute 9\Bin\TestExecute.exe")
parser.add_argument("-m", "--mhtExtractor", help="Absolute path to an mht extractor", type=str, default="C:\extract2htm\extractMHT.exe")

#Get the args that are passed in
args = parser.parse_args()

#Make sure we have an absolute path
suite = os.path.abspath(args.projectSuite)

#Prepare the project for consumption by TestExecute
project = "/p:" + args.project

#Get a path for the currenct mht
logFile = os.path.abspath(os.path.join(args.logDirectory, args.logFile))

#Tell TestExecute where to put the log
log = "/exportlog:" + logFile

#prepare TE Executables
psexec = os.path.abspath(args.psexec);
testExecute = os.path.abspath(args.testExecute)

#Execute testExecute through psexec
testExecuteCommand = [psexec,"-i" ,"-accepteula", testExecute, suite, project, "/exit", log, "/r", "/DoNotShowLog", "/SilentMode"]

#Blank array to hold the TE outcome
testExecuteError = [];

try:
    result = subprocess.run(args = testExecuteCommand, stderr=subprocess.STDOUT, shell=True)
    result.check_returncode()
    testExecuteError.append(result)
except subprocess.CalledProcessError as e:
    result = e.returncode
    testExecuteError.append(e)
    print("Error code: {0}".format(result))

extractor = os.path.abspath(args.mhtExtractor)
outputDir = os.path.join(args.logDirectory)

extractorExecuteCommand = [extractor, logFile, outputDir]

try:
    result = subprocess.run(args = extractorExecuteCommand, stderr=subprocess.STDOUT, shell=True)
    result.check_returncode()
except subprocess.CalledProcessError as err:
    result = err.returncode
    print("Error code: {0}".format(result))

#exit the script with the exit code from testexetute
sys.exit(testExecuteError[0])

The way in which I handle my exceptions from testExecute are the part that I'm not happy about but it is working as intended. Any suggestions are welcome.

type=str is already the default way argparse interprets things. However, for all the options where the only further processing is calling os.path.abspath, you can use that as type. This affects args.psexec and args.testExecute.

Python has an official style-guide, PEP8, which you should follow. It recommends using lower_case_with_underscore for variables and functions.

Comments should explain why the code is the way it is, not what it does. The latter should be obvious from the code itself, the name of the function you encapsulated it in and/or the docstring accompanying that function.

In that vein, you could use more functions. One candidate for this would be get_args, where you can stick all your argparse code. Also the two subprocess calls could be in their own function, since they are completely independent. The least you should do is wrap your code in if __name__ == "__main__":. This way you can import this module from another script without running the tests.

In Python, ; serve (almost) no purpose, even though they are not a SyntaxError. You definitely do not need them at the end of normal lines.

Final code:

import sys
import argparse
import subprocess
import os.path

def get_args():
    parser = argparse.ArgumentParser()
    parser.add_argument("-s", "--projectSuite", help=".pjs that you want to execute", type=os.path.abspath)
    parser.add_argument("-p", "--project", help=".pj that you want to execute", default="ExploratoryTests")
    parser.add_argument("-l", "--logDirectory", help="The directory to log the mht's too", default="C:\TestLogs", type=os.path.join)
    parser.add_argument("-lf", "--logFile", help="The name of the log", default="TextExecuteLog.mht")
    parser.add_argument("-ps", "--psexec", help="Absolute path to psexec", type=os.path.abspath, default="c:\Program Files (x86)\PsTools\PsExec.exe")
    parser.add_argument("-te", "--testExecute", help="Absolute path to testExecute", type=os.path.abspath, default=r"C:\Program Files (x86)\SmartBear\TestExecute 9\Bin\TestExecute.exe")
    parser.add_argument("-m", "--mhtExtractor", help="Absolute path to an mht extractor", type=os.path.abspath, default="C:\extract2htm\extractMHT.exe")
    args = parser.parse_args()
    args.project = "/p:" + args.project
    args.logFile = os.path.abspath(os.path.join(args.logDirectory, args.logFile))
    args.log = "/exportlog:" + logFile
    return args

def execute_test(psexec, testExecute, suite, project, log, **kwargs):
    """Execute testExecute through psexec"""
    command = [psexec,"-i" ,"-accepteula", testExecute, suite, project, "/exit", log, "/r", "/DoNotShowLog", "/SilentMode"]

    #Blank array to hold the TE outcome
    testExecuteError = []
    try:
        result = subprocess.run(args=command, stderr=subprocess.STDOUT, shell=True)
        result.check_returncode()
        testExecuteError.append(result)
    except subprocess.CalledProcessError as e:
        testExecuteError.append(e)
        print("Error code: {0.returncode}".format(e))
    return testExecuteError

def execute_extractor(mhtExtractor, logDirectory, logFile, **kwargs):
    """Descriptive docstring"""
    command = [extractor, logFile, logDirectory]
    try:
        result = subprocess.run(args=command, stderr=subprocess.STDOUT, shell=True)
        result.check_returncode()
    except subprocess.CalledProcessError as e:
        print("Error code: {.returncode}".format(e))

if __name__ == "__main__":
    args = vars(get_args())
    testExecuteError = execute_test(**args)
    execute_extractor(**args)

    sys.exit(testExecuteError[0])

Note that I made all parameters part of the argparse.Namespace to have it all in one place. Afterwards I used vars to convert it to a dict. Then I used keyword arguments to make the function calls easier but at the same time giving the parameters of the function names.

To see how the dictionary unpacking works, have a look at this example:

def f(x, y, **kwargs):
    print x, y, kwargs

def g(x, z, **kwargs):
    print x, z, kwargs

args = {'x': 1, 'y': 2, 'z': 3}
f(**args)
# 1 2 {'z': 3}
f(1, 2)
# 1 2 {}
g(**args)
# 1 3 {'y': 2}

kwargs swallows all left-over keyword arguments and allows in my code above the passing in of the whole args dict, even though both functions need only some of its content.


Simple Tic-Tac-Toe PHP/Pure HTML I've been doing a simple Tic-Tac-Toe with PHP, generating only HTML code.

A few notes:

I didn't bother yet to give an AI to the opponents (the Os), and it is intentional.
There is no CSS style to it, I intend to append it with a css file.
tictactoe.php is naturally the name of the script itself, so each links are refering to the page itself.

Now I don't know if I did everything correctly, here are my concern:

If I want to expand the functionalities of the game in the future (adding javascript, opponent AIs, new game features), I fear that the structure of the code is not adapted for it. Should I change my code so it is Object Oriented? EDIT I have in mind to add new feature to the game, like powers who would change the state of the board, like rewind to a previous state and; some css and javascript in a non-intrusive way and add some way to check if options are disabled (like javascript/flash), or check for the version of the browser for html5/css3 features.
As for now, it is easy to "cheat" and directly introduce the wished board state. I would like to prevent that, and if possible without using javascript, as I want the game to be playable also without it.

<?php
//Determine if a player as aligned three of its symbols and return the id of the player (1
//for X Player, -1 for O Player(Computer)). Otherwise return 0;
function isWinState($board){
    $winning_sequences = '012345678036147258048642';
    for($i=0;$i<=21;$i+=3){
        $player = $board[$winning_sequences[$i]];
        if($player == $board[$winning_sequences[$i+1]]){
            if($player == $board[$winning_sequences[$i+2]]){
                if($player!=0){
                    return $player;
                }
            }
        }
    }
    return 0;
}

//Player O plays its turn at random
function OsTurn($board){
    if(in_array(0,$board)){
        $i = mt_rand(0,8);
        while($board[$i]!=0){
            $i = mt_rand(0,8);
        }
        $board[$i]=-1;
    }
    return $board;
}

$winstate = 0;
$values = array();

if(empty($_GET['values'])){
    //initializing the board
    $values = array_fill(0,9,0);
    //determine who begins at random
    if(mt_rand(0,1)){
        $values = OsTurn($values);
    }
}else{
    //get the board
    $values = explode(',',$_GET['values']);
    //Check if a player X won, if not, player 0 plays its turn.
    $winstate = isWinState($values);
    if($winstate==0){
        $values = OsTurn($values);
    }
    //Check if a player 0 won
    $winstate = isWinState($values);
}
//Display the board as a table
for($i=0;$i<9;$i++){
    //begin of a row
    if(fmod($i,3)==0){echo '
';}
    echo '';
    //representation of the player token, depending on the ID
    if($values[$i]==1){
        echo 'X';
    }else if($values[$i]==-1){
        echo 'O';
    }else{
        //If noone put a token on this, and if noone won, make a link to allow player X to
        //put its token here. Otherwise, empty space.
        if($winstate==0){
            $values_link = $values;
            $values_link[$i]=1;
            echo '&nbsp;';
        }else{
            echo '&nbsp;';
        }
    }
    echo '	';
    //end of a row
    if(fmod($i,3)==2){echo '
';}
}
?>

<?php
//If someone won, display the message
if($winstate!=0){
    echo '
Player '.(($winstate==1)?'X':'O').' won!
';
}
?>

There isn't really a reason to make this "more object-oriented" if you don't need to. To add more AI constructs, for example, you can do:

switch ($aiLevel) {
   case 1: $values = $OsTurn($values);
   case 2: $values = $betterAi($values);
   case 3: $values = $unbeatableAi($values);
}

Adding more features could be done in a similiar manner. I'm not sure exacly wht you have in mind, so I can only make some general comments. Break down everything into functions, and make sure that it's easy to see the flow of the program. The way you've structured your code is good. It should not be difficult to add more features if you use similiar style.

CSS won't affect your script; it just changes how the page looks. You'll just need to be sure you use the right elements when outputting html. Javascript may be trickier, but there are many ways of doing that including form elements (possibly hidden) and page submits. There may be other ways that I'm not aware of as I'm not primarily a web programmer.

I believe when you are talking about cheating, you're saying that you can send a winning board value set to the script by typing the URL yourself. To prevent this you need a way to preserve state. A cookie would be an easy way to start; store the current state of the board in it after every page call, and check that the only change was the placing of another piece. A more robust, but somewhat more involved, solution would be to use session variables to store the state. This would avoid the "cookys are bad" problem and the possibility that someone might fake the cooky!


Appending URL parameters to links I've got some pages that may or may not load with a ReturnUrl parameter, and some links that will be generated without that parameter (though I've applied a class to them).

The site uses jQuery, and I think I've found the most elegant and least breaky solution as follows:

$(document).ready(function() {
    $('a.my_returnlink').each(function(){
        $(this).attr('href',$(this).attr('href')+window.location.search);
    });
});

That is, given a page url like http://example.com/page.aspx?ReturnUrl=blahblahblah and the following link on the page: Some link, that link will end up pointing to http://example.com/someotherpage.aspx?ReturnUrl=blahblahblah.

Is there any reason not to do it this way?

Generally speaking, doing something with jQuery will perform worse than doing that same thing directly through the DOM. In this case, using the following code inside the each will yield better performance:

this.href += window.location.search;

Note, however, that code will create an invalid URL if there is already a query on the original href.


Checking an authentication decision in a JSON response I've been working on this login screen logic.

It checks for the parameters in the JSON response to decide which action to follow. It works but it's kinda ugly and verbose. Any suggestions to refactor to make it look more elegant and simple?

  Alamofire.request(apiLink, method: .post, parameters: parameters, encoding: URLEncoding.default, headers: nil)
    .responseJSON { response in
            guard response.result.error == nil else {
                print("error calling POST in /###/###/###")
                print("Error: \(response.result.error)")
                return
            }

            if let JSON = response.result.value {
                print(response.result)
                print("JSON: \(JSON)")

                if let dictionary = response.result.value as? [String: Any] {
                    if let status = dictionary["status"] as? String {
                        if status == "ok" {
                              self.performSegue(withIdentifier: "Home", sender: nil)
                        } else {
                            if let message = dictionary["message"] as? String {
                                if message == "invalid_username" {
                                    let alertMessage = UIAlertController(title: "Invalid User", message: "Please check out the user name", preferredStyle: .alert)
                                    alertMessage.addAction(UIAlertAction(title: "Ok", style: .default, handler: nil))
                                    self.present(alertMessage, animated: true, completion: nil)
                                } else {
                                    if let message = dictionary["message"] as? String {
                                        if message == "incorrect_password" {
                                            let alertMessage = UIAlertController(title: "Invalid Password", message: "Please check out the password", preferredStyle: .alert)
                                            alertMessage.addAction(UIAlertAction(title: "Ok", style: .default, handler: nil))
                                            self.present(alertMessage, animated: true, completion: nil)
                                        }
                                    }
                                }
                            }
                        }
                    }
                }
            }
        }
    }

Guard Statements

My proposed changes takes advantage of Swift's guard statements. In this case I find them useful not only for breaking up the nested if statements, but also for clarifying the "requirements" or key conditions and increasing the readability for others and yourself.

For example:

guard let dictionary = json as? [String: Any] else {
   return
}

Can be interpreted as:

  "This dictionary must be valid or else we cannot continue"

The alternative to this is using a traditional if statement. However, this can lead to additional nesting and potentially also requires you to handle the alternative conditions, or in other words the else and else if conditions.

if let dictionary as? [String: Any] else {
    // I need another if statement to get something from the dictionary, but what if that key is not in the dictionary?
}

// Do I need an else condition? What if the dictionary is not valid?

Repetitive Code

The use of the guard statements also had the effect of eliminating some repetitive code used for setting up the UIAlertController. Repetitive code can lead to additional maintenance overhead and also bugs where you make a change in one place but forgot to replicate it in the duplicate code.

Unused Constant

I also made use of the json constant you declared. It seems that after it was declared, you did not use it in the block that follows.

Additional note

My proposed changes also incorporates oopexpert's advice. From a security standpoint, it would be better to not be specific as to why a login attempt has failed.

Proposed Refactoring

Alamofire.request(apiLink, method: .post, parameters: parameters, encoding: URLEncoding.default, headers: nil).responseJSON {
    response in

    guard response.result.error == nil else {
        print("error calling POST in /###/###/###")
        print("Error: \(response.result.error)")
        return
    }

    if let json = response.result.value {
        print(response.result)
        print("JSON: \(json)")

        guard let dictionary = json as? [String: Any] else {
            return
        }

        guard let status = dictionary["status"] as? String, let message = dictionary["message"] as? String else {
            return
        }

        guard status == "ok" else {
            if message == "invalid_login" {
                let alertMessage = UIAlertController(title: "Unable to login", message: "Please try again", preferredStyle: .alert)
                alertMessage.addAction(UIAlertAction(title: "Ok", style: .default, handler: nil))
                self.present(alertMessage, animated: true, completion: nil)
            }
            return
        }

        self.performSegue(withIdentifier: "Home", sender: nil)
    }
}


Filtering visible map layers based on multiple conditions I am working on a webapp using OpenLayers. I have written a filter function that I can use to tell whether a given layer is visible.

Since there are a bunch of conditions for a layer to be visible, I condensed them all into a single switch (true) block, and I was wondering if it was a good thing, since:

switch (boolean) has obviously only two branches
I think Java forbids switch (true) so it must be really bad somehow
A case "falling through" is most of the time an error (forgot to break), so as a rule of thumb we forbid such case statements
The language structure whose purpose is conditions is the if statement, which can be made shorter (less bytes of code) than the switch equivalent

But here I wrote a switch (true) version and I think this pattern is powerful since it's really easy to add/remove conditions, keep track of them, describe them, etc. After playing around with this method, I also wrote a version which uses an array of expressions instead of the switch.

So now I have three versions of the method. Which one do you think is the best and why?

/*global map */
// Sample usage
var visibleLayers = map.getLayers().getArray().filter(isVisible);

/*global map */
function isVisible(layer) {
    // Layer is not visible if it is hidden, has no data, or is transparent
    if (!layer || !layer.getVisible() || !layer.getSource() ||
            !layer.getOpacity()) {
        return false;
    }
    // Resolution limits
    var minRes = layer.getMinResolution() || 0;
    var maxRes = layer.getMaxResolution() || Infinity;
    var curRes = map.getView().getResolution();

    // Layer is not visible if the current resolution is too low or high
    if (curRes < minRes || maxRes < curRes) {
        return false;
    }
    // Layer is visible in our app only if its type matches
    if (['WMS', 'Vector'].indexOf(layer.getType()) !== -1) {
        return true;
    }
    return false;
}

/*global map */
function isVisible(layer) {
    if (!layer) {
        return false;
    }
    // Resolution limits
    var minRes = layer.getMinResolution() || 0;
    var maxRes = layer.getMaxResolution() || Infinity;
    var curRes = map.getView().getResolution();

    switch (true) {
    // Layer is hidden when any of these is true:
    case !layer.getVisible():  // - is hidden
    case !layer.getSource():   // - has no data
    case !layer.getOpacity():  // - is completely transparent
    case curRes < minRes:      // - too low resolution
    case maxRes < curRes:      // - too high resolution
        return false;
    // Layer is hidden *in our app* if none of these is true:
    case layer.getType() === 'WMS':
    case layer.getType() === 'Vector':
        return true;
    }
    return false;
}

/*global map */
function isVisible(layer) {
    if (!layer) {
        return false;
    }
    // Resolution limits
    var minRes = layer.getMinResolution() || 0;
    var maxRes = layer.getMaxResolution() || Infinity;
    var curRes = map.getView().getResolution();

    // Layer is hidden when any of these is true:
    var hideOn = [
        !layer.getVisible(),  // - is hidden
        !layer.getSource(),   // - has no data
        !layer.getOpacity(),  // - is completely transparent
        curRes < minRes,      // - too low resolution
        maxRes < curRes,      // - too high resolution
    ];

    // Layer type must match
    var typeMatch = ['WMS', 'Vector'].indexOf(layer.getType()) !== -1;

    return typeMatch && !hideOn.some(Boolean);
}

(PS: layer.getType() is not an OpenLayers method.)

Okay, so here is how I decided (yes we can say a lot of things about 20 little lines):

Performance

The first code (with a few if) is probably
the best, since the || operator stops whenever a condition is
evaluated to true.
The second one (with the big switch) is a very lil bit slower,
since we compute minRes, maxRes and curRes before evaluating
the first exit conditions where they are unused.
Otherwise, the switch statement stops whenever a truthy case is found.
The third one (with the Array) is the most wrong approach if I want
speed. We compute the variables, store the N conditions in an array
of size N, so they're all evaluated, and then we loop through said array
(.some() method) to tell whether it contains a truthy value.

However, regarding this specific app I'm working on, code legibility
and future features are more important than a few microseconds.

Legibility

My problem here is that legibility is kinda subjective.
I follow specific coding style and guidelines, and it's easier for me
to read code which follows these guidelines that code which doesn't.

Sadly, on this project, there are no defined conventions and there's an
interesting diversity of whitespace habits.

The first code is the most legible for most people coming from C,
but anyone who would come after me would be very tempted to write nested
if, since there are loads of nested code in the repo.
The second one is still quite legible for C pals, and honestly
the individual comments on each condition are nice. Also I believe it is
less tempting to nest conditions, since we would have to change the
structure first.
The last one is the most "eye-candy": it has the advantages of the
switch, while being less heavy (case, break). However, it is
trickier: "What on Earth is this !hideOn.some(Boolean) thing?
Damn, I'll have to check the docs."

Actually, reading the doc is nice I think. Learning about .some()
does not take long, and it's better if maintainers make sure they
know some of the language before, well, maintaining.

Extensibility

Extensibility is also important to me. I know this code will not be
reviewed now, but in a few months someone will probably ask:
"what if we also check this and that?".

In fact, what's likely to happen is "what if we make these conditions
conditional?", ie, add a parameter somewhere, and if this parameter has
some value, then we'll do some more things. This is an interesting way to
check extensibility. Now I'll insert some code since there's been too much
text.

// First code with ifs; nice but we end up nesting conditions
if (configuration === 'wow') {
    doSomething();
    // Comment both conditions
    if (newCond1 || newCond2) {
        return false;
    }
} // Continue

// Second code with switch; I had higher expectations for you
switch () {
case configuration === 'wow':
    doSomething();
    // Comment both conditions
    if (newCond1 || newCond2) {
        return false;
    }
     // Non-breaking case!!
case ...
}

// Other possibility; we may end up with very long lines,
// or multiline conditions (I limit my lines at 80 chars)
switch () {
case configuration === 'wow':
    doSomething();
    // Non-breaking case!!
case configuration === 'wow' && newCond1:  // Comment condition 1
case configuration === 'wow' && newCond2:  // Comment condition 2
case ...
}

// Last code with Array; free candy
if (configuration === 'wow') {
    doSomething();
    hideOn.concat([
        newCond1,  // Comment condition 1
        newCond2   // Comment condition 2
    ]);
}

In this particular situation, I think the last one is the most elegant.
Given it's also the most legible in my opinion, and that I'm clearly biased,
and that I want maintainers to read at least ES5 docs once in a while, my choice has been to use the third versions, with the array of booleans.


Optimizing location updates when adding a new object I'm trying to keep a bunch of Objects called Machine in their proper order. I was wondering if it'd be possible to further optimize the function HandleLocationCount for performance.
Any general feedback is of course also welcome. The function HandleLocationCount is only executed when a new Machine object is added.
The Machine object is below, it references a Section object, which acts as a container for the various machines.
public class Machine
{
    [Key]
    public int ID { get; set; }

    public string OrderNo { get; set; }

    public Section Section { get; set; }

    public int LocationInSection { get; set; }

    public ForcedLocation ForcedLocation { get; set; } = ForcedLocation.Blank;
}

Section object & enum for completeness; it's an enum instead of a boolean for future proofing:
public class Section
{
    [Key]
    public int ID { get; set; }

    public string Name { get; set; }

    public string OrderNo { get; set; }

    public IEnumerable<Machine> MachinesInSection { get; set; }
}

public enum ForcedLocation
{
    /// <summary>
    /// Does nothing for the location.
    /// </summary>
    Blank,

    /// <summary>
    /// This entity is always last in the location.
    /// </summary>
    Keep_Last,
}

The function that calls the HandleLocationCount function looks like this; a user is able to manually set the location if needed.
    public Machine Add(Machine entity)
    {
        if (entity.LocationInSection == 0)
        {
            IEnumerable<Machine> AmountMachines = GetMachinesByOrderNoAndSection(entity.OrderNo, entity.Section);
            entity.LocationInSection = handleLocationCount(AmountMachines);
        }

        _machineRepository.Add(entity);

        return entity;
    }

The function that returns the index just searches every machine for their order number & name, the repository itself is just an interface that has a generic index function
public IEnumerable<Machine> GetMachinesByOrderNoAndSection(string orderno, Section section)
    {
        return _machineRepository.Index()
         .Where(m => m.OrderNo == orderno && m.Section == section);
    }

And the code for the location looks like this, it updates older machines if this is needed; else just passes the new location for the machine
    private int HandleLocationCount(IEnumerable<Machine> AmountOfMachines)
    {
        int newLocation = AmountOfMachines.Count() + 1;

        foreach (var Machine in AmountOfMachines)
        {
            int oldLocation = Machine.LocationInSection;
            switch (Machine.ForcedLocation)
            {
                case ForcedLocation.Blank:
                    break;
                case ForcedLocation.Keep_Last:
                    Machine.LocationInSection = AmountOfMachines.Count() + 1;
                    break;
                default:
                    break;
            }

            if (Machine.LocationInSection != oldLocation)
            {
                newLocation = oldLocation;
                Update(Machine);
            }
        }

        return newLocation;
    }

The method GetMachinesByOrderNoAndSection returns an IEnumerable<Machine>. Depending on how the method is implemented, iterating the enumeration may query the database. In HandleLocationCount you are iterating it and also calling .Count() on it several times. The LINQ Count extension method might also iterate the enumeration.
Make sure to iterate it only once, e.g. by working on a list crated with var list = AmountOfMachines.ToList();, if something like this is not already done in GetMachinesByOrderNoAndSection.
Also, you could use two distinct variables for storing newLocation and AmountOfMachines.Count() + 1 and call the latter only once.

I am not sure to understand the logic right. But you seem to assign the same AmountOfMachines.Count() + 1 to all the Keep_Last machines. If this is the case, you could simply assign them a constant number like Int32.MaxValue. They would be kept last once and for all.
Another possible approach is to partition the number range into two ranges:

[1 .. Int32.MaxValue / 2] for Blank.
[Int32.MaxValue / 2 + 1 .. Int32.MaxValue] for Keep_Last.

Adavantages:

The machines stay in their respective group.
They are ordered.
Adding a machine at the end of a group does not require the LocationInSection of others to be updated.

Instead of getting the count, you would get the maximium value in the two groups.
int nextBlank = machineList
    .Where(m => m.ForcedLocation == ForcedLocation.Blank)
    .DefaultIfEmpty(0)
    .Max() + 1;
int nextKeepLast = machineList
    .Where(m => m.ForcedLocation == ForcedLocation.Keep_Last)
    .DefaultIfEmpty(Int32.MaxValue / 2)
    .Max() + 1;


Adding a bookmark, or changing an existing one I have a very problematic nested if/else/for that I'm sure it could use some love. Here's how it looks originally:

function addBookmark(obj) {
    var isFound = false;
    var posFound;
    if (bookmarks.length > 0) {
        for (var j = 0; j < bookmarks.length; j++) {
            if (obj.mirror == bookmarks[j].mirror &&
                obj.url == bookmarks[j].url &&
                obj.chapUrl == bookmarks[j].chapUrl &&
                obj.type == bookmarks[j].type) {
                if (obj.type == "chapter") {
                    isFound = true;
                    posFound = j;
                    break;
                } else {
                    if (obj.scanUrl == bookmarks[j].scanUrl) {
                        isFound = true;
                        posFound = j;
                        break;
                    }
                }
            }
        }
    }
    if (!isFound) {
        bookmarks[bookmarks.length] = {
            mirror : obj.mirror,
            url : obj.url,
            chapUrl : obj.chapUrl,
            type : obj.type,
            name : obj.name,
            chapName : obj.chapName,
            scanUrl : obj.scanUrl,
            scanName : obj.scanName,
            note : obj.note
        };
    } else {
        bookmarks[posFound].note = obj.note;
    }
    localStorage["bookmarks"] = JSON.stringify(bookmarks);
}

Is supposed to retrieve an object with bookmark information to be added/changed, loop through the array of bookmarks and check if the one we want to add exist/match, if exist then just add/change the note, if it doesn't then append.

I tried to remove chunks of it, trying to remove unnecessary nesting and this is what I achieved so far:

function addBookmark(obj) {
    var isFound = false;
    var posFound;
    if (bookmarks.length > 0) {
        for (var j = 0; j < bookmarks.length; j++) {
            if (obj.mirror == bookmarks[j].mirror &&
                obj.url == bookmarks[j].url &&
                obj.chapUrl == bookmarks[j].chapUrl &&
                obj.type == bookmarks[j].type) {
                if (obj.type == "chapter" ||
                    obj.scanUrl == bookmarks[j].scanUrl) {
                    isFound = true;
                    posFound = j;
                    break;
                }
            }
        }
    }
// more runs here, specifically the posFound/isFound variables
}

I'm thinking on replacing the break with a return statement instead and abstract the whole ifelse. Should I? Or there's a way to simplify this further?

It would be more natural to call isFound simply found.

If I understand correctly,
after the loop, there is more code in your application.
In which case, the function does too much.
There should be at least 3 functions:

A function with the loop, returning the position found, or else -1, as is pretty common with functions that search for something in a container
A function that takes a position found, and does something with it (the code you didn't post)
A function that calls the previous two

This way, you could get rid of the found and posFound state variables.
And in any case, no need to check bookmarks.length,
the iterating logic automatically takes care of the empty case.

The function then becomes:

function addBookmark(obj) {
    for (var j = 0; j < bookmarks.length; j++) {
        if (obj.mirror == bookmarks[j].mirror &&
            obj.url == bookmarks[j].url &&
            obj.chapUrl == bookmarks[j].chapUrl &&
            obj.type == bookmarks[j].type) {
            if (obj.type == "chapter" ||
                obj.scanUrl == bookmarks[j].scanUrl) {
                return j;
            }
        }
    }
    return -1;
}

For readability, I would also extract the complex conditions to a helper function with a descriptive name.
The name of the function would describe the meaning of those conditions.


Counting the amount of objects in an array that contain a certain key that doesn't have an empty value Here is my source code:

// start at zero
var count = 0;
// loop through the items
for(var i = 0; i < hours.length; i++) {
  // 1. check to see if the current item has a key called "description"
  // 2. check to see if the current item's "description" key isn't empty
  // NOTE: white-space only values aren't valid
  if(hours[i].hasOwnProperty("description") && hours[i].description.replace(/\s/g, "").length > 0) {
    // increment the count (by one)
    count += 1;
  }
}
// return the count
return count;

When we remove the comments the code looks like this:

var count = 0;
for(var i = 0; i < hours.length; i++) {
  if(hours[i].hasOwnProperty("description") && hours[i].description.replace(/\s/g, "").length > 0) {
    count += 1;
  }
}
return count;

This appears to be rather expensive (especially with larger arrays). Is there a better (preferably a more concise) way to count the amount of objects in an array that contain a certain key that doesn't have an empty value?

INFO: I can't use jQuery or LoDash/Underscore, all methods must be natively available in the browser and have good browser support (IE8+).

Using String.replace means that you need to step over every character in every string you are testing. Not only that but it needs to allocate memory to hold the resulting new string.

Its much quicker if you test for a non white space characters. That way it need only test to the first non white space, and it needs no additional memory.

Also Object.hasOwnProperty is slow as it must work its way up the prototype chain to get an answer and should only be used if you think that the object you are testing has a prototype chain containing the property name you are testing.

Last point. This is not part of the code you have presented, but to improve the performance the property hour.description should not contain only white spaces. When you set that property vet it eg  hour.description = descriptionString.trim();

Thus If you have unvetted description strings

var i, count = 0;
for (i = 0; i < hours.length; i++) {
    count += hours[i].description && (/\S/).test(hours[i].description) ? 1 : 0;
}
return count;

Or

If the description has been vetted then the following will be the quickest

var i, count = 0;
for (i = 0; i < hours.length; i++) {
    count += hours[i].description ? 1 : 0;
}
return count;


Elemental selection sort functions in R A simple selection sort functions in R. Just for practice.

Given the choice of algorithm and tools I wonder what can most easily be improved about the functions.

NB. I wanted to keep the first function as elemental as possible.

# Selection sort playing with loops
sels <- function(x) {
  n <- length(x)
  for (i in 1:(n - 1)) {
    min <- i # this variable keeps track of smallest number in unsorted
             # just some prior, update if confronted with better info
    for (j in (i + 1):n) {
      if (x[j] < x[min]) {
        min <- j # update as we find something smaller
      }
    }
  if (min != i) { # now if we did update prior; swap
      temp   <- x[i]
      x[i]   <- x[min]
      x[min] <- temp
    }
  }
  x
}

# Selection sort in more R'ish-style
selsr <- function(x) {
  # Selection sort a vector
  n <- length(x)
  for (i in 1:(n - 1)) {
    j <- i + which.min(x[(i + 1):n])
    if (j != i) {
      temp <- x[i]
      x[i] <- x[j]
      x[j] <- temp
    }
  }
  x
}

Here are some ideas, somewhat sorted by order of importance. I hope it helps.

1) There is a problem with your second function, see that

> selsr(1:5)
[1] 2 3 4 5 1

You should be doing

j <- i - 1L + which.min(x[i:n])

2) Your choice of 1:(n - 1) will not properly handle the n == 0. Also, it does handle the n == 1 case properly but it is a bit of luck considering you are looping over 1:0... The robust alternative to the : operator is to use seq_len or seq_along. Or you could just have a if (n < 2L) return(x) near the top of your functions.

3) While on the topic of corner cases, your code assumes that x is a numerical vector. You could be checking for that by adding:

stopifnot(is.vector(x), is.numeric(x))

4) You can achieve the swaping without a temporary variable by doing:

x[c(i, j)] <- x[c(j, i)]

5) min is a questionable variable name, given it is also the name of a base function.

6) This is debatable: I would get rid of the if (j != i) condition since it won't hurt doing the swap even if j == i. IMO, the simpler code is preferable to the marginal computation time gain.

7) A nit: know the difference between 1 (numeric) and 1L (integer). Here you are using 1 when you mean to use 1L, which causes unnecessary conversions from integer to numeric (though the : operator brings you back to integers by doing the opposite conversion).


Player projectile shooting I am wondering if my code is needlessly redundant. I noticed, when looking over my code, that I had a unnecessary variable and if statement. Both Fire() methods achieve the desired result without error (from what I have seen). To me, the redundant Fire() method reads a lot nicer and is my preferable method but I'm just wondering if the alternate Fire() method is still worth the trade-off in readability due to efficiency. I am also wondering if there may be any underlying bugs that may occur in either of my methods that I am unaware of.

Unity version: 4.6.3

Fire()` method result: when the space key is pressed, it spawns a laser that fires upwards towards the enemy at the preset speed and is delayed by the reload time.

Desired result: when the space key is pressed, spawns a laser and has it fire in an upwards direction towards the enemies at the preset speed and than have a delay equal to the preset reload time.

Variables:

public float reloadTime = 0.2f;
public GameObject playerProjectilePrefab;
public float playerProjectileSpeed = 10f;

private bool canShoot = true;
private float reloadCountDown;

Redundant Fire Method:

void Fire (){
    Vector3 startPosition = transform.position + new Vector3(0 ,(renderer.bounds.size.y * 0.5f), 0);

    if (canShoot && Input.GetKey(KeyCode.Space)){
        canShoot = false;
        GameObject playerProjectile = Instantiate(playerProjectilePrefab, startPosition, Quaternion.identity) as GameObject;
        playerProjectile.rigidbody2D.velocity = new Vector3 (0, playerProjectileSpeed, 0);
        reloadCountDown = reloadTime;
    } else if (!canShoot){
        reloadCountDown -= Time.deltaTime;
    }
    if (reloadCountDown <= 0f){
        canShoot = true;
    }
}

Alternate Fire Method:

void Fire (){
    Vector3 startPosition = transform.position + new Vector3(0 ,(renderer.bounds.size.y * 0.5f), 0);

    if (reloadCountDown <= 0f && Input.GetKey(KeyCode.Space)){
        GameObject playerProjectile = Instantiate(playerProjectilePrefab, startPosition, Quaternion.identity) as GameObject;
        playerProjectile.rigidbody2D.velocity = new Vector3 (0, playerProjectileSpeed, 0);
        reloadCountDown = reloadTime;
    } else if (reloadCountDown > 0f){
        reloadCountDown -= Time.deltaTime;
    }
}

I would use the Alternate Fire Method but I would convert the canShoot field to a CanShoot property.  Also startPosition is only used within the then block, so I would move its definition there as well.  Example:

public bool CanShoot => reloadCountDown <= 0F;

private void Fire ()
{
    if (!Input.GetKey(KeyCode.Space))
    {
        return;
    }

    if (CanShoot)
    {
        Vector3 startPosition = transform.position + new Vector3(0 ,(renderer.bounds.size.y * 0.5f), 0);
        GameObject playerProjectile = Instantiate(playerProjectilePrefab, startPosition, Quaternion.identity) as GameObject;
        playerProjectile.rigidbody2D.velocity = new Vector3 (0, playerProjectileSpeed, 0);
        reloadCountDown = reloadTime;
    }
    else
    {
        reloadCountDown -= Time.deltaTime;
    }
}

Your code looks more Java-ish than C#.  Since you've tagged it with C#, I will focus my comments on that.  However, I am not familiar with Unity3d and don't know how it will like some syntax changes.

Braces on a new line

With C#, we prefer braces on their own line.  The one exception would be a one-liner such as:

if (!Input.GetKey(KeyCode.Space)) { return; }

Fields should never be public

Your first 3 class-level variables are called fields.  They should either be properties or constants.  For example, will reloadTime always be 0.2f?  If so, it can be a constant.  If not, and if you want it to be exposed publicly, then you should make it a property.  If reloadTime does not need to be public, then it can remain a field.

Method, Property, & Constant Names are CamelCase

Fields & Parameter Names are PascalCase

This doesn't YET apply to your original code, but let's combine it with the previous section.  Your declarations could now look like:

public const float ReloadTime = 0.2F;
public GameObject PlayerProjectilePrefab { get; private set; }
public const float PlayerProjectileSpeed = 10F;

private bool CanShoot => reloadCountDown <= 0.F;
private float reloadCountDown;

But I am just guessing as to the private setters.  Use what your app requires.

Numeric Type Designators should be Uppercase

Change the f to F.  Really doesn't affect your code here but the reason for this guideline is you ever use long where a lowercase l could be confused as a numeric 1, but a capital L would not.

Always declare accessibility modifier

Instead of simply void Fire() use private void Fire().  When I see a private missing, I assume a newbie coder has forgotten it, which leads me to wonder what else he or she has forgotten.

Variable declaration and scoping

As mentioned at the very top with startPosition, you should limit the scope of a variable to where you need it.  In my example, startPosition was placed inside the then block and declared immediately before the one place where it was referenced.  Someone reading your code doesn't have to remember where it was defined, or worry about it afterwards.