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 could use some feedback on the class I designed for the above purpose, the complete code is here:

https://github.com/tom-dignan/android-switched-service/blob/master/src/com/tomdignan/android/libs/switchedservice/SwitchedService.java

My main question is, does this make sense for its intended purpose?

But I will paste it all for convenience. Much of the class is explained in the doc-blocks.

package com.tomdignan.android.libs.switchedservice;

import android.app.Service;
import android.content.Context;
import android.content.Intent;
import android.os.IBinder;

/**
 * SwitchedService is a Service that can truly be turned on and off.
 * 
 * Android's nomenclature for sending messages to a Service is "startService"
 * when the task performed is really to "start if not started, and send a message". 
 * A call to stopService does not guarantee any sort of callback by the Service itself 
 * to be called. "onDestroy" is to be called "by the system to notify a Service that 
 * it is no longer used and is being removed" and therefore cannot be used to tell the 
 * service to "stop doing something" which is what would seem should be the intended 
 * purpose. This is really bothersome when calling the Service from a BroadcastReceiver, 
 * which cannot bind to the Service. This leaves only one solution, calling startService 
 * with an Intent that identifies whether the service is being started or stopped.
 * 
 * @see http://developer.android.com/reference/android/app/Service.html
 */
abstract public class SwitchedService extends Service {

    //=========================================================================
    // Static Helpers for Special Start/Stop cases
    //=========================================================================

    /** 
     * Does the tedious work of starting the service. Vanilla case,
     * no extras.
     * 
     * @param context
     * @throws NoSuchServiceException thrown if ComponentName is null, i.e.
     * the service does not exist.
     */
    public static void startService(Context context, Class<?> service) 
            throws NoSuchServiceException {
        sendIgnitionMessage(context, service, START);
    }

    /**
     * Stops the service. This wrapper is the most important one, because
     * it has to call "startService" which is a counter-intuitive name
     * if one intends to actually stop the service with said method call.
     *
     * The reason for this is that "stopService()" does not have a callback.
     *
     * This enables us to have an "onStop()" callback.
     */
    public static void stopService(Context context, Class<?> service)             
            throws NoSuchServiceException {
        sendIgnitionMessage(context, service, STOP);
    }

    /**
     * Helper method that accomplishes the goal of both startService and
     * stopService.
     * 
     * @param context
     * @param service
     * @param ignition
     * @throws NoSuchServiceException
     */
    private static void sendIgnitionMessage(Context context, Class<?> service, int ignition) 
            throws NoSuchServiceException {
        // Create the intent needed to start the service.
        Intent intent = new Intent(context, service);

        // Tell the service that this intent is meant to start/stop it.
        intent.putExtra(EXTRA_IGNITION, ignition);

        if (context.startService(intent) == null) {
            throw new NoSuchServiceException();
        }
    }

    //=========================================================================
    // The exception that should be thrown when a ComponentName is null
    //=========================================================================

    public static class NoSuchServiceException extends Exception {
        /** For serialization */
        private static final long serialVersionUID = 1823049847998820645L;
    }

    /** 
     * Identifier for the EXTRA that must be sent in the Intent that
     * starts the BetterService
     */
    public static final String EXTRA_IGNITION = "IGNITION";

    /** Identifier for when the service is being started */
    public static final int START = 1;

    /** Identifier for when the service is being stopped. */
    public static final int STOP = 2;

    /** 
     * Identifier for when the caller did not remember to use START
     * or STOP to throw exception.
     */
    public static final int WRONG = 3;

    /** Implement onStart logic here */
    public abstract void onStart();

    /** Override this to hook service stops */
    public abstract void onStop();

    /**
     * Calls either onStart or onStop depending on EXTRA_IGNITION. Always
     * starts
     */
    @Override
    public int onStartCommand(Intent intent, int flags, int startId) {
        int ignition = intent.getIntExtra(EXTRA_IGNITION, WRONG);

        switch (ignition) {
        case START:
            onStart();
            break;

        case STOP:
            onStop();
            stopSelf();
            break;

        case WRONG:
            throw new IllegalStateException("You must specify START or STOP for EXTRA_IGNITION");
        }


        // We only want to start and stop it ourselves.
        return START_NOT_STICKY;
    }

    //=========================================================================
    // Unused
    //=========================================================================

    @Override
    public IBinder onBind(Intent intent) {
        // NOOP
        return null;
    }

}
share|improve this question
add comment

1 Answer

Some small notes:

  1. Comments like this are unnecessary:

     * @param context
     * @param service
     * @param ignition
     * @throws NoSuchServiceException
    

    It says nothing more than the code already does, it's rather noise. (Clean Code by Robert C. Martin: Chapter 4: Comments, Noise Comments)

  2. throw new NoSuchServiceException();
    

    Consider filling the exception with some information which helps debugging (service, intent, etc., for example).

  3. Consider declaring the onStartCommand method as final to prevent accidental overriding.

  4. Have you considered using composition over inheritance? See: Effective Java, 2nd Edition, Item 16: Favor composition over inheritance

share|improve this answer
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.