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.

We have a rule that all of our validation messages must be in a summary, and thus the default "this field is required" doesn't cut it because the messages lose their context in a summary and therefore need specific field indicators.

I have a solution that I like rather well, but it soon became clear that there was a need for messages outside of just the required field (email, URL, custom methods like phoneUS, etc), so I made some additions to my function.

I've been using jQuery for a while, but I'm not an expert in the optimization area, so I wanted to get some expert help on whether the function below could be optimized...my question is, "is there actually a better way to handle custom error messages in a summary?"

$('.required, .email').each(function(index) {
  var $this = $(this);

  var label = (
    $this.is(':radio')
    ? $("label[data-name='"+$this.attr('name')+"']")
    : label = $("label[for='"+$this.attr('id')+"']")
  );

  var customMessages = [{}];

  if($this.hasClass('required')){
    customMessages.required = "'" + label.text() + "' is required.";
  }

  if($this.hasClass('email')){
    customMessages.email = "'" + label.text() + "' has an invalid email address.";
  }

  $this.rules("add", {
    messages: customMessages
  });
});

Here is the jsFiddle: http://jsfiddle.net/GD5nw/1/

share|improve this question
    
In general, you should provide a wrapping element to run your selectors on, rather than traversing the entire DOM. Also, why are you adding an empty object literal to customMessages? You are adding properties to an array. It would make more sense to use a plain object. –  Johan Oct 3 at 16:26

1 Answer 1

This looks fine to me.

I only have 2 minor points:

  1. You could assign the .text() once in the var statements so that your string concats look like customMessages.email = "'" + label + "' has an invalid email address."; This way you are not repeating text() all over the place.
  2. var customMessages = [{}]; looks odd, because you are afterwards assigning straight to the array instead of the empty object. Simply replacing that with var customMessages = {}; works in the fiddle.
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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