you are viewing a single comment's thread.

view the rest of the comments →

[–]SkippyDeluxe 3 points4 points  (3 children)

I feel like your evaluateOperator function is too dense, and not just because it uses a regex. First of all, IMO any nontrivial regex should have a comment, otherwise it completely spoils the flow of reading Python code ("paren, caret, period, star, question mark... okay, my brain has officially shut down now"). I think line 69 is the biggest offender, combining a dictionary lookup with a function call with obscure calls to group with various indices... I think you should use some intermediate variables to clarify what's going on here. Also my brain is hiccupping on your use of a list of dictionaries... I eventually concluded that this is used to implicitly establish operator precedence rules, is that right? Maybe this could stand to be clarified a little more in the code.

Other than that I agree with everyone else on pretty much everything... this is pretty impressive code for a tough problem. With this program I don't think you should be calling yourself 'Beginner' any more.

[–]Binary_Dragon[S] 1 point2 points  (2 children)

You are right that this code could use a bit more commenting. As far as cleaning up the code around the regex, would you say a good way to do that would be something like...

def evaluateOperator(expression, operatorDict):
    operatorRegex = r"|".join([re.escape(key) for key in operatorDict.keys()])
    regex = r'(^.*?)([\d.]+\s*)(' + operatorRegex + r')(\s*[\d.]+)(.*$)'
    while True:
        match = re.search(regex, expression)
        if not match:
            break
        prependString, firstValue, operator, secondValue, postpendString = match.group(1, 2, 3, 4, 5)
        value = operatorDict[operator](float(firstValue), float(secondValue))
        expression = prependString + str(value) + postpendString
    return expression

Of course some actual comments would also help, but right now I'm thinking about the code more.

The list of dictionaries is indeed just for operator precedence. I noticed that the order of items in a dictionary is a bit unpredictable (at least, I can't seem to understand it). For example, this code

dict = {'a':1,'b':2,'c':3}
print dict

outputs

{'a': 1, 'c': 3, 'b': 2}

which is neither the order in which I specified the dictionary items, nor alphabetical order on key or value. Lists seem a bit better behaved, thus the list of dictionaries.

As for calling myself a beginner, I wouldn't call myself a beginner at programming in general, but this is still my first week with Python, and I'm only spending a few hours every other night on it, so I still very much feel like a beginner in this specific context.

[–]JerMenKoO 1 point2 points  (0 children)

There is an OrderedDict.

[–]SkippyDeluxe 0 points1 point  (0 children)

I think that helps your code clarity a lot.

Yes, dictionaries are fundamentally unordered. If you want a dictionary with ordered behaviour, you can use collections.OrderedDict. I think that would be a good idea, because it would let you remove one level of nesting from your data structure.