Merge Sort Implementation in Java (that seems slow...) I decided to write my own implementation of merge sort. Here is the code:

public static int[] mergeSort(int[] array) {
    if (array.length <= 1) {
        return array;
    }
    int half = array.length / 2;
    return merge(mergeSort(Arrays.copyOfRange(array, 0, half)),
            mergeSort(Arrays.copyOfRange(array, half, array.length)));
}

private static int[] merge(int[] array1, int[] array2) {
    int len1 = array1.length, len2 = array2.length;
    int totalLength = len1 + len2;
    int[] result = new int[totalLength];
    for(int i = 0, i1 = 0, i2 = 0; i < totalLength; ) {
        if(i1 >= len1) {
            while(i2 < len2) {
                result[i++] = array2[i2++];
            }
            break;
        } else if(i2 >= len2) {
            while(i1 < len1) {
                result[i++] = array1[i1++];
            }
            break;
        }
        if(array1[i1] > array2[i2]) {
            result[i++] = array2[i2++];
        } else {
            result[i++] = array1[i1++];
        }
    }
    return result;
}

When used like:

public static void main(String[] args) {
    Random random = new Random();
    int[] array = new int[10000];
    for(int i = 0, len = array.length; i < len; i++) {
        array[i] = random.nextInt(25000);
    }
    long before = System.nanoTime();
    System.out.println(toString(array) + "\n\n" + toString(mergeSort(array)) + "\nSorted in: " + (System.nanoTime() - before) + " nanoseconds.");
}

private static String toString(int[] array) {
    StringBuffer sb = new StringBuffer("[ ");
    int len = array.length;
    for(int i = 0; i < len - 1; i++) {
        sb.append(array[i] + ", ");
    }
    sb.append(array[len - 1] + " ]");
    return sb.toString();
}

A sample output would be:

[ 8828, 11730, 24944, 18351, 24154, 12634, ... 18114, 3517, 221, 1808, 10474, 18710, 10050, 505, 11304, 5945, 19927, 652 ]

[ 0, 14, 21, 23, 24, 25, 27, 35, 35, ... 24926, 24928, 24929, 24931, 24932, 24935, 24936, 24937, 24937, 24940, 24940, 24992 ]

Sorted in 79326188 nanoseconds.

Another sample output:

[ 11905, 14630, 17973, 9145, 10181, 20421, 24063, 13459, 5806, 22352, 1880, 24100, ... 14299, 15733, 2189, 16743, 6046, 331, 11915, 4008, 14482, 3785, 921, 13954 ]

[ 3, 5, 8, 9, 9, 14, 17, 17, 18, 19, 21, 23, 26, 27, 28, 28, 30, 35, 36, 39, 44, 46, 47, 48, 50, 54, ... 24982, 24983, 24983, 24987, 24989, 24991, 24993, 24995, 24997 ]

Sorted in 23808700 nanoseconds.

The average time needed to sort each number is ((79326188 / 10000) + (23808700 / 10000)) / 2 = 515.67444 nanoseconds, which to me seems a bit slow. Is this implementation as optimized as it can get, or is there a faster way?

General suggestion not related to the sorting implementation: You should be timing the time it takes to call and return from your mergeSort() method, not the time it takes to call System.out.println() and your toString() methods as well. Furthermore, a better microbenchmark calls for running a few iterations and then taking averages to smooth out outlying cases due to "warm-up".

Also, I will suggest using a StringBuilder instead of StringBuffer since your presumably do not require synchronization here, and append values one by one instead of concatenating them together:

stringBuilder.append(array[i]).append(", ");

Even if you feel like timing how long it takes for your toString() method, doing both of them is likely to shave a bit of time off too if used with enough repetition.


Java Cross with characters I managed to make a perfectly even cross with for loops.

I'm trying to make this cross easier to code.  I have to keep it with for loops, but it's too long to implement.

public class CroixDemoAmmeliore {
    public static void main(String args[]) {

        int e = 5;
        int i;
        int j;
        int k;
        int l;

        for (k = 1; k <= e; k++) {
            for (i = 1; i <= e; i++) {
                System.out.print(" ");
            }
            for (j = 1; j <= e; j++) {
                System.out.print("*");
            }
            System.out.println();
        }
        for (k = 1; k <= e; k++) {
            for (i = 1; i <= e * 3; i++) {
                System.out.print("*");
            }
            System.out.println();
        }
        for (k = 1; k <= e; k++) {
            for (i = 1; i <= e; i++) {
                System.out.print(" ");
            }
            for (j = 1; j <= e; j++) {
                System.out.print("*");
            }
            System.out.println();
        }
    }
}

First thing I would say is that you could reduce the number of variables you use. The iterator of a For loop, if instanced from outside the loop's context (as in your case), can be reused. Doing so you could already reduce the numbers of variables you use.

I do not understand what you mean by "easier to code", but if you mean that you seek a way to more easily re-use this code, you could either make a static method that would display a cross with specific parameters or an object with a display function.

Here is an exemple of a Cross object:

public class Cross {

    private int sectionSize;

    public Cross(int sectionSize) {
        this.sectionSize = sectionSize;
    }

    public void display() {
        final int height = sectionSize * 3;
        final char character = '*';
        //The iterators
        int i, j;

        //This loop controls the line
        for (i = 0; i < height; i++) {
            //Check wether or not to add spaces
            if (i < sectionSize || i > sectionSize * 2) {
                //Add spaces
                for (j = 0; j < sectionSize; j++)
                    System.out.print(" ");
                //Add the characters
                for (j = 0; j < sectionSize; j++)
                    System.out.print(character);
            }
            else
                for (j = 0; j < height; j++)
                    System.out.print(character);
            System.out.println();
        }
    }
}

Or in the format of a static method (which is actually the display method of the last object without the comments):

public static void drawCross(int sectionSize, char character) {
    final int height = sectionSize * 3;
    int i, j;

    for (i = 0; i < height; i++) {
        if (i < sectionSize || i > sectionSize * 2) {
            for (j = 0; j < sectionSize; j++)
                System.out.print(" ");
            for (j = 0; j < sectionSize; j++)
                System.out.print(character);
        }
        else
            for (j = 0; j < height; j++)
                System.out.print(character);
        System.out.println();
    }
}

Here is how you would use either of these solutions:

public static void main(String [] args) {
    final int sectionSize = 5;

    //Use of the Object
    Cross cross = new Cross(sectionSize);
    cross.display();

    //Use of the static method
    Cross.drawCross(sectionSize, '*');
}

Now, if it were not for the necessity of using For loops, I would have replaced the loop that adds the space with a simpler:

System.out.printf(("%," + sectionSize + "s"), "");

and then I would have added the characters of the cross in a loop.
I hope this helps you.


Function to delete oldest items out of HTML5 localStorage. Can I make it more efficient? In a current javascript project, I'm working with the browsers localStorage, and it will pretty consistently be full. To overcome this, I wrote a wrapper to surround the localStorage object that would keep track of when an object was added.

The hard(er) part, was coming up with the algorithm to make room for any new data. I was wondering if anyone could critique my method, and let me know if a better method exists.

DataStore.prototype.makeRoom = function() {
    var tmpData = {};
    for (var key in localStorage) {
        var expTime = JSON.parse(localStorage.getItem(key)).expirationTime;
        if (Object.size(tmpData) < 3) {
            tmpData[key] = expTime;
            continue;
        }
        for (var tmpKey in tmpData) {
            var tmpExp = JSON.parse(localStorage.getItem(tmpKey)).expirationTime;
            if (tmpExp > expTime) {
                delete tmpData[tmpKey];
                tmpData[key] = expTime;
                break;
            }
        }
    }
    for (var deleteKey in tmpData) {
        this.destroyItem(deleteKey);
    }
    return true;
};

And for reference, DataStore.destroyItem(), and DataStore.store() where the data is written:

DataStore.prototype.store = function(dataKey, data, minutesUntilExpiration, overWrite) {
    if (!overWrite && this.hasItem(dataKey))
        return false;
    var dataToSave = {
        data: data,
        expirationTime: (new Date().getTime() + (minutesUntilExpiration * 60 * 1000))
    };
    try {
        localStorage.setItem(dataKey, JSON.stringify(dataToSave));
    } catch (e) {
        this.makeRoom();
        this.store(dataKey,data,minutesUntilExpiration,overWrite);
    }
    return true;
};

DataStore.prototype.destroyItem = function(dataKey) {
    if (this.hasItem(dataKey)) {
        localStorage.removeItem(dataKey);
        return true;
    }
    return false;
};

Object.size = function(obj) {
    var size = 0, key;
    for (key in obj) {
        if (obj.hasOwnProperty(key)) size++;
    }
    return size;
};

Object.size

First off, if it's ok with you to use ES5 APIs (you use localStorage, it should be okay), then here's a quicker way to get the number of keys in an object using Object.keys.

return Object.keys(obj).length

Delete by N

Also, from what I understand in your code, you're deleting keys by 3's. If so, then you should not hardcode the 3. Instead, have it configurable in your object.

Verbose Variable Naming

Here's one I get often. Name variables according to use. tmpExp will not help anyone understand its purpose. Name them verbosely. I should understand code like I read the back of a milk box or something.

Compress the algorithm

I suggest you compress the algorithm, make it simple.

Nested loops are just bad for performance. Also, getItem is a synchronous operation, which means that if it freezes, it freezes the UI as well. Nesting that along with a JSON.parse makes it even more slow.

If you can, do operations with the least loop runs and function calls as much as possible. If you can cram several loops into one big loop (like batched operations), then do so.

Here's a proposed fix for makeRoom. Keeping it simple, it just removes the oldest among the stored data.

// Just grab the expiry and store the expiry-key into an object
var expiries = Object.keys(localStorage).reduce(function(collection,key){
  var currentExpirationTime = JSON.parse(localStorage.getItem(key)).expirationTime;
  collection[currentExpirationTime] = key;
  return collection;
},{});

// Get the expiry dates into an array
var expiryDates = Object.keys(expiries);

// For N times, find the oldest (smallest timestamp) and destroy
for(var i = 0; i < N; i++){
  var oldestDate = Math.min.apply(null,expiryDates);
  this.destroyItem(expiries[oldestDate]);
}

getTime now()!

Instead of new Date().getTime(), there's the new Date.now(). Same effect, but saves you memory from creating a new Date instance just to get the current timestamp.


What improvements are needed in this class to fill DropDown using PetaPoco? Below is the class which I am using to fill multiple DropDowns on ASP.NET Page Load event:

public sealed class getBlocks
{
    public getBlocks(DropDownList dropDownName, string districtId)
    {
        returnBlocks(dropDownName, districtId);
    }

    public void returnBlocks(DropDownList DropDownName, string DistrictId)
    {
        var DB = new PetaPoco.Database("cnWebDems");
        string Query = "SELECT distinct blockname, blockid FROM hab_master WHERE distid = '" + DistrictId + "' ORDER BY blockname";
        var result = DB.Fetch<hab_master>(Query);
        DropDownName.DataSource = result;
        DropDownName.DataTextField = "blockname";
        DropDownName.DataValueField = "blockid";
        DropDownName.DataBind();
        DropDownName.Items.Insert(0, "-- Select --");
        DB.Dispose();
    }

Suggest further improvements.

Firstly, data access should be separate from your UI logic. Certainly create a new layer, where you'll manage CRUD operations.

Secondly, I'm not really fond of your naming convention of classes. Even though it may be a subjective matter, many people tend to use capitalized names for classes and certainly not using words like "getBlocks". This isn't a name for a class, that's a name for a getter method.

Thirdly, you're dealing with your parameters the wrong way, at least in my opinion. I believe it would be more appropriate to have private fields of type DropDownList and String, populate them in the constructor and then just use these private variables (and/or properties, depends on your needs) instead of specifying parameters for the method itself. Seems more OOP to me that way. Otherwise I don't really see a reason why not just create a helper class with a static method you'll call whenever needed, without the need to instantiate the class itself.

Another thing to consider - usually it's better to use an using statement instead of manually calling Dispose method.

Edit based on the comment:

1.) example using private fields

public sealed class Blocks
{
    private DropDownList _ddList;
    private int _districtId;

    public Blocks(DropDownList dropDownList, int districtId)
    {
        _ddList = dropDownList;
        _districtId = districtId;
    }

    public void PopulateDropDownList()
    {
        var results = MyDbAccessClass.GetBlocks(_districtId);

        _ddList.DataSource = results;
        _ddList.DataTextField = "blockname";
        _ddList.DataValueField = "blockid";
        _ddList.DataBind();
        _ddList.Items.Insert(0, "-- Select --");
    }
}

2.) example using static method

public sealed class MyHelperMethods
{
    public static void PopulateWithBlocks(DropDownList ddList, int districtId)
    {
         var results = MyDbAccessClass.GetBlocks(districtId);

        ddList.DataSource = results;
        ddList.DataTextField = "blockname";
        ddList.DataValueField = "blockid";
        ddList.DataBind();
        ddList.Items.Insert(0, "-- Select --");
    }
}

Hope you get the idea...


Binary search tree in Java with only one class I have written a BST using one class. Is my code okey or something could be better?

/**
* BST using one class
* @author Mateusz Stankiewicz
*/
public class Node {
private Comparable data;
private Node left;
private Node right;

private static Node root;

/**
 * Tree constructor
 */
public Node() {
    root = null;
}

/**
 * Node constructor
 */
private Node(Comparable data) {
    this.data = data;
    left = null;
    right = null;
}

/**
 * Tree method to add new element
 *
 * @param data is a new data we want to add to a tree
 */
public void addToTree(int data) {
    if (root == null)
        root = new Node(data);
    else
        add(root, data);
}

// Private method that add new element
private void add(Node target, Comparable data) {
    if (data.compareTo(target.data)<0) {
        if (target.left == null)
            target.left = new Node(data);
        else
            add(target.left, data);
    } else {
        if (target.right == null)
            target.right = new Node(data);
        else
            add(target.right, data);
    }

}

/**
 * Prints out tree nodes
 */
public void printTree() {
    print(root);
}

// Private recursive method to print a tree
private void print(Node n) {
    if (n.left != null)
        print(n.left);
    System.out.println(n.data);
    if (n.right != null)
        print(n.right);
}

public static void main(String[] args) {
    Node tree = new Node();
    tree.addToTree(1);
    tree.addToTree(7);
    tree.addToTree(3);
    tree.addToTree(4);
    tree.addToTree(2);
    tree.printTree();
}
}

There are however a couple of improvements and refactoring that you could definitely make. The problem in your logic is that a node is holding a reference to the root node, when it shouldn't. A node should only know what data it contains and what are its left and right nodes, nothing more (you could perhaps argue that, in some cases, it could also contain a reference to its parent).

Never use raw types. In this case, you are using Comparable as a raw type. Since your Node class can hold data for every type, what you want is to make that class a typed class.

public class Node<T extends Comparable<T>> {
    private T data;
    private Node<T> left;
    private Node<T> right;
    // ...
}

This makes the rest of the code type-safe. In this case, you are saying that your node can contain data of a type T that is comparable with itself, for any type matching this.

Making this change will show the problem with your design:

private static Node<T> root;

This won't compile anymore and it echoes to what I said in the beginning of this answer: this shouldn't exist in the first place. So you should delete it.
Then, the default constructor, which is

public Node() {
    root = null;
}

can also be removed: when constructing a node, you should just need to specify the value its going to hold. As such, your next private constructor should be made public. Furthermore, you don't need to explicitely set the left and right node to null since it will be their default value.

public Node(T data) {
    this.data = data;
}

The method addToTree should be renamed add. Furthermore, it takes an int as argument when it should be made more generic: you can accept in your node any object of type T. You could then refactor it to the following. Note that I also added explicits curly braces: it isn't a good practice to omit them.

public void add(T data) {
    if (data.compareTo(this.data) < 0) {
        if (left == null) {
            left = new Node<>(data);
        } else {
            left.add(data);
        }
    } else {
        if (right == null) {
            right = new Node<>(data);
        } else {
            right.add(data);
        }
    }
}

Your current code doesn't handle the case where you're adding an element that is already present in the tree. If this is intended, it should be documented because BST are allowed not to contain duplicated elements (and most do not).
In the same way, printTree should be renamed print. Consider adding curly braces here also.

public void print() {
    if (left != null) {
        left.print();
    }
    System.out.println(data);
    if (right != null) {
        right.print();
    }
}

For now, you are accepting in your node any Comparable objects. It would be better to accept any type T but also accept a Comparator<T>. The reason is that you may want to have in your tree:

data for which you have no control over so you can't make it Comparable (note: you could still wrap it in a Comparable delegate).
the same type of data but ordered differently (e.g. String ordered by their length, or their lexicographic order...).

This would make your class even more generic since it wouldn't be restricted to Comparable objects.


Manipulate JSON data from the client with JavaScript I am making an application in which the user has the ability to add, remove, and edit JSON data through DOM interaction. I have created a working JavaScript-only prototype that accepts certain variables to manipulate certain elements of the JSON data, although it is rather messy and seems rather impossible for me to implement with the DOM.

Right now, my current structure is:

  Example Modding Session

  Modding Section

  A mod
  A mod

  Another Modding Section

  A mod
  A mod
  A mod

  Another Modding Session

  Modding Section

  Another mod
  Another mod
  Another mod

  Modding Section

  Another mod

Which has worked fine for me, however, my functions are very repetitive, which I would like to fix.

var addSession = function(sessionName) {

    var selfSessionName = sessionName || undefined;
    if(!selfSessionName) return;

    moddingSessions.push({
        "name": selfSessionName,
        "sections": []
    });

}

Here you may start to see a pattern:

var addSection = function(sectionName, sessionName) {

    var selfSessionName = sessionName || undefined,
        selfSectionName = sectionName || undefined;
    if(!selfSectionName || !selfSectionName) return;

    for(var i = 0; i < moddingSessions.length; i++) {
        if(moddingSessions[i].name === selfSessionName) {
            moddingSessions[i].sections.push({
                "name": selfSectionName,
                "mods": []
            });
        }
    }

}

And here:

var addMod = function(modContent, sectionName, sessionName) {

    var selfSessionName = sessionName || undefined,
        selfSectionName = sectionName || undefined,
        selfModContent  = modContent  || undefined;
    if(!selfModContent || !selfSectionName || !selfSectionName) return;

    for(var i = 0; i < moddingSessions.length; i++) {
        if(moddingSessions[i].name === selfSessionName) {
            for(var j = 0; j < moddingSessions[i].sections.length; j++) {
                if(moddingSessions[i].sections[j].name === selfSectionName) {
                    moddingSessions[i].sections[j].mods.push(modContent);
                }
            }
        }
    }

}

As you can see, it works, but isn't very effective. I also have no idea how to handle each and every parameter when using this with DOM interaction. I hope there is a better way to achieve the same functionality.

I see several problems.

Redeclaring function arguments as local variables
Assigning variables undefined by default
Failing silently when "falsey" arguments are passed

Redeclaring Function Arguments

This is absolutely unnecessary. You're only creating more variables that must be garbage collected by the browser, adding weight to your code and getting no tangible benefit from it. Now if you wanted copies of the arguments because you need to modify the data but still know the original values passed to the function, then you have a case for creating variables separate from the arguments, but this is an exception to the rule.

Assigning Variables undefined By Default

This is already how JavaScript works. This pattern should be used to give meaningful default values where applicable.

Failing Silently When "Falsey" Arguments Are Passed

Fail early, and fail noisily. You don't want to chase production bugs caused because someone forgot to pass an argument and have no indication that the argument was required. If they truly are required, throw errors:

var addMod = function(modContent, sectionName, sessionName) {
    if (!modContent) {
        throw new Error("Missing required argument: modContent");
    }

    if (!sessionName) {
        throw new Error("Missing required argument: sessionName");
    }

    if (!sessionName) {
        throw new Error("Missing required argument: sessionName");
    }

It makes for extra code, but development and maintenance become easier because you get clear errors (and stack traces). This really is a judgement call. There is a fine line between bullet proofing your code and expecting some level of intelligence from the people using it, especially for dynamically typed languages like JavaScript. If you have a public API, document it thoroughly. And "public" could mean "pushed to GitHub" or "used by coworkers at work."


PHP library that can use different authentication mechanisms I am making a library which can use different auth mechanisms. I have abstracted the code within an interface:

interface Authenticable
{
    public function getName(): string;

    /**
     * Based on auth type, return the right format of credentials to be sent to the server
     *
     * @param Authenticable $credentials
     * @return mixed
     */
    public function encodedCredentials(): string;
}

One of the possible methods to use this is Plain.php:

class Plain implements Authenticable
{
    protected $name = 'PLAIN';
    protected $options;

    public function __construct(Options $options)
    {
        $this->options = $options;
    }

    public function encodedCredentials(): string
    {
        $credentials = "\x00{$this->options->getUsername()}\x00{$this->options->getPassword()}";
        return XML::quote(base64_encode($credentials));
    }

    public function getName(): string
    {
        return $this->name;
    }
}

And when initializing auth methods I am using this part of code

public function getAuthType()
{
    if (!$this->authType)
        $this->authType = new Plain($this->options);

    return $this->authType;
}

which is initializing Plain if no other method was previously given.

What I am wondering here is: should I forward $username and $password to Plain so that I could decouple it from Options class (since those are the only two fields needed in the class)?

On the other hand, that is what Options class is all about. Besides username and pass, it also holds host, port, logger and resource setters and getters, and few helper methods

I would lay it out something like this:

interface AuthenticableInterface
{
    public function getName(): string;

    /**
     * Based on auth type, return the right format of credentials to be sent to the server
     *
     * @param Authenticable $credentials
     * @return mixed
     */
    public function encodedCredentials(): string;
}

class AbstractAuthenticable implements AuthenticableInterface
{
    protected $name = 'PLAIN';
    protected $options;

    public function __construct(Options $options)
    {
        $this->options = $options;
    }

    abstract public function encodedCredentials(): string;

    public function getName(): string
    {
        return $this->name;
    }
}

class Plain extends AbstractAuthenticable{

    public function encodedCredentials(): string
    {
        $credentials = "\x00{$this->options->getUsername()}\x00{$this->options->getPassword()}";
        return XML::quote(base64_encode($credentials));
    }

}

I kept the interface, a lot of people get confused between using abstract and interface.  I should add that the interface is inherited through the extending of the abstract class, so there is no need to declare (implement) it again.

Interfaces basically just outline the classes API (application program interface, or I like to say public interface), Abstract classes do the same thing (basically) as an interface but you can have concrete implementation of common functionality within the class.  Interface doesn't allow you to implement any functionality.

By keeping the interface you could in theory create a Authenticable that uses a base other then the Abstract class.  This is useful especially when type casting.

class foo{
     public function __construct(AuthenticableInterface $Authenticable){
          //...
     }
}

So instead of hinting the Abstract class or the Concrete class you can type hint the interface which can be satisfied even when using a completely different base class.  In this case it may not be strictly necessary to carry the interface but it does give you that little bit of extra flexibility for the hierarchy.

A good example of the above would be to make a token based login for something like REST where you don't have a username and password, and so that class could be quite different then the base class that uses a user and a password.

Another example I can think of is if you have a hierarchy in your user system, such as a Parent and Subusers, or Admins and non-admins.  You may want something like a passwordless Login As functionality that relies more on the user's role and relationship to decide if it should authenticate.  Something like a button you just click to login as one of your associated subusers etc.  We use subusers a lot in our user systems.

Rather or not you should decouple the username and password depends really on how you want to pass that data.  Obviously username and password are going to be really important so it may be worth adding them as function arguments.  But as I mentioned above you may create authentication that does not use a username and password.

I would say the biggest argument for it is, then the password and username is not "hidden" or buried in some kind of BLOB or array data. But on the same token (pun intended) you may have some classes that don't need them.  So the biggest disadvantage is then requiring it as part of the class, which may break the single interface design.

For example consider this interface and class:

interface Authenticable
{
    public function setPassword(string $password);
}

class LoginAs
{
     //public function setPassword(string $password);
}

So you can see setting a password for "LoginAs" doesn't really make sense and this would stretch what we can do with our interface.  By having them as options they are not exposed to the interface because they are wrapped in another object.

I'll end by saying if you keep them in the options you will want to do some more robust error checking such as this:

public function encodedCredentials(): string
{
    $credentials = "\x00{$this->options->getUsername()}\x00{$this->options->getPassword()}";
    return XML::quote(base64_encode($credentials));
}

I modified your getUsername function in Options. I like to set them up this way.

class options{

     public function getUsername($default=null){
           if(isset($this->username)) return $this->username;

           if(!is_null($default))  return $default;

           throw new OptionException("Option {$username} is not set");
     }
}

By doing this we can control what we get back when its not set.  By setting a default other than null, when default is not set we throw an error.  Typically I do this using the magic __call method etc..  Basically I showed this to show how you can implement some error checking on those values.  Obviously you could check this in each encodedCredentials() but you can also bake it into the options class so it happens when we try to get an options etc...

Hope that helps!


Convert UTF8 string to UTF32 string in C I'm doing some recreational programming in C (after spending some time in C++, but professionally using only PHP/JavaScript).
I wrote a UTF8 to UTF32 converter and just wanted to know if I made some obvious mistakes. (For example, is it a big no-no to malloc inside a function (because of possible memory leaks) – and if yes – how would you rather do it?)
#include <stdlib.h>
#include <stdint.h>

size_t utf8_strlen(uint8_t* text) {
    size_t i = 0;
    size_t num_chars = 0;

    while (text[i] != 0) {
        num_chars++;

        if ((text[i] & 0b10000000) == 0) {
            // 1 byte code point, ASCII
            i += 1;
        }
        else if ((text[i] & 0b11100000) == 0b11000000) {
            // 2 byte code point
            i += 2;
        }
        else if ((text[i] & 0b11110000) == 0b11100000) {
            // 3 byte code point
            i += 3;
        }
        else {
            // 4 byte code point
            i += 4;
        }
    }

    return num_chars;
}

uint32_t* utf8_to_utf32(uint8_t* text) {
    size_t num_chars = utf8_strlen(text);
    uint32_t* c = malloc(sizeof(uint32_t) * num_chars);
    size_t i = 0;

    for (size_t n = 0; n < num_chars; n++) {
        if ((text[i] & 0b10000000) == 0) {
            // 1 byte code point, ASCII
            c[n] = (text[i] & 0b01111111);
            i += 1;
        }
        else if ((text[i] & 0b11100000) == 0b11000000) {
            // 2 byte code point
            c[n] = (text[i] & 0b00011111) << 6 | (text[i + 1] & 0b00111111);
            i += 2;
        }
        else if ((text[i] & 0b11110000) == 0b11100000) {
            // 3 byte code point
            c[n] = (text[i] & 0b00001111) << 12 | (text[i + 1] & 0b00111111) << 6 | (text[i + 2] & 0b00111111);
            i += 3;
        }
        else {
            // 4 byte code point
            c[n] = (text[i] & 0b00000111) << 18 | (text[i + 1] & 0b00111111) << 12 | (text[i + 2] & 0b00111111) << 6 | (text[i + 3] & 0b00111111);
            i += 4;
        }
    }

    return c;
}

Edit:
I've posted an updated version of the code to GitHub for anyone that is interested: https://github.com/s22h/cutils/blob/master/src/unicode.h

There are two main things to talk about: checking the input, and buffer handling (your malloc question).

It's a very bad idea to do things like array[i + 1], unless you're absolutely, 100% sure that this refers to memory that is both allocated and part of the string. If it's not allocated, your program is begging for a crash; if it's not part of the string, it's a vulnerability issue and every script kiddie will be able to read your process' memory.

Checking the input

Unless you're absolutely, 100% sure that your user will be supplying you with a valid, NULL-terminated UTF-8 string, you definitely need to do some checking. Note that when I say "user", I mean the developer who will be using your functions.

When writing APIs, it really helps to write, or at least think about, some usage code first, in order to get an idea of what developers would need and how your code will be used. This will help again later, when discussing your malloc question.

Now, think about your general use cases. The developer gets some input through a socket, file, hardware device, form field, whatever. Sockets, files and hardware are prone to losing some bytes due to many different reasons. Forms are generally safe, but you will come across a library that will leave off bytes of the last UTF-8 character when you give it a too small buffer, or not null-terminate the string when the last character fits exactly in the buffer (some C standard lib functions are guilty of the latter). Malicious users may also try to exploit the form input field.

Out of all these, only the form field will be naturally NULL-terminated (probably). With all the others, you get a stream of data that simply ends at some point. So you're forcing the user to manually add the NULL byte, which could possibly lead to reallocation... you can see it's not a good idea.

The developer, however, knows exactly how many bytes he got. Why not exploit this? Rewrite your utf8_strlen function like this:

size_t utf8_strlen(uint8_t* text, size_t nb_text, size_t* nb_valid);

Do pretty much the same thing you were doing, however:

Do not blindly increment i without testing whether i + NUM_BYTES_IN_NEXT_CHAR <= nb_text
Stop immediately if that fails
Stop immediately at a null byte, even if you've used less than nb_text bytes
Stop when you've used nb_text bytes
You may check that every single byte conforms to the UTF-8 standard.
Depends on how thorough you want to be.  Best to do it anyways.
In short, only increment i when you're sure that you've got enough bytes left for the next character, and all bytes of the next character are valid UTF-8. Don't increment i at the terminating null byte and immediately stop.
In the end, set *nb_valid = i. This allows the user to check how many bytes were valid UTF-8. Note that text + *nb_valid would point to the byte immediately after the last valid UTF-8 character. If the string ended with a null byte, text + *nb_valid would point to it instead.
Be nice, and allow nb_valid to be a NULL pointer if the developer doesn't care about this information.
Return the number of valid characters parsed.

Now that you're sure you're only going through valid UTF-8, your utf8_to_utf32 can remain the same. Just add the needed parameters:

uint32_t* utf8_to_utf32(uint8_t* text, size_t nb_text, size_t* nb_valid) {
    size_t num_chars = utf8_strlen(text, nb_text, nb_valid);
    /* ... rest is the same ... */
}

... but do consider what the previous answers mentioned, especially the portability issue of left-shifts, if you plan to program arduinos or other microcontrollers.

Buffer handling

is the reason C is hard. But again, let's think about the use cases. At first, allocating the buffer for the user may seem very helpful, but actually it's rarely something that the user wants. Besides, it burdens them with freeing the buffer, which sucks.

Let's say I'm reading a file or some socket line by line. I'd actually want to allocate just one buffer, and re-use it for each line (reallocating or doing partial reads if I get a very long line). Thus, I'd like for utf8_to_utf32 to write to a particular buffer that I give it. Much better than having many, many allocations and deallocations.
Let's say I'm updating some user input. In most cases I'd already have a UTF-32 buffer where the old user input is stored, and I want to update it. Again, I'd like for utf8_to_utf32 to write to a particular buffer.
I may wish to concatenate or append text. Again, writing to a particular buffer.

So it seems you don't want to actually allocate the buffer, which is just dandy, since the best memory management is none at all. (Side note: the more experienced a C developer, the less you'll see them messing around with memory and allocations, for example see Casey Muratori [1] [2]
[3], a very experienced C developer).

The function signature changes to:

void utf8_to_utf32(uint8_t* text, size_t nb_text, uint32_t* out_buf, size_t* nb_valid) {
    /* don't allocate c, replace c with out_buf */
}

Great, but how will you ensure that there's enough space in out_buf? Two options, each with its pros and cons.

A. Let the user worry about it

You're already providing a function - utf8_strlen, that will let the developer know how many characters to allocate for the buffer (plus one more for the NULL-terminator). If they don't, then that's their problem. Many standard lib functions operate like this: for example mbstowcs.

B. You worry about it

Add another parameter to the function that tells you the size of out_buf. You should never write more than this number of characters. This is a bit safer, though the programmer can of course still lie. This will make your code a bit more complicated, and will allow for partial conversions, though this usually falls under the YAGNI principle. Again, many standard lib functions operate like this too: for example strncpy, snprintf

Pick your poison and keep it consistent

People are familiar with both styles, so choose whatever you prefer and keep it consistent if you decide to add more functions later.

Now the only question that remains is what to do with the return value, which is currently void. Looking at the standard lib functions, you'll see that they would usually return the number of characters written to out_buf.

Another reasonable thing to do would be to return 1 or 0, based on whether the input UTF-8 string was valid all the way through, or there was some invalid data. I'm suggesting it because the check would be

is_utf8_valid = (nb_text == *nb_valid) || (text[*nb_valid] == '\0');

which is something you don't want to bother the user with getting right.


Timeouts when calculating stock ratings using LINQ I'm getting a lot of timeouts with the following code and I'm running this method around 10,000 times so I need to figure out how I can optimize this so the timeouts will stop. I am getting timeout errors when I try to call the fill method and I believe it is because the queries aren't done processing yet. It is a local database and it is running SQL Server 2014.

public static StockRating performCalculations(SymbolInfo info)
{
    var calc = new Calculations(info.Symbol, info.Market);
    StockRating rating = new StockRating();

    try
    {
        using (DailyGlobalDataTableAdapter dailyGlobalAdapter = new DailyGlobalDataTableAdapter())
        using (DailyNyseDataTableAdapter dailyNyseAdapter = new DailyNyseDataTableAdapter())
        using (DailyGlobalDataDataTable sandp500Table = new DailyGlobalDataDataTable())
        using (DailyNyseDataDataTable dailyNyseTable = new DailyNyseDataDataTable())
        {
            // fill the s&p500 info first
            dailyGlobalAdapter.ClearBeforeFill = true;
            dailyGlobalAdapter.FillBySymbol(sandp500Table, Calculations.sp500);

            var sp500StockDataQuery = from c in sandp500Table
                                      select new StockData { Close = c.AdjustedClose, High = c.High, Low = c.Low, Volume = c.Volume, Date = c.Date };

            calc.sp500Data = sp500StockDataQuery.AsParallel().ToList();

                    dailyNyseAdapter.FillBySymbol(dailyNyseTable, info.Symbol);

                    var nyseQuery = from c in sandp500Table
                                    where c.Date >= DateTime.Now.Subtract(TimeSpan.FromDays(60))
                                    join d in dailyNyseTable on c.Date equals d.Date
                                    select new StockMarketCompare { stockClose = d.AdjustedClose, marketClose = c.AdjustedClose };

                    var nyseStockDataQuery = from c in dailyNyseTable
                                             select new StockData { Close = c.AdjustedClose, High = c.High, Low = c.Low, Volume = c.Volume, Date = c.Date };

                    calc.stockVsMarketCompareData = nyseQuery.AsParallel().ToList();
                    calc.stockData = nyseStockDataQuery.AsParallel().ToList();

            // get the pct return data that is needed for calculations
            calc.fillPctReturns();

            if (calc.stockPctReturns.Count > 0 && calc.marketPctReturns.Count > 0)
            {
                // perform the calculations here
                rating.estimatedReturn = calc.calculateExpectedReturn();
                rating.longRating = calc.calculateRating(Calculations.ratingType.long_rating);
                rating.market = calc.Market;
                rating.mediumRating = calc.calculateRating(Calculations.ratingType.medium_rating);
                rating.shortRating = calc.calculateRating(Calculations.ratingType.short_rating);
                rating.symbol = calc.Symbol;
                rating.risk = calc.calculateRisk();
                rating.isMarginEnabled = calc.isMarginEnabled();
                Console.WriteLine("Ratings calculated for " + info.Symbol + " in the " + info.Market + ". " + stopwatch.ElapsedMilliseconds / 1000 + " seconds elapsed.");
            }
            else
            {
                rating = null;
            }
        }
    }
    catch (Exception ex)
    {
        rating = performCalculations(info);
    }

    return rating;
}

Parallel.For(0, activeSymbolsCount, j =>
                            {
                                symbol = symbolTable.AsParallel().ElementAtOrDefault(j).Symbol;
                                SymbolInfo symbolClass = new SymbolInfo(symbol, market);
                                StockRating rating = performCalculations(symbolClass);
                                saveToTable(rating);
                            });

Exception handling

You are doing some very creative exception handling here. If an exception occurs, no matter what kind of, you are just calling the method again. Do you think there is some magic going on, cleaning the exceptions by calling the code again ?
If you need to call the method again you shouldn't do this from within the method, but from outside at the calling point.
You should also do this only in cases of exceptions where you can assume that by giving some time they could be resolved. But you shouldn't do this infinite. You should use some sort of counter and stop if it has reached a value/border.

"Fill" exception

The linq seems to be unrelated to the exceptions caused by the "Fill" methods. If the exceptions occur if any of the fill methods is called, then there is no linq involved. You should better check how you have implemented these methods.

EDIT

I guess the problems are inside the .FillBySymbol() method. Without seeing this method it is hard to tell the cause of the problems but I assume that your sql query is just returning to many data.

General

You should extract the evaluating of the date withing 60 days from now to a separate variable.

DateTime searchDate = DateTime.Now.Subtract(TimeSpan.FromDays(60));

var nyseQuery = from c in sandp500Table
                where c.Date >= searchDate
                join d in dailyNyseTable on c.Date equals d.Date
                select new StockMarketCompare { stockClose = d.AdjustedClose, marketClose = c.AdjustedClose };

You have some code duplication which could be removed.

This

var sp500StockDataQuery = from c in sandp500Table
                          select new StockData { Close = c.AdjustedClose, High = c.High, Low = c.Low, Volume = c.Volume, Date = c.Date };

and this

var nyseStockDataQuery = from c in dailyNyseTable
                         select new StockData { Close = c.AdjustedClose, High = c.High, Low = c.Low, Volume = c.Volume, Date = c.Date };

together with the call to .AsParallel().ToList()

If both DailyGlobalDataDataTable and DailyNyseDataDataTable share a base class or an interface whith the properties AdjustedClose, High,Low,VolumeandDate` this can be extracted like

private static List<StockData> GetStockData(commonBaseOrInterface table)
{
    return (from c in table
            select new StockData { Close = c.AdjustedClose, High = c.High, Low = c.Low, Volume = c.Volume, Date = c.Date }
            ).AsParallel().ToList();;
}

this can then be called like

calc.sp500Data = GetStockData(sandp500Table);
calc.stockData = GetStockData(dailyNyseTable);

Naming

Based on the naming guidelines methods should be named using PascalCase casing.

Nit picking

Your indention of the code could be better.


Extraction of data from Flickr I'm crawling Flickr for data for my university research project. However it's very slow and I'm not sure what it is exactly.  It could be the FileWriter slowing it down. Any advice on speeding it up?

try {

    String userID = file.getUserIDFromList(i);

    System.out.println(i + "." + " " + userID);//to screen

    String urlString = "https://api.flickr.com/services/rest/?method=flickr.people.getPublicGroups&api_key=myAPIKey&user_id=" + userID;
    System.out.println(urlString);
    //print userID to file
    file.writeGroupID(userID);
    Collection<Group> groupNames = people.getPublicGroups(userID);

    String groupCount = Integer.toString(groupNames.size());
    //write number of groups to the file
    file.writeFile(groupCount);

    Iterator<Group> iteratorDetails = groupNames.iterator();
    //iterate over list to get each group's details

    while (iteratorDetails.hasNext()) {
        Group groupName = (Group)iteratorDetails.next();

        //get group name
        String name = groupName.getName();

        //get group id
        String id = groupName.getId();

        GroupsInterface groupInter = flickr.getGroupsInterface();

        Group gInfo = groupInter.getInfo(id);

        DocumentBuilderFactory f = DocumentBuilderFactory.newInstance();
        DocumentBuilder b = f.newDocumentBuilder();
        Document doc = b.parse(urlString);

        doc.getDocumentElement().normalize();
        NodeList items = doc.getElementsByTagName("group");
        Node n = items.item(poolCounter);
        Element e = (Element) n;

        //get total photos in group
        String numberOfPhotos = e.getAttribute("pool_count");
        //get members in the group
        int numberOfMembers = gInfo.getMembers();

        //write group details to file
        file.writeFile("\t" + name + " " + numberOfPhotos + " photos" + ", " + numberOfMembers + " members");
        System.out.println("\t" + name + " " + numberOfPhotos + " photos" + ", " + numberOfMembers + " members");
        poolCounter++;
    }
    file.newLine();
}

There are a number of things going on here, but it boils down to Amdahl's Law.

I am going to take a stab at some guessing here, but I expect my numbers to be in the right ballparks. Your code does:

Collection groupNames = people.getPublicGroups(userID);
For each Group:

you connect to flickr
you download the data
you parse it
you write-to-file.

Here are things that you are doing slower than necessary:

you are parsing the same URL many times (as many times as you have groups to process), as the URL does not change inside the loop. If you process it once outside the loop, you will get the same results.
Is you file output buffered?
Do you need to doc.getDocumentElement().normalize(); ?

Now, as for the real application of Amdahl's law, and, assuming the URL is wrong that you are processing (and that there should be 1 URL per group), you should: run things in parallel.

Consider something like:

ExecutorService service = Executors.newFixedThreadPool(....);
....
LinkedList<Future<String>> futureops = new LinkedList<>();

while (...) {
    futureops.add(service.submit(new Callable<Integer>() {
        public String call() throws IOException {
            // ****** Will need to probably do this as a separate class to get  *****
            // ****** it to compile right, or use `final` judiciously.          *****
            DocumentBuilderFactory f = DocumentBuilderFactory.newInstance();
            DocumentBuilder b = f.newDocumentBuilder();
            Document doc = b.parse(urlString);

            doc.getDocumentElement().normalize();
            NodeList items = doc.getElementsByTagName("group");
            Node n = items.item(poolCounter);
            Element e = (Element) n;

            //get total photos in group
            String numberOfPhotos = e.getAttribute("pool_count");
            //get members in the group
            int numberOfMembers = gInfo.getMembers();
            return "\t" + name + " " + numberOfPhotos + " photos" + ", " + numberOfMembers + " members";
        }
    });

    while (!futureops.isEmpty()) {
        String result = futureops.removeFirst();
        file.writeFile(result);
        System.out.println(result);
        poolCounter++;
    }
}


Efficient Traversal and Manipulation of the DOM with Native JavaScript Using For/In Loop With native JavaScript, I intend to traverse a collection of elements in the DOM that contain a link and an image (and possibly other elements). The image may or may not be inside the link—in most cases the image will be in the link, but in some cases it may be directly adjacent to the link, or it may be a child of another div or span that is adjacent to its relevant link. However, both the link and the image will always be contained within a container element, of which there may be several.

For example, envision a stock photo gallery arranged in a grid layout. The HTML for each item in the gallery's collection may look like any one of the snippets below (simplified for cleanliness and brevity).

It might be:

Or it may be:

  Image Title

Or it may be:

    Image Title

Note: I can't control the given HTML. Nevertheless, I would like to traverse and manipulate the DOM flexibly enough to handle any instance of HTML structure given above. To do so efficiently, I'd first need to build the collection of container elements before traversing, yes?

So this:

var items = document.querySelectorAll('.item');
for(i in items) { ... }                           // more efficient

rather than this:

for(i in document.querySelectorAll('.item')) { ... } // less efficient

as the latter would need to re-query the .item selector again inside the loop, which is inefficient. Right?

However, to manipulate the link and image of each item being looped, I still need to access the link and image of said item. To do that, I could use querySelector() on the .item like so:

var items = document.querySelectorAll('.item');
for(i in items) {
  var link = items[i].querySelector('a'), img = items[i].querySelector('img');
  // manipulate link and img elements
}

But is triggering querySelector() twice per loop-instance the most efficient approach?

Is there a more efficient approach to access the link and image of each .item element being looped-through than by calling querySelector() twice on each looped .item element?

Consider that the primary intent of manipulation is to change the link URL to the image source URL and add some attributes to the link and/or image. Seems simple enough. I just want to make sure the loop logic is efficient. Ideally, the logic will ultimately be used in either a bookmarklet or a browser plugin. The collection of .item elements could potentially number in the thousands, so I'm hoping to keep this traversal and manipulation process as efficient as possible.

You should not iterate over an array or array-like object using a foreach loop,
you should use a good old-fashioned for loop instead:

var items = document.querySelectorAll('.item');
for (var i = 0; i < items.length; ++i) { ... }

Yes, this sequential for loop doesn't look as elegant as a foreach loop,
but unfortunately, using a foreach loop for arrays in JavaScript is incorrect.

As for this:

var items = document.querySelectorAll('.item');
for (var i = 0; i < items.length; ++i) {
  var item = items[i];
  var link = item.querySelector('a');
  var img = item.querySelector('img');
}

Having two .querySelector lookups in each iteration is not really a problem,
because the lookups are done on a small object, a tiny subset of the entire document.

I wrote the loop body slightly differently from your original:

I put the value of items[i] in a local item variable.
This is to avoid duplicating item[i].
It's good to eliminate duplications,
because if you ever need to change something, you need to do it in all duplicates,
which is potentially error-prone.
I declared link and img on two lines.
This is slightly more verbose, but a bit clearer.


0/1 knapsack algorithm implementation with possibilities vector The following code uses dynamic approach to solve 0/1 knapsack problem. (I know the maximum profit function I have used is not as good as the one I have defined here and I still am working on that . Are any optimizations possible for the following code?

#include "algorithms.h"

struct Item {
    int index = 1;
    int profit = 1;
    int weight = 1;
    Item() = delete;
    explicit Item(int i, int _profit, int w) {
        index = i;
        profit = _profit;
        weight = w;
    }
    bool operator<(const Item& item) {
        return this->profit < item.profit;
    }
    bool operator<=(const Item& item) {
        return this->profit <= item.profit;
    }
    bool operator>(const Item& item) {
        return this->profit > item.profit;
    }
    bool operator>=(const Item& item) {
        return this->profit >= item.profit;
    }
    friend std::ostream& operator<<(std::ostream& out, const Item item) {
        out << item.index;
        return out;
    }
};

long weight(const std::vector<Item>& item_list, const std::vector<int>& item_switch) {
    long sum = 0;
    for (int i = 0; i < item_switch.size(); i++) {
        sum += item_switch[i] * item_list[i].weight;
    }
    return sum;
}

long profit(const std::vector<Item>& item_list, const std::vector<int>& item_switch) {
    long sum = 0;
    for (int i = 0; i < item_switch.size(); i++) {
        sum += item_switch[i] * item_list[i].profit;
    }
    return sum;
}

void increment(std::vector<int>& vec) {
    auto it_bit = vec.end();
    it_bit--;
    while (*it_bit == 1) {
        *it_bit = 0;
        if (it_bit == vec.begin()) {
            return;
        }
        it_bit--;
    }
    *it_bit = 1;
}

int main() {
    long M = 25;
    Item i1(1, 10, 9);
    Item i2(2, 12, 8);
    Item i3(3, 14, 12);
    Item i4(4, 16, 14);
    std::vector<Item> items = { i1,i2,i3,i4 };
    std::vector<int> enable(4,0);
    std::vector<std::vector<int>> possible;
    for (int i = 1; i <= 16; i++) {
        if (weight(items, enable) <= M) {
            possible.push_back(enable);
        }
        increment(enable);
    }
    long pr = 0;
    for (int i = 0; i < possible.size(); i++) {
        long temp = profit(items, possible[i]);
        if (temp > pr) {
            pr = temp;
        }
    }
    std::cout << pr;
    return 0;
}

P.S. I did not implement the nice suggestion here regarding creation of objects, as during the assignment submission, I am supposed to make objects at run-time.

Since you declare a three parameter constructor, the default constructor will not be implicitly defined, so it does not need to be explicitly deleted.  There's no real harm in providing a default constructor, since you have initializers for all your members.  Alternatively, since the only constructor you supply requires three parameters that initialize all three members of your class, you do not need to provide initializers for them (although doing so can lead to fewer problems in the future if this is expanded on).

The comparison operators should a be declared as const functions.  The use of this-> in them is not necessary.

The output operator should take item as a reference to avoid making a copy.

The weight and profit functions assume that both provided vectors have the same size.  The size used to end the loop can be stored in a variable to avoid potentially recomputing it every time.

In increment, predecrement should be used for iterators (--it_bit;) to avoid making an unnecessary copy.  Have you considered using reverse iterators here (using vec.rbegin())?

The last for loop in main can use the range-for-loop (e.g. for (auto p: possible)).


Maintaining state in a pagination handler In my application I wrote a pagination code that retrieves data from the server as the user scrolls down. In this case, I need to mantain the current page, whether a request is being processed, and whether the server has more data to serve. I didn't want to use globals, but still needed to mantain these 3 variables state.

Here's my code:

fetchNextNotificationPage = (function() {

    var currentPage = 0;
    var maxPageHit = false;
    var fetching = false;

    return function () {

        // Prevent firing concurrent or pointless requests.
        if (fetching || maxPageHit) return;

        fetching = true;

       $.getJSON("GetPageUrl", { page: currentPage + 1 }, function (data) {

           if (!data.error) {

               // Finished fetching, so now it is safe
               // to say we are at previous page + 1.
               currentPage += 1;

               updateData(data.page);

               if (data.lastPage) {
                   maxPageHit = true;
               }
           }
           else {

               console.log(data.error);
           }

       }).always(function() {.

           fetching = false;
       });
   };
}());

Is there a better way to solve this other than using closure?

I think you have the right approaches, however, you can generalize your implementations, for example, on the second approach:

function installHandler ( that, eventName, userOneTime, userRegular ) {
    that.on(eventName, function ( event ) {
        that.off(eventName).on(eventName, userRegular);
        userOneTime ( event );
        userRegular ( event );
    });
}

installHandler ( $(this), "click",
    function () { alert ("special click"); },
    function () { alert ("regular click"); } );


Should I put python3 argparse filetype objects in a contextlib stack? I just read the Lib/argparse.py code (class FileType) on http://hg.python.org/cpython/file/default/Lib/argparse.py. File objects are opened without the with statement. For safe file opening/closing, should I instead encapsulate my argparse.ArgumentParser() in a contextlib stack like below? I feel very stupid for asking a question like this, but I am very much in doubt about the proper procedure.

import argparse
import contextlib
import gzip
import os
import fileinput
import sys

def main():

    with contextlib.ExitStack() as stack:
        input = argparse(stack)
        process_arguments(input)

def argparse(stack):

    parser = argparse.ArgumentParser()
    parser.add_argument('--input', default=[sys.stdin], nargs='+')
    d_args = vars(parser.parse_args())
    if d_args['input'] != [sys.stdin]:
        input = stack.enter_context(fileinput.FileInput(
            files=d_args['input'], openhook=hook_compressed_text))
    else:
        input = stack.enter_context(sys.stdin)

    return input

def hook_compressed_text(filename, mode):

    ext = os.path.splitext(filename)[1]
    if ext == '.gz':
        f = gzip.open(filename, mode + 't')
    else:
        f = open(filename, mode)

    return f

Here the class FileType from Lib/argparse.py:

class FileType(object):
    """Factory for creating file object types

    Instances of FileType are typically passed as type= arguments to the
    ArgumentParser add_argument() method.

    Keyword Arguments:
        - mode -- A string indicating how the file is to be opened. Accepts the
            same values as the builtin open() function.
        - bufsize -- The file's desired buffer size. Accepts the same values as
            the builtin open() function.
        - encoding -- The file's encoding. Accepts the same values as the
            builtin open() function.
        - errors -- A string indicating how encoding and decoding errors are to
            be handled. Accepts the same value as the builtin open() function.
    """

    def __init__(self, mode='r', bufsize=-1, encoding=None, errors=None):
        self._mode = mode
        self._bufsize = bufsize
        self._encoding = encoding
        self._errors = errors

    def __call__(self, string):
        # the special argument "-" means sys.std{in,out}
        if string == '-':
            if 'r' in self._mode:
                return _sys.stdin
            elif 'w' in self._mode:
                return _sys.stdout
            else:
                msg = _('argument "-" with mode %r') % self._mode
                raise ValueError(msg)

        # all other arguments are used as file names
        try:
            return open(string, self._mode, self._bufsize, self._encoding,
                        self._errors)
        except OSError as e:
            message = _("can't open '%s': %s")
            raise ArgumentTypeError(message % (string, e))

I've expanded on your example, and explored some alternatives.

Observations:

your def argparse shadowed the module; I changed its name.
using FileType to process the argparse input results in unclosed file warnings.  It opens the files, but does not close them.  So for multiple files, it is not right tool.
simpler just runs the FileInput without a context.  FileInput handles stdin itself.  It does not close stdin when done.  It closes other files, even when processing is interrupted.
main with the context, closes stdin.
I wrote a process_arguments with a break to test whether files are closed. FileInput properly closes the files, with or without the context.
FileInput also can read sys.argv, so you don't actually need argparse in this simple case.  See simplest.

The script:

import argparse
import contextlib
import fileinput
import sys

files = set()

def filetype():
    parser = argparse.ArgumentParser()
    parser.add_argument('--input', nargs='*',type=argparse.FileType('r'))
    args = parser.parse_args()
    print(args)
    # gives ResourceWarning: unclosed file

def simpler():
    parser = argparse.ArgumentParser()
    parser.add_argument('--input', nargs='*')
    args = parser.parse_args()
    input = fileinput.FileInput(
        files=args.input)
    process_arguments(input)

def simplest():
    # uses sys.argv[1:]
    process_arguments(fileinput.input())

def main():
    with contextlib.ExitStack() as stack:
        input = myparse(stack)
        process_arguments(input)

def myparse(stack):
    # change name so as to not shadow the module
    parser = argparse.ArgumentParser()
    parser.add_argument('--input', default=[sys.stdin], nargs='+')
    d_args = vars(parser.parse_args())
    print('d_args',d_args)
    if d_args['input'] != [sys.stdin]:
        input = stack.enter_context(fileinput.FileInput(
            files=d_args['input']))
    else:
        input = stack.enter_context(sys.stdin)
    return input

def process_arguments(input):
    try:
        print(input, input._files)
    except AttributeError as e:
        # error if input is not a FileInput
        print(e)
        return
    for l in input:
        print(input.filename(), input.fileno(), input.lineno(), input.filelineno())
        if not input.isstdin():
            files.add(input._file)
        if input.lineno()>15:
            break

if __name__=="__main__":
    # simplest()
    simpler()
    # main()
    if files:
        print([(f.name, f.closed) for f in files])
    print('is stdin closed?', sys.stdin.closed)

This function is a simpler example (without fileinput) of processing a list of files with a proper context.  I learned about this use of stdin.fileno() from another argparse bug issue, http://bugs.python.org/issue14156.

def other():
    # example with simple 'with open...'
    parser = argparse.ArgumentParser()
    parser.add_argument('--input', nargs='*', default=[sys.stdin])
    args = parser.parse_args()
    for file in args.input:
        if file is sys.stdin:
            # open(fileno) does not close the underlying file
            file = file.fileno()
        with open(file) as f:
            lines = f.readlines()
            print(f.name, f.fileno(), len(lines))


Generating table rows for a form using js As you can see, the following is pretty ugly and seems to be unnecessarily long.
Is there a shorter/neater way to do this?

function addNewRow(body, id){
    var myRow = document.createElement("tr");
    myRow.id = id+'';
    var c1 = document.createElement("td");
    var c2 = document.createElement("td");
    var c3 = document.createElement("td");
    var c4 = document.createElement("td");
    var d = document.createElement("div");
    myRow.appendChild(c1);
    myRow.appendChild(c2);
    myRow.appendChild(c3);
    myRow.appendChild(c4);
    myRow.appendChild(d);

    var i1 = document.createElement("input");
    var i2 = document.createElement("input");
    var i3 = document.createElement("input");
    var i4 = document.createElement("input");

    i1.type = 'text';
    i1.id = 'drwID_' + id;
    i1.name = 'drwID_' + id;
    i1.disabled = !editing;

    i2.type = 'text';
    i2.id = 'pos_' + id;
    i2.name = 'pos_' + id;
    i2.disabled = !editing;

    i3.type = 'text';
    i3.id = 'stm_' + id;
    i3.name = 'stm_' + id;
    i3.disabled = !editing;

    i4.type = 'text';
    i4.id = 'note_' + id;
    i4.name = 'note_' + id;
    i4.disabled = !editing;

    c1.appendChild(i1);
    c2.appendChild(i2);
    c3.appendChild(i3);
    c4.appendChild(i4);
    body.appendChild(myRow);
}

You need to use loops. Really, that's about it - all of the element creation and appending can be done in a loop, while the prefix for each of the element's name and id be stored in an array.

function addNewRow(body, id) {
    var myRow = document.createElement("tr"),
        d = document.createElement("div"),
        inputPrefix = ['drwID_', 'pos_', 'stm_', 'note_'];

    myRow.id = id + '';

    for(var i = 0; i < inputPrefix.length; i++){
        var td = document.createElement('td'),
            input = document.createElement('input');

        input.type = 'text';
        input.id = inputPrefix[i] + id;
        input.name = inputPrefix[i] + id;
        input.disabled = !editing;

        td.appendChild(input);
        myRow.appendChild(td);
    }

    myRow.appendChild(d);
    body.appendChild(myRow);
}

You are generating invalid HTML here though - trs cannot contain div elements.


Showing "open" or "closed" based on the current time I'm using this code to have a site show either "Open" or "Closed" depending on what the current time  is.
I'm a junior developer and I want to show this to my boss but one of the Senior Developers said to make it more robust/reusable before I do.

var currentDate = new Date();
var currentMonth = currentDate.getMonth();
var currentDay = currentDate.getDay();
var currentHour = currentDate.getHours();
var currentMinute = currentDate.getMinutes();

function openClose() {
  if(currentDay >= 1 && currentDay <= 4) {
    document.getElementById("friday").style.display = "none";
    if (currentHour >= 8 && currentHour <= 21) {
      document.getElementById("closed").style.display = "none";
    }
  else {
    document.getElementById("open").style.display = "none";
  }
}
if (currentDay === 5) {
  document.getElementById("mon-thurs").style.display = "none";
  if (currentHour >= 8 && currentHour <= 17.5) {
    document.getElementById("closed").style.display = "none";
  }
  else {
    document.getElementById("open").style.display = "none";
  }
}
if (currentDay === 0 || currentDay === 6) {
  document.getElementById("open").style.display = "none";
  document.getElementById("friday").style.display = "none";
}
};

setInterval(openClose(), 5000);
.layout-header .header-one #open,
.layout-header .header-one #closed,
.layout-header .header-one #friday,
.layout-header .header-one #mon-thurs {
   display: inline-block;
   color: #000;
}

Open

Closed

Sales: 8am - 8pm

Sales: 8am - 5:30pm

The first bug is that you are misusing setInterval().  You should be passing a function, not the void result of the function, when calling setInterval().

Even if you had used setInterval() correctly, you still have a second bug, which is that currentDate is set just once when the code loads.  If you call openClose() every five seconds, you would get the same result every time!

Third, the JavaScript sets .style.display = 'none', but never resets elements to be visible.  Therefore, even if the first two issues were corrected, your page would appear emptier and emptier through the course of the week.

The code should degrade gracefully in the absence of JavaScript.

Display decisions should be written in CSS.  Ideally, the JavaScript should refrain from making decisions like .style.display = 'none', and should restrict itself to doing the minimum necessary (i.e. setting classes) to let the CSS work.

There is a giant maintenance headache: the hours are encoded both in the HTML and the JavaScript.  It is likely that someone in the future would edit one but forget to edit the other accordingly.  Furthermore, the JavaScript representation is both cryptic (what does 17.5 mean?) and cumbersome (all those if conditions!).  I recommend changing the JavaScript code so that it interprets the opening hours from the HTML.  The easiest way would be to store the hours redundantly as machine-readable attributes in the HTML.

function isOpen(timeRangeEl, date) {
    var day = '' + date.getDay();
    var hhmm = ('0' + date.getHours()).slice(-2) + ':' + ('0' + date.getMinutes()).slice(-2);

    var days = timeRangeEl.getAttribute('data-days');
    var openTime = timeRangeEl.getAttribute('data-open');
    var closeTime = timeRangeEl.getAttribute('data-close');
    return days.indexOf(day) >= 0 && openTime <= hhmm && hhmm < closeTime;
}

function openClose() {
    var date = new Date();
    var display = document.getElementById('open-display');
    var els = display.getElementsByClassName('timerange');
    var anyActive = false;
    for (var i = 0; i < els.length; i++) {
        if (isOpen(els[i], date)) {
            anyActive = true;
            els[i].className = els[i].className.replace(/ *inactive\b/g, '');
        } else if (els[i].className.indexOf('inactive') < 0) {
            els[i].className += ' inactive';
        }
    }
    if (anyActive) {
        display.className = 'open';
    } else {
        display.className = 'closed';
    }
}

setInterval(openClose, 5000);
openClose();
#open-display.open > .timerange.inactive,
#open-display.open .days {
     display: none;
}
#open-display.open > .openclosed::before {
    content: 'Open';
}
#open-display.closed > .openclosed::before {
    content: 'Closed';
}

Sales: Mon - Thu 8am - 8pm

Sales: Fri 8am - 5:30pm


Making repeated ADODB queries from Excel-SQL Server I need to run 12 queries on a SQL Server DB.  Currently I pass the "SELECT..." string to a function below.

Is it optimal to open and close the connection again and again (as my code is doing), or can I set up an ADODB connection as a global variable and reuse the same connection?

Sub ConnectServer(FileName As String, rngToPrint As Range, strSqlQuery As String)
    Dim conn As ADODB.Connection
    Dim rs As ADODB.Recordset
    Dim sConnString As String
    Dim iCols As Long

    If Not (rs Is Nothing) Then
        If (rs.State And adStateOpen) = adStateOpen Then rs.Close
        Set rs = Nothing
    End If

    sConnString = "Poop;"

    Set conn = New ADODB.Connection
    Set rs = New ADODB.Recordset

    conn.CommandTimeout = 90

    conn.Open sConnString

    Set rs = conn.Execute(strSqlQuery)

    'Add Headers
    For iCols = 0 To rs.Fields.Count - 1
        Sheets(rngToPrint.Worksheet.Name).Cells(rngToPrint.Row - 1, iCols + 1).Value = rs.Fields(iCols).Name
    Next

    If Not rs.EOF Then
        ' Transfer result.
        rngToPrint.CopyFromRecordset rs
    ' Close the recordset
        rs.Close
    Else
        MsgBox "Error: No records returned.", vbCritical
    End If

    If CBool(conn.State And adStateOpen) Then conn.Close
    Set conn = Nothing
    Set rs = Nothing
End Sub

can I set up an ADODB connection as a global variable and reuse the same connection?

That would be the single worst thing to do. First because you would be making a critical resource globally scoped, and therefore you would lose control over its lifetime.

Second, because ADODB does connection pooling for you anyway, so the "cost" of opening a connection isn't really a thing; if a connection exists in the connection pool and is free to use, that's the connection ADODB will give you - it's there already.

From code perspective, a database connection should always be as short-lived as possible: open, fetch/execute, close.

This is interesting:

Dim rs As ADODB.Recordset

If Not (rs Is Nothing) Then
    If (rs.State And adStateOpen) = adStateOpen Then rs.Close
    Set rs = Nothing
End If

That whole block is never going to execute. You just declared rs and haven't assigned it yet: it's always going to be Nothing.

If CBool(conn.State And adStateOpen) Then conn.Close
Set conn = Nothing
Set rs = Nothing

If no error occurred, your connection is always going to be opened here. And if an error occurred, your code isn't even running anymore. Make the conn.Close unconditional, and get rid of the redundant assignments to Nothing; the objects are going out of scope at End Sub, they're being destroyed anyway.

Actually, that Set rs = Nothing is potentially extending the lifetime of the rs object: without it, there's no in-scope reference to rs beyond rs.Close, and (assuming the VBA runtime can destroy objects while inside a procedure scope) VBA can destroy the object. But because of that Set rs = Nothing, you're keeping a reference around, and VBA cannot destroy the object until it passes that instruction.

If Not (rs Is Nothing) Then

The parentheses are redundant here: If Not rs Is Nothing evaluates to the same thing. Not (rs Is Nothing) simply forces rs Is Nothing to be evaluated to a value before Not processes (inverts) it, ...but that's exactly what happens either way, because of operator precedence rules.

If CBool(conn.State And adStateOpen) Then

The explicit conversion to Boolean is redundant here. That And is actually a bitwise operator here; it's not clear how the bitwise operation correctly converts to a Boolean - I'd suggest having a little helper function for this:

Private Function HasFlag(ByVal value As Long, ByVal flag As Long) As Boolean
    HasFlag = (value And flag) = flag
End Function

That way you can do If HasFlag(conn.State, adStateOpen) Then and have crystal-clear intentions.

I said earlier:

  And if an error occurred, your code isn't even running anymore.

That's quite a problem. You need to handle runtime errors. Database connections drop, a syntax error in strSqlQuery is likely (you're taking it as a parameter - who knows if it's legal SQL?), the rngToPrint could be in another workbook - so many things can go wrong!

First thing to fix is this:

Sheets(rngToPrint.Worksheet.Name).Cells(...)

The Sheets collection contains charts and other non-worksheet objects. Use the Worksheets collection when you want worksheets. But the biggest issue is that it's implicitly referring to the ActiveWorkbook. Yet nothing says rngToPrint comes from that workbook: if the active workbook has worksheets "Sheet1", "Sheet2" and "Sheet3" and rngToPrint is in another workbook and its Worksheet.Name is "DATA", your code blows up right here.

Why retrieve the worksheet from the Sheets/Worksheets collection, by its name, when you already have a reference to it?

rngToPrint.Worksheet.Cells(...)

Now, handle runtime errors:

Public Sub DoSomething()
    On Error GoTo CleanFail

    'code

CleanExit:
    'cleanup code
    Exit Sub
CleanFail:
    'error handling/recovering code
    Resume CleanExit
End If

With this pattern you close the recordset and connection under the CleanExit label, and because it runs whether or not an error has occurred then you make the clean-up conditional to the state of your object references (e.g. if rs Is Nothing don't try to close it) and/or your connection.

Your parameters are all passed ByRef implicitly, and they could all passed ByVal instead. The name ConnectServer doesn't convey what the procedure is doing at all (it's doing much more than just connecting to a server), and it's implicitly Public as well; making it explicitly Public would be good, or make it Private if it's not meant to be used beyond the scope of the module it's written in.

Hungarian Notation prefixes are superfluous, and misleading:

iCols - you're looking at a Long, not an "Integer".
sConnString - the name says "string", what's the s for?
rngToPrint - a better name would be simply destination.
FileName - no prefix? Parameter isn't used, remove it.

Whenever you feel the need to comment "this chunk of code does X", like here:

'Add headers

...you're looking a a chunk of code that actually belongs elsewhere. In this case, iCols belongs in the scope of a helper procedure that iterates a given recordset's fields, and writes the names of the fields in the first row of a given Range - add an explicit reference to the ADOR (Microsoft ActiveX Data Objects Recordset 6.0 Library) library too, so you can early-bind the Fields and Field object types:

Private Sub WriteColumnHeader(ByVal target As Worksheet, ByVal source As ADOR.Fields, Optional ByVal headerRow As Long = 1)
    Dim current As Long
    Dim fld As ADOR.Field
    For Each fld In source
        current = current + 1
        target.Cells(headerRow, current).Value = fld.Name
    Next
End Sub

Note that iterating a collection of objects is faster with a For Each loop; keep For loops for iterating arrays.

I find it a bit weird that you're writing the column names in row rngToPrint.Row - 1: that means I can't pass "A1" to your function as a destination, because you'll be attempting to write to row 0 and blow up. I would assume the first row of the destination range is the row where the column headers belong.

MsgBox "Error: No records returned.", vbCritical

"no records" isn't necessarily an error. It depends on the query! If I give your procedure a query that lists all reports for employee injuries in the past week, I'll be happy to see no records returned.

I wouldn't bother interpreting the results like this in this procedure - it's beyond its purpose: you don't know whether the client code is expecting records or not.

You could, however, be informative about it:

MsgBox "Query was executed, but no records were returned.", vbInformation

And keep vbCritical for actual errors, like if there's a syntax error in the query, or if the connection times out or otherwise can't be made to the server for whatever reason.


Determining if a C-style string is alphabetic I'm looking for the fastest way to determine if a C-style string contains only alphabetic characters.

For this question, there are a few assumptions:

strlen(c) > 0
Strings are null-terminated
Continuous character encoding from a-z and A-Z is not guaranteed, but likely.
The string is not a null pointer, nor is the pointer its "default value".

#include <stdbool.h>
#include <ctype.h>

bool stralpha(const char *c)
{
    bool alphabetic = true;

    while(*c)
        alphabetic &= !!isalpha(*c++);

    return alphabetic;
}

/* so gcc does not complain */
int main(void){}

To be clear: I know that any performance difference in such a function will be minimal at best. This is just for fun.

In my case, I was having issues with the function returning false even when c was purely alphabetic. That is why I use double negation !! on isalpha(), because it is not guaranteed to return consistently 0 or 1 per the standard.

The main problem with your code (in terms of performance) is that it processes every character in the string regardless. It's probably better to actually exit early if you find a non-alpha character, with something like:

bool isStrAlpha (const char *chPtr) {
    // if (*chPtr == '\0') return false;
    while (*chPtr != '\0')
        if (! isalpha (*chPtr++))
            return false;
    return true;
}

And just a couple of extra notes:

This code works fine even for zero-length strings, assuming the intent is to ensure whatever characters are in the string are all alpha. In other words, it considers an empty string valid because it contains no non-alpha characters. If you want to ensure the string has to have at least one alpha, just reinstate the commented-out line at the start of the function.
Your second assumption is superfluous. A series of characters that are not terminated with \0 is not actually a string in C.
Whether alpha characters are likely to be consecutive is not relevant in my opinion, I usually prefer correct, portable code over speed. But, in any case, you can be reasonably certain that the people who created your implementation will have optimised the wazoo out of the character classification stuff.


Remove duplicated functions inside different if within a function? I have a function :
function __throwError(func) {
  if (func.length === 1) {
    function passNumber() {
      func(0);
    }
    function passString() {
      func("item");
    }
    function passEmptyArray() {
      func([]);
    }
    function passUndefinedOrNull() {
      func(undefined || null);
    }
    expect(passNumber).toThrowError("The parameter should be an array");
    expect(passString).toThrowError("The parameter should be an array");
    expect(passEmptyArray).toThrowError("The array is empty");
    expect(passUndefinedOrNull).toThrowError("The parameter is null or undefined");
  }
  if (func.length === 2) {
    function passNumber() {
      func(0, 1);
    }
    function passString() {
      func("item", 1);
    }
    function passEmptyArray() {
      func([], 1);
    }
    function passUndefinedOrNull() {
      func(undefined || null, 1);
    }
    expect(passNumber).toThrowError("The parameter should be an array");
    expect(passString).toThrowError("The parameter should be an array");
    expect(passEmptyArray).toThrowError("The array is empty");
    expect(passUndefinedOrNull).toThrowError("The parameter is null or undefined");
  }
}

You may notice that there are duplicated passNumber,passString,passEmpty inside different if statements and each function call different callback function func.
How do I remove duplicated functions: passNumber,passString,passEmpty and just have once call with different parameters ?

The only difference between the two parts is that the second part has an additional parameter 1? You can provide an array with the original param, if function.length is 2, push 1 into it. the just call func.apply(null, params).
Here is a solution base on the idea:
function __throwError (func) {
  const testcases = [
    new TestCase(func, 0, 'The parameter should be an array'),
    new TestCase(func, 'item', 'The parameter should be an array'),
    new TestCase(func, [], 'The array is empty'),
    new TestCase(func, undefined, 'The parameter is null or undefined'),
    new TestCase(func, null, 'The parameter is null or undefined')
  ];
  if (func.length === 2) {
    testcases.forEach(testcase => testcase.value.push(1));
  }
  for (const testcase of testcases) {
    expect(testcase.fn).toThrowError(testcase.errMsg);
  }
}

class TestCase {
  constructor (testFn, value, errMsg) {
    this.value = [value];
    this.errMsg = errMsg;
    this.testFn = testFn;
  }
  fn () {
    this.testFn.apply(null, this.value);
  }
}
```


Creating 3 sublists based on unique conditions I have written the following code for one of my requirements. Kindly help me if I can write this logic in a better.

public class PersonService {
    public PersonCategorized getCategorizedPerson(List<Person> allPersonFromDb) {
        List<Person> adultList = allPersonFromDb.stream().filter(PersonUtil::isAdult).collect(Collectors.toList());
        List<Person> underAgedList = allPersonFromDb.stream().filter(PersonUtil::isUnderAged).collect(Collectors.toList());
        List<Person> seniorCitizenList = allPersonFromDb.stream().filter(PersonUtil::isSeniorCitizen).collect(Collectors.toList());

        return new PersonCategorized(underAgedList, adultList, seniorCitizenList);
    }
}

public class PersonCategorized {
    private List<Person> underAgedPersonList;
    private List<Person> adultList;
    private List<Person> seniorCitizenList;

    public PersonCategorized(List<Person> underAgedPersonList, List<Person> adultList, List<Person> seniorCitizenList) {
        this.underAgedPersonList = underAgedPersonList;
        this.adultList = adultList;
        this.seniorCitizenList = seniorCitizenList;
    }

    public List<Person> getAdultList() {
        return adultList;
    }

    public List<Person> getSeniorCitizenList() {
        return seniorCitizenList;
    }

    public List<Person> getUnderAgedPersonList() {
        return underAgedPersonList;
    }
}

public class Person {
    private int age;
    private String name;

    public Person(String name, int age) {
        this.name = name;
        this.age = age;
    }

    public int getAge() {
        return age;
    }

    public String getName() {
        return name;
    }
}

public class PersonUtil {
    public static boolean isAdult(Person person) {
        return person.getAge() >= 18 && person.getAge() < 60;
    }

    public static boolean isUnderAged(Person person) {
        return person.getAge() >= 0 && person.getAge() < 18;
    }

    public static boolean isSeniorCitizen(Person person) {
        return person.getAge() >= 0 && person.getAge() < 18;
    }
}

One observation I had after posting here is I am iterating over the list 3 times to collect 3 different lists.

No much room for improvement here. The code is clear, readable, maintainable, short and nice. You could get rid of the three iterations by using a loop and filling three lists all in one iteration, but you'd sacrifice maintainability for a few microseconds saved. Don't do this unless you are in a real critical spot. For real world applications, keep it as it is.

One thing however is the PersonUtil. From an OO perspective, the Person should probably know whether it is underaged or senior or whatever and contain that business logic itself, so that you don't have to resort to a util-class. However, if the Person is an entity, eventually generated, you basically have no other choice.


SQL cursors and dynamic SQL The idea is to have a heading for each date returned by the query.  This could be any number of dates.  I dynamically altered the #table which is ugly but there is no other way.

Ultimately I would like to reduce the amount of cursors but I'm not sure if it is possible.  This is using the Northwind database on SQL Server 2000 (I know, its lifecycle has ended).

  USE Northwind

  DECLARE @startdate DATETIME, @enddate DATETIME, @sql VARCHAR(1000)
  SELECT @startdate = '1998-01-01', @enddate = '1998-06-01'

  DROP TABLE #OUTPUT
  DROP TABLE #TEMP

  CREATE TABLE #OUTPUT
  (
    product VARCHAR(50)
  )

  SELECT o.orderdate, p.ProductName
  INTO #TEMP
  FROM [Northwind].[dbo].[Orders] o
  join [order details] od ON od.orderid = o.orderid
  join Products p on p.ProductID = od.ProductID
  WHERE o.OrderDate >= @startdate AND o.OrderDate < @enddate

  --Cursor storage variables
  DECLARE @prod VARCHAR(30), @orderdate VARCHAR(20), @cnt AS INT
  --

  --Add date column headings
  DECLARE CUR1 CURSOR LOCAL FOR
    SELECT DISTINCT(CONVERT(VARCHAR, orderdate, 101)) FROM #TEMP

  OPEN CUR1
  FETCH NEXT FROM CUR1 INTO @orderdate

  WHILE @@FETCH_STATUS = 0
  BEGIN
    EXEC('ALTER TABLE #OUTPUT ADD [' + @orderdate + '] VARCHAR(20) DEFAULT 0')
    FETCH NEXT FROM CUR1 INTO @orderdate
  END
  --

  --Add left column containing list of products
  INSERT INTO #OUTPUT (product)
  SELECT DISTINCT(productname) FROM #TEMP
  --

  --Update all rows setting the approriate date column to the count
  DECLARE CUR2 CURSOR LOCAL FOR
    SELECT CONVERT(VARCHAR, orderdate, 101), productname, COUNT(*) as cnt
    FROM #TEMP
    GROUP BY CONVERT(VARCHAR, orderdate, 101), productname

  OPEN CUR2

  FETCH NEXT FROM CUR2 INTO @orderdate, @prod, @cnt

  WHILE @@FETCH_STATUS = 0
  BEGIN
    SELECT @sql = 'UPDATE #OUTPUT set [' + @orderDate + '] =  [' + @orderdate + '] + ' + CAST(@cnt AS VARCHAR) + '  WHERE LTRIM(RTRIM(product)) = ''' + LTRIM(RTRIM(REPLACE(@prod, '''', ''))) + ''' '
    EXEC(@sql)
    PRINT @sql
    FETCH NEXT FROM CUR2 INTO @orderdate, @prod, @cnt
  END

  SELECT * FROM #OUTPUT

The result should look like this:

product           01/01/1998    01/02/1998
Alice Mutton           0            2
Aniseed Syrup          3            1
Boston Crab Meat       1            2

You stated you are stuck with SQL Server 2000, which sucks; but with that in mind, here are my thoughts. Bear in mind, I'm not a SQL Server specialist, and other answers may provide a more appropriate solution.

Good things

Your SQL syntax & indentation are great. There are a few keywords that are in different case, but overall I think you are good. Your logic is easy to follow. There are a lot of missing ; statement terminators but SQL Server is very permissive. I recommend you always use terminators. Also, your single letter aliases could be improved. I will include that in my code below the next section.

But now, I must address your...

Approach

What you are essentially doing is a pivot. SQL Server 2000 is an outdated product, and there are much more appropriate solutions available with newer versions, which require neither a CURSOR nor dynamic SQL. That said, I strongly question the usefulness of the result data set.

If you're querying only a few days, sure; but, imagine a query like that over a year or longer. You would be stuck with 365+ columns of data to look through rows for each product. It would be much more useful if the data was not pivoted, i.e., have a column for products and a column for dates. It is much easier to graph using Excel and such. And pivoting the data is a trivial task in Excel.

SQL really sucks at looping, and a cursor does exactly that which sucks. My opinion is that you would be better off just getting the data and not locking down your database server while this cursor is working to rearrange how the data is displayed to pivot it. Here is an example query using Northwind:

  USE Northwind;

  DECLARE @startdate DATETIME;
  DECLARE @enddate DATETIME;
  SET @startdate = '1998-01-01';
  SET @enddate = '1998-06-01';

  SELECT
      ord.orderdate,
      prod.ProductName,
      COUNT(ordDetail.Quantity) AS [Qty Sold]
  FROM [Northwind].[dbo].[Orders] AS ord
    JOIN [order details] AS ordDetail
       ON ordDetail.orderid = ord.orderid
    JOIN Products AS prod
       ON prod.ProductID = ordDetail.ProductID
  WHERE ord.OrderDate >= @startdate
    AND ord.OrderDate < @enddate
  GROUP BY [Qty Sold],
      ord.orderdate,
      prod.ProductName;

And just let the user pivot and manipulate it as they see fit. The purpose of a database system is to manage data, not manage how data is presented.


Improving time complexity for Count the Islands DFS For game AI development, I am currently using a slight modification on the DFS "count the islands" problem (specification below) as a solution to the One Hive Rule for the game of Hive. This may not be the ideal solution so I am open to any other ideas if you note DFS is not the best approach. Thank you.

Given a 2d grid map of '1's (land) and '0's (water), count the number of islands. An island is surrounded by water and is formed by connecting adjacent lands horizontally or vertically. You may assume all four edges of the grid are all surrounded by water.

My version utilises a 2D map (matrix) which represents a hexagonal board for the game.
The differences are:

My implementation uses hexagons so each cell has 6 neighbours, not 8
Blanks represent 0s and anything else are 1s
My implementation purposely stops if/when it finds more than one island/hive

My question is can the code be improved with regards to time complexity?
from insects import Blank

class HiveGraph:
    def __init__(self, board):
        self.row = board.height
        self.col = board.width
        self.visited = [[False for _ in range(self.col)] for _ in range(self.row)]
        self.graph = board.board

    # A function to check if a given hexagon can be included in DFS
    def is_safe(self, row, col):
        # row number is in range,
        # column number is in range,
        # and hexagon is Blank and not yet visited
        return (0 <= row < self.row and
                0 <= col < self.col and
                not self.visited[row][col] and
                type(self.graph[row][col]) is not Blank)

    # DFS for a 2D matrix. It only considers
    # the 6 neighbours as adjacent pieces
    def dfs(self, row, col):
        print(row, col)
        # These arrays are used to get row and
        # column numbers of 6 neighbours
        # of a given hexagon
        if col % 2 == 1:
            row_nbr = [-1,  0, 0,  1, 1, 1]
            col_nbr = [ 0, -1, 1, -1, 0, 1]
        else:
            row_nbr = [-1, -1, -1,  0, 0, 1]
            col_nbr = [-1,  0,  1, -1, 1, 0]

        # Mark this hexagon as visited
        self.visited[row][col] = True

        # Recur for all connected neighbours
        for k in range(6):
            if self.is_safe(row + row_nbr[k], col + col_nbr[k]):
                self.dfs(row + row_nbr[k], col + col_nbr[k])

    def one_hive(self):
        # Initialize count as 0 and traverse
        # through the all hexagons of given matrix
        count = 0
        for row in range(self.row):
            for col in range(self.col):
                # If a hexagon not Blank and is not visited yet,
                # then new hive found
                if not self.visited[row][col] and type(self.graph[row][col]) is not Blank:
                    # Visit all hexagons in this hive
                    # and increment hive count
                    count += 1
                    if count > 1:
                        return False
                    self.dfs(row, col)
        return True

Refer to this image for row, col matrix values for the layout of hexagons:

Given there are only 22-28 tiles in a Hive game, the time complexity of the check probably doesn't matter much.  Nevertheless, a modified scanline fill algorithm might be faster than a DFS.  This answer explains the algorithm fairly well. In this use case it might be easier to scan down the columns instead of across the rows.
From the rules of the Hive game, it seems the "one hive rule" would need to be checked when a tile is put down on the playing field and when it is picked up from the playing field (e.g., to move it).  When a tile is put down, it is sufficient to make sure it touches another tile (beside or on top).
When a tile is picked up, it is necessary to make sure it's former neighbor tiles are still connected via another path.  This may be trivial such as when the neighbors are adjacent, or may require searching for a path using a DFS, BFS, spanning tree, or fill/paint algorithm.
In any case, the AI should know which tile is being moved, so it shouldn't be necessary to scan the hex grid.  Just start from one of the tiles neighbors.