3
\$\begingroup\$

I recently had to work in a tricky piece of work for a project I am working on. You can find its code on GitHub and I'd like to have a feedback about some specific parts of it.

The project is a library and if you look at the README you will see how it needs to be integrated with an android application. I report it here:

public class YOUR_APP_NAMEApplication extends Application{
    private RateMyApp rateMyApp;
    @Override
    public void onCreate(){
        RateMyAppBuilder builder = new RateMyAppBuilder();
        builder.setLaunchesBeforeAlert(3);
        builder.setDaysBeforeAlert(7);
        builder.setEmailAddress("mariosangiorgio@gmail.com");
        PreferencesManager preferencesManager = SharedPreferencesManager.buildFromContext(this);
        rateMyApp = builder.build(preferencesManager);
    }

    public void activityCreated(Activity activity){
        if(rateMyApp != null)
            rateMyApp.appLaunched(activity);
    }

    public void activityDestroyed(){
        if(rateMyApp != null)
            rateMyApp.activityDestroyed();
    }
}

public class MainActivity extends Activity {
    @Override
    public void onCreate(Bundle savedInstanceState) {
        setContentView(R.layout.activity_main);
        super.onCreate(savedInstanceState);

        FutsalCoachApplication application = (FutsalCoachApplication)     getApplicationContext();
        application.activityCreated(this);
    }

    @Override
    public void onDestroy(){
        FutsalCoachApplication application = (FutsalCoachApplication) getApplicationContext();
        application.activityDestroyed();
        super.onDestroy();
    }
}

The first thing I'd like to ask is if it acceptable to require the creation of a new class extending Application just to hold the main component of my library.

I'd also like to get a feedback about the fact that my library needs to create instances of DialogFragment and to start new activities. Both these tasks require an active Activity and this is a source of a bit of trickery.

I like the fact that RateMyApp class has the dependency on the Activity instance isolated in the RateMyApp.appLaunched(Activity) method.

I don't really like how I get the class extending Application to get a reference of the current Activity. Needing to override Activity.onCreate(Bundle) and Activity.onDestroy() feels a bit dirty.

What do you think about this approach? Would you do it in an alternative (and possible cleaner) way?

\$\endgroup\$
  • 1
    \$\begingroup\$ Even though this code is reviewable (and I'm working on an answer for it), code that is expected to be reviewed should be contained within the question. Do you also want your RateMyApp class to be reviewed? If so, add that class to your question. \$\endgroup\$ – Simon Forsberg Apr 22 '14 at 15:29
  • \$\begingroup\$ I think that for now it would be enough to review only this piece of code. Actually I received a pull request with an alternate solution, which looks much better, but I'm still interested in comments about the code I wrote. \$\endgroup\$ – mariosangiorgio Apr 23 '14 at 20:21
3
\$\begingroup\$

First, I would recommend that you take a look at the null object pattern to avoid the != null check here:

if (rateMyApp != null)
    rateMyApp.appLaunched(activity);

By using a "null object", simply rateMyApp.appLaunched(activity); would be enough. Your null object is an instance of a special class that just doesn't do anything in the appLaunched method.


The first thing I'd like to ask is if it acceptable to require the creation of a new class extending Application just to hold the main component of my library.

No. Absolutely not. I don't really see why you are using the Application to do that. You could write a regular Java class for holding the main component of your library. Or rather, you could let the user of your library determine how to hold the main component of your library.

Instead of using the onCreate method of the Application, you could let the user initialize the RateMyApp object in the onCreate of a regular Activity.

I'd also like to get a feedback about the fact that my library needs to create instances of DialogFragment and to start new activities. Both these tasks require an active Activity and this is a source of a bit of trickery.

Technically, those tasks don't need an active Activity. They need a Context. However, "acquiring" an activity should not be that much of a problem. See below.

I don't really like how I get the class extending Application to get a reference of the current Activity.

Me neither. Why does it need a reference of the current activity?

Would you do it in an alternative (and possible cleaner) way?

Let the user whenever he likes create a RateMyApp object, and call the .appLaunched method on it (as you've done in the sample). There is no need for your RateMyApp to know which is the current showing activity, it only needs to know about an activity (or rather, Context) when it should do something - i.e. possibly show a dialog or read/write the preferences to keep track on how many times/days the application has been used. When the user of your library wants your library to possibly show the dialog, let it call a method on your library and pass along the current Activity as a parameter. That's it. No need to make things more complicated than necessary.

Summary

By extending Application as you've done in your question, you have only added a middle hand that doesn't add any value at all. All the things that are done there can, and in this case really should, be done in the MainActivity instead.

\$\endgroup\$
  • \$\begingroup\$ Thanks for your answer. Actually the real issue was another. The reason I used the class extending Application was to discriminate between the creation of an Activity at the startup of the application and the creation of the activity for another reason. I then discovered, as you can see in the sample, that the parameter of onCreate is null when the activity is created for the first time and I used this knowledge to get rid of the class extending Application \$\endgroup\$ – mariosangiorgio Apr 24 '14 at 20:40
  • \$\begingroup\$ Minor note: As to the first point, rateMyApp will never be null, so the null check is just unnecessary anyway. (Application.onCreate() will always be called before any activities are created) \$\endgroup\$ – kcoppock May 1 '14 at 6:31

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.