Verifying that all form fields are filled in I grabbed the form from some random site because I'm only interested writing the javascript at the moment.

I am trying to check that a user has selected or entered text for all fields. I've made it a long if if-else but that can't be the best/most elegant/easiest solution.

Leaving aside the radio button validation for now, what's the better way to check that the text fields, drop down, and checkboxes all have a value/input?

I'm teaching myself javascript so I'm open to being told the proper way and I'll research it and do it on my own, or updating my fiddle would be fine too. (Be gentle with me. I'm sure this code is janky.)

Any thoughts on this would be appreciated.

Fiddle: https://jsfiddle.net/kiddigit/g0rur21a/

document.getElementById("newForm").addEventListener("submit", enterForm);

function enterForm(event) {
    event.preventDefault();

    var dropdown = document.getElementById('dropDown');

    if (document.getElementById('fname').value === ''){
            document.getElementById('fname').focus();
            alert('Enter text.');
        } else if (document.getElementById('eMail').value === ''){
            document.getElementById('eMail').focus();
            alert('Enter text.');
            } else if  (document.getElementById('textArea').value === '') {
                document.getElementById('textArea').focus();
                alert('Enter text.');
                } else if (!dropDown.value) {
                    document.getElementById('dropDown').focus();
                    alert('Choose an option.');
                    } else if ( ( newForm.checkbox[0].checked == false ) && ( newForm.checkbox[1].checked == false ) )
                        { alert ( "Please choose a checkbox" );
                            return false;
                            }

    var radios = document.getElementsByName("radio");
    var formValid = false;
    var i = 0;
    while (!formValid && i < radios.length) {
        if (radios[i].checked) formValid = true;
            i++;
        }

        if (!formValid) alert("Please check a radio button.");
        return formValid;

        return false;
};

Couple of suggestions from my end.

1 > There is a better way to check null or empty in javascript,this can be done more efficiently with this

 if (document.getElementById('fname').value)
 {
   //value exists.
 }

Above condition evaluates to true if value is not equal to any of the following:

null
undefined
NaN
empty string ("")
0
false

2 > Try to use switch case instead of so many if else.It will make your code more elegant and readable.

3 > Take care of indentation currently it's not very good.

4 > Instead of writing ewForm.checkbox[0].checked == false you can use

if(!ewForm.checkbox[0].checked)
{
//not checked
}


jQuery form with price format I have this jQuery form with 4 text inputs.
The first one only accepts digits, the second one formats value into 'de-DE' price format accepting 2 decimals, the second does the same but accepts 5 decimals after decimal separator, fourth one automatically adds periods for date input.

    $(window).ready(function () {
             function validateNumber(event) {
     var $txtBox = $(this);
     var key = window.event ? event.keyCode : event.which;
                      //activates the Backspace button and right/left arrows
     if (event.keyCode == 8 || event.keyCode == 9 || event.keyCode == 37 || event.keyCode == 39) {
      return true;
     } // prevent if not number/dot
      else if ( key < 44 || key > 57 && event.keyCode != 44) {
      event.preventDefault();
     }else {
     var len = $txtBox.val().length;
     var index = $txtBox.val().indexOf(',');

     if (index > 0 && event.keyCode == 44) {
     return false;
     }
     if (index > 0) {
     var charAfterdot = (len + 1) - index;
     if (charAfterdot > 6) {
      return false;
     }
    }
   }
      return $txtBox;
        };
            $('[id^=int]').keypress(validateNumber);
   $('[id^=float1]').keypress(validateNumber);
   $('[id^=float1]').on("blur", numberFormat);

     function numberFormat(event) {
        var number = parseFloat(this.value);
     console.log(this.value);
     console.log("before: " + number);
     console.log("after: " + number.toLocaleString('de-DE',{maximumFractionDigits: 5, style:'currency', currency:'EUR'}));
      this.value = isNaN(number) ? '0': number.toLocaleString('de-DE',{maximumFractionDigits:5, style:'currency', currency:'EUR'});
      var digits = maximumFractionDigits;
    };
   $('[id^=float2]').keypress(validateNumber);
   $('[id^=float2]').on("blur", numberFormat);
     var dt = dateFormatter('#date');
       $('[id^=date]').keypress(validateNumber);
        });
<!DOCTYPE html>

 <meta charset="utf-8"/>

  <form style="display:block;margin:0 auto;border:1px solid #aaa;border-radius:10px;width:215px;padding:10px;margin-top:100px;background-color:#F0F0F0;box-shadow:2px 2px 2px 2px rgba(0,0,0,0.2);">

     Number:
     <input id="int" type="input" value="0" size="5" maxlength="13" style=width:100px;margin-left:5px;,/>

     FloatComma1:
     <input id="float1" decimalPlaces="2" type="text" maxlength="13" style=width:100px;margin-left:5px;/>

     FloatComma2:
     <input id="float2" decimalPlaces="5" type="text" maxlength="13" style=width:100px;margin-left:5px;/>

     Date:
     <input id="date" type="text" value="" maxlength="10"  style=width:100px;margin-left:5px; />

  </form>

As you can see, the FloatComma1 variable is allowing more than 2 decimals because my numberFormat function has this.value = number.toLocaleString('de-DE',{maximumFractionDigits:5});, so it allows 5.

Does anyone know a way how I could just replace the 5 with, let´s say a variable decimals, which I can give separate values to?
For example, I could call the variable in FloatComma1 decimals = 3 , and in FloatComma2 decimals = 5?

I do not know how to do it. Can someone help?

I'm solely going to post on your coding structure rather than the method of procedure for the task your code does.

In validateNumber(), your indentation starts out good and then goes downhill from there.

}else {
var len = $txtBox.val().length;
var index = $txtBox.val().indexOf(',');
if (index > 0 && event.keyCode == 44) {
return false;
}
if (index > 0) {
var charAfterdot = (len + 1) - index;
if (charAfterdot > 6) {
        return false;
}

with whitespace added would transform into:

} else {
    var len = $txtBox.val().length;
    var index = $txtBox.val().indexOf(',');
    if (index > 0 && event.keyCode == 44) {
        return false;
    }
    if (index > 0) {
        var charAfterdot = (len + 1) - index;
        if (charAfterdot > 6) {
            return false;
        }

Your naming conventions could be improved also:
For example, charAfterdot into charAfterDot.

Instead of blindly comparing random event.keyCodes with numbers, declare the numbers before hand as constants.

const BACKSPACE = 8;
const TAB = 9;
const LEFT_ARROW = 37;
const RIGHT_ARROW = 39;

However, your version of JavaScript may not support constants, so use var in that case, and then just:

if (event.keyCode == BACKSPACE || event.keyCode == TAB || event.keyCode == LEFT_ARROW || event.keyCode == RIGHT_ARROW)

Declaring constants is a much clearer way of programming, than comparing to random numbers.


applyFilters - function which take array of items and array of excludes and return filtered by excludes array of items I wrote some function which should take an array of items:Array<Object> and array of excludes:Array<Object> with keys which values can't be present in items. and should return an array of filtered items.

Is there some way to improve it? Is complexity of that function equal O(n + k)?

const items = [
  { type: 'phone', name: 'iPhone', color: 'red' },
  { type: 'laptop', name: 'Chromebook', color: 'gray' },
  // 15,000 items
];

const excludes = [
  { k: 'color', v: 'gold' },
  { k: 'type', v: 'laptop' },
  // 1,000 items
];

const applyFilters = (items, excludes) => {
  const newExcludes = excludes.reduce((acc, curr) => {
    if (!acc[curr.k])
      acc[curr.k] = new Set();

    acc[curr.k].add(curr.v);

    return acc;
  }, {});

  return items.reduce((acc, curr) => {
    for (let key in curr) {
      if (newExcludes[key] && newExcludes[key].has(curr[key]))
        return acc;
    }

    return [...acc, curr];
  }, []);
};

console.log(applyFilters(items, excludes));

I believe you should take a bit more time on documenting what the applyFilters functionality requires from input parameters. You seem to have pretty specific requirements which would be good to have them documented. You could document them using JsDoc syntax in the following way

/**
 * @method applyFilters
 * @param { object[] } items: An array of objects
 * @param { {k: string, v: object}[] } exludes: An array of objects containing key that need to be checked for certain values
 * @returns { object[] } A filtered array of objects
 */
 const applyFilters = (items, excludes) => { /*...*/ };

I think the algorithmn can be improved however with 1 very simple change, nl:

const applyFilters = (items, excludes) => {
  const newExcludes = excludes.reduce((acc, curr) => {
    if (!acc[curr.k])
      acc[curr.k] = new Set();

    acc[curr.k].add(curr.v);

    return acc;
  }, {});

  return items.reduce((acc, curr) => {
    for (let key in curr) {
      if (newExcludes[key] && newExcludes[key].has(curr[key]))
        return acc;
    }
    // using [...acc, curr] is very expensive here
    acc.push(curr);
    return acc;
  }, []);
};

The operation [...acc, curr] is a very expensive operation to use inside a reduce function. Since you start your operation with an empty arr and you only wish to add 1 item, I don't see why you use the simpler version of pushing an item and returning the existing array. The reason why it is that expensive is because it creates a complete copy of the previous array and just add's one item.

As an addition, I also made a small test that clearly shows the difference in performance, the test generates a set of 1500 items and around a 100 exclude filters, and clearly shows the difference in performance by taking out the expensive copy the spread operator introduces

/**
 * @method applyFilters
 * @param { object[] } items: An array of objects
 * @param { {k: string, v: object}[] } exludes: An array of objects containing key that need to be checked for certain values
 * @returns { object[] } A filtered array of objects
 */
const applyFilters = (items, excludes) => {
  const newExcludes = excludes.reduce((acc, curr) => {
    if (!acc[curr.k])
      acc[curr.k] = new Set();

    acc[curr.k].add(curr.v);

    return acc;
  }, {});

  return items.reduce((acc, curr) => {
    for (let key in curr) {
      if (newExcludes[key] && newExcludes[key].has(curr[key]))
        return acc;
    }
  return [...acc, curr];
  }, []);
};

/**
 * @method applyFiltersNoSpread
 * @param { object[] } items: An array of objects
 * @param { {k: string, v: object}[] } exludes: An array of objects containing key that need to be checked for certain values
 * @returns { object[] } A filtered array of objects
 */
const applyFiltersNoSpread = (items, excludes) => {
  const newExcludes = excludes.reduce((acc, curr) => {
    if (!acc[curr.k])
      acc[curr.k] = new Set();

    acc[curr.k].add(curr.v);

    return acc;
  }, {});

  return items.reduce((acc, curr) => {
    for (let key in curr) {
      if (newExcludes[key] && newExcludes[key].has(curr[key]))
        return acc;
    }
  acc.push(curr);
    return acc;
  }, []);
};

/**
 * @method testScenario
 * @param { number } runTest: how many time should the test be repeated
 */
const testScenario = function(runTest) {

  /**
   * @method generateItems
   * @param { number } totalItems: how many items should be generated
   * @param { object } template: an object describing how the items should look like
   * @param { string[]=* } on or more arrays that provide values for the template to be filled
   * @returns { object[] }
   */
  function generateItems(totalItems, template) {

    let result = [];
    for (let i = 0; i < totalItems; i++) {
      let item = {};
      for (let field in template) {
        let target = arguments[template[field]];
        let index = parseInt(Math.random() * target.length);
        item[field] = target[index];
      }
      result.push(item);
    }
    return result;
  }

  // this will create some larger seeds
  console.time('generate_items');
  const items = generateItems(
    1500, {
      type: 2,
      name: 3,
      color: 4
    }, ['phone', 'laptop', 'computer', 'netbook', 'tablet'], ['iPhone', 'Sony', 'Acer', 'Asus', 'MSI'], ['red', 'gray', 'pink', 'gold', 'silver', 'metalic', 'orange']
  );
  console.timeEnd('generate_items');

  console.time('generate_filters');
  const excludes = generateItems(
    100, {
      k: 2,
      v: 3
    }, ['color', 'type'], ['red', 'black', 'metalic', 'sony', 'iPhone']
  );
  console.timeEnd('generate_filters');

  for (let i = 0; i < runTest; i++) {
    console.log(`running test loop ${i}`);
    console.time('withSpread');
    let setResults = applyFilters(items, excludes);
    console.timeEnd('withSpread');

    console.time('noSpread');
    let noSpread = applyFiltersNoSpread(items, excludes);
    console.timeEnd('noSpread');

    !(noSpread.length === setResults.length && noSpread[0] === setResults[0] && noSpread[noSpread.length] === setResults[setResults.length]) && console.error('results do not match');
  }
};

testScenario(10);


How can I make this C code for checking checksum shorter? This is a function for checking if the checksum provided by card[] is valid. card[] contains six pairs of characters. The first five pairs make up a hexadecimal number, and the last pair contains the checksum. The checksum is valid if it is equal to XOR of the first five pairs.

It is working fine, but the code is awful. I was trying to use a nested loop, but it wasn't working. Now I'm getting pairs from card[], converting them into numbers and checking whether the checksum is valid.

bool checksum(char card[])
{
int a=0;
char character0[2];
char character1[2];
char character2[2];
char character3[2];
char character4[2];
char character5[2];
long n0,n1,n2,n3,n4,n5;
char card_number;

    for(int i=0;i<2;i++)
    {
        for(a=0;a<2;a++)
        {
        character0[a]=card[i];
        }
    }
    for(int i=2;i<4;i++)
    {
        for(a=0;a<2;a++)
        {
            character1[a]=card[i];
        }
    }
    for(int i=4;i<6;i++)
    {
        for(a=0;a<2;a++)
        {
            character2[a]=card[i];
        }
    }
    for(int i=6;i<8;i++)
    {
        for(a=0;a<2;a++)
        {
            character3[a]=card[i];
        }
    }
    for(int i=8;i<10;i++)
    {
        for(a=0;a<2;a++)
        {
            character4[a]=card[i];
        }
    }
    for(int i=10;i<12;i++)
    {
        for(a=0;a<2;a++)
        {
            character5[a]=card[i];
        }
    }
    n0 = strtol(character0, NULL, 16);
    n1 = strtol(character1, NULL, 16);
    n2 = strtol(character2, NULL, 16);
    n3 = strtol(character3, NULL, 16);
    n4 = strtol(character4, NULL, 16);
    n5 = strtol(character5, NULL, 16);

if(n0^n1^n2^n3^n4==n5)     return true;
else return false;

}

The input for the example is "1E00EDE5E5F3", so 1E^00^ED^E5^E5 should be F3.

And something is wrong with this code. I see it now, because

if(n0^n1^n2^n3^n4==n5)     return true;
else return false;

is working good, but

if((n0^n1^n2^n3^n4)==n5))     return true;
else return false;

is not working at all.

Use a 2D array instead of 6 1D arrays (vs. character0, character1, ...)

Use a 1D array for n instead of (n0, n1, ...)

Add EOS termination to the string that gets passed to strtol

Here's a refactored version:

bool
checksum(char card[])
{
    char chars[6][3];
    long nx[6];

    for (int col = 0;  col < 6;  ++col) {
        int lo = col << 1;
        int hi = lo + 2;
        for (int i = lo;  i < hi;  ++i)
            chars[col][i - lo] = card[i];
    }

    for (int i = 0;  i < 6;  ++i) {
        char *ptr = chars[i];
        ptr[2] = 0;
        nx[i] = strtol(ptr,NULL,16);
    }

    if (nx[0] ^ nx[1] ^ nx[2] ^ nx[3] ^ nx[4] == nx[5])
        return true;
    else
        return false;
}

UPDATE:

Here's a cleaner/simpler version:

bool
checksum(char card[])
{
    char tmp[3];
    long nx[6];

    tmp[2] = 0;
    for (int col = 0;  col < 6;  ++col) {
        int lo = col << 1;

        tmp[0] = card[lo + 0];
        tmp[1] = card[lo + 1];

        nx[col] = strtol(tmp,NULL,16);
    }

    if (nx[0] ^ nx[1] ^ nx[2] ^ nx[3] ^ nx[4] == nx[5])
        return true;
    else
        return false;
}

UPDATE #2:

Here's an even simpler version:

bool
checksum(char card[])
{
    char tmp[3];
    long nx;

    tmp[2] = 0;
    nx = 0;
    for (int idx = 0;  idx < 12;  idx += 2) {
        tmp[0] = card[idx + 0];
        tmp[1] = card[idx + 1];

        nx ^= strtol(tmp,NULL,16);
    }

    if (nx == 0)
        return true;
    else
        return false;
}

UPDATE #3:

Based on some feedback ...

The first two examples above, were to eliminate "parallel scalar" variables (e.g.) v0, v1, v2, ... vN in favor of an array: v[N+1]. This allowed replicated code to be replaced with loops. OP's code had two such instances for character* and n* variables, so I converted both to arrays.

When starting out [as a programmer], when to use an array isn't always obvious [particularly for small numbers]. In the above case, N was 6. So, the code could be built up by cut-and-paste.

If, however, N had been a much larger number, say, 1000, the original code would then not scale well. And, the array solution would have become [more] obvious.

OP's code was trying to copy two bytes from the buffer into different char arrays of the form character* in the first code block [to add an EOS char to allow strtol to work]. This still had a bug because there was no space for the EOS.

OP's second block would use strtol on the intermediate character* variables to produce n* variables.

My first example [in my original post], did the conversion from separate scalar variables to arrays.

My second example [in my first update], combined both blocks/loops into one, so that character* [which I had replaced with the 2D chars array], could be eliminated with a single tmp array.

When I did my example in update #2, I assumed that OP's algorithm was correct. I didn't realize that:

if (nx[0] ^ nx[1] ^ nx[2] ^ nx[3] ^ nx[4] == nx[5])

was being interpreted [by the compiler, based on precedence] as:

if (nx[0] ^ nx[1] ^ nx[2] ^ nx[3] ^ (nx[4] == nx[5]))

I assumed it was grouped as:

if ((nx[0] ^ nx[1] ^ nx[2] ^ nx[3] ^ nx[4]) == nx[5])

Because that's what made sense for the CRC calculations [and I assumed OP had done it correctly].

My refinement [to eliminate the nx array in favor of a running CRC], was based on the following identity:

(x == y) === ((x ^ y) == 0)

So, XORing all values (including the checksum), if the message was correct/intact, would produce a final value of zero. So, by doing this, I fixed OP's second bug, based on some serendipity.

Others have pointed out that:

if (nx == 0)
    return true;
else
    return false;

Can be replaced with:

return (nx == 0);

I had debated doing that, but decided that the example was already far afield from OP's original and that it would be clearer to leave the return sequence as it was. And, the optimizer would [probably] produce the same exact code for both.

At that point, I had debated coming up with a hex function that decoded a single hex char as others have suggested, calling it twice and eliminating the copy to tmp and call to strtol, but, again, felt I was getting far enough away from the original code.

But, just for the sake of completeness, here is my final/best example, generalized to allow an arbitrary buffer size:

unsigned int
hex(unsigned int chr)
{

    // NOTE: hopefully, this function gets inlined ...

    do {
        if ((chr >= '0') && (chr <= '9')) {
            chr -= '0';
            break;
        }

        chr = tolower(chr);

        if ((chr >= 'a') && (chr <= 'f')) {
            chr -= 'a';
            chr += 10;
            break;
        }

        // should blow up here (but there was no error checking in original)
        chr = 0;
    } while (0);

    return chr;
}

bool
checksum(const char *card,size_t len)
{
    unsigned int cur;
    unsigned int crc = 0;

    for (size_t idx = 0;  idx < len;  idx += 2) {
        cur = hex(card[idx + 0]);
        cur <<= 4;

        cur |= hex(card[idx + 1]);

        crc ^= cur;
    }

    return (crc == 0);
}

Note that even this could be tweaked a bit more for speed with some careful benchmarking ...

UPDATE #4:

Eliminating the call to hex in favor of a [single] table lookup may be faster and provide some error checking:

int
checksum(const char *card,size_t len)
{
    static unsigned char hex[256] = { ['0'] = 0xFF };
    unsigned int chr;
    unsigned int cur;
    unsigned int crc = 0;

    // one time init of translation table
    if (hex['0'] == 0xFF) {
        for (chr = 0x00;  chr <= 0xFF;  ++chr)
            hex[chr] = 0xFF;

        for (chr = 0;  chr <= 9;  ++chr)
            hex[chr + '0'] = chr;

        for (chr = 0x00;  chr <= 0x05;  ++chr) {
            hex[chr + 'a'] = chr + 0x0A;
            hex[chr + 'A'] = chr + 0x0A;
        }
    }

    for (size_t idx = 0;  idx < len;  idx += 2) {
        chr = hex[card[idx + 0]];
#ifdef ABORT_ON_ERROR
        if (chr == 0xFF)
            return -1;
#endif
        cur = chr;
        cur <<= 4;

        chr = hex[card[idx + 1]];
#ifdef ABORT_ON_ERROR
        if (chr == 0xFF)
            return -1;
#endif
        cur |= chr;

        crc ^= cur;
    }

    return (crc == 0);
}


Insertion sort in Rust Here is the code:

pub fn insertion_sort<T>(vec: &mut Vec<T>) where T: Ord + Copy {
    fn insert<U>(vec: &mut Vec<U>, pos: usize, value: U) where U: Ord + Copy {
        assert!(pos > 0);
        let mut pos: usize = pos - 1;
        loop {
            let value_at_pos = vec[pos];
            if value_at_pos <= value {
                break;
            }
            vec[pos + 1] = value_at_pos;
            if pos == 0 {
                vec[pos] = value;
                return ();
            }
            pos -= 1;
        }
        vec[pos + 1] = value;
    }
    for i in 1..vec.len() {
        let value = vec[i];
        insert(vec, i, value);
    }
}

#[test]
fn test_insertion_sort() {
    let mut vec = vec![9, 8, 7, 11, 10];
    insertion_sort(&mut vec);
    let vec_res: Vec<_> = (7..12).collect();
    assert_eq!(vec, vec_res);
}

It's slightly more complex than in textbooks due to the fact negative integers are not allowed for indexing in Rust, not a huge problem though.

As you have already been made aware, you should not be using &mut Vec<T> unless you plan on adding or removing items from the Vec. Using &mut [T] better expresses the contract of the function and is more flexible, allowing you to also sort arrays and anything else that can be expressed as a slice.
where clauses go on a separate line. This allows them to be easily found, which is important considering how much they affect the behavior of the function.
There's no need to declare the type of pos. Type inference will take care of it.
There's no need to redeclare pos just to make it mutable and decrement it. Just make the variable binding in the function declaration mut.
There's no need to return the unit value (()). Just return will suffice.
slice::swap exists. In the broader world, so does mem::swap.
With the power of swap, you can remove the need for the Copy bound.
Quickcheck is an invaluable tool for problems like this. You can create a property that can be validated across a wide range of automatically generated input.

pub fn insertion_sort<T>(values: &mut [T])
    where T: Ord
{
    for i in 0..values.len() {
        for j in (0..i).rev() {
            if values[j] >= values[j + 1] {
                values.swap(j, j + 1);
            } else {
                break
            }
        }
    }
}

#[macro_use]
extern crate quickcheck;

#[test]
fn test_insertion_sort_empty() {
    let mut values: [i32; 0] = [];
    insertion_sort(&mut values);
    assert_eq!(values, [])
}

#[test]
fn test_insertion_sort_one() {
    let mut values = [1];
    insertion_sort(&mut values);
    assert_eq!(values, [1]);
}

#[test]
fn test_insertion_multi() {
    let mut values = [9, 8, 7, 11, 10];
    insertion_sort(&mut values);
    let values_expected: Vec<_> = (7..12).collect();
    assert_eq!(values_expected, values);
}

quickcheck! {
    fn test_insertion_everything(xs: Vec<i32>) -> bool {
        // Macro doesn't allow `mut` in the `fn` declaration :-(
        let mut xs = xs;

        let mut expected_sorted = xs.clone();
        expected_sorted.sort();

        insertion_sort(&mut xs);

        expected_sorted == xs
    }
}


More functional approach I wonder if there is a more "functional" way of implementing something like this:

@months.zipWithIndex.foreach { case (month, index) =>
    if (index%3 == 0) {

    }
    @month
    if (index%3 == 0) {

    }
}

Can these two if-statements be removed by using something other than foreach?

I am not sure which framework you are using, but this is how you could do it using Scala´s native XML library:

 { for (row <- months grouped 3) yield (
 { for (month <- row) yield ( {month} 	) }
) }

Rendered slightly more prettily, the code looks like this:

for (row <- months grouped 3) yield {

  { for (month <- row) yield ( {month} 	) }

}

Which hopefully makes the structure clear.

If your framework works with yield, all you need to do is add @ in the appropriate places.  If it does not, you might have to do something like this:

for (row <- @months grouped 3) {

  for (month <- @row) {
     @month
  }

}

Your code is structured only around months, so has to use conditional logic to create your rows.  My code is structured around rows (and within that around months), so needs no conditionals.  My code also only has the value for the number of months in one place, which is less error prone and makes it easier to change the number of months in a row.

You don't have to use a list comprehension, though.  The important thing is simply to slice the list up into groups of 3 (or n, where n is the number of months you want in each row) and iterate over those.


Finding the date from "2nd Friday of X month"-style input I've been working through some puzzles from exercism.io. Current puzzle asks me to return a date() from input that essentially says "The 3rd Friday of December" or "The 1st Monday of October". I'd love to hear some feedback on how to clean this code up:

def meetup_day(year, month, weekday, spec_weekday):
    import calendar
    from datetime import date

    last_day = calendar.monthrange(year, month)[1]
    wkday = {'Monday': 0, 'Tuesday': 1, 'Wednesday': 2, 'Thursday': 3, 'Friday': 4, 'Saturday': 5, 'Sunday': 6}
    schedule_day = wkday[weekday]

    if spec_weekday == "teenth":
        check_range = range(13, 20)
    elif spec_weekday == "last":
        check_range = range(last_day - 6, last_day + 1)
    else:
        spec_weekday = int(spec_weekday[0:1])
        check_range = range(7 * spec_weekday - 6, 7 * spec_weekday + 1)

    for index in check_range:
        if index > last_day:
            break
        if schedule_day == calendar.weekday(year, month, index):
            schedule_day = index

    return date(year, month, schedule_day)

meetup_day(2014, 3, "Friday", "2nd")
meetup_day(2013, 6, "Wednesday", "4th")
meetup_day(2013, 12, "Monday", "1st")
meetup_day(2015, 5, "Tuesday", "teenth")
meetup_day(2015, 4, "Thursday", "last")

I'm still fairly new to Python, and datetime stuff throws me through a loop. I chose to use calendar as well as date because I thought the syntax was easier to understand. However, I'm definitely open to ideas on more efficient ways to do this.

Quick note: The "teenth" thing:

  Note that "Monteenth", "Tuesteenth", etc are all made up words. There
  was a meetup whose members realised that there are exactly 7 days that
  end in '-teenth'. Therefore, one is guaranteed that each day of the
  week (Monday, Tuesday, ...) will have exactly one date that is named
  with '-teenth' in every month.

Your code is pretty clear but can be improved using builtin capabilities of calendar: the monthcalendar method return a list of list representing each week of the selected month (zeroes are days that are not in the month). By filtering these weeks using the index of the day you are looking for, you get all the possible day of the month that are the requested day. You can then just pick the right one into this list.

Python related remarks:

Put your import at the beginning of the file, not into functions. It will slow you down if you are heavily calling functions that contain import statements;
wkday is a constant and must be declared as such: outside of the function and with proper case;
there is no really need to make wkday a dictionary given that you can get the same information it holds using .index on a list;
[optional] depending on your needs, you may want to perform proper input validations.

import calendar
from datetime import date

DAY_NAMES = [day for day in calendar.day_name]

def meetup_day(year, month, weekday, spec_weekday):
    day_index = DAY_NAMES.index(weekday)
    possible_dates = [
        week[day_index]
        for week in calendar.monthcalendar(year, month)
        if week[day_index]] # remove zeroes

    if spec_weekday == 'teenth':
        for day_num in possible_dates:
            if 13 <= day_num <= 19:
                return date(year, month, day_num)
    elif spec_weekday == 'last':
        day_index = -1
    elif spec_weekday == 'first':
        day_index = 0
    else:
        day_index = int(spec_weekday[0]) - 1
    return date(year, month, possible_dates[day_index])

meetup_day(2014, 3, "Friday", "2nd")
meetup_day(2013, 6, "Wednesday", "4th")
meetup_day(2013, 12, "Monday", "1st")
meetup_day(2015, 5, "Tuesday", "teenth")
meetup_day(2015, 4, "Thursday", "last")


A Tour of Go, problem #56 - error handling New to Go, trying to solve Tour of Go, Exercise 56, which is about error handling.

Can the following error handling method can be further improved?

package main

import (
    "fmt"
    "math"
)

type ErrNegativeSqrt float64

func (e ErrNegativeSqrt) Error() string {
    return fmt.Sprintf("cannot Sqrt negative number: %f", float64(e))
}

func Sqrt(f float64) (float64, error) {
    if f > 0 {
        return math.Sqrt(f), nil
    } else {
        return 0, ErrNegativeSqrt(f)
    }
}

func main() {
    fmt.Println(Sqrt(2))
    fmt.Println(Sqrt(-2))
}

I would name the error ErrSqrtNegative instead of ErrNegativeSqrt.

Sqrt(0) should not be an error.

In case of error, I would use math.NaN(), ErrSqrtNegative(f) as the return values.


Checking if two strings are anagrams I have a piece of code that checks if two strings are anagrams. I would like this to be as pythonic as possible.

def are_two_string_anagram(string1, string2):
    if len(string1) != len(string2):
        return False

    def char_count(string):
        char_count = {}
        for s in string:
            char_count[s] = char_count.get(s, 0) + 1
        return char_count

    chr_count = char_count(string1)
    chr1_count = char_count(string2)
    return chr_count == chr1_count

string1 = "mary"
string2 = "yram"
print(are_two_string_anagram(string1, string2))

string1 = "albert"
string2 = "abelrt"
print(are_two_string_anagram(string1, string2))

string1 = "something"
string2 = "anotherus"
print(are_two_string_anagram(string1, string2))

Most python users would just use sorted(string1) == sorted(string2). That's \$O(n log n)\$, which is not that much worse than your \$O(n)\$ solution.

But the code you've actually written is just a reimplementation of:

collections.Counter(string1) == collections.Counter(string2)

That said, your short-circuit if the lengths mismatch is a good idea no matter which implementation you use.

There are some style issues that jump out with your implementation:

char_count doesn't have a pressing need to be a nested function, I would put it at top level.
Don't have a local variable named char_count inside the same-named function.
Don't cross numbers like chr_count = char_count(string1);chr1_count = char_count(string2). If you need numbered variables, use the same number for all transformed forms also.

For your demonstration code, I would have written:

def test_two_string_anagrams():
    assert are_two_string_anagram("mary", "yram")
    assert are_two_string_anagram("albert", "abelrt")
    assert are_two_string_anagram("something", "anotherus")

Or possibly:

def test_two_string_anagrams():
    for s1, s2 in [
        ("mary", "yram"),
        ("albert", "abelrt"),
        ("something", "anotherus"),

    ]:
        assert are_two_string_anagram(s1, s2)

That way it can be run directly with py.test (and possibly other harnesses, I know the standard unittest requires wrapping them in a dummy class, but I haven't used any others).


HackerRank "Service Lane" in Python Problem statement:

  Calvin is driving his favorite vehicle on the 101 freeway. He notices that the check engine light of his vehicle is on, and he wants to service it immediately to avoid any risks. Luckily, a service lane runs parallel to the highway. The length of the highway and the service lane is N units. The service lane consists of N segments of unit length, where each segment can have different widths.

  Calvin can enter into and exit from any segment. Let's call the entry segment as index i and the exit segment as index j. Assume that the exit segment lies after the entry segment(j>i) and i ≥ 0. Calvin has to pass through all segments from index i to indexj (both inclusive).

  Calvin has three types of vehicles - bike, car and truck, represented by 1, 2 and 3 respectively. These numbers also denote the width of the vehicle. We are given an array width[] of length N, where width[k] represents the width of kth segment of our service lane. It is guaranteed that while servicing he can pass through at most 1000 segments, including entry and exit segments.

  If width[k] is 1, only the bike can pass through kth segment.
  If width[k] is 2, the bike and car can pass through kth segment.
  If width[k] is 3, any of the bike, car or truck can pass through kth segment.

  Given the entry and exit point of Calvin's vehicle in the service lane, output the type of largest vehicle which can pass through the service lane (including the entry & exit segment)

  Input Format

  The first line of input contains two integers - N & T, where N is the length of the freeway, and T is the number of test cases. The next line has N space separated integers which represents the width array.

  T test cases follow. Each test case contains two integers - i & j, where i is the index of segment through which Calvin enters the service lane and j is the index of the lane segment where he exits.

  Output Format

  For each test case, print the number that represents the largest vehicle type that can pass through the service lane.

  Note:
  Calvin has to pass through all segments from index i to index j (both inclusive).

  Constraints:

  2 <= N <= 100000

  1 <= T <= 1000

  0 <= i < j < N

  2 <= j-i+1 <= min(N,1000)

  1 <= width[k] <= 3, where 0 <= k < N

Solution:

N, T = map(int, input().split())
widths = list(map(int, input().split()))
for _ in range(T):
    i, j = map(int, input().split())
    vehicle = 3
    for x in range(i, j+1):
        width = widths[x]
        if width == 1:
            vehicle = 1 # Only a bike can pass
            break
        elif width == 2:
            vehicle = 2 # Only a car or bike can pass
    print(vehicle)

I'm looking for any way to make this shorter, simpler or more Pythonic.

Shorter, simpler, and more Pythonic would be:

N, T = map(int, input().split())
widths = list(map(int, input().split()))
for _ in range(T):
    i, j = map(int, input().split())
    vehicle = min(widths[segment] for segment in range(i, j + 1))
    print(vehicle)

You would miss out on the shortcut when encountering a bike-only segment, though.

I've also renamed x to segment for clarity.


Find the smallest number whose sum of digits add to a number Request for review, optimization, best practice recommendations etc.

/**
 * Find the smallest positive number whose individual digits adds to the input number.
 * eg: consider the input 14,
 * The smallest number whose digit add to the given number is 59 ie 5 + 9 = 14.
 *
 * Complexity: O(n), where n is the input number
 *
 */
public class SmallestNumber {

    private SmallestNumber() {}

    private static int getIntNum(int x, int y) {
        String number = new StringBuilder().append(x).append(y).toString();
        return Integer.parseInt(number);
    }

    /**
     * Given an integer, returns the smallest positive number whose digits adds up to that number.
     *
     * @param x the number used to find another number whose digits add up to itself
     * @returns the smallest positive number whose digits adds up to input number.
     */
    public static int getVal(int x) {
        x = Math.abs(x);
        int min = Integer.MAX_VALUE;

        for (int i = 1; i <= x / 2; i++) {
            int diff = x - i;

            int p1 = getIntNum(i, diff);
            int p2 = getIntNum(diff, i);

            /**
             * http://stackoverflow.com/questions/9576557/most-efficient-way-to-
             * find-smallest-of-3-numbers-java
             */
            if (p1 < min) min = p1;
            if (p2 < min) min = p2;
        }
        return min;
    }

    /**
     * http://stackoverflow.com/questions/2674707/how-to-concatenate-int-values-
     * in-java
     *
     * @param args
     */
    public static void main(String[] args) {
       int x1 = getVal(14);
       System.out.println("Expected 59, Actual: " + x1);

       int x2 = getVal(16);
       System.out.println("Expected 79, Actual: " + x2);
    }
}

I am struggling to understand your algorithm. I don't fathom why you have to go through all that work of adding digits to values when the math behind this problem is so much simpler.

The smallest value will always be a digit followed by nines. The number of nines is the number of whole times 9 divides in to the value, and the first digit is the remainder.

Mathematically this translates to like:

public static int getVal(int val) {
    val = Math.abs(val);
    int ninecount =  val / 9;
    int remainder = val % 9;
    int scale = (int)Math.pow(10, ninecount);
    int result = remainder * scale + (scale - 1);
    return result;
}

Which is an O(1) operation (or, as has been pointed out by James, the complexity of this function is dependent on the complexity of Math.pow(...,...)). If you want to keep O(1) complexity you can precompute the scale for valid ranges of ninecount. The largest possible ninecount for an int result is 9, and for a long result it is 19. This is a small data set to keep in an indexed array accessed with int scale = scales[ninecount];


A simple jQuery function that removes empty or tags containing just ' ' I've made a simple function, this time in jQuery rather than JS, that just removes empty

, span and strong tags. I'm curious to see what everyone thinks about my solution as I'm relatively new to jQuery, and whether it could be improved. My original solution is as follows:

$('p').filter(function() {
 return ($(this).html().trim() == '&nbsp;');
}).remove();
$('span').filter(function() {
 return ($(this).html().trim() == '&nbsp;');
}).remove();
$('span').filter(function() {
 return ($(this).html().trim() == '');
}).remove();
$('strong').filter(function() {
 return ($(this).html().trim() == '');
}).remove();
$('p').filter(function() {
 return ($(this).html().trim() == '');
}).remove();

This isn't exactly dry... So I made a couple of other solutions:

/* MY DRIER SOLUTION 1 */
$.fn.jCleaner = function() {
    return $(this).filter(function() {
        return ($(this).text().trim() === '&nbsp;' || $(this).text().trim() === '');
        }).remove();
}
$('p, span, strong').jCleaner();

/* MY DRIER SOLUTION 2 */
function jCleaner() {
    return $(this).filter(function() {
        return ($(this).text().trim() == '&nbsp;' || $(this).text().trim() === '');
        }).remove();
}
$('p, span, strong').each(jCleaner);

And the original HTML is:

/* ORIGINAL HTML */

text

&nbsp;

text

text

text

text

&nbsp;

text

&nbsp;

text

&nbsp;

text

&nbsp;

text

So my questions are essentially how can my solutions be improved (if at all)? I had to write an entire function and then call it on each element. Thanks for any advice here.

Note: here's the solution in JS:

document.querySelectorAll("span, p, strong")
    .forEach(el => el.textContent.trim() === "" && el.parentNode.removeChild(el))

"This isn't exactly DRY" is pretty much what I think about

$(this).text().trim() == '&nbsp;' || $(this).text().trim() === ''

text() can be a very expensive call: I would refactor to only do $(this).text().trim() once.

As a minor concern, the use of trim() suggests that you want to remove nodes whose text is purely whitespace. So shouldn't a node whose text is &nsbp; &nbsp; be removed? Perhaps you should be using a regex match instead of trim()? Something like (warning: untested) $(this).text().match(/^(\s|&nbsp;)*$/).

And then you could take it further: &nbsp; isn't the only way of escaping U+00a0, and the other whitespace characters can also be escaped in various ways...


Enum conversion to Enumeration class with small Business Logic inside I had old enum, I converted it to Enumeration class and added one method inside (isValid). I am not sure if it is good approach to add such small business logic into Enumeration class. I would like to understand if this is a good approach or not.

Here is old enum:

internal enum Rule
{
    None = 0,

    CompensationIsZero = 1,

    CompensationLessThanDemand = 2,

    CompensationBetweenZeroAndDemand = 3,

    CompensationEqualsDemand = 4,

    CompensationLessOrEqualsDemand = 5
}

Here it is rewritten to Enumeration class and IsValid method is added.

 public class Rule : Enumeration
{
        public static readonly Rule None = new Rule (0, "None");
        public static readonly Rule CompensationIsZero  = new Rule (1, "CompensationIsZero ");
        public static readonly Rule CompensationLessThanDemand = new Rule(2, "CompensationLessThanDemand ");
       ....
       // there are about 5 rules

       public Rule(int id, string name)
        : base(id, name)
       { }

        public bool IsValid(decimal compensation, decimal demand, int rule) {
                if (rule == Rule.None.Id) return true;
                if (rule == Rule.CompensationIsZero.Id) then return compensation == 0;
                if (rule == Rule.CompensationLessThanDemand .Id) then return compensation < demand;

                ...
         }
}

Enumeration class implementation version is very similar like in this article (https://docs.microsoft.com/en-us/dotnet/architecture/microservices/microservice-ddd-cqrs-patterns/enumeration-classes-over-enum-types)

Thus I have values compensation and demand (these come from UI).
And I have some type based on which I get Rule value from database.
And so I call method IsValid (compensation, demand, ruleIdFromDb). If it is not validated then I show error on UI.

Shouldn't the 'IsValid' method be static? I suppose you use it like Rule.IsValid(calculatedCompensation, 0, Rule.CompensationIsZero.Id), (as general idea, obviously it can be simplified).

Anyway PERSONALLY I would not do this, because Rule is no more an Enum, you gave it logic, that usually Enum don't have. I would have keep the Enum as original and built on top of that a Class with the required logic. (Maybe called RuleValidator with a static method public static bool IsValid(Rule rule, decimal compensation, decimal demand) {...}.

If this does not concern you, than another way could be to write an extension method:

static class RuleExtensions
{
  public static bool Validate(this Rule rule, decimal compensation, decimal demand)
  {
    switch (rule)
    {
      case None: return true;
      case CompensationIsZero: return compensation == 0;
      case CompensationLessThanDemand : return compensation < demand;
      // ecc...
    }
  }
}


Handling periods ('.') in CSV column names with Pandas I'm reading and processing a fairly large csv using Pandas and Python 3.7. Header names in the CSV have periods in them ('full stops', Britons say). That's a problem when you want to address data cells by column name.

test.csv:

"name","birth.place","not.important"
"John","",""
"Paul","Liverpool","blue"

# -*- coding: utf-8 -*-

import pandas as pd

infile = 'test.csv'
useful_cols = ['name', 'birth.place']
df = pd.read_csv(infile, usecols=useful_cols, encoding='utf-8-sig', engine='python')

# replace '.' by '_'
df.columns = df.columns.str.replace('.', '_')

# we may want to iterate over useful_cols later, so to keep things consistent:
useful_cols = [s.replace('', '') for s in useful_cols]

# now we can do this..
print(df['birth_place'])

# ... and this
for row in df.itertuples():
    print(row.birth_place)

# ain't that nice?

It works, but since Pandas is such a powerful library and the use case is quite common, I'm wondering if there isn't an even better way of doing this.

Did a little digging and found that you can use df._columnid when pandas df.columns runs into an issue with a name (in this example dealing with a ".")

I am sure you already know that you could just do df['birth.place'], since it's inside a string container, however it becomes tricky for row.birth.placeas you mentioned. For that you can do the following:

for row in df.itertuples():
   print(row._2)

The _2 corresponds to the column id that pandas had issues parsing. It renamed it with an underscore and enumerated id in the column's list. Note that this renaming process only occurs when pandas ran into an issue grabbing the actual column name (i.e. row.name is still row.name, and you cannot use row._1 in-place of it). Hope that helps! Happy pythoning!


Path finding algorithm using recursion in Python Here is a code I developed some time ago. I wonder if there is anything I can do to improve it. It works for non-loopy mazes which was already my goal.

In the maze defined, True are the open ways and False are the walls.

The main subjects that I gladly accept suggestions can be listed like this:

Is the solution pythonic, if not what can be done to improve?
Because of the recursion, after I reach the ending point, I cannot directly return the true_path because the dead ends encountered by the player makes the function return  None. So I have to save it in another variable and let the recursion end after that. It is not directly an issue but I am eager to hear suggestions about it.(That is the reason i didn't return the recursions done in the function. They are just called.)
Is a while loop a better solution to this kind of problem?
Even if the solution is pythonic, are there any better ways to implement this path finding algorithm.

def possible_paths(maze, coor):
    # This function checks the 4 available routes around the current point.

    paths = [
    (coor[0]- 1, coor[1]),
    (coor[0],coor[1]-1),
    (coor[0], coor[1] + 1),
    (coor[0] + 1, coor[1])
        ]
    possible_paths = []

    for path in paths:
        if path[0] >= 0 and path[1] >= 0 and path[0] < len(maze[0]) \
        and path[1] < len(maze):
            if maze[path[1]][path[0]]:
                possible_paths.append(path)
    return possible_paths

answer = []
def solve(maze, pos, exit, true_path = []):
    global answer
    paths = [path for path in possible_paths(maze, pos) if path not in true_path]

    if pos == exit:
        answer = true_path
    elif len(paths) == 1:

        pos = paths[0]
        true_path.append(pos)
        solve(maze, pos, exit, true_path)

    else:
        for path in paths:
            temp_path = true_path[:] + [path]
            solve(maze, path, exit, temp_path)

maze = [[True, False, True,  False],
        [True, False, True,  False],
        [True, True , True,  False],
        [True, False, False, False],
        [True, True,  True, True],
        [True, False, False, False]]

solve(maze, (0,0), (2,0), [(0,0)])
print answer
# Gives [(0, 0), (0, 1), (0, 2), (1, 2), (2, 2), (2, 1), (2, 0)]

Note: Please notice that (2,0) is a coordinate, so it is actually equivalent to maze[0][2].

What's in a name?

This code is pretty easy to follow, and I could mostly understand it on the first read through. However, we could do better. Let's take a look at possible_paths(). This function doesn't return paths - it returns neighbors. It returns all the adjacent squares. Moreover, what is path[0] and what is path[1]? Let's name these things too.

Point = collections.namedtuple('Point', 'x y')

def neighbors(maze, coor):
    adjacent = (Point(coor.x - 1, coor.y),
        Point(coor.x, coor.y-1),
        Point(coor.x, coor.y+1),
        Point(coor.x+1, coor.y)
    )

    for p in adjacent:
        if 0 <= p.x < len(maze[0]) and 0 <= p.y < len(maze):
            if maze[p.y][p.x]: # I don't understand this flipping
                yield p

Having x and y is easier to grok than path[0], which looks like a point in a path, rather than an individual coordinate of a point. You could split this up even more by putting the adjacent points in their own function:

def adjacent(coor):
    yield Point(coor.x - 1, coor.y)
    yield Point(coor.x, coor.y - 1)
    yield Point(coor.x, coor.y + 1)
    yield Point(coor.x + 1, coor.y)

def neighbors(maze, coor):
    for p in adjacent(coor):
        ...

That's nice and bite-size. This idea should get propagated through to solve() so instead of starting with paths = ... we call it next_nodes = ... (or next_neighbors or something equivalent suggesting that we're talking about single points, not paths).

Global Variables

Right now, your solver is a little hard to use. You have to call it, then separately, check some global variable to see the result. Global variables are bad for a lot of reasons that I won't get into here, but the important one here is: you want your solver to return the answer. So let's do that:

if pos == exit:
    # we made it!
    return current_path

Okay that part is easy. What do we do in the other cases? First, there's not really a special case for len(paths) == 1 if you think about it. There is a special case for 0 though. Did we hit a dead end? Let's quit.

elif not next_nodes:
    # no path found
    return []

And for the rest, we just have to check the return of solve() every time. Note that there's no reason to copy the full list of current_path every time! That's expensive. You can just append to the end before the call to solve and pop afterwards:

else:
    for node in next_nodes:
        current_path.append(node)
        res = solve(maze, node, current_path)
        if res:
            return res
        current_path.pop()

Since current_path is what gets returned anyway, this can be simplified into just:

    for node in next_nodes:
        current_path.append(node)
        if solve(maze, node, current_path):
            return current_path:
        current_path.pop()

Now your usage is just:

>>> print solve(maze, Point(0,0), Point(2,0), [Point(0,0)])
[Point(0,0), ...]

Default values

Be very wary of having default values that are mutable. The canonical counterexample is something like:

def foo(x = []):
    x.append(1)
    return x

>>> foo()
[1]
>>> foo()
[1,1]
>>> foo()
[1,1,1]

Prefer to default to None. Then you can check it and assign it as appropriate:

def solve(maze, pos, exit, current_path = None):
    if current_path is None:
        current_path = [pos]

    next_nodes = [...]

What about other paths?

There may be more than one path. Let's return all of them. Or, rather than returning all of them, let's make this a generator. If the caller only wants one path, he can just take the first one!

def solve(maze, pos, exit, current_path = None):
    if current_path is None:
        current_path = [pos]
    if pos == exit:
        yield current_path # instead of return
    else:
        for node in neighbors(maze, pos):
            if node not in current_path:
                for result in solve(maze, node, exit, current_path + [node]):
                    yield result

A Path != Best Path

Depth-first search (what you're doing) will definitely find a path if it exists. But it won't necessary find the shortest one. Try out a few of the other path-finding algorithms. The direct corollary to DFS is Breadth-first search (which does exactly what it sounds like). And then there is a lot of room for optimization.


Solve Robot Paths using backtracking I got this problem during a mock interview, and I would like to get code review for the backtracking solution. I include 7 test cases, and my solution passes 7 out of 7 test cases. See the online code compiler here

  Robot Paths

  Prompt:   Given a matrix of zeroes, determine how many unique paths exist from the top left corner to the bottom right corner

  Input:    An Array of Array of Integers (matrix)

  Output:   Integer

  Examples:

matrix = [[0,0,0,0],
          [0,0,0,0],
          [0,0,0,0]]
robotPaths(matrix) = 38

matrix = [[0,0,0],
          [0,0,0]]
robotPaths(matrix) = 4

# Note:     From any point, you can travel in the four cardinal directions. I decided to do backtracking approach to solve this problem.
#           (north, south, east, west). A path is valid as long as it travels
#           from the top left corner to the bottom right corner, does not go
#           off of the matrix, and does not travel back on itself

def robot_paths(matrix):
  num_of_rows = len(matrix)
  num_of_cols = len(matrix[0])

  def traverse(row, col):
    nonlocal num_of_rows
    nonlocal num_of_cols
    # is row and col in bounds?
    if row < 0 or row >= num_of_rows or col < 0 or col >= num_of_cols:
      return 0

    # has row, col already been visited?
    if matrix[row][col] == 1:
      return 0

    # is row, col the destination?
    if row == num_of_rows - 1 and col == num_of_cols - 1:
      return 1

    # mark coordinate as visited
    matrix[row][col] = 1

    # initialize sum of total unique paths to end from that coordinate
    s  = traverse(row, col + 1) + traverse(row + 1, col) + traverse(row - 1, col) + traverse(row, col - 1)

    # backtrack; mark coordinate as unvisited so it can be
    matrix[row][col] = 0

    return s

  return traverse(0, 0)

#############################################
########  DO NOT TOUCH TEST BELOW!!!  #######
#############################################

def expect(count, name, test):
    if (count == None or not isinstance(count, list) or len(count) != 2):
        count = [0, 0]
    else:
        count[1] += 1

    result = 'false'
    errMsg = None
    try:
        if test():
            result = ' true'
            count[0] += 1
    except Exception as err:
        errMsg = str(err)

    print('  ' + (str(count[1]) + ')   ') + result + ' : ' + name)
    if errMsg != None:
        print('       ' + errMsg + '\n')

def lists_equal(lst1, lst2):
    if len(lst1) != len(lst2):
        return False
    for i in range(0, len(lst1)):
        if lst1[i] != lst2[i]:
            return False
    return True

print('Robot Paths Tests')
test_count = [0, 0]

def test():
    matrix = [[0, 0, 0, 0],
              [0, 0, 0, 0],
              [0, 0, 0, 0]]
    example = robot_paths(matrix)
    return example == 38

expect(test_count, 'should work on first example input', test)

def test():
    matrix = [[0, 0, 0],
              [0, 0, 0]]
    example = robot_paths(matrix)
    return example == 4

expect(test_count, 'should work on second example input', test)

def test():
    matrix = [[0]]
    example = robot_paths(matrix)
    return example == 1

expect(test_count, 'should work on single-element input', test)

def test():
    matrix = [[0, 0, 0, 0, 0, 0]]
    example = robot_paths(matrix)
    return example == 1

expect(test_count, 'should work on single-row input', test)

def test():
    matrix = [[0],
              [0],
              [0],
              [0],
              [0]]
    example = robot_paths(matrix)
    return example == 1

expect(test_count, 'should work on a 5 x 8 matrix input', test)

def test():
    matrix = [[0, 0, 0, 0, 0, 0, 0, 0],
              [0, 0, 0, 0, 0, 0, 0, 0],
              [0, 0, 0, 0, 0, 0, 0, 0],
              [0, 0, 0, 0, 0, 0, 0, 0],
              [0, 0, 0, 0, 0, 0, 0, 0]]
    print("  Please be patient, test 6 may take longer to run")
    example = robot_paths(matrix)
    return example == 7110272

print('PASSED: ' + str(test_count[0]) + ' / ' + str(test_count[1]) + '\n\n')

There are a few things I would like to point out to you.

Style

Python has an official style guide called PEP8. Under "Indendation" there is the following rule most Python programmers stick to:

  Use 4 spaces per indentation level.

Reference: PEP8 - Indentation

This would also make your code consistent with the provided test code.

Though not strictly part of PEP8, most of the tools integrated in Python IDEs would also tell you that s is not a good variable name. Use something that is more descriptive (n_paths maybe?).

While we're at it, you can wrap long lines such as the sum of the four traverse calls on multiple lines to allow better readability in case you have, say two documents next to each other or a small screen. Including the previous note, s = ... could be transformed into, e.g.

n_paths = traverse(row, col + 1) \
    + traverse(row + 1, col) \
    + traverse(row - 1, col) \
    + traverse(row, col - 1)

The nonlocal keyword

I suspect you slightly missunderstood what nonlocal does. All variables from an outer scope are automatically available to read from in nested scopes. This means in your case, traverse can see num_of_rows and num_of_cols from robot_paths automatically.

Statements like nonlocal or global only come into play whenever you want to assign to a variable not in your local scope. The Python interpreter would then recognize, that you don't want to create a new local variable with the given name, but instead use an already existing one from an outer scope. This SO answer has a nice example to show you that effect.


Is this a safe/correct way to make a python LogHandler asynchronous? I'm using some slow-ish emit() methods in Python (2.7) logging (email, http POST, etc.) and having them done synchronously in the calling thread is delaying web requests.  I put together this function to take an existing handler and make it asynchronous, but I'm new to python and want to make sure my assumptions about object scope are correct.  Here's the code:

def patchAsyncEmit(handler):
    base_emit = handler.emit
    queue = Queue.Queue()
    def loop():
        while True:
            record = queue.get(True) # blocks
            try :
                base_emit(record)
            except: # not much you can do when your logger is broken
                print sys.exc_info()
    thread = threading.Thread(target=loop)
    thread.daemon = True
    thread.start()
    def asyncEmit(record):
        queue.put(record)
    handler.emit = asyncEmit
    return handler

That let's me create an asynchronous LogHandler using code like:

handler = patchAsyncEmit(logging.handlers.HttpHandler(...))

I've confirmed the messages are sent, and not in the calling thread, but I want to make sure I'm getting only one queue and only one thread per Handler, and that I'm not at risk of those somehow going out of scope.

Am I off base or did I get that okay?

It would seem to me that writing a class would be better:

class AsyncHandler(threading.Thread):
    def __init__(self, handler):
         self._handler = handler
         self._queue = Queue.Queue()

         self.daemon = True
         self.start()

    def run(self):
        while True:
           record = self._queue.get(True)
           self._handler.emit(record)

    def emit(self, record):
        self._queue.put(record)

Patching objects can be useful, but its usually clearer to create another object like this.


Recursion memorization table The following code is for this problem.

  Input

  The input consists of a single line with three integers n
  , s
  , and k
   (1 ≤ n ≤ 10,000, 1≤ k ≤ s ≤ 500
  ). n
   is the number of throws, k
   the number of different numbers that are needed to win and s
   is the number of sides the die has.

  Output

  Output one line with the probability that a player throws at least k
   different numbers within n
   throws with an s
  -sided die. Your answer should be within absolute or relative error at most 10−7
  .

  Sample Input 1

  3 3 2

  Sample Output 1

  0.888888889

  Sample Input 2

  3 3 3

  Sample Output 2

  0.222222222

This program used 0.89s to pass all test cases. But I find other people use Java can achieve ~0.20s. How can I improve my code?

The idea is to establish a table for the recursive call to look up previously calculated the value.

import java.util.*;

public class Dice
{
  static double table[][];

  public static double game(int s, int r, int d)
  {
    if(table[r][d] != -1) return table[r][d];
    double a = (1 - (double)d/(double)s) * game(s, r - 1, d + 1);
    double b = ((double)d/(double)s) * game(s, r - 1, d);
    double sum = a + b;
    table[r][d] = sum;
    return sum;
  }

  public static void main(String[] args)
  {
    Scanner in = new Scanner(System.in);
    int n = in.nextInt();
    int s = in.nextInt();
    int k = in.nextInt();
    table = new double[n+1][n+1];

    for(int i = 0; i < n+1; i++)
     for(int j = 0; j < n+1; j++)
       table[i][j] = (double)-1;

    for(int i = 0; i < n+1; i++)
     for(int j = 0; j < n+1; j++)
      if(i + j < k)
        table[i][j] = (double)0;

    for(int j = 0; j < n+1; j++)
     for(int i = k; i < n+1; i++)
       table[j][i] = (double)1;

    System.out.println(game(s, n, 0));
   }
 }

Don't Repeat Yourself
Your main() function starts with 3 loops to initialize your table array. There are several problems here. First, it writes past the end of the array. If your array has n + 1 elements, then you can only write to 0 through n. Element n + 1 is out-of-bounds. So each of your loops is writing to one element of the next row, and then one extra row at the end, and one element past that, too. The loops should only go from 0 to n. This happens to work because you later read past the end of the array, too. If you're going to do that, you need to allocate n + 2 rows and columns.
Next, you're writing several elements 3 times. You can combine these 3 loops into 1 and do far less initialization. Further, you should use braces around all loop and conditional bodies because there's the potential to create errors in the future if you don't.
Also, you've switched i and j in the last loop which is highly confusing and could easily be missed by a reader.
Here's how I would write your initialization code:
table = new double [ n + 2 ][ n + 2 ]
for (int i = 0; i < n + 1; i++)
{
    for (int j = 0; j < k; j++)
    {
        if (i + j < k)
        {
            table [ i ][ j ] = 0.0;
        }
        else
        {
            table [ i ][ j ] = -1.0;
        }
    }

    for (int j = k; j < n + 1; j++)
    {
        table [ i ][ j ] = 1.0;
    }
}

Now no element should be touched more than once, no elements past the end of the array should be written to, and no array indexes are flipped.
You're also calculating (double)d / (double)s twice. You should calculate it once and re-use this value.
Naming
You should choose better names for your variables. i and j are fine for loop indexes as they are commonly used that way and nobody will be confused by them. However, I'd rename n, s, and k. I'd name n something descriptive like numThrows; k should be something like numNeeded, and s should be numSides.
Likewise, s, r, and d are not very helpful names for the arguments to game().
Speed
I haven't profiled your code, which is the real way to find out which parts are slow. I have seen many cases where casting variables from one type to another causes significant slowdowns. Since the bulk of your game() function is either calling itself or casting values, I recommend just making the arguments to the function be double to begin with. Of course, you can't use a double as an index into an array. So you could either pass the r and d values as both doubles and ints.


In place solution to remove duplicates from a sorted list I am working on the problem removeDuplicatesFromSortedList

  Given a sorted array nums, remove the duplicates in-place such that each element appear only once and return the new length.

  Do not allocate extra space for another array, you must do this by modifying the input array in-place with O(1) extra memory.

  Example 1:

Given nums = [1,1,2],

Your function should return length = 2, with the first two elements of nums being 1 and 2 respectively.

It doesn't matter what you leave beyond the returned length.

My solution and TestCase

class Solution:
    def removeDuplicates(self, nums: List[int]) -> int:
        """
        """
        #Base Case
        if len(nums) < 2: return len(nums)

        #iteraton Case
        i = 0  #slow-run pointer
        for j in range(1, len(nums)):
            if nums[j] == nums[i]:
                continue
            if nums[j] != nums[i]: #capture the result
                i += 1
                nums[i] = nums[j] #in place overriden
        return i + 1

class MyCase(unittest.TestCase):
    def setUp(self):
        self.solution = Solution()

    def test_raw1(self):
        nums = [1, 1, 2]
        check = self.solution.removeDuplicates(nums)
        answer = 2
        self.assertEqual(check, answer)

    def test_raw2(self):
        nums = [0,0,1,1,1,2,2,3,3,4]
        check = self.solution.removeDuplicates(nums)
        answer = 5
        self.assertEqual(check, answer)

unittest.main()

This runs but I get a report:

  Runtime: 72 ms, faster than 49.32% of Python3 online submissions for Remove Duplicates from Sorted Array.
  Memory Usage: 14.8 MB, less than 5.43% of Python3 online submissions for Remove Duplicates from Sorted Array.

Less than 5.43%, I employ the in-place strategies but get such a low rank, how could improve it?

One way that might speed up your solution slightly is to only place the value if necessary. Also note that only one of the two if conditions can be true, so just use else. Or even better, since there was a continue in the other case, just don't indent it.

i = 0  #slow-run pointer
for j in range(1, len(nums)):
    if nums[j] == nums[i]:
        continue
    # capture the result
    i += 1
    if i != j:
        nums[i] = nums[j] #in place overriden

You can also save half of the index lookups by iterating over the values. Of course it will need slightly more memory this way.

def removeDuplicates(nums):
    if len(nums) < 2:
        return len(nums)
    i = 0  #slow-run pointer
    for j, value in enumerate(nums):
        if value == nums[i]:
            continue
        # capture the result
        i += 1
        if i != j:
            nums[i] = value # in place overriden
    return i + 1

This can probably be further sped-up by saving nums[i] in a variable as well.

What is interesting is to see timing comparisons to using the itertools recipe unique_justseen (which I would recommend you use in production if you want to get duplicate free values in vanilla Python):

from itertools import groupby
from operator import itemgetter

def unique_justseen(iterable, key=None):
    "List unique elements, preserving order. Remember only the element just seen."
    # unique_justseen('AAAABBBCCDAABBB') --> A B C D A B
    # unique_justseen('ABBCcAD', str.lower) --> A B C A D
    return map(next, map(itemgetter(1), groupby(iterable, key)))

def remove_duplicates(nums):
    for i, value in enumerate(unique_justseen(nums)):
        nums[i] = value
    return i + 1

For nums = list(np.random.randint(100, size=10000)) they take almost the same time:

removeDuplicates took 0.0023s
remove_duplicates took 0.0026s


File processor for text and HTML I recently took a test. One of the questions was to find problems in the following code and suggest refactoring which can provide adding new formats of data:

public class FileProcessor
{
    enum FileType
    {
        Html, Text
    }

    public void ProcessFile(string fileName)
    {
        StreamReader fileStream = new StreamReader(File.OpenRead(fileName));
        string fileContent = fileStream.ReadToEnd();

        if (fileContent.IndexOf("<html") != -1)
            ProcessHtmlFile(fileContent);
        else ProcessTextFile(fileContent);

        ProcessFile(fileContent,fileContent.IndexOf("<html")!= -1?
                                     FileType.Html:FileType.Text);
        fileStream.Close();

    }

    private void ProcessFile(string content,  FileType fileType)
    {

        switch(fileType)
        {
            case FileType.Html:
                ProcessHtmlFile(content);
                break;
            case FileType.Text:
                ProcessTextFile(content);
                break;
            default:
                throw new Exception("Unknown file format");
        }
    }

    private void ProcessHtmlFile(string content){...}

    private void ProcessTextFile(string content){...}

}

What I've suggested:

I've suggested to use open closed principle and my implementation is:

public class FileProcessor
{
   public void ProcessFile(string fileName, IFile file)
   {
      StreamReader fileStream = new StreamReader(File.OpenRead(fileName));
      string fileContent = fileStream.ReadToEnd();

      file.ProcessFooFile(fileContent);
      fileStream.Close();
   }
}

public interface IFile
{
    void ProcessFooFile(string fileContent);
}

public class HtmlFile : IFile
{
   public void ProcessFooFile(string fileContent)
   {
      //Process HTML File
   }
}

public class TextFile : IFile
{
   public void ProcessFooFile(string fileContent)
   {
      //Process Text File
   }
}

My questions are:

Is it appropriate to use open closed principle in this case?
Is my implementation correct? Especially, please, see that I deleted a row ProcessFile(fileContent,fileContent.IndexOf("<html")!= -1?FileType.Html:FileType.Text); from public void ProcessFile(string fileName) as there is no need in the row.
Is there some drawbacks in my implementation. Please, provide improvements.

Ad 1. Yes, it's reasonable to refactor it the way you did. It's consistent with Single Responsibility Principle, and as such it makes it easy to eg. add some new formats without having to meddle with the base routine.

Ad 2. It's questionable in some places.

a) No exception handling. You may prefer it to be caller's responsibility, but at the very least you should ensure that the file stream always gets closed, even if an exception is thrown (with a try-finally or using).

b) The names IFile and its derivatives are misleading. It's a IFileProcessor (HtmlFileProcessor etc.). It's obvious that it doesn't represent a file itself, it represents a way of dealing with a file whose contents are passed from outside.

Ad 3. Your new implementation lacks the part that associated appropriate file processing method with a given FileType. This could be implemented with a FileProcessorFactory class taking a FileType instance and returning an IFileProcessor instance. Calling code doesn't need to know whether it's HtmlFileProcessor etc., that's the point: such nuts and bolts are something left for the factory to worry about, and caller is only concerned whether the factory returns something that matches the interface.

EDIT:

Here's a suggested improvement.

public class FileProcessor
{
    public void ProcessFile(string fileName, IFileProcessorBehavior file)
    {
        // using ensures we won't leave the method without closing the fileStream
        // even if an exception is thrown. this prevents resource leaks.
        using (var fileStream = new StreamReader(File.OpenRead(fileName)))
        {
            string fileContent = fileStream.ReadToEnd();
            file.ProcessFooFile(fileContent);
        }
    }
}

// Behavior, or Strategy.
// we need to differentiate file processor the general facade
// from format-specific file processor.
// in any case, this is not a "file", it's a way of dealing with a file.
// if I knew more about the context of this code,
// I could probably come up with a more natural naming scheme
public interface IFileProcessorBehavior
{
    void ProcessFooFile(string fileContent);
}

public class FileBehaviorFactory
{
    public IFileProcessorBehavior Create(FileType fileType)
    {
        switch(fileType)
        {
            case FileType.Html:
                return new HtmlBehavior();
            case FileType.Text:
                return new TextFileBehavior();
            default:
                // don't use the most generic Exception - use a meaningful exception type
                throw new NotSupportedException("Unknown file format: " + fileType);
                // ^ and quote the value that was not recognized -
                // it will help to track the cause of an error
        }
    }
}

public class HtmlBehavior : IFileProcessorBehavior
{
    public void ProcessFooFile(string fileContent)
    {
        // ...
    }
}

public class TextFileBehavior : IFileProcessorBehavior
{
    public void ProcessFooFile(string fileContent)
    {
        // ...
    }
}

And now you would just:

var factory = new FileBehaviorFactory();
IFileProcessorBehavior behavior = factory.Create(fileType);
// at this point we don't know and we don't care of what type this behavior thing is
var processor = new FileProcessor("some file", behavior);

Thanks to this pattern any new formats can be added as separate classes (supporting the interface), so the system is open for extension, and they can be registered within the factory. The base routine (FileProcessor) is closed, which is what we want. The factory only carries one responsibility, which is to match any given file type to respective behavior, or file processing strategy. This means there is only one ever one reason to modify the factory: when this logic needs to be enhanced, or modified.


Read integers from text file and write to CSV file Background:

I've recently started learning bash due to my new job in a VFX company. We backup all of our media to LTO tapes (one master and one clone). I was tasked with writing a script that split the tape list of master and clone to a CSV file. I feel I've done so in a crude manner as this was my first ever script and would love some feedback as to how I could improve the efficiency / syntax / code in general so I can learn from this experience.

Here is the text file: https://1drv.ms/t/s!AkWewdosAYGuhiaBbehlLBF64Qsg

I've been executing the script by calling it like this:
$ sh /scriptname.sh filename.txt

Code:

#!/bin/bash
# This script will split the presstore list of tapes into a .CSV file with two seperate coloumns.
file="$1"
echo "Splitting tape list....."
touch tempsplit.csv #creates temporary file for use later in script
while IFs= read line
do
    lastchar=$(echo $line | tail -c 2)
        if [ "$lastchar" == : ]     #Ommits any lines that end with : else error
            then
                echo -ne
        elif [ "$lastchar" -ge 0 -a "$lastchar" -le 9 ]     #Selects lines that end in number
            then
                breakdown=$(echo "$line" | cut -d':' -f2,6) #selects fileds 2 & 6 containing tape numbers
                master=$(echo "$breakdown" | cut -d'a' -f1) #cuts first number
                clone=$(echo "$breakdown" | cut -d':' -f2) #cuts second  number
                final=$(echo -e "$master,$clone" | tr -d ' ' >> tempsplit.txt) #outputs to a temp csv file
        fi
done < $file
touch tapelist_split.csv
awk 'NR % 2 == 0' tempsplit.txt | sort -n >> tapelist_split.csv #removes every 2nd line, sorts numerically, converts to a .CSV file
rm -rf tempsplit.txt #removes tempfile
echo "Complete"

Summary:

I'm not sure why the while read statement doesn't work if I don't call file="$1" as in line 3 and again output to that varaible in line 20 done < $file- an explanation on this would be amazing.

I realise I shouldn't have to call my first if statement, as I only care about the numbers but if I don't I get an error: "integer expected" when I run the script - does anyone know why this might be.

The reason I create the file "tempslipt.txt" is because my code to remove every 2nd line and sort the file wasn't working within the wile read statement so I figured this was a clean way of doing it.

My code may not be very efficient or good which is why I'm asking for tips on how I can refine and correct it so in future I can write much cleaner scripts.

Brave attempt :-) This can be so much better ;-)

Running scripts

The script has the shebang #!/bin/bash, but you invoke it as sh script.sh.
The purpose of a shebang is to make scripts runnable as ./script.sh.
When invoked this way, the shell looks at the shebang, and runs the script with the specified executable.
sh is often symlinked to Bash, but not always.

In short, if a script doesn't require Bash, then use the shebang line #!/bin/sh and run it as ./script.sh or sh ./script.sh.
If it requires Bash, then use the shebang line #!/usr/bin/env bash and run it with ./script.sh or bash ./script.sh.

Filtering lines

Bash is not particularly well-suited to filter lines by patterns.
grep is a great tool for that.
So instead of reading line by line with Bash to filter,
look for ways to use grep.
In this example, replace the loop with:

grep '[0-9]$' "$file" | while IFS= read line; do
    ...
done

Empty conditional branches

I think your intention here was to do nothing when the condition is true:

if [ "$lastchar" == : ]
    then
        echo -ne
elif ...

If you ever need to do nothing, you can use true or : like this:

if ...; then
    :
elif ...

But this is not a good example to use this trick,
because a better solution exists.
A better solution would have been to drop the if, and change the elif to if.
An even better solution would have been to call continue if the line doesn't match the required pattern, so the loop body would be flatter:

if ! [ "$lastchar" -ge 0 -a "$lastchar" -le 9 ]; then
    continue
fi

breakdown=...

With my tip with grep in the previous point, you don't need a condition at all.

Extracting two numbers

This is very inefficient:

breakdown=$(echo "$line" | cut -d':' -f2,6)
master=$(echo "$breakdown" | cut -d'a' -f1)
clone=$(echo "$breakdown" | cut -d':' -f2)
final=$(echo -e "$master,$clone" | tr -d ' ' >> tempsplit.txt)

The problem is that for each line this runs many processes: echo, cut, tr, and multiple $(...) sub-shells.

I see a simpler way to achieve what you want. Looking at these sample lines:

tape with barcode: 000053 and is: offline at listed location: MCR Shelves and is: Full and is copy of tape with barcode: 000047
tape with barcode: 000044 and is: offline at listed location: MCR Shelves and is: Full and is copy of tape with barcode: 000042

We could extract the two numbers and put a comma in between like this:

Replace all non-digits at the beginning with empty string (= remove)
Replace all non-digits with a comma

Try this simple pipeline:

grep '[0-9]$' file | head | sed -e 's/^[^0-9]*//' -e 's/[^0-9][^0-9]*/,/'

This works for the head of the file, but not the entire file. On closer look, there are lines that have a third number between the target numbers, which breaks the above pattern:

  tape with barcode: 000484 and is: online at listed location: i40 QUANTUM and is: Appendable and is copy of tape with barcode: 000483

After replacement this line becomes:

  000484 and is: online at listed location: i40 QUANTUM and is: Appendable and is copy of tape with barcode: 000483

Observe that there is a space after the first number and before the last. So to handle such case, we could change the logic of the second step to: "replace everything between two spaces with a comma".

Let's give that a try, on lines containing "i40":

grep 'i40.*[0-9]$' file | head | sed -e 's/^[^0-9]*//' -e 's/ .* /,/'

It seems the script could be replaced with:

grep '[0-9]$' "$file" | \
    sed -e 's/^[^0-9]*//' -e 's/ .* /,/' | \
    awk 'NR % 2 == 0' | sort -n > tapelist_split.csv

No temporary files needed.

If you have the GNU version of sed (typically in Linux), then you can use it delete every 2nd line instead of awk, slightly simpler:

grep '[0-9]$' "$file" | \
    sed -e 's/^[^0-9]*//' -e 's/ .* /,/' -e '1~2d' | \
    sort -n > tapelist_split.csv

Quoting

When you use variables as command parameters,
it's important to double-quote them to protect from word splitting and globbing. So a loop should read from $file like this:

while ...; do ...; done < "$file"

Understanding command parameters

Why did you use the -rf flags here?

rm -rf tempsplit.txt

There's no good reason to use those flags in the above script.

The -r flag is to delete directories recursively. But the parameter above is a single file. The -r flag serves no purpose.
The -f flag is to force deleting a file that might be protected, or to suppress error in case the file doesn't exist. Neither is the case here. The flag serves no purpose.

Flags in a script that serve no purpose or noise, confusing.
Understand all the flags you are using,
and make sure they have a reason to be there.

The field separator

The variable name is incorrect here:

while IFs= read line

Should have been:

while IFS= read line

Variable names in Bash are case sensitive.


C++ - printing a binary tree vertically, Depth first eg:
   5
  / \
 3   4
/\   /\
7 8  9 1

should print: 5 3 7 8 4 9 1

#include <iostream>
include <stack>
using namespace std;`

class node{
public:
int val;
node* left;
node* right;
bool istraversed=false;
};

void printdfs(node* root){
stack<node*> s;
s.push(root);

while(!s.empty()){

    node *temp = s.top();
    if(temp->istraversed==false){
        cout<<temp->val<<" ";
        temp->istraversed=true;
    }
    if(temp->left!=NULL){
        if(temp->left->istraversed==false){
            s.push(temp->left);
            continue;
        }
    }
    if(temp->right!=NULL){
        if(temp->right->istraversed==false){
            s.push(temp->right);
            continue;
        }
    }
    s.pop();
}
}

int main() {

node* root;
root->val=1;
cout<<root->val<<",";

root->left->val = 2;
cout<<root->left->val<<",";
root->right->val = 3;
cout<<root->right->val<<",";

root->left->left->val=4;
cout<<root->left->left->val<<",";
root->left->right->val=5;
cout<<root->left->right->val<<",";
root->right->left->val=6;
root->right->right->val=7;
root->left->left->left->val=8;

printdfs(root);

return 0;

}

The order in which you're traversing the tree is a simple pre-order traversal. If you can use recursion, it's a much easier way to do this traversal:

void preorder(node *root) {
    if (root == nullptr)
        return;
    std::cout << root->val << " ";
    preorder(root->left);
    preorder(root->right);
}

Other than that, your code currently has undefined behavior--in main you don't seem to be allocating any actual nodes for your tree. You're just taking an uninitialized pointer (root) and overwriting whatever happens to be in the memory it points at. To get something meaningful, you need to allocate your nodes.

I'd probably add a constructor to node to make construction a little easier:

struct node {
    int val;
    node *left, *right;

    node(int val) : val(val), left(nullptr), right(nullptr) {}
};

Then main would look something like this:

int main() {
    node *root = new node(5);
    root->left = new node(3);
    root->left->left = new node(7);
    root->left->right = new node(8);
    root->right = new node(4);
    root->right->left = new node(9);
    root->right->right = new node(1);
    preorder(root);
    // possibly code here to delete the nodes of the tree.
}


Operations on files and their locks - code too bulky? A couple of hours ago I asked a question on Stackoverflow to find out if there is a good way to delete files in a folder only if all files are indeed deletable and I got some good answers that led me (I think) in the right direction

Based on the two currently most upvoted answers I scrambled together the following code:

import java.io.File;
import java.io.IOException;
import java.io.RandomAccessFile;
import java.nio.channels.FileLock;
import java.util.ArrayList;
import java.util.List;

public class Main {

    public static void main(String[] args) {
        File directory = new File("T:\\download\\test\\1234");

        if (safeDelete(directory)) {
            System.out.println("All deleted");
        } else {
            System.out.println("All kept");
        }
    }

    private static boolean safeDelete(File directory) {
        if (directory == null || !directory.isDirectory()
                || !directory.canWrite()) {
            return false;
        }

        // First get a lock on the directory itself
        FileLock dirLock;
        if ((dirLock = getLock(directory)) == null) {
            return false;
        }

        List<File> filesToDelete = new ArrayList<>();
        List<FileLock> fileLocks = new ArrayList<>();

        // Get a lock on all the files in the folder
        for (File file : directory.listFiles()) {

            FileLock lock;
            if (file.canWrite() && ((lock = getLock(file)) != null)) {
                filesToDelete.add(file);
                fileLocks.add(lock);
            } else {
                System.out.println("Currently unable to write to "
                        + file.getName());
                releaseLocks(fileLocks);
                return false;
            }
        }

        // If we reach this line all the files are locked for us now
        for (File file : filesToDelete) {
            if (!file.delete()) {
                System.out.println("IMPOSSIBLE");
            }
        }
        releaseLocks(fileLocks);

        directory.delete();
        releaseLock(dirLock);
        return true;
    }

    private static FileLock getLock(File file) {
        try {
            return new RandomAccessFile(file, "rw").getChannel().tryLock();
        } catch (IOException ex) {
            System.out.println(ex.toString());
            return null;
        }
    }

    private static void releaseLocks(List<FileLock> locks) {
        for (FileLock lock : locks) {
            releaseLock(lock);
        }
    }

    private static void releaseLock(FileLock lock) {
        if (lock != null) {
            try {
                lock.release();
                System.out.println("Released lock on " + lock.toString());
            } catch (IOException ex) {
                System.out.println("Cant release lock on " + lock.toString());
            }
        }

    }
}

But it seems so... bulky to me. There are many return statements in there (I personally like it better when there is only one at the very end), a lot of if-else branching, two ArrayLists that manage almost the same stuff and a lot of calls to the releaseLocks() method.

The core problem is in my opinion that I have to manage two sets of data on which I both have to operate:

The Files on which I have to execute the deletion
And the Locks which I have to release after I'm done

So the question is:Can this be done more efficiently? Should I refactor the safeDelete() into more sub-methods?

Why not use a class to hold both the file and the lock, so you don`t need to manage two Lists like:

public class SafeDirectory {

    private File directory = null;
    private FileLock lock = null;
    private List<SafeFile> deleteableSafeFiles = new ArrayList<>();

    public SafeDirectory(String directoryPath) {
        directory = new File(directoryPath);
    }

    public boolean CanBeDeleted() {
        boolean canBeDeleted = false;
        if (directory != null && directory.canWrite() && directory.isDirectory()
                && ((lock = getLock(directory)) != null)) {

            canBeDeleted = true;
            for (File file : directory.listFiles()) {
                SafeFile safeFile = new SafeFile(file);

                if (safeFile.CanBeDeleted()) {
                    deleteableSafeFiles.add(safeFile);
                } else {
                    canBeDeleted = false;
                    System.out.println("Currently unable to write to "
                        + file.getName());
                    RelaseLock();
                    break;
                }
            }

        }
        return canBeDeleted;
    }

    public boolean Delete() {
        for (SafeFile safeFile : deleteableSafeFiles) {
            safeFile.Delete();
        }
        directory.delete();

        RelaseLock();
        return true;
    }

    private void RelaseLock() {
        for (SafeFile safeFile : deleteableSafeFiles) {
            releaseLock(safeFile.lock);
        }
        releaseLock(lock);
    }

    private boolean releaseLock(FileLock lock) {
        if (lock != null) {
            try {
                lock.release();
                System.out.println("Released lock on " + lock.toString());
                return true;
            } catch (IOException ex) {
                System.out.println("Cant release lock on " + lock.toString());
                return false;
            }
        }
        return true;

    }

    private static FileLock getLock(File file) {
        try {
            return new RandomAccessFile(file, "rw").getChannel().tryLock();

        } catch (IOException ex) {
            System.out.println(ex.toString());
            return null;
        }
    }

    private class SafeFile {

        private FileLock lock = null;
        private File file = null;

        SafeFile(File file) {
            this.file = file;
        }

        boolean CanBeDeleted() {
            return (file.canWrite() && ((lock = SafeDirectory.getLock(file)) != null));
        }

        boolean Delete() {
            return file.delete();
        }
    }
}