Brace pairing ({}[]()) cleanup/speedup This is working as expected, except in the speed area + I need to make it more readable and shorter if possible, I probably have lot's of things I don't need :).

Edit it is working now

charset = dict(opening='{[(<',\
    closing='}])>',\
    string = ('"', "'"),\
    comment=(('<!--', '-->'), ('"""', '"""'), ('#', '\n')))

allowed = ''.join([x[0][0] + x[1][0] for x in charset['comment']])
allowed += ''.join(charset['string'])
allowed += charset['opening']
allowed += charset['closing']

def brace_check(text):
    o = []
    c = []
    notr = []
    found = []
    busy = False
    last_pos = None
    for i in xrange(len(text)):
        ch = text[i]
        if not busy:
            cont = True
            for comment in charset['comment']:
                if ch == comment[0][0]:
                    como = text[i:len(comment[0])]
                    if como == comment[0]:
                        busy = comment[1]
                        if ch in charset['opening']:
                            last_pos = i
                        cont = False
                        break
            if cont:
                if ch in charset['string']:
                    busy = ch
                elif ch in charset['opening']:
                    o.append((ch, i))
                elif  ch in charset['closing']:
                    c.append((ch, i))
        else:
            if ch == busy[0]:
                if len(busy) == 1:
                    comc = ch
                else:
                    comc = text[i:i + len(busy)]
                if comc == busy:
                    if last_pos is not None:
                        if busy[-1] in charset['closing']:
                            found.append((last_pos, i))
                        last_pos = None
                        text = text[:i] + '\n' * len(comc) +\
                            text[i + len(comc):]
                    busy = not busy
            elif busy in charset['string']:
                if ch == '\n':
                    busy = not busy
    for t, e in reversed(o):
        try:
            n = next((b, v) for b, v in c\
                if b == charset['closing'][\
                    charset['opening'].find(t)] and v > e)
            c.remove(n)
            n = n[1]
            if found != []:
                if e < found[-1][0] and n > found[-1][0] and n < found[-1][1]\
                or e < found[-1][1] and n > found[-1][1] and e > found[-1][0]:
                    found.append((n, False))
                    n = False
        except StopIteration:
            n = False
        found.append((e, n))
    for t, e in c:
        found.append((e, False))
    return found

def brace_check(string):

string is the name of a python module, it might be best to avoid it

    # Imports
    import re

imports should be done at the module level not inside a function

    # Configuration
    charset = dict(opening='{[(<',\
        closing='}])>',\
        string = ['"', "'"],\
        single=['#', '//'],\
        multi=[['/*', '*/'], '"""', ['<!--', '-->']])

Constant values should be at the module level not inside the function

    found = []# found objects

found isn't used until way later. Declare it way later.

    notr = []# busy locations

notr and its comment don't suggest anything the same.

    temp = {}# temporary holding for opening braces
    end = {}# temporary holding for closing braces

All variables are temporary. Don't give names or comments that tell me such obvious things

    # Find all ocurrances and its position
    newline = [m.start() for m in re.finditer('\n', string)] # every newline

    for mn in charset['multi']:# find every multiline comment
        if type(mn) == type(str()):# if there only is one element
            op = [m.start() for m in re.finditer(re.escape(mn), string)]
            cl = op[1::2]
            op = op[0::2]
            mn = (mn, mn)

        else:
            op = [m.start() for m in re.finditer(re.escape(mn[0]), string)]
            cl = [m.start() for m in re.finditer(re.escape(mn[1]), string)]

These short cryptic variables name are asking for confusion.

        for i in xrange(len(op)):# find and close every comment
            om = op[i]

Use the enumerate function. It lets you use a foreach loop and get the indexes at the same time
                try:
                    cm = next(v for v in cl if v > om) + len(mn[1])

add a line like: start_comment, end_comment = mn, that way you can avoid the indexes are your code will be clearer.

                if mn[0][0] in charset['opening'] and\
                        mn[1][-1] in charset['closing']:
                    for i in xrange(om + 1, cm - 1):
                        if i not in notr:
                            notr.append(i)

Rather the constantly checking whether a value is in notr, use a set. Then the last three lines can simply be notr.add( xrange(om + 1, cm - 1) )

                else:
                    for i in xrange(om, cm):
                        if i not in notr:
                            notr.append(i)

Did we really need that special case?

            except:

Don't catch all exceptions, you'll hide bugs that way. Instead, catch only the specific type of exception you are interested in. Also, you want as little code in the try block as possible to avoid catching other stray exceptions

                for i in xrange(om, len(string)):
                    if i not in notr:
                        notr.append(i)

You at least need a comment explaining why this make sense.

        del op, cl

Don't del variables, its rarely useful.

    for sn in charset['single']:
        rep = [m.start() for m in re.finditer(re.escape(sn), string)]
        for s in rep:
            try:
                new = next(v for v in newline if v > s)
            except:
                new = len(string)
                newline.append(len(string))

Why not use the string search functions rather then having built a newline list? Also, if you need to treat the end of input as a newline, put that in when you create the list not in an exception handler.

            if not s in notr:
                for i in xrange(s, new):
                    if i not in notr:
                        notr.append(i)
                newline.remove(new)
        del rep

    for sn in charset['string']:
        rep = [m.start() for m in re.finditer(re.escape(sn), string)]
        last = False
        for i in xrange(len(rep)):
            if not last:
                s = rep[i]
                if s not in notr:
                    try:
                        new = next(v for v in newline if v > s)
                    except:
                        new = len(string)
                        newline.append(len(string))

If code repeats, you need a function.

                    try:
                        n = next(v for v in rep if v > s)
                    except:
                        n = new + 1
                    if n < new:
                        last = True
                        for i in xrange(s, n + 1):
                            if i not in notr:
                                notr.append(i)
                    else:
                        for i in xrange(s, new):
                            if i not in notr:
                                notr.append(i)
                        newline.remove(new)
            else:
                last = False
        del rep

    for cn in charset['closing']:
        on = charset['opening'][charset['closing'].find(cn)]
        end[on] = [m.start() for m in re.finditer(re.escape(cn), string)\
            if m.start() not in notr]

Given the cn is a single character, use of a regular expression to find it is overkill.

    for on in charset['opening']:
        op = [m.start() for m in re.finditer(re.escape(on), string)\
            if m.start() not in notr]
        for i in xrange(len(op)):
            temp[op[i]] = on

    for k in sorted(temp.keys(), reverse=True):
        try:
            c = next(v for v in end[temp[k]] if v > k and v not in notr)
            end[temp[k]].remove(c)
            for i in xrange(k, c + 1):
                if i not in notr:
                        notr.append(i)
        except:
            c = False
        found.append((k, c))

It seems to me that using a stack would be simpler then what you are doing here.

    for v in end.values():
        for c in v:
            found.append((c, False))

No explanation is given for what you are putting inside found. I have no idea what the return value means.
        return found

General thoughts:

The code is complicated, it really should be split up into a lot of functions.

Rather then all that code matching different types of comments and strings, have a set of ignore regexes. The regex should match an entire comment or string which can then be removed.

Your code builds lists of everything which would only serve to complicate what is going on. Lists can be great, but they aren't the solution to every problem.

My rewrite of your code: (I didn't start with your code, but it should support the same features:)

from nose.tools import assert_equal
import re

class SimpleTokenizer(object):
    """
    The constructor takes a dictionary of keys which should be regular
    expression strings to arbitrary value. The tokenize method will produce
    all the substrings in the text which match the regular expression.
    It will return the value provided in the original dictionary
    to identify which expression was matched
    """
    def __init__(self, data):
        # I need a consistent order from data, so I dump the dictionary
        # into a list
        data = list(data.items())
        self.values = [value for key, value in data]
        # this regular expression matches any key
        self.expression = '|'.join('(%s)' % key for key, value in data)

    def tokenize(self, text):
        for match in re.finditer(self.expression, text):
            # group 0 is the whole string, we subtract one
            # to make up for that
            yield match.start(), self.values[match.lastindex - 1]

CLOSER = 0
OPENER = 1
IGNORE = 2
class Language(object):
    """
    Provides brace checking
    """
    def __init__(self, opening, closing, ignore = ()):
        """
        opening and closing should be sequences of the same size denoting
        the start and end symbols that should matched. Ignore should be a
        sequence of regular expression which should be ignored for the
        purpose of matching such as comments, strings, etc.
        """

        data = {}
        for opener, closer in zip(opening, closing):
            data[re.escape(opener)] = (OPENER, closer)
            data[re.escape(closer)] = (CLOSER, closer)
        for ignore in ignore:
            data[ignore] = (IGNORE, None)
        self._tokenizer = SimpleTokenizer(data)

    def check(self, text):
        """
        Given text produces the location of the various braces
        returns a list of tuple, start, end where a pair of matches
        braces exist. If either brace is missing it will be recorded
        as None.
        """

        braces = []

        waiting = []
        # waiting is a stack holding the braces which have been opened
        # but not yet closed
        for idx, (category, extra) in self._tokenizer.tokenize(text):
            if category == OPENER:
                waiting.append( (extra, len(braces)) )
                # we start with None as the end of a brace, it'll be
                # updated if we find it
                braces.append( (idx, None) )
            elif category == CLOSER:
                # find the last entry that matches
                index = None
                for element, element_index in reversed(waiting):
                    if element == extra:
                        index = element_index
                        break

                if index is None:
                    # no matches found
                    # in that case, record the start as None
                    braces.append( (None, idx) )
                else:
                    # we found the match, replace the existing record
                    # with a new one
                    position, waste = braces[index]
                    braces[index] = (position, idx)
                    # elminate anything we had to ignore
                    del waiting[index:]

        return braces

LANGUAGE_TESTS = [
    ("[()]", [(0, 3), (1, 2)]),
    ("[(a)]", [(0, 4), (1, 3)]),
    ("[(]", [(0, 2), (1, None)]),
    ("[()", [(0, None), (1, 2)]),
    ("[(])", [(0, 2), (1, None), (None,3)]),
    ("])", [(None, 0), (None, 1)]),
    ("(])", [(0, 2), (None, 1)]),
]

def test_basic_language():
    language = Language(
        opening = '[(',
        closing = '])')
    def inner(text, expected):
        result = language.check(text)
        assert_equal(list(result), expected)

    for text, expected in LANGUAGE_TESTS:
        yield inner, text, expected

STRING_LANGUAGE = [
    ('[(")]")]', [(0, 7), (1, 6)])
]
def test_string_language():
    language = Language(
        opening = '[(',
        closing = '])',
        ignore = [r'"[^"]*"']
    )
    def inner(text, expected):
        result = language.check(text)
        assert_equal(list(result), expected)

    for text, expected in STRING_LANGUAGE:
        yield inner, text, expected


Synchronizer for importing XML files into a database when folder content changes I have created a Synchronizer, the purpose of which is to read data from an XML source file and store the result in a DB.

I have different source types, for example Student.XML, School.XML, etc. These files are copied in my source directory, C:\Source.

My Synchronizer watches the Source folder, and every time there is a new file, it should read it, map it to my domain class and write the domain class into the DB.

If C:\Source folder contains Student.XML, it should read the content of the XML file and copy it in Student Table in the DB. If C:\Source contains School.XML, it should read the file and copy the content to School Table.

To solve this, I have defined an ISyncer interface:

public interface ISyncer
{
    // Read input and synchronize destination, return error message if any
    string Sync();
}

Now all my Syncer types (e.g. StudentSyncer, SchoolSyncer, etc) should implement this interface:

public class StudentSyncer : ISyncer
{
    // simple XML reader, reads XML file into a list of List<XMLStudent>
    private XMLStudentReader _reader;

    public StudentSyncer(string file)
    {
        _reader = new XMLStudentReader(file);
    }

    public string Sync()
    {
        // read the input
        List<XMLStudent> source = _reader.ReadAll();

        // using automapper map to domain object
        List<Student> dbStudent = Mapper.Map<List<XMLStudent>, List<Student>>(source);
        dbStudent.ForEach(s => s.IsCurrent = true);                // set IsCurrent

        // write records to Student table, using Unit of Work patterns
        using (var context = new DbContext())
        {
            UnitOfWork uow = new UnitOfWork(context);

            // set the existing records to not current
            var curSet = uow.Student.FindByTrackingChanges(s => s.IsCurrent == true).ToList();
            curSet.ForEach(s => s.IsCurrent = false);
            uow.Student.AddRange(dbStudent);
            uow.SaveChanges();
        }

        return "";  // no error
    }
}

I have a Windows service, which calls the DoSync() method every 30 seconds:

public void DoSync()
{
    foreach (string file in Directory.EnumerateFiles(@"C:\Source", "*.xml"))
    {
        // use syncer factory to initialize the correct instance of syncer based on input file name
        ISyncer syncer = _syncerFactory.CreateInstance(file);

        // do the synchronization task
        syncer.Sync();

        // Move file to processed folder
        MoveFile(@"C:\Destination");
    }
}

The Factory is a simple factory. It reads the source file name, e.g. Student.XML, School.XML and based on the file name. It initializes the correct Syncer, e.g. StudentSyncer, SchoolSyncer.

I am happy with the code, but it is bothering me because I feel it is not following the Single Responsibility Principle. My Syncer class is not doing 1 task, but it is doing 3 tasks: read input, map to domain class, write to DB.

I know this is a little late, but instead of the timed call to DoSync(), you could consider to use FileSystemWatcher and then subscribe to the relevant events.


Subclasses instantiating superclass of class Human I have a test which looks like this:

Create a base class Human
Human class needs to have a method Talk
The Human class needs to have to descendant class Man and Woman
Both Man and Woman should have their own Talk method (basically rewrite the method)
The Man class should have a private property _foo
It should have a method getInfo (which needs to be an Ajax call and log the response)
I need to make 1000 instances of the Women class in the window namespace (I mean global)
On document.body single click, a random Woman should call the Talk method
On document.body double click, the Man's getInfo method should be called

function Human(){};

Human.prototype.talk = function(){
    console.log('Make an Human sound');
}

function Woman(){
    Human.call(this);
}

Woman.prototype = Object.create(Human.prototype);
Woman.prototype.constructor = Woman;

Woman.prototype.talk = function(){
    console.log('Miau');
}

function Man(){
    var foo = 10;
    Human.call(this);
}

Man.prototype = Object.create(Human.prototype);
Man.prototype.constructor = Man;

Man.prototype.talk = function(){
    console.log('Wuff');
}

Man.prototype.getInfo = function(method, url){
    var xhr = new XMLHttpRequest();

    xhr.open(method, url);
    xhr.send(null);

    xhr.onreadystatechange = function() {
        console.log('Ajax response: ' + xhr.readyState);
    };
}

var woman = new Woman();

console.log('woman instance of Woman ' + (woman instanceof Woman));
console.log('woman instance of Human ' + (woman instanceof Human));

woman.talk();

var man = new Man();

console.log('man instance of Man ' + (man instanceof Man));
console.log('man instance of Human ' + (man instanceof Human));

man.talk();

womans = [];
for(var i = 0; i < 1000; i++) {

    womans[i] = new Woman();
}

document.body.onclick = function(){
    var randNr = Math.floor((Math.random()*1000)+1);
    womans[randNr].talk();
    console.log('Random Woman: ' + randNr);
 }
 document.body.ondblclick = function(){
          Man.prototype.getInfo();
    }

Some opinions:

function Woman(){
    Human.call(this);
}
Woman.prototype = new Human();

Don't use new for prototype (even when it doesn't matter here as long as Human is empty, but the Huma.call(this) suggests otherwise). Check https://stackoverflow.com/q/10898786/1048572

// Woman subClass
function Man(){

Apparently not.

    var _foo = 10;

It's a local-scoped ("private") variable anyway, you don't need to prefix it with an underscore. Notice that it is only accessible from functions declared within the constructor, of which you don't have any.

    return Human.call(this);

No reason to return anything here. It might even be wrong it Human did return an object.

Man.prototype.getInfo = function(method, url){
    […]
}

This doesn't seem to have anything to do with instances of Man, so I wonder why it is a method? But that's probably just the design of your test case.

console.log('man private variable _foo: ' + (man._foo));

Not sure what you did expect, but yes, since _foo has been a variable and not a property of the object so this is undefined.

document.body.onclick = function(){
    […]

…is missing a closing brace.

var randNr = Math.floor((Math.random()*1000)+1);
womans[randNr].talk();

No. That randNr will be in the range 1-1000 (both inclusive), while your array indices are 0-999. With a small chance, this will throw an exception. Remove the +1.

console.log('Random Woman: ' + randNr);

…and add it here if you want that output one-based.

Man.prototype.getInfo();

That confirms what I said above - you don't use an instance for that method. You should hardly ever call methods on the prototype object - man.getInfo() would be better. But if you really intended this to be a static function, you might write Man.getInfo = function() {…}; and then call Man.getInfo();.


Improving time complexity of cheapest travel route algorithm Below is the code to find the cheapest route for a passenger. stationsUnexplored is a Queue to which stations are added and examined. Once we are done with a station, it is removed from Queue and added to a Set stationsExplored. The purpose of stationsExplored is to avoid queuing of stations more than once. Before adding a station to stationsUnexplored I check if it has been explored earlier i.e  !stationsExplored.contains(adjacentStation.getKey(). Or it is already present in the queue i.e  !stationsUnexplored.contains(adjacentStation.getKey()). First contains() is on a Set, so it has time complexity of \$\mathcal{O}(1)\$. But second contains() is called on a Queue hence the time complexity is \$\mathcal{O}(N)\$.

Is there a way to get rid of the second check on queue or improve it to \$\mathcal{O}(1)\$?

 Queue<Integer> stationsUnexplored = new ArrayDeque<>();
 Set<Integer> stationsExplored = new HashSet<>();
 stationsUnexplored.add(1);

 while (!stationsUnexplored.isEmpty()) {
     int currentStation = stationsUnexplored.remove();
     Map<Integer, Integer> stations = stationsNFares.get(currentStation); // returns neighboring stations.
     if (stations != null) {
        Iterator<Map.Entry<Integer, Integer>> stationIterator = stations.entrySet().iterator();
        while (stationIterator.hasNext()) {
           Map.Entry<Integer, Integer> adjacentStation = stationIterator.next();

           // code for computing cheapest fare for current adjacent station.....

           if (!stationsExplored.contains(adjacentStation.getKey()) && !stationsUnexplored.contains(adjacentStation.getKey())) { //O(N)
              stationsUnexplored.add(adjacentStation.getKey());
           }
        }
     }
     stationsExplored.add(currentStation);
 }

I find it a little bit confusing that the title of your question doesn't seem to match its content. That is, you mention you're looking for the cheapest route, and your code sort of references fares, but your algorithm is a BFS that pays no attention whatsoever to the costs.

That said, the question you actually asked has a fairly simple answer. Change this bit of code:

if (!stationsExplored.contains(adjacentStation.getKey()) && !stationsUnexplored.contains(adjacentStation.getKey())) {
    stationsUnexplored.add(adjacentStation.getKey());
}

To this:

if ( !stationsExplored.contains(adjacentStation.getKey()) ) {
    stationsUnexplored.add(adjacentStation.getKey());
    stationsExplored.add(adjacentStation.getKey());
}

You'll also have to add a stationsExplored.add(1) around the top and you should remove the stationsExplored.add(currentStation) from the bottom. (That was wrong anyway I think. It ought to have been currentStation.getKey())

All you want is to add everything to the Queue exactly once, and this accomplishes that just as well as what you had before. It doesn't matter to your code if you mark the station as explored as soon as you add it to your Queue or if you wait until you pop it off, and by adding it early, you allow yourself to use only the faster Set contains method.


Parser for file made up of sections I'm parsing a file made up of various sections. I have a current_section variable that tracks which portion of the file I'm currently processing and the following check_for_next_section method that returns either the next section based on the contents of the current line or current_section if there are no matches based on the contents of the current line.
This method is getting flagged by rubocop for both 'perceived complexity' and 'cyclomatic complexity'. How can I reduce these?
SECTION_HEADERS = {
  hawaii_1: '"[2] For Hawaii, the following Postal Codes are Zone 44 for Ground',
  hawaii_2: '"For Hawaii, the following Postal Codes are Zone 46 for Ground',
  alaska_1: '"[3] For Alaska, the following Postal Codes are Zone 44 for Ground',
  alaska_2: '"For Alaska, the following Postal Codes are Zone 46 for Ground'
}.freeze

def check_for_next_section(line, current_section)
  return :zones if current_section == :preamble && line.starts_with?('Dest. ZIP')
  return :hawaii_1 if current_section == :zones && !line.blank? && line.starts_with?(SECTION_HEADERS[:hawaii_1])
  return :hawaii_2 if current_section == :hawaii_1 && line.starts_with?(SECTION_HEADERS[:hawaii_2])
  return :alaska_1 if current_section == :hawaii_2 && line.starts_with?(SECTION_HEADERS[:alaska_1])
  return :alaska_2 if current_section == :alaska_1 && line.starts_with?(SECTION_HEADERS[:alaska_2])

  current_section
end

If complexity is getting to high for a method it's usually a good idea to split out helper methods. If you end up to pass in the same parameters to each of these methods, it could also be a good idea to extract a class.
Some solution could be
def check_for_next_section(line, current_section)
  ZoneCheck.new(line, current_section).run
end

class ZoneCheck
  SECTION_HEADERS = {
    hawaii_1: '"[2] For Hawaii, the following Postal Codes are Zone 44 for Ground',
    hawaii_2: '"For Hawaii, the following Postal Codes are Zone 46 for Ground',
    alaska_1: '"[3] For Alaska, the following Postal Codes are Zone 44 for Ground',
    alaska_2: '"For Alaska, the following Postal Codes are Zone 46 for Ground'
  }.freeze

  def initialize(line, current_section)
    @line = line
    @current_section = current_section
  end

  def run
    if zones?
      :zones
    elsif hawaii_1?
      :hawaii_1
    elsif hawaii_2?
      :hawaii_2
    elsif alaska_1?
      :alaska_1
    elsif alaska_2?
      :alaska_2
    else
      current_section
    end
  end

  private

  attr_reader :line, :current_section

  def zones?
    current_section == :preamble &&
      line.starts_with?('Dest. ZIP')
  end

  def hawaii_1?
    current_section == :zones &&
      !line.blank? &&
      line.starts_with?(SECTION_HEADERS[:hawaii_1])
  end

  def hawaii_2?
    current_section == :hawaii_1 &&
      line.starts_with?(SECTION_HEADERS[:hawaii_2])
  end

  def alaska_1?
    current_section == :hawaii_2 &&
      line.starts_with?(SECTION_HEADERS[:alaska_1])
  end

  def alaska_2?
    current_section == :alaska_1 &&
      line.starts_with?(SECTION_HEADERS[:alaska_2])
  end
end


Static helper function to set a property on another object A situation has come up in which we have a block of code that will be repeated in multiple files, and this block of code has to:

Create some object based on the values of other controls/properties.
Set a property on another object to be the object from #1.

Here is an example of what this looks like:

Repeated code before refactoring (TypeScript)

public class PageViewModel {
  private _name: TextBox;
  private _type: TextBox;
  private _timestampFormat: RadioButton;

  ..

  // This should generate a data model out of the current state of its controls.
  public generateDataModel(): IDataModel {
    // Here will be some logic to generate a property value based on
    // the radio button value and some other conditions.
    let timestampProperty;
    if (this._radioControl.value() === 1) {
      timestampProperty = { format: "foo" };
    } else if(this._radioControl.value() === 2) {
      timestampProperty = { format: "bar" };
    }

    var dataModelObject = <IDataModel>{
      name: this._name.value(),
      type: this._type.value(),
      timestamp: timestampProperty
    };
    return dataModelObject;
  }

}

Problem

The generateDataModel function will be present on more than one view model, all of which will generate the timestamp field from the radio control with that logic, so code will be repeated.

How it was solved

A helper method was extracted to do both #1 and #2 from above. This method is being called with references to both the control and the IDataModel object being created.

Helpers.ts:

public static setTimestampRadioButton(radioControl: RadioButton, dataModelObject: IDataModel): void {
    let timestampProperty;
    if (radioControl.value() === 1) {
      timestampProperty = { format: "foo" };
    } else if(radioControl.value() === 2) {
      timestampProperty = { format: "bar" };
    }

    dataModelObject.timestamp = timestampProperty;
}

PageViewModel.ts:

public generateDataModel(): IDataModel {
  var dataModelObject = <IDataModel>{
    name: this._name.value(),
    type: this._type.value()
  };

  Helpers.setTimestampRadioButton(this._radioControl, dataModelObject);

  return dataModelObject;
}

I have some concerns with this approach from the point of view of readability, and also because I think Helpers.ts knows too much about IDataModel and how it is used.

My questions:

Is this a good way to solve the problem of repetition here? Why / why not?

If not, is there a better way you'd suggest?

What do you think of the pattern static setSomethingOnSomethingElse(thing1, thing2)?

I would argue that setTimestampRadioButton can be improved in three ways.

Side effects. By mutating the passed dataModelObject, the function immediately makes any function that uses setTimestampRadioButton harder to understand as it magically sets a property name that the calling function did not specify.
Typing. Mutating the argument the function forces you to create a Partial<IDataModel> (or cast an object which is not an IDataModel to IDataModel). This is not ideal.
Alternative property names. Currently only IDataModel uses the format from a RadioButton, what happens when ITimeModel needs the timestamp format stored in a format property? We are now back to the original problem.

I would instead recommend creating a method to get the expected format.

public static getTimestampFormat(radioControl: RadioButton): IFormat {
    if (radioControl.value() === 1) {
      return { format: "foo" };
    } else if(radioControl.value() === 2) {
      return { format: "bar" };
    }
    return { format:
}

This simplifies your generateDataModel method even more.

public generateDataModel(): IDataModel {
  return {
    name: this._name.value(),
    type: this._type.value(),
    timestamp: Helpers.getTimestampFormat(this._radioControl)
  };
}

Instead of a static helper method, you might also consider adding the method as an instance method of the RadioButton class, or creating a child of the RadioButton class with a TimestampRadioButton class.

A couple other general things:

Avoid casting like the plague. It is almost as bad as using the any type as it hides potential problems in a very similar way.
I can't think of any instance in which adding Object or data to a variable name makes it more descriptive. dataModelObject doesn't really tell you any more than model.


Counting subtrees where all nodes have the same value A unival tree (which stands for "universal value") is a tree where all
  nodes under it have the same value.

  Given the root to a binary tree, count the number of unival subtrees.

  For example, the following tree has 5 unival subtrees:

   0
  / \
 1   0
    / \
   1   0
  / \
 1   1

class DailyCodingProblem8 {

    public static void main(String args[]) {

        BinaryTree tree = new BinaryTree();
        tree.root = new Node(0);
        tree.root.left = new Node(1);
        tree.root.right = new Node(0);
        tree.root.right.left = new Node(1);
        tree.root.right.right = new Node(0);
        tree.root.right.left.left = new Node(1);
        tree.root.right.left.right = new Node(1);
        int res = tree.countUnivalTrees(tree.root);
        System.out.println(res);

        /*
              5
             / \
            4   5
           / \   \
          4   4   5

        */

        tree = new BinaryTree();
        tree.root = new Node(5);
        tree.root.left = new Node(4);
        tree.root.right = new Node(5);
        tree.root.left.left= new Node(4);
        tree.root.left.right= new Node(4);
        tree.root.right.right = new Node(5);
        res = tree.countUnivalTrees(tree.root);
        System.out.println(res);

    }

}

class Node {
    public int value;
    public Node left, right;

    Node(int value) {
        this.value = value;
        this.left = this.right = null;
    }
}

class BinaryTree {
    Node root;

    int countUnivalTrees(Node root) {
        if (root == null) {
            return 0;
        }
        int count = countUnivalTrees(root.left) + countUnivalTrees(root.right);
        if (root.left != null && root.value != root.left.value) {
            return count;
        }

        if (root.right != null && root.value != root.right.value) {
            return count;
        }

        // if whole tree is unival tree
        return count + 1;
    }

}

What is the best way to supply binary tree as input? Should I be creating a insert  method and insert nodes? Will the interviewer feel that I am deviating from the actual problem if I do so?

Allow Node take left and right, and BinaryTree take a node.

new BinaryTree(
    new Node(
        0,
        new Node(1),
        new Node(
            0,
            new Node(
                1,
                new Node(1)
                new Node(1)
            ),
            new Node(0),
        )
    )
)

Takes a fair amount of lines, but clearly shows the shape of the tree and removes the need for the messy tree.root.right.left.right usage.


Calculating when trains will meet, iteratively and using OOP I've done a little bit of programming in the past, mostly just dabbling.
After a long time of not touching an IDE, I am getting back into it, with Visual Studio. I just threw this quick project together to make sure I remember how to use classes and objects before I start playing with bigger projects.

I would love a quick critique to make sure there arent any glaring poor practices that could develop into bad habbits in the future.

I know the standard way to solve this problem is just time = distance/(velA+velB), but what would the point of using objects be if i didnt have the objects do something and change their status in someway?

What do you think, looks good?

/*

The goal for this practice program is to solve the common math word problem below using classes and objects.
I feel that using iterations rather than the basic math formula to solve, although clearly less efficient, would be more true to thinking in terms of objects.

Train A, traveling X miles per hour (mph), leaves Westford heading toward Eastford, 260 miles away.
At the same time Train B, traveling Y mph, leaves Eastford heading toward Westford.
When do the two trains meet? How far from each city do they meet?

*/

#include "pch.h"
#include <iostream>
using namespace std;

class Train
{
public:
    Train(int, int);
    int getLocation();
    void update();
private:
    int location, velocity;
};

Train::Train(int loc, int vel) //to initialize the train object and set its location and velocity
{
    location = loc;
    velocity = vel;
}

int Train::getLocation() // returns the location of the train object
{
    return location;
}

void Train::update() // updates the train object for one iteration
{
    location += velocity;
}

int main()
{
    int velA, velB, distance; //to take the values from the user input
    int time;   // to keep track of the number of iterations
                // time is declared here so it can be used outside of the for loop

    //input
    cout << "Enter velocity of the train from Westford:\n";
    cin >> velA;
    cout << "\n\nEnter velocity of the train from Eastford:\n";
    cin >> velB;
    cout << "\n\nEnter the distance between Westford and Eastford:\n";
    cin >> distance;

    //initialize each train
    Train trainA(0, velA);
    Train trainB(distance, 0 - velB);   //location of trainB is distance because the distance between an x coordinate at 0 and another x coordinate is equal to the second x coordinate
                                        //the velocity of trainB is the negative of velB because it is traveling in the opposite direction of trainA

    //run the sim
    for (time = 0; trainA.getLocation() < trainB.getLocation(); time++)
    {
        trainA.update();
        trainB.update();
    }

    //output
    cout << "\n\nThe Trains pass eachother after " << time << " hours."
        << "\nAt that time, the Westford train is " << trainA.getLocation() << " miles from Westford\n"
        << "and the Eastford train is " << distance - trainB.getLocation() << " miles from Eastford.\n\n";

    return 0;
}

Note that pch.h header is not standard, and probably should not be part of this review.
Don't do using namespace std;.
Use initializer lists. That is, the constructor should be written as Train::Train(int loc, int vel) : location(loc), velocity(vel) { }. For POD types this might not matter, but it is idiomatic.
Because int getLocation() does not modify the state of the object, it should be made const, i.e., int getLocation() const.
In your main program, don't strive to declare all variables as soon as possible as this is C++ and not C. Instead, introduce variables as late as possible and as close to their site of usage as possible. As a remark, you should also initialize e.g., time when you define it to improve readability.
Yes, you could use a while-loop, but the for-loop has the advantage that you have to make an explicit decision as to what happens at the end of the loop in order for the program to compile. Because you need the time variable outside of the loop, you could do say for(; trainA.getLocation() < trainB.getLocation(); ++time) { ... }. A while-loop is also perfectly fine, and this boils down mostly to personal preference.
Perhaps you should enforce the train speeds to be positive. Otherwise you can get stuck in an infinite loop as the trains will never meet.
Your comments are too verbose. Good code is self-documenting via variables names and logical structure. Prefer comments that answer "why" to "what". For example, a comment like "input", "initialize each train", and "run the sim" are unnecessary, and only hurt readability.


Printing the bits of an integer using bitfields and union I am given a short int. I need to print the bits of it into two bytes. My code is as follows:

#include <stdio.h>

union info{
    short z;
    struct data{
        unsigned a:1;
        unsigned b:1;
        unsigned c:1;
        unsigned d:1;
        unsigned e:1;
        unsigned f:1;
        unsigned g:1;
        unsigned h:1;
  };
}t;

union byte{
short n;
struct inside{
    char p:8;
    char q:8;
  };
}v;

int main(void)
{
 short x;

 scanf("%hd",&x);

 v.n=x;

 t.z=v.q;

 printf("%d %d %d %d %d %d %d %d\n",t.h,t.g,t.f,t.e,t.d,t.c,t.b,t.a);

 t.z=v.p;

 printf("%d %d %d %d %d %d %d %d",t.h,t.g,t.f,t.e,t.d,t.c,t.b,t.a);

 return 0;
}

My code gives the correct result. But I think that I've written an unnecessarily long code. Moreover, for larger bytes ( like in case of int ) , the process seems to be tiresome. Can I write the code in a simpler way ?

Avoid global variables - there's no need for t and v to exist outside main().

Always check the return value of scanf() before using the written values.

Don't assume that CHAR_BIT is 8, or that sizeof (short) is 2.  Neither of those is portable.

Don't assume a particular ordering of bit fields within a struct - that's entirely compiler-dependent.

Portable code needs a loop to print the bits, like this:

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

int main(void)
{
    short x;
    if (scanf("%hi", &x) != 1) {
        fputs("Input error\n", stderr);
        return EXIT_FAILURE;
    }

    unsigned short v = (unsigned short)x;

    unsigned short mask = -1u;                /* 11111... */
    mask -= (unsigned short)(mask / 2);       /* 10000... */

    while (mask) {
        printf("%d ", (v & mask) != 0);
        mask >>= 1;
    }
    printf("\n");
}


Missing Numbers Between Two Lists Purpose

I came across this relatively simple question on HackerRank regarding finding missing values across two Lists. I took a fairly straight-forward approach, but is there a "better" / "slicker" way of implementing a solution?

Task

Strategy

Iterate throughList with complete data.
If Integer is in List with incomplete data, remove Integer from both Lists. Else, add Integer to a TreeSet of all missing values.

Implementation

public class MissingNumbersImpl implements MissingNumbers {
    @Override
    public Set<Integer> identifyMissingNumbers(final List<Integer> all, final List<Integer> incomplete) {
        if (all.size() <= incomplete.size()) {
            throw new IllegalArgumentException("list containing all numbers must have a size greater than list not containing all numbers");
        }

        final Set<Integer> missingNumbers = new TreeSet<>();

        final Iterator<Integer> iterator = all.iterator();
        while (iterator.hasNext()) {
            final Integer next = iterator.next();
            if (incomplete.contains(next)) {
                incomplete.remove(next);
                iterator.remove();
            } else {
                missingNumbers.add(next);
            }
        }

        return missingNumbers;
    }
}

Building on the comments made by h.j.k., you forgot to handle one case in your implementation: the case when the bigger list has a count of a number n that is less than the count for that number n for the smaller list. For example, if you consider the following:

public static void main(String[] args) {
    List<Integer> all = new ArrayList<>(Arrays.asList(1, 1, 2, 3, 4, 5, 5));
    List<Integer> incomplete = new ArrayList<>(Arrays.asList(1, 1, 1, 2));
    System.out.println(identifyMissingNumbers(all, incomplete));
}

It will print with your current code [3, 4, 5] disregarding 1 as a missing number although it should be missing since it does not appear with the same frequency in both lists.

The reason is that you're iterating on the number in the bigger list, removing them from the smaller list as you encounter them and adding them to the missing number when you finally don't find a match. But if the smaller list as a bigger count for that number, it won't be taken into account.

There are a couple of possibilities to tackle this: you could introduce a new loop that would have the same logic but iterating on the smaller list instead and removing elements from the bigger list.

I find a better approach would be the following:

Sort each input list in ascending order
Create an empty Map<Integer, Integer> that will store the count for each number
Iterate over the first list. If the number if not in the Map, add it with a value of 1. Else, increment the current count by 1.
Iterate over the second list. If the number if not in the Map, add it with a value of -1. Else, decrement the current count by 1.
At this point, all values in the map that are different than 0 are missing numbers: since we added 1 when encountered in the first list and subtracted 1 when encountered in the second list, the result must be 0 if that number is not missing. Hence, we remove all entries in the map where the value is 0.
Finally, we return the keys of the map in ascending order.

Assuming Java 8, an implementation would be the following:

public static Set<Integer> identifyMissingNumbers(List<Integer> list1, List<Integer> list2) {
    Map<Integer, Integer> count = new HashMap<>();
    for (Integer number : list1) {
        count.merge(number, 1, Integer::sum);
    }
    for (Integer number : list2) {
        count.merge(number, -1, Integer::sum);
    }
    count.values().removeIf(i -> i == 0);
    return new TreeSet<>(count.keySet());
}


Extension vs Class For ViewModel What the advantages & disadvantages to each of these approaches pertaining to creating file that takes care of the view configuration to reduce a controller's file size.

Main Purposes Are:

Memory
Performance
Testing
Usability

This is the simplest, capable of working, example to demonstrate the question, but when many views are present using many methods do any of the above concerns alter when comparing the Extension & ViewModel Class?

Reminder: The ViewModel Class or Extension would be placed in a separate file.

ViewModel Approach:

    class VC: UIViewController {
        lazy var viewModel: ViewModel {
            return (main: self)
        }()

        override viewDidLoad() {
            super.viewDidLoad()

            initializeUI()
        }

        func initializeUI() {
            viewModel.configureView()
        }
    }

    class ViewModel {
        private let main: UIViewController

        init(main: UIViewController) {
            self.main = main
        }

        func configureView() {
            main.view.backgroundColor = UIColor.blue
        }
    }

Extension Approach:

    class VC: UIViewController {
        lazy var viewModel: ViewModel(main: self)

        override viewDidLoad() {
            super.viewDidLoad()

            initializeUI()
        }

        func initializeUI() {
            configureView()
        }
    }

    extension VC {
        func configureView() {
            main.view.backgroundColor = UIColor.blue
        }
    }

The viewModel property on the Extension example should be removed as it is redundant:

lazy var viewModel: ViewModel(main: self)

Irrespective of this, it seems unnecessary to create a separate ViewModel class solely for the purpose of encapsulating a configureView function. On that basis, the Extension approach seems like a much more sensible and performant way of managing this kind of configuration.


Basic graphical R code draws an ellipse instead of a circle I wrote some very basic R code.

This has at least two problems.

It uses a very basic imperative-programming style instead of good R style.
It draws the result as an ellipse, not a circle.  This effect is barely visible on RGui on Windows, but extremely visible in RStudio on Linux. [Edit: rcs has mentioned that the asp parameter of the image command can fix this. Thanks, rcs.]

There are some debatable issues.  For example, there is no good reason why I draw the radius at the distance that I do, so I consider this to be a "magic" number, and I try to avoid "magic numbers" in code because they cause maintenance problems.  The number is in there because I don't think it's worthwhile specifying a user input or a function argument to control it.

edge<-100
magicradius=.81
magicmargin=.01
width<-(2*edge)+1

x<-c(-edge:edge)
y<-c(-edge:edge)

squarelist<-c(1:((width^2)))

dim(squarelist)<-c(width, width)

counter<-0
for (i in 1:length(x)){
  for (j in 1:length(y)){
    counter<-counter+1
    if( (((magicradius-magicmargin)*(edge))<sqrt( x[i]^2 + y[j]^2  ) )
        &(((magicradius+magicmargin)*(edge))>sqrt( x[i]^2 + y[j]^2 ) ) ){
      squarelist[counter]<-sqrt( x[i]^2 + y[j]^2)
    } else {
      squarelist[counter]<-0

    }
  }
}

gg<-matrix(squarelist,nrow=width,ncol=width)

image(gg)

A vectorized solution using outer (Outer Product of Arrays):

edge <- 100
magicradius <- .81
magicmargin <- .01
width <- (2*edge) + 1

x <- -edge:edge
y <- -edge:edge

mat <- outer(x, y, function(x,y) sqrt(x^2 + y^2))
ind <- mat > ((magicradius-magicmargin)*edge) &
       mat < ((magicradius+magicmargin)*edge)
image(ind, asp=1)

The aspect ratio in image() can be set with the asp argument (see the Details section in ?plot.window)


Querying a database with Dapper In the current project that I am working on, we are using dapper to query the database and sometime when mapping from dynamic types to concrete types we end up with a messy block of code.

I created a folder called mappers and have created an Interface<T>, an abstract class inheriting from that interface and I have also set up my IOC to map according to naming conventions so that I can inject my mappers through the constructor of my query classes.

public interface IMapper<out T>
    {
        T Map(SqlMapper.GridReader reader);
    }

    public abstract class MapperBase<T> : IMapper<T>
    {
        public abstract T Map(SqlMapper.GridReader reader);
    }

    public class UserDto
    {
        public int Id { get; set; }
        public string Name { get; set; }
    }

    public class UserDtoMapper : MapperBase<UserDto>
    {
        public override UserDto Map(SqlMapper.GridReader reader)
        {
            return reader.Read().Select(x => new UserDto()
            {
                Id = x.Id,
                Name = x.Name
            }).SingleOrDefault();
        }
    }

    public class QueryObject
    {
        private IMapper<UserDto> userDtoMapper;

        public QueryObject(IMapper<UserDto> userDtoMapper)
        {
            this.userDtoMapper = userDtoMapper;
        }

        public UserDto Query(int id)
        {
            ..........
             return userDtoMapper.Map(reader)
            ...
        }
    }

Setting aside that you won't go with a Mapping framework like Jeroen Vannevel suggested in his comment, this basically looks good but can still be improved.

I don't see any value added by creating an abstract class which implements the IMapper<T> interface. So you better just do it like

public class UserDtoMapper : IMapper<UserDto>
{
    public UserDto Map(SqlMapper.GridReader reader)
    {
        return reader.Read().Select(x => new UserDto()
        {
            Id = x.Id,
            Name = x.Name
        }).SingleOrDefault();
    }
}

But if you travel this route you should do it right and also make QueryObject class generic like

public class QueryObject<T>
{
    private IMapper<T> mapper;

    public QueryObject(IMapper<T> mapper)
    {
        this.mapper = mapper;
    }

    public T Query(int id)
    {
        ..........
         return mapper.Map(reader)
        ...
    }
}


Search datatable faster I would like to make the search faster as it takes a long time to retrieve the data that I require. Is there a faster method to do this? I have written the code below to allow me to search through a data table and retrieve data, which currently is fully functional. It's the nested for loops which I am using and wondering if there is another method of completing the tasks that the for loops get?

 DataTable sqlData = GetConfiguration();

       // var q = sqlData.AsEnumerable().Where(data=> data.Field<String>("slideNo")=="5");

        var w = sqlData.AsEnumerable().Where(data => data.Field<String>("slideNo") == "5")
            .Select(data => data.Field<String>("QuestionStartText")).Distinct();

        List<String> queryResult = new List<String>();
        foreach (var item in w.ToArray<string>())
        {
            if (item != null)
            {
                String queryString = item;
                //queryResult.Clear();
                for (int i = 0; i < excelDataTable.Columns.Count; i++)
                {
                    for (int k = 2; k < excelDataTable.Rows.Count; k++)
                    {
                       String row = "";
                       bool check = excelDataTable.Rows[k][0].ToString().StartsWith(queryString);
                       if (check)
                       {
                           for (int j = 0; j < excelDataTable.Columns.Count; j++)
                           {
                               string value = excelDataTable.Rows[k][j].ToString()+":";
                               row += value;
                           }
                           if (!queryResult.Contains(row))
                           {
                               queryResult.Add(row);
                           }
                       }
                    }
                }
            }
        }

The most obvious problem is when there are large number of strings, string concatenation is inherently slow. Try using a StringBuilder instead, like this:

 DataTable sqlData = GetConfiguration();

       // var q = sqlData.AsEnumerable().Where(data=> data.Field<String>("slideNo")=="5");

        var w = sqlData.AsEnumerable().Where(data => data.Field<String>("slideNo") == "5")
            .Select(data => data.Field<String>("QuestionStartText")).Distinct();

        List<String> queryResult = new List<String>();
        StringBuilder sb = new StringBuilder();
        foreach (var item in w.ToArray<string>())
        {
            if (item != null)
            {
                String queryString = item;
                //queryResult.Clear();
                for (int i = 0; i < excelDataTable.Columns.Count; i++)
                {
                    for (int k = 2; k < excelDataTable.Rows.Count; k++)
                    {
                       sb.Clear();
                       bool check = excelDataTable.Rows[k][0].ToString().StartsWith(queryString);
                       if (check)
                       {
                           for (int j = 0; j < excelDataTable.Columns.Count; j++)
                           {
                               sb.Append(excelDataTable.Rows[k][j].ToString());
                               sb.Append(':');
                           }
                           string row = sb.ToString();
                           if (!queryResult.Contains(row))
                           {
                               queryResult.Add(row);
                           }
                       }
                    }
                }
            }
        }


Implementation of itoa int get_digits (int num)
{
   if(num < 10)
      return 1;

   if(num < 100)
      return 2;

    if(num < 1000)
      return 3;

    if(num < 10000)
      return 4;

    if(num < 100000)
      return 5;

    if(num < 1000000)
      return 6;

    if(num < 10000000)
      return 7;

    if(num < 100000000)
      return 8;

    if(num < 1000000000)
      return 9;

    return 10; /* num > 1000000000 */
}

char *itoa (int n)
{
    static char temp[10]; // MN that can be replaced by some def?
    int nDigits = 0;
    int i = 0;

    if(n == 0)
    {
        temp[0] = '0';
        temp[1] = '\0';

       return temp; // or just return "0"; ?
    }

    nDigits = get_digits(n); // fast function to count digits..

    temp[nDigits] = '\0'; // ..needed just here

   for(i = n; i >= 1; i /= 10) // whole method stinks
    {
        temp[--nDigits] = ((i % 10) + '0'); // modulo is quite slow
    }

    return temp;
}

This is my implementation of the infamous function itoa(), which isn't available everywhere and more importantly not available in my environment. Generally, the implementation of this function is ought to be different anyway. Performance and memory optimization is important.
This function converts an integer to a string. The function owns the reference to the returned string, which is statically allocated and the caller is responsible to make a copy of the returned string if he plans on changing it or preserving it across subsequent calls.

The code almost works.
To make it work in all cases, test the program with Valgrind, which detects undefined behavior because of invalid memory access. This will prove that the buffer needs to be 11 bytes long, not only 10.
What about platforms where int has 64 bits instead of just 32? For these you need a larger buffer. Until then, you should use a compile-time assertion (static_assert) to ensure this implicit assumption.
What about negative numbers? -6 is a valid integer as well, and it should be converted appropriately.
If this function is the bottleneck of your whole program because it is too slow, have a look at how the Go programming language converts integers to strings. It's in the strconv package and uses lots of nice tricks to cut down the number of integer divisions, since that's the most expensive machine instruction in your code.
You can get rid of the get_digits function if you have the end of the string at a fixed address. Start with:
char *p = buf + sizeof buf - 1;
*p = '\0';

and then continue to fill the buffer from right to left by doing *(--p) = '0' + digit. At the end just return p, which will point to the first digit.
The return type should be const char * instead of char * since the caller is not supposed to do anything to the buffer.


Python: improve in elegant way (code saving) a function in order to avoid several statements I wrote this function to read Las file and save a shapefile. The function creates a shapefile with 8 fields. What I wish insert a parse element in the function in order to select the fields I wish to save LAS2SHP(inFile, outFile=None, parse=None). If parse=None all fields are saved. If  parse="irn" the fields intensity, return_number, and number_of_returns are saved, following the legend:

"i": p.intensity,
"r": p.return_number,
"n": p.number_of_returns,
"s": p.scan_direction,
"e": p.flightline_edge,
"c": p.classification,
"a": p.scan_angle,

I wrote a solution if....ifelse....else really code consuming (and not elegant). Thanks for all helps and suggestions for saving code.

Here is the original function in Python:

import shapefile
from liblas import file as lasfile

    def LAS2SHP(inFile,outFile=None):
        w = shapefile.Writer(shapefile.POINT)
        w.field('Z','C','10')
        w.field('Intensity','C','10')
        w.field('Return','C','10')
        w.field('NumberRet','C','10')
        w.field('ScanDir','C','10')
        w.field('FlightEdge','C','10')
        w.field('Class','C','10')
        w.field('ScanAngle','C','10')
        for p in lasfile.File(inFile,None,'r'):
            w.point(p.x,p.y)
            w.record(float(p.z),float(p.intensity),float(p.return_number),float(p.number_of_returns),float(p.scan_direction),float(p.flightline_edge),float(p.classification),float(p.scan_angle))
        if outFile == None:
            inFile_path, inFile_name_ext = os.path.split(os.path.abspath(inFile))
            inFile_name = os.path.splitext(inFile_name_ext)[0]
            w.save("{0}\\{1}.shp".format(inFile_path,inFile_name))
        else:
            w.save(outFile)

A possible dictionary based solution:

import os.path

import shapefile
from liblas import file as lasfile

# map fields representing characters to field name and attribute
FIELDS = {
    'i': ('Intensity', 'intensity'),
    'r': ('Return', 'return_number'),
    'n': ('NumberRet', 'number_of_returns'),
    's': ('ScanDir', 'scan_direction'),
    'e': ('FlightEdge', 'flightline_edge'),
    'c': ('Class', 'classification'),
    'a': ('ScanAngle', 'scan_angle'),
}

# assuming that field order is important define it explicitly, otherwise it
# could simply be DEFAULT_PARSE = FIELDS.keys()
DEFAULT_PARSE = 'irnseca'

def enhanced_LAS2SHP(inFile, outFile=None, parse=DEFAULT_PARSE):
    w = shapefile.Writer(shapefile.POINT)
    # add 'Z' field not present in fields map
    w.field('Z', 'C', '10')
    # initialise list of desired record attributes
    attributes = []
    # add mapped 'shapefile' fields and desired attributes to list
    for f in parse:
        field, attr = FIELDS[f]
        w.field(field_name, 'C', '10')
        attributes.append(attr)
    # create record from attributes in list of desired attributes
    for p in lasfile.File(inFile, None, 'r'):
        w.point(p.x, p.y)
        record_args = [float(p.z)]
        record_args += (float(getattr(p, attr)) for attr in attributes)
        w.record(*record_args)
    # if not output filename was supplied derive one from input filename
    if outFile is None:
        inFile_path, inFile_name_ext = os.path.split(os.path.abspath(inFile))
        inFile_name, _ = os.path.splitext(inFile_name_ext)
        outFile = os.path.join(inFile_path, '{}.shp'.format(inFile_name))
    # save records to 'shapefile'
    w.save(outFile)


Find position of a word in a list in Python The aim of this program is to say which position a word is in a list. This  is just a basic version of it. I made to see if I could shorten it more than what it is currently.

position, word, word_list = 0, 'hello', 'hello what is your name i am bob this is a sentence a very n ice sentence'
word_list = word_list.split(" ")
print(word_list)
if word in word_list:
    for name in word_list:
        position = position + 1
        if word == name:
            print('your word is in the list at ',position)
else:
    print('Your word is not in the sentence')

A similar solution using enumerate():

word, word_list = 'hello', 'hello what is your name i am bob this is a sentence a very n ice sentence'

if word in word_list:
    for position, name in enumerate(word_list):
        if name == word:
            print("Your word is in the list at ", position)
else:
    print('Your word is not in the sentence')

You could also use a list comprehension:

word, word_list = 'hello', 'hello what is your name i am bob this is a sentence a very n ice sentence'

positions = [x for x, n in enumerate(word_list).split() if n == word]
if positions:
    for position in positions:
        print('your word is in the list at ', position)
else:
    print('Your word is not in the sentence')

Also, a generator expression that's a little more efficient:

word, word_list = 'hello', 'hello what is your name i am bob this is a sentence a very n ice sentence'

found = False
for position in (x for x, n in enumerate(word_list) if n == word):
    print('your word is in the list at ', position)
    found = True
if not found:
    print('Your word is not in the sentence')


WebAPI - Return models vs entity and partial class with meta data Wanted some opinions on something I have been working with, I've tried a few ways of structuring the way I retrieve and return data to my WebAPI, and I'm pretty happy with what I have so far, but I was looking for a comparison of the pro's and con's of using models vs entities for returning data on the WebAPI.

In my opinion, from what I've seen, adding a model factory and returning models is good because you get control over what your object looks like, but it does mean you now have to map the entities to that model somewhere, increasing the code you have to write, particularly if you have a lot of entities.

On the other hand, having partial classes of the generated objects allows me the same extensibility, and by adding a meta data class using MetadataType(typeof(typename)), I am able to decide what will be serialized and how they are built, giving me full control again over the structure, and being able to extend it when I want to, rather than having to for every entity.

So, if I can get the same benefits with partials and metadata, without the additional code in mapping to models, what are other downsides, how come this approach is not as common? I mean, if I wanted I could do the mapping this way too, without the need for any factories to do the mapping, just by attributing the fields to be ignored and creating new ones with Get/Set to point to the original property.

An example:

Generated by T4 template:

 public partial class FileHeader : BaseEntity
 {
    public FileHeader()
    {
        this.FileVersionSubs = new HashSet<FileVersion>();
    }

    public int FileId { get; set; }
    public string Name { get; set; }
    public string Caption { get; set; }

    public virtual ICollection<FileVersion> FileVersionSubs { get; set; }
 }

The partial I make:

[MetadataType(typeof(FileHeaderMetaData))]
public partial class FileHeader
{
    // Additional fields go here
}

Metadata file:

public class FileHeaderMetaData
{
    // Any attributes for existing fields here, eg:
    [MyCustomAttribute]
    public int FileId { get; set; }
}

If I were to approach this from a Model point of view, I would be repeating the same properties for every class I generate and wanted to return as well as mapping them, rather than the ones I explicitly want to change or hide.

Thoughts, feelings and suggestions welcome!

No matter how simple I think my application is going to be, I always end up separating my viewmodels from my entities. Not only do I always encounter at least one case where the apparent convenience of any other approach makes it very unwieldly for me to finish a feature, but it's also good to practice loose coupling between your views and your domain.

Say you're using entities with partial classes directly in your views, and the same entity is used in multiple views. Now you have a new feature that requires a change to your entities. How do you know it won't break your existing views? The compiler won't catch it, unless you have an automated suite of UI tests, you'll need to remember to manually test every screen again.

Instead, if you create one viewmodel per view, you get the following advantages:

Each viewmodel can be constructed to meet the needs of a single view. Don't need to some properties of an entity? Leave it out of the viewmodel! Want to include a few extra properties? Throw them in! Want to flatten out some entity hierarchy because it will be easier to display? Do it!
Some (but not all) breaking changes you make to your entities will now be detected by your compiler. Even if you have unit tests (and you should), this is a nice reminder.
Factories are much easier to test than views.

In summary, the viewmodel should be the simplest representation of what the view needs, and no more. This decouples your entities from your views as well as decouples your views from each other. It also places mode code where it can be easily tested and maintained. It is a little more work initially but a lot less work in the long run.


Does this ugly code fit into a design pattern? I'm trying to figure out a pattern to best accommodate this pretty ugly code:

var userDiscounts = _discountRepository.GetUserDiscountsForUsersWithDiscounts(new List<int> { buyerUserId, sellerUserId }).ToList();

// Buyer.
var buyerHasFreeCommission = false;
var buyerDiscounts = userDiscounts.Where(ud => ud.UserId == buyerUserId);
foreach(var userDiscount in buyerDiscounts)
{
    switch (userDiscount.Discount.Type)
    {
        case DiscountTypes.SingleFreeCommisionPerUser:
            buyerHasFreeCommission = true;
            break;
    }
}

var sellerHasFreeCommission = false;
var sellerDiscounts = userDiscounts.Where(ud => ud.UserId == sellerUserId);
foreach(var userDiscount in sellerDiscounts)
{
    switch (userDiscount.Discount.Type)
    {
        case DiscountTypes.SingleFreeCommisionPerUser:
            sellerHasFreeCommission = true;
            break;
    }
}

if (!sellerHasFreeCommission)
{
    var sellerCommisionChargeTransaction = new Transaction
    {
        AccountId = sellerAccountId,
        Type = TransactionTypes.Fee,
        Amount = -2
    };

    var sellerTransferFeeTransaction = new Transaction
    {
        AccountId = 5020,
        Type = TransactionTypes.Fee,
        Amount = 2
    };

    _accountRepository.AddTransaction(sellerCommisionChargeTransaction);
    _accountRepository.AddTransaction(sellerTransferFeeTransaction);
}

if (!buyerHasFreeCommission)
{
    var buyerCommisionChargeTransaction  = new Transaction
    {
        AccountId = buyerAccountId,
        Type = TransactionTypes.Fee,
        Amount = -2
    };

    var buyerTransferFeeTransaction = new Transaction
    {
        AccountId = 5020,
        Type = TransactionTypes.Fee,
        Amount = 2
    };

    _accountRepository.AddTransaction(buyerCommisionChargeTransaction);
    _accountRepository.AddTransaction(buyerTransferFeeTransaction);
}

if (!buyerHasFreeCommission || !sellerHasFreeCommission)
    _accountRepository.SaveChanges();

This won't be very maintainable when I need to add a new discount... I was thinking maybe somehow I could use the strategy pattern?

The strategy pattern is a good candidate for this, one way you could implement it would be something like as follows (the discounts are the strategies).

Define an interface for classes which can apply discounts:

public interface IApplyDiscount
{
    void Apply(buyerUserId, buyerTransaction, sellerUserId, sellerTransaction);
}

Create an implementation of the interface for the SingleFreeCommisionPerUser discount.

public class SingleFreeCommisionPerUserDiscount : IApplyDiscount
{
    public void Apply(buyerUserId, buyerTransaction, sellerUserId, sellerTransaction)
    {
        var userDiscounts = _discountRepository.GetUserDiscountsForUsersWithDiscounts(new List<int> { buyerUserId, sellerUserId }).ToList();

        var buyerHasFreeCommission = userDiscounts.Any(ud => ud.UserId == buyerUserId && ud.Discount.Type == DiscountTypes.SingleFreeCommisionPerUser);

        if(buyerHasFreeCommission)
        {
            buyerTransaction.Amount = 0;
        }

        var sellerHasFreeCommission = userDiscounts.Any(ud => ud.UserId == buyerUserId && ud.Discount.Type == DiscountTypes.SingleFreeCommisionPerUser);

        if(sellerHasFreeCommission)
        {
            sellerTransaction.Amount = 0;
        }
    }
}

The main code then creates the initial commission transactions:

var sellerCommisionChargeTransaction = new Transaction
{
    AccountId = sellerAccountId,
    Type = TransactionTypes.Fee,
    Amount = -2
};

var buyerCommisionChargeTransaction  = new Transaction
{
    AccountId = buyerAccountId,
    Type = TransactionTypes.Fee,
    Amount = -2
};

Applies all the available discounts:

List<IApplyDiscount> discounts = GetAllDiscounts();
discounts.ForEach(d => d.Apply(buyerUserId, buyerCommisionChargeTransaction, sellerUserId, sellerCommisionChargeTransaction));

Then, only add the corresponding charge if a transaction amount is due.

if (sellerCommisionChargeTransaction.Amount < 0)
{
    var sellerTransferFeeTransaction = new Transaction
    {
        AccountId = 5020,
        Type = TransactionTypes.Fee,
        Amount = -sellerCommisionChargeTransaction.Amount
    };

    _accountRepository.AddTransaction(sellerCommisionChargeTransaction);
    _accountRepository.AddTransaction(sellerTransferFeeTransaction);
}

if (buyerCommisionChargeTransaction.Amount < 0)
{
    var buyerTransferFeeTransaction = new Transaction
    {
        AccountId = 5020,
        Type = TransactionTypes.Fee,
        Amount = -buyerCommisionChargeTransaction.Amount
    };

    _accountRepository.AddTransaction(buyerCommisionChargeTransaction);
    _accountRepository.AddTransaction(buyerTransferFeeTransaction);
}

Then if you want to add other discount types and rates, you can just create additional IApplyDiscount implementations and include them in the results of the GetAllDiscounts() call.

This implementation is more flexible, you don't need to change any of the current implementation to add additional discounts.

The discounts could be percentages and the corresponding TransferFeeTransaction will cater for that by charging the positive value of whatever amount remains on the CommissionChargeTransaction.


Sort 3 numbers in Common Lisp with the minimum number of operations I’m wondering is the following function returns a list of three numbers sorted in ascending order with the minimum possible number of operations, or if there is some more efficient or elegant method (a part from using the primitive function sort!)

(defun order (min mid max)
"sort three numbers in ascending order"
  (when (< mid min)
    (rotatef mid min))
  (if (< max min)
      (rotatef max mid min)
      (when (< max mid)
        (rotatef mid max)))
  (list min mid max))

With “minimum possible number of operations” I mean that the function should solve the problem with a number of tests and assignments (rotatef swaps two variables or rotates three or more variables) which is the minimum on the average on all the possible cases.

It does seem to be correct. As for elegance, I believe it can be expressed without nested conditionals and I would consider that more elegant, even if it takes the average number of comparisons from 2.5 to 3. This code does 3 comparisons (always) and up to six writes. Your code does 2 or 3 comparisons and up to seven writes (ignoring temps, that would take the code below to "up to nine" and your code to "up to ten").

On the balance, your code probably has a slight edge in performance, so if this would be a frequently-called function, I would consider using order over order3.

That is:

(defun order3 (min mid max)
  (when (< max min)
     (rotatef max min))
  (when (< mid min)
     (rotatef mid min))
  (when (< max mid)
     (rotatef max mid))
  (list min mid max))


Selectable date filters I created a simple app with Backbone and Marionette. In a view I created an onRender function.
    I created a page that displays a Report with a date filter, using a select tag and affect to an input when the select tag is chosen.
    I made that with Backbone and JQuery. And for date I'm using Moment.js and Pikaday.js.

This is a part of my code

var selectdate = $('#publicdate',this.el)[0];
selectdate.addEventListener("change",function(){
    var value = selectdate.value;
    var date1 = $('#date1',this.el)[0];
    var date2 = $('#date2',this.el)[0];
    if(value==="today"){
        date1.value = moment().add('days').format('DD-MM-YYYY')
        date2.value = moment().add('days').format('DD-MM-YYYY')
        $(date1).attr('disabled',true);
        $(date2).attr('disabled',true);
    } else if(value==="yesterday"){
        date1.value = moment().add(-1,'days').format('DD-MM-YYYY')
        date2.value = moment().add(-1,'days').format('DD-MM-YYYY')
        $(date1).attr('disabled',true);
        $(date2).attr('disabled',true);
    } else if(value==="thismonth"){
        date1.value = moment().add('month').startOf('month').format('DD-MM-YYYY')
        date2.value = moment().add('month').endOf('month').format('DD-MM-YYYY')

        $(date1).removeAttr('disabled',true);
        $(date2).removeAttr('disabled',true);
    } else if(value==="lastmonth"){
        date1.value = moment().add(-1,'month').startOf('month').format('DD-MM-YYYY')
        date2.value = moment().add(-1,'month').endOf('month').format('DD-MM-YYYY')
        $(date1).attr('disabled',true);
        $(date2).attr('disabled',true);
    }
})

Is that code is already clean? Is there another way to make that code shorter? Like using a function or something.

jQuery

Your code uses jQuery, but you don't seem to be using any features of jQuery that offer any advantages over the DOM - unless you need to target older browsers you can remove your jQuery dependency and thus make your code better: faster (less abstraction, fewer dependencies to download), more portable, more future-proof, etc).

Simply replace $("#id") with document.getElementById and $(selector) with document.querySelectorAll (you can alias it for brevity if necessary: var q = document.querySelectorAll;).

Time-safety

The moment() function, when called, returns the current time value the moment the function is called - your code keeps on calling the moment() function which could return different values each time it's called (if there's a long-enough delay in the interpreter, or if your code executes on a time-unit boundary) - issues like these can cause bugs if midnight at the end of a month happens while your function is executing.

For this reason, you should get the value of "now" only once, at the start of your function, and perform all operations on that value (you should treat it as an immutable value too, even though Date is mutable, as that's best-practice).

var now = moment();
// ...
date1.value = now.add('days').format('yada');

Culture-sensitivity

You're using the dd-MM-yyyy pattern - only a few countries use dashes between the components and it is not ISO-8601 compliant. If this is intentional then that's fine - but if your users are coming from a variety of different countries (with their own date-formatting preferences) then you have two choices:

Use ISO-8601 (yyyy-MM-dd HH:mm:ss) which is unambiguous and as a bonus: is lexicographically sortable.
Use Moment's built-in localization ("i18n") support: http://momentjs.com/docs/#/i18n/

To respect your users' settings, do this:

Get moment-with-locales.js from their website
Modify your initialization code:

// Get the ISO culture settings for the current user, e.g. "en-US" or "es-MX":
var locale = window.navigator.userLanguage || window.navigator.language;

// Configure moment:
moment.locale( locale );

When using locales, use the "locale formats" rather than specific formats. Moment uses "L" to denote a "numeric date" (e.g. "09/07/2016" in en-GB, "07/09/2016" in en-US, or "2016-07-09" in ISO-8601).

Everything

So your code would become:

var locale = window.navigator.userLanguage || window.navigator.language;
moment.locale(locale);

var selectDate = document.getElementById('publicdate');
selectDate.addEventListener('change', onSelectDateChange);

function onSelectDateChange(e) {
    var date1 = document.getElementById('date1');
    var date2 = document.getElementById('date2');
    var now = moment();
    if( selectDate.value == "today" ) {
        date1.value = now.add('days').format('L');
        date2.value = now.add('days').format('L');
        date1.setAttribute('disabled', 'disabled');
        date2.setAttribute('disabled', 'disabled');
    }
    else if( value == "yesterday" ) {
        // and so on...
    }
}


Performance for creating millions of database entries class Generator
{
    public function createCodes($count, $project, $generator)
    {
        $batchSize = 250;
        $batches = ceil($count / $batchSize);

        $sharedColumns = [
            'timestamp' => time(),
            'parent_id' => $project->id,
            'source' => $generator->name,
            'generator_id' => $generator->id,
        ];

        $created = 0;
        for ($batch = 0; $batch < $batches; $batch++) {
            $size = min($batchSize, $count - $created);

            $time = time();
            $inserts = [];
            for ($i = 0; $i < $size; $i++) {
                $parentCode = self::generateCode();
                $inserts[] = [
                    'type' => 'parent',
                    'parent_code' => '',
                    'code' => $parentCode,
                    'series' => self::generateSeries(),
                ];

                for ($j = 0; $j < $generator->children; $j++) {
                    $inserts[] = [
                        'type' => 'child',
                        'parent_code' => $parentCode,
                        'code' => self::generateCode(),
                        'series' => self::generateSeries(),
                    ];
                }
            }

            $columns = array_keys($sharedColumns + $inserts[0]);
            $valueString = '(' . implode(', ', array_fill(0, count($columns), '?')) . ')';
            $queryString = 'INSERT INTO project_codes (' . implode(',', array_map('Database::quoteIdentifier', $columns)) . ') VALUES ';
            $queryString .= implode(',', array_fill(0, count($inserts), $valueString));

            // Flatten all entries and add shared columns
            $values = array_reduce($inserts, function ($carry, $item) use ($sharedColumns) {
                return array_merge($carry, array_values($sharedColumns), array_values($item));
            }, []);

            $query = \Database::getInstance()->prepare($queryString);
            $query->execute($values);

            $created += $size;
        }
    }

    static $codeChars = 'ABCDEFGHJKLMNPQRTUVWXY34789';

    protected static function generateCode()
    {
        return substr(str_shuffle(self::$codeChars), 0, 9);
    }

    protected static function generateSeries()
    {
        $numbers = range(1, 30);
        shuffle($numbers);
        $numbers = array_slice($numbers, 0, 5);

        return implode(',', $numbers);
    }
}

The software this code is used in manages prize draws. Companies let the software generate codes that they can e.g. print on their products. Buyers of these products can enter codes back into the system to enter a prize draw. At specified dates the system draws winners from the entered codes and notifies them.

What this particular piece of code should be capable of is to generate a specified amount of random (and unique) codes so they can later be exported by the company and printed on their products. The generated code is stored in the code column, but winners are determined by the series column which is hidden from the end user/customer.

The code doesn't yet guarantee uniqueness. My plan is to select all duplicates for code and series (after generation and inserts) and just generate new ones until there are no duplicates anymore. That way I don't have to check each of the million generated codes for uniqueness.

I tested this with a count of 1 million, which took 163s to complete and according to memory_get_peak_usage(true) consumed 4MB of RAM (which I find unlikely, but ok).

I experimented with different batch sizes but the gain of fewer queries seemed to pretty much cancel out with the additional function calls and higher array sizes for the array reduction.

Is there anything I can do to decrease execution time? I don't expect a million entries to be generated in 10 seconds, but if there's any gain to be had I'd appreciate it.

Prepare once. That's the very idea behind a prepared statement. It should give you like 5% gain.

Wrap all batches in a single transaction. It could also help you in several ways. In case your engine is myisam, consider dropping all indexes before inserts and adding them back after.

Reduce the record size. 'parent' is 5 megabytes written on the disk for 1 mil records 1 megabyte if your value is 1.


Drawing a table for data from file I wrote a function for drawing a table for data that comes from file:

import data_manager
import os

table = data_manager.get_table_from_file('games.csv')

len_of_longest_name = 0
i = 0
while i < len(table):
    if len_of_longest_name < len(table[i][1]):
        len_of_longest_name = len(table[i][1])
    i += 1

i = 0
len_of_longest_studio = 0
while i < len(table):
    if len_of_longest_studio < len(table[i][2]):
        len_of_longest_studio = len(table[i][2])
    i += 1
print("/", 6 * "-", "|", (len_of_longest_name - 2) * "-", "|", (len_of_longest_studio - 2) * "-", "|", "---", "\\")

# constructing string

i = 0
while i < len(table):
    string = '|'
    string += table[i][0] + '|'
    if len(table[i][1]) < len_of_longest_name:
        spaces = (len_of_longest_name - len(table[i][1])) * ' '
        string += table[i][1] + spaces + '|'
    else:
        string += table[i][1] + '|'
    if len(table[i][2]) < len_of_longest_studio:
        spaces = (len_of_longest_studio - len(table[i][2])) * ' '
        string += table[i][2] + spaces + '|'
    else:
        string += table[i][2] + '|'
    string += table[i][3] + '|'
    if len(table[i][4]) < 2:
        string += table[i][4] + ' |'
    else:
        string += table[i][4] + '|'
    if i < len(table) - 1:
        print(string)
        print("|", 6 * "-", "|", (len_of_longest_name - 2) * "-", "|", (len_of_longest_studio - 2) * "-", "|", "---", "|")
    else:
        print(string)
    i += 1
print("\\", 6 * "-", "|", (len_of_longest_name - 2) * "-", "|", (len_of_longest_studio - 2) * "-", "|", "---", "/")

The code works as I want but I think there's a better (shorter) method to do the same. Could you show me the method?

The output looks like this:

Python has many great ways to iterate.  In general if you find yourself looping on an integer, like in the following code, there is a good chance you are doing it wrong.  It is generally better to iterate on the data structure itself.

Sample code:

I will use this loop as an example:

len_of_longest_name = 0
i = 0
while i < len(table):
    if len_of_longest_name < len(table[i][1]):
        len_of_longest_name = len(table[i][1])
    i += 1

Don't iterate on the length of a data structure:

This is probably not what you want:

while i < len(table):

It would likely be better as something like:

for row in table:

Or, if for some reason you really need the index, then:

for i, row in enumerate(table):

Example with looping on length removed:

So removing i, and looping on table, we get:

len_of_longest_name = 0
for row in table:
    if len_of_longest_name < len(row[1]):
        len_of_longest_name = len(row[1])

Use builtin functions.

Python have a max() function.  Using max(), the example becomes:

len_of_longest_name = 0
for row in table:
    len_of_longest_name = max(len_of_longest_name, len(row[1]))

Comprehensions:

Since max() can take a comprehension the example loop can be reduced to:

len_of_longest_name = max(len(row[1]) for row in table)

That is six lines of code reduced to one.


Optimize/ refactor program that highlights differences between two text collections I made a simple comparison window in WPF that has two RichTextBoxes highlighting the differences (in red) of two StringCollections.

I knocked the code up in about 5 minutes and having gone back to look at it it appears pretty shoddy. I've included comments to show my intentions.

private static void DifferentiateText(RichTextBox richTextBox1, RichTextBox richTextBox2, StringCollection text1,
    StringCollection text2, SolidColorBrush brushColour)
{
    var text1Length = text1.Count;
    var text2Length = text2.Count;

    //Loop through text1 array
    for (var i = 0; i < text1Length; i++)
    {
        var text1Line = text1[i] + "\r\n";

        //If we're within text1 and text2 array boundaries, then compare their values
        if (i < text2Length)
        {
            var text2Line = text2[i] + "\r\n";

            //If lines aren't the same, apply brush
            if (text1Line != text2Line)
            {
                AssignTextToTextBox(richTextBox1, text1Line, brushColour);
                AssignTextToTextBox(richTextBox2, text2Line, brushColour);
            }
            else //Output with default brush
            {
                AssignTextToTextBox(richTextBox1, text1Line);
                AssignTextToTextBox(richTextBox2, text2Line);
            }
        }
        else //At the end of text2 array, so just output the rest of text1
        {
            AssignTextToTextBox(richTextBox1, text1Line, brushColour);
        }

        //If we're at the end of text1 array but not at the end of text2 array, then just output the rest of text2
        if (i == text1Length - 1 && text1Length < text2Length)
        {
            for (; i < text2Length; i++)
            {
                var text2Line = text2[i] + "\r\n";
                AssignTextToTextBox(richTextBox2, text2Line, brushColour);
            }
        }
    }
}

private static void AssignTextToTextBox(RichTextBox richTextBox, string text, SolidColorBrush brushColour = null)
{
    var textRange = new TextRange(richTextBox.Document.ContentEnd, richTextBox.Document.ContentEnd)
    {
        Text = text
    };

    if(brushColour != null)
        textRange.ApplyPropertyValue(TextElement.ForegroundProperty, brushColour);
}

I don't particularly like how AssignToTextBox is used 6 times in the first for loop when really it will only be called a maximum of two times.

Is there any way to optimize/ refactor/ make-less-shoddy this code?

Since you're using WPF, you should really consider implementing MVVM.

Wouldn't Environment.Newline be better than "\r\n"?

Also, why do you add the new line at those times? That way you're intorducing several points where you could forget to do so. Wouldn't it be better to apply this only in AssignTextToTextBox(), e.g. Text = text + Environment.Newline?

Comments should explain why something was implemented in that way, not what the code is doing. For example: //If lines aren't the same, apply brush doesn't tell me anything the code isn't showing me already. If you need comments to explain what your code does, it's a sign you need to re-write the code.

I don't particularly see the point of text1Length and text2Length, just use text1.Count and text2.Count directly. But more importantly, why are you using StringCollection when List<string> is available?

If you apply these changes, I think your logic can be reduced to this:

var maximum = (list1.Count > list2.Count) ? list1.Count : list2.Count;

for (var i = 0; i < maximum; i++)
{
    var text1Line = (i >= list1.Count) ? string.Empty : list1[i];
    var text2Line = (i >= list2.Count) ? string.Empty : list2[i];

    var applicableBbrushColour = (text1Line == text2Line) ? null : brushColour;

    AssignTextToTextBox(richTextBox1, text1Line, applicableBbrushColour);
    AssignTextToTextBox(richTextBox2, text2Line, applicableBbrushColour);
}

You could even move (i >= list1.Count) ? string.Empty : list1[i]; to a separate method, considering this logic is repeated; perhaps even to an extension method?


Class ISBN - checks for valid ISBN 13 I am at the moment trying to grasp multi-file programming instead of one big main.cpp.

The class I wrote is about ISBN 13.

Can you point out what could be done to improve my code?

Also is there a good guide how to write include guards? My sources differate in this respect.

Runnable code here on Wandbox

ISBN.hpp

#include <ostream>

class ISBN
{
/* examples 978-345-314-697-6
978-3-12-732320-7
*/
private:
    //the numbers between the minus signs
    unsigned int isbn_field1;
    unsigned int isbn_field2;
    unsigned int isbn_field3;
    unsigned int isbn_field4;
    unsigned int isbn_field5;
public:
    //constructors
    ISBN()
        : ISBN{978,3,12,732320,7} // a valid ISBN as default
    {}
    ISBN(const unsigned int& a, const unsigned int& b, const unsigned int& c, const unsigned int& d, const unsigned int& e);

    //output operator overloading
    friend std::ostream& operator<<(std::ostream& os, ISBN const& i);
};//ISBN

ISBN.cpp

#include "ISBN.hpp"
#include <iostream>
#include <string>

bool is_valid_isbn(const std::string& digits)
{
    unsigned int weighted_sum=0;
    std::string temp;
    for (unsigned int i=0; i<(digits.length());i++)
    {
        temp=digits[i];
        if ((i%2)==0)
        {
            weighted_sum+=std::stoi(temp);
        }
        else
        {
            weighted_sum+=std::stoi(temp)*3;
        }
    }
    if ((weighted_sum%10)!=0)
    {
        return false;
    }
    return true;
}

ISBN::ISBN(const unsigned int& a, const unsigned int& b, const unsigned int& c, const unsigned int& d, const unsigned int& e)
{
    const std::string digit_string=std::to_string(a)
    +std::to_string(b)
    +std::to_string(c)
    +std::to_string(d)
    +std::to_string(e);
    std::string temp;
    //check if last field is exactly one digit
    temp=std::to_string(e);
    if(temp.length()!=1)
    {
        throw "last unit is not a single digit\n";
    }
    //check if first field is 978 or 979
    if(a!=978 && a!=979)
    {
        throw "first unit is neither 978 nor 979\n";
    }
    //check number of digits=13
    if(digit_string.length()!=13)
    {
        throw "number of digits is not exactly 13\n";
    }
    if (!is_valid_isbn(digit_string))
    {
        throw "not a valid ISBN\n";
    }

    isbn_field1=a;
    isbn_field2=b;
    isbn_field3=c;
    isbn_field4=d;
    isbn_field5=e;
}

std::ostream& operator<<(std::ostream& os, ISBN const& i)
{
    return os << i.isbn_field1 << "-"
    << i.isbn_field2 << "-"
    << i.isbn_field3 << "-"
    << i.isbn_field4 << "-"
    << i.isbn_field5;
}

main.cpp

#include "ISBN.hpp"

#include <iostream>

int main()
{
    try
    {
        ISBN test;
        std::cout << test;
    }
    catch(char const* e)
    {
        std::cout << "Error:" << e;
    }
    catch(...)
    {
        std::cout << "test error";
    }
    return 0;
}

The comment in the ISBN class should rather be outside the class declaration, and it should contain a few words for the human reader, such as the expanded abbreviation and the Wikipedia page, to make it easy to get additional information about the topic of ISBNs.

Storing an ISBN as 5 numbers is wrong. If the length of these numbers doesn't sum up to 13, you cannot be sure where to add the zeroes. The Wikipedia article contains a table of example ISBN-10 numbers, and some of them have leading digits.

The variable names isbn_field1 are bad. The word isbn is not necessary since that information is given by the class ISBN already. Therefore field1 is better than isbn_field1. But what is field1, what does it mean? The Wikipedia article gives nice names to these fields, and so should your code.

The characters between the digits are not "minus signs" (as you say) but hyphens.

The no-argument constructor is wrong. There should be no way to construct an ISBN object without specifying all its parts. Having a single "best default" is simply not possible for an ISBN. You definitely don't want to convert all your programming mistakes to a book about analytic geometry.

In the constructor with the const unsigned int& parameters, the const& is not necessary. Integer values are typically passed directly instead of referencing them since they are one machine word long, and the computer can process them quickly. References make your code run slower in this case. For larger types such as std::vector<std::vector<ISBN>> it makes sense to use references instead of simple values.

In the is_valid_isbn function, you should add spaces around the tokens in i=0. Most C++ programmers are used to read this as i = 0.

In the same line, the parentheses around digits.length() are not necessary.

Instead of calling std::stoi, you can also write digits[i] - '0', which gives you the numeric value of a digit character. Which reminds me that some of the characters might not be digits at all. Your code should skip them and later verify that the string contained 13 digits in total.

In the ISBN::ISBN constructor, instead of checking whether the last field has length 1, you can just check whether e < 10. This is simpler and faster.

When validating multiple values, you should validate them in reading order. Start with the first field, and validate the last field last.

You didn't test your code by calling ISBN{0, 0, 0, 0, 0}.

When you throw something from your code, it should be a proper exception from the C++ standard library, such as std::invalid_argument.

Instead of the verbose std::string temp; temp = std::to_string(e); temp.length() != 1, you can simply write std::to_string(e).length() != 1. This way you don't have to think about a better variable name instead of temp. The name temp is always a bad variable name since it doesn't tell enough of a story. (Well, except if you use it as an abbreviation for temperature, but that's not the case here.)

The exception strings should not include a newline character. That character should be added when the exception is printed in a log file. If you want to save the exception somewhere else, the newline is probably wrong.

The main program is pretty useless. There is no way to enter an ISBN and validate it. You should rather define the following functions to get some automated tests:

void assert_valid_isbn(const std::string &isbn)
{
    /* TODO */
}

void assert_invalid_isbn(const std::string &isbn, const std::string &expected_error)
{
    /* TODO */
}


Gridview shuttering when scrolling I have more than 1000 items in my GridView. When scrolling though, it appears to make the app 'shutter'. Each time I load images from Picasso GC, it increases.

import java.util.ArrayList;
import java.util.HashMap;

import android.app.Activity;
import android.content.Context;
import android.view.LayoutInflater;
import android.view.View;
import android.view.ViewGroup;
import android.widget.BaseAdapter;
import android.widget.ImageView;
import android.widget.TextView;

import com.squareup.picasso.Picasso;

public class BrandsItemAdapter extends BaseAdapter {

    public Activity activity;
    private static LayoutInflater inflater = null;

    public ArrayList<HashMap<String, String>> data;

    public HashMap<String, String> ITEM;

    public BrandsItemAdapter(Activity activity,ArrayList<HashMap<String, String>> data)
    {
        this.activity=activity;
        inflater = (LayoutInflater) activity.getSystemService(Context.LAYOUT_INFLATER_SERVICE);
        this.data = data;
    }
    @Override
    public int getCount() {
        // TODO Auto-generated method stub
        return data.size();
    }

    @Override
    public Object getItem(int position) {
        // TODO Auto-generated method stub
        return position;
    }

    @Override
    public long getItemId(int position) {
        // TODO Auto-generated method stub
        return position;
    }

    @Override
    public View getView(int position, View convertView, ViewGroup parent) {
        // TODO Auto-generated method stub
        ViewHolder holder = null;
        ITEM = new HashMap<String, String>();
        ITEM = data.get(position);

        if(convertView == null)
        {
        convertView  = inflater.inflate(R.layout.department_viewadapter, parent, false);
        holder = new ViewHolder();

        holder.mTitleView = (TextView) convertView.findViewById(R.id.textView1);
        holder.imageView1 = (ImageView) convertView.findViewById(R.id.imageView1);

        convertView.setTag(holder);
        }else
        {
        holder = (ViewHolder) convertView.getTag();
        }

        try
        {
            holder.mTitleView.setText(ITEM.get("name"));
            Picasso.with(activity).load(ITEM.get("image")).placeholder(R.drawable.icon).resize(150, 150).error(R.drawable.icon).into(holder.imageView1);
            System.gc();

        }
        catch(Exception e)
        {
            e.printStackTrace();
        }
        return convertView;
    }

    private static class ViewHolder {
        TextView mTitleView;
        ImageView imageView1;
    }

}

Easy things

auto-generated comments

@Override
public int getCount() {
    // TODO Auto-generated method stub
    return data.size();
}

The method is not auto-generated any more, remove the comment. (this happens in a few places).
brace-placement

In Java, the opening brace { goes at the end of the line, not the start of a new line. Code like this:

    }
    catch(Exception e)
    {
        e.printStackTrace();
    }

should look like:

} catch(Exception e) {
    e.printStackTrace();
}

Hard Stuff

This code is likely your problem:

holder.mTitleView.setText(ITEM.get("name"));
Picasso.with(activity).load(ITEM.get("image"))
       .placeholder(R.drawable.icon).resize(150, 150)
       .error(R.drawable.icon).into(holder.imageView1);
System.gc();

There are two things here.

Firstly, System.gc() in any code that is not directly related to the system itself, is almost always a bug. There should never be a need for this. Let the system look after itself. It really does not need your help.

The second item is that it may be taking picasso a long time to load the Images. If you are scrolling to unseen areas of the system then the code may need to be calling the getView() method a lot. You need this method to be fast.

Convert the Picasso line to be an AsyncTask, and let it populate the imageView in the background. That will make populating the views a lot faster