1

For design of an API in java, I came up with following pattern to fulfill certain requirements which are listed here

  • actual public API class should be implemented as a final class, to prevent inheritance and possible misuses
  • actual public API class should not expose anything more than required methods.
  • separation of API classes and internal implementation into different packages
  • scope for extensibility or evolution in both public and internal classes

the sample code is as follows:

package external;

import internal.AbstractProduct;

public final class Product extends AbstractProduct{

    protected Product(int a, int b){
        super(a,b);
    }

    @Override
    public int result(){
        return super.result();
    }
}

public class ProductFactory {
    public static Product createProudct(int a, int b){
        return new Product(a, b);
    }
}

and internal classes are as follows:

package internal;

public abstract class AbstractProduct {

    private final AbstractProduct impl;

    protected AbstractProduct(int a, int b){
        impl = new ProductImpl(a, b);
    }

    protected AbstractProduct(){
        impl = null;
    }

    protected int result(){
        return impl.result();
    }
}

class ProductImpl extends AbstractProduct{

    private int a;
    private int b;

    ProductImpl(int a, int b){
        this.a = a;
        this.b = b;
    }

    @Override
    protected int result(){
        return a*b;
    }
}

Although it works fine and also has appropriate access levels, but I only have beginner level skills with design patterns or API design, so it seems difficult for me to spot possible glitches with this. So are there any problems with this Or is it some already practiced pattern?

cpz
  • 1,002
  • 2
  • 12
  • 27
  • 1
    http://codereview.stackexchange.com – Suresh Atta Sep 27 '13 at 04:53
  • Thanks @sᴜʀᴇsʜᴀᴛᴛᴀ, wasn't aware about "Code Review". So shall I delete and repost in CR or what? – cpz Sep 27 '13 at 04:56
  • 1
    IMO this question shouldn't be deleted. Apart from the basic non-flexible factory method pattern, your design is very odd. Why to have `AbstractProduct impl` marked as `final`? Looks like you wanted to implement [decorator](http://en.wikipedia.org/wiki/Decorator_pattern). – Luiggi Mendoza Sep 27 '13 at 05:01
  • And yes, there are many flaws in this code. It will be better if you post what's the problem you're trying to solve here or what are you trying to accomplish with this exercise to get better guidance. – Luiggi Mendoza Sep 27 '13 at 05:03
  • @LuiggiMendoza final with `impl` is not really necessary. Please have a look at comment http://stackoverflow.com/a/19043032/1443529 which I posted on this answer in which I explain my thought process behind this design. – cpz Sep 27 '13 at 05:12
  • Yes I've read that comment. You say *`AbstractProduct` serves as an internal common contract between `Product` and `ProductImpl`. And also this way I can flexibly add/remove internal methods in `ProductImpl` to evolve or extend.* but your code is pretty inflexible. I don't understand the **real** problem i.e. the functional requirement. Explain it so people here could help you to get a better design on the problem. – Luiggi Mendoza Sep 27 '13 at 05:17

3 Answers3

4

The only design pattern you're trying to implement is Factory Method in ProductFactory class. That's the only design pattern wannabe.

Since your current code is very inflexible, the whole can even be considered as an anti-pattern, more specifically:

  • Poltergeist, since Product just exists to execute ProductImpl#result.
  • Call super, since Product only uses super calls.
  • Accidental complexity, even if the process is more than a simple int multiplication.
  • Cargo cult, since you don't still realize why and when to use design patterns.

(and probably more...)

Explanation: your factory method pattern is very inflexible. Note that Product class is public but has a protected constructor (even marked as a final class, which is odd: why to have protected methods on a class that can never be inherited?), meaning that ProductFactory should at least be in the same package as Product.

As noted in my other comments directly on your question, it will be great if you explain the functional requirement to receive better and more accurate help on your design.

IMO in order to learn about design patterns, it will be better to go to real world examples instead of keep reading more and more about them on the net, and then start practicing. I highly recommend this Q/A from BalusC (Java and Java EE expert): Examples of GoF Design Patterns in Java's core libraries

Community
  • 1
  • 1
Luiggi Mendoza
  • 85,076
  • 16
  • 154
  • 332
  • Okay, I guess I will need sometime to explain, I am really bad at explaining things. will also try to detail out functional requirement. – cpz Sep 27 '13 at 05:43
  • And thanks for pointing out anti-patterns! Btw `ProductFactory` is assured to be in same package, although as of now there is nothing much for factory to control about creation of 'Product's but in future there might be so I kept this simple factory. – cpz Sep 27 '13 at 05:48
  • @cpz answer edited to include real world examples of design patterns in Java SE and Java EE. – Luiggi Mendoza Sep 27 '13 at 05:51
  • wow, that answer(@BalusC 's) is really something! And actually I am not blindly after the design patterns, but considering some API design recommendations and constraints I wanted to come up with optimal design, and then I wanted to see if this design matches some pattern, or if there is any mistake in it. I should prepare the explanation for the actual requirements soon to further discuss it. – cpz Sep 27 '13 at 05:58
0

Why do this?

protected AbstractProduct(){
    impl = null;
}

This will cause a NullPointerException when calling result().

I also don't see the point of AbstractProduct and ProductImpl. Just put the code in Product.

I would also question why you need a ProductFactory if there is only one implementation but if you plan on having future implementations then it would be OK.

dkatzel
  • 31,188
  • 3
  • 63
  • 67
  • Yeah there is possibility for future implementations of `Product`. So factory is okay. And `Product` is part of API for which I have to provide javadoc, so to keep it clean and minimal I just wanted to have actual required methods in Product. And `AbstractProduct` serves as an internal common contract betweeen `Product` and `ProductImpl`. And also this way I can flexibly add/remove internal methods in `ProductImpl` to evolve or extend. – cpz Sep 27 '13 at 05:08
  • You can have internal methods in `Product` that don't become part of the public API, just make them private. Then when making the Javadoc set it to only create html for public methods. I just worry you are over complicating things. If you haven't already read it, I recommend Jaroslav Tulach's _Practical API Design_ which covers this topic very well. – dkatzel Sep 27 '13 at 05:13
  • I agree that this is kind of over-complicating but if I have everything in `Product` then will somehow extensibility or evolution be affected? and in case of my design is there advantage with extensibility or evolution of overall API design. I got that book very recently, haven't gone thru it completely, but it seems difficult to consider all the things at once and adapt for the project :-/ – cpz Sep 27 '13 at 05:18
  • @dkatzel - Good advice, but sometimes (rarely) not practical. In order to do its job a class sometimes needs help from collaborators. So you can use package visibility, or avoid exposing the internals, with the Abstract Factory. . . I'm not disagreeing at all, just pointing out a possible exception. – Jasper Blues Sep 27 '13 at 05:20
  • @JasperBlues in this case, I don't think we need a collaborator to multiply 2 numbers together @cpz since `Product` is already final, I don't think adding private methods will change its extensibility at all – dkatzel Sep 27 '13 at 05:24
  • @JasperBlues I first time heard about Class Cluster pattern and read about it just now, and yes it looks similar to that. Actually the sample example I chose is not appropriate to discuss all the possibilities clearly but in future There will be another kind of `Product` which will have to satisfy this internal contract `AbstractProduct` and might have separate implementation. so here `AbstractProduct` will be only public class which is internal and can provide different implementations of product. – cpz Sep 27 '13 at 05:33
0

If it were me, I'd break it down like this:

package external;

import internal.InternalProduct;
import internal.ProductImpl;

public final class Product {

    private final InternalProduct internalProduct;

    Product(final InternalProduct internalProduct) {
        this.internalProduct = internalProduct;
        assert this.internalProduct != null;
    }

    public int result() {
        return this.internalProduct.result();
    }
}

public class ProductFactory {
    public static Product createProduct(final int a, final int b) {
        return new Product(new ProductImpl(a, b));
    }
}

With the internal-facing package contents as:

package internal;

public interface InternalProduct {
    int result();
}

public final class ProductImpl implements InternalProduct {

    private final int a;
    private final int b;

    public ProductImpl(final int a, final int b) {
        this.a = a;
        this.b = b;
    }

    @Override
    protected int result() {
        return a * b;
    }
}

You don't need an abstract class, and you definitely don't want Product extending it. You should be using encapsulation here. This protects your Product class from changes to the internal interface - methods can be added without Product getting them unless you choose it.

If you wind up with more implementations and you want an abstract parent, you can choose whether or not each implemention extends it because your external package is relying on the interface.

Eric Stein
  • 13,209
  • 3
  • 37
  • 52