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.

To parameterize or internalize ...

Hi,

I was discussing the differences between these two snippets:

someObject.execute( dbTransaction );

And:

someObject.setDBTransaction( dbTransaction );
someObject.execute();

It dawned on me that the reason I prefer the second is because the dbTransaction is an /intrinsic/ attribute of someObject. In other words, the execute() method cannot preform its most basic task if it does not have a reference to a valid dbTransaction instance.

This led me to generalize about parameters to functions and procedures.

If the value of a parameter used by a method does not fall within a range, then chances are it is not a parameter, but something that the object needs to know.

If the value of a parameter used by a method does have a range of values and is only required for that one method, then it does not necessarily need to be internalized.

The result is that you'll find that many method calls really should take the following form:

object.setX( x );
object.setY( y );
object.setZ( z );
object.performTask();

Rather than:

object.performTask( x, y, z );

I've also noticed that this tends to improve reusability.

Thoughts?
Dave Jarvis Send private email
Wednesday, November 28, 2007
 
 
Apart from the fact that yours is more verbose, using a setDBTransaction method requires someObject to have a (long-lived) reference to the dbTransaction as part of its internal state.

What if someObject is long-lived, whereas dbTransaction has a short life-time?
Christopher Wells Send private email
Wednesday, November 28, 2007
 
 
I'm not a big fan of the pattern:

  setFoo();
  setBar();
  execute();

Almost across the board, I've found that this sort of class interface makes it more difficult to write thread-safe objects. 

I tend to think of things this way:  "Properties" of classes are mostly meant to provide read-only acess to internal object state.  "Methods" of classes can be thought of as "Requests to modify the internal state of an object in an atomic way."

In the model you mention, where you call a bunch of 'set' functions and then later call 'execute', your class is now holding "potential future state" instead of just "current state."

This can create coupling problems.  If a dumb coder makes the 'setter' functions depend on each other (Example, SetBar() uses a value of Foo previously set by calling SetFoo()) then every client of your class must know that calls to SetBar() must only follow calls to SetFoo().

In the real world, people change code in dumb ways to create these sorts of dependencies.

I think a better model is to try to keep a minimal amount of state in an instance of a class that comprise the valid, current state of the object.  If you need to provide a method that performs some complex operation, I think that, generally speaking, you are better off defining a parameter class representing the arguments, and passing THAT into Execute().

As an exception, I think that there is a design pattern named "Command" that is more similar to the model you propose.  I haven't used it, but someone I work with someone made an argument for using it under some circumstances.  I think it had to do with the fact there were many types of classes, and the only thing that they had in common was that they COULD be executed. In this case, the execute function couldn't possibly take parameters, since each class could have vastly different requirements as far as parameters.

I'll be interested to hear how this debate runs.
Meganonymous Rex Send private email
Wednesday, November 28, 2007
 
 
What would be the benefit of

object.setX( x );
object.setY( y );
object.setZ( z );
object.performTask();

over, say,

object = new Object123( x, y, z );
object.performTask();

In the first case, what happens if the programmer forgets to call 'setZ'?  Must the programmer dig into 'performTask' to see what internal state is required?

I would argue the following: If the parameters aren't passed into the constructor or similar 'initialize' function, I would want to see these parameters passed directly into the function that uses them.
Derek Illchuk Send private email
Wednesday, November 28, 2007
 
 
To me, whether to require the transaction in the constructor or in the Execute method is a toss-up.

But either is preferable to requiring setting the transaction as a separate step.  An API should have as few "You just have to remember" points as possible.

If the transaction is always required, then the API should always require it.  Even if it's not always required, I'd prefer overloaded constructors or Execute methods (one with transaction parameter, one without) rather than just a property that you may or may not remember to set.
Kyralessa Send private email
Wednesday, November 28, 2007
 
 
Hi,

Derek wrote, "In the first case, what happens if the programmer forgets to call 'setZ'?"

The behaviour should be the same as whatever would happen if the programmer called execute() with invalid parameters. That is, the execute() method should handle invalid data, just as it should handle an invalid state. It should also throw a proper exception to pin-point the problem.

Meganonymous Rex: Yes, this closely resembles the command pattern. You mentioned that it creates a dependency with the assumption that for every public set accessor there is a corresponding *public* get accessor. I agree that this could cause bad dependencies.

I, personally, believe public get accessors as evil. As such, I tend to make them either protected, private, or off-limits (see the JiGo API for how to make this work http://www.davidjarvis.ca/jigo/2.1/api/). The potential for tight coupling drops away. The get accessor debate has already been discussed here (years ago), and is probably best left mentioned in passing.

Christopher Wells: It is initially verbose, yes. But it lends itself to terse subclasses. It can also reduce code duplication (which can otherwise be a huge maintenance burden). As for the lifetime differences, if the dbTransaction is short-lived, then you probably should do it as follows:

someObject.execute( dbTransaction );

Which can be coded in many ways, including:

someObject.setDBTransaction( dbTransaction );
someObject.execute();
someObject.setDBTransaction( null );

And, again, the execute() method should be responsible for validating the objects it uses before it uses them. It's not a hard-and-fast rule, but a guideline to help increase reusability, reduce tight coupling, and decrease the maintenance burden of duplicated code.

More thoughts?
Dave Jarvis Send private email
Wednesday, November 28, 2007
 
 
Dave,

Could you give a more in-depth explanation for each of the following?  I'm just being curious here, as always.

> increase reusability, reduce tight coupling, and decrease the maintenance burden of duplicated code.
Derek Illchuk Send private email
Wednesday, November 28, 2007
 
 
+1 Derek

I immediately realized that after I clicked "Submit" on my first reply.  However, it seems to me that your "constructor takes the parameters" applies best to the command pattern itself, and not on a general class upon which the Execute method may be called more than once.  Is that what you intended to convey?

Dave: Exposing too much internal information is bad.  However, an instance of a "stock" or "bond" will probably always have a "price" attribute.    So: make it a function that returns the value.

What's bad is making the member variable public.  Providing a public method that returns the attribute doesn't seem so evil to me at all. 

Another reason that the:

  SetX();
  SetY();
  SetZ();
  Execute();

Paradigm is bad is as follows: under this scenario, I think you're required to do locking outside. (If you're being threadsafe.)  This is easy to do with C# because you can just call lock() on the object, but not as easy in C++.  What if you're interested in making a class that handles the locking for you?

Execute(X, Y, Z) fits the bill here.  The implementation of Execute could grab a critical section (that could be a member variable.)  Your proposal would require:
  1) The object to be able to hand its critical section to the caller (making it a public attribute, which you've already said you don't like), or the caller to agree to use some same critical section ALWAYS when calling these functions..
  2) All callers must know lock, period.

Generally speaking, I think that APIs that require less knowledge on the part of the caller should be preferred to APIs requiring MORE knowledege.

Practically speaking, I think that designs such as the one you propose, while maybe OK initially, tend to be corrupted by maintenance programmers and descend into spaghetti land very quickly.

That said, software design is about tradeoffs, and there are probably situations where both methodologies have merits.  My answers are borne from my experience working on an old codebase with questionable programming, where there's a huge need to be careful about making changes.

This is at least a  great discussion.
Meganonymous Rex Send private email
Wednesday, November 28, 2007
 
 
Dave: Saw your site, Love the Lorax.  Maybe we should send a few copies over to China where all the Truffulas are being whacked at this very moment.
Meganonymous Rex Send private email
Wednesday, November 28, 2007
 
 
To expand on the original example, I think you could go either way. The interesting things about the DbTransaction, for example, are that:

1) The transaction is optional
2) If you do multiple things (say run multiple sql statements) it reuses the same transaction object

These qualities make a property make sense to me. However, if either of them were not true, I'd lean towards the parameter.

There's no hard and fast rule here, of course.
Chris Tavares Send private email
Thursday, November 29, 2007
 
 
Dave Jarvis:
"I, personally, believe public get accessors as evil."

Huh. Interesting.

I have pretty much the opposite opinion. Public getters are great, but public setters are usually evil.

I try to make my objects immutable, if at all possible. Limiting mutability makes it simpler to create threadsafe code, and also helps ensure that classes are always in a state that honors their contract invariants.

If possible, I like to set all the internal state of the object in the constructor. If any of the constructor arguments are illegal (or inconsistent with one another), I throw an exception. That way, I minimize the likelihood of invalid objects floating around.

Public setters make it very hard to enforce the contract invariants, since you have to keep checking the validity of the object all over the place. (You do validate that your objects always honor their contract, right?)

So, to answer the question above, I really don't like this style at all:

  Executable x = new WizBangExecutable();
  x.setWiz(wiz);
  x.setBang(bang);
  x.execute();

I'd so much rather see something like this:

  Executable x = new WizBangExecutable(wiz, bang);
  x.execute();

And I HATE HATE HATE this:

  Executable x = new WizBangExecutable();
  x.setWiz(wiz);
  x.setBang(bang);
  x.execute();
  x.setWiz(otherWiz);
  x.setBang(otherBang);
  x.execute();

I think executable objects (commands, threads, etc) should throw an exception if you try to call the 'execute' or 'run' method more than once.

But I'm kind of a picky bastard.
BenjiSmith Send private email
Thursday, November 29, 2007
 
 
Hi,

Derek, you asked for some examples.

First, I'd like to argue against currying parameters into a structure (whoops! I mean "class") for the sake of reducing the number parameters. Ultimately it becomes the same as what I originally proposed, except now you have an additional class to maintain, and expose data best left hidden. In simplified terms:

class C {
  int p1, p2, p3;
};

Results in a very familiar:

c.p1 = 1;
c.p2 = 2;
c.p3 = 3;
object.execute( c );

That is not object-oriented. Writing functions that operate on parameters is easy to grasp, but comes with the pitfalls of procedural programming. Encapsulation is not as easy to grasp, yet when executed properly, helps keep data safe.

Imagine this:

dbCall.execute( getDBTransaction() );

You can only reuse that method if you have an instance of a database transaction. If you only had a database connection or only had a callable statement, you'd have to overload:

dbCall.execute( getConnection() );
dbCall.execute( getCallableStatement() );

But all these tasks are conceptually the same: you want to execute a database call. What you want to say is:

dbCall.execute();

All three types of execute should be rolled into a single, reusable method:

dbCall.setDatabaseSource( getDBTransaction() );
dbCall.execute();

Next, if there are multiple calls to execute, the database source need not necessarily be provided each time. This avoids code duplication. Consider:

dbCall.execute( getDBTransaction() );
dbCall.execute( getDBTransaction() );
dbCall.execute( getConnection() );
dbCall.execute( getConnection() );
dbCall.execute( getConnection() );
dbCall.execute( getCallableStatement() );
dbCall.execute( getCallableStatement() );
dbCall.execute( getCallableStatement() );

Versus:

dbCall.setDatabaseSource( getDBTransaction() );
dbCall.execute();
dbCall.execute();
dbCall.setDatabaseSource( getConnection() );
dbCall.execute();
dbCall.execute();
dbCall.execute();
dbCall.setDatabaseSource( getCallableStatement() );
dbCall.execute();
dbCall.execute();
dbCall.execute();

Calls to getConnection(), getDBTransaction(), and getCallableStatement() are no longer duplicated.

The point about reusing the method follows naturally into maintenance.

Eventually, you might find that the dbCall object can get a handle to the database by itself. Or the DBA says you have to use a database connection pool -- which is not available to any of the callers. Or you figure its a smart idea to acquire database connectivity from a Singleton.

In the first case, you'd have to change eight lines. In the second case, you *remove* three, and all the execute() lines remain unchanged.

Essentially, because you don't know how this /intrinsic/ requirement is going to change in the future, you remove it from the method signature. The caller is no longer forced to supply a parameter that the callee needs to do its duty. Thus decreasing the coupling between caller and callee.

Meganonymous Rex: What I like about this is that it can help with two things:

(1) Careful construction of methods that operate on data. (More error condition and exception handling.)

(2) Improved documentation. People have asked: How does the developer supposed to know what needs to be set before invoking the method?

In my day, we called it "documentation". It went something like this (no need to cover your eyes, it isn't all that scary):

/* DESCRIPTION
 * Uses a database connection to make a stored procedure
 * call.
 *
 * PRE-CONDITIONS
 * (1) An open database connection has been set. See
 * the setDatabaseSource() method(s) for details.
 * (2) The IN parameters have been set, in call order,
 * with successive calls to setInParameter( Type, String ).
 * (3) The name of the package has been set using
 * setPackageName( String ).
 * (4) The name of the stored procedure has been set
 * using setProcedureName( String ).
 *
 * POST-CONDITIONS
 * (1) OUT parameters ... etc.
 */
public void execute() throws SQLException {
  ...
}

These days you can get fancy with Doxygen or JavaDocs.

My favourite way of documenting source code, though, is shown ~1 minute and 40 seconds into this short video clip showing a conceptual 4D programming environment: http://www.youtube.com/watch?v=u3QqjhzhnAw

As for thread safety:

(1) Retain the parameters in the method signature
(2) Clone the object (or otherwise create another instance)
Dave Jarvis Send private email
Thursday, November 29, 2007
 
 
Hi, Ben.

  Executable x = new WizBangExecutable( wiz, bang );
  x.execute();
  x = new WizBangExecutable( wiz2, bang2 );
  x.execute();

I see that as a waste of a perfectly good, reusable, autonomous object. I didn't really want to do this, but just briefly:

  Person p = new Person( hair );
  Hair h = p.getHair();
  h.dye( blue );

The person, in this case, has absolutely no say in the matter on whether or not they wanted their hair dyed blue. Data is not encapsulated. Delegate, instead:

Person p = new Person( hair );
p.changeHairColour( Colour.BLUE );

class Person {
  void changeHairColour( Colour colour )
    throws CustomerComplaint {

    if( colour.equals( Colour.BLUE ) ) {
      throw new InvalidHairColour( colour );
    }

    getHair().setColour( colour );
  }
}

Notice, too, that p.changeHairColour( Colour.BLUE ) is more reusable than getHair() followed by setColour(). It is also more extensible and easier to maintain (one line of code versus two).
Dave Jarvis Send private email
Thursday, November 29, 2007
 
 
> As for the lifetime differences, if the dbTransaction is short-lived, then you probably should do it as follows: someObject.execute( dbTransaction );

Yes: you should do that if the dbTransaction is shorter-lived than someObject.

If, on the other hand, someObject is short-lived than the dbTransaction, then I think that dbTransaction should be passed to someObject's constructor.

And if you don't know which one is longer-lived, then you still shouldn't be storing one as a property of the other.

> Eventually, you might find that the dbCall object can get a handle to the database by itself. Or the DBA says you have to use a database connection pool -- which is not available to any of the callers. Or you figure its a smart idea to acquire database connectivity from a Singleton. In the first case, you'd have to change eight lines. In the second case, you *remove* three, and all the execute() lines remain unchanged.

I'll argue that it's better to change the 8 lines. I define "easily maintainable" as meaning, not so much "can be changed with minimal edits", but more "any change is guaranteed to be correct".

Consider the case where you want to add a second property or parameter:
* If it's coded as an additional property which you mut set before you invoke execute, then you need to find the places where you need to add a new "set" statement (and it's hard to maintain because you might not find all those places)
* If it's coded as an additional parameter which you pass to the execute method, then when you change the signature of the declaration of the execute method then the compiler forces you to revisit and edit all the places which need changing (and so it's easy to maintain correctly because its enforced by the compiler).
Christopher Wells Send private email
Thursday, November 29, 2007
 
 
+1 Dave, for not handing out an object's internal references, as in that hair dye example.  Reference getters break open the object's internals.

+1 Christopher, regarding changed function signatures allowing for easy maintenance.  When the required parameters are passed in directly, the error may be found at compile time.  In the set-set-set-execute case, the error may only be found at run-time.
Derek Illchuk Send private email
Thursday, November 29, 2007
 
 
+1 Benji, for preferring immutable objects and not reusing objects just because they're handy and willing to transform into what you need.  It's just cleaner, IMO.

+1 My mom, she retired last week!
Derek Illchuk Send private email
Thursday, November 29, 2007
 
 
Dave: I now understand that your problem with Get() accessors is really with accessors that return references/pointers to internal state.  I couldn't agree more.  I've no problems with accessors that provide a description of the object; but accessors that return references/pointers to implementation break encapsulation.

If the class is meant to be executed once, (as in the command pattern) I'm still a fan of the Benji/Derek style "Pass parameters in constructor."  If the class is
meant to allow execution again,  I'm a fan of passing in the necessary context with each all to Execute.

I still think that the style you describe makes it harder to do thread safety correctly and efficiently in many cases, but I will grant that each approach has some advantages in various situations.  That's what makes our jobs so interesting, I suppose: analyzing the various tradeoffs and picking the one that works best.

I'd love to work with more people like all of you who are willing to engage in a debate!

+1 This thread for promoting a thoughtful discussion

+1 My Dad for turning 65 and being in better shape than
  I am, and for still working because he loves it.
Meganonymous Rex Send private email
Thursday, November 29, 2007
 
 
Hey Dave! Thanks for the reply! (PS: I don't go by the name 'Ben'; sorry to be picky :)

Now I understand your reluctance about public getters.

I'd argue that public getters should always either return immutable objects, or they should return defensive copies. The only exception I can think of is collection classes, where returning a copy would defeat the whole point.

As for my implementation of Person, I'd still provide a getHair() method, but it'd either return a defensive copy of the person's Hair object, or instances of the Hair class would be immutable to begin with.

Then again, it doesn't make much sense for Hair to be immutable. Everyone knows that hair can be cut or colored, so an immutable Hair class would be dumb.

As long as an instance of Hair is owned by a Person, I'd think that the Person would safeguard all of the public mutators of the Hair, only granting read-only access to interlocutors. Anyone calling getHair() would obviously be given a defensive copy.

If I give you a copy of my hair, you can do anything you want with it. Dye it blue. Give me a mohawk. No problemo.

I think you're spot on, though, with the implementation of the person.changeHairColor(color) method.
BenjiSmith Send private email
Friday, November 30, 2007
 
 
And +1 to my cat, who chases little bits of string.
BenjiSmith Send private email
Friday, November 30, 2007
 
 
Benji, be careful letting your cat play with string.  Seriously, if they eat it, it can get caught up in their intestinal tract and cause major problems. 

MREX
Meganonymous Rex Send private email
Friday, November 30, 2007
 
 
Oh, I only let him play with big, chunky bits. Like a shoelace.

So he's cool.
BenjiSmith Send private email
Friday, November 30, 2007
 
 
Do you want high or low cohesion. Do you want high or low coupling?
J Phillips Send private email
Tuesday, December 11, 2007
 
 

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

Other recent topics Other recent topics
 
Powered by FogBugz