Replace string in file

Question

This is my first module I wrote in Rust. I'm new to the language and couldn't find a way to easily replace some words in a file, so that's my go at it. If there's already something like that and I just missed it, feel free to point me to it.

I'm looking for advice as to how I can make this more efficient and more "rustician" "Rustacean", so I can publish a crate with this for others to use.

Since I have to call "string".to_string() all the time, &str would be easier for this case here I guess, but would it be better? When should I generally use String and when &str?

main.rs

mod str_replace;

use str_replace::StrReplace;

fn main()
{
    let mut str_test = StrReplace::from_str("hello 1 2 1 2 hello bye");

    str_test.replace("hello".to_string(), "bye".to_string())
        .replace("1".to_string(), "0".to_string())
        .replace("2".to_string(), "1".to_string());

    println!("{:?}", *str_test);

    let mut file_test = StrReplace::from_file("src/test.txt");

    file_test.replace("hello".to_string(), "bye".to_string())
        .replace("1".to_string(), "0".to_string())
        .replace("2".to_string(), "1".to_string())
        .replace("\n".to_string(), " ".to_string());

    println!("{:?}", file_test.to_str());
    file_test.to_file("src/to_file_test.txt");

    let replace_here_test = StrReplace::replace_here("hello".to_string(), "bye".to_string(), "hello hello".to_string());
    println!("{:?}", replace_here_test);
}

str_replace.rs

use std::path::Path;
use std::fs::File;
use std::io::prelude::{Read, Write};
use std::ops::Deref;

#[derive(Debug)]
pub struct StrReplace
{
    /// String or &str?
    data: String,
}

impl StrReplace
{
    ///
    /// Creates StrReplace from the contents of a file at the given path
    ///
    /// Tried to wrap the "try!" macro around "file.read_to_string",
    /// like in the documentation, but I got an error:
    ///
    /// error[E0308]: mismatched types
    /// --> src/str_replace.rs:37:9
    /// |
    /// 37 |      try!(file.read_to_string(&mut data));
    /// |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected struct `str_replace::StrReplace`, found enum `std::result::Result`
    /// |
    /// = note: expected type `str_replace::StrReplace`
    /// = note:    found type `std::result::Result<_, _>`
    /// = note: this error originates in a macro outside of the current crate
    ///
    pub fn from_file(path: &str) -> StrReplace
    {
        let filepath = Path::new(path);
        let mut file = File::open(filepath).unwrap();
        let mut data = String::new();

        file.read_to_string(&mut data).expect("Failed to read file.");
        //try!(file.read_to_string(&mut data));

        StrReplace { data: data }
    }

    ///
    /// Creates StrReplace from a given &str
    ///
    /// or maybe from a String?
    ///
    pub fn from_str(str: &str) -> StrReplace
    {
        StrReplace { data: str.to_string() }
    }

    ///
    /// Replace the occurence of one string with another
    /// uses the std String.replace function
    /// returns self for chainability.
    ///
    /// any downsides to chaining this way?
    /// function arguments String or &str?
    ///
    pub fn replace(&mut self, search: String, replacement: String) -> &mut Self
    {
        self.data = self.data.replace(&*search, &*replacement);
        self
    }

    ///
    /// Writes the possibly mutated data to a file at the given destination
    ///
    /// Again, should dst be &str or String?
    /// Also when trying to use try! here I get an mismatched types error again,
    /// but this time with an other expected type:
    ///
    /// error[E0308]: mismatched types
    /// --> src/str_replace.rs:88:9
    /// |
    /// 88 |      try!(file.write_all(self.data.as_bytes()));
    /// |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected (), found enum `std::result::Result`
    /// |
    /// = note: expected type `()`
    /// = note:    found type `std::result::Result<_, _>`
    /// = note: this error originates in a macro outside of the current crate
    ///
    pub fn to_file(&self, dst: &str)
    {
        let mut file = File::create(dst).unwrap();
        file.write_all(self.data.as_bytes()).expect("Failed to write file.");
        //try!(file.write_all(self.data.as_bytes()));
    }

    ///
    /// Makes a &str out of StrReplace for further use
    ///
    /// Better to return &str or String?
    ///
    pub fn to_str(&self) -> &str
    {
        &*self.data
    }

    ///
    /// Replace the occurence of one string with another
    /// without creating a StrReplace
    ///
    /// Again, String or &str?
    ///
    pub fn replace_here(search: String, replacement: String, input: String) -> String
    {
        input.replace(&*search, &*replacement)
    }
}

/// Does it make sense to implement this? I guess yes
/// But when would it not make sense?
impl Deref for StrReplace
{
    /// String or &str?
    /// &str didn't work for me yet because of the lifetime thingy I have yet to fully understand
    type Target = String;

    fn deref(&self) -> &Self::Target
    {
        &self.data
    }
}

Answer 1

1. You should become familiar with rustfmt, which will help you enforce the community's coding style. For example, opening braces generally go on the same line (for functions, structs, impls, etc.):

   -fn main()
   -{
   +fn main() {
   

2. You should write real tests, not just a main method that exercises the code. That's more of an example.

3. "std String.replace function" - Functions are referred to using the double colon syntax: String::replace.

4. Speaking of that, don't explain the implementation details in documentation comments. Users of your code are the wrong audience for that.

5. Additionally, don't put "notes to oneself" as documentation comments. Regular comments are more appropriate for programmer-to-programmer communication.

6. There are typos in the doc comments (occurence). Some editors can be configured to automatically spell check comments, that might be worth investigating.

7. None of the arguments need to be Strings, which generally means that they should be &strs. Changing to &str reduces mandatory allocations (note the reduction of to_string) and also removes the need for &* (which probably could have just been & in the first place).

8. File::open and File::create take anything that can be converted to a Path, and offering only a &str limits the functionality of those methods. It's important to understand why a file path is not a string.

9. Read Why is it discouraged to accept a reference to a String (&String) or Vec (&Vec) as a function argument? to see why you should prefer &str over &String in the overwhelming majority of cases.

10. Storing a String requires that there is always one allocation; replacing a constant string just got more inefficient. Perhaps a Cow would be useful.

11. "Does it make sense to implement this?": No, you should not implement Deref because you are not creating a smart pointer. Do you really want people to be able to call file_test.capacity() or str_test.len()? What would those even mean?

12. For try! to work, you must return a Result. Re-read The Rust Programming Language chapter on error handling for an in-depth analysis.

13. Chaining involves making a decision and tradeoffs; &mut self chaining generally requires an extra line to establish a variable, self chaining requires ownership and potentially re-assignment of the variable. It comes down to expected use cases.

str_replace.rs

use std::path::Path;
use std::fs::File;
use std::io::prelude::{Read, Write};

#[derive(Debug)]
pub struct StrReplace {
    data: String,
}

impl StrReplace {
    /// Creates StrReplace from the contents of a file at the given path
    pub fn from_file(path: &str) -> StrReplace {
        let filepath = Path::new(path);
        let mut file = File::open(filepath).unwrap();
        let mut data = String::new();

        file.read_to_string(&mut data).expect("Failed to read file.");

        StrReplace { data: data }
    }

    /// Creates StrReplace from a given &str
    pub fn from_str(str: &str) -> StrReplace {
        StrReplace { data: str.to_string() }
    }

    /// Replace the occurence of one string with another
    /// returns self for chainability.

    pub fn replace(&mut self, search: &str, replacement: &str) -> &mut Self {
        self.data = self.data.replace(search, replacement);
        self
    }

    /// Writes the possibly mutated data to a file at the given destination
    pub fn to_file(&self, dst: &str) {
        let mut file = File::create(dst).unwrap();
        file.write_all(self.data.as_bytes()).expect("Failed to write file.");
    }

    /// Makes a &str out of StrReplace for further use
    pub fn to_str(&self) -> &str {
        &self.data
    }

    /// Replace the occurence of one string with another
    /// without creating a StrReplace
    pub fn replace_here(search: &str, replacement: &str, input: &str) -> String {
        input.replace(search, replacement)
    }
}

main.rs

mod str_replace;

use str_replace::StrReplace;

fn main() {
    let mut str_test = StrReplace::from_str("hello 1 2 1 2 hello bye");

    str_test.replace("hello", "bye")
        .replace("1", "0")
        .replace("2", "1");

    println!("{:?}", str_test);

    let str_test = "hello 1 2 1 2 hello bye"
        .replace("hello", "bye")
        .replace("1", "0")
        .replace("2", "1");

    println!("{:?}", str_test);


    let mut file_test = StrReplace::from_file("src/test.txt");

    file_test.replace("hello", "bye")
        .replace("1", "0")
        .replace("2", "1")
        .replace("\n", " ");

    println!("{:?}", file_test.to_str());
    file_test.to_file("src/to_file_test.txt");

    let replace_here_test = StrReplace::replace_here("hello", "bye", "hello hello");
    println!("{:?}", replace_here_test);
}

---

Now we get to the tough(er) love part of the review

I'm looking for advice [...] so I can publish a crate with this for others to use.

I don't want to discourage you or anyone else from publishing a crate, but I do not feel that this crate at this point in time would be a beneficial addition to the Rust ecosystem.

You have self-identified some weaknesses in your understanding of Rust, and that's an immense first step! You've also taken it upon yourself to seek out feedback on your code, something not a lot of people do. Again, this is a strong thing to have done.

However, I believe that you should have a solid understanding of ownership, lifetimes and error handling before publishing a crate like this. People will not be excited to use a library that panics and over-allocates memory.

As a concrete example, check out your code:

let mut str_test = StrReplace::from_str("hello 1 2 1 2 hello bye");

str_test.replace("hello", "bye")
    .replace("1", "0")
    .replace("2", "1");

println!("{:?}", str_test);

Compared to the standard library implementation:

let str_test = "hello 1 2 1 2 hello bye"
    .replace("hello", "bye")
    .replace("1", "0")
    .replace("2", "1");

println!("{:?}", str_test);

I'd say that the standard library version is easier to understand. It also only creates 3 Strings instead of 4. The file-based code is basically the same, except it has a "read an entire file into a String" helper function bolted in the middle. Once you have that code to load the String, the same code as above will work.

Again, please do not let me dissuade you from ever publishing a crate. Keep working at it and you will find something missing in the ecosystem that you can fill the gap for. At that point in time, you'll probably have honed your abilities greatly!

more "rustician"

On a light-hearted end note, the accepted term is Rustacean.