I took it upon myself to improve PHP Monitor’s code this week-end. After all, I originally hastily threw together the code to get things working, and I haven’t really bothered with code quality on this project. Remember, it was initially started as a super hacky project back in 2019.
Now, it’s an important utility that I use almost every day, so I want to make sure everything is in order. So, I cleaned up some of the code! You can look at the latest changes here. In this post I’d like to look at three examples of changes I made that made the code more maintainable for myself.
Example #1: Environment Checks
Here we go. The first example: when the app starts up, it performs various environment checks. If the condition is met, something is broken. Then, the app will throw up a modal informing you of said problem. For example, I use this method to check for the absence of the php binary.
This is what the code looked like:
private func performEnvironmentCheck(
_ condition: Bool,
messageText: String,
informativeText: String,
breaking: Bool
) {
if (condition) {
if (breaking) {
self.failed = true
}
DispatchQueue.main.async {
_ = Alert.present(
messageText: messageText,
informativeText: informativeText
)
if (breaking) {
self.failureCallback()
}
}
}
}
There’s a couple of issues here.
Obviously, not exiting early is one of those things that results in tons of nasty indentation, so that’s the first thing I addressed. Next up is the unnecessary if that ends up assigning a boolean. Finally, cleaning up the callback that takes place on the main thread cleans up the function nicely.
After improving the code, the function now looks like this:
private func performEnvironmentCheck(
_ condition: Bool,
messageText: String,
informativeText: String,
breaking: Bool
) {
// Early exit
if (!condition) { return }
// Directly assign here
self.failed = breaking
DispatchQueue.main.async {
// No more assign to _ required due to new `notify` method
Alert.notify(message: messageText, info: informativeText)
// Unchanged, but cleaner without indentation
if (breaking) { self.failureCallback() }
}
}
Example #2: Running brew commands
PHP Monitor runs a ton of brew instructions. To do so, I previously had a lot of stuff like this in the Actions class:
public static func restartPhpFpm() {
let version = App.shared.currentVersion!.short
if (version == App.shared.brewPhpVersion) {
Shell.user.run("sudo \(Paths.brew()) services restart php")
} else {
Shell.user.run("sudo \(Paths.brew()) services restart php@\(version)")
}
}
Okay, so this one seems obvious. Usually, having if / else in your code can be quite the code smell. There’s usually a better way to do whatever it is you want to do. Exiting early, simplifying code, etc. This is what the code looks like now:
public static func restartPhpFpm() {
brew("services restart \(App.phpInstall!.formula)", sudo: true)
}
There’s a couple of things that happened here. First, App.phpInstall now contains the information about the current PHP installation, along with a new computed variable (formula), which looks like this:
var formula: String {
return (self.version.short == App.shared.brewPhpVersion)
? "php"
: "php@\(self.version.short)"
}
The new brew method was added, as well:
private static func brew(_ command: String, sudo: Bool = false) {
Shell.run("\(sudo ? "sudo " : "")" + "\(Paths.brew) \(command)")
}
Much better. If you notice, Paths.brew is now also a computed variable, as opposed to a method. That significantly improves the readability of the code.
Example #3: Reworking PhpVersion
Previously, the information about the current PHP installation (including plugin state, version number, etc.) was all included in the PhpVersion class. When all this class contained was version information, this was fine.
At some point, I added support for detecting the Xdebug extension. The state of whether the extension is enabled was now also stored in this particular model. This meant that the name of the class was no longer a great fit.
The structure of the class was also problematic. Consider these variables in the old source:
var short : String = "???" // short version representation (e.g. 8.0)
var long : String = "???" // long version representation (e.g. 8.0.1)
var xdebugFound: Bool = false // Xdebug module was found in config file
var xdebugEnabled : Bool = false // Xdebug module was enabled
var error : Bool = false // there's an issue with PHP (after running php -v)
They work, but I was not a fan of the structure here. Before we can look at the new variables, you need to see these structs:
// MARK: - Structs
struct Version {
var short = "???"
var long = "???"
var error = false
}
struct Xdebug {
var found: Bool = false
var enabled: Bool = false
}
Okay, so these structs contain the relevant information in a much more accessible fashion. To retrieve whether Xdebug was found, I’d need to access install.xdebug.found.
Here’s the variables:
// MARK: - Structs
var version: Version = Version()
var xdebug: Xdebug = Xdebug()
// MARK: - Computed
var formula: String {
return (self.version.short == App.shared.brewPhpVersion)
? "php"
: "php@\(self.version.short)"
}
As you can see, the computed property can instantly leverage this functionality, and I don’t need a ton of variables in the PhpInstall class either.
I prefer this approach over the previous one, which would only get worse as I add support for more features and flags that are associated with a particular PHP installation.
This, on the other hand, would scale nicely. Or would it? You see, I could build something a bit more complex that allows registering multiple extensions, like so:
var extensions: [PhpExtension] = []
self.extensions = [
new PhpExtension("Xdebug", enabledBy: "zend_extension=\"xdebug.so\""),
new PhpExtension("Imagick", enabledBy: "extension=\"imagick.so\""),
]
This in particular is another change I’d like to make in the near future.
I’ve implemented this in an even better way! This change will be part of the next update to PHP Monitor. You can grab the app here for free.