On Dec 11, 2010, at 4:23 PM, Rupert Westenthaler wrote: > Hi > > see my comments inline > (Note that I removed points of the original mail with no comments) > > Am 11.12.2010, 12:32 Uhr, schrieb Stefane Fermigier <[email protected]>: >> Hi, >> >> during the trip back from Amsterdam I spent some time reviewing the >> current Stanbol code base, and fixing some issues. >> >> Here are some remarks: >> >> 1. Stanbol doesn't build, it complains that >> eu.iksproject:eu.iksproject.kres.shared.dependency.owlapi:jar:0.6-SNAPSHOT >> is missing. >> >> This is a major issue since it prevents building the whole stack, and >> putting it under continuous integration. >> > There is already an issue on that. > see https://issues.apache.org/jira/browse/STANBOL-6
Yeah, it's already one week old and nothing seems to have been done to fix it. In the meantime it's a blocker for building the whole project. It's not even possible to open the project in Eclipse actually! (Since mvn eclipse:eclipse fails with the same error). >> 2. Lots of files (almost 600) have tabs instead of (or mixed with) >> spaces. This is bad behavior, because, depending on how your editor is >> set-up (tabs can be 2, 4 or 8 spaces long, depending on people choices), >> some people will get ugly looking code. >> >> I have a massive patch that expands all the tabs to 8 spaces. Will apply >> it soon, unless there is some protests. > > I usually use an intend of 2 spaces, so I would not be very happy with 8. I'm talking about tabs. Tabs are usually expanded to 8 chars, but some people expand them to something different, so it's much better to not have tabs at all in your source code, or, even worse, to mix tabs with spaces. Regarding indentation, which is a related issue that I forgot to write about, most of the open source code out there is indented with 4 spaces per indent (for the Java code, for JavaScript it's 2 or 4, for Python it's 4 in the python.org guidelines but 2 in the google internal guidelines, for XML or HTML it's more often 2 than 4). So I'm strongly for going with 4 spaces per indent everywhere in the Stanbol Java code (and 2 for XML and HTML). > >> >> - There are lots (~20) files that have been copied from geonames (I >> believe). I'm not really happy about this, specially since >> >> a. It's not obvious which files have been modified and which ones have >> been kept as-is. >> >> b. When file as been modified, it's not obvious where. > > I added the "Double getScore()" to the class "org.geonames.Toponym". Just for adding one method you had to copy 20 files ? > This extension is needed by FISE to get score values for recommended > locations. > In addition I provided a pom that creates a bundle. OK. > More infors: > - the discussion of the fise list: > http://lists.iks-project.eu/pipermail/iks-fise/2010-July/000347.html > - the issue with my changes on sourceforge.net > > http://sourceforge.net/tracker/index.php?func=detail&aid=3018007&group_id=175424&atid=873182 I don't care what's written on a mailing list or a bug tracker, if it's not commented in the code there is very little chance I will stumble upon it when I'm reviewing the code. > >> >> 8. Remember that interfaces are not the right place to define constants. >> And it's usually better to use enums for integer constants (see Bloch's >> Effective Java, 2nd ed., chapters 19 and 30). >> > Within RICK I used the Interfaces to define the string constants used > as keys for the OSGI Configs. > The intension of that was that implementations need not to invent there > own keys to configure properties defined in the Interface. > No problem to change that if that is against some kind of best practice. Thanks. Also the best practices change over time (because of experience gained or new paradigms, such as TDD or BDD or DDD, etc. S. -- Stefane Fermigier, Founder and Chairman, Nuxeo Open Source, Java EE based, Enterprise Content Management (ECM) http://www.nuxeo.com/ - +33 1 40 33 79 87 - http://twitter.com/sfermigier Join the Nuxeo Group on LinkedIn: http://linkedin.com/groups?gid=43314 New Nuxeo release: http://nuxeo.com/dm54 "There's no such thing as can't. You always have a choice."
