current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 6 down vote favorite

I would like to have this code reviewed. I am learning C++ from a C background. How would I improve this code to make it more C++ like? This is a simple program that I wrote, which is a stripped down version of the actual full program I'm writing. 

#include <iostream>
using namespace std;

class Doge{
public:
    Doge();
    Doge(Doge&);
    ~Doge();
    int GetAge() const { return itsAge; }
    void SetAge(int age) { itsAge = age; }

private:
    int itsAge;
};

Doge::Doge(){
    cout << "Doge Constructor" << endl;
    itsAge = 5;
}

Doge::Doge(Doge&){
    cout << "Doge Copy Constructor" << endl;
}

Doge::~Doge(){
    cout << "Doge Destructor" << endl;
}

Doge * Function( Doge * theDoge){
    cout << "In Function" << endl;
    cout << "Scott is now " << theDoge->GetAge() << endl;
}

int main(){

    cout << "Make Doge" << endl;
    Doge Scott;
    cout << "Scott is " << Scott.GetAge() << endl;
    Scott.SetAge(10);
    Function(&Scott);
    cout << endl;
    return 0;
}
share|improve this question
1  
This is C++ and not C. –  EngieOP 8 hours ago
1  
I completely agree with @EngieOP, the C tag is for use on questions that are only written in C. C++ is a superset of C, it is not the same and therefore those tags should rarely be put together. –  syb0rg 7 hours ago

2 Answers 2

up vote 5 down vote accepted

First off, using namespace std is bad practice. Do not pollute the global namespace. Read this

You are passing objects by pointers which solves the problem of making multiple copies. But you are still using pointers, and that can be cumbersome. And there is the potential danger of the function changing the object.

Instead try passing a const pointer. This protects the object from being called by any non-const method and offers the security of passing by value. **As noted by Janos, you aren't returning anything here? Perhaps you are in your actual program.

const Doge * const Function(const Doge * const theDoge){
         std::cout << "In Function" << std::endl;
         std::cout << "Scott is now " << theDoge->GetAge() << std::endl;
}

Better yet, pass references to the object. This is the preferred way in C++.

const Doge & Function(const Doge & theDoge){
         std::cout << "In Function" << std::endl;
         std::cout << "Scott is now " << theDoge.GetAge() << std::endl;
}

Call function:

Function(Scott);

Using references is preferred because it is safer and easier to use than pointers.

share|improve this answer
up vote 4 down vote

It doesn't feel natural to initialize a Doge with a fixed age:

Doge::Doge(){
    cout << "Doge Constructor" << endl;
    itsAge = 5;
}

It would be more natural to make age a mandatory constructor parameter:

Doge::Doge(int itsAge): itsAge(itsAge) {
    std::cout << "Doge Constructor" << std::endl;
}

The copy constructor doesn't actually copy anything. Either remove it, or implement copying, for example:

Doge::Doge(Doge& other){
    std::cout << "Doge Copy Constructor" << std::endl;
    this->itsAge = other.itsAge;
}

This function is declared to return a Doge *, but it returns nothing:

Doge * Function( Doge * theDoge){
    cout << "In Function" << endl;
    cout << "Scott is now " << theDoge->GetAge() << endl;
}

This raises a compiler warning, at least with g++.

You probably implemented the destructor just to see when it gets called. That's fine, for testing. When you're done, just remember to remove it if it's not needed.

share|improve this answer
1  
(int itsAge): itsAge(itsAge) This won't cause a name collision? –  Dave 7 hours ago
1  
g++ happily compiles it without warnings –  janos 7 hours ago
1  
@DaveSmith This –  EngieOP 7 hours ago

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.