Logging framework and code review

[nuest]

Did you consider using a logging framework yet?

Would you welcome a code review from an experienced software developer from 52N?

[cDanowski]

Benno has it in mind, I think we will tackle this issue in during the upcoming semester break. Is there any logging framework you prefer/recommend?

A code review of an experienced software developer will be very helpful! I welcome this offering and I think Benno thinks alike.

[nuest]

Logging framework: Log4j2 (http://logging.apache.org/log4j/2.x/) is quite new and supposed to be fast (faster than the alternative: logback). To not depend on a particular framework, or if you want to allow users to use a different framework, you can use slf4j (http://www.slf4j.org/) as an abstraction layer. slf4j + logback is a commen setup at 52N, but log4j2 seems to be a bit ahead of logback with the latest version, so I'd recommend to use slf4j + log4j2.

Regarding the review: Would you do this in this issue, or on the Trello board? Can you tell me when you will have a window for the review (i.e. not doing any commits for half a day so that the reviewer does not look at something that is changed anyway) ?

[cDanowski]

Okay thank you for the recommendation. We will have a look into it during the next weeks.

Tomorrow we will have a Skype meeting with Adhitya (15:30 CEST) on which we will talk about this issue. I think after that we can tell you a concrete window.

[cDanowski]

@nuest we just had a Skype meeting in which we discussed th above topics. We will start logging the next week or the week after that. Regarding the code review: we agreed that we would like to wait for another two weeks until there is some more code. During the next days changes to the repository are likely. Would that be okay for you?

[nuest]

I don't see the reason why to delay starting with logging, but that's your choice.

Regarding the review: Sure - just get in touch a couple of days ahead and I'll see which developer can do it. Looking forward to "see" more :smile:

[nuest]

Let's leave this issue open for the review.

[cDanowski]

@nuest Hi Daniel, sorry for the late response. We would like to come back to your offering about a code review. We now have a good state of the project and could pause for some days. Do you require anything else from us? How do we proceed with the code review?

Thank you in advance!

[nuest]

I asked around - please ping me again in a couple of days if I forget to get back to you.

[cDanowski]

@nuest What about the code review? Is anyone free to do it?

[nuest]

I'll take some time to do it myself. Are all current developments integrated in the master branch? Can I just start with the README?

[kamakshidasan]

Yes. All developments from my side have been integrated into the Master branch.

On Mon, Aug 10, 2015 at 6:37 PM, Daniel Nüst notifications@github.com wrote:

I'll take some time to do it. Are all current developments integrated in the master branch? Can I just start with the README?

— Reply to this email directly or view it on GitHub https://github.com/cDanowski/worldviz/issues/6#issuecomment-129436095.

[cDanowski]

@nuest okay thank you :) Do you intend to only review the code of the GSOC project, or do you also intend to review the code that Benno and I have put into this repository? If you only want to review the GSOC-related code then we should point you to the corresponding subpackages.

[nuest]

Ok, here we go. Several things were discussed before, but I include them here so that I don't miss anything. I trust you will create all the separate issues based on my remarks as you see fit. Also, I tried to formulate short comments, please excuse me if some thing sound harsh.

Project setup

• use fork and pull for all commits, and have the main repo at 52N (I know this is planned ;-) )
• mention worldviz on the Triturus website at 52N
• for all documents (i.e. not in issues like this one :smile: ) please use "52°North" (with degree, no spaces), never 52N
• the repository is quite huge, which is not due to the lib-repository as I first expected but due to the data... binary data files (and jpgs/shapefiles are binary) do not belong in a git repository actually, but they will now be in the history. are all the files needed, or could they be put in a zip file as a wiki attachment and downloaded by users from there? another alternative: a sepearte "worldviz-data" project, which allows you to keep the source code repo clean.
  • lib-repository: you should move the respective libs to 52North organisation and then we create a jenkins build job (or you do manual deployments) to the online Maven repository.
  • some large files are even duplicated (several times - worldmap!) in the demo directories. can you remove those?
  • where does poly2tri come from? The pom has no author or license information.
  • where does csvreader come from? The pom has no author or license information.
  • Thumbs.db files - are you using them? If not, gitignore them.
• file test/graph.x3d - follow the maven directory structure and move this to src/test/resources (or where it fits better)
• /doc - good idea, but try to avoid placeholders, add the directory when there is content
• nbactions.xml - a Netbeans-specific file, which you normally do not commit. what if y use wants to use a different setup or has different paths - you will be committing many changes to this file. Or are these actions the same for all users?
• commit messages: it is common practice to write them in present tense. from the perspective of the repository, the commit is happening "now". try to avoid phrases like "a bit", and make them more meaningful. "Added demo" can be found several times. Which demo? Name? "Another bug solved" - which one?
  • I recommend to have a git issue for every bug that you solve, you can then reference them in the commit message (or even close them https://help.github.com/articles/closing-issues-via-commit-messages/)
  • there are several commits "clearer formatting". Do you use a code formatter? The problem with these commits that change the layout of a file is that, when another person merges them, it looks like the whole file has changed but in reality only the formatting changed. Try to avoid, reformat only the lines that you edited.

Source code

(I just randomly look at files - please point me to the package where Adhitya did most developments - Thanks!)

• all classes need a license header, see https://wiki.52north.org/bin/view/Documentation/BestPracticeLicenseManagementInSoftwareProjects
  • YES, the plugins must be executed on every build. The build must break when a new source code file or library dependency is added.
• logger.isDebugEnabled() is not needed anymore if you use the template mechanism of slf4j: log.debug("there was a problem with item {} in list {}", item, list);, because there is no performance overhead since item and list will never be touched.
• are all your applications run via main(...) methods? Please add documentation on how to run the applications

Tests

• very good you have tests :-)
• make the test method names more speaking, e.g.testTriangulationOfMultipolygon().
• also check that your test functions do not cover to much different aspects. the reporting is a lot easier to read if one method only tests one aspect. also helpful is message you can pass to the Assert.assertThat(..) method because you can just read out the test aloud: assertThat("polygons that are tringulated have a non-empty TIN list", vgTINS, is(not(empty()))) (I'm also using hamcrest matchers here, very useful)
• no need to catch exceptions in tests! they are included in the reports anyway. It is fine if a test method just has throws Exception (unless you're testing the exceptions of course).

Documentation

• "traditional visualisation pipeline" - can you add a reference for that, for the uninitiated like me?
• "simply run Maven's 'package' task" - use Markdown highlighting and put in the actual command, also add a link to Maven documentation: "simply run Maven's 'package' task: mvn package."
  • the "typical" task for Maven is mvn clean install
  • when I clone the repo and run mvn clean install I actually get an error. That should not be the case. seems something is wrong in the repo configuration... maybe you have the geootols repo configured in you own ~.m2/settings.xml ?

[ERROR] Failed to execute goal on project 52n-v3d-worldviz: Could not resolve dependencies for project org.n52.v3d:52n-v3d-worldviz:jar:1.0-SNAPSHOT: The following artifacts could not be resolved: org.geotools:gt-shapefile:jar:13.1, org.geotools:gt-opengis:jar:13.1, org.geotools:gt-data:jar:13.1, org.geotools:gt-api:jar:13.1, org.geotools:gt-main:jar:13.1, org.geotools:gt-metadata:jar:13.1, org.geotools:gt-referencing:jar:13.1: Could not find artifact org.geotools:gt-shapefile:jar:13.1 in lib-repository (file:///C:\Users\Daniel\dev\git\worldviz/lib-repository/) -> [Help 1]

• Contributing > put into a separate file "CONTRIBUTE.MD", see https://wiki.52north.org/bin/view/Contributor/ContributeMd
• a few examples of how to start using the library would be useful (e.g. by pointing to some demo files)

[cDanowski]

thank you for the great feedback :) . We will deal with each recommendation during the next weeks. Your comments are very helpful!

The main subpackages with Adhityas work are as follows:

• src/main/java/org/n52/v3d/worldviz/featurenet
• src/main/java/org/n52/v3d/worldviz/demoapplications (without the "ene" supbackage)
• src/main/resources/Javascript