Style question - forwarding to subclasses

[gw7rib]

I'm writing a program involving having notes which can store text. In
the beginning, I had a class called Note which did nearly everything,
but I felt it was getting too big and so have created other classes to
deal with the text handling and with the displaying of the note. So
some functions which were previously handled by the Note class are
forwarded to these other classes. For example, the definition of the
class includes:

void hide(void) { display.hide(); }
void cleartext(void) { text.cleartext(); }
void repaint(int newsize) { display.repaint(newsize); }
int isshowing(void) { return display.isshowing(); }
void adjustHscroll(void) { display.adjustHscroll(); }
void adjustVscroll(void) { display.adjustVscroll(); }
int getystep(void) { return display.getystep(); }
int getmousestep(void) { return display.getmousestep(); }
void notehidden(void) { display.notehidden(); }

I thought this was a little ugly and so was considering using macros
to change it to:

FORWARD (display, hide)
FORWARD (text, cleartext)
FORWARDvi (display, repaint, newsize)
FORWARDi (display, isshowing)
FORWARD (display, adjustHscroll)
FORWARD (display, adjustVscroll)
FORWARDi (display, getystep)
FORWARDi (display, getmousestep)
FORWARD (display, notehidden)

Any comments on this? Does it look neat? Or ugly? Are there flaws I
haven't thought of? Does the fact that so many routines are forwarded
show the whole design is bad? Can you come up with better names?

Thanks for any thoughts.
Paul.

 

[Victor Bazarov]

I'm writing a program involving having notes which can store text. In
the beginning, I had a class called Note which did nearly everything,
but I felt it was getting too big and so have created other classes to
deal with the text handling and with the displaying of the note. So
some functions which were previously handled by the Note class are
forwarded to these other classes. For example, the definition of the
class includes:

void hide(void) { display.hide(); }
void cleartext(void) { text.cleartext(); }
void repaint(int newsize) { display.repaint(newsize); }
int isshowing(void) { return display.isshowing(); }
void adjustHscroll(void) { display.adjustHscroll(); }
void adjustVscroll(void) { display.adjustVscroll(); }
int getystep(void) { return display.getystep(); }
int getmousestep(void) { return display.getmousestep(); }
void notehidden(void) { display.notehidden(); }1

First off, drop all those "void" keywords between the parentheses. If
there is to be nothing, don't use anything. It's like putting "This
page is intentionally left blank" on a blank page. If it's to be blank,
keep it blank.

I thought this was a little ugly and so was considering using macros
to change it to:

FORWARD (display, hide)
FORWARD (text, cleartext)
FORWARDvi (display, repaint, newsize)
FORWARDi (display, isshowing)
FORWARD (display, adjustHscroll)
FORWARD (display, adjustVscroll)
FORWARDi (display, getystep)
FORWARDi (display, getmousestep)
FORWARD (display, notehidden)

Any comments on this? Does it look neat? Or ugly? Are there flaws I
haven't thought of? Does the fact that so many routines are forwarded
show the whole design is bad? Can you come up with better names?

The original does NOT look ugly. However, I'd have hard time grasping
the code with all those macros (they give me shivers). Perhaps if you,
say, combined the functions slightly differently in the class definition
and provided more proper declaration syntax (and semantics, think
'const'), then...

// the following manipulates the 'text' data member
void cleartext() { text.cleartext(); }

// the following ones change the 'diplay' data member
void hide() { display.hide(); }
void repaint(int newsize) { display.repaint(newsize); }
void adjustHscroll() { display.adjustHscroll(); }
void adjustVscroll() { display.adjustVscroll(); }

void notehidden() { display.notehidden(); } // ???

// those return the state of the 'display' data member
int isshowing() const { return display.isshowing(); }
int getystep() const { return display.getystep(); }
int getmousestep() const { return display.getmousestep(); }

I marked 'notehidden' because its name sounds like an accessor for some
binary/boolean flag, but the functionality apparently doesn't follow.

Also, consider that 'adjustHscroll' and 'adjustVscroll' go against your
style which has no capital letters. If there seems to be the need to
have capital letters in those member function names, perhaps you should
consider having them everywhere (which means you will use camelCase, or
you could use underscore_separated_words like the Standard Library does).

V

 

[gw7rib]

Thanks for your comments, Victor.

(etc)

First off, drop all those "void" keywords between the parentheses.  If
there is to be nothing, don't use anything.  It's like putting "This
page is intentionally left blank" on a blank page.  If it's to be blank,
keep it blank.

It makes a difference in C, but it doesn't in C++ - one day I'll
persuade myself to write in C++ properly...

(etc)

The original does NOT look ugly.  However, I'd have hard time grasping
the code with all those macros (they give me shivers).  Perhaps if you,
say, combined the functions slightly differently in the class definition
and provided more proper declaration syntax (and semantics, think
'const'), then...

   // the following manipulates the 'text' data member
   void cleartext() { text.cleartext(); }

   // the following ones change the 'diplay' data member
   void hide()               { display.hide(); }
   void repaint(int newsize) { display.repaint(newsize); }
   void adjustHscroll()      { display.adjustHscroll();  }
   void adjustVscroll()      { display.adjustVscroll();  }

   void notehidden()         { display.notehidden(); }    // ???

   // those return the state of the 'display' data member
   int isshowing() const    { return display.isshowing();  }
   int getystep()  const    { return display.getystep();    }
   int getmousestep() const { return display.getmousestep(); }

Yes, this does look neater, just by lining things up nicely. This
could well be the way forward.

The comments look a little odd, though. For instance, "hide" causes
the note, previously shown in a window, to disappear. In my view it's
something you genuinely do to the note, not just a request to change
the data in the subclass, even if the note deals with it completely in
the subclass.

I marked 'notehidden' because its name sounds like an accessor for some
binary/boolean flag, but the functionality apparently doesn't follow.

Ah - the display class stores a handle to the window. This function
sets that stored handle to NULL when the window closes. IE the Note
should "note" that it is now "hidden" ie it is not on display any
more.

Also, consider that 'adjustHscroll' and 'adjustVscroll' go against your
style which has no capital letters.  If there seems to be the need to
have capital letters in those member function names, perhaps you should
consider having them everywhere (which means you will use camelCase, or
you could use underscore_separated_words like the Standard Library does).

Yes, the code has a variety of styles - camelCase, using_underscores
and wordsjustshovedtogether - and would perhaps benefit from being
rationalised. One feature seems to be that if a function has a very
long name then it needs care in using it, which seems a reasonable
approach.

 

[Victor Bazarov]

Thanks for your comments, Victor.



It makes a difference in C, but it doesn't in C++ - one day I'll
persuade myself to write in C++ properly...


Yes, this does look neater, just by lining things up nicely. This
could well be the way forward.

The comments look a little odd, though. For instance, "hide" causes
the note, previously shown in a window, to disappear. In my view it's
something you genuinely do to the note, not just a request to change
the data in the subclass, even if the note deals with it completely in
the subclass.

It's up to you to change them so they make sense. I don't even want to
pretend that I know your functionality. The comments are for you, to
explain what criterion I used for splitting them up and grouping them.

Ah - the display class stores a handle to the window. This function
sets that stored handle to NULL when the window closes. IE the Note
should "note" that it is now "hidden" ie it is not on display any
more.

For some reason 'note' is rarely used in my environment as a verb
(although I do use it in Usenet postings)... Since there is noun/verb
ambiguity I'd probably use another word to disambiguate, like 'set' or
'store'.

Also, consider that 'adjustHscroll' and 'adjustVscroll' go against your
style which has no capital letters. If there seems to be the need to
have capital letters in those member function names, perhaps you should
consider having them everywhere (which means you will use camelCase, or
you could use underscore_separated_words like the Standard Library does).

Yes, the code has a variety of styles - camelCase, using_underscores
and wordsjustshovedtogether - and would perhaps benefit from being
rationalised. [..]

I believe the mantra AFA style is concerned is, "does not matter *what*
style you have, as long as you have one, and use it *consistently*."

As soon as you started asking questions, you're on the right track.
Good luck!

V

 

[mzdude]

I thought this was a little ugly and so was considering using macros
to change it to:

  FORWARD   (display, hide)
  FORWARD   (text,    cleartext)
  FORWARDvi (display, repaint, newsize)
  FORWARDi  (display, isshowing)
  FORWARD   (display, adjustHscroll)
  FORWARD   (display, adjustVscroll)
  FORWARDi  (display, getystep)
  FORWARDi  (display, getmousestep)
  FORWARD   (display, notehidden)

Any comments on this? Does it look neat? Or ugly? Are there flaws I
haven't thought of?

Stepping through macros with a debugger can be problematic.
Single line expansion might be handled correctly.
Multiple lines can cause problems. Just my 0.02

 

[Jim Langston]

I'm writing a program involving having notes which can store text. In
the beginning, I had a class called Note which did nearly everything,
but I felt it was getting too big and so have created other classes to
deal with the text handling and with the displaying of the note. So
some functions which were previously handled by the Note class are
forwarded to these other classes. For example, the definition of the
class includes:

void hide(void) { display.hide(); }
void cleartext(void) { text.cleartext(); }
void repaint(int newsize) { display.repaint(newsize); }
int isshowing(void) { return display.isshowing(); }
void adjustHscroll(void) { display.adjustHscroll(); }
void adjustVscroll(void) { display.adjustVscroll(); }
int getystep(void) { return display.getystep(); }
int getmousestep(void) { return display.getmousestep(); }
void notehidden(void) { display.notehidden(); }

I thought this was a little ugly and so was considering using macros
to change it to:

FORWARD (display, hide)
FORWARD (text, cleartext)
FORWARDvi (display, repaint, newsize)
FORWARDi (display, isshowing)
FORWARD (display, adjustHscroll)
FORWARD (display, adjustVscroll)
FORWARDi (display, getystep)
FORWARDi (display, getmousestep)
FORWARD (display, notehidden)

Any comments on this? Does it look neat? Or ugly? Are there flaws I
haven't thought of? Does the fact that so many routines are forwarded
show the whole design is bad? Can you come up with better names?

Oh gawd please no. It is so hard to follow code where they #define things
and to find out what is actually going on you have to go look up #defines,
and when trying to debug... In C it worked, in C it was pretty much the only
thing we had, in C++ we have classes and such. So please, forgo the
needless #defines.

In one package I was using he acutally had:

#define TDF typedef

Please!

Thanks.

 

[Alf P. Steinbach]

* blargg:

an instance of the
Don't Repeat Yourself (DRY) principle.

Heh, thanks,

- Alf (collector of profound acronyms currently extant throughout IT community)