Skip to main content

[JSR-354] Re: suggestions

  • From: Manuela Grindei <manuelag2004@...>
  • To: "jcurrency_mail@..." <jcurrency_mail@...>
  • Subject: [JSR-354] Re: suggestions
  • Date: Sun, 11 May 2014 21:45:11 +0100 (BST)

Hi Otavio,

Thank you for your quick reply.
I am familiar with creating pull requests in general, but I don't know the 
procedure for this project. How should I do it? Please advise.

Best wishes,

Manuela Grindei
On Sunday, 11 May 2014, 19:38, Otávio Gonçalves de Santana 
<otaviopolianasantana@...> wrote:
 
Hi Manuela.
What do you think do a pull request with these submits?
If you need a help, please, let me know.
On May 11, 2014 3:16 PM, "Manuela Grindei" <manuelag2004@...> wrote:

Hi,
>
>I've taken a first look at the code these days and I want to make some 
>suggestions:
>
>- ExchangeRate.compareTo: The code contains:
>
>    public int compareTo(ExchangeRate o){
>        if(o == null){
>            return -1;
>        }
>       ...
>    }
>
>Instead of returning -1, the method should definitely throw NPE.
>
>According to the specification for Comparable interface 
>(http://docs.oracle.com/javase/7/docs/api/java/lang/Comparable.html ;): "Note 
>that null is not an instance of any class, ande.compareTo(null) should throw 
>a NullPointerException even though e.equals(null) returns false."
>
>
>- The strings used as exception messages should be extracted into a 
>properties file. It would be great to have properties with placeholders. 
>This would make changes of messages a lot easier, without touching the Java 
>code. In addition, it would also help with internationalization.
>
>
>- There are some classes with private default constructors. As explained in 
>"Effective Java", it would be a good idea to throw an error/exception from 
>the constructor, because even if it's private, it can still get called using 
>reflection. In "Effective Java", I think an AssertionError was thrown. 
>
>Some refactoring ideas for the JUnit tests:
>
>- the names of some of the tests are quite long and confusing: e.g.: 
>testExchangeRateExchangeRateTypeCurrencyUnitCurrencyUnitNumberStringLongLongExchangeRateArray
>
>- in general, to make debugging easier, there should be only one assert per 
>test 
>
>- the asserts should have meaningful messages to be displayed in case a test 
>fails, it would help with debugging (the assert functions are overloaded to 
>take a String message parameter)
>
>- some tests have in the beginning the same code, it could be extracted 
>inside a @Before or @BeforeClass method, it would make the code more 
>readable and concise
>
>- in case we test that a method throws an exception, it would be a good idea 
>to also check that the exception message is what it should be
>
>I hope these comments are useful; if you have any questions, do
 not hesitate to ask me.
>
>Best wishes,
>
>Manuela Grindei
>

[JSR-354] suggestions

Manuela Grindei 05/11/2014

[JSR-354] Re: suggestions

Otávio Gonçalves de Santana 05/11/2014

[JSR-354] Re: suggestions

Manuela Grindei 05/11/2014

[JSR-354] Re: suggestions

Otávio Gonçalves de Santana 05/11/2014
 
 
Close
loading
Please Confirm
Close