Skip to content
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

refactor: extract each case in handle packet in messenger #2329

Merged

Conversation

Green-Sky
Copy link
Member

@Green-Sky Green-Sky commented Sep 18, 2022

alternative / continuation of #2328 using case: return to satisfy cimple


This change is Reviewable

@auto-add-label auto-add-label bot added the refactor Refactoring production code, eg. renaming a variable, not affecting semantics label Sep 18, 2022
@Green-Sky Green-Sky force-pushed the green_messenger_pkg_handle_switch2 branch 3 times, most recently from 7077d58 to 07fb5ff Compare September 18, 2022 16:13
@Green-Sky Green-Sky marked this pull request as ready for review September 18, 2022 16:22
Copy link

@Tha14 Tha14 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Functioning

@codecov
Copy link

codecov bot commented Sep 18, 2022

Codecov Report

Merging #2329 (d3819b2) into master (306693a) will increase coverage by 0.01%.
The diff coverage is 85.33%.

@@            Coverage Diff             @@
##           master    #2329      +/-   ##
==========================================
+ Coverage   78.91%   78.92%   +0.01%     
==========================================
  Files         127      127              
  Lines       24078    24109      +31     
==========================================
+ Hits        19002    19029      +27     
- Misses       5076     5080       +4     
Impacted Files Coverage Δ
toxcore/Messenger.c 82.30% <85.33%> (-0.47%) ⬇️
toxcore/TCP_client.c 83.37% <0.00%> (-0.71%) ⬇️
auto_tests/conference_test.c 99.49% <0.00%> (-0.51%) ⬇️
toxcore/group.c 84.35% <0.00%> (-0.14%) ⬇️
toxav/toxav.c 69.23% <0.00%> (+0.14%) ⬆️
toxcore/DHT.c 83.94% <0.00%> (+0.15%) ⬆️
toxcore/TCP_server.c 78.77% <0.00%> (+0.37%) ⬆️
toxav/groupav.c 74.58% <0.00%> (+0.66%) ⬆️
toxcore/friend_connection.c 83.99% <0.00%> (+1.16%) ⬆️
toxcore/ping.c 87.03% <0.00%> (+1.23%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@Green-Sky Green-Sky force-pushed the green_messenger_pkg_handle_switch2 branch 2 times, most recently from cda18b8 to 58e250a Compare September 18, 2022 23:28
Copy link
Member

@JFreegman JFreegman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 0 of 1 approvals obtained (waiting on @Green-Sky)


toxcore/Messenger.c line 1942 at r1 (raw file):

non_null(1, 3) nullable(5)
static int m_handle_packet_offline(Messenger *m, const int i, const uint8_t *data, const uint16_t data_length, void *userdata)

This function can be simplified:

Code snippet:

if (clause) { 
  do_thing();  
} 

return 0;

toxcore/Messenger.c line 2271 at r1 (raw file):

    switch (packet_id) {
        // TODO(Green-Sky): now all return 0 on error AND success, make errors errors?

This was the case to begin with, best to leave it as-is for now.


toxcore/Messenger.c line 2298 at r1 (raw file):

    }

    return m_handle_custom_lossless_packet(object, i, temp, len, userdata);

Should this go under a default case? Logically makes no difference but I think it might be cleaner.

Copy link
Member Author

@Green-Sky Green-Sky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 0 of 1 approvals obtained (waiting on @JFreegman)


toxcore/Messenger.c line 1942 at r1 (raw file):

Previously, JFreegman wrote…

This function can be simplified:

what is this packet for anyway?


toxcore/Messenger.c line 2271 at r1 (raw file):

Previously, JFreegman wrote…

This was the case to begin with, best to leave it as-is for now.

yes, that's why i put this TODO


toxcore/Messenger.c line 2298 at r1 (raw file):

Previously, JFreegman wrote…

Should this go under a default case? Logically makes no difference but I think it might be cleaner.

but then we cant have a final return 0; at the end of the function. imo it looks better like this, but that's personal preference.

Copy link
Member

@JFreegman JFreegman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: :shipit: complete! 1 of 1 approvals obtained (waiting on @Green-Sky)


toxcore/Messenger.c line 1942 at r1 (raw file):

Previously, Green-Sky (Erik Scholz) wrote…

what is this packet for anyway?

It's self-explanatory if you follow the call stack


toxcore/Messenger.c line 2298 at r1 (raw file):

Previously, Green-Sky (Erik Scholz) wrote…

but then we cant have a final return 0; at the end of the function. imo it looks better like this, but that's personal preference.

Yeah I suppose that's true.

@JFreegman JFreegman added this to the v0.2.19 milestone Sep 19, 2022
@Green-Sky Green-Sky force-pushed the green_messenger_pkg_handle_switch2 branch from 58e250a to 1a1de89 Compare September 19, 2022 18:07
Copy link
Member Author

@Green-Sky Green-Sky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: :shipit: complete! 1 of 1 approvals obtained (waiting on @Green-Sky and @JFreegman)


toxcore/Messenger.c line 2298 at r1 (raw file):

Previously, JFreegman wrote…

Yeah I suppose that's true.

Done.

@Green-Sky Green-Sky force-pushed the green_messenger_pkg_handle_switch2 branch from 1a1de89 to d3819b2 Compare September 19, 2022 18:44
@JFreegman JFreegman merged commit d3819b2 into TokTok:master Sep 20, 2022
@Green-Sky Green-Sky deleted the green_messenger_pkg_handle_switch2 branch September 20, 2022 20:35
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactor Refactoring production code, eg. renaming a variable, not affecting semantics
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants