Where to go for code review?

help
1

Hi there,

I'm just curious when you've written your first rust program, where do people typically go for general feedback about correctness? I don't personally know anyone that has written any rust code.

Thanks!

2

You can get first automatic opinion from cargo clippy. It catches a lot of noob mistakes, such as use of &Vec.

2 Likes
3

I'm in the same boat. We are a two man company but my partner is a Python head and I'm not going to start pushing Rust on him. I know precious few people around here who actually code, never mind in Rust.

But all is not lost. Because:

  1. You can't actually create incorrect code, as far as memory use goes, unless you go out your way. So that is a whole raft of things not to have to worry about.

  2. The compiler produces excellent error messages and offers good suggestions as to what to do about them. Someone described the Rust compiler as being like pair programming, where your partener in the pair is extremely fussy and never sleeps!

  3. The clippy tool is fantastic. And very often has great suggestions.

  4. At the end of all that one checks for operational correctness by the usual means, run it and test it. Rust has ones back covered there with the built in test facilities.

In short, when programming in Rust it's like having a huge team of the best software developers in the world watching your every move.

Of course one might not be writing the best "idiomatic" Rust and one might not have discovered the best ways to do things in Rust. But so what? Your program is memory safe and is known to work. What more "correctness" does one need?

Over time one will of course learn more about Rust, likely read more of other peoples Rust code and your code will become more Rusty.

Finally, and best of all perhaps, I have found that posting a question here, perhaps with a little code snippet, will stimulate a lot of useful feed back.

10 Likes
4

There's a specific code review category on this forum, for this purpose.

5
  1. use clippy with the pedantic feature turned on :joy:
  2. ask here in this forum
6

Thanks for all the helpful replies. I didn't know about clippy. If anyone is interested, here's the output:

$ cargo clippy
    Checking cfg-if v0.1.10
    Checking uuid v0.8.1
    Checking vec_map v0.8.2
    Checking fnv v1.0.7
    Checking libc v0.2.76
    Checking core-foundation-sys v0.6.2
    Checking log v0.4.11
    Checking mach v0.2.3
    Checking core-foundation v0.6.4
    Checking io-kit-sys v0.1.0
    Checking gilrs-core v0.2.6
    Checking gilrs v0.7.4
    Checking xbox v0.1.0 (/Users/leoj/src/wip/rustlang/xbox)
warning: this `else { if .. }` block can be collapsed
  --> src/main.rs:68:22
   |
68 |                 } else {
   |  ______________________^
69 | |                   if y >= min {
70 | |                       stick = Some(north);
71 | |                   } else if y <= -min {
72 | |                       stick = Some(south);
73 | |                   }
74 | |               }
   | |_______________^
   |
   = note: `#[warn(clippy::collapsible_if)]` on by default
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#collapsible_if
help: try
   |
68 |               } else if y >= min {
69 |                   stick = Some(north);
70 |               } else if y <= -min {
71 |                   stick = Some(south);
72 |               }
   |

warning: this `if` statement can be collapsed
   --> src/main.rs:100:9
    |
100 | /         if gamepad.value(Axis::RightStickX) < min && gamepad.value(Axis::RightStickX) > -min
101 | |             && gamepad.value(Axis::RightStickY) < min && gamepad.value(Axis::RightStickY) > -min
102 | |             && gamepad.value(Axis::LeftStickX) < min && gamepad.value(Axis::LeftStickX) > -min
103 | |             && gamepad.value(Axis::LeftStickY) < min && gamepad.value(Axis::LeftStickY) > -min {
...   |
111 | |                 }
112 | |         }
    | |_________^
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#collapsible_if
help: try
    |
100 |         if gamepad.value(Axis::RightStickX) < min && gamepad.value(Axis::RightStickX) > -min
101 |             && gamepad.value(Axis::RightStickY) < min && gamepad.value(Axis::RightStickY) > -min
102 |             && gamepad.value(Axis::LeftStickX) < min && gamepad.value(Axis::LeftStickX) > -min
103 |             && gamepad.value(Axis::LeftStickY) < min && gamepad.value(Axis::LeftStickY) > -min && self.vec.len() > 0 {
104 |             match self.map.get(&self.vec) {
105 |                 Some(a) => println!("{:?}", *a),
  ...

warning: this function has too many arguments (8/7)
  --> src/main.rs:56:5
   |
56 |     fn check_bounds_and_push(&mut self, gamepad: Gamepad, x: Axis, y: Axis, north: Stick, east: Stick, south: Stick, west: Stick) {
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
   = note: `#[warn(clippy::too_many_arguments)]` on by default
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#too_many_arguments

warning: you seem to be trying to use match for destructuring a single pattern. Consider using `if let`
  --> src/main.rs:60:9
   |
60 | /         match (gamepad.value(x), gamepad.value(y)) {
61 | |           (x, y) => {
62 | |               if x.abs() > y.abs() {
63 | |                   if x >= min {
...  |
76 | |           _ => {}
77 | |         }
   | |_________^
   |
   = note: `#[warn(clippy::single_match)]` on by default
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#single_match
help: try this
   |
60 |         if let (x, y) = (gamepad.value(x), gamepad.value(y)) {
61 |             if x.abs() > y.abs() {
62 |                 if x >= min {
63 |                     stick = Some(east);
64 |                 } else if x <= -min {
65 |                     stick = Some(west);
 ...

warning: length comparison to zero
   --> src/main.rs:104:20
    |
104 |                 if self.vec.len() > 0 {
    |                    ^^^^^^^^^^^^^^^^^^ help: using `!is_empty` is clearer and more explicit: `!self.vec.is_empty()`
    |
    = note: `#[warn(clippy::len_zero)]` on by default
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#len_zero

warning: you seem to be trying to use match for destructuring a single pattern. Consider using `if let`
   --> src/main.rs:105:21
    |
105 | /                     match self.map.get(&self.vec) {
106 | |                         Some(a) => println!("{:?}", *a),
107 | |                         _ => (),
108 | |                     }
    | |_____________________^ help: try this: `if let Some(a) = self.map.get(&self.vec) { println!("{:?}", *a) }`
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#single_match

warning: 6 warnings emitted

    Finished dev [unoptimized + debuginfo] target(s) in 7.44s

I also tried running cargo clippy -- -A clippy::pedantic which seemed to produce the same warnings.

This is helpful; and it's interesting to see how I was "trying to use match for destructuring a single pattern" everywhere because that's all I knew; I didn't know of the more terse if let pattern yet.

As far as code review; I just question if I'm interfacing with the Gilrs library in the best way possible and if how I'm using std::collections::HashMap and enum as keys is a good way to map things.

The last time I wrote code, it was in JavaScript (of course!) and it's been a long time since I messed with a more structured language. So, this program seems to function just fine, but when it comes to the structure, I'm worried I might be doing something dumb that will need to be restructured.

Here is the code for this program if anyone cares to look (if you do please give your opinion of what you would do the same or differently):

https://github.com/leoj3n/xkbm/blob/34403f4487ce6390b8385e7f1d65aca769788ebb/src/main.rs

The eventual goal would be to emulate normal keyboard keystrokes and mouse input, so you can type on your computer using only the gamepad joysticks, so for instance my Xbox controller can act as a sort of multifunction keyer.

This is just a fun idea I had for my first Rust lang program, but it could allow you to code an entire game using nothing but an Xbox controller, maybe the first game ever programmed using just a gamepad, lol.

For now, I'm going to fix these clippy warnings. Thanks for the suggestions!

7

I wanted to make sure such a generic request would be acceptable on this forum. Thanks for pointing that out! I did not notice the category at first (but even so some forums get mad when you aren't asking a specific enough question; such as a general code review). Glad to know I can come here for help.

1 Like
8

I've found this community to be highly friendly and willing to answer the dumbest questions I could come up with. I always heard that rust has a friendly community before but I actually saw it after coming here.

btw what ide/code editor are you using? on vscode + rust_analyzer I can set OnSave to clippy so just hitting Ctrl + S will use cargo clippy instead of cargo check and all of these suggestions will show up as warnings.

1 Like
9

Without thinking about it really, I just used vim, since I was already on the command line after installing. Thanks to cargo working like npm, I was able to get started with the Gilrs library right away. I don't mind running the clippy command for now, but I will fire up vscode in the future and check out that extension.

I'm sure there are a lot of other useful extensions but the compiler was pretty much all I needed to get a working program; actually, as long as I got close enough to what the code should look like, the compiler seemed to span that last 20% and teach me how the syntax should look, so I was learning from the compiler as I went. I have always failed to be interested in any language where I feel the need to be tied to an IDE like IntelliJ or Eclipse or Xcode or to install a giant runtime, etc. I guess I'm just impatient and any such barrier to entry was always enough to drive me back to JavaScript and PHP and shell scripts.

I was pleasantly surprised by what I was able to accomplish within hours of installing and running cargo init. The experience did replicate the kind of joy I'm sure many of us experienced the first time using npm and writing Node. However, there was never a book written on JavaScript, and that community can be very fragmented. In that respect, this "users" forum could be a boon in comparison.

Still a lot to try; I'm going to have to read more about lifetimes (the syntax looks a bit intimidating). I also want to see what it's like writing website code in Rust; I take it there isn't the same isomorphism like with JavaScript on the client and server? Or can you write a whole frontend in Rust and HTML if it compiles to WASM and uses a VDOM? Need to research that yet; not sure if it's going to be as nice as JavaScript in respect to building websites.

2 Likes
10

Sure! Stating your specific concerns would be helpful, if you have any, but asking for a general review is definitely fine too.

So far, this thread has gotten a bit long, so to ease navigation, I recommend posting a new thread after you fix the clippy warnings. Also, in case you don't know, you can run cargo fmt on your file to automatically format it according to the Rust standard formatting standards after you install rustfmt (in fact, I have it as a save hook).

11

Sure, I'll ask for a sanity check of the code in a new thread. Thanks for the tip about cargo fmt.

12

Sounds like you are doing very well to me.

My take on this is that there is nothing dumb. Your program compiles, therefore it has no memory use surprises in store. If it does what you want then it is not dumb, it is a correct working program. It's nice to put some tests in place using cargo's built in test facilities to show that it works correctly.

Of course as you extend your program with new features you may find out that would be easier, or even made possible at all, if the original code was structured differently. In that respect you might look back and think "that was dumb". But that rather depends where you want to go with it in the future.

I have never been a great IDE user. Mostly they seemed to be big, slow and complex, cough eclipse, and did not offer compelling features for me. Vim with syntax highlighting is most of what I wanted and still my tool of habit for most things.

However with VS Code and the rust-analyzer extension is wonderful.

Still, if you want to stick with vim you can do all of that IDE stuff in there as well. Watch how the amazing Jon Gjengset does it: "Desktop and editor setup for Rust development" - https://www.youtube.com/watch?v=ycMiMDHopNc&t=6498s

Indeed the lifetime syntax is intimidating and I don't like to see such syntactic "line noise" cluttering up my code. Luckily one can go a long way with out ever using any lifetime tick marks. I'm pretty sure I don't have any in all the code I have written in my first year of Rust. And that includes a Rocket API server, a web socket server, various data massaging/filtering/routing services., all working with a postgres database and a NATS messaging system.

Lifetimes are not about those tick marks though. Lifetimes are about where data is created, and where it is destroyed. That is to say "who owns the data and for how long". That in turn is determined by your program structure, not those tick marks.

I imagine coming from Java script that might be tricky to grasp, given that in JS an objects lifetime extends automatically as long as there is anything around that references the object. A key point here is that the equivalent to a JS style reference is not a Rust reference, it is a smart pointer, Rc Rc in std::rc - Rust or Arc if you are using threads Arc in std::sync - Rust

My experience of that is only building a web server with the Rocket crate. So far all my front end stuff is still JS. I keep an eye on the Yew framework: GitHub - yewstack/yew: Rust / Wasm framework for creating reliable and efficient web applications but I don't see how I can use it so easily to do what I want in the client yet.

2 Likes
13

One of the things about Rust that I've only come to really appreciate after I spent more time on it is how easy it generally speaking is to restructure your code. I've done fairly extensive refactoring and most of the time it's enough to make the change you want and fix the resulting compiler errors. It's a breeze compared to dynamically typed languages, so while it's a good thing to keep in mind, I wouldn't worry too much about it.

2 Likes
14

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.