From ffa6e10c448771914b018b6840b6061d631c2777 Mon Sep 17 00:00:00 2001 From: Thomas Gran Date: Fri, 15 Sep 2023 15:53:41 +0200 Subject: [PATCH 1/4] doc: Architectural Decision Records --- .../workflows/close_stale_pr_and_issues.yml | 4 +- ADRs.md | 94 +++++++++++++++++++ ARCHITECTURE.md | 6 +- CODE_CONVENTIONS.md | 25 ++--- ISSUE_TEMPLATE/ADR.md | 32 +++++++ .../ISSUE_TEMPLATE.md | 0 .../transit/service/StopModel.java | 2 + 7 files changed, 145 insertions(+), 18 deletions(-) create mode 100644 ADRs.md create mode 100644 ISSUE_TEMPLATE/ADR.md rename ISSUE_TEMPLATE => ISSUE_TEMPLATE/ISSUE_TEMPLATE.md (100%) diff --git a/.github/workflows/close_stale_pr_and_issues.yml b/.github/workflows/close_stale_pr_and_issues.yml index 98619f7e763..0c337fbc485 100644 --- a/.github/workflows/close_stale_pr_and_issues.yml +++ b/.github/workflows/close_stale_pr_and_issues.yml @@ -20,5 +20,7 @@ jobs: days-before-stale: 90 days-before-close: 30 operations-per-run: 260 - exempt-issue-labels: 'Roadmap' + exempt-issue-labels: + - 'Roadmap' + - 'ADR' ascending: true diff --git a/ADRs.md b/ADRs.md new file mode 100644 index 00000000000..27fa73faf8c --- /dev/null +++ b/ADRs.md @@ -0,0 +1,94 @@ +# Architectural Decision Records (ADRs) + +An Architectural Decision (AD) is a justified software design choice that addresses a functional or +non-functional requirement that is architecturally significant. ([adr.github.io](https://adr.github.io/)) + +## Process + +Architectural decisions we make in the developer meetings are recorded here. If the decision is +small and uncontroversial, but yet important and can be expressed in maximum 2 sentences, we will +list it here without any reference. If the decision require a bit more discussion and explanations +an issue on GitHub should be created - use the template below. + +### How to discuss and document an Architectural Decision + + - [Create a new issue](https://github.com/opentripplanner/OpenTripPlanner/issues/new/choose?template=adr.md) +using the [ADR.md](https://github.com/opentripplanner/OpenTripPlanner/blob/dev-2.x/ISSUE_TEMPLATE/ADR.md) +template. + - Make sure to update the main description based on the feedback/discussion and decisions in the + developer meeting. + - The final approval is done in the developer meeting, at least 3 developers representing 3 + different organisations should approve - and no vote against the proposal. If the developers + are not able to agree the PLC can decide. + - Add the ADR to the list below. Maximum two sentences should be used - try to keep it as short as + possible. Remember to link to the discussion. + - References to Architectural Decision Records in reviews can be done by linking or just typing + e.g. `ADR2`. Use the `[ADR1]()` + + +## Records + +### ADR-0 Scout rule +Leave Things BETTER than you found them - clean up code you visit or/and add unit +tests. Expect to include some code cleanup as part of all PRs. + +### ADR-1 Naming +[Follow naming conventions](CODE_CONVENTIONS.md#naming-conventions) . Make sure the +code is easy to read and understand. + +### ADR-2 Code documentation - JavaDoc +Document the business intention and decisions in the relevant code. Do not repeat the logic +expressed in the code. The prefered way is to use JavaDoc, you may have to refactor part of your +code to encapsulate the business logic into a method or class to do this. + +> If you decide to NOT follow an Architectural Decision, then expect to document why. + +**See also** + - [Developers-Guide > Code comments](docs/Developers-Guide.md#code-comments). + - [Codestyle > Javadoc Guidlines](docs/Codestyle.md#javadoc-guidlines) - JavaDoc checklist + +### ADR-3 Code style +OTP uses prettier to format code. For more information on code style see the +[Codestyle](docs/Codestyle.md) document. + +### ADR-4 OOP +Respect Object-Oriented principals + - Honer encapsulation & Single-responsibility principle + - Abstraction - Use interfaces when a module need "someone" to play a role + - Inheritance - Inheritances expresses “is-a” and/or “has-a” relationship, do not use it "just" + to share data/functionality. + - Use polymorphism and not `instanceof`. + +### ADR-5 Dependency injection +Use dependency injection to wire components. You can use manual DI or Dagger. Put the +wiring code in `/configure/Module.java`. + +### ADR-6 Module encapsulation +Keep modules clean. Consider adding an `api`, `spi` and mapping code to +isolate the module from the rest of the code. Avoid circular dependencies between modules. + +### ADR-7 JavaDoc +Document all `public` types, methods and fields. + +### ADR-8 API doc +Document API and configuration parameters. + +### ADR-9 DRY +Keep the code [DRY](https://en.wikipedia.org/wiki/Don%27t_repeat_yourself) - Do not +repeat yourself. Avoid implementing the same business rule in two places -> refactor. + +### ADR-10 Feature envy +[Feature envy](https://refactoring.guru/smells/feature-envy) + +### ADR-11 Test coverage +All business logic should have unit tests. Keep integration/system tests to a +minimum. Add test at the lowest level practical to test the business feature. Prefer unit tests on +a method over a class, over a module, over the system. + +### ADR-12 Immutable types +Prefer immutable types over mutable. Use builders where appropriate. See +[Records, POJOs and Builders](CODE_CONVENTIONS.md#records-pojos-and-builders) + +### ADR-13 Records +[Avoid using records if you can not encapsulate it properly](CODE_CONVENTIONS.md#records) + diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md index dd0f986e518..7233bf9d0b3 100644 --- a/ARCHITECTURE.md +++ b/ARCHITECTURE.md @@ -1,6 +1,6 @@ # OTP Architecture -OTP is developed over more than 10 years, and most of the design documentation is in the code as +OTP is developed over more than 15 years, and most of the design documentation is in the code as comments and JavaDoc. Over the years the complexity have increased, and the natural developer turnover creates a demand for more architecture and design documentation. The new OTP2 documentation is put together with the source; hopefully making it easier to maintain. Instead of documenting @@ -11,6 +11,10 @@ This document is far from complete - hopefully it can evolve over time and becom introduction to OTP architecture. The OTP project GitHub issues are a good place to look for detailed discussions on many design decisions. +We document Architectural Decision in a log. Make sure you as a developer is familiar with the +decisions and follow them. Reviewers should use them actively when reviewing code and may use them +to ask for changes. + Be sure to also read the [developer documentation](docs/Developers-Guide.md). ## Modules/Components diff --git a/CODE_CONVENTIONS.md b/CODE_CONVENTIONS.md index a9fd73a0497..9a53e32fcc1 100644 --- a/CODE_CONVENTIONS.md +++ b/CODE_CONVENTIONS.md @@ -12,21 +12,6 @@ These conventions are not "hard" rules, and often there might be other forces wh decision in another direction, in that case documenting your choice is often enough to pass the review. -## Best practices - in focus - -- [ ] Document `public` interfaces, classes and methods - especially those part of a module api. -- [ ] Leave Things BETTER than you found them - clean up code you visit or/and add unit tests. -- [ ] [DRY](https://en.wikipedia.org/wiki/Don%27t_repeat_yourself) - Do not repeat yourself. Avoid implementing the same business rule in two places -> refactor. -- [ ] [Feature envy](https://refactoring.guru/smells/feature-envy) -- [ ] Make types immutable if possible. References to other Entities might need to be mutable, if - so try to init them once, and throw an exception if set again. - Example: - -```java -Builder initStop(Stop stop) { - this.stop = requireNotInitialized(this.stop, stop); -} -``` ## Naming Conventions @@ -104,7 +89,15 @@ stop = stop.copyOf().withPrivateCode("TEX").build(); ## Records, POJOs and Builders We prefer immutable typesafe types over flexibility and "short" class definitions. This make -the code more robust and less error-prune. +the code more robust and less error-prune. References to other entities might need to be mutable, +if so try to init them once, and throw an exception if set again. Example: + +```java +Builder initStop(Stop stop) { + this.stop = requireNotInitialized(this.stop, stop); +} +``` + ### Records diff --git a/ISSUE_TEMPLATE/ADR.md b/ISSUE_TEMPLATE/ADR.md new file mode 100644 index 00000000000..d0d04cc9cf8 --- /dev/null +++ b/ISSUE_TEMPLATE/ADR.md @@ -0,0 +1,32 @@ + + +### Description +*One or two sentences witch goes into the [Architectural Decision Records](../ADRs.md) document +later* + +### Context and Problem Statement +*Describe the context and problem statement, e.g., in free form using two to three sentences. You +may want to articulate the issue in form of a question* + +### Other options + + - + +### Decision & Consequences +Describes the effects of the change. What becomes easier? What will be more difficult? + +#### Positive Consequences + + - + +#### Negative Consequences + + - + +#### Checklist +- [ ] Tag issue with `ADR`. +- [ ] Give it a meaningful title that quickly lets the reader understand what this ADR is all about. +- [ ] Approved in a developer meeting with 3 votes in favor (3 organisations) +- [ ] This issue description is up-to-date with the discussion below. +- [ ] The name and description is added to the + [Architectural Decision Records](../ADRs.md) document and the ADR is linked this issue. diff --git a/ISSUE_TEMPLATE b/ISSUE_TEMPLATE/ISSUE_TEMPLATE.md similarity index 100% rename from ISSUE_TEMPLATE rename to ISSUE_TEMPLATE/ISSUE_TEMPLATE.md diff --git a/src/main/java/org/opentripplanner/transit/service/StopModel.java b/src/main/java/org/opentripplanner/transit/service/StopModel.java index 110260c1cde..ee90bd0ce71 100644 --- a/src/main/java/org/opentripplanner/transit/service/StopModel.java +++ b/src/main/java/org/opentripplanner/transit/service/StopModel.java @@ -23,6 +23,8 @@ /** * Repository for Stop entities. + * + * ARCHITECTURAL_DECISION_RECORDS.md */ public class StopModel implements Serializable { From 37ac87da3eb78ed2580873b749777c364b725409 Mon Sep 17 00:00:00 2001 From: Thomas Gran Date: Thu, 5 Oct 2023 18:18:40 +0200 Subject: [PATCH 2/4] Apply suggestions from code review Co-authored-by: Leonard Ehrenfried --- ADRs.md | 2 +- CODE_CONVENTIONS.md | 2 +- ISSUE_TEMPLATE/ADR.md | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/ADRs.md b/ADRs.md index 27fa73faf8c..b13054aff0e 100644 --- a/ADRs.md +++ b/ADRs.md @@ -53,7 +53,7 @@ OTP uses prettier to format code. For more information on code style see the ### ADR-4 OOP Respect Object-Oriented principals - - Honer encapsulation & Single-responsibility principle + - Honor encapsulation & Single-responsibility principle - Abstraction - Use interfaces when a module need "someone" to play a role - Inheritance - Inheritances expresses “is-a” and/or “has-a” relationship, do not use it "just" to share data/functionality. diff --git a/CODE_CONVENTIONS.md b/CODE_CONVENTIONS.md index 9a53e32fcc1..093c2afdcb5 100644 --- a/CODE_CONVENTIONS.md +++ b/CODE_CONVENTIONS.md @@ -89,7 +89,7 @@ stop = stop.copyOf().withPrivateCode("TEX").build(); ## Records, POJOs and Builders We prefer immutable typesafe types over flexibility and "short" class definitions. This make -the code more robust and less error-prune. References to other entities might need to be mutable, +the code more robust and less error-prone. References to other entities might need to be mutable, if so try to init them once, and throw an exception if set again. Example: ```java diff --git a/ISSUE_TEMPLATE/ADR.md b/ISSUE_TEMPLATE/ADR.md index d0d04cc9cf8..9e32a45204b 100644 --- a/ISSUE_TEMPLATE/ADR.md +++ b/ISSUE_TEMPLATE/ADR.md @@ -1,7 +1,7 @@ ### Description -*One or two sentences witch goes into the [Architectural Decision Records](../ADRs.md) document +*One or two sentences which goes into the [Architectural Decision Records](../ADRs.md) document later* ### Context and Problem Statement From 247ec7d629ffb7b1293a4887a23895d1708fa305 Mon Sep 17 00:00:00 2001 From: Andrew Byrd Date: Thu, 25 Jan 2024 20:19:19 +0800 Subject: [PATCH 3/4] Rewording in ARCHITECTURE.md Co-authored-by: Jim Martens --- ARCHITECTURE.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md index 7233bf9d0b3..ece6cf811dd 100644 --- a/ARCHITECTURE.md +++ b/ARCHITECTURE.md @@ -1,6 +1,6 @@ # OTP Architecture -OTP is developed over more than 15 years, and most of the design documentation is in the code as +OTP has been developed for more than 15 years, and most of the design documentation is in the code as comments and JavaDoc. Over the years the complexity have increased, and the natural developer turnover creates a demand for more architecture and design documentation. The new OTP2 documentation is put together with the source; hopefully making it easier to maintain. Instead of documenting From 254657f4b478d7fdc8a3e06908fba4d5b1523b3a Mon Sep 17 00:00:00 2001 From: Andrew Byrd Date: Thu, 25 Jan 2024 20:50:06 +0800 Subject: [PATCH 4/4] add ADR for dependency injection --- ADRs.md | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/ADRs.md b/ADRs.md index b13054aff0e..3bcd7213e91 100644 --- a/ADRs.md +++ b/ADRs.md @@ -92,3 +92,10 @@ Prefer immutable types over mutable. Use builders where appropriate. See ### ADR-13 Records [Avoid using records if you can not encapsulate it properly](CODE_CONVENTIONS.md#records) +### ADR-14 Dependency Injection +OTP will use a dependency injection library or framework to handle object lifecycles (particularly +request-scoped vs. singleton scoped) and ensure selective availability of components, services, +context, and configuration at their site of use. Systems that operate via imperative Java code +(whether hand-written or generated) will be preferred over those operating through reflection or +external markup files. See [additional background](https://github.com/opentripplanner/OpenTripPlanner/pull/5360#issuecomment-1910134299). +