Skip to main content
deleted 258 characters in body
Source Link
user34073
user34073

First, you should not compare values to boolean literals in an if statement as the statement evaluates boolean values anyway. This is better off written as bSuccess instead of bSuccess == true and !bSuccess instead of bSuccess == false.

Second, you have this structure:

if (bSuccess) { }
if (!bSuccess) { }

These two conditions are mutually exclusive and are better off written like this:

if (bSuccess) { }
else { }

Third, you are modifying a local variable right here, which will go out of scope as soon as you leave the method:

else
{
  WRITELOG("Call to socket API 'select' failed inside recv method, error: %d", WSAGetLastError());
  bSuccess = false;
  break;
}

Right after this, you break out of your loop and return bSuccess;. This could be written to demonstrate you are immediately returning a fail signal like this:

else
{
  WRITELOG("Call to socket API 'select' failed inside recv method, error: %d", WSAGetLastError());
  return false;
}

FourthThird, you have this snippet:

//decide success or failure
if((iRet > 0) && (FD_ISSET(m_hSocket, &fdWrite)))
{
  return true;
}

return false;

This can be written without ifs like this:

return (iRet > 0) && FD_ISSET(m_hSocket, &fdWrite);

The parenthesis around the first statement are not necessary, but might help readability.

First, you should not compare values to boolean literals in an if statement as the statement evaluates boolean values anyway. This is better off written as bSuccess instead of bSuccess == true and !bSuccess instead of bSuccess == false.

Second, you have this structure:

if (bSuccess) { }
if (!bSuccess) { }

These two conditions are mutually exclusive and are better off written like this:

if (bSuccess) { }
else { }

Third, you are modifying a local variable right here, which will go out of scope as soon as you leave the method:

else
{
  WRITELOG("Call to socket API 'select' failed inside recv method, error: %d", WSAGetLastError());
  bSuccess = false;
  break;
}

Right after this, you break out of your loop and return bSuccess;. This could be written to demonstrate you are immediately returning a fail signal like this:

else
{
  WRITELOG("Call to socket API 'select' failed inside recv method, error: %d", WSAGetLastError());
  return false;
}

Fourth, you have this snippet:

//decide success or failure
if((iRet > 0) && (FD_ISSET(m_hSocket, &fdWrite)))
{
  return true;
}

return false;

This can be written without ifs like this:

return (iRet > 0) && FD_ISSET(m_hSocket, &fdWrite);

The parenthesis around the first statement are not necessary, but might help readability.

First, you should not compare values to boolean literals in an if statement. This is better off written as bSuccess instead of bSuccess == true and !bSuccess instead of bSuccess == false.

Second, you are modifying a local variable right here, which will go out of scope as soon as you leave the method:

else
{
  WRITELOG("Call to socket API 'select' failed inside recv method, error: %d", WSAGetLastError());
  bSuccess = false;
  break;
}

Right after this, you break out of your loop and return bSuccess;. This could be written to demonstrate you are immediately returning a fail signal like this:

else
{
  WRITELOG("Call to socket API 'select' failed inside recv method, error: %d", WSAGetLastError());
  return false;
}

Third, you have this snippet:

//decide success or failure
if((iRet > 0) && (FD_ISSET(m_hSocket, &fdWrite)))
{
  return true;
}

return false;

This can be written without ifs like this:

return (iRet > 0) && FD_ISSET(m_hSocket, &fdWrite);

The parenthesis around the first statement are not necessary, but might help readability.

deleted 401 characters in body
Source Link
user34073
user34073

This snippet can be improved:

if (bSuccess == true)
{
  // Receive data from socket
  char chRecvBuff[MAX_RECV_LEN+1] = {0};
  bSuccess = ReceiveSocketData(chRecvBuff, MAX_RECV_LEN+1);

  // Verify response packet for proper GUID
  if (bSuccess == true)
  {
    CString csRecvBuff = CString(chRecvBuff);
    bSuccess = ValidateACK(eSendMessageID, csRecvBuff, csSendPacketGUID);
    if (bSuccess == true)
    {
      break;
    }
  }
}

if (bSuccess == false)
{
  RetryConnection();
  m_iRetryCount++;
  if(m_iRetryCount <= MAX_RETRY)
  {
    ReportStatus(App_Stat_Retry, m_iRetryCount);
    WRITELOG("Attempting retry %d", m_iRetryCount);
  }
}

First, you should not compare values to boolean literals in an if statement as the statement evaluates boolean values anyway. This is better off written as bSuccess instead of bSuccess == true and !bSuccess instead of bSuccess == false.

Second, you have this structure:

if (bSuccess) { }
if (!bSuccess) { }

These two conditions are mutually exclusive and are better off written like this:

if (bSuccess) { }
else { }

Third, you are modifying a local variable right here, which will go out of scope as soon as you leave the method:

else
{
  WRITELOG("Call to socket API 'select' failed inside recv method, error: %d", WSAGetLastError());
  bSuccess = false;
  break;
}

Right after this, you break out of your loop and return bSuccess;. This could be written to demonstrate you are immediately returning a fail signal like this:

else
{
  WRITELOG("Call to socket API 'select' failed inside recv method, error: %d", WSAGetLastError());
  return false;
}

Fourth, you have this snippet:

//decide success or failure
if((iRet > 0) && (FD_ISSET(m_hSocket, &fdWrite)))
{
  return true;
}

return false;

This can be written without ifs like this:

return (iRet > 0) && FD_ISSET(m_hSocket, &fdWrite);

The parenthesis around the first statement are not necessary, but might help readability.

This snippet can be improved:

if (bSuccess == true)
{
  // Receive data from socket
  char chRecvBuff[MAX_RECV_LEN+1] = {0};
  bSuccess = ReceiveSocketData(chRecvBuff, MAX_RECV_LEN+1);

  // Verify response packet for proper GUID
  if (bSuccess == true)
  {
    CString csRecvBuff = CString(chRecvBuff);
    bSuccess = ValidateACK(eSendMessageID, csRecvBuff, csSendPacketGUID);
    if (bSuccess == true)
    {
      break;
    }
  }
}

if (bSuccess == false)
{
  RetryConnection();
  m_iRetryCount++;
  if(m_iRetryCount <= MAX_RETRY)
  {
    ReportStatus(App_Stat_Retry, m_iRetryCount);
    WRITELOG("Attempting retry %d", m_iRetryCount);
  }
}

First, you should not compare values to boolean literals in an if statement as the statement evaluates boolean values anyway. This is better off written as bSuccess instead of bSuccess == true and !bSuccess instead of bSuccess == false.

Second, you have this structure:

if (bSuccess) { }
if (!bSuccess) { }

These two conditions are mutually exclusive and are better off written like this:

if (bSuccess) { }
else { }

Third, you are modifying a local variable right here, which will go out of scope as soon as you leave the method:

else
{
  WRITELOG("Call to socket API 'select' failed inside recv method, error: %d", WSAGetLastError());
  bSuccess = false;
  break;
}

Right after this, you break out of your loop and return bSuccess;. This could be written to demonstrate you are immediately returning a fail signal like this:

else
{
  WRITELOG("Call to socket API 'select' failed inside recv method, error: %d", WSAGetLastError());
  return false;
}

First, you should not compare values to boolean literals in an if statement as the statement evaluates boolean values anyway. This is better off written as bSuccess instead of bSuccess == true and !bSuccess instead of bSuccess == false.

Second, you have this structure:

if (bSuccess) { }
if (!bSuccess) { }

These two conditions are mutually exclusive and are better off written like this:

if (bSuccess) { }
else { }

Third, you are modifying a local variable right here, which will go out of scope as soon as you leave the method:

else
{
  WRITELOG("Call to socket API 'select' failed inside recv method, error: %d", WSAGetLastError());
  bSuccess = false;
  break;
}

Right after this, you break out of your loop and return bSuccess;. This could be written to demonstrate you are immediately returning a fail signal like this:

else
{
  WRITELOG("Call to socket API 'select' failed inside recv method, error: %d", WSAGetLastError());
  return false;
}

Fourth, you have this snippet:

//decide success or failure
if((iRet > 0) && (FD_ISSET(m_hSocket, &fdWrite)))
{
  return true;
}

return false;

This can be written without ifs like this:

return (iRet > 0) && FD_ISSET(m_hSocket, &fdWrite);

The parenthesis around the first statement are not necessary, but might help readability.

Source Link
user34073
user34073

This snippet can be improved:

if (bSuccess == true)
{
  // Receive data from socket
  char chRecvBuff[MAX_RECV_LEN+1] = {0};
  bSuccess = ReceiveSocketData(chRecvBuff, MAX_RECV_LEN+1);

  // Verify response packet for proper GUID
  if (bSuccess == true)
  {
    CString csRecvBuff = CString(chRecvBuff);
    bSuccess = ValidateACK(eSendMessageID, csRecvBuff, csSendPacketGUID);
    if (bSuccess == true)
    {
      break;
    }
  }
}

if (bSuccess == false)
{
  RetryConnection();
  m_iRetryCount++;
  if(m_iRetryCount <= MAX_RETRY)
  {
    ReportStatus(App_Stat_Retry, m_iRetryCount);
    WRITELOG("Attempting retry %d", m_iRetryCount);
  }
}

First, you should not compare values to boolean literals in an if statement as the statement evaluates boolean values anyway. This is better off written as bSuccess instead of bSuccess == true and !bSuccess instead of bSuccess == false.

Second, you have this structure:

if (bSuccess) { }
if (!bSuccess) { }

These two conditions are mutually exclusive and are better off written like this:

if (bSuccess) { }
else { }

Third, you are modifying a local variable right here, which will go out of scope as soon as you leave the method:

else
{
  WRITELOG("Call to socket API 'select' failed inside recv method, error: %d", WSAGetLastError());
  bSuccess = false;
  break;
}

Right after this, you break out of your loop and return bSuccess;. This could be written to demonstrate you are immediately returning a fail signal like this:

else
{
  WRITELOG("Call to socket API 'select' failed inside recv method, error: %d", WSAGetLastError());
  return false;
}