Since I've been working with C strings these days, I decided to make a string class from the ground-up just to see how it would go. I wrote all labels in Spanish (my mother language) just for the sake of it. If you have problems understanding it, I'll simply translate them into English

(DISCLOSURE: I'm not thinking about actually using this in production code. I was just trying something new. I think the native std::string class is much better for these things. But if you can tell me how this would perform in production code, it would be of great help!).

My main goal was to made a class that could be edited and used with legacy code that needs a const char* instead of std::string.

What do you think about it? I have some doubts about my implementation: for instance, I don't know if returning the buffer as const char* is safe, or if using static_cast is necessary.

Here is my header file (Cadena.h):

#pragma once
#include <iostream>

namespace David 

 class Cadena 
 private:
 char* bufer;
 size_t tamanio;

 size_t obtenerLongitud(const char* cadena) 
 size_t longitud = 0;
 for (longitud = 0; cadena[longitud] != ''; ++longitud);
 ++longitud;
 return longitud;
 

 void borrarBufer() 
 delete bufer;
 bufer = nullptr;
 

 void ajustar(size_t longitud) 
 char* cadenaTemporal = new char[longitud + 1];
 if (bufer != nullptr) 
 for (size_t i = 0; i < tamanio; ++i) 
 cadenaTemporal[i] = bufer[i];
 
 
 borrarBufer();
 bufer = cadenaTemporal;
 cadenaTemporal = nullptr;
 tamanio = longitud + 1;
 

 public:
 Cadena(const char* cadena) 
 tamanio = obtenerLongitud(cadena);
 bufer = new char[tamanio];
 for (size_t longitud = 0; cadena[longitud] != ''; ++longitud) 
 bufer[longitud] = cadena[longitud];
 
 bufer[tamanio - 1] = '';
 

 Cadena() 
 tamanio = 0;
 bufer = nullptr;
 

 ~Cadena() 
 borrarBufer();
 

 const char* formaConstante() const 
 return static_cast<const char*>(bufer);
 

 const size_t len() const 
 return tamanio - 1;
 

 char& operator (size_t indice) 
 if (indice < tamanio)
 return bufer[indice];
 return bufer[tamanio - 1];
 

 void operator = (const char* cadena) 
 borrarBufer();
 tamanio = obtenerLongitud(cadena);
 bufer = new char[tamanio];
 for (size_t longitud = 0; cadena[longitud] != ''; ++longitud) 
 bufer[longitud] = cadena[longitud];
 
 bufer[tamanio - 1] = '';
 

 friend std::ostream& operator << (std::ostream& salida, const Cadena& cadena) 
 for (size_t longitud = 0; cadena.formaConstante()[longitud] != ''; ++longitud) 
 salida << cadena.formaConstante()[longitud];
 
 return salida;
 

 friend void operator >> (std::istream& entrada, Cadena& cadena) 
 if(cadena.bufer != nullptr) cadena.borrarBufer();
 char letra = ' ';
 size_t longitud = 0;
 while (letra != 'n') 
 entrada >> std::noskipws >> letra;
 cadena.ajustar(longitud);
 cadena.bufer[longitud] = letra;
 ++longitud;
 
 cadena.bufer[longitud - 1] = '';
 
 ;

Possible undefined behaviour

There are several places that will lead to undefined behaviour.

In ajustar it can be possible that longitud < taminio. We will then happily copy into cadenaTemporal[i] where longitud < i < taminio. It's easy to create an example for this:

David::Cadena example("oh oh");
std::cin >> example;

So instead of taminio use both longitud as well as taminio as upper limit.

In operator (size_t indice), you return bufer[-1] on example[0] if the string hasn't been initialized. This should either get documented or checked.

Consistency

borrarBufer() should also set tamino to zero. That seems to be your "empty string" scenario.

Don't include the in len()

If the string is empty, len() should return 0, not -1. strlen() or std::string::size() don't return the either.

 const size_t len() const 
 return tamanio;
 

Use common interface

operator= should return a Cadena&, not void. Similar, operator>> should return istream&, not void.

Follow the rule of five

You provide a custom constructor. Therefore, you should also provide

• a custom copy constructor,


• a custom move constructor,


• a custom copy assignment,


• a custom move assignment and


• a custom destructor.

You don't provide any of them, but you should. For example, I cannot copy a Cadena at the moment. I have to write

Cadena my_copy(other.formaConstante())

Use copy and swap idiom for operator=

As soon as you have a copy constructor, you can write operator= very easy:

 void swap(Cadena & other) 
 std::swap(tamino, other.tamino);
 std::swap(bufer, other.bufer);
 

 Cadena& operator=(const char* cadena) 
 Cadena temporary(cadena);
 swap(cadena);
 return *this;
 

Provide push(char) to add single characters

This will make operator>> a lot simpler.

Use a capacity

Your operator>> is $mathcal O(n^2)$ since you have to realloc for every single char. Instead, allocate more memory than you actually need. Or use std::vector for your internal memory instead, it will take care of that automatically.

1. Not using the standard naming conventions is worse in C++ than in a language without so powerful templates or other liberally-used reflection facilities. It in essence bars your class from interoperation with the standard-library.




2. Making the 0-terminator part of the string is an interesting choice. As in the curse "may you live in interesting times".




3. Provide the standard iterator-interface. Doing so enables use of many standard-algorithm and the for-range-loop.




4. Don't cast if you don't have to. Casting is a way too bypass the compiler enforcing type-safety, and is thus dangerous.
   
   In your case of converting from char* to const char*, an implicit conversion is enough.




5. While it's a valid decision to return the terminator if the index is out of range, that might be a bit costly for marginal utility.




6. Provide a const-interface besides a non-const one. Otherwise, being const-correct will be quite hard on your consumers.




7. Streaming each and every single character in the string by itself is contra-indicated, as it means all the setup and error-checking must be done for every one by itself, instead for all of them together. Also, other output may be interleaved.




8. You didn't provide copy-/move ctor/assignment, though you provide a dtor to free the contained resource. Mind the rule of 5/3/0.




9. While you can decide not to provide a capacity, consider at least using one in op>> to avoid quadratic behavior.

Code Review

Pragmas

Not all compilers support this pragma.

#pragma once

Size Vs Capacity

Having only a size dooms you to re-allocating a new buffer every-time text is added to the string. It can be more efficient to allocate a buffer slightly larger than you need (capacity) and only reallocating when size (tamanio) exceeds capacity.

 char* bufer;
 size_t tamanio;

Size includes the

Sure.

 size_t obtenerLongitud(const char* cadena) 
 size_t longitud = 0;
 for (longitud = 0; cadena[longitud] != ''; ++longitud);
 ++longitud;
 return longitud;
 

But you need to be consistent about making sure that buffer[tamanio] == '' in the rest of the code.

Adjusting the size?

You don't guarantee:

buffer[tamanio] == ''

This new

 char* cadenaTemporal = new char[longitud + 1];

Does not null the space before returning it. The content is quite random.

This is completely a waste of time:

 cadenaTemporal = nullptr;

Constructor

You don't obey the rule of three.

Bsically if you define one of Destructor/Copy Constructor/Copy Assignment you should probably define all three. In your case you don't define Copy Constructor. As a result you will get a double delete if you every copy your string.

 Cadena tmp1("Test");
 Cadena tmp2(tmp1); // Compiler generated copy constructor
 // Does a simple shallow copy. So both
 // objects point at the same buffer.

// Destructor of both objects called here.
// The destructor of `tmp2` first which deletes the buffer,
// then the destructor of `tmp1` which destroys the same buffer.

Operator Vs at()

In C++ we have a guiding principle. You should not have to pay for something you do not use:

 char& operator (size_t indice) 
 if (indice < tamanio)
 return bufer[indice];
 return bufer[tamanio - 1];
 

In most cases the indice has already been checked and guranteed to be inside the range [0, tamanio]. Look at the standard use case.

 Cadena tmp1("Test");
 for(int loop = 0; loop < tmp1.len(); ++loop) 
 std::cout << tmp1[loop];
 

We have already guaranteed that the index is in the correct range. So every call to the operator has a check that is not needed. This is why the standard std::string has two methods accessing the buffer. operator which is unchecked and at() which checks the index is in range.

It is questionable if it is useful to return something valid when the index is out of range. Why not throw an exception so the developer knows that something went wrong.

Copy Assignment

Sure it works (and is perfectly valid). But normally we return a reference to the object. So that it can act like a standard type and be used in a chained expression.

You should also look up the copy and swap idiom.

Move Semantics

Since C++11 (seven years ago now) the language has had move semantics. Your class should also support this. It allows for an efficient move of the object. it requires you to define two operators.

 Cadena(Cadena&& m) noexcept; // Move constructor
 Cadena& operator=(Cadena&&) noexcept; // Move assignement

Input operator

Sure this works.

I would note that it has different semantics to the std::string which imply reads one word. Also remember my note about capacity and size above. This function is very inefficient as it reallocates the buffer after reading every character.

 friend void operator >> (std::istream& entrada, Cadena& cadena) 
 if(cadena.bufer != nullptr) cadena.borrarBufer();
 char letra = ' ';
 size_t longitud = 0;
 while (letra != 'n') 
 entrada >> std::noskipws >> letra;
 cadena.ajustar(longitud);
 cadena.bufer[longitud] = letra;
 ++longitud;
 
 cadena.bufer[longitud - 1] = '';