2015-06-07 17 views
7

Trong ứng dụng ray của tôi, tôi có một phương pháp như thế này:Tại sao một phương pháp có quá nhiều dòng là một điều xấu?

def cart 
    if user_signed_in? 
     @user = current_user 
     if @user.cart.present? 
      @cart = @user.cart 
     else 
      @cart = false 
     end 
    else 
     cart_id = session[:cart_id] 

     if cart_id.present? 
      @cart = Cart.find(cart_id.to_i) 
     else 
      @cart = false 
     end 
    end 
end 

Rubocop gắn cờ phương pháp này như Method had too many lines. Tại sao nó xấu để viết một phương thức với quá nhiều dòng? Điều gì sẽ xảy ra nếu chúng ta phải làm rất nhiều việc trong đó? Làm thế nào tôi có thể tái yếu tố này và viết mã tốt?

+1

Thật không may, điều này đã được hoãn lại như được dựa vào ý kiến. Nhưng chắc chắn là không. Các phương pháp ngắn hơn luôn tốt hơn. Các lý do thực tế cụ thể được đưa ra ở đây: https://www.cqse.eu/en/blog/the-real-benefits-of-short-methods/ –

Trả lời

6

Một cách là bạn có thể cấu trúc lại nó bằng cách sử dụng ternary operator, nhưng với chi phí dễ đọc.

def cart 
    if user_signed_in? 
    @user = current_user 
    @cart = @user.cart.present? ? @user.cart : false 
    else 
    cart_id = session[:cart_id] 
    @cart = cart_id.present? ? Cart.find(cart_id.to_i) : false 
    end 
end 

Thứ hai, nếu bạn bắt buộc phải viết một phương pháp rất dài, điều đó có nghĩa là đã xảy ra sự cố với Thiết kế hướng đối tượng của bạn. Có lẽ, bạn cần phải thiết kế một lớp mới cho các chức năng bổ sung, hoặc bạn cần phải phân chia các chức năng trong cùng một lớp bằng cách viết nhiều phương pháp khi kết hợp, làm công việc của một phương pháp duy nhất dài.

Tại sao viết một phương thức có quá nhiều dòng là xấu?

Cũng giống như một bài luận với đoạn lớn là khó khăn hơn để đọc, tương tự như vậy một chương trình với các phương pháp còn rất khó để đọc, và ít có khả năng tái sử dụng được. Bạn càng phân chia mã của mình càng nhiều, mã càng có thể điều chỉnh, có thể sử dụng lại và dễ hiểu hơn.

Điều gì xảy ra nếu chúng ta phải thực hiện nhiều công việc trong đó?

Nếu bạn phải thực hiện nhiều công việc trong đó; nó chắc chắn có nghĩa là bạn đã thiết kế các lớp học của bạn theo cách không tốt. Có lẽ, bạn cần thiết kế một lớp mới để xử lý chức năng này, hoặc bạn cần chia nhỏ phương thức này thành các phần nhỏ hơn.

Làm thế nào tôi có thể tái yếu tố này và viết mã tốt?

Cho rằng, tôi khuyên bạn nên cho bạn một cuốn sách tuyệt vời có tên là: Refactoring bởi Martin Fowler, ông là vô cùng tuyệt vời tại giải thích như thế nào, khi nào, và tại sao phải cấu trúc lại mã của bạn.

+0

"với chi phí dễ đọc" - khi bạn đang tái cấu trúc mã của mình và đây là kết quả cuối cùng, bạn nên suy nghĩ kỹ về nó – Kilves

1
  • Giả sử số lượng lỗi tỷ lệ thuận với độ dài của mã, tốt hơn là tạo mã ngắn hơn.
  • Thậm chí nếu độ dài mã được duy trì (hoặc có thể lâu hơn một chút), tốt hơn là nên phân tách một phương pháp dài thành những phương pháp ngắn. thông qua một cái dài.
1

Khi tôi đọc mã với nhiều phương pháp rất ngắn, tôi thấy rằng thường mất nhiều thời gian hơn để hiểu cách mọi thứ khớp với nhau. Một số lý do được minh họa độc đáo trong mã ví dụ của bạn.Chúng ta hãy thử phá vỡ nó thành phương pháp nhỏ:

def cart 
    if user_signed_in? 
    process_current_user 
    else 
    setup_cart 
    end 
end 

private 

def process_current_user 
    @user = current_user 
    if @user.cart.present? 
    assign_cart_when_user_present 
    else 
    assign_cart_when_user_not_present 
    end 
end 

def assign_cart_when_user_present 
    @cart = @user.cart 
end 

def assign_cart_when_user_not_present 
    @cart = false 
end 

def setup_cart 
    cart_id = session[:cart_id] 
    if cart_id.present? 
    assign_cart_when_id_present 
    else 
    assign_cart_when_id_present 
    end 
end 

def assign_cart_when_id_present 
    @cart = Cart.find(cart_id.to_i) 
end 

def assign_cart_when_id_not_present 
    @cart = false 
end 

Ngay lập tức có một vài vấn đề lớn:

  • Sau khi đọc các phương pháp cart Tôi không có ý kiến ​​gì nó có. Đó là một phần vì các giá trị được gán cho các biến mẫu thay vì trả về các giá trị cho phương thức được gọi là cart.
  • Tôi muốn tên của các phương pháp process_current_usersetup_cart để cho người đọc biết họ làm gì. Những cái tên đó không làm điều đó. Họ có lẽ có thể được cải thiện, nhưng họ là tốt nhất tôi có thể nghĩ ra. Vấn đề là không phải lúc nào cũng có thể đưa ra các tên phương thức thông tin.

Tôi muốn thực hiện tất cả các phương thức khác ngoài cart riêng tư. Điều đó giới thiệu một vấn đề khác. Tôi có đặt tất cả các phương thức riêng vào cuối cùng không - trong trường hợp nào tôi có thể phải di chuyển qua một số phương thức không liên quan để tìm chúng - hoặc tôi có luân phiên giữa các phương thức công cộng và riêng tư thông qua mã? (Tất nhiên, vấn đề này có thể được giảm nhẹ bằng cách giữ các tệp mã nhỏ và sử dụng mixin, giả sử tôi có thể nhớ tệp mã nào làm gì.)

Hãy xem xét điều gì đã xảy ra với số dòng mã. Với nhiều dòng mã hơn, có nhiều cơ hội hơn cho các lỗi. Nó có thể dễ dàng hơn để kiểm tra các phương pháp riêng lẻ, nhưng bây giờ chúng ta cần phải kiểm tra xem nhiều phương thức riêng biệt có hoạt động đúng với nhau hay không.

Bây giờ chúng ta hãy so sánh với phương pháp của bạn tinh chỉnh một chút:

def cart 
    if user_signed_in? 
    @user = current_user 
    @cart = case @user.cart.present? 
      when true then @user.cart 
      else   false 
      end 
    else 
    cart_id = session[:cart_id] 
    @cart = case cart_id.present? 
     when true then Cart.find(cart_id.to_i) 
     else   false 
    end 
    end 
end 

này cho người đọc trong nháy mắt những gì đang xảy ra:

  • @user được thiết lập để current_user nếu người dùng đang ký trong; và
  • @cart được chỉ định cho một giá trị phụ thuộc vào việc người dùng có đăng nhập hay không và trong từng trường hợp, cho dù có id hay không.

Tôi không nghĩ rằng việc thử nghiệm một phương pháp có kích thước này khó khăn hơn kiểm tra phân tích mã trước đó của tôi theo các phương thức nhỏ hơn. Trong thực tế, nó có thể được dễ dàng hơn để đảm bảo rằng các bài kiểm tra là toàn diện.

Chúng tôi cũng có thể trở cart hơn là gán nó vào một biến Ví dụ, và tránh gán một giá trị để @user nếu nó chỉ cần phải xác định cart nếu người dùng đang đăng nhập:

def cart 
    if user_signed_in? 
    cart = current_user.cart   
    case cart.present? 
    when true then cart 
    else   false 
    end 
    else 
    cart_id = session[:cart_id] 
    case cart_id.present? 
    when true then Cart.find(cart_id.to_i) 
    else   false 
    end 
    end 
end 
0

Bạn có câu trả lời tuyệt vời ở trên, nhưng cho phép tôi đề xuất một cách khác để viết cùng một phương thức ...:

# by the way, I would change the name of the method, as cart isn't a property of the controller. 
def cart 
    # Assume that `current_user` returns nil or false if a user isn't logged in. 
    @user = current_user 

    # The following || operator replaces the if-else statements. 
    # If both fail, @cart will be nil or false. 
    @cart = (@user && @user.cart) || (session[:cart_id] && Cart.find(session[:cart_id])) 
end 

Như bạn thấy, đôi khi các câu lệnh bị lãng phí. Biết được phương thức của bạn sẽ trở lại khi nào "thất bại" có thể làm cho việc viết và duy trì mã viết dễ dàng hơn.

Là một mặt lưu ý, chỉ để hiểu được || báo cáo, xin vui lòng nhận thấy rằng:

"anything" && 8 # => 8 # if the statements is true, the second value is returned 
"anything" && nil && 5 # => nil # The statement stopped evaluating on `nil`. 
# hence, if @user and @user.cart exist: 
@user && @user.cart #=> @user.cart # ... Presto :-) 

Chúc may mắn!

P.S.

tôi sẽ xem xét cách viết một phương thức trong lớp Cart cho điều này (hoặc là logic điều khiển theo ý kiến ​​của bạn?):

# in cart.rb 
class Cart 
    def self.find_active user, cart_id 
     return user.cart if user 
     return self.find(cart_id) if cart_id 
     false 
    end 
end 

# in controller: 

@cart = Cart.find_active((@user = current_user), session[:cart_id]) 
Các vấn đề liên quan