Style question

[Nick]

I'm not after a definitive answer here - I know there aren't any. But
I'm curious to know how people feel about something.

A few days ago I found myself writing something like this:

find_item(name)->identifier

Where find_item looks up a name in a data structure and returns a
pointer to the node (a struct). The "identifier" is an integer.

I was, in fact, using this as a paramater to another function, that took
an integer identifier and did some more stuff with it.

There's a bit of me that thinks this is really neat, and another bit of
me that found the resulting three-line statement horrible.

In the end I realised I needed different items at different times, so
introduced a temporary variable. But how do others feel about this
idea of getting a struct pointer back from a function and extracting a
member directly from it?

 

[Felix Palmen]

I'm not after a definitive answer here - I know there aren't any. But
I'm curious to know how people feel about something.

A few days ago I found myself writing something like this:

find_item(name)->identifier

Where find_item looks up a name in a data structure and returns a
pointer to the node (a struct). The "identifier" is an integer.

I was, in fact, using this as a paramater to another function, that took
an integer identifier and did some more stuff with it.

There's a bit of me that thinks this is really neat, and another bit of
me that found the resulting three-line statement horrible.

In the end I realised I needed different items at different times, so
introduced a temporary variable. But how do others feel about this
idea of getting a struct pointer back from a function and extracting a
member directly from it?

I'd put it a little more generally and discuss chained and nested
function calls. And I know this tradeoff from my own experience:

Given proper names and logical nesting/chaining that really describes
the intent of the statement, it just looks quite elegant.

The downside is IMO twofold:

- As your program evolves, you may need to add some functionality in
your already complex statement, missing the point where it turns from
being just obvious, nice and clean into some incomprehensible monster.
- When debugging, you're missing the opportunity to easily single-step
over each involved function, checking the intermediate results.

So, most of the time I'd vote against to much chaining/nesting of
function calls.

Regards,
Felix

 

[Seebs]

In the end I realised I needed different items at different times, so
introduced a temporary variable. But how do others feel about this
idea of getting a struct pointer back from a function and extracting a
member directly from it?

Is it remotely, ever, conceivable that the function will return NULL?

-s

 

[James Dow Allen]

find_item(name)->identifier

Perhaps even more problematic is
copy_item(name).identifier

There are some restrictions on such constructions;
perhaps someone can post an "executive summary."

James

 

[Ian Collins]

Perhaps even more problematic is
copy_item(name).identifier

Probably less problematic, copy_item can't return null.

 

[Tom St Denis]

In the end I realised I needed different items at different times, so
introduced a temporary variable.  But how do others feel about this
idea of getting a struct pointer back from a function and extracting a
member directly from it?

I'd avoid that solely on the premise that function could potentially
fail [return NULL] and thus would segfault your application.

The typical way I would do that is

item = find_item(name);
if (item == NULL) { return error_code_of_some_sort; }
// ... use item

If it's a short usage you might get away with

if ((item = find_item(name)) != NULL) {
// use item
} else {
// return error
}

but if you subsequently "find" another item the code can get nested
crazy deep and weird.

Tom

 

[ImpalerCore]

I'm not after a definitive answer here - I know there aren't any.  But
I'm curious to know how people feel about something.

A few days ago I found myself writing something like this:

find_item(name)->identifier

Where find_item looks up a name in a data structure and returns a
pointer to the node (a struct).  The "identifier" is an integer.

I was, in fact, using this as a paramater to another function, that took
an integer identifier and did some more stuff with it.

There's a bit of me that thinks this is really neat, and another bit of
me that found the resulting three-line statement horrible.

In the end I realised I needed different items at different times, so
introduced a temporary variable.  But how do others feel about this
idea of getting a struct pointer back from a function and extracting a
member directly from it?

In general, any function that returns a struct pointer with a 'find'
or 'search' in the function name is likely to return NULL eventually,
as NULL typically represents the 'not found' return value semantic of
the lookup. To me, this is a bomb with a relatively short fuse. If
the maintenance programmer that follows you finds a bug as a result of
'find_item(name)->identifier', I doubt your cleverness will be
appreciated.

Best regards,
John D.

 

[Tom St Denis]

Probably less problematic, copy_item can't return null.

Nah, it's just a performance hazard and what you're referencing could
be NULL e.g.

printf("name is %s\n", find_item(ID).name);

Brilliant! Combining two problems into one!

 

[Willem]

Kenneth Brody wrote:
) On the other hand, sometimes the resulting code of directly using the
) returned pointer just looks awkward.
)
) foo(bar(plugh)->item[n].addr)->name = "foo";

This is only awkward if you're not used to functional programming.


SaSW, Willem
--
Disclaimer: I am in no way responsible for any of the statements
made in the above text. For all I know I might be
drugged or something..
No I'm not paranoid. You all think I'm paranoid, don't you !
#EOT

 

[Nick]

Is it remotely, ever, conceivable that the function will return NULL?

Not in this case. But I agree that that is a perfectly natural thing
for a search function to do, and completely breaks this approach.

 

[Nick]

On the other hand, sometimes the resulting code of directly using the
returned pointer just looks awkward.

foo(bar(plugh)->item[n].addr)->name = "foo";

And, indeed, that was just the sort of mess I ended up with.

 

[Jorgen Grahn]

Not in this case. But I agree that that is a perfectly natural thing
for a search function to do, and completely breaks this approach.

find_item(name)->identifier

That word "find" is actually my only objection. It hints that you're
not quite sure you'll find anything (or it would have been called
get_item() or simply item()).

/Jorgen

 

[Jorgen Grahn]

.

I'd put it a little more generally and discuss chained and nested
function calls. And I know this tradeoff from my own experience:

Given proper names and logical nesting/chaining that really describes
the intent of the statement, it just looks quite elegant.

The downside is IMO twofold:

- As your program evolves, you may need to add some functionality in
your already complex statement, missing the point where it turns from
being just obvious, nice and clean into some incomprehensible monster.

I object to that. You need to learn to detect that point and to act on
it when you detect it /anyway/.

Code should IMHO not be structured to be prepared for some
hypothetical future extension. It's better to do that work
("refactoring") when there's an immediate need for it.

- When debugging, you're missing the opportunity to easily single-step
over each involved function, checking the intermediate results.

Can't modern debuggers handle that? I must admit that I never use
stepping symbolic deuggers.

/Jorgen

 

[Felix Palmen]

I object to that. You need to learn to detect that point and to act on
it when you detect it /anyway/.

Sure you /should/, but I guess you know it happens.

Code should IMHO not be structured to be prepared for some
hypothetical future extension. It's better to do that work
("refactoring") when there's an immediate need for it.

And it's best to avoid constructs that make refactoring unnecessarily
hard. At least as long as they don't provide some other benefit.

But I didn't say I dislike chaining and nesting in any case. Those were
just some issues to think about.

Can't modern debuggers handle that? I must admit that I never use
stepping symbolic deuggers.

Maybe I'm missing some feature, but I didn't succeed to display an
intermediate result that is not assigned to a variable in gdb as well as
the microsoft debugger. For the single stepping -- at least using
microsoft's debugger in visual studio, this works, but it's pretty
pointless as long as you don't see a function's return value...

Regards,
Felix

 

[ImpalerCore]

Not in this case.  But I agree that that is a perfectly natural thing
for a search function to do, and completely breaks this approach.

Someone else mentioned it as well, but I would avoid using 'find' in
that function name since most of my experience with functions
containing 'find' or 'search' imply the possibility of returning
NULL. If I wasn't familiar with the function, and didn't read the
documentation, I would likely assume that NULL is a possibility and
automatically add code to guard against NULL dereferencing.

Using 'get' or 'lookup' feels better to me, but these are just my
opinions.

Best regards,
John D.

 

[Heikki Kallasjoki]

Maybe I'm missing some feature, but I didn't succeed to display an
intermediate result that is not assigned to a variable in gdb as well as
the microsoft debugger. For the single stepping -- at least using
microsoft's debugger in visual studio, this works, but it's pretty
pointless as long as you don't see a function's return value...

FWIW, gdb's "finish" command displays the return value, but it is a bit
cumbersome: you need to single-step in, and then "finish" out, like this:

$ cat > test.c
int func(x) { return x + 42; }
int main(void) { return func(42); }

Breakpoint 1, main () at test.c:2
2 int main(void) { return func(42); }
(gdb) step
func (x=42) at test.c:1
1 int func(x) { return x + 42; }
(gdb) fini
Run till exit from #0 func (x=42) at test.c:1
0x00000000004004d6 in main () at test.c:2
2 int main(void) { return func(42); }
Value returned is $1 = 84
(gdb) print $1
$2 = 84

On the positive side, you do get the return value and it is stored into
a gdb variable for further manipulations.

 

[Felix Palmen]

FWIW, gdb's "finish" command displays the return value, but it is a bit
cumbersome: you need to single-step in, and then "finish" out, like this:

Nice, thank you; I'll safe that for later reference.

Regards,
Felix

 

[luser- -droog]

On the other hand, sometimes the resulting code of directly using the
returned pointer just looks awkward.

     foo(bar(plugh)->item[n].addr)->name = "foo";

I little whitespace helps a little:

foo( bar(plugh)->item[n].addr )->name = "foo";

or even:

foo(
bar(plugh)->item[n].addr
)->name = "foo";

 

[Nick]

Someone else mentioned it as well, but I would avoid using 'find' in
that function name since most of my experience with functions
containing 'find' or 'search' imply the possibility of returning
NULL. If I wasn't familiar with the function, and didn't read the
documentation, I would likely assume that NULL is a possibility and
automatically add code to guard against NULL dereferencing.

Using 'get' or 'lookup' feels better to me, but these are just my
opinions.

To be honest, find was a poor example, for just that reason, when I
invented my example.

Here's the actual code I'd first written:

Set_Lookup_String(state->parent->index->holds,V_LINK_NOSKIP,
Radix_Key_To_Id(First_Non_Skippable(state->link->place[1],
state->link, state->skip_things)->id));

Glossary available on request

 

[Paul N]

On the other hand, sometimes the resulting code of directly using the
returned pointer just looks awkward.

     foo(bar(plugh)->item[n].addr)->name = "foo";

I little whitespace helps a little:

     foo( bar(plugh)->item[n].addr )->name = "foo";

or even:

     foo(
         bar(plugh)->item[n].addr
         )->name = "foo";

I think I'd go instead for having a load of intermediate values, with
descriptive names. Perhaps I'm just a wimp.