search

orazdow / SAR

0 stars 1 forks source link

Initial Feedback on Codebase #1

Open marques-work opened 4 years ago

marques-work commented 4 years ago

Summary

While this codebase is sort of a prototype rather than code targeted for production, there are a few improvements that would still improve the overall quality and maintainability.

  1. This codebase can most benefit from being split into modules by component class/responsibility
  2. Is the text-to-speech code from a library? If not, we can certainly leverage an existing TTS lib so we maintain less code ourselves. Perhaps https://github.com/tom-s/speak-tts ?
  3. Traversing and Reading classes seem to call each other in a way where they know too much about the implementation of the other. I'd say the separation of concerns, i.e., the abstraction boundaries, may not be ideal. Refactoring them to be truly independent of each other, but composable together leads to more resilient design.
  4. P5 and TTS lib ought to be included in package.json and imported via modules using the packager
  5. Use DOMContentLoaded event rather than loading scripts at the bottom of markup
  6. Extract magic numbers and such as constants for readability
  7. Certain complex pieces might benefit from testing? Again, I recognize this is not aimed at production, however, it is still a good idea to write some tests around critical parts to ensure they don't inadvertently break. Just some minimal coverage around core parts. Perhaps the code around auto traversing and speech?
  8. Extract common code for Tree + Branch to share between tree and vine experiments? If these experiments aren't maintained any further, it may be better to move them out of the repo?

Other suggestions

  1. Use Webpack?
  2. TypeScript? Not absolutely necessary, but I feel that if we take advantage of TS language features to explicitly define interfaces can help us think about problems more abstractly and keep solutions consistent. The dev toolchain is pretty nice too.

Tracking progress

I'll be working on some PRs to build out some of these enhancements. This issue can be the place to track the progress of this refactoring work.

orazdow commented 4 years ago

Hi thanks for getting in touch.

I've been switching focus to the "graphics engine" side of the project so I haven't been updating the web app for a bit. There are some things I want to add to it though. Looking at the points you raised, there are a couple of things I could add:

For things involving the p5 experiments, those two folders were more to test animation ideas for something that might be integrated later but It's really in the repo because it was an easy github-pages url to post to slack, so it and any p5 stuff is not really a component of the project yet at least.

The TTS is from the built in web api, I'm not sure on the pros and cons of using a library, I think the one you linked just wraps the same api so the native one seems better to me at least. I did mean to set the voice parameter but didn't get around to it. It defaults to browser defaults now, so it's female in firefox vs male in chrome for example.

As far as the general architecture I'd agree generally, but I've been pretty agnostic on that front since it's so tailored one website. I did notice some issues though in the behavior between chrome and firefox so something might not be translating in the way the two browsers handle event, I'm not sure, but that leads to the following. Here are the list of things I had to do / things to fix that I've been putting off while I get into OpenGL:

Issues: 1) The behavior upon clicking nodes with traverse and speech checked does not work in firefox as it does in chrome, which is correct. 2) styling is a bit iffy, Probably not consistent depending on devicepixelratio, people can mess up the layout by zooming in the wrong part of the screen or with touch gestures, etc. 3) The TTS voice parameters are left to default

features: 1) Add a "follow" checkbox to translate the selected node to the center of the view while it traverses. 2) "loop" checkbox to return to the root node after hitting a terminal node (that isn't a portal) 3) implement the "portal" instruction, this will probably involve just adding graphics to the tree svg, as the d3 tree plugin probably doesn't support dag-like graphs. 4) try putting some p5 animations behind the tree svg

For the other suggestions, I'm not such a fan of webpack personally. I also think switching to typescript might be to involved a change for this part of the project.

marques-work commented 4 years ago

@orazdow Thanks for replying so quickly.

For the features you listed, we can probably open an issue for each to implement, just to keep track.

I'll leave the p5 experiments alone then as it sounds like we have yet to really use them in the main project. When we do, we can integrate them as part of build process.

Regarding the TTS library - certainly, any tts lib on the browser will need to wrap the native API. The main reasons to use a library are consistency and compatibility cross-browser. The main README on the library I linked indicates that chrome loads voices differently from other browsers (async vs sync). The library intends to provide a useful es6 Promise interface, as well as take care of making events and other miscellanies consistent in modern browsers that support speech synthesis (IIRC, it names Firefox, Chrome, iOS Safari, and Android Chromium). Another thing that might be nice is that the library splits sentences so the utterances sound more natural. Now, I haven't actually used the library (or the native speech synth) myself, but these might be compelling reasons to consider choosing it over the native -- it would mean you don't need to implement these enhancements yourself. Of course, if none of this is concerning to you (e.g., you only care about supporting Chrome and nothing else), then maybe the library isn't necessary. Just something to think about.

We can stick with browserify -- it's a perfectly fine packager/bundler. I only suggested webpack because it includes a lot of functionality OOTB (e.g., tree-shaking dependency graph optimization) whereas browserify relies heavily on plugins (and thus, extra packages/dependencies) for its core benefits. Since you are more comfortable with browserify, let's stick to that.

I'll first work on splitting out/refactoring the components into separate files and possibly simplifying or splitting classes. Since that will require refactoring anyway, I might see if it's a nominal effort to port to TypeScript as I might be breaking up logic anyway. If it adds a lot of work (or if you are really bothered by TypeScript or otherwise opposed), I will probably just stick to es6 (though TypeScript is really just an es6 superset anyway).

WDYT?

orazdow commented 4 years ago

Hi, sorry for the delay.

I think for the time being it's actually best to hold off from merging architectural or refactoring changes. Since I'm on a tight time budget to get various things on my checklist done, integrating those changes now and familiarizing myself with possible architectural updates like typescript wouldn't be the best thing at this time for my workflow, especially as I bounce between the two elements of the project for me it would be better to hold those things static while I work on the various functional parts of the checklist, and do a pass of larger architectural or compatibility/safety/refactoring once it's a bit further along.

I think keeping a parallel branch where I can merge larger chunks of commits without constraints and then we can integrate later in the month. One of the reasons I decided to go back to merging over collaborator access was just because I want to hold off of any possible larger changes to the master branch while I am in this faze of development, so I think freely updating a dev branch would be a good policy for now.