Developer Decision Records

.github/pull_request_template.md

@@ -47,7 +47,7 @@ Write a few words on how the new code is tested.
 - Was the code designed so it is unit testable?
 - Were any tests applied to the smallest appropriate unit?
 - Do all tests
-  pass [the continuous integration service](https://github.com/opentripplanner/OpenTripPlanner/blob/dev-2.x/docs/Developers-Guide.md#continuous-integration)
+  pass [the continuous integration service](https://github.com/opentripplanner/OpenTripPlanner/blob/dev-2.x/doc/user/Developers-Guide.md#continuous-integration)
   ?
 
 ### Documentation
@@ -59,7 +59,7 @@ Write a few words on how the new code is tested.
 
 ### Changelog
 
-The [changelog file](https://github.com/opentripplanner/OpenTripPlanner/blob/dev-2.x/docs/Changelog.md)
+The [changelog file](https://github.com/opentripplanner/OpenTripPlanner/blob/dev-2.x/doc/user/Changelog.md)
 is generated from the pull-request title, make sure the title describe the feature or issue fixed.
 To exclude the PR from the changelog add the label `skip changelog` to the PR.
 

.github/workflows/cibuild.yml

@@ -108,7 +108,7 @@ jobs:
         if: github.event_name == 'pull_request'
 
       - name: Install Python dependencies
-        run: pip install -r docs/requirements.txt
+        run: pip install -r doc/user/requirements.txt
 
 
       - name: Build main documentation

.github/workflows/post-merge.yml

@@ -29,8 +29,8 @@ jobs:
         run: |
           # add a line above the one which contains AUTOMATIC_CHANGELOG_PLACEHOLDER
           ITEM="${TITLE} [#${NUMBER}](${URL})"
-          TEMP_FILE=docs/Changelog.generated.md
-          FILE=docs/Changelog.md
+          TEMP_FILE=doc/user/Changelog.generated.md
+          FILE=doc/user/Changelog.md
           awk "/CHANGELOG_PLACEHOLDER/{print \"- $ITEM\"}1" $FILE > $TEMP_FILE
           mv $TEMP_FILE $FILE
           git add $FILE

.gitignore

@@ -30,7 +30,7 @@ o_o_standalone_config_IncludeFileDirectiveTest_part.json
 _site/
 build/
 dist/
-docs/_build/
+doc/user/_build/
 gen-java/
 gen-javabean/
 gen-py/

ARCHITECTURE.md

@@ -1,32 +1,36 @@
 # OTP Architecture
 
-OTP is developed over more than 10 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
 modules in old style _package-info.java_ files we use _package.md_ files. This document should serve
 as an index to all existing top-level documented components.
 
-This document is far from complete - hopefully it can evolve over time and become a good
+This document is far from complete. Hopefully it can evolve over time and become a good
 introduction to OTP architecture. The OTP project GitHub issues are a good place to look for
 detailed discussions on many design decisions.
 
-Be sure to also read the [developer documentation](docs/Developers-Guide.md).
+We document [Decision Records](DECISION_RECORDS.md) in a log. Make sure you as a developer are 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](doc/user/Developers-Guide.md).
 
 ## Modules/Components
 
 The diagram shows a simplified/generic version on how we want to model the OTP components with 2 
 examples. The Transit model is more complex than the VehiclePosition model.   
 
-![MainModelOverview](docs/images/ServiceModelOverview.png)
+![MainModelOverview](doc/user/images/ServiceModelOverview.png)
 
  - `Use Case Service` A service which combine the functionality in many `Domain Services` to fulfill
    a use-case or set of features. It may have an api with request/response classes. These are 
    usually stateless; Hence the `Use Case Service` does normally not have a model. The implementing
    class has the same name as the interface with prefix `Default`.
  - `Domain Model` A model which encapsulate a business area. In the drawing two examples are shown,
    the `transit` and `vhicleposition` domain model. The transit model is more complex so the 
-   implementation have a separate `Service` and `Repository`. Almost all http endpoints are , 
+   implementation has a separate `Service` and `Repository`. Almost all http endpoints are, 
    read-only so the `Service` can focus on serving the http API endpoints, while the repository
    is used to maintain the model by the updaters. 
 

DEVELOPMENT_DECISION_RECORDS.md

@@ -0,0 +1,106 @@
+# Development Decision Records
+
+
+## Use-Decision-Records
+
+We will [use decision-records](doc/dev/decisionrecords/UseDecisionRecords.md) to document OTP 
+development relevant decision. Use the [template](doc/dev/decisionrecords/_TEMPLATE.md) to describe
+new decision records.
+
+
+## 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.
+
+## Follow-Naming-Conventions
+
+Use established terminology from GTFS, NeTEx or the existing OTP code. Make sure the code is easy
+to read and understand. [Follow naming conventions](CODE_CONVENTIONS.md#naming-conventions) . 
+
+
+## Write-Code-Documentation - Use JavaDoc
+
+Document the business intention and decisions in the relevant code. Do not repeat the logic
+expressed in the code. Use JavaDoc, you may have to refactor part of your code to encapsulate the
+business logic into a method or class to do this. 
+
+Document all `public` types, methods and fields with JavaDoc. It is ok to document implementation 
+notes on `private` members and as inline comments.
+
+> If you decide to NOT follow these decision records, then you must document why.
+
+**See also**
+ - [Developers-Guide > Code comments](doc/user/Developers-Guide.md#code-comments).
+ - [Codestyle > Javadoc Guidlines](doc/dev/decisionrecords/Codestyle.md#javadoc-guidlines) - JavaDoc checklist
+
+
+## Document-Config-and-APIs
+
+Document API and configuration parameters.
+
+
+## Respect-Codestyle
+
+OTP uses prettier to format code. For more information on code style see the 
+[Codestyle](doc/dev/decisionrecords/Codestyle.md) document.
+
+
+## Use-Object-Oriented-Principals
+
+Respect Object-Oriented principals
+  - Honor encapsulation & Single-responsibility principle
+  - Abstraction - Use interfaces when a module needs "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`.
+
+
+## Use-Dependency-Injection
+
+Use dependency injection to wire components. You can use manual DI or Dagger. Put the 
+wiring code in `<module-name>/configure/<Module-name>Module.java`.
+
+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).
+
+## Use-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.
+
+
+## DRY - Do not repeat yourself
+
+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.
+
+
+## Avoid-Feature-Envy
+
+[Feature envy](https://refactoring.guru/smells/feature-envy)
+
+
+## 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. On all non-trivial code, full _branch_ test coverage is preferable. 
+Tests should be designed to genuinely demonstrate correctness or adherence to specifications, not 
+simply inflate line coverage numbers.
+
+
+## Use-Immutable-Types
+
+Prefer immutable types over mutable. Use builders where appropriate. See 
+[Records, POJOs and Builders](doc/dev/decisionrecords/RecordsPOJOsBuilders.md#records-pojos-and-builders)
+
+
+## Be-Careful-With-Records
+
+[Avoid using records if you cannot encapsulate it properly](doc/dev/decisionrecords/RecordsPOJOsBuilders.md#records)
+
+

docs/Codestyle.md → doc/dev/decisionrecords/Codestyle.md

@@ -58,7 +58,7 @@ Editor > Code Style_. Then select **Project** in the \_Scheme drop down.
 You can run the Prettier Maven plugin as an external tool in IntelliJ. Set it up as an
 `External tool` and assign a key-shortcut to the tool execution.
 
-![External Tool Dialog](images/ExternalToolDialog.png)
+![External Tool Dialog](../../../doc/user/images/ExternalToolDialog.png)
 
 ```
 Name:              Prettier Format Current File

CODE_CONVENTIONS.md → doc/dev/decisionrecords/NamingConventions.md

@@ -1,42 +1,12 @@
-# Code Conventions
-
-We try to follow these conventions or best practices. The goal is to get cleaner code and make the
-review process easier:
-
-- the developer knows what to expect
-- the reviewer knows what to look for
-- discussions and personal preferences can be avoided saving time
-- new topics should be documented here
-
-These conventions are not "hard" rules, and often there might be other forces which pull a
-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
+# Naming Conventions
 
 In general, we use American English. We use the GTFS terminology inside OTP as the transit domain
 specific language. In cases where GTFS does not provide an alternative we use NeTEx. The naming
 should follow the Java standard naming conventions. For example a "real-time updater" class
 is named `RealTimeUpdater`. If in doubt check the Oxford Dictionary(American).
 
 
-### Packages
+## Packages
 
 Try to arrange code by domain functionality, not technology. The main structure of a package should
 be `org.opentripplanner.<domain>.<component>.<sub-component>`.
@@ -55,14 +25,14 @@ be `org.opentripplanner.<domain>.<component>.<sub-component>`.
 | `util`                          | General "util" functionality, often characterized by `static` methods. Dependencies to other OTP packages is NOT allowed, only 3rd party utils libs.                                                     |
 | `o.o.apis`                      | OTP external endpoints. Note! Many apis are in the Sandbox where they are in the `o.o.ext` package.                                                                                                      |
 
-> **Note!** The above is the goal, the current package structure needs cleanup. 
+> **Note!** The above is the goal, the current package structure needs cleanup.
 
 > **Note!** Util methods depending on an OTP type/component should go into that type/component, not in the
-utils class. E.g. static factory methods. Warning the "pure" utilities right now are placed into 
+utils class. E.g. static factory methods. Warning the "pure" utilities right now are placed into
 sub-packages of `o.o.util`, the root package needs cleanup.
 
 
-### Methods
+## Methods
 
 Here are a list of common prefixes used, and what to expect.
 
@@ -88,15 +58,15 @@ These prefixes are also "allowed", but not preferred - they have some kind of ne
 | `setStop(Stop stop)`                        | Set a mutable stop reference. Avoid if not part of natural lifecycle. Use `initStop(...)` if possible |
 | `getStop() : Stop`                          | Old style accessor, use the shorter form `stop() : Stop`                                              |
 
-### Service, Model and Repository
+## Service, Model and Repository
 
-![MainModelOverview](docs/images/ServiceModelOverview.png) 
+![MainModelOverview](doc/user/images/ServiceModelOverview.png)
 
 
 
 Naming convention for builders with and without a context.
 
-##### Graph Build and tests run without a context
+#### Graph Build and tests run without a context
 
 ```Java
 // Create a new Stop
@@ -105,112 +75,3 @@ trip = Trip.of(id).withName("The Express").build();
 // Modify and existing stop
 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.
-
-### Records
-
-You may use records, but avoid using records if you can not encapsulate it properly. Be especially
-aware of arrays fields (can not be protected) and collections (remember to make a defensive copy).
-If you need to override `equals` and `hashCode`, then it is probably not worth it.
-Be aware that `equals` compare references, not the value of a field. Consider overriding `toString`.
-
-### Builders
-
-OTP used a simple builder pattern in many places, especially when creating immutable types.
-
-#### Builder conventions
-- Use factory methods to create builder, either `of()` or `copyOf()`. The _copyOf_ uses an existing
-  instance as its base. The `of()` creates a builder with all default values set. All constructors 
-  should be private (or package local) to enforce the use of the factory methods.
-- If the class have more than 5 fields avoid using an inner class builder, instead create a builder
-  in the same package.
-- Make all fields in the main class final to enforce immutability.
-- Consider using utility methods for parameter checking, like `Objects#requireNonNull` and 
-  `ObjectUtils.ifNotNull`.
-- Validate all fields in the main type constructor(i.e. not in the builder), especially null checks. 
-  Prefer default values over null-checks. All business logic using the type can rely on its validity.
-- You may keep the original instance in the builder to avoid creating a new object if nothing 
-  changed. This prevents polluting the heap for long-lived objects and make comparison very fast.
-- There is no need to provide all get accessors in the Builder if not needed.
-- Unit-test builders and verify all fields are copied over.
-- For nested builders see the field `nested` in the example.
-
-<details>
-    <summary><b>Builder example</b></summary>
-
-```Java
-/**
- * THIS CLASS IS IMMUTABLE AND THREAD-SAFE
- */
-public class A {
-  public static final A DEFAULT = new A();
-  private final List<String> names;
-  private final int age;
-  private final B nested;
-
-  private A() {
-    this.names = List.of("default");
-    this.age = 7;
-    this.nested = B.of();
-  }
-
-  private A(Builder builder) {
-    this.names = List.copyOf(builder.names);
-    this.age = builder.age;
-    this.nested = builder.nested();
-
-    if(age < 0 || age > 150) {
-      throw new IllegalArgumentException("Age is out of range[0..150]: " + age);
-    }
-  }
-
-  public static A.Builder of() { return DEFAULT.copyOf(); }
-  public A.Builder copyOf() { return new Builder(this); }
-
-  public List<String> listNames() { return names; }
-  public int age()                { return age; }
-
-  public boolean equals(Object other) { ... }
-  public int hashCode()               { ... }
-  public String toString()            { return ToStringBuilder.of(A.class)...; }
-
-  public static class Builder {
-    private final A original;
-    private final List<String> names;
-    private int age;
-    private B.Builder nested = null;
-
-    public Builder(A original) {
-      this.original = original;
-      this.names = new ArrayList<>(original.names);
-      this.age = original.age;
-    }
-
-    public Builder withName(String name) { this.names.add(name); return this; }
-
-    public int age()                     { return age; }
-    public Builder withAge(int age)      { this.age = age; return this; }
-
-    private B nested() { return nested==null ? original.nested() : nested.build(); } 
-    public Builder withB(Consumer<B.Builder> body) {
-      if(nested == null) { nested = original.nested.copyOf(); } 
-      body.accept(nested);
-      return this;
-    }
-    public A build() {
-      A value = new A(this);
-      return original.equals(value) ? original : value;
-    }
-  }
-}
-```
-
-</details>
-
-
-