I am new to Java, I am bored with if else statement I want to refactor my code and relief from if-else statement, not I am not able to reduce my code, In my code nothing is static every thing comes from user side. I share my codes for your reference.

if (orderDetail != null && item != null && !orderDetail.isUnitPriceModified()) {
  ItemSizeColorStyle itemSizeColorStyle = itemSizeColorStyleRepository.findByRecordID(orderDetail.getItemSCSID());

  if (origin != null && (origin.contains("b2baccess") || (origin.contains(".myappdev") && !origin.contains("apps.myappdev")))) {
    RepGroupManufacturer repGroupManufacturer = repGroupManufacturerRepository.findByRepGroupIDAndManufacturerIDAndRecordDeleted(repGroupID, manufacturerID, NOT_DELETED);
    if (repGroupManufacturer != null && repGroupManufacturer.getB2bItemPricingPolicy() != null && repGroupManufacturer.getB2bItemPricingPolicy().equalsIgnoreCase(ReptimeConstants.SHOWRETAILPRICE)) {
      if (orderDetail.getItemSCSID() == null && item.getRetailPrice() != null) {
        orderDetail.setUnitPrice(item.getRetailPrice());
      } else {
        //                        ItemSizeColorStyle itemSizeColorStyle = itemSizeColorStyleRepository.findByRecordID(orderDetail.getItemSCSID());
        if (itemSizeColorStyle != null && itemSizeColorStyle.getRetailPrice() != null) {
          orderDetail.setUnitPrice(itemSizeColorStyle.getRetailPrice());
        } else if (itemSizeColorStyle != null && itemSizeColorStyle.getRetailPrice() == null && item.getRetailPrice() != null) {
          orderDetail.setUnitPrice(item.getRetailPrice());
        } else if (itemSizeColorStyle != null && itemSizeColorStyle.getRetailPrice() == null && item.getRetailPrice() == null) {
          throw new NullPointerException("item price can not be null.");
        }
      }
    }
  }

How to convert this if else to map.

CodePudding user response：

Since i dont know what types you are returning and what type of objects some variables are i had to improvise but this should still work you just need to change to the correct values

public static void main(String[] args) {
    setUnitPrice(orderDetail,origin);
}
public static void setUnitPrice(OrderDetail orderDetail,Origin origin){
    if (orderDetail == null && item == null && orderDetail.isUnitPriceModified()) return;
    if (origin == null && !origin.contains("b2baccess")  ||
            !origin.contains(".myappdev") && origin.contains("apps.myappdev")) return;
   
    RepGroupManufacturer repGroupManufacturer = repGroupManufacturerRepository
            .findByRepGroupIDAndManufacturerIDAndRecordDeleted(repGroupID, manufacturerID, NOT_DELETED);
    
    if (repGroupManufacturer == null &&
            repGroupManufacturer.getB2bItemPricingPolicy() == null &&
            !repGroupManufacturer.getB2bItemPricingPolicy().equalsIgnoreCase(ReptimeConstants.SHOWRETAILPRICE)) return;
    
    ItemSizeColorStyle itemSizeColorStyle = itemSizeColorStyleRepository.findByRecordID(orderDetail.getItemSCSID());
    orderDetail.setUnitPrice(getUnitPrice(itemSizeColorStyle,item,orderDetail));
}
public static int getUnitPrice(ItemSizeColorStyle itemSizeColorStyle, Item item, OrderDetail orderDetail) {
    if (orderDetail.getItemSCSID() == null && item.getRetailPrice() != null) return item.getRetailPrice();
    if (itemSizeColorStyle != null && itemSizeColorStyle.getRetailPrice() != null) return itemSizeColorStyle.getRetailPrice();
    if (itemSizeColorStyle != null && itemSizeColorStyle.getRetailPrice() == null && item.getRetailPrice() != null) return item.getRetailPrice();
    throw new NullPointerException("item price can not be null.");
}

CodePudding user response：

At-least break into multiple small methods to start with. This will make if-else less scary, easy to understand.
Also there are areas / paths to avoid duplicate checks.

Maybe you could break responsibility of validation checks into different classes as well.

import java.util.Optional;

import static java.util.Objects.isNull;
import static java.util.Objects.nonNull;

class Scratch {
    private static String NOT_DELETED = "not-deleted";
    private ItemSizeColorStyleRepository itemSizeColorStyleRepository;
    private RepGroupManufacturerRepository repGroupManufacturerRepository;


    public void setUnitPrice(OrderDetail orderDetail, Item item, String origin, Integer repGroupID, Integer manufacturerID) {
        if (isValidOrderDetail(orderDetail) && nonNull(item)) {
            if (isValidOrigin(origin)) {
                if (isValidRepGroupManufacturer(repGroupID, manufacturerID)) {
                    if (isNull(orderDetail.getItemSCSID()) && nonNull(item.getRetailPrice())) {
                        orderDetail.setUnitPrice(item.getRetailPrice());
                    } else {
                        ItemSizeColorStyle itemSizeColorStyle = itemSizeColorStyleRepository.findByRecordID(orderDetail.getItemSCSID());
                        if (nonNull(itemSizeColorStyle)) {
                            if (nonNull(itemSizeColorStyle.getRetailPrice())) {
                                orderDetail.setUnitPrice(itemSizeColorStyle.getRetailPrice());
                            } else {
                                if (nonNull(item.getRetailPrice())) {
                                    orderDetail.setUnitPrice(item.getRetailPrice());
                                } else {
                                    throw new NullPointerException("item price can not be null.");
                                }
                            }
                        }
                    }
                }
            }
        }
    }

    private boolean isValidOrderDetail(OrderDetail orderDetail) {
        return Optional
                .ofNullable(orderDetail)
                .map(e -> !e.isUnitPriceModified())
                .orElse(false);
    }

    private boolean isValidOrigin(String origin) {
        return nonNull(origin) &&
                (origin.contains("b2baccess") || (origin.contains(".myappdev") && !origin.contains("apps.myappdev")));
    }

    private boolean isValidRepGroupManufacturer(Integer repGroupID, Integer manufacturerID) {
        RepGroupManufacturer repGroupManufacturer = repGroupManufacturerRepository
                .findByRepGroupIDAndManufacturerIDAndRecordDeleted(repGroupID, manufacturerID, NOT_DELETED);
        return Optional
                .ofNullable(repGroupManufacturer)
                .flatMap(e -> Optional.ofNullable(e.getB2bItemPricingPolicy()))
                .map(e -> e.equalsIgnoreCase(ReptimeConstants.SHOWRETAILPRICE))
                .orElse(false);
    }
}

Note: I have assumed certain datatypes, which I don't have any impact on overall solution.

CodePudding user response：

Some if-else statements express business logic which should be anything but boring. The code you posted appears to contain at least some business logic but the manner in which it is written makes it difficult to read and maintain.

There are some rules I follow when refactoring this type of code:

1. "positive thinking" - avoid if not statements, that is what else is for.
2. replace nested if-statements with functions.
3. be as lazy as possible, only lookup values when needed.
4. use variable names that have a meaning.

On 1, replace this

if (x is not null) {
  // more code
}

with

if (x is null) {
  return;
}
// more code

On 2, replace this

if (x > 1 ) {
  // lots of code
}

with

if (x > 1) {
  updateStuff(x);
}

function updateStuff(int x) {
  // lots of code
}

This also opens up the opportunity to let the function return a value that can be used further on.

On 3, replace this

x = queryDatabase(y);
if (y > 1) {
  // use x
}

with

if (y > 1) {
  x = queryDatabase(y);  
  // use x
}

Finally, on 4, I sometimes introduce boolean values to convey meaning to complex conditions. E.g. compare this:

if ((x < 1 and y > 100) ||  (z not 0 and y < 1)) {
  // do stuff
}

with

boolean findPrice = (x < 1 and y > 100);
boolean findProduct = (z not 0 and y < 1);
if (findPrice || findProduct) {
  // do stuff
}

When all these rules are applied it becomes much easier to read the code and also easier to maintain and (unit) test the code.

CodePudding user response：

Disclaimer: it merely impossible to reimplement a chunk of code containing so much logic and guaranty that you can simply copy-past it, and it would work just fine.

The very first advice I can give to change the return type in your Repositories and leverage the Optional API (that alone wouldn't resolve all the issues, but it would give some room for improvement).

Secondly, you need to break presented logic into self-contained meaningful peaces and give them names. The current code violates the first principle of SOLID - Single responsibility principle in a very obvious way. How can you test it, or how can change this functionality without rewriting from scratch? Such code is very hard to maintain.

Here's my attempt to reimplement this logic, assuming that both Repositories would produce an Optional as a result:

public void foo(OrderDetail orderDetail, Item item) {
    if (!isValid(orderDetail, item, origin)) return;

    repGroupManufacturerRepository
        .findByRepGroupIDAndManufacturerIDAndRecordDeleted(repGroupID, manufacturerID, NOT_DELETED);
        .filter(manufacturer -> manufacturer.getB2bItemPricingPolicy() != null)
        .filter(manufacturer -> manufacturer.getB2bItemPricingPolicy().equalsIgnoreCase(ReptimeConstants.SHOWRETAILPRICE))
        .ifPresent(manufacturer -> itemSizeColorStyleRepository.findByRecordID(orderDetail.getItemSCSID())
            .filter(itemStyle -> itemSizeColorStyle.getRetailPrice() != null)
            .ifPresent(itemStyle -> {
                if (needToApplyItemPrice(orderDetail, item, itemStyle)) orderDetail.setUnitPrice(item.getRetailPrice());
                else if (needToApplyItemStylePrice(orderDetail, item, itemStyle)) orderDetail.setUnitPrice(item.getRetailPrice());
                else throw new NullPointerException("item price can not be null.");
            })
        );
}

public boolean isValid(OrderDetail orderDetail, Item item, Origin origin) {
    return orderDetail != null && item != null && !orderDetail.isUnitPriceModified()
        && origin != null
        && (origin.contains("b2baccess") || origin.contains(".myappdev") && !origin.contains("apps.myappdev"));
}

public boolean needToApplyItemPrice(OrderDetail orderDetail, Item item, ItemSizeColorStyle itemStyle) {
    return (orderDetail.getItemSCSID() == null || itemStyle.getRetailPrice() == null)
        && item.getRetailPrice() != null;
}

public boolean needToApplyItemStylePrice(OrderDetail orderDetail, Item item, ItemSizeColorStyle itemStyle) {
    return !(orderDetail.getItemSCSID() == null && item.getRetailPrice() != null)
        && itemStyle.getRetailPrice() != null;
}