HiveBrain v1.2.0
Get Started
← Back to all entries
patternrustMinor

Replace string in file

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
filestringreplace

Problem

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
/// |

Solution

-
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() {


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

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

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

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

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

-
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).

-
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.

-
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.

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

-
"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?

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

-
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 publish

Code Snippets

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)
    }
}
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);
}
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);

Context

StackExchange Code Review Q#156540, answer score: 3

Revisions (0)

No revisions yet.