4
\$\begingroup\$

I solved this problem using custom functions and calling them in main function. The challenge was to avoid global variables. Please note that I'm C++ noob and this program is fairly large. I'm posting it here so experts can give me suggestion and point out my mistakes for good programming practices.

Is there any better code to improve calculator program function? For instance, a user enters 123+45*11/2 and it automatically solve the equation using std::stringstream?

#include <iostream>
#include <string>
#include <sstream> // for stringstream and eof()

//to avoid global variable declaration, I've used some parameters. Here I'm "Passing arguments by reference"
void startPoll(std::string &pollQuestion,
               std::string &pollOption1,
               std::string &pollOption2,
               std::string &pollOption3);
void displayPoll(std::string &pollQuestion,
           std::string &pollOption1,
           std::string &pollOption2,
           std::string &pollOption3,
           int &pollVote,
           int &plusOne,
           int &plusTwo,
           int &plusThree);
void acceptOnlyNumbers(std::string &pollVoteString,int &pollVote);
void printGraphBar (float &votePercentage);
void calculationFunction (double &result);
void checkAndCalculate (std::string &logicalOperator,
                        double &firstNumber,
                        double &secondNumber,
                        double &result);


int main (void)
{
    std::string pollQuestion, pollOption1, pollOption2, pollOption3,
                mainMenu;
    int         pollVote = 0, plusOne, plusTwo, plusThree,
                selectMenu;
    double result;

    std::cout << "What would you like to do?\n"
              << "1. Create a poll\n"
              << "2. Start calculator\n"
              << "3. Smile\n";
    getline (std::cin, mainMenu, '\n');
    acceptOnlyNumbers(mainMenu,selectMenu);

    if      (selectMenu == 1)
    {
        startPoll(pollQuestion, pollOption1, pollOption2, pollOption3);
        displayPoll(pollQuestion, pollOption1, pollOption2, pollOption3, pollVote, plusOne, plusTwo, plusThree);
    }

    else if (selectMenu == 2)
    {
        calculationFunction (result);
        std::cout << result << "\n";
    }
    else if (selectMenu == 3)
    {
        std::cout << "^_^\n";
        //will add another function later :)
    }

    else
    {
        std::cout << "Critical Error! The type of variable checking function is broken.\n";
    }


    std::cout << "The program will exit now.\n";
    return 0;
}



void startPoll(std::string &pollQuestion,
               std::string &pollOption1,
               std::string &pollOption2,
               std::string &pollOption3)
{
    std::cout << "Enter the poll question:\n";
        getline (std::cin, pollQuestion, '\n');

        std::cout << "\nEnter first poll option:\n";
        getline (std::cin, pollOption1, '\n');

        std::cout << "\nEnter second poll option:\n";
        getline (std::cin, pollOption2, '\n');

        std::cout << "\nEnter third poll option:\n";
        getline (std::cin, pollOption3, '\n');

        std::cout << "\n\n\n";
}

void displayPoll(std::string &pollQuestion,
                 std::string &pollOption1,
                 std::string &pollOption2,
                 std::string &pollOption3,
                 int &pollVote,
                 int &plusOne,
                 int &plusTwo,
                 int &plusThree)
{

    std::string pollVoteString;
    float votePercentage;
    int totalVotes = 0;

    std::cout << "=*=*=*=*=*=*=*=*=*=*=*=*=*=*=*=*=\n"
              << "         POLL OF THE DAY         \n"
              << "=*=*=*=*=*=*=*=*=*=*=*=*=*=*=*=*=\n\n"
              << pollQuestion << "\n"
              << "1. " << pollOption1 << "\n"
              << "2. " << pollOption2 << "\n"
              << "3. " << pollOption3 << "\n\n"
              << "Enter your vote: \n";
    getline (std::cin, pollVoteString, '\n');
    acceptOnlyNumbers(pollVoteString,pollVote);

    do
    {
        totalVotes++;
        if (pollVote == 1)
        {
            plusOne++;
        }
        else if (pollVote == 2)
        {
            plusTwo++;
        }
        else if (pollVote == 3)
        {
            plusThree++;
        }

        std::cout << "\nSuccessfully Voted.\nEnter 0 to terminate, or continue voting:\n";
        getline (std::cin, pollVoteString, '\n' );
        acceptOnlyNumbers(pollVoteString,pollVote);
    } while (pollVote != 0);

    std::cout << "====================***********====================\n"
              << "                      RESULTS                      \n"
              << "====================***********====================\n"
              << "Total Votes: " << totalVotes << "\n"
              << pollOption1 << ": " << plusOne << " votes.\n"
              << pollOption2 << ": " << plusTwo << " votes.\n"
              << pollOption3 << ": " << plusThree << " votes.\n\n\n";

    votePercentage = plusOne * 100 / totalVotes;
    std::cout << pollOption1 << ":\n";
    printGraphBar (votePercentage);

    votePercentage = plusTwo * 100 / totalVotes;
    std::cout << pollOption2 << ":\n";
    printGraphBar (votePercentage);

    votePercentage = plusThree * 100 / totalVotes;
    std::cout << pollOption3 << ":\n";
    printGraphBar (votePercentage);
}



void acceptOnlyNumbers (std::string &aString,int &number)
{

    do
    {
        std::stringstream verifyPoll(aString);
        if (verifyPoll >> number && verifyPoll.eof()) // To accept only numbers :)
        {
            break;
        }
        else
        {
            std::cout << "Invalid number, try again: \n";
            getline (std::cin, aString, '\n');
        }
    } while (!(number > 0 && number < 4));
}


void printGraphBar (float &votePercentage)
{
    int intPercentage = votePercentage;
    std::cout << "|";
    while (intPercentage != 0)
    {
        std::cout << "I";
        intPercentage--;
    }
    std::cout << "| " << votePercentage << "%\n\n";
}


void calculationFunction (double &result)
{
    double firstNumber, secondNumber;
    std::string logicalOperator;

    std::cout << "~-~-~-  CALCULATOR -~-~-~\n\n"
              << "Enter a number:\n";
    std::cin  >> firstNumber;
    std::cout << "Enter another number:\n";
    std::cin  >> secondNumber;
    std::cout << "Enter logical operator:\n";
    std::cin  >> logicalOperator;
    checkAndCalculate (logicalOperator, firstNumber, secondNumber, result);
}


void checkAndCalculate (std::string &logicalOperator,
                        double &firstNumber,
                        double &secondNumber,
                        double &result)
{
    if      (logicalOperator == "/")
    {
        result = firstNumber / secondNumber;
    }
    else if (logicalOperator == "*")
    {
        result = firstNumber * secondNumber;
    }
    else if (logicalOperator == "+")
    {
        result = firstNumber + secondNumber;
    }
    else if (logicalOperator == "-")
    {
        result = firstNumber - secondNumber;
    }
    else
    {
        std::cout << "The logical operator is not valid, try again.\n";
        std::cout << "Enter logical operator:\n";
        std::cin  >> logicalOperator;
        return checkAndCalculate (logicalOperator, firstNumber, secondNumber, result);
    }
}
\$\endgroup\$
  • \$\begingroup\$ do you know object oriented programming? \$\endgroup\$ – Emanuele Paolini Aug 17 '14 at 12:21
  • \$\begingroup\$ @EmanuelePaolini No, I don't. I'm almost done with C programming and now learning C++ \$\endgroup\$ – Amession Aug 17 '14 at 12:33
  • \$\begingroup\$ You may want to study other questions tagged with math-expression-eval, such as this one. \$\endgroup\$ – 200_success Aug 17 '14 at 14:29
3
\$\begingroup\$

As @EmanuelePaolini gave already some good advice let me go on from/add to this.

Separate independent parts into own files

You stated yourself that the code is rather big. While that it subjective, I find it big enough for it to be split up into several files. Doing so eases later reuse of the separated parts. This comes at the cost of managing "more complex" builds (which can be a nuisance for a beginner).

The usual layout is:

  • header files (*.hpp) for declarations
  • source files (*.cpp) for implementations

In your case there are three main tasks of your code: Polls, calculation and smiling. A good separation would therefore be

  • poll.hpp and poll.cpp
  • calculator.hpp and calculator.cpp
  • smile.hpp and smile.cpp (although there is arguably few to put into these files)

Reduce main

There is far too much logic in main. You should extract the whole body of main into an own function that can be reused and call that function from main.

Use switch

Your code for deciding what to do uses cascading ifs where a switch would be appropriate:

switch(selectMenu) {
case 1: //poll...
break;
case 2: //calculator ...
break;
case 3: //smile ...
break;
default: std::cout << "Invalid selection!";
break;
}

If you have to decide based on an integer value and each case is mutually exclusive, the switch is a more efficient and more readable solution than cascading ifs.

Use objects

Your code could really gain from using objects. Look at your function names: startPoll and displayPoll both contain the word Poll which is a hint that they are related to poll objects. So you should use this and represent a poll as what it is: An object.

First we need a class for that:

class Poll {
public:
    typedef std::vector<std::string> Options;

    Poll(std::string question, Options options);

    const std::string& getQuestion() const;
    const Options& getOptions() const;
private:
    std::string question;
    Options options;
};

This class holds all information for the poll without the results. We can construct a new Poll from a question and an arbitrary number of options. We can retrieve the question and options via getters. After a Poll object has been created we cannot change the question or the options.

For keeping track of the votes we can create a PollResult class:

class PollResult {
public:
    typedef std::vector<int> Votes;

    PollResult(const Poll &poll);

    void registerVoteFor(int option);

    const Poll &getPoll()const;

    const Votes& getVotes()const;

    int calculateNumberOfTotalVotes()const;
private:
    const Poll &poll;
    Votes votes;
};

This class has an associated poll and registers the votes for each option. We can get the associated poll to retrieve the question and option strings and we can get the votes per question as well as the number of total votes that have been cast. Based on these two classes we can update some of the existing functions' signatures:

Poll startPoll();

void display(const Poll &poll);

Single Responsibility Principle

While looking at the function's implementation I noticed that many of your functions do more than their name tells and have more than one responsibility.

displayPoll Does not just display the poll but also asks the user for votes and displays the results. We should break this apart into these three parts:

  • display poll
  • ask for votes
  • display poll results

acceptOnlyNumbers does not just parse for an integer (note the difference between the concept of integers and numbers!) but also makes sure the integer is in a given range. These are two responsibilities that should be broken apart.

checkAndCalculate already bears the multiple responsibilities in its name.

Use const and proper returning

Some of your functions return their results via reference parameters and have void given as their return type. This clutters your function signature unnecessarily. For example

void acceptOnlyNumbers (std::string &aString,int &number);

could become

int acceptOnlyNumbers (const std::string &aString);

Note the const of aString! If you don't change a parameter in a function and pass it by reference then declare it const to indicate to callers that these variables will not be changed! (This makes it easier to reason about (non existing) side-effects of functions). Using global variables like std::cout and std::cin also is a side effect.

This post has already gotten long enough so I will just paste reworked code here. If you have any questions about it feel free to ask. I have mainly worked on the Poll part.

main.cpp

#include <stdexcept>
#include <iostream>
#include <string>

#include "poll.hpp"
#include "pollresult.hpp"
#include "calculator.hpp"

void displayMenuAndExecute();

int main (void)
{
    displayMenuAndExecute();
    std::cout << "The program will exit now.\n";
    return 0;
}

void displayMenuAndExecute() {
    std::string mainMenu;
    int selectMenu;

    std::cout << "What would you like to do?\n" << "1. Create a poll\n"
            << "2. Start calculator\n" << "3. Smile\n";
    getline(std::cin, mainMenu, '\n');
    acceptOnlyNumbers(mainMenu, selectMenu);

    switch (selectMenu) {
    case 1: {
        Poll poll = startPoll();
        PollResult pollResult(poll);
        bool nonNullVote = true;
        do {
            try {
                nonNullVote = requestOneVoteFor(pollResult);
            } catch (const std::out_of_range &err) {
                std::cout << "Invalid vote. Votes must be integers 1 <= x <="
                        << poll.getOptions().size() << "\n";
        }
        }while(nonNullVote);
        display(pollResult);
    } break;
    case 2: {
        double result;
        calculationFunction(result);
        std::cout << result << "\n";
    }break;
    case 3:
        std::cout << "^_^\n";
        //will add another function later :)
        break;
    default:
        std::cout
                << "Critical Error! The type of variable checking function is broken.\n";
        break;
    }
}

poll.hpp

#ifndef POLL_HPP_
#define POLL_HPP_

#include <string>
#include <vector>

class Poll {
public:
    typedef std::vector<std::string> Options;

    Poll(std::string question, Options options);

    const std::string& getQuestion() const;
    const Options& getOptions() const;
private:
    std::string question;
    Options options;
};

Poll startPoll();

void display(const Poll &poll);

#endif /* POLL_HPP_ */

poll.cpp

#include "poll.hpp"

#include <iostream>

Poll startPoll()
{
    std::string question;
    std::cout << "Enter the poll question:\n";
    getline (std::cin, question, '\n');

    Poll::Options options;
    std::string currentOption;
    do {
        std::cout << "\nEnter " << options.size() + 1 << " poll option:\n";
        getline (std::cin, currentOption, '\n');
        if(!currentOption.empty())
            options.push_back(currentOption);
    } while(!currentOption.empty());
    return Poll(question, options);
}

Poll::Poll(std::string question, Options options)
: question(question), options(options)
{}

const std::string& Poll::getQuestion() const {
    return question;
}

const Poll::Options& Poll::getOptions() const {
    return options;
}

void display(const Poll &poll)
{
    std::cout << "=*=*=*=*=*=*=*=*=*=*=*=*=*=*=*=*=\n"
                  << "         POLL OF THE DAY         \n"
                  << "=*=*=*=*=*=*=*=*=*=*=*=*=*=*=*=*=\n\n"
                  << poll.getQuestion() << "\n";

    for(int i = 0; i < (int)poll.getOptions().size(); ++i) {
        std::cout << 1 + i << ". " << poll.getOptions().at(i) << "\n";
    }
    std::cout << "\n";
}

pollresult.hpp

#ifndef POLLRESULT_HPP_
#define POLLRESULT_HPP_

#include "poll.hpp"

class PollResult {
public:
    typedef std::vector<int> Votes;

    PollResult(const Poll &poll);

    void registerVoteFor(int option);

    const Poll &getPoll()const;

    const Votes& getVotes()const;

    int calculateNumberOfTotalVotes()const;
private:
    const Poll &poll;
    Votes votes;
};

bool requestOneVoteFor(PollResult &pollResult);

void display(const PollResult &pollResult);

void acceptOnlyNumbers(std::string &pollVoteString, int &pollVote);

void printGraphBar(float &votePercentage);

#endif /* POLLRESULT_HPP_ */

pollresult.cpp

#include "pollresult.hpp"

#include <iostream>
#include <numeric> // for std::accumulate
#include <sstream> // for stringstream and eof()

PollResult::PollResult(const Poll &poll)
: poll(poll), votes(poll.getOptions().size(), 0){}

void PollResult::registerVoteFor(int option) {
    ++votes.at(option);
}

const Poll &PollResult::getPoll()const {
    return poll;
}

const PollResult::Votes& PollResult::getVotes()const {
    return votes;
}

int PollResult::calculateNumberOfTotalVotes()const {
    return std::accumulate(votes.begin(), votes.end(), 0);
}

bool requestOneVoteFor(PollResult &pollResult) {
    display(pollResult.getPoll());

    std::string pollVoteString;
    std::cout << "Enter your vote: \n";
    getline (std::cin, pollVoteString, '\n');
    int pollVote;
    acceptOnlyNumbers(pollVoteString, pollVote);
    if(pollVote != 0) {
        pollResult.registerVoteFor(pollVote - 1);
        return true;
    }
    return false;
}

void display(const PollResult &pollResult) {
    int totalVotes = pollResult.calculateNumberOfTotalVotes();
    std::cout << "====================***********====================\n"
              << "                      RESULTS                      \n"
              << "====================***********====================\n"
              << "Total Votes: " << totalVotes << "\n";
    for(int i = 0; i < (int)pollResult.getPoll().getOptions().size(); ++i) {
        std::cout << pollResult.getPoll().getOptions().at(i) << ": "
                << pollResult.getVotes().at(i) << " votes.\n";
    }
    std::cout << "\n\n";

    for(int i = 0; i < (int)pollResult.getVotes().size(); ++i) {
        float votePercentage = pollResult.getVotes().at(i) * 100 / totalVotes;
        std::cout << pollResult.getPoll().getOptions().at(i) << ":\n";
        printGraphBar (votePercentage);
    }
}

void acceptOnlyNumbers (std::string &aString,int &number)
{
    do
    {
        std::stringstream verifyPoll(aString);
        if (verifyPoll >> number && verifyPoll.eof()) // To accept only numbers :)
        {
            break;
        }
        else
        {
            std::cout << "Invalid number, try again: \n";
            getline (std::cin, aString, '\n');
        }
    } while (!(number > 0 && number < 4));
}


void printGraphBar (float &votePercentage)
{
    int intPercentage = votePercentage;
    std::cout << "|";
    while (intPercentage != 0)
    {
        std::cout << "I";
        intPercentage--;
    }
    std::cout << "| " << votePercentage << "%\n\n";
}

calculator.hpp

#ifndef CALCULATOR_HPP_
#define CALCULATOR_HPP_

#include <string>

void calculationFunction(double &result);

void checkAndCalculate(std::string &logicalOperator, double &firstNumber,
        double &secondNumber, double &result);

#endif /* CALCULATOR_HPP_ */

calculator.cpp

#include "calculator.hpp"

#include <iostream>

void calculationFunction (double &result)
{
    double firstNumber, secondNumber;
    std::string logicalOperator;

    std::cout << "~-~-~-  CALCULATOR -~-~-~\n\n"
              << "Enter a number:\n";
    std::cin  >> firstNumber;
    std::cout << "Enter another number:\n";
    std::cin  >> secondNumber;
    std::cout << "Enter logical operator:\n";
    std::cin  >> logicalOperator;
    checkAndCalculate (logicalOperator, firstNumber, secondNumber, result);
}


void checkAndCalculate (std::string &logicalOperator,
                        double &firstNumber,
                        double &secondNumber,
                        double &result)
{
    if      (logicalOperator == "/")
    {
        result = firstNumber / secondNumber;
    }
    else if (logicalOperator == "*")
    {
        result = firstNumber * secondNumber;
    }
    else if (logicalOperator == "+")
    {
        result = firstNumber + secondNumber;
    }
    else if (logicalOperator == "-")
    {
        result = firstNumber - secondNumber;
    }
    else
    {
        std::cout << "The logical operator is not valid, try again.\n";
        std::cout << "Enter logical operator:\n";
        std::cin  >> logicalOperator;
        return checkAndCalculate (logicalOperator, firstNumber, secondNumber, result);
    }
}
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Thank you, it really helped me a lot. Declaring class is a new thing for me and I never thought we can break the programs in multiple parts like this but after reading carefully, I got it. I'll solve more problems to improve my skills :) And I appreciate your help, if you've FB please add me. \$\endgroup\$ – Amession Aug 19 '14 at 9:52
4
\$\begingroup\$
  1. Your code would greatly benefit from Object Oriented programming. Poll and PollOptions are good candidate to become a class.

  2. Having three options passed explicitly is ugly and it is difficult to maintain. Even if you are 100% sure you will always have three options, your could would benefit in treating them as a vector of strings:

    typedef std::vector<std::string> Options;
    

    so that you can iterate on them without repeating three times the same code. For two options this could be questionable, but starting from three I think there is no doubt about this. The same is true for the "plus" variables, they should be a single array.

  3. In this snippet:

        getline (std::cin, pollVoteString, '\n');
        acceptOnlyNumbers(pollVoteString,pollVote);
    

    you are treating in a different way the first answer (with explicit getline) and the following ones (inside the function acceptOnlyNumbers). You should modify the function to something like:

    int getNumber(std::string prompt);
    

    which takes care of printing the prompt, get the input, check it and repeat until a valid answer is given.

  4. Here:

    void checkAndCalculate (std::string &logicalOperator,
                            double &firstNumber,
                            double &secondNumber,
                            double &result)
    

    it seems more appropriate a signature like the following:

    double checkAndCalculate (std::string operator, 
                              double firstNumber,
                              double secondNumber);
    
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Thanks for your time and all the suggestions, and yes I'll modify that function right away. \$\endgroup\$ – Amession Aug 17 '14 at 12:38

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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