4
\$\begingroup\$

This was my code challenge for a Junior JavaScript Developer position. The app is mostly running error-free, but I was rejected for the role, so I assume there were major issues with my code and have no idea what they were. I know this is a long piece of code, but as it already runs, I'm not asking for a fix - more for possible reasons that it was rejected. I would really appreciate any advice to help me improve, thank you!

The app allows the user to either: 1) submit a Github username in a textarea, get a list of his/her repos from the GitHub API, click a repo and get a list of issues. 2) submit both username and repo name in two textareas and get the list of issues.

import React from 'react';           
export default class App extends React.Component {

constructor(props) {
super(props);
this.state = {}
this.submitNameAndRepo = this.submitNameAndRepo.bind(this);
this.fetchRepoIssues = this.fetchRepoIssues.bind(this)
this.handleKeyPress = this.handleKeyPress.bind(this)
  }

 //FUNCTION TO ALLOW BUTTON TO BE CLICKED WITH 'ENTER'
 handleKeyPress(e) {
  if (event.key == 'Enter') {
  this.submitNameAndRepo(e)
}
}
 //FUNCTION RUNS WHEN CLICKING 'SEARCH'

 submitNameAndRepo(e) {
  this.setState({noIssues: false, error: false});

  //REMOVING PREVIOUS ISSUES FOUND

  if (this.state.issues) {
  this.setState({issues: undefined})

  //IF BOTH USER AND REPO WERE ENTERED

} else if (this.state.text && this.state.repo_name) {
  const text = this.state.text;
  const repoName = this.state.repo_name;
  const url = "https://api.github.com/repos/" + text + "/" + repoName + 
  "/issues?per_page=20";
  fetch(url).then(response => response.json()).then(data => {
    if (data.length !== 0) {
      this.setState({issues: data})
    } else {
      this.setState({noIssues: true})
    }
  }).catch((err) => {
    this.setState({error: true})
  })

  //IF ONLY USER WAS ENTERED, FETCH LIST OF REPOS

} else if (this.state.text && !this.state.repo_name) {
  const text = this.state.text;
  const url = "https://api.github.com/users/" + text + "/repos";
  fetch(url).then(response => response.json()).then(data => 
   this.setState({repos: data})).catch((err) => {
    this.setState({error: true})
  })

  // IF REPO WAS PROVIDED BUT NO USERNAME

} else if (this.state.repo_name && !this.state.text) {
  this.setState({noText: true})
}
this.setState({repo_name: false})
 }
// FUNCTION BELOW RUNS WHEN USER CLICKS ON A REPO NAME (IF ONLY GITHUB USERNAME WAS 
// ORIGINALLY PROVIDED

fetchRepoIssues(repo) {
this.setState({
  repo_name: repo
}, () => {
  this.submitNameAndRepo();
})
}

render() {

  //MAPPING ISSUES ARRAY IF ISSUES WERE FOUND

  let issueList;
  let issues = this.state.issues;
   if (issues && issues.length !== 0 && this.state.error !== true) {
  issueList = issues.map(issue => <div id='single-issue-container'>
      <span className='desc-title'>Issue ID: &nbsp;</span>
      {issue.id}
    <div className='issue-text'>
      <span className='desc-title'>Title:&nbsp;
      </span>{issue.title}</div>
    <div className='issue-text'>
      <span className='desc-title'>Created at: &nbsp;</span>
      {issue.created_at}</div>
  </div>)

}

 //MAPPING REPO ARRAY IF REPOS WERE FOUND. CLICK ON REPO RUNS fetchRepoIssues Function

let repoList;
let repos = this.state.repos;
if (repos) {
  repoList = repos.map(repo => <button className='repo-btn' style={{
      cursor: 'pointer'
    }} onClick={() => this.fetchRepoIssues(repo.name)}>
    {repo.name}
  </button>)
}

return (<div id='main'>
  <div id='search-field-container'>
    <h2 className='app-title'>Welcome to issueSearch</h2>
    <div className='search-header search'>Please enter username and repo to retrieve issues</div>
    <textarea className='textarea-search search' onKeyPress={this.handleKeyPress} placeholder='Search by username' onChange={(e) => this.setState({text: e.target.value})}></textarea>
    <textarea className='textarea-search search' onKeyPress={this.handleKeyPress} placeholder='Enter repo' onChange={(e) => this.setState({repo_name: e.target.value})}></textarea>
    <button className='default-btn search' onClick={this.submitNameAndRepo}>Search</button>
  </div>

  //DISPLAY REPOS IF EXIST

  {
    this.state.repos && <div id='repos-section'>
        <div>Please choose from this user's repositories</div>
        <div id='repos-container'>{repoList}</div>
      </div>
  } 
    //DISPLAY ISSUES IF FOUND
    {
    this.state.issues && <div id='issues-section-container'>{issueList}</div>
  } 
   //DISPLAY THIS MESSAGE IF NO ISSUES FOUND
   {
    this.state.noIssues && <div className='error'>
        No issues found</div>
  } 
   //DISPLAY THIS IF USER DID NOT ENTER TEXT
  {
    this.state.noText && <div className='error'>Please enter at least username to search</div>
  } 
  //DISPLAY THIS MESSAGE IN CASE OF ERROR
  {
    this.state.error === true && <div className='error'>There seems to be an error, please make sure all fields were entered correctly</div>
  } 
  </div>)
}
}
\$\endgroup\$
3
\$\begingroup\$

Indentation and formatting

First and foremost, this code is not properly indented. If a candidate presents me with an unindented or sloppily indented piece of code, I wouldn't hire him, no matter how good his code otherwise.

  • Some lines end with semicolon, and some don't. This looks sloppy and inconsistent.
  • Comments are written in all-caps. That's generally considered really bad style. Also known as "screaming".

Presentation matters. It really does. If you would apply for a newspaper reporter position, would you submit an essay where paragraphs would start without indentation and sentences would end without full stop?

Any fool can write a program that a machine can understand. A greater challenge is writing code that other people can easily understand.

You need to learn how to properly format your code. Google "JavaScript style guide" and follow whichever you like - doesn't really matter, as long as you format your code consistently.

Code structure

One of the benefits of a framework like React is that it allows you to split you code up into multiple simpler components which you can then compose into other components that perform more complex tasks.

Your code does not take advantage of that. Instead it has been written as a single large App component.

The same applies for functions. Instead of large function like render() you should strive to break your logic down to smaller functions. Giving these functions good names, will also allow you to make your code more self-documenting without needing to write a comment.

\$\endgroup\$
  • \$\begingroup\$ Rene, thank you so much for the thoughtful and quick reply. The company already replied to me with their comments, and basically repeated what you said with 'code has no formatting'. I will definitely go into the indentation issue much more, since I'm currently just using 'beautify' on my code, which apparently doesn't really work. I didn't know capitals is a problem, I will change that as well and go research JS's style guide in depth. Again, much appreciated! J \$\endgroup\$ – Jonathan Bareket Apr 27 '18 at 13:06

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.