Feedback

[github-classroom]

👋! 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:

• Click the Files changed tab to see all of the changes pushed to the default branch since the assignment started. To leave comments on specific lines of code, put your cursor over a line of code and click the blue + (plus sign). To learn more about comments, read “Commenting on a pull request”.
• Click the Commits tab to see the commits pushed to the default branch. Click a commit to see specific changes.
• If you turned on autograding, then click the Checks tab to see the results.
• This page is an overview. It shows commits, line comments, and general comments. You can leave a general comment below.
  For more information about this pull request, read “Leaving assignment feedback in GitHub”.

Subscribed: @AlvaroGianola @alexFiorenza @kevinanton01 @NatyMartirosyan @Valentin-Sosa

[tfloxolodeiro]

Aca les dejamos las correcciones del TP.

Para la proxima entrega corrijan estas cosas y sigan avanzando con el juego. Tambien, hagan que hayan varios elevadores que se activen por botones o por palancas. Por lo menos que hayan 4 elevadores, 2 por botones y 2 por palancas. Deberian modelarlo de una forma que pueda haber cualquier cantidad de elevadores por boton o elevadores por palanca en un nivel.

[tfloxolodeiro]

Toda la logica de salto se puede simplificar haciendo algo como:

	method movIzquierda() 
	{
		self.moverse(position.left(1))
	}

	method movDerecha() 
	{
		self.moverse(position.right(1))
	}

	method movSubir() 
	{
		self.moverse(position.up(1))
		game.sound("salto.ogg").play()
	}
	
	method movSaltar() 
	{
		if (cantSaltos < 2) 	{
			self.movSubir()
			cantSaltos += 1
		}
	}
	
	method caer() 
	{
		if(self.puedeMoverse(position.down(1))){
			position = position.down(1)
		}
		else {
			cantSaltos = 0
		}
	}

Y en el main.wpgm hacer algo como:

	game.onTick(500, "Caer", {fireboy.caer()})
	game.onTick(500, "Caer", {watergirl.caer()})

De esta forma, siempre estamos checkeando si los personajes deberian caer, y no cada vez que hace un movimiento. Tambien simplificamos el modelo, porque sacamos atributos innecesarios, como estaSaltando.

[tfloxolodeiro]

Como el elevador solo puede estar en una de 2 posiciones posibles, esto se puede simplificar como:

class Elevador inherits Elemento
{
    const posicionInicial
    var activado = false
    method image() = "elevador2.png"
    override method tipo() = "NoColisionable"

    method activarse()
    {
        game.sound("elevador.ogg").play()
    }

    method position(){
        if(activado){
            return posicionInicial.down(1)
        }

        return posicionInicial
    }
}

Esto borra los metodos subir y bajar, que usan en otros objetos, asi que la activacion tiene que tratarse de otra forma. En los proximos comentarios hablo al respecto.

[tfloxolodeiro]

Con la nueva logica de elevador, ya no seria necesario implementar estos metodos, asi que el objeto quedaria sin implementacion, como el elevadorPorPalanca.

[tfloxolodeiro]

Aca podrian hacer directamente

object botonAbajo inherits Boton (position = game.at(5, 7))
{
    method chequearBoton()
    {
        if(self.estaPresionado() || botonArriba.estaPresionado()) elevadorPorBoton.activo(true)
        else elevadorPorBoton.activo(false)
    }
}

[tfloxolodeiro]

Comento aca respecto un patron que usan en varias partes del codigo, que es el checkeo por tipo. Osea a este tipo de logica:

class Boton inherits Elemento 
{
    method estaPresionado()
    {
        const posibles = game.getObjectsIn(self.position())
        return posibles.any({objeto => objeto.tipo() == "Personaje"}) // Esto es un checkeo de tipo
    }
}

Esto lo hacen para saber como pueden interactuar los distintos elementos entre si. Por ejemplo, un boton solo puede ser presionado por un "Personaje", y un personaje no puede moverse a una posicion si hay un "NoColisionable" ahi.

Un primer problema con este patron es el modelado con strings. Los strings unicamente se deberian usar para modelar cosas que son necesariamente textos, como nombres. Aca se está usando strings para modelar un "identificador" de tipo. Un problema con esto es que hay que tener siempre en la cabeza (o tener que ir buscar en algun lado) que tipos existen y como es que se escriben. Tambien, tenemos que siempre tener cuidado con no escribir mal el string. Esto puede llevar a casos donde en algun lado escribimos "NoColisoinable" en vez de "NoColisionable" y despues estamos 2 horas tratando de encontrar el error. Por lo menos una vez por año hay un grupo que le pasa eso.
Modelar con strings cosas que no son naturalmente texto es un error que suele surgir en el parcial. Como regla general, si estan modelando algo con string que no es algo como un nombre, muy posiblemente haya una mejor solucion.

Por otra parte, hay un problema con definir un "tipo" a cada elemento, y es que no puede tener mas de un tipo. Por ejemplo, si quisiesemos hacer que el personaje sea tanto "Personaje" como "NoColisionable", con el modelado actual no seria posible.

Para solucionar todo esto, podriamos plantear esto de otra forma. En vez de pensar que "tipo" es un objeto, podriamos pensar que comportamientos permite el objeto. Por ejemplo:

class Boton inherits Elemento 
{
    method estaPresionado()
    {
        const posibles = game.getObjectsIn(self.position())
        return posibles.any({objeto => objeto.puedePresionarBoton()})
    }
}

Cada elemento sabrá como responder ese mensaje. Esto nos da una mayor flexibilidad. Por ejemplo ahora podriamos hacer que el cubo pueda presionar el boton.

De esta forma, ya no tenemos un atributo "tipo", si no que directamente le preguntamos al objeto si permite el comportamiento que queremos hacer.

Lo mismo deberian hacer con los "NoColisionable", y con cualquier otro objeto que tenga un comportamiento particular respecto a su tipo.