3
\$\begingroup\$

The explanation

I'm making a website for a hypothetical theatre to get a better feel of MVC web-applications in ASP.net. The idea is that there are newsmessages that are displayed on the homepage, and these messages can be edited and added via an adminpanel. This is how it looks:

screenshot


The page works according to the following concept:

  • The web-administrator selects either an existing message from the dropdown-menu or selects 'Add new news message'
  • He can then add/edit the title and the message in the fields below
  • There is also the option to include an image for the news message. This is not required however, since if no image is added a dummy image is used.
  • If the administrator is editing a message that has an image associated with it, the image is displayed beneath the the upload button.
  • After editing a message/creating a new message the administrator submits the form. The button changes according the the action. ('Edit', 'Create', 'Try again') The uploaded file gets checked if it isn't too large, if so the user is shown the form again (hence the 'Try again' button).

The messages are stored in a SQLserver database into the following table: Picture of the table fields

I assume the 'id', 'title' and 'message' fields speak for themselves. The 'date' field is the date the news message was added, the 'userid' is a foreign-key relationship to the users table indicating which administrator posted the news message.

I have generated Linq-To-Sql classes in Visual Studio to provide me a means to connect to the database.


The code

Now for my own code; This webpage is linked to the /Admin route as /Admin/Index and I have the following AdminController:

using Theatre.Models;
using System.Web;
using System.Web.Mvc;

namespace Theatre.Controllers {
    public class AdminController : Controller {

        /// <summary>
        /// Shows an empty newsmessage form, or a filled on for a given newsmessage
        /// </summary>
        /// <param name="id">The id of the newsmessage to be displayed, if desired</param>
        /// <returns>View</returns>
        public ActionResult Index(int? id) {
            // If we have been given an id, try to get the corresponding newsmessage out of the database
            if (id != null && id.HasValue) {
                newsmessage value = newsmessage.GetFromID(id.Value);

                // If a newsmessage is returned, display its information in the form
                if (value != null) {
                    ViewBag.ButtonText = "Bewerken";
                    return View(value);
                }
            }

            // If we're here - either a fake id was given or none - return an empty form
            ViewBag.ButtonText = "Toevoegen";
            return View();            
        }

        /// <summary>
        /// Edit/Submit a newsmessage to the database
        /// </summary>
        /// <param name="nm">The newsmessage to be submitted</param>
        /// <param name="fileUploadNews">An associated file, if wanted</param>
        /// <returns>Routes to /Index on succes and failure, Shows errors if the given data was invalid</returns>
        [HttpPost]
        public ActionResult Edit(newsmessage nm, HttpPostedFileBase fileUploadNews) {
            // Is the submited data valid?
            if (ModelState.IsValid) {
                int previousId = nm.id;
                int? resultId = newsmessage.Update(nm);

                // Only try to write an image if the newsmessage was processed correctly
                if (resultId != null && resultId.HasValue) {
                    // Have we been passed a file?
                    if (fileUploadNews != null && fileUploadNews.ContentLength > 0) {
                        // Save the file to the correct folder, give it the id as name
                        fileUploadNews.SaveAs(HttpContext.Server.MapPath("~/Content/news/" + resultId.Value + ".jpg"));
                    }

                    // If the id's match, we have edited a message
                    if (previousId == resultId.Value) {
                        TempData["SuccessMessage"] = "Nieuwsbericht bijgewerkt.";
                    }
                    // Else we have successfully added a new message
                    else {
                        TempData["SuccessMessage"] = "Nieuwsbericht toegevoegd.";
                    }
                }

                // If we reach this code, there was a failure for submitting the newsmessage to the database, say so in the failure message
                else {
                    TempData["FailureMessage"] = "Toevoegen/bewerken mislukt. Gegevensbankverbindingsfout.";
                }

                // Return to the index, with either a successMessage of failureMessage
                return RedirectToAction("Index", "Admin");
            }

            // If the submitted data was not valid, show the form again with the errors
            else {
                // Display 'Try again' on the button
                ViewBag.ButtonText = "Opnieuw proberen";

                // Return the Index page for the given newsmessage (will display errors in the ValidationSummary)
                return View("Index", nm);
            }            
        }
    }
}

The code above uses the following partial class I've written for the newsmessage Linq-To-Sql model: (one feature that isn't implemented yet is the id of the administrator that posted the message, as I do not yet have a way to login/authenticate)

using System;
using System.Collections.Generic;
using System.ComponentModel.DataAnnotations;
using System.Linq;
using System.Web;

namespace Theatre.Models {

    [MetadataType(typeof(newsmessageValidation))]
    public partial class newsmessage {

        public HttpPostedFileBase fileUploadNews { get; set; } 

        /// <summary>
        /// Gets the last 2 newsmessages from the database
        /// </summary>
        /// <returns>An array of 2 newsmessages</returns>
        public static newsmessage[] GetLatestNews() {
            try {
                using (DataClassesDataContext db = new DataClassesDataContext()) {
                    return db.newsmessages.OrderBy(e => e.date).Take(2).ToArray();
                }
            } catch {
                return null;
            }
        }

        /// <summary>
        /// Gets all newsmessages from the database
        /// </summary>
        /// <returns>A list of newsmessages</returns>
        public static List<newsmessage> GetNews() {
            try {
                using (DataClassesDataContext db = new DataClassesDataContext()) {
                    return db.newsmessages.OrderBy(e => e.date).ToList();
                }
            }
            catch {
                return null;
            }
        }

        /// <summary>
        /// Gets a newsmessage for a given id, returns null if non-existent
        /// </summary>
        /// <param name="id">The id of the newsmessage to be retrieved</param>
        /// <returns>The newsmessage matching the id, returns null if non was found</returns>
        public static newsmessage GetFromID(int id) {
            try {
                using (DataClassesDataContext db = new DataClassesDataContext()) {
                    return db.newsmessages.Single(e => e.id == id);
                }
            } catch {
                return null;
            }
        }

        /// <summary>
        /// Updates/inserts a given newsmessage into the database
        /// </summary>
        /// <param name="nm">The newsmessage to be submitted</param>
        /// <returns>The id of the submitted newsmessage, null if the action failed</returns>
        public static int? Update(newsmessage nm) {
            try {
                using (DataClassesDataContext db = new DataClassesDataContext()) {
                    // Does the message already exist? then update its title and message
                    if (db.newsmessages.Contains(nm)) {
                        newsmessage old = db.newsmessages.Single(e => e.id == nm.id);
                        old.message = nm.message;
                        old.title = nm.title;
                    }

                    // Else, give it todays date and time
                    else {
                        nm.date = DateTime.Now;
                        nm.userid = 1; // This is temporary until I implement a login feature
                        db.newsmessages.InsertOnSubmit(nm);
                    }
                    db.SubmitChanges();
                    return nm.id;
                }
            } catch {
                return null;
            }
        }
    }

    /// <summary>
    /// Extra class providing validation requirements for a newsmessage
    /// </summary>
    public class newsmessageValidation {
        [Required(ErrorMessage = "Dit veld is verplicht.")]
        [StringLength(50, MinimumLength = 5, ErrorMessage = "Een titel moet tussen de 5 en 50 karakters lang zijn.")]
        [Display(Name = "Titel")]
        public string title { get; set; }

        [Required(ErrorMessage = "Dit veld is verplicht.")]
        [StringLength(450, MinimumLength = 5, ErrorMessage = "Een bericht moet tussen de 5 en 450 karakters lang zijn.")]
        [Display(Name = "Bericht")]
        public string message { get; set; }

        [ValidateFile(ErrorMessage = "Als u een afbeelding meegeeft, gelieve dan een te selecteren die kleiner is dan 1MB.")]
        public HttpPostedFile fileUploadNews { get; set; }
    }
}

This code uses a custom-made RequiredAttribute called 'ValidateFile':

using System.ComponentModel.DataAnnotations;
using System.Web;

namespace Theatre.Models {
    public class ValidateFileAttribute : RequiredAttribute {
        public override bool IsValid(object value) {
            HttpPostedFileBase file = value as HttpPostedFileBase;
            if (file == null) {
                return true;
            }
            if (file.ContentLength > 1 * 1024 * 1024) {
                return false ;
            }
            return true;
        }
    }
}

To finish, here is my cshtml code for the /Admin/Index view. There is also a _Layout file in the Shared folder, which adds a header, footer and renders the scripts and css - I have not included it here because it doesn't contain any noteworthy code.

@using Theatre.Models;
@model newsmessage
@{
    ViewBag.Title = "Admin"; // set the page title to 'Admin - My Theatre'
}

<h2>Beheerderspaneel</h2>
<p>Via deze module kan je de homepage newsitems inhoud bewerken.</p>

@if (TempData["SuccessMessage"] != null) {
    <div class="clearfix alert alert-success">@TempData["SuccessMessage"]</div>
}

@if (TempData["FailureMessage"] != null) {
    <div class="clearfix alert alert-warning">@TempData["FailureMessage"]</div>
}


@using (Html.BeginForm("Index", "Admin", FormMethod.Post, new { @id = "dropdownForm" })) {
    <div id="beheernieuws">
        <h3>Selecteer uw nieuwsbericht</h3>
        <em>Opgelet: wanneer er geen nieuwsbericht geselecteerd is zal een nieuw bericht toegevoegd worden.</em>
        <p>
            @Html.DropDownListFor(nm => nm.id, new SelectList(newsmessage.GetNews(), "id", "title"), "Een nieuw bedricht aanmaken", new { @id = "dropdownList", @onchange = "dropdownForm.submit()" })
        </p>
    </div>
}

@using (Html.BeginForm("Edit", "Admin", FormMethod.Post, new { @id = "detailForm", @class = "form", @enctype = "multipart/form-data" })) {
    @Html.ValidationSummary()
    <h3>Nieuwsbericht:</h3>
    <fieldset>
        <p>
            @Html.LabelFor(x => x.title)
            @Html.TextBoxFor(x => x.title)
            @Html.ValidationMessageFor(x => x.title)
        </p>
        <p>
            @Html.LabelFor(x => x.message)
            @Html.TextAreaFor(x => x.message, new { @style = "width: 300px; height:100px;" })
            @Html.ValidationMessageFor(x => x.message)
        </p>
        <p>
            Selecteer het bestand:<br />
            <input type="file" name="fileUploadNews" id="fileUploadNews" />
            @if (Model != null && System.IO.File.Exists(Request.MapPath("~/Content/news/" + Model.id + ".jpg"))) {
                <img alt="" src="@Url.Content("~/Content/news/" + Model.id + ".jpg")" width="100" height="100" />
            }
        </p>

        @if (Model != null) {
            @Html.HiddenFor(x => x.id)
        }

        <input type="submit" value="@ViewBag.ButtonText" class="button" name="news_message" />
    </fieldset>
}

The question

I feel I've already learned a lot from doing this exercise and I'm posting this code here in the hope that people will point out some mistakes I made, so that I can learn from this even more. I'm finding this mvc model to be something very interesting, therefore I'm really interesting in hearing what I should do to improve!

\$\endgroup\$
  • \$\begingroup\$ Welcome to Code Review! I have rolled back the last edit. Please see what you may and may not do after receiving answers. Changing the code in question isn't allowed after receiving answers because otherwise existing answers will be invalidated. \$\endgroup\$ – Heslacher Oct 31 '15 at 11:13
4
\$\begingroup\$

Just some quick shoots.

  • if (id != null && id.HasValue) this two checks are doing the same where the first is just shortcut of the second.

  • Classes should be named using PascalCase casing. See the NET naming guidelines.

  • the comment here

          // If we're here - either a fake id was given or none - return an empty form
          ViewBag.ButtonText = "Toevoegen";
          return View();   
    

    isn't telling the truth. For the case that newsmessage.GetFromID(id.Value); return null this would also reach this position.

  • in the Edit() method double check of the nullable too.

  • if you would reverse the condition if (ModelState.IsValid) you could return early and by omitting the else you will save some horizontal space which increases readability.

  • speaking about readability, IMO you are using to many comments. The used comments mostly only explain what is done, which should be done by the code itself by using meaningful and descriptive names for variables, methods and classes. Comments should only tell why something is done in the way it is done.

\$\endgroup\$
  • \$\begingroup\$ I assume you are referencing to the ´newsmessage´ class. Since it is a partial class I have to keep it having the same name as the Linq-To-Sql generated class (I think). And these generated classes apparently don't use CameCasing. About the comment. I still believe it is correct, if the GetFromID(int it) method is given a fake id, it returns null - which is what you described. \$\endgroup\$ – Bjorn De Rijcke Oct 31 '15 at 9:33
  • \$\begingroup\$ @BjornDeRijcke you can change that using the Linq-To-SQL designer by opening the properties pane and editing the name. \$\endgroup\$ – RubberDuck Oct 31 '15 at 13:28
4
\$\begingroup\$

In addition to Heslacher's points:

  • Careful with keys like "SuccessMessage" and "FailureMessage": a typo easily happens, so instead make these const string in a dedicated class.

  • I find K&R style braces an odd sight in C# code; but you do seem to be consistent, so that's good.

  • Properties should be PascalCase: fileUploadNews, message, title

  • I wouldn't name the existing message old, I'd name it existing.

\$\endgroup\$
4
\$\begingroup\$

I've also spotted a few things in your code...


This

if (id != null && id.HasValue)

is exactly the same as

if (id.HasValue)

A nullable value has the HasValue so that you don't have to check against null, it cannot be null, it either has a value or not


Here

using (DataClassesDataContext db = new DataClassesDataContext()) 
{
    // ... 
}

you can use var what makes you code more readable, the type is clear so it's not necessary to repeat it twice:

using (var db = new DataClassesDataContext()) 
{
    // ... 
}

It's not a good idea to hardcode some calculation inside an if

if (file.ContentLength > 1 * 1024 * 1024)
{
    return false ;
}

You should use a helper variable:

const int oneMegabyte = 1 * 1024 * 1024;
if (file.ContentLength > oneMegabyte)
{
    return false ;
}

at the same time this

HttpPostedFileBase file = value as HttpPostedFileBase;
if (file == null) {
  return true; 
}
if (file.ContentLength > 1 * 1024 * 1024) {
  return false ;
}
return true;

could be replaced by simply

var file = value as HttpPostedFileBase;
return file == null || file.ContentLength > oneMegabyte;
\$\endgroup\$
  • \$\begingroup\$ Thanks for your answer, I've updated my code with your suggestions. I do however believe you've made a little mistake in your last piece of code with the greater-than sign. I think it should be a lesser-than sign. \$\endgroup\$ – Bjorn De Rijcke Oct 31 '15 at 9:26
  • \$\begingroup\$ @BjornDeRijcke oh, you're right, it should be indeed < there ;-) \$\endgroup\$ – t3chb0t Oct 31 '15 at 9:29
1
\$\begingroup\$

You do this a lot when accessing the database.

} catch {
    return null;
}

Trust me when I say this, you are going to eventually want to know why something returned null.

I don't necessarily see an issue with returning a null when an error happens so that your program continues to work, but you should absolutely be logging these exceptions somewhere. It's much easier to track down than setting up a remote debugger and all the pain that comes along with it.

\$\endgroup\$
  • \$\begingroup\$ You raise a valid point! I hadn't been giving much thought to this since I was more focused on the MVC part and this code was just for learning purposes, but you are correct that simply returning null isn't a good practice. I'll keep this in mind. \$\endgroup\$ – Bjorn De Rijcke Nov 1 '15 at 15:28
  • \$\begingroup\$ You're welcome @BjornDeRijcke! I only mention it because I've fought with a similar pattern in my controllers. I tend to move this exception handling up a layer into the controller. Then, if an unexpected exception is thrown, I return a shared error view. I wasn't logging either, and it was a nightmare. I had to hack in the logging as quickly as possible, redeploy, then examine the log to find the bug, fix it, redeploy, then go back to make the logging right, then redeploy a third time. TL; DR: getting your logging right from the start is well worth the time. =) \$\endgroup\$ – RubberDuck Nov 1 '15 at 15:40

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.