0

I am reviewing a big C# code base and thought on how to mark it as reviewed when it is, in order to not review it twice.

Currently I have a ReviewedAttribute which I use to decorate members that have been reviewed:

public class ReviewedAttribute : Attribute
{
}

That approach however, misses an important aspect:

  • How to find code that hasn't been reviewed yet ?

Easy you'll tell me, a code block that doesn't have it hasn't been reviewed ! The problem with this approach is that you'd have to open each file to see if the attribute is in it ...

I could also add a NotReviewed attribute on each place that hasn't been reviewed yet but it's an extremely tedious approach as well ...

Basically I'm looking for an approach like XML documentation does when it's enabled : a warning issued for each place that hasn't been documented.

Since if I haven't found such a way, I'm starting to think this is not the best approach.

Question:

Is there an effective strategy to mark blocks of code as reviewed ?

10
  • 3
    Usually, you want to review commits, not blocks of code.
    – Euphoric
    Jul 17, 2017 at 19:22
  • @Euphoric ...except when you have large amounts of unreviewed code, in which case reviewing blocks of code is a good idea before changing to the saner strategy, reviewing commits.
    – juhist
    Jul 17, 2017 at 19:23
  • Actually I thought about commits but it's not really practical, rather I review entire topics, i.e. not only some changes.
    – aybe
    Jul 17, 2017 at 19:25
  • 1
    There are free tools to do it (and paid). Edit: I do not list them because that goes against the Meta rules (I thought) about asking for tools. I saw this, marketplace.visualstudio.com/…
    – johnny
    Jul 17, 2017 at 19:31
  • 6
    "Currently I have a ReviewedAttribute which I use to decorate members that have been reviewed:" This breaks development integrity. Who reviews the reviewer? Changed code must be tested. A code review should add nothing to a code file. All changes, no matter how trivial, must be referred back to the coder. Tomorrow it'll be adding comments. Then "cleaning up" a trivial if statement. Where's the feedback loop to the original coder?
    – radarbob
    Jul 18, 2017 at 4:37

3 Answers 3

2

You could use the #warning directive to alert you to code that still needs to be reviewed, but the downside is that you need to mark every file with this. Fortunately, it would be pretty easy to write a little script to append a line at the end of each *.cs file in your project.

Other than that, take John Wu's advice and remove it after it's been reviewed. The advantage over a plain comment and grep is you'll get a bit of help from the IDE and compiler. You'll get zero false positives this way.

Note: Append it to the end because the file will still compile if you put it at the end. I don't think it will compile if you put it at the top.

1
  • 1
    Even though I don't have per method granularity with this approach, it allows me to jump to a file in the error list, which is a nice feature !
    – aybe
    Jul 17, 2017 at 23:27
3

Given that this question pertains to "catch up" reviews, not ongoing reviews (which should be done as a matter of course on checkins/changesets, not files), I would actually invert this problem. I don't care if a code file has been reviewed-- after all, that still doesn't mean I don't need to review it again! Instead, I care if a file has never been reviewed.

So:

  1. Add a comment to every single code file that is pending review. The comment could be something like /** Pending baseline code review **/

  2. When you review a code file, remove the comment.

  3. To find files that still need a review, search for the comment.

  4. When all the comments are gone, your initial/baseline code review is complete

2

I would simply use a comment having a special marker string that is unlikely to occur elsewhere indicating the entire file has been reviewed, and then grep as the tool to find non-reviewed files. However, if you're using Windows, then you may lack grep. The grep option -L will print files without match.

And about reviewing files vs commits: you do want to at some point of time switch to the strategy of reviewing every single change to the repository. It's much safer that way, and also less work: if you review a file, somebody changes it, then you need to review it again.

2
  • Actually that's a pretty darn good idea !!! I have grep and tested it, does it jobs, it'd be perfect if I could expand it per method to not get false positives (another regex nightmare :).
    – aybe
    Jul 17, 2017 at 19:39
  • <remarks>reviewed</remarks> ?
    – Laiv
    Jul 17, 2017 at 20:55

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.