Skip to content

Good Coding Practices

Jump to bottom
herougo edited this page May 2, 2025 · 19 revisions

PR Process Guidelines

Reviewing Code Ideas from Me

  • leave comments in a nice way
    • suggested
      • "I'd recommend making this magic variable a global constant." or "I think this magic variable should be a global constant."
      • "There is an unnecessary amount of computation occurring by zipping and unzipping the lists. Can you fix that?"
      • "Why is this line here? This isn't actionable, I'm just asking for clarity."
    • not recommended
      • "Make this magic variable a global constant."

Submitting code - Clement - https://www.youtube.com/watch?v=1Ge__2Yx_XQ

  • minimize the amount of code you submit in 1 code review (ideally 10-100)
    • large code diffs become very difficult to review
    • if the feature requires lots of changes, break it up into multiple PRs
  • leave a good description (e.g. describe the context/learnings behind a bug investigation, screenshots for frontend changes, include commands used to run generated code, etc)
  • consider pre-emptive comments in PR for non-obvious parts of the changes to help the reviewer

Reviewing Code - Clement - https://www.youtube.com/watch?v=1Ge__2Yx_XQ

  • communicate well (e.g. explain well in your feedback, make it clear how much you want the change, etc)
  • know what the approve procedure is (e.g. approve only after comments addressed)
  • know when to take discussion offline

Example PR comments

  • basic
    • magic variables
    • unecessary computation (ie unzip dict and back)
    • changing a function and forgetting to update other places that use it
    • hacky solution (the tests pass, but it does not address the real feature)
    • mistake of using thing.get('stuff', '') instead of (thing.get('stuff') or '')
  • hard to read
    • missing comments
    • overly pythonic
    • PR usually cap length at 50-200 (otherwise split into separate PRs)
    • long variable names instead of a comment
    • when reading a test case, you should be able to understand it wuthout going to helper functions
      • possibly bad: assertEqual in helper function
    • pretty print json in snapshot for easier readability
  • code style
    • not use snapshot for small arrays (easier review and prevents the snapshot files getting clogged)
  • specific
    • modifying protos is wrong

Clean Code Guidelines

Web Dev Simplified (https://www.youtube.com/watch?v=5B587bQ-TNg)

  • JavaScript: avoid nested callbacks and use promise and wait
  • look for repeated code and generalize
  • example: separate save_user and validate_user into separate functions

Clem 1 (https://www.youtube.com/watch?v=HcijbAI4eB0)

  • name class variables which leaves little ambiguity for the type (e.g. expired to is_expired or expiry_date_string)
  • break complicated conditions into human-readable steps (example)
  • avoid negative boolean names (e.g. is_not_blocked)

Clem 2 (https://www.youtube.com/watch?v=OHQoytCkQUY)

  • prefer if then continue instead of indenting an else block
  • consistency (e.g. naming variables button vs btn)
  • add comments for logic that may not make sense at first glance (e.g. weight JS tricks to get desired behaviour)

Nick Chapsas guidelines (C#) (https://www.youtube.com/watch?v=gyrSiY4SHxI)

  1. treating primitive obsession with ValueObjects (C#)
    • e.g. Temperature should not be a double, it should be a ValueObject which throws an exception if it is below absolute zero
    • ValueObject comparison uses values and not the references
  2. Consider replacing dictionaries with custom classes built specifically for that purpose
  3. One dot per line (example)
    • easier debugging
  4. Don't abbreviate
    • bad: cpm class variable (should be class_per_mille)
    • redundant: UserService.addUser() (I personally disagree with this specific example)
  5. keep entities small: be wary of class files with more than 250 lines.

SOLID (descriptions below from Wikipedia)

  • Single-responsibility principle: "There should never be more than one reason for a class to change." In other words, every class should have only one responsibility.
  • Open–closed principle: "Software entities ... should be open for extension, but closed for modification."
  • Liskov substitution principle: "Functions that use pointers or references to base classes must be able to use objects of derived classes without knowing it." See also design by contract.
  • Interface segregation principle: "Many client-specific interfaces are better than one general-purpose interface."
  • Dependency inversion principle: "Depend upon abstractions, [not] concretions."

Folder Structure

Django

See https://github.com/herougo/SoftwareEngineerKnowledgeRepository/wiki/Django-Folder-Structure

Other

See https://github.com/herougo/SoftwareEngineerKnowledgeRepository/wiki/Software-Architecture

React

Source: Web Dev Simplified ( https://www.youtube.com/watch?v=UUga4-z7b6s )

Beginner

  • src
    • components
    • hooks
    • utils

Intermediate

  • public
  • src
    • assets (global.css, logo.svg, images, etc)
    • components
      • form
      • ui
    • context
    • data (json)
    • hooks
    • pages
      • Home
      • Login
      • Settings
      • Signup
    • utils

Advanced

  • public
  • src
    • assets
    • components
    • context
    • data
    • features
      • authentication (folder structure is same as src minus what you don't need)
        • components
        • hooks
        • services
        • index.js (when importing from src/features/authentication/..., import from this file instead)
      • todos
        • components
          • tab-component1 (NEW: folder per component group)
            • tabs
              • ...
            • TabComponent1Container.ts
            • TabComponent1.ts
            • types.ts
            • TabComponent1.css
        • ...
    • hooks
    • layouts (e.g. navbar, page container, sidebar, etc)
    • lib (i.e. facade for things like fetch)
    • pages (Home.js, Login.js, Settings.js, Signup.js)
    • services (i.e. API integration such as logging analytics)
    • utils

Comments

  • Addition(s) to the advanced folder structure
    • components should be broken into display components and container components, at which point you should create a folder per component pair

Rejected

Nick Chapsas

  • don't throw exceptions, return a result instead (https://www.youtube.com/watch?v=a1ye9eGTB98)
    • Youtube comment quote: "I've tried this approach in a production system. The code rapidly becomes unwieldy because you have to check the result at every exit and if the return type of the calling method is different you have to transform that result as well. The deeper the call stack, the more overhead / boiler plate / noise you add to the code purely to avoid throwing an exception. I don't think the maintenance overhead is worth the trade-off unless you desperately need the performance. I went with throwing useful exceptions and mapping them to problem details in middleware. It was easy to understand and follow and even easier to use and maintain the surrounding code in the call stack."

Nick Chapsas guidelines (C#)

  1. At most 1 level of indentation (e.g. 1 if block, but no nested if blocks)
  2. Don't use else ??
  3. keep entities small
    • what people say
      • keep classes under 100-150 lines
      • keep folders at at most 10 files
      • keep functions/methods under 10 lines
      • keep classes under 5 methods
    • his opinion (not rejected)
      • do what you want as long as you don't violate single responsibility rule
      • be wary of class files being > lines
  4. no classes with more than 2 instance variables
  5. No getters/setters/properties

Undecided

  • place for utils which should be private to that package

Clone this wiki locally