3
\$\begingroup\$

This ASP.NET MVC website is for my professor to track my progress on my Checkers app. This is the first time I have build an ASP.NET website entirely on my own, and I would like a review to make sure I am doing everything in the best way possible.

First, my HomeController:

public class HomeController : Controller
{
    private T Deserialize<T>(string filePath)
    {
        var jss = new System.Web.Script.Serialization.JavaScriptSerializer();
        var data = System.IO.File.ReadAllText(filePath);
        return jss.Deserialize<T>(data);
    }

    public ActionResult Index()
    {
        var announcements = Deserialize<ListOfItems<Announcement>>(Server.MapPath("~/App_Data/Announcements.json"));
        var latestAnnouncement = announcements.Items.Last();
        if (DateTime.Now.Date.Subtract(TimeSpan.FromDays(7)) > latestAnnouncement.AnnouncementDate)
        {
            latestAnnouncement = null;
        }

        return View(latestAnnouncement);
    }

    public ActionResult About()
    {
        return View();
    }

    public ActionResult Contact()
    {
        return View();
    }

    public ActionResult Reports()
    {
        var reports = Deserialize<ListOfItems<BiweeklyReport>>(Server.MapPath("~/App_Data/Reports.json"));

        return View(reports.Items);
    }

    public ActionResult Resources()
    {
        var resources = Deserialize<ListOfItems<Resource>>(Server.MapPath("~/App_Data/Resources.json"));

        return View(resources.Items);
    }

    public ActionResult Announcements()
    {
        var announcements = Deserialize<ListOfItems<Announcement>>(Server.MapPath("~/App_Data/Announcements.json"));

        return View(announcements.Items);
    }
}

My models are as follows:

First, a generic class for easy serialization of a list of items:

[Serializable]
public class ListOfItems<T>
{
    public List<T> Items { get; set; }
}

And a set of items to serialize. To keep this post slim, I will only post one:

[Serializable]
public class Announcement
{
    public string Title { get; set; }
    public DateTime AnnouncementDate { get; set; }
    public string Content { get; set; }
}

My views are all either static information or a simple iteration over a list of data; here is my view for the above model class:

@model List<Announcement>
@using ITM3804Website.Models

@{
    ViewBag.Title = "Announcements";
}
<h2>@ViewBag.Title</h2>
<hr />

@{
    var orderedAnnouncements = Model.OrderByDescending(x => x.AnnouncementDate);
    foreach (var announcement in orderedAnnouncements)
    {
         <p>
             <strong class="announcement-title">@announcement.Title</strong><br/>
             <i>@announcement.AnnouncementDate.ToString("MMMM d, yyyy")</i><br/>
             @announcement.Content
         </p>

        if (announcement != orderedAnnouncements.Last())
        {
             <hr/>
         }
     }
}
\$\endgroup\$

1 Answer 1

Reset to default
2
\$\begingroup\$

I want to talk about your View while I'm on mobile and thinking about it. Just like regular C# code we should definitely define structure for our Razor code. The first thing that rubs me wrong is that @using statement. I really don't like it because you only need it for the @model declaration at the top of the file. I would just get rid of it and explicitly specify the model full type.

Next, I really don't like the @{ } block with the foreach and such in it. Generally I just prefer to prefix each code block with @whatever instead. This helps create a more consistent convention throughout. (Right now you mix code and @code in your view which is generally bad.)

As far as the HTML itself, we've started leaning more on CSS to do the styling for things, so elements like strong and i should be span class="title" then in your CSS .title would have font-weight: bold;.

Let's talk about your serialization:

private T Deserialize<T>(string filePath)
{
    var jss = new System.Web.Script.Serialization.JavaScriptSerializer();
    var data = System.IO.File.ReadAllText(filePath);
    return jss.Deserialize<T>(data);
}

You always call it as Deserialize<T>(Server.MapPath(...)), so let's encapsulate that:

private T DeserializeAppData<T>(string file) =>
    Deserialize<T>(Server.MapPath($"~/App_Data/{file}.json"));

Then call it as DeserializeAppData<T>("Reports"). Much simpler, straightforward and to the point. You still have the other Deserialize<T> method, in case you need it for other things.

\$\endgroup\$

You must log in to answer this question.