Tell me more ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 1 down vote favorite

I Have the code that work well but it too long and look too bad. I try to format it, but it all set data, so I don't know how to make it better

Here is the code:

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_rate()));
        result.setShipping_comp(compareData(brandProfitAndLoss.getShipping(), brandProfitAndLossComp.getShipping()));
        result.setAll_delivery_price_comp(compareData(brandProfitAndLoss.getAll_delivery_price_comp(), brandProfitAndLossComp.getAll_delivery_price_comp()));
        result.setReturn_sales_comp(compareData(brandProfitAndLoss.getReturn_sales(), brandProfitAndLossComp.getReturn_sales()));
        result.setPoint_discount_comp(compareData(brandProfitAndLoss.getPoint_discount(), brandProfitAndLossComp.getPoint_discount()));
        result.setPoint_allowance_comp(compareData(brandProfitAndLoss.getPoint_allowance(), brandProfitAndLossComp.getPoint_allowance()));
        result.setSales_amount_comp(compareData(brandProfitAndLoss.getSales_amount(), brandProfitAndLossComp.getSales_amount()));
        result.setPurchasing_cost_comp(compareData(brandProfitAndLoss.getPurchasing_cost(), brandProfitAndLossComp.getPurchasing_cost()));
        result.setProfit_comp(compareData(brandProfitAndLoss.getProfit(), brandProfitAndLossComp.getProfit()));
        result.setProfit_gross_comp(compareData(brandProfitAndLoss.getProfit_gross(), brandProfitAndLossComp.getProfit_gross()));
        result.setYfc_cost_comp(compareData(brandProfitAndLoss.getYfc_cost(), brandProfitAndLossComp.getYfc_cost()));
        result.setCommission_cost_comp(compareData(brandProfitAndLoss.getCommission_cost(), brandProfitAndLossComp.getCommission_cost()));
        result.setServer_cost_comp(compareData(brandProfitAndLoss.getServer_cost(), brandProfitAndLossComp.getServer_cost()));
        result.setBrand_cost_comp(compareData(brandProfitAndLoss.getBrand_cost(), brandProfitAndLossComp.getBrand_cost()));
        result.setBrand_pol_comp(compareData(brandProfitAndLoss.getBrand_pol(), brandProfitAndLossComp.getBrand_pol()));
        result.setCompLastYearRate_comp(compareData(brandProfitAndLoss.getCompLastYearRate(), brandProfitAndLossComp.getCompLastYearRate()));
        return result;
    }

And here is the compareDate method

private static float compareData(float thisYear, float lastYear) {
        float result = 0;
        if(thisYear == 0 || lastYear == 0){
            return result;
        }

        return (thisYear / lastYear) - 1;
    }

Thank for any help

share|improve this question

1 Answer

up vote 1 down vote

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!

share|improve this answer
1  
+1 just for "shorter variable names". Using e.g. b1 and b2 instead of brandProfitAndLoss and brandProfitAndLossComp will make each line 36 characters shorter and much more readable. – tobias_k Jul 2 at 8:18
@tobias_k thanks for nice hint – user25815 Jul 3 at 1:27
@James McMillan thank for your help, it's very necessary for me – user25815 Jul 3 at 1:28
-1 for shorter variable names - the problem with this method is already readability, you're trying to compensate for an unclear design by taking unnecessary shortcuts. But +1 for the comment on camelCase - it's not standard Java convention to use underscores in method names. – Trisha yesterday
@Trisha Partially agreed. The names of the method parameters should be kept long and descriptive, but in the method body, shorter names pointing to the same objects could be used. – tobias_k 8 hours ago

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.