GoogleParser does the following:

• Managing the state of a flag which ParserService depends on.
• Finding all the pending keywords.
• Creating an asynchronous task to scrape each keyword.
• Performing the actual scraping of the Google page.
• Persisting the scraping results for a keyword.
• Sending messages to the WebSocket clients.

That goes quite a bit against the Single Responsibility principle in OOP 🙈 It would also make this class very hard to test and maintain over time.

Environment

• Platform: Web
• Device: MacBook Pro
• OS Version: Mac OS 11.3
• App Version: main branch
• Environment: local

Steps to Reproduce

1. Boot the application with dev.sh.
2. Navigate with a browser to the URL: https://localhost:5001/.
3. Inspect the browser console.

Expected

There should be no JS errors in the browser console.

Actual

The following errors appear in the browser console:

This is the output from the local terminal:

Environment

• Platform: Web
• Device: MacBook Pro
• OS Version: Mac OS 11.3
• App Version: main branch
• Environment: local

Steps to Reproduce

1. Boot the application with dev.sh.
2. Inspect the terminal output.

Expected

All dependencies should be defined in /keywords/KeywordsApp/ClientApp/package.json to ensure reproducibility of the development and production environments.

Actual

popper.js is missing from the definition of the dependencies:

There are also some missing attribute in package.json which should be referenced.

The project leverage the usage of classes in JS (so not prototype-based approach). However, the classes do seem more like a wrapper of methods instead of an actual object with attributes, public and static attributes.

1. Why not using the construct method to initialize the classes?

This would allow changing the class invocation from:

$(function () {
  new UploadForm().initForm();
});

to:

$(function () {
  new UploadForm();
});

2. Instead of hard-coding the element selector, shouldn't be an argument in the constructor method:

So the end result would be:

$(function () {
  const $form = $('.csv-form');
  new UploadForm($form);
});

This would abide by the composition principle in OOP.

3. initForm (and other initialization methods) are fairly long and procedural.

There are also some repetitions:

This could be resolved by extracting to "private" methods and/or separate objects.

4. Using events instead of method calls to trigger JS functionality.

this.form.change(() => {}) adds an event handler for the change event. However the submit event is not leveraged and a method is called directly with this.submitForm();. Instead the same approach should be used this.form.submit(() => {}) as it can be called with this.form.submit().

Note that using .on('event_type', fn()) is usually preferred for the reasons well summarize on this StackOverflow post.

Since a user needs to be authenticated to perform any action, there are checks in each controller method:

The issues:

• Since this is repeated for every public controller method, wouldn't it better to find a way to DRY this?
• Would it make sense to return an unauthorized error (401) instead of a not found (404)?

Environment

• Platform: Web
• Device: MacBook Pro
• OS Version: Mac OS 11.3
• App Version: main branch
• Environment: local

Steps to Reproduce

1. Boot the application with dev.sh.
2. Navigate with a browser to the URL: https://localhost:5001/.
3. Inspect the homepage

Expected

Some UI elements meant to be hidden are actually displayed.

Actual

This is the actual UI displayed:

I highlighted the areas which are intended to be hidden.

This gets weirder when uploading files 🙈 :

This issue could be caused by #26 🤔 but I am not 100% sure about it yet.

We have a not so clean way to do declarations:

• Extra line after headers.
• We don't group the declarations by type. e.g. We have Stream, long, string, IHeaderDictionary, string, string.

While it's somewhat related to #32, the implementation of async operations can make it hard to reason about the whole parsing logic. There is a complexity level that needs to be addressed.

First, in ParserService, the first level of complexity comes from understanding the purposes for the double Task.Delay:

Then in GoogleParser, there is the usage of await in a foreach loop:

In the end, only one keyword is processed at any given time. Therefore, the level of complexity could be reduced by having ParserService schedule/trigger googleParser.ParseAsync() for only one keyword.

What are your thoughts?

I had problems identifying which is the file naming convention on the migrations. Also, while checking the file content the class names also don't follow a consistent pattern all over. e.g. sometimes it's camelcase, sometimes all lowercase. I'm not a .Net expert, but afaik all classes should be Pascal case.

I'm unsure what's the correct convention in terms of indentation.

Checking multiple files the problem seems to keep happening, sometimes the related block is indented other times it isn't

It appears than JS scripts are added twice:

• First in the head tag.
• Second at the end of the body tag with defer.

What are the reasons for this? Would you see an opportunity for an improvement in the way the third party dependency are added to the page to improve performance?