Looking for constructive criticism

This is a discussion on Looking for constructive criticism within the C++ Programming forums, part of the General Programming Boards category; This is the final assignment for my introductory c++ class. I decided to continue learning on my own, so I'm ...

Looking for constructive criticism

This is the final assignment for my introductory c++ class. I decided to continue learning on my own, so I'm gonna start by rewritting my project. I would greatly appreciate any feedback/constructive criticism you guys can provide.

Please, bare in mind; the course did not cover, classes, inheritance or any kind of object oriented programming. In fact, we only learned about data types, operators, basic input/output, control structures, functions, arrays and data structures.
The little I know about any other topic, I learned it indirectly, on my own, by trying to solve the problems the project offered.

I hope this also can help future students, scavenging the web for answers, to get a general idea of how this can be approached, and avoid being as lost as I was when I first started. Word of caution though, the professor sends all submitted code through a code analyzer that compares your code with a database of previous submissions. Any similarity higher than 60% will raise red flags all over and you will most likely get an F. So take the concepts and implement them in your own way.

I suggest you use parentheses when mixing AND and OR operators to insure you are properly evaluating these statements in the proper order. I don't recommend you rely on operator precedence as the proper parentheses make your intent much clearer.

Also I'm not too fond of using the global variables for your structure instances. I would make them local to a function and pass them to the functions as required.

Also I would consider wrapping some of your long lines to make reading your program a bit easier.

Problem is that whether char is signed or unsigned by default is implementation defined AFAIK.
So to get around it, if you want to use char, is to explicitly use unsigned char.

Aside from the global variables, just scanning through the code, if you have C++11 support in your compiler, consider replacing C-arrays with std::array and NULL with nullptr.
I also see inconsistencies. You use "using namespace std;", yet you explicitly refer to, for example, cout as std::cout. Why? Is there a reason you are using "using namespace std;" if you want to explicitly qualify the namespaces?
In your functions that take a string, consider making them into an enum and using a case. It's much more efficient and faster.

I don't agree that classes and inheritance should be taught early in a C++ class, or that failing to do so is "backwards".

In this example, I'm more concerned about the use of static variables, and the fact of duplication of low-level operations that could be moved to separate functions. The usage of enums as array indices is a really bad habit to get into. Rather than using lots of if/else blocks testing strings, I'd look into using arrays (or maps), and using some algorithm (or simply a loop construct) to do the heavy lifting. This code is therefore a lot more error prone (hard to get right) than it needs to be. If you want to clear the screen, there are better ways than system("cls") .... albeit all of them just as non-portable as system("cls").

If I seem grumpy or unhelpful in reply to you, or tell you you need to demonstrate more effort before you can expect help, it is likely you deserve it. Suck it up, Buttercup, and read this, this, and this before posting again.

That is a VERY VERY useful compiler you have. I got none of such warnings,

A signed character can only hold values between -127 to 127 so a value of 193 is too large for this data type.

The professor specifically asked for the ┴ symbol to be used. According to the ASCII table, the numeric value for that symbol is 193. What you say however would explain why the symbol was never properly displayed in my machine. (Though it did on windows.)

This warning message is telling you there is something wrong with your if statement on line 365 and you probably want to look into that problem because it shouldn't be an empty body.

Again that is a NICE catch. I am loving that compiler.
Indeed that is a mistake, and one that brought me many headaches.

I suggest you use parentheses when mixing AND and OR operators to insure you are properly evaluating these statements in the proper order. I don't recommend you rely on operator precedence as the proper parentheses make your intent much clearer.

I will do that from now on. Thanks.

Also I'm not too fond of using the global variables for your structure instances. I would make them local to a function and pass them to the functions as required.

Do you mean all the structure members or just the static constants in "board"

Also I would consider wrapping some of your long lines to make reading your program a bit easier.

Splitting statements like that is one of the things I learned on my own. Rather late into the project. I went back and used it on drawMap(), but I guess I missed
a few other long lines.

You may also want to consider breaking up your moveSub() function into smaller sub functions.

That is the first thing I'm doing in my rewrite.

Overall I would say for an introductory level class this program seems well done.
Jim

Thank you.

Originally Posted by Elysia

Problem is that whether char is signed or unsigned by default is implementation defined AFAIK.
So to get around it, if you want to use char, is to explicitly use unsigned char.

Aside from the global variables, just scanning through the code, if you have C++11 support in your compiler, consider replacing C-arrays with std::array and NULL with nullptr.
I also see inconsistencies. You use "using namespace std;", yet you explicitly refer to, for example, cout as std::cout. Why? Is there a reason you are using "using namespace std; if you want to explicitly qualify the namespaces?
In your functions that take a string, consider making them into an enum and using a case. It's much more efficient and faster"

I have no idea, what the using namespace std" statement does. I always thought it had to do with "standard spaces" ???. In any case when I started my project in linux, the starting console project had this "std::cout <<"hello world" <<std::endl;" line, and I noted that a simple cout, wouldn't work. So I started using "std::"

Not long after starting, I switched over to DevC++, cause the professor said we should use that and only that IDE to avoid unnecessary problems. The starting console project in DevC++ did have the "using namespace std" line. I never put much thought into it, since the previous code worked fine, so I kept on using std::.

Originally Posted by oogabooga

This is a very C-ish C++ program.
Why structs instead of classes?
Why are you are using globals?.
Why shorts instead of ints?

The course did not cover classes. We couldn't use them and if we did we had to got to the professor's office and demonstrate our knowledge of classes in front of him.
I know i shouldn't use globals, but I used just one, and specified the only place where it is modified. isn't that what is hold against global vars - that they can be modified everywhere in the program- ?

We used shorts in most of the programs in class. Shorts occupy only 2 bytes in memory, instead of the 4, integers take. And the numbers used are so small, that they fit more than comfortably in a short var.

You are creating a structure inside of another structure, again I've never seen that before.That's a very clever way to do it

Originally Posted by grumpy

In this example, I'm more concerned about the use of static variables, and the fact of duplication of low-level operations that could be moved to separate functions.

I'm not too happy with that duplication of code either. I'm gonna see how can I make it into functions.

Rather than using lots of if/else blocks testing strings, I'd look into using arrays (or maps), and using some algorithm (or simply a loop construct) to do the heavy lifting

Are you suggesting to use an array to store the message strings, and then comparing the indexes in a loop.?
That would save me the time when checking, but I would still have to assign the string to each array index manually. Unless I use numbers instead of strings, as arguments for message, in which case the program would lose readability.
message("gameOver") is more meaningful that message(7).

The usage of enums as array indices is a really bad habit to get into

Would you please explain why, and show me an alternative way to do it.

That is a VERY VERY useful compiler you have. I got none of such warnings,

You need to read the documentation for your compiler. Compilers "out of the box" often do not give detailed warnings or diagnostics. However, there are ways to configure most modern compilers so they do give detailed diagnostics.

It is generally considered good practice to both use your compiler in a way that it gives as many warnings or diagnostics as possible, and to write your code well enough that the compiler does not complain.

This code both specifies a struct type named vessel, and two instances of vessel named sub and destroyer. Defining a type at file scope is a good idea as it allows the type to be reused. Defining instances is not. So I would find a way to tease out the type definition

Code:

struct vessel
{ short ammo;
short wepDamage;
short hullIntegrity;
short movePoints;
short position[2];
short heading;
short *destination;
bool visibility;
char shape;
};

and then create sub and destroyer within some function, and pass them as arguments to functions, rather than simply having their names globally available to all functions.

Note also, that if you are passing larger structs to a function, it is often practically a good idea to pass them by reference rather than by value.

Code:

void do_something(vessel &sub)
{
// do things to the submarine, that may or may not change its state
}
void do_something_else(const vessel &sub)
{
// do things to members of sub that do not change the sub
}

Originally Posted by skyliner

I have no idea, what the using namespace std" statement does. I always thought it had to do with "standard spaces" ???. In any case when I started my project in linux, the starting console project had this "std::cout <<"hello world" <<std::endl;" line, and I noted that a simple cout, wouldn't work. So I started using "std::"

A namespace is, in mathematical lingo, a space containing names. std is the name of a namespace. std::cout is an object named cout, within namespace std. If you had another namespace, say X, it could also contain something named cout. X's cout is distinct from std's cout, and is accesses as X::cout.

A "using namespace std;" tells the compiler to treat all names within the std namespace as candidates later on. After a "using namespace std;", the statement "cout << x;" will view std::cout as a candidate match for "cout" (assuming <iostream> has also been #include'd).

The problem with "using namespace" is that they cause ambiguity. Let's say you have

In this case both std::cout and X::cout are candidates to match "cout". The compiler has no way of knowing which, so will complain about ambiguity. To resolve that, you either need to remove one or both "using namespace" directives, or use the full name (say std::cout) that you intend.

Generally, it is considered a good idea to either use using directives, or fully qualify your names. But you can do both.

If I seem grumpy or unhelpful in reply to you, or tell you you need to demonstrate more effort before you can expect help, it is likely you deserve it. Suck it up, Buttercup, and read this, this, and this before posting again.

When you use namespaces you have to resolve the name to use any of the stuff inside. This is why you see foo::bar in places.

The using namespace directive assists the programmer: where you type the name of something, when the compiler looks for a definition during the compile process, it will consider that namespace. It works for any degree of scope (file, function, block, etc). That is precisely why it is used by newbies the way it is. Everything you need typically is in std, so that gets exposed for entire files.

You can also expose a single function or class from a namespace with a using directive.

Code:

using std::string;
using std::fabs;

Both things come with many caveats:
0. Many times, in many places, "using namespace ..." is a bad idea because two names will be used in two different namespaces.
1. "using namespace ..." is a bad idea in header files since it will apply wherever the header is included, which will eventually collide.
2. You will probably collide with something with the same name if you don't know what you're doing. Everyone has a story about sometime this happened to them.
...
Mine was Rectangle.

That is a VERY VERY useful compiler you have. I got none of such warnings,

If you are using Dev-C++ you are probably using the same basic compiler (although an earlier version). The difference is I am compiling with more warnings enabled. I suggest you read your compiler manual to determine how to enable more warnings.

Originally Posted by skyliner

The professor specifically asked for the ┴ symbol to be used. According to the ASCII table, the numeric value for that symbol is 193. What you say however would explain why the symbol was never properly displayed in my machine. (Though it did on windows.)

There are a couple of problems here. First in order to use the "characters" above 127 you must use an unsigned char for the data type. Second these extended characters are not always available, because there were many different extended character sets. For example look at the differences between these characters in the following two links: ASCII TABLE and ASCII CODE and ASCII Comparison. So as you can see with one character set you get one set of extended characters while with the other character set you get something different.

Originally Posted by skyliner

We used shorts in most of the programs in class. Shorts occupy only 2 bytes in memory, instead of the 4, integers take. And the numbers used are so small, that they fit more than comfortably in a short var.

Actually the C++ standard doesn't guarantee this. A short and an int are both only guaranteed to hold a value of 32767. Today unless you are programming for a severely memory constrained system I recommend you use the int. The int is usually considered the optimum type for the processor.

The professor specifically asked for the ┴ symbol to be used. According to the ASCII table, the numeric value for that symbol is 193. What you say however would explain why the symbol was never properly displayed in my machine. (Though it did on windows.)

OK, so first, you can just type '┴', not 193. Makes it easier to read, don't you think?
Second, the problem with extended characters. They are not standard. There are a lot of so called extended code pages which define these characters above the first 128. It is likely that Linux and Windows does not use the same codepage. Therefore, what shows up on Linux using these extended character sets may show up wrong on Windows and vice versa.
So what do you do about it? You use Unicode. For starters, read this: The Absolute Minimum Every Software Developer Absolutely, Positively Must Know About Unicode and Character Sets (No Excuses!) - Joel on Software
If you can't use Unicode, then complain to your Professor that you shouldn't use it. If that doesn't work, then drop the course. If you can't do that, then you will just have to accept what problems this brings. Just be aware of why it is happening, why you should never use extended character sets and why your Professor is stupid unless he/she has a very good excuse for teaching it. Don't use it in real world programs. Use Unicode, like the rest of the modern world.

Not long after starting, I switched over to DevC++, cause the professor said we should use that and only that IDE to avoid unnecessary problems. The starting console project in DevC++ did have the "using namespace std" line. I never put much thought into it, since the previous code worked fine, so I kept on using std::.

Don't use Dev-C++. It's old and unmaintained. If your Professor recommends you to use it, ignore him/her. If your Professor requires you to use it, then complain. If that doesn't work, drop the course. If you can't do that, then use, but know that whenever you possible can, you should drop it and get a modern IDE. Here is a small list of modern IDEs, just for reference: SourceForge.net: Integrated Development Environment - cpwiki

I know i shouldn't use globals, but I used just one, and specified the only place where it is modified. isn't that what is hold against global vars - that they can be modified everywhere in the program- ?

There are mainly two problems:
- Your program modifies these variables from different functions, making the flow of the program difficult to ascertain, making optimizations very difficult, and making it difficult to verify code correctness.
- Your program have the ability (but does not necessarily do it) the variable from multiple functions. This opens bugs which the compiler cannot catch because the variable can be modified from every function, but should not necessarily be. Some function may inadvertently modify a global variable when you did not intend do, leading to a possibly difficult to track bug.

We used shorts in most of the programs in class. Shorts occupy only 2 bytes in memory, instead of the 4, integers take. And the numbers used are so small, that they fit more than comfortably in a short var.

What is 2 bytes vs 4 bytes in today's memory? Besides, 2 bytes mean you have a lower margin for overflow, possibly leading to bugs (though, I admit, that is a weak argument).
But, point is, unless you are really tight on memory, don't bother with such optimizations.

i wasn't aware I could make functions inside of structures. I'll look into it.

Well, to be honest, those are usually known as classes.

I'm not familiar with the operator "::"

It's called the namespace operator. It's used to access types inside namespaces, as well as structs and classes.

Are you making a structure a member of another structure. ?
That is some clever use of structures. I've never seen that before.

You are creating a structure inside of another structure, again I've never seen that before.That's a very clever way to do it

It's good practice, since the struct itself (or later, classes, as you shall learn) knows better than the outside world what data it stores, and therefore, it can define some types that makes sense to use to manipulate its data. It increases abstraction (and thus, code quality).

I'm not too happy with that duplication of code either. I'm gonna see how can I make it into functions.

Are you suggesting to use an array to store the message strings, and then comparing the indexes in a loop.?
That would save me the time when checking, but I would still have to assign the string to each array index manually. Unless I use numbers instead of strings, as arguments for message, in which case the program would lose readability.
message("gameOver") is more meaningful that message(7).

Would you please explain why, and show me an alternative way to do it.

Also, the idea was that instead of
message("gameOver")
you would do
message(GAME_OVER);
(Possibly message(Msgs::GameOver) if you have C++11 support; see strongly typed enums.)
Where GAME_OVER is a constant from a Messages enum.

It's called the namespace operator. It's used to access types inside namespaces, as well as structs and classes.

I've never heard it called that before. It is usually referred to as the "scope resolution" operator, and is used for resolving names within named scopes (a scope is sometimes called a context in natural english). A namespace is one type of named scope. The name of a struct or class is another type (where the name is that of the struct/class type).

If I seem grumpy or unhelpful in reply to you, or tell you you need to demonstrate more effort before you can expect help, it is likely you deserve it. Suck it up, Buttercup, and read this, this, and this before posting again.

OK, so first, you can just type '┴', not 193. Makes it easier to read, don't you think?
Second, the problem with extended characters. They are not standard. ... So what do you do about it? You use Unicode. If you can't use Unicode, then complain to your Professor that you shouldn't use it. If that doesn't work, then drop the course.

This can't be worth more than 3 points on the rubric. By all means drop the course because you'll get an A- ...