Add XML doccomments to classes and methods #14
Conversation
First pass at adding doc-comments to the methods of the base classes.
There was a problem hiding this comment.
Thanks!
Extra notes:
- I like to limit the length of a line of code to 80 character, even though it is quite annoying to do manually (it's not a big deal if it's not exactly 80, it's just for ease of reading)
- I think it could be worth mentionning the algorithmic complexity of the methods.
| /// <summary> | ||
| /// Links two <see cref="Vertex"/>s together, and may or may not be part of a <see cref="Face"/>. | ||
| /// </summary> | ||
| /// <remarks>Multiple <see cref="Face"/>s can share the same <see cref="Edge"/>.</remarks> |
There was a problem hiding this comment.
The technical note above is worth including as well, as a <remarks>
There was a problem hiding this comment.
The technical note above is worth including as well, as a
<remarks>
I think it would be better to not to, the end user doesn't really need to know how it works internally. As long as the API is clear how it works that is all the user should need to care about.
Also worth noting that XML comments don't really support much in the way of formatting, so it would be rather unsightly.
Maybe an abridged version would be better to include?
Library/Edge.cs
Outdated
| /// <summary> | ||
| /// Returns the other <see cref="Vertex"/> that makes up the <see cref="Edge"/> of the specified <see cref="Vertex"/>. | ||
| /// </summary> | ||
| /// <remarks>Assumes that the specified <see cref="Vertex"/> is one of the vertices that make up the <see cref="Edge"/>; otherwise, behavior is undefined.</remarks> |
There was a problem hiding this comment.
... that make up the <see cref="Edge"/>, i.e. that <see cref="ContainsVertex"/> returns <c>true</c>. Otherwise, **the** behavior is undefined.
There was a problem hiding this comment.
... that make up the <see cref="Edge"/>, i.e. that <see cref="ContainsVertex"/> returns <c>true</c>. Otherwise, **the** behavior is undefined.
That seems implicitly stated, more so if you read the ContainsVertex comment first. Also I think it is best to avoid referencing the behavior of other methods because they could change or be removed and the comment would then be inaccurate.
Library/Loop.cs
Outdated
| * the same edge. | ||
| */ | ||
| /// <summary> | ||
| /// Represents a portion of a <see cref="Face"/>. |
There was a problem hiding this comment.
All above comment again :) What is a bit misleading is that a Loop object can be seen either as representing the whole loop or one node (i.e. element) of this loop. It's a bit like with a linked list:
class LinkedList {
object head;
LinkedList tail;
}Here a LinkedList technically only contains the data of the head, so only one of the nodes of the list, but it can be thought as the whole list as well. It's more subtle with the Loop because first it's a linked list with the end connected to the beginning, so that there is no item flagged as "end" and "beginning" actually, any item can represent the whole loop; and secondly it is also used as an element/node of another looped list, which contain all the edges which share a same vertex.
Maybe the most intuitive way to describe a loop item is to tell it is the combination of a vertex, an edge and a face, and that it is meant to provide fast access to neighboring edges, either by "turning around" the face (Next/Prev) or "turning around" the vertex (RadialNext/RadialPrev).
(BTW I realized lately that a face can be made of several sequences of loop actually, say if it contains a hole in the middle, but this is an edge case we don't support here)
There was a problem hiding this comment.
To be honest, I feel like the technical comment doesn't actually clear anything up, at least for me. Though I also don't like the doc comment I write since it doesn't really tell anything useful about the class itself.
While writing the doc comments, I am trying to get away from telling the user how the underlining structure works, and instead tell them how it works in the context of the rest of the API. While my doc comment for the Loop isn't great, I feel like it at least illustrates what I mean. It tells you what the Loop is in relation to the rest of the API.
Maybe the most intuitive way to describe a loop item is to tell it is the combination of a vertex, an edge...
I think that this is the most understandable way I have seen to explain a Loop.
There was a problem hiding this comment.
"turning around" the vertex (RadialNext/RadialPrev).
What are you talking about here?
There was a problem hiding this comment.
If you don't understand the technical comment don't include it to <summary> but please don't remove it it is at least useful for myself to remember!
There was a problem hiding this comment.
I need to look at it more, but I think that diagram helps. Of course! I didn't remove any of the technical comments on the classes, I think they are important when working with the source code to describe how and why things are structured!
I just don't think that info is needed/should be given when working with the API (Apposed to reading/modifying the source code)
| * (Used in constructor) | ||
| */ | ||
| /// <summary> | ||
| /// Insert the <see cref="Loop"/> in to the linked list of the specified <see cref="Face"/>. |
There was a problem hiding this comment.
The note "Used in constructor" meant that this was likely not supposed to be used by external code, but exposed publicly anyway in case people would like to do weird things.
There was a problem hiding this comment.
I kind of feel like it just shouldn't be public then. If there is something that it enables the user to do, then maybe it should have a proper API, but otherwise I think it should be private since I don't think we should let the user explicitly break things like this.
| /// </summary> | ||
| /// <remarks> | ||
| /// Multiple <see cref="Edge"/>s and <see cref="Face"/>s can use the same <see cref="Vertex"/>, and multiple vertices can be located at the exact same position. | ||
| /// </remarks> |
There was a problem hiding this comment.
Add the other remark about data structure details.
There was a problem hiding this comment.
And what I said about the Loop comment goes for here too. I feel like some of the info in the technical comment could be added, but I don't think that it should all be added.
There was a problem hiding this comment.
So same answer ^^ Put it in a remark or even in an invented <dev> téag but still include the whole of it because I need it to remember.
|
At this point almost all the methods in The Attribute methods in In I also have included more of the technical comments in remarks. I think at this point it is good for a 'doc comments part one'. |
Add doccomments to classes and methods that follow the styling of other C# doccomments.
This will need to be thoroughly reviewed to ensure that the accuracy of the comments.
Current State
Only the
Vertex, Edge, Loop, Faceclass are commented so far.All comments in the
Loopclass including the one on the class it self need to be rewritten for more clarity in what they do, however I am not 100% clear on what they are doing my self.Edge.SetNextandEdge.SetPrevare not commented as I was not able to understand what they do in enough clarity to put in to a comment.