C++

Forum

 
looking for some pointers to improve my C++ code

steakyfask (1)
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: 


1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
115
116
117
118
119
120
121
122
123
124
125
126
127
128
129
130
131
132
133
134
135
136
137
138
139
140
141
142
143
144
145
146
147
148
149
150
 
 

#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






}






Stewbond (2827)
Your main problem is formatting.
1. Make sure you indent when applicable. That way we can see what is in what loop and in what function easily.

2. Don't put 5 new lines between code when it doesn't serve any purpose. If its a completely new section and has nothing to do with what's above it, it's one thing, but when you have related code in the same function seperated by 5 spaces, 
1
2
3
 
it          
                       gets              hard   
     to                     read


3. Put spaces where you need it e.g. line 62/64 (above) and especially line 45 (above) which contains multiple if statements (one of which is empty!).

4. Give variables as small a scope as possible. You have stringstream ss declared in a global scope. This means that ANYONE can use it which means in a complicated project you increase the chances that someone is going to modify it without your consent. Declare things in functions whenever possible. If it is only used inside an if statement, then declare it inside that if statement so that it is destroyed as soon as you are done with it. Really there are very few excuses for using global variables, you should be able to pass things as arguments to functions for anything.

This is what I would consider to be a properly formatted document. You'll see it is much easier to read. People will be able to help you more and you'll be able to debug more effectively.

Note that I don't put things on the same line as the {s and that I always indent anything that belongs to an if statement .

Formatting/indenting standards are flexible and there are many styles you can choose, but make sure you do choose one and that you are consistent. For structured programming controls some people prefer:
1
2
3
 
if (condition) {
    //Stuff
}

While others (including me) prefer
1
2
3
4
 
if (condition)
{
    //Stuff
}


Some people like to use { and } at all times, even if there is only one statement in the if, others like to put the single statement on the same line as the if statement while others like to put it on the next line indented. You can choose whichever one, but make sure that you are consistent.

For declaring local variables in functions, some people like to declare all of their variables at the top of the required scope (which is something that was big in C) while others like to declare them very close to their first use. I prefer the latter.

Anyways this is how I would have formatted your code:
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
115
116
117
118
119
120
121
122
123
124
125
126
127
128
129
130
131
132
133
134
135
136
137
138
139
140
141
142
143
144
145
146
147
148
149
150
151
152
153
154
155
156
157
158
159
160
 
#include <iostream> 
#include <string>
#include <sstream>
#include <iomanip>
using namespace std;

int Year;
int Month;
int Day;

int date[4];
bool DateFound=false;

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)
        {
            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();
    }
    else 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(true)
    {
        string input;
        cout <<"Please Enter a date \n";
        cin>>input;
        cout<<endl;

        for (int x=0, y=1; y<=3; y++, x++)
        {
            stringstream ss;

            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)
        {
            cout <<"The smallest valid date is: "
                 <<setw(2) << setfill('0') <<Year  << "-"
                 <<setw(2) << setfill('0') <<Month << "-"
                 <<setw(2) << setfill('0') <<Day   << endl;
        }
        else 
            cout <<date[1]<<"/"<<date[2]<<"/"<<date[3]<<" Is illegal \n";

        DateFound=false;

        string in;
        cout <<"Again? 'Y' or 'N' \n";
        cin >>in;
        cout << endl;

        if(in=="n" || in=="N")    
            break;
    }//End of Loop
}


Last edited on
Topic archived. No new replies allowed.