5
\$\begingroup\$

I am currently working on a crash log sender. As the application crashes, the FindLogFiles() will search for the different crash logs we are maintaining. It will then process these files, check their size and send them to the server in fused, or split chunks of max 500k bytes (receive limit per file from the server we use).

Now I was wondering if the check could be simplified. I tried to use recursion to already simplify what I had before, but I still have a feeling it is overly complex.

class CrashLogger()
{
    private static List<string> ToBigFiles = new List<string>();
    private static List<string> ReadyToPostFiles = new List<string>();
    public static void PreSendingLogs(string incomingString)
    {
        List<string> queue = FindLogFiles._processingQueue;
        var text = "";

        // If limit has been reached, stop sending for xxx duration
        if (timesSendToServer >= 25)
        {
            Console.WriteLine("Limit reached. (" + timesSendToServer + "/" + 25 + ")");
            return;
        }

        if (queue.Count > 0)
        {
            try { using (StreamReader sr = new StreamReader(queue[0])) { text = sr.ReadToEnd(); } }
            catch (UnauthorizedAccessException ua) { LogFile.Log("unauthorized reading of file: " + queue[0] + "   ||   " + ua); }
            catch (Exception e) { LogFile.Log("Something went wrong in preparing the file: " + queue[0] + "   ||   " + e); }

            // Catch strings that are to short to be worth our time and dispose of them
            if (text.Length <= 0)
            {
                FindLogFiles._processingQueue.RemoveAt(0);
                PreSendingLogs("" + incomingString);
                return;
            }

            var textByteLength = Encoding.Unicode.GetBytes(text).Length;
            var incomingByteLength = Encoding.Unicode.GetBytes(incomingString).Length;

            // If the new file is big enough to post, or to big to post by itself. 
            // We store it for later proccesing, and go on to the next file.
            if (textByteLength >= 47500 && textByteLength < 500000)
            {
                ReadyToPostFiles.Add(queue[0]);
                FindLogFiles._processingQueue.RemoveAt(0);
                PreSendingLogs("" + incomingString);
                return;
            }
            else if (textByteLength >= 500000)
            {
                ToBigFiles.Add(queue[0]);
                FindLogFiles._processingQueue.RemoveAt(0);
                PreSendingLogs("" + incomingString);
                return;
            }

            var totalByteSize = textByteLength + incomingByteLength;

            if (totalByteSize < 500000 && totalByteSize > 475000)
            {
                // If the file size is big enough after the fusion. send it to Server
                if (SendToServer(text, false))
                {
                    Console.WriteLine("Succesfully posted to Server");
                    FindLogFiles._processingQueue.RemoveAt(0);
                }
            }
            else if (totalByteSize >= 500000)
            {
                ToBigFiles.Add(text + " ||--|||--|| " + incomingString);
                FindLogFiles._processingQueue.RemoveAt(0);
                PreSendingLogs("");
                return;
            }
            else
            {
                FindLogFiles._processingQueue.RemoveAt(0);
                PreSendingLogs(text + " ||--|||--|| " + incomingString);
                return;
            }
        }
        else if (incomingString.Length > 0)
        {
            // If the file size is big enough after the fusion. send it to server
            if (SendToServer(text, false))
            {
                Console.WriteLine("Succesfully posted to server");
                FindLogFiles._processingQueue.RemoveAt(0);
            }
            return;
        }
        else
        {
            // there is no queue and nothing incoming, what are we doing here?

            if (ReadyToPostFiles.Count > 0)
            {
                // We still have stuff ready to send

                try { using (StreamReader sr = new StreamReader(ReadyToPostFiles[0])) { text = sr.ReadToEnd(); } }
                catch (UnauthorizedAccessException ua) { LogFile.Log("unauthorized reading of file: " + ReadyToPostFiles[0] + "   ||   " + ua); }
                catch (Exception e) { LogFile.Log("Something went wrong in preparing the file: " + ReadyToPostFiles[0] + "   ||   " + e); }

                // In case of a uncaught exception
                if (text.Length > 0)
                {
                    if (SendToServer(text, false))
                    {
                        Console.WriteLine("Succesfully posted to pastebin");
                        FindLogFiles._processingQueue.RemoveAt(0);
                    }
                }
                else
                {
                    ReadyToPostFiles.RemoveAt(0);
                    throw new NullReferenceException("No text found in document, removing from queue: " + ReadyToPostFiles[0]);
                }
            }
            else if (ToBigFiles.Count > 0)
            {
                ProccesBigFiles();
            }
            else
            {
                return;
                throw new FileNotFoundException("No more file to procces found");
            }
        }
    }
}

The big files are currently split like this:

private static void ProccesBigFiles()
{
    var maxLength = 495000;
    var text = "";

    for (int i = 0; i < ToBigFiles.Count; i++)
    {
        try { using (StreamReader sr = new StreamReader(ReadyToPostFiles[0])) { text = sr.ReadToEnd(); } }
        catch (UnauthorizedAccessException ua) { LogFile.Log("unauthorized reading of file: " + ReadyToPostFiles[0] + "   ||   " + ua); continue; }
        catch (Exception e) { LogFile.Log("Something went wrong in preparing the file: " + ReadyToPostFiles[0] + "   ||   " + e); continue; }

        var textByteLength = Encoding.Unicode.GetBytes(text).Length;
        var splitPieces = 0;

        splitPieces = (int)(textByteLength/maxLength);

        for (int e = 0; e < splitPieces; e++)
        {
            ReadyToPostFiles.Add(text.Substring(e * 225000, 225000));
        }
    }

    ToBigFiles = new List<string>();
    GC.Collect();
}
\$\endgroup\$
3
\$\begingroup\$
return;
throw new FileNotFoundException("No more file to procces found");

The second line here is unreachable, and should trigger a warning from your compiler.


List<string> queue = FindLogFiles._processingQueue;

Having a List<string> named queue makes me think you really want a Queue<string>. Especially since you're later calling FindLogFiles._processingQueue.RemoveAt(0); which will take time proportional to the number of elements in the list, while for a queue it would take constant time.


The _ on _processingQueue makes me think you're directly accessing a field of another class. This is generally a bad idea as it tightly couples the classes.


Sometimes queue is used, other times FindLogFiles._processingQueue. Since these are the same object, why the inconsistency?


if (text.Length <= 0)

A string will not have a negative length.


using (StreamReader sr = new StreamReader(queue[0])) { text = sr.ReadToEnd(); }

This can be replaced with a call to File.ReadAllText.


PreSendingLogs("" + incomingString);

You can remove the "" +.


timesSendToServer >= 25
textByteLength >= 47500 && textByteLength < 500000
text.Substring(e * 225000, 225000)

Where are these numbers coming from? Introduce named constants for them, preferably with a comment explaining their derivation.

Working backwards, I guess you've come to 225,000 from 225,000 * 2 bytes = 450KB, a bit under the 500KB limit you mentioned. If you encode the strings as UTF-8 instead of UTF-16, you will probably get a lot more bang for your buck. On that note, consider zipping the log files before sending them.


In ProccesBigFiles (should be ProcessBigFiles), text can be moved inside the loop.


// there is no queue and nothing incoming, what are we doing here?

Good question.


if (text.Length > 0)
{
    ...
}
else
{
    ReadyToPostFiles.RemoveAt(0);
    throw new NullReferenceException("No text found in document, removing from queue: " + ReadyToPostFiles[0]);
}

That's not an appropriate use of NullReferenceException, "The exception that is thrown when there is an attempt to dereference a null object reference".


private static List<string> ToBigFiles = new List<string>();
private static List<string> ReadyToPostFiles = new List<string>();

These fields can be made readonly (you will need to change ToBigFiles = new List<string>(); to ToBigFiles.Clear();).


catch (Exception e)

You should really only catch exceptions that you know how to deal with.


GC.Collect();

This at least deserves a comment, and probably should not be called at all.


var text = "";

// If limit has been reached, stop sending for xxx duration
if (timesSendToServer >= 25)
{
    ...
    return;
}

if (queue.Count > 0)
{
    ...
}
else if (incomingString.Length > 0)
{
    // If the file size is big enough after the fusion. send it to server
    if (SendToServer(text, false))

In the last line, if (SendToServer(text, false)), text will always be the empty string, which is probably not what you want, judging by the comment. One problem with big methods like this is that these sort of things are hard to spot.


The logic of PreSendingLogs is very hard to follow. Try to split it up into smaller, more understandable methods.

\$\endgroup\$
  • 1
    \$\begingroup\$ Pretty nice review. \$\endgroup\$ – Heslacher Feb 14 '15 at 6:31
  • \$\begingroup\$ 'Sometimes queue is used, other times FindLogFiles._processingQueue. Since these are the same object, why the inconsistency?' As finding the log files is not to say the fastest method, the queue might still update, and the locally used array gets overwritten again. Guess I should first finish the fetching, and then start the prepping. Lots of use full info in the review. Thanks allot. \$\endgroup\$ – MX D Feb 16 '15 at 11:11

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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