Is this taking DRY too far?

Question

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.

svidgen · Accepted Answer · 2015-09-21T18:05:16.373

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.

score 6 · Answer 2 · answered Sep 21 '15 at 18:54

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.

score 6 · Answer 3 · answered Sep 21 '15 at 20:28

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.

score 1 · Answer 4 · answered Sep 21 '15 at 17:53

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.

answered Sep 21 '15 at 17:53

Scott

184

score 0 · Answer 5 · answered Sep 21 '15 at 21:18

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.

Is this taking DRY too far?

5 Answers5