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

SDAP-75 #22

Merged
merged 3 commits into from Jun 5, 2018
Merged

SDAP-75 #22

merged 3 commits into from Jun 5, 2018

Conversation

quintinali
Copy link
Contributor

Fix bugs in AggregateTriple.java found by sonar coverage

@asfgit
Copy link

asfgit commented May 21, 2018

Can one of the admins verify this patch?

Copy link
Member

@lewismc lewismc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@quintinali there are several areas for improvement here please address the comments, thanks.

FileWriter fw;
try {
fw = new FileWriter(file.getAbsoluteFile());
try(FileWriter fw = new FileWriter(file.getAbsoluteFile())){
bw = new BufferedWriter(fw);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this BufferedWriter not also be pulled into the try-with-resources?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ontology module is written by you and I'm not familiar with functions in the class. I found bw is defined as a class variable and can't be pulled it into the try-with-resource since bw is used in other functions.

} catch (IOException e1) {
e1.printStackTrace();
File[] files = new File(this.props.getProperty(MudrodConstants.ONTOLOGY_INPUT_PATH)).listFiles();
for (File file_in : files) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

file_in does not comply with Java code conventions.
It should be changed to fileIn

String ext = FilenameUtils.getExtension(file_in.getAbsolutePath());
if ("owl".equals(ext)) {
try {
loadxml(file_in.getAbsolutePath());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

loadxml should be changed to loadXml

}
} catch (IOException e) {
// TODO Auto-generated catch block
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Never leave comment stubs present in production level code. Either LOG or rethrow this exception.

@@ -91,6 +91,7 @@ public Object execute() {
e.printStackTrace();
} catch (InterruptedException e) {
e.printStackTrace();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should not be printing these stack traces, we should be logging them or re-throwing the exceptions.

@lewismc lewismc merged commit 6f304fa into apache:master Jun 5, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants