The Design of Software (CLOSED)

A public forum for discussing the design of software, from the user interface to the code architecture. Now closed.

The "Design of Software" discussion group has been merged with the main Joel on Software discussion group.

The archives will remain online indefinitely.

Design Problem: Inheritance vs Composition

Please help me design around the following restrictions:

1) Client, Server modules that share common code in Shared module.

2) The following classes are defined (for the sake of our discussion): Image, ContentType, User

3) An Image has an associated ContentType (i.e. mime type) and User (i.e. author) but they do not determine its identity. That is, the image identity is fully determined by the contents of the Image class.

4) Image and ContentType are visible in the Shared module (because both client and server have them) but User is only defined on the server module because clients should not have access to this information.

5) There are modules that extend the Server module. These modules associate more classes with the Image module. The key point is that Image will gain more associations over time.

Now it gets interesting...

6) I favor composition over inheritance (due to point #9). As such, I want class Image to remain the same even as more classes are associated with it. The only way I know of doing this is by storing a "Map<Image, associatedClass>" in a ImageManager class.

7) I am using JPA which is a Java ORM: http://en.wikipedia.org/wiki/Object-relational_mapping. JPA stores associations as part of the class. So for example, if Image is associated with a ContentType it would contain a ContentType field. It is possible to use "Map<Image, associatedClass>" in ImageManager as mentioned in point #6 however this is not covered by JPA and I'd have to use unportable code.

8) Even if use a "Map<Image, associatedClass>" this is very likely to be far less efficient than storing the associations as part of Image (this is probably why JPA encourages it). That is, reading the ContentTypeId column inside an Image table is far more efficient than using an external ImageToContentType table with two columns: imageId and contentTypeId.

9) Alternatively, I could try using inheritance instead of composition and store associations directly inside the class as fields. I would define SharedImage that contains a ContentType field. ServerImage would then extend SharedImage and add a User field.

  The problem with this approach is that ImageManager defines a method called getAllImages() which returns a set of images in the database.

SharedImageManager.getImages() would return Set<SharedImage> whereas ServerImageManager.getImages() should return Set<ServerImage>. Unfortunately Set<ServerImage> is not a co-variant return type of Set<SharedImage> so overriding the method in this manner would be illegal.

  The Server module can either manually cast Set<SharedImage> elements to ServerImage one by one, or I can define a new method alongside getAllImages() which would return Set<ServerImage>. Either way this seems ugly to me.

Any ideas?
Gili Send private email
Wednesday, August 29, 2007
 
 
Sorry, Gilli, apparently not.
AllanL5
Thursday, August 30, 2007
 
 
I may be misunderstanding, but in your final situation can you not create a templatized (i.e. generic) common class that you instantiate appropriately in your client and server libraries?

class ImageManager<T extends BaseImageType >
{
 ...

  Set<T extends BaseImageType> getAllImages()
  ...
}

your client can then create an instance of ImageManager<SharedImage>  and your server an instance of ImageManager<ServerImage>

I'm not familiar with JPA, so I'm not certain if you can just use a generic "Object" and have JPA take care of figuring out the type and how to load it from your persistence or not.


In general if you are considering inheritance, simply ask yourself if the hierarchy describes TRUE "is-a" relationships.  If the derived type "is-a" base type, then that's a good sign.  If the derived type is just appending some sort of utility method or data unrelated to the base type, then consider composting.
Dan Fleet Send private email
Friday, August 31, 2007
 
 
"Open Close Principle: Classes should be opened for extension and closed for modification." - Bob Martin

You can use the bridge pattern to extend your image classes:

interface Image {
    ImageExtension getExtension();
}

inteface ImageExtension {}

class ServerImageExtension implements ImageExtension { }
class SharedImageExtension implements ImageExtension { }


class SharedImage implements Image {
    ImageExtension getExtension() {
        return new SharedImageExtension();
    }
}

class ServerImage implements Image {
    ImageExtension getExtension() {
        return new ServerImageExtension();
    }
}

Or if you want a single image class, you can use bridge & IoC:

class ImageImplementation Image {
  final ImageExtension extension;

  ImageImplementation(ImageExtension extension) {
      // Alternatively you can have a set method
      // with a non final extension, but that's
      // a less robust.
      this.extension = extension;
  }

  ImageExtension getExtension() { return extension }

  Class<? extends ImageExtension> getExtensionClass() {
      return extension.getClass();
  }
}

Further on, some of the Image functionality can be delegated back into the extension:

  void doSomething() {
      extension.doSomething(this);
  }
Dino Send private email
Friday, August 31, 2007
 
 
Dan and Dino, replies below...

Dan,

I originally tried your approach but it quickly gets out of hand. You end up needing one type parameter per association. For example:

class Image<SpecificationType, ContentType, UserType, ...>
{}

class Specification<ImageType>
{}

class ContentType<ImageType>
{}

etc... before you know it the Image class has over five different type-parameters and the code is unreadable. I actually ran this by Gilad Bracha and he suggested that "Generics isn't the right solution for all problems. If the solution ends up looking worse than type-unsafe code then you should reconsider your approach" (I am paraphrasing of course).


Dino,

I can't use the bridge design pattern because I am exposing functionality in subclasses which is not visible in superclasses. Bridge design pattern assumes all functionality is exposed in the abstract supertype.
Gili Send private email
Sunday, September 02, 2007
 
 
It seems something is not sitting quite right in your design. It could be that
- you are breaking the LSP class design principle.
- your interfaces are not properly segregated.

Would the example below work any better? (This is the interface/behavior/implementation I was writing about on my blog http://dinosstuff.blogspot.com/2007/08/interface-behavior-implementation.html).

// Segregate your interfaces. An interface should have only one reason for change (it solves a single problem).
public interface Image {
  void show();
  void hide();
}

public interface ExtendableImage{
  ImageExtension getExtension();
}

// Tagging image extension interface
public interface ImageExtension {}

public interface ExtensionA implements ImageExtension {
  void doA();
}

public interface ExtensionB implements ImageExtension {
  void doB();
}

class abstract AbstractImage implements Image {
  public void show() {...}
  public void hide() {...}
}

class ExtendableBImageImplementation extends AbstractImage implements ExtendableImage {
    ImageExtension getExtension() {
      return new ExtensionB() {
          doB() { ...} ;
      }
    }
}

class ExtendableAImageImplementation extends AbstractImage implements ExtendableImage {
    ImageExtension getExtension() {
      return new ExtensionB() {
          doA() { ...} ;
      }
    }
}


...

In client classes you could use reflection, duck typing and X-casting - in any combination:

Image image = ... ; // get the image somehow.

// Figure out the types and execute accordingly.
if(image instanceof ExtendableImage) {
  ImageExtension ext = ((ImageExtension)image).getExtension();
  if(ext instanceof ExtensionA) {
        ((ExtensionA)ext).doA();
  } else if(ext instanceof ExtensionB) {
        ((ExtensionA)ext).doB();
  }
}

This relates to "http://www.joelonsoftware.com/items/2006/08/01.html". Again my answer is "Yes, it can", provided your have the right data and type structure.

What you would export from your packages are: Image, ImageExtension, ExtendableImage interfaces (and possibly the hierarchy of Image and ImageExtension interfaces).
Dino Send private email
Tuesday, September 04, 2007
 
 
Dino,

I agree that the primary flaw of my design is that I violate the Liskov Substitution Principle (LSP), but I don't see how your extension mechanism is any different. Doesn't it simply move from an explicit violation to an indirect one? Whether I add subclass-specific properties directly in the subclass or whether I do it as part of the "extension" mechanism isn't it the same?

PS: How does one get notified of replies to posts in Joel's forum? I only seem to be getting notifications when new topics are added, as opposed to when someone replies to my posts.

Thank you,
Gili
Gili Send private email
Wednesday, September 05, 2007
 
 
On second glance I think I need to take you up on some of the points you brought up. I read your blog and I understand a bit more of what you were saying but it is no longer clear to me why I am violating LSP. The way I see ClientImage extends Image satisfies LSP because ClientImage can always be used against methods that take in type Image.

I think what is wrong with my design is that subclasses are introducing new properties, but I guess this isn't what the LSP is addressing.

Can you please go into more detail on how I am breaking the LSP design principal or how my interfaces are not properly segregated? I believe I understand the example code on your blog but I don't understand what is specifically wrong with my own code.

thanks,
Gili
Gili Send private email
Wednesday, September 05, 2007
 
 
Gili,

"I agree that the primary flaw of my design is that I violate the Liskov Substitution Principle (LSP), but I don't see how your extension mechanism is any different."

You are right, my extension mechanism has the same problem as your desing - if I understood it correctly. If in your code, you need to if around type info to resolve which way to substitute for base classes you are in violation of LSP. This is obvious from:

if(image instanceof ExtendableImage) {
  ... // LSP is broken right here.
}

Now I belive the interfaces are somewhat mixed up because of

"I can't use the bridge design pattern because I am exposing functionality in subclasses which is not visible in superclasses."

The way I interpreted this is: subclasses have interfaces larger than necessary, most likely interfaces improperly defined / segregated.

In my example, I was trying to fix the interface problem and not the LSP - which seems to be broken straight from requirements:
"SharedImageManager.getImages() would return Set<SharedImage> whereas ServerImageManager.getImages() should return Set<ServerImage>. " with the two not having a common base class.

I hope this was helpful.
Dino Send private email
Wednesday, September 05, 2007
 
 
PS

If your code is simple and elegant, proportional to the problem it solves, then probably there is nothing wrong with it. However, if you end up writing lots of code to do "apparently" simple things, then there is something wrong either with your data structures or your class hierarchy.
Dino Send private email
Wednesday, September 05, 2007
 
 
Dino,

So I think we're both in agreement: my design is fundamentally broken because the inheritance tree is violating LSP.

I'll restart my basic requirements and see if we can come up with something cleaner.

1) Server stores images in a database.
2) Client downloads images from Server but also stores some images which do not exist on the server. For example, it might decide to cache a down-scaled version of an image it downloaded.
3) Server and Client share implementation for image manipulation.

So far so good. I think the flaw comes in #4.

4) Server-side and client-side images has more classes associated with them than the SharedImage in #3. My assumption was that the server-side or client-side could should inherit from or use composition against the code in #3 and this is where all the problem begins.

I still think that to a certain degree composition is the right way to go here. I think that an Image declared in #3 should be identical to an Image in #4 but I need some new class which will associate that Image with related classes and/or database specific information such as "id" and "version".

I've got a meeting in 5 minutes so I've got to run. I'll give this some more thought and try continuing this post sometime tomorrow :)

Thanks,
Gili
Gili Send private email
Wednesday, September 05, 2007
 
 
First of all, the server side use cases != client side use cases less a common set, probably small.

For example

common
1) Image transformation
  scenarios:
   


server
1) CRUD image operations
 scenarios:
    a) image uploading (create, update).
    b) image deleting
    c) image search based on name, category, creation date, size, etc.
2) image categorizing
  scenarios:
      a) category CRUD operations (depends on 2b and 2c).
      b) add / remove category to / from picture(s)
      c) remove picture(s) from category
3) image downloading
  scenarios:
    a) download thumbnails only (minimize traffic).
    b) download thumbnails for pictures that changed since <date>
    c) download full size pictures.

client
1) query for images
  scenario:
      a) full query: query for thumbnails and cache locally (uses server 1c, 3a)
      b) incremental query: query for thumbnails for pictures that changed since last query (uses server 1c, 3b)
2) Image CRUD operations
  scenario:
      a) upload picture(s) (uses server 1a).
      b) remove picture(s) (uses server 1b).
      c) update picture info: categories, name, etc (uses server 2b, 2c)
3) Category CRUD operations
    scenario:
      a) CRUD operations (uses server 2a)
Dino Send private email
Wednesday, September 05, 2007
 
 
... I've hit the wrong button, sorry

common
1) Image transformation
  scenarios:
    a) thumbnailing
    b) other image utilities

common 1a) is used by both the client and the server. But that seems to be all there is.

I wouldn't try to have a local database which replicates the server database. That is hard to keep in sync, especially in a multi user context. Instead I would rely on a stateless protocol between the client and the server (client requesting all, then only diffs). That creates a bunch of other use cases around the protocol itself

2) Protocol
  scenarios:
    a) create request
    b) parse request
    c) create response
    d) parse response

with the client using 2a and 2d and the server using 2b and 2c

A bit on dependencies:

server -> common
client -> common, server

The first you need to define are the messages in the protocol (an .xsd maybe?). But both the client and the server depend on that protocol. At this point it is worth spending time on getting all scenarios right. At the end of this exercise, both client and server interfaces should be fairly well defined.

So in a nutshell, this is how I would approach the problem, before diving into coding. Basically I would make sure that whatever messages the client and the server exchange they make sense from a quality of server perspective:
- transaction boundaries
- network traffic & bandwidth
- caching strategies
- packaging and deployment (at least a clear idea of).

What I expect to get after all this is the correct hierarchy of interfaces on the client and the server. This is where I would dive into coding and derive the behavior and then, from behavior, the implementations.
Dino Send private email
Wednesday, September 05, 2007
 
 
Dino,

You are right that the code shared between client and server consists of two parts: thumbnailing and message exchange.

I disagree slightly on the point that the client shouldn't try to mirror the server database. I mean, it doesn't, but it does cache any images it downloads. If its local database falls out of date that's okay (just like a normal cache which expires over time).


I just read the LSP article and I think I understand new things:

1) LSP isn't against adding new properties in subclasses so long as it does not violate design-by-contract (which seems to be possible).

2) It seems my code does not actually violate LSP. That is, you can substitute a ServerImage where an Image is required and it will behave as expected. The key is that if you want *extra* behavior beyond those that Image requires then you need to subclass. I believe that this does not contradict LSP, although it does contradict Effective Java Item 7 which states "There is simply no way to extend an instantiable class and add an aspect while preserving the equals() contract."

3) If subclasses that add properties require one to change equals() then I am violating LSP, but this doesn't seem to be the case for my class hierarchy.

So while it doesn't seem to be the case that I am violating LSP it still seems to be that there is a problem because I have to down-cast. I am thinking that problem has more to do with ImageManager than anything else because it returns Set<Image> when I want Set<ServerImage> in certain cases. I'm beginning to think it actually makes good sense to define getImages() in shared.ImageManager and getServerImages() in server.ImageManager and not have an interface-level relationship between the two methods.

Anyway, I'll let you know soon whether my latest iteration of code-cleanup yields anything.
Gili Send private email
Friday, September 07, 2007
 
 
Gili,

thanks for your answer.


"I disagree slightly on the point that the client shouldn't try to mirror the server database. I mean, it doesn't, but it does cache any images it downloads."

You're right to disagree because I'm bias. My experience is that the client database has different requirements than the server one. But maybe you're after some sort of replication in which case you may have lots of reusable code. It's worth looking at the client&server db use cases. They'll make the answer obvious.

Side note: if you have separate client and server teams, things are not straight forward to organize. The common infrastructure needs to finish first before the client and the server can proceed. In my experience, that can be achieved only if allow the common part to stay one version behind. Here's the process

c/s v1.0 ---> commons v1.0 ---> c/s v2.0 ---> commons v2.0

1) commons v1.0 is extracted from c/s v1.0;
2) c/s v2.0 is based on commons v1.0 then commons v2.0 is extracted from c/s v2.0
...

The reason for this process is that commonalities in the code cannot be anticipated nor imagined to an useful level of detail. They have to extracted from already existing code. Designing "common code", "for code reuse", "frameworks" and such simply doesn't work and is a recipe for disaster and architecture astronautics. What works (again in my experience) is finding common code in a well written code base. Hence the process above.

Another big point, don't be religious about class design principles. Instead, be deliberate. The simple facts that you think about all the things you do and that you actively listen to other opinions and take what works for you is very good. This is a lot more than what most people do: you think before you act !!!

But try to be both effective and correct at the same time. You'll find out that for most cases correctness is the same with effectiveness. But sometimes the two are mutually exclusive. Maybe this is now the case. Maybe it's a good idea if you break LSP in one or two places in the code for the sake of effectiveness; trade-off correctness for effectiveness. Make sure though that you document the exception and that you don't make the exception a rule. As I wrote above: in most cases CORRECTNESS = EFFECTIVENESS

For more articles: www.objectmentor.com (I'm a Bob Martin fan :)

Cheers and let me know how it goes.
Dino Send private email
Friday, September 07, 2007
 
 

This topic is archived. No further replies will be accepted.

Other recent topics Other recent topics
 
Powered by FogBugz