Skip to content

Clean Code for bundles/org.eclipse.core.commands #3258

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.

Sign up for GitHub

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

Merged
laeubi merged 2 commits into from
Sep 18, 2025
Merged

Clean Code for bundles/org.eclipse.core.commands #3258

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0cfd0be
Perform clean code of bundles/org.eclipse.core.commands
eclipse-platform-bot Sep 18, 2025
cc1be79
Version bump(s) for 4.38 stream
eclipse-platform-bot Sep 18, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion bundles/org.eclipse.core.commands/META-INF/MANIFEST.MF
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ Manifest-Version: 1.0
Bundle-ManifestVersion: 2
Bundle-Name: %pluginName
Bundle-SymbolicName: org.eclipse.core.commands
Bundle-Version: 3.12.400.qualifier
Bundle-Version: 3.12.500.qualifier
Bundle-Vendor: %providerName
Bundle-Localization: plugin
Export-Package: org.eclipse.core.commands,
Expand Down
12 changes: 4 additions & 8 deletions bundles/org.eclipse.core.commands/src/org/eclipse/core/commands/Command.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -550,8 +550,7 @@ private void fireNotDefined(final NotDefinedException e) {

if (executionListeners != null) {
for (final IExecutionListener object : executionListeners) {
if (object instanceof IExecutionListenerWithChecks) {
final IExecutionListenerWithChecks listener = (IExecutionListenerWithChecks) object;
if (object instanceof final IExecutionListenerWithChecks listener) {
listener.notDefined(getId(), e);
}
}
Expand All @@ -576,8 +575,7 @@ private void fireNotEnabled(final NotEnabledException e) {

if (executionListeners != null) {
for (final IExecutionListener object : executionListeners) {
if (object instanceof IExecutionListenerWithChecks) {
final IExecutionListenerWithChecks listener = (IExecutionListenerWithChecks) object;
if (object instanceof final IExecutionListenerWithChecks listener) {
listener.notEnabled(getId(), e);
}
}
Expand Down Expand Up @@ -788,8 +786,7 @@ public IParameter[] getParameters() throws NotDefinedException {
*/
public ParameterType getParameterType(final String parameterId) throws NotDefinedException {
final IParameter parameter = getParameter(parameterId);
if (parameter instanceof ITypedParameter) {
final ITypedParameter parameterWithType = (ITypedParameter) parameter;
if (parameter instanceof final ITypedParameter parameterWithType) {
return parameterWithType.getParameterType();
}
return null;
Expand Down Expand Up @@ -1077,8 +1074,7 @@ public void undefine() {

final String[] stateIds = getStateIds();
if (stateIds != null) {
if (handler instanceof IObjectWithState) {
final IObjectWithState handlerWithState = (IObjectWithState) handler;
if (handler instanceof final IObjectWithState handlerWithState) {
for (final String stateId : stateIds) {
handlerWithState.removeState(stateId);

Expand Down
8 changes: 3 additions & 5 deletions bundles/org.eclipse.core.commands/src/org/eclipse/core/commands/CommandManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,8 +54,7 @@ private final class ExecutionListener implements
public void notDefined(String commandId, NotDefinedException exception) {
if (executionListeners != null) {
for (final IExecutionListener object : executionListeners) {
if (object instanceof IExecutionListenerWithChecks) {
final IExecutionListenerWithChecks listener = (IExecutionListenerWithChecks) object;
if (object instanceof final IExecutionListenerWithChecks listener) {
listener.notDefined(commandId, exception);
}
}
Expand All @@ -66,8 +65,7 @@ public void notDefined(String commandId, NotDefinedException exception) {
public void notEnabled(String commandId, NotEnabledException exception) {
if (executionListeners != null) {
for (final IExecutionListener object : executionListeners) {
if (object instanceof IExecutionListenerWithChecks) {
final IExecutionListenerWithChecks listener = (IExecutionListenerWithChecks) object;
if (object instanceof final IExecutionListenerWithChecks listener) {
listener.notEnabled(commandId, exception);
}
}
Expand Down Expand Up @@ -246,7 +244,7 @@ private static String unescape(final String escapedText)
*/
private IExecutionListenerWithChecks executionListener;

private boolean shouldCommandFireEvents = true;
private final boolean shouldCommandFireEvents = true;

/**
* The collection of execution listeners. This collection is
Expand Down
3 changes: 1 addition & 2 deletions bundles/org.eclipse.core.commands/src/org/eclipse/core/commands/Parameterization.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,11 +90,10 @@ public final boolean equals(final Object object) {
return true;
}

if (!(object instanceof Parameterization)) {
if (!(object instanceof final Parameterization parameterization)) {
return false;
}

final Parameterization parameterization = (Parameterization) object;
if (!(Objects.equals(this.parameter.getId(), parameterization.parameter.getId()))) {
return false;
}
Expand Down
3 changes: 1 addition & 2 deletions bundles/org.eclipse.core.commands/src/org/eclipse/core/commands/ParameterizedCommand.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -423,11 +423,10 @@ public boolean equals(final Object object) {
return true;
}

if (!(object instanceof ParameterizedCommand)) {
if (!(object instanceof final ParameterizedCommand command)) {
return false;
}

final ParameterizedCommand command = (ParameterizedCommand) object;
return Objects.equals(this.command, command.command)
&& Arrays.equals(this.parameterizations, command.parameterizations);
}
Expand Down
3 changes: 1 addition & 2 deletions bundles/org.eclipse.core.commands/src/org/eclipse/core/commands/common/HandleObject.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,12 +118,11 @@ public boolean equals(final Object object) {
}

// Check if they're the same type.
if (!(object instanceof HandleObject)) {
if (!(object instanceof final HandleObject handle)) {
Copy link
Copy Markdown
Contributor

@laeubi laeubi Sep 16, 2025

Choose a reason for hiding this comment

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

@jjohnstn this looks a bit strange and it seems final is not added to all instanceof checks (where I assume we likely never modify variables used in instanceof checks anyways)

Copy link
Copy Markdown
Contributor

@jjohnstn jjohnstn Sep 16, 2025

Choose a reason for hiding this comment

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

The "add final" cleanup was modified to start with a fresh AST and apply previous cleanups. So, if you asked for the pattern instanceof cleanup, the final cleanup will see the new pattern variable that was added and will make it final if conditions are appropriate. In the past, the final cleanup would not have seen the new pattern instanceof expression.

Copy link
Copy Markdown
Contributor

@laeubi laeubi Sep 17, 2025

Choose a reason for hiding this comment

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

@jjohnstn so while your explanation seems valid to me, I suspect something is wrong here, because I would not have expected this because we only enabled to make fields as

Add final modifier to private fields

but not for local variables or parameters...

So is my expectation maybe wrong?

Copy link
Copy Markdown
Contributor

@jjohnstn jjohnstn Sep 17, 2025

Choose a reason for hiding this comment

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

Actually, in this case, the declaration of handle that is used to make the pattern instanceof variable is declared final and casts object to HandleObject. The code is doing the right thing. You have a final HandleObject called handle in both cases.

Copy link
Copy Markdown
Contributor

@laeubi laeubi Sep 18, 2025

Choose a reason for hiding this comment

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

I think I begin to understand so because previously

final HandleObject handle= (HandleObject) object;

is defined final this is applied to the instanceof as well.. now it seems obvious, thanks for the pointer!

return false;
}

// Check each property in turn.
final HandleObject handle= (HandleObject) object;
return Objects.equals(id, handle.id)
&& (this.getClass() == handle.getClass());
}
Expand Down
9 changes: 4 additions & 5 deletions ...lipse.core.commands/src/org/eclipse/core/commands/operations/DefaultOperationHistory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,7 @@ public final class DefaultOperationHistory implements IOperationHistory {
/**
* a map of undo limits per context
*/
private Map<IUndoContext, Integer> limits = Collections.synchronizedMap(new HashMap<>());
private final Map<IUndoContext, Integer> limits = Collections.synchronizedMap(new HashMap<>());

/**
* the list of {@link IOperationHistoryListener}s
Expand All @@ -137,12 +137,12 @@ public final class DefaultOperationHistory implements IOperationHistory {
/**
* the list of operations available for redo, LIFO
*/
private List<IUndoableOperation> redoList = Collections.synchronizedList(new ArrayList<>());
private final List<IUndoableOperation> redoList = Collections.synchronizedList(new ArrayList<>());

/**
* the list of operations available for undo, LIFO
*/
private List<IUndoableOperation> undoList = Collections.synchronizedList(new ArrayList<>());
private final List<IUndoableOperation> undoList = Collections.synchronizedList(new ArrayList<>());

/**
* a lock that is used to synchronize access between the undo and redo
Expand Down Expand Up @@ -807,8 +807,7 @@ public IUndoableOperation getUndoOperation(IUndoContext context) {
private IStatus getExecuteApproval(IUndoableOperation operation, IAdaptable info) {

for (IOperationApprover tmp : approvers) {
if (tmp instanceof IOperationApprover2) {
IOperationApprover2 approver = (IOperationApprover2) tmp;
if (tmp instanceof IOperationApprover2 approver) {
IStatus approval = approver.proceedExecuting(operation, this, info);
if (!approval.isOK()) {
if (DEBUG_OPERATION_HISTORY_APPROVAL) {
Expand Down
6 changes: 3 additions & 3 deletions ...org.eclipse.core.commands/src/org/eclipse/core/commands/operations/ObjectUndoContext.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,11 +27,11 @@
*/
public final class ObjectUndoContext extends UndoContext {

private Object object;
private final Object object;

private String label;
private final String label;

private List<IUndoContext> children = new ArrayList<>();
private final List<IUndoContext> children = new ArrayList<>();

/**
* Construct an operation context that represents the given object.
Expand Down
6 changes: 3 additions & 3 deletions ...eclipse.core.commands/src/org/eclipse/core/commands/operations/OperationHistoryEvent.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,12 +144,12 @@ public final class OperationHistoryEvent {

private int code = 0;

private IOperationHistory history;
private final IOperationHistory history;

private IUndoableOperation operation;
private final IUndoableOperation operation;

/* @since 3.2 */
private IStatus status;
private final IStatus status;

/**
* Construct an event for the specified operation history.
Expand Down
2 changes: 1 addition & 1 deletion ...g.eclipse.core.commands/src/org/eclipse/core/commands/operations/TriggeredOperations.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ public final class TriggeredOperations extends AbstractOperation implements

private IUndoableOperation triggeringOperation;

private IOperationHistory history;
private final IOperationHistory history;

private List<IUndoableOperation> children = new ArrayList<>();

Expand Down
Loading