code_readability.md

It is one of those things which goes hand in hand when compared to code perusing and peer reviewing PRs. It is important to understand the implication of having too much longer strings in a condition. Since not everyone has ultra wide screen monitors and even if for some reason we do. I like to setup mine with 3 Code windows in my IDE to quickly work through the code overall.

Naming schemes

Naming appropriately with variables is important. Try to avoid appending the atype to the variable since the IDE can always show quick help or infer those types automatically with its tooltip option + click.

Less is more.

Two many characters on same line

Also when you refactor this in a function.
Make sure all your conditional checks are on a separate lines.
It's easier for reader to quickly check all the conditions.

Old code

if let manifestUrl = asset?.manifestUrl, let comLocationPostalCode = asset?.comLocationPostalCodeString, let comDeviceType = asset?.comDeviceTypeString  {
    asset?.manifestUrl = "\(manifestUrl)sz=\(comLocationPostalCode)&sz=\(comDeviceType)"
}

New code

if let manifestUrl = asset?.manifestUrl,
   let comLocationPostalCode = asset?.comLocationPostalCodeString, 
   let comDeviceType = asset?.comDeviceTypeString  {
     asset?.manifestUrl = "\(manifestUrl)sz=\(comLocationPostalCode)&sz=\(comDeviceType)"
}

Another example of Ultra wide code

NotificationCenter.default.post(name:NSNotification.Name(rawValue: CustomName), object: player, userInfo: [CustomName_SIGNAL_ID:"x",CustomName_STREAM_ID:"1"])

NotificationCenter
.default
.post(name: NSNotification.Name(rawValue: CustomName), 
	  object: player,
	  userInfo:[
	  CustomName_SIGNAL_ID:"x",
	  CustomName_STREAM_ID:"1"
	  ]
)

more readable rather than one liner and I do have ultra wide monitor but I split my code editors in x3 format so this makes it easier to read and review while on mobile, iPad, or portrait screens like a browser tabs 2/3 of space.

Reactive Paradigm Flow

Stylistic nitpick: Could break the part after ?. output .... on respective lines for easier code reading. Its harder to keep up with more than 2 access like = , . , { , : on a single line.

lifeCycleCancellable = eventOrchestrator?.output.errorEvent.sink(receiveValue: { [weak self] in
	self?.eventOrchestrator?.input.handleError.send(($0, false))
        })

lifeCycleCancellable = eventOrchestrator?
	.output
	.errorEvent
	.sink { [weak self] in
		self?.eventOrchestrator?
			.input
			.handleError
			.send(($0, false))
    }

Parameters naming

Code snippet in review

static func customPlayer(with asset: Asset, authAPI: customAuth) throws -> PlayerConstruction {|
	return try PlayerDirector.construct(with: asset, drmAuthorizer: customAuth) 
}

Stylistic comment

I usually don't like to change params being passed around too much. Since we now have authAPI, drmAuthorizer If you want it named drmAuthorizer internally maybe having similar external param could help and internally reference same.

Since while reading from .construct() it just gets more hazy with 3 diff names -> of course customAuth param value might always change depending on the managed solution construction but we can always control the params internal / external.

Bloated Initializer with Long Array or list

Question:

But wouldn't adding a comment also suffice unless we are using this allowList elsewhere?

Old code

init() { 
	self.bus.add(observer: self, for: [.adQProgress,
            .assetSourceChanged,
            .adEvent,
            .bufferingStarted,
            .bufferingCompleted,
            .esp,
            .error,
            .fragmentWarning,
            .seekStarted,
            .seekCompleted,
    ])
}

New code

init() { 
	let allowedEventTypes: [CustomBusEventType] = [
            .adQProgress,
            .assetSourceChanged,
            .adEvent,
            .bufferingStarted,
            .bufferingCompleted,
            .esp,
            .error,
            .fragmentWarning,
            .seekStarted,
            .seekCompleted,
        ]
    self.bus.add(observer: self, for: allowedEventTypes)	 
}

My reasoning:

Rather than adding comments every time, sometimes having a variable succinct enough to make the code readable in the init makes it easier for the code breakage and structurally having a mind map of what is going on.
In this case you can go either way but since the init had a long list of array having it as a variable makes it more easier for future git diffs or adding more things to the array or being independent from an overloaded init()` while reading it.