Recently in my team at work, we focus on maintaining older micro-services. While this might not be the most exciting job to do, it is an opportunity to work on a developer craftsmanship. A micro-service or any code that you write, can be old after a year or even a half, cause our developer habits changes. Not only technology goes forward, but we tend to use different nuget packages and in result write the same code in a different way.
Refactoring, which I’m referring to in this post, can be playful, can be fun, but it needs to be done with caution. And foremost, we cannot go too far with it, cause drawing a line here is not a child’s play.
Simplest application possible
Here is a very simple API that fetches a user from the database and fills in his description from a different REST service. Code is written in .Net Core.
[Route("api/[controller]")]
public class UsersController : Controller
{
private readonly IConfiguration _configuration;
public UsersController(IConfiguration configuration)
{
_configuration = configuration;
}
[HttpGet("{userId}")]
public async Task<IActionResult> Get(int userId)
{
try
{
var conf = _configuration.GetSection("ConnectionStrings")["Blog"];
using (var connection = new SqlConnection(conf))
{
var user = await connection.QueryFirstOrDefaultAsync<UserDto>(
"SELECT [Id], [Name], [LastUpdatedAt] FROM [Users] WHERE Id = @Id",
new { Id = userId }).ConfigureAwait(false);
var userDesctiption = await GetUserDescription(userId);
return Json(
new {
Id = user.Id,
Name = user.Name,
LastModified = user.LastModified,
Description = userDesctiption
});
}
}
catch (Exception)
{
return StatusCode(500);
}
}
private async Task<string> GetUserDescription(int userId)
{
var client = new HttpClient();
var response = await client.GetAsync($"users/{userId}/description");
return await response.Content.ReadAsStringAsync();
}
}
As you see it almost looks as a rookie developer might write it, but it’s not that bad – configuration is injected with an interface IConfiguration.
What is bad here?
- There’s no abstractions – you cannot just swap parts of the code to different implementations. It might be useful for example to use abstraction over HttpClient
- Everything is in one class – Single Responsibility rule is non-existent
- One method does multiple things – hard to test
- It’s not written in a modular way, as an experienced developer might expect it
Have a look at projects structure – it is really minimal:
Those are the most obvious things that should be fixed. Let’s go step by step.
Database and REST calls should have it’s own classes
So I moved that to separate classes and this is how controller looks like:
[Route("api/[controller]")]
public class UsersController : Controller
{
private readonly IUsersRepository _usersRepository;
private readonly IUserDescriptionClient _userDescriptionClient;
public UsersController(IUsersRepository usersRepository, IUserDescriptionClient userDescriptionClient)
{
_usersRepository = usersRepository;
_userDescriptionClient = userDescriptionClient;
}
[HttpGet("{userId}")]
public async Task<IActionResult> Get(int userId)
{
try
{
var user = await _usersRepository.Get(userId);
var userDesctiption = await _userDescriptionClient.GetUserDescription(userId);
return Json(user);
}
catch (Exception)
{
return StatusCode(500);
}
}
}
UsersRepository now looks very decent:
public class UsersRepository : IUsersRepository
{
private static class SqlQueries {
internal static string GetUser = "SELECT [Id], [Name], [LastUpdatedAt] FROM [Users] WHERE Id = @Id";
}
private readonly IConfiguration _configuration;
public UsersRepository(IConfiguration configuration)
{
_configuration = configuration;
}
public async Task<UserDto> Get(int userId)
{
var conf = _configuration.GetSection("ConnectionStrings")["Blog"];
using (var connection = new SqlConnection(conf))
{
var user = await connection.QueryFirstOrDefaultAsync<UserDto>(
SqlQueries.GetUser,
new { Id = userId }).ConfigureAwait(false);
return user;
}
}
}
UserDescriptionClient is still very minimal:
public class UserDescriptionClient : IUserDescriptionClient
{
public async Task<string> GetUserDescription(int userId)
{
var client = new HttpClient();
var response = await client.GetAsync($"users/{userId}/description");
return await response.Content.ReadAsStringAsync();
}
}
And project structure:
This is a level of refactoring that I feel comfortable with. The code is nicely decoupled, easy to test and read. However, as a project gets larger you can refactor more to have a more shared code. If you then jump to a small project, you might want to do things ‘the right way’, so the code is ready for future. You will use your best approaches from previous projects – but isn’t that going too far?
Let’s go further
First thing I did is create a base class for my UserDescriptionClient:
public abstract class BaseClient<T> where T : class
{
public async Task<T> Get(string uri)
{
var client = new HttpClient();
var response = await client.GetAsync(uri);
if (response.IsSuccessStatusCode)
{
var contentAsString = await response.Content.ReadAsStringAsync();
if (typeof(T) == typeof(string))
{
return contentAsString as T;
}
return JsonConvert.DeserializeObject<T>(contentAsString);
}
throw new System.Exception($"Could not fetch data from {uri}");
}
public async Task Post(string uri, T data)
{
var client = new HttpClient();
var response = await client.PostAsync(
uri,
new StringContent(JsonConvert.SerializeObject(data), System.Text.Encoding.UTF8, "application/json"));
if (!response.IsSuccessStatusCode)
{
throw new System.Exception($"Could not post data to {uri}");
}
}
}
And UserDescriptionClient now gets very simple:
public class UserDescriptionClient : BaseClient<string>, IUserDescriptionClient
{
public async Task<string> GetUserDescription(int userId)
{
return await Get($"users/{userId}/description");
}
}
We can do very similar thing with UsersRepository – create a base class
public abstract class BaseRepository
{
private readonly IConfiguration _configuration;
public BaseRepository(IConfiguration configuration)
{
_configuration = configuration;
}
internal IDbConnection GetBlogConnection()
{
var conf = _configuration.GetSection("ConnectionStrings")["Blog"];
return new SqlConnection(conf);
}
}
And now users repository looks like this:
public class UsersRepository : BaseRepository, IUsersRepository
{
private static class SqlQueries {
internal static string GetUser = "SELECT [Id], [Name], [LastUpdatedAt] FROM [Users] WHERE Id = @Id";
}
public UsersRepository(IConfiguration configuration) : base(configuration) {}
public async Task<UserDto> Get(int userId)
{
using (var connection = GetBlogConnection())
{
var user = await connection.QueryFirstOrDefaultAsync<UserDto>(
SqlQueries.GetUser,
new { Id = userId }).ConfigureAwait(false);
return user;
}
}
}
We also can add more layers – there has to be a service between controller and repository.
Project tree looks like this:
And I just got started. There is actually much more that you can do:
- introduce more folders so that interfaces are in a separate directory
- create factories for everything:
- preparing controller answer
- preparing a request to REST service
- creating url for REST service
- creation of User instance
- move folders to separate libraries
- and much more…
It all depends on your imagination, but notice one thing – it didn’t actually add value to the project.
Is refactoring always good?
My refactored project is better designed and decoupled, but you never know which direction project might go. It is a threat when implementing completely new micro-service. You can implement whatever you want in the beginning, but you want to implement as much as possible so that the next developer will have an easier job to do. But would it be really easier? Trying to figure out why you wrote so much code for such a little value. In fact reading and understanding bigger project just takes more time than it should.
Did I get too far with refactoring? What do you think?
You can find code posted here on my GitHub: https://github.com/mikuam/MichalBialecki.com-refactorings