Skip to main content

[JSR-354] Re: suggestions

  • From: Otávio Gonçalves de Santana <otaviopolianasantana@...>
  • To: Manuela Grindei <manuelag2004@...>
  • Cc: "jcurrency_mail@..." <jcurrency_mail@...>
  • Subject: [JSR-354] Re: suggestions
  • Date: Sun, 11 May 2014 20:04:41 -0300

I just do a pull request explain the submit.
You can use it [1], for example.
[1]https://github.com/JavaMoney/jsr354-ri/pull/7



On Sun, May 11, 2014 at 5:45 PM, Manuela Grindei
<manuelag2004@...>wrote:

> 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, and e.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
>
>
>
>


-- 
Cheers!.

Otávio Gonçalves de Santana

blog:     http://otaviosantana.blogspot.com.br/
twitter: http://twitter.com/otaviojava
site:     *http://about.me/otaviojava ;<http://about.me/otaviojava>*
55 (11) 98255-3513


[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