Ocena kodu napisanego w C#

Question

Cześć :) Rzadko pokazuję swój kod, postanowiłem zasięgnąć waszej opinii- czy pisze wg was poprawnie i czytelnie?

Przykład apki robionej wedle planu (Nie uwzględniającego paru rzeczy, jak macierze choćby): 

I kilka klas napisanych przeze mnie:

 

DataContainer- http://wklej.org/id/1913524/
DataRepository- http://wklej.org/id/1913873/
Matrix - http://wklej.org/id/1913529/ (Służy do operacji na macierzach- nie skończona, choć działająca już dość sprawnie)
DataProcessor - http://wklej.org/id/1913530/


I 2 klasy testujące:

DataContainerTest - http://wklej.org/id/1913533/
DataProcessorTest - http://wklej.org/id/1913541/

Answer 1

IMO RemoveMeasurements powinno być rozdzielone na kilka funkcji lub inaczej zaprojektowane - zbyt dużo pętli.

http://wklej.org/id/1913529/ tu podobne są wrażenia. A pętlę w linii 163 możesz inaczej zapisać ;) Tak samo wszystkie inne w których iterujesz tylko zmienną.

Podpowiem tak. Wrzuć kod na GH, jako projekt. zrobię Ci issue do rzeczy które warto poprawić - raczej będą to pierdoły estetyczne, ponieważ kod jest zaskakująco (jak na to forum) dobry i ładny.

Comments

https://github.com/Surykat 

Projekt nazywa się Meteo.

Jest bałagan w repozytorium, pisałem o tym w komentarzu u Radka- dlatego wrzuciłem trochę swojego kodu na wklejkę, nie robię całego projektu samemu.

Dzięki za pomoc. :)

Pętla 163- masz na myśli zastosowanie pętli foreach? 

---

https://github.com/TransportKrausPoloTeam/Meteo/blob/master/meteo/meteo/Charts/ModelView.cs

Tu macie dużo powtórzeń tych samych wywołań w różnych metodach. Można pomyśleć o czymś, co będzie wyżej i załatwi to tylko w jednym miejscu.

W konstruktorze używacie dużo operatorów new. Można pomyśleć o jakiś fabrykach (różnego typu, byle nie singleton).

Same metody SetUPCOŚ możnaby było zrobić nieco inaczej, żeby jakaś abstrakcja budowała te obiekty. 

https://github.com/TransportKrausPoloTeam/Meteo/blob/master/meteo/meteo/Charts/ModelView.cs#L133 To nic nie mówi o błędzie. Klasa Exception jest zbyt ogólnikowa.

Patrzę na ten kod i widzę - super ktoś dobrze myśli. Jednak przyglądam się i widzę jednak, że nie do końca. Pomyślcie tutaj nad jakąś fabryką abstrakcyjną. Może Prototyp. Te metody są do siebie bardzo podobne, tak samo jak te metody, które później je wywołują (Temperatura, Wilgotnosc, Ciśnienie). 

Inna sprawa: mieszaczie angielski z polskim czasem są metody angielskie, czasem polskie, czasem zmienne są po angielsku a czasem po polsku - tak się nie robi. 

https://github.com/TransportKrausPoloTeam/Meteo/blob/master/meteo/meteo/DataInputs/DataSetter.cs#L16

Nazwa metody mówi coś zupełnie innego niż komentarz który jest jej przypisany. Uwierzyć metodzie czy komentarzowi? Nie wiem jak w C#, ale chyba można zrobić return new Temperature { bla bla bla } ? Po co tworzyć nową zmienną? 

https://github.com/TransportKrausPoloTeam/Meteo/blob/master/meteo/meteo/DataInputs/DataSetter.cs#L71

Przy nowoczesnych IDE odchodzi się od takich komentarzy, choć to jest kwestia estetyki. Dla mnie akurat takie coś jest brzydkie, dla innych standardowe. Nie krytykuję bo wiem, że są ludzie którzy to lubią. Uznałem, że warto wspomnieć. 

https://github.com/TransportKrausPoloTeam/Meteo/blob/master/meteo/meteo/DataInputs/DataSetter.cs#L74

Tu znów nazwa metody mówi coś zupełnie innego niż komentarz. Do tego można zrobić zwykły return new i nie mieszać nazw polskich z angielskimi. 

https://github.com/TransportKrausPoloTeam/Meteo/blob/master/meteo/meteo/DataInputs/DataSetter.cs#L108 ?? 

https://github.com/TransportKrausPoloTeam/Meteo/blob/master/meteo/meteo/DataOutputs/DataDisplayer.cs (ostatni commit ;[[[[[) :D

O ile "dr" można zrozumieć w lokalnym zakresie tej metody, to tyle nie mam pojęcia jak mam rozumieć "q" i czym jest "a".

https://github.com/TransportKrausPoloTeam/Meteo/blob/master/meteo/meteo/DataOutputs/DataExporter.cs#L24-L28 Może pętelka? Nie wiem czy to skończony kawałek kodu, ale jeżeli tak, to można spokojnie tu pętlę walnąć. Ogólnie zastanawiałbym się nad rozbiciem tej metody na nieco mniejsze. Ale to tylko zastanowienie - nic na siłę.

Poniżej widzę taką samą metodę. Tu jest Export to excell a poniżej "to doc". Od razu człowiek widzi w głowie "Strategy" - warto zastosować - dzięki temu na przykład z łatwością kiedyś dodacie eksport do csv, pdf czy html. 

Poza tym druga metoda na pewno jest zbyt długa. Jej złożoność cyklometryczna na stówę jest powyżej 10, a na 50% powyżej 20 punktów. To zły znak. Długie metody oznaczają ukryte błędy. Praktycznie każdy fragment kodu oddzielony komentarzami można ze spokojem wyekstrachować do oddzielnych metod. Będzie to dużo czytelniejsze:

createDocument, addHeader, addFoot, addContent(albo set albo change), createTable(x, y) -- coś tego typu będzie równie łatwe i komentarze staną się zbędne. Poza tym małe metody można użyć również w innych miejscach. Całej tej metody nie użyłbyś raczej w wielu miejscach poza tym.

https://github.com/TransportKrausPoloTeam/Meteo/blob/master/meteo/meteo/DataProcessing/FunctionsClasses/MatrixesOperations.cs#L37-L43

private static bool canMultiply(double [,] leftMatrix, double[,] rightMatrix)
        {
            return (leftMatrix.GetLength(1) == rightMatrix.GetLength(0)) ? true : false;
        }

Zadziała to w C#? :)

https://github.com/TransportKrausPoloTeam/Meteo/blob/master/meteo/meteo/DataProcessing/FunctionsClasses/MatrixesOperations.cs#L50-L57 To się zrobi jedną pętlą for - naprawdę :)

https://github.com/TransportKrausPoloTeam/Meteo/blob/master/meteo/meteo/DataProcessing/FunctionsClasses/Polynomial.cs

Brzydka praktyka - Nazywajmy zmienne tak, aby same za siebie mówiły co robią. Majac tak duży program będzie Ci się chciało szukać komentarza, który opisuje co robi dana zmienna?

https://github.com/TransportKrausPoloTeam/Meteo/blob/master/meteo/meteo/DataProcessing/DataProcessor.cs rozumiem że nie jest to jeszcze skończone

https://github.com/TransportKrausPoloTeam/Meteo/blob/master/meteo/meteo/DataProcessing/MatrixesOperations.cs#L37-L43 znasz już sztuczkę z tym :) 

Swoją drogą widzę kod tej klasy i chyba widziałem go gdzieś wcześniej - to oznacza duplikację w kodzie. Pomyślcie coś o tym. 

https://github.com/TransportKrausPoloTeam/Meteo/blob/master/meteo/meteo/DataProcessing/Polynomial.cs to też już widziałem. 

https://github.com/TransportKrausPoloTeam/Meteo/blob/master/meteo/meteo/DatabaseClasses/PocoClasses/MeteoStation.cs#L14 ?? :P

https://github.com/TransportKrausPoloTeam/Meteo/blob/master/meteo/meteo/DatabaseClasses/DataContainer.cs#L28-L41

Wcale nie ma takiej pewności, jednak powinno się sprawdzić a nie przyjmować, że tak jest. Miejsce potencjalnego buga. 

https://github.com/TransportKrausPoloTeam/Meteo/blob/master/meteo/meteo/DatabaseClasses/DataContainer.cs#L45-L51 Tu else w sumie nie jest potrzebny. Skoro if coś jeb wyjątkiem to i tak funkcja przestanie się wykonywać po tym. Można spokojnie tą asercje zostawić w ifie a blok else wywalić (zostawiając ciało oczywiście - bo przecież musi się coś wykonać)

https://github.com/TransportKrausPoloTeam/Meteo/blob/master/meteo/meteo/DatabaseClasses/DataContainer.cs#L68-L85 zastanawiam się, czy nie da się tego jakoś krócej zrobić. Może się nie da. Jeżeli nie to zastanawiam się czy nie powinno się tego rozdrobnić na mniejsze metodki. To tylko spekulacje.

https://github.com/TransportKrausPoloTeam/Meteo/blob/master/meteo/meteo/DatabaseClasses/DataRepository.cs jeśli to ma związek z wzorcem repository, to muszę powiedzieć że jest on źle zaimplementowany. Repository powinien posiadać metody: save, findByCośtam, ewentualnie persist. Jeśli będzie się coś chciało w repozytorium odczytywać to ogólnie jest coś takiego jak Read Model, a gdy zapisywać to jest Write Model. Do tego dochodzi Unit of Work. Jednak nie wiem czy przy tej aplikacji nie byłoby to przesadą. Widzę, że dbacie o kod i szukacie raczej dobrych rozwiązań - stąd tu moja dygresja. 

https://github.com/TransportKrausPoloTeam/Meteo/blob/master/meteo/meteo/DataExporter.cs to już widziałem gdzieś - duplikacja?

https://github.com/TransportKrausPoloTeam/Meteo/blob/master/meteo/meteo/GridPomiary.xaml.cs chyba najbrzydszy kod w całym programie. Bardzo duża złożoność, wiele zagnieżdżanych ifów - pomyślcie o refaktoryzacji bo tu jest co najmniej kilka potencjalnych bugów. Do tego widać ogromną duplikację wewnątrz samej klasy. 

https://github.com/TransportKrausPoloTeam/Meteo/blob/master/meteo/meteo/DodawanieStacji.xaml.cs#L74 nie wiem czy to kluczowa metoda bo możliwe, że gdzieś w modelu może była metoda usuwania. Jednak gdy usuwamy jakiś obiekt to warto aby się upewnić czy on istnieje. Tak samo przy edycji.

 

---

Ogólnie dobra robota. Jest kilka pierdółek na które zwróciłem uwagę. Warto zostawić chociaż trochę abstrakcji na bazę danych. A kto wie, może przejdziecie też na mobilki, na web? Na cokolwiek. Tu zawsze się przyda.

Widać starania o dobrze napisany kod, ale widać też miejsca, w których albo jeszcze nie było refaktoru, albo po prostu się nie chciało.

Warto też zrobić wartewkę API, w której wykorzysta się CQRS - dzięki temu całe gui będzie niezależne. Nie jest to konieczność ale taki feature. Przy okazji kod zyska kilka dodatkowych klas, z których będzie trzeba się nauczyć korzystać.

Sprawa odnośnie testów - jest ich bardzo mało. Nie wszystkie sprawdzają to, co powinny. Poza tym one pokazują, w jaki sposób korzysta się z "API" waszej aplikacji - widać że nie jest to wygodne. Stąd moja wzmianka o CQRS.

---

Dzięki serdeczne za taką rzeczową opinię. :) Chwilowo doturlamy ten projekt do końca tak, jak potrafimy, a po sesji przyjdzie czas na wnioski. :)

Answer 2

Co prawda nie znam składni C# czy .NETa, ale polecam założyć konto na githubie i tam wrzucać takie projekty, a z czasem je tylko aktualizować - fajna sprawa, szczególnie jeżeli program jest spory i trzeba go wklejać 20 razy na jakieś pastebiny czy wklejki.

Pozdrawiam ;)

Comments

Cała apka jest na gicie, ale wstyd pokazać, bo mamy straszny bałagan w repo, nie robię tej apki samemu, a to jest jeden z pierwszych zespołowych projektów, w jakich biorę udział, więc ciężko ustalić jakąś wspólną konwencję :) Po drugie, korzystamy z gita od niedawna i tak naprawdę go nie znamy należycie dobrze, stąd chaos.

 

Oczywiście zapraszam, z czasem będę tam wrzucać inne projekty.

https://github.com/Surykat

Powyższy plan dotyczy aplikacji Meteo :)

Również pozdrawiam. :)

---

Rozumiem, w takim razie powodzenia w kodowaniu ;)

http://pcottle.github.io/learnGitBranching/
Polecam jakbyś chciał się podszkolić z gita.

---

Ale fajne! :) Dzięki za link.

Answer 3

1. Nazwa zmiennych z wielkich liter nie szczególnie jest czytelne. Przyjąłbym jakąś konwencję np:. https://msdn.microsoft.com/en-us/library/ff926074.aspx

2. Tablice wielowymiarowe "double[,] matrix" raczej rzadko się używa, częściej używa się jagged arrays "double[][] matrix". Nie wiem czy to źle. Plus za znalezienie zastosowania tych tablic.

3. // Pośredniczy między aplikacją a bazą danych public class DataContainer. Nie wiem czy nie będzie upierdliwe w czasie rozwoju aplikacji wszystko wykonywać przez takie "proxy". Ja kiedyś też tak próbowałem robić i po jakimś czasie zrezygnowałem, bo pożytek żaden a niepotrzebnie komplikuje kod. Z jednej strony to fajne bo daje poczucie "mogę w przyszłości zmienić framework gadający z bazą", z drugiej strony, jeśli framework jest dojrzały (np. EF) to nie będziesz zmieniał frameworka, bo nie będzie takiej potrzeby. Tutaj używasz "meteo" - nie znam. Ja bym nie tworzył dodatkowej warstwy pomiędzy bazą danych a aplikacją. Ale to moje zdanie - miej na uwadze, że mogę się mylić, bo nie znam specyfiki Twojego projektu.

4. DataContainer- http://wklej.org/id/1913524/
DataRepository- http://wklej.org/id/1913524/

mają taki sam link

5.

public void ExportToDoc()
        {

        }

Jaa bym mimo wszystko wyrzucał NotImplementedException lub wyraźnie oznaczył w kodzie, że to trzeba dokończyć, tak na wszelki wypadek aby później o tym nie zapomnieć.

EDIT1:

6. Jeśli już piszesz wszystko po angielsku, to komentarze i doc stringi (/// ) też pisałbym w tymże języku - to tylko takie moje purystyczne dyrdymały...

EDIT2:

7. BTW. przestrzenie też powinny raczej być z wielkiej litery np. zamiast "namespace meteo.DatabaseClasses" bym użył "namespace Meteo.DatabaseClasses". Ale to jak wcześniej pisałem zależy od przyjętej konwencji.

Comments

1. Których zmiennych? Jeśli masz na myśli właściwości, to piszę tak, bo tak mnie nauczył wykładowca. :)

2. Jagged są czytelniejsze, ale zacząłem pisać te operacje na macierzach używając takiej formuły, to zostalem przy niej. I tak okazało się, że jeśli chce przeciążyć operator, musiałem pójść poziom wyżej i zamknąć macierze w klasie niestatycznej, tam trochę trzeba popoprawiać, bo jest parę nieścisłości.

3. Pomyślałem właśnie o rozwoju w sensie, gdybym chciał dodać jakieś źródło danych kolejne, to wystarczy je sprzęgnąć z DataContainerem. Czasem nie wiem, czy nie przeginam z tymi rozwiązaniami, ale daje mi to poczucie porządku. :) Meteo to nazwa projektu, do połączenia z bazą używam Entity Framework właśnie. ;)

Dziękuję ci za opinię. :)

---

1. Np w "public class DataProcessor" "Values"

//Nie znam linq, nie wiem czy to jest dobrze
        private List<double> GetValues(List<Measurement> data)
        {
            var Values = from measurement in data
                         select measurement.Value;
            return Values.ToList();
        }

2. Ok. Ja nie mówię, czy jedno jest lepsze od drugiego. Po prostu dla mnie jest to trochę egzotyczne - świadczy tylko o moim małym doświadczeniu :)

3. Tutaj jak uważasz, jeśli nie czujesz dyskomfortu z tego powodu to OK :)

---

1. Popatrz no, faktycznie. :)

2. Wiadomo, to bez znaczenia, ważne, że spełnia swoje zadanie. W następnym projekcie, dla treningu użyję tablic jagged. :)

3. Ważne, żeby zamysł, "po co to jest", był klarowny dla innych- od kiedy kodzę coś z kimś, doświadczam, jak to jest ważne :)

PS Poprawiłem link.