# 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 ```java boolean checkPassword(String pwd) { if( pwd.equals(„secret”) ){ createSession(); // Unexpected side effect return true; } else { return false; } } ``` Example: Bad: Unintentional return values ```java int createProcess(); // Which one is correct? errorCode = createProcess() processNumber = createProcess() processCount = createProcess() createdInMillis = createProcess() ``` Example: Bad: Method doing more than one thing ```java // 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 ```java // 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)* ```java // 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) ```java 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 ```java // 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 ```java // Overloads doing different things // relevant details are hidden (what lists are created) List scores = initScores(10, 0); List scores = initScores(10); ``` Example: Good: No overloads and Hiding ```java // Method intention is expressed in their names // only irrelevant details are hidden (how lists are created) List scores = createListWithEqualNumbers(10, 0); List scores = createListWithDifferentNumbers(10); ``` ### Two types of methods * "Coordinator" or "orchestrator" * "Technical" or "Algorithm" Example: Good: Types of methods ```java // 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 ```java @Override public List loadOptionDeadlineRemindersByPoses(List gdUserPoses) { UriComponentsBuilder componentsBuilder = UriComponentsBuilder.fromHttpUrl(GSP_ROOT_URI + SpsRestEndpoint.OPTION_DEADLINE_REMINDERS_GD.getValue()); componentsBuilder.queryParam("gdUserPoses", gdUserPoses.toArray()); ResponseEntity> responseEntity = restTemplate.exchange( componentsBuilder.build().toString(), HttpMethod.GET, null, new ParameterizedTypeReference>() {}, Collections.emptyMap()); return responseEntity.getBody(); } @Override public List loadNameDeadlineRemindersByPoses(List gdUserPoses) { UriComponentsBuilder componentsBuilder = UriComponentsBuilder.fromHttpUrl(GSP_ROOT_URI + SpsRestEndpoint.NAME_DEADLINE_REMINDERS_GD.getValue()); componentsBuilder.queryParam("gdUserPoses", gdUserPoses.toArray()); ResponseEntity> responseEntity = restTemplate.exchange( componentsBuilder.build().toString(), HttpMethod.GET, null, new ParameterizedTypeReference>() {}, Collections.emptyMap()); return responseEntity.getBody(); } @Override public List loadTicketingDeadlineRemindersByPoses(List gdUserPoses) { UriComponentsBuilder componentsBuilder = UriComponentsBuilder.fromHttpUrl(GSP_ROOT_URI + SpsRestEndpoint.TICKETING_DEADLINE_REMINDERS_GD.getValue()); componentsBuilder.queryParam("gdUserPoses", gdUserPoses.toArray()); ResponseEntity> responseEntity = restTemplate.exchange( componentsBuilder.build().toString(), HttpMethod.GET, null, new ParameterizedTypeReference>() {}, Collections.emptyMap()); return responseEntity.getBody(); } ``` Example: Good: No repeating technical details ```java import static com.lhsystems.sales.gst.services.spsaccessor.rest.SpsRestEndpoint.*; @Override public List loadOptionDeadlineRemindersByPoses(List gdUserPoses) { return loadDeadlineRemindersByPoses(OPTION_DEADLINE_REMINDERS_GD, gdUserPoses); } @Override public List loadNameDeadlineRemindersByPoses(List gdUserPoses) { return loadDeadlineRemindersByPoses(NAME_DEADLINE_REMINDERS_GD, gdUserPoses); } @Override public List loadTicketingDeadlineRemindersByPoses(List gdUserPoses) { return loadDeadlineRemindersByPoses(TICKETING_DEADLINE_REMINDERS_GD, gdUserPoses); } private List loadDeadlineRemindersByPoses(SpsRestEndpoint endpoint, List gdUserPoses) { UriComponentsBuilder componentsBuilder = UriComponentsBuilder.fromHttpUrl(GSP_ROOT_URI + endpoint.getValue()); componentsBuilder.queryParam("gdUserPoses", gdUserPoses.toArray()); return loadDeadlineReminderMessages(componentsBuilder); } private List loadDeadlineReminderMessages(UriComponentsBuilder componentsBuilder) { ResponseEntity> responseEntity = restTemplate.exchange( componentsBuilder.build().toString(), HttpMethod.GET, null, new ParameterizedTypeReference>() {}, 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 :-) --- # Agent Instructions: Querying This Documentation If you need additional information that is not directly available in this page, you can query the documentation dynamically by asking a question. Perform an HTTP GET request on the current page URL with the `ask` query parameter: ``` GET https://petozoltan.gitbook.io/clearcode/clean-code/clean-code-outline/methods.md?ask= ``` The question should be specific, self-contained, and written in natural language. The response will contain a direct answer to the question and relevant excerpts and sources from the documentation. Use this mechanism when the answer is not explicitly present in the current page, you need clarification or additional context, or you want to retrieve related documentation sections.