OOP design for election board

Question

I am trying to build an OOP design for an election system.

A citizen can nominate themselves to be a contender. So all contenders are citizens. I was thinking of keeping the relation ship IS-A by having contender extend citizen, but I am not able to understand how i will have a citizen nominate themselves then.

Citizen can become followers of contenders and receive a mail about any idea they post. Further more if they receive a mail and were also a contender they would send mail to their followers. Contenders can post at most 3 ideas in their manifesto and low quality contenders are dropped from the election (ie if they have at least 1 idea which has rating <5 from more than 3 people). Also if a person gives a rating > 5 to an idea, they are auto added as followers of the contender

Also if a contender is removed their rating of an idea still counts as they were a citizen also when they made it.

eg.

class Citizen {
   public:
     bool Citizen::nominate(Eboard* b) {
         return b->addContender(Contender(this));
     }
}

class Contender: public Citizen {
      Citizen* c;
    public:
      Contender(Citizen* c);
}

(1) I first thought it is fine to not have an ISA relationship. But with the mail constraint, i need to maintain a list of base class objects, as followers can be citizen or contenders.

(2) When system is required to maintain some constraints, take additional actions upon things like candidate giving rating- should the system be passed by reference from the main or should this be directly able to get the system singleton and operate on that.

Searching what can be done I got this helpful answer https://softwareengineering.stackexchange.com/questions/317297/inheritance-vs-additional-property-with-null-value?newreg=f39ae86e18e04b2f8136380610c4ffc8 which seemed to fit with what i could do

Can someone please help me with the best design choice here?

I was not able to get this so i did something like this in which i made the property whether a citizen is a contender which forced me to have functions like postIdea for citizens also.

#include <iostream>
#include <list>
#include <vector>
#include <unordered_map>

using namespace std;

class Citizen;
class Idea;
class Manifesto {
public:
    vector<Idea*> ideas;
    Citizen* contender;
    Manifesto(Citizen* contender) {
        this->contender = contender;
    }
    bool addIdea(Idea* idea) {
        if (ideas.size() == 3) {
            return false;
        }
        ideas.push_back(idea);
    }

};
//the rating is more than 5, then citizen is added as a follower of the contender.

class Idea {
public:
    unordered_map<Citizen*, int> citizenRating;
    Citizen* contender;
    Idea(Citizen* contender){
        this->contender = contender;
    }
    void setRating(Citizen* citizen, int rating) {
        citizenRating[citizen] = rating;
    }
    bool isRatedLt5ByGt3Voters() {
        int cnt = 0;
        for(auto iter = citizenRating.begin(); iter != citizenRating.end();iter++) {
            if (iter->second < 5) {
                cnt++;
            }
        }
        return cnt > 3;
    }

};
class Citizen {
public:
    bool isContender;
    list<Citizen*> followers;
    list<Citizen*> following; // for removing
    Manifesto* manifesto;
    Citizen() {
        isContender = false;
        manifesto = nullptr;
    }

    bool nominate() {
        cout<<"here";
        isContender = true;
        manifesto = new Manifesto(this);
    }

    bool setRating(Idea* idea, int rating) {
        idea->setRating(this, rating);
        // Ideally logic below this should be done by election system
        // not by the citizen, so probably call like Election.setRating(citizen, idea, rating) which will call this
 // and do below logic
        if (rating > 5) {
            makeFollower(manifesto->contender);
        }
        for(auto idea:manifesto->ideas) {
            if (idea->isRatedLt5ByGt3Voters()) {
                removeAsContender();
                break;
            }
        }
    }

    void makeFollower(Citizen* contender) {
        contender->followers.push_back(this);
        following.push_back(contender);
    }

    bool postIdea(Idea* idea) {
        this->manifesto->addIdea(idea);
        sendMail();
    }

    bool sendMail() {
        for(auto el: followers) {
            el->getMail();
        }
    }

    bool getMail() {
        std::cout<<"Got mail";
        sendMail();
    }

    bool removeAsContender() {
        this->isContender = false;
        followers.clear();
        for (auto iter = following.begin(); iter != following.end(); iter++) {
            for (auto iter2 = (*iter)->followers.begin(); iter2 != (*iter)->followers.end(); iter2++) {
                if (*iter2 == this) {
                    (*iter)->followers.erase(iter2);
                }
            }
        }
    }
};


int main() {
    std::cout << "Hello, World!" << std::endl;
    Citizen c;
    c.nominate();
    c.postIdea(new Idea(&c));
    Citizen c2;
    c2.makeFollower(&c);
    c.postIdea(new Idea(&c));
    return 0;
}

In the second version of my attempt i try to separate what the election does from what citizen does. So eg. the election should be responsible for sending mail , or adding followers on citizen's actions. Is this design better?

#include <iostream>
#include <unordered_set>
#include <vector>
#include <unordered_map>

using namespace std;

class Citizen;

class Idea;

class Manifesto {
public:
    vector<Idea *> ideas;
    Citizen *contender;

    Manifesto(Citizen *contender) {
        this->contender = contender;
    }

    bool addIdea(Idea *idea) {
        if (ideas.size() == 3) {
            return false;
        }
        ideas.push_back(idea);
    }

};

class Idea {
public:
    unordered_map<Citizen *, int> citizenRating;
    Citizen *contender;

    Idea(Citizen *contender) {
        this->contender = contender;
    }

    void setRating(Citizen *citizen, int rating) {
        citizenRating[citizen] = rating;
    }

    bool isRatedLt5ByGt3Voters() {
        int cnt = 0;
        for (auto iter = citizenRating.begin(); iter != citizenRating.end(); iter++) {
            if (iter->second < 5) {
                cnt++;
            }
        }
        return cnt > 3;
    }

};

class Election;
class Citizen {
public:
    bool isContender;
    unordered_set<Citizen *> followers;
    unordered_set<Citizen *> following; // for removing
    Manifesto *manifesto;
    Election *election;

    Citizen(Election *election) {
        isContender = false;
        manifesto = nullptr;
        this->election = election;
    }

    // Citizen should be able to get all contenders.
    unordered_set<Citizen *> getAllContenders();
    // Citizen should be able to nominate self.
    bool nominate();

    // These are defined outside as they need to dereference Election
    // and Election needs to dereference citizen

    bool setRating(Idea *idea, int rating) {
        idea->setRating(this, rating);
    }

    bool postIdea(Idea *idea) {
        this->manifesto->addIdea(idea);
    }

};

class Election {
    unordered_set<Citizen *> contenders;
public:
    unordered_set<Citizen *> getAllContenders() {
        return contenders;
    }

    void addContender(Citizen *contender) {
        contenders.insert(contender);
    }

    void nominate(Citizen *citizen) {
        citizen->nominate();
        contenders.insert(citizen);
    }

    void makeFollower(Citizen *citizen, Citizen *contender) {
        contender->followers.insert(citizen);
        citizen->following.insert(contender);
    }

    bool removeAsContender(Citizen *contender) {
        contender->isContender = false;
        contenders.erase(contender);
        contender->followers.clear();
        for (auto iter = contender->following.begin(); iter != contender->following.end(); iter++) {
            (*iter)->followers.erase(contender);
        }
    }

public:
    bool setRating(Citizen *citizen, Citizen *contender, Idea *idea, int rating) {
        citizen->setRating(idea, rating);

        if (rating > 5) {
            makeFollower(citizen, contender);
        }
        for (auto idea:contender->manifesto->ideas) {
            if (idea->isRatedLt5ByGt3Voters()) {
                removeAsContender(contender);
                break;
            }
        }
    };

    bool postIdea(Citizen *contender, Idea *idea) {
        contender->postIdea(idea);
        sendMail(contender);
    }

    bool sendMail(Citizen *contender) {
        cout << "in send " << contender->followers.size();
        for (auto el: contender->followers) {
            cout << "trying ";
            getMail(el);
        }
    }

    bool getMail(Citizen *c) {
        std::cout << "Got mail";
        if (c->isContender) {
            sendMail(c);
        }
    }

};

unordered_set<Citizen *> Citizen::getAllContenders() {
    return election->getAllContenders();
}

bool Citizen::nominate() {
    isContender = true;
    manifesto = new Manifesto(this);
    election->addContender(this);
}


int main() {
    Election e;
    std::cout << "Hello, World!" << std::endl;
    Citizen c(&e);
    c.nominate(); // citizen can self nominate
    Idea *idea = new Idea(&c);
    e.postIdea(&c, idea);
    Citizen c2(&e);
    e.setRating(&c2, &c, idea, 8);
    e.postIdea(&c, new Idea(&c));
    cout << "CONT" << e.getAllContenders().size();
    return 0;
}

I do not like this design as Citizen class should not have anything to do with functions like sendMail, but since the same object can be made a contender anytime, i can only think of composition which fails due to it also requiring to extend citizen.

Answer 1

Here are some things that may help you improve your code. I'll start with some details and then move on to the larger design issues.

Make sure all paths return a value

The Citizen::setRating() and Citizen::postIdea() and many other functions claim to return bool but don't return anything.

Don't abuse using namespace std

Especially in a very simple program like this, there's little reason to use that line. Putting using namespace std at the top of every program is a bad habit that you'd do well to avoid.

Keep internal class details private

It's best to keep the internals of a class private to reduce linkage among objects to only what they need. This simplifies the interface and therefore the maintenance. Right now, for example, one could easily bypass the Manifesto::addIdea() function and add 300 ideas directly to the structure. That's poor encapsulation.

Avoid using pointers

Modern C++ doesn't really need pointers very often. It's usually better to either use a smart pointer, such as std::unique_ptr or simply use objects or object references. For this code, which is used depends very much on the class design which is the subject of some suggestions below.

Avoid new and delete

If you don't use pointers, you have much less reason to use new and delete. But if you do use new, each instance of new must be matched to a corresponding delete or your code will leak memory. Often the best way to make sure no memory is leaked is to put delete in a destructor.

Rethink your class design

One way to approach class design is to begin with a written description. Examining the description, nouns suggest objects and verbs suggest methods. You have already identified Citizen, Idea, Manifesto and Election as potential classes and Candidate as a potential class. Each of the suggestions below will discuss a specific way to approach the class design.

Decide what the code should do

It seems to me that the first thing to decide is how you want to use this code. Given the title of the question, I would expect that everything should happen in the context of an Election which suggests to me that rather than directly manipulating Idea or Citizen objects, main() should only have a single Election object and do everything within that context. That leads directly to the next suggestion.

Decide what each object needs to do

It is odd to me that an Election object doesn't actually seem to include voting but does seem to include sending mail. Perhaps this is only intended to represent a particular phase of the election which involves nominations of candidates and the dissemination of ideas. Because of the particular features of the class, I'm going to infer that it's intended to run a simulation of that particular phase of an election rather than being software that is used for an actual election. This is an important distinction because it suggests a very different pattern of use. I'm going to assume that it's a simulation for all of the suggestions below, but the general approach is applicable either way.

Decide on sequence and responsibility

The written description includes sentences like this one:

Contenders can post at most 3 ideas in their manifesto and low quality contenders are dropped from the election (ie if they have at least 1 idea which has rating 5 from more than 3 people).

We can glean many useful things from this particular sentence. First, only a contender can post an idea to their own manifesto. This seems to imply that the Manifesto should be publicly visible, but only managed by a contender, suggesting a private class with read-only public access. Second it says that a contender might be dropped from the election based on idea ratings. This implies a sequence but fails to spell out what that sequence is. That is, clearly the ideas must be rated before contenders are culled, and that ideas must be posted before they can be rated. We also know from the description that contenders must be nominated before they are changed from citizens to candidates. This fuzziness about sequence is why this code is not yet well designed. Are there distinct phases such as nomination, manifesto generation, voting, culling? Or can any action be taken at any time? Further what happens to the already rated ideas if a candidate is culled from the election and loses contender status? Also, it is not explicitly stated which entity does the culling of candidates. In general passive verbs are a sign of a vague or incomplete description.

Start from the public interface

After the sequence and responsibilities are clarified, it is often useful to start with a public interface for a class. For example, I would say that if we're trying to run a simulation, what do we want from that simulation? What are we trying to learn or see? Do we want to, say, derive an equation that predicts the number of emails sent given the size of the election and number of candidates? Or maybe we just want the detailed chronology to do a more open ended investigation. It's good practice to define the minimally sufficient public interface for the main class or classes before working on the details of implementation.

Don't let technology drive the solution

The very first sentence in the description is this:

I am trying to build an OOP design for an election system.

It seems to me that there are some potential problems with that. First, is the goal to create "an OOP design" or actual working code? Second, it's not necessarily true that OOP is the right approach for every problem. It seems to me that the problem definition is still too vague to assume that. Third, "an election system" is ambiguous as noted above because it actually seems that perhaps it's a real election, or maybe it's a simulation and that it's apparently only a particular subset of election activities. I'm sure that seems overly picky about one sentence, but I find that the greater clarity one can achieve for a succinct problem statement, the better the subsequent design and code. It's worthwhile trying to clarify before investing a lot of time in design or coding.

Focus on the user

Instead of starting with a code-centric approach, think of the user instead. "Who will be using the software and what will they want from it?" is much more likely to guide a successful design and implementation than thinking about code.

Answer 2

Start by writing down the use-cases in plain text. What is it that you are designing and how should it work. "It is a noun so it must be a class" is a common mistake. You don't seem to be designing "citizens" but rather mail lists.

So the class here should perhaps be MailList, which can contain a list of registered citizens. One instance of the mail list per contender, contender probably doesn't need to be a class, meaning you'll have a MailList array the size of the number of contenders. There's no direct relation between citizen and contender that motivates inheritance or polymorphism.

You can have a citizen class still, but it would be a pretty "dumb" one only containing name and e-mail address. Or possibly just the e-mail address, in which case a class isn't needed and it can be a string.