Java EE core utilities module review

Date/time: 15 November 2007 15:00 CET
Reviewee(s): Andrei Badea, Erno Mononen
Reviewer(s): Petr Hejl, David Konecny, Tomas Mysik
Code source: j2ee/core/utilities download
Meeting room: Mercury

Reviewers notes

PH01: SourceGroups tests are failing - this should be fixed. (done)

PH02: Avoid star imports in GenerationUtils and SourceUtils.

PH03: Consider using the special package for progress support.

PH04: The ProgressSupport synchronization model (cancelled and currentActionIndex in ActionInvoker) is confusing. The way it is written is probably correct, however it is hard to read and fragile (partially fixed in utilities.diff).

PH05: The ProgressSupport api usage is a bit strange. Typically user has to create instance and then call invoke. While the invoke is running it can't be invoked again. Simpler usage would be with static invoke method (utilities.diff).

PH06: Package private field should be private with package private accessor. However I don't like the idea of publishing the field due to unit tests at all - this necessity can be eliminated with static invoke method (PH05).

PH07: Document synchronization model of ProgressSupport to make future maintenance easier.

PH08: The documentation of invoke says "Returns true if all actions were invoked". I'm not sure about this. Shouldn't it be: "Returns true if invocation was not cancelled"?

PH09: The naming of classes is imo incosistent - some classes are named *Utils (SourceUtils, GenerationUtils) and others *s (Strings, Wizards). (not accepted)

PH10: Wizards, line 88 - use Integer.valueOf(). It should be a bit faster for small numbers. (done)

PH11: SourceGroups, line 257 - small opt. List<FileObject> result = new ArrayList<FileObject>(urls.length). (n/a - method removed)

PH12: GenerationUtils, line 83, 84 - should be possible to make the constants private.

PH13: GenerationUtils and SourceUtils - factory methods are not documented.

DK:

• fix licence in: Bundle, Strings, JavaIdentifiers, Wizards (done)

• add Javadoc on all public/protected methods - it is API after all (done)

• in SourceGroups delete methods createFoldersToSourceGroupsMap, getTestSourceGroups, getTestTargets, getFileObjects and rewrite getJavaSourceGroups to just iterate sourceGroups and for each check UnitTestForSourceQuery.findUnitTests(sourceGroup.getRootFolder()).length > 0 or am I missing something? (done)

• SourceGroups.isFolderWritable: I'm not sure when this check is used but if package does not exist it always returns true. should not it return in such a case sourceGroup.getRootFolder().canWrite()? (done)

• SourceGroups.getPackageForFolder should throw IllegalStateEx if FileUtil.getRelativePath returns null (done)

• ProgressSupport:
  • typo in "dispathing", "the a";
  • "void invoke" should be boolean invoke;
  • SynchronousAction is actually "EventThreadAction";
  • ProgressSupport.Action.isEnabled - what's the usecase? why would you add action which is disabled?
  • why Context and Progress are two different classes?;
  • Progress.getPanel() should be removed and ActionInvoker should use directly progressPanel;
  • is EventThreadAction expected to show its own "progress UI"?

• SourceUtils.newInstance(CompilationController controller) should document that null can be returned or perhaps should throw an exception instead; isMainMethod not used;

• GenerationUtils:
  • GenerationUtils.newInstance(WorkingCopy copy) should document that null can be returned or perhaps should throw an exception instead;
  • merge two createDataObjectFromTemplate into one;
  • consider refactoring common body from three createAnnotationArgument methods into one

TM01: general: coding style, '*' imports, missing JavaDoc, incorrect licenses, usage of {@link CLASS} instead of <code>CLASS</code> in JavaDoc, missing @authors tags (done)

TM02: JavaIdentifiers, line 59: StringTokenizer should not be used (done)

TM03: JavaIdentifiers, line 126: fqn.endsWith('.') could be used (done)

TM04: SourceGroups, line 107: missing unit test for isFolderWritable() method, it's wrong (done)

TM05: ProgressPanel, line 64: usage of FQN (done)

TM06: ProgressSupport: missing sample(s) of usage in JavaDoc (maybe could be applied for few other classes as well)

TM07: ProgressSupport, line 403 and 418: missing 'final' keyword

TM08: unit tests: GenerationUtilsTest: probably MockLookup could be used

Attachments

j2ee-core-utilities.txt		187576 bytes
utilities.diff		11010 bytes