JavaScript modular framework for a large web-site In my ASP.NET MVC project I simply inserted <script></script> into my views - it caused code duplications and spaghetti code.

Now, I have decided to switch to modular JS and have implemented the following mechanism:

ALL actions are only executed through the framework and its modules - whether it is a view or object behavior, date-to-string converter or websocket connection
every module exposes exports functions, which can be called by anybody, including other modules
all functions including exports and init are called within a sandboxed context, which includes both objects specific for the module and objects, injected by a framework (currently, I am implementing the mechanism to "hide" / disallow non-context methods within a module)

Framework object with the only register function:

window.kibfw = {
    register: function(name, module) {
        if (!name || !module.init)
            throw new Error('Could not load a module ' + name);

        if (name === 'register')
            throw new Error('Module cannot have name "register" which is reserved');

        if (this[name])
            throw new Error('Module ' + name + ' is already registered.');

        module.context = module.context || {};

        /* Here, inject the required context objects (jQuery, libraries etc.) */
        module.context.$ = window.jQuery;
        module.context.kibfw = window.kibfw;

        try {
            module.init.apply(module.context, arguments);
        } catch(e) {
            throw new Error('Error during module initialization: ' + e.message + '\n' + e.stackTrace);
        }

        var moduleExports = (this[name] = {});
        for (var exportFnName in module.exports) {
            if (!module.exports.hasOwnProperty(exportFnName))
                continue;

            moduleExports[exportFnName] = (function (exportFn) {
                return function() {
                    try {
                        exportFn.apply(module.context, arguments);
                    } catch (e) {
                        throw new Error('Module: ' + name + '. Error during method invocation: ' + e.message + '\n' + e.stackTrace);
                    }
                };
            })(module.exports[exportFnName]);
        }
    }
};

Example module:

kibfw.register('decorator', {
  init: function () {
    this.$decorateItems = this.$('input');
  },
  context: {
    $decorateItems: undefined
  },
  exports: {
    setColor: function (color) {
      this.$decorateItems.css('color', color);
    }
  }
});

// Usage:
$("button").click(function() {
    kibfw.decorator.setColor($(this).attr('data-color'));
});

JSFiddle demonstration

How do you think, would it be useful and scalable?
Which problems can I stumble upon while using this approach?

Very readable code, some minor comments:

Some folks dislike the lacking curly braces for one liners, I dont mind it here myself
JsHint will complain about you creating a function in a for loop, I think you should indeed refactor that
Using this[name] to register modules would be to fickle for me (shenanigans with bind, apply, and call come to mind), I would prefer a self executing function with a var statement.
You only have 1 line of comment, for a very straightforward piece of code, I would have commented the moduleExports[exportFnName] = (function (exportFn) { line
I would split this out in to 2 different error messages:  if (!name || !module.init)
        throw new Error('Could not load a module ' + name);
Actually, in your code I see that init is a bit contrived, I would not make that mandatory personally

Now, when I take a step back, I think you might be suffering from Second System Syndrome. Consider that in effect you are replacing

$("button").click(function() {
    $('input').attr('data-color'));
});

with

kibfw.register('decorator', {
  init: function () {
    this.$decorateItems = this.$('input');
  },
  context: {
    $decorateItems: undefined
  },
  exports: {
    setColor: function (color) {
      this.$decorateItems.css('color', color);
    }
  }
});

// Usage:
$("button").click(function() {
    kibfw.decorator.setColor($(this).attr('data-color'));
});

I like window.kibfw, it is pretty beautiful. But I would hate to have to write a website with it.


A* C++ implementation is too slow My A* Algorithm in C++ is pretty slow. I'm not sure if it is because of a bad implementation or just because I have way too many Nodes (I have a field of 256x256 Nodes). It takes the algorithm about 5 seconds to complete.

I'm using a vector for the open and closed List. For the openList I use std::make_heap to sort the Nodes.

/************
* Method to calculate the shortes path
*
**/
std::vector<Position*> Pathfinding::getShortestPath(MapField* startField, MapField* targetField) {

PathNode* startNode = new PathNode(startField, argosMap);
openList.push_back(startNode);

push_heap(openList.begin(), openList.end(), ComparePathNodePointer());

while(!(openList.empty())){

    make_heap(openList.begin(), openList.end(), ComparePathNodePointer());
    PathNode* currentNode = openList.front();

    pop_heap(openList.begin(), openList.end(), ComparePathNodePointer());

    openList.pop_back();
    make_heap(openList.begin(), openList.end(), ComparePathNodePointer());
    if(*(currentNode->getMapField()) == *targetField){
        Logger::getInstance()->log("Target found");
        return buildpath(currentNode);
    }
    expandNode(currentNode, targetField);
    closedList.push_back(currentNode);
}
return buildpath(startNode);
};

/************
* expands a PathNode and adds it to the openList if it meets the conditions of A*
*
**/
void Pathfinding::expandNode(PathNode* currentNode, MapField* targetField) {

std::vector<PathNode*> children = currentNode->getChildren();
std::vector<PathNode*>::iterator child;

for (child = children.begin(); child != children.end(); child++) {

    Utils::PointerValuesEquals<PathNode> childToFind = {*(child)};

    //check if Node is alreay in closedList
    if(std::find_if(closedList.begin(), closedList.end(), childToFind) == closedList.end()) {
        double actualCost = (*child)->getCostOfField(currentNode)+currentNode->getActualCost();

        //check if Node is already in openList, if so, check if the value is better if it is better, expand the new Node otherwise use the existing Node
        if(!((std::find_if(openList.begin(), openList.end(), childToFind) != openList.end()) && actualCost >= (*std::find_if(openList.begin(), openList.end(), childToFind))->getActualCost())){
            double estimatedCosts = actualCost+(*child)->getCostToTarget(targetField);

            //when Node is already in Openlist update it, if not add the new Node
            if(std::find_if(openList.begin(), openList.end(), childToFind) != openList.end()) {
                (*std::find_if(openList.begin(), openList.end(), childToFind))->setParent(currentNode);
                (*std::find_if(openList.begin(), openList.end(), childToFind))->setActualCost(actualCost);
                (*std::find_if(openList.begin(), openList.end(), childToFind))->setTotalCost(estimatedCosts);

                make_heap(openList.begin(), openList.end(), ComparePathNodePointer());

            } else {
                (*child)->setParent(currentNode);
                (*child)->setActualCost(actualCost);
                (*child)->setTotalCost(estimatedCosts);
                this->openList.push_back(*child);
                push_heap(openList.begin(), openList.end(), ComparePathNodePointer());
                make_heap(openList.begin(), openList.end(), ComparePathNodePointer());
            }
        }
    }
}
};

As a guideline: your inner loop should contain no operation more expensive than O(log(N)). This means no iterating over the entire closed/open list, and no making heaps over and over again! Here's how:

Use a std::set sorted by weight instead of a heap. begin() is your next node, and to update the weight of a node first remove it from the set, change the weight, and put it back again.
Don't use find_if to test for set inclusion. Use an std::unordered_set for the closed or open nodes.


Class for simple python object to be hashed and referenced I need objects in python that I can compare, reference to and hash.

In principal a tuple or list would be good enough just that
a list I cannot hash and the tuple I cannot change,
except replacing it which means losing references to it.

So I created a class for it, but still have the feeling there should be a simpler/better solution to it.

class MyClass:
    def __init__(self, k, l=0, m=0):
        self.k = k
        self.l = l
        self.m = m
    def __hash__(self):
        n_attr = len(self.__dict__)
        return sum(hash(attr)/n_attr for attr in self.__dict__.values())
    def __eq__(self, other):
        return all(sa==oa for sa, oa in zip(self.__dict__.values(),
                                           other.__dict__.values()))

Now I can do what I need: creating some objects

x = MyClass(1,2,3)
y = MyClass(7)
z = MyClass(7,1,0)

and some references

a = x; b = y; c = y; d = z

and compare, manipulate ...

b == c # True
b == d # False
b.l = 1
b == c # still True
b == d # now True
s = {a, d, (a,b)}
b in s # True
c in s # True
(a,c) in s # True

I wished I could just forget about the class and do

x = 1,2,3
y = 7,
z = 7,1

etc ...

Is writing this class the best way to approach this problem? Or did I miss something?

edit:

After the comment by WinstonEwert I confirmed that the objects cause breakage as keys in dictionary

s = {a: 'a', b: 'b'}
s[c] # 'b'
c.l = 99
s[c] # key error
s # key error

and decided to replace dictionaries that use these objects as keys by

class MyDict:
    def __init__(self, d={}):
        self._data = d.items()
    def __getitem__(self, key):
        for k, v in self._data:
            if k == key: return v
        raise KeyError
    def __setitem__(self, key, value):
        for i, (k, v) in enumerate(self._data):
            if k == key:
                self._data[i] = key, value
                return
        self._data.append((key,value))
    def __contains__(self, key):
        for k, v in self._data:
            if k == key: return True
        return False

Now I have the desired behavior

s = MyDict()
s[a] = 'a'
s[b] = 'b'
s[c] # 'b'
c.l = 99
s[c] # still 'b'

And I do not need MyClass any more and can use list

x = [1,2,3]
y = [7,0,0]
z = [7,1,0]

The python dictionary assumes that the keys are immutable. It assumes they will not change. That's why mutable classes in python (such as lists) don't hash. Its to prevent you from putting them in dicts which just won't work.

As a result the entire premise of your class is wrong. You shouldn't do that. To follow what python expects, an object should return the same hash and be equal to the same objects throughout its lifetime. If it can't it shouldn't support hashing.

We might be able to suggest a better way to do this, but you'll need to show more of how you want to use this class.


Click handlers that display the status of a submission It's been years since I've touched javascript and the strange scoping trips me up each time.  Please could you point me into the right direction of how to both tidy this up and prevent the duplication?

It is simply two click events, with the only difference being the color updated on success.

$(function () {

    $(".manualSubmitComplete")
        .click(function () {

            $(".spinner").show();

            var $this = $(this);

            $.get(this.href)
                .done(function () {

                    var $parent = $this.parent();

                    $parent.children("spinner").show();

                    $parent.children("a").remove();
                    $parent.children("span").remove();
                    $parent.attr("class", "");
                    $parent.css("background-color", "#B0FFB0");

                    $(".spinner").hide();
                });

            return false;

        });

    $(".manualSubmitReject")
            .click(function () {

                $(".spinner").show();

                var $this = $(this);

                $.get(this.href)
                    .done(function () {

                        var $parent = $this.parent();

                        $parent.children("spinner").show();

                        $parent.children("a").remove();
                        $parent.children("span").remove();
                        $parent.attr("class", "");
                        $parent.css("background-color", "#FF0000");

                        $(".spinner").hide();
                    });

                return false;

            });

})

You can simply extract the common code into a function.  Arguably you could pass this rather than href and $target and derive ref and target in the function or pass $parent rather than $target, but the current arguments seem slightly clearer.

$(function() {

    var fetchUrl = function (href, $target, color) {
      $(".spinner").show();

      $.get(href)
          .done(function() {

              var $parent = $target.parent();

              $parent.children("spinner").show();

              $parent.children("a").remove();
              $parent.children("span").remove();
              $parent.attr("class", "");
              $parent.css("background-color", color);

              $(".spinner").hide();
          });
    };

    $(".manualSubmitComplete")
        .click(function() {
            fetchUrl(this.href, $(this), "#B0FFB0")
            return false;
        });

    $(".manualSubmitReject")
        .click(function() {
          fetchUrl(this.href, $(this), "#FF0000")
          return false;
        });
});


Pythonic data processing? Should I use an iterator? In a data processing and analysis application I have a dataCleaner class that conducts a series of functions that help me to clean raw time series data.

One of the problems I see in my data is that they often include large gaps at the beginning and end. This is due to the occasional corruption of the timestamp, a behaviour that I cannot influence but can lead to arbitrary timings of individual data records.

Imagine an hourly dataset covering November 2011. Sometimes one of the timestamps is corrupted and may end up recording a date of January 2011. Since the data are sorted by date this puts a point at the beginning which needs to be removed. It is possible that this corruption can occur more than once in any given dataset. I need to detect and remove these outliers if they exist.

So I designed this function to trim off contiguous values at each end of the data if the time gap is considered large. My data arrive into this function in the form of two numpy arrays (timestamps and values) and must be filtered together.

@staticmethod
def _trimmed_ends(timestamps, values, big_gap = 60*60*24*7):
    """
    Uses timestamps array to identify and trim big gaps at either end of a dataset.
    The values array is trimmed to match.
    """
    keep = np.ones(len(timestamps), dtype=bool)
    big_gaps = (np.diff(timestamps) >= big_gap)
    n = (0, 0)
    for i in xrange(len(keep)):
        if big_gaps[i]:
            keep[i] = False
            n[0] += 1
        else:
            break

    for i in xrange(len(keep)):
        if big_gaps[::-1][i]:
            keep[::-1][i] = False
            n[1] += 1
        else:
            break

    if sum(n) > 0:
        logging.info("%i points trimmed (%i from beginning, %i from end)" % (sum(n), n[0], n[1]))
    else:
        logging.info("No points trimmed")
    return timestamps[keep], values[keep]

Is this a pythonic way to do this? I have been advised that I might want to convert this into an iterator but I'm not sure it that is possible, let alone desirable. As I understand it, I need to attack the array twice, once forwards and once backwards in order to achieve the desired result.

Firstly, a loopless solution (not actually tested)

big_gaps = np.diff(timestamps) >= big_gap

As you did

front_gaps = np.logical_and.accumulate(big_gaps)

This produces an array which is True until the first False in big_gaps.

end_gaps = np.logical_and.accumulate(big_gaps[::-1])[::-1]

We do the same thing again, but this time we apply on the reversed array.

big_gaps = np.logical_or(front_gaps, end_gaps)

Now that we have arrays for the front and end portions, combine them

n = np.sum(front_gaps), np.sum(end_gaps)

Take the sums of the gaps arrays, 1 = True, 0 = False to figure out the values for n

keep = np.logical_not(big_gaps)

Invert the logic to figure out which ones to keep

return timestamps[keep], values[keep]

Produce your actual values

A few comments on pieces of your code

@staticmethod
def _trimmed_ends(timestamps, values, big_gap = 60*60*24*7):

I'd make a constant: ONE_WEEK = 60*60*24*7, as I think it would make this clearer

big_gaps = (np.diff(timestamps) >= big_gap)

You don't need those parens

n = (0, 0)

This is a tuple here, but you modify it later. That won't work.

And finally, no you shouldn't use an iterator, for the reasons you mention.


FileSystemWatcher with threaded FIFO processing I have single threaded C# console application that uses FileSystemWatcher to watch folder for new files.

My app sees a file being written, waits until it is released, then picks it up and processes it. At the same time it writes a rolling log to a text file:

2014-08-06 16:20:1.500 - Found file : C:\test1.pdf
2014-08-06 16:20:1.510 - Waiting for file to become available
2014-08-06 16:20:2.010 - Processing file
2014-08-06 16:20:8.256 - Finished processing file: C:\test1.pdf
2014-08-06 16:20:8.785 - File C:\test2.pdf found!

etc..etc..

The current method ensures the following:

Each file is processed in the order that FileSystemWatcher sees it.
The log file is written in a linear fashion, one file at a time.

Files created in the folder seem to be cached FileSystemWatcher until the current file has finished processing.
This works OK but I don't like the fact that FileSystemWatcher is caching it, it must have a buffer limit somewhere and files might drop of the end.

I think I need two threads:

FileSystemWatcher thread that sees the new files and passes to a 'Files to Process' collection.
File processing thread that sees there are items in the 'Files to Process' collection and processes them, FIFO style.

I started work on this and decided to use an ObservableCollection for the 'Files to Process' list, I thought I could hang methods off the NotifyCollectionChangedAction events but I am a bit stuck as to where to now put the thread.

My questions are:

Do I need two threads?
Is ObservableCollection the best object to use to manage the list of files to process?
Where do I put a second thread? I am guessing that I need a new thread each time a new file is added to the ObservableCollection but won't that trigger a thread for each file, then the log will be jumbled up with each file that is added to the collection?

The files that FileSystemWatcher pick up become Document objects.
Here is the code I have so far:

//Make the objects and set events
watcher = new FileSystemWatcher();
watcher.Created += new FileSystemEventHandler(AddDocument);
docsToProcess = new ObservableCollection<Document>();
docsToProcess.CollectionChanged += new NotifyCollectionChangedEventHandler(CollectionChanged);

//Called by FileSystemWatcher when it sees a new file
 void AddDocument(object source, FileSystemEventArgs f)
        {
            Document doc = new Document(f.FullPath);
            docsToProcess.Add(doc);
        }

//Called when the collection changes
 void CollectionChanged(object sender, NotifyCollectionChangedEventArgs e)
        {
            switch (e.Action)
            {
                case NotifyCollectionChangedAction.Add:
                    foreach (Document doc in e.NewItems)
                    {
                        log.Write("Found file : " + doc.FullPath);
                        ProcessDoc(doc);
                    }
                    break;

                case NotifyCollectionChangedAction.Remove:
                    foreach (Document doc in e.OldItems)
                    {
                        log.Write("Finished Processing file : " + doc.FullPath);
                    }
                    break;

                default:
                    break;
            }
        }

//Called when a new document is added to the collection
 void ProcessDoc(Document doc)
        {
            //Do processing stuff to the document, also write to the log
            //Then remove itself from the collection
            docsToProcess.Remove(doc);
        }

Files created in the folder seem to be cached FileSystemWatcher until the current file has finished processing.

Not really cached, FileSystemWatcher just raises its events one at a time. So, until your event handler returns, you won't get another notification. And since you execute all your code in that event handler, that can take a very long time.

The docs say this:

  To avoid missing events, follow these guidelines:

  […]
  Keep your event handling code as short as possible.

So you shouldn't do it this way.

  Do I need two threads?

If you want to make reasonably sure you won't miss anything, yes, you need (at least) two threads.

  Is ObservableCollection the best object to use to manage the list of files to process?

In the current state, ObservableCollection doesn't make much sense, because there is no list, you will only ever have one item in the collection.

If you switched to two (or more) threads, ObservableCollection is still not a great choice, since it's not thread-safe.

  Where do I put a second thread? I am guessing that I need a new thread each time a new file is added to the ObservableCollection but won't that trigger a thread for each file, then the log will be jumbled up with each file that is added to the collection?

If you wanted to process each file in parallel, you would need more threads. To keep the log clean (and to perform any other operations that are not thread-safe), you should use a lock.

In any case, you shouldn't directly use Threads. Instead, you should use Tasks, or some higher-level constructs (see below for more), since they are more efficient and easier to work with.

But if you don't need parallelism, a single processing thread (i.e. two threads in total) is enough and it also means you don't need any locks (assuming only this thread writes to the log).

Some specific options on how you could implement this:

Use ActionBlock from TPL Dataflow (requires .Net 4.5). This could be the simplest option, since it means you don't need to create any Threads or Tasks manually.

In the event handler, you would call Post(), and the work would be handled by the delegate that you passed to ActionBlock's constructor.

ActionBlock can also work in parallel, if you set its MaxDegreeOfParallelism.
Use BlockingCollection and a Task with a loop.

In the event handler, you would call Add(). You would then also create a Task (using Task.Run() or Task.Factory.StartNew()) with a foreach loop over GetConsumingEnumerable(), that process the files.

This will work best if there is only a single Task, which means it won't be parallel.
Create a separate Task for each file.

In the event handler, you would create a Task with a delegate that processes the file.

This way, files will be processed in parallel.


Beginner word-guessing game The aim is to guess a four letter string. Each letter can be tried four times. If the letter cannot be guessed, we go to the next letter until the fourth letter is reached.

I looked at my code and even though I'm satisfied because I've done everything by myself, I'm wondering how I can improve it. I'm probably relying too much on if but so far. I do not have ideas on what I can do to make it leaner.

# -*- coding: utf-8 -*-

def typeYourword():
  global yourword
  yourword= raw_input("type your word \n")
  while len(yourword)<4 or len(yourword)>4:
    yourword= raw_input("type your word again \n")
  else:
    print("Now your word : ",yourword," has the correct format")
    return yourword

#yourword='tata'

def monMot(tonmot):
    global var_glob
    var_glob=list(tonmot)
    return var_glob

def comptage(monmot):
    global var_comptage
    var_comptage = 0
    for cpte in range(0,(len(monmot))):
      print("this is your " + str(cpte+1) + " letter")
      for count in range(1,5):
        comptage_mot=cpte+1
        b = raw_input("Type your letter \n")
        while len(b)>1 or len(b)==0:
          b=raw_input("type your letter again \n")
        else:
          if var_glob[cpte] == b and comptage_mot<4:
            var_comptage+=1
            print ("yes")
            break
          elif var_glob[cpte] == b and comptage_mot==4:
            var_comptage+=1
            print ("You won. End of game")
            break
          else:
            if count == 4 and comptage_mot<4:
              print("this was your "+str(count)+ " last try for the "+str(comptage_mot)+" letter. let's go to the next letter")
            elif count == 4 and comptage_mot==4:
              print("this was your last try for the word. You guessed "+str(count)+ " letters. End of game.")
            else:
              print("wrong letter, try again" + " ,this was your " + str(count) + " try. Now your " +str(count+1) + " try")
    print(var_comptage)

typeYourword()
monMot(yourword)
comptage(var_glob)

Firstly, your function names do not conform to standards. For example, typeYourword should be typeYourWord, thought I'd suggest actually using something similar to inputWord, so the verb is related to what the program does rather than the user.

You should read PEP 8, or set your GUI (If you are using one) to warn you of violations of the standard. For example, you are missing a lot of spaces around your operators. For example, b=raw_input("type your letter again \n") should be b = raw_input("type your letter again \n").

Your indent length varies between different functions making the code a bit harder to reason about.

The use of global variables is frowned upon. You are already returning the variable from typeYourword, so consider storing this in a variable and passing it into the other functions. For example:

word = typeYourword()
monmot(word)

monmot appears to have no function, as strings can already be indexed, though perhaps I am not seeing something there.

You do checks for greater than or less than when you could just be using the != operator to check for inequality (while len(yourword) != 4)

Code that is run when the module is loaded directly (the last three lines) should be contained within a check for __name__ == "__main__"

You do not need the else clause on the while loops, this is mainly for complicated loops involving break statements, and you can just outdent your code:

  while len(yourword)<4 or len(yourword)>4:
    yourword= raw_input("type your word again \n")
  print("Now your word : ",yourword," has the correct format")
  return yourword

Hopefully this can help you, and comment if you want anything I've said to be clarified


Extract subarray of elements that appear after the last occurrence of "4" I am doing some challenges, and in one challenge given a non-empty array of int, I am to return a new array containing the elements from the original array that come after the last 4 in the original array. The original array will contain at least one 4.

E.g:

  post4([2, 4, 1, 2]) → [1, 2]
  post4([4, 1, 4, 2]) → [2]
  post4([4, 4, 1, 2, 3]) → [1, 2, 3]

As such, I have written code that works but it is far too complicated and unprofessional, at least it feels this way.

public static int[] post4(int[] nums) {

    int startPoint = nums.length - 1;
    int[] newArray;
    int amtOfElements = 0;
    int stopIndex = 0;

    for (int i = startPoint; i >= 0; i--) {

        if (nums[i] == 4) {
            stopIndex = i;
            i = 0;
        } else {
            amtOfElements++;
        }
    }

    newArray = new int[amtOfElements];

    for (int i = 0, j = stopIndex; i < amtOfElements; i++, j++) {
        newArray[i] = nums[j + 1];
    }

    return newArray;
}

As such, this looks nasty to me.
How can I shorten this to a single for-loop?

I would use this:

public int[] post4(int[] arr) {
  for(int i = arr.length-1; i >= 0; --i) {
    if(arr[i] == 4) {
      return Arrays.copyOfRange(arr, i+1, arr.length);
    }
  }
  return new int[0]; // or null
}


A repeated string formatting operation As I'm reading the Java code of some other people, I found him using an approach of assigning a format string to a variable and using the variable for String.format in the code:

private final static String URI_SUFFIX_FORMAT = "view/%s";

@Inject
private UriBuilder uriBuilder;

public void buildUriForMusic(String dir)
{
    ...
    String ret = uriBuilder.buildContentUri(dir, String.format(URI_SUFFIX_FORMAT, "music"));
    ...
}

public void buildUriForBook(String dir)
{
    ...
    String ret = uriBuilder.buildContentUri(dir, String.format(URI_SUFFIX_FORMAT, "book"));
    ...
}

I don't like the use of URI_SUFFIX_FORMAT in the above code because I cannot tell what that String.format is trying to compose and if the number of Strings passed in is correct or not.

I prefer to do this instead:

private String formatUriWithSuffix(String dir, String service)
{
    return uriBuilder.buildContentUri(dir, String.format("view/%s", service));
}

public void buildUriForMusic(String dir)
{
    ...
    String ret = formatUriWithSuffix(dir, "music");
    ...
}

public void buildUriForBook(String dir)
{
    ...
    String ret = formatUriWithSuffix(dir, "book");
    ...
}

What do you think? Which one is better, for what reasons? Do you have a better approach?

Sure, your method is better. It's a pretty straightforward example of some fundamental programming and design concepts:

You spotted repetition. The same information is being stated multiple times. In this case, the information is how to perform a particular operation for formatting a string. Generally, you should not repeat yourself.
This clued you in to an abstraction: the implementation of this formatting operation can be abstracted out behind a meaningful name
With this knowledge you abstracted out the operation by applying an extract method refactoring.
As a result, you now have code which is:

More readable, since the methods are simpler, and the name of the formatting method explains exactly what its purpose is.
More maintainable, since now any change to this formatting operation just has one place to occur, rather than having to be made in all the places that it's repeated


React wrapper for existing library https://github.com/BingXiong1995/react-flv-player/blob/master/lib/wrapper/ReactFlvPlayer.js
import React, { Component } from 'react';
import flvjs from './flv.min';
import PropTypes from 'prop-types';

class ReactFlvPlayer extends Component {
  constructor(props) {
    super(props);
    this.myRef = React.createRef();
    this.flvPlayerRef = element => {
      this.flvPlayerRef = element;
    };
  }

  componentDidMount() {

    const {type , url, isLive, enableStashBuffer, stashInitialSize, hasAudio, hasVideo, handleError, enableWarning, enableError} = this.props;

    // 组件挂载后，拿到Ref进行操作
    if (flvjs.isSupported()) {
      const flvPlayer = flvjs.createPlayer({
        type,
        isLive,
        url,
        hasAudio,
        hasVideo
      },{
        enableStashBuffer,
        stashInitialSize
      });

      flvjs.LoggingControl.enableError = false;
      flvjs.LoggingControl.enableWarn = enableWarning;

      flvPlayer.attachMediaElement(this.myRef.current); // 将这个DOM付给第三方库
      flvPlayer.load();
      flvPlayer.play();
      flvPlayer.on('error', (err)=>{
        // console.log(err);
        handleError(err);
      });
    }
  }

  render() {
    const { height, width, isMuted,showControls } = this.props;
    return (
      <div>
        <video
          controls={showControls}
          muted={{isMuted}}
          ref={this.myRef}
          style={{height, width}}
        />
      </div>
    );
  }
}

ReactFlvPlayer.propTypes = {
  type: PropTypes.string,
  url: PropTypes.string.isRequired,
  isLive: PropTypes.bool,
  showControls: PropTypes.bool,
  hasAudio: PropTypes.bool,
  hasVideo: PropTypes.bool,
  enableStashBuffer: PropTypes.bool,
  stashInitialSize: PropTypes.number,
  height: PropTypes.string,
  width: PropTypes.string,
  isMuted: PropTypes.bool,
  enableWarning: PropTypes.bool,
  enableError: PropTypes.bool,
  handleError: PropTypes.func
};

ReactFlvPlayer.defaultProps = {
  type: 'flv',
  isLive: true,
  hasAudio: true,
  hasVideo: true,
  showControls: true,
  enableStashBuffer: true,
  stashInitialSize: 128,
  height: '100%',
  width: '100%',
  isMuted: false,
  handleError: (err)=>{console.log(err)},
  enableWarning: false,
  enableError: false
};

export default ReactFlvPlayer;

Wrote some wrapper a long time ago. I am wondering if I could have done it in a better way. What are some of the improvements I could make or issues with the code. Thanks.

flvPlayerRef?
In the constructor, you have
this.myRef = React.createRef();
this.flvPlayerRef = element => {
  this.flvPlayerRef = element;
};

This is pretty confusing. The property is either a function or an element, depending on whether it was called as a function before, and either way, it's not a ref, so it's also misnamed. It also isn't used anywhere else in the code, and consumers of the instance already can get a reference to the <video> element through the myRef property.
I'd remove flvPlayerRef completely, and also rename the less-than-informative myRef property name to videoRef or to flvPlayerRef.
At that point, you can make things concise by using class fields instead of a constructor:
class ReactFlvPlayer extends Component {
  videoRef = React.createRef();

  componentDidMount() {
    // ...

You can also consider using a functional component instead of a class-based component, as React tentatively recommends for new code - but that's not required.
Destructured props
This line is hard to read:
const {type , url, isLive, enableStashBuffer, stashInitialSize, hasAudio, hasVideo, handleError, enableWarning, enableError} = this.props;

When there are more than 2 or 3 properties to be destructured, I'd recommend putting each on a separate line
const {
  type,
  url,
  isLive,
  // ...
} = this.props;

But, in this case, a significant fraction of the properties are used only to be passed into flvjs.createPlayer later. Consider using rest syntax to collect those options into a single object, without having to specify each one individually:
const {
  enableStashBuffer,
  stashInitialSize,
  handleError,
  enableWarning,
  enableError,
  ...createPlayerOptions
} = this.props;

The enableError variable is not used. If that's deliberate, best to not extract it from the props in the first place. Or maybe you meant to assign it to LoggingControl? Change
flvjs.LoggingControl.enableError = false;

to
flvjs.LoggingControl.enableError = enableError;

Nicer indentation Rather than creating another indentation block after checking if flvjs is supported, you can consider returning early if it's not supported:
componentDidMount() {
  if (!flvjs.isSupported()) {
    return;
  }
  const {
    enableStashBuffer,
    stashInitialSize,
    handleError,
    enableWarning,
    enableError,
    ...createPlayerOptions
  } = this.props;

  const flvPlayer = flvjs.createPlayer(
    createPlayerOptions,
    {
      enableStashBuffer,
      stashInitialSize
    }
  );
  // etc

Returning early is pretty nice, especially with more complex logic which would otherwise require multiple levels of indentation, which can get pretty hard to read.
Spacing There are a few places where I'd expect to see a space but don't see any given the code style in the rest of the script, or where I see spaces where there probably shouldn't be any, like const {type , url,, },{, (err)=>{, const { height, width, (do you want leading/trailing space when destructuring and with objects, or no?).
Whatever you want your code style to be, it would be good to be consistent - consider using ESLint to keep your style consistent, to fix things automatically, and warn you of potential bugs before they turn into runtime errors.


Formatting mm:ss to string with Javascript I have a function that takes a time of minutes and seconds, and converts it to a string in the following way: '0:30' would become '30 Seconds', '1:30' would become '1 minute 30 seconds', '2:01' would become '2 minutes 1 second', etc.  My function works fine, but I'm wondering if there are areas for improvement.

function formatTime(formattedTime) {
  let minStr, secStr;
  let minNum = formattedTime.split(':')[0];
  let secNum = formattedTime.split(':')[1];
  if (minNum === '1') {
    minStr = 'minute';
  } else {
    minStr = 'minutes';
  }
  if (secNum === '01') {
    secStr = 'second';
    secNum = '1';
  } else {
    secStr = 'seconds';
  }
  if (minNum === '0') {
    return `${secNum} ${secStr}`;
  } else if (secNum === '00') {
    return `${minNum} ${minStr}`;
  } else {
    return `${minNum} ${minStr} ${secNum} ${secStr}`;
 }
}

I made a few changes to the function that I think improve readability.  By using parseInt() on the seconds, I eliminated the need to later set the secNum from 01 to 1.  Since we no longer need to change that, the minStr and secStr can be set with a ternary operator. I also used destructuring to clean up the splitting of the time.

To make it less error prone, you'd also want to make sure to do some sanity checks on the input, as mentioned in the comments.

All told, it ended up looking like this:

// Tests
console.log(formatTime('1:00'))
console.log(formatTime('1:01'))
console.log(formatTime('1:30'))
console.log(formatTime('0:30'))
console.log(formatTime('bad data'))

function formatTime(formattedTime) {
    // Set up minutes and seconds
    const [match, minNum, secNum] = formattedTime.match(/(\d+):(\d+)/) || [];

    // Make sure we're getting the data in correct
    if(!match) return false;

    // Get the correct minutes and seconds strings
    let minStr = (minNum === '1') ? 'minute' : 'minutes'
    let secStr = (parseInt(secNum) === 1) ? 'second' : 'seconds'

    // Return the formatted time
    if (parseInt(minNum) === 0) {
        return `${secNum} ${secStr}`;
    } else if (parseInt(secNum) === 0) {
        return `${minNum} ${minStr}`;
    } else {
        return `${minNum} ${minStr} ${parseInt(secNum)} ${secStr}`;
    }
}

This is making use of some ES6 additions, so if you want good browser support you'll need to make sure to run this through Babel.


SICP - exercise 2.5 - representing pairs of nonnegative integers using only numbers and arithmetic operations From SICP

Exercise 2.5: Show that we can represent pairs of nonnegative integers using only numbers and arithmetic operations if we represent the pair a and b as the integer that is the product 2^x*3^y. Give the corresponding definitions of the procedures cons, car, and cdr.

Please review my code.

(define (cons x y)
    (* (expt 2 x) (expt 3 y)))

I created repeat-divide to find x and y.

(define (repeat-divide x y)
    (if (> (remainder x y) 0)
        0
    (+ 1 (repeat-divide (/ x y) y))))

Selectors

(define (car p) (repeat-divide p 2))

(define (cdr p) (repeat-divide p 3))

I am not mathematically inclined, so I feel this solution might be extremely inefficient. How can I make this code better and more efficient?

Well this is going to be inefficient no matter what, it's just that way. So we aren't going to improve the built-in expt, so that leaves repeat-divide for improvement. Let call it de-factor.

So first I thought something like

(define (de-factor p n)
 (let* ((max-expt (ceiling (/ (log p) (log n))))
             ;;log of p in base n
        (gcd-of (gcd p (expt n max-expt)))
        (res (/ (log gcd-of) (log n))))
   res))

which works so long as log internal is accurate enough to return the closest integer.It runs into significant rounding errors for some x and y less than a hundred.  If you built a very large log table you could solve in log time so long as you could accurately cast from your log table to the input x and y. (gcd runs in log time) Of course in such case the memory overhead of such a table or map would be huge, so not really that great of an optimization.

(define (de-factor p n)
 (let* ((rough-max-expt (let loop ((x 1))
                           (if (>= (expt n x) p)
                               x
                               (loop (* 2 x)))))
             ;;can't use log trick, returns inexact math
        (gcd-of (gcd p (expt n rough-max-expt))))
    (x-to-what-y n gcd-of)))

(define (x-to-what-y x n) ;return y such that (= n (expt x y)) is #t
 (if (= n 1)
     0
    (let loop ((y 1) (step 1) (acc x) (narrow? #f))
      (cond ((= acc n) y)
            ((< step 1) (error x step acc narrow?))
            (else (let ((next (* acc (expt x step))))
                     (if (<= next n)
                         (loop (+ y step)
                               (if narrow? (/ step 2) (* step 2))
                               next
                               narrow?)
                         (loop y
                               (/ step 2)
                               acc
                               #t))))))))

Minimal changes to the rest

(define (cons x y)
    (* (expt 2 x) (expt 3 y)))

(define (car p) (de-factor p 2))

(define (cdr p) (de-factor p 3))

As far as performance it's still less than linear because of the exact bignum math. (cdr (cons 12345 54321)) returns 12345 in a few seconds, but (cdr (cons 123456 654321)) in about a minute and a half.

When I tried your (cdr (car 12345 54321)) it ran out of memory, with a basic tail-recursion optimization, it took about 10 seconds. (cdr (cons 123456 654321)) is about a half hour. So my code is an optimization, but really just the nature of that big of numbers is they are very inefficient to do a lot of math on, and even more so the bigger they get. You would never do something this weird in production code.

Tail optimization for benchmarking

(define (repeat-divide x y)
  (let loop ((acc 0) (x x))
    (if (> (remainder x y) 0)
        acc
        (loop (+ 1 acc) (/ x y)))))


Refactoring a simple Ruby CLI program I tried to help someone in Stackoverflow with a refactoring exercise. Did many changes to his original code and made a somewhat decent solution (at least in my eyes). Was thinking, whether someone can critique on my implementation.

class Theatre
  COST = { running: 3, fixed: 180 }

  attr_accessor :number_of_audience, :ticket_price

  def revenue
    @number_of_audience * @ticket_price
  end

  def total_cost
    COST[:fixed] + (@number_of_audience * COST[:running])
  end

  def net
    revenue - total_cost
  end

  def profit?
    net > 0
  end
end

class TheatreCLI
  def initialize
    @theatre = Theatre.new
  end

  def seek_number_of_attendes
    print 'Number of audience: '
    @theatre.number_of_audience = gets.chomp.to_i
  end

  def seek_ticket_price
    print 'Ticket price: '
    @theatre.ticket_price = gets.chomp.to_i
  end

  def print_revenue
    puts "Revenue for the theatre is RM #{@theatre.revenue}."
  end

  def print_profit
    message_prefix = @theatre.profit? ? 'Profit made' : 'Loss incurred'
    puts "#{message_prefix} #{@theatre.net.abs}."
  end

  def self.run
    TheatreCLI.new.instance_eval do
      seek_ticket_price
      seek_number_of_attendes

      print_revenue
      print_profit
    end
  end
end

TheatreCLI.run

It's very strange you're using instance_eval. That's not how a regular program should work and is not needed in your code.

If you want to avoid calling 4 instance methods from a class instance method, then you can create a new instance method like:

def workflow
  seek_ticket_price
  seek_number_of_attendes

  print_revenue
  print_profit
end
def self.run
  TheatreCLI.new.workflow
end

Maybe it's not a problem here, but using a lot of instance_eval for saving keystrokes looks like working around a bad API.  Also my gut feeling tells me that such practice can lead to unexpected troubles if used outside class body.


Is there a way to simple Haskell class instance? I define a Pixel data with different size like this.

data Pixel = RGB8 Word8 Word8 Word8
           | RGBA8 Word8 Word8 Word8 Word8
           | RGB16 Word16 Word16 Word16
           | RGBA16 Word16 Word16 Word16 Word16
           | Empty
           deriving (Eq)

and Num instance

instance Num Pixel where
  x + Empty = x
  Empty + x = x
  (RGB8 ru gu bu) + (RGB8 rv gv bv) = RGB8 (ru + rv) (gu + gv) (bu + bv)
  (RGBA8 ru gu bu au) + (RGBA8 rv gv bv av) = RGBA8 (ru + rv) (gu + gv) (bu + bv) (au + av)
  (RGB16 ru gu bu) + (RGB16 rv gv bv) = RGB16 (ru + rv) (gu + gv) (bu + bv)
  (RGBA16 ru gu bu au) + (RGBA16 rv gv bv av) = RGBA16 (ru + rv) (gu + gv) (bu + bv) (au + av)
  x - Empty = x
  Empty - x = x
  (RGB8 ru gu bu) - (RGB8 rv gv bv) = RGB8 (ru - rv) (gu - gv) (bu - bv)
  (RGB16 ru gu bu) - (RGB16 rv gv bv) = RGB16 (ru - rv) (gu - gv) (bu - bv)
  (RGBA8 ru gu bu au) - (RGBA8 rv gv bv av) = RGBA8 (ru - rv) (gu - gv) (bu - bv) (au - av)
  (RGBA16 ru gu bu au) - (RGBA16 rv gv bv av) = RGBA16 (ru - rv) (gu - gv) (bu - bv) (au - av)
  _ * _ = error "Not support multiply"
  abs  = id
  signum _ = error "Not support signum"
  fromInteger _ = error "Not support fromInteger"

It's very ugly and bored. I think Haskell is a Elegant language, so can I have a better style to write this?

Above all, such an instance doesn't make any sense. A composite entity can't have much in common with a number, which the fact that your instance consists of just partial and undefined functions is only the evidence of.

With regards to your concern about the verbosity of the instance declaration, it was caused by you choosing an incorrect approach to type declaration.

Pixels of different bit-depths are appropriately described as different types, and not different constructors of the same type. The analogy is right before you: Word8 and Word16 are different types, not constructors.

Taking the said above into account I recommend you to reapproach your problem the following way:

-- The separate datatypes. Note the absense of `Empty` constructors, as there
-- is no such thing as empty pixel. There are black and transparent pixels,
-- which should be described as `PixelRGB8 0 0 0` or `PixelRGBA8 0 0 0 0`,
-- but not empty. And for dealing with missing pixels we have a `Maybe` type.
data PixelRGB8 = PixelRGB8 Word8 Word8 Word8
-- ...
data PixelRGBA16 = PixelRGBA16 Word16 Word16 Word16 Word16

-- We're taking an analogous approach to Num/Integral typeclasses, while
-- combining both the "to" and "from" functionality in a single typeclass.
class Pixel a where
  -- whether its toPixelRGBA16 or possibly toRGBA64Pixel should depend on
  -- the highest precision type you want to be dealing with
  toPixelRGBA16 :: a -> PixelRGBA16
  fromPixelRGBA16 :: PixelRGBA16 -> a
  -- And here's addition for pixels of same type:
  addPixel :: a -> a -> a

instance Pixel PixelRGB8 where
  toPixelRGBA16 = error "TODO"
  fromPixelRGBA16 = error "TODO"
  addPixel = error "TODO"

instance Pixel PixelRGBA16 where
  toPixelRGBA16 = id
  fromPixelRGBA16 = id
  addPixel = error "TODO"

-- Here's addition for pixels of differing types:
addArbitraryPixel :: (Pixel a, Pixel b) => a -> b -> PixelRGBA16
addArbitraryPixel a b = addPixel (toPixelRGBA16 a) (toPixelRGBA16 b)


Get metadata from an Icecast radio stream I am new to Python and not very familiar with advanced Python data structures.

I have written a function to receive data from a socket in Python and perform string manipulations on it. The basic purpose of the code is to get the metadata from an Icecast radio stream based on suggestions I found elsewhere.

The function seems to work but it would be great it someone can help me optimize the string operation.

def radioPoller():
    msg1="GET / HTTP/1.1"+'\r'+'\n'+"Host: sc.buddharadio.com"+'\r'+'\n'+"User-Agent: VLC/2.0.5 LibVLC/2.0.5"+'\r'+'\n'+"Range: bytes=0-"+'\r'+'\n'+"Connection: close"+'\r'+'\n'+"Icy-MetaData: 1"+'\r'+'\n'+'\r'+'\n'

    if os.path.exists("/media/hdd1/data.txt"):
        os.unlink("/media/hdd1/data.txt")

    HOST = 'sc.buddharadio.com'                      # The remote host
    PORT = 80                                # The same port as used by the server
    s = socket.socket(socket.AF_INET, socket.SOCK_STREAM)
    s.connect((HOST, PORT))

    f=open("/media/hdd1/data.txt","w")

    s.send(msg1)                                 #sending http request
    data=""
    while len(data)<1024:
        data=s.recv(1024)               #recieving a few characters

    t=data.find("metaint")                       #finding the metaint response header which contains interval after which metadata will be visible in data stream eg:-icy-metaint:32768
    data=data[t+8:]                      #jumping 8 characters to the end of the string

    splitter="\r\n"                         # to split the contents and find when header ends and
    data1=data.split(splitter)                      #data1 stores the splitted data. First list has length and last has data

    metaInt=int(data1[0])                      #find the metadata interval byte length
    #print metaInt

    string12=data1[len(data1)-1]                #contains only the data part

    lengthTotal= len(string12)                  #length of data we have

    print "total data is", lengthTotal,"\n"

    print "TOtal bytes we should get is", (metaInt+4080),"\n"

    while lengthTotal<(metaInt+4080):
        data = s.recv(8192)
        lengthTotal=lengthTotal+len(data)
        print "Total length now is", lengthTotal
        string12=string12+data
        #The first character after MetaInterval number of characters define the length of actual metadata/16
    metaLen=ord(string12[metaInt])
    print "This is multiplier", metaLen, "No of characters to read ", (metaLen*16)

    metaString=string12[metaInt+1:metaInt+1+(metaLen*16)]+'\n'
    print "Extracted String is \n", metaString

    f.write(str(metaString))
    s.close
    f.close

The code sends a request header to the radio stream and then receives a response like this:

  ICY 200 OK
  icy-notice1:This stream requires Winamp
  icy-notice2:SHOUTcast Distributed Network Audio Server/Linux v1.9.8
  icy-name:Buddha
  icy-genre:
  icy-url:http://www.buddharadio.com
  content-type:audio/aac
  icy-pub:0
  icy-metaint:32768
  icy-br:32

The icy-metaint defines the data interval after the metadata appears in the data stream.

I use a while loop to read in the Meta Data Interval+Max Meta Data Length of data and then slice the metadata and save it in a file.

def radioPoller():

Python convention says that function names should be lowercase_with_undescores. Also, they should be verb, so: poll_radio would be better

    s = socket.socket(socket.AF_INET, socket.SOCK_STREAM)
    s.connect((HOST, PORT))

I recommend not using s as its not obvious what that means.

    finalData=[]

Python convention, local names should be lowercase_with_underscores.

    f=open("/media/hdd1/data.txt","a")

Again, don't use single letter variable names.

    s.send(msg1)                                 #sending http request

Instead of doing HTTP requests yourself, it'd make more sense to use urllib2 to do it for you. That way it'd take care of parsing etc.
Where is msg1 coming from? Also, you don't know that this sent all the data. To be sure all data was sent, use s.sendall

    data=""
    while len(data)<1024:
        data=s.recv(1024)               #recieving a few characters

Shouldn't you be using +=?

    t=data.find("metaint")                       #finding the metaint response header
    data=data[t+8:]                      #jumping 8 characters to the end of the string

    splitter="\r\n"                         # to split the contents and find when header ends and
    data1=data.split(splitter)                      #data1 stores the splitted data. First list has length and last has data

Why assign splitter to a local variable? just pass the constant.

    metaInt=int(data1[0])                      #find the metadata interval byte length
    #print metaInt

Rather then mucking with this use, use python's tool to parse the headers for you.

    finalData.append(data1[len(data1)-1])           #contains only the data part

You can use data1[-1] for the same result.

    lengthTotal= len(finalData[0])                  #length of data we have

    #print "total data is", lengthTotal,"\n"

    #print "TOtal bytes we should get is", (metaInt+4080),"\n"

Don't leave dead code in comments

    while lengthTotal<(metaInt+4080):
        data = s.recv(8192)

        lengthTotal=lengthTotal+len(data)
        #print "Total length now is", lengthTotal
        finalData.append(data)

In this case I suggest using StringIO rather than a list.

    print finalData
    string12=''.join(finalData)

Don't use meaningless names

    metaLen=ord(string12[metaInt])
    #print "This is multiplier", metaLen, "No of characters to read ", (metaLen*16)

    metaString=string12[metaInt+1:metaInt+1+(metaLen*16)]
    metaString=metaString+"\r\n"

    #print "Extracted String is \n", metaString

    f.write(str(metaString))

It's already a string, don't pass it to str

    s.close
    f.close

These last two don't do anything.

Here's my reworking of your code:

def parse_headers(response):
    headers = {}
    while True:
        line = response.readline()
        if line == '\r\n':
            break # end of headers
        if ':' in line:
            key, value = line.split(':', 1)
            headers[key] = value
    return headers

def poll_radio():
    request = urllib2.Request("http://sc.buddharadio.com:80/", headers = {
        'User-Agent' : 'User-Agent: VLC/2.0.5 LibVLC/2.0.5',
        'Icy-MetaData' : '1',
        'Range' : 'bytes=0-',
    })
    # the connection will be close on exit from with block
    with contextlib.closing(urllib2.urlopen(request)) as response:

        headers = parse_headers(response)

        meta_interval = int(headers['icy-metaint'])
        response.read(meta_interval) # throw away the data until the meta interval

        length = ord(response.read(1)) * 16 # length is encoded in the stream
        metadata = response.read(length)
        print metadata


Is using java enums to store filepaths good practice I'm working on an android application, but this is basically plain 'ole java.

The aim is to do the right thing and not repeat myself.

public enum Datafile {
    ACTIVITIES() {
        @Override public String filepath() {
            return "activities.yaml";
        }
    },
    OPEN_LOOPS() {
        @Override public String filepath() {
            return "open_loops.yaml";
        }
    };

    public abstract String filepath();
}

Later on, when I want to edit the file I try this

File file = getDataFile(Datafile.OPEN_LOOPS);

Which calls this...

private File getDataFile(Datafile datafile) {
    File file = null;

    String baseDir = Environment.getExternalStorageDirectory().getAbsolutePath();
    file = new File(baseDir + "/" + datafile.filepath());

    return file;
}

Points for a better name for the enum and good advice regarding who responsibility for the getFileData method (currently all this code is in an Activity) really lies (a new class).

Enum are a good way to do this in my opinion if this is really static.

Might I suggest you do:

public enum Datafile {
    ACTIVITIES("activities.yaml"),
    OPEN_LOOPS("open_loops.yaml");

private String filepath;

private Datafile(String filepath) {
    this.filepath = filepath;
}

public String getFilepath() {
    return filepath;
}

This is a bit more readable than the overriden methods in each declaration, especially with long enums.

If the values of the filepaths are subject to changes from time to time. I suggest using a properties file. These can be reloaded without recompiling you package, meaning non-developers could maintain them. That is useful in some team setups.

EDIT:
For properties, I suggest you read the java properties tutorial: http://download.oracle.com/javase/tutorial/essential/environment/properties.html. There are also a lot of already well written tutorials about them and I could never hope to match their quality here. Also, never having worked on Android, I do not know if google has provided something that is more user friendly.

For small applications, I would go for enums personally, as they are very easy to manage and usually do the job pretty well.


Count the occurence of nucleobases in DNA string Inspired by this meta question I decided to take a look at Rosalind. Their first challenge seemed easy enough:

An example of a length 21 DNA string (whose alphabet contains the symbols 'A', 'C', 'G', and 'T') is "ATGCTTCAGAAAGGTCTTACG."
Given: A DNA string s of length at most 1000 nt.
Return: Four integers (separated by spaces) counting the respective number of times that the symbols 'A', 'C', 'G', and 'T' occur in s.
Sample Dataset
AGCTTTTCATTCTGACTGCAACGGGCAATATGTCTCTGTGTGGATTAAAAAAAGAGTGTCTGATAGCAGC

Sample Output
20 12 17 21

Since I'm still on my quest to learn both regex and Ruby, I decided to go that route:
def countACGT(str)
  list = [0,0,0,0]
  str.scan(/A|C|G|T/) do |sub|
    if sub == "A"
      list[0] += 1
    end
    if sub == "C"
      list[1] += 1
    end
    if sub == "G"
      list[2] += 1
    end
    if sub == "T"
      list[3] += 1
    end
  end
  return list
end

I'm not a big fan of long if chains. Luckily, Ruby has a case statement as well:
def countACGT(str)
  list = [0,0,0,0]
  str.scan(/A|C|G|T/) do |sub|
    puts case sub
    when "A"
      list[0] += 1
    when "C"
      list[1] += 1
    when "G"
      list[2] += 1
    when "T"
      list[3] += 1
    end
  end
  return list
end

Both can be invoked like:
p countACGT("TCCCACTTCAGGGTCAGGGAGCTCCAAACTCTCTTTCTAGAGATGACAATCGAGAGTGAGATAAGGTGGATAGCAATCGTTATGGGATGTAAGCGCCAAGCGTTCGGGTAGCCCACGTTGCGGGCTAATCGCTAGGCTAGAACCTCTAAGCTGTACTTCTGTCAAAACGGAAAGAATCATACCGCACACCAACACTCGATGTAATGTAAGGATATCCTGTGCAGATGAGGTGCTTGGTACGCTAGATACTAGTATTACTAACACACAACATTACCGCCCAAGCGTGTCAGCCACGGACCAGATGACTCTTGCCGATTGAATACCTATCATCCTTACGGTCCGGAATCAGTATATCGCGTGCACAGTTACAGTGGTTAACTTGAGCTAGAGCAAGATAATGTGCGATCTGCGCACTCGGTGGGCTTGGATCACCCTACTTCCAATTGCCCGCGTATGATAGTTCCACCACTCACAAGTCTGTCATAGTGATTATCAAGAGTAGGCGTAGTGGGCACCCAAGAAATTAATGAATCTCACAGTCGAGTGTATCTTCGGCCATATCCCTACGGCAAATGGTCGCTCAGCTTGTCTCCGAGAGTTCGTTGGTTCAGAACCTCCGAAGGGTTGGGTGATTGTTGCGGCGCGCATGCGAGCTATGGTGGCTGTGTGTGGAGGTATTATCAGGGGAAATTTATTCCGAGTACTTGCTTGACTGCTCTTTTGTAAGCCGTTTGGGGTGCGTCCTCTGTATAGTCGTCGCCGCGAAGCCGATTCCCTCTAATCAAACACGCCAGAGGATCACTGGTCTTCTTCAAATTCCTGTATACCTCTGGCTAAATGACCCGACGGTACGAGCGTTTACTTCGAAGTG")

Yes, the dataset I had to solve was that long.
However, my interpreter suddenly feels the need to print the updated list[x] every time a new value gets assigned. This leads to a significant decrease in performance.
So I downloaded the official Ruby interpreter via the downloader (version 2.2.3 (x64)) and the exact same thing happens.
It almost looks like case is not the preferred way of doing this, but that's not intuitive.
I'm mainly looking for a definitive answer on which version I should stick with (and why) and general maintainability improvements. I'm perfectly aware regex may not be the optimal solution here, but I'd like to stick with it anyway.

Your code is fine and readable from a C/Java perspective. I don't think it's particularly Ruby-ic to use return statements. Just put list at the end on its own.

Why your case is slow

You have this extra puts here:

str.scan(/A|C|G|T/) do |sub|
  puts case sub
  ^^^^
  when "A"
     ...

You may want to get rid of that :) That's why you interpreter prints the list every time. You told it to...

Functionally better

But functionally, we can do way better. Ruby's enumerables have a group_by method:

def countACGT(str)
    str.chars.group_by(&:chr)
end

That'll give you a hash from each key (nucleotide base) to the values in the collection (a list of each occurence of each nucleotide base). All you have to do then is map it to just give you the size:

def countACGT(str)
    str.chars.group_by(&:chr).map { |k, v| [k, v.size] }
end

That'll give you, for your example, the list:

[["T", 228], ["C", 209], ["A", 214], ["G", 220]]

If you want to get it in ACGT order like your original, you can just sort it and map off the key:

def countACGT(str)
    str.chars.group_by(&:chr).sort.map{|k, v| v.size}
end


BigInteger check in C from a string I have started to learn C. At the time of writing I don't know anything about the language. The following I wrote today in a few hours. Though I do have a lot of programming experience, I find C hard to learn. That may be only a temporary impression, however.

The purpose of the following function is to determine if the string, whatever its length is, contains an integer number. There might be many ways, and/or I might have done some rookie mistakes.

#include <stdbool.h> // bool type, true / false values, ...
#include <stdio.h> // printf()
#include <ctype.h> // isdigit()
#include <string.h> // strlen(), strncmp(), ...

bool string_contains_integer(const char *str)
/*
    This function iterates through an array of chars,
    and checks each character to be a digit.

    Returns true if the string contains a number (string of digits);
    Returns false if the string contains another character(s).

    Starting "-" gets ignored, as we accept negative numbers too.
*/
{
    // I'd like to avoid repeated strlen() calls
    int string_length = strlen(str);

    // if the string is empty, return right away
    if (string_length == 0)
        return false;

    bool negative_sign = false;

    // iterate through all characters in the string
    for (int i = 0; i < string_length; i++)
    {
        // extract character on index
        char chr = str[i];

        // repeated presence of negative sign is handled here
        if (strncmp("-", &chr, 1) == 0)
        {
            // if the negative sign gets detected
            // for the second time, return right away
            if (negative_sign)
                return false;

            // once a negative sign is detected, we make note of it here
            negative_sign = true;

            // now we want to skip the check for a digit
            continue;
        }

        // this is actually the core part checking on digits
        if (!isdigit(chr))
            return false;
    }

    // If we got here, then all the characters are digits,
    // possibly starting with a negative sign.
    return true;
}

int main()
{
    // the function can handle very big numbers as shown here
    if (string_contains_integer("-123456789123456789123456789123456789123456789123456789123456789"))
    {
        printf("%s\n", "PASS: Input is a number.");
    }
    else
    {
        printf("%s\n", "FAIL: Input is not a number!");
    }
}

This program I compile on Linux Mint 19.1 as follows:

gcc-8 -std=c18 -Wall -Wextra -Werror -Wpedantic -pedantic-errors -o bigInteger bigInteger.c

Would you be that kind to review the code? However, there might be alternative ways of doing the same thing, you may, of course, suggest them, but my primary goal now is to check upon my learning curve and to fix bugs of course, optimize this code, etc.

Good observation

Many new C programmers do not realize that n = strlen(s) when called in a loop can take code from linear performance O(n) to worse O(n*n) performance when  s may change or with weak compilers.

// I'd like to avoid repeated strlen() calls
int string_length = strlen(str);  // Good!!

Good use of const

Nice. string_contains_integer(const char *str)

Nicely formatted code

I hope you are using an auto formatter.  Life is too short for manual formatting.

"whatever its length is"

Strings can have a length that exceeds INT_MAX.  Note that strlen() returns type size_t.  That is the right-sized type for array indexing and sizing.  Note: size_t is some unsigned type.

// int string_length = strlen(str);
// for (int i = 0; i < string_length; i++)

size_t string_length = strlen(str);
for (size_t i = 0; i < string_length; i++)

Expensive sign compare

Rather than call strncmp(), just compare characters.

// char chr = str[i];
// if (strncmp("-", &chr, 1) == 0)
if (str[i] == '-')

OP's code is tricky here.  strncmp() can handle non-strings (which is what &chr points to) as long as they do not exceed n.  With n==1, code is effectively doing a char compare.

Sign at the beginning

Conversion of strings to numeric values allow a leading sign, either '-', '+' or none.  This code errantly allows a '-' someplace.  @Martin R.

Just start with

bool string_contains_integer(const char *str) {
  if (*str == '-' || *str == '+') {
    str++;
  }
  ....

  // Other sign code not needed

strlen() not needed.

Rather than i < string_length, just test str[i] != '\0'.  This speeds up detection of non-numeric strings as the entire string does not need to be read.

is...(int ch) quirk

is...() functions are specified for int values in the unsigned char range and EOF.  On common platforms where char is signed, isdigit(chr) risks undefined behavior (UB) when chr < 0.  Best to cast here.

// isdigit(chr)
isdigit((unsigned char) chr)

{ block }

Of coding style, I recommend to always use {} after if,else, even if one line.

//if (string_length == 0)
//    return false;

if (string_length == 0) {
    return false;
}

Minor: Simplification

To simply prints a line, code could use puts().  But what you did is OK.  Note puts() appends a '\n'.

// printf("%s\n", "PASS: Input is a number.");
puts("PASS: Input is a number.");

Alternative

This does not use array indexing.  Just increment the pointer as needed.

bool string_contains_integer_alt(const char *str) {
  if (*str == '-' || *str == '+') {
    str++;
  }

  if (*str == '\0') {
    return false;
  }

  while (isdigit((unsigned char)*str)) {
    str++;
  }

  return *str == '\0';
}


Using get-setf-expansion In a previous post at Simplifying complex setf expressions, @sds suggested using the get-setf-expansion macro for implementing place modification, as discussed in On Lisp (p171ff) by Paul Graham . In a related post at Destructive place-modifying operators, @acelent provides additional guidance. Based on their advice, I've written a place modifying macro (below), and would appreciate any comments, improvements, corrections, or error checking procedures. It has passed the preliminary tests I have performed.  However, my experience with macros, is limited, and I know their workings can be subtle.

The macro is intended to perform (and return) an in-place modification based on an arbitrary common-lisp function. A simple example would be (! 1+ @x) == (incf x). The ! signifies modification, and @ indicates the place being modified (and returned). A more complex example is (! delete 3 @(car x) :test #'equal) == (setf (car x) (delete 3 (car x) :test #'equal)). I've had to include the @ marker for the place, since I can't think of another way to pass the place on to (get-setf-expansion place). (Is there any way to analyze the features of the input function in order to zero in on the appropriate place argument?) Otherwise, Paul Graham's macro template (he calls it _f) seems to work, if you substitute get-setf-expansion for his get-setf-method.

(defmacro ! (fn &rest args)
  "Modifies an object at a place according to a given function.
   Obviates the need for some specialized macros--eg, (incf x) == (! 1+ @x);
   and generalizes place modification for standard common-lisp functions."
  (let* ((sym (find-if #'(lambda (arg)  ;find the place argument signaled by @
                           (and (symbolp arg) (string= (aref (symbol-name arg) 0) "@")))
                       args))
         (fn-args (cond ((eq sym '@)  ;sym indicates a generalized reference
                         (remove '@ args))
                        ((> (length (symbol-name sym)) 1)  ;sym indicates a variable
                         (subst (find-symbol (subseq (symbol-name sym) 1)) sym args))
                        (t (error "Modify macro call ~A is ill-formed."
                                  (cons '! (cons fn args))))))
         (place (if (eq sym '@)
                    (elt args (1+ (position '@ args)))  ;generalized reference
                  (find-symbol (subseq (symbol-name sym) 1)))))  ;normal variable
    (multiple-value-bind (vars forms var set access)
        (get-setf-expansion place)
      ;(ut::prt fn args sym fn-args place vars forms var set access)
      (declare (ignore access))
      `(let* (,@(mapcar #'list vars forms)
                 (,(car var) (,fn ,@fn-args)))
         ,set))))

First things first, you should know about the order of evaluation in modifying forms:

  (operator preceding-form* place following-form*)

  The evaluation of each such form proceeds like this:

  Evaluate each of the preceding-forms, in left-to-right order.
  Evaluate the subforms of the place, in the order specified by the second value of the setf expansion for that place.
  Evaluate each of the following-forms, in left-to-right order.
  Read the old value from place.
  Compute the new value.
  Store the new value into place.

I'll take your idea with these changes:

Instead of !, I'll call it updatef
Instead of @, I'll use an identifying form, (:place <place>), where <place> is the actual place
Instead of splashing the form (e.g. (updatef delete 2 (:place *x*))), I'll make it a single destructuring argument of the macro (e.g. (updatef (delete 2 (:place *x*))))

I'm aware that this syntax is more verbose, but due to the specific nature of updatef's behavior, I think it shouldn't be used just about anywhere.

(defmacro updatef ((&whole form function &rest args) &environment env)
  (flet ((is-place-form (arg)
           (and (consp arg)
                (eq (card arg) :place)
                (consp (cdr arg)))))
    (let ((place-rest (member-if #'is-place-form args)))
      (assert (not (null place-rest)) ()
              "No place form in updatef.")
      (assert (null (member-if #'is-place-form (rest place-rest))) ()
              "More than one place form in updatef.")
      (let* ((preceding-forms (ldiff args place-rest))
             (place (second (first place-rest)))
             (following-forms (rest place-rest))
             (preceding-vars (loop for preceding-form in preceding-forms collect (gensym "preceding-var")))
             (following-vars (loop for following-form in following-forms collect (gensym "following-var"))))
        (multiple-value-bind (vars vals store-vars writer-form reader-form)
            (get-setf-expansion place env)
          (assert (= (length vars) (length vals)) ()
                  "Place expansion has different amount of variables than initializing forms in updatef.")
          `(let* (,@(mapcar #'(lambda (var form)
                                `(,var ,form))
                            (append preceding-vars vars following-vars)
                            (append preceding-forms vals following-forms)))
             (multiple-value-bind (,@store-vars)
                 (,function ,@preceding-vars ,reader-form ,@following-vars)
               ,writer-form)))))))

One of the good things that this macro does is to evaluate the subforms only once.  However, it doesn't help with certain kinds of places where it would be useful to operate atomically, such as setf of gethash or implementation specific atomic operations on places.

Here's a more permissive version that allows certain errors from setf expansions, namely: more or less vars than vals and a writer-form that doesn't return the new value(s).

Also, I made it so it does nothing special (regarding setf behavior) in case no form is (:place <place>), and to use the first such form in case there are more than one.

(defmacro updatef ((&whole form function &rest args) &environment env)
  (flet ((is-place-form (arg)
           (and (consp arg)
                (eq (card arg) :place)
                (consp (cdr arg)))))
    (let ((place-rest (member-if #'is-place-form args)))
      (cond ((null place-rest)
             (warn "No place form in updatef, no update generated.")
             form)
            (t
             (when (not (null (member-if #'is-place-form (rest place-rest))))
               (warn "More than one place form in updatef, first place will be used."))
             (let* ((preceding-forms (ldiff args place-rest))
                    (place (second (first place-rest)))
                    (following-forms (rest place-rest))
                    (preceding-vars (loop for preceding-form in preceding-forms collect (gensym "preceding-var")))
                    (following-vars (loop for following-form in following-forms collect (gensym "following-var"))))
               (multiple-value-bind (vars vals store-vars writer-form reader-form)
                   (get-setf-expansion place env)
                 (when (not (= (length vars) (length vals)))
                   (warn "Place expansion has different amount of variables than initializing forms in updatef."))
                 `(let* (,@(mapcar #'(lambda (var form)
                                       `(,var ,form))
                                   preceding-vars preceding-forms)
                         ,@(mapcar #'(lambda (var form)
                                       `(,var ,form))
                                   vars vals)
                         ;; In case there are more vars than vals
                         ,@(nthcdr (length vals) vars))
                    ;; In case there are more vals than vars
                    ,@(nthcdr (length vars) vals)
                    (let* (,@(mapcar #'(lambda (var form)
                                         `(,var ,form))
                                     following-vars following-forms))
                      (multiple-value-bind (,@store-vars)
                          (,function ,@preceding-vars ,reader-form ,@following-vars)
                        ,writer-form
                        ;; In case writer-form doesn't return the new value(s)
                        (values ,@store-vars)))))))))))

I'm sure that if you try, you can find many more places where to check for errors and warnings, and decide if you're permissive about them or not.  It has been quite a while since I've played with places myself.


Generic database select method for within my application I want to implement a generic Get method for my ASP MVC application, which gets data from an MS Access database.

This is what I have so far, where I am testing it on the Pallet object.

Access

Get

public List<T> Get<T>(List<string> parameters)
{
    query = DatabaseConstants.Select<T>();
    argNames = DatabaseConstants.SelectParameters<T>();
    args = new DynamicParameters();
    foreach (var p in parameters)
    {
        args.Add(argNames[parameters.IndexOf(p)], p);
    }

    return NewSelect<T>(query, args);
}

Select

public List<T> NewSelect<T>(string query, DynamicParameters args = null)
{
    using (var db = new OleDbConnection(connectionString))
    {
        return (List<T>)db.Query<T>(query, args);
    }
}

DatabaseConstants

SelectQuery

public static string SelectQuery<T>()
{
    if (typeof(T) == typeof(Pallet))
    {
        return "SELECT * FROM subGRVReceived WHERE GRVNo=@grv";
    }
    else if (typeof(T) == typeof(Sample))
    {
        return "SELECT * FROM subReceivingQC WHERE GRV=@grv AND PalletSeq=@pallet";
    }
    else if (typeof(T) == typeof(QCItem))
    {
        return "SELECT * FROM subQCItems WHERE GRV=@grv AND PalletSeq=@pallet AND SampleNo=@sampleno";
    }
    else
    {
        return string.Empty;
    }
}

SelectParameters

public static List<string> SelectParameter<T>()
{
    if (typeof(T) == typeof(Pallet))
    {
        return new List<string>() { "@GRV" };
    }
    else if (typeof(T) == typeof(Sample))
    {
        return new List<string>() { "@grv", "@pallet" };
    }
    else if (typeof(T) == typeof(QCItem))
    {
        return new List<string>() { "@grv", "@pallet", "sampleno" };
    }
    else
    {
        return new List<string>();
    }
}

The method can be called like this:

Pallets = access.Get<Pallet>(new List<string>() { grvNumber });

Once I have completed this I will also look into creating generic Insert, Update etc methods. my main goal here is to just reduce the amount of code in the project, as I currently have a seperate method for each database query.

Does anyone have any thoughts or suggestionss on this? Many thanks for taking the time to look!

___________-

I initially wanted to implement an IDatabaseObject containing Query and ParameterNames, where Pallet can inherit from this and store the values inside itself, but I got an error, as the property Pallets is of type List<Pallet>, and C# does not seem to allow me to assign the output of Get<T> to this value when I included a where T : IDatabaseObject.

First things first: your current code has a huge optimization to be made.

foreach (var p in parameters)
{
    args.Add(argNames[parameters.IndexOf(p)], p);
}

This is wrong, because argNames has an indexer and parameters is a List<string>. The IndexOf(T) method you call on parameters to get the index increases the amount of effort, work and time that your program has to perform in order to give you a result when all you really need to do is index the list.

for (int i = 0; i < parameters.Count; i++)
{
    args.Add(argNames[i], parameters[i]);
}

Much simpler. Of course, if the parameters list is larger than argNames, it will throw an exception. You can either keep it that way, or throw and explicit exception (which you should do to inform the user why their code failed).

public List<T> Get<T>(List<string> parameters)
{
    argNames = DatabaseConstants.SelectParameters<T>();
    if (argNames.Length != parameters.Count) // Is argNames a List? If so replace `Length` with `Count`
    {
        throw new ArgumentException($"The argument provided for '{nameof(parameters)}' was not the same length as the required number of parameters for this query.");
    }

    query = DatabaseConstants.Select<T>();
    args = new DynamicParameters();
    foreach (var p in parameters)
    {
        args.Add(argNames[parameters.IndexOf(p)], p);
    }

    return NewSelect<T>(query, args);
}

Next, let's rethink your process.

public static string SelectQuery<T>()
{
    if (typeof(T) == typeof(Pallet))
    {
        return "SELECT * FROM subGRVReceived WHERE GRVNo=@grv";
    }
    else if (typeof(T) == typeof(Sample))
    {
        return "SELECT * FROM subReceivingQC WHERE GRV=@grv AND PalletSeq=@pallet";
    }
    else if (typeof(T) == typeof(QCItem))
    {
        return "SELECT * FROM subQCItems WHERE GRV=@grv AND PalletSeq=@pallet AND SampleNo=@sampleno";
    }
    else
    {
        return string.Empty;
    }
}

This screams for a proper ORM, or at least a fake implementation of one. You also mention in the comments:

  I added some examples, but while doing so I realised that I'm actually going to need to query the same table but with an extra parameter at some point, so this probably wont work .. However as it stands it could be fairly useful (although not as reusable or generic as I might like!)

What if I told you that not only could we make this generic (and generate the query dynamically), but we could solve that problem at the same time?

I'm going to solve them one-at-a-time, but it won't be too difficult.

First, we need two pieces of data. We need the table name and property name, assuming that they're not defined by the property name itself. (As it appears.)

If you have access to the System.ComponentModel.DataAnnotations library, and the System.ComponentModel.DataAnnotations and System.ComponentModel.DataAnnotations.Schema namespaces, then this is significantly easer, otherwise you need to define the three attributes we're going to use. I'm going to use built-in's.

First, we're going to define our keys on the object with the System.ComponentModel.DataAnnotations.Key attribute, then we'll define the System.ComponentModel.DataAnnotations.Schema.Table attribute, and the System.ComponentModel.DataAnnotations.Schema.Column attribute.

[Table("TestTable")]
public class Foo
{
    [Key]
    [Column("fooId")]
    public int Id { get; set; }

    public string Name { get; set; }
}

[Table("TableBar")]
public class Bar
{
    [Key]
    [Column("barId")]
    public int BarId { get; set; }

    [Column("barName")]
    public string Name { get; set; }
}

public class Baz
{
    [Key]
    public int Id { get; set; }

    public int Order { get; set; }
}

The result is going to be something like:

SELECT * FROM TestTable WHERE fooId = @fooId
SELECT * FROM TableBar WHERE barId = @barId AND Name = @Name
SELECT * FROM Baz WHERE Id = @Id

So, how do we get there? A lot of reflection.

public static List<string> GetParameters<T>(IEnumerable<string> additionalConstraints = null)
{
    var type = typeof(T);

    var result = new List<string>();
    var keys = type.GetProperties().Where(x => x.IsDefined(typeof(KeyAttribute), true));
    if (keys.Any())
    {
        foreach (var key in keys)
        {
            if (key.IsDefined(typeof(ColumnAttribute), true))
            {
                var nameAttr = (ColumnAttribute)key.GetCustomAttributes(typeof(ColumnAttribute), true).FirstOrDefault();
                result.Add($"@{nameAttr.Name}");
            }
            else
            {
                result.Add($"@{key.Name}");
            }
        }
    }

    if (additionalConstraints?.Any() == true)
    {
        foreach (var constraint in additionalConstraints)
        {
            var prop = typeof(TableAttribute).GetProperties().Where(x => x.Name == constraint).First();
            if (prop.IsDefined(typeof(ColumnAttribute), true))
            {
                var nameAttr = (ColumnAttribute)prop.GetCustomAttributes(typeof(ColumnAttribute), true).FirstOrDefault();
                result.Add($"@{nameAttr.Name}");
            }
            else
            {
                result.Add($"@{prop.Name}");
            }
        }
    }

    return result;
}

public static string BuildSelect<T>(IEnumerable<string> additionalConstraints = null)
{
    var type = typeof(T);

    var sb = new StringBuilder();
    sb.Append("SELECT * FROM ");

    var tableAttribute = type.GetCustomAttributes(typeof(TableAttribute), true).FirstOrDefault();
    if (tableAttribute != null)
    {
        sb.Append(((TableAttribute)tableAttribute).Name);
    }
    else
    {
        sb.Append(type.Name);
    }

    var appendedClause = false;
    var parameters = GetParameters<T>(additionalConstraints);
    if (parameters.Count > 0)
    {
        sb.Append(" WHERE ");
        foreach (var p in parameters)
        {
            if (appendedClause)
            {
                sb.Append(" AND ");
            }

            sb.Append($"{p.Substring(1)} = {p}");
            appendedClause = true;
        }
    }

    return sb.ToString();
}

Advantages? Your code doesn't need modified, at all, to add support for a new type. Simply add the type, define [Table(name)], [Key] and [Column(name)] if necessary.

Downsides? Calling it frequently can become expensive, it would not be difficult to build a cache to hold all the queries and select the relevant one from, so that if you call BuildSelect<Foo>() it would return the result from the cache.

Now the point we're at is great if you only need select queries, if you need more than that we hit a slight issue in that we have a lot of code to reproduce to build a new query. Now there is a fix for that.

public class QueryProperty
{
    public PropertyInfo Property { get; set; }
    public string Name { get; set; }
}

public class QueryData
{
    public string TableName { get; set; }
    public List<QueryProperty> KeyProperties { get; set; } = new List<QueryProperty>();
    public List<QueryProperty> NonKeyProperties { get; set; } = new List<QueryProperty>();
    public List<QueryProperty> AllProperties { get; set; } = new List<QueryProperty>();

    public string AsSelect(bool explicitColumns = true, bool includeKey = true)
    {
        var sb = new StringBuilder();
        sb.Append("SELECT ");

        if (explicitColumns)
        {
            sb.Append(string.Join(", ", AllProperties.Select(x => x.Name)));
        }
        else
        {
            sb.Append('*');
        }

        sb.Append(" FROM ").Append(TableName);

        if (includeKey)
        {
            sb.Append(" WHERE ");
            sb.Append(string.Join(" AND ", KeyProperties.Select(x => $"{x.Name} = @{x.Name}")));
        }

        return sb.ToString();
    }

    public string AsInsert(bool explicitColumns = true)
    {
        var sb = new StringBuilder();
        sb.Append("INSERT INTO ").Append(TableName).Append(' ');

        if (explicitColumns)
        {
            sb.Append('(');
            sb.Append(string.Join(", ", AllProperties.Select(x => x.Name)));
            sb.Append(')');
        }

        sb.Append(" VALUES (");
        sb.Append(string.Join(", ", AllProperties.Select(x => $"@{x.Name}")));
        sb.Append(')');

        return sb.ToString();
    }

    public string AsUpdate(bool explicitColumns = true, bool includeKey = true)
    {
        var sb = new StringBuilder();
        sb.Append("UPDATE ").Append(TableName).Append(' ');

        sb.Append("SET ");
        sb.Append(string.Join(", ", NonKeyProperties.Select(x => $"{x.Name} = @{x.Name}")));

        if (includeKey)
        {
            sb.Append(" WHERE ");
            sb.Append(string.Join(" AND ", KeyProperties.Select(x => $"{x.Name} = @{x.Name}")));
        }

        return sb.ToString();
    }

    public static QueryData Build<T>(IEnumerable<string> additionalConstraints = null)
    {
        var result = new QueryData();

        var type = typeof(T);
        if (type.IsDefined(typeof(TableAttribute)))
        {
            var attr = (TableAttribute)type.GetCustomAttribute(typeof(TableAttribute), true);
            result.TableName = attr.Name;
        }
        else
        {
            result.TableName = type.Name;
        }

        foreach (var prop in type.GetProperties())
        {
            if (prop.IsDefined(typeof(ColumnAttribute), true))
            {
                var nameAttr = (ColumnAttribute)prop.GetCustomAttributes(typeof(ColumnAttribute), true).FirstOrDefault();
                result.AllProperties.Add(new QueryProperty { Property = prop, Name = nameAttr.Name });
            }
            else
            {
                result.AllProperties.Add(new QueryProperty { Property = prop, Name = prop.Name });
            }
        }

        var keys = result.AllProperties.Where(x => x.Property.IsDefined(typeof(KeyAttribute), true));
        if (keys.Any())
        {
            foreach (var key in keys)
            {
                result.KeyProperties.Add(key);
            }
        }

        var nonKeys = result.AllProperties.Where(x => !x.Property.IsDefined(typeof(KeyAttribute), true));
        if (nonKeys.Any())
        {
            foreach (var nonKey in nonKeys)
            {
                result.NonKeyProperties.Add(nonKey);
            }
        }

        if (additionalConstraints?.Any() == true)
        {
            foreach (var keyName in additionalConstraints)
            {
                var key = result.AllProperties.Where(x => x.Property.Name == keyName).FirstOrDefault();
                if (key == null)
                {
                    throw new ArgumentException($"The value '{key}' provided for '{nameof(key)}' was not a property of the type '{type.FullName}'.");
                }

                result.KeyProperties.Add(key);
            }
        }

        return result;
    }
}

Now the call becomes:

var queryData = QueryData.Build<Foo>(new string[] { nameof(Foo.Name) });
var selectQuery = queryData.AsSelect();

I leave building a DELETE query up to you, once you have it I recommend posting a new Code Review question. :)


Activity Selection to maximize the number of activity executions I solved this problem from a challenge.The problem is as below:-

Problem Statement:

Given N activities with their start and finish times. Select the maximum number of activities that can be performed by a single person, assuming that a person can only work on a single activity at a time.

Note : The start time and end time of two activities may coincide.

Input:
The first line contains T denoting the number of testcases. Then follows description of testcases. First line is N number of activities then second line contains N numbers which are starting time of activies.Third line contains N finishing time of activities.

Output:
For each test case, output a single number denoting maximum activities which can be performed in new line.

Constraints:

1<=T<=50
1<=N<=1000
1<=A[i]<=100

Example:

Input:

2
6
1 3 2 5 8 5
2 4 6 7 9 9
4
1 3 2 5
2 4 3 6

Output:

4
4

C++ code for the problem:-

#include<iostream>
void swap(int a[],int i,int j)
{
    int temp=a[i];
    a[i]=a[j];
    a[j]=temp;
}
int activitySelection(int start[], int end[], int n){

    long i,j,count=1,pointer;
    for(i=0;i<n;i++)         //sorting both arrays in the order of the end array
    {
        for(j=i+1;j<n;j++)
        if(end[j]<end[i])
        {
            swap(end,i,j);
            swap(start,i,j);
        }

    }
    pointer=end[0];  //pointer set to end time of the first activity
    for(i=1;i<n;i++)
    {
        if(start[i]>=pointer) //if
        {
            count++;

            pointer=end[i];
        }

    }
    return count;
}
int main()
{
  int t;
  std::cin>>t; /*number of testcases*/
  while(t--)
  {
    int n;
    std::cin >> n;
    int start[n], end[n];
    for(int i=0;i<n;i++)
        std::cin>>start[i];
    for(int i=0;i<n;i++)
        std::cin>>end[i];

   std:: cout << activitySelection(start, end, n) << std::endl;
  }
}

The code works as I intended to.But there are few openings that i feel could be improved in the code.The above code basically sorts the both arrays according to the end[] array.Then it checks if the starting time of the current activity is greater or equal to the end time of the previous activity,if yes then count increments and the pointer is set to the end time of the current activity for comparing the next activity.Two things i want to know:

1.Is there a better approach than sorting arrays and comparing?

2.What is a better way to sort the two arrays on the basis of the second array?

Please suggest changes to improve.Thanks for reading ;-)

Note: your code doesn't currently compile for at least two reasons: you are not telling the compiler cout and endl live in the std namespace, and you can't declare your arrays start and end because n is not known at compile-time (instead, you'd have to use dynamic memory allocations).

In any case, your code smells like your background might be in C (but then you'd probably also know about dynamic memory... well, anyway). Indeed, with C++, you typically define variables as close to their site of usage. Moreover, you could take advantage of the algorithms and data structures provided by the language.

  Is there a better approach than sorting arrays and comparing?

Essentially no: you must sort the arrays (with no additional knowledge, you can't beat the log-linear bound). This dominates your runtime, but it is scalable and practical enough that you don't have to worry about it.

  What is a better way to sort the two arrays on the basis of the second array?

Because start and end times are logically and tightly coupled (i.e., something is wrong if we suddenly lose the correspondence), it's a good idea to tie them together. For that reason, we can use std::pair. To store such objects, we can use a dynamic array, i.e., std::vector. For sorting that according to the second element, we can use std::sort.

To summarize, we could proceed along the following lines (I'm taking the liberty to omit reading input to avoid clutter):

#include <iostream>
#include <algorithm>
#include <vector>

typedef std::vector<std::pair<int, int> > PairList;

int activitySelection(const PairList& p)
{
    PairList s(p);
    std::sort(s.begin(), s.end(), [](const auto& x, const auto& y) {
        return x.second < y.second;
    });

    int end = s[0].second;
    int count = 1;

    for (int i = 1; i < s.size(); ++i)
    {
        if (s[i].first >= end)
        {
            ++count;
            end = s[i].second;
        }
    }

    return count;
}

int main()
{
    PairList v{ {1,2},{3,4},{2,6},{5,7},{8,9},{5,9} };
    std::cout << activitySelection(v) << "\n";
}


Country, city, and address tables for a customer I need to create a customer and I have foreign keys for each table. Below is an image displaying the relationships of the tables when I want to insert a new customer. Then below the diagram I list out 3 methods that I would like to know any suggestions on how to best remove all the duplication. The main obstacle I encounter is that each method has a different amount of PrepareStatement and params to set (i.e. setString, setLong, etc). I want to use this example to learn how to do it to clean up the rest of my code. Please note that I am not allowed to use any external libraries or frameworks for this project outside of the standard java libraries.

private void handleSaveButton() throws SQLException {
    countryId = getCountryId();
    cityId = getCityId();
    addressId = getAddressId();
}

/**
 *
 * @return @throws SQLException
 */
private long getCountryId() throws SQLException {
    getConnection();
    param = toTitleCase(countryTextField.getText());
    System.out.println("country param: " + param);
    String query = "insert into country (country) values (?)";
    ps = con.prepareStatement(query, Statement.RETURN_GENERATED_KEYS);
    ps.setString(1, param);
    try {
        ps.executeUpdate();
        rs = ps.getGeneratedKeys();
        rs.next();
        primaryKey = rs.getLong(1);
        System.out.println("inserted country id: " + primaryKey);
    } catch (SQLIntegrityConstraintViolationException e) {
        ps = con.prepareStatement("select countryId from country where country = ?");
        ps.setString(1, param);
        rs = ps.executeQuery();
        rs.next();
        primaryKey = (long) rs.getInt(1);
        System.out.println("existing country id: " + primaryKey);
    } finally {
        closeConnection(ps);
    }
    return primaryKey;
}

/**
 *
 * @return @throws SQLException
 */
private long getCityId() throws SQLException {
    getConnection();
    param = toTitleCase(cityTextField.getText());
    System.out.println("city param: " + param);
    String query = "insert into city (city, countryId) values (?, ?)";
    ps = con.prepareStatement(query, Statement.RETURN_GENERATED_KEYS);
    ps.setString(1, param);
    ps.setLong(2, countryId);
    try {
        ps.executeUpdate();
        rs = ps.getGeneratedKeys();
        rs.next();
        primaryKey = rs.getLong(1);
        System.out.println("inserted city id: " + primaryKey);
    } catch (SQLIntegrityConstraintViolationException e) {
        ps = con.prepareStatement("select cityId from city where city = ?");
        ps.setString(1, param);
        rs = ps.executeQuery();
        rs.next();
        primaryKey = (long) rs.getInt(1);
        System.out.println("existing city id: " + primaryKey);
    } finally {
        closeConnection(ps);
    }
    return primaryKey;
}

/**
 *
 * @return @throws SQLException
 */
private long getAddressId() throws SQLException {
    getConnection();
    param = toTitleCase(addressTextField.getText());
    System.out.println("address param: " + param);
    String query = "insert into address (address, cityId, postalCode, phone) values (?,?,?,?)";
    ps = con.prepareStatement(query, Statement.RETURN_GENERATED_KEYS);
    ps.setString(1, param);
    ps.setLong(2, cityId);
    ps.setString(3, postalTextField.getText());
    ps.setString(4, phoneTextField.getText());
    try {
        ps.executeUpdate();
        rs = ps.getGeneratedKeys();
        rs.next();
        primaryKey = rs.getLong(1);
        System.out.println("inserted address id: " + primaryKey);
    } catch (SQLIntegrityConstraintViolationException e) {
        ps = con.prepareStatement("select addressId from address where address = ? and cityId = ?");
        ps.setString(1, param);
        ps.setLong(2, cityId);
        rs = ps.executeQuery();
        rs.next();
        primaryKey = (long) rs.getInt(1);
        System.out.println("existing address id: " + primaryKey);
    } finally {
        closeConnection(ps);
    }
    return primaryKey;
}

besides removal of duplication, there are other issues, more important IMO, that need addressing:

Using Exceptions as control flow is considered an anti pattern. There are several reasons for this, they are outlined here. in your case, an added reason is that the insert is only needed for the first time for every key. the proper way to do it is to try to select and when you get no data, do an insert. this way, the decision to insert is done in an if statement, and after the 2nd time, there will be no redundant access to the DB.
naming convention: in Java (as well as several other languages) there is a naming convention for method that dictates that prefix getXxx() means the method retrieves information and does not modify it. your methods break the principle of least surprise. if the caller of these methods is not aware of the implementation, it will not expect changes to DB after calling them.