# Code Smells in ROS Code

**URL:** <https://discourse.openrobotics.org/t/code-smells-in-ros-code/19905>\
**Category:** Quality Assurance\
**Created:** [April 13, 2021, 5:56pm UTC](https://discourse.openrobotics.org/t/code-smells-in-ros-code/19905 "2021-04-13T17:56:28Z")\
**Posts on this page:** 7\
**Page:** 1

<div class="post-metadata">

**Author:** ![afsantos](https://sea2.discourse-cdn.com/flex022/user_avatar/discourse.openrobotics.org/afsantos/32/1966_2.png) [@afsantos](https://discourse.openrobotics.org/u/afsantos)\
**Post date:** [April 13, 2021, 5:56pm UTC](https://discourse.openrobotics.org/t/code-smells-in-ros-code/19905/1 "2021-04-13T17:56:28Z")

</div>

In the General forum, @tareq97 [asked](http://discourse.ros.org/t/are-there-any-out-standing-issues-that-need-to-be-addressed-in-software-quality-of-the-ros/19484/12) me to share some dubious code patterns (code smells?) I have observed over the years in various ROS projects. He is working on a static analysis tool, and adding checks for such patterns would be a nice addition to his tool.

I thought such a discussion would fit better in the QA forum, and so I will share here a small list of code patterns along with _my_ reasoning for finding them questionable. Some of them are found in public repositories, but I will refrain from citing the sources (_blaming_, or _shaming_), unless asked to. Feel free to tell me whether you agree or disagree with my assessment, as well as adding code patterns you have observed yourself.

* * *

**Pattern:** Creating one thread to process callbacks, while the main thread stays idle.

```cpp
ros::AsyncSpinner spinner(1);
spinner.start();
while (!gShutdownRequest) { ros::Duration(0.05).sleep(); }

```

**Reasoning:** Why create a new thread? Seems like a remnant from code refactoring. This pattern can be replaced with:

```cpp
ros::spin();

```

* * *

**Pattern:** Using a `Rate` object just to receive callbacks.

```cpp
ros::Rate r(100);
while (ros::ok()) {
  ros::spinOnce();
  r.sleep();
}

```

**Reasoning:** This is only justifiable if one wants to limit the rate at which callbacks are processed by some degree, at the risk of possibly dropping some messages. This pattern can be replaced with:

```cpp
ros::spin();

```

* * *

**Pattern:** Calling `spinOnce` at the end of the loop.

```cpp
  while (ros::ok())
  {
    /**... */
    chatter_pub.publish(msg);
    ros::spinOnce();
    loop_rate.sleep();
  }

```

**Reasoning:** If the published message is based on the received data, `spinOnce` should be called at the start of the loop, so that the newest data is processed as fast as possible.

* * *

**Pattern:** One thread processes callbacks, another publishes messages. They share internal state, but lack thread synchronization.

```cpp
void MyClass::onInit() {
  update_thread_.start(&MyClass::update, *this);
}

void MyClass::topicCB(const std_msgs::Int8ConstPtr msg) {
  var1_ = ...
  var2_ = ...
}

void MyClass::update() {
  ros::Rate spin_rate(10);
  while (!shutdown_requested_ && ros::ok()) {
    if (var1_ || var2_) { ... }
    spin_rate.sleep();
  }
}

```

**Reasoning:** Lack of thread synchronization means that variable updates on one thread might not be visible to the other thread. That is, updates to `var1_` or `var2_` in `topicCB()` might never make it to the thread that runs `update()`.

---

<div class="post-metadata">

**Author:** ![Katherine\_Scott](https://sea2.discourse-cdn.com/flex022/user_avatar/discourse.openrobotics.org/katherine_scott/32/4434_2.png) [@Katherine\_Scott](https://discourse.openrobotics.org/u/Katherine_Scott)\
**Post date:** [April 13, 2021, 6:34pm UTC](https://discourse.openrobotics.org/t/code-smells-in-ros-code/19905/2 "2021-04-13T18:34:34Z")

</div>

I love this pattern of discussion. We should write a book, “Learn ROS by Doing it the Wrong Way First.”

I made this slide for a talk a few weeks back. The intended audience was SREs / ops people so it addresses deployment patterns in production. I wanted the audience to be on the look out for non-standard ways of using ROS. Like the slide says, these aren’t forbidden or wrong, they are just things I would ask questions about if I saw them in production.

 ![ROS-101-Lesson](https://us1.discourse-cdn.com/flex022/uploads/ros/original/2X/0/0cee695c0bdeb50042e3bd16a841e523bda2cf15.png)

---

<div class="post-metadata">

**Author:** ![gavanderhoorn](https://sea2.discourse-cdn.com/flex022/user_avatar/discourse.openrobotics.org/gavanderhoorn/32/969_2.png) [@gavanderhoorn](https://discourse.openrobotics.org/u/gavanderhoorn)\
**Post date:** [April 13, 2021, 6:48pm UTC](https://discourse.openrobotics.org/t/code-smells-in-ros-code/19905/3 "2021-04-13T18:48:33Z")

</div>

> [@Katherine\_Scott](#):
>
> We should write a book, “Learn ROS by Doing it the Wrong Way First.”

I’ve thought about this, it could be the anti-pattern version of [Guidelines on how to architect ROS-based systems - Quality Assurance - Open Robotics Discourse](http://discourse.ros.org/t/guidelines-on-how-to-architect-ros-based-systems/12641) .

---

<div class="post-metadata">

**Author:** ![iivanoo](https://sea2.discourse-cdn.com/flex022/user_avatar/discourse.openrobotics.org/iivanoo/32/5518_2.png) [@iivanoo](https://discourse.openrobotics.org/u/iivanoo)\
**Post date:** [April 14, 2021, 11:08am UTC](https://discourse.openrobotics.org/t/code-smells-in-ros-code/19905/4 "2021-04-14T11:08:44Z")

</div>

Interesting! Well spotted @afsantos !  
By the way, we just finished to extend the original “guidelines paper” into a journal paper containing the description of ALL 47 guidelines. I will come back on this somewhere next week.

---

<div class="post-metadata">

**Author:** ![schornakj](https://sea2.discourse-cdn.com/flex022/user_avatar/discourse.openrobotics.org/schornakj/32/6108_2.png) [@schornakj](https://discourse.openrobotics.org/u/schornakj)\
**Post date:** [April 15, 2021, 4:20pm UTC](https://discourse.openrobotics.org/t/code-smells-in-ros-code/19905/5 "2021-04-15T16:20:27Z")

</div>

I think it would also be helpful to talk about this kind of thing in the context of ROS2. There are a few patterns which appear in the demos and example packages (and which are therefore implicitly endorsed) that can cause problems down the road. As an example:

* * *

**ROS2 (Anti-)Pattern:** Using `spin_until_future_complete` (which is used in many places, such as the [minimal\_client rclcpp example](https://github.com/ros2/examples/blob/master/rclcpp/services/minimal_client/main.cpp)):

```
  auto request = std::make_shared<AddTwoInts::Request>();
  request->a = 41;
  request->b = 1;
  auto result_future = client->async_send_request(request);
  if (rclcpp::spin_until_future_complete(node, result_future) !=
    rclcpp::FutureReturnCode::SUCCESS)
  {
    RCLCPP_ERROR(node->get_logger(), "service call failed :(");
    return 1;
  }
  auto result = result_future.get();

```

**Reasoning:** [It is not possible](https://github.com/ros2/rclcpp/issues/773#issuecomment-523249187) to call `spin_until_future_complete` from a callback which is being handled by an executor. If you’ve written some portable function that uses it, it may fail depending on the architecture of the node that is using it!

Also, if you’re able to use `spin_until_future_complete` in your node, then you have intermingled the custom functionality of your node with the scope of the node’s executor. This will make it much more difficult to structure the node so it is composable as a Component.

I’ve been using the `future` returned by the rclcpp service client’s `async_send_request` function to help wait until the service has finished, though this requires manual setup of callback groups and a `MultithreadedExecutor` to let the node get the service result response in a different thread.

---

<div class="post-metadata">

**Author:** ![agutenkunst](https://sea2.discourse-cdn.com/flex022/user_avatar/discourse.openrobotics.org/agutenkunst/32/2294_2.png) [@agutenkunst](https://discourse.openrobotics.org/u/agutenkunst)\
**Post date:** [April 19, 2021, 2:52pm UTC](https://discourse.openrobotics.org/t/code-smells-in-ros-code/19905/6 "2021-04-19T14:52:45Z")

</div>

I think having a collection of anti-patterns/code smells of some sort (book, repo, thread…) would be a great! Having some of these patterns checked by static analysis would be even greater!

---

<div class="post-metadata">

**Author:** ![Ingo\_Lutkebohle](https://sea2.discourse-cdn.com/flex022/user_avatar/discourse.openrobotics.org/ingo_lutkebohle/32/6730_2.png) [@Ingo\_Lutkebohle](https://discourse.openrobotics.org/u/Ingo_Lutkebohle)\
**Post date:** [April 19, 2021, 3:33pm UTC](https://discourse.openrobotics.org/t/code-smells-in-ros-code/19905/7 "2021-04-19T15:33:25Z")

</div>

> Using `spin_until_future_complete`

This could be considered an anti-pattern in itself, since, in its current form, the function should actually be called `spin_until_future_complete_and_then_some`, where “then\_some” may be rounding up to 1 second (that happens with FastRTPS at least, I surmise this may be middleware specific) or something else.