Code review aplikacji do szybkiego tworzenia listy wyrazów/zdań z napisów

Proszę o code review aplikacji do szybkiego tworzenia listy wyrazów/zdań z obecnie załadowanych napisów. Aplikacja ma możliwość pobrania timestampu z programu VLC poprzez interfejs lua http. Klawisz do aktywacji ekranu wyboru to F11.

Link: https://github.com/Krztk/KtSubs/tree/master

1. Co to za zmora z nazywaniem pierwszego commita "Initial commit"? Nie mogłeś w nim zawrzeć po prostu tego co tam jest?
2. Testy są dosyć słabej jakości, ale dobrze że jakieś chociaż są.
3. To "arrange", "act", "assert" tez jest użyte trochę dziwnie, bo idea za tymi sekcjami "arrange", "act", "assert" (czy tam given, when, then) odnoszą się do jakiegoś stanu pre-istniejącego przed akcją. A ty wielu miejscach nie masz żadnego stanu pre-istniejącego, to są po prostu parametry wejściowe.
4. Jakiś powód czemu ten kod jest w teście:

   var pattern = "1, 3, 8";
   //act
   var result = selectionConverter.Convert(pattern, 10);
   

   zamiast po prostu
   var result = selectionConverter.Convert("1, 3, 8", 10);?.
   To jest właśnie wpadanie w pułapkę dopasowania się do arrange/act/assert.
5. Klasa PatternToSelectionConverter jest dziwna. Czemu jej użycie to jest new PatternToSelectionConverter().Convert("pattern", 5);. Czy nie lepiej byłoby zrobić np new Pattern("pattern").ToSelection(5);?
6. Ja bym zrobił rename SingleSelection na po prostu Selection.
7. Nazwy klas sugerują że nie do końca dobrze wiesz jak organizować kod. niektóre rzeczy są dobrze nazwane, ISelection, Entry, SubtitlesTypes, ale inne widać jasno że nie miałes pojęcia jak je nazwać dobrze: MediaTimerProvider, LayersSettingsManager, SubtitlesEntryFinder. To wygląda jakbys nie do końca wiedział gdzie "upchać" kod. Czemu nie nazwać ich po prostu MediaTime, LayersSettings, SubtitlesEntries?
8. https://github.com/Krztk/KtSubs/blob/master/KtSubs.CoreTests/Selections/RangeSelectionTests.cs Jakiś powód czemu to displayEntry jest ustawiane w setupie klasy, zamiast normalnie w metodzie testującej? Jaki jest cel tego zabiegu.
9. https://github.com/Krztk/KtSubs/blob/master/KtSubs.CoreTests/Selections/SingleSelectionTests.cs Tutaj imo to że displayEntry to jest pole klasy to jest zły pomysł. Jeśli chcesz współdzielić zależności (czego w sumie nie powinieneś robić), to lepiej jakby to była funkcja, a nie pole.
10. Nazwa klasy EntryMerger jest bardzo słaba. Nazwa powinna mówić jakimi kryteriami się posługuje klasa podczas mergowania entry'sów. Tutaj widać że chyba wtedy je łączy, jak mają więcej niż 1 .Count overlaping. Więc dobrze byłoby to jakoś zawrzeć w nazwie klasy, może ConjoinedEntries czy jakoś tak.
11. Ja bym zrobił rename TimeInterval na Time albo Interval.
12. Rename MkvSubtitleExtractor na MkvSubtitle? Swoją drogą pod to chyba nie ma testów?

Dziękuję za poświęcony czas oraz cenne uwagi.

8. https://github.com/Krztk/KtSubs/blob/master/KtSubs.CoreTests/Selections/RangeSelectionTests.cs Jakiś powód czemu to displayEntry jest ustawiane w setupie klasy, zamiast normalnie w metodzie testującej? Jaki jest cel tego zabiegu.
9. https://github.com/Krztk/KtSubs/blob/master/KtSubs.CoreTests/Selections/SingleSelectionTests.cs Tutaj imo to że displayEntry to jest pole klasy to jest zły pomysł. Jeśli chcesz współdzielić zależności (czego w sumie nie powinieneś robić), to lepiej jakby to była funkcja, a nie pole.

Miałem zamiar napisać więcej testów niż 1 i reużywać to pole tak jak to zrobiłem w linku z punktu 9. Pole jest ustawiane przed każdym testem na nowo. https://xunit.net/docs/shared-context

12. Rename MkvSubtitleExtractor na MkvSubtitle? Swoją drogą pod to chyba nie ma testów?

Zgadza się. Nie ma testów. MkvSubtitleExtractor korzysta z zewnętrznych programów wchodzących w skład MKVToolNix. Nie wiem jak przetestować kod, który uruchamia CLI i odczytuje wyniki.

Kokoniłaj napisał(a):

Miałem zamiar napisać więcej testów niż 1 i reużywać to pole tak jak to zrobiłem w linku z punktu 9. Pole jest ustawiane przed każdym testem na nowo. https://xunit.net/docs/shared-context

Słabe podejście. W ogóle reużywanie danych w testach to jest słaby pomysł. Każdy test powinien używać minimum danych potrzebnych do tego konkretnego testu. Takie współdzielenie danych skutkuje powstawaniem god-objectów do testów, na który się potem patrzy i nie wie które dane są do czego. Staraj się tego unikać.

12. Rename MkvSubtitleExtractor na MkvSubtitle? Swoją drogą pod to chyba nie ma testów?

Kokoniłaj napisał(a):

Zgadza się. Nie ma testów. MkvSubtitleExtractor korzysta z zewnętrznego programów wchodzących w skład MKVToolNix. Nie wiem jak przetestować kod, który uruchamia CLI i odczytuje wyniki.

No po prostu wywołaj tą funkcję w teście, i zrób asercję na jej wynik. To czy ta klasa "pod spodem" ma kod C# czy zewnętrzne narzędzie nie ma znaczenia. Przecież ma działać tak samo.