2
\$\begingroup\$
namespace SharpDream.Api
{
    public enum SourceType
    {
        Test,
        Xml
    }

    public class UserFinder
    {
        private IUserInformationSource _sourceType;
        private IParser _parserType;
        private string _source;

        public UserFinder(SourceType sourceType)
        {
            switch (sourceType)
            {
                case SourceType.Test:
                    var testSource = new TestSource();
                    _sourceType = testSource;
                    break;
                case SourceType.Xml:
                    var xmlSource = new XmlSource();
                    var xmlParser = new XmlParser();
                    _sourceType = xmlSource;
                    _parserType = xmlParser;
                    break;
                default:
                    break;
            }
        }

        /// <summary>
        /// Returns a User object loaded with information.
        /// </summary>
        /// <param name="forumUserId">The DIC forum users ID number.</param>
        /// <returns>A User object.</returns>
        public User GetUserById(int forumUserId)
        {
            _source = _sourceType.GetResponseSource(forumUserId);
            return _parserType.ParseUserFromSource(_source);
        }
    }
}

What I'm aiming for:

  • Separation of concerns. Which is working believe it or not! It's so refreshing to feel confident that your code won't break if you change something somewhere else. I finished this bit, and now I can work on the parsing area without having to put anything else in the mental buffer.

  • Flexibility during usage.

Here is an example of how my library will be used:

UserFinder userFinder = new UserFinder(SourceType.Xml);
var foundUser = userFinder.GetUserById(1);

Compare this to the old version of the library:

//Let's create an XmlMemberFinder, since we'll use the XML api as our data source.
XmlMemberFinder xmlMemberFinder = new XmlMemberFinder();

//MemberList is the class you'll use in your application to get anything you need 
//relating to members.
MemberLister memberList = new MemberLister(xmlMemberFinder);

//This is an example of fetching and listing a user.
var member = memberList.FindMember(1);

I think I have made improvements, but there is always someone smarter out there. :)

Thanks for your time!

\$\endgroup\$
8
\$\begingroup\$

Personally I'd make the UserFinder class source-independent.
This could be either by seeing the Xml and Test sources as a mapper and the UserFinder as a model (some kind of DataMapper):

public interface UserMapperInterface {}
public class XmlSource : UserMapperInterface { // ... }
public class TestSource : UserMapperInterface { // ... }

public class UserFinder {
     public UserFinder(UserMapperInterface $source) { // ... }
     public UserFinder() { // use default source }
}

This way you make the GetUserById method source-independent. Basic usage would be:

UserFinder t = new UserFinder(new TestSource());
// UserFinder t = new UserFinder(); // using the default source
t.GetUserById(43);

This way you seperate the source logic from the operations and you can easily add new sources without touching the UserFinder class.

\$\endgroup\$
  • \$\begingroup\$ Can you explain a bit why this is different to what I'm doing? In my code I also set the type of course to be used in the constructor. The UserFinder class acts on an interface, not a concrete implementation. Thanks! :) \$\endgroup\$ – Sergio Tapia Mar 1 '11 at 16:13
  • 2
    \$\begingroup\$ @sergio : you don't have to change the UserFinder class when you add new sources. The class doesn't (need to) know which source it does use thus you programm to an interface, not to an implementation. \$\endgroup\$ – Fge Mar 1 '11 at 16:19
  • 1
    \$\begingroup\$ The brother is taking you down a whole new path here! Look up Inversion of Control. (Come back in about 6 months and let us know if you are ok..) :-) \$\endgroup\$ – Mongus Pong Mar 1 '11 at 17:23
1
\$\begingroup\$

Fge's answer (along with Mongus Pong's comment) really notes the major issue with your design. In general, you will want to avoid:

  1. Using enumerations in the place of polymorphism.
  2. Leaking the details of how you test a type into the type itself. You are doing this by explicitly referencing TestSource in UserFinder. Doing this complicates the core type with test code and/or test types that don't belong in it. Note that there are times when you'll have to break this "rule"; for example, when there is no other way to test the object in question, but generally you can avoid it.

Fge's answer provides a great fix for both of these issues (though the UserMapperInterface interface should really be named IUserMapper, but that's just nitpicking).

Once you get this pattern of IoC and Dependency Injection down you're on the cusp of something that can really transform how you write and test code: Mocking. A great introduction on the how and why of mocking can be found here.

Hope this helps.

\$\endgroup\$
0
\$\begingroup\$

This is looking pretty good to me. The main thing that stands out that I woudl change is that the UserFinder() method is somewhat large, and it isn't immediately understandable to me what each piece does.

I would try to break that up into several smaller, descriptively named methods:

public UserFinder(SourceType sourceType)        
{            
    if (requestIsTest())            
    {                
        createTestSource();
    }
    else
    {
        createXmlSource();
    }
}

Your original is more compact but less readable in my book. In six months when you come back to it, will you be able to remember what it does, or will you have to spend 10 minutes parsing it out and figuring out what it does?

\$\endgroup\$
  • \$\begingroup\$ UserFinder() is the constructor for the UserFinder.cs class. It's purpose it to set the source type the end developer is using. \$\endgroup\$ – Sergio Tapia Mar 1 '11 at 15:52

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy