Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix a few alerts from LGTM.com #168

Merged
merged 3 commits into from
Jan 30, 2019
Merged

Fix a few alerts from LGTM.com #168

merged 3 commits into from
Jan 30, 2019

Conversation

robertbrignull
Copy link

This PR fixes a few alerts that were raised on LGTM.com. These are the ones where I could quickly tell what the right thing to do was.

There are quite a lot of other interesting alerts there and the majority that I looked through seem to be correct. You may want to ignore the "array index out of bounds" alerts though as most of those seem wrong. In a few other cases the alert looked corrected but the code didn't seem to be used anywhere so I wasn't sure what to do. Probably with your knowledge you'd be able to tell what's going on quite quickly.

  • About the alert in IncompleteItem, the map has keys of type ItemCaract but we were looking up something of type NumericType so it would always be false. I'm assuming this is a mistake and we should be checking if the ItemCaract has been seen before. It's a shame that Map.containsKey doesn't use generic types otherwise this would be caught at compilation time.

  • In SuggestEngineResult.sameString I think it would have returned true for any pair of non-null strings or if either string was non-null. I'm assuming that this isn't what was intended. I found this because it said that on the a.equals(b) line, a would always be null, which is the case because it would always have returned if a was non-null. It's a shame we can't use the Java 7 Objects.equals but if I understand the pom file correctly this project builds using Java 5.

  • In HumanDuration, the ms variable was passed to String.format but in all cases the format string did not have enough formatting arguments to make use of it. Assuming that it is outputting the correct human readable string already then the thing to do I think would be to stop passing the ms variable.

For full disclosure, I do work on the team that makes LGTM.com, so I'm using my powers there to raise the point that the "array index out of bounds" alert has quite a lot of false positives. Do let me know if you have any questions at all.

@arnaudroques arnaudroques merged commit 8b34df2 into plantuml:master Jan 30, 2019
@arnaudroques
Copy link
Contributor

Thanks for your contribution, it really helps.
There are some dead code inside PlantUML that I should remove (SuggestEngineResult for example)

About array, the fact is that most of the time, the array has an even number of elements (this is because it store x and y coordonate). I should put some assertion on this. I don't know if it will help LGTM.com to remove the false positives.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants