Skip to content

Conversation

@nickldp
Copy link
Contributor

@nickldp nickldp commented Jun 21, 2023

Pull Request Info

PR Reviewing Guidelines

JIRA - https://jira.mongodb.org/browse/DOCSP-28793
Staging - https://docs-mongodbcom-staging.corp.mongodb.com/csharp/docsworker-xlarge/DOCSP-28793-count-fundamentals/fundamentals/crud/read-operations/count/

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?

Comment on lines 205 to 206
You can also include the :manual:`$count </reference/operator/aggregation/count/>`
stage to count the number of documents in an aggregation pipeline.
Copy link
Contributor

Choose a reason for hiding this comment

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

Issue: Instead of linking to the $count documentation, mention that this method uses the Count() builder method. In the additional information section, you could link to the Server documentation

Comment on lines 213 to 214
- Counts the number of documents where the ``finalGrade`` is greater than ``80``
- Assigns the count to the ``count`` field
Copy link
Contributor

Choose a reason for hiding this comment

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

I: this isn't exactly what's happening in this code -- it seems like you are specifying a filter in the match stage and then appending a count stage.

Comment on lines 222 to 226
var matchStage = Builders<Student>
.Filter.Gt(f => f.finalGrade, 80);
var pipeline = new EmptyPipelineDefinition<Student>()
.Match(matchStage).Count();
Console.WriteLine(pipeline);
Copy link
Contributor

Choose a reason for hiding this comment

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

I: format so that newlined method calls are indented. Also, this code should print the result of the aggregation. LMK if you want to review this code together.

@@ -0,0 +1,7 @@
//start-grades-struct
public class Student {
public ObjectId Id { get; set; }
Copy link
Contributor

Choose a reason for hiding this comment

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

I: Since in your sample data, the id field is ints, make sure that corresponds to the type here

Suggested change
public ObjectId Id { get; set; }
public int Id { get; set; }

@nickldp nickldp requested a review from rustagir June 21, 2023 20:05
Copy link
Contributor

@rustagir rustagir left a comment

Choose a reason for hiding this comment

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

a few more things

{ "_id": 6, "name": "Demarcus Smith", "finalGrade": 88.8 }

The following ``Student`` class models the documents in this
collection.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
collection.
collection:

Comment on lines 76 to 78
var filter = Builders<Student>.Filter.Lt(s => s.FinalGrade, 80.0);
var count = _myColl.CountDocuments(filter);
Console.WriteLine("Number of documents with a final grade less than 80: " + count);
Copy link
Contributor

Choose a reason for hiding this comment

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

I: fix indentation formatting


var filter = Builders<Student>.Filter.Empty;
CountOptions opts = new CountOptions(){Hint = "_id_"};
var counter = collection.CountDocuments(filter, opts);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
var counter = collection.CountDocuments(filter, opts);
var count = collection.CountDocuments(filter, opts);


.. note::

The ``EstimatedDocumentCount()`` method is quicker than the
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
The ``EstimatedDocumentCount()`` method is quicker than the
The ``EstimatedDocumentCount()`` method is more efficient than the

Comment on lines 176 to 190
.. io-code-block::
:copyable: true

.. input::
:language: csharp
:dedent:

var count = _myColl.EstimatedDocumentCount();
Console.WriteLine("Estimated number of documents in the students collection: " + count);

.. output::
:language: none
:visible: false

Estimated number of documents in the students collection: 6
Copy link
Contributor

Choose a reason for hiding this comment

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

I: example is not showing up in staging because of indentation errors
S: fix errors (text should be aligned with directive)

Aggregation
-----------

You can also include the ``Count()`` builder method to count the number
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
You can also include the ``Count()`` builder method to count the number
You can use the ``Count()`` builder method to count the number

Comment on lines 205 to 206
- Specifies a filter in the match stage
- Appends a count stage using ``Count()``
Copy link
Contributor

Choose a reason for hiding this comment

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

S: describe what is happening in each stage

Suggested change
- Specifies a filter in the match stage
- Appends a count stage using ``Count()``
- Specifies a match stage to find documents with a ``FinalGrade`` value greater than ``80``
- Counts the number of documents that match the criteria

Comment on lines 214 to 217
var matchStage = Builders<Student>
.Filter.Gt(s => s.FinalGrade, 80);
var result = _myColl.Aggregate().Match(matchStage).Count();
Console.WriteLine("Number of documents with a final grade more than 80: " + result.First().Count);
Copy link
Contributor

Choose a reason for hiding this comment

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

I: indentation error, also, these variables should be renamed correctly

Suggested change
var matchStage = Builders<Student>
.Filter.Gt(s => s.FinalGrade, 80);
var result = _myColl.Aggregate().Match(matchStage).Count();
Console.WriteLine("Number of documents with a final grade more than 80: " + result.First().Count);
var filter = Builders<Student>
.Filter.Gt(s => s.FinalGrade, 80);
var result = _myColl.Aggregate().Match(filter).Count();
Console.WriteLine("Number of documents with a final grade more than 80: " + result.First().Count);

Comment on lines 37 to 39
new() { Id= 1, Name = "Jonathon Howard ", FinalGrade = 87.5 },
new() { Id= 2, Name = "ABCDEF Howard ", FinalGrade = 10.5 },
new() { Id= 3, Name = "aeufh Howard ", FinalGrade = 90.5 }
Copy link
Contributor

Choose a reason for hiding this comment

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

I: since this is a public repo, make sure that the sample code matches what's in the page.
S: replace with the same sample data as in the page.

Copy link
Contributor

Choose a reason for hiding this comment

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

Bumping this comment since I dont think you saw it!

@nickldp nickldp requested a review from rustagir June 23, 2023 14:51
Copy link
Contributor

@rustagir rustagir left a comment

Choose a reason for hiding this comment

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

lgtm but consider changing the two outstanding comments! great work here

- :ref:`csharp-bson`
- :ref:`csharp-guids`
- :ref:`csharp-builders`
- :ref:`csharp-poco`
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion: add a link to the $count documentation in the Server manual

Comment on lines 37 to 39
new() { Id= 1, Name = "Jonathon Howard ", FinalGrade = 87.5 },
new() { Id= 2, Name = "ABCDEF Howard ", FinalGrade = 10.5 },
new() { Id= 3, Name = "aeufh Howard ", FinalGrade = 90.5 }
Copy link
Contributor

Choose a reason for hiding this comment

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

Bumping this comment since I dont think you saw it!

@nickldp nickldp merged commit 173895b into mongodb:master Jun 23, 2023
nickldp added a commit that referenced this pull request Jun 23, 2023
nickldp added a commit that referenced this pull request Jun 28, 2023
* DOCSP-28793: Count Fundementals

* test

* test

* test

* test

* test

* up through estimated

* up through estimated

* aggregate

* final?

* final?

* final?

* final

* api fix

* final except error

* final

* post review

* check before and after

* post review

* error fix

* error fix

* error fix

* error fix

* error fix

* error fix

* examples too long fix

* changes requested

* changes requested

(cherry picked from commit 173895b)
nickldp added a commit that referenced this pull request Jun 28, 2023
* DOCSP-28793: Count Fundementals

* test

* test

* test

* test

* test

* up through estimated

* up through estimated

* aggregate

* final?

* final?

* final?

* final

* api fix

* final except error

* final

* post review

* check before and after

* post review

* error fix

* error fix

* error fix

* error fix

* error fix

* error fix

* examples too long fix

* changes requested

* changes requested

(cherry picked from commit 173895b)
mongoKart pushed a commit to mongoKart/docs-csharp that referenced this pull request May 16, 2025
* DOCSP-28793: Count Fundementals

* test

* test

* test

* test

* test

* up through estimated

* up through estimated

* aggregate

* final?

* final?

* final?

* final

* api fix

* final except error

* final

* post review

* check before and after

* post review

* error fix

* error fix

* error fix

* error fix

* error fix

* error fix

* examples too long fix

* changes requested

* changes requested
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants