Download function

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);
}

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.

StackExchange Code Review Q#127714, answer score: 3