Skip to content
Advertisement

Refactor Java Program [closed]

Problem: I have list of houses in an Array. I need to make sure atleast one list consist of only houses with cat and atleast one list consist of only houses with dog. A list should only contain one kind of houses (i.e no mixed case). It is assumed that a house can either have a cat or a dog.

I tried writing the code, but it does not look good and would appreciate your help on refactoring or improving the design.

    Boolean isListWithHouseAndCatExist = false;
    Boolean isListWithHouseAndDogExist = false;
    List<House>[] arrOfHouseList = getArrOfHouseList();
    for(List<House> houseList : arrOfHouseList){
       Boolean isHousesWithCat = true;
       Boolean isHousesWithDog = true;
       for(House house: houseList){
          if(house.hasCat()){
             isHousesWithDog = false;
          }else{
             isHousesWithCat = false;
          }
       }
       if(!isHousesWithDog && ! isHousesWithCat){
          return false; //This is mixed case. The list contains both of kind of houses
       }
       isListWithHouseAndCatExist=isHousesWithCat?true:isListWithHouseAndCatExist;
       isListWithHouseAndDogExist=isHousesWithDog?true:isListWithHouseAndDogExist;  
    }

    // Now to check that we have atleast one list with all House-Cat and atleast one list with all
    // House-Dog
    if(!isListWithHouseAndCatExist || !isListWithHouseAndDogExist){
      return false;
   }
   return true;

As you can see I had to use four Boolean variable to validate conditions. Could you please help to improve the code.

Advertisement

Answer

If you don’t want to use streams and only want to iterate once, you could do something like this:

    Boolean isListWithHouseAndCatExist = false;
    Boolean isListWithHouseAndDogExist = false;
    List<House>[] arrOfHouseList = getArrOfHouseList();
    for(List<House> houseList : arrOfHouseList){

        Set<Boolean> hasCatFlags = new HashSet<>();
        for(House house: houseList){
            hasCatFlags.add(house.hasCat());
        }
        if(hasCatFlags.size() > 1){
            return false; //This is mixed case. The list contains both of kind of houses
        }
        if (hasCatFlags.contains(true)) {
            isListWithHouseAndCatExist = true;
        } else if (hasCatFlags.contains(false)) {
            isListWithHouseAndDogExist = true;
        }
    }

    return isListWithHouseAndCatExist && isListWithHouseAndDogExist;

If you can use streams but want to iterate only once, you could do something like this:

    Boolean isListWithHouseAndCatExist = false;
    Boolean isListWithHouseAndDogExist = false;
    List<House>[] arrOfHouseList = getArrOfHouseList();
    for(List<House> houseList : arrOfHouseList){

        Set<Boolean> hasCatFlags = houseList.stream().map(House::hasCat).collect(Collectors.toSet());
        if(hasCatFlags.size() > 1){
            return false; //This is mixed case. The list contains both of kind of houses
        }

        if (hasCatFlags.contains(true)) {
            isListWithHouseAndCatExist = true;
        } else if (hasCatFlags.contains(false)) {
            isListWithHouseAndDogExist = true;
        }
    }

    return isListWithHouseAndCatExist && isListWithHouseAndDogExist;

And if you can use streams and don’t mind iterating twice, you could do this:

    Boolean isListWithHouseAndCatExist = false;
    Boolean isListWithHouseAndDogExist = false;
    for(List<House> houseList : getArrOfHouseList()){
        if (houseList.stream().allMatch(House::hasCat)) {
            isListWithHouseAndCatExist = true;
        } else if (houseList.stream().noneMatch(House::hasCat)) {
            isListWithHouseAndDogExist = true;
        } else {
            return false;//mixed case
        } 
    }

    return isListWithHouseAndCatExist && isListWithHouseAndDogExist;

User contributions licensed under: CC BY-SA
10 People found this is helpful
Advertisement