5
\$\begingroup\$

Could you review it and said if it's written good and/or is better way to do that?

My algorithm is:

Input roomNumber
   If length of roomNumber is 8 set goToRoom state Enabled and color 0xFFFFFFFF
   Else set goToRoom state Disabled and color 0xBBFFFFFF

Code of that algorithm is:

public class Join_room_screen extends Activity {

    EditText numberRoom;
    Button goToRoom;

    @Override
    protected void onCreate(Bundle savedInstanceState) {
        super.onCreate(savedInstanceState);

        setContentView(R.layout.joinroom);

        numberRoom = (EditText) findViewById(R.id.roomNumber);
        goToRoom = (Button) findViewById(R.id.goToRoom);

        TextWatcher watcher = new LocalTextWatcher();
        goToRoom.addTextChangedListener(watcher);
        updateButtonState();
    }

    void updateButtonState() {
        boolean enabled = checkEditText(numberRoom);
        if (enabled) {
            goToRoom.setBackgroundColor(0xFFFFFFFF);
            goToRoom.setEnabled(enabled);
        } else {
            goToRoom.setBackgroundColor(0xBBFFFFFF);
            goToRoom.setEnabled(false);
        }
    }

    private boolean checkEditText(EditText edit) {
        return ((edit.getText().toString()).length() == 8 );
    }

    private class LocalTextWatcher implements TextWatcher {
        public void afterTextChanged(Editable s) {
            updateButtonState();
        }

        public void beforeTextChanged(CharSequence s, int start, int count, int after) {
        }

        public void onTextChanged(CharSequence s, int start, int before, int count) {
        }
    }
}
\$\endgroup\$
9
\$\begingroup\$

Here is my list of improvements:

  • Encapsulate all the logic in a custom textwatcher. This gives you the benefit to make the EnableViewTextWatcher reusable in other parts of your project (if you extract it from the Activity).

  • Programming against TextView vs EditText and View vs Button will make the code more flexible.

  • Limit visibility by using private keyword (class members, parameters and local variables)

  • Make class members, params and local variables final is possible

  • Follow the general Android coding guidelines regarding naming.

  • Use meaningful names when naming class, class members, params, and local variables

  • Use annotations in your code to improve code quality

  • Use constants instead of Magic Numbers.

public class RoomSelectActivity extends Activity {

    @Override
    protected void onCreate(@Nullable final Bundle savedInstanceState) {
        super.onCreate(savedInstanceState);
        setContentView(R.layout.activity_select_room);

        final TextView roomNumberText = (TextView) findViewById(R.id.tv_room_number);
        final View enabledView = findViewById(R.id.btn_go_to_room);

        final TextWatcher watcher = new EnableViewTextWatcher(roomNumberText, enabledView);
        roomNumberText.addTextChangedListener(watcher);
    }

    private static class EnableViewTextWatcher implements TextWatcher {

        private static final int LENGTH_TO_ENABLE_VIEW = 8;
        private static final int ENABLED_TEXT_COLOR  = 0xFFFFFFFF;
        private static final int DISABLED_TEXT_COLOR = 0xBBFFFFFF;

        private final TextView mWatchedTextView;
        private final View     mEnableView;

        public EnableViewTextWatcher(@NonNull final TextView textViewToWatch, @NonNull final View viewToEnable) {
            mWatchedTextView = textViewToWatch;
            mEnableView = viewToEnable;
            updateEnabledViewState();
        }

        public void afterTextChanged(@NonNull final Editable s) {
            updateEnabledViewState();
        }

        public void beforeTextChanged(@NonNull final CharSequence s, final int start, final int count, final int after) {
        }

        public void onTextChanged(@NonNull final CharSequence s, final int start, final int before, final int count) {
        }

        private boolean checkTextView(@NonNull final TextView textView) {
            return ((textView.getText().toString()).length() == LENGTH_TO_ENABLE_VIEW);
        }

        private void updateEnabledViewState() {
            final boolean enabled = checkTextView(mWatchedTextView);
            mEnableView.setBackgroundColor(enabled ? ENABLED_TEXT_COLOR : DISABLED_TEXT_COLOR);
            mEnableView.setEnabled(enabled);
        }
    }
}
\$\endgroup\$
3
\$\begingroup\$

The method void updateButtonState() can be simplified to:

void updateButtonState() {
    boolean enabled = checkEditText(numberRoom);
    int backgroundColor = enabled ? 0xFFFFFFFF : 0xBBFFFFFF;
    goToRoom.setBackgroundColor(backgroundColor);
    goToRoom.setEnabled(enabled);
}

I would rename checkEditText to be called something like shouldEnable to better reflect its purpose.

The class name Join_room_screen is not idiomatic Java. Consider JoinRoomActivity instead.

Having a separate class for the TextWatcher is an unnecessary overhead. Consider making your activity implement TextWatcher directly.

\$\endgroup\$
3
\$\begingroup\$

In general, color definitions should be extracted to values/color.xml file:

<color name="button_enabled_background">#FFFFFFFF</color>

and referenced like below:

goToRoom.setBackgroundColor(getResources().getColor(R.color.button_enabled_background));

This way you'll have fast and easy access to all color resources. Any changes in color scheme won't require you to dig through tons of code to find and update specific lines.

Edit: Actually, since you want to change the color whenever the button is enabled/disabled, an even better idea would be to make use of Color State List Resource.

A ColorStateList is an object you can define in XML that you can apply as a color, but will actually change colors, depending on the state of the View object to which it is applied.

So in this particular case, you can create a res/color/button_background.xml file:

<?xml version="1.0" encoding="utf-8"?>
<selector xmlns:android="http://schemas.android.com/apk/res/android">
    <item android:state_enabled="false"
          android:color="#BBFFFFFF"/> <!-- disabled -->
    <item android:color="#FFFFFFFF"/> <!-- default -->
</selector>

and set it as your background:

goToRoom.setBackgroundColor(getResources().getColor(R.color.button_background));

Now, when you switch the state of the button, colors are changed automatically as well.

XML resources in the documentation:

\$\endgroup\$
  • \$\begingroup\$ Very good answer. I was able to follow along easily \$\endgroup\$ – Malachi Jan 21 '15 at 19:04

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.