Python webpage parser I have a working code, but it has a lot of flaws.

The main project is to get a value from a website. I need to complete this action million of times, so i download the webpage returned from the class method, and parse it afterwards. In case the page is already downloaded i just open that one up.

This code is working, however even with the multiple except socket.error i do sometimes get socket errors when running it in parallel.

Also i am kinda new to programming, so any advice would be really appreciated. (I know this code is kinda too long)

class Uniprot:
    """
    Gets the UNIPROT XML or HTML, very bad code.... Cant come up with a better one yet :(
    """

    def __init__(self, uniid):
        """
        Initialize stuff
        :param uniid: Same as in parent class
        :return:
        """
        self.uniid = uniid.upper()
        self.soup = ""

    def xml(self):
        """
        returns the full XML file
        :return:
        """
        xmlpage = urllib2.urlopen("http://www.uniprot.org/uniprot/%s.xml" % self.uniid, timeout=2)
        if xmlpage.getcode() != 200:
            return
        self.soup = BeautifulSoup(xmlpage)
        return self.soup.get_text

    def web(self):
        """
        Returns the full HTML file
        :return:
        """
        webpage = urllib2.urlopen("http://www.uniprot.org/uniprot/%s" % self.uniid, timeout=2)
        # UNIPROT error handler
        if webpage.getcode() != 200:
            return
        websoup = BeautifulSoup(webpage)
        return websoup.get_text

def uniprotannotation():
    """
    Chaos, and anarchy right again, same with the errors, plus its an HTML variable instead of an XML
    :return: Annotation score from UNIPROT
    """
    uniprotxml = ""
    soup = ""
    # Check if the folder is present
    if not os.path.isdir("%s/%s/UNIPROT" % (DATA_DIR, FASTA_OBJECT.organism())):
        os.makedirs("%s/%s/UNIPROT" % (DATA_DIR, FASTA_OBJECT.organism()))
    # Check if the file is present, if not create it
    try:
        f = open("%s/%s/UNIPROT/%s.html" % (DATA_DIR,
                                            FASTA_OBJECT.organism(), FASTA_OBJECT.accession()), "r")
        for i in f:
            uniprotxml += i
        f.close()
        # Check if the file is empty
        if os.stat("%s/%s/UNIPROT/%s.html" % (DATA_DIR,
                                              FASTA_OBJECT.organism(), FASTA_OBJECT.accession())).st_size == 0:
            os.remove("%s/%s/UNIPROT/%s.html" % (DATA_DIR,
                                                 FASTA_OBJECT.organism(), FASTA_OBJECT.accession()))
            sys.stderr.write(
                    "%s, removed empty html, running it again\n" % sys.argv[1])
            uniprotannotation()
        soup = BeautifulSoup(uniprotxml)
    except IOError:
        try:
            soup = BeautifulSoup(str(FASTA_OBJECT.Uniprot(sys.argv[1]).web()))
        except socket.timeout:
            sys.stderr.write(
                    "%s, HTML socket timeout, trying again\n" % sys.argv[1])
            uniprotannotation()
        except socket.error:
            sys.stderr.write(
                    "%s, HTML socket error, trying again\n" % sys.argv[1])
            uniprotannotation()
        f = open("%s/%s/UNIPROT/%s.html" % (DATA_DIR,
                                            FASTA_OBJECT.organism(), FASTA_OBJECT.accession()), "w")
        f.write(str(FASTA_OBJECT.Uniprot(sys.argv[1]).web()))
        f.close()
    if not soup:
        return "-"
    result = re.search("Annotation score: \d out of \d", str(soup.get_text))
    if not result:
        return "-"
    return re.search("\d", str(result.group())).group()[0]

This looks good, nice work! I do think Uniprot is a bit long, but mostly because of comments.

"""
Gets the UNIPROT XML or HTML, very bad code.... Cant come up with a better one yet :(
"""

No need to criticise yourself in docstrings. Aside from everything else, this makes your comment too long without information. If it's really that bad, people will notice! Just stick with explaining unclear aspects to the code, or giving context to why the code does what it does. Other unnecessary comments:

    Initialize stuff

Most people familiar with classes will know what __init__ does. Also avoid vague language like "stuff". Initialise parameters would tell you slightly more, initialise uniid or even Unification ID. I obviously made that up, but only that last one adds information (telling the user what a uniid is).

    :return:

I'm not familiar with the docstring convention you're using, but does it really insist on including return lines even when they're blank? Not to mention the fact that you do return things from two of these functions, but your docstring claims otherwise. Either remove it or make the return lines accurate.

Next, you only ever use uniid to do this:

    "http://www.uniprot.org/uniprot/%s.xml" % self.uniid

Why not just initialise the URL in __init__ instead then?

def __init__(self, uniid):
    self.url = uniid.upper("http://www.uniprot.org/uniprot/%s" % self.uniid)

Now you can use self.url and self.url + '.xml' instead. Still on __init__, is there any need to initialise soup? Or even to store it as an attribute at all? You only need to return it from the function. Like you do with websoup. If there's an advantage to keeping it, why not do the same with websoup? You may have good reason, but if there's inconsistent treatment in your code you should comment it for the sake of clarity.

Next, you use a couple magic numbers in your code. Magic numbers are basically when you put numbers into code but it's not really clear from context what they mean or why they have to be that value.

    if xmlpage.getcode() != 200:

This is presumable a success code that the page returns? But it would be more readable like this:

    if xmlpage.getcode() != Uniprot.SUCCESS_CODE:

This is a constant value now, that you'd define as part of the class here:

class Uniprot:

    SUCCESS_CODE = 200

Neater and more readable, it makes perfect sense now (not that your initial case was the most egregious). Similarly, you set timeout to 2 in both cases. This is clearer to follow since timeout=2 is quite readable. But it's better organised if you put this as a class constant or even a value in __init__ that defaults to 2. Either way, it allows a user to change the value. What if they have woefully slow internet? Allowing them access makes the class more useful.

Your file reading seems a bit messy. I can't even tell why you're using such a complex method to check if the file is empty after reading it. So let's start with what I do know. All your file reading can be reduced to this:

    filename = "{}/{}/UNIPROT/{}.html".format(DATA_DIR,
                                              FASTA_OBJECT.organism(),
                                              FASTA_OBJECT.accession()))
    with open(filename) as f:
        uniprotxml = f.read()

format is the newer string formatting function that's recommended over the old % formatting. with is a way of opening files that will always ensure they are closed properly, no matter what. It's also generally recommended. And f.read() just reads the whole file as a string in one go. No need to iterate over the file to read line by line and add that to uniprotxml.

Also you're using recursion to retry. But that's a messy solution. Instead consider using a loop, and call either return or break to end the loop when you get a successful result, but otherwise loop indefinitely (or for a limited number of tries).


Solving Project Euler Problem 1 using extension methods As a self taught developer I never did any of the Project Euler problems, so I decided to just start with Problem 1 which states

If we list all the natural numbers below 10 that are multiples of 3 or 5, we get 3, 5, 6 and 9. The sum of these multiples is 23.
Find the sum of all the multiples of 3 or 5 below 1000.

The first step had been to create an extension method which takes a lowerLimit and an upperLimit returning an IEnumerable<int>
public static IEnumerable<int> GetSequence(this int lowerLimit, int upperLimit)
{
    if (upperLimit < lowerLimit) { throw new ArgumentOutOfRangeException("lowerLimit needs to be less or equal to upperLimit"); }

    for (int i = lowerLimit; i < upperLimit; i++)
    {
        yield return i;
    }
}

which can be called for instance like so
int lowerLimit = 1;
var sequence = lowerLimit.GetSequence(1000);

I am not 100% sure about the name of that method but couldn't come up with an alternative name so for sure I would like to hear suggestions for that.
The meat about solving the problem
public static class SolverOfProblemOne
{
    public static int Solve(this int lowerLimit, int upperLimit, params int[] multiples)
    {
        if (upperLimit < 0 || lowerLimit < 0) { throw new ArgumentOutOfRangeException("lowerLimit and upperLimit needs to be greater than or equal to 0"); }
        if (multiples == null || multiples.Length == 0) { return 0; }

        return lowerLimit.GetSequence(upperLimit)
            .Where(i => i.IsMultipleOf(multiples))
            .Sum();
    }

    private static bool IsMultipleOf(this int value, IEnumerable<int> multiples)
    {
        foreach (int multiple in multiples)
        {
            if (value % multiple == 0) { return true; }
        }
        return false;
    }

}

To solve the actual problem one would call it like
int lowerLimit = 1;
int upperLimit = 1000;
int res = lowerLimit.Solve(upperLimit, 3, 5);

Any suggestions about the shown code are welcome.
Because math has closed its doors for me after finishing school, I don't know if there is any math trick to do this any faster or easier.

This is not so much of a code answer as a mathematical answer.

We can calculate directly the number of multiples \$m\$ of a natural number \$n\$ that are below the limit \$x\$.  It's simply \$\lceil x/n\rceil\$.  The sum of those multiples is:
$$n\sum_{i=1}^{\lceil x/n\rceil}= n\left(\frac{1}{2}(\lceil x/n \rceil)(\lceil x/n\rceil+1)\right)$$
So for \$n=3\$ and \$x=999\$, we get $$3\left(\frac{1}{2}(333)(333+1)\right)= 166833$$
And for \$n=5\$ and \$x=999\$, we get $$5\left(\frac{1}{2}(200)(200+1)\right)=99500$$
So we could calculate the answer is $$166833 + 99500 = 266333$$  However the problem here (hat tip to @MartinR !) is that we have double-counted multiples of 15.  To fix that, we need to subtract that count:

$$ 15\left(\frac{1}{2}(66)(66+1)\right) = 33165$$
So the correct final answer is $$266333 - 33165 = 233168$$


How to deal with nested objects which might be string or undefined in a clean way? So I'm parsing an object using Typescript. I am pushing a value from within the object into an array of strings.
Here's the code:
if (data.InstanceStatuses != undefined)
            if (data.InstanceStatuses[0].InstanceId != undefined)
                instanceIds.push(data.InstanceStatuses[0].InstanceId as string)

I feel there must be a better way to deal with the fact that both data.InstanceStatuses and data.instanceStatuses[0].InstanceId can be of type string || undefined, however I am not sure how to get it down to a simpler statement with less IF blocks.
Any ideas?

Typescript has supported optional chaining for a while. To do this concisely and avoid the somewhat inelegant as string type assertion, extract the possibly nested value into a variable, then check its typeof against string:
const instanceId = data.InstanceStatuses?.[0].InstanceId;
if (typeof instanceId === 'string') {
  instanceIds.push(instanceId);
}

This way, instanceId will be either the nested value, or, if InstanceStatuses doesn't exist, it'll be undefined. So, it'll only push to the array if the nested value exists and the value is a string.


Binary Search Tree implementation with topological validation import sys

class Node:
    def __init__(self, value):
        self.left = None
        self.right = None
        self.value = value

    def __str__(self):
        return f"{self.value} "

class Sum:
    def __init__(self, val):
        self.s = val

    def getS(self):
        return self.s

    def update(self, val):
        self.s += val

class BST:
    def __init__(self):
        self.root = None

    def insert(self, key):
        curr = self.root
        parent = None
        if self.root:
            while curr and curr.value != key:
                parent = curr
                if curr.value < key:
                    curr = curr.right
                else:
                    curr = curr.left
        else:
            self.root = Node(key)
            return

        if parent:
            if parent.value < key:
                parent.right = Node(key)
            else:
                parent.left = Node(key)

    def delete(self, key):
        pass

    def _doFind(self, root, key):
        if root:
            if root.value == key:
                return root
            if root.value < key:
                self._doFind(root.right, key)
            else:
                self._doFind(root.left, key)

    def find(self, key):
        self._doFind(self.root, key)

    def _inorder(self, root):
        if root:
            self._inorder(root.left)
            print(root, " ")
            self._inorder(root.right)

    def inorder(self):
        self._inorder(self.root)

    def _preorder(self, root):
        if root:
            print(root, " ")
            self._preorder(root.left)
            self._preorder(root.right)

    def preorder(self):
        self._preorder(self.root)

    def _postorder(self, root):
        if root:
            self._postorder(root.left)
            self._postorder(root.right)
            print(root, " ")

    def postorder(self):
        self._postorder(self.root)

    def sumRToL(self, root, s):
        if root:
            self.sumRToL(root.right, s)
            s.update(root.value)
            root.value = s.getS()
            self.sumRToL(root.left, s)

    def sumelementsfromRtoLinplace(self):
        s = Sum(0)
        self.sumRToL(self.root, s)

    def validate(self, root, low, high):
        # Look for iterative solutions as well, probably using some stack
        return (not root) or (low <= root.value <= high and (
                self.validate(root.left, low, root.value) and self.validate(root.right, root.value, high)))

    def validatebst(self):
        max = sys.maxsize
        min = -sys.maxsize - 1
        return self.validate(self.root, min, max)

    def isSameTree(self, p, q):
        # Task : Can a level order solve this. Any non-recursive solutions as stack depth is not reliable?
        """
         Checks the value as well as topological order
        :type p: Node
        :type q: Node
        :rtype: bool
        """
        if not p and not q:
            return True
        elif p and q and p.value == q.value:
            return self.isSameTree(p.left, q.left) and self.isSameTree(p.right, q.right)
        return False

def test_main():
    bst = BST()
    bst.insert(1)
    bst.insert(2)
    bst.insert(3)
    bst.insert(4)
    bst.insert(5)
    # bst.root.left = Node(34) # Mess up the tree
    # bst.insert(2)
    # bst.insert(3)
    # bst.insert(4)
    # bst.insert(5)
    # bst.sumelementsfromRtoLinplace()
    # bst.inorder()
    bst1 = BST()
    bst1.insert(1)
    bst1.insert(2)
    bst1.insert(3)
    bst1.insert(4)
    bst1.insert(5)

    print('Same tree : ', bst.isSameTree(bst.root, bst1.root))
    print("Valid Tree : ", bst.validatebst())

if __name__ == '__main__':
    test_main()

P.S : I had to create the Sum class as there's no way to share the same primitive int across stack calls as there is no pass by reference in Python. I wanted to avoid using global variables throughout.

Coding style
Python has some conventions about coding style, for example snake_case for variables and functions etc. You can find these in pep-8
Node.__repr__
for troubleshooting, this can be handy:
def __repr__(self):
    return f"Node({self.value})"

with optionally the values of the children elements too
BST.update
adding a simple method to add multiple nodes can make initialization a lot simpler:
def update(self, values):
    for value in values:
        self.insert(value)

It also allows you to do this immediately in the __init__
def __init__(self, values=None):
    self.root = None
    if values is not None:
        self.update(values)

and use something like this in your tests:
bst = BST(range(5))

Node
All of the methods you prepend with an _ make more sense as methods on the Node
_xxorder
for example _inorder:
def inorder(self):
    if self.left is not None:
        yield from self.left.inorder()
    yield self
    if self.right is not None:
        yield from self.right.inorder()

and then BST.inorder:
def inorder(self):
    return self.root.inorder()

You can easily foresee a reverse iteration too (for example to find the maximum of the tree:
def inorder_reverse(self):
    if self.right is not None:
        yield from self.right.inorder_reverse()
    yield self
    if self.left is not None:
        yield from self.left.inorder_reverse()

same goes for the _doFind. Node.find:
def find(self, key):
    if self.value == key:
        return self
    next = self.right if self.value < key else self.left
    if next is None:
        return None  # or raise IndexError
    return next.find(key)

and BST.find:
def find(self, key):
    return self.root.find(key)

magic methods
isSameTree compares 2 trees. Why not name it __eq__.
Your implementation doesn't really use seld, so it might make more sense to transfer it to Node to compare subtrees
Node.__eq__:
def __eq__(self, other):
    if other is None:
        return False
    return (
        self.value == other.value
        and self.left == other.left
        and self.right == other.right
    )

BST.__eq__:
def __eq__(self, other):
    return self.root == other.root

You can easily implement the Iterator protocol on BST:
__iter__ = inorder

and reversed:
__reversed__ = inorder_reverse

Sum
You don't need the Sum class, you can just pass on a value. Also this method seems more appropriate under the Node class:
def sumRToL(self, partial_sum=0):
    if self.right is not None:
        partial_sum = self.right.sumRToL(partial_sum)
    self.value += partial_sum
    if self.left is not None:
        self.left.sumRTol(self.value)
    return self.value

Using this on mutable values might have strange effects.
on BST:
def sumelementsfromRtoLinplace(self):
    if self.root is not None:
        self.root.sumRToL()

validate
checking whether your tree is valid can become very easy via the iterator we just implemented. Using pairwise from the itertool recipes:
def validate(self):
    return all(a > b for a, b in pairwise(self)) # or self.inorder() for extra clarity

testing
These unit tests can be better done in another file, importing this file, and using one of the unit test frameworks. I'm quite happy with py.test.
import pytest

from binary_tree import BST

def test_order():
    bst = BST(range(10))
    assert [item.value for item in bst.inorder()] == list(range(10))
    assert [item.value for item in bst] == list(range(10))

def test_reverse():
    bst = BST(range(10))

    items = list(reversed(range(10)))
    assert [item.value for item in bst.inorder_reverse()] == items
    assert [item.value for item in reversed(bst)] == items

def test_equal():
    bst1 = BST(range(5))
    bst2 = BST(range(5))
    bst3 = BST(range(6))
    bst4 = BST(range(-3, 6))

    assert bst1 == bst2
    assert bst1 != bst3
    assert bst3 != bst1
    assert bst1 != bst4
...

total code
from general_tools.itertools_recipes import pairwise

class Node:
    def __init__(self, value):
        self.left: Node = None
        self.right: Node = None
        self.value = value

    def inorder(self):
        if self.left is not None:
            yield from self.left.inorder()
        yield self
        if self.right is not None:
            yield from self.right.inorder()

    def inorder_reverse(self):
        if self.right is not None:
            yield from self.right.inorder_reverse()
        yield self
        if self.left is not None:
            yield from self.left.inorder_reverse()

    def preorder(self):
        yield self
        if self.left is not None:
            yield from self.left.inorder()
        if self.right is not None:
            yield from self.right.inorder()

    def postorder(self):
        if self.left is not None:
            yield from self.left.inorder()
        if self.right is not None:
            yield from self.right.inorder()
        yield self

    def find(self, key):
        if self.value == key:
            return self
        next = self.right if self.value < key else self.left
        if next is None:
            return None  # or raise IndexError
        return next.find(key)

    def __eq__(self, other):
        if other is None:
            return False
        return (
            self.value == other.value
            and self.left == other.left
            and self.right == other.right
        )

    def sumRToL(self, partial_sum=0):
        if self.right is not None:
            partial_sum = self.right.sumRToL(partial_sum)
        self.value += partial_sum
        if self.left is not None:
            self.left.sumRTol(self.value)

    def __str__(self):
        return f"{self.value} "

    def __repr__(self):
        return f"Node({self.value})"

class BST:
    def __init__(self, values=None):
        self.root: Node = None
        if values is not None:
            self.update(values)

    def insert(self, key):
        if self.root is None:
            self.root = Node(key)
            return
        curr = self.root
        parent = None
        while curr and curr.value != key:
            parent, curr = curr, curr.right if curr.value < key else curr.left
        if parent is not None:
            if parent.value < key:
                parent.right = Node(key)
            else:
                parent.left = Node(key)

    def update(self, values):
        for value in values:
            self.insert(value)

    def delete(self, key):
        pass

    def find(self, key):
        return self.root.find(key)

    def inorder(self):
        return self.root.inorder()

    def inorder_reverse(self):
        return self.root.inorder_reverse()

    def preorder(self):
        return self.root.preorder()

    def postorder(self):
        return self.root.postorder()

    def sumelementsfromRtoLinplace(self):
        if self.root is not None:
            self.root.sumRToL()

    def validatebst(self):
        return all(a > b for a, b in pairwise(self))

    __iter__ = inorder
    __reversed__ = inorder_reverse

    def __eq__(self, other):
        return self.root == other.root


Efficiently returning the string basis on current datacenter We have machine hostname as -

dbx111.dc1.host.com
dbx112.dc2.host.com
dcx113.dc3.host.com

dcx115.dev.host.com

Here dc1, dc2, dc3 and dev are our datacenter and we will be having only four datacenter as of now. And also it might be possible that machine hostname can have more dots in between separated by another domain in future.

Now I need to find out which datacenter my current machine is in as I will be running below code on the actual machine.

And also I have two flows as of now: USERFLOW and DEVICEFLOW.

public enum FlowTypeEnum {
    USERFLOW, DEVICEFLOW
}

Problem Statement:

If my machine is in DC1 and flow type is USERFLOW then I need to return /test/datacenter/dc1 but if flow type is DEVICEFLOW then I need to return /testdevice/datacenter/dc1
But if my machine is in DC2 and flow type is USERFLOW then I need to return /test/datacenter/dc2 but if flow type is DEVICEFLOW then I need to return /testdevice/datacenter/dc2.
And if my machine is in DC3 and flow type is USERFLOW then I need to return /test/datacenter/dc3 but if flow type is DEVICEFLOW then I need to return /testdevice/datacenter/dc3.
But if my machine datacenter is in DEV, and flow type is USERFLOW then I need to return "/test/datacenter/dc1" but if flow type is DEVICEFLOW then I need to return /testdevice/datacenter/dc1.

The only difference between USERFLOW and DEVICEFLOW is - For USERFLOW, I need to use /test and for DEVICEFLOW, I need to use /testdevice and other things are same.

TestingEnum class -

public class TestingDatacenter {

    public static void main(String[] args) {
        String LOCAL_POOK = DatacenterEnum.getOurlocation().toLocalPook(FlowTypeEnum.USERFLOW);
        System.out.println(LOCAL_POOK);
    }
}

DatacenterEnum class -

public enum DatacenterEnum {

DEV, DC1, DC2, DC3;

    public static DatacenterEnum getOurlocation() {
        return ourlocation;
    }

    public static String forCode(int code) {
        return (code >= 0 && code < values().length) ? values()[code].name() : null;
    }

    private static final DatacenterEnum ourlocation = compareLocation();

    private static DatacenterEnum compareLocation() {
        String currenthost = getHostNameOfServer();

        if (currenthost != null) {
            if (isDevMachine(currenthost)) {
                return DC1;
            }

            for (DatacenterEnum dc : values()) {
                String namepart = "." + dc.name().toLowerCase() + ".";
                if (currenthost.indexOf(namepart) >= 0) {
                    return dc;
                }
            }
        }
        return null;
    }

    public String toLocalPook(FlowTypeEnum f) {
        String prefix = "";
        // below if else, looks pretty  odd, may be it can be improved better?
        if (f.equals(FlowTypeEnum.DEVICEFLOW)) {
            prefix = "/testdevice";
        } else if (f.equals(FlowTypeEnum.USERFLOW)) {
            prefix = "/test";
        }
        if (this == DEV) {
            return prefix + "/datacenter/dc1";
        }

        return prefix + "/datacenter/" + name().toLowerCase();
    }

    private static final String getHostNameOfServer() {
        try {
            return InetAddress.getLocalHost().getCanonicalHostName().toLowerCase();
        } catch (UnknownHostException e) {
            // log an exception
        }

        return null;
    }

    private static boolean isDevMachine(String hostName) {
        return hostName.indexOf("." + DEV.name().toLowerCase() + ".") >= 0;
    }
}

I'm opting for code review to see whether there is any better way of doing this. Is there anything which can be simplified and improved?

final is meaningless on static methods :

private static final String getHostNameOfServer() {

For the following :

// below if else, looks pretty  odd, may be it can be improved better?
if (f.equals(FlowTypeEnum.DEVICEFLOW)) {
    prefix = "/testdevice";
} else if (f.equals(FlowTypeEnum.USERFLOW)) {
    prefix = "/test";
}

there are 2 alternatives

make a getPrefix() method on FlowTypeEnum, reducing the above code to :

prefix = f.getPrefix();

FlowTypeEnum could then look like this :

public enum FlowTypeEnum {
    USERFLOW("/test"), DEVICEFLOW("/testdevice");

    private final String prefix;

    FlowTypeEnum(String prefix) {
        this.prefix = prefix;
    }

    public String getPrefix() {
        return prefix;
    }
}

make a map that maps the the FlowTypeEnum instances to a prefix, reducing the above code to :

prefix = mapToPrefix.get(f);

In both cases you might even inline the prefix variable.

Try to make code resistant to likely changes. In this case, server names and paths are likely to change. Pull these from config files.

All these static methods don't seem to be at home on the DatacenterEnum, they don't really operate on DatacenterEnum instances. Perhaps they need to find a home on a new class, where they can be non static.

It is unwise to hard code the data centers as enum instances. It simply won't scale. Renaming, removing or adding data centers in the field will require a new release.

Use more meaningful names for variables :

dc -> dataCenter
f -> flowType

Replace this :

currenthost.indexOf(namepart) >= 0

by the more readable :

currenthost.contains(namepart)


What is the best option to replace a loop? I'm using Flask on Python and I'm displaying the username at the top of the site (and it's working) as follows:
@app.route("/")
@login_required
def index():
    """Queries the user's First Name to display in the title"""
    names = db.execute("""
        SELECT first_name
        FROM users
        WHERE id=:user_id
        """,
        user_id = session["user_id"]
    )
    return render_template("index.html", names = names)

  {% for name in names %}

{{ name["first_name"] }}' Bookshelf

  {% else %}

Your Bookshelf

  {% endfor %}

I don't know if it is necessary, but here is my database configuration:

Although it is working, I believe that it is not ideal to use for loop to display just one name. Does anyone have any suggestions?

according to the query, it seems that the row count is either 0 or 1. so the loop would either have no iterations (the else part) or have exactly one iteration.
in terms of performance, I think that any improvement here is not dramatic.
however, in terms of choosing the right coding tools for the job, it seems that a loop is not the right tool.
maybe the following would be better:
@app.route("/")
@login_required
def index():
    """Queries the user's First Name to display in the title"""
    names = db.execute("""
        SELECT first_name
        FROM users
        WHERE id=:user_id
        """,
        user_id = session["user_id"]
    )
    first_name = names[0]["first_name"] if names else None
    return render_template("index.html", first_name = first_name)

and then:

  {% if first_name %}

{{ first_name }}'s Bookshelf

  {% else %}

Your Bookshelf

  {% endif %}

note: I didn't test the code.


Merge sort C# time and space efficiency Please comment on time and space efficiency.

using System;
using Microsoft.VisualStudio.TestTools.UnitTesting;

namespace JobInterviewTests
{
    [TestClass]
    public class MergeSortUnitTest
    {
        [TestMethod]
        public void MergeTest()
        {
            int[] array = new int[] { 5, 10, 1, 4, 2, 3, 7 };
            SortHalf(array, 0, array.Length-1);

            int[] expectedOutput = new[] { 1, 2, 3, 4, 5, 7, 10 };
            CollectionAssert.AreEqual(expectedOutput, array);
        }
        private void SortHalf(int[] array, int startIndex, int endIndex)
        {
            int mid;
            if (endIndex > startIndex)
            {
                mid = (endIndex + startIndex) / 2;
                SortHalf(array, startIndex, mid);
                SortHalf(array, mid + 1, endIndex);
                DoMerge(array, startIndex,mid+1, endIndex);
            }
        }

        private void DoMerge(int[] array, int startIndex, int mid, int endIndex)
        {
            int[] temp = new int[array.Length];
            int i, start, numOfElements, position;
            start = mid - 1;
            position = startIndex;
            numOfElements = endIndex - startIndex + 1;
            while((startIndex <= start)&& (mid <= endIndex))
            {
                if(array[startIndex] <= array[mid])
                {
                    temp[position++] = array[startIndex++];
                }
                else
                {
                    temp[position++] = array[mid++];
                }
            }
            while(startIndex <= start)
            {
                temp[position++] = array[startIndex++];
            }

            while(mid <= endIndex)
            {
                temp[position++] = array[mid++];
            }

            for (i = 0; i < numOfElements; i++)
            {
                array[endIndex] = temp[endIndex];
                endIndex--;
            }

        }
    }
}

mid = (endIndex + startIndex) / 2; may overflow. A canonical idiom is

    mid = startIndex + (endIndex - startIndex)/2;

Allocating (and deallocating) a new temp array in each invocation is a waste of time. Allocate the entire temp once, and pass it down, using same indices as for the actual array. Notice that it also allows to not copying data back from temp to array, which is another waste of time. Instead, ping-pong the arrays' roles.

Of course, it doesn't affect the asymptotic, but should speed it up twofold.
The algorithm looks much more natural with semi-open ranges (that is, end is one after the last element. This will avoid +1 and -1 index corrections, and use < instead of <= to compare indices.


Sorting a list of numbers, each with a character label I am working on a project that requires me to modify an input list as follows:

INPUT: List of numbers, each tagged with a label i.e. [[2, 'a'], [3, 'b'], [1, 'c'], ...]

DESIRED OUTPUT: Sort the list and grab the elements with the highest numerical component, store them in another list, and maybe rerun the function on the new list. This is an order determination scheme based on players rolling dice.

I have created the following code in Python 3.7 and it works just fine for me. I am curious to see if there are more clever ways to arrive at the same output. For some reason this took me about two hours to come up with (I am learning from scratch).

list = [[1,'a'],[3,'b'],[7,'c'],[6,'d'],[4,'e'],[1,'f'],[7,'g'],[4,'h'],[6,'i']]

def comp(list):

    new_list = []
    list.sort(reverse = True)
    for y in range(1, len(list)):
        if list[0][0] > list[y][0]:
            new_list.append(list[y])
    top_rolls = [x for x in list if x not in new_list]
    return(top_rolls)

Names

Using the name list for a variable is legal but usually avoided because it hides the list builtin. The name lst would be a better candidate.

The function name comp could be improved as well. Maybe get_best_elements (even though I am not fully convinced).

Tests

Before going further, it may be wise to add tests so that you can easily be sure that we do not break anything.

A proper solution would involve a unit-test framework but for the time being, we can go for the simple:

TESTS = (
    ([], []),
    ([[1,'a']], [[1, 'a']]),
    ([[1,'a'], [1,'a'], [1,'a'], [1,'a']], [[1,'a'], [1,'a'], [1,'a'], [1,'a']]),
    ([[1,'a'], [1,'b'], [1,'c'], [1,'d']], [[1,'d'], [1,'c'], [1,'b'], [1,'a']]),
    ([[1,'a'],[3,'b'],[7,'c'],[6,'d'],[4,'e'],[1,'f'],[7,'g'],[4,'h'],[6,'i']], [[7, 'g'], [7, 'c']]),
    ([[1,'a'],[3,'b'],[7,'c'],[6,'d'],[4,'e'],[1,'f'],[4,'h'],[6,'i']], [[7, 'c']]),
)

for test_input, expected_output in TESTS:
    output = get_best_elements(test_input)
    assert output == expected_output
print("OK")

Style

Parenthesis are not required in the return statements.
Also, the temporary variable is not really required.

List comprehension

The new_list could be defined with a list comprehension.

def get_best_elements(lst):
    lst.sort(reverse = True)
    new_lst = [lst[y] for y in range(1, len(lst)) if lst[0][0] > lst[y][0]]
    return [x for x in lst if x not in new_lst]

Loop like a native

I highly recommend Ned Batchelder's talk "Loop like a native" about iterators. One of the most simple take away is that whenever you're doing range(len(lst)), you can probably do things in a better way: more concise, clearer and more efficient.

In our case, we'd have something like:

def get_best_elements(lst):
    lst.sort(reverse = True)
    new_lst = [e for e in lst if e[0] < lst[0][0]]
    return [x for x in lst if x not in new_lst]

Another approach

The last list comprehension can be quite expensive because for each element x, you may perform a look-up in the list. This can lead to a O(n²) behavior.

Instead of filtering out elements that do not correspond to the biggest number, we could just keep the one that do correspond to the biggest number.

def get_best_elements(lst):
    if not lst:
        return []
    lst.sort(reverse = True)
    big_n = lst[0][0]
    return [x for x in lst if x[0] == big_n]

Sort a smaller numbers of elements

You could use max to get big_n.

Also, we could perform the sorting on the filtered list so that we have fewer elements to sort:

def get_best_elements(lst):
    if not lst:
        return []
    big_n = max(lst)[0]
    return list(sorted((x for x in lst if x[0] == big_n), reverse=True))


Dijkstra's Algorithm in Haskell I really enjoy Haskell but feel I still have a total beginner's style, and would like to move beyond that.  The code below - for Dijkstra's shortest path algorithm - is a case in point.  I feel as though I have ended up with a copy of the imperative pseudo code I began with, having done some small transformations, but not the sort of big ones an expert would come up with.  (Perhaps it was the very fact that I had pseudo code to start with that got me into this mess - without it would I have pursued a generate and prune strategy?)

But the big decision I needed was how to store the 3 elements of state

The results
The Dijkstra scores of the points not yet turned into results; and
The set of points already found (i.e. the keys of the results)

In the end I used a Vector to Dijkstra scores, and have been passing around my state variables, whereas they could probably have been put into the background with a Monad.  I know that having Vector manipulations in two places is not smart (I could probably have reduced that 1 by passing the deletion request into recalc....).

I'd welcome any constructive comments, on how an expert would approach the state challenge.

import qualified Data.ByteString.Char8 as BS
import qualified Data.Vector as V
import qualified Data.List   as L

type NodeName = Int
type Dist = Int
type Edge = (NodeName, Dist)
type Edges = [Edge]
type Graph = V.Vector Edges

-- STATE
type Explored = [NodeName] -- IntSet?
type Results = [(NodeName, Dist)] -- need to add to randomly
type DijkstraScores = V.Vector (Maybe Dist)

-- take top from DijkList
-- add to results
-- get update DijkList
mainLoop :: Graph -> Explored -> DijkstraScores -> Results -> Results
mainLoop gr exs ds res =
    let
        minIdx = V.minIndexBy maybeOrder ds
        Just minVal = ds V.! minIdx
        res' = (minIdx,minVal) : res
        exs' = minIdx : exs
        newDists = filter (not . (`elem` exs') . fst) $ gr V.! minIdx
        newDs = recalcDijkDist newDists minVal $ ds V.// [(minIdx,Nothing)] -- remove this value
    in case V.all (==Nothing) newDs of
        True -> res
        False -> mainLoop gr exs' newDs res'

-- run over new Dists
    -- change if lower
    -- leave if higher
    -- add if no existing data
    -- recombine with all other data
recalcDijkDist :: Edges -> Dist -> DijkstraScores -> DijkstraScores
recalcDijkDist newDists distOfNewPoint ds =
    let
        comps = map process newDists
        process :: Edge -> (Int, Maybe Int)
        process (x,y) =
            case ds V.! x of
                Just z -> (x, Just $ min (y + distOfNewPoint) z)
                Nothing -> (x, Just $ y + distOfNewPoint)
    in ds V.// comps

getEdges :: String -> IO Graph
getEdges path = do
    -- init removes a trailing '\r'
    lines <- (map (init . (BS.split '\t')) . BS.lines) `fmap` BS.readFile path
    let gr = V.fromList $ ([] :) $ map processLine lines
    return gr

processLine :: [BS.ByteString] -> Edges
processLine (n:connections) = map splitter connections
    where
        myread  = maybe (error "can't read Int") fst . BS.readInt
        splitter c = let
                (x,y) = (\(xx,yy) -> (xx, BS.tail yy)) $ BS.break (==',') c
            in (myread x, myread y)

main = do
    gr <- getEdges "dijkstra.txt"
    let
        --mloop = mainLoop gr [] (initDijkstra $ gr V.! 1) []
        mloop = mainLoop gr [] (recalcDijkDist (gr V.! 1) 0 $ V.replicate 201 Nothing) []
        anstoFind = [7,37,59,82,99,115,133,165,188,197]
        -- maybe :: b -> (a -> b) -> Maybe a -> b
        answers = map (\x -> snd $ maybe (error "not found") id $ L.find ((==x).fst) mloop) anstoFind
    return answers
    --return gr

-- Nothing is interpreted as 0 otherwise
maybeOrder :: Maybe Int -> Maybe Int -> Ordering
maybeOrder Nothing x = GT
maybeOrder x Nothing = LT
maybeOrder (Just x) (Just y) = compare x y

Nice exercise!

  But the big decision I needed was how to store the 3 elements of state

  The results
  The Dijkstra scores of the points not yet turned into results; and
  The set of points already found (i.e. the keys of the results)

An important feature of the algorithm is that the scores are kept in a min-priority-queue. Keeping the scores in a vector requires O(n) cost at every step. Also note that having using a priority queue API would simplify the mainLoop, as the operations on a vector are somewhat low-level wrt the task (remove the minimal value from the queue, if it exists).

The explored nodes form a set, to it'd be indeed more appropriate to use IntSet or Set. Whenever you use elem with [], it's a sign to think of a better data structure (unless you know the list length is bounded by some small constant).

Regarding the results, we know we add each node only once, so [] seems to be OK. Adding is then O(1) and whatever manipulation we want to do at the end can be deferred until then.

I don't think there is anything wrong with your approach to keeping the state. While you could use the State monad (or perhaps RWS where the reader part would be the graph, the writer part the results and the state the scores), it'd force (as any monad) more imperative, sequential style. And it'd cloud which part of the code needs what information. And having the value that's being modified as the last argument (like ... -> Results -> Results) allows to use point-free style where appropriate.

You might want to check if your mainLoop is tail-recursive.

Perhaps one way how to make mainLoop have less imperative feel would be to check for the termination condition first (which isn't such a bad idea, if the input is an empty graph or so). Something like:

mainLoop gr = loop
  where
    loop exs ds res | ifQueueEmpty ds = res
                    | otherwise =
      let ...
      in loop exs' newDs res'

There are multiple reasons for separating the loop in an inner function. One (that doesn't really apply here) is that by having the top-level function non-recursive, it allows it to be inlined. The other is that this clearly expresses what changes during the loop and what not.

Some more remarks:

I like that you included types for all functions, including helpers, that's often very convenient. And that you gave descriptive names to the types.

From the types it's not that obvious how are graphs represented. If an edge is

type Edge = (NodeName, Dist)

what is the other node it connects? My guess is that the other node is determined by the index of the [Edge] list in the vector. In any case, this should be documented in comments for the types.

Also, a question comes to my mind, why the graph is a Vector, but the list of edges is a []?

Regarding comments, I'd suggest you to use the Haddock syntax, as it's then very easy to generate documentation from the code.

Also it's not clear what Nothing means in the vector. It could mean that an element hasn't been explored yet (infinite distance), which would work well with manipulating distances, but it seems to be used for elements that have been removed from the queue. I'd rather use a queue that allows proper removal of elements, and use Dist that deals with infinity (see below)

For representing distances it'd be useful to have a separate data type, something like

newtype Dist = Dist (Maybe Int)

with the appropriate Ord instance and supply the minimum monoid instance where mempty is the infinity and mappend takes the minimum of its arguments. For example process could be greatly simplified by this.

The comments are sometimes somewhat misleading, for example in Nothing is interpreted as 0 otherwise it's not clear what "otherwise" means.


Is this a good Ruby implementation of Prime Factorization? I'm doing some practice problems on Khan Academy. The current one is Prime factorization. I came up with this:

require 'prime'

def prime_factorization(n, primes = nil)
  return [n] if Prime.prime? n

  primes ||= Prime.take_while { |p| p < n/2 }
  factorization = []
  prime = primes.detect { |p| n % p == 0 }
  factor = n / prime
  factorization << prime

  if Prime.prime? factor
    factorization << factor
  else
    factorization += prime_factorization factor, primes
  end

  factorization
end

The benchmarks for this are:

                            user       system     total       real
prime factorization of 75:  0.000000   0.000000   0.000000 (  0.000129)
prime factorization of 750:  0.000000   0.000000   0.000000 (  0.000241)
prime factorization of 4202:  0.000000   0.000000   0.000000 (  0.000498)
prime factorization of 39450:  0.010000   0.000000   0.010000 (  0.003061)
prime factorization of 460522:  0.050000   0.000000   0.050000 (  0.057704)

What is the big O notation for this method?
How does this algorithm compare to optimal prime factor algorithms in terms of complexity/performance?
Is recursion a good way to solve this problem?
How could it be improved?
Is this a stupid question?

Reinventing the wheel

There is an easier solution, considering that you are using the Prime class, which already has a Prime.prime_division() method that does almost the same thing.  The only difference is the output format: your prime_factorization(360) would output [2, 2, 2, 3, 3, 5], whereas Prime.prime_division(360) would output [[2, 3], [3, 2], [5, 1]].  It's just a matter of converting the output by repeating each prime factor the specified number of times.

require 'prime'

def prime_factorization(n)
  Prime.prime_division(n).flat_map { |factor, power| [factor] * power }
end

(Thanks to @Flambino and @tokland for simplifying the transformation.)

Critique

You've asked some very interesting questions.

The big-O complexity of this method is not at all simple to analyze.  You call methods such as Prime.prime?() that in turn make use of a generator.  Then you also call Prime.take_while(), Prime.prime?(factor).  On top of that, there's recursion.   All I will say, though, is that the method can be very inefficient.

The initial call to prime?(n) already does a trial division that resembles the kind of work your method will perform.  Calling prime? is altogether redundant — see Simplification 1 below.

The call to primes ||= Prime.take_while { |p| p < n / 2 } can also be very inefficient.  For example, to calculate prime_factorization(360), you would be asking it to generate primes up to 179, even though no prime factor above 5 exists.

Ruby provides an Integer.divmod() function that lets you avoid having to recalculate n / p immediately after having ascertained n % p == 0.

When you recurse, you test every prime number all over again, starting with 2, 3, 5, ….  Rather, you want to be able to continue by testing the same prime factor that succeeded.

Simplification 1

Assuming that you are deliberately reinventing-the-wheel, you can still improve your method in several ways:

Eliminate calls to prime?
Make early termination work by removing take_while in favour of a break
Use divmod
Replace recursion with looping
Just yield factors as you detect them.  Let to_enum and to_a take care of appending results to the array.

def prime_factorization(n)
  def factor_generator(n)
    Prime.instance.each do |p|
      break if p > n
      begin
        div, mod = n.divmod(p)
        if mod == 0
          yield p
          n = div
        end
      end while mod == 0
    end
  end

  to_enum(:factor_generator, n).to_a
end

Simplification 2

The implementation above is still inefficient, since there is already primality testing hidden inside Prime.instance.each.  To avoid duplication of effort between your code and the Prime class, you should be able to test any increasing sequence of numbers that is not obviously composite.

def prime_factorization(n)
  def factor_generator(n)
    for prime_candidate in Prime::Generator23.new
      break if prime_candidate > n
      begin
        div, mod = n.divmod(prime_candidate)
        if mod == 0
          yield prime_candidate
          n = div
        end
      end while mod == 0
    end
  end

  to_enum(:factor_generator, n).to_a
end


Displaying job status in a jQuery Datatable I was wondering if there was a better way to write this:

#Define Jquery Datatable
if $('#dtable').length
  table = $('#dtable').DataTable()

#If job is created, make new row in Jquery datatables
if data.status == "Created" then (
  count = 0
  new_data = [
    data.job_id
    data.start_ip
    data.end_ip
    data.ilo_username
    data.ilo_password
    data.status
    create_date
    'Details'
  ]
  new_row = table.row.add(new_data).draw().nodes().to$().find('td').each ->
    $(this).attr 'id', 'td_' + count++ + '_' + data.job_id
  new_row
  id = $('#row' + data.job_id).val()
  table.row(new_row).node().id = 'row' + data.job_id
)

#If job status is running
else if data.status != "Created" and data.status != "Job Deleted" and data.status != "Scan Complete" then (
  #Update Jquery datatable if it exists
  if $('#dtable').length
    table.cell('#td_5_' + data.job_id).data wait_status

  #Update job detail status if it exists
  if $(detail_status).length
    $(detail_status).html wait_status

  #Detail Status
  if $(respond_message).length then (
    if data.count == null or data.server_count != 0
      $(respond_message).html wait_message
    else if data.server_count == 0
      $(respond_message).html no_respond
  )
)
#If job status is complete
else if data.status == "Scan Complete" then (
  #Update Jquery Datatable if it exists
  if $('#dtable').length
    table.cell('#td_5_' + data.job_id).data(data.status)

  #Update job detail status if it exists
  if $(detail_status).length
    $(detail_status).html data.status

  #Detail Status
  if $(respond_message).length then (
    if data.server_count == 0
      $(respond_message).html no_respond
    else if data.server_count != 0
      $(respond_message).html respond_finish
  )

#Remove disabled class on buttons if exists
  if $('#edit_disable_' + data.job_id).length and data.status == 'Scan Complete'
    $('#edit_disable_' + data.job_id).removeClass 'disabled'
  if $('#delete_disable_' + data.job_id).length and data.status == 'Scan Complete'
    $('#delete_disable_' + data.job_id).removeClass 'disabled'
)
#If job is deleted, remove table row from Jquery datatable
else if data.status == "Job Deleted"
  tr = "#row" + data.job_id
  table.row(tr).remove().draw()

Even though it is working, the code seems a little sloppy to me with all the if/else statements.  Is there any way to simplify it?  I'm new to coffeescript.

First of all, you may benefit from the fact that in Coffee (and in JS, too), the following code

if condition
  variable = value

can be expressed simpler as

variable = condition && value

Therefore, the declaration of the table may be written as

table = $('#dtable').length && $('#dtable').DataTable()

Later in the code, you should not check again for the $('#dtable').length but you should check if the table has a value, possibly with the existential operator. For example, replace the

if $('#dtable').length
    table.cell('#td_5_' + data.job_id).data wait_status

with

table?.cell('#td_5_' + data.job_id).data wait_status

I don't know what the detail_status is but I suppose it is a string selector. Therefore, you don't need to check if it is available in the DOM. JQuery will do nothing if there is no such element, so you can safely call $(detail_status).html wait_status without checking for its length. The same goes for e.g. removeClass method.

You can also use Coffee's string interpolation and replace the "#row" + data.job_id with "#row#{data.job_id}" (and similar).

Last but not least, try replacing multiple conditions like if data.status != "Created" and data.status != "Job Deleted" and data.status != "Scan Complete" with one statement with the in operator, i.e. if data.status not in ['Created', 'Job Deleted', 'Scan Complete'].


Sort all the characters according to their brightness I wrote a little fractal noise generator the other day, and to test it I wanted to display the dark values with a darker character and the bright values with bright characters (I use a dark background with light foreground). So I wrote a simple java program to do this, and here it is:

package com.loovjo.brightSort;

import java.awt.Font;
import java.awt.Graphics;
import java.awt.event.KeyEvent;
import java.awt.image.BufferedImage;
import java.util.ArrayList;
import java.util.HashMap;
import java.util.Map.Entry;
import java.util.stream.Collectors;

public class Main {

    private static final int IMG_SIZE = 128;

    public static void main(String[] args) {
        ArrayList<Character> order = new ArrayList<Character>();

        HashMap<Character, Float> brightness = new HashMap<Character, Float>();

        long timer = System.currentTimeMillis();

        int max = 0xFF;

        for (char c = 0; c < max; c++) {
            Character.UnicodeBlock block = Character.UnicodeBlock.of(c);
            if (!(!Character.isISOControl(c)) && c != KeyEvent.CHAR_UNDEFINED && block != null
                && block != Character.UnicodeBlock.SPECIALS)
                // We don't want any unprintables in out gradient.
                continue;

            brightness.put(c, getBrightness(c));
            if (System.currentTimeMillis() - timer > 100) {
                timer = System.currentTimeMillis();
                System.out.println(((float) c / max) * 100 + "% done.");
            }
        }

        // Sort brightness into order

        while (!brightness.isEmpty()) {
            char min = '\0';
            float val = Float.MAX_VALUE;
            for (Entry<Character, Float> e : brightness.entrySet()) {
                if (e.getValue() < val) {
                    val = e.getValue();
                    min = e.getKey();
                }
            }
            brightness.remove(min);
            order.add(min);
        }
        for (int i = 0; i < 10; i++)
            System.out.println(order.stream().map(c -> "" + c).collect(Collectors.joining()));

     }

    private static float getBrightness(char c) {
        BufferedImage img = new BufferedImage(IMG_SIZE, IMG_SIZE, BufferedImage.TYPE_INT_RGB);
        Graphics g = img.getGraphics();
        g.setFont(new Font("Monaco", Font.PLAIN, (int) (img.getHeight() * 0.5)));
        // img.getHeight() * 0.5 because else some of the characters will be
        // rendered off-screen, giving them wierd values

        g.drawString(c + "", 0, g.getFont().getSize());
        g.dispose();

        int totalBrightness = 0;
        for (int x = 0; x < img.getWidth(); x++) {
            for (int y = 0; y < img.getHeight(); y++) {
                totalBrightness += img.getRGB(x, y) & 0xFF;
            }
        }

        return totalBrightness / (float) (img.getWidth() * img.getHeight());
    }

}

For those of you who can't run this program, here's a screenshot of the output:

You may want to separate the logic to determine if a character is printable into a method and rewrite the brightness calculation using streams:

Map<Character, Float> brightness = IntStream.range(0, max)
        .mapToObj(x -> (char) x)
        .filter(Main::isPrintable)
        .collect(toMap(c -> c, Main::getBrightness));

Your sort algorithm is very inefficient, and there isn't a need to write your own.  Streams API makes it nice and clean:

List<Character> order = brightness.entrySet().stream()
        .sorted(comparingByValue())
        .map(Map.Entry::getKey)
        .collect(toList());

Above snippets assume static imports

import static java.util.Map.Entry.comparingByValue;
import static java.util.stream.Collectors.toList;
import static java.util.stream.Collectors.toMap;


Tic-Tac-Toe project in Python Here's a Tic-Tac-Toe program I wrote in Python. How can I improve it (.eg. make it more readable)?

class Gameboard():

    def __init__(self):
        self.gameboard = {1:' ' , 2: ' ', 3: ' ', 4:' ',  5:' ',  6:' ',  7:' ',  8:' ',  9:' '}

    def setPeice(self, user, position, gameboard):
        gameboard[position] = user
        return gameboard
    @property
    def gameBoard(self):
        '''
        need to learn more about properties. They seem to be used primarily as
        getters and setters. ??
        '''
        return self.gameboard

    def clearboard(self):
        self.gameboard = {1:' ' , 2: ' ', 3: ' ', 4:' ',  5:' ',  6:' ',  7:' ',  8:' ',  9:' '}

    def is_place_taken(self, gameboard, index):
        if gameboard[index] != ' ' :
            return True

    def is_board_full(self, gameboard):
        for index in range(1, 10):
            if gameboard[index] == ' ':
                return False
        return True

    def is_game_won(self, gameboard):
        win_conds = ((1,2,3), (4,5,6), (7,8,9), (1,4,7), (2,5,8), (3,6,9), (1,5,9), (3,5,7))
        for win_cond in win_conds:
            if gameboard[win_cond[0]] == gameboard[win_cond[1]] and gameboard[win_cond[1]] == gameboard[win_cond[2]] and gameboard[win_cond[0]]!= ' ':
                return True

    def printBoard(self,gameboard):
        index = 0
        for row in range(1,4):
            for column in range(1,4):
                index += 1
                if column != 3:
                    print(gameboard[index], end='')
                    print('|', end='')
                else:
                    print(gameboard[index])

class Game():

    def on_start(self):
        '''
        Called on initial start and in subsequent restarts
        '''
        self.controlBoard = Gameboard()
        self.gameboard = self.controlBoard.gameBoard
        self.playerOne ='o'
        self.playerTwo = 'x'
        print('Welcome to tic-tac-toe')
        print("What is player one's name?")
        self.player_one = input(' : ')
        print("What is player two's name?")
        self.player_two = input(' : ')
        print('Here is your game board, each place is represented by 1-9, starting from left column each time and movng along the row')
        self.controlBoard.printBoard(self.gameboard)
        self.turn = 1

    def on_end(self):
        #check if a player wants to end the game
        if self.running == False:
            replay = input('Press 0 to quit or 1 to play again: ')
            try:
                if int(replay):
                    self.running = True
                    self.on_start()
            except:
                print("A number must be entered.")
                self.on_end()

    def takeTurn(self, user, peice):
        print(user + ' choose a place, 1-9')
        try:
            position = int(input(': '))
            if position > 9 or position < 1:
                raise Exception

        except:
            print('Pick a number between 1-9')
            return self.takeTurn(user, peice)

        if self.controlBoard.is_place_taken(self.gameboard, position):
            print("That place is taken")
            self.takeTurn(user,peice)
        else:
            self.controlBoard.setPeice(peice, position, self.gameboard)
            self.controlBoard.printBoard(self.gameboard)
            if self.controlBoard.is_game_won(self.gameboard):
                print(user +  " wins.")
                self.running = False

    def main(self,):
        self.running = True
        self.on_start()
        while self.running:
            if self.turn%2 != 0:
                self.takeTurn(self.player_one, 'o')
            else:
                self.takeTurn(self.player_two, 'x')

            if self.controlBoard.is_board_full(self.gameboard):
                print("Its a draw!! You both lose!")
                self.running = False
            self.turn += 1

            if not self.running:
                self.on_end()

if __name__ == '__main__':
    Game().main()

Strive for self-documenting code
Python's any and all are extraordinarily useful, but few people know about them, for example:
def is_board_full(self, gameboard):
    for index in range(1, 10):
        if gameboard[index] == ' ':
            return False
    return True

Is understandable but not obvious, using all it becomes:
def is_board_full(self, gameboard):
    return all(gameboard[index] != ' ' for index in range(1, 10))

Kind of better, now let's use native iteration:
def is_board_full(self, gameboard):
    return all(square != ' ' for square in gameboard.values())

Now we can define a constant:
EMPTY = ' '

...

def is_board_full(self, gameboard):
    return all(square != EMPTY for square in gameboard.values())

That reads as "the board is full if all the squares are not empty", you may prefer "the board is full if not at least one one square is empty"), expressed as:
def is_board_full(self, gameboard):
    return not any(square == EMPTY for square in gameboard.values())

The most self-documenting of all is probably using a helper:
def is_there_at_least_one_empty(self, gameboard):
    return any(square == EMPTY for square in gameboard.values())

def is_board_full(self, gameboard):
    return not self.is_there_at_least_one_empty(gameboard)

The code now looks like a high level description of the problem, not a specific implementation.
@staticmethod
You pass self into your methods, but most do not use it. A method that does not use self is called a static method and the convention is not to pass self to those methods by applying the @staticmethod decorator:
@staticmethod
def is_there_at_least_one_empty(gameboard):
    return any(square == EMPTY for square in gameboard.values())

@staticmethod
def is_board_full(gameboard):
    return not GameBoard.is_there_at_least_one_empty(gameboard)

Given that all the methods in Gameboard are static you may consider using top-level functions, and achieve separation of concerns by putting those in a separate file. Using a class adds complexity and in this case you do not seem to need nor want one (using only static methods defies OOP completely)
Simplify
Do not start by writing a lot of loops and ifs, you start with a description of the problem:

"To print a board, I print each one of its line"
"To print a line, I print the squares joined by |"
"To divide a board into lines, I divide it in chunks of 3"

If you are wondering, dividing a list in chunks of x is present on StackOverflow: https://stackoverflow.com/questions/10364391/how-to-split-python-list-into-chunks-of-equal-size
def split_into_lines(board):
    return zip(*[iter(board.values())]*3)

def _print_line(line):
    print(' | '.join(line))

def print_gameboard(board):
    for line in split_into_lines(board):
        _print_line(line)

This is much clearer and modular then your original version.


Finding the biggest number in a list of numbers This is the first time I tried to solve a problem and wrote code for it, rather than learning some code and trying out to see what I might be able to do with it. As such, it's not much, but I'd like any tips and help about how I wrote this. Basically, I tried to write a program to find the biggest number in a list of numbers.

def main():
    print("Enter a series of numbers separated only by spaces:", end='\t')
    n = input().split()
    n = [int(i) for i in n]
    while len(n) != 1:
        if n[0] > n[1]:
            n.remove(n[1])
        else:
            n.remove(n[0])
    print("the biggest number is: ", n[0])
main()

Feedback? Did I do anything kinda noobish?

The most obvious thing I can see is that you wrote a loop when one was not needed.

largest = max(n)

Also, check your boundary conditions. What happens if no numbers are entered?


"Open and closed doors" riddle Riddle Description:

  \$N\$ doors are closed. In the first pass, I open all of them. In the
  second pass, I toggle every second door. In the third pass, I toggle
  every third door. I continue this until I have completed the \$N\$th
  pass. Find all the doors that will remain open after \$N\$ passes.

public static void printDoorsOpened(int passes)
{
        HashMap<Integer, Boolean> doors=new HashMap<Integer, Boolean>();
        int i = 0;
        //Close all doors as default in the start
        while(i < passes)
        {
            doors.put(i, false);
            i++;
        }

        for(int j = 1; j <= passes; j++)
        {
            for(int k = j - 1; k < passes; k += j)
                doors.put(k, !(doors.get(k)));
        }

        for(int l = 0; l < passes; l++)
        {
            System.out.println("The door number "+(l + 1)+" is "+(doors.get(l) == true ? "Opened" : "Closed"));
        }
}

Please feel free to review it based on best practices, style and any other efficient solution, but please refrain from solely style-based reviews.

This looks like something that can get optimized to a mathematical function.

Let's run it pass by pass and try to interpret the result.

  N = 12

  Doors #1 through #12

Pass 1:  1 1 1  1 1 1  1 1 1  1 1 1 //All opened
Pass 2:  1 0 1  0 1 0  1 0 1  0 1 0 //2, 4, 6, 8, 10, 12 closed
Pass 3:  1 0 0  0 1 1  1 0 0  0 1 1 //3 and 9 closed, 6 and 12 opened
Pass 4:  1 0 0  1 1 1  1 1 0  0 1 0 //4 and 8 opened, 12 closed
Pass 5:  1 0 0  1 0 1  1 1 0  1 1 0 //5 closed, 10 opened
Pass 6:  1 0 0  1 0 0  1 1 0  1 1 1 //6 closed, 12 opened
Pass 7:  1 0 0  1 0 0  0 1 0  1 1 1 //7 closed
Pass 8:  1 0 0  1 0 0  0 0 0  1 1 1 //8 closed
Pass 9:  1 0 0  1 0 0  0 0 1  1 1 1 //9 opened
Pass 10: 1 0 0  1 0 0  0 0 1  0 1 1 //10 closed
Pass 11: 1 0 0  1 0 0  0 0 1  0 0 1 //11 closed
Pass 12: 1 0 0  1 0 0  0 0 1  0 0 0 //12 closed

It's a factor detector!

It detects whether for each positive integer \$N\$ or lower whether its divisible an even or uneven amount of times by its possible factors.

That is, 12 can be factorized as \$1\times 12\$, \$2\times 6\$, \$3\times 4\$, \$4\times 3\$, \$6\times 2\$, \$12\times 1\$. That's even, so the door ends up at 0, which is closed. 9 can be factorized as \$1\times 9\$, \$3\times 3\$ and \$9\times 1\$. That's uneven, so the door ends up at 1, which is opened.

Now, for any factorization \$x\times y\$ of a number, there exists an inverse factorization: \$y\times x\$. Unless \$x\$ equals \$y\$. If that's the case, then there is no inverse factorization because the inverse is the same!

So it's a square root detector! (Any number that has a positive integer as a square root has an uneven number of factorizations, the rest have an even number).

Now, this doesn't implement the algorithm that your problem statement describes, but it does end up with the same result:

public static void printDoorsOpened(final int passes)
{
    int j = 1;
    for(int i = 1; i <= passes; i++)
    {
        if(j*j == i){
            j++;
            System.out.println("The door number "+i+" is Opened");
        } else {
            System.out.println("The door number "+i+" is Closed");
        }
    }
}


Calculating doubling times from data points In the code below, noisy data points with unique errors are created. From this, an exponential function is fitted to the data points, and then doubling times (10 unit windows) are calculated.

I'm uncertain how to show the unique errors in the data points in the fitted function or doubling times.

Output:

from scipy import optimize
from matplotlib import pylab as plt
import numpy as np
import pdb
from numpy import log

def exp_growth(t, x0, r):
    return x0 * ((1 + r) ** t)

def doubling_time(m, x_pts, y_pts):
    window = 10

    x1 = x_pts[m]
    y1 = y_pts[m]
    x2 = x_pts[m+window]
    y2 = y_pts[m+window]

    return (x2 - x1) * log(2) / log(y2 / y1)

# First, artificially create data points to work with
data_points = 42

# Create the x-axis
x_pts = range(0, data_points)

# Create noisy points with: y = x^2 + noise, with unique possible errors
y_pts = []
y_err = []
for i in range(data_points):
    random_scale = np.random.random()
    y_pts.append((i * i) + data_points * random_scale)
    y_err.append(random_scale * 100 + 100)

x_pts = np.array(x_pts)
y_pts = np.array(y_pts)
y_err = np.array(y_err)

# Fit to function
[x0, r], pcov  = optimize.curve_fit(exp_growth, x_pts, y_pts, p0=(0.001, 1.0))
fitted_data = exp_growth(x_pts, x0, r)

# Find doubling times
x_t2 = range(32)
t2 = []
t2_fit = []
for i in range(32):
    t2.append(doubling_time(i, x_pts, y_pts))
    t2_fit.append(doubling_time(i, x_pts, fitted_data))

# Plot
fig, (ax1, ax2, ax3) = plt.subplots(3, 1, sharex=True)

ax1.plot(x_pts, y_pts, 'bo')
ax1.errorbar(x_pts, y_pts, yerr=y_err)
ax1.set_ylim([0, 2000])
ax1.set_title('Artificially created raw data points with unique errors', fontsize=8)

ax2.plot(fitted_data, 'g-')
ax2.set_ylim([0, 2000])
ax2.set_title('Fitted exponential function', fontsize=8)

ax3.plot(x_t2, t2, 'ro', label='From points')
ax3.plot(x_t2, t2_fit, 'bo', label='From fitted')
ax3.set_title('Doubling time at each point (10 unit window)', fontsize=8)
ax3.legend(fontsize='8')

plt.show()

Especially with mathematical-like programs, you want to write docstrings with your functions and classes.

In case you don't know what a docstring is, a docstring is a short piece of documentation that describes the parameters and return value of a function. And, it can include any sort of information/summary that is needed to fully explain a function.

Here is an example:

def add(a, b):
    """
    Adds two numbers together and returns the sum
    a + b
    @param(number) -- one number to add
    @param(number) -- another number to add
    @return(number) -- the sum of both parameters

    return a + b

In the case of your code where you have a few mathematical functions, providing a docstring is greatly important because there you can write out the formula that the function is using, what each part of the formula means, and what the return of the formula does for the rest of your code.

And, in places that aren't functions but need a docstring, you can just a # comments on the side (you don't have enough of these).

All of your main code (aside from functions) should be put into:

if __name__ == "__main__":
    [code or main()]

See here for why.

Your code seems very procedural. You have a few functions here and there but other than that, everything is just one after the other.

I'm having difficulty following your code so I can't give too many specific recommendations, but you should move more code to separate functions. For example, however, you could try to make a function call create_graph that will create a graph for you in just a single call.

That way, when you put your code into if __name__ == "__main__", every single line of code that you wrote won't be in there.


K&R 1-21 write an "entab" program From K&R exercise 1-21,

  Write a program entab that replaces strings of blanks by the minimum
  number of tabs and blanks to achieve the same spacing. Use the same
  tab stops as for detab. When either a tab or a single blank would
  suffice to reach a tab stop, which should be given preference?

My solution:

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

#define MAXLEN  1000    // max line length
#define TABSTOP 8       // tabstop width

int get_line(char line[], int maxlen);
char * entab(char line[], int len);

int main(void)
{
    char line[MAXLEN];  // input line container
    char *entabbed;     // entabbed version of input line
    int len = 0;        // input line length
    int e_len = 0;      // entabbed line length

    while((len = get_line(line, MAXLEN)) > 0) {
        // produce entabbed version
        entabbed = entab(line, len);
        while (entabbed[e_len] != '\0') {
            e_len++;
        }

        // print entabbed version
        printf("%s", entabbed);
        if (entabbed[e_len-1] != '\n') {
            printf("\n");
        }

        free(entabbed);
    }

    return 0;
}

int get_line(char line[], int maxlen)
{
    int i = 0;  // current index
    int c;      // current character

    while (i < maxlen-1 && (c = getchar()) != EOF) {
        line[i++] = c;
        if (c == '\n') {
            break;
        }
    }
    line[i] = '\0';

    // clear input buffer of overflow
    if (i > 0 && line[i-1] != '\n') {
        while ((c = getchar()) != '\n' && c != EOF);
    }

    return i;
}

char * entab(char line[], int len)
{
    int i;          // index into original line
    int e;          // index into entabbed version
    int spaces;     // buffered whitespace counter
    char c;         // current char
    char *entabbed; // entabbed version

    e        = 0;
    spaces   = 0;
    entabbed = (char *) malloc(len * sizeof(char) + 1);

    for (i = 0; i < len; i++) {
        c = line[i];

        if (c == ' ') {
            if ((i+1) % TABSTOP == 0) {
                entabbed[e++] = '\t';
                spaces = 0;
            } else {
                spaces++;
            }
        } else {
            while (spaces > 0) {
                entabbed[e++] = ' ';
                spaces--;
            }

            entabbed[e++] = c;
        }
    }

    return entabbed;
}

I know there are probably more sophisticated ways of handling the input, but I've restricted myself to material covered by the first chapter of K&R.

I see some things that could help you improve your program.

Learn a recent version of C

I still have my K&R first edition from 1978, however it would be a very poor choice to use for learning C today.  The version of C it teaches has been obsolete for decades.  I don't know which edition you're using (looks like a more recent one by the style of code) but it would be best to learn C99 or C11.  You may already be doing this, but it would be good to check.

Declare variables closer to where they're used

It may be a result of the book you're reading to learn C (per the previous point) but in modern C, we don't declare all of the variables at the top of the function that will be used in the entire function.  Instead, declare the variables closer to where they're first used.

Reorder functions to avoid prototypes

You can eliminate the function prototypes by rearranging the function definitions so that each function is defined before its first appearance. In this case, that simply means moving main last, below the other functions.

Use const where practical

The current version of entab doesn't alter the passed line.  This can be clearly indicated by making that parameter const:

char * entab(const char line[], int len)

Avoid frequent memory allocations

Memory allocation operations can be expensive in terms of time and are also yet another thing that can go wrong in terms of running out of memory or forgetting to free the block.  With that in mind, there are two obvious choices. One would be to pass in an output buffer and length to entab() and the other is to do the entabbing in place.  I'd advocate the latter approach since we know that an entabbed line will never be larger than the input string.

Fix bug #1

The first time through the loop, e_len is 0 and gets incremented to point to the terminating '\0', but the value is never reset to zero for the second and subsequent lines.  That's a bug.  Instead, I'd recommend changing from this

while (entabbed[e_len] != '\0') {
    e_len++;
}

To this:

for (e_len = 0; entabbed[e_len] != '\0'; ++e_len)
{}

Fix bug #2

Within the entab function, the terminating '\0' is never copied to the resulting copy of the line.  The result is that the line may or may not be properly terminated.  This is easily fixed by changing the for loop there from this:

for (i = 0; i < len; i++) {

to this:

for (i = 0; i <= len; i++) {

Eliminate return 0 at the end of main

Since C99, the compiler automatically generates the code corresponding to return 0 at the end of main so there is no need to explicitly write it.


Convert JSON from a webservice to text file I get list JSON text from a webservice and list_of_columns is predefined, I need the output in that format. I am not sure about few things which are as follows

I don't need "\n" towards the very end and this code seems sub-optimal to me but right now I can't think of any pythonic way to do it.
My order for the code should be defined by list_of_columns. I don't know if this I have done is the optimal way to do it.

All in all my code does the job but I want to improve more. I suspect I could have used something that's already present that would have been much easier from what I have done. I am here for some criticism and learning :)

NOTE: I have used csv module to do the same but since separator had to be 2 characters long I had to drop that idea.

For formatting I tried using strftime but since dates in the data were less than 1900, I had to drop that idea.
I think from what I have done with the date something better could have been done.

def read_json_text_write_text_file(json_text,file_name,list_of_columns,separator = "|~"):
    list_of_dictionaries = json.loads(json_text)

    with open(file_name, "w") as text_file:
        text_file.seek(0)
        for dictionary in list_of_dictionaries:
            for column in list_of_columns:
                if column in dictionary.keys():
                    if "DATE" in column:
                        text_file.write(convert_utc_to_est(dictionary[column]))
                    else:
                        text_file.write(str(dictionary[column]))
                if column != list_of_columns[-1]:
                    text_file.write(separator)
            if dictionary != list_of_dictionaries[-1]:
                text_file.write("\n")

def convert_utc_to_est(date_string):

    from_zone = dateutil.tz.gettz('UTC')
    to_zone = dateutil.tz.gettz('EST5EDT')

    utc = parser.parse(date_string)
    utc = utc.replace(tzinfo=from_zone)
    central = utc.astimezone(to_zone)
    # tzinfo is set to None else it would be present during the return
    central = central.replace(tzinfo=None)
    # This split is performed to remove the micro seconds part.
    return str(central).split(".")[0]

I am here for some criticism and learning :)

That's really a great attitude. Keep it up and you'll get far.

Handling the last item differently

You alluded to the issue of doing social treatment for the last item in a loop. There are two related relevant places in the posted code:

    for dictionary in list_of_dictionaries:
        for column in list_of_columns:
            # ...
            if column != list_of_columns[-1]:
                # special treatment 1
        if dictionary != list_of_dictionaries[-1]:
            # special treatment 2

Having a conditional statement that is executed for every item in a list, and you know it is false for all items except the last, is not pretty. It could be acceptable, if the condition is super fast, for example a simple comparison of two numbers. Here you are comparing strings and objects, which may not be super fast.

You could convert this to a simple numeric comparison by using a range loop, and then comparing indexes instead of objects.

You could also change the loops to iterate until the element before the end, and then do the special treatment for the last, outside the loop. That way you can eliminate the conditional statements.

Seeking in a file

When opening a file for writing with the w option only, there's no need to seek(0) in it, that will happen automatically, and the file will be rewritten.

Extracting the beginning of a string until some character

It's not efficient to use split for this. It will allocate a list to store the results, and scan until the end of the string, even though you only need the first part. It would be better to find the index of the dot, and then get a substring.


Updating an offset in a buffer after each read I'm having a bit of a debate at work regarding when to replace numeric literals with variable names.

I have a function that reads data from a buffer and after each read, updates the offset so that next time, we read starting at the right offset.

function readMessage(data: Buffer, offset: number) {
    let firstLen = data.readInt32LE(offset);
    offset += 4;

    let firstPieceOfMessage = data.slice(offset, offset + firstLen).toString();
    offset += firstLen;

    let secondLen = data.readInt32LE(offset);
    offset += 4;

    let secondPieceOfMessage = data.slice(offset, offset + secondLen).toString();
    offset += secondLen;

    let finalMessage = firstPieceOfMessage + secondPieceOfMessage;
    return [finalMessage, offset];
}

The calling code calls this function in a loop:

let offset = 0;
while (offset < data.length) {
    let message;
    ([message, offset] = readMessage(data, offset));
    console.log(message);
}

A coworker made a comment that I should replace the 4 with a constant defined elsewhere.

My view is that since 4 is intimately-bound to the function readInt32LE, and the offset updating happens immediately after reading the 32 bits, it's more readable to keep the 4 and 32 next to each other.

Replacing 4 with a variable like numberOfBytesPerMessagePiece would jump out at me, because then I'll wonder "Wait, this would only work if that variable is set to 4, because that aligns with the function that I'm using above it. Is it actually set to that?"

If Node had a more general function readIntLE(offset, 4), then I would gladly replace 4 with a constant, but since there isn't, and we must use a function whose name contains a literal, it feels better to keep the literal 4 next to it.

Second, changing the constant to something else would necessarily break the code, since one would have to link that change with replacing readInt32LE with something more apt.

This is complicated by the fact that we use 4 in other places in this file (but not in other files), but almost all places (all but one) use it next to a similar function:

let buff = Buffer.alloc(4 + headerLen);
buff.writeInt32LE(someLength, 0);
buff.write(header, 4);

I'm thinking now that I could call this constant bytesIn32Bits = 4;. Is this worth the trouble?

Update:

This is one option I'm exploring right now:

function readLength(data: Buffer, offset: number): [number, number] {
    const len: number = data.readInt32LE(offset);
    offset += 4;

    return [len, offset];
}

function readData(data: Buffer, offset: number, length: number): [Buffer, number] {
    let msg: Buffer = data.slice(offset, offset + length);
    offset += length;

    return [msg, offset];
}

This will at least allow me to get rid of the offset updates in the first example.

function readMessage(data: Buffer, offset: number) {
    let firstLen: number;
    ([firstLen, offset] = readLength(data, offset));

    let firstPieceOfMessage: string;
    ([firstPieceOfMessage, offset] = readData(data, offset, firstLen));

    let secondLen: number;
    ([secondLen, offset] = readLength(data, offset));

    let secondPieceOfMessage : string;
    ([secondPieceOfMessage, offset] = readData(data, offset, secondLen));

    let finalMessage = firstPieceOfMessage + secondPieceOfMessage;
    return [finalMessage, offset];
}

I'm not convinced that that looks any nicer though.

I tend to agree with your colleague - magic numbers are nearly always bad. In this case, it is clear why you are adding 4, but only once I've done 32/8 in my head.

Extracting this constant into a bytesIn32Bits would certainly be helpful for me and other developers who don't work on the byte level very often.

However, I don't believe this is the ideal solution. Though the code itself isn't really that repetitive, the logic is.

Read data
Advance offset by length of data

Unfortunately Node does not have a built in Buffer alternative which can keep track of an offset for you. You could do this with functions as shown in the edit, but it would be better to handle at the class level as you can then completely forget about the offset in most cases.

Ideally, the function should read something like this:

function readMessage(data: OffsetTrackingBuffer) {
    let firstLen = data.readInt32LE();
    let firstPieceOfMessage = data.readString(firstLen);
    let secondLen = data.readInt32LE();
    let secondPieceOfMessage = readString(secondLen);

    let finalMessage = firstPieceOfMessage + secondPieceOfMessage;
    return finalMessage;
}

The outer loop would also be slightly changed:

let data = new OffsetTrackingBuffer(rawData, 0);
while (data.offset < data.length) {
    let message = readMessage(data);
    console.log(message);
}

I'm not normally one to recommend adding libraries to your project, but in this case, all this logic has already been implemented in the smart-buffer npm package. According to the readme it also comes with TypeScript typings built in. I haven't used it, but it seems to be rather popular.


WCF data service class I have this WCF data service class and I believe it is doing a lot:

[System.ServiceModel.ServiceBehavior(IncludeExceptionDetailInFaults = true)]
public class PetSuppliesRelationalData : EntityFrameworkDataService<PetSuppliesEntities>
{
    private string _TenantID
    {
        get
        {
            return HttpContext.Current.User.Identity.Name;
        }
    }

    public static void InitializeService(DataServiceConfiguration config)
    {
        config.DataServiceBehavior.MaxProtocolVersion = DataServiceProtocolVersion.V3;
        config.UseVerboseErrors = true;

        config.SetEntitySetAccessRule("*", EntitySetRights.AllRead);

        config.SetEntitySetAccessRule("Categories", EntitySetRights.All);
        config.SetEntitySetAccessRule("Products", EntitySetRights.All);

        config.SetEntitySetAccessRule("RealErrors", EntitySetRights.None);
        config.SetEntitySetAccessRule("OtherDiagrams", EntitySetRights.None);
        config.SetEntitySetAccessRule("Favourites", EntitySetRights.None);
        config.SetEntitySetAccessRule("Auditing", EntitySetRights.None);

        config.SetEntitySetAccessRule("Compaines", EntitySetRights.None);

        config.SetServiceOperationAccessRule("*", ServiceOperationRights.All);
    }

    protected override void OnStartProcessingRequest(ProcessRequestArgs args)
    {
        base.OnStartProcessingRequest(args);
        ThrowIfNotAuthenticated();
    }

    protected override void HandleException(HandleExceptionArgs args)
    {
        LoggingFramework.LogError("Exception ", args.Exception);
        base.HandleException(args);
    }

    private static void ThrowIfNotAuthenticated()
    {
        var identity = HttpContext.Current.User.Identity;
        if (identity == null || !identity.IsAuthenticated)
        {
            throw new Exception("Not Authenticated");
        }
    }

    #region Query Intercepters

    [QueryInterceptor("Table1")]
    public Expression<Func<Table1, Boolean>> OnQueryTable1()
    {
        return x => x.TenantID == _TenantID;
    }

    // one for each table in db

    #endregion

    #region Change Interceptors

    [ChangeInterceptor("Table1")]
    public void OnChangeTable1(Table1 data1, UpdateOperations operations)
    {
        // check if user is authorized etc..
        switch (operations)
        {
            case UpdateOperations.Add:
                break;
            case UpdateOperations.Change:
                break;
            case UpdateOperations.Delete:
                break;
            case UpdateOperations.None:
                break;
        }
    }

    // one for each table in db

    #endregion
}

I'm thinking moving out Query and Change interceptors out of this class but not sure if this will be a good move. I also don't know if it is possible, but the code is working as it is, so any improvement would help.

From the outset the code looks ok. I would leave the Change and Query methods inside the class.

If the class does get abit too much then partial the class into sensible files. Also, as a side note, look up SOLID it will help you to break down and structure your code if appropriate.


Calculating SQL query processing All I want do to is simply get the result of how long (in seconds) the PHP takes to process the SQL query. Am I on a right track? Is my code correct to get the result I want?

<?php
$con = mysql_connect("localhost","root","");
if (!$con)
  {
  die('Could not connect: ' . mysql_error());
  }

mysql_select_db("test3", $con);

    $start = microtime(TRUE);
    $result = mysql_query("SELECT * FROM `test`
    ORDER BY `test`.`test2` DESC
    LIMIT 0,100");

echo "

ID
test1
test2
test3
test4

";

while($row = mysql_fetch_array($result))
  {
  echo "
";
  echo "" . $row['ID'] . "	";
  echo "" . $row['test1'] . "	";
  echo "" . $row['test2'] . "	";
  echo "" . $row['test3'] . "	";
  echo "" . $row['test4'] . "	";
  echo "
";
  }
echo "
";

$end = microtime(TRUE);

$sqlTime = $end - $start;

mysql_close($con);
echo $sqlTime
?>

Yes, microtime is designed to be used for this. The PHP documentation mentions this use case for microtime().
Consider putting this into a loop and print out the average result: if the measured time is too small, you can't provide a reliable estimation.
Concerning the MySQL request: you should switch to mysqli or PDO.


Improve AJAX response handling function This code handles a response from an RSS feed. It organizes and appends to content. If there's a video embeded then separate it from the rest of the content. I would like a review mainly on performance/efficiency but I'm open to any other suggestions as well. That star selector is really nagging me but I don't know of a better way to iterate over all the contained elements.

function getFeed(url, element, callback) {
    $.getJSON("https://ajax.googleapis.com/ajax/services/feed/load?v=1.0&callback=?&q="+encodeURIComponent(url), function(response) {
        var content = "",
            $element = $(element);

        for (var i = 0; i < response.responseData.feed.entries.length; i++) {
            content = content + response.responseData.feed.entries[i].content; //Join all the feed entries
        }

        $element.find(".content").html(content).addClass($element.find("embed").length? "withVideo" : "");

        $element.find("*").each(function() {
            var $this = $(this);

            $this.removeAttr("style width align"); //Reset all the crap that comes with the response

            if ($this.is("embed")) {
                $element.append("

");
                $this.attr("width", 640).attr("height", 360).parent().appendTo(element + " .video");
            };
        });

        if (typeof callback === 'function') {
            callback();
        };
    });
}

This is then called like so:

getFeed("http://www.kent.k12.wa.us/site/RSS.aspx?PageID=3854", "#TechExpo", optionalCallback);

Here's what a response might look like

Some text blah blah blah.

Some more text

Video Title

<embed src="http://..." width="360" height="202" type="application/x-shockwave-flash"></embed>
Watch the 7th annual Tech Expo highlights.

Late reply,

this code is good, I can only find 2 faults:

Your code should have error handling for the JSON call, things can go wrong
Too much horizontal stretching, I would introduce some sugar for this:

for (var i = 0; i < response.responseData.feed.entries.length; i++) {
    content = content + response.responseData.feed.entries[i].content; //Join all the feed entries
}

could be

var entries = response.responseData.feed.entries;
for (var i = 0; i < entries.length; i++) {
    content = content + entries[i].content; //Join all the feed entries
}

Also, I find

$element.find(".content").html(content)
                         .addClass($element.find("embed").length? "withVideo" : "");

more readable.

As for find("*"), it seems to be cleanest way to access all children.


Creating a lightweight strongly-typed parameter infrastructure I'm looking for some feedback on some code, designed to allow me to strongly-type parameters, which get passed to functions.

I'm only really looking for comments on making my parameters more strongly typed, as well as the existence of the factory method. The implementation of the FindUser method is not relevant.

At the minute the code base has a number of places where many pieces of data are represented as strings, however in actual fact they may be EmailAddresses, Usernames etc.

This can be awkward, so I am trying to consider a way in which this could be improved.

I have written the below code, as an attempt to address this problem, and allow for the parameters I pass around functions, to be more strongly typed.

Interface used to represent the values

Public Interface IStronglyTypedValue(Of T)
    Property Value As T
    // We could add IsValid, IsCorrectFormat, etc methods in future
End Interface

Base class to provide functionality around construction (and maybe properties in future such as IsValid, or ToString()).

Public MustInherit Class StronglyTypedValueBase(Of T, TTypeOfThing As {New, IStronglyTypedValue(Of T)})
    Implements IStronglyTypedValue(Of T)

    Public Property Value As T Implements IStronglyTypedValue(Of T).Value

    Public Shared Function Make(data As T) As TTypeOfThing
        Return New TTypeOfThing() With {.Value = data}
    End Function
End Class

This will then allow for the definition possible parameters to methods, for example:

Public Class EmailAddress
    Inherits StronglyTypedValueBase(Of String, EmailAddress)
End Class

Public Class UserName
    Inherits StronglyTypedValueBase(Of String, UserName)
End Class

Which could then be used like this

 EmailAddress.Make("my@email.com")
 UserName.Make("user123")

Some example ways BEFORE using this functionality (functions grouped into one class for brevity)

Public Class UserFinder
    Public Function FindUserByEmailAddress(email As String) As IUser
        //Lookup the user
    End Function

    Public Function FindUserByUsername(username As String) As IUser
         //Lookup the user
    End Function

    Public Sub DoWork()
        Dim paul = FindUserByEmailAddress("paul@email.com")
        Dim steve = FindUserByUsername("steve1")
    End Sub
End Class

Instead, we can now do:

Public Class UserFinder
    Public Function FindUser(email As EmailAddress) As IUser
        //Lookup the user
    End Function

    Public Function FindUser(username As UserName) As IUser
         //Lookup the user
    End Function

    Public Sub DoWork()
        Dim paul = FindUser(EmailAddress.Make("paul@email.com"))
        Dim steve = FindUser(Username.Make("steve1"))
    End Sub
End Class

This also means we can now more easily write ExtensionMethods, and refactor some of this functionality into the class.

The main area of concern/review is the Make() method on the StronglyTypeValueBase class.. Is this a dirty approach? Would it be better to have used a Public Constructor than a factory method?

Also, is there a better way of solving the problem of strongly typed parameters to methods, which may not need to have loads of function.

In short, I'm looking to add a compile-time enforcement of type. I want to  not allow using a string representing a Username in a function expecting an email address; whilst at the same time not introducing any anti-patterns.

Stop. This way madness lies.

It's great that you want to strongly type these things. They should be! The problem is in your approach. This isn't what generics/interfaces are for. This is what Structures and conversion operators are for.

Public Structure EmailAddress
    Private email As String

    Public Sub New(ByVal s As String)
        ' validation code here

        email = s
    End Sub

    Public Shared Narrowing Operator CType(ByVal s As String) As EmailAddress
        Return New EmailAddress(s)
    End Operator

    Public OverRides Function ToString() As String
        Return email
    End Function
End Structure

Which lets you use it as a strongly typed object, without much fuss and using VB.Net idioms that any VB dev will be familiar with.

Dim email = new EmailAddress("JohnDoe@domain.net")

email = CType("JohnDoe@domain.net", EmailAddress)

Dim value As String = email.ToString()


Improvement of delete method I was wondering if there is a better way to code this:

using (SqlConnection connection = new SqlConnection(ConnectionString)) {
   using (SqlCommand cmdDelete = new SqlCommand("DELETE FROM table WHERE
         (TestNumber = '" + testNumber + "')", connection)) {
      try
      {
         connection.Open();
         int recordExists = deletePolicyFromTdm.ExecuteNonQuery();
         connection.Close();

         if(recordExists == 1)
            result.Append("Record has been deleted");
         else
           result.Append("Record does not exist");
      ...

Couple of assumptions:

testNumber is unique and no duplicate records and was input tested, at least according to bus rules. So I realize that if more than one record is present the if statement won't work properly
Table has a PK but testNumber isn't it
Only one roundtrip to db
Can't change how DB is set up
SQL command will eventually be replaced with a sproc and parameter

The idea is that if user clicks a button and gets either a message that no record exists or that the records were deleted.

A few quick remarks:

In your second using statement, you declare a SqlCommand variable named cmdDelete. In the body of that using statement, however, you call ExecuteNonQuery on a variable named deletePolicyFromTdm. I don't see a declaration for deletePolicyFromTdm in your code, so I wonder if it should be cmdDelete instead.
I'm not sure where testNumber is coming from, but your approach potentially opens the door for SQL injection. I recommend defining it as a parameterized query (e.g. DELETE FROM table WHERE TestNumber = @TestNumber); it's a very small amount of work and will be one less thing to do when you switch to a sproc later.
Your brace style is not consistent. The braces following using are on the same line; the brace following try are on a separate line.