1. 12

    1. 15

      Polluting the namespace with single-caller functions/methods is not better than code comments.

      You still have text words that need to be maintained to be accurate by the programmers, the compiler can’t help you ensure that SpoolTurboEncabsulators actually spools up your encabs… And now you’ve polluted the namespace. You have useless things coming up in autocomplete, and a year from now whoever ends up maintaining this thing (it’s probably you after forgetting all this mess) will have to be jumping around the file trying to understand it again.

    2. 7
      • replace should comments with code that checks;
      • replace what comments with meaningful identifiers; and
      • explain why more often and more thoroughly.

      Nicely put! Unfortunately, the first and second steps are not taught or even said to students (based on my own experience and a lot of Stack Overflow questions), and a lot of orgs prefer to document whys in a separate (that is, unmaintained) place which gets replaced every 10 years or so.

    3. 3

      Two more or less related thoughts on this:

      1. Regarding that “The comments are duplicative (…)” and people “may” — which is better spelled will — forget to update them: I try hard to reduce the number of in-comment references, especially “as a rule” of those going “up the dependency tree”. Instead, I take a solid effort to “turn my mind around” and try to describe what the thing is by itself, as if the other thing did not exist at all. Like, when describing SafeCommand, to think of it as if RunCommand was never to be. The first instinct may be that it’s hard or even impossible — but that’s just laziness and looming deadlines singing their siren song of seduction. When I let go of the thought that “it can’t be done without referencing RunCommand”, I start to actually try to think and describe what SafeCommand really is in its very core and meaning. In other words, I start actually writing a good documentation comment.

        In this particular example case, after I quashed my thought of “not possible to describe without RunCommand”, my mind started going in the direction of something like: “SafeCommand is a Linux command that we know is safe to run against risks Foo and Bar when called with arguments ‘woop’ and ‘boing’. A SafeCommand is important in scope of package blargh because it consumes blargh and emits rainbow farts useful for further processing.” You see? No reference to RunCommand at all, but an explanation of why it may be useful for someone in package blargh (where it is defined) instead. (Package blargh is intrinsically referenced by SafeCommand’s very existence in it. It need not be even spelled out in the commend, I just did it because it’s hard to show the file’s path in this post. But in Go the package name is really a part of a name of every entity defined in it, and that matters.) On the other hand, RunCommand already references SafeCommand in its argument list, so it may not even be necessary to duplicate that in the comment — a common Go documentation practice in the standard library is to only reference the argument name in the godoc comment — and that in fact is what the article author already did at that point in the text.

      2. Kind of tangential — and at a glance maybe already exemplified by the article — is that a cool trick I found for Naming Things (esp. funcs, structs, and fields) in Go, is to first use some quick name; then, write the actual function code; and finally, write a documentation comment for the function/entity, thinking about it as if the function/entity was not named at all. Thus, don’t try to make the godoc fit the name of the function, but instead just consider the current name as a temporary placeholder, and focus on writing the most accurate godoc. Then, more often than not (though possibly after a few rewrites/redactions of the godoc to make it even better), I found that the first sentence of the comment basically contains 1-3 words that will make a really great name for the function.

    4. 2

      Now we use the SafeCommand type to enforce that commands are safe.

      Depending on the situation, dynamic safety checking may also still be necessary … If you create a minimal package such that all callers are outside the package, you may be able to leave the dynamic check out.

      Alas, in Go, you can always produce the zero value of any exported type. So while importers may not be able to produce a SafeCommand with a command of e.g. “malicious”, they can definitely produce one with a command of the empty string.

      var zero yourpkg.SafeCommand

      Assuming a zero value SafeCommand isn’t valid, then RunCommand can’t avoid some amount of runtime input validation — you don’t want to pass command.command directly to exec.Command if it’s an unexpected value. So at a minimum you would need

      func RunCommand(c SafeCommand) error {
      	if c.command == "" { return fmt.Errorf("invalid command") }

      What’s revealed here is that types like SafeCommand are kind of like a door lock — they keep honest people honest, but don’t necessarily prevent dishonest people from being dishonest, you know? Code that deals with SafeCommands still has to be defensive in a way that can only occur at runtime.

      Given that, and IMO, and with a huge grain of salt, and all that, but I think all of the additional “scaffolding” created by the SafeCommand type doesn’t deliver much more value than a more direct expression, like

      var SafeCommands = []string{"pumpkin", "noodle", "tootle"}
      var safeCommands = func() []string { return SafeCommands }() // prevent third party modification
      func RunCommand(c string) error {
      	if !slices.Contains(safeCommands, c) { return fmt.Errorf("invalid command") }
    5. 1

      Here are slides from a related talk I gave a few years ago: https://sschwarzer.com/download/comments_pycon_de2019.pdf . Please ask if something isn’t clear enough.