3
\$\begingroup\$

I have just somewhat completed a program, and was wondering what I could do to improve readability, and design as some functions seems to be rather large and an eye-sore.

using System;
using System.Collections.Generic;
using System.Diagnostics;
using System.IO;
using System.Linq;
using System.Net;
using System.Text.RegularExpressions;
using System.Threading.Tasks;
using System.Windows;
using System.Windows.Forms;
using System.Windows.Input;
using WoWLauncher.Properties;
using MessageBox = System.Windows.MessageBox;

namespace WoWLauncher
{
    /// <summary>
    /// Interaction logic for MainWindow.xaml
    /// </summary>
    public partial class MainWindow
    {

        public MainWindow()
        {
            InitializeComponent();
        }

        private void MainWindow_OnMouseDown(object sender, MouseButtonEventArgs e)
        {
            if (e.ChangedButton == MouseButton.Left)
                DragMove();
        }

        private void Minimize_OnMouseLeftButtonDown(object sender, MouseButtonEventArgs e)
        {
            WindowState = WindowState.Minimized;
        }

        private void Exit_OnMouseLeftButtonUp(object sender, MouseButtonEventArgs e)
        {
            Close();
        }

        private async void MainWindow_OnLoaded(object sender, RoutedEventArgs e)
        {
            IProgress<int> progressPercent = new Progress<int>(percent =>
            {
                if (progressBar.IsIndeterminate)
                    progressBar.IsIndeterminate = false;
                progressBar.Value = percent;
            });
            IProgress<string> progressText = new Progress<string>(text => updateLabel.Content = text);
            windowTitle.Content = Settings.Default.ServerTitle;

            news.Text = await GetNews(progressText);
            await GetWoWDirectory();
            await CheckUpdates(progressPercent, progressText);
        }

        private static async Task<string> GetNews(IProgress<string> updateText)
        {
            try
            {
                updateText.Report("Fetching news.");
                var webClient = new WebClient();
                return await webClient.DownloadStringTaskAsync(Settings.Default.NewsUrl);
            }
            catch (WebException)
            {
                return "Failed to fetch news";
            }
        }

        private static async Task GetWoWDirectory()
        {   
            var directory = await Task.Factory.StartNew(() => FindWoWDirectory());
            if (directory == null)
            {
                MessageBox.Show("Cannot find WoW Directory. Please choose.");
                var dialog = new FolderBrowserDialog();
                if (dialog.ShowDialog() == System.Windows.Forms.DialogResult.OK)
                {
                    directory = dialog.SelectedPath;
                }
            }
            Settings.Default.WoWFolder = directory;
            Settings.Default.Save();
        }

        private void Register_Click(object sender, RoutedEventArgs routedEventArgs)
        {
            Process.Start(Settings.Default.RegisterUrl);
        }

        private void SetRealmlist_Click(object sender, RoutedEventArgs routedEventArgs)
        {
            SetRealmlist();
        }

        private void Launch_Click(object sender, RoutedEventArgs routedEventArgs)
        {
            CheckRealmlist();
            Process.Start(Path.Combine(FindWoWDirectory(), "Wow.exe"));
        }

        private async Task CheckUpdates(IProgress<int> progressPercentage, IProgress<string> progressText)
        {
            progressText.Report("Checking for updates...");
            var data = Path.Combine(FindWoWDirectory(), "Data");
            var webClient = new WebClient();
            try
            {
                var updates = await webClient.DownloadStringTaskAsync(Settings.Default.UpdateUrl);
                if (string.IsNullOrEmpty(updates) || !updates.StartsWith("http"))
                {
                    progressPercentage.Report(100);
                    progressText.Report("No updates found");
                    launch.Content = "Launch";
                    launch.IsEnabled = true;
                }
                else
                {
                    var list = new List<string>(Regex.Split(updates, Environment.NewLine));
                    foreach (var url in list)
                    {
                        var fileName = url.Split('/').Last();
                        var fileSize = CheckSize(url);
                        var localFileName = Path.Combine(data, fileName);
                        var fileInfo = new FileInfo(localFileName);
                        if (fileInfo.Exists)
                        {
                            if (fileInfo.Length == fileSize)
                                continue;
                            fileInfo.MoveTo(fileInfo.FullName + ".bak");
                        }

                        progressText.Report($"Downloading file {fileName}");
                        await DownloadFile(url, fileInfo.FullName, progressPercentage);
                    }
                    progressPercentage.Report(100);
                    progressText.Report("Client up to date");
                    launch.Content = "Launch";
                    launch.IsEnabled = true;
                }
            }
            catch (WebException ex)
            {
                if (ex.Status == WebExceptionStatus.ReceiveFailure)
                {
                    progressText.Report("Failed to check for updates");
                    progressPercentage.Report(100);
                    launch.Content = "Launch";
                    launch.IsEnabled = true;
                }
            }
        }

        public static async Task DownloadFile(string url, string path, IProgress<int> progress = null)
        {
            using (var client = new WebClient())
            {
                client.DownloadProgressChanged += (s, e) =>
                {
                    progress?.Report(e.ProgressPercentage);
                };

                await client.DownloadFileTaskAsync(url, path);
            }
        }

        private static long CheckSize(string url)
        {
            var req = WebRequest.CreateHttp(url);
            req.Method = "HEAD";
            var resp = (HttpWebResponse)(req.GetResponse());
            return resp.ContentLength;
        }

        private static string FindWoWDirectory()
        {
            if (Settings.Default.WoWFolder != string.Empty)
                return Settings.Default.WoWFolder;

            var possibleDirectories = new[]
            {
                @"{0}World of Warcraft\",
                @"{0}Program Files\World of Warcraft\",
                @"{0}Program Files (x86)\World of Warcraft\"
            };
            var drives = DriveInfo.GetDrives();
            foreach (var wowPath in drives
                .SelectMany(drive => possibleDirectories.Select(directory => string.Format(directory, drive.Name))
                .Where(Directory.Exists)
                .Select(CheckVersion)))
            {
                return wowPath;
            }

            return drives.Select(drive => CheckVersion(drive.Name)).FirstOrDefault(wowPath => wowPath != null);
        }

        private static string CheckVersion(string wowPath)
        {
            var wowExes = Directory.GetFiles(wowPath, "Wow.exe", SearchOption.AllDirectories);

            return (from wowExe in wowExes
                    let versionInfo = FileVersionInfo.GetVersionInfo(wowExe)
                    where versionInfo.FileVersion.Replace(", ", ".") == Settings.Default.Patch
                    select new FileInfo(wowExe).DirectoryName).FirstOrDefault();
        }

        private static string GetLocaleFolder()
        {
            var wowDirectory = FindWoWDirectory();
            var realmlists = new[]
            {
                "GB",
                "US"
            };

            return (from realmlist in realmlists
                    select Path.Combine(wowDirectory, $@"Data\en{realmlist}\") 
                    into folder where Directory.Exists(folder)
                    select folder).FirstOrDefault();
        }

        private static string GetRealmlist()
        {
            return Path.Combine(GetLocaleFolder(), "realmlist.wtf");
        }

        private static void SetRealmlist()
        {
            var file = GetRealmlist();
            if (File.Exists(file))
                File.Copy(file, file + ".bak", true);
            File.WriteAllText(file, Settings.Default.Realmlist);
        }

        private static void CheckRealmlist()
        {
            var file = GetRealmlist();
            var contents = File.ReadAllText(file);
            if (contents != Settings.Default.Realmlist)
                SetRealmlist();
        }
    }
}
\$\endgroup\$
  • \$\begingroup\$ Are you sure this code works? I get a number of errors when I put it into Visual Studio. \$\endgroup\$ – ArtOfCode Aug 22 '15 at 20:46
  • \$\begingroup\$ @ArtOfCode I am sure that it works. The Settings file is not included and you need to be on .NET Framework 4.5 / 4.6 (not sure which). \$\endgroup\$ – Tristan McPherson Aug 22 '15 at 22:26
3
\$\begingroup\$
if (e.ChangedButton == MouseButton.Left)
     DragMove();

[ln 30-31] Style choice: I recommend using {} around conditionals, even if they're only one line - because if you update them and make them more than one line and forget to add the brackets... exceptions.

Also, your indents are off here. Indent the first line of the snippet above by one more space.


private static async Task<string> GetNews(IProgress<string> updateText)
{
    try
    {
        updateText.Report("Fetching news.");
        var webClient = new WebClient();
        return await webClient.DownloadStringTaskAsync(Settings.Default.NewsUrl);
    }
    catch (WebException)
    {
        return "Failed to fetch news";
    }
}

[ln 60] Single responsibility: if GetNews's purpose is to get news, it shouldn't also be dealing with notifying the user if there is none.

Make this method return null if there is no news to be had because of the WebException, and do the check in the UI code [ln 55]:

var news = await GetNews(progressText);
news.Text = news == null ? "Failed to fetch news" : news;

if (dialog.ShowDialog() == System.Windows.Forms.DialogResult.OK)

[ln 81] You've already included System.Windows.Forms - no need to have the entire name here.


Realmlist

[multiple] As mentioned in other answers: rename Realmlist to RealmList or Realms.


private static string FindWoWDirectory()
{
    if (Settings.Default.WoWFolder != string.Empty)

[ln 179] Folder !== Directory. Be consistent throughout: if this is a setting that a user needs to understand (hence why you used Folder instead of Directory), convert it to Directory code-side.


var realmlists = new[]
{
    "GB",
    "US"
};

[ln 215] realmlists? Seems like one list to me. Rename it; again, consider the point above that it should be realms.


More in general, this class is doing too much. This is the code-behind for MainWindow, it seems, which should be an effector class (handling what the user sees). Make another class containing the helper methods like GetNews, GetWoWDirectory, etc., for the business layer, and refer to it from MainWindow.

\$\endgroup\$
  • \$\begingroup\$ Very nice. The portion on GetNews returning null is a much better idea than what I had. The realmlist is a property of the WoW game client that's the location of the realmlist. \$\endgroup\$ – Tristan McPherson Aug 22 '15 at 22:24
5
\$\begingroup\$

It seems a bit odd to me that you suddenly use MessageBox.Show("Cannot find WoW Directory. Please choose.");, whereas in other places you update updateLabel.


Realmlist is a compound word, so it should be RealmList. Actually, scrap that, it should be Realms because you should avoid using the name of the type in the name of the property or field.


I'm not a big fan of some of your names. For instance: progressText doesn't really convey what it is. (Also please be consistent: in one place you name this updateText.)


Watch out for inconsistent naming: FindWoWDirectory vs WoWFolder, for instance.


You have a using System.Windows.Forms; at the top, yet you still specify the full namespace in the code: System.Windows.Forms.DialogResult.OK.


Even though this isn't that long (250 lines), I still feel that there's a lot of code in this class that belongs in a dedicated class, e.g. the update check, retrieval of the WoW directory,...

Web Forms MVP is probably overkill for this kind of project (since I doubt there will be much functionality to be added in the future), but I'd still try to limit the code in this MainWindow class to UI-related code, and move all other code to smaller dedicated 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.