Is this good code?

[Gaijinco]

I found one of that problems all of us have solve when they begin
programming: given 3 numbers print the greater and the lesser one of
the set.

I was trying to remember the if-then-else structure when something hit
me and I wrote:

// Function returns the greater or lesser numbers of two given numbers,

// specified by a flag: 1 for greater (default), 0 for lesser

int greater_lesser(int a, int b, bool flag=1){

int toBeReturned;

if(flag)
a > b ? toBeReturned = a : toBeReturned = b;
else
a > b ? toBeReturned = b : toBeReturned = a;

return toBeReturned;
}

int main(){

int x, y, z;
cin >> x >> y >> z;
cout << "Greater : " << greater_lesser(greater_lesser(x,y),z) << endl
<< "Lesser : " << greater_lesser(greater_lesser(x,y,0),z,0) << endl;

return 0;
}

The code works all right, but it seems odd, as if it can be improved...

Any thoughts? Thanks!

 

[Jim Langston]

Comments on your code inline:

I found one of that problems all of us have solve when they begin
programming: given 3 numbers print the greater and the lesser one of
the set.

I was trying to remember the if-then-else structure when something hit
me and I wrote:

// Function returns the greater or lesser numbers of two given numbers,

// specified by a flag: 1 for greater (default), 0 for lesser

int greater_lesser(int a, int b, bool flag=1){

bool flag = true

int toBeReturned;

Don't need this.

if(flag)
a > b ? toBeReturned = a : toBeReturned = b;

return a > b ? a : b;

else
a > b ? toBeReturned = b : toBeReturned = a;

return a > b ? b : a;

return toBeReturned;

Don't need this

}

int main(){

int x, y, z;
cin >> x >> y >> z;
cout << "Greater : " << greater_lesser(greater_lesser(x,y),z) << endl
<< "Lesser : " << greater_lesser(greater_lesser(x,y,0),z,0) << endl;

return 0;
}

The code works all right, but it seems odd, as if it can be improved...

Any thoughts? Thanks!

Now, this is not the way I would do it anyway. If I wanted a function to
give me the greater, or lesser of numbers I would allow the number of
elements to be arbitrary. Meaning passing in a reference to a vector and
returning the greater one. There might even already be some STL function
that does that, I don't know enough about all the STL functions to say.

There are many ways to do that, but I'll let others comment on STL methods
to do it before I post anything else cause I'm fairly sure it's simple with
some container iteratation.

 

[Kai-Uwe Bux]

I found one of that problems all of us have solve when they begin
programming: given 3 numbers print the greater and the lesser one of
the set.

I was trying to remember the if-then-else structure when something hit
me and I wrote:

// Function returns the greater or lesser numbers of two given numbers,

// specified by a flag: 1 for greater (default), 0 for lesser

int greater_lesser(int a, int b, bool flag=1){

int toBeReturned;

if(flag)
a > b ? toBeReturned = a : toBeReturned = b;
else
a > b ? toBeReturned = b : toBeReturned = a;

return toBeReturned;
}

int main(){

int x, y, z;
cin >> x >> y >> z;
cout << "Greater : " << greater_lesser(greater_lesser(x,y),z) << endl
<< "Lesser : " << greater_lesser(greater_lesser(x,y,0),z,0) << endl;

return 0;
}

The code works all right, but it seems odd, as if it can be improved...

Why reinvent the wheel?

#include <algorithm>
#include <iostream>

int main ( void ) {
int x, y, z;
std::cin >> x >> y >> z;
std::cout << "max: " << std::max( std::max( x, y ), z ) << '\n'
<< "min: " << std::min( std::min( x, y ), z ) << '\n';
}


Best

Kai-Uwe Bux

 

[Dave Townsend]

I found one of that problems all of us have solve when they begin
programming: given 3 numbers print the greater and the lesser one of
the set.

I was trying to remember the if-then-else structure when something hit
me and I wrote:

// Function returns the greater or lesser numbers of two given numbers,

// specified by a flag: 1 for greater (default), 0 for lesser

int greater_lesser(int a, int b, bool flag=1){

int toBeReturned;

if(flag)
a > b ? toBeReturned = a : toBeReturned = b;
else
a > b ? toBeReturned = b : toBeReturned = a;

return toBeReturned;
}

int main(){

int x, y, z;
cin >> x >> y >> z;
cout << "Greater : " << greater_lesser(greater_lesser(x,y),z) << endl
<< "Lesser : " << greater_lesser(greater_lesser(x,y,0),z,0) << endl;

return 0;
}

The code works all right, but it seems odd, as if it can be improved...

Any thoughts? Thanks!

No, this is horrible!

Flags which control the behavior of a function in such a radical way are
often
confusing and make code difficult to understand, especially something as
simple
as this. Perhaps in a more complex example where a basic algorithm can be
adapted
to perform different variations, flags might be more appropriate, but I
would generally
make that function private and provide simple wrappers of the basic function
to be more
meaningful to a user.

Flags have a habit of multiplying, so we often see two or three flag
parameters in code which
affect the behavior of the function making code harder to understand.


Surely the following is easier to understand ?

#include <iostream>


int lesser( int a, int b)
{
return a < b ? a : b;
}
int greater( int a, int b)
{
return a > b ? a : b;
}
int main()
{
int x, y, z;
std::cin >> x >> y >> z;
std::cout << "Greater : " << greater ( greater (x,y),z) << "\n"
<< "Lesser : " << lesser( lesser(x,y),z) << "\n";

return 0;
}

 

[gottlobfrege]

int greater_lesser(int a, int b, bool flag=1){

int toBeReturned;

if(flag)
a > b ? toBeReturned = a : toBeReturned = b;
else
a > b ? toBeReturned = b : toBeReturned = a;

return toBeReturned;
}

Any thoughts? Thanks!

9 times out of 10, if you have a function of the form:

func(bool flag)
{
// no code before if

if (flag)
...
else
...

// no code after if/else
}

such that there is ***no common code*** before or after the if/else,
then you really have 2 completely separate functions, right?

P.S. When I say '9 times out of 10', in this case I really mean 10
times out of 10.

- Tony

 

[davidrubin]

I found one of that problems all of us have solve when they begin
programming: given 3 numbers print the greater and the lesser one of
the set.

I was trying to remember the if-then-else structure when something hit
me and I wrote:

// Function returns the greater or lesser numbers of two given numbers,
// specified by a flag: 1 for greater (default), 0 for lesser

This is totally bogus, regardless of the implementation.

 

[Ben Bacarisse]

I found one of that problems all of us have solve when they begin
programming

// specified by a flag: 1 for greater (default), 0 for lesser

int greater_lesser(int a, int b, bool flag=1){

Sticking to general matters, in those situations where you need a boolean
flag (this not being one of them) possibly the very worst name for it is
"flag"! Choose a name that identifies what it is "flagging" when true:

bool candidate_found = false;
bool number_has_dot = strchr(number, '.') != NULL;

and so on.

 

[Gaijinco]

It just seemed that a function

int max(int a,int b){
return a > b ? a : b;
}

and another one like

int min(int a,int b){
return a > b ? b : a;
}

Were so similar that they could be rewritten as a single function so
the use of a flag seemed valid. I recognize that a flag named flag is
really dumb.

But if something like

int max_min(int a, int b, bool max=true){

if(max)
return a > b ? a : b;
else
return a > b ? b : a;
}

is a misuse of a flag, then what isn't?

 

[Luke Meyers]

Sticking to general matters, in those situations where you need a boolean
flag (this not being one of them) possibly the very worst name for it is
"flag"! Choose a name that identifies what it is "flagging" when true:

bool candidate_found = false;
bool number_has_dot = strchr(number, '.') != NULL;

I find an exception, in practice, for boolean mutators:

private:
bool happy;
public:
void setHappy(bool flag);

Given that the function name and parameter type contain all the
relevant information, what's the point in trying to express something
additional in the name of the parameter? What would you want to call
it? The best alternative I know is along the lines of:

private:
bool happy_; // Sutter-style trailing underscores for private
member data
public:
public setHappy(bool happy);

But I really don't like keeping the same information in two places like
that. "flag" is perfectly descriptive, when there just isn't enough
semantic content that anything else useful could be conveyed.

Luke

 

[roberts.noah]

I find an exception, in practice, for boolean mutators:

private:
bool happy;
public:
void setHappy(bool flag);

Given that the function name and parameter type contain all the
relevant information, what's the point in trying to express something
additional in the name of the parameter? What would you want to call
it?

What about setHappy(bool turnOn)

Your's makes the assumption that happy is a bool. What if it is a
string? Maybe setHappy(bool) is something like this:

void setHappy(bool on)
{
happy = on ? "I'm happy":"I'm pissy";
}

Granted, one could see the information in the class definition but you
never know; maybe you'll want to change that later. Might be nice to
call the 'flag' variable by what effect it might have.

On the other hand, IMHO the 'setHappy()' function shouldn't exist.
There should be two of them:

void makeHappy();
void makeUnhappy();

Now there is no knowledge of, nor reference to, any internal data
whatsoever...these functions could have any effect or combination of
effects. It also gets rid of an if since in the original your code
ends up being something like (assuming set happy got more complex, as
it naturally does):

if (?) flag = true;
else flag = false;

if (flag)
...
else
...

Finally, if you have a variable that makes sense as a "flag" then you
have an area that could use improvement in design. What effect does
the "happy" flag have? One would assume that it is later used to base
decisions on in other member variables and possibly client classes;
branches. It would be better to use a more OO design - possibly the
state pattern but others might be better depending on the effects
'happy' has.

 

[Ben Bacarisse]

I find an exception, in practice, for boolean mutators:

private:
bool happy;
public:
void setHappy(bool flag);

Good point. I've looked over some old code, I find that I opted for "bool
b" in one case -- probably because I had a blanket prejudice against
"flag" which is much better!

<secondary alternative snipped>

 

[Dave Townsend]

It just seemed that a function

int max(int a,int b){
return a > b ? a : b;
}

and another one like

int min(int a,int b){
return a > b ? b : a;
}

Were so similar that they could be rewritten as a single function so
the use of a flag seemed valid. I recognize that a flag named flag is
really dumb.

But if something like

int max_min(int a, int b, bool max=true){

if(max)
return a > b ? a : b;
else
return a > b ? b : a;
}

is a misuse of a flag, then what isn't?

This is a misuse of a flag because the code otherwise is very simple,
duplicating
the code is better than making it more complicated. True, these functions
have very similar
structure, but would you do the same thing for plus/minus/multiply/divide
since these have the
same basic structure - :

int BinaryArithmetic( int op1, int op2, int code )
{
if (code == 0 ) return op1 + op2;
else if (code ==1 ) return op1 - op2;
else if (code == 2) return op1 * op2;
else if (code ==3) return op1 / op2;
else // uh,oh....
}

( now see what we've got ourselves into, we now have to check that "code" is
a valid value, what do
we do when its not...using separate functions makes this a lot simpler since
we don't have to check).


Perhaps if the code were more complex, say finding the max/min element in a
vector<> , the traversal code is common, you can give a flag to control
the test to find max/min, although this is a bit contrived since STL
algorithms can do this for you. ( C++ world, we'd probably use a functor as
the customizing construct rather than a boolean).

Please don't use flags like this in C++!

dave

 

[Luke Meyers]

What about setHappy(bool turnOn)

I don't like it. It doesn't scan right. Probably because it's a verb,
which conflicts with the basic concept that variables represent state,
and hence are generally nouns and should be named accordingly absent a
compelling reason to vary.

Your's makes the assumption that happy is a bool. What if it is a
string? Maybe setHappy(bool) is something like this:

void setHappy(bool on)
{
happy = on ? "I'm happy":"I'm pissy";
}

I believe we were discussing the *interface* -- namely, the identifier
associated with the bool parameter to a setter/mutator. The
implementation obviously depends on the actual type being set.

More to the point, this is a rather spurious counterexample. If happy
can be appropriately specified by a bool, it's rather questionable to
store it as a string.

Granted, one could see the information in the class definition but you
never know; maybe you'll want to change that later. Might be nice to
call the 'flag' variable by what effect it might have.

If I change the parameter to something besides a bool, I'd be renaming
it anyway. If I changed the semantics of the function in such a way
that the parameter had a different meaning, I would also consider
changing the name. Basically, a parameter name must only consider
whether it is correct for the interface it is part of *as it currently
exists*, not all future possible changes to it. That's an impossible
and useless generalization. Anything which would require you to change
the name from "flag" to something else would be a big enough change
that all dependent code would have to be inspected and possibly
modified anyway. I say again -- in a boolean mutator (setter), the
entire semantic content of the function's signature is sufficiently
conveyed by the function name and the parameter type.

On the other hand, IMHO the 'setHappy()' function shouldn't exist.
There should be two of them:

void makeHappy();
void makeUnhappy();

How is this better? Happy and Unhappy are obviously related and
mutually exclusive possibilities which, taken together, correspond with
a component of the object's state whose natural representation is
boolean in nature. Of course, it's always possible that later we'll
decide to allow for a whole range of moods. To accommodate this, I
would do the following:

class Person {
public:
enum Mood { happy, sad, ambivalent, angry, content };
void setMood(Mood m) { mood_ = m; }

// deprecated in documentation.
// kept, with implementation changed, for compatibility as needed.
void setHappy(bool flag) { setMood(happy); }

private:
Mood mood_;
};

This is a clean, extensible generalization. Were we to take the
approach you suggest, we'd wind up with something more like:

class Person {
public:
void setHappy();
void setUnhappy();
void setAmbivalent();
void setAngry();
void setContent();
private:
// implementation details
};

So while setHappy() and setUnhappy() can seem like a good idea because
it "separates concerns" or "hides implementation details," really all
you've done is treat two particular data values (true and false) as
special cases and written separate interfaces for each. It's easy to
do this for bool because there are only two values in the domain. With
an enumerated type, it's irritating but possible. With pretty much
anything else, it's completely impractical. It doesn't separate
concerns, it tries to represent one thing as two. It produces tighter
coupling, in fact, because clients are tied to the number of methods in
your interface.

Now there is no knowledge of, nor reference to, any internal data
whatsoever...these functions could have any effect or combination of
effects.

I'd rather have functions that made sense and dealt with cohesive,
discrete responsibilities. I'd rather not have two functions modifying
the same state needlessly.

It also gets rid of an if since in the original your code
ends up being something like (assuming set happy got more complex, as
it naturally does):

if (?) flag = true;
else flag = false;

if (flag)
...
else
...

There is nothing wrong with the introduction of a conditional
statement, if it is a clear and effective representation of the logic
being implemented. A conditional statement within a single function
body is less complexity to manage than two separate, but semantically
interwove, functions. If, as in my example above, the requirements
require you to flex in a direction you didn't plan for, such that your
scheme is not extensible, you wind up having to scrap it and start over
with a totally new interface.

The best protection against future change is not to introduce
additional complexity in the name of being flexible, but to represent
concepts in a clear and expressive manner, use good judgement about
appropriate levels of abstraction, and assume as little as possible
about the nature of future changes.

Finally, if you have a variable that makes sense as a "flag" then you
have an area that could use improvement in design. What effect does
the "happy" flag have? One would assume that it is later used to base
decisions on in other member variables and possibly client classes;
branches. It would be better to use a more OO design - possibly the
state pattern but others might be better depending on the effects
'happy' has.

You're being unreasonable. Values with exactly two alternatives,
corresponding in a natural way to true and false, are an integral part
of the logical state of countless entities we might care to represent
as objects. Representing such values as being of type bool, whether in
interfaces or implementation or both, is completely natural and
difficult to argue with as a general practice. It's the most trivially
simple piece of state one can represent (a single bit), and you want to
replace it with a comparatively massive artifice? To what possible
end? How is this better? Design patterns are wonderful, but they are
not the answer to every question.

Luke

 

[I V]

Were so similar that they could be rewritten as a single function so
the use of a flag seemed valid. I recognize that a flag named flag is
really dumb.

But they're not really similar in the relevant sense; on the contrary,
they're opposites! You can, to some extent, see this from the code,
because you have to write the two operations completely separately in
your function (i.e., using the flag does not prevent code duplication):

But if something like

int max_min(int a, int b, bool max=true){

if(max)
return a > b ? a : b;
else
return a > b ? b : a;
}

is a misuse of a flag, then what isn't?

It would be less likely to be a misuse if a) the operations with and
without the flag are similar in the relevant sense, b) using a flag
prevented code duplication, and/or c) the flag is likely to vary at
run-time. Maybe something like:

void doComplexOperation(MyClass data, bool debug=false)
{
if( debug )
logOperation("Complex operation started");

/* ... insert long, complicated sequence of operations on data ...
*/

if( debug )
logOperation("Complex operation finished")
}

 

[Daniel T.]

I find an exception, in practice, for boolean mutators:

private:
bool happy;
public:
void setHappy(bool flag);

Given that the function name and parameter type contain all the
relevant information, what's the point in trying to express something
additional in the name of the parameter? What would you want to call
it? The best alternative I know is along the lines of:

private:
bool happy_; // Sutter-style trailing underscores for private
member data
public:
public setHappy(bool happy);

But I really don't like keeping the same information in two places like
that. "flag" is perfectly descriptive, when there just isn't enough
semantic content that anything else useful could be conveyed.

I would more likely do something like this:

class Foo {
public:
void makeHappy() { _happy = true; }
void makeUnhappy() { _happy = false; }
};

As someone earlier said, what if the implementation changes and 'happy'
is no longer represented as a bool? With 'setHappy(bool)' you would end
up having to put an 'if' statement in the member-function (making it
more complex,) by making two different functions the complexity of each
remains the same.

 

[Luke Meyers]

I would more likely do something like this:

class Foo {
public:
void makeHappy() { _happy = true; }
void makeUnhappy() { _happy = false; }
};

As someone earlier said, what if the implementation changes and 'happy'
is no longer represented as a bool? With 'setHappy(bool)' you would end
up having to put an 'if' statement in the member-function (making it
more complex,) by making two different functions the complexity of each
remains the same.

The specification of the parameter as a bool in no way ties the
implementation to anything. You can take in a boolean value and
perform whatever logic is appropriate. Happy and unhappy are obviously
related, mutually exclusive alternatives. The total amount of
information regarding them which we are prepared to receive from the
client is a single bit. Don't you think it's a bit excessive to write
two functions to handle a single bit of information? Also, as I
pointed out in response to Mr. Roberts, your solution doesn't scale
when there are more than two values in the domain.

Really, you're both just overthinking it, looking for a problem where
none exists. The client needs to indicate a single bit of information;
absent any other information, the best thing is to provide a single
function which takes a bool.

Luke

 

[Daniel T.]

There are many ways to do that, but I'll let others comment on STL methods
to do it before I post anything else cause I'm fairly sure it's simple with
some container iteratation.

int main() {
cout << "Input numbers to compare (non-number to end):\n";
vector<int> vec;
copy( istream_iterator<int>(cin), istream_iterator<int>(),
back_inserter( vec ) );
cout << '\n';
if ( !vec.empty() ) {
cout << "Greatest: " << *max_element( vec.begin(), vec.end() )
<< '\n';
cout << "Least: " << *min_element( vec.begin(), vec.end() )
<< '\n';
}
}

It's instructive to see how max_element works:

template < typename FwIt >
FwIt max_element( FwIt first, FwIt last ) {
if ( first == last ) return first;
FwIt result = first;
while ( ++first != last )
if ( *result < *first )
result = first;
return result;
}

'min_element' is the same except in the "if (*result < *first)" line
where 'first' and 'result' are reversed.

I'm sure Mr. Pibinger will find all of the above simply horrible
"geek-code" including the definition of 'max_element'

To the OP. Whenever you have a requirement dealing with 'x' items (in
this example it was '3' numbers) remember the phrase "one, optional,
many." These are the only ones that matter, either the code deals with
exactly one of them, the item is optional, or otherwise (as in this
case) there are just "a bunch".

 

[Daniel T.]

The specification of the parameter as a bool in no way ties the
implementation to anything. You can take in a boolean value and
perform whatever logic is appropriate.

We're just picking nits here so I'm not going to go much further with
this...

In the 'setHappy(bool flag)' function you probably need some "if (flag)
doX(); else doY();" code. It is likely that the client code *also* must
use a decision ('if' or '?:') to set the flag that is being sent to the
member-function... That's some obvious redundancy that can easily be
removed by creating two member-functions.

Don't you think it's a bit excessive to write two functions to handle
a single bit of information?

Not if it removes redundant decision code.

Also, as I pointed out in response to Mr. Roberts, your solution
doesn't scale when there are more than two values in the domain.

Obviously, if decision code is not at issue, then a different construct
should be used.

Really, you're both just overthinking it, looking for a problem where
none exists. The client needs to indicate a single bit of information;
absent any other information, the best thing is to provide a single
function which takes a bool.

I agree, it's not a big deal either way.

 

[Luke Meyers]

We're just picking nits here so I'm not going to go much further with
this...


In the 'setHappy(bool flag)' function you probably need some "if (flag)
doX(); else doY();" code. It is likely that the client code *also* must
use a decision ('if' or '?:') to set the flag that is being sent to the
member-function... That's some obvious redundancy that can easily be
removed by creating two member-functions.

I think it's unreasonable to expect that client code will usually have
a conditional statement whose semantice are essentially the same as the
decision between happy and unhappy. Not every invokation is going to
look like:

if (cond) setHappy(true);
else setHappy(false);

It's as likely to be:

void f() {
doStuff();
setHappy(true);
doMoreStuff();
}

void g() {
doDifferentStuff();
setHappy(false);
doMoreDifferentStuff();
}

Here, setting the state of happy is part of a larger chunk of logic --
the decision to perform all this logic is at a higher level than the
happy/unhappy decision. So, it is not in any way redundant with any
conditional logic within the implementation of setHappy()! There are a
*lot* of ways to generate a boolean value, as well.

Not if it removes redundant decision code.

It doesn't do that at all. At some point, I've got to decide which
function to call. If I could just pass a boolean value, I could let
setHappy() worry about the conditional structure. But since you've
separated one responsibility into two functions, the client must
generate a boolean value, inspect it, and decide which function to call
accordingly. E.g.:

bool decide(char c);

void f(char c) {
bool cond = decide(c);
cond ? setHappy() : setUnhappy();
}

// versus:

void g(char c) {
setHappy(decide(c));
}

Consider a class which provided all of setHappy(bool flag), setHappy(),
and setUnhappy(). Which of the three do you imagine would see the most
use by clients?

Obviously, if decision code is not at issue, then a different construct
should be used.

Are you suggesting that the appellation "decision code" only applies to
behavior that depends on the value of an expression whose type is bool,
as opposed to any other type? Why treat bool differently in this
respect?

I agree, it's not a big deal either way.

Well, I'm not worked up about it or anything, but I maintain that your
way is poor design, and poor design is very much a big deal, at least
in my line of work.

Consider one more argument. A class's actual state is an
implementation of its logical state, which is visible through the
class's interface. The simpler the interface, the easier for clients
to use. The mapping between interface and logical state should be
intuitive and avoid unnecessary proliferation of entities or
relationships (couplings). A simple (well, trivial) example of this
is:

class Foo {
int x_;
public:
void setX(int x) { x_ = x; }
int getX() const { return x_; }
};

Here, there is a direct mapping between logical state and its
implementation. setHappy(bool) possesses this same simplicity.
Conversely, setHappy()/setUnhappy() introduces an extra entity and an
implicit (in programmatic terms) semantic relationship between them,
because they manipulate the same piece of logical state.

Incidentally, what about accessors? Would you write:

bool isHappy() const { return happy_; }
bool isUnhappy() const { return not happy_; } // ?

See what a problem this coupling presents? A good program is concisely
expressive, and contains as much of the designer's intent as possible.
Where does your implementation express the invariant that (isHappy() ==
not isUnhappy()) is always true? Why do people generally implement
operator!= in terms of operator==?

Luke

 

[roberts.noah]

More to the point, this is a rather spurious counterexample. If happy
can be appropriately specified by a bool, it's rather questionable to
store it as a string.

I was just giving an example to show that there could be a lot more
going on.

How is this better?

For one thing it makes more sense. The specifics of what
setHappy(true) vs. setHappy(false) are not entirely clear: is
setHappy(false) just not being happy or being unhappy? There is no
doubt what the intention of the above is.

class Person {
public:
enum Mood { happy, sad, ambivalent, angry, content };
void setMood(Mood m) { mood_ = m; }

// deprecated in documentation.
// kept, with implementation changed, for compatibility as needed.
void setHappy(bool flag) { setMood(happy); }

private:
Mood mood_;
};

This is a clean, extensible generalization.

You have changed the behavior of setHappy!! Now there is absolutely no
use in the parameter at all!! You are implementing my "solution" even
as you argue against it only you are just implementing half of it,
leaving the rest undone.

Yes, it is a silly mistake and not what you meant...but look how easily
that happened. When you see "setHappy" you are not considering the
alternative. It happened because it is more natural to think of
setHappy as having the effect of making the object happy, not in making
it not so. This is actually a rather important part of the discussion
and a fundamental issue.

Were we to take the

approach you suggest, we'd wind up with something more like:

class Person {
public:
void setHappy();
void setUnhappy();
void setAmbivalent();
void setAngry();
void setContent();
private:
// implementation details
};

Incorrect. First as I mentioned before when there is a "flag" variable
you usually have room to improve your design. Second none of those
functions should exist. Just like you can't reach into someone's brain
and make them happy, angry, whatever...you shouldn't be able to do so
with an object. "setter" functions are another indication of bad
design; by what right does one object have to touch the privates of
another? Instead, actions that Person takes or that are performed on
Person should create this effect. In other words there should be no
interface for *setting* a person's mood although there may be reason
enough to "interpretMood()" (just in case you might behave differently
with an angry person vs. a happy one) which would likely return some
sort of mood object (that might have query functions like
"mightBeSnappy()" better yet you might want that query in Person itself
and avoid returning a mood at all) for unlike you are expressing, moods
are not mutually exclusive and can be very complex.

Also, why shouldn't there be setX() for certain common sets of moods?
For instance, setContent() might also make a person happy, no?
Operations that are commonly done in a set might well be implemented in
a function instead of leaving it all to the client every time.

Finally you are unfairly misrepresenting the possiblities. Just as you
can "adjust" your interface with the addition of an extra function so
can I:

setMood(Mood m) { mood_ |= m; }
unsetMood(Mood m) { mood_ &= ~m; }
makeHappy() { setMood(happy); }
makeUnhappy() { unsetMood(happy); }

I have just performed the same refactoring algorithm you did and the
result is very much the same. There is no /comparitive/ scaling issues
here. In fact as you can see, my code can represent a vast count more
moods and combination of moods than yours can. Your way would require
that I build my mood mask and pass it in even though I probably only
want to change a particular aspect of the mood.

So while setHappy() and setUnhappy() can seem like a good idea because
it "separates concerns" or "hides implementation details," really all
you've done is treat two particular data values (true and false) as
special cases and written separate interfaces for each. It's easy to
do this for bool because there are only two values in the domain. With
an enumerated type, it's irritating but possible. With pretty much
anything else, it's completely impractical. It doesn't separate
concerns, it tries to represent one thing as two. It produces tighter
coupling, in fact, because clients are tied to the number of methods in
your interface.

Such drastic growth indicates to me that an object is needed to
represent mood. Neither of our "solutions" scale. The problem
actually stems from the very fundamental issue of having setter
functions and state flags.

I'd rather have functions that made sense and dealt with cohesive,
discrete responsibilities. I'd rather not have two functions modifying
the same state needlessly.

Well that depends. What is a person likely to do upon becomming happy
vs. unhappy? Such things could easily grow beyond the feasability of
being done in a single function. At this point you have a function
performing two responsibilities...doing a bunch of stuff to make a
person happy as well as doing a bunch of stuff to make a person not
happy. Is a person likely to do the same thing when becomming happy as
they would when becomming not happy? I doubt it. Now you have a
single function performing two *unrelated* tasks that have the sole
relation of an input flag.

Imagine:

setHappy(bool flag)
{
if (flag)
{ laugh(); smile(); beNice(); throwParty(); }
else
{ cry(); drownInBooze(); beAntisocial(); }
happy = flag;
}

And that is a very simple case but notice how very little is actually
related at all. Becomming happy vs. unhappy have drastically different
effects on the Person in question.

Consider now that you have people with different cultures that have
different moods based on different events. Obviously the very concept
of having a setter for happyness is flawed as this should be done by
the object itself in response to what is done with/to it.

There is nothing wrong with the introduction of a conditional
statement, if it is a clear and effective representation of the logic
being implemented. A conditional statement within a single function
body is less complexity to manage than two separate, but semantically
interwove, functions. If, as in my example above, the requirements
require you to flex in a direction you didn't plan for, such that your
scheme is not extensible, you wind up having to scrap it and start over
with a totally new interface.

You do anyway. My functionality is almost exactly as "extensible"
(actually should say non-extensible) as yours.

The best protection against future change is not to introduce
additional complexity in the name of being flexible, but to represent
concepts in a clear and expressive manner, use good judgement about
appropriate levels of abstraction, and assume as little as possible
about the nature of future changes.


You're being unreasonable.

So you say but the whole thing fell appart, didn't it. Think about how
much of the behavior of a Person would be altered by a mood. Would
"performGreeting()" be the same for example? ( happy ? say("Nice to see
you"):say("Go to hell.") ) I think you are looking at a state and when
you look at a state there is a pattern that can be used, even in simple
cases, that is VERY extensible and clean.

Consider also how much of a person's mood is based on how they
interpret what happens to them and how little is actually based on the
events themselves. No two people feel the same way about the same
thing. Public setter functions take responsibility from where it
belongs, the object itself, and place it in objects that have no
business making these decisions.

There is a difference between thinking ahead a little, or just using
common tools available to you that are known to scale, and "analysis
paralysis". Just looking at something like a public setter and state
flag warned me that there were issues...and there are. Counter and
switch variables have their place but usually limited to a small scope
like a single function or less. If there IS a good reason to have a
flag variable embedded in an object then it is almost never a good
thing to have public setter functions for that flag...why do external
objects need to set your internal state?? Truely there is almost never
a good reason and such constructs almost always indicade poor design
decisions.

In other words your argument for using "flag" in a variable name under
such circumstances is flawed by the fact that such circumstances are
flawed.

Now, occasionally you may run into a reason why it is necessary to do
all of those things that indicate bad design but I have yet to run into
one that wasn't, "because it is already like that and I'm not allowed
(or don't have time) to change it." Occasionally there is, "I couldn't
think of nothing better," but I don't think of that as a *good* reason.