# Un consiglio per del buon codice

**URL:** <https://www.ruby-forum.com/t/un-consiglio-per-del-buon-codice/183690>\
**Category:** IT\
**Created:** [February 18, 2010, 7:19pm UTC](https://www.ruby-forum.com/t/un-consiglio-per-del-buon-codice/183690 "2010-02-18T19:19:32Z")\
**Posts on this page:** 9\
**Page:** 1

<div class="post-metadata">

**Author:** ![triplem](https://www.ruby-forum.com/letter_avatar_proxy/v4/letter/t/b19c9b/32.png) [@triplem](https://www.ruby-forum.com/u/triplem)\
**Post date:** [February 18, 2010, 7:19pm UTC](https://www.ruby-forum.com/t/un-consiglio-per-del-buon-codice/183690/1 "2010-02-18T19:19:32Z")

</div>

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?

---

<div class="post-metadata">

**Author:** ![triplem](https://www.ruby-forum.com/letter_avatar_proxy/v4/letter/t/b19c9b/32.png) [@triplem](https://www.ruby-forum.com/u/triplem)\
**Post date:** [February 18, 2010, 7:53pm UTC](https://www.ruby-forum.com/t/un-consiglio-per-del-buon-codice/183690/2 "2010-02-18T19:53:20Z")

</div>

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 D.

> **[GitHub - bolthar/freightrain: Ruby desktop development made easy](https://github.com/bolthar/freightrain)**
>
> Ruby desktop development made easy. Contribute to bolthar/freightrain development by creating an account on GitHub.

> **[using (IMHO)](https://usingimho.wordpress.com)**
>
> Andrea Dallera 's blog

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?

---

<div class="post-metadata">

**Author:** ![triplem](https://www.ruby-forum.com/letter_avatar_proxy/v4/letter/t/b19c9b/32.png) [@triplem](https://www.ruby-forum.com/u/triplem)\
**Post date:** [February 18, 2010, 8:14pm UTC](https://www.ruby-forum.com/t/un-consiglio-per-del-buon-codice/183690/3 "2010-02-18T20:14:25Z")

</div>

2010/2/18 Marco M. [removed\_email\_address@domain.invalid](mailto:removed_email_address@domain.invalid):

> 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](http://api.rubyonrails.org/classes/ActiveRecord/Batches/ClassMethods.html)

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

pietro

---

<div class="post-metadata">

**Author:** ![triplem](https://www.ruby-forum.com/letter_avatar_proxy/v4/letter/t/b19c9b/32.png) [@triplem](https://www.ruby-forum.com/u/triplem)\
**Post date:** [February 19, 2010, 10:09am UTC](https://www.ruby-forum.com/t/un-consiglio-per-del-buon-codice/183690/4 "2010-02-19T10:09:22Z")

</div>

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

---

<div class="post-metadata">

**Author:** ![triplem](https://www.ruby-forum.com/letter_avatar_proxy/v4/letter/t/b19c9b/32.png) [@triplem](https://www.ruby-forum.com/u/triplem)\
**Post date:** [February 18, 2010, 8:30pm UTC](https://www.ruby-forum.com/t/un-consiglio-per-del-buon-codice/183690/5 "2010-02-18T20:30:13Z")

</div>

Il 18 febbraio 2010 20.13, Pietro G. [removed\_email\_address@domain.invalid](mailto:removed_email_address@domain.invalid) 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

---

<div class="post-metadata">

**Author:** ![triplem](https://www.ruby-forum.com/letter_avatar_proxy/v4/letter/t/b19c9b/32.png) [@triplem](https://www.ruby-forum.com/u/triplem)\
**Post date:** [February 19, 2010, 1:48pm UTC](https://www.ruby-forum.com/t/un-consiglio-per-del-buon-codice/183690/6 "2010-02-19T13:48:04Z")

</div>

2010/2/19 Marco M. [removed\_email\_address@domain.invalid](mailto:removed_email_address@domain.invalid)

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

---

<div class="post-metadata">

**Author:** ![triplem](https://www.ruby-forum.com/letter_avatar_proxy/v4/letter/t/b19c9b/32.png) [@triplem](https://www.ruby-forum.com/u/triplem)\
**Post date:** [February 19, 2010, 10:34am UTC](https://www.ruby-forum.com/t/un-consiglio-per-del-buon-codice/183690/7 "2010-02-19T10:34:53Z")

</div>

Mi ero perso questo prezioso commento

Pietro G. wrote:

> 2010/2/18 Marco M. [removed\_email\_address@domain.invalid](mailto:removed_email_address@domain.invalid):
> 
> 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](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} }

---

<div class="post-metadata">

**Author:** ![triplem](https://www.ruby-forum.com/letter_avatar_proxy/v4/letter/t/b19c9b/32.png) [@triplem](https://www.ruby-forum.com/u/triplem)\
**Post date:** [February 23, 2010, 3:56pm UTC](https://www.ruby-forum.com/t/un-consiglio-per-del-buon-codice/183690/8 "2010-02-23T15:56:39Z")

</div>

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)

---

<div class="post-metadata">

**Author:** ![triplem](https://www.ruby-forum.com/letter_avatar_proxy/v4/letter/t/b19c9b/32.png) [@triplem](https://www.ruby-forum.com/u/triplem)\
**Post date:** [February 23, 2010, 3:42pm UTC](https://www.ruby-forum.com/t/un-consiglio-per-del-buon-codice/183690/9 "2010-02-23T15:42:22Z")

</div>

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.