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

Walk seems to be not elegant #3755

Closed
ArtemGet opened this issue Dec 25, 2024 · 4 comments
Closed

Walk seems to be not elegant #3755

ArtemGet opened this issue Dec 25, 2024 · 4 comments

Comments

@ArtemGet
Copy link
Contributor

Walk class is not elegant because of prestructors that accessing unmanaged resources and implementation inheritance.

I believe that:

  1. Walk** classes should implement Iterable<Path> instead of extending ListIterator<Path>.
  2. Resource acceccing should be done only when iterator() is called.
  3. There should not be including() or excluding() methods

Example of api usage:

            Stream.of(
                new WalkWith(
                    new RuleInclude(PlaceMojo.this.includeBinaries),
                    new WalkWith(
                        new RuleExclude(PlaceMojo.this.excludeBinaries),
                        new Walk(this.dir)
                    )
                ).iterator()
            ).filter(...)
            .filter(...)
            ...

instead of:

new Walk(this.dir)
                .includes(PlaceMojo.this.includeBinaries)
                .excludes(PlaceMojo.this.excludeBinaries)
                .stream()
                .filter(this::isNotEoSource)
                .filter(this::isNotAlreadyPlaced)
                .filter(this::hasEoSource)
                .peek(this::printLogInfoAboutBinary)
                .peek(this::placeBinary)
                .count();

This refactoring should be useful in PhiMojo and UnphiMojo does not use cache #3708, because there is a need to compare and exclude paths of already translated files. This logic is good to be placed in one of Rule** classes that could be represented as lamba function in most cases.

@ArtemGet
Copy link
Contributor Author

@yegor256 @maxonfjvipon WDYT?

@yegor256
Copy link
Member

@ArtemGet you may find this article relevant: https://www.yegor256.com/2015/10/01/vertical-horizontal-decorating.html Your reasoning is valid, but such a refactoring will only make our code more verbose (more lines of code) while giving no practical value -- the Walk class is not extended by anyone and is not an element of a larger design structure. It's simply a one-time-use throw-away component. The faster/shorter we call it and forget about it, the better. @maxonfjvipon @volodya-lombrozo what do you guys think?

@volodya-lombrozo
Copy link
Member

@ArtemGet Interesting suggestion, actually. The only problem I see now is the absence of a reason for the change. Your design potentially might simplify the code I understand, but it's hard to imagine if it will help or just add a few more lines of code. I suggest implementing #3708 first. Then, if you are right, we will see the places where this refactoring is needed.

@ArtemGet
Copy link
Contributor Author

I suggest implementing #3708 first

@volodya-lombrozo Thanks, will do Phi/Unphi cache first.

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

No branches or pull requests

3 participants