Duplicate code found in `core/record.go`

[jwnpoh]

Hello! It seems like there are two functions in core/record.go that do the same thing, ListRecords and loadAllRecords.

Being new to programming, I am not sure if this is a feature or bug :confused:

[dominikbraun]

You're right, those two functions seem to do the exact same things. Thanks for noticing this!

ListRecords is the public method that is used by outside callers - in our case, the cli package - to get a list of records. loadAllRecords is an internal method that is used by other internal methods.

The best solution to approach this is to remove the implementation of the ListRecords function and just call loadAllRecords inside instead. That way, the public ListRecords methods remains unchanged for all outside callers.

[krishnaindani]

Is this issue open to work, I would like to take a stab at it?

[jwnpoh]

The best solution to approach this is to remove the implementation of the ListRecords function and just call loadAllRecords inside instead. That way, the public ListRecords methods remains unchanged for all outside callers.

May I ask why this is preferable to simply removing the unexported function?

[dominikbraun]

The best solution to approach this is to remove the implementation of the ListRecords function and just call loadAllRecords inside instead. That way, the public ListRecords methods remains unchanged for all outside callers.

May I ask why this is preferable to simply removing the unexported function?

ListRecords provides the timetrace list records feature. At the moment, it does the exact same thing as loadAllRecords and nothing more. But what if the timetrace list records command requires some additional logic or validations in the future? In that case, if you would just have removed loadAllRecords and use ListRecords instead, this additional logic and validations would be executed for every other command that somehow loads records using ListRecords!

In short, they have two different responsibilities: Providing the list record feature vs. just loading records. It is just mere coincidence that these currently are the same two things. We want to keep them separated to indicate that these are two different responsibilities.

[dominikbraun]

Is this issue open to work, I would like to take a stab at it?

Yes, this is a good issue for getting started - are you ok with this, @jwnpoh?

[jwnpoh]

ListRecords provides the timetrace list records feature. At the moment, it does the exact same thing as loadAllRecords and nothing more. But what if the timetrace list records command requires some additional logic or validations in the future? In that case, if you would just have removed loadAllRecords and use ListRecords instead, this additional logic and validations would be executed for every other command that somehow loads records using ListRecords!

In short, they have two different responsibilities: Providing the list record feature vs. just loading records. It is just mere coincidence that these currently are the same two things. We want to keep them separated to indicate that these are two different responsibilities.

So, in a sense it's about "future-proofing" with a basic architecture that will be able to support additional functionality that will be built in the future.

Yes, this is a good issue for getting started - are you ok with this, @jwnpoh?

Sure thing! Please go ahead and assign this to @krishnaindani.

[krishnaindani]

Thank you. I will go through this and post if I have a questions.

[krishnaindani]

@dominikbraun I have created the PR https://github.com/dominikbraun/timetrace/pull/164, let me know what do you think.

[dominikbraun]

Closed by #164. Thanks, @jwnpoh and @krishnaindani!