Przejdź do głównej zawartości

An opinion on code review

Code review is often seen as a way to enhance code quality. My personal views on code review are quite different. I will try to summarize them in a short list. You are welcome to disagree.

  • code review frees the author from the responsibility for the code; "I don't have to care so much, since others are looking at it as well"
  • code review fits very very poorly into Scrum framework; it delays the completion of tasks and backlog items and produces an inclination in teams to aggregate the reviews into a several-days-long "phase" placed at the end of an iteration (making it effectively a wateration)
  • code reviews are much easier as a process to learn than TDD or SOLID, so developers do them gladly and receive in exchange a feeling of accomplishment (unfortunately):
    • "I give my code to others to review, because I care for code quality"
    • "I review the code of others, because they value me as a knowledgeable person"
  • code reviews drag the feedback to the very last moment, instead of asking for feedback early and continuously
Your code can be better without code reviews.


Popularne posty z tego bloga

Unit Testing code with IO file operations (in Python)

We may often come across a piece of code that was written without Unit Tests at all. In addition, the piece of code may be dealing with IO like file writing and reading, which makes it more difficult to Unit Test it when we are trying to refactor and modify. Let's suppose the code in question looks like this:

def writeInitialsToFile(filename, name, surname):
    initials = name[0] + '.' + surname[0] + '.'
    with open(filename, 'w') as file:

def readInitials(filename):
    initials = None
    with open(filename, 'r') as file:
        initials = file.readline()
    return initials

A straightforward and bad idea would be to write a couple of Unit Tests that make use of a real file and simply test the reading and writing. Is therea a better way to test this code?

First of all, we need a way to replace the real file with something else. For both reading and writing we will now have a couple of functions, one that expects a stream fo…

Piotr's Less Obvious Advice on Google Mock: State maintenance

Google Mock provides several ways to maintain state inside mock objects. One way of implementing state maintenance is with SaveArg. Consider the following example.

We have a class Configurator, which allows a caller to set and get values of a parameter:

class Configurator

    virtual ~Configurator() {}

    virtual void setParamX(int n) = 0;
    virtual int getParamX() = 0;

And we have a class Client that calls Configurator's methods and it also has a method incParamXBy, that can be used to increase the current value of paramX by a certain value.

class Client

    Client(Configurator & cfg);
    virtual ~Client() {}

    void setParamX(int n);
    void incParamXBy(int n);
    int getParamX();


    Configurator & _cfg;

incParamXBy internally calls setParamX and getParamX on Configurator:

void Client::incParamXBy(int n)
    _cfg.setParamX(_cfg.getParamX() + n);

Let's assume that the initial value of paramX is A and that we want to increase paramX by…

Piotr's Less Obvious Advice on Google Mock: Returning new objects from a mock

Google Mock provides a way to return newly created objects from a mock method. Suppose we have a  Generator class that is supposed to generate new objects when createNewRecord method is called:

class Generator
    virtual ~Generator() {}
    virtual Record * createNewRecord() = 0;

...and suppose we want to mock this class:

class MockGenerator : public Generator
    MOCK_METHOD0(createNewRecord, Record * ());

Suppose the caller class Client has run method defined as follows:

void Client::run()
    for(int i = 0; i < 3; i++)
        rec_tab[i] = gen.createNewRecord();

We want the mock to return a pointer to a new object each time createNewRecord is called. This is how we can express this in the test code:

TEST(ClientTest, CanRun)
    MockGenerator gen;
    Client c(gen);

    EXPECT_CALL(gen, createNewRecord())
                 //this is equivalent of returning new Record(1,2,3)