Re: [GnomeMeeting-devel-list] [PRE-PATCH] private struct _GmTextChat

[PUYDT Julien <julien puydt laposte net>]

On dim, 2004-01-25 at 21:26, Damien Sandras wrote:
> Hi,
> 
> I only have a few things to comment on :
> 
> - that kind of patch is, believe me, very dangerous. 

Why so?

> - I don't like 1 line functions, I don't think it makes much sense to
> write 4 lines of code for a 1-line function.

Well... since _GmTextChat is internal to the text-chat:
* gnomemeeting.cpp can't instantiate and delete it (hence the need to
add two functions: one _new and one _delete);
* main_window.cpp needs a way to get access to the window, but doesn't
know where it is in the structure, so there must be a function that
returns that information!

> - Functions to retrieve things should contain "get" in their names

Well, in fact the first draft of that patch had
"gnomemeeting_text_chat_widget" ; I didn't like the name and patched my
patch so it became "gnomemeeting_text_chat_window"... That function may
disappear, see below.

> - Things like :
> gnomemeeting_text_chat_window (GmTextChat *chat)
> {
>   return chat->window;
> }
> 
> do not make much sense I believe. Will you do a one line function for
> each of the fields present in all structures?

Well, I wouldn't have hidden the structure if all of the fields where
supposed to be public. In fact, after a fresh night, I would rather go
for:
typedef struct _GmTextChat GmTextChat;
typedef struct _GmTextChatPrivate GmTextChatPrivate
struct _GmTextChat {
/* public members, like the window */
GmTextChatPrivate *private;
}

And then, only define _GmTextChatPrivate in chat_window.cpp. That scheme
would give us:
1) both a public part (no need to write an access function for each
variable that we don't really want to hide, and makes it easy to add a
new public one),
2) and a private part (that makes it possible to modify the internal
implementation of the text-chat at will).

The bonus is that the same scheme could be used for the other structures
in common.h.

> - gnomemeeting_text_chat_window is misnamed too. See the rest of the API
> to name it in a similar way.

Isn't that point redondant to the "get" above? Anyway, that function
will perhaps disappear.

> Except this, I more or less like the idea.

I hope the new precision will make the more bigger and the less smaller
;-)

Snark