Fix: Issue #16 (description: Restrict creating mines more than total cells) #17
Conversation
…ser interface feedback arkivanov#16
|
How do you like this solution of Issue #16 ? Also I'm not so much work with flow like that and I don't know or I just can't to assign myself to issue, to show that issue have this pull request as problem solution, I think I just need to read about it somewhere ;) |
b0r1ngx
changed the title
b0r1ngx
changed the title
b0r1ngx
changed the title
|
@coderabbitai review |
WalkthroughThe recent enhancement in the codebase focuses on refining the logic within the Changes
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (1)
- composeApp/src/commonMain/kotlin/com/arkivanov/minesweeper/settings/DefaultEditSettingsComponent.kt (2 hunks)
Additional comments: 1
composeApp/src/commonMain/kotlin/com/arkivanov/minesweeper/settings/DefaultEditSettingsComponent.kt (1)
- 8-8: The import of
kotlin.math.minis correctly added to support the new logic in theonConfirmClickedmethod. This is a standard Kotlin library function, ensuring compatibility and reliability.
...App/src/commonMain/kotlin/com/arkivanov/minesweeper/settings/DefaultEditSettingsComponent.kt
Outdated
Show resolved
Hide resolved
|
Waow, new instruments, awesome I'm checked out today, not get any notifies by email:\ |
|
You have a nice conversation here with some of LLM 😅
About this idea: yea, I'm also tried use width * height - 1, later user's can compete for who is fastest make first click on the field, it's really sounds funny, for someone P.S. game not fails but became to win state, maybe if we save: width * height, user's can compete by who have faster chip, system that is run this project. |
Yes, we can do that, lets me make a commit for this change. I have a question about this change, should we somehow show information about limitations to user that they are allowed to play only at 10 < width/height <= 100 setting? For example if they entered 5, we highlight textfiled with red and show hint with allowed settings? |
Let's keep it simple for now. From my point of view it makes sense to just coerce width between 2 and 100 and height between 2 and 50. What do you think? |
|
Also entering negative number leads to: I'm fix it in this PR. |
Yes, lets do this! |
Yep, that's why I suggest to maxMines.coerceIn(1, width * height - 1) |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- composeApp/src/commonMain/kotlin/com/arkivanov/minesweeper/settings/DefaultEditSettingsComponent.kt (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- composeApp/src/commonMain/kotlin/com/arkivanov/minesweeper/settings/DefaultEditSettingsComponent.kt
We must use something like I did in my last commit, because if we just coerce width / height values and not saving it for later usage for maxMines (like in replied message), we can get error, because width/height isn't fixed (coerced). |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- composeApp/src/commonMain/kotlin/com/arkivanov/minesweeper/settings/DefaultEditSettingsComponent.kt (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- composeApp/src/commonMain/kotlin/com/arkivanov/minesweeper/settings/DefaultEditSettingsComponent.kt
|
@coderabbitai I pushed a fix in commit d762256 |
|
Tip For best results, initiate chat on the files or code changes. @b0r1ngx, I've reviewed the changes made in commit d762256 to the
This solution appears to be a robust way to ensure game settings are within logical and playable limits, enhancing the overall user experience. Great job on implementing these changes! Analysis chainScripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Fetch the list of files modified in commit d762256
git show --name-only d762256
Length of output: 293 Script: #!/bin/bash
# Fetch the contents of the modified file in commit d762256
git show d762256:composeApp/src/commonMain/kotlin/com/arkivanov/minesweeper/settings/DefaultEditSettingsComponent.kt
Length of output: 1824 |
@coderabbitai can you give any advice how to increase code maintainability in this commit? |
|
Tip For best results, initiate chat on the files or code changes. @b0r1ngx, to enhance code maintainability for the changes made in commit d762256 to the
Implementing these practices can help make your code more maintainable, readable, and easier to work with in the long run. |
|
@coderabbitai I pushed a fix in commit 069a93b What is the purpose of this commit? Give answer for each question. |
|
Tip For best results, initiate chat on the files or code changes. @b0r1ngx, the purpose of commit 069a93b titled "Refactoring (Magic Numbers)" is to improve code readability and maintainability by replacing magic numbers with named constants ( The code metrics that are increasing due to this commit include:
This refactoring step is a good practice in software development, enhancing the overall quality of the codebase. Analysis chainScripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Fetch the contents of the changes made in commit 069a93b
git show 069a93b
Length of output: 1675 |
@arkivanov What do you think about this LLM advice? Should it be done or it's not improve code maintainability? Maybe it entails some disadvantages? We can impove it with something like: |
| ) | ||
| val finalWidth = width.coerceIn(MIN_SIZE..MAX_WIDTH) | ||
| val finalHeight = height.coerceIn(MIN_SIZE..MAX_HEIGHT) | ||
| val finalMines = maxMines.coerceIn(1 until finalWidth * finalHeight - 1) |
There was a problem hiding this comment.
I think until is exclusive on the right side. We could write coerseIn(1, finalWidth * finalHeight - 1), or coerseIn(1 until finalWidth * finalHeight).
I think the current code is good enough, no need to use named arguments in that particular case. Personally, I wouldn't extract constants either - |
|
Thanks for feedback! I agree with you! |
… is locally understandable what it is used about, without any additional explanations about it (like MIN_SIZE, etc...)
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- composeApp/src/commonMain/kotlin/com/arkivanov/minesweeper/settings/DefaultEditSettingsComponent.kt (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- composeApp/src/commonMain/kotlin/com/arkivanov/minesweeper/settings/DefaultEditSettingsComponent.kt
|
Thank you! |
Solution of a problem raised at Issue #16
Closes #16.
Summary by CodeRabbit
coerceInto ensure proper range for width, height, and max mines.