- Forum
- General C++ Programming
- Finished application, please rate
Finished application, please rate
Ok, so I just finished a flashcard application that takes input from the user, saves it to a flashcard file for use later, then displays it randomly, but never the same card twice. It then calculates your score and gives you a percentage.
I just started coding 3-4 months ago, so I'd like some feedback on how my coding is, whether or not I've fallen into some newbie pitfalls or not, and in general, how good or bad a job I did.
http://www.mediafire.com/?0il3by52zal
I just started coding 3-4 months ago, so I'd like some feedback on how my coding is, whether or not I've fallen into some newbie pitfalls or not, and in general, how good or bad a job I did.
http://www.mediafire.com/?0il3by52zal
First comment:
I've just had a quick look, so I haven't really studied your code. I just want to say that you shouldn't implement your functions in header files. You typically have one main.cpp file, and all other functions are implemented in header files included in that file.
This is bad, because:
* You can't use those header files in other cpp files, because you will get multiple definition errors.
* You always have to recompile everything. One of the most important reasons to separate code into multiple files is that when you change something, you only need to recompile the files you changed.
* It's just something we don't do!
I've just had a quick look, so I haven't really studied your code. I just want to say that you shouldn't implement your functions in header files. You typically have one main.cpp file, and all other functions are implemented in header files included in that file.
This is bad, because:
* You can't use those header files in other cpp files, because you will get multiple definition errors.
* You always have to recompile everything. One of the most important reasons to separate code into multiple files is that when you change something, you only need to recompile the files you changed.
* It's just something we don't do!
I've just figured out that the previous version didn't build right because of unsatisfied library dependencies. Here is a fixed version.
http://www.mediafire.com/?gkz90z2mzxe
http://www.mediafire.com/?gkz90z2mzxe
Some comments, in no particular order:
* Use STL data structures instead of fixed-size arrays. This alleviates your
problem of only 999 cards.
* Don't declare variables in header files. It's OK to extern them there, but
the variables should be instantiated only within .cpp files.
* Don't call main(). This actually is against the C++ standard and shouldn't compile.
* Use pre-increment instead of post-increment unless you absolutely need
post-increment. "i++" is post-increment; "++i" is pre-increment. Pre-increment
is generally more efficient and allows compilers to optimize better.
* Never use namespaces in header files.
* Most C/C++ programmers think zero-based, but many of your loops are 1-based.
* Use STL data structures instead of fixed-size arrays. This alleviates your
problem of only 999 cards.
* Don't declare variables in header files. It's OK to extern them there, but
the variables should be instantiated only within .cpp files.
* Don't call main(). This actually is against the C++ standard and shouldn't compile.
* Use pre-increment instead of post-increment unless you absolutely need
post-increment. "i++" is post-increment; "++i" is pre-increment. Pre-increment
is generally more efficient and allows compilers to optimize better.
* Never use namespaces in header files.
* Most C/C++ programmers think zero-based, but many of your loops are 1-based.
I Disagree with this.
* Don't declare variables in header files. It's OK to extern them there, but
the variables should be instantiated only within .cpp files.
Global variables should never be used. Ideally you should be developing in OO using encapsulation for safety and good design.
* Most C/C++ programmers think zero-based, but many of your loops are 1-based.
Even if most C++ thought of arrays in 1 based. The language has them in 0 based so it would be inefficient. All arrays start from 0.
* Don't declare variables in header files. It's OK to extern them there, but
the variables should be instantiated only within .cpp files.
Global variables should never be used. Ideally you should be developing in OO using encapsulation for safety and good design.
* Most C/C++ programmers think zero-based, but many of your loops are 1-based.
Even if most C++ thought of arrays in 1 based. The language has them in 0 based so it would be inefficient. All arrays start from 0.
I must say the code is pretty tidy. Personally, I'd prefer to see an Object-Orientated approach. But that is just part of the design and development methodologies that I use.
I did notice that you are not doing any thing with dynamic memory; so there are not a lot of pitfalls you can fall into just yet. Change everything to use Pointers and then you will start to find yourself falling into some holes :)
I did notice that you are not doing any thing with dynamic memory; so there are not a lot of pitfalls you can fall into just yet. Change everything to use Pointers and then you will start to find yourself falling into some holes :)
Correction;
* Never use namespaces in header files.
There are many way of "using" a namespace. I assume you mean never import a namespace into the global namespace, with a statement like "using namespace xxx".
Other ways of "using" namespaces are to declare classes inside namespace:
and using namespaces without importing to the global namespace:
Of course it's no problem with doing this in header files.
* Never use namespaces in header files.
There are many way of "using" a namespace. I assume you mean never import a namespace into the global namespace, with a statement like "using namespace xxx".
Other ways of "using" namespaces are to declare classes inside namespace:
|
|
and using namespaces without importing to the global namespace:
|
|
Of course it's no problem with doing this in header files.
Ok, one thing I've known for a while now is to start all my loops from zero. In fact, I've changed the most recent versions of my program to fix this. However, I don't understand what I did wrong with the namespaces & functions in header files. I believe you guys hundred percent that I did something wrong, I just don't understand why it's wrong to use namespaces and functions in header files, and I don't know how to fix it. If it's not too much trouble, would you mind explaining how it's wrong, and a possible solution?
Wrong:
Correct:
|
|
Correct:
|
|
Better yet, pass by const reference:
Lots of programmers assume that copying STL strings is fast. It happens to be, because most STL ports implement copy-on-write semantics, but there is nothing in the C++ standard that mandates that implementation, and if C++ ever truly supports concurrency the copy-on-write semantics will almost certainly change.
Actually the passing by reference thing goes for practically all STL data structures, not just strings. Either pass by const reference or non-const reference, never by value, unless your function needs to make a copy of it anyway.
|
|
Lots of programmers assume that copying STL strings is fast. It happens to be, because most STL ports implement copy-on-write semantics, but there is nothing in the C++ standard that mandates that implementation, and if C++ ever truly supports concurrency the copy-on-write semantics will almost certainly change.
Actually the passing by reference thing goes for practically all STL data structures, not just strings. Either pass by const reference or non-const reference, never by value, unless your function needs to make a copy of it anyway.
Topic archived. No new replies allowed.