Opened 13 years ago

Closed 13 years ago

#323 closed enhancement (fixed)

Parameter class style

Reported by: Jari Häkkinen Owned by: Jari Häkkinen
Priority: major Milestone: svndigest 0.7
Component: core Version: trunk
Keywords: Cc:

Description

I think the help() member function should be declared const. There is a call to defaults() in help() that should be moved to the constructor.

This is simply a question of style, I prefer printing help not to change the state of the object, it should be the choice of the caller. Peter, you are free to make a verdict on this.

Change History (4)

comment:1 Changed 13 years ago by Peter Johansson

Well, in principle I have nothing against making function help() const. However, the function needs to know what the default are because this is displayed. Previously it was hard-coded, which was error prone. I interpret your description that default() should be moved to outside help() function just before help() function. I think that would be bad style because it is harder to maintain. Say that we add a call to help90 elsewhere, it would easy to miss about calling default(), and the help displayed will be wrong. I prefer maintainability before constness fetischsm.

A better solution would be to add info about defaults in member variables. One could, for example, replace bool verbose_ with pair<bool, const bool> verbose_. Then we don't need to change the state to know what the default are.

comment:2 Changed 13 years ago by Jari Häkkinen

I prefer the more complicated solution in favour of changing the Parameter object. Allowing help() to change the object makes sense in out current code because we obey the rule "Always exit after calling help" (remember that comment?). I think it is unexpected that a help call actually changes the object. Of course, here help() is private so ... Should we implement your suggestion with pair<>+?

comment:3 Changed 13 years ago by Peter Johansson

Milestone: svndigest 0.x+svndigest 0.7

Sure!

comment:4 Changed 13 years ago by Peter Johansson

Resolution: fixed
Status: newclosed

fixed in [625]

Note: See TracTickets for help on using tickets.