Getting a user's property

Question

Can this extension method be improved?

 public static string GetAvatar(this System.Security.Principal.IIdentity user)
 {
 var service = SmObjectFactory.Container.GetInstance<IApplicationUserManager>();
 return service.FindByIdAsync(int.Parse(user.GetUserId())).Result.Avatar;
 }

Now in my view I use the extension method this way:

User.Identity.GetAvatar()

But every time the page refreshes, it hits database for getting user's avatar.
I'd like to know this can be improved by caching that or something like that.

• Mathieu Guindon: Including the calling code might help give reviewers more context and room for improvement. Feel free to edit your post, and to use a title that states what the code is doing, not what you'd like out of a review.
• BCdotWEB: Why does user.GetUserId() return a string which you then need to int.Parse?
• Sirwan Afifi: Actually I have changed the identity system's Id to int

Answer 1

public static string GetAvatar(this System.Security.Principal.IIdentity user)
{
 var service = SmObjectFactory.Container.GetInstance<IApplicationUserManager>();
 return service.FindByIdAsync(int.Parse(user.GetUserId())).Result.Avatar;
} 

Do you know that you can access this static method not only as a extension method ? Assume the containing class is called UserExtensions one could execute this method using UserExtensions.GetAvatar(null) and if the creation of the service would take some time to be created it would nevertheless throw an ArgumentNullException. So adding a guard condition to the method like

if (user == null) { throw new ArgumentNullException("user"); } 

would be a good idea.

Otherwise the method is looking good.

---

By using a private static ConcurrentDictionary<string, string> you could add caching to this like so

private static ConcurrentDictionary<string, string> avatarDictionary = new ConcurrentDictionary<string, string>();
public static string GetAvatar(this System.Security.Principal.IIdentity user)
{
 if (user == null) { throw new ArgumentNullException("user"); } 
 string userId = user.GetUserId();
 string avatar;
 if (avatarDictionary.TryGetValue(userId, out avatar)) 
 {
 return avatar;
 }
 var service = SmObjectFactory.Container.GetInstance<IApplicationUserManager>();
 avatar = service.FindByIdAsync(int.Parse(user.GetUserId())).Result.Avatar;
 avatarDictionary.AddOrUpdate(userId, avatar, (key, oldValue) => avatar);
 return avatar;
}

Based on @mjolka's comment

Using a ConcurrentDictionary as a cache in this manner could be problematic, depending on the number of users and the size of the avatars. As they say, "A cache without an expiration policy is another name for a memory leak."

you should add some mechanism to let the entries of the dictionary expire.

Like @DavidArno has correctly stated in his comment

And now you have a global method with static state, which is not a good place to be.

this isn't the best way to do this. By having a static dictionary this just can't be tested correctly, because the dictionary keeps its content between tests which is a no go.

A better way would be to create an interface having the GetAvatar() method which is implemented by a simple class. The interface should then be constructor injected and stored in a private field.

public interface IAvatarService
{
 string GetAvatar(System.Security.Principal.IIdentity user);
} 

public class AvatarService : IAvatarService
{
 private ConcurrentDictionary<string, string> avatarDictionary = new ConcurrentDictionary<string, string>();
 public string GetAvatar(System.Security.Principal.IIdentity user)
 {
 if (user == null) { throw new ArgumentNullException("user"); } 
 string userId = user.GetUserId();
 string avatar;
 if (avatarDictionary.TryGetValue(userId, out avatar)) 
 {
 return avatar;
 }
 avatar = user.GetAvatar() // call of the extension method
 avatarDictionary.AddOrUpdate(userId, avatar, (key, oldValue) => avatar);
 return avatar;
 }
}

having a expire mechanism would still be needed and should be implemented.

• mjolka: Using a ConcurrentDictionary as a cache in this manner could be problematic, depending on the number of users and the size of the avatars. As they say, "A cache without an expiration policy is another name for a memory leak."
• Sirwan Afifi: Hmm. Good idea.
• David Arno: And now you have a global method with static state, which is not a good place to be.

Answer 2

Since you want to cache the avatars, an extension method isn't really a good solution as it then forces you into having global state for the cache.

Instead, create a new wrapper class than encapsulates the user object and the caching of the avatar. The following is example code using C# 6 (you'll need to adjust the syntax if using C# 5 or earlier):

class UserWithAvatar
{
 private string _avatar;
 public UserWithAvatar(IIdentity user)
 {
 if (user == null) { throw new ArgumentNullException(nameof(user)); }
 User = user;
 }
 public IIdentity User { get; }
 public string Avatar => _avatar ?? _avatar = GetAvatar();
 private string GetAvatar()
 {
 var service = SmObjectFactory.Container.GetInstance<IApplicationUserManager>();
 return service.FindByIdAsync(int.Parse(user.GetUserId())).Result.Avatar;
 }
}

• IEatBagels: The problem with this is : "Where do you cache the UserWithAvatar? This solution pushed the problem away but doesn't solve it.
• David Arno: @TopinFrassi, without access to the OP's entire application, that isn't a question that can be answered.