patternjavaMinor
Brand profit and loss
Viewed 0 times
andlossbrandprofit
Problem
This code works well, but it's too long and looks too bad. I've tried to format it, but it all sets data, so I don't know how to make it better.
Here is the class:
```
private static BrandProfitAndLoss getAllRateComparisonRecords(BrandProfitAndLoss brandProfitAndLoss , BrandProfitAndLoss brandProfitAndLossComp){
BrandProfitAndLoss result = new BrandProfitAndLoss();
result.setDate(brandProfitAndLossComp.getDate());
result.setOrder_item_num_comp(compareData(brandProfitAndLoss.getOrder_item_num(), brandProfitAndLossComp.getOrder_item_num()));
result.setOrder_count_comp(compareData(brandProfitAndLoss.getOrder_count(), brandProfitAndLossComp.getOrder_count()));
result.setOrder_price_comp(compareData(brandProfitAndLoss.getOrder_price(), brandProfitAndLossComp.getOrder_price()));
result.setOrder_amount_comp(compareData(brandProfitAndLoss.getOrder_amount(), brandProfitAndLossComp.getOrder_amount()));
result.setOrder_gross_comp(compareData(brandProfitAndLoss.getOrder_gross(), brandProfitAndLossComp.getOrder_gross()));
result.setOrder_wholesale_price_comp(compareData(brandProfitAndLoss.getOrder_wholesale_price(), brandProfitAndLossComp.getOrder_wholesale_price()));
result.setDelivery_count_comp(compareData(brandProfitAndLoss.getDelivery_count(), brandProfitAndLossComp.getDelivery_count()));
result.setDelivery_price_comp(compareData(brandProfitAndLoss.getDelivery_price(), brandProfitAndLossComp.getDelivery_price()));
result.setDelivery_item_num_comp(compareData(brandProfitAndLoss.getDelivery_item_num(), brandProfitAndLossComp.getDelivery_item_num()));
result.setDelivery_gross_comp(compareData(brandProfitAndLoss.getDelivery_gross(), brandProfitAndLossComp.getDelivery_gross()));
result.setDelivery_amount_comp(compareData(brandProfitAndLoss.getDelivery_amount(), brandProfitAndLossComp.getDelivery_amount()));
result.setApproval_rate_comp(compareData(brandProfitAndLoss.getApproval_rate(), brandProfitAndLossComp.getApproval_r
Here is the class:
```
private static BrandProfitAndLoss getAllRateComparisonRecords(BrandProfitAndLoss brandProfitAndLoss , BrandProfitAndLoss brandProfitAndLossComp){
BrandProfitAndLoss result = new BrandProfitAndLoss();
result.setDate(brandProfitAndLossComp.getDate());
result.setOrder_item_num_comp(compareData(brandProfitAndLoss.getOrder_item_num(), brandProfitAndLossComp.getOrder_item_num()));
result.setOrder_count_comp(compareData(brandProfitAndLoss.getOrder_count(), brandProfitAndLossComp.getOrder_count()));
result.setOrder_price_comp(compareData(brandProfitAndLoss.getOrder_price(), brandProfitAndLossComp.getOrder_price()));
result.setOrder_amount_comp(compareData(brandProfitAndLoss.getOrder_amount(), brandProfitAndLossComp.getOrder_amount()));
result.setOrder_gross_comp(compareData(brandProfitAndLoss.getOrder_gross(), brandProfitAndLossComp.getOrder_gross()));
result.setOrder_wholesale_price_comp(compareData(brandProfitAndLoss.getOrder_wholesale_price(), brandProfitAndLossComp.getOrder_wholesale_price()));
result.setDelivery_count_comp(compareData(brandProfitAndLoss.getDelivery_count(), brandProfitAndLossComp.getDelivery_count()));
result.setDelivery_price_comp(compareData(brandProfitAndLoss.getDelivery_price(), brandProfitAndLossComp.getDelivery_price()));
result.setDelivery_item_num_comp(compareData(brandProfitAndLoss.getDelivery_item_num(), brandProfitAndLossComp.getDelivery_item_num()));
result.setDelivery_gross_comp(compareData(brandProfitAndLoss.getDelivery_gross(), brandProfitAndLossComp.getDelivery_gross()));
result.setDelivery_amount_comp(compareData(brandProfitAndLoss.getDelivery_amount(), brandProfitAndLossComp.getDelivery_amount()));
result.setApproval_rate_comp(compareData(brandProfitAndLoss.getApproval_rate(), brandProfitAndLossComp.getApproval_r
Solution
Can you change the
Since the scope of your
Make
Make your methods more consistently named, you're using some odd combination of camelCase and underscores that is hard to read and easy to mix up. Java is most often camelCased, but adherence to any standard is better than a mishmosh.
Rename your method: it does not appear that this class "gets all rate comparison records". It seems to generate some sort of single comparison record and return it.
Rename your other method:
Regarding
There's no sense in storing a value into a variable if you're only using it once (in the case of
I'm not entirely sure what you're trying to get accomplished with this method, so I can't suggest much. Is it just to show which of those numbers is greater? Can you use some kind of inline comparison in the setters above? Perhaps the classes (even, say, Float) support compareTo (from the Comparator interface), which would simplify this a lot.
Good luck!
BrandProfitAndLoss class? If so, consider something like the Builder pattern. This will have the additional benefit of likely allowing the instance you return to be immutable.Since the scope of your
brandProfitAndLoss and brandProfitAndLossComp variables is pretty small, consider a shorter variable name. It just bulks up each line, and you're unlikely to lose track of the variable since you can see its entire lifespan at one time.Make
result and your parameters final variables.Make your methods more consistently named, you're using some odd combination of camelCase and underscores that is hard to read and easy to mix up. Java is most often camelCased, but adherence to any standard is better than a mishmosh.
Rename your method: it does not appear that this class "gets all rate comparison records". It seems to generate some sort of single comparison record and return it.
Rename your other method:
compareData is undescriptive as to what the method does.Regarding
compareData:There's no sense in storing a value into a variable if you're only using it once (in the case of
result). Also, you should not be using floats to store data that refers to money, as it will NOT work the way you hope it will. There are appropriate classes in Java for handling money in predictable ways (e.g. BigDecimal), you should use those wherever possible. I'm not entirely sure what you're trying to get accomplished with this method, so I can't suggest much. Is it just to show which of those numbers is greater? Can you use some kind of inline comparison in the setters above? Perhaps the classes (even, say, Float) support compareTo (from the Comparator interface), which would simplify this a lot.
Good luck!
Context
StackExchange Code Review Q#28022, answer score: 2
Revisions (0)
No revisions yet.