2
\$\begingroup\$

I developed three methods that have only some similarities, and the three must remain separate. However, it seems there is a significant amount of duplicate code. They basically seek the same list and set a BasicDBObject with a few different attributes.

How can I solve it? Or is acceptable as is? These stretches of duplicate codes don't result in a specific method, but in a "generic method for not having repeated code".

private BasicDBList getAllFiles(String validacaoId, String permalinkId) {
        List<ValidacaoResultado> validacoes = validacaoResultadoDAO.findAllByValidacao(validacaoId);
        if (validacoes == null || validacoes.isEmpty()) {
            throw new NotFoundException("Erro ao gerar permalink: Não foi encontrado a validação dos arquivos.");
        }
        BasicDBList allFiles = new BasicDBList();
        String url = String.format("https://oknok.com.br/api/validacao/%s/%s/permalink/", validacaoId, permalinkId);

        for (ValidacaoResultado validacao : validacoes) {
            BasicDBObject dbObject = new BasicDBObject();
            BasicDBObject atributos = validacao.getArquivo();
            if (validacao.getArquivo() != null) {
                dbObject.append("id", Util.getMd5Time(atributos.get("nome").toString()));
                dbObject.append("url", url.concat(atributos.get("nome").toString()));
                dbObject.append("nome", atributos.get("nomeOriginal"));
                dbObject.append("pathOriginal", atributos.get("pathOriginal"));
                allFiles.add(dbObject);
            }
        }
        return allFiles;
    }

    private BasicDBList getAllFilesWithValidation(String validacaoId, String permalinkId) {
        List<ValidacaoResultado> validacoes = validacaoResultadoDAO.findAllByValidacao(validacaoId);
        if (validacoes == null || validacoes.isEmpty()) {
            throw new NotFoundException("Erro ao gerar permalink: Não foi encontrado a validação dos arquivos.");
        }
        BasicDBList allFiles = new BasicDBList();
        String url = String.format("https://oknok.com.br/api/validacao/%s/%s/permalink/", validacaoId, permalinkId);

        for (ValidacaoResultado validacao : validacoes) {
            BasicDBObject dbObject = new BasicDBObject();
            BasicDBObject atributos = validacao.getArquivo();
            if (validacao.getArquivo() != null) {
                dbObject.append("id", Util.getMd5Time(atributos.get("nome").toString()));
                dbObject.append("url", url.concat(atributos.get("nome").toString()));
                dbObject.append("nome", atributos.get("nomeOriginal"));
                dbObject.append("pathOriginal", atributos.get("pathOriginal"));
                dbObject.append("validacao", validacao.getResultado());
                dbObject.append("status", validacao.getResultadoConsolidado());
                allFiles.add(dbObject);
            }
        }
        return allFiles;
    }

    private BasicDBList noksWithValidation(String validacaoId, String permalinkId) {
        List<ValidacaoResultado> validacoes = validacaoResultadoDAO.findAllByValidacao(validacaoId);
        if (validacoes == null || validacoes.isEmpty()) {
            throw new NotFoundException("Erro ao gerar permalink: Não foi encontrado a validação dos arquivos.");
        }
        BasicDBList noksFiles = new BasicDBList();
        String url = String.format("https://oknok.com.br/api/validacao/%s/%s/permalink/", validacaoId, permalinkId);

        for (ValidacaoResultado validacao : validacoes) {
            BasicDBObject dbObject = new BasicDBObject();
            BasicDBObject atributos = validacao.getArquivo();
            if (validacao.getArquivo() != null && "NOK".equals(validacao.getResultadoConsolidado())) {
                dbObject.append("id", Util.getMd5Time(atributos.get("nome").toString()));
                dbObject.append("url", url.concat(atributos.get("nome").toString()));
                dbObject.append("nome", atributos.get("nomeOriginal"));
                dbObject.append("pathOriginal", atributos.get("pathOriginal"));
                dbObject.append("validacao", validacao.getResultado());
                dbObject.append("status", validacao.getResultadoConsolidado());
                noksFiles.add(dbObject);
            }
        }
        return noksFiles;
    }
\$\endgroup\$
  • \$\begingroup\$ The code is exactly the same for all of your methods, what's the requirement for that ? why not calling only one method? \$\endgroup\$ – IndieTech Solutions May 14 '15 at 19:26
  • 1
    \$\begingroup\$ @Alundrathedreamwalker it looks very similar, but it's not actually the same. The last method has an extra condition in the if, and the first one uses fewer appends. \$\endgroup\$ – tim May 14 '15 at 19:59
3
\$\begingroup\$

I would say that duplication is almost always a bad thing. It's hard to maintain (what if you want to change something? you would have to change it in multiple places), and hard to read (it took me quite a while to find the differences).

You could create an interface to reduce the duplication, and you can extract the creation of the BasicDBObject into it's own method. The interface approach does add some boilerplate code, but can be shortened using Java8 syntax (if you still don't like it, you could also just use the createBaseObject method).

private interface DbObjectCreator {
    BasicDBObject create(ValidacaoResultado validacao, String url);
}

private BasicDBList getAllFilesGeneric(String validacaoId, String permalinkId, DbObjectCreator dbObjectCreator) {
    List<ValidacaoResultado> validacoes = validacaoResultadoDAO.findAllByValidacao(validacaoId);
    if (validacoes == null || validacoes.isEmpty()) {
        throw new NotFoundException("Erro ao gerar permalink: Não foi encontrado a validação dos arquivos.");
    }
    BasicDBList allFiles = new BasicDBList();
    String url = String.format("https://oknok.com.br/api/validacao/%s/%s/permalink/", validacaoId, permalinkId);

    for (ValidacaoResultado validacao : validacoes) {
        allFiles.add(dbObjectCreator.create(validacao, url));
    }
    return allFiles;
}

private BasicDBObject createBaseObject(ValidacaoResultado validacao, String url) {
    BasicDBObject dbObject = new BasicDBObject();
    BasicDBObject atributos = validacao.getArquivo();
    dbObject.append("id", Util.getMd5Time(atributos.get("nome").toString()));
    dbObject.append("url", url.concat(atributos.get("nome").toString()));
    dbObject.append("nome", atributos.get("nomeOriginal"));
    dbObject.append("pathOriginal", atributos.get("pathOriginal"));
    return dbObject;
}

private BasicDBList getAllFiles(String validacaoId, String permalinkId, ) {
    return getAllFilesGeneric(validacaoId, permalinkId,
        new DbObjectCreator() {
            @Override
            BasicDBObject create(ValidacaoResultado validacao, String url) {
                if (validacao.getArquivo() != null) {
                    return createBaseObject(validacao, url);
                }
            }
        });
}

private BasicDBList getAllFilesWithValidation(String validacaoId, String permalinkId, ) {
    return getAllFilesGeneric(validacaoId, permalinkId,
        new DbObjectCreator() {
            @Override
            BasicDBObject create(ValidacaoResultado validacao, String url) {
                if (validacao.getArquivo() != null) {
                    BasicDBObject dbObject = createBaseObject(validacao, url);
                    dbObject.append("validacao", validacao.getResultado());
                    dbObject.append("status", validacao.getResultadoConsolidado());
                    return dbObject;
                }
            }
        });
}

private BasicDBList noksWithValidation(String validacaoId, String permalinkId, ) {
    return getAllFilesGeneric(validacaoId, permalinkId,
        new DbObjectCreator() {
            @Override
            BasicDBObject create(ValidacaoResultado validacao, String url) {
                if (validacao.getArquivo() != null && "NOK".equals(validacao.getResultadoConsolidado())) {
                    BasicDBObject dbObject = createBaseObject(validacao, url);
                    dbObject.append("validacao", validacao.getResultado());
                    dbObject.append("status", validacao.getResultadoConsolidado());
                    return dbObject;
                }
            }
        });
}

Misc

  • declare variables in as small a scope as possible to increase readability. Eg dbObject and atributos aren't needed outside the if.
  • your naming is a bit confusing. When I read WithValidation I would assume that some validation takes place, but it seems that just more attributes are added.
  • BasicDBList also doesn't seem like a good name. It seems that you query your db for some stuff, and this code basically wraps the result into an object (and adds some other stuff). The result doesn't really seem to have anything to do with the database. Something like List<File> would probably make more sense. BasicDBObject would then be File.
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.