6
\$\begingroup\$

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 () {
        $("#<%=btnSearch.ClientID %>").click(function () {
            var dateOfBirth = new Date($("#<%=txtDate.ClientID%>").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
        $("#<%=txtDate.ClientID %>").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: { effect: "drop", direction: "up" },
            buttons: {
                "Okay": function () {
                    $("#<%=txtDate.ClientID %>").val("");
                    $(this).dialog("close");
                }
            }
        });
    });

    $(function () {
        $("#<%=txtDate.ClientID%>").datepicker({
            maxDate: '-18y'
        });
        $("#<%=txtDate.ClientID%>").mask("99/99/9999", { placeholder: "mm/dd/yyyy" });
    })
<table>
        <tr>
            <td>
                <asp:Label runat="server" ID="lblLastName" Text="Last Name:"></asp:Label>
                <sup class="required">*required</sup>
                &nbsp&nbsp&nbsp</td>
            <td>
                <asp:TextBox runat="server" ID="txtLastName" CssClass="textbox"></asp:TextBox></td>
            <td>
                <asp:Label runat="server" ID="lblDOB" Text="Date of Birth:"></asp:Label><sup class="required">*required</sup></td>
            <td>
                <asp:TextBox runat="server" ID="txtDate" CssClass="textbox"></asp:TextBox>

                <div id="dialog-DOB" title="You cannot search for a minor">
                    <p>
                        <span class="ui-icon ui-icon-alert" style="float: left; margin: 0 7px 20px 0;"></span>
                        You cannot perform a record search for a minor, please change the Date of Birth.
                    </p>
                </div>
            </td>
        </tr>
        <tr>
            <td>
                <asp:Label runat="server" ID="lblLastNameAlias" Text="Last Name Alias:"></asp:Label>
                <sup class="optional">*optional</sup></td>
            <td>
                <asp:TextBox runat="server" ID="txtAliasName" CssClass="textbox"></asp:TextBox></td>

        </tr>
        <tr>
            <td>Select Case Type to Include in Search</td>
            <td>
                <asp:CheckBox runat="server" ID="cbxProtection" Text="Protection Orders" /></td>
            <td>
                <asp:CheckBox runat="server" ID="cbxCriminal" Text="Criminal" /></td>
            <td>
                <asp:CheckBox runat="server" ID="cbxBoth" Text="Both" /></td>
        </tr>
    </table>
    <asp:Button ID="btnSearch" runat="server" Text="Search" />

    <div id="dialog-confirm" title="Confirm Credit Card Purchase">
        <p>
            <span class="ui-icon ui-icon-alert" style="float: left; margin: 0 7px 20px 0;"></span>
            Your Credit card will be charged if you proceed, whether or not a record is found.
        </p>
    </div>
\$\endgroup\$
  • 5
    \$\begingroup\$ and you will get your credit card charged That's a terrible idea. What if somebody misclicks? \$\endgroup\$ – Pikamander2 Jul 29 '15 at 1:58
  • 10
    \$\begingroup\$ I saw the confirmation. It's still a bad idea. Charging somebody and then not giving them the results is scummy at best and fraud at worst. Just return some kind of error telling them that they can't search for minors (without charging them). \$\endgroup\$ – Pikamander2 Jul 29 '15 at 6:05
  • 7
    \$\begingroup\$ Let me just repeatedly hit your server via a userscript and get you banned from credit card networks for fraud... (tip: don't charge a user when giving them nothing, it's fraud, plain and simple). Just warn the user. If you had virtual currency used for searches with an EULA, however, that's allowed. Just not a direct charge. \$\endgroup\$ – Amelia Jul 29 '15 at 9:02
  • \$\begingroup\$ The underlying problem you're trying to solve here seems to be more of a UX issue than a code issue. Thus, you might get better answers at User Experience. (But do note that they require a somewhat different question format; you'd need to replace the code dumps with screenshots or mock-ups of what the user actually sees.) \$\endgroup\$ – Ilmari Karonen Jul 29 '15 at 10:06
21
+50
\$\begingroup\$

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.

  1. 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?

  2. 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.

  3. 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.

  4. Library misuse:

    You repeat $("#<%=txtDate.ClientID%>") 4 times!

    You can chain everything and it will work. That's what jQuery is for!

    Please, chain them.

  5. 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.

  6. 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.

  7. 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 == "<%=btnSearch.ClientID %>" ){
            $("#dialog-confirm").dialog("open");
        }
        return false;
    });
    
  8. Did I mention library misuse?

    Look at the example below:

    var dateOfBirth = new Date($("#<%=txtDate.ClientID%>").val());
    

    Notice something?

    The use of jQuery just to retrieve the value of an <input>.

    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!

  1. You have unterminated HTML entities.

    Through all the code, you have &nbsp repeated over and over again

    You should use it like that --> &nbsp; (notice the missing ;)

  2. You're closing your <td>s in weird places

    An example, way at the bottom:

    <td>
        <asp:CheckBox runat="server" ID="cbxBoth" Text="Both" /></td>
    

    You should have one of these:

        <td>
            <asp:CheckBox runat="server" ID="cbxBoth" Text="Both" />
        </td>
    
        <td><asp:CheckBox runat="server" ID="cbxBoth" Text="Both" /></td>
    

    Pick one and stick with it.

  3. Inline styles

    While not entirelly bad, you are repeating them.

    You could just write a <style> 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.

  4. Inconsistent positioning of your <sup>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.

\$\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.