From Social Cat, 9 Years ago, written in Plain Text.
Embed
  1. I'm going to be frank, this library needs some work. I'm really sorry for the way this is going to come across, but as I kept scrolling through the code, I couldn't just let it be... Don't take this as some sort of beat down; I don't intend to be mean. :)
  2. Big things:
  3. There is ZERO error handling. You really need to lint/vet/errcheck/etc this code. Look into gometalinter. You'll be surprised just how many things you've looked over. I can't count the number of times you've ignored errors, either by not capturing return values, or explicitly discarding them with _. You even have top-level helper functions to get rid of errors! And, if you do happen to have an error, you panic, which is not error handling. Error handling is a core pillar of writing good Go code.
  4. You use log.Panic all over the place. A well-written library should 1) not do logging, or at least make it default disabled, and 2) hopefully not be panicking randomly at runtime, especially when it's not documented. This goes along with the error handling. If something doesn't parse, you should not just be panicking out (like in those helper functions). ParseInt has an error that it returns, use it.
  5. Why do you use any sort of global state? It's unreasonable to assume that a user is only going to want to have a single database handle.
  6. Nit: Every function returns int. That's really odd. If someone wants the length of something, let them do it, or provide a length function for whatever it is you're returning the length on.
  7. This code is not safe for concurrent use. Even though you've protected the table with a mutex (though exported...), that Rec struct both exposes the raw map, and has functions that do not access the map safely.
  8. Where you do use a mutex (i.e. the table), it's still not perfect, as you're returning the length of some map, after unlocking. Mutex locks and unlocks are a great place for the runtime to steal control away from your goroutine, and then maybe run someone else's table call, thus changing that Rec's length out from under you. Consider using deferred unlocks.
  9. You have many different string variables that should be constants, like "#c" and "#deleted". DefaultKeySize is a var, but could be a const.
  10. What happens when a user wants to name their fields "#c" or "#deleted"? You panic. Bad idea. (Use case to consider: IRC bot, which joins a channel named "#deleted" and tries to access some data with your library. Oops, the program panics, and the user has no clue why.)
  11. Within Rec, actual values, and those "special" items (I'm assuming "dirty bit" variables) are in the same map. Why? If you need it, store it separately. #c appears to be a bool disguised as a string; maybe it should just be a member variable.
  12. You shuffle everything into strings, including []byte. Bolt's core datatype is byte slices. Why take a []byte, base64 encode it, then convert it into a string, then have it converted back into a []byte inside the database?
  13. In your tests, you discard errors when creating inputs. Even though it may just be a test, it's a much better idea to make something like mustParse for time (or others) so that you don't screw up when writing a test, and don't know why. The stdlib templating and regexp libraries use this pattern. It's a very frustrating thing to have to debug, even though it is rare.
  14. That Sequence type isn't going to work at all. Look into value/pointer function receivers.
  15. var validTypes = "strintfloatbytesdatedateTimebool" with strings.Index to check membership is exactly what a map should be used for.
  16. Overall style comments:
  17. All of your documentation are in your readme, when it should be with your code. Try to figure out your library from what godoc tells you. Hard, right?
  18. You use this as the name for all of your function receivers. Please, please, don't do that.
  19. Nit: It's weird to be using fmt in your tests. testing.T has logging functions.
  20. Nit: Generally, the "Go pattern" (maybe oversimplified) is to figure out ways to exit early from a function. Make sure that you're not doing things like an if statement that returns in the "then" part, but still write the else. Overall, you've done a good job with that.
  21. Why do you expose things like mutexes? A good interface never exposes members that you don't want users to touch. The same goes for many of your other struct members, and StartRead and StartWrite.
  22. Be more descriptive with your variable and type names. FldMap is not as good as FieldMap. Load1 could be LoadOne.
  23. There isn't any whitespace in your functions! Don't sacrifice readability to make the code shorter.
  24. Judging by your import statements, you're not using any tooling like goimports. This will not only improve readability (sorting stdlib to the top and external below is nice), but improve your experience. Go has great tooling; make use of it.
  25. Don't create more types than you need. For example, FldMap is just a map from strings to strings, and does not have any associated functions. It doesn't need to be its own type. map[string]string is much more clear. Same goes for type bs []byte (and that type even makes it seem like typing []byte was too annoying).
  26. const Shared = true and const NotShared = false are pretty silly. The struct member you use them for is already called "Shared"!
  27. Nit: There's a capital letter in your repo (and therefore import) path. Not really the norm.
  28. Setdb should be SetDB to follow naming conventions.
  29. Nit: Your functions on "int" are actually on "int64". The names would be better off matching the types.
  30. You export a bunch of helper functions. Your library isn't a string-to-x library, so unexport them and only export what a user would care about.
  31. This is the first time I've ever seen anyone use testing.M. You should really not be doing anything with that (as the docs say). It looks like you're using it for setup before other tests, but maybe that should instead be delegated to an init() function.
  32. Now that I've ruined your day... Some positive things:
  33. In general, you've done a good job keeping nesting low, and functions small.
  34. You wrote tests! Way too many people's first libraries don't include tests.
  35. There aren't any weird dependencies outside of the standard library, other than Bolt itself. Good.
  36. You made good use of variadic parameters to add optional values to your functions.
  37. No interface{} in your exported API. Yay static typing!
  38. For the most part, you kept types and their associated functions confined to their own files. Good practice.
  39. Overall, I'm really not sure of the usage pattern. The library uses a global database, but then exposes some functions that use parts of bolt. There isn't any error handling, so as a user, I have no way of knowing what's going on unless I start calling recover() in all of my functions, and then attempt to parse the string that was included with the panic. Using some in-memory copy of the data feels a little redundant, considering Bolt itself mem-maps things anyway for performance. I wrote a somewhat similar library about a year ago for a project (which I'm now revisiting and have totally reimagined), and I felt much more comfortable instead making the focus on interpreting a Bolt bucket as a type, then casting the bucket into the type with my functions defined on it (or, making a struct if more info is needed).
  40. I suggest looking at something like simplebolt. It's clearly not the same as what you do (and isn't perfect in itself, but I won't waste your time on that), but is much more easy to use, especially considering it satisfies the same interface as xyproto's other libraries, simpleredis and simplemaria. You might find it more useful for things that you end up doing.
  41. I'm not sure of your background, but looking at your Github, you've only written a little bit of Go code, mostly libraries. If that's truly all you've worked with, it may be a good idea to write some code that uses other people's libraries, then get a feel for what works and what doesn't.