2009/9/15 Nicolás Sanguinetti <[email protected]>:
> 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.
Eh, por cierto, FAIL mio, tendría que ser algo como esto:
module Packet
BINARY_PACKET_TYPE = 224
ASCII_PACKET_TYPE = 36
def self.read(message)
case message
when BINARY_PACKET_TYPE; Packet::Binary.new(message)
when ASCII_PACKET_TYPE; Packet::Text.new(message)
else raise ArgumentError, "Wrong packet type: #{message}"
end
class Binary
end
class Text
end
end
O bueno, los nombres capaz que no son los mejores (de nuevo, ni idea
que es OpenDMTP :)), pero no esta bueno tirar numeros "al azar" en el
medio del codigo :)
-f
> 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
