Simple survey application

I've completed the application and it works against all tests I've thrown at it. Now, I'd like to see if there's a way I can optimise the application and lower the code footprint.

The application is intended to be used for collecting input and collating this into an email, to be used later.

All variables are pulled from a Properties file, just in-case you're wondering.

UserInput

```
public class UserInput
{
public static InputStream file = null;
public static String name;
public static String fName;
public static String sName;
public static String ans1;
public static String ans2;
public static String ans3;
public static String ans4;
public static String ans5;

@SuppressWarnings({ "resource", "unused" })
public static void main(String[] args) throws NullPointerException
{
try
{
file = new FileInputStream("C:/Exercise 7 - Emails/configuration.properties");

Scanner nameVar1 = new Scanner(System.in).useDelimiter("\\n");
Scanner nameVar2 = new Scanner(System.in).useDelimiter("\\n");

Properties props = new Properties();
props.load(file);

String recipient1 = props.getProperty("email1");
String recipient2 = props.getProperty("email2");
String recipient3 = props.getProperty("email3");

String host = props.getProperty("host");
String port = props.getProperty("port");
String from = props.getProperty("from");

String iterate= props.getProperty("noOfEmails");
int noOfEmails = Integer.parseInt(iterate);
String number = props.getProperty("noOfQuest");
int noOfQuest = Integer.parseInt(number);

String[] questionArr = new String[5];
questionArr[0] = props.getProperty("Q1");
questionArr[1] = props.getProperty("Q2");
questionArr[2] = props.getProperty("Q3");
questionArr[3] = props.getProperty("Q4");
questionArr[4] = props.getProperty("Q5");

//System.out

For now, just a couple of points:

Long class and method

Your main method is ~500 lines long. That is definitely too much.

Extract Duplicate Code

The first thing you should do is extract duplicate code to functions. For example, you have this pattern multiple times:

if(something)
    {
        System.out.println("some message");
        try
        {
            Thread.sleep(some time);
        }
        catch(InterruptedException e)
        {
            Thread.currentThread().interrupt();
        }
        System.exit(0);
    }

Often, the boolean check is, if a string is empty. So your method could look like this:

private void checkAndHandleEmtpyString(String string, Long sleep, String message) {
        if (string.isEmpty()) {
            System.out.println(message);
            try {
                Thread.sleep(sleep);
            } catch (InterruptedException e) {
                Thread.currentThread().interrupt();
            }
            System.exit(0);
        }
    }

More than One -> List

Whenever you have more than one of something, it is generally a good idea to put it in a list, and then handle the general case of more than one, instead of handling cases one, two, ...

So for your case, it might look something like this:

private StringBuilder assembleMessage(String username, String[] answers) {
    StringBuilder messageText = new StringBuilder();
    messageText.append("Username: ");
    messageText.append(userName);
    messageText.append("\n");  
    for (int j = 0; j < noOfQuest; j++) {
        messageText.append("Response ");
        messageText.append(j);
        messageText.append(":");
        messageText.append(answers[j]);
        messageText.append("\n");  
    }
    return messageText;
}

And then use it like this:

message.setText(assembleMessage(username, answers));

This alone will probably get rid of about 120 lines of code in your EmailSend class :)

If you then also store your recipients in an array, all your nested for-if-for-ifs (and thus most of the EmailSend class) could probably look like this:

// load properties

        for (int i = 0; i < recipientCount; i++) {
            message.setRecipient(Message.RecipientType.TO, new InternetAddress(recipientArray[i]));
        }
        assembleMessage(userName, answers); 

        Transport.send(message);

Split code up in functions

Splitting your code in different functions doesn't necessarily reduce the amount of code, but it will increase readability. You could have functions such as loadProperties, checkInput, etc.

Main

Your main method should really only do one thing: start the program. Everything else should be happening in properly named methods.

OOP

I think many of the problems in your code - such as the length - stem from the fact that you are not using OOP.

This also means that your code will be very hard to adapt later on when requirements change. For example, what if there can be more than 5 question/answers? Then you have to change your code in very very many places.

Or what if there are more than 3 recipients?

So you might want to add classes such as Email, EmailBody (or EmailMessage), Answer, etc. And also generally but things in lists (see More than One -> List).

Misc

• All your fields are public, this doesn't really seem necessary.

• use better naming: what does s/f stand for in sName/fName? What does iCount count? What is an ans? Questions like these should not come up with good naming.

• IsLeadingCharNumfName(fName)==true can be written as IsLeadingCharNumfName(fName)

• if(cond) { return true; } else { return false; } can be written as return cond;

• In Java, it is customary to put the opening { on the same line.

• use more spaces (all formatting issues can easily be fixed with any IDE).

• using System.exit is generally not recommended.

• too many continue and break statements can hint to bad code design.

• try-catch blocks should be around as small a scope as possible, that way it's easier to see what might actually fail.

• use else-if instead of multiple ifs for clarity.

• do not hardcode file paths inside methods. At a minimum, I would put them in a final class level field, so they are easier to find.

StackExchange Code Review Q#80261, answer score: 7