Welcome to Code Review and thanks for sharing your code!
General issues
Naming
Finding good names is the hardest part in programming. So always take your time to think carefully of your identifier names.
Naming Conventions
It looks like you already know the
Java Naming Conventions.
Avoid abbreviations
In your code you use some abbreviations such as maxSP
and baseMeleeDmg
.
Although this abbreviation makes sense to you (now) anyone reading your code being not familiar with the problem (like me) has a hard time finding out what this means.
If you do this to save typing work: remember that you way more often read your code than actually typing something. Also for Java you have good IDE support with code completion so that you most likely type a long identifier only once and later on select it from the IDEs code completion proposals.
The other identifiers make perfect sense to me. [because of context] – AJD
Context looks like your friend but in fact it is your enemy.
There are two reasons:
Context depends on knowlegde and experience. But different persons have different knowlegde and experience, so for one it might be easy to remember the context and for another it might be hard. Also your own knowledge and experiences change over time so you might find it hard to remember the context of any given code snipped you wrote when you come back to it in 3 in years or even 3 moth.
The point is that in any case you need (more or less) time to bring the context back to you brain.
But the only thing that you usually don't have when you do programming as a business ist time. So anything that makes you faster in understanding the code is a money worth benefit. And not needing to remember any context is such a time saver.
You may argue that we have a very simple promblem with an easy and common context. Thats true.
But:
Real life projects usually have higher complexity and less eays to remember contexts. The point here is:
At which point is your context so complext that you switch from "acronym naming" to "verbose nameing"?
Again this point changes with you knowledge and your experience wich may lead to code that others have a hard time to understand.
The much better way to deal with it is to always white your code in a way that the dumbest person you know may be able to understand it. And this includes not to use akronyms in your identifiers that might need context to understand.
This is a training project. When you train a physical skill like Highjumping you start with a very low bar that you can easily pass even not using the flop technique just to have a safe environment.
Same is here: The problem may be simple enough to be understood having the acronymed identifiers, but for the sake of training you should avoid acronyms.
Avoid misleading naming
Both of your method names are misleading: They claim to create and build something but in reality neither one is creating or building anything.
One method is doing user interaction and the other is configuring the object.
The names of the methods should reflect that.
Add units to identifiers for physical values
Physical values do mean nothing without a unit. This is a special case of the context problem mentioned above. The most famous example is the fail of the two space missions Mars Climate Orbiter and Mars Polar Lander. The Flight control software was build by NASA and worked with metric measurement (i.e. meters
, meters per second
or newton seconds
) while the engine (and their driver software) has been build by Lockheed Martin which use imperial units (i.e. feet
, feet per minute
or Pound-force second
).
The point is: not having units of physical values in your identifiers forces you to think if there is a problem or not:
double acceleration = flightManagement.calculateAcceleration();
engine.accelerate(acceleration);
But usually you don't quesion it unless you have a reason...
Having the units in the identifier names the problem becomes obvious:
double meterPerSquareSeconds =
flightManagement.calculateAccelerationInMeterPerSquareSecond();
engine.accelerateByFeedPerSquareMinute(
meterPerSquareSeconds); // oops
And we have the same argument again: your particular code is so easy and so small that we don't need the overhead.
But then: How do you decide at which point the overhead is needed? And again it depends on knowledge and experience which still are different among people...
Flawed implementation
Both methods using the same member variables.
E.g. you variable maxSP
: in createCustomClass()
you assign it the result of kb.nextInt()
. Then you pass this value as parameter to buildCustomClass()
where you again assign the parameter value again to the same member variable.
Beside this being useless it may lead to confusing bugs later.
keep same level of abstraction
Methods should either do "primitive" opeerations or call other methods, not both at the same time.
At the end of your method createCustomClass()
you call the other method (buildCustomClass()
). The better way to do this is to extract the code before the call to buildCustomClass()
in a separate (private) method:
public void createCustomClass() {
aquireDataFromUser();
buildCustomClass(
maxHP,
maxMP,
maxSP,
baseMeleeDmg,
baseSpellDmg,
baseAC,
baseSpeed);
}
private void aquireDataFromUser() {
kb = new Scanner(System.in);
System.out.println("Enter the information for your class: ");
System.out.println("Enter HP: ");
maxHP = kb.nextInt();
System.out.println("Enter MP: ");
maxMP = kb.nextInt();
System.out.println("Enter SP: ");
maxSP = kb.nextInt();
System.out.println("Enter Base Melee Damage: ");
baseMeleeDmg = kb.nextInt();
System.out.println("Enter Base Spell Damage: ");
baseSpellDmg = kb.nextInt();
System.out.println("Enter AC: ");
baseAC = kb.nextInt();
System.out.println("Enter Speed: ");
baseSpeed = kb.nextInt();
}
Beside making createCustomClass()
shorter it makes the useless reassingment of the member variables obvious.