Prosta gra w JS – code review

Witam, proszę o code review i konstruktywną krytykę dotyczącą moich pierwszych wypocin. Gra polega na kupnie i sprzedaży towarów. Przyznaję się, że jest jeszcze parę rzeczy które chcę poprawić i dodać. Wspomnę tylko, że programowania uczę się od lutego.

Link do githuba: https://github.com/malkrzysztof
github pages https://malkrzysztof.github.io/stock/

Totalnie nieintuicyjna obsługa. Kupiłem sobie 10 rzeczy.... i co dalej? Nie wiem, co mam zrobić, nie widzę przycisku "kolejna runda" ani niczego, co może mnie dalej poprowadzić.

Sugestia: po wpisaniu ilości danego materiału i wciśnięciu "buy" okienko do wpisywania wartości fajne, jakby się wyczyściło. Obecnie wpisana ilość pozostaje, mimo że zakup został dokonany.

Zrób coś z kursorem - cały czas jest "łapka", jak przy najeżdżaniu na linki. Jest to także mylące. Zamiast tego powinna być strzałka, która się zmienia w łapkę dopiero po najechaniu na coś klikalnego.

Lista "materials" po lewej stronie - przydałby się jakiś hover, który zmienia kolor/podkreśla/w jakiś inny sposób wyróżnia daną pozycję w chwili najechania na nią myszką (analogicznie do listy miast z prawej strony). Aktywna w danej chwili rzecz powinna być jakoś wyróżniona - może np. jakiś ptaszek obok.

Podczas zmieniania rozmiaru ekranu większość się skaluje OK, ale górna linia nie - więc zwężając okno, w pewnej chwili informacja o dostępnych pieniądzach ucieka poza ekran.

EDIT: Po chwili zauważyłem, że klikanie na miastach po lewej powoduje przejście do kolejnej rundy. Ale o co tu chodzi, co mam robić i jak się w to gra - nie mam zielonego pojęcia. Myślałem, że na GH może dałeś jakiś readme, który wyjaśnia zasady - ale niczego takiego nie znalazłem.

Nie podoba mi się ten kod w function buyWood() sellWood() itd. To praktycznie kopia kodu dla pozostałych surowców.
Warto byłoby trzymać dane gry w localStorage.

Haskell napisał(a):

Nie podoba mi się ten kod w function buyWood() sellWood() itd. To praktycznie kopia kodu dla pozostałych surowców.
Warto byłoby trzymać dane gry w localStorage.

Bo to w zasadzie jest kopia, nie za bardzo wiem jak mógłbym to zmiejszyć? Idea była taka że wybieram po lewej materiał i ten materiał wstawia się w pole które pobiera cene i mnoży przez podaną ilość, dlatego dla każdego surowca jest oddzielna funkcja, nie mam pojęcia jak to zoptymalizowac, jakieś pomysły? podpowiedź?
Miałem plan wdrożyć localStorage, ale nie umiem jeszcze tego, ogarnę jak to zrobić i na pewno wdroże.

O czyszczeniu inputa z ilością nie pomyślałem, ale na pewno się sprawdzi. Co do łapki to to wydawało mi się to dobrym pomysłem, ale może faktycznie będzie lepiej jak łapka będzie tylko na buttonach.
Pokazywać wybrany materiał miałem w zamyśle tylko na środku w sekcji akctions, dorobienie ptaszka po lewej na pewno nie zaszkodzi.
Nad responsywnością w ogóle nie pracowałem poza tym że zrobiłem wszystko na flex, będę jeszcze nad tym pracował bo chciałem żeby docelowo przy mniejszym ekranie zawartość łamała się.
Jeżeli chodzi o instrukcje i plik na githubie readme to jest to pierwsza rzecz którą teraz będę robił. Dodatkowo wszystko chce wrzucić w jedną funkcję która będzie pobierała imię gracza i miasto startowe.

to w zasadzie jest kopia, nie za bardzo wiem jak mógłbym to zmiejszyć

To "zmniejszanie" o którym piszesz nazywa się "parametry funkcji". Do poczytania i zrozumienia:
https://developer.mozilla.org/pl/docs/Web/JavaScript/Guide/Funkcje
http://kursjs.pl/kurs/super-podstawy/funkcje.php
http://webmaster.helion.pl/index.php/kjs-cechy-jezyka/kjs-funkcje/kursjs-argumenty-funkcji
https://blog.piotrnalepa.pl/2014/01/24/js-kilka-slow-o-parametrach-w-funkcjach-javascript/
https://jcubic.pl/2014/08/funkcje-w-javascript.html

Zamiast pisać 5 funkcji o nazwach sprzedajA, sprzedajB, sprzedajC itp, robisz jedną wspólną function sprzedaj(coś); i w niej obsługujesz w uniwersalny sposób kupowane albo sprzedaż danego surowca, a następnie ją wywołujesz w postaci: sprzedaj(A); sprzedaj(B); sprzedaj(C);. Plusem posiadania wszystkiego w jednej funkcji (poza świadomością zrobienia tego porządnie oraz większą czytelnością kodu) jest także to, że ewentualne zmiany wprowadzasz w jednym miejscu. Gdybyś chciał zmienić coś w mechanizmie sprzedawania surowców, to w obecnej postaci musiałbyś w każdej funkcji obsługującej dany typ surowca wprowadzać taką zmianę osobno. A posiadając specjalną funkcję, która się zajmuje uniwersalnie sprzedażą wszystkich/każdego z surowców - wprowadzasz zmianę raz i już dotyczy ona wszystkich możliwych materiałów/surowców.

Rzuć okiem na ten wątek - Canvas - Circle progress , a w szczególności na ten post - https://4programmers.net/Forum/1500531 . OP potrzebował funkcję rysującą kółeczka - trochę mu pomogłem, trochę zrobiłem za niego, dużo wyjaśniłem. Jak zajrzysz na jsfiddle to zauważysz, że sam skrypt jest dość prosty i krótki. Wprawdzie są rysowane 3 kółka, ale funkcja rysująca jest tylko jedna, za to wywołana 3 razy, za każdym razem z innymi parametrami (patrz - pkt. 5 wyjaśnień w podanym powyżej poście). Jeśli zrozumiesz o co chodzi w podanym przeze mnie wątku oraz jak działa podlinkowany skrypt - powinieneś wiedzieć, w jaki sposób masz przerobić swoje funkcje sprzedająco-kupujące.

W temacie uwag/poprawek/sugestii do Twojej gry mam jeszcze parę rzeczy do dodania/uzupełnienia wcześniejszego posta (tamten się skupiał na funkcjonalności, teraz odnoszę się do kodu):
1) widzę, że na GH jest plik background.js, a w nim jest coś nazwane particlesJS("particles-js". Nie sprawdzałem dokładnie o co tu chodzi, więc mogę się mylić, ale posiadane doświadczenie życiowe sugeruje, że gra powinna mieć jakieś animowane tło. Niestety - coś chyba nie wyszło, bo u mnie nie widać żadnego efektu, statyczna strona bez ozdobników w tle
2) w pliku game.js w liniach 1-32 masz definicje wielu zmiennych. W obecnej postaci Twój zapis nie jest błędny, ale po prostu nie wygląda zbyt ładnie. Moim zdaniem czytelniej byłoby zrobić to analogicznie do rozwiązania stosowanego w podanym przeze mnie przykładzie z animacją kółeczek - konkretnie chodzi mi o sposób wykorzystany chociażby do definiowania parametrów kółek, na przykład linie 1-6 określające parametry kolo1. Możesz tak samo zrobić z definicjami z linii 1-32 i zamiast wprowadzać pełno luźno wiszących sobie zmiennych, jakoś to uporządkować i zrobić coś na kształt:

var wood = {};
wood.price = document.querySelector('#woodPrice');
wood.item = document.querySelector('#wood');
wood.ownedWood = document.querySelector('#ownedWood');

3) ponownie pojawia się kwestia parametrów funkcji. Zamiast pisać coś takiego:

warsaw.addEventListener("click", function(){
	travelWarsaw();
});

i powtarzać to dla każdego miasta poprzez wywołanie osobnej funkcji przeznaczonej do obsługi danej lokalizacji, lepiej byłoby zrobić jedną uniwersalną funkcję obsługującą wszystkie miasta, a powyższ fragment zastąpić czymś w stylu:

warsaw.addEventListener("click", function(){
	travelToCity('warsaw');
});

4) To samo dotyczy funkcji z grupy berlinPrices(); - masz 6 razy powtórzone praktycznie do samo, więc powinieneś zrobić z tym porządek.

cerrato napisał(a):

2) w pliku game.js w liniach 1-32 masz definicje wielu zmiennych. W obecnej postaci Twój zapis nie jest błędny, ale po prostu nie wygląda zbyt ładnie. Moim zdaniem czytelniej byłoby zrobić to analogicznie do rozwiązania stosowanego w podanym przeze mnie przykładzie z animacją kółeczek - konkretnie chodzi mi o sposób wykorzystany chociażby do definiowania parametrów kółek, na przykład linie 1-6 określające parametry kolo1. Możesz tak samo zrobić z definicjami z linii 1-32 i zamiast wprowadzać pełno luźno wiszących sobie zmiennych, jakoś to uporządkować i zrobić coś na kształt:

Kopiuj

var wood = {};
wood.price = document.querySelector('#woodPrice');
wood.item = document.querySelector('#wood');
wood.ownedWood = document.querySelector('#ownedWood');

Czy to można nazwać programowaniem obiektowym ?

Sporo nowej wiedzy do przyswojenia. Nie pomyślałem o nadawaniu argumentów funkcji. W takim razie przynajmniej połowa kodu jest do przepisania na nowo. Najpierw zajmę się kodem js, potem porawie łapki i inne cssowe rzeczy.
Dzięki za dużą pomoć, zrefaktoruje kod i podbije wątek do oceny.

Hej,
jak zamierzasz poprawiać (do kodu nie zaglądałem), to proponuję jeszcze:

1. zrobić reset gry po 20 poziomie, bo jest dalej kontynuowana... w sumie można próbować zrobić reset w dowolnym punkcie gry...
2. grafika (nie jest za dobra)... zastosuj ramki... to duża zmienia... pozmieniaj czcionki dla różnych objektów...
3. w środkowej części zrób może jakieś odstępy między liniami...
4. zastosuj jakieś przyciski... np. reset gry...
5. ciężko się gra, jak nie znasz cen w różnych miastach... może warto na wstępie udostępnić ceny, aby można było łatwiej spekulować...
6. są jeszcze inne kolory niż szary...
7. spróbuj napisać parę podstron, żeby można było między nimi nawigować...

Pozdrawiam... :)

Czy to można nazwać programowaniem obiektowym ?

No tak trochę, ale nie do końca ;) (dla osób bardziej zorientowanych - wszystko poniżej jest pisane w pewnym uproszczeniu).

Prawdziwe OOP ma wiele cech, których w podanym przykładzie nie uświadczysz - przykładowo dziedziczenie po innych klasach oraz posiadanie poza polami z danymi także metod (czyli funkcji stanowiących elementy składowe danego obiektu i wykonujące na nim oraz "posiadanych" przez niego danych pewne operacje). W ogóle OOP w JavaScript jest uboższe, niż w "prawdziwych" językach. W moim przykładzie po prostu masz kilka zmiennych dotyczących jednego "tematu" zebrane razem. Do poczytania - https://www.w3schools.com/js/js_datatypes.asp oraz https://developer.mozilla.org/pl/docs/Web/JavaScript/Wprowadzenie_do_programowania_obiektowego_w_jezyku_JavaScript

Udało mi się przeczytać wszystko co podrzuciliście, teraz czas na działania.
Jeżeli chodzi o animowane tło to u mnie lokalnie jest, w githubie nie wyświetla, za to wywala błąd w konsoli

The page at 'https://malkrzysztof.github.io/stock/' was loaded over HTTPS, but requested an insecure script 'http://cdn.jsdelivr.net/particles.js/2.0.0/particles.min.js'. This request has been blocked; the content must be served over HTTPS

Utknąłem już chyba na początku...
Przerobiłem zmiene na obiekty w ten sposób:

var wood = {};
	wood.price = document.querySelector('#woodPrice');
	wood.item = document.querySelector('#wood');
	wood.ownedWood = document.querySelector('#ownedWood');

I wziąłem się za przerobienie funkcji kupowania, na razie zrobiłem tyle:

function buyMaterial(chooseMaterial) {
	var chooseMaterial = document.querySelector('#chooseMaterial');
	material = chooseMaterial.innerHTML.toLowerCase()
	chooseMaterialValue = material.price * amount.value
	return chooseMaterialValue
};

I tu pojawia się problem, bo funkcja nic nie zwraca. Przy debugowaniu w konsoli doszedłem do tego że problemem jest material.price. Material zwraca się prawidłowo, ale już material.price jest undefined, przy material.price.innerHTML w ogóle wyskakuje error. Z początku myślałem, że wielkość liter ma znaczenie, bo samo chooseMaterial.innerHTML zwraca "Wood", więć nie nie będzie czegoś takiego jak Wood.price ale okazuję się że toLowerCase() nie pomogło/ Jakaś podpowiedź?

Edit:
Poprawiłem jeszcze bardziej jsfiddle do bardziej czytelniejszej opcji. Dalej nie mogę sobie poradzić z odwołaniem się z argumentu funkcji do parametru obektu.
https://jsfiddle.net/malkrzysztof/zmfxbcu5/24/

Wydaje mi się, że wiem, w czym jest problem. Wybacz chaos w odpowiedzi, ale co chwila ktoś przychodził, zagadywał i odrywał od pisania :(

Pierwsza sprawa - powinieneś sprawdzać podczas klikania "buy" albo "sell" czy jakiś materiał został wybrany. Obecnie można sprzedawać/kupować nawet wtedy, gdy pole "Choose material:" jest puste. Moim zdaniem te przyciski powinny się aktywować dopiero po wybraniu materiału (być może w "pełnej" wersji tak jest, nie wiem, nie sprawdzałem - ale piszę na wszelki wypadek), ewentualnie powinien być jakiś komunikat o tym, że należy wybrać surowiec.

Po drugie - skoro dodajesz obsługę zdarzenia w postaci

btnBuy.addEventListener("click", function(){
	buyMaterial();
});

a następnie w samej procedurze masz

function buyMaterial(which) - to skąd ma się wziąć jakaś wartość "which"? Argumenty funkcji mają sens wtedy, gdy funkcje się wywołuje z jakimś argumentem/parametrem, w obecnej postaci ten "which" jest nieokreślony, więc po prostu można uznać, że go nie ma.

Po trzecie- zmieniając postać funkcji na poniższą:

  function buyMaterial() {
		alert(chooseMaterial.innerHTML.toLowerCase());
}

widać, że jeśli niczego nie wybrałeś, to okienko z komunikatem jest puste, ale po wybraniu np. wood - pojawia się komunikat "wood". O czym to świadczy? Że mamy wybrany materiał - czyli to działa. Ale jak teraz możemy się dostać do jego ceny?

W sumie to zacząłem pisać wyjaśnienie, ale trochę "skomplikuję" sprawę. Wprawdzie będzie to wymagać od Ciebie trochę kolejnych zmian, ale za to całość będzie porządnie napisana. Zamiast odczytywać dane z odpowiednich DIV'ów czy SPAN'ów na stronie, czy nie lepiej by było posiadać jakąś zmienną, w której byś trzymał aktualnie wybrany surowiec, i z której byś odczytywał aktualną cenę? Postaraj się sam to zrobić, a potem możesz rzucić okiem na punkt numer osiem :)

Po czwarte - znowu mamy kwestie parametrów funkcji. Fragment

btnPrice.addEventListener("click", function(){
	woodPrices();
  ironPrices();
});

Ja bym tutaj zrobił jedną funkcję w postaci changePrice i albo dał parametr określający, który materiał ma być zmieniony, albo (zakładając, że zawsze wszystkie ceny są zmieniane) po prostu jedną funkcję, która zmienia wszystko od razu.

Po piąte - w jsfiddle w sekcji "JavaScript" po kliknięciu rozwijalnego menu "JavaScript + No-Library (pure JS)" wybierz "load type" - "bottom of <BODY>".

Po szóste - zmieniłem niektóre przypisania funkcji do zdarzeń, zamiast eventListener dałem onclick w sekcji HTML. Zrobiłem to po to, żeby skrócić kod i zwiększyć jego czytelność. Wprawdzie w onclick nie ma niczego złego, ale stosowane przez Ciebie podejście jest raczej rekomendowane - to, co zrobiłem to raczej na czas prac, potem śmiało możesz wrócić do tego, co było pierwotnie.

Po siódme - zrobiłem, że cały element <P> jest klikalny - tak będzie chyba łatwiej i bardziej intuicyjnie.

Po ósme - w sumie to jak zacząłem przerabiać, to pojawiało się coraz więcej rzeczy niepotrzebnych, do poprawy itp. Ale ponieważ nie mam czasu na wnikanie w szczegóły tego, co "autor miał na myśli" - zrobiłem to od nowa. Wybacz :(
Wersja działająca i po przepisaniu jest dostępna tutaj - https://jsfiddle.net/kuLds4gq/

P.S. sorry za małe opóźnienie z odpowiedzią, ale przez kilka dni byłem tzw. "słomianym wdowcem", baba mi wyjechała i zostałem sam z dzieciakami :D

Ok, wielkie dzięki za odpowiedź i poświęcony czas. Troche namieszałem z tym co podałem w jsfiddle, ponieważ stowrzyłem tam trochę co innego, ale mimo wszystko oddającego problem.
W między czasie zająłem się wszystkimi (mam nadzieje że nic nie pominąłem) sugestiami dotyczącymi styli i rozgrywki. Poza tym doszedłem chyba co było nie tak w dostępie funkcji do parametru obiektu i pokrywa się wydaję mi się z tym co właśnie napisałeś. Zrobiłem tak:

function buyMaterial(material) {
	material = chooseMaterial.innerHTML.toLowerCase();
	materialPrice = this[material].price.innerHTML
	materialValue = materialPrice * amount.value
	console.log("Material is: " + material + ", materialPrice is: " + materialPrice + ", materialValue is: " + materialValue)
	if (money.innerHTML >= materialPrice && wareHouseLimit.innerHTML >= (parseInt(wareHouse.innerHTML) + parseInt(amount.value))){
				money.innerHTML = money.innerHTML - materialValue
				wareHouse.innerHTML = parseInt(wareHouse.innerHTML, 10) + parseInt(amount.value, 10)
				this[material].owned.innerHTML = parseInt(this[material].owned.innerHTML, 10) + parseInt(amount.value, 10)
				// console.log("buying " + material + " for " + this[material].price.innerHTML * amount.value + " $")
				// console.log(this[material].owned)
				amount.value = null;
				infoBox.innerHTML += '<p>You buy ' + material +  ' for '  + materialValue +  ' $ </p>'
			} else {
				infoBox.innerHTML += '<p>You don`t have enought money or space in warehouse to buy ' + material +  ' for '  + materialValue +  ' $</p>'
		};
};

Oczywiście cały kod do przejrzenia na githubie.
Problem teraz mam taki, że logika kupowania i sprzedawania niby działa, ale czasem dzieją się rożnie rzeczy np. mogę kupować więcej niż mam pieniędzy (schodzi na minus), ale jak zmienie surowiec to już warunek z powrotem zaczyna sprawdzać czy mam wystarczającą ilość kasy, albo wywala błąd że nie mam miejsca w magazynie jak mam i tutaj tak samo po zmianie np. surowca wszystko wraca do normy. Podejrzewam, że to może być problem dotyczący właśnie przechowywania wartości zmiennych o których pisałeś, może to o czym piszesz w punkcie 3 będzie lekarstwem.
Zastanawiam się tylko jak do tego podejść, w tej chwili zmienna odczytuje wartość w spanie, a chodzi o to żeby wartość spanu była wartością zmiennej ?

Ok, wracam z tematem, btw zaraz ten wątek powinien być pewnie przeniesiony do działu newbie :D

github: https://github.com/malkrzysztof/stock
jsfiddle: https://jsfiddle.net/malkrzysztof/s6tfuLb2/6/

Poszedłem za radami, na początku szło nieźle, a teraz wydaje mi się że wracam do punktu wyjścia. Jak na to patrzę to zaczynam odnosić wrażenie że kod zaczyna rozrastać się w szybkim tempie a ja przestaje mieć nad tym kontrole.

• Zmieniłem w zasadzie swoje podejście do zmiennych, są one tak jak wg. podpowiedzi jako obiekty - wiele to ułatwia w tym przypadku.
• Przestałem (wydaje mi się) powtarzać niepotrzebnie kod.
• Zacząłem wywoływać funkcje z poziomu html.
• Przy zmianie wartości zmiennej od razu pod nią aktualizuje wyświetlanie jej w dokumencie. I tu mam wątpliwości czy to w ogóle słuszne podejście ?

Teraz mam problem z dwoma rzeczami:

1. funkcja gameOver, raz działa i wyświetla wynik - raz niekoniecznie. Na chwile obecną nie mogę dojść do przyczyny tego problemu, może znowu błędne podejście ?
2. Chciałem całą gre wrzucić w start() albo coś takiego, tak żeby gra zaczynała się od wpisania imienia i wybrania miasta początowego. Póki co nie za bardzo wiem jak się do tego zabrać. Myślałem o stworzeniu dwóch buttonów "start game" i "reset". Do pierwszego wrzucić całą logike gry a do drugiego funckje resetujące np. gameOver()?

Dobrze by było dostać w ogóle info czy idę w dobrą stronę czy wróciłem do punktu wyjścia.

Tak na szybko odnośnie wyglądu (piszę w oparciu o to, co jest na jsfiddle) - duży postęp, wygląda 100 razy lepiej niż na początku. Nie jest to może interface na miarę Facebooka, ale daje radę, poza tym należy docenić duże ulepszenie w stosunku do tego, co było wcześniej.

W temacie rozrastania się kodu - rzeczywiście, 1/3 albo więcej kodu to są deklaracje zmiennych. Wprawdzie nie jest to złe ani błędne, ale możnaby to było poprawić. Na chwilę obecną przychodzą mi do głowy dwa rozwiązania, które możesz zastosować.

1. stworzyć osobny plik JS, w którym będziesz trzymać jedynie deklaracje zmiennych. Wprawdzie ilość kodu się nie zmieni, ale będziesz miał oddzieloną część z deklaracjami od części, gdzie się dzieją "prawdziwe" rzeczy. Na pewno zwiększy to czytelność kodu (uwaga - na jsfiddle się chyba nie da tego tak zrobić, ale na GH albo u siebie lokalnie już powinno pójść).
2. Teraz znowu będę mieszał - najpierw sugerowałem zamianę luźnych i niepowiązanych ze sobą zmiennych na obiekty, a teraz wyskakuję z czymś nowym. Dlatego tego punktu nie traktuj jako sugestii do wdrożenia, ale raczej jako coś, co kiedyś będzie mogło Ci się przydać. W każdym razie - nie namawiam do zmieniania niczego w kodzie, a jedynie pokazuję ciekawostkę. O ile zapis obiektowy jest czytelniejszy i podczas pracy nie ma z nim za wiele problemów, to rzeczywiście deklarowanie tych wszystkich wartości jest lekko uciążliwe. Dlatego, zamiast obiektów, można zastosować tablice. W tym momencie przypisanie wartości podczas deklaracji zmiennej wyglądałby trochę inaczej.

var madrit = {}
	madrit.name = "Madrit"
	madrit.woodPriceMin = 30;
	madrit.woodPriceMax = 37;
	madrit.woodPriceHolder = document.getElementById("woodPrice");
	madrit.ironPriceMin = 37;
	madrit.ironPriceMax = 45;

ZAMIENIA SIĘ NA:

var madrit = ["Madrit", 30, 37, Id, 37, 45]

Przy czym jest to taka pozorna oszczędność, bo poza tym, że sama deklaracja wartości jest znacznie krótsza, właściwie niczego nie zyskujesz. Oszczędność w dalszej części są znikome. Wprawdzie zamiast pisać madrid.woodPriceMin napiszesz madrid[1] więc niby kilka znaków mniej, ale za to tracisz totalnie plus opisowych nazw zmiennych. W obecnej postaci patrząc na nazwę zmiennej od razu wiesz, za co ona odpowiada. Po zastosowaniu tablic raczej bez sprawdzania w komentarzach albo innych źródłach, nikt nie będzie miał pojęcia, co się kryje pod hasłem madrid[5] - dlatego potraktuj to raczej jako ciekawostkę, jakiś nowy temat, o istnieniu którego się dowiedziałeś ;)

"Zacząłem wywoływać funkcje z poziomu html." - żebyśmy się rozumieli: dodawanie obsługi zdarzeń przez addEventListener nie jest żadnym błędem. Zdania są podzielone - niektórzy uważają oba rozwiązania za równorzędne, inni skłaniają się ku jednej z nich (częściej do listenera). Mi chodziło wcześniej o to, że (przynajmniej obecnie - w fazie poprawek i testowania) zapis "na sztywno" w HTML jest czytelniejszy, od razu widać patrząc na deklarację elementu, jaka akcja jest do niego przypisana. Sam osobiście wolę ten sposób zapisu, ale wcale nie oznacza to, że wcześniej miałeś źle ;)

Porada - obecnie jak wpisze się w pole coś, co nie jest liczbą, to wyskakuje komunikat "u dont have money or enought space in warehouse". Możesz (jak się uporasz z pozostałymi kwestiami) dodać sprawdzenie i jeśli będzie wpisane coś, co nie jest liczbą - powinien się pojawić stosowny komunikat. Albo jeszcze lepiej - dodaj obsługę zdarzenia onkeyup (chyba tego - niedawno do jakiejś aplikacji dodawałem funkcjonalność, która zamieniała kropki na przecinki i tak coś mi świta, że chyba nie działało to poprawnie dla onkeydown, ale właśnie up) dla tego pola tekstowego i jeśli wprowadzony znak jest inny niż cyfra, to go kasuj :)

"Przy zmianie wartości zmiennej od razu pod nią aktualizuje wyświetlanie jej w dokumencie." - nie rozumiem o co Ci chodzi. Czy możesz napisać to jaśniej - o jaką zmienną chodzi, gdzie ją wyświetlasz i czemu to ma służyć?

"funkcja gameOver, raz działa i wyświetla wynik - raz niekoniecznie" - abstrahując od tego, z czego ten błąd może wynikać, czy jesteś w stanie zaobserwować, kiedy ona nie działa? Może podczas pierwsze, drugiej albo kolejnej gry? Czy zauważyłeś jakieś inne okoliczności/czynniki, które mogą mieć z tym jakiś związek?

"Chciałem całą gre wrzucić w start() albo coś takiego" - rozwiń myśl.

"stworzeniu dwóch buttonów "start game" i "reset". Do pierwszego wrzucić całą logike gry a do drugiego funckje resetujące np. gameOver()?". Nie do końca rozumiem, co chcesz zrobić. Jeśli chodzi o odpalenie gry po wciśnięciu start - jest to do ogarnięcia, ale pytanie - po co takie coś robić? Skro gra nie jest na czas, to można przyjąć, że załadowanie strony jest tożsame z wciśnięciem przycisku "start". Ewentualnie bym może wprowadził konieczność podania imienia przed rozpoczęciem gry i dopiero po jego podaniu umożliwiłbym granie. Możesz nawet całą grę ukryć i jedyne co będzie widoczne to będzie pole do podania imienia. Po jego podaniu byś przełączył - tzn. chowamy pytanie o imię, ale za to pokazujemy grę. Niedawno wyjaśniałem komuś jak można pokazywać/chować elementy (miał 2 div'y i przełączał się między nimi - idealne rozwiązanie dla Ciebie, jeden div do imienia, a drugi z grą) - Skrypt który podmienia cały kontener za pomocą dwóch przycisków. Co do guzika reset - powinien on po prostu "zerować" stan gry, czyli w Twoim przypadku przenosić do rundy zero, przywracać kasę do wartości startowej oraz zerować stany magazynowe.

"czy idę w dobrą stronę czy wróciłem do punktu wyjścia." moim zdaniem idziesz w bardzo dobrym kierunku. Jeśli masz jeszcze gdzieś swoją pierwszą wersję, w której miałeś każdą funkcję kilak razy powtórzoną dla poszczególnych wariantów miast/surowców, gdzie był chaos i pełno luźnych zmiennych, to porównaj ją z wersją obecną, a sam zrozumiesz, że teraz jest OK. Super załapałeś, o co chodzi z funkcjami i ich parametrami. W sumie (aczkolwiek przyznaję się bez bicia, że bardzo dokładnie nie analizowałem tego, co napisałeś) to mam jeszcze dwie sugestie w tym zakresie:

1. chodzi mi o poniższą funkcję

function updateOwnedMaterial(which){
	switch (which) {
		case wood:
			document.getElementById("ownedWood").innerHTML = wood.owned
			break
		case iron:
			document.getElementById("ownedIron").innerHTML = iron.owned

a jakby do tych zmiennych wood. iron itp. dodać jeszcze jedno pole (nazwij je jak uważasz, ja je nazwałem kaszanka ;) ) Na początku skryptu byśmy zrobili przypisanie wartości w postaci

wood.kaszanka = document.getElementById("ownedWood").innerHTML;
iron.kaszanka = document.getElementById("ownedIron").innerHTML

i tak dla wszystkich surowców, a następnie zmienilibyśmy podaną powyżej funkcję tak, aby się mieściła w 2 liniach i miała postać

function updateOwnedMaterial(which){
which.kaszanka = which.owned;

Wydaje mi się, że to powinno działać, ale jakoś jestem dość mocno zamulony, za chwilę idę spać, więc być może piszę totalne głupoty, a jutro będzie mi z tego powodu wstyd. W każdym razie - możesz pokombinować, są szanse, że się uda :P

I ostatnia uwaga, tym razem dotyczy funkcji makeZero. Obecnie ona wygląda tak:

function makeZero (){
	wood.price = 0
	woodPrice.innerHTML = 0

	iron.price = 0
	ironPrice.innerHTML = 0

A co jakby ją rozbić na dwie, żeby całość wyglądała mniej-więcej tak:

function wyczysc(which, which2) {
     which.price = 0;
     which2.innerHTML = 0;
}

function makeZero (){
     wyzcysc(wood, woodPrice);
     wyczysc(iron, ironPrice);