The Old Joel on Software Forum: Part 4 (of 5) - Avoiding "chaining" Procedures.

Avoiding "chaining" Procedures.

Hi,

I'm a mediocre programmer. I'm always learning and thinking, though.

I'm redesigning a dozen or so programs to use ONE architecture.

I occurred to me that one error I made 9 years ago (with the original program) was that there are too many layers in the program, e.g., FunctionA calls FunctionB which calls FunctionC, etc.

I.e., it seems like, ideally, you'd want to have one main layer controlling the program, with a set of functions/procedures and/or objects.

Previously, I'd call a function which would call another function, essentially chaining them together:

Example:

StartLesson()
....
.....
call NextExcercise()

end;

NextExcercise()
---
....
Call ShowExcercise

I know intuitively that this makes for spaghetti code. Can enyone elaborate on this?

Why it's bad, experiences you've had with that sort of "chaining"

Mr. Analogy
Friday, May 7, 2004

I always have in mind the principle of never doing three thimes the same thing.. (I don't remember the source, but I'm sure it wasn't my idea. (I remember Martin Fowler once blogged abou it, but I don't think it was his idea neither))

So the rule says:

'When you need to do something one time, duh!, just do it.'

So, if I had to show a test with one excercise, I go and do it:

StartTest()
{
// all code here to show *one* excersice
}

Ok, so far so good.

If later I needed to add a second excercis, the rule says: 'well, if you need to do something similar next time, ok, just copy it all over..

StartTest()
{
// all code here to show *one* excersice

// similar code to show excercise two.
}

Then, If I had to add *another* one and was about to copy the same code again, the rule would shout at me: ' DON'T, you moron!!, first refactor'.

So, I refactor:

StartTest()
{
ShowExcercise(1);
ShowExcercise(2);
}

ShowExcercise(int excersice)
{
// generic code to show specified excercise
}

And then, I can happily add the third one:

StartTest()
{
ShowExcercise(1);
ShowExcercise(2);
ShowExcercise(3);

}

ShowExcercise(int excersice)
{
// generic code to show specified excercise
}

Now, I agree that I could see the fact comimng that I was going to need to add more than two excercises, this is a trivial example!.. In a real system, I have noticed that If I try to factor early, I end up with layer and layers of unneeded flexibility and abstraction.

This tip has tought me to add just the amount of indirection needed.

.NET Developer
Friday, May 7, 2004

Right, don't split it off into another function if it's only called once. (Conversely, if you have duplicate code, it should be in a separate function). An exception is the top-level main loop where you're doing separate things in sequence, or a dispatch function.

And If you end up with a 200-line function, it means there's probably a better way to do it (although some algorithms may end up that large).

Ron
Friday, May 7, 2004

Just to make it clear. I would not be happy with duplicated code, no matter if it is only two times, but the point I was trying to get across was that you should not try to factor very early (this skill comes only with experience) because you might end with more indirection than it is needed.

Just remember, refactor often and Do The Simples Thing That Works.

DISCLAIMER: I am aware that refactoring without unit testing is not really refactoring. Buy you knew that I knew.

.NET Developer
Friday, May 7, 2004

If the sequence is always the same, then encapsulating that sequence in another procedure (instead of chaining) would make that clearer, e.g. (using VB syntax):

Public Sub DoLesson()
StartLesson
NextExercise
ShowExercise
End Sub

This way you don't have to read through each individual procedure to see the sequence.

But also, if your original context doesn't need to be aware of the sequence or the individual procedures, except to start them going, then put all three of them plus the sequence procedure (DoLesson) in a separate class. Make the three individual procedures Private and leave only DoLesson as Public. Then all your original context has to know is when to start them off with DoLesson.

Kyralessa
Friday, May 7, 2004

I have a similar rule (to never doing anything three times): the second I find myself copying a chunk of code to paste it somewhere else, I cut it instead and make it its own procedure.

I've written quite a few 200-line functions in my day, but I'm trying to make shorter functions that just do one thing... works a little better than I'd have expected. (=

Sam Livingston-Gray
Friday, May 7, 2004

==>I know intuitively that this makes for spaghetti code. Can enyone elaborate on this?

Interesting. In my experience, lack of doing such leads to true spaghetti code.

Sgt. Sausage
Friday, May 7, 2004

My code has become a lot more clear since I started refactoring my methods to be smaller (typically around 10 lines long). Then I can understand each method's logic at a glance. 10 well-named methods, each 10 lines long, are much more comprehensible than a single 100-line function.

Julian
Friday, May 7, 2004

>>too many layers in the program, e.g., FunctionA calls FunctionB which calls FunctionC, etc.

What you're talking about is called the Law of Demeter. The wiki page for it is ...
http://c2.com/cgi/wiki?LawOfDemeter

There's a lot of other interesting reading on it around the web as well.

yet another anon
Saturday, May 8, 2004

Is the purpose of this thread to find the most innovative spelling of "exercise" ??

HeWhoMustBeConfused
Saturday, May 8, 2004

> Public Sub DoLesson()
> StartLesson
> NextExercise
> ShowExercise
> End Sub

I have seen the above done to improve readability, but I think that code folding/#region thing in c# is you friend in this case. I just collapse all the code into regions and expand them when I need them:

Public Sub DoLesson()
#region StartLesson
#region NextExercise
#region ShowExercise
End Sub

Matthew Lock
Saturday, May 8, 2004

I am also a good fan of 10 line functions. I factor my code to be less than one screen no matter what. I like to follow the rule that it should take no more than 15 seconds to understand 100% what a function does.

That is definitely true of 90% of the functions I write myself.

Roose
Saturday, May 8, 2004

Chaining generally makes it harder for the code to be modified or tested, creates excess dependencies, and makes the code less reusable.

If calls are done like this
A -> B -> C -> D

B is dependendent on C, C is dependent on D, and
A is directly or indirectly dependent on all the others.

If done like this
A
B
C
D

Each method can then be tested and modified independently of the others. It also makes it simpler to do B without C if necessary.

Of course, sometimes a limited amount of chaining is necessary because of the nature of the program. An example is with a layered architecture where you have something like

UI -> Business Logic -> Data Access

... but the chaining should be avoided where possible, and used only when there is a deliberate architectural reason for it.

NoName
Saturday, May 8, 2004

Some people seem to view too many procedures as an evil, while other seem to view having too much logic in a single procedure as the evil.

I prefer a procedure to be relatively concise and perform a few operations. I'd much rather be calling 15 procedures than having one large glop of code in a single proc.

I don't subscribe to the notion that the only reason to separate some logic into it's own procedure is if you need to call it more than once. I like to separate it just because clumping things together can easily lead to code rot.

Mark Hoffman
Saturday, May 8, 2004

This may be slightly off-topic, but I thought I'd give it a shot in case anyone finds this helpful.

We're developing a fairly complex system, and about a year ago, we started using sequence diagrams to document the construction plan for the application.

A nice side-effect of this was that if we attempted to include too many steps in one sequence diagram, it got really difficult to understand because of the volume of messages. We then would realize that we needed to break up the steps into multiple "chunks," which turned into other sequence diagrams. (Now, we actually do this even earlier in the process, as the use cases are written.)

Anyway, the point was that using this technique, we can scope each procedure before the code is written, saving us a lot of time by "refactoring" on paper, so to speak, before the code is written.

Dave

Dave
Saturday, May 8, 2004

THANK YOU everyone.

After I posted, I thought "gee, what stupid question", expecting lots of "you moron, of course you shouldn't chain".

What I got, instead, were some excellent, thoughtful and very helpful responses.

Mr. Analogy
Saturday, May 8, 2004

"clumping things together can easily lead to code rot."

Though spreading things apart can easily lead to spaghetti...

Ron
Saturday, May 8, 2004

<quote>
"Is the purpose of this thread to find the most innovative spelling of "exercise" ?? "
</quote>

I know I spelled it wrong, but remember: regarding naming conventions. no matter what you choose, just stick to it.

=)

.NET Developer
Saturday, May 8, 2004

.NET Developer:

Excellent response!

*grin*

HeWhoMustBeConfused
Sunday, May 9, 2004

Yeah like how referrer is incorrectly spelled "referer" in the http spec, but now it's stuck: http://www.hpl.hp.com/personal/ange/archives/archives-96/http-wg-archive/1794.html

Matthew Lock
Sunday, May 9, 2004