ColorViewer mini-app for viewing colors in different formats

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

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

-

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

The following is the same:

return new ArrayList(Arrays.asList(colorComponents));

-

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
}

-
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 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);
}

-

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.

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

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

-

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

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

-

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.)

StackExchange Code Review Q#21484, answer score: 5