View Issue Details

IDProjectCategoryView StatusLast Update
0015990ScribusGeneralpublic2019-12-12 17:48
Reporterale Assigned To 
PrioritynormalSeverityfeatureReproducibilityN/A
Status newResolutionopen 
Product Version1.5.6.svn 
Summary0015990: StoryText::indexOf(): document what it does and what the plen output argument is
Descriptionin text/storytext.h, please put a short description of what indexOf() does and please document what plen contains (difference to the int result returned and when to use the one or the other)
TagsNo tags attached.
PatchNo

Activities

jghali

2019-12-09 23:51

administrator   ~0047234

The pLen argument is named according to Hungarian notation (https://en.wikipedia.org/wiki/Hungarian_notation). The first lower case letter indicates this is a pointer, the following letters "Len" indicate this variable represents a length.

As this is already documented in storytext.h, the indexOf() methods allows to find specific text in the corresponding StoryText object. The pLen argument enable to retrieve the length of found text. Due to some subtlety in languages such as Arabic, this length may indeed not be the same as the length of searched text. Both indexOf() methods returns, as their names indicate it, the index of found text.

ale

2019-12-12 15:47

manager   ~0047253

scribus does normally not use hungarian notation.
in this case i i think that outMatchLength would be a better name.
(of course, after an attentive look at the function i did understand what the function was doing... but it's the goal of good variable names and the docstring to make the life of the developers easier...)

if the function, the arguments and the variables are carefully named, i think that we can often avoid docstrings.
on the other side, if we have a function that has the same name as the "parent" one but does not just forward to the original implementation, it's imo important to list the differences and allow new contributors to understand what happens.

with a signature like

int indexOf(const QString &searchString, int from = 0, Qt::CaseSensitivity cs = Qt::CaseSensitive, int* outMatchLength = 0) const;

we could remove from the docstring what is now obvious and edit it to say what is so special to this indexOf() that we can't simply use QString::indexOf().

jghali

2019-12-12 17:07

administrator   ~0047254

>> in this case i i think that outMatchLength would be a better name.

In this case, there is no need to even look at the variable name to know that it is an output argument because there is not much reason to pass an int pointer to a function other than to retrieve an output value...

ale

2019-12-12 17:24

manager   ~0047255

in modern c++ (and in in qt for many years) it's not common to have output arguments.

it's sometimes necessary, sometimes handy, but i would always always clearly document such cases.

of course, people with a c background and people having many years of c++ on their shoulders will recognize it immediately, but not programmers that come from other languages like java or python... or have been learning c++ during the last few years.

if we want new contributors, i think that we should make their life as easy as possible...
and not all of them will be seasoned c(++) programmers...

jghali

2019-12-12 17:48

administrator   ~0047258

>> in modern c++ (and in in qt for many years) it's not common to have output arguments.

Complete bullshit. You just have to look at Qt documentation to see that output argument are regularly used when needed. And doing so is in fact usually more efficient than returning a tuple or a structure/class, especially when you deal with vectors or other STL containers... Before C++17 and structured binding, using tuples as return values is also rather inconvenient.

Issue History

Date Modified Username Field Change
2019-12-06 16:55 ale New Issue
2019-12-09 23:51 jghali Note Added: 0047234
2019-12-12 15:47 ale Note Added: 0047253
2019-12-12 17:07 jghali Note Added: 0047254
2019-12-12 17:24 ale Note Added: 0047255
2019-12-12 17:48 jghali Note Added: 0047258