Wątek przeniesiony 2019-05-03 16:33 z przez Patryk27.

Mój pierwszy projekt

Odpowiedz Nowy wątek
2019-01-02 20:28
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);
edytowany 2x, ostatnio: furious programming, 2019-05-03 23:27
Kod wstawiamy w znaczniki kolorujące składnię – np. ```html przed kodem, a po kodzie samo ```. Poprawiłem. - furious programming 2019-05-03 23:27

Pozostało 580 znaków

2019-01-02 21:37
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.


That game of life is hard to play
I'm gonna lose it anyway
The losing card I'll someday lay
So this is all I have to say
edytowany 2x, ostatnio: cerrato, 2019-01-03 09:00

Pozostało 580 znaków

2019-05-03 09:05
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. :)

@Patryk27: w sprawie oceny, może być przenieść ten temat? Czy zostawić w "JavaScript"? - Silv 2019-05-03 16:24
Okie dokie; - Patryk27 2019-05-03 16:33
Dzięki. :) - Silv 2019-05-03 16:34

Pozostało 580 znaków

2019-05-03 10:17
3

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.


edytowany 1x, ostatnio: furious programming, 2019-05-03 23:27

Pozostało 580 znaków

2019-05-03 16:04
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;
              }
            }
          }
edytowany 1x, ostatnio: furious programming, 2019-05-03 23:28

Pozostało 580 znaków

2019-05-03 16:14
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+

edytowany 1x, ostatnio: debug, 2019-05-03 16:17

Pozostało 580 znaków

2019-05-03 16:20
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.
edytowany 2x, ostatnio: Silv, 2019-05-03 16:29
Czy do wykorzystywania plików *.mp3 masz odpowiednie prawa? do takiego projektu też trzeba? - WeiXiao 2019-05-03 17:17
@WeiXiao: to zależy od licencji, na jakiej zostały te pliki udostępnione osobie wykorzystującej je. - Silv 2019-05-03 17:22
PS. Ale ekspertem na temat prawa nie jestem, więc moje wypowiedzi to raczej taki przyczynek powinien być do szerszych poszukiwań na temat prawa autorskiego. - Silv 2019-05-03 17:27
No przecież on, wrzucając to na GH udostępnia te pliki teoretycznie wszystkim mieszkańcom ziemi. Jeśli są to pliki stricte komercyjne, to jak najbardziej łamie ich licencje - cerrato 2019-05-03 20:41

Pozostało 580 znaków

2019-05-04 17:20
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. :)

Pozostało 580 znaków

2019-05-11 01:17
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.

Jaki jest sens używania branchy w jednoosobowym projekcie? - kzkzg 2019-05-11 08:35
Do oddzielenia pracy nad konkretnymi feacherami. - adams0 2019-05-11 09:34
@adams0: *feature'ami. - Silv 2019-05-11 13:34

Pozostało 580 znaków

Odpowiedz
Liczba odpowiedzi na stronę

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