Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I've began to read the book Clean Code by Robert Martin. I've had a strong desire to learn how to write clear, easy-to-understand from other people. The ColorViewer application allows you to view the color in different formats.

Application on GitHub.com

ColorViewerActivity.java

public class ColorViewerActivity extends Activity {
    @Override
    protected void onCreate(Bundle savedInstanceState) {
        super.onCreate(savedInstanceState);
        setContentView(R.layout.activity_color_viewer);

        setUp();
        panelFactory.buildPanel(currentColorFormat);
    }

    @Override
    public boolean onCreateOptionsMenu(Menu menu) {
        radioButtonMenu = new RadioButtonMenu(menu);
        radioButtonMenu.addItems(ColorFormat.getNamesOfAllColorFormats());
        radioButtonMenu.setChecked(currentColorFormat.name());
        return true;
    }

    @Override
    public boolean onOptionsItemSelected(MenuItem item) {
        if (!item.isChecked()) {
            ColorFormat chosenColorFormat = getColorFormatOfMenuItem(item);
            panelFactory.rebuildPanel(chosenColorFormat);
            currentColorFormat = chosenColorFormat;
            radioButtonMenu.setChecked(item);
        }
        return true;
    }

    @Override
    protected void onDestroy() {
        if (isChangingConfigurations()) {
            saveCurrentColorFormatAndBackgroundColor();
        } else {
            saverLoader.deleteAllStoredValues();
        }
        super.onDestroy();
    }

    // Bad method name.
    public void setColor(Color newColor) {
        activityLayout.setBackgroundColor(newColor.getIntegerValue());
        descriptionOfColor.setText(newColor.getDescriptionText());
        descriptionOfColor.setTextColor(getInvertedColor(newColor.getIntegerValue()));
    }

    public int getBackgroundColor() {
        ColorDrawable colorDrawable = (ColorDrawable) activityLayout.getBackground();
        return colorDrawable.getColor();
    }



    private SaverLoader saverLoader;
    private ColorFormat currentColorFormat;
    private ColorViewerPanelFactory panelFactory;
    private RadioButtonMenu radioButtonMenu;
    private TextView descriptionOfColor;
    private View activityLayout;

    private void setUp() {
        descriptionOfColor = (TextView) findViewById(R.id.descriptionOfColor);
        activityLayout = findViewById(R.id.activityLayout);
        saverLoader = SaverLoader.createForActivity(this);
        panelFactory = ColorViewerPanelFactory.createForActivity(this);
        activityLayout.setBackgroundColor(saverLoader.getTheStoredColor());
        currentColorFormat = saverLoader.getTheStoredColorFormat();
    }

    private ColorFormat getColorFormatOfMenuItem(MenuItem item) {
        String titleOfMenuItem = item.getTitle().toString();
        return ColorFormat.valueOf(titleOfMenuItem);
    }

    private void saveCurrentColorFormatAndBackgroundColor() {
        saverLoader.saveColorFormat(currentColorFormat);
        saverLoader.saveColor(getBackgroundColor());
    }

    private int getInvertedColor(int colorInteger) {
        int invertedRed = 255 - android.graphics.Color.red(colorInteger);
        int invertedGreen = 255 - android.graphics.Color.green(colorInteger);
        int invertedBlue = 255 - android.graphics.Color.blue(colorInteger);
        return android.graphics.Color.rgb(invertedRed, invertedGreen, invertedBlue);
    }
}

ColorFormat.java

public enum ColorFormat {
    RGB,
    ARGB;


    public static ArrayList<String> getNamesOfAllColorFormats() {
        ColorFormat[] arrayOfColorFormats = ColorFormat.values();

        ArrayList<String> namesOfColorFormats = new ArrayList<String>(arrayOfColorFormats.length);
        for (ColorFormat colorFormat : arrayOfColorFormats) {
            namesOfColorFormats.add(colorFormat.name());
        }
        return namesOfColorFormats;
    }
}

RadioButtonMenu.java

public class RadioButtonMenu {

    final private Menu menu;
    private MenuItem checkedMenuItem;
    private ArrayList<String> titlesOfMenuItems;


    public RadioButtonMenu(Menu menu) {
        this.menu = menu;
        titlesOfMenuItems = new ArrayList<String>();
        checkedMenuItem = null;
    }

    public void addItems(ArrayList<String> menuItems) {
        for (String item : menuItems) {
            menu.add(item);
        }
        titlesOfMenuItems.addAll(menuItems);
    }

    public void setChecked(String titleOfMenuItem) {
        MenuItem menuItem = findMenuItemByTitle(titleOfMenuItem);
        setChecked(menuItem);
    }

    private MenuItem findMenuItemByTitle(String titleOfMenuItem) {
        int indexOfMenuItem = titlesOfMenuItems.indexOf(titleOfMenuItem);
        MenuItem menuItem = menu.getItem(indexOfMenuItem);
        return menuItem;
    }

    public void setChecked(MenuItem menuItem) {
        uncheckMenuItem(checkedMenuItem);
        checkMenuItem(menuItem);
        checkedMenuItem = menuItem;
    }

    private void checkMenuItem(MenuItem item) {
        item.setCheckable(true);
        item.setChecked(true);
    }

    private void uncheckMenuItem(MenuItem item) {
        if (item != null) {
            item.setChecked(false);
            item.setCheckable(false);
        }
    }
}

SaverLoader.java

public class SaverLoader {

    private ColorViewerActivity activity;
    private SharedPreferences preferences;

    private static final int DEF_COLOR = android.graphics.Color.WHITE;
    private static final ColorFormat DEF_COLOR_FORMAT = ColorFormat.RGB;

    private static final String KEY_COLOR = "BACKGROUND_COLOR";
    private static final String KEY_COLOR_FORMAT = "COLOR_FORMAT";

    public static SaverLoader createForActivity(ColorViewerActivity activity) {
        return new SaverLoader(activity);
    }

    private SaverLoader(ColorViewerActivity activity) {
        this.activity = activity;
    }

    public void saveColor(int colorInteger) {
        preferences = activity.getPreferences(Activity.MODE_PRIVATE);
        Editor editor = preferences.edit();
        editor.putInt(KEY_COLOR, colorInteger);
        editor.commit();
    }

    public int getTheStoredColor() {
        preferences = activity.getPreferences(Activity.MODE_PRIVATE);
        return preferences.getInt(KEY_COLOR, DEF_COLOR);
    }

    public void saveColorFormat(ColorFormat colorFormat) {
        preferences = activity.getPreferences(Activity.MODE_PRIVATE);
        Editor editor = preferences.edit();
        editor.putString(KEY_COLOR_FORMAT, colorFormat.name());
        editor.commit();
    }

    public ColorFormat getTheStoredColorFormat() {
        preferences = activity.getPreferences(Activity.MODE_PRIVATE);
        String nameOfSavedColorFormat = preferences.getString(KEY_COLOR_FORMAT, DEF_COLOR_FORMAT.name());
        return ColorFormat.valueOf(nameOfSavedColorFormat);
    }

    public void deleteAllStoredValues() {
        preferences = activity.getPreferences(Activity.MODE_PRIVATE);
        Editor editor = preferences.edit();
        editor.clear();
        editor.commit();
    }
}

Color.java

public interface Color {
    int getIntegerValue();
    String getDescriptionText();
}

RGBColor.java

public class RGBColor implements Color {

    protected ColorDescriptionTextCreator colorDescriptionText;
    protected int red, green, blue;

    public RGBColor(int red, int green, int blue) {
        colorDescriptionText = new ColorDescriptionTextCreator();
        this.red = red;
        this.green = green;
        this.blue = blue;
    }

    @Override
    public int getIntegerValue() {
        return android.graphics.Color.rgb(red, green, blue);
    }

    @Override
    public String getDescriptionText() {
        return colorDescriptionText.create(
                     new String[] { "red", "green", "blue" },
                     new String[] { String.valueOf(red),
                                    String.valueOf(green),
                                    String.valueOf(blue) }
        );
    }

}

ARGBColor.java

public class ARGBColor extends RGBColor {

    protected int alpha;

    public ARGBColor(int alpha, int red, int green, int blue) {
        super(red, green, blue);
        this.alpha = alpha;
    }

    @Override
    public int getIntegerValue() {
        return android.graphics.Color.argb(alpha, super.red, super.green, super.blue);
    }

    @Override
    public String getDescriptionText() {
        return colorDescriptionText.create(
                 new String[] { "alpha", "red", "green", "blue" },
                 new String[] { String.valueOf(alpha),
                                String.valueOf(super.red),
                                String.valueOf(super.green),
                                String.valueOf(super.blue) }
        );
    }

}

ColorDescriptionTextCreator.java

public final class ColorDescriptionTextCreator {
    private String first;
    private String last;
    private String separatorOfComponentNameAndItsValue;
    private String separatorOfComponents;
    private String[] componentNames;
    private String[] componentValues;

    ColorDescriptionTextCreator() {
        this("", "", " = ", "\n");
    }

    ColorDescriptionTextCreator(String first, String last,
                                String separatorOfComponentNameAndItsValue,
                                String separatorOfComponents) {
        this.first = first;
        this.last = last;
        this.separatorOfComponentNameAndItsValue = separatorOfComponentNameAndItsValue;
        this.separatorOfComponents = separatorOfComponents;
    }

    String create(String[] componentNames, String[] componentValues) {
        this.componentNames = componentNames;
        this.componentValues = componentValues;

        StringBuilder colorDescription = new StringBuilder();
        colorDescription.append(first);
        for (int i = 0; i < componentNames.length; ++i) {
            colorDescription.append(getColorComponentWithIndex(i));
        }
        colorDescription.append(last);
        return colorDescription.toString();
    }

    private StringBuilder getColorComponentWithIndex(int index) {
        StringBuilder colorComponent = new StringBuilder();
        colorComponent.append(componentNames[index]);
        colorComponent.append(separatorOfComponentNameAndItsValue);
        colorComponent.append(componentValues[index]);
        colorComponent.append(isIndexOfLastComponent(index) ? "" : separatorOfComponents);
        return colorComponent;
    }

    private boolean isIndexOfLastComponent(int index) {
        return (index == componentNames.length - 1);
    }
}

ColorViewerPanelFactory.java

public class ColorViewerPanelFactory {
    private ColorViewerActivity colorViewerActivity;
    private ColorViewerPanel colorViewerPanel = null;

    private ColorViewerPanelFactory(ColorViewerActivity colorViewerActivity) {
        this.colorViewerActivity = colorViewerActivity;
    }

    public static ColorViewerPanelFactory createForActivity(ColorViewerActivity colorViewerActivity) {
        return new ColorViewerPanelFactory(colorViewerActivity);
    }

    public void buildPanel(ColorFormat colorFormat) {
        switch (colorFormat) {
        case RGB:
            colorViewerPanel = new RGBPanel(colorViewerActivity);
            break;
        case ARGB:
            colorViewerPanel = new ARGBPanel(colorViewerActivity);
            break;
        default:
            throw new IllegalArgumentException(
                    "ColorViewerPanelFactory.buildPanel(): undefined colorFormat");
        }
    }

    public void destroyPanel() {
        colorViewerPanel.destroy();
    }

    public void rebuildPanel(ColorFormat newColorFormat) {
        destroyPanel();
        buildPanel(newColorFormat);
    }
}

ColorViewerPanel.java

public abstract class ColorViewerPanel {

    private static final int DEFAULT_MAXIMUM_OF_COLOR_SEEKBAR = 255;

    private ColorViewerActivity activity;
    private LinearLayout panelLayout;
    private OnColorSeekBarChangeListener colorChangeListener;
    private ArrayList<String> namesOfColorComponents;
    private ArrayList<SeekBar> colorSeekBars;
    private ArrayList<Integer> maximumsOfColorSeekBars;
    private ArrayList<Integer> startPositionOfTheSeekBars;  


    public abstract Color getColorSettingOnThePanel();


    ColorViewerPanel(ColorViewerActivity activity) {
        this(activity, null);
    }

    ColorViewerPanel(ColorViewerActivity activity,
                    ArrayList<Integer> maximumsOfColorComponentSeekBars) {

        this.activity = activity;
        this.maximumsOfColorSeekBars = maximumsOfColorComponentSeekBars;

        setUp();
        fillPanel();
        setOnSeekBarChangeListenersForColorSeekBars();

        activity.setColor(getColorSettingOnThePanel());
    }

    private void setUp() {
        panelLayout = (LinearLayout) activity.findViewById(R.id.panelLayout);
        namesOfColorComponents = getNamesOfColorComponents();
        colorChangeListener = new OnColorSeekBarChangeListener(this, activity);
        colorSeekBars = new ArrayList<SeekBar>();
        startPositionOfTheSeekBars = getThePositionOfTheSeekBarsForColor(activity.getBackgroundColor());
    }

    protected abstract ArrayList<String> getNamesOfColorComponents();

    // Bad method name
    // Return position of the seekbars that will be installed for colorIntegerValue.
    protected abstract ArrayList<Integer> getThePositionOfTheSeekBarsForColor(int colorIntegerValue);

    private void fillPanel() {
        LayoutInflater inflater = LayoutInflater.from(panelLayout.getContext());
        for (int index = 0; index < namesOfColorComponents.size(); ++index) {
            TextView nameOfColorComponent = prepareTextView(inflater, index);
            panelLayout.addView(nameOfColorComponent);
            SeekBar changerOfColorComponent = prepareSeekBar(inflater, index);
            panelLayout.addView(changerOfColorComponent);
            colorSeekBars.add(changerOfColorComponent);
        }
    }

    private TextView prepareTextView(LayoutInflater inflater, int index) {
        TextView textView = (TextView) inflater.inflate(R.layout.name_of_color_component, null);
        textView.setText(namesOfColorComponents.get(index));
        return textView;
    }

    private SeekBar prepareSeekBar(LayoutInflater inflater, int index) {
        SeekBar seekBar = (SeekBar) inflater.inflate(R.layout.color_seekbar, null);
        seekBar.setMax(getMaxOfSeekBar(index));
        seekBar.setProgress(startPositionOfTheSeekBars.get(index));
        return seekBar;
    }

    private int getMaxOfSeekBar(int indexOfSeekBar) {
        int maxOfSeekBar = (maximumsOfColorSeekBars != null
                                ? maximumsOfColorSeekBars.get(indexOfSeekBar)
                                : DEFAULT_MAXIMUM_OF_COLOR_SEEKBAR);
        return maxOfSeekBar;
    }

    private void setOnSeekBarChangeListenersForColorSeekBars() {
        for (SeekBar seekBar : colorSeekBars) {
            seekBar.setOnSeekBarChangeListener(colorChangeListener);
        }
    }

    protected ArrayList<Integer> getValuesOfColorComponents() {
        ArrayList<Integer> valuesOfColorComponents = new ArrayList<Integer>(colorSeekBars.size());
        for (SeekBar seekBar : colorSeekBars) {
            valuesOfColorComponents.add(seekBar.getProgress());
        }
        return valuesOfColorComponents;
    }

    void destroy() {
        panelLayout.removeAllViewsInLayout();
    }

}

RGBColorPanel.java

public class RGBPanel extends ColorViewerPanel {

    private static final String[] colorComponents = { "Red", "Green", "Blue" };

    protected ArrayList<String> getNamesOfColorComponents() {
        ArrayList<String> namesOfColorComponents = new ArrayList<String>(colorComponents.length);
        for (String component : colorComponents) {
            namesOfColorComponents.add(component);
        }
        return namesOfColorComponents;
    }

    RGBPanel(ColorViewerActivity activity) {
        super(activity);
    }

    @Override
    public Color getColorSettingOnThePanel() {
        ArrayList<Integer> valuesOfColorComponents = super.getValuesOfColorComponents();
        int red = valuesOfColorComponents.get(0);
        int green = valuesOfColorComponents.get(1);
        int blue = valuesOfColorComponents.get(2);
        return new RGBColor(red, green, blue);
    }

    @Override
    protected ArrayList<Integer> getThePositionOfTheSeekBarsForColor(int colorIntegerValue) {
        ArrayList<Integer> positionOfTheSeekBars = new ArrayList<Integer>();
        positionOfTheSeekBars.add(android.graphics.Color.red(colorIntegerValue));
        positionOfTheSeekBars.add(android.graphics.Color.green(colorIntegerValue));
        positionOfTheSeekBars.add(android.graphics.Color.blue(colorIntegerValue));
        return positionOfTheSeekBars;
    }

}

ARGBColorPanel.java

public class ARGBPanel extends ColorViewerPanel {

    private static final String[] colorComponents = { "Alpha", "Red", "Green", "Blue" };

    protected ArrayList<String> getNamesOfColorComponents() {
        ArrayList<String> namesOfColorComponents = new ArrayList<String>(colorComponents.length);
        for (String component : colorComponents) {
            namesOfColorComponents.add(component);
        }
        return namesOfColorComponents;
    }

    ARGBPanel(ColorViewerActivity activity) {
        super(activity);
    }

    @Override
    public Color getColorSettingOnThePanel() {
        ArrayList<Integer> valuesOfColorComponents = super.getValuesOfColorComponents();
        int alpha = valuesOfColorComponents.get(0);
        int red = valuesOfColorComponents.get(1);
        int green = valuesOfColorComponents.get(2);
        int blue = valuesOfColorComponents.get(3);
        return new ARGBColor(alpha, red, green, blue);
    }

    @Override
    protected ArrayList<Integer> getThePositionOfTheSeekBarsForColor(int colorIntegerValue) {
        ArrayList<Integer> positionOfTheSeekBars = new ArrayList<Integer>();
        positionOfTheSeekBars.add(android.graphics.Color.alpha(colorIntegerValue));
        positionOfTheSeekBars.add(android.graphics.Color.red(colorIntegerValue));
        positionOfTheSeekBars.add(android.graphics.Color.green(colorIntegerValue));
        positionOfTheSeekBars.add(android.graphics.Color.blue(colorIntegerValue));
        return positionOfTheSeekBars;
    }

}

OnColorSeekBarChangeListener.java

public class OnColorSeekBarChangeListener implements OnSeekBarChangeListener {

    private ColorViewerPanel panel;
    private ColorViewerActivity activity;

    public OnColorSeekBarChangeListener(ColorViewerPanel panel, ColorViewerActivity activity) {
        this.panel = panel;
        this.activity = activity;
    }

    @Override
    public void onProgressChanged(SeekBar seekBar, int progress, boolean fromUser) {
        activity.setColor(panel.getColorSettingOnThePanel());
    }

    @Override
    public void onStartTrackingTouch(SeekBar seekBar) {
    }

    @Override
    public void onStopTrackingTouch(SeekBar seekBar) {
    }

}
share|improve this question
3  
one thing that Uncle Bob was clear about in that book was Unit Tests... :) I don't see any on your GitHub site either :) as for the rest it isn't horrible. Since there is a bunch of things I would like to point out that in both child classes of ColorPanel you implement the same method exactly the same way. This means you can move implementation to the abstract class. Just make your array colorComponents abstract –  Robert Snyder Feb 9 '13 at 3:08
3  
you should break this up into multiple reviews. –  Malachi Nov 12 '13 at 20:46
add comment

1 Answer

I'm not an Android developer, so just some notes from a Java developer's perspective.

  1. List<String> namesOfColorComponents = new ArrayList<String>(colorComponents.length);
    for (String component : colorComponents) {
        namesOfColorComponents.add(component);
    }
    return namesOfColorComponents;
    

    The following is the same:

    return new ArrayList<String>(Arrays.asList(colorComponents));
    
  2. String create(String[] componentNames, String[] componentValues) {
        ...
    }
    

    It would be more encapsulated if you were using only one array (or List) with objects which encapsulates the name and the value:

    public class ComponentData {
        private String name;
        private String value;
    
        // constructors, getters
    }
    
  3. These indexes (0-3) looks very fragile because they could be changed in the superclass while it's too easy to forget changing them in the child class. Additionally, they are magic numbers. Named constants (with descriptive names) would be better but using named getter methods would be the best.

    @Override
    public Color getColorSettingOnThePanel() {
        ArrayList<Integer> valuesOfColorComponents = super.getValuesOfColorComponents();
        int alpha = valuesOfColorComponents.get(0);
        int red = valuesOfColorComponents.get(1);
        int green = valuesOfColorComponents.get(2);
        int blue = valuesOfColorComponents.get(3);
        return new ARGBColor(alpha, red, green, blue);
    }
    
  4. protected int red, green, blue;
    

    I'd put the variable declarations to separate lines. From Code Complete 2nd Edition, p759:

    With statements on their own lines, the code reads from top to bottom, instead of top to bottom and left to right. When you’re looking for a specific line of code, your eye should be able to follow the left margin of the code. It shouldn’t have to dip into each and every line just because a single line might contain two statements.

    Furthermore, protected fields does not suggest good encapsulation. Check Effective Java, Second Edition, Item 16: Favor composition over inheritance if you haven't seen it already.

  5. I like that there is only one switch-case, it is inside the builder class and the default case throws an exception (fail early).

  6. ArrayList<...> reference and return types should be simply List<...>. See: Effective Java, 2nd edition, Item 52: Refer to objects by their interfaces

  7. ColorDescriptionTextCreator(String first, String last,
           String separatorOfComponentNameAndItsValue,
           String separatorOfComponents) {
    

    Effective Java 2nd Edition, Item 2: Consider a builder when faced with many constructor parameters

  8. private SaverLoader saverLoader;
    private ColorFormat currentColorFormat;
    private ColorViewerPanelFactory panelFactory;
    private RadioButtonMenu radioButtonMenu;
    private TextView descriptionOfColor;
    private View activityLayout;
    private Andoid android;
    

    Fields should be at the beginning of the class, then comes the constructor, then the other methods. (According to the Code Conventions for the Java Programming Language, 3.1.3 Class and Interface Declarations.)

share|improve this answer
2  
+1 and Effective Java, 2nd Edition is a very great book :) I love it. –  Marco Acierno Mar 2 at 20:32
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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