Apply some idiomatic shortcuts

[frol]

rwmstatus/src/main.rs

Line 42 in 676830f

if display == ptr::null_mut() {

I would use display.is_null()

rwmstatus/src/lib.rs

Lines 30 to 35 in 676830f

	fn read_file(base: &PathBuf, filename: &str) -> io::Result<String> {
	let mut file = File::open(base.join(filename))?;
	let mut contents = String::new();
	file.read_to_string(&mut contents)?;
	Ok(contents)
	}

There is a built-in std::fs::read_to_string

Here are a few shortcuts:

rwmstatus/src/lib.rs

Lines 68 to 75 in 676830f

	match read_file(&batt, "present") {
	Ok(contents) => {
	if !contents.starts_with('1') {
	return format!("not present");
	}
	}
	Err(_) => return format!(""),
	};

if let Ok(contents) = read_file(&batt, "present") {
    if !contents.starts_with('1') {
        return "not present".into();
    }
} else {
    return "".into();
}

rwmstatus/src/lib.rs

Lines 77 to 85 in 676830f

	let co = match read_file(&batt, "charge_full_design") {
	Ok(contents) => contents,
	Err(_) => {
	match read_file(&batt, "energy_full_design") {
	Ok(contents) => contents,
	Err(_) => return format!(""),
	}
	}
	};

read_file(&batt, "charge_full_design")
    .or_else(|_| read_file(&batt, "energy_full_design"))
    .unwrap_or_else(|| "".into());

rwmstatus/src/lib.rs

Lines 87 to 90 in 676830f

	let desired_capacity: u64 = match co.trim().parse() {
	Ok(val) => val,
	Err(_) => return format!("invalid"),
	};

let desired_capacity: u64 = co.trim().parse().map_err(|_| "invalid".into())?;

[Wojtek242]

Thanks for taking the time to read the code and provide these pointers! I found them very helpful.

I have now applied them to the code. There are some differences with what you suggested as I changed how I handle the errors.

I will keep the issue open for some time if you want to have another look at some point.

[frol]

rwmstatus/src/lib.rs

Lines 128 to 136 in bd8df56

	let dir_filtered = dir.filter(|path_result| match path_result {
	Ok(path) => {
	match path.file_name().to_str() {
	Some(entry) => entry.starts_with(prefix),
	None => false,
	}
	}
	Err(_) => false,
	});

let dir_filtered = dir.filter(|path_result| {
    path_result
        .ok()
        .and_then(|path| path.file_name().to_str())
        .filter(|entry| entry.starts_with(prefix))
        .is_ok()
});

rwmstatus/src/lib.rs

Lines 138 to 143 in bd8df56

	let mut paths: Vec<PathBuf> = dir_filtered
	.map(|path_result| match path_result {
	Ok(path) => path.path(),
	Err(_) => panic!("Unexpected file path"),
	})
	.collect();

let mut paths: Vec<PathBuf> = dir_filtered
    .map(|path_result| path_result.expect("Unexpected file path").path())
    .collect();

I would also move

rwmstatus/src/main.rs

Line 43 in a6d3690

let mut stats = vec![];

out of the loop and only call stats.clear() at the beginning of the loop.

It also better to use _else variants of .unwrap_or* family to avoid unnecessary allocations.

format!("") is equivalent to "".into().

[frol]

By the way, use clippy to learn best practices and fmt to keep your code style consistent.

[Wojtek242]

Thanks for the further comments! I've applied all your new suggestions.

However, your suggestion to fix get_paths didn't work like that as the borrow checker wasn't happy with it. As I understand it, it was because ok() and and_then() move the value within the enum which filter does not like. Also, file_name() returns a reference which doesn't live past the end of the and_then in which it is used.

In the end, by rewriting the two blocks slightly differently, I did get it to work using Result/Option methods instead of matches. So from this

rwmstatus/src/lib.rs

Lines 128 to 143 in a6d3690

	let dir_filtered = dir.filter(|path_result| match path_result {
	Ok(path) => {
	match path.file_name().to_str() {
	Some(entry) => entry.starts_with(prefix),
	None => false,
	}
	}
	Err(_) => false,
	});
	let mut paths: Vec<PathBuf> = dir_filtered
	.map(|path_result| match path_result {
	Ok(path) => path.path(),
	Err(_) => panic!("Unexpected file path"),
	})
	.collect();

I went to this

rwmstatus/src/lib.rs

Lines 128 to 139 in ce2fe13

	let dir_contents = dir.filter_map(|path_result| {
	path_result.ok().and_then(|path| Some(path.path()))
	});
	let mut paths: Vec<PathBuf> = dir_contents
	.filter(|path| {
	path.file_name()
	.and_then(|os_str| os_str.to_str())
	.filter(|entry| entry.starts_with(prefix))
	.is_some()
	})
	.collect();

Here, file_name() is called first and thus all the subsequent calls only move the reference and there no longer is a lifetime issue. This also has the advantage of eliminating the panic! branch (which was hopefully unhittable anyway).

I do use fmt, though I use an old version, since the newest one requires you to be on the nightly channel, which I do not want to use.

And I haven't been using clippy as I prefer not to use rustup and clippy is easiest to install with rustup. I prefer to stick to the distribution provided toolchain. However, I did run clippy on the codebase in a VM in the end anyway and will try to do so on occasion.

[frol]

I am glad my tips helped you! Good luck!