Hot answers tagged

29

Issues Your vector assumes that the type being stored has a default constructor. arr = new element[size]; Also this is really inefficient for expensive to create classes where you don't use all the size members. You want to design your class so that the objects in the vector are only constructed when the element is first added. Bug You don't implement ...


22

Design The main thing about a vector is not constructing its members until they are put into the container. The vector you have constructs all the members immediately which can be very expensive if your type T has an expensive constructor (or you never use any of the members). T* storage = nullptr; As a result you don't want to use T* as your storage type ...


20

Memory management new T[m_capacity] default-constructs m_capacity objects of type T in there. This might hurt performance a lot if Ts default constructor is not trivial, and even then it still isn't necessary. Even worse, in pushBack a new T object gets copy constructed in the place of the already existing one (if m_size < m_capacity), without properly ...


20

Header files It's strange that this code uses the C header <string.h> but the C++ versions of <cmath>, <ctime> and <cstdlib>. I recommend sticking to the C++ headers except on the rare occasions that you need to compile the same code with a C compiler. In this case, I don't see anything using <cstring>, so we can probably ...


19

Your code is C++03 like (ie there are no move constructors or move assignment operators). You should definitely think about updating your class to be move aware. The first thing about the interface is that the assignment operator is not close to the constructors. Since these are highly linked I like to place the assignment operators very close to the ...


17

I haven't attempted to measure the timing of this, in part because I don't know how you're planning to use it, but here are some ideas for improving your code. Consider returning something useful from functions Your Split function doesn't return anything, which isn't an error, but it's an odd design. I'd expect it to be declared like this instead: std::...


15

Change your column loop to use a reference. for (const auto &c : column_vec) Without the reference, a copy will be made of each vector. This will involve a memory allocation. Using the reference you avoid all that, which should save a good deal of time since each c will be a single element vector. auto r can stay since r will be a double. Combine ...


14

I understand that some of this class is predicated by your professor; However I will review the code as a whole and you can then choose what can be changed and what cannot be changed. Interface/High-level comments Use of leading underscore _ has severe limitations. The C and C++ standards reserve the use of certain patterns of leading underscores for ...


14

First of all, lets answer the questions you have: Yes, no, maybe. Some people do it one way, some the other. Personally, I prefer splitting declarations and definitions up so that I have an overview of what my class offers at the top. Some people argue, however, that this method tends to be very verbose (and it can be, especially with multi-layer templates),...


13

The code can be simplified and improved in several ways, and the inner loop can be made tighter. Let me show you how: private static List<List<String>> readTXTFile(String csvFileName) throws IOException { String line = null; BufferedReader stream = null; List<List<String>> csvData = new ArrayList<List<String>>...


13

There is a rather large amount of code here and a lot of macro trickery, so I shall refrain from attempting to comment on it all. There are, however, a few things that immediately jumped out at me. Don't use new as your allocating macro. That's extremely confusing, and it's going to make compiling it as C++ rather difficult. (Not that having C be ...


13

Separation of Concerns The function should have only one concern--finding the indices. Printing out results is a separate concern that shouldn't be combined with finding the indices. Combining the two makes it difficult or impossible to use in a wide variety of situations. Vacuous Declaration Having the declaration followed immediately by the definition ...


12

A few things you can improve upon: Use more of <algorithm> and <memory> Raw loops are verbose and unreadable. Use algorithms whenever you can -I've just noticed that you did it in some cases, but left the raw loops in others (cf reserve). You also seem not to know std::uninitialized_fill, which does exactly what you do manually in your third ...


12

Use delete[] You are allocating an array of objects with new[], but attempting to delete them with delete, which results in undefined behavior. You should delete the array with delete[] to ensure that your code functions properly. Do this in both Vector<T>::~Vector() and Vector<T>::insert(). Implement your Iterators as pointers under-the-hood ...


12

The range-based for loop This works via the begin() and end() functions. So for any class X if there are stand alone functions begin(X&) and end(X&) the range based for will work. Normally this will default to the std::begin() and std::end() but if you have written explicit versions of these functions in the same namespace as X these will be used. ...


12

Is the vector-of-vectors representation imposed on you, or can you change it? It's going to be more efficient to allocate a single vector, and use simple arithmetic to convert row/column representation into a linear index. It's then possible to provide a transpose view without needing to move all the elements, which may be useful in some circumstances (in ...


12

You tagged c++11, but your code is not at all C++11. You have no move constructor and no brace-initializers. You should implement them. Here's a sample implementation using the copy-and-swap idiom. friend void swap(Vector& a, Vector& b) { using std::swap; swap(a.capacity, b.capacity); swap(a.sizet, b.sizet); swap(a.arr, b.arr); } ...


12

It's great that you provide a test program. Although it doesn't yet test very much, running it under Valgrind uncovers a few wild accesses: ==17803== Invalid write of size 4 ==17803== at 0x109773: Vector::resize(int) (221707.cpp:68) ==17803== by 0x10938A: main (221707.cpp:103) ==17803== Address 0x4d74c94 is 0 bytes after a block of size 20 alloc'd ==...


12

std::vector<std::string> ToProperCase(std::vector<std::string>& array) { // ... return array; } There are many styles when it comes to capitalization. ToProperCase should have a better name to indicate what the function does, which is to capitalize the first letter of each word. array is a non-local reference being returned by value....


11

STOP USING __ its reserved for the implementation. Basically stop using underscore until you know the rules. This code is basically all broken because of this. You code technically is all undefined. We told you this before. STOP IT. BUG: vector(const _Myt &_Rhs) : __size(_Rhs.__size), __capacity(_Rhs.__size + 20), __data(_Alloc().allocate(_Rhs....


11

Code: I realise you're not implementing the complete functionality, but you probably want add and use at least the value_type, size_type, reference and const_reference typedefs. operator== and operator!= are simple to implement and quite useful. Use std::size_t, not size_t (the latter is the C version). Prefer named constants to magic numbers (e.g. declare ...


11

Your two constructors don't store a value into m_capacity, so if the initial capacity requested (passed in as a parameter) is larger than the default capacity you'll have things in an inconsistent state and likely run into problems later. Is there a reason you're not using std::make_unique<int[]>, instead of allocating memory with new and constructing ...


10

For a prefix search, you can use std::mismatch() in <algorithm>. bool isPrefix(const std::vector<int>& v1, const std::vector<int>& v2) { if (v1.size() > v2.size()) { return v2.end() == std::mismatch(v2.begin(), v2.end(), v1.begin()).first; } return v1.end() == std::mismatch(v1.begin(), v1.end(), v2.begin())....


10

Using log to calculate capacity is very questionable. It forces a client to link the executable with -lm. I would not bother with Log at all: just double the capacity as needed. A repeated code in the constructors should be factored out. It is OK (and in fact preferable) to use std::copy() instead of loops. pop_back() must destroy a buffer[size - 1] object. ...


10

Size Requirements You talk about wanting to "use less memory" than std::vector. But std::vector doesn't use that much memory. It's three pointers. I guess if you set your MaxSizeType to int instead of size_t, you can have a vector of 16 bytes instead of 24, but I would really have to see what you're doing that that difference should matter. Performance ...


10

Coal_ already made some good points. Let's just elaborate on the one about private. A class makes everything in it private by default that is why you don't need it if you order your interface this way. However it's recommended to order your interface from public to private so people who read it can see which methods you expose without having to read through ...


10

First, show your teacher the C++ Core Guidelines and various videos from conferences saying that you should not use naked new or delete anymore. std::vector<Train * > v = {&i, &r}; You are storing pointers to local (stack based) objects, which is correct in this example because the vector is in that same scope. But usually you will never do ...


10

Use the well-known names for methods c_begin and c_end are so close to the standard-container cbegin and cend that users will forever curse you. Likewise for pushBack and popBack (instead of the usual push_back and pop_back). Use a consistent naming scheme within the class Members seem to have a mix of naming schemes: buff, m_size, Log. Pick one and ...


10

There is a bug in pushBack, you need to take into account the case where you call it on an existing member of the array. For example in the code Vector<int> v(1,0); v.pushBack(v[0]); pushBack will first resize the array, invalidating the reference to v[0]. It will then attempt to insert that invalid reference in the new array.


9

Allowing a Shape to be assigned to a circle does not sound like it is very safe. I suppose there are situations where it can be used but I would advice against it in the general case Also I would use Curiously Recurring Template Pattern to get around having to write the assignment operator-er in every derived class. class Shape { // STUFF }; template<...


Only top voted, non community-wiki answers of a minimum length are eligible