8
\$\begingroup\$

I'm the proud owner of the Vitsy programming language, which I've been working on for some time (except recently, because high school). It's only been used in PPCG so far, but I hope to expand it to more useful things. It is a top-access-only 2-dimensional stack-based language that moves in multiple directions through Vitsy code with the ability to use other "classes". The only problem is that, a lot of the time, the way I have been loading these "classes" is extremely inefficient, terribly written, and shoddily thought out.

Here is an excerpt that I would like to be reviewed:

public static void methodHandler(int source) throws InterruptedException, IOException, ScriptException {
    olddir.add(direction);
    direction = true;
    if (source == -1) {
        oldpos.add(new Integer[]{currin,position});
        currin = top().intValue();
        position = 0;
        rmtop();
        while (position < currin().length && !ending) {
            if (!looping.get(looping.size()-1) && position > currin().length - 1) {
                break;
            }
            else opHandle();
            position = (direction) ? (position + 1): (position - 1 >= 0) ? position - 1: currin().length-1;
        }
        currin = oldpos.get(oldpos.size()-1)[0].intValue();
        position = oldpos.get(oldpos.size()-1)[1].intValue();
        oldpos.remove(oldpos.size()-1);
        direction = olddir.get(olddir.size()-1);
        olddir.remove(olddir.size()-1);
        ending = false;
        return;
    }
    instruct.add(FileHandler.getFileInstruct(new String[]{(source == -2)?extender.get(extender.size()-1):users.get(users.size()-1)[source]}, new boolean[]{false, false}));
    users.add(FileHandler.getFileInstruct(new String[]{(source == -2)?extender.get(extender.size()-1):users.get(users.size()-1)[source]}, new boolean[]{true, false}).get(0));
    extender.add(FileHandler.getFileInstruct(new String[]{(source == -2)?extender.get(extender.size()-1):users.get(users.size()-2)[source]}, new boolean[]{false, true}).get(0)[0]);
    oldposext.add(new Integer[]{currin, position});
    currin = (source == -2)?currin:top().intValue();
    currclassname.add((source == -2)?extender.get(extender.size()-2):users.get(users.size()-2)[source]);
    position = 0;
    if (source != -2) rmtop();
    while (position < currin().length && !ending) {
        if (!looping.get(looping.size()-1) && position > currin().length - 1) {
            break;
        }
        else if (!currin()[position].equals(""))
            opHandle();
        position = (direction) ? (position + 1): (position - 1 >= 0) ? position - 1: currin().length-1;
    }
    instruct.remove(instruct.size()-1);
    currin = oldposext.get(oldposext.size()-1)[0].intValue();
    position = oldposext.get(oldposext.size()-1)[1].intValue();
    oldposext.remove(oldposext.size()-1);
    currclassname.remove(currclassname.size()-1);
    direction = olddir.get(olddir.size()-1);
    olddir.remove(olddir.size()-1);
    ending = false;
}

Relevant variable declarations:

private static boolean direction = true;

private static int position = 0;

private static ArrayList<Integer[]> oldpos = new ArrayList(0);

private static ArrayList<ArrayList<String[]>> instruct = new ArrayList(0);

private static ArrayList<Integer[]> oldposext = new ArrayList(0);

private static ArrayList<String> extender = new ArrayList(0);

private static ArrayList<String[]> users = new ArrayList(0);

private static ArrayList<String> currclassname = new ArrayList(0);

private static ArrayList<Boolean> olddir = new ArrayList(0);

private static boolean ending = false;

private static int currin = 0;

This is very spaghetti-like code, and is part of a major Vitsy reworking. The main problems I seek assistance in is 1) adhering to convention (which I certainly haven't), 2) attempting to increase the efficiency of the class loading and method calling, and 3) general tips on how to think about these concepts in the future.

Example usage in Vitsy:

Let's say I have two files, Vitsy and Hello:

Vitsy

iG 00k Z

;u Hello

Hello

" olleH"

In the program Vitsy, I get the name of the current class (i pushes -1 when there is no string input through CLAs and G gets the name of the referenced class, -1 referring to itself). Then, I call the 0th line index of the 0th index of the use declaration (;u Hello, meaning that I call the first line in the 1st use declaration, the file "Hello"). This means that I now have "Hello Vitsy" on the stack. I now use Z in the file Vitsy to output everything in the stack.

However, this simple task is seriously inefficient as of now, as, during these steps, I have to load the "Hello" file, execute the line of code I seek (saving all the information about where I was and stuff from the first program), unload the program, and continue. This would be a problem if I had to, say, call many different classes, and would take far longer than should be necessary.

\$\endgroup\$
  • \$\begingroup\$ And I posted this way too late. x.x It is quite late for me, so if I don't respond immediately, do not be alarmed - I'm around. \$\endgroup\$ – Addison Crump Feb 10 '16 at 23:04
  • \$\begingroup\$ Welcome to Code Review! Great job on your first question! \$\endgroup\$ – SirPython Feb 10 '16 at 23:26
  • \$\begingroup\$ Welcome to Code Review. I've replaced your artificially excessively commented code with the real code in GitHub, so that we may do a realistic review. \$\endgroup\$ – 200_success Feb 10 '16 at 23:55
  • \$\begingroup\$ @200_success Thank you! I'll remember that for my future posts, sorry! \$\endgroup\$ – Addison Crump Feb 11 '16 at 0:04
  • \$\begingroup\$ @Heslacher I added all the used variables' declarations to the question. c: \$\endgroup\$ – Addison Crump Feb 11 '16 at 8:23
2
\$\begingroup\$

Yeah this looks like golfed code :)

It's never a pleasure to review spaghetti code. So I won't give a comprehensive review, with luck somebody else will, or we can each improve some bits and pieces. So here are a few suggestions to get you started.

Spacing

Use more whitespace in general. Not excessively, but more. Paste your code in an IDE, and use the auto-reformat feature. It will add spaces around operators, and the code will become easier to read.

Then add some vertical space. This means empty lines, to separate chunks of code that are only loosely related, and keep lines together that are closely related. Each group of lines (not separated by empty lines in between) should form a cohesive unit, doing some logical step.

As a next step, add a comment above each group of lines to explain the logical step they accomplish.

As a next step, extract reach group of lines to a helper method, converting the comment to a method name.

Other specific tips

Given this:

instruct.add(FileHandler.getFileInstruct(new String[]{(source == -2)?extender.get(extender.size()-1):users.get(users.size()-1)[source]}, new boolean[]{false, false}));
users.add(FileHandler.getFileInstruct(new String[]{(source == -2)?extender.get(extender.size()-1):users.get(users.size()-1)[source]}, new boolean[]{true, false}).get(0));
extender.add(FileHandler.getFileInstruct(new String[]{(source == -2)?extender.get(extender.size()-1):users.get(users.size()-2)[source]}, new boolean[]{false, true}).get(0)[0]);
oldposext.add(new Integer[]{currin, position});
currin = (source == -2)?currin:top().intValue();
currclassname.add((source == -2)?extender.get(extender.size()-2):users.get(users.size()-2)[source]);

If I'm looking at this right (it's hard to see in the jumble), all these lines depend on the condition source == -2. Not only it's a waste to evaluate that multiple times unnecessarily, this is very hard to read.

Instead of repeating an ugly ternary on all these lines, rewrite as a proper if-else.

Refactoring spaghetti code

It's ok to make small steps that may seem insignificant at first. As the code gets cleaner bit by bit, more and more improvement opportunities become clear, gradually leading to more substantial improvements.

My tips so far don't change the logic of your code. When you start changing the logic, later, as you make progress with cleaning, how will you know you're not breaking something? You can know (almost), by writing unit tests first, covering all execution paths. After that you can go on and refactor boldly yet fairly safely, re-running the unit tests after every refactoring step. Without automated tests, verifying the behavior using manual testing will be a nightmare.

Martin Fowler has a famous book on refactoring techniques. That should help a lot.

\$\endgroup\$
  • \$\begingroup\$ Should I update my question with the changes? \$\endgroup\$ – Addison Crump Feb 11 '16 at 8:33
  • \$\begingroup\$ Also, this code could be much more golfed - I tried to avoid golfing it as much as possible. ;D I have a tendency to golf my code by accident, as I golf in Java, Vitsy, AppleScript and MineCraft frequently. \$\endgroup\$ – Addison Crump Feb 11 '16 at 8:41
  • \$\begingroup\$ Understood, no worries. No, don't update the code in the question, that would invalidate my answer. Wait a bit for further suggestions by other users. A few days later, you may want to post a new question with your revised code. This is how we do it around here and it works well, but it may require a bit of patience. \$\endgroup\$ – janos Feb 11 '16 at 9:15
  • \$\begingroup\$ thumbs up Well, post-modification looks much better already. :D Thanks. \$\endgroup\$ – Addison Crump Feb 11 '16 at 9:17

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.