-2
\$\begingroup\$

I would like to make this code more elegant and have it written in a more standard manner, but I cannot write anything better with my current JQuery knowledge. So I would appreciate any suggestions to refactor the code below for better performance.

(function ($) { var Doc = function () { this._init(); }; Doc.prototype = { _init: function () { $( document ).ready(function() { $('#language').change(function(){ Doc.prototype.commonMethod(); }).change(); }); $(document).ajaxComplete(function(){ Doc.prototype.commonMethod(); }); }, commonMethod: function() { var $prefLanguage = $('.editable-select-holder').find('#preferredLanguage').val(); if( $prefLanguage.length != 0){ $('.editable-select-holder').find('#language').val($('.editable-select-holder').find('#' + $prefLanguage).text()); } } }; new Doc(); })(jQuery);

HTML:

<form id="userInfoForm" action="/service/company/user/edit.do?id=2924" method="post"> <h3 class="colored">User details</h3> <table class="settings-edit-table"> <tbody> <tr> <th scope="row">E-mail </th> <td> <input type="email" name="email" id="email" value="[email protected]"></td> </tr> <tr> <th scope="row">Language </th> <td> <div class="editable-select-holder"> <input type="text" name="language" id="language" class="js-focus-expander" readonly=""> <button type="button" class="editable-select-expander"></button> <ul class="editable-select-options"> <li id="en">English </li> <li id="et">Estonian </li> <li id="lv">Latvian </li> <li id="lt">Lithuanian </li> </ul> <input type="text" name="preferredLanguage" id="preferredLanguage" value="en" hidden=""> </div> </td> </tr> </tbody> </table> </form>
\$\endgroup\$
1
  • 2
    \$\begingroup\$ Welcome to Code Review. Please, add some context on what is the purpose of this code and explain where and how you will use it. \$\endgroup\$ Commented Aug 11, 2015 at 18:11

2 Answers 2

Reset to default
2
\$\begingroup\$

Whitespace consistency

Following a standard (even if it's your own) makes code easier to read.

$( document ).ready(function() {...} $(document).ajaxComplete(function(){...}

Better selection

In commonMethod you use jQuery.find() in containers for items with ids 3 times:

$('.editable-select-holder').find('#preferredLanguage') $('.editable-select-holder').find('#language') $('.editable-select-holder').find('#' + $prefLanguage)

Ids should be unique in a given page.

Truthy/Falsy Values

0 is a Falsy value making (varible).length != 0 unecesarry, unless the length property would hold another valid falsy value for any reason

commonMethod: function() { var $prefLanguage = $('#preferredLanguage').val(); if($prefLanguage.length){ $('#language').val($('#' + $prefLanguage).text()); } }
\$\endgroup\$
1
  • \$\begingroup\$ @fishera The default text of a comment says, "Use comments to ask for more information or suggest improvements. Avoid comments like "+1" or "thanks"". Please follow this advice. \$\endgroup\$ Commented Aug 11, 2015 at 20:23
1
\$\begingroup\$

I'd say there is too much complexity for what it needs to do. The Doc-'class' isn't adding much (instance isn't even used), and id's should only exist once. So with that assumption, whats wrong with just simply:

(function ($) { var commonMethod = function() { var prefLanguage = $('#preferredLanguage').val(); if( prefLanguage.length != 0){ var perfLanguageValue = $('#' + prefLanguage).text(); $('#language').val(perfLanguageValue); } }; $(document).ready(function() { $('#language').change(function(){ commonMethod(); }).change(); }); $(document).ajaxComplete(function(){ commonMethod(); }); })(jQuery);

I don't think there are a lot of performance issues here. You can always extract $('...') calls and place them more globally (getting rid of the lookups if the html doesn't change). I wouldn't worry about that here though, this code probably isn't executed too often to make a difference.

\$\endgroup\$

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.