3

I have my own implementation of the GetUserId() function made static to be able to retrieve the ID in static context. But I also have many places where I use standard GetUserId() function built into an asp.net UserManager library. My fix for not using different logic for the same thing is overriding the non-static method and using the static one inside it (this is inside the UserManagerService class):

public override string GetUserId(ClaimsPrincipal user)
{
    return GetUserIdStatic(user);
}

public static string GetUserIdStatic(ClaimsPrincipal user)
{
    return user.FindFirst(ClaimTypes.NameIdentifier).Value;
}

I did that because I prefer to call the non-static method in the non-static context (generally it's over 90% of the calls). So I prefer to call _userManagerService.GetUserId(User) than UserManagerService.GetUserIdStatic(User) whenever I can.

From a readability and maintainability perspective (and eventual harmful consequences that I can't foresee right now) is it better to do it the way described above; switch all calls to the static version; or some other way that I didn't think of?

Michał Gacka
  • 2,935
  • 2
  • 29
  • 45

2 Answers2

2

Making a static and non-static versions of a method that do the same thing is highly questionable.

You should replace static method to get user id with a static method or a static property to get user manager service. This would let you obtain user id in static context by calling a non-static method:

var userId = StaticGetUserManagerSerice().GetUserIdStatic(user);

or

var userId = UserManagerSerice.Instance.GetUserIdStatic(user);
Sergey Kalinichenko
  • 714,442
  • 84
  • 1,110
  • 1,523
  • This is a good answer but you shouldn't be using `var` for a certain type – Aidan Connelly Jul 28 '17 at 11:24
  • Thank you for the advice. It does look like a much better solution, although I wouldn't know how to instantiate that UserManagerService object myself since the constructor takes multiple parameters that I don't explicitly control and everywhere else DI takes care of that for me (I never instantiate the object myself). But I believe this might be outside of the scope of this question. – Michał Gacka Jul 28 '17 at 12:22
  • 1
    @AidanConnelly Using `var` helps you avoid repeating the information that you have already supplied elsewhere, such as the return type of a method. I use `var` everywhere for brevity and consistency. – Sergey Kalinichenko Jul 28 '17 at 12:26
  • @dasblinkenlight I ended up assigning the service object in Startup.cs class during configuration routine: `LogHelpers.userManagerService = app.ApplicationServices.GetService();` This way DI takes care of the object construction - I think it might be helpful to people looking at this question - shall I add that to your answer (I think it's another, .net core way of doing what you described by the custom methods). – Michał Gacka Jul 31 '17 at 12:30
1

Firstly it's not clear which class you're putting this static and non-static method in.

It seems your method is what's known as a "pure function", i.e. it simply returns the same thing regardless of the input and has no side effects. In this case it doesn't really make sense for the method to be for an instance, as it doesn't deal with the instance's data. So from that point of view, calls should be made static.

However, in accordance with OOP principals, it seems like the best place for this method would be in the User class, as a non-static method.

Aidan Connelly
  • 164
  • 3
  • 13
  • I added a small comment about where the functions are implemented. I do agree, that it doesn't make obvious sense for it to be an instance method but it does seem like a readability improvement, since almost all operations performed by that class's methods are performed using an instance that's being injected. – Michał Gacka Jul 28 '17 at 11:14