Skip to main content
added note about getters and setters
Source Link
Edward
  • 66.6k
  • 4
  • 118
  • 282

The review by G. Sliepen has already covered most of what I would have said, so this is intended to be a complementary review addressing points not mentioned there.

Eliminate unused variables

The variable temp in your Library::takeInput() function is defined but never used. If you are just using the getchar() as a pause and don't care about the actual value, just write getchar();. Since unused variables are a sign of poor code quality, you should seek to eliminate them. Your compiler is probably smart enough to warn you about such things if you know how to ask it to do so.

Fix conflicting declarations

In the Library.cpp file, most of the functions are declared static but they have already been declared within the Library.h file and are therefore implicitly extern. Those are conflicting declarations that your compiler should warn you about. This problem is best resolved by converting this namespace into a proper class and not trying to force it to be a singleton, as G. Sliepen has already noted.

Use RAII

Whenever you find yourself writing code that looks like this:

Library::init(); //initiates program    
Library::run(); //run program

it should trigger a warning in your brain. What happens if the user tries to invoke run() before init()? Better design is to use RAII and make it so that functions, libraries and classes are easy to use correctly. In this case, changing Library to a class, and moving the code in init into a constructor means that the Library will be ready to use as soon as it exists.

Eliminate duplicate text

The string "db.txt" exists in three different places in two different files. Better would be to have it in a single named variable (perhaps a static constexpr variable in the Library class) and then refer to it where needed.

Don't write trivial getters and setters

The Book object has a changeAvailability and getAvailability public functions. This means that any other code or object can alter this data item. If that's the intent, better would be to simply make availability a public data member. Don't write Java in C++. See C.131 for details.

The review by G. Sliepen has already covered most of what I would have said, so this is intended to be a complementary review addressing points not mentioned there.

Eliminate unused variables

The variable temp in your Library::takeInput() function is defined but never used. If you are just using the getchar() as a pause and don't care about the actual value, just write getchar();. Since unused variables are a sign of poor code quality, you should seek to eliminate them. Your compiler is probably smart enough to warn you about such things if you know how to ask it to do so.

Fix conflicting declarations

In the Library.cpp file, most of the functions are declared static but they have already been declared within the Library.h file and are therefore implicitly extern. Those are conflicting declarations that your compiler should warn you about. This problem is best resolved by converting this namespace into a proper class and not trying to force it to be a singleton, as G. Sliepen has already noted.

Use RAII

Whenever you find yourself writing code that looks like this:

Library::init(); //initiates program    
Library::run(); //run program

it should trigger a warning in your brain. What happens if the user tries to invoke run() before init()? Better design is to use RAII and make it so that functions, libraries and classes are easy to use correctly. In this case, changing Library to a class, and moving the code in init into a constructor means that the Library will be ready to use as soon as it exists.

Eliminate duplicate text

The string "db.txt" exists in three different places in two different files. Better would be to have it in a single named variable (perhaps a static constexpr variable in the Library class) and then refer to it where needed.

The review by G. Sliepen has already covered most of what I would have said, so this is intended to be a complementary review addressing points not mentioned there.

Eliminate unused variables

The variable temp in your Library::takeInput() function is defined but never used. If you are just using the getchar() as a pause and don't care about the actual value, just write getchar();. Since unused variables are a sign of poor code quality, you should seek to eliminate them. Your compiler is probably smart enough to warn you about such things if you know how to ask it to do so.

Fix conflicting declarations

In the Library.cpp file, most of the functions are declared static but they have already been declared within the Library.h file and are therefore implicitly extern. Those are conflicting declarations that your compiler should warn you about. This problem is best resolved by converting this namespace into a proper class and not trying to force it to be a singleton, as G. Sliepen has already noted.

Use RAII

Whenever you find yourself writing code that looks like this:

Library::init(); //initiates program    
Library::run(); //run program

it should trigger a warning in your brain. What happens if the user tries to invoke run() before init()? Better design is to use RAII and make it so that functions, libraries and classes are easy to use correctly. In this case, changing Library to a class, and moving the code in init into a constructor means that the Library will be ready to use as soon as it exists.

Eliminate duplicate text

The string "db.txt" exists in three different places in two different files. Better would be to have it in a single named variable (perhaps a static constexpr variable in the Library class) and then refer to it where needed.

Don't write trivial getters and setters

The Book object has a changeAvailability and getAvailability public functions. This means that any other code or object can alter this data item. If that's the intent, better would be to simply make availability a public data member. Don't write Java in C++. See C.131 for details.

removed faulty extra punctuation
Source Link
Edward
  • 66.6k
  • 4
  • 118
  • 282

The review by G. Sliepen has already covered most of what I would have said, so this is intended to be a complementary review addressing points not mentioned there.

Eliminate unused variables

The variable temp in your Library::takeInput() function is defined but never used. If you are just using the getchar() as a pause and don't care about the actual value, just write getchar();. Since unused variables are a sign of poor code quality, you should seek to eliminate them. Your compiler is probably smart enough to warn you about such things if you know how to ask it to do so.

Fix conflicting declarations

In the Library.cpp file, most of the functions are declared static but they have already been declared within the Library.h file and are therefore implicitly extern. Those are conflicting declarations that your compiler should warn you about. This problem is best resolved by converting this namespace into a proper class and not trying to force it to be a singleton, as G. Sliepen has already noted.

Use RAII

Whenever you find yourself writing code that looks like this:

Library::init(); //initiates program    
Library::run(); //run program

it should trigger a warning in your brain. What happens if the user tries to invoke run() before init()? Better design is to use RAII and make it so that functions, libraries and classes are easy to use correctly. In this case, changing Library to a class, and moving the code in init into a constructor means that the Library will be ready to use as soon as it exists.

Eliminate duplicate text

The string "db.txt" exists in three different places in two different files. Better would be to have it in a single named variable (perhaps a static constexpr variable in the Library class`class) and then refer to it where needed.

The review by G. Sliepen has already covered most of what I would have said, so this is intended to be a complementary review addressing points not mentioned there.

Eliminate unused variables

The variable temp in your Library::takeInput() function is defined but never used. If you are just using the getchar() as a pause and don't care about the actual value, just write getchar();. Since unused variables are a sign of poor code quality, you should seek to eliminate them. Your compiler is probably smart enough to warn you about such things if you know how to ask it to do so.

Fix conflicting declarations

In the Library.cpp file, most of the functions are declared static but they have already been declared within the Library.h file and are therefore implicitly extern. Those are conflicting declarations that your compiler should warn you about. This problem is best resolved by converting this namespace into a proper class and not trying to force it to be a singleton, as G. Sliepen has already noted.

Use RAII

Whenever you find yourself writing code that looks like this:

Library::init(); //initiates program    
Library::run(); //run program

it should trigger a warning in your brain. What happens if the user tries to invoke run() before init()? Better design is to use RAII and make it so that functions, libraries and classes are easy to use correctly. In this case, changing Library to a class, and moving the code in init into a constructor means that the Library will be ready to use as soon as it exists.

Eliminate duplicate text

The string "db.txt" exists in three different places in two different files. Better would be to have it in a single named variable (perhaps a static constexpr variable in the Library class`) and then refer to it where needed.

The review by G. Sliepen has already covered most of what I would have said, so this is intended to be a complementary review addressing points not mentioned there.

Eliminate unused variables

The variable temp in your Library::takeInput() function is defined but never used. If you are just using the getchar() as a pause and don't care about the actual value, just write getchar();. Since unused variables are a sign of poor code quality, you should seek to eliminate them. Your compiler is probably smart enough to warn you about such things if you know how to ask it to do so.

Fix conflicting declarations

In the Library.cpp file, most of the functions are declared static but they have already been declared within the Library.h file and are therefore implicitly extern. Those are conflicting declarations that your compiler should warn you about. This problem is best resolved by converting this namespace into a proper class and not trying to force it to be a singleton, as G. Sliepen has already noted.

Use RAII

Whenever you find yourself writing code that looks like this:

Library::init(); //initiates program    
Library::run(); //run program

it should trigger a warning in your brain. What happens if the user tries to invoke run() before init()? Better design is to use RAII and make it so that functions, libraries and classes are easy to use correctly. In this case, changing Library to a class, and moving the code in init into a constructor means that the Library will be ready to use as soon as it exists.

Eliminate duplicate text

The string "db.txt" exists in three different places in two different files. Better would be to have it in a single named variable (perhaps a static constexpr variable in the Library class) and then refer to it where needed.

Source Link
Edward
  • 66.6k
  • 4
  • 118
  • 282

The review by G. Sliepen has already covered most of what I would have said, so this is intended to be a complementary review addressing points not mentioned there.

Eliminate unused variables

The variable temp in your Library::takeInput() function is defined but never used. If you are just using the getchar() as a pause and don't care about the actual value, just write getchar();. Since unused variables are a sign of poor code quality, you should seek to eliminate them. Your compiler is probably smart enough to warn you about such things if you know how to ask it to do so.

Fix conflicting declarations

In the Library.cpp file, most of the functions are declared static but they have already been declared within the Library.h file and are therefore implicitly extern. Those are conflicting declarations that your compiler should warn you about. This problem is best resolved by converting this namespace into a proper class and not trying to force it to be a singleton, as G. Sliepen has already noted.

Use RAII

Whenever you find yourself writing code that looks like this:

Library::init(); //initiates program    
Library::run(); //run program

it should trigger a warning in your brain. What happens if the user tries to invoke run() before init()? Better design is to use RAII and make it so that functions, libraries and classes are easy to use correctly. In this case, changing Library to a class, and moving the code in init into a constructor means that the Library will be ready to use as soon as it exists.

Eliminate duplicate text

The string "db.txt" exists in three different places in two different files. Better would be to have it in a single named variable (perhaps a static constexpr variable in the Library class`) and then refer to it where needed.