2011-02-09 13 views
8
int uploadsID; 
int pageNumber; 
int x; 
int y; 
int w; 
int h; 

bool isValidUploadID = int.TryParse(context.Request.QueryString["uploadID"], out uploadsID); 
bool isValidPage = int.TryParse(context.Request.QueryString["page"], out pageNumber); 
bool isValidX = int.TryParse(context.Request.QueryString["x"], out x); 
bool isValidY = int.TryParse(context.Request.QueryString["y"], out y); 
bool isValidW = int.TryParse(context.Request.QueryString["w"], out w); 
bool isValidH = int.TryParse(context.Request.QueryString["h"], out h); 

if (isValidUploadID && isValidPage && isValidX && isValidY & isValidW & isValidH) 
{ 

Este es un controlador de ajax, verificando que todos los parámetros aprobados estén bien. ¿Esto se considera malo, y hay una mejor manera de escribir esto, o no es tan importante?C# ¿hay alguna manera más agradable de escribir esto?

+1

Puede usar un objeto que contenga todas las propiedades y un constructor que requiera NameValueCollection (Request.QueryString o simplemente Request) y prepare object, este objeto expone un método: "IsValid" y verifica si todos los parámetros son correctos para la solicitud actual. –

+1

Es posible que desee considerar publicar esto en [codereview] (http://codereview.stackexchange.com/) – Benjol

Respuesta

7

Suponiendo que no va a use las variables individuales bool en otro lugar, usted podría escritura que a medida:

int uploadsID, pageNumber, x, y, w, h; 
if (int.TryParse(context.Request.QueryString["uploadID"], out uploadsID) && 
    int.TryParse(context.Request.QueryString["page"], out pageNumber) && 
    int.TryParse(context.Request.QueryString["x"], out x) && 
    int.TryParse(context.Request.QueryString["y"], out y) && 
    int.TryParse(context.Request.QueryString["w"], out w) && 
    int.TryParse(context.Request.QueryString["h"], out h)) 
{ 
} 

Es posible que desee extraer cabo int.TryParse(context.Request.QueryString[name], out variable en un método separado, dejándole con algo como:

int uploadsID, pageNumber, x, y, w, h; 
if (TryParseContextInt32("uploadID", out uploadsID) && 
    TryParseContextInt32("page", out pageNumber) && 
    TryParseContextInt32("x", out x) && 
    TryParseContextInt32("y", out y) && 
    TryParseContextInt32("w", out w) && 
    TryParseContextInt32("h", out h)) 
{ 
} 

Como alternativa, puede encapsular todos estos datos de contexto en un nuevo tipo con un método TryParse, por lo que tendría algo como:

PageDetails details; 
if (PageDetails.TryParse(context.Request.QueryString)) 
{ 
    // Now access details.Page, details.UploadID etc 
} 

Eso es, obviamente, más trabajo, pero creo que sería hacer el código más limpio.

+0

Eso es bueno gracias :) –

2

Una cosa que puede hacer es sustituir este:

int uploadsID; 
int pageNumber; 
int x; 
int y; 
int w; 
int h; 

Con esta

int uploadsID, pageNumber, x, y, w, h; 
+0

Downvote está bien, pero se agradecerá una razón. ¿Obviamente crees que el primer código tiene una legibilidad superior a mi código modificado? –

6

Sí, empezar por factorización de su int.TryParse(etc.) en una función separada.

(posiblemente influenciado más por F #)

//return a tuple (valid, value) from querystring of context, indexed with key 
private Tuple<bool, int> TryGet(HttpContext context, string key) 
{ 
    int val = 0; 
    bool ok = int.TryParse(context.request.QueryString[key], out val); 
    return Tuple.New(ok, val); 
} 

continuación:

var UploadId = TryGet(context, "uploadID"); 
//... 
if(UploadId.Item1 && etc..) 
{ 
    //do something with UploadId.Item2; 

Para hacer las cosas un poco más claro, usted podría

private class ValidValue 
{ 
    public bool Valid { get; private set; } 
    public int Value { get; private set; } 
    public ValidValue(bool valid, int value) 
    { 
     Valid = valid; 
     Value = value; 
    } 
    //etc., but this seems a bit too much like hard work, and you don't get 
    // equality for free as you would with Tuple, (if you need it) 
+0

Más como una clase separada, que valida sus propias propiedades. – Turrau

+1

¿Te importa expandir esto por favor? –

3

probablemente me iría para un formato como este


int uploadsID, pageNumber, x, y, h; 

if (Int32.TryParse(context.Request.QueryString["uploadID"], out uploadsID) 
    && Int32.TryParse(context.Request.QueryString["page"], out pageNumber) 
    && Int32.TryParse(context.Request.QueryString["x"], out x) 
    && Int32.TryParse(context.Request.QueryString["y"], out y) 
    && Int32.TryParse(context.Request.QueryString["w"], out w) 
    && Int32.TryParse(context.Request.QueryString["h"], out h)) 
{ 
    ... 
} 

pero yo no veo nada de malo en su enfoque.

1
try 
{ 
    // use Parse instead of TryParse 

    // all are valid, proceed 
} 
catch 
{ 
    // at least one is not valid 
} 
+0

Por lo general, es una mala idea incluir excepciones como parte del flujo normal del programa, y ​​en este caso, la entrada del usuario equivocada es obviamente una parte normal del flujo del programa, y ​​* no * debe ser manejado a través de excepciones. No es muy excepcional que un usuario ingrese algo incorrectamente. –

+0

Eso es claramente malo. No solo está utilizando excepciones para el flujo de control, sino que también perjudica el rendimiento. – Stilgar

+0

Parece una buena solución para mí si la declaración catch devuelve un código de error o vuelve a lanzar. @Stilgar: ¿realmente te importa el rendimiento en casos de error? – adrianm

0

Se podría escribir un ayudante que se deshace del estilo out paso fea de TryParse, tales como:

public delegate bool TryParser<T>(string text, out T result) where T : struct; 

public static T? TryParse<T>(string text, TryParser<T> tryParser) 
          where T : struct 
{ 
    // null checks here. 
    T result; 
    return tryParser(text, out result) ? result : new T?(); 
} 

Y entonces (suponiendo que sólo está interesado en la validez):

bool isValid = new [] { "uploadID" , "page", "x", "y", "w", "h" } 
       .Select(q => context.Request.QueryString[q]) 
       .All(t => TryParse<int>(t, int.TryParse).HasValue); 

Si necesita los valores individuales:

var numsByKey = new [] { "uploadID" , "page", "x", "y", "w", "h" } 
       .ToDictionary(q => q, 
          q => TryParse<int>(context.Request.QueryString[q], 
               int.TryParse)); 

bool isValid = numsByKey.Values.All(n => n.HasValue); 

Esto conserva prácticamente la misma información que antes, excepto que la información detallada necesita una búsqueda en lugar de un acceso de variable local.

Cuestiones relacionadas