-
Notifications
You must be signed in to change notification settings - Fork 0
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
Console display refactoring #11
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall I'm not a super fan of a Channel
structure and I would rely on interfaces instead. However, it would require major changes, so I understand why you went that way.
(Might not be the best review possible, It's late and I'm in the train and tired 😅)
Really happy to talk to you about this one of those days. I'll try to come up with an example design if you want.
err := morserino.SetupLogger(morserinoDebugLevel, morserinoDebugFilename) | ||
if err != nil { | ||
fmt.Println("\nERROR: " + err.Error() + "\n") | ||
cmd.Help() | ||
os.Exit(1) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Style nit: you can inline this.
Also Println
:
- already adds a
/n
at the end of the line. - Joins all given arguments with a whitespace character.
err := morserino.SetupLogger(morserinoDebugLevel, morserinoDebugFilename) | |
if err != nil { | |
fmt.Println("\nERROR: " + err.Error() + "\n") | |
cmd.Help() | |
os.Exit(1) | |
} | |
if err := morserino.SetupLogger(morserinoDebugLevel, morserinoDebugFilename); err != nil { | |
fmt.Println("\nERROR:", err) | |
cmd.Help() | |
os.Exit(1) | |
} |
//Global variable with the debug level | ||
var morserinoDebugLevel string | ||
|
||
//Global variable with the name of the log file (if defined) | ||
var morserinoDebugFilename string | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- You can use the multi-line
var
syntax here, which allows to group symbols together, preferably semantically. - Also, I don't see why we need to repeat
morserino
here. It doesn't bring much information, does it ? - What does
debugLevel
means ? Shouldn't it belogLevel
? - Those variable names are pretty self descripting. The comments only repeat what is already stated by the variable. I suggest you remove them?
//Global variable with the debug level | |
var morserinoDebugLevel string | |
//Global variable with the name of the log file (if defined) | |
var morserinoDebugFilename string | |
var ( | |
//Global variable with the debug level | |
debugLevel string | |
//Global variable with the name of the log file (if defined) | |
debugFilename string | |
) |
"strings" | ||
) | ||
// Structure containing all the channels used by the application | ||
type MorserinoChannels struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Design nit: What is this structure role? What is its responsibility in regard of your application logic?
Usually we try to have one structure, one behavior. Here it seems that this structure is a technical factorization, not a semantic one which isn't the best.
Alsoooo, is repeating Morserino
really necesasry here? We're already under the morserino
package 😄
} | ||
// Channel indicating that all data has been displayed and that the | ||
// application can be closed | ||
DisplayCompleted chan bool |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DisplayCompleted chan bool | |
DisplayCompleted chan struct{} |
If you don't care about the actual data carried inside the channel, use chan struct{}
instead (literally chan of empty struct} which doesn't allocate any space for the data.
You can use it like this
c := make(chan struct{})
c <- struct{}{}
func (mc *MorserinoChannels) Init() { | ||
mc.MessageBuffer = make(chan string, 10) | ||
mc.DisplayCompleted = make(chan bool) | ||
mc.Done = make(chan bool) | ||
mc.Error = make(chan error, 10) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Prefer using Initialization function instead of Init
method: doing the later exposes you to the risk of manipullating sturctures in an ill state (and it makes your structure more complicated to manipulate from the caller).
var c MorserinoChannels
// c is in an ill state, if I call anything on it it will panic
c.Init()
// now we're good
VS
cs := NewMorserinoChannels()
// And we're good :tada:, no risk of panics here.
"strings" | ||
"testing/iotest" | ||
|
||
"github.com/on4kjm/morserino_display/pkg/morserino_console" | ||
// "github.com/rs/zerolog" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Careful with that commented line 😄
// Main listen function with display to the console | ||
func ConsoleListen(morserinoPortName string, genericEnumPorts comPortEnumerator) error { | ||
func OpenAndListen(morserinoPortName string, genericEnumPorts comPortEnumerator, channels *MorserinoChannels) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not in fond of this change: You're removing an error
return and instead push down a set of channels that makes the code more complicated to understand.
How about keeping your error return and run the OpenAndListen
in an anonymous function like this:
var errCh = make(chan error)
// ...
go func() {
if err := OpenAndListen(foo, bar); err != nil {
errCh <- err
}
}()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, most of your operations here are initialization
phase error (aka misconfigured application for example) , not properly runtime. Those operations should be handled synchronously in your main goroutine I believe.
Maybe leverage a struct here, WDYT ?
type Listenser struct {
}
// Initialization function for the initialization phase
func NewListener(cfg Config) (*Listener, error) {
// Initialize things that could fail synchronously
smth, err := initializeSomething(cfg);
if err != nil {
return nil, err
}
return &Listener{smth: smth}, nil
}
Another "smell" that makes me think this is your function name OpenAndListen
: you're doing two things in the same function that could be separated.
Does it make sense ?
AppLogger.Debug().Msg("EOF detectd") | ||
AppLogger.Trace().Msg("Sending EOF to the console displayer") | ||
channels.MessageBuffer <- "\nEOF" | ||
//sending the exit marker to the diplay goroutine |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of exposing your synchronization primitive, how about hiding this being an interface, something like.
type MessageWriter interface {
WriteMessage(msg string) error
}
And hide the "technicalities` of how you actually send a message behind this ?
It will give you much better readability and testabillity 😸
From the caller It will be much more easier:
_ = w.WriteMessage("\nEOF")
type Buffer struct { | ||
buffer bytes.Buffer | ||
mutex sync.Mutex | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having to introduce this is kind of a smell: You can have multiples goroutines writing concurently to your buffer, which in some cases can be OK but I suspect it is not in yours 😺.
Refactoring of the program