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

Rule: UnusedPrivateMember #641

Merged
merged 9 commits into from
Feb 11, 2018

Conversation

Mauin
Copy link
Collaborator

@Mauin Mauin commented Dec 24, 2017

Resolves #8

This should cover unused private properties. Public properties are ignored as they might be used from other files.

@Mauin Mauin added the rules label Dec 24, 2017
}
}

class Test {
Copy link
Contributor

Choose a reason for hiding this comment

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

Hanging fruit?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oops, was testing with this one 🙃

Copy link
Member

@schalkms schalkms left a comment

Choose a reason for hiding this comment

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

I appreciate this changes. However, this rule needs a symbol table to function correctly for all cases.

@arturbosch
Copy link
Member

arturbosch commented Dec 24, 2017

Cool, this is a useful rule! Do you want to support parameters too? Unused private functions you can also find with the same reference counting.

I have implemented a UnusedPrivateMembers rule for java. Here is the test code:

@SuppressWarnings("ALL") public class DeadCodeDummy {

	private int deadField = 5;
	private int usedField = 5;
	private String usedString = "Hello";

	private int deadMethod(int deadParameter) {
		int deadLocaleVariable = 5;
		usedMethod("" + usedField++);
		usedField = usedString.length();
		return 5;
	}

	public int usedMethod(String usedParameter) {
		usedField = usedParameter.length() + usedString.length();
		int usedLocaleVariable = 4;
		return Integer.valueOf(usedField).compareTo(usedLocaleVariable);
	}

	public void usedParameterInAssignment(String uri, long j) {
		int i = (int) j;
		String fullUrl = uri + ("/commits/new" + 6);
		System.out.println(i + fullUrl);
	}
}

Also you should check the refences in all possible conditional statements:

	private void run(boolean go) {
		boolean used = true;
		if(go && used) {
			String concat = "";
			switch (concat) {
				case "truetrue": run2(); break;
				default: break;
			}
		}
	}

	private void run2() {
		List<String> strings = new ArrayList<>();
		for (String s : strings) {
			System.out.println("");
		}
}

@Mauin Mauin force-pushed the mr/rule_unused_private_member branch from 8c46ed9 to 60c414e Compare January 20, 2018 11:08
@Mauin Mauin force-pushed the mr/rule_unused_private_member branch from 60c414e to 30c0c00 Compare January 25, 2018 07:04
it("reports an unused member") {
val code = """
class Test {
private val unused = "This is not used"
Copy link
Contributor

Choose a reason for hiding this comment

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

indentation seems to be off. This seems to be using spaces over tabs.

super.visitNamedFunction(function)

function.valueParameterList?.parameters?.forEach {
val name = it.name ?: throw IllegalStateException("Value parameter should have a name.")
Copy link
Contributor

Choose a reason for hiding this comment

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

Couldn't you do just !!? That will throw for you. If the intention is to have a custom message, then I think the exception type should better to be NullPointerException

Copy link
Member

Choose a reason for hiding this comment

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

Are there any cases where a parameter has no name?
I think it is just for anonymous objects that it can be null.
How about using safeName or nameAsSafeName (dont remember how it is called)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

TIL about nameAsSafeName. Will change it!

Copy link
Member

@arturbosch arturbosch left a comment

Choose a reason for hiding this comment

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

Can you please provide some test cases with local methods and loops. Thanks :)
Cool great so far!

@Mauin
Copy link
Collaborator Author

Mauin commented Feb 1, 2018

Great idea @arturbosch I added some extra handling to catch unused loop parameters.

@arturbosch arturbosch merged commit 10b1b11 into detekt:master Feb 11, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants