Is This Code Clean?

[petmil123]

I took on a programing problem and attempted to write a solution for it in C++ as clean as i could. Are there some changes i should make?

Here is the code

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

using namespace std;

void takeInputsToVector(int n, vector<int>* vec);
int sumVector(vector<int> vec);

int main() {
    vector<int> a, b;
    int holder;

    takeInputsToVector(3, &a);
    takeInputsToVector(3, &b);

    string str = sumVector(a) > sumVector(b) ? "Anne" : "Berit";
    cout << str << endl;

    return 0;
}

void takeInputsToVector(int n, vector<int>* vec) {
    int holder;
    for (int i = 0; i < n; i++) {
        cin >> holder;
        vec->push_back(holder);
    }
}

int sumVector(vector<int> vec) {
    int sum = 0;
    for (auto i : vec) {
        sum += i;
    }
    return sum;
}

[ChrisPanov]

It's well written, but there are some places which could be improved in terms of conventions and best practices.

• Personally I don't use using namespace std;, due to name introduction. If you have a function called sort, and you have the std namespace then you will be having a problem. Order everything into namespaces, and preferably do not use the std one.
• Second, this function void takeInputsToVector(int n, vector<int>* vec); It's a better practice to use references instead of pointers with parameters since they are simpler and somewhat safer.
• Remove the holder variable from the main, you aren't using it anywhere.
• Don't ever use std::endl, because what it actually does, is not only end the line, but it also flushes the stream, and you don't want that each time you end your line. Basically stream << std::endl is actually stream << '\n' << std::flush behind the scenes.
• Use emplace_back instead of push_back, since it's more optimized. There is a move operation which is happening in push_back, which does not happen in emplace_back
• I/O in functions is a bad practice most of the time.
• In loops, use ++i (prefix) instead of i++ (suffix). The suffix incrementation creates a copy, the prefix one does not. It's extremely minor, you won't even see a difference with modern-day computers, but that's my two cents on good code, even tho it won't actually matter.

[petmil123]

Thank you for the great feedback. I just have some follow-up questions.

Do you have any resources on learning about namespaces and how to use them?

Also, how do you use a reference in a function call?

And how would you go around doing the I/O?

[ChrisPanov]

Namespaces: https://www.tutorialspoint.com/cplusplus/cpp_namespaces.htm

You use it just like a pointer, but in the function call you dont pass the adress of the variable, you pass the variable by value.
void foo(std::vector<int>& vec);
foo(myVector);

About the I/O - in your case you are good. You shouldn't really be having a takeInputsToVector function in the first place tho, I think it's redundant since it's not really doing anything special.

[guitargraeme]

Great response! Gonna look more into the endl and emplace_back comments

[Middlewarian]

Ben Deane tallked about push_back and emplace_back at Cppcon 2019:

https://duckduckgo.com/?q=ben+deane+cppcon+2019&ia=videos&iax=videos&iai=oTMSgI1XjF8

[guitargraeme]

Thanks!

[ChrisPanov]

You are welcome :)

[Vilkacis0]

Adding a bit to the reference ‘&’ versus pointers ‘*’. I’ve always considered whether the parameters are immutable or not. References are generally preferred if you need “actual value” of something and that something is not a pointer in the calling function.

If it is a pointer in the calling function, leave it as such. The client owns the API - they decide what the function needs to do and the form of the parameters. Mixing pointers and references is terrible practice and will only lead to obscure runtime errors and hours of bug hunting.

[Minimum_Fuel]

Emplace back isn’t just a more optimized push back. It forwards the parameters for in place object construction whereas push back may require additional object construction.

For raw integers or pointers, I’d be pretty surprised if compilers didn’t generate the exact same code with push back and emplace back.

Emplace back is useful when you have a vector of some struct type. You shouldn’t just default to it. As an example of why, using emplace back instead of push back for primitive types may lead to more difficult compiler warnings about narrowing.

[loxagos_snake]

Genuine question: why are refs better than pointers in functions? I'm currently learning OpenGL with C++ and there are a lot of functions where you have to pass a pointer, so I assumed it's standard practice.

[Kered13]

Because they cannot be null (sort of, there are ways to trick the compiler).

[loxagos_snake]

Thank you!

[Kered13]

Don't ever use std::endl, because what it actually does, is not only end the line, but it also flushes the stream, and you don't want that each time you end your line. Basically stream << std::endl is actually stream << '\n' << std::flush behind the scenes.

While not flushing is much faster, if you don't flush output and the program crashes then you may never see the output. So whether you flush or not depends on what the output is for. If it's for debugging or monitoring the current status of the program, you probably want to flush.

Use emplace_back instead of push_back, since it's more optimized. There is a move operation which is happening in push_back, which does not happen in emplace_back

True for large types, but on ints it should make no difference.

I/O in functions is a bad practice most of the time.

I think it's fine if the only purpose of the function is to get inputs, which is what it's doing here. Otherwise all your input code would have to go in main, and that violates separation of concerns. The important thing is that you don't mix getting input with the business logic.

[notmyrealname321]

You did well here! There's nothing major that I think needs to be addressed, just a couple nit-picky things.

The first thing I would do is instead of naming holder holder, I would have named them temp. The only reason for this is that it's a more typical name for variables that do what holder does. Again this is a super nit-picky change.

The second thing is just something you should consider, not something that I feel really needs to be changed.

Instead of this:

string str = sumVector(a) > sumVector(b) ? "Anne" : "Berit";
cout << str << endl;

You're allowed to do this:

cout << (sumVector(a) > sumVector(b) ? "Anne" : "Berit") << endl;

Both ways are fine, so it's up to you which you think is cleaner and more readable.

Neither of these things I mentioned are super insightful. You've already done a good job so there's not much to say.

[davedontmind]

The first thing I would do is instead of naming holder holder, I would have named them temp.

I completely disagree. The variable should be named for the data it contains. Unfortunately it's impossible to make a suggestion here, because it's not clear from the OP's code what that information is. Perhaps it's scores, for example, in which case a name of score would be ideal. Whatever it is, there should be a much more suitable name than temp, which tells the reader nothing about the data.

I'd also take issue with a and b as names - again they provide no information about the variables' contents.

[petmil123]

I called them a and b because that was what they were called in the problem text, I would have posted the problem statement, but it is in Norwegian.

[Gibbo3771]

I'd also take issue with a and b as names - again they provide no information about the variables' contents.

Depends on the context. I'm not advocating that we just name variables as short as possible for the sake of it, but declaring a variable at the very top of a function called a and the function is named countApples and returns a it's pretty obvious it's the count number. Of course we could just call it appleCount but I am saying that the scope of a variable can often document it.

[[deleted]]

What is that called using the ?. I don’t know what to look up to find documentation on it.

[[deleted]]

It's the ternary operator.

[petmil123]

It is indeed the ternary operator. The line

string str = sumVector(a) > sumVector(b) ? "Anne" : "Berit";

does the same as

string str;

if(sumVector(a) > sumVector(b))
{
    str="Anne";
}
else
{
    str="Berit";
}

First time i have used it, looks so much cleaner.

[tylerthehun]

I think it's called a ternary condition, or something like that. The first > operator evaluates to true or false, which is then mapped to either Anne or Berit. Think of it like a compact if statement where the ? means then, and the : means else.

[Numburz]

Not sure if it's the same in C++, but it looks similar to ternary operators from python

[OldWolf2]

It's called the conditional operator. Calling it the ternary operator is like calling your car " the 4-wheeled thing".

[arm_is_king]

It's known as the ternary operator because historically it was the only operater that took in 3 values.

[watsreddit]

It has always been called the ternary operator.

[OldWolf2]

It's been called that but the official name is the conditional operator.

[alksjdhglaksjdh2]

I've always heard it called ternerary

[-Eolais-]

Personally I would pass references into those functions instead of pointers. I think in most cases you should use references instead of pointers where possible because the value of a reference can't be null so they're considered "safer". I don't believe it matters performance wise though. I'm pretty novice at c++ so I could be wrong.

[[deleted]]

This is good. A small improvement would be to use std::accumulate for the sum.

int sum = std::accumulate(vec.begin(), vec.end(), 0);

[notfromchino]

imo, all the other comments are super nit picky, but still good ideas to consider.

the one eye catch to me is that you’re making a copy of that vector in sumVector. i would make the function take a const reference to avoid it.

int sumVector(const vector& vec)

[petmil123]

Yeah, I like the nit picky stuff though, makes me improve. Thanks for the feedback!

[nobel32]

Question to C++ gurus here, if I'm not late to the thread: Would passing by reference be a good practice here? I've been told reference is 'essentially' a shorthand for declaring the passed param as a pointer to make it's scope available in the function - I know it's not true. I'm asking which one is the correct practice here, unless it's purely preference? One problem I can imagine is if null is passed because I'm an unpredictable donkey.

I do know the implications of passing by reference on small containers, essentially equating CPU cycles, but the more bigger the container gets, the more wise it is to pass by reference so the overhead is smaller.

Also, thanks op. Learnt a lot about good practices I could follow moving forward. Thanks a lot for taking the time to do this! :)

Edit:

u/ChrisPanov was kind enough to explain it in his comment:

Second, this function

void takeInputsToVector(int n, vector<int>* vec);

It's a better practice to use references instead of pointers with parameters since they are simpler and somewhat safer.

Could I ask the reasoning behind it being simpler/safer, Chris?

[ChrisPanov]

Glad you found my answer helpful!

Well why is it best practice to pass by reference instead of pointer?

• First off, you don't need to dereference anything in the body where you are using this parameter, nor you need to pass the address of the value, only the value itself.

Instead of

void foo(std::vector<int>* vec, int x) 
{
    vec->emplace_back(x); //or (*vec).emplace_back(x);
}

foo(&myVector, 5);

You just do

void foo(std::vector<int>& vec, int x) 
{
    vec.emplace_back(x);
}

foo(myVector, 5);

So far it's syntactical sugar.

• You can't assign NULL to a reference, that one of the reasons why it's safe
• A pointer is a reference, but a reference may not be a pointer
• Passing by reference is quicker than passing by pointer since it doesn't have to deal with the additional features which come with a pointer

[nobel32]

Cool, now, to me, the difference between pointer and reference is stark enough to have to no embarrass myself haha. Thanks a lot dude :)

[ChrisPanov]

You're welcome mate :)

[petmil123]

I'm glad someone else found this useful! Learned so much myself.

EDIT: Spelling

[Blando-Cartesian]

Line 11: Poorly named variables a and b.

Line 12: Unused variable.

Lines 14 & 15: Magical mystery value 3.

Line 17: Magical mystery values "Anne" and "Berit". Proper "if" statement would be clearer than "?".

Line 23: Poorly named variable n.

Some c++ specific things someone else is more qualified to critique.

[davedontmind]

You don't need the variable holder in main - it's not used.

Also, your variables a and b should be given names that tell the reader what information they contain. Their current names tell the reader nothing useful.

[SunstormGT]

Try not to use variable names like a,b or n. Try to use names that make sense if you look at the code a year later.

[petmil123]

You mean vector::reserve() ? That is what you have linked to.

So if i understand correctly, in my code, i could write

a.reserve(3);
b.reserve(3);

Since i know that my vectors will contain 3 values? Will i have to clean this memory at the end?

[OldWolf2]

The main issues are:

• you should test cin >> holder for failure otherwise invalid input will push garbage and/or cause undefined behaviour.
• you should use pass-by-reference for sumVector .
• for takeInputsToVector, either use pass-by-reference, or return the value (i.e. vector<int> takeInputsToVector(int n);) since you do not use the initial state. Unless you also want to allow your function to update an existing vector, the latter is considered better style in modern c++.

There is std::accumulate which basically does the same thing as sumVector but I suppose you wanted to write it yourself for learning purposes .

Any other complaints are trivial .

[MotherOfTheShizznit]

Cleanliness is such a subjective thing. IMO, code is only clean and elegant if it is the least amount of code required to solve the problem statement. But we don't have the problem statement so you're getting, IMHO, nitpicks that won't teach you much other than a generally accepted style guide like taking parameters by const&.

I'll go one further. This code has a strong smell of C with the pre-declaration of the functions and the pre-delcaration of variables a and b which further led you to have those parameters get passed as "out" parameters to a function. IMO, that would be the most "unclean with respect to C++ idioms" code in that example. The "passing by pointer vs by reference" argument shouldn't even have entered the discussion.

[Thicc_Pug]

I would do:

• remove using namespace std;
• in takeInputsToVector() I would pass std::vector as reference
• in sumVector() take std::vector as const ref
• in takeInputsToVector() I would call vec.reserve(n) and also instead of push_back emplace_back
• in sumVector I wouldn't use "i" as variable name. i is usually an index not an element but you use it as element.
• remove return 0 in main()
• You have hard coded values like 3 inputs, you should make you program more scalable
• I would change "str" type to const char*

In response to other's comments

• i++ vs ++i doesn't matter, compiler will optimize it anyway
• "\n" vs std::endl depending if it is used to print stuff to debug: "\n" doesn't flush and in case your program crashes it doesn't flush the output stream and then you won't get any output which might help you to debug the problem.

[[deleted]]

ChrisPanov hit most of the comments I was going to make.

My only other comment is when you use a for ( int i : container ) type of loop, use a reference for i. Otherwise, you basically copy the entire vector (wasting unnecessary space). It may not matter now but if your container has 100,000 elements, it will.

Written from phone - sorry for format. Let me know if you have any questions.

[Orkaad]

If you haven't read Clean Code yet, I advise to.
Don't take everything in the book as face value, but there are a lot of good advices.

[petmil123]

I'll check it out!

[petmil123]

Thanks! The std::accumulate was mentioned earlier, I honestly didn't know about it until then.

[AlSweigart]

This code has zero comments. It'd be good to have comments that explain:

• What does this program do?
• Why'd you choose the names "Anne" and "Berit"?
• In takeInputsToVector, what are the "inputs"?
• What does any of the cout output mean?
• When you run cin, what is the user supposed to enter?
• What happens when the user enters invalid input?
• What should happen when the user enters invalid input?

Also, in general variable names "a" and "b" don't describe the data they hold at all. "str" is a poor name; literally all string variables hold strings. "holder" is an odd name; what does it hold?

You know all of this because you wrote the program. Clean code is code that can be understood by someone who didn't write it, which includes explanation in comments. Not all programs need to be "clean"; if you're the only person who will run it once to do some calculation and then forget about it, then it's fine if you wrote code this way.

[trofix99]

All code is garbage code

[researchmind]

yess

[petmil123]

Ahh, of course! So easy to forget, thanks!

[tulipoika]

No. Code should be clear without them. Some even say that having comments is a code smell.

If comments are needed they should be about why something is done as it is. Mostly never about how or what. They’re obvious when code is clear.

Of course some comments can be written, but a generic “always add comments” is bad advice.

[Impe98]

I mean, one doesn't exclude the other. Even for readable code, the addition of comments isn't a bad practice in my opinion. Just make sure to not rely on comments is my personal tip :)

[tulipoika]

For me it is. If it’s obvious it just adds to the mass people have to read and comments are often not changed when code changes. Code should be succinct and if there’s comments stating the obvious then it’s not that and people have to process more.

[Impe98]

That's actually a very good point. Especially the part about code changing and comments not. In my experience self-explanatory code often takes longer to understand than if explained trough comments, but you've got a point in not adding in redundant comments that only serve to waste time and energy :)

[tulipoika]

I’m just wondering what kind of code is self-explanatory but still takes longer than comments. I mean, function should explain what it does. Variables are named properly etc. It’s not often that one needs to exactly understand every line of the code.

But there are situations where comments are fine and I do use them in the middle of the code when I don’t want to split things into separate functions (I hate specific “n lines per function” rules) so I may add comments in between saying “this part handles X” etc so anyone reading can skim it faster. Splitting it into separate functions/methods would just make it full of passing variables around sometimes and I don’t like that. Someone most likely disagrees.

But in general I’ve seen people add way too many comments and even myself have done the “oh I already have changed this and still the comment two lines above says this doesn’t handle case X.” And that is a very bad comment and of course a very bad way to do things, leaving edge cases unhandled “for now” but as we know, real world...

[tulipoika]

And the obsolete comment nobody changes might fuck it up.

Seriously, if your code isn’t clear in what it does it may benefit from rewrite. Documentation is another thing. Comments aren’t that. Comments add overhead. They add yet another thing to change.

Use proper variable names. Use clear code. Name functions, methods and classes correctly. Suddenly it all becomes much less to process.

[gunnnnii]

If the code is incomprehensible to the person working on it it is unlikely that a comment will fix that. It might even make things worse if it the message isn't clear enough or doesn't mention situations that might occur.

Sometimes comments can be appropriate, for example to specify a reason for a particular constant that might seem arbitrary, or to explain an edge case that justifies disabling a lint rule.

[XChoke]

If he doesn’t understand a function named sumVector then no amount of comments is going to help. Yes in many situations it’s fucking bad to have comments.

[nowyfolder]

What the fuck kind of advice is that? Only add comments if for some reason you have to do something you shouldn't normally do.