I wrote a simple file differ as my first program in rust. It was quite enjoyable, and it made me aspire to see what could be done "better". So I was wondering if anyone would be interested in doing a simple code review.
What I don't like about my solution is the loops in combination with the match. It's a lot of code 
, maybe it can be expressed more concise while maintaining the readability?
Patterns are compositional; you don't have to match every layer of structure separately. You can just say Some(Ok(_)) in the same pattern.
Your code can be simplified a lot in several other places, too, but I'm on mobile right now. I'll try to write up a more idiomatic version soon.
This bit jumped out at me:
First, don't ever name something starting with an underscore unless it's intentionally unused. (That's the pattern to silence the dead code lint, so you shouldn't use it elsewhere.)
Then be sure to read the result method overview. That whole match is just
file2 = File::open(p).expect("the second file should have existed");`
And you don't need to declare all the variables up front either. match is an expression, so you could replace the match path2 (line 25) with
let file2 = match path2 {
Some(p) => File::open(p).expect("the second file should have existed"),
None => synopsis(),
};
chopping down 11 lines to just 4.
Similarly, check out the option method overview. You have a "I want them both to be Some pattern between the paths. That's called zip, so you could do something like
let Some((path1, path2)) = path1.zip(path2)
else { synopsis() };
And now both paths are just Strings, not Option<String>, so they'll be easier to use in the code later on.
As complement to this reference-like document, you might want check guide-like The Book's error handling section to review context.
Reduce code on this "unpack" process.
loop {
match (nextline1, nextline2) {
(Some(Ok(line1: String)), Some(Ok(line2: String))) => {
// 25 lines: let mut chars1 = line1.chars();----------------------
},
(Some(_), Some(_)) => {
panic!("oh no");
}
(Some(line1: Result<String, Error>), None) => {
// 8 lines: match line1 {----------------------------------------
},
(None, Some(line2: Result<String, Error>)) => {
// 8 lines: match line2 {----------------------------------------
},
(None, None) => {
// 2 lines: no more lines left in either file--------------------
},
}
diff.push(ch: '\n');
nextline1 = buf_read1.next();
nextline2 = buf_read2.next();
}
Also worth note that code reusability is important, as The Book said. This section also show a much regular way to open file and read it's content, I'd recommend review this whole chapter if you don't want reinvent a wheel.
Rust's standard library are usually low level, so combine these complex to your own intent require much more logic and line of code than some other high level language.
Always try to design and separate code to their own space before real coding, although this may difficult(Same as my project doesn't organized well), but it just bring problem out right now, rather than hide in cargo check when sometime you change code for extending it.
Thx a lot, I really appreciate you all 
I took your feedback and updated the program: diff: refactor · zwarag/rust@0db54ae · GitHub
It was really fun and I might try to do a simple grep program next
here are a few suggestions:
Use unwrap() instead of match to unwrap the Result values returned by lines(). This can make your code more concise and easier to read. For example, instead of:
match (nextline1, nextline2) {
(Some(line1), Some(line2)) => {
match (line1, line2) {
(Ok(line1), Ok(line2)) => {
// ...
}
_ => { /* handle error */ }
}
}
_ => { /* handle error */ }
}
You can write:
let line1 = nextline1.unwrap()?;
let line2 = nextline2.unwrap()?;
This assumes that you're OK with your program panicking if there is an error reading a line from either file. If you'd rather handle the errors more gracefully, you can use match or ? to handle the Result values explicitly.
Use for loops instead of loop loops with next(). This can make your code more concise and easier to read. For example, instead of:
let mut chars1 = line1.chars();
let mut chars2 = line2.chars();
let mut char1 = chars1.next();
let mut char2 = chars2.next();
loop {
match (char1, char2) {
(Some(c1), Some(c2)) => {
// ...
}
_ => break,
}
char1 = chars1.next();
char2 = chars2.next();
}
You can write:
for (c1, c2) in line1.chars().zip(line2.chars()) {
// ...
}
This assumes that you want to compare characters one-by-one between the two lines. If you'd rather compare whole lines at a time, you can modify this loop accordingly.
Consider using the zip_longest method from the itertools crate instead of manually zipping the lines. This can simplify your code and make it more concise. For example, instead of:
let mut nextline1 = buf_read1.next();
let mut nextline2 = buf_read2.next();
loop {
match (nextline1, nextline2) {
(Some(line1), Some(line2)) => {
// ...
}
_ => break,
}
nextline1 = buf_read1.next();
nextline2 = buf_read2.next();
}
You can write:
use itertools::Itertools;
for (line1, line2) in buf_read1.lines().zip_longest(buf_read2.lines()) {
// ...
}
This assumes that you've added the itertools crate to your Cargo.toml file.
I hope these suggestions are helpful!
This topic was automatically closed 90 days after the last reply. We invite you to open a new topic if you have further questions or comments.