@toad1359
Sorry to be off topic by posting a big critique of your code (if you post code, be prepared for comment), but my motivation is to help you out dude! If you want, start a new topic, discussion can continue there, rather than here.
EDIT: I hate it when huge cout's make a topic 4 screens wide !! You can build up output in parts you know, no need to have a massive long cout.
With this:
281 282 283 284 285 286 287 288 289 290
|
if (scbp == "sneak=E FOA=N") {
if (sneak_num == 1 or 2 or 3 or 4 or 5) {
cout << "Your sneak attack succeeds at getting into the village from the east side. They have the objective of raising the gate. Your other troops attack from the north side after the sneak attack and allow the sneak attack to enter unnoticed. The sneak attack raises the gate and you get in. You immediatly go after the leader and kill him. By killing the leader you became the new leader and all the other troops surrender.";
fmlead();}
return; }
if (scbp == "sneak=E FOA=N") {
if (sneak_num != 1 or 2 or 3 or 4 or 5) {
cout << "Your sneak attack gets shot at and dies. Your other troops attack from the north side after the sneak attack. Your soldiers can't break down the gate and die. You also die. Please try again.";
return; }}
|
The
or
operators are OK, but the way you use them is invalid. For example line 282 will always be true, because 2,3,4,5 all evaluate to true. Instead of having an else to do something when it's false, you have another if statement that does the inverse of the first one. So better IMO to do this:
1 2 3 4 5 6 7 8 9 10
|
if (scbp == "sneak=E FOA=N") {
if(sneak_num >= 1 or sneak_num <= 5) {
cout << "Your sneak attack succeeds at getting into the village from the east side.";
return;
}
else {
cout << "Your sneak attack gets shot at and dies. Your other troops attack from the north ";
return;
}
}
|
There are several things that could improve your code IMO.
It seems to me that the whole
cbpon
function (whatever that means) is about actions carried out when
sneak
and
FOA
(whatever they are) are one of N,E,W,S (directions?). So there are 16 combinations. You could put these combinations into an
enum
, then use the
enum
value in a
switch
statement. This would be tidier IMO, and would save all those
return;
statements, and you won't need the code to build the string. Speaking of those returns, I think I mentioned elsewhere why that is unnecessary at the end of a void function.
Can you come up with better names for variables & functions? One shouldn't have to guess what they mean - we are not mind readers you know? At the very least comment them.
Instead of explicitly cout'ing all the text, consider putting it all into an array or vector of
std::string
, then you could just
std::cout
an array or vector element. This will keep the
switch
tidy.
1 2
|
//so i don't have to type std::cout
using namespace std;
|
You can do this:
1 2 3 4 5 6
|
using std::cout;
using std::cin;
using std::endl;
using std::string;
using std::vector;
// other std things here
|
Then just use cout etc unqualified through your code. This is much better than bringing in the whole std namespace - which
WILL cause you problems one day.
Don't use global variables - they are a really bad idea.
With this:
1 2 3 4 5 6 7 8
|
if (answer == "E")
scbp = scbp + "E";
if (answer == "W")
scbp = scbp + "W";
if (answer == "S")
scbp = scbp + "S";
if (answer == "N")
scbp = scbp + "N";
|
could be replaced by this:
1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16
|
bool ValidInput = false;
char Direction = 'E'; // used this instead of answer string
while (!ValidInput) {
cout << "On the east, west, north or south side? (E/W/N/S)";
getline (cin, Direction); // does answer have to be a string - why not char?
answer = toupper(Direction); // needs char type to work
if (Direction == 'E' or Direction == 'W' or Direction == 'S' or Direction == 'N') {
sbcp += Direction; // just append it, string has += operator
ValidInput = true;
}
else {
std::cout << "Invalid Input - Enter E or W or N or S " << "\n";
}
}
|
That was just to show a better way of doing that particular thing, but as I said earlier that whole function isn't necessary.
I eventually found the main function on line 476!! I prefer to put functions declarations first, then main, then function definitions at the end. That way it is easy to see what main does without having to search for it, or scroll down 600 LOC to see it. Also try to declare & define functions in the order they are called if possible - your first function is
orc
, so that should be at the top of the list in the definitions. Otherwise, when looking at your code, I find myself scrolling up & down madly to see how things work.
The placement of braces is confusing. Put the closing brace directly under (in the same column) as the statement that caused the opening brace. In the
orc_sanctum
function, the closing brace on line 579 belongs to the
if
on line 567 - can you see how that is confusing? Also, proper indentation aids in understanding as well. I always use braces - even when there is only 1 statement - it will save you one day when you add more code. When using an IDE, you should be able to get it to format things automatically, and on demand.
Hope all these suggestion might help you write better code.