Is this good OOP?

For Beginners

Started by sheep19 January 05, 2008 02:40 PM

69 comments, last by sheep19 16 years, 3 months ago

2,123

January 05, 2008 04:20 PM

Quote:Original post by Lotus
I do not think the prefix m_ is that bad though, it does help avoid having to rename setter method parameter names to avoid conflicts, as well as make the distinction between temporary and class-level variable.

If you really believe that, your functions are too long.

Quote:As a general advice, if you're really interested in OO design then try to get your hands on the GOF (if you haven't already done so).

GoF is a patterns book, not an OO design book.

Quote:Original post by mwnoname
I agree with MadsGustaf and Lotus that the m_VarName or mVarName are useful and do contain valuable information as to whether the variable is a local or a member variable without having to look back at the class. Many company style guides I've seen will require this in code.

Many company style guides are crap.

Quote:I disagree that Get and Set functions are not valuable, they make it easier to refactor code at a later date.

Get* and Set* are semantic code smell; prefer action methods that localize the behavior within the class. There is virtually no reason to allow an external piece of code directly set a private member variable.

Oluseyi

2,123

January 05, 2008 04:22 PM

Quote:Original post by rothzael
Quote:Original post by MadsGustaf
The code isn't tested or anything
you may want to make a totally virtual Creature class.

Why not make a pure virtual object class, as you may find that game items will share similar functions with monsters/players.

YAGNI.

Too much up-front "design" and architecture, to support a bunch of flexibility you have no evidence you actually need, is bad.

Lotus

341

January 05, 2008 05:06 PM

Quote:Original post by Oluseyi
Quote:As a general advice, if you're really interested in OO design then try to get your hands on the GOF (if you haven't already done so).

GoF is a patterns book, not an OO design book.

It's definitely a patterns book! I believe that studying elegant solutions to specific problems in object-oriented software design is great way to improve one's object oriented design insight. Of course this is just "part" of the whole (there's much more to OO design), but it is an important one.

Check out War to the Core the world domination space MOBA-RTS hybrid.
Join us on Discord.
Into sci-fi novels? Then check out Spectral Legends.

mwnoname

128

January 05, 2008 07:23 PM

Original post by Oluseyi
Many company style guides are crap.

That may be true, and you may disagree with them, but if you don't follow them, you won't be working very long...

Quote:Original post by Oluseyi
Get* and Set* are semantic code smell; prefer action methods that localize the behavior within the class. There is virtually no reason to allow an external piece of code directly set a private member variable.

I agree, you want to localize the behavior wherever possible, but there are many cases where you don't want to create cross dependencies between systems. For example, you probably don't want your player class rendering your health status bar...

Silicon Munky

162

January 05, 2008 10:18 PM

Quote:Original post by Oluseyi
Quote:Original post by Lotus
I do not think the prefix m_ is that bad though, it does help avoid having to rename setter method parameter names to avoid conflicts, as well as make the distinction between temporary and class-level variable.

If you really believe that, your functions are too long.

I disagree, it has little to do with the length of the function. When I look at a variable, I want to immediately know if it is a local or if it is a member. I shouldn't need to look a few lines up to see if it's a parameter or a local variable. I believe it is much easier to read code using that notation.

Hungarian notation was created for a reason. While all of it's useful features are no longer needed because of advances with development environments, certain parts of it are still worth adhering to.

Some development enviroments are even easier to use when adhering to the mVariableName convention. What I mean is that you can type "m" and then VisualAssist or similar programs will give you the list of member variables to choose from.

ApochPiQ

23,138

January 05, 2008 10:32 PM

Hungarian wartation is a completely subjective thing. Personally, I agree that it's utterly hideous and pretty much useless if you have an IDE from this millenium, but hey - I'm not the one who has to stare at your code all day [wink]

For the OP - you have made one serious mistake, and that is adding functions before you actually have functionality. Specifically, I'm thinking of Attack() and RunAway() here. You've added them to the classes, but clearly haven't really thought about what they should do.

For example, when somebody calls Attack, how do you know what to attack? What rules and processes govern the combat? Does the player (or enemy character) just attack once, or keep attacking until the target dies? etc.

Or, for RunAway, what exactly do I run away from? Where do I run to?

Until you have at least some basic answers to those questions, you haven't thought out the functionality enough to be putting code behind it.

Now, at this point, I'm sure someone is going to pop in and try to whine about the "agile" fad and how you should continually make small changes to the code until it evolves into what it should be. The problem with this approach is that it often becomes tempting to put in little stubs, like you have done, since obviously you'll need something like that later on. However, since you don't have much OO design experience, you won't yet have the instincts to recognize when it is a proper time to be putting in those stubs, and what they should look like.

The net result of doing that kind of "tweak until it works" hackery - at least for newcomers to software design - is that you make more work for yourself, because you have to go back several times and make sweeping changes to handle things you didn't think about. A little bit of foresight goes a long way.

For a small text RPG, is this going to be a disasterous problem? No, probably not; but it's worth developing good habits early, and if you're interested in learning good OO design (as apparently you are), it's certainly a lesson you want to learn the easy way [smile]

Wielder of the Sacred Wands
[Work - ArenaNet] [Epoch Language] [Scribblings]

Oluseyi

2,123

January 05, 2008 11:07 PM

Quote:Original post by mwnoname
Quote:Original post by Oluseyi
Many company style guides are crap.

That may be true, and you may disagree with them, but if you don't follow them, you won't be working very long...

Which is relevant if you're working for a company with a style guide. This is a kid asking about good OO design in abstract; appealing to popularity - and citing hypothetical corporate code style guides - for a guy who's trying to learn good practice on his own is bogus.

Quote:I agree, you want to localize the behavior wherever possible, but there are many cases where you don't want to create cross dependencies between systems. For example, you probably don't want your player class rendering your health status bar...

That's a retrieval, which is fine. You don't want your renderer setting your player object's health either...

Quote:Original post by Silicon Munky
Hungarian notation was created for a reason. While all of it's useful features are no longer needed because of advances with development environments, certain parts of it are still worth adhering to.

Hungarian Notation was created for use in assembly language, where your register variables have no type information whatsoever. You're not using assembly language, but, in this case, C++, which at the very least has variable type information, including visibility and class membership. In truth, the m_ prefix is probably the least objectionable of common identifier warts, so whatever.

Quote:Some development enviroments are even easier to use when adhering to the mVariableName convention. What I mean is that you can type "m" and then VisualAssist or similar programs will give you the list of member variables to choose from.

You could type an object name, a period and then see the list. Or you could trigger the code completion by typing ctrl-space (in Visual Studio). Or the same environments you're talking about would still proffer completions after any character, not just m, right?

Yeah, that's a non-starter.

Gage64

1,235

January 06, 2008 01:04 AM

Quote:
Original post by Oluseyi
Quote:I agree, you want to localize the behavior wherever possible, but there are many cases where you don't want to create cross dependencies between systems. For example, you probably don't want your player class rendering your health status bar...

That's a retrieval, which is fine. You don't want your renderer setting your player object's health either...

Could you explain this a bit more please? I'm also one of those people who have a tendency to (over)use get/set methods, and I'd like to better understand when they are appropriate and when they are not.

OOP Articles | C++: A Dialog (free book) | Thinking in C++ (free book) | Google Books - Free preview for many books

Oluseyi

2,123

January 06, 2008 01:16 AM

Quote:Original post by Gage64
Could you explain this a bit more please? I'm also one of those people who have a tendency to (over)use get/set methods, and I'd like to better understand when they are appropriate and when they are not.

It's actually fairly simple. Properties (or Get/Set methods) are intended to act as a controlled interface to internal data of a class or object. Retrieving the data (Get) generally isn't a problem, unless Get is returning a complex object that is reference bound to your instance (say something that manages an external resource like memory). To illustrate, having a getHealth() member in a CombatUnit class is unlikely to cause any problems.

On the other hand, you want to control not only what happens when a property is modified, but which pieces of code can attempt to modify the property (as much as possible), and in what ways. It's one thing to add validation to setHealth(); it's a more important thing to ask why we're allowing any code that holds a reference to a CombatUnit instance to attempt to directly set its health in the first place. In all likelihood, we'd much rather have a function takeDamage(float impactForce, float distance), for example, that computes how much the health of the CombatUnit should drop - if at all - due to a given force incident at a given distance.

This allows us to fully encapsulate the behavior of the CombatUnit within itself (any other way and you have its response to attack actually being implemented outside itself). This also allows us to upgrade how CombatUnits respond to incident force in a single place, rather than looking for all the potential bits of code that might have called setHealth().

Hope that helps.

Gage64

1,235

January 06, 2008 02:57 AM

Quote:
Original post by Oluseyi
Hope that helps.

It certainly does! Thank you for the detailed explanation.

BTW, can you recommend a resource (book\article\...) that talks more about this sort of thing? Something beyond an introductory book on an OOP language but not as complex as a large book on OOD (I don't think I'm ready for those yet)?

OOP Articles | C++: A Dialog (free book) | Thinking in C++ (free book) | Google Books - Free preview for many books

Is this good OOP?

This topic is closed to new replies.

Popular Topics

Recommended Tutorials

Is this good OOP?

This topic is closed to new replies.

Popular Topics

Recommended Tutorials

Reticulating splines