I Logged Every Time a Senior Developer Said No in Code Review

A normal code review comment usually sounds harmless.

Rename this variable. Move this method. Add a test. Remove the duplicate condition. But sometimes an experienced developer leaves a much shorter comment: No.

Not maybe. Not could we simplify this. Just a clear rejection of the entire approach.

For a junior developer, this can feel strange. The code compiles, tests pass, the implementation is readable, and the ticket requirements seem complete. Why throw it away?

I started saving such cases after one of my pull requests was rejected for the third time in a week. The goal was not to prove that the reviewer was wrong. I simply wanted to understand what experienced engineers noticed before everyone else.

Over several months, I collected review discussions from backend services, internal tools, queue consumers, APIs, and data-processing jobs. I removed comments about formatting and naming. Only full design-level rejections remained.

The result was a small catalogue of professional paranoia.

And honestly, most of it was useful.

The code worked, but only because the system was quiet

The first pattern appeared in code that was technically correct under normal conditions.

One example came from an order-processing service. A handler received a payment event, loaded the order, changed its status, and sent a confirmation email. The implementation looked simple enough.

The reviewer rejected it.

The reason was not visible inside the method. Message brokers may deliver the same event more than once. A consumer may crash after updating the database but before acknowledging the message. The broker then retries the event, and the customer receives another email.

Nothing in the happy-path test showed this.

Here is a simplified version of the original approach.

from dataclasses import dataclassfrom enum import Enumfrom typing import Protocolclass OrderStatus(str, Enum):    CREATED = "created"    PAID = "paid"@dataclassclass PaymentCompleted:    event_id: str    order_id: str    amount_cents: int    customer_email: str@dataclassclass Order:    id: str    status: OrderStatus    paid_amount_cents: int = 0class OrderRepository(Protocol):    def get(self, order_id: str) -> Order:        ...    def save(self, order: Order) -> None:        ...class EmailService(Protocol):    def send_payment_confirmation(        self,        email: str,        order_id: str,        amount_cents: int,    ) -> None:        ...class PaymentHandler:    def __init__(        self,        orders: OrderRepository,        email_service: EmailService,    ) -> None:        self.orders = orders        self.email_service = email_service    def handle(self, event: PaymentCompleted) -> None:        order = self.orders.get(event.order_id)        if order.status == OrderStatus.PAID:            return        order.status = OrderStatus.PAID        order.paid_amount_cents = event.amount_cents        self.orders.save(order)        self.email_service.send_payment_confirmation(            email=event.customer_email,            order_id=event.order_id,            amount_cents=event.amount_cents,        )

At first glance, the status check seems to make the handler idempotent. It does not.

Imagine this sequence:

The order is saved as paid.
The email service receives the request.
The process crashes before the message is acknowledged.
The event is delivered again.
The handler sees the paid status and exits.

That sequence may be acceptable.

Now change the timing slightly:

The email is sent.
The database transaction fails.
The event is retried.
The email is sent again.

Or two workers receive duplicate events at nearly the same time. Both load the order before either one saves it. Both see the created status. Both send the email.

The senior developer was not reviewing one function. He was reviewing all possible interleavings of the system.

That was the first important lesson: working code is not the same as stable behavior.

During review, do you read the method from top to bottom, or do you mentally stop the process after every line? What happens if the application dies there? What happens if another instance executes the same code at the same moment?

After collecting enough comments, it became clear that experienced developers ask these questions almost automatically.

A rejection often meant that ownership was unclear

Another common no appeared when one piece of code knew too much about another part of the system.

This happened in a service that calculated delivery prices. The API layer loaded a customer, checked a subscription type, detected the warehouse region, applied a discount, and finally selected a courier tariff.

The code was readable. It was also the wrong place for almost every decision it made.

The reviewer asked a simple question: which component owns the delivery rule?

Nobody had a good answer.

That was the problem.

When business rules have no clear owner, they slowly spread across controllers, background jobs, command handlers, and SQL queries. The first implementation looks practical because everything is visible in one method. Six months later, three teams calculate the same value differently.

I noticed that experienced reviewers often rejected code before duplication existed. They were reacting to the shape of future duplication.

A junior developer usually asks whether the method works today. A senior developer often asks where the next developer will copy it tomorrow.

That difference is easy to miss.

One review contained a condition similar to this:

If the customer is premium, the warehouse is local, the order is abovea certain amount, and the delivery is not scheduled for Sunday,apply free shipping.

The code itself was not difficult. The danger was that free shipping had become an accidental feature of an HTTP endpoint.

The final version moved the decision into a delivery policy object. The controller became boring, which was a good sign. It received input, called the application service, and returned a response.

Boring boundaries are underrated.

They make it obvious where a rule belongs and where it does not. They also make review discussions less personal. Instead of arguing about whether a condition is readable, the team can ask whether the condition belongs to that module.

Whenever a reviewer says no to a small piece of logic, it may help to ask a broader question: is the code wrong, or is its location wrong?

Quite often, the answer is the second one.

The dangerous abstraction was the one that looked reusable

A surprising number of rejected changes involved helpers, generic repositories, shared utility classes, or universal service wrappers.

The intention was usually good. Someone noticed repeated code and tried to remove it.

Experienced reviewers were often suspicious.

One case involved a generic retry helper for HTTP requests. It accepted any function, retried on exceptions, and used exponential backoff. It looked clean and reusable.

The reviewer rejected it because not every failed operation is safe to repeat.

A GET request is usually repeatable. A request that creates a payment, reserves inventory, or sends a notification may not be. Retrying without understanding the remote API can produce duplicate side effects.

The abstraction removed duplicated code but also removed context.

Here is a simplified version of the safer design that replaced it.

type RetryDecision =  | { retry: false }  | { retry: true; delayMs: number };interface Logger {  warn(message: string, metadata?: Record<string, unknown>): void;}interface IdempotentOperation<T> {  readonly operationName: string;  readonly idempotencyKey: string;  execute(signal: AbortSignal): Promise<T>;  classifyError(    error: unknown,    attempt: number  ): RetryDecision;}class RetryLimitExceededError extends Error {  constructor(    public readonly operationName: string,    public readonly attempts: number,    public readonly lastError: unknown  ) {    super(      `Operation ${operationName} failed after ${attempts} attempts`    );  }}class IdempotentOperationExecutor {  constructor(    private readonly logger: Logger,    private readonly maxAttempts: number  ) {    if (maxAttempts < 1) {      throw new Error("maxAttempts must be at least 1");    }  }  async run<T>(    operation: IdempotentOperation<T>,    signal: AbortSignal  ): Promise<T> {    let lastError: unknown;    for (let attempt = 1; attempt <= this.maxAttempts; attempt++) {      if (signal.aborted) {        throw new DOMException(          "Operation was aborted",          "AbortError"        );      }      try {        return await operation.execute(signal);      } catch (error: unknown) {        lastError = error;        const decision = operation.classifyError(          error,          attempt        );        if (!decision.retry) {          throw error;        }        if (attempt === this.maxAttempts) {          break;        }        this.logger.warn("Retrying idempotent operation", {          operationName: operation.operationName,          idempotencyKey: operation.idempotencyKey,          attempt,          nextDelayMs: decision.delayMs,        });        await this.delay(          decision.delayMs,          signal        );      }    }    throw new RetryLimitExceededError(      operation.operationName,      this.maxAttempts,      lastError    );  }  private async delay(    delayMs: number,    signal: AbortSignal  ): Promise<void> {    await new Promise<void>((resolve, reject) => {      const timer = setTimeout(resolve, delayMs);      const abortHandler = (): void => {        clearTimeout(timer);        reject(          new DOMException(            "Operation was aborted",            "AbortError"          )        );      };      signal.addEventListener(        "abort",        abortHandler,        { once: true }      );    });  }}interface PaymentGateway {  capturePayment(input: {    paymentId: string;    amountCents: number;    idempotencyKey: string;    signal: AbortSignal;  }): Promise<{    transactionId: string;  }>;}class CapturePaymentOperation  implements IdempotentOperation<{ transactionId: string }>{  readonly operationName = "capture-payment";  constructor(    private readonly gateway: PaymentGateway,    private readonly paymentId: string,    private readonly amountCents: number,    readonly idempotencyKey: string  ) {}  async execute(    signal: AbortSignal  ): Promise<{ transactionId: string }> {    return this.gateway.capturePayment({      paymentId: this.paymentId,      amountCents: this.amountCents,      idempotencyKey: this.idempotencyKey,      signal,    });  }  classifyError(    error: unknown,    attempt: number  ): RetryDecision {    if (attempt >= 3) {      return { retry: false };    }    if (      error instanceof TypeError    ) {      return {        retry: true,        delayMs: 250 * 2 ** (attempt - 1),      };    }    if (      error instanceof Error &&      error.message.includes("HTTP 503")    ) {      return {        retry: true,        delayMs: 500 * attempt,      };    }    return { retry: false };  }}

This version is longer. That is not automatically bad.

It forces every retried operation to expose an idempotency key. It gives the operation control over error classification. It supports cancellation. It logs enough context to investigate repeated failures.

Most importantly, it makes an unsafe use difficult.

The rejected generic helper was elegant because it hid complexity. The accepted version was useful because it exposed the complexity that mattered.

After seeing several similar reviews, I stopped treating abstraction as a reward for finding repeated lines. Repetition may indicate shared behavior, but it may also hide different failure modes.

Before extracting a helper, try asking: which important differences will disappear from the call site?

If the answer includes transaction boundaries, authorization, retries, timeouts, ownership, or side effects, a little duplication may be cheaper than a clever abstraction.

Tests were rejected when they proved the implementation instead of the behavior

Another group of no comments appeared in test code.

This was painful because the tests often looked thorough. They used mocks, verified calls, covered branches, and reached a high coverage percentage.

Still, the reviewer rejected them.

The problem was that they described the internal structure of the current implementation rather than the behavior expected by the system.

For example, a test checked that a repository method was called once, a mapper was called once, and an event publisher received a specific object. The production code could not be refactored without rewriting the test, even if its external behavior remained identical.

Meanwhile, the test said nothing about duplicate events, concurrent updates, partial failures, invalid state transitions, or clock boundaries.

That is a strange trade: maximum confidence in method calls, minimum confidence in the actual system.

The most useful senior-review comments did not demand more tests. They demanded tests at a better boundary.

A good test might run the use case against a real database container, process the same message twice, and verify that only one durable state transition exists. Another might simulate a timeout after the remote system has already accepted a request. A third might execute two workers concurrently and check whether the invariant still holds.

These tests are slower and slightly annoying to write. They also catch the bugs that appear at 3 a.m., which gives them a certain charm.

One reviewer used a simple rule: if a test remains green after deleting the important business condition, it is probably not testing the important business condition.

That rule is not perfect, but it is surprisingly effective.

Try it on your own tests. Remove the lock. Remove the idempotency check. Change the transaction boundary. Return stale data. Does the test fail for the reason you expected?

Coverage can show which lines executed. It cannot tell you whether the system property you care about was ever challenged.

Most senior no comments were really questions about time

The strongest pattern appeared only after the notes had grown large enough.

Experienced developers were constantly thinking about time.

Not performance in milliseconds, although that mattered too. They were thinking about how code changes over weeks, how requests overlap over seconds, and how failures interrupt execution between two lines.

A boolean flag may be fine today, but what happens when a third state appears?

A database column may accept null now, but what happens after ten million rows depend on that meaning?

A synchronous request may be easy to debug, but what happens when the downstream service becomes slow?

A local cache may reduce latency, but what happens after deployment when five application instances hold five different answers?

A migration may work on a development database, but what happens while old and new application versions run at the same time?

This is why some review comments can feel unrelated to the ticket. The reviewer is not only evaluating the code in its current moment. The reviewer is mentally placing it inside future deployments, retries, schema changes, incidents, and team handovers.

That skill looks like intuition from the outside. In reality, it is compressed memory from previous failures.

After logging enough examples, I began using a small review routine.

First, read the change as written.

Then read it as if the process can stop after any line.

Then read it as if two copies run at once.

Then read it as if the requirement changes next month.

Finally, read it as if the original author has left the company and the only explanation is the code itself.

This does not turn anyone into a senior engineer overnight. It does make several invisible problems easier to notice.

And perhaps that is the real value of a firm no in code review. It interrupts the pleasant feeling that finished code is actually finished.

What changed in my own reviews

The main change was not becoming stricter.

It was becoming more specific.

Instead of writing this feels risky, I now try to describe the failure sequence.

Worker A reads version 4.
Worker B reads version 4.
Worker A writes version 5.
Worker B writes another version 5 and silently overwrites the first update.

Instead of saying this abstraction is too generic, I point out which domain difference has disappeared.

Instead of asking for more tests, I name the invariant that should survive retries, concurrency, or partial failure.

This also makes review conversations calmer. A vague objection sounds personal. A concrete failure scenario gives everyone something technical to inspect.

Sometimes the original author is right. The scenario is impossible because another layer already guarantees ordering or uniqueness. Good. That guarantee can be documented, tested, or made visible in the code.

Sometimes the reviewer is right, and a bug disappears before reaching production.

Both outcomes are useful.

The notes also changed how I react when an experienced developer rejects an approach. The first thought is no longer that the reviewer misunderstood the implementation. The first question is simpler:

What system behavior are they seeing that I am not seeing yet?

That question has saved more time than defending the first version ever did.

Conclusion

After several months, the collected no comments stopped looking random.

They usually pointed to one of a few problems:

The code was correct only on the happy path.
The business rule had no clear owner.
An abstraction removed important context.
The tests verified structure instead of behavior.
The implementation ignored time, concurrency, or future change.

None of these lessons are new. The interesting part was seeing how consistently they appeared in real reviews.

Experienced developers did not reject more code because they disliked complexity. Quite often, they rejected code because they had learned where complexity hides.

It hides between two database operations.
It hides inside a retry.
It hides behind a reusable helper.
It hides in a state that does not exist yet.
It hides in the sentence saying this can never happen.

So the next time someone leaves a short no on a perfectly reasonable pull request, it may be worth delaying the reply for a few minutes.

The code may be fine.

The system around it may not be.