How strict are you when doing code reviews?

A colleague sends you to review a code change. Your approval is required to allow submitting code to the system. You can comment on any fragment of the code that you don’t agree with / don’t like.

How picky should you be? Which of the following would you comment on, and which of the following would you let go through (the list below has no particular order):

I) A local string variable used twice (to represent a date) is named as d but not date

II) A private function has a boolean input argument;

III) A public function has a boolean input argument;

IV) A function implementation is getting long (about 25 lines of code);

V) A function implementation is long (about 100 lines of code);

VI) The test does not exercise a function through the public API;

VII) The function implementation does not have 100% test coverage, even if it’s realistic to achieve 100%;

VIII) 5 tests for the function are getting slow (e.g. 3 min. to run them all on one core);

IX) The function documentation uses abbreviations and in team jargon, which will not be understood by other teams;

X) The function is not tested at all;

XI) The function implementation is not correct for 10% of the input;

XII) The code does not compile

XIII) The function API starts having too many input arguments (4)

XIV) The function API has too many input arguments (8)

XV) The function implementation has duplicate code

XVI) The function implementation has a double nested if statement:

if (some_condition) { if (some_other_condition) { ... } } 

XVII) The function implementation has a triple nested if statement

XVIII) The function has a side effect, even if needs not to have

XIX) A for loop is written as (for i = 0; i < list.size(); i++) instead for (for element : list)

XX) The function returns the output by mutating the input (i.e. input and output variables are not separated)

XXI) A local variable is assigned once and then reassigned

XXII) A binary flag is defined in the library code, the library function relies on that flag directly

XXIII) Any other examples?

submitted by /u/mercury0114
[link] [comments]

from Software Development – methodologies, techniques, and tools. Covering Agile, RUP, Waterfall + more! https://ift.tt/D6KaJni

Leave a comment

Design a site like this with WordPress.com
Get started
search previous next tag category expand menu location phone mail time cart zoom edit close