We’re rewarding the question askers & reputations are being recalculated! Read more.
31

So the idea behind this is mainly educational... Cool. Always good to try and understand how things work under the covers. ...but I might even consider using it in reality if turns out to be good. Please rethink this. Smart pointer implementations are incredibly difficult to get right. Scott Myers, of Effective C++ fame, famously tried to implement a ...


23

Readability Please put a space after the include and before the < #include<iostream> #include<string> #include<memory> Don't do this You can read any other C++ revue on this site. using namespace std; See: Why is “using namespace std” considered bad practice? Don't add useless comments // Template Node template <typename T> ...


23

I'll hit the red flags first, and then review the details. template<typename T> const shr<T> make_shr(T * obj) { return shr<T>(obj); } "Returning by const value" is a red flag. It doesn't do anything except occasionally disable move ...


21

1. Bugs If the open succeeds but the lseek fails (as it might if the file is a named pipe), the function omits to close the file descriptor, thus leaking it. If read reaches the end of the file (as it might if the file was truncated after the lseek), then it returns 0. This will cause the function to go into an infinite loop. There's no check on the result ...


18

There are a number of things I would change here, so I'm going to go through the process incrementally, applying changes to the whole file in passes rather than in chunks. I think this should make it more clear why the changes are implemented. C99 Note You should have access to, at the very least, a C99 compiler. If that's not true, you can ignore this ...


18

If you had tagged this code as C, it would have been acceptable. Since you tagged it as C++, it's horrible. Instead of writing your own swap function, there's already std::swap in <algorithm>. Instead of writing bubble sort yourself, just use std::sort, also from <algorithm>. Instead of using arrays and resizing them yourself, just use std::...


17

is this a graph? Yes Am I missing anything? In terms of a graph: No. You have chosen to make your edges uni-directional (thus two edges are required to mark routes between cities). Not an issue in itself but you could have helper functions that create two edges automatically. In terms of good coding: Yes You have completely missed out encapsulation. ...


17

You don't need an include guard in Main.cpp #ifndef MAIN_CPP #define MAIN_CPP Include guards are meant to avoid a header file from being included more than once by the compiler/preprocessor (#include works just like text copy-pasting, so yes, the preprocessor is that dumb). For a struct that has all of its members publicly accessible, having get* methods ...


16

You don't need pointers to character pointers at all: int str_cmp(const char* s1, const char* s2) { while (*s1 != '\0' && *s1 == *s2) { ++s1; ++s2; } if (*s1 == *s2) { return 0; } return *s1 < *s2 ? -1 : 1; } Also, there is a bug in your second implementation: it returns 1 on compareStrings("...


15

Please fix your indentation all over the header. It should be consistent with everything else. You don't need the return 0 at the end of main(). Reaching this point implies successful termination, and the compiler will just insert it for you. In common stack implementations, pop() is void (merely pops off the top element). This is useful as it'll be much ...


15

I have a few questions about your design. Classes What's the purpose of using classes if all of your data and methods are public? Why bother having getters and setters like getMlvl(), getClvl(), getDlvl()? Personally, I'd make all the instance variables private and leave the accessors because if you later expand this it will be difficult to track who's ...


14

First of all, I will start with one of the most common remarks: please, do not use using namespace std;. It is especially bad if you write it in a header file since it leads to namespace pollution and name clashes. Instead of a method named traverse, it would be better to overload operator<< to print your list. Here is how you could adapt your ...


14

Const-correctness Since you tagged this question as c++, you must take care of const-correctness: cr54045.cpp:36:18: warning: conversion from string literal to 'char *' is deprecated [-Wdeprecated-writable-strings] res = strstrImp("helloworld","low"); ^ cr54045.cpp:36:31: warning: conversion from string literal to 'char *' is deprecated [-...


12

This is not unique_ptr, it is auto_ptr, which has problems, mainly that copying a value modifies it, which is counter intuitive. Assuming you really do not have move semantics and cannot do better (you should, C++11 is old now), there are still some problems: There is no way to release a pointer. If I am not happy with your SmartPointer I want to do ...


12

I like this. #ifndef LINKEDLIST_H #define LINKEDLIST_H But it is not very unique. If used in a big project this may clash with other guards. I usually include the name of the namespace in the guard. Alternatively you can generate a GUID. Don't do this. using namespace std; More detail can be found here Why is “using namespace std” considered bad ...


12

Input sanitization Your functions take a std::string as parameter, as such, they should be "well behaved" for any possible std::string passed to them. Note that well behaved does not mean that it has to "work", just that it should not break anything. Specifically, what would happen if I passed the string "Hello" to that function? 'H' - 'a' is -25, ruh roh! ...


12

Naming It's unconventional to name a type with all-uppercase - we normally reserve those names for preprocessor macros, to warn readers that they need treating with care. Avoid such names for ordinary identifiers. Avoid using identifiers that begin with an underscore - in many situations, those names are reserved for use by the implementation, which could ...


11

Lets start here: static void release(T *p) { if (p) { // No need to check for NULL // delete has no action when applied to a NULL pointer delete p; p = 0; // This is very dangerous. // It has no actual affect (as it is local) // but provides an illusionary sense of security. } } ...


11

Implementing your own smart pointer is very hard please don't try. If it is just to try and learn then fine you may learn something but after practicing go back to one that has been tested and is know to work. 20 seconds into looking Bug 1: int main() { auto_<int> x; auto_<int> y(x); auto_<int> z(x); } Problem caused ...


11

Programming / detail review Fan(Fan *peer__ = NULL); (and some more occurrences) Identifiers that contain a double underscore are reserved for the compiler/libary ("C++ implementation"). The NULL macro can be thought of as deprecated. Use the nullptr keyword instead. It is more typesafe. static Fan * create(Fan *peer = NULL); Since those pointers ...


11

I suggest you rename link to next which is a more conventional name for the next linked list node. struct node { int data; struct node* link; }; Currently, you have to write the type of the node as struct node. However, if you do: typedef struct node { int data; struct node* link; } node; You can allocate a new node simply by writing node ...


11

Let's have a look at some examples where it fails. Rule of Three You have not correctly over written the assignment operator. ptr::unq<int> x(new int(5)); ptr::unq<int> y; y = x; // This is broken. You should look up rule of three. The above code compiles. BUT is broken. This pointer will get deleted twice. In debug mode on my compiler ...


11

Looking at the performance, the two versions should perform just about identically. The second version has one less call/return, which can save a couple of CPU cycles, but if you have it multiple places in your code the additional code bytes and cache misses can overshadow that. Either way you probably won't notice a difference. Looking at readability and ...


10

It looks idiomatic and bug-free. s++; Perhaps ++s as a habit unless postfix s++ is required (as it is during the copy). while((*s++ = *t++)) Could have just a single pair of parentheses. void custom_strcat(char *s, char *t) The signature of strcat is usually char* strcat(char* destination, const char* source). The const means that the source buffer is ...


10

I see no point in avoiding the use of Standard Library functions. Learning to use them is part of becoming a proficient C programmer. Hence using strlen is good and reinventing strcmp or memcmp, as you have done, is bad. Making the return value unsigned is just extra typing/noise and achieves nothing. Here is a simplified version: int strend1(const char ...


10

That is a lot of code for the job. It can be done significantly more easily in two areas: use an array instead of a queue use the POSIX function, getline, which will handle lines of any size. If you don't want to use getline (which admittedly might not have existed when K&R wrote their book), concentrate on writing its equivalent. Here is a version ...


10

My question: Is borrowed_ptr silly? Is it a faux pas? Does it feel like a code smell? It seems a bit overengineered to me (YMMV). Do you feel I should rather do 1., 2. or something else altogether? Something else. Consider this alternative: class Master { private: std::vector<std::unique_ptr<Child>> children; ... public: // an ...


10

Given that the code is really simple, I see mostly coding style issues with it. Instead of this: char *names[] = { "John", "Mona", "Lisa", "Frank" }; I would prefer either of these writing styles: char *names[] = { "John", "Mona", "Lisa", "Frank" }; // or char *names[] = { "John", "Mona", "Lisa", "Frank" }; The pNames variable ...


10

Overall Ohhhhh. Is it well-formed? It compiles, so yes. i.e. does it follow common C++ standard and patterns (for example, should private members be declared before public ones? Personally I think so. * Private Variables * Public * Constuctor / Destructor * Copy Semantics * Move Semantics * Swap * Other Public Interface * ...


10

Better names The variable names are too short to be meaningful. It's common to use i as an index in for-loops, but r doesn't really tell us its purpose. Names like number_of_strings are self-descriptive, so we should use those. Unused variables We never use c in find_first_recurring_letter, therefore we should remove the parameter altogether. Strictly ...


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