Simple Language Interpreter

Question

I've been playing around with Python off and on for about the past year and recently came up with the following 68 (was 62) lines. I think I'll try making a calculator out of it. I'd really like to know what readers here think of its attributes such as coding style, readability, and feasible purposefulness.

# notes: separate addresses from data lest the loop of doom cometh

class Interpreter:

  def __init__(self):
    self.memory = { }
    self.dictionary = {"mov" : self.mov,
                       "put" : self.put,
                       "add" : self.add,
                       "sub" : self.sub,
                       "clr" : self.clr,
                       "cpy" : self.cpy,
                       "ref" : self.ref }
    self.hooks = {self.val("0") : self.out }

  def interpret(self, line):
    x = line.split(" ")
    vals = tuple(self.val(y) for y in x[1:])
    dereferenced = []
    keys_only = tuple(key for key in self.memory)
    for val in vals:
      while val in self.memory: val = self.memory[val]
      dereferenced.append(val)
    vals = tuple(y for y in dereferenced)
    self.dictionary[x[0]](vals)

  def val(self, x):
    return tuple(int(y) for y in str(x).split("."))

  def mov(self, value):
    self.ptr = value[0]

  def put(self, value):
    self.memory[self.ptr] = value[0]

  def clr(self, value):
    if self.ptr in self.hooks and self.ptr in self.memory:
      x = self.hooks[self.ptr]
      y = self.memory[self.ptr]
      for z in y: x(z)
    del self.memory[self.ptr]

  def add(self, values):
    self.put(self.mat(values, lambda x, y: x + y))

  def sub(self, values):
    self.put(self.mat(values, lambda x, y: x - y))

  def mat(self, values, op):
    a, b = self.memory[values[0]], self.memory[values[1]]
    if len(a) > len(b): a, b = b, a
    c = [op(a[x], b[x]) for x in xrange(len(b))] + [x for x in a[len(a):]]
    return [tuple(x for x in c)]

  def cpy(self, value):
    self.put(value)

  def out(self, x):
    print chr(x),

  def ref(self, x):
    self.put(x)

interp = Interpreter()
for x in file(__file__.split('/')[-1].split(".")[-2] + ".why"):
  interp.interpret(x.strip())

\$\begingroup\$ It would be nice to see some sample input too. \$\endgroup\$

John Gaines Jr.
– John Gaines Jr.

2011-06-17 14:37:04 +00:00
Commented Jun 17, 2011 at 14:37 — John Gaines Jr.
– John Gaines Jr., Commented Jun 17, 2011 at 14:37

John Gaines Jr. · Accepted Answer · 2011-06-17 14:41:17Z

To allow your module to be loadable by other files, it's customary to write the end of it with a if __name__ == '__main__': conditional like so:

if __name__ == '__main__':
    interp = Interpreter()
    for x in file(__file__.split('/')[-1].split(".")[-2] + ".why"):
        interp.interpret(x.strip())

Maybe I'm being picky (but you did ask for style input), read PEP8 and try to follow it as best you can (stand). One thing that jumped out at me right away was your 2 space indentation vs. the PEP8 recommendation of 4. One letter variables are usually only recommended for looping vars. You could probably increase the readability of your code by renaming some of those x's, y's, a's, etc.

Another maxim of Python programming is to use the tools provided, I was pondering what you were doing with:

__file__.split('/')[-1].split(".")[-2] + ".why"

an alternative that uses existing Python modules (and is more portable across platforms) is:

os.path.splitext(os.path.basename(__file__))[0] + ".why"

It's about the same length, and is a good deal more clear as to what you're doing as the function names spell it out.

Martin Vilcans · Accepted Answer · 2011-06-17 22:19:06Z

Sorry for bluntness, intended or not, but here we go!

Not very good readability IMO. It takes a while to understand what the code does. Unit tests or at least some sample input and output may make it obvious.

In general, naming should be improved. It's difficult to understand what a function or variable does from its name.

There's also much shuffling of data back and forth that seems unnecessary. It looks like this can be simplified quite a bit, see inline comments.

Also, you use tuples in many situations where a list would be more natural.

As I had problems understanding the code, I may have misunderstood things in it, so read the following inline comments with that in mind.

# notes: separate addresses from data lest the loop of doom cometh

Not a very helpful comment, is it?

class Interpreter:

Can you find a class name that better tells what this class does? It not only interprets.

  def __init__(self):
    self.memory = { }

I'd like a comment on what self.memory is.

    self.dictionary = {"mov" : self.mov,
                       "put" : self.put,
                       "add" : self.add,
                       "sub" : self.sub,
                       "clr" : self.clr,
                       "cpy" : self.cpy,
                       "ref" : self.ref }

No shit, it's a dictionary. But can you find a name that tells what kind of dictionary it is?

To reduce code size, you may consider self.dictionary = dict(self.getattr(attr) for attr in 'mov put add sub clr cpy ref'.split()), but some people may find it too "clever."

    self.hooks = {self.val("0") : self.out }

Very unclear what hooks is for. This is the only value that is every put in it. self.val("0") seems to be just a complicated way of saying (0,). What's up with that?

  def interpret(self, line):
    x = line.split(" ")

Bad variable name: x. Should it be tokens instead?

    vals = tuple(self.val(y) for y in x[1:])

Here, vals could be a list instead, like vals = [self.val(y) for y in x[1:]]. Making it a tuple just makes the code a little bit more complicated for no reason.

    dereferenced = []
    keys_only = tuple(key for key in self.memory)

Consider keys_only = self.memory.keys()? Oh, and by the way, keys_only is never used, so just remove it.

    for val in vals:
      while val in self.memory: val = self.memory[val]

Looks like this while loop will be infinite if it runs at all. Is it supposed to be an if?

      dereferenced.append(val)
    vals = tuple(y for y in dereferenced)

Pointless conversion of dereferenced to tuple.

    self.dictionary[x[0]](vals)

I think the whole interpret method can be rewritten to something like the following. Note the changed names.

  def interpret(self, line):
      command, *tokens = line.split()  # Python 3 only
      values = [self.val(y) for token in tokens]
      dereferenced = [self.memory.get(value, value) for value in values]
      self.dictionary[command](dereferenced)

Some may say it's a bit too terse. I could have written the dereferenced = ... line as a couple of more explicit statements perhaps.

I won't go into the rest, but the comments about naming and unnecessary complexity applies.

Stack Exchange Network

Simple Language Interpreter

2 Answers 2

You must log in to answer this question.

Hot Network Questions

Simple Language Interpreter

2 Answers 2

You must log in to answer this question.

Related

Hot Network Questions