HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMinor

C# FTP class for interaction with FTP server (Download only)

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
ftpwithinteractiondownloadforserverclassonly

Problem

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 = ftpfil

Solution

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.
}

Context

StackExchange Code Review Q#112351, answer score: 7

Revisions (0)

No revisions yet.