Interview practice: Integer to Roman

Question

Student here. I'm practicing for coding interviews right now. In that context, how would you recommend I improve my code for this practice problem?

import java.util.*;

class Solution {
    private static final int maxMagnitude = 3;

    public String intToRoman(int num) {
        assert 1 <= num && num <= 3999 : num;

        StringBuilder sb = new StringBuilder();

        int divisor = pow(10, maxMagnitude);
        for (int i = maxMagnitude; i >= 0; i--) {
            Magnitude syms = Magnitude.getByExponent(i);
            sb.append(makeRomanNumeralDigit(num / divisor, syms.ONE_UNIT_SYMBOL, syms.FIVE_UNITS_SYMBOL, syms.TEN_UNITS_SYMBOL));
            num %= divisor;
            divisor = Math.max(divisor / 10, 1);
        }

        return sb.toString();
    }

    private static String makeRomanNumeralDigit(int val, char symOneUnit, char symFiveUnits, char symTenUnits) {
        assert 0 <= val && val <= 10 : val;

        StringBuilder sb = new StringBuilder();
        switch (val) {
        case 0:
            break;
        case 1:
            sb.append(symOneUnit);
            break;
        case 2:
            sb.append(symOneUnit).append(symOneUnit);
            break;
        case 3:
            sb.append(symOneUnit).append(symOneUnit).append(symOneUnit);
            break;
        case 4:
            sb.append(symOneUnit);
            sb.append(symFiveUnits);
            break;
        case 5:
            sb.append(symFiveUnits);
            break;
        case 6:
            sb.append(symFiveUnits);
            sb.append(symOneUnit);
            break;
        case 7:
            sb.append(symFiveUnits);
            sb.append(symOneUnit).append(symOneUnit);
            break;
        case 8:
            sb.append(symFiveUnits);
            sb.append(symOneUnit).append(symOneUnit).append(symOneUnit);
            break;
        case 9:
            sb.append(symOneUnit);
            sb.append(symTenUnits);
            break;
        case 10:
            sb.append(symTenUnits);
            break;
        }
        return sb.toString();
    }

    private static enum RomanNumeral {
        ONE ('I'),
        FIVE ('V'),
        TEN ('X'),
        FIFTY ('L'),
        ONE_HUNDRED ('C'),
        FIVE_HUNDRED ('D'),
        ONE_THOUSAND ('M');

        public final char SYMBOL;

        private RomanNumeral(char symbol) {
            this.SYMBOL = symbol;
        }
    }

    private static enum Magnitude {
        ONES (RomanNumeral.ONE.SYMBOL, RomanNumeral.FIVE.SYMBOL, RomanNumeral.TEN.SYMBOL),
        TENS (RomanNumeral.TEN.SYMBOL, RomanNumeral.FIFTY.SYMBOL, RomanNumeral.ONE_HUNDRED.SYMBOL),
        HUNDREDS (RomanNumeral.ONE_HUNDRED.SYMBOL, RomanNumeral.FIVE_HUNDRED.SYMBOL, RomanNumeral.ONE_THOUSAND.SYMBOL),
        THOUSANDS (RomanNumeral.ONE_THOUSAND.SYMBOL, '\0', '\0');

        public final char ONE_UNIT_SYMBOL;
        public final char FIVE_UNITS_SYMBOL;
        public final char TEN_UNITS_SYMBOL;

        private Magnitude(char oneUnitSym, char fiveUnitsSym, char tenUnitsSym) {
            this.ONE_UNIT_SYMBOL = oneUnitSym;
            this.FIVE_UNITS_SYMBOL = fiveUnitsSym;
            this.TEN_UNITS_SYMBOL = tenUnitsSym;
        }

        public static Magnitude getByExponent(int exponent) {
            assert 0 <= exponent && exponent <= 3;

            Magnitude magnitude = null;
            switch (exponent) {
            case 0:
                magnitude = Magnitude.ONES;
                break;
            case 1:
                magnitude =  Magnitude.TENS;
                break;
            case 2:
                magnitude =  Magnitude.HUNDREDS;
                break;
            case 3:
                magnitude =  Magnitude.THOUSANDS;
                break;
            }
            return magnitude;
        }
    }

    private static int pow(int base, int exponent) {
        assert base > 0 : base;
        assert exponent >= 0 : exponent;

        if (exponent == 0)
            return 1;

        int result = pow(base, exponent / 2);
        result *= result;
        if (exponent % 2 != 0)
            result *= base;
        return result;
    }
}

Answer 1

Way too much code indeed. Java is verbose, but it doesn't have to be this verbose. There are also too many magic numbers. Code should be self-documenting and yours isn't. The variable naming is iffy at best and the case switch should've been a map, or at least an array, of sorts.

You have created a Magnitude, but it's unclear what you mean with this. Magnitude is about relative size. That's good. Roman numerals are a relative system. But your enum is a mess and extending it with getByExponent only makes it worse.

I'm going to be harsh here, but if you'd present this in an interview, you would not get the job. That's ok, you simply weren't familiar with the in-built constructs of the language. But those exist for a reason and you should show at an interview you grasp their basics. That's one of the things an interview-question asks of you: to show you know how to get something simple small done using the tools the language already provides for you. Don't re-invent the wheel unless specifically asked to.

Now you've tackled the problem, consider looking at how other people have solved this problem. Their approaches and the answers provided can teach you alternative methods to familiarize yourself with.

Answer 2

In additional to the others:

You should follow the Java naming guidelines, specifically public final char ONE_UNIT_SYMBOL; should be public final char oneUnitSymbol;. And you shouldn't make these fields public.

Using assert is a nice idea, but not really helpful, as in production code the feature is often switched off. You'd better throw an IllegalArgumentException for invalid integers.

As others already pointed out, your solution is over-engineered, and that's done in an inconsistent way.

E.g. your idea of a Magnitude class isn't bad, but then it should include the relevant operations instead of just being a structure offering public fields to the outside world. And there's no need for the symTenUnits and the case 10, as a digit can only range from 0 to 9.

I don't like your RomanNumeral enum. It doesn't serve a real purpose, as the Magnitude enum already defines the existing cases. Passing the characters to the Magnitude constructor is sufficient.

I don't like your solution for the missing 5000 roman numeral. Just using a NUL character will go completely unnoticed in case that something goes wrong. Having the Magnitude class throw an exception when trying to use the non-existent 5000 numeral (or at least using a visible character like a question mark) would be better.

So, as an interviewer, I wouldn't give you a job as a Java developer:

• You don't follow common conventions.
• You create an inconsistent, overly complicated architecture.
• Your code doesn't reliably inform its callers about failure.

Answer 3

For a coding interview, that's too much code.

You're asserted that your input is in the range [1,3999], so you should be able to do this in a single function, with not much in the way of looping.

Instead of being overly general, try coding specific conversions for the various digits, using a lookup for the actual digit values.

That is: if thousands, append thousands; if hundreds, append hundreds; etc.

Answer 4

In Addition to others. Why I wouldn't give you a job, if you were a senior:

• code convetions not applied
• using assert
• complicated solution

Basically that means, you're very inexperienced in java itself and you can't provide an easy and well written solution for a rather easy problem.

Why I wouldn't give you the job, if you were a junior with a degree (applies to senior, too):

• no test case
• bad naming
• write your own pow() method, instead of using Math.pow()

Not writing a test case is a huge part. Because, as a software developer, I say, that test code is as valuable and as important as production code. The other stuff can mean, that you're very inexperienced, never worked with other people on a project, or never did a code review. And you do not know the basics of the SDK.

Other tips:

• As a student and as a soon-to-be-developer: You have to go the extra mile. From my experience with my students or working mates who just started to work as a developer: What they learn during their education is, honestly, not enough. It's much, but not enough. When I did my education 15 years ago, what I learned was enough at that time, to do a okayish-decent job. But stuff got really complicated and projects get larger and larger. So, it's much harder for the younglings today, to do a decent job. Of course, you guys are still learning, but if you want to have a good job directly after your education (doesn't imply money, but creativity, challenge and commitment), the employer has to think "That dude, I want to invite him", when he sees your resumee. And not "oh, no work experience, did not do a single project of his own". Because to hire an inexperienced developer is a huge commitment of the employer. We say, in our project, an experienced developer needs a year to be able to work more or less independently. Double that for an inexperienced developer.
• Read the Clean Code Book (Robert C. Martin)
• Honestly, read all the books.
• Learn to write test cases. (Read "Growing object oriented software, guided by tests" by freeman)
• Make a github account, write your own software. Usable software.
• Make a book shelf, dedicated for technical books.
• Contribute to other software

Hope this helps,

slowy