On Refactoring

I am like most developers: the majority of the time, I cannot know (only anticipate) the future, so I write my code as succinctly as possible with reasonable room for flexibility. Life is good. But then reality comes knocking.

Requirements change. Features need to be added. Now your existing code needs to be modified in order for it to support these new constraints.

You think about a quick fix for a moment, but then you remember this:

“There is nothing more permanent than a temporary solution.”

You know what needs to be done. The dreaded refactor. Some of these refactors are absolutely not optional. They affect the foundation of your code. And if you don’t fix foundational issues soon enough, problems will soon become seemingly unfixable. The attempt to fix them will seem Herculean.

I’ve seen this happen plenty of times: when refactors are postponed, they are eventually forgotten or given up on. In practice this neglect caused issues for most projects: if not immediately, surely down the line.

The refactoring of existing code ends up being a boon when it comes to building the new functionality. Far too often one might focus on adding new features without modernizing existing code, which is an absolute necessity.

Observations on refactoring code

• It is often best to think about the entire impact and affected dependencies before you execute a refactor.
• The wrong abstraction can cause more trouble than it solves. If you start refactoring, you’re often altering the type of abstraction. Keep this in mind.
• A good refactor often reduces reduces complexity where it counts. (Some complexity is probably introduced, but this is often hidden complexity which should not affect systems that are actually in use.)
• You should avoid refactoring when not necessary: when a refactor is really necessary, it is usually quite obvious!
• Refactors are a short-term hassle but long-term benefit. They might not make a lot of sense to clients or customers, but it’s not about them (it’s about you!): it’s about paying off technical debt.
• If a dependency has been refactored, ensure to integrate the refactored code into your main branch as soon as possible, and encourage integration into feature branches if applicable.¹

PHP Monitor

Whenever I attempt to ship a new update of PHP Monitor, I have two goals in mind:

• Modernize existing code in such a way that some existing functionality behaves effectively the same but is much easier to maintain or adapt
• Add new features that benefit from the modernized code

For example, last night I reworked a clunky chunk of code in PHP Monitor for an upcoming release. If you’re interested, a technical analysis follows below.

If you’re not into Swift, you can stop reading here and tell your boss that no, in fact, you will be refactoring that thing that you know you should’ve ages ago! 😉

Technical analysis

The impactful changes I worked on recently? Something relatively silly, when I write it down. I added some convenience initializers for NSMenuItem and the addItems method for NSMenu.

These allowed me to significantly simplify the code and avoid having to create variables in order to change the menu item’s target, toolTip, etc.

Here’s what building the First Aid & Services menu item (programmatically) looked like prior to the refactor. I found it hard to read and maintain due to the many variables being instantiated here and lots of repeated code.²

func addFirstAidAndServicesMenuItems() {
    let services = NSMenuItem(title: "mi_other".localized, action: nil, keyEquivalent: "")
    let servicesMenu = NSMenu()

    servicesMenu.addItem(HeaderView.asMenuItem(text: "mi_first_aid".localized))
    let fixMyValetMenuItem = NSMenuItem(
        title: "mi_fix_my_valet".localized(PhpEnv.brewPhpVersion),
        action: #selector(MainMenu.fixMyValet), keyEquivalent: ""
    )
    fixMyValetMenuItem.toolTip = "mi_fix_my_valet_tooltip".localized
    servicesMenu.addItem(fixMyValetMenuItem)

    let fixHomebrewMenuItem = NSMenuItem(
        title: "mi_fix_brew_permissions".localized(),
        action: #selector(MainMenu.fixHomebrewPermissions), keyEquivalent: ""
    )
    fixHomebrewMenuItem.toolTip = "mi_fix_brew_permissions_tooltip".localized
    servicesMenu.addItem(fixHomebrewMenuItem)

    servicesMenu.addItem(NSMenuItem.separator())
    servicesMenu.addItem(HeaderView.asMenuItem(text: "mi_services".localized))

    servicesMenu.addItem(
        NSMenuItem(title: "mi_restart_dnsmasq".localized,
                   action: #selector(MainMenu.restartDnsMasq), keyEquivalent: "d")
    )
    servicesMenu.addItem(
        NSMenuItem(title: "mi_restart_php_fpm".localized,
                   action: #selector(MainMenu.restartPhpFpm), keyEquivalent: "p")
    )
    servicesMenu.addItem(
        NSMenuItem(title: "mi_restart_nginx".localized,
                   action: #selector(MainMenu.restartNginx), keyEquivalent: "n")
    )
    servicesMenu.addItem(
        NSMenuItem(title: "mi_restart_valet_services".localized,
                    action: #selector(MainMenu.restartValetServices), keyEquivalent: "s")
    )
    servicesMenu.addItem(
        NSMenuItem(title: "mi_stop_valet_services".localized,
                    action: #selector(MainMenu.stopValetServices), keyEquivalent: "s"),
        withKeyModifier: [.command, .shift]
    )

    servicesMenu.addItem(NSMenuItem.separator())
    servicesMenu.addItem(HeaderView.asMenuItem(text: "mi_manual_actions".localized))

    servicesMenu.addItem(
        NSMenuItem(title: "mi_php_refresh".localized,
                   action: #selector(MainMenu.reloadPhpMonitorMenuInForeground), keyEquivalent: "r")
    )

    for item in servicesMenu.items {
        item.target = MainMenu.shared
    }

    self.setSubmenu(servicesMenu, for: services)
    self.addItem(services)
}

The code worked just fine, but it was not very readable, nor was it pleasant to look at. I sat down, looked at this mess and said to myself: Wow, this should actually be easy to fix.

In order to address this, I introduced the following convenience initializer for NSMenuItem:

extension NSMenuItem {
    convenience init(
        title: String,
        action: Selector? = nil,
        keyEquivalent: String = "",
        keyModifier: NSEvent.ModifierFlags = [],
        toolTip: String? = nil
    ) {
        self.init(title: title, action: action, keyEquivalent: keyEquivalent)
        self.keyEquivalentModifierMask = keyModifier
        self.toolTip = toolTip
    }
}

There are a variety of advantages to this convenience initializer, but the biggest one is that only title is a required parameter.

Using the original initializer:

let item = NSMenuItem(title: "mi_other".localized, action: nil, keyEquivalent: "")

Using the new initializer:

let item = NSMenuItem(title: "mi_other".localized)

All the other properties default to a basic state which can be customized if necessary. If the menu bar item doesn’t even need an action (like in the example above) then it can be omitted as well.

I also introduced the following NSMenu extension, which makes it easy to add items to the menu in bulk:

extension NSMenu {
    // TODO: Add a convenience initializer: `NSMenu(items:target:)`?
    open func addItems(_ items: [NSMenuItem], target: NSObject? = nil) {
        for item in items {
            self.addItem(item)
            if target != nil {
                item.target = target
            }
        }
    }
}

Now, when you put all of these pieces together… This is what the code now looks like at the time of writing:

func addFirstAidAndServicesMenuItems() {
    let services = NSMenuItem(title: "mi_other".localized)

    let servicesMenu = NSMenu()
    servicesMenu.addItems([
        // FIRST AID
        HeaderView.asMenuItem(text: "mi_first_aid".localized),
        NSMenuItem(title: "mi_view_onboarding".localized, action: #selector(MainMenu.showWelcomeTour)),
        NSMenuItem(title: "mi_fa_php_doctor".localized, action: #selector(MainMenu.openWarnings)),
        NSMenuItem.separator(),
        NSMenuItem(title: "mi_fix_my_valet".localized(PhpEnv.brewPhpVersion),
                   action: #selector(MainMenu.fixMyValet),
                   toolTip: "mi_fix_my_valet_tooltip".localized),
        NSMenuItem(title: "mi_fix_brew_permissions".localized(), action: #selector(MainMenu.fixHomebrewPermissions),
                   toolTip: "mi_fix_brew_permissions_tooltip".localized),
        NSMenuItem.separator(),

        // SERVICES
        HeaderView.asMenuItem(text: "mi_services".localized),
        NSMenuItem(title: "mi_restart_dnsmasq".localized, action: #selector(MainMenu.restartDnsMasq),
                   keyEquivalent: "d"),
        NSMenuItem(title: "mi_restart_php_fpm".localized, action: #selector(MainMenu.restartPhpFpm),
                   keyEquivalent: "p"),
        NSMenuItem(title: "mi_restart_nginx".localized, action: #selector(MainMenu.restartNginx),
                   keyEquivalent: "n"),
        NSMenuItem(title: "mi_restart_valet_services".localized, action: #selector(MainMenu.restartValetServices),
                   keyEquivalent: "s"),
        NSMenuItem(title: "mi_stop_valet_services".localized, action: #selector(MainMenu.stopValetServices),
                   keyEquivalent: "s",
                   keyModifier: [.command, .shift]),
        NSMenuItem.separator(),

        // MANUAL ACTIONS
        HeaderView.asMenuItem(text: "mi_manual_actions".localized),
        NSMenuItem(title: "mi_php_refresh".localized,
                   action: #selector(MainMenu.reloadPhpMonitorMenuInForeground),
                   keyEquivalent: "r")
    ], target: MainMenu.shared)

    self.setSubmenu(servicesMenu, for: services)
    self.addItem(services)
}

Much nicer, don’t you agree?

There are still improvements that can be made (a convenience initializer for NSMenu would be nice, for example) but this is already a marked improvement over the existing code.

I’m now able to much more easily maintain the existing code, and the amount of thought required to add additional menu items is much lower.

This change had a massive impact on my entire StatusMenu class, which is used to programmatically build the contents of PHP Monitor’s menu at runtime, with various NSMenuItem entries, some of which use custom NSViews, including SwiftUI views. (Unfortunately I cannot use SwiftUI for this specific use case yet.)

Result Builders

To further improve upon this, I could make use of Swift’s result builders, which is something I’m considering.

If properly executed, those would allow me to do something like this (code example shortened):

MenuItem {
    HeaderView.asMenuItem(text: "mi_first_aid".localized)
    NSMenuItem(title: "mi_view_onboarding".localized, action: #selector(MainMenu.showWelcomeTour))
    NSMenuItem(title: "mi_fa_php_doctor".localized, action: #selector(MainMenu.openWarnings))
    [...]
}

I’ve had a good experience with result builders for a complex work project that had a massive, flexible sitemap-like structure which was honestly hell to maintain until I used result builders.