1
\$\begingroup\$

This is a follow-up question for Android APP connect to FTP server in Java. I am attempting to create a tiny FTP host manager with FTPHostProfile class and FTPHostProfiles class.

The experimental implementation

  • Project name: FTPHostManagement

  • FTPHostProfile class implementation:

    package com.example.ftphostmanagement;

public class FTPHostProfile {
    private String ProfileName = "";
    private String FTPHostName = "";
    private int FTPPort = 21;
    private String FTPUsername = "";
    private String FTPPassword = "";

    public FTPHostProfile(String hostnameInput, int portInput, String usernameInput, String passwordInput)
    {
        this.ProfileName = FTPHostName;
        this.FTPHostName = hostnameInput;
        this.FTPPort = portInput;
        this.FTPUsername = usernameInput;
        this.FTPPassword = passwordInput;
    }

    public FTPHostProfile(String profileNameInput, String hostnameInput, int portInput, String usernameInput, String passwordInput)
    {
        this.ProfileName = profileNameInput;
        this.FTPHostName = hostnameInput;
        this.FTPPort = portInput;
        this.FTPUsername = usernameInput;
        this.FTPPassword = passwordInput;
    }

    public String GetProfilename()
    {
        return this.ProfileName;
    }

    public String GetHostname()
    {
        return this.FTPHostName;
    }

    public int GetPort()
    {
        return this.FTPPort;
    }

    public String GetUsername()
    {
        return this.FTPUsername;
    }

    public String GetPassword()
    {
        return this.FTPPassword;
    }
}

  • FTPHostProfiles class implementation:

    package com.example.ftphostmanagement;

import android.util.Log;

import org.apache.commons.net.ftp.FTP;

import java.util.ArrayList;
import java.util.stream.Collectors;

public class FTPHostProfiles {
    private ArrayList<FTPHostProfile> Collection = new ArrayList<>();

    /// Empty constructor
    public FTPHostProfiles()
    {

    }

    /// Single FTPHostProfile handler
    public FTPHostProfiles(FTPHostProfile input)
    {
        this.Collection.add(input);
    }

    /// Multiple FTPHostProfiles handler
    public FTPHostProfiles(ArrayList<FTPHostProfile> input)
    {
        this.Collection = input;
    }

    public FTPHostProfiles(FTPHostProfiles input)
    {
        this.Collection.addAll(input.Collection);
    }

    public FTPHostProfiles AddProfile(FTPHostProfile input)
    {
        this.Collection.add(input);
        return this;
    }

    public FTPHostProfiles AddProfiles(ArrayList<FTPHostProfile> input)
    {
        this.Collection.addAll(input);
        return this;
    }

    public FTPHostProfiles AddProfiles(FTPHostProfiles input)
    {
        this.Collection.addAll(input.Collection);
        return this;
    }

    public FTPHostProfile GetProfile(String profileNameInput)
    {
        var filteredResult = this.Collection.stream().filter(element -> (element.GetProfilename().equals(profileNameInput))).collect(Collectors.toList());
        if (filteredResult.stream().count() >= 1)
        {
            return filteredResult.get(0);
        }
        else
        {
            Log.d("GetProfile", "no such item in stored profiles");
            throw new IllegalStateException("no such item in stored profiles");
        }
    }

}

  • FTPconnection class implementation:

    package com.example.ftphostmanagement;

import android.util.Log;

import java.io.IOException;
import java.net.InetAddress;
import java.net.SocketException;
import java.net.UnknownHostException;

import org.apache.commons.net.SocketClient;
import org.apache.commons.net.ftp.FTP;
import org.apache.commons.net.ftp.FTPClient;
import org.apache.commons.net.ftp.FTPReply;

public class FTPconnection {

    FTPHostProfiles mFTPHostProfiles = new FTPHostProfiles();

    public FTPconnection(FTPHostProfiles input)
    {
        this.mFTPHostProfiles = input;
    }

    public FTPClient connectftp(String profilename)
    {
        //  Reference: https://stackoverflow.com/a/8761268/6667035
        FTPClient ftp = new FTPClient();
        try {
            var FTPHostProfile = mFTPHostProfiles.GetProfile(profilename);

            //  Reference: https://stackoverflow.com/a/55950845/6667035
            //  The argument of `FTPClient.connect` method is hostname, not URL.
            ftp.connect(FTPHostProfile.GetHostname(), FTPHostProfile.GetPort());
            boolean status = ftp.login(FTPHostProfile.GetUsername(), FTPHostProfile.GetPassword());
            if (status)
            {
                ftp.enterLocalPassiveMode();
                ftp.setFileType(FTP.BINARY_FILE_TYPE);
                ftp.sendCommand("OPTS UTF8 ON");
            }
            System.out.println("status : " + ftp.getStatus());
        } catch (UnknownHostException ex) {
            ex.printStackTrace();
        } catch (SocketException en) {
            en.printStackTrace();
        } catch (IOException e) {
            e.printStackTrace();
        }
        return ftp;
    }
}

  • User permission setting

    <uses-permission android:name="android.permission.INTERNET" />
<uses-permission android:name="android.permission.ACCESS_NETWORK_STATE" />
<uses-permission android:name="android.permission.ACCESS_WIFI_STATE" />

  • AndroidManifest.xml

    <?xml version="1.0" encoding="utf-8"?>
<manifest xmlns:android="http://schemas.android.com/apk/res/android"
    package="com.example.ftphostmanagement">

    <application
        android:allowBackup="true"
        android:icon="@mipmap/ic_launcher"
        android:label="@string/app_name"
        android:roundIcon="@mipmap/ic_launcher_round"
        android:supportsRtl="true"
        android:theme="@style/Theme.FTPHostManagement">
        <activity
            android:name=".MainActivity"
            android:exported="true">
            <intent-filter>
                <action android:name="android.intent.action.MAIN" />

                <category android:name="android.intent.category.LAUNCHER" />
            </intent-filter>
        </activity>
    </application>
    <uses-permission android:name="android.permission.INTERNET" />
    <uses-permission android:name="android.permission.ACCESS_NETWORK_STATE" />
    <uses-permission android:name="android.permission.ACCESS_WIFI_STATE" />
</manifest>

  • build.gradle

    plugins {
    id 'com.android.application'
}

android {
    compileSdk 30

    defaultConfig {
        applicationId "com.example.ftphostmanagement"
        minSdk 26
        targetSdk 30
        versionCode 1
        versionName "1.0"

        testInstrumentationRunner "androidx.test.runner.AndroidJUnitRunner"
    }

    buildTypes {
        release {
            minifyEnabled false
            proguardFiles getDefaultProguardFile('proguard-android-optimize.txt'), 'proguard-rules.pro'
        }
    }
    compileOptions {
        sourceCompatibility JavaVersion.VERSION_11
        targetCompatibility JavaVersion.VERSION_11
    }
}

dependencies {
    implementation fileTree(include: ['*.jar'], dir: 'libs')
    // https://mvnrepository.com/artifact/commons-net/commons-net
    implementation group: 'commons-net', name: 'commons-net', version: '20030805.205232'
    implementation 'androidx.appcompat:appcompat:1.2.0'
    implementation 'com.google.android.material:material:1.3.0'
    implementation 'androidx.constraintlayout:constraintlayout:2.0.4'
    testImplementation 'junit:junit:4.13.2'
    androidTestImplementation 'androidx.test.ext:junit:1.1.2'
    androidTestImplementation 'androidx.test.espresso:espresso-core:3.3.0'
}

Full Testing Code

  • MainActivity.java implementation:

    package com.example.ftphostmanagement;

import androidx.appcompat.app.AppCompatActivity;

import android.content.Context;
import android.os.AsyncTask;
import android.os.Bundle;
import android.util.Log;
import android.widget.Toast;

import org.apache.commons.net.ftp.FTPClient;

public class MainActivity extends AppCompatActivity {

    private FTPClient ftpClient;

    @Override
    protected void onCreate(Bundle savedInstanceState) {
        super.onCreate(savedInstanceState);
        setContentView(R.layout.activity_main);
        new FtpTask().execute();
    }

    void ShowToast(String Text, int Duration)
    {
        Context context = getApplicationContext();
        CharSequence text = Text;
        int duration = Duration;

        Toast toast = Toast.makeText(context, text, duration);
        toast.show();
    }

    //  AsyncTask must be subclassed to be used. The subclass will override at least one method
    //  `doInBackground(Params...)`, and most often will override a second one `onPostExecute(Result)`
    //  Reference: https://developer.android.com/reference/android/os/AsyncTask?authuser=4
    //  Reference: https://stackoverflow.com/a/12447497/6667035
    private class FtpTask extends AsyncTask<Void, Void, FTPClient> {
        //  `doInBackground` invoked on the background thread immediately after `onPreExecute()`
        //  finishes executing. This step is used to perform background computation
        //  that can take a long time. The parameters of the asynchronous task are
        //  passed to this step. The result of the computation must be returned by
        //  this step and will be passed back to the last step. This step can also use
        //  `publishProgress(Progress...)` to publish one or more units of progress.
        //  These values are published on the UI thread, in the `onProgressUpdate(Progress...)`
        //  step.
        protected FTPClient doInBackground(Void... args) {
            FTPHostProfiles ftpHostProfiles = new FTPHostProfiles();
            ftpHostProfiles.AddProfile(new FTPHostProfile(  "Profile1",
                    "Hostname1",
                    21,
                    "Username1",
                    "Password1"));
            ftpHostProfiles.AddProfile(new FTPHostProfile(  "Profile2",
                    "Hostname2",
                    21,
                    "Username2",
                    "Password2"));

            FTPconnection ftpConnect = new FTPconnection(ftpHostProfiles);
            FTPClient ftp = ftpConnect.connectftp("Profile1");
            return ftp;
        }

        //  `onPostExecute` invoked on the UI thread after the background computation finishes.
        //  The result of the background computation is passed to this step as
        //  a parameter.
        protected void onPostExecute(FTPClient result) {
            Log.v("FTPTask","FTP connection complete");
            ftpClient = result;
            //Where ftpClient is a instance variable in the main activity
            Log.v("Boolean.toString(ftpClient.isConnected())", Boolean.toString(ftpClient.isConnected()));
            ShowToast(Boolean.toString(ftpClient.isConnected()), Toast.LENGTH_SHORT);
        }
    }
}

All suggestions are welcome.

The summary information:

  • Which question it is a follow-up to?

    Android APP connect to FTP server in Java

  • What changes has been made in the code since last question?

    The FTPHostProfile class and FTPHostProfiles class implementation is the key part in this post.

  • Why a new review is being asked for?

    If there is any possible improvement, please let me know.

Improve this question
\$\endgroup\$

1 Answer 1

Reset to default
1
\$\begingroup\$

Important caveat: I know nothing about the Special Android coding conventions that are different from vanilla java.

General

It's preferable to use lowercase letters for acronyms in class names. `FtpHostProfile` is easier to read than `FTPHostProfile`.

Classes which have not been carefully designed for extension should be final.

It's idiomatic for variable names to start with a lowercase letter. profileName, etc.

Variables which do not change after the object is constructed should all be marked final. This reduces cognitive load on the reader as they can be confident they will not be reassigned after construction.

It's idiomatic for method names to start with a lowercase letter.

It's idiomatic for curly braces to be on the same line as the method declaration, not a line by themselves.

Prefer using the most generally applicable type on the left hand side of assignments. List instead of ArrayList, etc.

It's unnecessary to assign variables a value when they will get immediately overwritten in the constructor.

FTPHostProfile

There's no point in assigning the variables to be `""` if you're just going to overwrite them in both constructors.

In the first constructor, why is profile name getting set to "" by setting it to the value of FTPHostName? If you want it to be blank, make it blank. This is especially confusing because FTPHostName is reassigned on the next line.

Lines longer than a hundred characters or so are hard to read. Consider breaking up the declaration of the second constructor over multiple lines, or at least starting the variable declarations on their own line.

username and hostname are sometimes written as one word. Profilename is not an English word, so it should be broken up as ProfileName. The variable names are also inconsistent with the method names.

FTPHostProfiles

Why does constructor number 3 require that multiple profiles be submitted in an `ArrayList`? What's wrong with a `LinkedList`, or (gasp!) a `Set` of some kind? This constructor should take a `Collection`. Likewise for `addProfiles`.

Directly assigning the input of that constructor to Collection is a bad idea. You've now lost control of your internals - whoever passed in input still has a pointer to it, and can change its contents without the knowledge or consent of this class. Just call addAll on your collection - you get the values stored in input, and nobody can muck with your collection.

Collection is not a great variable name. It's not descriptive, and it collides with a class name. Consider profiles or ftpHostProfiles as variable names.

Do you actually need both constructors and methods which add profiles? That seems like overkill.

Having the addProfile methods return this is convenient for chaining, but consider if you need to return something else instead, such as whether the addition was successful or not.

getProfile makes it clear that that profiles should be a Map, not a List.

FTPconnection

The `c` should be capitalized.

mFtpHostProfiles should probably be private final.

This class has several unused imports.

You can probably call the variable profile I think there's enough context that the FTPHost is redundant.

Perhaps loggedIn would be more descriptive than status?

If you made all these changes, your code might look more like:

FtpConnection:

public final class FtpConnection {

    private final FtpHostProfiles mFtpHostProfiles = new FtpHostProfiles();

    public FtpConnection(FtpHostProfiles input) {
        this.mFtpHostProfiles.addProfiles(input);
    }

    public FTPClient connectftp(String profileName) {
        //  Reference: https://stackoverflow.com/a/8761268/6667035
        FTPClient ftp = new FTPClient();
        try {
            FtpHostProfile profile = mFtpHostProfiles.getProfile(profileName);

            //  Reference: https://stackoverflow.com/a/55950845/6667035
            //  The argument of `FtpClient.connect` method is hostname, not URL.
            ftp.connect(profile.getHostname(), profile.getPort());
            boolean loggedIn = ftp.login(profile.getUsername(), profile.getPassword());
            if (loggedIn) {
                ftp.enterLocalPassiveMode();
                ftp.setFileType(FTP.BINARY_FILE_TYPE);
                ftp.sendCommand("OPTS UTF8 ON");
            }
            System.out.println("status : " + ftp.getStatus());
        } catch (UnknownHostException ex) {
            ex.printStackTrace();
        } catch (SocketException en) {
            en.printStackTrace();
        } catch (IOException e) {
            e.printStackTrace();
        }
        return ftp;
    }
}

FtpHostProfile:

public final class FtpHostProfile {
    private final String profileName;
    private final String ftpHostName;
    private final int ftpPort;
    private final String ftpUserName;
    private final String ftpPassword;

    public FtpHostProfile(String hostNameInput, int portInput, String userNameInput, String passwordInput) {
        this.profileName = "";
        this.ftpHostName = hostNameInput;
        this.ftpPort = portInput;
        this.ftpUserName = userNameInput;
        this.ftpPassword = passwordInput;
    }
    public FtpHostProfile(
            String profileNameInput,
            String hostnameInput,
            int portInput,
            String usernameInput,
            String passwordInput) {
        this.profileName = profileNameInput;
        this.ftpHostName = hostnameInput;
        this.ftpPort = portInput;
        this.ftpUserName = usernameInput;
        this.ftpPassword = passwordInput;
    }

    public String getProfileName() {
        return this.profileName;
    }

    public String getHostname() {
        return this.ftpHostName;
    }

    public int getPort() {
        return this.ftpPort;
    }

    public String getUsername() {
        return this.ftpUserName;
    }

    public String getPassword() {
        return this.ftpPassword;
    }
}

FtpHostProfiles:

public final class FtpHostProfiles {
    private final Map<String, FtpHostProfile> profiles = new HashMap<>();

    public FtpHostProfiles() {
        // Empty constructor
    }

    public FtpHostProfiles addProfile(FtpHostProfile input) {
        this.profiles.put(input.getProfileName(), input);
        return this;
    }

    public FtpHostProfiles addProfiles(Collection<FtpHostProfile> input) {
        input.stream().forEach(profile -> profiles.put(profile.getProfileName(), profile));
        return this;
    }

    public FtpHostProfiles addProfiles(FtpHostProfiles input) {
        this.profiles.putAll(input.profiles);
        return this;
    }

    public FtpHostProfile getProfile(String profileNameInput) {
        FtpHostProfile profile = profiles.get(profileNameInput);
        if (profile == null) {
            Log.d("GetProfile", "No such item in stored profiles");
            throw new IllegalStateException("No such item in stored profiles");
        }
        return profile;
    }

}
Improve this answer
\$\endgroup\$

Your Answer

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

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