Unikanie IF'ów problem

0

Hej
Pisząc swoją małą aplikację wpakowałem się w stos IF'ów, który wygląda tragicznie i łamie zasadę S z SOLID, ale nie mam pomysłu jak ją zastąpić czymś sensownym.

Kawałek kody wygląda tak:

public List<Offer> sortBy(List<Offer> offersToSort, String sortBy, String order) {
        if (order.equals("ASC")) {
            if (sortBy.equals("price")) {
                offersToSort.sort(Comparator.comparingInt(Offer::getPrice));
            }
            if (sortBy.equals("title")) {
                offersToSort.sort(Comparator.comparing(Offer::getTitle));
            }
            if (sortBy.equals("year")) {
                offersToSort.sort(Comparator.comparingInt(Offer::getYear));
            }
            if (sortBy.equals("mileage")) {
                offersToSort.sort(Comparator.comparingInt(Offer::getMileage));
            }
            if (sortBy.equals("engineSize")) {
                offersToSort.sort(Comparator.comparing(Offer::getEngineSize));
            }
            if (sortBy.equals("enginePower")) {
                offersToSort.sort(Comparator.comparingInt(Offer::getEnginePower));
            }
            if (sortBy.equals("doors")) {
                offersToSort.sort(Comparator.comparingInt(Offer::getDoors));
            }
            if (sortBy.equals("colour")) {
                offersToSort.sort(Comparator.comparing(Offer::getColour));
            }
            if (sortBy.equals("model")) {
                offersToSort.sort(Comparator.comparing(o -> o.getModel().getName()));
            }
            if (sortBy.equals("bodyStyle")) {
                offersToSort.sort(Comparator.comparing(o -> o.getBodyStyle().getName()));
            }
            if (sortBy.equals("fuelType")) {
                offersToSort.sort(Comparator.comparing(o -> o.getFuelType().getName()));
            }
        } else if (order.equals("DESC")) {
            if (sortBy.equals("price")) {
                offersToSort.sort(Comparator.comparingInt(Offer::getPrice).reversed());
            }
            if (sortBy.equals("title")) {
                offersToSort.sort(Comparator.comparing(Offer::getTitle).reversed());
            }
            if (sortBy.equals("year")) {
                offersToSort.sort(Comparator.comparingInt(Offer::getYear).reversed());
            }
            if (sortBy.equals("mileage")) {
                offersToSort.sort(Comparator.comparingInt(Offer::getMileage).reversed());
            }
            if (sortBy.equals("engineSize")) {
                offersToSort.sort(Comparator.comparing(Offer::getEngineSize).reversed());
            }
            if (sortBy.equals("enginePower")) {
                offersToSort.sort(Comparator.comparingInt(Offer::getEnginePower).reversed());
            }
            if (sortBy.equals("doors")) {
                offersToSort.sort(Comparator.comparingInt(Offer::getDoors).reversed());
            }
            if (sortBy.equals("colour")) {
                offersToSort.sort(Comparator.comparing(Offer::getColour).reversed());
            }
            if (sortBy.equals("model")) {
                offersToSort.sort(Comparator.comparing((Offer o) -> o.getModel().getName()).reversed());
            }
            if (sortBy.equals("bodyStyle")) {
                offersToSort.sort(Comparator.comparing((Offer o) -> o.getBodyStyle().getName()).reversed());
            }
            if (sortBy.equals("fuelType")) {
                offersToSort.sort(Comparator.comparing((Offer o) -> o.getFuelType().getName()).reversed());
            }
        }

        return offersToSort;
    }

W skrócie o co chodzi, do metody dostarczam liste ofert, znacznik po czym sortujemy i znacznik ordeowania. W zależności od znaczników zwracana jest odpowiednio posortowana/orderowana lista

Jak to rozgryźć bez ifów?

3

Nie powinieneś sortowania robić po stronie bazy danych?
(przy czym nie jest to tyle uniknięcie problemu, co przeniesienie go do miejsca wygodniejszego w obsłudze ;-))

2

Refleksją by dużo ugryzł.
Wprawdzie (moje subiektywne wyobrażenie) łatwiej jest przejść z fizycznej nazwy metody getEnginePower na nazwę beanową enginePower, a w drugą (chyba) nieco trudniej 1), ale tez do zrobienia. Są też biblioteki do obsługi beanów.

Więc wtedy masz RAZ kod, który na podstawie stringa z nazwą znajdzie stosowny getter

PS: zobacz w ten wątek Lambdy na kolekcjach - wybór pola

  1. mam przed oczami problem z metodą na wiele słów, czy choćby z opcjonalnym dla pól logicznych isXxxxx() / getXxxxx()
11 
    public List<Offer> sortBy(List<Offer> offersToSort, String sortBy, String order) {
        return "DESC".equals(order) ? descending(sortBy, offersToSort) : ascending(sortBy, offersToSort)
    }

    private List<Offer> ascending(String sortBy, List<Offer> data) {
        data.sort(applyComparator(sortBy));
        return data;
    }

    private List<Offer> descending(String sortBy, List<Offer> data) {
        data.sort(applyComparator(sortBy).reversed());
        return data;
    }

    private Comparator<Offer> applyComparator(String sortBy) {
        return switch (sortBy) {
            case "title" -> Comparator.comparing(Offer::getTitle);
            case "model" -> Comparator.comparing(Offer::getModel);
            case "price" -> Comparator.comparingInt(Offer::getPrice);
            default -> throw new IllegalStateException("Unexpected value: " + sortBy);
        };
    }

0 ifów ;)

1

A po co Ci ta metoda w ogole? Dlaczego w miejscach w ktorych potrzebujesz posortowac, nie wywolasz po prostu list.sort zamiast przekazywac te stringi do metody? Jedyny sens jaki w tym widze, to ze moznaby w ten sposob ukryc mechanizm sortowania, czyli java.util.Comparator ktory jest ze standardu javy przeciez, moznaby w przyszlosci na cos innego zamienic. Jesli jednak na pewno ma to takie znaczenie, to metoda moglaby przyjac ewentualnie Function<? super T,? extends U> keyExtractor gdzie U extends Comparable<? super U>

5

Jeśli zastosujesz enum-a to i switch-a można pożegnać:

enum SortBy {
    PRICE_ASC(Comparator.comparingInt(Offer::getPrice), Order.ASC),
    TITLE_DESC(Comparator.comparing(Offer::getTitle), Order.DESC),
    YEAR_ASC(Comparator.comparingInt(Offer::getYear), Order.DESC);

    enum Order{
        ASC(comparator -> comparator),
        DESC(Comparator::reversed);

        private final Function<Comparator<Offer>, Comparator<Offer>> processor;

        Order(Function<Comparator<Offer>, Comparator<Offer>> processor) {
            this.processor = processor;
        }

        Comparator<Offer> getComparator(Comparator<Offer> comparator){
            return processor.apply(comparator);
        }
    }

    private final Comparator<Offer> comparator;
    private final Order order;

    SortBy(Comparator<Offer> comparator, Order order){
        this.comparator = comparator;
        this.order = order;
    }

    public List<Offer> apply(List<Offer> offers){
        offers.sort(order.getComparator(comparator));
        return offers;
    }

    @Override
    public String toString() {
        return "Sort by " + name().toLowerCase().replace("_"," ");
    }
}

public class SortDemo {
    public static void main(String[] args) {
        List<Offer> offers = Arrays.asList(
                new Offer(100, "dudy", 2020),
                new Offer(90, "lody", 2019),
                new Offer(120, "buda", 2000),
                new Offer(86, "pufa", 2010)
        );
        System.out.println("Sort options");
        for(SortBy s: SortBy.values()){
            System.out.println(s.name() +" - " + s);
        }
        System.out.println("Input sort option");
        try{
            SortBy sortOption = SortBy.valueOf(new Scanner(System.in).next());
            System.out.println(sortOption.apply(offers));
        } catch (IllegalArgumentException e){
            System.out.println("Invalid option!!!");
        }
    }
}
1

@cs: lepiej zrobić dwa niezależne enumy - i wtedy bardzo łatwo takie coś uogólnić:

/** Jakies tam POJO */
public class Offer {
    private final String name;
    private final int price;

    public Offer(String name, int price) {
        this.name = name;
        this.price = price;
    }

    public String getName() {
        return name;
    }

    public int getPrice() {
        return price;
    }
}

/* Sortowanie */
public interface SortField<T> {
    Comparator<T> getComparator();
}

public enum SortOrder {
    ASC,
    DESC
    ;

    public <T> Comparator<T> decorate(final Comparator<T> decoratee) {
        if (this == DESC) {
            return decoratee.reversed();
        } else {
            return decoratee;
        }
    }

    public static SortOrder findByName(final String text) {
        return Stream.of(values())
                .filter(el -> el.name().equals(text.toUpperCase()))
                .findFirst()
                .orElseThrow();
    }
}

/* Sortowanie Offers */
public enum OfferSortField implements SortField<Offer> {
    NAME(Offer::getName),
    PRICE(Offer::getPrice)
    ;

    private final Comparator<Offer> comparator;
    OfferSortField(final Function<Offer, Comparable> keyExtractor) {
        this.comparator = Comparator.comparing(keyExtractor);
    }

    @Override
    public Comparator<Offer> getComparator() {
        return this.comparator;
    }

    public static OfferSortField findByName(final String fieldName) {
        return Stream.of(values())
                .filter(el -> el.name().equals(fieldName.toUpperCase()))
                .findFirst()
                .orElseThrow();
    }
}

/* i w końcu testy */
public class Sorting {
    public static <T> List<T> sort(final List<T> list, final SortField<T> sortField, final SortOrder order) {
        return list.stream()
                .sorted(order.decorate(sortField.getComparator()))
                .collect(Collectors.toList());
    }

    public static void test() {
        List<Offer> offers = getOffers();

        List<Offer> sorted = sort(offers, OfferSortField.findByName("price"), SortOrder.findByName("asc"));
    }
}

Dzięki temu unikniesz konieczności pisania 2x liczba pól w stylu TITLE_ASC, TITLE_DESC itp.

0

@wartek01: To był szkic na szybko zrobiony i oczywiście, że można sobie wyciągnąć tego enum-a i uogólniać. Chodziło mi o to, że w większości przypadków ten Order to powinien wyglądać tak:

enum SortBy {
    PRICE_ASC(Comparator.comparingInt(Offer::getPrice), Order.ASC),
    TITLE_DESC(Comparator.comparing(Offer::getTitle), Order.DESC),
    TITLE_ASC_THEN_PRICE_ASC(Comparator.comparing(Offer::getTitle), Order.ASC_THEN_PRICE_ASC),
    YEAR_ASC_THEN_TITLE_ASC(Comparator.comparingInt(Offer::getYear), Order.ASC_THEN_PRICE_ASC);

    private enum Order{
        ASC(comparator -> comparator),
        ASC_THEN_PRICE_ASC(comparator -> comparator.thenComparing(PRICE_ASC.comparator)),
        DESC(Comparator::reversed);
        private final Function<Comparator<Offer>, Comparator<Offer>> processor;
        Order(Function<Comparator<Offer>, Comparator<Offer>> processor) {
            this.processor = processor;
        }
        Comparator<Offer> getComparator(Comparator<Offer> comparator){
            return processor.apply(comparator);
        }
    }
...

Wtedy jest on specyficzny tylko dla enum-a sortującego klasę Offer i nawet warto zrobić go jako prywatny. Założenie jest takie, żeby nie tworzyć wszystkich możliwych kombinacji sortowań, ale wyspecyfikować te, które wymaga biznes.

I jedna uwaga, nie radziłbym tak definiować enum'a"

public enum SortOrder {
    ASC,
    DESC
    ;

    public <T> Comparator<T> decorate(final Comparator<T> decoratee) {
        if (this == DESC) {
            return decoratee.reversed();
        } else {
            return decoratee;
        }
    }

    public static SortOrder findByName(final String text) {
        return Stream.of(values())
                .filter(el -> el.name().equals(text.toUpperCase()))
                .findFirst()
                .orElseThrow();
    }
}

Dodanie następnej stałej wymaga grzebania w metodzie decorate i dodawania kolejnych if-ów, a metoda findByName robi praktycznie to samo co valueOf.

  public static Order findByName(final String text) {
            return SortOrder.valueOf(text.toUpperCase());
  }
2

jesteście kochani, tyle pomysłów, tyle sposobów!
dziękuje!

1
cs napisał(a):

@wartek01: To był szkic na szybko zrobiony i oczywiście, że można sobie wyciągnąć tego enum-a i uogólniać. Chodziło mi o to, że w większości przypadków ten Order to powinien wyglądać tak:

Rozumiem, że pewne rzeczy pisze się na szybko. Natomiast nie zgadzam się z klasą SortBy - łączenie porządku (Order) i tego, po czym się sortuje jest złym pomysłem. Nie mam pojęcia dlaczego miałbyś dopuszczać np. sortowanie po jednym polu rosnąco, ale już nie malejąco. W najgorszym przypadku możesz sobie zrobić zbiór par czy coś.

I jedna uwaga, nie radziłbym tak definiować enum'a"

W przypadku ogólnym - zgadzam się, że wewnętrzne metody nie powinny sprawdzać, czy coś jest jakąś metodą. W przypadku szczególnym - tj. gdy Order będzie miało tylko i wyłącznie dwie wartości - nie widzę w tym problemu.

metoda findByName robi praktycznie to samo co valueOf.

Tutaj zgoda. To takie stare przyzwyczajenie, które w innym przypadku się sprawdza. Natomiast tutaj lepiej zastosować valueOf.

0

@wartek01:

Rozumiem, że pewne rzeczy pisze się na szybko. Natomiast nie zgadzam się z klasą SortBy - łączenie porządku (Order) i tego, po czym się sortuje jest złym pomysłem. Nie mam pojęcia dlaczego miałbyś dopuszczać np. sortowanie po jednym polu rosnąco, ale już nie malejąco. W najgorszym przypadku możesz sobie zrobić zbiór par czy coś.

Jeszcze raz, nie chodzi o jakieś ogólne rozwiązanie super-hiper-fancy, ale czysto biznesową praktykę, w której aplikacja ma skończoną listę porządków sortowań np. po id i year, dla id rosnąco lub malejąco, bo jest unikalne, ale dla year już masz całe multum powtarzających się ofert i siłą rzeczy chcesz sortować po kolejnym polu. Żeby zrobić klasę, która wygeneruje wszystkie możliwe kombinacje porządków sortowań to nie da się tego zrobić samym enum-em, a wyliczenie z ASC i DESC pewnie by się przydało.

W przypadku ogólnym - zgadzam się, że wewnętrzne metody nie powinny sprawdzać, czy coś jest jakąś metodą. W przypadku szczególnym - tj. gdy Order będzie miało tylko i wyłącznie dwie wartości - nie widzę w tym problemu.

Nie zdajesz sobie jeszcze sprawy z siły enum-a, po części może z wrażenia, że dodanie nowej stałej wyliczeniowej to modyfikacja klasy, a to tylko dodanie kolejnej instancji klasy enum-a, bez ruszania definicji klasy. W Twoim rozwiązaniu definiujesz zachowanie obiektu, które zależy od istnienia innych instancji tej klasy. Twoje decorate nie powinno sprawdzać, którą instancją enum-a jest, bo to trochę na zasadzie: "jeśli jestem psem to szczekam".

2

Tak czytam i nasuwa mi się jedno pytanie. Na chwilę obecną retoryczne i być może głupie, ale po całym dniu telekonferencji czuję się odmóżdżony i usprawiedliwiony.

Co jeśli chcemy mieć rozwiązanie do sortowania wg dowolnej kombinacji pól? Sortowanie 5 razy kolekcji dla 5 pól, po których sortujemy, brzmi średnio (zakładając stabilność algorytmu sortowania).
Intuicyjnie sort powinien dostawać komparator, który wywoła łańcuch komparatorów dla wybranych pól. Tak, żeby nie pisać kolejnych TITLE_ASC_THEN_PRICE_ASC_THEN_FOO_THEN_BAR.

1

@yarel: Pytanie było o unikanie if-ów na przykładzie sortowań i to daje enum. To nie jest rozwiązanie na łatwe pisanie komparatorów, tylko skatalogowanie komparatorów tak, aby łatwo i bez if-ów je wyciągać na podstawie nazwy, co można zrobić też za pomocą mapy. Te stałą TITLE_ASC_THEN_PRICE_ASC_THEN_FOO_THEN_BAR musisz sam sobie zdefiniować.

1
cs napisał(a):

Jeszcze raz, nie chodzi o jakieś ogólne rozwiązanie super-hiper-fancy, ale czysto biznesową praktykę, w której aplikacja ma skończoną listę porządków sortowań np. po id i year, dla id rosnąco lub malejąco, bo jest unikalne, ale dla year już masz całe multum powtarzających się ofert i siłą rzeczy chcesz sortować po kolejnym polu. Żeby zrobić klasę, która wygeneruje wszystkie możliwe kombinacje porządków sortowań to nie da się tego zrobić samym enum-em, a wyliczenie z ASC i DESC pewnie by się przydało.

No spoko, ale takiego wymagania nie było. Jeśli jednak chcesz ograniczyć liczbę możliwych sortowań z poziomu użytkownika to powinieneś to zrobić na UI (EDIT: albo w warstwie walidacji), a nie w mechanizmie sortowania i rzucać wyjątkami.

Nie zdajesz sobie jeszcze sprawy z siły enum-a, po części może z wrażenia, że dodanie nowej stałej wyliczeniowej to modyfikacja klasy, a to tylko dodanie kolejnej instancji klasy enum-a, bez ruszania definicji klasy. W Twoim rozwiązaniu definiujesz zachowanie obiektu, które zależy od istnienia innych instancji tej klasy. Twoje decorate nie powinno sprawdzać, którą instancją enum-a jest, bo to trochę na zasadzie: "jeśli jestem psem to szczekam".

Całkowicie zlałeś to, co napisałem więc powtórzę ci to jeszcze raz. Zgadzam się w ogólności, jednak tu - w tym konkretnym przypadku - to w niczym nie przeszkadza bo istnieją dwa możliwe Order - ASC i DESC. Nie jestem w stanie znaleźć żadnego innego porządku dla dowolnej zależności jednowymiarowej.

@yarel: możesz łatwo łączyć komparatory z thenComparing (skorzystałem z poprzednich klas, które wkleiłem - ale łatwo obczaić o co chodzi):

public class Sorting {
    public static void main(String[] args) {
        String text1 = "NAME_ASC_THEN_PRICE_DESC";

        System.out.println("Unsorted");
        List<Offer> offers = getOffers();
        offers.stream()
            .map(o -> o.getName() + "=" + o.getPrice())
            .forEach(System.out::println);

        System.out.println("Sorted");
        offers.sort(parseComplexComparator(text1, OfferSortField::findByName));
        offers.stream()
                .map(o -> o.getName() + "=" + o.getPrice())
                .forEach(System.out::println);
    }

    private static <T> Comparator<T> parseComplexComparator(final String text, final Function<String, SortField<T>> sortFieldProvider) {
        String[] splitted = text.split("_THEN_");

        Comparator<T> comparator = null;
        for (String part : splitted) {
            if (comparator == null) {
                comparator = parseSingleComparator(part, sortFieldProvider);
            } else {
                comparator = comparator.thenComparing(parseSingleComparator(part, sortFieldProvider));
            }
        }

        return comparator;
    }

    private static <T> Comparator<T> parseSingleComparator(final String text, final Function<String, SortField<T>> sortFieldProvider) {
        String[] array = text.split("_");

        SortField<T> sortField = sortFieldProvider.apply(array[0]);
        SortOrder order = SortOrder.findByName(array[1]);

        return order.decorate(sortField.getComparator());
    }

    private static List<Offer> getOffers() {
        return Arrays.asList(new Offer("B", 1),
                            new Offer("C", 2),
                            new Offer("A", 1),
                            new Offer("C", 3),
                            new Offer("A", 2));
    }
}

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