ivannp / tradelib

A Java framework for backtesting and trading
Apache License 2.0
56 stars 29 forks source link

Consider using Lombok #3

Open benmccann opened 7 years ago

benmccann commented 7 years ago

I noticed you have all public variables in one of the first classes I opened: https://github.com/ivannp/tradelib/blob/master/src/main/java/net/tradelib/core/TradeSummary.java#L17

This is pretty uncommon for Java. Typically you'd make these private and use getters and setters. The reason for this which is that if you want to change the logic (e.g. add validation in a setter or make a getter return a computed value) is that it doesn't require much refactoring if you're using getters and setters.

Of course writing and maintaining these getters and setters is an ugly pain, but there's a great solution for this with Lombok: https://projectlombok.org/features/Data.html

ivannp commented 7 years ago

It was intentional. I spent some time looking into the reasoning when I coded it. If the only argument is "common practices", I'd rather leave it that way.

benmccann commented 7 years ago

It's a bit more than common practices, or I should say that it developed as a common practice for a reason. E.g. I worked on Google Spreadsheets and there was a variable value to get the value from a cell. After a few years we had a need to return a computed value in some cases. We had to go though the codebase and change every reference to value to getValue(). This change took weeks because it was a very common piece of code used almost everywhere. If we had encapsulated this behind a getter from the beginning this would have been a much easier change to make.