6
\$\begingroup\$

I'm new To C++ and decided to have a go at the Spotify challenges on their website.

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.

Their "Best Before" puzzle asks: Given a possibly ambiguous date written in "A/B/C" format, where A,B,C are integers between 0 and 2999, interpret them as M/D/Y or D/M/Y or whatever common date format. Pick the interpretation that yields the earliest possible legal date between Jan 1, 2000 and Dec 31, 2999 (inclusive) in "YYYY-MM-DD" format. If the string cannot be interpreted as a valid date, state so.

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;




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


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;} if(Month<10){}}

    }




}





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();
    }
};

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\$

migrated from stackoverflow.com Mar 12 '12 at 12:44

This question came from our site for professional and enthusiast programmers.

4
\$\begingroup\$
  1. You should avoid using global variables. If you import this code into a future project it might cause problems -- name conflicts etc.

    If you must do this, put them in an anonymous namespace so that they're only in scope for this file:

    namespace
    {
        int my;
        int globals;
        int defined;
        int here;
    }
    
  2. The declaration of Check_date() is not necessary here:

    void Check_date();
    void Check_date()
    {
    
  3. This could perhaps written better:

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

    There is little point using x in the for loop as you don't have an exit condition for it. Also, I would prefer a more meaningful name.

    Instead, perhaps:

    int inputPos = 0;
    for(int dateIndex = 1; dateIndex <= 3; ++dateIndex)
    {
        while(input[inputPos] != '/' && inputPos != input.length()) 
        {
            ss >> date[dateIndex];
            ss.clear();
        }
        ++inputPos;
    }
    
  4. More generally you've written this in a very C-like style -- I would have defined a type (class) for Date, which would have month and day members. Then the logic for whether month lengths are valid can be encapsulated properly.

    You could also encapsulate the read-from-stream code using an iostreams extractor.

\$\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.