Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Scripts getting long #296

Open
gregorriegler opened this issue Jul 7, 2022 · 6 comments
Open

Scripts getting long #296

gregorriegler opened this issue Jul 7, 2022 · 6 comments

Comments

@gregorriegler
Copy link
Collaborator

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:

  • configuration (loading it from cli, files and the struct)
  • branch
  • timer
  • and potentially others too

Would love to hear your opinions on this.

@simonharrer
Copy link
Member

simonharrer commented Jul 8, 2022

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
Copy link
Collaborator Author

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 gregorriegler linked a pull request Jul 8, 2022 that will close this issue
@gregorriegler
Copy link
Collaborator Author

gregorriegler commented Jul 8, 2022

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

Now we would have

mob ---------------------⬎
  ⤷configuration ------> say

@simonharrer
Copy link
Member

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
Copy link
Collaborator Author

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
Copy link
Contributor

I think the PR #297 is a good start. Why not merge it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants