WinSCP IDisposable Wrapper

Question

I have written a simple wrapper for WinSCP in C#. I have written it to simplify SFTP connections I will needing to perform.

public class WinSCPConn : IDisposable
{
 //private fields
 private SessionOptions sessionOptions;
 private TransferOptions transferOptions;
 private Session session;
 //public properties
 public bool IsSessionOpen { get { return session.Opened; } }
 public WinSCPConn(string hostname, string username, string password)
 {
 sessionOptions = new SessionOptions();
 sessionOptions.Protocol = Protocol.Sftp;
 sessionOptions.HostName = hostname;
 sessionOptions.PortNumber = 22;
 sessionOptions.Password = password;
 sessionOptions.UserName = username;
 sessionOptions.Timeout = new TimeSpan(0, 3, 0);
 sessionOptions.GiveUpSecurityAndAcceptAnySshHostKey = true;
 transferOptions = new TransferOptions();
 transferOptions.TransferMode = TransferMode.Binary;
 try
 {
 session = new Session();
 session.ExecutablePath = Properties.Settings.Default.WinSCPPath;
 }
 catch (SessionLocalException)
 {
 throw;
 } 
 }
 public void Open()
 {
 try
 {
 if (session.Opened == false)
 {
 session.Open(sessionOptions);
 }
 }
 catch (SessionRemoteException)
 {
 throw;
 }
 }
 public TransferOperationResult SendFile(string SourceFile, string DestFile)
 {
 TransferOperationResult result;
 try
 {
 result = session.PutFiles(SourceFile, DestFile, false, transferOptions);
 return result;
 }
 catch (SessionRemoteException)
 {
 throw;
 } 
 }
 public void CreateDirectory(string FolderPath)
 {
 try
 {
 if (!session.FileExists(FolderPath))
 {
 session.CreateDirectory(FolderPath);
 }
 }
 catch (SessionRemoteException)
 {
 throw;
 }
 }
 public void Close()
 {
 if (session.Opened == true)
 {
 session.Dispose();
 }
 }
 public void Dispose()
 {
 Close();
 }
 }

It would be used as follows:

 using (WinSCPConn conn = new WinSCPConn("host", "username", "password"))
 { 
 conn.Open();
 conn.CreateDirectory("/path/");
 conn.SendFile(@"C:\file.txt","/path/file.txt");
 }

My questions are as follows:

1. How is my exception handling?
2. Is there too much going on in the constructor?

• Kiquenet: Error handling ?

Answer 1

Timeout

sessionOptions.Timeout = new TimeSpan(0, 3, 0);

Instead of using the TimeSpan constructor, I would instead use the TimeSpan.FromX methods. In this case, it would be TimeSpan.FromMinutes:

sessionOptions.Timeout = TimeSpan.FromMinutes(3);

It is a little more obvious that the timeout is 3 minutes now.

Port

sessionOptions.PortNumber = 22;

For this, I see two options:

• Take port as a parameter
• Omit the line

If you are always using the default port, you can omit the assignment. According to the WinSCP API Doc, leaving the port to the default of 0 will cause it to use the protocol default (22 for SFTP).

Keep in mind that always is not always always :)

Session Creation

try
{
 session = new Session();
 session.ExecutablePath = Properties.Settings.Default.WinSCPPath;
}
catch (SessionLocalException)
{
 throw;
} 

This part actually has two problems with it:

• the catch does not do anything aside from re-throw the exception
• you are throwing an exception in the constructor of an IDisposable type, which breaks using statements

For the first point, you could just remove the try/catch. It only has a purpose if you are planning on doing something in the catch. This is true for all of your try/catch blocks.

The next point could be solved with lazy initialization of the session. If you change the if statement within Open to do a null check, you can create and open the session there.

public void Open()
{
 if (session == null)
 {
 session = new Session();
 session.ExecutablePath = Properties.Settings.Default.WinSCPPath;
 session.Open(sessionOptions);
 }
}

Perhaps more robust, though, is just to add a separate if check and do the creation/opening separately:

public void Open()
{
 if (session == null)
 {
 session = new Session();
 session.ExecutablePath = Properties.Settings.Default.WinSCPPath;
 }
 if (session.Opened == false)
 {
 session.Open(sessionOptions);
 }
}

IDisposable

There are a couple things you may want to address with your IDispoable implementation:

• suppress finalization
• support cases where Dispose is never called
• allow sub-classes to override disposal behavior or seal the class

The first one is pretty easy:

public void Dispose()
{
 Close();
 GC.SuppressFinalize(this);
}

The second one depends largely on how the WinSCP Session class is written. You may need to ensure that the session is closed even if Dispose is never called, though you only need to worry about actually disposing of it when Dispose is called.

For the last one, I would just mark the class as sealed, since you have neither virtual nor protected members. Otherwise, you should provide a virtual Dispose method. Generally, when doing so, your empty Dispose remains non-virtual, and you would provide an additional protected overload which takes in a boolean.

For more information, see: Code analysis rule CA1063 and the MSDN page on Implementing a Dispose Method.

Answer 2

• a condition to be met like if (someBoolean == true) can be simplified to if (someBoolean). For reverse check use the Not operator ! like if (!someBoolean).

• Dipsose() should not only call Close() but also get rid of session, sessionOptions and transferOptions by calling Dispose() if they implement IDisposible or at least setting them to null.

• SendFile()

  you should declare the variable as near as possible to its usage and instead of catching .. rethrowing the exception, let it bubble up the tree. So it can be simplified to

  public TransferOperationResult SendFile(string SourceFile, string DestFile)
  {
   return session.PutFiles(SourceFile, DestFile, false, transferOptions);
  }
  

• if comments don't add any value, like explaining why something is done (what is done should be explained by the code itself), they should be removed like

  //public properties
  

Answer 3

1. Is there a reason you're catching some exceptions and just re-throwing them? It doesn't appear that you're really doing anything. In this case, you might as well not even catch the exception and just let it propagate up naturally. If the API threw a lot of different exceptions you'd like to catch, in that case you might want to re-wrap them in your wrapper class to make them easier to catch for your calling classes.
2. I don't think so - the length of code looks reasonable and it's readable.

• chazjn: no reason, i was just catching and throwing!
• trousyt: Removing the redundant try... catch { throw; } statements will simplify your code. Like @RubberDuck mentioned below, if you want to test your class, it may be better to pass the Option classes in (dependency injection). I think testing needs to be discretionary though; you are simply wrapping a pre-existing 3rd party API in this case, so in my opinion I don't think it's absolutely necessary. As always, YMMV depending on your needs though.