Feature/converter

[DennisAng]

Implemented converter from issue #28 as a "import" feature in the new Hypedyn.

[benedictleejh]

Nice pull request. A few issues though:

• All code should be formatted according to style guidelines, See the style guide for details.
• Related to the code style, all doc-style comments should follow JavaDoc style, i.e.

/**
 *

instead of ScalaDoc style of

/**
  *

• What is the point of the MoveNode event? That exposes the implementation details of the story viewer. The original idea was that the story viewer need not be implemented as a graph viewer; having a move node event leaks that the story viewer must be a graph viewer. Has that changed?
• The original file dialog can easily be reused to fit the needs of a file import dialog. The legacy file dialog is unnecessary code duplication.
• You will need to add a dependency to core to use the Scala parser combinators library, as it isn't part of the Scala standard library. As it is, I'm not quite sure the code even compiles (on a fresh clone). On that note, I suggest taking a look at FastParse as the Scala parser combinators library is no longer being actively developed.
• Do not use null. The only valid reason for using null is for interfacing with Java code, and even then, wrap in Option if possible. The parser interfaces with no Java code whatsoever, and hence does not need null.
• Minimise the use of vars and mutable data structures. Especially do not have a mutable data structure in a var. Avoid global state whenever possible.

These are the most problematic issues at the moment. I'll do a more in-depth code review of the parser later.

[DennisAng]

2c4baa9

• Modified ScalaDoc style comments to JavaDoc style
• Removed MoveNode event and dependencies. Nothing has changed, earlier implementation was done as an unobtrusive proof of concept
• Switched from Scala parser combinators library to Fast Parse library
• Removed all occurrences of null, wrapped with Option where applicable.

f3148cb

• Minimise use of vars and mutable data structures.
• Remove all declarations of mutable data structures as var

[benedictleejh]

I've taken a closer look at your revised parser, and it's still not ready for merging. I'll explain as much as I can about what's wrong with the code, and some of the philosophy behind how code is written in HypeDyn2 (mostly applies to core and api, UI related things have to deal with Java, and thus can't be as nice).

By and large, we adopt a functional programming style in HypeDyn. This is a very vague term, understandably, but at least, in this context, it means no or absolutely minimal mutation, and proper use of higher order functions. I'll cover these points before moving on to more specific points on style.

There are exactly 6 vars in core, excluding your parser. There are in fact more 3 times the number vars in your parser than in the rest of core, and almost as many as the whole project. Your use of vars is absolutely unnecessary, as far as I can tell, because everything in Scala is an expression, which means there is no need to mutate vars to store results; expressions can be directly assigned to vals thus eliminating the use of vars. Excessive use of mutation is an imperative programming thing, which we want to avoid to improve the long-term maintainability of the code.

Speaking of imperative programming style, your use of that style causes your code to be very deeply indented, and structured into large blocks, making it very hard to read and understand. This reduces code maintainability, because it is a lot to keep track of while reading code. Your list processing style is also very steeped in imperative style, using iterators instead of more natural map, filter, or even pattern matching.

Functional programming also emphasises the use of functions to transform data. We specify how we want data transformed (through higher-order functions like map,filter, etc.), instead of looping over data structures manually. This especially applies to parser combinators, which take a functional approach to parsing. Simple parser combinators are meant to be combined in order to parse more complex data structures, using map to transform it into an appropriate type. This means that parser combinators should be combined in such a way as to reflect the data structure being parsed, and simply calling the right parser can return the data structure you need, instead of having to process its result further.

Related to this is how you call higher-order functions in your code. Please take another look at the style guide for how to call higher-order functions. Functions that have a symbolic name are considered operators, and are expected to be invoked in the way operators are expected to be invoked.

The remaining points I shall leave in point form, to reduce the length of this post.

• Use of Any. Any is the top type of the type hierarchy in Scala, and its use should be avoided as you end up losing type safety because of it.
• On a related note, there is excessive use of type annotations to upcast values. Upcasting is unnecessary within a function, its value is never leaked to the outside; upcasting is meant to provide a more general interface between components, and is generally pointless within one.
• lift returns an Option, and consequently, .get must not be called on its result.
• The argument to CharsWhile is extremely non-idiomatic Scala, and amounts to merely massaging the argument to pass the type checker. This is unacceptable. See this for how to use CharsWhile correctly.
• Also related to code style, please take a closer look at the style guide again on how to create empty collections. val map: Map[Int, String] = Map() is not an acceptable way to get a new empty Map.