Class design question.

Is this class evidence of poor design?

class Rectangle
{
double width, height;
public:
double get_width() const { return width; }
double get_height() const { return height; }

void set_width(double width) { this->width = width; }
void set_height(double height) { this->height = height; }
};

If so then why?

re: Class design question.

* Jason Heyes:[color=blue]
> Is this class evidence of poor design?
>
> class Rectangle
> {
> double width, height;
> public:
> double get_width() const { return width; }
> double get_height() const { return height; }
>
> void set_width(double width) { this->width = width; }
> void set_height(double height) { this->height = height; }
> };[/color]

Yes.

[color=blue]
> If so then why?[/color]

No user defined constructor => undefined state after construction.

And as it stands (it would be different with a constructor) no way
to assign both width and height at the same time.

Also there is evidence of poor coding, namely inconsistent use of
'this->'; even using it at all in C++ is generally poor style, but
using it inconsistently, just as a disambiguator, is worse.

--
A: Because it messes up the order in which people normally read text.
Q: Why is it such a bad thing?
A: Top-posting.
Q: What is the most annoying thing on usenet and in e-mail?

re: Class design question.

"Jason Heyes" <jasonheyes@optusnet.com.au> wrote in message
news:41a097e2$0$7560$afc38c87@news.optusnet.com.au ...[color=blue]
> Is this class evidence of poor design?
>
> class Rectangle
> {
> double width, height;
> public:
> double get_width() const { return width; }
> double get_height() const { return height; }
>
> void set_width(double width) { this->width = width; }
> void set_height(double height) { this->height = height; }
> };
>
> If so then why?[/color]

If there are no class invariants to enforce, and no reason
to expect having to add this capability in the future, I
would consider using a plain struct instead:
struct Rectangle {
//consider adding a constructor...
double width, height;
};


--
http://ivan.vecerina.com/contact/?subject=NG_POST <- email contact form

re: Class design question.

"Jason Heyes" writes:
[color=blue]
> Is this class evidence of poor design?
>
> class Rectangle
> {
> double width, height;
> public:
> double get_width() const { return width; }
> double get_height() const { return height; }
>
> void set_width(double width) { this->width = width; }
> void set_height(double height) { this->height = height; }
> };
>
> If so then why?[/color]

Yes, it's poor design. It is just an obfuscated way of doing what could be
done with a simple struct. There should be some reward for complexity,
there are no rewards here.

re: Class design question.

Jason Heyes wrote:
[color=blue]
> Is this class evidence of poor design?
>
> class Rectangle
> {
> double width, height;
> public:
> double get_width() const { return width; }
> double get_height() const { return height; }
>
> void set_width(double width) { this->width = width; }
> void set_height(double height) { this->height = height; }
> };
>
> If so then why?[/color]

Poor design in terms of what? Notation? Mechanics?

Some might argue that since you are giving read -and- write access to width
and height, you could as well make width and height public (like someone
else already mentioned).

However, I think in terms of OOP your approach is better, because you are
e.g. free to change the names of your class members at any time without
affecting the actual use your class for a potential client.

As to the style thing, I'd prefer notating data members with m_* rather than
using 'this' to express membership, but that's just a matter of taste I
guess.

re: Class design question.

osmium wrote:
[color=blue]
> Yes, it's poor design. It is just an obfuscated way of doing what could
> be
> done with a simple struct. There should be some reward for complexity,
> there are no rewards here.[/color]

If the class will never change, then you might be right.

Otherwise this is very short thought.
If for example some day you happen to want the class doing more tha just
setting the width or height, like e.g. logging the action, then in your
approach you would have to completely redesign your class. The client would
have to change ALL his calls where he is settings width and height.

That can't happen with his approach, thus, in my eyes, it's by far better
style than making it a struct (which I try to avoid in general in C++,
since it's more a backward compatibility to C thing instead of a useful C++
construct).

re: Class design question.

"Matthias Käppler" writes:
[color=blue]
> osmium wrote:
>[color=green]
>> Yes, it's poor design. It is just an obfuscated way of doing what could
>> be
>> done with a simple struct. There should be some reward for complexity,
>> there are no rewards here.[/color]
>
> If the class will never change, then you might be right.
>
> Otherwise this is very short thought.
> If for example some day you happen to want the class doing more tha just
> setting the width or height, like e.g. logging the action, then in your
> approach you would have to completely redesign your class. The client
> would
> have to change ALL his calls where he is settings width and height.
>
> That can't happen with his approach, thus, in my eyes, it's by far better
> style than making it a struct (which I try to avoid in general in C++,
> since it's more a backward compatibility to C thing instead of a useful
> C++
> construct).[/color]

I think the notion of writing for the future is mostly bullshit. The idea
that people can realistically predict the future is bogus. This is an
example of OOP gone wild. One of the first steps in design is to decide
what you want to design. An airplane that characteristically does a
controlled crash landing on an aircraft carrier should have a different
design than an
airplane that lands on a runway. A composite design penalizes both
applications.

There is entirely too much hokum involving OOP, it is not a silver bullet
and the sooner people realize that the better.

re: Class design question.

osmium wrote:
[color=blue]
> I think the notion of writing for the future is mostly bullshit. The idea
> that people can realistically predict the future is bogus.[/color]

Exactly, that's what I'm talking about. And that's why it's often a good
idea to design your class loose so you can extend it at any time without
changing its outer appearance.
[color=blue]
> This is an
> example of OOP gone wild. One of the first steps in design is to decide
> what you want to design.[/color]

If your designs hold 100% from day one to release, than you're an extremely
good software engineer :)
Most designs do not, that's why it's a good idea to keep some doors open.
[color=blue]
> There is entirely too much hokum involving OOP, it is not a silver bullet
> and the sooner people realize that the better.[/color]

Opinions may differ here I guess.

Cheers,
Matthias

re: Class design question.

"osmium" <r124c4u102@comcast.net> wrote in message
news:30brgsF2uj01dU1@uni-berlin.de...[color=blue]
> "Matthias Käppler" writes:
>[color=green]
> > osmium wrote:[/color]
> I think the notion of writing for the future is mostly bullshit. The idea
> that people can realistically predict the future is bogus. This is an
> example of OOP gone wild. One of the first steps in design is to decide
> what you want to design. An airplane that characteristically does a
> controlled crash landing on an aircraft carrier should have a different
> design than an
> airplane that lands on a runway. A composite design penalizes both
> applications.[/color]

But designing to allow for ease of modification is the point, not knowing
the future.

[color=blue]
> There is entirely too much hokum involving OOP, it is not a silver bullet
> and the sooner people realize that the better.[/color]

Amen to that, brother.
--
Gary

re: Class design question.

osmium wrote:
....[color=blue]
>
> I think the notion of writing for the future is mostly bullshit. The idea
> that people can realistically predict the future is bogus. This is an
> example of OOP gone wild. One of the first steps in design is to decide
> what you want to design. An airplane that characteristically does a
> controlled crash landing on an aircraft carrier should have a different
> design than an
> airplane that lands on a runway. A composite design penalizes both
> applications.
>
> There is entirely too much hokum involving OOP, it is not a silver bullet
> and the sooner people realize that the better.[/color]

There is a balance between overdoing it and underdoing it.

The way I express it is:
"Never artificially limit the utility of a class or function"

In other words, don't go out of your way to make somthing less generic.

As for the OP. I have no idea what the application of the class is so
it might be just perfect, it might not, I don't know.

If I needed a generic shape library, it's not even close.

re: Class design question.

"Matthias Käppler" <nospam@digitalraid.com> wrote in message
news:cnqbr3$e58$01$1@news.t-online.com...[color=blue]
> Jason Heyes wrote:
>[color=green]
>> Is this class evidence of poor design?
>>
>> class Rectangle
>> {
>> double width, height;
>> public:
>> double get_width() const { return width; }
>> double get_height() const { return height; }
>>
>> void set_width(double width) { this->width = width; }
>> void set_height(double height) { this->height = height; }
>> };
>>
>> If so then why?[/color]
>
> Poor design in terms of what? Notation? Mechanics?
>[/color]

Poor design in terms of lots of things maybe. In particular poor design in
terms of lacking purpose over a struct or std::pair.

re: Class design question.

"Jason Heyes" <jasonheyes@optusnet.com.au> wrote in message
news:41a097e2$0$7560$afc38c87@news.optusnet.com.au ...[color=blue]
> Is this class evidence of poor design?
>
> class Rectangle
> {
> double width, height;
> public:
> double get_width() const { return width; }
> double get_height() const { return height; }
>
> void set_width(double width) { this->width = width; }
> void set_height(double height) { this->height = height; }
> };
>
> If so then why?[/color]

Well, when you give access to all internal variables like that, it's not really
data hiding any more. However, the abstraction does allow you to change the way
you store height and width at a later time (assuming it's something that can be
converted from the doubles in the functions.) Also, using "this" isn't very
cool.

re: Class design question.

"osmium" <r124c4u102@comcast.net> wrote in message
news:30brgsF2uj01dU1@uni-berlin.de...[color=blue]
>
> I think the notion of writing for the future is mostly bullshit. The idea
> that people can realistically predict the future is bogus. This is an
> example of OOP gone wild.[/color]

That's completely the wrong approach. The reason you write "for the future" is
specifically because you can NOT predict the future, not because you can.
Separation of implementation and interface can have big benefits when things
change in the future.

re: Class design question.

"Jason Heyes" <jasonheyes@optusnet.com.au> wrote in message news:<41a097e2$0$7560$afc38c87@news.optusnet.com.a u>...[color=blue]
> class Rectangle
> {
> double width, height;
> public:
> double get_width() const { return width; }
> double get_height() const { return height; }
>
> void set_width(double width) { this->width = width; }
> void set_height(double height) { this->height = height; }
> };[/color]

Here's what I would have done:

#include "Helper.h"

class Rectangle {
ATTR_BBB(width, double);
ATTR_BBB(height,double);
};

I could then work with a rectangle as follows:

Rectangle r;
r.width(5.0); // sets the width to 5
cout << r.width(); // prints the width
cout << r._width; // tries to access the (protected) width member
directly.

Inside of my Helper.h file, I have definitions for automatically
declaring a protected member, along with accessor and modifier
functions. ATTR means I'm declaring an attribute, and the three Bs
are codes that determine whether the member, accessor, and modifier
functions work with base types, pointers, or references (B is for base
type, P for pointer, R for reference).

Here is the definition of ATTR_BBB:

#define ATTR_BBB(n,t) \
public: typedef t T_##n; \
t n() const { return _##n; } \
void n(t v) {_##n = v; } \
protected: t _##n

And also, FYI, here is the definition for ATTR_PRR, which has a
pointer to a type for its protected member, and the accessors and
modifiers deal with references:

#define ATTR_PRR(n,t) \
public: typedef t T_##n; \
t & n() { return * _##n; } \
const t & n() const { return * _##n; } \
void n(t & v) { _##n = & v; } \
protected: t * _##n

Cryptic, huh? Gotta love C. (## is the token pasting pre-processor
operator. _##n translates to "_width", or whatever)

Anyway, don't listen to those who tell you to just declare it as a
struct. I once made a coordinate class with public x and y members,
and, you guessed it, far into the project I came across a case that
required special handling for accessing the x and y values. If I had
been using accessors / modifiers, it would've been a trivial
modification, but instead it required a whole day of going through the
code. With inlined accessors and modifiers, there is zero performance
penalty associated with using them, and if you have a set of
accessor/modifier maker macros (like ATTR_BBB above), then you can
create them with a single line as well, making it just as terse and
readable.

Seems pretty compelling to me. Use accessor / modifier methods.

OH UNLESS UR SUPER LEET AND NEVER MAKE MISTAKES then you can do
whatever you want do it the oldsk00l C way lolol

re: Class design question.

>Well, when you give access to all internal variables like that, it's not[color=blue]
>really data hiding any more.[/color]

No, but as you point out it does prevent clients from developing dependencies
on the implementation which might have benefits down the road.

I would not even consider this a data hiding issue. The data is part of the
abstraction and is part of the visible state. A rectangle class is not too
useful if you cannot get its dimensions. It seems more of a "implementation
hiding" issue.

When I think of data hiding I think of preventing clients from being aware of
data members that could be of no interest to them.

For example, if I were implementing a reference counted string I would provide
a method to access the strings contents but I would not have a member that
allowed the current reference count to be read. Users of the string might
choose to use a reference counted string for efficiency purposes but knowing
the reference count is probably of no value so I would keep it hidden.

Just my one cent

re: Class design question.

"DaKoadMunky" <dakoadmunky@aol.com> wrote in message
news:20041122184754.11389.00001184@mb-m29.aol.com...[color=blue][color=green]
> >Well, when you give access to all internal variables like that, it's not
> >really data hiding any more.[/color]
>
> No, but as you point out it does prevent clients from developing dependencies
> on the implementation which might have benefits down the road.
>
> I would not even consider this a data hiding issue. The data is part of the
> abstraction and is part of the visible state. A rectangle class is not too
> useful if you cannot get its dimensions. It seems more of a "implementation
> hiding" issue.[/color]

Well, sometimes "data" and "implementation" are not so black and white. Let's
say the "data" is requested in the form of a double, but stored in the form of a
long with a separate variable indicating how many decimal places exist. Is that
data, or implementation? Where do you draw the line between returning data and
returning operations based on that data? In this case it just so happens that
the information requested is the same as the information stored, but it doesn't
have to be that way for the same principles to apply. For example, let's say we
ask the rectangle its area. Is that a piece of "data" we have to store in the
object, or can it be calculated on the fly? Is there any way the client can
tell the difference? The "implementation" of whether this is stored data or
calculated on the fly can change continually, as long as there's an interface,
but to the client, it just looks like "data". If we calculated it on the fly,
would you say the data exists already, but just in a different form? Would you
be OK with not putting the area function in the interface since the client can
easily calculate it itself based on the other data available (i.e. the client
already has all the information needed about the rectangle)? Sometimes there's
a direct connection, and sometimes there isn't. But if you're saying that if an
object has data that clients need to know about it's still often a good idea to
provide an interface, I agree.

re: Class design question.

"Grey Plastic" <greyplastic@hotmail.com> wrote in message
news:1de04d38.0411220927.c9f38b0@posting.google.co m...[color=blue]
>
> Anyway, don't listen to those who tell you to just declare it as a
> struct.[/color]

Just to be clear, a struct and a class in C++ are the same thing. Which to use?
Some use the convention that Plain Old Data should be implemented as a struct,
and otherwise it should be a class. But it makes absolutely no difference
whatsoever, technically.

re: Class design question.

greyplastic@hotmail.com (Grey Plastic) wrote in message news:<1de04d38.0411220927.c9f38b0@posting.google.c om>...
[snip][color=blue]
> And also, FYI, here is the definition for ATTR_PRR, which has a
> pointer to a type for its protected member, and the accessors and
> modifiers deal with references:
>
> #define ATTR_PRR(n,t) \
> public: typedef t T_##n; \
> t & n() { return * _##n; } \
> const t & n() const { return * _##n; } \
> void n(t & v) { _##n = & v; } \
> protected: t * _##n
>
> Cryptic, huh? Gotta love C. (## is the token pasting pre-processor
> operator. _##n translates to "_width", or whatever)[/color]
[snap]

I hope you show this to every potential employer. Because they
can all use a good laugh as they stamp your resume "Rejected!"
Socks

re: Class design question.

puppet_sock@hotmail.com wrote:
[color=blue]
> greyplastic@hotmail.com (Grey Plastic) wrote in message
> news:<1de04d38.0411220927.c9f38b0@posting.google.c om>...
> [snip][color=green]
>> And also, FYI, here is the definition for ATTR_PRR, which has a
>> pointer to a type for its protected member, and the accessors and
>> modifiers deal with references:
>>
>> #define ATTR_PRR(n,t) \
>> public: typedef t T_##n; \
>> t & n() { return * _##n; } \
>> const t & n() const { return * _##n; } \
>> void n(t & v) { _##n = & v; } \
>> protected: t * _##n
>>
>> Cryptic, huh? Gotta love C. (## is the token pasting pre-processor
>> operator. _##n translates to "_width", or whatever)[/color]
> [snap]
>
> I hope you show this to every potential employer. Because they
> can all use a good laugh as they stamp your resume "Rejected!"
> Socks[/color]

Agreed. This sort of preprocessor wizardry is not appropriate in C++.