Post-Merge cleanup of MutableDocuments #2468
Conversation
Coverage ReportAffected SDKs
Test Logs
NotesHTML coverage reports can be produced locally with Head commit (0952cb9c) is created by Prow via merging commits: 979000c a27fe2c. |
Binary Size ReportAffected SDKs
Test Logs
NotesHead commit (0952cb9c) is created by Prow via merging commits: 979000c a27fe2c. |
| @@ -17,6 +17,15 @@ | |||
| import androidx.annotation.NonNull; | |||
| import com.google.firestore.v1.Value; | |||
|
|
|||
| /** | |||
| * Represents a document in Firestore with a key, version, data and whether it has local mutations | |||
| * applied to it. Documents can start out as {@link DocumentType#INVALID} and transition to a valid | |||
There was a problem hiding this comment.
"Documents can start out as..." is not as relevant now since factory methods create the object directly into the desired state. Also, DocumentType is a private implementation detail that should not be mentioned in the javadocs for the class. This entire paragraph about starting in an invalid state and transitioning to a different state can probably just be deleted.
There was a problem hiding this comment.
Updated the comment.
| * mutations. | ||
| */ | ||
| public static MutableDocument newInvalidDocument(DocumentKey documentKey) { | ||
| return new MutableDocument( |
There was a problem hiding this comment.
These factory methods appear to duplicate the logic defined in their corresponding convertToXXX() methods. To be a bit more "dry" (go/dry), consider reviving the constructor that takes the DocumentKey, changing its visibility to private, using that constructor, and then calling the corresponding convertToXXX() method, returning its result.
There was a problem hiding this comment.
I opted to not call into the one argument constructor since keeping it initializes some of the values twice. The one argument constructor would call the default constructor, which then creates new objects, only to be immediately destroyed by the mutating function.
Furthermore, as least some of the code duplication would have to be moved to the one argument constructor.
There was a problem hiding this comment.
How about this idea. The only constructor is MutableDocument(DocumentKey), which initializes this.key and does not assign anything to the other instance variables (they will be initialized to null). If this constructor is called from the factory methods, followed by the corresponding convertToXXX() method, then all instance variables will be initialized and none will be multiply initialized. I realize that this leaves the object temporarily in a weird state after calling the constructor; however, since it is private you have complete control over how the newly-constructed object is used and can prevent ever exposing an object in this intermediate state.
There was a problem hiding this comment.
I like that. Updated.
| * mutations. | ||
| */ | ||
| public static MutableDocument newInvalidDocument(DocumentKey documentKey) { | ||
| return new MutableDocument( |
There was a problem hiding this comment.
How about this idea. The only constructor is MutableDocument(DocumentKey), which initializes this.key and does not assign anything to the other instance variables (they will be initialized to null). If this constructor is called from the factory methods, followed by the corresponding convertToXXX() method, then all instance variables will be initialized and none will be multiply initialized. I realize that this leaves the object temporarily in a weird state after calling the constructor; however, since it is private you have complete control over how the newly-constructed object is used and can prevent ever exposing an object in this intermediate state.
| * <p>Invalid documents serve as base documents for mutations. If a document remains invalid even | ||
| * after all mutations have been applied, it should be removed from all views. | ||
| * <p>Documents can transition between states via {@link | ||
| * #convertToFoundDocument(SnapshotVersion,ObjectValue)}, {@link |
There was a problem hiding this comment.
Optional: For maintainability and brevity of this documentation, you can omit the arguments in the @link tags. For example, {@link #convertToFoundDocument(SnapshotVersion,ObjectValue)} can simply be {@link #convertToFoundDocument}. Both javadoc generation and IDE code navigation will infer the correct arguments.
There was a problem hiding this comment.
Interesting. I thought that is how it worked, but I couldn't get the linking to work. It turns out it only works without ().
|
@schmidt-sebastian: The following test failed, say
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
Some small fixes to MutableDocuments that I noticed while porting to JS (more likely coming). I also added the often asked for factory functions since it helps with readability (otherwise the reader does not know what
new MutableDocumentmeans).