HiveBrain v1.2.0
Get Started
← Back to all entries
patterncppMinor

Interpreter programming challenge

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
interpreterprogrammingchallenge

Problem

I have posted here my working and accepted solution to the intepreter programming challenge (detailed here) for your review. The challenge is as follows:


A certain computer has 10 registers and 1000 words of RAM. Each register or RAM location holds a 3-digit integer between 0 and 999. Instructions are encoded as 3-digit integers and stored in RAM.
The encodings are as follows:



  • 100 means halt



  • 2dn means set register d to n (between 0 and 9)



  • 3dn means add n to register d



  • 4dn means multiply register d by n



  • 5ds means set register d to the value of register s



  • 6ds means add the value of register s to register d



  • 7ds means multiply register d by the value of register s



  • 8da means set register d to the value in RAM whose address is in register a



  • 9sa means set the value in RAM whose address is in register a to the value of register s



  • 0ds means goto the location in register d unless register s contains 0





All registers initially contain 000. The initial content of the RAM is read from standard input. The first instruction to be executed is at RAM address 0. All results are reduced modulo 1000.


Input


The input begins with a single positive integer on a line by itself indicating the number of the cases following, each of them as described below. This line is followed by a blank line, and there is also a
blank line between two consecutive inputs.


The input to your program consists of up to 1000 3-digit unsigned integers, representing the contents of consecutive RAM locations starting at 0. Unspecified RAM locations are initialized to 000.


Output


For each test case, the output must follow the description below. The outputs of two consecutive cases will be separated by a blank line.


The output from your program is a single integer: the number of instructions executed up to and including the halt instruction. You may assume that the program does

Solution

Here are some things that may help you improve your program.

Fix the bug

The code currently contains these two lines:

vector registers(N_RAM_WORDS, DEFAULT_VALUE); 
vector ram_words(N_REGISTERS, DEFAULT_VALUE);


However, it's obvious that N_RAM_WORDS and N_REGISTERS should be swapped. Being able to spot such errors is one advantage to having named constants as you do.

Eliminate global variables

In this case, eliminating global variables is easy and obvious. As @LokiAstari advises, simply wrap everything except n_cases up into a nice neat Machine object. Then n_cases can be declared within main.

Make all instruction handlers alike

All instructions except for halt look alike. They are each void functions taking two int arguments. I'd suggest making halt look like that, too, and move the parameter checking to within the body of the halt function. This makes it easier to implement the next suggestion.

Consider making the code more data driven

Right now there is a big switch with each case being almost identical except for the function called. I'd recommend instead using a table driven approach instead. In my case, I made all instruction handlers return bool which indicates "halted", so only halt() actually returns true. The call then looks like this (

if ((this->*inst[instruction])(parameter1, parameter2))
    return;


I've made all of the functions member functions of a Machine class as mentioned above. Then there is a table that is also part of that Machine class:

bool(Machine::*inst[10])(int, int) = {
            &Machine::jump_to,
            &Machine::halt,
            &Machine::set_register,
            &Machine::increment_register,
            &Machine::multiply_register,
            &Machine::copy_register,
            &Machine::sum_registers,
            &Machine::multiply_registers,
            &Machine::copy_ram_to_register,
            &Machine::set_ram,
    };


This is simply an array named inst of 10 pointers to member functions. The syntax would be similar for C-style functions except of course they would not have a Machine:: anywhere.

Use all required #includes

The code makes use of std::string and std::stoi but does not #include . It should.

Use only required #includes

It doesn't appear to me that this program actually needs anything from either ` or ` so these two lines can safely be deleted:

#include 
#include 

Code Snippets

vector<int> registers(N_RAM_WORDS, DEFAULT_VALUE); 
vector<int> ram_words(N_REGISTERS, DEFAULT_VALUE);
if ((this->*inst[instruction])(parameter1, parameter2))
    return;
bool(Machine::*inst[10])(int, int) = {
            &Machine::jump_to,
            &Machine::halt,
            &Machine::set_register,
            &Machine::increment_register,
            &Machine::multiply_register,
            &Machine::copy_register,
            &Machine::sum_registers,
            &Machine::multiply_registers,
            &Machine::copy_ram_to_register,
            &Machine::set_ram,
    };
#include <sstream>
#include <fstream>

Context

StackExchange Code Review Q#146003, answer score: 5

Revisions (0)

No revisions yet.