• Post Reply Bookmark Topic Watch Topic
  • New Topic

Regarding Incrementing  RSS feed

 
Anakela Bella
Ranch Hand
Posts: 33
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Hello, everyone. I'm VERY new here and to Java, and was hoping I could get some help from anyone who is able to lend a hand. I am currently take a class, and trying to increment the count of employees I enter into an ArrayList by 1 for every entry. However, instead of incrementing by 1 each time, the current number of employeeIDs adds itself to itself, then adds 1, giving a non-sequential number (i.e. an employeeID of 75 will add itself to 75 + 1 to get a new employeeID of 151). I have been up and down my code, but have not found a solution for about two weeks. Any help is truly appreciated!

The following code is what I have done in terms of incrementing. (More coding has been done, but it is not used to increment, so I have omitted it. I'm not looking for a solution, necessarily. Just for someone to point me in the correct direction.)
 
Ulf Dittmer
Rancher
Posts: 42972
73
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
newEmployeeID should not be declared static.

I don't understand the whole logic of employeeID and newEmployeeID, though. Can't you iterate over all Employee objects and do something like "employee.setEmployeeID(employee.getEmployeeID() + 1)" ?

Automatically changing something in the constructor is probably not a good approach, as you will likely want to use the constructor many times without it changing anything.
 
Mike. J. Thompson
Bartender
Posts: 689
17
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Welcome to the Ranch.

As far as I can see, there is nothing in the code you posted that will cause the behaviour you describe.

However you have a setEmployeeId(int) method, so any code that has a reference to an Employee will be able to alter the id to any value it wants. Do you have any code that calls that method?

Setter methods are usually a bad thing in my opinion. They break encapsulation and stop a class being immutable.
 
Mike. J. Thompson
Bartender
Posts: 689
17
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
And also newEmployeeId is not private, so something outside the Employee class could potentially be changing its value.
 
Ulf Dittmer
Rancher
Posts: 42972
73
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Setter methods are usually a bad thing in my opinion.

Good point. A method like "void incrementEmployeeID()" would be better.
 
Campbell Ritchie
Marshal
Posts: 56584
172
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Ulf Dittmer wrote:newEmployeeID should not be declared static. . . .
Why not? That looks perfectly all right to me.
 
Campbell Ritchie
Marshal
Posts: 56584
172
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Mike. J. Thompson wrote:Welcome to the Ranch.
. . .
Setter methods are usually a bad thing in my opinion. . . .
Agree to both parts . I think the ID field should be final, so every employee has an ID set up when the class is instantiated, and it never changes. I would suggest that you should have no methods which allow the ID to be changed. You may change name (most commonly when an employee marries) but you would expect an employee to have the same ID for ever.Note:-
  • 1: Because ID is final, you are not allowed any methods which set or alter it
  • 2: No no‑arguments constructor. This requires you to know the name before an object can be created.
  • The limits of the int datatype mean you cannot have more than 2147483647 employees
     
    Campbell Ritchie
    Marshal
    Posts: 56584
    172
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    Minor mistake in my last post (sorry): ID should not be in capital letters because it is not a constant.
     
    Ulf Dittmer
    Rancher
    Posts: 42972
    73
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    Campbell Ritchie wrote:
    Ulf Dittmer wrote:newEmployeeID should not be declared static. . . .
    Why not? That looks perfectly all right to me.

    I suspect the constructors are called twice for each employee - which would make it the source of the problem. While we haven't seen enough of the code to be sure, it could explain the discrepancy. Using constructors for changing the ID is dangerous, because care needs to be taken that it happens exactly once.
     
    Campbell Ritchie
    Marshal
    Posts: 56584
    172
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    Even if the no‑arguments constructor is used, in the code we have been shown, there is only one newEmployeeID++ call. Assuming the OP is correct in saying there is no other incrementation, that code will produce consecutive ID numbers for Employee instances.
    I agree the no‑arguments constructor should be deleted. I agree with getting rid of the setID methods and similar. I also don't like the use of NumberFormat: what is wrong with String#format or String#valueOf?
     
    Campbell Ritchie
    Marshal
    Posts: 56584
    172
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    If, however, there is other incrementation which we haven't been shown, or any other manipulation of newEmployeeID, we would not know about it here, and that might be the cause of the problems. It is worth going through the code you didn't show with ctrl‑F‑newEmployeeID and checking that there really is no other incrementation.
     
    Ulf Dittmer
    Rancher
    Posts: 42972
    73
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    Campbell Ritchie wrote:Even if the no‑arguments constructor is used, in the code we have been shown, there is only one newEmployeeID++ call. Assuming the OP is correct in saying there is no other incrementation, that code will produce consecutive ID numbers for Employee instances.

    I think you're misinterpreting what Anakela means by "non-sequential". As I read it, it does not mean that new IDs are not sequential, but rather that they are not "old ID + 1". Instead, they are "old ID +1 + someConstant", where in the example "someConstant = 75". I thought that was fairly obvious from the first post, but maybe it's not.

    So I continue to suspect that the constructor is called multiple times, and thus newEmployeeID being static is a big part of the problem.

    Of course, all this could be totally wrong since we haven't seen important parts of the code. Let's hope Anakela will post some clarifications soon.
     
    Anakela Bella
    Ranch Hand
    Posts: 33
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    Ulf Dittmer wrote:Of course, all this could be totally wrong since we haven't seen important parts of the code. Let's hope Anakela will post some clarifications soon.

    What other parts of the code would you like me to post? I do have writer and reader interfaces that are implemented in order to write the information to an ArrayList. Perhaps this is contributing to the problem? As I said in my initial post, I am VERY new to Java, and do not completely understand every element of code that is used to do this.
     
    Mike. J. Thompson
    Bartender
    Posts: 689
    17
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    I think we need to see all code that uses an Employee reference. You are either creating a lot of Employees that get thrown away (so it looks like the Employee ID is not sequential as you no longer have the Employees that had that ID), or something somewhere is altering the ID after the Employee is created.
     
    Ulf Dittmer
    Rancher
    Posts: 42972
    73
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    All code parts where you're calling the constructor - that's where the newEmployeeID is set, which as you said is what is not working.
     
    Campbell Ritchie
    Marshal
    Posts: 56584
    172
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    It might be worthwhile putting some debugging code into the Employee constructorThat will show whether you are calling the constructor many times and not using the instance. If that is the case, it is likely that the Employee class is correct and the error is in how it is used (as Ulf said earlier).

    There is another, naughty, way to find all the constructor calls: to follow in next post.
     
    Campbell Ritchie
    Marshal
    Posts: 56584
    172
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    Naughty way to find all calls to the Employee constructor.
  • 1: Go through the Employee class and make sure new Employee(...) does not appear anywhere.
  • 2: Change access for all Employee constructors to private.
  • 3: Compile all your other classes and count the errors where the compiler says Employee constructor has private access.

  • You need to be sure that you are not saying new Employee(...) anywhere and forgetting about it, which is what Ulf thinks is the cause of your problem.
    When you have sorted that out, you can take the private modifier off the Employee constructor.
     
    Campbell Ritchie
    Marshal
    Posts: 56584
    172
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    You may find you are implicitly calling new Employee(...) because you might have a subclassEvery call to create a SalariedEmployee instance also calls the Employee constructor. If you give constructors private access, that subclass will produce a compiler error similar to what I mentioned earlier.
     
    Campbell Ritchie
    Marshal
    Posts: 56584
    172
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    A few moments ago, I wrote:. . . . . .
    That would be correct code. This, however, is incorrect.That would cause double increments. But it can be prevented if you use good object‑oriented design and data hiding and make newEmployeeID private in Employee.
     
    Anakela Bella
    Ranch Hand
    Posts: 33
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    Campbell Ritchie wrote:It might be worthwhile putting some debugging code into the Employee constructorThat will show whether you are calling the constructor many times and not using the instance. If that is the case, it is likely that the Employee class is correct and the error is in how it is used (as Ulf said earlier).

    There is another, naughty, way to find all the constructor calls: to follow in next post.


    Okay, so I added the debugging statement, and this is what the output ended up being:

    Does this help to clarify how the application is jumping? I have been utilizing reader and writer interfaces to write the new employees to a text file, but I'm wondering if this is causing the jumping problem. The code is as follows:

    Thanks again!
     
    Ulf Dittmer
    Rancher
    Posts: 42972
    73
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    So it is indeed exactly as I suspected: the constructor is called multiple times during the run of the application, thus causing newEmployeeID to go up by the current number of employees every time you call getEmployees. A "get" operation should not alter data. newEmployeeID should only go up when you actually create a new employee, not when you read them from disk or at any other time.

    Why do you even need that field? A new employee should probably get "highest ID given so far + 1" - which you can determine by iterating once over the list.
     
    Anakela Bella
    Ranch Hand
    Posts: 33
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    Thanks for your help! So I've been calling the constructor (public Employee(String firstName, String lastName){// stuff in here}) multiple times when executing the program? Also, when you ask me if I need that field, are you talking about the newEmployeeID field?
     
    Ulf Dittmer
    Rancher
    Posts: 42972
    73
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    So I've been calling the constructor (public Employee(String firstName, String lastName){// stuff in here}) multiple times when executing the program?

    Yes, in the getEmployees method.

    when you ask me if I need that field, are you talking about the newEmployeeID field?

    Yes.
     
    Mike. J. Thompson
    Bartender
    Posts: 689
    17
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    You need to change your code to do the following things*:

    1) store the ID in the file. When you read the Employee from the file it should get the same ID again.

    2) when you create a new ID (asking the user to enter the details) it should give the Employee the next highest available ID, as Ulf has already suggested.

    * I haven't seen your requirements so this is just my interpretation of the best way to solve your problem. If your teacher told you to use the static field then (s)he is going to expect it to be there.
     
    Anakela Bella
    Ranch Hand
    Posts: 33
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    Ok. From the "getEmployees()" method, I've deleted "employees.add(e);" and the result has been interesting. Now, the employee ID counter resets to 0 every time I run the program over again. It still skips a couple of numbers, but not as badly as it did previously. The output is as follows:
     
    Don't get me started about those stupid light bulbs.
    • Post Reply Bookmark Topic Watch Topic
    • New Topic
    Boost this thread!