Need someone to review the code

help
1

Hello All,

I trying my hand in Rust after coding for full time in Python for 6 years (2.5 years in C# and 6 years in Perl in that order). I tried reading books but i was missing the real problems and thats when i decided to convert simple shell script to Rust (its gives no benefit to convert but it was only for learning). I spent about 1 hr / day for 4 days (not consecutive) to covert the code to Rust, it works and does what i wanted to do but i have no idea how good the code is or how can it be improved?

If anyone of you have some time to spare and could you please review my PR https://github.com/ninadmhatre/rust_practice/pull/1

There are few queries:

  1. I am running OS command using std::process::Command how can i print / log the whole command being executed?
  2. Stdout / Stderr is buffered above, so even with Stdio i am not able to see output / errors logged in file on real time? i got this from 1 recipe i found online.
  3. I have few variables in config.rs but how do i orverride them with env variable? I tried env module but then i have to make sure its defined and then only use it (i can achieve this with other function but just asking if it can be done in 1 line w/o panic)
  4. I am doing lot of "str".to_string or "String".as_str is correct? or can be avoided?

I am yet to finish the rust by example but i was getting tried of not doing anything with it so yes this is done with half knowledge of all of the Rust. But i am happy that i can produce something that works now need to learn more.

I am using Rust 1.32 2018 edition on linux

Thanks,
Ninad

2

I made very briefly review, hope it is helpfull. Since some point I stopped commenting same things. According to questions:

  1. I can't help here, never needed to run external processes in rust.
  2. As long, as you are creating stream application it should not be problem. Buffering may be controlled by terminal, so if you need real-time vision for tests purposes, just check how to disable buffering in your terminal. If for some reason want to enforce things to be flushed you may:
    a) use some terminal library (like curses), but it will make things harder, and break your app in many ways (eg. streaming will be impossible)
    b) use std::io::stdout::flush() but it may impact performance (forces flushing)
    c) use stderr (eprinln! instead of println!) for info/debug/error messages - stderr tends to not be buffered
  3. I am doing this by using dot_env, instead of creating variables in places like config.rs. If you still want to deliver hard-coded default, just take Option from environtment, and then unwrap_or it with your default.
  4. I don't like String::as_str() - just use &my_string - clearer, shorter, with same semantics. According to str::to_string() - personally I prefer .into(), or .to_owned() when context is not clear, I thing the second one is most widely used. However important thing is, to use exactly the type what you want and choose it as early as its possible - often you are using String in places, when &str is way to go, and this makes your code less reuseable.
2 Likes
3

You should use cargo clippy to check your code. It will give you a lot of useful hints that cover most of the basic cases.

3 Likes
4

There is also https://codereview.stackexchange.com/ where you can post code for others to review as well.

2 Likes
5

Thanks a lot for taking a time to review my code and tips. I will modify my code based on your & some other suggestions i received.

7

@johnthagen I was not aware of this site, thanks a lot!

@Riateche It took me some time to get this clippy installed, i was on stable toolchain and then i had to install nightly tool chain and then only i could get it installed. Thanks a lot for suggestion

8

Since clippy is now stable, you can just run rustup component add clippy and then use cargo clippy.