You've got your typical company started by ex-software salesmen,
where everything is Sales Sales Sales and we all exist to drive more sales.
On the other extreme you have typical software companies built by ex-programmers.
These companies are harder to find because in most circumstances they keep quietly
to themselves, polishing code in a garret somewhere, which nobody ever finds,
and so they fade quietly into oblivion right after the Great Ruby Rewrite,
their earth-changing refactoring-code code somehow unappreciated by The People.
-- The Developer Abstraction Layer by Joel Spolsky
Though my natural inclination is to be a bit
OCD
about keeping code clean,
I concede that
spending too much time and money on refactoring, writing programmer tools, and
endlessly polishing code will likely lead to commercial failure.
As will the converse, namely neglecting your developers and their code
and architectures in favour of sales and marketing.
Successful software companies tend to have a healthy balance.
Refactoring
Booking.com, perhaps the most commercially successful Perl-based company,
has caused a bit of controversy over the years with their attitude towards refactoring.
To give you a flavour, I present a couple of comments below:
Booking is destroying my career because I am not allowed to do anything new.
I am not allowed to use new technologies.
I'm not allowed to "design" anything big.
I am not allowed to write tests.
I am allowed to copy that 500 line subroutine into another module.
If people have done that several times before, maybe it should be refactored instead of duplicated?
If you do that, you get in trouble.
As one boss says, "we do not pay you to write nice code. We pay you to get job done."
Management, and the term is quite lose when applied to Booking.com, sees no gain in refactoring code.
By refactoring I'm talking about taking a few weeks to rewrite an existing piece of software.
By definition refactoring doesn't bring new functionality so this is why management is reluctant to go down that road.
We're quite lenient about code that gets added to the repo, as long as there's a business reason behind it.
If a quick hack can be deployed live and increase conversion then it will be accepted.
But rest assured that crappy code doesn't last long, specially if other devs have to use it or maintain it.
-- from Truth about Booking.com (Blog)
One of the posts specifically deals with the culture of "get it done and fast"
and how they do not encourage refactoring or basic testing.
I actually work in a Perl shop where management has the same kind of mentality,
and it is slowly killing our efficiency.
Regarding testing, it's true that we're not very unit testing focused.
This is mainly because we've decided to spend most of the time/money/infrastructure
that you might usually spend on unit testing on monitoring instead.
If you have unit tests you still need monitoring, but in practice if your monitoring
is good enough and you have an infrastructure to quickly rollout & rollback systems
you can replace much of unit testing with monitoring.
We're not adverse to refactoring when appropriate.
But if you're going to propose rewriting some code here you'll
actually have to make a compelling case for it which isn't just "the old code is hairy".
Do you actually understand what it does? Maybe it's hairy and complex because it's solving
a hairy and complex problem. Are you not aware of where this system fits into the big picture?
We've also had code that's looks fantastic, had tests, used lots of best practices
that we've had to throw away completely because it was implementing some idea that
turned out to be plain stupid.
-- from What exactly is up with Booking.com? (reddit)
Opportunistic Refactoring and The Boy Scout Rule
Some people object to such refactoring as taking time away from working on a valuable feature.
But the whole point of refactoring is that it makes the code base easier to work with,
thus allowing the team to add value more quickly.
If you don't spend time on taking your opportunities to refactor,
then the code base gradually degrades and you're faced with slower
progress and difficult conversations with sponsors about refactoring iterations.
There is a genuine danger of going down a rabbit hole here,
as you fix one thing you spot another, and another,
and before long you're deep in yak hair.
Skillful opportunistic refactoring requires good judgement,
where you decide when to call it a day.
You want to leave the code better than you found it,
but it can also wait for another visit to make it
the way you'd really like to see it.
If you always make things a little better,
then repeated applications will make a big
impact that's focused on the areas that are
frequently visited - which are exactly the
areas where clean code is most valuable.
-- Opportunistic Refactoring (Martin Fowler)
The Boy Scouts have a rule: "Always leave the campground cleaner than you found it"
What if we followed a similar rule in our code: "Always check a module in cleaner than when you checked it out"
-- The Boy Scout Rule (O'Reilly)
At work,
we perform opportunistic refactoring following the
Boy Scout rule, trusting the judgement of developers.
How do you do it at your workplace?
Code Reviews
By way of background, my company went agile about five years ago,
at first
with great zealotry,
nowadays with more maturity and less dogma.
Before check-in, all code must be reviewed, either continuously via
pair programming, or via a lightweight code review (typically over-the-shoulder).
We also have a coding standard, though it is not strongly enforced.
To give a concrete example, during a code review the other day,
I persuaded the author to eliminate unnecessary repetition
by changing this snippet:
my $config = <<'GROK';
ADD UDP_LISTENER ( 515 )
ADD UDP_LISTENER ( 616, 657 )
ADD UDP_LISTENER ( 987 )
GROK
my @test_cases = (
{ desc => "# Test 1",
conf => $config,
find => [ 'port = 515', 'port = 616', 'port = 657', 'port = 987'
+ ],
},
);
to:
my $liststr = 'ADD UDP_LISTENER';
my @ports = ( 515, 616, 657, 987 );
my $config = <<"GROK";
$liststr ( $ports[0] )
$liststr ( $ports[1], $ports[2] )
$liststr ( $ports[3] )
GROK
my @test_cases = (
{ desc => "# Test 1",
conf => $config,
find => [ map { "port = $_" } @ports ],
},
);
What would you have done?
I'm sure some other programmers at my company wouldn't have
bothered suggesting any changes at all: after all, the code worked as is,
it's pretty clear, plus "it's only a test script", so why bother?
Though I felt the code was more maintainable with duplication eliminated,
I had another motivation in this specific case: training.
You see, the programmer in question was very new to Perl and,
as I found out during the review, had never used map before!
Training (and improved teamwork) are important benefits of code reviews.
Eliminating unnecessary duplication and repetition is
a common discussion topic during code review in my experience.
(Note: I did not include this example to argue further about
what DRY means exactly in Room 12A :).
Other common discussion points during code review are:
- Commenting.
- Naming.
- Clarity vs Cleverness.
- Encapsulation.
- Interfaces.
- Error handling.
- Testability. Is the code testable in isolation?
- Supportability.
- Portability.
- Security.
- Performance.
Note that we do not normally discuss code layout because all code is pushed
through
Perl::Tidy before review.
I'm interested to learn about your workplace experiences. In particular:
- Do you have a coding standard? How is it enforced?
- Do you do pair programming?
- Do you do code reviews? Are they heavyweight (e.g. Fagan Inspection) or lightweight (e.g. over-the-shoulder)? Mandatory or optional?
- What are common discussion points during your code reviews?
Cleverness
To finish, here's another one, derived from Clever vs. Readable.
Would this statement pass your code review?
my $value = [ $x => $y ] -> [ $y <= $x ];
If not, would you suggest changing it to:
my $value = $x < $y ? $x : $y;
or:
use List::Util qw(min);
my $value = min( $x, $y );
Or something else?
References
Updated Aug 2015: Added Ternary vs. Sort vs. Max reference.
-
Are you posting in the right place? Check out Where do I post X? to know for sure.
-
Posts may use any of the Perl Monks Approved HTML tags. Currently these include the following:
<code> <a> <b> <big>
<blockquote> <br /> <dd>
<dl> <dt> <em> <font>
<h1> <h2> <h3> <h4>
<h5> <h6> <hr /> <i>
<li> <nbsp> <ol> <p>
<small> <strike> <strong>
<sub> <sup> <table>
<td> <th> <tr> <tt>
<u> <ul>
-
Snippets of code should be wrapped in
<code> tags not
<pre> tags. In fact, <pre>
tags should generally be avoided. If they must
be used, extreme care should be
taken to ensure that their contents do not
have long lines (<70 chars), in order to prevent
horizontal scrolling (and possible janitor
intervention).
-
Want more info? How to link
or How to display code and escape characters
are good places to start.
