Is this good code?

[Daniel T.]

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()!

Of course it is! You say yourself that some "chunk of logic" outside the
object is making the decision between 'happy' and 'unhappy', so why
would you *also* want a chunk of logic *inside* the object making the
exact same decision?

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();
}

I'd consider the above poor design as well. A decision is being made in
the 'decide' function, then again in the 'f' function. Duplicated
decision logic is bad, not good. But I think I understand your point.

// versus:

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

Again, somewhere inside the 'decide' method, a decision is being made as
to wether or not this object should be happy or unhappy. By putting
conditional logic inside the 'setHappy' method, you are forcing the
decision to be made yet again.

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?

Because 'bool' is a logical, rather than mathematical entity. It's the
old "one, optional, many" chestnut of programming. If there is a
one-to-one relationship then it should be treated one way, an optional
relationship should be treated another, and a 0 to many relationship
should be treated yet a third.

The example used in the other sub-thread is a great example:

class Object {
public:
enum emotion {happy, sad, angry};
void setEmotion( emotion e );
};

The above makes sense because it is reasonable to assume that more
emotions will be added.

There is a fundamental difference between 1, 0..1, and 0..*. Trying to
make a 0..1 relationship look like a 0..* relationship needlessly
increases maintenance and demonstrates a misunderstanding of the problem
space.

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.

Poor design is always a bid deal, but maybe this isn't a issue for
comp.lang.c++. Care to continue in comp.object? (I've already started a
thread on it.)

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.

No, it doesn't. All over the inside of Foo, are decisions "if (_happy)"
and even if we later factor those decisions out using a strategy
pattern, there will still have to be a decision made in the setHappy
method itself, despite the fact that the decision was *already made*
before the method was called.

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_; } // ?

This is a completely different issue. The client code is ether doing "if
(object.isHappy() == true)" or "if (object.isHappy() == false)", nothing
at all is to be gained by providing the second 'isUnhappy' method
because it does not serve to remove duplicated logic. Having two
separate set methods *does* remove duplicated logic.

And that's my final word in this forum at least. I'm interested to see
what the question generates in comp.object...

 

[Greg Buchholz]

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.

// Instead of a flag, write a generic compare routine and pass in a
// function object which does the acutal comparison.

#include<iostream>
#include<algorithm>

using namespace std;

template<class T, class F> T cmp(T a, T b, F f)
{
return f(a,b) ? a : b;
}

int main()
{
cout << cmp(2, 3, less<int>()) << "\n";
cout << cmp(2.0,3.0,greater<double>()) << "\n";

return 0;
}

 

[Daniel T.]

// Instead of a flag, write a generic compare routine and pass in a
// function object which does the acutal comparison.

#include<iostream>
#include<algorithm>

using namespace std;

template<class T, class F> T cmp(T a, T b, F f)
{
return f(a,b) ? a : b;
}

int main()
{
cout << cmp(2, 3, less<int>()) << "\n";
cout << cmp(2.0,3.0,greater<double>()) << "\n";

return 0;
}

Your supposed to be comparing three numbers, not just two...

 

[Greg Buchholz]

#include<iostream>
#include<algorithm>

int main()
{
int nums[] = { 3,1,4,1,5,9,2,6,5,4 };

std::cout << "min: " << *(std::min_element(nums,nums+10))
<< ", max: " << *(std::max_element(nums,nums+10));

return 0;
}


....Then, if you still wanted to roll your own you could use...

int maximum = accumulate(nums,nums+10,*nums,std::max<int>);

....but that doesn't compile (complaining about <unknown type>) with my
copy of g++-3.4.2. But it does work if you define...

template<class T> T my_max(T a, T b) { return std::max(a,b); }

....and then use it like...

int maximum = accumulate(nums,nums+10,*nums,my_max<int>);

....I'd be interested in knowing if it is possible to get the accumulate
version to work without the wrapper function.

Thanks,

Greg Buchholz

 

[Daniel T.]

#include<iostream>
#include<algorithm>

int main()
{
int nums[] = { 3,1,4,1,5,9,2,6,5,4 };

std::cout << "min: " << *(std::min_element(nums,nums+10))
<< ", max: " << *(std::max_element(nums,nums+10));

return 0;
}


...Then, if you still wanted to roll your own you could use...

int maximum = accumulate(nums,nums+10,*nums,std::max<int>);

...but that doesn't compile (complaining about <unknown type>) with my
copy of g++-3.4.2. But it does work if you define...

template<class T> T my_max(T a, T b) { return std::max(a,b); }

...and then use it like...

int maximum = accumulate(nums,nums+10,*nums,my_max<int>);

...I'd be interested in knowing if it is possible to get the accumulate
version to work without the wrapper function.

It can be done, but it isn't very pretty.

int main() {
int nums[] = { 3,1,4,1,5,9,2,6,5,4 };
const int& (*m)(const int&, const int&) = max<int>;
int i = accumulate( nums, nums + 10, *nums, m );
assert( i == 9 );
}

I think the problem is that there are two 'max' functions, you need some
way to explicitly tell the compiler which one to use otherwise max<int>
is ambiguous.