Service Review Guidelines

If no new feature work is upcoming for a service, review this document for ideas of things to fix or to create more stories for. Work like this should be done several times a year to ensure that consistency is being maintained.

  • Review and identify inconsistencies in documentation.

    • All docs starting with README, Swagger, & CHANGELOG:

      • Readability:

        • Are they understandable?

      • Standards:

        • Are we using the most up-to-date templates or standards?

        • Do the coding standards or best practices match what’s implemented?

        • Are common error responses well defined in the docs?

  • pom.xml:

    • Latest Mobile Framework (MF) version?

    • Latest Maven Tiles version?

    • Preferably dependencies to be managed by Mobile Framework (MF)

    • JIB Maven plugins for builds present

  • Review and identify inconsistencies in implementations, apis, etc.

    • Jenkinsfile: ( See jenkins-shared-libs, clone URI: https://coderepo.mobilehealth.va.gov/scm/ckm/jenkins-shared-libs.git )

      • Are we using the latest templates?

  • Review base package structure for standards and consistency:

    • Client package (defined in mobile-openapi-client module):

      • gov.va.mobile.{}*{}.v{}[majorVersion]*{}

    • Service package (defined in mobile-service module):

      • gov.va.mobile.{}*{}.v{}[majorVersion]*{}.service

  • Review and identify complex code that makes the code hard to test and can be refactored

    • Source code:

      • Readability:

        • Code and Function names are descriptive?

      • Log levels:

        • Debug for Devs

        • Info for Testers

        • Warn for expected issues

        • Error for unexpected issues

      • Dead code:

        • Are large blocks of code commented out or otherwise no longer used?

      • Keywords:

        • Are the following present in the code: TODO, magic, miracle, regex, fix

      • Tests:

        • Are unit and integration tests defined?

        • Make sure the build fails on compiler warnings

        • Were there any interesting “warnings” or “errors” displayed in the output?

      • Spring Probes:

        • Specify Readiness dependencies (no stars or wildcards present)

      • Overuse of Utility Classes

        • Is the class really a utility? Or has the code been written in such a way that business logic is hidden behind static method? Consider refactoring the "Utility Class" into a Component/Service class and using dependency injection.

  • Review service packaging and class names:

    • Consider package structuring:

      • Can you group related classes by "feature" instead of by "layer? Consider moving the code around. Benefits: you keep related feature code together and you can scope class access more appropriately (package access).

      • Are classes used across features, such as utility classes? Consider making a "support" package and putting the classes in there. (usually called the "utils" package, but consider that some classes may be shared but also may not be utilities. The name "support" is broader.

      • Consider Integration Test location. Recommended to keep "remote" integration tests (ones that rely on Skaffold) in the root package, while keeping unit tests in the appropriate package.

        • Consider also breaking down larger Integration Test classes into multiple classes where appropriate. Having a 3000 line Integration Test class can make things very difficult to read or find what you’re looking for. Tests are also code and can be written with more base classes than just the standard "BaseITCase" copied everywhere.

      • Consider abstracting external connection information for Integration Tests. This makes the code easier to maintain. Examples from vista-note-service (clone URI: https://coderepo.mobilehealth.va.gov/scm/vdsms/vista-note-service.git):

        • ITConfig (ignore the Docker Compose): ITConfig

        • BaseITCase (protected methods used by subclasses): BaseITCase

    • Consider using names for classes that match the functionality:

      • Annotated with @RestController? Consider using: "MyController" over "MyResource"

      • Annotated with @Service? Consider using: "MyService" over "MyClass"

  • Analyze for possible performance bottlenecks.

  • Service Lint Consideration:

    • Is the service-lint plugin added to the service’s pom? If not, if there is a reason why start a discussion on why not

    • Are there common issues that have come up in the past that would be prevented with a service-lint rule? Start a discussion around this so new rules can be created

  • General Cleanup:

    • Remove legacy "site" directory in the "service" module

    • Remove "sandbox" environment from services that don’t deploy to Sandbox

      • Remove Kustomize folder

      • Remove Skaffold config (NOT the build config though)

      • Remove the Spring Profile properties file

    • Revisit ".gitignore" file to check matching maintained archetype projects:

      • mobile-archetypes (clone URI: https://coderepo.mobilehealth.va.gov/scm/dhsss/mobile-archetypes.git)

Acceptance Criteria

  • Each service is reviewed for the items listed above.

  • Stories are written and placed in the backlog.

Reach out to Mike Ferraro for questions or ideas on improving services or major functionality changes.