Ok Chris,
I got a chance to look at your Pong game. Here's some feedback if you're interested.
Assets
- Don't duplicate art assets just to put an arrow in a different place in the menu. Instead, make the arrow it's own sprite, create one asset for the menu, then draw the arrow next to the menu item that is currently active.
- Unless you're looking for very stylized text, it's often better to use SpriteFont to draw text against menu background, rather than putting the text in the image directly. Then, if you decide to change the text, localize it, etc... it just requires changing a line of code, not re-visiting an image editing program.
- SpriteBatch works by drawing textures to the screen via 2D pre-transformed quads. Each time you switch textures for a different sprite, it has to do a sort of context switch. So instead of creating sprites as different image files, put them all in the same image file and use source rectangles to identify where in the sprite sheet to pull the texture from. This works for sprite sheets up to about 1024x1024. Anything bigger and you risk video cards not being able to load it effectively.
Objects & Classes
In your program you group data together such as this:
[source lang="csharp"]int ballSpeedX = 5;
int ballSpeedY = 1;
int ballDirectionY = -1;
int finalBallLocationX = -1;
int finalBallLocationY = -1;[/source]
This is the perfect place to create a Ball class. You can add to it [font=courier new,courier,monospace]ballTexture[/font] as well.
In your update method you have several if-else statements to determine what the current state is and then process the logic for that state. Additionally, in your Draw method you have several if statements for what to draw. Instead, look at the State Machine design pattern. Each of those states can be their own class, and then inside of [font=courier new,courier,monospace]Game.Update()[/font] you'd just call [font=courier new,courier,monospace]curretState.Update()[/font]. In [font=courier new,courier,monospace]Game.Draw()[/font] you'd call [font=courier new,courier,monospace]curretState.Draw()[/font]. The logic for switching between states is then embedded within each state, and your Game's Update and Draw methods become nice and clean.
Code Cleanliness
You call [font=courier new,courier,monospace]KeyboardState gameState = Keyboard.GetState();[/font] in each of your game state blocks. Instead, just call it once at the beginning of your update method, and then use the keyboard state as necessary in the rest of the method.
Even if you don't use an object to represent your game state code, you should switch from String to Enum for your game states. Enum is more type-safe, as string will allow the state to be literally any string value.
Your use of [font=courier new,courier,monospace]lastState [/font]and [font=courier new,courier,monospace]state [/font]can be restructured so you don't really need lastState except when entering or leaving a menu screen.
I'm not quite sure why you have [font=courier new,courier,monospace]finalBallLocation[/font]. I see you calculate it's value in several places, but don't immediately see it being used for much.
There are several complicated if-statements, especially in the collision detection section. It's a good idea to make the code self-documenting to break those clauses up into smaller chunks and wrap them in methods.
Perform static calculations once, and save the values. There are several places where you perform the following:
ball.Y = Window.ClientBounds.Height / 2 - ballTexture.Height / 2;
ball.X = Window.ClientBounds.Width / 2 - ballTexture.Width / 2;
pos1.Y = Window.ClientBounds.Height / 2 - texture.Height / 2;
pos2.Y = Window.ClientBounds.Height / 2 - texture.Height / 2;
Not only is the code present in your source file more than once, but it's called each time you enter the restart state, or exit state, even though it's a known value from the moment your application loads the texture. So instead, perform the above calculations in Initialize or LoadContent so you only perform the calculations once.
The same goes for things like this:
spriteBatch.Draw(player2Restart, new Vector2(Window.ClientBounds.Width / 2 - player1Main.Width / 2, Window.ClientBounds.Height / 2 - player2Main.Height / 2), Color.White);
That's a lot of calculations being performed each Draw, considering those are all known values. Compute them once, then keep re-using the calculated value. This is easiest if you create a Sprite class. Then you can create an instance of a Sprite for each object, set it's texture, position, etc... once, and just keep calling Draw on the sprite.
Commenting
Your code is over 800 lines, and there's about 5% commenting. Most of which are method headers that were auto-generated for you. If you're doing a complex if-checks, or if something isn't immediately readable, add a comment. Also, it's good to use comments to tell
why you're doing something, not just
what.
