private /*static*/ ResultDisplay createDisplay(
Optional<BigDecimal> rateOptional,
BigDecimal amount) {
if (rateOptional.isPresent()) {
CompoundInterestCalculator compoundInterestCalculator =
new CompoundInterestCalculator(
amount,
rateOptional.get());
return new LowestRateDisplay(
amount,
rateOptional.get(),
compoundInterestCalculator.getMonthlyRepayment(),
compoundInterestCalculator.getTotalRepayment());
} else {
return ResultDisplay.NO_RESULT;
}
}
// main
ResultDisplay resultDisplay = createDisplay(
rateOptional,
amount);
resultDisplay.display();
Usually classes using the same constants also tend to implement the same interface which would be a much better place for such constants. And values that are runtime constants but may change on proram start should be passed around as constructor parameters.
Regarding the static imports, what would you use instead of, let's say, the import of
InputValidator.validateArguments()? Would you make the methods non-static and then just use it asnew InputValidator().validateArguments(args)? – antonro
Yes.
I'd even go one step further and pass those instances as Constructor parameters to the RateCalculatorApp instancs:
public static void main(
String[] args
) {
RateCalculatorApp rateCalculator =
new RateCalculatorApp(
new InputValidator(),
new CsvReader());
rateCalculator.process(
args);
}
private void process(
String[] args
) {
inputValidator.validateArguments(
args);
Collection<LenderOffer> lenderOffers =
csvReader.csvToLenderInfoList(
args[0]);
//...
}
This opens an easy way to replace this rather expensive dependencies with test doubles (e.g. mocks created with a mocking framework) for unit testing.
private /*static*/ ResultDisplay createDisplay(
Optional<BigDecimal> rateOptional,
BigDecimal amount) {
if (rateOptional.isPresent()) {
CompoundInterestCalculator compoundInterestCalculator =
new CompoundInterestCalculator(
amount,
rateOptional.get());
return new LowestRateDisplay(
amount,
rateOptional.get(),
compoundInterestCalculator.getMonthlyRepayment(),
compoundInterestCalculator.getTotalRepayment());
} else {
return ResultDisplay.NO_RESULT;
}
}
// main
ResultDisplay resultDisplay = createDisplay(
rateOptional,
amount);
resultDisplay.display();
Usually classes using the same constants also tend to implement the same interface which would be a much better place for such constants. And values that are runtime constants but may change on proram start should be passed around as constructor parameters.
private /*static*/ ResultDisplay createDisplay(
Optional<BigDecimal> rateOptional,
BigDecimal amount) {
if (rateOptional.isPresent()) {
CompoundInterestCalculator compoundInterestCalculator =
new CompoundInterestCalculator(
amount,
rateOptional.get());
return new LowestRateDisplay(
amount,
rateOptional.get(),
compoundInterestCalculator.getMonthlyRepayment(),
compoundInterestCalculator.getTotalRepayment());
} else {
return ResultDisplay.NO_RESULT;
}
}
// main
ResultDisplay resultDisplay = createDisplay(
rateOptional,
amount);
resultDisplay.display();
Usually classes using the same constants also tend to implement the same interface which would be a much better place for such constants. And values that are runtime constants but may change on proram start should be passed around as constructor parameters.
Regarding the static imports, what would you use instead of, let's say, the import of
InputValidator.validateArguments()? Would you make the methods non-static and then just use it asnew InputValidator().validateArguments(args)? – antonro
Yes.
I'd even go one step further and pass those instances as Constructor parameters to the RateCalculatorApp instancs:
public static void main(
String[] args
) {
RateCalculatorApp rateCalculator =
new RateCalculatorApp(
new InputValidator(),
new CsvReader());
rateCalculator.process(
args);
}
private void process(
String[] args
) {
inputValidator.validateArguments(
args);
Collection<LenderOffer> lenderOffers =
csvReader.csvToLenderInfoList(
args[0]);
//...
}
This opens an easy way to replace this rather expensive dependencies with test doubles (e.g. mocks created with a mocking framework) for unit testing.
commenting the review
RateFinder is returning the rate, then the calculator is doing other calculations, but they should be together.
If you look at the loop in MultipleLendersRateFinder.java 3 of the 5 lines access properties of LenderOffer. This is a clear sign that these 3 lines should be in LenderOffer.
This "Responsibility Distribution" also occurs with the InputValidator and the CsvReader. IMHO the check for the existence of the file belongs to the CsvReader rather then to the InputValidator. After all javas own FileReader thows a FileNotFoundException too...
An another place is your main method where you do a single System.out.println() although you (could) have you ResultDisplay handling that case too.
RateFinder should return a set of lenders or something similar
Just guessing, but I think they wanted a list of lenders alomg with the ammount to lend and a summery line with the overall result.
overuse of static import, so much that it becomes a code smell
I strongly support that.
I use static key word only if I have a good reason and the ability to call something from main does not count here.
In you code the static import addiction tricked me to think validateArguments(args); was a method in RateCalculatorApp.
other findings
odd ball solution
All over your code you are using Java8 streams but in MultipleLendersRateFinder.java you fall back to loop with index (although foreach loop might have been less "old fashioned"...).
unnessessary state
Your ResultDisplay has only a single method but you enforce state by passing the values to display as constructor parameters. At least it is immutable which is the good part about it.
On the other hand I can imagin an approach where that might have been usefull:
ResultDisplay could have been an interface with one Implementation as you actually did and another NoResultDisplay that only outputs the requested prase as anonymous class instance or even a lambda assigned to a constant in that interface. Then your main could look like this:
ResulpDisplay resultDisplay = ResulpDisplay.NO_RESULT;
if (rateOptional.isPresent()) {
CompoundInterestCalculator compoundInterestCalculator =
new CompoundInterestCalculator(
amount,
rateOptional.get());
resultDisplay =
new LowestRateDisplay(
amount,
rateOptional.get(),
compoundInterestCalculator.getMonthlyRepayment(),
compoundInterestCalculator.getTotalRepayment());
}
resultDisplay.display();
and according to the same level of abstraction pattern I'd even move that to a separate method:
private /*static*/ ResultDisplay createDisplay(
Optional<BigDecimal> rateOptional,
BigDecimal amount) {
if (rateOptional.isPresent()) {
CompoundInterestCalculator compoundInterestCalculator =
new CompoundInterestCalculator(
amount,
rateOptional.get());
return new LowestRateDisplay(
amount,
rateOptional.get(),
compoundInterestCalculator.getMonthlyRepayment(),
compoundInterestCalculator.getTotalRepayment());
} else {
return ResultDisplay.NO_RESULT;
}
}
// main
ResultDisplay resultDisplay = createDisplay(
rateOptional,
amount);
resultDisplay.display();
naming
Choose your names from the problem domain (only). E.g. you named a variable rateOptional but what value does the technical aspect that thsi variable holds an Optional ad to someone hwho wants ti understand your algorithm?
None...
So get rid of that implementation related suffixes like *App, *Finder or *Calculator.
Constant container class
You created a Constant container class to provide system with constants. But there is only one constant you use in more then one class. And even this is quiestionable if that should be a compile time constant of it that might rather be something that we want to configure at program start.
Also you created another odd ball solution by having the static import to the Constant container class and a class constant in the same file.
So don't do this.
Usually classes using the same constants also tend to implement the same interface which would be a much better place for such constants. And values that are runtime constants but may change on proram start should be passed around as constructor parameters.