Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I would like to develop a student management system with repository pattern using ado.net (no EF or MVC).

I decided to create the model (business objects) and interfaces as one project "StudentModel". Implementing the interfaces to fetch data from DB as one project "StudentImpl" and the third will be the web application which will call the implementations and populate the views.

 <connectionStrings>
<add name="Dbconnection"
   connectionString="Server=localhost; Database=studentdb ; Integrated Security=True" providerName="System.Data.SqlClient" />

My Model

 public class Student
 {
   public int StudentID { get; set; }
   public string StudentName { get; set; }
 }

My Repository

public interface IRepository<T>
{
    T Create(T entity);
    T Update(T entity);
    T Delete(T entity);
    IList<T> FindAll();
}

My Implementation

public class StudentRepository:IRepository<Student>
{
    SqlConnection aConnection;
    public StudentRepository(string ConnectionString)
    {
         aConnection = new SqlConnection(ConnectionString);
    }

    public Student Create(Student entity)
    {
        throw new NotImplementedException();
    }

    public Student Update(Student entity)
    {
        throw new NotImplementedException();
    }

    public Student Delete(Student entity)
    {
        throw new NotImplementedException();
    }

    public IList<Student> FindAll()
    {
        IList<Student> Students = new List<Student>();
        using(aConnection)
        {
            SqlCommand cmd = new SqlCommand("SELECT studentID,StudentName FROM Student", aConnection);
            aConnection.Open();
            SqlDataReader rdr = cmd.ExecuteReader();
            while (rdr.Read())
            {
                Student aStudent = new Student();
                aStudent.StudentID = Convert.ToInt32(rdr["StudentID"]);
                aStudent.StudentName = rdr["StudentName"].ToString();
                Students.Add(aStudent);
            }
            return Students;
        }
    }

And my code behind

string ConString = ConfigurationManager.ConnectionStrings["myConnection"].ConnectionString;
IRepository<Student> aStudent = new StudentRepository(ConString);
IList<Student> Students=aStudent.FindAll();

What would be the best place to pass the connection string to the implementation and how can I improve this code further?

Please give your comments on my design and help me to improve.

Edit:

  protected void Page_Load(object sender, EventArgs e)
    {
        string ConString = WebConfigurationManager.ConnectionStrings["Dbconnection"].ToString();
        IStudent aStudent = new StudentRepository(ConString);
        IList<StudentModel.Student> students= aStudent.FindAll();
        GvStudent.DataSource = students;
        GvStudent.DataBind();

    }

    protected void GvStudent_RowDataBound(object sender, GridViewRowEventArgs e)
    {
        if (!e.Row.RowType.Equals(DataControlRowType.DataRow)) return;
        HyperLink lnkName = (HyperLink)e.Row.FindControl("lnkName");
        string id = ((Label)e.Row.FindControl("lblID")).Text;
        lnkName.NavigateUrl = "~/Edit.aspx?q=" + id;
    }

    protected void GvStudent_PageIndexChanging(object sender, GridViewPageEventArgs e)
    {
        GvStudent.PageIndex = e.NewPageIndex;
        string ConString = WebConfigurationManager.ConnectionStrings["Dbconnection"].ToString();
        IStudent aStudent = new StudentRepository(ConString);
        IList<StudentModel.Student> students = aStudent.FindAll();
        GvStudent.DataSource = students;
        GvStudent.DataBind();

    }

    protected void export_Click(object sender, EventArgs e)
    {
       string formatstring = default(string);
    Response.Clear();
    Response.Buffer = true;
    Response.ContentType = "application/vnd.ms-excel";
    string FileName = "Samepl.xls";
    Response.AddHeader("content-disposition", "attachment;filename=" + FileName);
    Response.Charset = "";
    EnableViewState = false;
    System.IO.StringWriter sw = new System.IO.StringWriter();
    System.Web.UI.HtmlTextWriter htw = new HtmlTextWriter(sw);
    GvStudent.AllowPaging = false;
    GvStudent.RenderControl(htw);
    string value = sw.ToString();
    GvStudent.AllowPaging = true;
    Response.Write(formatstring + value);
    Response.End();
    }

    public override void VerifyRenderingInServerForm(Control control)
    {

    }
}

    <asp:GridView ID="GvStudent" runat="server" AutoGenerateColumns="false" OnRowDataBound="GvStudent_RowDataBound" AllowPaging="true" PageSize="2" OnPageIndexChanging="GvStudent_PageIndexChanging" PagerSettings-Mode="NumericFirstLast">
    <Columns>
       <asp:TemplateField>
                        <HeaderTemplate>
                            Student Name
                        </HeaderTemplate>
                        <ItemTemplate>
                            <asp:HyperLink ID="lnkName" runat="server" Text='<%#Eval("StudentName") %>'></asp:HyperLink>
                           </ItemTemplate>
                        <HeaderStyle HorizontalAlign="Center" />
                        <ItemStyle HorizontalAlign="Center" />
                    </asp:TemplateField>
           <asp:TemplateField>
                        <HeaderTemplate>
                            Student ID
                        </HeaderTemplate>
                        <ItemTemplate>
                            <asp:Label ID="lblID" runat="server" Text='<%#Eval("StudentID") %>' ></asp:Label>
                           </ItemTemplate>
                        <HeaderStyle HorizontalAlign="Center" />
                        <ItemStyle HorizontalAlign="Center" />
                    </asp:TemplateField>
    </Columns>

</asp:GridView>
share|improve this question
    
If someone can guide me to achieve the same through MVC and EF, it would be great. –  Tech Jerk May 21 at 15:00

2 Answers 2

The only thing I'd say about your repository, is that FindAll shouldn't return an IList<T>, but an IEnumerable<T>. And you're probably missing a method like GetById that returns only a single record - right now the only way to fetch a single student is... to fetch them all and then filter in memory. The database backend is better at this, you will want LINQ-to-SQL to pick up the criteria and translate a WHERE clause on the server.

You're missing a unit of work. Each repository owns its own connection to the database, which means if you have a business requirement to Delete a Student and everything associated to it, you're going to have to do it with as many connections as you have tables affected.

One way to have a unit of work is to use Entity Framework (that would mootinate your repositories); another way is to roll your own: instead of having code-behind that depends on repositories, you have code-behind that depends on a unit-of-work:

string ConString = ConfigurationManager.ConnectionStrings["myConnection"].ConnectionString;
IUnitOfWork uow = new UnitOfWork(ConString);
IList<Student> Students = uow.Students.FindAll();

In other words, push the responsibility of owning the connection outside of your repositories - let the repositories depend on a connection instead, and let the unit of work pass it down to each repository. Doing this will enable deleting/inserting/updating data from multiple repositories within a transaction which can be rolled back if things go wrong halfway through.


Achieving the same thing with EF is simpler: EF already implements repositories and unit-of-work for you, and SaveChanges operates like a Commit - you can Rollback the changes by simply disposing the DbContext before SaveChanges is called.

share|improve this answer
    
Thanks! If I need to move the connection string to a common place, so that the connection string is initialised only once, how could I do that? Masterpage code behind or App code or in Global asax? –  Tech Jerk May 21 at 15:24
    
Putting it in the master page will initialize it once per request - putting it in global.asax will initialize it once in the app's lifetime. You decide ;) –  Mat's Mug May 21 at 15:26
    
I need it to be there in global.asax but how can I pass it from global.asax to my implementation while I access any method like FindALL() ? –  Tech Jerk May 21 at 15:29
    
Also I believe, I've implemented Repository pattern through Interfaces and Dependency Injection through StudentRepository constructor. Correct me if I am wrong –  Tech Jerk May 21 at 15:31
    
In ASP.NET MVC you could make the global setting a constructor argument for your controllers. I'd have to relook at the chapter about ASP.NET in Dependency Injection in .NET to see how DI works with ASP.NET, but if I remember correctly your composition root is at the page level and you have to work around that (I don't think you can control a page's constructor parameters), so you're probably better off composing your dependencies in the master page's code-behind. –  Mat's Mug May 21 at 15:34

Aside from Mat's comments there are a few minor things:

  • Parameters are lowerCamelCase so ConnectionString becomes connectionString.
  • In your SQL query you use studentID and in the reader you use StudentID.
  • Local variables are lowerCamelCase so Students becomes students.

Aside from that there is not much code to review.

share|improve this answer
    
Thanks! If I need to move the connection string to a common place, so that the connection string is initialised only once, how could I do that? Masterpage code behind or App code or in Global asax? –  Tech Jerk May 21 at 15:25
    
Use the App.Config or Web.Config configuration file where you can add your own keys under <AppSettings>. Afterwards you can use them with ConfigurationManager.AppSettings["myKey"] –  Jeroen Vannevel May 21 at 15:25
    
Also I believe, I've implemented Repository pattern through Interfaces and Dependency Injection through StudentRepository constructor. Correct me if I am wrong –  Tech Jerk May 21 at 15:32
    
W.r.t to Mat's comment, I need it to be there in global.asax but how can I pass it from global.asax to my implementation while I access any method like FindALL() –  Tech Jerk May 21 at 15:33

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.