Looking for Code Criticism

[anon951759]

Hey, I’ve been messing around attempting to start making a game for the past few days and I felt as if I should get someone more experienced to do a quick check-over just to tell me what I’m doing wrong, what I could/should change, etc… Just so that I don’t go getting into any bad habits or end up doing something that may come back to bite me later on.

Code:
http://pastebin.com/AUvX9Gw8

I’m still actively writing this code so it’s already a few lines outdated by the time anyone reads this but any criticism will help me either now or later on. ^.^

Thank’s in advance!

[HeroesGraveDev]

Removing all the comments will make it easier for most programmers to read and find optimizations.

Also, if you are using Java 7, you could use NIO for loading/saving.
I can give some help on NIO if you want.

Other than that, I can’t really see anything that’s wrong with all the comments distracting me, but I think there are lots of small things to do the make things easier to code for you that won’t affect the final result.

Using Arrays of components instead of loads of different components with a differences that could be stored in variables may help.
Instead of having “STR”, “DEX” etc, you could have [icode]stats[][/icode] and have constants called [icode]STR = 0[/icode] and [icode]DEX = 1[/icode] etc and put the strength variable in [icode]stats[STR][/icode] like that. Then you could use arrays to iterate over everything and save lots of time.

But then again, depending on the size (size so far and size left to go) of your project, refactoring might not be worth it.
Everything is up to you.

[anon951759]

I’m a bit of a nut when it comes to comments. I like to know exactly what everything is doing in-case I go back and can’t figure out what I’ve done; I’ll remember to remove them next time I post my code. =)
How would I go about checking if I’m using Java 7 or not, my CS class never covered different versions. I’d definitely like help with the save/loading as I only barley understood how to do it the way I currently have it from the various examples I’ve seen on google.

[HeroesGraveDev]

@anon951759

I’m a bit of a nut when it comes to comments. I like to know exactly what everything is doing in-case I go back and can’t figure out what I’ve done; I’ll remember to remove them next time I post my code. =)
How would I go about checking if I’m using Java 7 or not, my CS class never covered different versions. I’d definitely like help with the save/loading as I only barley understood how to do it the way I currently have it from the various examples I’ve seen on google.

Run [icode]java -version[/icode] in command line:

If it says something like 1.7.0_(some other number) then you have Java 7.
If it says somthing like 1.6.0_(some other number) then you have Java 6.

Oh, and when using BufferedWriter or any other kind of Buffered output stream (including BufferedOutputStream ;)) you should be calling [icode]out.flush()[/icode] to make sure the data is written to the file.

[anon951759]

Looks like I need to update Java, apparently I’m using 1.4.2_19 which I’m guessing must be quite old. (It’s the version that my course required)

[Sammidysam]

@anon951759

Looks like I need to update Java, apparently I’m using 1.4.2_19 which I’m guessing must be quite old. (It’s the version that my course required)

Man I haven’t seen a Java version that old in quite some time.

[HeroesGraveDev]

I believe that’s Java 5

Also, found something else:

When loading your file, you should be iterating over the lines instead of assuming there is at least 2 lines.

String line;
String output = "";
while((line = in.readLine()) != null)
{
output = output + line;
}

System.out.println(output);

[anon951759]

@HeroesGraveDev

I believe that’s Java 5

Also, found something else:

When loading your file, you should be iterating over the lines instead of assuming there is at least 2 lines.

String line;
String output = "";
while((line = in.readLine()) != null)
{
output = output + line;
}

System.out.println(output);

I’ve just updated my JDK to Java 7. I’ve also attempted to re-write my LoadingClass with the information you’ve provided, should it look something like:

class LoadingClass
{
	public static void loadGame() throws Exception //Change the return type (void) to something else later.
	{
		String line = "";
		String output = "";
		
		BufferedReader saveFile = new BufferedReader(new FileReader("TextSave.txt"));
		
		while((line = saveFile.readLine()) != null) //Note to Self: Re-read documentation on while loops.
		{
			saveFile.flush();
			output = output + line;
		}
		
		System.out.println(""+output+"");
		saveFile.close();
	}
}

I won’t be doing too much with the Save/Loading just yet but it’s always good to figure things out in advance ^.^

[sproingie]

Okay, there is commenting and there is over-commenting. Comments should explain something that isn’t obvious to someone already somewhat familiar with the language and API.

[icode]Foo x = new Foo(blah); // creates a new Foo that natter and gromish natter and gromish …[/icode]

Anyone who knows what a constructor does knows that statement creates a new Foo

[HeroesGraveDev]

@anon951759

I’ve just updated my JDK to Java 7. I’ve also attempted to re-write my LoadingClass with the information you’ve provided, should it look something like:

class LoadingClass
{
	public static void loadGame() throws Exception //Change the return type (void) to something else later.
	{
		String line;
		String output;
		
		BufferedReader saveFile = new BufferedReader(new FileReader("TextSave.txt"));
		
		while((line = saveFile.readLine()) != null) //Note to Self: Re-read documentation on while loops.
		{
			saveFile.flush();
			output = output + line;
		}
		
		System.out.println(""+output+"");
		saveFile.close();
	}
}

I won’t be doing too much with the Save/Loading just yet but it’s always good to figure things out in advance ^.^

You don’t flush input streams. It doesn’t make any sense.

Think of flushing like when you flush a toilet. :o
If there was no flush, water would be constantly flowing down, which is a waste.
If there is a flush, but it isn’t used, things get blocked up until the toilet overflows. :persecutioncomplex:
If the flush is used, it works normally.

Sorry for the ‘dirty’ metaphor

[Nate]

Wow I’ve never seen that much over commenting. Comments should only be used when the code and variable names are not clear. It might seem like a good idea, but all it does is clutter up the code and actually reduces readability (a lot!).

[anon951759]

@HeroesGraveDev

You don’t flush input streams. It doesn’t make any sense.

Think of flushing like when you flush a toilet. :o
If there was no flush, water would be constantly flowing down, which is a waste.
If there is a flush, but it isn’t used, things get blocked up until the toilet overflows. :persecutioncomplex:
If the flush is used, it works normally.

Sorry for the ‘dirty’ metaphor

The metaphor helped a lot. I’ll read up on ‘.flush’ a lot more when I get to the point where I need to write a proper save/loading system. 8)

As for the over-commenting, I half-expected to see something about it, I’ll try to do a lot less mass-commenting from now on.

[ctomni231]

Well, if it is for your own reference and you believe it’ll help you learn a lot more about Java, comment away. As long as you can find where things are within the code for yourself. Just when you are posting it on a forum like this, you just might want to either delete all the comments, or put all comments on a line above (or below) a code line so we can use an IDE to make them disappear.

[ReBirth]

Usually you don’t need someone else to do this, yourself from future will do it

[loom_weaver]

My three cents:

Storing variables in bytes and shorts is unnecessary and a premature optimization.

Naming your classes with verbs e.g. Create* is a bit strange.

Technically, instantiating your main frame should be run on the EDT as well.

[masteryoom]

I have never seen so much commenting in my life over things that everybody should know. :

[masteryoom]

I ran it in eclipse and it didn’t do anything. :emo:

[anon951759]

@loom_weaver

My three cents:

Storing variables in bytes and shorts is unnecessary and a premature optimization.

Naming your classes with verbs e.g. Create* is a bit strange.

Technically, instantiating your main frame should be run on the EDT as well.

@masteryoom

I ran it in eclipse and it didn’t do anything. :emo:

Just a few questions based on the above replies:

From what I learned in my CS class, don’t smaller data types such as bytes and shorts take up a bit less memory than integer? If so, is it alright if I keep using them or would it be a bit better if I just use Ints for now?
As for the class names, how come having the word Create in the names is strange?
I’m not sure what EDT stands for, could you explain it to me. :-\

The program compiles and runs fine from the cmd line, maybe eclipse works differently or something? (I only use Notepad++ so I don’t know too much about eclipse.)

Thank’s for all the feedback so-far!

[theagentd]

As just a field in a class ([icode]private int x;[/icode] or something), each variable takes at least 32 bytes, so there’s no need to use anything smaller than an int for them. The only time they consume less space in when used in arrays, in which case a byte[1024] will be a 1/4th the size of an int[1024] (8 vs 32 bits) if we ignore the overhead of the array object.

I strongly recommend using Eclipse. It’s a lot faster than manually compiling from the command line.

Your love / Mokyu

[anon951759]

@theagentd

As just a field in a class ([icode]private int x;[/icode] or something), each variable takes at least 32 bytes, so there’s no need to use anything smaller than an int for them. The only time they consume less space in when used in arrays, in which case a byte[1024] will be a 1/4th the size of an int[1024] (8 vs 32 bits) if we ignore the overhead of the array object.

I strongly recommend using Eclipse. It’s a lot faster than manually compiling from the command line.

Your love / Mokyu

Ah, well that should make working with all of my numbers a lot easier. Thanks!

~Off to try Eclipse