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.

This may be a poor title name. So I'm basing what I did from this website.

The idea is though that I have made a lot of work to a Service Object for a OPOS MSR device. (useless detail) One of the DirectIO calls gets some data off of a card and was supposed to return a byte array. That ended up being a big mistake because Unicode and translating was just a big big BIG headache. So my suggestion was to just convert the byte array to a string in C++ then send fire a DirectIO event with said string and call it good. So I did that, and it appears to work. One thing that has me concerned though is one of the lines from that website

The general rule to COM resource is, if you allocate it then you release it. The only exception is when a value is passed over a COM interface as an OUT param. In that case, the receiver of the value is responsible for releasing the resource

but I'm rather proud of my code none the less. I'll post my SO code and my mock up OPOS code (a test for me to show it works)

LONG ReaderHelper::ULCRead(LPVOID param)
{
    BYTE* bTmp = new BYTE[4];
    //a call to fill said bTmp with error checking and so forth
    CString str;
    for (int i = 0; i < 4; i++)
    {
        str.AppendFormat( _T("%02X"), bTmp[i]);
    }
    delete[] bTmp;
    BSTR bstring = str.AllocSysString();
    LONG data = str.GetLength();
    USNMSRRFID* pso = (USNMSRRFID*)param;
    COPOSMSR msr(pso->mlpDispatch);
    msr.SODirectIO(0, &data, &bstring);
    SysFreeString(bstring);
    msr.ReleaseDispatch();
}

MOCK UP OPOS yes it is ugly, sorry

    void msr_DirectIOEvent(int EventNumber, ref int pData, ref string pString)
    {
        Console.WriteLine();
        Console.WriteLine("\t\t<{0}><{1}><{2}>:Len:{4}<{3}>", EventNumber, pData,pString, GetHexBytes(pString), pString.Length);
        Console.WriteLine();
    }
    string GetHexBytes(string str)
    {
        var bytes = System.Text.UnicodeEncoding.Unicode.GetBytes(str);
        string strbytes = "";
        foreach (byte b in bytes)
            strbytes += string.Format("{0}:", b.ToString("X2"));

        return strbytes;
    }

yields me this

2013-02-12.21:59:30 #Page:3#
2013-02-12.21:59:30     ULCRead(4)-00:00:00:00:
            <0><8><00000000>:Len:8<30:00:30:00:30:00:30:00:30:00:30:00:30:00:30:00:>
2013-02-12.21:59:30 #Page:4#
2013-02-12.21:59:30     ULCRead(4)-9D:5A:AD:47:
            <0><8><9D5AAD47>:Len:8<39:00:44:00:35:00:41:00:41:00:44:00:34:00:37:00:>
2013-02-12.21:59:30 #Page:5#
2013-02-12.21:59:30     ULCRead(4)-5D:E3:EC:93:
            <0><8><5DE3EC93>:Len:8<35:00:44:00:45:00:33:00:45:00:43:00:39:00:33:00:>
share|improve this question
    
What exactly do you want reviewed? What should we look for? –  svick Feb 13 '13 at 11:51
    
basically is ULCRead done properly? I'm getting the desired results... but how is it? Did I do it right? Did i miss any memory leaks? Did I release my COM object correctly? –  Robert Snyder Feb 13 '13 at 13:58

1 Answer 1

up vote 2 down vote accepted
  1. This BYTE* bTmp = new BYTE[4]; could just be put on the stack with BYTE bTmp[4]; (the proper term is actually automatic storage duration which in most cases means it ends up on the stack). This normally avoids a call to malloc and you don't have to delete it explicitly.
  2. I'm not too versed in COM programming but if msr.SODirectIO(0, &data, &bstring); could throw an exception you might want to wrap it into a try {} finally {} in order to ensure the cleanup of the BSTR and COM resource is done regardless.
  3. I know it's just a quick mockup but GetHexBytes could be reduced by using Linq:

    string GetHexBytes(string str)
    {
        var bytes = System.Text.UnicodeEncoding.Unicode.GetBytes(str);
        var hexBytes = bytes.Select(b => b.ToString("X2")).ToArray();
        return string.Join(":", hexBytes);
    }
    
share|improve this answer

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.