Temple of The Roguelike Forums
Development => Programming => Topic started by: taffer on July 29, 2011, 07:25:47 PM
-
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 (http://eraciarl.blogspot.com)
-
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.
-
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).
-
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
-
Here is updated code much smaller half the size:
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
-
Much better, i would skip those empty lines. Your distance calculation e.g. is unecessary long:
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.
-
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.
-
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
-
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.
-
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)
/*
* 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)
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.
-
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).
-
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.
-
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.
-
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
-
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?
-
Yeah on second thought, because isInAreaOfEffect will have to be different for each type of AOE you might have to think harder about where to put it.
-
When working with objects I like to keep in mind the idea of "messaging" - forget about method calls and all that, view objects as independent entities that send messages to each other.
So I think it's perfectly reasonable to send a message to AreaOfEffect asking if a point is located inside of it.
-
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?
Never thought of that I agree here Polymorphism would be the best way. With my current code it was only working with circles I started to forget about Square, Cone and Beam. I'm going to take the advice about killing the enumerations. And Poly-morphing the different kind of AreaOfEffects.
This has been a good thread for me I am learning quite a bit from all of you. This is my first attempt at a real game. I've made Pong, Tic Tac Toe, Tetris. I've never had a program that was so complex as a roguelike. Truthfully I am walking into this blindly, I thought it was an easy game to program, but this has been a very fun experience so far.
Taffer
-
Truthfully I am walking into this blindly, I thought it was an easy game to program, but this has been a very fun experience so far.
"I suppose we all thought that, one way or another."