3
\$\begingroup\$

Here's the problem for Java vs C++ (JAVAC):

Java and C++ use different naming conventions:

  • In Java a multiword identifier is constructed in the following manner:
    • The first word is written starting from the small letter, and the following ones are written starting from the capital letter, no separators are used. All other letters are small.
      Examples of a Java identifier are: javaIdentifier, longAndMnemonicIdentifier, name, nEERC.
  • In C++ a multiword identifier is constructed in the following manner:
    • Use only small letters in their identifiers. To separate words they use underscore character ‘_’.
      Examples of C++ identifiers are: c_identifier, long_and_mnemonic_identifier, name

Note: When identifiers consist a single word then Java and C++ naming conventions are identical:

You are writing a translator that is intended to translate C++ programs to Java and vice versa. Of course, identifiers in the translated program must be formatted due to its language naming conventions — otherwise people will never like your translator.

The first thing you would like to write is an identifier translation routine. Given an identifier, it would detect whether it is Java identifier or C++ identifier and translate it to another dialect. If it is neither, then your routine should report an error. Translation must preserve the order of words and must only change the case of letters and/or add/remove underscores.

How can I improve this code? How can I make it faster? Are there better solutions?

#include<iostream>
#include<string>
#include<cctype>

std::string convert(const std::string& s) {
    std::string result{};
    bool java = false;
    bool cpp = false;
    if (isupper(s[0]) || s[0] == '_' || s[s.size()-1] == '_') {
        return "Error!";
    }
    for (std::size_t i = 0; i < s.size(); ++i) {
        if (isupper(s[i])) {
            cpp = true;
            result += "_";
            result += tolower(s[i]);
        } else if (s[i] == '_') {
            java = true;
            if (isupper(s[i+1]) || s[i+1] == '_') {
                return "Error!";
            } else {
                result += toupper(s[++i]);
            }
        } else if (isalpha(s[i])) {
            result += s[i];
        } else {
            return "Error!";
        }
    }
    if ((java != cpp) || (!java && !cpp)) {
        return result;
    } else {
        return "Error!";
    }
}

int main() {
    do {
        std::string str;
        std::cin >> str;
        std::cout << convert(str) << '\n';
    } while (std::cin);
}
\$\endgroup\$
2
  • 1
    \$\begingroup\$ Just FYI, the statement in the problem definition: Unlike them, C++ people use only small letters in their identifiers is false. Whoever wrote that is misinformed. \$\endgroup\$
    – Edward
    Jan 11 '15 at 15:41
  • \$\begingroup\$ @Edward it doesn't really matter, it's just an exercise. And long_and_mnemonic_identifier is an extremely ugly name \$\endgroup\$ Jan 11 '15 at 15:52
4
\$\begingroup\$

First, let me note that this code compiles cleanly with no warnings. That already makes it better than much of the code that's posted for review here, so well done! With that said, here are some things that may help you improve your code.

Put loop exit conditions at the beginning

Putting your exit condition at the top of the loop helps with readability. In this case, it also shortens the code. Your main routine could instead be written like this:

int main() {
    std::string str;
    while (std::cin >> str) {
        std::cout << convert(str) << '\n';
    }
}

Consolidate constant strings

The code current contains four instances of the line return "Error!"; It would probably make sense to create a static const variable for that instead. Your compiler is probably smart enough to combine those strings anyway, but doing so explictly makes your intent clear and aids in translation if internationalization is needed at some later date.

Consider testing for exceptions

At the moment, an identifier in_?valid gets interpreted as a "C++-style" identifier (as I mentioned in the comments, few real C++ programmers actually write code like that) and is translated into in?valid as a "Java-style" identifier. However, the input in?valid is rejected as an error. Since neither identifier is valid in either language, I suspect this is an error.

Consider early bailout within the loop

If both java and cpp are set at any point during the loop, it's an error condition and so the code could check for that and bail out early. One way to do that neatly might be to define a bool ok and make it an additional loop exit condition. It would be initialized to true and then set if ever cpp and java are both set. It could also be set for the already identified error conditions, so that the end of the convert() routine could then look like this:

if (ok) {
    return result;
} else {
    return errorString;
}

Use a state machine

The code already almost implements a state machine. The only difference is that at the moment, the loop within convert() is driven by the next character rather than by a current state. I think it would be easier to address the problems noted above if a real state machine were implemented. One could implement it as a switch and have both tidy and correct code. The state variable in code I write is typically declared as an enum so that I can use named identifiers for each state.

Consider using iterators instead of character indexing

Since you've specified C++11, it may make sense to use C++ iterators to go through the input string within convert() rather than using indexing with offsets. In this case, the offset is not really of interest -- only the letters to which they point are relevant to the problem domain, so using iterators is likely to be a more natural way to address the problem than using indexing. Performance is likely to be about the same either way, since the identifiers are guaranteed to be relatively short.

\$\endgroup\$
6
\$\begingroup\$

Input validation

The string literal "Error!" is duplicated in multiple places. If you decide one day to change that text, you'd have to remember to change everywhere, which can be error prone. It's better to turn string literals into global constants.

But actually returning "Error!" is not a good way to handle invalid input. How will the caller know if the name was invalid? The caller would have to know to compare returned values against the correct string literal used internally by the method. It would be better to return an empty string instead, which is easier for the caller to validate (instead of a full string comparison), and document this fact in a comment above the function declaration. Or you could take a non-const in/out parameter and return a boolean for success/failure. (Thanks @200_success!)

Add some test cases

It's good to add test cases with expected output, so that if you want to refactor using different approaches there's an easy way to verify that everything still works. For example:

std::cout << convert("javaIdentifier").compare("java_identifier") << "\n";
std::cout << convert("longAndMnemonicIdentifier").compare("long_and_mnemonic_identifier") << "\n";
std::cout << convert("name").compare("name") << "\n";
std::cout << convert("nEERC").compare("n_e_e_r_c") << "\n";
std::cout << convert("c_identifier").compare("cIdentifier") << "\n";
std::cout << convert("long_and_mnemonic_identifier").compare("longAndMnemonicIdentifier") << "\n";
std::cout << convert("n_e_e_r_c").compare("nEERC") << "\n";
std::cout << convert("!_e_e_r_c").compare("Error!") << "\n";
std::cout << convert("n_e_e_R_c").compare("Error!") << "\n";
std::cout << convert("n_e_e_r_c_").compare("Error!") << "\n";

These should all output 0. If you make any changes, it's easy to verify by rerunning and looking for non-zero values in the output.

Simplify

The condition (java != cpp) || (!java && !cpp) can be simplified to !(java && cpp).

Other things

I think it would make sense to reverse the logic of the cpp and java variables. The current code sets cpp to true when converting from java to cpp. It would seem more intuitive to set cpp to true when cpp naming is detected. But maybe this is a matter of taste.


Lastly, a minor thing, I think it's more conventional to put a space after #include, like this:

#include <iostream>
#include <string>
#include <cctype>
\$\endgroup\$
1
  • \$\begingroup\$ You're right that returning a string "Error!" is a poor way to handle errors, but unfortunately, the particular behavior is specified in the problem. \$\endgroup\$
    – Edward
    Jan 11 '15 at 20:05
5
\$\begingroup\$

Firstly I don't agree with a particular coding convention being tied to a language, implied by your excerpt. I know this isn't the point of your question but as an aside, coding conventions develop from an amalgamation of best practice, and what your development team is happy with adopting (I've been through this painful process).

Naming of Boolean Variables

A fairly neat convention I've adopted recently is naming of boolean variables by the name suggesting something is true (i.e. an assertion). What I mean by this is that where you have java and cpp, you could have isJava and isCpp. Then, when you need to use the variable in an if statement you have:

if (isJava) {
    ...
}

That's almost human readable! There are some variants on this like languageIsJava but they can begin to be a bit wordy, but the point is that you're implying that the variable is a boolean from its name.

Use of java and cpp Variables

In this context it doesn't make sense to have a boolean variable for each supported language. The reason being is that it is possible that both variables could be true, which you correctly identify as an error. I get the feeling you're procrastinating by leaving the error checking towards the end. My first recommendation is that you create an enum which stores the detected language, and can have one of three possible values:

enum ProgrammingLanguages{
    Undefined
    Cpp,
    Java
};

Then you can write your program such that it's only trying to guess the input language whilst your enum variable is set to Undefined. Of course you will still have to handle the case where your program has determined the input language but then encountered a character it wasn't expected (e.g. thought the input was a Java name only to encounter an underscore). This is an error and your code should throw an exception or do something to notify the calling code that they buggered up. It's not your responsibility to recover from it since that scenario is undefined based on your problem definition. Just to elaborate a bit more on why you shouldn't return "Error!" is that if something goes wrong your function always returns a string. This is ambiguous since your successful value goes in the same variable as an error. Don't make life hard for yourself, throw an exception so you force the calling code to implement error handling the proper way.

Consider Using Regular Expressions

Regular Expressions (RegEx for short) are a dark art and are very powerful for string pattern matching and manipulation. In this particular case I can't give you any real answers but a PHP framework I have used in the past has an Inflector class whose job is convert between naming conventions. You can find its implementation here.

Control Statement Nesting

This is a massive bugbear of mine and I see it all too often. Your code isn't especially bad but I think it's something you should be aware of. Control statement nesting is where you have an if-statement inside an if-statement inside a while-loop inside a switch statement, and so on. Doing this increases code complexity. Your for-loop is on the edge of what I consider being too complex, but I think given your implementation, there's not a lot you can do. You want to keep your code as flat as possible because it makes it easy for someone else reading your code (and yourself for that matter), to follow the logic.

\$\endgroup\$
4
  • \$\begingroup\$ Does not even attempt a code review. -1. \$\endgroup\$ Jan 11 '15 at 19:03
  • \$\begingroup\$ There is an old saying. If you have a problem and try and solve it with regular expressions. Now you have three problems. \$\endgroup\$ Jan 11 '15 at 19:04
  • \$\begingroup\$ Cheers for the comments @CrappyExperienceBye. Would you mind suggesting why my answer doesn't review his code, it would help my future answers. Thanks \$\endgroup\$ Jan 11 '15 at 19:59
  • \$\begingroup\$ You didn't delve deep into the code, which is fine since there isn't that much code, but did provide good suggestions, so I find your answer very adequate. The RegEx part might be controversial for some, but nonetheless, I wouldn't give a -1 because of that. \$\endgroup\$
    – glampert
    Jan 12 '15 at 2:27
5
\$\begingroup\$

There is going to cause confusion and potential errors:

bool java = false;
bool cpp = false;

What happens when a bug sets them both to true?
Use a enum:

enum LanguageType {Unknown, CPP, Java }; 
LanguageType  languageDetected = Unknown;    // looks like my convention is Java

Use self documenting code to explain what is happening:

languageDetected = detectLanguage(identifier);
switch(languageDetected)
{
    case CPP:  return convertIdentifierToCpp(identifier);
    case Java: return convertIdentifierToJava(identifier);
    default:
        // however you want to show errors
        // but do something.
}

Using return "Error!"; to indicate an error is a bit problematic (As it could be an identifier by the rules you specified above). So how does the code using this functions detect an error.

Assuming you are converting a valid program then errors should be very rare and you could use exception to indicate failure (as you probably want to terminate the program anyway).

Alternatively you could alter your function signature so that it returns true/false to indicate failure and then modify the identifier in place.

But since this is the interface required by the coding site fine (just leave it).

This is going to fail if the file contains an identifier with a space in it:

    std::string str;
    std::cin >> str;

Use getline.

    std::string str;
    std::getline(std::cin, str);

Bad Loop construct

Your while loop is not correct. Note the last succesfule read will read upto but not past the eof. So the stream will be good even though there is no dat on the stream. The first attempt to read it will cause it to become bad:

do {
    std::string str;

    // This read may fail
    // If the last read read the last line and there is no input
    // this read will fail
    std::cin >> str;

    // IF the read fails then this call is going to generate bogus ouput.
    std::cout << convert(str) << '\n';
} while (std::cin);

SO always test that a read worked.
This would be a better version that checks if the read worked.

do {
    std::string str;
    if (std::cin >> str) {
        std::cout << convert(str) << '\n';
    }
} while (std::cin);

More idiomatic is to test it as part of the loop:

std::string str;
while (std::cin >> str) 
{
    std::cout << convert(str) << '\n';
} 
\$\endgroup\$

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.