patternjavaMinor
Implementing Multi-File Programs in Vitsy
Viewed 0 times
filemultiprogramsvitsyimplementing
Problem
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 - 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}
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 - 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}
Solution
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:
If I'm looking at this right (it's hard to see in the jumble), all these lines depend on the condition
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.
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.
Code Snippets
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]);Context
StackExchange Code Review Q#119561, answer score: 2
Revisions (0)
No revisions yet.