Better OO design

[Dan John]

Below are 3 examples of code.. Which one is better OO design?

 
(A)
public class Work
{
   private List<String> names;
   private HashMap<String, String> relationships;
 

  public Work(HashMap<String, String> input)
  {
    relationships = input;
    names = new ArrayList<String>();
  }
 
   public void doSomething()
   {
      //use both variables in the method
   }
 
}
 

(B)
 
public class Work
{
  private List<String> names;
  private HashMap<String, String> relationships;
 
  public Work(HashMap<String, String> input)
  {
    relationships = input;
    names = new ArrayList<String>();
    doSomething();
  }
 
   public void doSomething()
   {
      names.clear();
      //use both variables in the class
   }
}
 

(C)
public class Work
{
  
  private HashMap<String, String> relationships;
 
  public Work(HashMap<String, String> input)
  {
    relationships = input;
    
  }
 
   public void doSomething()
   {
      List<String> names = new ArrayList<String>();
      //use both variables in the class
   }
 

[Sid Murrey]

It seems that your post boils down to two questions:

Is it preferable to use local rather than instance variables when possible?
-> Pushing variables down the scope tree is always good, in my opionion. It depends on whether you need to have access to your ArrayList later on or if it is just a temporary variable.

Should objects be initialized when they are instantiated or later on?
-> If the initialization does not perform tremendously huge tasks (that require lots of CPU time or memory) then I would always prefer to initialize what you can in the constructor, since it makes using the class much easier. What if another developer uses the class and forgets the call to doSomething()?

Cheers,
Ced

[Dan John]

The ArrayList will be used later on in another method. I want the ArrayList to be populated after doSomething() is called. Now, given those choices as above would it make a difference if the method signature of doSomething was changed from:

    
  public void doSomething()
 
to 
 
public List<String> doSomething()
{
   //add some stuff to the array list and return it.
}
 

[Sid Murrey]

Originally posted by Dan John:
The ArrayList will be used later on in another method. I want the ArrayList to be populated after doSomething() is called. Now, given those choices as above would it make a difference if the method signature of doSomething was changed from:

    
  public void doSomething()
 
to 
 
public List<String> doSomething()
{
   //add some stuff to the array list and return it.
}
 

It does not really change anything whether you just have function with no arguments that operators on an instance variable, one that returns a reference to a new ArrayList or you pass an already created ArrayList as a parameter. It's more a matter of style, I guess. However, passing the list as an argument eases testing of the method and class!

Anyway I would limit the method to be private and call it from the constructor.
[ July 10, 2008: Message edited by: Cedric Meury ]

[Dan John]

Thank you for your input.

[Rodrigo Tomita]

Hi, Dan,

Actually I believe it is more than just a coding style. If you're going to be purist about your OO design, you should think about what exactly your class is and especially how other classes will interact with it.

For instance, in your constructor, you will be given a Map (relationships). That map will be part of the internal state of the objects created from your class. Do you want that to be visible to others? If that map is changed, do you want your object state to be changed as well? Or should that be kept private and it would have a method like addRelationship()?

The same logic applies to your List (names). Is that part of your object's state? So it should be an attribute - because if it is truly part of its state, it most likely will be used elsewhere.

About your doSomething(), as Cedric mentioned, there is always the chance that someone else forgets to call it. So, I believe you would need to think if it is part of the object's initialization or some service provided by it

If it is initialization, you should call it from your constructor and it should be private. Or, if you need some processing, then it could be public. In that case, you could also keep a flag like "initialized" to know that your objects are ready to service.

Hope this helps - although it has more questions than answers