aaronvark / PeerReview1819

Repo for peer review assignments for year 2 development class of 18/19
0 stars 0 forks source link

Stijn van Deijzen - Blok 1 - Eindopdracht #18

Closed StijnOnline closed 4 years ago

StijnOnline commented 5 years ago

Issue aangemaakt, nog geen concept

StijnOnline commented 5 years ago

Mijn concept is Tetris + Bubble shooter maar dan:

Hieronder een UML voor zover ik kwam, ik vond het lastig het helemaal te bedenken voordat ik ermee bezig ben. Ik zal nieuwere versies maken verder in het proces.

Retro game v1

Ik ga nu een werkende basis proberen te maken

aaronvark commented 5 years ago

Zorg ervoor dat je nog een pull-request doet vanaf je branch naar de master, en daarin de description wel een #18 zet, want anders komt het hier niet terecht (en dat is wel de bedoeling).

Als het zo is dat de speler 1 block in zijn bezit heeft, dan zou het composition pijltje andersom moeten staan (nu staat er dat 1 block de speler heeft). Ook zouden member variables (zoals _color) niet met een underscore moeten zijn. Variabelen die je aanmaakt in een functie, en die na de functie "out of scope" gaan, geef je een underscore.

StijnOnline commented 5 years ago

TODO: Option to save a block for later Object Pooling Points go to the players' side the block is on Write plan for next week

StijnOnline commented 5 years ago

Ik heb momenteel alle main mechanics werkend, nu moet ik nog UI en beter visuals toevoegen. Daarna kan ik nog meer OOP, patterns, language features, etc. proberen toe te passen in het maken van powerups/powerblocks (Block class kan dan ge-inherit worden). Pull Request volgt.

aaronvark commented 5 years ago

Je bent in ieder geval goed op dreef qua features! Dat geeft je genoeg tijd om te experimenteren met structurele refactors.

Een ding waar ik bijvoorbeeld zou beginnen is de Middle class. Eigenlijk heeft die weinig responsibilities (hij moet een beetje draaien), het is vooral een ding waar blokjes naartoe getrokken moeten worden. Toch is die class van alles aan het doen. Hoe ik dit zou veranderen is dat je de Middle juist alleen als een object behandeld, dus bijv. dat het een variabele is die via de GameManager is op te vragen. En dan zou ik die gravitatie naar dat punt juist door de blokjes zelf laten regelen. Het midden kan dan ook op zichzelf draaien, en misschien is die variabele van de rotatie snelheid dan ook gewoon door de GameManager in te stellen op de Middle class, in plaats van dat Middle ook weer verhaal moet gaan halen bij de GameManager (dus meer een dependency injection, waar ik het maandag ook nog over ga hebben).

In de PlayerInput class heb je ook weer een dergelijke verbinding. Eigenlijk doet de GameManager te veel, en moet je de classes die je nu hebt onafhankelijker maken van de GameManager. Nu vormt de GameManager een "spil" tussen bijna alle responsibilities, en dus creëert het daardoor allerlei onnodige dependencies. Hier wat uitleg over waar dat uiteindelijk op kan gaan lijken: https://sourcemaking.com/antipatterns/the-blob

Altijd als ik powerups hoor, denk ik "decorator pattern". Misschien goed om eens te kijken of dat past, hier een kort voorbeeld van iemand anders: https://exceptionnotfound.net/decorator-the-daily-design-pattern/ Houd er wel rekening mee dat het misschien NIET past, dus kijk in je ontwerp of het inderdaad het gedrag oplevert dat je zoekt. In dit geval: terwijl het spel al draait objecten extra mogelijkheden geven. Dit is iets wat o.a. in games zoals Borderlands goed werkt omdat je randomized weapons hebt, en dus "gedrag combineert at runtime".

StijnOnline commented 5 years ago

Vandaag helaas geen toevoegingen, die komen later dit weekend

StijnOnline commented 4 years ago

Ik ben vergeten dit weekend een pull-request te doen, die komt nu tegelijk. Ik loop eigenlijk een beetje vast op motivatie, ik heb in principe iets werkends maar weet niet of de code goed is en hoe het beter zou kunnen. Dus als het kan zou ik graag uitgebreid feedback hebben op wat ik nu heb, en wat ik nog zou kunnen toevoegen om nog wat meer patterns etc. te gebruiken. Verder zal ik in de les nog met Valentijn spreken.

StijnOnline commented 4 years ago

Pull request staat nogsteeds open op #53

aaronvark commented 4 years ago

Goed dat je nog expliciet om feedback vraagt! Als ik door je code heen ga zie ik al een goede ontwikkeling in dat de classes al een stuk zelfstandiger geworden zijn. Wat ook opvalt is dat niet alles vanzelfsprekend is, bijvoorbeeld:

if (input[3] == 1) { SaveBlock(); }

is vrij vaag, omdat het allemaal nummers zijn en geen namen heeft. Dit zou je kunnen omzetten naar een enum. Als ik ga kijken in de PlayerInput dan vind ik daar wel de uitleg die ik verwacht, dus dat is goed! Hier is dus de kans om een enum te gebruiken, bijvoorbeeld:

public enum InputType
{
  HORIZONTAL,
  VERTICAL,
  ROTATE,
  SAVE
}

Die enum kan je dan gebruiken om de lijst aan te spreken, maar ook in de Player class, bijvoorbeeld:

if (input[(int)Input.SAVE] == 1) { SaveBlock(); }

Dit zou je ook nog kunnen omzetten tot een vraag die stelt aan de PlayerInput class, en dan zou het er ook zo uit kunnen zien:

//input is hier een variabele van het type PlayerInput, en die class heeft dan een functie Get waar je met een enum input ophaalt
//en playerIndex zou dan een variabele 0/1 zijn afhankelijk van voor welke speler je het opvraagt. Dit is dan een variabele die hoort bij de player class (nu is dit iets dat de GameManager doet).
if ( input.Get(playerIndex, Input.Save) == 1) { SaveBlock(); }

Dat maakt alles wat leesbaarder, en geeft de PlayerInput class meer eigendom over de input[], waardoor die niet altijd rondgestuurd hoeft te worden. Binnen PlayerInput kan je dan nog overwegen om de data handig te structureren. Nu check je de index van de player die je opvraagt, maar je zou dit ook met een Dictionary kunnen oplossen. Voor nu zou ik dat nog niet direct doen, maar het is wel een idee om bijvoorbeeld een structuur te maken zoals:

private List< Dictionary< Input, string > > playerControls = new List< Dictionary< Input, string > >();

Als je die dan tijdens Awake o.i.d. vult met de juiste data, dan kan je die als volgt gebruiken:

//dit haalt de lijst op voor de juiste speler
//playerControls[ playerIndex ]
//dit vraagt van die lijst direct de gevraagde input op (dus uit de dictionary die erin zit)
string axisName = playerControls[ playerIndex ][ inputType ];
//playerIndex is een int, 0 of 1.. inputType is een enum van het type Input (zoals hierboven aangegeven)
//en dan gebruik je die
return Input.GetAxis(axisName);

Wat ik ook terug zie als mogelijke verbetering, is waar arbitraire (niet essentiele, of erg vaste) eigenschappen best belangrijke dingen bepalen, zoals dit:

if (transform.position.x < 0)
{
  GameManager.Instance.players[0].AddScore(30);
}
else
{
  GameManager.Instance.players[1].AddScore(30);
}

Als het ooit moet voorkomen dat iemand punten moet krijgen van zijn eigen blokken, of voor wat voor reden dan ook anders dan links/rechts in het scherm, dan moet je dit compleet aanpassen. Ook is deze GameManager.Instance.player[1].... eigenlijk niet aan te raden, en is dit typisch een punt dat je een delegate/action zou gebruiken. Het blokje laat dan weten dat er gescoord moet worden, en wie daar naar luistert (bijv. de GameManager) die deelt dan de score uit. Nu is de Block class highly coupled met de GameManager.

Ook zal je bij "if (gameObject.GetInstanceID() > _other.GetInstanceID())//only 1 object will execute, hopefully" tegenkomen dat dit heel goed 2x kan plaatsvinden. Want als je 3 nummers hebt zijn er altijd 2 groter dan anderen. Wat je dan kan doen is de check 2x uitvoeren. A > B -> uitkomst (A of B) > C -< uitkomst daarvan (( A of B ) of C) voert het uit.

Om niet alleen kritiek te leveren: je toepassing van de delegate voor de attract functie is slim gevonden! Ik was al op zoek naar die Middle class, maar dit is best een goede oplossing.

Qua structuur zit er dus al verbetering in, maar kijk dus vooral of je, als dingen het eenmaal doen, classes minder afhankelijk van elkaar kunt maken. Dat is een constant proces van verbetering, dus blijf hier aan werken. Daarnaast kan je echt nog meer doen met DataStuctures (zoals lists, dictionaries, etc.) zodat de herhalende code (regels achter elkaar die op elkaar lijken, of if-statements zoals in PlayerInput) omlaag wordt gebracht, en de structuur van je data vooral aansluit op wat je wilt doen.

StijnOnline commented 4 years ago

Bedankt voor de feedback, ik heb echter vandaag tijdens de les redelijk wat omgegooid dus zal ik nog zien in hoeverre alles relevant is.

StijnOnline commented 4 years ago

Pull request staat nogsteeds open op #53 Hieronder alle files, de Build is om gekke redenen (physics is glitched) praktisch onspeelbaar Mijn hele Project Build Documentatie TetrisShooter UML v3