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

OptimizationTask is a bad class #2934

Open
levBagryansky opened this issue Mar 11, 2024 · 3 comments
Open

OptimizationTask is a bad class #2934

levBagryansky opened this issue Mar 11, 2024 · 3 comments

Comments

@levBagryansky
Copy link
Member

levBagryansky commented Mar 11, 2024

Every time I meet OptimizationTask class, I have a hard time understanding it.

  1. Firstly it has an empty commentary which clears nothing
/**
 * Optimized Tojos.
 * @since 0.34.0
 */
  1. It has unclear ctor
    /**
     * Ctor.
     * @param paths Map with optimization folder paths of Mojo.
     * @param dirs Map with target directory and EO cache directory of Mojo.
     * @param update Function that updates ForeignTojo.
     * @param source Function that gets Tojo path.
     * @checkstyle ParameterNumberCheck (15 lines)
     */
    OptimizationTask(
        final Map<String, Path> paths,
        final Map<String, String> dirs,
        final BiFunc<ForeignTojo, Path, ForeignTojo> update,
        final Func<ForeignTojo, Path> source
    )

I do not see the difference between paths and dirs fields. As a user I don't understand why I should provide maps.
3. The use of the maps paths and dirs is counterintuitive, and, most importantly, unnecessary. Just look at how they are used:

this.paths.get(
    OptimizationFolder.CACHE.key()
).resolve(this.dirs.get(OptimizationFolder.CACHE.key())).resolve(tojo.hash())

These maps need rwo items with known keys only. As I understand there were just fields before, but we don't like a lot of fields. Maybe it seems to me, but it's obviously worse now.
Just see how this class is used:

new OptimizationTask(
    new MapOf<String, Path>(
        new MapEntry<>(OptimizationFolder.TARGET.key(), this.targetDir.toPath()),
        new MapEntry<>(OptimizationFolder.CACHE.key(), this.cache)
    ),
    OptimizeMojo.DIRECTORIES,
    ForeignTojo::withOptimized,
    ForeignTojo::xmir
)

At a minimum, this class could be much safer if it accepted just Strings instead of maps because it does not check validity of maps in ctor.
This is the most weird point as for me.
4. The following method not just make a path but also saves xml according to implementation. That is, there are side effects in the method. So commentary and name are not clear enough.

/**
    * Make a path with optimized XML file after parsing.
    *
    * @param xml Optimized xml
    * @param file EO file
    * @return The file with optimized XMIR
    * @throws IOException If fails
    */
   private Path make(final XML xml, final Path file) throws IOException 
  1. It is more about OptimizationFolder but why we need key here https://github.com/objectionary/eo/blob/master/eo-maven-plugin/src/main/java/org/eolang/maven/OptimizationFolder.java#L46. For example it would be better to use instances of the enum instead of their key in private final Map<String, Path> paths;.
  2. It does not have tests so It is unclear how to use it. I read the code, come across this class and have to read its implementation. Moreover, the terms used in the explanations for this class are unclear in themselves.
@levBagryansky
Copy link
Member Author

@volodya-lombrozo @yegor256 WDYT?

@yegor256
Copy link
Member

@levBagryansky I suggest we improve the documentation of the class

@volodya-lombrozo
Copy link
Member

volodya-lombrozo commented Mar 11, 2024

@levBagryansky

  1. Firstly it has an empty commentary which clears nothing

Agree, it would be better to add at least some description to this class. Moreover, why the current description is Optimized Tojos., it's a mystery.

  1. It has unclear ctor

Totally agree. Tried to understand it from the code and failed myself

(3) (4) (5)

It's hard to deem something as 'unnecessary' here. I would suggest simplifying the logic in methods and constructors as you see fit and proposing specific changes in the code. Most likely, you are correct, but it's challenging to assess based on small code snippets.

  1. It does not have tests so It is unclear how to use it.

Agree, it's the most frustrating point here.

In general, I would suggest making several different PRs to enhance this class:

  1. Enhance the documentation of the class. It would be beneficial to understand where and how we use these fields and to add this description to the methods, fields, and constructors.
  2. Incorporate unit tests to demonstrate current functionality.
  3. Modify the logic as you see fit (refactoring).

Just adding 'documentation' is insufficient. The most crucial aspect is tests - they serve well as documentation and will lay the foundation for future refactoring.

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

No branches or pull requests

3 participants