Refactoring kodu

0

Witam,

Czy można jakoś poprawić ten kod?
Czy jest jakaś możliwość aby nie pisać tej ifologii?

private List<String> getStatus(Object obj, List<String> list) {
            List<String> tempList = new ArrayList<>();
            if (isStatus_1(obj)) {
                  tempList.add(list.get(0));
                  tempList.add("91");
                  return tempList;
            }
            if (isStatus_2(obj)) {

                  tempList.add(list.get(1));
                  tempList.add("92");
                  return tempList;
            }
            if (isStatus_3(obj)) {

                  tempList.add(list.get(2));
                  tempList.add("93");
                  return tempList;
            }
            if (isStatus_4(obj)) {

                  tempList.add(list.get(3));
                  tempList.add("94");
                  return tempList;
            }
            return getSuperStatus(obj);
      }
1

Można i to na wiele sposobów. Zacząłbym np. od tego że zamiast zabaw z temp list mogłeś zrobić return Arrays.asList(list.get(0), "91"); i juz trochę krócej.
Ale tak generalnie to zrobiłbym sobie jakąś listę możliwości i po niej zwyczajnie iterował. Dodanie kolejnego "ifa" to wtedy dodanie kolejnego elementu do listy. Więc np.

class Cośtam{
    private final Predicate<Object> statusPredicate;
    private final int listIndex;
    private final String value;
}

I teraz kod tej metody to:

private List<String> getStatus(Object obj, List<String> list) {
    List<Cośtam> options = getOptions();
    for(Cośtam c : options){
        if(c.getPredicate().apply(obj)){
            return Arrays.asList(list.get(c.getIndex()), c.getValue());
        }
    }        
    return getSuperStatus(obj);
}
0

1.Mozesz użyć switch-case
2.To co masz w środku if-ow zamień na funkcje, która będzie wywoływana z odpowiednimi parametrami (list.get(0),"91")

1

Tak można. Po pierwsze to nawet daleko się nie wgłębiając można powtarzający się kod wyciągnąć do metody:

private List<String> extract( List input,  int index, String param)
  List<String> tempList = new ArrayList<>();
                  tempList.add(input.get(input));
                  tempList.add(param);
                  return tempList;

Ale żeby to ładnie zrobić to najpierw pokaż co masz w isStatus_X()
A ogólnie to możesz użyc Map< Predicate, Function<List,List>> - (ale logicznie to tylko przesunięcie ifologii o jeden poziom ) - pytanie na ile można isStatus zautomatyzować....

1

1500 sposobów.
Najłatwiej niech zrób metodę isStatus która Ci zwróci wartość od 0 do 3 i zapisz coś takiego:

 int status = getStatus(obj));
 tempList.add(list.get(status));
 tempList.add(String.valueOf(91+status));
 return tempList;
           

ale tak naprawdę ta metoda jest kretyńska z założenia, lepiej opisz co chcesz zrobić pod jakimi warunkami i wtedy będzie łatwiej ci pomóc

0

Dzięki wszystkim za pomoc,

@jarekr000000 niestety ale tych isStatus nie da sie za bardzo zautomatyzowac kazdy sprawdza calkowicie cos innego.

Jeszcze raz dzięki.

0

Statusy:


      private boolean isStatus_1(Object obj) {
            return obj.getStatus().equals(Status.DECLARED);
      }

      
      private boolean isStatus_2(Object obj) {
            Status status = obj.getStatus();
            return status.equals(Status.CONNECTED) || status.equals(Status.CONFIRMED);
      }

     
      private boolean isStatus_3(Object obj) {
            boolean isRejected = obj.getStatus().equals(Status.REJECTED);
            boolean hasLimit = obj.getStatus().equals(Status.LIMIT_EXCEED);
            boolean hasLimit_3M = obj.getStatus().equals(Status.LIMIT_EXCEED_3M);
            boolean isBlocked = (obj.getBlocked() != null ? obj.getBlocked() : false);
            return isRejected || isBlocked || hasLimit || hasLimit_3M;
      }

      
      private boolean isStatus_4(Object obj) {
            boolean hasPackageName = obj.getPackageName() != null;
            
            boolean isInProgres = obj.getStatus().equals(Status.IN_PROGRESS);

            boolean isExecuted = obj.getStatus().equals(Status.EXECUTED);
            
            return hasPackageName || isInProgres || isExecuted;
      }
0

Tak tutaj wklejasz ten Object czy faktycznie nazwałeś tak klasę ?

w każdym razie pokaż skąd masz Object i jak wygląda ta klasa. Możesz zrobić sobie wewnątrze metodę int getStatus() która będzie tylko jakoś switchować po warunkach.

A najlepiej stworzyć Fabrykę Object która w zależności od warunków będzie tworzyła Strategię postepowanią w metodzie getStatus() tej Twojej

2

Widze 3 typy predykatów, które można skomponować (orem) (Status, isBlocked, hasPackage) , a potem tak jak np. @Shalom napisał

czyli coś w stylu

 ... 
 new Cośtam(  
    ObjectPredicates.of( Status.IN_PROGRESS)
   .or( ObjectPredicates.of( Status.EXECUTED))
   .or( ObjectPredicates.HasPackageName), 
    3, 94);

1 użytkowników online, w tym zalogowanych: 0, gości: 1