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

Refactoring of statistics #4114

Merged
merged 1 commit into from
Mar 7, 2016
Merged

Refactoring of statistics #4114

merged 1 commit into from
Mar 7, 2016

Conversation

musilto7
Copy link

@musilto7 musilto7 commented Mar 5, 2016

I've read the code of computing statistics. There is int variable called mType which can have only three values. I decided to create enum called AxisType and replace type of mType.
There is method calculateDone in Stats.java. Second method variable is boolean. The function of boolean is not described in javadoc. I decided replace boolean by Chartype and make the method private. Then two public methods will be created: calculateReviewCount and calculateReviewTime. This two new methods will use method calculateDone.

@musilto7 musilto7 changed the title issue - 4113 Refactoring of statistics Mar 5, 2016
@timrae
Copy link
Member

timrae commented Mar 6, 2016

Does this fix any bugs or just pure refactoring? What's the reason / motivation for the changes?

@musilto7
Copy link
Author

musilto7 commented Mar 6, 2016

It is pure refactoring. The motivatation is make the code more readable. It is my first attempt for commit to open source project, next time I will try bug fixing or implementing of the new feature :)

}
mAxisTitles = new int[] { type.ordinal(), R.string.stats_answers, R.string.stats_cumulative_answers };
} else if(charType == ChartType.REVIEW_TIME)
{
Copy link
Member

Choose a reason for hiding this comment

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

Make sure this opening brace { is on the same line as the beginning of the block to remain consistent with the rest of the codebase. Also, the indenting of this block is off.

@hssm
Copy link
Member

hssm commented Mar 6, 2016

Everything looks fine here. Just one minor nitpick for you to fix up. Once you've fixed it, you can amend the commit and force push the branch and it'll show up here.

It is my first attempt for commit to open source project

Refactoring code is always a great way to familiarize yourself with the codebase. Don't feel compelled to rush into adding new features or anything if you're comfortable with doing just this for a while. Thanks for taking the time to help us improve AnkiDroid!

@musilto7
Copy link
Author

musilto7 commented Mar 7, 2016

I fixed it. But the tests failed. I tried to start the tests on my local machine and they passed. I am not sure what was wrong.

@timrae
Copy link
Member

timrae commented Mar 7, 2016

Travis is going crazy lately, failing for no reason. Don't worry about it. Can you please squash your commits though:

https://github.com/ankidroid/Anki-Android/wiki/Development-Guide#squashing-a-series-of-commits-down-to-one

Only formating of the one if block
@musilto7
Copy link
Author

musilto7 commented Mar 7, 2016

Squashing is done.

hssm added a commit that referenced this pull request Mar 7, 2016
@hssm hssm merged commit 6d0a687 into ankidroid:develop Mar 7, 2016
@hssm
Copy link
Member

hssm commented Mar 7, 2016

In the future, if you are only making a small change to an existing commit, you can simply amend the previous one instead of committing on top of it:
git commit --amend

This will save you from having to squash it down later on.

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.

None yet

3 participants