A Question of Style

[Jorgen Grahn]

That's a stretch.

No; it was how I immediately interpreted it too. Who has anything
against Italy these days?

/Jorgen

 

[Ian Collins]

My pattern is to have a create and a kill. The create allocates the
structure, the kill destroys it (so C++ constructors / destructors,
in effect). Now you write the kill so that it's robust to partly created
structures. So it always does a no-op for a null. But it can't
distinguish a valid pointer from an uninitialised garbage pointer.
So you need to be careful to set all pointer members to null immediately
after allocating them.

Just using C++ where possible would save you a lot of pain

 

[Rui Maciel]

I found this code in a Quake 2 project. Would you call this a
case of premature optimization or a case of single-returnitis in
the face of a necessarily multi-return function. I am referring
to the multiple gotos in the function designed to bypass a single
line of code. [snip 74 line function definition]

I would call it atrocious code in need of re-writing. A
classic example of trying to do too much in a single function,
with hard-to-follow control flow being just one symptom.

I looked at your alternative and saw the same thing: atrocious code in need of re-writing.
You've declared single-purpose functions which only evaluate a single logical expression,
write to a text stream and return a value, for something which only takes a couple of lines on
a function and is executed once. In the process, motivated by a need to show off, you've
turned a simple function which is straight forward and written in easily readable and
maintainable code into a cryptic function which obscures the control flow for no good reason.

You see, you're arguing style instead of substance, and your code only reflects your personal
preferences and does absolutely nothing with regards to substance. Accusing someone else's
code of being atrocious just because it doesn't fit your questionable personal preferences,
particularly when they happen to worsen the quality of the code, is just bad form and does
nothing to improve anything. At all.

It
seems much better to divide the original function up so there
are a couple of subfunctions, with the smaller pieces being
more easily comprehensible.

You see? It "seems much better" to you. You've made a baseless assertion regarding your own
personal taste which reflects no technical aspect, with the only purpose of criticising
someone else's code without actually doing anything to improve it, and in the process arguably
worsening it.


Style and substance are two entirely different things. One reflects personal tastes, whims
and even technical limitations, the other tackles problems and provices appropriate ways to
solve them. It doesn't make sense and helps no one to refer to personal opinions regarding
style to criticise other people's work.


Rui Maciel

 

[glen herrmannsfeldt]

(snip)

No; it was how I immediately interpreted it too. Who has anything
against Italy these days?

Amanda Knox.

 

[Rui Maciel]

Code that involves "goto" can almost always be better written using
multiple functions and some sort of state variables to give clearer and
more maintainable code.

You generally see such "goto" style in older code, as a way of getting
faster code. More modern compliers and more modern C standards (giving
explicit and automatic "inline") can roll structured code with multiple
functions into a single block with goto's generated in the generated code.

I'm starting to believe this goto phobia is a sign of incompetence, and reflects a need to
blindly and mindlessly adopt beliefs which are believed to be held by a group in an effort to
try to fit in, while showing a complete lack of critical thinking and insight on the issue.

The code in question is perfectly fine. The goto statements are a classical example of how to
properly use goto statements: they don't mangle the control flow, their use significantly
enhance the readability of the code, and they provide an easy, simple and better yet localised
way to perform all cleanup tasks required by that function. They are used to simply implement
a destructor-like behavior. Anyone will be hard pressed to implement the exact same function
without resorting to code duplication, complicating things with extra loops and conditionals,
or declaring useless functions.

Plus, your comment regarding "faster code" is complete nonsense. If you'd read the code you'd
noticed that the goto statements are used by error handling functions for cleanup purposes, in
cases such as malloc and fread failures. I don't believe anyone worries about speed when
throwing a potentially and assuredly fatal error regarding memory allocation.

There is plenty of software development best practices beyond the "go to considered harmful"
paper, which is clearly a classical paper in the sense that everyone is aware if its existence
and refers to it extensively, but never took the time to actually read it. Holding on to ill-
conceived notions to criticise someone else's code is completely innapropriate and rude, and
entirely unnecessary.


Rui Maciel

 

[Rosario1903]

No; it was how I immediately interpreted it too. Who has anything
against Italy these days?

/Jorgen

for me you could be racist against italians etc

not all italians are the same, but the 90% is *not* ok for me
the way they follow is wrong for me

 

[Rosario1903]

what does it mean above?
....

for me you could be racist against italians etc

not all italians are the same, but the 90% is *not* ok for me
the way they follow is wrong for me

i not say i'm ok

 

[Malcolm McLean]

Malcolm McLean wrote:

Just using C++ where possible would save you a lot of pain

When you start declaring function pointer members of structures
to allow for run-time resolution of bindings, it's time to consider
whether you should move to C++. But the constructor / destructor
paradigm works perfectly well in C.
In modern C++ you can use vectors rather than arrays for variable-
number members, which simplifies the destructor. In C everything
has to be destroyed explicitly, which is a bit of a nuisance. But
it's not too bad if you always have matching create / kill calls,
all the complexity is in two places (the constructor and destructor).

 

[Malcolm McLean]

I'm starting to believe this goto phobia is a sign of incompetence,
and reflects a need to blindly and mindlessly adopt beliefs which are
believed to be held by a group in an effort to try to fit in, while showing
a complete lack of critical thinking and insight on the issue.

I agree.

Goto is fine for handling error conditions. One of the problems in C is
that there is no way of distinguishing error handling from normal control
flow, which can make code hard to read because error handling has a way
of dominating the paths. A goto helps to alleviate that problem.

 

[Ian Collins]

When you start declaring function pointer members of structures
to allow for run-time resolution of bindings, it's time to consider
whether you should move to C++. But the constructor / destructor
paradigm works perfectly well in C.

Only in a manual and therefore error prone way. The big gain from C++
is automatic object destruction which removes a whole class of
programmer errors (and the need to ever use goto!). This is also just
about the only trick in C++'s box that C can't replicate.

 

[Geoff]

That's a stretch.

The comment was clearly about italians, and how the entire nation is crassly incompetent with
regard to technical issues. It might be a joke, but it's still a racist one, and even if
someone follows the racist joke with any version of "lighten up", it's still racist.

Actually, it was intended as a pun about loops, and those who would
execute them.

Ethnicity and nationality is not race.

I would be crass to disparage Italians, after all I am ensconced in a
continent discovered by an Italian with the help of Spaniards. I am
descended from English and German, you may now begin disparaging my
ethnic heritage.

 

[Ian Collins]

I found this code in a Quake 2 project. Would you call this a
case of premature optimization or a case of single-returnitis in
the face of a necessarily multi-return function. I am referring
to the multiple gotos in the function designed to bypass a single
line of code. [snip 74 line function definition]

I would call it atrocious code in need of re-writing. A
classic example of trying to do too much in a single function,
with hard-to-follow control flow being just one symptom.

I looked at your alternative and saw the same thing: atrocious code in need of re-writing.
You've declared single-purpose functions which only evaluate a single logical expression,
write to a text stream and return a value, for something which only takes a couple of lines on
a function and is executed once.

So?

Which is easier to test, spaghetti or single-purpose functions?

Which is easier to understand and maintain, spaghetti or single-purpose
functions?

 

[Geoff]

what does it mean above?

Francesco Schettino didn't like sitting in his chair driving a
predetermined course. He chose to play captain and ordered a deviation
in a wide loop and hit a rock. He further turned back to Giglio in a
U-turn and crashed into another rock in an attempt to save the ship. A
captain's primary responsibility is the SAFETY of his ship and of the
souls on board, it's not about salutes and parties or impressing
mistresses.

The salvage operation was in the news during this thread. It was
merely on my mind while reading about the suggestion to write a loop.

Rosario, it was not about incompetent Italians in general or even
about you or your competence. It was about the word loop and the fact
I find your coding style to be quirky and strange, synonyms for loopy.

OT Aside: There was no way for Schettino to have known the contour of
the sea floor approaching that harbor, there was no way for him to
study the charts in sufficient detail to have decided to ground the
ship where it ended up. He was trying to drive it into a grounding
point within the harbor and actually ran it aground where it lays.

 

[Rui Maciel]

I found this code in a Quake 2 project. Would you call this a
case of premature optimization or a case of single-returnitis in
the face of a necessarily multi-return function. I am referring
to the multiple gotos in the function designed to bypass a single
line of code. [snip 74 line function definition]

I would call it atrocious code in need of re-writing. A
classic example of trying to do too much in a single function,
with hard-to-follow control flow being just one symptom.

I looked at your alternative and saw the same thing: atrocious code in
need of re-writing. You've declared single-purpose functions which only
evaluate a single logical expression, write to a text stream and return a
value, for something which only takes a couple of lines on a function and
is executed once.

So?

Which is easier to test, spaghetti or single-purpose functions?

Which is easier to understand and maintain, spaghetti or single-purpose
functions?

I fail to see where how your comparison applies to this case, as the code presented by Geoff
can't possibly be described as spaghetti code.


Rui Maciel

 

[Rosario1903]

Only in a manual and therefore error prone way. The big gain from C++
is automatic object destruction which removes a whole class of
programmer errors (and the need to ever use goto!). This is also just
about the only trick in C++'s box that C can't replicate.

than afther i have seen what is ok in the C++ language [class ini
function and end function],
why i prefer the same assembly?

 

[Geoff]

All,
In the end I took advantage of some internal knowledge of the Q2 engine and the fact it
uses its own allocator and the game DLLs are supposed to be using that allocator instead
of malloc/free. Q2's memory management posts diagnostic messages and terminates the
program in case of errors and this makes it unnecessary to actually check and report
errors when using it.

Especially helpful in putting my thoughts on the right track were Tim's use of __func__,
and Ben's note that guarding fclose(f) was a good idea and I quite agree with James Kuyper
on the value of single-return being desirable even without being dogmatic about it.

This is very old code and by necessity I am dealing with Microsoft's compiler and their
lack of support for newer C standards but I managed to incorporate some of them.

To that end I attempt the following rewrite, apologizing for the line length:

void TeamplaySpawnEntities (char *mapname, char *entities, char *spawnpoint)
{
FILE *f;
char filename[MAX_QPATH];
int nEntSize, nRead;
char *pszCustomEnt;

// Create pathname to the entity file and try to open it.
Com_sprintf (filename, MAX_QPATH, "%s/ent/%s.ent", gamedir->string, mapname);
f = fopen (filename, "rb");

// if we don't have a custom file or we're not
// configured to load one, just spawn the defaults
if (!f || (!custom_ents->value && !ctf->value))
{
SpawnEntities (mapname, entities, spawnpoint);
return;
}

// Get the size of the file.
if (fseek (f, 0, SEEK_END) != 0)
gi.dprintf ("ERROR: %s: fseek %s\n", __func__, filename);
else if ((nEntSize = ftell (f)) < 0)
gi.dprintf ("ERROR: %s: ftell %s (%d)\n", __func__, filename, nEntSize);
else if (fseek (f, 0, SEEK_SET) != 0)
gi.dprintf ("ERROR: %s: fseek %s\n", __func__, filename);
else
// allocate space for the entities we are about to read
// if gi.TagMalloc fails, it reports error and terminates the program
pszCustomEnt = gi.TagMalloc (nEntSize + 1, TAG_LEVEL);

if ((nRead = fread (pszCustomEnt, 1, nEntSize, f)) != nEntSize)
{
gi.dprintf ("ERROR: %s: fread %s (%d/%d) not all entities were loaded\n",
__func__, filename, nRead, nEntSize);
gi.TagFree (pszCustomEnt);
}

// Null-terminate the string.
pszCustomEnt[nEntSize] = '\0';

// Now spawn *these* entities!
SpawnEntities (mapname, pszCustomEnt, spawnpoint);

if (f) fclose (f);
}

I believe the guarding of fclose is unnecessary given the refactoring and may even be a
little annoying since there is only one fopen/fclose path.

I am open to more suggestions about how to improve this code. I know some of you will
probably have something to say about bracketing the statements for clarity or
maintainability. I have tested it but have not tested all the paths yet.

If you can throw in a few quips about my Teutonic and Saxon origins so I can call you
racist I would very much appreciate it. I know Europeans would be the last people on Earth
to cast aspersions on someone because of their ethnicity or religion. Especially in the
21st century.

 

[Rosario1903]

void TeamplaySpawnEntities(char* mapname, char* entities,
char* spawnpoint)
{FILE *f;
char szFile[MAX_QPATH];
int nEntSize, nRead;
char *pszCustomEnt;

// If teamplay is off, do the normal thing.
if(ctf->value==0)
{rexit1:
SpawnEntities (mapname, entities, spawnpoint);
rexit : return;
}

// Create the pathname to the entity file.
Com_sprintf(szFile, sizeof (szFile), "%s/ent/%s.ent",
gamedir->string, mapname);
// Try to open it.
f=fopen (szFile, "rb");

// No custom entity file, so just use the default.
if(f==0) goto rexit1;

// Get the size of the file.
if (fseek (f, 0, SEEK_END) != 0)
{gi.dprintf ("TeamplaySpawnEntities(): fseek %s\n", szFile);
errClose:
fclose (f);
goto rexit;
}

nEntSize = ftell (f);
if (nEntSize < 0)
{gi.dprintf("TeamplaySpawnEntities(): ftell %s (%d)\n",
szFile, nEntSize);
goto errClose;
}

if (fseek(f, 0, SEEK_SET) != 0)
{gi.dprintf ("TeamplaySpawnEntities(): fseek %s\n", szFile);
goto errClose;
}

// Create a buffer and read the custom entity file.
pszCustomEnt = malloc (nEntSize + 1);
if (pszCustomEnt==0)
{gi.dprintf ("TeamplaySpawnEntities(): malloc\n");
goto errClose;
}

nRead = fread(pszCustomEnt, 1, nEntSize, f);
if(nRead != nEntSize)
{gi.dprintf("TeamplaySpawnEntities(): fread %s (%d/%d)\n",
szFile, nRead, nEntSize);
errFree:
free (pszCustomEnt); goto errClose;
}

// Null-terminate the string.
pszCustomEnt[nEntSize] = '\0';

// Now spawn *these* entities!
SpawnEntities (mapname, pszCustomEnt, spawnpoint);

if(hereCallAnotherfunction()==-1)
goto errFree;

free (pszCustomEnt);
fclose (f);
}

but i think it would be better return one int
with something as R fclose(f);...

how you can see the path that follow routine is as usual

||
||
||
||
\/

instead the error path goes in reverse

/\
||
||
||
||

 

[Ben Bacarisse]

All,
In the end I took advantage of some internal knowledge of the Q2
engine and the fact it uses its own allocator and the game DLLs are
supposed to be using that allocator instead of malloc/free. Q2's
memory management posts diagnostic messages and terminates the program
in case of errors and this makes it unnecessary to actually check and
report errors when using it.

Especially helpful in putting my thoughts on the right track were
Tim's use of __func__, and Ben's note that guarding fclose(f) was a
good idea

Two minor things. I suggested it as a way to permit the flow of control
to pass, always, to the lexical end of the function. You don't do that
below, which seems to have introduced a bug.

In fact I'd almost always write multiple functions to do this kind of
thing. I only didn't do so here because I wanted to make a point about
the particular linear structure you had.

There seems to be opposition to single-use functions, but I use them
quite a lot. They encapsulate the parts of a more complex task in a
particularly easy to understand way. Function that must succeed or fail
depending on various conditions can be written very clearly in terms of
function calls:

return do_this(...) && then_this(...) && and_finally();

or maybe

return do_something_with(a_resource(...), ...) || fallback_here(...);

and so on.

Unfortunately C makes it harder than it should be to do this. You can't
define one function inside another, and because you have to declare a
function before calling it, you either end up writing the small parts
before the big picture or you have to write declarations as well as
definitions.

and I quite agree with James Kuyper on the value of
single-return being desirable even without being dogmatic about it.

This is very old code and by necessity I am dealing with Microsoft's
compiler and their lack of support for newer C standards but I managed
to incorporate some of them.

To that end I attempt the following rewrite, apologizing for the line
length:

void TeamplaySpawnEntities (char *mapname, char *entities, char *spawnpoint)
{
FILE *f;
char filename[MAX_QPATH];
int nEntSize, nRead;
char *pszCustomEnt;

// Create pathname to the entity file and try to open it.
Com_sprintf (filename, MAX_QPATH, "%s/ent/%s.ent", gamedir->string, mapname);
f = fopen (filename, "rb");

// if we don't have a custom file or we're not
// configured to load one, just spawn the defaults
if (!f || (!custom_ents->value && !ctf->value))
{
SpawnEntities (mapname, entities, spawnpoint);

You can get here with f != NULL and so never close the file. The whole
point I was making was to use an if ... else chain so you always got to
the end. Hence the use of if (f) fclose(f).

return;
}

// Get the size of the file.
if (fseek (f, 0, SEEK_END) != 0)
gi.dprintf ("ERROR: %s: fseek %s\n", __func__, filename);
else if ((nEntSize = ftell (f)) < 0)
gi.dprintf ("ERROR: %s: ftell %s (%d)\n", __func__, filename, nEntSize);
else if (fseek (f, 0, SEEK_SET) != 0)
gi.dprintf ("ERROR: %s: fseek %s\n", __func__, filename);
else
// allocate space for the entities we are about to read
// if gi.TagMalloc fails, it reports error and terminates the program
pszCustomEnt = gi.TagMalloc (nEntSize + 1, TAG_LEVEL);

if ((nRead = fread (pszCustomEnt, 1, nEntSize, f)) != nEntSize)
{
gi.dprintf ("ERROR: %s: fread %s (%d/%d) not all entities were loaded\n",
__func__, filename, nRead, nEntSize);
gi.TagFree (pszCustomEnt);
}

// Null-terminate the string.
pszCustomEnt[nEntSize] = '\0';

// Now spawn *these* entities!
SpawnEntities (mapname, pszCustomEnt, spawnpoint);

if (f) fclose (f);
}

I believe the guarding of fclose is unnecessary given the refactoring
and may even be a little annoying since there is only one fopen/fclose
path.

Quite. Either remove it or fix the problem above in which case it
becomes useful again.

I am open to more suggestions about how to improve this code. I know
some of you will probably have something to say about bracketing the
statements for clarity or maintainability. I have tested it but have
not tested all the paths yet.

Testing for files left open is hard. There are lots of problems that
don't show up in simple tests, which is why I put so much store on
clarity so that the problem can be avoided as early as possible.

If you can throw in a few quips about my Teutonic and Saxon origins so
I can call you racist I would very much appreciate it. I know
Europeans would be the last people on Earth to cast aspersions on
someone because of their ethnicity or religion. Especially in the 21st
century.

I am more tempted to make a quip about suitable times to stop digging...

 

[Geoff]

Two minor things. I suggested it as a way to permit the flow of control
to pass, always, to the lexical end of the function. You don't do that
below, which seems to have introduced a bug.

You are quite right. There are two.

In fact I'd almost always write multiple functions to do this kind of
thing. I only didn't do so here because I wanted to make a point about
the particular linear structure you had.

I understand. In this last cut I was hoping to preserve the linear
structure but I can see the point of breaking it up but not to the extent
that Tim did.

There seems to be opposition to single-use functions, but I use them
quite a lot. They encapsulate the parts of a more complex task in a
particularly easy to understand way. Function that must succeed or fail
depending on various conditions can be written very clearly in terms of
function calls:

I once worked on a project in Z80 assembler where there was heavy use of
macros. The macro calls were interspersed with the appropriate register
initializations and it was like working with a high level language. Beautiful.

Re-written yet again.

static size_t getEntityFileSize(const char *filename, FILE *f)
{
size_t size;

// Get the size of the file.
if (fseek (f, 0, SEEK_END) != 0) {
gi.dprintf ("ERROR: %s: fseek %s\n", __func__, filename);
return 0;
}
else if ((size = ftell (f)) < 0) {
gi.dprintf ("ERROR: %s: ftell %s (%d)\n", __func__, filename, size);
return 0;
}
else if (fseek (f, 0, SEEK_SET) != 0) {
gi.dprintf ("ERROR: %s: fseek %s\n", __func__, filename);
return 0;
}
else
return size;
}

void TeamplaySpawnEntities (char *mapname, char *entities, char *spawnpoint)
{
FILE *f;
char filename[MAX_QPATH];
size_t nEntSize;
size_t nRead;
char *pszCustomEnt;
int err;

if (!custom_ents->value && !ctf->value)
{
SpawnEntities (mapname, entities, spawnpoint);
return;
}

/* Create path to the entity file and try to open it. */
Com_sprintf (filename, MAX_QPATH, "%s/ent/%s.ent", gamedir->string, mapname);
if ((f = fopen (filename, "rb")) != NULL)
{
nEntSize = getEntityFileSize(filename, f);
if (nEntSize != 0)
/* if gi.TagMalloc fails it logs the error, aborts process */
pszCustomEnt = gi.TagMalloc (nEntSize + 1, TAG_LEVEL);

nRead = fread (pszCustomEnt, 1, nEntSize, f);
if (nRead != nEntSize)
{
gi.dprintf ("ERROR: %s: fread %s (%d/%d) not all entities were loaded\n",
__func__, filename, nRead, nEntSize);
gi.TagFree (pszCustomEnt);
err = true;
}

if (!err)
{
pszCustomEnt[nEntSize] = '\0';
SpawnEntities (mapname, pszCustomEnt, spawnpoint);
}
fclose (f);
}
}

 

[Jorgen Grahn]

I'm starting to believe this goto phobia is a sign of incompetence, and reflects a need to
blindly and mindlessly adopt beliefs which are believed to be held by a group in an effort to
try to fit in, while showing a complete lack of critical thinking and insight on the issue.

Odd. I think I'm seeing the opposite in recent years: "Wow, I can
fit in *and* break taboos at the same time, by using goto for error
handling! The Linux kernel does it so I can claim superiority by
association!"

I don't mind this when goto /is/ clearly the best approach. Or when
there wasn't time to do it better. I /do/ mind when the author didn't
find the better design (probably breaking the function down) simply
because he wasn't looking.

/Jorgen