6
\$\begingroup\$

I'm writing my own FTPclient for downloading files from my FTPserver as a project. So far I can download and go backwards/forwards in the folders using this code.

How does it look? What could be done better? etc.

Should I remove the interface and just create one class ftpItem instead of an interface and a file and folder class, they pretty much look the same, except that a file have no List which contains other items. But here a list is still created since file is part of the interface of ftpItem.

What's better practice, add the functionality of the big if clauses in the UI into the ftpItem class or create a ftpItem handler class which takes care of traversing the List?

Main GUI in Forms which uses Class ftpItem in namespace FtpLibrary:

using System;
using System.Collections.Generic;
using System.ComponentModel;
using System.Data;
using System.Drawing;
using System.Linq;
using System.Text;
using System.Threading.Tasks;
using System.Windows.Forms;
using System.Net;
using System.Security.Cryptography.X509Certificates;
using System.IO;
using System.Text.RegularExpressions;
using FtpLibrary;

namespace WindowsFormsApplication2
{
    public partial class Form1 : Form
{
    public Form1()
    {
        InitializeComponent();

        ftpfileList.MouseDoubleClick += FtpfileList_MouseDoubleClick;
        ftpfileList.MouseClick += FtpfileList_MouseClick;
    }

    private string _username = "******";
    private string _password = "*******";
    private string _adress = "ftp://" + "**.***.***.**/";
    private string _savepath = "C:/Users/" + Environment.UserName + "/Ftp/";
    private string _currentFtpItem;
    private ftpItem _next;

    private void FtpfileList_MouseClick(object sender, MouseEventArgs e)
    {
        _currentFtpItem = ftpfileList.GetItemText(ftpfileList.SelectedItem).Substring(0, ftpfileList.GetItemText(ftpfileList.SelectedItem).IndexOf(" , Size: "));
        if (ftpfileList.Items.Count >= 1)
        {
            ProgressLabel.Text = ftpfileList.GetItemText( ftpfileList.SelectedItem );
            if( _next.GetListItem( _currentFtpItem ).GetFileSize == 0 )
            {
                Label_currentPath.Text ="Number of items in Selected folder: " + _next.GetListItem( _currentFtpItem ).GetList.Count.ToString();
            }

        }

    }
    private void FtpfileList_MouseDoubleClick(object sender, MouseEventArgs e)
    {
        if ( _next.GetListItem( _currentFtpItem ).GetFileSize == 0 )
        {

            if ( _next.GetListItem( _currentFtpItem ).GetName == " /cd ..")
            {
                if( _next.GetPreviousItem == null)
                {
                    PrintDirectory(_next);
                }
                else
                {
                    _next = _next.GetPreviousItem;
                    PrintDirectory(_next);
                }

            }
            else if ( _next.GetListItem(_currentFtpItem).GetList.Count != 0 )
            {
                _next = _next.GetListItem( _currentFtpItem );
                PrintDirectory( _next );
            }
            else
            {
                _next.AddFolder( _currentFtpItem, new folder( _next.GetListItem( _currentFtpItem ).GetName, _next.GetListItem( _currentFtpItem ).GetPath, _next ) );
                _next = _next.GetListItem(_currentFtpItem);
                Task t = Task.Factory.StartNew( () => _next.Download( _username, _password ) ).ContinueWith( ( nextTask ) => { PrintDirectory( _next ); }, TaskScheduler.FromCurrentSynchronizationContext() );
            }

        }
            else
            {
            Task t = Task.Factory.StartNew( () => _next.GetListItem( _currentFtpItem ).Download( _username, _password ) );
            MessageBox.Show("File Selected! =D");
            MessageBox.Show(t.Status.ToString());
            }  
    }
    private void Connect_Click(object sender, EventArgs e) // init a session
    {
        _next = new folder( "root", _adress, null );
        Task t = Task.Factory.StartNew( () => _next.Download( _username, _password ) ).ContinueWith( ( nextTask ) => { PrintDirectory(_next); }, TaskScheduler.FromCurrentSynchronizationContext() );
    }
    private void PrintDirectory(ftpItem item)
    {
        ftpfileList.Items.Clear();
        debugList.Items.Clear();
        amountOfFolders.Text = "items in " + item.GetName + " : " + item.GetList.Count;
        foreach ( var t in item.GetList )
        {
            ftpfileList.Items.Add( t.Value.GetName + " , Size: " + t.Value.GetFileSize + " Byte" );
            debugList.Items.Add( t.Value.GetName + " , Size: " + t.Value.GetFileSize + " Byte, " + " Parent: " + t.Value.GetPreviousItem.GetName + " , Path: " + t.Value.GetPath );
        }
    }
    private void Button2_Click(object sender, EventArgs e) // used for debugging & testing tasks
    {
        MessageBox.Show("Total Number of folders created so for: " + _next.GetCount + " \n");


    }
}  
    }

FtpItem Class interface:

namespace FtpLibrary
    {
public interface ftpItem
{
   string GetName { get; }
    string GetPath { get; }
    int GetFileSize { get; }
    void AddItem( ftpItem item );
    SortedList<string, ftpItem> GetList { get; }
    ftpItem GetListItem( string name );
    void Download( string s, string t );
    ftpItem GetPreviousItem { get; }
    int GetCount { get; }
    void AddFolder( string key, ftpItem item );        
}

Class FtpItem : file

public class file : ftpItem
{
    private string _fileName;
    private int _fileSize;
    private string _filePath;
    private static int _fileAmount = 0;

    public file( string filename, string filesize, string filepath )
    {
        _fileName = filename;
        _fileSize = Int32.Parse( filesize );
        _filePath = filepath;
        _fileAmount += 1;
    }

    public string GetName { get { return _fileName; } }
    public string GetPath { get { return _filePath; } }
    public int GetFileSize { get { return _fileSize; } }
    public void AddItem( ftpItem item )
    {

    }
    public SortedList<string, ftpItem> GetList { get { return GetList; } }
    public void Download(string _username, string _password)
    {
        FtpWebRequest request = ( FtpWebRequest )WebRequest.Create( new Uri( this.GetPath ) );
        request.Credentials = new NetworkCredential( _username, _password );
        request.Proxy = null;
        request.UseBinary = true;
        request.EnableSsl = true;

        ServicePoint sp;
        sp = request.ServicePoint;
        sp.ConnectionLimit = 1;
        ServicePointManager.ServerCertificateValidationCallback = delegate { return true; };

        request.Method = WebRequestMethods.Ftp.DownloadFile;
        FtpWebResponse response = ( FtpWebResponse )request.GetResponse();
        Stream downloadStream = response.GetResponseStream();
        if(!Directory.Exists( "C:/Users/" + Environment.UserName + "/Ftp" ) )
        {
            Directory.CreateDirectory( "C:/Users/" + Environment.UserName + "/Ftp" );
        }

        FileStream newFile = File.Create( "C:/Users/" + Environment.UserName + "/Ftp/" + this.GetName );

        byte[] buffer = new byte[32 * 1024];
        int read;

        while ((read = downloadStream.Read( buffer, 0, buffer.Length ) ) > 0 )
        {
            newFile.Write( buffer, 0, read );
        }

        newFile.Close();
        downloadStream.Close();
        response.Close();
    }
    public ftpItem GetPreviousItem { get { return this; } }
    public ftpItem GetListItem( string name )
    {
        return this;
    }
    public int GetCount { get { return _fileAmount; } }
    public void AddFolder( string key, ftpItem item ) { }
    ~file() { _fileAmount--; }
}
    string GetName { get; }
    string GetPath { get; }
    int GetFileSize { get; }
    void AddItem( ftpItem item );
    SortedList<string, ftpItem> GetList { get; }
    ftpItem GetListItem( string name );
    void Download( string s, string t );
    ftpItem GetPreviousItem { get; }
    int GetCount { get; }
    void AddFolder( string key, ftpItem item );        
}

FtpItem Class : folder

public class folder : ftpItem
{
    private static int _folderAmount = 0;
    private string _folderName;
    private string _folderPath;
    private ftpItem _parent;
    private SortedList<string , ftpItem> _folderList;

    public folder( string foldername, string folderpath, ftpItem parent )
    {
        _folderName = foldername;
        _folderPath = folderpath;
        _parent = parent;
        _folderList = new SortedList<string, ftpItem>();
        _folderAmount += 1;
    }

    public string GetName { get { return _folderName; } }
    public void AddItem( ftpItem item )
    {
        _folderList.Add( item.GetName, item );
    }
    public string GetPath { get { return _folderPath; } }
    public ftpItem GetListItem( string name )
    {
        return GetList.ElementAt( GetList.IndexOfKey( name ) ).Value;
    }
    public SortedList<string, ftpItem> GetList { get { return _folderList; } }
    public int GetFileSize { get { return 0; } }
    public ftpItem GetPreviousItem { get { return _parent; } }
    public void Download( string _username, string _password )
    {

        FtpWebRequest request = ( FtpWebRequest )WebRequest.Create( new Uri( this.GetPath ) );
        request.Credentials = new NetworkCredential(_username, _password);
        request.Proxy = null;
        request.UseBinary = true;
        request.EnableSsl = true;

        ServicePoint sp;
        sp = request.ServicePoint;
        sp.ConnectionLimit = 1;
        ServicePointManager.ServerCertificateValidationCallback = delegate { return true; };


        request.Method = WebRequestMethods.Ftp.ListDirectoryDetails;
        FtpWebResponse response = ( FtpWebResponse )request.GetResponse();
        Stream responseStream = response.GetResponseStream();
        StreamReader reader = new StreamReader( responseStream );

        folder temp = new folder( " /cd ..", GetPath, this );
        AddItem(temp);

        while ( !reader.EndOfStream )
        {
            string ListDetails = reader.ReadLine();
            var listarray = ListDetails.Split( new char[] { ' ' }, 9, StringSplitOptions.RemoveEmptyEntries );

            if ( listarray.ElementAt(4) == "0" )
            {
                folder tempfolder = new folder( listarray.ElementAt(8), GetPath + listarray.ElementAt(8) + '/', this );
                AddItem( tempfolder );
            }
            else
            {
                file tempFile = new file( listarray.ElementAt(8), listarray.ElementAt(4), GetPath + listarray.ElementAt(8) );
                AddItem( tempFile );
            }
        }
        responseStream.Close();
        response.Close();
        reader.Close();
    }
    public int GetCount { get { return _folderAmount; } }
    public void AddFolder( string key, ftpItem item )
    {
        _folderList[ key ] = item;
    }
    ~folder() { _folderAmount--; }

}
\$\endgroup\$
  • \$\begingroup\$ What version of C# are you using? \$\endgroup\$ – Der Kommissar Nov 30 '15 at 17:32
  • \$\begingroup\$ Visual Studio 2015 community so should be c# 6.0 \$\endgroup\$ – A.J Nov 30 '15 at 18:39
  • \$\begingroup\$ Just as a remark: garbage collection may be multithreaded, so things like "~folder() { _folderAmount--; }" may cause unwanted behavior. \$\endgroup\$ – Caramiriel Nov 30 '15 at 19:53
7
\$\begingroup\$

Naming

  • First of all - naming. The standard C# naming guidelines suggest naming types with Capital letters - so Folder, File and FtpItem. (Note - Ftp, not FTP, as is the convention).

  • I would also suggest naming your concrete classes FtpFolder and FtpFile, just to keep them in line with the base class, and to prevent confusion if you're using both your library and System.IO types as well.

  • Next, your property names: properties in C# are an abstraction over get and/or set methods, but which behave like fields in terms of syntax - this means your property names should be Name and Path and FileSize, not GetName - the Get is implied when you use them as properties.

Inheritance Hierarchy

  • To answer your question, you definitely should use a base class/interface and specific implementations for File and Folder. Their behavior is similar but different enough. File-only methods and properties like FileSize should exist only in the FtpFile class. Folder-only members like Count should be only in FtpFolder. Since your users will likely be familiar with the System.IO namespace, it would be wise to see how it implements FileInfo and DirectoryInfo and maintain similar names and accessors.

Logic

  • I have to say it's hard for me to understand your logic with the whole ListItem/PreviousItem pair. Why not expose the list of FtpItems for the client to access?

  • Your GetListItem is misusing your data structure. The SortedList you're using is an implementation of IDictionary, meaning it's a collection that's accessible by key. All you need to do is _folderList[name], which will retrieve the value of that key. You don't need to access it by index after figuring out the index by the key.

  • Your Download methods have the potential to leak unmanaged resources. If an exception occurs while downloading, all your FileStreams and network streams could remain open, since you'll never get to the Close call. This is why they implement IDisposable, and this is what the using statement is for, ensuring that regardless of whether this code block completes or throws an exception, the StreamReader's Dispose method will be called, which will release the resources.

    using (StreamReader reader = new StreamReader( responseStream )) { // Your code. }

\$\endgroup\$
3
\$\begingroup\$

Some C#6.0 points:

  • You can use the expression-bodied feature to shorten up some vertical space, and remove some repetitive chunks. For Get-only properties, and methods which only have single return statements, you can use the => operator to shrink them up:

    public string GetName { get { return _folderName; } }
    public ftpItem GetListItem( string name )
    {
        return GetList.ElementAt( GetList.IndexOfKey( name ) ).Value;
    }
    

    to:

    public string GetName => _folderName;
    public ftpItem GetListItem(string name) => GetList.ElementAt(GetList.IndexOfKey(name)).Value;
    
  • String interpolation can do you some good here:

    "C:/Users/" + Environment.UserName + "/Ftp"
    

    to:

    $"C:/Users/{Environment.UserName}/Ftp"
    

    It tends to make it easier to follow strings along.

  • C#6.0 has read-only auto-implemented properties now. Simply define a property with only a get specification and it now functions the same as a readonly field, excepting that it's a property instead:

    private string _folderName;
    public string GetName { get { return _folderName; } }
    

    to:

    public string GetName { get; }
    

    And use GetName throughout. (This is where renaming GetName to just Name would come in handy, or better yet, create a base-class for it.)

\$\endgroup\$
  • 1
    \$\begingroup\$ $"C:/Users/{Environment.UserName}/Ftp" is a simple string interpolation example, but this should really use IO.Path.Combine. \$\endgroup\$ – Mark Hurd Dec 1 '15 at 4:51

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.