Community feedback on project

460 views
Skip to first unread message

John Wilson

unread,
Nov 21, 2014, 10:33:27 AM11/21/14
to golan...@googlegroups.com
Hi everyone,

I've released version 0.2 of Bytengine Content Repository and would really appreciate feedback (code and application review). I undertook this project to serve a personal need and to also learn Go. I'm sure there are a few coding horrors but please don't hesitate to point them out so I can learn and improve.

Here are a few links to fully appreciate the project's aim:

Cheers
John

egon

unread,
Nov 22, 2014, 7:18:15 AM11/22/14
to golan...@googlegroups.com
A lot of comments, based on first impressions...

# cmd structure

Common way to have command line utils in a packages is under a "cmd" folder. (e.g. see https://github.com/bradfitz/camlistore/tree/master/cmd)

I would recommend having:

cmd/bytengine
cmd/beshell

# Naming - avoid stutter

The package name is part of a name

The full name for that type is bfs.BFSResponse, bfs.Response is nicer.

mongo.MongodbBFS -> mongo.BFS or mongodb.BFS

There are more similar to this....

# bfs.Response

It would look better as:

type Status string
const (
OK Status = "ok"
Error Status = "error"
)

type Response struct {
Status Status
StatusMessage string
Data map[string]interface{}
}

This means you don't need a lot of the functions

In bfs/mongo you can now return

bfs.Response{bfs.OK, "", data}
bfs.Respones{bfs.Error, "something missing", nil}

Checking for success...
if bfs.Response.Status == bfs.OK {}


Of course the idiomatic way would be to have multiple returns

type Response map[string]interface{}
type BFS interface {
Start(config string, b *bst.ByteStore) error
ClearAll() (Response, error)
ListDatabase(filter string) (Response, error)
CreateDatabase(db string) (Response, error)
DropDatabase(db string) (Response, error)
NewDir(p, db string) (Response, error)
NewFile(p, db string, jsondata map[string]interface{}) (Response, error)
ListDir(p, filter, db string) (Response, error)
ReadJson(p, db string, fields []string) (Response, error)


CommandHandler would be more appropriate

Add would suffice.

# Naming of packages

ext -> plugin, then you can use the methods as:

plugin.New(...)
plugin.Register(...)

Similarly: fltcore -> plugin/datafilter

datafilter.New(...)
datafilter.Register(...)

Also, somehow the package layout isn't indicitave of the different parts.

auth, bfs, bst, core look like they should belong in the same package...

maybe pull auth, bfs, bts into the root level:
auth.User -> bytengine.User
core.Engine -> bytengine.Engine
bfs.BFS -> bytengine.FileSystem
bst.ByteStore -> bytengine.ByteStore 

auth.Authentication -> authentication.Interface
 // authentication.Register / authentication.New

bfs/mongo -> bytengine/filesystem/mongo
bts/mongo -> bytengine/bytestore/mongo

Basically I would leave the only the registration points in the separate packages... 

Anyways... I'm also not convinced that my would be the best either... anyways a point that could use some hard thinking :)

# misc


Name it "func user_new", then you won't need the comment.

# generally

Otherwise, the code looks great... nice and clean as it should be.

+ Egon

John Wilson

unread,
Nov 22, 2014, 11:30:36 AM11/22/14
to golan...@googlegroups.com
@Egon

Thanks a bunch for taking the time to look at the code and give me feedback. This is exactly what I need!
I'll do 'some hard thinking' based on your suggestions and update this post.

Thanks again for the feedback!

John Wilson

unread,
Nov 25, 2014, 10:57:05 AM11/25/14
to golan...@googlegroups.com
Hi everyone!

So I've done a bit of refactoring and here's a quick report.

# cmd structure

Common way to have command line utils in a packages is under a "cmd" folder. (e.g. see https://github.com/bradfitz/camlistore/tree/master/cmd)

I would recommend having:

cmd/bytengine
cmd/beshell
 
Done and updated the README. It makes the 'go get' command longer but it also makes a lot more sense (thanks for the camlistore link) 

# Naming - avoid stutter

The package name is part of a name

The full name for that type is bfs.BFSResponse, bfs.Response is nicer.

mongo.MongodbBFS -> mongo.BFS or mongodb.BFS

There are more similar to this....

Absolutely right, made changes still looking through to catch more of them

# bfs.Response
It would look better as:
type Status string
const (
OK Status = "ok"
Error Status = "error"
)
type Response struct {
Status Status
StatusMessage string
Data map[string]interface{}
}
This means you don't need a lot of the functions
In bfs/mongo you can now return
bfs.Response{bfs.OK, "", data}
bfs.Respones{bfs.Error, "something missing", nil}
Checking for success...
if bfs.Response.Status == bfs.OK {}

Thanks for pointing this out, i also added a Map() function https://github.com/johnwilson/bytengine/blob/master/filesystem/filesystem.go#L37 to make the 'pretty' data filter implementation a bit better https://github.com/johnwilson/bytengine/blob/master/plugin/datafilter/core.go#L33

Of course the idiomatic way would be to have multiple returns
type Response map[string]interface{}
type BFS interface {
Start(config string, b *bst.ByteStore) error
ClearAll() (Response, error)
ListDatabase(filter string) (Response, error)
CreateDatabase(db string) (Response, error)
DropDatabase(db string) (Response, error)
NewDir(p, db string) (Response, error)
NewFile(p, db string, jsondata map[string]interface{}) (Response, error)
ListDir(p, filter, db string) (Response, error)
ReadJson(p, db string, fields []string) (Response, error)
}

I chose to be 'less idiomatic' for the following reasons:
  1. I didn't want to use two channels when getting responses from  'filesystem' namely one for 'bfs.Response' types and one for 'error' types, thus requiring a 'switch statement' here https://github.com/johnwilson/bytengine/blob/master/cmd/bytengine/bytengine.go#L53
  2. I didn't want the http api to be responsible for creating the 'filesystem' error responses because of separation of concern. The http api is just one of the possible interfaces I have planned  for Bytengine.
Actually this has been updated to Router because that is really all it's doing, routing the commands to the handlers. I hope the correction makes a bit more sense now. Thanks for pointing that out. The 'Add' method suggestion has been implemented as well.

# Naming of packages
ext -> plugin, then you can use the methods as:
plugin.New(...)
plugin.Register(...)
Similarly: fltcore -> plugin/datafilter
datafilter.New(...)
datafilter.Register(...)
Also, somehow the package layout isn't indicitave of the different parts.
auth, bfs, bst, core look like they should belong in the same package...
maybe pull auth, bfs, bts into the root level:
auth.User -> bytengine.User
core.Engine -> bytengine.Engine
bfs.BFS -> bytengine.FileSystem
bst.ByteStore -> bytengine.ByteStore 
auth.Authentication -> authentication.Interface
 // authentication.Register / authentication.New
bfs/mongo -> bytengine/filesystem/mongo
bts/mongo -> bytengine/bytestore/mongo
Basically I would leave the only the registration points in the separate packages... 
Anyways... I'm also not convinced that my would be the best either... anyways a point that could use some hard thinking :)

Boy this was a big one! That's why it's taking me this long to post a reply. I took into account some of your suggestions and done quite a bit of refactoring! So this is what has been done:
  1. All plugins are now in the bytengine/plugin/ package https://github.com/johnwilson/bytengine/tree/master/plugin with a plugin.go file to handle registration and creation of plugins. https://github.com/johnwilson/bytengine/blob/master/plugin/plugin.go . I'm sure the code can be made better though so will work on it.
  2. Command handlers and routes as well as configuration have been moved to the bytengine/core package https://github.com/johnwilson/bytengine/tree/master/core The nice thing with the refactoring is that it makes the config.go more understandable than before in terms of what plugins are being imported https://github.com/johnwilson/bytengine/blob/master/core/config.go#L3
  3. Renaming of some packages to make the project layout less cryptic
# misc
https://github.com/johnwilson/bytengine/blob/master/bytengine/engine/handlers.go#L17
Name it "func user_new", then you won't need the comment.

No argument there, you're right, I wasn't thinking straight there (must have been the red bull!).

# generally
Otherwise, the code looks great... nice and clean as it should be.

This actually made my day because as a Go novice and "programming padawan" putting your code out there for scrutiny is very uncomfortable but very necessary!

Overall I think things are looking much better than before but as always I'm sure there's room for improvement so please keep the comments coming and thanks once again for forcing me to think harder!

Cheers 

egon

unread,
Nov 25, 2014, 12:31:18 PM11/25/14
to golan...@googlegroups.com


On Tuesday, 25 November 2014 17:57:05 UTC+2, John Wilson wrote:
Hi everyone!

So I've done a bit of refactoring and here's a quick report.

# cmd structure

Common way to have command line utils in a packages is under a "cmd" folder. (e.g. see https://github.com/bradfitz/camlistore/tree/master/cmd)

I would recommend having:

cmd/bytengine
cmd/beshell
 
Done and updated the README. It makes the 'go get' command longer but it also makes a lot more sense (thanks for the camlistore link) 

I believe I didn't explain this well enough:

Here, I meant that the bshell should be probably in "github.com/johnwilson/byteengine/cmd/bshell"... 

If there is only a single executable, it's fine to have the toplevel to be the main - it simplifies get.

I chose to be 'less idiomatic' for the following reasons:
  1. I didn't want to use two channels when getting responses from  'filesystem' namely one for 'bfs.Response' types and one for 'error' types, thus requiring a 'switch statement' here https://github.com/johnwilson/bytengine/blob/master/cmd/bytengine/bytengine.go#L53
There are two solutions for this... first one is to have an additional type for the channel value, but this is good only for internal use.

Generally the public API shouldn't contain channels, it's harder to use... of course there are exceptions. Try to expose a synchronous API and let the user make it async, if needed.

So:
c := &core.CommandRequest{cmd, form.Token, make(chan bfs.Response)}
ctx.Writer.Header().Set("Content-Type", "application/octet-stream")
CommandsChan <- c
data := <-c.ResultChannel

But it would look a lot better like:
data, err := core.ExecuteCommand(cmd, form.Token)

// if for some reason you need to do other things at there, the user of that API can always do:
var data core.Response
var err error
go func(){ data, err = engine.ExecuteCommand(cmd, form.Token) }()

If the user needs multiple engines at their disposal, then create that in the main... rather than in the core. I.e. the core is doing the "engine" stuff and also the "load-balancing" and those really don't fit together.

Of course this would also get rid of the "CommandRequest" type from core and should simplify the API.
Now things look much, much better. I can now see further/better ways of improving it.

You should have noticed that you still have some repetition:

auth.Authentication
bytestore.ByteStore
filesystem.BFS
datafilter.DataFilter
statestore.StateStore

Also when you have a single public item in the API of a package... it probably shouldn't be a package.

I have two alternative ideas how to reorganize things (I'm not sure which would be better, both should probably tried out):

1. The more I look at the things they look like they should belong together - i.e. put core, auth ,bytestore, filesystem, plugin, statestore into "github.com/johnwilson/byteengine" package.

It should get rid of lot imports... it would get rid of plugin package. etc. all this core importing the plugins would disappear etc.

The only things I would keep separate are the default router commands and concrete plugin implementations. i.e. create a registry for them https://github.com/johnwilson/bytengine/blob/master/core/routes.go and put the actual implementations somewhere like "commands/user.go", "commands/server.go"... and have a global registry for those similarly to plugins.

Also this makes it easier to use in other projects, i.e. there is one public API point for using the package.

2. Remove the plugin package.

Put the "core" package into "github.com/johnwilson/byteengine".
Put the registries back into auth, bytestore, filesystem, datafilter... make the public types similar to each other:

auth.Authentication -> auth.Interface
bytestore.ByteStore -> bytestore.Interface
filesystem.BFS -> filesystem.Interface
datafilter.DataFilter -> datafilter.Interface
statestore.StateStore -> statestore.Interface

// similarly all should have:
auth.Register(...)
auth.Create(...)

It would add nice symmetry to the packages.

Although... the more I think of it... having all of these things in separate package doesn't seem to be worth the effort. Basically it doesn't make things clearer, it doesn't make things easier to manage... so... I would guess 1. approach would be better.

egon

unread,
Nov 25, 2014, 12:33:02 PM11/25/14
to golan...@googlegroups.com
PS. also keep the registrations separate
bytestore.RegisterAuth
bytestore.RegisterFilter
bytestore.RegisterCommand
...

John Wilson

unread,
Nov 25, 2014, 12:49:33 PM11/25/14
to golan...@googlegroups.com
In regard to suggestion 1, when core, auth, bytestore, filesystem etc are put into "bytengine" (root) package this causes an import cycle error because they're used by the plugins and the plugins are in turn called by config.go which is at root package level.

egon

unread,
Nov 25, 2014, 12:59:01 PM11/25/14
to golan...@googlegroups.com
On Tuesday, 25 November 2014 19:49:33 UTC+2, John Wilson wrote:
In regard to suggestion 1, when core, auth, bytestore, filesystem etc are put into "bytengine" (root) package this causes an import cycle error because they're used by the plugins and the plugins are in turn called by config.go which is at root package level.

If you move the importing of specific plugins/extensions/commands into cmd/bytengine then it will break the cycle.

If for some reason you need to duplicate the things supported, then using a package "plugins" that imports all the "default" implementations is a possibility... and the main imports that "plugins".

Basically:
bytestore/cmd/bytestore -> bytestore/plugins -> [diskv, mongo, redis...] -> bytestore

Or:
bytestore/cmd/bytestore -> [diskv, mongo, redis...] -> bytestore

John Wilson

unread,
Nov 25, 2014, 1:11:56 PM11/25/14
to golan...@googlegroups.com
Right, that's not a bad idea at all. Blimey you're quick! 
So if I undrstand you clearly, this will mean that config.go which imports plugins and config.json which handles plugin configurations will be in the same location i.e. in cmd/bytengine.

egon

unread,
Nov 25, 2014, 1:23:53 PM11/25/14
to golan...@googlegroups.com


On Tuesday, 25 November 2014 20:11:56 UTC+2, John Wilson wrote:
Right, that's not a bad idea at all. Blimey you're quick! 
So if I undrstand you clearly, this will mean that config.go which imports plugins and config.json which handles plugin configurations will be in the same location i.e. in cmd/bytengine.

Yes..

John Wilson

unread,
Nov 27, 2014, 12:23:01 PM11/27/14
to golan...@googlegroups.com
Update:

  1. 'bshell' moved to bytengine/cmd/bshell (https://github.com/johnwilson/bytengine/tree/master/cmd/bshell) and created a bytengine client type (https://github.com/johnwilson/bytengine/blob/master/client.go)
  2. renamed bytengine/core.go to bytengine/engine.go and removed all channels/async code (https://github.com/johnwilson/bytengine/blob/master/engine.go)
  3. moved auth.go, statestore.go, bytestore.go to root directory (github.com/johnwilson/bytengine)
  4. updated plugin registration functions (https://github.com/johnwilson/bytengine/blob/master/plugin.go#L13)
  5. made datafilter (bytengine/datafilter) and command handlers (bytengine/cmdhandler) as plugins. datafilter functions have been rewritten to be simple functions (https://github.com/johnwilson/bytengine/blob/master/router.go)
  6. updated config.json (https://github.com/johnwilson/bytengine/blob/master/cmd/bytengine/config.json) to include plugin names making the loadable without recompilation (still a bit of work left there)
  7. Updated plugins and command handlers to return (Response, error) so they're more idiomatic. This has actually been advantageous because it allows me to return an api client friendly error and a more detailed error for logging/debugging (still some work to be done)
There's still a bit of refactoring to do I'm sure so i'll keep at it.
In the meantime i'm awaiting more precious feedback from everyone and thanks for the support.

Cheers John 


On Tuesday, 25 November 2014 17:31:18 UTC, egon wrote:

egon

unread,
Nov 27, 2014, 4:30:03 PM11/27/14
to golan...@googlegroups.com
Don't use all caps constants. And use full names for public API.

ModeRoot, ModeGuest, ModeUser, DefaultPasswordCost

bytengine.BytengineClient -> bytengine.Client

Also I would consider separating the client into a separate package. 
(e.g. bytengine/client.Client)

BFS -> FileSystem

Was there still a reason to use the Response type?
Removing that Status/Error packing would remove a lot of bytengine.ErrorResponse... code.

Not sure whether the Plugin suffix adds any info. I would simply use:

type Engine struct {
Authentication Authentication
FileSystem     FileSystem
ByteStore      ByteStore
StateStore     StateStore
}


You should be able to remove the dependency on simplejson with something like:

Also it would simplify the setup code.

And change the constructors to:
func createStateManager(state string, config []byte) (StateStore, error)

PS. also, try not to panic inside a package. Panic only if explicitly request (Must* methods) from outside or if it violates some constraint mentioned in the docs... return an error if the fault is in a configuration. Keeping the panics to the minimum means you can much more easily detect real problems.

Not sure whether such utility functions should be in the public API.?
Is there a direct dependency in the engine code to those? Or just useful utilities?

Moving those utilities to the auth, filesystem package might make sense.


I would each of those register/new functions near their interfaces. It feels more natural point where the registration should exist. This would also remove the plugin.go file.

The Command should be probably part of bytengine not dsl.
Although if the parser is part of the engine, then you would make that into an interface similarly to auth, bytestore etc.

I would probably try to write the commands as:
cmd := bytengine.Command{
Name: "directaccess",
Args: bytengine.Args{
"database": db,
"path": path,
"layer": layer,
"writer": writer,
},
IsAdmin: true,
}
And remove the dsl.NewCommand altogether

As usual... keep up the good work.

+ Egon

John Wilson

unread,
Dec 1, 2014, 1:50:39 PM12/1/14
to golan...@googlegroups.com
So this update took a bit longer ( a few things pending) but here's what has been done so far:
  1. Constants all caps changed
  2. Bytengine API Client is in its own package now
  3. References to 'BFS' have been changed to 'FileSystem'
  4. Bytengine error response has been removed: https://github.com/johnwilson/bytengine/blob/master/filesystem.go#L10 (although i added response functions in the cmd/bytengine/bytengine.go )
  5. 'Plugin' suffix removed https://github.com/johnwilson/bytengine/blob/master/engine.go#L27-L33
  6. Simplejson dependancy removed (thanks for the json.RawMessage tip/example)
  7. Removed panic in packages but I couldn't figure out a neat way of throwing errors in package init functions (I'm using log.Fatal at the moment but I'm sure there's a better solution to this) for example https://github.com/johnwilson/bytengine/blob/master/filesystem.go#L40 and https://github.com/johnwilson/bytengine/blob/master/filesystem/mongo/mongo.go#L1453 . If I return an error in the RegisterFileSystem function does the init function just log the error or is there a better way to handle it?
  8. Added utils.go to auth, bytestore and filesystem packages
  9. Moved plugin registration and creation functions to same files as their interface specification.
  10. Parser is now a plugin and is implemented here bytengine/parser/base
Still need to review quite a few sections of the code though, but do let me know if you spot omissions/mistakes that I might have made. And thanks once again for the feedback.

John Wilson

unread,
Dec 1, 2014, 1:56:25 PM12/1/14
to golan...@googlegroups.com
Actually I'm beginning to realise that I'm rushing to implement changes instead of focusing on keeping the code clean and well commented so i'll be spending some time cleaning things up a bit. But as I mentioned before please don't hesitate to point out any shortcomings.
cheers

egon

unread,
Dec 2, 2014, 2:24:54 AM12/2/14
to golan...@googlegroups.com


On Monday, 1 December 2014 20:50:39 UTC+2, John Wilson wrote:
So this update took a bit longer ( a few things pending) but here's what has been done so far:
  1. Constants all caps changed
  2. Bytengine API Client is in its own package now
  3. References to 'BFS' have been changed to 'FileSystem'
  4. Bytengine error response has been removed: https://github.com/johnwilson/bytengine/blob/master/filesystem.go#L10 (although i added response functions in the cmd/bytengine/bytengine.go )
  5. 'Plugin' suffix removed https://github.com/johnwilson/bytengine/blob/master/engine.go#L27-L33
  6. Simplejson dependancy removed (thanks for the json.RawMessage tip/example)
  7. Removed panic in packages but I couldn't figure out a neat way of throwing errors in package init functions (I'm using log.Fatal at the moment but I'm sure there's a better solution to this) for example https://github.com/johnwilson/bytengine/blob/master/filesystem.go#L40 and https://github.com/johnwilson/bytengine/blob/master/filesystem/mongo/mongo.go#L1453 . If I return an error in the RegisterFileSystem function does the init function just log the error or is there a better way to handle it?
Actually I would panic there, because it's an obvious programmer error.

Basically -
if the end-user is likely to make the mistake -> return/log an error
if the programmer is likely to make the mistake -> panic
  1. Added utils.go to auth, bytestore and filesystem packages
  2. Moved plugin registration and creation functions to same files as their interface specification.
  3. Parser is now a plugin and is implemented here bytengine/parser/base
Still need to review quite a few sections of the code though, but do let me know if you spot omissions/mistakes that I might have made. And thanks once again for the feedback.

Basically, after going over the code I failed to notice anything obvious. So I don't have more recommendations :).

+ Egon

John Wilson

unread,
Dec 2, 2014, 3:43:35 AM12/2/14
to golan...@googlegroups.com
Thanks for the awesome feedback! This has been a great experience.
I'll be making more updates/improvements to Bytengine (and do my best not to undo all the good work we've done) so please do take a look a the code every so often to make recommendations.

Oh and don't forget to actually play with Bytengine :-)

Cheers

John Wilson

unread,
Jan 1, 2015, 11:32:13 AM1/1/15
to golan...@googlegroups.com
Hello all.

You can now deploy bytengine to heroku for testing/playing: https://github.com/johnwilson/hemiv6

Cheers and happy new year!

On Friday, 21 November 2014 15:33:27 UTC, John Wilson wrote:
Reply all
Reply to author
Forward
0 new messages