INDAPlus21 / INDAPlus21/ogronman-chess-gui
Pass
- Dominant language
- Rust
- Stars
- 0
- Forks
- 0
- PR merge metrics
- No merged PRs in 30d
Description
**Very well done Oskar!**
Pretty interface. I like how you visualise the choise of promotion, even if it's not a colour exclusive choise.
I do have some notes on your practises:
- Why do you use dynamically allocated vectors instead of sized tuples? `board: Vec>` could have been `board: Vec<(u8, u8)>`.
- You suffer from copy-pasta syndrome. The use of variables increases the readability of code, which is one of the criterias for good code design.
_Example: One of your functions_:
```rust
fn get_promote_piece(&mut self, x:f32, y:f32) -> () {
if x > (SCREEN_SIZE.0/ 2f32 + (SCREEN_SIZE.0 +350.0)/2f32) as f32 +5f32 && x <= (SCREEN_SIZE.0/ 2f32 + (SCREEN_SIZE.0 +350.0)/2f32) as f32 + 75f32 {
if y > ((SCREEN_SIZE.1 as f32) / 2f32) as f32 && y <= ((SCREEN_SIZE.1 as f32) / 2f32 as f32) + 80f32{
self.promote_piece = 'r';
}
}
if x > (SCREEN_SIZE.0/ 2f32 + (SCREEN_SIZE.0 +100.0)/2f32) as f32 +5f32 && x <= (SCREEN_SIZE.0/ 2f32 + (SCREEN_SIZE.0 +100.0)/2f32) as f32 + 75f32 {
if y > ((SCREEN_SIZE.1 as f32) / 2f32) as f32 && y <= ((SCREEN_SIZE.1 as f32) / 2f32 as f32) + 80f32{
self.promote_piece = 'q';
}
}
if x > (SCREEN_SIZE.0/ 2f32 + (SCREEN_SIZE.0 +100.0)/2f32) as f32 +5f32 && x <= (SCREEN_SIZE.0/ 2f32 + (SCREEN_SIZE.0 +100.0)/2f32) as f32 + 75f32 {
if y > ((SCREEN_SIZE.1 as f32 + 250f32) / 2f32) as f32 && y <= ((SCREEN_SIZE.1 as f32 + 250f32) / 2f32 as f32) + 80f32{
self.promote_piece = 'b';
}
}
if x > (SCREEN_SIZE.0/ 2f32 + (SCREEN_SIZE.0 +350.0)/2f32) as f32 +5f32 && x <= (SCREEN_SIZE.0/ 2f32 + (SCREEN_SIZE.0 +350.0)/2f32) as f32 + 75f32 {
if y > ((SCREEN_SIZE.1 as f32 + 250f32 ) / 2f32) as f32&& y <= ((SCREEN_SIZE.1 as f32 + 250f32 ) / 2f32 as f32) + 80f32{
self.promote_piece = 'k';
}
}
}
```
_Example: Well designed (and optimised)_:
```rust
fn get_promote_piece(&mut self, x:f32, y:f32) -> () {
let left_offset = (SCREEN_SIZE.0/ 2f32 + (SCREEN_SIZE.0 +350.0)/2f32) as f32;
let right_offset = (SCREEN_SIZE.0/ 2f32 + (SCREEN_SIZE.0 +100.0)/2f32) as f32;
let upper_offset = (SCREEN_SIZE.1 as f32) / 2f32 as f32;
let lower_offset = (SCREEN_SIZE.1 as f32 + 250f32) / 2f32 as f32;
let is_left_selection = x > left_offset +5f32 && x <= left_offset + 75f32;
let is_right_selection = x > right_offset +5f32 && x <= right_offset + 75f32;
let is_upper_selection = y > upper_offset && y <= upper_offset + 80f32;
let is_lower_selection = y > lower_offset && y <= lower_offset + 80f32;
if (is_left_selection || is_right_selection) && (is_upper_selection || is_lower_selection) {
self.promote_piece =
if is_left_selection && is_upper_selection { 'r' }
else if is_right_selection && is_upper_selection { 'q' }
else if is_right_selection && is_lower_selection { 'b' }
else /*is_left_selection && is_lower_selection*/ { 'k' };
}
}
```
- Why the convoluted conversions?
_Example: One of your functions_:
```rust
fn piece_from_symbol(c:char) -> u8 {
let mut _s = c.to_string();
_s = _s.chars().map(|_s| match _s {
'p' => "1",
'n' => "2",
'b' => "3",
'r' => "4",
'q' => "5",
'k' => "6",
_ => "0"
}).collect();
let piece:u8 = _s.parse::().unwrap(); //Gör om bokstäver till siffror som kan motsvara till brädet t.ex.
return piece;
}
```
_Example: Proper conversion_:
```rust
fn piece_from_symbol(c:char) -> u8 {
match c {
'p' => 1,
'n' => 2,
'b' => 3,
'r' => 4,
'q' => 5,
'k' => 6,
_ => 0
}
}
```
- Commented out code is generally regarded as bad practice.
- The use of enumerables instead of dynamically allocated strings whould no only optimise your memory usage, but also make for prettier code.
Contributor guide
No contributing guide indexed for this repository
Research direction
No source file, test, or entry point is named; begin by locating the Rust implementations discussed in the review, including get_promote_piece, piece_from_symbol, and the board representation. The issue does not define a specific scope or acceptance criteria, so completion would require agreeing which review comments to address and how to verify them.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- rust
- Domain
- game-dev
- Issue type
- Refactor
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Stale
- Clarity
- Needs clarification
- Newbie friendliness
- 20/100