5
\$\begingroup\$

I've been trying to pick up some Rust experience and decided to try and make a text adventure game. I'd like some feedback on potential bad practice and non-Rust-style code I may have used. I'm moving towards Rust from a Python perspective.

use std::io::stdin;

struct Game {
    room: usize,
    inventory: Vec<Item>,
    rooms: Vec<Room>
}

impl Game {
    fn room(&self) -> &Room {
        &self.rooms[self.room]
    }

    fn room_mut(&mut self) -> &mut Room {
        &mut self.rooms[self.room]
    }

    fn exits(&self) {
        let mut index = 0;
        let mut s = String::from(
            format!("{} has {} exits:", &self.room().name, &self.room().exits.len())
        );

        for exit in &self.room().exits {
            s = format!("{}\n({}) {}", s, index, self.rooms[*exit].name);
            index += 1;
        }

        println!("{}", s);
    }

    fn view_inventory(&self) {
        let mut index = 0;
        let mut s = String::from(
            format!("You have {} items:", self.inventory.len())
        );

        for item in &self.inventory {
            s = format!("{}\n({}) {}", s, index, item.name);
            index += 1;
        }

        println!("{}", s);
    }

    fn move_room(&mut self, room: usize) {
        self.room = self.room().exits[room];
    }

    fn take(&mut self, item: usize) -> &Item {
        let item = self.room_mut().items.remove(item);
        self.inventory.push(item);

        &self.inventory[self.inventory.len() - 1]
    }
}

struct Item {
    name: String,
    description: String
}

struct Room {
    name: String,
    description: String,
    exits: Vec<usize>,
    items: Vec<Item>
}

impl Room {
    fn look(&self) {
        println!("{}", self.description)
    }

    fn inspect(&self) {
        let mut index = 0;
        let mut s = String::from(
            format!("{} has {} items:", &self.name, &self.items.len())
        );

        for item in &self.items {
            s = format!("{}\n({}) {}", s, index, item.name);
            index += 1;
        }

        println!("{}", s);
    }
}

fn main() {
    let mut rooms = vec![
        Room {
            name: String::from("Bedroom"),
            description: String::from("A tidy, clean bedroom with 1 door and a balcony"),
            exits: vec![1, 2],
            items: vec![ Item {
                name: String::from("Key"),
                description: String::from("A golden key")
            }]
        },

        Room {
            name: String::from("Balcony"),
            description: String::from("An outdoor balcony that overlooks a gray garden"),
            exits: vec![0],
            items: vec![]
        },

        Room {
            name: String::from("Landing"),
            description: String::from("A carpetted landing with doors leading off it. It overlooks a large living space. A set of stairs leads down"),
            exits: vec![0],
            items: vec![]
        },
    ];

    let mut player = Game {
        room: 0,
        rooms: rooms,
        inventory: vec![]
    };

    println!("Type `look' to look around. Type `move <room no>' to switch room");

    loop {
        let mut input = String::new();

        match stdin().read_line(&mut input) {
            Ok(_) => {
                let mut commands = input.trim().split_whitespace();

                match commands.next() {
                    Some("look") => {
                        player.room().look();
                        player.exits();
                    }

                    Some("move") => {

                        let args: Vec<&str> = commands.collect();

                        if args.len() != 1 {
                            println!("Incorrect args.");
                            continue;
                        }

                        let room_no: usize = match args[0].parse() {
                            Ok(a) => {a},

                            Err(e) => {
                                println!("{}", e);
                                continue
                            }
                        };

                        player.move_room(room_no);

                        println!("You moved to {}", player.room().name);
                    }

                    Some("inventory") => {
                        player.view_inventory();
                    }

                    Some("inspect") => {
                        player.room().inspect();
                    }

                    Some("take") => {
                        let args: Vec<&str> = commands.collect();

                        if args.len() != 1 {
                            println!("Incorrect args.");
                            continue;
                        }

                        let item_no: usize = match args[0].parse() {
                            Ok(a) => {a},

                            Err(e) => {
                                println!("{}", e);
                                continue
                            }
                        };

                        let item = player.take(item_no);

                        println!("You collected {}", item.name);
                    }

                    None => {},

                    _ => {},
                }
            }
            Err(error) => panic!("Error occured reading stdin: {}", error),
        }
    }
}

The source is also available on GitHub.

\$\endgroup\$
5
\$\begingroup\$
struct Game {
    room: usize,
    inventory: Vec<Item>,
    rooms: Vec<Room>
}

I would recommend current_room instead of room. The meaning is slightly clearer.

You have several functions like the following, the comments here apply to all of them

    fn exits(&self) {
        let mut index = 0;
        let mut s = String::from(
            format!("{} has {} exits:", &self.room().name, &self.room().exits.len())
        );

format! already produces a String, so you don't need String::from. You also do not need the & because the format! macro will already add them.

        for exit in &self.room().exits {

You can for (index, exit) in self.room().exits.iter().enumerate() {. Then you don't need to keep track of the index yourself.

            s = format!("{}\n({}) {}", s, index, self.rooms[*exit].name);

Rather than assign a new string object, it probably makes sense to use s.push_str to onto the existing string.

            index += 1;
        }

        println!("{}", s);

There doesn't appear to be a good reason to build up a string object and then print it. Your code would be simpler here if you just println! each piece of the string as you build it.

    }


    let mut rooms = vec![
        Room {
            name: String::from("Bedroom"),
            description: String::from("A tidy, clean bedroom with 1 door and a balcony"),
            exits: vec![1, 2],
            items: vec![ Item {
                name: String::from("Key"),
                description: String::from("A golden key")
            }]
        },

All your strings are static, so you may want to consider using &'static str to hold the various strings instead of String which will avoid having to call String::from when you create the room objects here.

You may also want to consider installing clippy. It is an extra Cargo command (cargo clippy) that has a number of extra lints for common Rust mistakes. It points to several of the points I showed here.

\$\endgroup\$
  • \$\begingroup\$ Your comments are really useful and a lot of the things kind of seem obvious now you point them out, thank you! I don't know the rules about accepting answers on here, since really any feedback is good, but I'll tick this one now. Again, many thanks for your notes. \$\endgroup\$ – JellyWX Oct 6 '18 at 17:44
4
\$\begingroup\$
  1. Rust is a statically compiled language, which means that the compiler tells you things that you should pay attention to. Your code should not have warnings:

    warning: variable does not need to be mutable
      --> src/main.rs:91:9
       |
    91 |     let mut rooms = vec![
       |         ----^^^^^
       |         |
       |         help: remove this `mut`
       |
       = note: #[warn(unused_mut)] on by default
    
    warning: field is never used: `description`
      --> src/main.rs:60:5
       |
    60 |     description: String
       |     ^^^^^^^^^^^^^^^^^^^
       |
       = note: #[warn(dead_code)] on by default
    
  2. Use tools like rustfmt to use the idiomatic Rust style. For example, trailing commas are expected on multiline blocks and Ok(a) => {a}, should just be Ok(a) => a,

  3. Use tools like clippy to get additional static analysis beyond what the compiler performs. For example:

    warning: redundant field names in struct initialization
       --> src/main.rs:117:9
        |
    117 |         rooms: rooms,
        |         ^^^^^^^^^^^^ help: replace it with: `rooms`
        |
        = note: #[warn(clippy::redundant_field_names)] on by default
        = help: for further information visit https://rust-lang-nursery.github.io/rust-clippy/v0.0.212/index.html#redundant_field_names
    
    warning: identical conversion
      --> src/main.rs:20:21
       |
    20 |           let mut s = String::from(format!(
       |  _____________________^
    21 | |             "{} has {} exits:",
    22 | |             &self.room().name,
    23 | |             &self.room().exits.len()
    24 | |         ));
       | |__________^
       |
       = note: #[warn(clippy::identity_conversion)] on by default
       = help: for further information visit https://rust-lang-nursery.github.io/rust-clippy/v0.0.212/index.html#identity_conversion
    help: consider removing `String::from()`
       |
    20 |         let mut s = format!(
    21 |             "{} has {} exits:",
    22 |             &self.room().name,
    23 |             &self.room().exits.len()
    24 |         );
       |
    
    warning: the variable `index` is used as a loop counter. Consider using `for (index, item) in &self.room().exits.enumerate()` or similar iterators
      --> src/main.rs:26:21
       |
    26 |         for exit in &self.room().exits {
       |                     ^^^^^^^^^^^^^^^^^^
       |
       = note: #[warn(clippy::explicit_counter_loop)] on by default
       = help: for further information visit https://rust-lang-nursery.github.io/rust-clippy/v0.0.212/index.html#explicit_counter_loop
    
    warning: identical conversion
      --> src/main.rs:36:21
       |
    36 |         let mut s = String::from(format!("You have {} items:", self.inventory.len()));
       |                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider removing `String::from()`: `format!("You have {} items:", self.inventory.len())`
       |
       = help: for further information visit https://rust-lang-nursery.github.io/rust-clippy/v0.0.212/index.html#identity_conversion
    
    warning: the variable `index` is used as a loop counter. Consider using `for (index, item) in &self.inventory.enumerate()` or similar iterators
      --> src/main.rs:38:21
       |
    38 |         for item in &self.inventory {
       |                     ^^^^^^^^^^^^^^^
       |
       = help: for further information visit https://rust-lang-nursery.github.io/rust-clippy/v0.0.212/index.html#explicit_counter_loop
    
    warning: identical conversion
      --> src/main.rs:77:21
       |
    77 |         let mut s = String::from(format!("{} has {} items:", &self.name, &self.items.len()));
       |                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider removing `String::from()`: `format!("{} has {} items:", &self.name, &self.items.len())`
       |
       = help: for further information visit https://rust-lang-nursery.github.io/rust-clippy/v0.0.212/index.html#identity_conversion
    
    warning: the variable `index` is used as a loop counter. Consider using `for (index, item) in &self.items.enumerate()` or similar iterators
      --> src/main.rs:79:21
       |
    79 |         for item in &self.items {
       |                     ^^^^^^^^^^^
       |
       = help: for further information visit https://rust-lang-nursery.github.io/rust-clippy/v0.0.212/index.html#explicit_counter_loop
    
  4. Building up large Strings just to print them is inefficient. Just print them directly.

  5. Removing an Item from a Vec can be more expensive than needed. Consider using a HashSet instead.

  6. v[v.len() - 1] can be v.last().unwrap()

  7. You are creating a new String for every iteration of your loop. If you want that, use BufRead::lines

  8. match { ... Err(x) => panic! } is Result::unwrap or Result::unwrap_or_else.

  9. Collecting an entire vector of strings is wasteful if you only care about the next value. Either call Iterator::next twice or at the very least only take 2 values from the iterator, reducing wasted memory when a user accidentally pastes hundreds of words.

  10. Don't specify the type of a collection's items' when collecting, it's redundant. Use _ instead.

  11. Consider writing a macro to avoid repeating the "Ok or continue" code. You could also extract this to a function and make use of the normal ? logic.

  12. Consider writing some code to parse the users argument so that it's out of the main loop and the bodies of each command's branch are at the same level of abstraction.

  13. Avoid duplicating match arms — there's no reason to have both None => {} and _ => {}.

use std::io::{stdin, BufRead};

struct Game {
    room: usize,
    inventory: Vec<Item>,
    rooms: Vec<Room>,
}

impl Game {
    fn room(&self) -> &Room {
        &self.rooms[self.room]
    }

    fn room_mut(&mut self) -> &mut Room {
        &mut self.rooms[self.room]
    }

    fn exits(&self) {
        println!(
            "{} has {} exits:",
            &self.room().name,
            &self.room().exits.len()
        );

        for (index, exit) in self.room().exits.iter().enumerate() {
            println!("({}) {}", index, self.rooms[*exit].name);
        }
    }

    fn view_inventory(&self) {
        println!("You have {} items:", self.inventory.len());

        for (index, item) in self.inventory.iter().enumerate() {
            println!("\n({}) {}", index, item.name);
        }
    }

    fn move_room(&mut self, room: usize) {
        self.room = self.room().exits[room];
    }

    fn take(&mut self, item: usize) -> &Item {
        let item = self.room_mut().items.remove(item);
        self.inventory.push(item);
        self.inventory.last().unwrap()
    }
}

struct Item {
    name: String,
}

struct Room {
    name: String,
    description: String,
    exits: Vec<usize>,
    items: Vec<Item>,
}

impl Room {
    fn look(&self) {
        println!("{}", self.description)
    }

    fn inspect(&self) {
        println!("{} has {} items:", &self.name, &self.items.len());

        for (index, item) in self.items.iter().enumerate() {
            println!("\n({}) {}", index, item.name);
        }
    }
}

fn main() {
    let rooms = vec![
        Room {
            name: String::from("Bedroom"),
            description: String::from("A tidy, clean bedroom with 1 door and a balcony"),
            exits: vec![1, 2],
            items: vec![ Item {
                name: String::from("Key"),
            }]
        },

        Room {
            name: String::from("Balcony"),
            description: String::from("An outdoor balcony that overlooks a gray garden"),
            exits: vec![0],
            items: vec![]
        },

        Room {
            name: String::from("Landing"),
            description: String::from("A carpetted landing with doors leading off it. It overlooks a large living space. A set of stairs leads down"),
            exits: vec![0],
            items: vec![]
        },
    ];

    let mut player = Game {
        room: 0,
        rooms,
        inventory: vec![],
    };

    println!("Type `look' to look around. Type `move <room no>' to switch room");

    let stdin = stdin();
    for line in stdin.lock().lines() {
        let input = line.unwrap_or_else(|e| panic!("Error occured reading stdin: {}", e));
        let mut commands = input.trim().split_whitespace();

        match commands.next() {
            Some("look") => {
                player.room().look();
                player.exits();
            }

            Some("move") => {
                let args: Vec<_> = commands.take(2).collect();

                if args.len() != 1 {
                    println!("Incorrect args.");
                    continue;
                }

                let room_no: usize = match args[0].parse() {
                    Ok(a) => a,

                    Err(e) => {
                        println!("{}", e);
                        continue;
                    }
                };

                player.move_room(room_no);

                println!("You moved to {}", player.room().name);
            }

            Some("inventory") => {
                player.view_inventory();
            }

            Some("inspect") => {
                player.room().inspect();
            }

            Some("take") => {
                let args: Vec<_> = commands.take(2).collect();

                if args.len() != 1 {
                    println!("Incorrect args.");
                    continue;
                }

                let item_no: usize = match args[0].parse() {
                    Ok(a) => a,

                    Err(e) => {
                        println!("{}", e);
                        continue;
                    }
                };

                let item = player.take(item_no);

                println!("You collected {}", item.name);
            }

            _ => {}
        }
    }
}
\$\endgroup\$
  • \$\begingroup\$ Your comment on hashsets... could using swap_remove rather than remove achieve a similar result? I don't understand how the memory difference is between a HashSet and a Vec \$\endgroup\$ – JellyWX Oct 8 '18 at 12:39
  • \$\begingroup\$ Also although its a different issue, I can't manage to install Clippy. It complains about not being available for the toolchain \$\endgroup\$ – JellyWX Oct 8 '18 at 12:42
  • \$\begingroup\$ @JellyWX yes, swap_remove would be better as it avoids moving the elements all around, but it still requires you do perform a linear search to find the element to delete. A HashSet has an amortized O(1) removal. \$\endgroup\$ – Shepmaster Oct 8 '18 at 12:44
  • \$\begingroup\$ @JellyWX rustup component add clippy-preview seems to work for me. Do you have Rust 1.29+? What toolchain does the error message complain about? \$\endgroup\$ – Shepmaster Oct 8 '18 at 12:47
  • 1
    \$\begingroup\$ @Shepmaster reinstalled Rust and its working now, thanks \$\endgroup\$ – JellyWX Oct 10 '18 at 20:49

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.