• Post Reply Bookmark Topic Watch Topic
  • New Topic
programming forums Java Mobile Certification Databases Caching Books Engineering Micro Controllers OS Languages Paradigms IDEs Build Tools Frameworks Application Servers Open Source This Site Careers Other Pie Elite all forums
this forum made possible by our volunteer staff, including ...
Marshals:
  • Campbell Ritchie
  • Paul Clapham
  • Tim Cooke
  • Jeanne Boyarsky
  • Liutauras Vilda
Sheriffs:
  • Frank Carver
  • Henry Wong
  • Ron McLeod
Saloon Keepers:
  • Tim Moores
  • Frits Walraven
  • Tim Holloway
  • Stephan van Hulst
  • Carey Brown
Bartenders:
  • Al Hobbs
  • Piet Souris
  • Himai Minh

Can this method be written better?

 
Greenhorn
Posts: 2
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
Hello all,

New here and need help with my method, can it be refined or changed to be more efficient?

 
Sheriff
Posts: 17068
298
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
Quite a few problems with this code.

First, it doesn't follow convention for a method with a name that starts with "set" -- such a method will be expected to take a parameter which is the value that you want to set the named attribute to. As such, these methods are normally declared with a void return type. Also, they are usually public. That is, something like this:


Second, the method sets the value of a local variable. The scope of this action and value is limited to the method itself. That is, nothing outside of this method will know anything even happened with the departmentChoice. This is the reason you have to return it from the method, breaking semantic convention for a setter method.

Third, you are mixing in user interaction. A setter method's purpose is to set an attribute. User interaction should be done separately. That is, interact with user to get the value, then call the setter method to set the attribute value. These are kept separate to facilitate code maintenance, testing, and debugging. Combining the two actions violates a basic design principle: Single Responsibility.
 
Junilu Lacar
Sheriff
Posts: 17068
298
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
And welcome to the Ranch!
 
Marshal
Posts: 76403
364
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
Welcome to the Ranch

Nice to see somebody who knows how to indent her code, and gets her < and > the right way round. You have got you algorithm worked out.
Please always use the code button; I shall edit your post with it, so you can see how much better it looks Unfortunately you have some long lines. I see you know how to split long lines (lines 7‑8) () but you should have split the code more aggressively. Don't split the last line of a do loop. Even though the compiler doesn't complain, don't write an if all on one line; use {}. This is how I think that do loop should be written:-My line 13 is where we sometimes see && instead of || or < and > the wrong way round. But I think I would prefer to have that test done elsewhere. Can you overload the askForInt() method to take maximum and minimum parameters, so the keyboard class prints the bit about, ”Please enter between 0 and 3”? That would make the keyboard class even better than it already is, I think (that sort of class is usually a good idea). It would also move your loop out of this method, and save you writing a similar loop the next time you need input in a particular range.Don't worry about efficiency; you want elegance. What use is efficiency, running in 10μs rather than 20μs when you are taking 10ms to print the instructions on screen, and ≥Ā½second to hit the correct number key?

A normal setXXX() method has void instead of a return type, and takes what you are setting as a parameter. What you have is something different. Have you been told to call it setDepartment()? I would call it something different myself.
I would probably use a slightly different algorithm myself. I am worried about your hard‑coding the department names in that method. That sort of information is probably better off being stored elsewhere. Maybe an array of departments, or a List, because you can make a List unmodifiable (see this method).
I don't like to see \n, which is probably the wrong line end. Try String#format and %n, which latter gives you the correct line end, instead:-I probably isn't a good idea to use Strings for something that isn't text. I would prefer to see a Department class instead. That does have the problem that you would be tempted to implement "none" as null; nulls are bad things. Indeed the method I linked to about Lists won't let you pass null in the first place.
 
Ilona Kot
Greenhorn
Posts: 2
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
All great points, thank you very much, the naming of the method was from a tutorial off Udemy. I was told by one of the developers at work to use a separate class for my input and i changed the program to make it a bit more elaborate, taking in user input instead of setting things in the constructor.
 
Junilu Lacar
Sheriff
Posts: 17068
298
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator

Ilona Kot wrote:the naming of the method was from a tutorial off Udemy


Then I would seriously question the quality of that course. This is such a basic thing in Java and if you are misled in this regard, what other things does it mislead you on? On the other hand, you may have misinterpreted the content. Did the tutorial instruct you to make the method return a String and make it private or was that your own decision?
 
Look ma! I'm selling my stuff!
the value of filler advertising in 2021
https://coderanch.com/t/730886/filler-advertising
reply
    Bookmark Topic Watch Topic
  • New Topic