Parallel Inheritance

There are two(+) associated classes A, B, and every sub type of A is associated with a (different?) sub type of B. e.g ProductEditor provides UI for a Product. Curtains and Books are different type of Products. CurtainEditor provides UI for Curtain. BookEditor provides UI for Book. And so on. Is this a CodeSmell?

If there is duplicate parallel relationship child classes, managing the duplicate relationship can be a problem.

class Product
{
}

class Book extends Product
{
}

class ProductEditor
{
  private Product product;
  public void setProduct(Product prd)
  {
    this.product = prd;
  }
}

class BookEditor extends ProductEditor
{
  // need to be careful?
  private Book book;

  public void setProduct(Book book)
  {
    this.product = prd; // need to call super also
  }
}


BookEditor bookEditor = ...;
Product product = new Book(...);
bookEditor.setProduct(product); // calls base class method.
Book book = new Book(...);
bookEditor.setProduct(product); // calls chile class methods.


Adding duplicate relationship for convenience is bad. Apart for code duplication, if not done correctly, there could be dangling/spurious references.
How ever I don't think this is a code smell (due to its -ve meaning).In fact relation ships among parallel inheritance hiearchies is common in many frameworks (is a good smell?). Are not these relations cornerstone for template methods?When such relation ships exist,

• Document them and advise against duplicate relations.
• If needed provide a controlled relation ship managers for setting relations among the participant Objects (do we need a DIP for this?)

What should methods return arrays or Collections?

To prolong Paul River's Weblog: By returning arrays we have a temporary advantage of typed objects(untill we java 5). There are many other advantages if a method returns Collections, We can decorate collections as needed.

• We can implement lazy loading returned objects very easily (with arrays also it may be possible by proxies etc in a long way).
• We can easily return (private) inner data (unmodifiable), instead of returning new cloned array each time.
• We can share data between multiple threads.
• Collection is as simple to use as an array type.

In summary returned collection can be changed to any sub type collection (which is very very useful from implementors perspective) with minimal client changes.

Method Overloading

There is a chance of surprise/confusion in using methods overloaded with subtype arguments (Effective java #26). e.g.

void foo(String s);
void foo(Long l);
void foo(Object o);
...
// in client code
Object x = "Some String";
foo(x); // call actually goes to foo(object).

Differenent method names instead of overloading is more elegant . e.g see below Bladestore interfaces to delete blades first one with overloaded and second one with different methods.
boolean delete(Blade blade);
boolean delete(long bladeID);
boolean delete(Blade[] blades);
boolean delete(String bladeName);

boolean delete(Blade blade);
boolean deleteById(long id);
boolean deleteByName(String name);
boolean deleteAll(Blade[] blades);

interface with different method names is better, It make intent absolutely clear.

Adding methods like is excess (overaction).

boolean deleteByName(char[] name);
boolean deleteById(Long l);

to second interface is an unnecessary(smelly) overloading. We are not adding any extra benefit.

Moral of the story : Overload only when method intent is absolutely same, method is useful and there is no possibility of confusion.

Methods with too many parameters

Method with too many parameters (esp in exported api) is a Codestench (?). It makes an API usage a pain. IMO 4-5 parameters should be the practical maximum.

I have seen them evolve in two (anti) patterns. One is a desire to create a feature rich API. And in doing so adding new enhancements very intrusively, e.g by adding a parameter for each parallel feature in existing methods. In turn producing a polluted feature creepy (YAGNI) API, making it very difficult for normal situations.

e.g.
// Normal API to find a blade in database by a its id a long type.
Blade blade = bladeFinder.getBladeById(id);

Developer modified above finder to search by multiple id types (by name, key, by group etc addin one param) , from multiple data sources(adding three params), and multiple service types (ejb/jms/corba servers) in total adding five more parameters.

// Ugly
List blades = (List) bladeFinder.getBlades(idType, id, userid, password, datasourceName, servertype);

In next version developer added a new feature to get data in different formats XML, formatted HTML, Blade object etc (adding one more param for dataformat). As part of this new version a new parameter object is created with all the parameters from old previous finder method + one more to support dataformats. A Response object is also added to wrap return data, exceptions, some more methods. Usage pattren changes to

// The ugly
BladeRequest request = bladeRequestFactory.createReadBladeRequest(idType, id, dataFormat, userid, password, datasourceName, usingJMS, ...);

BladeResponse response = bladeRequestProcessor.processRequest(request);

If (response.getAttribute(somecode).equals(successcode))
Blade blade = (Blade) response.getObjectType(OBJ_FORMAT);
else
String failMessage = (String) response.getFailureObject();


Other pattern is a need/plan to develop very loosely coupled modules. These modules agree only on a minimal set of interfaces and see world very differently.
e.g. proverbial OR mismatch. One layer looks a BladeObject and other looks it as a Map.

Possible refactorings ?

• Make one or more parameter objects with all the parameters. We are moving problem from one place to other place.
• Setters instead of constructor for Parameter object, with possible defaults for some. (confusing some times, difficult to document etc)
• A magic container like properties(map), that server assumes a default for missing properties. (confusing some times, difficult to document etc)
• Write Facades to hide ugly code(If we can't change any interfaces or if it is a result of a loose coupling design).
• Remove unused features/parameters, computable parameters if any.
• If a parameter is an enumeration provide different methods for each type.
  

The above example can be refactored by removing unneeded features data source (user and password too) parameter. And adding new Criteria a parameter objects and new methods for each data format.

// Refactoring to below
Criteria criteria = ...; // to make a search expression
List /**/blades = bladeFinder.findBlades(criteria);
//or xml as String
List /**/ blades = bladeFinder.findXmlBlades(criteria);

is synchronization not enough?

java5 has a class called Lock, with methods lock() and unlock(). They can be used to lock in one block and unlock in another block (even in different methods, but needs to be explicitly unlocked unlike synchronized). Lock is termed as an enhancement to synchronization

Now we can write code like
Lock a = ...;
Lock b = ...;
{
a.lock();
}
...;
b.lock();
...;
a.unlock(); // reversing the order.
{
b.unlock();
}

Where to use these, I am waiting for a execuse :)

Six Simple Rules

1. Clarity and simplicity are of paramount importance.
2. Modules should be as small as possible but no smaller.
3. Code should be reused rather than copied.
4. The dependencies between modules should be kept to a minimum.
5. Errors should be detected as soon as possible after they are made, ideally at compile time.
6. You should not slavishly follow these rules, but you should violate them only occasionally and with good reason.

Jashua Bloch in Effective java.