Searching by name

Question

I wrote a program for my C++ class (it finished a few weeks ago) and my instructor said a comment I didn't really understand (he didn't elaborate on the comment). He said that it seems to be written in Java or seems Java-like.

I'm not going to post whole thing here (too long) but he did say the comment on a particular piece of code, wherein we were to do the ff.: (Paraphrased)

Given a list of names, allow the user to enter a search term. Return all strings which have that search term as a substring. Write your own code to determine if a string is a substring of another.

Here's what I came up with:

vector<string> Directory::searchByName(string nameArg) {
 vector<string> matches;
 for (string r : records) {
 int pos = 0;
 int last = r.length();
 bool isFound = false;
 do {
 int x = pos, a;
 for (a = 0; a < nameArg.length() && x < last; a++, x++) {
 if (toupper(nameArg[a]) != toupper(r[x])) break;
 }
 if (a == nameArg.length()) {
 matches.push_back(r);
 isFound = true;
 break;
 }
 int temp_pos = pos;
 pos = r.find(toupper(nameArg[0]), temp_pos + 1);
 if (pos == string::npos) {
 pos = r.find(tolower(nameArg[0]), temp_pos + 1);
 }
 } while ( !isFound && pos != string::npos && pos < last);
 }
 return matches;
}

The code runs (and could be optimized, I admit) but I just don't understand that comment. Did I do something wrong or unconventional in terms of C++ in my syntax? If so, what is it and how can I improve it?

Somebody asked me to include code that can be compiled. We were also practicing the three file format thing so I'm not sure how to do that here.

---

Full code:

main.cpp

#include "Directory.h"
#include <iostream>
using namespace std;
int main() {
 Directory dir;
 string searchTerm;
 // dir.lazyInit();
 // dir.saveToFile("dat.dat");
 // dir.displayAllRecords();
 dir.loadDirFromFile("dat.dat");
 cout << "This program searches for records that " << endl
 << "matches a full name or a partial name." << endl
 << "Please enter a name as prompted." << endl << endl;
 cout << " Search? ";
 getline(cin, searchTerm);
 cout << endl;
 vector<string> results = dir.searchByName(searchTerm);
 if (results.size() == 0) {
 cout << "We're sorry. There are no matches." << endl;
 } else {
 cout << "The following records are found: " << endl;
 for (string r : results) {
 cout << " " << r << endl;
 }
 }
 return 0;
}

Directory.h

#ifndef HOMEWORK_CH12_QN11_DIRECTORY_H
#define HOMEWORK_CH12_QN11_DIRECTORY_H
#include <vector>
#include <string>
class Directory {
private:
 std::vector<std::string> records;
public:
 void lazyInit();
 void displayAllRecords();
 std::vector<std::string> searchByName(std::string);
 void saveToFile(std::string);
 void loadDirFromFile(std::string);
};

Directory.cpp

#include "Directory.h"
#include <iostream>
#include <fstream>
using namespace std;
void Directory::lazyInit() {
 records = {
 "Hoshikawa Tanaka, 678-1223",
 "Joe Looney, 586-0097",
 "Geri Palmer, 223-8787",
 "Lynn Lopez, 887-1212",
 "Holly Gaddis, 223-8878",
 "Sam Wiggins, 486-0998",
 "Bob Kain, 586-8712",
 "Tim Haynes, 586-7676",
 "Warren Gaddis, 223-9037",
 "Jean James, 678-4939",
 "Ron Palmer, 486-2783"
 };
}
void Directory::displayAllRecords() {
 for (string s : records) {
 cout << s << endl;
 }
}
 vector<string> Directory::searchByName(string nameArg) {
 vector<string> matches;
 for (string r : records) {
 int pos = 0;
 //int last = r.find(',');
 //if (last == string::npos) last = r.length();
 int last = r.length();
 bool isFound = false;
 do {
 int x = pos, a;
 for (a = 0; a < nameArg.length() && x < last; a++, x++) {
 if (toupper(nameArg[a]) != toupper(r[x])) break;
 }
 if (a == nameArg.length()) {
 matches.push_back(r);
 isFound = true;
 break;
 }
 int temp_pos = pos;
 pos = r.find(toupper(nameArg[0]), temp_pos + 1);
 if (pos == string::npos) {
 pos = r.find(tolower(nameArg[0]), temp_pos + 1);
 }
 } while ( !isFound && pos != string::npos && pos < last);
 }
 return matches;
 }
void Directory::saveToFile(string filename) {
 fstream file(filename, ios::out | ios::binary);
 if (!file) return;
 for (string r : records) {
 int l = r.length();
 file.write(reinterpret_cast<char *>(&l), sizeof(l));
 file.write(r.data(), l);
 }
 file.close();
}
void Directory::loadDirFromFile(string filename) {
 const int BUFFER_SIZE = 256;
 static char buffer[256];
 fstream file(filename, ios::in | ios::binary);
 if (!file) return;
 while(file.good() && file.peek() != EOF) {
 int l = 0;
 file.read(reinterpret_cast<char *>(&l), sizeof(l));
 file.read(buffer, l);
 buffer[l] = '0円';
 records.push_back(static_cast<string>(buffer));
 }
 file.close();
}

• TheLoneWoof: I have the whole program at GitHub. github.com/TheLoneWoof1102/FA17_CSC121001/tree/master/…. I'm not sure if I can paste the whole code here but the code I posted was at Directory.cpp Line 32.
• TheLoneWoof: And for now, I just want to know what my instructor meant by my C++ code being too Java-y. Any thoughts?
• Incomputable: post everything, and in text section specific part of the code you are most concerned about. I see why instructor told that, and I’d say that it is somewhat the case, not bad though for a beginner.
• TheLoneWoof: I'm only concerned about the section of code I posted. He only mentioned the comment as he was going over that one. And it's 157 total lines of mess. We were supposed to do a bunch of tasks on the whole project, not one whole cohesive program.
• TheLoneWoof: Everything posted.

Answer 1

The code you posted is good C++, although, it does show some traits from someone who programmed Java. I would accept it in a C++ code-base in the state it is in.

Braces

The brace style you are using is an exact match with the java style guide. I can only expect that your professor expected something different. However, out of all, I can't tell which would be most c++.

void function() {
 // Java
}
void function()
{
 // Allman
}
void function()
 {
 // Whitesmiths
 }

String.length()

In java, getting the length/size of a string is done via the length() method. In C++, both length() and size() exist. In my experience, I've only seen the last one being used.

int last = r.size();

Passing arguments by value ain't cheap

If I have to compare C++ with Java, I would state that java passes all arguments as something that looks like an std::shared_ptr. In C++, the arguments are copied unless you accept them by reference.

Instead I would suggest to make the signature:

vector<string> Directory::searchByName(const string &nameArg)

Similar for the ranged based for:

for (const string &r : records)

• TheLoneWoof: Thanks, dude. As for the brace style, my instructor didn't seem to have a brace style he prefers. Does it really matter though? We also haven't gotten to references yet AFAIK (It's a two-part class).
• JVApen: Tnx for the update
• Toby Speight: @TheLoneWoof - brace style is entirely a matter of preference in new code, or of matching what's already present in existing code. Don't worry about it as long as it's consistent (within any given program or library).

Answer 2

using namespace std;

Don't.

Of what use is Directory?

This class is just a collection of functions. It serves no real purpose: It doesn't really bring any data and logic together. All it does is needlessly limit the access to the data structure with your records.

In other words: It doesn't protect any(*) invariants. Why hide the std::vector in it?

(*): Actually it does, of course, but I'm relatively sure that's not needed: You cannot add, sort, remove, ... any records.

I'd guess this is what your instructor meant with the Java-ish comment. (Because in Java, everything needs to be inside some class).

My suggestion; implement the following "interface" (as free functions):

std::vector<std::string> readRecordsFromFile(std::string filename);
// return empty on error or throw exception
// or: bool readRecordsFromFile(std::vector<std::string> &, std::string)
bool writeRecordsToFile(std::string filename);
// or ditch the return type and throw an exception on error
std::vector<std::string> findRecordsWithSubstring(
 std::vector<std::string> const & records,
 std::string const & substring);

And then, to implement the last one, use std::copy_if and a predicate where you ...

Write your own code to determine if a string is a substring of another.

bool isSubstringOf(std::string const & part, std::string const & whole);

Sanitize / Secure input

const int BUFFER_SIZE = 256;
static char buffer[256];
// ...
 int l = 0;
 file.read(reinterpret_cast<char *>(&l), sizeof(l));
 file.read(buffer, l);
 buffer[l] = '0円';

What if l is read as 34544? If you have a fixed size buffer, then at least protect against overflow! Also use the BUFFER_SIZE instead of a magic number!

Tests

Add unit tests to ensure functions you wrote (the search function, the substring test, ...) work the way you intended. Use golden files ("golden tests") to ensure your input / output works as expected.

Minor

std::endl flushes the stream

Using std::endl means: Write a \n and flush the stream. The later is a potentially costly operation. So when writing text(*) to a stream, use literal \n for all newlines except the last.

(*) Unless you write lots of text and need it to be refreshed on "screen" repeatedly. Think of "following" logging output.

• TheLoneWoof: My instructor's feedback is different than yours, for some reason. (1) He once asked me why I didn't use using namespace std in one of my projects because he instructed us to use it every time. (2) He also wanted most of our projects to have classes. He once mentioned that in the industry, always using classes is the convention.
• TheLoneWoof: Also -- I'm not really sure what you meant by the Sanitize / Secure Input. We kind of glossed over that when we did object serialization so I don't fully understand Directory::loadDirFromFile anyway. Can you, perhaps, refer me to some articles/docs?
• TheLoneWoof: And thanks for the response, by the way! It's definitely more than what I expected.
• Toby Speight: @TheLoneWoof: Sadly, instructors aren't perfect. That's not to say that you can't learn from them (of course you can), but I recommend staying open to other advice (including participating here, as you already are). You now have a better understanding of using namespace than you had when you just parroted it, and the thing about always using classes is mere cargo cult - have a look at different projects and see if you can learn when classes are useful and when they have no benefit.
• Toby Speight: Also, C++ can be a challenging language to learn to use well, because it is designed to support several styles of programming. So not only object-orientated (as you know from Java), but also procedural and functional styles of programming, too - and quite often to mix several styles in the same program, depending on what's most appropriate for each part.