3

I'm writing a handler for download/import of data. After inserting data to the database, several import jobs can be called, but usually it is just one. So there are two methods to get the import job(s), that can be overridden from the abstract base class, one being abstract, the other one using the first one.

Please have a look at my code and doc and tell me if you think this design is okay or what practice may be best. I'm particular irritated by my design to have the one writing the subclass having to implement getImportJob() just to throw an UnsupportedOperationException, it somehow came up naturally. But is the bold marked part okay or am I breaking some conventions with that?

/**
 * Returns the name and parameters of the import job that may be called
 * after the data has been added to the database in
 * {@link #insertDataIntoDatabase(Map)}.<br>
 * If the import job is called or not in the aforementioned method is
 * determined by {@link #isExecutingImports()}. If there is no import job to
 * execute ever, this method should return <code>null</code>. If there are
 * several import jobs to be done, this method should throw a
 * {@link UnsupportedOperationException} and {@link #getImportJobList()}
 * should be overridden to return the names and parameters of the import
 * jobs in the order they should be executed.
 * 
 * @return the name of the single import job that can be executed or
 *         <code>null</code> if none can ever be executed.
 * @throws UnsupportedOperationException
 *             If this method was called when there are several import jobs.
 * 
 * @see #getImportJobList()
 */
protected abstract String getImportJob();

/**
 * Returns a list of names and parameters of import jobs that may be called
 * after the data has been added to the database in
 * {@link #insertDataIntoDatabase(Map)}.<br>
 * The default implementation calls {@link #getImportJob()} and returns an
 * empty or a singleton list depending on <code>getImportJob()</code> having
 * returned <code>null</code> or a concrete object. When this method is
 * overridden to provide several import jobs to be done,
 * <code>getImportJob()</code> should also be overriden, according to its
 * documentation.
 * 
 * @return a list of names and parameters of import jobs
 * @throws UnsupportedOperationException
 *             If <code>getImportJob()</code> was overridden to throw an
 *             <code>UnsupportedOperationException</code> without this
 *             method being overridden accordingly.
 * 
 * @see #getImportJob()
 */
public List<String> getImportJobList() {
    String singleImport = getImportJob();
    if (singleImport == null)
        return Collections.emptyList();
    return Collections.singletonList(singleImport);
}

It just occured to me that I can make both methods abstract and provide two interfaces, one for single and one for multi use, each one leaving one method abstract and implementing the other one with default according to the contract... That may be a misuse of the default concept, but I think it would serve my purpose. My question still stands.

EDIT: Thanks to the answers I changed the base class to an interface without the single-Job method and added an inheriting interface with the single-Job method and a default for the Job-List method.

  • At the very least, you should throw an IllegalStateException instead, but while I obviously don't know all the details of your API, it seems very surprising and impractical to use. A getImportJobList() that either returns null or a list with a single element doesn't make much sense to me. – Vincent Savard Dec 9 '16 at 16:30
  • @VincentSavard That doesn't happen, it always returns a list as long as it returns normally. – GreenThor Dec 9 '16 at 16:33
  • That makes more sense indeed, but it still seems weird to me that it can't return more than one element and that it throws if there's more than one job. Anyway, you know more than me about it. :) – Vincent Savard Dec 9 '16 at 16:34
  • @VincentSavard The idea was to provide a default implementation so the user only has to implement getImportJob(). But if there are several import jobs, he is expected to override getImportJobList() as well. – GreenThor Dec 9 '16 at 16:36
  • 1
    No. 100% no. UOE is the pinnacle of code smells, and should only be used as a last resort. If a type does not support a behavior, it should not be forced to implement said behavior (or you violate ISP). If your type implements the interface and throws an UOE, it's violating LSP. If you're forced to throw UOE, the contract of the interface you are implementing is too broad for the type that is implementing it. – Vince Emigh Dec 10 '16 at 0:09
5

Academic: if part of your interface does not need to be implemented to satisfy your interface requirements, then your interface is poorly defined for your use case. What you really have is a different interface.

So: No, you cannot throw an UnsupportedOperationException. The exception should not even exist. "Unsupported Operations" and "Not Implemented Methods" are clearly communicated by virtue of the fact that I did not code the interface to support them. Such things are bubble gum and duct tape over poor designs.

Pragmatic: Implementing only part of an interface rather than the whole one is definitely a code smell and introduces the risk that a consumer of your implementation will experience behavior that should not be expected from your interface;use at your own peril.

One should design their systems as if all programmers are psychotic ax murderers who know where the designer lives. You would not be the first person to do such a thing, and anyone with Win32 experience would likely give you a sigh, an eyeroll, and some muttered epithets under their breath. Then they'd move on. You'd probably be safe.

  • What is the proper metaphor, then, for indicating that the consumer of your type or interface is responsible for the implementation? – Robert Harvey Dec 12 '16 at 17:24
  • You are correct, UnsupportedOperationException should not exist and the fact that the JFC documents that it may be thrown by implementations of Collection is an aberration. – user22815 Dec 12 '16 at 17:26
  • @RobertHarvey implementers are always responsible for the implementation, so I'm not sure that anything specific needs to be communicated. If the designer can anticipate that implementers will likely break the interface to get specific jobs done, then the designer should either provide the interface they anticipate or explicitly exclude it in the interests of keeping from communicating that feature from the interface. – K. Alan Bates Dec 12 '16 at 17:28
  • @RoberHarvey re:metaphor ...interface routes that support the NotImplementedException and UnsupportedOperationException are conceptually similar to nullable data columns represented in corollary programmatic interfaces. It should be taken as a matter of course that "all nullable data fields are design disappointments"; so too it follows for programmatic interfaces. – K. Alan Bates Dec 12 '16 at 17:33
  • I'll rephrase my question. In a type (let's say for this discussion's purposes that it is an abstract type), how do you communicate that there are methods which that be implemented, but for which there is no corresponding implementation in the abstract class? Do you simply let the abstract class derive from an interface containing the required methods to be implemented by the consumer of the abstract class? – Robert Harvey Dec 12 '16 at 17:44
4

Is expecting the API user to implement an UnsupportedOperationException okay?

I would say not. You're likely contravening the Liskov substitution principle. Quoting:

No new exceptions should be thrown by methods of the subtype, except where those exceptions are themselves subtypes of exceptions thrown by the methods of the supertype.

so unless your base method is declared as throwing an UnsupportedOperationException (or a superclass), that behaviour would be unexpected and would contravene the principle of least astonishment.

  • Since its a runtime exception, I thought documenting it would be enough :-/ – GreenThor Dec 9 '16 at 16:23
  • 5
    I suspect I would only read the documentation once it's thrown that exception :-) – Brian Agnew Dec 9 '16 at 16:24
  • @GreenThor and It should be. Long life to the unchecked Exceptions. But that's not the subject here :-) – Laiv Dec 9 '16 at 19:22
  • 1
    Might also want to note that throwing an UOE suggests the interface is too broad and should be segregated – Vince Emigh Dec 9 '16 at 23:12

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.