HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMinor

Multi threading with webrequests/responses

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
multiwithresponsesthreadingwebrequests

Problem

I have been working with multi threading and would like some feedback on the code, i generally suck at class design/OO which i am trying to improve. Im sure there is a better way of making it neater and maximizing readability.

```
using System;
using System.Collections.Generic;
using System.IO;
using System.Linq;
using System.Net;
using System.Text;
using System.Threading;
using System.Threading.Tasks;
using System.Xml;

namespace XMLSender
{
class Program
{
private static string serverUrl;
static void Main(string[] args)
{
Console.WriteLine("Please enter the URL to send the XML File");
serverUrl = Console.ReadLine();

List threads = new List();
List names = new List();
string fileName = "";

do
{
Console.WriteLine("Please enter the XML File you Wish to send, To start simulation type 'start'");
fileName = Console.ReadLine();
if (fileName != "start")
{
Thread t = new Thread(new ParameterizedThreadStart(send));
threads.Add(t);
names.Add(fileName);
}
}
while (fileName != "start");

for (int i = 0; i < threads.Count; i++)
{
var t = threads[i];
var name = names[i];
t.Start(name);
}

foreach (Thread t in threads)
{
t.Join();
}

}
static private void send(object data)
{
try
{
//Set the connection limit of HTTP
System.Net.ServicePointManager.DefaultConnectionLimit = 10000;

//ServicePointManager.ServerCertificateValidationCallback = delegate { return true; };
HttpWebRequest request = (HttpWebRequest)WebRequest.Create(http://www.contoso.com/

Solution

You say you suck at class design but there's only one class in your code and everything is very "linear". I'm no expert anyway, but I'll try to help.

First of all you should separate the Main() function from the code you're executing (unlesss the code is short), this will improve readbility at it will be easier to update the code. To do this you can create a class where the main work will happen.

On the Main() function:

serverUrl = Console.ReadLine();


If you have many variables it might be better to declare this at the start of the function, but it isn't always necessary. In my opinion on the top you should put the variables that will be used many times or that are important.

while (fileName != "start");


I would do this with an if but this works fine. What I would also do is to save all the inputs in a list, either a List<> or a string[].
Also you're doing the "start" checking twice, one on the loop, and one on the code inside the loop.

It's simpler like this:

List fileNames = new List();
while (currentLine != "start")
    fileNames.Add(currentLine);


After this you iterate thorugh the items 2 times, I will go on that later.

Now we can create a class that does the "heavy work". Let's ask ourselves: what do we want this class to do? And what does it need to do that?

Well, now you have the class functions and its params.
We want the class to send data on, it needs the "XML file path", and we want to do it on a thread.

class TestClass
{
    private string xmlPath;
    public TestClass(string _xmlPath)
    {
        xmlPath = _xmlPath;
    }
    public void SendDataAsync(object data)
    {

    }
    public void SendData(object data)
    {

    }
}


We create a local class variable that's asked when we create the class and that will be accesed later: xmlPath.

Sometimes, using Threads it's nasty and can cause problems and performance issues (if using many of them, so I like using Tasks. This is what will be happening on the SendDataAsyncFunction:

public void SendDataAsync(object data)
        => Task.Run(() => SendData(data));


If we only have one line of code using => makes it simpler than using brackets.

Then on the SendData function:

On the first line we see a try block, then is it really necessary? What could throw an exception?
Things that could throw and exception are File.ReadAllBytes , request.Write and the response, in this case it's ok to have a try block on the whole function, but normally you shouldn't do that.

System.Net.ServicePointManager.DefaultConnectionLimit = 10000;


Is it necessary to do this every time? Why not take this to the main function? Since it's only executed once.

Then the bytes are read, to reduce code lines we can do this:

byte[] bytes = File.ReadAllBytes(xmlPath);


Next the HttpResquest is created, if we take this to another function (ideally each function should do only 1 thing), the code will be easier to read and if something has to be change less problems will occur.

private HttpWebRequest CreateRequest(int length)
    {
        WebRequest request = WebRequest.Create(ATMSimulator.Properties.Settings.Default.KTHBaseUri);
        request.ContentType = "text/xml; encoding='utf-8'";
        request.ContentLength = length;
        request.Method = "POST";
        return (HttpWebRequest)request;
    }


We take the cast to the end of the function, since it's not necessary to do it earlier.

Then we want to write the request. If you use using the stream will be closed itelf. We can change the code to this:

using (Stream requestStream = request.GetRequestStream())            
            requestStream.Write(bytes, 0, bytes.Length);


Then the function receives the response, we can take that to a different function too, like this:

private string ReceiveResponse(HttpWebRequest request)
    {            
        using (HttpWebResponse response = (HttpWebResponse)request.GetResponse())                          
            return new StreamReader(response.GetResponseStream()).ReadToEnd();             
    }


In this function we send Data and then we get the response, so the mame must be changed to something like SendDataAndGetResponse.

Now we go back to the top, and we see that we change some things that we had written before, like instead of using a string[] we can use a List of the class itelf.(I'm tired from explaining everything, so I just post the final code here):

```
using System;
using System.Collections.Generic;
using System.IO;
using System.Net;
using System.Threading.Tasks;

namespace test
{
internal class Program
{
private static void Main(string[] args)
{
ServicePointManager.DefaultConnectionLimit = 10000;
string serverURL;
List fileNames = new List();

Console.WriteLine("Please enter the URL to send the XML File");
serverURL = C

Code Snippets

serverUrl = Console.ReadLine();
while (fileName != "start");
List<string> fileNames = new List<string>();
while (currentLine != "start")
    fileNames.Add(currentLine);
class TestClass
{
    private string xmlPath;
    public TestClass(string _xmlPath)
    {
        xmlPath = _xmlPath;
    }
    public void SendDataAsync(object data)
    {

    }
    public void SendData(object data)
    {

    }
}
public void SendDataAsync(object data)
        => Task.Run(() => SendData(data));

Context

StackExchange Code Review Q#137826, answer score: 7

Revisions (0)

No revisions yet.