Skip to main content
Code Review

Return to Answer

replaced http://stackoverflow.com/ with https://stackoverflow.com/
Source Link

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? 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? 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.

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.

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.

added 71 characters in body
Source Link
Incomputable
  • 9.7k
  • 3
  • 34
  • 73

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.

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.

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.

Source Link
Incomputable
  • 9.7k
  • 3
  • 34
  • 73

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.

lang-cpp

AltStyle によって変換されたページ (->オリジナル) /