Many good things have been addressed in the comments. A few things I usually take into account when reviewing are below.
For the code:
understand the context of the changes
make sure the scope of each commit is correct (it makes it easier to revert, change the content or drop the commit, if wanted)
make sure the naming and the design are consistent with the existing architecture
make sure there aren’t duplicated code that could be encapsulated in a centralised method
make sure the visibility of the variables and methods are as strict as they must be (private, protected, public)
identify and address possible enhancements even in code, even if that’s not changed in the code under review, if that proves to improve the codebase (The boy scout rule)
identify comments that could be replaced by well-named methods that implement the logic
request to remove useless comments (e.g. a method save() with a comment “save the entity”).
search flag parameters (booleans) and try to eliminate them
make sure the scope of changes or the logic in the class method belong where they are - they may be part of some other component in the architecture
avoid the use of null (in Scala I try to replace them all with None or Optional)
there aren’t TODOs in the code not addressed with a ticket to be worked on later
For the ticket:
the title and description of the ticket are easy to understand by someone that hasn’t worked on it
all automated tests have passed
there is a well-written test plan
For further actions, you may consider blocking this person and/or reporting abuse
We're a place where coders share, stay up-to-date and grow their careers.
Many good things have been addressed in the comments. A few things I usually take into account when reviewing are below.
For the code:
save()with a comment “save the entity”).NoneorOptional)TODOs in the code not addressed with a ticket to be worked on laterFor the ticket: