patterncppMinor
Interpreter programming challenge
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:
All registers initially contain
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
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:
100means halt
2dnmeans set register d to n (between 0 and 9)
3dnmeans add n to register d
4dnmeans multiply register d by n
5dsmeans set register d to the value of register s
6dsmeans add the value of register s to register d
7dsmeans multiply register d by the value of register s
8dameans set register d to the value in RAM whose address is in register a
9sameans set the value in RAM whose address is in register a to the value of register s
0dsmeans 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:
However, it's obvious that
Eliminate global variables
In this case, eliminating global variables is easy and obvious. As @LokiAstari advises, simply wrap everything except
Make all instruction handlers alike
All instructions except for
Consider making the code more data driven
Right now there is a big
I've made all of the functions member functions of a
This is simply an array named
Use all required
The code makes use of
Use only required
It doesn't appear to me that this program actually needs anything from either `
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
#includesThe code makes use of
std::string and std::stoi but does not #include . It should.Use only required
#includesIt 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.