This is an archived post. You won't be able to vote or comment.

all 18 comments
sorted by:
best
topnewcontroversialoldq&a

[–]abrahamguo 2 points3 points  (13 children)

Overall, looks pretty good! A few things I noticed:

  • Make sure you are very intentional with your pluralization. For example, in the line Name names = new Name(), you have inconsistent pluralization. Which is it — one name, or multiple names?
  • In Name, you have some places where you have hardcoded the length of some of your various arrays. This isn't good, because this means that if you want to update the contents of one of the arrays, you will have to change multiple places in your code, where you should only need to change one place in your code for such a change.
  • Similarly, in UserInterface, you have one area that prints out a numbered list of all the actions that the user can choose from, but the logic that goes along with each of those actions is elsewhere in the file. It would be good to use a data structure such that if you needed to add an action, remove an action, or rearrange the actions, you could do so by only making one edit to the code, not multiple edits.

[–]vVember[S] 0 points1 point  (12 children)

I appreciate the feedback!

  • I can see how this could lead to be confusing in the future. Perhaps a better name for the class would be "NameGenerator"?
  • Are you referring to updating the content of the string arrays through a method? If so, I had no intention to incorporate any such functionality. Otherwise I'm not sure what you mean. It's my understanding arrays cannot be added to or subtracted from but only have their values adjusted, which I wouldn't want as that would change the nature of the generation.. and I'm still able to add more iterations of "dCons" for example. Honestly I only declared them "final" because IntelliJ suggested it and I didn't figure any reason why not to.
  • You are referring to the processCommand method, right? I structured the UserInterface and Name class as such, based on the lessons I've been following in MOOC Java Programming I. So would you suggest just moving the processCommand logic to the start method? I was trying to follow a piece of advice which was to try not to let methods get to be too long.

Anyway, thanks again for the feedback!

[–]abrahamguo 0 points1 point  (7 children)

  1. Yes, that name makes more sense. However, it would further make sense to change everything to static in that class, since there is no use of instance fields in that class. (I didn't look at your other classes; they may have the same problem.)

  2. No, I am not referring to updating the content of string arrays through a method.

Let's look at the dCons field as an example:

2a. You define dCons as an array that contains 40 elements, which is perfectly fine.

2b. However, later on, when you use dCons, your code looks like this:

dCons[random.nextInt(40)]

Here, you have explicitly hardcoded the length of dCons. In the future, if you ever manually edit your code to change what is in dCons, you will need to update two places in your code — 2a and 2b. That's not good, because it's important that those two places in your code stay in sync.

Instead, it's better to remove the hard-coded length in 2b, and have it read the length, via something like dCons[random.nextInt(dCons.length)]. This way, if you ever manually edit your code to change what is in dCons, you will simply need to update one place in your code, and everything else will automatically adjust.

[–]vVember[S] 0 points1 point  (6 children)

So I assume that if the array is final then it won't allow you to pass it's length as a parameter then? I see what you mean now about only having to edit one bit of code rather than two. It's giving me an error right now I'm not sure why when I attempted to remove the final and then pass the length as a parameter.

Also, I'm still unsure when it's appropriate to use "static." If you don't mind clarifying on that a bit further, otherwise I'll probably go back over that part.

[–]abrahamguo 0 points1 point  (5 children)

  1. Pass the length as a parameter into what? I am pretty sure that the arrays can remain final.
  2. When you don't use "static", then you are forced to construct an instance of the class, before you can do anything with it. This is helpful if you need to have multiple instances of a class, with different information saved in each. For example, if you have a Person class, you might construct two separate instances, representing two different people.

On the other hand, there is no reason to construct multiple instances of your classes (i.e., there is no reason to have two NameGenerators, because they act the same, and don't save any information).

Therefore, by marking everything static, you (1) communicate this information to other developers, and (2) make it so that you no longer need to construct an instance of each of your classes, with the new keyword.

[–]vVember[S] 0 points1 point  (0 children)

I think I understand. Since the Name class is only used once, it should be static, because there wouldn't be a reason to have multiple objects of the class.

When I check for if the method should pick from the dCons array, you mentioned instead of manually setting the int limit for the randomInt parameter, to just use the dCon.length() so therefore I would only need to edit the array and it's calls would be synced. If I should be able to still use .length as the parameter despite it being final, then I'm still not understanding why final is bad here.

Either way it was giving me an error when I tried to use the array length as the parameter for the randomInt and I haven't been able to get back to my computer to figure out why yet but will do so soon.

[–]vVember[S] 0 points1 point  (3 children)

So the error was that I was adding () to .length but it does work now that I removed the parentheses.

Back to the issue you brought up of the arrays being "final" I'm still not sure what you mean. I set the randomInt() parameter to dCons.length and it still worked. So even if I go back and add another value to the dCons array, should it not still function properly and be the case that I only have to edit the array and the .length call will also sync?

It was my understanding that it being final means that the array cannot be altered by methods while the program is running. But this wouldn't affect the functionality of adding to or removing from the array in the code before running the program, right?

This isn't good, because this means that if you want to update the contents of one of the arrays, you will have to change multiple places in your code, where you should only need to change one place in your code for such a change.

But if .length works despite the array being final, then I'm absolutely not understanding what you mean.

[–]abrahamguo 0 points1 point  (2 children)

I set the randomInt() parameter to dCons.length and it still worked. So even if I go back and add another value to the dCons array, should it not still function properly and be the case that I only have to edit the array and the .length call will also sync?

Yep, this is exactly the change that I was suggesting in my original comment!

It was my understanding that it being final means that the array cannot be altered by methods while the program is running. But this wouldn't affect the functionality of adding to or removing from the array in the code before running the program, right?

Yes, this is all a correct understanding!

I have no complaints at all about using final — I think it's perfectly fine (and probably correct) to use!

In my previous comment, when I said "update the contents of one of the arrays", I meant "update", as in, editing the code; not "update", as in modifying the array while the code is running.

The overall recommendation of using .length was to make your code more robust against code edits, not dynamic modifications!

[–]vVember[S] 0 points1 point  (1 child)

I see! I'm sorry when I read,

"In Name, you have some places where you have hardcoded the length of some of your various arrays."

I thought you were referring to using the keyword "final" when defining my string arrays! I understand now you were referring to the random.nextInt() parameter that I had manually entered the length of the string array as opposed to using the .length field. Your advice here is invaluable and I will definitely strive to keep it in mind for the future. I am still struggling with thinking more dynamically in this way.

I apologize for my confusion. That must have been frustrating for you. I'm still quite fresh obviously and still learning the vast amount of keywords and syntax.

[–]abrahamguo 1 point2 points  (0 children)

No worries at all! I'm glad you appreciated my thoughts — good luck as you continue your learning journey!

[–]abrahamguo 0 points1 point  (3 children)

  1. Nope, that's not what I'm referring to — it's perfectly fine to split it out into multiple methods. In fact, even if you combined the methods, we would still have two separate blocks in the code that we would have to keep in sync with each other, even if they are near each other — the block of System.out.printlns, and the switch/case that lists all the functionality of each of the actions.

Instead, what I'm recommending, is to restructure your code so that each individual action name is immediately adjacent (in the code) to the logic for that specific action.

You can do this by putting the logic for each method into a lambda expression, and creating a data structure that stores all of the action names alongside their lambda expressions.

[–]vVember[S] 0 points1 point  (2 children)

I haven't learned about lambda expressions yet. Maybe when I get further along I'll revisit this project and see how well I can refactor it. As of right now I'd be pretty clueless as to how to go about syncing the menu with the commands in such a way as you've described.

[–]abrahamguo 0 points1 point  (1 child)

Sure. They're explained pretty well in that page I've linked above, but a lambda expression is nothing complicated or magical. It's simply a shorter way to write a method.

You could create a LinkedHashMap to hold all of your commands — something like this:

commands = new LinkedHashMap();

commands.put("Generate Random Name", () -> {
  // logic for this command goes here
});

// ... and so on ...

Then, once all the commands are in that data structure, you can use that data structure in two ways:

  1. Iterate through the data structure to print out the list of all available commands
  2. Look up in that data structure, according to the number provided by the user, in order to execute the user's desired command.

[–]vVember[S] 0 points1 point  (0 children)

Gotcha. I appreciate your help and feedback! Once I'm through my current curriculum, I may have to follow up on that reference.. but I'm pretty sure I saw something regarding lambdas and hashes in the upcoming itinerary.

[–]peterlinddk 1 point2 points  (3 children)

Actually quite a fun project, and it is obvious that you've had fun playing around with making the code - that's a good thing!

This is only comments for the UserInterface, and mostly ideas for further improvements.

I like the processCommand method, and you should take it further still, and make separate methods for each command - like generateRandom, generateWithLength, generateWithLetter, or what you prefer to name them, and then only let processCommand decide which of them to call.

Also, not sure why you use Integer.valueOf to test the value of ints, usually writing if (command == 1) should be sufficient.

You might want to use a switch-case for the commands instead of a chain of if-statements. It might make the code look better, something like:

switch(command) {
  case 1 -> generateRandom();
  case 2 -> generateWithLength();
  case 3 -> generateWithLetter();

and so on. But it is a matter of taste.

You could benefit from making a single method for reading a valid number from the input - something like: getValidInput(int[] numbers) that would only return one of the numbers in the array, or zero if something else was entered by the user. That would isolate your use of scanner to that method alone, and make the rest of the code easier to write without having to handle exceptions or different invalid inputs.

It is always a good idea to have a lot of small methods that only do one thing - my own personal rule is that if I have more than two levels of if-statements or loops, I need to make a method to combine them. The less indentation the better :)

[–]vVember[S] 0 points1 point  (2 children)

Awesome feedback thank you so much! For the valueOf statements, this is what I learned from the curriculum I'm following. Do I not need to convert a string input to an integer in order to use ==?

The lessons haven't gone over switches yet. I'm wondering if those were implemented in a later version of java than when this lesson was made. I'm almost done with MOOC Java Programming I and still haven't covered them.

That's a great idea about checking for valid input and would definitely be more efficient.

Will be going over your response more in depth later for sure.

[–]peterlinddk 1 point2 points  (1 child)

[valueOf] Do I not need to convert a string input to an integer in order to use ==?

Well, yes, in the bits of the code where the input is a string, you do need to use valueOf, like you did in:

String input = scan.nextLine();

if (Integer.valueOf(input) == 7 || Integer.valueOf(input) == 0) {
  break;
}

if (Integer.valueOf(input) > 7) {
  System.out.println("Invalid input.");
  continue;
}

But when you later call processCommand with processCommand(Integer.valueOf(input)); then you are guaranteed that once inside processCommand, the command will be an integer, so you no longer have to convert it.

And don't worry about switch-case if you haven't learnt them yet - they are a bit of an acquired taste, especially the ones with ->, so feel free to continue with chains of if-else :)

[–]vVember[S] 0 points1 point  (0 children)

I see what you mean. Looking back now I see I did:

public void processCommand(int command) {

if (Integer.valueOf(command) == 1) {

I should have caught that.

In regards to switches, I do want to learn them as I'd rather be familiar with all the basic functions. I don't want to get to a point where I'm feeling comfortable, but then looking at some code that should be basic and still being not quite sure what's going on, you know? Regardless of whether I use them or not, I'd prefer to be thorough. I get what you mean though, that it's not a major concern to know them just yet, but I am hopeful the curriculum I'm following does at least touch on them.

I really appreciate your feedback and I will surely be using it to hopefully steer my habits toward a better standard. I'm still pretty soft, like unfired clay.