General formatting
Keeping code nicely formatted makes it easier to read, and therefore easier to work on. This is especially important when you work on a project with multiple people, where other people may make changes to code you wrote originally, or vice versa.
Some of your lines are indented too much, specifically the first line and the final try-catch block. You are also missing spaces in some places; for example, if(c!='i' && c!='q') { should be if (c != 'i' && c != 'q') {.
There should be an option in your IDE to auto-format your code with a simple keyboard command, or even every time you save.
do-while loops
In general, I suggest using while loops rather than do-while loops, unless you have a specific reason not to. do-while isn't wrong, per se, but it is used far less often and may catch some people off-guard.
The exception is if you join a team where everybody uses do-while loops all the time, and your code would stick out if you didn't use them. "When in Rome..."
try-with-resources
Remembering to call .close() on your readers and writers is always a bit of a hassle. You also run the risk that you will hit an exception somewhere along the way and never reach the .close() at all.
The solution is to use the try-with-resources statement. Java will auto-close the resources when the code exits the try-block, no matter how that happens.
//Once 3 characters have been generated, check in the textfile if this plate exists
try (FileInputStream fstream = new FileInputStream("plates.txt");
BufferedReader br = new BufferedReader(new InputStreamReader(fstream))) {
String strLine;
while ((strLine = br.readLine()) != null) {
if (strLine.equals(returnS)) {
returnS = returnS.substring(0, 5); //returnS wipes away the 3 characters if the plate exists
counter = 0;
}
}
}
Big loop structure
Your big loop has two tasks:
- Generate a suffix of three random letters and append it to the plate string
- Check the plate string for uniqueness against an external file
These tasks are currently intertwined in a slightly confusing way; the second task appears to be a sub-operation of the first task, even though it is really just waiting for the first task to finish.
It would be clearer to structure the two tasks as separate operations inside a larger, "neutral" loop:
while (true) {
// Generate a random suffix of length 3
int counter = 0;
String randomSuffix = "";
while (counter < 3) {
char c = (char) (r.nextInt(26) + 'a');
if (c != 'i' && c != 'q') {
randomSuffix += Character.toUpperCase(c);
counter++;
}
}
// Add suffix to plate and check for uniqueness
final String possiblyUniquePlate = returnS + randomSuffix;
boolean isUnique = true;
try (FileInputStream fstream = new FileInputStream("plates.txt");
BufferedReader br = new BufferedReader(new InputStreamReader(fstream))) {
String strLine;
while ((strLine = br.readLine()) != null) {
if (strLine.equals(possiblyUniquePlate)) {
isUnique = false;
break;
}
}
} catch (Exception e) {
// We'll talk about exception-handling later...
e.printStackTrace();
isUnique = false; // We can't verify uniqueness if we can't read the file
}
// If plate is confirmed unique, set as return value and exit loop
if (isUnique) {
returnS = possiblyUniquePlate;
break;
}
}
...or better yet, break the tasks out into separate functions:
private static String generateRandomSuffix(Random r) {
// Generate a random suffix of length 3
int counter = 0;
String randomSuffix = "";
while (counter < 3) {
char c = (char) (r.nextInt(26) + 'a');
if (c != 'i' && c != 'q') {
randomSuffix += Character.toUpperCase(c);
counter++;
}
}
return randomSuffix;
}
private static boolean isPlateUnique(final String possiblyUniquePlate) {
boolean isUnique = true;
try (FileInputStream fstream = new FileInputStream("plates.txt");
BufferedReader br = new BufferedReader(new InputStreamReader(fstream))) {
String strLine;
while ((strLine = br.readLine()) != null) {
if (strLine.equals(possiblyUniquePlate)) {
isUnique = false;
break;
}
}
} catch (Exception e) {
// We'll talk about exception-handling later...
e.printStackTrace();
isUnique = false; // We can't verify uniqueness if we can't read the file
}
return isUnique;
}
Random r = new Random();
// runs until 3 characters have been generated, and the plate is unique
while (true) {
String randomSuffix = generateRandomSuffix(r);
// Add suffix to plate and check for uniqueness
final String possiblyUniquePlate = returnS + randomSuffix;
boolean isUnique = isPlateUnique(possiblyUniquePlate);
// If plate is confirmed unique, set as return value and exit loop
if (isUnique) {
returnS = possiblyUniquePlate;
break;
}
}
Random char generation
If you're always going to capitalize the letters, and you're only using ASCII letters anyway, why not start with capital letters? 'A' instead of 'a'.
char c = (char) (r.nextInt(26) + 'A');
if (c != 'I' && c != 'Q') {
randomSuffix += c;
counter++;
}
Next, I don't like having 'I' and 'Q' in that if-statement. Those are basically "magic numbers", and should be replaced with one or more named constants.
Now, we could do something with an unmodifiable set, like this:
private static final Set<Character> INVALID_SUFFIX_CHARS = Set.of('I', 'Q');
while (counter < 3) {
char c = (char) (r.nextInt(26) + 'A');
if (!INVALID_SUFFIX_CHARS.contains(c)) {
...but I think it makes more sense to approach the problem from a "whitelist" perspective rather than a "blacklist" perspective. There are currently 149,186 characters in Unicode, and all but 24 of them are invalid for our purposes.
So, I would build an immutable list of valid characters, and then randomly select 3 from that list:
// I and Q are excluded
private static final List<Character> VALID_SUFFIX_CHARS = List.of(
'A', 'B', 'C', 'D', 'E', 'F', 'G', 'H', 'J', 'K', 'L', 'M',
'N', 'O', 'P', 'R', 'S', 'T', 'U', 'V', 'W', 'X', 'Y', 'Z');
private static String generateRandomSuffix(Random r) {
// Generate a random suffix of length 3
String randomSuffix = "";
for (int i = 0; i < 3; i++) {
char c = VALID_SUFFIX_CHARS.get(r.nextInt(VALID_SUFFIX_CHARS.size()));
randomSuffix += c;
}
return randomSuffix;
}
Exception handling
You have two catch blocks in your code where you catch exceptions:
} catch (Exception e) {
e.printStackTrace();
}
} catch(IOException e) {
}
Catching a generic Exception instead of more specific exceptions is a code smell. So is "swallowing" an exception—catching an exception and then doing nothing with it. There are rare cases when you may want to do these things, but if you can't give an specific explanation for why you are doing them, then you shouldn't.
The problem with the first block in particular—the one tied to reading the file for the uniqueness check—is that the loop continues after the exception is caught. If the exception was thrown by, say, new FileInputStream("plates.txt"), that means counter is still at 3, so the big loop will end and the program will proceed to the write phase. This could result in a duplicate license plate being written to the file, since the write might succeed even if the read failed.
Let's think about what your code is doing.
If you can't write to the file, then the method cannot succeed in its task. If you can't read the file to check the license plate's uniqueness, then it is not safe to write to the file, and so the method cannot succeed in its task.
In the real world, your method would be part of a larger program that would take inputs from a user, or perform tasks on a timer. There would be code "above" this method that would be responsible for calling it. In that case, you would want your method to notify that "higher" code whenever it failed. The "higher" code would be responsible for deciding what to do next: trying again, notifying the user, etc.
So, when your method triggers an exception that blocks it from doing its task, the logical course of action is to have your method throw its own exception to the code "above" it.
This could be done by not catching exceptions at all, and simply letting the code above you deal with it:
public static String numberPlate(String tag, Date date) throws IOException {
Alternatively, you could define a new custom exception class, and wrap all caught exceptions in the new class before re-throwing them:
public static class LicenseGenerationException extends Exception {
private static final long serialVersionUID = 1L;
public LicenseGenerationException(Throwable cause) {
super(cause);
}
}
public static String numberPlate(String tag, Date date) throws LicenseGenerationException {
} catch (IOException e) {
LicenseGenerationException lge = new LicenseGenerationException(e);
// TODO: maybe log the exception before re-throwing it?
throw lge;
}
Performance
First, you should definitely add a break to the subloop that goes through the file to check for uniqueness. As it's currently written, it always checks the candidate string against every line in the file, even if it finds a duplicate on the first line.
I also see that, whenever a string fails the uniqueness check, we close the readers for the file, and have to create new readers and start again for the next candidate. This is rather slow, relatively speaking. (It's still pretty fast from an human point of view.) As an alternative, you could read in the file at the start of the method, and store its contents as a HashSet<String>, if you have enough memory to do that comfortably. It is much faster to check uniqueness against a HashSet. Better yet, you could wait until you've constructed the prefix for your new plate, and then the set would only need to hold the strings from the file that start with that prefix.
But in a practical sense, reading the file twice is not that bad in a little program like this one. As a wise man once said, "premature [performance] optimization is the root of all evil"". You're not working with large amounts of data here. And if you did have that much data, you should definitely be using a proper database, not an unstructured text file.
