wiki: CodeReviewComments additions

[slewiskelly]

Propose adding a handful of sections to the CodeReviewComments page. The sections that follow provide advice given during Go readability reviews at Google.

Import _

Packages that are imported only for their side effects (using the syntax import _ "pkg") should only be imported in the main package of a program, or in tests that require them.

Avoid blank imports in library packages, even if the library indirectly depends on them. Containing side-effect imports in the main package helps control dependencies, and makes it possible to write tests that rely on a different side-effect without conflict.

If you create a library package that indirectly depends on a side-effect import, leave a comment reminding users to import that package in their main package.

Switch Break

Avoid the use of redundant break statements without target labels at the ends of switch clauses. Unlike in C or Java, switch clauses in Go automatically break, and a fallthrough statement is needed to achieve the C-style behavior.

Use a comment if you want to clarify the purpose of an empty clause.

Prefer:

switch x {
case "A", "B":
  buf.WriteString(x)
case "C":
  // Handled outside of the switch statement
default:
  return fmt.Errorf("unknown value: %q", x)
}

and not:

switch x {
case "A", "B":
  buf.WriteString(x)
  break  // Redundant
case "C":
  break // Redundant
default:
  return fmt.Errorf("unknown value: %q", x)
}

Type Aliases

Use a type definition (type T1 T2) to define a new type.

Use an alias declaration (type T1 = T2) to refer to an existing type without defining a new type.

Prefer type definitions over alias declarations.

Use %q

Go's format functions (fmt.Printf and friends) have a %q verb which prints a string inside quotation marks. It should be used in preference to doing the equivalent manually.

Prefer:

 fmt.Printf("value %q looks like English text", someText)

and not:

 fmt.Printf("value '%s' looks like English text", someText)

Also, note that using %q is a good idea in output intended for humans where the input value could possibly be empty. It can be very hard to notice a silent empty string, but "" stands out clearly as such.

[toothrot]

/cc @ianlancetaylor

[ianlancetaylor]

These all seem fine to me. I started a thread on golang-dev to raise awareness of the suggested changes (https://groups.google.com/forum/?pli=1#!topic/golang-dev/u1RugupaP_Y). Let's give it a few days. Thanks.

[ainar-g]

%q may produce unnecessary escaping when printing e.g. JSON. Maybe at least mention %#q?

[cespare]

I don't think I agree with the Type Aliases section.

Use a type definition (type T1 T2) to define a new type.

Use an alias declaration (type T1 = T2) to refer to an existing type without defining a new type.

These two sentences describe how the language works, which I don't think is the purpose of the document.

Prefer type definitions over alias declarations.

It's not clear to me that this is a matter of style. Usually you need the semantics of one or the other.

[vcabbage]

The import _ advice seems situational to me. For example, I would expect the import for a database driver to be in the same package as where the sql.Open happens. That could be in main or, as is typical in my code, it could happen in a package that abstracts much of the DB logic.

I also think importing in main rather than where a package is used is more likely to result in unused imports since removing the package which requires it will not remove the dependency.

The advice seems more applicable to library authors than those writing applications that are comprised of multiple packages.

[ChrisHines]

%q also obfuscates Windows file paths by doubling the \ path separators.