0

Is this a standard, good practice way of doing things? Basically return a list of itself? Should the actual fields (id, title, etc) be a separate class? (I've seen people call it DTO objects)

I'm starting a project & I want to try & get some of these fundamentals down--

Thanks!!

public class Calendar
{
    public int id { get; set; }
    public string title { get; set; }

    public List<calendar> GetAll()
    {
        var list = new List<calendar>();

        var db = new mssql2();
        db.set("s1");
        string sql = @"select * from [cal]";
        var dr = db.dr(sql);
        while (dr.Read())
        {
           var e = new calendar();
           e.id = (int)dr["id"];
           e.title = dr["title"].ToString();
           list.Add(e);
        }
        return list;
    }
}
Improve this question
6
  • FYI, since you mentioned standards, class names, property names and method names usually use TitleCase. Commented Feb 29, 2012 at 21:33
  • Some issues: 1) in C#, the general naming convention is initial caps camel casing, so your class name would be Calendar, and your get_all function would be named GetAll (but this is purely stylistic, and not germane to the issue) 2) short variable names are generally frowned upon. 3) get_all might be better as a static function. Commented Feb 29, 2012 at 21:35
  • 1
    Why is the calender variable called "e"? Also, why catch the exception if you are just going to throw it again (doing this will ruin your stack trace)? Commented Feb 29, 2012 at 21:38
  • No point catching an exception and re-throwing it right away. Most likely you do not need exception handling code here at all. I prefer to rely on central unhandled exception handler for database connection problems. Commented Feb 29, 2012 at 21:38
  • If you want to get the fundamentals right, start by using Microsoft's coding standards (msdn.microsoft.com/en-us/library/ms229002.aspx). Also, never use throw ex. Just us throw. Otherwise you loose your stack trace. And, to answer your question, I think it is ok to use the properties (not fields) in the class unless you want to separate you code class from your data classes (also know as POCO). Commented Feb 29, 2012 at 21:43

3 Answers 3

Reset to default
7

You seem to be mixing your Domain model with your Data Access layer.

Keep Calendar as it's own class, and maybe make another class called CalendarService or CalendarRepository that returns you a list of Calendar objects.

Here is an example:

public class Calendar
{
    public Calendar() { }
    public Calendar(int id, string title)
    {
       Id = id;
       Title = title;
    }
    public int Id { get; set; }
    public string Title { get; set; }
}

public class CalendarService
{
    public static List<Calendar> GetAll()
    {
        var list = new List<Calendar>();

        var db = new mssql2();
        db.set("s1");
        string sql = @"select * from [cal]";
        var dr = db.dr(sql);
        while (dr.Read())
        {
            // Use the constructor to create a new Calendar item
            list.Add(new Calendar((int)dr["id"], dr["title"].ToString()));
        }
        return list;
    }
}
Improve this answer
Sign up to request clarification or add additional context in comments.

7 Comments

Can you help me scaffold something up? I want to separate out the data access etc... what does that look like though? Do you use Interfaces etc--?
@ScottK, see update. You can use an interface if you want. For small applications I don't really use interfaces as they're just an additional layer that I don't need. They're not hard to add though if you decide you want one.
looks great, how would you you handle the database / sql statement -- I repeat those same 4 lines all over the place--
@ScottK, If you need to re-use code, put it in another class. You're kind of drifting away from the original question though. I think you'd actually be better served if you posted some of what you had on Code Review (codereview.stackexchange.com). A site like SO that's specifically meant for people to post code and get feedback.
didn't know about that site-- maybe I should sign up & transfer this ? there--
|
2

The general idea is for the classes to represent domain objects, and class members various properties of those domain objects. Class functions would represent what the objects can do.

In your case, it might be more fitting to remove the get_all() to a some class abstracting database operations. Calendar would have the functionalities of a calendar (getting/setting some dates, getting skip years, getting/setting some appointments); depending of what you want to accomplish with a calendar.

Object design

Improve this answer

1 Comment

great picture-- I actually have a working asp mvc app (using jquery fullcalendar) to populate/save/edit events (jquery/ajax)-(controller)-(to my own event() object) --> now I'm trying to refactor that as now I want to add multi calendars & my code is difficult to change-- THanks!
0

You're tightly coupling data access, and your "get_all" method isn't even using anything from the object of type calendar. If, as in this case, your method doesn't use any data from the instance of the class to which it belongs, then that method should either not be there, or should be a static method. My preference would be for the former -- have a class whose intent is to retrieve a calendar or calendars from the database. It is a more sensible organization of code, is more testable, can be more easily abstracted from the data layer, and it also makes your data object more portable.

Improve this answer

Comments

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.