Skip to content

fix(iot-dev): Fix inefficient way of building strings in https batch messages#1335

Merged
timtay-microsoft merged 4 commits intomasterfrom
timtay/stringBuilder
Sep 3, 2021
Merged

fix(iot-dev): Fix inefficient way of building strings in https batch messages#1335
timtay-microsoft merged 4 commits intomasterfrom
timtay/stringBuilder

Conversation

@timtay-microsoft
Copy link
Copy Markdown
Member

StringBuilder is much more efficient than just concatenating strings directly

…messages

StringBuilder is much more efficient than just concatenating strings directly
@timtay-microsoft
Copy link
Copy Markdown
Member Author

/azp run

@azure-pipelines
Copy link
Copy Markdown

Azure Pipelines successfully started running 4 pipeline(s).


if (atLeastOneMessage)
{
batchBodyBuilder.deleteCharAt(batchBodyBuilder.length() - 1); // remove the final comma from the list
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Another approach to consider, that doesn't requiring removing a character, but still has the overhead of one boolean checked on each iteration in the loop above (but only requires setting once), is to conditionally add the comma only after the first time.

So something like:

boolean isSubsequentMessage = false;
        for (HttpsSingleMessage message : messageList)
        {
            if (isSubsequentMessage) {
                batchBodyBuilder.append(','); // comma to separate each object in the json array
            } else {
                isSubsequentMessage = true;
            }
            addJsonToStringBuilder(message, batchBodyBuilder);
            this.numMsgs++;
        }

Copy link
Copy Markdown
Contributor

@jamdavi jamdavi Sep 2, 2021

Choose a reason for hiding this comment

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

Another thought to consider is String.join.

In this instance you'd move the JSON string creation to a method in the HttpsSingleMessage class, create the List<String> in sendMessage and create a List<String> constructor to be used.

A pro of this is we are using something built into the SDK and it makes it more readable; plus less lines of code. As well you're creating a method to be reused anytime you might want to spit out the JSON of a HttpsSingleMessage.

Some cons are you're moving the string creation up a level into sendMessage which may not want and you'd be adding a new method to the HttpsSingleMessage class and it may make the batch message creation too disjointed.

@timtay-microsoft
Copy link
Copy Markdown
Member Author

/azp run

@azure-pipelines
Copy link
Copy Markdown

Azure Pipelines successfully started running 4 pipeline(s).

{
jsonMsg.append("\"").append(key).append("\":");
jsonMsg.append("\"").append(allProperties.get(key)).append("\",");
jsonMsg.append("\"").append(allProperties.get(key)).append("\","); //TODO
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

todo what?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I'll send a PR out for this later. I've removed this todo for now

@timtay-microsoft
Copy link
Copy Markdown
Member Author

/azp run

@azure-pipelines
Copy link
Copy Markdown

Azure Pipelines successfully started running 4 pipeline(s).

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.

3 participants