2
\$\begingroup\$

I'm currently working in a project where the user needs to log in and after that I will check if the directory /cloud/user/Projects exists and if it doesn't, I will create them.

The code works, however I would like to know if there is any better way of handling exceptions, errors or if the internet is down while doing one of the steps.

I don't think that using try-catch blocks for each FTPCommand is the best way of doing it, but I cannot find information about it. (I'm using FluentFTP library).

Also, I'm using await and asynchronous methods because I do not want to freeze the UI (WPF).

  var ftp = StoryManager.MainWindow.FtpClient;

        ContentGrid.Opacity = 0;
        LoadingIndicator.Opacity = 1;

        if (UsernameTextBox.Text.Equals("") || PasswordTextBox.Password.ToString().Equals(""))
        {

            MessageBox.Show("Please, enter your credentials");
            ContentGrid.Opacity = 1;
            LoadingIndicator.Opacity = 0;
            return;
        }


        if ((Properties.Settings.Default.LoggedUser = await Rest.LoginAsync(UsernameTextBox.Text.ToLower(), PasswordTextBox.Password.ToString())) != null)
        {
            Properties.Settings.Default.KeepLogged = (bool)KeepLogged.IsChecked;
            Properties.Settings.Default.Save();
            Properties.Settings.Default.Upgrade();
            Properties.Settings.Default.Reload();
            string userDir = "/cloud/" + Properties.Settings.Default.LoggedUser.UserName;

            try
            {
                bool userDirExist = StoryManager.MainWindow.FtpClient.DirectoryExists(userDir);
                bool projectsDirExist = StoryManager.MainWindow.FtpClient.DirectoryExists(userDir + "/Projects");
                bool fileExist = StoryManager.MainWindow.FtpClient.FileExists("log.txt");

                if (userDirExist)
                {
                    try
                    {
                        await StoryManager.MainWindow.FtpClient.SetWorkingDirectoryAsync(userDir);
                    }
                    catch (Exception ex)
                    {
                        System.Windows.Forms.MessageBox.Show("An error ocurred please try again " + ex.Message);
                    }

                    if (!projectsDirExist)
                    {
                        try
                        {
                            await StoryManager.MainWindow.FtpClient.CreateDirectoryAsync(userDir + "/Projects");
                        }
                        catch (Exception ex)
                        {
                            System.Windows.Forms.MessageBox.Show("An error ocurred please try again " + ex.Message);
                        }
                    }
                }

                else
                {
                    try
                    {
                        await StoryManager.MainWindow.FtpClient.CreateDirectoryAsync(userDir);
                        await StoryManager.MainWindow.FtpClient.SetWorkingDirectoryAsync(userDir);
                        await StoryManager.MainWindow.FtpClient.CreateDirectoryAsync(userDir + "/Projects");
                    }
                    catch (Exception ex)
                    {
                        System.Windows.Forms.MessageBox.Show("An error ocurred please try again " + ex.Message);
                    }
                }

                if (!fileExist)
                {
                    var tempPath = Path.GetTempPath();
                    var textFile = "##### Log File #####";
                    textFile.AddLine(DateTime.Now.ToString());
                    File.WriteAllText(tempPath + "log.txt", textFile);
                    await StoryManager.MainWindow.FtpClient.UploadFileAsync(tempPath + "log.txt", "log.txt");
                    File.Delete(tempPath + "log.txt");
                }

                StoryManager.Add(new WelcomeScreen());
            }


            catch (Exception ex)
            {
                System.Windows.Forms.MessageBox.Show("An error ocurred please try again " + ex.Message);
            }
        }

        else
        {
            ContentGrid.Opacity = 1;
            LoadingIndicator.Opacity = 0;
        }
    }

Any tips/recommendations for better security, performance and error handling for this method or for FTP management in general?


The Rest.LoginAsync method implementation is:

  public static async Task<User> LoginAsync(string usernameValue, string passwordValue)
    {

        try
        {
            var result = await "https://foo.com/bar"
                .PostUrlEncodedAsync(new
                {
                    username = usernameValue,
                    password = passwordValue
                }).ReceiveString();

            var fields = result.Split(';');

            switch (result)
            {
                case "-1":
                    MessageBox.Show("User/Password error");
                    break;
                case "-2":
                    MessageBox.Show("User/Password error");
                    break;
                case "-3":
                    MessageBox.Show("No License");
                    break;
                case "-4":
                    MessageBox.Show("Connection error");
                    break;
                default:
                    break;
            }

            User loggedUser = new User
            {
                IdUser = int.Parse(fields[0]),
                UserName = fields[1],
                MembershipStatus = fields[2],
                Name = fields[3]
            };

            return loggedUser;

        }
        catch (Exception ex)

        {
            Console.WriteLine(ex.ToString());
        }

        return null;

    }

The rest of the operations are from C#, WPF or FluentFTP (CreateDirectory, UploadFile...)

\$\endgroup\$
  • 2
    \$\begingroup\$ It'd be better if you posted the complete method, not just a snippet. \$\endgroup\$ – t3chb0t Feb 21 '18 at 9:36
  • \$\begingroup\$ I updated my post with the other method code, however, i do not know how the FTP library is implemented or how C# or WPF implement there methods...Do you need anything else? \$\endgroup\$ – ODB8 Feb 21 '18 at 10:35
1
\$\begingroup\$
  • I would add a variable var ftpClient = StoryManager.MainWindow.FtpClient; to shorten the calls to FtpClient a bit
  • You catch exceptions several times, but handle them equally. And also continue after an error. I would make a single try..catch block around everything. So the exception handling code is not duplicated and the following actions don't get executed. (Why try to add a project dir to user dir project dir does not exist yet?)
\$\endgroup\$
1
\$\begingroup\$
try
{
  bool userDirExist = StoryManager.MainWindow.FtpClient.DirectoryExists(userDir);
  bool projectsDirExist = StoryManager.MainWindow.FtpClient.DirectoryExists(userDir + "/Projects");
  bool fileExist = StoryManager.MainWindow.FtpClient.FileExists("log.txt");

  if (userDirExist)
  {
      try
      {
          await StoryManager.MainWindow.FtpClient.SetWorkingDirectoryAsync(userDir);
      }
      catch (Exception ex)
      {
          System.Windows.Forms.MessageBox.Show("An error ocurred please try again " + ex.Message);
      }

      if (!projectsDirExist)
      {
          try
          {
              await StoryManager.MainWindow.FtpClient.CreateDirectoryAsync(userDir + "/Projects");
          }
          catch (Exception ex)
          {
              System.Windows.Forms.MessageBox.Show("An error ocurred please try again " + ex.Message);
          }
      }
  }

  else
  {
      try
      {
          await StoryManager.MainWindow.FtpClient.CreateDirectoryAsync(userDir);
          await StoryManager.MainWindow.FtpClient.SetWorkingDirectoryAsync(userDir);
          await StoryManager.MainWindow.FtpClient.CreateDirectoryAsync(userDir + "/Projects");
      }
      catch (Exception ex)
      {
          System.Windows.Forms.MessageBox.Show("An error ocurred please try again " + ex.Message);
      }
  }

  if (!fileExist)
  {
      var tempPath = Path.GetTempPath();
      var textFile = "##### Log File #####";
      textFile.AddLine(DateTime.Now.ToString());
      File.WriteAllText(tempPath + "log.txt", textFile);
      await StoryManager.MainWindow.FtpClient.UploadFileAsync(tempPath + "log.txt", "log.txt");
      File.Delete(tempPath + "log.txt");
  }

  StoryManager.Add(new WelcomeScreen());
}


catch (Exception ex)
{
  System.Windows.Forms.MessageBox.Show("An error ocurred please try again " + ex.Message);
}

I don't think this entire block is very useful. Regardless of any errors it tries to proceed with the next step. At the same time it confuses the user by showing them messages that read please try agian but the user has no chance in doing so as the code continues so he probably sees more of these messages asking themselfes what's going on? In the worst case, they'll see it four times.

What you should do:

  • if the next step relies on the results of the previous one, then remove all try/catches here and handle these exceptions inside the caller, only once and and with a single error message giving the user actually a chance to repeat the process
  • if, however, all steps are independent, then you should encapsulate them in separate methods and tell the user which steps failed and what needs to be tried again.

Tell the user exactly what went wrong like Could not create a new folder 'ABC' or Could not set working directory to 'ABC' etc. The stack trace won't help him very much as he would need to know the implementation to make heads or tails from them. And even you, in a couple of weeks, will be asking yourself what is that? and you'll end up debugging.


if (UsernameTextBox.Text.Equals("") || PasswordTextBox.Password.ToString().Equals(""))
{

  MessageBox.Show("Please, enter your credentials");
  ContentGrid.Opacity = 1;
  LoadingIndicator.Opacity = 0;
  return;
}

Instead of showing the user an error message after it's too late, you should validate these fields and prevent him from clicking OK (or whatever) until the input is valid.

There are many ways to do this and different frameworks might have their own ways but a native one is the Validating and Validated events. You can use them to show meaningful hints how to fill the input boxes.


public static async Task<User> LoginAsync(string usernameValue, string passwordValue)
{
  try
  {
      // ...

      return loggedUser;

  }
  catch (Exception ex)

  {
      Console.WriteLine(ex.ToString());
  }

  return null;
}

It's better to not return nulls. Let the caller handle the error as it must do it anyway in the form of a null. It would simplify the code not only here, because there wouldn't be any try/catch but also on the other side as you woulnd't need any null checks and probably more messages.

\$\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.