Skip to content

[IV-24-25] Objetivo 4 #23

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 53 commits into
base: main
Choose a base branch
from
Open

[IV-24-25] Objetivo 4 #23

wants to merge 53 commits into from

Conversation

abbonno
Copy link
Owner

@abbonno abbonno commented Jan 13, 2025

  • ¿Hay un camino claro que permita ir desde el código hasta la historia de
    usuario que desarrolla, pasando por el mensaje de commit y el issue
    correspondiente?
  • ¿Se ha intentado cubrir con tests una parte de la lógica de negocio que se
    esté desarrollando?
  • ¿Hay un producto mínimamente viable que describa lo que se va a entregar en
    este objetivo?
  • ¿En este producto mínimamente viable se han priorizado unas clases,
    módulos o paquetes más fundamentales frente a otros, movidos a otro
    PMV/Hito?
  • ¿Estoy leyendo todo lo que marco o simplemente marco lo que se me pone por
    delante?
    Sí, estoy leyendo
  • ¿Se han documentado las elecciones de biblioteca de aserciones y del
    de test runner?
  • ¿Te has asegurado que lo que mencionas como biblioteca de aserciones y
    test runners lo son realmente y tienen la misma funcionalidad? ¿O estás poniendo
    unittest como test runner y pytest como biblioteca de aserciones?
  • ¿Esa documentación incluye criterios de aceptación y también criterios de
    búsqueda para las diferentes opciones?
  • ¿Se han seguido los principios F.I.R.S.T. en la creación del test y
    se ha documentado cómo se ha hecho?
    Fast: son tests rápidos (unos 2 milisegundos)
    Independent: ninguno depende de otra función testeada
    Repeatable: los datos usados son invariables, da el mismo resultado cada vez
    Self-validating: aserciones claras que muestran cuando falla, en caso contrario el test se pasa correctamente
    Timely: los tests son oportunos ya que estamos siguiendo TDD y es a partir de ellos que generamos la lógica de negocio correcta.
  • ¿Se describe claramente en el PMV los tests que hay que pasar para que se
    considere viable?
  • ¿Se han testeado también las excepciones?

@juanbarearojo
Copy link

Buenas he revisado tu PR y he dejado un par de comentarios para que puedas mejorarlo. Mucho ánimo

Copy link

@hossam1522 hossam1522 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aparte de lo comentado, decirte que tengas cuidado ya que tienes muy pocos commits para la cantidad de código que has introducido.

Se eliminan números mágicos, comentarios innecesarios, constructor y otras funciones innecesarias
Deshacemos cambios en el código para reestructurarlos y que se muestren adecuadamente en los issues
Este test comprueba que no haya más de un enfermero asignado al mismo área al mismo tiempo en el plan generado
Comprueba que no falte nadie en un área en cualquier momento
Comprueba que la información sobre sus turnos se muestra correctamente
Crea el planning de turnos para todo un año dados los enfermeros contratados
Convierte los turnos asignados a un enfermero en string y lo devuelve
@abbonno
Copy link
Owner Author

abbonno commented Jan 13, 2025

Aparte de lo comentado, decirte que tengas cuidado ya que tienes muy pocos commits para la cantidad de código que has introducido.

He intentado volver a dar pasos sobre el código para documentar los cambios en los cambios realizados. Adicionalmente, he creado sus issues correspondientes, espero que sean suficientes. Si tienes alguna recomendación es bienvenida. Muchas gracias!

@abbonno
Copy link
Owner Author

abbonno commented Jan 13, 2025

Buenas he revisado tu PR y he dejado un par de comentarios para que puedas mejorarlo. Mucho ánimo

Gracias por tu colaboración Juan. He reorganizado un poco la estructura de los tests y cambiado los issues. Espero tú revisión,

Copy link

@wickeet wickeet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Solucionando lo que han dicho los compañeros y esta puntualización yo lo veo bastante bien

fechaActual := time.Now()

for i := 0; i < diasAnio; i++ {
fecha := fechaActual.AddDate(0, 0, i)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

¿Esto no crea todas las fechas con la hora en el momento que se crea el Plan Anual? No sé yo como de correcto es que alguien del turno de noche tenga que su fecha es el 05 de febrero a las 15:37. A no ser que el atributo hora del tipo de dato sea irrelevante, en ese caso igual sería mejor usar otro tipo de dato, ¿no crees? Algo como un struct con un dia, un mes y un año (puedes usar time.Month y time.Year) y crear tu propio AddDate y no tendrías que cambiar apenas el código que ya tienes hecho.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Y aquí sí que voy a parar. Tenéis que hacer modificaciones como respuesta a todos los comentarios. Cerrar un comentario sin ningún tipo de comentario ni modificación es una falta de respeto a la persona que lo ha hecho y ha invertido su tiempo en ello.

Copy link

@evaanngiil evaanngiil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Buenas, se me ha asignado revisar tu PR. Te he puesto algunos comentarios con ánimo de que mejores y entiendas los conceptos claves del objetivo. Ánimo.

Copy link

@juanmaaf juanmaaf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Buenas. Además de los comentarios realizados por otros compañeros, que veo que no se han resuelto, te dejo unas puntualizaciones para reflexionar y considerar en el código.

Un año puede durar 365 o 366 días por lo que es incorrecto llamar a la constante de esa manera
@abbonno abbonno requested a review from lmchaves January 22, 2025 11:18
Copy link

@lmchaves lmchaves left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahora entiendo mejor la elección pero, conforme a lo que ha dicho o JJ en telegram, al resolver los comentarios, hay que hacer los cambios en la líena comentada, para en este caso la elección de la herramienta quede claro

@abbonno
Copy link
Owner Author

abbonno commented Jan 22, 2025

Ahora entiendo mejor la elección pero, conforme a lo que ha dicho o JJ en telegram, al resolver los comentarios, hay que hacer los cambios en la líena comentada, para en este caso la elección de la herramienta quede claro

Tienes razón, lo acabo de modificar.

@abbonno abbonno requested a review from lmchaves January 22, 2025 13:14
@abbonno
Copy link
Owner Author

abbonno commented Jan 22, 2025

@JJ

Copy link

@JJ JJ left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

El que todo lo que se pide en la asignatura sea útil para desarrollar y aportar valor al cliente puedes creértelo o no, eso ya es cosa de cada uno. Si no te lo crees puedes superar un objetivo, dos objetivos o tres, quizás con un poco de suerte o con algún despiste mío. No vas a llegar mucho más allá, sin embargo. Porque según vas avanzando en los objetivos, todo lo que ves son "tonterías que hay que hacer para escribir código que es lo que de verdad importa".
Y no, no importa el código. El código lo pude escribir una IA. Lo que no puede hacer una IA es plantearse problemas, sopesar diferentes opciones para resolverlos, y finalmente generare el código que las resuelva de una forma mínima y satisfactoria. Eso no lo puede hacer.
Os rogaría que tratárais de aprender lo que no es capaz de hacer el código, porque eso es lo que os va a dar trabajo. No crear un issue que diga "hay que crear una fecha" y decirle a ChatGPT "hazme una fecha" y generar un total de unas 150 líneas de código que tengo yo que revisar y que, cualquier cosa que haga para revisarlas, no te va a aportar ningún valor porque seguirán siendo "las tonterías que hay que hacer para escribir código"
Hay dos formas para solucionar esto. Cambias esta línea o la otra, y nos metemos en un bucle donde vamos viendo en 10 o 12 iteraciones una forma de reconducir esto por un camino en el que se pueda pasar el objetivo. Es posible que para esas fechas sea mayo de 2026, pero se llegará.
Hay otra forma. Cambiar de actitud. Tratar de entender lo que se pide y por qué. Y ver que realmente, por ese camino, es más fácil avanzar y llegar a la meta antes del 7 de febrero. Puedes hacer lo que quieras, es decisión tuya, y cualquier decisión será legítima. Cuando lo hagas, pídeme revisión de nuevo, por favor.

La modificación del modelo ha sido un error y no debería haber abordado el tema de la fecha de esa manera, no aporta valor a la lógica del milestone 1 ni forma parte del pmv
Nos deshacemos de ella porque no forma parte del pmv
Para obtener el plan deberemos generarlo a través de los datos que se nos ofrezca por lo que se creará la función en base a los tests siguiendo TDD
Este test comprueba si hay áreas sin ocupar y las notifica, en lo que será un error
Este test comprueba si hay más de una persona asignada a un área al mismo tiempo, que indica sobreocupación, y lo notifica
Aplicamos el principio DRY para evitar repetición de código creando el dato Ocupación que guarda el número de enfermeros en cada área en cada momento
Tras completar los tests a tener en cuenta, completamos la función de creación del plan siguiendo el TDD
Se establecen las bases para considerar correcta la solución a aplicar para que los enfermeros comprueben sus turnos
Se crea la función que muestra al enfermero el área al que debe acudir
Al incluir una hora indeterminada usando time.Now() las igualdades entre fechas eran erróneas, se opta por crear una función que normalice las fechas dejando la hora en 0
@abbonno
Copy link
Owner Author

abbonno commented Jan 27, 2025

@JJ revisión.
He modificado los issues para tratarlos desde el punto de vista de un problema a descubrir cómo resolver antes que una tarea con obligada solución. En la construcción del código he tratado de seguir la metodología de Test Driven Design para crear el código a partir de lo que se debe cumplir en los tests y no al contrario. He intentado aplicar al máximo el principio DRY. Los tests tratan de ser todo lo unitarios que pueden y, a pesar de que haya bucles anidados, estos son imprescindibles ya que tratamos de recorrer maps de tres dimensiones. Además, el código trata de hacer sola y únicamente aquello para lo que ha sido ideado.

@JJ
Copy link

JJ commented Jan 31, 2025

@JJ revisión. He modificado los issues para tratarlos desde el punto de vista de un problema a descubrir cómo resolver antes que una tarea con obligada solución. En la construcción del código he tratado de seguir la metodología de Test Driven Design para crear el código a partir de lo que se debe cumplir en los tests y no al contrario. He intentado aplicar al máximo el principio DRY. Los tests tratan de ser todo lo unitarios que pueden y, a pesar de que haya bucles anidados, estos son imprescindibles ya que tratamos de recorrer maps de tres dimensiones. Además, el código trata de hacer sola y únicamente aquello para lo que ha sido ideado.

En informática siempre hay varias maneras de hacer las cosas. Si dices que algo es "imprescindible" es que simplemente no has explorado otras maneras.

Copy link

@JJ JJ left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

De veras que lo siento, pero no voy a seguir revisando en esta vuelta después de los comentarios que te he hecho en diferentes issues. No parece que estés llevando a cabo la metodología que se enuncia en el guión.

  • Estás creando issues que no representan problemas, y esto después de lo comentado. Si todo lo que entendéis cuando hago una revisión detallada en la que invierto una buena cantidad de tiempo es "voy a hacer otra vez lo mismo para que tenga que volver a invertir la misma cantidad de tiempo", no sé si merece la pena hacerlo. Después de decirte que no había que crear issues que dijeran "hay que crear una fecha" sigue habiendo uno que dice "como crear un plan" que fue creado antes de eso, pero sobre el cual sigue habiendo código presente, código que sigue siendo inevaluable.
  • Lo que has hecho es cerrar ese issue como completado. No está completado, simplemente es incorrecto. Pero si es incorrecto, todo el código que se ha creado lo es, y la forma correcta de hacerlo es borrar ese código, indicando en el issue donde se hace "closes #xxx", y comenzar de nuevo, con issues nuevos Si no al final tenemos una serie de parches que sólo hacen todo más difícil de evaluar.
    Voy a seguir corrigiendo en todo caso por si pudiera añadirte algún valor, pero si no se sigue una metodología correcta desde el principio es imposible que se haga así.

Copy link

@JJ JJ left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Por favor, hasta que no resuelvas todos los comentarios con cambios en la línea indicada, cambios que además resuelvan un problema especificado claramente en un issue, no solicites una nueva revisión.

fechaActual := time.Now()

for i := 0; i < diasAnio; i++ {
fecha := fechaActual.AddDate(0, 0, i)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Y aquí sí que voy a parar. Tenéis que hacer modificaciones como respuesta a todos los comentarios. Cerrar un comentario sin ningún tipo de comentario ni modificación es una falta de respeto a la persona que lo ha hecho y ha invertido su tiempo en ello.

…issue #39

Para determinar como correcta la solución al issue, un empleado debe poder comprobar el área al que asistir dados su nombre y la fecha y el turno en el que asiste
No hay necesidad en un principio de cambiar la fecha, por lo que se devuelve a su estado original y se cambia el nombre por uno más entendible
@abbonno
Copy link
Owner Author

abbonno commented Feb 2, 2025

@JJ revisión.

El issue no define un problema por lo que el código creado para el mismo está mal
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants