3

So, I had a coworker complain about how I made the following code:

...
var foo = getKendoDropdown(window.foo);
var bar = getKendoDropdown(window.foo);
var sna = getKendoDropdown(window.sna);
var fu = getKendoDropdown(window.fu);
...
function getKendoDropdownList(controlId) {
    return $('#' + controlId).data("kendoDropDownList")
}

He said that there's no need to make a one-line function when using jQuery, and said I should use the following instead:

...
var foo = $('#' + window.foo).data("kendoDropDownList");
var bar = $('#' + window.bar).data("kendoDropDownList");
var sna = $('#' + window.sna).data("kendoDropDownList");
var fu = $('#' + window.fu).data("kendoDropDownList");
...

However, I know that if I came across the following in C#:

...
var redFord = cars.Where(x => x.Name = "Ford").First(x => x.Colour = "Red");
var redToyota = cars.Where(x => x.Brand = "Toyota").First(x => x.Colour = "Red");
...

...I would tell whoever wrote it that they should stop repeating code and put the WET code into a method:

public static Car FindTheMatchingRed(this IEnumerable<Car> cars, string brand) =>
    cars.Where(x => x.Brand == brand).First(x => x.Colour = "Red");

So, is my coworker correct? If so, why? What makes JavaScript/jQuery different such that in this case DRY does not apply?

UPDATE

I should make my example stronger (and closer to the JS example) by pointing out that in this case, 'red' is meaningful (just as "kendoDropDownList" is meaningful). Maybe this code is required to run for a country in which red cars are illegal. As such, the 'x.Colour = "Red"' logic changes from being accidentally repeated to being meaningfully identical. Which, in my experience/learning, is the condition for whether you want to abstract multiple bits of repeated logic.

4
  • I'm not a huge proponent of currying, but this looks like a good use case. Might post a real answer with this later.
    – user949300
    Aug 2, 2019 at 0:04
  • 2
    Why would you do that in C# - what is the advantage?
    – user253751
    Aug 2, 2019 at 0:14
  • In and of itself, it doesn't really matter. Neither you or your coworker are right (or wrong) in a general sense, because the way to go depends a lot on how stable the perceived consistency (and, by extension, the method you would create) is in the face of changes. If the abstraction provided by the method is stable and useful over time, and can be consistently applied across all call sites, then you should have a method. If not, then you have the situation described in immibis's answer, which is really a consequence of code being only apparently WET. Aug 2, 2019 at 8:09
  • In that spirit, there is some merit in waiting a bit to see which way the code will go before introducing that method (it's the reasoning behind the Rule of Three). On the other hand, at this scale, if it's internal thing, and not on some boundary or interface, it's not that big of a deal if you get is wrong as you can go back and forth relatively easily - as long as you keep code rot under control. Aug 2, 2019 at 8:12

4 Answers 4

2

I see your coworker's point. You went from repeating a relatively short jQuery expression to repeating an only-slightly-shorter function call. I think this is in the size range where creating a function to call repeatedly doesn't buy you much, unless the name is significantly better.

You can remove more of the repetition using something like:

const [foo, bar, sna, fu] = ["foo", "bar", "sna", "fu"].map(getKendoDropdown);

This makes it obvious you are performing the same operation on all the ids, and is significantly shorter enough that I would deem creating the separate function worthwhile. Yes, you could also inline the getKendoDropdown body in a lambda, but I think this formulation feels a lot cleaner because it keeps everything at the same level of abstraction.

2

In my experience, creating the function hurts readability and provides no benefit.

This is only because the function is extremely short, and because it doesn't provide abstraction.

If the function was longer, or more abstract than the code inside it, then I would not be against adding the function.


My opinion on your C# example is the same. You have refactored this:

...
var redFord = cars.Where(x => x.Brand = "Ford").First(x => x.Colour = "Red");
var redToyota = cars.Where(x => x.Brand = "Toyota").First(x => x.Colour = "Red");
...

into this:

...
var redFord = FindBrandedRedCar(cars, "Ford");
var redToyota = FindBrandedRedCar(cars, "Toyota");
...

But why red? The next line might be:

var blueHonda = cars.Where(x => x.Brand = "Honda").First(x => x.Colour = "Blue");

and we can't refactor this into a call to FindBrandedRedCar because we're looking for a blue car. We could create a FindBrandedColouredCar that takes a brand and a colour as arguments:

var redFord = FindBrandedColouredCar(cars, "Ford", "Red");
var redToyota = FindBrandedColouredCar(cars, "Toyota", "Red");
var blueHonda = FindBrandedColouredCar(cars, "Honda", "Blue");

But what if you want to search for a different criteria? All Toyota station wagons? You can't do that with FindBrandedColouredCar, so let's generalize it to FindCarByFields:

var redFord = FindCarByFields(cars, "Brand", "Ford", "Colour", "Red");
var redToyota = FindCarByFields(cars, "Brand", "Toyota", "Colour", "Red");
var blueHonda = FindCarByFields(cars, "Brand", "Honda", "Colour", "Blue");
var toyotaStationWagon = FindCarByFields(cars, "Brand", "Toyota", "Style", "StationWagon");

But how do we find a car with an engine displacement less than 1.5 litres? We can't pass "Engine.TotalDisplacement" to FindCarByFields because it's not a field. So instead, let's make it so you can pass a lambda:

var redFord = FindCarByLambda(cars, x => x.Brand == "Ford" && x.Colour == "Red");
var redToyota = FindCarByLambda(cars, x => x.Brand == "Toyota" && x.Colour == "Red");
var blueHonda = FindCarByLambda(cars, x => x.Brand == "Honda" && x.Colour == "Blue");
var toyotaStationWagon = FindCarByLambda(cars, x => x.Brand == "Toyota" && x.Style == "StationWagon");
var smallEngineCar = FindCarByLambda(cars, x => x.Engine.TotalDisplacementLitres < 1.5);

You know, .NET actually has this method already. It's called First:

var redFord = cars.First(x => x.Brand == "Ford" && x.Colour == "Red");
var redToyota = cars.First(x => x.Brand == "Toyota" && x.Colour == "Red");
var blueHonda = cars.First(x => x.Brand == "Honda" && x.Colour == "Blue");
var toyotaStationWagon = cars.First(x => x.Brand == "Toyota" && x.Style == "StationWagon");
var smallEngineCar = cars.First(x => x.Engine.TotalDisplacementLitres < 1.5);

Oh look, we're right back where we started. Only extract functions if they are actually useful somehow. There are lots of different ways that it can be useful to extract a function, but hiding this sort of trivial duplication is not one of them.


Going back to the Javascript example, you have this:

var foo = getKendoDropdown(window.foo);
var bar = getKendoDropdown(window.foo);
var sna = getKendoDropdown(window.sna);
var fu = getKendoDropdown(window.fu);

There's still duplication here! Wouldn't it be much better to write the code like this?

...
var kendoDropdownLists = getKendoDropdownLists(["foo", "bar", "sna", "fu"]);
...
function getKendoDropdownLists(controlIds) {
    var result = {};
    for (var k = 0; k < controlIds.length; k++) { // pardon me not being up to date with modern JavaScript practices
    {
        var controlId = controlIds[k];
        result[controlId] = $('#' + controlId).data("kendoDropDownList");
    }
    return result;
}
...

Look mom, no duplication at all! But is it better? I don't think so.

5
  • Before reading this answer, I was thinking "that JS function is good, because it centralizes the knowledge how to retrieve a kendoDropDownList", but you made me think if such centralization is really needed in all cases, especially if the knowledge is contained in a single call/line already that can easily be adapted with search/replace if ever needed. Aug 2, 2019 at 8:00
  • "But why red?" I don't expect the 'kendoDropDownList' to ever change, so likewise assume that in my example, we only ever want red cars. Unless we change from a dropdown to a different component, but at that point I'd just make a different method (getKendoMultiSelect or something).
    – Sarov
    Aug 2, 2019 at 13:07
  • "You know, .NET actually has this method already. It's called First:" Yeah, I realized after posting that I could just combine the two. Though keeping them separate makes it more similar to the JS example.
    – Sarov
    Aug 2, 2019 at 13:07
  • @Sarov Then you have duplication between getKendoDropDownList and getKendoMultiSelect. How will you resolve that duplication?
    – user253751
    Aug 4, 2019 at 21:37
  • I suppose I could make a getControlById method, which I suppose would provide the advantage of being easy to change if we stop using jQuery. But I probably wouldn't bother (we're unlikely to ever stop using jQuery and everyone knows what $('#' + id) means). Additionally, doing that is arguably approaching a slippery slope.
    – Sarov
    Aug 6, 2019 at 13:19
1

Based on my experience, I agree with you. The proposed replacement introduces a lot of repitition of the element retrieval logic and the data retrieval logic, both of which might be modified in the future and risks getting out of sync.

More generally, the abstract concepts of software engineering apply regardless of syntax of specific language or tooling. At best, the cosmetic conventions will be different, but the logic we express using that tooling is the logic of your application, not of the programming language.

1

It's possible that he is right from a pure performance point of view, although the browser can optimize that function call away (based on information like https://www.codereadability.com/performance-cost-javascript-function-call-and-foreach/).

However, in this case your code seems more readable and less risky since if the key you are getting data from ("kendoDropDownList") only exists in one place. And if the browser is going to optimize it away, why not make it more readable/maintainable?

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