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.

I want to create a container that has an AutoList object list. I wonder if my container is correct.

class Auto{
    string model;
    int co2;
public:
    Auto ();
    Auto(string m, int c);
    ~Auto(){};
    string GetModel(){return model;}
    void SetModel(string s){model = s;}
    int GetCo2(){return co2;}
    void SetCo2(int k){co2 = k;}
    bool operator < (const Auto &kitas);
};

class AutoList{
private:
    Auto data;
    AutoList *next;
public:
    AutoList(Auto a = Auto(), AutoList *al = NULL):
        data(a), next(al){}
    ~AutoList(){}

    void SetData(const Auto &a){data = a;}
    Auto GetData() const {return data;}

    void Set(AutoList *newaut){next = newaut;}
    const AutoList *Get() const {return next;}
    AutoList *Get(){return next;}
};

// I need to make a container class which saves AutoList object list.

class Container{ // is this class is container of AutoList object LIST???
private:
    AutoList *beg, *end, *d;
public:
    Container():beg(NULL), end(NULL), d(NULL){}
    ~Container(){Delete();}
    void Delete();

    Container(const Container & kitas);
    Container & operator = (const Container & kitas);

    void Begin(){d = beg;}
    bool True(){return d != NULL;}
    void Next(){d = d->Get();}
    AutoList GetAutoList(){return d->GetData();}
    void SetAutoList(const Auto & data);
};

void Container::Delete(){
    while(beg){
        d = beg;
        beg = beg->Get();
        delete d;
    }
    end = NULL;
}

void Container::SetAutoList(const Auto & data){
//..........
}
share|improve this question

3 Answers 3

AutoList is for Node of Auto. The name AutoList is very confusing. I would name it as AutoNode so that a reader can know that it's a node element and not a List in itself.

share|improve this answer
1  
+1. It honestly took me a while to realize that, hence why I haven't mentioned it myself. Naming really is important. Bearing this in mind, you may mention that getters/setters are not good for node implementations (if you know enough about that). –  Jamal Apr 27 at 22:50
  • The absence of std:: in front of string can imply that you're using using namespace std. If so, this should be removed as it can break code (mainly with name-clashing) that includes this file. Read this for more information.

  • Your variables could have more descriptive names. There are many single-character names, which is discouraged except when they're used as loop counters. Giving poor names can greatly reduce readability as other readers may not be able to understand their purpose.

    One example is co2. If this is short for something, then spell it out. You may already know what it means, but it doesn't mean others will understand it as well.

  • Accessors ("getters"), conditional overloaded operators, and any other member member function that doesn't modify data members should be const. This will also make the intent clear to the reader and prevent any accidental modification of data members.

    Examples:

    int getSomething() const { return something; }
    

    bool operator==(Class const& rhs) const { return something == rhs.something; }
    
  • I see that you're trying to maintain The Rule of Three in Container (as it contains pointers as members), which is good. However, it appears to be incomplete as there's nothing in the overloaded copy constructor except for a placeholder. There's also a declaration for the overloaded assignment operator, but no implementation.

    For such instances of code to be added, it's common to leave a // TODO comment to state what must still be done in the future. This is much more clearer to readers than a line of ellipses.

share|improve this answer

Class call "Auto"

class Auto{

But to simular to the auto keyword for my liking.

If the destructor does nothing.

    ~Auto(){};

Then don't add it to your class; the compiler generated version works fine.

Hate Getters/Setters. Its bad design. You are exposing the internals of your class. Use only as a last resort. The members of your class should do the operations required.

    string GetModel(){return model;}
    void SetModel(string s){model = s;}
    int GetCo2(){return co2;}
    void SetCo2(int k){co2 = k;}

Also note: Geters don't mutate the state of your object (or should not). So you should mark them as const. Since they are not going to mutate the state you can return a const reference to the internal member.

  string const& GetModel()     const   {return model;}
  ///    ^^^^^^                             Return a const ref to member.
  //                           ^^^^^        Note this function does not mutate the object.

Same applies to comparison functions. You don't mutate the state so mark it as const.

    bool operator <  const (const Auto &kitas);
    //               ^^^^^

This is not a list object. More like a node in the list. class AutoList{

Usless Destructor again:

    ~AutoList(){}

This is really the list. The term container is very generic. Call it a list.

class Container{ // is this class is container of AutoList object LIST???

The member d seems to be used as a temprary. I don't think you actually need it as a member. Declare a local variable in each member.

    AutoList *beg, *end, *d;

Iterators in C++ are well defined. They are objects that respond to ++ and * (de-reference). Return an object like this. Otherwise your list can only support one iterator.

    void Begin(){d = beg;}
    void Next(){d = d->Get();}
    bool True(){return d != NULL;}
    AutoList GetAutoList(){return d->GetData();}
share|improve this answer

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.