tags:

views:

35

answers:

2

I'm refactoring my code. Consider this example...

    public virtual List<Student> FetchEnrollmentList(DateTime admissionDateFrom, 
 DateTime admissionDateTo)
    {
        var students = new List<Student>();

            using (oconn = new OleDbConnection(OracleConnection))
            {
                oconn.Open();
                query = "SELECT * FROM Enrollment Where AdmissionDate between @AdmissionDateFrom and @AdmissionDateTo ";

                using (ocmd = new OleDbCommand(query, oconn))
                {
                    ocmd.Parameters.Add("@AdmissionDateFrom", OleDbType.Date).Value = admissionDateFrom;
                    ocmd.Parameters.Add("@AdmissionDateTo", OleDbType.Date).Value = admissionDateTo;

                    using (odr = ocmd.ExecuteReader())
                    {
                        while (odr.Read())
                            students.Add(new Student { Name = odr["StudentName"].ToString() });
                    }
                }
            }

        return students;
    }

I just want to eliminate the From and To so I created a type like this

     public virtual List<Student> FetchEnrollmentList(DateSpan admissionDate)
    {
        var students = new List<Student>();

        using (oconn = new OleDbConnection(OracleConnection))
        {
            oconn.Open();
            query = "SELECT * FROM Enrollment Where AdmissionDate between @AdmissionDateFrom and @AdmissionDateTo ";

            using (ocmd = new OleDbCommand(query, oconn))
            {
                ocmd.Parameters.Add("@AdmissionDateFrom", OleDbType.Date).Value = admissionDate.Start;
                ocmd.Parameters.Add("@AdmissionDateTo", OleDbType.Date).Value = admissionDate.End;

                using (odr = ocmd.ExecuteReader())
                {
                    while (odr.Read())
                        students.Add(new Student { Name = odr["StudentName"].ToString() });
                }
            }
        }

        return students;
    }

Is it OK to have it like this? Any other ideas? thanks....

+1  A: 

Re the "imagine I had 3" comment: then you have 3 ;) (or 3 pairs)

One alternative there (as the params increases) is to create a class that represents the query parameters, then there is only 1 formal parameter. Plus you can add options painlessly. For example:

public class StudentSearchRequest {
    public DateTime FooStart {get;set;}
    public DateTime FooEnd {get;set;}
    public bool PublicDataOnly {get;set;}
    public int SomethigElse {get;set;}
}
Marc Gravell
hmmm sounds interesting but, actually Im thinking about this because in my querying methods, i have to overload a lot of methods because different number of parameters varies per client request.. with this technique it will obviously solve the problem of uber-overloading.. but, is this a good practice in OOP? thanks..
CSharpNoob
+2  A: 

I agree with Martin, with just two DateTime parameters defining the boundaries of the list, there really is not much need to refactor, there simply is no "code smell" there yet.

On the other hand if you introduce other methods that as you say use several date range parameters I would refactor once I actually have those methods with DateSpan. Generally YAGNI until you really introduce those methods, only then should you refactor existing methods for uniformity imo.

I wouldn't introduce too much generality until there really is a need for it, refactoring is not about what you might need in some distant future, but what you can do about creating more readable and maintainable code with the code base you have and the features you want to add at the time.

BrokenGlass
thanks for a good piece of advice/lecture about yagni principle.. :)
CSharpNoob