Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(application): servers don't have access on AdHocModule anymore #404

Merged
merged 2 commits into from
Sep 3, 2024

Conversation

kschrab
Copy link
Contributor

@kschrab kschrab commented Aug 15, 2024

Description

  • Major refactoring of operating system interface hierarchy with a clear definition of which modules are available via additional interfaces.
  • An OperatingSystem itself is not "owner" of a module anymore (but the implementing simulation unit is). Instead the specific OperatingSystem defines which modules it provides, by defining additional interfaces CommunicativeCell, Perceptive, Navigable and so on.
  • AbstractSimulationUnit still implements and provides functionality a specific unit not necessarily needs, e.g., the AbstractSimulationUnit provides an AdhocModule, even if the implementing ServerSimulationUnit does not need such. Still, a server application can not access the AdhocModule anymore due to the clear definition of the OperatingSystem interfaces.
  • Moved navigation package inside simulation package alongside perception and communication.

os

Issue(s) related to this PR

  • Resolves issue # / internal issue

Affected parts of the online documentation

Changes in the documentation required?

No, Names of OperatingSystems are kept

Definition of Done

Prerequisites

  • You have read CONTRIBUTING.md carefully.
  • You have signed the Contributor License Agreement.
  • Your GitHub user id is linked with your Eclipse Account.

Required

  • The title of this merge request follows the scheme type(scope): description (in the style of Conventional Commits)
  • You have assigned a suitable label to this pull request (e.g., enhancement, or bugfix)
  • origin/main has been merged into your Fork.
  • Coding guidelines have been followed (see CONTRIBUTING.md).
  • All checks on GitHub pass.
  • All tests on Jenkins pass.

Requested (can be enforced by maintainers)

  • New functionality is covered by unit tests or integration tests. Code coverage must not decrease.
  • If a bug has been fixed, a new unit test has been written (beforehand) to prove misbehavior
  • There are no new SpotBugs warnings.

Special notes to reviewer

* Major refactoring of operating system interface hierarchy with a clear definition of which modules are available via additional interfaces.
* An OperatingSystem itself is not "owner" of a module anymore (but the implementing simulation unit is). Instead an specific OperatingSystem defines which modules it provides, by defining additional interfaces `CommunicativeCell`, `Perceptive`, `Navigable` and so on.
* `AbstractSimulationUnit` still implements and provides functionality a specific unit not necessarily needs, e.g., the `AbstractSimulationUnit` provides an `AdhocModule`, even if the implementing `ServerSimulationUnit` does not need such. Still, a server application can not access the `AdhocModule` anymore due to the clear definition of the `OperatingSystem` interfaces.
* Moved `navigation` package inside `simulation` package alongside `perception` and `communication`.
@kschrab kschrab self-assigned this Aug 15, 2024
@kschrab kschrab added the enhancement New feature or request label Aug 15, 2024
@kschrab kschrab changed the title feat(application): servers don't have access on AdHocModule anymore Draft: feat(application): servers don't have access on AdHocModule anymore Aug 15, 2024
@kschrab kschrab marked this pull request as draft August 15, 2024 15:22
@kschrab kschrab changed the title Draft: feat(application): servers don't have access on AdHocModule anymore feat(application): servers don't have access on AdHocModule anymore Aug 15, 2024
Copy link
Contributor

@schwepmo schwepmo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a really valuable refactoring. However, I think we should at least discuss the naming scheme of the added interfaces.

@kschrab
Copy link
Contributor Author

kschrab commented Aug 16, 2024

In my initial suggestion for naming interfaces to declare functionality for the OperatingSystem I used adjectives, as it felt very suitable here and also complies with this suggestion here: https://www.baeldung.com/java-interface-naming-conventions (use adjectives for Interfaces as capabilities)

Googles style does not really care if Interfaces should be adjectives or nouns (allows both).

Therefore we just can/should decide which we use, but it should be consistent within the package (no mix of adjectives/nouns).

This MR (adjectives) Other Suggestions (Nouns)
AdHocCommunicative AdHocCommunicator
CellCommunicative CellCommunicator
Locatable Localizable (adjective)
SelfLocator
Navigable
Navigational
Navigator
Perceptive Perceiver
Routable
RouteCalculable
Router (meh)
?

@schwepmo
Copy link
Contributor

schwepmo commented Aug 16, 2024

In my initial suggestion for naming interfaces to declare functionality for the OperatingSystem I used adjectives, as it felt very suitable here and also complies with this suggestion here: https://www.baeldung.com/java-interface-naming-conventions (use adjectives for Interfaces as capabilities)

Googles style does not really care if Interfaces should be adjectives or nouns (allows both).

Therefore we just can/should decide which we use, but it should be consistent within the package (no mix of adjectives/nouns).
This MR (adjectives) Other Suggestions (Nouns)
AdHocCommunicative AdHocCommunicator
CellCommunicative CellCommunicator
Locatable Localizable (adjective)
SelfLocator
Navigable Navigator
Perceptive Perceiver
Routable Router (meh)
?

I agree with the general naming schema of using adjectives for the description of interfaces. However, in this case it is not really about adding "attributes" to an object but rather enabling the access to additional capabilities.
I.e., the ServerOperatingSystem itself is not routable but rather is able to route others.

Thinking about this just now, this logic really only applies to the Routable interface. Maybe also RouteCalculator would be an alternative.

Further suggestions:

  • Navigable -> Navigational
    • "Navigable" typically refers to something, such as a waterway, that can be navigated.
    • "Navigational" relates to the process or practice of navigation.

@rprotzmann should definitely comment on this issue

@schwepmo schwepmo marked this pull request as ready for review September 3, 2024 13:54
@kschrab kschrab merged commit eb5db43 into main Sep 3, 2024
6 checks passed
@kschrab kschrab deleted the 876-restructure-operating-system branch September 3, 2024 16:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants