Followers 0

Cleaning and 'improving' your code (const)

19 posts in this topic

Hi all,

I'm going through an exercise to clean up my code and put some more logic in it, regarding passing references and pointers.

Here's my goal:

** applies to all global functions and member functions of my classes

** if a function parameter is a class which is only input (not to be changed), use a const reference (const &...)

** if a function parameter needs to be altered by the function, use a pointer to the object (*....)

This also gives good readability if a function is changing an object, noticed by "->...".

** when it regards passing 'simple' objects like a vector/vertex, matrix, int, float etc., that is only input (not to be changed), I use const only (no reference). For example:

float DistCoordToCoord(const D3DXVERTEX3 pVector1, const D3DXVECTOR3 pVector2); or

** when it regards passing 'simple' objects which need to be altered, I use a pointer. For example:

void DistCoordToCoord(const D3DXVERTEX3 pVector1, const D3DXVECTOR3 pVector2, float *pResult);

The aim is to get better readability and less fault risks, by applying these style/ correctness.

My question:

- what would you do, looking at this? other approaches/ better approaches?

- am I overseeing things?

First thing I'm gonna do now is create a nice XLS with all classes and functions where this applies.

So any input is appreciated and can be used, since I'll need some time first to get the overview :)

0

Share on other sites

Whats the point for argument to be const if it is not reference or pointer?

0

Share on other sites

Stops you doing this

void CountDownFrom(int i)
{
while(i > 0)
{
cout << i-- << endl;
}
}


forcing you to copy the argument. Whether that is a good idea or not, I'll leave you to decide.

EDIT: OP should spend more time making member functions const-correct rather than concentrating solely on function arguments IMHO.

1

Share on other sites

Thanks.

Can you explain a bit more what you mean with making member functions const-correct?

I think it begins with the passed objects to the functions, which should or shouldn't be modified (and prevent copying variables when not needed).

Unless of course, the basics behind are not OK (structure of the classes, what's dependent on what, which member functions should be smaller/ split up etc.).

Not sure if this is the case 100%, but anyway this first step feels like good practice. Next could be to see if I can make it more efficient/ with smaller/ other member functions.

Or do you mean something else?

0

Share on other sites

If you havent already you should also make the methods of objects themselves const:

T object::method(params) const

if they have no side effects on the object (assuming you havent already)

EDIT:

In a few circumstances, instead of doing multiple return values using pointer/reference parameters, i use an std::pair (std::tuple for more than 2 members, havent used it).

For example if i need to query some data on some object at coordinates (x,y), i can return something like std::pair(bool exists, objectData data) so i can see if the object exists.

For larger objects the objectData would go in a reference or pointer parameter though to avoid copying (assuming the compiler doesnt optimize it out?)

Edited by Waterlimon
0

Share on other sites

Thanks, I understand and read into it.

What I'll also take into account in this 'action', is check which member functions actually change the object or just use vars of the object.

For example most member functions within my 'd3d' class, are const functions (not yet in the code, but I'm gonna change that).

Just on the details, how about a LPDIRECT3DDEVICE9, which is already a pointer. I would say pass it as const reference since it's only used and not changed. Not sure though if the device pointer needs 'mutable' rights to execute functions within in. I hope not, so the code's clean on that side too.

Ps.; in my opinion "const ...." is better readable for me then "..... const", so I'm going for that style, both in the class member functions and the function parameters.

Edited by cozzie
0

Share on other sites

const has to go after the function parameters for a const member function though, so they are not interchangeable.

const int T::foo()

function returns a const int

int T::foo() const

function does not modify the T object which calls foo.

0

Share on other sites

Thanks, I didn't know that.

So to be sure, this would be correct:

class MyClass

{
public:
int mAge;

void DoSometingWithNoneMembers(const float p1, float *p2) const;
void UpdateAge(const int &pAge);
}

void MyClass::DoSometingWithNoneMembers(const float p1, float *p2)

{
p2 = p1;
}

void MyClass:UpdateAge(const int &pAge)
{
mAge = *pAge;
}


Where I assume that the 'const' keyword for member functions is only needed in the prototype.

Is this correct, or does is also have to be in the implementation? (sounds logical for readability if it's also there)

Edited by cozzie
0

Share on other sites

It needs to be in the implementation too.

May I give a small piece of advice, based on 15+ years writing code? What you're doing, making things const correct, some will argue it is a waste and even const itself is bad, others will argue it is good and makes it safer. That is a matter of opinion, both sides having reasonable arguments, but if you feel it makes it better and safer for the way you program, then hey it's probably a good idea.

However, this can easily become a thing where you start renaming all your variables/functions/macros/etc EVERYWHERE to conform to some standard way of naming things.

At first, this seems good, so things are consistent, and also once you have decided to do so it is easy to get OCD and require it everywhere. However, the problem is that if this happens to you, down the road, you will inevitably change, again, how you feel things should be named, for clarity. Then what, redo all your source code again? But now there are 100s of files instead of 10s. And so on.

One of the best things I learned on this topic was to simply work on the file that needs worked on, and keep it consistent with how it already is. If a file uses a naming convention where all variables are camelCase, stay with it so the file is consistent. If another file uses CamelCase, because you changed your mind one day on what looks better, use it... on that file, but not on the first one. Otherwise you will drive yourself crazy as your style and preferences change. Consistency doesn't have to be project wide when it comes to arbitrary choices like this (I'm not really talking about safety or smart design here, just style preferences), in fact it will be impossible once you start introducing third party libraries with their own styles. But, it should be consistent within a same source file. Now you won't go crazy.

I realize that is not what you are addressing here... but from experience I can say how easy it is to become that, so you might as well get the advice now.

 That is what I am talking about, what is mentioned in the next post. Naming conventions are a good example. Keep it consistent within a file. And yes, if you prefer warts on your names, probably rAge for a reference versus pAge for a pointer, or you know, whatever you like.

Edited by achild
1

Share on other sites

You mean *p2 = p1 in example #2. They aren't members of the class so that is OK. EDIT: Why is it a member of the class though? It doesn't use the class at all. If you want it to be a member of the class make it static.

pAge is a reference not a pointer in example #3. So *pAge doesn't make any sense. You mean mAge = pAge. It can't be const though, correct, since it modifies mAge which is a member of the class.

You need the const in the definition too since you can provide a const and a non-const implementation of the same function (the const one will be called if called with a const variable).

I'd also rethink your naming conventions if you use p prefix for all function arguments. I'd use a p prefix for pointers. If you need a naming convention for parameters I'd use "a" for argument.

0

Share on other sites

Thanks. I understand what you mean with 'blind staring' on style (and possible OCD :))

During time I've developed my own style which feels good. My whole project (3d engine) uses this style, but never takes into account if parameters passed to a function can be modified or not, that's why I'm approaching this 'wider' then just one source file.

For example:

- I have a d3d class, which is the main class where the rendering happens

- for example the 'RenderFrame' member function, takes a d3dscene and other objects as parameters, but they're never changed

(and therefore would be safer to make them 'const'). At the moment I pass basically everything as a pointer, meaning I could accidentally change everything and can't see in a (member) function what the passed parameters/ variables are used for

- the same 'RenderFrame' function uses variables of the d3d object, but never changes them, so the member function 'RenderFrame' should be 'const' to (for safety)

etc. :)

One of the things I've put through over my complete project is variable namings.

mMyThing, means it's a class member, starting with "m", pMyOther, means it's a parameter given to the scope/ function, because it starts with 'p'.

It was quite a job to go through all code and apply it, but it definately helped and helps me when I change things now.

The same is with documentation/ comments in my code, also one style all over the engine.

Basically it's to late to prevent OCD, as you can read :)

0

Share on other sites

But if in your examples you confuse the syntax for pointers and references I feel you need to be OCD in other areas. I personally like the p prefix for pointers since the syntax is different ("->" vs. ".").

0

Share on other sites
I wont judge the style that works for you :)
After my changes I'll be able to recognize that variables that can be altered use '->' and other passed objects/parameters use the '.'.
Members and function params I distinct with the m and p prefix.
0

Share on other sites

Thanks, I didn't know that.

So to be sure, this would be correct:

class MyClass

{
public:
int mAge;

void DoSometingWithNoneMembers(const float p1, float *p2) const;
void UpdateAge(const int &pAge);
}

void MyClass::DoSometingWithNoneMembers(const float p1, float *p2)

{
p2 = p1;
}

void MyClass:UpdateAge(const int &pAge)
{
mAge = *pAge;
}


Where I assume that the 'const' keyword for member functions is only needed in the prototype.

Is this correct, or does is also have to be in the implementation? (sounds logical for readability if it's also there)

Sorta. UpdateAge is properly non const. But DoSometingWithNoneMembers shouldn't be a method at all, since it doesn't do anything with the object. A more typical example of a const method would be something that returns a value based on the members. For example:

int MyClass::getDogYears() const
{
return mAge*7;
}
Edited by King Mir
1

Share on other sites

Yep, I understand.

An example in my case:

bool CD3d::CheckDevice() const
{
if(SUCCEEDED(mD3ddev->TestCooperativeLevel())) return true;
return false;
}

0

Share on other sites

Thanks.

Can you explain a bit more what you mean with making member functions const-correct?

I think it begins with the passed objects to the functions, which should or shouldn't be modified (and prevent copying variables when not needed).

Unless of course, the basics behind are not OK (structure of the classes, what's dependent on what, which member functions should be smaller/ split up etc.).

Not sure if this is the case 100%, but anyway this first step feels like good practice. Next could be to see if I can make it more efficient/ with smaller/ other member functions.

Or do you mean something else?

This should help:

http://en.wikipedia.org/wiki/Const-correctness

Edited by Matt-D
0

Share on other sites

** when it regards passing 'simple' objects like a vector/vertex, matrix, int, float etc., that is only input (not to be changed), I use const only (no reference). For example:
float DistCoordToCoord(const D3DXVERTEX3 pVector1, const D3DXVECTOR3 pVector2); or

** when it regards passing 'simple' objects which need to be altered, I use a pointer. For example:
void DistCoordToCoord(const D3DXVERTEX3 pVector1, const D3DXVECTOR3 pVector2, float *pResult);

This is a code smell to me. If pResult is an output of the function then it should be returned.

float DistCoordToCoord(const D3DXVERTEX3 pVector1, const D3DXVECTOR3 pVector2);

If you need the initial value for some reason, pass that in.

float DistCoordToCoord(const D3DXVERTEX3 pVector1, const D3DXVECTOR3 pVector2, float initialValue);

Then do the assignment in the call

float someValue = 10f;
someValue = DistCoordToCoord(vector1, vector2, someValue);

Side note: If you're going to prefix p to pointers, make sure you're consistent about it. pVector1 and pVector2 are poorly named.
Even more side note: Why not make them references here? Sure, you're only saving a few bytes, but it costs you nothing in terms of readability and code maintenance.

0

Share on other sites
Thanks.
You're absolutely right on referencing also d3dxvector3 objects etc.

On the naming, pVector, pPos, pRange etc. Works for me, because I name all function parameters p... and all my class members are named m....
0

Share on other sites
float DistCoordToCoord(const D3DXVERTEX3 pVector1, const D3DXVECTOR3 pVector2);

This is going to invoke the copy constructor for pVector1 and pVector2, create new copies of each, and put them on the stack.  Not a big deal for a D3DXVECTOR3, but you need to be real careful if doing it with other object types.

0

Share on other sites
Clear, unless its just a float or an int, I'll also pass the other parameters by reference (vectors, strings etc.).
This will primarily also keel my code clear and readable (the few saved bytes are just a bonus :))
0

Create an account

Register a new account