patterncsharpMinor
C# FTP class for interaction with FTP server (Download only)
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
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
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
-
I would also suggest naming your concrete classes
-
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
Inheritance Hierarchy
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
-
Your
-
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
FileSizeshould exist only in theFtpFileclass. Folder-only members likeCountshould be only inFtpFolder. Since your users will likely be familiar with theSystem.IOnamespace, it would be wise to see how it implementsFileInfoandDirectoryInfoand 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.