mentor-review-1.md

Mentor review 1 (Melina, Theresa, Viki, Adrian)

Overall

good project structure, correct use of React / mongoDB / express concepts

backend: JSON body parser as middleware

The JSON body parser could just be included for all routes inside of server.js:

app.use(express.json());

Then you don't need to handle it inside your reparate routers

More efficient database migration

current process:

• go over all comments
  • for every comment, go over all movies if the comment belongs to the movie, insert it into the new database

if there are 100 movies and 1,000 comments, this process would have 100,000 steps

better process:

• go over all movies
  • for every movie, find all corresponding comments insert every comment into the new database

App.jsx: formatting

inconsistent formatting: some indendations are made with tabs, other indentations are made with spaces

App.jsx: state names

suggestions for state entry names:

• classOfMovieList -> isMovieListVisible
• classOfAboutPage -> isAboutPageVisible
• movieInfoComments -> isInfoCommentsSectionVisible

AddNewComment.jsx: state names

names errorMessage and errorMessage2 could be more specific / clearer

Events naming

events of custom components should start with on

counter examples: <Pagination setCurrentPage=... /> <Header homeButtonClick=... aboutButtonClick=... />

mentor-review-2.md

Mentor review 2 (Manuel, Hans, Kata)

Overall

good project structure, correct use of React / mongoDB / express concepts

Filtering by multiple aspects

In the backend, it's not possible to filter by 2 criteria at the same time (e.g. genres and year)

Ideally, it should be possible to provide multiple parameters (e.g. /films?year=1990&genre=action&page=2)

mflixConnection.js

Ideally there should be a separate script file for migrating the database - e.g. migrateDb.js - this should not be part of mflixConnection.js

passing className to custom components (don't)

Components should usually be self-contained and work on their own.

Mostly, it should not be necessary to pass a className property to a custom component - all the styling should be customizable at a higher level.

DON'T:

<Button className="Button-raised Button-primary">foo</Button>

DO:

<Button raised={true} color="primary" size="medium">
  foo
</Button>

overusing custom components

DON'T:

<Container className="detailViewContainer">
  <Container className="title">...</Container>
  <Container className="body">...</Container>
  ...
</Container>

DO:

<div className="detailViewContainer">
  <div className="title">...</div>
  <div className="body">....</div>
  ...
</div>

OR:

<DetailViewContainer>
  <DetailViewTitle>...</DetailViewTitle>
  <DetailViewBody>...</DetailViewBody>
</DetailViewContainer>

Where to place styles

Advice: Place CSS class declarations close to components in which they are used

E.g.: if you're using className="Bar" inside of Foo.jsx then Foo.css should contain the CSS declaration for the Bar class

Don't create too many components too early

e.g. all functionality inside of layouts/ could be included inside of files in pages/, avoiding one level of components

many components inside of components/ could be replaced by their plain HTML counterparts