Refactor FXIOS-7301 - Remove 2 closure_body_length violations from MainMenuMiddleware.swift (Firefox) and InternalTelemetrySettingsView.swift (Focus)

[ionixjunior]

📜 Tickets

Jira ticket
Github issue

💡 Description

This PR removes 2 closure_body_length violations from the MainMenuMiddleware.swift (Firefox) and InternalTelemetrySettingsView.swift (Focus). Also, it decreases the threshold to 42.

The most controversial change in this PR is in the MainMenuMiddleware.swift file. In this file, I changed the mainMenuProvider property, splitting it into separated functions. I created three new functions, each one based on the ActionType: MainMenuActionType, GeneralBrowserActionType, and MainMenuDetailsActionType. To the last one, I needed to add the @unknown keyword, because all option already available into switch case statement.

I've changed the strategy, so I just discard the paragraph above, and I made a comment bellow explaining this.

Please, feel free to give me suggestions about how can we improve this.

This PR is part of a series of PRs described here #16442 (comment) and here #16442 (comment).

📝 Checklist

You have to check all boxes before merging

• Filled in the above information (tickets numbers and description of your work)
• Updated the PR name to follow our PR naming guidelines
• Wrote unit tests and/or ensured the tests suite is passing
• When working on UI, I checked and implemented accessibility (minimum Dynamic Text and VoiceOver)
• If needed, I updated documentation / comments for complex code and public methods
• If needed, added a backport comment (example @Mergifyio backport release/v120)

[FilippoZazzeroni]

Just a nit refactor for MainMenuMiddleware the rest looks good to me

[FilippoZazzeroni]

Could we change this to instead of checking if the method handles the action, to check for the action type, like we did in other cases.

if let action = action as? GeneralBrowserAction {
 handleGeneralBrowserAction()
}

With this we can avoid also returning a boolean to check if the action was handled. What do you think @ionixjunior let me know if it makes sense what i said, thanks 😃

[ionixjunior]

Okay, I totally agree. Thank you for point this! I've made some small changes. I added to the methods the action type parameter to avoid another type casting inside each function. Let me know what do you think about it.

The branch was rebased and updated.

[FilippoZazzeroni]

hi @ionixjunior i like it, but i'd it has a slight problem that ActionType is not linked with Action so i'd do with other files like the TabManagerMiddleware where we check the Action directly not the action type. Also i'd prefer to do

if let acti = ...

else if let {
}

if else statements instead of only if since we don't want multiple action to accidentally be resolved from closure.

[FilippoZazzeroni]

do we need @unknow at this point ?

[ionixjunior]

You're right. I can remove it, but also I need to remove the default statement, because the switch statement defines all the enum possibilities, and I cannot have some code that will never be executed. The default case statement will never be reached, so the compiler will generate an error if it is kept in the code.

What's the best practice here? Keep the default statement with the @unknown keyword or just remove the entire default statement?

[FilippoZazzeroni]

hi @ionixjunior i like it, but i'd it has a slight problem that ActionType is not linked with Action so i'd do with other files like the TabManagerMiddleware where we check the Action directly not the action type. Also i'd prefer to do

if let acti = ...

else if let {
}

if else statements instead of only if since we don't want multiple action to accidentally be resolved from closure.

[ionixjunior]

@FilippoZazzeroni I understand your point about the slight problem that ActionType is not linked with Action, and I liked the example of TabManagerMiddleware, but I believe we need to think more about in the MainMenuMiddleware class.

To find the correct type and call the new small functions, it will be necessary to look at the ActionType, because on mainMenuProvider property, the action variable is from type MainMenuAction. See the guard let at the line 54. So, if we check only the type of action, we can't understand what small function to call. Is this makes sense?

Maybe my approach was not so good, trying to split this function based on the action type. I'll think how can we deal with this, but let me know what do you think.

About the else if statement, it makes sense to me.

[FilippoZazzeroni]

Sorry for answering late @ionixjunior i was out of office yesterday. Anyway yes i agree, if we know already that we are going to have one action and different action type, then we can check for the actionType directly.

[ionixjunior]

No problem at all @FilippoZazzeroni. This week I'll work on it again. Sorry for answering late too.

[FilippoZazzeroni]

no problem @ionixjunior there is no rush

[ionixjunior]

@FilippoZazzeroni I had an idea. I just extract the entire closure block to a new function and it solved the problem. I was afraid about another rule, the function body length, but I looked at the documentation, and I verifyed the default value for it is 50 https://realm.github.io/SwiftLint/function_body_length.html. So, the new function I've created has 41 lines and everthing will be okay with this another rule too.

Let me know what do you think about it. The branch was rebased and updated.

[FilippoZazzeroni]

Great work @ionixjunior 🎉 🎉 just a reminder that if we are going to apply the same procedure for function_body_length than we will need to refactor also this method.

[mobiletest-ci-bot]

	Messages
📖	Project coverage: 36.4%
📖	Edited 3 files
📖	Created 0 files

Client.app: Coverage: 35.06

File	Coverage
MainMenuMiddleware.swift	32.1%	⚠️

Generated by 🚫 Danger Swift against 28d2b1c

[ionixjunior]

@FilippoZazzeroni yes, absolutely.