Code review wanted, looking for improvements

code review
1

Hello,
I'm a Rust beginner and recently made an Elixir package using Rustler which makes it easy to use Rust as a NIF within Elixir/Erlang.

Although I managed to make everything work with not much effort (I love Rust so far!) the code seems very verbose to me. There is a lot of ok().unwrap() repetition and feels like there is a lot of room for improvement.

If anyone is willing to have a look and help out — everything is all here and the core logic is just about ~100LOC:

I'll appreciate all tips and ideas to improve. Thanks!

2

Generic advise:

  1. Always prefer foo.unwrap() to foo.ok().unwrap() (where foo: Result<_, _>).
    The former directly unwraps the result and panics with error info if it is Err(_).
    However, the latter discards the error info, so it will be hard to debug.
  2. Always prefer foo.expect("Some meaningful error message or context info here") to foo.unwrap().
    Result::expect() (and Option::expect()) is similar to Result::unwrap(), but expect() additionally prints the given message.
    foo.unwrap() does not tell why it can be unwrapped, but foo.expect() does.
    For example, I use expect() as below:
    • s.parse::<i32>().expect("Should never fail: s is already validated as i32 integer")
    • object.meta.expect("Caller must set the object metadata")

Conclusion: Replace foo.ok().unwrap() to foo.expect("message").

2 Likes
3

To create (pseudo) methods to some types, you can use extension trait technique.

Using this technique, you can write .map_put_expect(foo, bar, "msg") or something, instead of .map_put(foo, bar).expect("msg").

I'm not sure whether it is better to create .map_put_expect() function, but this is an example.
If you want to put similar codes into common function but also want to use method chains (i.e. want to use v.foo().bar() rather than bar(foo(v))), implementing extension traits would be useful.

1 Like
4

In addition to what's been said, already, you may also want to consider using my_result?.do_something() and returning the Err to the caller and let them handle it. You can even return a Result from main. I highly recommend you to read the error tutorial posted on this forum.

2 Likes
5

Thanks for the tips @nop_thread @Phlopsi! I'm going to read articles you've shared, they are very interesting, and come back to the code. I think the extension trait technique is something I'll definitely try!

6

I see a lot of .map_put(x.encode(env), y.encode(env)). lo48576 suggested a map_put_expect extension trait, but I think the trait I would want to see is one that lets you replace all of those with .map_put_encodable(x, y, env).

You might need something along the lines of:

// This may already exist, or it may be unnecessary if all your encodable
// objects are already the same type
trait Encodable {  
    type Target;
    fn encode(&self, env) -> Target;
}

trait MapPutEncoded {
    fn map_put_encoded<E1, E2>(x: E1, y: E2, env: Env) -> Something
    where
        E1: Encodable,
        E2: Encodable;
}

Edit: Reformatted code to remove horizontal scrolling.

7

This topic was automatically closed 90 days after the last reply. New replies are no longer allowed.