Is there a way to write this code to look more professional?

[kevlwig85]

I am a beginner and I want to know if my code can be created in a more professional look?

  void User_Account::set_username(std::string name)
    {

        if(!is_username(name)) std::cerr<<"Not a valid username\n";
        username=name;
    }
  bool is_username(const std::string& s)
    {
        if(!valid_length(s)) throw std::length_error("Length Error!");
        if(!con_upper(s)) std::cerr<<"Add a uppercase letter\n";
    }
    bool con_upper(const std::string& s)
    {
        int i=0;
        char c;
        while(s[i])
            {
                c=s[i];
                if(isupper(c)) return true;
                i++;
            }
        return false;
    }

    bool valid_length(const std::string& s)
    {
        const std::string::size_type acceptable_size = 8;
        return s.size() >= acceptable_size;
    }

[AbstractionAnon]

Lines 4,9,10,19: Style suggestion: Move the conditional statement to the next line indented under the if. If you're using a debugger, this convention allows you to set a breakpoint on the conditional statement rather than having to set a breakpoint on the entire if statement.

Lines 4-5: If the user name is not valid, you go ahead and set it anyway. You now have invalid data in your class variable. You've told the user it's not valid, but you've not returned any indication to the caller of the function.

Lines 25-29: What if the length of the name is 0?
Line 28: You're returning true if the name is too long. You should be returning false.

Line 12: con_upper implies a conversion to upper, but the function only tests if the passed string is upper case.

Line 19: You're returning true on the first upper case character. You fail to test the remaining characters.

Lines 15-19: There is no need for c. You can use s[i] directly in the call to isupper.

[kevlwig85]

Awesome @AbstractionAnon all this info will help me continue to grow, as far as line 25-29 should I create an if statement like:

if(s.size()<=0) throw bad_input("No username was given");

line 4-5 throwing an exception would satisfy that issue?
line 28 I am assuming you are suggesting that I create a range for an acceptable size length for a username?

line 19: I only want to find the first instance of a upper case character if any. I want the username name to be at least 8 char with a upper case char I don't care where its positioned. Is that a bad way to express that notion?

[AbstractionAnon]

Line 25-29: You already have a bool function. Why not simply return false;? It's somewhat inconsistent to return false for one condition and throw an exception for the other. Actually this second check is unnecessary. See below.

BTW, your compiler should give you a warning on s.size() <= 0. Size() returns an unsigned type. It's pointless to compare an unsigned type < 0. Unsigned types can not be negative.

line 4-5 throwing an exception would satisfy that issue?

Yes. That assumes you have appropriate try/catch blocks. IMO, it would be simpler to make set_username to be a bool function and have the caller check the result.

line 28 I am assuming you are suggesting that I create a range for an acceptable size length for a username?

I misread your intent here. As I read it, I took it that 8 was the maximum length and that the return value of the function would be incorrect. Since you meant the name must be 8 or longer, then this statement is correct. This is why comments and meaningful names are so important. Had line 27 been "minimum_size" instead of "acceptable_size", there would have been no confusion. That would also have eliminated my comment about checking for 0 length. Checking >= 8 makes checking for > 0 unnecessary.

Lines 16-21: Again, the intent of these lines were unclear. Keep in mind that many of the posts here are from beginners and contain gross mistakes. Without comments indicating the intent of those lines, I had no way of telling what you actually intended. You may know what your code is supposed to do, but that doesn't mean that it's clear to someone else reading it for the first time.

Is that a bad way to express that notion?

No. Your loop is fine, now that I understand the intent. Personally, I would have preferred a for loop for iterating over a string of known length, but a while loop works.

[kevlwig85]

Awesome this helps me out tremendously. Better naming of my identifiers and commenting is critical to readability. I am going to take your suggestions in making the set_username into a bool and go from there. I'll more than likely be back lol.

[kevlwig85]

Change the code around a little. Have a look when you have time and tell me if the changes make sense and reflect good programming. I got rid of the is_username() because it was handling errors as well as the set_username which seemed redundant. so I created a is_valid() that returns true if the criterion of a username or password is met.
header file:

#ifndef USER_ACCOUNT_H
#define USER_ACCOUNT_H
#include <string>

//Creates a User Account
namespace Users{
    class User_Account
    {
        public:
            User_Account();

            void set_username (std::string);
            void set_password (std::string);
            std::string get_username() const{return username;}
            std::string get_password()const{return pwd;}


        private:
            std::string username;
            std::string pwd;


    };
    //helper functions
    bool is_valid(const std::string&); //returns true if username/password criterion is met
    bool valid_length(const std::string&);//check if username/password 8 char or more
    bool find_upper(const std::string&);//check username/password for a uppercase char
}

#endif // USER_ACCOUNT_H 

cpp file:

#include "User_Account.h"
#include <string>
#include <stdexcept>
#include <iostream>

namespace Users {

    User_Account::User_Account(){}//default constructor

    void User_Account::set_username(std::string name)//sets password or throws invalid_argument for failed is_valid condition
    {

        if(!is_valid(name))
        {
             throw std::invalid_argument("Username must be 8 characters or more and contain at least 1 uppercase letter");
        }
        else{username=name;}
    }

    void User_Account::set_password(std::string pass)//sets password or throws invalid_argument for failed is_valid condition
    {
        if(!is_valid(pass))
        {
            throw std::invalid_argument("Password must be 8 characters or more and contain at least 1 uppercase letter");
        }
        else{pwd = pass;}

    }


    //helper functions
    bool is_valid(const std::string& s)//returns true if length and uppercase criterion is met
    {
        return valid_length(s)&&find_upper(s);
    }

    bool find_upper(const std::string& s)//returns true if string contain at least 1 uppercase
    {
        int i=0;
        while(s[i])
            {

                if(isupper(s[i]))
                {
                     return true;
                }
                i++;
            }
        return false;
    }

    bool valid_length(const std::string& s) //checks if minimum length met
    {
        const std::string::size_type min_size = 8;
        return s.size() >= min_size;
    }
}

[cire]

Given the comment after the prototype in the definition of find_upper, has_upper would be a better name.

#include <algorithm>

    bool has_upper(const std::string& s)//returns true if string contain at least 1 uppercase
    {
         return std::find_if(s.begin(), s.end(), std::isupper) != s.end();
    }

[JLBorges]

Header file:

#include <string>

struct user_account
{
    // note: the user of the class needs to know what the minimum acceptable name length is
    //       document it in the interface, in code
    static constexpr std::string::size_type min_name_length = 8;

    // throws std::invalid_argument if length is less than min_name_length
    //     or if name does not contain an upper-case letter
    // note: establish the class invariant in the constructor
    explicit user_account( const std::string& name /* ... */ ) ;

    std::string name() const { return user_name ; }

    // ...

    private: std::string user_name ;
    // ...
};

Implementation file:

#include "user_account.h"
#include <cctype>
#include <stdexcept>

namespace
{
    // hide implementation
    // everything in the anonymous namespace has internal linkage;
    // ie. they are not programatically visible outside the implementation

    bool is_sufficiently_long( const std::string& name )
    { return name.size() < user_account::min_name_length ; }

    bool has_upper_case_letter( const std::string& name )
    {
        // note: favour range-based loops over classical loops
        // https://msdn.microsoft.com/en-us/library/jj203382.aspx
        for( char c : name ) if( std::isupper(c) ) return true ;
        return false ;
    }

    // returns the name if it is a valid name
    // if the name is not valid, does not return (throws)
    std::string valid_name( const std::string& name )
    {
        if( is_sufficiently_long(name) && has_upper_case_letter(name) ) return name ;
        throw std::invalid_argument( "username must be at least 8 characters and must contain an uppercase letter" ) ;
    }
}

user_account::user_account( const std::string& name /* ... */  ) : user_name( valid_name(name) ) {}

// ... 

[kevlwig85]

Question @JLBorges why did you choose struct over class? I thought if you can come up with a invariant you chose class over struct? Can you tell me why?

[JLBorges]

The keywords class and struct are identical except for the default member (or base class) access.

In user-defined types, I tend to declare public members before private/protected members; and most of the time, I tend to inherit publicly. So, for me, struct is less verbose than class (default access is public).

However, there are (many) coding style guides which try to make more of the (non-)distinction between the two; if one of those is in effect, be consistent and follow the guideline.

For instance, CppCoreGuidelines:

C.2: Use class if the class has an invariant; use struct if the data members can vary independently

Reason

Readability. Ease of comprehension. The use of class alerts the programmer to the need for an invariant. This is a useful convention.

https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#S-class

C.8: use class rather that struct if any member is non-public

Reason

Readability. To make it clear that something is being hidden/abstracted. This is a useful convention.

https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#S-class

[kevlwig85]

awesome thank you