8
\$\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,

Note : I understand the need of using std::cin/std::cout etc,. Just for simplicity I have not made this one particular change.

I will start making this change with my future codes.

Also, if you can please rate this code out of 10 for usage of design principles and where all I can apply them and which one.

Also, I have been asked to use pointers and memory management.

#include <iostream>
    using namespace std;

    class product
        {
            public :
            const int MAX_STOCK = 10;

            string *name = new string;
            int *price = new int,
                *remaining_stock = new int;

            product(string n, int p)
                {
                    *price = p;
                    *name = *name + n;
                    *remaining_stock = MAX_STOCK;
                }
            ~product()
                {
                    delete price;
                    delete name;
                    delete remaining_stock;
                }
        };

Class for money :

  class money
        {
            public :

            int *max = new int;
            int *accepted_values = new int[10];
            int i;
            money()
                {
                }
            money(int *a, int how_many)
                {
                    *max = how_many;
                    //cout<<"\n this is max now : "<<*max;
                        for (i=0; i<how_many; i++)
                            {
                                *(accepted_values + i) = a[i];
                                //cout<<"\n inside money : "<<i<<" accepted value is "<<*(accepted_values + i)<<" value is a is :"<<a[i]<<endl;
                            }           
                }
            int check(int value, int loop_max)
                {
                    //cout<<"\nInside money checker, the value of max is :"<<loop_max;
                    bool present = 0;
                    //cout<<"\nInside money checker, the value of present is :"<<present;
                    for(i=0; i< loop_max; i++)
                        {
                            //cout<<"\n Accepted values - "<<i<<" is : "<<*(accepted_values + i)<<" value is :"<<value;
                            if (value == *(accepted_values + i))
                            {
                                //cout<<"\n value present!";
                                present = 1;
                            }
                        }
                    if(!present)
                        {
                            //cout<<"\n not present and present is now : "<<present;
                            value = 0;
                        }
                    return value;
                }
            ~money()
                {
                    delete max;
                    delete accepted_values;
                }
        };

Class vending machine :

    class Vending_machine
        {
            public :

            product *candy = new product("Candy",10);
            product *snack = new product("Snack",50);
            product *nuts = new product("Nuts",90);
            product *coke = new product("Coke",25);
            product *soda = new product("Soda",45);

            int accepted_coins[5] = {1,5,10,25,50};
            int accepted_notes[2] = {1,2};

            money *coins = new money(accepted_coins, 5);
            money *notes = new money(accepted_notes, 2);

            int *amount_deposited = new int;

            Vending_machine()
                {
                    *amount_deposited = 0;
                }
            Vending_machine(bool reset)
                {   
                    if (reset == true)
                        {
                            *amount_deposited = 0;
                            //delete candy;
                            //delete snack;
                            //delete nuts;
                            //delete coke;
                            //delete soda;
                            //delete coins;
                            //delete notes;

                            product *candy = new product("Candy",10);
                            product *snack = new product("Snack",50);
                            product *nuts = new product("Nuts",90);
                            product *coke = new product("Coke",25);
                            product *soda = new product("Soda",45);
                            }
                }
            ~Vending_machine()
                {
                    delete candy;
                    delete snack;
                    delete nuts;
                    delete coke;
                    delete soda;
                    delete coins;
                    delete notes;
                    delete amount_deposited;
                }
            void get_money(int choice,int value)
                {
                    switch(choice)
                        {
                            case(1):
                                //cout<<"\nthis is the value of amount deposited before : "<<*amount_deposited;
                                *amount_deposited = coins->check(value, *coins->max);
                                //cout<<"\nthis is the value of amount deposited after : "<<*amount_deposited;
                            break;
                            case(2):
                                //cout<<"\nthis is the value of amount deposited before : "<<*amount_deposited;
                                *amount_deposited = notes->check(value, *notes->max) * 100;
                                //cout<<"\nthis is the value of amount deposited after : "<<*amount_deposited;
                            break;
                            default:
                                *amount_deposited = 0;
                        }
                }
            void compare_availability(int *remain, int *price)
                {
                    try
                    {
                        if(*remain !=0 && *amount_deposited >= *price)
                            {
                                *remain -= 1;;
                                *amount_deposited -= *price;
                                cout<<"\n Dispatched";
                                return_money('Y');
                            }
                        else if (*amount_deposited < *price)
                            {
                                throw "Amount deposited is low";
                            }
                        else if (*remain == 0)
                            {
                                throw "Stock not available";
                            }
                        else
                            {
                                throw "Unknown error";
                            }
                    }catch (const char* msg)
                        {
                            cout<<msg<<endl;
                        }
                }
            void get_selection(int selection)
                {
                    switch(selection)
                        {
                            case (1):
                                compare_availability(candy->remaining_stock, candy->price);
                            break;
                            case (2):
                                compare_availability(snack->remaining_stock, snack->price);
                            break;
                            case (3):
                                compare_availability(nuts->remaining_stock, nuts->price);
                            break;
                            case (4):
                                compare_availability(coke->remaining_stock, coke->price);
                            break;
                            case (5):
                                compare_availability(soda->remaining_stock, soda->price);
                            break;
                            default:
                                cout<<"Wrong selection !"<<endl;
                        }
                }
            void return_money(char choice)
            {
                if (*amount_deposited != 0)
                    { 
                        if (choice == 'Y' || choice == 'y')
                            {
                                cout<<"\n Your remaining change is : " << *amount_deposited/100.0 <<" $ \n";
                                *amount_deposited = 0;
                            }
                    }
            }
        };

main :

    int main() 
        {
            int main_select, main_coin_note, main_val;
            char change_your_mind;
            Vending_machine *a = new Vending_machine(1);
            cout<<"Coin(1) or Note(2) : ";
            cin>>main_coin_note;
            cout<<"Enter your denomination :";
            cin>>main_val;
            a->get_money(main_coin_note, main_val);

            if (*a->amount_deposited == 0)
                {
                    cout<<"\n Wrong denomination, Value returned !";
                }
            else
                {
                    cout<<"\tEnter what product you want :"<<endl<<"\t\t1 for Candy"<<endl<<"\t\t2 for Snack"<<endl<<"\t\t3 for Nuts"<<endl<<"\t\t4 for Coke"<<endl<<"\t\t5 for Soda\n\t :..";
                    cin>>main_select;
                    cout<<"\n You want to cancel request?(Y/N) :";
                    cin>>change_your_mind;
                        if (change_your_mind == 'Y' || change_your_mind == 'y')
                            {
                                a->return_money(change_your_mind);
                            }
                        else
                            {
                                a->get_selection(main_select);
                            }
                }
        }
\$\endgroup\$
12
\$\begingroup\$

You asked for a rating of your code in regards to application of design principles. I'd give you a 1/10, and here's why:

  1. You have different classes with different responsibilities, which is a good thing. However, your class Vending_machine still does I/O which it shouldn't as that isn't its responsibility. Also, while the idea of having a class money is good, you barely make use of it at all, which is missing the point of having such a class to begin with.

  2. Commenting out single code lines is something we all do, but those lines should be removed from finished code as they are not useful comments in any sense. You current code invokes a very "hacky" feeling, which is not good.

  3. Since I'm lazy, I won't repeat all the things that I've written about your code before. Since you seem to have ignored almost all of my previous suggestions, please go back to my answer to the precursor of this question and work through the points again. In particular, please read my point about new and dynamic memory again. Your current use of the operator resembles Java a lot and is very much unfitting for C++.

  4. Please rethink your indentation scheme. While there is no consensus on a one-fits-all indentation model, and every answer about this topic is always to a certain degree opinionated, I really find your current indentation to be to much to keep your code in a well readable format. For example, why do you indent the curly braces after a function head and then indent the body another level? Why are the actual code statements in your switch-cases indented by 4 levels relative to the switch? What if you had a switch-case inside a switch case? That's a whole 8 levels of indentation. With each level of indentation being 4 characters, that would be a whole 32 characters lost, which is more than 1/3 of the standard line width of 80 chars! In the end, those things are up to your own liking, but as someone who has to read your code, I beg you to please, please choose a style that doesn't create more horizontal white space than actual code.

  5. Why is every variable outside of main a member variable? What use does i in money have that would justify keeping its value between function calls? None. Make it a local variable, preferably local to your for loops.

  6. void get_selection(int selection) constitutes an anti-pattern in itself. Almost always, get methods represent getters, a getter being a method that returns a member value of a class. However, get_selection returns void.

  7. Adding to point 6, you don't seem to have a firm grasp on the concept of returning values. One key to the Single Responsibility Principle and to good object oriented code is having objects offer a good interface, where the term good interface excludes the "just print it to stdout"-pattern. An important part of this interface are the so-called getter and setter methods. Getter methods, as the name implies, are responsible for "getting", i.e. returning a value contained in your object while setter, well, set the value of a member variable. If these concepts sound unfamiliar to you, you should absolutely read up on them as they are one of the cornerstones of OOP.


Although this answer is not what I would call short, many important points are missing from it because I didn't want just copy-and-paste my answer to your precursor question. As I said in point 3, please go back, read my old answer, correct those things, read this answer, correct these things and then post a follow-up question (only if you want to, of course) since there is still more to be said about your code.

As you could probably guess from my rating and this answer in general, your code is currently pretty far away from what I would call good C++. However, all's not lost; if you continue improving your code, I'm sure you'll arrive at something decent in a few days. Don't become discourage; writing (good) C++ is very hard!

\$\endgroup\$
  • \$\begingroup\$ regarding your point of having only calculations or only i/o i really thought about but at some places doesn't it become redundant to pass a value and then again write another piece of code to get that value? and also if i make a change to the first code which returns a value, lets say int at first and now it returns a float, now i would have to just make change in this part of the code so the other won't get affected. and i understand your other points. this one and the single responsibility principle seems like might end up making me make more changes than actually required. \$\endgroup\$ – Ram Subramaniam Dec 3 '17 at 17:34
  • 1
    \$\begingroup\$ @RamSubramaniam No. That is the whole point of good software design. Getters and setters are the bread and butter of OOP, and encapsulation is the only way to keep large programs sane. You don't change the type of parameters easily; once you define an interface, that interface should only change destructively when absolutely needed (i.e. adding things is okay, everything else is not). If you need your vending machine to work with multiple types, you should make it a template (this is totally not required here, though). \$\endgroup\$ – Ben Steffan Dec 3 '17 at 17:41
  • \$\begingroup\$ oh and encapsulation is something which i still didnt completely understand. and what do you mean by a template? we could change the variable types based during run time? \$\endgroup\$ – Ram Subramaniam Dec 3 '17 at 18:05
  • \$\begingroup\$ @RamSubramaniam No. You should learn C++ properly. Templates are a language feature of C++ that allow generics at compile time. Encapsulation just means hiding away your data members and everything else from public access and defining access in terms of methods. \$\endgroup\$ – Ben Steffan Dec 3 '17 at 18:08
  • 2
    \$\begingroup\$ Getters and setters ARE NOT the bread and butter of OOP! The presence of a large number of setters or getters indicate that you have not abstracted your problem in a good way out that what you have is actually a storage structure which could just as well be a struct. What difference is there between a public member and a private with get and set methods? It's not encapsulation just because you have a set and get.. \$\endgroup\$ – Emily L. Dec 4 '17 at 0:30
6
\$\begingroup\$

This is a distinct improvement over your first attempt. Keep up the good work.

Note : i understand the need of using std::cin/std::cout etc,. Just for simplicity i have not made this one particular change. Will start making this change with my future codes.

No, you need to practice using std:: so that it becomes a habit, it is a good habit to have.

Put Statements on a Separate Lines
The following code is a very bad habit:

    int *price = new int,
        *remaining_stock = new int;

It makes the code harder to maintain and other people who have to maintain the code may miss the comma. It is a much better habit to put each initialization on a separate line in a separate statement:

    int *price = new int;
    int *remaining_stock = new int;

Move the Memory Allocation Into the Constructor
In each of the classes the memory allocation occurs in the class itself prior to the constructor. In C++ it is a much better habit to have all the memory allocation in the constructor and no memory allocation in the class itself. Actually all initialization should take place in the constructor except for class static variables.

One of the reasons for this is that someone else that is going to maintain the code isn't going to be looking for the initialization in the class, they are going to be looking for it in the constructor.

Where I have seen this type of initialization before is when references are being created in C# classes, this might be true in Java also.

Another reason is that in C++ many people use header files (#include) to define the classes, but the functions themselves are in a CPP file

Example:

product.hpp:

class product
{
    public :
    const int MAX_STOCK = 10;

    std::string *name;
    int *price;
    int *remaining_stock;

    product(string n, int p);
    ~product();
};

product.cpp:

product::product(string n, int p)
{       
    name = new string(n);
    price = new int;
    remaining_stock = new int;

    *price = p;
    *remaining_stock = MAX_STOCK;
}       

product::~product()
{       
    delete price;
    delete name;
    delete remaining_stock;
}       

It's ok to have executable code in header files, especailly code that the compiler might optimize by making it inline, but for more complex code it might be easier to maintain in separate files. It also enhances the hiding of the implementation from the user.

Another reason to have executable code in CPP files is that you don't want to force the user of the code to recompile every time a bug is fixed or code is changed. Putting just the function declarations and not the function definitions in the header file means that unless an function interface changes the user doesn't have to recompile their code, they may just have to re-link. Even re-linking isn't necessary if the code is in a dynamic library.

Make the code More Readable
It's generally more acceptable to separate operators such as << and >> by spaces.

    std::cout<<"Wrong selection !"<<std::endl;

is less readable than

    std::cout << "Wrong selection !" << std::endl;

The << and >> are operators just like -= - += and +;

\$\endgroup\$
  • \$\begingroup\$ i'll work on all these changes. The only reason why i didn't use std:: is the fact that , i didnt exactly know where all it applies and i'll have to read up more on them. \$\endgroup\$ – Ram Subramaniam Dec 3 '17 at 17:39
  • \$\begingroup\$ Actually if you remove `using namespace std; the compiler will tell you where std:: needs to be. \$\endgroup\$ – pacmaninbw Dec 3 '17 at 17:56
  • \$\begingroup\$ right, but during running code it becomes a bit difficult. but i'll try to start implementing it. \$\endgroup\$ – Ram Subramaniam Dec 3 '17 at 18:03
  • \$\begingroup\$ @packmaninbw i have added another code for the same do review if you have the time codereview.stackexchange.com/questions/181996/… \$\endgroup\$ – Ram Subramaniam Dec 4 '17 at 18:04

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.