patternjavascriptMajor
Don't search for minors, it isn't allowed
Viewed 0 times
allowedsearchforisnminorsdon
Problem
I have a search form that doesn't require much in terms of input parameters, but the Date Of Birth is import for this particular search.
I have been working on creating a way that the form can be intuitive and keep people from attempting to search for a minor.
Is there anything that I can make simpler or more efficient or even more DRY?
How does my JavaScript look?
` $(document).ready(function () {
$("#").click(function () {
var dateOfBirth = new Date($("#").val());
var minAge = new Date();
minAge.setFullYear(minAge.getFullYear() - 18);
if (dateOfBirth > minAge) {
$("#dialog-DOB").dialog("open");
} else {
$("#dialog-confirm").dialog("open");
}
return false;
});
$("#dialog-confirm").dialog({
autoOpen: false,
resizable: false,
height: 'auto',
width: '400px',
modal: true,
buttons: {
"Charge Credit Card": function () {
$(this).dialog("close");
},
Cancel: function () {
$(this).dialog("close");
}
},
show: { effect: "drop", direction: "down" },
hide: { effect: "drop", direction: "up" }
});
//Date Validation
$("#").blur(function () {
var dateOfBirth = new Date(this.value);
var minAge = new Date();
minAge.setFullYear(minAge.getFullYear() - 18);
if (dateOfBirth > minAge) {
$("#dialog-DOB").dialog("open");
return false;
}
return false;
})
$("#dialog-DOB").dialog({
autoOpen: false,
resizable: false,
height: 'auto',
width: '400px',
modal: true,
show: { effect: "drop", direction: "down" },
hide: { effe
I have been working on creating a way that the form can be intuitive and keep people from attempting to search for a minor.
Is there anything that I can make simpler or more efficient or even more DRY?
How does my JavaScript look?
` $(document).ready(function () {
$("#").click(function () {
var dateOfBirth = new Date($("#").val());
var minAge = new Date();
minAge.setFullYear(minAge.getFullYear() - 18);
if (dateOfBirth > minAge) {
$("#dialog-DOB").dialog("open");
} else {
$("#dialog-confirm").dialog("open");
}
return false;
});
$("#dialog-confirm").dialog({
autoOpen: false,
resizable: false,
height: 'auto',
width: '400px',
modal: true,
buttons: {
"Charge Credit Card": function () {
$(this).dialog("close");
},
Cancel: function () {
$(this).dialog("close");
}
},
show: { effect: "drop", direction: "down" },
hide: { effect: "drop", direction: "up" }
});
//Date Validation
$("#").blur(function () {
var dateOfBirth = new Date(this.value);
var minAge = new Date();
minAge.setFullYear(minAge.getFullYear() - 18);
if (dateOfBirth > minAge) {
$("#dialog-DOB").dialog("open");
return false;
}
return false;
})
$("#dialog-DOB").dialog({
autoOpen: false,
resizable: false,
height: 'auto',
width: '400px',
modal: true,
show: { effect: "drop", direction: "down" },
hide: { effe
Solution
This part of the review is focused on the Javascript aspect of the answer
I will be really light on this review.
I will try to make it short and quick.
-
You have some mixed quotes.
Consider the following piece of code:
See the mixed quotes?
-
You have inconsistent quotes.
All your objects have different quote styles.
The same object has properties with quotes and without.
Pick one, stick to it.
-
Unnecessary code.
Observe the following example:
Why the double
-
Library misuse:
You repeat
You can chain everything and it will work. That's what jQuery is for!
Please, chain them.
-
Duplicated event handling.
You have this:
Nothing wrong, but then you have this:
Why? Why you close a
Move the content to the first handler, and you are set.
-
Repeated properties.
You have the following properties:
These values are repeated.
You should use something like this:
But in a more readable way. I can't make it more readable than that.
-
Repeated handlers.
Yes, your handler is the same, with a very small difference, that is fixed by checking the event type and the
Just store the function in a variable and you are done!
Like this:
-
Did I mention library misuse?
Look at the example below:
Notice something?
The use of jQuery just to retrieve the value of an `
You have them in a newline and right after an element.
You should choose one. My preference is keeping them in a newline. It helps a lot to read the code.
See? I told I was going to be short! I hope I've helped you.
I will be really light on this review.
I will try to make it short and quick.
-
You have some mixed quotes.
Consider the following piece of code:
$("#dialog-confirm").dialog({
autoOpen: false,
resizable: false,
height: 'auto',
width: '400px',
modal: true,
[...]See the mixed quotes?
-
You have inconsistent quotes.
All your objects have different quote styles.
The same object has properties with quotes and without.
Pick one, stick to it.
-
Unnecessary code.
Observe the following example:
if (dateOfBirth > minAge) {
$("#dialog-DOB").dialog("open");
return false;
}
return false;Why the double
return? You only need 1 and it is outside that if, since both return the same and there's no more code.-
Library misuse:
You repeat
$("#") 4 times!You can chain everything and it will work. That's what jQuery is for!
Please, chain them.
-
Duplicated event handling.
You have this:
$(document).ready(function () {Nothing wrong, but then you have this:
$(function () {Why? Why you close a
.ready to just repeat yourself, in a different way, on the next line?Move the content to the first handler, and you are set.
-
Repeated properties.
You have the following properties:
autoOpen: false,
resizable: false,
height: 'auto',
width: '400px',
modal: true,
show: { effect: "drop", direction: "down" },
hide: { effect: "drop", direction: "up" }These values are repeated.
You should use something like this:
var defaults = {
autoOpen: false,
resizable: false,
height: 'auto',
width: '400px',
modal: true,
show: { effect: "drop", direction: "down" },
hide: { effect: "drop", direction: "up" }
};
$("#dialog-confirm").dialog(
$.extend({},
defaults,
{
buttons: {
"Charge Credit Card": function () {
$(this).dialog("close");
},
Cancel: function () {
$(this).dialog("close");
}
}
}
)
);But in a more readable way. I can't make it more readable than that.
-
Repeated handlers.
Yes, your handler is the same, with a very small difference, that is fixed by checking the event type and the
id.Just store the function in a variable and you are done!
Like this:
var handler = function (event) {
var dateOfBirth = new Date(this.value);
var minAge = new Date();
minAge.setFullYear(minAge.getFullYear() - 18);
if (dateOfBirth > minAge) {
$("#dialog-DOB").dialog("open");
} else if( event.type == "blur" && this.id == "" ){
$("#dialog-confirm").dialog("open");
}
return false;
});-
Did I mention library misuse?
Look at the example below:
var dateOfBirth = new Date($("#").val());Notice something?
The use of jQuery just to retrieve the value of an `
.
Just remove the jQuery from there! this.value has the same effect at a WAY HIGHER speed, without ANY bloat.
You have it right a few lines below, and I give you credit for that.
I'll end the Javascript review here, since it is already too lengthy.
Notice that I didn't made any attempt on making the changes incremental. I took right from the original code and showed you with the changes. All these changes are left as an exercise to the O.P..
Now, lets focus on the HTML
This won't be long, I promise!
-
You have unterminated HTML entities.
Through all the code, you have   repeated over and over again
You should use it like that --> (notice the missing ;)
-
You're closing your s in weird places
An example, way at the bottom:
You should have one of these:
Pick one and stick with it.
-
Inline styles
While not entirelly bad, you are repeating them.
You could just write a tag with the CSS styles. Or add it to your CSS file.
Imagine that you want to change that padding to 15px. As-is, you have to replace manually. With CSS, you don't have to worry.
-
Inconsistent positioning of your `s.You have them in a newline and right after an element.
You should choose one. My preference is keeping them in a newline. It helps a lot to read the code.
See? I told I was going to be short! I hope I've helped you.
Code Snippets
$("#dialog-confirm").dialog({
autoOpen: false,
resizable: false,
height: 'auto',
width: '400px',
modal: true,
[...]if (dateOfBirth > minAge) {
$("#dialog-DOB").dialog("open");
return false;
}
return false;$(document).ready(function () {$(function () {autoOpen: false,
resizable: false,
height: 'auto',
width: '400px',
modal: true,
show: { effect: "drop", direction: "down" },
hide: { effect: "drop", direction: "up" }Context
StackExchange Code Review Q#98378, answer score: 21
Revisions (0)
No revisions yet.