Какие ошибки присутствуют в данном “алгоритме”? [закрыт]

155

08 августа 2021, 21:30

Закрыт. Этот вопрос необходимо уточнить или дополнить подробностями. Ответы на него в данный момент не принимаются.

Хотите улучшить этот вопрос? Добавьте больше подробностей и уточните проблему, отредактировав это сообщение.

Закрыт 1 год назад.

Улучшить вопрос

Есть рабочий кусок кода нуждающийся в рефакторинге. Он выполняет два ключевых этапа: isDocumentHasChanges - поиск изменений, и если есть то Update. Я не шибко специалист в плане налаживания кода, какие здесь есть косяки и как их можно исправить? (кроме именования переменных)

for (Map.Entry<String, DbList> entry : images.entrySet())
        {
            for(Object newImage : entry.getValue())
            {
                Map<String, Object> parameters = mappingpParameters((DataObject) newImage);
                Map<String, String> attributes = mappingAttributes((DataObject) newImage, importPars);
                if(!getURL((DataObject) newImage).equals(""))
                {
                    DataObject data = findDataById(attributes.get(ID), importPars);
                    File image = createImage(importPars, attributes, data);
                    if(!useImgImport || isDocumentHasChanges(image, data, parameters))
                    {
                        try
                        {
                            DbList forUpdate = docsForUpdate(image, newImage, attributes, parameters, importPars, data, useImgImport);
                            if (useImgImport)
                            {
                                update(forUpdate, importPars.getCollectionName(), data);
                            }
                        }
                        catch (Exception e)
                        {
                            logger.error("Current image has not saved", e);
                        }
                    }
                }
            }
        }

Answer 1

Из того, что видно

for (Map.Entry<String, DbList> entry: images.entrySet()) {
  for (Object newImage: entry.getValue()) {
    if (!getURL((DataObject) newImage).equals("")) {
      // Вначале проверяем URL, а потом, при необходимости, создаем Map
      Map<String, Object> parameters = mappingpParameters((DataObject) newImage);
      Map<String, String> attributes = mappingAttributes((DataObject) newImage, importPars);
      DataObject data = findDataById(attributes.get(ID), importPars);
      File image = createImage(importPars, attributes, data);
     // Почему так, смотри ниже
      if (useImgImport && isDocumentHasChanges(image, data, parameters)) {
        try {
          DbList forUpdate = docsForUpdate(image, newImage, attributes, parameters, importPars, data, useImgImport);
          update(forUpdate, importPars.getCollectionName(), data);
        } catch (Exception e) {
          logger.error("Current image has not saved", e);
        }
      }
    }
  }
}

Или, если разнести условие (useImgImport && isDocumentHasChanges(image, data, parameters)) то получится еще лучше

if (useImgImport) {  // useImgImport нигде не модифицируется и ее можно проверить только раз
  for (Map.Entry<String, DbList> entry: images.entrySet()) {
    for (Object newImage: entry.getValue()) {
      if (!getURL((DataObject) newImage).equals("")) {
        // Вначале проверяем URL, а потом, при необходимости, создаем Map
        Map<String, Object> parameters = mappingpParameters((DataObject) newImage);
        Map<String, String> attributes = mappingAttributes((DataObject) newImage, importPars);
        DataObject data = findDataById(attributes.get(ID), importPars);
        File image = createImage(importPars, attributes, data);
        if (isDocumentHasChanges(image, data, parameters)) {
          try {
            DbList forUpdate = docsForUpdate(image, newImage, attributes, parameters, importPars, data, useImgImport);
            update(forUpdate, importPars.getCollectionName(), data);
          } catch (Exception e) {
            logger.error("Current image has not saved", e);
          }
        }
      }
    }
  }
}

Пояснение по куску кода (try-catch выкинут для наглядности).

if (!useImgImport || isDocumentHasChanges(image, data, parameters)) {
  // (1)
  DbList forUpdate = docsForUpdate(image, newImage, attributes, parameters, importPars, data, useImgImport);  // (2)
  if (useImgImport) {
    // (3)
    update(forUpdate, importPars.getCollectionName(), data);
  }
}

В (1) мы попадем в случае если useImgImport = false или были изменения в документе
В (2) переменная useImgImport не изменяется
В (3) мы попадем если useImgImport = true

Если объединить условия (1) и (3), то update() выполнится при условии

(!useImgImport || isDocumentHasChanges) && useImgImport

или, раскрывая скобки

(!useImgImport && useImgImport) || (isDocumentHasChanges && useImgImport)

(!useImgImport && useImgImport) всегда равно false и на общее выражение не влияет. Остается только условие (isDocumentHasChanges && useImgImport)