Rasteroid code review

Hi Rustacean,

I have written a small size project, for learning purpose. It's an Asteroid clone based on SDL2 crate. It seems to be a quite common project :slight_smile: when learning rust.

The project is functional, and I wont make significant changes excepted improving code quality. Nevertheless, I would appreciate that competent people have a look at the code source, and give me suggestions/advices in order to write a more idiomatic code in particular on these subjects:

-My Iterator usage (feels very basic and C++ like).
-One/Zero/MinusOne traits in maths (Is that the way to do so ?).
-My usage of modules (I often hesitate between creating module or enclosing struct)
-Game constants: Is there a better ways to define/store them ?
-Dyn usage : I could have use it with a kind of 'drawable' trait but I'm ignorant of cost of dyn at runtime (equivalent to C++ virtual ?) . Nevertheless I use it in non intensive code.
-Any other suggestions/criticisms that contribute to a better rust usage.

You will also see that I'm quite reluctant to use external crate, that's why I use xorshift32 generator in place of the famous rand crate, and wrote my vector/matrix code (very interesting from learning point of view).

The code can be found : here

Thanks in advance and best regards,

Bruno

1 Like

This bit jumps out at me:

    pub fn get_shape_by_u8(c: u8) -> Shape {
        match c as char {
            '0' => Shape::Zero,
            '1' => Shape::One,
            …
            'D' => Shape::D,
            'E' => Shape::E,
            'F' => Shape::F,
            …
            '%' => Shape::Percent,
            _ => Shape::E, // Return an error !
        }
    }

You can't tell from the return value whether it was an error, because it's returning the same thing as for a value 'E' input.

This is a great place to use Option or Result instead.

For example, you might do something like this:

    pub fn get_shape_by_u8(c: u8) -> Option<Shape> {
        Some(match c as char {
            '0' => Shape::Zero,
            '1' => Shape::One,
            …
            'D' => Shape::D,
            'E' => Shape::E,
            'F' => Shape::F,
            …
            '%' => Shape::Percent,
            _ => return None, // Return an error !
        })
    }

That's also a good opportunity to mention "byte characters". How you have it is perfectly fine, but if you prefer you can also do this to avoid the cast:

    pub fn get_shape_by_u8(c: u8) -> Option<Shape> {
        Some(match c {
            b'0' => Shape::Zero,
            b'1' => Shape::One,
            …

(There are also byte strings, like b"ABCD", from which you can get &[u8]s instead of &strs.)

2 Likes

Thanks for your suggestion. It really was the place for an Option. I updated the source code accordingly.

Some comments, not in order of importance:

  • It's not necessary to explicitly tag the main loop with 'main_loop. In fact, needing to label stuff like that should be pretty rare if you structure your code properly. You can just break from that loop.

  • Re "One/Zero/MinusOne traits in maths": instead of recreating the traits yourself, you could use num-traits for better interop with the rest of the numeric computing ecosystem.

  • They serve completely different purposes. Modules are for organizing code at compile time; structs are for organizing actual data. If you want something close to C++'s namespaces, that'd be modules. Don't create data-less structs just for the sake of putting type methods inside them, use free functions in a module instead if you need free functions.

  • "Game constants: Is there a better ways to define/store them?" – definitely; a "constants" module is an anti-pattern (so would be a "structs" or "enums" module, that I often see in beginners' code).

    It's not helpful to group code mechanically by what language features it uses. As the implementation evolves, a struct might become an enum, a const might become an fn or a static, etc. Instead, group the code logically, according to what functionality of the system it implements.

  • "Dyn usage : I could have use it with a kind of 'drawable' trait but I'm ignorant of cost of dyn at runtime (equivalent to C++ virtual?)" – yes, dyn Trait means dynamic dispatch through a vtable, just like C++'s virtual functions.

  • Annotating unit-returning functions with -> () is non-idiomatic, you can just leave it off. Similarly, the accepted norm for formatting opening curly braces is the end of the current line, not the beginning of the next line.

  • In Atlas::get_vertexes(), the indexing with increasing constants and the repetition of the .vertexes field access seems like a code smell. You could add an explicit representation (e.g. #[repr(u8)]) to the enum and then simplify the whole function to ATLAS[s as u8 as usize].vertexes

  • the some_boolean == true here is an anti-pattern; you have a boolean, so you can just use it instead of comparing it to true.

  • Entity has all public fields, so it's not very nice to construct it mutably just to reassign its fields here. Just construct it with the required initial field values in the first place; maybe use FRU syntax if needed.

6 Likes

Thanks @H2CO3 for this nice answer, it's really appreciated.

I agree recreating the traits is counter productive and my code is probably of a lesser quality than num-traits crate. Nevertheless, I can't write my code based on what has been done in another crate because it's commonly used. If it were in the Rust standard library I would have use it, for sure. As already mentioned, I'm reluctant to import crates because of cascading import (SDL2 uses approximately 8 crates) and I can't control what's inside (Some android dev, didn't know that their application contained ads, because they didn't write the code, it was in a dependency). Maybe am I wrong with num-traits or crates.io crates status with Rust ?

Your explanation of modules is clear, I thank you for this. I'm a still unclear about what I should have done with the game constants, since there are used in differents modules. What would you have done of them ? I remember centuries ago that java programmers stored constants into public field of a dedicated class, then it became a sin, and constants were stored into interface that must be implemented by those who wanted to use them :slight_smile: :smile: Is there a consensus about global constant in Rust ?

Ok, specifying unit-returning should be avoided.

I have never heard about #[repr(u8)] , but it sound quite interesting. I'm gonna study this now.

About entity, I will also read again the rust update syntax chapter and follow you suggestion.

Thanks again for your nice answer, links to my code, and to The Book greatly help me.

Bruno

1 Like

I have no idea what this means. If you say you never want to use any 3rd-party crates, then that's certainly not realistic. You simply can't expect to write all foundational code by yourself and never ever have to interact with other people's code. Sure, it's nice to avoid accidental dependencies and code bloat, but a superstitious fear of importing anything (including core crates such as num-traits) won't get you anywhere.

1 Like

Sorry about this, my English speaking is rather poor.

Of course I can't avoid 3rd-party crates, I used SDL2. But, there are many crates on crates.io that do almost the same things. For example, RNG crates are legion. On which one should I base my code on ? The more popular ? The more efficient ? The easiest to use ? Maybe I missed something and num-traits is the official Rust crate to use for numrical computing ? Foundational code, should be in the standard library, or, if you don't want a bloated stdlib, these crate should be marked as the official one.

I hope to have made things clear (I doubt !)

1 Like

The Rust stdlib is intentionally minimal, because once something is in std, it's stuck there and in that exact form forever, due to backward compatibility guarantees. That would mean that it would be impossible to improve it – this is avoided by versioning them separately, on crates.io.

For the most part, just use rand.

It is.

You can always check who the crate author(s) or core contributor(s) is/are. If it's someone in the Rust foundation or language/lib teams (e.g. dtolnay, BurntSushi, etc.) then you can be assured that it is de facto official and you are expected to rely on it.

Ok, I wasn't aware that some crate were de facto the official one. Nevertheless, I keep saying that those crates should be clearly identified on crate.io. Relying on the knowledge of the rust dev team member name to determine which crates are 'official' sounds a bit strange, anyway, I don't want to troll on this subject.

I followed your suggestion about Atlas::get_vertexes. As you said it leads to a single line function. I have read things about #[repr(u8)]. In our case, I understand that the Enum Shape will be mapped on a u8 type allowing 256 different 'flavour'. I also noticed that the code compiles without this attribute, maybe mapping the Enum on something different. I guess that you suggest to write repr attribute to guaranty that enum type match the cast to u8 in ATLAS[s as u8 as usize].vertexes. Is there another benefit of this directive ?

Thanks again for you time.

3 Likes

If it works without the explicit repr, it might work with a single cast too (avoiding chaining two of them), like array[index_enum as usize] directly. In this case, I don't think it's especially beneficial to exactly assert the representation if it's not required by the compiler; that's more useful in situations like binary network protocols where the exact size matters for correctness.

Maybe fieldless enums are, by default, mapped on u8 ? This attribute then, would be redundant. Thanks for completing with network example. It sounds a bit like gcc attribute(packed) to control padding.

Regards.

I think the discriminant is made just big enough by the compiler to make all values fit.

1 Like

Yes, your code clearly shows that above 256 variants, the compiler maps them on a larger type. Nice demonstration.

This topic was automatically closed 90 days after the last reply. We invite you to open a new topic if you have further questions or comments.