25-09-2011, 01:16 PM
(Modification du message : 25-09-2011, 01:20 PM par Sephi-Chan.)
(25-09-2011, 11:09 AM)Maz a écrit : Le formulaire renvois sur ça:
def create
if current_user.city.ants.size
@decalage = 0
else
@lastAnt = current_user.city.ants.find_all().last
@decalage = @lastAnt.born - Time.now - 28800
if @decalage < 0
@decalage = 0
end
end
@quantity = params[:quantity].to_i - 1
for i in 0 .. @quantity
current_user.city.ants.build(ort => "bip",
:born => Time.now + i*6 + @decalage,
:name => ActiveRecord::Base.connection.select_value("SELECT nextval('fourmis_" + current_user.username + "_id_seq')"))
end
respond_to do |format|
if current_user.city.save
format.html { redirect_to user_ants_path, notice: 'Ant was successfully created.' }
format.json { render json: @ant, status: :created, location: @ant }
else
format.html { render action: "new" }
format.json { render json: @ant.errors, status: :unprocessable_entity }
end
end
end
Je me rends comptes que j'ai encore laisser la "gestion d'erreur" généré par défaut (le if save else...). Je peut donc vérifier l'existence de la variable @ant.errors et affiché en conséquence. Mais comment définir que le champs "quantity" soit obligatoire?
Si en parlant de réellement nécessaire tu parles de ce fameux formulaire qui n'est pas lié a un active record, oui il l'est. En fait, c'est un jeu (étonnant hein?), On choisi combien de fourmis on veut créé, juste la quantité, le reste est géré par rails, la caste est choisi aléatoirement parmis 3 possibles, la vie est par défaut à 100, le nom est unique pour chaque joueur et va de 1 à beaucoup(c'est pourquoi j'utilise une sequence par joueur), et le calcul de la naissance(born) et un peu plus complexe à expliqué.
Je veut vraiment que le joueur ne choisisse que la quantité.
Ah et encore un problème auquel j'ai été confronté. En fait le nextval qui incrémente la sequence SQL n'est exécuté qu'une seule fois.... Je ne comprends pas. Si l'on demande 5fourmis, elles auront toutes le nom "10" par exemple, et si on en redemande 5 elles auront "11", etc...
Et merci Sephi ,)
Diantre, ce code est très perfectible ! Quelques pistes :
- Tu devrais commencer par choisir une langue pour nommer des variables. Bien sûr, c'est l'anglais qu'il faut choisir pour tirer au mieux profit du principe de convention over configuration.
- Ensuite, tu devrais plutôt utiliser les underscore comme séparateur (@last_ant plutôt que @lastAnt). C'est la norme en Ruby. La cohérence est l'alliée de la lisibilité.
Concernant le code en lui même, tu as une faille de logique majeure dans ton premier test. En effet, le else ne sera jamais exécuté puisque Ruby évalue toute expression comme true à l'exception de nil et false. Ainsi, tout nombre — que ce soit -1, 0 ou 1 — vaut true dans un test logique, idem pour une chaîne vide, un tableau vide, etc.
Pour ce genre de tests, utilise plutôt l'une des notations suivantes, ça rend le code plus lisible :
city.ants.empty? # Retourne true si size == 0
city.ants.any? # Retourne true si size > 0
city.ants.many? # Retourne true si size > 1
Ensuite, ta requête pour récupérer la dernière fourmi n'est pas efficace, tu peux la simplifier.
@lastAnt = current_user.city.ants.find_all().last # Pas bien !
@last_ant = current_user.city.ants.last # Bien !
Ensuite, n'utilise pas les dates comme ça. Sers-toi de Ruby et de Rails ! Ici, ton 28800 n'évoque rien. Utilise plutôt 8.hours
# Pas bien !
@decalage = @lastAnt.born - Time.now - 28800
if @decalage < 0
@decalage = 0
end
# Bien !
@time_shift = @last_ant.born - 8.hours.from_now
@time_shift = 0 if @time_shift < 0
Bon, maintenant que tout ça est dit : je pense que ton code est mauvais. Voici mes arguments :
- Cette action ne devrait pas rencontrer d'erreur (si je me trompe — car c'est possible :p — explique moi les erreurs potentielles). À ce titre, je n'utiliserais pas les validateurs mais plutôt la politique du Fail-fast, qui consiste à lancer des exceptions dans les cas anormaux. N'utilise la validation que quand un utilisateur de bonne foi peut être amené à corriger les informations saisies.
- Tu ne distribues pas assez les responsabilités. Ton contrôleur prend en charge des choses dont il ne devrait pas avoir conscience. Ton code est difficile à tester et on ne comprend pas ce qu'il fait d'un coup d'œil : définis des méthodes dans tes modèles.
- Enfin, cette action a-t-elle vraiment besoin de gérer les formats JSON et HTML ?
Voici la version que je te propose. N'hésite pas à me dire si elle ne colle pas à tes besoins :
def create
@quantity = params[:quantity].to_i - 1
current_user.city.product_ants!(@quantity)
format.html { redirect_to(user_ants_path, notice: 'Ant was successfully created.') }
format.json { render(json: true)
end
class Ant < ActiveRecord::Base
PRODUCTION_DURATION = 6.seconds
# Quelque chose de plus cross-database ne serait-il pas plus intéressant ?
def self.next_name_for(user)
sequence = "fourmis_#{user.username}_id_seq"
query = "SELECT nextval('#{sequence}')"
ActiveRecord::Base.connection.select_value(query)
end
end
class City < ActiveRecord::Base
belongs_to :user
has_many :ants
# Add few ants to the production queue.
# Raise if something goes wrong.
def product_ants!(count)
# ...
end
end
Je te laisse implémenter product_ants! car je n'ai pas bien compris tes histoires de 6 secondes et de 8 heures. Bien sûr, si tu optes — même partiellement — pour ma version du code, j'aimerais que tu me montres l'implémentation de cette méthode.