Jump to content

  • Log In with Google      Sign In   
  • Create Account


Cleaning and 'improving' your code (const)


Old topic!
Guest, the last post of this topic is over 60 days old and at this point you may not reply in this topic. If you wish to continue this conversation start a new topic.

  • You cannot reply to this topic
19 replies to this topic

#1 cozzie   Members   -  Reputation: 1585

Like
0Likes
Like

Posted 21 June 2013 - 09:01 AM

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 :)

 



Sponsor:

#2 belfegor   Crossbones+   -  Reputation: 2560

Like
0Likes
Like

Posted 21 June 2013 - 09:34 AM

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



#3 Paradigm Shifter   Crossbones+   -  Reputation: 5255

Like
1Likes
Like

Posted 21 June 2013 - 09:53 AM

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.


Edited by Paradigm Shifter, 21 June 2013 - 09:57 AM.

"Most people think, great God will come from the sky, take away everything, and make everybody feel high" - Bob Marley

#4 cozzie   Members   -  Reputation: 1585

Like
0Likes
Like

Posted 21 June 2013 - 11:22 AM

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?



#5 Waterlimon   Crossbones+   -  Reputation: 2469

Like
0Likes
Like

Posted 21 June 2013 - 12:30 PM

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, 21 June 2013 - 12:34 PM.

o3o


#6 cozzie   Members   -  Reputation: 1585

Like
0Likes
Like

Posted 21 June 2013 - 12:39 PM

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, 21 June 2013 - 12:40 PM.


#7 Paradigm Shifter   Crossbones+   -  Reputation: 5255

Like
0Likes
Like

Posted 21 June 2013 - 12:57 PM

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.


"Most people think, great God will come from the sky, take away everything, and make everybody feel high" - Bob Marley

#8 cozzie   Members   -  Reputation: 1585

Like
0Likes
Like

Posted 21 June 2013 - 01:47 PM

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, 21 June 2013 - 01:50 PM.


#9 achild   Crossbones+   -  Reputation: 1703

Like
1Likes
Like

Posted 21 June 2013 - 02:11 PM

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.

 

[edit] 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, 21 June 2013 - 02:15 PM.


#10 Paradigm Shifter   Crossbones+   -  Reputation: 5255

Like
0Likes
Like

Posted 21 June 2013 - 02:11 PM

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.


Edited by Paradigm Shifter, 21 June 2013 - 02:14 PM.

"Most people think, great God will come from the sky, take away everything, and make everybody feel high" - Bob Marley

#11 cozzie   Members   -  Reputation: 1585

Like
0Likes
Like

Posted 21 June 2013 - 03:30 PM

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 :)



#12 Paradigm Shifter   Crossbones+   -  Reputation: 5255

Like
0Likes
Like

Posted 21 June 2013 - 03:39 PM

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. ".").


"Most people think, great God will come from the sky, take away everything, and make everybody feel high" - Bob Marley

#13 cozzie   Members   -  Reputation: 1585

Like
0Likes
Like

Posted 22 June 2013 - 05:14 AM

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.

#14 King Mir   Members   -  Reputation: 1952

Like
1Likes
Like

Posted 22 June 2013 - 09:30 PM

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, 22 June 2013 - 09:31 PM.


#15 cozzie   Members   -  Reputation: 1585

Like
0Likes
Like

Posted 23 June 2013 - 03:57 AM

Yep, I understand.

An example in my case:

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


#16 Matt-D   Crossbones+   -  Reputation: 1450

Like
0Likes
Like

Posted 23 June 2013 - 01:12 PM

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

[C++ FAQ / Section 18] Const correctness: http://www.parashift.com/c++-faq/const-correctness.html
In particular:
- http://www.parashift.com/c++-faq/overview-const.html
- http://www.parashift.com/c++-faq/const-and-type-safety.html
GotW #6a: http://herbsutter.com/2013/05/24/gotw-6a-const-correctness-part-1-3/
GotW #6b: http://herbsutter.com/2013/05/28/gotw-6b-solution-const-correctness-part-2/
 


Edited by Matt-D, 23 June 2013 - 01:13 PM.


#17 ChaosEngine   Crossbones+   -  Reputation: 2290

Like
0Likes
Like

Posted 23 June 2013 - 03:46 PM

** 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.


if you think programming is like sex, you probably haven't done much of either.-------------- - capn_midnight

#18 cozzie   Members   -  Reputation: 1585

Like
0Likes
Like

Posted 23 June 2013 - 04:43 PM

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....

#19 mhagain   Crossbones+   -  Reputation: 7823

Like
0Likes
Like

Posted 23 June 2013 - 04:58 PM

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.


It appears that the gentleman thought C++ was extremely difficult and he was overjoyed that the machine was absorbing it; he understood that good C++ is difficult but the best C++ is well-nigh unintelligible.


#20 cozzie   Members   -  Reputation: 1585

Like
0Likes
Like

Posted 23 June 2013 - 06:33 PM

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 :))




Old topic!
Guest, the last post of this topic is over 60 days old and at this point you may not reply in this topic. If you wish to continue this conversation start a new topic.



PARTNERS