Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[bugfix] Fix issue with incorrectly chart data to chart component. #1

Merged
merged 1 commit into from
Sep 12, 2019

Conversation

CWSpear
Copy link
Collaborator

@CWSpear CWSpear commented Sep 12, 2019

Task 1

Please provide a short code review of the base master branch:

  1. What is done well?

The project appears to use nrwl best practices in the directory structure.

Uses ngrx to manage state, even using the newer Facade pattern, which helps a lot when it comes to actually using and testing state-related stuff in the end-components.

I think this more or less comes with the nrwl/nx stuff, but I've found Jest a lot better to work with than Karma+Jasmine, so that goes in the "done well" column.

Uses tslint with codelyzer to help catch errors and keep code consistent, which is nice for large projects.

  1. What would you change?

This is a pretty open ended question... for what it is, this project is massively over-engineered. For a single page application like this, this structure is overkill. So if I was given the original problem statement, my code would look massively differently, as it'd be a much simpler application. But I'm assuming that's not what you're looking for...

But because this is an app that's emulating the beginnings of very large enterprise application, it's hard to try and push for any big changes, since it's generally following a fairly standard pattern in terms of what Nrwl is trying to do.

Some of the prefixes seem unnecessary... if there's gonna be a lot of data-access-, should that not just be in a folder.

Some of the config files could definitely be consolidated. TypeScript will automatically look in the parent folder, and there is a lot of duplicate code that only changes the output, but its in a shared folder, just a different subpath that could be turned into a single config (especially look at all the duplicate code in tsconfig.lib.json!). At least use the extends more efficiently. If you wanted to change a value, the current implementation would be a huge headache.

I'm not super familiar with the base set up of Nrwl, but it seems weird to me that an app would have a route directly to a component in libs... maybe that's just me?

One other thing: Update Angular to take advantage of the import() syntax for loadChildren. That would help a lot with looking for usages.

Oh, and I would definitely use NestJS over Hapi!

  1. Are there any code smells or problematic implementations?

On the more micro scale, there was definitely some issues with passing through data, and perhaps an understanding of how to properly use selectors. This PR includes fixes for correctly passing the data through to the coding-challenge-chart and down to the google-chart component. It gets rid of the leaky subscribe and uses the async pipe to have Angular handle unsubscribing.

The coding-challenge-chart component itself had some smell... it's billed as a "shared" component, but it has hard coded type and column names. I made these inputs

It also had any for the data. I was able to get a more specific type from the Google Chart types, but unfortunately, it still has any for the options :( Unfortunately, that's on Google, tho we could figure our own options interfaces from their docs if we wanted to.

As for smell... it has a Prettier config, but not all the all the files are formatted by Prettier. I didn't add this to this PR, cuz it'd add a bunch of noise. There isn't even an npm command for running Prettier manually. Apparently you can manually run it via nx format:write which is shortcut to npm run format. It would be nice to add it as a pre-commit hook (i.e. via husky) so as to handle it automatically.

Also, no need for ./node_modules/.bin/ in package.json. It's already in the path for npm run commands.

@CWSpear CWSpear merged commit 6f35a2a into master Sep 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant