Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
lgpawel opened this issue Apr 6, 2024 · 4 comments · May be fixed by #1709
Open

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

lgpawel opened this issue Apr 6, 2024 · 4 comments · May be fixed by #1709
Assignees

Comments

@lgpawel
Copy link
Contributor

lgpawel commented Apr 6, 2024

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:

  • enrollment/timetable/assets/prototype-index.js importuje
    • enrollment/timetable/assets/components/CourseFilter.vue,
    • enrollment/timetable/assets/components/CourseList.vue,
    • oraz enrollment/timetable/assets/components/Prototype.vue, który z kolei importuje to samo CourseList.vue, co powyżej (ten import okazał się jakoby niepotrzebny);
  • enrollment/courses/assets/course-list-index.js importuje
    • enrollment/timetable/assets/components/CourseFilter.vue (czyli to samo, co powyżej) i
    • enrollment/courses/assets/components/CourseList.vue (czyli inne, niż powyżej);
  • offer/proposal/assets/course-list-index.js importuje (oba inne niż powyższe)
    • offer/proposal/assets/components/CourseFilter.vue i
    • offer/proposal/assets/components/CourseList.vue
  • offer/vote/assets/point-counter.ts importuje offer/vote/assets/components/CourseFilter.vue (inne niż powyższe) i żadnego CourseList.vue (ale rzecz jasna są tam jeszcze inne importy, którym może przyjdzie się przyjrzeć, tak jak i w innych wymienionych plikach).
@rafalstarypan rafalstarypan self-assigned this Apr 26, 2024
@rafalstarypan
Copy link
Collaborator

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
Copy link
Collaborator

rafalstarypan commented Jun 6, 2024

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
Copy link
Contributor Author

lgpawel commented Jun 7, 2024

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.:

  • różnią się identyfikatory użyte w argumencie wywołania this.$store.subscribe();
  • jeden z nich deklaruje typ CourseObject, ale to wygląda na martwy kod

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:

  • jeden zawiera deklarację export default Vue.extend(...), w której środku jest fragment computed: /* coś tam */
  • w drugim zaś mamy export default class CourseList extends Vue {...} poprzedzoną blokiem @Component(computed: /* coś innego */, /* to samo co w tym drugim */)

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
Copy link
Collaborator

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment