Problem
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();
}
Solution
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.