HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMinor

Website news message edit panel

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
websiteeditnewsmessagepanel

Problem

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:

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:

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 {

///
/// Shows an empty newsmessage form, or a filled on for a given newsmessage
///
/// The id of the newsmessage to be displayed, if desired
/// View

Solution

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.

Code Snippets

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

Context

StackExchange Code Review Q#109239, answer score: 4

Revisions (0)

No revisions yet.