void COrange::CreateApple(void)
{
CApple Apple;//create apple
Apple.String1="Hello";//set initial values of strings
Apple.String2="Hey";
Orange.m_lstApples.push_back(Apple);//add apple to vector
}
void COrange::LogApple(const string &sAppleName)
{
//find apple in vector
vector<CApple>::iterator itApple=m_lstApples.begin();
while(itApple!=m_lstApples.end())
{
if(itApple->m_sAppleName==sAppleName)//found apple
{
//write apples variable string1 to log
g_log<<"Apple String1="<<itApple->String1<<endl;
return;//return without looping further
}
else//not found, try next apple in vector
++itApple;
}
}
Is my code good programming practise?
Author
Hi, I am using C++.
I have some example code below of what I tend to do in my program. I am wondering if I am doing anything that could cause bugs.
On of the things I am concerned about is how I create an 'apple' and add it to a vector to use it later in another function. As you see in the example code below, that's how I do it right now:
Should I create my 'apple' object as a pointer, eg
CApple *Apple=new CApple;//create apple
and then store it in a vector full of 'pointers to apple objects'?
Also tell me of any other programming practises that are not the best?
Any advice would be appreciated?
I'm sure everyone has their own opinion on these sorts of things but I am going to answer with the dreaded "it depends".
If your CApple class is a "value" class then you should carry on like you have been. However if it is a "reference" class then you should create it with the new operator. The difference is one of semantics, a value class represents a simple value like a length or a weight and is often immutable once created. A reference class represents some sort of state in the domain and usually represents a unique entity.
I'm sure everyone else has different opinions though :)
If your CApple class is a "value" class then you should carry on like you have been. However if it is a "reference" class then you should create it with the new operator. The difference is one of semantics, a value class represents a simple value like a length or a weight and is often immutable once created. A reference class represents some sort of state in the domain and usually represents a unique entity.
I'm sure everyone else has different opinions though :)
No. There are actual reasons to do it one way or another that go beyond semantics.
Here are some general "rules" to follow (although there will always be exceptions):
- Where you can, it is better to put an object into a container rather than a pointer to an object.
- Where you need to have polymorphic objects, you have no choice but to use a pointer.
- Where the object has an expensive copy-constructor it will run faster if you use a pointer or a container that does few or no copies (std::list good; std::vector bad).
So, yes, you'll often find that class objects end up with pointers while basic types and small POD types end up without pointers, the reason is not semantics.
And finally, and probably more importantly than the question of whether or not to use a pointer, is the fact that whenever you do use a pointer, it is best to use a smart pointer (not std::auto_ptr) both generally and when putting using containers.
Here are some general "rules" to follow (although there will always be exceptions):
- Where you can, it is better to put an object into a container rather than a pointer to an object.
- Where you need to have polymorphic objects, you have no choice but to use a pointer.
- Where the object has an expensive copy-constructor it will run faster if you use a pointer or a container that does few or no copies (std::list good; std::vector bad).
So, yes, you'll often find that class objects end up with pointers while basic types and small POD types end up without pointers, the reason is not semantics.
And finally, and probably more importantly than the question of whether or not to use a pointer, is the fact that whenever you do use a pointer, it is best to use a smart pointer (not std::auto_ptr) both generally and when putting using containers.
Here's some things to consider:
String1 and String2 may be better being simply parameters to the CApple constructor. This depends on how it's most often used, but from the looks of it, it would be a good idea.
Next, Most of the code within LogApple is performing the very common function of finding a particular item. You should train yourself to recognise things like that and replace them with simpler alternatives that do the work for you, such as:
Edit:
Also take note of how I've formatted the code with more spaces around some things to make it more readable.
After taking the above advice you could even reduce the first function to this if it seems appropriate for what you are doing:
String1 and String2 may be better being simply parameters to the CApple constructor. This depends on how it's most often used, but from the looks of it, it would be a good idea.
Next, Most of the code within LogApple is performing the very common function of finding a particular item. You should train yourself to recognise things like that and replace them with simpler alternatives that do the work for you, such as:
void COrange::LogApple(const string &sAppleName){ //find apple in vector vector<CApple>::iterator itApple = m_lstApples.find(sAppleName); if (itApple != m_lstApples.end()) { g_log << "Apple String1=" << itApple->String1 << endl; }}Edit:
Also take note of how I've formatted the code with more spaces around some things to make it more readable.
After taking the above advice you could even reduce the first function to this if it seems appropriate for what you are doing:
void COrange::CreateApple(void){ Orange.m_lstApples.push_back(CApple("Hello", "Hey");//add apple to vector}Quote:Original post by Andrew Russell
No. There are actual reasons to do it one way or another that go beyond semantics.
...
So, yes, you'll often find that class objects end up with pointers while basic types and small POD types end up without pointers, the reason is not semantics.
So you don't rate semantics? I find that semantics are the biggest aid or hinderance (depending on if they are "good" or "bad") to understanding and maintaining code.
I hope I don't offend but if I may challenge your rules:
Quote:
- Where you can, it is better to put an object into a container rather than a pointer to an object.
If we follow your advice about using smart pointers then I think using pointers is fine. When using ordinary pointers, if the container is owned by another object then the pointers can be freed in that objects destructor.
Quote:
- Where the object has an expensive copy-constructor it will run faster if you use a pointer or a container that does few or no copies (std::list good; std::vector bad).
I find that spurious aliasing caused by inappropriate use of pointers to be a much bigger threat than worrying about the copy constructor. Also this sounds a bit like premature optimisation to me, not all copy operations are significant to the run time and memory usage of a program and trying to stamp them out based on deciding they are all inherently evil can be a considerable waste of effort.
Author
Quote:Original post by iMalc
Next, Most of the code within LogApple is performing the very common function of finding a particular item. You should train yourself to recognise things like that and replace them with simpler alternatives that do the work for you, such as:Less lines of code means less bugs on average, besides that's what it's there for, and it can be more optimised that your own code for finding too.void COrange::LogApple(const string &sAppleName){ //find apple in vector vector<CApple>::iterator itApple = m_lstApples.find(sAppleName); if (itApple != m_lstApples.end()) { g_log << "Apple String1=" << itApple->String1 << endl; }}
What does 'find' do? And is that method (using find) faster than a while loop?
Quote:What does 'find' do? And is that method (using find) faster than a while loop?
c.find(x) returns an iterator to an object equal to x in the container c. If such an object doesn't exist, it returns c.end()
It's unlikely to be faster than a while() loop, as thats how it's probably implemented.
To use it with a string as a parameter, you'll need to provide an
operator ==(const std::string&) in your CApple class (or provide an equality predicate to the find function as an extra parameter).
Quote:Original post by vaugulaQuote:Original post by Andrew Russell
No. There are actual reasons to do it one way or another that go beyond semantics.
...
So, yes, you'll often find that class objects end up with pointers while basic types and small POD types end up without pointers, the reason is not semantics.
So you don't rate semantics? I find that semantics are the biggest aid or hinderance (depending on if they are "good" or "bad") to understanding and maintaining code.
I hope I don't offend but if I may challenge your rules:Quote:
- Where you can, it is better to put an object into a container rather than a pointer to an object.
If we follow your advice about using smart pointers then I think using pointers is fine. When using ordinary pointers, if the container is owned by another object then the pointers can be freed in that objects destructor.Quote:
- Where the object has an expensive copy-constructor it will run faster if you use a pointer or a container that does few or no copies (std::list good; std::vector bad).
I find that spurious aliasing caused by inappropriate use of pointers to be a much bigger threat than worrying about the copy constructor. Also this sounds a bit like premature optimisation to me, not all copy operations are significant to the run time and memory usage of a program and trying to stamp them out based on deciding they are all inherently evil can be a considerable waste of effort.
Preferring to pass around references or pointers instead of objects by value is not a premature optimization - it's good practice.
It's very likely that reference counted smart pointers will be added to the next version of the standard library (until then we have boost::shared_pointer), so using pointers instead of values in containers doesn't pose many problems.
Quote:Original post by NitageWhilst find() may not be quicker than manually iterating through for a vector, it certainly is faster when it comes to maps, sets and the like.Quote:What does 'find' do? And is that method (using find) faster than a while loop?
c.find(x) returns an iterator to an object equal to x in the container c. If such an object doesn't exist, it returns c.end()
It's unlikely to be faster than a while() loop, as thats how it's probably implemented.
To use it with a string as a parameter, you'll need to provide an
operator ==(const std::string&) in your CApple class (or provide an equality predicate to the find function as an extra parameter).
Besides, if you have two things of equal speed, but one of them is simpler, then ... (Apply Occam's Razor here).
Quote:Original post by vaugula
So you don't rate semantics? I find that semantics are the biggest aid or hinderance (depending on if they are "good" or "bad") to understanding and maintaining code.
Semantics are great. However in this case it would be bad to base the decision purely on the resultant semantics.
Quote:I find that spurious aliasing caused by inappropriate use of pointers to be a much bigger threat than worrying about the copy constructor. Also this sounds a bit like premature optimisation to me, not all copy operations are significant to the run time and memory usage of a program and trying to stamp them out based on deciding they are all inherently evil can be a considerable waste of effort.
Not prematurely optimising is also great. However selection of the correct data structure for a given situation is not exactly premature optimisation.
In particular, in my example - an object that is expensive (which will naturally be situation-defined) to copy shouldn't go into a vector without a level of indirection with a cheep copy (such as a pointer). And the reason I noted this is because it is an important exception to the idea that objects should "by default" go directly into containers where possible.
Quote:Original post by Nitage
Preferring to pass around references or pointers instead of objects by value is not a premature optimization - it's good practice.
I have no problem with pointers and references to reference types being passed around. I can also understand const references to anything being passed because then you can't end up with an aliasing problem. However I don't see a problem in passing value types by value. Just to take it to an extreme example, I don't think many people would advocate passing an double or long double by reference and in my mind there is very little difference between that and passing a value type by value.
This topic is closed to new replies.
Advertisement
Popular Topics
Advertisement
