[#4303] improvement(core): Improved removed loop code and readability

[khmgobe]

What changes were proposed in this pull request?
Reflect changes to improve readability

Why are the changes needed?
Readability and no unnecessary repetition compared to existing code

Fix: #4303

Does this PR introduce any user-facing change?
No

How was this patch tested?
Check change code and existing code comparison

[jerryshao]

@xloya would you please help to review this code?

[xloya]

How about using namespace.levels().length >=1 here? The same treatment is also applied below.

[khmgobe]

@xloya

Thank you for your suggestion. I guess I missed something.

if (namespace.levels().length >= 1) {
      parentEntityId = MetalakeMetaService.getInstance().getMetalakeIdByName(namespace.level(0));
}

if (namespace.levels().length >= 2) {
      parentEntityId =
          CatalogMetaService.getInstance()
              .getCatalogIdByMetalakeIdAndName(parentEntityId, namespace.level(1));
}

if (namespace.levels().length >= 3) {
      parentEntityId =
          SchemaMetaService.getInstance()
              .getSchemaIdByCatalogIdAndName(parentEntityId, namespace.level(2));
}

Is it correct that you mentioned changing it like this? If it is, we will proceed with the change like this.

[justinmclean]

parentEntityId only needs to be set once, if the level is 3 it will be set 3 times above.

[xloya]

@justinmclean Hi, according to the current storage structure design, if the namespace has 3 levels, it is reasonable to recursively obtain the parentEntityId 3 times, because we cannot guarantee that the parent resource can be obtained uniquely through Name, so we need to use parentEntityId and Name to locate the upper-level resource at the same time.

[xloya]

@xloya

Thank you for your suggestion. I guess I missed something.

if (namespace.levels().length >= 1) {
      parentEntityId = MetalakeMetaService.getInstance().getMetalakeIdByName(namespace.level(0));
}

if (namespace.levels().length >= 2) {
      parentEntityId =
          CatalogMetaService.getInstance()
              .getCatalogIdByMetalakeIdAndName(parentEntityId, namespace.level(1));
}

if (namespace.levels().length >= 3) {
      parentEntityId =
          SchemaMetaService.getInstance()
              .getSchemaIdByCatalogIdAndName(parentEntityId, namespace.level(2));
}

Is it correct that you mentioned changing it like this? If it is, we will proceed with the change like this.

Yes, I think this can better express the Namespace levels:

if (namespace.levels().length >= 1) {
      ......
}

if (namespace.levels().length >= 2) {
      ......
}

if (namespace.levels().length == 3) {
      ......
}

[justinmclean]

For example if level is 3 then parentEntityId will be set three times with 2 of those values being thrown away. In this order:

1. via getMetalakeIdByName (result overwritten by 2 and 3)
2. via getCatalogIdByMetalakeIdAndName (result overwritten by 3)
3. via getSchemaIdByCatalogIdAndName

Why not just obtain it by directly calling getSchemaIdByCatalogIdAndName? e.g reverse the order of the if statements and change them to if/else ifs.

[jerryshao]

I think what @khmgobe is reasonable, it is more readable than the original code, we can also improve like what @xloya mentioned.

The only bad thing is that parentEntityId is mutable, which is not a good programming practice, but I think it is acceptable as there's no side-effect here.

@khmgobe would you please modify the code like what @xloya suggested?

[khmgobe]

@jerryshao @justinmclean

Is there a consensus?
Please let me know if you don't.

If it's matched, we're going to make a change.

The reason for asking again is that I think I'll change it
when it's decided for sure because I'm afraid there will be a change in the future.

[jerryshao]

Yes, please go ahead.

[khmgobe]

@jerryshao I revised the code as per the common opinion, so please check it.

[xloya]

This change is generally okay for me, except for one minor suggestion.

[jerryshao]

LGTM, thanks @khmgobe for the contributions.