Refactor suggestions about my git hook written in Rust

Hello!

I want to learn Rust, I've taken some courses and I've solved many simple problems by myself using Rust. But that's not enough, so I decided to build a side project.

I wanted to build a git hook, so before I commit my files, the hook will launch, analyze my diffs, found new comments or some other keywords and if the code have these words, it will exit with error code and abort the commit.

I wanted to build this program because when I am getting involved in a new project, I fill the code with many tags and log messages to better understand what is happening underneath the hood. But often I put too many and I forgot to erase some of these tags when committing

I first build the project using my "go to" language, Ruby, and the I copied the project using Rust. Here is the original code on Ruby: GitHub - KarlHeitmann/chuletas_cleanup and here is the clone GitHub - KarlHeitmann/chuletas_warning_hook: A little script in rust desifned to warn you if there are any "cheat sheet" (chuletas or torpedoes in spanish) you are committing in your source code.

On README.md in Ruby there is a large explanation of the project. Both versions are basic and functional. But I want to ask you guys, what could be improved on the Rust code. I used structs, traits, and some few matchers. But I think the code can be refactored much more better. Any feedback on refactorization will be appreciated.

Fun fact: on colloquial spanish, "chuleta" means "cheat sheet", but it is used when you go to an exam on school, and you hide your cheat sheet, or you write on your forearm the formulas for the exam.

There are lot of things that can be improved in this little script (showing the line on the file where the offense is found, add other languages, etc), but right now I am interested in how many different ways I can write this code to do the same thing.

I wanted to build this hook because I wanted to prevent -my- tags, comments and logs being committed, not the previous tags, comments and logs that other people may have put on the source code before.

Best,

Karl

Welcome!
I recommend formatting everything with the standard Rust formatter rustfmt by running cargo fmt. After that, run cargo clippy to run clippy, a linter that will recommend some changes to make your code more idiomatic (for example, replacing format!("") with String::new()).

The Display impls look a bit suspect. They replace the cadena variable over and over again and finally write whatever key-value pair happened to be processed last. It looks like you might be looking for String::push_str instead?

The unwrap at chuletas_warning_hook/main.rs at 8351f8aa9b9791018113a2e5976fd3aca850bf3c · KarlHeitmann/chuletas_warning_hook · GitHub causes the program to crash if there are any files with no . in their names. You can instead handle such cases like so

        if file
            .rsplit_once('.')
            .map(|(_, ext)| ext != "rb") // turns the Option<(&str, &str)> into an Option<bool>
            .unwrap_or(true) // take the bool from the Option if it's Some, use true otherwise to skip the file
        {
            println!("Continue because {} it is not a ruby file", file);
            continue;
        } // use matcher

Another small change I would make is turn

    pub fn add_offense(& mut self, file: &str, offense: Offenses) {
        self.data.insert(file.to_string(), offense);
    }

into

    pub fn add_offense(& mut self, file: String, offense: Offenses) {
        self.data.insert(file, offense);
    }

It doesn't really make a difference for a project this size, but the first signature forces you to pass a &str that is then turned into a String even if you already have a String.

2 Likes

Thank you Heliozoa!

I run those cargocommands and they were pretty helpful. fmt put the code in a nice format, and clippy showed me a lot of suggestions. String::new() was just the function I was looking for, much better than format!(""). There was an if blockthat clippy caught, and suggested me to use a matcher. I tried to implement it but I've had some issues. I will continue with this tomorrow, or the week end when I'll have a spare time to check it.

One of the features of Rust that I really like are these match sentences. I don't like very much if sentences or case - switch, and the match sentence is a nice way to approach conditions.

About your suggestion on line 38 of main.rs, I didn't know about the existence of unwrap_or, as soon as I'll finish the clippy part, I'll try your suggestion. Besides making the program more robust, it helps incrementing my Rust vocabulary. And the second suggestion I think it helps making the code less verbose.

Thank you!

1 Like