current community

your communities

more stack exchange communities

Stack Exchange
tour help
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.
up vote 2 down vote favorite
1

I have code which styles standard Android widgets programmatically (not from XML). The code is working, but I have this bunch of if instanceof else if instanceof block which is ugly.

I have read discussions on Stack Overflow, and my conclusion is instanceofis better on my case, but I'm not so sure, maybe I'm missing something?

public static void styleAllControls2(Context c, List<View> controls, CxpResources cxpr) {
    if (cxpr == null) {
        return;
    }
    File xfiles = c.getExternalFilesDir(null);
    if (xfiles == null) {
        Log.e(TAG, "couldnt get external files dir!");
        return;
    }
    Resources res = c.getResources();
    String filepath = xfiles.getAbsolutePath() + File.separator;
    for (View view : controls) {
        ViewModifier vModfr = null;
        if (view instanceof RatingBar) {
            // viewModifier = new RatingBarModifier(res, cxpr, filepath);
        } else if (view instanceof SeekBar) {
            vModfr = new SeekBarModifier(res, cxpr, filepath);
        } else if (view instanceof ProgressBar) {
            vModfr = new ProgressBarModifier(res, cxpr, filepath);
        } else if (view instanceof CheckBox) {
            vModfr = new CheckBoxModifier(res, cxpr, filepath);
        } else if (view instanceof RadioButton) {
            vModfr = new RadioButtonModifier(res, cxpr, filepath);
        } else if (view instanceof Switch) {
            vModfr = new SwitchModifier(res, cxpr, filepath);
        } else if (view instanceof ToggleButton) {
            vModfr = new ToggleButtonModifier(res, cxpr, filepath);
        } else if (view instanceof Button) {
            vModfr = new ButtonModifier(res, cxpr, filepath);
        } else if (view instanceof Spinner) {
            vModfr = new SpinnerModifier(res, cxpr, filepath);
        } else if (view instanceof EditText) {
            vModfr = new EditTextModifier(res, cxpr, filepath);
        }
        if (vModfr != null) {
            vModfr.modify(view);
        }
    }
}

ViewModifier class:

public abstract class ViewModifier<T extends View> {
    private static final String TAG = "ViewModifier";
    protected final CxpResources cxpResources;
    protected final String basedir;
    protected final Resources resources;
    protected final TypedValue typedValue;

    public ViewModifier(Resources resources, CxpResources cxpResources, final String basedir) {
        this.resources = resources;
        this.cxpResources = cxpResources;
        this.basedir = basedir;
        typedValue = new TypedValue();
        typedValue.density = resources.getDisplayMetrics().densityDpi;
    }

    public abstract void modify(T v);

    // some helper codes....
}

All xxxModifier class inherit from ViewModifier class.

My questions:

  1. Is a bunch of instanceofs really the best way? As far as I know, I can't use visitor pattern because I can't modify Android widgets.
  2. Am I using generics right? Particularly in ViewModifier class signature.
  3. Which is better, my current implementation, or to make modify an interface and then have xxxModifier classes implement that? ViewModifier has a bunch of helper methods though.
  4. Review of my code in general, according to Android and Java best practices and conventions.
  5. Intellij Idea / Android Studio tricks to help self-review perhaps?
share|improve this question
1  
2) Yes, this is fine 3) I would make it an interface, later on you can program against this interface 4) if I have time later on I will do a code review, there is improvement possible 5) go settings > inspections (lot of options) –  user1281750 Dec 24 '14 at 11:55
    
1) for the best anwser post this question to stackoverflow –  user1281750 Dec 24 '14 at 12:20

1 Answer 1

up vote 1 down vote accepted

I already answered some questions in a comment here is the code review.

Some tips and suggestions:

  • Use full names for params and local variables to improve readability
  • Don't use numbers in the names of methods
  • Try to use Iterable interface above the List if possible
  • Follow naming conventions for Android
  • Use annotations to improve code quality and usage
  • Use final modifier if possible

'

public static void styleControls(@NonNull final Context context, @NonNull final Iterable<View> controls, @NonNull final Object object) {
    if (object == null) {
        return;
    }
    final File file = context.getExternalFilesDir(null);
    if (file == null) {
        Log.e(TAG, "couldnt get external files dir!");
        return;
    }
    final Resources resources = context.getResources();
    final String filepath = file.getAbsolutePath() + File.separator;
    for (final View view : controls) {
        final Modify modify = null;
        //... instance of checks
        if (modify != null) {
            modify.modify(view);
        }
    }
}

public abstract class ViewModifier implements Modify {

    private static final String TAG = "ViewModifier";

    protected final Object     mObject;
    protected final String     mBasedir;
    protected final Resources  mResources;
    protected final TypedValue mTypedValue;

    public ViewModifier(@NonNull final Resources resources, @NonNull final Object object, @NonNull final String basedir) {
        mResources = resources;
        mObject = object;
        mBasedir = basedir;
        mTypedValue = new TypedValue();
        mTypedValue.density = resources.getDisplayMetrics().densityDpi;
    }

    // some helper codes....
}

public interface Modify<T extends View> {
    void modify(final T t);
}
share|improve this answer
    
I will not accepting this answer until I fully grasp it, but +1 for answering and for the annotations tip. –  eriuzo Dec 24 '14 at 12:26
    
Don't hesitate to ask questions, I will check later on to see if I can find some more improvements –  user1281750 Dec 24 '14 at 12:27
    
BTW: replace that Object method with CxpResources but rename that class to be a fully qualified name –  user1281750 Dec 24 '14 at 12:40

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.