Feedback #1
Feedback #1
Conversation
![github-classroom[bot]](https://avatars.githubusercontent.com/in/68187?s=60&v=4){kind=small}
Co-authored-by: Gonzalo Garcia Fontenla <[email protected]> Co-authored-by: SofiaBaudoUTN <[email protected]>
Co-authored-by: Gonzalo Garcia Fontenla <[email protected]> Co-authored-by: SofiaBaudoUTN <[email protected]>
There was a problem hiding this comment.
¡Hola @pdepjm/la-logia!
Les dejamos algunos comentarios sobre el código que tienen hasta ahora, van a poder charlalo con Ceci en más detalle si lo necesitan. Cualquier cosa pueden consultar por acá también.
Por otro lado, les consulto ¿tienen algun diagrama de clases hecho?
¡Saludos!
elementos.wlk
Outdated
| var property imagen = "Designer.jpeg" | ||
|
|
||
| method image() = imagen |
There was a problem hiding this comment.
¿Es necesario tener la imagen como variable si nunca va a cambiar dinámicamente en el objeto? Podríamos directamente tener:
method image() = "Designer.jpeg"Recordemos que vamos a querer guardanos cosas en variables justamente si necesitamos que cambien dinámicamente (es decir durante ejecución), si siempre es la misma, podemos hacerla constante o más fácil incluso retornar el valor directamente en el método de consulta.
Revisen esto en el resto de los objetos.
elementos.wlk
Outdated
| method moverse() { | ||
| const x = position.x() + 1 | ||
| const y = position.y() | ||
|
|
||
| if (x < game.width() + 1) { | ||
| position = game.at(x, y) | ||
| } else { | ||
| position = game.at(0, y) | ||
| } | ||
| } |
There was a problem hiding this comment.
Entiendo que esto es para que los vehiculos no queden fuera de los límites del mapa, bien 👍. Pero, tengan cuidado que se está repitiendo en todos los vehículos la misma lógica, el jueves vamos a ver un concepto llamado Herencia que les va a servir para resolver esto, revisenlo luego de esa clase (quizás necesitamos una clase abstracta llamada Vehiculo o algo de este estilo).
Un detallito mínimo, relacionado con el tema de declaratividad, es que quizás podrían delegar a un método aparte la idea de "está dentro de los límites" (x < game.width() + 1), es super mínimo pero teniendo if(self.estaDentroDeLimites(x)) hace que se entienda un poco mejor lo que hace el método moverse.
escenarios.wlk
Outdated
| class Nivel { | ||
| method configuracionInicial() { | ||
| } | ||
|
|
||
| method configuracionTeclado() { | ||
| } | ||
|
|
||
| method configuracionFondo() { | ||
| } | ||
|
|
||
| method instanciarObjetos() { | ||
| } | ||
|
|
||
| method configuracionVisual() { | ||
| } | ||
|
|
||
| method configuracionVisualExtra() { | ||
| } | ||
|
|
||
| method instanciarObjetosExtra() { | ||
| } | ||
|
|
||
| method configuracionVehiculos() { | ||
| } | ||
|
|
||
| method finalizarNivel(){} | ||
|
|
||
| } | ||
|
|
There was a problem hiding this comment.
Hay un concepto llamado clase abstracta que vamos a ver la clase del jueves (lean esto luego de esa clase para entenderlo mejor), esto que están haciendo acá es una clase abstracta, ya que la están usando para obligar a los niveles a tener estos métodos pero nunca se instanciará esta clase, sirve de alguna forma para tipar.
Con esto en mente, ojo que no es lo mismo tener un método que no hace nada como el siguiente:
method finalizarNivel(){}A un método abstracto, que se define sin las llaves y lo creamos para decir que las clases hijas tienen que definirlo:
method finalizarNivel()Aprovechen a poner acá configuraciones generales que sean iguales para todos los niveles (si es que las hay) , que no sea una clase sola de métodos para definir en las subclases.
escenarios.wlk
Outdated
| } | ||
| } | ||
|
|
||
| class Nivel1 { |
There was a problem hiding this comment.
¿No es un nivel? ¿Por qué no hereda de la clase Nivel?
escenarios.wlk
Outdated
| game.onTick(3000, "Oleada autos", {=> carTraffic.generarAutos(autos1,2, -4, 2)}) | ||
| game.onTick(1000, "Oleada primer auto", {=> carTraffic.generarAutos(autos2, 2, -4, 2)}) | ||
| // game.onTick(6000, "Oleada autos tres", {=> carTraffic.generarAutos(autos3, 2, -4, 2)}) | ||
|
|
||
| game.onTick(4500, "Oleada autos", {=> carTraffic.generarAutos(autos1,2, -4, 4)}) | ||
| game.onTick(1900, "Oleada primer auto", {=> carTraffic.generarAutos(autos2, 2, -4, 4)}) | ||
| // game.onTick(7000, "Oleada autos tres", {=> carTraffic.generarAutos(autos3, 2, -4, 4)}) |
There was a problem hiding this comment.
Ojo con que estos eventos se llamen igual si representan diferentes cosas, llamarlos igual nos quita control sobre cada evento específico (no tenemos forma de identificarlos por separado).
Por otro lado, por lo que entiendo los objetos vehiculos una vez que desaparecen de la pantalla se reposicionan al principio nuevamente, por lo que siempre serían los mismos objetos que estarían en pantalla, constantemente reposicionandose. Con esto en mente ¿qué solucionan los eventos estos? ¿En qué momento necesitaria instanciar nuevos vehiculos?
estados.wlk
Outdated
| object puntos { | ||
|
|
||
| method position() = game.at(42,23) | ||
|
|
||
| method text() = "Puntos: " | ||
| } | ||
|
|
||
| object vida { | ||
|
|
||
| method position() = game.at(42,24) | ||
|
|
||
| method text() = toby.valorVida().toString() | ||
| } | ||
|
|
||
| object vida2 { | ||
| method position() = game.at(41,24) | ||
| method text() = "Vida: " | ||
| } |
There was a problem hiding this comment.
Estaría bueno (a futuro) para la jugabilidad que el juego me avise visualmente o con un sonido que perdí vida o gane puntos, ahora mismo no se comunica al jugador estas cosas, si bien están los textos no son rápidos de encontrar o ver.
estados.wlk
Outdated
| object vida { | ||
|
|
||
| method position() = game.at(42,24) | ||
|
|
||
| method text() = toby.valorVida().toString() | ||
| } | ||
|
|
||
| object vida2 { | ||
| method position() = game.at(41,24) | ||
| method text() = "Vida: " | ||
| } |
There was a problem hiding this comment.
¿Hay alguna razón para tener la vida separada en dos? ¿Por qué no hacer esto?
method text() = "Vida: " + toby.valorVida().toString()
personaje.wlk
Outdated
|
|
||
| method valorVida() = valorVida | ||
|
|
||
| method image() = imagen |
There was a problem hiding this comment.
¿Cambia la imagen dinámicamente? Mismo comentario que las imagenes de los fondos
elementos.wlk
Outdated
| method chocasteCon(personaje) { | ||
| personaje.modificarVida(10) | ||
| } |
There was a problem hiding this comment.
Ahora mismo todos los vehiculos tienen el mismo efecto: bajar vida. Estaría bueno que alguno tuviera algun otro efecto, como por ejemplo subirle la velocidad a todos los vehículos, o hacer que sangre y pierda vida por segundo el personaje hasta que consiga un kit médico. Eso veanlo para que estemos usando el polimorfismo de forma más copada.
Más allá de esto fijense que cuando tengan una clase Vehiculo, que se repite la lógica de que el personaje pierda vida, lo que debería estar definido en cada tipo de vehiculo es cuánta vida le hace perder.
There was a problem hiding this comment.
También estaría bueno que cuando colisiona, el vehiculo desaparezca de la pantalla, para que se entienda que tuvo un efecto la colisión (a nivel visual).
| describe "Test de personaje" { | ||
| test "Personaje colisiona y pierde vida." { | ||
| const auto = new Car() | ||
| auto.chocasteCon(toby) | ||
| assert.that(toby.valorVida() < 100) |
👋! GitHub Classroom created this pull request as a place for your teacher to leave feedback on your work. It will update automatically. Don’t close or merge this pull request, unless you’re instructed to do so by your teacher.
In this pull request, your teacher can leave comments and feedback on your code. Click the Subscribe button to be notified if that happens.
Click the Files changed or Commits tab to see all of the changes pushed to the default branch since the assignment started. Your teacher can see this too.
Notes for teachers
Use this PR to leave feedback. Here are some tips:
For more information about this pull request, read “Leaving assignment feedback in GitHub”.
Subscribed: @GonGarciaFontenla @micarodriguezm @nanoo000 @SofiaBaudo @marcelobuergo @SofiaBaudoUTN