Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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);
                }
            }
}

}

share|improve this question
    
Is the method signature of ValidateQueryString fixed or can it be changed? –  Rob White Nov 14 '12 at 15:45
    
It can be changed. The idea is to write data (byte[]) to outputStream in ProccessData(). –  Alexandre Nov 14 '12 at 15:59
    
May I ask why it can't use instance variables? Does that include creating inner classes? –  dreza Nov 14 '12 at 20:32
    
It doesn't include. Even private variables are not allowed. –  Alexandre Nov 15 '12 at 1:51
2  
-1: please include relevant information in the question, instead of having odd limitations such as please note that it's not allowed to use instance variables here for some reason that you later accidentally clarify in comments such as (...) it's multithreaded app and IEnumerable make local variables be shared among the other threads. Sorry, I forgot to mention it. –  ANeves Nov 15 '12 at 16:07
show 2 more comments

2 Answers

up vote 4 down vote accepted

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);
        }
    }
}
share|improve this answer
    
Why are you using IEnumerable? Also I'd rather use if (errors.Any()) –  Alexandre Nov 15 '12 at 15:53
    
IEnumerable - to do yield return instead of creating and populating a list of errors. "if (errors.Any())" - agree, it would be slightly more expressive than checking count property –  almaz Nov 15 '12 at 15:57
    
What is the advantige of IEnumerable? I would not use it here because it's multithreaded app and IEnumerable make local variables be shared among the other threads. Sorry, I forgot to mention it. –  Alexandre Nov 15 '12 at 16:01
    
IEnumerable is completely sfafe here from multithreading point of view. There is no "sharing" between threads happening here –  almaz Nov 15 '12 at 16:25
    
Really, there are only constants. You are wright. –  Alexandre Nov 15 '12 at 16:40
show 1 more comment

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.

share|improve this answer
    
That's wrong. Where is ProcessData()? How is it going to work? –  Alexandre Nov 15 '12 at 9:01
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.