design question

[David Rubin]

This is a question related to my previous post titled "private member
functions:"

I recently saw that a colleage of mine submitted code which defines a
class Interface as follows[1]:

class Interface {
public:
Interface(UINT maxidx);

void set(UINT idx, UCHAR data);
/* ... */
private:
UINT d_maxidx;

bool isInRange(UINT idx) { return idx < d_maxidx; }
};

My first reaction to this code is that isInRange should have been
defined as a static function in the Interface.cc translation unit rather
than a member function:

static bool isInRange(UINT idx, UINT max) { return idx < max; }

Is my reaction correct? Any opinions?

/david

[1] simplified from original version

--
Andre, a simple peasant, had only one thing on his mind as he crept
along the East wall: 'Andre, creep... Andre, creep... Andre, creep.'
-- unknown

[David Rubin]

David Rubin wrote:

I recently saw that a colleage of mine submitted code which defines a
class Interface as follows[1]:
[snip - class definition] My first reaction to this code is that isInRange should have been
defined as a static function in the Interface.cc translation unit rather
than a member function:

Forgot to mention that this class will not be used as a base class.

/david

--
Andre, a simple peasant, had only one thing on his mind as he crept
along the East wall: 'Andre, creep... Andre, creep... Andre, creep.'
-- unknown

[Victor Bazarov]

"David Rubin" <bo***********@nomail.com> wrote...

This is a question related to my previous post titled "private member
functions:"

I recently saw that a colleage of mine submitted code which defines a
class Interface as follows[1]:

class Interface {
public:
Interface(UINT maxidx);

void set(UINT idx, UCHAR data);
/* ... */
private:
UINT d_maxidx;

bool isInRange(UINT idx) { return idx < d_maxidx; }
};

My first reaction to this code is that isInRange should have been
defined as a static function in the Interface.cc translation unit rather
than a member function:

static bool isInRange(UINT idx, UINT max) { return idx < max; }

Is my reaction correct? Any opinions?

There is no "correct" or "incorrect" reaction. It's your reaction,
nobody is going to tell you how to react.

As to the interface, my reaction to your reaction is "what the hell
is wrong with a non-static member?"

Victor

[David Rubin]

Victor Bazarov wrote:

class Interface {
public:
Interface(UINT maxidx);

void set(UINT idx, UCHAR data);
/* ... */
private:
UINT d_maxidx;

bool isInRange(UINT idx) { return idx < d_maxidx; }
};

My first reaction to this code is that isInRange should have been
defined as a static function in the Interface.cc translation unit rather
than a member function:

static bool isInRange(UINT idx, UINT max) { return idx < max; }

[snip] As to the interface, my reaction to your reaction is "what the hell
is wrong with a non-static member?"

It seems to pollute the interface. It's good to do error checking, but
why should this be an explicit part of the class definition? (Note my
self-reply that this class will only be used as a leaf.)

/david

--
Andre, a simple peasant, had only one thing on his mind as he crept
along the East wall: 'Andre, creep... Andre, creep... Andre, creep.'
-- unknown

[Andrey Tarasevich]

David Rubin wrote:

> class Interface {
> public:
> Interface(UINT maxidx);
>
> void set(UINT idx, UCHAR data);
> /* ... */
> private:
> UINT d_maxidx;
>
> bool isInRange(UINT idx) { return idx < d_maxidx; }
> };
>
> My first reaction to this code is that isInRange should have been
> defined as a static function in the Interface.cc translation unit rather
> than a member function:
>
> static bool isInRange(UINT idx, UINT max) { return idx < max; }

[snip]

As to the interface, my reaction to your reaction is "what the hell
is wrong with a non-static member?"

It seems to pollute the interface. It's good to do error checking, but
why should this be an explicit part of the class definition? (Note my
self-reply that this class will only be used as a leaf.)
...

Hm... I don't understand how are you planning to implement it as a
'static' function when it needs access to the 'd_maxidx' member of a
concrete object. Are you suggesting that 'd_maxidx' should also be
pulled out of 'Interface' and made a 'static' variable?

--
Best regards,
Andrey Tarasevich
Brainbench C and C++ Programming MVP

[jeffc]

"David Rubin" <bo***********@nomail.com> wrote in message
news:3F***************@nomail.com...

This is a question related to my previous post titled "private member
functions:"

I recently saw that a colleage of mine submitted code which defines a
class Interface as follows[1]:

class Interface {
public:
Interface(UINT maxidx);

void set(UINT idx, UCHAR data);
/* ... */
private:
UINT d_maxidx;

bool isInRange(UINT idx) { return idx < d_maxidx; }
};

My first reaction to this code is that isInRange should have been
defined as a static function in the Interface.cc translation unit rather
than a member function:

static bool isInRange(UINT idx, UINT max) { return idx < max; }

Is my reaction correct? Any opinions?

I think the point is you'd have to know what max is, and what if you don't?
In other words, there could be multiple instances of class Interface lying
around at any point in time. How are you going to keep track of the max
value for all of them? Isn't it better to let each instance keep track of
this number, without you having to do that work? That is, after all, one of
the basic principles of OO programming. There's nothing wrong with a static
function per se, although I have to question whether or not you really need
an Interface class to tell you if one number is bigger than another number.

[jeffc]

"David Rubin" <bo***********@nomail.com> wrote in message
news:3F***************@nomail.com...

It seems to pollute the interface. It's good to do error checking, but
why should this be an explicit part of the class definition?

Because the max number is an explicit part of the class definition, which is
exactly where it belongs. A class is data along with the functions that
operate on that data, encapsulated. If the data can't be static, then why
should be function be? I'm not saying there should never be static
functions, but they are usually best for when they don't need any data that
is specific to any one instance of the class.

[David Rubin]

Andrey Tarasevich wrote:

David Rubin wrote:

> class Interface {
> public:
> Interface(UINT maxidx);
>
> int set(UINT idx, UCHAR data); [change! ^^^] > /* ... */
> private:
> UINT d_maxidx;
>
> bool isInRange(UINT idx) { return idx < d_maxidx; }
> }; > My first reaction to this code is that isInRange should have been
> defined as a static function in the Interface.cc translation unit rather
> than a member function: > static bool isInRange(UINT idx, UINT max) { return idx < max; }

[snip] Hm... I don't understand how are you planning to implement it as a
'static' function when it needs access to the 'd_maxidx' member of a
concrete object. Are you suggesting that 'd_maxidx' should also be
pulled out of 'Interface' and made a 'static' variable?

I was imagining something like this:

/* Interface.cc */

static bool isInRange(UINT idx, UINT max) { return idx < max; }

int Interface::set(UINT idx, UCHAR data)
{
int r = ERROR;

if(isInRange(idx, d_maxidx)){
/* non-trivial code involving data */
r = OK;
}
return r;
}

isInRange does not appear in the Interface header file at all. This way,
a proliferation of validation functions will not affect the class
interface; they are merely implementation details. The fact that
Interface::set, for example, returns a (documented) error condition is
enough to suggest to implementors that some sort of validation must be
done. However, I'm conflicted about imposing the details on Interface
clients.

/david

--
Andre, a simple peasant, had only one thing on his mind as he crept
along the East wall: 'Andre, creep... Andre, creep... Andre, creep.'
-- unknown

[David Rubin]

jeffc wrote:

My first reaction to this code is that isInRange should have been
defined as a static function in the Interface.cc translation unit rather
than a member function:

[snip] I think the point is you'd have to know what max is, and what if you don't?
In other words, there could be multiple instances of class Interface lying
around at any point in time. How are you going to keep track of the max
value for all of them? Isn't it better to let each instance keep track of
this number, without you having to do that work? That is, after all, one of
the basic principles of OO programming. There's nothing wrong with a static
function per se, although I have to question whether or not you really need
an Interface class to tell you if one number is bigger than another number.

Most people seem to have misread my original comment, quoted above. I am
talking about a static function in a translation unit, not a class
function (declared as static). I think my response to Andrey Tarasevich
(elsethread) explains the point a little more clearly.

/david

--
Andre, a simple peasant, had only one thing on his mind as he crept
along the East wall: 'Andre, creep... Andre, creep... Andre, creep.'
-- unknown

[jeffc]

"David Rubin" <bo***********@nomail.com> wrote in message
news:3F***************@nomail.com...

jeffc wrote:

My first reaction to this code is that isInRange should have been
defined as a static function in the Interface.cc translation unit rather than a member function:
[snip]

I think the point is you'd have to know what max is, and what if you

don't? In other words, there could be multiple instances of class Interface lying around at any point in time. How are you going to keep track of the max
value for all of them? Isn't it better to let each instance keep track of this number, without you having to do that work? That is, after all, one of the basic principles of OO programming. There's nothing wrong with a static function per se, although I have to question whether or not you really need an Interface class to tell you if one number is bigger than another

number.
Most people seem to have misread my original comment, quoted above. I am
talking about a static function in a translation unit, not a class
function (declared as static). I think my response to Andrey Tarasevich
(elsethread) explains the point a little more clearly.

I didn't know that, but even so, my advice still stands, except for the
second half of the last sentence.

[lilburne]

David Rubin wrote:

Most people seem to have misread my original comment, quoted above. I am
talking about a static function in a translation unit, not a class
function (declared as static). I think my response to Andrey Tarasevich
(elsethread) explains the point a little more clearly.

I'd say that this was an implementation detail that ought to
be kept out of the class header.

I suspect that it is being used to check the 'idx' value
being passed to set(). Being private clients are unable to
use it to check that 'idx' is ok or not.

I suppose that UINT is some unsigned integer - do you really
want a 4Gb range, wont 2Gb suffice? In general 'unsigned' is
a nuisence because you can't check if the value is negative
or not, and the compiler will silently promote an int to
unsigned:

int main()
{
int i = -1;
unsigned ui = i;
return 0;
}

wont generate a warning on some compilers.

More importantly this isInRange method is an example of what
I was talking about in your post about private members
functions. It has utility outside of the class, particularly
if it takes an upper and lower value like:

bool isInRange(int value, int min_value, int max_value) {
return value >= min_value && value <= max_value;
}

you might want to only use greater than and less than though.

The point being is that often these 'private methods' have a
utility outside of the class they were originally written for.