-
Notifications
You must be signed in to change notification settings - Fork 131
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
refactor: switch case the error codes and add some new ones #283
Conversation
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.
Nested switches are 🤮🤮
switch (exitCode) { | ||
case 0x3: { | ||
// returned when selecting update in game | ||
return null; | ||
} | ||
case 0xc0000135: | ||
case 0xc0000409: | ||
case 0xc000007b: { | ||
return "Required DLLs for launching Dolphin are missing. Check the Help section in the settings page to fix this issue."; | ||
} | ||
case 0xc0000005: { | ||
return "Install the latest Windows update available and then restart your computer."; | ||
} | ||
} | ||
break; | ||
} |
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.
Yeah nah. Move this to a separate function like handleWindowsErrorCode(code: number): string | null
then you can have your switch statement in there. Likewise with the linux codes.
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.
Also while you're at it, move those error handling functions and the handleDolphinExitCode
function outside of the useAppListeners
hook. We don't really need to define them inside the main hook.
src/renderer/lib/utils.ts
Outdated
return "Install the latest Windows update available and then restart your computer."; | ||
} | ||
} | ||
return null; |
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 we move these return null
into the default case?
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.
LGTM
I'm on the fence about how I rewrote this. maybe nested switch statements are bad design, but it does make it easier to add new error codes.