Do not create unnecessary methods

Methods should be short, from 1 to 7 lines of code, maximum 10 in some cases (with the exception of some algorithms). Because a method does just one thing its name should describe it precisely. But it should not contain implementation details (like data types). The name of the method is the "what" and the implementation is the "how". Try to avoid implementation details in the name.
When you find yourself using the words "and/or" in your methods names, consider that you might be exposing too many implementation details and try to search for a better name.

Sometimes, people new to this kind of modularity end up creating methods that are not necessary:

  2. private boolean checkIfCharsAreEqual(char[] wordArray,int first_char, int second_char) {
  3. if (wordArray[first_char] != wordArray[second_char]){
  4. return false;
  5. }else{
  6. return true;
  7. }
  8. }

This method does not add any clarity to the code. Moreover boolean methods can just return the condition:

  2. private boolean checkIfCharsAreEqual(char[] wordArray,int first_char, int second_char) {
  3. return (wordArray[first_char] != wordArray[second_char]);
  4. }

There is no need for the "if-else" block when the result of the condition is the return value of the function/method. Even after this change, the method does not make sense. Try to "inline" the method to see whether the code invoking it reads equally good.

When conditionals contains more than one boolean expression (using logic operators AND/OR) then I usually prefer to extract a method so that the reader doesn't have to think, just read and understand:

  2. // before:
  3. if (someCondition(arg1) && !someOtherCondition(arg2))
  5. // after:
  6. if (isSomeCaseWithProperName(arg1, arg2))

When methods are boolean, try to name them so that they read well preceded by the "if" keyword. I like using the prefixes "is" and "are" or "areThere" for boolean methods operating on single variables and collections respectively.

When the new method is just an alias of another method, consider whether adding it adds any value in terms of readability:

  2. result = someVar.toUpperCase(); // is good/clear enough
  4. // Whilst this method does not make sense:
  5. public String convertToUpper(String input){
  6. return input.toUpperCase();
  7. }
  9. // And this method does not bring value:
  10. private void addWordToList(List<String> palindromes, String word) {
  11. palindromes.add(wordToBeChecked);
  12. }
  14. // And this method does not bring any value:
  15. public boolean stringContainsAtLeastOneSpace(String input) {
  16. if (input.contains(" ")){
  17. return true;
  18. }
  19. return false;
  20. }

Also notice that the prefix "string" in the method name is redundant because the parameter is a string. In the case of methods with one or two arguments, consider using names for the arguments that read well together with the method name:

  2. public User findBy(int phoneNumber){
  3. ...
  4. };
  • Andy Palmer

    I slightly disagree with stringContainsAtLeastOneSpace.
    I might use something like:

    assert weHaveASpaceIn(username);


    throw new InvalidPasswordComplaint if weDontHaveASpaceIn(username)

    also, aliasing a method is useful if you’re negating the case (weDontHaveASpace vs weHaveASpace), it can be easier to read than !weHaveASpace (although CamelCase is hard to read anyway 😀 )

  • carlosble

    I do prefer the “not/dont/doesnt” in the method name rather than the “!” operator when the sentences read better. The idea is to make the code understandable for the reader, without thinking (when possible). I don’t know if I would consider that “aliasing” though. I’ve found it’s a bit more difficult to read the implementation when the “negation” is part of the method name, so I usually have the public method with the “negation” and internally it uses the “!” operator in front of a private method that implements the opposite condition.

    In the case of positive assertions, I think that:

    throw new InvalidPasswordComplaint if username.contains(” “)

    reads good enough.

    Thanks Andy 🙂

  • Ronny Springer

    My impression at the first method block was: “what is if first_char and second_char are not part of wordArray?”.

    // Let us take a look at javascript:
    var wordArray = [], firstChar = secondChar = undefined || NaN; // have to write test cases first.
    function hasEqualChars(wordArray, firstChar, secondChar) {
    return (wordArray.firstChar === wordArray.secondChar);
    // marginal note:
    // I think that is really ugly to have a mix of camelcase and underscores (wordArray vs. first_char).
    // I prefer to rename “checkIfCharsAreEqual” to “hasEqualChars” because the method returns a boolean.

    So, for the next steps would it be better to use a getter method for wordArray property and drop the direct depandancy (truning further: to extract the check to a separate validator)? What’s a common way to go on refactoring?
    And my second question is how do I get rid of hidden dependency between my test cases?

  • carlosble

    Ronny I am not sure I understand your points. I definitely prefer camelcase in JavaScript, it’s the convention and I accept it. Considering the context of the method, where firstChar and secondChar are part of the array, the method does not make sense. That’s why context is so important to make design decisions.
    When extracting classes as of refactoring, make sure a single responsibility is not spread out between several classes. A class should have a single responsibility but it should have it all because we want cohesive pieces. So if the responsibility is shared between two classes, that is a code smell too and you should consider joining the two classes. It depends on the context again, remember, how many reasons to change the code are there?

    Regarding the second questions, can you provide a code example?

    Thanks 🙂