2

I am trying to create a list of objects. Is there a better way?

   // List
    public List<page> Select()
    {
      List<page> _list = new List<page>();
      string  SqlStatement = "select * from Pages";
      SqlConnection thisConnection = new SqlConnection(connStr);
      // Open the Connection
      thisConnection.Open();

      var thisCommand = thisConnection.CreateCommand();
      thisCommand.CommandText = SqlStatement;
      SqlDataReader thisReader = thisCommand.ExecuteReader();

      while (thisReader.Read())
      {
        // Create a new instance of the Current Page Object
        page currentPage = new page();
        // Fill the instance of the Current Page Object
        currentPage.PageID = Convert.ToInt32(thisReader["PageID"]);
        currentPage.ParentID = Convert.ToInt32(thisReader["ParentID"]);
        currentPage.CategoryID = Convert.ToInt32(thisReader["CategoryID"]);
        currentPage.Name = thisReader["Name"].ToString();
        currentPage.PageHTMLContent = thisReader["PageHTMLContent"].ToString();
        currentPage.NavigationText = thisReader["NavigationText"].ToString();
        currentPage.TopMenu = Convert.ToBoolean(thisReader["TopMenu"]);
        currentPage.SubMenu = Convert.ToBoolean(thisReader["SubMenu"]);
        currentPage.DisplayOrder = Convert.ToInt32(thisReader["DisplayOrder"]);
        currentPage.Active = Convert.ToBoolean(thisReader["Active"]);
        // Add the instance of the Current Page Object to the List<>.
        _list.Add(currentPage);
      }
      // Close the Database
      thisConnection.Close();
      return _list;      

    }
Nathan Stanford
  • 1,336
  • 3
  • 23
  • 37
  • Well you could use [L2SQL](http://msdn.microsoft.com/en-us/library/bb425822.aspx) - that might be a bit of overkill if you're trying to do something *really* simple and don't already know how to use it, though.. – BlueRaja - Danny Pflughoeft Aug 05 '10 at 15:32
  • @BlueRaja: http://blogs.msdn.com/b/adonet/archive/2008/10/29/update-on-linq-to-sql-and-linq-to-entities-roadmap.aspx – Yuriy Faktorovich Aug 05 '10 at 16:12
  • Since this is example code I was using the select * but yes I agree do not do that. – Nathan Stanford Aug 05 '10 at 16:46
  • @Yuriy: Yes, I've read that, what about it? Are you trying to claim that Linq2SQL is dead (despite the fact that it's still being [developed and maintained](http://damieng.com/blog/2009/06/01/linq-to-sql-changes-in-net-40)?) – BlueRaja - Danny Pflughoeft Aug 05 '10 at 16:48
  • @BlueRaja: That is a claim I've seen in blogs. I was more trying to point out that Microsoft is recommending the Entity Framework. – Yuriy Faktorovich Aug 05 '10 at 17:06

5 Answers5

3

Well, the easiest way is to use some kind of ORM (NHibernate, EF, etc).

If you have to pull it from the db and map it yourself, the main things I would change are:

1) Wrap your SqlConnection and SqlCommand objects in using(){} blocks.
2) Don't use Select *, call out your specific columns.
3) If you can, use a stored procedure instead of an inline sql statement.

AllenG
  • 8,112
  • 29
  • 40
  • +1 for good coding practices (using(), no select *, etc.) I haven't been a fan of sprocs lately, though. Sometimes logic belongs in the database, but I find that people often over-rely on sprocs and the code becomes difficult to manage. Getting away from the SQL statement is a must, and the ORM route has been big on that lately. (I'm currently working on a LINQ to DB2 provider for my company because our scores of SQL statements have become a mess.) – David Aug 05 '10 at 15:39
  • @David: the reason I mention stored procs is in the event ORM is not available. L2S would work as well, but I haven't used it much. Stored procs are good from a maintainence and performance standpoint, though, so if you're calling direcly to the DB, the stored proc is usually worth the effort. – AllenG Aug 05 '10 at 15:53
  • It definitely depends a lot on the environment. In my new job the sprocs are a hot mess, something that took me a long time to fix in my previous job. (Lack of source control on something that is essentially business logic code is a big thing, but so far no budget for DB Ghost.) I just can't see using sprocs for basic CRUD, though. We've evolved beyond that :) – David Aug 05 '10 at 16:11
  • @David: I agree with staying away from SPROC in general. But my guess is the developer time(and everything that goes along with making updates) is way more expensive than having a good backup tool. – Yuriy Faktorovich Aug 05 '10 at 16:25
3

Using LINQ to Datasets might make your code a little more readable. You should also be sure to wrap your objects with using statements where possible:

public List<page> Select()
{
    var sqlStatement = "select * from pages";

    var sqlResults = new DataTable();

    using(SqlConnection conn = new SqlConnection(connStr))
    {
        using(SqlCommand command = new SqlCommand(sqlStatement, conn))
        {
            var adapter = new SqlDataAdapter(command);
            adapter.Fill(sqlResults);
        }
    }

    return sqlResults.AsEnumerable().Select(r => new page {
               PageID = r.Field<int>("PageID"),
               ParentID = f.Field<int>("ParentID"),
               CategoryID = r.Field<int>("CategoryID"),
               Name = r.Field<string>("Name"),
               PageHtmlContent = r.Field<string>("PageHTMLContent"),
               // Fill the rest of the properties
               Active = r.Field<bool>("Active")
           }).ToList();
}
Justin Niessner
  • 242,243
  • 40
  • 408
  • 536
  • I've seen (int)r["PageID"] fail for int columns in the database. Why not `.Field["PageID"]`? Also you don't need `conn.Open();`, the SqlDataAdapter manages that for you. – Yuriy Faktorovich Aug 05 '10 at 16:15
  • @Yuriy Faktorovich - Stupid mistakes...doing too much at once. You're right on both counts. I've updated my answer. – Justin Niessner Aug 05 '10 at 16:31
1

I saw this earlier (Microsoft.Data.dll), still can't believe it will work.

var db = Database.OpenConnectionString(connString);
for(var row in db.Query("select * from Pages"))
{
   page currentPage = new page();
   currentPage.PageID  = row.PageID;
}

NOTE: I don't think this is smart in enterprise design... but if you want something quick and dirty....

I would recommend doing EF in this case and just mapping the table to an entity.

Community
  • 1
  • 1
Nix
  • 57,072
  • 29
  • 149
  • 198
1

Automapper may be able to help. I haven't used it yet, but it seems to do something very similar to this. There are also lots of other ORM solutions out there to map data to objects. NHibernate is a popular one.

David
  • 208,112
  • 36
  • 198
  • 279
-1

Seeing how all your property names match your column names, you could just loop through each column in the reader and use reflection to set it to the pages relevant property?

Iain Ward
  • 9,850
  • 5
  • 34
  • 41
  • While this isn't the greatest answer, is there something specific about the method that bad enough to deserve the downvote? – Yuriy Faktorovich Aug 05 '10 at 15:45
  • Thanks @Yuriy, does the downvoter wish to comment as to why they downvoted it? I was only suggesting a way he could map the columns to properties without the need for 3rd party tools. We do something similar at the place I work using Property attributes to map to columns from a table and it works really well. Saves you having to repeat the mappings all the time when you load an object from the database. – Iain Ward Aug 05 '10 at 16:11
  • Not sure who downvoted but in the past I might have done that but now I am trying to seperate the view fields from the backend, so they will not always map. – Nathan Stanford Aug 05 '10 at 16:48