Prośba o ocenę programu

Dzień dobry wszystkim / cześć

Otóż mam taki oto pierwszy większy program. Jeszcze w trakcie prac ale jako, że nie mam pojęcia czy to się do czegoś nada i czy warto to rozwijać i poprawiać czy lepiej wyrzucić i zacząć od nowa to proszę o konstruktywną krytykę.

https://github.com/SandraMarszalek/Fakturowanie

Czego brakuje, a mam zamiar wprowadzić (o ile nie będzie już teraz wszystko do wywalenia):

1. Czysty kod (jestem w trakcie lektury książki)
2. Walidacja wprowadzanych danych
3. Zapis pliku PDF pod wybraną nazwą i w wybranej lokalizacji
4. Zapis słowny finalnej kwoty
5. Dopracowanie ogólnych mniejszych i większych błędów, które widzę sama, np. przy kasowaniu wierszy tabeli głównej jeśli już wcześniej przeliczyłam finalne wyniki wartości w tabeli ostatecznej nie ulegają zmianie. A chciałabym żeby zmieniały się dynamicznie.
6. Dopracowanie graficzne.

Oczywiście jeśli okaże się, że coś ten kod jest jednak wart (cokolwiek) to na pewno usprawnień będzie więcej. Generalnie jeśli będzie działać dobrze to będzie wykorzystany przez lokalne mikroprzedsiębiorstwo i dlatego część rzeczy jest już spersonalizowana pod wymagania tej firmy (np. dodatkowe uwagi do faktury czy możliwość rozliczania produktów 8% i 23% tylko na osobnych fakturach).

C# to nie mój teren, ale takie ogólne spostrzeżenia:

1. Kod piszesz po angielsku - bardzo dobrze. Kometarze też pisz po angielsku.
2. Przydałoby się rozdzielić logikę od prezentacji, na razie większość się dzieje w jednym pliku Form1.cs
3. Repozytorium: 1 commit, tzn. nie korzystasz z gita. Korzystaj z gita i nie ładuj do repo wykomentowanego kodu. Po to właśnie masz gita, żeby móc sobie ewentualnie wrócić do poprzednich wersji. Nie ma README, więc nie ma informacji, co to robi (są tu na forum, ale to tak ogólnie dobrze przyjęta i słuszna praktyka, żeby opisać co się zrobiło i jak to zbudować w repozytorium).

@sugar_hiccup: dzięki za komentarz. Nie ma nic więcej na GitHub bo jakoś tak robiłam i zawsze obiecywałam sobie, że wrzucę, a później przy wyłączaniu komputera uświadamiałam sobie, że znowu tego nie zrobiłam. Obiecywałam sobie ,że jutro... No i tak się zeszło do dziś. Miałam jedna wersję z samego początku jak zaczynałam cokolwiek w tym grzebać ale tamten początek był tak kompletnie od tej dzisiejszej wersji inny, że już wolałam zrobić nowe repo.
Co do uwagi o rozdzieleniu funkcjonalności - MVC też mam w planach tylko już sama nie wiem czy powinnam to zrobić teraz czy już na gotowym programie poprzenosić.

Co do gita, to commituj mało a często. Praca na zasadzie jeden dzień pracy - jeden commit to też nie ma sensu za bardzo. Jeden działający feature - jeden commit. Jeden bugfix - jeden commit.

MVC też mam w planach tylko już sama nie wiem czy powinnam to zrobić teraz czy już na gotowym programie poprzenosić.

Jak zakładasz sobie jakąś architekturę, no to oczywiście od razu ją implementuj. To co już masz, przerób, nie jest tego dużo, a dalej już rób w MVC. Im więcej będziesz miała kodu, tym ciężej będzie go przenieść i trudniej będzie planować kolejne ficzery.

Aha, no i testy pisz (;

No i właśnie testy to jest to czego jeszcze nie dotknęłam nigdy i trochę nie wiem jak zacząć :) a też ciężko znaleźć mi coś fajnie to zagadnienie tłumaczącego w sieci.

Sandra napisał(a):

@sugar_hiccup: dzięki za komentarz. Nie ma nic więcej na GitHub bo jakoś tak robiłam i zawsze obiecywałam sobie, że wrzucę, a później przy wyłączaniu komputera uświadamiałam sobie, że znowu tego nie zrobiłam.

Trochę OOT, ale jak masz taki problem z pamięcią :) to możesz commitować rano, przed pracą, nie? :)

sugar_hiccup napisał(a):

Co do gita, to commituj mało a często. Praca na zasadzie jeden dzień pracy - jeden commit to też nie ma sensu za bardzo. Jeden działający feature - jeden commit. Jeden bugfix - jeden commit.

No nie wiem, od tego są gałęzie -- ale może masz na myśli commitowanie na serwer (choć ja i gałęzie zwykle pcham). Ja tam robię commity zawsze jak kończę/przerywam pracę (a i częściej), no i nie czekam aż mi coś działa...

@Sandra: Teraz bardziej merytorycznie (ale znowu zaczniemy od gita :)):

• Nie masz .gitignore a to się w jakiś sposób szybko zemści.

Sandra napisał(a):

No i właśnie testy to jest to czego jeszcze nie dotknęłam nigdy i trochę nie wiem jak zacząć :) a też ciężko znaleźć mi coś fajnie to zagadnienie tłumaczącego w sieci.

• Testy -- jak nie wiesz jak robić (w sensie nie umiesz, nie masz podejścia, praktyki itp.), to rób jakkolwiek. Byle testy są lepsze (IMHO) od żadnych. Zrób sobie modulik (lub kilka), który będzie Ci po Twojemu testował, co napisałaś (niech wyświetla wyniki itp.) -- tak, żebyś Ty wiedziała, czy działa w różnych przypadkach. Warto też zainteresować się TDD (w skrócie -- najpierw piszesz testy, a potem to, co ma w nich działać).

• Trochę z przerażeniem patrzę na tasiemcowe klasy/metody... W pliku Form1.cs masz klasę Invoice, która się ciągnie przez cały plik (ponad 400 linii), ale to jeszcze pół biedy. Są tam funkcje (motody), które nie sposób ogarnąć wzrokiem (a więc i umysłem), ale zdecydowanie najgorsza to PDF_Click (linie 313--429!). Musisz ją podzielić na sensowne mniejsze funkcje, które robią konkretnie wydzielone podzadania (i sensownie nazwane), bo się zgubisz.
  [Chyba, że to metoda wygenerowana automatycznie i właśnie robię z siebie idiotę... :)]

• Masz zakomentowane fragmenty kodu. Gdybyś używała gita, nie musiałabyś tego robić, a nie wróży to niczego dobrego... [C5 z "Czystego kodu" Roberta Martina]

@koszalek-opalek: wiem, że te tasiemce nie są najszczęśliwsze ale to pierwszy taki program (tak wiem marne tłumaczenie) i jakoś tak dodawałam kolejne rzeczy, na które wpadłam i tak to sobie szło... :) No tak wyszło. Poprawie na pewno. Już myślę, że samo MVC trochę to rozwiąże. A resztę jakoś popakuję ładniej. Chociażby to nieszczęsne PDF-Click :D tutaj tak bardzo skupiłam się na ogarnięciu itext, że już nawet długość przestała mi przeszkadzać xd

Co do gitignore - nie jestem w sumie pewna czy dobrze rozumiem jego założenie. Może jak tutaj napisze jak to rozumiem to nakierujesz mnie czy słusznie czy źle to rozumiem. Do gitignore powinnam wpisać wszystkie pliki poza tymi, które już mam w repo tak? Nie byłam pewna co zrobić z plikiem do bazy danych ...

Testy na pewno muszę spróbować zacząć robić. Nigdy żadnego nie robiłam więc trochę się boję, że nie dam rady...taka głupia obawa.

Komentarze znikną. Commity się pojawia :) kwestia wyrobienia nawyku. Muszę sobie przykleić jakaś kartkę na monitor z przypomnieniem.

Jeśli zapatrujesz się przyszłościowo na pisanie w C#, to staraj się oddzielać rzeczywisty kod od formy. Z klas formy korzystaj tylko w momencie, kiedy rzeczywiście musisz na niej wykonać jakąś operację.

Poza tym, jak widzę więcej niż jedną linijkę odstępu między kodem + spację pomiędzy if/while/switch/whatever do nawiasu to krew mnie zalewa.

Pozdrawiam.

@MexikanoS: czyli generalnie praktyka jest taka żeby nie robić spacji? Cóż przyznam, że tak mnie uczono jeszcze w czasach liceum i tak z tym zostałam nie zastanawiałam się nad tymi spacjami. Tak było czytelniej jakoś - pewnie dlatego, że tak mi wpojono

@Sandra: co do odpowiedzi @MexikanoS - zgadzam się, rób jedną linijkę odstępu pomiędzy metodami:

• https://github.com/SandraMarszalek/Fakturowanie/blob/master/Form1.cs#L28 - tu nie potrzeba
• https://github.com/SandraMarszalek/Fakturowanie/blob/master/Form1.cs#L44 - tu brak
• https://github.com/SandraMarszalek/Fakturowanie/blob/master/Form1.cs#L68 - tu masz niepotrzebnie dwie, nie wygląda to schludnie
• https://github.com/SandraMarszalek/Fakturowanie/blob/master/Form1.cs#L109 - tu też dwie
• https://github.com/SandraMarszalek/Fakturowanie/blob/master/Form1.cs#L179 - tutaj w ogóle brak

Rozumiesz o co chodzi? :)

Co do spacji po if/while/try/catch/etc to ja osobiście nie widzę nic złego w stawianiu spacji po tych słówkach, sam osobiście tak robię i wygląda to dla mnie czytelniej, nawet sam Microsof w swojej dokumentacji tak robi więc :)

Sandra napisał(a):

Co do gitignore - nie jestem w sumie pewna czy dobrze rozumiem jego założenie. Może jak tutaj napisze jak to rozumiem to nakierujesz mnie czy słusznie czy źle to rozumiem. Do gitignore powinnam wpisać wszystkie pliki poza tymi, które już mam w repo tak? Nie byłam pewna co zrobić z plikiem do bazy danych ...

Moim zdaniem nie do końca dobrze to rozumiesz. :)

Póki dodajesz pliki po jednym, konkretne, to bez tego pliku wytrzymasz. Wydaje mi się on jednak bardzo praktyczny, bo prędzej czy później ktoś (nie koniecznie Ty, to może być Twój współpracownik w projekcie) doda cały podkatalog i pójdzie do repo ze śmieciem. No .gitignore jest do odfiltorowania tego śmiecia. Zwróć uwagę na dostępne wzory takich plików, na przykład (do C#) ten:
https://gist.github.com/brazilnut2000/8226958
Obejmują one różne pliki, które mogą pojawić się "same" w związku z używaną technologią, edytorami, kompilatorami itp.

Rzuć okiem może jeszcze tu:
https://www.pluralsight.com/guides/how-to-use-gitignore-file
https://www.atlassian.com/git/tutorials/saving-changes/gitignore
https://help.github.com/en/github/using-git/ignoring-files

Sama składnia jest bardzo prosta -- w skrócie:

• # to komentarz
• jedna linijka (nie pusta i niezakomentowana) == jeden wzorzec pliku
• * we wzrocu == wiadomo :)
• ** we wzorcu == dowolna ścieżka dostępu (nie tylko katalog)

@Sandra: tu masz przykładowy wzór pliku .gitignore, który byś mogła użyć w swoim projekcie WinForms - https://github.com/AdamsLair/winforms/blob/master/.gitignore

@XardasLord:
@koszalek-opalek:
Wielkie dzięki :) będę poprawiać i pojawię się na pewno z kolejną wersją. Oczywiście jakbyście jeszcze coś zauważyli to czekam na info :)

@Sandra: No to tak żeby się jeszcze poprzyczepiać :)

• Nazwa pliku to Form1.cs, natomiast klasa nazywa się już Invoice :)
• https://github.com/SandraMarszalek/Fakturowanie/blob/master/Form1.cs#L46 - ten fragment kodu można znacząco uprościć, poprzez następującą linijkę, która załatwi Ci tego if-else: TextBoxAdditionalInvoiceInfo.Visible = AdditionalInvoiceInfo.Checked
• https://github.com/SandraMarszalek/Fakturowanie/blob/master/Form1.cs#L74 - w C# nazwy metod mają konwencję PascalCase, a nie camelCase
• https://github.com/SandraMarszalek/Fakturowanie/blob/master/Form1.cs#L96 - nigdy tego nie rób. Nie pozostawiaj wyjątku bez obsługi, bo program może Ci się sypać, a Ty nie będziesz wiedziała czemu, przynajmniej jakieś logi zastosuj, ale tutaj ten catch to sytuacja wyjątkowa, takie coś na pewno powinno zostać zasygnalizowane, że nastąpił wyjątek
• https://github.com/SandraMarszalek/Fakturowanie/blob/master/Form1.cs#L139 - zakomentowany kod do usunięcia
• https://github.com/SandraMarszalek/Fakturowanie/blob/master/Form1.cs#L167 oraz https://github.com/SandraMarszalek/Fakturowanie/blob/master/Form1.cs#L213 oraz https://github.com/SandraMarszalek/Fakturowanie/blob/master/Form1.cs#L256- a cóż to :)
• https://github.com/SandraMarszalek/Fakturowanie/blob/master/Form1.cs#L229 - staraj się wynieść konfigurację bazy danych (connection stringa) do Resource i z nich pobieraj wartości, po to one są :)
• https://github.com/SandraMarszalek/Fakturowanie/blob/master/Form1.cs#L313 - strasznie długa i nieczytelna metoda. Postaraj się wynieść pewne konfiguracje i ustawiania np paragrafów czy coś do prywatnych metod. A jeszcze lepiej wynieść to do osobnej klasy, która będzie odpowiedzialna za tworzenie PDF'a.