search

rstudio / ggvis

Interactive grammar of graphics for R
Other
713 stars 171 forks source link

Check for length rather than null #485

Closed lionel- closed 4 years ago

lionel- commented 4 years ago

Fixes #483

timcoote commented 4 years ago

Just a comment on this patch. It fixes an issue where the type of a returned value changed from either list, or NULL if there were no groups found, to always return a list, which is empty if nothing was found. This changes is good as there is only one type to worry about being returned.

In this context, I find it intellectually odd to treat the length of the list as a boolean, which, in R is 0 is true and anything else is false. Isn't !(length (x) > 0) [clearly a boolean expression] more easily understood than length(x)?

I realise that this could be a contentious point - having toiled in the wars between BCPL and Algol68C - and I'm not religious about it, but after many years watching many communities using programming languages, I've noted that loss of clarity of intent in code is an early step to loss of control of the code base.

lionel- commented 4 years ago

I think length() boolean checks are a fine idiom.

Isn't !(length (x) > 0) [clearly a boolean expression] more easily understood than length(x)?

This seems convoluted. I would use length(x) == 0. But in this case we're checking for > 0.

It doesn't seem very useful to be discussing this here though.