Skip to main content
added 190 characters in body
Source Link
alecxe
  • 17.5k
  • 8
  • 52
  • 93

Few nitpicks and suggested, possible improvements and random ideas:

  • if not val in registers: is a bit less natural as if val not in registers:

  • you can replace if str == '': with if not str:

  • str, though, is not a good variable name since it shadows a built-in str type

  • is_int though can probably use the EAFP approach - try converting it to int and handle possible errors:

     def is_int(value): try: int(value) return True except (ValueError, TypeError): return False 
  • max_register can be simplified to:

     return max(registers.values()) 
  • it is a good practice to define regular expression strings as raw strings

  • I think you can unpack the captured group values into variables:

     res = re.search(r'([^ ]*) ([^ ]*) ([^ ]*)', line) self.left, self.operator, self.right = res.groups() 
  • the Condition class may benefit from the operator module. Instead of having multiple if/elifs, you may define a mapping of operator strings to operator functions:

     import operator OPERATIONS = { '==': operator.eq, '!=': operator.ne, '>': operator.gt, '>=': operator.gte, '<': operator.lt, '<=': operator.le } l, r = (value(registers, self.left), value(registers, self.right)) return OPERATIONS[self.operator](l, r) 

Note that I would check for validness of the operator when you extract it via the regular expression in the __init__. Also, OPERATIONS can probably a module or class-level constant.

  • think of pre-compiling regular expressions with re.compile() and re-using

Few nitpicks and suggested improvements:

  • if not val in registers: is a bit less natural as if val not in registers:

  • you can replace if str == '': with if not str:

  • str, though, is not a good variable name since it shadows a built-in str type

  • is_int though can probably use the EAFP approach - try converting it to int and handle possible errors:

     def is_int(value): try: int(value) return True except (ValueError, TypeError): return False 
  • max_register can be simplified to:

     return max(registers.values()) 
  • it is a good practice to define regular expression strings as raw strings

  • I think you can unpack the captured group values into variables:

     res = re.search(r'([^ ]*) ([^ ]*) ([^ ]*)', line) self.left, self.operator, self.right = res.groups() 
  • the Condition class may benefit from the operator module. Instead of having multiple if/elifs, you may define a mapping of operator strings to operator functions:

     OPERATIONS = { '==': operator.eq, '!=': operator.ne, '>': operator.gt, '>=': operator.gte, '<': operator.lt, '<=': operator.le } l, r = (value(registers, self.left), value(registers, self.right)) return OPERATIONS[self.operator](l, r) 

Few nitpicks, possible improvements and random ideas:

  • if not val in registers: is a bit less natural as if val not in registers:

  • you can replace if str == '': with if not str:

  • str, though, is not a good variable name since it shadows a built-in str type

  • is_int though can probably use the EAFP approach - try converting it to int and handle possible errors:

     def is_int(value): try: int(value) return True except (ValueError, TypeError): return False 
  • max_register can be simplified to:

     return max(registers.values()) 
  • it is a good practice to define regular expression strings as raw strings

  • I think you can unpack the captured group values into variables:

     res = re.search(r'([^ ]*) ([^ ]*) ([^ ]*)', line) self.left, self.operator, self.right = res.groups() 
  • the Condition class may benefit from the operator module. Instead of having multiple if/elifs, you may define a mapping of operator strings to operator functions:

     import operator OPERATIONS = { '==': operator.eq, '!=': operator.ne, '>': operator.gt, '>=': operator.gte, '<': operator.lt, '<=': operator.le } l, r = value(registers, self.left), value(registers, self.right) return OPERATIONS[self.operator](l, r) 

Note that I would check for validness of the operator when you extract it via the regular expression in the __init__. Also, OPERATIONS can probably a module or class-level constant.

  • think of pre-compiling regular expressions with re.compile() and re-using
Source Link
alecxe
  • 17.5k
  • 8
  • 52
  • 93

Few nitpicks and suggested improvements:

  • if not val in registers: is a bit less natural as if val not in registers:

  • you can replace if str == '': with if not str:

  • str, though, is not a good variable name since it shadows a built-in str type

  • is_int though can probably use the EAFP approach - try converting it to int and handle possible errors:

     def is_int(value): try: int(value) return True except (ValueError, TypeError): return False 
  • max_register can be simplified to:

     return max(registers.values()) 
  • it is a good practice to define regular expression strings as raw strings

  • I think you can unpack the captured group values into variables:

     res = re.search(r'([^ ]*) ([^ ]*) ([^ ]*)', line) self.left, self.operator, self.right = res.groups() 
  • the Condition class may benefit from the operator module. Instead of having multiple if/elifs, you may define a mapping of operator strings to operator functions:

     OPERATIONS = { '==': operator.eq, '!=': operator.ne, '>': operator.gt, '>=': operator.gte, '<': operator.lt, '<=': operator.le } l, r = (value(registers, self.left), value(registers, self.right)) return OPERATIONS[self.operator](l, r)