Skip to content
This repository has been archived by the owner on Apr 19, 2023. It is now read-only.

Code quality, unit tests, exceptions, scopes, etc. #51

Closed
wants to merge 4 commits into from
Closed

Code quality, unit tests, exceptions, scopes, etc. #51

wants to merge 4 commits into from

Conversation

Copy link
Member

@petrdvorak petrdvorak left a comment

Choose a reason for hiding this comment

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

Hello @kasahiti, thank you for your contributions! I made a quick inspection.

Most changes you propose are a very nice code cleanup. Some of them, however, require additional work.

Please let me go through this PR with @romanstrobl first. We need to decide if it is easier for us to ask you for implementing the changes and accept the PR (in such case, we will need to ask you to fill in our contributor license agreement), or open isolated issues and fix them separately on our end.

@kasahiti kasahiti marked this pull request as draft December 10, 2020 15:36
@kasahiti
Copy link
Contributor Author

Hello @kasahiti, thank you for your contributions! I made a quick inspection.

Most changes you propose are a very nice code cleanup. Some of them, however, require additional work.

Please let me go through this PR with @romanstrobl first. We need to decide if it is easier for us to ask you for implementing the changes and accept the PR (in such case, we will need to ask you to fill in our contributor license agreement), or open isolated issues and fix them separately on our end.

I can implement the changes on this draft without problem if it is easier for you. Do not hesitate to ask me !

Copy link
Member

@petrdvorak petrdvorak left a comment

Choose a reason for hiding this comment

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

We are getting there - still several issues to fix! 🙂

@@ -44,7 +46,7 @@ public static String toOctetString(BigInteger n, int length) {
char[] chars = hex.toCharArray();
int expectedLength = length * 2;
if (chars.length > expectedLength) {
throw new InvalidParameterException("Number is too large, length: " + chars.length + ", expected: " + expectedLength);
throw new InvalidParameterException(Constants.Exceptions.NUMBER_TOO_LARGE + ", length: " + chars.length + ", expected: " + expectedLength);
Copy link
Member

Choose a reason for hiding this comment

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

This "partial localization" seems a bit weird to me. If we externalize the strings (as a part of a "programming figure skating":)), we should probably stick to it and use MessageFormat (also, everywhere else where we append some string like this).

https://docs.oracle.com/en/java/javase/15/docs/api/java.base/java/text/MessageFormat.html

Copy link
Contributor Author

@kasahiti kasahiti Dec 11, 2020

Choose a reason for hiding this comment

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

Would something like this be ok ? :

  MessageFormat form = new MessageFormat("{0}, length: \"{1}\", expected: {2}.");
  Object[] args = {Constants.Exceptions.INVALID_SECRET, chars.length, expectedLength};
  throw new InvalidParameterException(form.format(args));

I've never really used MessageFormat.

Copy link
Member

Choose a reason for hiding this comment

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

@kasahity In situations like this, I usually look at what the JDK team does to see if what I do is "an improvement" or just "a change". JDK team uses inline Strings when throwing exceptions, see: https://github.com/openjdk/jdk/search?q=throw+new

So I think that one option is to fully externalize Strings and use MessageFormat like this:

// Constants.Exceptions.NUMBER_TOO_LARGE = "Number is too large, length: {1}, expected: {2}."
throw new InvalidParameterException(
    MessageFormat.format(Constants.Exceptions.NUMBER_TOO_LARGE , new Object[] {
        chars.length, expectedLength
    })
);

The other option is to revert this approach back to inline strings (which would probably be my choice since I am not sure that externalizing exception messages improves code readability):

throw new InvalidParameterException(
    "Number is too large, length: " + chars.length + ", expected: " + expectedLength
);

@romanstrobl
Copy link
Member

@kasahiti I'd like to ask you: next time when you open a pull request, please try to keep the number of changes small. The current pull request is trying to fix many things, most of which are not errors, just code style issues. However, such issues are rather subjective. Furthermore, when the fixes are not well thought out, like in case of several changes included in this pull request, new issues may be caused.

Examples from this pull request so far:

  • the synchronization fix introduced a performance issue
  • the removal of public keyword does not follow best practices for writing tests and may cause issues with testing tools
  • the refactoring of error messages to constants makes investigation of error messages harder
  • the change of style of mathematics code is not consistent (only several cases were fixed, but other ones remained using original style)

I suggest an alternative approach - when you find an issue in the code, either by yourself or using a code analysis tool (which seems to be the case in this pull request), at first raise an issue for each problem found in the code. One problem found = one issue. The developer who is responsible for the project can comment whether this is an issue or not and how it should be fixed. Then open pull requests which fixes individual issues. This way you will make the pull request more focused and there will be a smaller chance of causing new issues. It will make both your life and the developers life easier.

The situation would be different if you were already contributing to the project for some time, but as a new contributor it is hard to think through the impacts of your changes. Thank you.

@kasahiti
Copy link
Contributor Author

kasahiti commented Dec 12, 2020

@romanstrobl Thank you for your message.

I would like to clarify the following points :

  • I changed the synchronized keyword because I read performance comparisons where it mentionned that in latter JVMs, there are little to no issue performance ; but I agree that I should have tested this change in this project and see if it raises performance issue.
  • The removal of public keyword does follow best practices according to sonarqube; I will try to find again the exact rule
  • The refactoring of error messages now allows to change specific repeating messages in all the codebase in a second (before it would take you to check everwhere where a message is raised)
  • I agree that the change in mathematics style was completely subjective; I'll revert that.

I will stick to your alternative approach! I'm sorry that I didn't follow all your best practices in this open-source project. Small suggestion : what about adding a CONTRIBUTING.md describing the exact same workflow you suggest ? This way future contributors will follow all the rules and make your job easier ?

@romanstrobl
Copy link
Member

@kasahiti You are right about the unit tests, the removal of public keyword is new to JUnit 5. I am used to the old way from JUnit 4 and the two books I checked still use the old way.

@romanstrobl
Copy link
Member

romanstrobl commented Dec 12, 2020

@kasahiti I disagree about the usefulness of the error message change. Please check popular Java cryptography libraries, such as the Bouncy Castle library, Google Tink, and others, how the error messages in exceptions are created for low level cryptography code. Your change makes it harder to investigate exceptions when they occur. Your advantage about sharing a message is not practical, because it is a good idea to create unique error messages for each error, because this makes error investigation easier.

@romanstrobl
Copy link
Member

@kasahiti Good point about CONTRIBUTING.md, we will prepare it.

@petrdvorak
Copy link
Member

I agree with @romanstrobl in general - I opened issues for the problems we should address:

#52: Fix double-checked locking when constructing SecureRandom
#53: Review use of public keyword in tests
#54: Add private non-arg constructor to classes that should not be instantiated directly

Let's focus on addressing those in this PR and nothing else.

Regarding renaming value to val, s to s1, etc. - this seems to be a rather minor enhancement to something we never had practical issues with. I would remove this change from the PR and open a discussion on this. One needs to take the outputs from various tools (Sonar, etc.) with some distance, or else we would only have "tabs vs. spaces" conversations instead of making an actual software. 🙂

Regarding the exceptions, I think using an inline String is the correct way to go:

  • The typical task is the following: Customer sends the stack trace with an error message and you need to find what is going on. I am not sure that introducing constants and formatting code helps with this task.
  • The typical task is NOT the following: Localization and text changes. Exceptions must be the same all the time, usually, even typos would remain once introduced. Since error analysis tools may rely on a certain exception text, changing the exception message is an API change (in the loose sense of the word).

Regarding the contributor guidelines... We currently have over 70 repositories (including the private ones) and we want to avoid placing guidelines in each of them. It is a good idea to prepare some guidelines, though! We will do this in the scope of the wultra/wultra-docs#12 (separate docs repo) and link this from the developer portal. At this moment, we only have a nice guide (inspired by Netbeans) team on how to report issues here:
https://developers.wultra.com/docs/2019.05/powerauth-crypto/Reporting-Issues

@romanstrobl
Copy link
Member

May I suggest to fix individual issues @petrdvorak raised in separate pull requests instead of this one? It will be easier to review the pull requests, we'll be fixing issues we agreed that they definitely are an issue, and the github history will not include commits which are partial attempts for fixes. We can close this pull request when all discussions are concluded and all issues are raised and handled.

I already opened #55 for the volatile keyword. @kasahiti If you want to open pull requests for the other two issues, please go ahead, otherwise I will prepare the pull requests. Thank you in advance!

@kasahiti
Copy link
Contributor Author

@romanstrobl : agreed. I will close this draft, and fix the following issues in two separate PRs :

@kasahiti kasahiti closed this Dec 12, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants