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.

Sign up for GitHub

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

Jump to bottom

Unused code report #2308

Closed
incendial opened this issue Jan 30, 2023 · 5 comments
Closed

Unused code report #2308

incendial opened this issue Jan 30, 2023 · 5 comments

Comments

@incendial
Copy link

incendial commented Jan 30, 2023 •
edited
Loading

Hi! I'm testing new check-unused-code command implementation that can find separate methods, fields, etc.

In this issue I'll list all the reported files and then we can decide how (if should) they will be removed (by me, by you, etc).

If you're not interested, feel free to close this issue 🙂.

Report output for packages/flame/ folder:

✔ Analysis is completed. Preparing the results: 16.3s

packages/flame/test/collisions/collision_test_helpers.dart:
    ⚠ unused method intersections
      at flame/test/collisions/collision_test_helpers.dart:128:3

packages/flame/test/components/component_test.dart:
    ⚠ unused field name
      at flame/test/components/component_test.dart:1041:3
    ⚠ unused field name
      at flame/test/components/component_test.dart:1167:3

packages/flame/test/events/game_mixins/multi_touch_tap_detector_test.dart:
    ⚠ unused field tapTimes
      at flame/test/events/game_mixins/multi_touch_tap_detector_test.dart:91:3

packages/flame/test/fixtures/fixture_reader.dart:
    ⚠ unused function fixtureForString
      at flame/test/fixtures/fixture_reader.dart:11:1

Report output for packages/flame_bloc/ folder:

✔ Analysis is completed. Preparing the results: 3.6s

packages/flame_bloc/example/lib/src/game_stats/bloc/game_stats_event.dart:
    ⚠ unused constructor ScoreEventCleared
      at flame_bloc/example/lib/src/game_stats/bloc/game_stats_event.dart:17:3

packages/flame_bloc/test/inventory_cubit.dart:
    ⚠ unused method selectSword
      at flame_bloc/test/inventory_cubit.dart:15:3

packages/flame_bloc/test/player_cubit.dart:
    ⚠ unused method riseFromTheDead
      at flame_bloc/test/player_cubit.dart:16:3

Report output for packages/flame_jenny/jenny/ folder:

✔ Analysis is completed. Preparing the results: 2.5s

packages/flame_jenny/jenny/lib/src/parse/parse.dart:
    ⚠ unused type alias FunctionBuilder
      at flame_jenny/jenny/lib/src/parse/parse.dart:968:1

✖ total unused code (classes, functions, variables, extensions, enums, mixins and type aliases) - 1
@incendial incendial changed the title [draft] Unused code report Unused code report Jan 30, 2023
@incendial
Copy link
Author

incendial commented Jan 30, 2023 •
edited
Loading

That's it! I'm truly impressed, considering the amount of code you have 🚀

@spydon
Copy link
Member

spydon commented Jan 30, 2023

Good stuff, all the extra test helper fields can be removed at least I think.

For flame_bloc I think @erickzanardo or @wolfenrain for example would have to check and for Jenny I'm guessing that it should stay, but @st-pasha can hopefully verify that.

@erickzanardo
Copy link
Member

Here is the reason why I love gamedev, The method riseFromTheDead is never used hahah

I will try get to the flame bloc part this week :D

@st-pasha st-pasha mentioned this issue Feb 2, 2023
Merged
2 tasks
This was referenced Mar 1, 2023
Merged
Merged
Merged
Merged
Merged
spydon pushed a commit that referenced this issue Mar 1, 2023
@luanpotter
refactor: Remove unused variable "tapTimes" from multi_touch_tap_dete…
cd2b2a1 
…ctor_test.dart (#2379)

This is a cleanup identified on this issue: #2308
Using an amazing unused-code tooling
Now, since Flame is a public API, unused code might not be trivial - it might just mean untested code.

This one was a no-brainer -- the variable was clearly replaced by specific counters during the writing of the test and forgotten there:

  int nOnTapDown = 0;
  int nOnLongTapDown = 0;
  int nOnTapUp = 0;
  int nOnTap = 0;
  int nOnTapCancel = 0;

It wasn't even increment and the taps are already thoroughly tested more granularly
spydon pushed a commit that referenced this issue Mar 1, 2023
@luanpotter
test: Add test for the intersections test helper method (#2378)
79db971 
This is a cleanup identified on this issue: #2308
Using an amazing unused-code tooling
Now, since Flame is a public API, unused code might not be trivial - it might just mean untested code.

In this case, we have a test helper, which is a public function we expose to help people write better tests.
Which means that the function could be useful, it was just lacking a test -- so I added it.
That being said, if this method is deemed unnecessary for users, I can just remove it instead - lmk.
spydon pushed a commit that referenced this issue Mar 1, 2023
@luanpotter
refactor: Use variable name on toString in component_test.dart (#2377)
f5c0e5e 
This is a cleanup identified on this issue: #2308
Using an amazing unused-code tooling
Now, since Flame is a public API, unused code might not be trivial - it might just mean untested code.

In this case, the unused code is in the tests, which likely means it is indeed necessary.
I tried to infer the intent of the author here, and even considered adding the names of the components to the events, but the events are already always per component, so that was meaningless. I also thought it might be a missing toString, that they would wish to show the component name for debugging purposes, but I didn't see any asserts/logs on the component objects themselves (just the events).
So I concluded there is no reason to keep this field, but lmk if anyone disagrees and I can bring it back and add some function to it.
luanpotter added a commit that referenced this issue Mar 1, 2023
@luanpotter @spydon
refactor: Remove unused event "ScoreEventCleared" from flame_block ex…
a9db3f4 
…ample (#2380)

# Description

This is a cleanup identified on this issue:
#2308
Using an amazing unused-code tooling
Now, since Flame is a public API, unused code might not be trivial - it
might just mean untested code.

Since this an example package, it definitely should never have unused
code.
In this case, the constructor wasn't being used, but upon deeper
investigation, that was because this particular event wasn't ever fired
at all.
So I removed it entirely. If this was supposed to be fired somewhere by
the example, please lmk and I can add the missing firing.
But as far as I can see, the event serves no purpose in this example.

## Checklist

- [x] I have followed the [Contributor Guide] when preparing my PR.
- [x] I have updated/added tests for ALL new/updated/fixed
functionality.
- [x] I have updated/added relevant documentation in `docs` and added
dartdoc comments with `///`.
- [x] I have updated/added relevant examples in `examples` or `docs`.

## Breaking Change?

- [ ] Yes, this PR is a breaking change.
- [x] No, this PR is not a breaking change.

<!-- Links -->
[Contributor Guide]:
https://github.com/flame-engine/flame/blob/main/CONTRIBUTING.md
[Conventional Commit]: https://conventionalcommits.org/
[CHANGELOG]:
https://github.com/flame-engine/flame/blob/main/CHANGELOG.md

Co-authored-by: Lukas Klingsbo <me@lukas.fyi>
spydon pushed a commit that referenced this issue Mar 2, 2023
@luanpotter
test: Add test for two subsequent state changes to flame_multi_bloc_p…
a0fff09 
…rovider_test.dart (#2381)

This is a cleanup identified on this issue: #2308
Using an amazing unused-code tooling
Now, since Flame is a public API, unused code might not be trivial - it might just mean untested code.

In this case, it is a test file, so there should definitely be no unused code.
However analyzing the unused code revealed to me some intent of testing multiple subsequent state changes (dead -> raise from dead).
I believe such test was missing entirely, so I added it. I think it holds value, but lmk if you disagree I can just delete the test and the method.
st-pasha pushed a commit that referenced this issue Mar 2, 2023
@luanpotter @st-pasha
refactor: Remove unused variable "tapTimes" from multi_touch_tap_dete…
34a1fd5 
…ctor_test.dart (#2379)

This is a cleanup identified on this issue: #2308
Using an amazing unused-code tooling
Now, since Flame is a public API, unused code might not be trivial - it might just mean untested code.

This one was a no-brainer -- the variable was clearly replaced by specific counters during the writing of the test and forgotten there:

  int nOnTapDown = 0;
  int nOnLongTapDown = 0;
  int nOnTapUp = 0;
  int nOnTap = 0;
  int nOnTapCancel = 0;

It wasn't even increment and the taps are already thoroughly tested more granularly
st-pasha pushed a commit that referenced this issue Mar 2, 2023
@luanpotter @st-pasha
test: Add test for the intersections test helper method (#2378)
62e45c7 
This is a cleanup identified on this issue: #2308
Using an amazing unused-code tooling
Now, since Flame is a public API, unused code might not be trivial - it might just mean untested code.

In this case, we have a test helper, which is a public function we expose to help people write better tests.
Which means that the function could be useful, it was just lacking a test -- so I added it.
That being said, if this method is deemed unnecessary for users, I can just remove it instead - lmk.
st-pasha pushed a commit that referenced this issue Mar 2, 2023
@luanpotter @st-pasha
refactor: Use variable name on toString in component_test.dart (#2377)
da5ac3b 
This is a cleanup identified on this issue: #2308
Using an amazing unused-code tooling
Now, since Flame is a public API, unused code might not be trivial - it might just mean untested code.

In this case, the unused code is in the tests, which likely means it is indeed necessary.
I tried to infer the intent of the author here, and even considered adding the names of the components to the events, but the events are already always per component, so that was meaningless. I also thought it might be a missing toString, that they would wish to show the component name for debugging purposes, but I didn't see any asserts/logs on the component objects themselves (just the events).
So I concluded there is no reason to keep this field, but lmk if anyone disagrees and I can bring it back and add some function to it.
st-pasha pushed a commit that referenced this issue Mar 2, 2023
@luanpotter @spydon @st-pasha
refactor: Remove unused event "ScoreEventCleared" from flame_block ex…
4e964af 
…ample (#2380)

# Description

This is a cleanup identified on this issue:
#2308
Using an amazing unused-code tooling
Now, since Flame is a public API, unused code might not be trivial - it
might just mean untested code.

Since this an example package, it definitely should never have unused
code.
In this case, the constructor wasn't being used, but upon deeper
investigation, that was because this particular event wasn't ever fired
at all.
So I removed it entirely. If this was supposed to be fired somewhere by
the example, please lmk and I can add the missing firing.
But as far as I can see, the event serves no purpose in this example.

## Checklist

- [x] I have followed the [Contributor Guide] when preparing my PR.
- [x] I have updated/added tests for ALL new/updated/fixed
functionality.
- [x] I have updated/added relevant documentation in `docs` and added
dartdoc comments with `///`.
- [x] I have updated/added relevant examples in `examples` or `docs`.

## Breaking Change?

- [ ] Yes, this PR is a breaking change.
- [x] No, this PR is not a breaking change.

<!-- Links -->
[Contributor Guide]:
https://github.com/flame-engine/flame/blob/main/CONTRIBUTING.md
[Conventional Commit]: https://conventionalcommits.org/
[CHANGELOG]:
https://github.com/flame-engine/flame/blob/main/CHANGELOG.md

Co-authored-by: Lukas Klingsbo <me@lukas.fyi>
st-pasha pushed a commit that referenced this issue Mar 2, 2023
@luanpotter @st-pasha
test: Add test for two subsequent state changes to flame_multi_bloc_p…
775de23 
…rovider_test.dart (#2381)

This is a cleanup identified on this issue: #2308
Using an amazing unused-code tooling
Now, since Flame is a public API, unused code might not be trivial - it might just mean untested code.

In this case, it is a test file, so there should definitely be no unused code.
However analyzing the unused code revealed to me some intent of testing multiple subsequent state changes (dead -> raise from dead).
I believe such test was missing entirely, so I added it. I think it holds value, but lmk if you disagree I can just delete the test and the method.
@luanpotter luanpotter mentioned this issue Apr 28, 2023
Merged
6 tasks
@luanpotter
Copy link
Member

That was the last one ;)

@luanpotter
Copy link
Member

Closing it as we merged everything! 🎉
Thanks, @incendial, for putting the list together! Always good to see someone else also passionate about keeping the code clean

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@spydon @erickzanardo @luanpotter @incendial