Requesting a Code Review

code review
1

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 :slight_smile:, maybe it can be expressed more concise while maintaining the readability?

2

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.

2 Likes
3

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.

2 Likes
4

As complement to this reference-like document, you might want check guide-like The Book's error handling section to review context.

5

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

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.

7

Thx a lot, I really appreciate you all :smiley:
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 :slight_smile: