2
\$\begingroup\$

This is a tags counter that I implemented to a site. The script is supposed to display the number of tags that a user can still input - the limit has been set to 5 items (tagsLeftTmp = 5). The input field $(".bootstrap-tagsinput") has two events attached to it - one event substracts 1 from tagsLeftTmp, the other adds 1.

As you can see, the DRY principle is violated by repeating this line three times:

tagsCounter.html(tagsLeftTmp + ' of ' + tagsLimit + ' tags left');

var tagsLeftTmp = tagsLimit;
var tagsCounter = $("#tags-counter");
tagsCounter.html(tagsLeftTmp + ' of ' + tagsLimit + ' tags left'); 

$(".bootstrap-tagsinput").on('itemAdded', function(e) {       
   tagsLeftTmp = tagsLeftTmp - 1;
   tagsCounter.html(tagsLeftTmp + ' of ' + tagsLimit + ' tags left'); 
   return tagsLeftTmp;
});

$(".bootstrap-tagsinput").on('itemRemoved', function(e) {       
   tagsLeftTmp = tagsLeftTmp + 1;
   tagsCounter.html(tagsLeftTmp + ' of ' + tagsLimit + ' tags left'); 
   return tagsLeftTmp;
}); 

How could I make this code adhere to the DRY principle?

\$\endgroup\$
3
\$\begingroup\$

You could probably encapsulate all that in a function and call the function three times. It would arguably be a slightly worse solution.

You could create a more generic event whenever the tag count is updated. This event is triggered from add and remove. This event will read the current tag count and display it.

You could encapsulate all this in an object RemainingTags which has Add methods, Remove methods and can automatically display the counter. Additionally you could sepparate the view from the model so RemainingTags just keeps the logic and RemainingTagsView will create the associated HTML based on an event triggered by RemainingTags so everything is decoupled.


But really, is it worth it? Why go so great lengths? DRY is a nice principle, especially when having a complicated logic or big code bases. But for 15 lines of code from which 3 are duplicated, with a good reason, why use DRY?

The only logic you would encapsulate would be the addition tagsLeftTmp + ' of ' + tagsLimit + ' tags left' so one could argue that maybe it is better to put all this in a function. Something like getTagsLeftMessage(left, max).

I would argue that the code is best in this state. It is simple enough, it is clear, don't over-engineer it. Take a note or left a comment if you feel like wanting to ensure it will be refactored if it will increase in complexity. But take the refactoring decision then, not now, so you can pick the best solution for that situation!.

What if you create a generic way of displaying that message and then you decide you want to change the color to red when you have one tag left. But then some users will have a bigger tag limit and there will also be automatically added tags by the application which will not count toward this limit, except when the automatically added tag is a tag taken from the user preference in which case it must count. And the color should be bright green if there is no tag currently set but should not color at all for mobile because it ruins the UI there.

You get the point. Don't solve a problem you don't have.

\$\endgroup\$
  • \$\begingroup\$ Yes, I get your point. Nevertheless, this code really stands to my eyes as too repetitive. I think there are too many redundancies. It's not really about this particular code snippet. I asked for review of it to take some lessons I could apply in the future when I'm faced with potentially more complex operations to perform, whrere redundancy should really be avoided. Still, thank you for your sensible reply not being solution to the problem, though. \$\endgroup\$ – luqo33 Mar 17 '15 at 16:23
  • \$\begingroup\$ It could help more to have this question but from a theoretical point of view on programmers.SE where indeed we could find some nice solutions to repetitive code. The first three ideas I've put in this message are actual solution I've used before for places where indeed it made sense so that could answer your real concerns a bit. \$\endgroup\$ – Memleak Mar 18 '15 at 7:50

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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