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: koray@tugay.biz 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: koray@tugay.biz 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: koray@tugay.biz 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: koray@tugay.biz 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: koray@tugay.biz 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: koray@tugay.biz 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: koray@tugay.biz 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: koray@tugay.biz 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: koray@tugay.biz 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: koray@tugay.biz 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: koray@tugay.biz 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: koray@tugay.biz 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: koray@tugay.biz Date: 10/03/15 Time: 15:51 */public interface Instruction {    InstructionType getInstructionType();    String getMnemonic();}package biz.tugay.hack.assembler.instruction;/* User: koray@tugay.biz Date: 09/03/15 Time: 20:52 */public enum InstructionType {    A_INSTRUCTION,C_INSTRUCTION;}package biz.tugay.hack.assembler.label;/* User: koray@tugay.biz 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: koray@tugay.biz 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: koray@tugay.biz 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();    }}
Assembler for Hack Assembly Language - Take 2
java;object oriented;assembly
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.parseHackInstructionFactory 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.