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.

Sign up for GitHub

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

Updating Game List to Display Players Username #1130

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Updating Game List to Display Players Username #1130

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f9e60df
fixing
zanzoonh Dec 12, 2024
a3503dd
added changes to gameList.js file and updated certain tests
zanzoonh Dec 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion api/helpers/ find-spectatable-games.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,17 @@ module.exports = {
const games = await Game.find({
status: gameService.GameStatus.STARTED,
updatedAt: { '>=': recentUpdateThreshhold },
}).populate('players');

// Map players to include only id and username
const transformedGames = games.map(game => {
game.players = game.players.map(player => ({
id: player.id,
username: player.username,
}));
return game;
Comment on lines +17 to +25
Copy link
Contributor

@itsalaidbacklife itsalaidbacklife Dec 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This works and does what the issue described, but can we actually refactor it to use the single-valued attributes of p0 and p1 and to then combine them and transform them into an array?

Right now the Game model has:
p0: the user who is player 0
p1: the user who is player 1
players: the collection of [p0, p1].

players is redundant and being deprecated for removal. The main reason for this is that at the database level, the players collection works by having the user table use a foreign key to the game table, which therefore imposes the constraint that a user can only be in one game at a time (something we'd like to change). Conversely, p0 and p1 are columns in the game table which are foreign keys to the user table, allowing any given user to be in as many games as desired. We plan to refactor the entire codebase to remove the use of .players from the backend and replace it with .p0 and .p1.

As one additional note, when using Array.map(), we generally avoid mutating the original array as this can have unintended consequences. In this case we are producing the list within the function and transforming it just to return so it's not a horrible thing to do, but generally it's preferred to copy the object instead (usually with spread) rather than doing something like game.players = inside the .map().

In this situation, that would look like this

Suggested change
}).populate('players');
// Map players to include only id and username
const transformedGames = games.map(game => {
game.players = game.players.map(player => ({
id: player.id,
username: player.username,
}));
return game;
}).populate('p0')
.populate('p1');
// Map players to include only id and username
const transformedGames = games.map(game => {
const players = [game.p0, game.p1].filter((player) => !!player)
.map(player => ({
id: player.id,
username: player.username,
}));
const transformedGame = {
...game,
players,
};
return transformedGame;

});
return exits.success(games);
return exits.success(transformedGames);
} catch (err) {
return exits.error(err);
}
Expand Down
27 changes: 20 additions & 7 deletions api/hooks/customGameHook/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,24 +30,37 @@ module.exports = function gameHook() {
},
findOpenGames: function () {
return new Promise(function (resolve, reject) {
const recentUpdateThreshhold = dayjs.utc().subtract(1, 'day')
const recentUpdateThreshold = dayjs.utc().subtract(1, 'day')
Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch; thank you

.toDate();

Game.find({
status: gameService.GameStatus.CREATED,
createdAt: { '>=': recentUpdateThreshhold },
createdAt: { '>=': recentUpdateThreshold },
})
.populate('players')
.populate('players') // Populate the players array
.exec(function (error, games) {
if (error) {
return reject(error);
} else if (!games) {
} else if (!games || games.length === 0) {
Copy link
Contributor

@itsalaidbacklife itsalaidbacklife Dec 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

We can combine these with optional chaining and utilize the fact that 0 is falsey like so

Suggested change
} else if (!games || games.length === 0) {
} else if (!games?.length) {

return reject({ message: "Can't find games" });
}
const openGames = games.filter(({ players }) => players.length < 2);
return resolve(openGames);

// Filter games with fewer than 2 players and transform the players array
const openGames = games
.filter(({ players }) => players.length < 2) // Keep games with fewer than 2 players
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this comment is sufficiently explained by the one above the openGames assignment

Suggested change
.filter(({ players }) => players.length < 2) // Keep games with fewer than 2 players
.filter(({ players }) => players.length < 2)

.map((game) => {
// Transform players array to include only id and username
game.players = game.players.map((player) => ({
id: player.id, // Use 'id' since Sails.js generates it automatically
username: player.username,
}));
return game; // Return the updated game object
});

return resolve(openGames); // Resolve with transformed open games
Comment on lines +54 to +60
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks good. I think the above comments are helpful but the ones on the same line are not adding much clarity beyond what the line already says. Let's remove these.

Generally, our approach to commenting is to either:

  1. Explain anything weird
  2. Break out a larger thing into sections for quick scanning
Suggested change
id: player.id, // Use 'id' since Sails.js generates it automatically
username: player.username,
}));
return game; // Return the updated game object
});
return resolve(openGames); // Resolve with transformed open games
id: player.id,
username: player.username,
}));
return game;
});
return resolve(openGames);

});
});
},
},
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
},
},

findGame: function (id) {
return new Promise(function (resolve, reject) {
Game.findOne(id)
Expand Down
2 changes: 2 additions & 0 deletions src/routes/home/HomeView.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@
:game-id="game.id"
:status="game.status"
:num-players="game.numPlayers"
:players="game.players"
:is-ranked="game.isRanked"
@error="handleSubscribeError(game.id, $event)"
/>
Expand All @@ -87,6 +88,7 @@
:p1ready="game.p1Ready ? 1 : 0"
:game-id="game.id"
:status="game.status"
:players="game.players"
:num-players="game.numPlayers"
:is-ranked="game.isRanked"
:is-spectatable="true"
Expand Down
20 changes: 17 additions & 3 deletions src/routes/home/components/GameListItem.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
{{ name }}
</p>
<p v-if="!isSpectatable" class="text-surface-1">
{{ readyText }} {{ t('home.players') }}
{{ playersText }}
</p>
</v-col>
<v-col lg="6" class="list-item__button pr-md-0">
Expand Down Expand Up @@ -81,6 +81,10 @@ export default {
type: Number,
required: true,
},
players: {
type: Array,
required: true,
},
Comment on lines +84 to +87
Copy link
Contributor

@itsalaidbacklife itsalaidbacklife Dec 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

We should also remove numPlayers as a prop now that we can just use this.players.length

isRanked: {
type: Boolean,
default: false,
Expand Down Expand Up @@ -109,8 +113,18 @@ export default {
numPlayersReady() {
return this.p0ready + this.p1ready;
},
readyText() {
return `${this.numPlayers} / 2`;
playersText() {
const numPlayers = this.players.length;
switch (numPlayers) {
case 0:
return 'Empty';
case 1:
return `vs ${this.players[0].username}`;
case 2:
return `${this.players[0].username} vs ${this.players[1].username}`;
default:
return '';
}
},
joinButtonText() {
return `${this.t('home.join')} ${this.isRanked ? this.t('global.ranked') : this.t('global.casual')}`;
Expand Down
3 changes: 3 additions & 0 deletions src/stores/gameList.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ class GameSummary {
this.id = obj.id ? obj.id : null;
this.name = obj.name ? obj.name : null;
this.numPlayers = Object.prototype.hasOwnProperty.call(obj, 'players') ? obj.players.length : 0;
this.players = Object.prototype.hasOwnProperty.call(obj, 'players') ? obj.players : [];
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's also remove numPlayers above since its just computed from players.length

Copy link
Contributor

Choose a reason for hiding this comment

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

We will additionally need to update the places numPlayers are referenced and remove them accordingly

this.status = Object.prototype.hasOwnProperty.call(obj, 'status') ? obj.status : GameStatus.ARCHIVED;
this.isRanked = Object.prototype.hasOwnProperty.call(obj, 'isRanked') ? obj.isRanked : false;
this.isOver = false;
Expand Down Expand Up @@ -53,13 +54,15 @@ export const useGameListStore = defineStore('gameList', {
const updatedGame = this.openGames.find((game) => game.id === data.gameId);
if (updatedGame) {
updatedGame.numPlayers = Math.min(2, updatedGame.numPlayers + 1);
updatedGame.players.push(data.newPlayer);
updatedGame.status = data.newStatus;
}
},
otherLeftGame(gameId) {
const updatedGame = this.openGames.find((game) => game.id === gameId);
if (updatedGame) {
updatedGame.numPlayers = Math.max(0, updatedGame.numPlayers - 1);
updatedGame.players.pop();
Copy link
Contributor

Choose a reason for hiding this comment

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

This is actually a little trickier than you have it here and will require a full stack change. Specifically, we don't know which player left, so we can't just assume it was the one at the end of the array. We need to update

api/controllers/game/leave-lobby.js

So that its sails.sockets.blast('leftGame', { id: values[0].id }, req); call additionally specifies which user id left and then we need to update this otherLeftGame method to look up which specific user left the game and remove that one. It could look like this

    otherLeftGame(gameId, userId) {
      const updatedGame = this.openGames.find((game) => game.id === gameId);
      if (updatedGame) {
        updatedGame.players. = updatedGame.players.filter((player) => player.id !== userId);
      }
  }

}
},
setIsRanked({ gameId, isRanked }) {
Expand Down
6 changes: 3 additions & 3 deletions tests/e2e/specs/out-of-game/home.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -497,7 +497,7 @@ describe('Home - Create Game', () => {
cy.get('[data-cy=game-list-item]')
.should('have.length', 1)
.should('include.text', 'test game')
.should('include.text', '1 / 2 players');
.should('include.text', 'vs');

// Test store
cy.window()
Expand Down Expand Up @@ -534,7 +534,7 @@ describe('Home - Create Game', () => {
cy.get('[data-cy=game-list-item]')
.should('have.length', 1)
.should('include.text', 'test game')
.should('include.text', '1 / 2 players');
.should('include.text', 'vs');
// Test store
cy.window()
.its('cuttle.gameListStore.openGames')
Expand Down Expand Up @@ -572,7 +572,7 @@ describe('Home - Create Game', () => {
cy.get('[data-cy=game-list-item]')
.should('have.length', 1)
.should('include.text', 'test game')
.should('include.text', '1 / 2 players');
.should('include.text', 'vs');

// Test store
cy.window()
Expand Down