Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add GameHandler class #14

Merged
merged 5 commits into from
Feb 8, 2021
Merged

Add GameHandler class #14

merged 5 commits into from
Feb 8, 2021

Conversation

AleksandraCyp
Copy link
Collaborator

No description provided.

Comment on lines 23 to 27
if (!allHPs.includes(0)) {
return false;
} else {
return true;
}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this logic says that if there exists at least ONE pokemon that has 0 hp - then the game is over - it should be different. If all three pokemons of one of players have 0 hp - then the game should be finished.

(includes method checks if there exists at least one such element)

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also - you shouldn't access private property "_currentHP" :) you should use "isAlive()" method

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also you can use ready getter from player class this._playerOne.alivePokemons and check if the list is empty

const getallHPs = (player: Player): number[] =>
player.pokemons.map((pokemon) => pokemon._currentHP);
const firstPlayerPokemonsHP: number[] = getallHPs(this._playerOne);
if (firstPlayerPokemonsHP.includes(0)) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"includes" refers to "one element"
[0, 50, 50].includes(0) - returns true, but should return false
[0,0,0].something() - should return true only in this case

if (!this.isGameFinished())
return new Error("You cannot get the winner. The game is not over.");
const getallHPs = (player: Player): number[] =>
player.pokemons.map((pokemon) => pokemon._currentHP);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as previously - use isAlive() instead of _currentHP

Comment on lines +14 to +15
if (this._currentPlayer === this._playerOne) return this._playerTwo;
return this._playerOne;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can flex your programming skills and astonish your peers by using ternary operator:

Suggested change
if (this._currentPlayer === this._playerOne) return this._playerTwo;
return this._playerOne;
return this._currentPlayer === this._playerOne ? this._playerTwo : this._playerOne;

import { Pokemon } from "./pokemonClass";

export class GameHandler {
constructor(private _playerOne: Player, private _playerTwo: Player) {}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice shorthand! Didn't know about it :)

Copy link
Owner

@lukaszdutka lukaszdutka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need some changes, there are bugs in this code. But some clever things also exists there :)


it("GameHandler.isGameFinished should return false if every player has at least one pokemon", () => {
// Given
const factory_0 = new PokemonFactory(pokeData);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what's with this naming convention?

each test is separated from others, so there is no need for this suffix _0, _1 etc.

Comment on lines 25 to 28
const isGameFinished_0 = new GameHandler(
playerOne_0,
playerTwo_0
).isGameFinished();
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GameHandler is part of given, in when there should be only isGameFinished()

Comment on lines 42 to 44
playerOne_1.pokemons.forEach((pokemon) =>
pokemon.subtractHP(pokemon.maxHP)
);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it looks more like when but it's discussable.

}

private didPlayerLoose(player: Player): boolean {
return player.alivePokemons.length < 1;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not simply .length == 0? 😕

Comment on lines 63 to 72
const playerOne_2 = new Player("Wojtek", [
factory_2.getPokemonByName("bulbasaur"),
factory_2.getPokemonByName("charmander"),
factory_2.getPokemonByName("pikachu"),
]);
const playerTwo_2 = new Player("Ania", [
factory_2.getPokemonByName("squirtle"),
factory_2.getPokemonByName("weedle"),
factory_2.getPokemonByName("pidgey"),
]);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can also create helper methods at the end of the file, like:

const getPlayerOne = () => {
 return new Player("Wojtek", [
      factory_2.getPokemonByName("bulbasaur"),
      factory_2.getPokemonByName("charmander"),
      factory_2.getPokemonByName("pikachu"),
    ])
}

and in test use only:

const playerOne = getPlayerOne();

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the reasoning is that:
"You don't need to know exact details. You just need to know there are some players with some pokemons"

then, in the when section you can modify it a bit (like zero all the pokemon healths)

Copy link
Owner

@lukaszdutka lukaszdutka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work, cool tests! However, there are some readability changes that could be made :)

Comment on lines +10 to +24
const getPlayerOne = (factory: PokemonFactory) => {
return new Player("Wojtek", [
factory.getPokemonByName("bulbasaur"),
factory.getPokemonByName("charmander"),
factory.getPokemonByName("pikachu"),
]);
}

const getPlayerTwo = (factory: PokemonFactory) => {
return new Player("Wojtek", [
factory.getPokemonByName("bulbasaur"),
factory.getPokemonByName("charmander"),
factory.getPokemonByName("pikachu"),
]);
}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if both players are the same - why bother to have two identical functions? :P


it("GameHandler.isGameFinished should return false if every player has at least one pokemon", () => {
// Given
const factory = new PokemonFactory(pokeData);
Copy link
Owner

@lukaszdutka lukaszdutka Feb 8, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that factory can be global, like getPlayerOne function.
Because using factory should be pure, factory.getPokemonByName should be (and is) pure function - it always returns the same (the same pokemon with same stats) for the same input (pokemon name)

Comment on lines +48 to +49
playerOne.pokemons.forEach((pokemon) =>
pokemon.subtractHP(pokemon.maxHP));
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can be in one line, but whatever :P

Comment on lines +81 to +83
expect(winner).toThrowError(
"You cannot get the winner. The game is not over."
);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it also can be in one line

Comment on lines +77 to +78
const winner = () =>
gameHandler.getWinner();
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it also can be in one line

Comment on lines +94 to +95
playerTwo.pokemons.forEach((pokemon) =>
pokemon.subtractHP(pokemon.maxHP));
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it also can be in one line

Copy link
Owner

@lukaszdutka lukaszdutka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🏆

@lukaszdutka lukaszdutka merged commit 3a67f72 into master Feb 8, 2021
@lukaszdutka lukaszdutka deleted the MNG-95-createGameHandler branch February 8, 2021 07:11
lukaszdutka pushed a commit that referenced this pull request Feb 12, 2021
* Function RenderChoosePokemonScreen

* MNG-102 fight method implemented

* MNG-102 Add tests for fight class. Fix pokemon subtract HP

* Appendig function to the whole index.ts

* MNG-88_StartButtonActivation (#12)

* Add keyup to input

* Add activateStart.ts

* Uncomment html

* Add class to activate/disable start button

* Add activateStartButton function

* Change class name to button-enabled

* Corrections

* MNG-89   clicking start should render choose pokemon screen (#17)

* Function RenderChoosePokemonScreen

* Appendig function to the whole index.ts

* Corrections

* MNG-95 Add GameHandler class (#14)

* Add GameHandler class

* Fix GameHandler class and add tests

* Fix tests

* MNG-107 Add geodude instead of tentacruel

Filter moves to 4  for each pokemon
Use mostly pokemon type moves

* MNG-103 choose pokemon system (#19)

* Add choosePokemonsPage.ts

* Add all functions for choose pokemon page

* Change event.target, loading time

* MNG-94 MNG-108 Start a new game, add functions that will render the correct view (#20)

* MNG-99 & MNG-100

> created menu and back button with options.
Options need to be fixed (now are default).
Animation also needs to look better

* MNG-109 added

* Blocking the start with unwritten name

* added magicFunction

* added animations for buttons

* Style button

* Blocking button when the name is too long

* MNG-98 Add useMango to gameHandler class (#22)

* Add useMango to gameHandler class

* corrections type of mango value

* Corrections

* useMango i PlayerClass

Co-authored-by: AleksandraCyp <73715885+AleksandraCyp@users.noreply.github.com>

* MNG-97 MNG-101 change player&change pokemon (#24)

* Add switch pokemon and switch Player game handler methods

* Implement mango, change the structure (build up magic function)

* remove an unnecessary comment

* Update test parametrization and do not use json data

* MNG-102 fight method implemented

* MNG-102 Add tests for fight class. Fix pokemon subtract HP

* Update test parametrization and do not use json data

* Apply max shortcut

Co-authored-by: AgataLudwiczynska <74932979+AgataLudwiczynska@users.noreply.github.com>
Co-authored-by: mariusz-sm <74978639+mariusz-sm@users.noreply.github.com>
Co-authored-by: AleksandraCyp <73715885+AleksandraCyp@users.noreply.github.com>
Co-authored-by: Gosia Dziewit <gosia.dziewit@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants