patterncsharpMinor
Website news message edit panel
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 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
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.
-
-
Classes should be named using
-
the comment here
isn't telling the truth. For the case that
-
in the
-
if you would reverse the condition
-
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.
-
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.