Nếu một hàm khá nhỏ và tỉ lệ cần modify là thấp, việc chia nó nhỏ hơn có cần thiết hoặc giúp ích?

#1

Bữa nay tui lôi cuốn Clean Code của uncle Bob ra đọc, sau khi đọc tới chương 3 nói về functions thì tui lôi đống code cũ ra thực hành với hai rule được đề cập là : Small và Do one thing. Và kết quả không thực sự dễ đọc hơn ban đầu. (Có thể do tui code kém).
Tuy nhiên như tiêu đề đã đề cập, việc chia nhỏ một hàm ko hề lớn cũng như không phức tạp (tui nghĩ thế) có thực là giúp ích gì không, đặc biệt nếu hàm này ít có khả năng sẽ phải modify.
Dưới đây là một filter (dispatch request từ client) có code ban đầu như sau:

@Override
public void doFilter(ServletRequest request, ServletResponse response,
        FilterChain chain)
        throws IOException, ServletException {
    HttpServletRequest httpRequest = (HttpServletRequest) request;
    HttpServletResponse httpResponse = (HttpServletResponse) response;

    ServletContext context = httpRequest.getServletContext();
    List<String> availableResources = (List<String>) context.getAttribute("AVAILABLE_RESOURCES_FOR_CLIENT_REQUEST");
    Map<String, String> roadMap = (Map<String, String>) context.getAttribute("ROAD_MAP");

    try {
        //Improve application data security
        httpResponse.setHeader("Cache-Control", "no-cache, no-store, private, must-revalidate, max-age=0, no-transform");

        //Get resource
        String uri = httpRequest.getRequestURI();
        String contextPath = context.getContextPath();
        String requestedResource = uri.substring(contextPath.length() + 1);

        //Logging (not static resources in assets folder)
        if (!uri.contains("/assets/")) {
            log("DispatchClientRequestFilter "
                    + "URI: " + uri + ", "
                    + "Resource: " + requestedResource);
        }

        boolean isAllowed = false;
        if (uri.contains("/assets/")) {
            isAllowed = true;
        }
        if (availableResources.contains(requestedResource)) {
            isAllowed = true;
        }

        if (!isAllowed) {
            httpResponse.sendError(RESOURCE_NOT_FOUND_CODE);
        }
        if (isAllowed) {
            //Mapping
            String url = null;
            if (!uri.contains("/assets/")) {
                url = roadMap.get(requestedResource);
            }
            //Dispatch
            if (url != null) {
                RequestDispatcher dispatcher = httpRequest.getRequestDispatcher(url);
                dispatcher.forward(request, response);
            }
            if (url == null) {
                chain.doFilter(request, response);
            }
        }
    } catch (IOException e) {
        log("DispatchClientRequestFilter IOEx: " + e.getMessage());
    } catch (ServletException e) {
        log("DispatchClientRequestFilter ServletEx: " + e.getMessage());
    } catch (Exception e) {
        log("DispatchClientRequestFilter Unknown: " + e.getMessage());
    }
}

Và sau khi chia nhỏ:

@Override
public void doFilter(ServletRequest request, ServletResponse response,
        FilterChain chain)
        throws IOException, ServletException {
    HttpServletRequest httpRequest = (HttpServletRequest) request;
    HttpServletResponse httpResponse = (HttpServletResponse) response;
    try {
        //Improve application data security
        httpResponse.setHeader("Cache-Control", "no-cache, no-store,"
                + " private, must-revalidate, max-age=0, no-transform");
        
        String requestedResource = getRequestedResourse(httpRequest);
        boolean isAllowed = isAllowedRequest(httpRequest, requestedResource);
        if (!isAllowed) {
            httpResponse.sendError(RESOURCE_NOT_FOUND_CODE);
        }
        if (isAllowed) {
            String url = mapResource(httpRequest, requestedResource);
            if (url != null) {
                RequestDispatcher dispatcher = httpRequest.getRequestDispatcher(url);
                dispatcher.forward(request, response);
            }
            if (url == null) {
                chain.doFilter(request, response);
            }
        }
    } catch (IOException e) {
        log("DispatchClientRequestFilter IOEx: " + e.getMessage());
    } catch (ServletException e) {
        log("DispatchClientRequestFilter ServletEx: " + e.getMessage());
    } catch (Exception e) {
        log("DispatchClientRequestFilter Unknown: " + e.getMessage());
    }
}

private boolean isAllowedRequest(HttpServletRequest request, String resource)
        throws Exception {
    if (isRequestForStaticResource(request)) {
        return true;
    }
    if (isAvailableResource(request, resource)) {
        return true;
    }
    return false;
}

private boolean isRequestForStaticResource(HttpServletRequest request)
        throws Exception {
    String uri = request.getRequestURI();
    return uri.contains("/assets/"); //css, img, js, ...
}

private boolean isAvailableResource(HttpServletRequest request, String resource)
        throws Exception {
    ServletContext context = request.getServletContext();
    List<String> availableResources
            = (List<String>) context.getAttribute("AVAILABLE_RESOURCES_FOR_CLIENT_REQUEST");
    return availableResources.contains(resource);
}

private String getRequestedResourse(HttpServletRequest request) throws Exception {
    ServletContext context = request.getServletContext();
    String contextPath = context.getContextPath();
    String uri = request.getRequestURI();
    // ContextPath/resource
    String resource = uri.substring(contextPath.length() + 1);

    if (!isRequestForStaticResource(request)) {
        log("DispatchClientRequestFilter URI: " + uri + ", Resource: " + resource);
    }
    return resource;
}

private String mapResource(HttpServletRequest request, String resource)
        throws Exception {
    String url = null;
    if (!isRequestForStaticResource(request)) {
        ServletContext context = request.getServletContext();
        Map<String, String> roadMap
                = (Map<String, String>) context.getAttribute("ROAD_MAP");
        url = roadMap.get(resource);
    }
    return url;
}
#2

bên dưới dễ đọc hơn mà, có đều sao ko xài if else mà đi xài 2 cái if thế kia :V

với lại đọc thấy logic cũng kì kì, isAllowed đâu phải là allow gì :V Có thể tách thành

if (isRequestForStaticResource(request)) {
    chain.doFilter(request, response);
} else {
    String requestedResource = getRequestedResourse(httpRequest);
    if (isAvailableResource(httpRequest, requestedResource)) {
        String url = mapResource(httpRequest, requestedResource);
        if (url != null) {
            ...
        } else { // trường hợp `roadMap.get(resource)` trả về null
            ...
        }
    } else {
        httpResponse.sendError(RESOURCE_NOT_FOUND_CODE);
    }
}

hoặc nếu roadMap.get(resource) ko bao giờ trả về null (mapResource chỉ trả về null vì đó là req static resource) thì khỏi cần if else ở đây:

if (isRequestForStaticResource(request)) {
    chain.doFilter(request, response);
} else {
    String requestedResource = getRequestedResourse(httpRequest);
    if (isAvailableResource(httpRequest, requestedResource)) {
        String url = mapResource(httpRequest, requestedResource);
        RequestDispatcher dispatcher = httpRequest.getRequestDispatcher(url);
        dispatcher.forward(request, response);
    } else {
        httpResponse.sendError(RESOURCE_NOT_FOUND_CODE);
    }
}
6 Likes
#3
3 Likes
#4

Code sau khi đã modify của cậu có nhiều vấn đề đấy :smile:

  • Phần exception log của cậu không log ra stacktrace. Cậu sẽ gặp rất nhiều vấn đề khi đưa code này lên production.
  • Cậu không nên swallow lỗi như vậy. Ví dụ, khi cậu gặp unknown exception, hiện tại cậu chỉ log 1 thông điệp nhỏ, và thực hiện các thao tác khác. Tớ không nghĩ đó là ý hay đâu.
    Cách tốt hơn là dừng toàn bộ request đó lại.
  • Một số method cậu có thể implement ngắn gọn và dễ hiểu hơn rất nhiều, nếu cậu return sớm nhất có thể.
    Ví dụ:
        //Improve application data security
        httpResponse.setHeader("Cache-Control", "no-cache, no-store,"
                + " private, must-revalidate, max-age=0, no-transform");
        
        String requestedResource = getRequestedResourse(httpRequest);
        boolean isAllowed = isAllowedRequest(httpRequest, requestedResource);
        if (!isAllowed) {
            httpResponse.sendError(RESOURCE_NOT_FOUND_CODE);
        }
        if (isAllowed) {
            String url = mapResource(httpRequest, requestedResource);
            if (url != null) {
                RequestDispatcher dispatcher = httpRequest.getRequestDispatcher(url);
                dispatcher.forward(request, response);
            }
            if (url == null) {
                chain.doFilter(request, response);
            }
        }

Hoàn toàn có thể viết lại thành

        //Improve application data security
        httpResponse.setHeader("Cache-Control", 
            "no-cache, no-store, private, must-revalidate, max-age=0, no-transform");
        
        String requestedResource = getRequestedResourse(httpRequest);
        if (!allowRequestToAccessResource(httpRequest, requestedResource)) {
            httpResponse.sendError(RESOURCE_NOT_FOUND_CODE);
            return;
        }

        String url = mapResource(httpRequest, requestedResource);
        if (url == null) {
            chain.doFilter(request, response);
            return;
        }
        RequestDispatcher dispatcher = httpRequest.getRequestDispatcher(url);
        dispatcher.forward(request, response);

Không còn if lồng nhau, và cũng ít if condition hơn, nếu cậu return sớm nhất có thể.
Hoặc

private boolean isAllowedRequest(HttpServletRequest request, String resource)
        throws Exception {
    if (isRequestForStaticResource(request)) {
        return true;
    }
    if (isAvailableResource(request, resource)) {
        return true;
    }
    return false;
}

Có thể viết ngắn gọn thành

private boolean allowRequestToAccessResource(HttpServletRequest request, String resource)
        throws Exception {
    return isRequestForStaticResource(request) || isAvailableResource(request, resource);
}

Dễ đọc hơn đúng không? Cho phép request access resource khi request yêu cầu static resource, hoặc resource được request tới available.

  • Cậu nên cân nhắc lại cách đặt tên method/biến.
    isAllowed có thể hơi trừu tượng quá (cậu cho phép cái gì làm gì? :smile: ), hay isAllowedRequest cũng không biết request đó được cho phép làm gì.
    getRequestedResourse nên đổi thành extractRequestedResourse, vì đó chính xác là điều cậu làm, tách phần resource từ request URI.

Thử đọc code dưới đây, sau khi đã sửa lại đôi chút

@Override
public void doFilter(ServletRequest request, ServletResponse response,
        FilterChain chain)
        throws IOException, ServletException {
    HttpServletRequest httpRequest = (HttpServletRequest) request;
    HttpServletResponse httpResponse = (HttpServletResponse) response;
    try {
        //Improve application data security
        httpResponse.setHeader("Cache-Control", 
            "no-cache, no-store, private, must-revalidate, max-age=0, no-transform");
        
        String requestedResource = extractRequestedResourse(httpRequest);
        if (!allowRequestToAccessResource(httpRequest, requestedResource)) {
            httpResponse.sendError(RESOURCE_NOT_FOUND_CODE);
            return;
        }

        String url = mapResource(httpRequest, requestedResource);
        if (url == null) {
            chain.doFilter(request, response);
            return;
        }
        RequestDispatcher dispatcher = httpRequest.getRequestDispatcher(url);
        dispatcher.forward(request, response);
    } catch (IOException e) {
        // Log stack trace is a must!
        log("DispatchClientRequestFilter IOEx: " + e.getMessage());
        // Do you really want to swallow this error?
    } catch (ServletException e) {
        // Log stack trace is a must!
        log("DispatchClientRequestFilter ServletEx: " + e.getMessage());
        // Do you really want to swallow this error?
    } catch (Exception e) {
        // Log stack trace is a must!
        log("DispatchClientRequestFilter Unknown: " + e.getMessage());
        // Do you really want to swallow this error?
    }
}

private boolean allowRequestToAccessResource(HttpServletRequest request, String resource)
        throws Exception {
    return isRequestForStaticResource(request) || isAvailableResource(request, resource);
}

private boolean isRequestForStaticResource(HttpServletRequest request)
        throws Exception {
    return request.getRequestURI().contains("/assets/"); //css, img, js, ...
}

private boolean isAvailableResource(HttpServletRequest request, String resource)
        throws Exception {
    ServletContext context = request.getServletContext();
    List<String> availableResources
            = (List<String>) context.getAttribute("AVAILABLE_RESOURCES_FOR_CLIENT_REQUEST");
    return availableResources.contains(resource);
}

private String extractRequestedResourse(HttpServletRequest request) throws Exception {
    ServletContext context = request.getServletContext();
    String contextPath = context.getContextPath();
    String uri = request.getRequestURI();
    // ContextPath/resource
    String resource = uri.substring(contextPath.length() + 1);

    if (!isRequestForStaticResource(request)) {
        log("DispatchClientRequestFilter URI: " + uri + ", Resource: " + resource);
    }
    return resource;
}

private String mapResource(HttpServletRequest request, String resource)
        throws Exception {
    if (isRequestForStaticResource(request)) {
        return null;
    }

    ServletContext context = request.getServletContext();
    Map<String, String> roadMap = (Map<String, String>) context.getAttribute("ROAD_MAP");
    return roadMap.get(resource);
}

Cậu cũng nên xem comment của @tntxtnt, chú ý cách đặt tên method và organize code của cậu ấy.

Giờ, việc chia nhỏ hơn có giúp ích hay không, thực ra là có đó :smile:
Ở method doFilter , cậu có thể thấy toàn bộ câu chuyện khi đọc code một cách dễ dàng.

        //Improve application data security
        httpResponse.setHeader("Cache-Control", 
            "no-cache, no-store, private, must-revalidate, max-age=0, no-transform");
        
        String requestedResource = extractRequestedResourse(httpRequest);
        if (!allowRequestToAccessResource(httpRequest, requestedResource)) {
            httpResponse.sendError(RESOURCE_NOT_FOUND_CODE);
            return;
        }

        String url = mapResource(httpRequest, requestedResource);
        if (url == null) {
            chain.doFilter(request, response);
            return;
        }
        RequestDispatcher dispatcher = httpRequest.getRequestDispatcher(url);
        dispatcher.forward(request, response);
  • Set cache control header để cải thiện data security (?)
  • Từ requested resource tách ra ở request, nếu request đó không được phép truy cập vào resource, trả về lỗi ở response.
  • Khi được phép rồi, lấy URL từ map resource. Nếu URL ấy không tồn tại, tức không có gì để làm nữa, nên mình chuyển trách nhiệm cho filter tiếp theo trong chuỗi filter.
  • Có URL rồi, forward request đó bằng dispatcher.

Cậu không cần phải biết chi tiết, thứ khiến cho cậu bị quá tải vì phải nhớ rất nhiều chỉ để hiểu đoạn code đó nói gì. Cậu chỉ cần biết câu chuyện được kể thôi. Dễ hiểu đúng không? :smile:

Khi cậu có thắc mắc “làm thế nào để biết cách tách requested request từ request?”, cậu chỉ cần tìm method đó và đọc chi tiết cài đặt. Không có bất cứ nhiễu nào khác.

Khi cậu cần định nghĩa thêm luật cho phép request access vào resource, cậu chỉ cần tập trung vào method tương ứng, mà không cần quan tâm những thứ khác. Nếu cậu để tất cả trong 1 method, hẳn nhiên cậu phải tìm đoạn cậu cần sửa khó khăn hơn, cũng như việc cậu phải đọc hết toàn bộ method để biết chắc chắn cậu không sửa ảnh hưởng tới chỗ khác.
Hoặc cậu phải thêm một khối if nữa để chắc chắn chỉ có TH mà cậu xác định sẽ kích hoạt đoạn code :smile:

Khi cậu cần viết test cho method, cậu cũng sẽ rất dễ dàng biết các kỳ vọng cho từng method con, so với việc đọc cả 1 method to.

Vậy nên, chắc chắn rồi, cậu nên chia nhỏ method của cậu ra. Nó sẽ giúp ích rất nhiều cho cậu đấy! :smile:

7 Likes
83% thành viên diễn đàn không hỏi bài tập, còn bạn thì sao?