current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote -2 down vote favorite

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>
share|improve this question

closed as off-topic by Mast, Ethan Bierlein, jacwah, SirPython, 200_success♦ Aug 11 at 20:39

This question appears to be off-topic. The users who voted to close gave this specific reason:

If this question can be reworded to fit the rules in the help center, please edit the question.
2  
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. –  Ismael Miguel Aug 11 at 18:11

2 Answers 2

up vote 2 down vote accepted

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());
        }
}
share|improve this answer
    
Thank you for reviewing my code :) –  fishera Aug 11 at 19:43
    
@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. –  SirPython Aug 11 at 20:23
up vote 1 down vote

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.

share|improve this answer

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