DOCSP-33345: Java comments pt. 4 #472

norareidy · 2023-10-10T18:36:50Z

Pull Request Info

JIRA - https://jira.mongodb.org/browse/DOCSP-33345
Staging:
Aggregation - https://preview-mongodbnorareidy.gatsbyjs.io/java/DOCSP-33345-java-comments-4/fundamentals/aggregation/
BulkWrite - https://preview-mongodbnorareidy.gatsbyjs.io/java/DOCSP-33345-java-comments-4/fundamentals/crud/write-operations/bulk/
Collation - https://preview-mongodbnorareidy.gatsbyjs.io/java/DOCSP-33345-java-comments-4/fundamentals/collations/
Compound operators and individual example - https://preview-mongodbnorareidy.gatsbyjs.io/java/DOCSP-33345-java-comments-4/fundamentals/crud/compound-operations/

Self-Review Checklist

Is this free of any warnings or errors in the RST?
Did you run a spell-check?
Did you run a grammar-check?
Are all the links working?

ccho-mongodb

Lots of great work here.

I think making the comments more consistent would be helpful in understanding the code, both by users and LLMs.

I think it might be helpful to keep track of (perhaps a separate doc) what code should be commented and which should not. I think we paired on some of this, but happy to contribute to a more formal list. I think it's a little unclear to us since we haven't gotten detailed feedback from CodeWhisperer, so maybe that list could help us identify the concerns.

source/includes/fundamentals/code-snippets/AggTour.java

source/includes/fundamentals/code-snippets/BulkWrite.java

source/includes/fundamentals/code-snippets/CompoundOperators.java

source/includes/fundamentals/code-snippets/CompoundOperatorsIndividualExamples.java

source/includes/fundamentals/code-snippets/BulkWrite.java

source/includes/fundamentals/code-snippets/CollationCollectionExample.java

source/includes/fundamentals/code-snippets/AggTour.java

ccho-mongodb

Looks good. There are a couple issues to address before merging, but approving since they're straightforward fixes.

ccho-mongodb · 2023-10-25T21:43:58Z

source/includes/fundamentals/code-snippets/BulkWrite.java

            collection.bulkWrite(bulkOperations); 

+        // Prints a message if any exceptions occur during the operations


Per the code comments sheet:

Suggested change

// Prints a message if any exceptions occur during the operations

// Prints a message if any exceptions occur during the bulk write operation

ccho-mongodb · 2023-10-25T21:45:17Z

source/includes/fundamentals/code-snippets/BulkWrite.java

        InsertOneModel<Document> insertDoc = new InsertOneModel<>(new Document("_id", 6)
                                                                .append("name", "Zaynab Omar")
                                                                .append("age", 37));
+
+        // Creates instructions to replace the matching document


Per the code comments sheet:

Suggested change

// Creates instructions to replace the matching document

// Creates instructions to replace the first document that matches the query

ccho-mongodb · 2023-10-25T21:49:41Z

source/includes/fundamentals/code-snippets/BulkWrite.java

        UpdateOneModel<Document> updateDoc = new UpdateOneModel<>(Filters.eq("name", "Zaynab Omar"), 
                                                Updates.set("name", "Zaynab Hassan"));
+
+        // Creates instructions to delete all matching documents


Suggestion:

I think it would be good for consistency to mention matching the query. Added to the code comment sheet.

Suggested change

// Creates instructions to delete all matching documents

// Creates instructions to delete all documents that match the query

ccho-mongodb · 2023-10-25T21:52:19Z

source/includes/fundamentals/code-snippets/BulkWrite.java

+                                                                .append("name", "Zaynab Omar")
+                                                                .append("age", 37));
+
+        // Creates instructions to replace the matching document                                            


Suggestion:

I think it's important to mention the first document matched by the query for all instances of "OneModel"s as described on the code comments sheet as it is otherwise inaccurate for the case in which there are more than one matches:

Suggested change

// Creates instructions to replace the matching document

// Creates instructions to replace the matching document

Applies to all instances.

ccho-mongodb · 2023-10-25T21:57:49Z

source/includes/fundamentals/code-snippets/CollationCollectionExample.java

                .collation(Collation.builder().locale("de").collationStrength(CollationStrength.PRIMARY).build());
+
+        // Prints the results of the aggregation that tallied value frequencies and sorted the results


Suggestion:

I think it could be good to keep the description consistent (e.g. line 65) within the file:

Suggested change

// Prints the results of the aggregation that tallied value frequencies and sorted the results

// Prints the JSON representation of the results

source/includes/fundamentals/code-snippets/BulkWrite.java

ccho-mongodb · 2023-10-25T22:13:54Z

source/includes/fundamentals/code-snippets/CompoundOperators.java

        Bson filter = Filters.eq("reserved", false);
+
+        // Atomically finds and updates a document to mark it as reserved


Suggestion:

I think we landed on the following for findOneAndDelete which should be applied to findOneAndUpdate in the code comments sheet:

Suggested change

// Atomically finds and updates a document to mark it as reserved

// Updates the first document that matches the filter to mark it as reserved

Applies to https://github.com/mongodb/docs-java/pull/472/files#diff-3d6441156a561267d83bde841144e80a788371a97373d78a95825cb1d584c919R118

ccho-mongodb · 2023-10-25T22:16:05Z

source/includes/fundamentals/code-snippets/CompoundOperatorsIndividualExamples.java

@@ -6,6 +6,9 @@
 import com.mongodb.client.MongoDatabase;
 import com.mongodb.client.model.*;
 import com.mongodb.client.result.InsertOneResult;
+
+import docs.builders.Filters;


Question:
Is this import necessary? From a quick scan, I can't tell which file is in the docs.builders package. Please let me know if there's any reason not to delete it.

ccho-mongodb · 2023-10-25T22:19:17Z

source/includes/fundamentals/code-snippets/CompoundOperatorsIndividualExamples.java

@@ -49,16 +56,26 @@ private void findOneAndUpdateExample() {
        MongoCollection<Document> collection = getCollection();
        //start findOneAndUpdate-example
        // <MongoCollection set up code here>
+
+        // Creates a projection to exclude the "_id" field from the printed documents


Issue:

I think printed documents is inaccurate. The projection tells the server which fields to return.

Suggestion:

I think it would be better to use the language "retrieved documents" similar to the descriptions of the projection method used in the code comments sheet:

Suggested change

// Creates a projection to exclude the "_id" field from the printed documents

// Creates a projection to exclude the "_id" field from the retrieved documents

ccho-mongodb · 2023-10-25T22:26:54Z

source/includes/fundamentals/code-snippets/CollationCollectionExample.java

@@ -67,18 +79,24 @@ private static void findOneAndUpdateExample(MongoCollection<Document> collection
                        .collation(Collation.builder().locale("de@collation=phonebook").build())
                        .sort(Sorts.ascending("first_name"))
                        .returnDocument(ReturnDocument.AFTER));
+
+        // Prints the JSON representation of the updated document, if an update occurred           


Suggestion:

I think it would be more grammatical to omit this comma.

Suggested change

// Prints the JSON representation of the updated document, if an update occurred

// Prints the JSON representation of the updated document if an update occurred

…comments-4

* DOCSP-33345: Java code comments pt. 4 * bulkwrite CC feedback * applying CC feedback * small edit * half of feedback * rest of feedback * single line comment * duplicate comments sheet suggestions * small edit * final feedback (cherry picked from commit 4b9070c)

* DOCSP-33345: Java code comments pt. 4 * bulkwrite CC feedback * applying CC feedback * small edit * half of feedback * rest of feedback * single line comment * duplicate comments sheet suggestions * small edit * final feedback (cherry picked from commit 4b9070c) (cherry picked from commit fd983e9)

DOCSP-33345: Java code comments pt. 4

ce57a6d

norareidy force-pushed the DOCSP-33345-java-comments-4 branch from d81935b to ce57a6d Compare October 10, 2023 18:48

norareidy added 3 commits October 12, 2023 17:41

bulkwrite CC feedback

e9bc6df

applying CC feedback

b001ba6

small edit

b6290a7

ccho-mongodb suggested changes Oct 16, 2023

View reviewed changes

half of feedback

a748ddf

ccho-mongodb reviewed Oct 17, 2023

View reviewed changes

source/includes/fundamentals/code-snippets/BulkWrite.java Show resolved Hide resolved

ccho-mongodb reviewed Oct 17, 2023

View reviewed changes

source/includes/fundamentals/code-snippets/CollationCollectionExample.java Outdated Show resolved Hide resolved

ccho-mongodb reviewed Oct 17, 2023

View reviewed changes

source/includes/fundamentals/code-snippets/AggTour.java Outdated Show resolved Hide resolved

norareidy added 4 commits October 17, 2023 14:00

rest of feedback

3630611

single line comment

2ae0dc9

duplicate comments sheet suggestions

e6c270b

small edit

ec30862

norareidy requested a review from ccho-mongodb October 19, 2023 14:42

ccho-mongodb approved these changes Oct 25, 2023

View reviewed changes

norareidy added 2 commits October 27, 2023 13:20

final feedback

68f46ec

Merge remote-tracking branch 'upstream/master' into DOCSP-33345-java-…

55966ff

…comments-4

norareidy merged commit 4b9070c into mongodb:master Oct 27, 2023

norareidy deleted the DOCSP-33345-java-comments-4 branch October 27, 2023 17:31

		collection.bulkWrite(bulkOperations);

		// Prints a message if any exceptions occur during the operations

	// Creates instructions to replace the matching document
	// Creates instructions to replace the first document that matches the query

	// Creates instructions to delete all matching documents
	// Creates instructions to delete all documents that match the query

		.collation(Collation.builder().locale("de").collationStrength(CollationStrength.PRIMARY).build());

		// Prints the results of the aggregation that tallied value frequencies and sorted the results

	// Prints the results of the aggregation that tallied value frequencies and sorted the results
	// Prints the JSON representation of the results

		Bson filter = Filters.eq("reserved", false);

		// Atomically finds and updates a document to mark it as reserved

	// Atomically finds and updates a document to mark it as reserved
	// Updates the first document that matches the filter to mark it as reserved

	// Creates a projection to exclude the "_id" field from the printed documents
	// Creates a projection to exclude the "_id" field from the retrieved documents

DOCSP-33345: Java comments pt. 4 #472

DOCSP-33345: Java comments pt. 4 #472

Uh oh!

Conversation

norareidy commented Oct 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Info

Self-Review Checklist

Uh oh!

ccho-mongodb left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ccho-mongodb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

norareidy commented Oct 10, 2023 •

edited

Loading