Path: utzoo!utgpu!cunews!mitel!melair!dataco!amodeo
From: amodeo@dataco.UUCP (Roy Amodeo)
Newsgroups: comp.lang.c
Subject: Re: Coding Standards.  was: a style question
Message-ID: <291@dcsun21.dataco.UUCP>
Date: 18 Nov 90 06:59:38 GMT
References: <1990Oct23.160116.10299@athena.mit.edu> <13@christmas.UUCP> <14369@smoke.brl.mil> <1990Nov10.191840.21113@clear.com>
Organization: Canadian Marconi Company (Datacomm), Ottawa, Ontario
Lines: 182
To: imp@marvin.Solbourne.COM
In-Reply-To: <IMP.90Nov16123956@marvin.Solbourne.COM>
Cc:
Bcc:

In article <IMP.90Nov16123956@marvin.Solbourne.COM> you write:
>In article <1990Nov10.191840.21113@clear.com> rmartin@clear.com (Bob Martin) writes:
>: It specifies that functions should have single entrance and single
>: exit points
>
>This is a bogus restriction, at least in terms of a single exit point.

It does solve some problems. The code fragments you've posted are
a pretty good illustration of why such standards exist. May I please
"correct" them?

>When you have small functions, the return statement can be used to
>give meaningful flow control to the functions w/o the need for a
>"goto".  I have found that when I adhere to this rule, I either get
>code that looks like:
>
>	if (allocate-memory) {
>		do some stuff
>		if (get-more-memory) {
>			do more stuff
>			if (open-file) {
>				do even more stuff
>				if (alloc more memory) {
>					...
>					status = OK;
>				}
>				else
>					status = NO_MEM
>			}
>			else
>				status = NO_FILE
>		}
>		else
>			status = NO_MEM
>	}
>	else
>		status = NO_MEM

	/*-- Don't forget to free memory and close files on failure!	--*/

	if ( status != OK ) {
		if ( file-is-opened )
			close-file;
		if ( more-memory-was-got )
			free-more-memory;
		if ( memory-allocated )
			free-memory;
	}
>
>	return status;
>
>Or I get code that looks like:
>
>	if (!allocate-memory) {
>		status = NO_MEM;
>		goto done;
>	}
>	...
>
>	last stuff
>	
>	status = OK;
>done:

	/*-- Insert same code fragment from above here		--*/
>	return status;
>
>When what I really want is:
	(with cleanup code added)
>
>	if (!allocate-memory)
>		return NO_MEM
>
>	do some stuff
>
>	if ( !get-more-memory)
	{
		free-memory-allocated-above
>		return NO_MEM
	}
>
>	do more stuff
>
>	if (!openfile)
	{
		free-memory-allocated-above
		free-memory-allocated-first-time
>		return NO_FILE
	}
>
>	do even more stuff
>
>	if (!alloc memory)
	{
		close-files
		...
>		return NO_MEM
	}
>
>	last stuff
>	
>	return OK;
>
>A quick check reveals that the above code segments are all the same.
I hope I've preserved that.

>However, I may have missed something.  The final one is the clearest
>one, IHO, of them all.  Comments....

But as you can see, it's also the most painful to modify to clean up
after itself. When there is no cleanup to be done, I agree that the
last example is clearest. However, if you introduce an operation near
the beginning that does have to be cleaned up when you fail then you
are setting yourself up for some pain.

That said, I think the first method is a pain, because of the indentation
required. The second one uses goto's which I still do not like (although
this is the only use of goto's that I consider justifiable (especially
if the programmers use a standard naming convention for the label at
the end of the routine. ))

A method we have been using on a very large project that retains readability
in exchange for making cleanup code a bit more awkward is the following:
( adapted to the conventions above )

In a project-wide include file we have

#define	FAILIF( cond, reason )	{ if (cond) { FAIL_ACTION; return reason; } }
#define	FAIL_ACTION	/*-- default is do nothing	--*/ 

The code we would write for your example would be:

#undef	FAIL_ACTION
#define	FAIL_ACTION	\
		if ( file-is-opened ) \
			close-file; \
		if ( more-memory-was-got ) \
			free-more-memory; \
		if ( memory-allocated ) \
			free-memory; \

	FAILIF( !allocate-memory, NO_MEM );
	do some stuff

	FAILIF( !get-more-memory, NO_MEM );
	do more stuff

	FAILIF( !open-file, NO_FILE );
	do even more stuff

	FAILIF( !alloc more memory, NO_MEM );
	return	OK;

The eye is forcibly directed to the abnormal termination conditions,
whereas embedded returns are a little harder to see. If one is concerned
about the amount of code generated, you could always do this:

#undef	FAIL
#define	FAIL(cond,reason)	{ if (cond) { status = reason; goto failed; } }

	FAILIF( !allocate-memory, NO_MEM );
	...

	return OK;

	failed:
		if ( file-is-opened )
			close-file;
		...
		return status;

Actually I really prefer the second form. The code produced is a little
tighter and it puts the cleanup code in a better place: near the normal
return statement.

Apologies for being longwinded, and I hope I didn't come off sounding
too arrogant. I'll second Warner's request for comments.

>Warner Losh		imp@Solbourne.COM

rba iv			(a signature file would be an admission of existence)
nrcaer!dataco!amodeo