I'm an experienced programmer, mainly with C++/C#. I just started learning rust and I started to write code for a learning project while I'm still learning. The project will be a key/value storage engine. I started by writing a File trait that will be responsible to abstract low level file operations. I also made a first implementation of this trait in the forme of a MemoryFile struct. As it's name imply this is not a real file on disk, but an implementation of the trait that stores data in memory.
I would really appreciate it if you could review my code and let me know what I can improve. Particularly if I do things in a non "rustic" way.
You can review the whole repo, but if you want to concentrate on a single file, the src/fs/memory_file.rs is probably the one with most code.
The first thing that caught my eye was, that your code does neither have a LICENSE, nor a README file. This may prevent other users from contributing to your codebase.
The comments of sync() are not formatted as a docstring (triple-/). Hence the doctests will not be run by cargo.
Also in your unit tests you inconsistently alternate between comments (//) and docstrings (///).
RandomBlob::new_default should be implementation of Default trait, not a static method.
Consider implementing AsRef<[u8] and AsMut<[u8]> for RandomBlob.
Use Path/PathBuf to represent filenames instead of String.
Consider renaming trait File to something like Storage. File can by confused with std::fs::File struct (or other file abstractions).
API of the File interface is really weird.
You are partially implementing std::io::{Read, Write} traits. You should probably just implements those traits directy, for better support (and reuse std::io::Error instead your own). You can do that by making them supertraits of File.
Your API is also easy to misuse. For example you must first call open, before you call write. But nothing in your API enforces that. Consider returning from open some sort of guard, on which in turn you can call write. That would make impossible calling write before opening file. Here is a quick draft of how you do this:
use std::io::{Read, Write, Error};
pub trait StorageOperations: Read + Write {
// add here some specific operations which are not covered by Read + Write
}
pub trait Storage {
type Guard: StorageOperations;
fn create(&self) -> Result<Self::Guard, Error>;
fn open(&self) -> Result<Self::Guard, Error>;
// this could instead be a method on `StorageOperations`
fn close(&self, guard: Self::Guard) -> Result<(), Error>;
}
Some methods in File (like open, create, close) taking &mut self seems odd to me. I cannot for sure tell that this is wrong, but I would try writing them first as &self.
Do not use glob imports (use crate::fs::file::*;).
MemoryFile::new_with_data should take Vec<u8> instead of &[u8]. Reason beeing that the caller should decide how to allocate data. If your caller already has a Vec, then you would needlessly allocate new memory.
expect("create should not fail") in tests doesn't really add new context. Note that tests can return Result<(), some error>, so you can use ? instead of unwrap/expect.
Not a specific problem, but I would recommend to read Rust API guidelines, to familiarize yourself with standards on designing APIs in Rust.
Thanks Schard and akrauze. Theses comment/suggestions are exactly what I was looking for. I feel I'm still writing Rust as if it was C++ and most of the items you pointed out I just don't know about yet.
I'll research each one and make the suggested corrections.
You're welcome. One more general suggestion I would add is, that making yourself familiar with Rust's standard library will help you better understand how to do things "the Rust way". Standard library is very well written, has excellent documentation and unlike other languages (ekhm C/C++) you can actually read source code of standard lib and not only understand it, but learn from it (look for links to source on the right of every documented item in rustdoc generated documentation).
For example, when you will look though std::io module, you will find that functions that take a filename, on which they operate, take P: AsRef<Path> instead of something like &str. This can lead you to documentation of std::path module, which explains abstractions Rust uses to describe filesystem paths.
Look also which traits are implemented by each struct. There are a ton of helpful "utility" traits that Rust uses to express many things, that you would otherwise do manually in other languages. Default, AsRef, From/Into, TryFrom/TryInto, Deref, Debug, Display, Clone, Iterator, IntoIterator, just to name a few.