-2

I'm doing an assignment for my school site and I'm trying to find a "best practice" way to go about solving my problem.

So, whenever a certain method is run I have to reupload five different files, to five different FTP. Each one of these FTPs require different credentials, so for each upload I have to create a new client with the corresponding set of credentials.

Right now, my code looks something like this, which I feel is very dirty (but I'm not sure of a better way!):

public static string url;
public static string username;
public static string password;

public static void UploadFiles()
{
   for (var i = 0; i < 5; i++)
       GetCredentials(i)

   using (var client = new FtpClient(url, username, password))
   {
       // connect to client
       // and upload the file
       // using the parameters set in GetCredentials()
   }
}

private static void GetCredentials(int id)
{
    case 0:
        username = "user0"
        password = "pass0"
    case 1:
        username = "user1"
        password = "pass1"
    case 2:
        username = "user2"
        password = "pass2"
    case 3:
        username = "user3"
        password = "pass3"
    case 4:
        username = "user4"
        password = "pass4"
}

This works all well and good and I'm about to set this into production, but I'd really like to learn something from this, instead of just using the first solution that comes to my mind. Any advice is appreciated!

7
  • Possible duplicate of Most efficient method for large switch statements
    – gnat
    Jan 12, 2018 at 13:15
  • I don't agree - I'm not asking for efficiency in the switch statement, I'm asking about how to generally doing these things, ie alternate methods.
    – Tauropi
    Jan 12, 2018 at 13:19
  • For starters, are you sure the username/passwords will never change? They should probably be moved out of the code and into a configuration file. That might help organize the code for you too. That way you can just pull out username/password/url from a configuration object like a Dictionary.
    – neilsimp1
    Jan 12, 2018 at 13:26
  • Great idea. I'll move them into environment variables, unless a dictionary is much better? I read that environment variables are safer.
    – Tauropi
    Jan 12, 2018 at 13:30
  • 1
    Questions asking for assistance in explaining, writing or debugging code are off-topic here. Voting to close.
    – David Arno
    Jan 12, 2018 at 13:40

1 Answer 1

0

try this

public class Client
{
    public string username;
    public string password;
}

public void main(string[] args)
{
   List<Client> clients = new List<Client>()
   clients.Add(new Client() {username = "user1", password = "pass1"});
   clients.Add(new Client() {username = "user2", password = "pass2"});
   clients.Add(new Client() {username = "user3", password = "pass3"});
   ....
   //really you should load them from a file
   var url = "get this form the user?";
   UploadFiles(url, clients).Wait;
}

public async Task UploadFiles(string url, IEnumerable<Client> clients)
{
    List<Task> tasks = new List<Task>();
    foreach(var c in clients)
    {
        tasks.Add(ftpUpload(url,c));
    }
    await Task.WhenAll(tasks);
}

public async Task ftpUpload(string url, Client client)
{
    using (var ftpClient = new FtpClient(url, c.username, c.password))
    {
       // connect to client
       // and upload the file 
       await ftpClient.UploadAsync(file);
    }
}

Explanation:

You should avoid global variables. Your example is very simple but potentially if GetCredentials is called from another thread while you are in your upload loop the wrong username and password will be used.

Secondly, because the ftp upload requires no computation, its makes sense to do it asynchronously. Although, this might depend on your network bandwidth

7
  • 1
    downvotes due to spelling errors?
    – Ewan
    Jan 12, 2018 at 16:55
  • 1
    my code being so squeeky clean needs no explaination!!!!!!
    – Ewan
    Jan 12, 2018 at 16:57
  • 1
    LISTS ARE BETTAR!!!
    – Ewan
    Jan 12, 2018 at 19:02
  • 1
    Umm. Well, I'll give you a -1 for offering a solution that still has a list of credentials baked into source code ... A practice we may all be guilty of at some point. Not a practice we should be proud of or publicly suggest.
    – svidgen
    Jan 13, 2018 at 2:05
  • 2
    I guess you always get a few down votes from people who stop reading at the top
    – Ewan
    Jan 13, 2018 at 7:16

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.