Profile with static and input modes (View and Update)

Question

I'm a solo developer and a friend told me that my code is not very readable. I always write code trying to keep it DRY while not taking longer than it would take doing it in an "easier" way.

Here is the skeleton for My Profile feature which will initially have read-only values. When the user wants to update it, it turns into an input when clicking on edit.

Is there anything wrong with this way of going about it?

class MyProfileEdit extends React.Component {
 constructor(props){
 super(props);
 this.state = {
 data: {},
 onEdit: ""
 }
 this.displayKey = {
 "first_name": "First name",
 "last_name": "Last name",
 "zipCode": "Zip"
 }
 }
 handleOnChange = key => e => e.persist() || this.setState(prevState => {
 prevState.data[key] = e.target.value;
 return prevState;
 })
 getInputValue = key => this.state.data[key] || this.props.currentUser[key]
 getKeyDisplay = key => this.displayKey[key] || key[0].toUpperCase() + key.slice(1)
 getStatic = key => (
 <label style={{display: "flex"}}>
 {this.getKeyDisplay(key)}: {this.getInputValue(key)} &nbsp;&nbsp;&nbsp;
 <div onClick={() => this.setState({onEdit: key})}>Edit</div>
 </label>
 )
 getInput = key => (
 <label style={{ display: "flex" }}>
 {this.getKeyDisplay(key)}:
 <input value={this.getInputValue(key)} onChange={this.handleOnChange(key)} /> 
 <div onClick={() => this.setState({ onEdit: "" })}>Done</div>
 </label>
 )
 getStaticOrInput = key => this.state.onEdit != key ? this.getStatic(key) : this.getInput(key);
 render(){
 return (
 <React.Fragment>
 My profile:<br /><br />
 { this.getStaticOrInput("first_name") }
 { this.getStaticOrInput("last_name") }
 <label>
 Notification option:
 <select 
 value={this.getInputValue("notification_option")} 
 onChange={this.handleOnChange("notification_option")}
 >
 <option value="SMS">SMS</option>
 <option value="EMAIL">Email</option>
 </select>
 </label>
 { this.getStaticOrInput("city") }
 { this.getStaticOrInput("address") }
 { this.getStaticOrInput("zipCode") }
 </React.Fragment>
 )
 }
}

Answer 1

Other than your handleOnChange handler is mutating state, this code seems understandable and readable enough to me, only minor comments about code style.

React-ness

React's synthetic events' persist function is a void return, so I don't think the logical OR between it and the state update necessary.

Mutating-state handler with odd event persisting.

handleOnChange = key => e => e.persist() || this.setState(prevState => {
 prevState.data[key] = e.target.value; // <-- this mutates existing state
 return prevState; // <-- saving previous state object back in state
})

Non-mutating-state handler. React class-based component state updates are merged in, but nested state needs to have their previous state merged manually. Also, instead of persisting the event for use in the "asynchronous" setState, it is more common to grab the event value and let react do what it needs with the event object (i.e. return back to pool).

handleOnChange = key => e => {
 const { value } = e.target;
 this.setState(prevState => ({
 data: {
 ...prevState.data,
 [key]: value,
 },
 }));
}

Readability

All other comments I'd have on the code are more about the readability, i.e. appropriate usage of whitespace, 2-space vs 4-space tabs, etc.. but these are largely dev team driven and tend to be subject to opinion.

Common practices though are

• Single line space between code blocks (functions and any other logical "chunk" of code
• Using curly brackets for functions that are more than simply an implicit return, i.e. they have more than a single expression
• Using appropriate line breaks when lines of code get too long, usually around 80 chars

Maintainability/Reusability

The one remaining comment I'd have would be to try and abstract the this.getStaticOrInput logic and utilities into a separate react component, something like EditableInput that handles its toggling state internally.

<EditableInput name="first_name" value={...} etc.. />
<EditableInput name="last_name" value={...} etc.. />
...

Answer 2

In regards to readability, I agree that there is room for improvement. It seems like you're trying to cram as much code in as you can in fewer lines.

Use the conventional style for Javascript class methods where you have `

name_of_method(arg1, arg2) {
 return ... 
}

instead of setting them like you are now, like you would a local variable. Additionally, include a line of whitespace between each method definition.

Yes, you will have to write more this way because you're putting a return statement and curly braces for each method now, but this will make your code much cleaner.

---

I like this idea you have going here, but I think you should take better advantage of the design that React tries to push. You've got several elements that are very similar: they display some sort of data, and when they are clicked, they become editable. There are about 5 instances of that here. Why not make a React component that does just that?

Create a new react component that has all that functionality built in, but just for a single element. Then, in this My Profile element, you could set it up something like this:

<Editable name="first_name"/>
<Editable name="last_name"/>
...

(note: you don't have to use the name attribute; I'm just using it for the sake of example)

This approach would be much more idiomatic for React and would take better advantage of what the library has to offer.