• Recent
  • Tags
  • Unsolved
  • Solved
  • MagicMirror² Repository
  • Documentation
  • 3rd-Party-Modules
  • Donate
  • Discord
  • Register
  • Login
MagicMirror Forum
  • Recent
  • Tags
  • Unsolved
  • Solved
  • MagicMirror² Repository
  • Documentation
  • 3rd-Party-Modules
  • Donate
  • Discord
  • Register
  • Login
A New Chapter for MagicMirror: The Community Takes the Lead
Read the statement by Michael Teeuw here.

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

Scheduled Pinned Locked Moved Development
9 Posts 4 Posters 4.1k Views 4 Watching
Loading More Posts
  • Oldest to Newest
  • Newest to Oldest
  • Most Votes
Reply
  • Reply as topic
Log in to reply
This topic has been deleted. Only users with topic management privileges can see it.
  • S Offline
    strawberry 3.141 Project Sponsor Module Developer @poutr
    last edited by Feb 6, 2017, 1:00 PM

    @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;
    }
    
    

    Please create a github issue if you need help, so I can keep track

    Y 1 Reply Last reply Feb 6, 2017, 2:04 PM Reply Quote 1
    • P Offline
      poutr
      last edited by Feb 6, 2017, 1:36 PM

      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 ?

      1 Reply Last reply Reply Quote 0
      • Y Offline
        yawns Moderator @strawberry 3.141
        last edited by yawns Feb 6, 2017, 2:04 PM Feb 6, 2017, 2:04 PM

        @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 :)

        1 Reply Last reply Reply Quote 1
        • P Offline
          poutr
          last edited by Feb 6, 2017, 7:23 PM

          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.

          1 Reply Last reply Reply Quote 0
          • Y Offline
            yawns Moderator
            last edited by Feb 6, 2017, 7:25 PM

            Enter your module folder:

            git pull origin master
            
            P 1 Reply Last reply Feb 6, 2017, 8:08 PM Reply Quote 0
            • P Offline
              poutr @yawns
              last edited by Feb 6, 2017, 8:08 PM

              @yawns ty, i will try it !

              1 Reply Last reply Reply Quote 0
              • B Offline
                Brice
                last edited by Brice Feb 17, 2017, 2:02 AM Feb 17, 2017, 2:01 AM

                Suggestions :

                • format header correctly (class name = module-header)
                • remove station number from name
                  Great module btw !
                P 1 Reply Last reply Feb 21, 2017, 6:21 PM Reply Quote 0
                • P Offline
                  poutr @Brice
                  last edited by Feb 21, 2017, 6:21 PM

                  @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

                  1 Reply Last reply Reply Quote 0
                  • 1 / 1
                  • First post
                    Last post
                  Enjoying MagicMirror? Please consider a donation!
                  MagicMirror created by Michael Teeuw.
                  Forum managed by Sam, technical setup by Karsten.
                  This forum is using NodeBB as its core | Contributors
                  Contact | Privacy Policy