Should one-line functions be avoided in jQuery/JavaScript?

Question

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.

Answer 1

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.

Answer 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.

Answer 3

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.

Answer 4

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?