INDAPlus21 / INDAPlus21/ogronman-chess-gui

Pass

Open
#1 0 comments 0 reactions 0 assignees View on GitHub
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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.