2
\$\begingroup\$

just wanted someone to review my code and perhaps simplify it a bit. Everything is working as expected for the moment but it seems too much code for what i'm trying to achieve. Especially the function "toggleItems". It just feels ... not right.

GOAL: The idea is that i'm having a list with a fixed set of items. When selecting an item, an indicator needs to light up which indicates which list item was pressed last (active state). All other list items need to be deactivated at that point. At first i was using variables in my data model (struct) but wanted to avoid that they could be changed from without the struct so i tried using an immutable struct here (credits to "Swiftful Thinking" on Youtube)

SoundModel:

struct SoundModel: Identifiable {
    let id: String
    let name: String
    let displayName: String
    let isActive: Bool
    
    init(id: String = UUID().uuidString, name: String, displayName: String, isActive: Bool) {
        self.id = id
        self.name = name
        self.displayName = displayName
        self.isActive = isActive
    }
    
    func updateActiveStateToFalse() -> SoundModel {
        return SoundModel(id: id, name: name, displayName: displayName, isActive: false)
    }
    
    func updateActiveStateToTrue() -> SoundModel {
        return SoundModel(id: id, name: name, displayName: displayName, isActive: true)
    }
}

the SoundViewViewModel:

class SoundViewViewModel: ObservableObject {
    
    @Published var sounds: [SoundModel] = []
    
    init() {
        sounds.append(contentsOf: [
            SoundModel(name: "alarm1", displayName: "Alarm 1", isActive: true),
            SoundModel(name: "alarm2", displayName: "Alarm 2", isActive: false),
            SoundModel(name: "alarm3", displayName: "Alarm 3", isActive: false),
            SoundModel(name: "bird1", displayName: "Bird", isActive: false),
            SoundModel(name: "carhorn1", displayName: "Carhorn", isActive: false),
            SoundModel(name: "fireAlarm1", displayName: "Fire Alarm", isActive: false),
            SoundModel(name: "mellow", displayName: "Mellow", isActive: false),
            SoundModel(name: "meltdown1", displayName: "Meltdown", isActive: false),
            SoundModel(name: "schoolbel1", displayName: "Schoolbel", isActive: false),
            SoundModel(name: "siren1", displayName: "Siren", isActive: false),
            SoundModel(name: "ufo1", displayName: "Ufo", isActive: false),
        ]
        )
    }

    func toggleItems(soundItem: SoundModel) {

        if let index = sounds.firstIndex(where: { $0.id == soundItem.id } ) {
            if !soundItem.isActive {
                sounds[index] = soundItem.updateActiveStateToTrue()
                
            }
        }
        
        for sound in sounds {
            if let index = sounds.firstIndex(where: { $0.id == sound.id && $0.id != soundItem.id }) {
                if sounds[index].isActive {
                    sounds[index] = sound.updateActiveStateToFalse()
                }
            }
        }
    }
}

And my SoundView:

struct SoundsView: View {
    
    @EnvironmentObject var theme: ThemeManager
    
    //var soundManager = SoundManager()
    @StateObject var soundViewVM = SoundViewViewModel()
    
    var body: some View {
        List {
            ForEach(soundViewVM.sounds) { sound in
                Button {
                    //soundManager.playSound(name: sound.name)
                    soundViewVM.toggleItems(soundItem: sound)

                } label: {
                    HStack(alignment: .center, spacing: 2) {
                        Text(sound.displayName)
                        Spacer()
                        sound.isActive ? Circle().frame(width: 10).foregroundColor(theme.lightColor) : nil
                    }
                }
            }
        }
        .listStyle(.elliptical)
        .navigationTitle {
            HStack {
                Text("Sounds")
                Spacer()
            }
        }
    }
}
\$\endgroup\$
3
  • \$\begingroup\$ Hmm weird, let me review it for a sec. About the listStyle and navigationTitle, I'm using it in a watchOS app. \$\endgroup\$ Apr 24, 2022 at 8:40
  • \$\begingroup\$ It works here. I guess the issue is within the toggleItems(soundItem: SoundModel) function as I had the same behavior as well when I started, but like it's written here it should function correctly. \$\endgroup\$ Apr 24, 2022 at 8:48
  • \$\begingroup\$ You are right, I'm sorry. don't know how I copied that code wrong :/ sounds[index] = sound.updateActiveStateToFalse() does the trick (I already have the sound object from the for loop). I will edit again in my code. Now, looking at that function, should it be the right way to do it ? getting the indexes. Or is there a more simple manner ? \$\endgroup\$ Apr 24, 2022 at 8:55

1 Answer 1

3
\$\begingroup\$

The toggleItems() function can be simplified to

func toggleItems(soundItem: SoundModel) {
    
    for (index, sound) in sounds.enumerated() {
        if sound.id == soundItem.id {
            sounds[index] = sound.updateActiveStateToTrue()
        } else {
            sounds[index] = sound.updateActiveStateToFalse()
        }
    }
}

with one simple loop instead of two loops in your code (where the second one is essentially a nested loop).

But what I would really do is to store the information about the currently selected sound in the view model, instead of having an isActive property in SoundModel:

class SoundViewViewModel: ObservableObject {
    
    @Published var sounds: [SoundModel]
    @Published var activeSoundId : SoundModel.ID?
    
    init() {
        let allSounds = [ ... ]
        sounds = allSounds
        activeSoundId = allSounds.first?.id
    }
}

Then making a sound “active” becomes as simple as

soundViewVM.activeSoundId = sound.id

without the need to traverse the array and updating each entry with a new (active or inactive) one. This makes the toggleItems() method obsolete, and the view code becomes

struct SoundsView: View {
    
    @StateObject var soundViewVM = SoundViewViewModel()
    
    var body: some View {
        List {
            ForEach(soundViewVM.sounds) { sound in
                Button {
                    soundViewVM.activeSoundId = sound.id
                } label: {
                    HStack(alignment: .center, spacing: 2) {
                        Text(sound.displayName)
                        Spacer()
                        if sound.id == soundViewVM.activeSoundId {
                            Circle().frame(width: 10)
                        }
                    }
                }
            }
        }
    }
}

The isActive property and the updateActiveStateToFalse/True() methods in SoundModel are no longer needed. Instead of converting the UUIDs to strings one can use the UUID directly for identification:

struct SoundModel: Identifiable {
    let id: UUID
    let name: String
    let displayName: String
    
    init(id: UUID = UUID(), name: String, displayName: String, isActive: Bool) {
        self.id = id
        self.name = name
        self.displayName = displayName
    }
}
\$\endgroup\$
1
  • \$\begingroup\$ Wow very nice approach. Going to look into it in depth an try to use this approach more. \$\endgroup\$ Apr 24, 2022 at 9:44

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.