C++

Forum

 
Ways to improve project..highly recommend negative critiscms

Frooster (28)
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
 
/*****************************************************************
 *	Chat Program in C++                                      *
 *		Developed By:			                 *
 *                    Frooster                                           *
 *****************************************************************/
#include "head.h"

//must include <mutex> and <synhapi.h>

/*******************************
 * Pointer variable to Users:  *
 *	* -> User #1 or A          *
 *	# -> User #2 or B          *
 *******************************/

//Random Declaraions 
using namespace std;
#define SIZE 100
mutex mtx;
bool written=true;

//Global variables
char text[100];
ifstream ifile("chat.txt",ios::ate);
ofstream ofile("chat.txt",ios::ate);
int len;

//Function for User A
void userA(){
	
	do{
		cout<<"You:";
		cin.getline(text,SIZE);
		strcat(text,"*");
		
		do{
			try{
				written=true;
				mtx.lock();
				ofile<<text;
				mtx.unlock();
	    	}catch(exception e){
	    		continue;
	    		written=false;
			 }
		}while(!written);
		
		memset(text,'\0',SIZE);
		
		while(1){
			memset(text,'\0',SIZE);
			try{
				mtx.lock();
				ifile.getline(text,SIZE);
				len=strlen(text);
				mtx.unlock();
			}catch(exception e){
					continue;
			 };
			if(text[len]=='*')
				continue;
			else if(text[len]=='#')
				break;
			Sleep(3000);
		};
		cout<<text;
	}while(strcmpi(text,"bye") || strcmpi(text,"bye"));
	
	ifile.close();
	ofile.close();
	system("pause");
	exit(0);
}

//Function for User B
void userB(){
	
	do{
		while(1){
			memset(text,'\0',SIZE);
			try{
				mtx.lock();
				ifile.getline(text,SIZE);
				len=strlen(text);
				mtx.unlock();
			}catch(exception e){
					continue;
			 };
			if(text[len]=='#')
				continue;
			else if(text[len]=='*')
				break;
			Sleep(3000);
		};
		cout<<"He:"<<text;
		
		memset(text,'\0',SIZE);
		
		cout<<"You:";
		cin.getline(text,SIZE);
		strcat(text,"#");
		
		do{
			try{
				written=true;
				mtx.lock();
				ofile<<text;
				mtx.unlock();
	    	}catch(exception e){
	    		continue;
	    		written=false;
			 }
		}while(!written);
		
		memset(text,'\0',SIZE);
	}while(strcmpi(text,"bye") || strcmpi(text,"bye"));
	
	ifile.close();
	ofile.close();
	system("pause");
	exit(0);
}


int main(int argc, char* argv[]){
	
	if(argc<2){
		cout<<"Failed to get User Priority";
		cout<<"Enter User Priority in Command Line Argument"<<endl;
		cout<<"Input A/a for User Priority #1"<<endl;
		cout<<"Input B/b for User Priority #2"<<endl;
		cout<<"Aborting Program Now..."<<endl;
		system("pause");	
		//delay(3000);
		exit(0);
    }
    
	if(strcmp(argv[1],"a")){
		userA();
	}
	
	if(strcmp(argv[1],"b")){
		userB();
	}
	
	else{
		cout<<"Wrong User Priority.."<<endl;
		//delay(3000);
		system("pause");
	}
	return 0;
}
Last edited on
codekiddy (783)
maybe first step would be to edit your post and format posted code with code tags <>

ex:
int x;
SamuelAdams (1535)
Any code with system("pause"); is a failure.
You don't even tell the user to hit any key to continue.
cire (8284)
Any code with system("pause"); is a failure.

That seems overly harsh.

You don't even tell the user to hit any key to continue.

The pause command tells the user to hit any key to continue...
jlb (4973)
//must include <mutex> and <synhapi.h>
So where are these includes? Relying on some compiler magic to #include the necessary include files is a bad habit. Always #include all the necessary include files in every file, don't rely on some other file including these files, even if you wrote the file.

1
2
3
4
5
6
7
8
 
mutex mtx;
bool written=true;

//Global variables
char text[100];
ifstream ifile("chat.txt",ios::ate);
ofstream ofile("chat.txt",ios::ate);
int len;

Bad, bad, bad. Using global variables should be avoided. Make these local to some function and pass them to any required functions.


Last edited on
Shadowwolf (168)
Your while(1) loop halfway your code can be replaced into a loop that has a condition. Currently you have a break condition at the end of your loop, you could easily refactor this into a loop with a condition, this makes your code much more readable, as people can directly tell for how long your loop will execute.
dhayden (5798)
Here is your code indented and in code tags. See comments below.
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
 
/*****************************************************************
 * Chat Program in C++ *
 * Developed By: *
 * Frooster *
 *****************************************************************/
#include "head.h"

//must include <mutex> and <synhapi.h>

/*******************************
 * Pointer variable to Users: *
 * * -> User #1 or A *
 * # -> User #2 or B *
 *******************************/

//Random Declaraions
using namespace std;
#define SIZE 100
mutex mtx;
bool written=true;

//Global variables
char text[100];
ifstream ifile("chat.txt",ios::ate);
ofstream ofile("chat.txt",ios::ate);
int len;

//Function for User A
void userA(){

    do{
        cout<<"You:";
        cin.getline(text,SIZE);
        strcat(text,"*");

        do{
            try{
                written=true;
                mtx.lock();
                ofile<<text;
                mtx.unlock();
            }catch(exception e){
                continue;
                written=false;
            }
        }while(!written);

        memset(text,'\0',SIZE);

        while(1){
            memset(text,'\0',SIZE);
            try{
                mtx.lock();
                ifile.getline(text,SIZE);
                len=strlen(text);
                mtx.unlock();
            }catch(exception e){
                continue;
            };
            if(text[len]=='*')
                continue;
            else if(text[len]=='#')
                break;
            Sleep(3000);
        };
        cout<<text;
    }while(strcmpi(text,"bye") || strcmpi(text,"bye"));

    ifile.close();
    ofile.close();
    system("pause");
    exit(0);
}

//Function for User B
void userB(){

    do{
        while(1){
            memset(text,'\0',SIZE);
            try{
                mtx.lock();
                ifile.getline(text,SIZE);
                len=strlen(text);
                mtx.unlock();
            }catch(exception e){
                continue;
            };
            if(text[len]=='#')
                continue;
            else if(text[len]=='*')
                break;
            Sleep(3000);
        };
        cout<<"He:"<<text;

        memset(text,'\0',SIZE);

        cout<<"You:";
        cin.getline(text,SIZE);
        strcat(text,"#");

        do{
            try{
                written=true;
                mtx.lock();
                ofile<<text;
                mtx.unlock();
            }catch(exception e){
                continue;
                written=false;
            }
        }while(!written);

        memset(text,'\0',SIZE);
    }while(strcmpi(text,"bye") || strcmpi(text,"bye"));

    ifile.close();
    ofile.close();
    system("pause");
    exit(0);
}


int main(int argc, char* argv[]){

    if(argc<2){
        cout<<"Failed to get User Priority";
        cout<<"Enter User Priority in Command Line Argument"<<endl;
        cout<<"Input A/a for User Priority #1"<<endl;
        cout<<"Input B/b for User Priority #2"<<endl;
        cout<<"Aborting Program Now..."<<endl;
        system("pause");
        //delay(3000);
        exit(0);
    }

    if(strcmp(argv[1],"a")){
        userA();
    }

    if(strcmp(argv[1],"b")){
        userB();
    }

    else{
        cout<<"Wrong User Priority.."<<endl;
        //delay(3000);
        system("pause");
    }
    return 0;
}

//Random Declaraions This comment isn't helpful.
1
2
 
//Function for User A
void userA(){
The comment doesn't tell me anything that I can't get from the function name. The comment should say what the function does.

General comment: If your intention is to make this multithreaded eventually so that both userA() and userB() run simultaneously then you should make text a local variable. Otherwise the two function can clobber it.

Line 23: Should be char text[SIZE];
Line 33: Make sure you have the right size here, especially since you're about to append an extra character.
Line 44: This code is never reached.
Lines 55 & 56. Swap these. There's no need to keep the length under a lock.
Line 60: if you caught an exception then len won't be set correctly.
Line 60: If you len == strlen(text) then text[len] is '\0'.
Line 60: If the user continues to a new line then the previous line is lost.
Line 67: why do you check for "bye" twice?


Gyiove (186)
-jlb
Way must we avoid creating global variables?
Thanks!
Moschops (7244)
Way must we avoid creating global variables?


Because we will lose track of what is changing them and what is reading them and our code will end up heaving with bugs that are nightmarishly hard to track down. Is it possible to avoid this? Yes. Are we smart enough and dedicated enough to avoid this? No.
jlb (4973)
Global variables, when improperly used as in this program, make following the program logic much harder and should be avoided. There are times when a global or two are acceptable but those times are fairly rare.

Also creating instances of complex classes in the global scope should be avoided. For example

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

ifstream ifile("chat.txt",ios::ate); // Open these files in global scope? Why the ate flag on an input stream?
ofstream ofile("chat.txt",ios::ate); // Why not open them in main(), check that they properly opened, and pass them into the functions as required.

...

//Function for User A
void userA(){

...
                ofile<<text;  // This function  expects that ofile is open and in a good state, note the lack of any checks for this fact.
...

...
    ifile.close(); // Close the global instance of input file, What if user B still needs the file?
    ofile.close(); // Why is this being done in this function instead of the calling function? Or better yet allowing the destructor to do it's job?
...
}

//Function for User B
void userB(){
...
                ifile.getline(text,SIZE); // No checks that the read succeeded, or that the stream is in a good state before or after this operation. 

...
                ofile<<text;

...
}


Also note that using the C function exit() should be avoided in C++ programs, this C function doesn't properly handle C++ class destructors and can lead to problems.
Frooster (28)
-jlb, -dhayden, thank you so much for pointing out the mistakes patiently....i have rectified em all

-codekiddy done! ;)

can the logic of the program be improved?
Topic archived. No new replies allowed.