Skip to content

Removed code duplication between 2 files. #3364

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed

Conversation

Harshit-7373
Copy link
Contributor

@Harshit-7373 Harshit-7373 commented Mar 5, 2025

Removed code duplication between 2 files. #2805

Fixes #2805

Changes:

  1. A common function was used in both files. To avoid duplication, I created a shared folder and added a new file (resolveUtils.js), where I wrote the common function along with the necessary import statements.
  2. Imported the shared function (resolveUtils.js) into both files and removed the redundant function definitions.
  3. Updated and corrected the RegExp.test() code to improve efficiency.
  4. Added important comments in the files wherever necessary.

Answers to the Questions Asked in the PR Description:

  1. There was a function named (resolvePathsForElementsWithAttribute) which was common in both the files. The exact code which was same in both the files was : - export function resolvePathsForElementsWithAttribute(attr, sketchDoc, files) { const elements = sketchDoc.querySelectorAll([${attr}]); const elementsArray = Array.prototype.slice.call(elements); elementsArray.forEach((element) => { if (element.getAttribute(attr).match(MEDIA_FILE_REGEX)) { const resolvedFile = resolvePathToFile(element.getAttribute(attr), files); if (resolvedFile && resolvedFile.url) { element.setAttribute(attr, resolvedFile.url); } } }); }

  2. Some parts of the code were similar but had slight differences due to the client vs. server environment requirements.

  3. Since the /embed/ endpoints are currently disabled due to phishing concerns, the server-side version of this code may no longer be necessary. If its primary purpose was handling these endpoints and they are no longer in use, it would make sense to remove it unless there is a plan to re-enable them securely.

I have verified that this pull request:

  • has no linting errors (npm run lint)
  • has no test errors (npm run test)
  • is from a uniquely-named feature branch and is up to date with the develop branch.
  • is descriptively named and links to an issue number, i.e. Fixes #123

@Harshit-7373 Harshit-7373 changed the title Removed code duplication between server previewGeneration.js and clie… Removed code duplication between 2 files. Mar 5, 2025
@Harshit-7373
Copy link
Contributor Author

@raclim @lindapaiste I have opened this PR for issue #2805 . Please take a look at it, as it will resolve the issue.

@Jatin24062005
Copy link
Contributor

Hey @Harshit-7373, The duplication removal looks good to me, but there are still several tasks outlined that need to be addressed. Specifically:

Add Explanatory Comments: The code needs better documentation. Please add comments explaining the logic behind complex sections of the code for better readability.

Add Unit Tests: The code should have corresponding unit tests to ensure functionality is verified. This is crucial for maintaining code quality and stability.

Code Improvements: Consider enhancing the code by using RegExp.test() instead of match() !== null. This change would improve the clarity and performance, as test() is faster and more concise.

Additionally, please verify whether these functions are referenced in any other files to avoid potential issues.

@Harshit-7373
Copy link
Contributor Author

@raclim @Jatin24062005 Due to some issues, I had to close this pull request. However, I have opened a new PR with all the required changes, including tests, explanatory comments, and necessary code improvements. Please review it once.

Here is the Link to the new PR : - #3384

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Repeated code between server previewGeneration.js and client EmbedFrame.jsx
3 participants