Program to reverse a string using std::string

Question

Background: I'm trying to learn C++ and I'm doing that by writing some programs by myself. I'm decent with C, but I'm having trouble translating to OOP paradigm. I'm reading from learncpp.com and a book called Thinking in C++.

I'm noticing that reading doesn't really help much as much as writing programs and running into issues and solving them.

Here is a program I've written to reverse a string in C++ using std::string (SO tells me this should be used as opposed to C-style strings/char arrays):

example_one.h

#ifndef example_one_h
#define example_one_h

#include<iostream>
#include<string>
#include<cstdio>

using namespace std;

class stringOps{

    private:
        string arr;
        int length;

    public:
        // Constructor - Destructor
        stringOps(string arr);
        ~stringOps();
        // Member functions

        //1. get length of string
        int getLength();

        //2. print string
        const string& displayString(const stringOps &a);

        //3. Reverse the string and return obj of type stringOps.
        stringOps reverseStr(stringOps &a);

};

#endif

example_one.cc

#include "example_one.h"

/* Constructor and Destructor */
stringOps::stringOps(string a):arr(a), length(a.size()) {
}

stringOps::~stringOps() {
}

/* Member functions */
// 1.  get Length
int stringOps::getLength()
{
    return arr.size();
}

//2. display string
const string& stringOps::displayString(const stringOps &a)
{
    //cout << a.arr << endl;
    return a.arr;
}

//3. reverse array of the object.
stringOps stringOps::reverseStr(stringOps &a)
{
    reverse(a.arr.begin(), a.arr.end());
    return a;
}


/*  -------------------------------- Main  --------------------- */
int main()
{
    stringOps strA("Hello");

    string x = strA.displayString(strA);
    printf("Original string: %s \n", x.c_str());

    int len = strA.getLength();
    printf("String length:%d \n",(len));

    strA.reverseStr(strA);
    string y = strA.displayString(strA);
    printf("Reverse string: %s \n", y.c_str());



    return 0;
}

Questions:

1. What is good about this code (so that I can continue to do this for other programs)?

2. What is bad about this code? (Be brutal in your assessment.) Specifically, I have doubts about the best approach to take while writing functions 2 and 3.

   Function 2:

   const string& stringOps::displayString(const stringOps &a)
   {
       //cout << a.arr << endl;
       return a.arr;
   }
   

   I tried doing this with return type as void and cout within the function, but apparently there's going to be an error for this with iostream. So is returning const string& a good way to do this? I passed by const reference because I do not change the object within the function.

   Function 3:

   stringOps stringOps::reverseStr(stringOps &a)
   {
       reverse(a.arr.begin(), a.arr.end());
       return a;
   }
   

   1. This again takes the argument of type stringOps object by reference and directly manipulates the string. However, if I were to pass this argument as a const, how do I go about this?

   2. Motivation for me to do so is that I want to return a manipulated string that is new, and not manipulate the object that is coming in directly.

   The header would look something like this:

   string stringOps::reverseStr(const stringOps &a)
   

   Do I declare static string inside the function and return it? I tried using a local non-static string (which would be destroyed outside the function, but still for experiment purposes) and I got an error that said:

   read-only variable is not assignable

   I'm assuming this is because it's a const reference.

Answer 1

The functionality is already present in the standard library. Your class has very limited usability and I would prefer simple std::reverse(str.begin(), str.end()). But lets put it aside and deal with the code you've written.

#include <iostream>
#include <cstdio>

Very bad idea to mix these two. I/O may become unordered.

using namespace std;

Why is “using namespace std” in C++ considered bad practice?.

class stringOps

Operations on class's data should be part of it, so naming it stringOps is not good idea. C++ programmers often use operator overloading to expand on some class.

You begin your class definition by private keyword, but everything in class is private by default, whereas in struct it is public. It is the only difference between the two.

    string arr;

What arr means? Nothing comes to mind. You should use something like str. It is good to give your variables good name. It greatly increases readability of the code.

   int length;

Declared, but never used.

    stringOps(string arr);

If you want to make a copy of the string type should be const std::string&. If you want to move from it, the type should be std::string&&.

    ~stringOps();

You declared destructor that does nothing. Compiler generates this by default if you don't provide any destructor. Remove it.

    int getLength();

From return type (int) I may suspect that the code can return negative size. It should be std::size_t, which is meant to be used in context of size. Further reading: When should I use std::size_t?

The functionality is present in std::string. On top of that, you use arr.length() as return value, not length.

    const string& displayString(const stringOps &a);

Why would you return const reference to it? I think users expect a copy of internal string. All operations on copying are noexcept, not including memory allocation, of course.

The next thing is arguments. It would be better to just omit argument, and return internal string.

stringOps reverseStr(stringOps &a);

Again, argument should be omitted. The internal string should be reversed and returned. Also, it would be better to return std::string.

Answer for your first question:

Don't do it in the future. It's not that your code is wrong. The purpose of it is very wrong.

Answer 2

I will try to add some things to already posted answers.

There are two ways about making this OOP.

1. one way is class holding the actual string, responsible for manipulation with it.
2. other way is helper utility class providing only functions, not holding any string at all.

Your code is unfortunately weird mix of both. I will try to show headers for both ways here (just by hand, will very likely not compile without fixing, plus it's missing implementation): (and I'm afraid I'm at the moment tainted by Java too much (coding in Java in other window, so I may mess up some C++ syntax badly, this answer is more about OOP-like ideas, how to design your API and classes).

class SmartString {
private:  // I don't mind explicit private, it's easier to read for me
    std::string str;  // my string, my responsibility
public:
    SmartString(const std::string & initstr) : str(initstr) { }
    const std::string & get() const { return str; }
    const std::string & reverseAndGet();
    int length() const { return str.length(); }
    void display(std::ostream & out) const;
}

Ideas behind this one:

• display can be redirected to any ostream, binding it with std::cout in method body would limit usage of such class. This is important especially with unit testing, where you want to fake and have under control any external systems the tested code does use, so providing the code with external system instance during construction, or per-call helps a lot (when done during construction, it's called "dependency injection").
• whenever string is returned, it's const reference to the string belonging to the class instance, so if outer code want to manipulate it, it must use only method available in the SmartString API, and SmartString instance has full control over inner representation of string (it can use instead of std::string an char [], and the public API would not need to be changed at all, as that part of implementation is completely hidden.
• the API feels weird BTW... the reverseAndGet... I was thinking about making it void reverse(); only, so to get reversed SmartString test("tset"); you would have to do test.reverse(); std::string reversedString = test.get();.

To make it less weird, and more OOP like, it would make more sense to extend std::string, as that's already well known defined API, and just adding one more reverse method to it makes much more sense in OOP world, having then extendedString class which can be used in place of std::string where needed.

Helper utility class (or just a namespace with functions would do probably even better) would tried to provide pure functions, taking everything needed on input, and returning output, not having any instance at all, so they are accessible from anywhere, when you have the input ready.

namespace stringAlgo {
    std::string reverse(const std::string & str);
    //there's little point to do "length", as it's part of original std::string
    //but just as an example
    int length(const std::string & str) { return str.length(); }
    void displayQuoted(std::ostream & out, const std::string & str) {
        out << "„" << str << "“";
    }
}

Ideas behind this one:

• there's no instance anywhere, the functions are ready to be called straight away when you have some std::string to process, and some std::ostream to output to. (in Java world these would be static methods of class/interface, in C++ it's OK to create them as global public functions (but put them into some namespace to avoid naming clashes))
• namespace ... actually stringAlgo is like asking for collision with some generic library, when working on library code, use very specific namespace names, like raajStrLib. When the code will be used only inside the single project (will it, really? What about the code re-use), you can get away with more generic namespace names, but still make sure they are not too generic (in case you will later have to add some 3rd party lib, which may collide), and it's actually easy to understand what is the purpose and to recall the name (now I like stringAlgo even less... :D).

---

Overall in OOP, when designing classes/interfaces, try:

• use problem-domain language and abstraction, so when providing bank account library, don't talk about strings in upper layers of abstractions, talk about account numbers (even when represented by std::string, but wrapped in bank_account_number type).
• try to visualise all the abstractions, and consider their level of abstraction toward problem-domain, decide on reasonable amount of levels, and group the things together. Adjust then everything in particular group to have the same level of abstraction. So you don't start suddenly to work with back_account_number.charAt() in the top most layer, when you have lower layer responsible for single char manipulating related methods.
• usually the main of the problem solution should be able to solve the problem with the top-most abstraction layer completely, without touching the lower levels. The lower levels should be accessible mostly for extending/building new top-layer blocks, not for regular usage.
• try to decide, what/who is responsible for what. An bank_account class can have utility function to decide whether some string forms valid bank account number (or when bank_account_number type/class exists, it belongs there), but will not provide sum of amounts over list of accounts, only provides amounts read from the account, which it is instance of. For sums over list you should have another utility method, or eventually class holding the list itself, and providing API for manipulating the list. But such class should not do transfer(account acFrom, account acTo, amount), that either belongs to single account or to transfersManager. The list of accounts may be responsible for suggestAccountsForSavingTransfer, which may return some general account and some saving account from it's list.
• try to hide inner implementation of classes, so no by-products of calculations are gettable trough API, no algorithms usage is explicitly stated/visible, try to keep the API minimal (but powerful enough), exposing only results on the desired level of abstraction. So anytime later you may completely change inner implementation, build it from other structures/data, but as long as you fulfil the API, nobody outside should notice.

Hmmm... anyway, I really like what you wrote here:

"I'm noticing that reading doesn't really help much as much as writing programs and running into issues and solving them."

This is absolutely spot on. All this general babbling around usually makes little sense when hit by real world problem.

Also refactor a lot. Not just the code, and implementations, but as learning is your goal, dive deep into finished project and go trough all the API, all the abstraction, and try to imagine something better, easier to use, easier to understand it's responsibilities, easier to read and get the idea and eventually easier to implement (although sometimes to get better API and abstraction the implementation can be more bloated, but the cost of bloat has to be clearly justified by the cleanliness of resulting API/abstraction).

Answer 3

Function two could simply be:

void stringOps::displayString()
{
    cout << this->arr << endl; // notice the "this"
}

Its usage:

int main()
{
    stringOps MyObject;
    MyObject.String = "This is displayed by (This) object";
    MyObject.DisplayString(); // Display "This" Object String
    return 0;
}

Function Three could be written as below and be used the same as Function Two:

void stringOps::reverseStr()
{
    reverse(this->arr.begin(), this->arr.end());
}