search

iiuni / projektzapisy

System Zapisów na zajęcia w Instytucie Informatyki Uniwersytetu Wrocławskiego
https://zapisy.ii.uni.wroc.pl
32 stars 10 forks source link

Refaktor i ew. deduplikacja komponentów `CourseFilter.vue`, `CourseList.vue` #1679

Open lgpawel opened 7 months ago

lgpawel commented 7 months ago

Niedawno doprowadzone do końca prace nad #1325 obejmowały zmiany w trzech komponentach o tej samej nazwie CourseFilter.vue robiących w trzech (a nawet czterech) miejscach systemu bardzo podobne rzeczy. Przy okazji prac te trzy źródła zostały upodobnione do siebie tak bardzo, jak się dało – różnice pomiędzy wersjami z aplikacji enrollment/timetable oraz offer/proposal są oczywiste; różnica względem trzeciego (z offer/vote) jest nieco dalej idąca, bo ten ostatni zawiera fragmenty kodu, które w pozostałych przypadkach znajdują się w towarzyszących komponentach CourseList.vue (a tutaj takiego towarzyszącego komponentu nie ma).

Naturalnie sytuacja, w której mamy kilka kawałków bardzo podobnego kodu robiącego prawie to samo, jest nieoptymalna, czego zresztą doświadczyliśmy przy ww. pracach. Należałoby więc przyjrzeć się dokładnie tym komponentom, zidentyfikować różnice pomiędzy nimi (głównie takie widoczne dla użytkownika, a nie wynikające wyłącznie z np. różnej organizacji kodu) i ujednolicić je jeszcze dalej. Idealnie byłoby móc zrobić to bez zmieniania funkcjonalności, ale jeśli okaże się, że ceną, którą należy za to ujednolicenie zapłacić, jest umieszczenie w interfejsie jakiejś (sensownej) kontrolki, której tam akurat nie ma, to oczywiście tak zrobimy (te decyzje będziemy podejmować, jak już poznamy konkretne wybory, przed jakimi stoimy). Komponenty CourseList.vue różnią się od siebie na pierwszy rzut oka dosyć mocno (nie grzebaliśmy przy nich przy ww. PRze) i nie jest jasne, czy da się w nich wiele ugrać, ale nawet jeśli nie, to trzeba to potwierdzić.

Punkt wyjścia do grzebania to następujące zależności, w które wchodzą interesujące nas pliki:

rafalstarypan commented 5 months ago

Do powyższego opisu dodaję również uwagi, które otrzymałem od Pana na Slacku w wiadomości prywatnej:

Moje doprecyzowanie do #1679 jest takie, że CourseFilter.vue wyglądają tak podobnie do siebie, dlatego, że przy okazji prac nad nimi zostały ręcznie uporządkowane z właśnie taką myślą. Chciałbym, żeby podobnie wyglądały CourseList.vue, albo żebyśmy mieli czarno na białym uzasadnienie nieusuwalnych różnic. Oczywiście robiąc to, trzeba uwzględnić np. że w jeden z CourseFilter.vue wykorzystywany w dwóch miejscach współdziała (?) z dwoma różnymi CourseList.vue, z kolei z których jedno współdziała dodatkowo (?) z Prototype.vue.

Z kolei ta "poważna" różnica pomiędzy jednym z CourseFilter.vue a pozostałymi pewnie będzie się wiązała z faktem, że on nie ma żadnej "swojej" CourseList.vue. To pewnie oznaczałoby, że jego upodobnienie do pozostałych wiązałoby się z jednym z dwóch kroków wydzieleniem z tego jednego CourseFilter.vue jakiegoś kadłubkowego, kilkulinijkowego komponentu odpowiadającemu CourseList.vue z pozostałych miejsc - to pewnie nie byłby dobry pomysł, ale chętnie zobaczyłbym jego działającą realizację jako proof of concept, a myślę, że sama implementacja byłaby na tyle małym wysiłkiem, że nie będzie wielką szkodą go później wycofać przeniesieniem do pozostałych CourseFilter.vue (i zapewne również Prototype.vue) kawałka logiki z CourseList.vue, żeby upodobnić te dwa "podobniejsze" do tego trzeciego (bez wydzielania kadłubka jak w alternatywnym podejściu).

Zdaję sobie w pełni sprawę, że to jest jakieś bredzenie na podstawie oglądania struktury importów i pobieżnych diffów, ale na tym polega ten issue, żeby to bredzenie skonkretyzować i zamienić na, w ramach absolutnego minimum, jakieś komentarze w kodzie rozjaśniające sytuację (ale założę się, że jakieś porządki w różnych CourseList.vue są możliwe i należy je zrealizować)

rafalstarypan commented 5 months ago

Przeprowadziłem analizę, o którą Pan prosił i doszedłem do następujących wniosków:

  1. Komponenty CourseFilter

W projekcie znajdują się 3 podobnie wyglądające komponenty o nazwie CourseFilter, które służą do filtrowania przedmiotów na różnych podstronach (strona główna, strona edycji prototypu planu, strona głosowania):

enrollment/timetable/assets/components/CourseFilter - dla uproszczenia dalszego opisu oznaczmy go przez A offer/proposal/assets/components/CourseFilter - dla uproszczenia dalszego opisu oznaczmy go przez B offer/vote/assets/components/CourseFilter - dla uproszczenia dalszego opisu oznaczmy go przez C

Komponenty te są do siebie dość podobne, jednak występują między nimi różnice w funkcjonalnościach (inne dostępne opcje filtrowania na różnych podstronach, różnice w stylach CSS, komponent C jest używany w innym kontekście niż A oraz B, ponieważ nie występuje wspólnie z komponentem CourseList). Kod komponentów jest czytelny, są one zbudowane w oparciu o regułę kompozycji poprzez składanie mniejszych, reużywalnych komponentów.

Różnica pomiędzy komponentami A oraz C, na którą zwrócił Pan uwagę podczas naszej rozmowy przedstawia się następująco:

Fragment komponentu A: 

methods: {
    ...mapMutations("filters", ["clearFilters"]),
},

Fragment komponentu C:

this.$store.subscribe((mutation, _) => {
    switch (mutation.type) {
    case "filters/registerFilter":
        this.refreshFun(this.tester);
        break;
    }
});

Komponent A posiada przycisk "Wyczyść filtry". Widoczny fragment kodu czyści stan filtrów po naciśnięciu tego przycisku. Komponent C nie posiada takiego przycisku. Co więcej, jak nadmieniłem powyżej, jest on używany w innym kontekście niż pozostałe i musi śledzić zmiany w stanie, za co odpowiada powyższy fragment kodu. W związku z tym, nie ma możliwości dalszego upodobnienia do siebie tych komponentów, a ta różnica jest jak najbardziej potrzebna i naturalna. Uważam obecny stan kodu za czytelny. Dalsza próba ugeneryczniania tych komponentów nie sprawdzi się, ponieważ przyszłe feature requesty będą prawdopodobnie dotyczyć dalszej customizacji podstron, co jest łatwe do wykonania w obecnym kodzie, a będzie znacznie trudniejsze i mniej czytelne przy jednym, bardzo ogólnym komponencie.

W moim pull requescie dodałem komentarze ułatwiające zapoznanie się z kodem i szybsze zrozumienie sytuacji.

  1. Komponenty CourseList.

W projekcie znajdują się 3 komponenty o nazwie CourseList, które służą do wyświetlania przedmiotów na różnych podstronach (strona główna, strona edycji prototypu planu, strona głosowania):

enrollment/timetable/assets/components/CourseList - dla uproszczenia dalszego opisu oznaczmy go przez A enrollment/courses/assets/components/CourseList - dla uproszczenia dalszego opisu oznaczmy go przez B offer/proposal/assets/components/CourseList - dla uproszczenia dalszego opisu oznaczmy go przez C

Komponenty te nie są do siebie w ogólne podobne. Każdy z nich jest wykorzystywany na innej podstronie i zawiera dużo logiki związanej z daną podstroną. Jakość kodu w tych komponentach jest dobra. Nie widzę żadnych możliwości ugenerycznienia tych komponentów ani upodobnienia ich do siebie, ponieważ realizują istotnie różne od siebie funkcjonalności.

Komponent A jest niepotrzebnie importowany w pliku enrollment/timetable/assets/components/Prototype.vue, co utrudnia czytanie kodu i wprowadza niepotrzebne zamieszanie.

W moim pull requescie pozbyłem się zbędnych importów oraz dodałem komentarze pozwalające na łatwiejsze zapoznanie się z kodem.

lgpawel commented 5 months ago

Nie widzę żadnych możliwości ugenerycznienia tych komponentów ani upodobnienia ich do siebie, ponieważ realizują istotnie różne od siebie funkcjonalności.

Nie zgadzam się, choć być może to dlatego, że najwyraźniej nie dogadujemy się co do oczekiwań. Pierwszy przykład z brzegu (może dość prymitywny, ale rzeczy pierwsze z brzegu mają to do siebie): w komponentach CourseList.vue z aplikacji enrollment m.in.:

i to są właśnie przykłady zmian może trywialnych, ale jednak sprawiających, że (i ja literalnie mam to na myśli) w diffie między tymi plikami atakuje nas mniej rzeczy. W tych samych dwóch plikach widać, że:

i oczywiście nie mam wprawdzie pewności, ale wciąż mocne poczucie, że różnice wynikają w dużej części z użytej dawki cukru syntaktycznego, tj. można którąś z tych rzeczy zrefaktorować tak, by różnice, które pozostaną (i być może zgadzam się, że powinny pozostać) ograniczyły się do tych stricte merytorycznych. W którą stronę miałyby pójść zmiany – na ten moment zupełnie nie mam zdania (oczywiście trzeci plik CourseList.vue może rozstrzygnąć tę wątpliwość – albo ją pogłębić).

rafalstarypan commented 5 months ago

Zgodnie z Pana sugestią usunąłem różnice pomiędzy komponentami CourseList tak, aby w diffie były widoczne tylko merytoryczne różnice pomiędzy komponentami.