1
\$\begingroup\$

Is there a better way to calculate the download speed than what I'm currently doing now? Are there any suggestions on improving the updated code?

Pure version:

using System;
using System.Diagnostics;
using System.IO;
using System.Net;
using System.Threading;

namespace Downloader
{
    public interface ILogger
    {
        void LogMessage(string message, params Object[] args);
    }

    public class ConsoleLogger : ILogger
    {
        public void LogMessage(string message, params Object[] args)
        {
            Console.WriteLine(string.Format(message, args));
        }
    }

    public class Download
    {
        private ILogger _Logger;

        public CallingClass(ILogger logger)
        {
            _Logger = logger;
        }

        public event EventHandler<DownloadStatusChangedEventArgs> ResumablityChanged;
        public event EventHandler<DownloadProgressChangedEventArgs> ProgressChanged;
        public event EventHandler Completed;

        public bool stop = true; // by default stop is true
        public bool paused = false;
        SemaphoreSlim pauseLock = new SemaphoreSlim(1);

        string filename;

        public void DownloadFile(string DownloadLink, string Path)
        {
            filename = System.IO.Path.GetFileName(Path);

            stop = false; // always set this bool to false, everytime this method is called

            var fileInfo = new FileInfo(Path);
            long existingLength = 0;
            if (fileInfo.Exists)
                existingLength = fileInfo.Length;

            var request = (HttpWebRequest)HttpWebRequest.Create(DownloadLink);
            request.Proxy = null;
            request.AddRange(existingLength);

            try
            {
                using (var response = (HttpWebResponse)request.GetResponse())
                {
                    long fileSize = existingLength + response.ContentLength; //response.ContentLength gives me the size that is remaining to be downloaded
                    bool downloadResumable; // need it for not sending any progress

                    if ((int)response.StatusCode == 206) //same as: response.StatusCode == HttpStatusCode.PartialContent
                    {
                        logger.LogMessage("Resumable");
                        downloadResumable = true;
                    }
                    else // sometimes a server that supports partial content will lose its ability to send partial content(weird behavior) and thus the download will lose its resumability
                    {
                        logger.LogMessage("Not Resumable");
                        existingLength = 0;
                        downloadResumable = false;
                    }
                    OnResumabilityChanged(new DownloadStatusChangedEventArgs(downloadResumable));

                    using (var saveFileStream = fileInfo.Open(downloadResumable ? FileMode.Append : FileMode.Create, FileAccess.Write))
                    using (var stream = response.GetResponseStream())
                    {
                        byte[] downBuffer = new byte[4096];
                        int byteSize = 0;
                        long totalReceived = byteSize + existingLength;
                        var sw = Stopwatch.StartNew();
                        while (!stop && (byteSize = stream.Read(downBuffer, 0, downBuffer.Length)) > 0)
                        {
                            saveFileStream.Write(downBuffer, 0, byteSize);
                            totalReceived += byteSize;

                            float currentSpeed = totalReceived / (float)sw.Elapsed.TotalSeconds;
                            OnProgressChanged(new DownloadProgressChangedEventArgs(totalReceived, fileSize, (long)currentSpeed));

                            pauseLock.Wait();
                            pauseLock.Release();
                        }
                        sw.Stop();
                    }
                }
                if (!stop) OnCompleted(EventArgs.Empty);
            }
            catch (WebException e)
            {
                logger.LogMessage(e);
            }
        }

        public void pause()
        {
            if (!paused)
            {
                paused = true;
                // Note this cannot block for more than a moment
                // since the download thread doesn't keep the lock held
                pauseLock.Wait();
            }
        }

        public void unpause()
        {
            if (paused)
            {
                paused = false;
                pauseLock.Release();
            }
        }

        public void StopDownload()
        {
            stop = true;
            this.unpause();  // stop waiting on lock if needed
        }

        protected virtual void OnResumabilityChanged(DownloadStatusChangedEventArgs e)
        {
            var handler = ResumablityChanged;
            if (handler != null)
            {
                handler(this, e);
            }
        }

        protected virtual void OnProgressChanged(DownloadProgressChangedEventArgs e)
        {
            var handler = ProgressChanged;
            if (handler != null)
            {
                handler(this, e);
            }
        }

        protected virtual void OnCompleted(EventArgs e)
        {
            var handler = Completed;
            if (handler != null)
            {
                handler(this, e);
            }
        }
    }

    public class DownloadStatusChangedEventArgs : EventArgs
    {
        public DownloadStatusChangedEventArgs(bool canResume)
        {
            ResumeSupported = canResume;
        }
        public bool ResumeSupported { get; private set; }
    }

    public class DownloadProgressChangedEventArgs : EventArgs
    {
        public DownloadProgressChangedEventArgs(long totalReceived, long fileSize, long currentSpeed)
        {
            BytesReceived = totalReceived;
            TotalBytesToReceive = fileSize;
            CurrentSpeed = currentSpeed;
        }

        public long BytesReceived { get; private set; }
        public long TotalBytesToReceive { get; private set; }
        public float ProgressPercentage 
        { 
            get 
            { 
                return ((float)BytesReceived / (float)TotalBytesToReceive) * 100; 
            } 
        }
        public float CurrentSpeed { get; private set; } // in bytes
        public TimeSpan TimeLeft
        {
            get
            {
                var bytesRemainingtoBeReceived = TotalBytesToReceive - BytesReceived;
                return TimeSpan.FromSeconds(bytesRemainingtoBeReceived / CurrentSpeed);
            }
        }
    }
}

GUI version:

using System;
using System.Diagnostics;
using System.IO;
using System.Net;
using System.Threading;

namespace Downloader
{
    public interface ILogger
    {
        void LogMessage(string message, params Object[] args);
    }

    public class ConsoleLogger : ILogger
    {
        public void LogMessage(string message, params Object[] args)
        {
            Console.WriteLine(string.Format(message, args));
        }
    }

    public class Download
    {
        private ILogger _Logger;

        public CallingClass(ILogger logger)
        {
            _Logger = logger;
        }

        public event EventHandler<DownloadStatusChangedEventArgs> ResumablityChanged;
        public event EventHandler<DownloadProgressChangedEventArgs> ProgressChanged;
        public event EventHandler Completed;

        public bool stop = true; // by default stop is true
        public bool paused = false;
        SemaphoreSlim pauseLock = new SemaphoreSlim(1);

        string filename;

        public void DownloadFile(string DownloadLink, string Path)
        {
            filename = System.IO.Path.GetFileName(Path);

            stop = false; // always set this bool to false, everytime this method is called

            var fileInfo = new FileInfo(Path);
            long existingLength = 0;
            if (fileInfo.Exists)
                existingLength = fileInfo.Length;

            var request = (HttpWebRequest)HttpWebRequest.Create(DownloadLink);
            request.Proxy = null;
            request.AddRange(existingLength);

            try
            {
                using (var response = (HttpWebResponse)request.GetResponse())
                {
                    long fileSize = existingLength + response.ContentLength; //response.ContentLength gives me the size that is remaining to be downloaded
                    bool downloadResumable; // need it for not sending any progress

                    if ((int)response.StatusCode == 206) //same as: response.StatusCode == HttpStatusCode.PartialContent
                    {
                        logger.LogMessage("Resumable");
                        downloadResumable = true;
                    }
                    else // sometimes a server that supports partial content will lose its ability to send partial content(weird behavior) and thus the download will lose its resumability
                    {
                        logger.LogMessage("Not Resumable");
                        if (existingLength > 0)
                        {
                            if (ResumeUnsupportedWarning() == false) // warn and ask for confirmation to continue if the half downloaded file is unresumable
                            {
                                return;
                            }
                        }
                        existingLength = 0;
                        downloadResumable = false;
                    }
                    OnResumabilityChanged(new DownloadStatusChangedEventArgs(downloadResumable));

                    using (var saveFileStream = fileInfo.Open(downloadResumable ? FileMode.Append : FileMode.Create, FileAccess.Write))
                    using (var stream = response.GetResponseStream())
                    {
                        byte[] downBuffer = new byte[4096];
                        int byteSize = 0;
                        long totalReceived = byteSize + existingLength;
                        var sw = Stopwatch.StartNew();
                        while (!stop && (byteSize = stream.Read(downBuffer, 0, downBuffer.Length)) > 0)
                        {
                            saveFileStream.Write(downBuffer, 0, byteSize);
                            totalReceived += byteSize;

                            float currentSpeed = totalReceived / (float)sw.Elapsed.TotalSeconds;
                            OnProgressChanged(new DownloadProgressChangedEventArgs(totalReceived, fileSize, (long)currentSpeed));

                            pauseLock.Wait();
                            pauseLock.Release();
                        }
                        sw.Stop();
                    }
                }
                if (!stop) OnCompleted(EventArgs.Empty);
            }
            catch (WebException e)
            {
                System.Windows.MessageBox.Show(e.Message, filename);
            }
        }

        public void pause()
        {
            if (!paused)
            {
                paused = true;
                // Note this cannot block for more than a moment
                // since the download thread doesn't keep the lock held
                pauseLock.Wait();
            }
        }

        public void unpause()
        {
            if (paused)
            {
                paused = false;
                pauseLock.Release();
            }
        }

        public void StopDownload()
        {
            stop = true;
            this.unpause();  // stop waiting on lock if needed
        }

        public bool ResumeUnsupportedWarning()
        {
            var messageBoxResult = System.Windows.MessageBox.Show("When trying to resume the download , Mackerel got a response from the server that it doesn't support resuming the download. It's possible that it's a temporary error of the server, and you will be able to resume the file at a later time, but at this time Mackerel can download this file from the beginning.\n\nDo you want to download this file from the beginning?", filename, System.Windows.MessageBoxButton.YesNo);
            if (messageBoxResult == System.Windows.MessageBoxResult.Yes)
            {
                return true;
            }
            else
            {
                return false;
            }
        }

        protected virtual void OnResumabilityChanged(DownloadStatusChangedEventArgs e)
        {
            var handler = ResumablityChanged;
            if (handler != null)
            {
                handler(this, e);
            }
        }

        protected virtual void OnProgressChanged(DownloadProgressChangedEventArgs e)
        {
            var handler = ProgressChanged;
            if (handler != null)
            {
                handler(this, e);
            }
        }

        protected virtual void OnCompleted(EventArgs e)
        {
            var handler = Completed;
            if (handler != null)
            {
                handler(this, e);
            }
        }
    }

    public class DownloadStatusChangedEventArgs : EventArgs
    {
        public DownloadStatusChangedEventArgs(bool canResume)
        {
            ResumeSupported = canResume;
        }
        public bool ResumeSupported { get; private set; }
    }

    public class DownloadProgressChangedEventArgs : EventArgs
    {
        public DownloadProgressChangedEventArgs(long totalReceived, long fileSize, long currentSpeed)
        {
            BytesReceived = totalReceived;
            TotalBytesToReceive = fileSize;
            CurrentSpeed = currentSpeed;
        }

        public long BytesReceived { get; private set; }
        public long TotalBytesToReceive { get; private set; }
        public float ProgressPercentage 
        { 
            get 
            { 
                return ((float)BytesReceived / (float)TotalBytesToReceive) * 100; 
            } 
        }
        public float CurrentSpeed { get; private set; } // in bytes
        public TimeSpan TimeLeft
        {
            get
            {
                var bytesRemainingtoBeReceived = TotalBytesToReceive - BytesReceived;
                return TimeSpan.FromSeconds(bytesRemainingtoBeReceived / CurrentSpeed);
            }
        }
    }
}
\$\endgroup\$
3
\$\begingroup\$

The first bit of code that sprang to my eyes had been something which should just be deleted.

public bool stop = true; // by default stop is true  

What is this comment for ? It doesn't add any value to your code because it is clearly visible to the reader that the stop variable is initialized to true.

Such comments are only adding noise to the code and should be deleted.

But this isn't the only problem with it. Making a variable public violates encapsulation. Instead of using public variables you should use public properties. In this way you can control/validate any given value.


Based on the naming guidelines methods should be named using PascalCase casing, so pause() -> Pause() and the badly named unpause() method should be better named Resume().

Based on the same naming guidelines method parameters should be named using camelCase casing.


You should either use always braces {} or never for single line if statements (for and else the same). Right now you are inconsistently switching your styles. Sometimes you use braces, sometimes you don use them and put the single command on the same line like the if and sometimes you place the single command on the next line.

I would like to encourage you to always use them to make your code less error prone.


DownloadFile()

  • filename = System.IO.Path.GetFileName(Path);  
    

    this variable isn't used at all, so there is no need to keep it.

  • if ((int)response.StatusCode == 206) //same as: response.StatusCode == HttpStatusCode.PartialContent  
    

    instead of using a comment about what is meant, why don't you just use the enum ? It would be more obvious if you write

    if (response.StatusCode == HttpStatusCode.PartialContent)  
    

    no one would need to read the comment because it is clear what you meant.

    But this whole if..else statement shouldn't be there. You should add a private method LogResumable(bool downloadResumable) where you do the logging based on the method parameter. This reduces the code of the DownloadFile() method.

    By ectracting this to the said method the former if..else woulc look like so

    bool downloadResumable = response.StatusCode == HttpStatusCode.PartialContent;
    if (!downloadResumable) 
    {
        existingLength = 0;
    }
    LogResumable(downloadResumable);
    OnResumabilityChanged(new DownloadStatusChangedEventArgs(downloadResumable));
    

I don't understand why you have a version of this class which is only for a GUI. This isn't the way it should be. You should just use the pure version and adjust the DownloadStatusChangedEventArgs class to have a property for the existingLength property.

By changing the order of the changed if..else statements to

    bool downloadResumable = response.StatusCode == HttpStatusCode.PartialContent;
    OnResumabilityChanged(new DownloadStatusChangedEventArgs(downloadResumable));
    if (!downloadResumable) 
    {
        existingLength = 0;
    }
    LogResumable(downloadResumable);

you could use the pure version inside a gui and could use the event to decide if the download should be aborted. Sure this needs some more work, because you need to extend the Download class to react on abortation but that is always better than using 2 almost same classes.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.