1

This question already has an answer here:

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.

marked as duplicate by gnat, user40980, durron597, Ixrec, user22815 Sep 22 '15 at 23:37

This question has been asked before and already has an answer. If those answers do not fully address your question, please ask a new question.

  • 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 '15 at 17:37
  • 1
    @gnat: That question doesn't seem specific enough to apply here. – Robert Harvey Sep 21 '15 at 17:38
  • 2
    If it never changes, just define it as a constant at the top of your code. – Robert Harvey Sep 21 '15 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 '15 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 '15 at 17:49
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.

  • 1
    Yes, if it changes in one place it would change in both. – Nathan Lippi Sep 21 '15 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 '15 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 '15 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 '15 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 '15 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.

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.

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.

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.

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