Code readability and Clippy

code review
1

One my student from Poland taught me to use the style of a negation, however Clippy doesn't like it:

66 |     if session.is_empty() == false {
   |        ^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `!session.is_empty()`

The reason is Unnecessary code.. However ! is hardly visible and a source of mistakes. Do you agree that Clippy suggestion reduces a readability of the code?

2 Likes
2

You can just allow specific patterns based on your personal preference project wide (most people do). I personally like clippy's style better for this specific case, but both should get optimized into same assembly by the compiler. Use #![allow(clippy::bool_comparison)] in lib.rs/main.rs

4 Likes
3

unfortunately

yeah, I can see this is a valid concern. maybe that's one of the reasons why C/C++ has #include <iso646.h>. also, lisp people have when and unless macros, even they already have the not function.

however, I don't think Clippy is wrong either. equiality comparison between boolean values are indeed "unnecessary" code, in most cases.

to be honest, I don't know what's the best solution.

how do you like a not function?

if not(session.is_empty()) {
    //...
}

what's funny is, this has the same count of characters in C with #include<iso646.h>, just the parenthesis is at different place:

if (not session.is_empty()) {
    //...
}

another similar idea is to use extension trait methods on bool type:

if session.is_empty().is_false() {
    //...
}

but I think this is really about personal habbits and preferences.

personally, the ! operator has never been a problem to me, since I use relative large font settings, and with mono spaced typefaces, the ! isn't that easy to overlook.

8 Likes
4 
use std::ops::Not;
// ...

    if session.is_empty().not() {
13 Likes
5

It was a really hot topic with ~700 replies 5 years ago. However, nobody suggested using == true or == false. Indeed, Rust has variants as is_some() and is_none(), but there is no variant is_not_empty(). Generally , I agree to have is_some() and is_empty(), however it will confuse a developer. Probably is_filled() is the best opposite to empty for me.

Since I develop on several different computers including tablets, a visibility of ! is different. You can also develop a code read and modified by others. So, it makes sense to provide the clearest for every one notation.

6

lol, you got this.

7

It occasionally comes up in this scenario:

// I prefer  a space after `!` but `rustfmt` removes it
if !matches!(...) { ... }
1 Like
8

I'd raise the question as to why session is designed in such a way that it has a method is_empty() in the first place. I'd rather expect something like if let Some(session) = self.session() { ... } or using the typestate pattern to ensure, that a session is always valid and never "empty".

5 Likes
9

Great to know this solution. As OP, I also wondered several times how to improve readability.

(Rushes to check all places with if !... in his crates...)

10

When you get data from web, you have no certainty. Very often data even got crafted by curious people.

11

That's actually a pretty good area to use the pattern @Schard suggested. Session should be Option.

12

It is an interesting problem. Unfortunately it's going beyond of this forum purpose. But I give you just short explanation:

https:://mycoolsite.rs?session=valid_session

https:://mycoolsite.rs?session=invalid_session

https:://mycoolsite.rs?session=&and-empty-session

https:://mycoolsite.rs?no_session=no_session_atall

But Option has only two states. So I prefer to use enum with 4 options instead.

3 Likes
13

I think that's a fair use case in my opinion. I am not really that experienced in web development. But that looks like a 'validation' step, where you match various states with an enum through parsing various URL's. The point of Option here would be to ensure that you don't forget to do a runtime check like you did in your original post, in the Option approach the compiler will force you to ensure that you are handling the valid vs. invalid session cases. That would probably be the more idiomatic pattern. But your approach wouldn't be wrong either if it works for your use case.

14

I still don't quite get it.
I mean no session, valid and invalid session infer by name what's going on but "empty session" has me somewhat stumped.

While agree some sort of state machine would probably a better/safer pattern what keeps you from implementing better readability yourself with adding methods like:

.exists() 
.does_not_exist() 
.is_valid()
.is_not_valid() 
.has_data()
.no_data()
15

if not session.is_empty() {
//...
}

I like this style.

16

It is why I love web development in Rust. So many possibilities and space for your creativity.

17

i have to say, ive never understood when people make style comments like this. maybe this is just my habit of writing APL, but ive never found this to be a source of mistakes for me, or heard of it being one. is this actually a mistake that you've made while reading code?

18

It doesn't always happen this way, but sometimes I'll start with if !list.is_empty() { and then realize later it's phrased better as what I actually do after that point:

  • using the first element: replace with let Some(first) = list.first() else {..};
  • operating on the list: sometimes it's obvious from some other state whether any elements were processed in the loop
  • mapping the list to another: maybe my function doesn't care about "empty" being a special case, and can happily collect into an empty collection with no extra checks

These don't cover a majority of cases, but just thinking to pull the "is the collection empty?" question down as late as possible since it can lead to easier to understand logic.
"Aah, this does nothing if it's empty, because it needs the first element" etc.

19

I have made mistakes due to not seeing a !. Not many times, maybe 2-3 times in all the years I've been programming. Hardly a common thing, but it definitely has happened.

To be fair, I've always been in the bad habit of having a too small font. I might even go so far as to admit that I've always had a font that is slightly too small for my eyes.

But yeah, after one of the times it caused me to miss the inverted logic, I started to write out if(something == false) in C (well, FALSE there) and C++. It was only when I started using clippy with Rust that I went back to using !.

20

It's more unusual situation, perhaps you will like to hear about it. I have C/Java background, so I tried Rust only recently. Since I do usually a low level programming, I just compiled rustc and started Rust development. I never used Clippy, although friends of mine told - I should. Based on my background I always used condition like if (s.length > 0).. and everything worked perfectly. In this Christmas time I purchased Raspberry Pi 4 and was able to use it and a tablet as development tools after. Since I didn't build rustc for Raspberry Pi and used rustup, I got also Clippy and tried it. This guy immediately told me that I had to replace all my if s.len() > 0 .. by if !s.is_empty() ... And guess what after I did that? Right, my code stopped be readable for me.

1 Like