0
\$\begingroup\$

I would love some help or some directions with refactoring this block of code. It perfectly works but it kind of looks ugly ;). It was messier but I tried to make it DRY-er.

 const uploadedFiles = images.map((item, index) => {
 if (images.length < 2) {
 if (item.file.name.match(/.(gif)$/i)) {
 return (
 <ImageGif
 key= {index}
 src= {item.blobUrl}
 onClose={this.props.removeImage}
 />
 );
 }
 return (<ImageContainer
 src={item.blobUrl}
 key={index}
 id={index}
 onClose={this.props.removeImage}
 />);
 }
 if (images.length >= 2) {
 if (MAX_SIZE === index + 1) {
 return (<ImageContainer
 src={item.blobUrl}
 key={index}
 id={index}
 onClose={this.props.removeImage}
 hiddenImages={hiddenImages}
 />);
 }
 if (MAX_SIZE !== index + 1) {
 return (<ImageContainer
 src={item.blobUrl}
 key={index}
 id={index}
 onClose={this.props.removeImage}
 />);
 }
 }
 return null;
});

Thank you and have a nice day!

asked Aug 1, 2017 at 10:10
\$\endgroup\$
2
  • 1
    \$\begingroup\$ Welcome to Code Review! The current question title, which states your concerns about the code, applies to too many questions on this site to be useful. Please edit to the site standard, which is for the title to simply state the task accomplished by the code. Please see How to get the best value out of Code Review: Asking Questions for guidance on writing good question titles. \$\endgroup\$ Commented Aug 1, 2017 at 12:00
  • \$\begingroup\$ Please read How to Ask to understand how to title your question, and then edit your question to change the title accordingly. \$\endgroup\$ Commented Aug 3, 2017 at 4:54

1 Answer 1

4
\$\begingroup\$

Use ES2017 object spread:

const uploadedFiles = images.map((item, index) => {
 const props = {
 key: index,
 src: item.blobUrl,
 onClose: this.props.removeImage,
 };
 const isSingleGif = images.length === 1 && /\.gif$/i.test(item.file.name);
 if (isSingleGif) {
 return (<ImageGif {...props} />);
 }
 const isLastOfVisible = index === MAX_SIZE - 1 && index > 0;
 return (<ImageContainer
 id={index}
 {...props}
 {...isLastOfVisible && {hiddenImages}}
 />);
});

Not tested.

Notes:

  • images.length < 2 was misleading and confusing. The .map callback runs only when there are some elements so 0 is not possible. Simply compare with 1.
  • images.length >= 2 is superfluous and thus confusing: the previous check ensured this is always the case.
  • MAX_SIZE === index + 1 this yoda style obfuscates the actual goal, which is to determine whether the element is the last in the range.
  • MAX_SIZE !== index + 1 is superfluous and thus confusing: the previous check ensured this is always the case.
  • /.(gif)$/i incorrectly allowed any character before gif. The dot should be escaped. Also, no need for the slower .match if you don't use the captured strings. Use .test
answered Aug 1, 2017 at 11:35
\$\endgroup\$
1
  • 1
    \$\begingroup\$ Object spreading correctly handles 0, false, null, undefined - just like Object.assign. \$\endgroup\$ Commented Aug 1, 2017 at 15:57

Your Answer

Draft saved
Draft discarded

Sign up or log in

Sign up using Google
Sign up using Email and Password

Post as a guest

Required, but never shown

Post as a guest

Required, but never shown

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

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.