search

paulmis / qz

A cool energy consumption quiz app for desktop built with Spring Boot
3 stars 1 forks source link

API endpoint for joining/getting lobbies - [merged] #260

Closed paulmis closed 2 years ago

paulmis commented 2 years ago

In GitLab by @gpezzali on Feb 25, 2022, 24:03

Merges 75-api-endpoints-for-joining-getting-lobbies -> dev

Created the LobbyController and expose endpoints to get lobbies and join them.

Closes #75

paulmis commented 2 years ago

In GitLab by @ddinucujianu on Feb 25, 2022, 24:07

Commented on server/src/main/java/server/api/LobbyController.java line 43

I think you achieve the opposite effect here since you remove if the status of a game is created. I think you should make it be game.getStatus() != GameStatus.CREATED

paulmis commented 2 years ago

You can use @Autowired to automatically initialize the repository bean. The constructor below is then obsolete. Also, I'm not sure if the GameRepository should be final.

I would keep the full Repository in the name, this is what IntelliJ will autogenerate for new variables and ends up being more convinient.

paulmis commented 2 years ago

You can use Java streams to filter lists. Here it's along the lines of

lobbyRepo.findAll().stream().filter(l -> l.getStatus() == GameStatus.CREATED).collect()

However, with Spring repositories you can type functions that reminisce SQL queries, and for most of them Spring will autogenerate the required function in the repository without you even having to provide the interface. IntelliJ will also suggest these names when autocompleting. Here you could use something like:

lobbyRepo.findAllByGameStatus(GameStatus.CREATED)

This is more efficient than filtering since the filtering is done at DB level.

paulmis commented 2 years ago

In GitLab by @ddinucujianu on Feb 25, 2022, 03:44

Commented on server/src/main/java/server/api/LobbyController.java line 43

Also this is probably not the best way to do since you filter it in memory instead of having a query that does this which gets really slow and expensive in theory.

paulmis commented 2 years ago

In GitLab by @gpezzali on Feb 26, 2022, 17:35

Commented on server/src/main/java/server/api/LobbyController.java line 43

changed this line in version 2 of the diff

paulmis commented 2 years ago

In GitLab by @gpezzali on Feb 26, 2022, 17:35

Commented on server/src/main/java/server/api/LobbyController.java line 23

changed this line in version 2 of the diff

paulmis commented 2 years ago

In GitLab by @gpezzali on Feb 26, 2022, 17:35

Commented on server/src/main/java/server/api/LobbyController.java line 42

changed this line in version 2 of the diff

paulmis commented 2 years ago

In GitLab by @gpezzali on Feb 26, 2022, 17:35

added 1 commit

Compare with previous version

paulmis commented 2 years ago

In GitLab by @gpezzali on Feb 26, 2022, 17:57

added 42 commits

Compare with previous version

paulmis commented 2 years ago

In GitLab by @gpezzali on Feb 26, 2022, 18:24

added 1 commit

Compare with previous version

paulmis commented 2 years ago

In GitLab by @gpezzali on Feb 27, 2022, 10:18

added 4 commits

Compare with previous version

paulmis commented 2 years ago

In GitLab by @gpezzali on Feb 27, 2022, 10:28

resolved all threads

paulmis commented 2 years ago

We're using singular resource names in api paths, so it should be /api/lobby

paulmis commented 2 years ago

These don't really have to have comments :cat:

paulmis commented 2 years ago

You can also use ResponseEntity.ok(lobbies)

paulmis commented 2 years ago

This can be just @GetMapping('/available') since the default parameter is the path.

paulmis commented 2 years ago

The information about whether the request succeeded is already included in the ResponseEntity (i.e. 2xx vs 4/5xx codes). You can just use ResponseEntity. IntelliJ will complain about the use of raw parametrized class, but you can ignore that.

paulmis commented 2 years ago

JPA's get functions generally wrap single objects in Optional. The variable should be an Optional<Game> allowing you to instead check whether one was found with isPresent().

paulmis commented 2 years ago

If you're not re-using a variable in the future, you should move the call to inside of if to simplify the code, i.e. if (toJoin.add(player)) {. You can then put the return with the ResponseEntity inside this if.

paulmis commented 2 years ago

You only need to save the entity being modified, i.e. the game. Spring will automatically update any back references.

paulmis commented 2 years ago

In GitLab by @ddinucujianu on Feb 27, 2022, 13:35

Commented on server/src/main/java/server/api/LobbyController.java line 80

As of now, this doesn't work as the game repository throws an exception if it can't find the lobby with the given id. This null check will basically never come out to be true. Consider wrapping with a try-catch or better use the method Paul described and do it with optionals.

paulmis commented 2 years ago

In GitLab by @gpezzali on Feb 27, 2022, 16:54

Commented on server/src/main/java/server/api/LobbyController.java line 103

It complains, it says that the returned object from getById is of type Game, not Optional

paulmis commented 2 years ago

In GitLab by @gpezzali on Feb 28, 2022, 15:01

Commented on server/src/main/java/server/api/LobbyController.java line 35

changed this line in version 6 of the diff

paulmis commented 2 years ago

In GitLab by @gpezzali on Feb 28, 2022, 15:01

Commented on server/src/main/java/server/api/LobbyController.java line 52

changed this line in version 6 of the diff

paulmis commented 2 years ago

In GitLab by @gpezzali on Feb 28, 2022, 15:01

Commented on server/src/main/java/server/api/LobbyController.java line 67

changed this line in version 6 of the diff

paulmis commented 2 years ago

In GitLab by @gpezzali on Feb 28, 2022, 15:01

Commented on server/src/main/java/server/api/LobbyController.java line 61

changed this line in version 6 of the diff

paulmis commented 2 years ago

In GitLab by @gpezzali on Feb 28, 2022, 15:01

Commented on server/src/main/java/server/api/LobbyController.java line 95

changed this line in version 6 of the diff

paulmis commented 2 years ago

In GitLab by @gpezzali on Feb 28, 2022, 15:01

Commented on server/src/main/java/server/api/LobbyController.java line 103

changed this line in version 6 of the diff

paulmis commented 2 years ago

In GitLab by @gpezzali on Feb 28, 2022, 15:01

Commented on server/src/main/java/server/api/LobbyController.java line 125

changed this line in version 6 of the diff

paulmis commented 2 years ago

In GitLab by @gpezzali on Feb 28, 2022, 15:01

Commented on server/src/main/java/server/api/LobbyController.java line 130

changed this line in version 6 of the diff

paulmis commented 2 years ago

In GitLab by @gpezzali on Feb 28, 2022, 15:01

Commented on server/src/main/java/server/api/LobbyController.java line 80

changed this line in version 6 of the diff

paulmis commented 2 years ago

In GitLab by @gpezzali on Feb 28, 2022, 15:01

added 1 commit

Compare with previous version

paulmis commented 2 years ago

In GitLab by @gpezzali on Feb 28, 2022, 15:07

marked this merge request as ready

paulmis commented 2 years ago

In GitLab by @gpezzali on Feb 28, 2022, 15:09

Commented on server/src/main/java/server/api/LobbyController.java line 103

I used David's suggestion.

paulmis commented 2 years ago

In GitLab by @gpezzali on Feb 28, 2022, 15:09

resolved all threads

paulmis commented 2 years ago

Ah it should be findById.

paulmis commented 2 years ago

In GitLab by @gpezzali on Feb 28, 2022, 15:11

marked this merge request as draft

paulmis commented 2 years ago

Can be simplified to @GetMapping("/{lobbyId}")

paulmis commented 2 years ago

This repo shouldn't exist.

paulmis commented 2 years ago

Use findById with Optional checks instead.

paulmis commented 2 years ago

In GitLab by @gpezzali on Feb 28, 2022, 16:02

resolved all threads

paulmis commented 2 years ago

In GitLab by @gpezzali on Feb 28, 2022, 16:02

Commented on server/src/main/java/server/api/LobbyController.java line 68

changed this line in version 7 of the diff

paulmis commented 2 years ago

In GitLab by @gpezzali on Feb 28, 2022, 16:02

Commented on server/src/main/java/server/api/LobbyController.java line 46

changed this line in version 7 of the diff

paulmis commented 2 years ago

In GitLab by @gpezzali on Feb 28, 2022, 16:02

added 2 commits

Compare with previous version

paulmis commented 2 years ago

In GitLab by @gpezzali on Feb 28, 2022, 18:05

added 1 commit

Compare with previous version

paulmis commented 2 years ago

You can assume beans are mocks in test suites, so you should call these with their name (i.e. gameRepository).

paulmis commented 2 years ago

Do not use random values in tests, as this makes them non-deterministic. You can just do UUID.fromString.

paulmis commented 2 years ago

If you keep using the same code for multiple test cases it's a good sign you need to add an init method annotated with @BeforeEach. This will allow you to put these entities as fields of the test class and they will be initialized every time a new test case runs using the code from init.

paulmis commented 2 years ago

CrudRepository.save never returns null, it will instead throw an exception if the PK is violated, making this mock redundant.

paulmis commented 2 years ago

Inconsistent formatting. Spreading these builder expressions over is the way to go, but do it consistently. Here:

this.mockMvc
        .perform(
            put("/api/lobby/" + UUID.randomUUID() + "/join/" + UUID.randomUUID())
            .contentType(MediaType.APPLICATION_JSON)
            .content(objectMapper.writeValueAsString(player)))
        .andExpect(status().isNotFound());