Skip to content
This repository was archived by the owner on Apr 1, 2026. It is now read-only.

Refactor PlatformResources Controller#283

Closed
zack-rma wants to merge 2 commits into
opendcs:feature/task_cwms_supportfrom
zack-rma:feature/task_cwms_support_platform_resources
Closed

Refactor PlatformResources Controller#283
zack-rma wants to merge 2 commits into
opendcs:feature/task_cwms_supportfrom
zack-rma:feature/task_cwms_support_platform_resources

Conversation

@zack-rma

Copy link
Copy Markdown
Collaborator

Problem Description

Fixes #228.

Solution

Replaces DAO usage with OpenDCS DAI implementations.

how you tested the change

Unit tests of data mapping included.

Where the following done:

  • Tests. Check all that apply:
    • Unit tests created or modified that run during ant test.
    • Integration tests created or modified that run during integration testing
      (Formerly called regression tests.)
    • Test procedure descriptions for manual testing
  • Was relevant documentation updated?
  • Were relevant config element (e.g. XML data) updated as appropriate

If you aren't sure leave unchecked and we will help guide you to want needs changing where.

@zack-rma

Copy link
Copy Markdown
Collaborator Author

References getAll method may require new OpenDCS DAI method to support filtering by transport media type

Comment on lines +95 to +97
ApiPlatformRef ref = new ApiPlatformRef();
Platform plat = platform.next();
ref.setName(plat.getDisplayName());

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Can this call the single object map() method?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Since the ApiPlatformRef has a different structure to ApiPlatform, the map can't be reused.

public Response getPlatformRefs(@QueryParam("tmtype") String tmtype)
throws DbException
{
HashMap<String, ApiPlatformRef> ret = new HashMap<>();

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

HashMap vs Map ? Is this driven by DTO? I'm always suspicious of HashMap. IMO the extra cost of LinkedHashMap is always worth it to maintain insertion order, even if its just to make debugging consistent.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I've swapped HashMap for Map and the implementation for LinkedHashMap. I've also replaced ArraList with List where possible/appropriate.

assertEquals(status.getLastRoutingSpecName(), apiStat.getRoutingSpecName());
}

private static int iterSize(Iterator<?> it)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

interesting.
+1 b/c its clear what this is doing but -1 b/c its weird to have to do this.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, if the implementation gave us more than just the iterator this wouldn't be needed.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

These will be migrated to a more standard DAO style, but platforms is not first on the list (Enum is in the works, routingspecs/schedule entries are probably next) so for now you're stuck with doing this.

@sonarqubecloud

Copy link
Copy Markdown

@zack-rma

Copy link
Copy Markdown
Collaborator Author

Closing due to newer PR with same changes: #312

@zack-rma zack-rma closed this Jan 16, 2025
@zack-rma zack-rma removed their assignment Jan 16, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants