1

A colleague and I are working together on a Meteor app.

One of us thinks that the following code in two places should be wrapped in a function to avoid duplication -- the other thinks that it leads to needless indirection/complexity.

I won't tell you which is me, because I would like the answers to be unbiased :).

We disagree on this particular point and would like some help coming to a conclusion which is beneficial to our codebase.

One file has:

Email.send({
    from: sender,
    to: recipient,
    subject: "Message from our company",
    message: message
});

Another file has the same code:

Email.send({
    from: sender,
    to: recipient,
    subject: "Message from our company",
    message: message
});

Please be keep your comments respectful.

Question: Should this code be abstracted out into a function?

Clarification/Edit: The "Message from our company" is the same in both Email.send calls.

Improve this question
12
  • 5
    That's just an ordinary function call. I don't see any benefit to wrapping it in another function. Those are just method parameters; you could put them into an object and pass the object instead, but then you're just pushing the complexity somewhere else. Verdict: already DRY.
    – Robert Harvey
    Sep 21, 2015 at 17:37
  • 1
    @gnat: That question doesn't seem specific enough to apply here.
    – Robert Harvey
    Sep 21, 2015 at 17:38
  • 2
    If it never changes, just define it as a constant at the top of your code.
    – Robert Harvey
    Sep 21, 2015 at 17:39
  • 1
    @NathanLippi You should have some means of sharing constants between all modules of your program. Having that is probably a prerequisite of any sort of "DRY" refactoring you're going to do anyway.
    – Brandin
    Sep 21, 2015 at 17:42
  • 1
    Then your choices are to live with having the same string in two different places, or providing some mechanism for sourcing the string from a single location. You have to decide for yourself if the benefit of a single source is worth the expense.
    – Robert Harvey
    Sep 21, 2015 at 17:49

5 Answers 5

Reset to default
17

It depends.

Assuming that the subject (and not the sender, for instance) is the only "repeated" parameter here, I'd turn the question back on you: If the subject text changes in one place, does it necessarily change in both?

DRY isn't so much about eliminating the amount you copy+paste. It's actually quite easy to copy and paste! What we aim to avoid is the typical fallout of a copy-paste event: N > 1 lines of code that need to be maintained in perfect unison.

Improve this answer
5
  • 1
    Yes, if it changes in one place it would change in both.
    – Nathan Lippi
    Sep 21, 2015 at 17:44
  • 1
    Then I'd argue it's bad repetition. But, it's between you, your colleague, and the context of this call to determine where you pull that common copy from.
    – svidgen
    Sep 21, 2015 at 17:46
  • 1
    @NathanLippi: Then this is your answer. The point of not repeating yourself is to concentrate the knowledge of how to do something into one place. A function called SendEmailFromUs() wouldn't be unreasonable. Sure, you only have two instances now, but what happens when someone adds more and you don't know about it?
    – Blrfl
    Sep 21, 2015 at 17:47
  • @Blrfl More or less. The point of DRY isn't so much to avoid copy+pasting -- it's actually really easy to copy+paste! The point is to make code maintainable.
    – svidgen
    Sep 21, 2015 at 17:55
  • 3
    @blrfl Maybe it's worth noting that the solution doesn't need to an additional method. It could be a constant, a config file, a database entry, a static dictionary of common strings...
    – svidgen
    Sep 21, 2015 at 18:17
6

I would at least like to have the string constant "Message from our company" consolidated in one place. If this is standard boilerplate, then some UI guy/copy editor is going to want it changed at some point and would want it to be consistent across the app.

Improve this answer
6

I don't think avoiding duplication of "code" is really the reason you should put this into another function but because this "requirement" is a duplication.

Is someone going to say, we no longer want to send an email in this case, but send something else like a Facebook post. This is contingent on why you have this in two code places to begin with. You could be doing it in two places for two entirely different reasons.

Just because two lines of code do the same thing doesn't mean they do it for the same reason. If the requirements indicate these two things should always be the same, put them in the same function.

Improve this answer
1

Generally speaking, the purpose of DRY is to avoid repeated application logic. I don't see any repeated application logic here. Email.send is already a high-level reusable method.

One approach might be to use a constant as the subject.

Improve this answer
0

If its just these two places in the code, I don't think it makes much difference.

But in most projects I've worked on, sending mail is rarely just sending mail. Often it also involves adding headers, merging with templates, generating unique id's, adding unsubscribe links, and logging the message to the database. In those cases, all attempts to send mail are piped into a single function that handles those tasks.

So, if you expect the system to be sending email from more places, or if you expect to eventually need to 'fancy up' the emails that get sent, yes, I'd wrap it in its own function.

Improve this answer

Not the answer you're looking for? Browse other questions tagged or ask your own question.