3
\$\begingroup\$

I wrote a functionality to read files and load them into the map m_fileParams.

Then I am searching the specific keys in the map container using find. Here using find I am searching different keys.

My code is working fine. But I wanted to know how best I can optimize the code?

In several places I have used find to search the key in the map. Is there any way I can write generic method?

m_fileParams is declared in a header file:

std::map<std::wstring, std::wstring> m_fileParams;

Here is the ReadFile function, which resides in a .cpp file:

bool McEMRMgr::ReadFile()
{
    IOperation* pFileReader = new McFileReader();
    std::map<std::wstring, std::wstring> mapFileReaderResponse;
    if(pFileReader)
    {
        std::wstring strVendor, wstrFileLocation, wstrRegLocation;
        std::wstring FileType, regType;

        auto manufacSearchValue = m_fileParams.find(VENDOR);
        if (manufacSearchValue != m_fileParams.end())
        {
            strVendor.assign(manufacSearchValue->second);
            transform(
                strVendor.begin(), strVendor.end(),
                strVendor.begin(),
                towlower);

        }
        else
        {
            strVendor.assign(L"default");
            FileType.assign(L".File");
            regType.assign(L".registry");
        }
        auto emailSearchValue = m_fileParams.find(L"email_locations." + strVendor + L".type" + FileType);
        if (emailSearchValue != m_fileParams.end())
        { 
            if (emailSearchValue->second.compare(L"File") == 0)
            {
                auto FileSearchPath = m_fileParams.find(L"email_locations." + strVendor + L".paths" + FileType);
                wstrFileLocation = FileSearchPath->second;
            }
        }

        { 
            emailSearchValue = m_fileParams.find(L"email_locations." + strVendor + L".type" + regType);
            if (emailSearchValue->second.compare(L"registry") == 0)
            {
                auto regSearchPath = m_fileParams.find(L"email_locations." + strVendor + L".paths" + regType);
                wstrRegLocation = regSearchPath->second;
            }
        }
        if (wstrFileLocation.length() != 0)
        {
            std::wstring oemFolderPath;
            auto oemSearchType = m_fileParams.find(L"oem_info_folder");
            if (oemSearchType != m_fileParams.end())
            {
                oemFolderPath.assign(oemSearchType->second);
            }
            else
            {
                oemFolderPath.assign(OEM_FOLDER_DEFAULT_PATH);
            }
            std::wstring oemPathPublicKey(oemFolderPath), oemPathSessionKey(oemFolderPath), oemPathUserChoices(oemFolderPath);
            oemPathPublicKey.append(PUBLIC_KEY_File);
            oemPathSessionKey.append(SESSION_KEY_File);
            oemPathUserChoices.append(USERCHOICES_File);
            pFileReader->SetParams((wchar_t*)oemPathPublicKey.c_str(), L"File");
            pFileReader->Execute();
            pFileReader->SetParams((wchar_t*)oemPathSessionKey.c_str(), L"File");
            pFileReader->Execute();
            pFileReader->SetParams((wchar_t*)oemPathUserChoices.c_str(), L"File");
            pFileReader->Execute();

            wchar_t* ptrWStr;
            wchar_t* buffer;
            ptrWStr = wcstok_s((wchar_t*)wstrFileLocation.c_str(), L" ", &buffer);
            while (ptrWStr != NULL)
            {
                pFileReader->SetParams(ptrWStr, L"File");
                pFileReader->Execute();
                mapFileReaderResponse = pFileReader->GetResponse();
                for (std::map<std::wstring, std::wstring>::iterator it = mapFileReaderResponse.begin(); it != mapFileReaderResponse.end(); ++it)
                {
                    m_fileParams.insert({ it->first, it->second });
                }
                ptrWStr = wcstok_s(nullptr, L" ", &buffer);
            }

            if (mapFileReaderResponse.size() != 0)
            {
                m_cJobState.eFileStatus = OperationState::Success;
            }
            else
            {
                m_cJobState.eFileStatus = OperationState::Failed;
            }
        }

        if (wstrRegLocation.length() != 0)
        { 
            pFileReader->SetParams((wchar_t*)wstrRegLocation.c_str(), L"registry");
            pFileReader->Execute();
            mapFileReaderResponse = pFileReader->GetResponse();
            for (std::map<std::wstring, std::wstring>::iterator it = mapFileReaderResponse.begin(); it != mapFileReaderResponse.end(); ++it)
            {
                m_fileParams.insert({ it->first, it->second });
            }
        }
        auto emailSearchType = m_fileParams.find(L"email");
        std::wstring emailValue = emailSearchType->second;
        if (emailValue.length() > 0)
        {
            MakeCopyofEmailToHKLM(emailValue);
        }
        else
        {
        }

        delete pFileReader;     
    }

    return true;
}

Someone please help me by providing your review comments.

\$\endgroup\$
1
  • 1
    \$\begingroup\$ Have you profiled the code or function in a release (or full optimizations turned on) build? It may not even be slow enough to be a problem. \$\endgroup\$ – Casey Aug 16 '19 at 19:05
0
\$\begingroup\$

I can't say the code is very readable in general, although it is probably better than average. This function is way too long and should be separated into several functions according to logic. And the way you use the standard library facilities doesn't seem optimal.

Here are some suggestions on the code:


IOperation* pFileReader = new McFileReader();

No, No, No. C++ is not Java. There is no garbage collector in C++. This line makes your code inherently exception-unsafely and error-prone. See Why should C++ programmers minimize use of 'new'?. The correct way is to declare a normal variable:

McFileReader fileReader{};

This avoids all sorts of problems.


if(pFileReader)

This is redundant because new never returns a null pointer. Instead, it throws an exception that is caught by an exception handler of type std::bad_alloc. Simply remove this check.


std::wstring strVendor, wstrFileLocation, wstrRegLocation;
std::wstring FileType, regType;

Declaring a bunch of variables at the start of a block is not advised in C++. A reader feels puzzled when seeing these two lines. Move these declarations to where these variables are actually used.


transform(
    strVendor.begin(), strVendor.end(),
    strVendor.begin(),
    towlower);

None of the arguments are guaranteed to nominate std in ADL, so use std::transform not transform. Also, towlower can't be used like this. Wrap it in a lambda.


strVendor.assign(manufacSearchValue->second);

Don't use assign when a simple assignment will do. strVender = manufacSearchValue->second is much better. This problem also recurs later in the code.

Similarly, use += instead of append.


pFileReader->SetParams((wchar_t*)oemPathPublicKey.c_str(), L"File");

The explicit cast to wchar_t* indicates a serious design problem. If you want to modify the string, use &oemPathPublicKey[0] in C++11. If you don't want to modify the string, remove the cast. If SetParams is not const-correct, correct it.


wchar_t* ptrWStr;
wchar_t* buffer;
ptrWStr = wcstok_s((wchar_t*)wstrFileLocation.c_str(), L" ", &buffer);

Now you are playing with pointers and wcstok_s. The first thing I notice is that wcstok_s doesn't exist in C++ at all. Even if it were to exist, you should be using the string operations. They are usually faster than the C functions if you use them properly.


m_fileParams.insert({ it->first, it->second });

Shouldn't it be insert(*it)? Or, if that doesn't work, emplace(it->first, it->second)?


auto emailSearchType = m_fileParams.find(L"email");
std::wstring emailValue = emailSearchType->second;

Why not auto emailValue = emailSearchType.at(L"email");?


else
{
}

This else clause is useless and can be removed.

\$\endgroup\$
7
  • \$\begingroup\$ Thank you for your valuable inputs I used the dynamic allocation like as shown in the below line, IOperation* pFileReader = new McFileReader(); because, we have written this function to read the files and loads into the map. we usually don't know what size the file has, so you can't decide how much memory to allocate until you run the program. \$\endgroup\$ – John Paul Coder Aug 17 '19 at 15:10
  • \$\begingroup\$ And also class MCFileReader is deriving from IOperation like as shown below, class McFileReader : public IOperation { public: void SetParams(wchar_t* wszParams, wchar_t* wszParamType); bool Execute(); }; \$\endgroup\$ – John Paul Coder Aug 17 '19 at 15:11
  • \$\begingroup\$ Even with this point also if it is not advisable to use dynamic allocation, as per your suggestion i will use local variable like McFileReader fileReader; \$\endgroup\$ – John Paul Coder Aug 17 '19 at 15:11
  • \$\begingroup\$ That’s irrelevant. You always use one McFileReader, so a local variable is better. \$\endgroup\$ – L. F. Aug 17 '19 at 15:15
  • 1
    \$\begingroup\$ @JohnPaulCoder That’s exactly why it should be removed. The pointer is always non null, so the check doesn’t really do anything. \$\endgroup\$ – L. F. Aug 17 '19 at 15:24

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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