This story is about pain, agony, and denial of ready-made solutions. It is also about changes that improve the code’s readability and help the development team stay happy. The object of this post is an interface that helps a program communicate with a database.
Disclaimer: If we would use various ORMs such as Gorm in this project, most probably we would not face this issue, yet, we decided to write our implementation, so this created the issue and therefore this post.
// IStorage represents database methods
type IStorage interface {
IfUserCanBeDeleted(ctx context.Context, userID int64) (bool, error)
CreateUser(ctx context.Context, u *model.User) (model.User, error)
CreateProfile(ctx context.Context, item *model.Profile) (*model.Profile, error)
DeleteUser(ctx context.Context, id int64) error
MarkUserAsDeleted(ctx context.Context, id int64, reason string) error
ListProfiles(ctx context.Context) ([]model.Profile, error)
UpdateProfile(ctx context.Context, item *model.Profile) error
GetUserByID(ctx context.Context, id int64) (model.User, error)
GetPinCodeByUserID(ctx context.Context, id int64) (string, error)
GetShortUserByID(ctx context.Context, id int64) (model.ShortUser, error)
UpdateProfilePersonID(ctx context.Context, accID, personID string) error
GetUserProfileByID(ctx context.Context, id int64) (*model.Profile, error)
GetUserProfileByStripeAccountID(ctx context.Context, stripeAccountID string) (*model.Profile, error)
GetUserProfileByStripeCustomerID(ctx context.Context, stripeCustomerID string) (*model.Profile, error)
GetUserIDByStripeAccount(ctx context.Context, stripeAccount string) (int64, error)
GetAgreementIDByPaymentIntentID(ctx context.Context, paymentIntentID string) (int64, error)
GetUserByPhone(ctx context.Context, phone string) (model.User, error)
CheckUserExists(ctx context.Context, phone string) (bool, error)
CheckUserExistsDeprecated(ctx context.Context, phones []string) (bool, error)
CreateAgreement(ctx context.Context, a *model.Agreement) (model.Agreement, error)
GetAgreementByPaymentIntentID(ctx context.Context, paymentIntentID string) (model.Agreement, error)
GetAgreementByResolutionServicesPaymentIntentID(ctx context.Context, paymentIntentClientSecret string) (model.Agreement, error)
UpdateAgreementResolutionServicePI(ctx context.Context, id int64, piClientSecret string) error
DeleteAgreement(ctx context.Context, id int64) error
GetPaidAgreementsForBuyer(ctx context.Context, buyerID int64) ([]model.Agreement, error)
UpdateAgreementStatus(ctx context.Context, a *model.Agreement) error
GetAgreementByID(ctx context.Context, id int64) (model.Agreement, error)
GetUserAgreements(ctx context.Context, filter *model.AgreementFilter) ([]model.Agreement, error)
GetAgreementMilestones(ctx context.Context, agreementID int64) ([]model.Milestone, error)
ConfirmAgreement(ctx context.Context, agreement model.Agreement) error
RejectChangesOnAgreement(ctx context.Context, a *model.Agreement) error
AcceptChangesOnAgreement(ctx context.Context, initialAgreement *model.Agreement, newAgreement model.Agreement, updatedMstList, deletedMstList, newAddedMstList []model.Milestone) error
UpdateMilestone(ctx context.Context, m model.Milestone) (model.Milestone, error)
UpdateAgreement(ctx context.Context, a *model.Agreement, updatedMstList, deletedMstList, newAddedMstList []model.Milestone) error
ContactsList(ctx context.Context, id int64) (model.ContactListForUser, error)
ContactsCreate(ctx context.Context, contact model.ContactPairShort) error
ContactsDelete(ctx context.Context, userID int64) error
ContactsListFriends(ctx context.Context, id int64) (model.ContactListForUser, error)
GetContactShort(ctx context.Context, id1, id2 int64) (model.ContactPairShort, error)
GetContactShortByChatID(ctx context.Context, chatID string) (model.ContactPairShort, error)
GetContactFullByChatID(ctx context.Context, chatID string) (model.ContactPairFull, error)
GetContactPartialInfoSecond(ctx context.Context, id1, id2 int64) (model.ContactPartialInfoSecond, error)
GetContactFull(ctx context.Context, id1, id2 int64) (model.ContactPairFull, error)
ContactsCreateAddRequest(ctx context.Context, idFrom, idTo int64) (model.ContactPairShort, error)
ContactsUpdateLastMessageTime(ctx context.Context, contact *model.ContactPairShort) error
ContactExists(ctx context.Context, id1, id2 int64) (bool, error)
GetUsersReferralCode(ctx context.Context, userID int64) (string, error)
ContactsUpdate(ctx context.Context, contact model.ContactPairShort) error
GetMilestoneByID(ctx context.Context, id int64) (model.Milestone, error)
UpdateMilestoneStatus(ctx context.Context, m *model.Milestone) error
GetShortAgreement(ctx context.Context, id int64) (model.Agreement, error)
UpdateUser(ctx context.Context, u *model.User) error
UpdateUserPinCode(ctx context.Context, u *model.User) error
SaveConfirmationCode(ctx context.Context, c model.ConfirmationCode) error
GetConfirmationCodeInfo(ctx context.Context, phone string) (model.ConfirmationCode, error)
AgreementMilestonesClosed(ctx context.Context, agreementID int64) (bool, error)
CreateUserDeviceToken(ctx context.Context, dt *model.UserDeviceToken) error
RemoveUserToken(ctx context.Context, userID int64) error
GetNotification(ctx context.Context, id int64) (model.Notification, error)
ListNotification(ctx context.Context, userID int64) ([]model.Notification, error)
CreateNotification(ctx context.Context, n *model.Notification) (model.Notification, error)
GetUserFCMToken(ctx context.Context, userID int64) (string, error)
SubmitAgreementForInternalResolution(ctx context.Context, id int64, piClientSecret string) error
SetAgreementResolution(ctx context.Context, id int64, resolution string) error
DenyAgreement(ctx context.Context, a model.Agreement) error
GetAgreementOfMilestone(ctx context.Context, milestoneID int64) (model.Agreement, error)
SoftDeleteMilestone(ctx context.Context, agreementID, milestoneID int64) error
SetMilestoneToDelete(ctx context.Context, agreementID, milestoneID int64) error
SetMilestoneToUpdate(ctx context.Context, milestoneID int64) error
ConfirmPhone(ctx context.Context, phone string) error
IsPhoneVerified(ctx context.Context, phone string) (bool, error)
RemoveConfirmationCode(ctx context.Context, phone string) error
GetAllShortUsers(ctx context.Context) ([]model.ShortUser, error)
SetNotificationAsRead(ctx context.Context, id int64) error
GetUnreadNotificationsCount(ctx context.Context, receiverID int64) (int, error)
ResetUnreadNotifications(ctx context.Context, receiverID int64) error
SaveWebhookEvent(ctx context.Context, item *model.WebhookEvent) (model.WebhookEvent, error)
UpdateWebhookEvent(ctx context.Context, item *model.WebhookEvent) error
FindWebhookEventBy(ctx context.Context, source, externalID string) (model.WebhookEvent, error)
GetFullAgreementByID(ctx context.Context, id, tokenUserID int64) (model.Agreement, error)
ReferralGetBonusByUserID(ctx context.Context, userID int64) (int64, error)
ReferralAddBonusToUsersBonuses(ctx context.Context, userID, bonus int64) error
ReferralExists(ctx context.Context, refCode string) (bool, error)
GetReferral(ctx context.Context, code string) (model.Referral, error)
AddReferralProgram(ctx context.Context, rp model.ReferralProgram) error
DeleteReferralPrograms(ctx context.Context) error
GetReferralProgram(ctx context.Context, code string) (*model.ReferralProgram, error)
ListReferralPrograms(ctx context.Context) ([]model.ReferralProgram, error)
ListAgreementsApplicableForReferral(ctx context.Context, code string) ([]model.Agreement, error)
ReferralUserAlreadyUsedRefferalCode(ctx context.Context, userID int64) (bool, error)
IfMilestoneWasUsedInReferral(ctx context.Context, milestoneID int64, code string) (bool, error)
MarkMilestoneAsUsedInReferral(ctx context.Context, milestoneID int64, code string) error
GetReferralByUserID(ctx context.Context, userID int64) (model.Referral, error)
CreateMilestoneChanges(ctx context.Context, milestone *model.Milestone) error
GetMilestoneChanges(ctx context.Context, milestoneID int64) (*model.Milestone, error)
DeleteMilestoneChanges(ctx context.Context, milestoneID int64) error
ReferralGetUserIDByCode(ctx context.Context, code string) (int64, error)
CreatePayout(ctx context.Context, item model.Payout) (model.Payout, error)
DeletePayout(ctx context.Context, id int64) error
ListPayouts(ctx context.Context, userID int64) ([]model.Payout, error)
ListPayoutsWithStatus(ctx context.Context, status model.PayoutStatus) ([]model.Payout, error)
UpdatePayoutStripeID(ctx context.Context, item model.Payout) error
UpdatePayoutStatus(ctx context.Context, item model.Payout) (model.Payout, error)
GetPayoutByMilestoneID(ctx context.Context, milestoneID int64) (model.Payout, error)
GetPayoutByStripeID(ctx context.Context, payout string) (model.Payout, error)
CreateFeedback(ctx context.Context, f *feedback.Feedback) (*feedback.Feedback, error)
GetUserIDByEmail(ctx context.Context, email string) (int64, error)
CreatePlatformTransfer(ctx context.Context, item model.PlatformTransfer) (model.PlatformTransfer, error)
ListAgreementPlatformTransfers(ctx context.Context, agreementID int64) ([]model.PlatformTransfer, error)
DisputeAgreement(ctx context.Context, agreementID int64, milestoneIDs []int64) error
CreateBlockedUser(ctx context.Context, item model.BlockedUser) (model.BlockedUser, error)
CheckIfUserIsBlocked(ctx context.Context, userID int64) (bool, error)
UnblockUser(ctx context.Context, stripeAccountID string) error
CreateUserRefreshToken(ctx context.Context, rt model.UserRefreshToken) error
GetUserRefreshToken(ctx context.Context, userID int64) (string, error)
UpdateUserRefreshToken(ctx context.Context, rt model.UserRefreshToken) error
UpdateUserDeviceToken(ctx context.Context, dt model.UserDeviceToken) error
GetConfirmedMilestones(ctx context.Context) ([]model.Milestone, error)
CollaboratorsGetAllPhonesWithContactStatus(ctx context.Context, callerID int64) ([]model.PhoneWithContactStatus, error)
SetShowRejectedChangesFalse(ctx context.Context, changesID int64) error
CreateAgreementChanges(ctx context.Context, item model.ChangesOnAgreement) (model.ChangesOnAgreement, error)
GetLatestAgreementChanges(ctx context.Context, agreementID int64) (model.ChangesOnAgreement, error)
DeleteAgreementChanges(ctx context.Context, id int64) error
DeleteAgreementChangesByAgreementID(ctx context.Context, id int64) error
CancelAgreementChanges(ctx context.Context, agreement model.Agreement, changesID int64) error
GetAgreementChangesByID(ctx context.Context, id int64) (model.ChangesOnAgreement, error)
SetChangesViewed(ctx context.Context, changesOnAgrID int64) error
CheckIfAgreementHasPendingChanges(ctx context.Context, agreementID int64) (bool, error)
GetDataOfMilestoneToInspect(ctx context.Context, previousRunTime time.Time) ([]model.MilestoneToInpect, error)
CreateMilestone(ctx context.Context, m *model.Milestone) (model.Milestone, error)
SetLatestLoggedUserDevice(ctx context.Context, u model.LatestLoggedUserDevice) (model.LatestLoggedUserDevice, error)
GetLatestLoggedUserDevice(ctx context.Context, userID int64) (model.LatestLoggedUserDevice, error)
}
The problem with this interface was its size — 130+ methods in one single interface! That’s a lot of methods and that is not what SOLID interface should look like. And since we develop in Go, we have to know (and follow) one of the Go proverbs which are:
The bigger the interface, the weaker the abstraction.
© Rob Pike
The further we developed the project, the heavier this interface grew and soon it became clear that to continue the development with fewer bugs, less time spent understanding the code, and more comfort, this interface should be refactored. We as a team could not use this interface with flexibility. We could not tell from the first glance what it does since it does everything. And that forced me to start its refactoring. Which is what I want to share with you.
Step 0: Cover Code (API) with Tests before any Refactoring
This is crucial, since dealing with interfaces, abstractions, and refactoring without tests that cover API logic makes no good. I would say that it can do exactly opposite — bring a lot of problems to your code since every change you make to the interface will result in 20+ files being changed. And if you do not have solid tests, there is a high chance you break something or create bugs. Please, be cautious!
Step 1: Imagine What the Result Would Look Like
I decided to concentrate on common aggregates that my project deals with. After some time of thinking and looking through the entire list of functions, I outlined how the future interface would look like and this is what I came up with:
// IStorage represents database methods
type IStorage interface {
User() User
Profile() Profile
Agreement() Agreement
AgreementChanges() AgreementChanges
Milestone() Milestone
Contact() Contact
Notification() Notification
Payout() Payout
WebhookEvent() WebhookEvent
Referral() Referral
VerificationCode() VerificationCode
Feedback() Feedback
PlatformTransfer() PlatformTransfer
}
This would allow writing the following code instead of just reference one of 130+ methods from the interface:
user, err := api.Storage.GetUserByID(ctx, userID)
err := api.Storage.DenyAgreement(ctx, agreement)
err := api.Storage.UpdateUserDeviceToken(ctx, model.UserDeviceToken{...})
After refactoring:
user, err := api.Storage.User().Get(ctx, userID)
err := api.Storage.Agreement().Deny(ctx, agreement)
err := api.Storage.User().UpdateDeviceToken(ctx, model.UserDeviceToken{...})
As you can see, this interface consists of multiple smaller interfaces each based on certain aggregates (users, agreements, etc.) and doing something with that aggregate. Reading such constructions is a much better experience and at the same time, it is more convenient since if you ever need to do anything with a user, you know where to search for the right methods and consider if they even exist.
Step 2: Implement Changes Step by Step
Having interface with 130+ methods makes it very complicated to do refactor in “once and for all” style. There are so many changes that turn every merge request into 50+ files changes. So, the next step, therefore, should be breaking the interface step by step, one aggregate after another and committing those changes often to make small, understandable merge request and making sure everything still works as expected (remember step 0!) For this I first break down methods into small portions.
// IStorage represents database methods
type IStorage interface {
// User() User
// Profile() Profile
// Agreement() Agreement
// AgreementChanges() AgreementChange
// Milestone() Milestone
// Contact() Contact
// Notifications() Notification
// Payout() Payout
// WebhookEvents() WebhookEvent
// Referrals() Referral
// VerificationCodes() VerificationCode
// Feedback() Feedback
// PlatformTransfe() PlatformTransfer
// this is just a temporary change not to break all the functionality. Will reimplment this
// for the version mentioned above as a next step with just a small chucks under improvements
User
Profile
Agreement
AgreementChange
Milestone
Contact
Notification
Payout
WebhookEvent
Referral
VerificationCode
Feedback
PlatformTransfer
}
// Notification inferface contains methods for notifications manipulation
type Notification interface {
CreateNotification(ctx context.Context, n *model.Notification) (model.Notification, error)
GetNotification(ctx context.Context, id int64) (model.Notification, error)
GetUnreadNotificationsCount(ctx context.Context, receiverID int64) (int, error)
ListNotification(ctx context.Context, userID int64) ([]model.Notification, error)
SetNotificationAsRead(ctx context.Context, id int64) error
ResetUnreadNotifications(ctx context.Context, receiverID int64) error
}
So, I created several sub-interfaces and stated that my IStorage interface implements them all. That did not change the code much but laid an important preparatory brick to what I wanted to do next, which is essentially replace my sub-interfaces with separate interfaces with their separate methods in CRUD fashion, adding missing methods & uniting those which are of the same nature. I added new structs as the implementation of those interfaces and replaced old methods with a new one with VSCode search & replace all features.
// IStorage represents database methods
type IStorage interface {
Feedback() Feedback
// this is just a temporary change not to break all the functionality. Will reimplment this
// for the version mentioned above as a next step with just a small chucks under improvements
User
Profile
Agreement
AgreementChange
Milestone
Contact
Notification
Payout
WebhookEvent
Referral
VerificationCode
PlatformTransfer
}
func (s *Storage) Feedback() Feedback {
return &FeedbackClient{cfg: s.cfg, logger: s.logger, pool: s.pool}
}
// Feedback inferface contains methods for feedback manipulation
type Feedback interface {
Create(ctx context.Context, f *feedback.Feedback) (*feedback.Feedback, error)
}
type FeedbackClient struct {
cfg config.Config
logger *logrus.Entry
pool *pgxpool.Pool
}
As a result, I have changed all IStorage sub-interfaces to its interfaces and made all functions more intuitive & understandable. Yes, it took me a while to do that, yet, the result is worth it.
Step 3: Clean up
Bulk edits with find -> replace all helps save time, but it also creates some side effects where you can rename not relevant function or the logs messages might get a bit silly and hard to understand. This what happened to me after refactoring.
err = api.Storage.Profile().Update(ctx, user)
if err != nil {
api.Logger.WithFields(logrus.Fields{"err": err}).Error("Update failed")
}
When this gets logged we get the message “Update failed” which might not be so clear to us at first glance. Yes, there is a “handler” param that indicates where exactly it happened, but this might not be enough to determine the exact spot. So, my advice here would be to go ahead and review the project’s log messages and other components that might get affected by the refactoring.
Side Effects from Refactoring
During this process, it became clear that we use a lot of different methods for the same thing (like updating items in various fields in various methods). This creates code smell and in the future, we would avoid that anti-pattern.
Also, I have found some functions that were not “fit” in certain places. For example when agreements were highly dependent on referrals in the update. That made no sense after I begin refactoring. Having this “God” interface allowed code to use it, but refactoring showed how terrible it was and made living with this code impossible. I have to re-write some functions to create more natural, intuitive functionality.
Yes, we have to write some more code, yet, this approach gives us way more to code’s readability, maintainability, and simple design. I would do it after all.
Conclusion
If you have a project with a very heavy database interface and you see that it continues to grow, consider refactoring as soon as possible since it would just get worse.
When refactoring, always check tests first, then decide what the end solution should look like and do partial updates, one component at a time. This will keep your project alive, will decrease chances of introducing new bugs, will keep code reviewers from anger, and keep you happy about the progress.
After refactoring large interfaces always check for ‘side effects” basically.
If you ever face the same issue, please share your solutions in the comments, since I am really curious about what else I might have done. Also, please share your thoughts on what I could (and still can) do to make it even better. Any feedback is more than welcomed!
Previosly published at maddevs.io.
Top comments (1)
I've done a lot of refactoring of huge projects. Your tip of having tests ready before you start is spot on. Best advice to anyone about to go through the process. Unit tests are the best way to validate what you're changing produces the same results. For me the time it takes to write the tests more than make up for the time saved refactoring simply because I can have confidence in my work as I go through the process.