Skip to content

Conversation

@dmoreux
Copy link

@dmoreux dmoreux commented Jul 22, 2025

No description provided.

@AlexandreChaussard
Copy link
Collaborator

Thanks for the PR. I'm confused by the changes though. The levelUp method is supposed to be a forward method, in the sense that it's only pushing the chain of leveling towards the next level. Why do you need to have the previous level as input then? In the instance, if you call levelUp, the current level becomes the previous level leaving spot for the level up, if that makes sense.

From my understanding, only PetLevelUpEvent constructor would "need" the owner to be provided, even though I would rather not add it as input but instead just check that the owner is not null before leveling up. At least that appears to me like a better practice, even though I'm not a dev so I'm not well qualified to judge this 👍

@AlexandreChaussard AlexandreChaussard added the invalid This doesn't seem right label Jul 22, 2025
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR enhances the PetLevelUpEvent class by adding missing data fields to improve event handling capabilities. The changes address a null owner issue and provide access to the previous level information.

  • Added owner and oldLevel fields to PetLevelUpEvent with corresponding getter methods
  • Updated the event constructor to accept the new parameters
  • Modified the level-up flow to pass the old level information and reordered operations for better data consistency

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
PetLevelUpEvent.java Added owner and oldLevel fields with getters and updated constructor signature
PetStats.java Reordered level-up operations to call levelUp() before updating current level
PetLevel.java Updated levelUp() method to accept oldLevel parameter and pass it to the event

Comment on lines +10 to +11
import java.util.UUID;

Copy link

Copilot AI Jul 22, 2025

Choose a reason for hiding this comment

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

[nitpick] The UUID import should be grouped with other Java standard library imports. Consider placing it after the existing imports or following the project's import ordering conventions.

Suggested change
import java.util.UUID;

Copilot uses AI. Check for mistakes.
private final UUID owner;

public PetLevelUpEvent(Pet pet, PetLevel petLevel) {
public PetLevelUpEvent(Pet pet, PetLevel petLevel, PetLevel oldLevel, UUID owner) {
Copy link

Copilot AI Jul 22, 2025

Choose a reason for hiding this comment

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

This constructor change introduces a breaking change to the public API. Existing code calling the old constructor will fail to compile. Consider providing both constructors for backward compatibility or implementing a builder pattern.

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

invalid This doesn't seem right

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants