remotemobprogramming / mob

Tool for smooth git handover.
https://mob.sh
MIT License
1.67k stars 150 forks source link

Scripts getting long #296

Open gregorriegler opened 2 years ago

gregorriegler commented 2 years ago

We're close to 2000 lines in mob.go and its getting a little painful to work with it. I can't find what I'm searching for by scrolling. I use find, and I'm always a little lost. And it's the same with mob_test.go. I think it would be a good idea to start and extract cohesive functionality to their own files. I'm thinking about things like:

Would love to hear your opinions on this.

simonharrer commented 2 years ago

At the moment, everything is highly coupled. That's why having it in one file is better than having it in more files but still being highly coupled (at least from my point of view). The current "architecture" already contains circular dependencies, which is really bad... I documented the current architecture in the docs folder as a plantuml file. Here's the current rendering:

architecture

The code is growing with more features, so we need to do something at some point. We need to decouple when we want to have multiple files. For that, we need clear interfaces between the parts, and they should not have circular dependencies. This will be costly, and there might never be a return of investment. Nevertheless, we can try to go forward.

The key question for me is, how we can achieve a better structure without circular dependencies and is still easy to understand and maintain. And this boils down to the interfaces between the parts, and what parts we need.

We would need some kind of logging/console-output package, a git-cli package, etc.

gregorriegler commented 2 years ago

We can focus on one leaf at a time, and try to extract that. While doing that we will notice whether there are any cyclic dependencies. And if there are any, we would address that. If we can't right now, we focus on another leaf. And then we can try and minimize the interface of the newly extracted module.

I would start with a simple thing.

gregorriegler commented 2 years ago

The PR shows how we could extract leaves without introducing cyclic dependencies.

Now we would have

mob ---------------------⬎
  ⤷configuration ------> say
simonharrer commented 2 years ago

I experimentally refactored a little bit this evening as well.

There are three things that almost every function uses: git, configuration, and say. We can sometimes mitigate git or configuration by passing in the necessary infos instead of getting them within a package, but probably not always does this lead to better code. I am still not sure which direction to take here ... or if this is currently the way to go.

I think having files for every major mob command might be the way forward, with mob.go containing the main entry point and all the other code that's not directly related to a single command. This splits up the code along boundaries every tool user understands. It leaves us with cyclic dependencies, but only to the main file. And from there, we might continue a refactoring, if we think it still necessary.

Overall, I think the say package makes sense. We can extract that anyway.

I am not convinced about the configuration package, as this does so many things and every new command might need to change this as well. Let's keep this in mob.go for now.

gregorriegler commented 2 years ago

Of all the code of mob.go, if I had the opportunity to choose one thing to pull out of it, it would be the configuration code. That code is really long and complicated, especially with the parsing and assigning of all the values. And it is even redundant in some ways, as its reading from multiple sources. So it takes a lot of lines that I'm mostly not interested in when I open up mob.go. It's an implementation detail I'd rather hide. It's really distracting, as it's also high up in the file.

The only reason I pulled out say is that I didn't want to create cyclic dependencies when pulling out configuration. If I didn't pull out say first, I would have created cyclic dependencies.

JanMeier1337 commented 2 years ago

I think the PR https://github.com/remotemobprogramming/mob/pull/297 is a good start. Why not merge it?