Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
feat(nm): Backend implementation to support EAP-TLS + Minor WebUI fixes #4872
feat(nm): Backend implementation to support EAP-TLS + Minor WebUI fixes #4872
Changes from all commits
ed3fd45
6ecf82f
d2a8f4e
24c1ad6
0d13fdc
0528ef6
1ff935b
d8b1478
e3ea4fb
66bb8ed
930b711
0081908
a38a52f
457e659
8d52d36
e6c53e7
ab1d76c
6d51d78
6e963a2
023df59
9a3846f
04280ee
2a31c6d
5150b1e
820d627
c95320e
f97bdcd
419a67f
748d729
7579211
3ede2ee
a9ebe52
2ff3a7f
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
One final thought: this is a Side Effect Method.
which is a known anti-pattern/bad practice (see Uncle Bob Martin's "Clean Code", there's a dedicated section for that).
The correct way to handle this would be to return a modified version of the passed argument, but we already had the
decryptAndConvertPasswordProperties
that behaved the same way and therefore we're doing the same for consistency.Again: leave it as-is but be aware of the fact that this is not the best implementation.
Also notice that you've hidden the actual modification of the map down a couple of layer of abstractions (you actually change the values inside
findAndDecodeCertificatesForInterface
), so you've hidden the Side Effect even more than what was done indecryptAndConvertPasswordProperties
.Regarding why this is considered a bad practice... you experienced it first hand while chasing that bug caused by the modified map 😆
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.
You can leave the code as-is but consider using Early Returns in the future. I think the code is much easier to read: