Fl01 #9
Fl01 #9
Conversation
| return if (result.matches(lastKey)) { | ||
| result | ||
| } else { | ||
| resultSet?.last() |
There was a problem hiding this comment.
Is this acceptable to have a null resultSet here?
There was a problem hiding this comment.
At the moment filterRecord is called just when resultSet is not null, but, as I think it could be used elsewhere, I think we can have a "relaxed" behaviour. The idea is "in case you have an open resultset, just set on EOF, if you don't have it, EOF is already on". Do you think it wolud be better to encapsulate this in a fun with a speaking name?
There was a problem hiding this comment.
I am not sure as I do not know the context very well, but if you would be "surprised" if resultSet is actually null then I would write resultSet!!.last() and if later you confirm it can be relaxed then I would change it back
| @@ -4,12 +4,13 @@ import com.smeup.rpgparser.interpreter.DBFile | |||
| import com.smeup.rpgparser.interpreter.Field | |||
| import com.smeup.rpgparser.interpreter.Record | |||
| import com.smeup.rpgparser.interpreter.Value | |||
| import java.lang.RuntimeException | |||
| import java.sql.Connection | |||
| import java.sql.ResultSet | |||
|
|
|||
| class DBSQLFile(private val name: String, private val connection: Connection) : DBFile { | |||
There was a problem hiding this comment.
Given I am not familiar with this part of the code I looked into DBFile. Depending on the background of other contributors what DBFile does could be not obvious and we may want to add a few comments
| import java.sql.Connection | ||
| import java.sql.ResultSet | ||
|
|
||
| class DBSQLFile(private val name: String, private val connection: Connection) : DBFile { | ||
| private var resultSet: ResultSet? = null | ||
| private var lastKey: List<Field> = emptyList() |
There was a problem hiding this comment.
is this specific for the readEqual? Or will this be updated by multiple operations?
There was a problem hiding this comment.
Good question. The answer is "Since I'm learning RPG while creating the interpreter, I don't really know" :-)
Should we write the code differently depending on this?
There was a problem hiding this comment.
no, just maybe a more specific name like readEqualLastKey
| @@ -19,7 +20,7 @@ class DBSQLFile(private val name: String, private val connection: Connection) : | |||
| if (resultSet == null) { | |||
There was a problem hiding this comment.
You could use require here. I personally find it more obvious
| return chain(toFields(key)) | ||
| } | ||
|
|
||
| private fun toFields(keyValue: Value): List<Field> { |
There was a problem hiding this comment.
It is not clear to me what this function does. Probably because I am not familiar with this portion of the codebase
There was a problem hiding this comment.
A chain operation (lookup a record in a table) can be perfomed using one or more keys. So we have to transform a value (e.g. "ABC") in a Field (i.e. "PART_NUMBER"->"ABC") and then encapsulate it in a list. Do you have any suggestion in order to make the code more clear?
There was a problem hiding this comment.
To me is not clear what a Field is in this context. Is it a data structure field? In your example is "PART_NUMBER" the name of the field used as a key and is "ABC" the actual value of the field?
There was a problem hiding this comment.
Also, the term Field makes me think to the data structure fields, while these are different fields, right? Perhaps we could have a more specific name to make this more obvious
| add(it) | ||
| } | ||
| } | ||
| fun matches(keyFields: List<Field>): Boolean { |
There was a problem hiding this comment.
I would add a newline before matches
| } | ||
| } | ||
| fun matches(keyFields: List<Field>): Boolean { | ||
| return keyFields.all { this[it.name] == it.value } |
There was a problem hiding this comment.
given the body is an expression you could rewrite it using the shortcut with the equals:
fun matches(keyFields: List<Field>) = keyFields.all { this[it.name] == it.value }
| assertEquals(DecimalValue(bigDecimalValue), chainedRecord[1].value) | ||
| val fieldsIterator = chainedRecord.iterator() | ||
| assertEquals(StringValue("XXX"), fieldsIterator.next().value) | ||
| assertEquals(DecimalValue(bigDecimalValue), fieldsIterator.next().value) | ||
| } | ||
| } |
There was a problem hiding this comment.
Normally I would add a newline at the end of the file
rpgJavaInterpreter-core/src/test/kotlin/com/smeup/rpgparser/interpreter/RecordTest.kt
Show resolved
Hide resolved
| ) | ||
| assertFalse(record.matches((keyFieldsKO))) | ||
| } | ||
| } |
There was a problem hiding this comment.
I would add a newline here
Implementing methods for DB access