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

Renaming variables for codebase consistency #834

Merged
merged 3 commits into from Feb 26, 2014

Conversation

Projects
None yet
3 participants
@mallamanis
Contributor

mallamanis commented Feb 25, 2014

I've been working on a research machine learning-based tool (link: http://groups.inf.ed.ac.uk/naturalize/ ) tool that analyzes source code identifiers and makes suggestions for renaming them. The goal is to reduce unnecessary diversity in variable naming and improve code readability. This pull request is only a small sample of the suggestions made for JUnit.

No functional changes were made in any of the commits.

Renamed "result" (18.69%) to "suite" (81.31%). The naturalize tool de…
…tected

that using "suite" is more consistent with the current codebase state.
@kcooney

View changes

Show outdated Hide outdated src/main/java/junit/framework/TestResult.java
@stefanbirkner

This comment has been minimized.

Show comment
Hide comment
@stefanbirkner

stefanbirkner Feb 25, 2014

Contributor

Your first commit (renaming "result" to "suite") is nice and I would like to merge it. But renaming "oldOut" is not good, because it is a more meaningful name than "oldPrintStream". Renaming "e" to "t" is no improvement, because we should consistently use e.

Could you please create a pull request with the first commit only.

Contributor

stefanbirkner commented Feb 25, 2014

Your first commit (renaming "result" to "suite") is nice and I would like to merge it. But renaming "oldOut" is not good, because it is a more meaningful name than "oldPrintStream". Renaming "e" to "t" is no improvement, because we should consistently use e.

Could you please create a pull request with the first commit only.

@mallamanis

This comment has been minimized.

Show comment
Hide comment
@mallamanis

mallamanis Feb 25, 2014

Contributor

@stefanbirkner thanks for the comment. It seems that you are right about the "e" to "t" renaming. Sorry for not noticing that before.

Concerning the oldPrintStream, I too like oldOut more. The tools suggested this renaming because it was used in a similar context in org.junit.tests.running.core.MainRunner (lines 276 and 288). Do you think that renaming that variable (oldPrintStream in org.junit.tests.running.core.MainRunner) would make any sense? Obviously, we can always just drop the oldPrintStream change.

Contributor

mallamanis commented Feb 25, 2014

@stefanbirkner thanks for the comment. It seems that you are right about the "e" to "t" renaming. Sorry for not noticing that before.

Concerning the oldPrintStream, I too like oldOut more. The tools suggested this renaming because it was used in a similar context in org.junit.tests.running.core.MainRunner (lines 276 and 288). Do you think that renaming that variable (oldPrintStream in org.junit.tests.running.core.MainRunner) would make any sense? Obviously, we can always just drop the oldPrintStream change.

@stefanbirkner

This comment has been minimized.

Show comment
Hide comment
@stefanbirkner

stefanbirkner Feb 25, 2014

Contributor

Renaming the variable in org.junit.tests.running.core.MainRunner makes sense. 👍

Contributor

stefanbirkner commented Feb 25, 2014

Renaming the variable in org.junit.tests.running.core.MainRunner makes sense. 👍

@mallamanis

This comment has been minimized.

Show comment
Hide comment
@mallamanis

mallamanis Feb 25, 2014

Contributor

I've reverted the changes and made new renamings as we discussed. What do you think?

Contributor

mallamanis commented Feb 25, 2014

I've reverted the changes and made new renamings as we discussed. What do you think?

@stefanbirkner

This comment has been minimized.

Show comment
Hide comment
@stefanbirkner

stefanbirkner Feb 26, 2014

Contributor

LGTM. @mallamanis do you have experience with Git? It would be nice if you rebase your commits to three commits (Throwable, System.out, suite).

Contributor

stefanbirkner commented Feb 26, 2014

LGTM. @mallamanis do you have experience with Git? It would be nice if you rebase your commits to three commits (Throwable, System.out, suite).

@mallamanis

This comment has been minimized.

Show comment
Hide comment
@mallamanis

mallamanis Feb 26, 2014

Contributor

@stefanbirkner I think I've done that now. Let me know, if I need to make any other changes.

Contributor

mallamanis commented Feb 26, 2014

@stefanbirkner I think I've done that now. Let me know, if I need to make any other changes.

stefanbirkner added a commit that referenced this pull request Feb 26, 2014

Merge pull request #834 from mallamanis/master
Renaming variables for codebase consistency

@stefanbirkner stefanbirkner merged commit 8a63df4 into junit-team:master Feb 26, 2014

@stefanbirkner stefanbirkner added this to the 4.12 milestone Feb 26, 2014

@stefanbirkner

This comment has been minimized.

Show comment
Hide comment
@stefanbirkner

stefanbirkner Feb 26, 2014

Contributor

Everything is fine now. Thank you.

Contributor

stefanbirkner commented Feb 26, 2014

Everything is fine now. Thank you.

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