Problem
I have this Brainfuck interpreter:
// Credit goes to where it is due
function throwException(cause, index) {
// Execute when there is an error in Brainf*** code
$('p#output').text("Interpreting Terminated because " + cause + " at char index " + index);
}
function runBrainfuck(data) {
console.time("Running Brainf*** code");
var memory = new Array(); // memory
// Initialize memory
console.time("Initialize memory");
for(var i = 0; i < 30000; i++) {
memory[i] = 0;
}
console.timeEnd("Initialize memory");
var p = 0; // pointer to memory
$('p#output').text("");
// Loop through the Brainf*** code
var data = $('#code').val();
for(var j = 0; j < data.length; j++) {
switch(true) {
case data[j] === ">": // go right one cell
if(p < 30000) {
p++;
} else {
throwException("There are only 30000 usable cells of memory.", j); // are you using too many cells?
}
break;
case data[j] === "<": // go left one cell
if(p > 0) {
p--;
} else {
throwException("Cannot decrement pointer when it is at first cell.", j); // are you going below cell 0?
}
break;
case data[j] === "+": // Increment cell value
memory[p]++;
if(memory[p] > 255) {
memory[p] = 0; // Whoops, overflown the memory!
}
break;
case data[j] === "-": // decrement cell value
if(memory[p] > 0) {
memory[p]--;
} else {
memory[p] = 255; // Overflow back to 255
}
break;
case data[j] === ".": // put character to screen
var memoryChar = String.fromCharCode(memory[p]);
if(memoryChar == 'n') {
memoryChar = ""; // Turn newlines into linebreaks
}
$('p#output').append(String.fromCharCode(memory[p]));
break;
case data[j] === ",":
memory[p] = window.prompt("Please enter one character").charCodeAt(0); // Set memory to the character code
break;
case data[j] === "[": // start loop
if(memory[p] != 0) {
continue;
} else {
var openLoopCnt = 0; // number of open loops
for(var k = j; k < data.length; k++) {
if(data[k] === '[') {
openLoopCnt++; // Another open loop is in existence
} else if(data[k] === ']') {
openLoopCnt--; // Decrement open count
if(openLoopCnt === 0) {
j = k;
break;
}
}
}
if(openLoopCnt != 0) {
throwException("Open loop.", j);
}
}
break;
case data[j] === "]": // end loop
if(memory[p] != 0) {
var closedLoopCnt = 0; // Due to the fact that we are at closing loops, we use closedLoopCnt
for(var l = j; l >= 0; l--) {
if(data[l] === "]") {
closedLoopCnt++;
} else if(data[l] === "[") {
closedLoopCnt--;
}
if(closedLoopCnt === 0) {
j = l;
break;
}
}
if(closedLoopCnt != 0) {
throwException("Too many loop closings.", i);
}
}
break;
}
}
console.timeEnd("Running Brainf*** code");
console.time("Saving output in localStorage");
localStorage.setItem("brainfuck_output", $('#output').text());
console.timeEnd("Saving output in localStorage");
}<script src="https://ajax.googleapis.com/ajax/libs/jquery/1.11.1/jquery.min.js"></script>
<h1>Interpret your Brainf*** code!</h1>
<form action="#" method="get" id="brainf***" onsubmit="runBrainfuck($('#code').val())">
<label for="brainf***">Brainf*** code:</label> <br />
<textarea name="code" rows="25" cols="70" id="code"></textarea> <br />
<input type="submit" value="Interpret Brainf*** code!" />
</form>
<h2>Brainf*** output:</h2>
<p id="output"></p>I will accept any review about cleanliness and other tips for this. I also am wondering if there is a [great] way to rewrite the JavaScript part using jQuery’s window.jQuery.submit() function.
Solution
Nice code! In this review, I’ll focus on simplification, refactoring, emphasizing OOP, and overall making your code easier to unit test.
You must you proper exception throwing; (you have) no exceptions!
This is an improper way to throw errors:
throwException("There are only 30000 usable cells of memory.", j); // are you using too many cells?
This makes no actual indication to the JavaScript environment that there was any error; it only notifies the user. However, you want to have this function throw errors so you can more easily debug and unit test it.
However, you can just have it start throwing exceptions; there’s nothing to catch them. So, instead, you’ll have to have some other JavaScript code call this function when the event is fired. Here’s what that’d look like:
document.getElementById("brainf***").onsubmit = function() {
try {
runBrainfuck(this.code.value);
} catch {
// ... show user error...
}
}
Now, your runBrainfuck function can simply throw errors/exceptions (custom or not) and this will catch them and display them to the user however you like.
switching true off, and finding a better light switch
This is basically what your switch statement is:
switch(true) {
case a === b:
case a === c:
case a === d:
...
}
That seems rather abusive of a switch statement. Why not write it as it should be written?
switch(a) {
case b:
case c:
case d:
...
}
And, on the note about switches, here’s an alternate solution: create a map mapping each character to it’s functionality. Here’s what I mean:
var commandMap = {
">": function() {...
"<": function() {...
...
}
Then, rather than having a long switch statement, you can now use this O(1)
look-up table:
if(commandMap.hasOwnProperty(data[j])) {
commandMap[data[j]]();
}
Gettin’ some objects OOP in this place
Looking at the scope of your runBrainfuck, there are a lot of “global” variables. Either way, it’d still be nice to refactor some code to keep things clean.
Here’s something you could do:
function BrainfuckMachine(tapeSize) {
this.tape = Array.apply(null, Array(tapeSize)).map(function(a){ return a; });
this.p = 0;
}
With this class, you can now keep the data separate from your main function. In fact, you could even move that function to be a method of this new class:
BrainfuckMachine.prototype.runCode = function(code) {
Now your code is more OO and cleaner. Note that with this new class, you now need to pass instances of it to the functions defined in the commandMap so they can access and modify the data. This also allows for easier unit testing.
Setting the tape
A slightly cleaner, more JavaScript-y way to initialize an Array with N copies of a value would be to do this:
Array.apply(null, Array(n)).map(function(a){ return a; });
This would make creating the tape a bit cleaner.
Misc.
- You are redefining
datain your main function. Don’t – it’s good as a parameter (this allows for unit-testing). - Those
console.timecalls are really distracting. - 30000 is a magic number. Don’t use magic numbers. Define a constant at the top of your code.
I’d like to point out two possible enhancements:
Separation from the run-time environment
Currently you code is tightly connected to running inside a browser using jQuery. Since BF has nothing to do with browsers it would make sense to implement it in a way, that it can run in a different environment, say for example Node.
This could be done by providing a object that implements the interaction with the environment. e.g.:
var jQueryInterface = {
input: function() {
return window.prompt("Please enter one character").charCodeAt(0);
},
output: function(charCode) {
$('p#output').append(String.fromCharCode(charCode));
}
};
runBrainfuck(data, jQueryInterface);
function runBrainfuck(data, bfIinterface) {
// ...
case data[j] === ".": // put character to screen
var memoryChar = String.fromCharCode(memory[p]);
if(memoryChar == 'n') {
memoryChar = ""; // Turn newlines into linebreaks
}
bfIinterface.output(memory[p]);
break;
case data[j] === ",":
memory[p] = bfIinterface.input();
break;
// ...
}
Utilize JavaScript’s sparse arrays
In JavaScript you don’t need to initialize an array to a fixed length and fill it. Instead when reading from the memory array, when encountering an undefined value, just return 0. Replace all places where your read from the memory with memory[p] with a call to a function such as:
function getMemory(p) {
return memory[p] || 0; // Both "undefined" and "0" are considered "false". In that case "0" is returned.
}
Your support for looping is completely broken and inefficient — in a language where the only tool for controlling program flow is looping!
This line is the culprit:
openLoopCnt++; // Decrement open count
After fixing that silly bug, you should look into rewriting the interpreter to build a jump table once at the start of execution (like this), so that you don’t have to re-scan the code to find the matching delimiter every time you encounter a bracket.