Assembler for Hack Assembly Language - Take 2

Question

I have been reading this great book and I decided to do the Project in Chapter 5: Writing an Assembler for the Hack Assembly Language.

You can find the specification for the Hack Machine Language and the Hack Assembly Language here.

I have a working implementation here already. That one took me about 2 - 3 hours. Then I wanted to code following the best practices. This time it took me 5 - 6 hours. And I am doubtful which one is more readable. But any review is welcome in this version as well. My goal was to de-couple everything as much as I could, code to Interfaces instead of Concrete classes.

I must say that both implementations do the same thing, but this one is like 5 - 6 times longer I guess. Maybe even more.. And it is full of classes, packages, interfaces etc..

Is this really following the best practices? Do you see any dependency problems? Should I have more classes? More modularitiy?

package biz.tugay.hack.assembler.assemble;
/* User: [email protected] Date: 10/03/15 Time: 15:56 */

import java.io.IOException;

public interface InstructionFileAssembler {
    public void assembleFile() throws IOException;
}

package biz.tugay.hack.assembler.assemble;
/* User: [email protected] Date: 09/03/15 Time: 22:20 */

import biz.tugay.hack.assembler.helperutils.commentRemover.InstructionCommentRemover;
import biz.tugay.hack.assembler.instruction.Instruction;
import biz.tugay.hack.assembler.instruction.factory.InstructionFactory;
import biz.tugay.hack.assembler.instruction.instructionConverter.InstructionToBinaryConverter;

import java.io.IOException;
import java.io.InputStream;
import java.util.List;
import java.util.Scanner;

public class InstructionFileAssemblerImpl implements InstructionFileAssembler {

    private InstructionCommentRemover instructionCommentRemover;
    private InputStream inputStream;
    private InstructionFactory instructionFactory;
    private List<InstructionToBinaryConverter> instructionToBinaryConverters;

    public InstructionFileAssemblerImpl(InstructionCommentRemover instructionCommentRemover,
                                        InputStream inputStream,
                                        InstructionFactory instructionFactory,
                                        List<InstructionToBinaryConverter> instructionToBinaryConverters) {
        this.instructionCommentRemover = instructionCommentRemover;
        this.inputStream = inputStream;
        this.instructionToBinaryConverters = instructionToBinaryConverters;
        this.instructionFactory = instructionFactory;
    }

    @Override
    public void assembleFile() throws IOException {
        Scanner scanner = new Scanner(inputStream);
        while (scanner.hasNextLine()) {
            String convert = convert(scanner.nextLine());
            if(convert != null && convert.length() != 0) {
                System.out.println(convert);
            }
        }
    }

    private String convert(String instructionToConvert) {

        // Obtain the next instruction from the input stream.
        // If the instruction contains comments, remove them and trim the instruction
        if (instructionToConvert.contains(instructionCommentRemover.getCommentRule())) {
            instructionToConvert = instructionCommentRemover.removeCommentFromSingleInstruction(instructionToConvert);
        }

        // Build the instruction from using the provided factory.
        Instruction instruction = instructionFactory.buildInstruction(instructionToConvert);

        // If we have a valid instruction..
        if (instruction != null && instruction.getMnemonic() != null && instruction.getMnemonic().length() != 0) {
            return doConvert(instruction);
        }
        return null;
    }

    private String doConvert(Instruction instruction) {
        for (InstructionToBinaryConverter instructionToBinaryConverter : instructionToBinaryConverters)
            if (instruction.getInstructionType().equals(instructionToBinaryConverter.getInstructionType()))
                return instructionToBinaryConverter.getBinaryRepresentationForInstruction(instruction);
        throw new UnsupportedOperationException();
    }

}

package biz.tugay.hack.assembler.helperutils.commentRemover;
/* User: [email protected] Date: 10/03/15 Time: 14:47 */

public interface InstructionCommentRemover {

    /**
     * Removes the comment part of a single line instuction.
     * The implementing class must hardcode the comment rule.
     * @param instruction : Single instruction line.
     * @return Returns the instruction without the comment part.
     */

    public String removeCommentFromSingleInstruction(String instruction);
    public String getCommentRule();
}

package biz.tugay.hack.assembler.helperutils.commentRemover;
/* User: [email protected] Date: 10/03/15 Time: 14:47 */

public class InstructionRemoverImpl implements InstructionCommentRemover {

    private String commentRule = "//";

    @Override
    public String removeCommentFromSingleInstruction(String instruction) {
        if (instruction.contains(commentRule)) {
            instruction = instruction.split(commentRule)[0];
        }
        return instruction.trim();
    }

    @Override
    public String getCommentRule() {
        return commentRule;
    }

}

package biz.tugay.hack.assembler.helperutils;
/* User: [email protected] Date: 09/03/15 Time: 20:25 */

public class BinaryFormatter {

    public static String appendZerosToString(String stringToBeAppended) {
        char[] chars = stringToBeAppended.toCharArray();
        int zerosToAppend = 16 - chars.length;
        StringBuilder stringBuilder = new StringBuilder();
        for(int i=0;i<zerosToAppend;i++) {
            stringBuilder.append("0");
        }
        stringBuilder.append(stringToBeAppended);
        return stringBuilder.toString();
    }

}

package biz.tugay.hack.assembler.instruction.factory;

import biz.tugay.hack.assembler.instruction.HackInstructionTypeA;
import biz.tugay.hack.assembler.instruction.HackInstructionTypeC;
import biz.tugay.hack.assembler.instruction.Instruction;

/* User: [email protected] Date: 09/03/15 Time: 22:23 */
public class HackInstructionFactory implements InstructionFactory {
    @Override
    public Instruction buildInstruction(String instruction) {
        Instruction hackInstruction;
        if(instruction.startsWith("(")) {
            return null;
        }
        if(instruction.startsWith("@")) {
            hackInstruction = new HackInstructionTypeA(instruction);
        }
        else {
            hackInstruction = new HackInstructionTypeC(instruction);
        }
        return hackInstruction;
    }
}

package biz.tugay.hack.assembler.instruction.factory;
/* User: [email protected] Date: 10/03/15 Time: 15:52 */

import biz.tugay.hack.assembler.instruction.Instruction;

/**
 * An Instruction Factory implementation should return a concrete and correct @Instruction object
 */
public interface InstructionFactory {
    /**
     * The implementing method must return one of the Instructions that the
     * Assembly Language implements.
     * @param instruction The code to be used to find the matching Instruction object and creating it.
     * @return Instruction object.
     */
    Instruction buildInstruction(String instruction);
}

package biz.tugay.hack.assembler.instruction.instructionConverter;
/* User: [email protected] Date: 09/03/15 Time: 21:10 */

import biz.tugay.hack.assembler.instruction.Instruction;
import biz.tugay.hack.assembler.instruction.InstructionType;

import java.util.Map;

public interface InstructionToBinaryConverter {

    public String getBinaryRepresentationForInstruction(Instruction instruction);
    public InstructionType getInstructionType();
    public void addToLookUpMap(Map<String, Integer> map);

}


package biz.tugay.hack.assembler.instruction.instructionConverter;
/* User: [email protected] Date: 09/03/15 Time: 21:27 */

import biz.tugay.hack.assembler.helperutils.BinaryFormatter;
import biz.tugay.hack.assembler.instruction.Instruction;
import biz.tugay.hack.assembler.instruction.InstructionType;

import java.util.HashMap;
import java.util.Map;

public class InstructionToBinaryConverterInstructionAImpl implements InstructionToBinaryConverter {

    private int memoryLocationCounter = 16;
    private Map<String,Integer> lookUpMap = new HashMap<String, Integer>();
    private InstructionType instructionType = InstructionType.A_INSTRUCTION;

    {
        lookUpMap.put("SP"       ,0);
        lookUpMap.put("LCL"      ,1);
        lookUpMap.put("ARG"      ,2);
        lookUpMap.put("THIS"     ,3);
        lookUpMap.put("THAT"     ,4);
        lookUpMap.put("R0"       ,0);
        lookUpMap.put("R1"       ,1);
        lookUpMap.put("R2"       ,2);
        lookUpMap.put("R3"       ,3);
        lookUpMap.put("R4"       ,4);
        lookUpMap.put("R5"       ,5);
        lookUpMap.put("R6"       ,6);
        lookUpMap.put("R7"       ,7);
        lookUpMap.put("R8"       ,8);
        lookUpMap.put("R9"       ,9);
        lookUpMap.put("R10"      ,10);
        lookUpMap.put("R11"      ,11);
        lookUpMap.put("R12"      ,12);
        lookUpMap.put("R13"      ,13);
        lookUpMap.put("R14"      ,14);
        lookUpMap.put("R15"      ,15);
        lookUpMap.put("SCREEN"   ,16384);
        lookUpMap.put("KBD"      ,24576);
    }

    @Override
    public String getBinaryRepresentationForInstruction(Instruction instruction) {

        String mnemonic = instruction.getMnemonic();
        String symbol   = mnemonic.substring(1);

        boolean isDirectAddress = true;

        try {
            Integer.parseInt(symbol);
        } catch (NumberFormatException e) {
            isDirectAddress = false;
        }

        if(isDirectAddress) {
            return BinaryFormatter.appendZerosToString(Integer.toBinaryString(Integer.valueOf(symbol)));
        }

        if(mnemonic.startsWith("@")) {
            if(lookUpMap.containsKey(symbol)) {
                return BinaryFormatter.appendZerosToString(Integer.toBinaryString(lookUpMap.get(symbol)));
            }
            if(!lookUpMap.containsKey(symbol)) {
                lookUpMap.put(symbol,memoryLocationCounter);
                memoryLocationCounter++;
            }
            if(lookUpMap.containsKey(symbol)) {
                return BinaryFormatter.appendZerosToString(Integer.toBinaryString(lookUpMap.get(symbol)));
            }
        }

        throw new UnsupportedOperationException();

    }

    public InstructionType getInstructionType() {
        return instructionType;
    }

    @Override
    public void addToLookUpMap(Map<String, Integer> map) {
        lookUpMap.putAll(map);
    }

}

package biz.tugay.hack.assembler.instruction.instructionConverter;
/* User: [email protected] Date: 09/03/15 Time: 22:10 */

import biz.tugay.hack.assembler.instruction.InstructionType;
import biz.tugay.hack.assembler.instruction.Instruction;

import java.util.HashMap;
import java.util.Map;

public class InstructionToBinaryConverterInstructionCImpl implements InstructionToBinaryConverter {

    private Map<String,String> lookUpMap = new HashMap<String, String>();
    private Map<String,String> compMap = new HashMap<String, String>();

    @Override
    public String getBinaryRepresentationForInstruction(Instruction instruction) {

        final String mnemonic = instruction.getMnemonic();

        // We definetly have compBinary in the instruction...
        String compBinaryRepresentation         = getBinaryRepresentationForCompMnemnonic(mnemonic);

        String jumpMnemonic                     = "null";
        String jumpBinaryRepresentation;

        if (mnemonic.contains(";")) {
            jumpMnemonic = mnemonic.split(";")[mnemonic.split(";").length-1];
        }

        jumpBinaryRepresentation = lookUpMap.get(jumpMnemonic);

        String destMnemonic                     = "null";
        String destinationBinaryRepresentation;

        if(mnemonic.contains("=")) {
            destMnemonic = mnemonic.split("=")[0];
        }

        destinationBinaryRepresentation = lookUpMap.get(destMnemonic);

        return "111" + compBinaryRepresentation + destinationBinaryRepresentation + jumpBinaryRepresentation;

    }

    @Override
    public InstructionType getInstructionType() {
        return InstructionType.C_INSTRUCTION;
    }

    private String getBinaryRepresentationForCompMnemnonic(String mnemonic) {
        String compMnemonnic  = mnemonic.split(";")[0];
        compMnemonnic = compMnemonnic.split("=")[compMnemonnic.split("=").length-1];
        return compMap.get(compMnemonnic);
    }

    {
        compMap.put("0"  ,"0101010");
        compMap.put("1"  ,"0111111");
        compMap.put("-1" ,"0111010");
        compMap.put("D"  ,"0001100");
        compMap.put("A"  ,"0110000");
        compMap.put("!D" ,"0001101");
        compMap.put("!A" ,"0110001");
        compMap.put("-D" ,"0001111");
        compMap.put("-A" ,"0110011");
        compMap.put("D+1","0011111");
        compMap.put("A+1","0110111");
        compMap.put("D-1","0001110");
        compMap.put("A-1","0110010");
        compMap.put("D+A","0000010");
        compMap.put("D-A","0010011");
        compMap.put("A-D","0000111");
        compMap.put("D&A","0000000");
        compMap.put("D|A","0010101");
        compMap.put("M"  ,"1110000");
        compMap.put("!M" ,"1110001");
        compMap.put("-M" ,"1110011");
        compMap.put("M+1","1110111");
        compMap.put("M-1","1110010");
        compMap.put("D+M","1000010");
        compMap.put("D-M","1010011");
        compMap.put("M-D","1000111");
        compMap.put("D&M","1000000");
        compMap.put("D|M","1010101");
    }

    {
        lookUpMap.put("null","000");
        lookUpMap.put("JGT" ,"001");
        lookUpMap.put("JEQ" ,"010");
        lookUpMap.put("JGE" ,"011");
        lookUpMap.put("JLT" ,"100");
        lookUpMap.put("JNE" ,"101");
        lookUpMap.put("JLE" ,"110");
        lookUpMap.put("JMP" ,"111");
    }

    {
        lookUpMap.put("null","000");
        lookUpMap.put("M"   ,"001");
        lookUpMap.put("D"   ,"010");
        lookUpMap.put("MD"  ,"011");
        lookUpMap.put("A"   ,"100");
        lookUpMap.put("AM"  ,"101");
        lookUpMap.put("AD"  ,"110");
        lookUpMap.put("AMD" ,"111");
    }

    @Override
    public void addToLookUpMap(Map<String, Integer> map) {
        throw new UnsupportedOperationException("You can not mess with this instruction type!");
    }

}

package biz.tugay.hack.assembler.instruction;
/* User: [email protected] Date: 09/03/15 Time: 21:01 */

public class HackInstructionTypeA implements Instruction {

    private String mnemonic;

    public HackInstructionTypeA(String instruction) {
        this.mnemonic = instruction;
    }

    @Override
    public InstructionType getInstructionType() {
        return InstructionType.A_INSTRUCTION;
    }

    @Override
    public String getMnemonic() {
        return mnemonic;
    }

}

package biz.tugay.hack.assembler.instruction;
/* User: [email protected] Date: 09/03/15 Time: 22:08 */

public class HackInstructionTypeC implements Instruction {

    private String mnemonic;

    public HackInstructionTypeC(String instruction) {
        this.mnemonic = instruction;
    }

    @Override
    public InstructionType getInstructionType() {
        return InstructionType.C_INSTRUCTION;
    }

    @Override
    public String getMnemonic() {
        return mnemonic;
    }
}

package biz.tugay.hack.assembler.instruction;
/* User: [email protected] Date: 10/03/15 Time: 15:51 */

public interface Instruction {
    InstructionType getInstructionType();
    String getMnemonic();
}


package biz.tugay.hack.assembler.instruction;
/* User: [email protected] Date: 09/03/15 Time: 20:52 */

public enum InstructionType {
    A_INSTRUCTION,C_INSTRUCTION;
}


package biz.tugay.hack.assembler.label;
/* User: [email protected] Date: 10/03/15 Time: 14:45 */

import java.util.Map;

public interface LabelMapBuilder {
    public Map<String, Integer> buildLabelMapForGivenFile();
}


package biz.tugay.hack.assembler.label;
/* User: [email protected] Date: 09/03/15 Time: 22:36 */

import biz.tugay.hack.assembler.helperutils.commentRemover.InstructionCommentRemover;

import java.io.InputStream;
import java.util.HashMap;
import java.util.Map;
import java.util.Scanner;

public class LabelMapBuilderImpl implements LabelMapBuilder {

    private Character labelStartCharacter;
    private InstructionCommentRemover instructionCommentRemover;
    private InputStream inputStream;

    public LabelMapBuilderImpl(InputStream inputStream,
                               InstructionCommentRemover instructionCommentRemover,
                               Character labelStartCharacter) {
        this.inputStream = inputStream;
        this.instructionCommentRemover = instructionCommentRemover;
        this.labelStartCharacter = labelStartCharacter;
    }

    @Override
    public Map<String, Integer> buildLabelMapForGivenFile() {
        int lineCounter = 0;
        Map<String, Integer> labelMap = new HashMap<String, Integer>();
        Scanner scanner = new Scanner(inputStream);
        while (scanner.hasNextLine()) {
            String instruction = scanner.nextLine();
            if(instruction.contains(instructionCommentRemover.getCommentRule())) {
                instruction = instructionCommentRemover.removeCommentFromSingleInstruction(instruction);
            }
            if (isInstructionLabel(instruction)) {
                addInstructionToLabelMap(lineCounter, labelMap, instruction);
                lineCounter--;
            }
            lineCounter++;
        }
        return labelMap;
    }

    private void addInstructionToLabelMap(int lineCounter, Map<String, Integer> labelMap, String instructionWithoutComment) {
        labelMap.put(instructionWithoutComment.substring(1, instructionWithoutComment.length() - 1), lineCounter);
    }

    private boolean isInstructionLabel(String instruction) {
        return instruction.startsWith(Character.toString(labelStartCharacter));
    }

}

and finally the Test Class:

package biz.tugay.hack.assembler;
/* User: [email protected] Date: 10/03/15 Time: 16:15 */

import biz.tugay.hack.assembler.assemble.InstructionFileAssembler;
import biz.tugay.hack.assembler.assemble.InstructionFileAssemblerImpl;
import biz.tugay.hack.assembler.helperutils.commentRemover.InstructionCommentRemover;
import biz.tugay.hack.assembler.helperutils.commentRemover.InstructionRemoverImpl;
import biz.tugay.hack.assembler.instruction.factory.HackInstructionFactory;
import biz.tugay.hack.assembler.instruction.factory.InstructionFactory;
import biz.tugay.hack.assembler.instruction.instructionConverter.InstructionToBinaryConverter;
import biz.tugay.hack.assembler.instruction.instructionConverter.InstructionToBinaryConverterInstructionAImpl;
import biz.tugay.hack.assembler.instruction.instructionConverter.InstructionToBinaryConverterInstructionCImpl;
import biz.tugay.hack.assembler.label.LabelMapBuilder;
import biz.tugay.hack.assembler.label.LabelMapBuilderImpl;

import java.io.*;
import java.util.ArrayList;
import java.util.List;
import java.util.Map;

public class TestClass {

    public static void main(String[] args) throws IOException {

        InputStream inputStream = new FileInputStream(new File(args[0]));
        InstructionCommentRemover instructionCommentRemover = new InstructionRemoverImpl();
        LabelMapBuilder labelMapBuilder = new LabelMapBuilderImpl(inputStream,instructionCommentRemover,'(');
        Map<String, Integer> labelMap = labelMapBuilder.buildLabelMapForGivenFile();

        InstructionToBinaryConverter instructionToBinaryConverterInstructionA = new InstructionToBinaryConverterInstructionAImpl();
        instructionToBinaryConverterInstructionA.addToLookUpMap(labelMap);

        InstructionToBinaryConverter instructionToBinaryConverterInstructionC = new InstructionToBinaryConverterInstructionCImpl();

        List<InstructionToBinaryConverter> instructionToBinaryConverterList = new ArrayList<InstructionToBinaryConverter>();
        instructionToBinaryConverterList.add(instructionToBinaryConverterInstructionA);
        instructionToBinaryConverterList.add(instructionToBinaryConverterInstructionC);

        InstructionFactory factory = new HackInstructionFactory();
        inputStream = new FileInputStream(new File(args[0]));

        InstructionFileAssembler instructionFileAssembler =
                new InstructionFileAssemblerImpl(instructionCommentRemover,inputStream,factory,instructionToBinaryConverterList);

        instructionFileAssembler.assembleFile();


    }

}

Answer 1

To point out the obvious, layers of indirection (interfaces, strategy classes, etc.) are only "better" if you stand to benefit from the flexibility they offer. Otherwise, it is overengineering.

But let us assume that your assembler will target two different CPU architectures. Is your code well-architected to do that? Well, I cannot answer that because I don't know what the 2nd architecture looks like. I don't know how it differs from the 1st and what parts of the code need to be flexible to support it. In the worst case, it may have to directly implement InstructionFileAssembler, in which case all of the other extensibility points you've created prematurely will actually go unused. The real goal of a programmer in a similar situation of uncertainty is to write code that is easy to make abstract later but is simple and elegant for the time being. Imagine the possible design patterns, but don't actually implement them.

Enough of me being difficult, though.

• Kill InstructionFileAssembler and every other interface that is implemented only once. (But keep the public methods of your classes general, as if they implement an interface.)
• Call InstructionFileAssembler simply Assembler. All assemblers work on files of instructions. Don't be captain obvious. Names must be concise.
• InstructionCommentRemover should be called a Preprocessor, which might in the future do things beside removing comments. When that happens, the preprocessor might call a class CommentRemover. There is no such thing as an "instruction comment."
• String convert = convert(scanner.nextLine()); should look more like String machineCode = assemble (scanner.nextLine());. Variable names should be nouns. Method names should be verbs that are specific.
• instructionToConvert should be instruction, unless you have some other instruction in your code block.
• if (instructionToConvert.contains(instructionCommentRemover.getCommentRule())) instructionToConvert = instructionCommentRemover.removeCommentFromSingleInstruction(instructionToConvert); is a code smell for several reasons. At worst, it should be if (commentRemover.containsComment (instruction)) instruction = commentRemover.process (instruction). Better would be simply instruction = commentRemover.process (instruction). But the best would be if you let the preprocessor do its work on the entire assembly file before passing it to the assembler.
• An Instruction should know how to encode itself (and expose toMachineCode()). That is the aim of OOP, after all. Internally, it can call a class Architecture with a method encode(Instruction).
• I would replace InstructionFactory.buildInstruction with Instruction.valueOf, but you can also have an InstructionParser.parse
• HackInstructionFactory should not return null or malformed instructions which can't be encoded. Take a look at the null object pattern.
• The class which encodes the instruction is complicated and I'm sure a lot can be done inside it, but I won't be able to get to it.

Answer 2

• Your lines are very long. There are good reasons to stick to an 80 char limit.
• I like your use of interfaces. Interfaces can be very helpful.
• You seem to be on the right track with making your code modular. Most of the classes are short and concise.

• There are few comments detailing what the methods do. BinaryFormatter for example, needs little or no explanation. InstructionToBinaryConverterInstructionAImpl. getBinaryRepresentationForInstruction can use a little explanation. The extra long name describes the expected outcome of using the method, but it contains no mention of how it is accomplishing the task. It seems you can get away with something like

  // Converts an instruction into binary form; depending on
  // instruction type. If instruction is a symbol, and the symbol is not
  // known, adds symbol to the local HashMap field lookUpMap.
  @Override
  public String representInBinary(Instruction instruction) { }
  

• This is a lot of files. I spent a good ten minutes organizing each file into my IDE. I think next time you ask for a code review you should keep the code much shorter; and post only the files you are concerned about.