6
\$\begingroup\$

I have just learned classes and was practicing some examples from the textbook when I came to a screeching halt.

Here's the program I have written, which is complete but not optimal. What this program does is that it inputs information about the company and then lets you display the data or search for a specific set of data.

This is a self-learning exercise, not homework and not for work either.

#include<iostream>
#include<cstring>
#include <conio.h>
using namespace std;

class company
{
    public:
    char companyname[20],companytype[10],companycode[5];
    int yearlyturnover;

    void displaydata()
    {
        cout<<companyname[20];
        cout<<companytype[10];
        cout<<companycode[5];
        cout<<yearlyturnover;
    }


};

int main()
{
 int count,num,option;


cout<<"How many companies do you want to input?:";
    cin>>num;

for(count=0;count<num;count++)
{
    company com1[count];
    cout<<"\nEnter the name of the company:";
        cin.getline(com1[count].companyname,20);
    cout<<"\nEnter the code of the company";
        cin.getline(com1[count].companycode,5);     
    cout<<"\nEnter the Type of company: Public Ltd. or Private Ltd.";
        cin.getline(com1[count].companytype,10);    
    cout<<"\nInput the yearly turnover of the company:";
        cin>>com1[count].yearlyturnover;    
}

cout<<"\nPress 1 to display the data:";
cout<<"\nPress 2 to search for a company";
    cin>>option;
switch(option)
{

case 1: for(count=0;count<num;count++)
            {
            company com1[count];
            com1[count].displaydata();
            }
        break;

case 2: for(count=0;count<num;count++)
        {
            char input[10];
            company com1[count];
        cout<<"\nPlease enter the company type:";
            cin.getline(input,10);
        if(strcmp(input,com1[count].companytype)==0)
        {
            com1[count].displaydata();
        }   

        }
        break;


default:cout<<"\nError: terminating program...";    
};

 getch();
}
\$\endgroup\$
  • 1
    \$\begingroup\$ Welcome to Code Review! I made an edit to the title to summarize what the code does, as all questions here are about improving code. I hope you get some great answers! \$\endgroup\$ – Phrancis Aug 26 '16 at 4:29
  • 1
    \$\begingroup\$ It seems that your count variable at the beginning of main ia only used as an iterator. I would suggest declaring it directly into the for like this: for(int count = 0; count < num; count++) \$\endgroup\$ – WhiteEyeTree Aug 26 '16 at 12:20
4
\$\begingroup\$

First, Welcome to code review, and a very nice question for a beginning self learner in C++.

user1118321 is correct about a everything he says, I'm going to emphasis one or two items and provide some references before I mention a few other things.

Use Functions:
Functions allow one to break up their code into smaller, easier to understand logical blocks. When designing software it is often easier to break a complex problem into smaller problems that are easier to program. Some people use flow charts to map out their program, and high level flow charts will only contain the functions. These will be followed by lower level flow charts that map out the logic of each function.

Using functions allows one to decrease the amount of code written in some cases.

The main() function should be primarily used to set up the environment for the rest of the program, call a function to execute the rest of the program and catch any errors.

If you find yourself repeating code move that code to a function so it only needs to be written and debugged once. This is actually a programming principle generally referred to as Don't Repeat Yourself and you may see it referred to here on code review as DRY.

Both functions and classes should follow the Single Responsibility Principle (SRP). The Single Responsibility Principle is that a function should do one thing, and one thing only. This makes code easier to write, debug and maintain. It also makes the function easier to reuse. The Single Responsibility Principle may also be a reference to Demeter's Law (I may be wrong about them being the same principle).

Another programming principle sometimes mentioned is Keep It Simple (KISS), although this isn't just for programming.

You may want to look into SOLID programming as well.

A more comprehensive list of programming principles can be found here.

Use Existing Classes
A long time ago when I started using C++ the Standard Template Library (STL), sometimes called the Standard Library, had not yet been defined or implemented. We had to create our own strings, queues, vectors, stacks and other container classes. Using the C++ container classes and the standard library requires A Lot Less Code and makes it much simpler to write correct software. Two great website to become familar with if you are going to write C++ code are CPlusPlus and the C++ reference.

These websites are better than using a book as a reference because it remains up to date, and documents the C++11, C++14 and C++17 standards all in one place.

The string container class provides one with much more functionality then cstrings.

The follow is new content not previously mentioned.

Magic Numbers
The term magic number is sometimes used for numeric constants in code that are not obvious. By Not Obvious I mean that it's not obvious what the number represents. Non-symbolic references to numbers should be replaced either by named constants, or enums (enumerator types). An example of a named constant is:

constexpr double PI = 3.1415926;

Now in your code below, it is somewhat obvious what 1 and 2 are, but generally switch statements use enums or symbolic constants.

    std::cout << "\nPress 1 to display the data:";
    std::cout << "\nPress 2 to search for a company";
    cin >> option;

    switch(option)
    {
        case 1:
           for(count = 0; count < num; count++)
           {
               company com1[count];
               com1[count].displaydata();
           }
           break;

        case 2:
            for(count = 0; count < num; count++)
            {
                char input[10];
                company com1[count];

                std::cout << "\nPlease enter the company type:";
                cin.getline(input, 10);

                if(strcmp(input,com1[count].companytype) == 0)
                {
                    com1[count].displaydata();
                }

            }
            break;

        default:
            std::cout<<"\nError: terminating program...";
    };

Please note that the default case should probably have a break; statement as well, especially if is embedded within another control statement such as a loop.

Readability and Maintainability
Making the code more readable makes it more maintainable by you or anyone else that needs to add functionality.

The code would be much more readable if it was consistently indented as I show above in the included section of the code. Blocks of code within { and } should be indented.

Operators in control statements such as for() loops, while loops and if statement should be separated by spaces.

Use vertical spacing to make the code easier to read as well.

In C++ and more modern languages it is more common to create variables as you need them. I would change the above code to

    std::cout << "\nPress 1 to display the data:";
    std::cout << "\nPress 2 to search for a company";

    int option;
    cin >> option;

This allows the maintainer to know the type of variable without jumping all over the code.

BUGS
This code may work due to the specific compiler you are using, but the fact is that you create the company array at least 3 times. Once in every loop that I see. The variable com1 should be created once at the top of main where count, num and option are created.

company com1[count];

The variable com1 should probably be created as vector com1;

This would provide a variable sized array of companies. The code for inputing the companies wouldn't change, but the code for displaying a company or searching for a company can change:

for (auto thiscompany: com1)
{
    thiscompany.displaydata();
} 

You could use the find_if() function instead of the current search.

\$\endgroup\$
  • \$\begingroup\$ Thanks pacmaninbw for all the helpful comments. This is not an excuse, but my knowledge of C++ syntax and coding is really limited. This was an attempt to see if the code that I have written is close to the industry standard or not. \$\endgroup\$ – returnNULL Aug 28 '16 at 16:12
3
\$\begingroup\$

This is a really good start! This code is straightforward and easy to understand, which is a plus when you want to update it in 6 months or a year to add new functionality. I see a few things that could be improved:

Improved Encapsulation

Your class is very straightforward. However, I would make the member data (companyname, companytype, and companycode) private to make it easier to track changes to it. I actually had this happen to me at work recently. There was a class with public data members and we needed to figure out where a particular public member was getting updated to a bad value. If we had written an accessor (say, setCompanyName()) method, we could have just put a breakpoint there and written a simple test to stop when the data went bad. But because it was public, we had to search for every location where that value was touched in our entire code base and figure out which one was the bad access. It cost us a lot of time. Having some accessors (like setCompanyName()) would keep the data private and give you a single point where the changes occur.

Use Functions

You put all of the logic to your program (except the displaydata() method) into main(). You can make the code easier to follow by breaking out the different chores that its doing into their own functions. I would do something like this:

int getNumberOfCompanies()
{
    int num;
    cout << "How many companies do you want to input?:";
    cin >> num;
    return num;
}

void getCompany(company& co)
{
    cout << "\nEnter the name of the company:";
    cin.getline(co.companyname, 20);

    cout << "\nEnter the code of the company";
    cin.getline(co.companycode, 5);

    cout << "\nEnter the Type of company: Public Ltd. or Private Ltd.";
    cin.getline(co.companytype, 10);

    cout << "\nInput the yearly turnover of the company:";
    cin >> co.yearlyturnover;
}

Similarly, I'd break out searching and displaying the array into separate functions as well.

Use Classes When You Can

You've made your own class, but don't forget to use the existing ones! Instead of having plain old C arrays of characters for your strings, you should use std::string. It can save you a lot of hassles and errors over using C strings.

\$\endgroup\$
  • \$\begingroup\$ Did you run this code? It segv's for me. \$\endgroup\$ – pacmaninbw Aug 26 '16 at 16:02
  • 1
    \$\begingroup\$ It does not segv for me. I do see that the prompt suggests "Public Ltd." or "Private Ltd.", which are both longer than the available 10 characters, though the getline should take care of that. I do have other problems with it, like cin accepting characters for the next prompt and essentially ignoring the next prompt. But that was in the original code, too. My main point with the above code was just to break the functionality up into to functions, not to fix all the problems with it. But good catch! FWIW, the static analyzer also doesn't flag any problems with it. \$\endgroup\$ – user1118321 Aug 26 '16 at 21:12
  • \$\begingroup\$ Thanks user1118321 for all the helpful comments. This is not an excuse, but my knowledge of C++ syntax and coding is really limited. This was an attempt to see if the code that I have written is close to the industry standard or not. \$\endgroup\$ – returnNULL Aug 28 '16 at 16:13

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.