Un consiglio per del buon codice

triplem · February 18, 2010, 7:19pm

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?

triplem · February 18, 2010, 7:53pm

Non so esattamente quale sia il tuo contesto ma:

discards[i]==nil … forse intendi discards[i] = nil (assegnazione)?
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 D.

On Thu, 2010-02-18 at 19:19 +0100, Marco M. wrote:

    discard.update_attributes(
#Fa qualcos’altro
sorprese con l’eliminare elementi durante un ciclo in ruby?

triplem · February 18, 2010, 8:14pm

2010/2/18 Marco M. [email protected]:

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

triplem · February 19, 2010, 10:09am

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 Ã¨:

Cercare i record nella tabella discards, nello stato da riciclare
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

triplem · February 18, 2010, 8:30pm

Il 18 febbraio 2010 20.13, Pietro G. [email protected] 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

triplem · February 19, 2010, 1:48pm

2010/2/19 Marco M. [email protected]

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

triplem · February 19, 2010, 10:34am

Mi ero perso questo prezioso commento

Pietro G. wrote:

2010/2/18 Marco M. [email protected]:

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} }

triplem · February 23, 2010, 3:56pm

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)

triplem · February 23, 2010, 3:42pm

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.