Jump to content

  • Log In with Google      Sign In   
  • Create Account

Interested in a FREE copy of HTML5 game maker Construct 2?

We'll be giving away three Personal Edition licences in next Tuesday's GDNet Direct email newsletter!

Sign up from the right-hand sidebar on our homepage and read Tuesday's newsletter for details!


We're also offering banner ads on our site from just $5! 1. Details HERE. 2. GDNet+ Subscriptions HERE. 3. Ad upload HERE.


#ActualIndifferent

Posted 16 April 2013 - 06:08 PM

This isn't an extensive list but I've tried to cover some of the major points.

Class length:
Break the code into classes; a single 600+ line class makes the code hard to follow. There's no hard rule on how long a class should be but each class should have one responsibility and should do it well. See this entry on the single responsibility principle for further information.

Method length:
While again there's no hard rule on how long a method should be, do try to refactor them by splitting them out into additional methods if they start to become too unwieldly. One clearly problematic method in your code is playGame().

Nesting depth:
You should do your best to avoid nesting code. Here's an example of what I mean by nesting:

for(int i = 0; i < 10; i++) {
    while(something) {
        if(something_else) {
            //do something
        } else {
            //do something else
        }
    }
}

This code is nested three levels deep and as soon as any real logic is added, not only will become difficult to understand but at some point in the future when you begin testing your code, you'll find it harder to test too.

At a glance, your playGame() is six levels deep at worst and has loops spanning dozens of lines. Try to refactor it so it's no deeper than one or two levels.

Simplify logic:
There are a few cases where your logic could be simplified. One instance is this:

        public static int setCurrentPlayer(int currentPlayer, int players)
        {
           if((currentPlayer+1)<=players)
            {
                currentPlayer+=1;
            }
            else if((currentPlayer+1)>players)
            {
                currentPlayer=1;
            }
            return currentPlayer;
       }

Replacing the else if with an else would achieve the same effect but would make the code easier to follow and hopefully save you from introducing buggy logic through adding conditions where they weren't needed. An if/else if doesn't inherently cover all possibilities but an if/else does.


Another example would be this piece of code in which the loop does nothing and could be safely removed.

for(int i=0;i<oldDice.length;i++)
{
    newDice=new int[oldDice.length];
}


Perfection is achieved, not when there is nothing more to add, but when there is nothing left to take away. ;)

Comments:
Good code should be self-documentating. While it's quite a subjective point, most of your comments provide no information that can't be gleaned by reading the method/argument names. If you feel a comment will be of value, use it to explain why you've chosen to implement functionality in a certain way, not how - that's what the code is there for.

Exceptions:
Handle the exceptions properly with try/catch at the point which they could be thrown, even if that means displaying an error and quitting. At the moment you're essentially just passing the buck up the call stack to methods that have nothing to do with the exceptions and shouldn't particularly need to concern themselves with them.

Formatting:
Your code formatting could be better. Think about the different sections of logic in your methods as paragraphs and add a blank line between each one.


Hope that's of some help to get you started. smile.png

 

Edit: Lost connection during posting, came out slightly broken.


#2Indifferent

Posted 16 April 2013 - 06:01 PM

This isn't an extensive list but I've tried to cover some of the major points.

Class length:
Break the code into classes; a single 600+ line class makes the code hard to follow. There's no hard rule on how long a class should be but each class should have one responsibility and should do it well. See this entry on the single responsibility principle for further information.

Method length:
While again there's no hard rule on how long a method should be, do try to refactor them by splitting them out into additional methods if they start to become too unwieldly. One clearly problematic method in your code is playGame().

Nesting depth:
You should do your best to avoid nesting code. Here's an example of what I mean by nesting:

for(int i = 0; i < 10; i++) {
    while(something) {
        if(something_else) {
            //do something
        } else {
            //do something else
        }
    }
}


This code is nested three levels deep and as soon as any real logic is added, not only will become difficult to understand but at some point in the future when you begin testing your code, you'll find it harder to test too.

At a glance, your playGame() is six levels deep at worst and has loops spanning dozens of lines. Try to refactor it so it's no deeper than one or two levels.

Simplify logic:
There are a few cases where your logic could be simplified. One instance is this:

        public static int setCurrentPlayer(int currentPlayer, int players)
        {
           if((currentPlayer+1)<=players)
            {
                currentPlayer+=1;
            }
            else if((currentPlayer+1)>players)
            {
                currentPlayer=1;
            }
            return currentPlayer;
       }


Replacing the else if with an else would achieve the same effect but would make the code easier to follow.


Another example would be this piece of code in which the loop does nothing and could be safely removed.

for(int i=0;i<oldDice.length;i++)
{
    newDice=new int[oldDice.length];
}


Perfection is achieved, not when there is nothing more to add, but when there is nothing left to take away. ;)

Comments:
Good code should be self-documentating. While it's quite a subjective point, most of your comments provide no information that can't be gleaned by reading the method/argument names. If you feel a comment will be of value, use it to explain why you've chosen to implement functionality in a certain way, not how - that's what the code is there for.

Exceptions:
Handle the exceptions properly with try/catch at the point which they could be thrown, even if that means displaying an error and quitting. At the moment you're essentially just passing the buck up the call stack to methods that have nothing to do with the exceptions and shouldn't particularly need to concern themselves with them.

Formatting:
Your code formatting could be better. Think about the different sections of logic in your methods as paragraphs and add a blank line between each one.

Hope that's of some help to get you started. :)

 

Edit: Lost connection during posting, came out slightly broken.


#1Indifferent

Posted 16 April 2013 - 05:55 PM

This isn't an extensive list but I've tried to cover some of the major points.

 

Class length:

Break the code into classes; a single 600+ line class makes the code hard to follow. There's no hard rule on how long a class should be but each class should have one responsibility and should do it well. See this entry on the single responsibility principle for further information.

 

Method length:

While again there's no hard rule on how long a method should be, do try to refactor them by splitting them out into additional methods if they start to become too unwieldly. One clearly problematic method in your code is playGame().

 

Nesting depth:

You should do your best to avoid nesting code. Here's an example of what I mean by nesting:

for(int i = 0; i < 10; i++) {
    while(something) {
        if(something_else) {
            //do something
        } else {
            //do something else
        }
    }
}

 

This code is nested three levels deep and as soon as any real logic is added, not only will become difficult to understand but at some point in the future when you begin testing your code, you'll find it harder to test too.

 

At a glance, your playGame() is six levels deep at worst and has loops spanning dozens of lines. Try to refactor it so it's no deeper than one or two levels.

 

Simplify logic:

There are a few cases where your logic could be simplified. One instance is this:

        public static int setCurrentPlayer(int currentPlayer, int players)
        {
           if((currentPlayer+1)<=players)
            {
                currentPlayer+=1;
            }
            else if((currentPlayer+1)>players)
            {
                currentPlayer=1;
            }
            return currentPlayer;
       }

PARTNERS