coldstar96 / cse403

Budget Manager Project
1 stars 2 forks source link

Entry changes #15

Closed brucec5 closed 11 years ago

brucec5 commented 11 years ago

I went to work on the API, I noticed that in Entry there was a place where the variable notes was used but it was never declared. So I went in, fixed that, and made some other changes in the class. The diff looks like a mess, but the main points of the change are:

jiwpark90 commented 11 years ago

I actually wanted to discuss that briefly tomorrow but I just went ahead and merged it so people could get rid of the errors.

Thank you for pointing those out tho

On Wed, May 1, 2013 at 11:32 PM, brucec5 notifications@github.com wrote:

I went to work on the API, I noticed that in Entry there was a place where the variable notes was used but it was never declared. So I went in, fixed that, and made some other changes in the class. The diff looks like a mess, but the main points of the change are:

  • Removed some of the excess constructors that I felt were unnecessary
  • Changed label to notes to conform with what we talked about
  • Made a copy of the date before returning it in getDate()
  • Add apiSave() and apiDelete() methods that will call into the ApiInterface, as this seems like better style to me than calling directly into the ApiInterface from an activity. If anyone thinks otherwise, please comment. You can merge this Pull Request by running: git pull https://github.com/coldstar96/cse403 entry-fix Or you can view, comment on it, or merge it online at: https://github.com/coldstar96/cse403/pull/15 -- Commit Summary --
coldstar96 commented 11 years ago

Use builder design pattern to avoid multiple constructors

brucec5 commented 11 years ago

I'm not entirely sure the builder pattern will be of too much use here. If we wanted to remove that last constructor, we could just use a setId() method or something. I guess with the way we have it now with the API, this is necessary in order to allow the API to set the ID. At some time past the alpha phase, I plan on restructuring the API interface to have most of the code within the Entry and Budget classes so that we don't have to expose things like the ID in the class's external interface.

But for now, we'll just use the constructor(s). Anyone else want to weigh in on this before the meeting?

jiwpark90 commented 11 years ago

Sounds to me like that'll be fine until past Alpha.

On May 2, 2013, at 1:51 PM, brucec5 notifications@github.com wrote:

I'm not entirely sure the builder pattern will be of too much use here. If we wanted to remove that last constructor, we could just use a setId() method or something. I guess with the way we have it now with the API, this is necessary in order to allow the API to set the ID. At some time past the alpha phase, I plan on restructuring the API interface to have most of the code within the Entry and Budget classes so that we don't have to expose things like the ID in the class's external interface.

But for now, we'll just use the constructor(s). Anyone else want to weigh in on this before the meeting?

— Reply to this email directly or view it on GitHub.

grablair commented 11 years ago

I don't see anything wrong with the two constructors. It's really just one field we will have to change when we get a response back from the server.

josephshieh commented 11 years ago

We should delete this branch? And for the compareTo() method? Will there be other methods for sorting by Date later on?