Passing methods as parameters

Question

I have reviewed this excellent answer but I am still confused. Ultimately I will have 10 method all behaving similar. Here is an example of three I have written already:

void CChristianLifeMinistryEditorDlg::OnSwapWithChairmanAssignment(UINT nID)
{
    // We need to locate the name for this assignment 
    auto iter = m_mapSwapAssignments.find(nID);

    if (iter != m_mapSwapAssignments.end())
    {
        // We found it
        CChristianLifeMinistryEntry *pReplacement = m_mapSwapAssignments[nID];
        CString strExistingName = _T(""), strReplacementName = _T("");
        GetDlgItemText(m_iSwapAssignmentSourceID, strExistingName);

        CString strPrompt = _T("");
        strPrompt.Format(_T("Swap assignments:\n\n%s - %s\n%s - %s\n\nPlease confirm."),
            m_pEntry->GetMeetingDateAsString(),
            (LPCTSTR)strExistingName,
            pReplacement->GetMeetingDateAsString(),
            (LPCTSTR)pReplacement->GetChairman());
        if (AfxMessageBox(strPrompt, MB_ICONQUESTION | MB_YESNO) == IDYES)
        {
            strReplacementName = pReplacement->GetChairman();
            pReplacement->SetChairman(strExistingName);

            SetDlgItemText(m_iSwapAssignmentSourceID, strReplacementName);
            SetModified(true);
        }
    }
}

void CChristianLifeMinistryEditorDlg::OnSwapWithOpenPrayerAssignment(UINT nID)
{
    // We need to locate the name for this assignment 
    auto iter = m_mapSwapAssignments.find(nID);

    if (iter != m_mapSwapAssignments.end())
    {
        // We found it
        CChristianLifeMinistryEntry *pReplacement = m_mapSwapAssignments[nID];
        CString strExistingName = _T(""), strReplacementName = _T("");
        GetDlgItemText(m_iSwapAssignmentSourceID, strExistingName);

        CString strPrompt = _T("");
        strPrompt.Format(_T("Swap assignments:\n\n%s - %s\n%s - %s\n\nPlease confirm."),
            m_pEntry->GetMeetingDateAsString(),
            (LPCTSTR)strExistingName,
            pReplacement->GetMeetingDateAsString(),
            (LPCTSTR)pReplacement->GetOpenPrayer());
        if (AfxMessageBox(strPrompt, MB_ICONQUESTION | MB_YESNO) == IDYES)
        {
            strReplacementName = pReplacement->GetOpenPrayer();
            pReplacement->SetOpenPrayer(strExistingName);

            SetDlgItemText(m_iSwapAssignmentSourceID, strReplacementName);
            SetModified(true);
        }
    }
}

void CChristianLifeMinistryEditorDlg::OnSwapWithClosePrayerAssignment(UINT nID)
{
    // We need to locate the name for this assignment 
    auto iter = m_mapSwapAssignments.find(nID);

    if (iter != m_mapSwapAssignments.end())
    {
        // We found it
        CChristianLifeMinistryEntry *pReplacement = m_mapSwapAssignments[nID];
        CString strExistingName = _T(""), strReplacementName = _T("");
        GetDlgItemText(m_iSwapAssignmentSourceID, strExistingName);

        CString strPrompt = _T("");
        strPrompt.Format(_T("Swap assignments:\n\n%s - %s\n%s - %s\n\nPlease confirm."),
            m_pEntry->GetMeetingDateAsString(),
            (LPCTSTR)strExistingName,
            pReplacement->GetMeetingDateAsString(),
            (LPCTSTR)pReplacement->GetClosePrayer());
        if (AfxMessageBox(strPrompt, MB_ICONQUESTION | MB_YESNO) == IDYES)
        {
            strReplacementName = pReplacement->GetClosePrayer();
            pReplacement->SetClosePrayer(strExistingName);

            SetDlgItemText(m_iSwapAssignmentSourceID, strReplacementName);
            SetModified(true);
        }
    }
}

I want to code the code from each of those event handlers into a method.

GetChairman
SetChairman
GetOpenPrayer
SetOpenPrayer
GetClosePrayer
SetClosePrayer

I have tried this:

void CChristianLifeMinistryEditorDlg::SwapAssignments(UINT nID, CString(*GetExistingName), void(*SetReplacementName)(CString))
{
    // We need to locate the name for this assignment 
    auto iter = m_mapSwapAssignments.find(nID);

    if (iter != m_mapSwapAssignments.end())
    {
        // We found it
        CChristianLifeMinistryEntry *pReplacement = m_mapSwapAssignments[nID];
        CString strExistingName = _T(""), strReplacementName = _T("");
        GetDlgItemText(m_iSwapAssignmentSourceID, strExistingName);

        CString strPrompt = _T("");
        strPrompt.Format(_T("Swap assignments:\n\n%s - %s\n%s - %s\n\nPlease confirm."),
            m_pEntry->GetMeetingDateAsString(),
            (LPCTSTR)strExistingName,
            pReplacement->GetMeetingDateAsString(),
            (LPCTSTR)pReplacement->(*GetExistingName));
        if (AfxMessageBox(strPrompt, MB_ICONQUESTION | MB_YESNO) == IDYES)
        {
            strReplacementName = pReplacement->(*GetExistingName);
            pReplacement->(*SetReplacementName)(strExistingName);

            SetDlgItemText(m_iSwapAssignmentSourceID, strReplacementName);
            SetModified(true);
        }
    }
}

I get build errors:

How can pass these methods as parameters?

I see this other answer that I was hoping I could use but the problem is that the method being called is a member of an object. So it won't allow it.

Current Workaround

At the moment I have implemented a couple of functions:

CString CChristianLifeMinistryEntry::GetName(SwapAssignment eAssignment)
{
    switch (eAssignment)
    {
    case SwapAssignment::Chairman:
        return GetChairman();
    case SwapAssignment::Counsellor1:
        return GetAuxiliaryCounsellor1();
    case SwapAssignment::Counsellor2:
        return GetAuxiliaryCounsellor2();
    case SwapAssignment::OpenPrayer:
        return GetOpenPrayer();
    case SwapAssignment::Treasures1:
        return GetTreasures1();
    case SwapAssignment::Treasures2:
        return GetTreasures2();
    case SwapAssignment::Living1:
        return GetLiving1();
    case SwapAssignment::Living2:
        return GetLiving2();
    case SwapAssignment::ConductorCBS:
        return GetCBSConductor();
    case SwapAssignment::ReaderCBS:
        return GetCBSReader();
    case SwapAssignment::ClosePrayer:
        return GetClosePrayer();
    }

    return _T("");
}

// AJT v18.1.6
void CChristianLifeMinistryEntry::SetName(CString strName, SwapAssignment eAssignment)
{
    switch (eAssignment)
    {
    case SwapAssignment::Chairman:
        SetChairman(strName);
        break;
    case SwapAssignment::Counsellor1:
        SetAuxiliaryCounsellor1(strName);
        break;
    case SwapAssignment::Counsellor2:
        SetAuxiliaryCounsellor2(strName);
        break;
    case SwapAssignment::OpenPrayer:
        SetOpenPrayer(strName);
        break;
    case SwapAssignment::Treasures1:
        SetTreasures1(strName);
        break;
    case SwapAssignment::Treasures2:
        SetTreasures2(strName);
        break;
    case SwapAssignment::Living1:
        SetLiving1(strName);
        break;
    case SwapAssignment::Living2:
        SetLiving2(strName);
        break;
    case SwapAssignment::ConductorCBS:
        SetCBSConductor(strName);
        break;
    case SwapAssignment::ReaderCBS:
        SetCBSReader(strName);
        break;
    case SwapAssignment::ClosePrayer:
        SetClosePrayer(strName);
        break;
    }
}

Then I moved the code I mentioned with a little adjustment into a new method:

// AJT v18.1.6
void CChristianLifeMinistryEditorDlg::SwapAssignments(UINT nID, SwapAssignment eAssignment)
{
    // We need to locate the name for this assignment 
    auto iter = m_mapSwapAssignments.find(nID);

    if (iter != m_mapSwapAssignments.end())
    {
        // We found it
        CChristianLifeMinistryEntry *pReplacement = m_mapSwapAssignments[nID];
        CString strExistingName = _T(""), strReplacementName = _T("");
        GetDlgItemText(m_iSwapAssignmentSourceID, strExistingName);

        CString strPrompt = _T("");
        strPrompt.Format(_T("Swap assignments:\n\n%s - %s\n%s - %s\n\nPlease confirm."),
            m_pEntry->GetMeetingDateAsString(),
            (LPCTSTR)strExistingName,
            pReplacement->GetMeetingDateAsString(),
            (LPCTSTR)pReplacement->GetName(eAssignment));
        if (AfxMessageBox(strPrompt, MB_ICONQUESTION | MB_YESNO) == IDYES)
        {
            strReplacementName = pReplacement->GetName(eAssignment);
            pReplacement->SetName(strExistingName, eAssignment);
            if (pReplacement == m_pEntry) // Swapping assignments on same meeting!
                SetDlgItemText(IDC_COMBO_OCLM_CBS_READER, strExistingName);

            SetDlgItemText(m_iSwapAssignmentSourceID, strReplacementName);
            SetModified(true);
        }
    }
}

So now my 11 handlers look similar to this:

// AJT v18.1.6
void CChristianLifeMinistryEditorDlg::OnSwapWithCBSReaderAssignment(UINT nID)
{
    SwapAssignments(nID, SwapAssignment::ReaderCBS);
}

It works. But I see a new comment has been added to my question with a link so I will see if I can get anywhere.

[Pointers to member functions are very strange animals](https://blogs.msdn.microsoft.com/oldnewthing/20040209-00/?p=40713). — IInspectable, Apr 10 '18 at 09:57
@IInspectable Thanks for the link. I am trying to get my head around that article but it is making my head hurt ... :) — Andrew Truckle, Apr 10 '18 at 10:06
Try to read this : https://stackoverflow.com/a/49636209/2303176 . If you find it less head hurting and more close to what you need, I can help you build it further. — badola, Apr 10 '18 at 15:09
@badola Thanks for the link to your answer which is useful. The issue I have here still is that the examples are all with member functions of the current class. As you can see in my final code I need to pass functions that are members of a specific object since we have to get / set based on member variables in that object. Can this approach still be used? — Andrew Truckle, Apr 10 '18 at 15:32
Yes. You can wrap a member function along with its object in a lambda and then pass it around just like a normal function. `auto is_5_even = [&obj]() { return obj.member_function(5); };` . Here we are binding the `obj` by reference. You can bind by copy as well. — badola, Apr 10 '18 at 15:38
@badola Does that `template` line have to go above the function in both the header and the cpp? And I still can't see how I can use this lambda since `CChristianLifeMinistryEntry *pReplacement = m_mapSwapAssignments[nID];` is made inside the handler and it is that which this lambda would need to be executed on. `pReplacement->XXXXXXX` etc. Sorry if I am dumb. This stuff is new to me. And I am only trying to learn it because I want to try new things. — Andrew Truckle, Apr 10 '18 at 16:17
templates are generally preferred in header files. Though there is no such limitation that it must be in the header file. I will try to refactor your code in half an hour or so, and maybe provide you with the desired answer. And don't worry about it being confusing, it is for all, in the beginning. — badola, Apr 10 '18 at 16:21
Let us [continue this discussion in chat](https://chat.stackoverflow.com/rooms/168668/discussion-between-badola-and-andrew-truckle). — badola, Apr 10 '18 at 17:24

score 1 · Accepted Answer · edited Jun 20 '20 at 09:12

So lets refactor the code in stages.

Stage 1: Isolate the common functionality, first, in a generic function OnSwapWith

template<typename Function>
void CChristianLifeMinistryEditorDlg::OnSwapWith(Function && function,
                                                 UINT        nID)
{
    // We need to locate the name for this assignment
    auto iter = m_mapSwapAssignments.find(nID);

    if (iter != m_mapSwapAssignments.end())
    {
        // We found it
        CChristianLifeMinistryEntry *pReplacement = m_mapSwapAssignments[nID];
        CString strExistingName = _T(""), strReplacementName = _T("");
        GetDlgItemText(m_iSwapAssignmentSourceID, strExistingName);

        CString strPrompt = _T("");
        strPrompt.Format(_T("Swap assignments:\n\n%s - %s\n%s - %s\n\nPlease confirm."),
            m_pEntry->GetMeetingDateAsString(),
            (LPCTSTR)strExistingName,
            pReplacement->GetMeetingDateAsString(),
            (LPCTSTR)pReplacement->GetChairman());

        if (AfxMessageBox(strPrompt, MB_ICONQUESTION | MB_YESNO) == IDYES)
        {
            function();
            SetDlgItemText(m_iSwapAssignmentSourceID, strReplacementName);
            SetModified(true);
        }
    }
}

void CChristianLifeMinistryEditorDlg::OnSwapWithChairmanAssignment(UINT nID)
{
    auto ChairmanAssignment = [&]() // capture by reference (i.e. &)
    {
        strReplacementName = pReplacement->GetChairman();
        pReplacement->SetChairman(strExistingName);
    };
    CChristianLifeMinistryEditorDlg::OnSwapWith(ChairmanAssignment, nID);
}

void CChristianLifeMinistryEditorDlg::OnSwapWithOpenPrayerAssignment(UINT nID)
{
    auto OpenPrayerAssignment = [&]() // capture by reference (i.e. &)
    {
        strReplacementName = pReplacement->GetOpenPrayer();
        pReplacement->SetOpenPrayer(strExistingName);
    };
    CChristianLifeMinistryEditorDlg::OnSwapWith(OpenPrayerAssignment, nID);
}

void CChristianLifeMinistryEditorDlg::OnSwapWithClosePrayerAssignment(UINT nID)
{
    auto ClosePrayerAssignment = [&]() // capture by reference (i.e. &)
    {
        strReplacementName = pReplacement->GetClosePrayer();
        pReplacement->SetClosePrayer(strExistingName);
    };
    CChristianLifeMinistryEditorDlg::OnSwapWith(ClosePrayerAssignment, nID);
}

// and similarly other functions can be defined

After this refactoring, we at least have reduced one level of redundancy.

In this refactoring we have captured everything by reference in the line:
auto ChairmanAssignment = [&]() {};

While capturing by reference, everything is being captured, such as this pointer.
That is why this statement is working :
strReplacementName = pReplacement->GetChairman();

It gets expanded into:
this->strReplacementName = pReplacement->GetChairman();

Stage 2: Understand that essentially what you are using, were just some generic getters and setters.

So lets refactor it further:

// instead of passing one function, we will pass two : getter and setter
template<typename GetterFunction, 
         typename SetterFunction>
void CChristianLifeMinistryEditorDlg::OnSwapWith(GetterFunction && getter,
                                                 SetterFunction && setter,
                                                 UINT              nID)
{
    ...

        if (AfxMessageBox(strPrompt, MB_ICONQUESTION | MB_YESNO) == IDYES)
        {
            strReplacementName = getter(pReplacement);
            setter(pReplacement, strExistingName);
            SetDlgItemText(m_iSwapAssignmentSourceID, strReplacementName);
            SetModified(true);
        }

    ...
}

And the caller side

void CChristianLifeMinistryEditorDlg::OnSwapWithChairmanAssignment(UINT nID)
{
    auto getter = [](CChristianLifeMinistryEntry * pReplacement) 
    { // remove the captures now, since we are getting the state with the pointer
        return pReplacement->GetChairman();
    };
    auto setter = [](CChristianLifeMinistryEntry * pReplacement,
                     CString               const & strExistingName) 
    {
        pReplacement->SetChairman(strExistingName);
    };
    CChristianLifeMinistryEditorDlg::OnSwapWith(getter, setter, nID);
}

Final working code

The 11 menu event handlers look something like:

void CChristianLifeMinistryEditorDlg::OnSwapWithChairmanAssignment(UINT nID)
{
    auto getter = [](CChristianLifeMinistryEntry * pReplacement)
    {
        return pReplacement->GetChairman();
    };
    auto setter = [](CChristianLifeMinistryEntry * pReplacement, 
                     CString                     & strExistingName)
    {
        pReplacement->SetChairman(strExistingName);
    };
    CChristianLifeMinistryEditorDlg::OnSwapWith(getter, setter,
                                                nID, IDC_COMBO_OCLM_CHAIRMAN);
}

The templated function now looks like:

template<typename GetterFunction, typename SetterFunction>
void CChristianLifeMinistryEditorDlg::OnSwapWith(GetterFunction && getter,
                                                 SetterFunction && setter,
                                                 UINT              nID,
                                                 UINT              nReplacementCtrlID)
{
    // We need to locate the name for this assignment
    auto iter = m_mapSwapAssignments.find(nID);

    if (iter != m_mapSwapAssignments.end())
    {
        // We found it
        CChristianLifeMinistryEntry *pReplacement = m_mapSwapAssignments[nID];
        CString strExistingName = _T(""), strReplacementName = _T("");
        GetDlgItemText(m_iSwapAssignmentSourceID, strExistingName);

        CString strPrompt = _T("");
        strPrompt.Format(_T("Swap assignments:\n\n%s - %s\n%s - %s\n\nPlease confirm."),
            m_pEntry->GetMeetingDateAsString(),
            (LPCTSTR)strExistingName,
            pReplacement->GetMeetingDateAsString(),
            (LPCTSTR)getter(pReplacement));

        if (AfxMessageBox(strPrompt, MB_ICONQUESTION | MB_YESNO) == IDYES)
        {
            strReplacementName = getter(pReplacement);
            setter(pReplacement, strExistingName);
            if (pReplacement == m_pEntry) // Swapping assignments on same meeting!
                SetDlgItemText(nReplacementCtrlID, strExistingName);

            SetDlgItemText(m_iSwapAssignmentSourceID, strReplacementName);
            SetModified(true);
        }
    }
}

@Andrew Truckle, if this works for you, then we can refactor further. Please use it in your code base, and let me know. I have very little knowledge of your variables, so didn't change them much. — badola, Apr 10 '18 at 17:24

Passing methods as parameters

Current Workaround

1 Answers1

Final working code