ClementGre / PDF4Teachers

PDF editing software in large quantities. Designed for teachers, this app keeps recorded previous annotations, and offers features like marking scale, PDF conversion, vectorial drawing...
https://pdf4teachers.org/
Apache License 2.0
140 stars 17 forks source link

Migrate to newer language features of Java #149

Closed arturhg closed 1 year ago

ClementGre commented 1 year ago

Thank you for all this work. I don't know if you used a refactoring tool or not, but great job.

About the lambdas expressions, I prefer to have something like that:

loadItemsInPage(config.getSection("vectors").entrySet(), elementData -> {
      VectorElement.readYAMLDataAndCreate(elementData.getValue(), elementData.getKey());
});

Than something like that :

loadItemsInPage(config.getSection("vectors").entrySet(), elementData -> VectorElement.readYAMLDataAndCreate(elementData.getValue(), elementData.getKey()) );

Cause it can get very long, and not fit in the screen. It would be very nice if you could correct all the lines that exceed ≈ 100 chars.

(I don't know if it is really more optimised to put everything in one line, maybe it can prevent the creation of an empty scope?)

arturhg commented 1 year ago

Thank you for your comment, and thank you for creating and maintaining such a wonderful piece of software.

I used built-in refactoring tools of IntelliJ IDEA and manual refactoring.

Personally, I prefer expression lambdas () -> doSomething() over statement lambdas () -> { doSomething();}, especially if it is possible to make it a "one-line lambda". This becomes even more compact if we need to return something: () -> doSomething() vs () -> {return doSomething();}.

But I agree with you that in more complex cases, expression lambdas make unnecessary long lines of code. I reverted my commits for converting statement lambdas to expression lambdas in this PR. I think we can revisit this part later. Other changes are still intact.

If you are OK having 2 different styles of lambdas in codebase (both expression and statement), I can make separate PR for converting only those statement lambdas into expression ones, which are possible to convert without exceeding ≈ 100 chars length and basically make them "one-line lambdas". The rest of lambdas may require additional refactoring like extracting the code of lambda into method, etc. That part can be done later in separate PR if necessary. As far as I know, there is no performance and resource usage difference between expression lambdas and statement lambdas.

About limiting line length to ≈ 100 chars in whole codebase, I think it can be addressed in separated PR.

ClementGre commented 1 year ago

Thank you very much. I totally agree to have two types of lambdas in the codebase. I'll merge this one and I let you make a new PR for short lambdas that can be put inline without making it too difficult to understand.

Creating separate functions just for making a lambda inline is a bit too heavy, and I think we can stay with the current bracket lambdas for longer expressions.