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

Online examination portal for learning

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

Problem

I am new to ASP.NET and I just developed a simple online examination portal for learning. I used ADO.NET, MySQL and developed in VS 2010.

I have a login page in which the user can login and register for a new user. After successful login, the user is redirected to the question page and I fetch the first question from the database. I populated the question in the label and options in the radio button list. The user can select one option and click the "next" button. In the click event of the "next" button I calculate the marks. I store all values in the session only. When the user click next of last question that is 4 user is redirected to the results page and prints the marks.

```
public partial class Questions : System.Web.UI.Page
{
protected void Page_Load(object sender, EventArgs e)
{
Response.Cache.SetCacheability(HttpCacheability.NoCache);
Response.Cache.SetExpires(DateTime.Now.AddSeconds(-1));
Response.Cache.SetNoStore();
if (!IsPostBack)
{
renderQuestions(1);
Session["buttonIndex"] = 1;
Session["Marks"] = 0;
}

}
public void renderQuestions(int index)
{

MySqlConnection con = null;
string conString = ConfigurationManager.ConnectionStrings["conString"].ConnectionString;
string qry = "SELECT * FROM QUESTIONS WHERE QUESTION_ID="+index+"";
try
{
using (con = new MySqlConnection(conString))
{
con.Open();
using (MySqlCommand cmd = new MySqlCommand(qry, con))
{
using (MySqlDataAdapter ada = new MySqlDataAdapter(cmd))
{
DataTable dt = new DataTable();
ada.Fill(dt);
if (dt.Rows.Count > 0)
{
clsQuestion ques = new clsQuestion();
ques.QuestionId = Convert.ToInt

Solution

Overall, I think this code is a great first try. That said, it has a lot of room for improvement in terms of simplification and improving readability.

As an aside, I'm not sure if you have a strong motivation for using Visual Studio 2010, but if you don't, I would recommend considering using a more up to date version. Visual Studio 2017 was recently released.

Naming Conventions

The naming of the methods should be PascalCase, and should not be abbreviated. For example, instead of renderQuesAndAnswers, use RenderQuestionAndAnswers. This improves readability and is consistent with C# conventions.

You also use Hungarian notation for the question class. I would also denote that it is a model. Instead of clsQuestion, it should simply be QuestionModel.

See https://msdn.microsoft.com/en-us/library/ms229045(v=vs.110).aspx and https://msdn.microsoft.com/en-us/library/ms229043(v=vs.110).aspx for reference. There are some great points in that documentation.

Use C# Auto-Implemented properties

The Question class can be greatly simplified using C# auto-implemented properties. Use public string Property { get; set; } instead of creating you own backing fields. See https://msdn.microsoft.com/en-us/library/bb384054.aspx for reference. I would also rename the Question property to QuestionText to avoid confusion.

With this, the Question class could be extremely simple:

public class QuestionModel {
    public int QuestionId { get; set; }
    public string QuestionText { get; set; }
    public string Option1 { get; set; }
    public string Option2 { get; set; }
    public string Option3 { get; set; }
    public string Option4 { get; set; }
    public int Answer { get; set; }
}


catch and throw

catch (Exception ex) {
    throw ex;
}


This is unnecessary. Without it, it will propagate the exception up, the same as manually rethrowing the exception.

Remove Unneeded Controls

You seem to have both a radio button list and 4 radio buttons. We really only need the radio button list. There are also a few panels that aren't needed.

Use ViewState Instead Of Session

Instead of using the session to store data between questions, we can use the ViewState. The ViewState doesn't persist the data on the server, but rather stores it on the client side. Since we don't need to make use of the internal state data (such as the current question), we can store it in the ViewState. Using ViewState also allows a user to have multiple question sessions going on at a time in separate windows, whereas the session data would otherwise be mixed between them.

We can also use properties to simplify this access. Instead of using strings to access the variables, we can implement the access in the property, which will prevent us from accidentally using the wrong key. For example:

private int QuestionNumber {
    get {
        return (int)ViewState["QuestionNumber"];
    }
    set {
        ViewState["QuestionNumber"] = value;
    }
}


We can also store the entire current question in the ViewState to avoid having to store individual properties of it, such as the answer index. We can do this by adding the [Serializable] attribute to the QuestionModel class. Note that if we do use the ViewState, we should also encrypt it to avoid disclosing the answers and other internal information. To do this, we can add ViewStateEncryptionMode="Always" to the Page directive on the .aspx file.

Also note that we'll need to find a way to provide access to the correct count to Results.aspx. For simplicity, we can use session data for this specific case:

Session["CorrectCount"] = CorrectCount.ToString();
Server.Transfer("Results.aspx");
Session.Contents.Remove("CorrectCount");


Ideally, it would be great to have a result information class that we could serialize and pass through, which might include the percentage, which answers were right/wrong, etc. Note: It really shouldn't be using the session at all here (see reasoning above), but the architecture of transferring the data is out of the scope of this question, so this is here for a proof of concept.

Separate Data Access From Rendering

Currently renderQuestions both handles data access as well as calling the rendering method. I would extract the data access to it's own method, something like:

private QuestionModel GetQuestion(int index) {
    // Data Access Logic
}


This allows us to have a much cleaner render logic:

private void RenderQuestion() {
    CurrentQuestion = GetQuestion(QuestionNumber);

    lblQuestion.Text = CurrentQuestion.QuestionText;

    rblOptions.Items.Clear();
    rblOptions.Items.Add(CurrentQuestion.Option1);
    rblOptions.Items.Add(CurrentQuestion.Option2);
    rblOptions.Items.Add(CurrentQuestion.Option3);
    rblOptions.Items.Add(CurrentQuestion.Option4);
}


Only Handle Answer Checking on Next Click

There's no good reason to check anything about the selected option when it's checked; we really only care when the u

Code Snippets

public class QuestionModel {
    public int QuestionId { get; set; }
    public string QuestionText { get; set; }
    public string Option1 { get; set; }
    public string Option2 { get; set; }
    public string Option3 { get; set; }
    public string Option4 { get; set; }
    public int Answer { get; set; }
}
catch (Exception ex) {
    throw ex;
}
private int QuestionNumber {
    get {
        return (int)ViewState["QuestionNumber"];
    }
    set {
        ViewState["QuestionNumber"] = value;
    }
}
Session["CorrectCount"] = CorrectCount.ToString();
Server.Transfer("Results.aspx");
Session.Contents.Remove("CorrectCount");
private QuestionModel GetQuestion(int index) {
    // Data Access Logic
}

Context

StackExchange Code Review Q#158591, answer score: 5

Revisions (0)

No revisions yet.