5 ms·

It's a shame when articles show non-idiomatic Rust code to people who might be not realise it :/ fn magic_num(&self) -> usize { return if self.name

It's a shame when articles show non-idiomatic Rust code to people who might be not realise it :/ fn magic_num(&self) -> usize { return if self.name.len() == 0 { 2 } else { 33 }; } The `return` statement is only used for early-exit, so this would be written as: fn magic_num(&self) -> usize { if self.name.len() == 0 { 2 } else { 33 } }

The_rationalist 5y ago
steveklabnik 5y ago
(Also clippy will probably suggest is_empty() over len() == 0)
- tialaramex 5y ago
  I should probably write is_empty() more often, but, it is actually doing anything that len() == 0 isn't doing, or is it just that is_empty() better explains what I presumably wanted to ask?
  - spoiler 5y ago
    I've not looked at the implementation, so I can't answer the first question, but to answer your second point/question: yes! I think getting across intent is very important for the other people reading your code. I guess here it doesn't matter as much since it's a simple enough cases, but I'm just talking in the general sense. With that said, I think is_empty probably has very slightly smaller overhead when it comes to figuring out the author's intent. Because, well, that was the intent. Length checking against 0 is merely a method of that intent. Also, this "intent decoding" while reading code (no pun intended) adds up in mental cost, IMO. So, it wears down the the reader/reviewer attention more quickly.
  - steveklabnik 5y ago
    From clippy’s docs: > Some structures can answer .is_empty() much faster than calculating their length. So it is good to get into the habit of using .is_empty(), and having it is cheap. Besides, it makes the intent clearer than a manual comparison in some contexts. It also has a lint for implementing both methods if you’ve only defined one, to help perpetuate the convention.
brabel 5y ago
Why are Rust developers so pedantic with this stuff. What the author wrote is perfectly valid, fine code. Please stop insisting everyone does something one way or another when there's no clear advantage here to doing so except for the lack of single bloody keyword.
- vlmutolo 5y ago
  It's "perfectly valid, fine" with the exception that it's different from how the vast majority of Rust programmers write code. That in itself is a problem. Rust has two widely-used tools that enforce style: `rustfmt` and `clippy`. The former enforces formatting style and the latter general programming style, such as whether or not to use a `return` statement (`clippy` also does non-style lints). For many of these formatting/stylistic lints, there are valid arguments to be made for an alternative choice. I personally often disagree with one or both tools. I still follow them. The huge benefit here is that most Rust codebases look identical. I can open up burntsushi's regex crate and see code that is formatted and styled exactly like mine. It makes reading new code a lot easier.
SquibblesRedux 5y ago
I prefer to use explicit return statements everywhere -- for greppability.
- berkut 5y ago
  Yep, and read-ability: it's more obvious at a glance where early-exits are.
- ReleaseCandidat 5y ago
  If you have to search for your returns you have either too many or your function is too long ;) I'd even say any return is actually one too many.
  - setr 5y ago
    Half the functions I write follow the format If !validation_rule1 { return default/err } If !validation_rule2 { return default/err } Work… If !validation_rule3 { return default/err } Work… return result/success Don’t know how you’d get away with single return except by horrifically nesting conditionals
    - dragonwriter 5y ago
      > Don’t know how you’d get away with single return except by horrifically nesting conditionals That pattern is addressed by chained not nested conditionals, which can then be wrapped in a match, i.e.: if !validation1 { err1 } else if !validation1 { err2 } . . . else { ...work...; success_result } the error checking part can be reduced to an iteration with appropriate definitions of a structure for the data to be validated, etc., like: match validators.find_map(|v| v(data)) { Some(err) => err, None => { ...work...; success_result } } EDIT: I missed the intermediate work steps in your post when I initially wrote the response. That can be done a number of ways, e.g., breaking things up into to chained steps (each of which might use the above pattern) that mix validation and work and return a Result type with either the data for the next step to consume or an error.
    - 5y ago
eliben 5y ago
Thanks for the comment! I actually forgot to run clippy on these samples because they're so small; I'll fix up the code in the post to be more idiomatic.

The_rationalist 5y ago

steveklabnik 5y ago

(Also clippy will probably suggest is_empty() over len() == 0)

tialaramex 5y ago
I should probably write is_empty() more often, but, it is actually doing anything that len() == 0 isn't doing, or is it just that is_empty() better explains what I presumably wanted to ask?
- spoiler 5y ago
  I've not looked at the implementation, so I can't answer the first question, but to answer your second point/question: yes! I think getting across intent is very important for the other people reading your code. I guess here it doesn't matter as much since it's a simple enough cases, but I'm just talking in the general sense. With that said, I think is_empty probably has very slightly smaller overhead when it comes to figuring out the author's intent. Because, well, that was the intent. Length checking against 0 is merely a method of that intent. Also, this "intent decoding" while reading code (no pun intended) adds up in mental cost, IMO. So, it wears down the the reader/reviewer attention more quickly.
- steveklabnik 5y ago
  From clippy’s docs: > Some structures can answer .is_empty() much faster than calculating their length. So it is good to get into the habit of using .is_empty(), and having it is cheap. Besides, it makes the intent clearer than a manual comparison in some contexts. It also has a lint for implementing both methods if you’ve only defined one, to help perpetuate the convention.

brabel 5y ago

Why are Rust developers so pedantic with this stuff. What the author wrote is perfectly valid, fine code. Please stop insisting everyone does something one way or another when there's no clear advantage here to doing so except for the lack of single bloody keyword.

vlmutolo 5y ago
It's "perfectly valid, fine" with the exception that it's different from how the vast majority of Rust programmers write code. That in itself is a problem. Rust has two widely-used tools that enforce style: `rustfmt` and `clippy`. The former enforces formatting style and the latter general programming style, such as whether or not to use a `return` statement (`clippy` also does non-style lints). For many of these formatting/stylistic lints, there are valid arguments to be made for an alternative choice. I personally often disagree with one or both tools. I still follow them. The huge benefit here is that most Rust codebases look identical. I can open up burntsushi's regex crate and see code that is formatted and styled exactly like mine. It makes reading new code a lot easier.

SquibblesRedux 5y ago

I prefer to use explicit return statements everywhere -- for greppability.

berkut 5y ago
Yep, and read-ability: it's more obvious at a glance where early-exits are.
ReleaseCandidat 5y ago
If you have to search for your returns you have either too many or your function is too long ;) I'd even say any return is actually one too many.
- setr 5y ago
  Half the functions I write follow the format If !validation_rule1 { return default/err } If !validation_rule2 { return default/err } Work… If !validation_rule3 { return default/err } Work… return result/success Don’t know how you’d get away with single return except by horrifically nesting conditionals
  - dragonwriter 5y ago
    > Don’t know how you’d get away with single return except by horrifically nesting conditionals That pattern is addressed by chained not nested conditionals, which can then be wrapped in a match, i.e.: if !validation1 { err1 } else if !validation1 { err2 } . . . else { ...work...; success_result } the error checking part can be reduced to an iteration with appropriate definitions of a structure for the data to be validated, etc., like: match validators.find_map(|v| v(data)) { Some(err) => err, None => { ...work...; success_result } } EDIT: I missed the intermediate work steps in your post when I initially wrote the response. That can be done a number of ways, e.g., breaking things up into to chained steps (each of which might use the above pattern) that mix validation and work and return a Result type with either the data for the next step to consume or an error.
  - 5y ago

eliben 5y ago

Thanks for the comment! I actually forgot to run clippy on these samples because they're so small; I'll fix up the code in the post to be more idiomatic.