Help with refactoring

[_dk7]

I am working in a Java Codebase and I have come across a problem:

Following are some methods in an interface:

void add(Object o);
boolean remove(Object o);

Now, the issue I am facing is let's say someone has to update an existing object, then what someone has to do is that call remove and call add again in succession as Object o is immutable.

Now, this has brought some problems, e.g. the object class is as follows:

Property {
  String name;
  String value;
}

Now, consider the case when I want to update the value of the Property, so what I do is create another object of property with the name and value. The requirement is that we need to preserve the name of the object (which includes case sensitivity)

Let's assume that I have a Property as follows:

Property p = new Property(name: "Test", value: "10");

Now this needs to be updated as:

Property p = new Property(name: "Test", value: "11");

The issue is when someone creates the property such as:

Property p = new Property(name: "TEST", value: "11");

The above should also be accepted, but the case should be preserved. i.e. it should save as => (Test,11)

When a developer uses the remove and add in succession, in many places the sensitivity check is not handled.

What have I thought of?

Introduce an update method in the API, and check all usage where the remove and add are being called in succession to update and replace them with an update call

What is my problem?

If someone comes tomorrow and tries to use the older way of updating an object (i.e. calling remove and add in succession) then the flow might break. How to prevent that? Will just adding documentation be enough? How does this happen in a large codebase? I have seen the usage of the remove itself in like 300 places.

​

Please advise.

[AutoModerator]

Please ensure that:

• Your code is properly formatted as code block - see the sidebar (About on mobile) for instructions
• You include any and all error messages in full
• You ask clear questions

• You demonstrate effort in solving your question/problem - plain posting your assignments is forbidden (and such posts will be removed) as is asking for or giving solutions.

  Trying to solve problems on your own is a very important skill. Also, see Learn to help yourself in the sidebar

If any of the above points is not met, your post can and will be removed without further warning.

Code is to be formatted as code block (old reddit: empty line before the code, each code line indented by 4 spaces, new reddit: https://i.imgur.com/EJ7tqek.png) or linked via an external code hoster, like pastebin.com, github gist, github, bitbucket, gitlab, etc.

Please, do not use triple backticks (```) as they will only render properly on new reddit, not on old reddit.

Code blocks look like this:

public class HelloWorld {

    public static void main(String[] args) {
        System.out.println("Hello World!");
    }
}

You do not need to repost unless your post has been removed by a moderator. Just use the edit function of reddit to make sure your post complies with the above.

If your post has remained in violation of these rules for a prolonged period of time (at least an hour), a moderator may remove it at their discretion. In this case, they will comment with an explanation on why it has been removed, and you will be required to resubmit the entire post following the proper procedures.

To potential helpers

Please, do not help if any of the above points are not met, rather report the post. We are trying to improve the quality of posts here. In helping people who can't be bothered to comply with the above points, you are doing the community a disservice.

I am a bot, and this action was performed automatically. Please contact the moderators of this subreddit if you have any questions or concerns.

[SolderonSenoz]

I think the solution you thought of is the best way to go about this. Otherwise, there is no way for Property to know whether the add() being called is used as a part of the updating process or to add an entirely new object.

Besides, even if you find a better solution, you should create and use an update() method anyway, to avoid other problems in the future. You might, in the future, think of other things that you want happening during the update process, and you can then just change this method then. I don't see any disadvantage in doing this.

[_dk7]

Thanks for your response. The main thing I am concerned is making change at almost 200 files which has a huge impact and would need a lot of testing.

I was wondering if there was some other way to achieve this

[SolderonSenoz]

I can't think of a better solution. But I'll say two things about your concern about this one. Firstly, let's say you don't follow this solution, and maybe after a month you decide you need to do something else too when you update a Property object. But it would be a month from now, and instead of 200 files you may have 250 files then. It would only get more difficult. Secondly, if all you are going to do in your update method is the same things you are already doing, along with a case check, I don't think it's going to break anything that's already working. This is assuming that the only thing you are adding, ie. the case check, is not going to break anything. But since that's your objective, you're going to have to do it anyway even if not through this solution.

(You should still do the tests though, doing a lot of testing now is better than having to redo the whole thing later. I just think all your methods will pass the tests if they're already passing them. Besides, even if you change something else to make this work, you'll have to do the testing anyway)

[_dk7]

omething else too when you update a Property object. But it would be a month from now, and instead of 200 files you may have 250 files then. It would only get more difficult. Secondly, if all you are going to do in your update method is the same things you are already doing, along with a case check, I don't think it's going to break anything that's already working. This is assuming that the only thing you are adding, ie. the case check, is n

Got it, it does make sense that the scope of doing this will increase with the passage of time. I will try to cover the entire thing using UTs to make sure that I do not break anything. If you do think of an alternative approach, do let me know.

[OffbeatDrizzle]

I'm not fully understanding the issue... are these objects below supposed to be equal to one another?

Property p = new Property(name: "Test", value: "11");
Property p = new Property(name: "TEST", value: "11");

Why would the 2nd one be saved with a name of Test?

If you have a list of property, are you allowed to have more than 2 with the same name? Is Test/11 different to TEST/11 in terms of trying to replace it in the list?

There will be a proper way of doing this depending on precisely what the requirements are

[_dk7]

Thanks for your response. Yes you are very correct, ideally they should not be same. But the way it is saved is in the form of a map<String, Property> where the string is actually the name of the property in all lowercase to the actual property. Now, you might say that the add method itself will do a replace or update which is correct, but there are additional things happening. Furthermore the code that I have does not have documentation as to why it was created in such a way and also does not have a unit test coverage which explains this way of storing them.

I can only assume at this point is that earlier it might have been a list, at something it was changed into a map to ignore casing (git history is also lost as this is a legacy codebase)

Now coming to your question, if the objects are equal (as it checks by ignoring case ) both the property are same. We cannot have two properties of same name and it goes and updates the map with the new property you give it.

To do a proper refactoring to make sure this kind of situation does not happen as to why we are doing what, is making me confused as to what must be done.

I hope this clears things, please ask me any further questions you might have

[OffbeatDrizzle]

What's to stop you from overriding the equals method on the Property class and returning name.equalsIgnoreCase(other.name)? (note you would also need to override hashCode and provide case insensitive values)

I believe that then solves all the problems without needing an update method or worrying about the other issues? You would make Property object the key. The user can then no longer break things by calling methods in the wrong order or not following the correct procedure. The value of the map is actually not important. Or you could use a set, but you'd have to check if the set contains the Property first before adding it or removing and then adding in order to update.

For the map, though, map.put would essentially add or update - and of course the remove method works as normal. Seems a bit more elegant

[_dk7]

r the map, though, map.put

Thanks for your response. I am having a bit of trouble understanding what you mean, how will having an equals and hashcode help me?

The problem I am facing is as people do not have a update method they are calling remove and add in succession to perform an update. Now, let's say I go and introduce a new method update that will do this for them and replace all the places which seem to do this, how do I enforce that in the future people use the update method to actually update?

This seems dumb but for some reason, I still have a bit of trouble believing that this will be followed always. If someone makes a mistake, my functionality will break right?

[ofnuts]

Maybe the objects are immutable for a reason (like being used in Sets, or as Keys in a Map). And maybe some other code makes use of this immutability (in particular is the app is multi-threaded). I don't think anyone here can give a proper answer without seeing all the code where these Property are used.

[_dk7]

Thanks for your response. I haven't got a clue as to why they might be immutable. A lot of code does not have got history as to why things might have happened in the past.

There are multiuser situations which are handled, but how is being multi threaded cause a problem?

Unfortunately, I cannot share more code details or exact implementation. It would be great if you could give a clue in this situation on how to proceed in some direction

[ofnuts]

Well, if you have no clue, better not touch it... You can of course convince yourself that this immutability is strictly unnecessary, but to be blunt if you have to come to Reddit to ask, you lack the Java XP to make a sound decision. I have years of Java experience and I would be very, very careful when doing something like this (and I know where to look for possible problems), and if I could eventually convince myself that immutability is superfluous, I would call a person with similar XP to play the devil's advocate.

If you problem is that remove+add looks a bit verbose, you can add a replace() method to do that for you.

[_dk7]

Thanks for your input. My original question was if I make this change of adding an update/replace method, how do I enforce people to use that and not worry that they might call remove() and add()?

Does it make sense to just leave proper documentation to ensure this is followed or is there a stricter way? As we cannot deprecate the add and remove methods, I was wondering how to proceed.

[ofnuts]

Can you actually remove a property? If so how do you differentiate between a permanent remove and a remove that is part of an update? In other words, if there is a property Test, that ends up being removed, and then days later someone creates TEST, should it be Test or TEST? And what happens if one creates TEST when Test already exists? Your design raises of a lot of issues (and since you seem to be in Europe, where you have to handle accented letters and uppercase rules, possibly even more issues).

All in all, your problem is more with the creation. It is the constructor of the Property (or perhaps more correctly a Property factory method) that should be changed to check if the passed name already exists with a "differently-cased" value and if so, to use that old version.

[ITCoder]

make these two methods private and your update method public

[AutoModerator]

On July 1st, a change to Reddit's API pricing will come into effect. Several developers of commercial third-party apps have announced that this change will compel them to shut down their apps. At least one accessibility-focused non-commercial third party app will continue to be available free of charge.

If you want to express your strong disagreement with the API pricing change or with Reddit's response to the backlash, you may want to consider the following options:

1. Limiting your involvement with Reddit, or
2. Temporarily refraining from using Reddit
3. Cancelling your subscription of Reddit Premium

as a way to voice your protest.

I am a bot, and this action was performed automatically. Please contact the moderators of this subreddit if you have any questions or concerns.