Pull Request for CREATE statements

[shadeven]

@alexfu, I have pushed what I have done so far, it is not full completed yet. I want to make sure I am on the right track before proceeding any further.

1. CREATE statement is slightly different to SELECT because it has two brackets involved. Initially I tried to chain them together like what you did in SELECT. However, it becomes difficult to implement unless we write something like this which I don't prefer: SQLiteQueryBuilder.createTable("myTable").bracket().column("column1", INT).bracket();
2. Still need to include more column types in ColumnType class.

Please review and provide your thoughts. Cheers, Steven

[alexfu]

@shadeven It's looking good so far. :+1:

[shadeven]

Thanks, @alexfu. By the way, do you know how to sync the forked repository with your repository? I am using SourceTree.

[alexfu]

@shadeven If you want to sync with the develop branch, you will want to merge the develop branch into your fork... See more here: https://help.github.com/articles/syncing-a-fork/

[shadeven]

Got it. Thanks.

[coveralls]

Coverage increased (+1.93%) to 90.57% when pulling 065a70dbfb3e03266a381f27f4abf8e2da1e4caa on shadeven:develop into 52626924f7b424107a6eee59aaad2e923cc6d513 on alexfu:develop.

[coveralls]

Coverage decreased (-1.14%) to 87.5% when pulling 0806c53ee61dc827e163909b388f8422f0da76c9 on shadeven:develop into 52626924f7b424107a6eee59aaad2e923cc6d513 on alexfu:develop.

[coveralls]

Coverage increased (+0.9%) to 89.53% when pulling fc58b49ff74e82dc8447848ea7d4dff90fe20825 on shadeven:develop into 52626924f7b424107a6eee59aaad2e923cc6d513 on alexfu:develop.

[shadeven]

@alexfu, since my last pull request, I tried a different approach to construct CREATE statement.

1. Creates a new ColumnDefinition class using builder pattern to define a column.
2. Adds optional attributes for a column: length, decimal, nullable, primary key and default value.
3. Columns now can be chained together and ends with "end" method.
4. Adds more data type to ColumnType.
   Please refer to the unit tests for more details. Please review and provide your feedback. If you are happy with it, you can pull it back to your branch.

Cheers, Steven

[alexfu]

@shadeven give me some time to look it over and I'll get back to you. thanks!

[shadeven]

No problem, mate. I want to upskill in android development. Happy to involve more!

[shadeven]

@alexfu, any updates on the pull request?

[alexfu]

@shadeven Sorry, been quite busy.

I looked it over and the API you created isn't quite there in terms of desired usage. The end() function is a bit weird, although I understand it was necessary since you can have N amount of column defs.

Here is an example of how I would imagine generating a CREATE statement with this library...

String query = SQLiteQueryBuilder.createTable("myTable")
    .column(
         new Column("a", Type.INTEGER, Constraint.PRIMARY_KEY), 
         new Column("b", Type.TEXT)
    ).toString();

Keep in mind the example above is an idea and not a hard requirement. Although, I think I would like to try and match it as closely as possible.

[shadeven]

@alexfu, thanks for your feedback.

Using end() method is necessary if the API requires to chain columns together like what you did in SELECT. If we can use an array to define columns, then end() is not needed. I have implemented both approaches for your reference.

In terms of the desired usage, my approach is cleaner and more flexible:

1. Uses builder pattern to construct column definition instance which is immutable.
2. Except for column name and type, other attributes are optional such as PRIMARY KEY, NULLABLE etc.

If the implementation is not what you have in mind, please understand that I did this without any specifications or requirements in place. I did this purely for the willingness to learn and support open source project.

Happy to discuss further if required. Cheers, Steven

[alexfu]

@shadeven Don't get me wrong, I deeply appreciate your contribution thus far, especially since you're the only other contributor. I also realize that there were no specs or requirements to go off of as I do not have a clear idea of what I would like this particular API to look like. Again, thank you for your contribution.

My ultimate goal for the API is to minimize verbosity and to get straight to the point. This becomes important when dealing with large amounts of columns.

The builder pattern is not exactly the least verbose when it comes to constructing objects which is why I'm not in favor of using it in this case. Builder pattern is great when dealing with large amount of arguments to a constructor, but in this case, we only really have 3 arguments (at most).

After giving this some more thought, I think something like this...

SQLiteQueryBuilder.createTable("myTable")
    .column(new Column("a", Type.INTEGER, Constraint.PRIMARY_KEY))
    .column(new Column("b", Type.TEXT))
    .toString();

Could actually be achievable (without the need of a terminator such as end()) if we change the way statements are created.

For example, if we collected the column definitions and then construct the statement at the end instead of constructing the statement on an as-needed basis...

class CreateTableImpl implements CreateTable {

  private List<Column> definitions;

  public CreateTableImpl() {
    definitions = new ArrayList<Column>();
  }

  public CreateTable column(Column columnDef) {
    definitions.add(columnDef);
    return this;
  }

  @Override
  public String toString() {
    StringBuilder statement = new StringBuilder();
    // Write CREATE TABLE table_name to StringBuilder
    for (Column columnDef : definitions) {
      // Append column definitions to StringBuilder
    }
    return statement.toString();
  }
}

[shadeven]

@alexfu, after reading your comment, now I understand what your vision is.

If the required argument is 3 at most, then you are right we don't need builder pattern. The project is designed for android SQLite database anyway.

From what we have implemented so far, I would say we are actually very close. Your sniffer is similar to my original approach where Array is used instead of List, and 'end()' method is not needed.

I will make the code change and submit for your second review. Please don't get me wrong either. It's actually fun to work with you thus far! That's how we learn and progress.

Cheers, Steven

[coveralls]

Coverage increased (+1.67%) to 90.7% when pulling de9425a0562f45ba89faff6af4cd45e0c85bb98f on shadeven:develop into fa21100f5085956f1da36f06d0593f191b320a9c on alexfu:develop.

[coveralls]

Coverage increased (+3.51%) to 92.54% when pulling d65a3d0f09280571e5a19d75c1f81634852af4c8 on shadeven:develop into fa21100f5085956f1da36f06d0593f191b320a9c on alexfu:develop.

[shadeven]

@alexfu, I just committed my latest changes. It is implemented as per your suggestion above. Please have a look and provide your feedback.

[alexfu]

@shadeven looks good. I'll leave this PR open until I finish #2 and then I'll merge it in.

[shadeven]

@alexfu, great! If you have another ticket, I am happy to take it.