Code review for the monthly challenge.

Started by
5 comments, last by Gureen Ryuu 5 years, 12 months ago

Hey all. I'm not sure if this is the right place to post it, but I'm almost done with the monthly game challenge, I still have one more requirement to finish. But before I do that I wanted to see how well I've written my code so far, and if I've done anything wrong or how I can improve myself. Because after all, if I can't write a game as simple as Pong right, I'll definitely have trouble with bigger games.

The game with done with Unity/C#. Here is the link for the source code: https://github.com/gureen-ryuu/LetUsPlayPong/

 

Any comments or criticism is much appreciated. :)

Advertisement

:)

The game is so near perfect that there isn't much to say.

There is only two things I would change:

Your AI always tracks the ball, this means that to win the ball needs to reach a very high speed. I would have damped the movement at the start, slowing down the AI to make it easier to beat the AI.

Then as the player builds up there score, I would have reduced the damping effect. So that I had a AI that got harder to beat as the game progresses.

 

It's hard to follow the ball when a point was just scored. I would have reseted the ball to the way it was when the game starts.

 

On 11/8/2017 at 7:39 PM, Gureen Ryuu said:

But before I do that I wanted to see how well I've written my code so far

Great code, it was easy to reed and understand.

I just feel that in your BallController you could have merged some functions into cleaner and easier to read functions. For example the inverse functions could have been one function.

But if you prefer it this way then keep it like is but some comments would help then.

 

 

Your game is well made, very professional.

Assuming your code works for you, that's a good first step.  A game that works is better than a game that doesn't. Players generally don't see or care about the game code.

 

Like a few others who requested reviews of their code, it looks like you have a few places that could be consolidated or eliminated. 

In your menu scripts you have two single-function scripts and one empty script.  Often a Unity UI script has a single class that responds to many different inputs, and buttons/sliders/etc from many UI panels all use that single script for their events. 

You also have many one-line functions through the code that could probably be consolidated or eliminated. For example, colliding with a wall calls HandleEnterWallCollision(), which only has one line to call BounceBallAgainstWall(), which only has one line to call InverseVerticalMovement(), which only has one line to ChangeBallSpeed(), which only has one line to pass along to a different form of ChangeBallSpeed() that optionally caps the ball's speed by calling a one line RectifyBallSpeed() that changes a parameter to consolidate two values and pass along to a different RectifyBallSpeed(), which immediately splits out those same two parameters.  Why all the redirection?  If it bounces of a wall you know you need to play a sound and reverse vertical movement, nothing more.  If you bounce of a paddle you play a sound, reverse horizontal movement, shift the direction of travel, and ensure the speed is appropriate. No need for the long chain of functions.

Along the same theme, you've broken up functionality that could be handled more generically.  For example, the queries of CanMoveUp() and CanMoveDown() do nothing but return flags. Those flags are set and cleared every frame, even if they are never queried. Personally I'd rather allow code to send the move commands all they want, but when they set it always clamp the results to within the valid range. You only incur the cost of checking against up or down if they actually attempt to move, and that cost is a single clamp call.

The way you use Unity's 2D collision system means you end up with more scripts than you probably need. In Pong the only thing needing collision response is the ball.  You've got them also on the PongController and the Goal objects. The ball controller already detects against wall and pong tags, you could add a goal tag to handle those, and remove the other two responses.

Dump the Start() and Update() functions if you aren't actually using them. Unity automatically generates them since they're used in so many scripts, but they have a small cost.

You have several variables that probably should be private --- you don't want others messing with the variable --- but you want them exposed in the inspector so you can attach a GameObject.  Keep them private, but add the [SerializeField] attribute above the variable.  For example:


public GameObject pauseMenu;
// could become
[SerializeField]
GameObject pauseMenu;

In larger projects you will want to wrap the scripts in namespaces. Sadly Unity doesn't automatically do this, by default new scripts are placed in the global namespace.  Your project is small so it isn't really need it, but it is something to know about as your projects grow.

 

Since the game works for you that can be good enough. Most of these changes are minor structural changes to simplify the code.

Hello,Which version of Unity3D do you use?

 I am using 4.7 and open the crash。

Hey all, sorry for the late replies! Life happened and things got hectic.

I'm very moved by all your detailed and well thought out replies. I've read your posts and taken them to heart, and will do my best to try and follow the good practices you've explained in my next project.

I'm still going to try and go back to basics, despite my intermediate experience. I've gotten a couple of courses for Unity and I hope I learn things from the ground up the right way for better code.

 

Thank you again so much for all your help. I know it must have taken you a long time to write. You're the best.

On 12/3/2017 at 6:40 PM, Zhao JieMin said:

Hello,Which version of Unity3D do you use?

 I am using 4.7 and open the crash。

I'm using Unity 2017/2018. 4.7 is quite old I think.

This topic is closed to new replies.

Advertisement