1
\$\begingroup\$

To help with learning code in my class, I've been working on this text based game to keep myself coding (almost) every day. I have a class called BasicUnit, and in it I have methods to create a custom class. I use 2 methods for this, allowing the user to enter the information for the class. I'm just wondering if I can do this in a more simplified manner?

    public void buildCustomClass(int maxHP, int maxMP, int maxSP, int baseMeleeDmg, int baseSpellDmg, int baseAC, int baseSpeed) {

    this.maxHP = maxHP;
    this.maxMP = maxMP;
    this.maxSP = maxSP;
    this.baseMeleeDmg = baseMeleeDmg;
    this.baseSpellDmg = baseSpellDmg;
    this.baseAC = baseAC;
    this.baseSpeed = baseSpeed;
    lvl = 1;
    xp = 0;
    curHP = maxHP;
    curMP = maxMP;
    curSP = maxSP;

}

public void createCustomClass() {
    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();

    buildCustomClass(maxHP, maxMP, maxSP, baseMeleeDmg, baseSpellDmg, baseAC, baseSpeed);
}
\$\endgroup\$
  • 3
    \$\begingroup\$ Could you include the actual class this is in? These 2 methods are used to initialise a certain class but you're not really creating one. Without the surrounding code it's hard to explain the difference. \$\endgroup\$ – Imus Apr 5 '18 at 8:32
  • \$\begingroup\$ This looks like it could be a pretty good spot for the Builder Pattern. But, I would need to see more code to make a recommendation. \$\endgroup\$ – adprocas Apr 5 '18 at 12:44
5
\$\begingroup\$

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:

  1. 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.

  2. 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.

\$\endgroup\$
  • 1
    \$\begingroup\$ @AJD "The other identifiers make perfect sense to me." - If this is your start into a programmers career keep in mind that you will have to collaborate with others. And the identifiers you choose must make "perfect sense" to your futur team mates too. Also: you asked for improvements. Accept them or not. There is no need to explain yourself... \$\endgroup\$ – Timothy Truckle Apr 6 '18 at 7:49
  • 1
    \$\begingroup\$ @AJD So your argument is that cryptic identifier names are OK because of context? Well my experience in the programming business is only 20 years. But I for myself find it easier to work with clear names without acronyms even if they are somewhat lengthly. And please keep in mind: modern languages (like java or even C) have virtually no limit for the length of identifiers, and actual monitors can show more than 80x25 characters, Some progress happened here in the last 40 years... ;o) \$\endgroup\$ – Timothy Truckle Apr 6 '18 at 8:07
  • 1
    \$\begingroup\$ @AJD "his is common parlance in that context" And BTW this was the reason why two mars missions failed (Mars Climate Orbiter and Mars Polar Lander): some teams contributing software had their own context (metric vs imperial units). So yes: I think that identifiers should not have any acronyms and only short living local variables may be very common abbreviations. \$\endgroup\$ – Timothy Truckle Apr 6 '18 at 8:28
  • 1
    \$\begingroup\$ I think the question to ask is "why?" Why would you use abbreviated names? The answer used to be simple. To save space and typing. But, with better monitors and autocomplete, those reasons have mostly gone by the way side. So, it is always good for a developer to be descriptive when naming things in these modern languages, whether or not the context is understood or will only be used by specific people that understand it. It's always good for a developer to code using descriptive naming; building good habits. There are exceptions, like JavaScript (size of files), but that doesn't apply here. \$\endgroup\$ – adprocas Apr 6 '18 at 11:20
  • 1
    \$\begingroup\$ @AJD, you're saying the same stuff you said before. It's also about building good habits and making sure the code is maximally readable by all developers. Abbreviating variable names is frowned upon in order to create good habits. It absolutely should be maxHitPoints. This developer is asking for a code review. Someone that knows the language may not understand the context and not understand what maxHP means in this very question. So, the abbreviation would create confusion when asking for code review on a site like this. This question is an example of why descriptive naming is important. \$\endgroup\$ – adprocas Apr 9 '18 at 15:32

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.