Self-Documenting Code

[LB]

I've been talking about this for a while now and I've promised to elaborate in an article multiples now, so here it is finally:

http://www.LB-Stuff.com/self-documenting.html

I'm not confident that I conveyed the point I wanted to, but hopefully I did. I encourage discussion on this topic as it is a relatively vague and abstract topic that needs discussion.

[MiiNiPaa]

You forgot about useful rule: name arguments in prototypes too.

article wrote:

void drawRectangle(Point, Dimension);
[...]
The second takes a point and a dimension, and a rectangle can be drawn with just a point, a width, and a height. It's still slightly ambiguous though, as we don't know if the given point is the center or the top-left corner,

Compare:

void drawRectangle(Point, Dimension);
//VS
void drawRectangle(Point top_left, Dimension dimensions);

[LB]

Yes, you're right, but I was considering the worst case scenario of useless argument names. I'll adjust the article for you though.

[MrHutch]

You're not alone - my current workplace discourages commenting code.

Thought it was bizarre when I started but it actually is a lot easier to read and less tedious to code.

To be fair, it's mostly related to MiiNiPaa's comment, though, and we tend to try and make variable names descriptive and completely unambiguous.

For example, we'd maybe have:

void DrawRectangleFromOrigin(const vec2 &origin, const vec2 &width, const vec2 &height);

void DrawRectangleFromBounds(const vec2 &topLeft,
                             const vec2 &topRight,
                             const vec2 &bottomLeft,
                             const vec2 &bottomRight);

We're sort of on the Hungarian wagon a bit, too. Not fully and not religiously. Don't really use it for data types (I guess it could become a pain if they change) other than pointers. We do use the m_ prefix for members, though, and the other odd feature, such as a k prefix for (non-local) constants.

[xander333]

Quite hard to read the articles on your site man, please use a fixed width for the paragraphs.

[MiiNiPaa]

my current workplace discourages commenting code.

Why?

We use javadoc style comments to document our functions. There is many things you cannot tell though the name

For example: your DrawRectangleFromOrigin. What color will that rectangle be? Will it be filled? Does it depend on global state (like PenColor/FillStyle)? Where will it be drawn? What is width? Percentage of drawing surface width? Pixels? Centimeters? Same for origin.

Some reference-style comment would be nice:

//Draws a rectange on drawing surface specified by global variable CurrentDrawingContext
//Color and style depends on DrawingTools of drawing surface
//@origin: distance from top-left corner in pixels
//@width, height: width and height in pixels
//@return: none
//@throw: IllegalArgument if either width or height is 0 or any pixel of resulting rectangle is outside drawing surface 

Some of it is redundant, but it is really good to automaticly make a reference for your library.

Some other comments I think were needed:

//A single shaker sort pass: Leaderboard cannot change drastically in 1 frame
//so it is enough. O(n) FTW.

//Bit hack to place 4 floats in 128 bit variable
//DO NOT change to function call unless you want extreme slowdown. 

//Balances bug in library we cannot change. Do not "fix" 

[LB]

I think the reason iHutch's workplace discourages comments is for these three reasons:
- it slows down development time to write comments
- it slows down development time to read comments
- if the comment is non-trivial, the code is poorly written

@xander: I'm still learning CSS, do you have a recommendation about how I can change my light.css? I'm considering something along the lines of making the body tag have a width of 75%

EDIT: I changed the left and right padding to 10% each, does that look better?

[MrHutch]

Because the code is much less polluted that way. We only really use them where necessary.

The rectangle example was exactly that, an example, but it could be expanded. Colour and whether it's filled could well be easily described arguments (vec4 colour, bool isFilled). My point isn't the method itself or the number of arguments; I was trying to highlight the naming style.

I should point out that we're a pretty small team (8) of engineers working on a very specific project. I guess on a bigger scale, comments would be slightly more necessary. I previously worked for a company that (as of 2011) had 3.5 millions lines of comments in their system (yuck).

Your second comment example is fair enough and present in what we do. We tend to comment when the code looks slightly odd. The mantra is that you should be able to look at the code itself and understand what it's doing.

The first comment example is hideous, though. Most of that information can be easily inferred and some of it is just plain unnecessary. Again, maybe less so on a bigger scale.

Edit: Quite right, LB. In particular the third point. Also, the padding does make the article a little easier on the eyes. :-)

[MatthewRock]

If I may suggest something - I would change background to be dark. Maybe it's just me, but it's much much easier for me to read bright or not-too-dark text from dark background(deep blue or black, for example), than from light one.

[MiiNiPaa]

The first comment example is hideous, though. Most of that information can be easily inferred and some of it is just plain unnecessary

It is comment for automatic documentation generation. Each night fresh and up to date html documentation will be generated from those comments. It looks like this: http://www.sfml-dev.org/documentation/2.1/classsf_1_1Sprite.php

You cannot really make it less verbose, but you can make it start collapsed and invisible to you (Use your IDE to the fullest)

[LB]

@MatthewRock: If you're using Chrome, press F12 to open the dev console and in the HTML head tag, edit the stylesheet from light.css to dark.css and tell me what you think. I'm personally not a fan of light-on-dark, but I'm also a terrible artist and suck at colors.

[MrHutch]

I get it, I've seem similar things, like Doxygen. I still think it's overkill, though. Of the information there, the functionality and arguments could be inferred by name. The return type is present in the declaration and implementation.

The only thing that might be of use is the throw. However, I'd expect to be able to see that with reasonable ease within the function, keeping in mind that the function itself should be reasonably sized.

[LB]

Exception handling is still a touchy subject for me, I can't make up my mind on it. As such, I have no idea how you would write self-documenting code with exceptions - maybe something with the comma operator and noexcept(false).

[htirwin]

I think that documentation is absolutely crucial, comments in the code is optional.

Providing no documentation for something that other people are going to be attempting to use is cruel in my opinion.

[MiiNiPaa]

The example is too simple.
Here is standard library one:

template< class InputIt, class UnaryPredicate >
InputIt find_if( InputIt first, InputIt last, UnaryPredicate p );

What is returned if nothing is found? What constraints are placed on p? Can it be statefull functor? Will predicate be applied consequentally? Can we be sure that it will not change in future?

I am not saying that it should be used by everyone. But if you are writing generic library which will be heavely reused, you should consider it: it saves time writing documentation, it is less likely that someone forge to update comment right before function that some text file in the depth of project, you can check if doc was updated properly in commit history.


Back to the article discussion. One problem with using aliases to classes which currenly works for you, but actually have a wider range, is that once you might notice that there is Suit_t::npos used everywere. So to lessen chance for that to happen, you should document possible usage of your class (i.e. Suit_t can be compared using equal and not equal operator, can be assigned values from SUITS container and can be assigned to other Suit_t)

[LB]

MiiNiPaa wrote:

What is returned if nothing is found?

Consistent with the rest of the C++ standard library, last is returned. Since this is consistent it only has to be learned once and recognized everywhere.

MiiNiPaa wrote:

What constraints are placed on p?

The constraints of a UnaryPredicate? It's described once and consistent everywhere.

MiiNiPaa wrote:

Can it be statefull functor?

Considering how it is passed to the function, yes.

MiiNiPaa wrote:

Will predicate be applied consequentally?

?

MiiNiPaa wrote:

Can we be sure that it will not change in future?

?

MiiNiPaa wrote:

Back to the article discussion. One problem with using aliases to classes which currenly works for you, but actually have a wider range, is that once you might notice that there is Suit_t::npos used everywere. So to lessen chance for that to happen, you should document possible usage of your class (i.e. Suit_t can be compared using equal and not equal operator, can be assigned values from SUITS container and can be assigned to other Suit_t)

Did you read only the odd-numbered sentences or something?

[MiiNiPaa]

LB wrote:

I have no idea how you would write self-documenting code with exceptions

double arcsin(double value)
{
    if (value < -1 || value > 1) //1st layer of self-documentation
        throw domain_error(format(
                            "arcsin defined in range [-1;1] value passed was: %1", value) ) //2nd layer 

[htirwin]

I don't like the idea of having to make assumptions about the behavior and use of something. Everything should have a clear unambiguous documentation that defines the use and behavior of a module and that definition itself should ideally be modular. You should not need to go searching around to put all of the pieces together if you haven't memorized everything, and you should not be required make any assumptions. Everything you need to know about the use should be in the form of a guarantee.

[MiiNiPaa]

Did you read only the odd-numbered sentences or something?

I do not know how I managed to miss tat sentence.
Still I prefer to say that self-documenting code should only have one right way to use it. Simple adapter which allow you to use only allowed methods will save you from someone who decide to use non-documented features of your type.

Consistent with the rest of the C++ standard library

It was an example.
Actually it was one point I wanted to say: consistency across your code and basically usage of Concepts will make your code cleanier and easier to use.
But still there will be cases when you cannot be sure on constraints on arguments and possible return values. Can your function returning double return a NaN? Or infinity? Can destination be the same as beginning? And other similar things. Like whole C library.

Considering how it is passed to the function, yes.

Scott Meyers thinks that you should not use stateful functors with standard library algorithms.

?

Will that work?

class functor
{
int x;
public: 
functor() : x(0) {}
bool operator()(int x)
{
    if (x < 0)
        ++x;
    if (x == 3)
        return true;
    return false;
}
//return third negative value:
auto x = find_if( begin, end, functor());

[LB]

@MiiNiPaa::exceptions: I meant, to self-document how the function deals with exception handling. Suppose the implementation is inaccessible.

@htirwin: The goal is to make the guarantees based on the common assumptions, not the other way around.

@MiiNiPaa: Simple adapter does not stop someone from doing memory magic can getting the wrapped object.

I'm not a fan of the C library :)

MiiNiPaa wrote:

Scott Meyers thinks that you should not use stateful functors with standard library algorithms.

If the operator() function is not const, then yeah. But that doesn't mean it can't have a state that it checks.