10
\$\begingroup\$

I'm new to C++ and decided to have a go at the spotify challenges on their website, http://www.spotify.com/uk/jobs/tech/best-before/

I have now finished but I get the feeling my code is just terrible, I'm guessing it would be very hard for someone else to read and I feel like there are much better ways to code this. If someone could kindly have a look and help me improve my code I would be very thankful.

Here is my code for the best before puzzle:

#include <iostream>
#include <string>
#include <sstream>
#include <iomanip>
using namespace std;


int Year;
int Month;
int Day;

bool loop=true;
int date[4];
bool DateFound=false;
stringstream ss;
string input;
string in;

void Check_date();
void Check_date()
{
    if(DateFound==false)
    {
        //Check if valid Year
        if (date[1]<2999 && date[1]>0 && date[2]>0 && date[3]>0)
        {   //check months & days are valid
            if(date[2]==1 || date[2]==3 || date[2]==5 || date[2]==7 || date[2]==8 || date[2]==10 || date[2]==12){if(date[3]<=31){DateFound=true;}}
            if(date[2]==4 || date[2]==6 || date[2]==9 || date[2]==11){if(date[3]<=30){DateFound=true;}}
            //Check For Leap Year
            if (date[2]==2){
                if(date[3]<28)DateFound=true;
                if(date[1]%4==0 && date[3]<=29)DateFound=true;
                if(date[1]%100==0 && date[1]%400!=0 && date[3]>28)DateFound=false;
            }
        }
        if(DateFound==true)
        {
            Year=date[1]; Month=date[2]; Day=date[3];
            if(Year<1000)Year=Year+2000;
        }
    }
}

void SwitchDate(){int temp; temp=date[2]; date[2]=date[3]; date[3]=temp;   Check_date();};
void ShiftDate(int places)              
{
    if(places==1)
    {   
        int temp; temp=date[3]; date[3]=date[2]; date[2]=temp; temp=date[1]; date[1]=date[2]; date[2]=temp;  Check_date();
    }   
    if(places==2)
    {       
        int temp; temp=date[1]; date[1]=date[2]; date[2]=temp; temp=date[2]; date[2]=date[3]; date[3]=temp; Check_date();
    }       
};

And Main

int main () 
{               
    //Main Loop
    while(loop==true)
    {       
        cout <<"Please Enter a date \n";
        cin>>input;
        cout<<endl; 

        for (int x=0, y=1; y<=3; y++, x++)
        {       
            while (input[x] !='/' && x !=input.length()) ss<<input[x++];
            ss>> date[y];
            ss.clear();
        }           

        //order small medium large
        for (int x=3, temp; x!=0; x--)
        {
            if (date[x] < date[x-1])
            {   temp=date[x-1];
                date[x-1]=date[x];
                date[x]=temp;
            }
            if (x==1 && (date[2] > date[3] ))
            {
                temp=date[3];
                date[3]=date[2];
                date[2]=temp;
            }
        }

        Check_date();
        SwitchDate();
        ShiftDate(1);
        SwitchDate();
        ShiftDate(2);
        SwitchDate();

        //PRINT
        if(DateFound==true)
        {
            cout <<"The smallest valid date is: ";
            cout <<setw(2)<<setfill('0')<<Year; cout<<"-";
            cout <<setw(2)<<setfill('0')<<Month; cout<<"-" ;
            cout <<setw(2)<<setfill('0')<<Day;
            cout<<endl;
        }
        else cout<<date[1]<<"/"<<date[2]<<"/"<<date[3]<<" Is illegal \n";

        DateFound=false;
        cout <<"Again? 'Y' or 'N' \n";
        cin >>in;
        cout << endl;
        if(in=="y" || in=="Y"){loop=true;}
        if(in=="n" || in=="N"){loop=false;}
    }//End of Loop
}
\$\endgroup\$
  • 2
    \$\begingroup\$ Proper indentation would be nice! \$\endgroup\$ – Martin York Mar 10 '12 at 17:26
  • \$\begingroup\$ Fixed formatting and added one '}' so that it compiles. \$\endgroup\$ – Martin York Mar 10 '12 at 17:30
18
\$\begingroup\$

Initial comments on just reading:

Don't do this

using namespace std;

Yes every crappy book on C++ has this line.
Once you get past 5 lines programs it becomes a nuisance (technical term polluting the global namespace). So get in to the habbit of not using it. There are a couple alternatives (read other C++ posts on this forum) personally I prefix anything in standard with std:: (i.e. std::cout)

Global Variables are not a good idea.

int Year;
int Month;
int Day;

bool loop=true;
int date[4];
bool DateFound=false;
stringstream ss;
string input;
string in;

They bind your code to global state which makes modify your code relly hard and writting unit tests and validation code next to imposable. The best practice is a function/methods should not use any external objects. It either is in the scope of the function/method or is passed as a parameter.

Get into the habit of breaking really long lines into small chunks to make it more readable. Also you nested if and its sub-statement all on the same line are a real no-no. It is very hard to read.

        if(date[2]==1 || date[2]==3 || date[2]==5 || date[2]==7 || date[2]==8 || date[2]==10 || date[2]==12){if(date[3]<=31){DateFound=true;}}
 ///   --> comment this way                                                                                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Better would have been:

        if(date[2]==1 || date[2]==3 || date[2]==5 || date[2]==7 || date[2]==8 || date[2]==10 || date[2]==12)
        {
            if(date[3]<=31)
            {    DateFound=true;
            }
        }

Even better would have been:

        if(date[2]==1 || date[2]==3 || date[2]==5 || date[2]==7 ||
           date[2]==8 || date[2]==10 || date[2]==12)
        {
            if(date[3]<=31)
            {    DateFound=true;
            }
        }

Also notice:

            if(date[3]<=31)
            {    DateFound=true;
            }

Can be written as:

            DateFound = date[3]<=31;

There are lots of standard function that can make life easier:

Examples:
Here you are doing a swap

void SwitchDate(){int temp; temp=date[2]; date[2]=date[3]; date[3]=temp;   Check_date();};

Try:

 void SwitchDate(){std::swap(date[2],date[3]); Check_date();};

Next you are implementing a simple bubble sort:

    //order small medium large
    for (int x=3, temp; x!=0; x--)
    {
        if (date[x] < date[x-1])
        {   temp=date[x-1];
            date[x-1]=date[x];
            date[x]=temp;
        }
        if (x==1 && (date[2] > date[3] ))
        {
            temp=date[3];
            date[3]=date[2];
            date[2]=temp;
        }
    }

Try:

std::sort(&date[0], &data[3]);

Next you are implementing a sort of rotation threw the different combinations (using switchDate() and ShiftDate()). This functionality can be achieved using std::next_permutation.

Look here for a list of standard functions:

With boolean expressions there is no point in testing a boolean against true/false. The point in making the variable boolean is so that it can be used directly and it should be named appropriately so that it's meaning is clear.

This is fine:

if(DateFound==false)

But a lot of people find the more succinct style more readable:

if(!DateFound)

As with the if statement above:

while(loop==true)

More concisely written as:

while(loop)

Not sure why you are writing the endl after reading the input.

    cout <<"Please Enter a date \n";
    cin>>input;
    cout<<endl; 

All it does is flush the output buffer. Which has already been flushed (because cin/cout are tied by magic that makes sure the user can read the question before answering).

You want to read three numbers divided by a slash?

    for (int x=0, y=1; y<=3; y++, x++)
    {       
        while (input[x] !='/' && x !=input.length()) ss<<input[x++];
        ss>> date[y];
        ss.clear();
    }          

The stream operators can do much of the work for you. Basically it looks like this:

std::cin >> date[0] >> std::noskipws >> sep[0] >> date[1] >> sep[1] >> date[2];

Notice that your inner while loop has been replaced by a single read:

while (input[x] !='/' && x !=input.length()) ss<<input[x++];

// can be written as:

std::cin >> number;

But to take into account error detection and correction a bit of extra work is needed. So this is how I would do it. (Note its the comments that make it so big).

    // Read one line of user input
    // I always read a line of user input into a string then parse the string
    // This makes error detection and recovery easier as the input stream is never
    // in a bad state or needs to be reset. Also it swallows the new line character
    // that can cause problems if you are not being careful.
    std::string line;
    std::getline(std::cin, line);

    // Now that I have a lone of input I want parse it.
    std::stringstream linestream(line);
    int  date[3];  // I want three numbers
    char sep[2];   // Seporated by '/'

    // Try and read the values you want.
    linestream >> date[0] >> std::noskipws >> sep[0] >> date[1] >> sep[1] >> date[2];
    if (!linestream || sep[0] != '/' || sep[1] != '/')
    {
        // linestream will be bad if reading any of the numbers failed.
        // When linsestream is bad !linestream will return true.
        //
        // So this block is entered if reading the number failed or either of the
        // separators is not a '/' character.
        // Failed.
        throw int(1); // Or whatever is appropriate
    }

Comments on algorithm

You testing for a valid day in a month is very hard to read:

        if(date[2]==1 || date[2]==3 || date[2]==5 || date[2]==7 || date[2]==8 || date[2]==10 || date[2]==12){if(date[3]<=31){DateFound=true;}}
        if(date[2]==4 || date[2]==6 || date[2]==9 || date[2]==11){if(date[3]<=30){DateFound=true;}}
        //Check For Leap Year
        if (date[2]==2){
            if(date[3]<28)DateFound=true;
            if(date[1]%4==0 && date[3]<=29)DateFound=true;
            if(date[1]%100==0 && date[1]%400!=0 && date[3]>28)DateFound=false;
        }

A typical solution is to use an array of values and then look up the correct size:

int daysInMonth[2][]  =   { {31,28,31,30,31,30,31,31,30,31,30,31},
                            {31,29,31,30,31,30,31,31,30,31,30,31} };

bool isLeapYear = date[1]%400==0 || (date[1]%4==0 && date[1]%100!=0);
bool dateFound  = date[3] <= daysInMonth[isLeapYear][date[2]];

Comments on encapsulation

You wrote your program very serially. Basically what you wrote was C code (you just happen to use some basic C++ constructs, this does not make the code C++ (C++ has a style that is distinct from C)).

What you should have done is encapsulate the date in a class of its own.
Then you can write an input method that read data from a stream so that the object initializes itself and can also print itself.

The main should have looked like this:

int main()
{
   bool finished = false;
   do
   {
       doTest();
       cout <<"Again? 'Y' or 'N' \n";
       // Need a loop to test for valid value.
       // Declare variables as close to the point of use as possible.
       // No point in declaring them before you need them.
       std::string line;
       do
       {
           std::getline(cin, line);
       }
       while(line != "y" && line != "Y" && line != "N" && line != "n");

       finished = (line=="N" || line=="n");
   }
   while(!finished);
}

Then we can write a nice test function that does the work.

void doTest()
{
    std::cout <<"Please Enter a date \n";

    try
    {
        MyDateObject   date;

        std::cin >> date;   // Date knows how to read itself from input
        std::cout << date;  // Date knows how to serialize itself to output.
    }
    catch(...)
    {
        std::cout << "Invalid Date\n";
    }
}

Now all you need to do is encapsulate the date functionality into a class call MyDateObject.

\$\endgroup\$
0
\$\begingroup\$

Here is my incomplete answer, this will tell you is you date is valid in any way but won't currently handle 2 figure years, it will print out all of the combinations not just the lowest, but the lowest will be the first. (note mine uses the British date format because it is the best :D, it is trivail to change that)

#include <algorithm>
#include <iterator>
#include <iostream>
#include <iosfwd>
#include <deque>
#include <array>

struct date{
    std::array<int, 3> val;
};
//stream operations
std::istream& operator>>(std::istream& is, date& d) {
    char t;
    return is >> d.val[0] >> t >> d.val[1] >> t >> d.val[2]; 
}
std::ostream& operator<<(std::ostream& os, const date& d) {
    return os << d.val[0] << '-' << d.val[1] << '-' << d.val[2];
}
//for sort
bool date_comp(const date& d1, const date& d2)
{
    //a reverse iterator could be used here, probably better
    for(size_t i=2; i!=0; --i)
    {
            if(d1.val[i] < d2.val[i])
                    return true;
            if(d1.val[i] > d2.val[i])
                    return false;
    }
    return false; //they are the same
}

int days_in_month(int m, int y) {
    switch(m){
        default: return 31;
        case 2: 
            if((year % 4   == 0 
            &&  year % 100 != 0) 
            || (year % 400 == 0))
                return 29
            return 28;
        case 9:  case 6: 
        case 11: case 4:
             return 30;
    }
}

bool is_date(const date& d) {
    int day=d.val[0], month=d.val[1], year=d.val[2];
    //it is often easier to say what isn't true rather than what is
    if(month>12  || month<1
    || year<2000 || year>2999
    || day < 0   || day > days_in_month(month, year))
            return false;
    return true;
}

int main()
{
    date d;
    std::cin >> d;
    std::deque<date> valid;
    std::sort(d.val.begin(), d.val.end());
    do
    {
        if(is_date(d))
            valid.push_back(d);

    }while(std::next_permutation(d.val.begin(), d.val.end()));

    std::sort(valid.begin(), valid.end(), &date_comp);

    if(valid.empty())
    {
        std::cerr << d << " Illegal" << std::endl;
        return 1;
    }

    std::copy(valid.begin(), valid.end(),
              std::ostream_iterator<date>(std::cout, "\n"));
    return 0;
}

I belive my version to be better than your because it heavily relises on the standard library to do the heavy lifting, the std lib is a lot better tested than my code so is a lot less likely to fail as well as being well documented.

Further more my code has better data encapsulation, the data elements are encapsulated in a struct and the instances are own by the stack rather than being global data.

EDIT: added sort before perm

\$\endgroup\$
  • 2
    \$\begingroup\$ The point of this forum is to provide feedback on the code provided. \$\endgroup\$ – Martin York Mar 10 '12 at 17:25
  • \$\begingroup\$ @LokiAstari and? there was a lot wrong with it, my solution shows another way of solving the problem: I genuinely believe the best way to learn is to see it done differently. \$\endgroup\$ – 111111 Mar 10 '12 at 17:28
  • \$\begingroup\$ @LokiAstari I said it was incomplete, any way I fixed it now. \$\endgroup\$ – 111111 Mar 10 '12 at 18:19
  • 1
    \$\begingroup\$ Yes I believe your solution is better. But code review is not about providing a better solution (that does not help people see their mistakes). You need to point out the weaknesses in their design and then from their give alternative approaches were possible. PS. Need work on your leap year calculation. \$\endgroup\$ – Martin York Mar 10 '12 at 18:25
  • 2
    \$\begingroup\$ Showing how it can be done better is good. I do it in most of my answers. But you should also really include comments pointing out the flaws in the original approach as well. \$\endgroup\$ – Winston Ewert Mar 10 '12 at 23: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.