Add class name to DOM element I have a function that adds a class name to a DOM element is met. The script provided works.
function rm_row(){

  var chk_inherent =  arcapi.dataResult().columnNameToIndex('Inherent Risk');
  var chk_residual = arcapi.dataResult().columnNameToIndex('Residual Risk')
  var chk_perf = arcapi.dataResult().columnNameToIndex(arcapi.getSetting('Column name'));

  d3.selectAll("#" + arcapi.chartId() + " .dataTables_scrollBody tbody tr").classed('removerow', function() {

    const perf = this.children[chk_perf].innerHTML
    const inherent = this.children[chk_inherent].innerHTML
    const residue = this.children[chk_residual].innerHTML
    const cf_regex = perf.replace(/[`~%]/gi, '');

    //  **** Check *****
    const inh_isEmpty = inherent === '' || inherent === 'Null'
    const rsk_isEmpty = residue === '' || residue === 'Null'
    const perf_isInvalid = cf_regex < 0 || cf_regex > 100

    /** Logic [if inherent is empty AND  Residual risk is empty AND is perf value is incorrect = True] **/
    return inh_isEmpty && rsk_isEmpty && perf_isInvalid === true
  });
}

However, I feel it's long-winded. I feel this can be further optimized. What is the best approach/best practices to further improve myself and optimize my script so that it will be easy to maintain?
I have thought of creating an array and store all my variables in an array, but I feel this approach will be expensive.

However, I feel it's long-winded.

To reduce the amount of repeated code, and to make it easier to adapt for different columns without writing the same sort of thing again 2 or 3 times, look for similarities across the different columns. For every column, you're calling arcapi.dataResult().columnNameToIndex, so you can make a function which, given a column name, calls that. You're also accessing the this.children[index].innerHTML for every index retrieved.
One option is to use objects instead of multiple standalone variables. You can have an object indexed by variable names, whose values are the column indicies. Then, in the selectAll callback, you can construct an object with the same properties but with the text values you're interested in by using mapping the object's entries and passing into Object.fromEntries:
function rm_row() {
  const getIndex = columnName => arcapi.dataResult().columnNameToIndex(columnName);
  const columnIndiciesByName = {
    inherent: getIndex('Inherent Risk'),
    residual: getIndex('Residual Risk'),
    perf: getIndex(arcapi.getSetting('Column name')),
  };

  d3.selectAll("#" + arcapi.chartId() + " .dataTables_scrollBody tbody tr").classed('removerow', function() {
    const columnTextsByName = Object.fromEntries(
      Object.entries(columnIndicies).map(
        ([key, columnIndex]) => ([key, this.children[columnIndex].innerHTML])
      )
    );
    columnTextsByName.perf = columnTextsByName.perf.replace(/[`~%]/gi, '');
    // Then refer to columnTextsByName.inherent, etc

The above is just an option to consider. While this makes the code less repetitive and more flexible for future added or removed columns you might want to check, I'm not entirely convinced that it's a solid improvement over your current code if you're not expecting your current code to change. YMMV; you may consider your current code to be more readable, or not.
Other possible improvements:
Don't use var in ES6: If you're going to write in ES6 syntax - which you should, it's great - there's no reason to use var, it has too many gotchas (such as function-scope hoisting and automatically being assigned to the global object on the top level) to be worth using. Always use const (or, when you must reassign the variable, let).
JS naming conventions The vast majority of professional JavaScript uses camelCase for functions and ordinary variables. snake_case is quite rare (and mixing snake case with camelCase in the same variable name is even weirder). Consider using camelCase everywhere that it's appropriate.
cf_regex On a similar note, name your variables appropriately: in your original code, cf_regex is not a regular expression, it's just a string which has had certain characters replaced. Better to call it something more accurate and longer (I don't have any idea what it's meant to be, and neither will other readers of the code given only this context)
Something odd about the regex - you're using the case-insensitive flag i, but you're not matching any letters, so the flag isn't doing anything. Might as well remove it. Or, to make it clearer what's going on, if you're expecting the result to contain only numbers, match digits instead:
columnTextsByName.perf = Number(columnTextsByName.perf.match(/\d+/)[0]);

(since you're going to be comparing with numbers later, it feels better to cast the variable being compared to a number too; it makes more sense and will make debugging a bit easier if problems arise later)
innerHTML or textContent? Unless you're deliberately retrieving HTML markup, which doesn't look present here, if you just care about the text of a cell, it would be more appropriate to use .textContent rather than .innerHTML. It's faster too.
Semicolons You're using a few semicolons, but also missing a bunch. Unless you're an expert or use semicolons everywhere, you may eventually get tripped up by Automatic Semicolon Insertion. Consider using a linter.
Performance You say

I have thought of creating an array and store all my variables in an array, but I feel this approach will be expensive.

On modern computers, the overhead of creating an array or object instead of multiple standalone variables is completely nonexistent. If using a particular data structure makes the code more maintainable (given whatever style you prefer), go ahead and do it. If you later find that there's a performance issue, feel free to go back and debug to find what exactly the bottleneck is, so you can figure out a more efficient method - but using an object or array will almost certainly not be the bottleneck. Try to avoid premature optimization, or at least don't sacrifice code clarity for it unless you have to.


LeetCode 665: Non-decreasing Array I'm posting a solution for LeetCode's "Non-decreasing Array". If you'd like to review, please do. Thank you!
Problem
Given an array nums with n integers, your task is to check if it could become non-decreasing by modifying at most 1 element.
We define an array is non-decreasing if nums[i] <= nums[i + 1] holds for every i (0-based) such that (0 <= i <= n - 2).
Example 1:

Input: nums = [4,2,3]
Output: true
Explanation: You could modify the first 4 to 1 to get a non-decreasing array.

Example 2:

Input: nums = [4,2,1]
Output: false
Explanation: You can't get a non-decreasing array by modify at most one element.

Constraints:

1 <= n <= 10 ^ 4
-10 ^ 5 <= nums[i] <= 10 ^ 5

Code
// Most of headers are already included;
// Can be removed;
#include <iostream>
#include <cstdint>
#include <vector>

// The following block might slightly improve the execution time;
// Can be removed;
static const auto __optimize__ = []() {
    std::ios::sync_with_stdio(false);
    std::cin.tie(nullptr);
    std::cout.tie(nullptr);
    return 0;
}();

struct Solution {
    using ValueType = std::int_fast32_t;
    static const bool checkPossibility(
        std::vector<int>& nums
    ) {

        if (std::size(nums) < 3) {
            return true;
        }

        ValueType max_changes = 0;

        for (ValueType index = 1; max_changes < 2 && index < std::size(nums); ++index) {
            if (nums[index - 1] > nums[index]) {
                ++max_changes;

                if (index - 2 < 0 || nums[index - 2] <= nums[index]) {
                    nums[index - 1] = nums[index];

                } else {
                    nums[index] = nums[index - 1];
                }
            }
        }

        return max_changes < 2;
    }
};

int main() {
    std::vector<int> nums = {3, 4, 2, 3};
    std::cout << std::to_string(Solution().checkPossibility(nums) == false) << "\n";
    return 0;
}

Avoid unnecessary special case handling
You exit early if the size of the array is less than 3, but this is unnecessary: the rest of the code already handles arrays of size 0, 1 and 2 correctly. You might save a cycle if you feed it a small array, but you pay for this check with a cycle or two for every time the function is called with std::size(nums) > 2.
Use std::size_t for indices
You made index a std::int_fast32_t, but this has a different size (likely) and a different signedness than the result of std::size(nums). This means the compiler should have warned you about a comparison between signed and unsigned integers. While things work out here, since you know the size of the input array is constrained, it is best to use std::size_t here to avoid the compiler warning. Performance is likely not going to differ one bit, since index can be kept in a CPU register at all times.
There is no need to use std::to_string() when using << on a std::ostream
When writing to a std::ostream, operator<< will already cause the argument to be formatted, so there is no need to call std::to_string(). In fact, you can tell the stream to format a bool as text:
int main() {
    std::vector<int> nums = {3, 4, 2, 3};
    std::cout << std::boolalpha << Solution().checkPossibility(nums) << "\n";
}


Basic slot machine game I have been learning PHP and wanted to see if I could make a very simple slot machine game.  Everything works, but I'm sure this is not the best way to do this. Please let me know how you would do this and what I can do to improve.

<?php

$num1 = rand(1, 5);
$num2 = rand(1, 5);
$num3 = rand(1, 5);

$result = $num1.' | '.$num2.' | '.$num3;

// Read points from file
$filename = 'points.txt';
$handle = fopen($filename, 'r');
$current = fread($handle, filesize($filename));
fclose($handle);

if ($num1 == $num2 && $num2 == $num3) {
$status = '<big>You are a winner!</big>';

$add_points = $current + 10;

// Add points to file
$handle = fopen($filename, 'w');
$current_points = fwrite($handle, $add_points);
fclose($handle);

} else {
$status = 'please try again!';
}

?>

<title>Slot Machine Game!</title>

<big><?php echo $result; ?></big>

<big><?php echo $status; ?></big>

<big><?php echo 'You have '.$current.' points!'; ?></big>

Looks good. Few initial things:

Indent inside control structures, so everything inside your if() statement would be prefixed by one tab. (You can use a fixed number of spaces, but that can cause problems if you share your code with people who use a different tab-spacing system).
It is a really good idea to split out your code into logic (or "controller") and template (or "view"). Your html would then go in a different file and be loaded by your PHP file - a good first step with this sort of thing is Smarty. From there you can progress to a proper PHP framework such as Symfony or Zend (but be aware the learning curve is steep).
Tags such as  and <big> tend to be discouraged - use CSS instead.
Keep your PHP statements in your view as small as possible. For example I'd rewrite your score thus:

    You have <?php echo $current ?> points!

You'll need to define a CSS class called 'points', of course.


Charging battery animation over time I have the following method that is working perfectly. Main problem I see is that I think it can be done just in one line, but I don't know how:

def _animate_display(self, display):
    self._timer += 1 / bge.logic.getAverageFrameRate()

    symbolic_level = self.battery.get_symbolic_level()

    if self._timer >= 5.0:
        self._timer = symbolic_level / 25
    elif (self._timer >= 4.0) and (symbolic_level <= 100):
        utils.change_texture(display, 'DisplayMat', 'icons/battery_100.png')
    elif (self._timer >= 3.0) and (symbolic_level <= 75):
        utils.change_texture(display, 'DisplayMat', 'icons/battery_75.png')
    elif (self._timer >= 2.0) and (symbolic_level <= 50):
        utils.change_texture(display, 'DisplayMat', 'icons/battery_50.png')
    elif (self._timer >= 1.0) and (symbolic_level <= 25):
        utils.change_texture(display, 'DisplayMat', 'icons/battery_25.png')
    elif (self._timer >= 0.0) and (symbolic_level <= 0):
        utils.change_texture(display, 'DisplayMat', 'icons/battery_0.png')

It models a simple animation of a charging battery in steps of 1 second. Something like: [....] --1s--> [=...] --1s--> [==..] --1s--> [===.] --1s--> [====] --1s--> [....] ...

Symbolic level is a variable that keep a value of [0, 25, 50, 75, 100], one of these, so animation loops from that symbolic value so it doesn't start from 0.

I took it to 6 to make it a little more readable:

def _animate_display(self, display):
    self._timer += 1 / bge.logic.getAverageFrameRate()
    symbolic_level = self.battery.get_symbolic_level()
    if self._timer >= 5.0:
        self._timer = symbolic_level / 25
    else:
        utils.change_texture(display, 'DisplayMat', 'icons/battery_{}.png'.format(next((x,y) for x,y in zip(xrange(100, -1, -25), xrange(4, -1, -1)) if symbolic_level <= x and self._timer >= y)))


Check if array is monotonic i.e either monotone increasing or monotone decreasing An array A is monotone increasing if for all i <= j, A[i] <= A[j].  An array A is monotone decreasing if for all i <= j, A[i] >= A[j].
Return true if and only if the given array A is monotonic.
public class MonotonicArray {

    public boolean IsMonotonic(int[] numbers) {
        if (numbers == null  || numbers.length == 0) {
            return false;
        }

        if (numbers.length == 1) {
            return true;
        }

        boolean increasing = false;
        boolean decreasing = false;

        for (int index = 0; index < numbers.length - 1; index++) {

            if (numbers[index + 1] == numbers[index]){
                continue;
            }

            if (numbers[index + 1] > numbers[index]) {
                if (!decreasing) {
                    increasing = true;
                } else {
                    return false;
                }
            }

            else  {
                if (!increasing) {
                    decreasing = true;
                } else {
                    return false;
                }

            }

        }
        return increasing || decreasing;
    }
}

Test cases:
class MonotonicArrayTest extends MonotonicArray {

@org.junit.jupiter.api.Test
void isMonotonic1() {

    int[] array =  new int[]{1,2,3};
   assertEquals(true,IsMonotonic(array));
}

@org.junit.jupiter.api.Test
void isMonotonic2() {

    int[] array =  new int[]{-1,-2,-3};
    assertEquals(true,IsMonotonic(array));
}

@org.junit.jupiter.api.Test
void isMonotonic3() {

    int[] array =  new int[]{1,2,1};
    assertEquals(false,IsMonotonic(array));
}

@org.junit.jupiter.api.Test
void isMonotonic4() {
    int[] array =  new int[]{-1,2,-9};
    assertEquals(false,IsMonotonic(array));
}

@org.junit.jupiter.api.Test
void isMonotonic5() {

    int[] array =  new int[]{9,3,2};
    assertEquals(true,IsMonotonic(array));
}

@org.junit.jupiter.api.Test
void isMonotonic6() {
    int[] array =  new int[]{};
    assertEquals(false,IsMonotonic(array));
}

@org.junit.jupiter.api.Test
void isMonotonic7() {
    int[] array =  new int[]{1};
    assertEquals(true,IsMonotonic(array));
}

@org.junit.jupiter.api.Test
void isMonotonic8() {
    int[] array =  new int[]{9,7,5,4,8,10};
    assertEquals(false,IsMonotonic(array));
}

@org.junit.jupiter.api.Test
void isMonotonic9() {
    int[] array =  new int[]{1,1,2,3};
    assertEquals(true,IsMonotonic(array));
}

@org.junit.jupiter.api.Test
void isMonotonic10() {
    int[] array =  new int[]{1,1,0,-1};
    assertEquals(true,IsMonotonic(array));
}

}

static
IsMonotonic(...) does not need an instance of the MonotonicArray class to function, therefore it should be static.
Consistency
You special case an array of length 1 as monotonic.  Is it really?  It is neither increasing nor decreasing.
What about IsMonotonic(new int[]{1, 1, 1, 1})?  Seems to me that should be true, but it will return false.  Definitely should be added as a test case.  And if it should return true, then ...
Optimization
... checking for length 1 is too restrictive.  Any length 2 array will always be monotonic as well.  Perhaps:
    if (numbers.length == 1) {
        return true;
    }

should be:
    if (numbers.length <= 2) {
        return true;
    }

Looping
This is ugly.  Will Java optimize the numbers.length - 1 calculation as a constant?
    for (int index = 0; index < numbers.length - 1; index++) {

        if (numbers[index + 1] == numbers[index]){
            continue;
        }
        ...

It may be better to use Java's enhanced for loop to extract numbers, and rely on monotonic behaviour allowing equality to handle the first element:
    int current = numbers[0];
    for(int value : numbers) {
        if (value != current) {
           if (value < current) {
              ...
           } else {
              ...
           }
           current = value;
        }
    }


Python tuition calculator 2.0 Here is my program that calculates tuition cost for the next five years. I have had some review on it so far, but I am seeking more. Please keep in mind I am new to programming, so I may require a few extra steps to understand something. Thanks in advance!

Version 1.0 is here

RESIDENCY_COST = {
                    "I": 10000,
                    "O": 24000,
                    "G": 40000,
                 }

cost_of_tuition = None
while True:
    residency = raw_input('Please input your type of residency, I for in-state, O for out-of-state, and G for graduate: ')
    try:
        cost_of_tuition = RESIDENCY_COST[residency]
        break
    except KeyError:
        print ('Please enter I, G or O ONLY.')

years = []
tuition_increase = []
academic_year = []
academic_year_inc = []

for _ in range(5):
    intMath = cost_of_tuition * 0.03
    tuition_increase.append(intMath)
    fnlMath = intMath + cost_of_tuition
    years.append(fnlMath)
    cost_of_tuition = fnlMath
    academic_year.append("${:,.2f}".format(fnlMath))
    academic_year_inc.append("${:,.2f}".format(intMath))
    total_tuition_increaseSum = sum(tuition_increase)
    total_tuition_increase = "${:,.2f}".format(total_tuition_increaseSum)

print('UNDERGRADUATE TUITION FOR THE NEXT FIVE YEARS ')
print('ACADEMIC YEAR          TUITION       INCREASE ')
print('-------------     ------------       -------- ')

for i, year in enumerate(range(16, 21)):
    print('{}-{}             {}        {}'.format(year + 2000, year + 1,
                                                  academic_year[i],
                                                  academic_year_inc[i]))

print('TOTAL TUITION INCREASE              ' + total_tuition_increase)

You don't use total_tuition_increaseSum for anything other than that one print, so you may as well call sum on it in the print line:

total_tuition_increase = "${:,.2f}".format(sum(total_tuition_increase))

I would also recommend you look more at splitting the code into functions. Functions usually make code easier to read, use and change. For example, you might put your input management into one function so it's a small self contained chunk. All it needs to do that's different is return the resulting tuition cost instead of setting it directly. This is how it'd work:

def get_cost():
    while True:
        residency = raw_input('Please input your type of residency, I for in-state, O for out-of-state, and G for graduate: ')
        try:
            return RESIDENCY_COST[residency.upper()]
            break
        except KeyError:
            print ('Please enter I, G or O ONLY.')

And you could call it like this:

cost_of_tuition = get_cost()

Similarly, you could then wrap your printing into a print_tuition_table function. This one would need to take parameters, your two lists and total_tuition_increase.

def print_tuition_table(academic_year, academic_year_inc, total_tuition_increase):
    print('UNDERGRADUATE TUITION FOR THE NEXT FIVE YEARS ')
    print('ACADEMIC YEAR          TUITION       INCREASE ')
    print('-------------     ------------       -------- ')

    for i, year in enumerate(range(16, 21)):
        print('{}-{}             {}        {}'.format(year + 2000, year + 1,
                                                      academic_year[i],
                                                      academic_year_inc[i]))

    print('TOTAL TUITION INCREASE              {}'.format(total_tuition_increase))

Of course, you could change this to be more re-usable now if you didn't hard code it as being five years. Instead, you could check the length of the academic_year list and then just change the way the loop works slightly:

def print_tuition_table(academic_year, academic_year_inc, total_tuition_increase):
    length = len(academic_year)
    print('UNDERGRADUATE TUITION FOR THE NEXT {} YEARS '.format(length))
    print('ACADEMIC YEAR          TUITION       INCREASE ')
    print('-------------     ------------       -------- ')

    for i, year in enumerate(range(16, 16 + length)):
        print('{}-{}             {}        {}'.format(year + 2000, year + 1,
                                                      academic_year[i],
                                                      academic_year_inc[i]))

    print('TOTAL TUITION INCREASE              ' + total_tuition_increase)

Then you could do the same with your calculation loop. It would need to take in cost_of_tuition and return multiple values, but return multiple values is easy in Python. First, you wrap it in a function like this:

def calculate_fees(cost_of_tuition):
    for _ in range(5):
        intMath = cost_of_tuition * 0.03
        tuition_increase.append(intMath)
        fnlMath = intMath + cost_of_tuition
        years.append(fnlMath)
        cost_of_tuition = fnlMath
        academic_year.append("${:,.2f}".format(fnlMath))
        academic_year_inc.append("${:,.2f}".format(intMath))
        total_tuition_increaseSum = sum(tuition_increase)
        total_tuition_increase = "${:,.2f}".format(total_tuition_increaseSum)

    return academic_year, academic_year_inc, cost_of_tuition

Then you can set those values like this:

academic_year, academic_year_inc, cost_of_tuition = calculate_fees(cost_of_tuition)

Also, you could easily expand this function to take a years parameter so that it doesn't have to be stuck with 5 years.

def calculate_fees(cost_of_tuition, years):
    for _ in range(years):

And since our printing function checks the length of your list, now you just need to change one number to be able to adjust the whole output.

With all those functions defined, here's how you could call them to run the script:

cost_of_tuition = get_cost()
academic_year, academic_year_inc, cost_of_tuition = calculate_fees(cost_of_tuition)
print_tuition_table(academic_year, academic_year_inc, cost_of_tuition)


Implementing String to int (atoi) in Java The function first discards as many whitespace characters as necessary until the first non-whitespace character is found. Then, starting from this character, takes an optional initial plus or minus sign followed by as many numerical digits as possible, and interprets them as a numerical value.
The string can contain additional characters after those that form the integral number, which are ignored and have no effect on the behavior of this function.
If the first sequence of non-whitespace characters in str is not a valid integral number, or if no such sequence exists because either str is empty or it contains only whitespace characters, no conversion is performed.
If no valid conversion could be performed, a zero value is returned. If the correct value is out of the range of representable values, INT_MAX (2147483647) or INT_MIN (-2147483648) is returned.

I'm not sure about my checks against integer overflow, but here is my implementation:
public int myAtoi(String str) {
    int i = 0;
    while (i < str.length() && Character.isWhitespace(str.charAt(i))) {
        ++i;
    }
    if (i == str.length()) {
        return 0;
    }

    boolean isNegative = false;
    if (str.charAt(i) == '+' || str.charAt(i) == '-') {
        isNegative = str.charAt(i) == '-';
        ++i;
    }

    int result = 0;
    while (i < str.length() && Character.isDigit(str.charAt(i))) {
        try {
            result = Math.multiplyExact(result, 10);
            result = Math.addExact(result, Character.getNumericValue(str.charAt(i)));
        } catch (ArithmeticException e) {
            return isNegative ? Integer.MIN_VALUE : Integer.MAX_VALUE;
        }
        ++i;
    }

    if (isNegative) {
        result = -result;
    }

    return result;
}

All in all, that's a pretty good implementation in a number of key details.

Using the Character.isDigit() and Character.getNumericValue() methods are good to see.

The Math.* methods that handle the overflow conditions are good too.

I am not sure if you intended it, but you also correctly handle an obscure edge-case in 32-bit signed integer systems (not just Java), where Integer.MIN_VALUE is not the same as - Integer.MAX_VALUE ... and your code actually gets it right for an exact input of the text "-2147483648"

So, you have good details in your code.... and I can't see any broken edge-cases.

My only recommendation would be that a state-machine may make things simpler... with just one loop..... but the state-machine may be a bit messy too, although I think it works out better in the long run...

public static int rlAtoi(String str) {
    boolean started = false;
    boolean negative = false;
    int result = 0;
    try {
        for (char c : str.toCharArray()) {
            if (!started && Character.isWhitespace(c)) {
                // great, ignore it.
            } else if (!started && (c == '+' || c == '-')) {
                // great, a sign
                negative = c == '-';
                started = true;
            } else if (Character.isDigit(c)) {
                result = Math.multiplyExact(result, 10);
                result = Math.addExact(result, Character.getNumericValue(c));
                started = true;
            } else {
                // done....
                break;
            }
        }
    } catch (ArithmeticException e) {
        return negative ? Integer.MIN_VALUE : Integer.MAX_VALUE;
    }
    return negative ? -result : result;
}

Note that in a raw performance benchmark, I suspect your solution will be (slightly) faster, but I prefer readability over small incremental performance gains, unless performance is extremely critical.


Is my code utilizing proper Object Oriented features? class element {
   constructor(name){
    this.name = name;
   }
   createNew(){
    var elem = document.createElement(this.name);
    document.body.appendChild(elem);
   }
   setElementAttribute(attr_name, attr_value){
    this.attr_name = attr_name;
    this.attr_value = attr_value;
   }
   createNewWithAttribute(){
    var elem = document.createElement(this.name);
    document.body.appendChild(elem);
    elem.setAttribute(this.attr_name, this.attr_value);
   }
 }
 class elementWithText extends element {
   constructor(name, text){
    super(name);
    this.text = text;
   }
   createNew(){
   var elem = document.createElement(this.name);
   document.body.appendChild(elem);
   elem.innerHTML = this.text;
  }
   setElementAttribute(attr_name, attr_value){
    super.setElementAttribute(attr_name, attr_value);
  }
   createNewWithAttribute(){
    var elem = document.createElement(this.name);
    document.body.appendChild(elem);
    elem.setAttribute(this.attr_name, this.attr_value);
    elem.innerHTML = this.text;
  }
}
   var inputBox = new element('input');
   inputBox.setElementAttribute('type','radio');
   inputBox.createNewWithAttribute();
   var heading = new elementWithText('h1','Hello World');
   heading.setElementAttribute('style','color: red;');
   heading.createNewWithAttribute();

I don't suggest encapsulating such standard functionality in classes unless you are just playing around or learning.
I suggest renaming element to ElementFactory or ElementSpawner or something alike as it doesn't encapsulate a DOM element but creates and appends them. You then might want to return the created elements, too.
Also, what happens when you call createNewWithAttribute without previous call to setElementAttribute? Shouldn't you rather keep an internal map of attributes and make their addition optional via createNew(withAttributes = true) parameter?
Also, elem.innerHTML = this.text should probably be written as elem.textContent = this.text unless text means html in which case you should rename the class.
Don't mix CamelCase (elementWithText) with unser_scores (attr_value) and preferably capitalize class names.


Write to files in Delphi I'm doing a program in Delphi, writing in all kinds of files, either text or executable:

var
  openfile: TextFile;
begin
      AssignFile(openfile, 'test.exe');
      Append(openfile);
      WriteLn(openfile,'[test]hi world[test]');
      CloseFile(openfile);
end;

How can I improve this?

Yes. I would add try..finally so that the file will be properly closed even in the case of exception (hopefully my remembrance of the syntax isn't too rusty):

var
  openfile: TextFile;
begin
      AssignFile(openfile, 'test.exe');
      Append(openfile);
      try
          WriteLn(openfile,'[test]hi world[test]');
      finally
          CloseFile(openfile);
      end;
end;

also, you say you want to write binary files as well as text files.. so don't use TextFile. The "modern" way would be to use TFileStream:

with TFileStream.Create('test.exe', fmOpenWrite) do
try
  Seek(0,soFromEnd);
  Write(...);
finally
  Free;
end;


Bash script wallpaper randomizer I thought it would be fun to write a simple and small bash script which randomly chooses a wallpaper from a given directory of a set of wallpaper images. The code is as follows.
#!/bin/bash

# simple script to choose random image from
# Wallpaper directory and set it as desktop background

# cd into Wallpaper dir

cd /home/user/Pictures/Wallpapers

# using nano time mod # of imgs in Wallpapers (+1 b/c awk fields start at ind 1)
# to provide a semi-random field number for the awk statement

RAND=$(($(date +%N) % $(ls -h | wc -l) + 1))
IMG=$(ls -h | awk -v r=$RAND 'BEGIN{FS = "\n"; RS = ""} {print $r}')

# change the desktop with the img file provided from the awk statement
# (the way to set the background is system dependent but the gist is the same)

gsettings set org.gnome.desktop.background picture-uri "file:///home/user/Pictures/Wallpapers/$IMG"

This is really just a small, fun project, but I am almost certain I am not doing this the most efficient way, so any feedback would be great.

Why you shouldn't parse ls.
Let's consider ls bad practice, and allow me to introduce a different method to solve your issue all together. Find every file in the directory, /home/user/Pictures/Wallpapers, perform a random sort on it and then grab the first result.
find /home/user/Pictures/Wallpapers -type f | sort -R | head -1
Sort man page
       -R, --random-sort
              shuffle, but group identical keys.  See shuf(1)


Merging two files' lines of text for output My program is supposed to merge two text files' lines together. For instance, if I have two files, one.txt:

a b c d e f g h i j k l m
n o p q r s t u v w x y z

and two.txt:

0 1 2 3 4
5 6 7 8 9

The output is:

a b c d e f g h i j k l m0 1 2 3 4
n o p q r s t u v w x y z5 6 7 8 9

However, I  feel as though some of the conditions are redundant. However, I'm not sure how I can better design this.

// Program to merge lines from two files and output results
#include <stdio.h>

int main(int argc, char *argv[])
{
    char *inName1 = argv[1], *inName2 = argv[2];
    FILE *in1, *in2;
    int c, d;

    // Ensure correct usage
    if (argc != 3)
    {
        fprintf(stderr, "Usage: ./merge <file1> <file2>\n");
        return 1;
    }

    // Open input files and return if unable to open
    if ((in1 = fopen(inName1, "r")) == NULL)
    {
        fprintf(stderr, "Can't open %s.\n", inName1);
        return 2;
    }

    if ((in2 = fopen(inName2, "r")) == NULL)
    {
        fprintf(stderr, "Can't open %s.\n", inName2);
        return 3;
    }

    // Take care of output
    while ((c = getc(in1)) != EOF)
    {
        if (c != '\n')
            putc(c, stdout);
        else
        {
            while ((d = getc(in2)) != EOF)
            {
                if (d != '\n')
                    putc(d, stdout);
                else
                {
                    putc('\n', stdout);
                    break;
                }
            }

            if (d == EOF)
            {
                while (c != EOF)
                {
                    putc(c, stdout);
                    c = getc(in1);
                }
            }
        }
    }

    if (c == EOF)
    {
        while ((d = getc(in2)) != EOF)
        {
            putc(d, stdout);
        }
    }

    fclose(in1);
    fclose(in2);

    printf("\nProcess ended successfully.\n");

    return 0;
}

Some good things

This compiles cleanly with all my usual compiler warnings enabled - well done!

You've used stderr appropriately for the output (with the exception of the final message); that's good practice.

Don't dereference argv before you've checked its bounds

This is wrong:

    char *inName1 = argv[1], *inName2 = argv[2];

    if (argc != 3) {
        fprintf(stderr, "Usage: ./merge <file1> <file2>\n");
        return 1;
    }

We can't read from argv[1] or argv[2] until after we know that argc is big enough:

    if (argc != 3)
    {
        fprintf(stderr, "Usage: %s <file1> <file2>\n", argv[0]);
        return 1;
    }
    char *inName1 = argv[1], *inName2 = argv[2];

I've enhanced your error message to use the actual name of the program, as passed in argv[0].

Report actual error type on failure

"Can't open " at least specifies which file couldn't be opened, but we can be more specific (file not found, permission denied, ...) using the perror() function:

    FILE *const in1 = fopen(inName1, "r");
    if (!in1) {
        perror(inName1);
        return 1;
    }

I also changed this to return 1 from any error - there isn't any great benefit in distinguishing the different causes in the error code.

A slight tweak to the logic

The loops look mostly correct; there's a small duplication we can avoid here:

        while ((d = getc(in2)) != EOF)
        {
            if (d != '\n')
                putc(d, stdout);
            else
            {
                putc('\n', stdout);
                break;
            }
        }

We can bring the putc() outside the if-else like this:

        while ((d = getc(in2)) != EOF) {
            putc(d, stdout);
            if (d == '\n') {
                break;
            }
        }

A redundant test

while ((c = getc(in1)) != EOF)
{
    /* ... */
}

/* c can only be EOF here */
if (c == EOF)
{
    /* ... */
}

Consider some edge cases

You've taken good care to ensure that when one file ends, the rest of the other is output.  Have you thought about when to output a newline, if the ending file doesn't itself end with one.  It's not clear what you intend to happen in that case - comments should explain the desired output.

Alternative logic

I prefer a less deeply nested set of loops, where the first loop is the "normal" case where we're reading from both files, and we leave that loop when either file is exhausted, and finish copying from the other:

int c, d;
// Take care of output
while ((c = getc(in1)) != EOF) {
    if (c != '\n') {
        putc(c, stdout);
        continue;
    }

    while ((d = getc(in2)) != EOF) {
        putc(d, stdout);
        if (d == '\n') {
            break;
        }
    }

    if (d == EOF) {
        break;
    }
}

while ((c = getc(in1)) != EOF) {
    putc(c, stdout);
}
while ((d = getc(in2)) != EOF) {
    putc(d, stdout);
}

I've kept both c and d variables for this, although we only really need one for both files.

Error checking of output

None of the above code ever checks the return value of putc().  Filesystems do fill up, and users do disconnect streams and worse, so we really ought to ensure that the writes succeed.

Error checking of input

We currently assume that EOF returned from getc() means that we've reached the end of the file.  However, it could mean that we have an error of reading, and we should distinguish those cases using feof() or ferror().

Final message

printf("\nProcess ended successfully.\n");

This isn't part of the output (and you don't want it to be passed along in a pipeline).  Send status messages to stderr - or better still, be silent whilst successful, like the standard utilities.


Linked List Interview Code methods, runtime, and edge cases Refactored code here

I want to write a very simple Linked List with only 3 methods Append, Remove and Print. This is not for production and is to be treated as code that could be used in an interview or a quick and dirty prototype. I'm really curious about the approach I've taken here to ensure no duplicates appear in my Linked List. I feel the data structure will be much easier to work with if remove any complexity around duplicate data and want to use this Linked List to implement a Stack, or Queue, or Binary Search Tree etc. I had int data before as the member data field for my Linked List and don't want to make this overly complex by introducing a concept of ids.

First I'd like to know if my member functions for the Linked List have any edge cases I am not catching and any improvements I can make to run time efficiency . Anyway I can simplify this code further with c++11 features, shorter variable names or any other suggestions would be appreciated too.

#include <iostream>

using namespace std;

struct Node
{
    int id;
    Node* next;

    Node(int id) : id(id), next(nullptr) { }

    void append(int newId) {
        Node* current = this;
        while (current->next != nullptr) {
            if (current->id == newId) {
                return;
            }
            else {
                current = current->next;
            }
        }
        current->next = new Node(newId);
    }

    void remove(int targetId) {
        Node* current = this;
        Node* previous;
        while (current->id != targetId) {
            if (current->next != nullptr) {
                previous = current;
                current = current->next;
            }
            else {
                cout << "node not found :(\n";
                return;
            }
        }
        if (current->next == nullptr) {
            delete current;
            previous->next = nullptr;
        }
        else {
            Node* danglingPtr = current->next;
            current->id = current->next->id;
            current->next = current->next->next;
            delete danglingPtr;
        }
    }

     void print() {
        if (this->next == nullptr) {
            cout << this->id << endl;
        }
        else {
            cout << this->id << " ";
            this->next->print();
        }
    }
};

int main()
{
    Node list(1);

    list.append(2);
    list.append(3);
    list.append(4);
    list.print();

    list.remove(3);
    list.print();

    list.remove(4);
    list.print();

    list.remove(1337);
}

The namespace std is not designed for wholesale importation, see "Why is “using namespace std” considered bad practice?" for more detail.
You could instead do a using std::cout; or better qualify the three use-sites.

You don't in any way encapsulate the list and abstract over the list. It's just a bunch of Nodes. Consider putting it all into a List owning and managing the whole lot.

Trying to remove() the root-Node uncovers a bug. Try to trace it through.

pointer != nullptr is just a long-winded way to write pointer in a boolean context. Respectively for pointer == nullptr and !pointer. Yes, Java needs that, but this is C++.

When you return from the if-branch, putting the alternative in an else-branch is superfluous.

A function for printing an object should allow the caller to specify the stream, and be called operator<<.

There is no reason to rely on the compiler transforming recursion to iteration, especially as it might not always be able.

this should rarely be used explicitly.


Image Scrolling in UITableView with Parse I'm trying to create an app that is scalable. One of the main features of the app is to view images from a database to a UITableView. Here is my code:

import UIKit

class HomePageTableViewController : UITableViewController {

private var imageObjects: [ImageParseObject] = []
private var skipNumObjects = 0
private var notificationKey = "bananasaregood"

override func viewDidLoad() {
  super.viewDidLoad()
  tableView.rowHeight = UITableViewAutomaticDimension
  tableView.estimatedRowHeight = 320
  loadMoreImages()
  NSNotificationCenter.defaultCenter().addObserver(self, selector: "refreshData", name: notificationKey, object: nil)
}

func loadMoreImages() {
  let query = PFQuery(className: "ImagePost")
  query.limit = 25
  query.skip = skipNumObjects
  query.findObjectsInBackgroundWithBlock({
    objects, error in
    if error != nil {
      print(error)
    } else {
      self.imageObjects += self.convertPFObjectsToImageParseObjects(objects!)
    }
  })
  skipNumObjects += 25
}

 func refreshData() {
    tableView.reloadData()
 }

 func convertPFObjectsToImageParseObjects(pfObjects: [PFObject]) -> [ImageParseObject] {
   var imgObjects: [ImageParseObject] = []
   for i in 0..<pfObjects.count {
     let object = pfObjects[i]
     imgObjects.append(ImageParseObject(imageObject: object, index: i))
  }
  return imgObjects
  }

 }

 extension HomePageTableViewController {

 override func numberOfSectionsInTableView(tableView: UITableView) -> Int {
   return 1
 }

 override func tableView(tableView: UITableView, numberOfRowsInSection section: Int) -> Int {
   return imageObjects.count
  }

 override func tableView(tableView: UITableView, cellForRowAtIndexPath indexPath: NSIndexPath) -> UITableViewCell {
   let cell = tableView.dequeueReusableCellWithIdentifier("cell") as! HomePageTableViewCell
   let imageParseObject = imageObjects[indexPath.row]
   cell.imagePost.image = UIImage(named: "default")
   cell.imagePost.file = imageParseObject.image
   cell.imagePost.loadInBackground()
   cell.userImage.image = UIImage(named: "default")
   cell.userImage.file = imageParseObject.userImage
   cell.userImage.loadInBackground()
   cell.timeAgo.text = imageParseObject.timeAgo!
   return cell
 }
}

extension HomePageTableViewController {

override func scrollViewDidEndDecelerating(scrollView: UIScrollView) {
  if tableView.contentOffset.y >= tableView.contentSize.height - tableView.frame.height {
    print("Reached the end")
    loadMoreImages()
   }
 }
 }

Ideally, I would like this app to have the versatility to scale to thousands of users. So if I have thousands of images in the database it will still run smoothly. I'm not sure if I've accomplish that, that's why I'm asking for review. Do you think this code would fail in large scale?

Basically what I'm doing is I'm storing the objects, ImageParseObject, in an array, (Storing objects aren't as expensive as storing the images) the objects contain references to the actual image files in the database. When I load my tableview I simply reference the file and load them in the background. When I scroll to the end, I loadMoreImages(). The NSNotificationCenter is a start signal, I reload my tableview only after the first ImageParseObject is ready.

I predict that scrolling in this tableview would be choppy.  The problem you are trying to solve is pretty common - for example, find a shopping app that has to display many images of products in a UICollectionView.  The scrolling in these UICollectionViews is smooth even though you might be scrolling through hundreds of images.  How do they do this?

Here are my suggestions.

1)  Find an image caching framework to use like Kingfisher or its Objc predecessor SDWebImage.

2)  Parse objects have a URL property (according to their docs).

3)  When you populate the tableview, each cell gets configured with its own Parse object.  Within each cell, use the image caching frameworks method for setting an image on that cell's image view with that cell's Parse object's url property.

4)  Paging - if you have thousands of users, you don't want to download all 50000 at once.  Inside cellForRowAtIndexPath, you should check the current row against the data source array of the table view and see if you're about to run out of data to display, and when that happens, eg (row <= data.count - 4) call your fetch method for more Parse data.


Stack implemented using a single linked list I am studying data structures and I have implemented a stack using a linked list.

stack.h

#ifndef STACK_H
#define STACK_H

#include <stdbool.h>

typedef char StackElement;

typedef struct stack_CDT *Stack;

Stack stack_init();

void stack_destroy(Stack s);

bool stack_is_empty(Stack s);

void stack_push(Stack s, StackElement elem);

StackElement stack_pop(Stack s);

#endif

stack.c

#include <stdio.h>
#include <stdlib.h>
#include <stdbool.h>
#include <assert.h>
#include "stack.h"

typedef struct node {
    StackElement data;
    struct node *next;
} Node;

Node *node_create(StackElement data)
{
    Node *new = malloc(sizeof *new);
    new->data = data;
    new->next = NULL;
    return new;
}

void node_insert(Node **node_ref, Node *new)
{
    if (new == NULL)
        return;

    new->next = *node_ref;
    *node_ref = new;
}

void node_delete(Node **node_ref)
{
    if (*node_ref == NULL)
        return;

    Node *next = (*node_ref)->next;
    free(*node_ref);
    *node_ref = next;
}

void list_delete(Node **head_ref)
{
    while ((*head_ref) != NULL)
        node_delete(head_ref);
}

typedef struct stack_CDT {
    Node *top;
} StackCDT;

StackCDT *stack_init()
{
    StackCDT *new = malloc(sizeof *new);
    new->top = NULL;
    return new;
}

void stack_destroy(StackCDT *s)
{
    list_delete(&(s->top));
    free(s);
}

bool stack_is_empty(StackCDT *s)
{
    return s->top == NULL;
}

void stack_push(StackCDT *s, StackElement elem)
{
    Node *new = node_create(elem);
    node_insert(&(s->top), new);
}

StackElement stack_pop(StackCDT *s)
{
    if (stack_is_empty(s)) {
        fprintf(stderr, "Can't pop element from stack, stack is empty.\n");
        exit(1);
    }

    StackElement elem = s->top->data;
    node_delete(&(s->top));
    return elem;
}

stacktest.c

#include <stdio.h>
#include <string.h>
#include "stack.h"

#define MAX_STR_LEN 100

void stacktest()
{
    printf("Enter a string(not more than %d characters): ", MAX_STR_LEN);
    char input[MAX_STR_LEN + 1];
    fgets(input, sizeof input, stdin);
    char *pos;
    if ((pos = strchr(input, '\n')) != NULL)
        *pos = '\0';

    Stack stack = stack_init();
    for (char *c = input; *c; c++)
        stack_push(stack, *c);

    printf("Popped characters are: ");
    while (!stack_is_empty(stack))
        printf("%c", stack_pop(stack));
    printf("\n");

    stack_destroy(stack);
}

int main()
{
    stacktest();
    return 0;
}

Is my implementation all right?

No, your implementation is broken:

You forgot to check whether allocation succeeded.

Reviewing your code, you are going way overboard following the "small methods"-mantra in your library-code:

While the Single responsibility principle means every function should do one thing, and only one thing, the size of each task should be properly chosen.

Too little, and the function isn't a useful abstraction, using, writing and remembering it takes more effort than it saves.
(The performance-cost may be negligible (or even non-existent), if it is inlined. Consider marking it static to promote inlining and avoid exporting.)

Too much, and reusability and composability suffer.

Header:

I would avoid a typedef for the element-type. What does it buy you?
Using an opaque pointer is a good idea allowing you to replace the implementation with something more performant later. So, well done there.
Consider leaving the parameter-name out of the declaration when it doesn't make it more descriptive.
You should add a variation on stack_push which signals success, so a caller can opt in to handling failure gracefully:

bool stack_tryPush(Stack, StackElement);

Some more useful additions:

StackElement stack_peek(Stack);
StackElement stack_replace(Stack, StackElement);

Sourcefile:

You should re-order your includes: Always include the header you are implementing first, then all other headers, sorted for consistency (use the IDE's sorting-macro).
That way, you will know that your header is self-sufficient and you didn't forget a crucial dependency.
There's abort() for abnormal termination. Consider writing a helper-function if you need to output an error-message and abort in multiple places:

static void die(const char* message);

All your internal functions should not have external linkage, for performance and correctness reasons: Mark them inline.
Putting next first is more common, and possibly slightly more efficient. Also, for a single value value is more idiomatic than data.
Is there any reason why your typedef-name and struct-tag aren't quite equal? That's ittitating.
node_create is a waste: It's literally more work to read, write, and use it than leaving the single use inline.
As a rule of thumb, extracting 2 lines into their own function is a net loss of readability. Exceptons are often-used pieces of code.

Actually, that can be generalized to all your internal functions, with the possible exception of node_delete, which is marginal.
There are two kinds of precondition-checks:

Sanity checks, verifying that the programs logic is sane. These are normally disabled in a release build for performance reasons, and use assert().
Tests of untrusted input. How far you can/should/must trust an API's user must be decided in each case. It's your task as API-designer to decide how much you should penalize your users to catch egregious abuse even in release-mode, and whether there's also a debug-mode.
But there's no excuse for making your internal helper-functions re-assert preconditions even in release-mode.

As you are using C99 (see <stdbool.h>), you can use compound-literals to set all members of a struct-type comfortably even after initialization.

void stack_push(StackCDT *s, StackElement value)
{
    Node *new = malloc(sizeof *new);
    *new = (Node){s->top, value};
    if(!new) die("Failed to allocate a new node.");
    s->top = new;
}

Testprogram:

return 0; is implicit in main since C99.
Any reason for extracting your whole main into a different function?


LISP-like list class So, here's my code:

public class NList : SExp, IEnumerable<Object>
{
    private Object _car;
    private NList _cdr;

    IEnumerator<Object> IEnumerable<Object>.GetEnumerator()
    {
        return new NListEnumerator(this);
    }

    IEnumerator IEnumerable.GetEnumerator()
    {
        return new NListEnumerator(this);
    }

    public object car ()
    {
        return _car;
    }

    public NList cdr ()
    {
        return _cdr;
    }

    public NList cons (object o)
    {
        return new NList(o, this);
    }

    public NList() // empty list
    {

    }

    public NList(List<Object> lst)
    {
        if (lst.Count == 1)
        {
            this._car = lst[0];
            this._cdr = null;
        }
        else
        {
            this._car = lst[0];
            this._cdr = new NList(lst.GetRange(1, lst.Count - 1));
        }
    }

    public NList (Object fst)
    {
        this._car = fst;
        this._cdr = null;
    }

    public NList (Object fst, NList rst)
    {
        this._car = fst;
        this._cdr = rst;
    }

    public object Last()
    {
        NList list = this;
        while(list.cdr() != null)
        {
            list = list.cdr();
        }
        return list.car();
    }

    public int Count { get { return this.length(); } }

    public int length()
    {
        if (this._car == null && this._cdr == null) return 0;
        NList list = this;
        int len = 1;
        while(list.cdr() != null)
        {
            list = list.cdr();
            len++;
        }
        return len;
    }

    public NList cddr()
    {
        return this.cdr().cdr();
    }

    public object cadr()
    {
        return this.cdr().car();
    }

    public object elm(int k)
    {
        if(k == 0) return car();
        NList list = cdr();
        for(int i = 1; i < k; i++, list = list.cdr()) ;
        return list.car();
    }

    public NList append(NList lst)
    {
        NList lst1 = this;
        NList lst2 = lst;
        if (this._car == null && this._cdr == null)
            return lst2;
        foreach(var e in Reverse(lst1))
        {
            lst2 = lst2.cons(e);
        }
        return lst2;
        //return lst1.Aggregate(lst2, (NList acc, object b) => acc);
    }

    public static NList Reverse(NList lst)
    {
        NList l = lst;
        NList res = null;
        while(l != null)
        {
            res = new NList(l.car(), res);
            l = l.cdr();
        }
        return res;
    }

    public override string ToString ()
    {
        if (this._car == null && this._cdr == null) return "()";
        string s = "(";
        for(int i = 0; i<this.Count-1; i++)
        {
            s+=this.elm(i).ToString() + " ";
        }
        s+=this.Last().ToString() + ")";
        return s;
    }
    public NObj[] ToNObjArray()
    {
        NObj[] a = new NObj[this.Count];
        int k = 0;
        foreach (var e in this)
        {
            a[k] = (e as NObj);
            k++;
        }
        return a;
    }
    public object[] ToArray()
    {
        object[] a = new object[this.Count];
        int k = 0;
        foreach(var e in this)
        {
            a[k] = e;
            k++;
        }
        return a;
    }
}

public class NListEnumerator : IEnumerator<Object>, IEnumerator
{
    NList list;
    NList tmp;
    public void Dispose()
    {
    }
    public NListEnumerator(NList list)
    {
        this.list = list;
        this.tmp = list.cons(null);
    }

    public object Current
    {
        get { return tmp.car(); }
    }

    public bool MoveNext()
    {
        tmp = tmp.cdr();
        return (tmp != null);
    }

    public void Reset()
    {
        tmp = list.cons(null);
    }
}

I am kinda worried that I have no separate class for simple pairs (eg: non-properly terminated lists like (1 . 2), (a . b)). Should I create a separate class (and an interface for both classes), or should I try to expand this one?

TIA.

There is a serious flaw in your handling of the empty list. A user would expect that these definitions are equivalent, but they ain't:

Nlist n1 = new NList("x");
Nlist n2 = new NList("x", new NList());

I strongly recommend to have an explicit subclass for the empty list (possibly a singleton) and to prohibit null for _cdr.


Inorder, Preorder, and Postorder traversal of a binary tree in C++ I am a newbie in this world of data structures and algorithms (returning to these topics after 5 long years).  Can someone please check if the code I have written for Binary Tree is OK?

Test-case
10, 3,11,11,5,0

PrintInorder
3,0,5,10,11,11

PrintPreOrder
10,3,0,5,11,11

PrintPostOrder
3,0,5,11,11,10

using namespace std;

template<typename T>
struct Node
{
    T data;
    Node<T>* left;
    Node<T>* right;

    Node()
    {
        left = nullptr;
        right = nullptr;
    }
};

template<typename T>
class BTree
{
public:
    BTree();
    ~BTree();
    void Add(T val);
    void Print(int o);
    T Find(T key);
private:
    Node<T>* root;
    Node<T>* temp;
    int height;
    Node<T>* Add(T val, Node<T>* node);
    void PrintInorder(Node<T>* ptr);
    void PrintPreOrder(Node<T>* ptr);
    void PrintPostOrder(Node<T>* ptr);
};

template<typename T>
T BTree<T>::Find(T key)
{
    Node<T>* current = this->root;

    while (current!= nullptr)
    {
        if (current->data == key)
        {
            return current->data;
        }
        else if(current->data > key)
        {
            current = current->left;
            continue;
        }
        else if (current->data < key)
        {
            current = current->right;
            continue;
        }
    }
}

template<typename T>
void BTree<T>::PrintInorder(Node<T>* ptr)
{
    PrintPreOrder(ptr->left);
    cout<<ptr->data<<endl;
    PrintPreOrder(ptr->right);
}

template<typename T>
void BTree<T>::PrintPostOrder(Node<T>* ptr)
{
    PrintPreOrder(ptr->left);
    PrintPreOrder(ptr->right);
    cout<<ptr->data<<endl;
}

template<typename T>
BTree<T>::BTree()
{
    root = nullptr;
    height = 0;
}

template<typename T>
void BTree<T>::PrintPreOrder(Node<T>* ptr)
{
    if (ptr!=nullptr)
    {
        cout<<ptr->data<<endl;
    }

    if(ptr->left!=nullptr)
        PrintPreOrder(ptr->left);
    if(ptr->right!=nullptr)
        PrintPreOrder(ptr->right);
}

template<typename T>
void BTree<T>::Print(int o)
{
    if (o==1)
    {
        PrintInorder(root);
    }
    else if (o==2)
    {
        PrintPreOrder(root);
    }
    else if(o==3)
    {
        PrintPostOrder(root);
    }
}

template<typename T>
BTree<T>::~BTree()
{
}

template<typename T>
void BTree<T>:: Add(T val)
{
    if (root==nullptr)
    {
        root = Add(val, nullptr);
    }
    else
    {
        root = Add(val,root);
    }
}

template<typename T>
Node<T>* BTree<T>::Add(T val, Node<T>* node)
{
    if (node == nullptr)
    {
        node = new Node<T>();
        node->data = val;
        return node;
    }
    else
    {
        if (val <= node->data)
        {
            node->left = Add(val,node->left);
        }
        else
        {
            node->right =  Add(val,node->right);
        }
    }

    return node;
}

I can see a few changes that at least might be worth considering.

Minimize Requirements

All else being equal, you typically want to minimize the requirements you place on the type contained in a container like this. For example, where you need to do comparisons, you can use the same operator (typically <) to do all the comparisons. For example, in your Find, we can do just a little rearranging to get this:

template<typename T>
T BTree<T>::Find(T key)
{
    Node<T>* current = this->root;

    while (current!= nullptr)
    {
        if (current->data < key)
            current = current->right;
        else if (key < current->data)
            current = current->left;
        else // Neither is less than the other, so they must be equal
            return current->data;
    }
}

With this, T only needs to define "less than" for our code to work (where it previously required <, > and ==).

Check recursion

Your PrintInOrder looks broken:

template<typename T>
void BTree<T>::PrintInorder(Node<T>* ptr)
{
    PrintPreOrder(ptr->left);
    cout<<ptr->data<<endl;
    PrintPreOrder(ptr->right);
}

This seems to do a pre-order traversal of the left sub-tree, prints the current, then a pre-order traversal of the right sub-tree. For in-order, you want an in-order traversal of the left-subtree, process the current node, then in-order traversal of the right subtree. With only a few nodes (so the tree is never very deep) this probably may not be visible, but it's clearly wrong.

The same applies to your PrintPostOrder.

Simplify where possible

At least to me, your PrintPreOrder seems unnecessarily complex. I'd do something like:

template <class T>
void PrintPreOrder(Node<T> *ptr) {
    if (ptr == nullptr)
        return;

    std::cout << ptr->data << "\n";
    PrintPreOrder(ptr->left);
    PrintPreOrder(ptr->right);
}

With this we only do one check for a null pointer in one place.

Likewise, your Add currently looks like this:

template<typename T>
void BTree<T>:: Add(T val)
{
    if (root==nullptr)
    {
        root = Add(val, nullptr);
    }
    else
    {
        root = Add(val,root);
    }
}

This checks whether root is a null pointer, and if it is it explicitly passes a null pointer. Otherwise, it pass root. If we just pass root, (whether it's a null pointer or not) we get exactly the same effect.

template<typename T>
void BTree<T>:: Add(T val)
{
    Add(val, root);
}

Avoid std::endl

When people use std::endl, they typically just want to write a new-line to a stream. Unfortunately, that's not quite all that std::endl actually does--in addition to writing the new-line, it flushes the stream. Flushing the stream unnecessarily can lead to a dramatic speed loss (e.g., 10x is pretty common).

On the rare occasion that you actually do want to both write a new-line and flush the stream, I think it's better to do that explicitly:

std::cout << "\n" << std::flush;

At least in my experience, actual uses for this are fairly rare though.

Prefer enum's to magic numbers

Your Print currently sorts out the order for the printing like this:

if (o==1)       // ...
else if (o==2) // ...
else if(o==3)  // ...

Rather than 1, 2 and 3, I'd rather use an enumeration:

enum order { PRE_ORDER, IN_ORDER, POST_ORDER };

Then Print would look something like this:

template<typename T>
void BTree<T>::Print(order o)
{
    switch (o) {
        case PRE_ORDER:  PrintPreOrder(root);    break;
        case IN_ORDER:   PrintInOrder(root);     break;
        case POST_ORDER: PrintPostOrder(root);   break;
    }
}

At least to me, this seems considerably clearer and more readable (also note the use of a switch statement to directly express the intent of choosing one of multiple alternatives).

Don't Repeat Yourself

Right now, you have code in both Add and Find to find a node (if any) with a particular value. Oddly, the two use entirely different approaches to this problem. Rather than having two copies of code to find a node based on a key, I'd rather have one piece of code that's used from both:

Node *&find_key(T key) {
    Node *current = root;

    for (Node *current = root; current != nullptr;) {
        if (key < current->data)
            current = current->left;
        else if (current->data < key)
            current = current->right;
        else
           break;
    }
    return current;
}

With this, we can rewrite Find to look something like:

T Find(T key) {
    Node *pos = find_key(key);
    if (pos != nullptr)
        return pos->data;
}

Likewise, Add would look something like this:

void Add(T key) {
    Node *&pos = find_key(key);
    if (pos == nullptr)
        pos = new Node(key);
}

Avoid Undefined Behavior

As currently defined, if you call Find for a key that isn't present in the tree, execution flows off the end of the function without returning a value, leading to undefined behavior. There are various ways to deal with this, such as a Maybe or Optional type that carries both the return value itself and an indication whether a value is actually present. Another possibility (probably not as nice, but at least it does work) is to use an approach about like the find_key shown above, returning a null pointer if a value isn't present.


Extracting key and array value from javascript object? My data looks like this:

const sleepStages = [
  {deep: Array(40)},
  {light: Array(40)},
  {rem: Array(40)},
  {awake: Array(40)},
]

I'm using the key and values separately and am using TypeScript.

sleepStages.flatMap((sleepStage) => {
  const stageName = Object.keys(sleepStage)[0];
  const stageValues = Object.values(sleepStage)[0];
  console.log(stageName, stageValues);
});

Does this approach make sense? Whenever I type [0] it smells.

A more concise approach would be to use Object.entries to get the 0th key and value at once:

const [stageName, stageValues] = Object.entries(sleepStage)[0];

Yes, the [0] looks weird, but unless you know the keys and/or in advance, you have to use a method which iterates over them, and then you need to extract the first item. In Javascript, there's no good way of avoiding the [0] - there's nothing like Object.prototype.getFirstEntry.

You could nest the destructuring on the left side, but that looks much less readable IMO:

const [[stageName, stageValues]] = Object.entries(sleepStage);

Ideally, in this sort of situation, you would fix the input data so that the keys are predictable and static:

const sleepStages = [
  { name: 'deep', values: Array(40) },
  { name: 'light', values: Array(40) },
  { name: 'rem', values: Array(40) },
  { name: 'awake', values: Array(40) },
]

Then, in the loop, you could do:

const { name, values } = sleepStage;


Simple command line tool to query database. Need code optimized public class ConsoleQuery {
    public static void main(String[] args) throws SQLException {
        if (args.length == 0) {
            System.out.println("You must specify name");
            return;
        }
        String name = args[0];
        Map < String, Object > device = getDeviceByName(name);
        if (device != null) {
            System.out.printf("Found device by name %s: %s",
                name, device);
        } else {
            System.out.printf("There is no device by name %s", name);
        }
    }

    private static Map < String, Object > getDeviceByName(String name) throws SQLException {
        DriverManager.registerDriver(new com.mysql.jdbc.Driver());
        Connection connection = DriverManager.getConnection("jdbc:mysql://127.0.0.1:3306/test", "test", "test");
        PreparedStatement statement = connection.prepareStatement("SELECT id, mac, name FROM device");
        ResultSet rs = statement.executeQuery();
        List < Map < String, Object >> deviceList = new LinkedList < Map < String, Object >> ();
        try {
            ResultSetMetaData rsmd = rs.getMetaData();
            Integer columnCount = rsmd.getColumnCount();
            while (rs.next()) {
                Map < String, Object > row = new LinkedHashMap < String, Object > ();
                for (int i = 1; i <= columnCount; i++) {
                    row.put(rsmd.getColumnName(i), rs.getObject(i));
                }
                deviceList.add(row);
            }
        } finally {
            rs.close();
        }
        for (Map < String, Object > device: deviceList) {
            if (name.equals(String.valueOf(device.get("name")))) {
                return device;
            }
        }
        return null;
    }
}

Well, the most important optimization you can make to this code is to improve your SQL query. You are currently pulling down the entire db but only care about one row; you're doing half your query on the client side. Use SQL, not Java, for selecting data from the db! Consider the following version of getDeviceByName:

private static Map<String, Object> getDeviceByName(String name) throws SQLException {
    DriverManager.registerDriver(new com.mysql.jbdc.Driver());
    Connection connection = DriverManager.getConnection(
        "jdbc:mysql://127.0.0.1:3306/test", "test", "test");
    PreparedStatement statement = connection.prepareStatement(
        "SELECT id, mac, name FROM device WHERE name = ? LIMIT 1");
    statement.setString(1, name);
    ResultSet rs = statement.executeQuery();
    try {
        if (!rs.next()) {
            return null;
        }
        return readRowIntoMap(rs);
    } finally {
        rs.close();
    }
}

private Map<String, Object> readRowIntoMap(ResultSet rs) throws SQLException {
    ResultSetMetaData metadata = rs.getMetaData();
    Map<String, Object> row = new LinkedHashMap<>();
    for (int i = 1; i <= metadata.getColumnCount(); i++) {
        row.put(metadata.getColumnName(i), rs.getObject(i));
    }
    return row;
}


Appointment Program In Ruby For a homework assignment for an online class (pretty much learning everything from the internet anyway) we were asked to build an appointment program. This is just a start, however I feel I am definitely over-complicating things and going about this the complete wrong way.
These were the instructions we were given:

Implement a superclass Appointment and sub classes OneTime, Day and Month. An appointment has a description (for example, "Root Canal"), and dates information (you can use Date object or Int Year, Int Month, Int Day). Fill an array of Appointment objects with a mixture of appointments.
Write a method OccursOn inside each of the sub classes that checks whether the appointment occurs on that date (OneTime), day (Day) or month (Month). Ask the user to enter a date to check (for example, 2006 10 5), and ask to user if they want to check against OneTime, Day or Month appointment. Based on what the user selected, OccursOn inside each of the sub class should run and display any matching appointment and associated descriptions.

require 'date'

class Appointment
    attr_accessor :info, :date, :year, :day, :month
    def initialize(info, date)
        @info = info
        @date = Date.parse(date)
    end

    def get_info
        @info
    end

    def get_date
        @date
    end

    def get_year
        @year = date.year
    end

    def get_month
        @month = date.mon
    end

end

class OneTime < Appointment

    def OccursOn(year, month, day)
        puts "The year you entered is #{year}"
        puts "The month you entered is #{month}"
        puts "The day you entered is #{day}"
        appointments.each do |x|
            puts "Month #{x.get_month}"
            if x.get_month == month
                puts x.get_date
                puts x.get_info
            end
        end
    end

end

class Day < Appointment

    def OccursOn(day)
        puts day
    end

end

class Month < Appointment

    def OccursOn(month)
        puts month
    end
end

appointments = []
appointment1 = Appointment.new("Root Canal", "31/10/2017")
appointment2 = Appointment.new("Cavity Filling", "31/12/2017")
appointments << appointment1 << appointment2
puts appointment1.get_date
puts appointment2.get_date
puts appointment1.get_info

onetime1 = OneTime.new("Root Canal", "31-10-2017")

puts "Enter a date to check (for example, 31-10-2017)"
checkdate = gets.chomp
checkdate = Date.parse(checkdate)

puts "Would you like to check against OneTime, Day or Month appointment?"
checkagainst = gets.chomp

if checkagainst == "OneTime" or checkagainst == "onetime"
    onetime1.OccursOn(checkdate.year, checkdate.month, checkdate.day)
elsif checkagainst == "Day" or checkagainst == "day"
    puts "Day" # run day check function
elsif checkagainst == "Month" or checkagainst == "Month"
    puts "Month" # run month check function
else
    puts "Invalid option."
end

I feel I am definitely overcomplicating things

I agree with you that the code is longer than necessary. Let's take a look.

YAGNI - you ain't gonna need it

Try to avoid writing code until a caller (such as a test method for a Story) forces you to write it. Then you know you really need it.

attr_accessor :info, :date, :year, :day, :month

The first two look good. I didn't notice any compelling reason to list y,m,d, as caller could get those from date. The day might be called mday for consistency with library.

def get_info
    @info
end

def get_date
    @date
end

Ummm, didn't attr_accessor already take care of those? We're writing in ruby, not java. Take advantage of its strengths!

I commented above that get_year & get_month seem redundant. Also, it seems odd to list them without get_mday (or perhaps get_day). Better to elide them entirely.

    appointments.each do |x|

This seems to be a global reference. I can't imagine that's a good thing. Consider moving that list into a class, such as the Appointment class.

        if x.get_month == month

I suggest it would be more natural to phrase that as x.date.mon.

Your recurring Day & Month classes appear to be unimplemented stubs, so far.
You have a design decision to make. Will adding a recurring appointment create several new entries until a date "very far" in the future? Or will it add a single entry and then you deal with that at query time? There is no right answer; each offers its own trade-offs.

puts "Would you like to check against OneTime, Day or Month appointment?"
checkagainst = gets.chomp

This seems a cumbersome UX. Rather than prompting, simply loop over all three, reporting the appointment and its type whenever you find a match.


Codewars: Make a spiral Link to full problem description: https://www.codewars.com/kata/534e01fbbb17187c7e0000c6
My code:
def spiralize(size):
    def get_change_values():
        change_x = 0
        change_y = 0
        if direction == 'right':
            change_x = 1
        elif direction == 'left':
            change_x = -1
        elif direction == 'down':
            change_y = 1
        elif direction == 'up':
            change_y = -1
        return change_x, change_y

    def check_collision():
        if 0 <= y + change_y * 2 < size and 0 <= x + change_x * 2 < size:
            if direction in ('right', 'left'):
                if spiral[y][x + change_x * 2]:
                    return True
            elif direction in ('down', 'up'):
                if spiral[y + change_y * 2][x]:
                    return True
        return False

    spiral = [[0 for _ in range(size)] for _ in range(size)]
    directions = ['right', 'down', 'left', 'up']
    x = y = 0
    change_directions = 0
    while change_directions < size:
        direction = directions[change_directions % 4]
        change_x, change_y = get_change_values()
        while 0 <= y + change_y < size and 0 <= x + change_x < size:
            spiral[y][x] = 1
            if check_collision():
                break
            y += change_y
            x += change_x
        change_directions += 1
    return spiral

The basic idea of what my algortihm is trying to do here:

Initialize a matrix of 0s of the given size
Starting from the top left and heading towards the right, go through each element and change it to '1' until it can no longer move in that direction
Once it can no longer move in a given direction, change the direction based on the order right -> down -> left -> up
Repeat until spiral is complete (until size-1 changes of direction have been made)

The main advice I'm looking for here is how to better structure this code to make it more readable and understandable to the average viewer. Granted, if there are any glaring performance optimizations that can be made to improve this code, I'd be more than willing to hear those out as well. But again, I'm mostly looking for advice on how to make this code cleaner in general. I'm not exactly a new programmer per se, but the concept of 'clean code' is still something I very much struggle with.

Without changing your algorithm we can make your code simpler in a few places. I'd say the algorithm is quite sensible, I would've chosen the same approach.

Comments and docstrings
Providing some comments here and there and explaining the general algorithmic approach in a docstring would be good start for increasing readability. In particular, it's not intuitively obvious why while change_directions < size:
is the correct approach.

Nested functions
I find nested functions to be relatively hard to read. Especially when they start using variables from the outer scope (the outer function), thereby mixing namespaces. Personally, I generally try to avoid them. In the case of check_collision I'd say it might improve readability as it reduces the maximum indentation level. That's generally a good thing, since every indentation level is another layer of context to keep track of.

get_change_values()
We can easily get rid of this function by taking a closer look at the underlying logic. We don't actually need to pass around direction strings, since we always follow the same pattern RIGHT -> DOWN -> LEFT -> UP -> RIGHT -> .... The itertools module from the standard library provides cycle, which basically provides an endless iterator for a given iterable (example from the docs: cycle('ABCD') --> A B C D A B C D A B C D ...). So we might want to cycle(['right', 'down', 'left', 'up']). But since we only need the strings to get the corresponding change_x, change_y values, we can replace them by those values directly:
directions = cycle(((1, 0), (0, 1), (-1, 0), (0, -1)))

We now replace
direction = directions[change_directions % 4]
change_x, change_y = get_change_values()

by
change_x, change_y = next(directions)

and get rid of get_change_values().

0 <= variable < limit
Conditions that follow the pattern 0 <= variable < limit can be replaced by variable in range(limit). I personally find this to be more readable, but it's not always preferable and up to personal preference.

check_collision()
This function can be simplified to the point where we could directly put it into our nested while loop. This is a valid option here. But as mentioned above you might want to keep it in a seperate function to reduce the maximum indentation level.
We don't actually need to check for the direction:
if direction in ('right', 'left'):
    if spiral[y][x + change_x * 2]:
        return True
elif direction in ('down', 'up'):
    if spiral[y + change_y * 2][x]:
        return True

if we simply check
if spiral[y + change_y * 2][x + change_x * 2] == 1:
    return True

We can also directly return:
def check_collision() -> bool:
    if y + change_y * 2 in range(size) and x + change_x * 2 in range(size):
        return spiral[y + change_y * 2][x + change_x * 2] == 1

    return False

I find exit conditions to generally improve code readability as well. They're usually closer to the reader's thought process ("If the step is out of bounds there's no collision, so we don't need to ckeck anything else. Otherwise check if the step leads to a 1."):
def check_collision() -> bool:
    if y + change_y * 2 not in range(size) or x + change_x * 2 not in range(size):
        return False

    return spiral[y + change_y * 2][x + change_x * 2] == 1

Both options are already pretty clean. I personally prefer this branchless approach:
def check_collision() -> bool:
    step_inside_bounds = y + change_y * 2 in range(size) and x + change_x * 2 in range(size)
    return step_inside_bounds and spiral[y + change_y * 2][x + change_x * 2] == 1

This works because and is evaluated lazily, so if step_inside_bounds is False, the rest of the condition is not evaluated.

spiral initialization
Instead of
spiral = [[0 for _ in range(size)] for _ in range(size)]

we can initialize spiral like this
spiral = [[0] * size for _ in range(size)]

This again is just another way to do things, and I wouldn't say it's definitely better for multi-dimensional lists. For one-dimensional lists, I would prefer the suggested syntax. Compare these examples:
my_list = [0 for _ in range(my_length)]
my_list = [0] * my_length

Complete code
I renamed change_directions to direction_changes and check_collision to collision_in_two_steps. I also added type hints (PEP 484).
from itertools import cycle

def spiralize(size: int) -> list[list[int]]:
    def collision_in_two_steps() -> bool:
        step_inside_bounds = y + change_y * 2 in range(size) and x + change_x * 2 in range(size)
        return step_inside_bounds and spiral[y + change_y * 2][x + change_x * 2] == 1

    directions = cycle(((1, 0), (0, 1), (-1, 0), (0, -1)))

    spiral = [[0] * size for _ in range(size)]
    x, y = 0, 0
    direction_changes = 0

    while direction_changes < size:
        change_x, change_y = next(directions)

        while y + change_y in range(size) and x + change_x in range(size):
            spiral[y][x] = 1

            if collision_in_two_steps():
                break

            y += change_y
            x += change_x

        direction_changes += 1

    return spiral


Implementing the "Thinking in React" example from React documentation without using Refs I'm quite new to React and was working through the "Thinking in React" example,
I noticed they used refs for updating state, but I also read on SO that using refs isn't that good in the long run, so I tried to redo this without using refs:

JSFiddle

var SearchBar = React.createClass({
    handleChange: function(event) {
        if (event.target.type ==='text') {
            this.props.onUserInput(
                event.target.value,
                this.props.inStockOnly
            );
        } else {
            this.props.onUserInput(
                this.props.filterText,
                event.target.checked
            );
        }
    },

    render: function() {
        return (
            <form>
                <input
                    type="text"
                    placeholder="Search..."
                    value={this.props.filterText}
                    onChange={this.handleChange}
                />
                <p>
                    <input
                        type="checkbox"
                        checked={this.props.inStockOnly}
                        onChange={this.handleChange}
                    />
                    {' '}
                    Only show products in stock
                </p>
            </form>
        );
    }
});

It works but it seems repetitive, how would someone comfortable with React do this without refs?

Actually, I'd worry more about the issue that's bound to happen - your handleChange trying to do everything. I suggest you split them off to two distinct functions, one for the text input, and the other for the checkbox. This is much more maintainable in the long run, as each function is only responsible for one task. This ain't mixed martial arts :P

There's nothing wrong with refs. They're simply for easy access to the DOM elements inside the component. What's probably wrong is when an parent component accesses child refs, or a component exposing its refs to other components. Components should only receive props and state, and emit events with data (like a regular DOM element). You don't mess with how it implements its internals.

Anyways, not much change here:

var SearchBar = React.createClass({
  handleInputChange: function(){
    this.props.onUserInput(this.refs.myInput.value, this.props.inStockOnly);
  },
  handleCheckboxChange: function(){
    this.props.onUserInput(this.props.filterText, this.refs.myCheckbox.value);
  },
  render: function() {
    return (
      <form>
        <input type="text" placeholder="Search..." value={this.props.filterText} onChange={this.handleInputChange} ref="myInput" />

            <input type="checkbox" checked={this.props.inStockOnly} onChange={this.handleCheckboxChange} ref="myCheckbox" id="myCheckbox" />
            Only show products in stock

      </form>
    );
  }
});

Now regarding your markup,
 means paragraph but you have an <input>. I believe
 was simply used for layouting.
 is more appropriate. It also appears that your text is a label for the input. I suggest you wrap them in a  so clicking the label toggles the box. An additional  is recommended since you might want to style the text differently from a text-only label.


Handling multiple MassTransit response types I needed to publish a message across the ESB (MassTransit) that will return a different type of object depending on how the message is processed. I wanted to implement this in a generic way so I could wire up other services with similar requirements in the same way.

Publish a request

public IGenericResponse<TPayload> PublishRequest<TMessage, TPayload>(TMessage message, Type[] potentialResponseTypes)
        where TMessage : class
{
    IGenericResponse<TPayload> commandResult = null;

    _bus.PublishRequest(message, c =>
    {
        foreach (Type responseType in potentialResponseTypes)
        {
            Type genericHandler = typeof(GenericHandler<,>).MakeGenericType(new[]{ responseType, typeof(TMessage)});

            var handler = new Action<object>(result =>
            {
                commandResult = (IGenericResponse<TPayload>) result;
            });

            Activator.CreateInstance(genericHandler, new object[] {c, handler});
        }

        c.SetTimeout(10.Seconds());
    });

    return commandResult;
}

Generic Handler to wire it up

internal class GenericHandler<TPayload, TMessage>
    where TMessage : class
    where TPayload : class
{
    public GenericHandler(InlineRequestConfigurator<TMessage> configurator, Action<object> handler)
    {
        configurator.Handle<GenericResponse<TPayload>>(handler);
    }
}

Thoughts?

It's possible, providing c is your response from the bus, to do a lot of this without needing Reflection. Instead we can use a separate method and the joys of implicit typing to work out the type of TResponse for us.

Proposed solution

public IGenericResponse<TPayload> PublishRequest<TMessage, TPayload>(TMessage message, Type[] potentialResponseTypes)
        where TMessage : class
{
    IGenericResponse<TPayload> commandResult = null;

    _bus.PublishRequest(message, response =>
    {
        foreach (Type responseType in potentialResponseTypes)
        {
            var genericHandler = CreateHandler(response, message);
        }

        c.SetTimeout(10.Seconds());
    });

    return commandResult;
}

private GenericHandler<TResponse, TMessage> CreateHandler<TResponse,TMessage>(TResponse response, TMessage message)
{
    var handler = new Action<object>(result =>
    {
        commandResult = (IGenericResponse<TPayload>) result;
    });
    return new GenericHandler<TResponse, TMessage>(response, handler);
}


Passing data from one model to another In my task I have to pass some data from one model to another, where each model contains lists:

List<VisualProcess> vProcesses = new ArrayList<VisualProcess>();
    for (Process process : processList) {
                VisualProcess vProcess = new VisualProcess();
                vProcess.setId(process.getId());
                List<VisualProcessStep> steps = new ArrayList<VisualProcessStep>();
                List<VisualProcessStepConnection> connections = new ArrayList<VisualProcessStepConnection>();
                for(ProcessStep processStep : process.getSteps()){
                    VisualProcessStep vps = new VisualProcessStep();
                    vps.setActivityId(processStep.getActivityId());
                    vps.setDefinition(processStep.getDefinition());
                    vps.setId(processStep.getId());
                    vps.setParameters(processStep.getParameters());
                    vps.setStatus(processStep.getStatus());
                    for (Activity activity : process.getActivities()) {
                        if (activity.getId().equals(vps.getActivityId())){
                            vps.setActivityName(activity.getName());
                            vps.setActivityRealization(activity.getRealization());
                            break;
                        }
                    }
                    steps.add(vps);
                    for (ProcessStep nextProcessStep : processStep.getNextSteps()) {
                        VisualProcessStepConnection vpsc = new VisualProcessStepConnection();
                        vpsc.setFrom(processStep.getId());
                        vpsc.setTo(nextProcessStep.getId());
                        connections.add(vpsc);
                    }
                }
                vProcess.setSteps(steps);
                vProcess.setConnections(connections);
                vProcesses.add(vProcess);
            }

However, while I think it looks awful, I don't have any other ideas as to making it more elegant. Another issue is about the size of the method: Would it be better to split it into several, smaller methods?

Is it too awful code?

Not too awful and not even bad, but the huge wall of code looks ugly.

There are a few minor issues like missing blank after "for" in

for(ProcessStep processStep : process.getSteps()){

and before "{" in

if (activity.getId().equals(vps.getActivityId())){

but that's something your IDE can do for you (Ctrl-Shift-F in Eclipse).

Otherwise, all you need is to extract a few methods like

private VisualProcessStep toVisualProcessStep(Process process, ProcessStep processStep) {
    VisualProcessStep result = new VisualProcessStep();
    result.setActivityId(processStep.getActivityId());
    result.setDefinition(processStep.getDefinition());
    result.setId(processStep.getId());
    result.setParameters(processStep.getParameters());
    result.setStatus(processStep.getStatus());
    for (Activity activity : process.getActivities()) {
        if (activity.getId().equals(result.getActivityId())) {
            result.setActivityName(activity.getName());
            result.setActivityRealization(activity.getRealization());
            break;
        }
    }
    return result;
}

This can be repeated many times and you surely shouldn't extract everything. When the methods don't get used elsewhere, this is very subjective as you're trading a too long method for too many methods.

The sweet spot are methods somewhere about 15 lines long. As all the methods do is just copying everything and there isn't much to think about, they may be a bit longer. I'd personally split it into probably 3 methods.

Obviously, there may be a smarter way avoiding the manual copying. It surely can be done via reflection, but reflection is verbose and slow and ugly itself, so I wouldn't consider it for this task.

Maybe you could create a constructor (or a factory method) for VisualProcessStep?


Locating a sequence in a sorted array I read the following question: Searching an element in a sorted array and I thought that I could give it a try in Python.

Given a sorted list of integers and an integer. Return the (index) bounds of the sequence of this element in the list.

Example:

l = [1, 2, 2, 3, 3, 3, 4, 4, 4, 4, 4, 4, 6, 7, 8, 9, 9, 9, 9]

0 not found in list
1:(0, 0)
2:(1, 2)
3:(3, 5)
4:(6, 11)
5 not found in list
6:(12, 12)
7:(13, 13)
8:(14, 14)
9:(15, 18)

Here is my program (using Python 3).
It uses a dichotomic search and returns the bounds to help the search of the start and end of the sequence.

def main():
    l = [1, 2, 2, 3, 3, 3, 4, 4, 4, 4, 4, 4, 6, 7, 8, 9, 9, 9, 9]
    print(l)
    for i in range(10):
        try:
            print(find_sequence(i, l))
        except Exception as e:
            print(str(e))

def find_sequence(x, l):
    """Return a tuple (begin, end) containing the index bounds of the sequence of x"""
    left, found, right = dichotomic_search(x, l)
    begin = outside_bound(x, l, left, found)
    end = outside_bound(x, l, right, found)
    return begin, end

def outside_bound(x, l, outside, inside):
    """Return the outside bound of the sequence"""
    if l[outside] == x:
        return outside

    middle = -1
    previous_middle = -2
    while middle != previous_middle:
        previous_middle = middle
        middle = (outside + inside) // 2
        if l[middle] == x:
            inside = middle
        else:
            outside = middle
    return inside

def dichotomic_search(x, l):
    """Return a tuple of indexes (left, found, right)

    left: leftmost index where x might be found
    found: index where x is
    right: rightmost index where x might be found
    """
    left = 0
    right = len(l) - 1

    if l[left] > x or l[right] < x:
        raise Exception(str(x) + ' not found in list')

    if l[left] == x:
        return left, left, right
    if l[right] == x:
        return left+1, right, right # we know that l[left]!=x

    while left < right:
        middle = (left + right) // 2
        if l[middle] == x:
            return left, middle, right
        elif l[middle] < x:
            left = middle + 1 # to prevent fixed point
        elif l[middle] > x:
            right = middle # impossible to do -1 because of the integer division

    raise Exception(str(x) + ' not found in list')

if __name__ == "__main__":
    main()

I'm not so fond of the middle != previous_middle, but I didn't find a more elegant way (yet).

I am afraid you're approach is too complicated. Just create a function that finds the first occurrence of an element using a binary search, an another function that finds the last one. And then you can write:

def find_indexes(xs, x)
  start = find_first_index(xs, x)
  return ((start, find_last_index(xs, x, start=start)) if start else None)

For a simple implementation of find_first_index and find_last_index, check bisect.bisect_left and bisect.bisect_right source code here.


Ensuring user input is an integer in a range I have isolated a little bit of code that was causing a small debate between myself and another user.  I have taken some of the things that he said and meshed it with the code that was being reviewed in the first place here.

Original Code

while (!validInput)
{
    var playerChoice = -1;
    Console.WriteLine("Please choose your Gesture ");
    gameSetup(listOfGestures);
    try
    {
        playerChoice = Convert.ToInt32 (Console.ReadLine());
    }
    catch (FormatException e)
    {
        validInput = false;
    }

    if (playerChoice > 0 && playerChoice <= listOfGestures.Count)
    {
        playerGesture = listOfGestures[playerChoice - 1];
        validInput = true;
    }
    else
    {
        validInput = false;
    }
}

Here is what I came up with based on our conversation.

New Code

var validInput = false; //forgot this part
while (!validInput)
{
    var playerChoice = -1;
    Console.WriteLine("Please choose your Gesture ");
    gameSetup(listOfGestures);
    validInput = int.TryParse(Console.ReadLine(),out playerChoice);
    if (playerChoice > 0 && playerChoice <= listOfGestures.Count)
    {
        playerGesture = listOfGestures[playerChoice - 1];
        validInput = true;
    }
}

Do I need to create a new method to sort out user input and whether or not it is an integer inside the min and max bounds?

The second sample is definitely better. I would be inclined to continue to improve the code by separating mechanisms from policies. The policy is the code that actually expresses the meaning of the program; the mechanism is the code that expresses what specific operations implement the policy.

This idea comes from security design; you don't want the code that computes "is Bob allowed to open this door?" and the code that computes "is that really Bob's card key?" to be the same code. The first is a policy, the second is a mechanism. But it applies generally to all code.

By separating mechanism from policy we enable both to be understood more easily. The "main line" of your code should read like the specification. If you had to describe your code in English you'd probably say something like "present the user with a choice of gestures. If the user chooses an invalid gesture, keep trying until they choose a valid gesture."  That's the meaning of your program, but that's not what your code looks like. Rather, your code looks like the most important things in the world are integer and bool variables, list counts, and so on.

Let's identify a mechanism: parsing an integer and testing whether it is in range is the mechanism behind the policy of "the user must choose a valid gesture". So let's isolate that mechanism into a purely mechanistic method, that knows nothing about your policy domain:

static class Extensions
{
    public static int? BoundedParse(this string str, int lower, int upper)
    {
        if (str == null)
            return null;
        int result;
        bool success;
        success = int.TryParse(str, out result);
        if (!success)
            return null;
        if (result < lower)
            return null;
        if (result > upper)
            return null;
        return result;
    }
}

Let's now rewrite your loop as a do-while instead of a while. Look how much ugly mechanism we removed, leaving your policy shining brightly through:

gameSetup(listOfGestures);
int? choice;
do
{
    Console.WriteLine("Please choose your Gesture ");
    choice = Console.ReadLine().BoundedParse(1, listofGestures.Count);
}
while(choice == null);
playerGesture = listOfGestures[choice.Value - 1];

And as a bonus, we now have a reusable method that we can apply to any task that requires that a string be parsed as a bounded integer.

We can take this even further. Isn't that loop a mechanism? Move it into a helper method:

static int PromptForNumber(string prompt, int lower, int upper)
{
    int? choice;
    do
    {
        Console.WriteLine(prompt);
        choice = Console.ReadLine().BoundedParse(lower, upper);
    }
    while(choice == null);
    return choice.Value;
}

And now your method is:

gameSetup(listOfGestures);
int choice = PromptForNumber("Please choose your gesture", 1, listofGestures.Count);
playerGesture = listOfGestures[choice - 1];

Now the code is super clear what is going on at the policy level because all the stuff you don't care about is now the responsibility of some mechanism somewhere. (And again we have a great mechanism for prompting for a number that we can re-use later.)

Even better: maybe there is a better way to display a prompt and get a number. If we decided that we wanted to write a GUI instead, we can change the mechanism code without changing the policy code. The policy code doesn't care how the valid gesture gets chosen, just that it does.