Text rendering for a Node class in SFML

Question

I needed a method which would print a square with 4 values in each corner, my current "prototype" looks like this:

void Renderer::drawNode(sf::RenderWindow &window, const Node &node)
 {
 float nodesHor = static_cast<float>(node.location.width);
 float nodesVert = static_cast<float>(node.location.height);
 float sideLength = static_cast<float>(NODE_SIDE_LENGTH);
 float positionHor = nodesHor * sideLength;
 float positionVer = nodesVert * sideLength;
 nodeRect.setPosition(sf::Vector2f(positionVer, positionHor));
 nodeRect.setFillColor(stateColor(node.state));
 window.draw(nodeRect);
 auto giveInf = [](int32_t num){
 if(num == std::numeric_limits<int32_t>::max())
 {
 return std::string("inf");
 }
 else
 return std::to_string(num);
 };
 if (appStatePtr->renderNodeInfo)
 {
 text.setString(giveInf(node.g));
 text.setPosition(sf::Vector2f(positionVer + NODE_INFO_OFFSET, positionHor + NODE_INFO_OFFSET));
 window.draw(text);
 float widthOfGText = text.getLocalBounds().width;
 text.setString(giveInf(node.rhs));
 float widthOfRHSText = text.getLocalBounds().width;
 float freeSpaceHor = NODE_SIDE_LENGTH - widthOfGText - widthOfRHSText;
 text.setPosition(sf::Vector2f(positionVer + freeSpaceHor + widthOfGText -NODE_INFO_OFFSET, positionHor + NODE_INFO_OFFSET));
 window.draw(text);
 float textHeight = text.getLocalBounds().height;
 float freeSpaceVert = NODE_SIDE_LENGTH - 2 * textHeight;
 text.setString(giveInf(node.key.k1));
 text.setPosition(sf::Vector2f(positionVer + NODE_INFO_OFFSET, positionHor + freeSpaceVert + textHeight - NODE_INFO_OFFSET));
 window.draw(text);
 float widthOfK1Text = text.getLocalBounds().width;
 text.setString(giveInf(node.key.k2));
 float widthOfK2Text = text.getLocalBounds().width;
 freeSpaceHor = NODE_SIDE_LENGTH - widthOfK1Text - widthOfK2Text;
 text.setPosition(sf::Vector2f(positionVer + widthOfK1Text + freeSpaceHor - NODE_INFO_OFFSET, positionHor + freeSpaceVert + textHeight - NODE_INFO_OFFSET));
 window.draw(text);
 }
 }

How unreadable is this? How could I improve the readability?

• pacmaninbw: For those in the Close Vote Queue, while this only one function and doesn't include the header files, there is a very good answer on it that does review the code. My personal opinion is this is a good question.
• user673679: I've changed the title to match the site guidelines ( codereview.stackexchange.com/help/how-to-ask ). Feel free to update it if you think it could be improved.

Answer 1

Be consistent

Follow consistent rules of code layout, so as to ease the reader.

For this code:

 if(num == std::numeric_limits<int32_t>::max())
 {
 return std::string("inf");
 }
 else
 return std::to_string(num);

1. Either both if and else should have curly brackets or both shouldn't.
2. The indentation of the returns should be same.

Use ternary operators.

For very simple logic - ternary operators allow to simplify and condense coded logic.

This code:

 auto giveInf = [](int32_t num){
 if(num == std::numeric_limits<int32_t>::max())
 {
 return std::string("inf");
 }
 else
 return std::to_string(num);
 };

becomes:

auto giveInf = [](int32_t num)
{
 return (num == std::numeric_limits<int32_t>::max()) //
 ? std::string("inf") //
 : std::to_string(num); //
};

Do you really need a lambda (in this scope)?

The giveinf lambda does not make use of any other external variable.

It is not passed into any function as a parameter.

It looks generic enough to be useful in other code.

Consider moving it out of function scope and declaring it as a global/static function.

Naming is HARD

The name giveinf does not describe what it does. As a result of this function I would expect an "inf something" to return from this function.

Maybe a more suitable name would be "convertToString"?

Happy path and early return

(Quick read: Align the happy path to the left edge)

As I understand it the "happy path" of this code is to draw a node (that's what name says).

To this effect everything required to achieve this goal, should be in the first indentation.

So use an early return to get the code out of the if, by inverting the if:

 if (appStatePtr->renderNodeInfo)
 {
 ...
 }
 

becomes:

 if (!appStatePtr->renderNodeInfo)
 {
 return;
 }
 ...

Divide and conquer (the complexity)

Generally code should be self-describing.

If it's not easily visible what the function is doing, then:

1. If the logic is simple (like it is here) consider adding comments, naming blocks of code.
2. With more complex logic or very long pieces of code - consider moving the code to a function.

Consider applying this to the code currently inside the if.