Being the OCD guy I am, my code usually ends up looking very unclean and it seriously does bother me enough to make me want to quit working on it.
Can you guys give me some good tips on how to keep it clean looking?
What do you mean unclean? The file organization? The tabs and spacing?
Perhaps post some code and we can give you more info.
Follow basic coding conventions and never forget the following quote when things start to look grim:
[quote]There’s gotta be a better way.
[/quote]
To clarify: whenever you have to write something that looks ugly or just seems wrong there’s usually a better way of doing it.
On a slightly unrelated note, I just managed to finish a skeleton animation test program, which probably is the most hackish program I’ve ever written that actually runs. My main() function is colossal, all variables are static and I’ve got more temporary variables than I can count, but it works. Sometimes it’s okay that way though, since this abomination isn’t going into the final game anyway. Maybe that’s another good tip: Whenever you’re going to implement something new (and decently big), create a small stand-alone test program before you start integrating things into your game. It keeps your game code cleaner, and you’ll most likely discover any problems with your idea when making the test program. I’ve pretty much followed this idea since I started with OpenGL, since there are so many ways that things can go wrong and when they do you often just end up with a black screen. Simply leaking OpenGL state or forgetting a single method call can make your screen black, and it’s very time consuming to debug since if you want to System.out.println() some data you usually have to allocate buffers e.t.c to read back data from OpenGL. Sometimes you also have way too much data (for example a render target texture) so printing it as text isn’t going to help at all. Geh, now I’m just rambling again, but my point is: Go forward a little slower and catch problems earlier. It’s easier to find bugs in a small test program than in your complete game.
Like Loom_weaver wrote, do you have any examples of when your code looks “unclean”?
Like literally code.
E.g.
SpriteSheet sheet;
Animation playerDown = new Animation();
Animation playerUp = new Animation();
Animation playerRight = new Animation();
Animation playerLeft = new Animation();
Animation playerStillUp = new Animation();
Animation playerStillLeft = new Animation();
Animation playerStillDown = new Animation();
Animation playerStillRight = new Animation();
Animation currentAnim = playerDown;
int playerDir = 2;
public int ux = 0;
public int uy = 0;
int destinationX = 0;
int destinationY = 0;
int collidingWithInt = -1;
boolean moving = false;
public boolean interacting = false;
Polygon playerPoly;
public Player(int x, int y){
super(x,y);
ux = x;
uy = y;
playerPoly = new Polygon(new float[]{
x,y,
x+16,y,
x+16,y+16,
x,y+16
});
try{
sheet = new SpriteSheet("res/sprites/playerSheet.png",49,62);
for (int a = 1; a < 7; a++){
playerDown.addFrame(sheet.getSprite(a,0), 100);
playerUp.addFrame(sheet.getSprite(a,1), 100);
playerLeft.addFrame(sheet.getSprite(a,2), 100);
playerRight.addFrame(sheet.getSprite(a,3), 100);
}
playerStillDown.addFrame(sheet.getSprite(0, 0), 1);
playerStillUp.addFrame(sheet.getSprite(0, 1), 1);
playerStillRight.addFrame(sheet.getSprite(0, 3), 1);
playerStillLeft.addFrame(sheet.getSprite(0, 2), 1);
}catch(SlickException e){}
}
public void tick(Graphics g, Input input, BlockMap map, textBoxBase txbox, StateBasedGame sg){
g.drawAnimation(currentAnim, ux-16, uy-46);
try{
if (txbox.isDone()){
manageMovement(input, map);
manageInteraction(input, map, txbox, g, sg);
}
}catch(SlickException e){}
}
public boolean entityCollisionWith(BlockMap map) throws SlickException {
for (int i = 0; i < map.entities.size(); i++) {
Block entity1 = (Block) map.entities.get(i);
if (playerPoly.intersects(entity1.poly)) {
return true;
}
}
return false;
}
public boolean entityCollisionWithInt(BlockMap map) throws SlickException {
for (int i = 0; i < map.interactables.size(); i++) {
Interactable entity1 = (Interactable) map.interactables.get(i);
if (playerPoly.intersects(entity1.poly)) {
collidingWithInt = i;
return true;
}
}
return false;
}
//And etc.
This feels quite unorganized to me.
For the most part, it doesn’t look too bad. There are several broken conventions that I can see, however it doesn’t look much worse than a lot of the ‘good’ code that gets put into the actual Java API.
Teach me the ways of making it look quite nice then, please. lol.
catch(SlickException e){}
That’s something you need to fix right there. If you ignore exceptions, your failure at that point will be replaced by mysterious NPE’s later. If you really need some boilerplate handler to shut up the whole checked exception thing, then fill it in with “throw new RuntimeException(e)”. But fergodsake, don’t silently ignore exceptions.
throw new RuntimeException(e) will shut up Eclipse then? The only reason that’s there is because it complains it’s not. These errors don’t tend to happen, so normally I just put a println command and use system.exit.
Yes, it will shut up Eclipse. It effectively turns a checked exception into an unchecked one. That unchecked exception should be allowed to propagate all the way up and kill your program with a traceback. For a GUI app, you should catch it, show an error screen with the traceback, then exit.
A pure style thing I see is that entityCollisionWith could use a foreach loop instead of a C-style loop (the other one uses the index, so just leave it as-is). As for code organization in general: it’s rare you’ll ever write code that’s so perfect that you never feel tempted to tweak or trim or rearrange it some way. As a rule, programmers tend to be OCD cases, comes with the territory. The real skill to learn with experience is knowing when to stop.
Following up on what Skyaphid and Sproingie just said…
Well, it’s more that you’re catching an error and then doing nothing with it than anything else. It’s bad form to do that.
At the very least, you should put a comment there stating that it’s intentional so that when you look back at it (Or others view it) they won’t go “huh”. Also, since SlickException covers such a large area of possible exceptions, it’s not a good plan to use a blanket catch like that if you’re not going to pass it on up. If you have some sort of logging functionality (Even just Java’s basic Logger), then you should output some information about the exception so that you can figure out if it was actually an exception or just something you want to ignore (I admit I tend to use Integer.parseInt(String s) surrounded by a catch(NumberFormatException nfe) {} for the purposes of checking text input that’s supposed to be an integer).
And here I might be a little mean, too specific in my picking, but…
Another issue is, you have several ‘Magic Constants’ sprinkled through out your code. I see a bunch of +16 (In your constructor), hard-coded values for your sprite size (Line 42), hard-coded guards in your iterator in your constructor, a for-loop iteration that starts at 1 (Which I’ve had to do {PreparedStatements, something for parameterizing SQL statements, are indexed at 1 instead of 0}, but since convention is that iterators tend to start with the 0th element, starting at any other number, especially hard coded in, you should put a note to ensure people know why). In and of themselves, these aren’t necessarily bad things. They’re just things that you should watch out for, since any changes in say… Whatever the +16 is, the size of your sprite sheet, the number of possible sprites in your sheet, will force you to search through your code to find these places and change them.
The only other thing that jumps out at me are other minor convention/readability issues. Such as is in line 58, the tick method. You have a class textBoxBase. While the language allows this, it goes against a major convention in Java that states that class names should be UpperCamelCase (TextBoxBase) and instance/variables should be lowerCamelCase (textBoxBase). This is primarily so that when skimming the code, it’s easy to differentiate between different things. It’s especially bad when the text processor colors things based on their caps (Look at the code chunk below). If you’re skimming it, it looks like there are two variable names, one after another in that method declaration.
It wasn’t mean at all! It was very helpful ^^
It’s funny though, I went ahead and changed TextBoxBase with the refactor, but it’s funny because it was the first class I made in the game. That’s probably why it’s the only one that starts with a lower cased character. I didn’t know the importance prior.
Let’s see some other information that might end up being helpful for you:
Something about what you should do.
Java Coding Conventions: http://geosoft.no/development/javastyle.html
Somethings about what you shouldn’t do.
Code Smell (Things to watch out for): http://en.wikipedia.org/wiki/Code_smell
Anti-patterns: http://en.wikipedia.org/wiki/Anti-pattern
You can replace all the Animation objects with an Animation[] and use constants to index into it. For example:
private static final int DOWN = 0;
private static final int UP = 1;
private static final int LEFT = 2;
private static final int RIGHT = 3;
private static final int STILL = 0;
private static final int MOVING = 4;
Animation[] animations = new Animation[8]; //Remember to create each individual Animation in constructor
//Then when you want a specific animation, just write <direction> + <still/moving>:
Animation currentAnimation = animations[UP + MOVING); //index: 5
Even better, if you keep direction and movement state as ints too you can with a single line find which animation to use:
private int direction = LEFT;
private int movementState = STILL;
Animation currentAnimation = animations[direction + movementState];
Pro tips:
- Use bitwise OR instead of plus when calculating the animation index
- Use enums instead of int constants.
I strongly advice against such code. It’s so easy to make a mistake, and you end up with lots of ‘magic’ offsets. When you expand your code to 8 directions, you’d have to check all those indices and make the constants ‘STILL’ and ‘MOVING’ multiples of 8. When your project is growing, more and more patches are required to make it all work again.
If anything, it should be:
private static final int STILL = 0;
private static final int MOVING = 1; // was 4
Animation[][] animations = new Animation[2][4];
Animation currentAnimation = animations[movementState][direction];
but you should try to make it more readable… we’re not coding in C after all.
Use enums for starters. Enum.ordinal() can be used to provide an index into an array.
Would you guys mind explaining the purpose of an Enum to me? I’ve read on it, and it reminds me a bit of constants without values. I read this at the Java doc, and honestly I hate their explanations, so if you could please, enlighten me. Haha.
An Enum is a constant without value (Though, not always).
Basically, it allows you to specify a set of possible inputs that will always be bounded and cannot be changed programmatically in anyway. Once you define your Enum its structure, and the number of elements in it, will stay constant the whole time. This allows you to remove a lot of code that you might otherwise need to catch certain types of errors and helps make your code more readable. Further, if you require the constants to be used in several different places, it allows you to simply provide the Enum instead of including like a Constants interface to hold all of these values. While it can be just about the same as using public final int CONSTANT_NAME = 1, it makes sure that you don’t sometimes think “Oh, I know what this constant is supposed to be, so I’ll just use the value instead of the variable”.
However, they don’t do anything magical. If your constant isn’t meant to define a part of a logical set, then you probably won’t want to use an Enum for it. Further, if your logical set ever changes (As in the elements change, not the sate of the elements) you don’t want to use an Enum. There is also something of a trade-off in readability/understandability when the number of elements in your Enum gets too large.
Quick, silly example of some enums that form a logical set. Instead of having a bunch of integer values, that might end up being the same value (ACE = 1, HEART = 1), you allow the language to take care of it. Also, you never have to worry about another program attempting to interface with yours, that has Face.REVERSE_ORDER, Face.DRAW_TWO_CARDS, etc that might have had the same constant values are you Face.ACE and FACE.Two.
public enum Face {
ACE, TWO, THREE, FOUR, FIVE, SIX, SEVEN, EIGHT, NINE, JACK, KING, QUEEN
}
public enum Suit {
HEART, DIAMON, CLUB, SPADE
}
public class Card {
public Face face;
public Suit suit;
public Card(Face face, Suit suit) {
this.face = face;
this.suit = suit;
}
public static void main(String[] args) {
Card card = new Card(Face.ACE, Suit.SPADE);
}
}
An Enum is a class whose instances are all statically enumerated (named) in the declaration of the class. They’re most commonly used as symbol types, an object which has itself as its own value, but you can literally use them like any class, except that they can’t participate in class inheritance (interfaces are still okay). Additionally you can use them in switch/case statements, and you get some extra methods for looking them up by name, storing them in EnumSets, and so on.
That was awesome man. Lol. I completely understand now. I will take advantage of this from now on! Although, I’m a bit too far in to put this every part of the game now that I’m this far in. Haha
Psst! There’s an appreciate button next to reply!
Hahahahaha you’re turning into me! 