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 code based on review #58

Closed
4 of 5 tasks
hbeni opened this issue Nov 30, 2020 · 5 comments · May be fixed by #84
Closed
4 of 5 tasks

Refactor code based on review #58

hbeni opened this issue Nov 30, 2020 · 5 comments · May be fixed by #84
Assignees
Labels
mumble-plugin Affecting mumble plugin
Milestone

Comments

@hbeni
Copy link
Owner

hbeni commented Nov 30, 2020

James Turner was so glad to provide some comments on the code:

https://sourceforge.net/p/flightgear/mailman/flightgear-devel/thread/3C2C3D58-6F58-4949-879E-261978FD24F0%40flightgear.org/#msg37165208

  • Use a class for global stuff
  • prefer <STL container>.empty() over <STL container>.size() > 0
  • make sure pluginDbg is efficient (noop when debug is not active)
  • mumble_onAudioSourceFetched is too long (break down into helper functions; rel Make minimum noise/audio filters depending on radio model #122)
  • use ‘const bool' locals to avoid complex if clauses
@hbeni hbeni added the mumble-plugin Affecting mumble plugin label Nov 30, 2020
@hbeni
Copy link
Owner Author

hbeni commented Dec 8, 2020

make sure pluginDbg is efficient (noop when debug is not active)

checked that, precompiler statements excluide any code when non-debug, so the function is a noop.

@hbeni hbeni added this to the Soon™ (=Future) milestone Dec 9, 2020
@hbeni hbeni linked a pull request Mar 8, 2021 that will close this issue
11 tasks
@hbeni hbeni modified the milestones: Soon™ (=Future), 1.0 Aug 28, 2021
@hbeni
Copy link
Owner Author

hbeni commented Aug 30, 2021

use ‘const bool' locals to avoid complex if clauses

Implemented already in current master, most of the part

@hbeni hbeni self-assigned this Aug 30, 2021
@hbeni
Copy link
Owner Author

hbeni commented Aug 30, 2021

prefer .empty() over .size() > 0

Implemented in audio refactor branch

hbeni added a commit that referenced this issue Aug 30, 2021
hbeni added a commit that referenced this issue Aug 30, 2021
The previously global audio filters are now part of the radio model.
Now we can define different audio filter characteristics for different radio models.
This also made the mumble_onAudioSourceFetched a little less long and a little more generic.

Fix #122
Helps with #58
@hbeni
Copy link
Owner Author

hbeni commented Aug 30, 2021

mumble_onAudioSourceFetched is too long (break down into helper functions; rel Make minimum noise/audio filters depending on radio model #122)

I consider this fixed by #122; The function is still very long and hard to read. The latest PR to restructure the audio processing into the radio model helped already to make it easier/shorter and removed some of the obscure if-branches.

@hbeni
Copy link
Owner Author

hbeni commented Aug 30, 2021

  • Use a class for global stuff

I skip this one on purpose. While I agree, it should be tackled some day, the more appropriate approach is surely the pending clean OO rewrite (#82 / #84).

@hbeni hbeni closed this as completed Aug 30, 2021
hbeni added a commit that referenced this issue Aug 30, 2021
The previously global audio filters are now part of the radio model.
Now we can define different audio filter characteristics for different radio models.
This also made the mumble_onAudioSourceFetched a little less long and a little more generic.

Fix #122
Helps with #58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
mumble-plugin Affecting mumble plugin
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant