Code cleanup

[asolntsev]

Description

Code cleanup. Just applied few IDEA suggestions to shorten the code.

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (9bcccf2) 58.07% compared to head (b467b29) 58.07%.
Report is 1 commits behind head on trunk.

❗ Current head b467b29 differs from pull request most recent head 0f01329. Consider uploading reports for the commit 0f01329 to get more accurate results

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@           Coverage Diff           @@
##            trunk   #12934   +/-   ##
=======================================
  Coverage   58.07%   58.07%           
=======================================
  Files          88       88           
  Lines        5340     5340           
  Branches      224      224           
=======================================
  Hits         3101     3101           
  Misses       2015     2015           
  Partials      224      224           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[pujagani]

Thank you! However, the tests are failing with the error "Cannot use final field org.openqa.selenium.grid.server.BaseServerFlags#bindHost as a parameter; compile-time constant inlining may hide new values written to it." Those changes need to be reverted.

[asolntsev]

@pujagani Thank you for the hint (to be honest, it's hard to me to understand where is flaky test, and where is a real problem in Selenium tests output).

I've reverted final for BaseServerFlags field, hope it fixes the tests.

[titusfortner]

We need to be careful changing public or protected designations, people might be using them (even if maybe they shouldn't be). Ideally we don't change that without warning.

[pujagani]

Following up on Titus's point, this might be a breaking change for some users.

[asolntsev]

Ideally we don't change that without warning.

@titusfortner @pujagani I agree, there is a small risk that some people do modify these fields (tough, they should not).
That's why it's a good idea to release such a change now - when we released new major version (which is anyway backward incompatible). Ideally, it could be released in Selenium 4.14.0, but 14.4.2 is also good enough. I believe most people haven't upgraded yet.

In this case, there is no warning that could help. We cannot mark these fields as "@deprecated" or something like that. We can only write a blog post, but people would not notice it.

So making such a change in a next major released is the only option I see.

[titusfortner]

We should be able to mark fields as deprecated, though, right? And point people to the getters/setters?

[asolntsev]

And point people to the getters/setters?

No, using getters-setters is not better than just using fields. Getters-setters is the biggest bullshit in Java. They don't really make anything better.

[diemol]

I haven't looked at this PR and think I won't. 62 files are way too much to review for this type of change. I'd be happy to quickly review PRs with a few changes to make this whole process faster.

[titusfortner]

I'm confused. If you don't like getters and setters, then why are you making the fields not public?

[asolntsev]

why are you making the fields not public?

Now I'm confused :)
Because this is one of best practices in programming:

1. prefer immutable objects to mutable ones
2. prefer private field to public field

Unless you really need the field to be mutable (which is not really often needed), make it final by default.
Unless you really need to make the field accessible from outside your class (which should not happen too often), make it private.
This is the essence of object-oriented programming: class should encapsulate its implementation details (read: make field private) and expose its behaviour (read: methods), not data (read: fields).

[titusfortner]

Right, but exposing access to the private fields means using getters and setters.

[asolntsev]

Right, but exposing access to the private fields means using getters and setters.

Yes. But you don't really need to expose access to private fields.
No.
Instead, you should expose behaviour (methods) which can somehow use the private fields, but should not expose these fields.

[titusfortner]

The entire point of Point and Rectangle is to provide structured access to the values so people can use them where necessary. I'm okay if we want to mark the fields as deprecated and direct people to the getters/setters, otherwise we need to leave these as public.

[asolntsev]

The entire point of Point and Rectangle is to provide structured access to the values so people can use them where necessary.

I believe people want to ASK width and height, not CHANGE them.
In other words, the intention of Point and Rectangle classes to be immutable. Am I right?
Selenium users cannot change the width and height of rectangles in the browser, right?
Leaving these fields mutable is a lie. De facto, it's not possible to change them.

[titusfortner]

Rectangle is used to set window size and position. Points for mouse movement and scrolling. You can create new instances and set values with the constructor, but there's nothing inherently immutable about storing integers with contextually relevant attributes.

[titusfortner]

Ok, so re-reading, I see what you are saying. Yes, they are immutable after creation, so we do not want to give access to those fields directly. BUT, we also don't want to remove functionality without marking them deprecated. If we are going to change it, we need to mark them as deprecated for a 2 releases.

So, we can merge this if you update that and revert the changes to CdpClientGenerator.java because we're just going to be super conservative with that class.

Thanks.

[diemol]

And please send smaller change sets in future PRs. This would have been merged much faster.

[asolntsev]

BUT, we also don't want to remove functionality without marking them deprecated. If we are going to change it, we need to mark them as deprecated for a 2 releases.

What exactly should I mark as deprecated?
If these classes had setters, I would add @Deprecated to setters.
But these are public fields. We cannot add @Deprecated to the fields because we still need these fields. We just want to make them immutable:

public class Rectangle {

  // Where should I add @Deprecated here?

  public int x;   // -> private final
  public int y;   // -> private final

[titusfortner]

you can deprecate public methods that you are going to make internal instead of removing.

[asolntsev]

you can deprecate public methods that you are going to make internal instead of removing.

Yes, I understand. But what about fields? What if we want to make fields private/immutable? @titusfortner

[asolntsev]

@titusfortner Sorry for such a long delay.
Now I reverted the backward incompatible changes.
Now this PR adds final only to private fields. So it cannot break anything.

Thank you for your patience.

[pujagani]

Final version LGTM

Already approved in comment.