EDIT: Fixed line breaks. Now it’s not all a big mess, yay!
Hi!
I thought I’d point out a few problems with tonegodgui’s code, and suggest improvements where possible.
I hope I don’t come across as overly negative, that is not my intention. I just want to improve tonegodgui. 
@t0neg0d you said you saw Simple Made Easy (http://www.infoq.com/presentations/Simple-Made-Easy), right? I’m gonna try to explain the problems in terms of complexity, as defined therein. I find that to be enlightening.
By the way, I have looked most closely at Screen and Element, so I’m mostly going to mention problems in those.
And also, I know I have mentioned some of these elsewhere, but I thought I’d just collect all my concerns in one place.
The first and by far the biggest problem: Lots and lots of code duplication. In fact, it will be much easier to spot additional problems and make improvements if the code duplication is taken care of first.
Examples:
In Element’s resize the code:
if (getLockToParentBounds()) { if (y <= 0) { y = 0; } }
if (minDimensions != null) {
if (getY()+getHeight()-y <= minDimensions.y) { y = getY()+getHeight()-minDimensions.y; }
}
if (resizeN) {
setHeight(getY()+getHeight()-y);
setY(y);
}
appears 3 times in short succession, with no changes.
In Screen’s getEventElement, getContactElement and getTargetElement share this code, with very few changes:
guiRayOrigin.set(x, y, 0f);
elementZOrderRay.setOrigin(guiRayOrigin);
results.clear();
t0neg0dGUI.collideWith(elementZOrderRay, results);
float z = 0;
Element testEl = null, el = null;
for (CollisionResult result : results) {
boolean discard = false;
if (result.getGeometry().getParent() instanceof Element) {
testEl = ((Element)(result.getGeometry().getParent()));
if (testEl.getIgnoreMouse()) {
discard = true;
} else if (testEl.getIsClipped()) {
if (result.getContactPoint().getX() < testEl.getClippingBounds().getX() ||
result.getContactPoint().getX() > testEl.getClippingBounds().getZ() ||
result.getContactPoint().getY() < testEl.getClippingBounds().getY() ||
result.getContactPoint().getY() > testEl.getClippingBounds().getW()) {
discard = true;
}
}
}
if (!discard) {
if (result.getGeometry().getParent() instanceof Element) {
el = testEl;
}
}
}
Those are just two examples, but code duplication is so rampant in the codebase that I think the number of lines of code in tonegodgui could easily be halved if the duplication is taken care of properly.
The downsides to code duplications are of course:
It makes the code harder to read, for two reasons: 1: More code. 2: One needs to look extra carefully at the duplicated code to see if there are any differences.
But the biggest problem is that when maintaining the code, it’s much more error prone, because if one modifies one of the places, one probably needs to make the same change to all of the other places with the same code in them. One could say that the places that contain the same code are complected with each other, because they should probably work the same way, but one needs to make sure they all in fact do every time a change is made.
The are no benefits to code duplication. Please fix, then make Ctrl-C a no-op, or something worse, to make yourself stop copying.
Another problem is lack of factoring, i.e. breaking up large methods into several small, reusable, simple methods.
Example: onMouseLeftReleased in DragElement, reproduced here for easy access:
public void onMouseLeftReleased(MouseButtonEvent evt) {
Element dropEl = screen.getDropElement();
int index = -1;
boolean success = onDragEnd(evt, dropEl);
if (success) {
if (parentDroppable != null && parentDroppable != dropEl) {
parentDroppable.removeChild(this);
parentDroppable = null;
}
parentDroppable = dropEl;
Vector2f pos = new Vector2f(getAbsoluteX(),getAbsoluteY());
Element parent = getElementParent();
if (parent != dropEl) {
if (parent != null) {
parent.removeChild(this);
} else {
screen.removeElement(this);
}
float nextY = (pos.y-dropEl.getAbsoluteY());
nextY = -nextY;
setPosition(pos.x-dropEl.getAbsoluteX(), nextY);
dropEl.addChild(this);
this.setZOrder(screen.getZOrderStepMinor());
}
if (lockToDropElementCenter) {
Vector2f destination = new Vector2f(
(dropEl.getWidth()/2)-(getWidth()/2),
(dropEl.getHeight()/2)-(getHeight()/2)
);
if (useLockToDropElementEffect) {
slideTo = new Effect(Effect.EffectType.SlideTo, Effect.EffectEvent.Release, .15f);
slideTo.setElement(this);
slideTo.setEffectDestination(destination);
screen.getEffectManager().applyEffect(slideTo);
} else {
setPosition(destination);
}
originalPosition = destination.clone();
}
} else {
if (useSpringBack) {
Vector2f destination = originalPosition.clone();
if (useSpringBackEffect) {
slideTo = new Effect(Effect.EffectType.SlideTo, Effect.EffectEvent.Release, .15f);
slideTo.setElement(this);
slideTo.setEffectDestination(destination);
screen.getEffectManager().applyEffect(slideTo);
} else {
setPosition(destination);
}
}
}
}
Wouldn’t the following be much easier to understand? (Also, it makes it easier to see that index is in fact unused.)
public void onMouseLeftReleased(MouseButtonEvent evt) {
Element dropEl = screen.getDropElement();
int index = -1;
boolean success = onDragEnd(evt, dropEl);
if (success) {
putIntoDropElement(dropEl);
}
} else {
if (useSpringBack) {
springBack();
}
}
}
And not only that, the two new methods putIntoDropElement and springBack might come in handy for other stuff. The original version is complex because it does too much, and one can’t use the putIntoDropElement and springBack parts separately; the functionality is all twisted together.
Which brings us to another problem: Too much side effects. When I brought up the fact that I needed a method like putIntoDropElement before, you suggested that I use bindToDroppable. That method is a good example of a method with too much side effects. Here it is for reference:
public void bindToDroppable(Element el) {
if (el.getIsDragDropDropElement()) {
if (getElementParent() != null)
getElementParent().removeChild(this);
if (screen.getElementById(getUID()) == null)
screen.addElement(this);
float x = el.getAbsoluteX()+(el.getWidth()/2);
float y = el.getAbsoluteY()+(el.getHeight()/2);
MouseButtonEvent evt = new MouseButtonEvent(0, false, (int)x, (int)y);
if (screen instanceof Screen) {
((Screen)screen).forceEventElement(this);
((Screen)screen).onMouseButtonEvent(evt);
}
}
}
First of all, it simulates a mouse click, which has side effects. And the method that handles mouse clicks in Screen (onMouseButtonEvent) is 100+ lines of nested if statements. Second, it calls forceEventElement which is another side effect, and that method is also long and complicated; 50+ lines of nested ifs. And it finally brings us back to onMouseLeftReleased, which does more than putting the element into a droppable. So, in trying to understand what bindToDroppable does (or make sure that it is correct) not only does one have to read the whole of onMouseLeftReleased instead of just the part that I suggested you factor out, but one also has to understand what happens in bindToDroppable, Screen’s forceEventElement and in onMouseButtonEvent. Additionally, it triggers onDropEnd, another side effect, that I personally did not expect or want. Also, onMouseLeftReleased calls Screen’s getDropElement without arguments, which has something to do with a side effect (I haven’t figured out exactly what).
All these side effects make it extremely hard to read and understand the code. Side effects also complicates reuse, because one never knows what calling the methods will do to other parts of the system. Also, another aspect of the problem is that since bindToDroppable depends on so much other code, it is very fragile; changes to that other code may break bindToDroppable.
Of course, anything that modifies mutable state, which is the default in Java, is a side effect. But I don’t expect you to shun all state. However, try to avoid state whenever possible. For example, I totally understand an Element needs a position and a geom. But does it really need both isClipped and wasClipped?
On to a few smaller and more specific problems.
In Element’s addChild there is the following code:
if (screen.getElementById(child.getUID()) != null) {
try {
throw new ConflictingIDException();
} catch (ConflictingIDException ex) {
Logger.getLogger(Element.class.getName()).log(Level.SEVERE, "The child element '" + child.getUID() + "' (" + child.getClass() + ") conflicts with a previously added child element in parent element '" + getUID() + "'.", ex);
System.exit(0);
}
}
First, why the try/catch if you are catching the exception you just created?
Screen’s throwParserException does that too.
private void throwParserException() {
try {
throw new java.text.ParseException("The provided texture information does not conform to the expected standard of ?x=(int)&y=(int)&w=(int)&h=(int)", 0);
} catch (ParseException ex) {
Logger.getLogger(Screen.class.getName()).log(Level.SEVERE, "The provided texture information does not conform to the expected standard of ?x=(int)&y=(int)&w=(int)&h=(int)", ex);
}
}
And there's duplication in there too. Why not just do String message = "The provided…";?
Second, why System.exit? That can cause prolembs. In fact, it has for me. 
I'm using Clojure, which has an interactive shell/REPL, and I don't restart the JVM often; if I can: never. But that System.exit terminates my REPL. What you probably should do instead is create an Error, see http://docs.oracle.com/javase/7/docs/api/java/lang/Error.html.
Another thing I noticed was that orgPosition, orgDimensions and orgRelDimensions are not used. Take them out to make the code base smaller. A smaller code base is easier to read and understand. Also, I think this code in Element's addChild is odd:
child.setY(this.getHeight()-child.getHeight()-child.getY());
child.orgPosition = position.clone();
child.orgPosition.setY(child.getY());
First, you set the Y before cloning to orgPosition, which is odd. And then you set orgPosition's Y to something that it already is. But it's not used, so take it out.
Finally, I wanted to mention Element's resize method, not only because it contains a lot of duplication, but because it is a good example of something being complex, in the sense of the word that Simple Made Easy uses. I have included the code of the method below if you want to quickly look at it. As I mentioned in the beginning of this post, the code
if (getLockToParentBounds()) { if (y <= 0) { y = 0; } }
if (minDimensions != null) {
if (getY()+getHeight()-y <= minDimensions.y) { y = getY()+getHeight()-minDimensions.y; }
}
if (resizeN) {
setHeight(getY()+getHeight()-y);
setY(y);
}
is used 3 times, everytime the direction is N, NE, or NW. In fact, the code that has to do with any of the 4 directions (N, S, E, W) is duplicated 3 times for each direction that involves it! That's 3 times as much code as needed in total. Why not just do something like the following:
if (direction involves N) do the stuff that has to do with N;
if (direction involves S) do the stuff that has to do with S;
if (direction involves E) do the stuff that has to do with E;
if (direction involves W) do the stuff that has to do with W;
Which brings me to the complexity I was talking about: You have complected the directions (N, S, E, W) with each other in the Enum. As discussion above shows, it would be better if they were not combined as they are in Borders.SW, etc.
The thing that is important to understand about the kind of complexity that Rich Hickey discusses in Simple Made Easy, is that it's not just about large-scale things like separating logic from presentation. It is relevant on every level, and this is an example of that.
Oh, thought of another thing. See you below resize!
public void resize(float x, float y, Borders dir) {
float prevWidth = getWidth();
float prevHeight = getHeight();
float oX = x, oY = y;
if (getElementParent() != null) { x -= getAbsoluteX()-getX(); }
if (getElementParent() != null) { y -= getAbsoluteY()-getY(); }
float nextX, nextY;
if (dir == Borders.NW) {
if (getLockToParentBounds()) { if (x <= 0) { x = 0; } }
if (minDimensions != null) {
if (getX()+getWidth()-x <= minDimensions.x) { x = getX()+getWidth()-minDimensions.x; }
}
if (resizeW) {
setWidth(getX()+getWidth()-x);
setX(x);
}
if (getLockToParentBounds()) { if (y <= 0) { y = 0; } }
if (minDimensions != null) {
if (getY()+getHeight()-y <= minDimensions.y) { y = getY()+getHeight()-minDimensions.y; }
}
if (resizeN) {
setHeight(getY()+getHeight()-y);
setY(y);
}
} else if (dir == Borders.N) {
if (getLockToParentBounds()) { if (y <= 0) { y = 0; } }
if (minDimensions != null) {
if (getY()+getHeight()-y <= minDimensions.y) { y = getY()+getHeight()-minDimensions.y; }
}
if (resizeN) {
setHeight(getY()+getHeight()-y);
setY(y);
}
} else if (dir == Borders.NE) {
nextX = oX-getAbsoluteX();
if (getLockToParentBounds()) {
float checkWidth = (getElementParent() == null) ? screen.getWidth() : getElementParent().getWidth();
if (nextX >= checkWidth-getX()) {
nextX = checkWidth-getX();
}
}
if (minDimensions != null) {
if (nextX <= minDimensions.x) { nextX = minDimensions.x; }
}
if (resizeE) {
setWidth(nextX);
}
if (getLockToParentBounds()) { if (y <= 0) { y = 0; } }
if (minDimensions != null) {
if (getY()+getHeight()-y <= minDimensions.y) { y = getY()+getHeight()-minDimensions.y; }
}
if (resizeN) {
setHeight(getY()+getHeight()-y);
setY(y);
}
} else if (dir == Borders.W) {
if (getLockToParentBounds()) { if (x <= 0) { x = 0; } }
if (minDimensions != null) {
if (getX()+getWidth()-x <= minDimensions.x) { x = getX()+getWidth()-minDimensions.x; }
}
if (resizeW) {
setWidth(getX()+getWidth()-x);
setX(x);
}
} else if (dir == Borders.E) {
nextX = oX-getAbsoluteX();
if (getLockToParentBounds()) {
float checkWidth = (getElementParent() == null) ? screen.getWidth() : getElementParent().getWidth();
if (nextX >= checkWidth-getX()) {
nextX = checkWidth-getX();
}
}
if (minDimensions != null) {
if (nextX <= minDimensions.x) { nextX = minDimensions.x; }
}
if (resizeE) {
setWidth(nextX);
}
} else if (dir == Borders.SW) {
if (getLockToParentBounds()) { if (x <= 0) { x = 0; } }
if (minDimensions != null) {
if (getX()+getWidth()-x <= minDimensions.x) { x = getX()+getWidth()-minDimensions.x; }
}
if (resizeW) {
setWidth(getX()+getWidth()-x);
setX(x);
}
nextY = oY-getAbsoluteY();
if (getLockToParentBounds()) {
float checkHeight = (getElementParent() == null) ? screen.getHeight() : getElementParent().getHeight();
if (nextY >= checkHeight-getY()) {
nextY = checkHeight-getY();
}
}
if (minDimensions != null) {
if (nextY <= minDimensions.y) { nextY = minDimensions.y; }
}
if (resizeS) {
setHeight(nextY);
}
} else if (dir == Borders.S) {
nextY = oY-getAbsoluteY();
if (getLockToParentBounds()) {
float checkHeight = (getElementParent() == null) ? screen.getHeight() : getElementParent().getHeight();
if (nextY >= checkHeight-getY()) {
nextY = checkHeight-getY();
}
}
if (minDimensions != null) {
if (nextY <= minDimensions.y) { nextY = minDimensions.y; }
}
if (resizeS) {
setHeight(nextY);
}
} else if (dir == Borders.SE) {
nextX = oX-getAbsoluteX();
if (getLockToParentBounds()) {
float checkWidth = (getElementParent() == null) ? screen.getWidth() : getElementParent().getWidth();
if (nextX >= checkWidth-getX()) {
nextX = checkWidth-getX();
}
}
if (minDimensions != null) {
if (nextX <= minDimensions.x) { nextX = minDimensions.x; }
}
if (resizeE) {
setWidth(nextX);
}
nextY = oY-getAbsoluteY();
if (getLockToParentBounds()) {
float checkHeight = (getElementParent() == null) ? screen.getHeight() : getElementParent().getHeight();
if (nextY >= checkHeight-getY()) {
nextY = checkHeight-getY();
}
}
if (minDimensions != null) {
if (nextY <= minDimensions.y) { nextY = minDimensions.y; }
}
if (resizeS) {
setHeight(nextY);
}
}
float diffX = prevWidth-getWidth();
float diffY = prevHeight-getHeight();
controlResizeHook();
for (Element el : elementChildren.values()) {
el.childResize(diffX,diffY,dir);
el.controlResizeHook();
}
}
So, here is the other thing: Element’s constructor takes a texture path, loads the texture and creates a material in the constructor. That stuff should be factored out so that one can load a new material given a path without having to create a new Element. More functionality and more readable code (the contructor)!
Phew, that’s all I can think of right now. I hope you find it helpful! If you fix these problems, not only do you help yourself when working on tonegodgui, and others when reading/understanding it, but you might have a better chance of attracting contributors. For example, one could argue that I should fix these problems myself, and I wouldn’t really mind, but the thing is that I don’t understand everything (would be much easier with less duplication, better refactoring, less state and side effects!) in tonegodgui, and I would probably break something if I tried.