6
\$\begingroup\$

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;
    }
}
\$\endgroup\$
1
  • \$\begingroup\$ Where are the tests? That’s the first thing I would look for as an interviewer. \$\endgroup\$
    – RubberDuck
    Dec 29, 2017 at 21:10

4 Answers 4

4
\$\begingroup\$

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 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.

\$\endgroup\$
2
  • \$\begingroup\$ Thank you. Which switch statement are you referring to? The one in makeRomanNumeralDigit or getByExponent? \$\endgroup\$
    – Travis
    Dec 28, 2017 at 19:53
  • \$\begingroup\$ @Travis The one in makeRomanNumeralDigit. \$\endgroup\$
    – Mast
    Dec 28, 2017 at 20:16
4
\$\begingroup\$

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.
\$\endgroup\$
3
  • \$\begingroup\$ Thank you. Three questions. (1) Should constant identifiers use camel case as well? I was under the impression that constants should use upper case. (2) Would assert be appropriate for private functions? (3) Also, would assert be appropriate for documenting guaranteed input properties in LeetCode practice problems, or should I really be using proper exceptions even then since the primary method is public? \$\endgroup\$
    – Travis
    Dec 28, 2017 at 19:51
  • \$\begingroup\$ Your ONE_UNIT_SYMBOL isn't a constant, it's an instance field having different values per instance. "Constant" means public static final. assert is good for debugging, but should be considered a no-op in production. To signal failure to your caller, throw an exception or let an exception you got simply pass through. Of course, if you can GUARANTEE that input parameters will ALWAYS meet your expectations, you don't need any kind of check. But Roman Numerals conversion looks like a library function to be called by foreigners, so check and throw! \$\endgroup\$ Dec 29, 2017 at 11:35
  • \$\begingroup\$ And one more thing I just saw in your code: Don't use wildcard imports. You'll get into problems if in some future Java library the java.util package gets a new class colliding with some name you already have. \$\endgroup\$ Dec 29, 2017 at 11:43
3
\$\begingroup\$

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.

\$\endgroup\$
5
  • \$\begingroup\$ “For a coding interview that’s too much code.” That’s not constructive feedback. I, for one, appreciate the attempt to factor the problem into generic pieces, even if it wasn’t done particularly well. This answer doesn’t help OP improve their code, it just kinda trashes on it. \$\endgroup\$
    – RubberDuck
    Dec 29, 2017 at 21:16
  • \$\begingroup\$ @RubberDuck I disagree. I wasn't trying to trash the code, but point out that for a live, in-person coding interview, that was too many lines of too-complex code. If someone asked me to write that on a whiteboard, or in a sandbox environment, I would expect that this was the first question of potentially many. Accordingly, I would try to get something written and debugged in a short time, so as to be ready for whatever came next. \$\endgroup\$
    – aghast
    Dec 29, 2017 at 21:26
  • \$\begingroup\$ And how does your answer help OP do that? \$\endgroup\$
    – RubberDuck
    Dec 29, 2017 at 22:04
  • \$\begingroup\$ By conveying that s/he has written too much code, and suggesting a direction to pursue that would involve less code and less complexity. \$\endgroup\$
    – aghast
    Dec 29, 2017 at 23:49
  • \$\begingroup\$ you can argue with me, or take the criticism & improve your answer. Your call, but I’m done responding. \$\endgroup\$
    – RubberDuck
    Dec 30, 2017 at 0:10
2
\$\begingroup\$

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

\$\endgroup\$
4
  • \$\begingroup\$ Downvoted for lack of detail about your objections. \$\endgroup\$ Dec 29, 2017 at 18:14
  • \$\begingroup\$ No downvote, but I disagree about assertions. Even when disabled, assertions are a programmatic documentation of constraints envisioned by the developer. They are like unit tests, in that they provide unambiguous documentation of the developer's vision. \$\endgroup\$
    – aghast
    Dec 29, 2017 at 21:32
  • \$\begingroup\$ @AustinHastings: Yeah, but when disabled, the application doesn't behave the same, if not implemented correctly. That's why there is a Validate.isTrue as part of the apache commons lib and a Preconditions.checkNotNull as part of the the guava lib. \$\endgroup\$
    – slowy
    Jan 2, 2018 at 16:11
  • \$\begingroup\$ @200_success: I do agree, my answer implies the other answers. It is from a interviewer perspective. \$\endgroup\$
    – slowy
    Jan 2, 2018 at 16:12

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.