Coding style survey

[Tom St Denis]

This is considered very *poor* programming style
for a number of reasons:

1. you declare variables where constants may be required,

Well I'm assuming you'd have one statement for constants and one for not.
E.g.

const char *p, c;
char q, *x;

2. your variables are undefined and

So far. That's just the declaration my friend.

3. your variables are undocumented.

True that. But it depends. Sometimes variables are trivial, e.g. "int x"
would often be just a "I want to step through an array".

const
char c = 'c'; // character constant
char q = '\0'; // character variable
const
char* const x = "some constant string";
const
char* p = &c; // reseatable pointer to a
// character constant

These *definitions* should be placed as close as possible
to the point where they are first used.

Your style is just plain messy. Try writing a function that has say 10 or
so variables. Now try maintaining 400 similar functions ;-). Giving them
values initially is just plain bad coding [hint constants other than trivial
ones should be #define not set in local variables].

I think this is just another "bad advice post" which you seem good at...

Tom

 

[CBFalconer]

Why is mr. Tisdale hated so much??

As Joona has said, because of his virtually universal bad advice,
and especially because of his habit of altering quotations of
other posters. This means he has to be watched and rebutted
continuously, just so newbies such as yourself don't get
misinformed. We can't just plonk and forget him. Like
BullSchildt, he often sounds good to the uninformed.

 

[BruceS]

As Joona has said, because of his virtually universal bad advice, and
especially because of his habit of altering quotations of other posters.
This means he has to be watched and rebutted continuously, just so
newbies such as yourself don't get misinformed. We can't just plonk and
forget him. Like BullSchildt, he often sounds good to the uninformed.

For what it's worth, as a lurker who seldom posts, I don't hate Tisdale at
all. He's one of the posters I sometimes look for in clc, if he's gotten a
response from a clued regular. He is most certainly a troll, and most of
his "advice" is ludicrous, but this means he often provides a laugh. Also,
because (as Chuck says) clued regulars rebut him, newbies get to see some
good advice as a result of ERTroll's bad advice. OTOH, his dishonest habit
of editing others' posts is disturbing. It's one thing to know that you
shouldn't trust what he says. It's much worse to know that you can't trust
that people have said what he quotes them as saying. Perhaps he could get
a job with a major U.S. newspaper.

 

[Richard Heathfield]

Papadopoulos Giannis wrote:

Why is mr. Tisdale hated so much??

"Hate" is, in my opinion, too strong a word. Suffice to say that his
knowledge of C is rather shaky, compared to his determination to give
advice about it. If he could only learn, he might even become... well,
given his past track-record, there is no point in speculating on that
possibility.

PS. I am new to comp.lang.c

For a while at least, read every article you find. After a while, you'll get
a feel for who knows what they're talking about and who doesn't.

 

[E. Robert Tisdale]

Well I'm assuming you'd have one statement for constants and one for not.
E.g.

const char *p, c;

> cat main.c

#include <stdio.h>

int main(int argc, char* argv[]) {
const char *p, c;
p = NULL;
c = 'c';
return 0;
}

> gcc -Wall -std=c99 -pedantic -o main main.c

main.c: In function `main':
main.c:6: warning: assignment of read-only variable `c'

 

[Arthur J. O'Dwyer]

Oh, I just read the "From:" part of the message to know that.

Yeah, sometimes that works for me, too. ;-) I was just reading
on autopilot, and was jolted awake by the incredibly bad advice
Trollsdale was dishing out.

I disagree, and I think this is at best a matter of opinion. Since
there is nothing controvertial about what the compiler does (except
the issue of the "*" being associated with the variable/type being
declared instead of the defining type)

Funny, I'd say the opposite -- the association of the "*" is
absolutely, positively NON-controversial! It's dictated by an
international standard, for Pete's sake! The controversial bit
of your post was where you seemed to advocate writing

char c, *p, q, *x;

or whatever it really was; I'm not going to take the time to look
back right now. I maintain that this is a Bad Idea.

Also, its not contrived -- go download Bstrlib and see for yourself.

I don't see any Bad Idea declarations in the CVS tree for bstrlib
at first glance. You do use 'int i,j;' and 'int i,v,n;' several times,
but I hope you can appreciate that declaring two or three closely
related objects of the same type on a single line is a far cry from
declaring four or five objects of different types on the same line!
As it happens, I *would* have re-written

bstring bBase64Encode (const bstring b) {
int i, c0, c1, c2, c3;
as
bstring bBase64Encode (bstring b)
{
int c0, c1, c2, c3;
int i;

which IMHO even without the (IMVHO) more-readable indentation makes
it more obvious that 'i' is unrelated to 'c0' etc. [And yes, IMO
it does *not* make more sense to use a four-element array here,
just in case anyone was going to piggy-back on this post. ;-) ]

But when I was talking about Bad Ideas, I was thinking of the common
newbie and ancient-scientific-code practice of writing

/* (e-mail address removed) */
int compare(FILE* , FILE* , long* , long* ), j;

/* (e-mail address removed) */
float sig_in[30], numerator[3], denominator[3], x[3], y[2], s_out[30];

which is just plain write-only code.

Most good compilers already help you with this potential problem, and

this is not a practical/useful suggestion for a large struct or ADT.

^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Since initialization, is of arbitrary complexity, attempting to do so
solely within the declaration section is kind of futile in general.

If you remove the line I underscored above, your comment makes sense
and is true. I don't know why you added that second line, though --
it doesn't make any sense at all. Define variables when you need
them, and not [too far] before. As a corollary, if you find you need
an arbitrarily complex initialization scheme, it makes sense to put
that initialization in a function and write

struct myADT foo;
myADT_initialize(&foo, bar, baz, quux);

bstrlib does not seem at a glance to suffer from this problem, though.

-Arthur

 

[E. Robert Tisdale]

Define variables when you need them, and not [too far] before.
As a corollary,
if you find you need an arbitrarily complex initialization scheme,
it makes sense to put that initialization in a function and write

struct myADT foo;
myADT_initialize(&foo, bar, baz, quux);

This is very bad advice because you can't define a const struct myADT.
Instead, you should implement a "pseudoconstructor":

struct myADT myADT_create(Bar, Baz, Quux);

so that you can define a constant:

const struct myADT foo = myADT_create(bar, baz, quux);

Or you could use it to define a variable:

struct myADT foo = myADT_create(bar, baz, quux);

and define a function

struct myADT* myADT_modify(struct myADT*, Bar, Baz, Quux);

so that you can *modify* the variable:

myADT_modify(&foo, bar, baz, quux);

Note that the myADT_modify function returns a pointer to foo so that
it can be used in any expression where &foo might otherwise appear
subsequent to the application of myADT_modify.

 

[Arthur J. O'Dwyer]

This is very bad advice because you can't define a const struct myADT.
Instead, you should implement a "pseudoconstructor":

struct myADT myADT_create(Bar, Baz, Quux);

so that you can define a constant:

const struct myADT foo = myADT_create(bar, baz, quux);

That's good advice. I myself religiously avoid returning struct
types by value, and I also rarely 'const'ify anything anyway
(preferring to #define CONSTANT if it's never going to change, or
make a static global 'Constant' if the user might want to change
it, or make it a parameter if it's local to a function). However,
*if* one wanted to make a 'const struct myADT', this would be an
excellent way to do so.

Or you could use it to define a variable:

struct myADT foo = myADT_create(bar, baz, quux);

and define a function

struct myADT* myADT_modify(struct myADT*, Bar, Baz, Quux);

so that you can *modify* the variable:

myADT_modify(&foo, bar, baz, quux);

Note that the myADT_modify function returns a pointer to foo so that
it can be used in any expression where &foo might otherwise appear
subsequent to the application of myADT_modify.

Yes. I tend to use this more C++-looking idiom with pointers,
occasionally:

struct myADT *foo = new_myADT(bar, baz, quux);
myADT_modify(foo, bar, baz, quux);
delete_myADT(foo);

but it is a widely followed practice to let functions operate
on "buffers" rather than "OO objects" when writing in C. That
is, it becomes the caller's responsibility to allocate space for
a "buffer" object, on which the function operates; rather than
the function's responsibility to allocate a temporary struct
object and then copy the data over to the caller [which in C is
done automagically, but still has to happen somewhere].
I hope that makes sense. IOW: both ways work, and have their
uses, but I find my way more sensible more often -- in my code,
of course.

-Arthur

 

[E. Robert Tisdale]

I tend to use this more C++-looking idiom with pointers,
occasionally:

struct myADT *foo = new_myADT(bar, baz, quux);
myADT_modify(foo, bar, baz, quux);
delete_myADT(foo);

but it is a widely followed practice to let functions operate
on "buffers" rather than "OO objects" when writing in C.
That is, it becomes the caller's responsibility to allocate
space for a "buffer" object, on which the function operates;
rather than the function's responsibility
to allocate a temporary struct object
and then copy the data over to the caller
[which, in C, is done automagically
but still has to happen somewhere]. I hope that makes sense.

That's just the point.
It isn't necessary for the C *programmer*
to explicitly allocate a "buffer" object.
This is done automatically by your optimizing C *compiler*.
Almost all modern C compilers implement
the Named Return Value Optimization (NRVO).
The function does *not* "allocate a temporary struct object
and then copy the data over to the caller".
Instead, it recognizes the temporary
as a reference to the return value and initializes it directly.
In the definition of object foo:

struct myADT foo = myADT_create(bar, baz, quux);

the optimizing C compiler allocates [automatic] storage for foo
then passes a pointer to foo as a [hidden] argument to myADT_create
which is used to reference to the return value.
*No* copies of a struct myADT are ever required.

No delete_myADT function is required
but the implementation should provide a function

void myADT_destroy(const struct myADT* p) {
}

which should be called

myADT_destroy(&foo);

before the thread of execution passes out of the scope of foo
in case the struct myADT is ever redefined to include a pointer
to an object allocated from free storage when it is created.

Note that you still need myADT_new (and myADT_delete)
to create objects that must survive the current scope.

 

[Old Wolf]

char c, *p, q, *x;

or whatever it really was; I'm not going to take the time to look
back right now. I maintain that this is a Bad Idea.

What do you think of
char , *p[], q(), *x;
then?

Recently I wrote some code like:

const unsigned char s[] = {
foo(1),
foo(2),
/* ... (about 10 entries) */
}, *ps = s;

so that later on I could iterate with ps, or use the results
from the functioncalls in many places. Using this syntax
avoided having to type out "const unsigned char" twice (or use typedef)

 

[Arthur J. O'Dwyer]

[Arthur wrote:]

char c, *p, q, *x;

or whatever it really was; I'm not going to take the time to look
back right now. I maintain that this is a Bad Idea.

What do you think of
char , *p[], q(), *x;
then?

Slightly better, in that the compiler will refuse to compile it,
rather than let it sit on your disk misleading future programmers.

Recently I wrote some code like:

const unsigned char s[] = {
foo(1),
foo(2),
/* ... (about 10 entries) */
}, *ps = s;

so that later on I could iterate with ps, or use the results
from the functioncalls in many places. Using this syntax
avoided having to type out "const unsigned char" twice (or use typedef)

Ooh, dear, typing 'const unsigned char' twice! ;-) Anyway, this
sort of thing is one of very few exceptions to the general rule.
I have seen and appreciated the idiom

typedef struct foo { bar blah; } foo, *pfoo;

even though I would obviously prefer to write

typedef struct foo foo;
typedef foo *pfoo;
struct foo { bar blah; };

in such situations, if they ever arose in my own code. And it's
possible to get borderline cases like the one you describe, even
in real code. But I'd still write

const unsigned char s[] = {
foo(1),
foo(2),
/* ... (about 10 entries) */
};
const unsigned char *ps = s;

in code meant for human consumption. The extra 'const unsigned char'
is quick to code, in an editor with cut-and-paste; and if you are
seriously concerned that 'unsigned char' may change in future versions,
you should be using a typedef anyway.

-Arthur

 

[CBFalconer]

.... snip ...

... I myself religiously avoid returning struct
types by value, and I also rarely 'const'ify anything anyway
(preferring to #define CONSTANT if it's never going to change, or
make a static global 'Constant' if the user might want to change
it, or make it a parameter if it's local to a function). However,
*if* one wanted to make a 'const struct myADT', this would be an
excellent way to do so.

Counter example, taken from my hashlib package. This contains a
defined structure holding various fields with current data about
the hashtable, including the number of items stored, deleted,
count of probes, collisions, and error status. The record itself
is NEVER exposed to the user, since it is essential that it be
modified only by the operation of the package. However the
definition of the structure is published in the header file. The
user can access the current contents by executing:

status = hshstatus(h);

where h is the value returned on opening the hashtable (something
like the FILE * returned on opening a file). Assuming the user
has correctly declared status as something like:

hshstats status;

he can safely retrieve all those fields, and I have never exposed
them to prying hands.

 

[E. Robert Tisdale]

Counter example, taken from my hashlib package.

How can we download a copy of hashlib?

 

[Richard Bos]

Sure. The basic idea of structured programming is that the program is
divided into a hierarchy of functions, becoming more general-purpose as you
move down the hierachy, more specific as you move up.

Exactly. So it is perfectly reasonable to define a low-level function to
handle generic data, and then add a function to handle specific data
which does nothing but find the specific data and then invoke the first
function on it.

The other principle is that each function performs one closely-defined task,
and is a black box to the function that calls it.

Exactly. So when foo(bar) foos a bar, but you don't know or care how,
foo_largest(bar) should not try to second-guess it, but should find the
largest bar and then foo it using the lower-level function foo().

This means that barb() shouldn't know the intricacies of foo(). Any change
to foo() shouldn't imply a change in barb(), but only in bar() (where it is
unavoidable).
This does not absolutely preclude returning a result directly from foo() in
barb(), but it does mean that occasions for doing so will be rare,

Nonsense. It is exactly because you don't care what foo() and bar() do
internally, but do know that bar() returns a struct baz, that it can
call foo() which also returns a struct baz.

and can be used as a marker for code that is likely to be poorly-designed,
because the function hierarchy isn't abstracting foo() from bar().

But that is exactly what it _does_ do!

It means that foo() is too general for the real caller to use.

For _that_ real caller to use. Other functions could easily put it to
good use.

Richard

 

[CBFalconer]

How can we download a copy of hashlib?

By keeping your eyes open. However, I don't think you want to.
It is copyright and released under the GPL, and I would look amiss
upon your altering it in any way. The rest of the world would
probably look amiss upon any code you released after using it.

 

[Programmer Dude]

More data points...

Which do you think is best?

I take that to mean, "Which do I prefer to use?" (-:
'There is no best. There is only prefer or not prefer.'

1.
a) type* p;

To me, p is a type*. And because I usually initialize my
variables, and because I usually put them on one line each, the
"type* p, q;" issue is not an issue for me.

2.
c) return var;

3.
c) return ptr->var;

4.
c) return foo(ptr->var);

5.
d) a=b+c;

But prefer "a = b + c;".

6.
c)
type foo(type arg1, type arg2, ...)
{
declarations;
code;
return;
}

C is closest, but prefer 4-space indent (NO TABS!!) and a blank
line after declarations. It does depend on the complexity of the
function.

 

[Malcolm]

Exactly. So it is perfectly reasonable to define a low-level function to
handle generic data, and then add a function to handle specific data
which does nothing but find the specific data and then invoke the first
function on it.

Problem is there are two ways a function can be "more general". The first
way is by being simpler. For instance, a triangle is the simplest
geometrical shapes. All other polygons can be built up out of triangles. It
is therefore reasonable to have "triangle" as your lowest level graphics
function.
The other way is by being more complex. "quad" takes more parameters and is
harder to write than "triangle". It can also draw a triangle (by making two
points identical). However it is not usually a good idea to implement
"triangle" in terms of "quad".

Nonsense. It is exactly because you don't care what foo() and bar()
do internally, but do know that bar() returns a struct baz, that it can
call foo() which also returns a struct baz.

Read through the example again. You will see that a change to foo(), the
lowest level function, doesn't require any changes to bar(), but does break
barb(), the highest-level function. This is the sort of undesirable
situation you will get if you handle information at the wrong level, and it
is very easy for bugs to slip through.

For _that_ real caller to use. Other functions could easily put it to
good use.

So we're in the situation where the user wants to draw a triangle but the
graphics system will only draw quads. Sometimes this is unavoidable (the
Sega Saturn had hardware that would only rasterise quads), but most people
would say that this indicates something is wrong with the way the library is
defined. Special cases are only seldom well handled by supplying the
parameters to the more general cases.
I'll give an exception that proves the rule. MS Windows has a function
TextOut that takes x, y co-ordinates, a string, and the length of the string
to write. Obviously 99% of the time you want to pass str, strlen(str). It
makes sense to write a convenience function

int TextOutZ(HWND hwnd, int x, int y, char *str)
{
return TextOut(hwnd, x, y, str, strlen(str));
}

However it only makes sense because TextOut() wasn't designed with C in
mind - it is a hang over from the Pascal days. If Windows was being
rewritten from scratch today everyone would use C NUL-terminated strings as
a matter of course.

We don't want to be too strong and say never, ever must one return the
return value of another function directly. It's rather like global
variables, gotos, and very long functions - occasionally they are called for
but too many indicate that the person who wrote the code hasn't really
grasped the principles of structured programming.

 

[Malcolm]

Its not unusual, and it might be a macro rather than a function.

Better qualify that by "unusual in well-written C programs". A macro that
evaluates to a value based on its parameters is a function. You might say it
isn't a "function" in C terms. Here you're making a logic error based on
confusion between the linguistic and the mathematical set theory use of
categories. This error is "Before you cross the road, wait until there are
no cars coming" Tum-tee-tum-tum, crump! Kid run over by a bus.

 

[E. Robert Tisdale]

By keeping your eyes open.

It's vaporware!

However, I don't think you want to.
It is copyright and released under the GPL and
I would look amiss upon your altering it in any way.

You had not better release it under the GPL then.
The GPL gives me the right to modify and redistribute it.

The rest of the world would probably look amiss
upon any code you released after using it.

I don't think that I could use it.

 

[Arthur J. O'Dwyer]

Counter example, taken from my hashlib package. This contains a
defined structure holding various fields with current data about
the hashtable, including the number of items stored, deleted,
count of probes, collisions, and error status. The record itself
is NEVER exposed to the user, since it is essential that it be
modified only by the operation of the package. However the
definition of the structure is published in the header file. The
user can access the current contents by executing:

status = hshstatus(h);

where h is the value returned on opening the hashtable (something
like the FILE * returned on opening a file). Assuming the user
has correctly declared status as something like:

hshstats status;

he can safely retrieve all those fields, and I have never exposed
them to prying hands.

To what is this a "counter-example"? I see allusions to "private"
data, user-defined types, and encapsulation, but the part of my
post that you quoted wasn't talking about such high-level stuff.
I just said that if you wanted to make a const struct, then initializing
it with the value returned by a function would be an excellent idea.

So, if you're disagreeing with something I wrote, could you say
so? Or if you're agreeing with something I wrote, could you say
that too? Or if you're just describing hashlib, could you start a
new thread? ;-)

On second glance, I realize that you can't use the result of a
function to initialize a struct at file scope; is *that* what you
meant to say, maybe?

-Arthur