0
\$\begingroup\$

Task:

Design a vending machine which does the following:

Accepts coins of 1, 5, 10, 25, 50 cents or notes of 1 and 2

User selections: Candy, snacks, nuts, Coke, Pepsi and soda

Allow user by cancelling the request, returns the product and change, allow reset for the supplier.

I am supposed to use the best of memory allocation and maximum usage of pointers.

#include <iostream>
using namespace std;
class Vending_machine
{
    public : const int  PRICE_OF_CANDY = 10, 
                        PRICE_OF_SNACK = 50, 
                        PRICE_OF_NUTS = 90, 
                        PRICE_OF_COKE = 25,
                        PRICE_OF_SODA = 45,
                        MAX_STOCK = 10,
                        VALUE_OF_A_NOTE = 100;

    public: int *Amount_Deposited = new int,
                *Remaining_candy = new int,
                *Remaining_snack = new int,
                *Remaining_nuts = new int ,
                *Remaining_coke = new int,
                *Remaining_soda = new int;

    Vending_machine()
        {   
            *Amount_Deposited =  0;
        }
    Vending_machine(int reset)
        {
            if (reset == 1)
                {
                    *Amount_Deposited = 0,
                    *Remaining_candy = *Remaining_snack = *Remaining_nuts = *Remaining_coke = *Remaining_soda = MAX_STOCK;

                }
        }
    ~Vending_machine()
        {
            cout<<"\n Candy left :"<<*Remaining_candy<<"\n Snack left :"<<*Remaining_snack<<"\n Nuts left :"<<*Remaining_nuts<<"\n Coke Left :"<<*Remaining_coke
                <<"\n Soda Left : "<<*Remaining_soda;
            delete Amount_Deposited, Remaining_candy, Remaining_snack, Remaining_nuts, Remaining_coke, Remaining_soda;
        }

    public: void get_coin(int value)
        {
            if (value == 1 or value == 5 or value == 10 or value == 25 or value == 50)
                {
                    *Amount_Deposited =+ value;
                }
            else
                {
                    cout<<"\n Incorrect Denomination";
                }
        }

    public: void get_notes(int value)
        {
            if (value == 1 or value == 2)
                {
                    *Amount_Deposited =+ value * VALUE_OF_A_NOTE;
                }
            else
                {
                    cout<<"\n Incorrect Denomination";
                }
        }

    public: void deduct_money(int cost)
        {
            *Amount_Deposited = *Amount_Deposited - cost;
        }

    public: void error_messages(int error_int)
        {
            if  (error_int == 1)
                {
                    cout<<"Out of Stock";
                }
            else if(error_int == 2)
                {
                    cout<<"Please deposit more money";
                }
        }

    public: void which_product(int selection)
        {
            switch(selection)
                {
                    case(1):
                        if (*Remaining_candy !=0 && *Amount_Deposited >= PRICE_OF_CANDY)
                            {
                                *Remaining_candy = *Remaining_candy - 1;
                                deduct_money(PRICE_OF_CANDY);
                                cout << "\n Candy dispensed";
                            }
                        else 
                            {
                                if (*Amount_Deposited < PRICE_OF_CANDY)
                                    {
                                        error_messages(2);
                                    }
                                else if(*Remaining_candy == 0)
                                    {
                                        error_messages(1);
                                    }
                            }
                    break;
                    case(2):
                        if (*Remaining_snack !=0 && *Amount_Deposited >= PRICE_OF_SNACK)
                            {
                                *Remaining_snack = *Remaining_snack - 1;
                                deduct_money(PRICE_OF_SNACK);
                                cout << "\n Snack dispensed";
                            }
                        else 
                            {
                                if (*Amount_Deposited < PRICE_OF_SNACK)
                                    {
                                        error_messages(2);
                                    }
                                else if(*Remaining_snack == 0)
                                    {
                                        error_messages(1);
                                    }
                            }
                    break;
                    case(3):
                        if (*Remaining_nuts !=0 && *Amount_Deposited >= PRICE_OF_NUTS)
                            {
                                *Remaining_nuts = *Remaining_nuts - 1;
                                deduct_money(PRICE_OF_NUTS);
                                cout << "\n Nuts dispensed";
                            }
                        else 
                            {
                                if (*Amount_Deposited < PRICE_OF_NUTS)
                                    {
                                        error_messages(2);
                                    }
                                else if(*Remaining_nuts == 0)
                                    {
                                        error_messages(1);
                                    }
                            }
                    break;
                    case(4):
                        if (*Remaining_coke !=0  && *Amount_Deposited >= PRICE_OF_COKE)
                            {
                                *Remaining_coke = *Remaining_coke - 1;
                                deduct_money(PRICE_OF_COKE);
                                cout << "\n Coke dispensed";
                            }
                        else 
                            {
                                if (*Amount_Deposited < PRICE_OF_COKE)
                                    {
                                        error_messages(2);
                                    }
                                else if(*Remaining_nuts == 0)
                                    {
                                        error_messages(1);
                                    }
                            }
                    break;
                    case(5):
                        if (*Remaining_soda !=0 && *Amount_Deposited >= PRICE_OF_SODA)
                            {
                                *Remaining_soda = *Remaining_soda - 1;
                                deduct_money(PRICE_OF_SODA);
                                cout << "\n Soda dispensed";
                            }
                    else 
                            {
                                if (*Amount_Deposited < PRICE_OF_SODA)
                                    {
                                        error_messages(2);
                                    }
                                else if(*Remaining_soda == 0)
                                    {
                                        error_messages(1);
                                    }
                            }
                    break;
                    default :
                        cout << "\n Invalid Selection";
                }
            return_money('Y');
        }

    public :void return_money(char choice)
        {
            if (*Amount_Deposited != 0)
                { 
                    if (choice == 'Y')
                        {
                            cout<<"\n Your remaining change is : " << *Amount_Deposited/100.0 <<" $ \n";
                            deduct_money(*Amount_Deposited);
                        }
                }
        }

};

int main() 
    {
    Vending_machine *a = new Vending_machine(1); 
    int i = 1, val, product_what;
    char currency, select;
    while(i<12)
    {
        RESETED:
        cout<<"\n Enter Notes(N),Coin(C) and Value :";
        cin>>currency>>val;
        if (currency == 'N')
            {
                a->get_notes(val);
            }
        else if(currency == 'C')
            {
                a->get_coin(val);
            }
        else if(currency == 'R' && val == 0)
            {
                delete a;
                Vending_machine *a = new Vending_machine(1); 
                goto RESETED;
            }
        else if(currency == 'E' && val == 0)
            {
                goto END_LOOP;
            }
        else 
        {
            cout<<"\n Wrong denomination !";
            goto END_IT;
        }
        cout<<"\n Do you want to undo : (Y/N) :";
        cin>>select;
            if (select!= 'N')
                {
                    a->return_money(select);
                    goto END_IT;
                }
        cout<<"\n Enter 1/2/3/4/5 for product :";
        cin>>product_what;
        a->which_product(product_what);
    END_IT:
        {
        }
    }
    END_LOOP:
        {

        }
}
\$\endgroup\$
  • \$\begingroup\$ Are you really supposed to use "maximum pointers"? To write reliable programs, using "minimal pointers" is better. \$\endgroup\$ – Roland Illig Dec 2 '17 at 18:27
  • \$\begingroup\$ so the reason why to use maximum pointers is that i am supposed to practice on understanding where i might have to deal with bigger data. \$\endgroup\$ – Ram Subramaniam Dec 2 '17 at 18:31
  • 2
    \$\begingroup\$ The code seems to be broken, instead of += the code contains =+. Instead of || the code contains or. It is best never to use a goto in C++, goto is probably maintained in the language because the C programming language is allowed. The code really doesn't appear to be C++, perhaps more like Java or C#. The comma , operator in the delete line should not be allowed. \$\endgroup\$ – pacmaninbw Dec 2 '17 at 21:02
  • 2
    \$\begingroup\$ When you require a goto, it breaks your program flow. All of a sudden you're writing something resembling assembly instead of a high-level programming language. In larger programs, it will bite you. \$\endgroup\$ – Mast Dec 2 '17 at 21:13
  • 1
    \$\begingroup\$ Such magic would be achieved using if/else statements and return values, yes. \$\endgroup\$ – Mast Dec 2 '17 at 21:20
3
\$\begingroup\$

There is a long list of things to be improved here, but please do not get discouraged:

  1. Don't use using namespace std; because it can introduce subtle bugs. Writing std:: will probably not kill you, so keep your hands away from using namespace std;.
  2. One public is enough. You don't need to prefix all your members with this, write it once and then just declare all members. This blunder is commonly made by people coming from Java or related languages, but in C++ public, private and apply to all following declarations until a different access specifier is encountered.
  3. The constructor Vending_machine(int reset) should take a bool instead of an int as a parameter, because you are passing whether to reset or not. Whether-relations are almost always expressed through bool.

  4. This

    *Amount_Deposited = 0,
    *Remaining_candy = *Remaining_snack = *Remaining_nuts = *Remaining_coke = *Remaining_soda = MAX_STOCK;
    

    is a horrible piece of code. Try to reduce the things you do on a single line. These should be five separate assignments on five separate lines. Also, don't use the comma operator like that; the comma operator should only be used when it is required, which it absolutely is not here.

  5. Kind of hitting the same spot as point 4, but delete Amount_Deposited, Remaining_candy, Remaining_snack, Remaining_nuts, Remaining_coke, Remaining_soda; does not do what you think it does. To be precise, it calls delete for Amount_Deposited, and does nothing else with the other values. Usually, you should compile with warnings enabled so your compiler can tell you about such things. Again, don't (ab)use the comma operator!

  6. or is extremely uncommon in C++ and, as far as I know, supposed to be a replacement for || on compilers which don't treat special characters very well. Although your usage is not wrong, you should prefer to use || instead or people might look at you weirdly.

  7. get_coin could be implemented nicer with a switch statement. Instead of

    if (value == 1 or value == 5 or value == 10 or value == 25 or value == 50)
            {
                *Amount_Deposited =+ value;
            }
    else
            {
                cout<<"\n Incorrect Denomination";
            }
    

    , this could be much nicer implemented as

    switch (value) {
        case 1:
        case 5:
        case 10:
        case 25:
        case 50:
            *Amount_Deposited += value;
            break;
        default:
            std::cout << "\n Incorrect Denomination!";
    }
    

    which, to my mind, is much better readable.

  8. Look at the two code blocks in the point before, to the line where the assignment happens, to be precise. Do you notice something? Right. While you wrote *Amount_Deposited =+ value;, I corrected it to *Amount_Deposited += value;. As you can see, the order of + and = is different. Accordingly, the two lines do two very different things: You line assigns the unary plus of value (which is just value) to the number at Amount_Deposited while the corrected version adds value to it. The order in compound assignment operators matters!

  9. int error_int is a little like the C way of doing errors, except worse because you don't define any error macros. What does it mean if I pass 1 to error_messages, or 2, or -45633421? If I take a look at the function body, I can guess that 1 stands for "Out of Stock" and 2 for "Incorrect Denomination", but how am I supposed to know that if I don't have the function body available? C++11 introduced enum class, which looks like it's made exactly for this use case. If you don't have access to C++11 and beyond, there is still the plain old enum, which has its drawbacks but is still a huge improvement over a magic-number-int parameter.

  10. Don't use goto. Never. goto is bad. Why is it bad? Because it leads to what is called Spaghetti code, a state in which it is nearly impossible to follow program execution and reason about the code, which also applies to the compiler.

  11. There is no reason here to have anything on the heap. Those five ints you allocate would work just as well, probably even better as local parameters. Don't use the heap unless you have a good reason to; it's slow, it leads to memory leaks and all other pesky kinds of problems (such as use-after-free) and it makes you handle pointers which is a little more convoluted than usually necessary. Point 5 should have made you realize the dangers of dynamic memory already.

  12. Reduce your class to a single responsibility. In particular, have your class do either calculations or I/O. This is called the Single Responsibility Principle (who'd have thought) and is one of the fundamentals of object oriented software design.


There is more to review in your code, but this answer is becoming more and more of a wall of text, and you'll have a lot to do understanding my points and the reasoning behind them and realizing them when reworking your code.

As you may have realized already, your code is not exactly top-of-class (which is totally okay; changing that is the reason I suppose you're here). In particular, it doesn't even work as expected, which is actually a disqualifying criterion for CodeReview, but I'll turn the other cheek that this time since you didn't seem to be aware of this and didn't mention it at all.

My suggestion is this: Take your time, work through this answer, ask if you don't understand anything, and, when you're finished implementing all suggestions, post a follow-up question with your reworked code. I'll be happy to do another iteration on your program.

\$\endgroup\$
  • \$\begingroup\$ You've missed a few new comments. \$\endgroup\$ – pacmaninbw Dec 2 '17 at 21:23
  • \$\begingroup\$ @pacmaninbw It seems I did. I don't really see how any of the comments would have changed my answer, though (apart from the point on goto, maybe, which has become kind of obsolete). \$\endgroup\$ – Ben Steffan Dec 2 '17 at 21:41
  • 1
    \$\begingroup\$ @BenSteffan i am rewriting my code and would put it up ASAP. \$\endgroup\$ – Ram Subramaniam Dec 2 '17 at 21:44
  • 1
    \$\begingroup\$ @pacmaninbw Shouldn't that read "can't change"? \$\endgroup\$ – Ben Steffan Dec 2 '17 at 23:12
  • 1
    \$\begingroup\$ @pacmaninbw sure i'll post it as a new question and share the link here \$\endgroup\$ – Ram Subramaniam Dec 3 '17 at 7:55

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