Let me start by saying I really loved reading your code, respect for on your approach to learning rust and congratulations on such a great first project. It took me a little longer to reply than I wanted, I see you got some great feedback already and have implemented it!
Here are a few more idiomatic patterns & thoughts.
These are snippets and I wrote them without an IDE, so please forgive any typos... They won't work just by copy-paste but they should get you to a point that the compiler guides you the rest of the way.
Code organisation
main wraps lib
Placing your logic in a lib and wrapping it with a binary that simply handles the CLI:
- allows for better testing of the main functionality
- let's you and others reuse the logic outside the app in future
- increases your reach - the lib is on crates.io & docs.rs
- saves the
main() / inner_main() split
Like this:
ahtapot
- src
- main.rs
- lib.rs
- tests
- file_formats.rs
- invalid_formats.rs
- assets
- image1.gif
- image1.png
- not_an_image.gif
- text_file.txt
- Cargo.toml
Cargo.toml
[package]
name = "ahtapot"
version = "0.1.0"
edition = "2024"
[lib]
[[bin]]
name = "aht"
path = "src/main.rs"
[dependencies]
clap = { version = "4.6.1", features = ["derive"] }
image = "0.25.10"
main.rs
use ahtapot::{Image}
...
Handle input validation directly in main
I always find it easiest to have main read like a "list of tasks to do".
I know others, with lots of experience have other views, but I also find that main returning Result is fine, as long as I'm happy just having exit codes 0 / 1 and a slightly more technical error message.
- The output formatting is
"Error: {:?}" which means the debug representation of the error prefixed by "Error: "
- depending on the error this might be nice or might be uglier than the
Display formatted version
- if stderr is unavailable (unlikely)
Result handles this without panicing
- you can find out more here std::process::Termination
either way you could simplify by bringing the logic into main/inner_main
fn main() -> Result<(), AhtError> {
let args = Args::try_parse()?;
let dir = fs::read_dir(Args.dir)?;
if dir.is_empty() {
...
}
for (i, filename) in dir.iter().enumerate() {
...
}
Ok(())
}
Take advantage of standard traits & conventions
Consolidated Error handling with From
One of the first traits to look at is std::convert::From. This will also let you create your own error type, which is very idiomatic:
lib.rs
/// Error types used by aht lib & ahtapot binary
pub enum AhtError {
/// An error for the CLI app ahtapot
CLIError(clap::Error),
IOError(io::Error),
ImageError,
}
/// io::Errors give an AhtError::IOError
impl From<io::Error> for AhtError {
fn from(err: io::Error) -> AhtError {
AhtError::IOError(err)
}
}
// you might not need IOError if ImageError already does this, or you might want it yourself ...
...
// Also think about (start and let the compiler & docs.rs guide you to get everything right):
impl Error for AhtError {
fn source ...
}
Document everything
You might notice any comments above that could stay in the code have /// (3) not just //(2).
These comments will be turned into documentation and automatically published to docs.rs if you publish your crate to crates.io
They will also show up in your IDE to help you when coding.
Use ? rather than LBYL checks
Do you really need to check path.is_file() or will Image::new()? handle this fine.
Or do you actually have a logic bug ... and any resizing error will break your for loop when you would rather just skip that file?
Derive everything you can for your types
The API guidelines explain this really well.
tldr; it's really annoying to try to do something with a type you made and be told Foo doesn't implement Clone etc. It's even more annoying when it's from someone else's library.
Put the following on top of every struct and enum then delete the bits the compiler complains about:
#[derive(Debug, Clone, Copy, Eq, PartialEq, Ord PartialOrd)]
Use well known & popular crates (once you've understood how to do something yourself)
clap for argument parsing
Great, that you first looked at how argument parsing works by doing it yourself!
derive_more
To make your types easier to use
thiserror & anyhow for errors
Once you've hand-rolled an Error enum of your own you might want to use thiserror - lots of people do, personally I like to do errors by hand.
Anyhow provides an even lazier option, everything can be an anyhow::Error. It's good for quick & dirty (e.g. in your main/inner_main) if you want to keep lib errors & app errors as separate concerns. You can just return anyhow::Result<()> and use ? on anything: image::ImageError, io::Error, aht::AhtError, ...
