3
\$\begingroup\$

After learning Javascript for quite some time, I began learning ReactJS and... I am confused with the proper design patterns.

import React, { useState } from 'react';
import ReactDOM from 'react-dom';

const Search = ({persons, filter}) =>
{
    const [name, setName] = useState(void 0);

    function search(event){
        event.preventDefault();
        filter(event.target.value.toLowerCase());
    }

    return (
        <div>
            <h1>Search</h1>
            <form onSubmit={search}>
                <input onChange={(event) => search(event)}/>
                <button type="submit">Search</button>
            </form>
        </div>
    )
}

const Persons = ({persons}) => 
{
    function renderPersons(){
        const elements = [];

        persons.forEach(person => {
            if(person.show){
                elements.push(<li>{person.name} -- his/her telephone number: {person.phone}</li>);
            }
        });

        return elements;
    }

    return(
        <ul>
            {renderPersons()}
        </ul>
    )
}

const App = () => 
{
    const [ persons, setPersons ] = useState([
        { name: 'Arto Hellas', phone: '040-123456', show: true },
        { name: 'Ada Lovelace', phone: '39-44-5323523', show: true },
        { name: 'Dan Abramov', phone: '12-43-234345', show: true },
        { name: 'Mary Poppendieck', phone: '39-23-6423122', show:true }
    ]);


    const [ formInfo, setNewFormInfo ] = useState({name: '', phone: ''});

    function addPerson(event){
        event.preventDefault();

        let able = true;

        persons.forEach(person => { // checks if the person's name is already added
            if(person.name == formInfo.name)
                able = false;       
        });

        if(able)
            setPersons(persons.concat({name:formInfo.name, phone:formInfo.phone, show:true}));
        else
            alert(`${formInfo.name} is already added to the list!`);
     }

    function filter(nameToFilter){
        if(nameToFilter === ''){ // if the search form is empty, then show everyone
            setPersons((prev) => prev.map(person => ({...person, show:true})));
        }
        else{
            const filtered = [];
            persons.forEach(person => {
                if(!person.name.toLowerCase().includes(nameToFilter))
                    filtered.push({...person, show:false});
                else
                    filtered.push({...person});
            });
            setPersons(filtered);
        }
    }

    return (
        <div>
            <Search persons={persons} filter={filter} />
            <h2>Phonebook</h2>
            <form onSubmit={addPerson}>
                <div>
                    name: <input onChange={(event) => setNewFormInfo({name: event.target.value, phone: formInfo.phone})}/>
                    <br/>
                    number: <input onChange={(event) => setNewFormInfo({name: formInfo.name, phone: event.target.value})}/>
                </div>
                <div>
                    <button type="submit">add</button>
                </div>
            </form>
            <h2>Persons</h2>
            <Persons persons={persons} />
        </div>
    )
}

ReactDOM.render(<App />, document.getElementById("root"))

It works, but I have a feeling the code really is poorly designed; the course I am taking doesn't focus on details and motivates you to self-learn a lot.

I do have a specific question, though. Can I make persons a class? I just feel like in larger applications, each person would have methods and more variables; can I hold classes inside of React states instead of dicts?

\$\endgroup\$

1 Answer 1

2
\$\begingroup\$

There are a bunch of small improvements you can make.

Since you aren't using name or setName, you can remove them and their useState call (consider a linter to warn on unused variables).

You have <form onSubmit={search}>, but the search function does filter(event.target.value.toLowerCase()). Forms don't have .values, so this will throw an error when the button is pressed. Also, the search function could be confused with the Search component - the difference is only in capitalization, which isn't enough. The button doesn't do anything otherwise, because searching occurs when the input's change event fires. You might consider removing the button completely - this way you can remove the search function and just call filter:

<form>
    <input onChange={event => filter(event.target.value)} />
</form>

In your code, eveywhere you return JSX at the bottom of a function, there's no trailing ; after the final ). This can result in bugs if you ever decided to put the fragment into a variable instead of returning immediately, due to automatic semicolon insertion. Since you're using semicolons everywhere else, consider using them everywhere they're appropriate as well. (again, linting will help with this)

Similarly to the search vs Search above, you also have a persons array and a Persons component. Maybe call the component PersonsList instead, since it creates a <ul>.

Rather than using forEach and conditionally pushing to an array outside, it may be a better idea to construct the array directly by filtering for elements which match, then .mapping to the individual <li>s. Higher-order functions are great:

const PersonsList = ({ persons }) => (
    <ul>
        {(
            persons
                .filter(person => person.show)
                .map(person => <li>{person.name} -- his/her telephone number: {person.phone}</li>)
        )}
    </ul>
);

In addPerson, if you want to check whether any items in an array match a condition, rather than setting a flag outside and conditionally reassigning it while iterating over an array, it's more appropriate to use Array.prototype.some:

const alreadyExists = persons.some(person => person.name === name);

If you were to use your original code, at least use strict equality with === instead of loose equality ==.

Also in addPerson, using window.alert is almost never a good choice. It's user-unfriendly, since it completely blocks them from using the rest of the page. It also prevents any other Javascript from executing until the alert is dismissed, which would be a problem if there were anything on the page other than this phone number component. Inform the user of the problem some other way. A very simple tweak would be to add errorMessage to the form's state:

if (!alreadyExists)
    setPersons(persons.concat({ name, phone, show: true }));
else
    setNewFormInfo({ name, phone, errorMessage: `${name} is already added to the list!` });

and add to the form's JSX:

{(errorMessage && <div className="error">{errorMessage}</div>)}

In the filter function, there's no need to check if the input is empty - if it is, everything will be rendered anyway. Feel free to remove that part and make the code simpler.

Also in filter, since you're creating a filtered array by iterating over each element of persons and pushing an element to filtered, using .map would be more appropriate than forEach / if / push:

function filter(nameToFilter) {
    const nameToFilterLower = nameToFilter.toLowerCase();
    setPersons(persons.map(person =>
        ({
            ...person,
            show: person.name.toLowerCase().includes(nameToFilterLower),
        })
    ));
}

If you run the development version, you'll see the warning:

Warning: Each child in a list should have a unique "key" prop.

Without such a prop, each element will have to be re-created each time. See here for an in-depth explanation. Since you're only adding items to the list, easiest fix would be to add <li key={name}.

Putting all of the above together into a live Stack Snippet:

const { useState } = React;

const Search = ({ filter }) => {
    return (
        <div>
            <h1>Search</h1>
            <form>
                <input onChange={event => filter(event.target.value)} />
            </form>
        </div>
    );
};

const PersonsList = ({ persons }) => (
    <ul>
        {(
            persons
                .filter(person => person.show)
                .map(({ name, phone }) => <li key={name} >{name} -- his/her telephone number: {phone}</li>)
        )}
    </ul>
);

const App = () => {
    const [persons, setPersons] = useState([
        { name: 'Arto Hellas', phone: '040-123456', show: true },
        { name: 'Ada Lovelace', phone: '39-44-5323523', show: true },
        { name: 'Dan Abramov', phone: '12-43-234345', show: true },
        { name: 'Mary Poppendieck', phone: '39-23-6423122', show: true }
    ]);

    const [{ name, phone, errorMessage = '' }, setNewFormInfo] = useState({ name: '', phone: '' });
    function addPerson(event) {
        event.preventDefault();

        const alreadyExists = persons.some(person => person.name === name);
        if (!alreadyExists)
            setPersons(persons.concat({ name, phone, show: true }));
        else
            setNewFormInfo({ name, phone, errorMessage: `${name} is already added to the list!` });
    }

    function filter(nameToFilter) {
        const nameToFilterLower = nameToFilter.toLowerCase();
        setPersons(persons.map(person =>
            ({
                ...person,
                show: person.name.toLowerCase().includes(nameToFilterLower),
            })
        ));
    }

    return (
        <div>
            <Search filter={filter} />
            <h2>Phonebook</h2>
            <form onSubmit={addPerson}>
                <div>
                    name: <input onChange={event => setNewFormInfo({ name: event.target.value, phone })} />
                    <br />
                    number: <input onChange={event => setNewFormInfo({ name, phone: event.target.value })} />
                </div>
                <div>
                    <button type="submit">add</button>
                </div>
                {(errorMessage && <div className="error">{errorMessage}</div>)}
            </form>
            <h2>Persons</h2>
            <PersonsList persons={persons} />
        </div>
    );
};
ReactDOM.render(<App />, document.getElementById("root"))
.error {
  color: red;
}
<script crossorigin src="https://unpkg.com/react@16/umd/react.development.js"></script>
<script crossorigin src="https://unpkg.com/react-dom@16/umd/react-dom.development.js"></script>
<div id="root"></div>

Can I make persons a class?

You can, but React highly recommends using hooks (and stateless functional components) when possible. They're easier to make sense of too, IMO.

in larger applications, each person would have methods and more variables

If you want to encapsulate common functionality for Persons, you can easily do so by putting it into its own file and defining those functions you need inside that file (or by importing them), eg:

// example helper function
const transformName = name => name.toUpperCase();
export const PersonsList = ({persons}) => 
  // ...
  <li key={name} >{transformName(name)} -- his/her telephone number: {phone}</li>
  // ...

There are a few rare cases for which classes must still be used, but none of those circumstances are relevant here.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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