Break up the massive installPackage function

[buresdv]

No description provided.

[mattmassicotte]

Here's a link to the function in question:

https://github.com/buresdv/Cork/blob/main/Cork/Logic/Installation%20%26%20Removal/Installation/Install%20Selected%20Packages.swift

[mattmassicotte]

Ok InstallationProgressTracker has zero logic. I think it may make sense to move stuff in there.

[mattmassicotte]

One easy way to recognize this is:

• The function is used in only one place
• The function takes an object as an argument

This feels like a method! The usage looks like this:

let installationResult = try await installPackage(installationProgressTracker: installationProgressTracker, brewData: brewData)

With method it would look more like this:

let installationResult = try await installationProgressTracker.installPackage(using: brewData)

What do you think?

[buresdv]

Hmm that's an interesting idea! I think it could work. The current architecture is a bit unconventional (most of the logic is completely detached from objects, with some exceptions like BrewDataStorage and OutdatedPackageTracker). I was thinking of gradually making those single-use functions into methods

[mattmassicotte]

Making it a top-level function does come with a number of major drawbacks. One is the mutation of the internal state of installationProgressTracker is really awkward, motivating the use of things like installationProgressTracker.packagesBeingInstalled[0]. The other is the logic and the type the logic operates on are in different places.

This isn't inherently wrong! And I want to leave stylistic things like this up to you. It is possible to improve this with more top-level functions as well.

[buresdv]

Thanks for the insight! You're right, the mutation of internal state is one of the motivations I had for adding methods to BrewDataStorage etc.

Truth be told, Cork is my second ever programming project, so it has a lot of weirdness stemming from me having no idea what I was doing back then 😂 I'm always happy to learn, so if you think making it a method would make it more convenient, I'm all for it. I just don't want to make massive changes all over the place, which I would not be able to maintain. But I'm okay with a few changes here and there.

[mattmassicotte]

I think you'll find that not making structural improvements over time will lead to a higher maintenance burden - like this current issue!

The secret is just to go slowly.

[buresdv]

Yes, you're absolutely correct 😊

[mattmassicotte]

Ok I've begun a PR: #295

[mattmassicotte]

I think I may have found a bug though:

line 104 of "Installing.swift"

for var packageToInstall in installationProgressTracker.packagesBeingInstalled

packageToInstall is unused?

[buresdv]

Oh yeah, this is a vestige from WAY back when there was still support for installing more than one package at the same time, which is not coming back. I've been meaning to completely remove the packagesBeingInstalled property and merge it into packageBeingCurrentlyInstalled (which is always PackageInProgressOfBeingInstalled.name, anyway ), creating a new property packageBeingInstalled, like so:

class InstallationProgressTracker: ObservableObject
{
    @Published var packageBeingInstalled: PackageInProgressOfBeingInstalled = .init()

    @Published var numberOfPackageDependencies: Int = 0
    @Published var numberInLineOfPackageCurrentlyBeingFetched: Int = 0
    @Published var numberInLineOfPackageCurrentlyBeingInstalled: Int = 0
}

[buresdv]

So in the end, that loop isn't needed anymore, since there can now be only one package being installed at a time.

[mattmassicotte]

Ah this explains the packagesBeingInstalled[0] all over the place! Ok I will ignore. Fixing this by moving more logic out out of the views and into the ObservedObjects could also help a lot too. But I'm going to leave that.

[mattmassicotte]

I'm also now noticing some real concurrency issues. In particular, it almost never makes sense to have an ObservedObject that is not MainActor-isolated.

Using targeted checking is a good start! But, this refactor may expose more issues. I'm also not going to address this right now, but it's definitely something you'll need to think about.

[buresdv]

You're right! That's definitely something I'm planning. There are a lot of hidden concurrency issues (like #190, I suspect). I'm reading up on the in-depths of concurrency to hopefully fix it soon 😊

[mattmassicotte]

I'm reading up on the in-depths of concurrency to hopefully fix it soon

This can be tough, but please do let me know what you are reading and how it goes! This is an active project of mine.

[buresdv]

I'm reading this: https://donnywals.gumroad.com/l/practical-swift-concurrency and I'd definitely recommend it!

[mattmassicotte]

Ah, I know it and I have already read a few chapters! I was slightly worried, because Donny wrote it before Swift 5.10 shipped and that version contains major changes related to concurrency. But I really should finish it up!

[mattmassicotte]

Ok, I know this is closed, but I thought I'd just throw some more ideas in here. We did the easy stuff: break up a big if-else into two functions and make use of an instance method instead of a top-level function.

But the two remaining functions are still quite large. And now it's harder to simplify. I think there are two things contributing to the verbosity: the logging and the string pattern matching. One idea might be to move the pattern matching into an enum-based system. This might make testing a little easier too. It's possible that using an enum could also internalize some of the logging. Would have to experiment a little to see how that works.

But, my next area of investigation would be changing all of the conditionals like if outputLine.contains("Downloading") into an enum initializer and going from there.

[buresdv]

No problem :) I'll reopen this.

I originally wanted to have the matching to be done in an enum, but I couldn't figure out how to do it. You can see if you can figure something out, I'd be really grateful.