Code review size

[Davitosan]

Hello! I'm currently learning C++ along with SFML, and I've been working on a space shooter. I would like to post the code to hear your suggestions and corrections. I just wanted to know if it's OK to post around 250 lines of code here.

Thanks.

[Disch]

250 might be a little much for a forum post.

As an alternative you can use a paste site like www.pastebin.com. Paste it there, then just post a link to it on here.

[Browni3141]

We've seen longer here. It's okay.
On second thought 250 is a lot. If there are smaller fragments you need help with post them.

[Davitosan]

Thanks. I'm not familiar with pastebin, but I'll give it a shot. Let me just comment the code and I'll post the link here.

[Davitosan]

The code is ready! It's in pastebin in the following address:

http://pastebin.com/kAS9E164

Basically, the program compiles and works as expected, but I'm sure there is a lot of room for improvement and optimization.

For example, I've seen on this forum that the use of system() is not recommended. Is there any non-recommended practices that I'm doing in my program?

Just a quick overview and opinion will suffice.

Thanks in advance!

EDIT: Don't expect a complete space shooter! At this stage, you can fire you weapons and move the player. Also, an enemy sprites moves down the screen, but that's it. I haven't implemented pretty much anything yet. I wanted to see if I was heading in the right direction first.

[Disch]

I'll check it out when I get home. It's hard to give it a good review at work =x

[Return 0]

Without looking further here is my first bit of feedback. You are writing a C++ program using a C++ API. Don't #define, use const and use classes not structs. I realize they both get the job done, but it's a C thing.

Edit: Also, it looks like this is all in 1 file? You should start to use object oriented principles and break out your program into multiple files. It's something you should get used to. Separate interface from implementation.

[ne555]

enum could work too.

void Move()
{
	// The player moves the ship by pressing the left or right arrow keys. 
	//It stops when it reaches the predefined borders of the screen.
	if ( window.GetInput().IsKeyDown(sf::Key::Left)

You better use events than polling.
Check if you can define a function that is called every time that an key is pressed (a keyboard callback) and from there distribute your program.

Edit: how can I copy easily from pastebin?

[Davitosan]

Thanks for the replies!

@Return 0: What do you mean, separating interface from implementation? I haven't been using multiple files, but if I did, I guess I would put functions in one file, classes in another, and main in yet another one. I'm not really sure in which other way to split the program.

@ne555: I think I can implement your suggestion. I can't get to it right now, but I'll post the upgraded version of the code as soon as I write it, also using const and classes.

Thanks again!

[Disch]

I don't suppose you feel like uploading the images somewhere too, so we can try it out. ;D

[Davitosan]

Sure thing! It was about time I got a rapidshare account anyways. Here's the link:

http://rapidshare.com/files/444919968/images.tar

It's just programmer art, but it does the trick. I hope you don't mind it's .tar :D

[Disch]

Those graphics are way better than the crap I do. Nice job.

I'm getting access violations when I run it (not crashing the program, but they're showing up in the debugger). I haven't pinpointed what's causing it, but I'd wager it has something to do with your bullets (you really shouldn't be using pointers for those ;P)


Other than that, really the only thing is organizational/stylistic stuff. Criticism to follow -- please don't be offended or anything, I'm not trying to be judgemental -- I just figured you posted this thread because you're looking for this kind of honest feedback.


0) Return 0 is right -- don't use #define unless you have to

1) Some of your functions are a bit haphazardly designed and overly complicated. Take for instance the Shoot function:

/*
	The player shoots by pressing the Space key.

	The shooting function needs to check for three main states. First, which bullet are we going to fire? That's the reason behind the for loop. Second,
  does this bullet already exists? That's the reason behind the (!bulletExists[i]). And third, has the last bullet moved up enough for the next shot to
  be fired (has moved beyond the 370 'y' value.
*/

void Shoot()
{
	for (int i = 0; i < 3; i++)
	{
		if ( (i != 0) && (!bulletExists[i]) && (bulletExists[i-1]) )
		{
			if ( (window.GetInput().IsKeyDown(sf::Key::Space)) && (bullet[i-1]->y < 370) )
			{
				bullet[i] = new BulletStruct;
				bulletExists[i] = true;
			}
		}
		else if ( (i == 0) && (window.GetInput().IsKeyDown(sf::Key::Space)) && (bulletExists[i] == false) )
		{
			bullet[i] = new BulletStruct;
			bulletExists[i] = true;
		}
	}
}

The first thing that has me scratching my head here is that you're checking the status of the space bar. Since the function name is "Shoot" I would expect it to do just that: shoot a bullet. Not check to see whether or not the user pressed the space bar and shoot only if they pressed it.

Basically your function name is misleading. Or you should have designed the function to do less.

Right now you're calling Shoot unconditionally and having it check user input. Instead, I would conditionally call it (based on user input) and have it unconditionally shoot. Something like this:

if(WasSpaceJustPressed())
  Shoot();

Then all Shoot() has to do it what you'd expect: shoot a bullet.


The next thing that strikes me as odd about this function is.. why do you care about bullet[i-1]? You seem to be checking its state in the loop, but why?

Also, what does the Y position have to do with anything? Isn't the bullet removed once it goes offscreen?

A simpler approach would be this:

void Shoot()
{
  int slot = FindOpenBulletSlot();  // returns -1 if no available slot

  if(slot >= 0)
  {
    // shoot bullet[slot] here
  }
}

continuing with that... FindOpenBulltSlot could be simplified:

int FindOpenBulletSlot()
{
  for(int i = 0; i < 3; ++i)
  {
    if(!bulletExists[i])  // if it doesn't exist, it's an open slot
      return i;
  }

  // if all bullets exist, there's no open slots
  return -1;
}

(although even this is not ideal, see #3 below)

KISS. Your functions are trying to do too much. Break it up into smaller jobs. A function should do 1 clear thing.

A general rule of thumb is if you find yourself using && or ||, you're doing it wrong. (Of course that's not always the case, but && and || are very much overused by beginners).

Other functions have similar problems, but this was a good example one to show you what I mean.

2) Don't get cute with variable names. Example:

	sf::Image Nme1;
	sf::Image Nme2;

It took me a minute to figure out that NME wasn't an acronym for something, but actually was a leetspeak way to type 'enemy'. Don't do that.

Also don't name things foo1, foo2, foo3. If you find yourself doing that, you're doing it wrong. Use arrays.

    sf::Image enemy[2]; // much better 

3) Avoid globals / You could stand to objectify more. You seem to be doing the C-style faux-OOP where you make a bunch of structs, then make global functions that work with those structs.

A better approach is to use classes. This also helps get rid of all your globals (globals should be avoided!)

Take a look at your MoveBullet function:

void MoveBullet(int i)

Seems harmless for the most part, but it assumes too much. It assumes there's a global array of bullets and that they can be indexed. What if you decide to change that later? What if you get rid of the globals? Then this function needs to be rewritten. What if you decide to use a std::list of bullets instead of an array? Have to rewrite it. What if you decide to add another player and they both have their own set of bullets in two separate arrays? Have to rewrite it.

This is one of the big problems with globals is that they make [i]the entire program[/i] be aware of them. This means you're stuck with it. What's worse, code you write that uses globals can't be reused because it was written to work ONLY with globals.

A smarter approach would be to pass the bullet to the function, rather than having the function find the bullet on its own:

void MoveBullet( BulletStruct& bullet )

This is much better. Then you can just modify 'bullet' as you see fit, without having to worry about accessing a global array. Then you can get rid of or change the global array at any time.

Of course if you're doing that, then you might as well make MoveBullet a member function, which would be the next step up:

void BulletStruct::Move()

That's about all I can think of for now. Keep up the good work =)

[Davitosan]

Thanks a lot, Disch. Far from being offended, I really appreciate your thorough response. It reminded me of a quote I saw on this forum a few days ago:

It is practically impossible to teach good programming to students that have had a prior exposure to BASIC: as potential programmers they are mentally mutilated beyond hope of regeneration. --- Edsger Dijkstra

I hope he was exaggerating : ) I began programming with QBasic, and procedural programming kinda stuck with me.

I still haven't fully understood OOP yet, as you clearly pointed out. Is there any reading material that you could recommend? I'm pretty much on my own here, and since my programs end up doing what they're supposed to do, I can't really tell where I'm going wrong.

Well, I have to sleep now, and I have to work tomorrow, but as soon as I can apply all these suggestions I'll repost the code, in case anybody is interested in my progress (especially all those newbies like me, who would benefit from my experience)

EDIT: I already changed the structs to classes, and #define to const. Next stop is making the function which reads the keyboard input, in order to condition the access to input-related functions.

[Disch]

I still haven't fully understood OOP yet

I didn't really fully understand it either until just a few years ago. It's kind of hard to really "get it", but then one day it just clicks and makes perfect sense.

EDIT: maybe it was longer ago than I thought -- I just realized how old I'm getting =x /EDIT

Is there any reading material that you could recommend?

Sadly, no. Not because there's no good reading material -- I just haven't read it. Maybe someone else here will have some suggestions for you.

Pretty much all of what I know is from conversations with other programmers and my own experimentation. I haven't actually read a book on the topic of programming since my very first C++ w/ MFC book that got me started 14+ years ago (although I've skimmed a few e-books).

and since my programs end up doing what they're supposed to do, I can't really tell where I'm going wrong.

That's what makes it so tricky. Strictly speaking there's nothing wrong with your program now. It runs just fine and isn't doing anything criminally inefficient. So, really, you don't have to change anything.

Where that comes to bite you, though, is trying to expand or change. Right now you're just slapping it together as you go, which works fine until you hit a certain size. Then you find out that to add the next feature you want, you have to change something else in your program, which is next to impossible because you make reference to that one thing in 500 different places and you'd have to change them all to change it. You might find that it's easier to scrap the project and start over than to mess around and try to work in a new feature.

That's the goal of OOP and encapsulation. You separate the program into specific classes. Each class does its own thing, and the rest of the program doesn't have to care about how the class works, it just has to know how to use it. That way if something changes, you only need to change the appropriate class. How the class is used doesn't change -- so the code you have to change is minimized and all in the same area.


Really, the first step to getting on "the right path" is to eliminate the use of global vars. Globals are maintainance nightmares because the whole program is accessing them all the time, which means they're rigid and can't be easily changed.

Instead of having functions work with globals, pass whatever you want the function to use to the function as a parameter. That makes it much more flexible, and it allows for more code re-use.

So yeah, that's what I recommend you do next: get rid of the globals.

[z05DSL3A]

Davitosan wrote:

I still haven't fully understood OOP yet, as you clearly pointed out. Is there any reading material that you could recommend?

It may be worth taking a look at the following but it would be worth 'trying before buying' as I know some people did not like it.

The Object-Oriented Thought Process
Matt Weisfeld
http://www.amazon.com/Object-Oriented-Thought-Process-3rd/dp/0672330164/ref=sr_1_1?ie=UTF8&qid=1296213054&sr=8-1