Thanks for the thorough code review Benedikt! Comments inline. > 1. IP clearance! There are several comments talking about code adapted from >PHP libraries. This needs to be listed in NOTICE.txt. For an example see >the NOTICE.txt in [lang]. Filed https://issues.apache.org/jira/browse/SANDBOX-497 And first try to fix it:
https://git1-us-west.apache.org/repos/asf?p=commons-text.git;a=commitdiff;h=refs/heads/SANDBOX-497 Does that look correct? I also checked with the author before porting it to the initial Java version, and told him about the [text] component - https://github.com/jasonpriem/HumanNameParser.php/issues/11 > 2. The names package needs work. Completely agree. We can try to make it thread safe too. This will take a little longer. File new issue for tracking it https://issues.apache.org/jira/browse/SANDBOX-498 > 3. There are a some useless JavaDocs, which can be dropped. For example: > (...)> Better rename the variable to middleName and drop the comment. What about renaming the variables but leaving the comments? > 4. Several classes have no tests. The overall test coverage looks good, so >this may not be a problem. Before the initial release I can start a development cycle just to add new tests for parts that seem important - like code with somewhat high [+5] cyclomatic complexity and uncovered branches. Just to make sure that the initial release will have an acceptable quality and less hidden bugs :-) > 5. I don't think the shade configuration is correct. To me it looks like >commons-text depends on [lang] and also shades some classes. Do we really >need the explicit dependency? Furthermore, why do we need to shade so many >classes? Why do we need anything beside StringUtils? My mistake. I copied the [functor] configuration without much thinking. IIRC, this dependency was added because of the names package. But it shouldn't be too complicate to replace it. It is mainly used for checking empty/blank values. So let's drop the [lang] dependency and use something like `$VAR != null && ! "".equals($VAR)` instead? Thanks again for reviewing the code! All the best,Bruno From: Benedikt Ritter <brit...@apache.org> To: Commons Developers List <dev@commons.apache.org> Sent: Sunday, April 19, 2015 9:01 PM Subject: [TEXT] Review of the code base Hi all, I've looked through the code base of [text] and I already did some clean up. However there are a few things, that need more work IMHO: 1. IP clearance! There are several comments talking about code adapted from PHP libraries. This needs to be listed in NOTICE.txt. For an example see the NOTICE.txt in [lang]. 2. The names package needs work. Currently the HumanNameParser takes a Name object (which is just a wrapper around a String) as parameter. The parse result is then saved in fields (e.g. firstName). This makes the parser unusable after it has parsed a name. I think the API should be reworked such as: - The constructor of the parser takes configuration options which can be reused for several names to parse - the parse method takes a string as parameter, containing a name - the parse method returns an immutable Name objects which has getters for firstName, lastName etc. 3. There are a some useless JavaDocs, which can be dropped. For example: /** * Middle name. */ private String middle; Better rename the variable to middleName and drop the comment. 4. Several classes have no tests. The overall test coverage looks good, so this may not be a problem. 5. I don't think the shade configuration is correct. To me it looks like commons-text depends on [lang] and also shades some classes. Do we really need the explicit dependency? Furthermore, why do we need to shade so many classes? Why do we need anything beside StringUtils? Nevertheless, this library looks very good. Kudos to Bruno for pushing this forward! Regards, Benedikt -- http://people.apache.org/~britter/ http://www.systemoutprintln.de/ http://twitter.com/BenediktRitter http://github.com/britter
