Optimize checkLocalConfig method in ClientWoker

[PleaseGiveMeTheCoke]

Problem Statement

The current implementation of the checkLocalConfig method in ClientWoker has multiple conditional checks that are not only redundant but also make the code difficult to read and maintain. In addition, the method contains duplication in logic for handling failover file creation, changes, and deletion events.

Proposed Change

I propose an optimization of the checkLocalConfig method that simplifies the code, reduces redundancy, and enhances maintainability. The key changes include:

1. Consolidating the conditions that check for the existence, creation, and modification of the failover file into a streamlined logical structure.
2. Removing duplicate code blocks and extracting them into reusable code snippets where appropriate.

Impact of Change

This optimization does not impact the existing functionality but rather improves the internal code quality of the method. It is a non-breaking change that retains all existing behaviors while streamlining the code.

Suggested Implementation

I have already implemented a version of this optimized method and am prepared to submit a Pull Request for further review and testing by the Nacos community. The proposed implementation can be found in the following snippet:

public void checkLocalConfig(CacheData cacheData) {
      final String dataId = cacheData.dataId;
      final String group = cacheData.group;
      final String tenant = cacheData.tenant;
      final String envName = cacheData.envName;

      // Check if a failover file exists for the specified dataId, group, and tenant.
      File file = LocalConfigInfoProcessor.getFailoverFile(envName, dataId, group, tenant);

      // not using local config info, but a failover file exists
      boolean failOverFileCreated = !cacheData.isUseLocalConfigInfo() && file.exists();

      // using local config info, but there is a change in local configuration
      boolean failOverFileChanged= cacheData.isUseLocalConfigInfo() && file.exists() && cacheData.getLocalConfigInfoVersion() != file.lastModified();

      // using local config info, but the failover file is deleted
      boolean failOverFileDeleted = cacheData.isUseLocalConfigInfo() && !file.exists();

      if (failOverFileCreated || failOverFileChanged) {
          // load and use the file content
          String content = LocalConfigInfoProcessor.getFailover(envName, dataId, group, tenant);
          final String md5 = MD5Utils.md5Hex(content, Constants.ENCODE);
          cacheData.setUseLocalConfigInfo(true);
          cacheData.setLocalConfigInfoVersion(file.lastModified());
          cacheData.setContent(content);
          LOGGER.warn(
                  "[{}] [failover-change] failover file {}. dataId={}, group={}, tenant={}, md5={}, content={}",
                  failOverFileCreated ? "created" : "changed", envName, dataId, group, tenant, md5, ContentUtils.truncateContent(content));;
      }

      if (failOverFileDeleted) {
          // switch back to server config.
          cacheData.setUseLocalConfigInfo(false);
          LOGGER.warn("[{}] [failover-change] failover file deleted. dataId={}, group={}, tenant={}", envName,
                  dataId, group, tenant);
      }
  }

Please let me know if there are any concerns or additional considerations that should be taken into account. I believe this optimization aligns with the goals of maintaining high code quality and I look forward to the community's feedback.

Thanks for considering this proposal.

[stone-98]

I think your approach is feasible, but I have a suggestion: would it be better to use 'else if' for the branches of 'failOverFileCreated || failOverFileChanged' and 'failOverFileDeleted'? Does that sound okay to you?

[PleaseGiveMeTheCoke]

I think your approach is feasible, but I have a suggestion: would it be better to use 'else if' for the branches of 'failOverFileCreated || failOverFileChanged' and 'failOverFileDeleted'? Does that sound okay to you?

Thanks for reviewing and approving. else-if is certainly fine, but I'll try to avoid using else, for the sake of the code looking a bit more stylized (a bit of personal preference on my part) Or is there a reason why else if must be used here?

[stone-98]

I think your approach is feasible, but I have a suggestion: would it be better to use 'else if' for the branches of 'failOverFileCreated || failOverFileChanged' and 'failOverFileDeleted'? Does that sound okay to you?

Thanks for reviewing and approving. else-if is certainly fine, but I'll try to avoid using else, for the sake of the code looking a bit more stylized (a bit of personal preference on my part) Or is there a reason why else if must be used here?

I just noticed that in the previous logic, if it enters an if branch, it breaks without executing other if checks. However, I think it doesn’t have a significant impact. Personally, I believe your approach is also okay. 😊

[PleaseGiveMeTheCoke]

@KomachiSion Can I submit a PR to optimize the code for this issue?

[KomachiSion]

If only do refactor, not change the logic, PR welcome, but the judgement is depend by @shiyiyue1102 .

[TARNA95]

public void checkLocalConfig(CacheData cacheData) { final String dataId = cacheData.dataId; final String group = cacheData.group; final String tenant = cacheData.tenant; final String envName = cacheData.envName;

  // Check if a failover file exists for the specified dataId, group, and tenant.
  File file = LocalConfigInfoProcessor.getFailoverFile(envName, dataId, group, tenant);

  // not using local config info, but a failover file exists
  boolean failOverFileCreated = !cacheData.isUseLocalConfigInfo() && file.exists();

  // using local config info, but there is a change in local configuration
  boolean failOverFileChanged= cacheData.isUseLocalConfigInfo() && file.exists() && cacheData.getLocalConfigInfoVersion() != file.lastModified();

  // using local config info, but the failover file is deleted
  boolean failOverFileDeleted = cacheData.isUseLocalConfigInfo() && !file.exists();

  if (failOverFileCreated || failOverFileChanged) {
      // load and use the file content
      String content = LocalConfigInfoProcessor.getFailover(envName, dataId, group, tenant);
      final String md5 = MD5Utils.md5Hex(content, Constants.ENCODE);
      cacheData.setUseLocalConfigInfo(true);
      cacheData.setLocalConfigInfoVersion(file.lastModified());
      cacheData.setContent(content);
      LOGGER.warn(
              "[{}] [failover-change] failover file {}. dataId={}, group={}, tenant={}, md5={}, content={}",
              failOverFileCreated ? "created" : "changed", envName, dataId, group, tenant, md5, ContentUtils.truncateContent(content));;
  }

  if (failOverFileDeleted) {
      // switch back to server config.
      cacheData.setUseLocalConfigInfo(false);
      LOGGER.warn("[{}] [failover-change] failover file deleted. dataId={}, group={}, tenant={}", envName,
              dataId, group, tenant);
  }

}