Set max-len to 80 columns

[mccraveiro]

Airbnb's uses 100 columns as max length of a line however our experience using Github tells that 80 columns still the best setting. It also ensures max readability and forces developers to write clean code.

[thalesmello]

What are the drawbacks of using 100 in spite of 80?

On Sat, Jul 22, 2017, 12:57 Mateus Craveiro notifications@github.com wrote:

Airbnb's uses 100 columns as max length of a line however our experience using Github tells that 80 columns still the best setting. It also ensures max readability and forces developers to write clean code.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/pagarme/javascript-style-guide/issues/10, or mute the thread https://github.com/notifications/unsubscribe-auth/ABEz4vYq4Ofuxme1sWFUIMb6jFdJeTyRks5sQhv9gaJpZM4OgNXd .

[mccraveiro]

For once, Github uses 80 columns on all visualizations. Therefore forcing 80 columns improves code reviews.

[thalesmello]

@mccraveiro That's not true. That actually depends on the resolution of your monitor.

If you "ctrl -" your browser, you will see more characters fit in the screen.

[thalesmello]

I'm actually agains using 80 characters.

100 is short enough.

If we contrain ourselves to 80 characters, it will be more difficult to use expressive names, kind of forcing people to write shorter variable names, such as m or f.

100 sounds like the sweet spot.

[MarcoWorms]

I don't agree with you @thalesmello. 80 characters doesn't enforce shorter names, in javascript you can always break the line to not reach 80 characters unless your function name is gigantic which means it's a bad function name.

[MarcoWorms]

IMO 80 characters is a really good limit and unless you are messing with HTML you should probably never reach it with good indentation practices. Also, shorter overall code width means we can have more vertical splits in our monitors, I'm in favor of 80 chars width

[thalesmello]

80 characters is too short, 120 characters would be too long.

100 is just right.

Relevant discussions: https://www.quora.com/Why-does-Google-Java-coding-style-guide-chose-maximum-of-100-characters-column-limit-per-line-why-not-120-just-curious-to-know https://wincent.com/blog/on-the-80-column-limit

[MarcoWorms]

Could you show me an example of well writen javascript where 80 column limit fail?

The articles aren't about js :P

[derekstavis]

My personal preference is 80 columns, but I always try to keep at 72.

The first thing to have in mind is: Expressive names comes first.

Having this said, I like keeping at 80 to force myself to write less nested code. The only exception for never breaking the like are strings that I want to keep verbatim, as adding newlines could erroneously include additional spaces, tabs etc.

But let's expand to the main problem that we have today:

This is a code sample from one of our projects:

|--------------------------------------------------------------------------------------------------| 100
|------------------------------------------------------------------------------| 80

return res.send(400, responseHelper.singleErrorResponse(req, 'action_forbidden', null,`"Transação com status '${transaction.status}' não pode ser estornada`))

Yeah, looks pretty ugly. It helps if we format this way:

|--------------------------------------------------------------------------------------------------| 100
|------------------------------------------------------------------------------| 80
const { singleErrorResponse } = responseHelper

const responseBody = singleErrorResponse(
    req,
    'action_forbidden',
    null,
    `Transação com status '${transaction.status}' não pode ser estornada`
)

return res.send(400, responseBody)

With beautiful API design, we move that optional null argument to the end and just don't fill it:

|--------------------------------------------------------------------------------------------------| 100
|------------------------------------------------------------------------------| 80
const { singleErrorResponse } = responseHelper

const responseBody = singleErrorResponse(
  req,
  'action_forbidden',
  `Transação com status '${transaction.status}' não pode ser estornada`
)

return res.send(400, responseBody)

With this examples, I'd like to argument that it's more productive to have a constrain of 80 columns for most code and a standard way of breaking lines on function declaration/call, class, if and other possible constructs, as, inherently code with expressive names will need line breaks.

[mccraveiro]

@derekstavis you nailed it!

We should not be discussing if the technology we have today support 80 or more columns. Instead we should view 80 columns as a restriction that enforces better code. I personally have never seen a 80+ columns code that couldn't be refactored into cleaner code that uses less than 80 columns.

Side note: ESLint has support for strings that cannot be broken into more lines.

[thalesmello]

Splitting function calls across multiple lines is terrible, in IMHO. If makes it convenient to fit more arguments in a function call, which is bad design. Let me explain what I mean.

Consider the following example:

// I want to write the following function implementation (present in the DataOps migration project).
// If I allow up to 100 characters, I could write everything comfortably in the same line.
|--------------------------------------------------------------------------------------------------| 100
|------------------------------------------------------------------------------| 80
async function run () {
  const template = await readFileAsync('./redshift_migrations/.migration_template.js', 'utf-8')
}

|--------------------------------------------------------------------------------------------------| 100
|------------------------------------------------------------------------------| 80
// If I were to use 80 characters, I would have two options:
async function run () {
  // Splitting in the arguments in separate lines, which feels quite weird, to be sincere
  const template = await readFileAsync(
    './redshift_migrations/.migration_template.js',
    'utf-8'
  )

  // Or extracting the argument in separate variables:
  const path = './redshift_migrations/.migration_template.js'
  const template = await readFileAsync(path, 'utf-8')

  // Which, in my opinion, is not worth it. The 100 characters version isn't that long, fits nicely in the screen monitor, and allows for more code to be written as a single line, without having to break everything into separate lines.
}

Let's work on @derekstavis "ugly" version of the example. IMHO, splitting in different lines yields bad results. In my opinion, a better approach would be one of the two approaches:

|--------------------------------------------------------------------------------------------------| 100
|------------------------------------------------------------------------------| 80
const type = 'action_forbidden'
const param = null
// What a convenience! Fits under 100 characters
const message =  `Transação com status '${transaction.status}' não pode ser estornada`
const responseBody = singleErrorResponse(req, type, param, message)
// As a bonus, we get documentation.

// The second approach, even better IMHO, would be to change the interface of the
// function to take an object instead.
const responseBody = singleErrorResponse({
    req,
    type: 'action_forbidden',
    parameter: null,
    message: `Transação com status '${transaction.status}' não pode ser estornada`
})

[thalesmello]

My final point is that 100 characters is fine. The added 20 characters aren't much longer, still fits just right in the screen, and make people less tempted to split function calls into multiple lines.

It's not worth it to change this rule.

[MarcoWorms]

See for yourself the examples you gave:

I'm in the office's notebook and in fullscreen. Your 100 max width example is unreadable without scrolling, while the 80 one is completely fine to read.

I am even more convinced now after this post that limiting in 80 makes more sense. 100 is arbitrary and still doesn't make sense to me, if you want to break the 80 limit than I rather have no limit at all :P but I'm still totally in favor of limiting in 80 for better readability.

[derekstavis]

The problem @thalesmello is that in async function run () { ... example you already need to break lines. If someone ever resolves to create a ./redshift_migrations/.migration_template_join_table.js the utf-8 argument will render unreadable, even with 100 columns :)

[thalesmello]

@MarcoWorms

Which browser are you using?

100 characters fits just right.

[thalesmello]

@derekstavis In my original example, no.

But, if in fact I get a longer path, I would just use:

async function run () {
  const path = './redshift_migrations/.migration_template_join_table.js'
  const template = await readFileAsync(path, 'utf-8')
}

[MarcoWorms]

@thalesmello it's a normal chrome

fullscreen example

[MarcoWorms]

Also, IMO it's unfair to use high res monitors as a standard for example. This one I'm using is 1366 x 768 and almost anyone in the company has this resolution, with 80 max width you can have 2 vertical splits at a decent font size, 100 would be impossible to fit in 2 vsplits without getting the font to a small and almost undeadable size.

[thalesmello]

Considering the arguments provided, I now agree with having a max of 80 columns.