Simplify the code in C#

Question

I wonder how could I simplify the code below? Please note that it's not allowed to use instance variables here for some reason.

        private bool ValidateQueryString(NameValueCollection queryString, out byte[] errorMessages)
        {
            var errorsList = new List<string>();
            int _aId;
            if (!int.TryParse(queryString["a_id"], out _aId))
            {
                errorMessageList.Add("aId invalid");
            }

            string bName = queryString["b"];
            if (string.IsNullOrWhiteSpace(bName))
            {
                errorsList.Add("b invalid");
            }
            else
            {
                string fullFileName = Path.Combine(MainFolder, bName);
                if (!File.Exists(fullFileName))
                {
                    errorsList.Add("file not exist;");
                }
            }

            if (errorsList.Any())
            {
                using (var memoryStream = new MemoryStream())
                {
                    byte[] newLineByteArray = Encoding.UTF8.GetBytes(Environment.NewLine);
                    foreach (var item in errorsList)
                    {
                        byte[] currentErrorMessage = Encoding.UTF8.GetBytes(item);
                        memoryStream.Write(currentErrorMessage, 0, currentErrorMessage.Length);
                        memoryStream.Write(newLineByteArray, 0, newLineByteArray.Length);
                    }

                    errorMessages = memoryStream.ToArray();
                    return false;
                }
            }
            else
            {
                errorMessages = null;
                return true;
            }
        }


        private QueryStringData GetQueryStringData(NameValueCollection queryString)
        {
            string bName = Path.Combine(_folder, queryString["b"]);
            int aId = int.Parse(queryString["a_id"]);
            return new QueryStringData(bName, aId); 
        }


         private void ProcessData()
         {    
            using (Stream outputStream = context.Response.OutputStream)
            using (MemoryStream outputMemoryStream = new MemoryStream())
            {
                byte[] errorMessages;
                long streamLength;
                if (ValidateQueryString(context.Request.QueryString, out errorMessages))
                {
                    QueryStringData queryStringData = GetQueryStringData(context.Request.QueryString);
                    byte[] value = GetValueByID();
                    if (value != null)
                    {
                        context.Response.StatusCode = (int)HttpStatusCode.OK;
                        outputMemoryStream.Write(value, 0, value.Length);
                        streamLength = value.LongLength;
                    }
                    else
                    {
                        context.Response.StatusCode = (int)HttpStatusCode.NotFound;
                        byte[] errorMessage = Encoding.UTF8.GetBytes(string.Format("not found", queryStringData.Var1));
                        outputMemoryStream.Write(errorMessage, 0, errorMessage.Length);
                        streamLength = errorMessage.LongLength;
                    }

                    context.Response.ContentLength64 = streamLength;
                    outputStream.Write(outputMemoryStream.ToArray(), 0, (int)streamLength);
                }
                else
                {
                    context.Response.StatusCode = (int)HttpStatusCode.BadRequest;
                    outputMemoryStream.Write(errorMessages, 0, errorMessages.Length);
                    streamLength = errorMessages.LongLength;
                    context.Response.ContentLength64 = streamLength;
                    outputStream.Write(outputMemoryStream.ToArray(), 0, (int)streamLength);
                }
            }
}

}

Answer 1

First of all, your solution overuses the MemoryStream object. You can write your data directly to output stream using StreamWriter. Also, don't pass byte arrays around, it can easily cause memory pressure issues once your site hits any load. Instead of returning byte array just pass the same StreamWriter object to the method that is expected to add smth. to output.

Also your methods have incorrect names, e.g. GetValueByID method doesn't accept any parameters (while method name suggests there should be one), and it returns a binary data rather than "value" (I assume it also uses MemoryStream to serialize the value into binary).

You should rewrite GetValueByID to either return a typed object representing a "value" and write it to stream, or pass the StreamWriter so that method has a chance to write everything directly to the stream.

I would provide a solution that assumes that original "value" type in GetValueByID is string, you can adjust it as necessary. Also I replaced ValidateQueryString with GetQueryStringValidationErrors that returns enumerable of errors, it simplifies the code of the method. The only method that takes care of data serialization and pushing it to stream is the ProcessData method.

private IEnumerable<string> GetQueryStringValidationErrors(NameValueCollection queryString)
{
    int aId;
    if (!int.TryParse(queryString["a_id"], out aId))
        yield return "aId invalid";

    string bName = queryString["b"];
    if (string.IsNullOrWhiteSpace(bName))
        yield return "b invalid";
    else if (!File.Exists(Path.Combine(MainFolder, bName)))
        yield return "file not exist;";
}


private QueryStringData GetQueryStringData(NameValueCollection queryString)
{
    string bName = Path.Combine(_folder, queryString["b"]);
    int aId = int.Parse(queryString["a_id"]);
    return new QueryStringData(bName, aId);
}

private void ProcessData()
{
    using (StreamWriter writer = new StreamWriter(context.Response.OutputStream))
    {
        List<string> errors = GetQueryStringValidationErrors(context.Request.QueryString).ToList();
        if (errors.Count > 0)
        {
            context.Response.StatusCode = (int)HttpStatusCode.BadRequest;
            errors.ForEach(writer.WriteLine);
            return;
        }

        string value = GetValueByID(/*you should pass the ID here*/);
        if (value != null)
        {
            context.Response.StatusCode = (int)HttpStatusCode.OK;
            writer.Write(value);
        }
        else
        {
            context.Response.StatusCode = (int)HttpStatusCode.NotFound;
            //Don't think you need QueryStringData object here
            QueryStringData queryStringData = GetQueryStringData(context.Request.QueryString);
            writer.Write("not found {0}", queryStringData.Var1);
        }
    }
}

Answer 2

I notice that methods should be separated.

private bool TryGetQSErrors(NameValueCollection queryString, out List<string> errorsList)
{
    int aId;
    if (!int.TryParse(queryString["a_id"], out aId))
    {
        errorsList.Add("aId invalid");
    }

    string bName = queryString["b"];
    if (string.IsNullOrWhiteSpace(bName))
    {
        errorsList.Add("b invalid");
    }
    else
    {
        string fullFileName = Path.Combine(MainFolder, bName);
        if (!File.Exists(fullFileName))
        {
            errorsList.Add("file not exist;");
        }
    }
    return errorsList.Any();
}

private static byte[] CreateErrorsByteArray(List<string> errorsList)
{
    using (var memoryStream = new MemoryStream())
    {
        byte[] newLineByteArray = Encoding.UTF8.GetBytes(Environment.NewLine);
        foreach (var item in errorsList)
        {
            byte[] currentErrorMessage = Encoding.UTF8.GetBytes(item);
            memoryStream.Write(currentErrorMessage, 0, currentErrorMessage.Length);
            memoryStream.Write(newLineByteArray, 0, newLineByteArray.Length);
        }

        return memoryStream.ToArray();
    }
}

ValidateQueryString is not good name for method as it creates bytes array too. I prefer to use TryGetSomething names for this kind of methods. It is more readable.