I want to cleanup and refactor this library (v40 proposal and discussion)

Intro

Here are things that I'd like to cleanup going forward. Since this has ramifications on all existing projects using FluentFTP, and since these will be major breaking changes to the API, I'd like community feedback on these changes so that they are not hasty, disruptive, or out-of-touch with the actual userbase.

I would like these to be part of the v40 release so its easy for existing users to understand that v39 is the "legacy API" and that v40+ will use the "newer API". I have never undertaken such a large systematic refactoring before.

Topic 1

Problem: We have a lot of old/weird/unused functionality that is probably broken or not well tested or not being used.
- EnableThreadSafeDataConnections - Crazy feature which spawns new FTP connections for each file transferred
- QuickTransferLimit - Implemented but disabled
- FtpClient.Connect(Uri) - Static constructors
- FtpClient.GetPublicIP() - Static methods
- DereferenceLink - does anyone even use this?
- 200 different constructors for FtpClient - I would just like 1 or 2 constructors for common patterns.
Solution: Drop these features entirely.

Topic 2

Problem: We have a massive set of properties directly within the FtpClient class which clutters up its API.
Solution: Store all configuration setting properties into child classes whose references are stored within the FtpClient. This groups up the properties into logical categories and also eliminates them from the FtpClient API.
- client.Connect() becomes client.Connect(FtpProfile)
- client.FXPProgressInterval becomes client.FXP.ProgressInterval
- client.DownloadZeroByteFiles becomes client.Download.ZeroByteFiles
- client.TransferChunkSize becomes client.Download.ChunkSize and client.Upload.ChunkSize

Topic 3

Problem: Most FTP servers don't support directly downloading/uploading from an absolute path (eg: /root/animals/duck.txt, therefore we should always set the working directory to the folder (eg: /root/animals/) and then download the file (duck.txt). And we should not allow the user to manually modify the working directory because it would conflict with this system.
Solution: Drop support for GetWorkingDirectory and SetWorkingDirectory and instead always automatically set the working directory before file transfers or even getting the file size, etc. This would even fix issues where there are spaces in directory paths, and many other things. It would put us in parity with FileZilla.

Topic 4

Problem: Our logging is a mess. We should be able to integrate with NLog and other standard .NET loggers.
Solution: Drop support for FtpTrace and its outdated API and instead support a client.Logger property which can be set to our default logger or an NLog instance, etc.

Topic 5

Problem: Poor implementation of timezone conversion. Fixed implementation that does not allow users to customize it.
Solution: Implement a strategy pattern so users can select from:
- NoTimeConversion
- TimeZoneConversion - a new method that allows for DST to be observed (missing feature)
- TimeOffsetConversion - users specify which is their PC timezone and the server timezone and we do simple math to convert it (current method)

Topic 6

Problem: We have 2 implementations of each function, a synchronous version and an async version. This causes bloat, mistakes/mismatches in the implementation, and weird behavior.
Solution: Drop support for synchronous API. Drop support for <.NET 3.5 as it doesn't support async.

Topic 7

Problem: Too much bloat with older IAsyncResult style async functions.
Solution: Remove it. Anyone who wants async can just use async/await.

Lets talk!

Please start the discussions below! Add your comments and mention which topic you are talking about. You can comment on any topic(s) that you like.

@FanDjango

@FanDjango @tommysor @ilc-ilopez @fire-lizard @daviddenis-stx @hez2010 @n0ix @aliquid @wakabayashik @Mortens4444 @radiy @WolfspiritM @Lukazoid @iamjay @peterfortuin @taoyouh @artiomchi @sierrodc @jblacker @L3Z4 @jnyrup @Adhara3 @brucemcmillan

Hi,

Foreword

as I already suggested in other issues, I would try, whenever possible, to avoid behaviour by enums, favoring a strategy pattern instead. This would have the benefit to help (not solve) Topic 2 for example, moving specific configuration only where necessary. To be clear I would favour something like new XxxFtpClient() which is an FTPClient of type Xxx ad internally it is built by injecting specific strategies for that server type. It's not easy but I see a potentially big architectural advantage.

Topic 3

This is the one I am mostly involved in. I agree that not allowing browsing the tree would help and that would also be strategy oriented. User provides the full path, it's up to the implementation use it as it is or navigate to the folder and then download

Topic 4

Having a simple IFtpClientLogger interface is a good way to go, you just need 5 or 6 logging methods and the user can easily plug in any implementation. Do not add any specific dependency.

Topic 6

I am not a huge fan of async code, to put it mild. But I get this is a feature largely used nowadays, the only thing I could say is that at the moment I'm only using sync methods from FluentFTP.

Hope this helps
A

Topic 3/4/5: Agreed (more or less)

Forward;

as I already suggested in other issues, I would try, whenever possible, to avoid behaviour by enums, favoring a strategy pattern instead.

It sounds pretty cool and could fix issues by offering simple and complex strategies. Assuming we have just one FtpClient for simplicity and all features are implemented in modular strategies, suggest the implementation. For example how would functionality be divided into strategies. For something like downloading a file, we don't really have strategies. Unless we should? Like SimpleFileDownload and ChunkedFileDownload etc. And what should the API look from the client? client.Downloader = new SimpleFileDownload()? Please suggest the concrete implementation so I can get a better idea.

I'm not expert on FluentFTP client internals, so I'm still talking in a very theoretical way, based mainly on the Find What Is Varying and Encapsulate It basic principle.
So I would ideally try to have something like the following (pseudocode):

public class FTPClient
{
  public FTPClient(IDownloadStrategy downloadStrategy, IUploadStrategy uploadStrategy, IAuthenticationStrategy authStrategy)
  {
    // Of course, as many strategies as required
  }

  public void Download()
  {
    // You can du stuff here before calling the strategy, but it MUST be server independent, guaranteed
    m_downloadStrategy.Download();

    // Or the call could contain another strategy
   m_downloadStrategy.Download(m_authStrategy);

    /* This is a key point, strategies could have other strategies from constructor or at runtime, 
it really depends on the scenario. Passing at runtime may allow different behaviour, there is no 
good or bad, passing in construction gives the dev a message: this is readonly and cannot be 
changed, so depending on the scenario may be the right choice */
  }
}

public static class FritzBoxFtpClient
{
  // Here custom properties for this kind of client, should help with cluttered config
  // e.g. TransferChunkSize goes into the proper stategy
  public static FTPClient Create() 
  {
    return new FTPClient(); // Inject strategies for this specific client
  }
}

I do expect having, for each strategy type, a number N of implementations which is less than the actual number of clients, i.e. I guess a BasicDownloadStrategy may be reused for multiple clients.
I used the static factory method to avoid inheritance and because that would force to put all different behaviours into strategies and not in method overloading (mixing the two would kill code readability).

The other advantage is that this pattern would allow native extendibility. E.g. a user has a new client, maybe a custom one. He can create his own strategies and inject into FTPClient constructor and he should have something working. Basically, you would end up having some core classes + a bunch of add-ons (each client).

Moreover, async is in the syntax, so it doesn't work, but a Download strategy could be one file at a time and another implementation may be EnableThreadSafeDataConnections, basically instead of an option, it's a behaviour.
Topic 5 would also fall into this. You provide the API (i.e. the interface) and, if you like, a couple of very basic implementations, but then a user can always customize if required by injecting a different implementation.

The hard part is:

exact opposite of what you have today, instead of a huge single FTPClient class that does everything, several smaller classes. This helps with Unit testing, though
modularity for working properly needs to be as atomic as the smallest change, so it may end up being a bit iterative, e.g. IDownloadStrategy may not be enoght, you may need to split it (or to provide parts to it) like IPathBuildingStrategy or IServerResponseStrategy, just to give an idea

Just coming from my insular view for the IBM parts of this:

I am worried about:

Topic 3

Anyone writing a FTP Client GUI and wanting to give the user a way to walk the server-OS's file system will want to have these two functions, right? And the IBM parts really heavily use PWD/CWD.

Topic 6

I am not a huge fan of async code, to put it mild. But I get this is a feature largely used nowadays, the only thing I could say is that at the moment I'm only using sync methods from FluentFTP

Same for me.

I find it strange that C# religion goes for "not duplicating code" but makes it so difficult to have the same basic algorithm present in both sync and async modes. Certainly in an C(++) environment I could envision a single code set (using the preprocessor) that represents both flavors in a single source. How to do in C#?

Topic 3

Anyone writing a FTP Client GUI and wanting to give the user a way to walk the server-OS's file system will want to have these two functions, right? And the IBM parts really heavily use PWD/CWD.

Maybe we can have a strategy for this?

ManualDirectoryStrategy - the current get/set PWD/CWD
AutomaticDirectoryStrategy - a new one which can auto set the dir based on the file commands

Names are just suggestions, you can suggest better ones. I also don't like having the word "Strategy" as a suffix, is there a better naming scheme? <Variant> <Subject> "Strategy" seems like a very Java-esque too-many-words naming scheme and I would like a better one!

For backwards compatibility, we could support all the older constructors, which would be implemented like:

public FtpClient() {
	this.DownloadStrategy = new ChunkedDownloadStrategy();
	this.UploadStrategy = new ChunkedUploadStrategy();
	this.PathStrategy = new ManualDirectoryStrategy();
	....
}

Topic 6

I am not a huge fan of async code,

Same for me, however lots of contributors were excited to contribute async versions for some reason... And it makes the library look more "modern". I don't know why users can't just create a background worker/thread and use the sync API, maybe its because of the whole node.js "everything is an async/non-blocking operation" pattern that took the world by storm.

This may be silly or very naif.
What about adding sync stuff as ExtensionMethods? That would allow keeping the class basically sync, users would be happy as they can call the method directly on the object.

A

This may be silly or very naif. What about adding sync stuff as ExtensionMethods? That would allow keeping the class basically sync, users would be happy as they can call the method directly on the object.

No, that would not solve the main problem of duplicating code.

I would like to see

Drop net3.5 support and keep only async variant of APIs, for those who want to use blocking sync call, they can always do FooAsync().ConfigureAwait(false).GetAwaiter().GetResult().
Bringing IAsyncEnumerable support again to support "list streaming", without the dependency of Rx/Lx.Net.

Latest master works for me. LogUserName and LogPassword: works also

@FanDjango

Should I release the v40 nuget @FanDjango and @jnyrup ?

I have only a few minor comments left, but only the first one might be blocking v40 as it changes the API.

From the C# Design Guidelines

❌ DO NOT provide instance fields that are public or protected.
You should provide properties for accessing fields instead of making them public or protected.

There are 77 violations of that guideline

list of files

FluentFTP\Helpers\FtpListParser.cs:17
FluentFTP\Helpers\FtpListParser.cs:26
FluentFTP\Helpers\FtpListParser.cs:31
FluentFTP\Helpers\FtpListParser.cs:36
FluentFTP\Helpers\FtpListParser.cs:45
FluentFTP\Model\FtpClientState.cs:14
FluentFTP\Model\FtpClientState.cs:21
FluentFTP\Model\FtpClientState.cs:27
FluentFTP\Model\FtpClientState.cs:32
FluentFTP\Model\FtpClientState.cs:37
FluentFTP\Model\FtpClientState.cs:42
FluentFTP\Model\FtpClientState.cs:47
FluentFTP\Model\FtpClientState.cs:52
FluentFTP\Model\FtpClientState.cs:57
FluentFTP\Model\FtpFxpSession.cs:15
FluentFTP\Model\FtpFxpSession.cs:20
FluentFTP\Model\FtpFxpSession.cs:25
FluentFTP\Model\FtpFxpSessionAsync.cs:17
FluentFTP\Model\FtpFxpSessionAsync.cs:22
FluentFTP\Model\FtpFxpSessionAsync.cs:27
FluentFTP\Model\FtpListItem.cs:51
FluentFTP\Model\FtpListItem.cs:56
FluentFTP\Model\FtpListItem.cs:61
FluentFTP\Model\FtpListItem.cs:81
FluentFTP\Model\FtpListItem.cs:86
FluentFTP\Model\FtpListItem.cs:93
FluentFTP\Model\FtpListItem.cs:98
FluentFTP\Model\FtpListItem.cs:103
FluentFTP\Model\FtpListItem.cs:108
FluentFTP\Model\FtpListItem.cs:113
FluentFTP\Model\FtpListItem.cs:118
FluentFTP\Model\FtpListItem.cs:123
FluentFTP\Model\FtpListItem.cs:128
FluentFTP\Model\FtpListItem.cs:133
FluentFTP\Model\FtpListItem.cs:138
FluentFTP\Model\FtpListItem.cs:144
FluentFTP\Model\FtpListItem.cs:149
FluentFTP\Model\FtpListItem.cs:155
FluentFTP\Model\FtpListItem.cs:161
FluentFTP\Model\FtpListItem.cs:168
FluentFTP\Model\FtpProfile.cs:21
FluentFTP\Model\FtpProfile.cs:26
FluentFTP\Model\FtpProfile.cs:31
FluentFTP\Model\FtpProfile.cs:36
FluentFTP\Model\FtpProfile.cs:41
FluentFTP\Model\FtpProfile.cs:46
FluentFTP\Model\FtpProfile.cs:51
FluentFTP\Model\FtpProfile.cs:56
FluentFTP\Model\FtpProfile.cs:61
FluentFTP\Model\FtpProfile.cs:66
FluentFTP\Model\FtpResult.cs:15
FluentFTP\Model\FtpResult.cs:20
FluentFTP\Model\FtpResult.cs:25
FluentFTP\Model\FtpResult.cs:30
FluentFTP\Model\FtpResult.cs:45
FluentFTP\Model\FtpResult.cs:50
FluentFTP\Model\FtpResult.cs:55
FluentFTP\Model\FtpResult.cs:60
FluentFTP\Model\FtpResult.cs:65
FluentFTP\Model\FtpSizeReply.cs:10
FluentFTP\Model\IntRef.cs:7
FluentFTP\Rules\FtpFileExtensionRule.cs:17
FluentFTP\Rules\FtpFileExtensionRule.cs:22
FluentFTP\Rules\FtpFIleNameRegexRule.cs:19
FluentFTP\Rules\FtpFIleNameRegexRule.cs:24
FluentFTP\Rules\FtpFileNameRule.cs:16
FluentFTP\Rules\FtpFileNameRule.cs:21
FluentFTP\Rules\FtpFolderNameRegexRule.cs:19
FluentFTP\Rules\FtpFolderNameRegexRule.cs:24
FluentFTP\Rules\FtpFolderNameRegexRule.cs:29
FluentFTP\Rules\FtpFolderNameRule.cs:13
FluentFTP\Rules\FtpFolderNameRule.cs:24
FluentFTP\Rules\FtpFolderNameRule.cs:29
FluentFTP\Rules\FtpFolderNameRule.cs:34
FluentFTP\Rules\FtpSizeRule.cs:16
FluentFTP\Rules\FtpSizeRule.cs:21
FluentFTP\Rules\FtpSizeRule.cs:26

FtpSocketStream.SslStream is unused

Unused return values

Use the return value of method 'Trim'.	FluentFTP\Helpers\Parsers\NonStopParser.cs:59
Use the return value of method 'Parse'.	FluentFTP\Helpers\Parsers\VMSParser.cs:96

DO NOT provide instance fields that are public or protected

Yes I never understood why C# forced the slower properties to be used instead of public fields. It actually has a negative effect on run time performance (may be negligible). Most of the uses here are just plain POCOs which don't really need properties.

A benefit of properties is that in debug mode, you can set breakpoints on them.

In release mode the call to the property is inlined, so the generated assembly is identical whether you use a field with or without a an explicit backing field.

SharpLab

@jnyrup

@jnyrup Ok, agreed that we should follow guidelines as it makes the lib more professional.

I fixed all issues except for the last 2 (return values), they are valid, or I cannot figure it out.

Release https://www.nuget.org/packages/FluentFTP/40.0.0-beta5

Guys please check and confirm it works, so I can release the final nuget.

A benefit of properties is that in debug mode, you can set breakpoints on them.

Something one takes for granted until you need to find out from whence came an update, or can you not break-point on a value (i.e. a field) changing its value?

In release mode the call to the property is inlined, so the generated assembly is identical whether you use a field with or without a an explicit backing field.

This is a profoundly valuable statement that reconciles me with the tediousness of making "quasi" unneeded properties.

@robinrodricks

@robinrodricks Newest master works for me

One point:

If I compare, file by file, the Async client with the Sync client, there are subtle differences in the code. See the following screenshot of ExamDiff on Authenticate.cs just a an example.

There are actually a few of these, some only the comments, but some more than just that. Worth looking at, file by file (61 files)?

Should I release the v40 nuget

I am just a single one of your users. Who am I to decide. You are raring to go and the changes are sensible, the code is much nicer this way, it becomes much easier (See my "ExamDiff" on previous post) to see any SYNC-ASYNC discrepancies, much less "#if"finess in the code.

I needed a small amount of code modifications. Users might gripe and grouch, but they gain a logging framework or can use legacy logging, they get cleaner code leading to better maintenance. If you follow through with handling their issues, they will see the upside of the V39-V40 switch.

So ok, go for it... IMHO

So, I am finished with this thread and will move on to other topics on my Todo-List. I thank all for the fruitful discussions and I learned a lot also.

Who am I to decide. You are raring to go and the changes are sensible, the code is much nicer this way, it becomes much easier (See my "ExamDiff" on previous post) to see any SYNC-ASYNC discrepancies, much less "#if"finess in the code.

Thank you!

If I compare, file by file, the Async client with the Sync client, there are subtle differences in the code. See the following screenshot of ExamDiff on Authenticate.cs just a an example.

Seems like a bug.

There are actually a few of these, some only the comments, but some more than just that. Worth looking at, file by file (61 files)?

You can go ahead if you have time, sorry I won't have time this week. Beyond Compare would make quick work of it.

I am just a single one of your users. Who am I to decide. .. So ok, go for it... IMHO

I'll wait some days and then release.

@jnyrup

@jnyrup

In release mode the call to the property is inlined, so the generated assembly is identical whether you use a field with or without a an explicit backing field.

I was not aware of this.

Published v40!

https://www.nuget.org/packages/FluentFTP/40.0.0

Now to wait for the coming storm (of issues!)

Thanks to ALL who participated in this thread. I am closing it, we can take the remaining features or any new issues as part of new discussions.

Navigation Menu

Repository navigation

I want to cleanup and refactor this library (v40 proposal and discussion) #906

Intro

Topic 1

Topic 2

Topic 3

Topic 4

Topic 5

Topic 6

Topic 7

Lets talk!

Foreword

Topic 3

Topic 4

Topic 6

132 remaining items

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

I want to cleanup and refactor this library (v40 proposal and discussion) #906

Description

Intro

Topic 1

Topic 2

Topic 3

Topic 4

Topic 5

Topic 6

Topic 7

Lets talk!

Activity

robinrodricks commented on Aug 5, 2022

Adhara3 commented on Aug 5, 2022

Foreword

Topic 3

Topic 4

Topic 6

robinrodricks commented on Aug 6, 2022

Adhara3 commented on Aug 6, 2022

FanDjango commented on Aug 6, 2022

robinrodricks commented on Aug 7, 2022

Adhara3 commented on Aug 7, 2022

robinrodricks commented on Aug 7, 2022

hez2010 commented on Aug 7, 2022

132 remaining items

FanDjango commented on Sep 4, 2022

robinrodricks commented on Sep 4, 2022

jnyrup commented on Sep 4, 2022

robinrodricks commented on Sep 4, 2022

jnyrup commented on Sep 4, 2022

robinrodricks commented on Sep 4, 2022

FanDjango commented on Sep 4, 2022

FanDjango commented on Sep 4, 2022

FanDjango commented on Sep 4, 2022

robinrodricks commented on Sep 5, 2022

robinrodricks commented on Sep 7, 2022

robinrodricks commented on Sep 7, 2022

Metadata

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Issue actions