efhiii/github-search

github-search's Issues

Advice on improving code to make it more production-ready

First of all, congrats on finishing the challenge! In this project you have demonstrated familiarity around the Reason language and common patterns in general, and here is my feedback to make more production-ready.

Advice on project configuration

1. Ignore the generated *.bs.js files (for example, src/Search/Result.bs.js) in git. The *. bs.js files are generated from the corresponding .re files so we don't want git to keep track of it, because the .re files are the actual source.

   # in .gitignore
   **/*.bs.js
   

2. I'm a bit confused by the UNUSED_webapck.config.js. Are we using webpack to build the project at all? If we are using webpack, you might want to check out webpack-dev-server and other plugins as well. Also, you probably want to separate production and development build config. Right now, the UNUSED_webapck.config.js is configured towards production builds.

3. What's the purpose of separating index.html and indexProduction.html?

4. There are scaffolding tools such as create-react-app and bsb -init to spin up project in seconds.

Advice on structuring code for React
A general rule of thumb when it comes to production code is Separation of concerns. For example, we want to separate visual styling from the DOM structure, and rendering of DOM tree from related data processing, and the data processing from logic handling user interactions. An example in case of User Interface would be Model-view-controller, or MVC. In React, a similar mapping exists, in a form of Model <-> State, View <-> Rendering function and Controller <-> Hooks/callbacks.

Using this project as an example, we can

1. Make Search.htmlResults() a rendering function that renders DOM layout from state and store Result.result in state. Avoid saving rendered DOM in state because rendered DOM is a product from state and the rendering process is internal to the React library. The Result.result is the source of truth not the rendered DOM.

2. Refactor to remove the Matches.styleMatches() method. What it's trying to achieve is adjust styling of rendered DOM. This is better achieved using CSS. There's a pattern for applying different styles for odd and even items, for example

.match {
  background(white);
}
// overrides style of even items (2nd, 4th, 6th ...)
.match:nth-child(2n) {
  background(azure);
}

Advice on functional programming styles

1. Choose the best helper function. For example, for Result.filterResults, since we are filtering results not containing query keywords, it's more expressive to use Belt.Array.keep or Array.filter rather than Belt.Array.reduce here.

let filterResults = (results, query) =>
  results
  ->Belt.Array.keep(item => repoContainsWords(item, query))
  ->Belt.Array.slice(~offset=0, ~len=10);

2. Choose function or variable names that represent the action/intent/purpose. Using the Result.filterResults an example here, since it only takes first 10 items, it's easier for others to understand the code if we rename it to some thing like first10ResultsContainingWords(result, query). Even better, is to abstract out the number of results to take or slice here, because we might want to adjust that number later, so it becomes

let keepFirstNResultItemsMatchingQuery = (~query, ~len, results) =>
  results
  ->Belt.Array.keep(item => repoContainsWords(item, query))
  ->Belt.Array.slice(~offset=0, ~len);

3. If we are apply simple transformations with .map() and .reduce(), it's actually better to combine them into single .reduce() call. However, **do not do this if the transformation is complicated **(for example, over 30 lines of code).

let repoContainsWords = (item, query) =>
  query
  ->String.lowercase_ascii
  ->Js.String2.split(" ")
  ->Belt.Array.reduce(
     false, 
    (prevResult, word) =>
      prevResult || repoContainsWord(item, word)
  );

In this specific case, since we are checking if there's any query keywords in item, a much better solution it to make use of the Belt.Array.some() method

let repoContainsWords = (item, query) =>
  query
  ->String.lowercase_ascii
  ->Js.String2.split(" ")
  ->Belt.Array.some(word => repoContainsWord(item, word)); 
  // or curry the `word` params so that is become:
  //  ->Belt.Array.some(repoContainsWord(item))

Advice with ReasonML

1. There are tools and libraries to help us serializing and deserializing between JSON and Reason types. For example there's a Foreign Function Interface available (@bs.deriving abstract) and we are using Decco in our project, because it provides rigid type checking when serializing and deserializing in the runtime.

[@decco]
type result = {
  id: string,
  url: string,
  updatedAt: float, // Js.Date.t serialize to float
  nameWithOwner: string,
  description: option(string),
  stargazerCount: int,
  avatarUrl: string,
};

// example decode result
let url = 
  switch (result_decode(json)) {
    | Ok({url}) => url
    | Error(decodeError) =>
      Js.Console.error(decodeError);
      Js.Exn.raiseError("My error handling code");
  };

2. Try make pattern matching explicitly exhaustive. The | _ => ... wildcard is helpful is some cases but when possible, we should be as specific writing pattern matching. The reason compiler will check if we've exhaustively covered all cases and we should make good use of that. For example, we can rewrite Result. repoContainsWord() to

let repoContainsWord = (item, word) =>
switch (word) {
  | ("", _) => false
  | (_, Some(desc)) =>
    Js.String.includes(word, desc->String.lowercase_ascii)
    || Js.String.includes(word, item.nameWithOwner->String.lowercase_ascii)
  | (_, None) => Js.String.includes(word, item.nameWithOwner->String.lowercase_ascii)
};

3. Make type-checking your friend not your enemy. The type-checking is a helpful mechanism to help us verify that our code is correct at the type level. The better we specify types, the better the compiler would help us verify that, and more likely to catch bugs caused by type mismatch early on. Using open types such as Js.t({..}) here is dangerous and might cause you issue later on. There is a binding library, Webapi, that we are using to interface between Reason and the DOM world.

let style = Webapi.Dom.document |> Webapi.Dom.Document.createElement("style");
let htmlDocument = switch (Webapi.Dom.document->Webapi.Dom.HtmlDocument.ofDocument) {
  | Some(htmlDocument) => htmlDocument
  | None =>
    Js.Exn.raiseError("Expect htmlDocument");
};
htmlDocument
|> Webapi.Dom.HtmlDocument.head
|> Webapi.Dom.Element.appendChild(style);
style.setInnerHTML(Styles.style);

let makeContainer = text => {
  let container = Webapi.Dom.document |> Webapi.Dom.Document.createElement("div");
  container->Webapi.Dom.Element.setClassName("container");
  // ...
  content;
};