Dh code review #1

maltevogl · 2024-04-29T12:38:08Z

The ticket for this code review is: Issue 8

This repository is ready for review

…graph

raffazizzi

General comments

The application works as intended and the code is readable and easy to understand and navigate (though there is a fair bit of commented out code here in and there that should be cleaned up). However, the code can be substantially optimized, but it will require some strategizing and potentially substantial refactoring. If time /resources are short, the code as is will be relatively safe, but I have some concerns about accessibility in terms of compatibility with even not-so-old browsers, and potential interaction with hard-to-predict user browser configurations and extensions. The comments throughout the code offer some potential solutions.

The JavaScript code would be best handled as a module (e.g. via Node) with a dedicated package.json. This will provide several advantages:

you will have a centralized place to manage dependencies, including tools to notify you of security concerns and mechanisms for updating things manually
you will have access to code bundling tools (such as rollup). This will take care of things like minimization to reduce the size of the code you send to clients, and tree-shaking to get rid of unnecessary code from libraries.
code bundling will also help with avoiding polluting the global namespace, ensuring that your code can be used in other applications without conflict (see my comments on the code, starting from visDynamic.html)
your pages will only need to import fewer scripts (likely just one), reducing HTTP requests
if you chose to test your JS code, you will be able to able to access testing libraries that can run on Node, such as Mocha.

Interface and accessibility

This isn't the main focus of this review, so these are just some general observations as feedback.

For accessibility and ease of use it would be good to include keyboard controls to navigate the visualization.
Some menu buttons cannot be reached via tabbing, limiting interaction to mouse users only
The hamburger menu on "Fields of research" is hard to see

raffazizzi · 2024-07-18T15:47:22Z

src/citationnet/templates/visDynamic.html

+  import { CitationNet } from "{{ url_for('static', filename='force-citationNet.js') }}";
+
+  const jsondata = await '{{ jsondata|tojson }}';
+  window.net = new CitationNet(jsondata);


It's generally not a good idea to add anything to the global namespace (including the window object) as this will reduce code re-usability. It may seem unnecessary if your code is strongly coupled with this specific page and plan to vet the libraries you include in order to avoid conflict. However, it also risks messing with people's browser extensions, which is unpredictable (just in case some extension does create a "net" global object, for example).

Doing this may need a fair bit of refactoring however. One challenge will be handling click events: they are better handled within the JavaScript code, by adding event listeners. For example:

// Attach event listener to the button document.querySelector('#rngLayoutRadius button').addEventListener('click', readLayout);

Code bundlers like rollup can help with these operations.

raffazizzi · 2024-07-18T15:47:41Z

src/citationnet/templates/visDynamic.html

+  import { CitationNet } from "{{ url_for('static', filename='force-citationNet.js') }}";
+
+  const jsondata = await '{{ jsondata|tojson }}';
+  window.net = new CitationNet(jsondata);


It's generally not a good idea to add anything to the global namespace (including the window object) as this will reduce code re-usability. It may seem unnecessary if your code is strongly coupled with this specific page and you plan to vet the libraries you include in order to avoid conflict. However, it also risks messing with people's browser extensions, which is unpredictable (just in case some extension does create a "net" global object, for example).

Doing this may need a fair bit of refactoring however. One challenge will be handling click events: they are better handled within the JavaScript code, by adding event listeners. For example:

// Attach event listener to the button document.querySelector('#rngLayoutRadius button').addEventListener('click', readLayout);

Code bundlers like rollup can help with these operations.

raffazizzi · 2024-07-18T15:51:11Z

src/citationnet/templates/visDynamic.html

+  document.getElementById("sidebar").style.width = "300px";
+
+  window.adaptWindowSize = () => {
+    var navHeight = document.getElementsByClassName("navbar")[0].scrollHeight;


Instead of var, it's safer to use const for variables that do not need to be updated (constants) and let for most other variables, unless you need to make them available outside of their immediate scope.

raffazizzi · 2024-07-18T15:56:08Z

src/citationnet/templates/visDynamic.html

+  </div>
+</div>
+
+<script type="module">


While this is totally fine and in line with modern practices, many older browsers will not know what to do. Consulting "Can I Use," is a good way of getting a sense of which environments will support your code. If you want to make sure your application is more accessible, consider using tools to compile your code based on a target browser that can work as a common denominator. Babel is the right tool for this and it can be integrated with rollup (mentioned elsewhere).

raffazizzi · 2024-07-18T16:11:38Z

src/citationnet/static/force-citationNet.js

+        try {
+            if (!(jsondata)) jsondata = jsondata;
+        } catch (error) {
+            alert("no JSON containing citation data specified, graph cannot be displayed");


I strongly recommend not to use alert(). It blocks the browser and the javascript thread, forcing the user to interact to continue doing anything else at all. Some popup blockers may block it anyway. Instead, consider showing a UI component with this information. Since you're using Bootstrap, consider using an alert or a toast. You can generate the HTML dynamically and append it to the DOM (and remove it when dismissed or after a timeout). If that sounds too complicated, you may also consider setting a property to the class and have the code in visDynamic.html monitor the property to react accordingly (e.g. by showing some text in the main <div>...)

raffazizzi · 2024-07-18T16:47:45Z

src/citationnet/static/pieChart_vega-lite.js

+}
+
+async function loadFOR() {
+    const stats = await window.net.getStats();


In order to avoid polluting the global namespace, I would recommend passing net as a parameter instead

raffazizzi · 2024-07-18T16:49:08Z

src/citationnet/templates/visDynamic.html

+
+  window.onresize = window.adaptWindowSize;
+
+  await showFOR();


Instead of relying on window, pass net to this function to make it accessible from loadFOR in pieChart_vega-lite.js

raffazizzi · 2024-07-18T16:49:51Z

src/citationnet/static/pieChart_vega-lite.js

+
+    console.log(vegaLiteSpec);
+
+    vegaEmbed('#forChartDivvis', vegaLiteSpec);


Consider attaching the click events here in order to avoid polluting the global scope.

raffazizzi · 2024-07-18T16:50:19Z

src/citationnet/static/pieChart_vega-lite.js

+    forChartDivloader.hidden = true;
+}
+
+function hideFOR() {


If you end up attaching click events in loadFOR, make sure to remove them here.

raffazizzi · 2024-07-18T16:53:07Z

src/citationnet/static/pieChart_vega-lite.js

+        document.getElementById(elmnt.id + "header").onmousedown = dragMouseDown;
+    } else {
+        /* otherwise, move the DIV from anywhere inside the DIV:*/
+        elmnt.onmousedown = dragMouseDown;


This is fine, but I'd recommend using addEventListener instead for more control and reusability of the handler (as well as being able to detach it properly if/when needed!)

Just thought that you may also want to consider using the same handler to add a touch event listener (though I haven't tested the application with a touch screen, so I don't know to what extent it's worth supporting touch).

rlskoeser

As I think you expected from our initial conversation, there's a lot of cleanup that can be done here, and hopefully in our wrap-up conversation we can help you gauge what is most applicable and highest priority and impact.

@raffazizzi has already touched on the important javascript handling and embedding aspects, which I agree with. If you don't want to make the js a full-blown reusable package, at least integrating webpack into your flask should get you some good improvement in terms of compiling and minimizing the js code so you only have one file to include.

In general, the main higher-level problems I see are along the lines of encapsulation - there are connections both ways between html, force graph code, and pie chart code. I would focus on making the html more semantic (e.g., radio buttons and checkboxes for controls) and move the event handlers and logic bindings into the javascript. The pie chart code should not need to know about or interact with the controls or the force network code, but just take a containing element for display and a set of statistics to render.

Two suggestions on the UI of the application based on experimenting with it as I worked through the code:

Display menu by default if screen is large enough
Provide some sort of warning before linking out to the article, or an alternate display? that wasn't the behavior I expected in the context (I thought I would be interacting with the graph). Maybe a future enhancement could be to load the article or some preview version in a card on the page.

rlskoeser · 2024-07-19T14:22:55Z

src/citationnet/static/force-citationNet.js

+                if (node.attributes.nodeyear >= this.inputNode.attributes.nodeyear) {
+                    node.fz = 5 * (node.attributes.nodeyear - this.inputNode.attributes.nodeyear);
+                } else {
+                    node.fz = 5 * (node.attributes.nodeyear - this.inputNode.attributes.nodeyear);
+                }


This conditional seems unnecessary, since the logic is the same either way.

rlskoeser · 2024-07-19T14:24:05Z

src/citationnet/static/force-citationNet.js

+                if (node.attributes.nodeyear >= this.inputNode.attributes.nodeyear) {
+                    node.fz = 5 * (node.attributes.nodeyear - this.inputNode.attributes.nodeyear + 20);
+                } else {
+                    node.fz = 5 * (node.attributes.nodeyear - this.inputNode.attributes.nodeyear - 20);
+                }


Ah, maybe the earlier conditional was based on this one and the adjustment got dropped.

Given that the logic is nearly the same I would collapse the larger if/else and add an adjustment amount that is +/-20 or +/- 0 depending on whether the spacing is enabled.

rlskoeser · 2024-07-19T14:29:25Z

src/citationnet/static/force-citationNet.js

+    constructor(jsondata = null) {
+        // if jsonPath is not provided try to get globJsonPath, show alert if fails
+        try {
+            if (!(jsondata)) jsondata = jsondata;


The comment indicates a global fallback, but I'm not sure it's working as described. If you need a global fallback it should have a different variable name and be clearly described/defined before this class.

Looking at the calling code in the html, I think if the passed in data doesn't work then the global won't either.

rlskoeser · 2024-07-19T14:41:06Z

src/citationnet/static/pieChart_vega-lite.js

+const forChartDiv = document.getElementById("forChartDiv");
+const forChartDivvis = document.getElementById("forChartDivvis");
+const forChartDivloader = document.getElementById("forChartDivloader");


Given how tightly coupled this is to the html structure in your template, it would be cleaner and more maintainable if you passed in a container element and constructed the other elements using createElement when the chart is first initialized.

There's a fair bit of code for making this element draggable, but I'm not sure how important that is - it wasn't obvious to me in the UI that it was draggable, I only found it after looking at the code and experimenting. You might consider whether that is actually necessary, or ask some users if they find themselves using that feature. If it's not essential you could simplify the code quite a bit by removing it.

Consider refactoring into a class.

rlskoeser · 2024-07-19T14:42:48Z

src/citationnet/static/pieChart_vega-lite.js

+const forChartDiv = document.getElementById("forChartDiv");
+const forChartDivvis = document.getElementById("forChartDivvis");
+const forChartDivloader = document.getElementById("forChartDivloader");


Would be cleaner to group into an object, e.g.

Suggested change

const forChartDiv = document.getElementById("forChartDiv");

const forChartDivvis = document.getElementById("forChartDivvis");

const forChartDivloader = document.getElementById("forChartDivloader");

const chart = {

container: document.getElementById("forChartDiv"),

vis: document.getElementById("forChartDivvis"),

loader: document.getElementById("forChartDivloader")

};

Then you can reference container.chart etc.

Also recommend more semantic names (container vs div), and maybe loader should be loading.

rlskoeser · 2024-07-19T15:44:38Z

src/citationnet/static/force-citationNet.js

+        if (outerValue == 0) {
+            this.graph.d3Force('radialOuter', d3.forceRadial(radius).strength(1.0));
+        } else {
+            this.graph.d3Force('radialOuter', d3.forceRadial(radius).strength(CitationNet.strengthFuncFactory(0.0, 1.0, 0, outerValue)));
+        }


This another place you can consolidate mostly redundant code to make it easier to understand what changes based on the conditional, e.g.

let radialForceStrength = 1.0 if (outerValue) { radialForceStrength = CitationNet.strengthFuncFactory(0.0, 1.0, 0, outerValue); } this.graph.d3Force('radialOuter', d3.forceRadial(radius).strength(radialForceStrength);

rlskoeser · 2024-07-19T15:45:48Z

src/citationnet/static/force-citationNet.js

+              a.outgoingLinks.push(edge);
+
+              !a.outgoingLinkTo && (a.outgoingLinkTo = []);
+              a.outgoingLinkTo.push(b);
+
+              !b.incomingLinks && (b.incomingLinks = []);
+              b.incomingLinks.push(edge);
+
+              !b.incomingLinkFrom && (b.incomingLinkFrom = []);
+              b.incomingLinkFrom.push(a);
+


Do you need to store links and references in both directions? I wonder if there is a better data structure that would let you avoid the redundancy here and still do whatever calculations or operations you need to do.

I couldn't find where else these are referenced so I'm not sure what they are for.

rlskoeser · 2024-07-19T15:46:17Z

src/citationnet/static/force-citationNet.js

+              delete edge.color;
+              delete edge.size;


I don't see this in your example data, maybe the code is unneeded?

rlskoeser · 2024-07-19T15:59:56Z

src/citationnet/static/force-citationNet.js

+    * Constructs a new CitationNet object, but does not initialize it. Call object.initialize() right after this.
+    * @param {String} jsondata - path or url to citation data as JSON file/stream
+    */
+    constructor(jsondata = null) {


The comments describe jsondata as path/url to the JSON data, and the method above is called fetchJSON, but the use of |tojson in the flask template and from inspecting the running version (which seems slightly different), it looks like you are passing json data as a string. Based on some commented out code I suspect that an earlier implementation used a url and a fetch call.

It would help to more clearly name the variables and methods to accurately reflect this, and perhaps distinguish between the json data as string and as object.

rlskoeser · 2024-07-19T16:01:20Z

src/citationnet/static/force-citationNet.js

+* @param {String} path - path or url to JSON file
+* @returns {object} parsed JSON object
+*/
+async function fetchJSON(data) {


This is only used once, not sure it merits a separate function.

maltevogl · 2024-07-25T14:38:12Z

Thanks a lot @raffazizzi and @rlskoeser for your extensive and valuable comments. The meetup at the end of the code review helped me a lot to understand the context of your comments.

Based on your input it seems the most sustainable approach to cleaning up the code is to actually restart from scratch and develop a standalone js package. This way I can better address re-usability and namespace pollution and so on, and test the critical logic parts of the code for the network creation.

Have a nice week and see you another time 👋 💙

maltevogl added 24 commits March 15, 2022 17:15

init com

1c9e232

wip fix template install, todo: clean template syntax

af12688

wip add new vs

6f3718d

wip

65f19d2

wip not yet reading json

5d25a17

wip add startpage, autocomplete

f9933ac

wip working 3d graph

c910861

start post to startpage

0482ba2

wip working version, minor fixes needed, especially FOR colors of 3d …

774abe7

…graph

sync colors of pie chart and 3d graph

a6e2d8c

working version, TODO fix ret val fpr failing login

8b8a669

wip add orig

8d6ab6d

wip add check on edges source and target

fded794

upt org

8d57355

add rtd, tests, tox, docs

b843ae8

fix wording

7a2fa5d

fix index

8dd7b75

bump version

d97344a

change build backend

9ab3534

cleanup after changing to pyprojecttoml setup

e6c8b40

add citation / updt readme

e5e271c

fix empty key

defc89e

linting

79cc648

Merge branch 'main' into dh-code-review

4f3d397

jdamerow requested review from rlskoeser and raffazizzi July 2, 2024 13:02

raffazizzi reviewed Jul 18, 2024

View reviewed changes

rlskoeser reviewed Jul 19, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dh code review #1

Dh code review #1

maltevogl commented Apr 29, 2024

raffazizzi left a comment

raffazizzi Jul 18, 2024

raffazizzi Jul 18, 2024

raffazizzi Jul 18, 2024

raffazizzi Jul 18, 2024

raffazizzi Jul 18, 2024

raffazizzi Jul 18, 2024

raffazizzi Jul 18, 2024

raffazizzi Jul 18, 2024

raffazizzi Jul 18, 2024

raffazizzi Jul 18, 2024

raffazizzi Jul 18, 2024

rlskoeser left a comment

rlskoeser Jul 19, 2024

rlskoeser Jul 19, 2024

rlskoeser Jul 19, 2024

rlskoeser Jul 19, 2024

rlskoeser Jul 19, 2024

rlskoeser Jul 19, 2024

rlskoeser Jul 19, 2024

rlskoeser Jul 19, 2024

rlskoeser Jul 19, 2024

rlskoeser Jul 19, 2024

maltevogl commented Jul 25, 2024


		console.log(vegaLiteSpec);

		vegaEmbed('#forChartDivvis', vegaLiteSpec);

-const forChartDiv = document.getElementById("forChartDiv");
-const forChartDivvis = document.getElementById("forChartDivvis");
-const forChartDivloader = document.getElementById("forChartDivloader");
+const chart = {
+  container: document.getElementById("forChartDiv"),
+  vis: document.getElementById("forChartDivvis"),
+  loader: document.getElementById("forChartDivloader")
+};

Dh code review #1

Are you sure you want to change the base?

Dh code review #1

Conversation

maltevogl commented Apr 29, 2024

raffazizzi left a comment

Choose a reason for hiding this comment

General comments

Interface and accessibility

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rlskoeser left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maltevogl commented Jul 25, 2024