• Post Reply Bookmark Topic Watch Topic
  • New Topic
programming forums Java Mobile Certification Databases Caching Books Engineering Micro Controllers OS Languages Paradigms IDEs Build Tools Frameworks Application Servers Open Source This Site Careers Other all forums
this forum made possible by our volunteer staff, including ...
Marshals:
  • Campbell Ritchie
  • Liutauras Vilda
  • Tim Cooke
  • Jeanne Boyarsky
  • Bear Bibeault
Sheriffs:
  • Knute Snortum
  • paul wheaton
  • Devaka Cooray
Saloon Keepers:
  • Tim Moores
  • Stephan van Hulst
  • Ron McLeod
  • Piet Souris
  • Ganesh Patekar
Bartenders:
  • Tim Holloway
  • Carey Brown
  • salvin francis

Review on application responsible for fetching data from web txt

 
Ranch Hand
Posts: 103
1
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Hi guys. I would love to hear any opinions from your site about my application.
It is responsible for fetching data from National Polish Bank about buying rate and selling rate from days between given two dates. Next it calculates average buying rate from these days + standard deviation of selling rate.
For example we put 2018-01-28 and 2018-01-31 and it counts 28,29,30,31 of January.

I would like to hear:
1) if tests are properly written.
2) if NBPParserEngine is properly written (what does it mean? - if method names are good, if methods are not too long, if they are, how to shorten them)
3) if DataFetcher is properly written (same as 2) )

To sum up, Im not sure about methods names and if they have proper length.

I made pull request: https://github.com/must1/NBP-Parser/pull/1
If you want to "normal" version of GH: https://github.com/must1/NBP-Parser/tree/dev

I can paste also here some classes:

NBPParserEngine



DataFetcher



Here are tests: https://github.com/must1/NBP-Parser/tree/dev/src/test/java/pl/parser/nbp

Thanks a lot!
 
must Janik
Ranch Hand
Posts: 103
1
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
If needed to paste all classes here, just tell me and I will do it. It's important to me to hear something from "bigger" brains than me :P
 
Sheriff
Posts: 6173
163
Eclipse IDE Postgres Database VI Editor Chrome Java Ubuntu
  • Likes 1
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
I have some minor suggestions.

* You're method and variable names are long, but long is often good.  It's more descriptive.  Here's one place where I might shorten a method name:
Since this method returns a boolean, start the name with "is", maybe something like "isDayIncludedInCurrentYear()"

* I tend to put all my public-facing methods higher in the code and all the private methods lower.  This is so the API is more noticable.

* Sometimes if you have a very long then clause for an if statement...
I instead use a short statement with a return (this is up for debate, though)

* Don't use ALL_CAPS for variable names unless they are constants (static final).
 
must Janik
Ranch Hand
Posts: 103
1
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Thanks! It means a lot for me.

You're method and variable names are long, but long is often good.  It's more descriptive.  Here's one place where I might shorten a method name:


Done!

* I tend to put all my public-facing methods higher in the code and all the private methods lower.  This is so the API is more noticable.


Done!

* Sometimes if you have a very long then clause for an if statement...




yea, but in this situation I need to act when line is not null, if it is a null then nothing happens

* Don't use ALL_CAPS for variable names unless they are constants (static final).


I think I used it only when variables were constants.

If anybody has some other suggestions. Let me know!
 
Knute Snortum
Sheriff
Posts: 6173
163
Eclipse IDE Postgres Database VI Editor Chrome Java Ubuntu
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

must Janik wrote:Thanks! It means a lot for me.


You're welcome.

* Sometimes if you have a very long then clause for an if statement...




yea, but in this situation I need to act when line is not null, if it is a null then nothing happens


My suggestion was to put a test for null at the top of the method, then do all the other stuff if it gets past the test.  So to be clearer:


* Don't use ALL_CAPS for variable names unless they are constants (static final).


I think I used it only when variables were constants.


Constants are both static and final, and usually are member variables (not local).
 
must Janik
Ranch Hand
Posts: 103
1
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

My suggestion was to put a test for null at the top of the method, then do all the other stuff if it gets past the test.  So to be clearer:


Oh, I got it. Done!

Constants are both static and final, and usually are member variables (not local).



Ye, I know it, but the point is I think I used it in application properly. All constants are both static and final.

Greetings!
 
Too many men are afraid of being fools - Henry Ford. Foolish tiny ad:
professionally read, modify and write PDF files from Java
https://products.aspose.com/pdf/java
  • Post Reply Bookmark Topic Watch Topic
  • New Topic
Boost this thread!