Methods

Rules

• "Small" - Not an exact rule

• Do only one thing - even small methods may do more things!

• One level of abstraction - See Law of Demeter later

• No unexpected behavior (side effects)

• No unexpected or not-understandable input and return values

• No or the least possible dependencies

• Avoid inline implementation - put it always in a separate method

• Set visibility modifiers precisely - For the compiler and for the reader too

• Check parameter validity at the beginning - Item 38 in Effective Java (2nd Edition, Joshua Bloch, 2008)

• Prevent overriding non-abstract methods - Item 17 in Effective Java (2nd Edition, Joshua Bloch, 2008)

Example: Bad: Unexpected behavior

boolean checkPassword(String pwd) {
    if( pwd.equals(„secret”) ){
        createSession(); // Unexpected side effect
        return true;
    } else {
        return false;
    }
}

Example: Bad: Unintentional return values

    int createProcess();

    // Which one is correct?
    errorCode = createProcess()
    processNumber = createProcess()
    processCount = createProcess()
    createdInMillis = createProcess()

Example: Bad: Method doing more than one thing

// Are the lines in the proper order?
public synchronized void flush() throws IOException {
    getContentBuffer().flush();
    IOUtils.copy(new StringReader(getContentBuffer().getBuffer().toString()), getBufferedWriter());
    getBufferedWriter().flush();
    contentBuffer = new StringWriter();
}

Example: Good: Methods doing one thing

// Method complies with the contract
@Override // This is also important, not only for the compiler
public synchronized void flush() throws IOException {
    flushMyBuffer();
    getBufferedWriter().flush();
}

private synchronized void flushMyBuffer() throws IOException {
    contentBuffer.flush();
    IOUtils.copy(new StringReader(getContentBuffer().getBuffer().toString()), getBufferedWriter());
    contentBuffer = new StringWriter();
}

// Now it is clear that it has two buffers. Is is a good solution?
// Clean code is not the final goal, it is a way to the goal:
// is the code correct?

Example: Bad: Method doing more than one thing (from the book)

// It does three things
public void pay() {
    for (Employee e : employees) {
        if (e.isPayday()) {
            Money pay = e.calculatePay();
            e.deliverPay(pay);
        }
    }
}

Example: Good: Methods doing one thing (from the book)

public void pay() {
    for (Employee e : employees) {
        payIfNecessary(e);
    }
}

private void payIfNecessary(Employee e) {
    if (e.isPayday()) {
        calculateAndDeliverPay(e);
    }
}

private void calculateAndDeliverPay(Employee e) {
    Money pay = e.calculatePay();
    e.deliverPay(pay);
}

Example: Bad: Side effect: modifying the input parameter

// Incorrect name
private SelectedDirectionDTO createSelectedOption(OptionDTO option, String fareFamily) {

    SelectedDirectionDTO so = new SelectedDirectionDTO();
    so.setId(option.getId());
    so.setOrigin(option.getOrigin());
    so.setOriginCountry(option.getOrginCountry());
    so.setDestination(option.getDestination());
    so.setDepartureDate(option.getDepartureDate());
    so.setDepartureTime(option.getDepartureTime());
    so.setArrivalDate(option.getArrivalDate());
    so.setArrivalTime(option.getArrivalTime());
    so.setMultipleOrigin(option.isMultipleOrigin());
    so.getFlights().addAll(option.getFlights());

    // Non-standard TODO comment
    // Not informative comment
    //FB_TODO: the code from here might need changes
    // Negative condition first: harder to understand
    if (!moreOptions) {
        // Inline implementation
        for (RoutingFareDTO cFare : option.getFares()) {
            if (routingFareMatches(fareFamily, cFare)) {
                so.setFare(cFare);
            }
        }
    } else {
        // Inline implementation
        String fam = famMoreOptionMapping.get(fareFamily);
        if (fam != null) {
            for (RoutingFareDTO fare : option.getFares()) {
                if (fam.equalsIgnoreCase(fare.getCompartment())) {
                    so.setFare(fare);
                    // Modifying the input parameter!
                    so.getFare().setFareFamily(fareFamily);
                }
            }
        }
    }
    return so;
}

Avoid Bad Input Parameter Types

• Object

• String for non-textual types

• boolean for switches - use constants or enum instead

Program to Methods

Stateless, independent methods:

• Prefer return values to modifying input variables

• Prefer methods that depend only on the input parameters -No configuration parameters or data mining inside

• Independent = easily testable

At the end of the method calls there should always be a method that depend only on the input parameter and whose only result is the return value (or exception).

Overloading

• Use overloading only for convenience

• For example for default values

• Do not use it for hiding differences

• They should call each other - code smell if they don't

Hiding

• Hide irrelevant details in a separate method

• Do not hide relevant details in a separate method

Example: Bad: Overloads and Hiding

// Overloads doing different things
// relevant details are hidden (what lists are created)
List<Integer> scores = initScores(10, 0);
List<Integer> scores = initScores(10);

Example: Good: No overloads and Hiding

// Method intention is expressed in their names
// only irrelevant details are hidden (how lists are created)
List<Integer> scores = createListWithEqualNumbers(10, 0);
List<Integer> scores = createListWithDifferentNumbers(10);

Two types of methods

• "Coordinator" or "orchestrator"

• "Technical" or "Algorithm"

Example: Good: Types of methods

// Orchestrator method - Easy to read - Business logic
public void modifySomething(Something s) {
    s.setAnything( s.getThis(), s.getThat() );
    if( s.isBig() ) {
        s.setSomethingElse( DEFAULT_SOMETHING_ELSE );
    }
}

// Algorithm method - Easy to test
public Anything calculateAnything(This this, That that) {
    return this.getCount() * that.getSize();
}

How to use these types:

• Do not mix business lines with technical lines

• Do not pollute the code with technical details

• Hide irrelevant details

• Do not hide relevant details

Example: Bad: Pollution by repeating irrelevant technical details

@Override
public List<DeadlineReminderMessageDTO> loadOptionDeadlineRemindersByPoses(List<String> gdUserPoses) {
    UriComponentsBuilder componentsBuilder = UriComponentsBuilder.fromHttpUrl(GSP_ROOT_URI + SpsRestEndpoint.OPTION_DEADLINE_REMINDERS_GD.getValue());
    componentsBuilder.queryParam("gdUserPoses", gdUserPoses.toArray());
    ResponseEntity<List<DeadlineReminderMessageDTO>> responseEntity = restTemplate.exchange(
            componentsBuilder.build().toString(),
            HttpMethod.GET, 
            null, 
            new ParameterizedTypeReference<List<DeadlineReminderMessageDTO>>() {}, 
            Collections.emptyMap());
    return responseEntity.getBody();
}

@Override
public List<DeadlineReminderMessageDTO> loadNameDeadlineRemindersByPoses(List<String> gdUserPoses) {
    UriComponentsBuilder componentsBuilder = UriComponentsBuilder.fromHttpUrl(GSP_ROOT_URI + SpsRestEndpoint.NAME_DEADLINE_REMINDERS_GD.getValue());
    componentsBuilder.queryParam("gdUserPoses", gdUserPoses.toArray());
    ResponseEntity<List<DeadlineReminderMessageDTO>> responseEntity = restTemplate.exchange(
            componentsBuilder.build().toString(), 
            HttpMethod.GET, 
            null, 
            new ParameterizedTypeReference<List<DeadlineReminderMessageDTO>>() {}, 
            Collections.emptyMap());
    return responseEntity.getBody();
}

@Override
public List<DeadlineReminderMessageDTO> loadTicketingDeadlineRemindersByPoses(List<String> gdUserPoses) {
    UriComponentsBuilder componentsBuilder = UriComponentsBuilder.fromHttpUrl(GSP_ROOT_URI + SpsRestEndpoint.TICKETING_DEADLINE_REMINDERS_GD.getValue());
    componentsBuilder.queryParam("gdUserPoses", gdUserPoses.toArray());
    ResponseEntity<List<DeadlineReminderMessageDTO>> responseEntity = restTemplate.exchange(
            componentsBuilder.build().toString(),
            HttpMethod.GET, 
            null, 
            new ParameterizedTypeReference<List<DeadlineReminderMessageDTO>>() {}, 
            Collections.emptyMap());
    return responseEntity.getBody();
}

Example: Good: No repeating technical details

import static com.lhsystems.sales.gst.services.spsaccessor.rest.SpsRestEndpoint.*;

@Override
public List<DeadlineReminderMessageDTO> loadOptionDeadlineRemindersByPoses(List<String> gdUserPoses) {
    return loadDeadlineRemindersByPoses(OPTION_DEADLINE_REMINDERS_GD, gdUserPoses);
}

@Override
public List<DeadlineReminderMessageDTO> loadNameDeadlineRemindersByPoses(List<String> gdUserPoses) {
    return loadDeadlineRemindersByPoses(NAME_DEADLINE_REMINDERS_GD, gdUserPoses);
}

@Override
public List<DeadlineReminderMessageDTO> loadTicketingDeadlineRemindersByPoses(List<String> gdUserPoses) {
    return loadDeadlineRemindersByPoses(TICKETING_DEADLINE_REMINDERS_GD, gdUserPoses);
}

private List<DeadlineReminderMessageDTO> loadDeadlineRemindersByPoses(SpsRestEndpoint endpoint, List<String> gdUserPoses) {
    UriComponentsBuilder componentsBuilder = UriComponentsBuilder.fromHttpUrl(GSP_ROOT_URI + endpoint.getValue());
    componentsBuilder.queryParam("gdUserPoses", gdUserPoses.toArray());
    return loadDeadlineReminderMessages(componentsBuilder);
}

private List<DeadlineReminderMessageDTO> loadDeadlineReminderMessages(UriComponentsBuilder componentsBuilder) {
    ResponseEntity<List<DeadlineReminderMessageDTO>> responseEntity = restTemplate.exchange(
            componentsBuilder.build().toString(),
            HttpMethod.GET, 
            null, 
            new ParameterizedTypeReference<List<DeadlineReminderMessageDTO>>() {}, 
            Collections.emptyMap());
    return responseEntity.getBody();
}

Method Smells

• Passing this to a method - it could be implemented with more, smaller and readable classes.

If you can mix a method's lines and it still compiles, it probably does more things. (Zoltan Peto :-)