4
\$\begingroup\$

Recently I was reading a book called Code Complete by Steve McConnell and I came across a chapter called The PseudoCode Programming Process(PPP) where the chapter taught me on how to write Pseudo code and gave guidelines on how to convert that pseudo code to real code. That chapter inspired me a lot and I decided to follow that technique for writing all of my code. Inspired from the example given in the book I tried to write a simple routine that nearly follows those guidelines. Can you guys help me out on whether I am going in the right direction after looking the code below.

Here is the Pseudo code and real code that I wrote for a function called getErrorLocation that basically returns the coordinates of where the error occurred in a piece of code while parsing it.

Pseudo Code:

This routine outputs coordinate of error location found in gml code in 
x/80 format based on the startlocation and currentlocation as supplied 
by the calling routine. Also it outputs a error state indicating whether
the operation was successful
or not.

Set default value of error to FAIL

*Precondition
IF currentlocation<startlocation
    return FAIL
Calculate distance by subtracting currentlocation from startlocation
Divide distance by 80 and set the rownumber and colnumber to quotient and remainder of the division result respectively.
Set errorstate to SUCCESS
return coordinate with errorstate

Here is the C++ code written from the Pseudo Code given above

#include<iostream>
#include<cstdlib>

struct Point
{
    int x;
    int y;

};

/*This routine outputs coordinate of error location found in gml code in 
x/80 format based on the startlocation and currentlocation as supplied by
the calling routine. Also it outputs a error state indicating whether the operation 
was successful or not.*/

template<typename Iter>
std::pair<Point, bool>getErrorLocation(Iter locationcurrent, Iter locationstart)
{

    //Set default value of errorstate to FAIL
    bool error = true;

    //*Precondition

    //IF currentlocation<startlocation
    if (locationcurrent < locationstart)
        // return FAIL
        return { {0,0},error };

    //Calculate distance by subtracting currentlocation from startlocation
    int distance = locationcurrent - locationstart;

    //Divide distance by 80 and set the rownumber and colnumber to quotient and remainder of the division result respectively.
    auto [rownum, colnum] = div(distance, 80);
    //Set errorstate to SUCCESS
    error = false;
    //return coordinate with errorstate
    return { {rownum,colnum},error };
}

The main thing I want to highlight here are the comments that were actually the Pseudo code itself. I read in Clean Code by Robert C Martin that comments are harmful for the code and must be used to the minimum. I clearly understand why they are harmful, but I like this method also. Advantages weigh more I think for comments rather than their disadvantages. I feel like writing comments helps novices like me to understand the idea in the first glance itself rather than digging into the technical details and figuring out the code's real purpose. So now I am in a complete dilemma. There are contradictory statements from two of my favorite books.

What should I do?

\$\endgroup\$
  • \$\begingroup\$ Welcome to code review! Form an opinion by reading code (including yours from years ago) and pondering whether existing comments where harmful or helpful. Try identify places where you missed comments and think up ones that would make a real difference and stand a chance of staying helpful when the code gets modified. \$\endgroup\$ – greybeard Aug 3 at 8:51
3
\$\begingroup\$

Writing psuedo-code first, then turning it into code is great. However, we should then delete the pseudo-code.

The problem with comments like this is that they mainly just repeat the code. If we need a comment to explain what the code does, we should make the code clearer instead. Comments that explain why the code does something the way it does are more helpful.

Note also that if we change the code in the future, we would have to update all these comments too, which is twice the work.

Comments for documentation purposes tend to be structured according to the documentation tool in use (e.g. short description, purpose of arguments, return values, etc.). However, they are probably unnecessary for a small function that will be used internally in a project.


Some issues with the code:

  • Do we ever really expect locationStart to be after locationCurrent? Perhaps an assertion, or an exception would be more appropriate.

  • Otherwise std::optional would be a better choice for the return value.

  • We should use appropriate types for indexing. If the range of indices for Iter cannot be negative, and needs to cover the range of a standard container (e.g. std::string), we should use std::size_t, not int.

  • It's probably clearer to use the divide and remainder operators directly, instead of std::div (if that's what div is). Note that std::div, std::ldiv, and std::lldiv all use signed integer types, which probably don't cover the appropriate range.

  • The local error variable is unnecessary.

  • What is 80? Perhaps this should be a function argument (or at least a named constant).

  • We can be more consistent with naming. (The function is getErrorPosition, but we're returning a Point. Perhaps CharacterPosition would be a better name than Point).

  • Although it's called getErrorPosition, this function is not specific to errors - it just gets the character position from the iterators. If we give it a more general name, then we can use it for other things too.


So I'd suggest something like this (though the doc comments are probably not needed):

#include <cassert>
#include <cstdlib>
#include <iterator>

struct CharacterPosition
{
    std::size_t row;
    std::size_t column;
};

/// \brief Returns the character position (row / column) from its index.
/// \param index The distance of the character from start of the document.
/// \param lineLength The line length of the document (all lines must be this length for the returned character position to be correct).
/// \return Row and column of the character.
CharacterPosition getCharacterPosition(std::size_t index, std::size_t lineLength)
{
    return { index / lineLength, index % lineLength };
}

/// \brief Returns the character position (row / column) from its iterator position.
/// \param start An iterator pointing to the first character of the document.
/// \param current An iterator pointing to the current character (for which we will return the position).
/// \param lineLength The line length of the document (all lines must be this length for the returned character position to be correct).
/// \return Row and column of the character.
/// \pre The current iterator must not be positioned before the start iterator
template<typename Iter>
CharacterPosition getCharacterPosition(Iter start, Iter current, std::size_t lineLength)
{
    assert(start <= current);

    auto distance = std::distance(start, current);

    return getCharacterPosition(distance, lineLength);
}
\$\endgroup\$
  • \$\begingroup\$ Sir you are exactly right!!! The routine that you wrote now is much more easily understandable and short. Extremely Thankyou for the help \$\endgroup\$ – Cosmo Ferrito Aug 3 at 9:03

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.