Roman Numeral to Integer: what is wrong with my code

Question

So I'm trying to make a converter from roman numerals to integers. What I have so far is this:

public int toNumber(String n){
   int number = 0;
   int first = 1;
   int last = 0;      
   String substring = n.substring (last, first);

   while(substring.equals("I")){
            number = number+1;
            last = last +1;
            first=first +1;
        }


   while(substring.equals("V")){
            number = number+5;
            last = last +1;
            first=first +1;
        }
        return number;

}

Obviously I only have I and V in right now, but when I make a tester class to try this out, it returns nothing and keeps letting me put in in a new line.

For the record, here is my tester class

import java.util.Scanner;
public class Tester{
    public static void main (String[] args){
        RomanNumeralConverter quantity = new RomanNumeralConverter();
       Scanner user_input = new Scanner(System.in);

       //this is for roman to number  
          System.out.println("input roman numeral");
          String j  = user_input.nextLine(); 
          int g = quantity.toNumber(j);
          System.out.println(g);

    }
}

I'm almost entirely certain that it's a logic problem, but I have no idea what and I feel like I've tried everything I can think of

`while(substring.equals("I"))` ... and where do you change `substring` so this could return `false`? Your program doesn't keep letting you input stuff, it stuck in an infinite loop. — Tom, Nov 05 '14 at 00:21
the String class has a charAt(int index) method, that can be used to get a character at a particular location, just so you know. — jmrah, Nov 05 '14 at 00:23
I kind of figured that it was stuck in an infinite loop, but how do I get it to return false when I want it to — Matt, Nov 05 '14 at 00:34

score 0 · Accepted Answer · answered Nov 05 '14 at 00:35

Your problem is a loop like this:

while(substring.equals("I")) {
    number = number+1;
    last = last +1;
    first=first +1;
}

If the program enters that loop, it will stuck there, because you're not changing the value of substring. Therefore substring.equals("I") will always return true and the only way to stop that loop is terminating the application.

A better way to convert the entered String could be this:

public int toNumber(String n) {
    int number = 0;

    for (char chr : n.toUpperCase().toCharArray()) {
        switch(chr) {
            case 'I': number += 1;
                      break;
            case 'V': number += 5;
                      break;
            default: break;
        }
    }

    return number;
}

It converts the provided String to uppercase (viii -> VIII) to avoid checking both cases of every char and converts it into a char array. This array will be used in a foreach loop that takes every single entry of it and tests it in a switch block. Every supported roman numeral has his own case there to increment number accordingly. Currently it supports I and V like in your code. Btw, number += 1 is just a short version of number = number + 1.

After that loop, it returns the converted integer value.

Roman Numeral to Integer: what is wrong with my code

1 Answers1