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 https://4programmers.net/Foru[...]ybor_pola?p=1723204#id1723204

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.

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