CODEWITHSUNDEEP
javaandroidoptimization
droid192
droid192

Reputation: 2242

More elegant code than multiple if (){do} checks

Can the following behavior, combining three criteria, be achieved with more elegant solution. If read a lot that switch case is bad smell and if else would get to nested. 

    @Override
public void onCheckedChanged(CompoundButton buttonView, boolean isChecked) {
    Switch sr = (Switch) findViewById(R.id.switch_ros);
    Switch srs = (Switch) findViewById(R.id.switch_ros_stream);
    boolean wifi_state = isConnected(this);
     if (buttonView.getId() == R.id.switch_ros & buttonView.isChecked() & wifi_state) {
        Toast.makeText(this, "ros intent ready", Toast.LENGTH_SHORT).show();
    }
    if (buttonView.getId() == R.id.switch_ros & buttonView.isChecked() & !wifi_state) {
        Toast.makeText(this, "log: wifi bad", Toast.LENGTH_SHORT).show();
        sr.setChecked(false);
    }
    if (buttonView.getId() == R.id.switch_ros & !buttonView.isChecked()) {
        Toast.makeText(this, "ros intent stopped", Toast.LENGTH_SHORT).show();
        srs.setChecked(false); // and stop ros intent, automatically calls onCheckedChange again
    }
    if (buttonView.getId() == R.id.switch_ros_stream & buttonView.isChecked() & sr.isChecked()) {
        Toast.makeText(this, "stream intent ready", Toast.LENGTH_SHORT).show();
    }
    if (buttonView.getId() == R.id.switch_ros_stream & buttonView.isChecked() & !sr.isChecked()) {
        Toast.makeText(this, "log: first switch is off", Toast.LENGTH_SHORT).show();
        srs.setChecked(false);
    }
    if (buttonView.getId() == R.id.switch_ros_stream & !buttonView.isChecked()) {
        Toast.makeText(this, "stream intent stopped", Toast.LENGTH_SHORT).show();
        // and stop stream intent
    }
}

Upvotes: 0

Views: 98

Answers (3)

Stultuske
Stultuske

Reputation: 9437

I've refactored your code a bit, with the changes I suggested in the comments:

    @Override
public void onCheckedChanged(CompoundButton buttonView, boolean isChecked) {
    Switch sr = (Switch) findViewById(R.id.switch_ros);
    Switch srs = (Switch) findViewById(R.id.switch_ros_stream);
    boolean wifi_state = isConnected(this);

    String text = "";
    if ( buttonView.getId() == R.id.switch_ros){
    if ( buttonView.isChecked()){
        text = wifi_state ? "ros intent ready":"log: wifi bad";
        if ( !wifi_state){
        sr.setChecked(false);
        }
    }
    else{
        text = "ros intent stopped";
        sr.setChecked(false);
    }

    }
    else if ( buttonView.getId() == R.id.switch_ros_stream){
    if ( buttonView.isChecked()){
        text = sr.isChecked() ? "stream intent ready" : "log: first switch is off";
        if ( !sr.isChecked()){
        srs.setChecked(false);
        }
    }
    else{
        text = "stream intent stopped";
    }
    }
    Toast.makeText(this, text, Toast.LENGTH_SHORT).show();
    }

Upvotes: 0

gus27
gus27

Reputation: 2656

I'd suggest something like this:

if (buttonView.getId() == R.id.switch_ros)
    if (buttonView.isChecked()) {
        if (wifi_state) {
            Toast.makeText(this, "ros intent ready", Toast.LENGTH_SHORT).show();
        } else {
            Toast.makeText(this, "log: wifi bad", Toast.LENGTH_SHORT).show();
            sr.setChecked(false);
        }
    } else {
        Toast.makeText(this, "ros intent stopped", Toast.LENGTH_SHORT).show();
        srs.setChecked(false); // and stop ros intent, automatically calls onCheckedChange again
    }
} else
if (buttonView.getId() == R.id.switch_ros_stream) {
    if (buttonView.isChecked()) {
        if (sr.isChecked()) {
            Toast.makeText(this, "stream intent ready", Toast.LENGTH_SHORT).show();
        } else {
            Toast.makeText(this, "log: first switch is off", Toast.LENGTH_SHORT).show();
            srs.setChecked(false);
        }        
    } else {
        Toast.makeText(this, "stream intent stopped", Toast.LENGTH_SHORT).show();
        // and stop stream intent
    }
}

This removes the unnecessary checks (@Stultuske) and is little bit more readable.

Upvotes: 0

OldCurmudgeon
OldCurmudgeon

Reputation: 65879

I would roll them out into an enum. Here's the first two done for you.

enum Check {

    IntentReady {

                @Override
                boolean check(CompoundButton buttonView, boolean wifi_state) {
                    return buttonView.getId() == R.id.switch_ros & buttonView.isChecked() & wifi_state;
                }

                @Override
                void apply(Switch sr, Switch srs) {
                    Toast.makeText(this, "ros intent ready", Toast.LENGTH_SHORT).show();
                }

            },
    WIFIBad {

                @Override
                boolean check(CompoundButton buttonView, boolean wifi_state) {
                    return buttonView.getId() == R.id.switch_ros & buttonView.isChecked() & !wifi_state;
                }

                @Override
                void apply(Switch sr, Switch srs) {
                    Toast.makeText(this, "log: wifi bad", Toast.LENGTH_SHORT).show();
                    sr.setChecked(false);
                }

            };

    abstract boolean check(CompoundButton buttonView, boolean wifi_state);

    abstract void apply(Switch sr, Switch srs);
}

public void onCheckedChangedNew(CompoundButton buttonView, boolean isChecked) {
    Switch sr = (Switch) findViewById(R.id.switch_ros);
    Switch srs = (Switch) findViewById(R.id.switch_ros_stream);
    boolean wifi_state = isConnected(this);
    for ( Check c : Check.values()) {
        if ( c.check(buttonView, wifi_state)) {
            c.apply(sr, srs);
        }
    }
}

The primary benefit here is that you can transform the code robotically. Once done you have more flexibility and clarity.

Upvotes: 1

Related Questions