2.0.0 Redesign branch discussion

[Col-E]

2.0.0 - Redesign

In this branch Recaf is being rewritten from the ground up. The main goals can be found on the 2.0.0 project board.

I'd like to hear any thoughts on what should go into this redesign. What functionality should be factored in to the core design? What use cases would you like to see covered?

Feel free to respond here or create new issues on this topic, I'd like to hear your opinions.

---

Temporary release binary 01/31/2020: recaf-2.0.0.jar.zip (remove .zip to run)

[andylizi]

Use proper exception handling

Recaf/src/main/java/me/coley/recaf/util/Reflect.java

Lines 26 to 29 in 257783d

	} catch (Exception e) {
	Logging.fatal(e);
	return null;
	}

Recaf/src/main/java/me/coley/recaf/util/Parse.java

Lines 18 to 23 in 257783d

	try {
	Integer.parseInt(s);
	return true;
	} catch (Exception e) {
	return false;
	}

Recaf/src/main/java/me/coley/recaf/ui/FxAssembler.java

Lines 378 to 381 in 257783d

	} catch(Exception e) {
	// If we fail, don't suggest anything
	return null;
	}

1. Pass exceptions to a proper handler using throws, don't just log it on the spot and do nothing. return null isn't the answer to everything either.
2. Prefer specific exceptions instead of catching generic Exceptions and Throwables everywhere.
3. Only ignore exceptions when you're 100% sure about how it would happen and it really is a good idea. And never silently swallow Exception or Throwable without anything indicates it happened or later it'll be extremely confusing to debug anything in it for anyone except you (and even that's true only if you hadn't forget that there's a catch-all-and-ignore here...)

---

Fix confusing and inconsistent utility classes

(Utility class as in "a stateless class or enum that's used to group several utility methods together and cannot be instantiated", not every class under the util package)

1. Most of them are under me.coley.recaf.util package, but some of them are not. For example me.coley.recaf.bytecode.TypeUtil. And I guess AccessFlag counts as a utility class too?
2. Some of their names end with "Util" but some of them are not. For example ScreenUtil and Lang. This causes difficulty deciding which class is designed to used as an instance (e.g. Pair and RollingList), and which class is designed to act as a group for static methods (e.g. Classpath and Clipboard).
3. The grouping of utility methods are confusing. It's hard to decide what method should goes into what utility class. Can't find a good example for this but some of them definitely feel inconsistent.

---

Use method references when applicable

Recaf/src/main/java/me/coley/recaf/ui/FxWindow.java

Lines 52 to 56 in 257783d

	Runnable rSearch = () -> FxSearch.open();
	Runnable rConfig = () -> FxConfig.open();
	Runnable rHistory = () -> FxHistory.open();
	Runnable rAttach = () -> FxAttach.open();
	Runnable rAbout = () -> FxAbout.open();

Pros:

1. Cleaner-looking code.
2. javac will generate a synthetic bridge method for every lambda expression regardless of whether it can be folded to a method reference (therefor skipping the redundant bridge method). Manually fold it can result in:
   • lambda execution being (a little) faster (at least before JIT kicks in, then it should be the same)
   • reduce a tiny bit of code size. Recaf uses a LOT of lambdas, so as the saying goes "from small increments comes abundance" (or something like that. Not a native speaker.)

Cons:

1. Sometimes a method reference could be less direct and harder to read than lambda expression. But this kind of circumstance is (IMO) rather rare.

Oh, and s -> s can be replaced with Function.identity().

---

Remove empty javadoc tags

Recaf/src/main/java/me/coley/recaf/Input.java

Lines 534 to 536 in 257783d

	* @param className
	* @param classfileBuffer
	* @throws IOException

As the title says.

[Col-E]

Use method references when applicable

👍

---

Fix confusing and inconsistent utility classes

I'll have all utility classes be suffixed by "Util"

For structures I will have them in the util.struct package.

---

Remove empty javadoc tags

Done. It will be enforced by checkstyle. I'm moving it so that it is a required part of the build process rather than an optional check. It'll be based off of the google java specification but with a few changes.

---

Use proper exception handling

👍

That example in parse is intended, as it's purpose is to determine if some input is an int.

[andylizi]

That example in parse is intended

Yeah... that's probably not the best example. What I meant was catching Exception instead of NumberFormatException, but in that particular case it doesn't matter much besides formality anyway.

Perhaps a better example would be

Recaf/src/main/java/me/coley/recaf/parse/assembly/util/LineData.java

Lines 34 to 38 in c211533

	try {
	opcode = OpcodeUtil.nameToOpcode(opText);
	} catch(Exception e) {
	throw new IllegalStateException("Unknown opcode: " + opText);
	}

or

Recaf/src/main/java/me/coley/recaf/util/Lang.java

Lines 47 to 55 in ebab127

	try {
	String file = "resources/lang/" + lang + ".json";
	URL url = Thread.currentThread().getContextClassLoader().getResource(file);
	String jsStr = Streams.toString(url.openStream());
	JsonObject json = Json.parse(jsStr).asObject();
	json.forEach(v -> map.put(v.getName(), v.getValue().asString()));
	} catch (Exception e) {
	throw new RuntimeException(e);
	}

[Col-E]

Oh no worries. I know what you meant. I don't think I have anything like that in 2.0.0 yet. If I do let me know that I'm a dummy.

[artemking4]

Theme customization. And, if possible similar interface to dnspy.

[andylizi]

Suggestion - enforce WhitespaceAround in checkstyle

In the current codebase, whitespaces preceding if, for etc. are very inconsistent. This normally wouldn't be an issue if we were not using VCS (except, of course, looking bad 😏). But with Git recording even the tiniest bit of change, I must be careful about triggering IDE's auto-reformatting when editing existing code, or the eventual commit diff will be spammed with a lot of irrelevant, one-character-pre-line changes.

So I suggest adding the relevant part in Google checkstyle template to our configuration. This won't be too much trouble as corrections can be done by IDE automatically.

[Col-E]

I must be careful about triggering IDE's auto-reformatting when editing existing code

I used to be auto-format trigger-happy but have been training myself not to use it so often. I really hate how it handles streams and some certain line-wrapping cases.

Typically I only highlight specific blocks and auto-format that small portion.

Suggestion - enforce WhitespaceAround in checkstyle

I didn't include it (and a few other settings) so far because I found working on a project with it to be very annoying. Small style differences like this aren't that big of a deal to me so I thought it would just be an annoyance to anyone looking to contribute.

Now things like brace placement, tabs vs spaces, line length, etc. that's different. The potential differences in style for those are significant enough to warrant checkstyle enforcement in my opinion.