Find the intervals which have a non-empty intersection with a given interval The problem is the following: I have a list intervals which consists of tuples of the form (start, end) [with start <= end]. Each tuple represents an interval (of the real line). We assume that the intervals in intervals are not overlapping each other. Given a new interval (s,e), I would like to write a Python function which checks if (s, e) overlaps any of the intervals in intervals. If (s, e) has a non-empty intersection with at least one of the intervals in intervals, the function should return the indices of these intervals in the list intervals.

Say that the function is called find_intersections. Then, given that intervals = [(1, 3.5), (5.5, 8.7), (10.2, 22.6), (22.7, 23.1)], expected outputs would be:

find_intersection(intervals, (3.2, 5.)) returns array([0])
find_intersection(intervals, (6.1, 7.3)) returns array([1])
find_intersection(intervals, (9.1, 10.2)) returns No intersection.
find_intersection(intervals, (5.8, 22.9)) returns array([1, 2, 3]).

The code for find_intersection I have written is:

import itertools

def find_intersection(intervals, new_interval):
    _times = sorted(list(itertools.chain.from_iterable(intervals)))
    ind = np.searchsorted(_times, np.asarray(new_interval))
    parity = np.mod(ind, 2)
    if (not np.any(parity)) and ind[1] == ind[0]:
        print('No intersection.')
    elif parity[0] == 1:
        ub = ind[1] if parity[1] == 1 else ind[1] - 1
        return np.arange((ind[0] - 1) / 2, (ub - 1) / 2 + 1)
    elif parity[1] == 1:
        lb = ind[0] if parity[0] == 1 else ind[0] + 1
        return np.arange((lb - 1) / 2, (ind[1] - 1) / 2 + 1)
    else:
        lb = ind[0] if parity[0] == 1 else ind[0] + 1
        ub = ind[1] if parity[1] == 1 else ind[1] - 1
        return np.arange((lb - 1) / 2, (ub - 1) / 2 + 1)

I believe that the code does the job.

Is there an easier/smarter way to address this problem?

Edit: this question was cross-posted on stackoverflow.com.

1. Review

There is no docstring. What does the function find_intersections do? The text in the post is pretty clear, so you could use this as the basis of a docstring. I would make it explicit that the intervals are open (if that's what you really intend), since in Python intervals are normally half-open.
There seems to be some uncertainty about whether the input intervals is supposed to be already sorted. On the one hand, if it's already sorted then there's no point in calling sorted. But on the other, if it's not sorted then returning a range of indexes is not very useful, as these are indexes into _times, not into intervals.

I recommend picking one possibility or the other: if intervals is supposed to be sorted, then don't sort it (you might assert that it is already sorted); but if not, then translate the returned indexes so that they refer to intervals.
If there is no intersection, find_intersections prints a message. But if the caller is already printing some output, it would be inconvenient (at best) to have this kind of message appear in the middle of the output. Better to leave the printing of messages (or not) up to the caller.
If there is an intersection, find_intersections returns a range of indexes. But if there are no intersection, find_intersections returns None. It is not a good idea to return an exceptional value like this, because it would be easy for the caller to forget to check for the exceptional value, and to write something like this:

result = find_intersection(intervals, new_interval)
print(f"Number of intersections = {len(result)}.")

which would fail with:

TypeError: object of type 'NoneType' has no len()

What they should have written is:

result = find_intersection(intervals, new_interval)
if result is None:
    print("Number of intersections = 0.")
else:
    print(f"Number of intersections = {len(result)}.")

but this is very long-winded. In this case a simple improvement would be to return an empty range of indexes.
If you're going to use NumPy, then you might as well use it wherever possible. So instead of itertools.chain.from_iterable, use numpy.reshape; instead of sorted, use numpy.sort; and so on.

2. Revised code if intervals are unsorted

If intervals is unsorted (as suggested by the call to sorted), then there's no point in sorting it. Sorting costs \$O(n\log n)\$ whereas comparing the query interval with every element of intervals only costs \$O(n)\$.

So in this case we can take advantage of the fact that the open interval \$(a, b)\$ overlaps with the open interval \$(c, d)\$ if and only if \$a < d\$ and \$c < b\$, and write:

def find_intersection(intervals, query):
    """Find intersections between intervals.
    Intervals are open and are represented as pairs (lower bound,
    upper bound).

    Arguments:
    intervals: array_like, shape=(N, 2) -- Array of intervals.
    query: array_like, shape=(2,) -- Interval to query.

    Returns:
    Array of indexes of intervals that overlap with query.

    """
    intervals = np.asarray(intervals)
    lower, upper = query
    return np.argwhere((lower < intervals[:, 1]) & (intervals[:, 0] < upper))

3. Revised code if intervals are sorted

If intervals is already sorted, as suggested by returning indexes into the sorted array, then take advantage of the side argument to numpy.searchsorted and the floor division operator, //:

def find_intersection(intervals, query):
    """Find intersections between intervals.
    Intervals are open and are represented as pairs (lower bound,
    upper bound).

    Arguments:
    intervals: array_like, shape=(N, 2) -- Array of intervals.
    query: array_like, shape=(2,) -- Interval to query.

    Returns:
    Array of indexes of intervals that overlap with query.

    """
    endpoints = np.reshape(intervals, -1)
    lower, upper = query
    i = np.searchsorted(endpoints, lower, side='right')
    j = np.searchsorted(endpoints, upper, side='left')
    return np.arange(i // 2, (j + 1) // 2)


Finding longest list in list of lists in Python I have the following code to find the longest list in a list of lists:

max_len = max([len(path) for path in paths])
[path for path in paths if len(path) == max_len][0]

Each path is a list of integers.  paths doesn't have to be a list.  So if I have, for example:

path_a = [1, 2, 3]
path_b = [1, 4]
path_c = [3, 5, 6, 2, 7]

Then the code should give me path_c.  In case of ties, it doesn't matter what's returned as long as it has the correct (longest) length.

Is there a more pythonic way?

Note that your code iterates over paths twice: once to calculate the max length,
and one more time to find all elements that have the max length.

It would be better to iterate only once,
keeping track of the max value, and the item that had the max value.
At the end of the iteration (single pass),
return the item that had the max value.

You can code this algorithm yourself,
and in many programming languages you would have to.
In Python,
the max built-in takes a key parameter,
to perform such "max by" operation.
The key parameter must be a one-argument function.
For each item in the collection,
the function will receive the item,
and should return a value that will be used as the rank of the item.

In your example, the rank of each item is its length.
That gives this simple pythonic code:

max(paths, key=lambda coll: len(coll))

... which gets even simpler with η-reduction to just (thanks @Gareth!):

max(paths, key=len)


js oop best practise I have to set up a new JavaScript library for a big project, which isn't Object Oriented yet. So I built a small file to test some needs. The result is the following:

var Module =
{
    Config: (function()
    {
        //- private members
        var _systemName =   'mySystem',
            _version    =   '1.1.4';

        //- private methods
        //...

        //- public methods
        var api =
        {
            getSystemInfo: function()
            {
                return "system: '"+_systemName+"' @ version "+_version;
            }
        };

        return api;
    })(),

    Dialog:
    {
        _: (function(){
            $(document).ready(function()
            {
                $('.showDialog').on('click',function()
                {
                    var obj = {
                        element:    this,
                        foo:        'foo',
                        bar:        'bar',
                        target:     $('#title')
                    };

                    Module.Dialog.setParam(obj);
                    Module.Dialog.show();
                })
            });
        })(),

        setParam: function(config)
        {
            console.log('config set for dialog box');
            console.log(config);
        },

        show: function()
        {
            console.log('dialogbox show | '+Module.Config.getSystemInfo());
        },

        close: function()
        {
            console.log('dialogbox close');
        }
    },

    Savescreen:
    {
        setParam: function(config,foo)
        {
            console.log('config set for savescreen');
        },

        show: function()
        {
            console.log('savescreen show | '+Module.Config.getSystemInfo());
        }
    }
}

Goals:

access via Module.Object.Function()
no instantiation via keyword new
private members and methods (in config section or event-listner)
easy structured and object orientated
each section must have its own scope (section = savescreen, config,
dialogbox, etc)
each section should hold its event-listner

To start off, your module should be the bare minimum, but extendable (like jQuery). That way, you won't have problems with scalability and maintenance. Here's a simple module template, where everything gets declared in the Module namespace:

//this is a simple module declaration:
(function(Module){

  //everything here is private unless attached to the Module namespace
  var imPrivate = 'cant see me!';

  //here's an example of an exposed functionality
  Module.imaPublicProperty = 'public property here';

  Module.imaPublicFunction = function(){
    console.log('im public');
  }

  //here's an example privileged methods, public methods that have access
  //to the private scope. These are your "setters and getters" in Java-speak
  Module.imaPrivilegedFunction = function(){
    console.log(imPrivate);
  }

}(this.Module = this.Module || {}));

I suggest you build a helper function that actually appends your sections to your namespace. That way, the module would scale easily. Also, you don't have to fiddle with the base-code and lose commas along the way.

Here's a simple, yet incomplete implementation using this new approach (it lacks the document.ready part). It is long, yet it's modular and easily maintainable. You can even split the section codes in separate files.

(function (ns) {

  ns.addSection = function (name, def) {
    ns[name] = new def();
  }

}(this.Module = this.Module || {}));

//adding config
Module.addSection('Config', function () {
  var systemName = 'mySystem',
    version = '1.1.4',
    section = this;

  section.getSystemInfo = function () {
    return systemName + ' v' + version;
  }

});

//adding Dialog
Module.addSection('Dialog', function () {

  var section = this;

  section.onDocumentReady = function () {
    $('.showDialog').on('click', function () {
      var obj = {
        element: this,
        foo: 'foo',
        bar: 'bar',
        target: $('#title')
      };
      section.setParam(obj);
      section.show();
    })
  }

  section.setParam = function (config) {
    console.log('config set for dialog box');
    console.log(config);
  }

  section.show = function () {
    console.log('dialogbox show | ' + Module.Config.getSystemInfo());
  }

  section.close: function () {
    console.log('dialogbox close');
  }

});

//Savescreen section
Module.addSection('Savescreen', function () {

  var section = this;

  section.setParam = function (config, foo) {
    console.log('config set for savescreen');
  }

  section.show = function () {
    console.log('savescreen show | ' + Module.Config.getSystemInfo());
  }

});

Here's how it's possibly used:

Module.Config.getSystemInfo();
//mySystem v1.1.4

So basically, you have base code and you just append to the namespace the sections via a helper function. Keeps the code clean and loose.

I suggest looking at KernelJS, which implements this pattern


Is a string a bit string New to the C language. Which design mistakes have I made in my first C program? Or what can I improve? Any little detail is appreciated.

Note: I have experience in multiple coding languages. (this is the beginning of my C journey)

Description of the program: checks if a string/array of characters is a bit string. Example: "010100" -> valid.

#include <stdio.h>
#include <string.h>

int main() {
    char str[100];
    fgets(str, 100, stdin);

    int len = strlen(str);
    int len2 = len - 1;
    int count = 0;

    //loop through string
    for(int i = 0; i < len2; i++) {
        switch (str[i]) {
            case '0':
            case '1':
                count++;
                break;
        }
    }

    //is string a bit string
    if (count == len2)
        printf("Valid!\n");
    else
        printf("Not Valid...\n");

    return 0;
}

Ultimately, you are looking to see if any character is not either '0' or '1'.  The moment you find such a character, you can break the loop immediately:

int isBitString = 1;
for(int i = 0; i < len2; i++) {
    if (str[i] != '0' && str[i] != '1') {
        isBitString = 0;
        break;
    }
}


Instantiating a run-time implementation of an interface Below is my code that was written for the following task (based on an SO question):

  Given a Java Collection (of N elements) create an ArrayList, containing N
  collections of the same type with just one element in each.

Here's my code for such method:

public static <E> ArrayList<Collection<E>> SplitCollection (Collection<E> col) throws InstantiationException, IllegalAccessException {
    ArrayList<Collection<E>> listOfCollections = new ArrayList<>();
    for(E el: col) {
        // creating an empty collection of type E
        Collection<E> colEl = col.getClass().newInstance();
        colEl.add(el);
        listOfCollections.add(colEl);
    }
    return listOfCollections;
}

Is there a better way to create a run-time instance of an implementation class for an interface?
Did I overdo anything in light of type erasure?
Other code critique?

When I decided to use no-argument constructor I was aware that the very existence of one is not (and cannot be) enforced by the interface. As it stated in the docs:

  All general-purpose Collection implementation classes (which typically
  implement Collection indirectly through one of its subinterfaces)
  should provide two "standard" constructors: a void (no arguments)
  constructor, which creates an empty collection, and a constructor with
  a single argument of type Collection, which creates a new collection
  with the same elements as its argument. In effect, the latter
  constructor allows the user to copy any collection, producing an
  equivalent collection of the desired implementation type. There is no
  way to enforce this convention (as interfaces cannot contain
  constructors) but all of the general-purpose Collection
  implementations in the Java platform libraries comply.

So I was writing my code for general-purpose Collection implementations.

I never intended this code to be used for production purposes.  I merely wanted to see how far programming to interface together with Generics could get you before you have to use a concrete type and implementation.

Two parts to this review:

the 1-liner for the newInstance()
the general mechanism of the method

newInstance()

This code will work, but only for a subset of Collections. For example, there are many Collection implementations that do not have an accessible default constructor. What if the input collection is:

Collection<String> input = Arrays.asList(new String[]{"hello","world"});

The newInstance call on the class of that List will fail.... because that is a class called java.util.Arrays$ArrayList and it is not public at all...

The 'specification' for this problem is probably contrived, and there is no expectation that it should behave well when the inputs are bad. As a contrived example, I guess contrived code is OK.

Which leads on to the next thing, what is the right thing to do?

Well, I don't think there is a right thing to do. If the input requires the support of bare 'Collection' then you have no hope.

In a real example, I would probably specify the method to allow specific known collections that the program would expect. Then not use the newInstance at all.

Alternatively, I would use the Java-8 style Supplier, and have an input that supplies you with the collection you need, and shift the burden to the user of your method. Something like:

public interface CollectionSupplier<T> {
    Collection<T> supply();
}

and then have your method declared as:

public static <E> ArrayList<Collection<E>> SplitCollection (
        final Collection<E> col, final CollectionSupplier<E> supplier) {
    .....
    Collection<E> colEl = supplier.supply();
    ....
}

That would also remove the ugly exceptions you declare to throw... as it is, I would probably (bad practice) 'try' the whole loop, and 'catch' the ugly exceptions and replace them with a wrapper of an IllegalStateException that explains that an inner Collection could not be instantiated.

General

Overall your method is neat, and well structured. I can't really fault it, given the constraints of the problem.

Update: Actually, the method should have a lower-case 's' to start the name. How did I miss that the first time? Should be splitCollection not SplitCollection

I would prefer that the method declared the input col as final:    public static <E> ArrayList<Collection<E>> SplitCollection (final Collection<E> col) because that is a habit I am in...

Typically I would recommend against returning the data typed as an ArrayList because the implementation should not be reflected, it should just be List, but, again, in this case, the specification requires an ArrayList return value.

Bonus

A Java 8 implementation of this would be a good exercise.

This is what I would try:

public static <T, U extends Collection<T>, V extends Collection<T>> ArrayList<V> splitCollection (U col, Supplier<V> supplier) {

    return col.stream().map(v -> {
           V subcol = supplier.get();
           subcol.add(v);
           return subcol;
        }).collect(Collectors.toCollection(ArrayList::new));
}

and it would be used like:

public static void main(String[] args) {
    System.out.println("Result:"
       + splitCollection(Arrays.asList("hello","world"), ArrayList::new));
}

which gives the output:

Result:[[hello], [world]]

Note, in the Java 8 example, I allow the sub collection to be any supported type that you can add the content to. So, for the above example, the input is an inaccessible list (no public default constructor), but the supplier gives you what you need.


Highlight Current Link I'm using this technique for highlighting current links (e.g. how the "Questions" link is highlighted on this very page you're looking at). I changed the code a little bit and came up with this extension method:

    public static MvcHtmlString MenuLink(
        this HtmlHelper helper,
        string content, string action, string controller)
    {
        var routeData = helper.ViewContext.RouteData.Values;
        var currentController = routeData["controller"];
        var currentAction = routeData["action"];

        var builder = new TagBuilder("li")
        {
            InnerHtml = content
        };
        if (String.Equals(action, currentAction as string,
                  StringComparison.OrdinalIgnoreCase)
            &&
           String.Equals(controller, currentController as string,
                   StringComparison.OrdinalIgnoreCase))
        {
            builder.AddCssClass("active");
            return MvcHtmlString.Create(builder.ToString());
        }
        return MvcHtmlString.Create(builder.ToString());
    }

It works fine, But that’s some ugly code. I use the extension method this way:

@Html.MenuLink("Ticket Page", "MainPage", "Ticket")

As you can see, part of that isn't strongly typed.

Do you have any ideas to improve the extension method?

One thing that I see that I would change is the if statement

    if (String.Equals(action, currentAction as string,
              StringComparison.OrdinalIgnoreCase)
        &&
       String.Equals(controller, currentController as string,
               StringComparison.OrdinalIgnoreCase))
    {
        builder.AddCssClass("active");
        return MvcHtmlString.Create(builder.ToString());
    }

I personally don't like having a conditional that spans over multiple lines,  so the first thing that I would do is to take the boolean conditions and make them into boolean variables and then use them inside the conditional.  I think it would be easier to read.

bool isCurrentAction = String.Equals(action, currentAction as string,
          StringComparison.OrdinalIgnoreCase);
bool isCurrentController = String.Equals(controller, currentController as string,
           StringComparison.OrdinalIgnoreCase);
if (isCurrentAction && isCurrentController)
{
    builder.AddCssClass("active");
    return MvcHtmlString.Create(builder.ToString());
}

And since you are returning builder immediately after the if statement, there isn't really a reason to return inside the if statement, it is rather redundant.  Just delete that from the if block

bool isCurrentAction = String.Equals(action, currentAction as string,
          StringComparison.OrdinalIgnoreCase);
bool isCurrentController = String.Equals(controller, currentController as string,
           StringComparison.OrdinalIgnoreCase);
if (isCurrentAction && isCurrentController)
{
    builder.AddCssClass("active");
}


Printing the month name for the month number This program asks a user to input any number equal to or between 1-12. It then converts the number to a message that will be printed (Copy the program to see it yourself). Is there a way to make the code shorter by still using the switch statement?

This is homework, so using a switch is a requirement.

import javax.swing.JOptionPane;

public class NumOfMonth {

   public static void main(String[] args) {

    int num = Integer.parseInt (JOptionPane.showInputDialog ("Enter any number equal to or between 1-12 to display the month"));

    switch (num)
    {
    case 1:
        System.out.println ("The name of month number 1 is January");
        break;
    case 2:
        System.out.println ("The name of month number 2 is February");
        break;
    case 3:
        System.out.println ("The name of month number 3 is March");
        break;
    case 4:
        System.out.println ("The name of month number 4 is April");
        break;
    case 5:
        System.out.println ("The name of month number 5 is May");
        break;
    case 6:
        System.out.println ("The name of month number 6 is June");
        break;
    case 7:
        System.out.println ("The name of month number 7 is July");
        break;
    case 8:
        System.out.println ("The name of month number 8 is August");
        break;
    case 9:
        System.out.println ("The name of month number 9 is September");
        break;
    case 10:
        System.out.println ("The name of month number 10 is October");
        break;
    case 11:
        System.out.println ("The name of month number 11 is November");
        break;
    case 12:
        System.out.println ("The name of month number 12 is December");
        break;
        default:
            System.out.println ("You have entered an invalid number");
        }
    } // main method
}

As you have probably noticed, there is a lot of repetition which we can take advantage of. We can declare an array containing all the string representations of the months like so.

String[] months = {"January", "February", "March", ..., "December"};

Once you have that, we can simply do:

if (num >= 1 && num <= 12) {
    System.out.format("The name of month number %d is %s\n", num, months[num-1]);
} else {
    System.out.println("You have entered an invalid number");
}

Edit: sorry, just saw the bit about "by still using the switch statement". I'll still leave this here though because I think it's a better solution than a switch statement.

If you really insist on using the switch (but you shouldn't),
then you can at least extract the repeated string part and formatting logic to a helper method:

private static void printMonth(int num, String name) {
    System.out.printf("The name of month number %d is %s%n", num, name);
}

And then rewrite the switch using that method:

switch (num)
{
case 1:
    printMonth(num, "January");
    break;
case 2:
    printMonth(num, "February");
    break;
// .... and so on

But really, insisting on a switch here is forced,
and goes against good practices.


Fetching ALL data set from API iterating via response data Does this make sense? How would you achieve the same thing, with cleaner code?

function retrieveBatch(offset){
  var opts = {}
  opts.limit = 100
  if(offset) opts.offset = offset
  return new Promise(function(resolve, reject) {
    chargebee.subscription
    .list(opts)
    .request(function(err, value){
      if(err) return reject(err)
      return resolve(value)
    })
  })
}

function retireveAll(){
  var subscriptions = []
  function inception(data){
    if(!data) return retrieveBatch().then(inception)
    if(data && data.list) subscriptions.push(data.list)
    if(data && data.next_offset) return retrieveBatch(data.next_offset).then(inception)
    if(data && !data.next_offset) return _.flatten(subscriptions)
  }
  return Promise.resolve(inception(false))
}

I only have some minor additions, maybe a little syntax sugar. I've tested most of it in the JavaScript console, and it still needs at least some JSBin trials before going live.

function retrieveBatch(offset){
  var opts = {
    limit  : 100,
    offset : offset || undefined
  };

  return new Promise(function(resolve, reject){
    chargebee.subscription
      .list(opts)
      .request(function(err, value){
        // return only if err is defined
        err && return reject(err);
        // if you got this far, there's only one way out
        return resolve(value);
      })
  })
}

function retrieveAll(){
  var subscriptions = [];

  function inception(data){
    if(!data){
      return retrieveBatch().then(inception);
    }
    if(data.list){
      subscriptions.push(data.list);
    }
    if(data.next_offset){
      return retrieveBatch(data.next_offset).then(inception);
    }

    return _.flatten(subscriptions);
  }
  return Promise.resolve(inception(false))
}


Upvote distribution SEDE query There is a 1-to-many relationship between the posts table and the votes table, and they join on posts.id = votes.postid. I would like to know how many posts have 1, 2, 3, 5, etc. votes.

Thanks to Waffles' most controversial posts query, it was trivial to write this:

declare @VoteStats table (PostId int, up int)

set nocount on

insert @VoteStats
select
    PostId,
    up = sum(case when VoteTypeId = 2 then 1 else 0 end)
from Votes
where VoteTypeId in (2,3)
group by PostId

set nocount off

select up, count(*)
from @VoteStats
group by up;

However, it runs in a full 22 seconds on the Stack Overflow data dump.

Am I overlooking an obvious way to speed this query up, perhaps avoiding the creation of  the VoteStats table?

Not creating the VoteStats table makes a big difference. Gets it down to ~5.5 seconds.

set nocount on

select up, count(*)
from (
    select
        PostId,
        up = sum(case when VoteTypeId = 2 then 1 else 0 end)
    from Votes
    where VoteTypeId in (2,3)
    group by PostId
) a
group by up
order by up

http://data.stackexchange.com/stackoverflow/q/112108/

From the execution plan of the table variable version, it looks like the table scan and the sort on it is pretty expensive.


Implementing Binary Tree using Queue in C# I am in the process of learning data structures and I am trying to implement Binary Tree using Queue in C#.

Below are my Tree implementation classes.

public class Tree
{
    public char Data { get; set; }
    public Tree LChild { get; set; }
    public Tree RChild { get; set; }
}

public class TreeImplementation
{
    //Initialize tree nodes with data.
    Tree _rootNodeF = new Tree {Data = 'F'};

    Tree _lChildNodeD = new Tree { Data = 'D' };
    Tree _rChildNodeJ = new Tree { Data = 'J' };

    Tree _lChildNodeB = new Tree { Data = 'B' };
    Tree _rChildNodeE = new Tree { Data = 'E' };

    //Initialize an empty queue to use for tree traversal.
    Queue<Tree> _treeQueue = new Queue<Tree>();

    public  TreeImplementation()
    {
        _rootNodeF.LChild = _lChildNodeD;
        _rootNodeF.RChild = _rChildNodeJ;

        _lChildNodeD.LChild = _lChildNodeB;
        _lChildNodeD.RChild = _rChildNodeE;

    }

    public void LevelOrder()
    {
            //Add root node to the queue.
            _treeQueue.Enqueue(_rootNodeF);

            //Create tempNode to add next node to the queue.
            Tree tempNode = new Tree();

            while (_treeQueue.Count != 0)
            {
                tempNode = _treeQueue.Dequeue();

                VisitedTreeNode(tempNode.Data);

                if (tempNode.LChild == null) continue;
                _treeQueue.Enqueue(tempNode.LChild);

                if (tempNode.RChild != null)
                {
                    _treeQueue.Enqueue(tempNode.RChild);
                }
            }
    }

    //Access the data at the node.
    public void VisitedTreeNode(char queueData)
    {
        Console.WriteLine(queueData);
    }

}

I am calling the LevelOrder() method in the main method in another class.

How can I improve my code?

As a first step, TreeImplementation can create the tree in the constructor and only keep a reference to the root node. Also the queue is only used in LevelOrder and should be moved there.

public class TreeImplementation
{
    Tree _rootNodeF;

    public TreeImplementation()
    {
        _rootNodeF = new Tree {
            Data = 'F',
            LChild = new Tree {
                Data = 'D',
                LChild = new Tree { Data = 'B' },
                RChild = new Tree { Data = 'E' }
            },
            RChild = new Tree { Data = 'J' }
        };
    }

    public void LevelOrder()
    {
        //Initialize an empty queue to use for tree traversal.
        Queue<Tree> _treeQueue = new Queue<Tree>();
        .
        .
        .
    }
}

To make sure future tree manipulations doesn't cause a bug in LevelOrder(), I'd just remove the mini-optimization of the continue statement. Also, checking if the queue has Any() elements is semantically better than checking if Count() != 0. tempNode isn't used outside the while loop and is initialized with an object that is never used. Move the declaration inside the loop instead.

LevelOrder() is actually a tree walker method. I'd rename it to ForEach() instead and pass in an Action<char> it could call instead of the hard coded method VisitedTreeNode(). The walker could then be reused easier.

Now LevelOrder() has been replaced with:

    public void ForEach(Action<char> action)
    {
        Queue<Tree> _treeQueue = new Queue<Tree>();
        _treeQueue.Enqueue(_rootNodeF);

        while (_treeQueue.Any())
        {
            var tempNode = _treeQueue.Dequeue();

            action(tempNode.Data);

            if (tempNode.LChild != null)
            {
                _treeQueue.Enqueue(tempNode.LChild);
            }

            if (tempNode.RChild != null)
            {
                _treeQueue.Enqueue(tempNode.RChild);
            }
        }
    }

.. and can be called with treeImplementation.ForEach((queueData) => Console.WriteLine(queueData));

Implementing the IEnumerable interface

It could probably be userful to manipulate Tree structures with the built in methods of C#, such as foreach() or any Linq calls. To do this, you need to implement the IEnumerable interface in Tree:

public class Tree : IEnumerable<char>
{
    public char Data { get; set; }
    public Tree LChild { get; set; }
    public Tree RChild { get; set; }

    IEnumerator IEnumerable.GetEnumerator()
    {
        return this.GetEnumerator();
    }

    public IEnumerator<char> GetEnumerator()
    {
        yield return Data;
        if (LChild != null)
        {
            foreach (var data in LChild)
            {
                yield return data;
            }
        }
        if (RChild != null)
        {
            foreach (var data in RChild)
            {
                yield return data;
            }
        }
    }
}

Of course this implementation totally makes ForEach above redundant, as it now can be replaced with:

    public void ForEach(Action<char> action)
    {
        foreach (var data in _rootNodeF)
            action(data);
    }

Generics

As a final thought, Tree is a pretty generic construct and would be a lot more useful as:

public class Tree<T> : IEnumerable<T>
{
    public T Data { get; set; }
    public Tree<T> LChild { get; set; }
    public Tree<T> RChild { get; set; }

    IEnumerator IEnumerable.GetEnumerator()
    {
        return this.GetEnumerator();
    }

    public IEnumerator<T> GetEnumerator()
    {
        yield return Data;
        if (LChild != null)
        {
            foreach (var data in LChild)
            {
                yield return data;
            }
        }
        if (RChild != null)
        {
            foreach (var data in RChild)
            {
                yield return data;
            }
        }
    }
}

Creating your specific tree is the done with

var myTree = new Tree<char>
{
    Data = 'F',
    LChild = new Tree<char>
    {
        Data = 'D',
        LChild = new Tree<char> { Data = 'B' },
        RChild = new Tree<char> { Data = 'E' }
    },
    RChild = new Tree<char> { Data = 'J' }
};

foreach(var data in myTree)
{
    Console.WriteLine(data);
}


Given a series of characters from stdin, print their squared integer ASCII counterparts This program in essence is extremely simple, however, it can be written in many different ways. I'm a beginner at C and I've written it in 5 different variations (mostly differing in the use of getchar over scanf, while over do while loops..).

Apart from readability and/or maintainability, is there any difference between these 5 code blocks (such as perhaps performance or efficiency, if we disregard the use of getchar over scanf)?

unsigned char d;

while (1)
    while(scanf("%c", &d), d != '\n') {
        printf("%d\n", d * d);
    }

do {
    scanf(" %c", &d);
    printf("%d\n", d * d);
} while (1);

while (1)
    while((d = getchar()) != '\n') { //could also be written as while(d = getchar(), d != '\n')
        printf("%d\n", d * d);
    }

while ((d = getchar())) {
    if(d == '\n')
        continue;
    printf("%d\n", d * d);
}

do {
    d = getchar();
    if(d == '\n')
        continue;
    printf("%d\n", d * d);
} while (1);

while (1)
    while(scanf("%c", &d), d != '\n') {
        printf("%d\n", d * d);
    }

It's better to consistently add braces around the body of statements like while and if.
A common mistake is to write something like this:

while (1)
    while(scanf("%c", &d), d != '\n') {
        printf("%d\n", d * d);
    }
    printf("...");

This may mislead a reader to think that the last printf is part of the outer loop, when it isn't. Just put the braces around the body always.

Another negative point in this version is having two statements inside the inner while. This is not easy to read. It's better to write in a way that there is no more than one statement per line. More generally, I recommend to avoid the , operator entirely.

The same point applies to this line in the 3rd version:

while((d = getchar()) != '\n') {

That is, there is an assignment within the conditional statement.
Avoid such "clever" writing style.
Stick to one statement per line.

And one more variation of the same issue, in the 4th version:

while ((d = getchar())) {


Beating memcmp in C++ Once again I decided to beat system memcmp function. This time I decided to use a template and to "precompile" all cases from 0 to 31 bytes.

Result is 400% improvement - from about 1:15 min to 0:25 min.

Finally I had rewritten memcmp_fixed_ with naive looking for statement and I noticed that the compiler can optimize it as well.

However I did not tested with random data, so I am not sure what role the cache line and branch predictor plays in the tests.

Here is the code:

#include <cstdint>
#include <cstring>

namespace{

template<size_t SIZE>
int memcmp_fixed_(const unsigned char *s1, const unsigned char *s2){
    for(size_t i = 0; i < SIZE; ++i)
        if (s1[i] != s2[i])
            return s1[i] - s2[i];

    return 0;
}

template<>
int memcmp_fixed_<1>(const unsigned char *s1, const unsigned char *s2){
    return *s1 - *s2;
}

template<size_t SIZE>
int memcmp_fixed_(const void *a1, const void *a2){
    const unsigned char *s1 = (const unsigned char *) a1;
    const unsigned char *s2 = (const unsigned char *) a2;

    return memcmp_fixed_<SIZE>(s1, s2);
}

}

inline int fast_memcmp(const void *a1, const void *a2, size_t const size){
    switch(size){
        case  0: return 0;

        case  1: return memcmp_fixed_< 1>(a1, a2);
        case  2: return memcmp_fixed_< 2>(a1, a2);
        case  3: return memcmp_fixed_< 3>(a1, a2);
        case  4: return memcmp_fixed_< 4>(a1, a2);
        case  5: return memcmp_fixed_< 5>(a1, a2);
        case  6: return memcmp_fixed_< 6>(a1, a2);
        case  7: return memcmp_fixed_< 7>(a1, a2);
        case  8: return memcmp_fixed_< 8>(a1, a2);
        case  9: return memcmp_fixed_< 9>(a1, a2);
        case 10: return memcmp_fixed_<10>(a1, a2);
        case 21: return memcmp_fixed_<21>(a1, a2);
        case 22: return memcmp_fixed_<22>(a1, a2);
        case 23: return memcmp_fixed_<23>(a1, a2);
        case 24: return memcmp_fixed_<24>(a1, a2);
        case 25: return memcmp_fixed_<25>(a1, a2);
        case 26: return memcmp_fixed_<26>(a1, a2);
        case 27: return memcmp_fixed_<27>(a1, a2);
        case 28: return memcmp_fixed_<28>(a1, a2);
        case 29: return memcmp_fixed_<29>(a1, a2);
        case 30: return memcmp_fixed_<30>(a1, a2);
        case 31: return memcmp_fixed_<31>(a1, a2);

        default: return memcmp(a1, a2, size);
    }
}

#include <cstdio>

#include <algorithm>    // min

size_t const MAX = 10000000000;

int main(int argc, char **argv){
    if (argc != 3){
        printf("Usage:\n");
        printf("\t%s [string1] [string2]\n", argv[0]);
        return 1;
    }

    const char *s1 = argv[1];
    const char *s2 = argv[2];

    size_t const size1 = strlen(s1);
    size_t const size2 = strlen(s2);
    size_t const size  = std::min(size1, size2);

    volatile int x = 0;
    for(volatile size_t i = 0; i < MAX; ++i)
        x += fast_memcmp(s1, s2, size);

    printf("%d %d\n", fast_memcmp(s1, s2, size), x );
}

And the baseline is this:

#include <cstdint>
#include <cstring>

#include <cstdio>

#include <algorithm>    // min

size_t const MAX = 10000000000;

int main(int argc, char **argv){
    if (argc != 3){
        printf("Usage:\n");
        printf("\t%s [string1] [string2]\n", argv[0]);
        return 1;
    }

    const char *s1 = argv[1];
    const char *s2 = argv[2];

    size_t const size1 = strlen(s1);
    size_t const size2 = strlen(s2);
    size_t const size  = std::min(size1, size2);

    volatile int x = 0;
    for(volatile size_t i = 0; i < MAX; ++i)
        x += memcmp(s1, s2, size);

    printf("%d %d\n", memcmp(s1, s2, size), x );
}

Since we're using the C++ wrappers around standard C functions (which is good - I like it!), we need namespace qualification of std::size_t and std::memcmp).  Although your implementation evidently takes advantage of the license to also include the unqualified names, it's not portable to rely on that.

Instead of repeating the type when casting a1 to s1 and a2 to s2, we can just use auto (and let's be clear about the cast - prefer reinterpret_cast to a catch-all C-style cast).

I managed to simplify memcmp_fixed_ by using a recursive template.  For me (with g++ -03), this gave roughly the same execution speed (I guess that loop unrolling makes the resultant binary very similar).  I got rid of the separate void* and unsigned char* overloads - that's just a compile-time overhead and makes no different to runtime speed, which gives 9 lines, compared to your 14 (counting physical lines that contain alphanumerics).

template<std::size_t SIZE>
int memcmp_fixed_(const void *a1, const void *a2)
{
    auto const s1 = reinterpret_cast<const unsigned char*>(a1);
    auto const s2 = reinterpret_cast<const unsigned char*>(a2);
    auto const diff = *s1 - *s2;
    return diff ? diff : memcmp_fixed_<SIZE-1>(s1+1, s2+1);
}

template<>
int memcmp_fixed_<0>(const void*, const void*)
{
    return 0;
}

I slightly simplified the test program to make it a single-character change to build the baseline test (and to send error messages where they belong), and massively reducing the repeated code:

#if 1
#define test_memcmp fast_memcmp
#else
#define test_memcmp std::memcmp
#endif

int main(int argc, char **argv)
{
    if (argc != 3) {
        std::fprintf(stderr, "Usage:\n\t%s [string1] [string2]\n", argv[0]);
        return 1;
    }

    const char *s1 = argv[1];
    const char *s2 = argv[2];
    auto const size  = std::min(strlen(s1), strlen(s2));

    volatile int x = 0;
    for (volatile std::size_t i = 0;  i < MAX;  ++i)
        x += test_memcmp(s1, s2, size);

    std::printf("%d %d\n", test_memcmp(s1, s2, size), x);
}


Tic Tac Toe in Python practice 2 The previous code on codereview
I'm quite novice here so apologies for any silly mistake in advance
I've been writing a simple tic tac toe game which is a part of my course in udemy
Since this is my first project, I want to do my best in order to learn new things besides learning how to code better. Generally, I want to optimize my code as much as possible.
Can anyone help me with this?
# Tic Tac Toe
# 17 July 2020

import os

test_board = [' ', ' ', ' ', ' ', ' ', ' ', ' ', ' ', ' ']
player_input_num = 0    # numbers of inputs entered by player
player_num = 0
won = False
marker = 'X'

def clear_screen():
    os.system("clear")

def display_board(board_cords):
    '''
    display the board
    board_cords = list(left to right from top to bottom)
    '''
    line = '-'*40
    for counter in range(0, len(board_cords), 3):
        print("\t|\t\t|")
        print(
            f"{board_cords[counter]}\t|\t{board_cords[counter+1]}\t|\t{board_cords[counter+2]}")
        print(line)
        counter += 3    # go to next row

def check_player_input(number):
    if number < 10 and number >= 0:  # checking the range
        return True
    else:
        print('Sorry the input is not in range [0-9] .')
        return False

def player_input(player_in):
    global player_input_num  # access to player_input_num
    if player_input_num >= 2:
        # check if the position is free to use
        if check_capacity(test_board, player_in):
            result = place_marker(test_board, 'X', player_in)
            clear_screen()
            display_board(result)

            if check_win(test_board, marker):
                print("You Won!")
                global won
                won = True
        else:
            print("The current position is occupied.")
    else:
        if check_capacity(test_board, player_in):
            result = place_marker(test_board, 'X', player_in)
            clear_screen()
            display_board(result)
            player_input_num += 1
        else:
            print("The current position is occupied.")

def check_capacity(board, position):
    '''
    Check if the current position is free to use.
    '''
    return board[position] == ' '

def place_marker(board, marker, position):
    '''
    Replace the position with a marker
    '''
    board[position] = marker
    return board

def check_win(board, marker):
    '''
    Check if the current game is finished
    '''
    if board[0] == board[1] == board[2] == marker:
        return True
    if board[0] == board[3] == board[6] == marker:
        return True
    if board[0] == board[4] == board[8] == marker:
        return True
    if board[2] == board[5] == board[8] == marker:
        return True
    if board[6] == board[7] == board[8] == marker:
        return True
    if board[2] == board[4] == board[6] == marker:
        return True
    return False

def wanna_play():
    answer = input("Wanna play again? (Y or N)")
    return answer

# Main
display_board(test_board)
while True:
    while won == False:
        try:  # checking if input is int
            player_num = int(input("Enter a position: "))
        except:
            print("Input is not a number")
            continue

        if check_player_input(player_num):
            player_input(player_num)

    if wanna_play() in ['y', 'Y', 'n', 'N']:
        print("something")
    else:
        print("Invalid input.")
        # print("Thanks for playing :)")

Note that code is working, I just want to know other ways of doing it and optimize it overalls.

Instead of using indices from 0 to 9, change the way you store your board to be a 3x3 list.
board = list([[0] * 3] * 3)
"""
1 | 2 | 3
4 | 5 | 6
7 | 8 | 9
"""

This will allow you to come back to it and add a GUI.
It also becomes easier for anyone reading the code to understand what's happening visually.
Next is your printout of the board. Join will be your friend here.
for row in board:
  printout = "\t|\t".join(str(element) for element in row)
  print(printout) # this can be reduced to a single line if you want.

ranges
range() accepts 3 variables. START, STOP and INCREMENT AMOUNT
for i in range(0, 9, 3):

You can also reverse with negative values
for i in range(9, 0, -1):

or
for row in board[::-1]:

if statements
In math class you know how you were able to write a < x > b? You can do that in python without requiring and.
if 0 <= number > 10:

Then there is checking the board state if someone has won.
Your current approach requires a lot of coding and can't be expanded on quickly
There are a few different approaches you can do with this. All will "work."
First the rows
for row in board:
  if len(set(row)) == 1:
    return True

for row in board:
  if all(element == row[0] for element in rows):
    return True

Diagonals next (I would use a tempory dummy variable for len(board) to increase readability. -> _ = len(board)
if len(set(board[i][i] for i in range(len(board)) == 1:
  return True
if len(set(board[i][len(board)-i-1] for i in range(len(board)) == 1:
  return True

The columns can be a trickier but transposing would probably be the most pythonic way to approach it.
board_T = [list(column) for column in zip(board)]

This will allow you to call the rows again and check to see if someone won.
It should be noted that the return values for the winner state could return the PLAYER of rather than True or False. Python sees 0, None, Empty iterables, and False as False in an if statement.
I also noticed you didn't follow a lot of the advice given to you in the original post. You are still lacking an if __name__ == "__main__": statement
def playTicTacToe():
   display_board(test_board)
   ...

...
if __name__ == "__main__":
  playTicTacToe()


Calculating overlap of segments in chromosome data I wrote an R code that basically performs 2 operations:

For each segment in file A, find all segments in file B that lie in that segment.
Find the percentage of overlap for each case in previous point.

The code that I have written works fine but given I have 74 files of type A and the rows in file B are about 200K, the approximate run time for this on my cluster is about ~18 days! I am writing the code and input files below and would really appreciate your comments on how I can improve the run time, with code if possible.

File A

Chromosome    Start       End
1         0           2420037
1         2420037     2522634
1         2522634     10794763
1         10794763    10925915
1         10925915    11280057
...

File B

chr       start   end     variantaccession    variantsubtype
1     10001   127330  nsv7879             Gain+Loss
1     10001   846808  dgv2n71             Gain
1     10377   177417  dgv1e1              Complex
1     10377   177417  nsv428112           Gain
1     10377   1018704 dgv3e1              Complex
1     10499   177368  esv27265            Gain+Loss
...

A lot of rows from file B will map to each row of file A. Once my code finds indices of mapped rows from file B it also extracts certain other values part of additional columns of file B.

setwd("/abc/xyz")

filenames <- basename(list.files(path=getwd())) # reading list of all files of type A

# Reading File B
ref <- read.table("xyz.txt", header=TRUE,sep="\t")

for (i in 1:length(filenames))
{
  # Reading file
  file <- read.table(filenames[i],header=TRUE,sep="\t")
  cat("Working with ",filenames[i], " i.e. ",i,"/",length(filenames),"\n")
  # Start the clock!
  ptm <- proc.time()
  for(j in 1:nrow(file))
  {
    cat(j,"\n")
    ind <- which(file$Chromosome[j] == ref$chr)
    ref2 <- ref[ind,]
    B <- ""
    C <- ""
    D <- ""
    count <- 0
    for(k in 1:length(ind))
    {
      if((ref2$start[k]>=file$Start[j] & ref2$start[k]<=file$End[j]) | (ref2$end[k]>=file$Start[j] & ref2$end[k]<=file$End[j]))
      {
        B[k] <- as.character(ref2$variantaccession[k])
        C <- as.integer(append(C,ref2$start[k]:ref2$end[k]))
        D[k] <- as.character(ref2$variantsubtype[k])
        count <- count+1
      }
    }
    file$CNVcount[j] <- count
    C <- C[2:length(C)]
    E <- file$Start[j]:file$End[j]
    file$PerCNVoverlap[j] <- (length(E)/length(which(C %in% E)))*100
    B2 <- paste(unique(B[complete.cases(B)]),collapse=",")
    B3 <- paste(B2,",",sep="")
    file$CNVs[j] <- B3
    D2 <- paste(unique(D[complete.cases(D)]),collapse=",")
    D3 <- paste(D2,",",sep="")
    file$CNVtypes <- D3
  }
  # Stop the clock
  proc.time() - ptm
  file <- file[,c(1,2,3,4,5,6,8,7)]
  write.table(file,filenames[i],col.names=TRUE,row.names=FALSE,sep="\t")
}

How I am calculating overlap:

  File A, row 01: 1 2 3 4 5 6 7 8 9 10
  File B, seg 01:   2 3 4
  File B, seg 32:         5 6 7
  File B, seg 12:     3 4 5 6 7 8 9
Consensus File B:   2 3 4 5 6 7 8 9
Percentage overlap = 8/10 -> 80%

Required Output (i.e add additional columns to File A)

Chromosome    Start       End    Overlap_Count     Overlap_Percentage     Overlaps_Names     Overlap_subtype
1         0           2420037    800             22.12%            nsv7879,dgv1e1...    Gain+Loss,Complex
1         2420037     2522634    626             35.12%            nsv7879,dgv1e1...    Gain+Loss,Complex
1         2522634     10794763    200             17.12%            nsv7879,dgv1e1...    Gain+Loss,Complex
1         10794763    10925915    75             42.12%            nsv7879,dgv1e1...    Gain+Loss,Complex
1         10925915    11280057    800             22.12%            nsv7879,dgv1e1...    Gain+Loss,Complex

I read the data in

a <- read.delim(textConnection("Chromosome\tStart\tEnd
1\t0\t2420037
1\t2420037\t2522634
1\t2522634\t10794763
1\t10794763\t10925915
1\t10925915\t11280057"))

b <- read.delim(textConnection("chr\tstart\tend\tvariantaccession\tvariantsubtype
1\t10001\t127330\tnsv7879\tGain+Loss
1\t10001\t846808\tdgv2n71\tGain
1\t10377\t177417\tdgv1e1\tComplex
1\t10377\t177417\tnsv428112\tGain
1\t10377\t1018704\tdgv3e1\tComplex
1\t10499\t177368\tesv27265\tGain+Loss"))

and then used the GenomicRanges package to efficiently find overlaps between the query "A" and subject "B".

library(GenomicRanges)
A <- makeGRangesFromDataFrame(a)
B <- makeGRangesFromDataFrame(b, keep.extra.columns=TRUE)
olaps <- findOverlaps(A, B)

olaps is basically a two-column matrix, indicating which element(s) of A overlap the corresponding elements of B. In the invocation above A and B enter symmetrically, but could be re-ordered if there were restrictions on the type of overlap (argument type, e.g., all of B strictly within A). To find the extent of overlap, I looked at the parallel (element wise) intersection of each overlap

> isect <- pintersect(A[queryHits(olaps)], B[subjectHits(olaps)])
> isect
GRanges with 6 ranges and 0 metadata columns:
      seqnames           ranges strand
         <Rle>        <IRanges>  <Rle>
  [1]        1 [10001,  127330]      *
  [2]        1 [10001,  846808]      *
  [3]        1 [10377,  177417]      *
  [4]        1 [10377,  177417]      *
  [5]        1 [10377, 1018704]      *
  [6]        1 [10499,  177368]      *
  ---
  seqlengths:
    1
   NA

I could then create a data.frame with whatever information I want, e.g.,

data.frame(query=queryHits(olaps), subject=subjectHits(olaps),
           olap_width=width(isect),
           query_width=width(A)[queryHits(olaps)],
           variantaccession=B$variantaccession[subjectHits(olaps)])

This will be fast for millions of records in A and B.

makeGRangesFromDataFrame is available in the current release version of the package, installed by default when using R-3.1. It's easy to make a GRanges 'by hand', e.g.,

A <- with(a, GRanges(Chromosome, IRanges(Start, End)))
B <- with(b, GRanges(chr, IRanges(start, end), variantaccession=variantaccession))


Counting matches won by teams This script counts how many matches were won by the team with a first half, second half, and full game time of possession differential advantage.

The data is stored in a pandas data frame:

   Points  H1  H2  Total
0      36  46  50     96
1      -5  16  18     34
2      47  12  65     77
3      12  79  13     92
4      -7  53 -64    -11
5      36  -3 -55    -58
6       9  27  87    114
7      17  66 -10     56

The script counts matches won by a team with a positive time of possession differential as well as matches lost by a team with a negative time of possession differential.

From the above example data, matches 0,2,3,4,6,7 would be selected for total wins because the team with the possession advantage won or the team with the possession disadvantage lost.

Match 1 was lost by the team with more total possession. Match 5 was won by the team with less total possession.

This snippet finds the winners for first half, second half, and the total match time.

total_win = ((poss_diff['Points'] > 0) & (poss_diff['Total'] > 0) | (poss_diff['Points'] < 0) & (poss_diff['Total'] < 0))
h1_win = ((poss_diff['Points'] > 0) & (poss_diff['H1'] > 0) | (poss_diff['Points'] < 0) & (poss_diff['H1'] < 0))
h2_win = ((poss_diff['Points'] > 0) & (poss_diff['H2'] > 0) | (poss_diff['Points'] < 0) & (poss_diff['H2'] < 0))
print len(poss_diff[total_win])
print len(poss_diff[h1_win])
print len(poss_diff[h2_win])

Is there a more Pythonic way to calculate and store the results using lambda, applymap, or another function?

condition a > 0 and b > 0 or a < 0 and b < 0 often can be write as a * b > 0 (it depends on multiply cost)
maybe you can use it something as total_win = ( poss_diff.Points * poss_diff.Total > 0 )
len(poss_diff[total_win]) can be translate to total_win.sum()


Builder Pattern for Car class I have some experience in Java. I am reading Effective Java by Joshua Bloch, the book talks about Builder pattern which I was unaware of. So,I have assembled a quick example of what I think builder pattern is all about. Is my implementation correct? Are there any gotchas in Builder pattern?

/* Object to be built using builder */

class Car{
 public String wheels;
 public String color;
 public int speed;
}

class CarBuilder{
 private Car car = new Car();

 /* method to get car object */
 public Car getCar(){
  return car;
 }

 public CarBuilder setWheels(String brand){
  car.wheels = brand;
  return this;
 }

 public CarBuilder setColor(String color){
  car.color = color;
  return this;
 }

 public CarBuilder setSpeed(int speed){
  car.speed = speed;
  return this;
 }

 public Car build(){
  return car;
 }

}

public class builder{
 public static void main(String args[]){
  CarBuilder cb = new CarBuilder();
  Car newcar = cb.setColor("Red").setSpeed(180).setWheels("MRF").build();
  System.out.println("Your new car is "+newcar.color+" in color");
 }
}

There are three key features of the builder pattern that need to exist before the pattern becomes recognizable.

the class you are tying to build should have a lot of properties/fields that you would normally want to pass as a constructor
you want those properties to be 'immutable' - constant for the life of the car (and this implies that there should not be 'setter' methods for those values)
the class is constructed from many places so having a more convenient system is... convenient

In your case, you have three parameters, the color, wheels, and speed for the car. These things can't be changed after the Car is built. You want a Car that has getColor(), but not setColor(...).

The non-builder way to do this would be to have a single constructor that takes all the parameters:

public Car(String color, String wheels, int speed) {
    ....
}

and then call that constructor from everywhere. If there are a lot more parameters though, it gets messy to call the constructor:

Car mine = new Car(view.getCarColorDropDown().getSelectedValue(), view.getCarWheelsOptionRadioButton().getSelectedValue(), view.getCarSpeedSpinner().getValue());

Now imagine if you also want to select an interior color, automatic, or manual transmission, etc.

What happens when you have a lot of inputs is that it becomes hard to 'align' the values with the parameters of the class.... you have to cross reference them, and count the values to make sure you are putting the color in to the color parameter, etc.

Finally, what if you want some default values as well... or you need to validate combinations of choices.... like, you cannot select wheels 'abc' and not have speed > 150? Validating that all in the constructor is ... hard.

The builder class is the solution to this. It makes the construction of the Car class a sequential process... you add the data to a builder, it accumulates, validates, and isolates the inputs until you have captured all the information you need. Making sure the right parameters are given the right values is easy because the method has just one argument, and a logical name: setColor(view.getCarColorDropDown().getSelectedValue());. Then, the builder organises the data in to a single call to the constructor (often private) of the Car class.

It is common to make the Builder class a sub-class of the class that is built, too.

Putting this all together, the builder has special permissions to access the Car constructor, the builder accumulates and validates the inputs in to changable values. It then calls the ugly constructor itself, creating a single, immutable class instance, and only one place in the code has to have the ugliness.

public class Car {

    public static Builder builder() {
        return new Builder();
    }

    // static inner class builder
    public static class Builder {
        private String builderColor = "white"; // default color
        private String builderWheels = "round"; // default wheels
        private int builderSpeed = 100; // default speed

        private Builder() {
            // inaccessible constructor, does nothing
        }

        public String getColor() {
            return builderColor;
        }

        public void setColor(String color) {
            // validate color....
            this.builderColor = color;
        }

        public .... //  more setters/getters

        public Car build() {
            return new Car(builderColor, builderWheels, builderSpeed);
        }
    }

    // final (immutable) fields.
    private final String color;
    private final String wheels;
    private final int speed;

    // private constructor, only the builder calls this....
    private Car(String color, String wheels, int speed) {
        this.color = color;
        this.wheels = wheels;
        this.speed = speed;
    }

    public String getColor() {
        return color;
    }

    .... // other getters

}

Now, how does one use the builder? Like this:

Car.Builder builder = Car.builder();

builder.setColor("red");
builder.setWheels("rwd");
builder.setSpeed(200);

Car mine = builder.build();

An often overlooked feature of a builder is that it is reusable as well, so, the above code can create many cars (it is a template of sorts)....

Car[] productionLine = new Car[100];
for (int i = 0; i < productionLine.length; i++) {
    productionLine[i] = builder.build();
}

Or, you can change just small factors, like the VIN number... :

int vin = 123456000;
for (int i = 0; i < productionLine.length; i++) {
    builder.setVIN(vin++);
    productionLine[i] = builder.build();
}


Find, on a sequence of size L, the number that appears more than L/2 times Yes, yet another challenge on CodeEval!

This one has the purpose of finding the major element of a sequence: the element which appears in a sequence of size L more than L/2 times. I have this:

import java.io.BufferedReader;
import java.io.File;
import java.io.FileReader;
import java.io.IOException;
import java.util.Arrays;

public class Main {
    public static void main (String[] args) throws IOException {
        File file = new File(args[0]);
        BufferedReader buffer = new BufferedReader(new FileReader(file));
        String line;
        String[] numbersRaw;
        int[] numbers;
        while ((line = buffer.readLine()) != null) {
            line = line.trim();
            numbersRaw = line.split(",");
            numbers = new int[numbersRaw.length];
            for(int i = 0; i < numbersRaw.length; i++){
                numbers[i] = Integer.valueOf(numbersRaw[i]);
            }
            System.out.println(mostFrequentElement(numbers));
        }
    }

    private static String mostFrequentElement(int[] arr) {
        Arrays.sort(arr);
        int counter = 0, big = -1, bigCounter = -1;
        for (int i = 0; i < arr.length - 1; i++) {
            if (arr[i] == arr[i + 1]) {
                counter++;
            }
            else {
                counter++;
                if (counter > bigCounter) {
                    big = arr[i];
                    bigCounter = counter;
                }
                counter = 0;
            }
        }
        if(bigCounter > arr.length / 2) {
            return big + "";
        }
        return "None";
    }
}

I have this solution, with appears to solve the challenge - however I think its performance is a bit sluggish (measured running time is about 1300 milliseconds). Is there any way to improve this?

Bug / unexpected result

Focusing only on mostFrequentElement() method

Assume passing the array {2,2,3,3,3} this will result in "None" but the correct result should be "3".

To get rid of this unwanted result, we need after the loop to increment the counter by 1 and add a check

counter++;
if (counter > bigCounter) {
    big = arr[arr.length - 1];
    bigCounter = counter;
}

Simplification

Because you are incrementing counter wether the if condition is true or false this can be simplified to

for (int i = 0; i < arr.length - 1; i++) {
    counter++;
    if (arr[i] != arr[i + 1]) {
        if (counter > bigCounter) {
            big = arr[i];
            bigCounter = counter;
        }
        counter = 0;
    }
}

General

Instead of return big + ""; you should use the return String.valueOf(big); because it is more readable.
you should also check if the given array contains any element.
declaring multiple variables on one line removes readability.
you can initialize bigCounter with 0 because counter is always > 0.
big and bigCounter aren't good names. Better name them mostFrequentElement and mostFrequentElementCounter and rename the method to getMostFrequentElement.
Using verbs or verb phrases for methodnames improves readability.

Refactoring

private static String getMostFrequentElement(int[] arr) {
    if (arr.length == 0) {
        return "None";
    }

    Arrays.sort(arr);

    int counter = 0;
    int mostFrequentElement = -1;
    int mostFrequentElementCounter = 0;

    for (int i = 0; i < arr.length - 1; i++) {
        counter++;
        if (arr[i] != arr[i + 1]) {
            if (counter > mostFrequentElementCounter) {
                mostFrequentElement = arr[i];
                mostFrequentElementCounter = counter;
            }
            counter = 0;
        }
    }

    counter++;
    if (counter > mostFrequentElementCounter) {
        mostFrequentElement = arr[arr.length - 1];
        mostFrequentElementCounter = counter;
    }

    if (mostFrequentElementCounter > arr.length / 2) {
        return String.valueOf(mostFrequentElement);
    }
    return "None";
}


Long function as a lambda, right or wrong? I have been reviewing the code of a new co-worker and, although in general I think it's OK, there's a thing that causes me mixed feelings:

  private void Method(...)
  {
       Thread t = new Thread(() =>
       {
           // Some ~50 lines of code here
       });
       t.Start();
  }

This is no big deal, but I'm curious about what you guys think of it.
Is this exactly what lambdas are for? Or just the opposite? I try to avoid any lambda which doesn't fit on a couple lines at most...

I think it should be a separate method rather than a lambda expression. If the method is as large as 50 lines then you should seriously think about breaking it up anyway, being an anonymous method only compounds the issue. It may be better to extract portions of the method instead, but that is a decision that you will need to make based on the actual code in the method.

My general rule-of-thumb is that if an anonymous method spans more than half the screens height then it's too big and should be a separate named method.

Update in response to Timwi's comments:

When a method is well named, the name helps to describe what the method is intended to do, both where it is used and where it is defined. Anonymous methods don't get this, and so to understand it you need to rely on the context more. This is fine when if the anonymous method is simple but if its large and/or there is deep nesting involved (within or containing various control structures) the behavior can become much less obvious.

The "exact" example used by the OP is not too bad because the containing method is very simple, but I don't see any real benefit (for either performance or readability) and so would still prefer to have it as a separate method.

Of-course, "readability" is a bit subjective. Most people will agree on the extreme cases but the edge cases are probably better discussed with, and a consensus drawn from those actually responsible for maintaining it.


Optimising this simple jQuery script that vertically centers elements on pages This is a simple jQuery script that I use to vertically center elements on pages. It's quite simple, but I would love to know if there is room for improvement.

(function( $ ){

    $.fn.flexVerticalCenter = function( onAttribute, verticalOffset, parentSelector ) {

        return this.each(function() {

            var $this           = $(this);                          // store the object
            var attribute       = onAttribute || 'margin-top';      // the attribute to put the calculated value on
            var offset          = parseInt(verticalOffset) || 0;    // the number of pixels to offset the vertical alignment by
            var parent_selector = parentSelector || null;           // a selector representing the parent to vertically center this element within

            // recalculate the distance to the top of the element to keep it centered
            var resizer = function () {
                var parent_height = (parent_selector) ? $this.parents(parent_selector).first().height() : $this.parent().height();

                $this.css(
                    attribute, ( ( ( parent_height - $this.height() ) / 2 ) + offset )
                );
            };

            // Call once to set.
            resizer();

            // Call on resize. Opera debounces their resize by default.
            $(window).resize(resizer);

            // Apply a load event to images within the element so it fires again after an image is loaded
            $this.find('img').load(resizer);

        });
    };
})( jQuery );

From the comments above, I think the code is ok :)


Image-processing filters using decorator pattern in Python 3 The goal of the following short Python 3 program is to improve my understanding of the decorator design pattern. Therefore, I would be mostly interested about feedback on this aspect, i.e. whether this is a correct usage of mentioned pattern. Also, I would like some comments regarding Python coding best practices, i.e. if there are ways to make my code more "Pythonesque". Of course, any other hints and remarks (e.g., related to the chosen use case, image editing), are welcome too.

#!/usr/bin/python3

import PIL.Image, PIL.ImageFilter, PIL.ImageOps

class IImage:
  def get_img_obj(self):
    raise NotImplementedError("cannot call method of IImage.getImgArr on the interface")

class BaseImage(IImage):
  def __init__(self, img_obj):
    self.img_obj = img_obj

  def get_img_obj(self):
    return self.img_obj

class ImageDecorator(IImage):
  def get_img_obj(self):
    raise NotImplementedError("cannot call method of ImageDecorator.getImgArr on the base class")
  def __init__(self, img):
    self.img = img

class BlurFilter(ImageDecorator):
  def __init__(self, img, radius):
    ImageDecorator.__init__(self, img)
    self.radius = radius
  def get_img_obj(self):
    imageIternal = self.img.get_img_obj()
    return imageIternal.filter(PIL.ImageFilter.GaussianBlur(self.radius))

class ImageFlipper(ImageDecorator):
  def __init__(self, img):
    ImageDecorator.__init__(self, img)
  def get_img_obj(self):
    imageIternal = self.img.get_img_obj()
    return PIL.ImageOps.flip(imageIternal)

im_raw = PIL.Image.open("lena.gif")
im_raw = im_raw.convert('L')
im_obj = BaseImage(im_raw)
im_obj_flipped = ImageFlipper(im_obj)
im_obj_flipped_blurred = BlurFilter(im_obj_flipped, 5)

im_obj.get_img_obj().show("original")
im_obj_flipped.get_img_obj().show("flipped")
im_obj_flipped_blurred.get_img_obj().show("flipped and blurred")

First of all, since Python is dynamically typed, there is not really a need for interfaces. That’s why there is no such language concept. So your IImage type does not really serve any need.

Similarly, the base type ImageDecorator is not really that useful. In statically typed language you are using these design patterns, and ultimately such a base type, because you need to interact with some common base type. Since we have duck typing in Python, this is not really necessary.

Following the Zen of Python, “Simple is better than complex”; and looking at those classes you wrote, it looks somewhat complex to me considering what it does. Compare it with the following solution:

#!/usr/bin/python3
import PIL.Image, PIL.ImageFilter, PIL.ImageOps

def blur(image, radius):
    return image.filter(PIL.ImageFilter.GaussianBlur(radius))

def flip(image):
    return PIL.ImageOps.flip(image)

im_obj = PIL.Image.open('lena.gif')
im_obj = im_raw.convert('L')
im_obj_flipped = flip(im_obj)
im_obj_flipped_blurred = blur(im_obj_flipped, 5)

im_obj.show('original')
im_obj_flipped.show('flipped')
im_obj_flipped_blurred.show('flipped and blurred')

I think this is a lot more to the point, and it shows exactly what’s going on without unnecessarily wrapping the image in an object that does not really serve any purpose.

Of course, I don’t know if your BaseImage is actually needed for something; but if it’s just what you’re showing here, then it’s just a wrapper that doesn’t do anything on its own. You should avoid creating types that don’t have any behavior, especially if they only contain a single object. It’s much easier to just use that actual object then instead.

That being said, if the BaseImage actually is more complicated, then you can still keep your filters simple:

def blur(image, radius):
    rawImage = image.get_img_obj()
    modifiedImage = rawImage.filter(PIL.ImageFilter.GaussianBlur(radius))
    return BaseImage(modifiedImage)

def flip(image):
    rawImage = image.get_img_obj()
    modifiedImage = PIL.ImageOps.flip(rawImage)
    return BaseImage(modifiedImage)

Now you retain your image type, even after the transformation.


Is it ok to break long html elements down into multiple lines? I often see very long code lines like this:

<input type="email" name="email" autocomplete="email" id="email_address" value="<?= $block->escapeHtmlAttr($block->getFormData()->getEmail()) ?>" title="<?= $block->escapeHtmlAttr(__('Email')) ?>" class="input-text" data-mage-init='{"mage/trim-input":{}}' data-validate="{required:true, 'validate-email':true}">

Is it ok to break them down like:

<input type="email"
       name="email"
       autocomplete="email"
       id="email_address"
       value="<?= $block->escapeHtmlAttr($block->getFormData()->getEmail()) ?>"
       title="<?= $block->escapeHtmlAttr(__('Email')) ?>"
       class="input-text"
       data-mage-init='{"mage/trim-input":{}}'
       data-validate="{required:true, 'validate-email':true}"
>

Because this makes it much clearer and easier to read in my opinion.

I usually do this if there are more than about 80-100 chars.

Validation with https://validator.w3.org/

I'd already answered this in the comments (which I shouldn't, we got answer fields for this), but anyway...

Because this makes it much clearer and easier to read in my opinion.

Absolutely. And that's a good reason to do it. Your 80-100 character limit makes sense as well. 80 is considered a standard 'limit' of sorts in many languages (Python is famous for it, but back in the day punch cards and teletypes were already limited to 80 columns).
Google's HTML style guide states:

Break long lines (optional).
While there is no column limit recommendation for HTML, you may consider wrapping long lines if it significantly improves readability.
When line-wrapping, each continuation line should be indented at least 4 additional spaces from the original line.

You've already verified your validator doesn't have a problem with it either, so, go for it. In hindsight, if the validator would've had problems with it, it would've been time for a better validator.
Better readability => better maintainability => less bugs and less development time.


Comparing Server Version Number History I have this code that compares all version numbers of servers. (The version numbers are stored in an XML file) Before the code would compare the current version number with the previous. This caused some problems because in the cluster of servers, one has a different version number than the rest. If my code happened to connect to that server, it would think it was a change. So then I made it compare the three previous version numbers. The same issue was happening. So I got frustrated and made it load all the version numbers in a list and check the current one against the list. Will this cause a huge performance decrease (the list would be 50 elements at most)? Is there a better way?

#Loads all version numbers in list,
#starts at index 1 because the the first
#element in the XML file is has version number of 0

def loadAllVersions(self, hist):
    historyLength = len(hist)
    l =[]
    for i in range(historyLength):
        if i >= 1:
            l.append(hist[len(hist)-i].get("version"))
    return l

#Checks the current version number to each element
#in the list to see if it is the same. If there is a
#match then the it is not a newer version number,
#otherwise it is a new version number and the function
#returns True.

def checkAllVersions(self, currentVersion, allHist):
    historyLength = len(allHist)
    for i in range(historyLength):
        if(allHist[i] == currentVersion):
            return False
    return True

def compareVersions(self,parent,componentTarget, write):
    componentsTree = parent.findall("component")
    currentVersion = self.componentVersions[self.components.index(componentTarget)]

    for comp in componentsTree:
        if(comp.get("id") == componentTarget):
            hist = comp.findall("history")
            allHist = self.loadAllVersions(hist)
            isNewVersion = self.checkAllVersions(currentVersion, allHist)
            if isNewVersion:
                if write and currentVersion != None and self.getComponentURL(componentTarget) != None:
                        event = ET.SubElement(comp,"history")
                        event.set("time",str(int(time.time())))
                        event.set("version",currentVersion)
                        event.set("sourceUrl",self.getComponentURL(componentTarget))
                        event.set("pretty",self.componentsPretty[self.components.index(componentTarget)])
                return True
    return False

Before anything, I reckon you should have a look at PEP 8 which is the usual Style Guide for Python Code. Among other things, your names for variables and functions does not follow the lowercase_with_words_separated_by_underscores convention.

In :

def checkAllVersions(self, currentVersion, allHist):
    historyLength = len(allHist)
    for i in range(historyLength):
        if(allHist[i] == currentVersion):
            return False
    return True

this does not correspond to the pythonic way of looping over a container. There's a cleaner way that doesn't involve indices.

def checkAllVersions(self, currentVersion, allHist):
    for e in allHist:
        if(e == currentVersion):
            return False
    return True

As an additional tip, there's a cool way to write this using all or any :

def checkAllVersions(self, currentVersion, allHist):
    return all(e != current version for e in allHist)

Edit:

And there is an even more obvious way to write it and I completely missed it :

def checkAllVersions(self, currentVersion, allHist):
    return currentVersion not in allHist

In :

def loadAllVersions(self, hist):
    historyLength = len(hist)
    l =[]
    for i in range(historyLength):
        if i >= 1:
            l.append(hist[len(hist)-i].get("version"))
    return l

instead of iterating from 0 to historyLength-1 and then consider only values i such that i >= 1, you could just iterate from 1 to historyLength-1. Just use range(1,historyLength).

def loadAllVersions(self, hist):
    historyLength = len(hist)
    l =[]
    for i in range(1,historyLength):
        l.append(hist[len(hist)-i].get("version"))
    return l

However, it seems like something even smarter could be done here.

At the end of the day, my assumption is that what you want to do is just to iterate over hist from the beginning to the end and call .get("version") on each element. This is not what you are currently doing because you are not processing the first element of the list (index is 0 and would correspond to i  = historyLength).

You have more than one way to do it but the most straight-forward solution involves looping over your list in reversed order with reversed :

def loadAllVersions(self, hist):
    l =[]
    for e in reversed(hist):
        l.append(e.get("version"))
    return l

and then, this calls for list comprehension :

def loadAllVersions(self, hist):
    return [e.get("version") for e in reversed(hist)]

It is now time to have a look at def compareVersions(self,parent,componentTarget, write), a much bigger chunk.

First thing I'd like to point out, you do no follow this part of PEP 8 :

  Comparisons to singletons like None should always be done with is or
  is not, never the equality operators.

I'd like you to confirm my assumption about loadAllVersions before going any further.


PI Calculator, Interview Challenge Greg Beech says here that he asks C# candidates to produce a formula that calculates PI Given that Pi can be estimated using the function \$4 * (1 - \frac{1}{3} + \frac{1}{5} - \frac{1}{7} + \dots)\$.

I'm far from C# interview ready, but I like the challenge. Here is my attempt at the formula. Let me know if there is a better way to do it.

using System;

class MainApp
{
    static void Main ()
    {
        double number = 0;
        double pi;
        int i = 1;

        do
        {
            if ((i/2) % 2 == 0)
            {
                number += (double)(1) / i;
            }
            else
            {
                number -= (double)(1) / i;
            }
            pi = 4 * number;
            i += 2;
        } while (Math.Round(pi,5) != 3.14159);

        Console.Clear();
        Console.WriteLine("Pi can be found using the formula 4 * (1 - 1/3 + 1/5 - 1/7 + ...) with {0} iterations.\n" +
        "At this point 4 is multiplied by: {1} to get {2}\n" +
        "This Rounds up to: {3}",i,number,pi,Math.Round(pi,5));
        Console.ReadKey();
    }
}

In an interview it usually doesn't matter if you actually solve the problem. What is most important is the way you (try to) solve it. If they don't tell you it should be the possibly fastest solution ever you should not optimize it prematurely but instead show that you know how to write SOLID code like encapsulate propertly, make the code testable etc. show that you know the techniques.

Here you could split the logic up into multiple methods for example you could have one generating the alternating sequence [1, -3, 5, -7, 9, -11, ...] that you can validate with a test:

public static IEnumerable<int> AlternatingSequence()
{
    var i = 1;
    yield return i;
    var b = false;
    while (true) yield return ((b = !b) ? -1 : 1) * (i = i + 2);
}

then you can work on the PI itself and use it for the actual calculation which is then as simple as a LINQ expression:

public static double EstimatePI(int sumLength)
{
    return (4 * AlternatingSequence().Take(sumLength).Sum(x => 1.0 / x));
}

var pi = EstimatePI(500_000).Dump(); // 3,14159065358969

There's all kind of stuff you can show you are familiar with in this simple task like DI or strategy pattern:

public static double EstimatePI(int sumLength, IEnumerable<int> sequenceGenerator)
{
    return (4 * sequenceGenerator.Take(sumLength).Sum(x => 1.0 / x));
}

var pi = EstimatePI(500_000, AlternatingSequence);

or even more advanced DI and encapsulation

class PiCalculator
{
    private readonly IEnumerable<int> _sequenceGenerator;

    public PiCalculator(IEnumerable<int> sequenceGenerator)
    {
        _sequenceGenerator = sequenceGenerator;
    }

    public double EstimatePi(int sumLength)
    {
        ...
    }
}

or you can add an interface to show you know what abstraction means:

interface IPiCalculator
{
    double EstimatePi(int sumLength);
}

Is this an overkill? It might be but it shows me that you are able to write modular and testable code. Writing short code is a good thing too but if you write only five lines inside a Main in a job interview, you'll go home without any prospects of getting the job. Again, in an inteview you should sell your coding skills and not show how smart you are in solving something with a fewest possible lines of code that no one can verify.


Repetitive code for a projected field in a lambda I have a simple projection in my code. Taking away the majority of the fields which are not of interest, it looks like the following:

var result = employments.Select(x => new EmpListItem
        {
            EndDate = x.EmpRelationship.OrderByDescending(y => y.EndTime).FirstOrDefault().CancelledBeforeTime ?
                        x.EmpRelationship.OrderByDescending(y => y.EndTime).FirstOrDefault().CancelEndTime :
                        x.EmpRelationship.OrderByDescending(y => y.EndTime).FirstOrDefault().EndTime,
            JobType = x.JobType
        });

I really dislike how I use x.EmpRelationship.OrderByDescending(y => y.EndTime).FirstOrDefault() three times. It looks ugly and readability is poor.

Is there a way to use this only once while staying inline? By inline I mean that I don't want to write any code outside of the projection.

You can extract out that repeat call to a variable inside your Select.

var result = employments.Select(x =>
{
    var empRelationship = x.EmpRelationship.OrderByDescending(y => y.EndTime).FirstOrDefault();

    return new EmpListItem
    {
        EndDate = empRelationship.CancelledBeforeTime ?
                      empRelationship.CancelEndTime :
                      empRelationship.EndTime,
        JobType = x.JobType
    };
});

This will also speed up execution.

The trick behind this is remembering you can execute more than just one statement inside a lambda by using { and }


Properly mark up multiple person using schema.org microdata I am creating a radio episode that will have multiple conductors (usually 1 to 3): inside a loop I am associating those conductors to director providing also the url linking to their respective conductors page:

    Conducted by

    start loop...

            {{name}}}

        <meta itemprop="jobTitle" content="Conductor">

    end loop....

but I am not sure if I am doing it well and I feel I could have done multiple mistakes:

The way I define the multiple person
How I am associating the url
The jobTitle (I expect this to better specify the director in some way)
span inside span (trying to have conductors on one line and inside an heading for better semantically specify that they important to the radio episode)

Microdata / Schema.org

Your use of Microdata and the Schema.org vocabulary is fine.

To specify multiple directors, you have to provide multiple director properties, which your loop seems to achieve:

…
…
…

To specify a URL value, you have to use a suitable link element (with href or src attribute), like a, link, img etc. So your use of the a element achieves this.

I prefer using http Schema.org URIs (reasons why), but using https is fine, too.

HTML

Is this h3 really the heading for a section of content? If yes, your markup is fine. But then you might want to add a textual delimiter (like ,) to separate the conductor names.

If the section should just specify the conductors, and there is no other content under its scope, the h3 should just be something like "Conducted by", with the list of conductors under its scope:

Conducted by

…

…

…

Headings should not be used for content just because this content is important. h1-h6 don’t represent important content, they represent "headings for their sections". There is typically no case where you should have a heading without any content in its section.


Small function and level of abstraction - clean code - refactor further In the book Clean Code from Robert C. Martin are given many recommendations. Two of them are

to keep functions small and
within a function to only have one level of abstraction.

I already cleaned the following method up a bit by extracting a few methods (RemoveFontColor(), RemoveTextHighlight()). But i am not sure if the few-liners (font-style, font-weight, etc.) should be extracted into a method as well. The advise from the author is to not mix different-levels of abstraction within a function because this could lead to other details creeping in. This would mean that even the two liners will be extracted into their own methods.

The partly refactored code example

private string Fix(FixPreferences preferences,string value)
{
    if (preferences.IsRemoveBold) {
        value = value.Replace("font-weight:bold", "font-weight:normal");
        value = value.Replace("font-weight: bold", "font-weight:normal");
    }
    if (preferences.IsRemoveItalics) {
        value = value.Replace("font-style:italic;", "font-style:normal;");
        value = value.Replace("font-style: italic;", "font-style:normal;");
    }
    if (preferences.IsRemoveFontColor) {
        value = RemoveFontColor(value);
    }
    if (preferences.IsRemoveTextHighlight) {
        value = RemoveTextHighlight(value);
    }
    if (preferences.IsRemoveStrikethrough){
       value =
        value.Replace("text-decoration: line-through;", "text-decoration:none;");
       value =
        value.Replace("text-decoration:line-through;", "text-decoration:none;");
    }
    if (preferences.IsRemoveUnderline)
    {
      value =
       value.Replace("text-decoration: underline;", "text-decoration:none;");
      value =
       value.Replace("text-decoration:underline;", "text-decoration:none;");
    }
    return value;
}

Update

I changed to extract even the few lines - but it still looks not pretty.

    private string Fix(ChangeLogFixOperationPreferences preferences,string value)
    {
        if (preferences.IsRemoveBold)
                value = RemoveBold(value);

        if (preferences.IsRemoveItalics)
                value = RemoveItalics(value);

        if (preferences.IsRemoveFontColor)
                value = RemoveFontColor(value);

        if (preferences.IsRemoveTextHighlight)
                value = RemoveTextHighlight(value);

        if (preferences.IsRemoveStrikethrough)
                value = RemoveStrikethrough(value);

        if (preferences.IsRemoveUnderline)
                value = RemoveUnderline(value);

        if (preferences.IsResetFontSize)
                value = ResetFontSize(value);

        return value;
    }

Is there a better way - for example to put the preferences and the methods into a dictionary and for each preference that is set to true the function from the dictionary is called within a loop?

My question

How would you refactor the code above?
Are there any ways to group the different font-* or text-decoration:* togehter in one function? Would that be wise since it violates the single responsibility pattern (SRI)?
Update: Would you recommend calling a function from a dictionary

store functions in a dictionary or may be
call method dynamically or
from Jon Skeet storing functions in a dictionary by using Action

First of all, each condition and formatting will be moved to your own class. Let's create a common interface:

interface FormattingRemover
{
    bool Match(string value);
    string RemoveFormat(string value);
}

Now we can use it to abstract the formatting:

public class RemoveBold : FormattingRemover
{
    public bool Match(string value) {
        // your condition here
        return true;
    }

    public string RemoveFormat(string value) {
        return RemoveBold(value);
    }
}

In your class you'll need a list with all implementantions of FormattingRemover, iterate over it and when the value matches, you can remove its formatting.

private removers = AllRemovers();

private string Fix(ChangeLogFixOperationPreferences preferences,string value)
{
    foreach (var remover in removers)
    {
        if (remover.Match(value))
        {
            value = remover.RemoveFormat(value);
        }
    }

    return value;
}

For now on, you just need to add new implementations to provide it with AllRemovers. A good idea is inject it with some DI container, like Ninject.


Implementing a domain model My domain model consists mostly of simple DTOs, i.e. 'Data Transfer Objects' which this article distinguishes from 'Plain Old C# Objects', like this one:

public class Settings
{
    public bool FullScreen = false;
    public int WindowWidth = 800;
    public int WindowHeight = 600;
}

I realize this model is "anemic," but it works well with the JSON serializer I use for persistence. Also, I'm still in the exploratory stage of development and I read that it's okay at this point to have a bunch of "property bags."

Other classes look less like DTOs and more like POCOs (or POJOs although I'm using C#):

public class Sprite
{
    [JsonProperty]
    private readonly string assetName;

    [JsonProperty]
    private Vector2 position;

    // other properties...

    public Sprite( string assetName, Vector2 position )
    {
        this.assetName = assetName;
        this.position = position;
    }

    public void Move( Vector2 direction )
    {
        // do move
    }

    // other methods...
}

Because this type of object needs to be persisted by the JSON serializer, the properties need either to be public or have a JsonProperty attribute. I went with the attribute in this case because I want to restrict access to its properties.

Is this a good design? Does it make sense for my "entities" (I'm not sure if that's the correct term) to look so different from one another? My gut told me no, so I tried extracting the stuff not needed by the serializer into a separate class outside of the namespace:

public class SpriteModel
{
    private readonly Sprite sprite;

    public SpriteModel( Sprite sprite )
    {
        this.sprite = sprite;
    }

    public void Move( Vector2 direction )
    {
        // do move
    }

    public void GetSprite()
    {
        return sprite;
    }

    // other methods...
}

Now the Sprite class looks more like a DTO:

public class Sprite
{
    public string AssetName;
    public Vector2 Position;
    // other properties...
}

Someone told me that my domain model is actually the SpriteModel class now and not the Sprite class itself. I'm obviously confused about what to call these things exactly, and any clarification would be appreciated.

But my main question is: which of these approaches is better and why? Are they both terrible? And in that case is there another approach that I should use instead? If I go with the second approach, then I could have SpriteModel implement ISpriteModel, and the classes that use it could be more loosely coupled, which is a nice side benefit I think. But I don't want to go that route just for a side benefit that might not actually pay off (and could just add more complexity).

I would say that one of the major differences between a DTO and a POCO is the fact that a DTO should be immutable because it is meant to be used for transferring only. See what are the differences between a POCO and a DTO in this SO question. That said, someone could suggest that your DTO implementation should be the following:

public class Settings
{
    public bool FullScreen{get; private set;}
    public int WindowWidth{get; private set;}
    public int WindowHeight{get; private set;}
    public Setting(int width, int height, bool fullScreen){
       WindowWidth = width;
       WindowHeight = height;
       FullScreen = fullScreen
    }
}

This is only if you want to be a purist in the implementation of a DTO because the usage of this is a pain, especially if your DTO class has many properties. The work around is the one you already have, although I would trade the fields for properties because properties respect the principle of encapsulation and are implemented with the same space in C# (in Java I wouldn't mind having public fields though).

About your second issue, I would say that your real problem is to isolate the domain from its representation, be it JSON or something else. So how could you separate them? I would personally use the proxy pattern:

public class Sprite
{
    public virtual string AssetName{get; set;}
    public virtual Vector2 Position{get; set;}
}

public class SpriteJson : Sprite
{
    public SpriteJson(Sprite sprite){
       AssetName = sprite.AssetName;
       Position = sprite.Position;
    }
    public SpriteJson(){}
    [JSonProperty]
    public override string AssetName{get; set;}
    [JSonProperty]
    public override Vector2 Position{get; set;}
}

It may seem that you have too much redundancy, but it does isolate the objects from their representation. You could receive a Sprite object anywhere in your code, whether it is a SpriteJson object or not.

Edit: I think I only answered your question partially, so I edited my answer to provide more details.

Leaving the representation aside, the logic associated with the object would be in your Sprite class:

public class Sprite
{
    public virtual string AssetName{get; set;}
    public virtual Vector2 Position{get; set;}
    public void Move(Vector2 direction)
    {
         // do move
    }
}

I think an object should always have the logic associated with it whenever it makes sense and is reasonable. There is no need for making a wrapper only to separate the logic (a useless abstraction, in my opinion). The question I linked also stated that POCO classes may contain logic operations. So when is it not reasonable? Imagine that your move code is highly dependent on another class and many of your objects also have their own move logic. In that case, it would be better to refactor the move code to another place so you wouldn't have code redundancy and eliminate those dependencies.