Rozwój aplikacji kalkulator

1

Zabrałem się za projekt typu kalkulator. Niby proste, a jednak...

Na chwilę obecną mam wersję roboczą najprostszego wariantu, kod poniżej.
Czy jest to czytelne, a może przekombinowałem? Chodzi mi o to, czy przyjąłem dobrą koncepcję.

<!DOCTYPE html>
<html lang="pl">

<head>
    <meta charset="UTF-8">
    <meta name="viewport" content="width = device-width, initial-scale = 1.0, shrink-to-fit = no">
    <title>Kalkulator</title>
    <link rel="stylesheet" href="kalkulator.css">
    <script src="kalkulator.js" defer></script>
</head>

<body>
    <table id = "calculator">
        <tr>
            <td colspan = "5" id = "calculatorScreen"></td>
        </tr>
        <tr>
            <td class = "numberKey">7</td>
            <td class = "numberKey">8</td>
            <td class = "numberKey">9</td>
            <td class = "operationKey">/</td>
            <td class = "functionKey">C</td>
        </tr>
        <tr>
            <td class = "numberKey">4</td>
            <td class = "numberKey">5</td>
            <td class = "numberKey">6</td>
            <td class = "operationKey">*</td>
            <td class = "functionKey">+/-</td>
        </tr>
        <tr>
            <td class = "numberKey">1</td>
            <td class = "numberKey">2</td>
            <td class = "numberKey">3</td>
            <td class = "operationKey">-</td>
            <td rowspan = "2" class = "functionKey">=</td>
        </tr>
        <tr>
            <td colspan = "2" class = "numberKey">0</td>
            <td class = "functionKey">.</td>
            <td class = "operationKey">+</td>
        </tr>
    </table>
</body>
</html>
class Calculation
{
    firstNumber = "";
    gotFirstNumber = false;
    secondNumber = "";
    gotSecondNumber = false;
    operation = "";
    gotOperation = false;
    score = 0;
    gotScore = false;

    screen = document.getElementById('calculatorScreen');

    // obsługa klawiszy z cyframi -> SELECT NUMBER
    selectNumber(e)
    {
        let number = e.target.textContent;
        if (!this.gotFirstNumber)
        {
            this.firstNumber += number;
            this.firstNumber = this.limitZeros(this.firstNumber);
            this.displayCalculation();
        }
        if ((this.gotFirstNumber) && (!this.gotSecondNumber))
        {
            this.secondNumber += this.limitZeros(number);
            this.secondNumber = this.limitZeros(this.secondNumber);
            this.displayCalculation();
        }
    }
    // obsługa klawiszy z operacjami matematycznymi -> SELECT OPERATION
    selectOperation(e)
    {
        if ((this.checkIfEntered(this.firstNumber)) && (!this.gotOperation))
        {
            this.operation = e.target.textContent;
            this.gotFirstNumber = true;
            this.gotOperation = true;
            this.displayCalculation();
        }
    }
    displayCalculation()
    {
        if (!this.gotScore)
        {
            this.screen.textContent = this.firstNumber + " " + this.operation + " " + this.secondNumber;
        }
    }
    // obsługa klawiszy funkcyjnych -> SELECT FUNCTION
    selectFunction(e)
    {
        let functionKey = e.target.textContent;
        switch (functionKey) 
        {
            case 'C':
                this.clearScreen();
                break;
            case '=':
                this.displayScore();
                break;
            case '.':
                this.addComma();
                break;
            case '+/-':
                this.addMinus();
                break;
        }        
    }
    // case 'C':
    clearScreen()
    {
        this.resetCalculation();
        this.screen.textContent = "";
    }
    resetCalculation()
    {
        this.firstNumber = "";
        this.gotFirstNumber = false;
        this.secondNumber = "";
        this.gotSecondNumber = false;
        this.operation = "";
        this.gotOperation = false;
        this.score = 0;
        this.gotScore = false;
    }
    // case '='
    displayScore()
    {
        if (this.checkIfEntered(this.secondNumber))
        {
            this.calculateScore();
            this.gotScore = true;
            this.gotSecondNumber = true;
            this.screen.textContent = this.score;
        }
    }
    calculateScore()
    {
        let X = parseFloat(this.firstNumber);
        let Y = parseFloat(this.secondNumber);
        switch (this.operation) 
        {
            case '+':
                this.score = X + Y;
                break;
            case '-':
                this.score = X - Y;
                break;
            case '*':
                this.score = X * Y;
                break;
            case '/':
                (Y == 0) ? (this.score = "WTF?") : (this.score = X / Y);
                break;
        }
    }
    // case '.'
    addComma()
    {
        let numberToCheck;
        (this.gotOperation) ? (numberToCheck = this.secondNumber) : (numberToCheck = this.firstNumber);
        if ((this.checkIfEntered(numberToCheck)) && (this.checkIfMinus(numberToCheck)))
        {
            (this.gotOperation) ? this.secondNumber = this.puttingComma(this.secondNumber) : this.firstNumber = this.puttingComma(this.firstNumber);
            this.displayCalculation();
        }
    }
    puttingComma(number)
    {
        return (number.includes('.')) ? (number = number.replace(".","")) : (number += ".");
    }
    // case '+/-'
    addMinus()
    {
        (this.gotOperation) ? this.secondNumber = this.puttingMinus(this.secondNumber) : this.firstNumber = this.puttingMinus(this.firstNumber);
        this.displayCalculation();
    }
    puttingMinus(number)
    {
        return (number.includes('-')) ? (number.slice(1)) : (number = "-" + number);
    }
    // metoda pomoocnicza - pusty (string) number?
    checkIfEntered(number)
    {
        let numberOK = false;
        (number.length == 0) ? (numberOK = false ) : (numberOK = true);
        return numberOK;
    }
    // metoda pomoocnicza - ogranicz liczbę zer
    limitZeros(number)
    {
        let numberReturned = number;
        if ((this.checkIfEntered(number)) && (number[1] == "0"))
        {
            numberReturned = numberReturned.slice(1);
        }
        else
        {
            numberReturned = number;
        }
        return numberReturned;
    }
    // metoda pomoocnicza - pierwszy znak to minus?
    checkIfMinus(number)
    {
        let numberOK = false;
        if ((number.length == 1) && (number == '-'))
        {
            numberOK = false;
        }
        else
        {
            numberOK = true;
        }
        return numberOK; 
    }
    // MAIN CASE
    decodeKey(e)
    {   
        let keyClass = e.target.className;
        switch (keyClass) 
        {
            case 'numberKey':
                this.selectNumber(e);
                break;
            case 'operationKey':
                this.selectOperation(e);
                break;
            case 'functionKey':
                this.selectFunction(e);
                break;
        }
    }
}

const currentCalculation = new Calculation;
const calculator = document.getElementById('calculator').addEventListener('click', e => currentCalculation.decodeKey(e));
4

No ogólnie projekt amatorski dosyć; sporo możnaby w nim poprawić.

  1. Unikaj tabelek do ogarniania layout'u. Użyj raczej grid albo flexbox: https://css-tricks.com/snippets/css/a-guide-to-flexbox/
  2. Wsadziłeś wszystko do jednej klasy; maintainability tego raczej słabe.
  3. checkIfMinus(number)
    {
        let numberOK = false;
        if ((number.length == 1) && (number == '-'))
        {
            numberOK = false;
        }
        else
        {
            numberOK = true;
        }
        return numberOK; 
    }
    
    tragedia. Można:
    checkIfMinus(number) {
        return number.length !== 1 || number !== '-';
    }
    
  4. Masz pomieszaną logikę kalkulatora z wyświetlaniem.
  5. checkIfEntered(number) {
        let numberOK = false;
        (number.length == 0) ? (numberOK = false ) : (numberOK = true);
        return numberOK;
    }
    
    czy to nie jest przypadkiem to samo co to?
    checkIfEntered(number) {
        return number.length > 0;
    }
    
    Wiesz że możesz zwracać całe wyrażenia prawda? Nie tylko zmienne?
  6. To samo w limitZeros()
  7. Komentarze są u Ciebie do bani. Masz metodę checkIfMinus i zaraz obok niej komentarz pierwszy znak to minus?. Skoro tak, to czemu nie nazwisz funkcji isFirstCharacterMinus() i nie wywalisz komentarza?
  8. Zamiast przypinać do wszystkich elementów ten sam handler, i potem switchem po klasie sprawdzać który; to lepiej podpiąć różne handlery do różnych elementów.
0

Wsadziłeś wszystko do jednej klasy; maintainability tego raczej słabe.

Powiedzmy, że chciałem poćwiczyć tworzenie klas w tym języku. Jak rozumiem, lepiej by było zadeklarować każdą funkcję osobno w jednym pliku *.js. Tylko przy takiej koncepcji muszę użyć zmiennych globalnych...

   checkIfMinus(number)
   {
       let numberOK = false;
       if ((number.length == 1) && (number == '-'))
       {
           numberOK = false;
       }
       else
       {
           numberOK = true;
       }
       return numberOK; 
   }

tragedia. Można:

checkIfMinus(number) {
    return number.length !== 1 || number !== '-';
}

Owszem, można. Kwestia "tylko" doświadczenia :)

Masz pomieszaną logikę kalkulatora z wyświetlaniem.

Też mam takie wrażnie. Lepiej by było zrobić osobną funkcję to generowania stringa z działaniem,
a potem przekazać to do funkcji wyświetlającej. Czyli najpierw formatuję (przecinek, nadmiar zer itd).

    checkIfEntered(number) {
        let numberOK = false;
        (number.length == 0) ? (numberOK = false ) : (numberOK = true);
        return numberOK;
    }
    ```
      czy to nie jest przypadkiem to samo co to?
    ```
    checkIfEntered(number) {
        return number.length > 0;
    }
    ```
    Wiesz że możesz zwracać całe wyrażenia prawda? Nie tylko zmienne?

Zgoda, to ma sens. No chyba, że w przyszłości funkcję rozbuduję.

Komentarze są u Ciebie do bani. Masz metodę checkIfMinus i zaraz obok niej komentarz pierwszy znak to minus?. Skoro tak, to czemu nie nazwisz funkcji isFirstCharacterMinus() i nie wywalisz komentarza?

Jeżeli deklarowanie funkcji/metody o nazwie checkIfSomethingOrSomethingAndSomethingWhatever() jest ok, to zgoda.

Zamiast przypinać do wszystkich elementów ten sam handler, i potem switchem po klasie sprawdzać który; to lepiej podpiąć różne handlery do różnych elementów.

Co mi to da?

3
kosmonauta80 napisał(a):

Wsadziłeś wszystko do jednej klasy; maintainability tego raczej słabe.

Powiedzmy, że chciałem poćwiczyć tworzenie klas w tym języku. Jak rozumiem, lepiej by było zadeklarować każdą funkcję osobno w jednym pliku *.js. Tylko przy takiej koncepcji muszę użyć zmiennych globalnych...

I poćwiczyłeś w taki sposób że masz jedną, w której jest wszystko?

Klasy nie są dobre dlatego że "są", albo dlatego że są składnią języka, albo że mają konstruktor, atrybuty i inne takie gadanie. Używa się ich, ponieważ we wmiare dobry sposób pozwalają dzielić kod na kawałki; to jest ich cały zamysł. Jeśli nie używasz klas do dzielenia kodu na kawałki, to równie dobrze mógłbyś ich w ogóle nie używać.

Masz pomieszaną logikę kalkulatora z wyświetlaniem.

Też mam takie wrażnie. Lepiej by było zrobić osobną funkcję to generowania stringa z działaniem,
a potem przekazać to do funkcji wyświetlającej. Czyli najpierw formatuję (przecinek, nadmiar zer itd).

Nie, to też byłoby pomieszanie logiki z wyświetlaniem.

    checkIfEntered(number) {
        let numberOK = false;
        (number.length == 0) ? (numberOK = false ) : (numberOK = true);
        return numberOK;
    }
    ```
      czy to nie jest przypadkiem to samo co to?
    ```
    checkIfEntered(number) {
        return number.length > 0;
    }
    ```
    Wiesz że możesz zwracać całe wyrażenia prawda? Nie tylko zmienne?

Zgoda, to ma sens. No chyba, że w przyszłości funkcję rozbuduję.

A po co "rozbudować"? Nie. Jedyne dobrego co się z tą funkcją może stać to, możesz ją albo edytować, tak żeby działała inaczej; albo możesz ją usunąć i ewentualnie zastąpić inną. Jeśli chcesz rozbudować program to dodaj więcej małch funkcji, a nie powiększaj już istniejące.

Komentarze są u Ciebie do bani. Masz metodę checkIfMinus i zaraz obok niej komentarz pierwszy znak to minus?. Skoro tak, to czemu nie nazwisz funkcji isFirstCharacterMinus() i nie wywalisz komentarza?

Jeżeli deklarowanie funkcji/metody o nazwie checkIfSomethingOrSomethingAndSomethingWhatever() jest ok, to zgoda.

Masz tutaj PDF'a książki "Clean Code": https://pdf.helion.pl/czykov/czykov.pdf Zainteresuj się rozdziałem 4tym. Jak tylko go przeczytasz, to zrozumiesz że da się pisać krótkie, czytelne i opisowe funkcje, i nigdy nie zrobisz funkcji checkIfSomethingOrSomethingAndSomethingWhatever().

Zamiast przypinać do wszystkich elementów ten sam handler, i potem switchem po klasie sprawdzać który; to lepiej podpiąć różne handlery do różnych elementów.

Co mi to da?

W przykładzie który podałeś - nic, oprócz tego że pozbędziesz się switcha który i tak jest niepotrzebny. W normalnych aplikacjach większą kontrolkę nad UI i nad logiką, większe oddzielenie elementów od siebie; większą utrzymywalność na przyszłość; Ty nie masz testów ale mniejsze handlery są dużo bardziej testowalne niż większe; ogólnie same zalety i 0 wad takiego rozwiązania.

Jedyną wadą jest tylko to że amatorzy mogą nie widzieć róznicy, i przez to myślą że "jedno znaczy lepiej niż dużo". Ale to jest tak samo bliskie prawdy jak powiedzenie "jedna kupa z ubraniami jest lepsza niż ubrania pochowane na 5ciu półkach".

1

Wychodzi na to, że taką aplikację powinienem podzielić na około 20-30 małych podfunkcji. Czy to nie będzie przesadą?

0
kosmonauta80 napisał(a):

Wychodzi na to, że taką aplikację powinienem podzielić na około 20-30 małych podfunkcji. Czy to nie będzie przesadą?

No tak, dokładnie na to wychodzi, i nie, nie będzie to przesadą.

Napisałeś jakiś kod, żeby wykonać pewne działanie aplikacji, i ten kod musi się znaleźć w tej aplikacji tak czy tak. Pytanie tylko czy wrzucisz go na kupę ubrań, czy znajdziesz dla każdego kawałka odpowiednią małą funkcję. Dla mnie wybór jest oczywisty.

1
kosmonauta80 napisał(a):

Oczywiście co innego taki projekt, a co innego wielka aplikacja robiona przez cały zespół.

Nie specjalnie, bo robisz różne rzeczy, które na dłuższą metę nie są dobre i tylko przeszkadzają.

Nie możesz ich teraz naprawić sam, bo to wymaga dużo umiejętności, ale to co robisz w swoim projekcie to tak:

  • Starasz się odwzorować wygląd kodu do wyglądu aplikacji, dlatego np masz te switche. To jest sztuczne i niepotrzebne; spodziewam się że w większych aplikacjach zrobiłbyś to samo.
  • Łączysz widok z logiką, dlatego że na 99% nie zdajesz sobie do końca sprawy z tego który kod jest faktycznie widokiem a który logiką, więc nie możesz ich oddzielić nawet jakbyś chciał. To jakby daltonista miał posegregować ubrania względem koloru.
  • Używasz ternary operator (?:) w zasadzie nie wiadomo po co
  • Nie zauważyłeś że dodałeś tight-coupling do DOM'u w swojej klasie Calculator.
  • Przechowujesz stan i mutujesz go jako normalna częsć logiki

Jesli chcesz to mogę Ci powiedzieć jak to poprawić, i jak to zrobić dobrze; ale to wymagałoby tego że musiałbyś być skłonny zmienić swój program drastycznie.

1
kosmonauta80 napisał(a):

Więc od czego zacząć? Kod HTML może zostać? GRIDa zrobię, nie problem

@kosmonauta80: Moim zdaniem powinieneś zacząć od zrobienia klasy Calculator od nowa, która będzie czystą klasą JavaScript'ową i nie będzie widziała nic o żadnym UI i żadnym HTML i DOM'ie.

No bo co wiemy:

  1. Userzy mają podawać liczby i znaki wpisując je z klawiatury w gridzie. Czyli nie wpiszą od razu "173", tylko klikną klawisze 1, 7, 3.
  2. Mogą używać cyfr 1-9, znaku plus, minus, mnożenia i dzielenia. mogą też zmazać ostatni znak.
  3. W pewnym momencie klikną też klawisz =, i wtedy kalkulator ma pokazać liczbę.

Powinieneś zacząć od domeny biznesowej, czyli od klasy Calculator która nie wie nic o UI i na których można zrobić takie rzeczy;

const calc = new Calculator();
calc.type("1"); // wpisanie cyfry 1
calc.type("7"); // wpisanie cyfry 7
calc.type("3"); // wpisanie cyfry 3

calc.type("-"); // odjąć, albo możesz też calc.typeMinus(); wszystko jedno

calc.text(); // jak teraz zawołasz tą metodę to powinna zwrócić "173-"

calc.type("6");
calc.type("1");

calc.text(); // jak teraz zawołasz tą metodę to powinna zwrócić "173-61"

calc.delete();
calc.type("2");

calc.displayText(); // jak teraz zawołasz tą metodę to powinna zwrócić "173-62"

calc.result();  // policz wynik

calc.text(); // jak teraz zawołasz tą metodę to powinna zwrócić "111"

I ta klasa powinna działać tak że metoda .type() "dodaje" na koniec cyfrę lub znak, metoda .delete() cofa wpisaną liczbę (Albo nic, jeśli nie ma żadnej liczby), a text() zwraca aktualnie wyświetlany tekst. Z tego text możesz zrobić getter. Zauważ, że w tym kodzie nie powinno być żadnej logiki związanej z wyświetlaniem, żadnej z HTML'em ani domem.

Jak już to napiszesz, to dodanie pod to UI będzie banalne, po prostu dodasz sobie listenery które wołają odpowiednie metody tej klasy, a potem wyświetlają w labelce to co zwróci calc.dispalyText().

Od razu uprzedzam pytanie; większość nowicjuszy jak zobaczy taki kod to się złapie za głowie i powie, matko co to jest, po co robisz calc.type("1"); calc.type("7"); calc.type("3");, skoro mógłbyś calc.type("173")? Owszem, móglbyś, grybyś projektował interfejs programistyczny klasy (funkcje, metody, klasy). Ale Ty projektujesz interfejs użytkownika, a użytkonik będzie klikał liczby przyciskami, zawsze jedną na raz, więć musisz to wziąć pod uwagę.

Zacznij od takiej klasy.

0
class Calculator
{
    calculation = '';
    type(char)
    {
        this.calculation += char;
    }
    delete()
    {
        this.calculation = this.calculation.substring(0,(this.calculation.length-1));
    }
    text()
    {
        console.log(this.calculation);
    }
    result()
    {
        console.log(eval(this.calculation));
    }
}
1
kosmonauta80 napisał(a):

Alternatywa dla eval()?

Pozwól że przytoczę Ci Twój własny kod z postu wyżej.

calculateScore()
{
    let X = parseFloat(this.firstNumber);
    let Y = parseFloat(this.secondNumber);
    switch (this.operation) 
    {
        case '+':
            this.score = X + Y;
            break;
        case '-':
            this.score = X - Y;
            break;
        case '*':
            this.score = X * Y;
            break;
        case '/':
            (Y == 0) ? (this.score = "WTF?") : (this.score = X / Y);
            break;
    }
}

Bo zastanów się, co by to znaczyło że używasz eval(). Otóż znaczy to, ni mniej ni więcej, że Twoja implementacja polega na tym, że: Twój interfejs jest dokładnie taki sam jak język JavaScript. Że Twój kalkulator wspiera wyrażenie (1+2)*3 wtedy, i tylko wtedy gdy JavaScript wspiera to samo wyrażenie. To jest conajmniej proszenie się o kłopoty. Nie mówiąc już o dużo gorszym problemie evaluowania arbitralnego kodu.

Ten Twój nowy kod, to ma być dokładnie to samo co stary, tylko bez DOMu, bez HTML'a, bez dodatkowych zależności.

Mogę Ci napisać kod w PHP, coś jak pseudokod; a Ty możesz go przepisać na JavaScript; jak chcesz.

3

@kosmonauta80: Napisałem cały kalkulator w PHP za Ciebie (wybrałem PHP, żebyś nie mół sobie po prostu wziąć i skopiować, tylko żebyś mógł się wzorować). Napisałem też cały zestaw unit testów, które też mógłbyś przepisać np w Mocha, Jest, Ava czy jakiego tam rozwiązania używasz.

Zauważ, że w klasie Calculator nie ma nic o UI, żadnych echo, <br>, HTML'a, DOM'u, ani nic takiego. Jest tylko logika którą możesz sterować odpowiednimi metodami plus(), minus(), divide(), multiply(), calculate() i odczytać stan z text().

class Calculator
{
    /** @var ?string */
    private $firstOperand = null;

    /** @var ?string */
    private $operation = null;

    /** @var ?string */
    private $secondOperand = null;

    public function type(string $letter): void
    {
        if ($this->operation === null) {
            $this->firstOperand = ($this->firstOperand ?? '') . $letter;
        } else {
            $this->secondOperand = ($this->secondOperand ?? '') . $letter;
        }
    }

    public function plus(): void
    {
        $this->addOperation('+');
    }

    public function minus(): void
    {
        $this->addOperation('-');
    }

    public function multiply(): void
    {
        $this->addOperation('*');
    }

    public function divide(): void
    {
        $this->addOperation('/');
    }

    private function addOperation(string $operation): void
    {
        if ($this->secondOperand !== null) {
            $this->calculate();
        }
        $this->operation = $operation;
    }

    public function text(): string
    {
        if ($this->operation === null) {
            return $this->firstOperand ?? '0';
        }
        return $this->firstOperand . $this->operation . $this->secondOperand;
    }

    public function calculate(): void
    {
        $this->firstOperand = $this->operationResult($this->firstOperand, $this->secondOperand);
        $this->operation = null;
        $this->secondOperand = null;
    }

    private function operationResult(float $first, float $second): float
    {
        if ($this->operation === '+') {
            return $first + $second;
        }
        if ($this->operation === '*') {
            return $first * $second;
        }
        if ($this->operation === '/') {
            return $first / $second;
        }
        return $first - $second;
    }
}

class CalculatorTest extends TestCase
{
    /**
     * @test
     */
    public function shouldGetZeroAsFirstInput()
    {
        $this->assertSame('0', (new Calculator())->text());
    }

    /**
     * @test
     */
    public function shouldTypeOne()
    {
        // given
        $calculator = new Calculator();
        // when
        $calculator->type('1');
        // then
        $this->assertSame('1', $calculator->text());
    }

    /**
     * @test
     */
    public function shouldTypeOnePlus()
    {
        // given
        $calculator = new Calculator();
        // when
        $calculator->type('1');
        $calculator->plus();
        // then
        $this->assertSame('1+', $calculator->text());
    }

    /**
     * @test
     */
    public function shouldTypeOneMinus()
    {
        // given
        $calculator = new Calculator();
        // when
        $calculator->type('1');
        $calculator->minus();
        // then
        $this->assertSame('1-', $calculator->text());
    }

    /**
     * @test
     */
    public function shouldTypeOneMinusAfterPlus()
    {
        // given
        $calculator = new Calculator();
        // when
        $calculator->type('1');
        $calculator->plus();
        $calculator->minus();
        // then
        $this->assertSame('1-', $calculator->text());
    }

    /**
     * @test
     */
    public function shouldTypeTwoDigitNumber()
    {
        // given
        $calculator = new Calculator();
        // when
        $calculator->type('1');
        $calculator->type('2');
        // then
        $this->assertSame('12', $calculator->text());
    }

    /**
     * @test
     */
    public function shouldTypeTwoNumbers()
    {
        // given
        $calculator = new Calculator();
        $calculator->type('1');
        $calculator->type('2');
        $calculator->plus();
        // when
        $calculator->type('3');
        $calculator->type('4');
        // then
        $this->assertSame('12+34', $calculator->text());
    }

    /**
     * @test
     */
    public function shouldAdd()
    {
        // given
        $calculator = new Calculator();
        $calculator->type('4');
        $calculator->plus();
        $calculator->type('7');
        // when
        $calculator->calculate();
        // then
        $this->assertSame('11', $calculator->text());
    }

    /**
     * @test
     */
    public function shouldAddTwoNumbersAndTypePlus()
    {
        // given
        $calculator = new Calculator();
        $calculator->type('4');
        $calculator->plus();
        $calculator->type('7');
        $calculator->calculate();
        // when
        $calculator->plus();
        // then
        $this->assertSame('11+', $calculator->text());
    }

    /**
     * @test
     */
    public function shouldAddThreeNumbers()
    {
        // given
        $calculator = new Calculator();
        $calculator->type('4');
        $calculator->plus();
        $calculator->type('7');
        $calculator->calculate();
        // when
        $calculator->plus();
        $calculator->type('2');
        $calculator->calculate();
        // then
        $this->assertSame('13', $calculator->text());
    }

    /**
     * @test
     */
    public function shouldSubtract()
    {
        // given
        $calculator = new Calculator();
        $calculator->type('4');
        $calculator->minus();
        $calculator->type('7');
        // when
        $calculator->calculate();
        // then
        $this->assertSame('-3', $calculator->text());
    }

    /**
     * @test
     */
    public function shouldTypeMultiplication()
    {
        // given
        $calculator = new Calculator();
        $calculator->type('4');
        // when
        $calculator->multiply();
        $calculator->type('7');
        // then
        $this->assertSame('4*7', $calculator->text());
    }

    /**
     * @test
     */
    public function shouldTypeDivision()
    {
        // given
        $calculator = new Calculator();
        $calculator->type('21');
        // when
        $calculator->divide();
        $calculator->type('7');
        // then
        $this->assertSame('21/7', $calculator->text());
    }

    /**
     * @test
     */
    public function shouldMultiply()
    {
        // given
        $calculator = new Calculator();
        $calculator->type('4');
        $calculator->multiply();
        $calculator->type('7');
        // when
        $calculator->calculate();
        // then
        $this->assertSame('28', $calculator->text());
    }

    /**
     * @test
     */
    public function shouldDivide()
    {
        // given
        $calculator = new Calculator();
        $calculator->type('21');
        $calculator->divide();
        $calculator->type('7');
        // when
        $calculator->calculate();
        // then
        $this->assertSame('3', $calculator->text());
    }

    /**
     * @test
     */
    public function shouldCalculateAutomaticallyPlus()
    {
        // given
        $calculator = new Calculator();
        $calculator->type('21');
        $calculator->divide();
        $calculator->type('7');
        // when
        $calculator->plus();
        // then
        $this->assertSame('3+', $calculator->text());
    }

    /**
     * @test
     */
    public function shouldCalculateAutomaticallySubtract()
    {
        // given
        $calculator = new Calculator();
        $calculator->type('21');
        $calculator->divide();
        $calculator->type('7');
        // when
        $calculator->minus();
        // then
        $this->assertSame('3-', $calculator->text());
    }

    /**
     * @test
     */
    public function shouldResultRealValue()
    {
        // given
        $calculator = new Calculator();
        $calculator->type('1');
        $calculator->divide();
        $calculator->type('2');
        // when
        $calculator->calculate();
        // then
        $this->assertSame('0.5', $calculator->text());
    }

    /**
     * @test
     */
    public function shouldOperateOnRealValues()
    {
        // given
        $calculator = new Calculator();
        $calculator->type('1');
        $calculator->type('.');
        $calculator->type('5');
        $calculator->plus();
        $calculator->type('2');
        $calculator->type('.');
        $calculator->type('5');
        // when
        $calculator->calculate();
        // then
        $this->assertSame('4', $calculator->text());
    }
}
0
class Calculator
{
    firstOperand = null;
    secondOperand = null;
    operation = null;
    
    type(char)
    {
        if (this.operation === null)
        {
            this.firstOperand += char;
        }
        else
        {
            this.secondOperand += char;
        }
    }
    plus()
    {
        this.addOperation('+');
    }
    minus()
    {
        this.addOperation('/');
    }
    multiply()
    {
        this.addOperation('*');
    }
    divide()
    {
        this.addOperation('/');
    }
    addOperation(operation)
    {
        if (this.secondOperand !== null)
        {
            this.calculate();
        }
        this.operation = operation;
    }
    text()
    {
        return this.firstOperand + this.operation + this.secondOperand;
    }
    calculate()
    {
        this.firstOperand = this.operationResult(this.firstOperand, this.secondOperand)
        this.operation = null;
        this.secondOperand = null;
    }
    operationResult(X, Y)
    {
        if (this.operation == '+')
        {
            return X + Y;
        }
        if (this.operation == '-')
        {
            return X - Y;
        }
        if (this.operation == '*')
        {
            return X * Y;
        }
        if (this.operation == '/')
        {
            return X / Y;
        }
    }
}
0

@kosmonauta80: No liczyłem jednak że napiszesz to samemu, ale niech będzie.

Skoro logikę, powiedzmy że już masz, możesz dorobić do niego interfejs; strzelam że chcesz zrobić interfejs użytkownika.

  1. Najpierw zrób layout, używając flexbox lub grid;
  2. Potem dodaj <script>, ale w taki sposób żeby tam się znalazła jedna instancja new Calculator()
  3. Dodaj do buttonów zdarzenia, tak by wołałby calculator.type("1"), calculator.plus() oraz calcuator.result(), i w każdym z nich powinieneś wsadzić calculator.text() do pola w którym wyświetlasz tekst.

I to tyle :>

0

Nie tyle, bo jeszcze trzeba ogarnąć na przykład dzielenie przez 0, albo metodę delete().

Zastanawia mnie co będzie lepsze:

 Operand = null;

czy

 Operand = ' ';
1
kosmonauta80 napisał(a):

Nie tyle, bo jeszcze trzeba ogarnąć na przykład dzielenie przez 0, albo metodę delete().

Zastanawia mnie co będzie lepsze:

 Operand = null;

czy

 Operand = ' ';

No, tylko to nadal powinno być niezależne od UI, więc powinieneś to zmienić w swojej klasie Calculator. Możnaby to wyrazić takim unit testem:

/**
 * @test
 */
public function shouldCalculateAutomaticallySubtract()
{
    // given
    $calculator = new Calculator();
    $calculator->type('21');
    $calculator->divide();
    $calculator->type('0');
    // when
    $calculator->result(); // albo ewentualnie to może rzucić wyjątek
    // then
    $this->assertSame('Error', $calculator->text()); // albo inną asercje, zależy od tego co chcesz zrobić
}

Zależy jak zdecydujesz się zaprojektować interfejs.

PS: Tylko pamiętaj, że jeśli chcesz wspierać dzielenie przez zero, to musisz też ogarnąć np 0^0, oraz log(0) i inne niezdefiniowane operacje.

0

@kosmonauta80: Masz już coś?

1
kosmonauta80 napisał(a):

Zacząłem to dłubać. Całkiem tricky to się robi :>

Możesz edytować calculate()

operationResult(X, Y) {
    if (this.operation == '+')
    {
        return X + Y;
    }
    if (this.operation == '-')
    {
        return X - Y;
    }
    if (this.operation == '*')
    {
        return X * Y;
    }
    if (this.operation == '/')
    {
        if (Y === 0) {
           this.errorState = true;
           return X:
        }
        return X / Y;
    }
}

Tak żeby ustawiała errorState, i potem w text().

text() {
    if (this.errorState) {
         return "Error";
    }
    return this.firstOperand + this.operation + this.secondOperand;
}

Tylko pamiętaj żeby w konstruktoze ustawić errorState = false.
Wtedy jak spróbujesz policzyć dzielenie przez zero to text() zwróci słowo "Error", a UI nie będzie o tym nic wiedział, tak jak powinno być :)

0

@kosmonauta80: To nie jest trudne, weźmy trzy pierwsze, te które napisałem. Nie potrzebujesz żadnego frameworka do tego.

Mamy te moje testy:

/**
 * @test
 */
public function shouldGetZeroAsFirstInput()
{
    $this->assertSame('0', (new Calculator())->text());
}

/**
 * @test
 */
public function shouldTypeOne()
{
    // given
    $calculator = new Calculator();
    // when
    $calculator->type('1');
    // then
    $this->assertSame('1', $calculator->text());
}

/**
 * @test
 */
public function shouldTypeOnePlus()
{
    // given
    $calculator = new Calculator();
    // when
    $calculator->type('1');
    $calculator->plus();
    // then
    $this->assertSame('1+', $calculator->text());
}

W JS to mogłoby wyglądać jakoś tak

<script>
function assertEquals(expected, actual) {
  if (expected !== actual) {
    console.error(`Test failed! Expected: ${expected}, but ${actual} was given`);
  }
};

const tests = [
  function shouldGetZeroAsFirstInput() {
    assertEquals('0', new Calculator().text();
  },
  function shouldTypeOne() {
    // given
    const calculator = new Calculator();
    // when
    calculator.type('1');
    // then
    assertEquals('1', calculator.text());
  },
  function shouldTypeOnePlus() {
      // given
      const calculator = new Calculator();
      // when
      calculator.type('1');
      calculator.plus();
      // then
      assertEquals('1+', calculator.text());
  }
];

tests.forEach(test => test());
</script>

Dodajesz coś takiego do swojego kodu, patrzysz do konsoli i widzisz:
screenshot-20220310191848.png

Wystarczy że skopiujesz ten <script> i wsadzisz do siebie do kodu. Z dopisaniem kolejnych chyba sobie poradzisz? ;)

0

Oto całkiem sensownie działająca wersja. Celowo okrojona do minimum na tym etapie. Proszę o uwagi, a w międzyczasie przeanalizuję te testy.
https://github.com/radioactiveCode/calculator

3
kosmonauta80 napisał(a):

Oto całkiem sensownie działająca wersja. Celowo okrojona do minimum na tym etapie. Proszę o uwagi, a w międzyczasie przeanalizuję te testy.
https://github.com/radioactiveCode/calculator

Klasa Calculator miała być całkowicie pozbawiona logiki UI'a, a jednak dodałeś do niej:

display(field)
{
    field.textContent = this.text();
}

Proszę wywalić tą metode. textContent to jest atrybut DOM'u, a Calculator miał o niej nic nie wiedzieć.

2
kosmonauta80 napisał(a):

done

No tylko teraz masz duplikację w listenerach. Miałeś dobry pomysł, żeby schować to do wspólnej funkcji, ale ta funkcja nie powinna się znaleźć w Calculator.

Powinieneś to zrobić jakoś tak

class Calculator {
    // [...]
    
    text()
    {
        return this.firstOperand + this.operation + this.secondOperand;
    }
}

function print(calc, field)
{
    field.textContent = calc.text();
}

const calculator = new Calculator();
const display = document.getElementById('display');

document.getElementById('zero').addEventListener('click', () => {calculator.type(0); print(calculator, display);})
document.getElementById('one').addEventListener('click', () => {calculator.type(1); print(calculator, display);})
document.getElementById('two').addEventListener('click', () => {calculator.type(2); print(calculator, display);})
// ...

PS: Staraj sie nie robić if (this.operation == '/'), raczej użyj === albo !==.

1

@kosmonauta80: Co do zduplikowanych addEventListener w przypadku cyfr. Czy słyszałeś o dataset?

Uwspólnienie wybierania cyfr

Zabierz id z przycisków cyfr, a dodaj do nich data-digit, tak:

<div id="calculator">
  <div id="display"></div>
  
  <div class="numberKey" data-digit="7">7</div>
  <div class="numberKey" data-digit="8">8</div>
  <div class="numberKey" data-digit="8">9</div>
  <div class="operationKey" id="divide">/</div>
  <div class="functionKey" id="reset">DEL</div>

Dzięki temu będziesz mógł złapać wszystkie elementy z klasą "numberKey" i dodać do nich ten sam listener, który używa data-digit:

Array.from(document.getElementsByClassName("numberKey")).forEach(button => {
   button.addEventListener('click', () => {
     calculator.type(button.dataset.digit);
     print(calculator, display);
   });
});

Skrócenie kodu elementów UI operacji

Co do pozostałych listenerów, nie da się tego za mądrze uwspólnić, ponieważ mają inne interfejsy, najlepsze co można zrobić to:

addListener(document.getElementById('plus'), () => calculator.plus());
addListener(document.getElementById('minus'), () => calculator.minus());
addListener(document.getElementById('multiply'), () => calculator.multiply());
addListener(document.getElementById('divide'), () => calculator.divide());
addListener(document.getElementById('equal'), () => calculator.calculate());
addListener(document.getElementById('reset'), () => calculator.delete());

function addListener(domElement, listener) {
  domElement.addEventListener('click', () => {
    listener();
    print(calculator, display);
  });
}

Nie ma sensu dodawać nazw funkcji do dataset, i nie ma sensu tez przekazywać samego id zamiast domElement.

0

Można też tak:

[...numberKeys].forEach((key) =>
{
    key.addEventListener('click', () =>
    {
        currentCalculation.type(key.dataset.digit); 
        print(screen, currentCalculation);
    });
});
0
TomRiddle napisał(a):

@kosmonauta80: To nie jest trudne, weźmy trzy pierwsze, te które napisałem. Nie potrzebujesz żadnego frameworka do tego.

Mamy te moje testy:

/**
 * @test
 */
public function shouldGetZeroAsFirstInput()
{
    $this->assertSame('0', (new Calculator())->text());
}

/**
 * @test
 */
public function shouldTypeOne()
{
    // given
    $calculator = new Calculator();
    // when
    $calculator->type('1');
    // then
    $this->assertSame('1', $calculator->text());
}

/**
 * @test
 */
public function shouldTypeOnePlus()
{
    // given
    $calculator = new Calculator();
    // when
    $calculator->type('1');
    $calculator->plus();
    // then
    $this->assertSame('1+', $calculator->text());
}

W JS to mogłoby wyglądać jakoś tak

<script>
function assertEquals(expected, actual) {
  if (expected !== actual) {
    console.error(`Test failed! Expected: ${expected}, but ${actual} was given`);
  }
};

const tests = [
  function shouldGetZeroAsFirstInput() {
    assertEquals('0', new Calculator().text();
  },
  function shouldTypeOne() {
    // given
    const calculator = new Calculator();
    // when
    calculator.type('1');
    // then
    assertEquals('1', calculator.text());
  },
  function shouldTypeOnePlus() {
      // given
      const calculator = new Calculator();
      // when
      calculator.type('1');
      calculator.plus();
      // then
      assertEquals('1+', calculator.text());
  }
];

tests.forEach(test => test());
</script>

Dodajesz coś takiego do swojego kodu, patrzysz do konsoli i widzisz:
screenshot-20220310191848.png

Wystarczy że skopiujesz ten <script> i wsadzisz do siebie do kodu. Z dopisaniem kolejnych chyba sobie poradzisz? ;)

Wynik powyższych testów (zgodnie z przewidywaniami):
screenshot-20220312223908.png
Co dalej? Mam bohatersko przerabiać swój kod? Na czym polega haczyk? :>

class Calculator
{
    firstOperand = 0;
    secondOperand = 0;
    operation = '';
    ...
}

Powyższe zaliczy test, ale będzie wymagało większej zmiany w kodzie.

0
kosmonauta80 napisał(a):

Co dalej? Mam bohatersko przerabiać swój kod? Na czym polega haczyk? :>

Noi co w tym dziwnego?

Jeśli test wykazał jakiś przypadek w którym kod prezentuje defekt to nie pozostaje nic innego jak go poprawić.

0

Czyli te całe testy jednostkowe ogranicza tylko moja wyobraźnia.

0

Czy ma to sens, by dodawać zabezpieczenie na wypadek takiej sytuacji?

calculator.type('troll by scrum master');

Teoretycznie nie jest to możliwe. Z drugiej strony łatwo to ogarnąć za pomocą wyrażeń regularnych.

0
kosmonauta80 napisał(a):

Czy ma to sens, by dodawać zabezpieczenie na wypadek takiej sytuacji?

calculator.type('troll by scrum master');

Teoretycznie nie jest to możliwe. Z drugiej strony łatwo to ogarnąć za pomocą wyrażeń regularnych.

"Zabezpieczenie"?

Zależ po co. Jeśli żeby się upewnić czy ktoś wpisze tylko liczby, żeby zachować zgodność typów - tak, wtedy napisz test pod

calculator.type('abc');

oraz

calculator.type('');

I w teście upewnij się że leci wyjątek.

Jeśli po to żeby "hacker nie wpisał" to nie, nie ma żadnego sensu.

0

Dodałem metody, które formatują to, co zwraca metoda text(). Na razie dla firstOperand.

https://github.com/radioactiveCode/calculator

Pytanie, czy nie komplikuję tego za bardzo. Bo owszem, ładnie działa. Ale nie to chodzi.

1
kosmonauta80 napisał(a):

Dodałem metody, które formatują to, co zwraca metoda text(). Na razie dla firstOperand.

Pytanie, czy nie komplikuję tego za bardzo. Bo owszem, ładnie działa. Ale nie to chodzi.

Jest okej na razie. Dobrze zrobiłeś, tak ma być.

Pytanie tylko czemu do jasnej anielki nie napiszesz testów pod to? Oszczędziłbyś sobie trudu.

Napisz testy jednostkowe pod Calculator!!

https://github.com/radioactiveCode/calculator

if (charToCheck === '0')
{
    return this.limitZeros();
}
if (charToCheck === '.')
{
    return charToCheck;
}
return charToCheck;

ten kod jest bez sensu.

Użyj

if (charToCheck === '0') {
    return this.limitZeros();
}
return charToCheck;

1 użytkowników online, w tym zalogowanych: 0, gości: 1