metron-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Michael Miklavcic <michael.miklav...@gmail.com>
Subject [DISCUSS] Refactoring
Date Wed, 30 May 2018 00:05:44 GMT
I want to bring up the subject of code refactoring and how we should manage
this in PR's as our product evolves. As Metron matures, it's only natural
that we'll have and increasing number of contributors, and subsequently
contributions affecting many hardened parts of the code base. We've
generally not been particular about mixing refactoring changes with other
types of improvements or bug fixes. As a general best practice for software
engineering it is indeed desirable to undergo regular refactoring as a
matter of "scouts' rules" or "fixing broken windows." This helps keep code
readable and has the benefit of a fresh pair of eyes to see code in a new
way that allows the newcomer to introduce clarifying changes that the
original author(s) may not have considered.

While refactoring is generally applauded (because we have unit,
integration, and acceptance tests backing our changes), it does pose some
challenges during the review process. Depending on the type of PR, the
refactoring work can at times be many orders of magnitude larger than the
code pertinent to the desired change in functionality, whether bug fix or
feature enhancement, itself. While tests should protect against unintended
side effects (and sometimes they are also refactored) it does introduce the
possibility of new subtle bugs. It also makes a lot of PR's a conflated mix
of comments pertinent to the improvement/fix and opinions about best
practices around coding style.

I propose a simple change - we update our coding style guidelines in
section 2.1 to expand on refactoring. We currently cover whitespace and
comments:

"Don’t combine code changes with lots of edits of whitespace or comments;
it makes code review too difficult. It’s okay to fix an occasional comment
or indenting, but if wholesale comment or whitespace changes are needed,
make them a separate PR."

I propose we expand this to say:

"Don’t combine code changes with lots of edits of whitespace, comments, or
code changes specifically for refactoring purposes; it makes code review
too difficult. It’s okay to fix an occasional comment or indenting, but if
wholesale comment, whitespace or other refactoring changes are needed, make
them a separate PR."


I believe this provides additional clarity. I think it's one thing to
extract a method or introduce changes for code you're specifically
modifying, and another thing to introduce changes that affect surrounding
code. I would also propose we emphasize the Google checkstyle and
auto-formatting tooling when submitting any changes, but dealing with
enforcement is not my focus for this discuss thread.

https://cwiki.apache.org/confluence/display/METRON/Development+Guidelines

Best,
Michael Miklavcic

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message