My First Rust Learning Project. Not Completed Yet But Wanted to Share

Hello, I’m new to Rust. The project I’m working on involves adding comments to code using artificial intelligence. I’d like to know how good my code is and what suggestions you might have for improving it. The project isn’t finished yet; some parts are static and some are dynamic. I’m open to feedback and eager to improve my skills in Rust.

Project Link: https://github.com/kaanboraoz/ai-comment-app

I’m new to the community and still learning English, so I might make some mistakes. My intention is not to advertise; I simply want to seek advice from experienced individuals.

Hi, Welcome Kaan.

I just have minor details to say that may help you in a long run.

I strongly recommend you to use cargo fmt. This will standardize your code with formatting.
I also checked your other projects too. I recommend that please explain what you did in commit messages little further, they are a bit hard to understand.

Seems like you've already known many Rust concepts which is awesome.

Welcome again.

3 Likes

Thank you so much for your suggestions I needed this :saluting_face::white_check_mark:

2 Likes

This looks good! I just have some random comments.

Some minor forum usage comments: I noticed this post's category is "tutorial" and I think it should be "code review". And I saw you selected a solution for this, which is normally only done for "help" posts. When someone replies to a post with a selected solution, a warning will appear saying that it has already been solved. I assume you want more feedback on this, and if so I would uncheck the solution.

        if let Some(captures) = re_start.captures(&normal_text) {
            if let Some(main_py_code) = captures.get(1) {
                return Some(main_py_code.as_str().to_string());
            }
        } else {
            println!("No match found");
        }

        None

Above, this won't always print a message when returning None. Was it your intention to only print a message when the first condition fails, but not when the second condition fails? I'm wondering if this is accidental.

In a couple places I noticed that there is unnecessary String cloning, which is not a problem at all when learning Rust and when performance is not critical. However, I'll point these out just to help expand your knowledge of Rust.

The first is that MessageContent has a blanket From impl saying it will convert from any type that has an Into<String> impl. This includes &str since it does have an Into<String> impl, which is the type of the field you're converting into a String. Therefore, you can just pass the &str values. Again, this is not a problem at all, but can be simplified as follows:

-            ChatMessage::system(&self.prompt_text.to_owned()),
-            ChatMessage::user(&self.user_text.to_owned())
+            ChatMessage::system(self.prompt_text),
+            ChatMessage::user(self.user_text),

Another thing that caught my eye is this cloning of the text param:

    fn extracted_text(&self, text: String, file_name: &str) -> Option<String> {
        let normal_text = text.clone();

This makes me wonder if you have a misunderstanding of owned values. The type String is not a reference, so the function owns it when it is passed. Therefore it only needs to be cloned if the function needs two copies of it, and it does not. So the clone can be removed.

I'll try to post more comments later.


Another unnecessary String clone is caused by passing the text param mentioned above as a String when only &str is needed. The param type can be changed to &str (in the interface as well) to avoid the clone. This plus the prior change will avoid two String clones:

-    fn extracted_text(&self, text: String, file_name: &str) -> Option<String>;
+    fn extracted_text(&self, text: &str, file_name: &str) -> Option<String>;

-                let item = self.extracted_text(text.clone(), file_name).unwrap();
+                let item = self.extracted_text(&text, file_name).unwrap();

-    fn extracted_text(&self, text: String, file_name: &str) -> Option<String> {
-        let normal_text = text.clone();
+    fn extracted_text(&self, text: &str, file_name: &str) -> Option<String> {

-        if let Some(captures) = re_start.captures(&normal_text) {
+        if let Some(captures) = re_start.captures(text) {

It is common to tell people learning Rust to clone as much as they need to when trying to fix compiler errors, since this makes learning easier. OTOH, in the cases above there is no need to clone and you could say the parameter types were incorrect. When a parameter value is only used by the function temporarily -- it is not consumed and ownership is not needed -- then a reference (borrow) should be used. In addition to being efficient, this helps in understanding the code, and it gives more flexibility to the caller of the function since the caller does not have to give up ownership or consider the cost of cloning.

4 Likes

I strongly recommend using clippy to check for problems beyond those reported by the compiler. If you run it with default settings you'll see 4 warnings that should be fixed to make the code more uniform/idiomatic and avoid confusing the person reading it.

In two places there is an unnecessary & that can be removed:

-        for entry in WalkDir::new(&self.dir)
+        for entry in WalkDir::new(self.dir)

-        .exec_chat_stream(&ai_client.model, ai_client.build_message(), None)
+       .exec_chat_stream(ai_client.model, ai_client.build_message(), None)

OptionOptions::write(true) can be removed because it is implied by the call to append(true):

-                let mut file = match OpenOptions::new().read(true).write(true).append(true).open(path) {

+                let mut file = match OpenOptions::new().read(true).append(true).open(path) {

Since Rust is an expression based language, by convention this intermediate chat_req variable would not be used (in addition to the other change here):

     fn build_message(&self) -> ChatRequest {
-        let chat_req = ChatRequest::new(vec![
-            ChatMessage::system(self.prompt_text.to_owned()),
-            ChatMessage::user(self.user_text.to_owned())
-        ]);
-
-        chat_req
+        ChatRequest::new(vec![
+            ChatMessage::system(self.prompt_text),
+            ChatMessage::user(self.user_text),
+        ])
     }
1 Like

Just a couple more comments.

Some code comments should definitely be added to explain the use of the regex, how the text returned is formatted, and how you're extracting what you need from it.

Unless I misunderstand the code below, it looks like you're reading the entire file in order to seek to the end of the file before appending to it, since the buffer is not used. But you configured append mode, so this shouldn't be necessary. And if you do need to seek to the end you can call seek with SeekFrom::End to do that.

                let mut buffer = String::new();

                file.read_to_string(&mut buffer)?;

The clone below is unnecessary and can be removed. It may be worth discussing why you added it, to see if there is a misunderstanding about how parameter passing works or what the as_bytes method does.

-                if let Err(err) = file.write_all(item.clone().as_bytes()) {
+                if let Err(err) = file.write_all(item.as_bytes()) {

There are several unwrap calls that look like they should be handled as errors instead of panicking, but I didn't point them out because you said the code is incomplete.

1 Like