Code review #1

advanced-user · 2023-10-08T16:58:30Z

No description provided.

nikmas-dev · 2023-10-08T18:42:06Z

rust_backend/src/game.rs

+        };
+    }
+
+    fn in_range(&self, val: &f64, left_border: &f64, right_border: &f64) -> bool {


there is no need to take primitive copy types by reference, just take them by value

shurikv · 2023-10-08T18:49:21Z

rust_backend/src/game.rs

+    }
+
+    fn is_alive(&self) -> bool {
+        if self.health <= 0 {


Can be simplify

fn is_alive(&self) -> bool { self.health > 0 }

shurikv · 2023-10-08T18:57:27Z

rust_backend/src/game.rs

+    }
+
+    fn in_range(&self, val: &f64, left_border: &f64, right_border: &f64) -> bool {
+        if val >= left_border && val <= right_border {


Why you don't use short condition?

fn in_range(&self, val: &f64, left_border: &f64, right_border: &f64) -> bool { val >= left_border && val <= right_border }

shurikv · 2023-10-08T19:00:05Z

rust_backend/src/lobby.rs

+        if let Some(socket_recipient) = self.sessions.get(id_to) {
+            let _ = socket_recipient.do_send(WsMessage(message.to_owned()));
+        } else {
+            println!("attempting to send message but couldn't find user id.");


Use logger instead println!

shurikv · 2023-10-08T19:02:46Z

rust_backend/src/main.rs

+            .service(start_connection_route)
+            .app_data(Data::new(game_server.clone()))
+    })
+    .bind("127.0.0.1:8080")?


It would better to use configuration for host and port

shurikv · 2023-10-08T19:04:07Z

rust_backend/src/ws.rs

+    fn hb(&self, ctx: &mut ws::WebsocketContext<Self>) {
+        ctx.run_interval(HEARTBEAT_INTERVAL, |act, ctx| {
+            if Instant::now().duration_since(act.hb) > CLIENT_TIMEOUT {
+                println!("Disconnecting failed heartbeat");


shurikv · 2023-10-08T19:04:37Z

rust_backend/src/ws.rs

+            }
+            Ok(ws::Message::Binary(bin)) => ctx.binary(bin),
+            Ok(ws::Message::Close(reason)) => {
+                println!("Connection closed: {:?}", reason);


shurikv · 2023-10-08T19:06:57Z

rust_cli/src/main.rs

+    let game_id_str = "8ec45aa2-fa28-42e8-a80c-fd6ea3098424";
+    let server_addr = format!("ws://127.0.0.1:8080/{}", game_id_str);
+
+    println!("Connecting to {}", server_addr);


Use logger. Check all your print and replace it to logger

VictoriaGrasshopper

The code is well-structured and intuitive. The game takes into account many nuances: Even the ping of the players was handled. However, it could benefit from enhanced error handling (I mean .unwrap()) and logging.

VictoriaGrasshopper · 2023-10-08T19:20:28Z

rust_backend/src/lobby.rs

+        if self.sessions.remove(&msg.id).is_some() {
+            self.games_users_ids
+                .get(&msg.game_id)
+                .unwrap()


Is there a possibility that the unwrapped value would be None?

nerditation

added some minor suggestions, mainly related to error handling, also some style related.

error handling in rust is a big topic, but let's start simple.

nerditation · 2023-10-09T01:13:34Z

rust_backend/src/errors.rs

don't use String in error, use enum type directly, then implements std::fmt::Display (and std::error::Error) instead. for example:

pub enum AppError { GameStateError(GameState), NumberOfPlayersError(usize), PlayerNotFoundError, OpponentNotFoundError, } impl Display for AppError { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { match self { //... } } }

you can use libraries like thiserror or snafu to help you reduce boilerplates.

nerditation · 2023-10-09T01:16:06Z

rust_backend/src/game.rs

+            return Err(AppError {
+                message: "Player not found".to_string(),
+                error_type: AppErrorType::PlayerNotFoundError,
+            })


example:

return Err(AppError::PlayNotFoundError);

nerditation · 2023-10-09T01:17:30Z

rust_backend/src/game.rs

+            return Err(AppError {
+                message: format!("Game state is {:?}", self.state),
+                error_type: AppErrorType::GameStateError,
+            });


return Err(AppError::GameStateError(self.state));

nerditation · 2023-10-09T01:26:34Z

rust_backend/src/game.rs

+
+    pub fn add_player(&mut self, player_id: Uuid) -> Result<(), AppError> {
+        if self.players.len() >= 2 {
+            return Err(AppError { message: "The number of players cannot exceed 2".to_string(), error_type: AppErrorType::NumberOfPlayersError });


return Err(AppError::NumberOfPlayersError(self.players.len()));

nerditation · 2023-10-09T01:27:51Z

rust_backend/src/game.rs

+        Err(AppError {
+            message: "Opponent player not found".to_string(),
+            error_type: AppErrorType::PlayerNotFoundError,
+        })


Err(AppError::OpponentNotFoundError)

nerditation · 2023-10-09T01:33:58Z

rust_backend/src/lobby.rs

+                Err(err) => {
+                    let _ = msg.addr.do_send(WsMessage(err.message));
+                    return;


types that implements Display automatically implements ToString

let _ = msg.addr.do_send(WsMessage(err.to_string()));

nerditation · 2023-10-09T01:42:27Z

rust_backend/src/lobby.rs

+        let response;
+
+        match game.handle_msg(&msg) {
+            Ok(_) => response = serde_json::to_string(&game).unwrap(),
+            Err(err) => response = err.message,
+        }


match is an expression and can be used in assignment:

let response = match game.handle_msg(&msg) { Ok(_) => serde_json::to_string(&game).unwrap(), Err(err) => err.to_string(), };

one possible alternatives to match is Result::map_or_else(), but it's just a matter of personal taste:

let response = game.handle_msg.map_or_else( |err| err.to_string(), |_| serde_json::to_string(&game).unwrap(), );

nerditation · 2023-10-09T01:58:04Z

rust_backend/src/messages.rs

+        chars.next();
+        chars.next_back();
+
+        let msg_str= chars.as_str();
+
+        match msg_str {
+            "Left" => Ok(Msg::Left),
+            "Right" => Ok(Msg::Right),
+            "Hit" => Ok(Msg::Hit),
+            "Wait" => Ok(Msg::Wait),
+            _ => {
+                Err(())
+            },
+        }


this is not bad or wrong, just a question, is unicode handling necessary in this use case?

revsered unicode iteration for a utf8 string (next_back() in your code) is uncommon. it's totally ok if the use case requires it.

I suggest FromStr only be used for simple situations like round trip conversion of ToString , and if you need to parse arbitrary strings, FromStr probably is not a good choice.

nerditation · 2023-10-09T02:04:02Z

rust_cli/src/main.rs

+        async move {
+            while let Some(Ok(msg)) = read.next().await {
+                match msg {
+                    Message::Text(text) => {
+                        if !*has_id.lock().unwrap() {
+                            let message: Vec<&str> = text.split(" ").collect();
+                            // "your id is {uuid}"
+                            match message.get(3) {
+                                None => {}
+                                Some(uuid) => {
+                                    match Uuid::parse_str(uuid) {
+                                        Ok(uuid) => {
+                                            *user_id.lock().unwrap() = uuid;
+                                            *has_id.lock().unwrap() = true;
+                                        }
+                                        Err(_) => println!("Invalid Uuid format"),
+                                    };
+                                    println!("Received message from server: {:?}", uuid.clone());
+                                }
+                            };
+                        } else {
+                            println!("Received message from server: {}", text);
+                        }
+                    }
+                    _ => {}
+                }
+            }
+        }


I suggest refactor this code, its is deeply nested and shifted too much to the right.

nerditation · 2023-10-09T02:06:20Z

rust_cli/src/main.rs

+                let msg = match line.trim().to_lowercase().as_str() {
+                    "left" => Msg::Left,
+                    "right" => Msg::Right,
+                    "wait" => Msg::Wait,
+                    "hit" => Msg::Hit,
+                    _ => continue,
+                };


can the FromStr impl be adapted and reused for this?

danik-tro · 2023-10-09T21:03:52Z

rust_backend/src/game.rs

+        if let Some(player) =  self.players.get(player_id) {
+            Ok(player)
+        } else {
+            return Err(AppError {
+                message: "Player not found".to_string(),
+                error_type: AppErrorType::PlayerNotFoundError,
+            })


Looks like can be simplified to

self.players.get(player_id).ok_or_else(|| AppError { message: "Player not found".to_string(), error_type: AppErrorType::PlayerNotFoundError, })?;

danik-tro · 2023-10-09T21:10:22Z

rust_backend/src/game.rs

+        for (uuid, _) in &self.players {
+            if uuid != player_id {
+                return Ok(uuid.clone());
+            }
+        }


I found it more idiomatic

if self.players.iter().any(|(uuid, _)| uuid != player_id) { return Ok(uuid.clone) }

Up to you

advanced-user added 4 commits October 8, 2023 16:27

Basic server implementation for a fighting game

9fcddd8

Added an example of using a game server

b46bad3

Added readme file

e354de5

Added error handling in game

9bc9438

0x07C0 approved these changes Oct 8, 2023

View reviewed changes

Added handling of incorrect messages

15dbef3

nikmas-dev approved these changes Oct 8, 2023

View reviewed changes

shurikv suggested changes Oct 8, 2023

View reviewed changes

VictoriaGrasshopper approved these changes Oct 8, 2023

View reviewed changes

TheBestTvarynka approved these changes Oct 8, 2023

View reviewed changes

nerditation reviewed Oct 9, 2023

View reviewed changes

danik-tro approved these changes Oct 9, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Code review #1

Code review #1

advanced-user commented Oct 8, 2023

nikmas-dev Oct 8, 2023

shurikv Oct 8, 2023

shurikv Oct 8, 2023

shurikv Oct 8, 2023

shurikv Oct 8, 2023

shurikv Oct 8, 2023

shurikv Oct 8, 2023

shurikv Oct 8, 2023

VictoriaGrasshopper left a comment

VictoriaGrasshopper Oct 8, 2023

nerditation left a comment

nerditation Oct 9, 2023

nerditation Oct 9, 2023

nerditation Oct 9, 2023

nerditation Oct 9, 2023

nerditation Oct 9, 2023

nerditation Oct 9, 2023

nerditation Oct 9, 2023

nerditation Oct 9, 2023

nerditation Oct 9, 2023

nerditation Oct 9, 2023

danik-tro Oct 9, 2023

danik-tro Oct 9, 2023

Code review #1

Are you sure you want to change the base?

Code review #1

Conversation

advanced-user commented Oct 8, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VictoriaGrasshopper left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nerditation left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment