[native] Clean up lines in `retryMultimediaMessage`
ClosedPublic
Actions

Authored by • abosh on Aug 18 2022, 1:16 PM.

Details

Reviewers

atul

Commits

rCOMM2cc525b67b96: [native] Clean up lines in `retryMultimediaMessage`

Summary

More fixes I saw while reading this for video work.

Test Plan

For the nullish coalescing operator, it's the same reasoning as D4873: pendingUploads is an object, so !pendingUploads will only be true when pendingUploads is null or undefined. Thus the nullish coalescing operator covers these cases.

The removal of the else is because of no else after return.

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

• abosh created this revision.Aug 18 2022, 1:16 PM

Herald added subscribers: atul, tomek, ashoat. · View Herald TranscriptAug 18 2022, 1:16 PM

• abosh added a reviewer: atul.Aug 18 2022, 1:16 PM

• abosh edited the test plan for this revision. (Show Details)

Harbormaster completed remote builds in B11453: Diff 15759.Aug 18 2022, 1:26 PM

• abosh requested review of this revision.Aug 18 2022, 1:26 PM

I'm also reasoning through InputStateContainer at the moment so understand and appreciate the initiative to try to refactor things!

That said, trying to clean up all of these little stylistic syntax things could be a slippery slope. Even when the changes are simple there's still overhead in terms of putting up a diff, writing out Summary/Test Plan, having it reviewed, etc.

I think we should try to focus our refactoring efforts on changes that help improve comprehension. That could be breaking down chunks of logic into separate functions, renaming variables/functions to be more descriptive, making minor architecture changes to make things more intuitive, etc.

The issue you pointed out with failed is a great candidate for improving comprehension via refactoring:

Like failed being a string instead of being a boolean makes little sense. Because if !failed is true (so failed is null basically), that means an upload is in progress, and if failed is not null or undefined (so failed is truthy), that means an upload is NOT in progress. Like what, shouldn't failed being truthy mean an upload failed, since failed being truthy kind of sounds like something failed. just make it a boolean! Then if failed is true, that means the upload failed

That said, if you notice these little syntax things as you're working and it's minimal effort to just quickly put up a diff, feel free to keep putting them up! I think it's definitely an improvement, just don't want us to get too carried away with trying to solve all of these minor style things.

This revision is now accepted and ready to land.Aug 19 2022, 9:54 AM

In D4877#141648, @atul wrote:

I'm also reasoning through InputStateContainer at the moment so understand and appreciate the initiative to try to refactor things!

That said, trying to clean up all of these little stylistic syntax things could be a slippery slope. Even when the changes are simple there's still overhead in terms of putting up a diff, writing out Summary/Test Plan, having it reviewed, etc.

I think we should try to focus our refactoring efforts on changes that help improve comprehension. That could be breaking down chunks of logic into separate functions, renaming variables/functions to be more descriptive, making minor architecture changes to make things more intuitive, etc.

The issue you pointed out with failed is a great candidate for improving comprehension via refactoring:

Like failed being a string instead of being a boolean makes little sense. Because if !failed is true (so failed is null basically), that means an upload is in progress, and if failed is not null or undefined (so failed is truthy), that means an upload is NOT in progress. Like what, shouldn't failed being truthy mean an upload failed, since failed being truthy kind of sounds like something failed. just make it a boolean! Then if failed is true, that means the upload failed

That said, if you notice these little syntax things as you're working and it's minimal effort to just quickly put up a diff, feel free to keep putting them up! I think it's definitely an improvement, just don't want us to get too carried away with trying to solve all of these minor style things.