CODEWITHSUNDEEP
javarefactoring
Bart
Bart

Reputation: 145

Can you get rid of these ifs and make this snippet of code more object oriented

I have found the following task on GitHub. It kind of interested me. Could somebody give me some tips on how to proceed with such tasks? Can you get rid of these ifs and make this snippet of code more object oriented?"

public class TheService {
private final FileHandler fileHandler;
private final FooRepository fooRepository;

public TheService(FileHandler fileHandler, FooRepository fooRepository) {
    this.fileHandler = fileHandler;
    this.fooRepository = fooRepository;
}

public String Execute(final String file) {

    final String rewrittenUrl = fileHandler.getXmlFileFromFileName(file);
    final String executionId = fileHandler.getExecutionIdFromFileName(file);

    if (executionId.equals("") || rewrittenUrl.equals("")) {
        return "";
    }

    Foo knownFoo = fooRepository.getFooByXmlFileName(rewrittenUrl);

    if (knownFoo == null) {
        return "";
    }

    return knownFoo.DoThat(file);
}

}

Upvotes: 2

Views: 364

Answers (3)

gargkshitiz
gargkshitiz

Reputation: 2168

This code looks good already, but I have still done a small bit of refactoring on it.

I guess we could use guard clauses so that further code processing (or nested ifs processing) is done only when it is required. Because of these tiny guard clauses, I don't think a separate object for validating these params is required. And we don't have to worry about having multiple returns, see: Shall I use guard clause, and try to avoid else clause? also.

I have also changed the method name from Execute to execute, reversed String comparisons because of potential Null pointer exceptions, extracted "" as a string constant, renamed rewrittenUrl to xmlFileName and renamed knownFoo to foo. Please note that I have not used StringUtils.isEmpty as that might change the semantics of the code (this method also does a null check, which is not happening in the original code).

private static final String EMPTY = "";

public String execute(final String file) {

    final String executionId = fileHandler.getExecutionIdFromFileName(file);

    if (EMPTY.equals(executionId)) {
        return EMPTY;
    }

    final String xmlFileName = fileHandler.getXmlFileFromFileName(file);

    if (EMPTY.equals(xmlFileName)) {
        return EMPTY;
    }

    Foo foo = fooRepository.getFooByXmlFileName(xmlFileName);

    if (foo == null) {
        return EMPTY;
    }

    return foo.DoThat(file);
}

Upvotes: 2

Thiyagu
Thiyagu

Reputation: 17920

As mentioned in @davidxxx's answer, this code is OOP. But if you want to get rid of the final if you could use Optional for the last part.

Also, you can use isEmpty to check if a string is empty.

if (executionId.isEmpty() || rewrittenUrl.isEmpty()) {
    return "";
}

return Optional.ofNullable(fooRepository.getFooByXmlFileName(rewrittenUrl))
                .map(knownFoo -> knownFoo.DoThat(file))
                .orElse("");

Upvotes: 1

davidxxx
davidxxx

Reputation: 131526

This code is OOP way : it relies on instance methods and it makes objects to collaborate between.
Every conditional statements are not necessary bad.

The conditional statements in the actual code are logic, more particularly some data checks.
It doesn't have any relationship with conditional statements where each branch matches to a specific behavior that could be moved into a specific subclass/implementation.

Upvotes: 1

Related Questions