Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Read jot presenter vc #6

Merged
merged 17 commits into from
Nov 20, 2018
Merged

Read jot presenter vc #6

merged 17 commits into from
Nov 20, 2018

Conversation

RezaAkhtar
Copy link
Contributor

Default jot ids are usually random integers, but we should add a utility function that gets the "next" id or something like that because it will eventually conflict.

Copy link
Contributor

@AWiryaman AWiryaman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall just comments needed for functions if they're not in a protocol which is commented

Contuity/Views/Jot/Write/WriteJotPresenter.swift Outdated Show resolved Hide resolved
ContuityTests/ModelsTests/JotTests.swift Show resolved Hide resolved
ContuityTests/ModelsTests/JotTests.swift Outdated Show resolved Hide resolved
super.tearDown()
}

func testViewDidLoad() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

comments

Copy link
Contributor

@chuchi-chaschtli chuchi-chaschtli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The main comment that isn't nitpicky stuff is addressing what the text var in the ReadJotPresenter is really doing, and why we need to conform to the UITextViewDelegate in the ReadJotVC. I might jus tbe missing something though

return nil
}
do {
for row in try conn.prepare("SELECT * FROM jot WHERE id = \(givenID)") {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think all this logic can be condensed to a smaller function

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you're interested I did it in my branch:
` static func read(givenID: Int) throws -> Jot {
guard let conn = DatabaseManager.shared.conn
else {
throw DatabaseError.selectFailed
}
do {
for row in try conn.prepare("SELECT * FROM jot WHERE id = (givenID)") {
return try parseRow(row: row)
}
}
catch {
throw DatabaseError.selectFailed
}
throw DatabaseError.selectFailed
}

/// Parses a single row from the database into a Jot
///
/// - Parameter row: a row from the database
/// - Returns: a Jot
/// - Throws: DatabaseError.selectFailed if the database SELECT failed
static func parseRow(row: Statement.Element) throws -> Jot {
    guard let optionalID: Int64 = row[0] as? Int64,
        let newData = row[1] as? String
        else {
            throw DatabaseError.selectFailed
    }
    let id = Int(optionalID)
    let row2 = row[2]
    var newQueue = false
    switch row2 {
    case let row2 as Int64:
        if row2 == 1 {
            newQueue = true
        }
    default:
        break
    }
    var newCreatedAt = ""
    let row3 = row[3]
    switch row3 {
    case let row3 as String:
        newCreatedAt = String(row3)
    default:
        throw DatabaseError.selectFailed
    }
    guard let newModifiedAt = row[4] as? String? else{
        throw DatabaseError.selectFailed
    }
    var newLat: Double?
    let row5 = row[5]
    switch row5 {
    case let row5 as Double:
        newLat = Double(row5)
    default:
        throw DatabaseError.selectFailed
    }
    var newLong: Double?
    let row6 = row[6]
    switch row6 {
    case let row6 as Double:
        newLong = Double(row6)
    default:
        throw DatabaseError.selectFailed
    }
    return Jot(id: id,
               data: newData,
               queue: newQueue,
               createdAt: newCreatedAt,
               modifiedAt: newModifiedAt,
               latitude: newLat,
               longitude: newLong)
}`

We can probably just leave it like this and I'll just do the refactor in my branch

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sgtm


static func read(givenID: Int) -> Jot? {
guard let conn = DatabaseManager.shared.conn
else {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Put the else on this same line as the guard unless the first line is 150+ chars

Contuity/Models/Initiative.swift Outdated Show resolved Hide resolved

extension ReadJotViewController: UITextViewDelegate {
func textViewDidChange(_ textView: UITextView) {
presenter.text = textView.text
Copy link
Contributor

@chuchi-chaschtli chuchi-chaschtli Nov 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will this ever even get called?? When will the text view change in the ReadJotVC? I might just be missing something here

If it's not getting called, please:

  • remove the UITextViewDelegate logic
  • remove the var text: String from the Presenter and just make setText(jotID: Int) to something like getText(from jotId: Int) -> String which returns the text for the viewcontroller to display

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call, it wasn't getting called. That was just there when I was trying to mimic what you had to see why mine wasn't working. It's deleted now.

ContuityTests/ModelsTests/JotTests.swift Show resolved Hide resolved
return nil
}
do {
for row in try conn.prepare("SELECT * FROM jot WHERE id = \(givenID)") {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sgtm

@RezaAkhtar RezaAkhtar merged commit 9c72bb2 into master Nov 20, 2018
@RezaAkhtar RezaAkhtar deleted the read-jot-presenter-vc branch November 24, 2018 01:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants