When should a function that calculates an object's property be defined within that same class?

Question

As an example below, should the isClient() function be part of the Attendee class?

import Foundation

///A Node that can be used with the CriteriaEvaluator to determine if clients are present in a Meeting.
class ClientCriteria  : Node {

/**
 Calculates a score from 0..1 to identify the likelihood that a list of Attendees contains a client
 */
func score(attendees : [Attendee]) -> Double
{
    for attendee in attendees
    {
        if isClient(attendee: attendee)
        {
            return 1
        }
    }
    return 0
}

private func isClient(attendee: Attendee) -> Bool
{
    let IBMEmailRegex = "[\\/\\w+\\s*]+\\/IBM\\@?\\w*$"
    if attendee.email.contains(IBMEmailRegex) {
        return false
    }
    else{
        return true
    }
}

}

Answer 1

isClient() is a true function. It doesn't rely on state and always returns the same value given the same input.

This means it can be used anywhere. If it's valuable elsewhere, consider moving it to the super class, or perhaps into a utility class. If it's only valuable here, then keep it here to prevent clutter. You can always move it later if someone else can make use of it.

one minor personal preference: I prefer

return !attendee.email.contains(IBMEmailRegex) 

to what you have here. I prefer small code :)

Answer 2

As noted in another answer, the specific function does not access the state of the object. Therefore it can be defined anywhere.

But any function that does access the state should be defined in the class itself. That's what encapsulation is all about.

Answer 3

The reason not to make it a method of Attendee is not because it would not access Attendee data, it would access attendee data were you to make it a method member (email address). The mask to compare it to though is quite arbitrary, it is knowledge outside the Attendee domain. That is why you would not include it. Next thing you would add a method that checks if the attendee has children, which is another thing unrelated to attendence and before you know it you have a God class.

So look at the scope of the data used. If it deals with attendee stuff only, make it a method. If there is other kind of knowledge involved, take it outside or inject the foreign data into the method through an argument.

Answer 4

A general rule of thumb is that if a method doesn't access self and it isn't there to implement/override a protocol/super-class method, then it's on the wrong class. If there is a different class where it would access self if it was moved there, then that's where it should be. In this case, isClient relies on Attendee so it should be attached to Attendee. However, since it doesn't rely on any private state, it should be put in an extension.

Let me explain the reasoning for making it an extension... Ultimately, the isClient method as defined below is a global function that takes one argument... The only difference between func Attendee.isClient: Bool and func isClient(attendee: Attendee) -> Bool is what it looks like at the call site. There is no other difference! By putting the method in an extension, this fact is made more evident. (If it was on the object itself, that would imply that the method requires private parts of the class to function.)

Also, since it's O(1) and always returns the same value given the same input, it should be defined as a computed property:

extension Attendee {
    var isClient: Bool {
        let IBMEmailRegex = "[\\/\\w+\\s*]+\\/IBM\\@?\\w*$"
        return email.contains(IBMEmailRegex) == false
    }
}

I see that score also doesn't depend on self... Maybe score is implementing a protocol that you didn't mention? If not, this should also be moved out of the class, but since it's implementation is O(n) it should be a function:

extension Sequence where Iterator.Element == Attendee {
    /// Calculates a score from 0..1 to identify the likelihood that a list of Attendees contains a client
    func score() -> Double {
        return contains(where: { $0.isClient }) ? 1 : 0
    }
}

Maybe ClientCriteria contains an array of attendees? If so, then I would leave it in the ClientCriteria class

class ClientCriteria : Node {
    private var attendees: [Attendees] = []
    /// Calculates a score from 0..1 to identify the likelihood that a list of Attendees contains a client
    func score() -> Double {
        return attendees.contains(where: { $0.isClient }) ? 1 : 0
    }
}

or:

class ClientCriteria : Node {
    private (set) var attendees: [Attendees] = [] // at least the setter should probably be private.
}

extension ClientCriteria {
    /// Calculates a score from 0..1 to identify the likelihood that a list of Attendees contains a client
    func score() -> Double {
        return attendees.contains(where: { $0.isClient }) ? 1 : 0
    }
}