HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavaMinor

Java (Android) abstract class correct implementation

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
abstractandroidjavacorrectimplementationclass

Problem

I am looking for some comments on my code and whether or not this is the best way to create an abstract class. In addition, some of the abstract methods are rarely called so I use the base variables within the subclass to make sure onCreateDialog selects the correct stuff. Hence, the getTitle() > 0 conditional.

```
public abstract class BaseDialogFragment extends DialogFragment {

// Base Variables
public static final int NONE = -1;
public static final int ITEM_SET = 1;

// Extras
public String extraTitle = "";
public String extraMessage = "";
private View dialogView;
private ListView list;

public BaseDialogFragment() {}

@Override
public Dialog onCreateDialog(Bundle savedInstanceState) {
ContextThemeWrapper context = null;
String theme = ActivityPrefHandler.getTheme(getActivity());
if (theme.equals(PreferencesActivity.MATERIAL)) {
context = new ContextThemeWrapper(getActivity(), R.style.ThemeMainDialog);
} else if (theme.equals(PreferencesActivity.LIGHT)) {
context = new ContextThemeWrapper(getActivity(), R.style.ThemeLightDialog);
} else if (theme.equals(PreferencesActivity.DARK)) {
context = new ContextThemeWrapper(getActivity(), R.style.ThemeDarkDialog);
}
CustomAlertDialogBuilder builder = new CustomAlertDialogBuilder(context);
if (getIcon() > 0) {
builder.setIcon(getIcon());
}
if (getTitle() > 0) {
if (getExtraTitle().equals("")) {
builder.setTitle(getTitle());
} else {
builder.setTitle(getExtraTitle());
}
}
if (getMessage() > 0) {
if (getExtraMessage().equals("")) {
builder.setMessage(getMessage());
} else {
builder.setMessage(getExtraMessage());
}
}
if (getDialogView() > 0) {
LayoutInflater inflater = getActivity().getLayoutInflater();
dialogView = inflater.inflate(getDialogView(), null);
builder.setView(dialogView);
}
if (getPositiveButton

Solution

Here are my thoughts going line by line:

-
Comments like // Base Variables are useless, so get rid of them. The first two variables you declare are in fact constants and their role is obvious from their instantiation. It looks like you just have two performing a related role, though. Maybe an enum would be more appropriate?

-
No need for the empty constructor.

-
The initial theme if-else block rubs me the wrong way, though I might just be pedantic, since it's so terse. Still, what I would rather see is a mapping of PreferenceActivity enum values to R.style values so you can do a simple lookup, remove the duplication, and add error handling if a key isn't found.

-
onCreateDialog does a lot of different things. You can extract methods to raise the abstraction level, since all you seem to need is you CustomAlertDialogBuilder object passed in.

-
If you are able to use Java 8, you can greatly simply all of your OnClickListener methods.

-
What are getExtraTitle and getExtraMessage even doing? Look at getExtraTitle. It says, if extraTitle isn't the empty string, return extraTitle, otherwise return the empty string. That's equivalent to just return extraTitle;.

I have used this format myself, however, but with the consideration of the fact that the string might be null. In that case, I would use the Apache commons-lang lib to do the following:

public String getExtraTitle() {
  return Strings.isNullOrEmpty(extraTitle) ? "" : extraTitle;
}

Code Snippets

public String getExtraTitle() {
  return Strings.isNullOrEmpty(extraTitle) ? "" : extraTitle;
}

Context

StackExchange Code Review Q#84095, answer score: 2

Revisions (0)

No revisions yet.