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 ConditionSerializer class #311

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

emaiannone
Copy link

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.

@codecov-io
Copy link

codecov-io commented Jan 18, 2020

Codecov Report

Merging #311 into master will increase coverage by 8.12%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #311      +/-   ##
============================================
+ Coverage      9.26%   17.38%   +8.12%     
============================================
  Files           430      443      +13     
  Lines         12373    12975     +602     
  Branches       1411     1517     +106     
============================================
+ Hits           1146     2256    +1110     
+ Misses        10985    10408     -577     
- Partials        242      311      +69
Impacted Files Coverage Δ Complexity Δ
...ge/backend/json/condition/ConditionSerializer.java 0% <ø> (ø) 0 <0> (ø) ⬇️
...ain/java/ryey/easer/core/log/ActivityLogService.kt 17.5% <0%> (-1.42%) 0% <0%> (-3%)
...ey/easer/core/data/storage/ProfileDataStorage.java 0% <0%> (ø) 0% <0%> (ø) ⬇️
.../easer/core/ui/data/script/EditScriptActivity.java 0% <0%> (ø) 0% <0%> (ø) ⬇️
...pp/src/main/java/ryey/easer/skills/SkillUtils.java 0% <0%> (ø) 0% <0%> (ø) ⬇️
...data/storage/backend/json/script/ScriptParser.java 0% <0%> (ø) 0% <0%> (ø) ⬇️
...aser/core/ui/data/profile/EditProfileActivity.java 0% <0%> (ø) 0% <0%> (ø) ⬇️
...ryey/easer/core/data/storage/EventDataStorage.java 0% <0%> (ø) 0% <0%> (ø) ⬇️
...yey/easer/core/data/storage/ScriptDataStorage.java 0% <0%> (ø) 0% <0%> (ø) ⬇️
...in/java/ryey/easer/core/ui/data/EditDataProto.java 0% <0%> (ø) 0% <0%> (ø) ⬇️
... and 217 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update eb61218...fa30b32. Read the comment docs.

@renyuneyun
Copy link
Owner

renyuneyun commented Jan 18, 2020

Thanks for the patch (though automatic I suppose, but I'll leave the comment anyway) :)
Indeed, that method can be turned into a function, and it should be. Though, for this case, the impact should be minimum.

Anyway, I'm happy to merge it. But could you please recheck the intentindent? It seems to be incorrect.

@idealist1508
Copy link
Contributor

idealist1508 commented Jan 18, 2020

But could you please recheck the intent? It seems to be incorrect.

Do you mean a failing check? I think it is something wrong with codecov settings.
It seems that coverage does't work since Multidex was enabled. I don't know how to fix it.

json_situation.put(C.SPEC, LocalSkillRegistry.getInstance().condition().findSkill(condition).id());
json_situation.put(C.DATA, condition.serialize(PluginDataFormat.JSON));
return json_situation;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Indent is wrong

Copy link
Author

Choose a reason for hiding this comment

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

Thank you for your feedback. Yeah, I already know this issue, it's because the tool works through ASTs, and I leave the default reformatting of the library when i write back the changes into source files.

Copy link
Owner

Choose a reason for hiding this comment

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

You are right... I misspelt the word.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants