Skip to content

Feedback about code base in services/api layer. #419

Description

@jvaqueroh
  1. What you'd change

At the first sight to services/api/src/stories/backlog.ts, for example, I found very long methods that can be considered as “spaghetti code”.

Taken deep into, I found complex logic in some services like ProjectBacklogService and functions like reconcileTurnBranch.

Additionally I’ve seen the very long test setup in integration tests. This could be a signal of a complex design.

That said, I’d suggest a more object oriented approach. This approach would bring better encapsulation and long-term maintainability and, at the same time, the code would be easier to read, understand and test.

Given the open source nature of this project, I think it would be a good idea to refactor the code to a OO approach.

Details and evidences

  • backlog.ts

    • items() -> long method, multiple responsibilities
      • DB logic, pulls logic, stories logic, etc.
      • It is hard to understand what it does.
    • potential improvements
      • extract and encapsulate data access logic into a repository pattern.
      • extract and encapsulate specific model logic into domain classes and bring semantic meaning to the code.
      • turn the method into an high level orchestrator method.
  • backlog.ts, phase.ts:

    • potential anemic domain model -> domain classes (or types) with no logic.
    • specific domain logic spread by the services objects like ProjectBacklogService.
      • E.g. function expandPhases() -> it would be filters.toPhases() given that it only needs filters data.
      • E.g. const search -> it would be query.toSearch() given that it only needs query data.
      • E.g. nested functions matchesSearch(...) and searchMatches() -> they could be item.matches(search: String)
    • potential improvements
      • apply “tell don´t ask” principle.
      • improve encapsulation, readability and maintainability.
      • improve small, scoped, unit tests for specific domain logic.
  • branch.ts

    • reconcileTurnBranch() -> long method, multiple responsibilities
      • validations, DB queries, DB writes
    • potential improvements
      • extract and encapsulate DB operations into repository pattern -> limiting responsibilities, bringing semantic meaning, simplify the code, simplify unit and integration testing, improve maintainability.
  1. One thing you'd question

Has this code been generated with an AI agent? If true, do you think there is place for an improvement of the process/loop/harness around the AI agent?

  1. Where you'd naturally jump in

Being coherent with point 1, I’d jump into refactoring services/api with the goal to keep it clean, readable, testable and maintainable in the long-term. It seems to be where deterministic logic and models of the system live. It is important to take care of this core code base.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions