5
\$\begingroup\$

I have been working on a port of JavaCC for Android. It is mainly an interface. I needed only to modify the original source to redirect output to a TextView. The point of this project is to rekindle my coding skills after years of dormancy, to actually finish a coding project, and to make a useful port of JavaCC that I can use to play around with AIDE. This project was actually made in part with AIDE. I am excited to read your opinion on my code regarding what I consider to be the most difficult portion of the code. I was able to modularize many portions of this UI package, but this class persisted to be difficult. Please feel free to ask any applicable questions.

import java.io.File;
import java.io.IOException;
import java.io.InputStream;
import java.io.PrintStream;

import android.app.Activity;
import android.content.Context;
import android.os.Bundle;
import android.os.Handler;
import android.os.Message;
import android.text.Editable;
import android.text.InputType;
import android.text.TextWatcher;
import android.view.View;
import android.view.ViewGroup.LayoutParams;
import android.widget.Button;
import android.widget.CheckBox;
import android.widget.EditText;
import android.widget.LinearLayout;
import android.widget.ScrollView;
import android.widget.TextView;

public class MainActivity extends Activity {
    public static Context context;
    private String[] menuOptions = new String[] { "view license",
            "view parameters", "run program", "view output" };
    private Runnable[] menuOptionThreads = new Runnable[] { new Runnable() {
        @Override
        public void run() {
            try {
                println(getLicenseString().toString(), handler);
            } catch (IOException e) {
                println("Error reading license file.", handler);
            }
            scrolly.removeAllViews();
            scrolly.addView(text);
        }
    }, new Runnable() {

        @Override
        public void run() {
            scrolly.removeAllViews();
            scrolly.addView(linear);
        }

    }, new Runnable() {

        @Override
        public void run() {

            text.setText("");
            scrolly.removeAllViews();
            scrolly.addView(text);
            new Run(handler, options, fPath, fOutPath);
        }

    }, new Runnable() {

        @Override
        public void run() {

            scrolly.removeAllViews();
            scrolly.addView(text);
        }

    } };
    private TextView text;
    private Handler handler = new Handler() {
        @Override
        public void handleMessage(Message message) {
            final String messageString = message.getData().getString("print");
            runOnUiThread(new Runnable() {
                @Override
                public void run() {
                    if (text == null)
                        return;
                    text.setText(text.getText() + messageString + "\n");
                }
            });
        }
    };
    private static Option options[];
    private static String fPath;
    private static String fOutPath;
    private ScrollView scrolly;
    private LinearLayout linear;
    private EditText path;
    private EditText outPath;

    @Override
    public void onCreate(Bundle savedInstanceState) {
        super.onCreate(savedInstanceState);
        context = this;
        Streams.error = new PrintStream(new Streams.ErrorStream(handler));
        Streams.standard = new PrintStream(new Streams.StandardStream(handler));
        LinearLayout rootView = new LinearLayout(this);
        rootView.setOrientation(LinearLayout.VERTICAL);
        scrolly = new ScrollView(this);
        linear = new LinearLayout(this);
        linear.setOrientation(LinearLayout.VERTICAL);
        try {
            fPath = getIntent().getDataString();
            fPath = fPath.substring(fPath.indexOf("//") + 2);
        } catch (NullPointerException e) {
            fPath = "/";
        }
        text = new TextView(this);
        options = Option.generateDefaultOptionsObjectsArray();

        Button menuButton = new Button(this);
        menuButton.setText("menu");
        new AppMenu(context, menuOptions, menuButton, MainActivity.this,
                menuOptionThreads);
        Button fileSelectButton = new Button(this);
        fileSelectButton.setText("file");
        new FileSelect(context, new File("/").list(), fileSelectButton,
                MainActivity.this, "/", false);
        path = new EditText(this);
        path.setText(fPath);
        path.addTextChangedListener(new TextWatcher() {
            @Override
            public void beforeTextChanged(CharSequence p1, int p2, int p3,
                    int p4) {
            }

            @Override
            public void onTextChanged(CharSequence p1, int p2, int p3, int p4) {
            }

            @Override
            public void afterTextChanged(Editable arg0) {
                fPath = arg0.toString();
                int dot = fPath.lastIndexOf(".");
                    if (dot > 0) outPath.setText(fPath.substring(0, dot));
            }
        });
        outPath = new EditText(this);
        outPath.addTextChangedListener(new TextWatcher() {

            @Override
            public void afterTextChanged(Editable arg0) {
                // TODO Auto-generated method stub

            }

            @Override
            public void beforeTextChanged(CharSequence arg0, int arg1,
                    int arg2, int arg3) {
                // TODO Auto-generated method stub

            }

            @Override
            public void onTextChanged(CharSequence arg0, int arg1, int arg2,
                    int arg3) {
                fOutPath = arg0.toString();
            }

        });
        LinearLayout outPathOuterView = new LinearLayout(this);
        TextView outPathLabel = new TextView(this);
        outPathLabel.setText(Option.OUTPUT_DIRECTORY_OPTION);
        outPathOuterView.addView(outPathLabel);
        outPathOuterView.addView(outPath);
        linear.addView(outPathOuterView);
        for (Option option : options)
            linear.addView(generateViewForOption(option));
        LinearLayout topLayout = new LinearLayout(this);
        topLayout.addView(menuButton);
        topLayout.addView(fileSelectButton);
        topLayout.addView(path);
        rootView.addView(topLayout);
        scrolly.addView(linear);
        rootView.addView(scrolly);
        setContentView(rootView);
    }

    private CharSequence getLicenseString() throws IOException {
        InputStream is = getResources().openRawResource(R.raw.license);
        StringBuilder result = new StringBuilder();
        while (is.available() > 0)
            result.append((char) is.read());
        return result.toString();
    }

    private LinearLayout generateViewForOption(final Option option) {
        LinearLayout result = new LinearLayout(this);
        TextView name = new TextView(this);
        name.setText(option.getOptionName() + " = ");
        result.addView(name);
        if (option.getType().equals(Option.Type.INTEGER)) {
            EditText value = new EditText(this);
            value.setInputType(InputType.TYPE_CLASS_NUMBER);
            value.setText(String.valueOf(option.getInt()));
            option.setValueView(value);
            result.addView(value);
        } else if (option.getType().equals(Option.Type.BOOLEAN)) {
            CheckBox value = new CheckBox(this);
            value.setChecked(Boolean.valueOf(option.getBool()));
            option.setValueView(value);
            result.addView(value);
        } else if (option.getType().equals(Option.Type.STRING_OPTIONLISTABLE)) {
            Button value = new Button(this);
            value.setText(option.getCurrentValueAsString());
            new ParameterPopup(context, option.getPossibleOptions(), value);
            option.setValueView(value);
            result.addView(value);
        } else if (option.getType().equals(Option.Type.STRING)) {
            EditText value = new EditText(this);
            value.setText(option.getCurrentValueAsString());
            option.setValueView(value);
            result.addView(value);
        } else
            throw new UnsupportedOperationException();
        result.setLayoutParams(new LinearLayout.LayoutParams(
                LayoutParams.MATCH_PARENT, LayoutParams.WRAP_CONTENT));
        return result;
    }

    public static void println(String string, Handler handler) {
        Bundle bundle = new Bundle();
        bundle.putString("print", string);
        Message message = new Message();
        message.setData(bundle);
        handler.sendMessage(message);
    }

    public void setfpath(final String pathString) {
        runOnUiThread(new Runnable() {
            @Override
            public void run() {
                path.setText(pathString);
            }
        });
    }
}
\$\endgroup\$
  • 1
    \$\begingroup\$ Nice way to break down your previous question into smaller pieces! \$\endgroup\$ – Phrancis Nov 19 '14 at 3:17
2
\$\begingroup\$
private String[] menuOptions = new String[] { "view license",
        "view parameters", "run program", "view output" };
private Runnable[] menuOptionThreads = new Runnable[] { new Runnable() {
    @Override
    public void run() {
        try {
            println(getLicenseString().toString(), handler);
        } catch (IOException e) {
            println("Error reading license file.", handler);
        }
        scrolly.removeAllViews();
        scrolly.addView(text);
    }
}, new Runnable() {

    @Override
    public void run() {
        scrolly.removeAllViews();
        scrolly.addView(linear);
    }

}, new Runnable() {

    @Override
    public void run() {

        text.setText("");
        scrolly.removeAllViews();
        scrolly.addView(text);
        new Run(handler, options, fPath, fOutPath);
    }

}, new Runnable() {

    @Override
    public void run() {

        scrolly.removeAllViews();
        scrolly.addView(text);
    }

} };

So, there's an abstract "menu option" of sorts that has a String and Runnable.

Turn it into a class.


You could use some blank lines in your code. onCreate reads as a wall of text, and without semantic breaks ("paragraphs"), it's hard to see at a glance what's going on.

Additionally, you'll find that blank lines show how a function would be split up into other functions.

Let's take a look at your code:

@Override
public void onCreate(Bundle savedInstanceState) {
    super.onCreate(savedInstanceState);
    context = this;
    Streams.error = new PrintStream(new Streams.ErrorStream(handler));
    Streams.standard = new PrintStream(new Streams.StandardStream(handler));
    LinearLayout rootView = new LinearLayout(this);
    rootView.setOrientation(LinearLayout.VERTICAL);
    scrolly = new ScrollView(this);
    linear = new LinearLayout(this);
    linear.setOrientation(LinearLayout.VERTICAL);
    try {
        fPath = getIntent().getDataString();
        fPath = fPath.substring(fPath.indexOf("//") + 2);
    } catch (NullPointerException e) {
        fPath = "/";
    }
    text = new TextView(this);
    options = Option.generateDefaultOptionsObjectsArray();

    Button menuButton = new Button(this);
    menuButton.setText("menu");
    new AppMenu(context, menuOptions, menuButton, MainActivity.this,
            menuOptionThreads);
    Button fileSelectButton = new Button(this);
    fileSelectButton.setText("file");
    new FileSelect(context, new File("/").list(), fileSelectButton,
            MainActivity.this, "/", false);
    path = new EditText(this);
    path.setText(fPath);
    path.addTextChangedListener(new TextWatcher() {
        @Override
        public void beforeTextChanged(CharSequence p1, int p2, int p3,
                int p4) {
        }

        @Override
        public void onTextChanged(CharSequence p1, int p2, int p3, int p4) {
        }

        @Override
        public void afterTextChanged(Editable arg0) {
            fPath = arg0.toString();
            int dot = fPath.lastIndexOf(".");
                if (dot > 0) outPath.setText(fPath.substring(0, dot));
        }
    });
    outPath = new EditText(this);
    outPath.addTextChangedListener(new TextWatcher() {

        @Override
        public void afterTextChanged(Editable arg0) {
            // TODO Auto-generated method stub

        }

        @Override
        public void beforeTextChanged(CharSequence arg0, int arg1,
                int arg2, int arg3) {
            // TODO Auto-generated method stub

        }

        @Override
        public void onTextChanged(CharSequence arg0, int arg1, int arg2,
                int arg3) {
            fOutPath = arg0.toString();
        }

    });
    LinearLayout outPathOuterView = new LinearLayout(this);
    TextView outPathLabel = new TextView(this);
    outPathLabel.setText(Option.OUTPUT_DIRECTORY_OPTION);
    outPathOuterView.addView(outPathLabel);
    outPathOuterView.addView(outPath);
    linear.addView(outPathOuterView);
    for (Option option : options)
        linear.addView(generateViewForOption(option));
    LinearLayout topLayout = new LinearLayout(this);
    topLayout.addView(menuButton);
    topLayout.addView(fileSelectButton);
    topLayout.addView(path);
    rootView.addView(topLayout);
    scrolly.addView(linear);
    rootView.addView(scrolly);
    setContentView(rootView);
}

I'd start by sorting relevant lines.

fPath is defined as some value in a try catch, but then not used until 10 statements later. By sorting the statements you can say "this region handles fPath, that region handles..." I would give examples, but the code is a bit confusing right now.

Lastly, once you have blank lines, I'd suggest migrating whole paragraphs to separate functions. The function as a whole is too long. A good start would be migrating outPath and path out of the onCreate function, as the listeners take a lot of vertical space.


    if (option.getType().equals(Option.Type.INTEGER)) {
        EditText value = new EditText(this);
        value.setInputType(InputType.TYPE_CLASS_NUMBER);
        value.setText(String.valueOf(option.getInt()));
        option.setValueView(value);
        result.addView(value);
    } else if (option.getType().equals(Option.Type.BOOLEAN)) {
        CheckBox value = new CheckBox(this);
        value.setChecked(Boolean.valueOf(option.getBool()));
        option.setValueView(value);
        result.addView(value);
    } else if (option.getType().equals(Option.Type.STRING_OPTIONLISTABLE)) {
        Button value = new Button(this);
        value.setText(option.getCurrentValueAsString());
        new ParameterPopup(context, option.getPossibleOptions(), value);
        option.setValueView(value);
        result.addView(value);
    } else if (option.getType().equals(Option.Type.STRING)) {
        EditText value = new EditText(this);
        value.setText(option.getCurrentValueAsString());
        option.setValueView(value);
        result.addView(value)

First, migrate the bodies of these conditionals to separate functions. createEditText, createCheckbox, etc.

This helps anyone that wishes to understand the function - if the option type is a boolean it will create a checkbox, if the option type is a ... then they will create a...

The details of creating something aren't relevant in a function that decides what has to be created.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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