{"id":2180,"date":"2016-03-07T00:00:00","date_gmt":"2016-03-07T00:00:00","guid":{"rendered":"https:\/\/test.simple-talk.com\/uncategorized\/the-zen-of-code-reviews-review-as-if-you-own-the-code\/"},"modified":"2023-09-27T20:17:33","modified_gmt":"2023-09-27T20:17:33","slug":"the-zen-of-code-reviews-review-as-if-you-own-the-code","status":"publish","type":"post","link":"https:\/\/www.red-gate.com\/simple-talk\/development\/dotnet-development\/the-zen-of-code-reviews-review-as-if-you-own-the-code\/","title":{"rendered":"The Zen of Code Reviews: Review As If You Own the Code"},"content":{"rendered":"<ol>\n<li>The Zen of Code Reviews: Deep Digging\n<ol>\n<li><a href=\"#Toc443117356\">Approaching a Code Review <\/a><\/li>\n<li><a href=\"#Toc443117357\">Review for SOLID Principles <\/a><\/li>\n<li><a href=\"#Toc443117358\">SRP &#8211; Single Responsibility Principle <\/a><\/li>\n<li><a href=\"#Toc443117359\">OCP &#8211; Open Closed Principle <\/a><\/li>\n<li><a href=\"#Toc443117360\">LSP &#8211; Liskov Substitution Principle <\/a><\/li>\n<li><a href=\"#Toc443117361\">ISP &#8211; Interface Segregation Principle <\/a><\/li>\n<li><a href=\"#Toc443117362\">DIP &#8211; Dependency Inversion Principle <\/a><\/li>\n<\/ol>\n<\/li>\n<li><a href=\"#Toc443117363\">Review the Code&#8230; And Everything Else <\/a><\/li>\n<li><a href=\"#Toc443117365\">Look for the code equivalent of tautologies<\/a><\/li>\n<li><a href=\"#Toc443117366\">Less magic usually means less maintenance<\/a><\/li>\n<li><a href=\"#Toc443117367\">Look beyond the changes<\/a><\/li>\n<li><a href=\"#Toc443117368\">Is the code self-documenting?<\/a><\/li>\n<li><a href=\"#Toc443117369\">Eschew building semantic structures by cobbling text together<\/a><\/li>\n<li><a href=\"#Toc443117370\">Conclusion<\/a><\/li>\n<\/ol>\n<p>Before leaping into this new installment in <b>The Zen of Code Reviews<\/b> series, here is a quick recap of the previous articles:<\/p>\n<ul>\n<li><a href=\"http:\/\/www.simple-talk.com\/dotnet\/.net-framework\/the-zen-of-code-reviews-pre-review-comments\/\">Pre-Review Comments<\/a> and <a href=\"http:\/\/www.simple-talk.com\/dotnet\/.net-framework\/the-zen-of-code-reviews-best-practices\/\">Best Practices<\/a> focused on techniques to improve a code review when you wear your <b> <i>author<\/i><\/b> hat, i.e. you write the code to be reviewed by your colleagues.<\/li>\n<li><a href=\"https:\/\/www.simple-talk.com\/dotnet\/.net-framework\/the-zen-of-code-reviews-the-reviewer's-tale\/\">The Reviewer&#8217;s Tale<\/a> considered the opposite perspective, wearing your <b> <i>reviewer<\/i><\/b> hat; however, it told only half the story.<\/li>\n<\/ul>\n<p>The Reviewer&#8217;s Tale focused on the question &#8220;Does the code you are reviewing accurately and completely cover the requirements it allegedly addresses?&#8221; It did this by describing techniques to analyze the unit tests provided in the code review, largely ignoring the production code that those unit tests are testing! (If you practice TDD, then you likely are fine with that; if not, you might think it peculiar. I encourage you to go look at that article and see if my arguments make sense to you.) However, in no way was that article advocating that we should \u00a0ignore production code, \u00a0just that it should not be the first thing you look at. This article is the natural follow-on to that narrative, by discussing techniques to evaluate the production code itself. In one sense, it is less important than the material covered in the previous article, as it is &#8220;just&#8221; discussing the implementation details. After all, one could argue, if the unit tests prove that the code substantially covers all the necessary requirements that the code is supposed to cover, does it really matter whether the production code might be a bit scruffy-looking? To which I would respond with an emphatic &#8220;Yes!&#8221; If the code is too fragile, too inefficient, or just plain too confusing, it could be difficult or impossible to maintain subsequently. Matt Lacey, in his recent blog post <a href=\"https:\/\/dzone.com\/articles\/5-tips-on-working-with-technical-debt-matt-lacey\"> 5 Tips On Working With Technical Debt<\/a> , distilled a lot of what I was going to suggest \u00a0into this single sentence: &#8220;The person who signs off on a review agrees that they will be able to support it in [the] future, should the original author not be available.&#8221;<\/p>\n<p>Right: <i>You<\/i> have been asked to do a code review; the guidelines in this article will make you more effective at being able to sign off and <i>own<\/i> that code.<\/p>\n<h2 id=\"Toc443117356\">Approaching a Code Review<\/h2>\n<p>Your goal, then, is clear: question, probe, analyze, poke, and prod to make sure that <i>you<\/i>, the reviewer, could support the code presented to you for review. From an overall perspective, there are several questions to keep in mind as you begin your task:<\/p>\n<p>Has the author provided an issue\/ticket reference? With few exceptions, all code changes should have an associated ticket-even technical debt. If there was not even sufficient cause to justify putting an item on your task board, should the code change even be merited?<\/p>\n<p>With the ticket in hand, your first job as a reviewer is to review that ticket! You need to have the same understanding of the overall issue as the code author had going in. If there are parts that are not clear, ask for clarification (which could perhaps be done as the first volley in your code review feedback).<\/p>\n<p>Next, do an initial pass of scanning the entire code review, akin to skimming the table of contents when you open a book. Browse the list of files so that you have an idea of the scope, including which projects\/modules are being changed. Browse the code itself so you see the extent of the changes, perhaps gauging that, though there are many changes, much of it is &#8220;noise&#8221; due to simple refactoring. As you do this initial pass, the real goal is to answer this: is every change in the code review in support of the given issue\/ticket? If it is not clear that it is related, question it!<\/p>\n<p>Also in the initial pass, determine whether the code review is a <i>new<\/i> code review or a <i>revised<\/i> code review. (In my experience when there are good code reviews sent out and good feedback returned, it is not uncommon to see a revised code review, incorporating the initial reviewer feedback.) Typically, a revised code review includes all the changes you saw in the last code review-because nothing has been checked in yet-along with a few tweaks here and there.\u00a0 Kudos to a code author for taking the initiative to do a second round, but keep up your diligence as a reviewer: has the author clearly indicated what has changed in the changes? If not, question it! To know what is new in the second code review, you should not have to either (a) remember what you saw in the first code review, or (b) flip back and forth to the original code review. The author should be able to specifically point out what to look at so you do not have to revisit the entire code review when you&#8217;ve likely already reviewed 90% of it.<\/p>\n<p>As you then undertake the second pass, an in-depth review of everything, it is your responsibility to make sure that you are clear about what each particular change is actually doing. Remember that you are (hypothetically) going to take over the task of \u00a0maintaining this code! If you are not clear, question it. A given change might very well be necessary, as part of the ticket, but it is the author&#8217;s job to make that plain to you. Look for comments on <i>why<\/i> things were done rather than <i>what<\/i> things were done. For example, &#8220;Moved all the caching into the execute method as shown&#8221; might help explain <i>what<\/i> you&#8217;re looking at, but ultimately you need to know <i>why<\/i> or <i>how<\/i> that supports the ticket at hand.<\/p>\n<p>If the code review includes UI changes, perhaps HTML or CSS, it should be mandatory to get an annotated screen shot showing not just how the code with changes now renders on screen but also <i>what changed<\/i> on screen. Often you will have a screen mockup in the original ticket; that&#8217;s certainly necessary&#8211;a good starting point&#8211;but not sufficient! You need an actual screen shot of what the author&#8217;s changes produce. If you have been supplied no screen shot and\/or no commentary on the rendering changes, question it! As a simple example, here is a typical CSS change that is typical of code reviews I have been sent (the left side is the original code; the right side is the revised code):<\/p>\n<p><img decoding=\"async\" src=\"https:\/\/www.red-gate.com\/simple-talk\/wp-content\/uploads\/imported\/2380-img13.jpg\" alt=\"2380-img13.jpg\" \/><\/p>\n<p>That&#8217;s it: no accompanying screen shot; no comments in the file; no review comments attached explaining the what or the why of this change. This is from a fairly substantial project so it is <i>not<\/i> reasonable to expect that each reviewer would know offhand what elements those CSS classes were referring to. Now look at the same thing again with some <a href=\"https:\/\/www.simple-talk.com\/dotnet\/.net-framework\/the-zen-of-code-reviews-pre-review-comments\/\"> pre-review commentary<\/a> attached by the author:<\/p>\n<p><img decoding=\"async\" src=\"https:\/\/www.red-gate.com\/simple-talk\/wp-content\/uploads\/imported\/2380-img14.jpg\" alt=\"2380-img14.jpg\" \/><\/p>\n<p>Notice how the text is carefully worded to provide sufficient context for evaluating the code change without requiring the reviewer to study the HTML that the CSS relates to: right here it mentions that the removed specifiers were lower level and the added specifier is higher level. It mentions that the author originally needed to add a third specifier but came up with a better approach, actually reducing the code while making it do more. This code could now be reviewed standalone. The point once again is this: if you have no screen shot and\/or no commentary on the rendering changes and\/or no code comments, question it!<\/p>\n<h2 id=\"Toc443117357\">Review for SOLID Principles<\/h2>\n<p>There are many things you might look at when you review code, from simple stylistic issues (consistent indents, always using braces around the target of a conditional even if just a single statement, correct casing for classes &amp; variables, etc.) to code inefficiencies (code duplication, algorithm choices, performance issues) to principles of code design. It would be well-nigh impossible to provide a single, all-encompassing checklist that would meet the needs of all developers and all code bases. Rather, consider this article as being a starting point. This section considers important code design principles and the final section presents ideas on how to think beyond the code review proper to help you maintain a cleaner code base. (Note that some few examples use C# but that is only cosmetic; the principles apply to any language.)<\/p>\n<p>Let&#8217;s start with a common set of class design principles referred to by the SOLID acronym, coined by Michael Feathers to be a handy mnemonic for the first five principles of dependency management introduced by &#8220;Uncle Bob&#8221; in his article <a href=\"http:\/\/butunclebob.com\/ArticleS.UncleBob.PrinciplesOfOod\">The Principles of OOD<\/a> (object-oriented design).\u00a0 Each letter of that acronym expands into its own acronym:<\/p>\n<ol>\n<li><strong>S<\/strong> R P &#8211; Single Responsibility Principle<b><\/b><\/li>\n<li><b>O <\/b> C P &#8211; Open Closed Principle<b><\/b><\/li>\n<li><b>L <\/b> S P &#8211; Liskov Substitution Principle<b><\/b><\/li>\n<li><b>I <\/b> S P &#8211; Interface Segregation Principle<b><\/b><\/li>\n<li><b>D <\/b> I P &#8211; Dependency Inversion Principle<\/li>\n<\/ol>\n<p>Uncle Bob goes into each of these in detail in the referenced article, so I won&#8217;t repeat that here, but to get the most out of this section you should have some familiarity with the SOLID principles. Here we examine how to recognize violations of each principle, the discovery of which would indicate code that could be made cleaner, more robust, or more maintainable.<\/p>\n<p>(Illustrations in this section are from Derick Bailey&#8217;s <a href=\"https:\/\/lostechies.com\/derickbailey\/2009\/02\/11\/solid-development-principles-in-motivational-pictures\/\"> SOLID Development Principles &#8211; In Motivational Pictures<\/a> and reproduced here under terms of <a href=\"http:\/\/creativecommons.org\/licenses\/by-sa\/3.0\/us\/\">CC BY-SA 3.0 US<\/a>.)<\/p>\n<h3 id=\"Toc443117358\">SRP &#8211; Single Responsibility Principle<\/h3>\n<div class=\"float-left\"><img decoding=\"async\" src=\"https:\/\/www.red-gate.com\/simple-talk\/wp-content\/uploads\/imported\/2380-img17.jpg\" alt=\"2380-img17.jpg\" \/><\/div>\n<p>SRP states: <i>A class should have only one reason to change.<\/i> Where to look? Start with classes that have the most number of methods and dependencies. Class coupling can generally point you to such classes, and you can get the class coupling factor from Visual Studio&#8217;s code analysis tools. In VS2015 use <b>Run Code Analysis<\/b> to see just the items considered suspect by the default analysis rule set, or use <b>Calculate Code Metrics<\/b> to see the class coupling for every class. Using the former any violations of the default rule set will show up in the standard Errors window as warnings; in this case <i>CA1506: Avoid excessive class coupling<\/i> is the item to look for.\u00a0 High class coupling is reflected by owning too many dependencies, i.e. instantiating too many concrete objects within a class.<\/p>\n<p>Note that a class having &#8220;a single reason to change&#8221;:<\/p>\n<ul>\n<li>&#8230;is <i>less<\/i> restrictive, in a sense, than having a single reason to exist; see Derick Bailey&#8217;s excellent discussion of this about a third of the way through his <a href=\"http:\/\/www.codemag.com\/article\/1001061\">S.O.L.I.D. Software Development, One Step at a Time<\/a> , wherein he posits that if taken to the extreme, this would mean &#8220;you may end up with only one method per class.&#8221;<\/li>\n<li>&#8230;is <i>more<\/i> restrictive than handling a single object or purpose; in Patkos Csaba&#8217;s <a href=\"http:\/\/code.tutsplus.com\/tutorials\/solid-part-1-the-single-responsibility-principle--net-36074\"> SOLID: Part 1 &#8211; The Single Responsibility Principle<\/a> he presents an example of a seemingly reasonable book class with functions that get the title, get the author, turn the page, and print the page. Dividing along the lines of actors, though, there are two very different actors (book management and data presentation). As it is actors that provide &#8220;reasons to change&#8221; there are two here, which is non-optimal.<\/li>\n<\/ul>\n<h3 id=\"Toc443117359\">OCP &#8211; Open Closed Principle<\/h3>\n<div class=\"float-left\"><img decoding=\"async\" src=\"https:\/\/www.red-gate.com\/simple-talk\/wp-content\/uploads\/imported\/2380-img1E.jpg\" alt=\"2380-img1E.jpg\" \/><\/div>\n<p>OCP states: <i>A class should be open for extension but closed for modification.<\/i> As Dawson Kroeker states in <a href=\"http:\/\/ig.obsglobal.com\/2014\/02\/refactoring-code-to-meet-solid-design-principles-part-1\/\"> Refactoring Code to Meet SOLID Design Principles &#8211; Part 1<\/a>: &#8220;The priority when enforcing OCP is identifying anticipated places of future change. Fixing violations does not give your project an immediate benefit; it is only helpful when new code is added. You want to anticipate those changes and ensure you have a good design that will allow you to make them easily bug-free!&#8221;<\/p>\n<p>Obvious places to look for OCP violations include occurrences of switch statements or multiple if-then-else statements. Not all such occurrences are bad, of course: But consider what would happen if you were, at some future date, to add another subtype of the base class you are examining: Or what would happen if the gizmo that your class manipulates currently comes in three flavors and you were to add a fourth. If those scenarios were controlled by switch or if statements in the class, then you are almost guaranteed that the class itself would need to be modified, an indication of a fragile, less maintainable design.<\/p>\n<h3 id=\"Toc443117360\">LSP &#8211; Liskov Substitution Principle<\/h3>\n<div class=\"float-left\"><img decoding=\"async\" src=\"https:\/\/www.red-gate.com\/simple-talk\/wp-content\/uploads\/imported\/2380-img25.jpg\" alt=\"2380-img25.jpg\" \/><\/div>\n<p>LSP states: <i>Derived classes must be substitutable for their base classes<\/i>. In a sense LSP overlaps with OCP, in that the easy to identify violation is a switch or if statement that branches on the subtype of a given class. However, as Dawson Kroeker <a href=\"http:\/\/ig.obsglobal.com\/2014\/03\/refactoring-code-to-meet-solid-design-principles-part-2\/\"> points out<\/a>, an active LSP violation will not yet have such a type check in place. So instead, look for subclasses that implement an empty method or a method that throws a <span class=\"Code-inline\"> NotImplementedException<\/span>.<\/p>\n<p>Those are the easy LSP violations to spot. However, there are much more subtle ones as well. Uncle Bob&#8217;s <a href=\"http:\/\/docs.google.com\/a\/cleancoder.com\/viewer?a=v&amp;pid=explorer&amp;chrome=true&amp;srcid=0BwhCYaYDn8EgNzAzZjA5ZmItNjU3NS00MzQ5LTkwYjMtMDJhNDU5ZTM0MTlh&amp;hl=en\"> example<\/a><span class=\"MsoHyperlink\">,<\/span> of a rectangle type and a square type deriving from it, has been deemed the canonical example of this. Essentially, you have a <span class=\"Code-inline\"> Rectangle<\/span> as a base type, and a <span class=\"Code-inline\"> Square<\/span> type that derives from it. You tailor the <span class=\"Code-inline\"> SetHeight<\/span> and <span class=\"Code-inline\"> SetWidth<\/span> methods for the <span class=\"Code-inline\"> Square<\/span> subclass so that each sets <i>both<\/i> the height and the width; albeit somewhat of a code smell, that makes the <span class=\"Code-inline\"> Rectangle<\/span> class usable as a base class for <span class=\"Code-inline\"> Square<\/span>. The main issue here is for a user of the Rectangle class, like this <span class=\"Code-inline\"> CheckRect<\/span> method:<\/p>\n<pre>public void CheckRect(Rectangle r)\r\n{\r\n\u00a0\u00a0\u00a0 r.SetWidth(5);\r\n\u00a0\u00a0\u00a0 r.SetHeight(4);\r\n\u00a0\u00a0\u00a0 Debug.Assert(r.GetWidth() * r.GetHeight()) == 20);\r\n}\r\n<\/pre>\n<p>That last line seems like a perfectly reasonable assertion for a rectangle: after setting the width and height then the rectangle&#8217;s area is the product of the values you specified; but when you pass a square in-perfectly legitimate since <span class=\"Code-inline\"> Square<\/span> inherits from <span class=\"Code-inline\"> Rectangle<\/span>-setting the width to five also sets the height to five. At that point, its area is 25. Next setting the height to four also sets its width, so its area is now 16. Thus, the assertion will fail! So whenever you are using a class that has derived subtypes, make sure that the code is deliberately oblivious to knowing anything about each subtype and that each subtype works.<\/p>\n<h3 id=\"Toc443117361\">ISP &#8211; Interface Segregation Principle<\/h3>\n<div class=\"float-left\"><img decoding=\"async\" src=\"https:\/\/www.red-gate.com\/simple-talk\/wp-content\/uploads\/imported\/2380-img29.jpg\" alt=\"2380-img29.jpg\" \/><\/div>\n<p>ISP states: <i>A client should not be forced to depend on methods it does not use<\/i>. Look for interfaces with lots of methods and chances are you will find an ISP violation. Such a violation will manifest in one or more of the implementations containing methods solely to satisfy the interface. These will-just like for LSP-be empty methods or methods that throws a <span class=\"Code-inline\"> NotImplementedException<\/span>. You might certainly find one or more implementations that use all the methods of the interface. But regardless, if there is at least one implementation that does not, it is an ISP violation. The fix for this is simple: subdivide the single interface into multiple interfaces, then let the clients of the original interface now inherit just those members of the new set of interfaces that they need. This next example is trivial but the principle is evident:<\/p>\n<pre class=\"lang:c# theme:vs2012\">\u00a0\u00a0\u00a0 interface IOriginalInterface\r\n\u00a0\u00a0\u00a0 {\r\n\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0  void methodA();\r\n\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0  bool methodB();\r\n\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0  int methodC();\r\n\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0  int methodD();\r\n\u00a0\u00a0\u00a0 }\r\n\r\n\u00a0\u00a0\u00a0 class X : IOriginalInterface\r\n\u00a0\u00a0\u00a0 {\r\n\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0  public void methodA() { }\r\n\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0  public bool methodB() { }\r\n\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0  public int methodC() { }\r\n\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0  public int methodD() { }\r\n\u00a0\u00a0\u00a0 }\r\n\r\n\u00a0\u00a0\u00a0 class Y : IOriginalInterface\r\n\u00a0\u00a0\u00a0 {\r\n\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0  public void methodA() { throw new  NotImplementedException(); }\r\n\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0  public bool methodB() { }\r\n\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0  public int methodC() { throw new  NotImplementedException(); }\r\n\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0  public int methodD() { throw new  NotImplementedException(); }\r\n\u00a0\u00a0\u00a0 }\r\n\r\n\u00a0\u00a0\u00a0 class Z : IOriginalInterface\r\n\u00a0\u00a0\u00a0 {\r\n\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0  public void methodA() { throw new  NotImplementedException(); }\r\n\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0  public bool methodB() { throw new  NotImplementedException(); }\r\n\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0  public int methodC() { }\r\n\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0  public int methodD() { throw new  NotImplementedException(); }\r\n\u00a0\u00a0\u00a0 }\r\n<\/pre>\n<p>Here you see several client classes that need the <span class=\"Code-inline\"> IOriginalInterface<\/span>-but only a subset of the methods in that interface. Revise the code by refining the granularity of that one interface into three separate interfaces, then let the clients implement only the ones they need:<\/p>\n<pre class=\"lang:c# theme:vs2012\">\u00a0\u00a0\u00a0 interface INewInterface1\r\n\u00a0\u00a0\u00a0 {\r\n\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0  void methodA();\r\n\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0  int methodD();\r\n\u00a0\u00a0\u00a0 }\r\n\r\n\u00a0\u00a0\u00a0 interface INewInterface2\r\n\u00a0\u00a0\u00a0 {\r\n\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0  bool methodB();\r\n\u00a0\u00a0\u00a0 }\r\n\r\n\u00a0\u00a0\u00a0 interface INewInterface3\r\n\u00a0\u00a0\u00a0 {\r\n\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0  int methodC();\r\n\u00a0\u00a0\u00a0 }\r\n\r\n\u00a0\u00a0\u00a0 class X : INewInterface1, INewInterface2, INewInterface3\r\n\u00a0\u00a0\u00a0 {\r\n\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0  public void methodA() { }\r\n\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0  public bool methodB() { }\r\n\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0  public int methodC() { }\r\n\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0  public int methodD() { }\r\n\u00a0\u00a0\u00a0 }\r\n\r\n\u00a0\u00a0\u00a0 class Y : INewInterface2\r\n\u00a0\u00a0\u00a0 {\r\n\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0  public bool methodB() { }\r\n\u00a0\u00a0\u00a0 }\r\n\r\n\u00a0\u00a0\u00a0 class Z : INewInterface3\r\n\u00a0\u00a0\u00a0 {\r\n\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0  public int methodC() { }\r\n\u00a0\u00a0\u00a0 }\r\n<\/pre>\n<h3 id=\"Toc443117362\">DIP &#8211; Dependency Inversion Principle<\/h3>\n<div class=\"float-left\"><img decoding=\"async\" src=\"https:\/\/www.red-gate.com\/simple-talk\/wp-content\/uploads\/imported\/2380-img2E.jpg\" alt=\"2380-img2E.jpg\" \/><\/div>\n<p>DIP states two things:\u00a0 <i>High-level modules should not depend on low-level modules; both should depend on abstractions. Abstractions should not depend on details; rather details should depend on abstractions.<\/i> In general terms, public methods should have parameters that are interfaces rather than concrete classes. Alternately, you might supply a factory as an argument rather than a concrete class. Either way, you want to eliminate the dependency of the receiving class (the high-level module) having to know about a worker object it will use (the low-level module). You can recognize DIP violations by your public methods accepting concrete types, as well as new objects being created within your class. Both of these directly lead to the inability to write full-coverage unit tests for your class. So if the code you are reviewing is missing wide swathes of unit tests, DIP violation is likely the cause of it!<\/p>\n<h2 id=\"Toc443117363\">Review the Code&#8230; And Everything Else<\/h2>\n<p>This final section presents some useful suggestions for improving not just the code that you have been asked to review, but in following the ripples of those code changes, in order to improve the code base as a whole. This is far from an exhaustive list; in fact, it is just barely a beginning! But it is a reasonable beginning.<\/p>\n<h3>Can you get by with less code?<\/h3>\n<p>If you can do something in fewer lines of code, it is easier to maintain, easier to comprehend, and easier to test. Instead of this, for example&#8230;<\/p>\n<pre>var output = new StringBuilder();\r\nfor (var i = 0; i &lt; params.Length; i++)\r\n{\r\n\u00a0\u00a0\u00a0\u00a0 output.Append(params[i].name);\r\n\u00a0\u00a0\u00a0\u00a0 if (i &lt; params.Length - 1) { output.Append(\",\"); }\r\n}\r\nreturn output.ToString();\r\n<\/pre>\n<p>&#8230;suggest the author use this:<\/p>\n<pre>return String.Join(\",\", params.Select(p =&gt; p.name);<\/pre>\n<p>Simplification possibilities can be very easy to miss. Even something as straightforward as this&#8230;<\/p>\n<pre>var user = db.Users.Where(u =&gt; u.UserID == userID).Single();<\/pre>\n<p>&#8230;could be simplified further to:<\/p>\n<pre>var user = db.Users.Single(u =&gt; u.UserID == userID);<\/pre>\n<p>Next, look for repeated chunks of code that could, with a bit of parameterization, be made into a reusable method, reducing code duplication. While you can often find this in the mainline code, in my experience it is even more prevalent in unit test code, where test after test includes the same boilerplate setup.<\/p>\n<p>Finally, could you eliminate a whole method of custom code, and just call something that already exists? That is, are there existing methods (in your code or in the .NET framework) that can do the same thing as a block of code the author has sent you to review? Let&#8217;s say an author sent you code that has a home-brewed HTML encoder, not realizing there were a few alternatives already available in .NET. Suggest instead to use <span class=\"Code-inline\"> HttpUtility.HtmlEncode<\/span>, or <span class=\"Code-inline\"> WebUtility.HtmlEncode<\/span>, or <span class=\"Code-inline\"> System.Security.SecurityElement.Escape<\/span>&#8230; (you get the idea!).<\/p>\n<h3 id=\"Toc443117365\">Look for the code equivalent of tautologies<\/h3>\n<p>You would probably <i> not<\/i> write this from scratch, but it actually did show up in a code review that passed my desk:<\/p>\n<pre>return string.Format(\"{0}\", message);<\/pre>\n<p>How did it get there? The code had gone through multiple revisions where this line started out making effective use of <span class=\"Code-inline\"> String.Format<\/span>. That line got whittled down bit by bit to having only a single parameter-and nothing else in the string. Obviously, it could be simplified to this:<\/p>\n<pre>return message;<\/pre>\n<p>When editing code, in the heat of the moment, such things are easier to slip by than you might think.<\/p>\n<h3 id=\"Toc443117366\">Less magic usually means less maintenance<\/h3>\n<p>Are there magic strings\/numbers present with no explanation? I encountered this one recently:<\/p>\n<pre>var overflowCount = 231;<\/pre>\n<p>I was working on the same project but had no clue what that number was for. If using a &#8220;magic number&#8221; like that, there should at least be a comment on it, but preferably suggest turning it into a named constant that would be self-documenting, e.g.<\/p>\n<pre>var overflowCount = MAX_WIDTH_OF_VIEWPORT_IN_STANDARD_BROWSER;<\/pre>\n<p>In general, be on the lookout for magic values that should, at a minimum, be turned into named constants private to the current class. But also consider if the same magic value appears in other classes, in which case it would need a wider visibility.<\/p>\n<p>There are magic <i> numbers<\/i>, such as that just described. A number is hardly ever self-documenting so that is sufficient reason to replace it with a named constant. There are also magic <i>strings<\/i>. With a string, of course, it is much more possible that it could be self-documenting, but that still does not satisfy the second criteria for eradicating magic values: maintainability. If the code author uses the same hard-coded string constant in more than one place in the code, suggest replacing it with a named constant. Then, in the future should that value need to change, that guarantees it will change everywhere.<\/p>\n<p>Then there are magic <i> Booleans<\/i>. Yes, Booleans! You will often see a method that take a Boolean parameter to effect some behavior in a method. Consider this signature, for example:<\/p>\n<pre>string ConvertText(string input, bool isCaseSensitive);<\/pre>\n<p>Calling that method would frequently be done like this:<\/p>\n<pre>ConvertText(inputString, true);\r\n ConvertText(inputString, false);<\/pre>\n<p>If you hover over that expression in Visual Studio, you could ask Visual Studio to reveal the parameter name, but why should you have to? Suggest that the author rewrite it with named constants making the intent instantly self-evident:<\/p>\n<pre>ConvertText(inputString, CASE_SENSITIVE);\r\n ConvertText(inputString, NOT_CASE_SENSITIVE);<\/pre>\n<h3 id=\"Toc443117367\">Look beyond the changes<\/h3>\n<p>In a code review, you&#8217;re being asked to evaluate very specific chunks of code. The code review tool typically highlights those changed regions nicely for you so you can see the before and after. To do a thorough job, though, you need to consider if there are there any ripple effects caused by the changes in front of you.<\/p>\n<p>Say, for example, a method name was changed from <span class=\"Code-inline\"> GetCurrentImage<\/span> to <span class=\"Code-inline\"> GetFullImage<\/span>. Visual Studio itself, of course, will take care of applying that name change to any callers of the method. If you are using Resharper, it will add an additional vital feature, looking for changes in <i>related<\/i> names (e.g. if you rename an interface from IFoo to IBar it will suggest to also rename the Foo class to the Bar class). However, those tools can only go so far. If you have used that name as the basis for a variable name or a test name, those often need to be changed by hand, e.g. you would probably want to change this&#8230;<\/p>\n<pre class=\"lang:c# theme:vs2012\">var thisCurrentImage = GetFullImage();\r\n\r\n[Test]\r\nvoid SaveImage_stores_current_image_to_repository() { ...}\r\n<\/pre>\n<p>To this&#8230;<\/p>\n<pre class=\"lang:c# theme:vs2012\">var thisFullImage = GetFullImage();\r\n\r\n[Test]\r\nvoid SaveImage_stores_full_image_to_repository() { ...}\r\n<\/pre>\n<p>Beyond the code, It is worth checking whether there are any references to that object name in your API documentation, any &#8220;read me&#8221; files, or even external documentation. You would be surprised what a quick global search of the code base turns up.<\/p>\n<p>Finally, besides just the name, if a public method has changed behavior in any way, has its documentation been updated to reflect that change?<\/p>\n<h3 id=\"Toc443117368\">Is the code self-documenting?<\/h3>\n<p>Latest best practices suggest that comments, if not outright evil, are evil&#8217;s third cousin twice removed. Yes, there are situations where comments are useful and vital, but they should not be used when a simple name change would suffice. So if a method is called <span class=\"Code-inline\"> Transmit<\/span> and has a comment attached &#8220;transmits header in XML format&#8221;, suggest a simple rename, e.g. <span class=\"Code-inline\"> TransmitXmlHeader<\/span>, obviating the need for the comment.<\/p>\n<p>Similarly, are parameters of a new method sufficiently documented? Consider a parameter that specifies a timeout value. If it is called simply <span class=\"Code-inline\"> timeout<\/span>, then the associated comment might say, e.g. &#8220;timeout for operation x in minutes&#8221;. Even better, suggest making the name more enlightening, as in <span class=\"Code-inline\"> timeoutInMinutes<\/span>.<\/p>\n<p>Finally, did you come across a clever bit of code in the review that has the look and feel of coming from an external source? Make sure at a minimum that it has a comment attributing the source (a URL or a publication name, etc.).<\/p>\n<h3 id=\"Toc443117369\">Eschew building semantic structures by cobbling text\u00a0 together<\/h3>\n<p>In my experience, it is very common to see some piece of XML cobbled together as if it were plain text like this:<\/p>\n<pre>var myFooElement = \"&lt;Foo Bar='some value&gt;\" +\r\n \"\u00a0\u00a0\u00a0 &lt;Nested&gt;data&lt;Nested&gt;\"  +\r\n \"&lt;\/Foo&gt;\";<\/pre>\n<p>Did you notice the two errors there? Since it is just text, you are free to mangle the XML any way you please! If you find such abominations (pardon my strong language) suggest firmly that the author build the XML <i>as XML<\/i>, completely avoiding such risks. Here is just one of a variety of ways you could do this:<\/p>\n<pre>var myFooElement = \r\n\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0  new XElement(\"Foo\",\r\n\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0new XAttribute(\"Bar\", \"some value\"),\r\n\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0new XElement(\"Nested\", \"data\"))); <\/pre>\n<p>Another very common example is building file paths. If you see instances like any of these&#8230;<\/p>\n<pre>var path = root + \"\\\\\" + otherPiece + \"\\\\\" + finalPiece;\r\n var path = string.Format(@\"{0}\\{1}\\{2}\", root, otherPiece, finalPiece);\r\n var path = $@\"{root}\\{otherPiece}\\{finalPiece}\";<\/pre>\n<p>&#8230; than suggest instead that the code should do this:<\/p>\n<pre>var path = Path.Combine(root, otherPiece, finalPiece);<\/pre>\n<h2 id=\"Toc443117370\">Conclusion<\/h2>\n<p>Writing code is challenging. Reviewing code is challenging, too. The tips presented above are just a starting point, but they should help you start thinking about \u00a0taking a broader perspective when it comes to doing code reviews.<\/p>\n<p>From this article (along with the other articles in my Zen of Code Review series) you can likely discern my message: <i>Code reviews are important<\/i>. <i> Just as important as writing code itself<\/i>. At my current company I have been evangelizing all the principles and practices I have been writing about and the effort has definitely shown positive results. My team has been able to step up and produce better code as a group, and individually each of us has become a better code designer\/developer.<\/p>\n<p>Reviewing code is more of an art than a science; but, after all, so is writing code. You could say that both are two sides of the same craft, requiring diligent practice to do well. With practice, comes experience. With experience, wisdom. Wherever <i>you<\/i> are on this never-ending journey to enlightenment, here is hoping that this series has been able to provide a few nuggets to guide you!<\/p>\n","protected":false},"excerpt":{"rendered":"<p>A code review is a serious business; an essential part of development. Whoever signs off on a code review agrees, essentially, that they would be able to support it in the future, should the original author of the code be unavailable to do it. Review code with the energy you&#8217;d use if you owned the code. Michael Sorens runs through the principles of reviewing C# code.&hellip;<\/p>\n","protected":false},"author":221868,"featured_media":0,"comment_status":"open","ping_status":"closed","sticky":false,"template":"","format":"standard","meta":{"_acf_changed":false,"footnotes":""},"categories":[143538],"tags":[4143,4229,4371],"coauthors":[6802],"class_list":["post-2180","post","type-post","status-publish","format-standard","hentry","category-dotnet-development","tag-net","tag-net-framework","tag-c"],"acf":[],"_links":{"self":[{"href":"https:\/\/www.red-gate.com\/simple-talk\/wp-json\/wp\/v2\/posts\/2180","targetHints":{"allow":["GET"]}}],"collection":[{"href":"https:\/\/www.red-gate.com\/simple-talk\/wp-json\/wp\/v2\/posts"}],"about":[{"href":"https:\/\/www.red-gate.com\/simple-talk\/wp-json\/wp\/v2\/types\/post"}],"author":[{"embeddable":true,"href":"https:\/\/www.red-gate.com\/simple-talk\/wp-json\/wp\/v2\/users\/221868"}],"replies":[{"embeddable":true,"href":"https:\/\/www.red-gate.com\/simple-talk\/wp-json\/wp\/v2\/comments?post=2180"}],"version-history":[{"count":10,"href":"https:\/\/www.red-gate.com\/simple-talk\/wp-json\/wp\/v2\/posts\/2180\/revisions"}],"predecessor-version":[{"id":98479,"href":"https:\/\/www.red-gate.com\/simple-talk\/wp-json\/wp\/v2\/posts\/2180\/revisions\/98479"}],"wp:attachment":[{"href":"https:\/\/www.red-gate.com\/simple-talk\/wp-json\/wp\/v2\/media?parent=2180"}],"wp:term":[{"taxonomy":"category","embeddable":true,"href":"https:\/\/www.red-gate.com\/simple-talk\/wp-json\/wp\/v2\/categories?post=2180"},{"taxonomy":"post_tag","embeddable":true,"href":"https:\/\/www.red-gate.com\/simple-talk\/wp-json\/wp\/v2\/tags?post=2180"},{"taxonomy":"author","embeddable":true,"href":"https:\/\/www.red-gate.com\/simple-talk\/wp-json\/wp\/v2\/coauthors?post=2180"}],"curies":[{"name":"wp","href":"https:\/\/api.w.org\/{rel}","templated":true}]}}