Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Im currently working on a system that will communicate with other systems via webservice (or some sort of communication). I have a system that stores all user data already and don't want to duplicate data in this new system so I have come up with a way of accessing the data when needed.

In my current system I am planning to just store the user ID from the user system and fetch the data when required. My question is, is the following code considered acceptable/understood or would you suggest an alternative way of achieving this?

public class Person
{
    private string id;
    [Transient]
    private string name;
    [Transient]
    private bool isPopulated;

    public Person(string id){
       this.id = id;
    }

    public string Id{get;set;}

    public string Name{
        get{
            init();
            return this.name;
        }
        set{
            this.name = value;
        }
    }

    private void init(){
        if(!isPopulated){
            TempPerson tempPerson = UserService.getPerson(this.id);
            this.name = tempPerson.Name;
            this.isPopulated = true;
        }
    }
}

Is there a better way to do this and are there any problem with this way?

share|improve this question

2 Answers 2

up vote 2 down vote accepted

What you have implemented is a sort of Active Record where the record itself knows how to communicate with the storage.

What is bad about your design is that this kind of code will be extremely hard to unit test. Imagine that you need to write a unit test for a class that uses Person objects. How can you prevent it from calling UserService?

The proper solution for your problem depends on use cases.

  • Will you need to update your entities and propagate those changes back to server?
  • Can you load several entities at once using methods other than UserService.getPerson(this.id);?
  • What is the lifetime of the entities loaded?

Generally the most flexible solution (as I see it) would be to implement a repository and unit-of-work pattern (similar to ISession in NHibernate or DbContext in Entity Framework). Basically it's better not to hide communication with 3rd-party but rather expose it in such a way that you have maximum control and flexibility.

Primitive implementation may look like:

public class Person
{
    public string Id { get; set; }

    public string Name { get; set; }
}

public interface ISessionFactory
{
    IUserServiceSession CreateSession();
}

public interface IUserServiceSession
{
    Person GetPerson(string id);
}

public class SessionFactory : ISessionFactory
{
    public IUserServiceSession CreateSession()
    {
        UserService userService = new UserService(); //better use dependency injection, or cache it once if it's thread-safe.
        return new UserServiceSession(userService);
    }
}

public class UserServiceSession : IUserServiceSession
{
    private readonly Dictionary<string, Person> _cache = new Dictionary<string, Person>();
    private readonly UserService _userService;

    public UserServiceSession(UserService userService)
    {
        _userService = userService;
    }

    public Person GetPerson(string id)
    {
        Person result;
        if (!_cache.TryGetValue(id, out result))
            result = _cache[id] = _userService.getPerson(id);

        return result;
    }
}
share|improve this answer
    
I like this approach. I've modified my code since to provide two Person classes, one is the base Person class that simply contains what needs to be persisted to the local database and another PersonFull class that acts as a view class that fetches the data from the service. With regards to your questions: no data will be pushed back to the remote server, it is simply lookup. There are methods to load several at once and would likely need to go down this route if performance becomes an issue and finally the lifespan isnt going to be more than a few hours. –  AverageMarcus Feb 6 '13 at 12:56

As a matter of opinion without knowing exactly what you would like to achieve, I can see some potential issues.

  • Name doesn't need a set option (except if you are populating back to the source which is not shown here), otherwise it would just cause confusion.
  • You are caching name, not retrieving again from the source for the whole life of the object, that would obviously be a problem if the source change. I don't think this is a good option, retrieving every time if possible or at least keep a expiry time for the information retrieved would help.
  • Lately if you need this there are already tools that manage this and are well proved (NHibernate for instance), maybe is better take a look at them.

    Regards,

share|improve this answer
    
I don't think I understand your first point. With regards to the second, the life of the object isn't going to be very long and during that time the chance of the source changing is highly unlikely. I will look into NHibernate. Thank you :) –  AverageMarcus Feb 5 '13 at 17:13

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.