[wip] pubsub #10
[wip] pubsub #10
Conversation
Reviewer's Guide by SourceryThis pull request introduces a basic pubsub mechanism. It includes functions for creating a pubsub system, subscribing to topics, and publishing messages to topics. The implementation uses a linked list to store subscribers. A test case is added to demonstrate the functionality. Sequence diagram for pubsub publishsequenceDiagram
participant Publisher
participant Pubsub
participant Subscriber1
participant Subscriber2
Publisher->>Pubsub: publish(event)
Pubsub->>Pubsub: find_in_list(event.topic)
alt Topic found
Pubsub->>Subscriber1: subscriber_cb_fptr(event.data)
Pubsub->>Subscriber2: subscriber_cb_fptr(event.data)
else Topic not found
Pubsub-->>Publisher: return
end
Class diagram for PubsubclassDiagram
class PubsubPublisher {
char topic[20]
void* data
}
class PubsubSubscriber {
char topic[20]
subscriber_cb_t subscriber_cb_fptr
void* sub_data
}
class Pubsub {
Head* subscriber_list
bool pubsub_create()
bool pubsub_publish(PubsubPublisher event)
bool pubsub_subscribe(PubsubSubscriber sub)
}
Pubsub -- PubsubPublisher : Uses
Pubsub -- PubsubSubscriber : Manages
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @devprabal - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider using a more robust string comparison function like
strcmpinstead of comparing lengths first. - Think about how you will handle concurrent access to the subscriber list.
Here's what I looked at during the review
- 🟡 General issues: 2 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| snprintf(sub1.topic, strlen("/topic1") + 1, "%s", "/topic1"); | ||
| pubsub_subscribe(sub1); | ||
|
|
||
| snprintf(sub1.topic, strlen("/topic1") + 1, "%s", "/topic1"); | ||
| pubsub_subscribe(sub2); |
There was a problem hiding this comment.
issue (bug_risk): Potential mix-up in topic assignment for sub2.
The second subscription uses 'snprintf' to write into sub1.topic instead of sub2.topic before subscribing sub2. This may lead to unexpected behavior if sub2's topic is not set properly. Please verify if this is intentional or update it to 'snprintf(sub2.topic, ...)' to match the subscriber.
pubsub.c
Outdated
| return false; | ||
| } | ||
|
|
||
| bool pubsub_publish(PubsubPublisher event) { |
There was a problem hiding this comment.
question (bug_risk): No callback invocation in pubsub_publish.
Currently, pubsub_publish only checks and prints a message when a matching topic is found. If the design intends to notify subscribers, iterating over the list and invoking each subscriber's callback function might be required. Ensure that this behavior aligns with the intended functionality of the publish method.
| return false; | ||
| } | ||
|
|
||
| static bool compare_topic(void* t1, void* t2) |
There was a problem hiding this comment.
issue (complexity): Consider simplifying compare_topic by using strcmp and refactoring pubsub_publish to use early returns instead of nested ifs to improve readability and reduce unnecessary checks and nesting, while keeping all functionality intact, which reduces complexity
**Consider simplifying both `compare_topic` and the nested flow in `pubsub_publish`.**
For example, in `compare_topic`, you can simplify string comparisons by using `strcmp` directly:
```c
static bool compare_topic(void* t1, void* t2) {
char* topic1 = (char*)t1;
char* topic2 = (char*)t2;
if (topic1 && topic2) {
return strcmp(topic1, topic2) == 0;
}
return false;
}For pubsub_publish, refactor to use early returns instead of nested ifs:
bool pubsub_publish(PubsubPublisher event) {
if (!subscriber_list) {
DBG;
return true;
}
if (find_in_list(subscriber_list, event.topic, compare_topic)) {
printf("\nfound\n");
} else {
DBG;
}
return true;
}These changes reduce the extra checks and nesting while keeping all functionality intact.
This PR is a Work In Progress.
This PR is only to test out sourcery gh ai code review
Summary by Sourcery
Implement a basic publish-subscribe (pubsub) messaging system with topic-based subscription and event publishing
New Features:
Enhancements:
Tests: