From 53a5728009adf1e91115ee0eb839eafa00262adf Mon Sep 17 00:00:00 2001 From: Jaana Dogan Date: Mon, 21 Apr 2025 12:15:47 -0700 Subject: [PATCH] Remove redundant else branches (#86) Else branches are an anti pattern especially if you can easily return from the previous branch. Over time, else branches cause deep nesting and make code unreadable and unmaintainable. Remove elses where possible. --- packages/cli/src/gemini.ts | 27 +++++----- packages/cli/src/ui/hooks/useStdin.ts | 10 ++-- packages/server/src/core/gemini-client.ts | 5 +- packages/server/src/core/turn.ts | 49 +++++++++---------- .../src/utils/BackgroundTerminalAnalyzer.ts | 29 ++++++----- 5 files changed, 59 insertions(+), 61 deletions(-) diff --git a/packages/cli/src/gemini.ts b/packages/cli/src/gemini.ts index 60863c6a..97502399 100644 --- a/packages/cli/src/gemini.ts +++ b/packages/cli/src/gemini.ts @@ -54,20 +54,21 @@ process.on('unhandledRejection', (reason, _promise) => { ); console.warn('-----------------------------------------'); console.warn('Reason:', reason); - // No process.exit(1); - } else { - // Log other unexpected unhandled rejections as critical errors - console.error('========================================='); - console.error('CRITICAL: Unhandled Promise Rejection!'); - console.error('========================================='); - console.error('Reason:', reason); - console.error('Stack trace may follow:'); - if (!(reason instanceof Error)) { - console.error(reason); - } - // Exit for genuinely unhandled errors - process.exit(1); + return; + // No process.exit(1); Don't exit. } + + // Log other unexpected unhandled rejections as critical errors + console.error('========================================='); + console.error('CRITICAL: Unhandled Promise Rejection!'); + console.error('========================================='); + console.error('Reason:', reason); + console.error('Stack trace may follow:'); + if (!(reason instanceof Error)) { + console.error(reason); + } + // Exit for genuinely unhandled errors + process.exit(1); }); // --- Global Entry Point --- diff --git a/packages/cli/src/ui/hooks/useStdin.ts b/packages/cli/src/ui/hooks/useStdin.ts index e91b90e6..dc245254 100644 --- a/packages/cli/src/ui/hooks/useStdin.ts +++ b/packages/cli/src/ui/hooks/useStdin.ts @@ -70,13 +70,13 @@ export function usePipedInput(): PipedInputState { stdin.removeListener('error', handleError); stdin.removeListener('end', handleEnd); }; - } else { - // No piped input (running interactively) - setIsLoading(false); - // Optionally set an 'info' state or just let isLoading=false & isPiped=false suffice - // setError('No piped input detected.'); // Maybe don't treat this as an 'error' } + // No piped input (running interactively) + setIsLoading(false); + // Optionally set an 'info' state or just let isLoading=false & isPiped=false suffice + // setError('No piped input detected.'); // Maybe don't treat this as an 'error' + // Intentionally run only once on mount or when stdin theoretically changes }, [stdin, isRawModeSupported, setRawMode /*, exit */]); diff --git a/packages/server/src/core/gemini-client.ts b/packages/server/src/core/gemini-client.ts index b9b44534..5829afa8 100644 --- a/packages/server/src/core/gemini-client.ts +++ b/packages/server/src/core/gemini-client.ts @@ -129,10 +129,9 @@ export class GeminiClient { if (error instanceof Error && error.name === 'AbortError') { console.log('Gemini stream request aborted by user.'); throw error; - } else { - console.error(`Error during Gemini stream or tool interaction:`, error); - throw error; } + console.error(`Error during Gemini stream or tool interaction:`, error); + throw error; } } diff --git a/packages/server/src/core/turn.ts b/packages/server/src/core/turn.ts index 31656466..a93cbc57 100644 --- a/packages/server/src/core/turn.ts +++ b/packages/server/src/core/turn.ts @@ -152,14 +152,13 @@ export class Turn { ); if (confirmationDetails) { return { ...pendingToolCall, confirmationDetails }; - } else { - const result = await tool.execute(pendingToolCall.args); - return { - ...pendingToolCall, - result, - confirmationDetails: undefined, - }; } + const result = await tool.execute(pendingToolCall.args); + return { + ...pendingToolCall, + result, + confirmationDetails: undefined, + }; } catch (execError: unknown) { return { ...pendingToolCall, @@ -191,17 +190,17 @@ export class Turn { type: GeminiEventType.ToolCallConfirmation, value: serverConfirmationetails, }; - } else { - const responsePart = this.buildFunctionResponse(outcome); - this.fnResponses.push(responsePart); - const responseInfo: ToolCallResponseInfo = { - callId: outcome.callId, - responsePart, - resultDisplay: outcome.result?.returnDisplay, - error: outcome.error, - }; - yield { type: GeminiEventType.ToolCallResponse, value: responseInfo }; } + const responsePart = this.buildFunctionResponse(outcome); + this.fnResponses.push(responsePart); + const responseInfo: ToolCallResponseInfo = { + callId: outcome.callId, + responsePart, + resultDisplay: outcome.result?.returnDisplay, + error: outcome.error, + }; + yield { type: GeminiEventType.ToolCallResponse, value: responseInfo }; + return; } } @@ -225,23 +224,23 @@ export class Turn { // Builds the Part array expected by the Google GenAI API private buildFunctionResponse(outcome: ServerToolExecutionOutcome): Part { const { name, result, error } = outcome; - let fnResponsePayload: Record; - if (error) { // Format error for the LLM const errorMessage = error?.message || String(error); - fnResponsePayload = { error: `Tool execution failed: ${errorMessage}` }; console.error(`[Server Turn] Error executing tool ${name}:`, error); - } else { - // Pass successful tool result (content meant for LLM) - fnResponsePayload = { output: result?.llmContent ?? '' }; // Default to empty string if no content + return { + functionResponse: { + name, + id: outcome.callId, + response: { error: `Tool execution failed: ${errorMessage}` }, + }, + }; } - return { functionResponse: { name, id: outcome.callId, - response: fnResponsePayload, + response: { output: result?.llmContent ?? '' }, }, }; } diff --git a/packages/server/src/utils/BackgroundTerminalAnalyzer.ts b/packages/server/src/utils/BackgroundTerminalAnalyzer.ts index 31cd91c6..5ff29997 100644 --- a/packages/server/src/utils/BackgroundTerminalAnalyzer.ts +++ b/packages/server/src/utils/BackgroundTerminalAnalyzer.ts @@ -299,32 +299,31 @@ export class BackgroundTerminalAnalyzer { const { stdout } = await execAsync(command); // Check if the output contains the process information (it will have the image name if found) return stdout.toLowerCase().includes('.exe'); // A simple check, adjust if needed - } else { - // Linux/macOS/Unix-like: Use kill -0 signal - // process.kill sends signal 0 to check existence without killing - process.kill(pid, 0); - return true; // If no error is thrown, process exists } + // Linux/macOS/Unix-like: Use kill -0 signal + // process.kill sends signal 0 to check existence without killing + process.kill(pid, 0); + return true; // If no error is thrown, process exists } catch (error: unknown) { if (isNodeError(error) && error.code === 'ESRCH') { // ESRCH: Standard error code for "No such process" on Unix-like systems return false; - } else if ( + } + if ( process.platform === 'win32' && getErrorMessage(error).includes('No tasks are running') ) { // tasklist specific error when PID doesn't exist return false; - } else { - // Other errors (e.g., EPERM - permission denied) mean we couldn't determine status. - // Re-throwing might be appropriate depending on desired behavior. - // Here, we log it and cautiously return true, assuming it *might* still be running. - console.warn( - `isProcessRunning(${pid}) encountered error: ${getErrorMessage(error)}. Assuming process might still exist.`, - ); - // Or you could throw the error: throw new Error(`Failed to check process status for PID ${pid}: ${error.message}`); - return true; // Cautious assumption } + // Other errors (e.g., EPERM - permission denied) mean we couldn't determine status. + // Re-throwing might be appropriate depending on desired behavior. + // Here, we log it and cautiously return true, assuming it *might* still be running. + console.warn( + `isProcessRunning(${pid}) encountered error: ${getErrorMessage(error)}. Assuming process might still exist.`, + ); + // Or you could throw the error: throw new Error(`Failed to check process status for PID ${pid}: ${error.message}`); + return true; // Cautious assumption } }