Hello! This is Matsun from the freee card team. I am developing and operating freee card Unlimited . I joined freee a year ago and am a member of the freee card team. Looking at my career as an engineer (more than 10 years), I feel that my current team has learned a lot from PR reviews and refactoring. I will summarize the top 5 (my subjective opinion) of what I learned personally and what deepened my knowledge as a team.
The freee card system consists of a front end (TS, React), BFF (RoR), and back end (Go), and since the majority of the development is done in Go, this article makes many references to Go code. Looking at freee as a whole, many systems are developed using Rails, but some services are also developed using Go! *This is a complete aside, but I was happy to see his name as freee on the JobBoard of “Go Conference mini 2023 Winter IN KYOTO” the other day.
What you can get from this article
- Learn the points to keep in mind when developing with Go
- In general, you can understand the points to be careful about when developing backends (1st place, 2nd place, etc.)
- You can see how the freee card team is deepening their knowledge within the team.
Announcing the top 5!
So without further ado, I’d like to announce the top 5 things (in my opinion) that I learned personally and deepened my knowledge as a team!
No. 5: How to write error flow
PR review (*from actual review)
How about changing the error flow like this? The following may also be helpful. CodeReviewComments is Go’s official guideline, so it’s a good idea to read it through.
CodeReviewComments (Japanese) > Indent Error Flow
CodeReviewComments (original) > Indent Error Flow
My Thoughts
The person who submitted the PR was a new graduate member, and personally I think it was a very good review for the team because it conveyed the message that it would be a good idea to read the official guidelines. I ranked this as my No.5 because I thought it is a good idea to gain alignment while sharing a document, rather than ranking it based on the fact that it is easier to read (= subjective).
No.4: Consider the layers when adding logic
Background/requirements
We have implemented a new API that allows you to specify a period and retrieve data externally.The period (end date) is specified as a string in the YYYY-MM-DD format, but it is expected that it will include the end date (for example, if you receive a request for 2023-12-24, 12/24 23: 59:59 data was returned).
The freee card system is implemented using clean architecture, as introduced in [Series Part 4] Clean Architecture Practice with freee Card Unlimited .
Broadly speaking, you need to keep in mind which layer to implement it in: adapter (external adapter layer), domain, and use case.
PR review (*from actual review)
Code from PR review
Here is an example of some of the review comments:
IMO, it is the responsibility of the adapter layer to return the input exactly as it is input, and avoid adding addDate logic. I think this really requires domain knowledge, so I think it’s appropriate to put it in the domain layer.
The argument of the method here is time, and I think this will lead to incorrect behavior when more implementations call this function from other places later.
My Thoughts
Even if the guidelines for where to implement each layer have been decided, the team may not understand the specific implementation location. I feel that good communication is to use PR reviews to reconcile these points where perceptions tend to deviate. I believe that if the team understands what kind of problems are likely to occur due to different implementation layers, it will become a very strong team.
No.3: Technique to handle panic with interceptor
Background/requirements
When I experienced panic, there were times when the stack trace could not be taken properly and operation was difficult. It was necessary to wrap it into a unique error for error handling in the in-house library, so I did the following.
PR review (or rather, the PR itself)
My Thoughts
I added an implementation to include panic handling and stacktrace in errors as Go middleware. I learned that by implementing this type of processing as middleware in Go, everything will be covered without any missing parts when handling panic.
No.2: DeepModule should be based on the ASoSD book definition
There is a book called “A Philosophy of Software Design”, which is a very good book that provides insight into design guidelines ( page 21, which provides a quick understanding of “A Philosophy of Software Design” in 30 minutes, is very helpful). In the book, it mentioned that Module Should be deep.
My translation of Module Should be deep is : Modules should be “deep”. In other words, IF should be simple, but the internal implementation should be complex and include detailed behavior.
PR review (* from actual review)
A convenient “Condition” structure was added to be able to easily specify any retrieval condition .
PR review
The FindCards() method is used in multiple parts, but should it really be used as a standard method? What if we split the methods into different granularities needed for different use cases?As it is mentioned in the linked doc, Module Should be deep. This means, IF should be simple and the implementation should be deep, but in this case, I think the implementation approach does not align with that idea. https://speakerdeck.com/iwashi86/understand-roughly-philosophy-of-software-design-in-30-minutes?slide=21
My Thoughts
Even if a method seems good at first glance because it is standardized, we should not forget that Module Should be deep. I have found that it is a pattern that should be clearly avoided from this standpoint. In other words, I learned that it is better to have a method that is easy to understand and have a simple IF (avoiding complex arguments because it is too general like condition) rather than something that can be commonly referenced from multiple use cases. I learned a lot from a perspective that can be applied not only to this method but also to future design guidelines.
No. 1: Base method design on condensibility
Background/requirements
As part of the requirements for implementing external linkage processing, it was necessary to obtain records that are currently being processed in order to determine whether linkage has been completed and to determine whether authentication is complete.
PR review (* from actual review)
isAuthorized bool IMO Instead of branching internally on the arguments here , why not explicitly separate them into different methods? This is because functions with internal processing branches based on the value of an argument is considered to be a bad design because it has a low degree “logical cohesion.”
Reference: About cohesion
https://zenn.dev/portinc/articles/design-principle-cohesion
Comment from another member
FYI) Regarding “logical condensation”, this was also very easy to understand
My Thoughts
Like the “condensation” concept, the team was able to understand the importance of designing based on that idea. If it’s just a concept, you might think, “I guess I understand…?” and find it difficult to put it into practice, but I was able to use it as a guideline for making decisions based on actual examples, so it was a great learning experience for me.
Summary
These are the Top 5 lessons I learned from the freee card team’s development (Go).
- No.5: How to write error flow (read CodeReviewComments)
- No.4: Consider the layers when adding logic
- No.3: Technique to handle panic with interceptor
- No.2: DeepModule should be based on the ASoSD book definition
- No. 1: Base method design on condensibility
It’s been a year since I joined the team, and the freee card team is made up of very talented new graduates to mid-career members with a wealth of experience (each with different experience and strengths), and I’m learning a lot every day. I feel like this is a very exciting team. I have summarized the top 5 lessons I learned from this experience, which many were inspirational about being able to learn from those around me.
