-
Notifications
You must be signed in to change notification settings - Fork 193
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
Improve Modular Code pattern #475
Conversation
fioddor
commented
Oct 22, 2022
- Problem Generalize the problem (less contextual). Modularization solves more problems in different contexts.
- Context
- Remove specific circumstances
- Add generic ones
- Forces:
- Group in against / in favour / neutral
- Turn 'Frequent turnover' into a favorable circumstance
- Explain impact opportunity for newbies
- Solutions:
- Group and order advice bits
- Add: education on why
- Add advice on finding modular feature candidates
- Add advice to address bug changes in small bits
- Resulting Context
- Move bits from solutions section here (they describe the effects, not the undertakings)
Generalize the problem (less contextual): - Predisposition is more of a force than a problem. - Modularization solves more problems in different contexts. Detail further the effects of lack of modularization.
- Remove the lack of mandate because it might or not be the case. - Remove the exclusion of legacy code. It is a clear use case. - Making code modular doesn't take extra effort and time over the alternatives. + Include advantages for documentation
* Group in pro/con/neutral * Turn Frequent turnover into a pro * Explain impact opportunity for newbies
Move some points from Solutions section to Resulting Context. They describe effects of modularization, not actions to be taken towards it.
1. Describe modularization as the main solution. The rest are secondary actions to implement and/or support modularization as a practice. 2. Prepare (offer education) first 3. Then, decide to go for it 4. Finally take the first steps. - Mitigating risk and fear of changes is a result, not an action. - Providing incentives is questionable. Its benefits should suffice. - Paralell labour moved to 'Resulting context' section already as it is a result, not an action. Remove duplication.
Extend the first steps. * Group standards for reusability * Advice small steps
+ Extend the education part. + Structure it
Thank you for improving this pattern @fioddor. Can you share what triggered this work? |
Thank you for working on patterns on a regular basis.
I wanted to re-use the pattern as is but found that some bits were at the wrong section, etc.
Yes. Two corporations working on different verticals. I don't dare to publicly disclose any more than that, right now. I expect to name at least one of them if not both on the pattern in the near future. Meanwhile we can discuss more under Chatham house rules. |
Co-authored-by: Mishari Muqbil <mishari@mishari.net>
@NewMexicoKid would you be willing to handle the review for this PR, given that you wrote the pattern? Also curious to hear what you think about the "against/in-favor/neutral" approach chosen in the Forces section. Have not seen this one before :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Many orgs don't realize semantic versioning and dependency management also applies to their internal code base. I propose these lines so people can look up more information.
Add more steps to modularization sequence. Co-authored-by: Mishari Muqbil <mishari@mishari.net>
Thanks @NewMexicoKid
The unsuccessful linkChecker fails at a URL not present in the changed document. |
@fioddor thanks for raising this issue. Not sure why the link checker does that. It should only check links in markdown files that are part of this PR but apparently it is failing here. We can simply ignore the link check for this PR. |
@fioddor sorry for the slow response on this PR. Was trying to find another reviewer for this content but I failed. I will go ahead and review it now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fioddor I did a first round of review. Great job!
Left a bunch of comments, mostly related to some spelling errors.
I had a hard time assessing how the old and new Solution section are different, as so much was changed there. Will read it one more time in the coming days, problem just focusing on whether the new Solution section makes sense for me (rather than comparing it to the old one).
Btw by all means add yourself to the "Authors" section, as you are doing a rather significant rewrite here.
patterns/1-initial/modular-code.md
Outdated
* With rare exceptions, monolithinc code is difficult to contribute to. | ||
* The same functionality is reimplemented redundantly causing | ||
* Waste of unneeded re-design and re-development | ||
* Duplicated mainteinance effort |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* With rare exceptions, monolithinc code is difficult to contribute to. | |
* The same functionality is reimplemented redundantly causing | |
* Waste of unneeded re-design and re-development | |
* Duplicated mainteinance effort | |
With rare exceptions, monolithic code is difficult to contribute to and extend. | |
That leads development teams to build similar functionality multiple times, leading to both waste (unneeded re-design and re-development) as well as duplicated maintenance effort. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In terms of formatting, I am suggesting to not use bullets here. We already have enough bullets in the other sections :)
Also fixed some typos and rewrote the Problem statement a bit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typos and formatting: ok
I see that monolithic code might be a cause for redundant implementation, but it is not always so. Redundant implementation might happen for other reasons, so I'd keep it as another problem that modularization solves.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry but I could not follow your train of thought here, please help me :)
Are you saying that your lines 11 and 12 above are describing two distinct unrelated problems, that can be improved through more modular code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, 2 different problems:
- Monolithic code packing much functionality makes it difficult to contribute (much to read, much to grasp, much to test, much to review, and much to document). Finding slots and the right mood to attack big chunks of work is difficult. This is regardless of how many other alternative implementations you have.
- Redundant implementations tend to be a waste of effort developing, documenting, and maintaining them all.
Strictly speaking, modularity is about reusing parts (modules) and solves redundancy.
But engineering the parts to be small and narrow-focused (breaking the monolith) helps a lot in fostering collaboration. More so in the case of volunteer collaboration.
No worries. I also was absent a lot of time. This is a voluntary effort and we're busy people. ;-) |
Co-authored-by: Sebastian Spier <github@spier.hu>
Co-authored-by: Sebastian Spier <github@spier.hu>
Co-authored-by: Sebastian Spier <github@spier.hu>
@fioddor left some clarifying questions inline in the comment threads. |
According to review comments. Thank you, Sebastian Spier
Elaborate on how modular code helps agile.
As suggested by my reviewer.
@spier, I separated the changes in different commits. For the Solution section we have 2. For the big one, some clues can be found in their description notes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fioddor had time now to step through the changes, so I do understand how the new Solution section came together. Thanks for your pointers to that!
I left a couple more comments for you to consider.
After that this PR is good to be merged from my end.
patterns/1-initial/modular-code.md
Outdated
1. find internal would-be customers. Better ask them over guessing on your own. | ||
1. define decoupling scope and preferred decoupling methods, | ||
1. develop automated testing protection | ||
1. document usage of modules. | ||
1. introduce semantic versioning | ||
1. use a dependency management software such as pip, nuget, npm |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1. find internal would-be customers. Better ask them over guessing on your own. | |
1. define decoupling scope and preferred decoupling methods, | |
1. develop automated testing protection | |
1. document usage of modules. | |
1. introduce semantic versioning | |
1. use a dependency management software such as pip, nuget, npm | |
1. Find internal would-be customers. Better ask them over guessing on your own. | |
1. Define decoupling scope and preferred decoupling methods. | |
1. Develop automated testing protection. | |
1. Document usage of modules. | |
1. Introduce semantic versioning. | |
1. Use a dependency management software such as pip, nuget, npm. |
I think we should start all points with capital letters, to be consistent with the rest of the pattern.
Also added punctuation after each bullet point.
One question:
This is the only part of the pattern where we use a numbered list.
Is this on purpose? i.e. is there an order to this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fioddor This is the only part of the pattern where we use a numbered list.
Is this on purpose? i.e. are these points meant to be used in this order?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Arguably yes. I thought of a sequence, but it isn't the only possible one, so I don't have a strong opinion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it. Let's keep it as is then, and see if it sticks :)
Suggested by reviewer Co-authored-by: Sebastian Spier <github@spier.hu>
As suggested by reviewer. Co-authored-by: Sebastian Spier <github@spier.hu>
Reviewer commented: > Is this meant to be it's own bullet, or should it be part of the previous one? > > If the latter, then we need to check after publishing this whether gitbook renders this correctly. I vaguely recall some behavior there where GitHub and gitbook rendering of markdown differ.
Hi @fioddor thanks for working in my feedback. I stepped through the resolved conversations now to see which way you were leaning. Thanks again for your work on this. If you could review the re-opened conversations, then we should be pretty close to merging this PR. |
As suggested by reviewer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow, this has been a long time in the making but now all the amazing improvements to the Modular Code pattern are available on the To get this pattern published in our online book, we would have to get a company to confirm that they are using this pattern as part of their InnerSource approach. So far the pattern states this
I wonder if we should consider this to be enough to get this pattern published? |