compare two logic commented one to my logic java

Question

I'm trying to review this block of code to make sure logic is fine. The first version that I commented will create duplicated order 'my college developer' but I fixed it but "really not sure" I want to make sure my fix it exactly same as commented code

the old logic "this code include duplicated"

public void setOrderedProduct(String prodName, int eoq) {
    String productCd = getProdCd(prodName);
    // Here only one time one product
    boolean prodPresent = false;
    int indexOfProd = 0;
    int sizeOfItems = items.size();
    for (int i = 0; i < sizeOfItems; i++) {
        PurchaseOrderDTO itemVal = items.get(i);
        if (itemVal.getProdcd().equalsIgnoreCase(productCd)) {
            prodPresent = true;
            indexOfProd = i;
            break;
        }
    }
    itemData.setProdname(prodName);
    itemData.setQty(eoq);
    itemData.setProdcd(productCd);
    double costPrice = ProductView.getProductCost(vendorno, productCd);
    itemData.setPrice(costPrice);
    double extPrice = ProductView.getProductMSRP(vendorno, productCd);
    itemData.setExt(extPrice);

    if (!prodPresent) {
        if (eoq != 0) {
            items.add(itemData);
            // Now add the details to the database
            ProductView.addPurchaseDetails(itemData, vendorno);
        } 
    } else {
        if (eoq > 0) {
            items.set(indexOfProd, itemData);
            ProductView.updatePurchaseDetails(itemData, vendorno);
        } else {
            items.remove(indexOfProd);
            ProductView.removePurchaseDetails(itemData, vendorno);
        }
    }
    if (items.size() > 0)
        calculateTotal();
}

my fixed logic

public void setOrderedProduct(String prodName, int eoq) {         
    boolean dataPresent = false;
    if (orderedProdNameList.contains((Object) prodName)) {
        dataPresent = true;
    }
    if (dataPresent) {
        int indexOfProdName = orderedProdNameList.indexOf((Object) prodName);
        String productCd = getProdCd(prodName);
        // Here only one time one product
        PurchaseOrderDTO itemData = new PurchaseOrderDTO();

        boolean prodPresent = false;
        itemData.setProdname(prodName);
        itemData.setQty(eoq);
        itemData.setProdcd(productCd);
        double costPrice = ProductView.getProductCost(vendorno, productCd);
        itemData.setPrice(costPrice);
        double extPrice = ProductView.getProductMSRP(vendorno, productCd);
        itemData.setExt(extPrice);
        // Now add the details to the database
        ProductView.addPurchaseDetails(itemData, vendorno);
        if (eoq > 0) {
            items.set(indexOfProdName, itemData);
            ProductView.updatePurchaseDetails(itemData, vendorno);
        } else if (eoq == 0) {
            for (int i = 0; i < items.size(); i++) {
                if (items.get(i).getProdname().equalsIgnoreCase(prodName)) {
                    items.remove(i);
                    ProductView.removePurchaseDetails(itemData, vendorno);
                }
            }
        } 

    } else {
        orderedProdNameList.add(prodName);
        String productCd = getProdCd(prodName);
        // Here only one time one product
        PurchaseOrderDTO itemData = new PurchaseOrderDTO();

        boolean prodPresent = false;
        itemData.setProdname(prodName);
        itemData.setQty(eoq);
        itemData.setProdcd(productCd);
        double costPrice = ProductView.getProductCost(vendorno, productCd);
        itemData.setPrice(costPrice);
        double extPrice = ProductView.getProductMSRP(vendorno, productCd);
        itemData.setExt(extPrice);
        items.add(items.size(), itemData);
        ProductView.addPurchaseDetails(itemData, vendorno);
    }
    if (items.size() > 0) {
        calculateTotal();
    }
}

Answer 1

Not sure if your fixed logic actually works. But I see couple of issues with the code.

1. both if and else block is setting a boolean. But I don't find any use of it in your code. boolean prodPresent = false

2. How do you get access to items ? is it instance variable ?

3. The if and else block has lot of common code which can be written outside of the condition.

Here I am not concerned with your logic but tried to organize the second version of the code which you posted. My intention is to avoid repetitive code in both the if-else block

public void setOrderedProduct(String prodName, int eoq) {
PurchaseOrderDTO itemData = new PurchaseOrderDTO();
String productCd = getProdCd(prodName);

itemData.setProdname(prodName);
itemData.setQty(eoq);
itemData.setProdcd(productCd);

double costPrice = ProductView.getProductCost(vendorno, productCd);
itemData.setPrice(costPrice);

double extPrice = ProductView.getProductMSRP(vendorno, productCd);
itemData.setExt(extPrice);

if (orderedProdNameList.contains((Object) prodName)) {
  int indexOfProdName = orderedProdNameList.indexOf((Object) prodName);

  ProductView.addPurchaseDetails(itemData, vendorno);
  if (eoq > 0) {
    items.set(indexOfProdName, itemData);
    ProductView.updatePurchaseDetails(itemData, vendorno);
  } else if (eoq == 0) {
    for (int i = 0; i < items.size(); i++) {
      if (items.get(i).getProdname().equalsIgnoreCase(prodName)) {
        items.remove(i);
        ProductView.removePurchaseDetails(itemData, vendorno);
      }
    }
  }

} else {
  orderedProdNameList.add(prodName);
  items.add(items.size(), itemData);
  ProductView.addPurchaseDetails(itemData, vendorno);
}
 if (items.size() > 0) { calculateTotal(); }
}

Answer 2

Your method is very long. This is code smell - if method is long it maybe do a lot of thinks, but method should do one thing. I suggest you to split method into smaller peaces which defines smaller units of work and are reusable and easy to read. If you put piece of code into method you give name to the code which work as a hint to he programmer what code enclosed in method does.

Also use full name of variables, not some abbreviation like eoq. You write them once (if you are using ide) but read it multiple times so you invest the effor into the writing the proper name.

I did not checked your logic. Your logic should be captured in unit test - if there is no unit test i would hesitate to refactor some code. If there is no unit test dont write them now if you dont have business requirements, but you ca try Characterization testing to capture current behavior of the method. Because without any test you cannot be sure if you did not broke something when rewriting the method.