-1
\$\begingroup\$

The following is a full example to illustrate my question:

#define MAX_LENGTH  32
#define ITEM_COUNT  3

using namespace std;


void doSomething(char ** name, int items)
{

    *name = new char[MAX_LENGTH * items];

    strncpy_s(*name, MAX_LENGTH, "Oracle", 7);
    strncpy_s(*name + MAX_LENGTH, MAX_LENGTH, "Apple", 5);
    strncpy_s(*name + MAX_LENGTH * 2, MAX_LENGTH, "Dell", 4);

}

int _tmain(int argc, _TCHAR* argv[])
{
    char * name = 0;

    doSomething(&name, ITEM_COUNT);

    // Approach 1
    for (int i = 0; i < ITEM_COUNT; i++)
    {
        cout << name + (i * MAX_LENGTH) << endl;
    }

    cout << endl; // blank line

    // Approach 2
    for (int i = 0; i < ITEM_COUNT; i++)
    {
        cout << &name[i * MAX_LENGTH] << endl;
    }

    cout << endl;

    return 0;
}

My question is which approach is better to read back the name in main function. One way is to treat name as an array, another is to do pointer arithmetic.

I like the pointer arithmetic more in this case because that make more sense tome because it is not defined as an array but than it really is same as array. Is there any other consideration which is better?

\$\endgroup\$
  • 1
    \$\begingroup\$ You are using name to store an array or array of char, but declared it as a 1D array of char. This is problematic (incorrect usage for pointers) Since this is C++ recommend using std::vector<std::string> (or std::vector<std::vector<char> > if you need to deal with raw char). Pass by non-const reference (instead of pointers) \$\endgroup\$ – Chintan May 17 '16 at 16:40
  • \$\begingroup\$ @Chintan This part is actually given and beyond my control. I have simulated this in the example above but this MFC RFX_Text_Bulk macro which does this thing. I am focusing on how to read the name when its filled up. \$\endgroup\$ – zadane May 17 '16 at 17:12
  • 1
    \$\begingroup\$ @Chintan It is not an array of array of char. It is a pointer to a pointer. Nothing problematic here. \$\endgroup\$ – vnp May 17 '16 at 17:32
  • \$\begingroup\$ @vnp: Its problematic in that the technique is fragile at best. \$\endgroup\$ – Martin York May 18 '16 at 5:45
  • \$\begingroup\$ @zadane: The two methods are identical. The standard defines x[y] as *(x+y). Therefore: name[i * MAX_LENGTH] => *(name + (i * MAX_LENGTH)). So the best technique is the one that reads the best to your eye (ie it is totally cosmetic). \$\endgroup\$ – Martin York May 18 '16 at 5:46
3
\$\begingroup\$

Avoid the array form of new

In my view, code of the form foo = new T[N]; is right up there with mutable global variables and gotos for instantly setting off alarms. In truth, no, it's not quite equal to either global variables or goto statements--although both of those set off alarms and need to be examined carefully, there are still situations where either or both can be justified. I'm reasonably certain there is not a situation where the array form of new can really be justified.

If you're thinking of using new T[N], you probably want a container such as std::string, std::vector or std::array. In this case, given that your contents are of fixed, constant size, std::array might make sense. On the other hand, it looks to me like the fixed, constant size is primarily accidental--probably not something that was really intentionally designed, just something that happened because dealing with variable lengths would have added even more ugliness and complexity to code that already has far too much of both.

Avoid magic numbers

Right now, you're supplying the lengths of the strings manually when you call strncpy_s. This is error-prone (and, in fact, it looks like you probably have an error--you specify the length of "Oracle" as 7, when it's actually 6).

Avoid C-style strings

Using an std::vector<std::string>, this all becomes almost stupidly simple:

std::vector<std::string> names { "Oracle", "Apple", "Dell" };

for (auto const &s : names) 
    std::cout << s << "\n";

Note that there's no use of either array or pointer style notation throughout--both are rendered superfluous.

Don't go ab-using the language

The line:

using namespace std;

...is seen in a lot of beginner's books (and such), but it's an almost spectacularly bad idea in practice. If you insist on doing it at all, at least protect yourself a little bit by putting it in an outer scope, and nesting all your code in a separate scope inside it.

int main() { 

    using namespace std;

    {
         // Your code here
    }
}

This way if main defines something with the same name as something from namespace std;, your name will be the one that's found instead of creating an ambiguity.

Two notes:

  1. It's generally better to just avoid the problem entirely.
  2. This is nearly unique to the std namespace--doing the same with other namespaces is much less problematic.

Avoid std::endl

Most people who use std::endl are just using it to write a new-line to a stream. Unfortunately, along with writing a new-line, std::endl also flushes the stream. This can lead to substantial slow-downs, usually without accomplishing anything useful at all. When you want a new-line, just write a new-line and be done with it.

Avoid object-like macros

The preprocessor supports giving symbolic names to values, like:

#define MAX_LENGTH  32
#define ITEM_COUNT  3

These have a number of problems though, such as the name not being visible during debugging, rendering the code substantially less readable. For this case, you can just use a const variable instead:

const int max_length = 32;
const int item_count = 3;

See previous comment though--in this case, the numbers are artifacts of other shortcomings in the code; when the rest of the code is improved, the need for these will almost certainly disappear.

Avoid arbitrary limits

The numbers mentioned in the previous point are problematic for another reason as well: they represent arbitrary limits, in this case on the number and size of items you can store in your collection. Again, if we just use std::vector<std::string>>, the need/use for these simply disappears.

|improve this answer|||||
\$\endgroup\$
  • \$\begingroup\$ Thanks for the insight. It doesn't quite addressed referencing as pointer or offset but you had a lot of insight on other issues. \$\endgroup\$ – zadane May 20 '16 at 17:10
  • \$\begingroup\$ @zadane: I specifically avoided addressing that, because 1) it's not really related to this code (remember, the site is about reviewing code), and 2) people have been arguing about it almost as long as C has existed--I have nothing new to add, and no hope that the question will be resolved. \$\endgroup\$ – Jerry Coffin May 20 '16 at 22:44

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.