Web app that splits a URL graphically

Question

I wrote this web app that splits a URL graphically so that its parts can be customized to create a new URL. https://github.com/ibrahim-islam/urlpad. The app currently works as expected for usual URls but I am not so sure about the code structure.

module UrlPad {
 const map = (o: OmniboxFull): CustomOmniboxMap => {
 return {
 protocol: o.protocol,
 host: o.host,
 port: o.port,
 pathnames: o.pathname.split('/').filter(char => char),
 hash: o.hash,
 query: o.query
 };
 }
 class FormBuilder {
 private template: string[] = [];
 private for: string = '';
 constructor(forClass?: string) {
 this.for = forClass;
 this.template.push('<div class="form-group">');
 }
 label(title: string) {
 this.template.push(`<label class="col-sm-2">${title}</label>`);
 return this;
 }
 input(value: string, id?: string, size: number = 10) {
 let id_att = id ? 'id="'+ id +'"' : '';
 this.template.push(`<div class="col-sm-${size}">
 <input ${id_att} spellcheck="false" type="text" class="form-control ${this.for}" value="${value}"></div>`);
 return this;
 }
 build() {
 this.template.push('</div>')
 return this.template.join('');
 }
 }
 export function parse(url: string): string {
 let o = map(omnibox.parse(url));
 return Object.keys(o)
 .filter((key, index) => o[key])
 .map((key, index) => {
 let value = o[key];
 switch (key) {
 case 'query':
 return Object.keys(value)
 .map((key, index) => new FormBuilder('query').label(`Querystring #${index}`).input(key, '', 5).input(value[key], '', 5).build())
 .join('');
 case 'pathnames':
 return value
 .map((value, index) => new FormBuilder('path').label(`Pathname #${index}`).input(value).build())
 .join('');
 default:
 return new FormBuilder().label(key).input(value, key).build();
 }
 })
 .join('');
 }
 export function make(o: CustomOmniboxMap): string {
 let querystrings = Object.keys(o.query)
 .map((key, index) => `${key}=${o.query[key]}`)
 .join('&'),
 paths = o.pathnames.join('/');
 return `${o.protocol}://${o.host}${o.port ? ':' + o.port : ''}/${paths}${querystrings ? '?' + querystrings : ''}${o.hash ? o.hash : ''}`;
 }
}
window["UrlPad"] = UrlPad;

1. I made the app related functionality into a module, should it have been a class?

2. map function is only used inside parse function, should it have been a local function?

3. When testing this app should I test only the exported functions or any class or function I wrote as well?

Any other observations or suggestions to improve the code is also welcomed.

• eckes: BTW: your app does not work if I hit enter instead of pressing the parse button.
• eckes: I would name the map() function parseURL().
• lbrahim: @eckes thanks for taking a look. I will rename it and yes hitting enter does not work. My current priority is to structure the code as best possible then I will work on validation and other stuff.
• eckes: Actually I have to revert it,seems like Omnibox does the parsing, in that case urlToMap() might be better. Sorry can't comment on the structure, typescript is not my domain. However I am thinking that the module is reasonable small so it might not be that important. Why is make() exported? parse() is named like an utility function but it looks more like an view controller - not sure if this is normal with typescript/omnibox.
• lbrahim: @eckes make() is exported because it will be invoked from UI. If you browse index.html it will make sense. I did not understand what you meant by "like view controller". Please elaborate.

Answer 1

For the most part, this looks good. However, there are a few things I'd recommend changing.

1. Don't use module. namespace should be used instead. From the Typescript Handbook:

   A note about terminology: It’s important to note that in TypeScript 1.5, the nomenclature has changed. "Internal modules" are now "namespaces". "External modules" are now simply "modules", as to align with ECMAScript 2015’s terminology, (namely that module X { is equivalent to the now-preferred namespace X {).

2. You do not properly escape special characters. If a double quote appears anywhere in the URL, the form will break. Try the url https://example.com/?a=" >Broke to see an example.

3. Related to the above, building HTML with strings is an incredibly error prone process. If at all possible, avoid it. It is a better idea to use the <template> tag.

4. You don't need omnibox browsers have a built in URL api which covers everything that omnibox does and more. If you need to support IE, you can use the url-polyfill package - and gain the advantage that newer browsers don't need to load it.

5. Consider using Object.entries instead of Object.keys as it can slightly simplify some of your code.

   const querystrings = Object.entries(o.query)
    .map(([key, value]) => `${key}=${value}`)
    .join('&')
   

For your specific questions:

1. A (削除) module (削除ここまで) namespace is a reasonable choice for the app. I personally never use them, preferring to use ES Modules. A larger concern is that you export the module to the global scope unnecessarily. Instead of using onclick="parse()" in your HTML, it would be better to add the event handler in your index.ts file and have this module in it's own file.

2. I don't believe you even need the map function, it wouldn't be that big of a deal to check for pathname instead of pathnames in the switch statement and split the string there. If you do keep the map function, it should be renamed to something more descriptive. When I saw map I expected it to be roughly equivalent to Array.prototype.map.

3. For tests, it depends on who you ask. Since you aren't parsing the URL yourself (good!) there isn't really too much that can go wrong here. I'd probably only write a few integration tests for this app as a whole.