CODEWITHSUNDEEP
rustidioms
Nathan Long
Nathan Long

Reputation: 126062

Read a file and get an array of strings

I want to read a file and get back a vector of Strings. The following function works, but is there a more concise or idiomatic way?

use std::fs::File;
use std::io::Read;

fn lines_from_file(filename: &str) -> Vec<String> {
    let mut file = match File::open(filename) {
        Ok(file) => file,
        Err(_) => panic!("no such file"),
    };
    let mut file_contents = String::new();
    file.read_to_string(&mut file_contents)
        .ok()
        .expect("failed to read!");
    let lines: Vec<String> = file_contents.split("\n")
        .map(|s: &str| s.to_string())
        .collect();
    lines
}

Some things that seem suboptimal to me:

How can this be improved?

Upvotes: 50

Views: 37549

Answers (2)

Shepmaster
Shepmaster

Reputation: 431589

DK.'s answer is quite right and has great explanation. However, you stated:

Read a file and get an array of strings

Rust arrays have a fixed length, known at compile time, so I assume you really mean "vector". I would write it like this:

use std::{
    fs::File,
    io::{prelude::*, BufReader},
    path::Path,
};

fn lines_from_file(filename: impl AsRef<Path>) -> Vec<String> {
    let file = File::open(filename).expect("no such file");
    let buf = BufReader::new(file);
    buf.lines()
        .map(|l| l.expect("Could not parse line"))
        .collect()
}

// ---

fn main() {
    let lines = lines_from_file("/etc/hosts");
    for line in lines {
        println!("{:?}", line);
    }
}
  1. As in the other answer, it's worth it to use a generic type that implements AsRef for the filename.
  2. Result::expect shortens the panic on Err.
  3. BufRead::lines handles multiple types of newlines, not just "\n".
  4. BufRead::lines also gives you separately allocated Strings, instead of one big glob.
  5. There's no reason to collect to a temporary variable just to return it. There's especially no reason to repeat the type (Vec<String>).

If you wanted to return a Result on failure, you can squash the implementation down to one line if you want:

use std::{
    fs::File,
    io::{self, BufRead, BufReader},
    path::Path,
};

fn lines_from_file(filename: impl AsRef<Path>) -> io::Result<Vec<String>> {
    BufReader::new(File::open(filename)?).lines().collect()
}

// ---

fn main() {
    let lines = lines_from_file("/etc/hosts").expect("Could not load lines");
    for line in lines {
        println!("{:?}", line);
    }
}

Upvotes: 40

DK.
DK.

Reputation: 59095

As BurntSushi said, you could just use the lines() iterator. But, to address your question as-is:

  • You should probably read Error Handling in Rust; those unwrap()s should be turned into ?s, with the function's result becoming a Result<Vec<String>, E> for some reasonable E. Here, we reuse the io::Result type alias.

  • Use the lines() iterator. The other thing you can do is read the whole file into a String and return that; there's a lines() iterator for strings as well.

  • This one you can't do anything about: file_contents owns its contents, and you can't split them up into multiple, owned Strings. The only thing you can do is borrow the contents of each line, then convert that into a new String. That said, the way you've put this implies that you believe creating a &str is expensive; it isn't. It's literally just computing a pair of offsets and returning those. A &str slice is effectively equivalent to (*const u8, usize).

Here's a modified version which does basically the same thing:

use std::fs::File;
use std::io::{self, BufRead};
use std::path::Path;

fn lines_from_file<P>(filename: P) -> io::Result<io::Lines<io::BufReader<File>>>
where
    P: AsRef<Path>,
{
    let file = File::open(filename)?;
    Ok(io::BufReader::new(file).lines())
}

One other change I made: filename is now a generic P: AsRef<Path>, because that's what File::open wants, so it will accept more types without needing conversion.

Upvotes: 22

Related Questions