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 Member Ignoring Method smell in TvMainActivity class #8555

Open
wants to merge 1 commit into
base: master
from

Conversation

@emaiannone
Copy link

emaiannone commented Jan 11, 2020

Hi, I'm Emanuele Iannone, a master student at University of Salerno.
Since my bachelor's thesis I have been working on a code smell refactoring plugin called aDoctor, which is able to identify and fix energy-related problems in Android apps.
I launched it on your project, finding different instances of code smells. I chose one of them and let the plugin automatically fix it.
In this case I chose Member Ignoring Method, that is present when a non static method does not use at all instance variables and other non static methods. These kind of smell may have a non trivial impact on energy consumption, as shown in this paper: https://www.sciencedirect.com/science/article/pii/S0950584918301678.
Besides, this kind of refactoring does not impact on the functionalities of your app, so it is totally safe. Let me know if you are interested in this refactoring proposal.

@jordan-woyak

This comment has been minimized.

Copy link
Member

jordan-woyak commented Jan 11, 2020

I realize this change was done by a program but it has removed comments and changed the formatting of the code. Please undo those changes.

@BhaaLseN

This comment has been minimized.

Copy link
Member

BhaaLseN commented Jan 12, 2020

Is the actual change there making the method static? If so, I wonder if it would be more consistent to keep it the same across different buildXxxxRow methods (preferably: make all of them static, but I'm guessing that isn't possible since otherwise the tool would've made it?)

@Helios747

This comment has been minimized.

Copy link
Contributor

Helios747 commented Jan 14, 2020

Given that Dolphin barely spends any time in the Java UI code and the vast majority of the CPU time in the common emulation code compiled with the NDK, it's hard to justify refactors with the goal of energy consumption. Especially since you don't really have any data to support that this helps Dolphin specifically. A paper on how it works for mobile workloads is great, but we're hardly a standard mobile workload. (As shown by mobile CPU governors getting wildly confused with Dolphin :))

@emaiannone

This comment has been minimized.

Copy link
Author

emaiannone commented Jan 18, 2020

Thanks for the feedbacks, I will improve the tool in order to avoid situations where it changes the formatting instead of forcing one in particular.
@Helios747 if you show me an important class (or package) where to run the tool I might try to find other smells. Let me know, if you are interested.

@jordan-woyak

This comment has been minimized.

Copy link
Member

jordan-woyak commented Feb 12, 2020

@emaiannone Do you plan on fixing the formatting issues in this PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.