Review source code.

Gần đây khi mình tham gia các buổi phỏng vấn Senior thì nhận ra một điều, hầu hết các bạn "Senior" đều có note trong CV rằng "review source code" là một phần công việc của bạn ý, có điều khi mình hỏi các bạn review source code như thế nào thì đều chỉ nói được rất chung chung theo kiểu: kiểm tra source code có dễ đọc ko, có đúng logic trong file specs ko..etc, rất hiếm người trả lời một cách bài bản.

Vậy rốt cuộc " Review source code" là task thế nào hoặc chúng ta nên trả lời cái gì khi bị hỏi: review source là review cái gì?

Table of contents

• Rule khi review source code
• Một số quan điểm review
• Review thử một đoạn source
• Authors

Rule khi review source code

1. Review dựa trên check list.

   Bạn luôn luôn phải xây dựng một bộ check list, một bộ quan điểm cho dòng dự án mình đang tham gia. Mỗi dòng dự án sẽ có độ ưu tiên khác nhau cho từng mục trong checklist, sẽ có những mục là mandatory với dự án này nhưng là optional với dự án khác.

   Check list này phải được training cho dev trong dự án để hiểu từng item một. Điều này hạn chế việc lack quan điểm review, hạn chế các sai lầm trong những lúc mệt mỏi kém tỉnh táo trong ngày làm việc căng thẳng, hạn chế sự xung đột của reviewer với developer.

2. Mọi quan điểm đúng/sai đều phải dựa trên offical document nào đó.

   Nếu là đúng sai về mặt requirement thì phải có requirement chứng minh lỗi sai của developer, nếu là đúng sai về mặt coding, framework thì phải chỉ ra được lỗi quy định trên ngôn ngữ lập trình hoặc official document của framework.

   Tuyệt đối ko dựa vào "Kinh nghiệm cá nhân" để áp đặt tư tưởng bản thân xuống team. Thậm chí kể cả coding convention cũng phải có document để refer.

Một số quan điểm review

1. Coding convention

Hầu hết các ngon ngữ lập trình, các dòng framework đều có các page define rõ các rule liên quan đến convention, ví dụ:

https://source.android.com/setup/contribute/code-style

https://developer.android.com/kotlin/style-guide

https://dart.dev/guides/language/effective-dart/style

Việc cần làm của chúng ta là đọc hiểu rồi tuân theo. Điều này khiến source code của dự án trở lên "thân thiện" với người đọc hơn, có vẻ chuyên nghiệp hơn. Bạn đã làm quen với các project chuẩn coding convention mà chuyển sang đọc source một bạn sinh viên lần đầu đi làm, hoặc các bạn làm theo kiểu free style sẽ cảm thấy rất khác biệt, đôi khi có follow coding convention hay không lại là cơ sở để đánh giá liệu rằng coder đã code đủ lâu, đã có kinh nghiệm đủ lâu với lĩnh vực mà họ đang làm hay chưa.

2. Design

Quan điểm review liên quan đến design là một quan điểm rất rộng và tốn giấy mực, nó không chỉ đơn thuần việc các module/class làm việc với nhau thế nào, tạo interface ra sao, abstraction ổn chưa..etc mà còn là việc liên quan đến việc thiết kế bao nhiêu luồng chạy cho ứng dụng là đủ, context để các task chạy có đủ lâu, đủ ổn định không, các component đã đủ yêu cầu liên quan đến security chưa… và nhiều thứ phức tạp liên quan nữa

• Security: Secure coding, việc tổ chức source code thế nào, trong code lưu các api key ra sao, có bao nhiêu module dữ liệu được đi ra ngoài, cơ chế lưu dữ liệu, mã hoá dữ liệu, cơ chế đảm bảo an toàn cho các component khi bị gọi đến, quan điểm này thật sự dài, có thể mình sẽ viết trong các seri tiếp theo tại đây (https://magz.techover.io/2021/08/02/giao-thuc-bao-mat-https-va-mitm-attacksecure-coding-p1/)
• Performance: Bao nhiêu thread là đủ, có cần pool thread không? đã giải phóng các resource chưa, dùng loại layout này đã tối ưu chưa, dùng loop nào để optimize tốc độ, có cần cache không, dùng cấu trúc dữ liệu nào để optimize đoạn xử lý này? task này chạy trên worker thread hay main thread..etc
• Cấu trúc: có cực kỳ nhiều thứ để check, vd: sử dụng pattern nào, có nên dùng DI ko, đoạn source này nên do class/moduel nào xử lý..etc Mình rất dị ứng cụm từ "dựa vào kinh nghiệm" nhưng thực sự quan điểm này phần lớn do đến từ kinh nghiệm, đại khái thì chúng ta sẽ quan tâm đến high cohesion, low coupling, tạo ra các boundaries – các ranh giới cho class/ module của chương trình

3. Mistake liên quan đến Logic code/requirement

Đây chính là phần mà hầu hết các bạn đi phỏng vấn sẽ trả lời vào, đại khái là check xem có đúng requirement ko, code logic có nhầm đoạn nào ko? có null rồi bị bug ko..etc

Review thử một đoạn source

Rồi,đó là lý thuyết, giờ chúng ta thử áp dụng để review đoạn source bên dưới

public class BroadCastTestJava extends BroadcastReceiver {
    @Override
    public void onReceive(Context context, Intent intent) {
        decodeImage(intent.getStringExtra("imageData"));
        new MyAsyncTask("A").execute();
        new MyAsyncTask("B").executeOnExecutor();
    }

    private Bitmap decodeImage(String image) {
        byte[] imgBytes = Base64.decode(image, 0);
        return BitmapFactory.decodeByteArray(imgBytes, 0, imgBytes.length);
    }

    public class MyAsyncTask extends AsyncTask {
        private String name;
        public MyAsyncTask(String name) {
            this.name = name;
        }
        @Override
        protected Object doInBackground(Object[] objects) {
            for (int i = 0; i < 10; i++) {
                Log.d("test", "My name is " + this.name + " " + i);
                try {
                    Thread.sleep(1000);
                } catch (InterruptedException e) {
                    e.printStackTrace();
                }
            }
            return null;
        }
    }

1. Dùng Tool check.

Đa số các IDE đều hỗ trợ, ví dụ android studio thì có thể làm như dưới

Hầu hết các lỗi do tool chỉ ra, đều đúng là lỗi thật và có thể fix được, nên đương nhiên, chúng ta sẽ xử lý nhé 😛

2. Coding convention Anh em thử xem có các lỗi convention nào :p

3. Design Có thể thấy có các issue sau:

• decodeImage() không được chạy trên main thread (task vụ nặng)

• public class phải chuyển thành private static class(Trong java inner class sẽ refer sang outer class -> leak memory)

• Không được dùng asynctask trên BroadcastReceiver, context của broadcast không đủ dài để xử lý background, nên phải đưa về context sống lâu hơn, ví dụ service để xử lý tiếp. Đây là vấn đề liên quan đến thiết kế các task chạy ở đâu cho phù hợp. Hoặc nếu task ngắn hạn thì có thể dùng goAsync(https://developer.android.com/reference/android/content/BroadcastReceiver#goAsync())

• Security: Liệu rằng intent gửi đến(để ra lệnh cho app decode image) có phải intent của app ko? có đúng là được phép để xử lý không? hay do attacker đang cố tình tấn công vào app? liệu rằng Broadcast có được bảo vệ bởi permission không? – phần này mình sẽ break sang một bài viết khác liên quan đến Secure Coding

4. Logic code

• decodeImage không verify intent đầu vào/ không handle exception, giả sử intent nhả ra null hoặc rỗng thì sẽ có exception xảy ra, có thể app sẽ crash??

=> gọi coder ra "nhờ" bạn ý fix thôi

Việc review code easy hơn rồi – đúng ko nào. Chắc sẽ còn nhiều lỗi nữa, anh em thử check tiếp nhé.

Tổng kết lại:

1. Chúng ta nên có 1 bộ checklist dùng để review(có thể tuỳ vào dòng dự án, mức độ khó tính của khách hàng, mức độ chặt chẽ dự án yêu cầu)- để hạn chế các mistake về mặt con người
2. Các quan điểm review đúng/sai về mặt kỹ thuật phải thuần tuý dựa vào quan điểm kỹ thuật, official document để quyết định đúng/sai, không được áp đặt dựa vào "kinh nghiệm cá nhân"

Code tiếp thôi nào !

Authors

[email protected]