2
\$\begingroup\$

Is there any way to make this for loop more concise or efficient? I fell like there should be a way that you can cahnge the for loop so that it is about half the size that it is know. By the way, the array list defaultArabic has the values of {1,5,10,50,100,500,1000). Also, I'm not sure if you need to know this, but the goal of this code is the convert tradition roman letter such as (I,V,X,etc) to our form of numbers. Additionally, I am not looking for a whole new for loop just a revised one, as this for loop works correctly. However, it is just really long, bulky, and repetitive.

for (c = 0; c < 15; c++)
{
    findRomans = romans.get(c);
    if (findRomans.equals("I"))
    {
        if (romans.get(c + 1).equals("I"))
        {
            arabic += defaultArabics.get(0);
        }
        else
        {
            arabic -= defaultArabics.get(0);
        }
    }
    else if (findRomans.equals("V"))
    {
        if (romans.get(c + 1).equals("I") || romans.get(c + 1).equals("V"))
        {
            arabic += defaultArabics.get(1);
        }
        else
        {
            arabic -= defaultArabics.get(1);
        }
    }
    else if (findRomans.equals("X"))
    {
        if (romans.get(c + 1).equals("I") || romans.get(c +  1).equals("V") || romans.get(c + 1).equals("X"))
        {
            arabic += defaultArabics.get(2);
        }
        else
        {
            arabic -= defaultArabics.get(2);
        }
    }
    else if (findRomans.equals("L"))
    {
        if (romans.get(c + 1).equals("I") || romans.get(c + 1).equals("V") || romans.get(c + 1).equals("X") || romans.get(c + 1).equals("L"))
        {
            arabic += defaultArabics.get(3);
        }
        else
        {
            arabic -= defaultArabics.get(3);
        }
    }
    else if (findRomans.equals("C"))
    {
        if (romans.get(c + 1).equals("I") || romans.get(c + 1).equals("V") || romans.get(c + 1).equals("X") || romans.get(c + 1).equals("L") || romans.get(c + 1).equals("C"))
        {
            arabic += defaultArabics.get(4);
        }
        else
        {
            arabic -= defaultArabics.get(4);
        }
    }
    else if (findRomans.equals("D"))
    {
        if (romans.get(c + 1).equals("I") || romans.get(c + 1).equals("V") || romans.get(c + 1).equals("X") || romans.get(c + 1).equals("L") || romans.get(c + 1).equals("C") || romans.get(c + 1).equals("D"))
        {
            arabic += defaultArabics.get(5);
        }
        else
        {
            arabic -= defaultArabics.get(5);
        }
    }
    else if (findRomans.equals("M"))
    {
        if (romans.get(c + 1).equals("I") || romans.get(c + 1).equals("V") || romans.get(c + 1).equals("X") || romans.get(c + 1).equals("L") || romans.get(c + 1).equals("C") || romans.get(c + 1).equals("D") || romans.get(c + 1).equals("M"))
        {
            arabic += defaultArabics.get(6);
        }
        else
        {
            arabic += defaultArabics.get(6);
        }
    }
}
return arabic;
\$\endgroup\$
  • 1
    \$\begingroup\$ What is romans? Is it a List<String> that is always exactly 15 elements in size? What if the user enters a different-sized number? \$\endgroup\$ – Klitos Kyriacou Jan 22 '17 at 23:10
4
\$\begingroup\$

There is a common pattern followed in all the if-else blocks in the given code. This can be moved to a function or a loop.

Here the pseudo code does this using a loop. As the condition in each successive block is cumulative, it is tracked as a separate variable modified by each successive iteration.

arr = {"I", "V", "X", "L", "C", "D", "M"}; // Store these in an array
for (c = 0; c < 15; c++) {
    findRomans = romans.get(c);
    condition = false;
    for(index = 0; index < size(arr); index++) {
        condition = condition || romans.get(c + 1).equals(arr[index]);
        if (findRomans.equals(arr[index])) {
            int multiplier = condition ? 1 : -1;
            arabic += multiplier * defaultArabics.get(index);
        }
    }
}
return arabic;

This code is just more concise than the original one. There is no efficiency gained here.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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