Secret Santa with Groups on Swift

I am working on an special version of Secret Santa with internal sub-groups.

Typical Secret Santa (SS) requirements:

• Cannot be assigned to themselves

• Should be random

• Every participant should be assigned to someone

• Every participant should have someone assigned to

Special requirements; there are subgroups that should be taken into consideration:

• Reduce participants assigned to someone into their same group as much as possible (none if possible)

Implementation

I am new to Swift, I am working on a playground. Let's first consider the problems to solve:

Problems to consider

For one group (normal SS)

Let's assume a 3 people group

1->2
2->1
3->?

In that case there is no possible person for 3 to give to.

For multiple groups

Assume these 3 groups group-participant:

[1-1,1-2]
[2-1]
[3-1]

This could be assigned like this

2-1 -> 3-1
3-1 -> 2-1
1-1 -> 1->2
1-2 -> 1->1

Which is not a desired outcome.

Algorithm

Pick random participant p from all the possible participants. Also save the first p apart.

if p is not from the largest group
    r is a random element from the largest group
else if p is from the largest group and there are multiple non-empty groups
    r is a random element from the any other group
else if p is not from the largest group and is from the only remaining group
    r is a random element from its group that it is not p
p gives present to r

Now remove p from the groups and repeat the same but make p=r until it is there are no more participants. The last one gives to the first one and the shuffle is complete.

Code

```
class Participant: CustomStringConvertible {
var name = "noname"
var contact = "nocontact"
var giveTo:Participant?
init(name:String,contact:String){
self.name = name
self.contact = contact
}
var description: String {return name + ((giveTo==nil) ?"":"->\(giveTo!.name)")}
}

class Group: Cu

Your code works – as far as I can see – correctly. I cannot judge
the fairness of the algorithm itself, but I have some suggestions
concerning the implementation.

My first point of criticism is the use of the static Group property
allGroups:

• Each created Group instance is implicitly appended to allGroups which

is not obvious to the user of your code. The call Group(participants:[...])
creates a group and discards the result (which gives a compiler warning),
so the initializer is called purely for its side effect.

• It makes the code less reusable. You cannot create an independent second

set of groups.

I would suggest to build an array of groups explicitly:

let g1 = Group(participants:[...])
let g2 = Group(participants:[...])
let g3 = Group(participants:[...])
let allGroups = [g1, g2, g3]

Similarly, the static Group property counter is used to assign
consecutive names to the each created group. This is acceptable, but why
not create each group with an explicit name (as you already do for the
participants)?

let g1 = Group(name: "G1", participants:[...])
let g2 = Group(name: "G2", participants:[...])
let g3 = Group(name: "G3", participants:[...])

There is too much logic in the Group class. In fact all the static methods
are used for the "Secret Santa" algorithm, and do not use any state of
the Group class. I suggest to create a separate
class for that purpose:

let g1 = Group(name: "G1", participants:[...])
let g2 = Group(name: "G2", participants:[...])
let g3 = Group(name: "G3", participants:[...])

let santa = SecretSanta(groups: [g1, g2, g3])
let pairs = santa.assignPairs()
print(pairs)

Now lets have a look at the Participant class. There is no need to assign
default initial values "noname", "nocontact" because both properties are
assigned to in the init method. Moreover, name and contact do never change,
so they can be constant properties with let:

class Participant {
    let name : String
    let contact : String
    var giveTo : Participant?

    init(name: String, contact: String) {
        self.name = name
        self.contact = contact
    }
}

For better structuring of the code, protocol implementations such as
CustomStringConvertible can be written as an extension.
Testing against nil and implicit unwrapping should be avoided in favor
of optional binding:

extension Participant: CustomStringConvertible {
    var description: String {
        var desc = name
        if let receiver = giveTo {
            desc += " -> " + receiver.name
        }
        return desc
    }
}

or more compactly using the Optional.map() method:

extension Participant: CustomStringConvertible {
    var description : String {
        return name + (giveTo.map { " -> " + $0.name } ?? "")
    }
}

In the remove() method of Group the name of the participant is used
to identify it in an array. This is error-prone because it relies on unique
names. A better solution is to implement the Equatable protocol, so that
participants can be compared with ==. Since Participant is a class,
i.e. a reference type, this can be done with the "identical-to"
operator ===, i.e. only identical instances are considered equal:

extension Participant : Equatable { }

func ==(lhs : Participant, rhs : Participant) -> Bool {
    return lhs === rhs
}

Now a simple participants.indexOf(p) can be used to find a participant
in an array.

The same suggestions apply to the Group class. If we remove the
static properties and methods, this is what it could look like:

class Group {

    let name : String
    var participants: [Participant]

    init(name: String, participants: [Participant]) {
        self.name = name
        self.participants = participants
    }

    var size : Int {
        return participants.count
    }

    func randomParticipant() -> Participant {
        precondition(participants.count > 0, "participants array is empty")
        return participants[Int(arc4random_uniform(UInt32(size)))]
    }

    func removeParticipant(participant : Participant) {
        guard let index = participants.indexOf(participant) else {
            fatalError("participant not found in group")
        }
        participants.removeAtIndex(index)
    }
}

extension Group : CustomStringConvertible {

    var description : String { return name }
}

extension Group : Equatable { }

func ==(lhs : Group, rhs : Group) -> Bool {
    return lhs === rhs
}

• I have made size a computed property instead of a function, similar to

count for arrays or length for strings.

• In getRandParticipant() I have removed the "get" prefix which should not

be used. In addition, a precondition is added to detect programming errors.

• remove() is renamed to removeParticipant() and expects that the

participant is present in the array. Instead of filtering the array,
indexOf and removeAtIndex are used as a small optimization.

The remaining logic is m

StackExchange Code Review Q#114480, answer score: 4