Lepsze są dwa SRP obroty, czy połączenie ich pod wydajność?

Piszę sobie parser. Używam go do podmiany pewnych elementów oraz raportowania. Tzn po przejechaniu pliku parserem dostaję nową wersję pliku oraz listę z warningami.

Generowanie tych warningow jest niezależnie od podmianek.

I teraz zastanawiam się jak zrobić - czy lepiej przejechać parserem raz, robiąc zarówno podmianki jak i zbierać warningi razem? Czy zrobić dwa obroty, za pierwszym razem tylko podmieniać (i w miejsce warningow wsadzić dummy impl), a za drugim zbierać warningi i w miejsce podmianki wsadzić implementację która zostawia kod tak jak był?

Co w ogóle parsujesz i w jakim celu?

I czego potrzebujesz do wygenerowania warningów?

Czy możesz wszystkie warningi wygenerować w locie w trakcie parsowania pliku?

Czy może istnieją warningi, do wygenerowania którychś musisz mieć w pełni sparsowany plik i dopiero musiałbyś się przejechać po kompletnym AST, żeby wyciągnąć pewne dane?

np. powiedzmy, że masz warning "nieużywana zmienna w kodzie" - żeby wiedzieć, która zmienna jest nieużywana, to trzeba by było przypuszczalnie sparsować wszystko i zanalizować te użycia. Chociaż w sumie... może by dało się to załatwić za pomocą jednego przebiegu cały czas śledząc użycie i na bieżąco to gdzieś zapisywać i potem jak sparsujesz raz, to już masz pełny komplet informacji. Trzeba by zrobić jakiś eksperyment.

LukeJL napisał(a):

Co w ogóle parsujesz i w jakim celu?

I czego potrzebujesz do wygenerowania warningów?

Out of scope.

Czy możesz wszystkie warningi wygenerować w locie w trakcie parsowania pliku?

Tak, są niezależne od przerabiania.

Czy może istnieją warningi, do wygenerowania którychś musisz mieć w pełni sparsowany plik i dopiero musiałbyś się przejechać po kompletnym AST, żeby wyciągnąć pewne dane?

Zarówno podmiana treści jak i warningi wymagają AST.

np. powiedzmy, że masz warning "nieużywana zmienna w kodzie" - żeby wiedzieć, która zmienna jest nieużywana, to trzeba by było przypuszczalnie sparsować wszystko i zanalizować te użycia. Chociaż w sumie... może by dało się to załatwić za pomocą jednego przebiegu cały czas śledząc użycie i na bieżąco to gdzieś zapisywać i potem jak sparsujesz raz, to już masz pełny komplet informacji. Trzeba by zrobić jakiś eksperyment.

Ale ja nie pytam o to - ja to już napisałem.

Bardziej pytam o wersję

Fun doStuff():
  foreach (parser.parse() as node) 
    replaceNode(node) 
    AddWarnings(node)

Vs

Fun doStuff1():
  foreach (parser.parse() as node) 
    replaceNode(node) 
Fun doStuff2():
  foreach (parser.parse() as node)
    AddWarnings(node)

Z jednej strony, podział na funkcje lepszy, bo SRP. Z drugiej, niepotrzebny narzut performance, s trzeciej jednak to mikrooptymalizacja, z czwartej, może te dwie rzeczy które korzystają z parsera powinny być razem.

Riddle napisał(a):

Bardziej pytam o wersję

Fun doStuff() +:
  foreach (parser.parse() as node) 
    replaceNode(node) 
    AddWarnings(node)

Vs

Fun doStuff1():
  foreach (parser.parse() as node) 
    replaceNode(node) 
Fun doStuff2():
  foreach (parser.parse() as node)
    AddWarnings(node)

i w tym kontekście (zakładam, że wynik dla obu przypadków jest taki sam i nie może być różny) to teraz co dla Ciebie jest lepiej? Będzie szybciej? - o ile? Gdzieś musisz te funkcje wywołać - jakakolwiek różnica czy zawołasz jedną funkcję czy dwie (oczywiście odpowiednio nazwane).

Osobiście wolę pierwsze podejście

@Riddle:

Gdyby to był post kogokolwiek innego, już byś go dawno przerobił w "trosce" o nieczytelność

Jak musisz to walnij sobie:

Fun doStuff(processors):
  foreach (parser.parse() as node)
     foreach (processors as process) 
        process(node)

ale IMO przepalasz czas i dokładasz sobie dodatkowej pracy, bez jakichś wyraźnych korzyści.

Jeśli czujesz, że tracisz wydajność to chociaż sprawdziłeś ile jej w ogóle tracisz? Nie przedstawiłeś, żadnych pomiarów więc zakładam, że w danej chwili nie boli Cię różnica, lecz może jakiś fakt posiadania nieidealnej wersji tego co sobie byś życzył. Dla mnie to jest słabość, perfekcjonizm. Patrz na kod przez pryzmat kolejnych wersji, przy czym pierwszej wersji pozwól być typowym gównem. To życie weryfikuje na ile to jest użyteczne, i na ile potrzebna jest kolejna zmiana. Bez sensu tak siedzieć i pisać po to by pisać.

A co do SRP, to jeśli czujesz, że tracisz, bo nie masz idealnie rozszerzalnego kodu to powiedz mi czy:

1. potrzebujesz każdą drobną rzecz osobno testować? czy jak je razem przetestujesz to będzie źle?
2. czy potrzebujesz takiej elastyczności w doStuff, aby co wywołanie inaczej przetwarzać te elementy?
3. czy zawsze całe przetwarzanie węzlów ogarnie Ci jedna funkcja z jednym schematem? a co jeśli będziesz musiał procesować węzły parami? albo jakimiś większymi grupkami? Bo wtedy może okazać się, że w zależności od przypadku te funkcje modyfikujące węzłów mają różną ilość argumentów.

Jeśli nie masz jakichś uzasadnionych ograniczeń to sprzęż tę pętlę z krokami krokami, a te funkcje replaceNode, AddWarnings oznacz jako prywatne i masz temat zamknięty. Jeśli replaceNode bądź AddWarnings mają implementację po 1-3 linie kodu to nawet nie pisałbym pod nie definicji, lecz te 1-3 linie kodu wsunąłbym bezpośrednio pod tą pętlę.

I dopóki całość dotyczy modyfikowania węzłów bez jakichś postronnych efektów typu zapis danych do bazy to większe rozbicie nie da Ci żadnej uzasadnionej korzyści poza sztuką dla sztuki, bo jak zmienisz technikę przetwarzania to kroki będziesz musiał prawdopodobnie na nowo implementować.

Zostawiam dwie rzeczy na które warto spojrzeć:

1. https://tonsky.me/blog/concrete-vs-abstract/
2. Oraz https://programmingisterrible.com/post/139222674273/write-code-that-is-easy-to-delete-not-easy-to poniżej fragment z tego postu:

Instead of breaking code into parts with common functionality, we break code apart by what it does not share with the rest. We isolate the most frustrating parts to write, maintain, or delete away from each other.
We are not building modules around being able to re-use them, but being able to change them.
Unfortunately, some problems are more intertwined and hard to separate than others. Although the single responsibility principle suggests that ‘each module should only handle one hard problem’, it is more important that ‘each hard problem is only handled by one module’

Lepsze jest to co się lepiej czyta. Jeżeli łamie SRP, to tym gorzej dla SRP.

Jeżeli zależy ci na czytelności to wybierz wersję pierwszą.
Z drugiej strony jeśli zależy Ci na wydajności to wybierz wersję pierwszą.

Widocznie źle się wyraziłem, bo widzę że większość odpowiedzi jest nie na temat który mnie interesował.

Waham się pomiędzy dwoma podejściami:

warnings = get_warnings(content) // to leci parserem, i wyciąga inspekcje 
if warnings.empty:
    new_content = transform(content) // to leci parserem drugi raz 
else:
    new_content = content

Vs

(new_content, warnings) = transformed_with_warnings(content) 
if warnings.empty:
    return new_content
else:
    return content

W pierwszym podejściu:

• dwie funkcje są od siebie całkowicie niezależne
• parser leci dwa razy (nie ma potrzeby usuwać, bo mikrooptymalizacja)
• obie pod spodem korzystają z podobnych mechanizmów - ten sam parser, takie samo AST, tylko różne instancje
• łatwiej rozwijać program z uwagi na loose-coupling, ale na pierwszy rzut oka ciężej zrozumieć że dwie funkcje wołają parser

W drugim podejściu:

• funkcje bardzo zależne od siebie
• jeden przejazd po AST
• łatwiej "zobrazować" całość działania programu, ale ciężej go rozwijać przez tight-coupling

Szczerze to ja nic nie rozumiem z tego postu i tematu.
Piszesz o SRP i wydajności. No i ok. Ale primo czy Warningi mogą istnieć oddzielnie od sparsowanej zawartości? Czy dając możliwość rozdzielenia tych funkcji, bo SRP, nie robisz w ogóle błędu?
Może jakieś Chain of responsiblities?
Jeśli chcesz z kolei badać pod kątem wydajności, to gdzie opis co parsujesz, jak duże itd. Bo tak to można powiedzieć chyba, że jedna pętla powinna być szybsza niż dwie.
Jak wszystko źle zrozumiałem, to opisz problem jeszcze raz.

Riddle napisał(a):

W pierwszym podejściu […] łatwiej rozwijać program z uwagi na loose-coupling
W drugim podejściu […] łatwiej "zobrazować" całość działania programu, ale ciężej go rozwijać przez tight-coupling

Mówisz? Dosyć trudno mi sobie wyobrazić parser, w którym w ogóle da się i jest sens osiągać taką niezależność, że generowanie błędów jest fundamentalnie niezależne od samego efektu działania parsera… Myślisz, że jest w jakimkolwiek stopniu realistycznym scenariusz, w którym wymieniasz raportowanie błędów, nie ruszając jednocześnie części modyfikującej plik; albo vice versa?

Istotnie łatwiej, ale wciąż nietrywialnie, jest mi przyjąć, że jednocześnie łatwo jest zrozumieć, co działa, jak działa, i czemu tak (zakładam, że właśnie to miałeś na myśli, pisząc o „łatwości zobrazowania”); oraz „wyjaśnienie” rzeczy czyni wyjaśnianą rzecz bardziej oporną na modyfikację.

No ale, przyjmując za dobrą monetę ww. wnioski, to pierwsze podejście wydaje się mieć praktycznie same zalety i bardzo mało wad. Mówisz, że przelecenie dwa razy to nie problem (i ruszanie tego to by była „mikrooptymalizacja” — IMO to brzmi bardziej, jak optymalizacja architektoniczna, czyli właściwie przeciwieństwo mikrooptymalizacji, ale miałem przyjmować Twoje stanowisko na wiarę…); a po co komu łatwość w zrozumieniu czegoś, skoro nie będzie mógł tego łatwo ruszyć tak czy owak.

Zatem jeśli faktycznie jest tak, jak piszesz, to nie widzę nawet powodu do zastanawiania się — bierz pierwszą opcję i fajrant. Jedyna wada (dwukrotny przejazd po pliku) nie jest wadą, a jedyna zaleta (łatwe zrozumienie nie tylko nieidące w parze z jakimkolwiek zyskiem praktycznym, ale wręcz powiązane z faktycznymi problemami) nie jest zaletą.

warnings = get_warnings(content) // to leci parserem, i wyciąga inspekcje 
if warnings.empty:
    new_content = transform(content) // to leci parserem drugi raz 
else:
    new_content = content

(new_content, warnings) = transformed_with_warnings(content) 
if warnings.empty:
    return new_content
else:
    return content

To ewidentnie zależy od przypadku użycia.

Czy jest jakikolwiek rzeczywisty scenariusz w którym ktoś chciałby pozyskać tylko warningi?

Albo scenariusz w którym ktoś chciałby dostać tylko output, bez warningów, bo miałby pewność że input jest OK?

A może chciałbyś mieć 3 takie APIs? TryParse zwracający output + warningi, ValidateInput zwracający tylko output; oraz ten trzeci, który tu już dziwnie wygląda ParseValidInput zwracający tylko output, ale będący jakoś tam uzasadniony wydajnościowo, bo załóżmy że jest to drogie obliczenie tych warningów.

Aczkolwiek jeżeli wszyscy korzystaliby tylko z TryParse, to jak najbardziej powinno to być w 1 pętli - po to, abyś nie marnował prądu na dwa przejścia.

Althorion napisał(a):

Riddle napisał(a):

W pierwszym podejściu […] łatwiej rozwijać program z uwagi na loose-coupling
W drugim podejściu […] łatwiej "zobrazować" całość działania programu, ale ciężej go rozwijać przez tight-coupling

Mówisz? Dosyć trudno mi sobie wyobrazić parser, w którym w ogóle da się i jest sens osiągać taką niezależność, że generowanie błędów jest fundamentalnie niezależne od samego efektu działania parsera… Myślisz, że jest w jakimkolwiek stopniu realistycznym scenariusz, w którym wymieniasz raportowanie błędów, nie ruszając jednocześnie części modyfikującej plik; albo vice versa?

Ale parser to jest szczegół implementacyjny mojej funkcji, a to właśnie warningi i podmiana treści jest elementem biznesowym.

jurek1980 napisał(a):

Szczerze to ja nic nie rozumiem z tego postu i tematu.
Piszesz o SRP i wydajności. No i ok. Ale primo czy Warningi mogą istnieć oddzielnie od sparsowanej zawartości?

W mojej aplikacji, w tym konkretnym przypadku - tak.

Czy dając możliwość rozdzielenia tych funkcji, bo SRP, nie robisz w ogóle błędu?

Nie sądzę, ale po to również jest ten wątek.

Może jakieś Chain of responsiblities?

Rozwiń.

Jeśli chcesz z kolei badać pod kątem wydajności, to gdzie opis co parsujesz, jak duże itd. Bo tak to można powiedzieć chyba, że jedna pętla powinna być szybsza niż dwie.

Powinna, ale w profilerze nie widać różnic pomiędzy jedną i dwoma, a więc to mikrooptymalizacja.

Jak wszystko źle zrozumiałem, to opisz problem jeszcze raz.

Pisze aplikacje, która dostaje plik na wejście, i musi wprowadzić w nim pewne zmiany. Możliwe że plik może mieć błędy składniowe lub smenatyczne, i wtedy aplikacja ma wypluć warningi. Parser to szczegół implementacyjny.

Pytanie polega na tym, czy:

• zrobić funkcje które leci parserem i znajduje warningi, a jak nie znajdzie żadnego to znowu leci parserem tym razem podmieniając treść

• czy zrobić jeden iterator AST, który robi jeden obrót, i podczas iteracji zarówno znajduje warningi jak i podmienia treść

To nadal pytanie bardziej "biznesowe", a nie czysto techniczne, bo rozchodzi się o to jak chcesz zamodelować swoje API i jak userzy będą z tego korzystać, albo jak chcesz ich "zachęcić" do korzystania z twojego API.

W podanym przypadku robisz jedną pętlę dla czytelności, gdyby modyfikacje bolały wydajnościowo to by lepiej było rozdzielić, bo może nie ma sensu tego robić.

Nie łamiesz SRP, tam nie ma szukania warningów i robienia zmian, tylko 2 node processory, możesz mieć ich na liście nawet 5.

Parsujesz plik, więc masz na wejściu kosztowne I/O, to mocno sugeruje robienie obu rzeczy równolegle. Otwierasz plik, ładujesz go w bufory parsera i walidatora, na koniec zbierasz wyniki i zwracasz jako całość.

Dzięki temu masz wydajnie, bo wejście, parser, walidator działają na osobnych wątkach, a do tego masz podzielone pomiędzy te elementy odpowiedzialności, więc jest też "ładnie".