3
\$\begingroup\$

I am looking for some suggestions on how to make my code cleaner and simpler. Or put another way: more elegant and idiomatic. Any other comments on quality or anything would be appreciated.

class App extends Component {
      constructor(props){
        super(props);

        this.state = {
            colIds : ["winnie", "bob", "thomas", "george"], 
            cols : [
              {name:"Winnnie", headerColor: "#8E6E95", id : "winnie" },
              {name:"Bob",  headerColor: "#39A59C", id :"bob"},
              {name:"Thomas",  headerColor: "#344759", id: "thomas"},
              {name:"George",  headerColor: "#E8741E", id: "george"}
            ],
            cards : [
              {colid: "winnie", task: "buy eggs"},
              {colid: "bob", task: "clean basement"},
              {colid: "thomas", task: "pack"},
              {colid: "george", task: "pay bills"},
            ]
          }

          this.addCard = this.addCard.bind(this);
          this.moveCardRight = this.moveCardRight.bind(this);
          this.moveCardLeft = this.moveCardLeft.bind(this);
      }

      getColCards(colid){
          let cards = this.state.cards.filter( c => c.colid == colid);
          return cards;
      }

      addCard(colid){
        let cardTask = window.prompt("Whats the task you want to add");
        let currentCards = this.state.cards.slice();
        let newCard = {colid: colid, task: cardTask};
        currentCards.push(newCard);
        this.setState({cards: currentCards});

      }
      moveCardRight(card){
        let currentCardCol = card.colid;
        let nextIndex =  this.state.colIds.indexOf(currentCardCol) + 1;

        if(nextIndex > this.state.colIds.length - 1 ){
          return null;
        }
        let currentCards = this.state.cards.slice();
        for(let i = 0; i < currentCards.length; i++){
            if(currentCards[i].task === card.task){
               currentCards[i] = {
                 ...currentCards[i],
                 colid : this.state.colIds[nextIndex]
               }
            }
        }
        this.setState({cards: currentCards});
      }

      moveCardLeft(card){
        let currentCardCol = card.colid;
        let nextIndex =  this.state.colIds.indexOf(currentCardCol) - 1;

        if(nextIndex < 0 ){
          return null;
        }
        let currentCards = this.state.cards.slice();
        for(let i = 0; i < currentCards.length; i++){
            if(currentCards[i].task === card.task){
               currentCards[i] = {
                 ...currentCards[i],
                 colid : this.state.colIds[nextIndex]
               }
            }
        }
        this.setState({cards: currentCards});
      }
      render() {
        let cols =[  
                      {name:"Winnnie", headerColor: "#8E6E95"},
                      {name:"Bob",  headerColor: "#39A59C"},
                      {name:"Thomas",  headerColor: "#344759"},
                      {name:"George",  headerColor: "#E8741E"}
        ];
        let colCards = this.state.cols.map(c => {
        let cards = this.getColCards(c.id).map(c =>  {
            return <div>
                    <span onClick = {() => this.moveCardLeft(c)}> {"<   "} </span> 
                    {c.task}   
                    <span onClick = {() => this.moveCardRight(c)}> {"    >"} </span> 
                   </div> 
          })

          return  <CardCol  name={c.name}  headerColor={c.headerColor} addCard={this.addCard} id={c.id}>
                     {cards}
                  </CardCol>
        })
        return (
          <div className="App">
          {colCards}
          </div>
        );
      }
    }
\$\endgroup\$
1
  • \$\begingroup\$ Could you please edit you post to include the code for the CardCol component? And are the moveCardLeft() and moveCardRight() functions supposed to move the cards left and right? if so, does that currently work? \$\endgroup\$ Commented Aug 22, 2018 at 23:30

2 Answers 2

Reset to default
2
+50
\$\begingroup\$

Your implementation is great and works well, but there are some things you could change.

Eliminate repetitive code

You have two large methods, moveCardLeft and moveCardRight, that do almost the same thing. These can either be combined entirely or the matching code can be separated out to a new method and referenced by the original two. For my example, I chose the former:

moveCard(card, dir) {
  let nextIndex = this.state.colIds.indexOf(card.colid);

  if (dir === "left") nextIndex--;
  else if (dir === "right") nextIndex++;
  else return null;

  if (nextIndex < 0 || nextIndex > this.state.colIds.length - 1) {
    return null;
  }

  let currentCards = this.state.cards.slice();
  for (let i = 0; i < currentCards.length; i++) {
    if (currentCards[i].task === card.task) {
      currentCards[i] = {
        ...currentCards[i],
        colid: this.state.colIds[nextIndex]
      }
    }
  }
  this.setState({ cards: currentCards });
}

With this, you just have to pass in an extra parameter for the direction (left or right). It eliminates the repetition which is almost always beneficial.

Use consistent formatting

This includes indentation, new lines, and general spacing.

As you have it written in your post (maybe this is just a copy/paste error), the map inside of a map in your render method is not indented properly, which will undoubtedly confuse you at some point. Here is what it should look like:

render() {
  let cols = [
    { name: "Winnnie", headerColor: "#8E6E95" },
    { name: "Bob", headerColor: "#39A59C" },
    { name: "Thomas", headerColor: "#344759" },
    { name: "George", headerColor: "#E8741E" }
  ];
  let colCards = this.state.cols.map(c => {
    let cards = this.getColCards(c.id).map(c => {
      return (
        <div>
          <span onClick={() => this.moveCard(c, "left")}>{"<   "}</span>
            {c.task}
          <span onClick={() => this.moveCard(c, "right")}>{"    >"}</span>
        </div>
      );
    });

    return (
      <CardCol name={c.name} headerColor={c.headerColor} addCard={this.addCard} id={c.id}>
        {cards}
      </CardCol>
    );
  });

  return (
    <div className="App">
      {colCards}
    </div>
  )
}

Other thoughts

  • Use strict equality (===) by default and only fall back to loose/abstract equality (==) when absolutely necessary.
  • Consider how much more concise your code could be if you got rid of the colIds and simply used the cols instead to organize the cards. Additionally, consider placing the cards inside of their respective cols instead of separating them out into their own object. I haven't tried this on your code, so it may end up being less concise, but it's good to consider various data structures in anything you write.
\$\endgroup\$
1
\$\begingroup\$

Use const wherever value is not re-assigned

The code makes use of let for the majority of the variables. Generally it is wise to Use const for variables and then fall back to let if the value does need to be re-assigned. This avoids unintentional re-assignment.

getColCards method

I wonder about the method getColCards: 

getColCards(colid){
    let cards = this.state.cards.filter( c => c.colid == colid);
    return cards;
}

Why store the filtered array in cards before returning it? Why not just return the filtered list (without the intermediary assignment)? While it isn't an unused variable, I wonder if it was leftover from debugging the value. Some may argue that it is more readable but others may argue that it is pointless to create a variable for a value and then immediately return that value. For more information on this, refer to these answers on a related topic.

Perhaps naming the method something like getFilteredColCards() would maintain readability.

The method could be simplified as follows:

getFilteredColCards(colid){
    return this.state.cards.filter( c => c.colid == colid);
}

This may not use significantly less memory, but (since it is similar to this example on the MDN documentation about return values in functions) it is "quicker to write, and more compact"1

Function partials can be used instead of an extra lambda function

The extra lambda/anonymous functions for the left & right arrow click handlers could be removed by using Funtion.bind() to create a partially applied function:

<span onClick = {this.moveCardLeft.bind(this, c)}> {"<   "} </span>

and 

<span onClick = {this.moveCardRight.bind(this, c)}> {"    >"} </span>

Useless variable

The method render() has a variable cols but that doesn't appear to be used (though correct me if I am wrong about that). It does appear that this.state.cols is used - was cols left over from previous code? It appears to be a primitive version of cols from the state object.

Iterator variable name re-used

The method render() has the following lines:

let colCards = this.state.cols.map(c => {
let cards = this.getColCards(c.id).map(c =>  {

So in both cases c is used. When I tried to run the code I saw the warning:

Warning: Each child in an array or iterator should have a unique "key" prop.

Check the render method of App. See https://fb.me/react-warning-keys for more information.

So I added a key to make those <div> elements unique and to utilize the outer iterator, I renamed the inner iterator variable cc:

const colCards = this.state.cols.map(c => {
      const cards = this.getColCards(c.id).map(cc =>  {
        return <div key={c.id + '_' + cc.id}>

1https://developer.mozilla.org/en-US/docs/Learn/JavaScript/Building_blocks/Return_values#Using_return_values_in_your_own_functions

\$\endgroup\$
2
  • \$\begingroup\$ I don't agree with your excessive assignment argument. Sure, directly returning an inline operation's result requires less lines of code, but ultimately it's better to assign the operation's result to a variable first so as to name the value you're returning. It's all about readable code. \$\endgroup\$ Commented Aug 27, 2018 at 6:37
  • \$\begingroup\$ I understand your point and have expanded the section \$\endgroup\$ Commented Aug 27, 2018 at 19:22

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.