|
123456789101112131415161718192021222324252627282930313233343536373839404142434445464748495051525354555657585960616263646566676869707172737475767778798081828384858687888990919293949596979899100101102103104105106107108109110111112113114115116117118119120121122123124125126127128129130131132133134135136137138139140141142143144145146147148149150151152153154155156157158159160161162163164165166167168169170171172173174175176177178179180181182183184185186187188189190191192193194195196197198199200201202203204205206207208209210211212213214215216217218219220221222223224225226227228229230 |
- title: Every line of code is always documented
- url: https://mislav.net/2014/02/hidden-documentation/
- hash_url: d92f0d39965b2b41d2701d3a6a114c3c
-
- <p>Every line of code comes with a hidden piece of documentation.</p>
-
- <p>Whoever wrote line 4 of the following code snippet decided to access the
- <code>clientLeft</code> property of a DOM node for some reason, but do nothing with the
- result. It’s pretty mysterious. Can you tell why they did it, or is it safe to
- change or remove that call in the future?</p>
-
- <div class="highlight"><pre><code class="language-js" data-lang="js"><span class="lineno">1</span> <span class="c1">// ...</span>
- <span class="lineno">2</span> <span class="k">if</span> <span class="p">(</span><span class="nx">duration</span> <span class="o">></span> <span class="mi">0</span><span class="p">)</span> <span class="k">this</span><span class="p">.</span><span class="nx">bind</span><span class="p">(</span><span class="nx">endEvent</span><span class="p">,</span> <span class="nx">wrappedCallback</span><span class="p">)</span>
- <span class="lineno">3</span>
- <span class="lineno">4</span> <span class="hll"><span class="k">this</span><span class="p">.</span><span class="nx">get</span><span class="p">(</span><span class="mi">0</span><span class="p">).</span><span class="nx">clientLeft</span>
- </span><span class="lineno">5</span>
- <span class="lineno">6</span> <span class="k">this</span><span class="p">.</span><span class="nx">css</span><span class="p">(</span><span class="nx">cssValues</span><span class="p">)</span></code></pre></div>
-
- <p>If someone pasted you this code, like I did here, you probably won’t be able to
- tell who wrote this line, what was their reasoning, and is it necessary to keep
- it. <em>However</em>, most of the time when working on a project you’ll have access to
- its history via version control systems.</p>
-
- <p><strong>A project’s history is its most valuable documentation.</strong></p>
-
- <p>The mystery ends when we view the commit message which introduced this line:</p>
-
- <div class="highlight"><pre><code class="language-bash" data-lang="bash"><span class="nv">$ </span>git show <span class="k">$(</span>git blame example.js -L 4,4 <span class="p">|</span> awk <span class="s1">'{print $1}'</span><span class="k">)</span></code></pre></div>
-
- <blockquote><p><strong>Fix animate() for elements just added to DOM</strong></p>
-
- <p>Activating CSS transitions for an element just added to the DOM won’t work in
- either Webkit or Mozilla. To work around this, we used to defer setting CSS
- properties with setTimeout (see 272513b).</p>
-
- <p>This solved the problem for Webkit, but not for latest versions of Firefox.
- Mozilla seems to need at least 15ms timeout, and even this value varies.</p>
-
- <p>A better solution for both engines is to trigger “layout”. This is done here
- by reading <code>clientLeft</code> from an element. There are other properties and
- methods that trigger layout; see
- <a href="http://gent.ilcore.com/2011/03/how-not-to-trigger-layout-in-webkit.html">gent.ilcore.com/2011/03/how-not-to-trigger-layout-in-webkit</a></p></blockquote>
-
- <p>As it turns out, this line—more specifically, the <em>change</em> which introduced this
- line—is <strong>heavily documented</strong> with information about why it was necessary, why did
- the previous approach (referred to by a commit SHA) not work, which browsers are
- affected, and a link for further reading.</p>
-
- <p>As it also turns out, <a href="https://github.com/madrobby/zepto/pull/586">the author of that mysterious line was me</a>. There are
- ways I could have written that code itself better: by <strong>encapsulating the magic
- property access</strong> in a function with an <a href="http://signalvnoise.com/posts/3531-intention-revealing-methods">intention-revealing name</a> such as
- <code>triggerLayout()</code>, or at least by <strong>adding a code comment</strong> with a short
- explanation that this kicks off the animation. For whatever reason, I might have
- failed that day to make this particular code expressive. <strong>Code happens, and
- it’s not always perfect</strong>.</p>
-
- <p>Even if this code <em>was</em> more expressive or if it <em>had</em> contained lines of code
- comments, a project’s history will be able to provide even richer information:</p>
-
- <ol>
- <li><em>Who</em> added this code;</li>
- <li><em>When</em> did they add this code;</li>
- <li>Which was the <em>accompanying test</em> (if any);</li>
- <li>The full commit message can be a whole novel (while code comments should be
- kept succinct).</li>
- </ol>
-
- <p>Code quality still matters a lot. But when pondering how you could improve your
- coding even further, you should consider aiming for better commit messages. You
- should request this not just from yourself, but from your entire team and all
- the contributors. <strong>The story of a software matters as much as its latest
- checkout</strong>.</p>
-
- <h2>Effective spelunking of project’s history</h2>
-
- <h3>git blame</h3>
-
- <p>I’ve already demonstrated how to use <code>git blame</code> from the command line above.
- When you don’t have access to the local git repository, you can also open the
- “Blame” view for <a href="https://github.com/madrobby/zepto/blame/2ed0123eaddc023a8579df0a3a084a70a392d792/src/fx.js#L90">any file on GitHub</a>.</p>
-
- <p>A very effective way of exploring a file’s history is with Vim and <a href="https://github.com/tpope/vim-fugitive" title="fugitive.vim: a Git wrapper so awesome, it should be illegal">Fugitive</a>:</p>
-
- <ol>
- <li>Use <code>:Gblame</code> in a buffer to open the blame view;</li>
- <li>If you need to go deeper, press <kbd>Shift-P</kbd> on a line of blame pane to
- re-blame at the parent of that commit;</li>
- <li>Press <kbd>o</kbd> to open a split showing the commit currently selected in
- the blame pane.</li>
- <li>Use <code>:Gbrowse</code> in the commit split to open the commit in the GitHub web interface;</li>
- <li>Press <kbd>gq</kbd> to close the blame pane and return to the main buffer.</li>
- </ol>
-
- <p><img width=827 height=445 style="max-width:100%" alt="git blame view in vim Fugitive"
- src="https://mislav.net/images/Screen%20Shot%202014-02-07%20at%203.38.20%20PM.png"></p>
-
- <p>See <code>:help Gblame</code> for more information.</p>
-
- <h3>Find the pull request where a commit originated</h3>
-
- <p>With git blame you might have obtained a commit SHA that introduced a change,
- but commit messages don’t always carry enough information or context to explain
- the rationale behind the change. However, if the team behind a project practices
- <a href="http://guides.github.com/overviews/flow/" title="Lightweight, branch-based workflow that supports teams and projects where deployments are made regularly">GitHub Flow</a>, the context might be found in the pull request discussion:</p>
-
- <div class="highlight"><pre><code class="language-bash" data-lang="bash"><span class="nv">$ </span>git log --merges --ancestry-path --oneline <SHA>..origin <span class="p">|</span> tail
- ...
- <span class="hll">bc4712d Merge pull request <span class="c">#42 from sticky-sidebar</span>
- </span>3f883f0 Merge branch <span class="s1">'master'</span> into sticky-sidebar</code></pre></div>
-
- <p>Here, a single commit SHA was enough to discover that it originated in pull
- request #42.</p>
-
- <h3>The git pickaxe</h3>
-
- <p>Sometimes you’ll be trying to find something that is missing: for instance, a
- past call to a function that is no longer invoked from anywhere. The best way to
- find which commits have introduced or removed a certain keyword is with the
- ‘pickaxe’ argument to <code>git log</code>:</p>
-
- <pre><code>$ git log -S<string>
- </code></pre>
-
- <p>This way you can dig up commits that have, for example, removed calls to a
- specific function, or added a certain CSS classname.</p>
-
- <h3>git churn</h3>
-
- <p>It’s possible to get valuable insight from history of a project not only by
- viewing individual commits, but by <strong>analyzing sets of changes as a whole</strong>. For
- instance, <a href="https://github.com/garybernhardt/dotfiles/blob/f0c0ff92209e5aed4fa3ef6faf056eb9944a8f12/bin/git-churn">git-churn</a> is a simple but valuable script that wraps
- <code>git log</code> to compile stats about which files change the most. For example, to
- see where the development of an app was focused on in the past 6 months:</p>
-
- <pre><code>$ git churn --since='6 months ago' app/ | tail
- </code></pre>
-
- <p>Incidentally, such analysis also highlights potential problems with technical
- debt in a project. A specific file changing too often is generally a red flag,
- since it probably means that the code in that file either needed to be
- frequently fixed for bugs, or that the file holds too much responsibility in
- general and should be split into smaller units.</p>
-
- <p>Similar methods of history analysis can be employed to see which people were
- responsible recently for development of a certain part of the codebase. For
- instance, to see who contributed most often to the API part of an application:</p>
-
- <pre><code>$ git log --format='%an' --since='6 months ago' app/controllers/api/ | \
- sort | uniq -c | sort -rn | head
-
- 109 Edmond Dantès
- 13 Jonathan Livingston
- 7 Ebanezer Scrooge
- </code></pre>
-
- <h2>Being on the right side of history</h2>
-
- <p>Keep in mind that everything that you’re making today is going to enter the
- project’s history and stay there forever. To be nicer to other people who work
- with you (even if it’s a solo project, that includes yourself in 3 months),
- follow these ground rules when making commits:</p>
-
- <ul>
- <li><p>Always write commit messages as if you are <strong>explaining the change</strong> to a
- colleague sitting next to you who has no idea of what’s going on. Per
- <a href="http://robots.thoughtbot.com/5-useful-tips-for-a-better-commit-message" title="5 Useful Tips For A Better Commit Message">Thoughtbot’s tips for better commit messages</a>:</p>
-
- <blockquote><p>Answer the following questions:</p>
-
- <ul>
- <li>Why is this change necessary?</li>
- <li>How does it address the issue?</li>
- <li>What side effects does this change have?</li>
- <li>Consider including a link [to the discussion.]</li>
- </ul>
- </blockquote></li>
- <li><p><strong>Avoid unrelated changes in a single commit</strong>. You might have spotted a typo
- or did tiny code refactoring in the same file where you made some other changes,
- but resist the temptation to record them together with the main change unless
- they’re directly related.</p></li>
- <li><p><strong>Always be cleaning up your history before pushing</strong>. If the commits haven’t
- been shared yet, it’s safe to <a href="/2013/02/merge-vs-rebase/" title="Git merge vs. rebase">rebase the heck out of them</a>. The
- following could have been permanent history of the Faraday project, but I
- squashed it down to only 2 commits and edited their messages to hide the fact
- I had troubles setting the script up in the first place:</p>
-
- <p><img width=470 style="max-width:100%" alt="messy git history before rebase"
- src="https://mislav.net/images/Image%202013-04-04%20at%201.38.33%20AM.png"></p></li>
- <li><p>Corollary of avoiding unrelated changes: <strong>stick to a line-based coding
- style</strong> that allows you to append, edit or remove values from lists without
- changing adjacent lines. Some examples:</p>
-
- <pre><code> var one = "foo"
- , two = "bar"
- , three = "baz" // Comma-first style allows us to add or remove a
- // new variable without touching other lines
-
- # Ruby:
- result = make_http_request(
- :method => 'POST',
- :url => api_url,
- :body => '...', // Ruby allows us to leave a trailing comma, making it
- ) // possible to add/remove params while not touching others
- </code></pre>
-
- <p>Why would you want to use such coding styles? Well, always think about the
- person who’s going to <code>git blame</code> this. In the JavaScript example, if you were
- the one who added a committed the value <code>"baz"</code>, you don’t want your name to
- show up when somebody blames the line that added <code>"bar"</code>, since the two
- variables might be unrelated.</p></li>
- </ul>
-
- <h2>Bonus script</h2>
-
- <p>Since you’ve read this far, I’ll reward you with an extra script. I call it
- <a href="https://github.com/mislav/dotfiles/blob/7ac8cbfcd56cfa6c39b5719ea183e87878ea6ed5/bin/git-overwritten">git-overwritten</a> and it shows blame information about original authors of
- lines changed or removed in a given branch:</p>
-
- <pre><code>$ git overwritten feature origin/master
- </code></pre>
-
- <pre class='ansi'> 28 2014-02-04 <span class='ansi-0 ansi-yellow'>1fb2633</span> <span class='ansi-0 ansi-green'>Mislav Marohnić</span>: Add Makefile for building and testing
- 1 2014-01-13 <span class='ansi-0 ansi-yellow'>b2d896a</span> <span class='ansi-0 ansi-green'>Jingwen Owen Ou</span>: Add -t to mktemp in script/make
- 17 2014-01-07 <span class='ansi-0 ansi-yellow'>385ccee</span> <span class='ansi-0 ansi-green'>Jingwen Owen Ou</span>: Add script/make for homebrew build
- </pre>
-
- <p>This is useful when opening pull requests per <a href="http://guides.github.com/overviews/flow/" title="Lightweight, branch-based workflow that supports teams and projects where deployments are made regularly">GitHub Flow</a>; you’ll want your
- pull request reviewed by colleagues but you might not be sure who to ping. With
- <code>git-overwritten</code> you’ll get the names of people who wrote the lines you just
- changed, so you’ll know who to @-mention when opening a pull request.</p>
|