Feedback first console application (Rust basics, MySQL, File operations, memory operations, tests, doc comments,...)

Hello!

In this post I started my Rust journey by writing simple code covering structs, traits, crates,... to get the hang of Rust.
I've expanded that to a console application allowing you to manage your cats. You can choose if you want to save your cats in memory, a file, or a MySQL Database.

Now I'd like some feedback on this code. I'd like to know what can be improved, how the code can get more readable, how everything can be done more 'Rustier', if the project-structure (use of modules etc) can be improved,...

Code: https://pastebin.com/TprHzaFA
(Max characters exceeded.)

Until now I actually like Rust a lot. I'm now planning on making a GUI for this application in Rust, and to than make a web-application out of it using a MVC-framework. Than I think I have a grasp on the language. :wink:

Thanks!

This isn't regarding the code per-se, but I would recommend uploading the project to GitHub and breaking each module out into its own file. It makes the project a lot easier to get an understanding of at a glance, and also means people can check the code out and run it on their own computers.

Instead of a Cat having CatColour and CatRace fields, I'd just call them Colour and Race. There's no real need for the extra Cat prefix.

Instead of the explicit Cat::to_string() method, you should look at implementing std::fmt::Display. That'll allow you to println!() a Cat directly, and the ToString trait (which has a to_string() method) gets automatically implemented for anything implementing Display.

Instead of having a CatColour enum with a pre-defined set of colours, what about allowing a colour to be any RGB value and define constants like CatColour::WHITE for the common colours? One day someone is going to want to add a tabby cat or some random colour like turqouise, so you may want to make this sort of thing more flexible...
It depends on how far down the rabbit-hole you want to go, of course.

You could say the same about CatRace. An enum is best used when you've got a closed set of possibilities known ahead of time, but the CatRace seems like it'll have a possibly unbounded number of values. Especially when you take into account things like mixed races. I'm guessing the easiest solution would be to have a CatRace struct with a single user-defined name: String field.

If you're using cargo I'd recommend checking out some sort of readline library to read input, that way your users get nice things like tab-completion and history and it feels like a proper REPL. I've had good experiences with rustyline.

You don't need to use io::Write::flush(&mut io::stdout()), you should be able to do io::stdout().flush() if the std::io::Write trait is in scope.

When reading input, you probably don't want to print eprintln!("Corrupted input '{}' given!", input) and exit. A lot of tools will read input in a loop, printing out an error message and prompting again when there is an error. Your get() function may want to accept a prompt: &str parameter which gets print!()-ed before trying to read a line of input.

Looking at your DataManager trait I'm guessing you come from a more stereotypical OO language like C# or Java? The naming and methods look like some of the code I've seen people use when writing Android apps (e.g. "Data Access Object"). That's not necessarily a bad thing though, using a trait gives you a seam for testing and swapping out backends (e.g. mysql vs postgres vs file vs firebase). You should probably rename it to something which indicates it's in charge of storing and retrieving Cats... Maybe Cats or CatDatabase or CatStore? I dunno, naming things is hard :sweat_smile:

Something to keep in mind with your DataManager is that each operation can fail (due to a unique SQL constraint, broken database connection, etc.) so each method should return some sort of Result.

I don't think DataManager::read() should return a &Vec<Cat>. Because you're returning a reference it means the Cats are actually stored inside the DataManager somewhere (e.g. in a buffer/cache). Additionally, because of lifetimes it means you can't use the DataManager again until the returned value gets dropped. I'd just return a Result<Vec<Cat>, SomeDatabaseError>, returning a reference won't make a difference in performance if you're already talking to a database.

The DataManager::get() and DataManager::delete() methods should probably accept &str instead of &String. A String is actually a pointer to a str so you've got a bit of double-indirection going on. Plus if I wanted to delete all cats with the hard-coded name "steve" I'd need to copy it to an intermediate String, which is kinda annoying.


This review is getting a bit long and would probably be easier to do using a proper review interface like that provided by GitLab or GitHub, but hopefully you find some of my comments useful.

Despite the large number of points above I'm actually quite impressed with your code so far! While a couple things aren't the most idiomatic, the code overall is well structured and quite readable, it feels like you're the kind of coder people would want to work with.

I'm keen to see how far this goes :slightly_smiling_face:

1 Like

Thanks a lot for your helpful reply! I'll implement your suggestions!

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