View Issue Details
| ID | Project | Category | View Status | Date Submitted | Last Update |
|---|---|---|---|---|---|
| 0015990 | Scribus | General | public | 2019-12-06 16:55 | 2019-12-12 17:48 |
| Reporter | ale | Assigned To | |||
| Priority | normal | Severity | feature | Reproducibility | N/A |
| Status | new | Resolution | open | ||
| Product Version | 1.5.6.svn | ||||
| Summary | 0015990: StoryText::indexOf(): document what it does and what the plen output argument is | ||||
| Description | in 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) | ||||
| Tags | No tags attached. | ||||
| Patch | No | ||||
|
|
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. |
|
|
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(). |
|
|
>> 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... |
|
|
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... |
|
|
>> 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. |
