9
\$\begingroup\$

This is a simple program that can tell you your zodiac sign (Chinese or Western) and the year in which you were born. I made this using very basic beginning knowledge with C++ as practice.

I'm looking for some quick reviews on my code to see if there is anything I'm doing wrong or could be done better, making sure I'm not picking up any bad habits, and that it is easy to read.

#include <iostream>
#include <ctime>

int main()
{
    int iZodiac;

    time_t     rawtime;
    struct tm* timeinfo;

    time( &rawtime );
    timeinfo = localtime( &rawtime );

    std::cout << "----Zodiac Program----" << std::endl;

    do {
        std::cout << "\nWhat would you like to do?" << std::endl;
        std::cout << "1.) Chinese Zodiac" << std::endl;
        std::cout << "2.) Western Zodiac" << std::endl;
        std::cout << "3.) Year Born" << std::endl;
        std::cout << "4.) Quit Program" << std::endl;
        std::cin >> iZodiac;

        if (!std::cin || iZodiac <= 0 || iZodiac > 4) {
            std::cin.clear();
            std::cin.ignore(1000, '\n');
            std::cout << "\nI'm sorry but that is not an option. Please answer 1, 2, 3 or 4." << std::endl;
        }


//Start Chinese Zodiac Option
        if (iZodiac == 1) {

            int iCbirth;
            int iCzodiac;

            while ((std::cout << "\nEnter year born." << std::endl) && !(std::cin >> iCbirth)) {
                std::cout << "\nI'm sorry but that is an invalid answer. Please answer using numbers." << std::endl;
                std::cin.clear();
                std::cin.ignore(1000, '\n');
            }

            iCzodiac = iCbirth-((iCbirth/12)*12);

            if (iCzodiac == 0){
                std::cout << "\nYour Chinese Zodiac is the Monkey." << std::endl;
            }
            else if (iCzodiac == 1){
                std::cout << "\nYour Chinese Zodiac is the Rooster." << std::endl;
            }
            else if (iCzodiac == 2){
                std::cout << "\nYour Chinese Zodiac is the Dog." << std::endl;
            }
            else if (iCzodiac == 3){
                std::cout << "\nYour Chinese Zodiac is the Pig." << std::endl;
            }
            else if (iCzodiac == 4){
                std::cout << "\nYour Chinese Zodiac is the Rat." << std::endl;
            }
            else if (iCzodiac == 5){
                std::cout << "\nYour Chinese Zodiac is the Ox." << std::endl;
            }
            else if (iCzodiac == 6){
                std::cout << "\nYour Chinese Zodiac is the Tiger." << std::endl;
            }
            else if (iCzodiac == 7){
                std::cout << "\nYour Chinese Zodiac is the Rabbit." << std::endl;
            }
            else if (iCzodiac == 8){
                std::cout << "\nYour Chinese Zodiac is the Dragon." << std::endl;
            }
            else if (iCzodiac == 9){
                std::cout << "\nYour Chinese Zodiac is the Snake." << std::endl;
            }
            else if (iCzodiac == 10){
                std::cout << "\nYour Chinese Zodiac is the Horse." << std::endl;
            }
            else if (iCzodiac == 11){
                std::cout << "\nYour Chinese Zodiac is the Goat." << std::endl;
            }
        }
//End Chinese Zodiac Option


//Start Western Zodiac Option
        if (iZodiac == 2) {

            int iMonth;
            int iDay;

            while ((std::cout << "\nEnter month born." << std::endl) && !(std::cin >> iMonth)) {
                std::cout << "\nI'm sorry but that is an invalid answer. Please answer using numbers." << std::endl;
                std::cin.clear();
                std::cin.ignore(1000, '\n');
            }
            while ((std::cout << "\nEnter day born." << std::endl) && !(std::cin >> iDay)) {
                std::cout << "\nI'm sorry but that is an invalid answer. Please answer using numbers." << std::endl;
                std::cin.clear();
                std::cin.ignore(1000, '\n');
            }

            if (iMonth == 12) {
                if (iDay >= 22) {
                    std::cout << "\nYour Western Zodiac sign is Capricorn." << std::endl;
                }
                else if (iDay <= 21) {
                    std::cout << "\nYour Western Zodiac sign is Sagittarius." << std::endl;
                }
            }
            else if (iMonth == 11) {
                if (iDay >= 23) {
                    std::cout << "\nYour Western Zodiac sign is Sagittarius." << std::endl;
                }
                else if (iDay <= 22) {
                    std::cout << "\nYour Western Zodiac sign is Scorpio." <<std::endl;
                }
            }
            else if (iMonth == 10) {
                if (iDay >= 23) {
                    std::cout << "\nYour Western Zodiac sign is Scorpio." << std::endl;
                }
                else if (iDay <= 22) {
                    std::cout << "\nYour Western Zodiac sign is Libra." <<std::endl;
                }
            }
            else if (iMonth == 9) {
                if (iDay >= 23) {
                    std::cout << "\nYour Western Zodiac sign is Libra." << std::endl;
                }
                else if (iDay <= 22) {
                    std::cout << "\nYour Western Zodiac sign is Virgo." <<std::endl;
                }
            }
            else if (iMonth == 8) {
                if (iDay >= 23) {
                    std::cout << "\nYour Western Zodiac sign is Virgo." << std::endl;
                }
                else if (iDay <= 22) {
                    std::cout << "\nYour Western Zodiac sign is Leo." <<std::endl;
                }
            }
            else if (iMonth == 7) {
                if (iDay >= 23) {
                    std::cout << "\nYour Western Zodiac sign is Leo." << std::endl;
                }
                else if (iDay <= 22) {
                    std::cout << "\nYour Western Zodiac sign is Cancer." <<std::endl;
                }
            }
            else if (iMonth == 6) {
                if (iDay >= 22) {
                    std::cout << "\nYour Western Zodiac sign is Cancer." << std::endl;
                }
                else if (iDay <= 21) {
                    std::cout << "\nYour Western Zodiac sign is Gemini." <<std::endl;
                }
            }
            else if (iMonth == 5) {
                if (iDay >= 22) {
                    std::cout << "\nYour Western Zodiac sign is Gemini." << std::endl;
                }
                else if (iDay <= 21) {
                    std::cout << "\nYour Western Zodiac sign is Taurus." <<std::endl;
                }
            }
            else if (iMonth == 4) {
                if (iDay >= 21) {
                    std::cout << "\nYour Western Zodiac sign is Taurus." << std::endl;
                }
                else if (iDay <= 20) {
                    std::cout << "\nYour Western Zodiac sign is Aries." <<std::endl;
                }
            }
            else if (iMonth == 3) {
                if (iDay >= 21) {
                    std::cout << "\nYour Western Zodiac sign is Aries." << std::endl;
                }
                else if (iDay <= 20) {
                    std::cout << "\nYour Western Zodiac sign is Pisces." <<std::endl;
                }
            }
            else if (iMonth == 2) {
                if (iDay >= 20) {
                    std::cout << "\nYour Western Zodiac sign is Pisces." << std::endl;
                }
                else if (iDay <= 19) {
                    std::cout << "\nYour Western Zodiac sign is Aquarius." <<std::endl;
                }
            }
            else if (iMonth == 1) {
                if (iDay >= 21) {
                    std::cout << "\nYour Western Zodiac sign is Aquarius." << std::endl;
                }
                else if (iDay <= 20) {
                    std::cout << "\nYour Western Zodiac sign is Capricorn." <<std::endl;
                }
            }
        }
//End Western Zodiac Option


//Start Year Born Option
        if (iZodiac == 3) {
            int iAge;
            int iYear;
            int iBirth;

            iYear = (timeinfo->tm_year + 1900);

            while ((std::cout << "\nEnter your current age." << std::endl) && !(std::cin >> iAge)) {
                std::cout << "\nI'm sorry but that is an invalid answer. Please answer using numbers." << std::endl;
                std::cin.clear();
                std::cin.ignore(1000, '\n');
            }
            iBirth = iYear-iAge;
            std::cout << "\nYou were born in the year " << iBirth << std::endl;
        }
//End Year Born Option


    } while (iZodiac != 4);

    std::cout << "\nThank you for playing, Good bye." << std::endl;

    return 0;
}
\$\endgroup\$
  • \$\begingroup\$ Thank you everyone for all your help. This has given me a good bit to study, some of which I need to become more knowledgeable on, (such as functions & loops) others I need to learn still.(like switchs, strings, and that the get thing) There is lots mentioned that I don't understand reasons for, but those questions are easily cleared up through so googling. Anywho it may be a while and I can't promise it will be the same program but I look forward to coming back with something better for everyone. Also need to better learn how to use this forum... @jamal \$\endgroup\$ – Macchan Sep 24 '13 at 13:54
  • \$\begingroup\$ You're welcome! Good luck with your studying. These are some common mistakes for newcomers to the language, so it's great that you're willing to learn about and maintain good habits. Feel free to come back here whenever. Moreover, you may accept one of these answers if you've received satisfying reviews. It's entirely optional, though. \$\endgroup\$ – Jamal Sep 24 '13 at 14:04
14
\$\begingroup\$
  • Since this is C++, prefer std::size_t over C-like size_t. With <ctime>, time_t should also be std::time_t. Keep the std:: consistent for all necessary aspects of the STL.

  • Declare/initialize variables as close in scope as possible:

    int iZodiac;
    std::cin >> iZodiac;
    

    This is also an example of Hungarian notation, which is generally discouraged. Just name it zodiac or something similar.

  • There's no need for this within the while condition:

    while ((std::cout << "\nEnter your current age." << std::endl) /* ... */)
    

    As this isn't a condition, it should just go inside the loop body. The std::cin should stay, as you're needing to verify that the input was valid.

  • This program should definitely be modular. In other words, it should utilize more functions. The do-while loop extends through all of main(), reducing readability and maintainability.

    For instance, the menu and input validation should stay in main(). That can be in a do-while loop by itself. This will prevent the program from shifting control to the other functions until the user selects an appropriate menu choice. Each choice could could a function (except "quit," which will just fall back to the end of main() for program termination).

  • All the conditional blocks are confusing to navigate. One option, although not the best but simple, is with a concise switch statement. For instance, you could put this into a function that receives zodiac and returns the corresponding animal string (not the entire message).

    This uses std::string, which is the C++ STL implementation of a char array with added features and optimization. I recommend that you become familiar with it.

    int zodiac = birth-((birth/12)*12);
    
    std::cout << "Your Chinese Zodiac is " << getChineseSign(zodiac);
    
    std::string getChineseSign(const int zodiac)
    {
        switch (zodiac)
        {
            case 0 : return "Monkey";
            case 1 : return "Rooster";
            // ...
            case 10: return "Horse";
            case 11: return "Goat";
    
            // throw an exception if not 0-11
            // include <stdexcept> to use this
            default: throw std::logic_error("unknown zodiac");
        }
    }
    
\$\endgroup\$
  • \$\begingroup\$ Great, only that I'd advise to use some kind of map (not necessarily a map DS) in place of the switch block. Long switches are ugly. \$\endgroup\$ – idoby Sep 22 '13 at 18:43
  • \$\begingroup\$ @busy_wait: I was actually just thinking of a map at first, but wasn't sure how exactly it would be done here. I'd say a switch could be a start, as long as it shortens some of the code. \$\endgroup\$ – Jamal Sep 22 '13 at 18:46
  • \$\begingroup\$ Actually, the division and multiplication won't cancel each other out as it'll be an integer division. Its a convoluted way to calculate the modulus. \$\endgroup\$ – Winston Ewert Sep 22 '13 at 23:43
  • \$\begingroup\$ @WinstonEwert: Hm. I never knew about that. I suppose it was obvious since there were no comments explaining it. \$\endgroup\$ – Jamal Sep 22 '13 at 23:47
  • \$\begingroup\$ '\n' will still cause a flush just the same as std::endl as the output is to an interactive device. See stackoverflow.com/a/25569849/2498188. \$\endgroup\$ – Emily L. Oct 22 '17 at 14:42
8
\$\begingroup\$

I would do the following 3 things.
Then I would come back and ask the question again.

  1. Modularize your code.
    Put each zodiac variant into its own function.

  2. Remove repeated code.
    Anything that is done repeatedly refactor to be only done once. Then the small amounts that are different are the bits you can concentrate on.

        if (iCzodiac == 0){
            std::cout << "\nYour Chinese Zodiac is the Monkey." << std::endl;
        }
        else if (iCzodiac == 1){
            std::cout << "\nYour Chinese Zodiac is the Rooster." << std::endl;
        }
        .....
    
    // Can be made much easier to read as:
    
    std::string zodiakAnimal = getZodiakAnimal(iCzodiac);
    std::cout << "\nYour Chinese Zodiac is the ." << zodiakAnimal << "\n";
    
  3. Stop using Hungarian notation for your variables.

    int iCzodiac;  // Really. I can see it's an int by the type.
    
    // Prefer human readable names
    int chinese_zodiac;
    
\$\endgroup\$
4
\$\begingroup\$

I don't have much to say after Jamal's comment.

However, I just wanted to point out that some tests are not useful. All the logic around Western Zodia sign could be simplified and probably should be extracted in a function on its own.

    switch(iMonth) {
        case 12: return (iDay >= 22) ? "Capricorn" : "Sagittarius";
        case 11: return (iDay >= 23) ? "Sagittarius" : "Scorpio";
        case 10: return (iDay >= 23) ? "Scorpio" : "Libra";
        case 9:  return (iDay >= 23) ? "Libra" : "Virgo";
        case 8:  return (iDay >= 23) ? "Virgo" : "Leo";
        case 7:  return (iDay >= 23) ? "Leo" : "Cancer";
        case 6:  return (iDay >= 22) ? "Cancer" : "Gemini";
        case 5:  return (iDay >= 22) ? "Gemini" : "Taurus";
        case 4:  return (iDay >= 21) ? "Taurus" : "Aries";
        case 3:  return (iDay >= 21) ? "Aries" : "Pisces";
        case 2:  return (iDay >= 20) ? "Pisces" : "Aquarius";
        case 1:  return (iDay >= 21) ? "Aquarius" : "Capricorn";
    }
\$\endgroup\$
  • \$\begingroup\$ This is mostly why I based my switch example around the Chinese Zodiac in particular-- it's much simpler. As far as switch goes, this would probably be good enough. Surely there's a nicer way of doing both zodiacs, thinking back to @busy_wait's mentioning of a map. \$\endgroup\$ – Jamal Sep 23 '13 at 0:25
2
\$\begingroup\$

Just as an aside to the good answers you have already been given:

Consider handling leap years

During a leap year the dates for the different zodiacs change.

bool leapyear = (year % 4 == 0) && ((year % 100 != 0) || (year % 400 == 0)); 

Consider expressing your if zodiac in day of year

You are currently testing each of the zodiac signs for month and then day, giving you a total of 12*2 = 24 if statements. If you instead convert (month + day) into (day of year) you only need 12 statements.

You can do something like:

int dayOfYear(int month, int day){
    // Ignores leap years
    static int daysUntilMonth[] = {0,  // Jan 1st 
                                   31, // Feb 1st
                                   31 + 28, // Mar 1st
                                   59 + 31, // Apr 1st
                                   90 + 30,  // May 1st
                                   120 + 31, // Jun 1st
                                   151 + 30, // Jul 1st
                                   181 + 31, // Aug 1st
                                   212 + 31, // Sep 1st
                                   243 + 30, // Oct 1st
                                   273 + 31, // Nov 1st
                                   304 + 30  // Dec 1st
                                   };
    if( month < 1 || month > 12){
        throw std::exception("Invalid month!");
    }
    return daysUntilMonth[month - 1] + day;
}

And then when determining which zodiac it is you can do something like this:

int day_year = dayOfYear(month, day);
if(day_year >= dayOfYear(12, 22)){
    // Capricorn
} else if(day_year >= dayOfYear(11, 23)){
    // Sagittarius
} else if(day_year >= dayOfYear(10, 23)){
    // Scorpio
} .. // And so forth
\$\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.