1
\$\begingroup\$

As a complete beginner, I didn't care about how the code looked as long as it works. However, now I am starting to think about how to make code be more maintainable and easier to read.

I feel I have too many else ifs in the following code. I invite you to give me constructive criticism and suggestions.

The purpose of the code is to convert a number from x base system to y base system. The user does this by selecting two drop down menus. One for selecting which base system they are converting from and the second one for selecting the desired base system when the number is converted.

The code runs on click of a button, and hits an else if block depending on the values selected in the drop down menus.

ShowToast Displays a message to the user.

public void convert(View view) {

    Spinner spinner2 = (Spinner) findViewById(R.id.spinner);
    Spinner spinner = (Spinner) findViewById(R.id.spinner2);

    EditText input = (EditText) findViewById(R.id.input);
    EditText output = (EditText) findViewById(R.id.editText);

    String spinnerinput =  spinner.getItemAtPosition(spinner.getSelectedItemPosition()).toString();
    String spinnerinput2 = spinner2.getItemAtPosition(spinner2.getSelectedItemPosition()).toString();

    if(spinnerinput.equals(spinnerinput2)) {
        output.setText(input.getText());
    }

    else if(spinnerinput.equals("Base 2") && spinnerinput2.equals("Base 10")) {
        String regex = "[0-1]+";

        if (input.getText().toString().matches(regex)) {
            output.setText(binaryToDecimal(input.getText().toString()));
        } else {
            Context context = getApplicationContext();
            showToast(context, "Invalid characters for a base 2 number!");

        }
    }

    else if(spinnerinput.equals("Base 2") && spinnerinput2.equals("Base 16")) {
        String regex = "[0-1]+";

        if(input.getText().toString().matches(regex)) {
            output.setText(binaryToHex(input.getText().toString().toUpperCase()));
        } else {
            Context context = getApplicationContext();
            showToast(context, "Invalid characters for a base 2 number!");
        }
    }

    else if(spinnerinput.equals("Base 2") && spinnerinput2.equals("Base 8")) {
        String regex = "[0-1]+";

        if(input.getText().toString().matches(regex)) {
            output.setText(binaryToOctal(input.getText().toString()));
        } else {
            Context context = getApplicationContext();
            showToast(context, "Invalid characters for a base 2 number!");
        }
    }

    else if(spinnerinput.equals("Base 8") && spinnerinput2.equals("Base 2")) {
        String regex = "[0-7]+";

        if(input.getText().toString().matches(regex)) {
            output.setText(octalToBinary(input.getText().toString()));
        } else {
            Context context = getApplicationContext();
            showToast(context, "Invalid characters for a base 8 number!");
        }
    }

    else if(spinnerinput.equals("Base 8") && spinnerinput2.equals("Base 10")) {
        String regex = "[0-7]+";

        if(input.getText().toString().matches(regex)) {
            output.setText(octalToDecimal(input.getText().toString()));
        } else {
            Context context = getApplicationContext();
            showToast(context, "Invalid input for a base 8 number!");
        }
    }

    else if(spinnerinput.equals("Base 10") && spinnerinput2.equals("Base 2")) {
        String regex = "[0-9]+";

        if(input.getText().toString().matches(regex)) {
            output.setText(decimalToBinary(input.getText().toString()));
        } else {
            Context context = getApplicationContext();
            showToast(context, "Invalid characters for a base 10 number!");
        }
    }

    else if(spinnerinput.equals("Base 10") && spinnerinput2.equals("Base 16")) {
        String regex = "[0-9]+";

        if(input.getText().toString().matches(regex)) {
            output.setText(decimalToHex(input.getText().toString()));
        } else {
            Context context = getApplicationContext();
            showToast(context, "Invalid characters for a base 10 number!");
        }
    }

    else if(spinnerinput.equals("Base 10") && spinnerinput2.equals("Base 8")) {
        String regex = "[0-9]+";

        if(input.getText().toString().matches(regex)) {
            output.setText(decimalToOctal(input.getText().toString()));
        } else {
            Context context = getApplicationContext();
            showToast(context, "Invalid characters for a base 10 number!");
        }
    }


}
\$\endgroup\$
1
\$\begingroup\$

You are right, your code can be rewritten more elegantly. A lot of else ... if statements is usually a flag to use a switch statement instead.

You could put all the error checking into a separate method:

// 'base' is the base being used;
// 'digits' is the number written in that base.
private boolean checkOK(String base, String digits) {

  String regex;

  switch (base) {

    case: "Base 2": regex = "[0-1]+"; break;
    case: "Base 8": regex = "[0-7]+"; break;
    case: "Base 10": regex = "[0-9]+"; break;
    case: "Base 16": regex = "[0-9A-Fa-f]+"; break;

    default:
      showToast(context, "Invalid base input to checkOK.");
      return false;

    } // end switch

    return digits.matches(regex);

} // end checkOK()

Then I would cut down on the number of option combinations by always converting the input to base 10, and then converting it to the required base in a second step. That indicates two more methods

// 'base' is the base being used;
// 'digits' is the number written in that base.
private int toBase10(String base, String digits) {

  int base10;

  switch (base) {

    case: "Base 2": base10 = Integer.parseInt(digits, 2); break;
    case: "Base 8": base10 = Integer.parseInt(digits, 8); break;
    case: "Base 10": base10 = Integer.parseInt(digits, 10); break;
    case: "Base 16": base10 = Integer.parseInt(digits, 16); break;

    default:
      showToast(context, "Invalid base input to toBase10.");
      return -1;

    } // end switch

    return base10;

} // end toBase10()

You can use a similar method to convert from base 10 to the output base required. I leave that as an exercise for yourself, since you say you are learning.

With those three methods, your overall code will look something like this:

public void convert(View view) {

  // Set up variables as before.

  if (checkOK (spinnerinput, input.getText().toString()) {

    int intermediateValue = toBase10(spinnerinput, input.getText().toString());

    output = toNewBase(spinnerinput2, intermediateValue);

  } else {
    // You are toast!
    //...

  } // end if

} // end convert()

I have left out some of the detail, but that should give you enough to structure your code more simply. In short, a lot of else ... if statements usually indicate the use of a switch instead. In your case it made more sense to put the switch into separate methods, but that may not always be needed.

I haven't tested any of my code, so be sure to test it first. I was more interested in showing the overall restructure.

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