Implement leaderboard model/entity and functions #100
Conversation
beeguy74
force-pushed
the
feat-Implement-leaderboard-model
branch
from
e50c23a to
623f676
Compare
|
Warning Rate limit exceeded@beeguy74 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 10 minutes and 28 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThis pull request introduces several significant changes to the bytebeasts application. A new dependency, Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
|
@coxmars Hello sir, can you look at my PR... 🙏 |
|
@beeguy74 at the end of the work, could you please add a pic of the outputs of
|
…derboard's test file moved to test directory.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (5)
src/models/leaderboard.cairo (3)
25-41: SimplifyPartialOrdimplementation using derived traitsThe manual implementation of
PartialOrdforLeaderboardEntrycan be simplified by deriving the trait automatically. This improves code readability and maintainability.Apply this change:
- //trait for sorting by score - impl LeaderboardEntryPartialOrd of PartialOrd<LeaderboardEntry> { - fn le(lhs: LeaderboardEntry, rhs: LeaderboardEntry) -> bool { - lhs.score <= rhs.score - } - fn ge(lhs: LeaderboardEntry, rhs: LeaderboardEntry) -> bool { - lhs.score >= rhs.score - } - fn lt(lhs: LeaderboardEntry, rhs: LeaderboardEntry) -> bool { - lhs.score < rhs.score - } - fn gt(lhs: LeaderboardEntry, rhs: LeaderboardEntry) -> bool { - lhs.score > rhs.score - } - } + #[derive(PartialOrd)]
118-118: Typographical error in commentThere's a typo in the comment on line 118: "addning" should be "adding".
Apply this fix:
- // addning new wins, losses and changing highest score to an old entry + // adding new wins, losses and changing highest score to an old entry
140-156: Inefficient search inget_index_by_player_idCreating a dummy
LeaderboardEntrywith default values to find the index byplayer_idmay lead to inefficiencies and potential bugs if fields other thanplayer_idare considered in equality checks in the future. Consider iterating over the entries and comparingplayer_iddirectly.Apply this change:
- let entry = LeaderboardEntry { - player_id: player_id, - player_name: '', - score: 0, - wins: 0, - losses: 0, - highest_score: 0, - is_active: false, - }; - match self.entries.index_of(entry) { + let mut index = 0; + let mut found = false; + while (index < self.entries.len()) { + if self.entries.at(index).player_id == player_id { + found = true; + break; + }; + index += 1; + }; + if found { + Result::Ok(index) + } else {src/tests/test_leaderboard.cairo (2)
10-10: Remove unused importbubble_sort_elementsThe import of
bubble_sort_elementsis not used in the test file. Removing it will clean up the code.Apply this change:
- use alexandria_sorting::bubble_sort::bubble_sort_elements;
37-37: Check the result ofadd_entryin testsIn
test_add_single_entry, the result ofleaderboard.add_entry(entry)is stored inresbut not used meaningfully. Consider asserting that the operation was successful to validate the test.Apply this change:
let res = leaderboard.add_entry(entry); + assert_eq!(res.is_ok(), true, "Failed to add entry");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
src/lib.cairo(1 hunks)src/models/leaderboard.cairo(1 hunks)src/tests/test_leaderboard.cairo(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/lib.cairo
🔇 Additional comments (5)
src/models/leaderboard.cairo (4)
159-169: Inefficient sorting with bubble sort
Using bubble sort (bubble_sort_elements) is inefficient for large datasets due to its O(n²) time complexity. Consider using a more efficient sorting algorithm like quicksort or mergesort to improve performance.
187-200: Optimize remove_entry to avoid reconstructing the array
The remove_entry method reconstructs the array after removal, which is inefficient. Consider using an in-place removal method or swapping the element to remove with the last element and popping it.
202-213: Inefficient update_entry method
The update_entry method removes and re-adds an entry, which is not optimal. Updating the entry in place would be more efficient.
221-242:
Potential off-by-one error in get_slice method
The check if (end > self.entries.len()) might allow end to equal self.entries.len(), leading to an out-of-bounds access since arrays are zero-indexed. Adjust the condition to prevent this.
Apply this fix:
- if (end > self.entries.len()) {
+ if (end > self.entries.len() || end == self.entries.len()) {Or more succinctly:
- if (end > self.entries.len()) {
+ if (end >= self.entries.len()) {Likely invalid or redundant comment.
src/tests/test_leaderboard.cairo (1)
223-223: Potential overflow in score calculation
When upgrading entry stats, adding entry3.wins and new_wins could result in an overflow if the totals exceed u32::MAX. Consider adding checks or using larger integer types to prevent this.
Run the following script to verify the maximum values:
|
after my last commit: tests are ok 👍 Screenshot of tests
build is ok, but with some warnings about old cairo version, as i mentioned in PR description Screenshot of build
migration applied as well Screenshot of migrate
If you are satisfied with my work, please mark the PR as ready, sir! |
* Add tests for tournament_system * Quickfixes * sozo build
* branch * sozo build
…derboard's test file moved to test directory.
beeguy74
force-pushed
the
feat-Implement-leaderboard-model
branch
from
95e8ec1 to
770d8f0
Compare
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
beeguy74
force-pushed
the
feat-Implement-leaderboard-model
branch
from
512b728 to
21d3948
Compare
Closes #28
In this pull request i work on a dynamically updated leaderboard feature.
There is a conflict with new version of
Alexandriawhich uses starknet2.8.5butdojoin project uses 2.7.0.I imported previous version of
Alexandriafor starknet version2.6.0- it works but there are a lot of warnings on compilation/Screenshot of build
Screenshot of tests
Screenshot of migrate
About solution: The main idea in maintaining Leaderboard up to date: after each changes into leaderboard, i sort entries array by rating. That way players with best rank always will be on top of the leaderboard and you can now rank of player by get his index in array.
This pull request introduces implementing the
LeaderboardandLeaderboardEntrymodels with associated methods and tests.Dependencies
alexandria_sortingtag=cairo-v2.6.0to manipulate arrays.New Module:
leaderboardmodule to thesrc/lib.cairofile to encapsulate the leaderboard functionality.Module Structure:
leaderboardto the project insrc/lib.cairo.Leaderboard Implementation:
LeaderboardEntryandLeaderboardmodels insrc/models/leaderboard.cairo, based on models fromLeaderboardEntrymodel I wrote an impl ofPartialEqtrait for searching entry by player_id and an impl ofPartialordfor sorting by scoreLeaderboardis always in sorted state with best player in the 0 index. Score is calculated with this formulaself.wins * 100 + self.highest_score - self.losses * 70.Leaderboardhas methods for add, delete, and update entries.LeaderboardandLeaderboardEntrymodels to ensure proper functionality with some negative scenarios as wellSummary by CodeRabbit
Release Notes
New Features
Improvements
Testing
These updates enhance user interaction with leaderboard and tournament features in the application.