I Feel Like Sharing some Code - My Unfinished AOE Class

[taffer]

Code: [Select]

package eracia;

/**
 *
 * @author Jeff Desrosiers
 */

//imports
import java.awt.Dimension;

//This class calculates every square given within an Area of Effect
public class AreaOfEffect
{
   
    //------------------------InClass Declarations------------------------------
    public enum AOEType {CIRCLE, BEAM, CONE}    //The enumeration of AOE
   
    public AOEType              Type;           //The Type of Area Of Effect
   
    private int                 intSize;        //The Size of Area of Effect
    private Dimension           dimCenter;      //The Center of the AOE
   
    public AreaOfEffect(AOEType Type, int intSize, Dimension dimCenter)
    {
       
        this.Type = Type;
        this.intSize = intSize;
        this.dimCenter = dimCenter;
               
    }//end of Constructor
   
    //----------------------------Public Methods--------------------------------
    //This method finds out if the current tile in question is in the Area of
    //Effect's range
    //ix:       The X Axis value
    //iy:       The Y Axis value
    public boolean isInAOE(int ix, int iy)
    {
       
        //--------------------InMethod Declarations-----------------------------
        int intLine = 1;                //What line to calculate for X axis
        int ix2 = 0;                    //The length of the line for ix
       
        //Checks to see if it is in the Y axis of the AOE if true go on if false
        //return false
        if(iy >= this.dimCenter.height - this.intSize &&
           iy <= this.dimCenter.height + this.intSize)
        {
           
            //It was true so find out which line iy is in the AOE
            for(int iy2 = iy; iy2 != this.dimCenter.height -
                this.intSize; iy2--)
            {
               
                intLine++;              //increment intLine
               
            }//end of for(int iy2 = iy; iy2 != this.dimCenter.height -
             //this.intSize; iy2--)
           
            //Found the right line so now find the size of the line for X Axis
            ix2 = this.findXNum(intLine);
           
            //Check to see if it reaches the X Axis limit to the line in
            //question
            if(ix >= this.dimCenter.width - (ix2 / 2) &&
               ix <= this.dimCenter.width + (ix2 / 2))
            {
               
                return true;            //Both x and y are in the AOE
               
            }//end of if(ix >= this.dimCenter.width - (ix2 / 2) &&
             //          ix <= this.dimCenter.width + (ix2 / 2))
           
        }//end of if(iy >= this.dimCenter.height - this.intSize &&
         //          iy <= this.dimCenter.height + this.intSize)
       
        return false;  //The Tile is not in the AOE
       
    }//end of isInAOE(int ix, int iy)
   
    //Returns the number of Tiles from the Tile in Question to the Center of the
    //AOE
    //ix:           The X Axis value
    //iy:           The Y Axis value
    public int distanceFromCenter(int ix, int iy)
    {
       
        int intDistance = 0;        //The Distance between Center to Tile
        int iyd = Math.abs(this.dimCenter.height - iy); //The change in Y
        int ixd = Math.abs(this.dimCenter.width - ix);  //The change in X
       
        //Check to see if iyd = 0 if so The Distance is ixd
        if(iyd == 0)
        {
           
            intDistance = ixd;
           
        }//end of if(iyd == 0)
        //Check of ixd = 0 if so the distance is iyd
        else if(ixd == 0)
        {
           
            intDistance = iyd;
           
        }//end of else if(ixd == 0)
        //Else it is the larger of the two
        else
        {
           
            if(iyd > ixd)
            {
               
                intDistance = iyd;
               
            }//end of if(iyd > ixd)
            else
            {
               
                intDistance = ixd;
               
            }//end of else
           
        }//end of else
       
        //If both are 0 set intDistance to 1 so there will be no division by 0
        if(intDistance == 0)
        {
           
            intDistance = 1;
           
        }//end of if(intDistance == 0)
       
        return intDistance; //Done
       
    }//end of distanceFromCenter(int ix, int iy)
   
    //This method updates the Center of AOE
    public void updatePosition(Dimension dimPos)
    {
       
        this.dimCenter = dimPos;
       
    }//end of updatePosition(Dimension dimPos)
   
    //-------------------------Private Methods----------------------------------
    //This method finds the X value in a given line of the AOE
    //iy:       is the Y Axis of the AOE not the map
    private int findXNum(int iy)
    {
       
        //------------------InMethod Declarations-------------------------------
        int ix =                0;      //The x value
       
        //These variables calculate the middle of the AOE
        int imStart =           this.intSize - (this.intSize / 2) + 1;
        int imEnd   =           imStart + this.intSize - 1;
       
        //If iy is in the middle of the AOE
        if(iy >= imStart && iy <= imEnd)
        {
           
            ix = (this.intSize * 2) + 1;  //ix = 2 * The Size of AOE + 1
           
        }//end of if(iy >= imStart && iy <= imEnd)
        //If iy is before the middle of the AOE
        else if(iy < imStart)
        {
           
            //Set ix to the The size of the middle and for each line ii travels
            //minus 2 to ix
            ix = (this.intSize * 2) + 1;
            for(int ii = iy; ii < imStart; ii++)
            {
               
                ix -= 2;
               
            }//end of for(int ii = iy; ii < imStart; ii++)
           
        }//end of else if(iy < imStart)
        //If iy is after the middleof the AOE
        else
        {
           
            //Set ix to the size of the AOE and add two for every line ii
            //travels through
            ix = this.intSize;
            for(int ii = (this.intSize * 2) + 1; ii > iy; ii--)
            {
               
                ix += 2;
               
            }//end of for(int ii = (this.intSize * 2) + 1; ii > iy; ii--)
           
        }//end of else
       
        return ix;
       
    }//end of findXNum(int iy)
   
}//end of AreaOfEffect

Post any constructive criticism that you can.  I love to learn new ideas. 

I also want to know how my documentation / coding style is.  Any tips will be welcomed.  Thankyou in advance

Taffer
eraciarl.blogspot.com

[purestrain]

Ok, personal opinion:

Too much documentation and unnecessary documentation., very bloated. Methods are bloated too, usually distance calculation is made within <3 loc. Is this empty-line thing because of pasting it to a forum or do you have so much empty lines in your original code?

I don't like it.

[Kyzrati]

Comments are for explaining things that may not be obvious--putting an "end of" at the end of every single block is overkill to the tenth degree. That's only recommendable when you've got a seriously massive block or some crazy nesting (which is best avoided in the first place).

[taffer]

Thank you.  The comments is the style my teacher in college taught me.  I honestly hate doing them like that I can always delete that and be much happier.  Less typing on my part. 

I like the empty lines to me it looks better for readability.

And my methods being bloated does this mean Im doing unnecessary calculations?

I'm doing this to become a better programmer.  I'm always open for more criticism.

Taffer

[taffer]

Here is updated code much smaller half the size:

Code: [Select]

package eracia;

/**
 *
 * @author Jeff Desrosiers
 */
import java.awt.Dimension;

//This class calculates every square given within an Area of Effect
public class AreaOfEffect
{
    
    //------------------------InClass Declarations------------------------------
    public enum AOEType {CIRCLE, BEAM, CONE}    //The enumeration of AOE    
    public  AOEType             Type;           //The Type of Area Of Effect    
    private int                 intSize;        //The Size of Area of Effect
    private Dimension           dimCenter;      //The Center of the AOE
    
    public AreaOfEffect(AOEType Type, int intSize, Dimension dimCenter)
    {
        
        this.Type = Type;
        this.intSize = intSize;
        this.dimCenter = dimCenter;
                
    }    
    //----------------------------Public Methods--------------------------------
    public boolean isInAOE(int ix, int iy)
    {
                
        int intLine = 1;                //What line to calculate for X axis
        int ix2 = 0;                    //The length of the line for ix
                
        if(iy >= this.dimCenter.height - this.intSize &&
           iy <= this.dimCenter.height + this.intSize)
        {
            
            for(int iy2 = iy; iy2 != this.dimCenter.height -
                this.intSize; iy2--)
                    intLine++;
            
            ix2 = this.findXNum(intLine);                        
            if(ix >= this.dimCenter.width - (ix2 / 2) &&
               ix <= this.dimCenter.width + (ix2 / 2))
                    return true;
                
        }
        
        return false;  
        
    }    
    
    public int distanceFromCenter(int ix, int iy)
    {
        
        int intDistance = 0;        
        int iyd = Math.abs(this.dimCenter.height - iy);
        int ixd = Math.abs(this.dimCenter.width - ix);
        
        if(iyd == 0)
            intDistance = ixd;
        else if(ixd == 0)
            intDistance = iyd;
        else
        {
            
            if(iyd > ixd)
                intDistance = iyd;
            else
                intDistance = ixd;
            
        }        
        
        if(intDistance == 0)
            intDistance = 1;
                  
        return intDistance;
        
    }    
    public void updatePosition(Dimension dimPos)
    {
        
        this.dimCenter = dimPos;
        
    }    
    private int findXNum(int iy)
    {
                
        int ix =                0;      //The x value
        
        int imStart =           this.intSize - (this.intSize / 2) + 1;
        int imEnd   =           imStart + this.intSize - 1;
                
        if(iy >= imStart && iy <= imEnd)
            ix = (this.intSize * 2) + 1;
        else if(iy < imStart)
        {
                        
            ix = (this.intSize * 2) + 1;
            for(int ii = iy; ii < imStart; ii++)
                ix -= 2;
            
        }
        else
        {            
            
            ix = this.intSize;
            for(int ii = (this.intSize * 2) + 1; ii > iy; ii--)
                ix += 2;

        }
        
        return ix;
        
    }
    
}



I hope its a little better without all those comments

Taffer

[purestrain]

Much better, i would skip those empty lines. Your distance calculation e.g. is unecessary long:

Code: [Select]

public double distanceTo(Point otherPoint) {
return Math.sqrt(Math.pow(x - otherPoint.x, 2) + Math.pow(y - otherPoint.y, 2));
}

public double manhattenDistanceTo(Point otherPoint) {
return Math.max(Math.abs(x - otherPoint.x), Math.abs(y - otherPoint.y));
}

Maybe introduce a temporary variable or two in the above two lines.

Skip the enumeration and make discrete classes for cone/circle/square area of effects which share a common class/interface.

[Z]

Thank you.  The comments is the style my teacher in college taught me.  I honestly hate doing them like that I can always delete that and be much happier.  Less typing on my part. 

Maybe the teacher just wanted to teach you good practices on small examples, and you should do that on large loops, but ignore on small ones.

(a >= b-c && a <= b+c) can be written as Math.abs(a-b) <= c, and maybe it would be better to replace a loop of += 2 by a closed formula.

[taffer]

Wow thanks purestrain and Z, I will try to incorporate this

Yes I like using interfaces but I thought they were used only for global declares and global methods.  I'm very happy you guys are willing to help teach me. 

Purestrain your two methods are a little above my Math comprehension ATM but I will change those methods and play around.  I am very intrigued.

Z your example makes a lot of sense thankyou.

I love coding, but sadly I'm still very new at Math coding. You two are great help.

Taffer

 

[Krice]

I think it looks nice, although I don't agree with the style (but that's a matter of opinion anyway). But I've seen lot worse source code in most of the roguelike games.

[corremn]

Hmm, I would keep the method comments. That is important, especially for public ones.
Here is how I would comment the function (in c++ that is)

Code: [Select]

 
  /*
   * Calculates the number of Tiles from a reference point to the origin of the Area of Effect
   * param otherPoint The reference point.
   * throws InvalidPointException when otherPoint does not meet requirements
   * return: The distance from the reference point to the origin of the Area of Effect
   */
public int distanceFromCenter(Point otherPoint)

   
And as mentioned use a Point class for x,y pairs.
You should also be throwing exceptions on invalid parameters.
Might also mention whether it should round up or down, or choose the closest integer on the return.
Also I would tend not to use uncommon acronyms in my code, i.e AOE, if you want others to read it.

In C++ I would probably use a helper function to determine if a point is in an Area of Effect.  But that would be hard for Java.
E.g bool isInAreaOfEffect(AreaOfEffect area, Point reference);
Because I dont believe it is the AreaOfEffect class's job to determine that. (But that is just me)

As mentioned use separate classes for each AOE type, as there will be much difference between calculations.

My thoughts on variable naming. 
Member variables should start with a 'm' and use Camel case for names, e.g , mIntSize
(You could append argument variables with 'a' and scoped variables with 'v', but that might be going too far.
Also I would drop the variable type labeling. Nearly everyone working with code uses a modern IDE that will tell you what the variable type is.  Find a modern style guide that was not written by a teacher

Also some of you variables are poorly named. E.g anything starting with an 'i' that is not an index variable in a loop is bad.   (I get it is 'i' for int)

Code: [Select]

int ix2 = 0;                    //The length of the line for ix
As soon as I saw this code I though, ok ix2 is a loop variable but what is ix1?  This should be named lineLengthOfX or lineLength_x or whatever.

[TSMI]

It's good to see commented code, and many of your comments help readability. However, there are also many comments that don't add anything, such as;

public enum AOEType {CIRCLE, BEAM, CONE}    //The enumeration of AOE

The code on the left side pretty clearly tells us the comment on the right so the comment is a bit redundant.

You only really need to comment on what your code does above blocks, just to give the reader a bit of context. However commenting on why your code does things can be done anyhere, and often should (ie, why did you use a linked list over a dynamic array? why a nested for loop? etc etc).

[TSMI]

A few more (hopefully constructive!) criticisms:

//This class calculates every square given within an Area of Effect

calculates what about every square given? a bit vague

public int distanceFromCenter(int ix, int iy)

This is good - you wrote out distanceFromCenter instead of doing some horrible shortening like "distFrmC" which is so common in code.

[corremn]

Is this class designed to be used for constructing  a list of tiles in the area of effect or just for testing whether a tile is in an Area of effect.

Usually you would have a list of tiles that are in the Area of Effect. So you apply the effect to those tiles rather than testing if it is in the area of effect and then applying the effect.

*shrug* Much of a Muchness really.

[taffer]

Again Thanks guys I really appreciate everyone's opinions.  All advice is always welcomed and accepted.

Funny this class started making a list of Tiles and I just checked through the list.  But when I started making the AreaOfEffect class movable I need to redo the list per move. I thought maybe this would be too slow so I redid that idea and came up with what I have now.

In all honesty after much thought I realized I needed to rework my class structure.   I'm going to keep 90% of the code but just rewrite The actual structure of the game. 

In C++ I would probably use a helper function to determine if a point is in an Area of Effect.  But that would be hard for Java.
E.g bool isInAreaOfEffect(AreaOfEffect area, Point reference);
Because I dont believe it is the AreaOfEffect class's job to determine that.

I agree with this.  In my rewriting I created a little class that contains public methods like your bool isInAreaOfEffect(AreaOfEffect area, Point reference).  I do come from C/C++ and I really dislike Java's everything must be a class idea.  Most of my distance from Point to Point, Monster to Player, line of sight methods will be in this little class.  The class has no data just C like Global Methods that's all.

Point Class added to game Thankyou.

Redundant and vague comments noted and will be much better.

Method comments are coming back but all those end of comments will never come back.

Thankyou corremn, TSMI,and Krice for the new advice.


Taffer

[purestrain]

In C++ I would probably use a helper function to determine if a point is in an Area of Effect.  But that would be hard for Java.
E.g bool isInAreaOfEffect(AreaOfEffect area, Point reference);
Because I dont believe it is the AreaOfEffect class's job to determine that.

I agree with this.  In my rewriting I created a little class that contains public methods like your bool isInAreaOfEffect(AreaOfEffect area, Point reference).  I do come from C/C++ and I really dislike Java's everything must be a class idea.  Most of my distance from Point to Point, Monster to Player, line of sight methods will be in this little class. 

I strongly disagree.

Well; _distance_ calculation should be the responsibility of a point class, right. But with some static methods like "isInAreaOfEffect(AreaOfEffect area, Point reference)" you are going the wrong way.

Instances of ConeAreaOfEffect, Square..., Circle... should calculate that - discard the enumerations, polymorphism is the way to go. By using enumerations you end with ugly switch case statements.

If you add another area of effect, do you really want to change multiple code locations?