Forum: Italian Ruby user group Un consiglio per del buon codice

Ho un task rake che ricicla degli scarti. Mi piacerebbe sapere se è
chiaro e come migliorereste questo codice:

begin
  #Fa qualcosa
  Discard.transaction do
    for discard in discards
      if defined? i; i+=1; else; i=0; end
      begin
        #Lavoro sporco
      rescue StandardError => err
        discard.update_attributes(
                          :state_id=>State::ERROR,
                          :description=>err.message)
        discards[i]==nil
      end
    end
    discards.compact!
    Discard.update_all("state_id=#{State::COMPLETED}",
                      "id in(#{discards.map{|r|r[:id]}.join(',')}) and
state_id=#{State::RECYCLED}") if discards.size > 0
  end
  #Fa qualcos'altro
  exit 0
rescue StandardError => err
  puts "Error " << err.message
  exit 9
end

Non so come verrà mostrato nella ml, al limite copy-paste in un editor.
Non mi piace quel discards[i]==nil, preferirei eliminare l'elemento
dall'interno del blocco, c'è un modo?
Stesso discorso per discards.slice i ...e a proposito, qualcuno ha avuto
sorprese con l'eliminare elementi durante un ciclo in ruby?

Non so esattamente quale sia il tuo contesto ma:
1) discards[i]==nil ... forse intendi discards[i] = nil (assegnazione)?
2) se vuoi evitare discards[i] = nil :

  index = 0
  while index < discards.length
  begin
    #lavoro sporco
     index += 1
  rescue StandardError => err
    discard.update_attributes(
                          :state_id=>State::ERROR,
                          :description=>err.message)
    discards.delete(discard)
    index -= 1
  end
che secondo me è ancora peggio :-) ma tant'è 



--
Andrea Dallera
http://github.com/bolthar/freightrain
http://usingimho.wordpress.com

On Thu, 2010-02-18 at 19:19 +0100, Marco Mastrodonato wrote:
>         discard.update_attributes(
>   #Fa qualcos'altro
> sorprese con l'eliminare elementi durante un ciclo in ruby?

2010/2/18 Marco Mastrodonato <m.mastrodonato@gmail.com>:
> Ho un task rake che ricicla degli scarti. Mi piacerebbe sapere se è
> chiaro e come migliorereste questo codice:

Ciao,

non ho capito cos'è discards; potrebbe essere importante per capire
cosa deve fare il codice.
In realtà non ho capito neanche cosa sia Discard; forse mi faccio
fuorviare dal nome, ma fa niente.
L'unica cosa che credo di aver capito è che in caso di errori nel
lavoro sporco, i record restano invariati.

Immagino che quel discards[i] == nil in realtà sia discards[i] = nil,
perché altrimenti non fa assolutamente niente.

Così, al volo, proporrei:
begin
  # Fa qualcosa
  Discart.transaction do
    for i in 0...discards.size
      discard = discards[i]
      begin
        # Lavoro sporco
      rescue StandardError => err
        discard.update_attributes(:state_id=>State::ERROR,
                                  :description=>err.message)
        discards.delete_at i
      end
    end
    if discards.any?
      Discard.update_all("state_id=#{State::COMPLETED}",
                         ["id in (?) and state_id = ?",
                          discards.map(&:id),
                          State::RECYCLED])
    end
    # Fa qualcos'altro
    exit 0
  end
rescue StandardError => err
  puts "Error " << err.message
  exit 9
end

Questo perché quel

if defined? i; i+=1; else; i=0; end

non mi piace per niente. Inoltre è sempre bene sanitizzare le
condizioni (vedi la chiamata ad update_all), anche quando le scrivi tu
e sai che non ci sono rischi di injection.
Nota anche che:

Discard.update_all("state_id=#{State::COMPLETED}",
                   ["id in (?) and state_id = ?",
                    discards.map(&:id),
                    State::RECYCLED])

è identico a:

Discard.scoped(:conditions => ["id in (?) and state_id = ?",
                               discards.map(&:id),
                               State::RECYCLED
                              ]).update_all("state_id=#{State::COMPLETED}")

per cui si può scegliere quella che sembra più chiara.

Infine, se quei discards sono record (ad esempio, se fai discards =
Discard.all(...)), potresti considerare l'idea di usare find_each, che
riduce i problemi in caso di tanti record.

http://api.rubyonrails.org/classes/ActiveRecord/Batches/ClassMethods.html


Spiegaci meglio il problema, magari ci viene un'idea migliore!


pietro

Il 18 febbraio 2010 20.13, Pietro Giorgianni <giorgian@gmail.com> ha
scritto una sciocchezza tremenda, scusabile (forse) perché in pieno
calo di zuccheri:
>        discards.delete_at i
>  end
> rescue StandardError => err
>  puts "Error " << err.message
>  exit 9
> end

Ecco, quel codice è buggatissimo. Pregasi far finta di non averlo mai
letto e che invece ci fosse questo:

begin
  # Fa qualcosa
  Discart.transaction do
    for i in 0...discards.size
      discard = discards[i]
      begin
        # Lavoro sporco
      rescue StandardError => err
        discard.update_attributes(:state_id=>State::ERROR,
                                  :description=>err.message)
        discards[i] = 0
      end
    end
    discards.compact!
    if discards.any?
      Discard.update_all("state_id=#{State::COMPLETED}",
                         ["id in (?) and state_id = ?",
                          discards.map(&:id),
                          State::RECYCLED])
    end
    # Fa qualcos'altro
    exit 0
  end
rescue StandardError => err
  puts "Error " << err.message
  exit 9
end



pietro

Ciao Andrea e Pietro

Intanto chiedo scusa per quel discards[i]==nil, naturalmente è 
un'assegnazione, ho fatto un pò di caciara coi copia incolla per 
semplificare l'esempio.

Cerco di spiegare un minimo di contesto: sto modificando un task batch 
di un applicativo rails usato nella mia azienda. Credo sia un'operazione 
molto comune nell'informatica, sostanzialmente lo scopo è:

1. Cercare i record nella tabella discards, nello stato da riciclare
2. Ciclare su ognuno per elaborarli e modificare lo stato in base 
all'esito

Nella mia soluzione, se l'elaborazione ha esito negativo modifico lo 
stato e rimuovo l'elemento dall'array discards perchè i restanti 
positivi vengono usati per reperire gli id ed eseguire la update_all

I records possono essere anche migliaia e per evitare tutte quelle 
update ne ho preferito una finale. Che pericoli di sql injection 
potrebbero esserci?
Le anomalie, solitamente, sono poche per cui mi sono permesso l'update 
secca nel ciclo, in questo modo catturerei anche il messaggio. Poi, una 
commit finale nel caso si concluda bene tutto il giro.
Dovrò poi verificare che funzioni bene perchè provando a bloccare il 
debug nel mezzo dell'elaborazione, non è mica stata eseguita la 
rollback.

Andrea, anche a me piace meno la tua versione e la riga 
discards.delete(discard) credo sia più lenta della slice, che usa un 
indice.

Pietro, nella tua invece non ho capito perchè usi discards[i] = 0, la 
compact non elimina le righe a zero.

Forse quella che mi piace di più è questa:

begin
  #Fa qualcosa
  Discard.transaction do
    discards.size.times do |i|
      begin
        #Lavoro sporco
      rescue StandardError => err
        discard.update_attributes(
                          :state_id=>State::ERROR,
                          :description=>err.message)
        discards[i]=nil
      end
    end
    discards.compact!
    Discard.update_all("state_id=#{State::COMPLETED}",
                      "id in(#{discards.map{|r|r[:id]}.join(',')}) and 
state_id=#{State::RECYCLED}") if discards.size > 0
  end
  #Fa qualcos'altro
  exit 0
rescue StandardError => err
  puts "Error " << err.message
  exit 9
end

Mi ero perso questo prezioso commento

Pietro Giorgianni wrote:
> 2010/2/18 Marco Mastrodonato <m.mastrodonato@gmail.com>:
> 
> non ho capito cos'� discards; potrebbe essere importante per capire
> cosa deve fare il codice.
> In realt� non ho capito neanche cosa sia Discard; forse mi faccio
> fuorviare dal nome, ma fa niente.
> L'unica cosa che credo di aver capito � che in caso di errori nel
> lavoro sporco, i record restano invariati.

Discard è un modello rails, una tabella

> 
> Questo perch� quel
> 
> if defined? i; i+=1; else; i=0; end

Si, è abbastanza brutto, ogni tanto mi escono queste cose quando devo 
creare un indice

> 
> non mi piace per niente. Inoltre � sempre bene sanitizzare le
> condizioni (vedi la chiamata ad update_all), anche quando le scrivi tu
> e sai che non ci sono rischi di injection.
> Nota anche che:
> 
> Discard.update_all("state_id=#{State::COMPLETED}",
>                    ["id in (?) and state_id = ?",
>                     discards.map(&:id),
>                     State::RECYCLED])
> 
> � identico a:
> 
> Discard.scoped(:conditions => ["id in (?) and state_id = ?",
>                                discards.map(&:id),
>                                State::RECYCLED
>                               ]).update_all("state_id=#{State::COMPLETED}")
> 
> per cui si pu� scegliere quella che sembra pi� chiara.

Il risultato mi sembra identico, ma nella seconda viene rieseguita la 
select

> 
> Infine, se quei discards sono record (ad esempio, se fai discards =
> Discard.all(...)), potresti considerare l'idea di usare find_each, che
> riduce i problemi in caso di tanti record.
> 
> http://api.rubyonrails.org/classes/ActiveRecord/Batches/ClassMethods.html
> 
> 
> Spiegaci meglio il problema, magari ci viene un'idea migliore!
> 
> 
> pietro

Non ho mai avuto occasione di sperimentare queste select di gruppo, 
dovrei capire come vengono eseguite le query e da che quantità di 
records si iniziano ad apprezzarne i benefici, credo sia fortemente 
legato al server.

Comunque questa è la versione con la selezione:

discards = Discard.to_recycle.including(:container)
begin
  #Fa qualcosa
  Discard.transaction do
    discards.size.times do |i|
      begin
        #Lavoro sporco
      rescue StandardError => err
        discard.update_attributes(
                          :state_id=>State::ERROR,
                          :description=>err.message)
        discards[i]=nil
      end
    end
    discards.compact!
    Discard.update_all("state_id=#{State::COMPLETED}",
                      "id in(#{discards.map{|r|r[:id]}.join(',')}) and 
state_id=#{State::RECYCLED}") if discards.size > 0
  end
  #Fa qualcos'altro
  exit 0
rescue StandardError => err
  puts "Error " << err.message
  exit 9
end

#models/discard.rb
named_scope :to_recycle, :conditions => {:state_id => State::RECYCLED}
named_scope :including, lambda { |*args| {:include => args} }

2010/2/19 Marco Mastrodonato <m.mastrodonato@gmail.com>

Io farei proprio a meno di preoccuparmi di accedere all'array discards 
per
poi smanacciarlo con la compact, oltretutto non so neanche che cosa 
succede
a mettere l'accesso alla collezione x come array dentro un iteratore.

Farei una cosa come:

recoverable = discards.select do |d|
  begin
    # lavoro sporco
    true
  rescue
     # aggiorna records sballati con il messaggio di errore.
     false
  end
end

Ora recoverable contiene gli oggetti che puoi aggiornare come 
recuperati..
per il resto, e' vero che puoi fare collection.map(&:id) al posto di
collection.map {|r| r.id} e che facendo fare il lavoro ad ActiveRecord 
puoi
usare id in(?) e passargli l'array di id senza dover fare tu la join con 
le
','.

Ciao

Grazie Luca, la select è l'ideale per creare un comodo ciclo senza 
doversi preoccupare dell'indice per la rimozione degli elementi. Grazie 
anche per avermi evidenziato la modifica all'update_all che aveva 
scritto Pietro e che mi ero perso.

Ho apportato le modifiche all'esempio precedente:

Discard.transaction do
  bol_esito = false
  begin
    #fa qualcosa
    discards = Discard.to_recycle.including(:container).select do 
|discard|
      begin
        #lavoro sporco
        true
      rescue StandardError => err
        discard.update_attributes(:state_id => State::ERROR,
                                  :description => err.message)
        false
      end
    end
    #puts pippo #per testare la rollback
    if discards.any?
      Discard.update_all("state_id=#{State::COMPLETED}",
                        ["id in (?) and state_id = ?",
                          discards.map(&:id),
                          State::RECYCLED]
                      )
    end
    #fa qualcosa
    bol_esito = true
  rescue StandardError => err
    puts err.message
    raise ActiveRecord::Rollback
  end
  bol_esito ? exit(0) : exit(9)
end

Questa versione mi piace decisamente di più. Se qualcuno ha ancora 
qualche consiglio per migliorarlo, ben venga. Anche approcci totalmente 
differenti.

Chiedo scusa, non mi ero accorto che la exit 9 era dentro la transazione 
e veniva eseguita la rollback, questa funziona:

bol_esito = false
Discard.transaction do
  begin
    #fa qualcosa
    discards = Discard.to_recycle.including(:container).select do 
|discard|
      begin
        #lavoro sporco
        true
      rescue StandardError => err
        discard.update_attributes(:state_id => State::ERROR,
                                  :description => err.message)
        false
      end
    end
    #puts pippo #per testare la rollback
    if discards.any?
      Discard.update_all("state_id=#{State::COMPLETED}",
                        ["id in (?) and state_id = ?",
                          discards.map(&:id),
                          State::RECYCLED]
                      )
    end
    #fa qualcosa
    bol_esito = true
  rescue StandardError => err
    puts err.message
    raise ActiveRecord::Rollback
  end
end
bol_esito ? exit(0) : exit(9)