Refactoring kodu

Refactoring kodu
0

Witam,

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

Kopiuj
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);
      }
Shalom
  • Rejestracja:około 21 lat
  • Ostatnio:około 3 lata
  • Lokalizacja:Space: the final frontier
  • Postów:26433
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.

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

I teraz kod tej metody to:

Kopiuj
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);
}

"Nie brookliński most, ale przemienić w jasny, nowy dzień najsmutniejszą noc - to jest dopiero coś!"
edytowany 1x, ostatnio: Shalom
shagrin
  • Rejestracja:około 17 lat
  • Ostatnio:ponad 6 lat
  • Lokalizacja:Norwegia, Stavanger
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")


jarekr000000
  • Rejestracja:ponad 8 lat
  • Ostatnio:minuta
  • Lokalizacja:U krasnoludów - pod górą
  • Postów:4707
1

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

Kopiuj
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ć....


jeden i pół terabajta powinno wystarczyć każdemu
wojciechmaciejewski
  • Rejestracja:ponad 12 lat
  • Ostatnio:około 2 lata
  • Postów:560
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:

Kopiuj
 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

wojciechmaciejewski
ależ się rzucili wszyscy do odpowiadania :D
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.

wojciechmaciejewski
pokaż te metody isStatus
0

Statusy:

Kopiuj

      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;
      }
wojciechmaciejewski
  • Rejestracja:ponad 12 lat
  • Ostatnio:około 2 lata
  • Postów:560
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

jarekr000000
  • Rejestracja:ponad 8 lat
  • Ostatnio:minuta
  • Lokalizacja:U krasnoludów - pod górą
  • Postów:4707
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

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

jeden i pół terabajta powinno wystarczyć każdemu
edytowany 2x, ostatnio: jarekr000000

Zarejestruj się i dołącz do największej społeczności programistów w Polsce.

Otrzymaj wsparcie, dziel się wiedzą i rozwijaj swoje umiejętności z najlepszymi.