current community

your communities

more stack exchange communities

Stack Exchange Inbox Reputation and Badges
tour help
Sign up ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 4 down vote favorite
1

Is it wrong? Should I have these 2 classes in separate files?

public interface InternetDependentFlow {
    void onInternetIsAvailable();
    void onInternetIsNotAvailable();

    public static abstract class InternetIsAvailable implements InternetDependentFlow {
        @Override
        public final void onInternetIsNotAvailable() {
            // do nothing
        }
    }

    public static abstract class InternetIsNotAvailable implements InternetDependentFlow {
        @Override
        public final void onInternetIsAvailable() {
            // do nothing
        }
    }
}

Where I use this interface and the classes:

public class InternetDependentFlowTask extends AsyncTask<InternetDependentFlow,Void,Boolean> {

    private final Context context;
    private InternetDependentFlow[] internetDependentFlows;

    public static void run(Context context, InternetDependentFlow... internetDependentFlows) {
        InternetDependentFlowTask internetDependentFlowTask = new InternetDependentFlowTask(context);
        internetDependentFlowTask.execute(internetDependentFlows);
    }

    public InternetDependentFlowTask(Context context) {
        this.context = context;
    }

    @Override
    protected Boolean doInBackground(InternetDependentFlow... internetDependentFlows) {
        this.internetDependentFlows = internetDependentFlows;
        return NetworkUtils.isInternetAvailable(context);
    }

    @Override
    protected void onPostExecute(Boolean internetIsAvailable) {
        super.onPostExecute(internetIsAvailable);
        InternetDependentFlowChooser flowChooser = (internetIsAvailable == Boolean.TRUE)
                ? INTERNET_IS_AVAILABLE_FLOW_CHOOSER : INTERNET_IS_NOT_AVAILABLE_FLOW_CHOOSER;
        for (InternetDependentFlow internetDependentFlow : internetDependentFlows) {
            if (internetDependentFlow != null) {
                flowChooser.choose(internetDependentFlow);
            }
        }
    }

    private static interface InternetDependentFlowChooser {
        void choose(InternetDependentFlow internetDependentFlow);
    }

    private static final InternetDependentFlowChooser INTERNET_IS_AVAILABLE_FLOW_CHOOSER = new InternetDependentFlowChooser() {
        @Override
        public void choose(InternetDependentFlow internetDependentFlow) {
            internetDependentFlow.onInternetIsAvailable();
        }
    };

    private static final InternetDependentFlowChooser INTERNET_IS_NOT_AVAILABLE_FLOW_CHOOSER = new InternetDependentFlowChooser() {
        @Override
        public void choose(InternetDependentFlow internetDependentFlow) {
            internetDependentFlow.onInternetIsNotAvailable();
        }
    };
}

And how InternetDependentFlowTask is using:

playButton.setOnClickListener(new View.OnClickListener() {
    public void onClick(View v) {
        MusicManager.playClickBig();
        InternetDependentFlowTask.run(WelcomeActivity.this, new InternetDependentFlow() {
            @Override
            public void onInternetIsAvailable() {
                startGameActivity();
                finish();
            }
            @Override
            public void onInternetIsNotAvailable() {
                notifyUserInternetIsNotAvailable();
            }
        });
    }
});
share|improve this question
    
I have to downvote this because you have not provided any information at all about how you are calling the methods of this interface, or what the use-case of them is. –  Simon Forsberg♦ Nov 11 '14 at 12:14
    
@SimonAndréForsberg I updated the question –  Leonid Semyonov Nov 11 '14 at 12:34

3 Answers 3

up vote 7 down vote accepted

As a matter of principle, an interface definition should not include implementation. These abstract classes are by definition implementations, even though they are mostly empty and seemingly harmless. As such, yes, you should move the inner abstract class definitions outside.

There are different kinds of "outside":

  • Outside of the class but in the same file
  • Outside of the class in other file(s)

If they are in the same file, they cannot be public.

If they are in different files, they can be public.

Either way, to achieve the cleanest separation of interface and implementations, I recommend to move them to different file(s).

share|improve this answer
up vote 2 down vote

If you are using Java 8 you can use default methods in the interface. They are both good and bad at the same time.

public interface InternetDependentFlow {
    default void onInternetIsAvailable() {
    }
    default void onInternetIsNotAvailable() {
    }
}

On the one hand, they provide a default (empty) implementation so you don't have to override both methods. On the other hand, it's not as easy for the classes implementing this interface to know what they should implement (although the options can be easily figured out by using Ctrl + Space).

The fact that your InternetIsAvailable and InternetIsNotAvailable even exists in the first place is telling me that you often want to implement one function but not the other.

By the way, I think your name InternetDependentFlow says nothing, pretty much. A better name would be InternetConnectionListener.

Also, some libraries which have plenty of listeners with many methods often provide a default easy-to-subclass class that provides empty implementations already.

public class InternetConnectionAdapter implements InternetConnectionListener {
    public void onInternetIsAvailable() {
    }
    public void onInternetIsNotAvailable() {
    }
}

If you use this InternetConnectionAdapter, then you won't have any use for your current InternetIsAvailable and InternetIsNotAvailable classes. Each time, you just subclass the InternetConnectionAdapter and override the method you want - leading to very similar functionality as that of how default methods in an interface works.

share|improve this answer
up vote 4 down vote

I agree with Janos that, as you currently have it, the code will be improved by putting the static inner classes in regular abstract class files.... but, I would go further than that. Your code has the feel of being inverted. I would recommend you have two interfaces:

public interface InternetIsAvailable {
    public void onInternetIsAvailable();
}

and

public interface InternetIsNotAvailable {
    public void onInternetIsNotAvailable();
}

Then, you should have your abstract classes extend just the interface they depend on.

If you need to, you can implement both interfaces in aother interface, or an abstract class, to get both methods in one place.

It strikes me as being unnecessary for classes to implement event handlers for events they are not interested in... why implement the 'Not available' event when all you are interested in is when it becomes available, or visa-versa.

share|improve this answer
    
I updated the question. Actually I need objects that have two methods, so there is no use to have two interfaces. –  Leonid Semyonov Nov 11 '14 at 12:29

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.