trjhtr

I'm beginning my programming adventure in C++. I want to write a small program, which has to create an array in function create (size has to be defined in create function) and transfer them to function display which will print entered values.

#include <iostream>

using namespace std;

int *create(int & n);
void display(int * arr, int n);

int main()

 int n;
 int * arr = create(n);
 display(arr, n);


 delete arr;

 return 0;


int *create(int & n) 
 cout << "How many elements? ";
 cin >> n;
 int * arr = new int[n];
 int element;
 for(int i = 0; i < n; i++) 
 cout << endl << "Enter " << i << " array element: ";
 cin >> element;
 arr[i] = element;
 

 return arr;


void display(int * arr, int n) 
 for(int i = 0; i < n; i++) 
 cout << endl << i << " element: " << arr[i];
 

I'm looking for every criticism, optimization suggest and everything which will help me be a better programmer and not to learn bad habits from the beginning.

First, the usual criticisms:

• Don't use using namespace std;
  


• Don't use raw new and delete
  


• Include the headers your code uses (e.g. <iostream>)


• Prefer prefix ++i over postfix i++ as a general habit


• Prefer to omit the implied return 0; from main (unless you're trying to emphasize that the return value is different via different codepaths, which doesn't apply in this case)

Then, the point specific to your API design:

• Your function create has two outputs: the pointer to a new array and the size of the new array, n. The former is returned as the return value; the latter is passed in by reference (the out-parameter pattern). Instead of returning these two pieces of information by two different routes, you should unify them: Either pass in two out-parameters (yuck!) or pass out two values by return.

Taking the latter approach (two values by return) and applying all our rules except "no raw new and delete", we end up with this:

#include <iostream>
#include <utility>

std::pair<int *, int> create() 
 int n;
 std::cout << "How many elements? ";
 std::cin >> n;
 int *arr = new int[n];
 for (int i = 0; i < n; ++i) 
 int element;
 std::cout << std::endl << "Enter " << i << " array element: ";
 std::cin >> element;
 arr[i] = element;
 
 return arr, n;


void display(int *arr, int n) 
 for (int i = 0; i < n; ++i) 
 std::cout << std::endl << i << " element: " << arr[i];
 


int main() 
 auto [arr, n] = create(n);
 display(arr, n);
 delete arr;

The JavaScript-looking [arr, n] is C++17 syntax, but don't worry, we're about to get rid of it in our next refactor.

Applying "no raw new and delete" allows us to simplify much further:

#include <iostream>
#include <vector>

std::vector<int> create() 
 int n;
 std::cout << "How many elements? ";
 std::cin >> n;
 std::vector<int> result;
 for (int i = 0; i < n; ++i) 
 int element;
 std::cout << std::endl << "Enter " << i << " array element: ";
 std::cin >> element;
 result.push_back(element);
 
 return result;


void display(const std::vector<int>& vec) 
 for (int i = 0; i < int(vec.size()); ++i) 
 std::cout << std::endl << i << " element: " << vec[i];
 


int main() 
 auto vec = create(n);
 display(vec);

The last thing I'd consider doing is eliminating mutation from the code. You've got these variables int n and int element that aren't initialized, and then you use std::cin >> ... to mutate their values. It might be more straightforward to write an "int-getting function" and use it consistently. Plus, this gives us a place to hang the missing std::flush:

#include <iostream>
#include <vector>

template<class T>
T input(const char *prompt) 
 T result;
 // Print out everything in `prompt` (C++17 syntax)
 std::cout << prompt << " " << std::flush;
 std::cin >> result; // Should we add some error-checking?
 // What if the user types "hello world" here?
 return result;


std::vector<int> create() 
 std::vector<int> result;
 int n = input<int>("How many elements?");
 for (int i = 0; i < n; ++i) 
 result.push_back(input<int>("Enter the next array element:"));
 
 return result;


void display(const std::vector<int>& vec) 
 for (int i = 0; i < int(vec.size()); ++i) 
 std::cout << std::endl << i << " element: " << vec[i];
 


int main() 
 display(create(n));

Notice that I changed your prompt from "Enter the 42 array element:" to simply "Enter the next array element:". This was because I didn't want to bother explaining how to write a variadic-template version of input that would accept input<int>("Enter the ", i, " array element:"). It's possible — even trivial, in C++17 where we can write (std::cout << ... << prompt_pieces) << " " << std::flush — but I want to keep the simple code simple.

Now we've localized all the icky "state mutation" into the innards of input<int>; we don't have to think about mutation while we're reading create.