gra w statki - single player (szybki projekt)

1

Projekt na 4 wieczory, taki na szybko w celu zdobycia doświadczenia, rozeznania i rozgrzewki przed:
gra w statki - koncepcja

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?

0

Widzę że nie wnosisz nic nowego do tematu i jedynie chcesz siać zamęt, ale postaram się odpowiedzieć.

pan_krewetek napisał(a):

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ść?

Rozumiem że rozchodzi się o ten wątek?

TomRiddle napisał(a):
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.

Otóż spieszę z wyjaśnieniem.

Odebrałem Twój post, tak jakbyś sugerował że kod który napisany przez zadającego pytanie jest dobry taki jaki jest, nie wymaga żadnych poprawek; i odebrałem to również tak że sugerowałeś że jeśli znajdziemy jakieś poprawki, to tylko takie niewarte uwagi, które nie wniosły by wiele i były, jak to ująłeś? I że jedynie jeśli przejdzie do większej skali, to wtedy będzie można szukać jakichś błędów.

Czy dobrze odebrałem przesłanie Twojego posta?

Jeśli tak, to nie zgadzam się z tym, bo w kodzie zadającego pytanie jest wiele rzeczy które dałoby się zrobić lepiej. Przyznaję, nie zmieniłoby to sposobu działania programu w żaden sposób; ale kod byłby wyższej jakości. Znaczy to że byłby czytelniejszy, bardziej przejżysty i bardziej podatny na utrzymanie, łatwiej również byłoby go edytować.

Zapytasz może co dadzą nam te wszystkie rady i poprawki. Po co mieć czytelny kod, łatwy do utrzymania, i łatwy do zmiany skoro i tak nie będziemy z nim pracować bo jest taki krótki? A no właśnie po to żeby nauczyć się pisać dobry kod, i kiedy już będziemy pisać coś poważnego, to będziemy mieli odpowiednie umiejętności jak zrobić to dobrze.

Dziękuję za poruszenie tematu.

1
kosmonauta80 napisał(a):

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ś.

W niektórych przypadkach pewnie tak, w innych nie koniecznie.

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

W jak sposób i za ile? ;)

Na discordzie, za darmo. Albo ewentualnie na google meets.

Jeśli chcesz zacząć to musisz ściągnąć program git: https://git-scm.com/

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?

Nie, źle mnie zrozumiałeś. Program ma działać dokładnie tak jak teraz działa. Chodzi o organizację kodu.

Plan jest taki, żeby podzielić program na funkcję. Jak to zrobić, spytasz, skoro program robi wiele rożnych rzeczy. Ano tak, że bierzesz najmniejszy kawałek funkcji którą program ma robić, i wydzielasz do funkcji. Potem znowu i znowu. Potem wydzielasz funkcje które wywołują inne funkcje, a z kolei kolejne wywołują tamte.

Efektem jest kod, który ma około 50-100 małych funkcji, z których każda robi jedną rzecz, wszystkie przekazują sobie dane przez arugmenty,nie ma duplikacji pomiędzy zmianami; zmiany można wprowadzić tylko w jednej z nich; i dane nie są już zależne od widoku.

PS: Aha, noi brak wstydu przy pokazaniu komuś tego kodu.

0

Myślę, że 100 funkcji to za mało. Tak ze 200-400 to już spoko. Zresztą, każda linijka robi co innego to każda może być funkcją.

5
// czy nie ma kolizji?
function checkIfColision(p, d, l)

Wystrzegaj się komentarzy, które niczego nie wnoszą. Są całkowicie zbędne.

1

Masz dużo niepotrzebnych komentarzy, np.:

// tymczasowe
let temp = 1;

albo:

// czy trafiliśmy?
function checkIfHitOrMiss(p)

czy // pokaż w konsoli, po czym następują console.log.

tego typu komentarze można wywalić, bo powtarzasz to, co już i tak widać w kodzie. Niestety z jakichś powodów uczy się programistów, żeby pisali jak najwięcej komentarzy (też kiedyś myślałem, że trzeba komentować kod). A komentarzy powinno być jak najmniej, a nie jak najwięcej.

Dla kontrastu widzę też komentarze, które są faktycznie potrzebne, np. // sprawdzamy w dół bo pionowo - to ma jakieś uzasadnienie, bo kod jest niezbyt czytelny (pytanie, tylko czy nie lepiej byłoby to np. wydzielić to do funkcji jakoś).

Dalej, nazewnictwo zmiennych:

let generateShipPositionResult = 0;

ta zmienna wygląda jak nazwa funkcji ("generuj rezultat pozycji statku"), więc jeśli ma być to zmienna, w której trzymasz liczbę, to trochę dziwna nazwa.
Raczej ładniej by było po prostu "shipPositionResult" albo "generatedShipPositionResult" (wygenerowany rezultat pozycji statku). Też nie jestem przekonany, że słówko "result" jest potrzebne (shipPosition?)

let noOfShips = 5;

czemu no? Lepsze byłoby numberOfShips, bardziej przewidywalna nazwa(least surprise principle). Albo shipCount.

let currentScore = document.getElementsByTagName("p");

currentScore brzmi jak liczba, a nie jak "lista elementów HTML, do których będę pakował komunikaty dla użytkownika". Więc ta nazwa zmiennej nazywa się kompletnie inaczej niż to, co zawiera.

for (let i = 0 ; i < freeAreas.length ; i++) 
{
    freeAreas[i].addEventListener('click', function()

nie musisz przydzielać eventu dla każdego elementu. Wystarczy, że złapiesz event wyżej, w rodzicu elementu (albo rodzicu-rodzica, nawet w obiekcie document możesz łapać eventy).

polecam poczytać o event delegation
https://developer.mozilla.org/en-US/docs/Learn/JavaScript/Building_blocks/Events

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