views:

248

answers:

7

What is this pattern called, when you give an object a parameter in its constructor and the object populates it's fields with the parameter?

For example:

/// <summary>
/// Represents a user on the Dream.In.Code website.
/// </summary>
public class User
{       
    /// <summary>
    /// Load a user by providing an ID.
    /// </summary>
    /// <param name="ID">A user's individual ID number.</param>
    public User(string ID)
    {
        WebClient webClient = new WebClient();
        string htmlSource = webClient.DownloadString(new Uri(String.Format("http://www.dreamincode.net/forums/xml.php?showuser={0}",ID)));
        XDocument xml = XDocument.Parse(htmlSource);

        var profileXML = xml.Element("ipb").Element("profile");

        //Load general profile information.
        this.ID = profileXML.Element("id").Value;
        this.Name = profileXML.Element("name").Value;
        this.Rating = profileXML.Element("rating").Value;
        this.Photo = profileXML.Element("photo").Value;
        this.Reputation = profileXML.Element("reputation").Value;
        this.Group = profileXML.Element("group").Element("span").Value;
        this.Posts = profileXML.Element("posts").Value;
        this.PostsPerDay = profileXML.Element("postsperday").Value;
        this.JoinDate = profileXML.Element("joined").Value;
        this.ProfileViews = profileXML.Element("views").Value;
        this.LastActive = profileXML.Element("lastactive").Value;
        this.Location = profileXML.Element("location").Value;
        this.Title = profileXML.Element("title").Value;
        this.Age = profileXML.Element("age").Value;
        this.Birthday = profileXML.Element("birthday").Value;
        this.Gender = profileXML.Element("gender").Element("gender").Element("value").Value;

        //Load contact information.
        var contactXML = xml.Element("ipb").Element("profile").Element("contactinformation");
        this.AIM = contactXML.XPathSelectElement("contact[title='AIM']/value").Value;
        this.MSN = contactXML.XPathSelectElement("contact[title='MSN']/value").Value;
        this.Website = contactXML.XPathSelectElement("contact[title='Website URL']/value").Value;
        this.ICQ = contactXML.XPathSelectElement("contact[title='ICQ']/value").Value;
        this.Yahoo = contactXML.XPathSelectElement("contact[title='Yahoo']/value").Value;
        this.Jabber = contactXML.XPathSelectElement("contact[title='Jabber']/value").Value;
        this.Skype = contactXML.XPathSelectElement("contact[title='Skype']/value").Value;
        this.LinkedIn = contactXML.XPathSelectElement("contact[title='LinkedIn']/value").Value;
        this.Facebook = contactXML.XPathSelectElement("contact[title='Facebook']/value").Value;
        this.Twitter = contactXML.XPathSelectElement("contact[title='Twitter']/value").Value;
        this.XFire = contactXML.XPathSelectElement("contact[title='Xfire']/value").Value;

        //Load latest visitors.
        var visitorXML = xml.Element("ipb").Element("profile").Element("latestvisitors");
        this.Visitors = (from visitor in visitorXML.Descendants("user")
                        select new Visitor(){
                            ID = visitor.Element("id").Value,
                            Name = visitor.Element("name").Value,
                            Url = visitor.Element("url").Value,
                            Photo = visitor.Element("photo").Value,
                            Visited = visitor.Element("visited").Value,
                        }).ToList();

        //Load friends.
        var friendsXML = xml.Element("ipb").Element("profile").Element("friends");
        this.Friends = (from friend in friendsXML.Descendants("user")
                        select new Friend()
                        {
                            ID = friend.Element("id").Value,
                            Name = friend.Element("name").Value,
                            Url = friend.Element("url").Value,
                            Photo = friend.Element("photo").Value
                        }).ToList();

        //Load comments.
        var commentsXML = xml.Element("ipb").Element("profile").Element("comments");
        this.Comments = (from comment in commentsXML.Descendants("comment")
                            select new Comment()
                            {
                                ID = comment.Element("id").Value,
                                Text = comment.Element("text").Value,
                                Date = comment.Element("date").Value,
                                UserWhoPosted = new Friend()
                                {
                                    ID = comment.Element("user").Element("id").Value,
                                    Name = comment.Element("user").Element("name").Value,
                                    Url = comment.Element("user").Element("url").Value,
                                    Photo = comment.Element("user").Element("photo").Value
                                }
                            }).ToList();
    } 
}

What would this pattern be called? Also, in an effort to make my code cleaner, do you recommend I use this pattern when creating objects within objects, like in my UserWhoPosted variable. Instead of having:

UserWhoPosted = new Friend()
{
    ID = comment.Element("user").Element("id").Value,
    Name = comment.Element("user").Element("name").Value,
    Url = comment.Element("user").Element("url").Value,
    Photo = comment.Element("user").Element("photo").Value
}

I'd have:

UserWhoPosted = new Friend(myFriendXElementVariable);

And just let it parse what it needs and populate it's fields.

Thanks for the guidance.

EDIT: After reading all your suggestions I cleaned the code up a bit. Do you think this is better? What would you improve?

Thanks for taking the time to help, I'm trying to cast aside bad programming habits.

http://pastebin.com/AykLjF2i

+2  A: 

I would want to call it a variation of the Adaptor , because all you are really doing is wrapping some XML structure to a more friendly type.

Nix
+2  A: 

It's not a pattern. A design pattern is something that solves a common problem, such as how to keep clients updated when a value changes (Observer). This isn't really a problem, it's just language syntax. And the answer is yes to your second question.

Aidan
A: 

In your example, I have a preference to pass any required data into the constructor, as it defines a dependancy my class requires.

The other issue with passing xml data into a constructor as opposed to a concrete type. A concrete type defines a known set of properties or fields, where as xml doesn't guarantee structure...

Matthew Abbott
+8  A: 

For me it's more an anti-pattern : you include xml requesting, reading, and field initialization in your constructor. Maybe you could use a Factory pattern with like this :

public static class FactoryUser{
public static User GetUserFromXml(Xml){
//your code here
}
}

public static class UserWebrequester{
public static Xml GetXmlUser(id){
}
}

Maybe you can add some singleton to be able to unit test your classes.

remi bourgarel
@remi bourgarel : how a singleton will help for unit testing ?
Matthieu
@Matthieu because you can inject a dependency and mock your Factory, which is not possible with a static class because you can't implement an interface.
remi bourgarel
@remi bourgarel: Then, don't use static! =D
bloparod
it depends if you're gone do unit test ^^ You know when your colleague use static only because "it doesn't appear in intellisense when I type 'myObject.'", you just forget about unit testing.
remi bourgarel
+2  A: 

It's bad practice to do such heavy things in constructor, I suggest you to make constructor private and create static Load method that will return User's instance. I don't know how this pattern called.

Orsol
+2  A: 

Well, this is not a "known pattern", could be one that you use though (no problem there... patterns are just that, common solutions to well established problems).

However, since your user class is responsible for loading itself, I'd say that the class is working as a DAO.

As other users have stated, you have too much logic in your constructor. Constructors should only have parameters that are essential to creating the object and do as little work as possible. It would be preferable that constructors avoid throwing exceptions too; Since the more code, the higher chance of an exception,the more you do on a constructor, higher the chance it throws.

Now, take a look at your class... I see that your "User" have fields such as Name, Rating, etc. Isn't it allowed, during the lifetime of an user object, for it not to have such fields filled? Probably, when creating a new user, that would be true. So it's not a pre-requisite for the user to have this ID, which would make the constructor invalid by the rule I stated.

Try making a Load static method that returns a new User or create a Factory. It will help you in the future with maintainability, etc.

Finally, try taking a look at the Repository Pattern. You could use it instead of a plain factory to maintain your User and other objects.

Bruno Brant
+2  A: 

It's definitely a a violation of the Law of Demeter to do the :

D = comment.Element("user").Element("id").Value,

You really don't want to have such a long chain of method calls. Making the Friend ask for the values from its argument is a much better design, with a lot less coupling.

The subparts, friends, visitors, ... should populate themselves not have work done to them. This separates your concerns. It also makes testing easier as you can stub the toplevel argument without needing to go three levels deep to satisfy the constructor.

Paul Rubel