3
\$\begingroup\$

My program displays the price of a movie ticket. The price is based on the customer’s age. If the user enters a negative number, the program displays the “Invalid age” message.

#include<iostream>

using namespace std;

int main(){
    
    int age = 0;
    int price = 0;
    
    
    cout << " What is your age?: ";
    cin>> age;
    
    
    if (age >= 0 && age < 4)
        price = 0;
        if( age >= 4 && age <= 64 )
            price = 9;
   
    if( age > 65)
        price = 6;
    
    if (age<0)
        cout<< " Invalid Age "<< endl;
    
    else
        if(age >= 0 && age <= 3)
            cout << " The price of your ticket is: $ " << price << endl ;
    
    if( age >= 4 && age <= 64)
        cout<< " The price of your ticket is: $ " << price << endl ;
    
    if (age > 65)
        cout<< " The price of your ticket is: $ " << price << endl ;
    
    return 0;

}
\$\endgroup\$
4
  • \$\begingroup\$ Welcome to Code Review! I changed the title so that it describes what the code does per site goals: "State what your code does in your title, not your main concerns about it.". Please check that I haven't misrepresented your code, and correct it if I have. \$\endgroup\$ Oct 7 at 7:16
  • 1
    \$\begingroup\$ 65-year-olds get free tickets! They also get no output. \$\endgroup\$
    – ojchase
    Oct 7 at 17:56
  • \$\begingroup\$ A common issue beginners have is copying code. A common acronym used in the software world is DRY - Don't Repeat Yourself. cout<< " The price of your ticket is: $ " << price << endl ; is a prime example of you repeating yourself. I would also criticise whitespace inconsistency (easily solved by using an editor that understands and will auto-format code). I applaud the verbose variable names price and age, new programmers tend to use single letters which is hard to understand. Long term I would recommend reading gist.github.com/wojteklu/73c6914cc446146b8b533c0988cf8d29 \$\endgroup\$
    – Drise
    Oct 7 at 18:15
  • \$\begingroup\$ This gist is derived from Clean Code by Robert Martin. Some of the concepts may be a bit advanced at your level, but it's not a bad idea to get a basic understanding early on. \$\endgroup\$
    – Drise
    Oct 7 at 18:18
7
\$\begingroup\$

Your overall code works fine, but there are some improvements.

Using else if's

You are comparing age against 0, 4, 64, 65 pointlessly, it can only be one. In this case we can use else if.

Duplicate code

Once you have determined the price, you do not need to check age again, you can directly print the output. Of course after restructuring your if-else if ladder.

using namespace std;

You should avoid using using namespace std; as it is a bad practice. Refer this StackOverflow answer for more light.

Inconsistent whitespace

At some places in the code, you leave whitespace which makes it look good. Whereas some places you do not. Be consistent with your whitespace.

Flushing

Do not use std::endl unless you want to flush a buffer; use "\n".

Checking Input

If the user enters "One" then your program will never change price, it will remain 0. So you should check the input stream after taking user input.

if (!std::cin) 
{
    std::cerr << "Input error\n";
    return 0;
}

Rest seems fine to me. I hope you are having a good time with C++.

Happy Coding!

\$\endgroup\$
4
\$\begingroup\$

We have some popular beginner problems:

  • Don't using namespace std - this makes it harder to tell which identifiers are your own and which come from the standard library. We have namespaces to help us, and this just throws away all the benefits. Unfortunately, many tutorials present this as if it were a good practice!

  • Error messages should go to the standard error stream std::cerr, rather than to std::cout.

  • When reading from a stream, it's essential to check that the read succeeded:

    std::cin >> age;
    if (!std::cin) {
        std::cerr << "Invalid input (expected integer)\n";
        return EXIT_FAILURE;
    }
    

    (EXIT_FAILURE is defined in <cstdlib>).

  • Don't use std::endl unless you really mean to flush the output. I recommend never using std::endl - instead use both \n and std::flush to be absolutely clear when you mean to flush.

  • We are allowed to omit return 0; from the end of main() (but not from any other function). Writing it isn't wrong, just unnecessary.


At the moment, everything is in a big main() that has several responsibilities: reading input, calculating the price and printing it. To make the code easier to test, I recommend creating a separate function for the calculation. We can start with the outline:

int price(int age)
{
    return 9;
}

And write some simple tests:

#include<iostream>

int main()
{
    for (int age: {0, 3, 4, 64, 65}) {
        std::cout << "Price for someone aged " << age
                  << " is " << price(age) << "\n";
    }
}

Obviously, this gives 9 for all ages. But we can now start to fill in the body of price() to give the correct results:

int price(int age)
{
    if (age < 4) { return 0; }
    if (age < 65) { return 9; }
    // else age >= 65
    return 6;
}

This just leaves the question of how to deal with age < 0. For this, I would throw an exception:

#include <stdexcept>

int price(int age)
{
    if (age < 0) { throw std::invalid_argument("Invalid Age"); }
    if (age < 4) { return 0; }
    if (age < 65) { return 9; }
    // else age >= 65
    return 6;
}

We can test it in our main():

try {
    price(-1);
    std::cerr << "Expected exception not thrown!";
} catch (const std::invalid_argument &e) {
    std::cout << "Caught exception: " << e.what() << '\n';
}

Once we're happy it all works, we can replace our test main() with one that reads input and prints the result:

#include <stdexcept>

int price(int age)
{
    if (age < 0) { throw std::invalid_argument("Invalid Age"); }
    if (age < 4) { return 0; }
    if (age < 65) { return 9; }
    return 6;
}


#include <cstdlib>
#include <iostream>

int main()
{
    int age;
    std::cout << "What is your age?: ";
    if (!(std::cin >> age)) {
          std::cerr << "Invalid input (expected integer)\n";
          return EXIT_FAILURE;
    }

    try {
        auto p = price(age);
        std::cout << "The price of your ticket is: $"
                  << p << "\n";
    }
    catch (const std::invalid_argument &e) {
        std::cerr << e.what() << '\n';
        return EXIT_FAILURE;
    }
}
\$\endgroup\$
1
\$\begingroup\$
    else
        if(age >= 0 && age <= 3)
            cout << " The price of your ticket is: $ " << price << endl ;
    
    if( age >= 4 && age <= 64)
        cout<< " The price of your ticket is: $ " << price << endl ;
    
    if (age > 65)
        cout<< " The price of your ticket is: $ " << price << endl ;

You seem to be doing the exact same thing regardless of the condition!
I think maybe you refactored the code to separate the computation from the printing, but never cleaned up the old code after making the replacement. I suggest that you read through the code yourself once, before showing it to others to review. When editing, you often don't actually re-read the whole thing — take a short break, and before calling it "done", read through the code top to bottom. It's a good habit to get into. Even experienced developers spot funny spacing and comments that are notes-to-self that need to be removed, or comments that need to be updated.

\$\endgroup\$

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.