My ever-evolving coding style

This topic is 1835 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

Recommended Posts

I am a self taught hobbyist programmer. Today, my subconcious decided to be helpful and torture me with randomly fetched memories of my old coding style. Here's a sample of my coding style from when I was still in the C-with-classes phase, circa 2006:
class CFoobar
{
private:
// Public member variables are bad, mmkay?
int m_iFoo;
RGB m_rgbBar;
double m_dBaz;

public:
// Encapsulation is good, mmkay?
// I'm so clever, using a pointer to avoid both a get AND a set for each member variable!
int *GetFoo() { return &m_iFoo; }
RGB *GetBar() { return &m_rgbBar; }
// (Repeat for every member of CFoobar)
};

void frobnicate()
{
CFoobar *foo = new CFoobar();

// Wow! This is much easer than writing foo->setFoo(foo->getFoo()+7);
*foo->GetFoo() = *foo->GetFoo()+7;

double thing = *foo->GetBaz();
// etc...

delete foo;
}

Yup. Complete with the Microsoft bastardization of the Hungarian notation and the ever-so-important C-prefix on the class names so you always when you're using a class and when you're using a struct.

Guuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuu.

Share on other sites
void frobnicate()
{
CFoobar *foo = new CFoobar();

//Fixed
int* var = foo->GetFoo();
*var += 7;

double thing = *foo->GetBaz();
// etc...

delete foo;
}


Honestly I like returning pointers for most cases. Using the pointer you dont have to make a thousand function calls. Especially in parallel programming this is very useful when you want to avoid static variables. Sometimes its not a good idea, or not even possible depending on the use, but I believe this is a perfectly valid programming habit. IMO

Edited by Slig Commando

Share on other sites

Honestly I like returning pointers for most cases. Using the pointer you dont have to make a thousand function calls. Especially in parallel programming this is very useful when you want to avoid static variables. Sometimes its not a good idea, or not even possible depending on the use, but I believe this is a perfectly valid programming habit. IMO

You do realize that that method destroys your ability to enforce class invariants? And that it has no advantage over simply making your members public (and has the disadvantage of being clunky and a really non-obvious and non-idiomatic way of modifying an object)?And that this doesn't do anything to help avoid static variables (how is this even related to static variables?)?

There might be a few exceptional cases where it's a decent idea, but it should be just that, exceptional cases. If it's your preferred/common method... something is wrong.

Edited by Cornstalks

Share on other sites

Honestly I like returning pointers for most cases

I think the point is that his 'getters' are useless, because his code is equivalent to as if the variables were just public...

It's just a really bloated way of writing this:

struct Foobar
{
int foo;
RGB bar;
double baz;
};
void frobnicate()
{
Foobar foo;
foo->foo = foo->foo+7;
double thing = foo->baz;
}

Share on other sites

Whenever writing a million getters and setters gets annoying, I usually ask myself two things:

a) if I just go and write trivial set/get methods for everything, why not just stop pretending and make it public?

b) why not be lazy and wrap it in a template that handles it?

class ...

{

...

Property<int> stuff;

};

int value = obj.stuff();

obj.stuff(5);

obj.stuff( obj.stuff() + 5 );

Not calling it get and set allows to use the template for all trivial cases and do custom implementations where needed without requiring a different syntax.

Still, if a class has a dozen members and trivial accessors for all of them, encapsulation probably just went out the window. So it's more of a convenient way to remain consistent.

Share on other sites

Yes, if your class has a million getters and setters it's probably doing far too much for its own good. I like the template idea, it reduces boilerplate code while preserving encapsulation. On the other hand, making the member public is not a good idea unless it's a trivial member or a trivial class, because as soon as you need to enforce a condition on this member your class interface has to change, and all code depending on the class breaks (sure, the fix is a find/replace away, but then you need to recompile etc..)

I kind of liked the Object Pascal/Delphi way for properties (sorry if there are any syntax errors, it's been months since I wrote any):

interface

...

private
fFoo: Integer;
fBar: String;

public
property Foo: Integer read fFoo; // read-only, default getter (no boilerplate getter method)

...

implementation

function getBar: String;
begin
if (fBar <> "sentinel") then Result := fBar;
end;

procedure setBar(Value: String);
begin
if (Value <> "sentinel") then fBar := Value;
end;


Which I think is advantageous in that you immediately see from the interface what is read-only and what is read-write (or write-only, should you have a use case for that) and you can easily switch from a default getter (or setter) without even changing the class interface. As far as client code goes, it just thinks Foo and Bar are properties as if they were ordinary members (accessed by class.Foo) and any reads go through the getter, any writes go through the setter. It also works with indexed properties. I think it's rather neat, I'm sure it has its downsides though.

Of course, if you are absolutely certain that any value of the member is valid and you don't need to do anything when it changes, and that you will never reconsider this design decision, just make it public. This is rather rare, though, usually this kind of member is set inside a constructor or other initialization method via some argument.

Share on other sites

Today, my subconcious decided to be helpful and torture me with randomly fetched memories of my old coding style

I wonder what you're trying to tell yourself by self-inflicting this torture. Thanks god those things are gone!

Share on other sites

Oh god, when I decided to check my old Battle City code (from about 9 months ago) and found stuff like this:

namespace BattleTanks
{
class Tank
{
#region Variables
public void LoadContent(ContentManager content, string TankAssetName, string BulletAssetName) { ... }
public void Draw(SpriteBatch spriteBatch) { ... }
public void Initialize(int X, int Y, float rotation, float scaling, int speed, bool playerbullet) { ... }
public void MoveTank(bool HorAxis, int direction, bool cancel) { ... }
public void RemoveTank(TimeSpan time) { ... }
public void MoveBullet() { ... }
public void LaunchBullet() { ... }
public string BulletDirection() { ... }
public void RemoveBullet() { ... }
public void RotateAI(out bool HorAxis, out int direction, out string text) { ... }
public void FireAI() { ... }
public void TurnAI(string direction) { ... }
}
}


And I was happy at that time, especially because it worked. Try to guess what is hidden inside some of the methods.

2 months later I open the same code out of curiosity... Reaction: "What in the mother of f- was I doing back then >.<"

And for some reason I'm pretty sure that I'll say the same for my current code after 1 year.

Share on other sites

Try to guess what is hidden inside some of the methods.

• LoadContent: retrieves the assets used by this tank and stores them in the member variables.
• Draw: adds the tank sprite to the sprite batch so it gets drawn later.
• Initialize: sets some member variables that affect the behavior of the tank.
• MoveTank: makes the tank move in the specific direction, applying physics as needed.
• RemoveTank: called when a tank is exploding? No idea what the time is for.
• MoveBullet: as MoveTank but for the bullets of each tank. I guess they're tied to tanks to ensure they can shoot only one bullet each (doesn't seem very smart, a tank could blow up before the bullet vanishes).
• LaunchBullet: makes the tank shoot a bullet.
• BulletDirection: retrieve bullet's direction? Why is this a string?! o_O
• RemoveBullet: gets rid of the bullet when it hits something.
• RotateAI: makes the AI try to rotate in a specific direction. Again, what is the string for?!
• FireAI: makes the AI try to fire a bullet.
• TurnAI: Like RotateAI but giving the angle directly. ...also I take that, the angles are given as strings -_-'

How much of that was wrong?

Share on other sites

BulletDirection: retrieve bullet's direction? Why is this a string?! o_O

Hey, it's just flexible design. That way the function can return, in addition to actual directions, "over there", "this way", "behind you" or even "nowhere". You can never have too many strings.

• 38
• 12
• 10
• 10
• 9
• Forum Statistics

• Total Topics
631362
• Total Posts
2999577
×