Empty line code smell

[troessner]

http://www.yegor256.com/2014/11/03/empty-line-code-smell.html

Thanks @tuexss for bringing this up, I love it.

@mvz && @chastell WDYT?

[tuexss]

Should be fairly easy to implement, but quite difficult to explain. I guess it should be off by default, because otherwise running reek with this smell for the first time against a large codebase will probably explode with warnings.

[troessner]

Unfortunately it's very hard to implement :laughing: Reek operates on an abstract syntax tree that has no knowledge about whitespace since this is completely irrelevant from a parsers and interpreters point of view. So with the current Reek structure there is no "elegant" way to implement this. Might be a good task for a hackathon though ;)

[tuexss]

Tell the lexer that \n+whitespace+\n in a method without any comments and outside a heredoc should be considered as an empty_line, teach the parser that empty_line is always a null_statement, and then trigger a smell on all null_statements :)

[tuexss]

I would even go so far as to argue that this is what lexers parsers should be doing, also for ";" and empty blocks, and compiler/interpreters should turn this into NOPs.

[troessner]

Tell the lexer that \n+whitespace+\n in a method without any comments and outside a heredoc should be considered as an empty_line, teach the parser that empty_line is always a null_statement, and then trigger a smell on all null_statements :)

I'm looking forward to your incoming pull requests on the parser gem and what not :laughing: BTW. I kind of agree with you.

[tuexss]

yeah, well, that's the trouble with settled language/frameworks/communities - they don't embrace change too happily, and noone wants to be that guy/girl who fights for it to happen...

"but it has always been like this" ¯_(ツ)_/¯

[chastell]

I agree we could have it as a code smell, I agree that it should be off by default and I agree that adding it to the parser gem might not be trivial (nor neccesarily welcome there). I don’t think we should break our parser-purity and implement a workaround for this, though, unless someone really wants this in. :)

[mvz]

After reading the article I've come to the conclusion that this rule is more of a proxy for method complexity. We already limit the number of statements in a method and thus capture most of the cases where the empty line rule would be relevant.

[troessner]

I agree we could have it as a code smell, I agree that it should be off by default and I agree that adding it to the parser gem might not be trivial (nor neccesarily welcome there). I don’t think we should break our parser-purity and implement a workaround for this, though, unless someone really wants this in. :)

Yep, we could implement that easily in Reek by ..... just implementing a second infrastracture :laughing:

After reading the article I've come to the conclusion that this rule is more of a proxy for method complexity. We already limit the number of statements in a method and thus capture most of the cases where the empty line rule would be relevant.

Agreed. But I think the EmptyLineSmell is still kind of complementary to this.

[mvz]

But I think the EmptyLineSmell is still kind of complementary to this.

True, but in that case I'm not convinced of its usefulness. To just pull an example from code I'm working on now:

    def set_property_extended(property_name, value)
      type_info = get_property_type property_name
      adjusted_value = adjust_value_to_type(value, type_info)

      set_property property_name, adjusted_value
    end

I could just remove the empty line and make the smell shut up, but the fact is that there is first some argument mangling and then the 'meat' of the method. The only thing that could be extracted would be the mangling, leading to, e.g.:

    def set_property_extended(property_name, value)
      set_property property_name, adjust_value(property_name, value)
    end

I'm not sure I find this more readable, and the fact is you're just enforcing an even lower number of statements per method, or forcing people to just remove the empty lines.

Another alternative is to just inline all temporary variables, but I also find that less readable:

    def set_property_extended(property_name, value)
      set_property property_name,
        adjust_value_to_type(value, get_property_type(property_name))
    end

The method still does exacly the same things, but doesn't trigger the smell. Since the point was to make methods do only one thing, I think this smell pushes the programmer in the wrong direction, given that we already limit the number of statements.

[troessner]

Good points @mvz I guess since Ruby has poor support for functional programming there is no good way to have more or less side effect free methods. Since it's next to impossible to have methods that are defined by their return type you kind of naturally have more than one line. And then you have the need for empty lines :)

[mvz]

@troessner yes, that about sums it up. Languages like LISP also have better constructs for separating out parts of your function using let etc.

[tuexss]

@mvz regarding the example, I'd expect to find a solution where the parameter preparation for the change that will happen in the second step wouldn't be a nested one. so I'd have it more like:

 def set_property_extended(property_name, value)
      adjusted_value = adjust_value_to_property_type(value, property_name)
      set_property property_name, adjusted_value
    end

and let adjust_value_to_property_type deal with extracting the type info, as this is not in the responsibility of set_property_extended. Maybe this way the adjustment can also be pulled out of the method altogether and set_property_extended only gets property_name, adjusted_value as parameters.

I think it's even good to have both parameter preparation and change execution on their own lines, as it clearly separates preparing the change and doing the change. The real smell happens when the change needs several statements, and they get divided by empty lines. But the smell for too_many_statements more or less covers it.

I guess the empty line smell is not really fit for reek, as this is a code file smell while reek finds code AST smells. So it is a dumber but quicker heuristic, which would make sense in an AST agnostic smell detector based on the file like in rubocop or some editor-plugin. ( see http://www.rubydoc.info/github/bbatsov/rubocop/Rubocop/Cop/Style/EmptyLines (interestingly enough, they have the challenge of dealing with heredoc empty lines ;) ( https://github.com/bbatsov/rubocop/issues/815 ) ) )

[mvz]

@tuexss thanks for the explanation. I was thinking some more about this and one problem with Ruby and similar languages is that extracting a small method (like adjust_value_to_property_type) means that this immediately becomes a method on the whole class, which may not always be sensible and can lead to awkward naming. Pascal has nested functions if I remember correctly, and something like that would be very useful here. Lisp has similar structures.

[tuexss]

@mvz Yeah, I'm used to nested subroutines from Pascal, and I kinda miss them. Limiting responsibility (by limiting memory access) and avoiding side-effects is so helpful for writing clean code.

The thing is, I don't know the context of your example, but from the naming I would infer that "property" may be complex enough to justify a class of its own. I think it's possible to run without nested functions, if you take OOD to the limits. But it requires more mental effort for writing and reading.

[troessner]

Closing this one since this smell would be a job for another toot, not Reek, for the reasons given above. Interesting discussion though ;)