3
\$\begingroup\$

I am writing a recreational language based off the speech pattern of Hodor from Game of Thrones. Basically the only commands are Hodor. You can find the complete spec here.

One of my friends had a look at a different piece of code (but still somewhat related) and said that it was

No joke, it [your program] was very well structured and organised. Good job!

Now I know this sounds condescending but he is teaching me how to code better.

I then asked him to have a look at the Hodor interpreter I've written but he never got round to it so I thought I'd post it here for review.

The code is written in Python and can be found here. I'm starting to become more comfortable with classes and have used them in my code (don't say I didn't warn you)

Are there any tips you can give me on it? Is it perfect? (If so I'll quit this site and never return. Scouts honour)

Here is the code (as required). I know that the errors are unhelpful. They're deliberately designed like that so feel free to talk about them all you want, I'm not changing them:

import operand

VALID_CHARS = 'HODORhodor!?,:;.Walder \n'

class Exit(Exception):
    pass

class Accumulator(int):

    def __init__(self):
        self.copy = 0
        self.value = 0

    def add(self):
        self.value += 1

    def sub(self):
        self.value -= 1

    def clear(self):
        self.value = 0

    def math_operand(self,oper,oper_value):
        operands = operand.all_attrs()[oper-1]
        self.value = operands(self.value,oper_value)

    @property
    def out(self):
        return chr(abs(self.value))

    def take_input(self,input_):
        input_ = eval(input_)
        if type(input_) == int:
            self.value += input_
        else:
            self.value += ord(input_[0])

    def save(self):
        self.copy = self.value

    def give(self):
        self.value = self.copy
        self.copy = 0

    def swap(self):
        self.value, self.copy = self.copy, self.value

accumulator = Accumulator()

class IfStatement:

    def __init__(self,number,code,accumulator):
        self.commands = dict(
            enumerate([self.iseven,self.isodd,self.isprime,self.isnotprime,
                       self.iszero,self.isone,self.isnegative,self.ispositive,
                       self.isascii,self.isprintable,self.isletter],1))

        self.result = self.commands[number](accumulator.value)

    def iseven(self,acc):
        return acc % 2 == 0

    def isodd(self,acc):
        return not self.iseven(acc)

    def isprime(self,acc):
        if acc < 2:
            return False
        for i in range(2,acc//2):
            if acc % i == 0:
                return False
        return True

    def isnotprime(self,acc):
        return not self.isprime(acc)

    def iszero(self,acc):
        return acc == 0

    def isone(self,acc):
        return acc == 1

    def isnegative(self,acc):
        return acc < 0

    def ispositive(self,acc):
        return acc > 0

    def isascii(self,acc):
        return acc <= 126

    def isprintable(self,acc):
        return 32 <= acc <= 126

    def isletter(self,acc):
        return chr(acc).isalpha()

class Execute:

    def __init__(self,code,acc):

        STDIN = open('stdin.txt').readlines()

        input_line = 0    
        code = code.strip()
        self.syntax_check(code)
        code = code.split('\n')[1:-1]

        for line in code:

            cmd = line.split()[0]

            if cmd == 'Hodor,':
                args = list(map(str.split, line.split(',')[1:-1]))
                math_args = []
                if args[1][0] == 'hodor!':
                    args[1][0] = acc.copy
                    math_args = [len(args[0]),args[1][0]]
                else:
                    for arg in args:
                        arg.insert(0,0)
                        for a in arg[1:]:
                            if a == 'Hodor':
                                arg[0] += 1
                            if a == 'Hodor!':
                                arg[0] -= 1
                            if a == 'Hodor.':
                                arg[0] = 0
                        arg = arg[0]
                        math_args.append(arg)
                acc.math_operand(*math_args)

            elif cmd == 'Hodor?':
                args = line.split('?')[1:-1]
                args = [[0]+args[0].split()]+[args[1].strip()]
                if_args = []
                for a in args[0][1:]:
                    if a == 'Hodor':
                        args[0][0] += 1
                    if a == 'Hodor!':
                        args[0][0] -= 1
                    if a == 'Hodor.':
                        args[0][0] = 0
                if_args = [args[0][0], args[1], acc]
                if IfStatement(*if_args).result:
                    self.__init__('Wylis\n'+args[1]+'\nHODOR!!!',acc)

            elif cmd == 'Hodor:':
                args = line.split(':')[1].split(';')[0]
                for i in range(acc.value):
                    self.__init__('Wylis\n'+args+'\nHODOR!!!',acc)

            elif cmd == 'Hodor?!':
                acc.take_input(STDIN[input_line])
                input_line += 1

            elif cmd == 'HODOR!!!':
                raise Exit()

            elif cmd == 'hodor.':
                acc.save()

            elif cmd == 'hodor!':
                acc.give()

            elif cmd == 'hodor,':
                acc.swap()

            else:
                for command in line.split():
                    if command == 'Hodor':
                        acc.add()
                    elif command == 'Hodor!':
                        acc.sub()
                    elif command == 'Hodor.':
                        acc.clear()
                    elif cmd == 'HODOR!!!':
                        raise Exit()
                    elif cmd == 'HODOR!!':
                        print(acc.value,end=' ')
                    else:
                        print(acc.out,end='')

    def syntax_check(self,code):
        if not code.startswith('Walder\n'):
            raise SyntaxError("Hodor hodor hodor hodor: {} hodor hodor 'Walder'".format(code.split()[0]))
        if not code.endswith('\nHODOR!!!'):
            raise SyntaxError("Hodor hodor hodor: hodor hodor hodor hodor HODOR!!! hodor '{}'".format(code.split()[-1]))
        for char in code:
            if char not in VALID_CHARS:
                raise SyntaxError("Hodor hodor: '{}'".format(char))
\$\endgroup\$
4
\$\begingroup\$

This is quite good.

  1. I find your code a little annoying to read, as I'm used to reading Python code that follows PEP8. You may want to give following it a try, but since your code already has a style and all that matters is keeping to it.
  2. When making static methods, ones that don't need self, use staticmethod.
  3. Don't subclass int on Accumulator. You're not using it.
  4. Try to stay away from eval. You can normally achieve the same result without using it. In this case if you only take numbers and strings you can use:

    def take_input(self,input_):
        try:
            value = int(input_)
        except ValueError:
            value = ord(input_[0])
        self.value += value
    
  5. isprime can be more efficient. If \$n = ab\$, where \$a \le b\$ what is the largest possible \$a\$? Since the largest \$a\$ would equal \$b\$ then the equation becomes \$n = a^2\$. And so, rather than checking up to \$n / 2\$ you can check up to \$\sqrt{n}\$.

    def isprime(self,acc):
        if acc < 2:
            return False
        return all(
            acc % i
            for i in range(2, int(acc**0.5) + 1)
        )
    

    But if the domain of acc is small then you may want to instead create a set of prime numbers and check if it's in that set.

    However you can use something like lru_cache. Which can be implemented as:

    @staticmethod
    @lru_cache()
    def isprime(acc):
        if acc < 2:
            return False
        return all(
            acc % i
            for i in range(2, int(acc**0.5) + 1)
        )
    
  6. In isprintable, prefixing all your functions with is is redundant.

  7. Use with to safely close a file.
  8. Rather than reading the entire file to memory, you can instead read the file line by line by looping through a file object.

    with open('file') as file:
        for line in file:
            print(line)
    
  9. Don't use self.__init__. I'm somewhat surprised your code isn't doing anything dodgy because of it. Instead just make another function that is independent from Python internals.

  10. Split up Execute.__init__ into smaller functions, I don't know what I can edit freely without messing up your program.
  11. Use variables rather than indexes of lists. Some of your code is really hard to read due to this. Take:

    for arg in args:
        arg.insert(0,0)
        for a in arg[1:]:
            if a == 'Hodor':
                arg[0] += 1
            if a == 'Hodor!':
                arg[0] -= 1
            if a == 'Hodor.':
                arg[0] = 0
        arg = arg[0]
        math_args.append(arg)
    

    This is hard to understand, does arg = arg[0] do something magical? Why do you arg.insert(0,0) to then make your code harder to read. Instead use something like:

    for arg in args:
        value = 0
        for a in arg:
            if a == 'Hodor':
                value += 1
            if a == 'Hodor!':
                value -= 1
            if a == 'Hodor.':
                value = 0
        math_args.append(value)
    
  12. Finally, you can make a joke esoteric programming language, without spamming your joke everywhere. You should make your errors actually somewhat helpful. And make your GitHub documentation helpful. I don't care much for Hodor, or the dead meme he is. But when I go to a readme I want to be able to read it, and for it to be informative. Bottom line, it makes people go from finding it a little funny, to largely annoyed.

\$\endgroup\$
  • \$\begingroup\$ Check the English folder on the GitHub page. \$\endgroup\$ – caird coinheringaahing Apr 11 '17 at 9:19
  • \$\begingroup\$ @ThisGuy Make that the default documentation. \$\endgroup\$ – Peilonrayz Apr 11 '17 at 9:19
  • 1
    \$\begingroup\$ I still like the Hodor meme, even though it's dead. \$\endgroup\$ – caird coinheringaahing Apr 11 '17 at 9:20

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.