1
\$\begingroup\$

This code contains an array of objects that I loop over and append to hidden input fields in the DOM. As these inputs have no ID, I first select them by name using $.each and then check the value by looping over the array using another $.each. If there is a match, I then append some HTML to the DOM.

It works fine, but I have doubts about the code, especially where I am using two $.each. Is there a better way of doing this?

var messages = [
            {
                "code":"203294641",
                "message":"A night costs 332 miles"
            },
            {
                "code":"203294642",
                "message":"The night is dark and full of terrors. Costs 32 miles."
            },
            {
                "code":"203294643",
                "message":"The night is dark and full of terrors. Costs 67 miles."
            },
            {
                "code":"203294644",
                "message":"The night is dark and full of terrors. Costs 423 miles."
            },
            {
                "code":"203294645",
                "message":"The night is dark and full of terrors. Costs 431 miles."
            },
            {
                "code":"203294646",
                "message":"The night is dark and full of terrors. Costs 76 miles."
            }
        ];



    $('input[name="code"]').each(function(){
        var inp = $(this);
        $.each(messages, function(i) {
            if (inp.val() == messages[i].code){
                inp.closest('.plan').find('.loyalty').append('<div class="frequent-flyer-message">' + messages[i].message + '</div>');
            }
        });
    });
\$\endgroup\$
0
4
\$\begingroup\$

It would be much easier to store the code/message pairs in an object, then no loop is needed:

var messagesByCode = {
  "203294641": "A night costs 332 miles",
  "203294642": "The night is dark and full of terrors. Costs 32 miles."
  // etc.
};

$('input[name="code"]').each(function(){
  var inp = $(this);
  inp.closest('.plan').find('.loyalty').append('<div class="frequent-flyer-message">' + messagesByCode[inp.val()] + '</div>');
});

If you can't change the structure, generate the better structure once at the beginning:

var messagesByCode = messages.reduce(function(object, item) {object[item.code] = item.message}, {});

EDIT: Some more remarks.

Another thing that should be considered is the point of this code. Why are you doing this? Why only once at the beginning? What is the bigger picture?

The reason I ask is, because this looks like something that should happen server-side. Do the input values change client-side? If yes you should structure your code in a way that allows you to update a single input.

What does your HTML look like? Why isn't the <div class="frequent-flyer-message"></div> part of the existing HTML?

I'm not really fond of the hard coded "path" to the target element (....closest('.plan').find('.loyalty')...). I generally prefer to write JavaScript as generic (and reuseable) as possible, but without the bigger picture I can't think of a good alternative right now.

\$\endgroup\$
3
  • \$\begingroup\$ Thanks. I do agree with you. Thing is, I'm not allowed to change how messages is structured. \$\endgroup\$ – Jose the hose Jul 28 '16 at 13:06
  • \$\begingroup\$ I've added an alternative. But there should be the question: Why can't you change the structure? Well written code also requires well structured data. \$\endgroup\$ – RoToRa Jul 28 '16 at 13:47
  • \$\begingroup\$ @Josethehose I've added some more remarks. \$\endgroup\$ – RoToRa Jul 28 '16 at 14:03
1
\$\begingroup\$

Try this ;)

$('input[name="code"]').each(function() {
    var inp = $(this);

    /* You can put value in variable so no need to call val() function every time in each if condition */
    var value = inp.val(); 
    $.each(messages, function(i) {
        if (value == messages[i].code) {
            inp.closest('.plan').find('.loyalty').append('<div class="frequent-flyer-message">' + messages[i].message + '</div>');
        }
    });
});

If you are not allowed to change how messages is structure

var messages = [{
    "code": "203294641",
    "message": "A night costs 332 miles"
}, {
    "code": "203294642",
    "message": "The night is dark and full of terrors. Costs 32 miles."
}, {
    "code": "203294643",
    "message": "The night is dark and full of terrors. Costs 67 miles."
}, {
    "code": "203294644",
    "message": "The night is dark and full of terrors. Costs 423 miles."
}, {
    "code": "203294645",
    "message": "The night is dark and full of terrors. Costs 431 miles."
}, {
    "code": "203294646",
    "message": "The night is dark and full of terrors. Costs 76 miles."
}];

/* before using messages variable change it to key=>value object */
/* suggested by @rotora  */
var messagesObj = {};
$.each(messages, function(i){
  messagesObj[messages[i].code] = messagesObj[messages[i].message];
});

/* suggested by @rotora  */    
$('input[name="code"]').each(function(){
  var inp = $(this);
  inp.closest('.plan').find('.loyalty').append('<div class="frequent-flyer-message">' + messagesObj[inp.val()] + '</div>');
});
\$\endgroup\$

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.