General C++ coding style

[pieterjnr]

Hi guys!

I just started learning C++ this week and I finished my first non-trivial program: a class of intrusive list elements (or a linked list or whatever you want to call it). Any comments about the code style, common C++ errors, use of pointers, how to comment the code etc. would be greatly appreciated. Thanks!

#include <iostream>
#include <string>

class intrusive_list {
public:
intrusive_list() { // Default constructor
   value_ = 0;
   previous_ = nullptr;
   next_ = nullptr;
}

intrusive_list(int value, intrusive_list* previous, intrusive_list* next ) // Constructor from an integer and a pointer to a list
   : value_(value), previous_(previous), next_(next) {
}

intrusive_list(intrusive_list const& that) // Copy constructor
   : value_(that.value_), previous_(that.previous_), next_(that.next_) {
}

intrusive_list& operator=(intrusive_list that) { // Definition of assignment operator
   value_ = that.value_;
   previous_ = that.previous_;
   next_ = that.next_;
   return *this;
}

~intrusive_list() {} // Destructor

void append(intrusive_list& appended_list) { // Add element at the end of the list
   next_ = &appended_list; // Value of pointer is copied
   appended_list.previous_ = this;
}

void insert(intrusive_list& inserted_list) { // Add element at the beginning of the list
   previous_ = &inserted_list; // Value of pointer is copied
   inserted_list.next_ = this;
}

void delete_item() {
   // Do linking
   if (next_ != nullptr) {
      intrusive_list& next(*next_); // Intrusive list to the right
      next.previous_ = previous_; // Value of pointer is copied
   }
   if (previous_ != nullptr) {
      intrusive_list& previous(*previous_); // Intrusive list to the left
      previous.next_ = next_; // Value of pointer is copied
   }
   // Remove linkages from this element
   next_ = nullptr;
   previous_ = nullptr;
}

   int size() {
   intrusive_list root = get_root(); // Get root element in list
   int size = 1;
   while (root.next_ != nullptr) {
      ++size;
      root = intrusive_list(*root.next_); // Go to next element
   }
   return size;
}

void print() { // Print all elements in the list
   intrusive_list root = get_root(); // Get root element in list
   std::string printstring = "";
   while (root.next_ != nullptr) {
      printstring += std::to_string(root.value_) + ", ";
      root = intrusive_list(*root.next_); // Go to next element
   }
   printstring += std::to_string(root.value()) + ", "; // Don't forget the last element in the list!
   std::cout << printstring << std::endl;
}

intrusive_list get_root() {
   if (previous_ == nullptr)
      return *this;
   else
      return intrusive_list(*previous_).get_root(); // Recursive call using copy constructor
}

// Getters
int& value() { return value_; }
intrusive_list* previous() { return previous_; }
intrusive_list* next() { return next_; }
// Constant getters
int value() const { return value_; } // Cannot change a constant object, hence do not return a pointer
intrusive_list* previous() const { return previous_; }
intrusive_list* next() const { return next_; }

private:
int value_;
intrusive_list* previous_;
intrusive_list* next_;

};

int main()	{
   // make a new intrusive list
   intrusive_list mylist(5, nullptr, nullptr);
   std::cout << "The root element has value " << mylist.get_root().value() << std::endl;
   // append an element
   intrusive_list appended_list(3, nullptr, nullptr);
   mylist.append(appended_list);
   std::cout << "The root element of appended_list has value " << appended_list.get_root().value() << std::endl;
   // insert an element
   intrusive_list inserted_list(7, nullptr, nullptr);
   mylist.insert(inserted_list);
   std::cout << "The root element of appended_list has value " << appended_list.get_root().value() << std::endl;
   // delete an element
   mylist.delete_item();
   std::cout << "The root element of appended_list has value " << appended_list.get_root().value() << std::endl;
   // check size
   std::cout << "The intrusive_list has " << appended_list.size() << " elements." << std::endl;
   appended_list.print();
   std::cout << "Mylist has " << mylist.size() << " element." << std::endl;
   mylist.print();
}

[dhayden]

Have you programmed in other languages before? If this is your first non-trivial program ever than I want to say that you have a distinguished career in programming ahead of you. It takes most people a long time to grasp the concept of pointers.

Still, there are some problems, but they are pretty common.

First, it's usually easier to distinguish between the list and a an individual item within the list. These are usually represented as different classes. If you think about it, the person using the list class shouldn't have to care about previous and next pointers. They just want to store their ints. In other words, the user wants the main program to look sort of like this:

int main()	{
   // make a new intrusive list
   intrusive_list mylist;
   mylist.insert(5);
   std::cout << "The root element has value " << mylist.get_root_value() << std::endl;
   // append an element
   mylist.append(3);
   std::cout << "The root element has value " << myList.get_root_value() << std::endl;
   // insert an element
   mylist.insert(7);
   std::cout << "The root element has value " << myList.get_root_value() << std::endl;
   // delete an element
   mylist.delete_item();
   std::cout << "The root element has value " << myList.get_root_value() << std::endl;
   // check size
   std::cout << "The list has " << myList.size() << " elements." << std::endl;
   myList.print();
}

See how much easier this is to use? You can tell someone "it's a list of integers" and they don't have to know much more about it.

The trick to coding it this way is that the things like the insert() and append() methods have to create a list item using new. And that means that they also have to delete it in the list destructor. Have you dealt with this sort of memory management before? It's another tough topic for beginners.

Your insert() and append() methods won't work if you're inserting/appending to the middle of the list. Append() is dropping a new item between "this" and the item after "this." So you need to adjust some more pointers. Insert() does the same. Look at you delete_item method. It has the logic for dealing with all the cases.

Comments on methods should explain what the method does, what the parameters are and what the return value is. You've done a good job here. In a professional setting you wouldn't need to indicate the default constructor, copy constructor and assignment operator - the reader can tell that's what they are from the declaration.

I'd declare the methods in the class and put the definitions below. This way a reader can see what the class does and what methods are available without having to scroll through hundreds of lines of implementation details.

get_root() should return a pointer to the root node, not a copy of it.

Nice job.

[pieterjnr]

Thank you for the valuable comments. I have quite some experience in Matlab and did some basic Java programming. But still, the concept of thinking about when to use a pointer or the value itself is pretty new to me. I will certainly try the adaptations you suggested, especially the first one would make the use of this class much easier.

Thanks

[AbstractionAnon]

Very nice for just a week of C++.

Lines 54-62: incomplete code fragment, Cut and paste error?

General comments:
You seem to mix commenting on the right of the line (declaration) and placing comments on a separate line (main). I prefer to see one style of commenting (separate line).