0

My name is Matt. I'm new to StackOverflow and am fairly new to C++. Currently working my way through C++ Primer by Lippman.

I'm doing an exercise in the book, and task is to read integers in to the vector, and then multiply those integers by doing first and last, second and second to last, third and third last etc.

I did it myself without looking anything up, or else I'd barely learn if I just copied... my program compiles and acts as expected. My question is: did I do this correctly? is there a more efficient way of doing it?

I not only want to learn how to make working code, but I want to do it correctly. Thank you in advance!

#include <iostream>
#include <string>
#include <vector>
#include <cctype>

using std::cout; using std::cin; using std::vector; using std::endl;
using std::string;


int main()
{
    vector<int> numbers;
    int usernum = 0;

    cout << "Enter some numbers: ";
    while (cin >> usernum)
    {
        numbers.push_back(usernum);
    }

    unsigned maxElement = numbers.size() - 1;
    unsigned minElement = 0;

    for (auto i : numbers)
    {
        cout << numbers[minElement] << " * " << numbers[maxElement] << " = " << numbers[minElement] * numbers[maxElement] << "\n";
        ++minElement;
        --maxElement;
    }

    return 0;
}
MKSnazzy
  • 71
  • 1
  • 1
  • 10
  • 4
    If you have working code, and you're looking for advice on how to improve it, [Code Review](https://codereview.stackexchange.com/) may be a better site to post your question. – milleniumbug Sep 12 '16 at 19:36
  • And your for loop goes over the entire vector, thought it only needs to go over half of the vector. – Some Guy Sep 12 '16 at 19:38
  • @SomeGuy can you tell me how do it like that? Also, I've noticed (and this may be what you mean, that it multiplies everything twice putting the operands on different sides. How would I do a condition that stops the loop once they have been multiplied once? – MKSnazzy Sep 12 '16 at 19:40
  • 1
    If your program works correctly, this will be a good post at http://codereview.stackexchange.com. – R Sahu Sep 12 '16 at 19:41
  • @RSahu Yeah, I've noticed it doesn't actually work as fully expected. It goes through vector and multiplies everything twice as maxElement goes right through to beginning, and minElement goes through to end. Not sure how to stop it once it's only done the operation on each one time – MKSnazzy Sep 12 '16 at 19:47

2 Answers2

1

In a comment, you said:

I've noticed it doesn't actually work as fully expected. It goes through vector and multiplies everything twice as maxElement goes right through to beginning, and minElement goes through to end. Not sure how to stop it once it's only done the operation on each one time.

If you don't want to repeat the multiplications, you need to change the for loop a bit.

for ( ; minElement <= maxElement; ++minElement, --maxElement)
{
    cout << numbers[minElement] << " * " << numbers[maxElement] << " = " << numbers[minElement] * numbers[maxElement] << "\n";
}

PS

When you use this logic, you'll need to make sure that minElement and maxElement are of a signed type. Otherwise, you will run into problems if numbers has only one element.

R Sahu
  • 204,454
  • 14
  • 159
  • 270
  • Thank you! I Works great.. appreciate it. – MKSnazzy Sep 12 '16 at 19:53
  • So the middle element of a vector with an odd number of elements is squared? – nonsensickle Sep 12 '16 at 20:02
  • sorry, I don't understand. I know signed is for being able to have negative values, but how does it work here? Also, if you don't mind, could you tell me for maxElement why i had to have -1 for size. I did it but don't know why vectors need that -1. – MKSnazzy Sep 12 '16 at 20:03
  • @nonsensickle, with my answer, yes. Not sure whether that is the OP's intention. – R Sahu Sep 12 '16 at 20:03
  • @Matt, if `numbers` has only one element, `maxElements` starts out being zero but in the next step, by executing `--maxElements`, it will morph to a large number instead of being set to `-1`. – R Sahu Sep 12 '16 at 20:06
  • @nonsensickle It's not my intention to have an odd number of elements. Actually trying to figure out a solution for an error if it's odd number of elements. Was going to try using modulus on both variables to see if it amounts to 0 but not sure if that's proper or how to set it up. Any suggestions? – MKSnazzy Sep 12 '16 at 20:09
0
  1. The first thing that feels strange for me is the namespaces you are using. Instead of doing: using namespace std::vector; you can fairly just do using namespace std; because you call std::vector anyways: vector<int> numbers;***. This applies to any "used" namespace you work with. Just do using namespace std; once and for all. ***I am unsure that std::vector/std::cout/... is even a namespace. std - is a namespace. std::vector should be a class under std namespace:

namespace std { template<typename T> class vector<T> {...}; }

  1. How come it "acts as expected". I do not get an idea of this loop: while(cin >> usernum). How do you know when the user input is finished from that point? For a first glimpse (did not compile/run it myself) I expect it either:

    • not compile
    • crash at runtime or having undefined behaviour
    • run the while-loop infinitely

  2. Use this instead: for (int i = 0, end_of_vector = numbers.size(); i < end_of_vector/2; i++) { cout << numbers[i] << " * " << numbers[end_of_vector - 1 - i] << " = " << numbers[i] * numbers[end_of_vector - 1 - i] << "\n"; } Reasons:

    • In this case you do not need any special variables to store first and last vector indexes.
    • You iterate only through a half of an array.
    • Using for (auto i : numbers), is is expected to use i as an element of numbers vector. But you do not do this, instead, you use numbers as is. Thus, this for-loop is ambiguous
andrgolubev
  • 825
  • 6
  • 21
  • Thanks for your reply. I use the individual namespaces as that is what the book C++ Primer instructed rather than just using everything under std. I'm trying to wrap my head around your code. Thank you so much for your time. – MKSnazzy Sep 12 '16 at 20:14
  • Also, I know that my code isn't perfect in terms of conditions to break loop. It's just a console program so I use ctrl+z. I'm just trying to do the main task that the book asks. I know that doesn't lessen the importance, but the main tasks are hard enough for me haha – MKSnazzy Sep 12 '16 at 20:16
  • I suggest to check if these namespaces even exist. Personally, it is the first time I see such namespaces. There is a reason to use special namespaces instead of plain std, of course, to lessen the amount of things that become visible to your sources. But for me it is just strange to see such practice. – andrgolubev Sep 12 '16 at 20:27
  • I guess the reason is just to name what you're using to lessen the chance of a clash or something so just declare what you're using instead of everything. Not saying you're wrong that is just my interpretation from the book C++ Primer which I'm reading because it's one of highest regarded C++ books – MKSnazzy Sep 12 '16 at 20:37
  • @Matt your book is more right than wrong on this. After reading [Why is “using namespace std;” considered bad practice](http://stackoverflow.com/questions/1452721/why-is-using-namespace-std-considered-bad-practice), the practice of staying on the side of caution will make more sense. (you may have to read it a few times). – WhozCraig Sep 12 '16 at 20:42
  • I don't understand why you divide by 2 and -1 then - i. Could you help me understand if you don't mind? I know vectors are very important so I want to understand rather than just copy your code and not realize why it works. Thank you. – MKSnazzy Sep 12 '16 at 20:45
  • @WhozCraig thanks for the link. Yeah after reading some of it I think I might just start writing std:: prefix. – MKSnazzy Sep 12 '16 at 20:58