2009/9/14 Sebastian A. Espindola <[email protected]>:
> Gente,
> Estaba tirando código para un proyecto personal (una implementación
> en Ruby de
> un server del protocolo OpenDMTP) y me encontre con la necesidad de
> que una clase
> interprete si los datos que recibe desde un socket estan en un
> formato binario o texto
> y genere una instancia que actue sobre esa codificación.
Hola, me llamo Nicolás y no entiendo nada de diplomacia. Tu solucion
es horrible.
Sin saber bien lo que estas haciendo (lease, no se que es OpenDMTP) lo
que yo haría (y después, abajo, comento tu código parte por parte.
module Packet
def self.read(message)
case message
when 224; Packet::Binary.new(message)
when 36; Packet::Text.new(message)
else raise ArgumentError, "Wrong packet type: #{message}"
end
class Binary
end
class Text
end
end
Packet.read(...)
Fijate como no tenés que andar haciendo aliases ni mapeando strings a
clases, y como me ahorro varios niveles de indirección.
> Leyendo varios articulos encontrados en google respecto a la
> implementación del patron
> factory en ruby y jugando un poco con metaprogramación, me quedó el
> siguiente codigo:
>
> class Packet
>
> class << self
> alias :nuevo :new
>
> def inherited(subclass)
> class << subclass
> alias :new :nuevo
> end
> end
Sobre esto:
1) (esta es a título personal) por favor, POR FAVOR, no escriban
nombres de métodos en español. Queda horrible leer código con pedazos
en otros idiomas.
Imaginate si mañana buscás una librería que soluciona un problema X, y
empezás a usarla, y cuando te encontrás con un error y querés revisar
el código, los comentarios y la mitad de los métodos están escritos en
ruso? :-/
(ojo, digo ruso como podría decir cualquier idioma, si justo sabés
ruso cambialo por otro :P)
2) En mi opinion personal, no esta bueno redefinir Class#new (en este
caso particular Packet.new). Prefiero mil veces agregar otro metodo de
clase que llame new. Pero ta, ponele que queres igual usar new, porque
te gusto, esto alcanza:
class Packet
class << self
alias_method :new_without_factory, :new
end
def self.new(*args)
# haces cosas
new_without_factory(*args)
end
end
No tenes que usar self.inherited ni dar esas vueltas que te hacen leer
como 3 veces el código para saber que querés lograr.
> def new(*args)
> msg = *args
> encoding = get_packet_encoding(msg)
> Kernel.const_get(encoding).new unless encoding == "Unknown"
> end
Tiene sentido que Packet.new reciba más de un argumento? Porque
después cuando llamas BinaryPacket.new, o TextPacket.new, no les estás
pasando los args.
No se si te falto un new(*args) o simplemente querés usar el primer
argumento para deducir el tipo de clase a usar. Si es lo segundo,
entonces usar splat-args no tiene sentido. Definí simplemente 'msg'
como parametro.
> def get_packet_encoding(msg)
> encoding = case msg[0]
> when 224 then "BinaryPacket"
> when 36 then "TextPacket"
> else "Unknown"
> end
> end
> end
Aca viene la parte que creo es más fea. Para qué querés devolver
strings y hacer const_get de esos strings, cuando podés devolver
directamente la clase que querés? Estás agregandole niveles de
indirección totalmente innecesarios.
Y por último, "Unknown". Si yo hago Packet.new(42) con tu codigo,
devuelve nil. Eso es feo. Tener que andar chequeando por nil es Feo
(con f mayúscula).
Vos querés que *no funcione*, entonces lo ideal es que el usuario
reciba una notificación de que lo que hace, *no funciona*. Ergo, una
excepción me parece una mejor idea. Cuál? Depende, si estás haciendo
una libreria capaz que definir tu propia excepción tipo class
WrongPacketTypeProvided < ArgumentError; end tiene sentido. Si no, con
tirar un ArgumentError está bien (en mi opinión, es debatible :))
> def initialize
> puts "hola desde #{self.class}"
> end
> end
>
> class TextPacket < Packet
> end
>
> class BinaryPacket < Packet
> end
Y ta, después, lo de hacer Packet::Text en lugar de TextPacket <
Packet es puramente un tema de estilo personal, dado lo fácil que es
en ruby de pisar cosas sin darte cuenta, prefiero poner todo en
namespaces.
Si TextPacket y BinaryPacket heredaban de Packet por algún tema de
funcionalidad compartida, es fácil de cambiar. O directamente, definí
la funcionalidad compartida en el módulo Packet, y hace include Packet
adentro de las child classes.
> No conocia la posibilidad de definir alias de metodos, la
> posibilidad de hacer modificaciones
> temporales en runtime "backupeando" el metodo con un alias es alucinante.
>
> Si uno le muestra esto a un programador que no conozca ruby, cuando
> entienda como
> funciona se le vuela el peluquín. :)
>
> Calculo que en un par de meses voy a tener una implementacion
> cliente-servidor de
> OpenDMTP decente basada en EventMachine, cuando la termine la voy a liberar.
>
> En fin, queria compartir mi "descubrimiento" con ustedes.
Y ta, por último, no te lo tomes a mal, si el lenguaje te parece
violento o algo, creeme que no es intencional, cuando digo que no
entiendo nada de diplomacia, es cierto, simplemente te digo lo que me
parece haría a tu código mejor :)
Y ta, obviamente, todo el mundo está invitado a demostrarme como
mejorar mi ejeplo :D
<chivo> Los code reviews son divertidos, tu equipo puede mejorar su
calidad de código usando http://howsmycode.com :P </chivo>
-foca
_______________________________________________
Ruby mailing list
[email protected]
http://lista.rubyargentina.com.ar/listinfo.cgi/ruby-rubyargentina.com.ar
