Ok, so as the introduction for this series and this first in a series of blogs was done, let us kick off the party. With a simple one, with a nice acronym. DRY or Don’t Repeat Yourself. It is also, much less tho, known as DIE. Or Duplication is Evil. However you look at it, it is making a strong statement by expanding the acronym. Then again, is it black and white, as it claims to be?

Let us start with why we want to enforce this rule in our code base. First and foremost, maintainability. Simply meaning that there is a single place where you need to update your code and you know it will function everywhere where it is used. I often follow a simple rule when applying this principle in my code.

đź’ˇ I see the same code more than once, I make a note of it. Third time, time to do some refactoring.

It keeps me in check, so I don’t start jumping ahead before I even realize why there is a need for “duplication”. What do you mean? I prefer a bit of duplication over overengineering a simple solution for n-different scenarios. This is where my first “anti-pattern” comes from when it comes to this principle. One of the observations over time is that even if slight similarity in the code, developers would reach to make it fit into every solution. That even remotely looks “similar”. This leads to now higher cyclomatic complexity, and overall complexity, let alone cognitive load impact. There is a fine tradeoff here. When code becomes too generic and has more input parameters to “control” the flow, then code is not a duplication. There are fundamental differences between the two (or more), which now you’re cramming into a single piece of monstrosity.

Now, not to go without any examples, below I will give an example of a real-world example. Large code base, big company, huge mess. I honestly thought someone was messing with me originally when they said: We didn’t want to repeat this big model everywhere. Naturally, it will not be an exact model, but a really (and I mean really) small example of it. To illustrate the point of going overboard.

[Table("Users")]
public class User
{
    [Key]
    [DatabaseGenerated(DatabaseGeneratedOption.Identity)]
    [XmlIgnore]
    [JsonIgnore]
    public int Id { get; set; }

    [Required]
    [MaxLength(100)]
    [Column("FullName")]
    [JsonPropertyName("full_name")]
    [XmlElement("FullName")]
    public string Name { get; set; }

    [Required]
    [EmailAddress]
    [JsonPropertyName("email_address")]
    [XmlElement("EmailAddress")]
    public string Email { get; set; }

    [Column(TypeName = "datetime2")]
    [JsonPropertyName("registration_date")]
    [XmlElement("RegistrationDate")]
    public DateTime RegisteredAt { get; set; }

    [NotMapped]
    [JsonPropertyName("status")]
    [XmlIgnore]
    public string Status { get; set; }

    // And a lot (if memory serves me, about 30 in total) more properties, but you get the point
}

Now, the problem is that this interesting piece of code is not where it stops. No, no. It is a root cause for several other choices, that were forced due to it spreading like a disease through the code base. Too many cross-cutting concerns in a single place. To illustrate also the reason for the ICustomJsonConverter in another place that used same object but naminbg was different, here is a small piece of code that was found in there.

// This in another piece of code, in same code base, that used User object but with different naming and structure for some reason
internal class UserJsonConverter : ICustomJsonConverter<User>
{
    public override User ReadJson(JsonReader reader, Type objectType, User existingValue, bool hasExistingValue, JsonSerializer serializer)
    {
        var user = new User();
        var jObject = JObject.Load(reader);

        user.Id = jObject["user_id"].Value<int>();
        user.Name = jObject["user_name"].Value<string>();
        user.Email = jObject["email"].Value<string>();
        user.Status = jObject["user_status"].Value<string>();

        return user;
    }

    public override void WriteJson(JsonWriter writer, User value, JsonSerializer serializer)
    {
        var jObject = new JObject
        {
            ["user_id"] = value.Id,
            ["user_name"] = value.Name,
            ["email"] = value.Email,
            ["user_status"] = value.Status
        };

        jObject.WriteTo(writer);
    }
}

And this is just a small subset of the entire code base that needed to make a workaround around some interesting choices. That could have been easily avoided. Simply, it was being used by N number of different dependencies. They all had their own needs and different interpretations of it. It happens in real life, very often if I may add. While that is a valid reason to make some compromises, this one should not have been made in the example above. For clarification, this is just one of the examples I saw. There are several others, some maybe not doing overkill, and some even worse. There is a saying, not sure who is the original author of it.

đź’ˇ Bad code produces bad code.

And it shows. If you observed not all properties exist (or are used) in the ICustomJsonConverter. Solutions for these are simple while keeping code DRY. Code generation, explicit domain and edge models, etc. I would, honestly go for some generation based on any templating engine out there. Define the model and then configure it how should be outputted in all your edge models. With all of their small differences being encapsulated within the specific generator. Then your code is not the model, it is all different rules for the generation. The generated code is just a “side-effect” of it. I dare you to have these kinds of thoughts around people that are religious around dogmas as shown in the examples. You will never be the same afterward.

Another interesting example of going overboard with DRY is code that looks like it can do everything. Usually the suspects of this “code smell” are implementations that have N number of inputs to control the flow of that “DRY” code.

public void DoSomething(string action, string user, string role, string permission)
{
    if (action == "create")
    {
        if (user == "admin" && role == "admin" && permission == "admin")
        {
            // Do something
        }
        else if (user == "admin" && role == "admin" && permission == "user")
        {
            // Do something
        }
        else if (user == "admin" && role == "user" && permission == "admin")
        {
            // Do something
        }
        else if (user == "admin" && role == "user" && permission == "user")
        {
            // Do something
        }
        else if (user == "user" && role == "admin" && permission == "admin")
        {
            // Do something
        }
        else if (user == "user" && role == "admin" && permission == "user")
        {
            // Do something
        }
        else if (user == "user" && role == "user" && permission == "admin")
        {
            // Do something
        }
        else if (user == "user" && role == "user" && permission == "user")
        {
            // Do something
        }
    }
}

Oversimplified, I am aware. It illustrates the point. Where somehow code looks “similar”, yet has very different side-effects. But who cares, just combine it in one place, and it will be obvious. And then it just keeps growing, having a life of its own. Or even a package. Options are limitless. I often come across situations like this, where we need to retrieve user information from one source or another and check if it can perform certain actions. This is a relatable example for most of us. What happens next is that several things start appearing similar because it’s the same implementation every single time. Instead of refactoring it into a private helper method, we tend to get carried away and mix it with other “similar” but slightly different use cases that follow. Consequently, you end up with a more specific example like the one mentioned above. I have seen numerous CRUD services where that helper method evolves into a controller within a controller.

If the cyclomatic complexity of your code increases due to refactoring, it is a sign that you shouldn’t do it. You’re increasing complexity and if there is no good reason for it, and this is subjective, maybe simple is a way forward. So stop and think about it. And just refactor things that keep the complexity the same as before, just with a nicer alias that discloses its intended purpose. That is a simple rule I have, that is also measurable to support the opinion. And keep in mind, this is just another opinion out there. With experience in other languages and adding some other principles in the mix, solutions can have best of the both worlds. Higher order functions, open-closed principle, etc. They are some examples of how to make your code DRY and give an option to the caller to “customize” certain aspects of it. With a side warning: Just because you’re throwing in more acronyms into the bowl, doesn’t make it taste any better. The balance is delicate and the opinionated “rules” I follow, do make it more critical on what constitutes DRY code.

To sum it up, in a list:

  1. Don’t refactor at first glance, wait to see if the code repeats more than a couple of times. If not, there is no need to move it away from where it is used.
  2. Avoid “generic” DRY code, it increases the complexity and points out that those “similarities” are not as similar as they look. The indicator of this “code smell” is a control flow depending on who is calling the “DRY” code.
  3. There is no one stop for all solutions, it will ask for more code in other places. If you find yourself working around a “DRY” solution, then it is time to rethink. Makes code brittle and impossible to maintain, with hacks spread around to support all the new requirements.

Till next time, keep it DRY.