Java - converting Roman numerals

Question

I'm trying to convert Roman numerals into Arabic numerals. I take a string and run a loop checking each character for value. So 'M' would return 1000 etc. In Roman numerals, if a number is smaller than the next one, then you need to subtract it. So 'XL' or '10 50' would be 40. That's why I have an if statement inside the for loop where I check the next character and also return its value. Later I will be doing arithmetic operations but for now I'm just trying to return values.

The problem I have is that it always returns the last character value twice. No matter if I input 2 characters or 10 characters. The if statement is clearly checking the length-1 so it should end checking when it reaches last character. I've tried to debug it and all looks fine then at the last round debugging end and the program prints last value twice for some reason. Can you find what I'm doing wrong?

I can see questions about Roman numerals have been asked before, but the problem I have hasn't been discussed here before.

public class RomanNumerals{
    static int value(char a) {
        if (a=='m') return 1000;
        else if (a=='d') return 500;
        else if (a=='c') return 100;
        else if (a=='l') return 50;
        else if (a=='x') return 10;
        else if (a=='v') return 5;
        else if (a=='i') return 1;
        else return 0;
    }

    public static void main(String[] args) {
        Scanner in=new Scanner(System.in);
        System.out.print("Enter Roman numerals: ");
        String roman=in.nextLine();
        roman=roman.toLowerCase();
        int val=0;
        int val_next=0;

        for (int i=0;i<roman.length();i++) {
            val=value(roman.charAt(i));
            if (i<roman.length()-1) {
                val_next=value(roman.charAt(i+1));
            }
            System.out.println(val + "\t" + val_next);
        }
    }
}

Output:

Enter Roman numerals: MCD
1000    100
100     500
500     500
BUILD SUCCESSFUL (total time: 6 seconds)

Enter Roman numerals: LXMDC
50       10
10       1000
1000     500
500      100
100      100
BUILD SUCCESSFUL (total time: 6 seconds)

---

EDIT: I've finished it. If anyone needs the complete code, here it is:

package hello.world;

import java.util.Scanner;

public class HelloWorld {
    static int value(char a) {
        if (a=='m') return 1000;
        else if (a=='d') return 500;
        else if (a=='c') return 100;
        else if (a=='l') return 50;
        else if (a=='x') return 10;
        else if (a=='v') return 5;
        else if (a=='i') return 1;
        else return 0;
    }

    public static void main(String[] args) {
        Scanner in=new Scanner(System.in);
        System.out.print("Enter Roman numerals: ");
        String roman=in.nextLine();
        roman=roman.toLowerCase();
        int val=0;
        int val_next=0;
        int temp=0;
        int result=0;

        for (int i=0;i<roman.length();i++) {
            val=value(roman.charAt(i));
            if (i<roman.length()-1) {
                val_next=value(roman.charAt(i+1));
            } else val_next=0;

            if (val_next==0) {
                temp=val;
            } else {
                if (val>val_next) temp=val;
                else if (val<val_next) {
                    temp=val_next-val;
                    i++;
                } else if (val==val_next) temp=val;
            }            
            result+=temp;
        }
        System.out.println("Result is: " + result);
    }
}

Output:

Enter Roman numerals: MMMCDXLIX
Result is: 3449
BUILD SUCCESSFUL (total time: 10 seconds)

Enter Roman numerals: MMXIV
Result is: 2014
BUILD SUCCESSFUL (total time: 2 seconds)

Answer 1

Here's your issue:

    for (int i=0;i<roman.length();i++) {
        val=value(roman.charAt(i));
        if (i<roman.length()-1) {
            val_next=value(roman.charAt(i+1)); //<--
        }
        System.out.println(val + "\t" + val_next);
    }

At this line, you set val_next. For the last iteration, it keeps the previous value's iteration since it goes around the if loop, so it will keep 500 from MCD because it was set to 500 in the iteration for C.

    for (int i=0;i<roman.length();i++) {
        val=value(roman.charAt(i));
        if (i<roman.length()-1) {
            val_next=value(roman.charAt(i+1)); //<--
        }
        System.out.println(val + "\t" + val_next);
    }

You need to reset it to 0 for your calculations, I assume.

    for (int i = 0; i < roman.length(); i++) {
        val = value(roman.charAt(i));
        if (i < roman.length() - 1) {
            val_next = value(roman.charAt(i + 1));
        } else {
            val_next = 0;
        }
        System.out.println(val + "\t" + val_next);
    }

The output becomes:

Enter Roman numerals: MCD
1000    100
100     500
500     0

Since you've said you just want the values for now, I'll leave the actual arithmetic up to you :D

Answer 2

you are rechecking the element. So depending on how you write your for-loop, you either have to increment or decrement inside the loop again.

class Solution {
 public int romanToInt(String s) {
    int x=0;
    for(int i=s.length()-1;i>=0;i--){
        if(i!=0&& returnNumber(s.charAt(i))>returnNumber(s.charAt(i-1))){
            x=x+returnNumber(s.charAt(i))-returnNumber(s.charAt(i-1));
            i--;
        }
        else{
            x=x+returnNumber(s.charAt(i));
        }
    } return x;

}
public int returnNumber(char s){
    switch(s){
        case 'I': return 1;

        case 'V': return 5;

        case 'X': return 10;

        case 'L': return 50;

        case 'C': return 100;

        case 'D': return 500;

        case 'M': return 1000;

    }
    return 0;
}
}