Return to Answer

• 1

• 6.4k
• 15
• 25

The most important thing that needs to be fixed that I see:

User us = new User(Convert.ToInt32(Session["ActionUserID"]));

Using Session State for Request Data

Consider this example scenario:

Suppose the user views a User userOne in one page then another User userTwo on another page, say two compare them against each other. Then he decides that the two Users are duplicates and userOne is the superfluous one. So he goes back to the page displaying userOne and click "Delete" button. But because the page displaying userTwo was loaded later it overwritten the Session["ActionUserID"] property with the ID of userTwo. Therefore userTwo is deleted even though the user clicked the "Delete" on userOne's page.

Using session and application state should be avoided as much as possible:

• The state associated with a page goes in the ViewState or hidden fields etc.

• The state associated with a session goes in the Session.

• The state associated with the whole application goes in the HttpContext.Application.

Using Magic Constants as Keys

You are getting Session["ActionUserID"] above, i.e. ... = ... Session["ActionUserID"] ... There must be somewhere else that you must be setting it, i.e. something like Session["ActionUserID"] = user.ID . Suppose you decided to rename the key from "ActionUserID" to "UserID", but instead renamed one occurence to "UserID" and the other to "UserId". You have a serious bug, but your application happily builds.

All string keys should be defined as constants:

private const string UserIdKey = "ActionUserID";

Now if you type UserIDKey instead of UserIdKey you have a build error right away. If you decide to change the key value, just change the constant value and it will work (for all new sessions that is. You should make sure that you do not need to change keys often.)

For other points see @BenAaronson's answer