-
Notifications
You must be signed in to change notification settings - Fork 117
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.
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
base: main
Are you sure you want to change the base?
refactor: convert more if-else chains to switch statements #1683
Conversation
| // 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) { |
There was a problem hiding this comment.
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;
}
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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;
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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) ||....
Refactored: