ALL THE THINGS: Prioritized List of Refactorings

Refactoring 0: Privatize as Many Fields/Methods as Possible

Issue Being Addressed

Many of the methods throughout Refactoring-SDD-2010 are public. These should be made private.

Planned Refactoring

Switch public methods to private, and create get() and set() methods where necessary. These methods should ultimately be replaced or removed as well, but in the short term this is the plan.

Technical Difficulty

This should be a relatively simple refactoring. While it is very likely that setting all public methods to private will initially cause issues, the inclusion of - hopefully temporary - get() and set() methods should remedy this problem.

Impact on the Code

This refactoring will hopefully tell us more about the locations of methods, and whether they are placed appropriately. In the situation that they are not, we can then move methods to the appropriate classes.

Refactoring 1: Make Polygon.java More Readable

Issue Being Addressed

There is a lot of terrible math in here. We might be able to make it more readable, but we should be careful that we don't make it more confusing.

Planned Refactoring

We currently do not have a solid plan for this refactoring. We now that the logic is horrible and confusing, and hope to remedy this, but we are not yet sure of the appropriate way of going about this. May possibly involve the creation of new methods.

Technical Difficulty

This refactoring could be difficult. There is a lot of potential to break functionality here, and so we will need to proceed with caution, testing and committing any progress frequently.

Impact on the Code

If this refactoring is successful, the results should significantly improve the readability of Polygon.java.

Refactoring 2: Some Potentially Misplaced Methods in View.

Issue Being Addressed

Some of the methods in the view folder are potentially inappropriately placed. Example: the lightColorScheme() method in PointsPanel might be better in PointsTable.

Planned Refactoring

Move problem methods to the appropriate classes.

Technical Difficulty

One potential problem here is that we are not very familiar with this code, and do not know how dependent these classes/methods are on one another. But we will have to try it before we know the extent.

Impact on the Code

This will hopefully improve readability of the code, and allow us to remove some of the get() and set() methods - both those already in place and those that will be created after privatizing the currently public methods - which are a potential security issue.

Refactoring 3: There is an odd method, getInstance(), in PolygonAreaLoader. Can we make it better, or less insane-looking?

Issue Being Addressed

The getInstance() method in PolygonAreaLoader gets the constructor for PolygonModel, and then uses this to return a new instance of the constructor. This seems like a phenomenally BAD IDEA.

Planned Refactoring

Hopefully, we can just create a new instance of PolygonModel here, rather than getting the constructor.

Technical Difficulty

While this seems relatively straightforward, this is such an odd coding choice that it seems inevitable that there will be some sort of issue that crops up. However, until we attempt this refactoring, it will be difficult to tell with an certainty.

Impact on the Code

This refactoring should make the PolygonAreaLoader class more readable.

Refactoring 4: Move the generateRandomPoint() method from PolygonModel.java to Point.java and change it to just a return statement.

Issue Being Addressed

The generateRandomPoint() method does not belong in the PolygonModel class.

Planned Refactoring

Move the generateRandomPoint() method from the PolygonModel class to the Point class.

Technically Difficulty

This refactoring should not be difficult. Some of the calls to it may need to be changed, but again, this should not be a difficult task.

Impact on the Code

This will likely not have a great impact on the code; however, in moving generateRandomPoint() to the appropriate class, the code should hopefully be more readable.

Refactoring 5: Possible Redundant Method: PolygonModel.isInside(), Compare to Polygon.contains()

Issue Being Addressed

There is redundant logic in the isInside() method from PolygonModel and the contains() method from Polygon. This logic could potentially be combined.

Planned Refactoring

Combine logic from isInside() and contains() methods to reduce redundancy and the need for shotgun surgery.

Technical Difficulty

This refactoring should not be terribly difficult. There may need to be some adjustments to the code in order for the functionality to prove valid for all applications, but this is to be expected.

Impact on the Code

This refactoring will reduce redundancy in the code, thereby also reducing the need for shotgun surgery in the case of additional functionality. This will also likely reduce the amount of total code.

Results

Refactoring 0

Privatizing the fields was easy. It was as simple as making a field private and adding the getter/setter methods from Eclipse's built in methods. I then removed the setter methods where necessary and as many getters as I could.

However, privatizing the methods might be impossible. Most, if not all of the methods are required for tests and for other classes in different packages, so unfortunately because there's so many dependencies in the code, I don't think it will be possible to make the methods privatized unfortunately.

Refactoring 1

Most of the refactoring was changing code from this form

void addPoint(final Point newPoint, final int position) {
	if (counterClockWise(points.get(position), points.get((position + 1) % numPoints), newPoint) && counterClockWise(points.get(((position - 1) + numPoints) % numPoints), newPoint, points.get(position)) && counterClockWise(newPoint, points.get((position + 2) % numPoints), points.get((position + 1) % numPoints))) {
		points.add(position + 1, newPoint);
		numPoints++;
	}
}

to the far more readable version

void addPoint(final Point newPoint, final int position) {
	Point twoBefore = points.get(((position - 1) + numPoints) % numPoints);
	Point oneBefore = points.get(position);
	Point oneAfter = points.get((position + 1) % numPoints);
	Point twoAfter = points.get((position + 2) % numPoints);

	//This checks if the new point would be counterclockwise with the other points nearby where it is being added.
	if (counterClockWise(oneBefore, oneAfter, newPoint) && counterClockWise(twoBefore, newPoint, oneBefore) && counterClockWise(newPoint, twoAfter, oneAfter)) {
		points.add(position + 1, newPoint);
		numPoints++;
	}
}

In other words we saved calls to variables so that sections with logic (such as if statements) were not riddled with 4+ copies of object.method(input)+1%number, making reading what should be simple logic a chore.

We did change contains() to be far more readable by changing a list {0,0,0,0} to just be a boolean. There was a lot of spacing changing done, as before a single line of code could span many lines.

Refactoring 2

This one wasn't too hard. I removed the lightColorScheme from PointsPanel and added a getter-like method in the classes that needed it. lightColorScheme was also in the PointsTable class, and I added the necessary fields to call the method from PointsTable, so it was as easy as just deleting the unnecessary lightColorScheme in PointsTable and fixing where it broke in the other files by adding in a PointsTable.lightColorScheme. I tried adjusting some of the other methods to be in different classes, but as mentioned in the Refactoring 0, these methods are very hard to move around and adjust their modifiers to be private due to the many dependencies. It also wasn't entirely necessary to move around anything else I would say.

Refactoring 3

We decided to leave this as is, as we are not fully comfortable messing with this branch of constructor tomfoolery.

Refactoring 4

Instead of doing what we planned, we instead added a new constructor to Point.java that takes no input and simply starts with random X and Y values. The other constructor required an X and Y input. There were two calls that needed to be changed in order to make this work, but those were trivial.

Refactoring 5

This was not refactored, as it did not function in the way we originally assumed. We initially believed that this was a case of duplicated code. It is actually the case that one method calls the other, and both methods are necessary.