Skip to content

refactor: convert more if-else chains to switch statements #1683

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

refactor: convert more if-else chains to switch statements #1683

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
69866e7
refactor: convert if-else chains to switch statements
bobtista Oct 10, 2025
4661df4
refactor: convert if-else chains to switch statements in Generals
bobtista Oct 10, 2025
cd58b22
refactor(network): Use NO_ERROR constant instead of magic number 0 in…
bobtista Oct 10, 2025
9fffbab
refactor(network): simplify conditionals for allConnectionsDone
bobtista Oct 10, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
214 changes: 125 additions & 89 deletions Generals/Code/GameEngine/Source/Common/Recorder.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1445,117 +1445,153 @@ void RecorderClass::appendNextCommand() {
}

void RecorderClass::readArgument(GameMessageArgumentDataType type, GameMessage *msg) {
if (type == ARGUMENTDATATYPE_INTEGER) {
Int theint;
m_file->read(&theint, sizeof(theint));
msg->appendIntegerArgument(theint);
switch (type) {
case ARGUMENTDATATYPE_INTEGER: {
Int theint;
m_file->read(&theint, sizeof(theint));
msg->appendIntegerArgument(theint);
#ifdef DEBUG_LOGGING
if (m_doingAnalysis)
{
DEBUG_LOG(("Integer argument: %d (%8.8X)", theint, theint));
}
if (m_doingAnalysis)
{
DEBUG_LOG(("Integer argument: %d (%8.8X)", theint, theint));
}
#endif
} else if (type == ARGUMENTDATATYPE_REAL) {
Real thereal;
m_file->read(&thereal, sizeof(thereal));
msg->appendRealArgument(thereal);
#ifdef DEBUG_LOGGING
if (m_doingAnalysis)
{
DEBUG_LOG(("Real argument: %g (%8.8X)", thereal, *(int *)&thereal));
break;
}
#endif
} else if (type == ARGUMENTDATATYPE_BOOLEAN) {
Bool thebool;
m_file->read(&thebool, sizeof(thebool));
msg->appendBooleanArgument(thebool);
case ARGUMENTDATATYPE_REAL: {
Real thereal;
m_file->read(&thereal, sizeof(thereal));
msg->appendRealArgument(thereal);
#ifdef DEBUG_LOGGING
if (m_doingAnalysis)
{
DEBUG_LOG(("Bool argument: %d", thebool));
}
if (m_doingAnalysis)
{
DEBUG_LOG(("Real argument: %g (%8.8X)", thereal, *(int *)&thereal));
}
#endif
} else if (type == ARGUMENTDATATYPE_OBJECTID) {
ObjectID theid;
m_file->read(&theid, sizeof(theid));
msg->appendObjectIDArgument(theid);
#ifdef DEBUG_LOGGING
if (m_doingAnalysis)
{
DEBUG_LOG(("Object ID argument: %d", theid));
break;
}
#endif
} else if (type == ARGUMENTDATATYPE_DRAWABLEID) {
DrawableID theid;
m_file->read(&theid, sizeof(theid));
msg->appendDrawableIDArgument(theid);
case ARGUMENTDATATYPE_BOOLEAN: {
Bool thebool;
m_file->read(&thebool, sizeof(thebool));
msg->appendBooleanArgument(thebool);
#ifdef DEBUG_LOGGING
if (m_doingAnalysis)
{
DEBUG_LOG(("Drawable ID argument: %d", theid));
}
if (m_doingAnalysis)
{
DEBUG_LOG(("Bool argument: %d", thebool));
}
#endif
} else if (type == ARGUMENTDATATYPE_TEAMID) {
UnsignedInt theid;
m_file->read(&theid, sizeof(theid));
msg->appendTeamIDArgument(theid);
#ifdef DEBUG_LOGGING
if (m_doingAnalysis)
{
DEBUG_LOG(("Team ID argument: %d", theid));
break;
}
#endif
} else if (type == ARGUMENTDATATYPE_LOCATION) {
Coord3D loc;
m_file->read(&loc, sizeof(loc));
msg->appendLocationArgument(loc);
case ARGUMENTDATATYPE_OBJECTID: {
ObjectID theid;
m_file->read(&theid, sizeof(theid));
msg->appendObjectIDArgument(theid);
#ifdef DEBUG_LOGGING
if (m_doingAnalysis)
{
DEBUG_LOG(("Coord3D argument: %g %g %g (%8.8X %8.8X %8.8X)", loc.x, loc.y, loc.z,
*(int *)&loc.x, *(int *)&loc.y, *(int *)&loc.z));
}
if (m_doingAnalysis)
{
DEBUG_LOG(("Object ID argument: %d", theid));
}
#endif
} else if (type == ARGUMENTDATATYPE_PIXEL) {
ICoord2D pixel;
m_file->read(&pixel, sizeof(pixel));
msg->appendPixelArgument(pixel);
#ifdef DEBUG_LOGGING
if (m_doingAnalysis)
{
DEBUG_LOG(("Pixel argument: %d,%d", pixel.x, pixel.y));
break;
}

case ARGUMENTDATATYPE_DRAWABLEID: {
DrawableID theid;
m_file->read(&theid, sizeof(theid));
msg->appendDrawableIDArgument(theid);
#ifdef DEBUG_LOGGING
if (m_doingAnalysis)
{
DEBUG_LOG(("Drawable ID argument: %d", theid));
}
#endif
} else if (type == ARGUMENTDATATYPE_PIXELREGION) {
IRegion2D reg;
m_file->read(&reg, sizeof(reg));
msg->appendPixelRegionArgument(reg);
break;
}

case ARGUMENTDATATYPE_TEAMID: {
UnsignedInt theid;
m_file->read(&theid, sizeof(theid));
msg->appendTeamIDArgument(theid);
#ifdef DEBUG_LOGGING
if (m_doingAnalysis)
{
DEBUG_LOG(("Pixel Region argument: %d,%d -> %d,%d", reg.lo.x, reg.lo.y, reg.hi.x, reg.hi.y));
if (m_doingAnalysis)
{
DEBUG_LOG(("Team ID argument: %d", theid));
}
#endif
break;
}

case ARGUMENTDATATYPE_LOCATION: {
Coord3D loc;
m_file->read(&loc, sizeof(loc));
msg->appendLocationArgument(loc);
#ifdef DEBUG_LOGGING
if (m_doingAnalysis)
{
DEBUG_LOG(("Coord3D argument: %g %g %g (%8.8X %8.8X %8.8X)", loc.x, loc.y, loc.z,
*(int *)&loc.x, *(int *)&loc.y, *(int *)&loc.z));
}
#endif
} else if (type == ARGUMENTDATATYPE_TIMESTAMP) { // Not to be confused with Terrance Stamp... Kneel before Zod!!!
UnsignedInt stamp;
m_file->read(&stamp, sizeof(stamp));
msg->appendTimestampArgument(stamp);
break;
}

case ARGUMENTDATATYPE_PIXEL: {
ICoord2D pixel;
m_file->read(&pixel, sizeof(pixel));
msg->appendPixelArgument(pixel);
#ifdef DEBUG_LOGGING
if (m_doingAnalysis)
{
DEBUG_LOG(("Timestamp argument: %d", stamp));
if (m_doingAnalysis)
{
DEBUG_LOG(("Pixel argument: %d,%d", pixel.x, pixel.y));
}
#endif
break;
}

case ARGUMENTDATATYPE_PIXELREGION: {
IRegion2D reg;
m_file->read(&reg, sizeof(reg));
msg->appendPixelRegionArgument(reg);
#ifdef DEBUG_LOGGING
if (m_doingAnalysis)
{
DEBUG_LOG(("Pixel Region argument: %d,%d -> %d,%d", reg.lo.x, reg.lo.y, reg.hi.x, reg.hi.y));
}
#endif
} else if (type == ARGUMENTDATATYPE_WIDECHAR) {
WideChar theid;
m_file->read(&theid, sizeof(theid));
msg->appendWideCharArgument(theid);
break;
}

case ARGUMENTDATATYPE_TIMESTAMP: { // Not to be confused with Terrance Stamp... Kneel before Zod!!!
UnsignedInt stamp;
m_file->read(&stamp, sizeof(stamp));
msg->appendTimestampArgument(stamp);
#ifdef DEBUG_LOGGING
if (m_doingAnalysis)
{
DEBUG_LOG(("WideChar argument: %d (%lc)", theid, theid));
if (m_doingAnalysis)
{
DEBUG_LOG(("Timestamp argument: %d", stamp));
}
#endif
break;
}

case ARGUMENTDATATYPE_WIDECHAR: {
WideChar theid;
m_file->read(&theid, sizeof(theid));
msg->appendWideCharArgument(theid);
#ifdef DEBUG_LOGGING
if (m_doingAnalysis)
{
DEBUG_LOG(("WideChar argument: %d (%lc)", theid, theid));
}
#endif
break;
}

default:
break;
}
}

Expand Down
36 changes: 8 additions & 28 deletions Generals/Code/GameEngine/Source/GameNetwork/NAT.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -907,34 +907,14 @@ void NAT::processManglerResponse(UnsignedShort mangledPort) {
// check to see if we've completed all the rounds
// this is kind of a cheesy way to check, but it works.
Bool NAT::allConnectionsDone() {
if (m_numNodes == 2) {
Copy link

@johnneijzen johnneijzen Oct 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't this just be simplified to this, or is there a reason why it was coded this way?

Bool NAT::allConnectionsDone() {
	if ((m_numNodes == 2 && m_connectionRound >= 1) ||
	    (m_numNodes == 3 && m_connectionRound >= 3) ||
	    (m_numNodes == 4 && m_connectionRound >= 3) ||
	    (m_numNodes == 5 && m_connectionRound >= 5) ||
	    (m_numNodes == 6 && m_connectionRound >= 5) ||
	    (m_numNodes == 7 && m_connectionRound >= 7) ||
	    (m_numNodes == 8 && m_connectionRound >= 7)) {
		return TRUE;
	}
	return FALSE;
}

Copy link
Author

@bobtista bobtista Oct 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had the same thought, so yeah I've added your version.

Copy link

@Caball009 Caball009 Oct 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could maybe even do something like this:

Bool NAT::allConnectionsDone()
{
	switch (m_numNodes)
	{
		case 2:
			return m_connectionRound >= 1;
		case 3:
			return m_connectionRound >= 3;
		case 4:
			return m_connectionRound >= 3;
		case 5:
			return m_connectionRound >= 5;
		case 6:
			return m_connectionRound >= 5;
		case 7:
			return m_connectionRound >= 7;
		case 8:
			return m_connectionRound >= 7;
		default:
			return FALSE;
	}
}

I'm not sure if VC6 will warn for a missing return statement, but if it does, you can make the default case break and add a return statement outside the switch statement.

Copy link

@Skyaero42 Skyaero42 Oct 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

more compact, not sure if it is more performant.

    if (m_numNodes < 2 || m_numNodes > 8) {
        return FALSE;
    }
    
    int requiredRounds = (m_numNodes % 2 == 0) ? m_numNodes - 1 : m_numNodes;
    return m_connectionRound >= requiredRounds;

Copy link

@Caball009 Caball009 Oct 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

more compact, not sure if it is more performant.

    if (m_numNodes < 2 || m_numNodes > 8) {
        return FALSE;
    }
    
    int requiredRounds = (m_numNodes % 2 == 0) ? m_numNodes - 1 : m_numNodes;
    return m_connectionRound >= requiredRounds;

This generates better code with modern compilers, not sure about VC6. I'd suggest to make it unsigned, though, to avoid a warning for comparison with unsigned in the return statement.

I don't know if VC6 optimizes % 2 == 1 to & 1, which modern compilers do (for unsigned integers).

Copy link
Author

@bobtista bobtista Oct 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like your first suggestion if ((m_numNodes == 2 && m_connectionRound >= 1) ||....

if (m_connectionRound >= 1) {
return TRUE;
}
} else if (m_numNodes == 3) {
if (m_connectionRound >= 3) {
return TRUE;
}
} else if (m_numNodes == 4) {
if (m_connectionRound >= 3) {
return TRUE;
}
} else if (m_numNodes == 5) {
if (m_connectionRound >= 5) {
return TRUE;
}
} else if (m_numNodes == 6) {
if (m_connectionRound >= 5) {
return TRUE;
}
} else if (m_numNodes == 7) {
if (m_connectionRound >= 7) {
return TRUE;
}
} else if (m_numNodes == 8) {
if (m_connectionRound >= 7) {
return TRUE;
}
if ((m_numNodes == 2 && m_connectionRound >= 1) ||
(m_numNodes == 3 && m_connectionRound >= 3) ||
(m_numNodes == 4 && m_connectionRound >= 3) ||
(m_numNodes == 5 && m_connectionRound >= 5) ||
(m_numNodes == 6 && m_connectionRound >= 5) ||
(m_numNodes == 7 && m_connectionRound >= 7) ||
(m_numNodes == 8 && m_connectionRound >= 7)) {
return TRUE;
}
return FALSE;
}
Expand Down
79 changes: 54 additions & 25 deletions Generals/Code/GameEngine/Source/GameNetwork/udp.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -327,33 +327,62 @@ UDP::sockStat UDP::GetStatus(void)
Int status = m_lastError;
#ifdef _WINDOWS
//int status=WSAGetLastError();
if (status==0) return(OK);
else if (status==WSAEINTR) return(INTR);
else if (status==WSAEINPROGRESS) return(INPROGRESS);
else if (status==WSAECONNREFUSED) return(CONNREFUSED);
else if (status==WSAEINVAL) return(INVAL);
else if (status==WSAEISCONN) return(ISCONN);
else if (status==WSAENOTSOCK) return(NOTSOCK);
else if (status==WSAETIMEDOUT) return(TIMEDOUT);
else if (status==WSAEALREADY) return(ALREADY);
else if (status==WSAEWOULDBLOCK) return(WOULDBLOCK);
else if (status==WSAEBADF) return(BADF);
else return((UDP::sockStat)status);
switch (status) {
case NO_ERROR:
return OK;
case WSAEINTR:
return INTR;
case WSAEINPROGRESS:
return INPROGRESS;
case WSAECONNREFUSED:
return CONNREFUSED;
case WSAEINVAL:
return INVAL;
case WSAEISCONN:
return ISCONN;
case WSAENOTSOCK:
return NOTSOCK;
case WSAETIMEDOUT:
return TIMEDOUT;
case WSAEALREADY:
return ALREADY;
case WSAEWOULDBLOCK:
return WOULDBLOCK;
case WSAEBADF:
return BADF;
default:
return (UDP::sockStat)status;
}
#else
//int status=errno;
if (status==0) return(OK);
else if (status==EINTR) return(INTR);
else if (status==EINPROGRESS) return(INPROGRESS);
else if (status==ECONNREFUSED) return(CONNREFUSED);
else if (status==EINVAL) return(INVAL);
else if (status==EISCONN) return(ISCONN);
else if (status==ENOTSOCK) return(NOTSOCK);
else if (status==ETIMEDOUT) return(TIMEDOUT);
else if (status==EALREADY) return(ALREADY);
else if (status==EAGAIN) return(AGAIN);
else if (status==EWOULDBLOCK) return(WOULDBLOCK);
else if (status==EBADF) return(BADF);
else return(UNKNOWN);
switch (status) {
case 0:
return OK;
case EINTR:
return INTR;
case EINPROGRESS:
return INPROGRESS;
case ECONNREFUSED:
return CONNREFUSED;
case EINVAL:
return INVAL;
case EISCONN:
return ISCONN;
case ENOTSOCK:
return NOTSOCK;
case ETIMEDOUT:
return TIMEDOUT;
case EALREADY:
return ALREADY;
case EAGAIN:
return AGAIN;
case EWOULDBLOCK:
return WOULDBLOCK;
case EBADF:
return BADF;
default:
return UNKNOWN;
}
#endif
}

Expand Down
Loading
Loading