6
\$\begingroup\$

I was given this assignment below and wrote the code below that as the solution. My instructor has been blasting me for writing inefficient code for my last several assignments, and rails at me for using strings as I do. She won't show me why its wrong but just states that it is wrong. I need to know why what I am writing here is so wrong and why. Where am I being so inefficient? I am also being blasted for the use of too many strings, what is wrong with using strings and why?

The Assignment:

Design a program that will determine based on the temperature and amount of rain whether or not the sprinklers should be turned on.

Note: The inputs and outputs below are for the specified data. When I test the program you will be asked to type in different values during program execution.

  • There are 3 regions in the Garden: F-Flowers V-Vegetable B-Berries. Report an INVALID garden choice if F, V or B is not entered for the garden choice.
  • If the temperature is above 40 degrees then based on the precipitation it can be watered. The Flower Garden it should be watered when the precipitation is less than 0.15 inches. The Vegetable Garden should be watered when less than 0.375 inches and the Berries should be watered when there is less than 0.425 inches. It should state which garden is being watered and or not.

Example program Outputs

Garden Sprinkler System

What is the temperature in degrees(F)? 78                             
How much precipitation today(in inches)?.1                            
The Garden Divisions are F-Flowers  V-Vegetable B-Berries             
Which do you choose (FVB)? B

Given the temperature is 78 degrees and 0.100 inches of precipitation
today.  The Berries in the Garden will be watered.       

OR

Garden Sprinkler System

What is the temperature in degrees(F) ?32                             
How much precipitation today(in inches)?0.2                           
The Garden Divisions are F-Flowers  V-Vegetable B-Berries             
Which do you choose (FVB)? V                                          

Given the temperature is 32 degrees and 0.200 inches of precipitation
today.  The Vegetables in the Garden will NOT be watered.

My code answer

# include <iostream>
# include <stdio.h>
# include <iomanip>

using namespace std;

int main()
{
    char Typ;
    string Bry = "berries";
    string Veg = "vegetables";
    string Flr = "flowers";
    string  AllStr;
    float Tmp1, Precip;
    int Tmp, FlrW, VegW, BryW, x, Selct;
    bool Cont = true;
    AllStr = Flr + ", " +   Bry + ", " + "and " + Veg;
    Selct = 0;
    x = 0;
    cout << setprecision(3)<<fixed;
    cout<< "Garden Sprinkler System\n\n\n\n";
    cout<< "What is the temperature in degrees(F) ? ";
    cin>>Tmp1;
    cout<<"How much precipitation today (in inches)? ";
    cin>>Precip;
    cout<< "The Garden Divisions are F-Flowers  V-Vegetable B-Berries";
    cout<<"\nWhich do you choose (FVB)? ";
    cin>> Typ;
    Tmp = static_cast<int>(Tmp1);
    for (x  = 1; x <= 4; x++){
        if(x == 4 ){
            cout<<"\nToo many failed attempts.... exiting program";                cout<<"\n\n\n\n\n\n\n\n\n\n\n\n\n\n(loser)";
            Cont = false;                break;
        }
        if (Typ == 'B' || Typ == 'b'){
            Selct =  100;
            break;
        }
        else if  (Typ == 'F' || Typ == 'f'){
            Selct =  10;
            break;
        }
        else if  (Typ == 'V' || Typ == 'v'){
            Selct =  1;
            break;
        }
        else {
            cout<< "\nThe value entered was an INVALID value!";
            cout<< "\nThe Garden Divisions are F-Flowers  V-Vegetable B-Berries";
            cout<<"\nWhich do you choose (FVB)? ";                cin>> Typ;
        }        }
    if (Cont == true){
        if (Tmp > 40){

            switch(Selct){
                case 100:
                    if (Precip < 0.425){
                        cout<< "\n\n\n\nGiven the temperature is "<< Tmp <<" degrees and "<<               Precip<<" inches ";
                        cout<< "of precipitation today.";
                        cout<< "\nThe "<< Bry<<" in the Garden will be watered.";
                        break;
                    }
                    else{
                        cout<< "\n\n\n\nGiven the temperature is "<< Tmp <<" degrees and "<< Precip <<" inches of";
                        cout<<" precipitation today.\nThe "<< Bry <<" in the Garden will NOT be watered.";
                        break;
                    }
                case 10:
                    if (Precip < 0.375){
                        cout<< "\n\n\n\nGiven the temperature is "<< Tmp <<" degrees and "<< Precip<<" inches ";
                        cout<< "of precipitation today.";
                        cout<< "\nThe "<< Flr <<" in the Garden will be watered.";
                        break;
                    }
                    else{
                        cout<< "\n\n\n\nGiven the temperature is "<< Tmp <<" degrees and "<< Precip <<" inches of";
                        cout<<" precipitation today.\nThe "<< Flr <<" in the Garden will NOT be watered.";
                        break;
                    }
                case 1:
                    if (Precip < 0.15){
                        cout<< "\n\n\n\nGiven the temperature is "<< Tmp <<" degrees and "<< Precip<<" inches ";
                        cout<< "of precipitation today.";
                        cout<< "\nThe "<< Veg<<" in the Garden will be watered.";
                        break;
                    }
                    else{
                        cout<< "\n\n\n\nGiven the temperature is "<< Tmp <<" degrees and "<< Precip <<" inches of";
                        cout<<" precipitation today.\nThe "<< Veg<< " in the Garden will NOT be watered.";
                        break;
                    }
                default:
                    cout<< "\n\n\n\nThough the temperature is "<< Tmp <<" degrees and "<< Precip <<" inches of";
                    cout<<" precipitation today.\nThe garden type selection was invalid and the "<< AllStr<<endl;
                    cout<<"in the Garden will NOT be watered.";
            }
        }
        else{
            cout<< "\n\n\n\nGiven the temperature is "<< Tmp <<" degrees and "<< Precip <<" inches of";
            cout<<" precipitation.\nThe "<< AllStr<< " in the Garden will NOT be watered today.";
            cout<<"\nBecause it is too cold.";
        }
    }
    getchar();
    getchar();

    return 0;
}
\$\endgroup\$
  • 3
    \$\begingroup\$ Please learn to format your code so it is easier to read. \$\endgroup\$ – Martin York Oct 15 '11 at 19:02
  • \$\begingroup\$ by that do you mean tabing/indenting bracketed items? Or is there another "formating" being refered to? \$\endgroup\$ – rpshwin Oct 16 '11 at 6:24
  • \$\begingroup\$ uh....I just realized that is exactly what you meant. Will do. \$\endgroup\$ – rpshwin Oct 16 '11 at 6:28
  • 2
    \$\begingroup\$ @RSherwin: spacing as well. Just put exactly one space on each side of every operator and between adjacent identifiers. This will do for a start (until you may later want to align similar statements into columns). Right now you seem to randomly use one, two, or no spaces (around <<, after string etc.) \$\endgroup\$ – Felix Dombek Oct 16 '11 at 7:41
  • 2
    \$\begingroup\$ @RSherwin: ...and please don't do if (Cont == true) …. Cont is itself already a Boolean variable, there is no need to compare it to one, just use if (Cont) …. \$\endgroup\$ – Felix Dombek Oct 16 '11 at 7:48
11
\$\begingroup\$

@Winston has already given some good suggestions, but I think I'd go a slightly different direction. I'd start with a data structure something like this:

struct plant {
    char short_name;
    std::string name;
    float min_water;
};

and of course initialize it with your basic data:

plant plants[] = {
    {'b', "berries", 0.425},
    {'f', "flowers", 0.375},
    {'v', "vegatables", 0.15}
};

That lets you consolidate the data about each type of plant in one place. It also means that most of the rest of the program becomes pretty generic, just displaying data from there, comparing inputs to what it say is required, and so on. It's pretty easy to write the rest of the program so that if (for example) you were to add data for a different type of plant, it would get used by the rest of the code without any modification.

\$\endgroup\$
  • \$\begingroup\$ Jerry's approach is better, but might require more programming experience is appreciate. \$\endgroup\$ – Winston Ewert Oct 15 '11 at 14:21
  • \$\begingroup\$ why the square brackets after plants and not parenthesis? I'm sure that is a dumb question but as I said...structs are still chapters ahead. \$\endgroup\$ – rpshwin Oct 16 '11 at 6:31
  • \$\begingroup\$ @RSherwin: Because we're defining an array of structs, and the syntax for an array uses square brackets. \$\endgroup\$ – Jerry Coffin Oct 16 '11 at 6:45
11
\$\begingroup\$

My thoughts:

  1. Your variables are all declared at the beginning of the function: don't do that. Declare variables close to where they are used
  2. Your variable names are confusing and give little indication of what they are for
  3. You use magic numbers 100, 10, 1 to denote the different types of plants, that's hard to keep track of
  4. You have the exact same lines of code over and over again (this is probably what the teacher meant by having too many strings)
  5. Your code does not have consistent indentation, that makes it hard to read.

Let's look at a small piece of code:

 if (Precip < 0.375){
 cout<< "\n\n\n\nGiven the temperature is "<< Tmp <<" degrees and "<< Precip<<" inches ";
 cout<< "of precipitation today.";
 cout<< "\nThe "<< Flr <<" in the Garden will be watered.";
 break;
 }
 else{
 cout<< "\n\n\n\nGiven the temperature is "<< Tmp <<" degrees and "<< Precip <<" inches of";
  cout<<" precipitation today.\nThe "<< Flr <<" in the Garden will NOT be watered.";     
  break;
  }

The first line of code is the same in both cases, we don't need to repeat them, instead we can do:

 cout<< "\n\n\n\nGiven the temperature is "<< Tmp <<" degrees and "<< Precip<<" inches ";
 if (Precip < 0.375){

 cout<< "of precipitation today.";
 cout<< "\nThe "<< Flr <<" in the Garden will be watered.";
 break;
 }
 else{
  cout<<" precipitation today.\nThe "<< Flr <<" in the Garden will NOT be watered.";     
  break;
  }

See that the code is simpler and less repetitive but does the same thing? We can continue. Most of the text inside the code is

 cout<< "\n\n\n\nGiven the temperature is "<< Tmp <<" degrees and "<< Precip<<" inches ";
 cout<< "of precipitation today.";
 cout<< "\nThe "<< Flr <<" in the Garden will be watered.";
 if (Precip >= 0.375){
     cout << "NOT"
 }

 cout << "be watered.";     
 break;

That way you have less strings, your code is clearer and more "efficient." If you apply this sort of logic several times you should be able to eliminate the same strings appearing multiple times.

Here is what I would for this problem. I may be using techniques you have not learned yet. But, hopefully it will give you some idea of the lack of duplication you should be striving for.

#include <iostream>
#include <stdio.h>
#include <iomanip>

using namespace std;

enum GardenDivision
{
    BERRIES,
    VEGETABLES,
    FLOWERS
};

struct Garden
{
    GardenDivision division;
    int temperature;
    float percipitation;
};

bool read_user_input(Garden & garden)
{
    cout << setprecision(3) << fixed;
    cout << "Garden Sprinkler System\n\n\n\n";
    cout << "What is the temperature in degrees(F) ? ";
    cin >> garden.temperature;
    cout << "How much precipitation today (in inches)? ";
    cin >> garden.percipitation;
    cout << "The Garden Divisions are F-Flowers  V-Vegetable B-Berries";
    cout << "\nWhich do you choose (FVB)? ";

    for(int counter = 0; counter < 4;counter++)
    {
        char division_code;
        cin >> division_code;
        switch(division_code)
        {
            case 'F':
            case 'f':
                garden.division = FLOWERS;
                return true;
            case 'V':
            case 'v':
                garden.division = VEGETABLES;
                return true;
            case 'B':
            case 'b':
                garden.division = BERRIES;
                return true;
        }

        cout << "\nThe value entered was an INVALID value!";
        cout << "\nThe Garden Divisions are F-Flowers  V-Vegetable B-Berries";
        cout << "\nWhich do you choose (FVB)? ";
    }

    cout << "\nToo many failed attempts.... exiting program";  
    cout << "\n\n\n\n\n\n\n\n\n\n\n\n\n\n(loser)";
    return false;
}

void display_weather(Garden & garden)
{
    cout << "\n\n\n\nGiven the temperature is "<< garden.temperature <<
           " degrees and " << garden.percipitation 
           << " inches of precipitation today.\n";

}

void water_section(Garden & garden, string name, float max_perciptiation)
{
    if(garden.percipitation < max_perciptiation)
    {
        cout << "\nThe "<< name << " in the Garden will be watered.";
    }
    else
    {
        cout << "\nThe "<< name << " in the Garden will NOT be watered.";
    }
}

void water_by_section(Garden & garden)
{
    switch(garden.division)
    {
        case FLOWERS:
            water_section(garden, "Flowers", 0.375);
            break;
        case VEGETABLES:
            water_section(garden, "Vegetables", 0.15);
            break;
        case BERRIES:
            water_section(garden, "Berries", 0.425);
            break;
    }
}

int main()
{
    Garden garden;
    if(read_user_input(garden))
    {
        display_weather(garden);
        if(garden.temperature > 40)
        {
            water_by_section(garden);
        }
        else
        {
            cout << "The berries, vegetables, and flowers in the Garden will NOT be watered today.";
            cout << "\nBecause it is too cold.";
        }
    }

    getchar();
    getchar();

    return 0;
}

Good Luck!

\$\endgroup\$
  • \$\begingroup\$ The problem with "string" is that I use the string type variable at all, she wants everything typed out. She was upset I used 5 string variables as that was inefficient code. I think she is a c programmer teaching c++. She isn't really thrilled I use cout as opposed to printf either. We have just began for next loops and structs are chapters away right now. And when we get there I'll be able to follow better what it was you entered. But as for "magic" numbers I get it, I see your use of case and didn't know it could be used in that fashion that will help alot. \$\endgroup\$ – rpshwin Oct 15 '11 at 2:45
  • \$\begingroup\$ @RSherwin, well if the teacher wants you to use printf and not use std::string, there isn't much you can do about that. But all the duplication of having the exact same text over and over again you should reduce. \$\endgroup\$ – Winston Ewert Oct 15 '11 at 2:56
  • \$\begingroup\$ I tried the code posted but it didn't work. The enum looks like what in VB would be a type def, and the "Void, and bool as functions, so I tried seeing why it didn't work but was unsuccesful thank you very much for the input though I appreciate all and any help I can get. \$\endgroup\$ – rpshwin Oct 15 '11 at 3:55
  • \$\begingroup\$ @RSherwin I didn't test the code, so it's probably not quite correct. I just wanted to demonstrate what could be done. \$\endgroup\$ – Winston Ewert Oct 15 '11 at 4:16
  • \$\begingroup\$ @RSherwin: VB has an enum statement in both VB6 and VB.NET; see msdn.microsoft.com/en-us/library/8h84wky1%28v=vs.80%29.aspx \$\endgroup\$ – Felix Dombek Oct 16 '11 at 8:00

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.