Download function

Question

I wrote a download function and I wanted to know if my code clean, readable and maintainable, or if I could make things easier. Please tell me if this function could be used in the real world.

public static void Download(Uri _downloadUri, string _path, string _passedSong, Song _currentObj) {
 if (_downloadUri != null) {
 if (!string.IsNullOrEmpty(_path) && !string.IsNullOrEmpty(_passedSong)) {
 try {
 var downloadedData = new byte[0];
 var webReq = WebRequest.Create(_downloadUri);
 using (var webResponse = webReq.GetResponse()) {
 using (var dataStream = webResponse.GetResponseStream()) {
 //Download chuncks
 byte[] dataBuffer = new byte[1024];
 //Get size
 int dataLength = (int) webResponse.ContentLength;
 ByteArgs byteArgs = new ByteArgs();
 byteArgs.downloaded = 0;
 byteArgs.total = dataLength;
 if (_bytesDownloaded != null) {
 _bytesDownloaded(byteArgs, _currentObj);
 }
 //Download
 using (var memoryStream = new MemoryStream()) {
 while (true) {
 int bytesFromStream = dataStream.Read(dataBuffer, 0, dataBuffer.Length);
 if (bytesFromStream == 0) {
 byteArgs.downloaded = dataLength;
 byteArgs.total = dataLength;
 if (_bytesDownloaded != null) {
 _bytesDownloaded(byteArgs, _currentObj);
 }
 //Download complete
 break;
 }
 else {
 //Write the downloaded data
 memoryStream.Write(dataBuffer, 0, bytesFromStream);
 byteArgs.downloaded = bytesFromStream;
 byteArgs.total = dataLength;
// for download progressbar
 if (_bytesDownloaded != null) {
 _bytesDownloaded(byteArgs, _currentObj);
 }
 }
 }
 //Convert to byte array
 downloadedData = memoryStream.ToArray();
 //Write bytes to the specified file
 if (Directory.Exists(Settings.Default.downloadDirectory)) {
 using (var newFile = new FileStream(_path, FileMode.Create)) {
 newFile.Write(downloadedData, 0, downloadedData.Length);
 }
 }
 }
 }
 }
 }
 catch(Exception ex) {
 ExceptionLog.LogException(ex, "Download()", "in GeneralSettings.cs");
 }
 }
 }
 }

• user3791372: What's wrong with WebClient?

Answer 1

This isn't a comprehensive answer by any means but it's the stuff that immediately stands out to me.

public static void Download(Uri _downloadUri, string _path, string _passedSong, Song _currentObj)

• Don't prefix parameter names with an underscore.

• _currentObj is not a good name for a Song object. Why not name it song?

• It's not clear what the point of _passedSong is. You check if it's null or empty, but then never use it in the method. Also - assuming it's the name of the song - I'd expect that to be a property of the Song object, in which case you wouldn't need to pass it separately.

var downloadedData = new byte[0]; // line 9
...
downloadedData = memoryStream.ToArray(); // line 67

Why declare downloadedData so far before you actually use it? Keep variables localised to their use. (Mind, this wouldn't be an issue if your method was smaller - which it should be.)

Another issue is you're creating a new byte array and assigning it to downloadedData, but then you immediately overwrite it with downloadedData = memoryStream.ToArray(), so there's no point creating a new byte array in the first place. Just do this: var downloadedData = memoryStream.ToArray(); instead.

//Download chuncks
byte[] dataBuffer = new byte[1024];

If "Download" is a verb then the comment is misleading; if it's an adjective then you should rename dataBuffer to something describing its intent/purpose, given you feel the need to have a comment to clarify it.

byteArgs.downloaded = 0;
byteArgs.total = dataLength;
if (_bytesDownloaded != null) {
 _bytesDownloaded(byteArgs, _currentObj);
}

There are three places where the above code exists with slight variations in each. My issue with it is that byteArgs is never used unless _bytesDownloaded != null, but the work of assigning values to byteArgs is done outside that check. Sure in real life it doesn't matter because what it does is negligible, but as a general principle it doesn't sit right with me.

Furthermore I would probably extract that bit of code to a method and call the method in three places, passing in the values as args. That would in fact be more inefficient than what you're currently doing, though it would be equally negligible in this specific situation.

Answer 2

Let's talk about early returns and catching input validation issues for a moment.

Generally, we try to return as early as possible, and throw any necessary exceptions as quickly as possible. This means we use what are called "guard clauses" to validate our input.

if (_downloadUri != null) {

Instead of wrapping all your logic in this block, let's return early instead.

if (_downloadUri == null)
{
 return;
}

Now, we can remove one of the levels of indentation of the rest of your code. (This is a good thing.)

We'll do the same with:

if (!string.IsNullOrEmpty(_path) && !string.IsNullOrEmpty(_passedSong)) {

But, we'll break it up.

if (string.IsNullOrEmpty(_path))
{
 return;
}
if (string.IsNullOrEmpty(_passedSong))
{
 return;
}

But, we're allowing some invalid data to be passed. It may not be plainly obvious, but string.IsNullOrEmpty still allows some clearly invalid paths. (All whitespace, in fact.)

So, we'll replace that with string.IsNullOrWhiteSpace. This will mean any of the valid paths are invalid: a. null paths, b. empty paths, c. whitespace paths.

if (string.IsNullOrWhiteSpace(_path))
{
 return;
}
if (string.IsNullOrWhiteSpace(_passedSong))
{
 return;
}

But, we're still doing something wrong. We're just returning nothing and not doing anything to inform the user why we returned immediately.

Let's throw an ArgumentException instead of returning. This means the caller can wrap the whole thing in a try/catch block and handle failures as they like.

if (_downloadUri == null)
{
 throw new ArgumentException($"The value passed for {nameof(_dow nloadUri)} must not be null.");
}

This also throws a little C#6.0 in there: the string interpolation, the nameof expression. We can also do the following for

if (_downloadUri == null)
{
 throw new ArgumentException(string.Format("The value passed for {0} must not be null.", "_downloadUri"));
}

The same can be done for the other two issues.

---

Usually in C# we try to place braces on their own lines. I.e.:

if (_downloadUri == null) {
 throw new ArgumentException($"The value passed for {nameof(_downloadUri)} must not be null.");
}

Is usually written as:

if (_downloadUri == null)
{
 throw new ArgumentException($"The value passed for {nameof(_downloadUri)} must not be null.");
}

---

I see you've included braces everywhere, which is very good. The only recommendation I would make here (which removes arrow-code in Visual Studio) is to omit braces around using directives that are directly proceeded by other using directives.

using (var webResponse = webReq.GetResponse()) {
 using (var dataStream = webResponse.GetResponseStream()) {
 /* ... */
 }
}

Can be less-indented as:

using (var webResponse = webReq.GetResponse())
using (var dataStream = webResponse.GetResponseStream())
{
 /* ... */
}

This makes it clear where the using statements belong.

---

I assume _bytesDownloaded is an event, so you should structure your code as such. Create a method for it, and call the event in that method. (This means you don't need to null-check everywhere you call the event as you are right now.)

private static void OnBytesDownloaded(ByteArgs byteArgs, Song song)
{
 var handler = _bytesDownloaded;
 handler?.Invoke(args, song);
}

If you don't have C#6.0:

private static void OnBytesDownloaded(ByteArgs byteArgs, Song song)
{
 var handler = _bytesDownloaded;
 if (handler != null)
 {
 handler(byteArgs, song);
 }
}

This is the thread-safe and concise way to do event throwing.

---

A lot of your naming could use some work. Public, protected and internal properties/events/methods should be PascalCase. Private fields/properties/events/methods, parameters, and local variables should be camelCase.

Some examples of violating this:

ByteArgs.downloaded -> ByteArgs.Downloaded
ByteArgs.total -> ByteArgs.Total
Settings.Default.downloadDirectory -> Settings.Default.DownloadDirectory

---

Many of your comments are superfluous. You should only comment things that aren't obvious by the code (and if they aren't obvious, try to make them obvious.)

---

You should consider a bufferSize parameter (or even setting value) so that 1024 isn't a "magic number." As it stands, if the user wants to use a larger or smaller buffer, it's impossible without modifying the actual code.

---

You can remove the webReq variable entirely and replace using (var webResponse = webReq.GetResponse()) with:

using (var webResponse = WebRequest.Create(downloadUri).GetResponse())

---

A couple of repeat critiques:

• As mentioned previously, do not include underscores (_) in parameter or local variable names. We reserve this idiom for use in private field names.
• Declare variables as close to their use as possible. This helps make their use-case more clear and keeps clutter to a minimum. (Why are there 150 variables declared at the top of my C kernel file?)
• Catch errors before they can cause bigger issues. If a directory doesn't exist, check for it before any other logic is performed. (Add a guard clause like we mentioned earlier.)
• Do NOT catch Exception unless you really, really, really mean to. It should only be caught if you are logging it. Otherwise, catch whatever appropriate exception you need to. I would also recommend throwing them after catching them to make sure the caller has a chance to handle what exceptions may occur. (Simply add a throw; to the catch block at the end of it.)

---

After making all these edits, we have:

public static void Download(Uri downloadUri, string path, string passedSong, Song song)
{
 if (downloadUri == null)
 {
 throw new ArgumentException($"The value passed for {nameof(downloadUri)} must not be null.");
 }
 if (string.IsNullOrWhiteSpace(path))
 {
 throw new ArgumentException($"The value passed for {nameof(path)} must not be a null, empty or whitespace string.");
 }
 if (string.IsNullOrWhiteSpace(passedSong))
 {
 throw new ArgumentException($"The value passed for {nameof(passedSong)} must not be a null, empty or whitespace string.");
 }
 if (!Directory.Exists(Settings.Default.DownloadDirectory))
 {
 throw new ArgumentException($"The directory at {Settings.Default.DownloadDirectory} does not exist.");
 }
 try
 {
 using (var webResponse = WebRequest.Create(downloadUri).GetResponse())
 using (var dataStream = webResponse.GetResponseStream())
 {
 //Download chuncks
 byte[] dataBuffer = new byte[1024];
 //Get size
 int dataLength = (int)webResponse.ContentLength;
 ByteArgs byteArgs = new ByteArgs();
 byteArgs.Downloaded = 0;
 byteArgs.Total = dataLength;
 OnBytesDownloaded(byteArgs, song);
 //Download
 using (var memoryStream = new MemoryStream())
 {
 while (true)
 {
 int bytesFromStream = dataStream.Read(dataBuffer, 0, dataBuffer.Length);
 if (bytesFromStream == 0)
 {
 byteArgs.Downloaded = dataLength;
 byteArgs.Total = dataLength;
 OnBytesDownloaded(byteArgs, song);
 //Download complete
 break;
 }
 else
 {
 //Write the downloaded data
 memoryStream.Write(dataBuffer, 0, bytesFromStream);
 byteArgs.Downloaded = bytesFromStream;
 byteArgs.Total = dataLength;
 // for download progressbar
 OnBytesDownloaded(byteArgs, song);
 }
 }
 //Convert to byte array
 var downloadedData = memoryStream.ToArray();
 //Write bytes to the specified file
 using (var newFile = new FileStream(path, FileMode.Create))
 {
 newFile.Write(downloadedData, 0, downloadedData.Length);
 }
 }
 }
 }
 catch (Exception ex)
 {
 ExceptionLog.LogException(ex, nameof(Download), "in GeneralSettings.cs");
 // Ensures that the caller has a chance to recieve and handle the exception.
 throw;
 }
}

---

Overall this is quite good, the logic is sound and I don't see any obvious errors that would make it painful to maintain. Hopefully this advice is helpful for this project and future projects as well.

• Denis: Nice answer EBrown. Wouldn't it be better if we put the 4 ifs into 1 method ?
• Der Kommissar: @denis Personally, I would leave them as guard clauses in the method they are guarding. There's no need to extract them when they are supposed to be guarding that specific method.
• iNCEPTiON_: Thanks for your awesome answer EBrown !! Now the Code is so much cleaner and understandable, although i diasagree with you on the braces ive learned it so and i like them behind the statement .

Answer 3

this is not the definitive answer but reading your code, I may suggest the following :

if (Directory.Exists(Settings.Default.downloadDirectory))

It means we are testing the directory existence after downloading and before saving, a waste of time if the directory doesn't exist it seems

downloadedData = memoryStream.ToArray();

May be Something like memoryStream.CopyTo(newFile) will remove the downloadedData byte array

byteArgs.total = dataLength;

It appears 3 times, and dataLength will not vary after setting up initial value

catch(Exception ex) {

As most of times, catching a general Exception instead of a specific one means a bad pattern, I suggest to catch only specific exception, may be IOException. Althought you are logging the exception, may be rethrow them AFTER the logging.

Hope this helps.