# Días promedio en el cálculo del mercado -- ruby campo con ruby-on-rails camp codereview Relacionados El problema

## Average days on market calculation

2

### problema

Español

Tengo el siguiente método que calcula los días promedio en el mercado para un segmento de tipo de venta:

` ` def average_days_on_market_sale_type_segment     # on market days     on_market_days = {}      # 1. get last year sales - note that the dates need to be sequential in time (oldest in time comes first, most recent in time commes last)     last_year_sales = self.real_property_sale_projects.where(status: 'reviewed')       .where('deal_date between ? and ?', Time.at(Time.now - 1.years), Time.now)       .map(&:real_property_sale)      # 2. loop each sale type, convert sale description string to symbol     last_year_sales.each do |sale|       sd = sale.sale_description.parameterize.underscore.to_sym        on_market_days[sd] ||= { count: 0, days: 0 }        on_market_days[sd][:count] += 1        # get the days on market value       on_market_days[sd][:days] += sale.real_property_sale_project.days_on_market     end      # 3. summarize average     summary = {}      on_market_days.each do |k, v|       summary[k.to_s.humanize] = (v[:days].to_f / v[:count].to_f).round     end      # 4. return summarized result     # it should contain each sale description type as a key and average day as a number     summary   end  ` `

¿Hay una manera más elegante de lograr lo mismo? ¿La solución contiene algún problema de rendimiento / gotchas?

Original en ingles

I have the following method which calculates average days on market for a sale type segment:

``def average_days_on_market_sale_type_segment     # on market days     on_market_days = {}      # 1. get last year sales - note that the dates need to be sequential in time (oldest in time comes first, most recent in time commes last)     last_year_sales = self.real_property_sale_projects.where(status: 'reviewed')       .where('deal_date between ? and ?', Time.at(Time.now - 1.years), Time.now)       .map(&:real_property_sale)      # 2. loop each sale type, convert sale description string to symbol     last_year_sales.each do |sale|       sd = sale.sale_description.parameterize.underscore.to_sym        on_market_days[sd] ||= { count: 0, days: 0 }        on_market_days[sd][:count] += 1        # get the days on market value       on_market_days[sd][:days] += sale.real_property_sale_project.days_on_market     end      # 3. summarize average     summary = {}      on_market_days.each do |k, v|       summary[k.to_s.humanize] = (v[:days].to_f / v[:count].to_f).round     end      # 4. return summarized result     # it should contain each sale description type as a key and average day as a number     summary   end ``

Is there a more elegant way to achieve the same thing? Does the solution contain any performance issues/gotchas?

## Lista de respuestas

3

La mejor respuesta

1. Uso ` getIsomorphism3 Para obtener el inicio del rango de tiempo. `

2. Crear un método en el modelo ` getIsomorphism6655443374 getIsomorphism5 + getIsomorphism6 para "normalizar" cadenas ligeramente diferentes que significan lo mismo, probablemente debería normalizar su base de datos con una migración en su lugar. Si las cadenas ya están normalizadas, solo use "em directamente. `

3. Uso ` getIsomorphism7 ` a grupo por tipo de venta.

4. Uso ` getIsomorphism8 Si desea evitar las matemáticas enteras. `

El registro: recuperar un poco de complejo, pero llegaremos a eso. Mirando solo en el cálculo promedio, puede hacerlo en su lugar:

` ` getIsomorphism9  ``
Sin embargo,

En general, su base de datos parece estar diseñada de una manera que hace que esta consulta sea difícil.

Estás buscando a todos los asociados ` Comparator<DirectedGraphNode> comparator = Comparator.comparing(n -> n.children().size()) .thenComparing(n -> n.parents().size()); 0 ` (Simplemente diré "proyectos"), y luego, para cada uno de esos, ¡obtendrás un ` 99887766555443381 `? Pero cuando tiene que hacer el promediario, el ` 99887766555443382 no contiene toda la información necesaria: el número 998877766554443383 se adjunta al proyecto. `

y estás haciendo todo esto en un modelo de tercero en total, parece. Así que eso es bastante complejo.

Sin embargo, también puede hacerlo todo en la base de datos, es extremadamente bueno en el agregado de datos. Por ejemplo, esto, creo, debería funcionar:

` ` Comparator<DirectedGraphNode> comparator = Comparator.comparing(n -> n.children().size())       .thenComparing(n -> n.parents().size()); 4 ` `

Eso debería darle el resultado. Puede haber una mejor manera de escribirlo.

1. Use `1.year.ago` to get the start of the time range.

2. Create a method on the `sale` model that returns the description in the form you want (i.e. underscored symbol). On the other hand, is it necessary? You turn it back into a string afterward. If you're relying on `parameterize` + `underscore` to "normalize" slightly different strings that mean the same, you should probably normalize your database with a migration instead. If the strings are already normalized, just use 'em directly.

3. Use `#group_by` to group by sales type.

4. Use `#fdiv` if you want to avoid integer math.

The record-fetching a bit complex, but we'll get to that. Looking just at the average calculation, you can do this instead:

``averages = last_year_sales   .group_by(&:description) # I'm assuming that there's no real need to symbolize the description   .map do |description, sales|     sum = sales.map { |sale| sale.real_property_sale_project.days_on_market }.reduce(:+)     [description, sum.fdiv(sales.count)]   end  Hash[averages] ``

Overall though, your database seems to be laid out in a way that makes this query extra tricky.

You're fetching all associated `real_property_sale_projects` (I'll just say "projects"), and then - for each of those - you fetch an associated `real_property_sale`? But when you have to do the averaging, the `sale` doesn't contain all the necessary information - the `days_on_market` number is attached to the project.

And you're doing all this in a 3rd model altogether it seems. So that's pretty complex.

However, you can also do it all in the database - it's exceedingly good at aggregating data. For instance, this, I think, should work:

``self.real_property_sale_projects   .joins(:real_property_sale)   .where(status: "reviewed")   .where("real_property_sale_projects.deal_date between ? and ?", 1.year.ago, Time.now)   .group("real_property_sales.description")   .average(:days_on_market) ``

That should give you the result. There may be a nicer way to write it.

3

Notas principales:

1. Cada vez que tenga un método dividido en secciones comentadas, considere extraer cada sección en su propio método con nombre claramente llamado. Esto hará que las piezas de su código se enfocaran y legibles, y eliminarán la necesidad de comentarios.

2. Utilice los métodos funcionales que se construyen en Ruby en lugar de las cosas de bucle procesal y las características que se colocan manualmente. ` Comparator<DirectedGraphNode> comparator = Comparator.comparing(n -> n.children().size()) .thenComparing(n -> n.parents().size()); 5 hace la mayor parte del trabajo para usted en este caso. `

3. Puede mapear sobre los hashes, devolviendo una matriz de imágenes Comparator<DirectedGraphNode> comparator = Comparator.comparing(n -> n.children().size()) .thenComparing(n -> n.parents().size()); 6 , y luego llame ` Comparator<DirectedGraphNode> comparator = Comparator.comparing(n -> n.children().size()) .thenComparing(n -> n.parents().size()); 7 al final para recuperar un hash transformado. `

4. ` Comparator<DirectedGraphNode> comparator = Comparator.comparing(n -> n.children().size()) .thenComparing(n -> n.parents().size()); 8 ` es un nombre difícil de manejar. ` Comparator<DirectedGraphNode> comparator = Comparator.comparing(n -> n.children().size()) .thenComparing(n -> n.parents().size()); 9 Parece una pequeña mejora que podría hacer (generalmente nombrar los hashes como inDegree0 es más clara), pero sigue siendo largo. Si tiene experiencia en bienes raíces, es posible que pueda encontrar algo más claro que refleje el lenguaje utilizado por los agentes. Una búsqueda rápida de Google se le ocurrió con "Cdom", así que tal vez inDegree1 funcionaría, pero con nombramiento, no hay respuesta "correcta", solo lo que será claro a las personas que trabajan con el sistema. < / p> `

un par de otras cosas:

1. Esto no se prueba, ya que no tengo sus datos para probar. Así que puede haber algunos errores triviales.

2. Los métodos ` inDegree2 ` inDegree33 que he introducido No son realmente Métodos de instancia, ya que son funciones puras que operan en un Venta o variedad de ventas. Esto sugerirá más oportunidades para refactorizar mediante la introducción de objetos para ` inDegree4 99887766555443395 `, pero eso puede estar exagerado en este punto.

` ` inDegree6 ` `

Main notes:

1. Whenever you have a method broken up into commented sections, consider pulling out each section into its own, clearly named method. This will make the pieces of your code focused and readable, and eliminate the need for comments.

2. Use ruby's built in functional methods rather than procedurally looping and bulding things manually. `group_by` does most of the work for you in this case.

3. You can map over hashes, returning an array of `[key, value]` arrays, and then call `to_h` at the end to get back a transformed hash.

4. `average_days_on_market_sale_type_segment` is an unwieldy name. `avg_days_on_market_by_type` seems like a small improvement you could make (generally naming hashes as `value_by_key` is clearer), but it's still long. If you have expertise in real estate, you might be able to come up with something clearer that reflects language used by agents. A quick google search came up with "CDOM", so maybe `cdom_by_type` would work, but with naming, there's no "correct" answer -- only what will be clear to the people working with the system.

A couple other things:

1. This isn't tested, since I don't have your data to test with. So there may be some trivial errors.

2. The methods `sales_description` and `avg_days_on_market` that I introduced aren't really instance methods, since they are pure functions operating on a sale or array of sales. This hints at further opportunities for refactoring by introducing objects for `Sale` or `Sales`, but that may be overkill at this point.

Refactored code:

``def average_days_on_market_sale_type_segment   last_years_sales.group_by {|sale| sales_description(sale)}                   .map {|desc, sales| [desc, avg_days_on_market(sales)]}.to_h end  def last_year_sales   self.real_property_sale_projects   .where(status: 'reviewed')   .where('deal_date between ? and ?', Time.at(Time.now - 1.years), Time.now)   .map(&:real_property_sale) end  def sales_description(sale)   sale.sale_description.parameterize.underscore.to_sym end  def avg_days_on_market(sales)   total_days = sales.map {|sale| sale.real_property_sale_project.days_on_market}                     .inject(:+)   total_days / sales.size end ``

3  Semilla una base de datos  ( Seed a database )
Estoy buscando consejos / mejores prácticas para evitar hacer un conjunto profundo anidado de IF / 99887766555443333 en Rails. La intención es sembrar una...

1  Rake Tarea para enviar a los usuarios un recordatorio para publicar con condiciones  ( Rake task to send users a reminder to post with conditions )
Estoy implementando una característica que recuerda a los usuarios que realicen una publicación por correo electrónico si El usuario ha establecido recorda...

3  Prueba de un controlador API de rieles  ( Testing a rails api controller )
Estoy construyendo una API de JSON. Estoy escribiendo mis pruebas de controlador para que comparen el organismo de respuesta al JSON real, estoy generando en ...

1  Importando archivos de marcas  ( Importing markdown files )
Estoy construyendo una aplicación de rieles que, entre otras cosas, importan los archivos de marca de texto como publicaciones de blogs. La idea es que la ver...

4  Convertidor de temperatura que parece violar SRP  ( Temperature converter that seems to violate srp )
He escrito esta clase para dos propósitos (creo que solo este hecho muestra que este código viola srp < / a>): convertir el peso de una unidad a otra y para ...

1  Sistema de eventos de Rails JS  ( Rails js event system )
Estoy trabajando en una gema que es relativamente simple. funciona agregando data-controller data-action55544335 Atribuye a la etiqueta del cuerpo y lu...

2  PEQUEÑO: configuración regional  ( Small setting locale )
Soy nuevo en rieles / Ruby y me gustaría obtener comentarios sobre mi pequeño 99887766655443312 que puede detectar el idioma del visitante. Estoy especialme...

2  Prueba de unidad RSPEC para actualizar una propiedad de un modelo  ( Rspec unit test for updating a property of a model )
Sé cómo escribir pruebas de RSPEC simples y pruebas de unidad. Solo quiero saber si esto es lo suficientemente bueno o si hay mejoras que pueda hacer. Tengo...

2  Estructura de ruta para múltiples asociaciones en rieles  ( Route structure for multiple associations in rails )
Estoy haciendo un sitio en este momento (primer sitio de Rails, aprendiendo como código) y me preocupa que estoy complicando las rutas. Mis asociaciones se ...

1  Cómo reducir el número de "Si las declaraciones" jerárquicas "[CERRADO]  ( How to reduce number of hierarchical if statements )
cerrado . Esta pregunta necesita detalles o claridad . Actualmente no está aceptando respuestas. ...