Review và refactor opensource ERP có 1.1k sao git P1
WebVella là một opensource ERP, đã được phát triển từ .netframework và mới được nâng cấp lên .net. Dù nhận được 1.1k sao Git sau 6 năm đưa lên Github, nhưng WebVella cũng không thực sự thành công khi không nhận được sự tài trợ từ công ty lớn nào. Sự hạn chế về nguồn lực khiến WebVella thực sự hụt hơi khi nâng cấp lên .net cũng như bảo trì mã nguồn của mình. Nhưng mình thấy đấy lại là cơ hội tốt cho dân lập trình, mở ra cơ hội học hỏi lớn từ thực tiễn. Hiện tại, khi chúng ta được tiếp xúc với hầu hết các mẫu code mới tốt hơn cho ngữ cảnh hiện tại, nhưng nền tảng phát triển của những best practices và conventions đấy thì không phải ai cũng biết và hiểu. Loạt bài này sẽ cố gẳng refactor một vài phần code của WebVella để nhấn mạnh cũng như cố gắng giải thích các best practices và conventions trong kỹ thuật lập trình. Bài viết sẽ sử dụng xen kẽ tiếng Anh, tiếng Việt vì các từ chuyên ngành miễn cưỡng dịch sang tiếng Việt có thể khiến mọi người khó theo dõi.
Trước tiên, cùng xem phương thức sau đây và cố gắng tìm ra các vấn đề của nó (mình ko gọi nó là lỗi. Lỗi sẽ làm phương thức này chạy sai).
[Route("api/v3/en_US/eql-ds")]
[HttpPost]
public ActionResult DataSourceQueryAction([FromBody] JObject submitObj)
{
ResponseModel response = new ResponseModel();
response.Success = true;
if (submitObj == null)
return NotFound();
EqlDataSourceQuery model = new EqlDataSourceQuery();
#region << Init SubmitObj >>
foreach (var prop in submitObj.Properties())
{
switch (prop.Name.ToLower())
{
case "name":
if (!string.IsNullOrWhiteSpace(prop.Value.ToString()))
model.Name = prop.Value.ToString();
else
{
throw new Exception("DataSource Name is required");
}
break;
case "parameters":
var jParams = (JArray)prop.Value;
model.Parameters = new List<EqlParameter>();
foreach (JObject jParam in jParams)
{
var name = jParam["name"].ToString();
var value = jParam["value"].ToString();
var eqlParam = new EqlParameter(name, value);
model.Parameters.Add(eqlParam);
}
break;
}
}
#endregion
try
{
DataSourceManager dsMan = new DataSourceManager();
var dataSources = dsMan.GetAll();
var ds = dataSources.SingleOrDefault(x => x.Name == model.Name);
if (ds == null)
{
response.Success = false;
response.Message = $"DataSource with name '{model.Name}' not found.";
return Json(response);
}
if (ds is DatabaseDataSource)
{
var list = (EntityRecordList)dsMan.Execute(ds.Id, model.Parameters);
response.Object = new { list, total_count = list.TotalCount };
}
else if (ds is CodeDataSource)
{
Dictionary<string, object> arguments = new Dictionary<string, object>();
foreach (var par in model.Parameters)
arguments[par.ParameterName] = par.Value;
response.Object = ((CodeDataSource)ds).Execute(arguments);
}
else
{
response.Success = false;
response.Message = $"DataSource type is not supported.";
return Json(response);
}
}
catch (EqlException eqlEx)
{
response.Success = false;
foreach (var eqlError in eqlEx.Errors)
{
response.Errors.Add(new ErrorModel("eql", "", eqlError.Message));
}
return Json(response);
}
catch (Exception ex)
{
response.Success = false;
response.Message = ex.Message;
return Json(response);
}
return Json(response);
}
Review code thực sự là một công việc tốn thời gian nhưng cũng thực sự hữu ích. Nó không phải câu chuyện một chiều của người giàu kinh nghiệm hơn xem xét code của người ít kinh nghiệm hơn, hoàn toàn không phải là trách nhiệm của một cá nhân cụ thể nào trong đội. Nó hữu ích cho tất cả cá nhân trong đội cũng như giá trị chung của đội ngũ và dự án.
Hi vọng bạn đã bỏ thời gian để đọc toàn bộ phương thức trên cũng như có đánh giá về nó. Vậy phương thức này gặp phải những vấn đề gì? Dễ nhận thấy nhất là nó khá dài! Chúng ta có thể liệt kê các vấn đề của nó như sau:
- Tên phương thức không đúng conventions. Tên phương thức nên thể hiện hành động, hiện tại dang là danh từ. Nó nên là QueryByDataSource
- Tên của request parameter không hợp lý. submitObj là một tên gây nhầm lẫn ở phía server, dù nó là một cái tên hợp lý ở phía client, và đúng ngữ pháp thì nó còn phải là submittedObject. Mình đề xuất tên mới requestJObject. Việc đặt tên chưa bao giờ đơn giản và việc có một cái tên hợp lý nhất có thể rất khó khăn trong một số trường hợp. Có thể bạn sẽ không bao giờ tự mình có được một cái tên làm bạn hài lòng, đối với mình là trong trường hợp này. Và lúc này là lúc mà code review cũng như đồng nghiệp lên tiếng.
- Khởi tạo kết quả ResponseModel với Success = true. Các phương thức mà chúng ta viết để phục vụ một mục đích duy nhất (single reponsibility), và có thể xảy ra hữu hạn các trường hợp đúng để có kết quả mà consumer mong muốn. Việc khởi tạo đối tượng kết quả với Success = true có lẽ đi ngược với tư duy trên, chúng ta có hữu hạn các trường hợp lỗi cần xử lý và tất cả các trường hợp còn lại, thu được kết quả như mong muốn. Lỗi - exception - bug luôn là thứ chúng ta không nắm bắt được.
- EqlDataSourceQuery model. "model" là một cái tên chung chung. Trong một phương thức dài như thế này, việc có một cái tên chung chung sẽ gây khó cho người đọc về sau. Chúng ta có những IDE mạnh mẽ, việc đặt tên cụ thể hơn cũng giúp chức năng nhắc lệnh của IDE thông minh hơn. Không có lý gì lại không cho "model" một cái tên đẹp hơn như queryModel hay thậm chí eqlDataSourceQueryModel.
- region << Init SumitObj >>: phần này để ở đây chỉ khiến người đọc mất tập trung. Tác giả đoạn code này cũng nghĩ thế nên gói nó trong một region để tiện đóng đoạn code này lại. Vậy tại sao không tách nó ra một private function? Toàn bộ đoạn code này có thể gói lại trong 1 dòng: EqlDataSourceQuery queryModel = InitializeQueryModelFromRequestJson(requestJson); Ta cũng nên đưa đoạn này vào trong try vì nó tiềm ẩn Null Reference Exception.
- Đối với phần code còn lại, cấu trúc của nó đã đơn giản, phổ thông, dễ hiểu. Tuy nhiên, nhiều code block liên quan đến khởi tạo ResponseModel bị lặp lại. Là người đọc, chúng ta chẳng cần để mắt tới nó.
- Cấu trúc khởi tạo đối tượng kết quả đầu hàm và trả về cuối hàm đã phổ biến trong giới lập trình. Nhưng trong phương thức này, cách làm đó có phần vô dụng. Đối tượng response được khởi tạo đầu nhưng không được sử dụng nếu có exception. Rất lãng phí. Có một nguyên tắc ít được nhắc tới khi lập trình: kết thúc hàm ngay khi nó cần phải kết thúc. Trong phương thức này, toàn bộ code trong try có thể return được rồi. Chúng ta sẽ hoãn lại việc khởi tạo reponse đến cuối của try block.
Sau khi refactor, chúng ta có được kết quả như sau:
[Route("api/v3/en_US/eql-ds")]
[HttpPost]
public ActionResult QueryEqlDataByDataSource([FromBody] JObject requestJson)
{
if (requestJson == null)
{
return NotFound();
}
try
{
var queryModel = InitializeEqlQueryModel(requestJson);
var dataSource = GetDataSource(queryModel.Name);
if (dataSource == null)
{
return Ok(GetFailedResponseModel($"DataSource with name '{queryModel.Name}' not found."));
}
if (dataSource is DatabaseDataSource)
{
var result = GetDataBaseDataSourceResult(queryModel.Parameters, dataSource.Id);
return Ok(GetSuccessResponseModel(result));
}
else if (dataSource is CodeDataSource)
{
var result = GetCodeDataSourceResult(queryModel.Parameters, dataSource);
return Ok(GetSuccessResponseModel(result));
}
else
{
return Ok(GetFailedResponseModel("DataSource type is not supported."));
}
}
catch (EqlException eqlEx)
{
var errors = eqlEx.Errors.Select(error => new ErrorModel("eql", "", error.Message)).ToList();
return Ok(GetFailedResponseModel(errors));
}
catch (Exception ex)
{
return Ok(GetFailedResponseModel(ex.Message));
}
}
private DataSourceBase GetDataSource(string dataSourceName)
{
var allDataSources = this.dataSourceManager.GetAll();
var dataSource = allDataSources.SingleOrDefault(x => x.Name == dataSourceName);
return dataSource;
}
private ResponseModel GetCodeDataSourceResult(List<EqlParameter> eqlParameters, DataSourceBase dataSource)
{
var arguments = eqlParameters.ToDictionary(p => p.ParameterName, p => p.Value);
var resultObject = ((CodeDataSource)dataSource).Execute(arguments);
return new ResponseModel
{
Success = true,
Object = resultObject
};
}
private ResponseModel GetDataBaseDataSourceResult(List<EqlParameter> eqlParameters, Guid dataSourceId)
{
EntityRecordList list = this.dataSourceManager.Execute(dataSourceId, eqlParameters);
return new ResponseModel
{
Success = true,
Object = new { list, total_count = list.TotalCount }
};
}
private EqlDataSourceQuery InitializeEqlQueryModel(JObject submitObj)
{
EqlDataSourceQuery queryModel = new EqlDataSourceQuery();
foreach (var prop in submitObj.Properties())
{
switch (prop.Name.ToLower())
{
case "name":
if (!string.IsNullOrWhiteSpace(prop.Value.ToString()))
queryModel.Name = prop.Value.ToString();
else
{
throw new Exception("DataSource Name is required");
}
break;
case "parameters":
var jParams = (JArray)prop.Value;
queryModel.Parameters = new List<EqlParameter>();
foreach (JObject jParam in jParams)
{
var name = jParam["name"].ToString();
var value = jParam["value"].ToString();
var eqlParam = new EqlParameter(name, value);
queryModel.Parameters.Add(eqlParam);
}
break;
}
}
return queryModel;
}
Nhận định về kết quả:
- Một phương thức rõ ràng, dễ đọc hơn. Mạch tư duy của bạn không bị ngắt quãng với tên biến khó hiểu, các logic không cần thiết xuất hiện, giúp việc tiếp nhận ý nghĩa của phương thức này dễ hơn. Các hàm private được tạo mới cũng đơn giản đối với người đọc vì nó là đơn nhiệm. Các bạn chú ý rằng, các hàm private về cơ bản không cần try catch throw để xử lý lỗi. Chúng ta dùng try catch trong hàm private để xử lý business logic mà thôi.
- Tiêu chí của mình đối với một hàm được viết ra: nằm trong 1 trang màn hình. Một trang màn hình ở độ phân giải full HD chưa được tầm 40-50 dòng code. Một convention mà mình tuân theo nữa là sau mỗi dòng lệnh là một dòng trắng, tức là chỉ còn lại 20 dòng code thực sự cho một hàm. Đây là một kỉ luật quan trọng thúc đẩy bạn tối ưu code của mình. Không có nghĩa rằng chúng ta phải đạt được điều này trong mọi trường hợp, nhưng trong đa số trường hợp, bạn sẽ thực hiện được điều này. Viết một hàm ngắn, súc tích không phải là cắt ra hàm private vô tội vạ. Về cơ bản, phương thức sau khi refactor không khác gì phương thức trước khi refactor. Bạn sẽ thấy các ý chính của phương thức sau khi refactor không bị cắt gọt đi. Tất cả các bước vẫn nằm ở đấy. Những ý tứ bạn nhận được khi đọc 2 phương thức là như nhau.
- Comment: Chúng ta đều được hướng dẫn rằng comment code quan trọng. Chính xác là thế. Khi nào comment và khi nào không comment là quan trọng như nhau. Một điều khác nữa các bạn nên khắc cốt ghi tâm: document quan trọng nhất là code, comment quan trọng nhất là code. Phương thức sau khi refactor đọc rất xuôi, bởi việc cấu trúc lại cũng như đặt tên biến chi tiết giúp nội dung mà phương thức muốn diễn đạt sáng rõ hơn. Comment là không còn cần thiết vì các dòng code đã biểu đạt ý nghĩa của nó. Hãy dùng chính code để comment. Comment quá nhiều sẽ tốn nguồn lực để maintain chính những comments đấy sau này.
Refactoring là hoạt động tự nhiên và thiết yêu đối với lập trình. Bạn làm nó ngay khi đang thực hiện một chức năng hoặc sau khi ai đó hoàn thành một chức năng hoặc rất nhiều năm về sau này. Việc này cũng được lặp lại nhiều lần và cũng cần trí tuệ tập thể. Viết code một lần mà không cần sửa đổi là việc phi tự nhiên và phi thực tế. Khi refactor không phải sửa lỗi vì đoạn code được refactor không sai. Refactoring là nỗ lực để bắt kịp thời đại, cũng như tránh "legacy code", và là cách thực hành hiệu quả để cải thiện kĩ thuật lập trình.
Tip nhỏ cho bạn:
Một cái tên dài vẫn tốt hơn một cái tên tối nghĩa
Mã nguồn các bạn có thể xem ở đây: Git
Ngoài ra, các bạn có thể xem thêm youtube. Qúa trình làm việc trực tiếp sẽ có một số thứ bổ sung so với bài viết.
Nếu các bạn có source code nào cần review, xin để lại thông tin tại phần comment hoặc email về ore@refacore.com. Chúng ta sẽ cùng mổ xẻ và học hỏi 
All rights reserved
