2
\$\begingroup\$

I'm returning to Web Development after a number of years working in other areas.

The below image shows what what I have so far looks like. I'm not 100% satisfied with the styling yet, but that's a separate question - I include it here mostly to show what this does. (There will eventually be more fields on the table).

enter image description here

The History dialog is a standard jQuery UI dialog that comes up when you hover over the History button. Each link refers to what's internally referred to as a "Master Schedule." The list of Master Schedules is retrieved through a stored procedure.

When you click on a link, it populates the table with details of that schedule (and, again, I'll eventually have more fields than are shown on the table right now).

The table and button are inside an UpdatePanel because I don't want to do a full page refresh every time they view a new item from the history.

Here's my Update Panel:

<asp:UpdatePanel ID="detailsTableUpdatePanel" runat="server">
    <ContentTemplate>
        <asp:Button ID="history" Text="History" runat="server" OnClick="history_Click" />
        <input id="masterIDInput" type="hidden" runat="server" />
        <div id="dialog" runat="server"></div>
        <p />
        <asp:Table ID="detailsTable" Visible="false" runat="server" />
    </ContentTemplate>
</asp:UpdatePanel>

Here's my JavaScript:

<script type="text/javascript">
    $(document).ready(AddHover)

    function AddHover()
    {
        $("#MainContent_history").hover(function () {
            $("#MainContent_dialog").html(<%= divContent %>);
            $("#MainContent_dialog").dialog({
                title: "History",
                close: function (ev, ui) {
                    $(this).hide();

                    // I was having an issue where you could only see the History dialog once - once you
                    // closed it once it would never show up again
                    // This (along with a few other hacks) works but it seems like there should be a better way to do this
                    AddHover();
                }
            })
        })
    }

    function bringUpDetails(masterID) {
        // Add the ID of the Master Schedule we're looking for to the hidden form input
        $("#MainContent_masterIDInput").val(masterID);
        // Simulate clicking on the button to call the button's event handler
        // I don't really like this - it seems like I should just do some kind of
        // AJAX call here
        $("#MainContent_history").trigger("click");
        // Same as previous AddHover() call
        AddHover();
    }
</script>

Here's my C# code behind:

public string divContent;

    protected void Page_Load(object sender, EventArgs e)
    {
        if (!Page.IsPostBack)
        {
            BuildDivContent();
        }
    }

    private void BuildDivContent()
    {
        // I'm using LINQ to SQL for now
        // In the final version I'll probably replace this with Entity Framework
        DataClasses1DataContext context = new DataClasses1DataContext();
        List<spGetMasterScheduleHistoryForClientResult> results = context.spGetMasterScheduleHistoryForClient(295615).ToList();

        StringBuilder sb = new StringBuilder();

        if (results.Any())
        {
            sb.Append("\"");
        }

        foreach (var result in results)
        {
            sb.AppendFormat("<a onclick='bringUpDetails({0})' href='#'>{1} - {2}</a><br/>", result.MasterScheduleID, result.EffectiveBeginDate, result.EffectiveEndDate);
        }

        if (results.Any())
        {
            sb.Append("\"");
        }

        divContent = sb.ToString();
    }

    protected void history_Click(object sender, EventArgs e)
    {
        // Figure out from the hidden form field which master
        int id =
            masterIDInput.Value != null && masterIDInput.Value.Trim().Length > 0 ?
                int.Parse(masterIDInput.Value) :
                1;

        DataClasses1DataContext context = new DataClasses1DataContext();
        spGetMasterScheduleByIDResult result = context.spGetMasterScheduleByID(id).First();

        TableRow headerRow = new TableRow();
        headerRow.Cells.Add(new TableCell()
        {
            Text = "<b>Begin Date</b>"
        });
        headerRow.Cells.Add(new TableCell()
        {
            Text = "<b>End Date</b>"
        });

        detailsTable.Rows.Add(headerRow);

        TableRow detailsRow = new TableRow();
        detailsRow.Cells.Add(new TableCell()
        {
            Text = result.EffectiveBeginDate.ToString()
        });

        detailsRow.Cells.Add(new TableCell()
        {
            Text = result.EffectiveEndDate?.ToString() ?? "N/A"
        });

        detailsTable.Rows.Add(detailsRow);

        detailsTable.Visible = true;
    }

And, finally, here's my CSS:

<!-- Yes, I do realize that this should probably go in a separate stylesheet -->
<style type="text/css">
    table, th, td {
        border: 1px solid black;
        min-width: 250px;
    }

    a:link {
        color: green
    }

    a:visited {
        color: red
    }

    a:hover {
        color: blue
    }

    a:active {
        color: blue;
    }
</style>

I have code comments over a few specific things I was concerned about. Also, I'm sure that there are plenty of ways that I could optimize this as well and was hoping that someone could give me feedback on my overall approach.

\$\endgroup\$
  • \$\begingroup\$ a:active, a:hover { color : blue; } a:visited { color:red; } a:link { color: green; } table, th, td { border: 1px solid black; min-width: 250px; } \$\endgroup\$ – rajesh Mar 6 '17 at 15:46
  • \$\begingroup\$ @rJ7 Good point, thx - do you have any other suggestions on things I can improve here? (As I mentioned in the question, I've been out of web development for awhile, so I'm sure there are plenty of things I can improve on here). \$\endgroup\$ – EJoshuaS - Reinstate Monica Mar 6 '17 at 15:57
2
\$\begingroup\$

Only targeting the C# code behind

BuildDivContent()

  • Instead of calling two times if (results.Any()) for appending " you should check once for if (!results.Any()) and if that is true, assign string.Empty" todivContent` and return early.
  • If you can estimate how many chars the StringBuilder will contain at the end you can use the overloaded constructor which takes an initial capacity. This removes the need of the StringBuilder if its initial capicity isn't high enough to increase the internal data structure.
  • Because you are allready using C# 6 you could take advantage of the string interpolation feature using the $ sign instead of using AppendFormat of the StringBuilder.

Implementing the mentioned points leads to

private void BuildDivContent()
{
    // I'm using LINQ to SQL for now
    // In the final version I'll probably replace this with Entity Framework
    DataClasses1DataContext context = new DataClasses1DataContext();
    List<spGetMasterScheduleHistoryForClientResult> results = context.spGetMasterScheduleHistoryForClient(295615).ToList();

    if (!results.Any())
    {
        divContent = string.Empty;
        return; 
    }

    StringBuilder sb = new StringBuilder("\"", 1024);

    foreach (var result in results)
    {
        sb.Append($"<a onclick='bringUpDetails({result.MasterScheduleID})' href='#'>{result.EffectiveBeginDate} - {result.EffectiveEndDate}</a><br/>");
    }

    sb.Append("\"");

    divContent = sb.ToString();
}

history_Click()

Like any method this method should be named using PascalCase casing. In addition, please don't use underscores in method names.

The evaluation of int id should be done more idiomatic by using string.IsNullOrWhitheSpace() like so

int id = string.IsNullOrWhiteSpace(masterIDInput.Value) ? 1 : int.Parse(masterIDInput.Value);
\$\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.