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 DefaultFunctionCallExpr #168

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

hduelme
Copy link
Contributor

@hduelme hduelme commented Feb 15, 2023

I made the fields prefix and functionName final.
Gentrified all lists and replaced the for loop by the foreach loop (available size Java 5).

The foreach loop is easier to read and faster, compared to the normal for loop.

@@ -77,7 +77,7 @@ public void addParameter(Expr parameter)
}


public List getParameters()
public List<Expr> getParameters()
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a potentially code breaking public API change, which we won't do. (Consider subclasses that override this method.) Ditto for other signature changes below.

Copy link
Contributor Author

@hduelme hduelme Feb 16, 2023

Choose a reason for hiding this comment

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

I have done a little test.
I have written an class OverwriteTest like so:

package org.jaxen.expr;

import java.util.List;

public class OverwriteTest extends DefaultFunctionCallExpr {
    public OverwriteTest(String prefix, String functionName) {
        super(prefix, functionName);
    }

    @Override
    public List getParameters() {
        return super.getParameters();
    }
}

No Problem for the Java Compiler. Indeed it is not a good practice to drop the generics, but it is possible without an API break.

If you agree the signature in the interface FunctionCallExpr should also be changed.


paramValues.add(eachValue);
for (Expr eachParam : paramExprs) {
Copy link
Contributor

Choose a reason for hiding this comment

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

foreach might be clearer. There's no proof it's faster.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right. This is possibly a flawed assumption on my part, but still it's also not slower. The compiler should optimize both to the same speed.

@hduelme hduelme requested a review from elharo February 16, 2023 17:06
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.

2 participants