5 reasons you are not doing code reviews

Whether your work in a startup of at a multinational, getting your team to start doing code review probably got stuck with the typical answer:

That’s a very good idea, but…

Having been there several time, and being quite convinced on the added value of code review, I listed the most common pushbacks I’ve encountered, and some ways to go past them.


…I don’t have time

Of course you don’t. This feature that need to be finished ASAP, and there is a module you want to refactor since months, and that is just the most important stuff…

This is probably the most pervasive answer I get and the one that seems to be the most reasonable. Yet it is not. Opposing deadlines and code quality is a false dichotomy: we insist on code quality because we want to meet deadlines.

In other words: you should always apply and justify best practices to win time (in the form of faster releases or better ones). The convincing (and correct) argument here should always be economical.

As very well put by GeePawHill: We’re in this for the money or as per this Martin’s Fowler slide (talk presented at OOP 2014):

code-review-fowler


 You are finishing the feature, and ready to hand a review. Now, the only available person to take it right now is the young graduate you just hired two months ago. He looks at you and says:

… I don’t have the level to review your code.

18.03.2014 Hack On Business & Coder Dojo

Hiring Junior Developers (Photo taken at a coder dojo, credit: Medialab Prado)

That’s not a problem, and you should reassure him: a code review is not a teacher grading a student’s homework. It’s a conversation. Questions have as much values as advices there. No one said you as the reviewer could not  be the one to learn something.

As a junior, if he don’t understand something, he’s more than allowed to ask. Maybe he’ll get a nice explanation and learn something, which is good. And maybe it will help you to realize your code is not as clear as you wanted, and he will have helped to make it better.


Now, this is better: you finally finished that feature on project KillerWhale, and by chance, your colleague Andy just ended his own feature on project BuddistBunny. Having worked on all projects in the company, you start to review his code on BuddistBunny, sending him your modifications on KillerWhale to review. Andy was hired some months ago, and is a solid programmer with a very different experience than yours, so you are eager to get his review. Five minutes after, he’s back at your desk:

…can’t do that. I don’t know anything about the KillerWhale project

Of course, your company works on many different projects, and that’s true that Andy never had the opportunity to work on KillerWhale – the only things he knows about the project comes from what he heard at stand-up meetings.

So that’s actually a very good opportunity to get Andy onboard on KillerWhale: he’ll only need to focus on the small pieces of code you just wrote, and there are test to showcase what it should do. You are quite glad of the domain design you did on KillerWhale, and this will be a good test to see how good the naming actually is, as Andy is still an outsider on the project.


 So, you’re motivated to start, and want to get this code review practice in the team practices. You just have a meeting with your manager, Peter, so you explain quickly to him that you’ll start doing reviews and how great it will be. Peter is not a coder, so take some time for an explanation about quality improvement. After a short silence, Peter says:

… let me think about first, and get this to the management team.

Peter then goes back to your list of estimation of the last features the customer has asked for.

Of course, getting your boss’s support will always make things easier, especially when you want to spread a practice in a team or whole organization. But remember that ultimately, Peter pays you for your skills, skills that he himself doesn’t have (he has a lot others regarding planning and customer contact). Would you ask Peter whether you should do unit test? Or what library you should use to print PDFs?

At some point, as a craftsman, you’re responsible of your tools and of sharpening them. If you show results, no one will complains. At minima, ask to try it for a sprint or two. The experience will settle it much better that any discussion.


 So motivated after getting your first review of Andy’s code, you start looking at your other colleague, Bob, last feature. When you ask him a question, he snap back:

… I don’t need those review things. My code is good enough.shairez_rockstar_ninja

Bob is a good coder, but he mostly work by himself and sees code reviews as a waste of time and a way to control what he does.

There may be a way to work around that: turn the table. Ask Bob for advice on your code. That will get the discussion going, and it may ease him into it. Also, code reviews have a big advantage: they don’t need much to get started. You don’t need everyone in the company to agree – just one single person ready to do it with you once. Should it work well, maybe even Bob will take part. at some point.

Start now! Finish your current feature, and ask one of your colleague for his review.

Enhanced by Zemanta




PullReview: Badge and integration with BitBucket and GitLab

PullReview recently gets a few new features:

  • Badge
  • Public Review for Public Repo
  • BitBucket and GitLab
  • and lots more:
    • HipChat notification
    • Performance of the home page
    • Profile
    • Heartbleed
    • Support Contact

Badge

Badges in README.md are a very common way to inform people about your project good health. PullReview now provides its own badges:

badge PullReview KO

badge PullReview OK

Each badge presents a brief status as following:

  • ✗ the number of detected issues
  • ✔ the number of fixed issues
  • (+ the number of issues fixed since the previous review)

It will be colored depending on the progress:

  • red if
    • more issues have just been added than fixed
    • there are more to fix than fixed
  • otherwise it will be green

When you click on the badge you’ll then access the corresponding review page.

You’ll find the link and the markdown snippet for the badge just below the review title by clicking “Badge urls”.

badge-urls

The badges are also available for private repositories. In that case, the URI also contains a unique token given the access to the badge. The URI is then enough in itself to see the badge (but only the badge). Share it carefully.

 Share your badge and show off all your improvements!


Public Repositories reviews are now public

You can now share the URI of your review with anybody, and they will have access to your review. For instance, by clicking on a badge, a person can visit your review and discover its details. All the details are shared except the security vulnerabilities. To avoid premature disclosure, those ones are only readable by the owner of the review.


BitBucket and GitLab support

It’s now possible to review a repo hosted either on BitBucket or on GitLab. On the list of repositories, click on the button “Add a repository”

add-repo-button

Then fill the form to add your BitBucket or GitLab repo:

add-repo-form

Notice, that it’s also possible for GitHub repository when you didn’t grant us enough permission to retrieve the list of your repositories, and add deploy key and webhook.

For BitBucket as for GitHub, you can also grant us some permission that will ease your setup: retrieving the list of all your repositories and one-click setup adding deploy key and webhook.

bitbucket-auth


HipChat notification

HipChat is one of the great chat applications. It’s really a great way to communicate but also to be notified of everything that happens inside your team. In addition to Email and GitHub Status, you can now be notified directly into your HipChat room when a review is ready. Just go on your account page, browse the Notification tab, and setup the connection with HipChat

hipchat-form


Performance of the home page

We’ve improved the performance of the home page with the list of available reviews. For some of you, it will make a big difference as it’s no more slowed down by the size of your reviews.

We’re working now on the performance of the review page.


Profile

As you may have noticed, there is now a profile page

profile

Stay tuned – more and more metrics, awards and badges coming your way in the next weeks.


HeartBleed

You’ve certainly heard of HeartBleed vulnerability. We’ve already took several measures including:

  • We’ve upgraded all our systems to use the newer and protected version of OpenSSL. We did it on the productions servers the day just after the vulnerability became public.
  • We’ve reissued new SSL Certificates and revoked the old ones.

We’re still working on the issue, but on less urgent side effects.

For GitHub, BitBucket, and GitLab, we advise you to read their respective blog post on the topic. In addition, as PullReview relies on GitHub for login, we recommend you to follow their recommendations:


Support Contact

If you need support, have any questions, comments or feedback, there are several ways to contact us:


Happy Spring!

The sunny days are back for most of us. Always a great time to get out, spend time with friends and family, and leave the code behind you.

We’re here if you have any questions or need anything. As listed above, there are plenty of way to reach us.





7 daily use cases of Ruby String

Strings are everywhere. You deal with String instances not only every day, but probably every minute. They came from files, databases, REST APIs, or you simply use them to print results. It’s a pervasive representation, and Ruby provides plenty to ease its manipulation. But String comes with its own share of problems and you won’t always find a quick solution in the doc like how to deal with invalid byte sequence or convert back a String to a Date with an uncommon format.

Below, I share 7 common use cases of String I met very often and should be useful to you.

  1. How to remove enclosing characters like parenthesis?
  2. How to compare two Strings insensitively?
  3. How to clean a String by removing newline, carriage return, leading and trailing spaces?
  4. How to convert a String to a Regexp?
  5. How to convert a String to a Date with uncommon format?
  6. How to remove accents?
  7. How to clean up invalid byte sequences?

1. How to remove enclosing characters like parenthesis?

You retrieve data from a file:

line = "(this is a data from the file)"

The data is always wrapped by () that you don’t want. You can get the data without them as following:

line[1..-2]
# =>"this is a data from the file"

This actually removes the first and last characters of the String.

reference: String#[], String#slice

2. How to compare two Strings insensitively?

In an online shop, the user can search for product by their name:

products = [
 'Banana',
 'Apple',
 'Orange'
]

# ...

search_string = 'apple'

You don’t want to force your users to be case sensitive (they aren’t), so you can do:

products.select { |product| product.casecmp(search_string) == 0 }
# => ['Apple']

As pointed out by apeiros in the comments, it won’t work for any non-ASCII characters. So, a better version would be:

products.select { |product| product =~ /#{Regexp.escape(search_string)}/i }

reference: String#casecmp

3. How to clean a String by removing newline, carriage return, leading and trailing spaces?

In a little script, you read data from the console:

command = gets

You can clean up what you read by simply doing:

command = gets.strip

Thanks to apeiros for making me notice that strip removes also trailing newline and carriage return. It’s not needed to use chomp.

reference: String#chomp, String#strip

4. How to convert a String to a Regexp?

You’re developing a Gem that maintains a database of locations of files. In the configuration file, the user can blacklist specific pathnames with regular expressions. You retrieved one of them (you don’t want to locate the files in your trash):

blacklisted_pathname = '\.\/\.trash'

You can then use it to directly check:

file_paths.select  { |file_path| !(file_path =~ /#{blacklisted_pathname}/) }

or keep it for later:

r = Regexp.new blacklisted_pathnam

Don’t forget to correctly escaping in your string, or even better, use Regexp.escape as suggested by apeiros.

reference: Regexp#new, Regular Expressions literal and interpolation
Aside note: for that particular use case, you should probably use File#fnmatch

5. How to convert a String to a Date with uncommon format?

In an old database, the dates are stored as String as following:

date = '1979;4;2'

You can convert it as a Date as following:

Date.strptime(date, '%Y;%m;%d')
# => #<Date: 1979-04-02 ((2443966j,0s,0n),+0s,2299161j)>

reference: Date#strptime

6. How to remove accents?

You’re building a web app and you’d like to allow a quick search among the members like

'Kurt Gödel'

Accents and diacritics could be easily forgotten, and you want to remove them from the searched string. You can do it easily with ActiveSupport

require 'active_support/inflector' # not necessary if Rails

ActiveSupport::Inflector.transliterate('Kurt Gödel')
# => "Kurt Godel"

reference: Inflector#transliterate

7. How to clean up invalid byte sequences?

You read a text File encoded in UTF-8. You manipulate each of its line, but at some point you get the error

ArgumentError: invalid byte sequence in UTF-8

on the line

line = "This is an invalid byte sequence \244"
# => "This is an invalid byte sequence \xA4"

Well, you need to clean up your line before working on it by using scrub:

# if &lt; 2.1, use the backport gem string-scrub
# by adding to your Gemfile
# <code>gem 'string-scrub'
</code>
# if &gt;= 2.1, it's part of String (yeah \o/)

line.scrub!('')
# =&gt; "This is an invalid byte sequence "

Old way:

<del>line.encode!('UTF-8', 'binary', invalid: :replace, undef: :replace, replace: '')
# =&gt; "This is an invalid byte sequence "</del>

But if you have non-ASCII characters, it will remove them. In that case, you should go for a solution as apeiros’ one.

references: String#encode!, String#scrub! (2.1)
read also: Fight Back UTF-8 Invalid Byte Sequences

What’s your daily use cases of Strings?

Compared with daily use cases of Array, Hash, or Regexp, some for String are very basic, but very common. I love how Ruby allows you to deal with slicing for instance. It’s very elegant.

What’s yours? What are your typical daily use cases of String and elegant Ruby to resolve them?