Sign in to follow this  
Gaius Baltar

Missile Command code review request

Recommended Posts

Downloaded and ran just fine. Nice job. Good, simple sound effects and graphics. Good response. I like the mouseover sounds in the menu. Nice touch.

 

Really don't need the console window. I'm guessing you use that for debugging.

Share this post


Link to post
Share on other sites

Downloaded and ran just fine. Nice job. Good, simple sound effects and graphics. Good response. I like the mouseover sounds in the menu. Nice touch.
 
Really don't need the console window. I'm guessing you use that for debugging.

 
Thanks for trying it out!
Yeah, the console window is a little intrusive at times, but is required for score keeping in this case (also debugging). I suppose I just didn't want to write a class utilizing text objects to keep track of user input.
 
 

1) Don't start identifiers with '_'., In many scopes, with many different rules they are reserved for the implementation of the compiler and standard libraries. You'll run into something weird at some point.


First of all, thanks for taking the time to read through my code!
As to the underscore naming convention, could you suggest what I should use instead?
 

2) When defining a constant, prefer to use the correct constant types for the value so the compiler doesn't accidentally insert runtime casts when they're not needed.

const float GOOD = 1000.0f;
const float NOT_SO_GOOD = 1000;


I'll make sure to do that.

3) Style nitpick.  Prefer to place public class members first, protected second, private last.  Anyone browsing your code is going to want to see the public interface to the class, and only dig into the details of how that is implemented after reading the public interface.


That's interesting, so far I've always written my classes: private > protected > public. I'll keep that in mind for my next project.
 

4) In Entity.h.  Prefer to reserve() your vectors when you know the rough size you expect to avoid odd hickups as you allocate new objects.  Vector is amortized O(1) inserts, meaning that some inserts take longer, but on average it comes out to constant time.


I never really got into reserving memory for vectors before... I'll have to look that up, but does it really make in impact on performance to reserve vector space?
 

5) For your particles class, if you don't actually care about the ordering, you can make your remove function faster by std::swap()ing.

std::swap(vector.begin() + index, vector.begin() + (vector.size() - 1));
vector.pop_back();
this avoids having to copy everything past the deletion point.


Wow, that actually makes way more sense than my code, but wouldn't it be vector.end() and not vector.begin() + (vector.size() - 1)? Or is there some sort of difference?
 

6) Button.h.  Rule of Three.  If you must implement the copy operator, you probably need to implement a copy-constructor and destructor.


You're right about the copy constructor, but I believe sfml takes care of deallocating it's own allocated memory, so the destructor wouldn't really be necessary(?) I could implement it anyway, just to stay consistent with the C++ law.
 

7) Missiles.h, you probably want to be returning a reference (or const reference) from getExplosions() to avoid unnecessary copies.


I thought you only had to do the const reference thing on parameters, but I suppose now that you mentioned it, it kind of does make sense to return a const reference as well.
 

8) Missiles.cpp:70, you should always include { } on your if/else/for/etc. constructs.  It helps you avoid errors.


Yeah, biggrin.png I keep telling myself that, because most of the time I end up expanding my if statements anyway, but I suppose in some cases it's just faster to not include the curly brackets. I think I'll eventually find an equilibrium between curly brackets and no curly brackets.
 

9) CollisionDetection.cpp:29.  There's no need for the sqrt.  You can compare squared values on both sides  (a2 + b2 = c2, so no need to square root if both sides are squared)


Woops.

Share this post


Link to post
Share on other sites


Wow, that actually makes way more sense than my code, but wouldn't it be vector.end() and not vector.begin() + (vector.size() - 1)? Or is there some sort of difference?

vector.end() is

1) you could do (vector.end() - 1) as well.  The end iterator is one-past the end of the array, and does not point at a valid object.

2) It's all relative to the container.  Not all containers provide iterators that support (end() -1).  Vector happens to be a random-access iterator, so vector.begin() + x is just as fast as vector.end() - y.  So yes, you could base the second point off the end iterator.

Share this post


Link to post
Share on other sites

You can also use:

std::swap(vector[index], vector.back());
vector.pop_back();

Note that in 2D games, this can cause sprites to "pop" over one another. For small particles in large swarms, this probably isn't going to be noticeable, but for key entities in a game, it might not be worth it.

 

This is the kind of consequence that caused Kulseran to note:


If you don't actually care about the ordering...

Share this post


Link to post
Share on other sites

Here are my thoughts. Overall, the code is reasonably good and mostly easy to understand and free of obvious bugs.
 
 init.h isn't a particularly good name, as the contents of this header aren't just initialisation related

 Headers like "init.h" are not scaleable as your project grows. Better to have smaller, more focused headers

 It looks like you're header structure is not standard. Ideally, a header file can be included into any source file in isolation without causing compile errors. It appears your Missile_Command header cannot be included in isolation as it depends on items that are included via init.h. The most common C++ convention is that classname.cpp #includes classname.h as the very first action in the file, which guarantees this property.

 
So, I should make multiple headers?
includes.h and constants.h?
 
But I also don't really see how "init.h" isn't scalable, is it because of the merging of includes and constants in a single file, or is there another reason?
 
Even with the "classname.cpp #includes classname.h" thing, wouldn't it still be the same? Because the classname.h still might depend on stuff from init.h.
Or am I getting this wrong somehow?

 

const float SECOND = 1000;
const float BLOCKSIZE = 20;
const int GRIDSIZE_X = 50;
const int GRIDSIZE_Y = 30;
const int SCRW = (int)BLOCKSIZE * GRIDSIZE_X;
const int SCRH = (int)BLOCKSIZE * GRIDSIZE_Y;
const double INTERVAL = 16.6; // 1000 / INTERVAL = FPS -> ~60
Some of these constants have poor names. Instead of SECOND, try MILLISECONDS_IN_SECOND. SCRW should probably be more descriptive, I'd guess SCREEN_WIDTH but perhaps this has another purpose. INTERVAL is fairly meaningless, perhaps MILLISECONDS_PER_FRAME or something might be better. Also, consider deriving your constants like so:

const int MILLISECONDS_IN_SECOND = 1000;
const int TARGET_FRAMES_PER_SECOND = 60;
const double MILLISECONDS_PER_FRAME = MILLISECONDS_IN_SECOND / (double)TARGET_FRAMES_PER_SECOND;

 
Yeah, I try to spend the least amount of time on naming my variables, if I don't think of a name fast, it might get a bad name tongue.png.

 

Your Missile_Command class is enormous. Consider splitting it into multiple classes, for example, one (or more) for handling the menus, another to handle the actual game, another to handle the high score list, etc.

 
I know. After a while it got really confusing to try and find code I was looking for in that file, In my next project I'm going to have my entire GUI in a separate class, that should save me a few lines. 
 
 

Consider moving generic helper functions such as std::vector<std::string> split(std::string string, char splitParam); to a separate file as a free function, rather than as a member function of a large stateful class.

 
I was thinking of doing that, but then I didn't... I suppose it was too much work for me tongue.png, especially because it was only used for the high scores. 

 

It is good defensive coding to obey the rule of three even for classes like Missile_Command that are unlikely to be copied. It is always possible to make a mistake (e.g. intend to pass to a function by reference but forget the reference). However, given that the destructor is empty, consider just omitting the destructor instead.

 
 Yeah, KulSeran told me the same thing just about. And my question to him still stands:  "[background=#fafbfc]...I believe sfml takes care of deallocating it's own allocated memory, so the destructor wouldn't really be necessary(?)[/size][/background]". Is it always necessary to implement all three? Because in the case of the button class, the destructor would be empty.

 

Exiting the application because you cannot load a file isn't great. At the very least, consider writing a relevant log message to standard error or a dedicated error log file, so that when someone reports the game doesn't start for them, you can quickly rule in or out such issues.

 
 I remember someone posting on a previous thread of mine that I shouldn't keep the application running after an error has occured. Should I keep it running or not? I'm confused, but I suppose I could write the error to a error log file or something.

 

The class names EntityR and EntityC are poor. At this point in the review, I've not encountered their definitions so I cannot even guess what these are supposed to mean. Long names aren't all that difficult to type, in fact they can sometimes be easier than trying to remember how the name was shortened. Most IDEs will offer auto-completion too, so the length generally isn't a problem at all.

 
My attempt at trying to stay consistent with the sfml naming convention (mostly inspired by the sf::Vector<> class) tongue.png, I suppose I failed.

 

Be consistent with your naming. Missile_Command includes an underscore, but EntityR and EntityC do not.

 
 That's a mistake, I think most of the time I just completely forget that I'm following any kind of naming convention and just name the damn thing whatever seems right at the time tongue.png.
 
 

_bases[0] = EntityC(50, 10, sf::Vector2f(100, 520), _baseColor);
_bases[1] = EntityC(50, 10, sf::Vector2f(200, 520), _baseColor);
_bases[2] = EntityC(50, 10, sf::Vector2f(300, 520), _baseColor);
_bases[3] = EntityC(50, 10, sf::Vector2f(600, 520), _baseColor);
_bases[4] = EntityC(50, 10, sf::Vector2f(700, 520), _baseColor);
_bases[5] = EntityC(50, 10, sf::Vector2f(800, 520), _baseColor);
This could be a loop.

 

 
Huh, but how would I integrate the jump from 300 to 600? I can't think of a way.

 

You don't seem to be checking for errors loading your font. Even if you don't care that something isn't loaded, at least output a warning to standard error or a dedicated log file.

  
Woops, I think the problem here is that I used to have a text class. When I got rid of that I tried to rewrite everything into the button class, I suppose I just missed a few things on the way.
 
 

if (_playerName.size() <= 0) {
    // ...
}
How would playerName.size() be less than zero? A better condition might be:

if (_playerName.empty()) {
    // ...
}
You have this pattern in many other places too.

 

 
I keep forgetting some of these built in methods to make life easier... I'll implement that. 
 
 

Seeing as only the string value of "lastHovered" is used, you can avoid creating an entire Button for it and just have a string. An alternative might be to use a pointer.

 
I used a whole button class, because I thought there was a method to test if one class was equal to another, that failed, so I used the string value in the button, seems stupid now that I think about it.
 
 

void Missile_Command::removeEnemyAt(size_t index){
    _enemies.erase(_enemies.begin() + index);
    _enemyVel.erase(_enemyVel.begin() + index);
}
When you have code that seems to be maintaining parallel lists, it might mean you'd be better off with a single list of composite objects. That said, there are reasons to separate lists sometimes (e.g. structure of arrays vs array of structure layout).

 

 
That's what I did with the missiles, because I first had like five vectors just for the missiles in my Missile_Command class. But I didn't want to make a whole new class for the enemies, I suppose it slipped my mind that I could have easily used a structure to store both items...
 
 

I'll stop now, there is more to review, but that is a lot for the time being.
 
Much of this is stylistic, so don't worry about the volume of comments - people can have different preferences for solving similar problems!
 
Hope this helps...

 
Thanks for reading through my code, I appreciate the time that you took to do so. This review really gave me a further push in the right direction and I'll most likely use this as a reference for future projects. 
 
 

 

Wow, that actually makes way more sense than my code, but wouldn't it be vector.end() and not vector.begin() + (vector.size() - 1)? Or is there some sort of difference?

vector.end() is
1) you could do (vector.end() - 1) as well.  The end iterator is one-past the end of the array, and does not point at a valid object.
2) It's all relative to the container.  Not all containers provide iterators that support (end() -1).  Vector happens to be a random-access iterator, so vector.begin() + x is just as fast as vector.end() - y.  So yes, you could base the second point off the end iterator.

 

 
Oh, I thought vector.end() was just the last item in the array.

 

You can also use:
 

std::swap(vector[index], vector.back());
vector.pop_back();
Note that in 2D games, this can cause sprites to "pop" over one another. For small particles in large swarms, this probably isn't going to be noticeable, but for key entities in a game, it might not be worth it.

 

You're right, in my case it wouldn't matter for the _trails particles, because they all have a one pixel area and are mostly white.

 

Share this post


Link to post
Share on other sites

 

_bases[0] = EntityC(50, 10, sf::Vector2f(100, 520), _baseColor);
_bases[1] = EntityC(50, 10, sf::Vector2f(200, 520), _baseColor);
_bases[2] = EntityC(50, 10, sf::Vector2f(300, 520), _baseColor);
_bases[3] = EntityC(50, 10, sf::Vector2f(600, 520), _baseColor);
_bases[4] = EntityC(50, 10, sf::Vector2f(700, 520), _baseColor);
_bases[5] = EntityC(50, 10, sf::Vector2f(800, 520), _baseColor);
This could be a loop.

 

 
Huh, but how would I integrate the jump from 300 to 600? I can't think of a way.

For example, using an array.

for(int n=0; n<6; ++n) {
    static float const data[] = {100, 200, 300, 600, 700, 800};
    _bases[n] = EntityC(50, 10, sf::Vector2f(data[n], 520), _baseColor);
}

Maybe not worth it for such a small piece of repetitive code, but at least it begins to separates the data from the code.

Share this post


Link to post
Share on other sites

So, I should make multiple headers?

Not particularly. For games, I sometimes have a "constants.h" or similar. However, an alternative is to place the constants in the header or source file they are actually used, rather than moving them into a file full of unrelated values.

 

 


But I also don't really see how "init.h" isn't scalable, is it because of the merging of includes and constants in a single file, or is there another reason?

As the number of classes grows, your compile time will get worse and worse. Ideally, when you update a header, only source files that depend (directly or indirectly) on that header should need to be recompiled. When you have a single header that includes all of the classes, any change is going to cause all the files to be rebuilt.

 

 


Even with the "classname.cpp #includes classname.h" thing, wouldn't it still be the same? Because the classname.h still might depend on stuff from init.h.
Or am I getting this wrong somehow?

If each header and source file is conservative about what it includes, then it would not be the same - provided your classes don't have unreasonable dependencies on one another.

 

 


In my next project I'm going to have my entire GUI in a separate class, that should save me a few lines. 

It isn't just about saving lines, it is about manageable complexity. For example, one way to implement your GUI would be to have a class per "screen", so a main menu class, a high score class, etc.

 

 


 Is it always necessary to implement all three? Because in the case of the button class, the destructor would be empty.

The rule of three states that if a class has a destructor, copy constructor or an assignment operator, it probably should have all three. If a class needs none of these, then that is fine and consistent with the rule.

 

 


 I remember someone posting on a previous thread of mine that I shouldn't keep the application running after an error has occured. Should I keep it running or not? I'm confused, but I suppose I could write the error to a error log file or something.

It depends: how fun will the game be in the event of that particular error? If a sound fails to load, then you can probably continue. If the player's sprite fails to load, I'm not sure anyone will want to play the game.

 

Whether you continue or halt, you should log information. This will allow you to diagnose strange behaviour on other people's computers once you release your game. A dedicated log file isn't particularly hard to add, but writing to standard error is too easy to pass up - and you can easily make this output to a log file by running your game from the command line and using stream redirection!

 

 


My attempt at trying to stay consistent with the sfml naming convention (mostly inspired by the sf::Vector<> class)

It can make sense to take shortcuts with classes that would be used everywhere, that would otherwise have verbose names and that are unambiguous. For example, "Rect" is a frequent contraction of Rectangle, as there isn't much ambiguity here.

 

 


Huh, but how would I integrate the jump from 300 to 600? I can't think of a way.

It is possible, a simple example:

for (int i = 0 ; i < NUMBER_OF_BASES ; ++i) {
     int x = (i + 1) * 100;
     if (i > NUMBER_OF_BASES / 2) {
         x += 300;
     }
    _bases[i] = EntityC(50, 10, sf::Vector2f(x, 520), _baseColor);
}

That is actually less clear than the long way. I didn't actually spot that jump however. Even if you didn't have a jump, the code would not be much clearer with a loop, so don't worry.

 

 


But I didn't want to make a whole new class for the enemies...

Creating a class or structure can actually be easier than writing the code to maintain several arrays in parallel.

 

 


Thanks for reading through my code, I appreciate the time that you took to do so.

Glad to assist!

Edited by rip-off

Share this post


Link to post
Share on other sites

As the number of classes grows, your compile time will get worse and worse. Ideally, when you update a header, only source files that depend (directly or indirectly) on that header should need to be recompiled. When you have a single header that includes all of the classes, any change is going to cause all the files to be rebuilt.


I'm using Visual Studio, as far as I can tell it only compiles the files that have been edited, and if I'm not mistaken, I believe that it doesn't compile files just because they were included by another. Or, I could be totally wrong about that...


For example, using an array.
for(int n=0; n<6; ++n) {
static float const data[] = {100, 200, 300, 600, 700, 800};
_bases[n] = EntityC(50, 10, sf::Vector2f(data[n], 520), _baseColor);
}
Maybe not worth it for such a small piece of repetitive code, but at least it begins to separates the data from the code.


I never really considered the possibility of simply creating a data array to fill another array. Wouldn't it be expensive though to recreate the array every iteration of the loop?

Share this post


Link to post
Share on other sites


For example, using an array.
for(int n=0; n<6; ++n) {
static float const data[] = {100, 200, 300, 600, 700, 800};
_bases[n] = EntityC(50, 10, sf::Vector2f(data[n], 520), _baseColor);
}
Maybe not worth it for such a small piece of repetitive code, but at least it begins to separates the data from the code.


I never really considered the possibility of simply creating a data array to fill another array. Wouldn't it be expensive though to recreate the array every iteration of the loop?

 

The array is constant and has static duration. Your compiler will allocate it at compile time and there's zero additional cost at run-time other than actually accessing it from memory.

Share this post


Link to post
Share on other sites


I'm using Visual Studio, as far as I can tell it only compiles the files that have been edited, and if I'm not mistaken, I believe that it doesn't compile files just because they were included by another. Or, I could be totally wrong about that...

You can think of #include as basically copying the included file into the current file.

 

If "A.cpp" has #include "B.h", and something inside "B.h" changes, "A.cpp" will need to recompile.

Share this post


Link to post
Share on other sites

The array is constant and has static duration. Your compiler will allocate it at compile time and there's zero additional cost at run-time other than actually accessing it from memory.


Oh, I missed the static part. Thanks for clarification.

Share this post


Link to post
Share on other sites

You can think of #include as basically copying the included file into the current file.
 
If "A.cpp" has #include "B.h", and something inside "B.h" changes, "A.cpp" will need to recompile.


I figured that, but I thought Visual Studio somehow did it differently. Suppose not.

Share this post


Link to post
Share on other sites


I figured that, but I thought Visual Studio somehow did it differently. Suppose not.

You were right, from what I remember of VS and most other robust IDEs. If you edit, like Lactose! pointed out, a header file, then usually the IDE will recompile all files that include it, but there are some IDEs that will recompile everything. That said, it usually is a better idea to do a fresh rebuild of all files every once in awhile, just to be on the safe side.

Share this post


Link to post
Share on other sites

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now

Sign in to follow this