Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added new StringJoin() utility function #203

Merged
merged 1 commit into from
Dec 8, 2023

Conversation

larsewi
Copy link
Contributor

@larsewi larsewi commented Dec 8, 2023

The function joins a sequence of strings with an optional separator in between the elements.

The function joins a sequence of strings with an optional separator in
between the elements.

Ticket: None
Changelog: None
Signed-off-by: Lars Erik Wik <lars.erik.wik@northern.tech>
@larsewi larsewi marked this pull request as ready for review December 8, 2023 15:07
@mender-test-bot
Copy link

There was an error running your pipeline, see logs for details.

Copy link
Contributor

@craigcomstock craigcomstock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the code looks so familiar to something I saw recently! ;) 🏆

Copy link
Contributor

@vpodzime vpodzime left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me otherwise.


Writer *const writer = StringWriter();
const size_t len = SeqLength(seq);
for (size_t i = 0; i < len; i++)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd do i < (len -1) writing the separator after every item and then writing the last item after the for loop without a separator. But YMMV.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then the program would crash if the sequence was empty, right ? 🤔

Copy link
Contributor

@vpodzime vpodzime Dec 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, but that's easy to check upfront. Which is better than having an unnecessary if inside the loop.

@larsewi larsewi merged commit 25e755c into NorthernTechHQ:master Dec 8, 2023
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants