En esta función que maneja una llamada REST API, cualquiera de las funciones llamadas para manejar partes de la solicitud puede generar un error para indicar que se debe enviar un código de error como respuesta. Sin embargo, la función en sí misma también podría descubrir un error, en cuyo punto debería saltar al bloque de manejo de excepciones.

static async handleRequest(req) {
    try {
        let isAllowed = await checkIfIsAllowed(req);
        if (!isAllowed) {
            throw new ForbiddenException("You're not allowed to do that.");
        }
        let result = await doSomething(req); // can also raise exceptions
        sendResult(result);
    } catch(err) {
        sendErrorCode(err);
    }
}

Webstorm subrayará el throw con el siguiente mensaje: 'throw' of exception caught locally. This inspection reports any instances of JavaScript throw statements whose exceptions are always caught by containing try statements. Using throw statements as a "goto" to change the local flow of control is likely to be confusing.

Sin embargo, no estoy seguro de cómo refactorizar el código para mejorar la situación.

Podría copiar el código del bloque catch en la comprobación if, pero creo que esto haría que mi código sea menos legible y más difícil de mantener.

Podría escribir una nueva función que haga la comprobación isAllowed y arroje una excepción si no tiene éxito, pero eso parece evitar el problema, en lugar de solucionar un problema de diseño que supuestamente informa Webstorm.

¿Estamos usando las excepciones de una manera incorrecta, y es por eso que estamos encontrando este problema, o el error de Webstorm es simplemente erróneo y debería deshabilitarse?

32
cib 30 oct. 2017 a las 15:48

7 respuestas

La mejor respuesta

Estás buscando algo y lanzando una excepción si isAllowed falla, pero sabes qué hacer en esa situación: llama a sendErrorCode. Debe lanzar excepciones a las llamadas externas si no sabe cómo manejar la situación, es decir, en circunstancias excepcionales.

En este caso, ya tiene un proceso definido de qué hacer si esto sucede, simplemente utilícelo directamente sin el lanzamiento / captura indirecto:

static async handleRequest(req) {
    try {
        let isAllowed = await checkIfIsAllowed(req);
        if (!isAllowed) {
            sendErrorCode("You're not allowed to do that.");
            return;
        }
        let result = await doSomething(req); // can also raise exceptions
        sendResult(result);
    } catch(err) {
        sendErrorCode(err);
    }
}

Podría copiar el código del bloque catch en la comprobación if, pero creo que esto haría que mi código sea menos legible y más difícil de mantener.

Por el contrario, como anteriormente, esperaría que esta sea la forma de manejar esta situación.

29
James Thorpe 30 oct. 2017 a las 13:16

Hay buenas respuestas a la pregunta "¿Por qué no usar excepciones como control de flujo normal?" aquí.

La razón para no lanzar una excepción que atraparás localmente es que localmente sabes cómo manejar esa situación, por lo que, por definición, no es excepcional.

La respuesta de @James Thorpe me parece buena, pero @matchish siente que viola DRY. Yo digo que, en general, no lo hace. DRY, que significa No te repitas, se define por las personas que acuñaron la frase como "Todo conocimiento debe tener una representación autoritaria, inequívoca y única dentro de un sistema". Tal como se aplica a la escritura de código de software, se trata de no repetir el código complejo .

Prácticamente cualquier código que se dice que viola DRY se dice que está "arreglado" extrayendo el código repetido en una función y luego llamando esa función desde los lugares donde se repitió anteriormente. Tener varias partes de su código de llamada sendErrorCode es la solución para solucionar un problema SECO. Todo el conocimiento de qué hacer con el error está en un lugar definitivo, a saber, la función sendErrorCode.

Modificaría ligeramente la respuesta de @James Thorpe, pero es más una objeción que una crítica real, que es que sendErrorCode debería recibir objetos o cadenas de excepción, pero no ambos:

static async handleRequest(req) {
    try {
        let isAllowed = await checkIfIsAllowed(req);
        if (!isAllowed) {
            sendErrorCode(new ForbiddenException("You're not allowed to do that."));
            return;
        }
        let result = await doSomething(req); // can also raise exceptions
        sendResult(result);
    } catch(err) {
        sendErrorCode(err);
    }
}

La pregunta más importante es cuál es la probabilidad del error y es apropiado tratar !isAllowed como una excepción. Las excepciones están destinadas a manejar situaciones inusuales o impredecibles. Esperaría que !isAllowed sea una ocurrencia normal que debería manejarse con lógica específica para esa situación, a diferencia, por ejemplo, de una incapacidad repentina para consultar la base de datos que tiene la respuesta a la pregunta isAllowed.

La solución propuesta de @ matchish cambia el contrato de doSomethingOnAllowedRequest de algo que nunca arrojará una excepción a algo que rutinariamente lanzar una excepción, colocando la carga del manejo de excepciones en todas sus personas que llaman. Es probable que esto cause una violación de DRY al hacer que varias personas que llaman tengan repeticiones del mismo código de manejo de errores, por lo que en resumen no me gusta. En la práctica, dependería de la situación general, como la cantidad de personas que llaman y, de hecho, comparten la misma respuesta a los errores.

1
Old Pro 19 ene. 2019 a las 23:56

La respuesta de James Thorpe tiene una desventaja en mi opinión. No está SECO, en ambos casos cuando llamas a sendError manejas Excepciones. Imaginemos que tenemos muchas líneas de código con lógica como esta donde se puede lanzar Exception. Creo que puede ser mejor.

Esta es mi solucion

async function doSomethingOnAllowedRequest(req) {
    let isAllowed = await checkIfIsAllowed(req);
    if (!isAllowed) {
       throw new ForbiddenException("You're not allowed to do that.");
    }
    doSomething(req);
}
static async handleRequest(req) {
    try {
        let result = await doSomethingOnAllowedRequest(req);
        sendResult(result);
    } catch(err) {
        sendErrorCode(err);
    }
}
1
matchish 17 ene. 2019 a las 16:41

Esto podría darle algunos consejos, tal vez esa sea la causa (no estoy seguro si es relevante). La declaración de captura no detecta el error arrojado

"La razón por la cual su bloque try catch está fallando es porque una solicitud ajax es asíncrona. El bloque try catch se ejecutará antes de la llamada Ajax y enviará la solicitud en sí, pero el error se produce cuando se devuelve el resultado, EN UN PUNTO MÁS TARDE HORA.

Cuando se ejecuta el bloque try catch, no hay error. Cuando se produce el error, no hay intento de captura. Si necesita probar catch para solicitudes ajax, siempre coloque bloques ajax try catch dentro de la devolución de llamada correcta, NUNCA fuera de ella ".

-3
Di V 30 oct. 2017 a las 13:15

Contrariamente a la opinión de James Thorpe, prefiero ligeramente el patrón de lanzamiento. No veo ninguna razón convincente para tratar los errores locales en el bloque de prueba de manera diferente a los errores que surgen desde lo más profundo de la pila de llamadas ... simplemente tírelos. En mi opinión, esta es una mejor aplicación de consistencia.

Debido a que este patrón es más consistente, naturalmente se presta mejor para refactorizar cuando desea extraer la lógica en el bloque try a otra función que tal vez esté en otro módulo / archivo.

// main.js
try {
  if (!data) throw Error('missing data')
} catch (error) {
  handleError(error)
}

// Refactor...

// validate.js
function checkData(data) {
  if (!data) throw Error('missing data')
}

// main.js
try {
  checkData(data)
} catch (error) {
  handleError(error)
}

Si en lugar de tirar el bloque try maneja el error, entonces la lógica tiene que cambiar si lo refactoriza fuera del bloque try.

Además, manejar el error tiene el inconveniente de recordarle que debe regresar temprano para que el bloque try no continúe ejecutando la lógica después de encontrar el error. Esto puede ser bastante fácil de olvidar.

try {
  if (!data) {
    handleError(error)
    return // if you forget this, you might execute code you didn't mean to. this isn't a problem with throw.
  }
  // more logic down here
} catch (error) {
  handleError(error)
}

Si le preocupa qué método es más eficaz, no debería serlo. Manejar el error es técnicamente más eficaz, pero la diferencia entre los dos es absolutamente trivial.

Considere la posibilidad de que WebStorm sea demasiado obstinado aquí. ESLint ni siquiera tiene una regla para esto. Cualquiera de los patrones es completamente válido.

8
J. Munson 11 dic. 2019 a las 13:11

Como este no es un error de bloqueo, sino solo una recomendación del IDE, la pregunta debe verse desde dos lados.

El primer lado es el rendimiento. Si se trata de un cuello de botella y es posible usarlo con compilación o al transferirlo a versiones nuevas (aún no lanzadas) de nodejs, la presencia de repeticiones no siempre es una mala solución. Parece que el IDE sugiere precisamente en este caso y que tal diseño puede conducir a una optimización deficiente en algunos casos.

El segundo lado es el diseño del código. Si hará que el código sea más legible y simplifique el trabajo para otros desarrolladores, consérvelo. Desde este punto de vista, las soluciones ya se han propuesto anteriormente.

3
SlowSuperman 21 ene. 2019 a las 11:33

Devuelve un rechazo de promesa en lugar de arrojar un error en el bloque de prueba

  try {
    const isAllowed = await checkIfIsAllowed(request);

    if (!isAllowed) {
      return Promise.reject(Error("You're not allowed to do that."));
    }

    const result = await doSomething(request);

    sendResult(result);
  } catch (error) {
    throw error;
  }
0
backslash 17 mar. 2020 a las 15:31