calvinmetcalf / topojson.py

Topojson ported to python
Other
92 stars 37 forks source link

lines equal #5

Closed calvinmetcalf closed 11 years ago

calvinmetcalf commented 11 years ago

these two functions end up not being equivalent as when the latter is used something t that is not iterable gets through p and not lines_equal(p,t)

def lines_equal(a, b):
    if not (type(a) == type(b) == type([])):
        return True
    n = len(a)
    i = 0
    if len(b) != n:
        return False
    while i < n:
        if a[i] != b[i]:
            return False
        i += 1
    return True

and

def lines_equal(a, b):
    for arg in (a, b):
        if not isinstance(arg, list):
            return False
    return a == b
calvinmetcalf commented 11 years ago

er ping @scardine

calvinmetcalf commented 11 years ago

ah but

def lines_equal(a, b):
    for arg in (a, b):
        if not isinstance(arg, list):
            return True
    return a == b

is equivalent, leading to me changing the name of this one to mysterious_line_test

scardine commented 11 years ago

Weird, if lines are not lists it returns True without comparing the arguments. Mysterious indeed, given the original name of the function, I would not discard a bug in the original code that never triggered because the function was never fed with anything but arrays.

calvinmetcalf commented 11 years ago

yeah it looks like it wasn't just checking if they were equal but returning false in that case to avoid a latter check

scardine commented 11 years ago

Mike makes such complex visualizations perform well in the browser, his style sacrifices legibility for performance. For example, instead of Math.pow(x + y, 2) he writes (_ = x + y) * _, it took me awhile to understand, but looks like it gives a 5% speed boost in chrome - if this code is in a hot loop, this can be a huge difference.

He does this little obfuscating optimizations naturally, but this makes his code sub-optimal as a format specification despite being blazing fast.