Xamarin.Forms request / Failed request function

Question

I've wrote a little iPad app with Xamarin.Forms. What I'm trying to do here is send some support tickets via a request to my server. I'm also keeping the "Failed" tickets in a collection so that I can try to send them in an other thread at app start. Can I improve my code or is it hopeless?

   public void SendFailedTicketsTicketModul(ObservableCollection<TicketModel> queue) {
        try {

            if (!CheckConnection()) {
                return;
            }

            for (var i = queue.Count - 1; i >= 0; i--) {

                if (string.IsNullOrEmpty(queue[i].TicketId)) {

                    var requestResult = CreateRequest(queue[i]);
                    if (string.IsNullOrEmpty(requestResult)) {
                        continue;
                    }

                    var xmlDoc = new XmlDocument();
                    xmlDoc.LoadXml(requestResult);

                    var success = xmlDoc.GetElementsByTagName("status")[0].InnerText.Equals("Success");
                    if (!success) {
                        continue;
                    }

                    queue[i].TicketId = xmlDoc.GetElementsByTagName("workorderid")[0].InnerText;
                }

                if (queue[i].AttachmentCollection.Count > 0) {

                    for (var j = queue[i].AttachmentCollection.Count - 1; i >= 0; i--) {
                        var fileInfo = new FileInfo(queue[i].AttachmentCollection[j].FullPath);
                        if (!fileInfo.Exists) {
                            continue;
                        }

                        var attachmentResult = AddAttachmentRequest(queue[i].TicketId, fileInfo.Name,
                            fileInfo.FullName);

                        if (attachmentResult.Contains("Success")) {
                            queue[i].AttachmentCollection.RemoveAt(j);
                        }
                    }
                }

                if (!string.IsNullOrEmpty(queue[i].TicketId) && queue[i].AttachmentCollection.Count == 0) {
                    queue.RemoveAt(i);
                }
            }

        }
        catch (Exception ex) {
            Debug.WriteLine(ex.Message);
            Debug.WriteLine(ex.Source);
            Debug.WriteLine(ex.StackTrace);
        }
    }

    public XmlDocument FillXmlDoc(TicketModel ticketData) {
        try {
            var xmlData = Util.ReadPostDataXml();
            if (string.IsNullOrEmpty(xmlData)) {
                throw new ArgumentException($"{xmlData} is null or empty");
            }

            var postXml = new XmlDocument();
            postXml.LoadXml(xmlData);
            postXml.DocumentElement["requester"].InnerText = Util.Username;
            postXml.DocumentElement["subject"].InnerText = ticketData.Subject;
            postXml.DocumentElement["description"].InnerText = ticketData.Content;
            postXml.DocumentElement["group"].InnerText = ticketData.Group;

            var dateDiff = ticketData.DueDate.Subtract(DateTime.Today).Days;
            postXml.DocumentElement["priority"].InnerText = dateDiff > 30
                ? "30 Tage"
                : dateDiff.ToString("D2") + " Tage";

            if (dateDiff == 1) {
                postXml.DocumentElement["priority"].InnerText =
                    postXml.DocumentElement["priority"].InnerText.Remove(
                        postXml.DocumentElement["priority"].InnerText.Length - 1);
            }

            return postXml;
        }
        catch (Exception ex) {
            Debug.WriteLine(ex.Message + " FillXmlDoc");
            Debug.WriteLine(ex.StackTrace);
        }
        return null;
    }

    public string SendTicket(TicketModel ticketModel) {
        Debug.WriteLine("SendTicket() start");

        if (!CheckConnection()) return null;

        try {
            var requestResult = CreateRequest(ticketModel);

            var xmlDoc = new XmlDocument();
            xmlDoc.LoadXml(requestResult);

            var success = xmlDoc.GetElementsByTagName("status")[0].InnerText.Equals("Success");

            if (!success) {
                Debug.WriteLine("Request Failed: " + xmlDoc.OuterXml);
                return null;
            }

            ticketModel.TicketId = xmlDoc.GetElementsByTagName("workorderid")[0].InnerText;

            if (ticketModel.AttachmentCollection.Count > 0) {
                for (var j = ticketModel.AttachmentCollection.Count - 1; j >= 0; j--) {
                    var fileInfo = new FileInfo(ticketModel.AttachmentCollection[j].FullPath);
                    if (!fileInfo.Exists) {
                        Debug.WriteLine("Attachment not found");
                        ticketModel.AttachmentCollection.RemoveAt(j);
                        continue;
                    }

                    var attachmentResult = AddAttachmentRequest(ticketModel.TicketId, fileInfo.Name,
                        fileInfo.FullName);

                    if (attachmentResult.Contains("Success")) {
                        ticketModel.AttachmentCollection.RemoveAt(j);
                    }
                    else {
                        Debug.WriteLine("Add Attachment failed: " + attachmentResult);
                    }
                }
            }

            if (!string.IsNullOrEmpty(ticketModel.TicketId) && ticketModel.AttachmentCollection.Count == 0)
                return ticketModel.TicketId;

            Debug.WriteLine("SendTicket() end");
            return null;
        }
        catch (Exception ex) {
            Util.PrintException(ex);
            return null;
        }
    }

    public string CreateRequest(TicketModel ticket) {
        var requestXml = FillXmlDoc(ticket);
        if (requestXml == null) {
            return null;
        }

        try {
            var taskString = new TaskCompletionSource<string>();
            _requestDictionary = new Dictionary<string, string> {
                {"TECHNICIAN_KEY", "49626A79-A60C-4565-B125-56AFDC332FE7"},
                {"OPERATION_NAME", "ADD_REQUEST"},
                {"INPUT_DATA", requestXml.OuterXml}
            };

            var restRequest = new RestRequest(Method.POST);
            var restClient = new RestClient(new Uri("https://url.com")) {
                Authenticator = new NtlmAuthenticator(new NetworkCredential("test", "test", "test"))
            };

            foreach (var param in _requestDictionary) {
                restRequest.AddParameter(param.Key, param.Value);
            }

            restClient.ExecuteAsync(restRequest, response => {
                taskString.SetResult(response.Content);
            });

            return taskString.Task.Result;
        }
        catch (Exception ex) {
            Debug.WriteLine(ex.Message);
            Debug.WriteLine(ex.StackTrace);
        }
        return string.Empty;
    }

    public string AddAttachmentRequest(string ticketId, string name, string fullName) {
        try {
            var taskString = new TaskCompletionSource<string>();
            var restRequest = new RestRequest(Method.POST);
            var restClient = new RestClient(new Uri("https://url.com" + ticketId +
                                                    "/attachment" + "?OPERATION_NAME=ADD_ATTACHMENT&TECHNICIAN_KEY=49626A79-A60C-4565-B125-56AFDC332FE7")) {
                Authenticator = new NtlmAuthenticator(new NetworkCredential("test", "test", "test"))
            };

            restRequest.AddHeader("Content-Type", "multipart/form-data");
            restRequest.AddFile(name, fullName);
            restRequest.AlwaysMultipartFormData = true;

            restClient.ExecuteAsync(restRequest, response => {
                taskString.SetResult(response.Content);
            });

            return taskString.Task.Result;
        }
        catch (Exception ex) {
            Debug.WriteLine(ex.Message);
            Debug.WriteLine(ex.StackTrace);
        }
        return string.Empty;
    }

Adriano Repetti · Accepted Answer · 2016-09-14 21:34:25Z

Your code is little bit long then, even if usually I try to rewrite portions to explain what I suggest, in this case I will mostly highlight issues I see without much code.

In general the two biggest issues I see are:

Sub-optimal exception handling, first of all you catch a generic Exception. It may be caused by an exceptional condition (like running out-of-memory) which needs a proper action (terminating application immediately?); it may be caused by a programming error (like an invalid argument or an unchecked result) which needs to terminate application with proper logging; it may be caused by input errors or other run-time conditions which need to be logged (and usually the user to be notified, if a corrective action cannot be started).
Your methods are too long and your class does too many things. This issue has many drawbacks: code is impossible to reuse, harder to test in isolation and more complicate to understand (then error prone).

From top to bottom browsing your code.

SendFailedTicketsTicketModul may be a typo (isn't it Module?) Also it is a little bit obscure to me (but I don't know your domain the it may be clear with a little bit more knowledge.)

To have ObservableCollection<TicketModel> is a missed chance of abstraction. Today it's an observable collection, tomorrow? Also classes in System.Collections.ObjectModel are not usually intended to be used as-is but extended. Derive your own TicketModelCollection, at first it may even be empty but if you change implementation you will have less chances to also go and find every place you wrote ObservableCollection<TicketModel> (not to mention that you can add as much helper methods you need without the need of extension methods.)

queue argument name is exposing an implementation detail (it's intended to be a queue, or even better it's not but you should use it as it was.) If it's a queue then maybe you're using wrong data structure and anyway parameter should be named according to its content (ticketsToProcess, for example.)

try/catch block is too generic (as I said before) and too outer. If processing of a single ticket fails then maybe other tickets may be still processed (but you may keep a counter of failed operations and specifically handle some exceptions

CheckConnection() method may need a more descriptive name. Connection to what? What does it do? I mean something like IsDatabaseConnectionAvailable(), note that if it also performs connection then it has to be another name (with a verb...)

for loop in reverse order seems to imply that items are somehow ordered however I do not see any comment about that, did you do it only to have to use RemoveAt() without worrying? Implications (for the reader) of a reverse loop are not obvious, comment about this or refactor your code (you may use a different data structure, maybe decrement loop index RemoveAt(i--) or - what I usually do - make a copy of input array or keep a separate list of items to remove.

What String.IsNullOrEmpty(queue[i].TicketId) means? I don't know, please help me and use a local bool variable with a proper name (something like isTicketAlreadyProcessed) or - better - use the opportunity to introduce a function for that.

InnerText.Equals("Success") is performing a locale aware string comparison. I don't see the whole code but given that it comes from an XML file and it is compared with an hardcoded string (move it to a static readonly string field)...I'd guess you do not want blindly assume that string comparison rules for the local where your application will run match rules on your development machine. Add StringComparer.InvariantCulture (or Ordinal or actually anything else, someone else reading your code will not ask himself this same question - even if you want to apply current culture rules.)

Use this opportunity to move the whole code into a separate method, there I'd like to read something like this:

public void SendFailedTicketsTicketModule(TicketModelCollection ticketsToProcess) {
    if (IsDatabaseConnectionUnavailable())
        return;

    var ticketsToRemove = new List<TicketModel>();
    foreach (var ticketToProcess in ticketsToProcess) {
        if (TryToSendTicket(ticketToProcess))
            ticketsToRemove.Add(ticketToProcess);        
    }

    ticketsToProcess.RemoveRange(ticketsToRemove);
}

I love when I can understand what a method does reading just few lines of code (as a bonus it's also trivial to make it parallel, in case you care.) Note that most of your code is now inside TryToSendTicket() method (with a proper exception handling) but also that method has to be short and self-descriptive:

private bool TryToSendTicket(TicketModel ticketToSend) {
    try {
        if (IsTicketUnprocessed(ticketToSend))
            ExecuteTicketProcessingRequest(ticketToSend);

        AddAttachmentsToRequest(ticketToSend);
    }
    catch (WhateverException e) {
        HandleError(e);

        return false;
    }

    return true;
}

And so on, note that you may not need to always keep methods so short but it's the target you should set. Same applies to all the other methods then I won't repeat this concept.

Checking if (queue[i].AttachmentCollection.Count > 0) is useless because empty condition is already handled by for (or foreach) loop. You just make your code more indented.

String.Contains() has the same problem of String.Equals() but in this case it performs an ordinal case-sensitive comparison. Make yourself and your future readers aware of this (get rid of String.Contains() and eventually use String.IndexOf() which accepts a string comparer.

All your methods are public. Hmmmmm do you really need to call them all from outside the class? Make private methods...private.

I'm sure also the other methods may be improved but this answer is getting too long, as first step I'd really really split them in smaller focused methods (get rid of hardcoded strings). You will then start to see that they tend to group together, let them join each other into separate classes! Maybe a Request class with its derived class HttpRequest, now AddAttachmentsToRequest will call generic methods without knowledge of underlying protocol/implementation...and so on. Come back with your 2nd version of this code and post it as a new question!

Man im blown away by your Codereview ! Thanks for this it really opend my eyes . I dont have much to say except that i need to question myself about my coding style. — iNCEPTiON_
– iNCEPTiON_, Commented Sep 14, 2016 at 21:54
We all have to question ourselves about our coding style, after all that's why we're here!!! I hope you learned something new but, trust me, reviewing code both who asks and who answers are learning. — Adriano Repetti
– Adriano Repetti, Commented Sep 14, 2016 at 22:53

Stack Exchange Network

Xamarin.Forms request / Failed request function

1 Answer 1

You must log in to answer this question.

Hot Network Questions

Xamarin.Forms request / Failed request function

1 Answer 1

You must log in to answer this question.

Related

Hot Network Questions