Code Monkey home page Code Monkey logo

fe26's People

Contributors

andresbott avatar

Watchers

 avatar  avatar

fe26's Issues

Code review

Code organization

I think the organization of the internal/fe26 package could be improved. The package does three things. First, it inspect the command line and the environment variables to collect the necessary configuration. Second, it creates a router based on that configuration. Third, it starts an HTTP server using that router. I would split these responsibilities in three separate packages.

Decouple the definition of the router from the HTTP server

Coupling the definition of the router and the launch of an HTTP server based on that handler prevents you from reusing that hander for different purposes or instantiate it for testing purposes. Fixing this is quite easy. Just move the call to http.ListenAndServe from fe26Router to Fe26. Of course, you need to change fe26Router in order to return a http.Handler.

func fe26Router() http.Handler {
	r := mux.NewRouter()
	// Configure the handlers...
	return r
}

func Fe26() {
	preStartChecks()

	r := fe26Router()

	http.Handle("/", r)

	if err := http.ListenAndServe(Config.ip+":"+strconv.Itoa(Config.port), nil); err != nil {
		log.Fatal(err)
	}
}

Decouple the configuration of the routers

Instead of creating a main router on the fly in fe26Router by reading the configuration directly from a global variable, a better approach would be to pass the router configuration when the router is created. That's relatively easy for fe26Router:

func fe26Router(cfg *Settings) http.Handler {
	r := mux.NewRouter()

	// Base path: i.e GET fe26 & fe26.json
	r.HandleFunc("/"+cfg.FeBase, ListFilesHTML).Methods("GET")
	r.HandleFunc("/"+cfg.FeBase+".json", ListFilesJson).Methods("GET")

	// Handle static files like css and js files
	staticFiles := packr.New("static", "../../web/static")
	r.PathPrefix("/static.file").Handler(http.StripPrefix("/static.file", http.FileServer(staticFiles))).Methods("GET")

	// if GET to / redirect to fe26
	r.HandleFunc("/", redirectToView).Methods("GET")

	// Handle static files that exist
	r.Handle("/{filePath:.*}", http.FileServer(Fe26Dir(cfg.docRoot))).Methods("GET")

	// Handle Post requests
	//r.HandleFunc("/"+config.FeBase, handlePost).Methods("POST")
	r.HandleFunc("/"+cfg.FeBase+".json", handlePost).Methods("POST")

	return r
}

Unluckily, this change should propagate to the rest of the handlers. One handler and some of the code used by the handlers still use Config directly. This should be decoupled little by little. A very simple example is the redirect handler. You could implement it by leveraging function closures:

func newRedirectHandler(newPath string) http.Handler {
	return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
		log.Info("REDIRECT: from: " + r.URL.Path + "to" + newPath)
		if q := r.URL.RawQuery; q != "" {
			newPath += "?" + q
		}
		w.Header().Set("Location", newPath)
		w.WriteHeader(StatusMovedPermanently)
	})
}

You would register this new handler like this:

r.Handle("/", newRedirectHandler(cfg.FeBase)).Methods("GET")

Make steps to test the /fe62 and /fe26.json routers

These two routers do very similar things, but they change the way they present data. There are some commonalities that can be exploited. First of all, they both have the same initial logic. Maybe you can encapsulate this into its own wrapper http.Handler. For example:

func newFileDataHandler(next http.Handler) http.Handler {
	return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
		d := r.URL.Query().Get("d")
		data, err := getFilesData(d)

		if err != nil {
			log.Warn("Wrong query parameter, redirecting to: " + data.RequestUrl)
			http.Redirect(w, r, Config.FeBase+"?d="+data.RequestUrl, 302)
			return
		}

		ctx := context.WithValue(r.Context(), "data", data)
		next.ServeHTTP(w, r.WithContext(ctx))
	})
}

With such a "filter" in place, you can focus about just the presentation logic in ListFilesHTML and ListFilesJson. I won't repeat everything, but ListFilesHTML should look something like that:

func ListFilesHTML(w http.ResponseWriter, r *http.Request) {
	data, ok := r.Context().Value("data").(listFileData)
	if !ok {
		panic("file data expected in the request context")
	}

	log.Info("LIST: " + data.RequestUrl)
}

When you register the handler, you need to wrap your handlers like this:

r.HandleFunc("/"+Config.FeBase, newFileDataHandler(http.HandlerFunc(ListFilesHTML))).Methods("GET")

I find that this solution has two benefits. First, it prevents you from repeating the same code in two different handlers. Second, it improves the testability of your code. With this improvement, you don't need a real filesystem to test ListFilesHTML. Instead, you can create an instance of listFileData, put it in the request context, and use the package httptest to test your handlers completely in memory. You can also go one step farther and isolate the new file data handler from file-system operations.

type fileDataReader func(string) (listFileData, error)

func newFileDataHandler(fdr fileDataReader, next http.Handler) http.Handler {
	return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
		data, err := fdr(r.URL.Query().Get("d"))

		if err != nil {
			log.Warn("Wrong query parameter, redirecting to: " + data.RequestUrl)
			http.Redirect(w, r, Config.FeBase+"?d="+data.RequestUrl, 302)
			return
		}

		ctx := context.WithValue(r.Context(), contextKeyFileData, data)
		next.ServeHTTP(w, r.WithContext(ctx))
	})
}

The registration for this handler ties all the pieces together.

r.HandleFunc("/"+Config.FeBase, newFileDataHandler(getFilesData, http.HandlerFunc(ListFilesHTML))).Methods("GET")

You can also unit test the new file data handler in isolation from the file system by creating a test implementaiton of fileDataReader to test every possible corner case.

Don't swallow errors

If a component can't handle errors, it should just bubble them up to the caller, optionally adding some additionally context. Logging an error must be a conscious choice to handle that error. I have to admit that I don't know all the internals of this project, but the choice to log errors in FileManager instead of passing them to the caller looks a bit suspicious.

For example, FileManager.ReadDir should return an error if it's not possible to read the directory, and NewFileManager should probably return that error up the chain. Eventually, an HTTP handler will have to deal with it, and I think that a handler the right place to either log it or show an error message to the user (or both).

The FileManager doesn't manage much

I think that the FileManager API could be simplified a bit. In order to use a FileManager, I have to create one by invoking NewFileManager passing both the URL and the document root. Once I have a FileManager, the only thing I can do is invoke ReadDir, which will just list the files in the directory identified by the URL provided at construction time. Moreover, the result of reading the content of a directory is stored in the fields of the FileManager itself.

In this case, I would prefer to either have a function, like this:

type DirContent struct {
	Files []File
	Dirs  []File
	Path  string
}

func ReadDir(root, url string) (*DirContent, error) {
	panic("not implemented")
}

or a type that encapsulates the context common to every call (e.g. the document root), like this:

type DirContent struct {
	Files []File
	Dirs  []File
	Path  string
}

type FileManager struct {
	root string
}

func NewFileManager(root string) *FileManager {
	return &FileManager{root}
}

func (m *FileManager) ReadDir(url string) (*DirContent, error) {
	panic("not implemented")
}

Since the document root comes from the configuration and is not supposed to change for the lifetime of the application, I would prefer the second option. With the second solution, you could create just an instance of a FileManager and reuse it in all the handlers that need it. In addition, it's easy to mock a FileManager for testing purposes and decouple functionality if you opt for the second solution.

Recommend Projects

  • React photo React

    A declarative, efficient, and flexible JavaScript library for building user interfaces.

  • Vue.js photo Vue.js

    ๐Ÿ–– Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.

  • Typescript photo Typescript

    TypeScript is a superset of JavaScript that compiles to clean JavaScript output.

  • TensorFlow photo TensorFlow

    An Open Source Machine Learning Framework for Everyone

  • Django photo Django

    The Web framework for perfectionists with deadlines.

  • D3 photo D3

    Bring data to life with SVG, Canvas and HTML. ๐Ÿ“Š๐Ÿ“ˆ๐ŸŽ‰

Recommend Topics

  • javascript

    JavaScript (JS) is a lightweight interpreted programming language with first-class functions.

  • web

    Some thing interesting about web. New door for the world.

  • server

    A server is a program made to process requests and deliver data to clients.

  • Machine learning

    Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.

  • Game

    Some thing interesting about game, make everyone happy.

Recommend Org

  • Facebook photo Facebook

    We are working to build community through open source technology. NB: members must have two-factor auth.

  • Microsoft photo Microsoft

    Open source projects and samples from Microsoft.

  • Google photo Google

    Google โค๏ธ Open Source for everyone.

  • D3 photo D3

    Data-Driven Documents codes.