-4
\$\begingroup\$

I'm trying to figure out some way to improve the code below. My main problem is the method getPrice.

Some advices:

  • In the else clause, I need to query the information in the database (using EJB injection).
  • The ProductPrice and MarketPrice are very different JPA entities. So, I can't refactor them to use inheritance/composition (per example).

The code is weird, but I can't figure out a way to really improve.

import java.util.*;
import java.math.*;
import java.lang.*;
import java.io.*;

class Ideone
{
    public static void main (String[] args) throws java.lang.Exception
    {
        PriceService priceService = new PriceService();
        ProductPrice productPrice = new ProductPrice();
        ProductMarketPrice productMarketPrice = priceService.getPrice(productPrice, 1L);
        System.out.println(productMarketPrice.getAdvice());

        productMarketPrice = priceService.getPrice(null, 1L);
        System.out.println(productMarketPrice.getAdvice());
    }
}

class PriceService {

    //@EJB
    MarketPriceService marketPriceService = new MarketPriceService();

    public ProductMarketPrice getPrice(ProductPrice productPrice, Long productId) {
        BigDecimal price = BigDecimal.ZERO;
        Date date = null;
        boolean isFromProduct = false;
        if (productPrice != null) {
            price = productPrice.getPrice();
            date = productPrice.getRegisterDate();
            isFromProduct = true; // i need to know if the values are from 'one' or 'x'
        } else {
            MarketPrice marketPrice = this.marketPriceService.findTheMarketPriceForProduct(productId);
            price = marketPrice.getPrice();
            date = marketPrice.getDate();
        }

        return new ProductMarketPrice(price, date, isFromProduct);
    }

}

class ProductPrice {
    public BigDecimal getPrice() {
        return new BigDecimal("1.00");
    }

    public Date getRegisterDate() {
        return new Date();
    }
}


class ProductMarketPrice {
    private BigDecimal price;
    private Date date;
    private boolean isFromProduct;
    public ProductMarketPrice(BigDecimal price, Date date, boolean isFromProduct){
        this.price = price;
        this.date = date;
        this.isFromProduct = isFromProduct;
    }
    public String getAdvice() {
        String answer = this.isFromProduct ? "yes" : "no";
        return "Hi, user! This price and date is from our Product? " + answer + ". So, be sure about that when you do your stuff.";
    }
}

class MarketPriceService {
    public MarketPrice findTheMarketPriceForProduct(Long productId) {
        return new MarketPrice();
    }
}

class MarketPrice {
    public BigDecimal getPrice() {
        return new BigDecimal("2.00");
    }

    public Date getDate() {
        return new Date();
    }
}

You can find the working code here.

Thanks!

\$\endgroup\$

closed as off-topic by Jamal Jan 4 '17 at 20:40

This question appears to be off-topic. The users who voted to close gave this specific reason:

If this question can be reworded to fit the rules in the help center, please edit the question.

  • 1
    \$\begingroup\$ @Jamal, I created a working code now and edited the question. \$\endgroup\$ – Dherik Jan 4 '17 at 22:03
  • 2
    \$\begingroup\$ @Mast The border is narrow (very narrow, just like std::epsilon). In 1st place I've learned SE Code Review requires working code to review, so working code should be shown. Just claiming that it's working in a larger context isn't really useful, regarding the unknown parts of it. I know that this restricts SE Code Review in some way, and may be that's one of the culprits for too low traffic and underestimation of this site. \$\endgroup\$ – πάντα ῥεῖ Jan 4 '17 at 22:21
  • 2
    \$\begingroup\$ This still contains hypothetical code (SomeClass, getSomething(), ...), so it's still off-topic. If you cannot show the true code, then it just doesn't fit with this site. \$\endgroup\$ – Jamal Jan 5 '17 at 4:07
  • 3
    \$\begingroup\$ I have rolled back the last edit made by another contributor. Please don't edit the code in the original post if your are not the original author. \$\endgroup\$ – Phrancis Jan 5 '17 at 7:26
  • \$\begingroup\$ @Jamal, I will edit (again) to change the names for something more meaniful. But If you cannot show the true code, then it just doesn't fit with this site it's a (another) new information for me. \$\endgroup\$ – Dherik Jan 5 '17 at 13:10
2
\$\begingroup\$
   isOne = true; // i need to know if the values are from 'one' or 'x'

The question is: why do you need to know that later?

My guess is because you later make a decision on that boolean value.

My suggestion is to create a ResultOne class and a ResultX class where both extend the Result class.

The class Result gets an additional (abstract) method which is implemented differently in ResultOne and ResultX.


from the comments:

Hi! The final user needs to know that the value is from EntityX, so this information is used to show as a advice to him.

abstract class Result {
  // property and getter for  value and date

  abstract public String getResultTypeMessage();
}

class ResultOne extends Result{
   public String getResultTypeMessage(){
     return "this is result One";
    }
}

class ResultX extends Result{
   public String getResultTypeMessage(){
     return "this is result X";
    }
}



private Result getSomething(EntityOne entityOne, Long someId) {

    Result result;
    if (entityOne != null) {
       result = new ResultOne(entityOne.getOneValue(),entityOne.getOneDate());
    } else {
        EntityX entityX = this.someEjb.findInDatabase(someId);
       result = new ResultX(entityX.getOneValue(),entityX.getOneDate());
    }

    return result;
}

in your UI:

 System.out.println("Type:"+ result.getResultTypeMessage());
 System.out.println("Value:"+ result.getValue());
 System.out.println("Date:"+ result.getDate());
\$\endgroup\$
  • \$\begingroup\$ Hi! The final user needs to know that the value is from EntityX, so this information is used to show as a advice to him. \$\endgroup\$ – Dherik Jan 4 '17 at 21:28
  • \$\begingroup\$ @Dherik Don't bother users of your API that way. Abstracting such to a common base class seems to be a way you can go about. \$\endgroup\$ – πάντα ῥεῖ Jan 4 '17 at 21:35
  • \$\begingroup\$ @Timothy s/MyResult /Result in your example please. Omit [edit] also. The history of your answer is already inspectable. \$\endgroup\$ – πάντα ῥεῖ Jan 4 '17 at 21:49
  • \$\begingroup\$ Hi, thanks for the help. It's better now, but the if/else still bother me... I shared a working code now too. \$\endgroup\$ – Dherik Jan 4 '17 at 22:02
  • \$\begingroup\$ @Dherik "but the if/else still bother me..." you cannot avoid making a decision when creating objects. That's life... ;o) \$\endgroup\$ – Timothy Truckle Jan 5 '17 at 7:38

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