3
\$\begingroup\$

I'm trying to make a simple app to help me store data about food to use this database in a future diet application. What can I do to improve the code? What part of code should I make a class etc.?

    #include <iostream>
    #include <vector>
    #include <string>
    #include <map>
    #include <iterator>


    using namespace std;

    struct food
    {
        int mass;
        double kcal;
        double prot;
        double carb;
        double fat;
    };

    map<string, food> foodMap_prot;
    map<string, food> foodMap_carb;
    map<string, food> foodMap_fat;
    int main()
    {

        int mass;
        double kcal;
        double prot;
        double carb;
        double fat;
        string name;
        cout << "Enter a name for a product: ";
        getline(cin,name,'\n');
        cout << "\nmass of [" << name << "]: ";
        cin >> mass;
        cout << "\ncalories of [" << name << "]: ";
        cin >> kcal;
        cout << "\nprotein in [" << name << "]: ";
        cin >> prot;
        cout << "\ncarbs in [" << name << "]: ";
        cin >> carb;
        cout << "\nfat in [" << name << "]: ";
        cin >> fat;
        string kategory;
        double factor;
        kcal /= 100;
        prot /=100;
        carb /=100;
        fat /=100;
        food temp1 = {mass, mass*kcal, mass*prot, mass*carb, mass*fat};
        map<string, food>::iterator itr, koniec;
        if(prot>carb && prot>fat)
        {
            factor = prot*100/7;
            mass = mass/factor;
            temp1 = {mass, mass*kcal, mass*prot, mass*carb, mass*fat};
            foodMap_prot[name] = temp1;
            itr = foodMap_prot.find(name);
            koniec = foodMap_prot.end();
            kategory = "Protein";
}
else if(carb>prot&&carb>fat)
{
    factor = carb*100/9;
    mass = mass/factor;
    temp1 = {mass, mass*kcal, mass*prot, mass*carb, mass*fat};
    foodMap_carb[name] = temp1;
    itr = foodMap_carb.find(name);
    koniec = foodMap_carb.end();
    kategory = "Carbs";
}
else if(fat>prot&&fat>carb)
{
    factor = fat*100/1.5;
    mass = mass/factor;
    temp1 = {mass, mass*kcal, mass*prot, mass*carb, mass*fat};
    foodMap_fat[name] = temp1;
    itr = foodMap_fat.find(name);
    koniec = foodMap_fat.end();
    kategory = "Fat";
}

for(;itr!=koniec; ++itr)
{
    cout <<"\n\nyou added a new product \t[" << name <<"]";
    cout <<"\n\ncategory: \t\t\t" << kategory;
    cout <<"\n\nmass of one block of [" << name << "]: \t" << itr->second.mass<<"g";
    cout <<"\n\ncalories in [" << name << "]: \t" << itr->second.kcal<<" kcal";
    cout << "\n\nprotein [" << name << "]: \t"<< itr->second.prot<<"g";
    cout <<"\n\ncarbs [" << name << "]: \t" << itr->second.carb<<"g";
    cout << "\n\nfat [" << name << "]: \t" << itr->second.fat<<"g";
}

    }
\$\endgroup\$
3
\$\begingroup\$

Namespace std

Stop doing this:

using namespace std;

See every other C++ code review and Why is “using namespace std;” considered bad practice?

Monolithic Functions

Break your code into multiple smaller functions. A good rule of thumb is no more than a dozen lines. If you give your function meaningful names then your code becomes self documenting.

Global Variables

Global variables are a bad idea. It makes testing hard. It makes debugging hard. You don't the result of the function biased on its inputs (it also depends on some other external global state that other functions may mutate while you are not watching.

Pass parameters to functions that should be the only state the worry about.

Objects

This looks like it should be an object:

map<string, food> foodMap_prot;
map<string, food> foodMap_carb;
map<string, food> foodMap_fat;

How about

class FoodMap
{
    typedef  std::map<std::string, Food>  FoodMap;

    FoodMap   protein;     // avoid the temptation to shorten names
    FoodMap   carbs;       // unless like this they are hard to spell.
    FoodMap   fat;
};

// Note on naming conventions.
// User defined types (usually) start with an Uppercase letters.
// Object usually start with a lowercase letter.
// This makes it easy to spot types over objects.

Looks like all the code in your main is designed to correctly fill up the map. So this should really be part of the FoodMap class. The class should be the only thing defining where things go internally. If you let functions (like main) fill up the map you have no control to validate input.

\$\endgroup\$
  • \$\begingroup\$ Looks like protein is also hard to spell (sorry couldn't resist). \$\endgroup\$ – vnp Apr 1 '15 at 16:35
  • \$\begingroup\$ I see this comment and this SO question each time using namespace std; is found in code. But I do not see any compelling reason not to do that. Nor had I any issues with this kind of code in my experience. When one have a conflict, one just disambiguates in the place of conflict, not cluttering whole file with this std:: thing. \$\endgroup\$ – GeniusIsme Apr 2 '15 at 11:54
  • \$\begingroup\$ @GeniusIsme: Then you have not read the top two answers to the question. Specifically read this to see how it can silently break your code. In short. This is never allowed in real production code (if you see then that's not a team you want to work with). Don't get into the habit of doing in your private code. \$\endgroup\$ – Martin York Apr 11 '15 at 2:48
  • \$\begingroup\$ Thank you for reply. But it doesn't really add anything to what i've already read on this topic. Referenced problems occured because fuctions were applyed to wrong objects, not beacause of using namespace std. Writing std:: everywhere is like wearing a helmet everyday: you can always find accidents in which helmet would save people, but you still will not wear it. \$\endgroup\$ – GeniusIsme Apr 12 '15 at 7:47
  • \$\begingroup\$ @GeniusIsme: I advise you to wear the helmet. I have not come across a professional programming environment were it is not required. \$\endgroup\$ – Martin York Apr 12 '15 at 13:59

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.