HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavaMinor

Brand profit and loss

Submitted by: @import:stackexchange-codereview··
0
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

Solution

Can you change the 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.