Input: careermonk

Output: camonk

Input: mississippi

Output: m

I want to improve this code.

#include <iostream>
#include <string>

#define max 1000

class Stack

 int top;
public:
 char stk[max];
 Stack()
 
 top = -1;
 
 bool isStackEmpty();
 void push(char);
 char pop();
 std::string modifyString(std::string);
private:
 void removeAdjacentDuplicate(std::string);
;

bool Stack::isStackEmpty()

 if (top == -1)
 return true;
 else
 return false;


void Stack::push(char val)

 int s = max - 1;
 if (top < s)
 
 top = top + 1;
 stk[top] = val;
 
 else
 std::cerr << "Stack Overflow n";


char Stack::pop()

 if (isStackEmpty() == true)
 std::cerr << "Stack Underflow n";
 else
 
 --top;
 return stk[top + 1];
 


void Stack::removeAdjacentDuplicate(std::string str)

 int len = str.length();
 for (int i = 0; i < len; i++)
 
 if (isStackEmpty() == true)
 
 push(str[i]);
 
 else if (str[i] == stk[top])
 
 int discard = pop();
 
 else
 
 push(str[i]);
 
 


std::string Stack::modifyString(std::string str)

 removeAdjacentDuplicate(str);
 str.resize(top + 1);
 for (int i = 0; i <= top; i++)
 
 str[i] = stk[i];
 
 str[top + 1] = '';
 return str;


int main()

 Stack stack1;
 std::string str = "careermonk";
 std::cout << "Orignal string : " << str << "n";
 str = stack1.modifyString(str);
 std::cout << "New string : " << str << "n";

 Stack stack2;
 std::string str2 = "mississippi";
 std::cout << "Original string : " << str2 << "n";
 str2 = stack2.modifyString(str2);
 std::cout << "New string : " << str2 << "n";

std::array

You could use std::array and get rid of max define. In general, I'd like to suggest avoiding defines which can conflict with any stl names by any cost.

One of Windows header has its own #define max and this is horrible when your code wants std::numeric_limits<int>::max(), std::max(a, b, c), etc.

Things are even worse if you're writing header only library:

#if defined max
# undef max
# define HAD_MAX
#endif 

// ... C++ code ...

#if defined HAD_MAX
# define max(a, b) ... // copy-paste from Windows.h
#endif

Stack::removeAdjacentDuplicate signature

Signature of Stack::removeAdjacentDuplicate(std::string) can be improved:

At least, it doesn't actually removes anything. It perform some kind of initialization from given string, so consider choosing more intuitive name.

Additionally, avoid copying parameter - you can use const std::string&

Stack::modifyString

Stack::modifyString also says nothing about its purpose and it also constructs another string instead of modifying something. Consider better naming.

Use standard library

When copying N characters, consider using standard library functions instead of for loop: strncpy, memcpy, std::copy_n and others works fine, looks clear and may perform some performance optimizations.

Use standard library (alternative)

As alternative, you can construct new string directly from Stack::stk. There is corresponding constructor: std::string(const char* s, size_t n);. In this case you can pass all std::string parameters by const reference.

BTW, I'm assure you're already bored with my variable naming suggestions, so I don't need to add another one here :)

Non-const public members are dangerous

Consider using some naming conventions

It's hard to determine whether some variable is a class member or not. In order to simplify your work, make member variables somehow. You can use m_member, member_, etc.

Avoid arbitrary limits

The definition of max (and especially as a #define rather than as a C++ constant) is worrying. Especially as the checking simply prints a diagnostic and then ignores the action that was requested.

Simple refactorings

if (test) return true; else return false; is the same as return test;. So we can write

bool Stack::isStackEmpty()

 return top == -1;

Similarly, testing ==true is redundant:

 if (isStackEmpty())

A more involved refactoring

void Stack::removeAdjacentDuplicate(std::string str)

 int len = str.length();
 for (int i = 0; i < len; i++)
 
 if (isStackEmpty() == true)
 
 push(str[i]);
 
 else if (str[i] == stk[top])
 
 int discard = pop();
 
 else
 
 push(str[i]);
 
 

Here, we're iterating over the elements of str, so we can use for (char c: str), which is clearer and less error-prone than using indexes.

The first and last branch are the same, so we can change the condition to bring them together:

void Stack::removeAdjacentDuplicate(const std::string& str)

 for (auto c: str) 
 if (isStackEmpty() 

Think like standard algorithms

We have standard algorithms - one similar to our needs is std::unique(). This makes a single pass over its input, and returns an iterator to the new end position.

If we make a function with a similar interface, we can use the 'erase/remove' idiom to replace characters in a string:

#include <string>

std::string repeatedly_remove_duplicates(std::string s)

 s.erase(repeatedly_remove_duplicates(s.begin(), s.end()), s.end());
 return s;

How do we implement repeatedly_remove_duplicates()? I would assume a single pass called remove_duplicates(), and call it until the returned end doesn't change:

template <typename Iter>
Iter repeatedly_remove_duplicates(Iter begin, Iter end)

 Iter new_end;
 while ((new_end = remove_duplicates(begin, end)) != end) 
 end = new_end;
 
 return end;

(I've made this a template, so it can work with any container. The also allows us to make repeatedly_remove_duplicates() a template to work with any string type, including std::wstring, for instance).

Now, we need an implementation of remove_duplicates. We can do this in-place:

template <typename Iter>
Iter remove_duplicates(Iter begin, Iter end)

 auto dest = begin;
 if (begin == end) return dest; 

 auto prev = begin;
 bool is_duplicate = false;
 while (++begin != end) 
 if (*begin == *prev) 
 is_duplicate = true;
 else 
 if (!is_duplicate) 
 *dest++ = *prev;
 
 is_duplicate = false;
 prev = begin;
 
 

 if (!is_duplicate) 
 *dest++ = *prev;
 

 return dest;

There are other ways to do this; we might use std::adjacent_find(), for example:

#include <algorithm>
#include <functional>

template <typename Iter>
Iter remove_duplicates(Iter begin, Iter end)

 using namespace std::placeholders;
 using not_equal_to = std::not_equal_to<typename Iter::value_type>;

 auto dest = begin;

 do 
 Iter pair = std::adjacent_find(begin, end);
 if (dest != begin) 
 std::copy(begin, pair, dest);
 dest += std::distance(begin, pair);
 else 
 dest = pair;
 
 while (begin != end);

 return dest;

Putting it all together

#include <algorithm>
#include <functional>

template <typename Iter>
Iter remove_duplicates(Iter begin, Iter end)

 using namespace std::placeholders;
 using not_equal_to = std::not_equal_to<typename Iter::value_type>;

 auto dest = begin;

 do 
 Iter pair = std::adjacent_find(begin, end);
 if (dest != begin) 
 std::copy(begin, pair, dest);
 dest += std::distance(begin, pair);
 else 
 dest = pair;
 
#ifdef REMOVE_PAIR_ONLY
 begin = pair;
 if (pair != end) 
 begin += 2;
 
#else
 begin = std::adjacent_find(pair, end, not_equal_to());
 if (begin != end) 
 ++begin;
 
#endif
 while (begin != end);

 return dest;


template <typename Iter>
Iter repeatedly_remove_duplicates(Iter begin, Iter end)

 Iter new_end;
 while ((new_end = remove_duplicates(begin, end)) != end) 
 end = new_end;
 
 return end;



#include <string>
std::string repeatedly_remove_duplicates(std::string s)

 s.erase(repeatedly_remove_duplicates(s.begin(), s.end()), s.end());
 return s;


#include <iostream>
int main(int argc, char **argv)

 for (auto i = 1; i < argc; ++i) 
 std::cout << argv[i] << " ⇒ "
 << repeatedly_remove_duplicates(argv[i])
 << std::endl;