4
\$\begingroup\$

I'm doing some coding challenges to get better practiced at my coding. I'm trying to do one that requires us to do a ROT13. I've got the implementation correct, and I just want to know a couple of things.

The space character is showing up as a crazy character in my terminal screen, which I'm assuming is because of character encoding, although I could be wrong. Also, how does my code look?

#include <iostream>
#include <string>
using namespace std;

const int LOWER_A = 97;
const int LOWER_M = 109;
const int LOWER_N = 110;
const int LOWER_Z = 122;

const int UPPER_A = 65;
const int UPPER_M = 77;
const int UPPER_N = 78;
const int UPPER_Z = 90;

string rot(string input) {
    int inputSize = input.size();
    int index = 0;

    while(index != inputSize) {
        if(input[index] >= LOWER_A && input[index] <= LOWER_M)
            input[index] = input[index] + 13;
        else if(input[index] >= LOWER_N && input[index] <= LOWER_Z)
            input[index] = input[index] - 13;
        else if(input[index] >= UPPER_A && input[index] <= UPPER_M)
            input[index] = input[index] + 13;
        else if(input[index] <= UPPER_N && input[index] <= UPPER_Z)
            input[index] = input[index] - 13;

        index++;
    }
    return input;
}

int main() {
    string plaintext;
    string cypher;

    cout << "input: ";
    getline(cin,plaintext);
    cypher = rot(plaintext);
    cout << cypher << endl;

    return 0;
}
\$\endgroup\$
6
\$\begingroup\$

A few fairly minor things:


Constants for the characters

Instead of using the constants, you can just use a character inline:

if(input[index] >= 'a' && input[index] <= 'm')

It's worth noting though that a literal character will be a char rather than an int like your constants are (won't affect anything in this program though).


using namespace std;

It's fine in short programs like this, but as you hinted that you're fairly new to C++, I wanted to mention that this can be a bad habit to get into. In particular, in large applications with many different segments of code, this can cause naming clashes.

This question on SO addresses the problems it can cause.


Naming

I would probably call rot, rot13 since there are other kinds of rotations. This is mainly me just being overly picky though :).


Strange Character For Space

The space character is showing up as a crazy character in my terminal screen, I'm assuming it's because of character encoding, although I could be wrong.

rot13 traditionally only operates on alpha characters (a-z and A-Z), so your space characters should not be changing.

I believe your program has a bug in it:

else if(input[index] <= UPPER_N && input[index] <= UPPER_Z)

If this is not a bug, you should know that this will be always be true if input[index] <= UPPER_N, so really there's no point in the second conditional.

Anyway, I think what's happening is that the space character (and anything under 'N') is falling into this block when it shouldn't be.

Space is ASCII 32, so it's getting changed to 19, which is not a printable character.


while vs for

Your loop seems to fit the construct of a for loop very well, so I might consider using that:

for (int inputSize = input.size(), index = 0; index != inputSize; ++index) {

}

This comes down to personal preference though.


You should usually try to match the return type of a method

std::string::size() doesn't return an int; it returns a std::string::size_type which is always (in every implementation I've ever seen anyway), a size_t (which is in turn an unsigned 32 or 64 bit integer depending on the platform and compiler).

Anyway, when working with standard containers (or really APIs in general), unless you have a reason to use a different type, I would try to stick with the return type specified. For example:

for (std::string::size_type len = input.size(), idx = 0; idx != len; ++idx) {
    //input[idx] = ...;
}

A potential implementation

I suspect that the if-else tree can be simplified a bit using isalpha and family, but this is what I might implement your algorithm like;

std::string rot13(std::string input) {

    for (std::string::size_type len = input.length(), idx = 0; idx != len; ++idx) {
        if (input[idx] >= 'a' && input[idx] <= 'm') {
            input[idx] = input[index] + 13;
        } else if (input[idx] >= 'n' && input[idx] <= 'z') {
            input[idx] = input[idx] - 13;
        } else if(input[idx] >= 'A' && input[idx] <= 'M') {
            input[index] = input[index] + 13;
        } else if(input[idx] >= 'N' && input[idx] <= 'Z') {
            input[index] = input[index] - 13;
        }
    }

    return input;

}

To get a little carried away for a moment, you could also implement it using iterators. Using iterators would allow you to apply it to any container that implements iterators (all of the standard containers and plain pointers).

template <typename Iter>
void rot13(Iter begin, const Iter& end) {

    while (begin != end) {

        //Doesn't need to be here, but I'm lazy and don't like
        //typing *begin over and over again.
        char& c = *begin;

        if (c >= 'a' && c <= 'm') {
            c += 13;
        } else if (c >= 'n' && c <= 'z') {
            c -= 13;
        } else if (c >= 'A' && c <= 'M') {
            c += 13;
        } else if (c >= 'N' && c <= 'Z') {
            c -= 13;
        }

        ++begin;

    }

}

Note that this modifies a container in place rather than creating a copy:

char str[] = "Hello World";
rot13(str, str + strlen(str));
std::cout << str << std::endl;
rot13(str, str + strlen(str));
std::cout << str << std::endl;

Would output:

Uryyb Jbeyq
Hello World

You could of course modify it to operate on a copy instead though.

\$\endgroup\$
6
  • \$\begingroup\$ “always use braces” – I’m against that. It needlessly clutters the code, and the error you conjure up has never happened to me. \$\endgroup\$ Aug 13 '12 at 12:27
  • \$\begingroup\$ Konrad, from what I can tell, programmers seem divided on the issue. I agree with you though, it seems cluttery to me. What's generally done on larger projects? You're not seeing the same weird character display in your terminal? \$\endgroup\$
    – Sir_Blake_
    Aug 14 '12 at 0:07
  • \$\begingroup\$ @KonradRudolph Fair enough. I mostly just prefer the look of it more so than the ability to add more lines in without having to add the braces. I have removed the section though as there's not really a point to it in hindsight. \$\endgroup\$
    – Corbin
    Aug 14 '12 at 1:26
  • \$\begingroup\$ @Corbin About namespaces, is it bad practice to use something like 'using std::cout' as opposed to writing 'std::cout' a bunch of times? Thanks a lot for your help, I'm already trying to implement some of this. \$\endgroup\$
    – Sir_Blake_
    Aug 15 '12 at 1:40
  • \$\begingroup\$ @Sir_Blake_ Hopefully someone more knowledgeable than I will comment on this. But anyway, the end goal is to leave the global namespace alone as much as possible. Anytime you think of using a using statement, basically ask yourself if the statement will affect anything other than the immediate section you're using it in. For example, using std::cout; in a header file unwrapped (not in a class or function) is a bad idea. That will mean that any file that includes that file will also have std::cout pulled into the global namespace (though I doubt anyone would ever name anything cout) \$\endgroup\$
    – Corbin
    Aug 15 '12 at 2:34
4
\$\begingroup\$

Like I said elsewhere, avoid loops where possible. This reduces the chance of making off-by-one or buffer overflow errors. C++ offers algorithms for ranges which should be used preferably.

In your case, that’d be std::transform. Combined with C++11 lambdas, this makes the code terse, readable and robust.

std::string rot13(std::string text) {
    std::transform(
        begin(text), end(text), begin(text),
        [] (char c) -> char {
            if (not std::isalpha(c))
                return c;

            char const pivot = std::isupper(c) ? 'A' : 'a';
            return (c - pivot + 13) % 26 + pivot;
        });
    return text;
}

Alternatively, this lambda could also be written as a free-standing function but I prefer to keep scope limited, and since the function presumably wouldn’t be used anywhere else and is reasonably short, it’s perfectly suited for a lambda.

\$\endgroup\$
1
  • \$\begingroup\$ I'll look into the new additions to C++. I'm still trying to grasp the basics first. Thanks for your help! \$\endgroup\$
    – Sir_Blake_
    Aug 15 '12 at 1:12
0
\$\begingroup\$

To add to Corbin's reply, always prefer passing immutable input parameter by constant reference, this way you'll protect yourself from unwanted copying of the object which often results in some performance penalties.

std::string rot13(const std::string &input)
\$\endgroup\$
1
  • 1
    \$\begingroup\$ The code copies the string anyway, by design. OP’s way is totally legitimate, and just as efficient. \$\endgroup\$ Aug 13 '12 at 12:28

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.