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.
