\$\begingroup\$What's "do something"? An unfinished code or some removed stuff for the purpose of this example? Code Review is about improving existing, working code. Here, it already are many improvement possible on your code, but try to give a full working code example. If moderators think you're not off-topic, i'll be happy to help you improving your code and logic, but i'm afraid you are.\$\endgroup\$
– CalakNov 8 '18 at 8:20

\$\begingroup\$@Calak I don't know C++ so it'd be a great help if you could point to the code that is missing or not implemented.\$\endgroup\$
– PeilonrayzNov 8 '18 at 9:21

\$\begingroup\$You need to provide a minimal working example. I couldn't compile your code. It seems that g_list is some global variable that isn't mentioned in the code bits you've presented so far.\$\endgroup\$
– papagagaNov 8 '18 at 9:27

7

\$\begingroup\$@papagag, a minimal working example is a Stack Overflow thing - what we need on CR is real code, preferably complete (but certainly with sufficient context that we know the types of all variables, and the intended purpose).\$\endgroup\$
– Toby SpeightNov 8 '18 at 10:40

2 Answers
2

There are a number of things that could be improved in this code. I hope you find these suggestions helpful.

Don't abuse using namespace std

Putting using namespace std at the top of every program is a bad habit that you'd do well to avoid.

Eliminate global variables where practical

Having routines dependent on global variables makes it that much more difficult to understand the logic and introduces many opportunities for error. Eliminating global variables where practical is always a good idea, so I would strongly suggest passing g_list as a parameter to functions that need it rather than having it as a global variable.

Always return an appropriate value

Your print_list() and update_list_v2() routines have control paths that cause them to end without returning any int value. This is an error and should be fixed.

Don't write this-> in member function

The use of this-> in member functions just clutters the code and adds nothing of value. Instead, simply omit it for clarity. Thus this:

However, I wouldn't actually write it that way for several reasons which are enumerated in the next few points.

Use const where practical

A number of places in the code should have the const keyword added. For example instead of this:

void update_bmi(element &a) {

I would write it this way:

void update_bmi(const element &a) {

Pass by const reference where practical

The arguments to is_lower_id and is_same are declared as element but that causes the entire object to be duplicated. Better would be to make it const element & because it is not modified and it doesn't need to be duplicated.

Prefer stream I/O to printf

The printf function was a capable workhorse for many years, but the C++ iostream library is better in a number of regards. Although it's often more typing for the programmer initially, it's better because it has better type checking, less possibility for runtime overhead, and it fits well with the rest of C++. So I would probably change your element::print to this:

This is called aggregate initialization and makes this much neater and easier to understand and maintain. Note that to do this, I reordered the data members is element so that id, name and bmi are the first three members.

Use standard operators

The code currently contains this function:

static bool is_lower_id(element a, element b) {
return a.id < b.id;
}

I would instead use the standard operator for this:

bool operator<(const element &b) const {
return id < b.id;
}

Now instead of writing this:

if (element::is_lower_id(g_list[m], e))

you can write it like this:

if (g_list[m] < e)

Also this:

sort(v.begin(), v.end(), element::is_lower_id);

becomes this:

std::sort(v.begin(), v.end());

because std::sort uses operator< by default. Similarly, to replace is_same we can use this:

Eliminate processing variables from structures

The remove flag in element is just a by-product of the processing and isn't really a part of the node itself. I'd recommend removing it and using better algorithms instead, as shown later in this answer.

Keep private things private

It's usually good to keep data members private if they are expected to have an invariant, that is, a condition that must always be true. In the case of this program, the max_bmi is expected to be set properly. For that reason, I'd make all of the data members private and provide access mechanisms as needed.

Simplify your algorithm

The way the rules are constructed, the result after update_list_v2() is that the resulting g_list will have exactly the elements that v had, with any updates to max_bmi applied. Further, we can reduce the complexity of the algorithm by noting that if the two input lists are sorted, we can make a single pass through both rather than searching exhaustively for every element.

Results

Here's a version of the code that applies all of these ideas. Note that because the class is really a class now with private data members, we need to use initializer_lists to provide the initialization that had been done with aggregate initialization.

Don't use using namespace std;

It's bad practice that can cause a lot of problems that are easy to avoid, most people are used to the prefix, and three letters aren't that hard to type.

Don't write C code in C++

They are different languages with different standard libraries, different conventions and standard practices.

The C++ version of <stdio.h> is <cstdio>. Always prefer the C++ headers of the C library when writing in C++. They always begin with c and won't end in .h. To that end the <iostream> is preferred for printing in C++.

In C++ it is preferred to declare the type specifier with the type not the variable.

vector<element> &v

would be declared

vector<element>& v

You should also eschew C style arrays and char* arrays. Prefer std::array for arrays you know the size of at compile time and std::vector for arrays you don't know the size of or that need to change size. (std::array is not dynamic and can't be resized)

You can replace the char* array with an std::array<char> or std::array<std::string>

And don't declare multiple variables on a single line. it is error prone and difficult to read. You did keep most variables on separate lines but you did declare:

vector<element> a, b;

Don't use Global variables

They are dangerous and can lead to hard to find bugs. Can they have a usefulness? Sure. I use them to name Magic Numbers and there are probably other uses but in this case almost definitely not. You could absolutely pass your vector by reference.

Don't return 0 from main()

The compiler will generate it for you and the implication is that you might return anything other than 0.

Use proper return types

Don't return int from functions that don't have a return value. They should be marked void. Your compiler also should have warned you about this.

So turn up your compiler warnings. this will help you tremendously. W4 on VS or Wall on gcc/Clang. Werror is also useful and highly recommended.

Use proper encapsulation

Your class member variables are exposed. This is almost as error prone as global variables and defeats the entire concept of encapsulation. This is however sometimes done with POD types. which I think is what your element essentially is. Switch to a struct (which is the publicly accessable user-defined type) and pull out the functions and you'll have:

Then prefer member initialization where possible. You have for loop dedicated to initializing every elements remove to true. That can be done with brace initialization. bool remove{ true }

You use a Yoda conditional in your is_same() function. These are considered outdated and difficult to read. Also any decent linter should catch the typo that they are used to prevent. You also unnecessarily use the std::stringcompare method. You don't need the conversion to int only to compare the int to 0 and convert to Boolean. Two strings can be compared with ==.

You should also pass by const reference in those functions to avoid unnecessary copying.

By removing your printf call and using std::cout those functions now look like this:

Now all the traditional comparators will work with your user defined type.

In print_list why do you do this?:

printf("%s:%d\n", __func__, __LINE__);

I'm not certain what purpose this serves. After all most compilers and IDE's will give you the line of a function in the case of an error or exception. But by all means put it back in if you need it. This is also a great opportunity to use a ranged-for loop. They're less error-prone and neater and easier to read.

Now for your update_list function. If I understand correctly you have two vectors. If an element of vector two doesn't exist in vector one then it is added. In your code you remove values from the second vector if adding it to the first leaving the second vector populated with the duplicates. However you seem only really interested in the (no-longer)global vector. If you don't need the vectors of duplicates then you can greatly simplify your code.

Switch to an std::map and set the keys to the element.id. This will keep your collection sorted and make merging non-duplicates fantastically easy. (this assumes elements that share an id are implicitly duplicate. Before you insert the new elements into the map test to see if they exist in the old one and update_max_bmi accordingly.