Bug 155630 - Deduplicate copy/paste code
Summary: Deduplicate copy/paste code
Status: NEW
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: LibreOffice (show other bugs)
Version:
(earliest affected)
unspecified
Hardware: All All
: medium normal
Assignee: Not Assigned
URL:
Whiteboard: target:7.6.0 target:24.2.0
Keywords: difficultyMedium, easyHack, skillCpp, skillJava, topicCleanup
Depends on:
Blocks: Dev-related
  Show dependency treegraph
 
Reported: 2023-06-01 13:59 UTC by Hossein
Modified: 2023-07-20 00:30 UTC (History)
4 users (show)

See Also:
Crash report or crash signature:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Hossein 2023-06-01 13:59:57 UTC
Description.
Copy pasting the code to do (almost) similar things in different places of the code base is usually considered a bad practice. There are many constructs in C/C++/Java and other programming languages that let the programmer to avoid duplicate code.
This is the task to find duplicate code in LibreOffice, and de-duplicate them by introducing new functions that do the shared things, and other similar tricks.

Finding duplicate code
Finding parts of the code that are similar to other parts is not an easy task if you do it manually. But, using appropriate tool it can be much easier. One of such tools that can find duplicate code is PMD:

PMD Source Code Analyzer:
https://pmd.github.io/
https://github.com/pmd/pmd

You can use this tool to find the duplicate code, and then try changing it to remove the duplicate parts.

Changing the code
One of the ways to de-duplicate the code is to move the shared part of the code to a new function, and then call it in place of the duplicated code.

Examples
This is a reboot of the previous task tdf#39593, so you can refer to the example commits from the above issue. As an example, you can see this commit among them:

tdf#39593 vcl/win/gdi: extract brush updating into method
https://git.libreoffice.org/core/+/7311a88baa8c30eeb61d897f43ac3f5b481ed01f%5E%21

Note: You have to make sure that the new code has the same functionality as the old one, and it does not cause bad impact on performance.
Comment 1 Xisco Faulí 2023-06-01 14:21:14 UTC Comment hidden (obsolete)
Comment 2 Hossein 2023-06-01 14:52:43 UTC Comment hidden (obsolete)
Comment 3 Commit Notification 2023-06-04 05:48:55 UTC
Julien Nabet committed a patch related to this issue.
It has been pushed to "master":

https://git.libreoffice.org/core/commit/befc6ac8e20863c913da0fd0bac2fa26bab1b025

Related tdf#155630: deduplicate in starmath/visitors

It will be available in 7.6.0.

The patch should be included in the daily builds available at
https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
https://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.
Comment 4 Commit Notification 2023-06-04 07:59:09 UTC
Julien Nabet committed a patch related to this issue.
It has been pushed to "master":

https://git.libreoffice.org/core/commit/7932427a39e684d78f2108d14660288a46f42df9

Related tdf#155630: deduplicate in starmath/visitors (2)

It will be available in 7.6.0.

The patch should be included in the daily builds available at
https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
https://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.
Comment 5 Xisco Faulí 2023-06-05 09:17:45 UTC
Hi Hossein,
Please, do not forget to add the relevant keywords for this easyhack, otherwise it won't be listed in the wiki
Comment 6 Hossein 2023-06-05 09:45:07 UTC
(In reply to Xisco Faulí from comment #5)
> Hi Hossein,
> Please, do not forget to add the relevant keywords for this easyhack,
> otherwise it won't be listed in the wiki
Thank you. It should be fine now.
Comment 7 Dave Gilbert 2023-07-01 14:08:33 UTC
I've played a bit with pmd, and it's reasonably nice, but hmm; some notes:

a) Throwing the whole LO source at it makes it run out of Java stack space; passing a subset works; but then you do want to pass a subset where you're likely to find commonality between them (as well as internally to one dir)

b) It seems to get quite confused on big chunks of data

c) You've got to be really careful not to miss subtle differences after the bit it spots - e.g. it spots that two functions start the same, so you decide to merge them, but you really need to check that there's not some small, crazy difference.

d) There's some art to trying to decide where a merged piece of code will live.


See my set at:
https://gerrit.libreoffice.org/c/core/+/153826
Comment 8 Commit Notification 2023-07-03 19:34:14 UTC
Dr. David Alan Gilbert committed a patch related to this issue.
It has been pushed to "master":

https://git.libreoffice.org/core/commit/32fd602d3918be032d79322f76021c6ebc27ccba

tdf#155630 Move sc's SetLineEnds into svx

It will be available in 24.2.0.

The patch should be included in the daily builds available at
https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
https://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.
Comment 9 Commit Notification 2023-07-03 19:35:16 UTC
Dr. David Alan Gilbert committed a patch related to this issue.
It has been pushed to "master":

https://git.libreoffice.org/core/commit/f239b3dec1acbe7ad70df31e8264bb16ac095502

tdf#155630 Use common SetLineEnds in sw

It will be available in 24.2.0.

The patch should be included in the daily builds available at
https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
https://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.
Comment 10 Commit Notification 2023-07-03 20:22:22 UTC
Dr. David Alan Gilbert committed a patch related to this issue.
It has been pushed to "master":

https://git.libreoffice.org/core/commit/a780c820f5cf3bfbb5d88bd5c6ca670e0a0c730f

tdf#155630 Use common getPolygon for sd

It will be available in 24.2.0.

The patch should be included in the daily builds available at
https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
https://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.
Comment 11 Dave Gilbert 2023-07-20 00:30:14 UTC
Here's some notes from potential dupes I'd found if someone else would like to attack them.
Note you probably need to check these things with people who know the relevant code, this is just me noticing things:

a) Import GDI/PDF
  svx/source/svdraw/svdfmtf.cxx and svx/source/svdraw/svdpdf.cxx
  have ImpSdrGDIMetaFileImport::InsertObj and ImpSdrPdfImport::InsertObj  which are big and almost identical.  It feels like you could make a base class and share large chunks of those two.  You can trigger it by inserting a pdf into a draw document, right clicking and selecting 'break'.
  My only worry is that this PDF import seems entirely separate from the loading PDF files at startup; so maybe that['s what really needs merging - not dug yet.

b) Unicode character tools in  sw/source/core/text/portlay.cxx and editeng/source/editeng/impedit3.cxx
  These have been explicitly copied between them; I wonder if they can be shared?
  (In particular all the isBehChar and related stuff).