3
\$\begingroup\$

I took upon myself to try to do a OOP design for Waze, please give a feedback which is OOP design related. Also if you were the interviewer can you think of flow up questions?

using System;
using Microsoft.VisualStudio.TestTools.UnitTesting;
using System.Collections.Generic;
using System.Drawing;

namespace JobInterviewTests
{
    [TestClass]
    public class WazeDesign
    {
        [TestMethod]
        public void CarTest()
        {
            Car myCar = new Car();
            UserMap mapByLocation = new UserMap(myCar.CurrentLocation);
            mapByLocation.SetDestination(104410);
            Assert.AreEqual(System.Drawing.Color.FromName("Purple"), mapByLocation.SuggestedRoute.RoadColor);
        }
    }

    // different types, motorcycle, bus, private car....
    public abstract class Vehicale
    {
        public int CurrentLocation { get; private set; }
        public System.Drawing.Color RoadColor { get; set; }
        public Vehicale()
        {
            GetLocation();
        }
        public void GetLocation()
        {
            CurrentLocation = Gps.Instance.GetLocation();
        }
    }

    public class Car : Vehicale
    {
        public Car()
        {

        }
    }

    //should be a singletone, this is not a must.
    //but looks reasonable
    public class Gps
    {
        private static Gps _instance = null;
        private Random random = null;
        private Gps()
        {
            random = new Random();
        }
        public static Gps Instance
        {
            get
            {
                if (_instance == null)
                {
                    _instance = new Gps();
                }
                return _instance;
            }
        }
        public int GetLocation()
        {
            int result = -1;
            try
            {
                result = random.Next(1000000);
            }
            catch (Exception ex)
            {
                throw new Exception("there was an error locating GPS info");
            }
            return result;
        }
    }

    public class Road
    {
        //a road can contain all the cars you are passing by.
        public List<Vehicale> Cars { get; private set; }
        public System.Drawing.Color RoadColor { get; set; }
        public Road()
        {
            Cars = new List<Vehicale>();
        }
    }

    public class SuggestedRoute : Road
    {
        public SuggestedRoute()
        {
            RoadColor = System.Drawing.Color.FromName("Purple");
        }
    }


    //the map contains all the roads nearby and the suggested Route
    //Day and night colors
    public class UserMap
    {
        public int CurrentLocation { get; set; }
        public int Destination { get; set; }
        public List<Road> Roads { get; set; }
        public List<Vehicale> Vechicals { get; set; }

        public SuggestedRoute SuggestedRoute { get; set; }

        //first time you build the map you add all the car and roads near you
        public UserMap(int currentLocation)
        {
            CurrentLocation = currentLocation;
            Roads = DataServer.Instance.GetRoadsAroundLocation(CurrentLocation);
            Vechicals = DataServer.Instance.GetVechicalesAroundLocation(CurrentLocation);
        }

        //you need to update the view, you do not need to rebuild everything
        public void UpdateLocation(int currentLocation)
        {
            //update from GPS every 2 secs...
        }

        public void SetDestination(int destination)
        {
            Destination = destination;
            SuggestedRoute = DataServer.Instance.GetSuggestedRoute(CurrentLocation, destination);
        }
    }

    public class DataServer
    {
        private static DataServer _instance = null;
        private DataServer() { }
        public static DataServer Instance
        {
            get
            {
                if (_instance == null)
                {
                    _instance = new DataServer();
                }
                return _instance;
            }
        }

        public List<Road> GetRoadsAroundLocation(int currentLocation)
        {
            return new List<Road>();
        }

        public List<Vehicale> GetVechicalesAroundLocation(int currentLocation)
        {
            return new List<Vehicale>();
        }

        public SuggestedRoute GetSuggestedRoute(int currentLocation, int destination)
        {
            return new SuggestedRoute();
        }
    }
}
\$\endgroup\$
  • 2
    \$\begingroup\$ Making everything a singleton is easy, designing software using proper DI and/or IoC is much harder. On the interview I would want to see the proof that you can do the latter. But that's me, your interviewer might disagree. \$\endgroup\$ – Nikita B Jun 2 '17 at 13:36
  • 2
    \$\begingroup\$ Also your code does not do anything. Even if you mock GPS data, I would want to see some basic algorithm for selecting a route. You always return "purple", and that's it. But maybe that wasn't the requirement. \$\endgroup\$ – Nikita B Jun 2 '17 at 13:42
2
\$\begingroup\$

Class Vehicale

  • RoadColor property. The colour of the road is not a property of a vehicle. Maybe you meant the vehicle's colour?
  • public void GetLocation() - a method that "gets" should return something. You're not "getting" anything, you're "updating" the location.
  • CurrentLocation = Gps.Instance.GetLocation(); - hard coded dependency in your abstract Vehicle class on a GPS class? I don't think so! Vehicles exist without GPS. What's more, it's also explicitly linked to the current design of the GPS class, what with going through the Instance property to call a method on the object. What if you decide to move away from a singleton design?
  • Typo in the class name - should be Vehicle

Class Gps

  • Design: singleton is generally considered an anti-pattern. Google it for articles discussing pros and cons of this design.
  • GetLocation()'s try\catch (1): I cannot see a legitimate reason to expect this to potentially throw an exception. Can you? If not, don't try/catch.
  • GetLocation()'s try\catch (2): If you're catching an exception and throwing a new one, throw something useful. Throwing a generic Exception is not very helpful, and it's made worse by the fact you're actually hiding what went wrong (you swallowed the original exception, so whoever catch the one you throw will have no idea what actually caused it). Create a custom exception type (or use a more specific existing one if you want), and either set the InnerException property to the original exception, or at least get the exception message from the original exception before you throw a new one. In fact, at this low level, I would not try/catch it even if I thought it could reasonably throw an exception - that would be the job of the calling code, and it can then decide how to deal with it.

Class Road

  • public List<Vehicale> Cars You only have a list of cars? There are other vehicles too you know.

Also I don't feel this is the correct design here, you have thousands of Roads and each has a List of Vehicles? How do you keep track of moving a Vehicle from one Road to another? To be honest I'm not sure how I would do it differently. I don't think a Road should contain a list of Vehicles, I would maybe be slightly more inclined for each Vehicle to link to a Road, but I don't like that either (especially as a Vehicle doesn't have to be on a Road). Some sort of link between them, but not quite so direct as this. Maybe some separate class that keeps track of them.

Class SuggestedRoute

This is really more to do with the underlying class since the RoadColor property is inherited, but this one is where I saw its true purpose (I was initially thinking of a real road, it has a colour... usually grey). You're setting it to purple, obviously for the UI. Now if I squint I can kind of see some merit to this, because you have multiple classes inheriting from your base Road, and in good object oriented fashion you can just get the RoadColour of the supplied Road variable and it will be appropriate for the specific road type. Except I think the colour used to display a Suggested Route (or any other type of road you have) is not appropriate to have at the "model" level (which is here). It's not business logic, it's got nothing to do with the road model itself, it's purely a decision of how it's displayed on the UI (which can be subject to change), and who says it will always be used through a GUI anyway? So I think it's inappropriate to have UI logic down here.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ it took me a while but I finally got to reading your full review, thank you very much I have learned from it. I will do another version and change many things like you suggested. thank you \$\endgroup\$ – Gilad Jun 24 '17 at 12:02

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.