How would I make my library cleaner and more idiomatic?

First of all, here's the link in case you don't care about the rest: https://github.com/Admicos/thatcord

So, I needed a Discord API library that worked with Futures 0.3 and Tokio 0.2 for another project I'm working on, so I decided to make my own.

Initially, this was going to be somewhat integrated to my project, but I realized I could extract it as a library, so it can be useful for more than just me.

Now, I am not the best at Rust, and in fact, I won't have any idea what I'm doing if the code involves more than just single-threaded, mixed procedural and OOP stuff. And now I am writing an async library with an event system based on a lot of hacks, with some macros to make it more straightforward, trying to shoehorn parts of OOP into Rust Traits.

I am 99% sure there is no substantial part in this library that can be considered idiomatic. So I require help.

How can I clean up this pile of spaghetti? What patterns/best practices can be applied to what parts? I have no clue.

Any feedback, as long as it's constructive, is appreciated.


tldr: code bad, how fix? thx

It's difficult to come with recommendations on the overall structure, but looking at a few files I do have some comments:

  1. Try to avoid the Rc/RefCell design if possible.
  2. Try to avoid storing event handlers because they make lifetimes a pain for the user. I'd prefer to just have a function that returns the next event, letting me dispact to however I want to handle it.
  3. I notice that you have some async fns that never call await internally, e.g. here. If you don't await anything, it doesn't make sense to make them async.
  4. Please don't make the error enum invisible to users. It's much harder to correctly handle errors if you make it private. I also recommend not using error libraries and just implementing the traits manually in libraries, because they usually force the user to use the same error handling library. I'm not familiar if snafu does this, but it's the case for most of them.

You should also consider if you can parse the json directly into the corresponding type instead of going through serde_json::Value.

3 Likes

On that note, #[non_exhaustive], which has been stabilized in 1.40 might come in handy, because it allows adding new errors later on without breaking backwards-compatibility.

facepalm

I spent so much time trying to mold traits into the abomination that is my event handlers, that I forgot this was a thing. Thanks for reminding!

Also, just a quick question: Discord::connect() starts an internal loop to receive messages, so it won't return until the connection closes. How would I handle this with the event polling (?) system you're describing? With a closure, maybe?

I was just copying what snafu's documentation did with hiding the enum.

For manually implementing errors, snafu says it supports libraries, but I haven't tried to handle library errors, so it might just make the user depend on itself, not sure.


edit:

The Discord gateway API returns everything inside an gateway payload object. So, unless serde can do some impressive matching with the JSON structure and whatnot, there needs to be a value object until I can determine what the event actually is.

I can give some more details when I get home, but here's some quick answers.

Polling: why not just have a next event method that runs the loop until it gets a message?

Serde: how are the payloads determined? Serde has some impressive options for parsing into enums, which you can read more about here.

The websocket connection needs to stay up, or I'd need to do some juggling with connection resuming (which I still need to implement) for every single event, which doesn't seem pretty efficient.

That's pretty impressive, actually. It might just work with Discord's payload structure.