C++ linked list class

I have a lot of experience in Python and Haskell but am new to C++, so I'm wondering if my code is idiomatic. For one of the first assignments in my class, I wrote a linked list class (the "cities" are nodes in the linked list, the teacher wanted to make things interesting, I guess). Here's the header I had to implement, the functions are described with comments:

#ifndef COMMUNICATIONNETWORK_H
#define COMMUNICATIONNETWORK_H
#include 
struct City{
    std::string cityName;
    std::string message; //Ignore this bit, it's not relevant
    City *next;
    City *previous;

    City(){};

    City(std::string initName, City *initNext, City *initPrevious, std::string initMessage)
    {
        cityName = initName;
        next = initNext;
        previous = initPrevious;
        message = initMessage;
    }

};

class CommunicationNetwork
{
    public:
        CommunicationNetwork();
        ~CommunicationNetwork();
        void addCity(std::string, std::string); //Insert city with given name after city with given name
        void printNetwork(); //Go through and print all city names
        void deleteCity(std::string); //Remove city with given name
        void deleteNetwork(); //Free all memory, name the cities being deleted, replace head with NULL
    protected:
    private:
        City *head;
        City *tail;
};

#endif // COMMUNICATIONNETWORK_H

Here's my implementation (the weird output formatting was mandatory):

```
#include "CommunicationNetwork.h"
#include
#include
using namespace std;

CommunicationNetwork::CommunicationNetwork()
{ head = new City; tail = new City; }

CommunicationNetwork::~CommunicationNetwork()
{ if(head) CommunicationNetwork::deleteNetwork(); }

void CommunicationNetwork::printNetwork()
{
if(head == NULL)
{
cout next != NULL)
{
cout cityName ";
currentCity = currentCity->next;
}
cout cityName NULL" previous = new City(newCity, head, NULL, "");
head = head->previous;
}
else

A couple of good points:

• Good use of header guards

• Avoiding use of using namespace std in the

header is good.

The use of new and delete is discouraged in modern C++, but in this case, since it is for a class project, it is probably okay. Normally you wouldn't be implementing linked lists yourself. It does mean you have to be extra careful about memory management though.

You haven't defined or deleted a copy constructor or assignment operator, so you could easily have memory issues. In your case you would probably just want them to be deleted.

CommunicationNetwork(const CommunicationNetwork &) = delete;
void operator=(const CommunicationNetwork &) = delete;

Your City class looks like it is intended to only be used with the CommunicationNetwork. You might consider making it be a nested class:

class CommunicationNetwork {
   .
   .
   .
   private:
       struct City;
};

struct CommunicationNetwork::City {
   .
   .
   .
};

Members should be initialized with a constructor initializer list when possible:

City(
    std::string initName,
    City *initNext,
    City *initPrevious,
    std::string initMessage
)
: cityName(initName),
  message(initMessage),
  previous(initPrevious),
  next(initNext)
{
}

It's a good idea to initialize members that don't have a default value, especially pointers:

City()
 : next(nullptr),
   previous(nullptr)
 {
 };

As a general rule, large objects (using more than a few words of memory) should be passed by const reference:

void addCity(const std::string &, const std::string &);

Prefer using '\n' instead of endl:

cout previous->cityName << '\n';

There's nothing technically wrong with endl, but every time you use it, it flushes the output, which is often an unnecessary performance hit.

StackExchange Code Review Q#120109, answer score: 4