5
\$\begingroup\$

I'm using advent of code 2019 to start learning Python. I've read the Python style guide, and I'm trying to write readable and reusable code.

This piece of code is an implementation of the IntCode computer used in many of the assignments. If you'd like to see code for the specific assignments, it's on my Github.

Here's the code for the IntCode computer itself. Any suggestions for improvement are welcome, especially relating to the 'pythonic' way of doing things. For reference, my background is in Java.

import threading
from typing import List


class IntCoder:
    _memory = []
    __pointer = 0
    __relative_base = 0

    def __init__(self, memory: List[int]):
        self._memory = memory

    def run(self):
        while self.__step():
            pass

    def get_input(self):
        raise ValueError('No input defined')

    def handle_output(self, value):
        print(value)

    def read_pointer(self):
        return self.read(self.__pointer)

    def read(self, index):
        return self._memory[index]

    def __step(self):
        instruction = self._memory[self.__pointer]
        opcode = instruction % 100
        mode1 = (instruction // 100) % 10
        mode2 = (instruction // 1000) % 10
        mode3 = instruction // 10000
        if opcode == 1:
            self.__add(self.__val(self.__pointer + 1, mode1), self.__val(self.__pointer + 2, mode2), self.__index(self.__pointer + 3, mode3))
        elif opcode == 2:
            self.__mul(self.__val(self.__pointer + 1, mode1), self.__val(self.__pointer + 2, mode2), self.__index(self.__pointer + 3, mode3))
        elif opcode == 3:
            self.__in(self.__index(self.__pointer + 1, mode1))
        elif opcode == 4:
            self.__out(self.__val(self.__pointer + 1, mode1))
        elif opcode == 5:
            self.__if_true(self.__val(self.__pointer + 1, mode1), self.__val(self.__pointer + 2, mode2))
        elif opcode == 6:
            self.__if_false(self.__val(self.__pointer + 1, mode1), self.__val(self.__pointer + 2, mode2))
        elif opcode == 7:
            self.__lt(self.__val(self.__pointer + 1, mode1), self.__val(self.__pointer + 2, mode2), self.__index(self.__pointer + 3, mode3))
        elif opcode == 8:
            self.__eq(self.__val(self.__pointer + 1, mode1), self.__val(self.__pointer + 2, mode2), self.__index(self.__pointer + 3, mode3))
        elif opcode == 9:
            self.__arb(self.__val(self.__pointer + 1, mode1))
        elif opcode == 99:
            return False
        else:
            raise ValueError('Unknown opcode {} in instruction {}'.format(opcode, instruction))
        return True

    def __val(self, address, mode):
        index = self._memory[address]
        if mode == 0:
            return self._memory[index]
        elif mode == 1:
            return index
        elif mode == 2:
            return self._memory[index + self.__relative_base]
        else:
            raise ValueError('Unknown parameter mode {}'.format(mode))

    def __index(self, address, mode):
        if mode == 0:
            return self._memory[address]
        elif mode == 2:
            return self._memory[address] + self.__relative_base
        raise ValueError('Got mode {} for index only type param'.format(mode))

    def __add(self, first, second, result):
        self._memory[result] = first + second
        self.__pointer += 4

    def __mul(self, first, second, result):
        self._memory[result] = first * second
        self.__pointer += 4

    def __in(self, index):
        self._memory[index] = self.get_input()
        self.__pointer += 2

    def __out(self, value):
        self.handle_output(value)
        self.__pointer += 2

    def __if_true(self, first, second):
        self.__pointer = second if first != 0 else self.__pointer + 3

    def __if_false(self, first, second):
        self.__pointer = second if first == 0 else self.__pointer + 3

    def __lt(self, first, second, result):
        self._memory[result] = 1 if first < second else 0
        self.__pointer += 4

    def __eq(self, first, second, result):
        self._memory[result] = 1 if first == second else 0
        self.__pointer += 4

    def __arb(self, value):
        self.__relative_base += value
        self.__pointer += 2

    @staticmethod
    def extended_memory(program, size):
        memory = [0] * size
        memory[0:len(program)] = program
        return memory

    @staticmethod
    def read_file(file):
        with open(file, 'r') as f:
            return [int(s) for s in f.read().split(',')]


class IntCoderWithIo(IntCoder):
    def __init__(self, memory, input_values: List[int]):
        super(IntCoderWithIo, self).__init__(memory)
        self.input_values = (n for n in input_values)
        self.output = []

    def get_input(self):
        return next(self.input_values)

    def handle_output(self, value):
        self.output.append(value)
\$\endgroup\$
  • \$\begingroup\$ Please add an excerpt of the programming challenge to the question itself. Links can rot. Thank you. \$\endgroup\$ – Mast Dec 9 '19 at 18:48
  • \$\begingroup\$ There are multiple IntCode challenges from Advent of Code, can you specify which days this one fulfills the requirements for? \$\endgroup\$ – Simon Forsberg Dec 9 '19 at 18:56
  • \$\begingroup\$ Do you have a particular Python3 version you are targeting? \$\endgroup\$ – richardb Dec 9 '19 at 21:13
  • \$\begingroup\$ @SimonForsberg It's the combined final product from specifications found in days 2, 5, 7 and 9. Day 9 has a test program that validates the entire thing. \$\endgroup\$ – Jorn Dec 10 '19 at 9:05
  • \$\begingroup\$ @Jorn Great. You’ve got all the specifications. Now you should edit your post and add the specifications to it. Links can rot; adding the specs preserves the integrity of your question for future readers. Also, maybe you made a mistake in your implementation not covered in day 9 tests; if we knew the specs, we can see if you covered them all. Maybe there are efficiencies suggested by the spec that have been missed; we could catch those too. \$\endgroup\$ – AJNeufeld Dec 10 '19 at 14:25
6
\$\begingroup\$

Unused Imports

You've imported threading, but you are not using it anywhere in this module.

Dunder

Dunder, or double-underscore, members have special meaning in Python. Specifically, the Python interpreter does a form of name-mangling, to prevent member names defined in the parent class from colliding with member names defined in a derived class. This also prevents the derived class from accessing members defined in the parent class. I doubt this is what you were intending here. Avoid creating your own double underscores members.

Class Variables

Java lets you initialize member variables outside the constructor; Python does not. What you are doing is ugly, dangerous, and will bite you down the road.

class IntCoder:

    _memory = []                 # Class member
    _pointer = 0                 # Class member
    _relative_base = 0           # Class member

    def __init__(self, memory: List[int]):
        self._memory = memory    # Instance member

This code creates 3 class member variables (Ie, static in Java terminology), and then the IntCoder() constructor creates one instance member, shadowing the first class member.

When Python looks up self.member, it first looks in the self object's dictionary for member. If that fails, it will also look in the class's dictionary. Assignments to self.member will always create member in the object instance.

Now consider:

    self._pointer += 4

If _pointer is defined in the instance object, this is interpreted as:

    self._pointer = self._pointer + 4

If _pointer is not defined in the instance object, but exists in the class, this is interpreted as looking up the class member, and creating the instance member:

    self._pointer = IntCoder._pointer + 4

Yikes! Mutating code interpretation!

You really want each IntCoder to have its own, independent state, so they each should create their own instance members in the constructor:

class IntCoder:

    def __init__(self, memory: List[int]):
        self._memory = memory 
        self._pointer = 0      
        self._relative_base = 0

Pointer Management

These next few items all go together ...

self.read_pointer()

Never called. Why is this here?

instruction fetching

    instruction = self._memory[self.__pointer]

Looks like that could have been instruction = self.read_pointer(). But ...

Separation of concerns

    def __add(self, first, second, result):
        self._memory[result] = first + second
        self.__pointer += 4

Whoa, whoa, hold the phone. Ok, I understand you are adding the values first and second together, and storing the result in memory at address result. But where the heck did that self.__pointer += 4 come from? And why 4?

        if opcode == 1:
            self.__add(self.__val(self.__pointer + 1, mode1), self.__val(self.__pointer + 2, mode2), self.__index(self.__pointer + 3, mode3))

Ok, looking back, we can see that we got a value using from self._pointer + 1, a second value using self._pointer + 2, and use self._pointer + 3 to get the address to store the result. So that explains 3 of the increment of 4...

        instruction = self._memory[self.__pointer]

Oh ya, and the instruction itself in the 4th value.

What is the IntCoder really doing? It is:

  1. Fetching the opcode
  2. Fetching the first operand, or index to it
  3. Fetching the second operand, or index to it
  4. Fetching the destination address or index

Or, to summarize, it is fetching, ... and implicit in the fetch is an increment of the pointer.

    def _fetch(self):
        value = self._memory[self._pointer]
        self._pointer += 1
        return value

Now you can use:

        instruction = self._fetch()

and know that if you called _fetch() again, the next value would be read, and then the next, and the next. No need to externally code self._pointer += 1 anymore. Let's do the same for _val():

    def _val(self, mode):
        index = self._fetch()
        if mode == 0:
            return self._memory[index]
        if mode == 1:
            return index
        if mode == 2:
            return self._memory[index + self._relative_base]
        raise ValueError(f"Unknown parameter mode {mode}")

Note: Python 3.6 introduced f-strings, which allow you to embed variables right in strings, rather than needing .format(...)

If you also added the _fetch() to _index, you'd could now write:

        if opcode == 1:
            self._add(self._val(mode1), self._val(mode2), self._index(mode3))

and

    def _add(self, first, second, result):
        self._memory[result] = first + second

Decoding Instructions

A large chain of if opcode == 0: elif opcode == 1: elif opcode == 2: elif opcode == 3: ... is an indication of doing something wrong, or at least the non-Pythonic way.

Methods are first class objects in Python, and can be used as values and stored in containers. Which means you can use a dictionary to decode and dispatch based on the opcode:

class IntCoder:

    def __init__(self):
        self._opcodes = { 1: self._add,
                          2: self._mul,
                          3: self._in,
                          ...
                        }

    def _step(self):
        instruction = self._fetch()
        opcode = instruction % 100
        mode1 = (instruction // 100) % 10
        mode2 = (instruction // 1000) % 10
        mode3 = instruction // 10000

        try:
            self._opcodes[opcode](mode1, mode2, mode3)
        except KeyError:
            raise ValueError('Unknown opcode {opcode} in instruction {instruction-1}')

    def _add(self, mode1, mode2, mode3):
        a = self._val(mode1)
        b = self._val(mode2)
        dst = self._index(mode3)
        self._memory[dst] = a + b

    def _in(self, mode1, mode2, mode3):
        index = self._index(mode1)
        self._memory[index] = self.get_input()

Note we've changed thing a bit. The responsibility for fetching the values to add, and the destination to store the results into has moved into the individual functions. Additionally, we are passing unnecessary information to the _in() method; it doesn't need the mode2 and mode3 values, but it needs those arguments since with this dispatch method, all the methods have to use a common parameter list.

Iterators

The following is creating and storing a generator expression, to walk through the list of values, returning the values one at a time:

self.input_values = (n for n in input_values)

Internally, for n in input_values is walking through input_values, return each value one at a time. It works because input_values is an iterable. We can retrieve an iterator for that iterable object and use that directly.

self.input_values = iter(input_values)

Then you could pass in any input_values: Iterable[int], instead of just lists.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Very helpful, thank you! It's going to take me a while to work through all of this. The first question I have is on the dunder, I took it to be a kind of 'private'. I understand I should define pointer as an instance variable only, but would you recommend also making it visible? Nothing outside the class should need to access it. Oh, and the reason memory only has a single underscore is that it should not need to be visible, except I'm reading it in a test. The Java equivalent would be @VisibleForTesting with package visibility. \$\endgroup\$ – Jorn Dec 10 '19 at 8:38
  • \$\begingroup\$ I made a lot of improvements based on your comments. Thanks again for the input! One more question: Am I using type hints correctly? Should I be using more of them? They are currently on the constructor only, should they also be on the 'public' methods? \$\endgroup\$ – Jorn Dec 10 '19 at 10:45
  • 1
    \$\begingroup\$ "Private" in Python is by convention; it is not enforced. A single leading underscore implies private/protected. Checkers (like PyLint) will detected accesses to variable._member and complain, where as self._member is allowed. However, in Python, "we're all consenting adults", and nothing prevents you from playing with another object's private members. Not even dunders: print(coder._IntCoder__pointer) works just fine. Feel free to access uut._member from test code all you want; people will be happy test code exists, and not complain about private member access. \$\endgroup\$ – AJNeufeld Dec 10 '19 at 22:51
  • 1
    \$\begingroup\$ Your type-hint usage looked correct, albeit sparse. If you are using type-hints, all public members should have type-hints. All public members should have """docstrings""", too. This helps automatic documentation generation, allowing other people to use your class without needing to read the source code. Internal (private/protected) members would also benefit from type-hints, as this will allow static analysis tools to detect bugs or inconsistencies. But the more your document these internals, the more likely people will be to reach in & access your private members, so double edged sword. \$\endgroup\$ – AJNeufeld Dec 10 '19 at 23:00

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.