Constructive Criticism

[motil]

Hey,

Well based on Gary example I've build this code:

protected void Page_Load(object sender, EventArgs e)
    {
        List<DataCollection>  mycollection = GetData();
      
        foreach(DataCollection dc in mycollection)
        {
            Trace.Warn(dc.id.ToString());
            Trace.Warn(dc.loginName);
        }
    }


    public List<DataCollection> GetData()
    {
        using (SqlConnection connection = new SqlConnection(GetConnectionString()))
        {
            using (SqlCommand command = new SqlCommand("SELECT id,login_name FROM users_details",connection ))
            {
                connection.Open();
                return CollectData(command.ExecuteReader());
            }
        }
    }

    public List<DataCollection> CollectData(SqlDataReader reader)
    {
        List<DataCollection> dataCollection = new List<DataCollection>();
        while (reader.Read())
        {
            DataCollection myClass = new DataCollection();
            myClass.id = (int)reader[0];
            myClass.loginName = (string)reader[1];
            dataCollection.Add(myClass);
        }
        return dataCollection;
    }

now some questions if i may:

A) is this a good practice of pulling data?
B) though i fully understand how this code works, i don't understand why using this method is better practice then returning datasets or arraylist for example?
C) should i always pull data this way and this is mean i should build a custom class for every query i have ?

thanks.

[mendhak]

A) Almost. Stop passing the reader around, it's not a good idea. You haven't closed your reader. Do your object creation and reader looping in the GetData method itself.

B) It's going to be strongly typed when you use it on your page. Go ahead and try it. None of this ds.Tables[0].Rows[4]["colname"].ToString() stuff. Instead, you'd have something like dcList[4].LoginName; You know exactly what you're dealing with.

C) You should build custom classes that closely match the types of data in your database, not the queries. So if your site's about movies, create classes that represent movies and its related entities.

[motil]

thank you the answer Mend
ok i understands and agree with A & B but i do have some questions about C:

are you saying i should build classes for my db tables and every time i need to get info from certain table i'll create an instance of that table class and use the data i need ?

also, what with Queries that involves INNER JOIN ?, I'm sure that i'm still don't see the big picture, but i think i'm getting there, i really want to understand this concept please explain a bit more

[mendhak]

Your classes will be related to each other. An inner join is essentially a way of saying "These actors in my inner join are related to this movie." Therefore, the Actors are a property of the Movie. It can be another property in the Movie class.

public List<Actor> Actors { get; set;}


When you do your select with an inner join, you populate the usual Movie properties, you then also populate the Actors.

You eventually end up building a relationship between your classes which closely matches the database setup. In fact, if you look at your database diagram, your end goal is going to be a class diagram that closely resembles it.

[mendhak]

The other thing about this is that it's going to force you to think in terms of entities and relationships. In turn, that'll force you to separate your logic into different methods instead of a big amalgamation. I'll stop there, ask questions.

[motil]

let's say i have soccer team, and i need to to pull data of the player id, player name team name and team arena name

so it will be something like this in SQL terms

SELECT A.player_id,A.player_name,B.team_name,B.team_arena 
FROM players_details A INNER JOIN teams_details B ON (A.team_id = B.team_id)

now, do i need to have 2 classes for both the player details and team details (both classes will have more details (properties) that i don't use in this query)

or i should build one class that contain both the the team and players properties ?

[mendhak]

Two classes.

public class Player
{
public int PlayerID {get; set;}
public string Name {get; set;}
public Team PlayerTeam {get; set;}
}

public class Team
{
public int TeamID {get; set;}
public string TeamName {get; set;}
public string Arena {get; set;}
}

Now you run your SQL query. When doing your loop of the reader or dataset, create a new Player object, populate the ID and name. Create a new Team object, populate its name and arena, assign the Team object to the Player's PlayerTeam property.

I won't go into too much detail, but note that this is a one way relationship. You could theoretically have a property of Player called Team. You could have a property of Team called Players but that don't concern yourself with that right now. Right now, focus on bringing it close to what's in the database.

[motil]

I need to go to sleep. work in 7 hours but i must try it first :B , i let you know once i'm done.

[motil]

ok i think i got it.

is this what you meant?

public class PlayersDetails
{
    public int id { get; set; }
    public string firstName {get;set;}
    public string lastName { get; set; }
    public UserDetails userDetails {get;set;}
}

public class UserDetails
{
    public int id { get; set; }
    public string teamName { get; set; }
}

public partial class _Default : System.Web.UI.Page 
{
    protected void Page_Load(object sender, EventArgs e)
    {
        List<PlayersDetails> lstPlayerDetails = GetData();
        foreach (PlayersDetails   ff in lstPlayerDetails)
        {
            Trace.Warn(ff.firstName);
            Trace.Warn(ff.lastName);
            Trace.Warn(ff.userDetails.teamName);
        }
    }

    protected List<PlayersDetails> GetData()
    {
        using (SqlConnection connection = new SqlConnection(GetConnectionString()))
        {
            using (SqlCommand command = new SqlCommand("SELECT A.id As Player_id,A.first_name,A.last_name,B.id AS user_id," +
                                                " B.team_name" +
                                                " FROM players_details A INNER JOIN users_details B " +
                                                " ON (A.team_id=B.team_id)" + 
                                                " WHERE B.team_id = 1948", connection))
            {
                connection.Open();
                SqlDataReader reader = command.ExecuteReader();
                List<PlayersDetails> lstPlayersDetails = new List<PlayersDetails>();
                while (reader.Read())
                {
                    UserDetails userDetails = new UserDetails();
                    PlayersDetails playersDetails = new PlayersDetails();
                    userDetails.id = (int)reader["user_id"];
                    userDetails.teamName = (string)reader["team_name"];
                    playersDetails.id = (int)reader["player_id"];
                    playersDetails.firstName = (string)reader["first_name"];
                    playersDetails.lastName = (string)reader["last_name"];
                    playersDetails.userDetails = userDetails;
                    lstPlayersDetails.Add(playersDetails);
                }
                connection.Close();
                connection.Dispose();
                reader.Close();
                reader.Dispose();
                return lstPlayersDetails;
            }
        }
    }

btw, should i use both .dispose() and .close or .close()/.dispose() is enough?

any other suggestions / improvements will be very appreciated.

Thanks again Mend!

[mendhak]

That looks good. Have you used the List on your page yet? You can go a bit easy with the last bit - just close what you're using, don't worry about the Dispose. Also, move the return to the very end of the function and move the List<> declaration to the very beginning of the function.

                reader.Close();
                connection.Close();
                } 
      }
   return lstPlayersDetails;
}

[gep13]

Hey,

Nope, as far as I am aware, based on Mendhaks comments, which I am still going over in my head, you only need to call .Close(), .Dispose() is taken care of for you with the using statement.

Mendhak, question about this comment:

A) Almost. Stop passing the reader around, it's not a good idea. You haven't closed your reader. Do your object creation and reader looping in the GetData method itself.

The reason that I did this in this way is because I have an abstract base class that does this work. I then have two derived classes, one for SQL and one for MySql. I couldn't see the point in doing the looping work in both derived classes, hence I pass an IDataReader around to the base class. That sounds legitimate to me...

Comments?

Gary

[mendhak]

Passing a reader around makes for loss of ownership. So if I call some method1 and it returns a reader, then do I close the reader? Does that mean I need to close the connection too? Or if I write some method2 that takes a reader in, am I guaranteed that the fields I want will be there? Right now I'm not because I don't know what has been run against the database. The problem with the datareader is that because it's readonly forwardonly, you're holding a connection open for the duration of its usage, but the rule of thumb is to always open late, close early. You may have seen some threads in which someone decides to be clever and open a connection in a constructor somewhere, then pass it around. When you start passing these resource intensive objects around, you are opening yourself up to problems simply because the responsibility is now general rather than localized to a single method.

While having an abstract and two derived may work for your situation where I assume you're using both Sql and MySql at the same time, think about most developers - a lot of them end up coding for a "we might change data providers" scenario that never occurs. With IDataReader you do gain the advantage of being able to switch providers, but switching providers is going to have its own complications, regardless, so the gain is minimal.

[motil]

now for performance issues, using custom classes for collecting info from the DB has any performance impact on my application? is faster/slower then using the datareader or dataset ?

another thing, how can i sort the list<CustomClass> by any of its properties ?

thanks!

[gep13]

Hey,

A Generic List has a Sort Method on it:

http://msdn.microsoft.com/en-us/library/3da4abas.aspx

Which has a default implementation that may or may not sort the list in the way that you want it to.

However, you also have the ability to pass in your own class which tells the List how you are wanting things sorted:

http://msdn.microsoft.com/en-us/library/234b841s.aspx

As for performance, have a look here for a discussion on this topic:

http://www.rosscode.com/blog/index.p...&c=1&tb=1&pb=1

My personal preference would be a custom class object, in anything other than a very simple proof of concept application.

Gary

[mendhak]

Gep, nice article. Yes, that's what I'd say too, without referring to twitter or "peeps". I think you should try this in a sample app to start with at least, and you'll feel how clean and intuitive the coding is. It should take a small while to come to this way of thinking but it has its long term benefits. Try it try it try it try it

[gep13]

Try it, and grow to love it

[motil]

there is no doubt i'm going to adopt this method, I already see a lot of advantage by using it and i'm sure it's only small portion of what it can offer.

Say Gary, i didn't read yet read thoroughly the sorting examples you've supplied (I still need to work once in a while :P) but is it complicated as it looks?

[gep13]

Hey,

It takes a minute to get your head round, but once you have done it a couple of times, it should become easier.

Here is a more detailed walkthrough that might be of use to you:

http://aspalliance.com/1422_Implemen...n_Generic_List

Gary

[motil]

thanks Gary, the last link looks like it just what i needed i will go over it later @ home.

i'm not resolving this thread since there are more qustions coming up soon

[gep13]

No probs, post back when you have had a chance to look at it.

Gary

[motil]

Ok what is there "next level" to this concept ?

what i mean is, i asked in previous post in this thread if i need to create Class for each one of my SQL queries, turned out that i don't.

but do i have to create a "GetData()" method for each one of my queries ? can i create a more flexible "GetData" method that can handle more then a single query?

[gep13]

Hey,

Perhaps another example will help here.

What follows is the list of methods that I return from my database. Almost all of them return some form of ArticleDetails, CommentDetails, or CategoryDetails, or List of these, but what those ArticleDetails represents changes. i.e. GetArticles, GetArticles based on a category id, etc.

        // methods that work with categories
        public abstract List<CategoryDetails> GetCategories();
        public abstract CategoryDetails GetCategoryByID(int categoryID);
        public abstract bool DeleteCategory(int categoryID);
        public abstract bool UpdateCategory(CategoryDetails category);
        public abstract int InsertCategory(CategoryDetails category);

        // methods that work with articles
        public abstract List<ArticleDetails> GetArticles();
        public abstract List<ArticleDetails> GetArticles(int categoryID);
        public abstract int GetArticleCount();
        public abstract int GetArticleCount(int categoryID);
        public abstract List<ArticleDetails> GetPublishedArticles(DateTime currentDate);
        public abstract List<ArticleDetails> GetPublishedArticles(int categoryID, DateTime currentDate);
        public abstract int GetPublishedArticleCount(DateTime currentDate);
        public abstract int GetPublishedArticleCount(int categoryID, DateTime currentDate);
        public abstract ArticleDetails GetArticleByID(int articleID);
        public abstract bool DeleteArticle(int articleID);
        public abstract bool UpdateArticle(ArticleDetails article);
        public abstract int InsertArticle(ArticleDetails article);
        public abstract bool ApproveArticle(int articleID);
        public abstract bool IncrementArticleViewCount(int articleID);
        public abstract bool RateArticle(int articleID, int rating);
        public abstract string GetArticleBody(int articleID);

        // methods that work with comments
        public abstract List<CommentDetails> GetComments();
        public abstract List<CommentDetails> GetComments(int articleID);
        public abstract int GetCommentCount();
        public abstract int GetCommentCount(int articleID);
        public abstract CommentDetails GetCommentByID(int commentID);
        public abstract bool DeleteComment(int commentID);
        public abstract bool UpdateComment(CommentDetails article);
        public abstract int InsertComment(CommentDetails article);

Here ArticleDetails, CommentDetails and CategoryDetails are all one instance of a Custom Class that contain the definition of what that thing looks like, i.e. properties, fields, etc.

Gary

[motil]

Where do you declare those methods ?

and also, what does mean what yoo declare methods as "abstract"?

[gep13]

Hey,

This isn't something you have to worry about, what I was simply trying to show was the methods that I create in my application.

This goes back to what I was saying before, in my code, I override these methods, (since they are marked as abstract) in my derived classes for SQL Server and MySql Server. If you look at the other code I posted, you can see that I override the GetCategories() method with a specific implementation for both of the databases.

What I have shown here is basically the "contract" that each derived class has to adhere to. i.e. each derived class has to override each on the methods listed above.

Gary

[mendhak]

Don't fall for the trap of trying to do too much in a single method. You're not gaining anything by doing it (unless you like spaghetti for dinner) and there's no performance difference. Create a method anytime you need to do something. GetPlayerNameById, GetPlayerTeamByPlayerId, GetPlayers, things like that.

Basically - have your method do exactly what it says on the tin. Name it after what it's supposed to do. So that when you read it later, you know exactly what it does.

[motil]

Ok, I'm reading of the sorting methods from Gary's example above and i'm wondring, performance wise, should I use SQL sort when method or the .NET IComparable sort method?

and how do you prefer organize your classes, each one in a different page or putting them all together.

[motil]

More questions: is there any rule i should follow when creating classes? should i try and create class for each of my tables or is it sometimes better to join tables into the same class ?

[Pradeep1210]

Create class based on your program entities. They can then be filled from table or query or something else also if need be.
In most cases an entity structure would be same as your database table, so you are getting confused between the two.

[gep13]

Ok, I'm reading of the sorting methods from Gary's example above and i'm wondring, performance wise, should I use SQL sort when method or the .NET IComparable sort method?

and how do you prefer organize your classes, each one in a different page or putting them all together.

Hey,

If you can do the sort at the database level when you are requesting the data then yes, it would make sense to do this here. This may involve passing a parameter through to the query to indicate how the data should be sorted.

However, in some cases, once you have the data, you would still want to be able to perform a sort, without having to first go back to the database.

Gary

[gep13]

More questions: is there any rule i should follow when creating classes? should i try and create class for each of my tables or is it sometimes better to join tables into the same class ?

Hey,

Yip, pretty much as Pradeep said.

To carry on the example from my own site, I have a table called Article and Category in my database, and I also have a custom class in my code to represent each of these tables. An Article has a Category, i.e. there is a foreign key relationship between the two tables, however, each one of the table are represented separately in my code.

Gary

[mendhak]

Ok, I'm reading of the sorting methods from Gary's example above and i'm wondring, performance wise, should I use SQL sort when method or the .NET IComparable sort method?

and how do you prefer organize your classes, each one in a different page or putting them all together.

Depends on the situation. If you're giving the user the ability to sort by various columns, then get your data from the database without sorting, and sort it as the user asks you for it (within the limited number of rows being displayed).

[mendhak]

More questions: is there any rule i should follow when creating classes? should i try and create class for each of my tables or is it sometimes better to join tables into the same class ?

Your database tables would closely, but not necessarily exactly, match your business entity relationships. The classes that you create should represent concepts that you want to work with in your application.

[motil]

Thank you for the answers!

now, is containing methods is also one of the purposes of these classes ? for example:

i have Arena classwhich define the team arena, will it be proper if this class will contain
BuildArena and DestroyArena Methods?

[gep13]

Hey,

This is where another concept may start to poke it's head.

There is the concept of Presentation, Business Logic, and Data Access Layers, which I am sure you have heard/read about.

What we have been talking about up to now is really the Data Access Layer, the retrieval of the information from the database. To some extent, this is all that should happen in this layer, select, update, create and delete entries in your database.

Any additional logic that is performed on your information, should be performed in your Business Level logic.

Gary

[motil]

ok, so part of the BuildArena Method is logic and part of it is INSERT / DELETE SQL Commands, where should i go from here? where and how i write my Business logic layer and how do I connect between the Business layer and the Data Access Layer?

[Pradeep1210]

Separate them.
Then from your business logic layer call the database layer methods whenever you want to do any database specific operations.

[Pradeep1210]

But it all depends on how your application is designed.
You may use the two tier architecture or three tier architecture.

In two tier approach there is no separate business logic layer. It is partially in the presentation and partially in data access layer. It is usually used for simple applications where defining three layers is too much work as compared to the project as an overall.

In three tier approach (what is being discussed above) you separate it into three parts - presentation, business logic and data access layer.

[motil]

So, for example, i should have two classes for Players? one business logic and one for database layer? can someone please provide a very simple and basic example?

[motil]

My project is very large, and from former experience doing things "right" is my number one target

[gep13]

Hey,

In which case, I think an additional third layer would be beneficial, as it leaves it open for future changes/enhancements etc.

Have I directed you at this sample application before?

http://www.codeplex.com/TheBeerHouse

I think I have. This is the site that I used as the basis for my own site, and where I started to learn the concepts that we have been talking about. Take a look at it, and you will see how everything gets segregated out into the appropriate layers.

I only noticed this the other day, when I was referring someone else to this project, but start with this version:

http://thebeerhouse.codeplex.com/Rel...?ReleaseId=127

Later versions include things like the MVC Framework, and more recently Entity Framework, which we haven't discussed yet.

Gary