Return to Answer

replaced http://stackoverflow.com/ with https://stackoverflow.com/

edited May 23, 2017 at 12:41

Community Bot

• 1

Inline Side-by-side Side-by-side Markdown

typo, formatting of list in finally, extended answer as OP wanted

edited Sep 5, 2015 at 9:59

LAFK says Reinstate Monica

• 386
• 1
• 9

Inline Side-by-side Side-by-side Markdown

answered Sep 1, 2015 at 17:15

LAFK says Reinstate Monica

• 386
• 1
• 9

Quite nice helper. Also, thanks from RPG fan, I'll check out Eclipse Phase later as well. :-)

My remarks:

No packages!

If the project is to be a portfolio one, use package keyword and put your classes in packages. There is number of advantages:

1. packages organize areas or your program
2. they put related classes together
3. lack of packages means, that your code is all in ONE package, so-called default package. That's considered a code smell.
4. so-called Fully Qualified Name of a class includes package. Therefore, I can ran number of classes called MyClass within 1 JVM, as long as they have different packages. With default package, there's greater risk of confusing JVM: which MyClass is which?

Read on packages prior to choosing how to group your code.

Code quality, various remarks

I'm sorry for not being able to process and review code fully. Here are things that stood out for me.

1. Interfaces methods are public by default. You need not specify that.
2. Each time you have an if X then true else false simplify it to return X; (Skill, LifePathUI, CharacterSheetUI).
3. If you import a.b.*, there's no need to import a.b.something additionally, as a.b.* already did so.
4. In number of places you have methods, values, local variables and parameters that are never used. Consider cutting. IDE can show you these, as well as FindBugs or Checkstyle or SonarQube tools.
5. I found three type casts that were redundant. All to int. All in Character class.

I'm mentioning these only briefly as when one usually checks code for clarity, one uses tools. Tools report these, so you should be aware, if you want to use project to show your skills. Some, you may want to fix, depending on whom you want to impress.

Javadocs

1. The class Javadoc is often empty, safe @author tag. Please explain the class in class Javadoc, number of IDEs show such description when you hover over object of X type. It's quite helpful for others. Examples: Aptitude, Character.

2. Javadocs for getters and setters are sometimes poor (autogenerated, it seems). Please remove. They bring no value, functions are trivial one-liners with very common purpose and naming adhering to convention. That is enough, Javadocs for such things are only making the file bloated. Examples: Aptitude, Character.

3. If you have file comments that are empty, remove them. If you have file comments, that hold value to class (example: Function, Sleight, ExistsValidator) not to file, integrate them as Javadocs. Good file comment might be about encoding, or file being tool-generated. I'm yet to see one that's about code and it makes sense for it to be a file comment. For your convenience: File comment is a comment at the top of the file (or just under package and imports, but prior to class javadoc and class itself).

Working with Strings

There are two aspects here. One, this application is very String-reliant. Are you sure you wouldn't want to use types more? The other thing is technical aspect of using Strings.

Technically

You have a Utils class, that works with Strings heavily. Please take a look at projects like StringUtils from Apache (or other, similar). You may find them to your liking, they do a lot of nice String operations: join, split, isEmptyOrBlank, etc. I believe there's also a sister library, NumberUtils, which has a method to check if String contains a digit.

Of course, feel free to reject pulling outside libraries into your project - there may be good reasons not to. However:

1. NumberUtil.isNumber(String str) also factors in Unicode digits
2. Knowing common libraries looks good in portfolio.
3. You don't need to maintain code that doesn't add to your domain (Eclipse Phase RPG helper).

Up to you here. Both libraries can be downloaded together if you pull in Apache Commons. There's also Google alternative: Guava (even better IMO).

More info: http://stackoverflow.com/questions/1444437/apache-commons-vs-guava-formerly-google-collections

http://stackoverflow.com/questions/7015739/is-there-a-viable-generic-alternative-to-apache-commons-collections-collectionut

http://eclipsesource.com/blogs/2013/11/06/get-rid-of-your-stringutils/

Objects vs Strings

Consider Sleight class. In excerpt taken from it, everything is a String!

private String sleightType; 
private String isExsurgent;
private String name; 
private String psiType;
private String actionType;
private String range;
private String duration;
private String strainMod;
private String skillUsed;
private String description;
// stores all the below sleights
public static HashMap<String,Sleight> sleightList = new HashMap<String,Sleight>();
 
/**
 * @param sleightType Classification of Sleight : chi, gamma, etc
 * @param isExsurgent Whether the sleight is meant to be exsurgent only
 * @param name Name of the sleight
 * @param psiType Active or Passive
 * @param actionType What type of action the sleight uses
 * @param range Range of the sleight
 * @param duration Duration of the sleight
 * @param strainMod Strain mod of the sleight (if any)
 * @param skillUsed Skill the sleight uses (if active
 * @param description Human readable description of sleight

Now, from really great javadoc on Sleight class constructor, I can see that we're actually having an enum here, a boolean, and so on. Number of types. Why remain with only Strings? Booleans behave nicely in flow control, enums have compile-time checks for whether I chose correct value or not, as well as run-time checks, and so on.

Types give you great benefits over just Strings.

Domain classes

Please put them in package. Core, base, or ep (from Eclipse Phase, but lowercased).

Aptitude

1. Aptitudes are presented as String array. For such a case, I'd use Enum type. It's purpose are enumerations.

2. Aptitude maximum is set to 40. I'd add a comment citing source for this. If Eclipse Phase will get a second version (or any other), that may change. Also, if someone recalls this to be fifty, quoting source allows him/her to check it.

3. Is subValue method used anywhere?

4. Aptitude max and aptitudes array are both public static. Is that the way you want it?

Character

This class has 1357 lines. Consider splitting it to several classes. There's too much functionality in one class.

Indentation

You are indenting your files with Tabs. That's quite OK, but be aware, that your tab and mine may differ. I can set my OS to make Tab equal to 8 spaces. You may set it to 4. Somebody else, to two. That means, the indentation will not be consistent. IIRC, the standard is to set Tabs to 4 spaces and set your IDE to use SPACES instead. This way, when you hit Tab, IDE inserts 4 spaces and behaviour is consistent across everybody's machine.

Still - your flavour here. That's minor thing IMO (though some folks rage wars over it).

Your specific questions

Last but not least!

Is GridBagUIPanel a proper abstraction of functionality, or is it too little/too much?

At 557 lines of code, I'd say it needs a split. Number of methods should be extracted out of it. Why do you have 4 different constructors there? What about shortening 4 methods to add components into one, using int vararg? Is it really panel's job to have 4 methods to add components and 8 more methods to add other things? Perhaps you need PanelAdder or PanelDecorator where all those add methods would be? Such a class needs only one private field Panel and it can operate on this panel and add things to it as you please.

Is there anything I should be doing to clean up init()/update() more in LifePathUI?

YES.

1. When constructing your application, read both internal files into memory and keep them there. No need to init data proc when creating UI. These files are crucial and you will need them sooner or later. Don't bundle them with UI, bundle them with domain. That clears dependency I've spotted in LifePathUI's constructor.

2. Init method is rather long. Consider grouping relevant code together and putting it in functions. Consider creating object, which only has one purpose: to initialize LifePathUI. LifePathInitializer. Whenever you spot a piece of code with comment at the top, it's prime candidate for a method.

3. Aim for fluent interface (see below).

Consider this code:

import javax.swing.*;
/**
 * Created by LAFK on 01.09.15.
 */
public class MyJFrame extends JFrame {
 private final MyJFrame frame;
 public MyJFrame(JFrame frame) {
 this.frame = (MyJFrame)frame;
 }
 public MyJFrame addStatPanel(JPanel panel) {
 frame.add(panel);
 return frame;
 }
}

Now, let's say it has more methods, including addBonuses, addTotal, addSomethingElse and I can create MyJFrame and just add things to it easily, within few lines. Let me know if this is unclear.

Am I using things right with the TextChangeListeners and Button Events in LifePathUI? Defining classes inside method calls still feels

very off to me.

You'll get used to defining classes on the fly.

Is there any glaring reinventing the wheel or other mistakes going on? I want to cut out any particularly embarrassing things since this

is one of my few projects that would be able to be used in portfolios and such.

Strings vs Objects. Lack of packages. Public in interfaces. Too many unused things. Fix these and you're much better looking IMO. Also - if you want to present it to someone who likes testing, do change the name of the class UnitTests, as you may get scolded. :-)

Finally

Do not be discouraged. Despite all my findings, this is not such a bad code (that's not sarcasm). You obviously put lot of work into it and this can be spotted. There's just number of things yet to do.

Kudos for:

• coding this in the first place
• putting it up here for code review
• putting it on GitHub = sharing with everyone
• writing UI for it, number of such tools uses only esoteric CLI switches
• keeping files from Eclipse Phase in the repo
• writing readme for others
• writing number of comments in your code