Basic "bee game" labeled as messy Could you tell me how to make it quality code?

<?php
class Game {
    public function __construct() {
        session_start();
    }
    public function play(){
        return '<input type="button" value="Play" onclick="window.location = \'?play=1\'">';
    }
    public function start($msg=null) {
        unset($_SESSION['bees']);
        $bee = new Bee();
        $bee->setType('queen');
        $this->addBee($bee,1);
        $bee->setType('worker');
        $this->addBee($bee,5);
        $bee->setType('drone');
        $this->addBee($bee,8);
        if($msg)
            return $msg;
        else {
            var_dump($_SESSION['bees']); // debug
            return 'Game started <input type="button" value="Hit" onclick="window.location = \'?hit=1\'">';
        }
    }
    public function hit() {
        $msg = '';
        if($_SESSION['bees'])
            $rd = array_rand($_SESSION['bees'],1);
        else
            $this->start('Game will start again');
        $_SESSION['bees'][$rd]['lifespan'] = $_SESSION['bees'][$rd]['lifespan'] - $_SESSION['bees'][$rd]['hit'];
        if($_SESSION['bees'][$rd]['lifespan']<1) {
            $_SESSION['bees'][$rd]['bees'] -=1;
            $_SESSION['bees'][$rd]['lifespan'] = $_SESSION['bees'][$rd]['life'];
            switch($rd)
            {
                case 'queen': $msg .= $this->start('Game start again, the Queen is dead.'); break;
                default: $msg .= 'One \'' . $rd . '\' is gone';
            }
        }
        if($_SESSION['bees'][$rd]['bees']<1){
            unset($_SESSION['bees'][$rd]);
            $msg = 'The \'' . $rd . '\' team is gone';
        }
        var_dump($_SESSION['bees']); // debug
        return '<input type="button" value="Hit" onclick="window.location = \'?hit=1\'"> ' . $msg;
    }
    public function addBee(Bee $newbee, $number = 1) {
        $tipo = $newbee->getType();
        $bee = $newbee->get();
        $_SESSION['bees'][$tipo] = $bee;
        $_SESSION['bees'][$tipo]['bees'] = $number;
        $_SESSION['bees'][$tipo]['life'] = $newbee->getLifespan($tipo);
    }
}

class Bee
{
protected $_type = null;
protected $_types = array(
    'queen' => array(
    'hit' => 8,
    'lifespan' => 100
    ),
    'worker' => array(
    'hit'   =>  10,
    'lifespan' => 75
    ),
    'drone' => array(
    hit'    =>  12,
    'lifespan' => 50
    )
    );
    public function setHit(){}
    public function setLifespan(){}
    public function addNewType(){}
    protected function _set_hit(){}
    protected function _set_lifespan(){}
    protected function _add_new_type(){}
    public function getLifespan($type){
        if(array_key_exists($type,$this->_types))
            return $this->_types[$this->_type]['lifespan'];
        else
            throw new Exception('The Bee need to be a Queen, Worker or Drone');
    }
    public function getTypes() {
        return $this->_types;
    }
    public function getType() {
        return $this->_type;
    }
    public function setType($type) {
        if(array_key_exists($type,$this->_types))
            $this->_type = $type;
        else
            throw new Exception('The Bee need to be a Queen, Worker or Drone');
    }
    public function get() {
        return $this->_types[$this->_type];
    }
}
?>
<html>
<head>
<title>play Bee</title>
</head>
<body>
    <?php
        $game = new Game();
        if(isset($_GET['play']) && $_GET['play']) {
            echo $game->start();
        } else if(isset($_GET['hit']) && $_GET['hit']) {
            echo $game->hit();
        } else {
            echo $game->play();
        }
    ?>
</body>
</html>

One big issue with this code is that you've got a tight coupling between the business logic and the presentation.  For example, the method play() really jumps out:

public function play(){

    return '<input type="button" value="Play" onclick="window.location = \'?play=1\'">';

}

From its name I'd think that this method plays something (a game? A piece of music? A role in a play? Another issue is clear, unambiguous naming of classes, methods and variables).  I wouldn't expect it to output the HTML markup for a button with a JavaScript event attached to it.

This might not seem like a big deal, and in this particular case it probably isn't.  But ask yourself this, how much work would you need to do if whoever set you this project decided half way though that it should run through the command line instead of a web browser?

Ideally, your PHP classes should contain no presentation logic at all.  They should return data structures (arrays, scalar types, simple data transfer objects, etc) to the outside world, and then a separate process should take that information and render it to the user in a suitable presentation.  The classes that do the actual work (the business objects) should implement all the logic and rules needed to accomplish a task without any concern as to how the results of that task are presented.

This concept is known as separation of concerns.  A concern is a particular part of the software package you're developing which shouldn't be tightly bound (coupled in the lingo) to other parts of your package (sorry I can't really word it better than that).  Implementing the rules and maintaining the state of the game is one concern, getting user input and displaying output to the user is a different concern.  While there obviously has to be some coupling you should try to minimize the degree that one is bound to the other.  Additionally, it's generally considered preferable for the part of the program responsible for presentation (which will typically be unique to that program) to know something about the business objects than it is for the business objects (which may get used across a wide range of applications) to know something about the presentation.

A related issue is reliance on $_SESSION inside your class.  Again this has to do with separation of concerns.  The $_SESSION array and its associated methods (session_start (), etc) are a data storage mechanism.  Data storage is a separate concern from the business logic, so again you should try to separate the code that handles storing the game state out from the code that implements the business logic.  In the case of $_SESSION there's the additional problems of it being tightly coupled to HTTP and that the session_start () function can't be called once headers have been sent to the client.  Again, ask yourself how hard would it be to adapt your code to work in a command line or to use a database instead of the PHP session to store state?

I had to take this test a couple of days ago during an interview process. I wrote mine as procedural and paid the price (didn't get selected for the next stage).

Looking at your effort, I've spent a couple of hours on it and have adapted it. Here is my offering:

class bee
{
    private $type = NULL;
    private $qty = 0;
    private $life = 0;

    public static $available_types = array('queen' => array(
                                                            'hit'   =>  8,
                                                            'lifespan' => 100
                                                            ),
                                            'worker' => array(
                                                            'hit'   =>  10,
                                                            'lifespan' => 75
                                                            ),
                                            'drone' => array(
                                                            'hit'   =>  12,
                                                            'lifespan' => 50
                                                            ));

    public function __construct($type, $qty)
    {
        //create new bee set collection

        //does this bee type exist?
        if (array_key_exists($type, self::$available_types))
        {
            //set default properties
            $this->type = $type;
            $this->qty = $qty;
            $this->life = self::$available_types[$type]['lifespan'];
        }
        else
        {
            throw new Exception('The Bees need to be a Queen, Worker or a Drone');
        }

    }

    public function get_type()
    {
        return $this->type;
    }

    public function get_life()
    {
        return $this->life;
    }

    public function get_qty()
    {
        return $this->qty;
    }

}

class game
{
    public $message;

    public function __construct()
    {
        session_start();
    }

    public function reset_game($supress_message = FALSE)
    {
        //reset each type of bees
        $bee = new bee('queen', 1);
        $this->add_bees($bee);

        $bee = new bee('worker', 5);
        $this->add_bees($bee);

        $bee = new bee('drone', 8);
        $this->add_bees($bee);

        if (! $supress_message)
        {
            $this->message = 'Game has been reset. All bees have been respawned.';
        };
    }

    public function add_bees(bee $new_bee)
    {
         $_SESSION['bees'][$new_bee->get_type()] = array('qty' => $new_bee->get_qty(),
                                                        'life' => $new_bee->get_life(),
                                                        'hit_points' => bee::$available_types[$new_bee->get_type()]['hit']
                                                        );
    }

    public function hit()
    {
        $bees = (array) $_SESSION['bees'];

        //is the session object still valid?
        if ($bees)
        {
            //get random bee from session
            $seed = array_rand($bees, 1);

            //check if there are any bees of selected type actually left
            if ($bees[$seed]['life'] == 0 && $bees[$seed]['qty'] == 0)
            {
                //apply hit again
                $this->hit();
            };

            //deduct appropriate hit points from selected bee
            $bees[$seed]['life'] -= $bees[$seed]['hit_points'];

            //if this bee has no life left
            if ($bees[$seed]['life'] < 0)
            {
                //then reset its life to designated lifespan
                $bees[$seed]['life'] = bee::$available_types[$seed]['lifespan'];

                //reduce bee quantity by 1
                $bees[$seed]['qty'] -= 1;
            };

            //how many bees are remaining for this type
            if ($bees[$seed]['qty'] >0 )
            {
                //there is at least one bee still left
                $this->message =  $seed.' bee has been hit. There are '.$bees[$seed]['qty'].' bees left.';
            }
            else //no, either the queen is dead, or all workers are dead, or all drones are dead
            {
                    switch ($seed)
                    {
                        case 'queen': //the queen is dead, end game
                            $this->message =  'The queen is dead, therefore all bees are dead. The game has been reset';
                            $this->reset_game(TRUE);
                            return;
                            break;

                        default:
                            $bees[$seed]['life'] = 0;
                            $this->message =  $seed.' bee has been hit and killed. There are no more '.$seed.' bees left.';
                            break;
                    };
            };

            //update session data with changes
            $_SESSION['bees'][$seed] = $bees[$seed];

        }
        else //session no longer exists
        {
            $this->reset_game();
        }
    }

    public function show_view()
    {
        //set view data
        $view = new stdClass();
        $view->message = $this->message;
        $view->queen = $_SESSION['bees']['queen'];
        $view->worker = $_SESSION['bees']['worker'];
        $view->drone = $_SESSION['bees']['drone'];

        ?>
            <!DOCTYPE html>

Bee Test

<?=$view->message?>

                <form name="input" action="beetest_revised.php" method="post">
                    <input type="submit" name ="action" value="hit">
                </form>

Queen Bees

                <?=var_dump($view->queen)?>

Worker Bees

                <?=var_dump($view->worker)?>

Drone Bees

                <?=var_dump($view->drone)?>

        <?
    }

}

$controller = new game();

if ($_POST)
{
    $controller->hit();
    $controller->show_view();
}
else
{
    $controller->message = 'Game has been reset.';
    $controller->reset_game();
    $controller->show_view();
}


Attempt at the Most dynamic FizzBuzz possible in Java I was trying to come up with the most dynamic fizzbuzz answer I could.. and well, this is what I came up with:

public static void main(String[] args) {

    ultraFizzBuzz((Integer arg) -> {
        String retVal = "";
        if(arg % 3 == 0) { retVal += "fizz"; }
        if(arg % 5 == 0) { retVal += "buzz"; }
        return retVal;
    });
}

public static void ultraFizzBuzz(modConditions<Integer> tester) {

    for(int i = 1; i < 100; i++) {
        String result = tester.test(i);
        if(result != "")
            System.out.println(tester.test(i));
        else
            System.out.println(i);
    }
}

In here the modConditions is an interface with a method (test) that returns type String.

What I was basically trying to do is create a super dynamic function that lets you add as many fizzes and buzzes and pops as you wanted.. but I think in the process accidentally made it so you end up writing 90% of the code in a lambda statement everytime you want to call it.

Am I overcomplicating things? Would this be considered bad programming practice?

The most important problem I have with your code is, that it totally fails to achieve the goal you set yourself. This is not dynamic at all. Basically, you take the "normal" method body and put it into a lambda, but this lambda now is a complex function, which contains all of the business logic.

Dynamically, you'd need a collection (probably list for being ordered) of lambdas, which each check a single input value and return an appropriate string representation (i.e. "fizz" or "buzz" or "whatever" or the empty string) and a method to apply this complete list on a single input, accumulate the results and perform the final check whether to output that string or the integer itself.

Thus, start out with something like:

Function<Integer, String> fizz = i -> i % 3 == 0 ? "fizz" : "";
Function<Integer, String> buzz = i -> i % 5 == 0 ? "buzz" : "";
Function<Integer, String> barz = i -> i % 7 == 0 ? "barz" : "";
List<Function<Integer, String>> fizzBuzzList = Arrays.asList(fizz, buzz, barz);

And try to come up with code that operates on such a list. Note: Function is the standard function type with an integer as an input and a string as the output. No need to define your own interface.

If you have achieved this, you can easily add another function to the list to check another value, and then you have achived something that might be considered dynamic.

I would probably create the following methods:

public static String singleUltraFizzBuzz(SortedMap<Integer, String> fizzBuzzes, int i) {
    StringBuilder result = new StringBuilder();
    bool isEmpty = true;

    for(Map.Entry<String, Object> entry : fizzBuzzes.entrySet()) {
        if(i % entry.getKey() == 0) {
            result.append(entry.getValue());
            isEmpty = false;
        }
    }

    return isEmpty ? i.toString() : result.toString();
}

public static String ultraFizzBuzz(SortedMap<Integer, String> fizzBuzzes, int min, int max) {
    String out = "";

    for(int i = min; i < max; i++) {
        out = out + singleUltraFizzBuzz(fizzBuzzes, i) + "\n";
    }
}

which would be called as follows:

SortedMap<Integer, String> fizzBuzzes = new TreeMap<>();
fizzBuzzes.put(3, "fizz");
fizzBuzzes.put(5, "buzz");
System.out.write(ultraFizzBuzz(fizzBuzzes, 1, 100));


Count unique digits in a given number How do I find the number of unique digits in a given number.

e.g
if 'input1' is 252 it must return 1, since 5 is the non-repeating digit;
if 'input1' is 25000 it must return 2, since digits 2 and 5 are the only non-repeating ones.

Is there a better way to solve this problem?

// COUNT UNIQUE DIGITS

int nonRepeatDigitsCount(int input1){
int i=0,j,k;
int n=input1;
int count=0,check,c2=0;

// FINDING NUMBER OF DIGITS ON THE GIVEN NUMBER
while(n>0){

    n=n/10;
    c2++;
    }
//CREATING ARRAY OF THE 'c2' SIZE
int arr[c2];
n=input1;
//STORING INDIVIDUAL DIGITS IN THE ARRAY
while(n>0)
{
    arr[i]=n%10;
    n=n/10;
    i++;

}
// CONDITION TO FIND NON REPEATING DIGITS
for(j=0;j<c2;j++)
{
    check=0;

    for(k=0;k<c2;k++)
    {
          if(j!=k)
          {
           if(arr[j]==arr[k])
           {
            check++;
           }
          }

    }

    if(check==0)
     {
        count++;
     }
 }

return count;
}

This is needlessly complicated and inefficient. Rather than making an array of all digits in the array, you should make an array of counted occurrences. Since a digit can have a value 0 to 9, this array size will be 10. For each occurrence, increase the value of the index in the array corresponding to the digit by 1.

This also means that you can count non-repeating characters on the fly while iterating through the number. When the number of occurrences is exactly 1, it should be counted. If more than that, it shouldn't.

This makes the code behave the same no matter the number of digits used as input.

Example:

#include <stdio.h>

int non_repeating (int val)
{
  if(val == 0) // special case, value 0 gives 1 digit
  {
    return 1;
  }

  int digit_count[10]={0};
  int non_rep=0;

  for(; val!=0; val/=10)
  {
    int i = val%10;
    digit_count[i]++;

    if(digit_count[i]==1)
    {
      non_rep++;
    }
    else if(digit_count[i]==2)
    {
      non_rep--;
    }
  }

  return non_rep;
}

int main (void)
{
  printf("%d %d\n", 252,        non_repeating(252));
  printf("%d %d\n", 25000,      non_repeating(25000));
  printf("%d %d\n", 1234567890, non_repeating(1234567890));
  printf("%d %d\n", 0,          non_repeating(0));
}

Output:

252 1
25000 2
1234567890 10
0 1

if input=n then the java code is,
int a[]=new int[10];
int rem;
int count=0;

while(n>0)
{
rem=n%10;
a[rem]=++a[rem]
n=n/10;
}
 for(int i=0;i<10;i++)
{
if(a[i]==1)
 count++;
}
return count;   // is the output


Assigning properties based on string parameters I am using the below method to assign value to my object(Team.Driver/Team.Codriver). This works fine but just wanted to know if I could improve this code via LINQ or other options.

public void MoveDriverCodriver(
    Team source, string sourceType, Team target, string targetType)
{
    if (sourceType == "Driver")
    {
        if (targetType == "Driver")
        {
            target.Driver = source.Driver;
        }
        else
        {
            target.Codriver = source.Driver;
        }
    }
    else
    {
        if (targetType == "Driver")
        {
            target.Driver = source.Codriver;
        }
        else
        {
            target.Codriver = source.Codriver;
        }
    }
}

Also second part is I want to update target Driver/Codriver details with the appropriate source. But I want to retain the id's of target Driver/Codriver.

class Driver
{
  public Guid Id { get; set; }
  public string Firstname { get; set; }
  public string Lastname { get; set; }
  public int Age { get; set; }
}

class Team
{
  public Guid Id { get; set; }
  public Driver Driver { get; set; }
  public Driver Codriver { get; set; }
}

calling move operation.

Team team1 = new Team(); //Do some operation on team1
Team team2 = new Team(); //Do some operation on team2
MoveDriverCodriver(team1, "Driver", team2, "Codriver"); //Here I am trying to assign team1's driver to team2's codriver.

Making the assumption you can't just use team1.Driver = team2.Codriver because you don't know the name of the properties first hand, I would build an even more generic function where it can be used with any type.

Also, I wouldn't name it Move because there is no move involved since the source still retains its values,

That said, my approach would be:

public void CopyProperty<T>(T source, string sourcePropertyName, T target, string targetPropertyName)
{
    if (source == null)
    {
        throw new ArgumentNullException(nameof(source));
    }

    if (target == null)
    {
        throw new ArgumentNullException(nameof(target));
    }

    if (string.IsNullOrWhiteSpace(sourcePropertyName))
    {
        ThrowInvalidParameterNameException(nameof(sourcePropertyName));
    }

    if (targetPropertyName == null)
    {
        ThrowInvalidParameterNameException(nameof(targetPropertyName));
    }

    Type elementType = typeof(T);
    PropertyInfo sourceProperty = elementType.GetProperty(sourcePropertyName);
    if (sourceProperty == null)
    {
        ThrowPropertyDoesNotExistException(sourcePropertyName, nameof(sourcePropertyName), elementType.Name);
    }

    if (!sourceProperty.CanRead)
    {
        ThrowPropertyAccessException(sourcePropertyName, PropertyAccessType.Read);
    }

    PropertyInfo targetProperty = elementType.GetProperty(targetPropertyName);
    if (targetProperty == null)
    {
        ThrowPropertyDoesNotExistException(targetPropertyName, nameof(targetPropertyName), elementType.Name);
    }

    if (!targetProperty.CanWrite)
    {
        ThrowPropertyAccessException(targetPropertyName, PropertyAccessType.Write);
    }

    if (sourceProperty.PropertyType != targetProperty.PropertyType)
    {
        throw new Exception(string.Concat("Property ", sourcePropertyName, " is not the same type as Property ", targetPropertyName));
    }

    targetProperty.SetValue(target, sourceProperty.GetValue(source, Type.EmptyTypes), Type.EmptyTypes);
}

private void ThrowInvalidParameterNameException(string parameterName)
{
    throw new ArgumentException("Invalid parameter name ", parameterName);
}

private void ThrowPropertyDoesNotExistException(string propertyParameter, string propertyParameterName, string typeName)
{
    throw new ArgumentException(string.Concat("Property ", propertyParameter, " does not exist in type ", typeName, " "), propertyParameterName);
}

private enum PropertyAccessType
{
    Read,
    Write
}

private void ThrowPropertyAccessException(string propertyParameter, PropertyAccessType accessType)
{
    throw new AccessViolationException(string.Concat("Property: ", propertyParameter, ", Access type: ", accessType));
}

With this you would get a safe and generic method to copy properties where you can use with any type and don't know which property is going to be copied (does not account for indexed properties).

You can use it calling:

CopyProperty(team1, "Driver", team2, "Codriver");

There is no collections to query for (unless you are searching for the right property) so I don't see how you can use linq.

This however has more if's than your first attempt.

Following is the change that I have done to simplify:-

public void MoveDriverCodriver(Team source, string sourceType, Team target, string targetType)
{
    Move(sourceType == "Driver" ? source.Driver : source.Codriver, targetType == "Driver" ? target.Driver : target.Codriver);
}

public void Move(Driver source, Driver target)
{
    source = target;
}


Join List with Separator I was implementing something similar to Python's join function, where

join([a1, a2, ..., aN], separator :: String)

returns

str(a1) + separator + str(a2) + separator + ... + str(aN)

e.g.,

join([1, 2, 3], '+') == '1+2+3'

I was implementing something similar and was wondering, what is a good pattern to do this? Because there is the issue of only adding the separator if it is not the last element

def join(l, sep):
    out_str = ''
    for i, el in enumerate(l):
        out_str += '{}{}'.format(el, sep)
    return out_str[:-len(sep)]

I'm quite happy with this, but is there a canoncial approach?

Strings in Python are immutable, and so 'string a' + 'string b' has to make a third string to combine them. Say you want to clone a string, by adding each item to the string will get \$O(n^2)\$ time, as opposed to \$O(n)\$ as you would get if it were a list.

And so, the best way to join an iterable by a separator is to use str.join.

>>> ','.join('abcdef')
'a,b,c,d,e,f'

If you want to do this manually, then I'd accept the \$O(n^2)\$ performance, and write something easy to understand. One way to do this is to take the first item, and add a separator and an item every time after, such as:

def join(iterator, seperator):
    it = map(str, iterator)
    seperator = str(seperator)
    string = next(it, '')
    for s in it:
        string += seperator + s
    return string

As join is already a Python built in function, it is recommended not to create a function identically named. I think will be a good idea to rename your function to exclude possible conflicts.


Count the number of unique elements in a sorted array Write a method named numUnique that accepts a sorted array of integers as a parameter and that returns the number of unique values in the array. The array is guaranteed to be in sorted order, which means that duplicates will be grouped together.

I'm mainly concerned with following proper conventions and maximizing readability; both of which I feel I know little about. In the last question I posted, I learned a bit about structuring if-statements. How can I think about these statements so that it's easier for me to write them, as well as easier for others to read them? Is there a sort of PEMDAS of programming logic that would help me understand writing conditional code with precedence? Any feedback is appreciated!

public static int numUnique(int[] array){
    if(array.length > 0){
        int countUnique = 1;
        for(int i = 0; i < array.length - 1; i++){
            if(array[i] != array[i+1]){
                countUnique++;
            }
        }
        return countUnique;
    } else {
        return 0;
    }
}

Just a few minor suggestions:

If you invert the condition on the length, then you can reduce the level of nesting, which is slightly easier to read
An array will never have a negative length. It's pure paranoia to check for array.length <= 0, I suggest to replace with array.length == 0
The spacing is not perfect. Use an IDE to reformat the code
This maybe a matter of taste, but I would rename countUnique to simply count, and the method from numUnique to countUnique

Putting it together:

public static int countUnique(int[] array) {
    if (array.length == 0) {
        return 0;
    }
    int count = 1;
    for (int i = 0; i < array.length - 1; i++) {
        if (array[i] != array[i + 1]) {
            count++;
        }
    }
    return count;
}

Solution 1

Maybe for readability a recursive function is a proper solution in this case. But think about it carefully because it will strain the stack.

public static int getUniqueValueCount(int[] array) {
    if (array.length == 0) {
        return 0;
    } else if (array.length == 1) {
        return 1;
    } else {
        return getUniqueValueCount(Arrays.copyOfRange(array, 1, array.length))
                + ((array[0] == array[1]) ? 0 : 1);
    }
}

Pros

getUniqueValueCount instead of numUnique --> more expressive name
no for-int-i iteration --> less control structures
no state involved, nearly functional
positive formulation of main condition

Cons

strains the stack
additional array operations  O(n^2)

Solution 2

The following solution is a summary of this discussion with some modifications to improve readability. It uses the binary search approach for efficiency and speed. Personally I am not that convinced of it because there are some specifics about Arrays.binarySearch(..) in the case of application and interpretation of the return value.

The return value can hold (as far as I understand) up to two things. In the normal case (key was found) the index of the (any?) key is returned. In the case when the binary search returns a negative value it is considered as "not found". Furthermore the value can be multiplied by (-1) to receive an insertion index. Finally you have to decrement the insertion index by 1.

   private static int getUniqueValueCount(int[] sorted) {
          int count = 0;
          for (int curentIndx = 0; (curentIndx = nextDeviationIndex(sorted, curentIndx)) != -1;) {
                 count++;
          }
          return count;
   }

   private static int nextDeviationIndex(int[] sorted, int currentIndex) {
          if (sorted.length == 1 && currentIndex == 0) {
                 return 1;
          } else if (currentIndex < sorted.length) {
                 return binarySearchNextDeviationIndex(sorted, currentIndex, sorted[currentIndex] + 1);
          } else {
                 return -1;
          }
   }

   private static int binarySearchNextDeviationIndex(int[] sorted, int startIndex, int key) {
          int searchResult = Arrays.binarySearch(sorted, startIndex, sorted.length, key);
          // this is very hard to get ...
          return searchResult < 0 ? -searchResult - 1 : searchResult;
   }

Although the solution inflates to three methods nobody can avoid the responsibilites involved to produce the result:

Counting while iterating over the interesting elements until there are no left
Decide how to find the next interesting element and provide the information if there is no element left
Provide an efficient search for the case of mass data

Solution 3 (OO)

This is an solution that shows how the responsibilities are cut in the OO. I guess this is the most verbose one for the given problem.

public static class DeviationIterator implements Iterator<Integer> {

    private int[] sorted;

    private int nextIndex;

    public DeviationIterator(int[] sorted) {
        super();
        this.sorted = sorted;
        this.nextIndex = calculateNextIndex();
    }

    @Override
    public boolean hasNext() {
        return nextIndex != -1;
    }

    @Override
    public Integer next() {
        if (nextIndex != -1) {
            int currentIndex = nextIndex;
            nextIndex = calculateNextIndex();
            return currentIndex;
        } else {
            throw new NoSuchElementException();
        }
    }

    private int calculateNextIndex() {
        if (sorted.length == 1 && nextIndex == 0) {
            return 1;
        } else if (nextIndex < sorted.length) {
            return binarySearchNextDeviationIndex(sorted, nextIndex,
                    sorted[nextIndex] + 1);
        } else {
            return -1;
        }
    }

}

private static int getUniqueValueCount(int[] sorted) {
    int count = 0;
    DeviationIterator deviationIterator = new DeviationIterator(sorted);
    while(deviationIterator.hasNext()) {
        deviationIterator.next();
        count++;
    }
    return count;
}

The iterator encapsulates the search algorithm
the getUniqueValueCount uses the iterator to count

The most interesting part is you can clearly see, where the responsibilities are located. Using the iterator pattern identifies the fragment -1 as the EOF for the iterator. The user of the iterator via contract has to ask for if another element is available, so the "Solution 2". But there -1 is not translated into false so the the caller has to interpret it by himself.


Largest rectangle areas after drawing boundaries I was asked the following question while trying to get a Hackerrank certificate. I couldn't solve on time but I now completed the solution. Can you please check if my solution seems good? Suggestions on improvement are appreciated.
Q: You have a rectangle of width w and height h. You will start to draw boundaries. The number of boundaries is given by the length of the given array, distance. isVertical[i] represents whether or not i-th boundary is vertical or not. 1 means vertical and 0 means horizontal. distance[i] tells the distance between your boundary and the rectangle. In case of a horizontal boundary, it represents the distance from the bottom of your rectangle, and in case of a vertical boundary, it represents the distance from the left of your rectangle.
You need to return the largest areas of your rectangle as an array after drawing each of your boundaries.
ex) w = 4, h = 4, isVertical = [0,1], distance = [3,1]
After drawing the first boundary, the largest possible rectangle area is 12.
After drawing the second boundary, the largest possible rectangle area is 9.
You are not given a new rectangle for each boundary. You draw boundaries on the same rectangle.
You should return [12, 9].
My solution:

I created one grid using a 2d-array and computed the largest possible rectangle area after each boundary is drawn by choosing the larger between the area taken up by the boundary and the current largest rectangle area minus the boundary area. I used the same, one grid to do computation for every boundary.

function largestArea(w, h, isVertical, distance) {
    if (isVertical.length !== distance.length) return null;
    if (w == 0 || h == 0) return 0;
    let grid = new Array(h).fill(0).map(() => new Array(w).fill()); // -1 represents areas of the original rectangle
    let largestArea = w*h;
    let areas = [];
    for (let i = 0; i < isVertical.length; i++) {
        let boundaryArea = 0;
        if (isVertical[i] == 1) {
            boundaryArea = drawVerticalBoundary(distance[i], grid); // distance[i] for vertical boundary is distance from the left
        } else {
            boundaryArea = drawHorizontalBoundary(h-distance[i], grid); // distance[i] for horizontal boundary is distance from the bottom
        }
        largestArea = Math.max(largestArea-boundaryArea, boundaryArea);
        areas.push(largestArea);
    }
    return areas;
}

function drawVerticalBoundary(maxCol, grid) {
    let area = 0;
    for (let row = 0; row < grid.length; row++) {
        for (let col = 0; col < maxCol; col++) { // area already taken by another boundary is not part of current boundary
            if (!grid[row][col]) {
                area++;
                grid[row][col] = 1;
            }
        }
    }
    return area;
}

function drawHorizontalBoundary(maxRow, grid) {
    let area = 0;
    for (let row = 0; row < maxRow; row++) {
        for (let col = 0; col < grid[0].length; col++) {
            if (!grid[row][col]) { // area already taken by another boundary is not part of current boundary
                area++;
                grid[row][col] = 1;
            }
        }
    }
    return area;
}

I do have a solution for this question but apology because I am proficient in python
please add the answer who can write the same logic in JS also.
Thanks
what I have done is maintain array of width and height and get amximum width difference and height difference between adjcent location of H and W. which leads to maxArea of rectangle
This is not efficient solution as it was giving timelimit exceed on 3-4 test cases.so appreciate if anyone can optimize it.
def getMaxArea(w, h, isVertical, distance):
    # Write your code here
    def insort(test_list, k):
        for i in range(len(test_list)):
            if test_list[i] > k:
                break
        return test_list[:i] + [k] + test_list[i:]

    maxy = 0
    wline = [0,w]
    hline = [0,h]
    res = []
    for i in range(len(isVertical)):
        if isVertical[i]:
            wline = insort(wline, distance[i])
        else:
            hline = insort(hline, distance[i])
        wmax = 0
        hmax = 0
        for i in range(1,len(wline)):
            if wline[i] - wline[i-1] > wmax:
                wmax = wline[i] - wline[i-1]
        for i in range(1,len(hline)):
            if hline[i] - hline[i-1] > hmax:
                hmax = hline[i] - hline[i-1]

        res.append(hmax * wmax)
    return res


Next multiple with only 1 and 0 as digit Problem:
Find a multiple of a given decimal number \$N\$ that looks like a binary number.

The input will consist of at most \$ 2 \times 10^5 \$ lines, each line consist of an integer \$N\$ (\$0 \lt N \lt 10^{12} \$). Find its described multiple \$M (\ne 0\$), this number \$M\$ must be less than \$10^{12}\$. If there is no such number, print '\$-1\$' on the output.

My solution to this problem is quite simple. Just generated all binary-like numbers less than \$10^{12}\$ excluding \$0\$ (there are \$4095\$ such numbers).

For each number from input, just do a linear search on the binary numbers list. Is it possible to do it faster?

#include <cstdio>
#include <vector>

std::vector<long long> bits;

void generateBits(){

    for (int i = 1; i < 4096; ++i) {

        long long e = 1; //e is the exponencial of 10
        long long n = i; //n will be used to extract the bits from the number i
        long long d = 0;//binary-like decimal number...

        while (n) {
            d += ((n&1)*e);
            n >>= 1, e *= 10;
        }

        bits.push_back(d);
    }
}

long long getBitMultiple(long long n){

    for (auto &bit : bits) {
        if(bit%n == 0){
            return bit;
        }
    }

    return -1;
}

int main(void){

    generateBits();

    long long n;

    while (scanf("%lld",&n) != EOF) {
        printf("%lld\n",getBitMultiple(n));
    }

    return 0;
}

I normally don't do C++ reviews, so I might be off on a few accounts, but here are some style and code review points.

Style and Code Review

Avoid cramped up statements – Mostly your code reads easily, but avoid stuff like if(bit%n == 0){, open it up to become if (bit % n == 0) {. Especially important since you seem to have decided on having the starting brace on the same line. It needs to stand out to be recognised. Along the same line of reasoning, add a space after commas like in the printf("...", getBitMultiple(n));
Aim for better names – Names like e, n and d are not really intuitive. Neither is bit or getBitMultiple, it is not a bit but a decimal number looking like a binary. Confusing. Maybe change bits into binaryLookingDecimals?
Add more comments – Parts of the code is unclear, like what you are aiming for in your functions, and where do you get your numbers from, and how does your while loop work
Avoid using globals – The main table of your code, bits, is a global variables. As it reads now, the generateBits() and getBitMultiple(n) are separate non-related functions. If however you add something like binaryLookingDecimals = generateBinaryLookingDecimals() and findMultiple(n, binaryLookingDecimals) they are clearly related

Edited: My solution review was based on a misunderstanding of the problem statement. As a foreigner I confused multiple/multiply/multiplicand, and thought you had done the same. Your code does however return the binary looking decimal which is a multiple of \$N\$ as requested, and that renders my previous solution void, which found the factor you need to multiply to \$N\$ to get a binary looking decimal.

The only comment I then have to your actual solution, is whether it is worth it to test whether the candidate is greater than \$N\$ before starting the modulo operation, but that has to be tested if it gives any significant speed increase. Most likely it is not worth it.

Thanks to Barry for correcting me regarding my mistake on the solution review

Yes, you can do it faster, in \$O(max(N, c))\$ where \$c\$ is the number of binary-like numbers under the specified limit, using breadth-first search.

The vertices are values mod \$N\$. There is an edge from \$x\$ to \$y\$ iff \$10x = y\$ or \$10x+1 = y\$. Starting at \$0\$, and when you get \$0\$ again, the sequence of the choices between the two equations is an answer.


Sorting an array of jobs by various criteria Below is my current code for sorting an array, "$jobs_list".

It's not very DRY and it's fugly. And there are several more similar case statements still to go in. How could I write it so my brain doesn't twitch to look at it?

switch ($sort) {
    case 'number':
        usort($jobs_list, function($a, $b){
            return ( (int) $a->number > (int) $b->number );
        });
        break;
    case 'client':
        usort($jobs_list, function($a, $b){
            return strcmp($a->client_contact_last_name, $b->client_contact_last_name);
        });
        break;
    case 'name':
        usort($jobs_list, function($a, $b){
            return strcmp($a->name, $b->name);
        });
        break;
    case 'status':
        usort($jobs_list, function($a, $b){
            return ( (int) $a->status > (int) $b->status );
        });
        break;
    case 'revenue':
        usort($jobs_list, function($a, $b){
            return ( (float) $a->base_revenue > (float) $b->base_revenue );
        });
        break;
    case 'profit':
        usort($jobs_list, function($a, $b){
            return ( (float) $a->profit > (float) $b->profit );
        });
        break;
    case 'margin':
        usort($jobs_list, function($a, $b){
            return ( (float) $a->margin > (float) $b->margin );
        });
        break;

    default:
        # do nothing...
}

First, it may make sense to make sure that the field you're checking in $sort, is the same as the propertyname of the object in $jobs_list.

Also, it looks like there's a lot of identical sort functions; the only difference being that the property you're checking is different.

You could just combine the switch statements that have an identical sorting strategy (revenue, status, profit, margin, etc..) and turn the field into a variable, such as:

switch ($sort) {
    case 'number':
    case 'status':
    case 'profit':
    case 'margin':
        usort($jobs_list, function($a, $b) use ($sort) {
            return ( (float) $a->$sort > (float) $b->$sort );
        });
        break;

}

I'd start by moving the switch statement into the callback function:

usort($jobs_list, function($a, $b){
 switch ($sort):
      case 'number':return ( (int) $a->number > (int) $b->number ); break;
      case 'client':return strcmp($a->client_contact_last_name, $b->client_contact_last_name); break;
      //etc...
 endswitch;
 });


Reading binary file of doubles I'm trying to read doubles from a relatively small binary file. This currently reads a 100 KB file in about 6 milliseconds in my system. I would like to reduce that if possible.

void readNParseData(const char* filePath, vector<double> *&data){

    ifstream ifs(filePath, ios::in | ios::binary);

    // If this is a valid file
    if (ifs) {
        // Temporary Variables
        std::streampos fileSize;
        double *fileBuffer;
        size_t sizeOfBuffer;

        // Check whether the parameter is already full
        if (data != 0){
            // Reset the output
            data->clear();
            data = 0;
        }

        // Get the size of the file
        ifs.seekg(0, std::ios::end);
        fileSize = ifs.tellg();
        ifs.seekg(0, std::ios::beg);

        sizeOfBuffer = fileSize / sizeof(double);
        fileBuffer = new double[sizeOfBuffer];

        ifs.read(reinterpret_cast<char*>(fileBuffer), fileSize);

        // Now convert the double array into vector
        data = new vector<double>(fileBuffer, fileBuffer + sizeOfBuffer);

        free(fileBuffer);
    }
}

As you can see there is a redundant copy of a double * array to a vector. Perhaps reading to the vector directly might speed it up, but I don't know how.

Suggestion 1

This block of code does not seem clean:

    // Check whether the parameter is already full
    if (data != 0){
        // Reset the output
        data->clear();
        data = 0;
    }

If data used to point to non-NULL, then you are just making it NULL. You should delete data before pointing it to NULL:

    if (data != 0){
        // Reset the output
        data->clear();
        delete data;
        data = 0;
    }

Suggestion 2

Still better, if you have the option, change the interface to

void readNParseData(const char* filePath, vector<double>& data);

Suggestion 3

There is nothing in your code to indicate to the calling function that you were able or unable to read the data from the file. There is no else to go with

if (ifs) {

One way of indicating whether the function was successful in reading the data is to change the return type of the function to bool. Then, you can add

return true;

at the end of the if block and then add an else block:

else {
   return false;
}

Suggestion 4

To remove the redundant memory allocation and deallocation to fileBuffer, simply use std::vector::data:

data.resize(sizeOfBuffer);
ifs.read(reinterpret_cast<char*>(data.data()), fileSize);

Suggestion 5

Add a check to make sure that ifs.read was successful:

ifs.read(reinterpret_cast<char*>(data.data()), fileSize);
if ( !ifs )
{
   return false;
}

This is how you get both text and numbers, floats, ints, doubles separated by , c++ using string stream to read each line.

if ( myFile.is_open( ) ) {
    while ( getline( myFile, reading ) )
    {
        stringstream eachline( reading ) ;
        while ( eachline( ) )
        {
            string x = ',';
            getline( eachline, StoreData, x ) ;

    //    read if float or read else word from file

            Increment++ here
        }
    }
}


Minimizing duplicate routes index.js I am trying to build a REST API with express router, which contains of nested sub routes. I have mounted these sub routes in my index.js file.

I have defined it as follows:

// Mounted routes
app.use('/api/v1/Project', new ProjectRouter().routes);
app.use('/api/v1/Project/:projectId/Context', new ContextRouter().routes);
app.use('/api/v1/Project/:projectId/Context/:contextId/Question', new QuestionRouter().routes);
app.use('/api/v1/Project/:projectId/Context/:contextId/Question/:questionId/Answer', new AnswerRouter().routes);

I want to arrange my routes revolved around the functionality and being more complaint towards REST standards.

In the case the route prefix /api/v1/Project/ is being repeated over and over again.

Is there some best practice to minimize the redundant routes by prefixing?

This is where the Express Router class comes in.  You could define a router for ‘api/v1/Project’, mount that router to you main app, and then add the individual routes to the router.

The main idea is you can define routes in each controller (no matter how). For hide the base route, you can use a base class with a method addRoute and a property baseRoute or just use a simple variable for base route.

With Ecma6 will look something like this:

in projects.controller.js

require('base.controller.js')
class ProjectsController extends BaseController {
  constructor(app){
    super(app, "/api/v1/Project");
    this.addRoute("/", "get", this.getAll);
    this.addRoute("/:id", "get", this.getOne)
  }
  getAll(){}
  getOne(){}
}
module.exports = ProjectsController;

in 'base.controller.js':

class BaseController {
  constructor(app, baseRoute){
    this.baseRoute = baseRoute;
    this.app = app
  }
  addRoute(route, method, callback){
    const url = this.baseRoute + route;
    console.log('controllerRoute', url, method);
    this.app[method](url, callback.bind(this));
 }
}
module.exports=BaseController;

and in index.js (or app/server.js), for each controller:

require("./projects.controller.")(app);

The simplest way:

let baseRoute = "/api/v1/Project";
app.use(baseRoute + "/", new ProjectRouter().routes);
app.use(baseRoute + '/:projectId/Context', new ContextRouter().routes);


JS how to optimize function that finds the text value in gematria Hello guys i wrote this function code:

function gematria(text){
  var newText = text.toLowerCase(); // handle capital letters
  var startValue=0; //initialize value to zero
  for(let i=0 ; i<newText.length; i++){ // loop through every letter on the text
    let currentCharValue = newText.charCodeAt(i) - 96; // get the distance from the 'a'
    startValue +=currentCharValue;
  }
  return startValue
}

gematria("someText");

it works well but is there any way to do it more short? or better? how good is that code for doing the task?

The shortest approach is to parse the character with a base of 36 and get the delta of 9 for summing.

function gematria(text) {
    return [...text].reduce((s, c) => s + parseInt(c, 36) - 9, 0);
}

console.log(gematria("someText"));

Shorter:

  const gematria = text => [...text].reduce((s, c) => s + c.toLowerCase().charCodeAt(0) - 96, 0);

Better: I don't know. Your code is totally fine.


Project Euler 38: Pandigital Multiples I just solved Project Euler 38 and was wondering if you guys could provide some suggestions on how to speed it up. I've tried adding extra conditions but they increase the time. Any ideas on how to optimize the numbers I'm checking would be appreciated.

from timeit import default_timer as timer

def create_pandigital_multiple(original_number):
    num = str(original_number)
    digits = range(1, 10)
    if len(num) >= 5:
        return 0
    else:
        temp = 2
        while len(num) < 9:
            num += str(original_number * temp)
            temp += 1
        if sorted([int(x) for x in num]) == digits:
            return int(num)
        else:
            return 0

start = timer()
ans = 0
for k in range(9, 10**5):
    if create_pandigital_multiple(k) > greatest:
        ans = create_pandigital_multiple(k)

elapsed_time = (timer() - start) * 1000 # s --> ms

print "Found %d in %r ms." % (ans, elapsed_time)

Checking if len(num) >= 5 is redundant because you only call the function with k < 10**5
Instead of sorted([int(x) for x in num]) you could compute just sorted(num) and compare that to a DIGITS = [str(i) for i in range(1, 10)] constant that you can compute outside the function.
Most of the multiples end up longer than 9 digits. Testing len(num) == 9 before sorting the digits should be faster.
You could take advantage of the example 918273645 provided in the problem statement. The number you are looking for must be greater or equal to that. For one thing, the first two digits must be in range 91 to 98, which is a small fraction of all the two-digit numbers you try.

Instead of

if create_pandigital_multiple(k) > greatest:
    ans = create_pandigital_multiple(k)

you can use

ans = max(ans, create_pandigital_multiple(k))


Google News scraper to fetch links with similar stories The following code takes either a URL or the title to an existing news article.

Searches Google News using the title.
Collects all links from search results.

import urllib2
from lxml import html
import requests

def get_page_tree(url=None):
    page = requests.get(url=url, verify=False)
    return html.fromstring(page.text)

def get_title(url=None):
    tree = get_page_tree(url=url)
    return tree.xpath('//title//text()')[0].strip().split(' -')[0]

def find_other_news_sources(url=None, title=None):
    # Google forwards the url using <google_domain>/url?q=    <actual_link>. This might change over time
    forwarding_identifier = '/url?q='
    if not title:
        title = get_title(url=url)
    google_news_search_url = 'http://www.google.com/search?q=' + urllib2.quote(title) + '&tbm=nws'
    google_news_search_tree = get_page_tree(url=google_news_search_url)
    other_news_sources_links = [a_link.replace(forwarding_identifier, '').split('&')[0] for a_link in
                            google_news_search_tree.xpath('//a//@href') if forwarding_identifier in a_link]
    return other_news_sources_links

Instead of constructing the google_news_search_url with two string concatenations, use string formatting.
The other_news_sources_links line is very dense. Please split it up.
In addition to the defaults for all your args being None, you're calling all of your functions with keyword arguments, which seems unnecessary.
But before you fix that, consider why you need these 2 line functions in the first place. They don't seem to do anything complicated enough to warrant having to jump around in the code.

Why do all of your function arguments have default values of None? What purpose would these functions serve if they were run with all argument values as None?

Secondly, you should have two blank lines between your functions, as per PEP8.

Finally, I'd recommend adding docstrings to your functions, and your file as well. A typical function docstring looks something like this:

def my_func( ... ):
    """
    Put a description of your function
    and it's arguments here.
    """
    ...

Anyways, other than that, your code looks really nice! Keep up the good work!


Detect that a class is marked by a custom annotation in Spring I would like to enable application behaviour based on the presence of a custom annotation that marks the configuration class (normally named something like ApplicationConfig.class) in a Spring 4 application.

This feature functions in the same way as do @EnableCache, @EnableRepositories etc, in the sense that adding these annotations adds behaviour to the application.

The following code shows the way I have decided to implement the feature. It detects and analyses the classes that are loaded by Spring at application startup and fires an event if it determines that one of those classes is annotated with the specified custom annotation. This code works as designed however the use of reflection feels clumsy and I would like to seek the advice of other developers who may have solved a similar problem in a more elegant/different manner.

The custom annotation class.

@Target({ElementType.TYPE})
@Retention(RetentionPolicy.RUNTIME)
public @interface ServiceRegistration {
    boolean enabled() default true;
}

The configuration class.

@ServiceRegistration(enabled = true)
@Configuration
@ComponentScan(basePackageClasses = Application.class, excludeFilters = @Filter({Controller.class, Configuration.class}))
@EnableJpaRepositories("com.package.repository")
class ApplicationConfig {

    @Bean
    public static PropertyPlaceholderConfigurer propertyPlaceholderConfigurer() {
        PropertyPlaceholderConfigurer ppc = new PropertyPlaceholderConfigurer();
        ppc.setLocation(new ClassPathResource("/persistence.properties"));
        return ppc;
    }

}

The class that detects the presence of the annotation.

@Component
public class ServiceRegistrationEventListener {

    private static final String ANNOTATED_CLASSES = "annotatedClasses";

    @Autowired
    ApplicationEventPublisher applicationEventPublisher;

    @Autowired
    ApplicationContext applicationContext;

    @EventListener
    void contextRefreshedEvent(ContextRefreshedEvent event) throws NoSuchFieldException, IllegalAccessException {

        ApplicationContext applicationContext = event.getApplicationContext();
        AnnotationConfigWebApplicationContext annotationContext = ((AnnotationConfigWebApplicationContext) applicationContext);
        for (Field field : annotationContext.getClass().getDeclaredFields()) {
            if (ANNOTATED_CLASSES.equals(field.getName())) {
                field.setAccessible(true);
                Object aContext = field.get(annotationContext);
                if (aContext instanceof Set) {
                    Set<Class<?>> classes = (Set<Class<?>>) aContext;
                    for (Class clazz : classes) {
                        for (Annotation annotation : clazz.getDeclaredAnnotations()) {
                            if (annotation.annotationType().isAssignableFrom(ServiceRegistration.class)) {
                                applicationEventPublisher.publishEvent(new RegistrationEvent(((ServiceRegistration) annotation).enabled()));
                            }
                        }
                    }
                }
            }
        }
    }

    class RegistrationEvent {

        private boolean enabled;

        public RegistrationEvent(boolean enabled) {
            this.enabled = enabled;
        }

        public boolean enabled() {
            return enabled;
        }

    }

}

Values set in the annotation e.g. @ServiceRegistration(enabled = true) are recovered and passed into the published event.

Any ideas on how to improve this code and especially ideas on completely different ways to achieve the same are welcome.

I'm not that familiar with Spring, but if your goal is to get all types annotated with FooBar and then the enabled attribute, you may just use the Reflections library.
When your application is bootstrapping you would:

Reflections reflections = new Reflections("my.project");

And in your listener:

@EventListener
void contextRefreshedEvent(ContextRefreshedEvent event) throws NoSuchFieldException, IllegalAccessException {
    Set<Class<?>> classes = reflections.getTypesAnnotatedWith(FooBar.class);
    for (Class clazz : classes) {
        boolean enabled = clazz.getAnnotation(FooBar.class).enabled();
        System.out.println("Enabled: " + enabled);
    }
}

Notice that you don't need to get all annotations in the class, check types and then cast if you use getAnnotation. The advantage of using Reflections is that class metadata is indexed, which means the expensive reflection operations aren't done all the time. It won't work, however, if Spring changes annotations on runtime (which I don't know if it does or not). If you can't use the Reflections library, just the getAnnotation method is already an improvement over your original for over each annotation.


Temperature converter using "switch" statement I wrote a program to convert temperature into different units, where the users have to put a space between temperature and unit:

  23 f, 34 c, 45 k

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;

namespace PracticeSet1
{

    class Program
    {
        public static float temp;
        public static char tempUnit;

        static void Main(string[] args)
        {
            //Getting user input
            Console.WriteLine("Enter Temprature in to convert it into other i.e  30 k, 45 f, 50 c *Put space between value and unit* ");
            string[] tempInput = Console.ReadLine().Split();

            //parse element 0
            temp = float.Parse(tempInput[0]);
            //assinging tempUnit
            tempUnit = char.Parse(tempInput[1]);

            switch (tempUnit)
            {
                //Converting temp to F and K if tempUnit == c
                case 'c':
                    Console.WriteLine("Celsius To Farhenheit and Kelvin");
                    convertCelsiusToFarhenheit();
                    convertCelsiusToKelvin();
                    break;
                //Converting temp to C and F if tempUnit == K

                case 'k':
                    Console.WriteLine("Kelvin");
                    convertKelvinToCelsius();
                    convertKelvinToFarhenheit();
                    break;
                //Converting temp to C and K if tempUnit == F

                case 'f':
                    Console.WriteLine("Farhenheit to Celsius and kelvin");
                    convertFarhenheitToCelsius();
                    convertFarhenheitToKelvin();
                    break;

            }

        }
        static void convertFarhenheitToCelsius()
        {
            Console.WriteLine((temp - 32) * 0.5556 + "°C");
        }
        static void convertFarhenheitToKelvin()
        {
            Console.WriteLine((temp + 459.67) * 5 / 9 + "°K");
        }
        static void convertCelsiusToFarhenheit()
        {
            Console.WriteLine((temp * 1.8) + 32 + "°F");
        }
        static void convertCelsiusToKelvin()
        {
            Console.WriteLine(temp + 273.15 + "°K");

        }
        static void convertKelvinToCelsius()
        {
            Console.WriteLine(temp - 273.15 + "°C");
        }
        static void convertKelvinToFarhenheit()
        {
            Console.WriteLine(temp - 459.67 + "°F");
        }
    }
}

public static float temp;
public static char tempUnit;

There's no reason to make these into global variables. It's better to use local variables:

static void Main(string[] args)
{
    float temp;
    char tempUnit;
}

If you want a first step into OOP, try wrapping the temp and tempUnit into a class (or struct) of their own.

While Console.ReadLine().Split() is essentially the same as Console.ReadLine().Split(' '), I would suggest putting the character in there.

It's a matter of readability. Initially, I assumed it would split on newlines, not spaces, until I remembered. Or, alternatively, add a comments that mentions you're splitting on spaces.

You're not handling garbage values. What happens if the user enters 123f or batman or k 123? Try to handle invalid data and show the user an error message that describes the problem.

if(!float.TryParse(tempInput[0], out temp)
{
     Console.WriteLine($"Cannot parse the value \"{tempInput[0]}\"");

     //Wait until user presses enter and close the application
     Console.ReadLine();
     return;
}

The subsequent code is the same, as TryParse will still put a usable value in the temp variable.

Console.WriteLine(temp + 273.15 + "°K");

While this currently works, I highly suggest not doing this.

It's very hard to spot that you're first doing a mathematical operation (number + number), and then a string concatenation (number + string, which is handled as string + string).

Suppose you're in a culture where the unit comes before the value (e.g. °K 123). You can't just simply invert the code ("°K" + temp + 273.15); because if temp equals 123, your result will be °K123273.15 because it is now concatenating three string values.
While you could work around this issue with parentheses, it's still not a good fix.

There's no reason to inline all statement. Especially when you're a beginner, but this applies to experts as well. Handling the same operations on multiple lines does not affect application performance, but it does dramatically lower readability.

var convertedValue = temp + 273.15;
var convertedValueString = convertedValue + "°K";

Notice that I didn't use Console.WriteLine() yet. You've had to copy paste that in every method, which is not a reusable pattern.

It would be better to have the method return a string:

static string convertCelsiusToKelvin()
{
    var convertedValue = temp + 273.15;
    var convertedValueString = convertedValue + "°K";
    return convertedValueString;
}

And wrap the method call in the Main() method:

switch (tempUnit)
{
    //Converting temp to F and K if tempUnit == c
    case 'c':
        Console.WriteLine("Celsius To Farhenheit and Kelvin");
        Console.WriteLine(convertCelsiusToFarhenheit());
        Console.WriteLine(convertCelsiusToKelvin());

Note: This can be optimized further, but this is already a good first step. It teaches you to separate the calculation from the presentation of the calculated value.

Try to separate the logic into methods more. Your Main() method currently is doing many things that don't really have anything to do with each other:

Read the values
Decide which conversion calculations to do
(As per my suggestion) Printing the converted values.

A minor redesign to show you what I mean. First, let's define a struct for our values:

public struct Temperature
{
    float Value;
    char Unit;
}

You could redesign your main method to be much more abstracted:

static void Main(string[] args)
{
     Temperature input = GetInputFromUser();

     Temperature valueKelvin = CalculateKelvin(input);
     Print(valueKelvin);

     Temperature valueFahrenheit = CalculateFahrenheit(input);
     Print(valueFahrenheit);

     Temperature valueCelsius = CalculateCelsius(input);
     Print(valueCelsius);

    Console.ReadLine();
}

Some explanatory examples:

public static Temperature CalculateKelvin(Temperature inputValue)
{
     switch(inputValue.Unit)
     {
         case 'K':
             //Same unit, so no conversion needed
             return value;
         case 'F':
             // F => K
             return convertFahrenheitToKelvin(inputValue);
         case 'C':
             // C => K
             return convertCelsiusToKelvin(inputValue);

     }
}

public static void Print(Temperature value)
{
    Console.WriteLine($"{value.Value} °{value.Unit}");
}

I suggest following things

Use delegates for calling appropriate conversion methods, and

Move the responsibility of conversions to a separate class.
We can have separate classes for each type of conversion, but for simplicity i have created every thing in same class

Declare a delegate
    public delegate string Convert(double temperature, Func<double, double> logic, string unit

Class "TemperatureConverter" that will do conversions.
    public class TemperatureConverter
    {
        public string PerformConversion (double temperature, Func<double, double> logic, string unit)
        {
            return string.Format("{0} {1}", logic.Invoke(temperature), unit);
        }

        public string FarhenheitUnit
        {
            get
            {
                return "°F";
            }
        }

        public string CelsiusUnit
        {
            get
            {
                return "°C";
            }
        }

        public string KalvinUnit
        {
            get
            {
                return "°K";
            }
        }

        public double FarhenheitToCelsius(double temperature)
        {
           return (temperature - 32) * 0.5556;
        }

        public double FarhenheitToKelvin(double temperature)
        {
            return (temperature + 459.67) * 5 / 9;
        }
        public double CelsiusToFarhenheit(double temperature)
        {
            return (temperature * 1.8) + 32;
        }
        public double CelsiusToKelvin(double temperature)
        {
            return temperature + 273.15;
        }
        public double KelvinToCelsius(double temperature)
        {
            return temperature - 273.15;
        }
        public double KelvinToFarhenheit(double temperature)
        {
            return temperature - 459.67;
        }
    }

Client code is as below.  Input can be taken in any format depending on the requirement.  I have not added any basic checks.
    class Program
    {
        static void Main(string[] args)
        {
            //Getting user input
            Console.WriteLine("Enter Temprature in to convert it into other i.e  30 k, 45 f, 50 c *Put space between value and unit* ");
            string[] input = Console.ReadLine().Split();
            var temperature = double.Parse(input[0]);
            var unit = char.Parse(input[1]);
            TemperatureConverter tempConverter = new TemperatureConverter();
            Convert cnv = new Convert(tempConverter.PerformConversion);
            switch (unit)
            {
                case 'c':
                    Console.WriteLine("Celsius To Farhenheit and Kelvin");
                    Console.WriteLine(cnv.Invoke(temperature, tempConverter.CelsiusToFarhenheit, tempConverter.FarhenheitUnit));
                    Console.WriteLine(cnv.Invoke(temperature, tempConverter.CelsiusToKelvin, tempConverter.KalvinUnit));
                    break;
                case 'f':
                    Console.WriteLine("Farhenheit To Celsius and Kelvin");
                    Console.WriteLine(cnv.Invoke(temperature, tempConverter.FarhenheitToCelsius, tempConverter.CelsiusUnit));
                    Console.WriteLine(cnv.Invoke(temperature, tempConverter.FarhenheitToKelvin, tempConverter.KalvinUnit));
                    break;
                case 'k':
                    Console.WriteLine("Kelvin To Celsius and Farhenheit");
                    Console.WriteLine(cnv.Invoke(temperature, tempConverter.KelvinToCelsius, tempConverter.CelsiusUnit));
                    Console.WriteLine(cnv.Invoke(temperature, tempConverter.KelvinToFarhenheit, tempConverter.FarhenheitUnit));
                    break;
            }

            Console.ReadLine();
        }
    }


Toggle menu under specified screen width I've created a simple toggle menu in jQuery, which is capable of specifying a breakpoint, but I'm afraid my code is too complicated. How can I make it simpler?

I also have a question about performance of code on window resize. I want to invoke a function only once if the window's width is smaller than a given breakpoint. Should I use some boolean variable, or is there a better solution?

Here's a pen.

$(document).ready(function() {
  var $menuBtn = $('[href="#menu"]'),
      $nav = $('#menu'),
      $window = $(window),
      breakpoint = 800;

  function changeMenuStyle() {
    if ($window.width() < breakpoint) {
      $menuBtn.show();
      $nav.addClass('toggled').hide();
    } else {
      $menuBtn.hide();
      $nav.removeClass('toggled').show();
    }
  }

  changeMenuStyle();

  $window.on('resize',function() {
    changeMenuStyle();
  });

  $menuBtn.on('click',function(e) {
    e.preventDefault();
    $nav.slideToggle();
  });
});
* {
  box-sizing: border-box;
}

body {
  background: #485373;
  font-family: "Lato", sans-serif;
  font-weight: 300;
}

.container {
  max-width: 940px;
  margin: 0 auto;
}

.menu-link {
  display: block;
  color: #fff;
  background: #202332;
  width: 100%;
  padding: 20px;
  text-decoration: none;
}
.menu-link i {
  display: inline-block;
  margin: 0 10px;
}

.menu {
  overflow: hidden;
}
.menu i {
  display: block;
  font-size: 25px;
  padding: 10px 0;
}
.menu ul {
  width: 100%;
}
.menu li {
  width: 20%;
  float: left;
  text-align: center;
}
.menu a {
  display: block;
  padding: 20px;
  width: 100%;
  background: #323A51;
  color: #fff;
  text-decoration: none;
  border-bottom: 2px transparent solid;
}
.menu a:hover {
  background: #202332;
  border-bottom: 2px #fff solid;
}
.menu.toggled i {
  display: inline-block;
  margin-right: 10px;
  padding: 0;
  font-size: 15px;
}
.menu.toggled li {
  width: 100%;
}
.menu.toggled a {
  padding: 10px;
}

main {
  padding: 50px;
  color: #fff;
}
main h1 {
  font-size: 30px;
  font-weight: 400;
  line-height: 40px;
  margin: 10px 0;
}

    Menu

Home

About

Services

Team

Contact

Toggle menu

Vestibulum tempus at ipsum a finibus. Mauris pharetra eleifend odio quis lacinia. Pellentesque ullamcorper, dolor vel congue tincidunt, mi nisl suscipit metus, nec bibendum elit ligula vitae leo. Integer volutpat semper iaculis. Mauris fermentum, enim ac finibus scelerisque, magna nibh volutpat dolor, vitae sodales mi augue molestie augue. Aliquam ultricies eget mi vel viverra. Morbi id tortor purus. Vivamus sed pretium justo. Fusce porttitor eu neque quis volutpat. Lorem ipsum dolor sit amet, consectetur adipiscing elit. Curabitur ultrices metus a aliquet tincidunt. Ut pellentesque dui nec ipsum fringilla ultricies.

Feedback

The code looks pretty good. The Javascript code waits until the DOM is ready, then caches references to DOM elements in variables before using those variables in the functions later on.

I initially thought some of the CSS could be consolidated (e.g. many common styles under .menu-link and .menu a but then I realized that might be tricky with the SCSS...

  I want to invoke a function only once if the window's width is smaller than a given breakpoint. Should I use some boolean variable, or is there a better solution?

One technique for this is to use a debounced function callback. There are jQuery plugins for that, or that article mentions ways to achieve it on your own (like I suggested in this SO answer).

Suggestions

Toggle the display using .toggle()

The .show() and .hide() calls can be replaced with a single call to .toggle(). Also, using the second form of .toggleClass(), the class name toggled can be added/removed depending on whether the menu button is being displayed or not (i.e. if the width is less than the break point). Thus the function changeMenuStyle() can be reduced to two lines (or one if you move the conditional out of a variable):

function changeMenuStyle() {
    var LTBreakpoint = $window.width() < breakpoint;
    $menuBtn.toggle(LTBreakpoint).toggleClass('toggled', LTBreakpoint);
}

Remove closure just to call function

The resize callback can simply be the name of the function - i.e.

$window.on('resize',function() {
    changeMenuStyle();
});

Can be simplified to:

$window.on('resize', changeMenuStyle);

Though be aware that the event parameter would get passed to the function, which could have adverse affects if the function is expecting other parameters.

CSS media query

I should have mentioned this, as Rotora did- CSS media queries can be used to eliminate much of the Javascript code (like you did in the CSS for this post).

Adding this CSS:

/* default settings ( width >= 800px) */
#menu{
    display: block;
}
[href="#menu"] {
    display: none;
}
/* only when screen width is less than 800px */
@media (max-width: 799px) {
    #menu{
        display: none;
    }
    [href="#menu"] {
        display: block;
    }
}

Can allow the JS to be simplified to:

$(document).ready(function() {
  var $menuBtn = $('[href="#menu"]'),
      $nav = $('#menu');

  $menuBtn.on('click',function(e) {
    e.preventDefault();
    $nav.slideToggle();
  });
});

$(document).ready(function() {
  var $menuBtn = $('[href="#menu"]'),
      $nav = $('#menu');

  $menuBtn.on('click',function(e) {
    e.preventDefault();
    $nav.slideToggle();
  });
});
* {
  box-sizing: border-box;
}

body {
  background: #485373;
  font-family: "Lato", sans-serif;
  font-weight: 300;
}

.container {
  max-width: 940px;
  margin: 0 auto;
}

.menu-link {
  display: block;
  color: #fff;
  background: #202332;
  width: 100%;
  padding: 20px;
  text-decoration: none;
}
.menu-link i {
  display: inline-block;
  margin: 0 10px;
}

.menu {
  overflow: hidden;
}
.menu i {
  display: block;
  font-size: 25px;
  padding: 10px 0;
}
.menu ul {
  width: 100%;
}
.menu li {
  width: 20%;
  float: left;
  text-align: center;
}
.menu a {
  display: block;
  padding: 20px;
  width: 100%;
  background: #323A51;
  color: #fff;
  text-decoration: none;
  border-bottom: 2px transparent solid;
}
.menu a:hover {
  background: #202332;
  border-bottom: 2px #fff solid;
}
.menu.toggled i {
  display: inline-block;
  margin-right: 10px;
  padding: 0;
  font-size: 15px;
}
.menu.toggled li {
  width: 100%;
}
.menu.toggled a {
  padding: 10px;
}

main {
  padding: 50px;
  color: #fff;
}
main h1 {
  font-size: 30px;
  font-weight: 400;
  line-height: 40px;
  margin: 10px 0;
}
/* default settings ( width >= 800px) */
#menu{
    display: block;
}
[href="#menu"] {
    display: none;
}
/* only when screen width is less than 800px */
@media (max-width: 799px) {
    #menu{
        display: none;
    }
    [href="#menu"] {
        display: block;
    }
}

    Menu

Home

About

Services

Team

Contact

Toggle menu

Vestibulum tempus at ipsum a finibus. Mauris pharetra eleifend odio quis lacinia. Pellentesque ullamcorper, dolor vel congue tincidunt, mi nisl suscipit metus, nec bibendum elit ligula vitae leo. Integer volutpat semper iaculis. Mauris fermentum, enim ac finibus scelerisque, magna nibh volutpat dolor, vitae sodales mi augue molestie augue. Aliquam ultricies eget mi vel viverra. Morbi id tortor purus. Vivamus sed pretium justo. Fusce porttitor eu neque quis volutpat. Lorem ipsum dolor sit amet, consectetur adipiscing elit. Curabitur ultrices metus a aliquet tincidunt. Ut pellentesque dui nec ipsum fringilla ultricies.

Just a little simple than yours, you can change your function for the same action.

  function changeMenuStyle() {
  $menuBtn.hide();
  $nav.removeClass('toggled');
     if ($window.width() < breakpoint) {
        $menuBtn.show();
        $nav.addClass('toggled');
    }
  }


Multiplying 2 numbers without using * operator in Java I saw this interview question and decided to solve using recursion in Java.

  Write a multiply function that multiples 2 integers without using *

public class Main {

  public static void main(String[] args) {

    Scanner in = new Scanner(System.in);
    System.out.println("Enter first num: ");
    double num = in.nextDouble();
    System.out.println("Enter second num: ");
    double numTwo = in.nextDouble();

    System.out.println(multiply(num, numTwo));
  }

  private static double multiply(double x, double y) {

    if (x == 0 || y == 0) {
      return 0;
    } else if (y > 0) {
      return x + multiply(x, y - 1);
    } else if (y < 0) {
      return -multiply(x, -y);
    } else {
      return -1;
    }
  }
}

What should I return instead of -1 to make this clear?

The challenge only requires you to work with integers. This technique would not actually be able to multiply two floating-point numbers, so don't use double. A proper solution for integers would not have a failure node that requires you to return -1.

A better algorithm to use would be the Russian peasant method which can be done by bitwise operations and addition.

As these are integers, a naive solution to add one input over and over again in a for loop indexed from 0 to the other input - 1. You have to handle negatives as well.


Improving efficiency for finding longest contiguous non-decreasing substring I was working on a problem set and I came across this problem:

Assume s is a string of lower case characters.
Write a program that prints the longest substring of s in which the letters occur in alphabetical order. For example, if s = 'azcbobobegghakl', then your program should print:
Longest substring in alphabetical order is: beggh

In the case of ties, print the first substring. For example, if s = 'abcbcd', then your program should print
Longest substring in alphabetical order is: abc

I actually was able to get a solution, but it was the most inefficient ever! And here's what I came up with:
def test():
    index = 1
    prev_index = 0
    count = 0
    global largest
    largest = ''
    test = s[prev_index]

    while count < len(s):
        if ord(s[index]) > ord(s[prev_index]):
            test += s[index]
            index += 1
            prev_index += 1
        elif ord(s[index]) == ord(s[prev_index]):
            test += s[index]
            index += 1
            prev_index += 1
        else:
            if len(largest) < len(test):
                largest = test[:]
            test = s[index]
            prev_index += 1
            index += 1
        count += 1
    return largest
try:
     test()
except IndexError:
     pass
finally:
     print largest

Can anyone give me an example of better code that produces the same output?

Your algorithm is basically fine, but it is expressed in the code clumsily.

The biggest problem is the interface of the function: it should accept a string parameter and return a string.  Instead, your function takes s from its environment and uses the global variable largest to store the answer.  Although there is a return largest statement, it is very misleading.  What actually happens is that s[index] raises an IndexError, so that the return largest is never reached, and the answer is passed back using the global variable instead.

The root cause for the IndexError is that there are too many variables, causing confusion.  With each iteration through the loop, you increment index, prev_index, and count.  That means that those three variables could be reduced to one, which could just be called i.  (If you think about it, you'll find that count is always one less than index.  Since the loop condition is count < len(s), s[index] will go one position beyond the end of the string.)

The next observation to make is that your if and elif blocks are almost identical.  You can combine them by using >= for the test.

Python strings are immutable, so largest = test[:] could just be largest = test.

Putting everything together…

def longest_nondecreasing_substring(s):
    if s is None or len(s) <= 1:
        return s

    longest = test = s[0]

    for i in range(1, len(s)):
        if ord(s[i]) >= ord(s[i - 1]):
            test += s[i]
        else:
            if len(longest) < len(test):
                longest = test
            test = s[i]
    if len(longest) < len(test):
        longest = test
    return longest

It's the famous Longest increasing subsequence problem. Convert the chars to ints using the ord function

def longest_increasing_subsequence(X):
    """
    Find and return longest increasing subsequence of S.
    If multiple increasing subsequences exist, the one that ends
    with the smallest value is preferred, and if multiple
    occurrences of that value can end the sequence, then the
    earliest occurrence is preferred.
    """
    n = len(X)
    X = [None] + X  # Pad sequence so that it starts at X[1]
    M = [None]*(n+1)  # Allocate arrays for M and P
    P = [None]*(n+1)
    L = 0
    for i in range(1,n+1):
        if L == 0 or X[M[1]] >= X[i]:
            # there is no j s.t. X[M[j]] < X[i]]
            j = 0
        else:
            # binary search for the largest j s.t. X[M[j]] < X[i]]
            lo = 1      # largest value known to be <= j
            hi = L+1    # smallest value known to be > j
            while lo < hi - 1:
                mid = (lo + hi)//2
                if X[M[mid]] < X[i]:
                    lo = mid
                else:
                    hi = mid
            j = lo

        P[i] = M[j]
        if j == L or X[i] < X[M[j+1]]:
            M[j+1] = i
            L = max(L,j+1)

    # Backtrack to find the optimal sequence in reverse order
    output = []
    pos = M[L]
    while L > 0:
        output.append(X[pos])
        pos = P[pos]
        L -= 1

    output.reverse()
    return output

def main():
    TEST_STRING = 'azcbobobegghakl'
    print(''.join(map(chr, longest_increasing_subsequence(map(ord, TEST_STRING)))))

if __name__ == '__main__':
    main()


Fixed size priority queue I've tried implementing priority queue with fixed size in Java using TreeSet and overriding add() method:

public class FixedSizePriorityQueue<E> extends TreeSet<E> {
    private final int capacity;

    public FixedSizePriorityQueue(final int capacity) {
        this.capacity = capacity;
    }

    public FixedSizePriorityQueue(
            final int capacity,
            final Comparator<? super E> comparator) {

        super(comparator);
        this.capacity = capacity;
    }

    @Override
    public boolean add(final E e) {
        // initialized with 0 or less than zero capacity
        if (capacity <= 0) {
            return false;
        }

        // keep adding until we fill the queue
        if (size() < capacity) {
            return super.add(e);
        }

        if (comparator() != null
                && comparator().compare(this.last(), e) < 0) {
            pollLast();
            return super.add(e);
        }

        return false;
    }
}

I would really appreciate your thoughts, hints and comments.

Two thoughts:

If there is other code and you need all the features of a TreeSet great, otherwise delegate to a TreeSet member variable.
Your code if (capacity <= 0) is somewhat superfluous because of the next test if (size() < capacity)

I rarely check the return of add methods. I would suggest throwing an exception to indicate the container is full.
Add methods that return available space and full state.


Determining if the kangaroos will land in the same position Problem

  There are two kangaroos on an x-axis ready to jump in the positive
  direction (i.e, toward positive infinity). The first kangaroo starts
  at location x1 and moves at a rate of v1 meters per jump. The second
  kangaroo starts at location x2 and moves at a rate of v2 meters per
  jump. Given the starting locations and movement rates for each
  kangaroo, can you determine if they'll ever land at the same location
  at the same time?

My Solution

#include <iostream>
using namespace std;

int main(){
    int x1, v1, x2, v2;
    cin >> x1 >> v1 >> x2 >> v2;

    // If one kangaroo is behind the other AND moving slower,
    //    he/she will never catch up to the other one
    if ((x1 < x2) && (v1 < v2)) cout << "NO";
    else if ((x2 < x1) && (v2 < v1)) cout << "NO";

    // Otherwise, move each kangaroo one jump at a time until
    //     the one behind is no longer behind.
    else {
        if (x1 < x2) {
            while (x1 < x2) {
                x1 += v1;
                x2 += v2;
            }
        } else {
            while (x2 < x1) {
                x1 += v1;
                x2 += v2;
            }
        }

        // Once he/she is no longer behind the other, check to see
        //    if he/she is in the same position, or if he/she has passed
        if (x1 == x2) cout << "YES";
        else cout << "NO";
    }
    return 0;
}

I am wondering if there is a way to optimize my code for this particular HackerRank problem. I have gotten all of the correct answers, and none of them timeout so for the problem it's 'good enough,' but I am curious if there is a better way of doing it.

Do I use too many conditionals, loops, etc? I have a bad habit of "do everything in for loops, if statements, etc."

Closed form solution

A better approach is to calculate the intercept point directly and then check whether or not it is an integer. The time of intercept is:

$$t = \frac{x_2 - x_1}{v_1 - v_2}$$

where both numerator and denominator are integers, but their ratio may not. We can also handle all special cases from the ratio.

bool kangaroos_meet(int x1, int v1, int x2, int v2) {
    int numerator = x2 - x1;
    int denominator = v1 - v2;
    if (denominator == 0)                // same velocity
    {
        return numerator == 0;           // they meet always or never
    }
    if (numerator % denominator != 0)    // intercept point not an integer
    {
        return false;
    }
    int t = numerator / denominator;     // calculate intercept point
    return t >= 0;                       // intercept point lies in the past
}

x1: initial location of kangaroo 1
& x2: initial location of kangaroo 2

If it is given that x2 > x1

static String kangaroo(int x1, int v1, int x2, int v2) {

    int i;
    for(i=0; x1<x2; i++){
        x1 = x1 + v1;
        x2 = x2 + v2;
        if(x1 == x2){
            return "YES";
        }
    }
    return "NO";
}

Below solution covers all the possibilities like, x1 > x2 or x1 == x2 or x1 < x2.

static String kangaroo(int x1, int v1, int x2, int v2) {

    int i, flag = 0;

    if(x1 > x2){
        for(i=0; x1>x2; i++){
            x1 = x1 + v1;
            x2 = x2 + v2;
            if(x1 == x2){
                return "YES";
            }
        }
    }else if(x1 < x2){
        for(i=0; x1<x2; i++){
            x1 = x1 + v1;
            x2 = x2 + v2;
            if(x1 == x2){
                return "YES";
            }
        }
    }else
        return "YES";

    return "NO";
}


Find the value nearest to k in the sorted array Given a sorted array, return the 'closest' element to the input 'x'.

I do understand the merits of unit testing in separate files, but I deliberately added it to the main method for personal convenience, so don't consider that in your feedback.

I'm looking for request code review, optimizations and best practices.

public final class ClosestToK {

    private ClosestToK() { }

    /**
     * Given a sorted array returns the 'closest' element to the input 'x'.
     * 'closest' is defined as Math.min(Math.abs(x), Math.abs(y))
     *
     * Expects a sorted array, and if array is not sorted then results are unpredictable.
     *
     * If two values are equi-distant then the greater value is returned.
     *
     * @param a  The sorted array a
     * @return   The nearest element
     */
    public static int getClosestK(int[] a, int x) {

        int low = 0;
        int high = a.length - 1;

        while (low <= high) {
            int mid = (low + high) / 2;

            // test lower case
            if (mid == 0) {
                if (a.length == 1) {
                    return a[0];
                }

                return Math.min(Math.abs(a[0] - x), Math.abs(a[1] - x)) + x;
            }

            // test upper case
            if (mid == a.length - 1) {
                return a[a.length - 1];
            }

            // test equality
            if (a[mid] == x || a[mid + 1] == x) {
                return x;
            }

            // test perfect range.
            if (a[mid] < x  && a[mid + 1] > x) {
                return Math.min(Math.abs(a[mid] - x), Math.abs(a[mid + 1] - x)) + x;
            }

            // keep searching.
            if (a[mid] < x) {
                low = mid + 1;
            } else {
                high = mid;
            }
        }

        throw new IllegalArgumentException("The array cannot be empty");
    }

    public static void main(String[] args) {
        // normal case.
        int[] a1 = {10, 20, 30, 40};
        assertEquals(30, getClosestK(a1, 28));

        // equidistant
        int[] a2 = {10, 20, 30, 40};
        assertEquals(30, getClosestK(a2, 25));

        // edge case lower boundary
        int[] a3 = {10, 20, 30, 40};
        assertEquals(10, getClosestK(a3, 5));
        int[] a4 = {10, 20, 30, 40};
        assertEquals(10, getClosestK(a4, 10));

        // edge case higher boundary
        int[] a5 = {10, 20, 30, 40};
        assertEquals(40, getClosestK(a5, 45));
        int[] a6 = {10, 20, 30, 40};
        assertEquals(40, getClosestK(a6, 40));

        // case equal to
        int[] a7 = {10, 20, 30, 40};
        assertEquals(30, getClosestK(a7, 30));

    }
}

Here's my solution which seems to work fine :

public static int getClosestK(int[] a, int x) {

    int low = 0;
    int high = a.length - 1;

    if (high < 0)
        throw new IllegalArgumentException("The array cannot be empty");

    while (low < high) {
        int mid = (low + high) / 2;
        assert(mid < high);
        int d1 = Math.abs(a[mid  ] - x);
        int d2 = Math.abs(a[mid+1] - x);
        if (d2 <= d1)
        {
            low = mid+1;
        }
        else
        {
            high = mid;
        }
    }
    return a[high];
}

Here's the principle : the interval [low, high] will contain the closest element to x at any time. At the beginning, this interval is the whole array ([low, high] = [0, length-1]). At each iteration, we'll make it strictly smaller. When the range is limited to a single element, this is the element we are looking for.

In order to make the range smaller, at each iteration, we'll consider mid the middle point of [low, high]. Because of the way mid is computed, mid+1 is also in the range. We'll check if the closest value is at mid or mid+1 and update high or low accordingly. One can check that the range actually gets smaller.

Edit to answer to comments:

As spotted by @Vick-Chijwani , this code doesn't handle perfectly the scenarios where an element appears multiple times in the input. One can add the following working tests to the code :

    // case similar elements
    int[] a8 = {10, 10, 10, 10, 10, 10, 10, 10, 10, 10, 10, 10, 10, 10, 10, 10, 10, 10, 10, 10, 10, 10, 10, 10, 10};
    assertEquals(10, getClosestK(a8, 9));
    assertEquals(10, getClosestK(a8, 10));
    assertEquals(10, getClosestK(a8, 11));

but this one will fail :

   int[] a9 = {1, 2, 100, 100, 101};
   assertEquals(3, getClosestK(a9, 2)); // actually returns 100

+1 for @Josay's answer.  In case the array a can contain equal entries, please consider this slight improvement:

public static int getClosestK(int[] a, int x) {
    int low = 0;
    int high = a.length - 1;

    if ( high < 0 )
        throw new IllegalArgumentException("The array cannot be empty");

    OUTER:
    while ( low < high ) {
        int mid = (low + high) / 2;
        assert ( mid < high );
        int d1 = Math.abs(a[mid  ] - x);
        int d2 = Math.abs(a[mid+1] - x);
        if ( d2 < d1 )
            low = mid + 1;
        else if ( d2 > d1 )
            high = mid;
        else { // --- handling "d1 == d2" ---
            for ( int right=mid+2; right<=high; right++ ) {
                d2 = Math.abs(a[right] - x);
                if ( d2 < d1 ) {
                    low = right;
                    continue OUTER;
                } else if ( d2 > d1 ) {
                    high = mid;
                    continue OUTER;
                }
            }
            high = mid;
        }
    }
    return a[high];
}

--- UPDATE ---

Why the hate (-1)? Handling d1 == d2 is important as the array is subset wrongly otherwise.

E.g., consider a=[1,2,2,3] and x=0. Then d1=d2=2 and Josay's answer sets low=mid+1 which subsets the array to [2,3]. This subset does not contain the correct answer (1) anymore.