Clean code and code reviews

In short

Clean code is source code that is readable, small-stepped, consistently named, easy to change and easy to test; a code review is the human (and tool-assisted) static examination of that code before it is merged. Both exist because software is read far more often than it is written, and because a defect caught in review costs a fraction of the same defect caught in production. University syllabi examine this as *coding standards, program comprehension, walkthroughs, inspections, software metrics and refactoring* under Software Engineering, and GATE-style questions ask for cyclomatic complexity, review types and quality metrics. Every Indian employer with a merge process - TCS, Infosys, Wipro, Cognizant, Zoho, Flipkart, Swiggy, Qualcomm, Samsung R&D, Bosch, Tata Elxsi - gates code on review plus a static-analysis quality gate, and "walk me through how you review a pull request" is now a standard interview question for freshers.

Explained

What "clean code" means

• Definition: code whose intent is obvious on first reading, with no duplication, minimal state, small units and no surprises - it does what its name says and nothing else.
• Readability over cleverness: a one-line nested ternary that needs three readings is worse than four plain if statements. Optimise for the next person who debugs it at 2 a.m.
• Core principles examined:
  • DRY (Don't Repeat Yourself) - one authoritative place for each piece of knowledge; duplication means two places to fix a bug.
  • KISS (Keep It Simple) - simplest construct that solves the actual requirement.
  • YAGNI (You Aren't Gonna Need It) - do not build for imagined future requirements; speculative generality is a smell.
  • Single Level of Abstraction - one function should not mix policy ("approve the loan") with plumbing ("open the socket").
  • Principle of Least Astonishment - a method named getUser() must not also write to the database.
  • Boy Scout Rule - leave the file a little cleaner than you found it; continuous small cleanup instead of a "big rewrite later".
• Quality attributes it serves (ISO/IEC 25010 *maintainability* sub-characteristics): modularity, reusability, analysability, modifiability, testability.
• Cohesion high, coupling low: a module should do one thing (functional cohesion) and know as little as possible about others (data coupling, not content/common coupling).

The clean-code rules: names, functions, comments, formatting, errors

• Names: intention-revealing, pronounceable, searchable. elapsedDays not d; MAX_RETRIES not 3 scattered inline. No type encoding (strName), no single letters except loop counters and maths.
• Conventions: follow the language's own standard - snake_case and PEP 8 in Python, camelCase methods / PascalCase classes in Java (Google/Oracle style), UPPER_SNAKE constants. Consistency inside a repo beats personal preference.
• Functions: small (ideally screen-sized), do one thing, few parameters (0-3; more means a parameter object), no flag arguments, no output parameters, and a single level of abstraction per function. Prefer early-return guard clauses to deep nesting.
• Side effects and purity: separate commands (change state) from queries (return data) - command-query separation. Pure functions are trivially testable.
• Comments: a comment is not a deodorant for bad code. Delete commented-out code (version control remembers it). Keep comments that carry *why*: legal headers, intent, warnings of consequence, non-obvious algorithm references, public API documentation (Javadoc / docstrings).
• Formatting: one statement per line, consistent indentation, vertical grouping of related code, line length limit (79-120 chars per the house style), and an auto-formatter so formatting is never a review topic - Black/isort (Python), Prettier (JS), google-java-format, clang-format.
• Error handling: throw exceptions rather than return error codes; never swallow an exception with an empty catch; do not return or pass null where an empty collection or Optional works; use try-with-resources/with so resources always close. Validate at the boundary, trust inside.
• Magic numbers and strings: replace with named constants or configuration; hard-coded values are an exam favourite as a maintainability defect.
• Dead code: unused variables, unreachable branches, obsolete flags - remove them; they confuse readers and inflate test effort.

Code smells, refactoring and the design principles behind them

• Code smell: a surface symptom suggesting a deeper design problem. It is not a bug - the program works - but it raises the cost of the next change.
• Common smells (Fowler's catalogue): Long Method, Large Class (God object), Long Parameter List, Duplicated Code, Feature Envy, Data Clumps, Primitive Obsession, Switch Statements on type, Shotgun Surgery (one change touches many files), Divergent Change, Message Chains (a.getB().getC().getD()), Middle Man, Comments-as-apology, Speculative Generality.
• Refactoring: changing internal structure without changing external behaviour. Precondition: a passing test suite - refactoring without tests is rewriting.
• Standard refactorings: Extract Method / Extract Variable, Inline Method, Rename, Move Method/Field, Extract Class, Replace Magic Number with Constant, Replace Conditional with Polymorphism, Replace Nested Conditional with Guard Clauses, Introduce Parameter Object, Decompose Conditional, Encapsulate Field.
• Red-green-refactor is the test-first rhythm; refactoring itself is also done on legacy code behind existing tests.
• SOLID, as applied to clean code: Single Responsibility (one reason to change), Open-Closed (extend without editing), Liskov Substitution (a subtype must honour the base contract), Interface Segregation (no fat interfaces), Dependency Inversion (depend on abstractions). Law of Demeter limits message chains.
• Limitation: refactoring consumes schedule with no visible feature; it must be budgeted explicitly, and large-scale restructuring needs a characterisation-test safety net first.

Measuring code quality: metrics, static analysis, technical debt

• Size: LOC / KLOC, SLOC excluding blanks and comments; comment density. Crude but used for review rate and defect density.
• Cyclomatic complexity V(G) (McCabe): number of linearly independent paths; guideline ≤10 per function, 11-20 risky, >20 refactor. It also equals the minimum number of tests for branch path coverage.
• Cognitive complexity (SonarQube): penalises nesting and breaks in flow rather than counting branches - closer to perceived difficulty.
• Halstead metrics: from operators/operands - vocabulary, length, volume V, difficulty, effort.
• Maintainability Index: composite of Halstead volume, complexity and LOC (formula below).
• Object-oriented metrics (CK suite): WMC (weighted methods per class), DIT (depth of inheritance tree), NOC (number of children), CBO (coupling between objects), RFC (response for a class), LCOM (lack of cohesion of methods). Martin metrics: afferent/efferent coupling, instability, abstractness.
• Static analysis (no execution): linters and analysers - pylint/flake8/mypy, SpotBugs/PMD/Checkstyle/Error Prone, ESLint, clang-tidy, cppcheck, SonarQube/SonarLint, Semgrep, CodeQL. They catch null dereferences, resource leaks, unused code, injection risks, style violations. Limitation: false positives and no knowledge of business intent - they cannot replace a reviewer.
• Dynamic analysis (execution-based: profilers, Valgrind, sanitisers) complements it. Review and static analysis are verification ("are we building it right"), not validation.
• Technical debt: the future cost of a shortcut taken today - deliberate-prudent, deliberate-reckless, inadvertent-prudent, inadvertent-reckless (Fowler's quadrant). "Interest" is paid as slower changes and more defects.
• Quality gate: a CI rule that fails the build on new code - e.g. zero new blocker/critical issues, duplication under 3%, technical-debt ratio under 5%, coverage on new code above a threshold. This is how review standards get enforced mechanically in Indian delivery organisations.

Code reviews: types under IEEE 1028 and the review process

• IEEE 1028 (Standard for Software Reviews and Audits) classifies five kinds:
  1. Management review - progress, risk, resources; decided by managers.
  2. Technical review - peers (and experts) judge fitness for purpose against specifications.
  3. Inspection - formal, trained moderator, defined roles, entry/exit criteria, checklists, logged defects and metrics (Fagan inspection).
  4. Walkthrough - the author leads, explains the code, the team raises questions; good for knowledge transfer and training.
  5. Audit - independent evaluation of compliance with standards, contracts and regulations.
• Informal/peer desk-check and pair programming sit below these: cheapest, continuous, no records.
• Roles: author, moderator/facilitator (never the author), reviewers/inspectors, scribe/recorder, reader (paraphrases code in Fagan inspections), manager (planning only, outside the defect meeting).
• Formal review process (ISTQB/IEEE sequence): planning (scope, entry criteria, checklists) → initiation/kick-off (distribute material) → individual preparation (the step that finds most defects) → review/inspection meeting (log defects, do not solve them) → rework by the author → follow-up and exit-criteria check by the moderator.
• Golden rule of the meeting: review the product, not the person; find defects, do not design fixes; cap the sitting at 60-90 minutes and 200-400 lines because yield collapses after that.
• Fagan inspection stages: planning, overview, preparation, inspection, rework, follow-up; its measured defect-removal efficiency is typically 50-70%, far higher than ad-hoc review.
• Static vs dynamic: reviews find defects testing cannot see - unreachable requirements, missing error handling, bad abstractions, maintainability and standards violations - and find them earlier, when fixing is cheap.
• Limitations: needs discipline and reviewer time; can degrade into rubber-stamping ("LGTM" in 30 seconds); reviewer fatigue on large diffs; interpersonal friction if feedback is phrased as criticism of the author.

Modern pull-request review in practice

• Flow: small branch → self-review the diff → open a pull/merge request with context (what, why, how tested, ticket id) → CI runs build, tests, linters, security scan → one or two reviewers comment → author pushes fixes → approval → squash/merge. Gerrit (used widely in telecom and silicon R&D) adds mandatory +2 review scores before submit.
• Keep diffs small: under ~400 changed lines; one logical change per request. Big-bang reviews get shallow reviews.
• Review checklist (what a reviewer actually looks for): correctness against the requirement; edge cases and error paths; naming and readability; duplication; tests present and meaningful; security (input validation, injection, secrets in code, authentication/authorisation); performance (N+1 queries, loops over network calls); concurrency (shared mutable state); logging without personal data; backward compatibility and migrations; documentation.
• Comment etiquette: be specific and about the code ("this loop runs a query per row; can it be a single IN query?"); label severity - blocking issue, suggestion, or nit (trivial); ask questions instead of issuing verdicts; praise good solutions; let the author decide among equivalent styles. Authors: assume good intent, reply to every comment, never take it personally.
• Automate the boring half: formatting, import order, line length, coverage and style go to pre-commit hooks and CI so humans spend their time on logic and design.
• Definition of Done in Agile teams usually includes "reviewed and approved" - unreviewed code is not done.

Standards, compliance and Indian industry practice

• Coding standards are organisation- or domain-mandated: MISRA C/C++ and AUTOSAR C++14 guidelines in automotive (Bosch, KPIT, Tata Elxsi, Tata Motors suppliers) under ISO 26262 functional safety; DO-178C for airborne software (HAL, aerospace suppliers); IEC 62304 for medical device software; CERT C and OWASP ASVS for security.
• Process frameworks: CMMI (Verification and Validation process areas require peer reviews with recorded data), ISO 9001/ISO 90003, Automotive SPICE. Indian service majors appraised at CMMI high maturity levels maintain peer-review records, review-effort norms and defect-removal data per project.
• Security code review is a compliance requirement in Indian finance and public digital infrastructure - RBI cyber-security framework expectations for banks, CERT-In directions on reporting, UIDAI/NPCI integration audits, SEBI system audits; application security testing and secure code review before go-live are routine, with the Information Technology Act, 2000 (ss. 43A/72A, and the Digital Personal Data Protection Act, 2023) making careless handling of personal data a legal exposure, so reviewers flag logging of Aadhaar numbers, card data or phone numbers.
• Exact clauses and threshold numbers vary by employer, standard version and regulator circular - treat the ranges above as typical industry guidance, not fixed law.
• Where reviews appear in placement tests: campus aptitude/technical rounds give a code snippet and ask for complexity, smells or a refactoring; some companies (Zoho, Thoughtworks-style) run a pair-programming or code-critique round.

Key points

• Clean code - readable, non-duplicated, small-unit code whose intent is obvious; optimised for the reader, not the writer.
• DRY / KISS / YAGNI - no duplicated knowledge, simplest construct, no speculative features.
• Meaningful names - intention-revealing, searchable, language-conventional; no magic numbers.
• Small functions - one thing, one level of abstraction, 0-3 parameters, guard clauses instead of deep nesting.
• Comments explain why, not what - code that needs a comment to be understood usually needs renaming or extracting.
• Code smell - a symptom (Long Method, God class, Duplicated Code, Feature Envy, Shotgun Surgery) of deeper design trouble, not a bug.
• Refactoring - behaviour-preserving structural change; requires passing tests first.
• Cyclomatic complexity V(G) - independent paths = decisions + 1; keep ≤10; equals minimum branch-path tests.
• Cohesion high, coupling low - the single most reused design heuristic in reviews.
• SOLID - single responsibility, open-closed, Liskov substitution, interface segregation, dependency inversion.
• IEEE 1028 review types - management review, technical review, inspection, walkthrough, audit.
• Walkthrough vs inspection - author-led and informal vs moderator-led, checklist-driven, metric-recording and formal.
• Preparation finds the defects - the inspection meeting logs them; it must not try to fix them.
• Review limits - ≤400 LOC per sitting, ≤60-90 minutes, 150-300 LOC/hour; beyond that defect-finding falls sharply.
• Reviews are verification (static) - they catch standards, maintainability and missing-case defects that no test run can see.
• Technical debt - accumulated shortcut cost; measured as technical-debt ratio (remediation effort ÷ development effort).
• Quality gate - CI rule failing the build on new critical issues, duplication or debt; makes standards non-negotiable.
• Review the artefact, not the author - specific, severity-labelled, question-shaped feedback; automate formatting disputes away.

Formulas and examples

Example 1 - cyclomatic complexity of a function, both ways

def grade(marks, attendance):
    if marks is None:              # d1
        return "INVALID"
    if attendance < 75:            # d2
        return "DETAINED"
    if marks >= 90:                # d3
        return "A"
    elif marks >= 75:              # d4
        return "B"
    elif marks >= 40:              # d5
        return "C"
    else:
        return "F"

• Predicate count: 5 binary decisions → V(G) = 5 + 1 = 6.
• Control-flow graph: 12 nodes (5 decision nodes, 6 return nodes, 1 exit), 16 edges → V = E − N + 2P = 16 − 12 + 2(1) = 6. Both methods agree; 6 independent paths means 6 tests for branch path coverage.

Example 2 - refactoring the same function and re-measuring

BANDS = ((90, "A"), (75, "B"), (40, "C"))      # cut-offs are now data

def grade(marks, attendance):
    if marks is None:
        return "INVALID"
    if attendance < 75:
        return "DETAINED"
    return band_for(marks)

def band_for(marks):
    for cutoff, band in BANDS:
        if marks >= cutoff:
            return band
    return "F"

• grade: 2 decisions → V = 3. band_for: loop + if → V = 3.
• No unit exceeds the ≤10 guideline, adding a new grade band is a data edit with zero code change, and each function is independently testable. Behaviour is unchanged - that is what makes it a refactoring.

Example 3 - review effort, yield and rupee ROI A release changes 9,000 LOC. House norm: 2 reviewers per change, effective review rate 200 LOC/hour, loaded engineer cost ₹900/hour. Pre-review defect density 12 defects/KLOC; review DRE 55%. Fixing a defect in review ≈ ₹1,500; the same defect escaping to production ≈ ₹30,000.

1. Reviewer-hours per reviewer = 9,000 ÷ 200 = 45 h; two reviewers → 90 person-hours.
2. Review cost = 90 × ₹900 = ₹81,000.
3. Latent defects = 9 KLOC × 12 = 108; caught in review = 108 × 0.55 = 59 (rounded).
4. Cost avoided per defect = ₹30,000 − ₹1,500 = ₹28,500 → total avoided = 59 × 28,500 = ₹16,81,500.
5. Net benefit = ₹16,81,500 − ₹81,000 = ₹16,00,500; ROI ≈ 16,81,500 ÷ 81,000 ≈ 20.8×.
6. Sessions needed = 9,000 ÷ 300 LOC per sitting = 30 sittings, so this must be spread across the sprint, not done the night before release.

Example 4 - Defect Removal Efficiency and release quality A 45 KLOC product: reviews found 110 defects, testing found 70, and users reported 20 in the first three months.

• D_before = 110 + 70 = 180; D_after = 20 → DRE = 180 ÷ 200 = 90%.
• Review-stage efficiency = 110 ÷ 200 = 55% - reviews caught more than testing did.
• Released defect density = 20 ÷ 45 = 0.44 defects/KLOC.
• If SonarQube estimates 14 person-days of remediation on a codebase that cost 480 person-days to build, TDR = 14 ÷ 480 = 2.9%, inside the usual "A" gate of ≤5%.

Example 5 - Maintainability Index before and after extraction A payment handler: LOC = 420, Halstead volume V = 1,800, G = 18.

• MI = 171 − 5.2 ln(1800) − 0.23(18) − 16.2 ln(420) = 171 − 5.2(7.4956) − 4.14 − 16.2(6.0403) = 171 − 38.98 − 4.14 − 97.85 = 30.0 (normalised 30.0 × 100 ÷ 171 ≈ 17.6 - flagged for refactoring).
• After Extract Class into three units of LOC = 140, V = 600, G = 6 each: MI = 171 − 5.2(6.3969) − 1.38 − 16.2(4.9416) = 171 − 33.26 − 1.38 − 80.05 = 56.3 (normalised ≈ 32.9, inside the acceptable band).

Where it is used

• IT services and product engineering: mandatory peer review plus SonarQube quality gates in TCS, Infosys, Wipro, HCLTech, Cognizant and Accenture delivery; review records feed CMMI appraisal evidence and project defect-removal dashboards.
• Product and internet companies: GitHub/GitLab merge-request review with required approvals and CI checks at Zoho, Freshworks, Flipkart, Swiggy, Zerodha, Razorpay; Gerrit +2 workflows at Qualcomm, Samsung R&D, Nokia and Ericsson India.
• Embedded, automotive and aerospace: MISRA C/AUTOSAR compliance checked by Polyspace/LDRA/Coverity plus formal inspections at Bosch, KPIT, Tata Elxsi, L&T Technology Services, HAL and ISRO suppliers, under ISO 26262 and DO-178C.
• Banking, fintech and govtech: secure code review and application security testing before go-live for RBI-regulated systems, UPI/NPCI integrations and state e-governance portals, with reviewers trained on OWASP Top 10 and personal-data handling.
• Job roles: software engineer (every merge), senior engineer/tech lead (approver and standards owner), SDET, SRE, static-analysis/DevSecOps engineer, quality engineer owning review metrics, open-source maintainer triaging contributions.
• Academic and campus use: mini-project vivas and lab records ask for coding-standard compliance, complexity figures and refactoring evidence; GitHub review history is now read by interviewers as proof of collaboration skill.

Interview questions

• Q: What makes code "clean"? A: It reads like a description of the problem - intention-revealing names, small single-purpose functions, no duplication, explicit error handling and tests - so that a new developer can change it safely without reading the whole system.
• Q: Should code have many comments? A: No - prefer self-explanatory code and keep comments for *why*: intent, trade-offs, warnings and public API docs. Commented-out code and comments restating the code should be deleted.
• Q: What is a code smell, and is it a bug? A: A surface indicator of a deeper design problem, such as a Long Method or Duplicated Code. It is not a bug; the code works but costs more to change and is more likely to break later.
• Q: Define refactoring and its precondition. A: Restructuring code without changing its external behaviour; the precondition is a reliable passing test suite, otherwise you cannot prove behaviour was preserved.
• Q: Compute the cyclomatic complexity of a function with 3 ifs, one of which has an else, and a while loop. A: Count binary decisions: 3 if + 1 while = 4 (an else adds no decision) → V(G) = 5. A compound condition with && would add one more.
• Q: Why is complexity ≤10 a common limit? A: V(G) equals the number of independent paths, so it sets the minimum branch-path tests; above about 10 the function becomes hard to test and comprehend, and defect density rises.
• Q: Difference between a walkthrough and an inspection? A: A walkthrough is informal and author-led, aimed at understanding; an inspection is formal, moderator-led with checklists, entry/exit criteria and recorded defect metrics, aimed at defect removal.
• Q: Why should a review meeting not fix the defects it finds? A: Designing fixes in the meeting consumes everyone's time, derails defect-finding and is better done by the author during rework; the meeting's job is to log and classify issues.
• Q: How big should a pull request be and why? A: Ideally under about 400 changed lines reviewed in under 60-90 minutes, because reviewer defect-detection drops sharply beyond that - large diffs get approved without real scrutiny.
• Q: Static analysis is already in CI - why still have human reviewers? A: Tools check syntax-level rules and known patterns, and produce false positives; only a human judges whether the code solves the right requirement, picks a sound abstraction and handles the business edge cases.
• Q: Does clean code conflict with performance? A: Rarely - clarity first, then optimise with profiler evidence at the measured hot spot, documenting why the ugly version exists. Premature optimisation usually buys nothing and costs readability.
• Q: As a reviewer you disagree with the author's style, but the code is correct. What do you do? A: Mark it as a non-blocking suggestion or nit, defer to the author or the house style guide, and reserve blocking comments for correctness, security, tests and maintainability risks.
• Q: What is technical debt and how is it tracked? A: The future cost of shortcuts taken today; it is tracked as a backlog of known issues plus tool metrics such as the technical-debt ratio (remediation effort ÷ development effort) with a quality gate threshold.

Common mistakes

• Using V(G) = E − N + 2P with a wrongly drawn control-flow graph instead of the quicker and safer "decisions + 1", or counting else / case fall-through as extra decisions.
• Treating a rewrite as a refactoring - if external behaviour changes, it is not refactoring, and without tests it is not verifiable either.
• Mixing up review categories in exams: calling an author-led session an inspection, or listing only three of IEEE 1028's five review types.
• Reviewing the person, not the code - unspecific or sarcastic comments; or the opposite, approving in seconds ("LGTM") on a 2,000-line diff, which is worse than no review because it records false assurance.
• Equating many comments, long names or 100% coverage with clean code, while leaving duplicated logic, magic numbers and a 300-line method untouched.
• Spending review time on formatting and import order that an auto-formatter and linter should have settled before the request was opened.
• Reporting defect counts without the denominator - "we found 40 defects" means nothing without KLOC, review hours and the defects that escaped to users (so quote defect density and DRE).

Related topics

Software testing basics · Test automation and TDD · Design patterns · Low-level design (object-oriented design) interviews · Git and version control · Software architecture: monoliths and microservices