Improve readability and maintainability of undo method

[TridentGalaxy]

Check for duplicates

• [X] I have searched for similar issues before opening a new one.

Problem

The Workspace.undo() method takes one bool parameter named "redo" which, if false, performs the undo action as one would expect of an "undo()" method, and if true, performs a redo of the last action that was reverted using undo(). Since the name of the parameter is not visible when calling the method (e.g. undo(false)), this seems unintuitive and decreases readability and maintainability of Blockly code. See Blockly reference.

To fix

1. In core/workspace.ts, find the undo method and give it a default value of false

This will allow users to simply call undo() to undo something, while still allowing undo(true) to redo.

If you'd like to work on this issue, please comment here so we can assign it to you. You can also read our contributing docs

Alternatives considered

• Option 1: Instead of just having one undo() method and a bool parameter, introduce a parameterless redo() method and remove the parameter of undo(). When using Blockly, the code would call myWorkspace.undo() or myWorkspace.redo() without any parameters. In order to avoid duplicate code, internally this could call a "performUndoRedo" method and pass a parameter that determines if undo or redo should be performed. Ideally, the type of this parameter would be an enum, e.g. "UndoRedoAction", with two elements, e.g. "UndoRedoAction.UNDO" and "UndoRedoAction.REDO". This could be made backward compatible by making the bool parameter optional, instead of removing it, and using false as the default value.

• Option 2: Use the internal implementation with the enum described above, but make it the official implementation. That means, the code using Blockly would call myWorkspace.performUndoRedo(UndoRedoAction.UNDO) or myWorkspace.performUndoRedo(UndoRedoAction.REDO).

Alternatives considered

No response

Additional context

No response

[maribethb]

Hi, thanks for the issue.

Making the parameter optional so that if you don't pass a boolean we'll default to false is reasonable. Your other suggestions might be a bit too heavyweight. Also, it's common for folks to annotate single parameters like this with a comment so that it's clear to future readers, e.g. undo( /* redo */ false)

[TridentGalaxy]

Thanks for the positive feedback. :)

If you follow Clean Code guidelines, comments like undo( /* redo */ false) should not be necessary. The method names should be clear enough.

Therefore, I would still suggest to add a redo() method that internally just calls undo(true), even if the solution with the performUndoRedo() method won't be implemented.

Thank you for considering my suggestion.