HiveBrain v1.2.0
Get Started
← Back to all entries
patterncppMinor

Mutable String Class

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
stringmutableclass

Problem

Here is a String class that I've made for fun in my spare time, for myself.
I have a few concerns about it, and am considering going immutable or a mix of the two where the string would be immutable when initialized from a constant, and then mutable when changed.

Also I am considering how to implement Unicode. I could prefer not to have a default format such as always translating to UTF32 internally, but rather to be flexible and to have each instance keep its source format unless told otherwise. But then I would need a Char class to abstract the difference between formats, and I find it very fun to keep things optimized for performance as well.

  • How would you go about this?



  • Is there something very wrong in my class?



Please be criticize my code so that I can improve on it.

Header

```
/*
String defaults to String::Empty
*/

#pragma once
#include "../Types.hpp"
#include "../DataStruct/Vector.hpp"

namespace Core
{
class String
{
static UInt NewLineLength;

public:
static CStr const Empty;

private:
typedef DataStruct::Vector Vector;
Vector _vctr;

public:
/ StrPtrVec internal class /
class StrPtrVec : public DataStruct::Vector
{
public:
StrPtrVec();
StrPtrVec(UInt capacity);
~StrPtrVec();
};

/ public static /
static UInt CStrLength(CStr);
static UInt CStrByteSize(CStr);
static void FormatToBuffer(TChar* buffer, UInt buffer_size, CStr format, ...);
static String FormatToString(CStr format, ...);
static Int Compare(CStr source, CStr target);
static Bool StartsWith(CStr text, UInt textLength, CStr value);
static Bool StartsWith(CStr text, CStr value);
static Bool EndsWith(CStr text, UInt textLength, CStr value);
static Bool EndsWith(CStr text, CStr value);
static UInt IndexOf(CStr text, UInt textLength, CStr value, UInt val

Solution

A few points open to improvement:

-
In your implementation file, you have two functions, _ReplaceRightToLeft() and _ReplaceLeftToRight() that are using a technically ilegal naming. Names starting with an underscore, followed by an uppercase letter are reserved by the C++ standard.

-
You are dealing with raw pointers here and there (Split() raises a few red flags), which is not ideal, since no information about ownership is implied. Ideally, you should give preference to smart pointers.

-
Personally, I think you are abusing a bit with the typedefs for every native type. This is the convention in a lot of Windows code, but is it really necessary in your case to typedef VoidPtr, Bool, Int? I sort of understand the reason, keeping an uniform naming for everything, but as time goes by, you are likely to get tired or hitting SHIFT every time you want to declare a simple int. So my suggestion is for you to rethink that and be less pedantic with the type naming, in exchange for more straightforwardness when writing code.

-
Another piece of personal advice: YMMV, but I have recently decided to stop indenting namespaces, after many years of doing so. I have come to the conclusion that this adds little to nothing to the readability of the code, at the expense of a default level of indentation everywhere. This is another case to weight pedantry vs practicality.

-
What's a CStr? Took me a while to realize that it must be char . This typedef is only obfuscating the code. Even though more verbose, const char (or const TChar * if you prefer), would make things a lot clearer.

-
Instead of doing something awkward like this:

_vctr.operator=((Vector&&)text);


Use std::move() instead.

-
Minor aspect: your naming convention for function parameters is apparently camelCase, though you have one or two instances of snake_case. I.e.: In FormatToBuffer() and DrivePointer(), that I've been able to notice.

Code Snippets

_vctr.operator=((Vector&&)text);

Context

StackExchange Code Review Q#74560, answer score: 7

Revisions (0)

No revisions yet.