My first Rust crate - hints welcome!

help
1

Hello!

I am following the Rust community since it early days, but never had the opportunity to create something with this interesting looking language. Because I wanted to use the Bittrex API in a project, I decided to give Rust a try :slight_smile:

I created a wrapper around the Bittrex API and tried to do my best to be 'Rust Best Practices' compliant.
You can find the libary on Github.

It would be great, if anyone has time to provide me some information about what I could have done better in regards of Code Style & Rust Best Pratices. And maybe my project structure isn't like it should be. Any constructive criticism is welcome!

Thank you very much! :slight_smile:

2 Likes
2

I just created a couple issues to help you get started. Overall the crate looks quite good, you may want to work on the top-level documentation though. For example, as the crate docs (i.e. the //! ... docs at the top of lib.rs) you usually have a paragraph explaining what the crate does and then a couple examples which exercise the crate and show off a couple basic use cases.

Also, I know we make a big deal about not allocating in Rust, but in this case it makes sense for the BittrexAPI to contain owned strings instead of &'a str. A BittrexAPI is the main type in your library so it'll end up living for a long time anyway, as such forcing it to be bound by a particular lifetime doesn't really make sense.

Plus the cost of allocating and copying a string pales in comparison to the amount of time it takes for a single network call...

Also, what use is the Default impl for BittrexAPI? I see you've used ..Default::default() to fill in the missing fields in your constructors (that looks nice by the way, it's quite an idiomatic way of doing things :100:), but I can't see any other uses of BittrexAPI::default() in src/api.rs.

impl<'a> Default for BittrexAPI<'a> {
    fn default() -> BittrexAPI<'a> {
        BittrexAPI {
            api_url: "https://bittrex.com/api/v1.1",
            api_key: "",
            api_secret: "",
            http_proxy: None,
            https_proxy: None
        }
    }

Usually you'll only implement Default if your type has a logical set of defaults someone could use straight out of the box. Given the api key and secret are initialized to an empty string and then there isn't any way to set the key and secret, if I were to create a default client with BittrexAPI::default(), it's be effectively useless.

One other note... rustfmt is your friend :wink: I've got my IDE set up to format on save. Clippy is sometimes useful too, although your code looks pretty good so I doubt much will show up.

4 Likes
3

That was exactly what I was hoping for! Thank you very much for the feedback! I already changed most of the things you mentioned.

The BittrexClient::default() implentation was mainly done to provide a default value for the api_url. I want to make the url overrideable - for example to provide an url for the tests. But you are right, that in this case I could just provide the default url in the constructors.

And I installed clippy and rustfmt now :wink:

Thank you again for the feedback!

1 Like
4

Looks good - nice job!

One quick suggestion is you have lots of return types like Result<Something, BittrexError. You can pub type Result<T> = std::result::Result<T, BittrexError> and then use that alias to avoid repetitive BittrexError declarations.

1 Like
5

You can also clean up some code. For instance, if you see any unwrap calls, there's usually a cleaner way to do it. There are some of these there.

Another pattern I spotted was of the form:

fn foo(...) -> SomeReturnType {
   match some.bool.expression {
      true => ...,
      false => ...

   }
}

Instead you can just do:

fn foo(...) -> ... {
  if some.bool.condition { ... } else { ... }
}

ie the match is a bit "heavy" for these cases.

6

Good advice. I'd also suggest familiarizing oneself with Cow. In particular, if it's possible that sometimes a string literal is used whereas other times a dynamic string is created, then using Cow<'static, str> is a nice compromise. This isn't really relevant to the BittrexClient for the reasons @Michael-F-Bryan mentions, but it's a good tool to know about.

1 Like
7

clippy, which @Michael-F-Bryan mentioned above, is good for catching anti-patterns like the one @vitalyd described. For now you need nightly Rust to build it, but rustup makes it easy to switch between channels, and you can use CI to ensure that your code still works on other channels - in fact it's generally recommended to test on all three. rustfmt is also a good idea, but be aware that it recently switched to using nightly-only APIs, so if you're using nightly Rust you need to cargo install rustfmt-nightly, not rustfmt.

1 Like