0

This question is quite general and not related to a specific language, but more to coding best practices.

Recently, I've been developing a feature for my app that is requested in many cases with slightly different behaviours.

This function send emails , but to different receivers, or with different texts according to the parameters. The method signature is something like

public static sendMail (t_message message = null  , t_user receiver = null , stream attachedPiece = null)

And then there are many condition inside the method, like

if(attachedPiece != null)
{
}

I've made the choice to do it this way (with a single method) because it prevents me to rewrite the (nearly) same method 10 times, but I'm not sure that it's a good practice.

What should I have done? Write 10 sendMail method with different parameters?

Are there obvious pros and cons for these different ways of programming?

Thanks a lot.

Krowar
  • 109
  • 2
  • Please consider the question: [Is it OK to split long functions and methods into smaller ones even though they won't be called by anything else?](http://programmers.stackexchange.com/q/195989/40980) –  May 26 '14 at 17:26

3 Answers3

3

is the attachment rarely missing? If so do as you've done. If it's rarely there, make a method without the attachment parameter that calls this method. No need to recode everything a dozen times.

Say with 3 parameters, 1 of which are almost always there, the third rarely, you'd get something like:

method1(param1, param2, param3) {
  // full implementation
}

method1(param1, param2) {
  method1(param1, param2, null);
}
jwenting
  • 9,783
  • 3
  • 28
  • 45
  • I can't find how this is useful, the full implementation is in the method1(param 1 , param2, param 3 = null). And for the use, I can anyway call method1(param1, param2) because the third will automatically be set to null – Krowar May 26 '14 at 13:56
  • 1
    @Krowar which is nice in theory, but now if you have parameters 1, 3, 4, 6, and 8 supplied, you need to supply nulls for parameters 2,5, and 7... – jwenting May 26 '14 at 14:01
  • Ow, ok I see. That's a good point, even though it's not totally true , because you could call the method like this method(param1 = something, param3 = something, ... ) which is quite verbose though =) Thanks for you clarification =) – Krowar May 26 '14 at 14:29
  • @jwenting Unless the arguments type are distinct, having overloads with arguments that are not a prefix of the full argument list will be a problem. That's why I suggested [fluent interface](http://en.wikipedia.org/wiki/Fluent_interface) in my answer. – Idan Arye May 26 '14 at 15:14
2

There is a downside to having all permutations on the parameters in the same method, since you will end up with a high cyclomatic complexity, i.e. there are many paths to traverse your logic, and this is hard to test and reason about. Every time you add a null-able parameter, you increase this complexity by a factor of 2 and it is recommended not to exceed 10, except for rare situations.

The simplest solution to avoid cyclomatic complexity is to refactor into smaller methods. Let me use Python as pseudocode for this simple example:

class MyApp():
    def send_mail(message=None, receiver=None, attachedPiece=None):
        if message:
            # ... do something ...
        if receiver:
            # ... do another thing ...
        if attachedPiece:
            # ... attach to message ...

should be rewritten into:

class MyApp():
    def send_mail(message=None, receiver=None, attachedPiece=None):
        process_body(message, receiver)
        attach_piece(attachedPiece)
    def process_body(message, receiver):
        if message:
            # ... do something ...
        if receiver:
            # ... do another thing ...
    def attach_piece(attachedPiece):
        if attachedPiece:
            # ... attach to message ...

Now instead of a single method with complexity 8, you have two methods of complexities 4 and 2. This is not repeating yourself, since code is not shared between the refactored methods. I grouped two of the conditionals in the original send_mail method into a single refactored method in order to show that this type of refactor does not necessarily lead to one small method for each of the original parameters.

logc
  • 2,190
  • 15
  • 19
1

Yes, it's a good practice, but if you don't feel comfortable with it you might want to consider a fluent interface. That way, you'll be able to put the different behaviours in different methods, but instead of writing a method for each combination of arguments you only need to write a method for each argument.

Idan Arye
  • 12,032
  • 31
  • 40
  • Thanks for your answer. So my way of doing it is a good practice? I thought it made my code more complex, and maybe less testable (i don't do test for the moment, so it's a guess) – Krowar May 26 '14 at 13:42
  • Writing the 10 methods(that don't use each other - otherwise there is no real difference between the approaches) means that each method is less complex, but it also means that you have 9 more methods, so I argue the code will be more complex that way. Also, while it's true that the single method approach uses more `if` statements, but since they are not nested you are only adding one more level of nesting so it doesn't add enough complexity to give a fuss about. – Idan Arye May 26 '14 at 13:48
  • 1
    As for testability - if you used the 10 methods approach, you would have to test 10 methods, and you would have to make some tests with null arguments to see what happens. Now you only have to test one method, and sending null arguments is actually a test of actual desired functionality rather than a test of possible misuse. – Idan Arye May 26 '14 at 13:50