C++

Forum

 
DO WHILE loop not behaving as expected

chuvak (67)
Hi forum, code is below.

Everything seems to work as expected, but as soon as sin goes out of the [MIN_SIN] to [MAX_SIN] range, it just prints "Please enter your social insurance number (0 to quit): " over and over again.

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
 
#ifndef SICT_CRA_ACCOUNTING
#define SICT_CRA_ACCOUNTING
#define MAX_NAME_LENGTH 40
#define MIN_SIN 100000000
#define MAX_SIN 999999999


namespace sict
{
	class CRA_Account
	{
		char* lastName;
		char* firstName;
		int numSIN;
	public:
		//CRA_Account();
		void set(const char* familyName, const char* givenName, int sin);
		void setEmpty();
		void display() const;
		bool isEmpty(int sin);
	};

}
#endif // !SICT_CRA_ACCOUNTING


#include <iostream>
//#include "CRA_Account.h"

using namespace std;

namespace sict
{
	void CRA_Account::setEmpty()
	{
		lastName = nullptr;
		firstName = 0;
		numSIN = 0;
	}
	void CRA_Account::set(const char* familyName, const char* givenName, int sin)
	{
		if (sin >= MIN_SIN && sin <= MAX_SIN)
		{
			lastName = new char[(strlen(familyName)) + 1];
			firstName = new char[(strlen(givenName)) + 1];
			strcpy(lastName, familyName);
			strcpy(firstName, givenName);
			numSIN = sin;
		}

	}
	void CRA_Account::display() const
	{
		if (0 != numSIN || nullptr != lastName || nullptr != firstName)
		{
			cout << "Family Name: " << this->lastName << endl;
			cout << "Given Name: " << this->firstName << endl;
			cout << "SIN: " << this->numSIN << endl << endl;
		}
		else
		{
			cout << "Account object is empty!" << endl << endl;
		}
	}
	bool CRA_Account::isEmpty(int sin)
	{
		if ((0 == sin || sin <= MIN_SIN || sin >= MAX_SIN))
		{
			return true;
		}
		else
		{
			return false;
		}
	}
}

#include <iostream>
//#include "CRA_Account.h"

//using namespace std;
//using namespace sict;

int main()
{
	CRA_Account myCRA;
	int sin;
	bool quit = false;
	char familyName[MAX_NAME_LENGTH + 1];
	char givenName[MAX_NAME_LENGTH + 1];

	myCRA.setEmpty();
	cout << "Canada Revenue Agency Account App" << endl
		<< "=================================" << endl << endl;
	cout << "Please enter your family name: ";
	cin >> familyName;
	cin.ignore();
	cout << familyName << endl;
	cout << "Please enter your given name: ";
	cin >> givenName;
	cin.ignore();
	cout << givenName << endl;


	do {
		cout << "Please enter your social insurance number (0 to quit): ";
		cin >> sin;
		cin.ignore();
		if (sin != 0)
		{
			myCRA.set(familyName, givenName, sin);
			if (myCRA.isEmpty(sin)) {
				cout << "Invalid input! Try again." << endl;
			}
			else {
				quit = true;
			}
		}
		else {
			quit = true;
		}
	} while (!quit);
	cout << "----------------------------------------" << endl;
	cout << "Testing the display function" << endl;
	cout << "----------------------------------------" << endl;
	myCRA.display();
	cout << "----------------------------------------" << endl;
		
	getchar();
		return 0;
	}
Last edited on
Handy Andy (5051)
Hello chuvak,

You say that the program runs, but I could not get the program to compile either here at this or on my system.

You have included <iostream> twice and are missing other header files.

"strlen" and "strcpy" will not even work for me and the header files needed for these are missing. I do have to ask if you are writing a C or C++ program? Using std::string" would work much easier.

As I look back at what you posted it looks like you have a header file and two .cpp files all together. If they are separate than post them as separate files.

Line 86 is missing a qualifier.

Shortly I will make the necessary changes and see how it works.

Hope that helps,

Andy
chuvak (67)
Thank you andy, below is the working code, can you please review it if I have any errors:

//header
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
 
#ifndef SICT_CRA_ACCOUNTING
#define SICT_CRA_ACCOUNTING
#define MAX_NAME_LENGTH 40
#define MIN_SIN 100000000
#define MAX_SIN 999999999


namespace sict
{
	class CRA_Account
	{
		char* lastName;
		char* firstName;
		int numSIN;
	public:
		//CRA_Account();
		void set(const char* familyName, const char* givenName, int sin);
		void setEmpty();
		void display() const;
		bool isEmpty(int sin);
	};

}
#endif // !SICT_CRA_ACCOUNTING


//implementation file
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
 
#include <iostream>
#include <cstring>
#include "CRA_Account.h"

using namespace std;

namespace sict
{
	void CRA_Account::setEmpty()
	{
		lastName = nullptr;
		firstName = 0;
		numSIN = 0;
	}
	void CRA_Account::set(const char* familyName, const char* givenName, int sin)
	{
		if (sin >= MIN_SIN && sin <= MAX_SIN)
		{
			lastName = new char[(strlen(familyName)) + 1];
			firstName = new char[(strlen(givenName)) + 1];
			strcpy(lastName, familyName);
			strcpy(firstName, givenName);
			numSIN = sin;
		}

	}
	void CRA_Account::display() const
	{
		if (0 != numSIN || nullptr != lastName || nullptr != firstName)
		{
			cout << "Family Name: " << this->lastName << endl;
			cout << "Given Name: " << this->firstName << endl;
			cout << "SIN: " << this->numSIN << endl << endl;
		}
		else
		{
			cout << "Account object is empty!" << endl << endl;
		}
	}
	bool CRA_Account::isEmpty(int sin)
	{
		if ((0 == sin || sin <= MIN_SIN || sin >= MAX_SIN))
		{
			return true;
		}
		else
		{
			return false;
		}
	}
}


//main
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
 
#include <iostream>
#include "CRA_Account.h"

using namespace std;
using namespace sict;

int main()
{
	CRA_Account myCRA;
	int sin;
	bool quit = false;
	char familyName[MAX_NAME_LENGTH + 1];
	char givenName[MAX_NAME_LENGTH + 1];

	myCRA.setEmpty();
	cout << "Canada Revenue Agency Account App" << endl
		<< "=================================" << endl << endl;
	cout << "Please enter your family name: ";
	cin >> familyName;
	cin.ignore();
	cout << "Please enter your given name: ";
	cin >> givenName;
	cin.ignore();


	do {
		cout << "Please enter your social insurance number (0 to quit): ";
		cin >> sin;
		cin.ignore();
		if (sin != 0 || sin > MAX_SIN)
		{
			myCRA.set(familyName, givenName, sin);
			if (myCRA.isEmpty(sin)) {
				cout << "Invalid input! Try again." << endl;
			}
			else {
				quit = true; 
			}
		}
		else {
			quit = true;
		}
	} while (!quit);
	cout << "----------------------------------------" << endl;
	cout << "Testing the display function" << endl;
	cout << "----------------------------------------" << endl;
	myCRA.display();
	cout << "----------------------------------------" << endl;
		
	getchar();
		return 0;
	}


I got it to work, but I feel like I am not implementing it as I am supposed to be
Last edited on
Handy Andy (5051)
Hello chuvak,

I tried your new code and made one addition so it would compile and the program works. All the testing I did worked as it should.

If you are going to use "sin" ass a way to quit the program it should be input first instead of last.

If you intend on a C++ program you should stay away from code like "strlen" and "strcpy" as these are outdated and there are better ways than using C code.

The following code is what I came up with. The changes are noted in the comments of the code. And I learned awhile ago to stay away from "using namespace" anything because it WILL get you in trouble some day.

Header file:
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
 
#ifndef SICT_CRA_ACCOUNTING
#define SICT_CRA_ACCOUNTING
#define MAX_NAME_LENGTH 40
#define MIN_SIN 100000000
#define MAX_SIN 999999999

namespace sict
{
	class CRA_Account
	{
		std::string lastName;  // <--- Changed.
		std::string firstName;  // <--- Changed.
		int numSIN;
	public:
		//CRA_Account();
		void set(const std::string familyName, const std::string givenName, int sin);  // <--- Changed.
		void setEmpty();
		void display() const;
		bool isEmpty(int sin);
	};

}
#endif // !SICT_CRA_ACCOUNTING


.cpp file:
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
 
#include <iostream>
#include <string>

#include "CRA_Account.h"

//using namespace std;

namespace sict
{
	void CRA_Account::setEmpty()
	{
		lastName = "";  // <--- Changed.
		firstName = "";  // <--- Changed.
		numSIN = 0;
	}
	void CRA_Account::set(const std::string familyName, const std::string givenName, int sin)  // <--- Changed.
	{
		if (sin >= MIN_SIN && sin <= MAX_SIN)
		{
			// <--- These two lines not needed.
			//lastName = new char[(strlen(familyName)) + 1];
			//firstName = new char[(strlen(givenName)) + 1];

			lastName = familyName;  // <--- Changed.
			firstName = givenName;  // <--- Changed.
			numSIN = sin;
		}

	}
	void CRA_Account::display() const
	{
		if (0 != numSIN && "" != lastName && "" != firstName)  // <--- Changed.
		{
			std::cout << "Family Name: " << this-> lastName << std::endl;
			std::cout << "Given Name: " << this-> firstName << std::endl;
			std::cout << "SIN: " << this-> numSIN << std::endl << std::endl;
		}
		else
		{
			std::cout << "Account object is empty!" << std::endl << std::endl;
		}
	}
	bool CRA_Account::isEmpty(int sin)
	{
		if ((0 == sin || sin <= MIN_SIN || sin >= MAX_SIN))  // May work better as &&.
		{
			return true;
		}
		else
		{
			return false;
		}
	}
}


Main:
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
 
#include <iostream>
#include <string>  // <--- Added.

#include "CRA_Account.h"  // <--- Added. Actually uncommented what you had.

int main()
{
	sict::CRA_Account myCRA;
	int sin;
	bool quit = false;
	std::string familyName{};
	std::string givenName{};

	myCRA.setEmpty();

	std::cout << "Canada Revenue Agency Account App" << std::endl
		<< "=================================" << std::endl << std::endl;
	std::cout << "Please enter your family name: ";
	std::cin >> familyName;  // <--- std::getline(std::cin, familyName) would work better.
	std::cin.ignore();  // <--- Not really needed.
	//std::cout << familyName << std::endl;  //<--- Not necessary.
	std::cout << "Please enter your given name: ";
	std::cin >> givenName;  // <--- std::getline(std::cin, givenName) would work better.
	std::cin.ignore();  // <--- Not really needed.
	//std::cout << givenName << std::endl;  //<--- Not necessary.

	do {
		std::cout << "Please enter your social insurance number (0 to quit): ";
		std::cin >> sin;
		std::cin.ignore();
		if (sin != 0)
		{
			myCRA.set(familyName, givenName, sin);
			if (myCRA.isEmpty(sin))
				std::cout << "Invalid input! Try again." << std::endl;
			else
				quit = true;
		}
		else
			quit = true;

	} while (!quit);

	std::cout << "\n\n----------------------------------------" << std::endl;  // <--- Added \n\n for a better look on output.
	std::cout << "Testing the display function" << std::endl;
	std::cout << "----------------------------------------" << std::endl;
	myCRA.display();
	std::cout << "----------------------------------------" << std::endl;

        std::cout << "\n\n Press enter to continue."; // <--- Added:
	getchar();
	return 0;
}


The code is mostly the same except for the changes I made and some cosmetic changes to the output.

If you have any questions let me know,

Andy
TheIdeasMan (6781)
Hi,

I would make some other changes too:

I don't like the use of the do loop here, notice you have quit = true twice , as well as while(!quit) I would make it a while(!quit) {} loop as opposed to a do loop. One can use break and continue in loops. At the moment the loops ends if the input is empty, contrary to what is alluded to in the code.

There is no need to use the this keyword in this context, member functions have direct access to member variables.

Meaningful variable names are important, they aid understanding and can prevent certain kinds of errors. Every time I see sin, I think of the trig function. A lot of people fall into the trap of over abbreviating their variables identifiers, perhaps something along the lines of SocialInsuranceNumber might be better? It's longer, but crystal clear as to what it is.

The CRA_Account::isEmpty is misleading too, IMO (In My Opinion) Empty is something that might apply to a container of objects. This function tests the validity of the Social Insurance Number, and doesn't seem to be related to the setEmpty function.

What does CRA and sict mean? I shouldn't have to guess. Reading code should be like reading a story, meaningful names document what is happening. Here is a comical version of what I mean:
http://www.cplusplus.com/forum/lounge/176684/#msg872881

Instead of a set function, consider using a constructor with a member initialization list

1
2
3
4
5
6
7
 
CRA_Account::CRA_Account(const std::string& familyNameArg,  // pass std::string by reference
                      const std::string& givenNameArg,   // I use this format when there are multiple parameters
                      int SocialInsuranceNumberArg)
                      : // colon introduces member initialization list
                      lastName(familyNameArg),  // try to be consistent with variable names, I would have familyName as the member variable
                      firstName (givenNameArg)
                      {}   // do validation inside the braces


A member initialization list is more efficient because class members are initialized once during creation of the object, as opposed to being initialized with default values first, then reassigning values to them again.

Good Luck !!
Last edited on
Topic archived. No new replies allowed.