Does my code violate DRY principle?

Question

Suppose that I have two services Person Service and Company Service and I want to maintain links between them for example a Person is linked to Company because he works there or he owns the company etc. So I will go ahead and create a database table like

PersonId, CompanyId, RelationType

Now the business logic can be written in either of the services that client will call to link them. But what if I have requirement to link multiple person to a single company and multiple companies to a single person. I need to have two methods, one that takes single PersonId and list of CompanyIds and one that takes single CompanyId and multiple PersonIds. So I have written two different methods in each service. Following method is in Person Service

void LinkPersonToCompanies(long personId,
                           IEnumerable<long> companyIds,
                           RelationType type)

and below method is in Company Service

void LinkCompanyToPersons(long companyId,
                          IEnumerable<long> personIds,
                          RelationType type)

I know that these are two different methods but they are doing the same task i.e. their logic repeats itself. It can become tough to maintain because a single change in linking mechanism should now be made to both methods. Is it against DRY principle? What should be the right design to efficiently solve this problem?

Answer 1

I know that these are two different methods but they are doing the same task

No they aren't. One is linking a person to a company. The other links a company to a person. They may use the same implementation to achieve these two tasks, but they are not the same task.

Let's assume though that the code in each method is identical. Is DRY violated? Yes, because you have repeated yourself and you have created two copies of the code that need to be maintained. But that may not matter. After all, if you pull the code out into a third method and have the first two call it, you have now created tight coupling between the original two methods and that third one. To clarify here, I'm saying that each of the two original methods are now coupled to the third one; they aren't coupled to each other though. Is it really worth creating that coupling just to avoid repeating yourself?

DRY is the enemy of loose coupling. Sometimes it's worth repeating yourself to avoid coupling. Sometimes it's worth creating tight coupling to avoid repeating yourself. Use your judgement to decide which is the better option in your case.

Answer 2

If you didn't have the relationship type you could refactor this to

LinkCompaniesToPersons)(
    IEnumerable<string> companyIds, 
    IEnumerable<string> personIds
    );

and save yourself some code.

With the type in there, its a bit more than a many to many relationship. You could have:

LinkCompaniesToPersons)(
    IEnumerable<CompanyPersonTypeRelationShip>
    );

Which is more generic, but adds code at every call site.

I would go with a combined approach. Have my second method as a private method on the repository and call it via your current easy to read public methods. Then you save code repetition in both the repository and the call sites.

Answer 3

Does having these as two separate functions increase readability (of the client code, i.e. is the code that calls them more readable if you leave them "duplicated")? If yes, I say keep them separate.

You can always add a throw (private) function that will perform the actual logic and have these two functions delegate to it - then you won't have duplication but without decreasing readability of the code that uses your interface.

Answer 4

It depends on your implementation of LinkPersonToCompanies and LinkCompanyToPersons. As it is both functions have slightly different jobs - one links multiple persons to a company, and the other links multiple companies to a person.

If you reuse the same code (as replicating the logic or shudder just copy-pasting the code) - then this is a violation of the DRY principle.

If you, however have them delegate the task (to a common function, which might even be one of the two functions itself) then it is not a violation of the DRY principle, as there are two distinct tasks (as said before) but you kept the common things, well, common.

For example LinkCompanyToPersons could call LinkPersonToCompanies once for each person (or vice versa), or both could call LinkPersonAndCompany once for each person or company (depending on the calling function).

Answer 5

What should be the right design to efficiently solve this problem?

Try to avoid bi-directional associations.

You mentioned in the comments that you have a Person object with a Companies collection and a Company with a Persons collection. Even if there are two use cases, with one screen that allows to add a company to the list of a person's companies and one screen to add a person to the list of a company's persons, that doesn't mean your persistent objects should reflect that.

Bi-directional associations can cause synchronization problems. If a Company and a related Person are loaded at the same time in memory as part of the same unit of work, you have to remember to update both sides as soon as their relationship changes.

Turning this into a one-directional association not only alleviates that issue but also forces you to choose which entity is primary and which is secondary. It makes the code much simpler to reason about because you know there's a single entry point to changes in the relationship, and only it needs to be loaded up in memory. Only one LinkXtoY service method needs to be implemented and you don't have any code duplication issues.