current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 5 down vote favorite
1

What it is doing basically is reading an std::string from a given pointer.

I was told that using a StringBuilder as I am is not the best way to achieve this, so I would like to know how this would be best written.

As additional information, that is a pointer of a std::string and I've made that because I was originally curious if there was a way to read a std::string in C# without having to create a bridge using C++/CLI.

How could I optimize this bit of code for best performance?

public static string ReadStdString(IntPtr address)
{
    var capacity = Marshal.ReadInt32(address + 24);
    var length = Marshal.ReadInt32(address + 20);
    var startStr = address + 4;
    if (capacity > 15)
        startStr = (IntPtr)Marshal.ReadInt32(address + 4);

    var result = new StringBuilder();
    for (var i = 0; i < length; i++)
    {
        result.Append((char)Marshal.ReadByte(startStr, i));
    }
    return result.ToString();
}
share|improve this question
    
Why do you keep referring to "std::string"? Do you mean that address is supposed to be a C-style string? A null-terminated array of characters? –  nhgrif Jul 17 at 0:00
    
@nhgrif yes, I have a c++ dll and from a function from it I get returned a Entity within this Entity structure I have a std::string property, however in c# you cant read a std::string with conventional ways –  Guapo Jul 17 at 0:02
    
How long is the string? –  Mat's Mug♦ Jul 17 at 3:38
    
@Mat'sMug could be empty or as big as 50 chars I don't think it can exceed 50 chars. –  Guapo Jul 17 at 7:02

1 Answer 1

up vote 3 down vote accepted 
var result = new StringBuilder();
for (var i = 0; i < length; i++)
{
    result.Append((char)Marshal.ReadByte(startStr, i));
}

You're working in a tight loop: a StringBuilder looks like a reasonable tool to use.

One thing I would change that could impact performance (depending on the length of the string involved), is the StringBuilder constructor being used:

var result = new StringBuilder(length);

There's no reason not to specify the length of the string you're building if you know it from the start; that will reduce the overhead, since the internals of the builder won't need to manage growth.

The loop is a one-liner. You have this one-liner just a few instructions above:

if (capacity > 15)
    startStr = (IntPtr)Marshal.ReadInt32(address + 4);

Why does the loop have an explicit { } scope, but not the if block? It would be better to be consistent about scoping braces, and have them everywhere:

 if (capacity > 15)
 {
     startStr = (IntPtr)Marshal.ReadInt32(address + 4);
 }

An alternative to the StringBuilder could be to write the bytes into a MemoryStream:

using (var stream = new MemoryStream(length))
{
    for (var i = 0; i < length; i++)
    {
        stream.WriteByte(Marshal.ReadByte(startStr, i));
    }
    return new StreamReader(stream).ReadToEnd();
}

The nice thing with this approach is that you don't need to cast every single byte of the string into a char. On the other hand, you need 2 objects instead of one, and you need to cleanly Dispose of the stream, too - depending on the length of the string, the overhead might just not be worth it, although my guts tell me the stream could be faster... but I think you'd need to race your horses to find out.

share|improve this answer
    
Thanks for the comment and all the insight, indeed I tend to wrap things around {} always but since I was pretty tired when I was doing this piece of code I might have slept it. The length is a really good point again I'm so used to create the stringbuilder without defining a length that it was automatically that way. So in the I could probably gain some performance switching to a MemoryStream but wouldn't be some that might be worth switching over? Or it would depend mainly on the string size? –  Guapo Jul 17 at 6:58
    
I believe for ~50 iterations it could be slower than the builder, I had a much higher number of iterations in mind... but really the only way to find out for sure is to profile it. I just suggested the stream as an alternative, and noted it removed the need for casting every character. I think the StringBuilder version might do just fine though. 50 iterations are completed in the blink of an eye anyway. –  Mat's Mug♦ Jul 17 at 12:26

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.