gra w statki - single player (szybki projekt)

1

Projekt na 4 wieczory, taki na szybko w celu zdobycia doświadczenia, rozeznania i rozgrzewki przed:
https://4programmers.net/Foru[...]gra_w_statki_koncepcja?page=1

Od strony wizualnej można by co nieco poprawić, mnie bardziej interesuje opinia na temat kodu w JS:

// ----------------------------------------------------------------
// deklaracja zmiennych

// przyciski
let generateButton = document.getElementById("losuj");
let resetButton = document.getElementById("resetuj");

// status pól planszy (max 100) (0-99)
let areaSize = 100;
let freeAreas = document.getElementsByTagName("td");

// zebrane punkty i oddane strzały
let totalHitsByUser = 0;
let totalHitsByPc = 0;
let totalShotsByUser = 0;
let totalShotsByPc = 0;
let currentScore = document.getElementsByTagName("p");

// wyniki losowania (pozycja i kierunek statku)
let generateShipPositionResult = 0;
let generateShipDirectionResult = false;

// rozmiary i ilość statków
let shipLength = 0;
let noOfShips = 5;

// tymczasowe
let temp = 1;

// ----------------------------------------------------------------
// obsługa kliknięcia na planszę oraz przycisków losuj, reset, xxx

// klinięcie każdego pola - albo trafione - Hit, albo nie - Miss
for (let i = 0 ; i < freeAreas.length ; i++) 
{
    freeAreas[i].addEventListener('click', function()
    {   
        if ((this.textContent != "M") && (this.textContent.charAt(0) != "H"))
        {  
            totalShotsByUser++;
            checkIfHitOrMiss(this.id);   
        }
        updateResults();
    })
}

// przycisk losujący planszę
generateButton.addEventListener('click', function()
{
    // wylosuj i zablokuj przycisk 
    generateButton.disabled = true;

    for (let i = 0 ; i < noOfShips ; i++) 
    {
        // ustal długość okrętu (1-2-3-4-5)
        shipLength+=1;

        // wylosuj pozycję tak, by zmieściła się na planszy uwzględniając długość okrętu
        do
        {
            generateShipPositionResult = Math.floor((areaSize - 1) * Math.random());
        }
        while (!((generateShipPositionResult + shipLength) < 101));

        // wylosuj kierunkek
        generateShipDirectionResult = Math.random() < 0.5;

        // pokaż w konsoli
        console.log("wylosowana pozycja: " + (1 + generateShipPositionResult));
        console.log("kierunek (true - w prawo): " + generateShipDirectionResult);
        console.log("długość statku: " + shipLength);

        // rysowanie
        if ((checkIfFits(generateShipPositionResult, generateShipDirectionResult, shipLength)) && (checkIfColision(generateShipPositionResult, generateShipDirectionResult, shipLength)))
        {
            // nie ma kolizji i mieści się na planszy - jest ok
            if (generateShipDirectionResult)
            {
                // true - poziomo w prawo
                for (let i = 0 ; i < shipLength ; i++) 
                {
                    freeAreas[generateShipPositionResult + i].textContent = "X" + shipLength;
                    freeAreas[generateShipPositionResult + i].className = "random";
                }
            }
            else
            {
                // false - pionowo w dół
                for (let i = 0 ; i < (shipLength * 10) ; i += 10) 
                {
                    freeAreas[generateShipPositionResult + i].textContent = "X" + shipLength;
                    freeAreas[generateShipPositionResult + i].className = "random";
                }
            }
        }
        else
        {
            // losuj do skutku - w razie niepowodzenia cofamy pętlę 
            i--;
            shipLength--;
        }
    }  
})

// przycisk resetujący planszę
resetButton.addEventListener('click', function()
{
    window.location.reload();
})

// ----------------------------------------------------------------
// metody pomocznicze

// czy mieściy się na planszy?
function checkIfFits(p, d, l)
{
    let noFit = true;

    if (d)
    {
        // sprawdzamy w prawo bo poziomo
        if ((parseInt(freeAreas[p].id.charAt(2)) + parseInt(l) > 11) || (freeAreas[p].id.substring(2) == "10"))
        {
            console.log("poza obszarem poziomo!");
            return false;
        }
    }
    else
    {
        // sprawdzamy w dół bo pionowo
        if ((parseInt(freeAreas[p].id.charAt(1)) + parseInt(l) > 11) || (freeAreas[p].id.substring(1,3) == "10") || (freeAreas[p].id == "x110"))
        {
            console.log("poza obszarem pionowo!");
            return false;
        }
    }

    return noFit;
}

// czy nie ma kolizji?
function checkIfColision(p, d, l)
{
    let noColision = true;

    if (d)
    {
        // sprawdzamy w prawo bo poziomo
        for (let i = 0 ; i < l ; i++) 
        {
            if (freeAreas[p+i].textContent.charAt(0) == "X")
            {
                console.log("kolizja poziomo!");
                return false;
            }
        }
    }
    else
    {
        // sprawdzamy w dół bo pionowo
        for (let i = 0 ; i < (l * 10) ; i+=10) 
        {
            if (freeAreas[p+i].textContent.charAt(0) == "X")
            {
                console.log("kolizja pionowo!");
                return false;
            }
        }
    }

    return noColision;
}

// czy trafiliśmy?
function checkIfHitOrMiss(p)
{
    let position = document.getElementById(p);

    if (position.textContent.charAt(0) == "X")
    {
        position.className = "hit";
        position.textContent = "H" + position.textContent.charAt(1);
        totalHitsByUser++;
    }
    else
    {
        position.className = "miss";
        position.textContent = "M";
    }

} 

function updateResults()
{
    currentScore[0].textContent = "Liczba oddanych strzałów: " + totalShotsByUser;
    currentScore[1].textContent = "Liczba trafień: " + totalHitsByUser;
    if (totalHitsByUser == 15)
    {
        alert("Wygrałeś!");
    }
}

tabelka.rar

0

Dlaczego bardziej interesuje Cię opinia na temat kodu? Czy nie lepiej byłoby otrzymać opinię na temat samej gry i tego jak uczynić ją ciekawszą?

A jeśli interesuje Cię opinia na temat kodu, to jaki w zasadzie masz problem z kodem? Czy jest coś co Ci w nim przeszkadza lub zakłóca pracę? Bo jeśli nie, to tracisz czas.

0

Ostatnio "oberwało mi się" za kod w JS, więc ciekaw jestem czy jest postęp. Ale ogólne uwagi też mile widziane.

5

Kod, ogólnie nie jest najlepszy, ja nad takim na pewno nie chciałbym pracować.

  1. Zmienne globalne. Do refactora teraz zaraz!
  2. Globalny kod (top-level, zamiast pochowany w funkcjach).
  3. Funkcji jest za mało i są za długie. Powinieneś mieć dużo małych funkcji, zamiast mało dużych.
  4. W ogóle to kod nie jest rozbity na takie funkcje jakie powinien być. U Ciebie to wygląda tak jakbyś robił jedną funkcję javascriptową na jedną funkcję programu (funkcjonalność). Podejście trochę średnie, jak masz tak dzielić to lepiej nie dzielić w ogóle.
  5. Ten event handler w generateButton na click, jest dużo za duży.
  6. Głupio że posługujesz się funkcjami alert() i console.log() do interakcji z userem.
  7. Zgaduje że testów automatycznych nie ma?
  8. Zmienna temp, nie wiem po co Ci ona, i czemu jest globalna ale to jest zły pomysł.
  9. Trzymasz dane o grze w postaci tekstowej, a to conajmniej proszenie się o kłopoty. Powinieneś zrobić matrycję/tablicę dwuwymiarową.
  10. Nie nazywaj zmiennych pojedynczymi literami.
1

@kosmonauta80 Myślę, że nie pytać o rady przy takich drobnych programach, one tylko rozpraszają uwagę. Stosowanie się do oklepanych praktyk na taką małą skalę jest raczej czymś ekstremalnym, a zarazem ciężkim do pojęcia. Lepiej pisz więcej, i ciekawsze rzeczy i czekaj, aż to co robisz zacznie Ci się burzyć, wtedy będziesz miał najlepsze źródło do analizy.

4
pan_krewetek napisał(a):

@kosmonauta80 Myślę, że nie pytać o rady przy takich drobnych programach, one tylko rozpraszają uwagę. Stosowanie się do oklepanych praktyk na taką małą skalę jest raczej czymś ekstremalnym, a zarazem ciężkim do pojęcia. Lepiej pisz więcej, i ciekawsze rzeczy i czekaj, aż to co robisz zacznie Ci się burzyć, wtedy będziesz miał najlepsze źródło do analizy.

Nie, to zły pomysł i zła rada.

tutaj ma program na około 100-150 linijek, i można go poprawić i napisać dobrze. Moim zdaniem to jest idealny program żeby spróbować go poprawić.

@kosmonauta80 Jak chcesz to zgłoś się do mnie, usiądziemy na 30 minut i powiem Ci jak z tej aplikacji zrobić program p̵i̵e̵r̵w̵s̵z̵a̵ ̵k̵l̵a̵s̵a̵ wysokiej jakości.

0

Zmienne globalne. Do refactora teraz zaraz!

Dla wygody tak zrobiłem :) Ale jak by co, to na półce już czeka Refaktoryzacja. Ulepszanie struktury istniejącego kodu. Martin Fowler

Globalny kod (top-level, zamiast pochowany w funkcjach).

Co konkretnie byś umieścił w funkcjach?

Funkcji jest za mało i są za długie. Powinieneś mieć dużo małych funkcji, zamiast mało dużych.

Przyjąłem.

W ogóle to kod nie jest rozbity na takie funkcje jakie powinien być. U Ciebie to wygląda tak jakbyś robił jedną funkcję javascriptową na jedną funkcję > programu (funkcjonalność). Podejście trochę średnie, jak masz tak dzielić to lepiej nie dzielić w ogóle.

W zasadzie to można to równie dobrze napisać stosując podejście procedularne.

Ten event handler w generateButton na click, jest dużo za duży.

Nie rozumiem.

Głupio że posługujesz się funkcjami alert() i console.log() do interakcji z userem.

To dla mnie. Na etapie testowania załatałem kilka ciekawych "bugów".

Zgaduje że testów automatycznych nie ma?

To jest II w życiu większy program napisany w JS, także spokojnie :) W sumie to nie wiem nawet, jak to się robi (testy jednostkowe).

Zmienna temp, nie wiem po co Ci ona, i czemu jest globalna ale to jest zły pomysł.

Pozostałość po testach. Można pominąć.

Trzymasz dane o grze w postaci tekstowej, a to conajmniej proszenie się o kłopoty. Powinieneś zrobić matrycję/tablicę dwuwymiarową.

A planszę generować dynamicznie z poziomu JS.

Nie nazywaj zmiennych pojedynczymi literami.

Masz na myśli liczniki pętel tj "i"?

2
kosmonauta80 napisał(a):

Zmienne globalne. Do refactora teraz zaraz!
Dla wygody tak zrobiłem :) Ale jak by co, to na półce już czeka Refaktoryzacja. Ulepszanie struktury istniejącego kodu. Martin Fowler

Nie wiem co jest niewygodnego w deklarowaniu zmiennych przy najbliższym ich użyciu. Jeśli sobie wmówisz że to "wygodne", to szybko zaczniesz pisać kod na który nie da się patrzeć.

Globalny kod (top-level, zamiast pochowany w funkcjach).
Co konkretnie być umieścił w funkcjach?

Każdy jeden element Twojej aplikacji, wszystko. Na górze zostawiłbym tylko włączenie głównej funkcji main(). Nie powinno być nic globalnego, poza może elementami UI (przyciski i tabelka).

Jeśli chcesz to mogę Ci powiedzieć jak można pomału zacząć migrować ten kod.

W ogóle to kod nie jest rozbity na takie funkcje jakie powinien być. U Ciebie to wygląda tak jakbyś robił jedną funkcję javascriptową na jedną funkcję > programu (funkcjonalność). Podejście trochę średnie, jak masz tak dzielić to lepiej nie dzielić w ogóle.
W zasadzie to można to równie dobrze napisać stosując podejście procedularne.

On jest proceduralny, i nie widzę szybkiej zmiany na inny paradygmat. To że używasz funkcji, nie znaczy że to nie kod proceduralny.

Ten event handler w generateButton na click, jest dużo za duży.
Nie rozumiem.

generateButton.addEventListener('click', function()
{
// Tutaj funkcja na 50 linijek albo i więcej

Głupio że posługujesz się funkcjami alert() i console.log() do interakcji z userem.
To dla mnie. Na etapie testowania załatałem kilka ciekawych "bugów".

Pozbądź się ich z kodu. Jeśli chcesz zabezpieczyć się przed bugami, polecam testy jednostkowe.

Zmienna temp, nie wiem po co Ci ona, i czemu jest globalna ale to jest zły pomysł.
Pozostałość po testach. Można pominąć.

Wywal.

Trzymasz dane o grze w postaci tekstowej, a to conajmniej proszenie się o kłopoty. Powinieneś zrobić matrycję/tablicę dwuwymiarową.
A planszę generować dynamicznie z poziomu JS.

Możesz spokojnie to zrobić. Chodzi o to żeby "dane" gry były oddzielone od "widoku" gry, czyli od interfejsu użytkownika.

Nie nazywaj zmiennych pojedynczymi literami.
Masz na myśli liczniki pętel tj "i"?

checkIfColision(p, d, l)
0

Nie, to zły pomysł i zła rada.

Dlaczego? Pod jakim względem sztuka dla sztuki jest lepsza od koncentracji na doświadczeniach usera?

Co z Twoich argumentów wynika poza faktem jaki stwierdzasz, że "na kod nie da się patrzeć"? Pod jakim względem Twoje zmiany rzeczywiście wnosza jakąś wartość?

0

Nie wiem co jest niewygodnego w deklarowaniu zmiennych przy najbliższym ich użyciu. Jeśli sobie wmówisz że to "wygodne", to szybko zaczniesz pisać kod na który nie da się patrzeć.

Konieczność przekazywania do funkcji większej liczy argumentów. Ale przyjmuję to, co napisałeś.

Jeśli chcesz to mogę Ci powiedzieć jak można pomału zacząć migrować ten kod.

W jak sposób i za ile? ;)

generateButton.addEventListener('click', function()
{
// Tutaj funkcja na 50 linijek albo i więcej

Czyli zrobić po prostu metodę reagującą na window.click i konkretne id pola planszy?

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

Robot: CCBot