[Java web] - Review lại code đã được review
Bài đăng này đã không được cập nhật trong 4 năm
Mình là lập trình viên java web. Mình mới dấn thân vào mảng này thôi, tính tới thời điểm bài viết này thì vừa tròn nửa năm. 2 tháng training, 4 tháng làm dự án chính thức - cái mà sử dụng framework Spring MVC. Ở công ty mình thì code được đẩy lên github, và trước khi merge sẽ được được 1 anh senior java review. Với 1 New Dev như mình, thì thấy cái việc được người khác review code rất là hay - cái mà hồi còn sinh viên, chả ai dạy, chỉ toàn ngồi lủi thủi coi blog StudyAndShare với DayNhauHoc trên youtube. =)) Vậy ý tưởng sẽ là: lên github, đọc lại tất cả các comment review code từ trước tới nay, tổng hợp lại lời hay ý đẹp của các anh senior, sau đó biến tấu đi 1 chút vừa để thêm gia vị ngọt bùi, vừa để không lộ code dự án ra ngoài :3 Perfect! :3 và thế là mình bắt đầu ngồi xuống và cố nặn ra các lợi ích, ý nghĩa của bài viết này, để lấy cảm hứng gõ phím. Bài viết này:
- Mấy ông newbie, kiểu mới tập tành code, chắc đọc sẽ thích lắm.
, kiểu "anh ấy bá vãi, mình cũng sẽ phấn đấu để được như ảnh" - Mấy ông junior thì sẽ "uầy, đoạn này theo tôi phải thế này, thế này..." hoặc "tưởng gì, cái này ai chả biết", tranh luận và tranh luận :3
- Mấy ông senior vào đọc giải trí thì "hô hô, thằng này review vui vãi"
- Với bản thân mình thì viết thêm 1 lần, là thêm nhớ. Vậy là ai cũng được gói mang về... Nhưng đời không như mơ, khi mà bắt đầu lội lại các comment review trên github, thì thấy: "sao mình lại từng code như thế nhỉ --, mấy cái này đơn giản quá, chắc ai cũng biết rồi". => Ý định không viết nữa vụt lên. Nhưng ngẫm lại thì mình cũng ngon zai mà, mình mà từng như vậy, thì chắc tương lai cũng sẽ có ai đó gặp phải. OK viết...
Để tiện follow đoạn hội thoại, mình sẽ đặt tên mình là Alice, còn anh senior review code là Bob.
1. Nghệ thuật giấu bug
Chuyện là mình có làm 1 cái chức năng đọc dữ liệu từ file excel, để lấy data, rồi xử lý vô database. Mà cái giống liên quan tới file rất hay dễ hay xảy ra Exception đúng không? Mình code nôm na kiểu thế này
try {
readFile(file);
} catch (Exception e) {
LOGGER.error("Error: " + e.getMessage()); /*/ show báo lỗi ra màn hình, giống prinstackTrace, với System.out.print*
}
Xong cái nghĩ lại: in LOGGER ra làm gì nhỉ, dài dòng, đọc được thì đọc, không đọc thì thôi, ko xử lý nữa, cần gì LOG. Xóa dòng
LOGGER.error("Error: " + e.getMessage());
đầy tự tin Đúng hôm ông Bob review không kỹ, code được merge và được deploy lên server. Và rồi:
- Bob: chức năng chú làm lỗi rồi, code thế đấy hả?
- Alice: ơ, em test local trên máy em ngon lành mà. Anh cho em xem log với
- Bob: anh vừa coi log, mà không thấy lỗi Exception nào hết, lạ nhỉ
- Alice: lặng lẽ chạy lại code bằng IDE, để debug, mà thấy chả có lỗi vẹo gì, bắt đầu nghi ngờ về chất lượng deploy
Sau một hồi dò lại code mãi, té ra là do mình không đặt LOGGER. Thiệt là ngại với anh Bob quá đi.
// Sau khi có LOGGER, thì nó show ra exception là trên server không có quyền tạo thư mục để lưu file =)), bảo sao
2. Hạn chế tạo mới đối tượng nếu không cần thiết
1 đoạn code logic tuần tự của mình
String userId = getUserId(); // get userId từ 1 function logic cao siêu nào đó
User user2 = userDAO.getUserById(userId); // gọi ra 1 bản ghi trong database bằng userId, và đẩy vào đối tượng User
Student student = new Student(user2, getStudentId()); // Đối tượng sinh viên, được tạo bằng mã sinh viên, và 1 tài khoản User
studentDAO.createStudent(student); // Lưu đối tượng sinh viên vào database
- Bob: sao chú code dài vãi thế, nhiều biến thừa quá. Nhìn 1 line của anh đây:
studentDAO.createStudent(new Student(userDAO.getUserById(getUserId()), getStudentId());
- Alice: code dài vậy, sau này ai đọc cũng dễ hiểu anh ạ...
- Bob: hừm
- Alice: updated
3. Hạn chế tạo mới đối tượng, vẫn bị chê
Mình có viết 1 đoạn code để query từ database, đến cái đoạn get dữ liệu mình đã code như sau (trả về SUM)
if (query.uniqueResult() == null){
return 0; }
return query.uniqueResult;
Lý do mình code như vậy, là vì mình bị ám ảnh 1 cái gọi là sợ trả về null, điều mà rất dễ gây ra Exception. Và mình cũng không muốn tạo thêm 1 biến đệm để lưu trữ cho giá trị query.uniqueResult(). Sợ lại bị chê là code dài dòng.
- Bob: code ngu thế, nếu query.uniqueResult() !=null, thì hệ thống sẽ chạy lại query lần nữa ah? quá lãng phí. => gán biến cho anh.
- Alice: Phũ TT
4. Giấy trắng mực đen, không chơi nói mồm
Chuyện 1:
- Alice: "anh ơi, review code giúp em, à code này để chạy được anh phải sửa db trước đấy nhé. Chạy câu này này
UPDATE A SET A.column='value' WHERE B.column='input';
- Bob: không gửi query qua chat như thế này nhé, không quản lý được, tạo file .sql có mã ticket hẳn hoi, rồi bỏ vào source code cho anh
Chuyện 2:
- Bob: đoạn này logic sao đây? ý nghĩa business của nó là gì? sao đọc khó hiểu thế, vào cái gì, ra cái gì?
- Alice: dạ dạ...bla bla
- Bob: viết java doc cho anh
- Alice: what is it?
- Bob: là comment trước funciton thế này này
/**
*
* @param input
* @return result
* @throws Exception
*/
5. Đừng có mà hard code
Chuyện 1:
- Alice:
if (resultCode == 1) {
doSomeThing();
}
- Bob: không hard code nhé, dùng constant cho anh
- Alice:
public static final int successCode = 1;
...
if (resultCode == successCode) {
doSomeThing();
}
- Bob: ở trường không dạy, khai báo static thì phải viết hoa ah?
- Alice:
public static final int SUCCESS_CODE = 1 ;
...
if (resultCode == SUCCESS_CODE) {
doSomeThing();
}
Chuyện 2: 1 thẻ html mình code như sau:
<section class="formbase" style="min-width: 1240px;">
- Bob: Sao lại hard code 1024px vây ? check lại xem trong style đang define các dimension nào ? có bị ảnh hưởng trên các thiết bị khác nhau không ? Cần test trên cả mobile và PC.
6. Hạn chế sửa cấu trúc code cũ
Mình gặp 1 tình huống, kiểu nâng cấp chức năng website. ví dụ như sau:
Có 1 service lấy tỷ giá ngoại tệ:
Ví dụ khi chạy
finance.getRate(USD);
Kết quả trả về là 22000 VNĐ
Mặc định thì xưa nay service lấy tỷ giá là luôn so với VNĐ
Bây giờ nâng cấp chức năng, thì getRate sẽ cải tiến thêm 1 param vào nữa.
Vd: finance.getRate(USD,VNĐ) , finance.getRate(USD,Bitcoin).
Mình bắt đầu sửa code búa xua, sau khi sửa lại cái service getRate(String currencyId) thành getRate(String currencyIdIn, String curencyIdOut); thì bắt đầu mình đi lùng tất cả các chỗ code cũ, chỗ nào gọi tới getRate(); => đều sửa lại với format kiểu getRate(input,VNĐ) .
- Bob: Khi maintain nâng cấp chức năng, thì không được thay đổi cấu trúc method của source cũ ==> Đây là nguyên tắc.
Điều đó sẽ ảnh hưởng đến rất nhiều chức năng đang chạy, nó có thể 1 bên thứ 3 gọi method đó qua reflection , hoặc call method ở nhiều nơi.
==> Và khi đó cần sửa hết tất cả thay đổi đó, hàm này là 1 ví dụ.
=> Cứ giữ nguyên getRate(String currencyId) cũ, và tạo thêm getRate(String currencyIdIn, String curencyIdOut).
Trước service getRate(String currencyId) thêm @Deprecated để chú thích code service này đã cũ.
7. Không nên code kiểu void
Khi thực hiện các hàm liên quan tới database, ví dụ như insert, update... dữ liệu.
Hồi mới đầu code, mình thường toàn khai báo chúng dưới dạng void. Tức là thực thi lệnh, và không trả về giá trị gì cả.
- Bob: nên trả về 1 cái gì đó, ví dụ như trả về boolean để biết được database đã được cập nhật chưa. Hoặc trả luôn về đối tượng vừa được insert,update. Mục đích là để các "chỗ khác", khi gọi tới method này thì có thể tận dụng được luôn.
8. Đừng code chay hoài, nên tìm hiểu thêm function để tận dụng.
Chuyện 1: tại file html, mình có show ra biến introduction, cái mà được truyền attribute từ Controller theo MVC
- Alice:
<textarea disabled rows="4" cols="40" class="template" >${introduction}</textarea>
- Bob: dùng c:out nhé, tránh XSS
<textarea disabled rows="4" cols="40" class="template" ><c:out value="${introduction}"/></textarea>
- Alice: what is XSS?
Chuyện 2: code Java để check chuỗi String có nằm trong List<String> nào đó không!
- Alice:
private boolean check(String input, List<String> listString) {
for (String a : listString) {
if (input.equals(a)) {
return true;
}
}
return false;
}
- Bob: không cần viết function, dùng hàm check boolean của java, listString.contain(input) là được
9. Code đúng, không sai, nhưng nên tuân theo các thiết kế về layer chuẩn
- Bob: Một service có thể gọi tới nhiều DAO, nhưng 1 DAO chỉ nên làm 1 nghiệp vụ duy nhất. 1 controller, nếu có thể, chỉ nên làm việc với 1 service 1 service, có thể làm việc với nhiều DAO
... Bài review này xin dừng tại đây! Thanks for reading...
All rights reserved
