C++

Forum

 
need reviews

philo mallik (3)
i wrote a program that accept a string and manipulate it
i need people reviews and ideas to make it better
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
161
162
163
164
165
166
167
168
169
170
171
172
173
174
175
176
177
178
179
180
181
182
183
184
185
186
187
188
189
190
191
192
193
194
 
#include <iostream>
#include <iomanip>
#include <cctype>
#include <string>
using namespace std;

const int MaxChar = 81;  

int CountWords(char string[MaxChar]);
int CountConst(char string[MaxChar]);
void JumbleString(char string[MaxChar],int length);

int main()
{
	int choice = 0;  //declaring variable choice that store the choice of the user
	char string[MaxChar];    //declaring an array in which the user enter the string,
	char string2[MaxChar];   //declaring a second array needed in some manipulation
	int  count = 0, numofwords, numofconst;   // declaring a count varible, and two other variables that store the number of words and the number of constant

	cout << "Enter a string: ";   //asking user to enter the string
	cin.getline(string, MaxChar, '\n');  //getting the string from the user

	int length = strlen(string);   //setting a variable that contain the length of the string it will be needed
	int end = length - 1;
	string2[length] = '\0';
	while (choice != 9)  //a loop where user cant come out unless choice is 9
	{
		cout << "USETHIS MENU TO MANIPULATE YOUR STRING" << endl  //printing out to the screen the options that the user can use
			<< "--------------------------------------" << endl
			<< "1) Inverse String"<<endl
			<< "2) Reverse string" << endl
			<< "3) To uprercase" << endl
			<< "4) Jumble string" << endl
			<< "5) Count number words" << endl
			<< "6) Count constants" << endl
			<< "7) Enter different string" << endl
			<< "8) print the string" << endl
			<< "9) Quit"<<endl;
		cin >> choice;

		switch (choice)  // a switch statement to determine what to do in which case 
		{
		case 1:
			count = 0;
			while (string[count] != '\0')
			{
				if (islower(string[count]))
				{
					string[count] = toupper(string[count]);
				}
				else if (isupper(string[count]))
				{
					string[count] = tolower(string[count]);
				}
				count++;
			}
			cout << "the string is inversed" << endl;
			break;
		case 2:
			count = 0;
			for (; count != length; count++)
			{
				string2[count] = string[end];
				end --;
			}
			string2[length] = '\0';
			strcpy_s(string, string2);
			cout << "the string is reversed" << endl;
			break;
		case 3:
			count = 0;
			for (; string[count] != '\0'; count++)
			{
				string[count] = toupper(string[count]);
			}
			cout << "the string is changed to uppercase" << endl;
			break;
		case 4:
		JumbleString(string,length);
			break;
		case 5:
			numofwords = CountWords(string);
			cout << "the number of words is " << numofwords << endl;
			break;
		case 6:
			numofconst = CountConst(string);
			cout << "the number of constants is " << numofconst << endl;
			break;
		case 7:
			cout << "the other string is: ";
			cin.ignore();
			cin.getline(string2,MaxChar,'\n');
			int l;
			l	=strlen(string2);
			string2[l] = '\0';
			strcpy_s(string, string2);
			length = strlen(string);   //reputting the new lengthin the  variable that contain the length of the string
			string2[length] = '\0';
			break;
		case 8:
			cout << "the string is: " << string << endl;
			break;
		}
	}
	return 0;
}

int CountWords(char string[MaxChar])
{
	int numofwords = 0;
	int start = 0;
	for (; string[start] != '\0'; start++)
	{
		if (isspace(string[start+1])&&!isspace(string[start]))
		{
			numofwords++;
		}
	}
	if (string[0] != ' ')
	{
		numofwords++;
	}
	return numofwords;
}
int CountConst(char string[MaxChar])
{
	int numofconst = 0;
	int start = 0, end = strlen(string) - 1;
	for (; string[start] != '\0'; start++)
	{
		if (tolower(string[start]) == 'b' 
			|| tolower(string[start]) == 'd' 
			|| tolower(string[start]) == 'c'
			|| tolower(string[start]) == 'd' 
			|| tolower(string[start]) == 'f'
			|| tolower(string[start]) == 'g'
			|| tolower(string[start]) == 'h'
			|| tolower(string[start]) == 'j'
			|| tolower(string[start]) == 'k'
			|| tolower(string[start]) == 'l'
			|| tolower(string[start]) == 'm'
			|| tolower(string[start]) == 'n'
			|| tolower(string[start]) == 'p'
			|| tolower(string[start]) == 'q'
			|| tolower(string[start]) == 'r'
			|| tolower(string[start]) == 's'
			|| tolower(string[start]) == 't'
			|| tolower(string[start]) == 'v'
			|| tolower(string[start]) == 'w'
			|| tolower(string[start]) == 'x'
			|| tolower(string[start]) == 'y'
			|| tolower(string[start]) == 'z')
		{
			numofconst += 1;
		}
	}
	return numofconst;
}
void JumbleString(char string[MaxChar],int length)
{
	char string2[MaxChar];   // an aray to store the jumbled string in then copy it back to the main array
	string2[length] = '\0';
	
	int value[MaxChar];      //array to store the random numbers in in
	value[length] = '\0';
	
	int count = 0;   //a variable that count the number of characcters in array setting a value for each one
	int x;       //variable to store random numbers in
	int start = 0;
	bool check; //boolean to check or number is already used
	
	srand(time(NULL));   //generate random numbers:
	
	for (; count<length; count++)                //count the number of characcters in array setting a value for each one
	{
		
		do{
			x = rand() % length;
			check = true;    //assuming the boalean is true
			for (start = 0; start < length; start++)  //check or number is already used:
			{
				if (x == value[start]) //if number is already used
				{
					check = false; //set check to false
					break; //no need to check the other elements of value[]
				}
			}
		} while (!check);    //loop until new, unique number is found
		value[count] = x;        //store the generated number in the array
		string2[count] = string[x];  // use the number to set a value in the array
	}
	cout << "the jumbled string is: " << string2 << endl;  //print the jumbled version to the screen
}
TheIdeasMan (6817)
Hi,

First up, it doesn't compile:

In function 'int main()':
23:28: error: 'strlen' was not declared in this scope
67:28: error: 'strcpy_s' was not declared in this scope
In function 'int CountConst(char*)':
128:36: error: 'strlen' was not declared in this scope
128:17: warning: unused variable 'end' [-Wunused-variable]


So you need to #include the files for those.

Is this an assignment? If so, what were the specific requirements ? Are you allowed to use std::string - you have it included. If you can there are lots of handy things in that container. You could avoid C-style coding then.

Just on that, don't #include <string> then use string as a variable name.

Are you allowed to use the STL? There heaps of really good algorithms in there that would radically change your code.

To be honest, you have some other terrible variable names: numofconst and CountConst - apparently that means : number of consonants not number of constants

And here:

1
2
3
4
5
6
7
 
for (; string[start] != '\0'; start++)
	{
         if (tolower(string[start]) == 'b' 
			|| tolower(string[start]) == 'd' 
			|| tolower(string[start]) == 'c'
			|| tolower(string[start]) == 'd' 
			|| tolower(string[start]) == 'f'


start is a bad name in this context, you increment it, so it is no longer the start. Perhaps Position might be better?

IMO, these confusing names come from the misguided idea that one needs to abbreviate variable names everywhere.

A for loop is not good on line 129, for loops are best when one knows exactly how many times to loop. Consider using a while loop there.

With lines 131 to 152, consider negating the test for a vowel, rather than testing for all the consonants. It's easier to test for it not being one of 5 things, rather than it is being one of 21 things. One can negate a condition like this if !(condition) {}

This:
numofconst += 1;

is better written, with the variable name change and the increment operator:

++NumOfConsonants;

With your switch , you show an option 9 on the menu, but there is no case 9: !! I know you have the while condition on line 26, but IMO it is better to do this:
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
 
bool Quit = false;
while (!Quit) {
   // show menu
   // get menu choice
   switch (choice) {
      // the options as you have them
      // ...
      // ...
      case 9:
          Quit = true;
           break;
      default:  // catch bad input
          std::cout << "Bad Input Try Again\n";
          break;
   } 
}


Each of you switch cases should call a function to do their work, this makes the code much easier to read.
This:
1
2
 
count = 0;
			for (; count != length; count++)


should be:
for (int count = 0; count < length; count++)

Important to understand the difference there.

Some style points:

Your code will display a lot better on this forum if you configure your editor to convert tabs to 4 spaces say. This site converts tabs to 8 spaces, so that results in much more unwanted indenting.

Consider putting comments before statements, not at the end of them. And split long comments into separate lines. Thus avoid having code which is 2 screens wide when posted here. There is a kind of a guide that code should not be more than 80 chars per line. Although it is not something to be set in stone, it does have some advantages, most notably where there is no word wrap. Note the compiler doesn't care if you press the enter key in a statement, it is looking for semi-colons to end statements. I make use of that all the time, especially for functions with multiple parameters: I put each parameter on it's own line.

Good Luck !!

Some of the comments are worthless:

1
2
 
check = false; //set check to false
cout << "the jumbled string is: " << string2 << endl;  //print the jumbled version to the screen


jlb (4973)
The first thing I notice is this:
1
2
3
4
5
6
 
#include <string>
using namespace std;

const int MaxChar = 81;  

int CountWords(char string[MaxChar]);


First naming a variable "string" in a C++ program is not a very good idea, especially since you #included the <string> header file. This header declares a class by the name "string" so you're probably going to have major problems.

Second why are you trying to use an array of char named string in the first place. I recommend you use a C++ string instead. Th string class has quite a few helper functions to make searching and parsing strings much easier.




booradley60 (896)
+1 on comments. Never use a banal comment in place of a good variable name.
TheIdeasMan (6817)
@philo mallik

Further to what booradley60 has just said :

Good code with good variable and function names should read like telling a story, here is a comical version of what I mean:

http://www.cplusplus.com/forum/lounge/176684/#msg872881
Topic archived. No new replies allowed.