thomas-mauran / chess-tui

A chess TUI implementation in rust 🦀
https://crates.io/crates/chess-tui
MIT License
361 stars 20 forks source link

improving storage of coordinates #75

Closed JeromeSchmied closed 2 months ago

JeromeSchmied commented 4 months ago

Title

Description

IMO the current implementation for coordinations [i8; 2] is not straightforward at all, also doesn't utilize Rust's awesome type-safety features.

I made a

struct Coord {
    row: u8,
    col: u8,
}

for this, which I think fits the purpose.

I've also implemented std::ops::Index and std::ops::IndexMut for Board to be able to index it with Coord, just like @joshka suggested.

Checklist:

JeromeSchmied commented 3 months ago

But I have difficulties, as for several functions, only board::Board.board is passed, for which I can not implement std::ops::Index, as it's not a struct or anything defined in this crate, and therefore I can't utilize the [] operator in those functions, which is quite sad.

To address this issue, what I could do is:

 struct JustTheBoard { board: [[Option<(PieceType, PieceColor)>; 8]; 8] }

and implement std::ops::Index for that

thomas-mauran commented 3 months ago

Looking it soon

nicholasmello commented 3 months ago

But I have difficulties, as for several functions, only board::Board.board is passed, for which I can not implement std::ops::Index, as it's not a struct or anything defined in this crate, and therefore I can't utilize the [] operator in those functions, which is quite sad.

To address this issue, what I could do is:

* make a new
 struct JustTheBoard { board: [[Option<(PieceType, PieceColor)>; 8]; 8] }

and implement std::ops::Index for that

* to every function, that only gets `board: [[Option<(PieceType, PieceColor)>; 8]; 8]`, pass the whole `board::Board` struct

* almost same as previous: make functions, that use `board: [[Option<(PieceType, PieceColor)>; 8]; 8]` methods of `board::Board`

If we defined our own type, something like type Board = [[Option<(PieceType, PieceColor)>; 8]; 8];, we should be able to implement using it without the need for the struct. I coded up a quick test just to make sure it would work.

type Board = [[Option<i32>; 2]; 2];

struct Coord {
    row: u8,
    col: u8
}

impl std::ops::Index<&Coord> for Board {
    type Output = Option<i32>;

    fn index(&self, index: &Coord) -> &Self::Output {
        &self[index.row as usize][index.col as usize]
    }
}

fn main() {
    let board: Board = [
        [Some(0),Some(1)],
        [Some(2),Some(3)],
    ];
    let coord: Coord = Coord {row: 1, col: 0};
    println!("{:?}", board);
    println!("{:?}", board[&coord]);
}
JeromeSchmied commented 3 months ago

Okay guys, thanks for the suggestions, I'll see what can be done and check back soon. One more thing though: what does squashing commits mean?

nicholasmello commented 3 months ago

Okay guys, thanks for the suggestions, I'll see what can be done and check back soon. One more thing though: what does squashing commits mean?

Squash is a rebase option to combine multiple commits into a single one. It is often done before merges to keep git history simpler, especially when history includes a lot of fixes that are related to each other. It doesn't have to be a single commit for something this big but fewer commits to not spam the git history would be preferred.

JeromeSchmied commented 3 months ago

okay, so if I squash the commits, is this pr ready to merge?

JeromeSchmied commented 2 months ago

squashing done

JeromeSchmied commented 2 months ago

is there anything else, I may do?

thomas-mauran commented 2 months ago

Nope my bad forgot to merge that, doing that tomorrow for sure been a bit overbooked recently 😅 @JeromeSchmied

thomas-mauran commented 2 months ago

Thank you very much for your contribution @JeromeSchmied and thx for the review @nicholasmello :muscle: