Is this good code?

Discussion in 'C++' started by Gaijinco, Feb 5, 2006.

  1. Gaijinco

    Gaijinco Guest

    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!
    Gaijinco, Feb 5, 2006
    #1
    1. Advertising

  2. Gaijinco

    Jim Langston Guest

    Comments on your code inline:

    "Gaijinco" <> wrote in message
    news:...
    >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.
    Jim Langston, Feb 5, 2006
    #2
    1. Advertising

  3. Gaijinco

    Kai-Uwe Bux Guest

    Gaijinco wrote:

    > 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
    Kai-Uwe Bux, Feb 5, 2006
    #3
  4. "Gaijinco" <> wrote in message
    news:...
    > 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;
    }
    Dave Townsend, Feb 5, 2006
    #4
  5. Gaijinco

    Guest

    Gaijinco wrote:
    >
    > 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
    , Feb 5, 2006
    #5
  6. Gaijinco

    Guest

    Gaijinco wrote:
    > 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.
    , Feb 5, 2006
    #6
  7. On Sat, 04 Feb 2006 20:02:46 -0800, Gaijinco wrote:

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

    <snip>
    > // 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.

    --
    Ben.
    Ben Bacarisse, Feb 5, 2006
    #7
  8. Gaijinco

    Gaijinco Guest

    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?
    Gaijinco, Feb 5, 2006
    #8
  9. Gaijinco

    Luke Meyers Guest

    Ben Bacarisse wrote:
    > 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
    Luke Meyers, Feb 5, 2006
    #9
  10. Gaijinco

    Guest

    Luke Meyers wrote:
    > Ben Bacarisse wrote:
    > > 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?


    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.
    , Feb 5, 2006
    #10
  11. On Sun, 05 Feb 2006 11:09:43 -0800, Luke Meyers wrote:

    > Ben Bacarisse wrote:
    >> 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);


    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>

    --
    Ben.
    Ben Bacarisse, Feb 5, 2006
    #11
  12. "Gaijinco" <> wrote in message
    news:...
    > 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
    Dave Townsend, Feb 5, 2006
    #12
  13. Gaijinco

    Luke Meyers Guest

    wrote:
    > Luke Meyers wrote:
    > > Ben Bacarisse wrote:
    > > > 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?

    >
    > 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
    Luke Meyers, Feb 5, 2006
    #13
  14. Gaijinco

    I V Guest

    Gaijinco wrote:
    > 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")
    }
    I V, Feb 6, 2006
    #14
  15. Gaijinco

    Daniel T. Guest

    In article <>,
    "Luke Meyers" <> wrote:

    > Ben Bacarisse wrote:
    > > 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.


    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.

    --
    Magic depends on tradition and belief. It does not welcome observation,
    nor does it profit by experiment. On the other hand, science is based
    on experience; it is open to correction by observation and experiment.
    Daniel T., Feb 6, 2006
    #15
  16. Gaijinco

    Luke Meyers Guest

    Daniel T. wrote:
    > 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
    Luke Meyers, Feb 6, 2006
    #16
  17. Gaijinco

    Daniel T. Guest

    In article <M5fFf.3671$>,
    "Jim Langston" <> wrote:

    > 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".

    --
    Magic depends on tradition and belief. It does not welcome observation,
    nor does it profit by experiment. On the other hand, science is based
    on experience; it is open to correction by observation and experiment.
    Daniel T., Feb 6, 2006
    #17
  18. Gaijinco

    Daniel T. Guest

    In article <>,
    "Luke Meyers" <> wrote:

    > Daniel T. wrote:
    > > 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.


    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.

    --
    Magic depends on tradition and belief. It does not welcome observation,
    nor does it profit by experiment. On the other hand, science is based
    on experience; it is open to correction by observation and experiment.
    Daniel T., Feb 6, 2006
    #18
  19. Gaijinco

    Luke Meyers Guest

    Daniel T. wrote:
    > In article <>,
    > "Luke Meyers" <> wrote:
    > 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.

    > > 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.


    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?

    > > 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.


    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?

    > > 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.


    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
    Luke Meyers, Feb 6, 2006
    #19
  20. Gaijinco

    Guest

    Luke Meyers wrote:

    > 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.
    >


    > > 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?


    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.

    >
    > > 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.


    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.

    >
    > > 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.


    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.
    >
    > > 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.


    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.
    , Feb 6, 2006
    #20
    1. Advertising

Want to reply to this thread or ask your own question?

It takes just 2 minutes to sign up (and it's free!). Just click the sign up button to choose a username and then you can ask your own questions on the forum.
Similar Threads
  1. sikka noel
    Replies:
    8
    Views:
    413
    Mike Wahler
    Aug 5, 2003
  2. vlsidesign
    Replies:
    26
    Views:
    951
    Keith Thompson
    Jan 2, 2007
  3. Cliff  Martin
    Replies:
    1
    Views:
    3,022
    Larry Smith
    Jan 31, 2007
  4. SM
    Replies:
    9
    Views:
    490
  5. Casey Hawthorne
    Replies:
    18
    Views:
    602
    Beej Jorgensen
    Nov 6, 2009
Loading...

Share This Page