Gfrktyl

This is a homework assignment. Convert an integer (1 - 2999) to roman numerals. I was wondering if I could make my code more efficient. The way I have it set up is that I take the digits of the input (the thousands, hundreds, tens, ones) and have cases for each of them. However, the approach I took to this problem is that there are a lot of if statements. My professor prefers that I utilize nothing that is not taught yet. I can only use for, if, while statements - the basics of Java. I was wondering if there are different approaches I could take to this problem so that the code is much more shorter as it is relatively long.

class romanNum {

    public static void main (String  args) {

        //Initialize all variables including Scanner

        Scanner input = new Scanner (System.in) ;

        int userInput;

        int onesDigit ;

        int tensDigit ;

        int hundredsDigit ;

        int thousandsDigit ;

        String one = "I";

        String five = "V";

        String ten = "X";

        String fifty = "L";

        String hundred = "C";

        String fiveHundred = "D";

        String thousand = "M";

        boolean keepGoing = true ;



        //User Input

        userInput = input.nextInt();



        //Seperate the digits 

        thousandsDigit = (userInput/1000) % 10;

        hundredsDigit = (userInput / 100) % 10;

        tensDigit = (userInput / 10) % 10;

        onesDigit = (userInput / 1) % 10;

        //Thousands (Because range is 1 - 2999, i didnt do more cases for thousands

        while (keepGoing) {

            for (int count = 0 ; count < thousandsDigit ; count++) {

                System.out.print (thousand);

            }



            //Hundreds (Checks all cases 1-9)

            //Checks 1 - 3

            if ((hundredsDigit >= 1) && (hundredsDigit <= 3))   {

                for (int count = 0 ; count < hundredsDigit ; count++) {

                    System.out.print (hundred);

                }

            }

            //Checks 4

            else if (hundredsDigit == 4) {

                System.out.print (hundred + fiveHundred); 

            }

            // Checks 9

            else if (hundredsDigit == 9) {

                System.out.print (hundred + thousand);

            }

            //Omit 0

            else if (hundredsDigit == 0) {

            }

            // Checks 5 - 8      

            else  {

                System.out.print (fiveHundred);

                for (int count = 0 ; count < hundredsDigit - 5 ; count++ ) { 

                    System.out.print (hundred);

                }

            }



            //Tens (Checks all cases 1-9)

            //Checks 1 - 3

            if ((tensDigit >= 1) && (tensDigit <= 3))   {

                for (int count = 0 ; count < tensDigit ; count++) {

                    System.out.print (ten);

                }

            }

            // Checks 4

            else if (tensDigit == 4) {

                System.out.print (ten + fifty); 

            }

            // Checks 9

            else if (tensDigit == 9) {

                System.out.print (ten + hundred);

            }

            // Omit 0

            else if (tensDigit == 0) {

            }

            // Checks 5 - 8

            else  {

                System.out.print (fifty);

                for (int count = 0 ; count < tensDigit - 5 ; count++ ) { 

                    System.out.print (ten);

                }

            }



            //Ones (Checks all cases 1-9)

            //Checks 1 - 3

            if ((onesDigit >= 1) && (onesDigit <= 3))   {

                for (int count = 0 ; count < onesDigit ; count++) {

                    System.out.print (one);

                }

            }

            // Checks 4

            else if (onesDigit == 4) {

                System.out.print (one + five);

            }

            //Checks 9

            else if (onesDigit == 9) {

                System.out.print (one + ten);

            }

            //Omit 0

            else if (onesDigit == 0) {

            }

            // else (checks 5 - 9)

            else  {

                System.out.print (five);

                for (int count = 0 ; count < onesDigit - 5 ; count++ ) { 

                    System.out.print (one);

                }

            }

            //Print line outside of all conditions

            System.out.println("");

            keepGoing = false ;

        }

    }

}

Because of the repeating scheme 1er, 5er, we can do it in a loop. The special cases 0/5 and 4/9 are as like as your code.

private static String toRomanNum(int val) {

    String out = "", chars = "IVXLCDM";

    int i, digit, d5, m5, idx=6, divi = 1000;



    while(idx>=0) {

        digit = val/divi;

        val %= divi;

        d5=digit/5;

        m5=digit%5;

        if(m5==0) {

            if(d5==1) out += chars.charAt(idx+1);

        }

        else if(m5==4) out += chars.charAt(idx) +""+ chars.charAt(idx+1+d5);

        else {

            if(d5==1) out += chars.charAt(idx+1);

            for(i=0;i<m5;i++) out += chars.charAt(idx);

        }

        divi/=10;

        idx-=2;

    }

    return out;

}

Control structure formatting

            }

            // Checks 4

            else if (onesDigit == 4) {

                System.out.print (one + five);

Please don't put comments in between curly braces and their associated control statements.

            }

            else if (onesDigit == 4) {

                // Checks 4

                System.out.print (one + five);

Just treat them as one unit. The problem with separating them is that it looks like the structure is over, because Java (and other C-style languages) use the same symbol for ending the control structure as ending the current piece of the structure.

I would actually write it as

           } else if (onesDigit == 4) {

Which is also the Java standard. But that's not as important as not adding more stuff between.

Comments

Most of your comments just restate what the code does. That makes sense in class, as the professor is telling you what the code does to help you learn. And since you may not understand what the code does, those kind of comments can make your programs easier to grade. But outside class, we expect programmers to be able to read code. Comments should not tell us what the code does, but why it is doing it.

Declarations

It is also the standard in Java to declare variables as late as possible, often at initialization. Your method looks more like old C code with all the declarations at the beginning.

try-with-resources

This may be past what your assignment allowed, but the right way to use an Autocloseable resource like a Scanner is

try (Scanner input = new Scanner(System.in)) {

    // do stuff with the Scanner

}

This will automatically close the Scanner when it is done, even if there is an exception. It's essentially the equivalent of adding

} finally {

    if (input != null) {

        input.close();

    }

I would also avoid putting spaces between a method name and the opening parenthesis. E.g. Scanner(System.in) is a constructor call. I would put a space in if (.

Putting it together

Adding the method from this answer (albeit with code that I find more readable):

class RomanNumeral {



    private static final String DIGITS = "IVXLCDM";



    public static void main(String args) {

        try (Scanner input = new Scanner(System.in)) {

            System.out.println(toRomanNumeral(input.nextInt()));

        }

    }



    private static String toRomanNumeral(int value) {

        StringBuilder out = new StringBuilder();



        int divisor = 1000;

        for (int i = DIGITS.length() - 1; i >= 0; i -= 2) {

            int unit = value / divisor;

            value %= divisor;



            int unit5 = unit / 5;

            int remainder = unit % 5;



            if (remainder == 4) {

                // if the remainder is 4, then one of IV, IX, XL, XC, CD, CM

                // i tells us if it is I, X, or C

                // i + 1 tells us which pair:  VX, LC, or DM.  

                // unit5 tells us if it is the first (V, L, or D) or second (X, C, or M)

                out.append(DIGITS.charAt(i))

                   .append(DIGITS.charAt(i + 1 + unit5));

            } else {

                // add V, L, or D if needed

                if (unit5 == 1) {

                    out.append(DIGITS.charAt(i + 1));

                }



                // add up to three I, X, or C

                for (int j = 0; j < remainder; j++) {

                    out.append(DIGITS.charAt(i));

                }

            }



            divisor /= 10;

        }



        return out.toString();

    }



}

Your professor also might bar StringBuilder, although he shouldn't. Mixing calculation and output the way that your program does is a code smell. StringBuilder should be taught before System.out.print in my opinion. That said, the algorithm doesn't change if you replace the append calls with System.out.print.

The initial check if there is no remainder was unnecessary. The else case will handle it correctly. So I removed it.

I used a for loop as shorter and more readable than a while loop.

Moving the code to a separate method is simply better practice. Again, if your professor has not taught you that, that is a teaching failure. IMnsHO, method calls should be taught before if, else, and loops (for or while).

This code will throw an exception if given an input of 3900 or more (also probably for negative values). The exception will be confusing. It would be better to check for invalid values and throw a more sensible exception. However, your professor may not have taught exceptions yet.

I made DIGITS a class constant because it never changes throughout the program. You can make it a method variable if your professor hasn't taught class constants yet.