Please **review** my code, since I lack experience - mmm_velib, A module for bike sharing schemes in Europe

poutr

Hi,

I finished this small module and I would greatly appreciate if people with experience could review the code. It is based on default module architecture calendar and newsfeed.

It’s a live update of (chosen by users) bike sharing stations. Should cover quite a few cities too.

https://github.com/Poutrathor/mmm_velib

I think it is good right now but :

almost no experience on JS, html, css and node.
Not that objective on my own work.

I let the logs and comment to help with reviewing.
I hope the readme.md is clear enough.

Thank you very much for your time

PS : Should I post it now to showcase or should I wait someone has checked for issues before ?

strawberry 3.141

@poutr in thefetcher you still name the functions like news, would be better to name it something with bike
the module js has unused methods like capitalizeFirstLetter and getTranslations, you can remove them completely and you could shorten your switch case to something like:


this.stations = {
  PARIS: "Velib'",
  LYON: "Vélo'v",
  MULHOUSE: "VéloCité",
  BESANCON: "VéloCité",
  MARSEILLE: "Le vélo",
  TOULOUSE: "VélôToulouse",
  ROUEN: "Cy'clic",
  AMIENS: "Velib",
  NANTES: "Bicloo",
  NANCY: "vélOstan'lib",
  "CERGY-PONTOISE": "VélO2",
  "PLAINE-COMMUNE": "Velcom",
  CRETEIL: "Cristolib"
};


var marque = "Cyclocity";
var contractName = this.stationsData[0].contract_name.toUpperCase();
if(this.stations.hasOwnProperty(contractName)){
  marque = contractName;
}

poutr

Thanks,

Yup, I have to clean this up. I get your fancy code over the switch case, that’s nice writing, I like it.

Anything else I should change, double proof or something ?

yawns

@strawberry-3.141
I shouldn’t go out with the dog. The long switch block was my first spot too, but you beat me to it

poutr

Ok, I have done the code clean up.

I use this call below to get the property value and not the propetry name, e.g. to get “Vélib’” and not “PARIS”. Am I right ?

var contractName = this.stationsData[0].contract_name.toUpperCase();
if (this.contractList.hasOwnProperty(contractName)) {
	marque = this.contractList[contractName];
}

If there is no other issues spotted, that’s great

Stupid question : how does one update his 3rd party module from github ? I used git clone https://github.com/Name/repo to install it on my mirror, but I don’t get how to update locally when the master on github is updated. Fetch failed me (no commit in common or something). I deleted and clone again but that’s so ugly.

yawns

Enter your module folder:

git pull origin master

poutr

@yawns ty, i will try it !

Brice

Suggestions :

format header correctly (class name = module-header)
remove station number from name
Great module btw !

poutr

@Brice Thanks!

ok for the format, will do that.

I am uneasy about removing the number from the name :

It comes as it from the JCDecaux server. Any parsing from my part would be under risk from future modifications.
The number is the ID, it feels more precise than a ‘name’.

I would need more users’ feedback for this point but with a grand total of one user ‘me’, I think I will let it like this, don’t you agree?

cheers