2
\$\begingroup\$

How may this code be improved upon, the user inputs array of specified length and replaces each element that is smaller than the mean of the first and last element with the mean.

Is there a better way to pass the array to the function (ref/ptr, etc.), or more efficient way to manage memory (when handling array)?

I know the vector container from STL is ideal but just wanted to gain some insight on fundamentals.

#include <iostream>
using namespace std;

void replaverage(int arr[], size_t n){
    cout<<"Enter array length"<<endl;
    cin>> n;
    for(int i=0; i<n; i++){
        cout<<"Enter the numbers"<<endl;
        cin>>arr[i];
        cout<<endl;
    }
    int f=arr[0];
    int l=arr[n-1];
    double av=(f+l)/2;
    for(int j=0; j<n; j++){
        if(arr[j]<av){
            arr[j]=av;
        }
    }
    for (int i=0; i<n; i++){
        cout<<arr[i]<< " ";}
}


int main(int argc, char* argv[]){
    size_t x=10;
    int arr[x];
    replaverage(arr,x);
    return 0;
} 
\$\endgroup\$
2
  • \$\begingroup\$ In c++ you should use a vector<int> instead of a raw c-style array. \$\endgroup\$ Feb 17 '20 at 0:11
  • \$\begingroup\$ Welcome to Code Review! I have rolled back your last edit. Please do not update the code in your question to incorporate feedback from answers, doing so goes against the Question + Answer style of Code Review. This is not a forum where you should keep the most updated version in your question. Please see what you may and may not do after receiving answers. \$\endgroup\$
    – Heslacher
    Feb 17 '20 at 5:06
2
\$\begingroup\$

Something to keep in mind is that this statement is truncating data:

        arr[j]=av;

because it is assigning a double to an int.

As was mentioned in a comment there are c++ container types that would be better than a old C style array, two of these are std::array<type, arraySize> and std::vector<type>. std::array is a fixed size and std::vector is a variable size. Both would allow the use of iterators that might simplify the code.

Avoid using namespace std;

If you are coding professionally you probably should get out of the habit of using the using namespace std; statement. The code will more clearly define where cout and other identifiers are coming from (std::cin, std::cout). As you start using namespaces in your code it is better to identify where each function comes from because there may be function name collisions from different namespaces. The identifiercout you may override within your own classes, and you may override the operator << in your own classes as well. This stack overflow question discusses this in more detail.

std::endl

It is better for performance if "\n" is used rather than std::endl in output, std::endl flushes the output buffer as well as putting out a new line, and this can slow down loops.

Complexity

The function replaverage should really be 2 functions, one to input the data and a second to process the data. Perhaps a third to print the data.

Horizontal Spacing

It would be better if there were spaces around operators that separate operands.

#include <iostream>
#include <vector>

std::vector<int> getData()
{
    int n;
    std::vector<int> intputData;
    std::cout << "Enter array length\n";
    std::cin >> n;
    std::cout << "Enter the numbers\n";
    for(int i = 0; i < n; i++){
        int tmpIn;
        std::cin >> tmpIn;
        intputData.push_back(tmpIn);
    }

    return intputData;
}

void modifyData(std::vector<int> &data)
{
    int f=data[0];
    int l=data[data.size() - 1];
    double av = (f + l)/2;
    for (auto j: data)
    {
        if(j < av){
            j = av;
        }
    }
}

void printData(std::vector<int> &data)
{
    for (auto i: data)
    {
        std::cout << i << " ";
    }
}

int main(int argc, char* argv[]){

    std::vector<int> data = getData();
    modifyData(data);
    printData(data);

    return 0;
}
\$\endgroup\$
1
  • \$\begingroup\$ The two places where std::endl are used both immediately precede reading from std::cin, where a flush is clearly desirable. That said, I would prefer to see it written explicitly (...\n" << std::flush) to ensure readers know it's intended and not just a newline. \$\endgroup\$ Feb 17 '20 at 11:03
1
\$\begingroup\$

Always check input succeeded.

std::cin >> n;
    std::cin >> arr[i];

In both these lines, we ignore all errors, and will produce the wrong output without any warnings, and happily return 0 from main(). This is bad for any program used as a processing step (e.g. driven by Make).

\$\endgroup\$
3
  • \$\begingroup\$ Check success of input via try-catch or throws? \$\endgroup\$ Feb 19 '20 at 9:14
  • \$\begingroup\$ Yes, you can check using exceptions, if you set the stream to throw on error (using std::cin::exceptions(std::ifstream::eofbit | std::ifstream::badbit), for example). Or check explicitly with if (!std::cin) { handle errors; }. \$\endgroup\$ Feb 19 '20 at 9:19
  • \$\begingroup\$ cin.fail() seems to be a good solution. \$\endgroup\$ Feb 19 '20 at 9:39
1
\$\begingroup\$

I restructured the code into object oriented format. Incorporated changes suggested by pacmaninbw.

#include <iostream>
#include <vector>

class replace_avg{

    private:
        int n;
        int tmpIn;
        double f;
        double l;
        double av;
        std::vector<double> intputData;

    public:
    std::vector<double> getData()
    {
        std::cout << "Enter array length\n";
        std::cin >> n;
        std::cout << "Enter the numbers\n";
        for(int i = 0; i < n; i++){
            std::cin >> tmpIn;
            intputData.push_back(tmpIn);
        }
        return intputData;
    }


    void modifyData(std::vector<double> &inputData)
    {
        f = inputData[0];
        l = inputData[inputData.size() - 1];
        av = (f + l)/2;
        for (auto &j: inputData)
        {
            if(j < av){
                j = av;
            }
        }
    }

    void printData(const std::vector<double> &inputData)
    {
        for (auto &i: inputData)
        {
            std::cout << i << " ";
        }
        std::cout << '\n';
    }
};

int main(){

    replace_avg vect;
    std::vector<double> v1 = vect.getData();
    vect.modifyData(v1);
    vect.printData(v1);

    return 0;
}
\$\endgroup\$
1
\$\begingroup\$

Consider absl::Span<T>. (or boost's) It may one day be replaced with std::span

It's actually quite hard to make what you're asking for, despite this being extremely useful and desired for a very long time.

However, with it, you can write:

void ReadFromIntArray(absl::Span<int const> int_array);
ReadFromIntArray({1,2,3});
ReadFromIntArray(std::vector<int>{1,2,3});
ReadFromIntArray(std::array<int, 3>{1,2,3});
ReadFromIntArray(absl::MakeSpan(pointer_to_int_array, count_of_int_array));

There's tremendous downsides to using iterators or pointer+count. Pointer+count is wildly dangerous and severely impairs readability. Iterators impair readability further and may also obliterate an architecture; either the interface becomes tightly coupled with std::vector, or the interface requires a template parameter adding a huge unnecessary compilation cost to every client.


In the very distant future, ranges will make things much more readable without a performance cost, and algorithms will be thought of as performing transformations on logical sets of data rather than doing logic within a loop.

\$\endgroup\$
2
  • \$\begingroup\$ How does passing a container by const ref differ from passing a span of said container apart from elements being modifiable in the latter case? What’s difference between non-const ref to the container and a span? \$\endgroup\$ Feb 19 '20 at 16:52
  • \$\begingroup\$ A mutable ref to a container means the implementation can resize the container, unlike a span. However a span of mutable items can modify the contents, unlike a const ref to a container. A span of const items is comparable to a const ref container, but works with any contiguous set of items in memory, not just one particular container type. \$\endgroup\$
    – butt
    Feb 19 '20 at 18:42

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.