Hi ,
This is my second Rust program, I would love some feedback
https://github.com/o2sh/onefetch
Thanks!
Hi ,
This is my second Rust program, I would love some feedback
https://github.com/o2sh/onefetch
Thanks!
Cool little tool - the ASCII language logos are extremely cute
The main tip I'd give you is to get into the habit of running Clippy on your codebase if you're not already - it's a really helpful tool for spotting common issues in Rust code.
A few things that it picked up when I ran it on your code (all of which are fairly minor):
Struct { field: field }
can become Struct { field }
. This will feel familiar if you've used JavaScript in recent years!self.authors.len() > 0
can be more clearly expressed as !self.authors.is_empty()
.unwrap_or
- this is ineffecient, as the function will get run even if the unwrap is successful. Instead, use unwrap_or_else
, which takes a closure as an argument, allowing you to hold off on constructing the default until it's actually needed!if let
syntax.url.split('/')
(with a char
literal) is slightly more efficient than url.split("/")
(with a &str
literal).The only other thing that sprung out to me is that I think your implementation of Display
for Info
could get away with a few less String
allocations by using write!
/writeln!
to write directly into your buffers:
impl fmt::Display for Info {
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
let mut buffer = String::new();
let color = get_color(&self.language);
writeln!(
buffer,
"{}{}",
"Project: ".color(color).bold(),
self.project_name
)?;
writeln!(
buffer,
"{}{}",
"Language: ".color(color).bold(),
self.language
)?;
if !self.authors.is_empty() {
let title = if self.authors.len() > 1 {
"Authors: "
} else {
"Author: "
};
writeln!(
buffer,
"{}{}",
title.color(color).bold(),
self.authors.first().unwrap()
)?;
let title = " ".repeat(title.len());
for author in self.authors.iter().skip(1) {
writeln!(buffer, "{}{}", title.color(color).bold(), author)?;
}
}
writeln!(buffer, "{}{}", "Repo: ".color(color).bold(), self.repo)?;
writeln!(
buffer,
"{}{}",
"Number of lines: ".color(color).bold(),
self.number_of_lines
)?;
writeln!(
buffer,
"{}{}",
"License: ".color(color).bold(),
self.license
)?;
let logo = self.get_ascii();
let mut lines = buffer.lines();
let left_pad = logo.lines().map(|l| l.len()).max().unwrap_or(0);
for a in logo.lines() {
let b = match lines.next() {
Some(line) => line,
None => "",
};
writeln!(
f,
"{:width$} {}",
a.color(color).bold(),
b,
width = left_pad
)?;
}
Ok(())
}
}
I have no idea if this is more performant, but it shouldn't make things any worse, and it's a little cleaner to my eye!
If you wanted to go optimization crazy, you could probably interleave the writing of the logo and the data in order to avoid allocating altogether, but it'd probably not worth the added complexity
Anyway, there goes half an hour of my life, hope this helped!
Done.
Great feedback, thanks a lot for taking the time