7 ms·

Another thing Martin advocates for is not putting your name in comments, e.g. "Fixed a bug here; there could still be problematic interactions with subsystem fo

Another thing Martin advocates for is not putting your name in comments, e.g. "Fixed a bug here; there could still be problematic interactions with subsystem foo -- ericb". He says, "Source control systems are very good at remembering who added what, when." (p. 68, 2009 edition) Rubbish! Multiple times I've had to track down the original author of code that was auto-refactored, reformatted, changed locations, changed source control, etc. "git blame" and such are useless in these cases; it ends up being a game of Whodunit that involves hours of search, Slack pings, and what not. Just put your name in the comment, if it's documenting something substantial and is the result of your own research and struggle. And if you're in such a position, allow and encourage your colleagues to do this too.

ziml77 5y ago
What is even the downside of adding a few extra characters to the end of a comment to show who wrote it? And has Martin ever even worked on large, non-greenfield projects? That's the only way I could see anyone professing such idealism.
- jen20 5y ago
  > What is even the downside of adding a few extra characters to the end of a comment to show who wrote it? You're right - in fact we should do this for every line of code, so that we know of whom to ask questions! func main() { // jen20 fmt.Println("Hello World") // ziml77 } // jen20 What's the downside of adding a few extra characters!? Of course, this view is already available to people: `git blame` - and it's the same for comments, so there is no need. The exception is "notes to future self" during the development of a feature (to be removed before review), in which case the most useful place for them to appear is at the _start_ of the comment with a marker: // TODO(jen20): also implement function X for type Y Then they are easy to find...
  - ericbarrett 5y ago
    What you've shown is code clutter and reductio ad absurdum to what I wrote in the top-level comment. I am speaking of architectural comments, bug-fixes, and especially in service architecture where unusual and catastrophic interactions might happen (or have happened) with code that's not under your control.
  - max46 5y ago
    >You're right - in fact we should do this for every line of code, so that we know of whom to ask questions! if you want to live in this kind of absolutism, I would rather have the name on every line than no comments at all.
    - jen20 5y ago
      Good news then: git blame already does this, and modern editors show it all the time (via copying Code Lens features from Visual Studio). Install the appropriate extension for GitHub and you can have it there too, with no extra effort or maintenance burden of duplicative metadata.
jacquesm 5y ago
Better put such a long explanation there that your name isn't needed any more. Because if it is your name that makes the difference chances are that you have left the company by the time someone comes across that comment and needs access to your brain.
- ericbarrett 5y ago
  Sometimes what is interesting is that you have found that another engineer—whom you might not know in a large enough organization—has put time and thought into the code you are working on, and you can be a lot more efficient and less likely to break things if you can talk to that engineer first. It's not always the comment itself.
  - jacquesm 5y ago
    Sure, but in IT the rule of assumption should be that the code will outlive the coder. If being able to talk to other engineers is going to make the difference (instead of just being an optimization) then you already have problems.
    - ericbarrett 5y ago
      Talking to other engineers is necessary when your codebase, organization, and architecture is large enough that changes can have far-reaching effects. I would say, in fact, that it's the most important distinction between a junior and senior engineer. Let me give you a real-world example. Facebook used to have a service called BCF‡; it was basically a "lookup" service for a given host, where you could do forward- or reverse-lookups of a hostname, or a class of hosts, and get information about their physical location, network properties, hardware configuration, and so forth. This code was old. It had survived the transition from SVN to Git, and I'm sure it has in some form survived the transition from Git to Mercurial, though that was after my time. It had also been moved several times as no team formally owned the service. It was originally slapped together under extreme pressure by a few engineers, basically a "hackathon." Despite that, it was so useful that it had been adopted by pretty much every team that touched infrastructure, which at the time I became involved was ~300 engineers. I was working on a service that made extensive use of the data in BCF. There was a problem with one of its Thrift RPCs which required a bugfix. This bug had plagued users of the service for several years, but because the code was so old and hoary—it didn't even have auto-generation of its Thrift bindings—nobody had bothered to fix it. Instead, every team who used this RPC had coded around it, or (worse still) skipped the binding and queried the backing database directly. Well, I was determined to fix the bug. "git blame" showed a bot. No problem, let's go back before that commit...another bot. Before that, a human! Cool, let's reach out—no, turns out he'd done some code formatting on it. Before that—whoops, the beginning of the code history! OK, so check the old SVN repo. Five contributors over its history. I pinged each and every one who was still at the company—they hadn't written it. Finally got ahold of somebody who said, "Oh yeah, Samir‡ wrote that, ask him." I looked up and realized I could see the back of Samir's head, because he sat 10m away. It took two hours to get to that point, and 5 minutes to sort out what I needed from him without literally bringing down the site. I fixed the bug. Every single one of those things, of course, defied the "rule of assumption." But every single one of those things was done under a specific kind of duress: keeping the company running and the features rolling. The real world is messy, and putting a little extra in your comments and leaving threads for future engineers is an enormously powerful lubricant. ‡ names changed to protect the guilty
SassyGrapefruit 5y ago
I think most of what martin says is rubbish, but this is not. I have never had `git blame` fail...ever. I know what user is responsible for every line of code. Doing this is contemporaneous. Its right up there with commenting out blocks of code so you don't lose them.
- ericbarrett 5y ago
  I don't know what to say, this is a real problem I have encountered in actual production code multiple times. Any code that lives longer than your company's source control of choice, style of choice, or code structure of choice is vulnerable. Moreover, what's the harm? It's more information, not less.
  - SassyGrapefruit 5y ago
    >Moreover, what's the harm? It's more information, not less If your code is outliving your source control system you got bigger problems than whatever is in your comments. I can confidently say that in 25 years in this industry this isn't something I've encountered. So probably sufficiently low enough probability to safely ignore. >Moreover, what's the harm? It's more information, not less. Too much information is every bit as bad as too little.
    - triceratops 5y ago
      > I can confidently say that in 25 years in this industry this isn't something I've encountered. I've been in the industry less than half that time and it's happened to me. blame will tell you the last person/commit to touch that line of code. To find out when it was originally written, I may have to (in some cases) manually binary search the history of the file.
      - SassyGrapefruit 5y ago
        why? Just go to the commit hash in the blame and run blame again. All in all it has a negligible impact on the readability of the code. It's mostly aesthetic for me. It's ugly and only solves far fetched problems. Do you scratch your name and SSN into the side of your car? What if your title blows away in the wind on the same day that city hall burns down destroying all ownership records?
nemetroid 5y ago
It does require some rigor in your version control practices, but I'm happily "git blame"-ing twenty year old code every now and then.
watwut 5y ago
If you have to track down the author, then it is already bad. The code should not hope that the author never finds a new job.
- throw1234651234 5y ago
  A 1000 times this. We never use git blame - who cares? The code should be self-explanatory, and if it's not, the author doesn't remember why they did it 5 years down the line either.
  - erik_seaberg 5y ago
    I rarely want the author name. What I want is the commit message explaining the changes, and I want to see what the code used to do. Code without history is nearly unsupportable without reverse engineering it all.
- cloverich 5y ago
  But sometimes it is bad, and not fixable within the author's control. I occasionally leave author notes, as a shortcut. If I'm no longer here, yeah you gotta figure it all out the hard way. But if I am, I can probably save you a week, maybe a month. And obviously if its something you can succintly describe, you'd just leave a comment. This is the domain of "Based on being here a few years on a few teams, and three services between this one, a few migrations etc etc". Some business problems have a lot of baggage that aren't easily documented or described, its the hard thing about professional development especially in a changing business. There's also cases where I _didnt'_ author the code, but did purposefully not change something that looks like it should be changed. In those cases, without my name comment, git blame wouldn't point you to me. YMMV.
Cthulhu_ 5y ago
I think your comment is controversial, for a number of reasons. One, I think nobody should own code. Code should be obvious, tested, documented and reviewed (bringing the number of people involved to at least two), the story behind it should be either in the git comments or referenced to e.g. a task management system. Code ownership just creates islands. I mean by all means assign a "domain expert" to a PART of your code, but no individual segment of code should belong to anyone. Second: There's something to be said about avoiding churn. Everybody loves refactoring and rewriting code, present company included, but it muddles the version control waters. I've seen a few github projects where the guidelines stated not to create PRs for minor refactorings, because they create churn and version control noise. Anyway, that's all "ideal world" thinking, I know in practice it doesn't work like that.
- Chris2048 5y ago
  Maybe not exclusive ownership, but there are always going to be those more familiar with a section of code that others. It's not really efficient to insist everyone know the codebase equally, especially with larger codebases.
bjohnson225 5y ago
I never found people adding a name useful. Either the code is recent (in which case 'git blame' works better since someone changing a few characters may or may not decide to add their name to the file) or it's old and the author has either left the company or has forgotten practically everything about the code.

ziml77 5y ago

What is even the downside of adding a few extra characters to the end of a comment to show who wrote it? And has Martin ever even worked on large, non-greenfield projects? That's the only way I could see anyone professing such idealism.

jen20 5y ago
> What is even the downside of adding a few extra characters to the end of a comment to show who wrote it? You're right - in fact we should do this for every line of code, so that we know of whom to ask questions! func main() { // jen20 fmt.Println("Hello World") // ziml77 } // jen20 What's the downside of adding a few extra characters!? Of course, this view is already available to people: `git blame` - and it's the same for comments, so there is no need. The exception is "notes to future self" during the development of a feature (to be removed before review), in which case the most useful place for them to appear is at the _start_ of the comment with a marker: // TODO(jen20): also implement function X for type Y Then they are easy to find...
- ericbarrett 5y ago
  What you've shown is code clutter and reductio ad absurdum to what I wrote in the top-level comment. I am speaking of architectural comments, bug-fixes, and especially in service architecture where unusual and catastrophic interactions might happen (or have happened) with code that's not under your control.
- max46 5y ago
  >You're right - in fact we should do this for every line of code, so that we know of whom to ask questions! if you want to live in this kind of absolutism, I would rather have the name on every line than no comments at all.
  - jen20 5y ago
    Good news then: git blame already does this, and modern editors show it all the time (via copying Code Lens features from Visual Studio). Install the appropriate extension for GitHub and you can have it there too, with no extra effort or maintenance burden of duplicative metadata.

jacquesm 5y ago

Better put such a long explanation there that your name isn't needed any more. Because if it is your name that makes the difference chances are that you have left the company by the time someone comes across that comment and needs access to your brain.

ericbarrett 5y ago
Sometimes what is interesting is that you have found that another engineer—whom you might not know in a large enough organization—has put time and thought into the code you are working on, and you can be a lot more efficient and less likely to break things if you can talk to that engineer first. It's not always the comment itself.
- jacquesm 5y ago
  Sure, but in IT the rule of assumption should be that the code will outlive the coder. If being able to talk to other engineers is going to make the difference (instead of just being an optimization) then you already have problems.
  - ericbarrett 5y ago
    Talking to other engineers is necessary when your codebase, organization, and architecture is large enough that changes can have far-reaching effects. I would say, in fact, that it's the most important distinction between a junior and senior engineer. Let me give you a real-world example. Facebook used to have a service called BCF‡; it was basically a "lookup" service for a given host, where you could do forward- or reverse-lookups of a hostname, or a class of hosts, and get information about their physical location, network properties, hardware configuration, and so forth. This code was old. It had survived the transition from SVN to Git, and I'm sure it has in some form survived the transition from Git to Mercurial, though that was after my time. It had also been moved several times as no team formally owned the service. It was originally slapped together under extreme pressure by a few engineers, basically a "hackathon." Despite that, it was so useful that it had been adopted by pretty much every team that touched infrastructure, which at the time I became involved was ~300 engineers. I was working on a service that made extensive use of the data in BCF. There was a problem with one of its Thrift RPCs which required a bugfix. This bug had plagued users of the service for several years, but because the code was so old and hoary—it didn't even have auto-generation of its Thrift bindings—nobody had bothered to fix it. Instead, every team who used this RPC had coded around it, or (worse still) skipped the binding and queried the backing database directly. Well, I was determined to fix the bug. "git blame" showed a bot. No problem, let's go back before that commit...another bot. Before that, a human! Cool, let's reach out—no, turns out he'd done some code formatting on it. Before that—whoops, the beginning of the code history! OK, so check the old SVN repo. Five contributors over its history. I pinged each and every one who was still at the company—they hadn't written it. Finally got ahold of somebody who said, "Oh yeah, Samir‡ wrote that, ask him." I looked up and realized I could see the back of Samir's head, because he sat 10m away. It took two hours to get to that point, and 5 minutes to sort out what I needed from him without literally bringing down the site. I fixed the bug. Every single one of those things, of course, defied the "rule of assumption." But every single one of those things was done under a specific kind of duress: keeping the company running and the features rolling. The real world is messy, and putting a little extra in your comments and leaving threads for future engineers is an enormously powerful lubricant. ‡ names changed to protect the guilty

SassyGrapefruit 5y ago

I think most of what martin says is rubbish, but this is not. I have never had `git blame` fail...ever. I know what user is responsible for every line of code. Doing this is contemporaneous. Its right up there with commenting out blocks of code so you don't lose them.

ericbarrett 5y ago
I don't know what to say, this is a real problem I have encountered in actual production code multiple times. Any code that lives longer than your company's source control of choice, style of choice, or code structure of choice is vulnerable. Moreover, what's the harm? It's more information, not less.
- SassyGrapefruit 5y ago
  >Moreover, what's the harm? It's more information, not less If your code is outliving your source control system you got bigger problems than whatever is in your comments. I can confidently say that in 25 years in this industry this isn't something I've encountered. So probably sufficiently low enough probability to safely ignore. >Moreover, what's the harm? It's more information, not less. Too much information is every bit as bad as too little.
  - triceratops 5y ago
    > I can confidently say that in 25 years in this industry this isn't something I've encountered. I've been in the industry less than half that time and it's happened to me. blame will tell you the last person/commit to touch that line of code. To find out when it was originally written, I may have to (in some cases) manually binary search the history of the file.
    - SassyGrapefruit 5y ago
      why? Just go to the commit hash in the blame and run blame again. All in all it has a negligible impact on the readability of the code. It's mostly aesthetic for me. It's ugly and only solves far fetched problems. Do you scratch your name and SSN into the side of your car? What if your title blows away in the wind on the same day that city hall burns down destroying all ownership records?

nemetroid 5y ago

It does require some rigor in your version control practices, but I'm happily "git blame"-ing twenty year old code every now and then.

watwut 5y ago

If you have to track down the author, then it is already bad. The code should not hope that the author never finds a new job.

throw1234651234 5y ago
A 1000 times this. We never use git blame - who cares? The code should be self-explanatory, and if it's not, the author doesn't remember why they did it 5 years down the line either.
- erik_seaberg 5y ago
  I rarely want the author name. What I want is the commit message explaining the changes, and I want to see what the code used to do. Code without history is nearly unsupportable without reverse engineering it all.
cloverich 5y ago
But sometimes it is bad, and not fixable within the author's control. I occasionally leave author notes, as a shortcut. If I'm no longer here, yeah you gotta figure it all out the hard way. But if I am, I can probably save you a week, maybe a month. And obviously if its something you can succintly describe, you'd just leave a comment. This is the domain of "Based on being here a few years on a few teams, and three services between this one, a few migrations etc etc". Some business problems have a lot of baggage that aren't easily documented or described, its the hard thing about professional development especially in a changing business. There's also cases where I _didnt'_ author the code, but did purposefully not change something that looks like it should be changed. In those cases, without my name comment, git blame wouldn't point you to me. YMMV.

Cthulhu_ 5y ago

I think your comment is controversial, for a number of reasons. One, I think nobody should own code. Code should be obvious, tested, documented and reviewed (bringing the number of people involved to at least two), the story behind it should be either in the git comments or referenced to e.g. a task management system. Code ownership just creates islands. I mean by all means assign a "domain expert" to a PART of your code, but no individual segment of code should belong to anyone. Second: There's something to be said about avoiding churn. Everybody loves refactoring and rewriting code, present company included, but it muddles the version control waters. I've seen a few github projects where the guidelines stated not to create PRs for minor refactorings, because they create churn and version control noise. Anyway, that's all "ideal world" thinking, I know in practice it doesn't work like that.

Chris2048 5y ago
Maybe not exclusive ownership, but there are always going to be those more familiar with a section of code that others. It's not really efficient to insist everyone know the codebase equally, especially with larger codebases.

bjohnson225 5y ago

I never found people adding a name useful. Either the code is recent (in which case 'git blame' works better since someone changing a few characters may or may not decide to add their name to the file) or it's old and the author has either left the company or has forgotten practically everything about the code.