grofab95/salarycalculator

Simple application to calculate salary. Different versions: console, win-forms and web-api.

salarycalculator's Issues

Testy

Dodać brakujące testy i poprawić istniejące.

Zła nazwa metody

https://github.com/grofab95/KalkulatorWynagrodzen/blob/3fa6576927dee8672ca27e7bc61ae26e7deef2c7/KalkulatorWynagrodzen/StringParser.cs#L11

Jak wcześniej - to jest po prostu ParseToInt.

Nowa nazwa klasy: StringToNumbersConverter albo Parser.

Pusta Linia

https://github.com/grofab95/KalkulatorWynagrodzen/blob/fa029b8ecbe073dc3313510478b95036250bbf24/SalaryCalculator.Dekstop/MonthConfigEditorV2Window.cs#L85-L87

Parser.cs -> Desktop ma referencje do konsoli.

https://github.com/grofab95/KalkulatorWynagrodzen/blob/fa029b8ecbe073dc3313510478b95036250bbf24/SalaryCalculator.Dekstop/CalculatorMainWindow.cs#L30

1. Parser jest z projektu Console. Przez to, że dodałeś referencję do niego, jak zrobisz builda to wypluje Ci w bin obie aplikacje. Parser.cs powinien zostać wyniesiony do innego projektu w tym wypadku - Np do ParsingUtilities. W projektach w pracy często znajdziesz takie nazwy jak "Common", "Utils", "Shared", "Tools" - to jest straszne. W ogóle nie wiesz czego można się spodziewać w takich projektach, dlatego warto być bardziej precyzyjnym.

Generowanie kontrolek

https://github.com/grofab95/KalkulatorWynagrodzen/blob/fa029b8ecbe073dc3313510478b95036250bbf24/SalaryCalculator.Dekstop/MonthConfigEditorV2Window.cs#L22

Niby spoko ale wiemy, że miesięcy nie ubędzie ani nie przybędzie, dlatego ogólnie jestem bardziej za pierwszą wersją, ale zostaw. Ta też jest ok ale można trochę lepiej.

Zmienne w klasie:

private readonly System.Drawing.Font _defaultMonthLabel = new System.Drawing.Font("Microsoft Sans Serif", 14F, System.Drawing.FontStyle.Regular, System.Drawing.GraphicsUnit.Point, 238);
private readonly Padding _defaultPadding = new Padding(0, 1, 0, 6);

Ciało metody:
...

        var monthsLabels = Enumerable
            .Range(1, 12)
            .Select(index => new Label
            {
                Text = _monthsWorkingHours[index].ToString(),
                Font = _defaultMonthLabel,
                Margin = _defaultPadding
            });
        foreach (var monthsLabel in monthsLabels)
        {
            flowLayoutPanel1.Controls.Add(monthsLabel);
        }

Myślę że tak jest czytelniej. Są dwie iteracje ale przy tej liczbie elementów to nie ma znaczenia, wcześniej było gorzej - przy każdej iteracji pętli tworzyłeś nowy obiekt Font i Padding chociaż jest taki sam dla każdej iteracji ;)

Zbędny projekt

usuń projekt SalaryCalculator z repo.

Magic string

https://github.com/grofab95/KalkulatorWynagrodzen/blob/fa029b8ecbe073dc3313510478b95036250bbf24/SalaryCalculator.Dekstop/CalculatorMainWindow.cs#L100

Do takich rzeczy jak już to się robie Enuma. Np:
public enum ConfigurationWindowMode {
FixInvalidConfiguration
EditConfiguration
}

Pusta linia

https://github.com/grofab95/KalkulatorWynagrodzen/blob/fa029b8ecbe073dc3313510478b95036250bbf24/SalaryCalculator.Dekstop/MonthConfigEditorV2Window.cs#L18-L20

Nie można kopiować raportu do schowka

Należy użyć innej kontrolki. TextArea czy jak to tam było w tych formsach i ustawić ją na readonly żeby nie można było w niej pisać.

1 linijka zmiast 3 w testach :)

https://github.com/grofab95/KalkulatorWynagrodzen/blob/3fa6576927dee8672ca27e7bc61ae26e7deef2c7/Console.Tests/UserInputConsoleReaderTests.cs#L13-L17

Assert.Throws(() => StringParser.ParseToDoubleReadWorkedHours(userInput))

FormatException -> nazwałbym to -> "InvalidFormat"
Poza tym w nazwie testu jet Throws_InvalidOperationException a rzuca FormatException. Kłamiesz ;>

Zduplikowana metoda

https://github.com/grofab95/KalkulatorWynagrodzen/blob/3fa6576927dee8672ca27e7bc61ae26e7deef2c7/KalkulatorWynagrodzen/StringParser.cs#L7-L10

Te dwie metody są identyczne :) Tylko inaczej się nazywają. Jedna jest do usunięcia 🙃

Usprawnienie

https://github.com/grofab95/KalkulatorWynagrodzen/blob/fa029b8ecbe073dc3313510478b95036250bbf24/SalaryCalculator.Dekstop/CalculatorMainWindow.cs#L30-L52

Dużo się tu dzieje, możemy trochę ulepszyć (iteracyjne ulepszanie to dobra praktyka inżynierska), Proponuję wprowadzenie w projekcie SalaryCalculator nowych klas, które będzą trzymadełkiem dla obliczeń i współczynników które posłużą do ich otrzymania. Np MonthSalaryReport. Będzie ona miała Property;

• Factors Factors
• Summary Summary

Powyżej są 2 nowe klasy. CalculationFactors z propertami:

• WorkedMonth
• HourlyFee
• WorkedHours

CalculationSummary:

• OverHoursAmount
• OverHoursGrossINcome
• OverHoursNetIncome
• TotalGrossIncome
• TotalNetIncome
  czyli wszystko to co przyjmuje report builder. Zgodnie z zasadami CleanCode (taka książka na później) dobra metoda oprócz dobrej nazwy i małego rozmiaru przyjmuje idealnie 0, 1 parametrów, czasami 2 są uzasadnione a 3 wymagają już wytłumaczenia. Więcej jest wręcz niedoposzczalna (to jest już coś co trzeba wyćwiczyć ale możesz mieć to z tyłu głowy, na razie zbytnio się nie przejmując).

Następnie Calculate_Button_Click można trochę rozdzielić. Ja bym to widział trochę tak

private void Calculate_Button_Click(object sender, EventArgs e) {
var factors = new Factors(
workedHours = Parser.ParseStringToDouble(WorkedHours_TextBox.Text_
...
);
var summary = CalculateSummary(factors);
var monthSalaryReport = new MonthSalaryReport(factors, summary);
var textReport = new SimpleTextReportBuilder(_monthsWorkingHours).BuildMonthlyReport(monthSalaryReport );
new CalculatorReportWindow(report).ShowDialog();
}

Jakoś tak - i będzie z 9 linijek w metodzie. Może uda Ci się coś usprawnić po drodze. Oczywiście będzie trzeba poprawić trochę apkę konsolową ;) Ale to powinny być proste zmiany - masz testy także możesz być pewien że nic nie zepsujesz po drodze (testy też będzie trzeba poprawić).

InputValidationsTests - brakuje testów.

https://github.com/grofab95/KalkulatorWynagrodzen/blob/3fa6576927dee8672ca27e7bc61ae26e7deef2c7/SalaryCalculator.Tests2/InputValidationsTests.cs#L11

Brakuje testów, które sprawdzają czy walidacja działa. Sprawdzane jest tylko czy rzucane są wyjątki.

Złe nazwy projektów

KalkulatorWynagrodzen -> SalaryCalculator.Console
    Console.Tests -> SalaryCalculator.Console.Tests

po zmianie nazwy projektów upewni się, że wszędzie są dobre namespaces i sprawdź czy default namespace też się zmienił. W razie czego zmień ręcznie (znajdziesz na nacie - how to set project default namespace in visual studio)

literówka

https://github.com/grofab95/KalkulatorWynagrodzen/blob/3fa6576927dee8672ca27e7bc61ae26e7deef2c7/SalaryCalculator.Tests2/InputValidationsTests.cs#L25

powinno być monthNumber ;)

Zbędny using

https://github.com/grofab95/KalkulatorWynagrodzen/blob/fa029b8ecbe073dc3313510478b95036250bbf24/SalaryCalculator.Dekstop/MonthConfigEditorV2Window.cs#L2

Zakomentowany kod

https://github.com/grofab95/KalkulatorWynagrodzen/blob/3fa6576927dee8672ca27e7bc61ae26e7deef2c7/SalaryCalculator.Tests2/MonthsWorkingHoursTests.cs#L12-L28

usuń zakomentowany kod

Nazwa projektu SalaryCalculator.Tests2

SalaryCalculator.Tests2
wywal tą dwójkę ;)

Brakuje testów

https://github.com/grofab95/KalkulatorWynagrodzen/blob/3fa6576927dee8672ca27e7bc61ae26e7deef2c7/Console.Tests/UserInputConsoleReaderTests.cs#L7

Nie ma testów które sprawdzają czy kod działa, są tylko takie które sprawdzają czy są rzucane wyjątki.

Zła nazwa metody

https://github.com/grofab95/KalkulatorWynagrodzen/blob/3fa6576927dee8672ca27e7bc61ae26e7deef2c7/KalkulatorWynagrodzen/StringParser.cs#L7-L8

Jak dla mnie to tu jest tylko ParseStringToDouble - dlaczego WorkedHours? WorkedHours jest później jak jest sprawdzanie czy ktoś nie pojechał z ich ilością.

Zły typ zmiennej

https://github.com/grofab95/KalkulatorWynagrodzen/blob/fa029b8ecbe073dc3313510478b95036250bbf24/SalaryCalculator.Dekstop/CalculatorMainWindow.cs#L86

Jakie 0, jakie 1? Chyba chciałeś tak;
var isConfigurationValid = true / false

zbędna linijka

https://github.com/grofab95/KalkulatorWynagrodzen/blob/fa029b8ecbe073dc3313510478b95036250bbf24/SalaryCalculator.Dekstop/CalculatorMainWindow.cs#L32-L33

To jest tak proste że mogło być razem.