"Pick up" images when tapped on and keep track of it

Question

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

Answer 1

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

2. 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.

3. 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.