Mój pierwszy projekt

0

Cześć.
Znam już całkiem nieźle Css, od kilku tygodni uczę się JS. Postanowiłem, że w ramach dodatkowej nauki napiszę grę podobną do Candy Crush Saga. Na razie wrzuciłem do dużego diva wrap 25 małych divów - kwadratów. W momencie pojawienia się, każdy z nich dostaje losowy kolor. Jeśli obok siebie (w poziomie) znajdują się 3 divy tej samej barwy, zadziała addEventListener, który po kliknięciu zmieni barwę wszystkich 25 kwadratów na różowy. Chciałbym teraz sprawić, żeby nie wszystkie kwadraty zmieniały kolor, tylko te trzy, które włączyły addEventListenera (3 kwadraty tego samego koloru leżące obok siebie w poziomie). Nie wiem jak to zrobić. Liczę na podpowiedź i konstruktywną krytykę mojego dotychczasowego kodu - zdaję sobie sprawę, że znaczną jego część pewnie można napisać prościej. Chciałbym, żeby ten temat był dla mnie dużym krokiem w nauce JS, dzięki Waszej pomocy mógłbym rozwiązywać kolejne napotykane problemy. Z góry dziękuję i pozdrawiam.

<!DOCTYPE html>
<html lang="en">
<head>
    <meta charset="UTF-8">
    <meta name="viewport" content="width=device-width, initial-scale=1.0">
    <meta http-equiv="X-UA-Compatible" content="ie=edge">
    <title>Document</title>
    <link rel="stylesheet" href="style.css">
</head>
<body>
    <div class = "wrap"></div>
    <script src="app.js"></script>
</body>
</html>
* {
    margin: 0;
    padding: 0;
    box-sizing: border-box;
}

.wrap {
    width: 80vh;
    height: 80vh;
    border: solid gray 5px;
    margin: 50px auto;
}

.candy {
    width: 20%;
    height: 20%;
    float: left;
    border: 1px black solid;
}

body {
    height: 100vh;
    width: 100vw;
}
const candy = document.querySelector('.candy');
const wrap = document.querySelector('.wrap');
let candyNumber = 1;

const start = () => {
 const candyColors = ['red', 'blue', 'green'];
 function createCandy() {
 const createDiv = document.createElement('div');
 document.querySelector('.wrap').appendChild(createDiv);
 const randomCandy = Math.floor(Math.random() * candyColors.length);
 createDiv.style.backgroundColor = candyColors[randomCandy];
 createDiv.classList.add('candy', candyNumber, "div" + candyNumber, candyColors[randomCandy]);
}
for (i=0; i<25; i++) {
createCandy();
candyNumber++;
}
}

start();

function a() {
    let x = 1;
    let y = 3;
    let z = 2;
    for (i = 0; i < 22; i++) {
        x++;
        y++;
        z++;
    if (document.querySelector('.div' + z).classList.contains("red") && document.querySelector('.div' + y).classList.contains("red") && document.querySelector('.div' + x).classList.contains("red") || document.querySelector('.div1').classList.contains("red") && document.querySelector('.div2').classList.contains("red") && document.querySelector('.div3').classList.contains("red") || document.querySelector('.div' + z).classList.contains("green") && document.querySelector('.div' + y).classList.contains("green") && document.querySelector('.div' + x).classList.contains("green") || document.querySelector('.div1').classList.contains("green") && document.querySelector('.div2').classList.contains("green") && document.querySelector('.div3').classList.contains("green") || document.querySelector('.div' + z).classList.contains("blue") && document.querySelector('.div' + y).classList.contains("blue") && document.querySelector('.div' + x).classList.contains("blue") || document.querySelector('.div1').classList.contains("blue") && document.querySelector('.div2').classList.contains("blue") && document.querySelector('.div3').classList.contains("blue"))  {
  const allCandy = document.querySelectorAll('.candy');
   allCandy.forEach((nazwa) => {
       nazwa.style.backgroundColor = "pink"; 
   })  
}
}
}

document.querySelector('.wrap').addEventListener('click', a);
0

Po pierwsze - wrzuć to na http://jsfiddle.net i daj link.

Po drugie - lepiej zmień nazwy zmiennych na jakieś bardziej opisowe, bo mając zmienne x, y, z albo funkcję o nazwie a można się pogubić (aczkolwiek muszę przyznać, że niektóre maja nazwy ok).

Po trzecie - skoro jesteś w stanie ustalić, że trzy elementy obok siebie maja określony kolor, to znaczy, że umiesz je jakoś "wyjąć" z tłumu. Czy nie da się analogicznie postąpić w zakresie zmiany koloru?

Po czwarte - wiele miejsc zawiera powtórzenia kodu. Aż się prosi, żeby te fragmenty wydzielić do osobnych funkcji.

0

Witam ponownie.

Projekt okazał się dla mnie za trudny i porzuciłem go. Jakieś dwa tygodnie temu, po kilku miesiącach nauki JS (niestety mam na to tylko 2 godziny dziennie), wróciłem do niego i go skończyłem. Złapałem się za głowę patrząc na mój kod ze stycznia i napisałem wszystko od nowa. Nauczyłem się też podstaw gita i zrobiłem repo [https://github.com/MarekSzczepanski/Square-Crush]. Liznąłem nieco programowania obiektowego i próbowałem jakoś je wdrożyć w mój projekt, nie wiem z jakim skutkiem. Proszę o ocenę mojego kodu, chętnie przyjmę krytykę, zwłaszcza że zrobiłem to wszystko całkiem sam od zera i nikt tego nie widział. Miłego dnia. :)

4

Brawo że sobie poradziłeś ale i tak widać że masz poważne braki.
Po pierwsze zamiast używać tablicy jednowymiarowej użyj dwuwymiarowej. Nie będziesz musiał potem sprawdzać co chwilę który to wiersz.

Poza tym masz masę powtarzającego się kodu, np:

              document.querySelector(".highscores"+10).textContent = document.querySelector(".highscores"+9).textContent;
              document.querySelector(".highscores"+9).textContent = document.querySelector(".highscores"+8).textContent;
              document.querySelector(".highscores"+8).textContent = document.querySelector(".highscores"+7).textContent;
              document.querySelector(".highscores"+7).textContent = document.querySelector(".highscores"+6).textContent;
              document.querySelector(".highscores"+6).textContent = document.querySelector(".highscores"+5).textContent;
              document.querySelector(".highscores"+5).textContent = document.querySelector(".highscores"+4
              ).textContent;
              document.querySelector(".highscores"+4).textContent = document.querySelector(".highscores"+3).textContent;
              document.querySelector(".highscores"+3).textContent = document.querySelector(".highscores"+2).textContent;
              document.querySelector(".highscores"+2).textContent = document.querySelector(".highscores"+1).textContent;
              document.querySelector(".highscores"+1).textContent = score;

Czemu nie wrzucisz tego do funkcji i nie wywołujesz? Ba, można to nawet jeszcze wrzucić do pętli for i będzie ladniej.

0

Dziękuję za odzew. Sam się sobie dziwie dlaczego nie przemyślałem tego fragmentu. Poprawiłem to w ten sposób:

const highscores = () => {
            for (let i=1; i<10; i++) {
              if (score > parseInt(document.querySelector(".highscores"+i).textContent, 10) || document.querySelector(".highscores"+i).textContent === "") {
                for (let j=10; j>i; j--) {
                  document.querySelector(".highscores"+j).textContent = document.querySelector(".highscores"+(j-1)).textContent;
                  document.querySelector(".playerName"+j).textContent = document.querySelector(".playerName"+(j-1)).textContent;
                }
                document.querySelector(".highscores"+i).textContent = score;
                document.querySelector(".playerName"+i).textContent = playerName; 
                return;
              }
            }
          }
2

Po pierwsze gratuluje wytrwałości ;).
W pliku "Animate.js" w funkcji animation. Mówię o tej funkcji:

animation(square, property, start, end, time) {
    if (property === "backgroundColor" && time === "short") {
      square.animate([{ backgroundColor: start }, { backgroundColor: end }], {
        duration: 300,
        fill: "forwards"
      });
    } else if (property === "backgroundColor") {
      square.animate([{ backgroundColor: start }, { backgroundColor: end }], {
        duration: 1000,
        fill: "forwards"
      });
    } else if (property === "border") {
      square.animate([{ border: start }, { border: end }], {
        duration: 1000,
        fill: "forwards"
      });
    } else if (property === "left") {
      square.animate([{ left: start }, { left: end }], {
        duration: 1000,
        fill: "forwards"
      });
    } else if (property === "width") {
      square.animate([{ width: start }, { width: end }], {
        duration: 300,
        fill: "forwards"
      });
    } else if (property === "height") {
      square.animate([{ height: start }, { height: end }], {
        duration: 300,
        fill: "forwards"
      });
    } else if (property === "color") {
      square.animate([{ color: start }, { color: end }], {
        duration: 1000,
        iterations: Infinity,
        direction: "alternate"
      });
    } else if (property === "top") {
      square.animate([{ top: start }, { top: end }], {
        duration: 1000,
        fill: "forwards"
      });
    }
  }

mogłeś użyć Switch/Case zamiast tylu if-else. Było by czytelniej. Tak samo argument time, zamiast przyjmować "short" || "normal" || "long" niech przyjmuje od razu czas w milisekundach.
Jeszcze krócej mógł byś napisać tą funkcje tak:

animation(square, property, start, end, time) {
    square.animate([{ [property]: start }, { [property]: end }], {
      duration: time || 1000,
      fill: "forwards"
    });
  }

6 linijek zamiast 40+

1

Jako pierwsze spostrzeżenie polecam w repozytorium wrzucić pliki o tym samym rozszerzeniu do jednego katalogu (oczywiście nie zapomnij zaktualizować ścieżek do nich we wszystkich odpowiednich plikach).


UPDATE:

  • Plik .gitignore masz pusty – tak powinno być?
  • Czy do wykorzystywania plików *.mp3 masz odpowiednie prawa?

UPDATE2:

  • Co do mojej wcześniejszej wzmianki: plik index.html zostawiłbym w głównym katalogu aplikacji.
2
debug napisał(a):
> animation(square, property, start, end, time) {
>     square.animate([{ [property]: start }, { [property]: end }], {

Pierwszy raz spotykam się z taką burzą różnych nawiasów. Czy to tablica zawierająca dwa obiekty, a w każdym z nich jest inna tablica, której zawartość to property z wywołania funkcji? Nie wiem czy dobrze to rozumiem, ale to jest świetne. :)

Poprawiłem w moim projekcie wszelkie powtórzenia "ifów", na tyle, na ile potrafię. Powrzucałem je w pętle i funkcje, zależnie od sytuacji.

Muzykę zmieniłem na taką z odpowiednią licencją i dodałem creditsy. Wcześniej wrzuciłem muzykę klasyczną. Nie przyszło mi do głowy, że mp3 z Chopinem, czy Mozartem może być objęte prawami autorskimi, ale w sumie to nie ich słychać na nagraniu, ktoś to musiał zagrać.

Wrzuciłem wszystkie pliki js do osobnego folderu, podobnie inne rodzaje plików, których rozszerzenie się powtarza np. dźwięki do osobnego folderu.

Plik .gitignore jest pusty, bo we wcześniejszych commitach przechowywałem w nim pliki, które stworzyłem na użytek w przyszłych commitach, i które na tamtą chwilę nie były potrzebne. Wszystkie z nich znalazły już zastosowanie i dlatego .gitignore został pusty.

Poświęciłem trochę czasu na tablice dwuwymiarowe, ale mam problem ze zrozumieniem ich zastosowania. Pewnie przyjdzie z czasem, tak jak wiele innych zagadnień, z którymi miałem problem, a już problemem nie są.

Zrobiłem nowego commita, zmiany można już zobaczyć. [https://github.com/MarekSzczepanski/Square-Crush] Dziękuję Wam za wskazówki. Miło mi że poświęcacie swój cenny czas na recenzję mojego projektu. :)

2

Cześć!

Przeglądałem historię twojego projektu.
Jeden commit potrafi dotyczyć zmian na kilkunastu plikach!
Nie mówiąc o tym że zmiany w niejednym z nich wypadało by podzielić na osobne commity.
Pamiętaj o zasadzie SRP i o tym że to odnosi się również do gita.
Dobra historia commitów ułatwi Ci zarządzanie projektem.
Używaj branchy.

Funkcje animation podziel na mniejsze funkcje.
Nie katuj się funkcjami z pięcioma argumentami.

Folder sounds waży jakieś 25MB. Za dużo. Wywal to co niepotrzebne.

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