Perché un metodo con troppe linee è una cosa negativa?

Question

Nella mia applicazione Rails, ho un metodo come questo:

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 contrassegnato questo metodo come Method had too many lines. Perché è sbagliato scrivere un metodo con troppe linee? E se dovessimo fare un sacco di lavoro? Come posso ricattarlo e scrivere un buon codice?

Answer 1

Un modo è che è possibile refactoring utilizzando ternary operator, ma al costo di leggibilità.

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 

In secondo luogo, se si è in dovere di scrivere una lunghissima metodo, significa che c'è qualcosa che non va con il tuo Object Oriented Design. Forse, è necessario progettare una nuova classe per le funzionalità aggiuntive, oppure è necessario suddividere le funzionalità nella stessa classe scrivendo più metodi che, combinati, eseguono il lavoro di un singolo metodo lungo.

Perché è sbagliato scrivere un metodo con troppe righe?

Proprio come un saggio con paragrafi grandi è più difficile da leggere, allo stesso modo un programma con metodi più lunghi è difficile da leggere e meno probabile riutilizzabile. Più blocchi dividi il tuo codice, più modulatore, riutilizzabile e comprensibile sarà il tuo codice.

E se dobbiamo lavorare molto?

Se devi fare molto lavoro in esso; significa sicuramente che hai progettato le tue lezioni in un modo che non è buono. Forse, è necessario progettare una nuova classe per gestire questa funzionalità, oppure è necessario dividere questo metodo in blocchi più piccoli.

Come posso ridimensionare questo e scrivere un buon codice?

Per questo, mi raccomando a voi un grande libro chiamato: Refactoring da Martin Fowler, lui è incredibilmente sorprendente a spiegare come, quando, e perché il refactoring del codice.

Answer 2

• Supponendo che il numero di bug sia proporzionale alla lunghezza del codice, è preferibile rendere un codice più breve.
• Anche se la lunghezza del codice viene mantenuta (o potrebbe essere leggermente più lunga), è meglio scomporre un metodo lungo in quelli brevi perché è più facile per gli umani leggere un metodo breve in una volta e trovare i suoi bug piuttosto che leggere attraverso un lungo

Answer 3

Quando leggo il codice con molti metodi molto brevi, scopro che spesso ci vuole più tempo di quanto dovrebbe per capire come tutto combaci. Alcune delle ragioni sono illustrate bene nel codice di esempio.Proviamo romperlo in metodi molto piccoli:

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 

Destra fuori del blocco ci sono un paio di grossi problemi:

• Dopo aver letto il metodo cart Non ho idea di che cosa lo fa. Ciò è in parte dovuto al fatto che i valori vengono assegnati alle variabili di istanza anziché restituire valori al metodo che ha chiamato cart.
• Vorrei i nomi dei metodi process_current_user e setup_cart per dire al lettore cosa fanno. Quei nomi non lo fanno. Probabilmente potrebbero essere migliorati, ma erano i migliori che potessi inventare. Il punto è che non è sempre possibile escogitare nomi di metodi informativi.

Vorrei rendere privati ​​tutti i metodi diversi da cart. Questo introduce un altro problema. Metto insieme tutti i metodi privati ​​alla fine - nel qual caso potrei dover scorrere diversi metodi non collegati per trovarli - o alternare metodi pubblici e privati ​​attraverso tutto il codice? (Naturalmente, questo problema può essere alleviato un po 'mantenendo i file di codice piccoli e usando i mixin, assumendo che io possa ricordare quale file di codice fa cosa.)

Considera anche cosa è successo al numero di righe di codice. Con più linee di codice ci sono più opportunità di errori. (Ho intenzionalmente commesso un errore comune per illustrare questo punto: l'hai notato?) Potrebbe essere più semplice testare i singoli metodi, ma ora dobbiamo verificare che i molti metodi separati funzionino correttamente insieme.

Ora confrontiamo che per il metodo di tweaked solo un po ':

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 

Questo dice al lettore a colpo d'occhio quello che sta accadendo:

• @user è impostato su current_user se l'utente è firmato in; e
• @cart viene assegnato a un valore che dipende dal fatto che l'utente sia connesso e, in ciascun caso, sia presente un ID.

Non credo che testare un metodo di queste dimensioni sia più difficile che testare la mia precedente suddivisione del codice in metodi più piccoli. In effetti, potrebbe essere più facile garantire che i test siano completi.

Potremmo anche tornare cart anziché assegnare ad una variabile di istanza, ed evitare di assegnare un valore ad @user se è solo necessario determinare cart se l'utente ha eseguito l'accesso:

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 

Answer 4

avete grandi risposte sopra, ma mi permetto suggerire un modo diverso di scrivere lo stesso metodo ...:

# 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 

Come si può vedere, a volte se le dichiarazioni sono sprecati. Sapere quali sono i metodi restituiti quando "falliscono" potrebbe rendere la lettura del codice più facile da leggere e mantenere.

Come nota a margine, solo per capire le || dichiarazioni, si prega di notare che:

"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 :-) 

Buona fortuna!

P.S.

vorrei non crei un metodo nella classe Cart per questo (o è questa logica di controllo secondo te?):

# 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])