Imágenes de "Recoger" cuando se toca y realizan un seguimiento de ello -- bject-oriented campo con swift camp codereview Relacionados El problema

“Pick up” images when tapped on and keep track of it


1
vote

problema

Español

Tratando de obtener mi primer trabajo como desarrollador que hice una aplicación en la que estaba extremadamente orgulloso de:
https://github.com/anatolyrudenko/crystals.dagons
El juego comienza en una habitación aleatoria de un laberinto al azar. Algunos artículos prebuiltados se ubican al azar alrededor del laberinto. El usuario puede ver flechas (de 1 a 4, dependiendo de las instrucciones disponibles para moverse) y los elementos (si hay alguna en la habitación actual) en su pantalla. Puede interactuar con los artículos: recogerlo, usarlo, soltarlo o destruirlo. Los artículos recogidos van al inventario, que es una colección. Para ganar, tienes una llave y abre un cofre con él. Cada vez que viajas desde una habitación a otra, pierdes HP.
Pero los comentarios recibidos en la aplicación me rompieron absolutamente:

  1. Extremadamente mala realización de la recogida de artículos. Todo es malo acerca de eso. Comenzando con crear un artículo en una habitación y terminar con definir qué artículo está en el inventario. Los artículos están ligados rígidamente a las imágenes, se fijó el número de artículos en el juego.
  2. los artículos no tienen identificadores. La identificación por nombre es una mala decisión.
  3. oop es débil. Algunas clases están ahí por el bien de eso. Pero no se utilizan polimorfismo ni herencia. Aunque hay muchas oportunidades para hacerlo.

El problema es que debido a la falta de experiencia, ni siquiera puedo imaginar otra forma de construir esta aplicación. El punto más confuso para mí es 1). Pero si alguien podría dar una sugerencia sobre cómo arreglar 2) y 3), eso sería muy apreciado.
1) -Dadevant código:
Item.swift :

  struct Item {     let imageName: String     let description: String }   

ItemsList.swift :

  struct ItemsList {     var items = [                 Item(imageName: K.icons.chest, description: K.descriptions.chest),                 Item(imageName: K.icons.key, description: K.descriptions.key),                 Item(imageName: K.icons.rock, description: K.descriptions.rock),                 Item(imageName: K.icons.bone, description: K.descriptions.bone),                 Item(imageName: K.icons.mushroom, description: K.descriptions.mushroom),                 Item(imageName: K.icons.apple, description: K.descriptions.apple)                 ] }   

CurrentData.swift :

  struct CurrentData {          var itemsList = ItemsList()     var items = [Item]()     var maze: MazeGenerator     var cell: MazeElement          var size: Int     var location: (x: Int, y: Int) {         didSet {             cell = maze.maze[location.x][location.y]         }     }          var createdImages: [UIImageView] = []     var collectedImages: [(collectedIndex: Int, item: Int, image: UIImage)] = []     var itemLocationArray: [(Int, Int)] = []     var itemCollected: Int?     var collectedImagesIndex = -1     var cellSelected: Int?          init(howManyRooms: Int) {         self.size = Int(ceil(sqrt(Double(howManyRooms))))         self.location = (Int.random(in: 0...self.size-1), Int.random(in: 0...self.size-1))                  items = itemsList.items                  maze = MazeGenerator(self.size, self.size) //generate a maze         cell = maze.maze[location.x][location.y]     } }   

GameplayViewController.swift :

  class GameplayViewController: UIViewController {      private var current: CurrentData?      private func startGame() {         current = CurrentData.init(howManyRooms: rooms)                  for index in 0...current!.items.count-1 {             createItems(imageName: current!.items[index].imageName)             let itemLocation = (Int.random(in: 0...current!.size-1), Int.random(in: 0...current!.size-1)) //Every item gets random location             current?.itemLocationArray.append(itemLocation)         }     }          private func createItems(imageName: String) { //Create item images when launched         let imageNamePNG = "(imageName).png"         let itemImage = UIImage(named: imageNamePNG)         let itemImageView = UIImageView(image: itemImage)         if let freeSpace = self.freeSpace {             itemImageView.frame = CGRect(             x: Int.random(in: Int(freeSpace.minX) ... Int(freeSpace.maxX)), //item images can't cover arrows or inventoryView             y: Int.random(in: Int(freeSpace.minY) ... Int(freeSpace.maxY)),             width: 63, height: 63)         }         let tapGestureRecognizer = UITapGestureRecognizer(target: self, action: #selector(imageTapped(tapGestureRecognizer:)))         itemImageView.isUserInteractionEnabled = true         itemImageView.addGestureRecognizer(tapGestureRecognizer) //item image can be tapped         view.addSubview(itemImageView)         current?.createdImages.append(itemImageView)     }          @objc func imageTapped(tapGestureRecognizer: UITapGestureRecognizer) { //item gets tapped         let tappedImageView = tapGestureRecognizer.view as! UIImageView         if let tappedImage = tappedImageView.image {             if tappedImageView != current!.createdImages[0] { //can't pick up the chest                 tappedImageView.isHidden = true                 tappedImageView.isUserInteractionEnabled = false                                  current?.collectedImagesIndex += 1 //keep track of the order in which we pick up the items                 current?.itemCollected = current!.createdImages.firstIndex(of: tappedImageView) //find out what item was picked                 if let whatItem = current!.itemCollected {                     current?.collectedImages.append((current!.collectedImagesIndex, whatItem, tappedImage))                 }                 collectionView.reloadData()             } else {                 descriptionLabel.text = current!.items[0].description //if chest was tapped             }         }     } }   // MARK: - UICollectionViewDataSource protocol extension GameplayViewController: UICollectionViewDataSource, UICollectionViewDelegate  {    func collectionView(_ collectionView: UICollectionView, numberOfItemsInSection section: Int) -> Int {        return current?.collectedImages.count ?? 0    }    func collectionView(_ collectionView: UICollectionView, cellForItemAt indexPath: IndexPath) -> UICollectionViewCell {                let cell = collectionView.dequeueReusableCell(withReuseIdentifier: K.Cell.reuseIdentifier, for: indexPath as IndexPath) as! CollectionViewCell        cell.inventoryImageView.image = current?.collectedImages[indexPath.item].image        return cell    }        // MARK: - UICollectionViewDelegate protocol        func collectionView(_ collectionView: UICollectionView, didSelectItemAt indexPath: IndexPath) {               for image in current!.collectedImages {            if indexPath.item == image.collectedIndex {                descriptionLabel.text = current?.items[image.item].description                current?.cellSelected = indexPath.item            }        }    } }   

¡Cualquier ayuda es muy apreciada! Gracias

Original en ingles

Trying to get my first job as a developer I made an app I was extremely proud of:
https://github.com/AnatolyRudenko/Crystals.Dragons
The game starts in a random room of a random maze. Some prebuilt items get randomly located all around the maze. User can see arrows (from 1 to 4 depending on directions available to move in) and items (if there are any in current room) on his screen. You can interact with the items: pick it up, use it, drop it or destroy it. Picked up items go to inventory which is a collectionView. To win you've got find a key and open a chest with it. Any time you travel from one room to another - you lose HP.
But the feedback received on the app absolutely smashed me:

  1. Extremely poor realization of item pick-up. Everything is bad about that. Starting with creating an item in a room and ending with defining what item is in inventory. Items are rigidly bound to images, fixed number of items in the game.
  2. Items have no identifiers. Identification by name is a bad decision.
  3. OOP is weak. Some classes are there just for the sake of it. But neither polymorphism nor inheritance are used. Even though there are a lot of opportunities to do so.

The problem is that due to lack of experience I can't even imagine another way of building this app. The most confusing point to me is 1). But if someone could give a hint on how to fix 2) and 3), that would be much appreciated.
1)-relevant code:
Item.swift:

struct Item {     let imageName: String     let description: String } 

ItemsList.swift:

struct ItemsList {     var items = [                 Item(imageName: K.icons.chest, description: K.descriptions.chest),                 Item(imageName: K.icons.key, description: K.descriptions.key),                 Item(imageName: K.icons.rock, description: K.descriptions.rock),                 Item(imageName: K.icons.bone, description: K.descriptions.bone),                 Item(imageName: K.icons.mushroom, description: K.descriptions.mushroom),                 Item(imageName: K.icons.apple, description: K.descriptions.apple)                 ] } 

CurrentData.swift:

struct CurrentData {          var itemsList = ItemsList()     var items = [Item]()     var maze: MazeGenerator     var cell: MazeElement          var size: Int     var location: (x: Int, y: Int) {         didSet {             cell = maze.maze[location.x][location.y]         }     }          var createdImages: [UIImageView] = []     var collectedImages: [(collectedIndex: Int, item: Int, image: UIImage)] = []     var itemLocationArray: [(Int, Int)] = []     var itemCollected: Int?     var collectedImagesIndex = -1     var cellSelected: Int?          init(howManyRooms: Int) {         self.size = Int(ceil(sqrt(Double(howManyRooms))))         self.location = (Int.random(in: 0...self.size-1), Int.random(in: 0...self.size-1))                  items = itemsList.items                  maze = MazeGenerator(self.size, self.size) //generate a maze         cell = maze.maze[location.x][location.y]     } } 

GameplayViewController.swift:

class GameplayViewController: UIViewController {      private var current: CurrentData?      private func startGame() {         current = CurrentData.init(howManyRooms: rooms)                  for index in 0...current!.items.count-1 {             createItems(imageName: current!.items[index].imageName)             let itemLocation = (Int.random(in: 0...current!.size-1), Int.random(in: 0...current!.size-1)) //Every item gets random location             current?.itemLocationArray.append(itemLocation)         }     }          private func createItems(imageName: String) { //Create item images when launched         let imageNamePNG = "\(imageName).png"         let itemImage = UIImage(named: imageNamePNG)         let itemImageView = UIImageView(image: itemImage)         if let freeSpace = self.freeSpace {             itemImageView.frame = CGRect(             x: Int.random(in: Int(freeSpace.minX) ... Int(freeSpace.maxX)), //item images can't cover arrows or inventoryView             y: Int.random(in: Int(freeSpace.minY) ... Int(freeSpace.maxY)),             width: 63, height: 63)         }         let tapGestureRecognizer = UITapGestureRecognizer(target: self, action: #selector(imageTapped(tapGestureRecognizer:)))         itemImageView.isUserInteractionEnabled = true         itemImageView.addGestureRecognizer(tapGestureRecognizer) //item image can be tapped         view.addSubview(itemImageView)         current?.createdImages.append(itemImageView)     }          @objc func imageTapped(tapGestureRecognizer: UITapGestureRecognizer) { //item gets tapped         let tappedImageView = tapGestureRecognizer.view as! UIImageView         if let tappedImage = tappedImageView.image {             if tappedImageView != current!.createdImages[0] { //can't pick up the chest                 tappedImageView.isHidden = true                 tappedImageView.isUserInteractionEnabled = false                                  current?.collectedImagesIndex += 1 //keep track of the order in which we pick up the items                 current?.itemCollected = current!.createdImages.firstIndex(of: tappedImageView) //find out what item was picked                 if let whatItem = current!.itemCollected {                     current?.collectedImages.append((current!.collectedImagesIndex, whatItem, tappedImage))                 }                 collectionView.reloadData()             } else {                 descriptionLabel.text = current!.items[0].description //if chest was tapped             }         }     } }   // MARK: - UICollectionViewDataSource protocol extension GameplayViewController: UICollectionViewDataSource, UICollectionViewDelegate  {    func collectionView(_ collectionView: UICollectionView, numberOfItemsInSection section: Int) -> Int {        return current?.collectedImages.count ?? 0    }    func collectionView(_ collectionView: UICollectionView, cellForItemAt indexPath: IndexPath) -> UICollectionViewCell {                let cell = collectionView.dequeueReusableCell(withReuseIdentifier: K.Cell.reuseIdentifier, for: indexPath as IndexPath) as! CollectionViewCell        cell.inventoryImageView.image = current?.collectedImages[indexPath.item].image        return cell    }        // MARK: - UICollectionViewDelegate protocol        func collectionView(_ collectionView: UICollectionView, didSelectItemAt indexPath: IndexPath) {               for image in current!.collectedImages {            if indexPath.item == image.collectedIndex {                descriptionLabel.text = current?.items[image.item].description                current?.cellSelected = indexPath.item            }        }    } } 

Any help is much appreciated! Thanks

     

Lista de respuestas

1
 
vote

Tus problemas incluyen:

  • naming
  • no siguiendo MVC
  • Decisiones de opcionalidad inconsistentes
  • por un solo controlador de visión
  • no mantener las cosas separadas separadas
  • nombrar cosas inconsistentemente
  1. naming es importante. Tienes:
  • var laberinto: Mazegenerator: un laberinto no es un generador de laberinto.
  • var célula: Mazeelement: elija la celda o el elemento y se pegue con una palabra para ello.
  • var creadoimages: [uiimageView]: una imagen no es una imagen
  1. MVC: los tipos de modelos no deben ser conscientes de la UI. Al diseñar su solicitud, podría ser útil considerar que habrá dos o más cosas que usarán su aplicación: la propia interfaz de IOS, una interfaz de línea de comandos y las pruebas de su unidad. Esto significa en el modelo, algo así como:

    var creadoimages: [UIIMAGEVIEW]

obviamente no pertenece al modelo, porque ata su modelo a su UI.

  1. Decisiones de opcionalidad inconsistentes

Una declaración como:

  // Base Variables6  

muestra que la opcionalidad de "corriente" no está clara. Debe decidir si desea forzar desenvolver (choque si nil) o la cadena opcional (no hacer nada si nil).

aún mejor es evitar la opcionalidad por completo. Cuando está definido, tienes:

  // Base Variables7  

Claramente eso es un modelo opcional. Está diciendo que este controlador de visión podría no tener un modelo, y eso es porque tiene otro problema de diseño, que es que no divides los controladores de su vista. El mismo controlador de vista se usa para tres estados: antes, durante y después de la jugabilidad. En su lugar, si tiene, por ejemplo, un GamesetupViewController, un juegoViewController y un juegoFinishiedViewController, ahora puede tener 'CurrentData' como "actualData actual", genuino, no opcional en GamiewViewController. También sería una buena vista para los controladores aún más vistas, un inventoryviewController, un sitio webViewController, etc.

Si escribe una afección si tiene (o necesita) un comentario que explica la condición de IF, debe eliminar el comentario y reescribir la condición de IF. Por ejemplo:

  // Base Variables8  

reescribir que como:

  // Base Variables9  

y luego considerar MVC: ¿Dónde debe poner el código para decidir si se puede recoger un artículo? Aquí está el ViewController, pero este es un trabajo para el modelo. El método de su controlador se convierte en:

  theme0  

El ViewController debe monitorear los cambios en el modelo, de modo que cuando se recopila el artículo, actualiza la UI.

Luego, en el modelo, debemos decidir si un artículo es cobrable o no: E.G.

  theme1  

Hay otras formas en que podrías hacerlo. Podría tener que ser una función es una función (tal vez devuelva algo como una razón por la que no puede recogerlo, tal vez si es coleccionable depende del gamestato).

Para el mapeo entre la ubicación del artículo del modelo del juego y las vistas de la imagen de su visorcontroller: Ocultar y mostrar imágenes de imágenes no es ideal.

Cuando la habitación cambia, eliminaría todas las vistas de la imagen y haría las nuevas para todos los artículos en esa habitación.

Al crear sus imágenes útiles y toque Reconocimientos de gesto, una de las cosas que necesita hacer es determinar qué artículo están. Un truco para hacerlo es reconocer que el objeto objetivo no necesita ser "uno mismo", es decir, el ViewController. Si usa un objeto de propósito general que puede envolver cualquier bloque de código que le guste:

  theme2  

Ahora, cuando cambia la habitación, y su ViewController realiza vistas de la imagen del artículo: puede hacer por ejemplo.:

  theme3  

"Definiendo qué artículo está en el inventario": Esperaría algo así como una matriz o un conjunto, de identificadores de artículos, en el modelo, que representa lo que el usuario ha recopilado. Si el artículo fuera una clase, no una estructura, entonces una matriz de los objetos en sí.

De esta manera, no necesita algo como CollectEntagesIndex para realizar un seguimiento del pedido en el que recogemos los artículos. La matriz del modelo de artículos recogidos hace eso.

Aquí hay un ejemplo de código que es un problema:

  theme4  
  • La declaración de IF es difícil de leer
  • la corriente es opcional, y podría ser mejor nombrada
  • Las pruebas de igualdad utilizando solo el nombre de imagen (y el controlador que deciden la prueba de igualdad)
  • Esta es la lógica del juego, por lo que debería estar en el modelo, no el ViewController.

Si realiza un tipo personalizado, como elemento, y dos elementos deben poder compararse para la igualdad, debe hacer que el tipo confirma RM al protocolo equitable de Swift. Su controlador debe ser capaz de decir si zatla == itemb, y tiene que definirse el tipo para sí mismo lo que significa ser el mismo.

En la función "Uso", usted realmente desea esta igualabilidad para verificar si se requiere cierta lógica especial. Y tienes una "si fuera la escalera". Este tipo de diseño a menudo no es tan bueno como poner la lógica en el tipo en sí. Por ejemplo:

  theme5  

o más de manera flexible:

  theme6  

Esto está diciendo que un artículo puede tener una función de "uso" que significa que un elemento puede tener lógica personalizada. La función de uso de su controlador envía el uso al modelo, el modelo dice entonces ejecute cualquier lógica personalizada.

 

Your problems include:

  • naming
  • not following MVC
  • inconsistent optionality decisions
  • having only a single view controller
  • not keep separate things separate
  • naming things inconsistently
  1. Naming is important. You have:
  • var maze: MazeGenerator - A maze is not a maze generator.
  • var cell: MazeElement - Choose cell or element and stick with one word for it.
  • var createdImages: [UIImageView] - an image is not an imageView
  1. MVC: Model types shouldn't be aware of UI. When designing your application it might be useful to consider there will be two or more things that will use your application: the iOS UI itself, a command-line interface, and your unit tests. This means in the model, something like:

    var createdImages: [UIImageView]

obviously doesn't belong in the model, because it ties your model to your UI.

  1. Inconsistent optionality decisions

A statement like:

 current?.itemCollected = current!.createdImages.firstIndex(of: tappedImageView)  

shows that the optionality of 'current' is not clear. You should decide whether you want to force unwrap (crash if nil) or optional-chain (do nothing if nil).

Even better is to avoid the optionality altogether. When it's defined, you have:

private var current: CurrentData? 

Clearly that's an optional model. You're saying this view controller might not have a model, and that's because you have another design issue which is that you don't split out your view controllers. The same view controller is used for three states: before, during and after gameplay. Instead, if you have, for example, a GameSetupViewController, a GameViewController and a GameFinishedViewController, now you can have 'CurrentData' as a real, genuine, non-optional 'CurrentData' on GameViewController. Splitting up to even more view controllers would be good too, an InventoryViewController, a RoomViewController etc.

If you write an if condition that has (or needs) a comment that explains the if condition, then you should remove the comment, and rewrite the if condition. For example:

if tappedImageView != current!.createdImages[0] { //can't pick up the chest 

Rewrite that as:

let canPickUpItem = tappedImageView != current.createdImages[0]  if canPickUpItem { ... } 

And then consider MVC: where should you put the code to decide if an item can be picked up? Here it is the ViewController, but this is a job for the model. Your controller method then becomes:

@objc func imageTapped(tapGestureRecognizer: UITapGestureRecognizer) { //item gets tapped     let tappedImageView = tapGestureRecognizer.view as! UIImageView     // Get the item associated with that image view ...     // tell the model that the user wanted to pick up item ...    } 

The ViewController should then monitor changes in the model, so that when the item is collected, you update the UI.

Then over in the model we should decide if an item is collectable or not: e.g.

struct Item {     let imageName: String     let description: String     let isCollectable: Bool  } 

There are other ways you could do it. You could have isCollectable be a function (maybe it returns something like a reason why you can't pick it up, maybe if it is collectable depends on the GameState).

For mapping between the game model's item location and your viewController's image views: Hiding and showing imageViews is not ideal.

When the room changes, I would remove all the image views and make new ones for all the items in that room.

When creating your imageViews and tap gesture recognizers, one of the things you need to do is determine which item they are. One trick to do that is to recognize that the target object doesn't need to be 'self,' i.e. the ViewController. If you use a general-purpose object that can wrap any block of code that you like:

final class TargetAction {     let work: () -> Void      init(_ work: @escaping () -> Void) {         self.work = work     }      @objc func action(_ sender: Any) {         work()     } } 

Now when the room changes, and your viewController makes item image views: you can do e.g.:

for item in currentRoomItems {     let imageView = ...      let didWantToPickUp = TargetAction { [weak self] in         self?.model.userDidWantToPickUp(item)     }      imageView.addTarget(didWantToPickUp, action: #selector(TargetAction.action(_:)) } 

"defining what item is in inventory": I would expect something like an array or a set, of Item identifiers, in the model, that represents what the user has collected. If Item were a class not a struct, then an array of the objects themselves.

This way you don't need something like collectedImagesIndex to keep track of the order in which we pick up the items. The model's array of collected items does that.

Here's an example of code that is a problem:

if current?.items[image.1].imageName == K.icons.apple 
  • The if statement is tricky to read
  • Current is optional, and could be better named
  • The testing for equality using just the imageName (and controller deciding how to test for equality)
  • This is game logic, so it should be in the model not the viewController.

If you make a custom type, like Item, and two items need to be able to compare themselves for equality, you should make the type conform to Swift's Equatable protocol. Your controller should then be able to say if itemA == itemB, and have the type define for itself what it means to be the same.

In the 'use' function you actually want this equatability to check whether certain special logic is required. And you have an "if else ladder". This kind of design is often not as good as putting the logic on the type itself. For example:

struct Item {     let imageName: String     let description: String     let hpToAddWhenUsed: Int // How much to add or subtract on use     let isFinalKey: Bool // means if used with chest then you win the game } 

or more flexibly:

struct Item {     let imageName: String     let description: String     var use: ((inout GameState) -> Void)? } 

This is saying an item can have a 'use' function that means an item can have custom logic. Your controller's use function then dispatches the use to the model, the model then says run any custom logic.

 
 

Relacionados problema

2  Swift HackerRank faltante Números Challenge  ( Swift hackerrank missing numbers challenge ) 
Estoy intentando resolverlo el desafío de "números faltantes" en Hackerrank donde Me encargo de encontrar elementos que faltan comparando dos matrices. {...

0  Estructuración eficiente de los datos con enfoque orientado al protocolo en SWIFT  ( Efficiently structuring data with protocol oriented approach in swift ) 
Necesito integrar algunas llamadas de API del servidor en las que es probable que obtenga una respuesta como: + Response 200 (application/json) + Body ...

3  Aplanar para obtener todos los controles infantiles de cierto tipo en una UIView  ( Flatten to get all child controls of certain type in a uiview ) 
extension Array { func flatten<TResult>(transform: (T) -> TResult?, childArray: (T) -> [T]) -> [TResult] { var result = [TResult]() for ...

5  Mostrando una cierta pantalla dependiendo de un estado particular  ( Showing a certain screen depending on a particular status ) 
En esta aplicación de iOS en la que estoy trabajando, hay dos pantallas de aterrizaje que se muestran al usuario dependiendo de un estado en particular. Las p...

10  Objeto de portafolio hecho de clases de activos y valores  ( Portfolio object made of asset classes and securities ) 
Estoy aprendiendo la programación rápida y orientada a objetos. Soy nuevo para diseñar patrones y programación orientada al protocolo. En mi código de prueb...

6  A * Implementación genérica en SWIFT  ( A generic implementation in swift ) 
My First Code Review Post. Estoy a la altura de una Implementación genérica del algoritmo de búsqueda A * en SWIFT (por ahora, es una sola implementación ...

2  Modelo para hechos matemáticos  ( Model for math facts ) 
He escrito un modelo para una aplicación que genera hechos de matemáticas Los niños tienen que resolver evaluando la operación y la dificultad que le pasa a l...

2  Cargando imágenes en el fondo para una vista de tabla  ( Loading images in the background for a table view ) 
Tengo fetchData2 MÉTODO QUE LLENA MI imagesArray : func fetchData(){ let imageQuery = PFUser.query() imageQuery?.whereKey("username", contained...

2  Formateo de un precio en SWIFT usando una extensión vs. una clase  ( Formatting a price in swift using an extension vs a class ) 
Tengo un precio como un Double , que debería formatearlo como un String . ¿Debo usar una extensión en lugar de clase clásica para encapsular "Formater", "...

4  Función de expresión del interruptor  ( Switch expression function ) 
Hice un montón de funciones y una enumeración, que juntos me permiten escribir algo como una expresión de conmutación. No es tan poderoso como las declaracion...




© 2022 respuesta.top Reservados todos los derechos. Centro de preguntas y respuestas reservados todos los derechos