Overview
Examples
Screenshots
Comparisons
Applications
Download
Documentation
Tutorials
Bazaar
Status & Roadmap
FAQ
Authors & License
Forums
Funding Ultimate++
Search on this site
Search in forums












SourceForge.net Logo
Home » Developing U++ » Bazaar » FTP class and reference example for U++
Re: FTP class and reference example for U++ [message #47869 is a reply to message #47859] Fri, 14 April 2017 23:45 Go to previous messageGo to next message
Oblivion is currently offline  Oblivion
Messages: 275
Registered: August 2007
Location: Turkey
Experienced Member
Hello,

As Klugier suggested above, I've re-written the convenience functions, using a more object-oriented approach.
As a result, Ftp convenience functions, both single-threaded and multithreaded variants, are changed, and much less cluttered and more extendible now.

// Single-threaded.
Ftp::Result FtpGet(const Ftp::Request& request, Gate<int64, int64> progress = false, Event<> whenwait = CNULL);
Ftp::Result FtpPut(const Ftp::Request& request, Gate<int64, int64> progress = false, Event<> whenwait = CNULL);

// Multi-threaded.
int FtpAsyncGet(Ftp::Request& request, Event<Ftp::Result> progress = CNULL);
int FtpAsyncPut(Ftp::Request& request, Event<Ftp::Result> progress = CNULL);        


You can find the updated package in the first post of this topic.

FtpBrowser example and FTP package documentation are also updated to reflect the changes.


Regards.

Oblivion.
Re: FTP class and reference example for U++ [message #47876 is a reply to message #47869] Sat, 15 April 2017 19:23 Go to previous messageGo to next message
Klugier is currently offline  Klugier
Messages: 420
Registered: September 2012
Location: Poland, Kraków
Senior Member
Hello,

It is good to hear you introduced my proposals Smile

_________________________________________

I see one minor things to improvement - you could change the parameter name from whenwait to whenWait it will be more readable.

DirEntry&       User(const String& u)                           { vm(USER) = u; return *this; }

Should it be SetUser(const String& u)? Moreover the u parameter is misleading for the API consumer it should be explicitly called user.

_________________________________________

You should also consider moving your enums to c++ style:
 enum Style      { UNIX, DOS }; 

enum class Type { UNIX, DOS }; // You could consider chaining the name to Type

// The user can refere:
DirEntry::Type::UNIX

instead of:
DirEntry::UNIX

_________________________________________

Do we rely need this friendship declaration here?
        ValueMap        vm;
        Bits            owner, group, other;
        friend bool     ParseFtpDirEntry(const String& in, Vector<Ftp::DirEntry>& out);


Why not just create
const ValueMap& GetVm(); // Read only access - i believe ParseFtpDirEntry doesn't set anything?

IMO the variable name "vm" is also misleading. I would call it dirInfo or something more verbose. The same improvment can be used in Result class - just call it resultInfo.

_________________________________________

(Cosmetic - tiny) The class beginning has got problem with readability. It could be:
class Ftp : private NoCopy {
public: // Just remove extra empty line.
	class Request;
	class Result; // Do we need forward declarations in class? I believe when we remove friendship we will not ;)

        class DirEntry final : Moveable<Ftp::DirEntry> {



Sincerely,
Klugier


Ultimate++ - one framework to rule them all.
Re: FTP class and reference example for U++ [message #47877 is a reply to message #47876] Sat, 15 April 2017 19:38 Go to previous messageGo to next message
Klugier is currently offline  Klugier
Messages: 420
Registered: September 2012
Location: Poland, Kraków
Senior Member
Hello,

In my opinion handling multi-threading API using #ifdef in header is not good for the end user. Because, he/seh is obligated to use #ifdef in theire code. Personally, i would hide it inside the interface and use One container (Movable will not work for this class).

class Result // Interface - all methods are abstract
{
public:
   // The same as previous

   virtual bool IsAsync() = 0;

   // Mt methods are presented without #ifdef guard
};

class RegularResult : public Result // The name could be different
{
public:
    bool IsAsync() override { return false; }

    // Mt methods returning falses, -1 etc.
};

class AsyncResult : public Result
{
public:
    bool IsAsync() override { return true; }
    
    // Return correct values for MT
};

One<Result> result(new AsyncResult());



What do you think?

Sincerely,
Klugier


Ultimate++ - one framework to rule them all.
Re: FTP class and reference example for U++ [message #47882 is a reply to message #47877] Sun, 16 April 2017 18:28 Go to previous message
Oblivion is currently offline  Oblivion
Messages: 275
Registered: August 2007
Location: Turkey
Experienced Member
Quote:
Hello,

In my opinion handling multi-threading API using #ifdef in header is not good for the end user. Because, he/seh is obligated to use #ifdef in theire code. Personally, i would hide it inside the interface and use One container (Movable will not work for this class).

class Result // Interface - all methods are abstract
{
public:
// The same as previous

virtual bool IsAsync() = 0;

// Mt methods are presented without #ifdef guard
};

class RegularResult : public Result // The name could be different
{
public:
bool IsAsync() override { return false; }

// Mt methods returning falses, -1 etc.
};

class AsyncResult : public Result
{
public:
bool IsAsync() override { return true; }

// Return correct values for MT
};

One<Result> result(new AsyncResult());



What do you think?

Sincerely,
Klugier



Hello Klugier,

Thank you very much for your constructive criticism and suggestions.


I believe we might have a simpler solution. Smile

FtpAsyncGet() and FtpAsyncPut() functions call FtpAsyncIO().It is possible to return en error code if anything goes wrong.
So we can simply define a function, say, IsMultitihreaded(), to check the U++ multi-threading infrastructure automatically, in Ftp.cpp:


static inline bool IsMultithreaded() 
{
#ifdef flagMT
	return true;
#else
	return false;
#endif
}


Then we can call it in FtpAsyncIO:


Ftp::Result FtpAsyncIO(Ftp::Request request, Event<Ftp::Result> progress, bool put)
{
		// Check if U++ MT is enabled.
	Ftp::Result r;
	try {
		if(!IsMultithreaded())
			throw Exc("Multithreading is not enabled.");


// ...


	}
	catch(Exc& e) {
		// Couldn'r create or run the worker thread.
		r.info("state") = "failed";
		r.info("rc")    = -1;
		r.info("msg")   = e;
		LLOG("-- FTP worker failed. Reason: " << e);
	}
	return r;


In this way we can remove other ifdefs, and user won't be confused.
(Multithreading as a requirement is already mentioned in API docs for each method and function. )

Note that I also changed the return value from int to Ftp::Result. (Now they are similar to single-threaded variants)


Calling an async function such as Result::InProgress() now won't do harm, it wil silently return false.

AS for your other suggestions:

ParseFtpDirEntry() actually modifies the ValueMap. It parses the string into key-value pairs. However, now I put the actual parser code into DirEntry::Parser() method, and got rid of the friend declaration.

Same thing goes for the Request class. So, the ugly forward declarations are removed.

I updated the package to reflect the changes...

Regards,

Oblivion



[Updated on: Sun, 16 April 2017 20:01]

Report message to a moderator

Previous Topic: JobQueue: A simple and programmable job/queue model.
Goto Forum:
  


Current Time: Sun Apr 23 07:27:57 CEST 2017

Total time taken to generate the page: 0.17371 seconds