[REQUEST]: Refactor - Figure the approach to use scala functional design in codebase

[thugsatbay]

Problem Statement

We are creating instance for case classes from create methods (using static method approach) to instantize a class based on certain values and conditions. However scala still allows you to declare the same class without the create method by using the internal apply method for the case class. This creates a semantic issue as we have two definitions of creating an instance of the same class and both ways are semantically different.

When codebase will become bigger this can have issues as it can lead to poor code health.

Background and Motivation

Using strict design patterns.

Proposed Solution

NA, needs discussion.

Alternatives

NA

Known/Potential Compatibility Issues

NA, as it is internal codebase change not user facing.

Design

NA

Implementation

NA

Performance Implications (if applicable)

NA

Open issues (if applicable) NA

Additional context (if applicable) NA

[clee704]

I only see IndexLogEntry having such create() method which is recently added. Are there any other such classes?

For IndexLogEntry, both apply() and create() are being used, so we should first check if both can be unified. @imback82 Do you have any idea? Can we just rename create() to apply() and remove uses of .create()?

[imback82]

Can we just rename create() to apply() and remove uses of .create()?

I believe this would cause stack overflow (infinite recursion) since they have the same parameters. You can try and check.

Basically, we want to to call create when we create a new IndexLogEntry to insert the current Hyperspace version, but use the constructor if we construct the object from the log entry file to retain the Hyperspace version in the log. Maybe we can check ObjectMapper to see whether we can call a def other than the constructor so that the def will be private to the invocation from ObjectMapper.

[andrei-ionescu]

To get rid of the recursion it is simple. In the helper constructor you use the new keyword.

The issue is that doing IndexLogEntry(...) will always resolve to the case class constructor and not the the helper one. It cannot be detected which one to use due to the fact that the signature of both is the same.

You could use IndexLogEntry.apply(...) to force it to go to the helper constructor but that defeats the purpose and is the same with what's already in the code.

I have 2 suggestions:

1. Because I've seen that there are about 4 places where the IndexLogEntry is created with different properties it may be helpful to use a factory pattern.
2. Redefine the helper constructor (the IndexLogEntry.apply) and add an extra boolean parameter called addVersion.

[imback82]

2. Redefine the helper constructor (the IndexLogEntry.apply) and add an extra boolean parameter called addVersion.

This seems reasonable to me since the intention should be clear from the caller side.

[clee704]

By the way, it seems you can override the apply method of the companion object since Scala 2.12.2: https://github.com/scala/scala/pull/5816. So this works:

case class A(a: Int)
object A {
  def apply(a: Int): A = { new A(a + 1) }
}