Conditionally eliminating one of two colliding bodies I like to explicit return function even for void functions to reduce the use of if-elses, like this code snippet:

private void processContactBetweenPlayers(Contact contact) {
    final Body bodyA = contact.getFixtureA().getBody();
    final Body bodyB = contact.getFixtureB().getBody();
    if (!isPlayer(bodyA) || !isPlayer(bodyB)) {
        return;
    }
    final float vx_a = Math.abs(bodyA.getLinearVelocity().x);
    final float vx_b = Math.abs(bodyB.getLinearVelocity().x);
    final float vy_a = Math.abs(bodyA.getLinearVelocity().y);
    final float vy_b = Math.abs(bodyB.getLinearVelocity().y);
    final float a_power = vx_a + vy_a;
    final float b_power = vx_b + vy_b;
    if ((a_power > b_power)) {
        if (save(bodyB)) return;
        markForDestroy(bodyB);
    } else {
        if (save(bodyA)) return;
        markForDestroy(bodyA);
    }
}

The function returns if both bodies are not players. But doing maybe this practice reduces the code readability. What do you think?

IMHO this is perfectly fine.

if (!isPlayer(bodyA) || !isPlayer(bodyB)) {
    return;
}

This is an early return, saving you one indentation level.

    if (save(bodyB)) return;
    markForDestroy(bodyB);

This is not as good. Here, I'd go for

    if (!save(bodyB)) markForDestroy(bodyB);

Note that braces should be used around single statements as well, i.e.,

    if (!save(bodyB)) {
        markForDestroy(bodyB);
    }

  But doing maybe this practice reduces the code readability.

No, used wisely it helps readability. When reading top down, you know that both bodies are players. In an if-block you know this too, but the indentation may get excessive. Moreover, using the early return makes clear that only players are dealt with.

As stated in the comments, processContact would be a much better name. A method called processContactBetweenPlayers could be extracted (not needed now as it's rather short). Such a method should use something like

checkArgument(isPlayer(bodyA) && isPlayer(bodyB));

in case it's public, otherwise I'd recommend

assert isPlayer(bodyA) && isPlayer(bodyB);

While asserts are often frowned upon, this is exactly what they're good for.

After answers and comments I refactoring my code spliting the code in several methods.

private void processContact(Contact contact) {
    final Body bodyA = contact.getFixtureA().getBody();
    final Body bodyB = contact.getFixtureB().getBody();
    if (areBothPlayers(bodyA, bodyB)) {
        processContactBetweenPlayers(contact, bodyA, bodyB);
    }
    // others contacts processing.
}

private void processContactBetweenPlayers(Contact contact, Body playerBodyA,     Body playerBodyB) {
    final float vx_a = Math.abs(playerBodyA.getLinearVelocity().x);
    final float vx_b = Math.abs(playerBodyB.getLinearVelocity().x);
    final float vy_a = Math.abs(playerBodyA.getLinearVelocity().y);
    final float vy_b = Math.abs(playerBodyB.getLinearVelocity().y);

    final float a_power = vx_a + vy_a;
    final float b_power = vx_b + vy_b;

    if (a_power > b_power) {
        if (!saved(playerBodyA)) {
            destroyIfNotSaved(playerBodyA);
        }
    } else {
        if (!saved(playerBodyB)) {
            destroyIfNotSaved(playerBodyB);
        }
    }
}

public void destroyIfNotSaved(Body playerBody) {
    if (!saved(playerBody)) {
        markForDestroy(playerBody);
    }
}

private boolean areBothPlayers(Body bodyA, Body bodyB) {
    return isPlayer(bodyA) && isPlayer(bodyB);
}


Select box to highlight choice deviation I have a select box that in default state looks as default (white, etc)
but when I select a different value, I want the entire table row to highlight in yellow.

I did this, but I want to make it better and more concise and more efficient.  Can you help?

Also I will have several such boxes, not just one.  Feel free to change/add/remove identifiers, I am looking for a better solution overall.

$(document).ready(function() {

  $("#closedRow").on('change', function() {
    if ($("#closedRow").val() != 0)
      $("#trRow").css('background-color', 'yellow');
    else
      $("#trRow").css('background-color', 'white');
  })

    $("#serviceRow").on('change', function() {
    if ($("#serviceRow").val() != 0)
      $("#trRow2").css('background-color', 'yellow');
    else
      $("#trRow2").css('background-color', 'white');

  })
})

    Closed:
    <select name="closed" id="closedRow">
            <option value="0">Show All</option>
            <option value="1">Hide Closed</option>
        </select>

    Service:
    <select name="service" id="serviceRow">
            <option value="0">Show All</option>
            <option value="1">Hide Service</option>
            <option value="2">Another Service</option>
        </select>

Maybe use a generic function to set the css on change if you don't have anything other than colors. It also helps with dry.

function evt_highlightRow() {
    row = $(this).parent().parent()
    if ($(this).val() != 0)
      row.css('background-color', 'yellow');
    else
      row.css('background-color', 'white');
}

Then use it like so:

$(some_target_nested_2_deep).on('change', evt_highlightRow);

// such as your select
$('select').on('change', evt_highlightRow);
// or a generic class applied to inputs in the same general location
$('.highlightRowTarget').on('change', evt_highlightRow);

I created the example here: https://codepen.io/anon/pen/bWraNB

$(document).ready(function() {

  $("#closedRow,#serviceRow").on('change', function() { // you can use multiple selector to factorize your code
  // here $(this) = $(idOfChangedElement)
  // $(idOfChangedElement).closest("tr") will select the parent tr of the changed element
 // then you can run with your code
   /* if ($(this).val() != 0)
      $(this).closest("tr").css('background-color', 'yellow');
    else
      $(this).closest("tr").css('background-color', 'white');
  })
*/
// or use css add toggle or add and remove class

  ($(this).val() != 0)? $(this).closest("tr").addClass("notShowed"):$(this).closest("tr").removeClass("notShowed");
})
});
.showed{
  background-color:white;
}
.notShowed{
  background-color:yellow;
}

    Closed:
    <select name="closed" id="closedRow">
            <option value="0">Show All</option>
            <option value="1">Hide Closed</option>
        </select>

    Service:
    <select name="service" id="serviceRow">
            <option value="0">Show All</option>
            <option value="1">Hide Service</option>
            <option value="2">Another Service</option>
        </select>


Optimizing simple C++ grading program I have started working on the projects listed here.  I want to practice my C++ coding skills.  I would love any constructive criticism I can get!

This is for the very first problem, #1 "Grading Program"

#include <iostream>
using std::cout;
using std::cin;

int main()
{
    int UserNumber;
    char Grade;

    cout << "Enter the grade you got on that test: ";
    cin >> UserNumber;
    cout << "\n\n\n";

    if (UserNumber > 89 && UserNumber < 101)
    {
        Grade = 'A';
    }

    else if (UserNumber > 79 && UserNumber < 91)
    {
        Grade = 'B';
    }

    else if (UserNumber > 69 && UserNumber < 81)
    {
        Grade = 'C';
    }

    else if (UserNumber > 59 && UserNumber < 71)
    {
        Grade = 'D';
    }

    else if (UserNumber > -1 && UserNumber < 61)
    {
        Grade = 'F';
    }
    else
    {
        Grade = 'Z';
    }

    switch (Grade)
    {
    case 'A':
        cout << "Perfect! You got a: " << Grade;
        break;
    case 'B':
        cout << "Could've been worse. You got a: " << Grade;
        break;
    case 'C':
        cout << "It's okay I guess, you got a: " << Grade;
        break;
    case 'D':
        cout << "Uggh.. You got a: " << Grade;
        break;
    case 'F':
        cout << "Better luck next time! You got a: " << Grade;
        break;
    case 'Z':
        cout << "Undefined value! Make sure it's a value from 0-100!";
        break;

    }
    cout << "\n\n\n\t\t\t";
    return 0;
}

There's probably much that can be done, but I'll give it a quick review:

Are only integer grades allowed?  They can be decimals in real life, and that would make for a more robust grading system.
Consider renaming UserNumber to something like numericalGrade.  The former sounds like it's for an ID number, not a grade.
Do not "list" variables at the top of a function in C++, unless it's absolutely necessary.  Here, you can declare UserNumber between the std::cout and std::cin.
There's no need for a Z case here.  Use a default case instead, which will be reached if no other case is reached.
You don't need an explicit return 0 at the end.  The compiler will do this return for you at the end of main() as successful termination is always implied here.

It seems to be poor code, so let's refactor it:

  #include <iostream>
  #include <string>

    char checkGrade(int num){
        char Grade = 'Z';

        if (num > 89 && num < 101) {
            Grade = 'A';
        }
        else if (num > 79 && num < 91) {
            Grade = 'B';
        }
        else if (num > 69 && num < 81) {
            Grade = 'C';
        }
        else if (num > 59 && num < 71) {
            Grade = 'D';
        }
        else if (num > -1 && num < 61) {
            Grade = 'F';
        }

        return Grade;
    };

    string printMessage(char Grade){
        std::string msg;

        switch (Grade)
        {
        case 'A':
            msg =  "Perfect! You got a: "  + Grade;
            break;
        case 'B':
            msg =  "Could've been worse. You got a: " + Grade;
            break;
        case 'C':
            msg =  "It's okay I guess, you got a: " + Grade;
            break;
        case 'D':
            msg =  "Uggh.. You got a: " + Grade;
            break;
        case 'F':
            msg =  "Better luck next time! You got a: " + Grade;
            break;
        case default:
            msg =  "Undefined value! Make sure it's a value from 0-100!";
            break;
        }

        return msg;
    };

    int main()
    {
        int numericalGrade;
        char Grade;

            cout << "Enter the grade you got on that test: ";
            cin >> numericalGrade;
            cout << "\n\n\n";

            // at least one line ...
            cout << printMessage( checkGrade(numericalGrade) );

            cout << "\n\n\n\t\t\t";
            return 0;
        }

You do this with design patterns, offering better reusability.


Calculate next working day I have tried to tackle this question as follows:

The program Calculates the next Working day, taking into account:
   - weekends (Saturdays and Sundays)
   - holidays happening in the middle of the week

param name="OrderDate" the date on which the order is placed
param name="workingDays" the number of workingdays to process the order
returns the date on which the Order is Requested to Ship

    public static DateTime getNextWorkingDay(DateTime OrderDate, int workingDays)
    {
        DateTime Holiday = new DateTime();
        List<DateTime> SelectedHolidays = new List<DateTime>();
        List<HolidayList> HolidayList = new List<HolidayList>();

        int calculatedWorkingDays=0;
        if (workingDays <= 5)
            calculatedWorkingDays = workingDays;
        else if (workingDays % 6 == 0)
        {
            calculatedWorkingDays = workingDays + workingDays / 6 * 2;
        }
        else
        {
            calculatedWorkingDays = workingDays + workingDays / 7 * 2;
        }

        HolidayList =  "Read the list from Json file";

        foreach (var holiday in HolidayList)
        {
            string year = DateTime.Today.Year.ToString();
            Holiday = DateTime.Parse(holiday.MonthAndDayValue + "-" + year);

            if (Holiday.Day == 1 && Holiday.Month == 1)
            {
                year = (DateTime.Today.Year + 1).ToString();
                Holiday = DateTime.Parse(holiday.MonthAndDayValue + "-" + year);
            }

            SelectedHolidays.Add(Holiday.Date);

            if (Holiday.DayOfWeek == DayOfWeek.Sunday || Holiday.DayOfWeek == DayOfWeek.Saturday)
            {
                //do nothing
            }
            else
            {
                if (Holiday.Date == OrderDate.Date)
                    OrderDate.AddDays(1);
                if (Holiday.Date > OrderDate.Date)
                {
                    int K = (int)(Holiday.Date - OrderDate.Date).TotalDays;
                    if (K <= calculatedWorkingDays)
                    {
                        calculatedWorkingDays++;
                    }
                }
            }
        }
        if (OrderDate.DayOfWeek == DayOfWeek.Sunday)
        {
            calculatedWorkingDays++;
        }
        else if (OrderDate.DayOfWeek == DayOfWeek.Saturday || OrderDate.DayOfWeek == DayOfWeek.Friday)
        {
            calculatedWorkingDays += 2;
        }

        // this is required date
        DateTime date_OrderRequestedToShip = OrderDate.AddDays(calculatedWorkingDays);

        //check if the last date fall on a non working day(holiday  or weekend)
        calculatedWorkingDays = 0;
        foreach (DateTime d in SelectedHolidays)
            if (d.Date == date_OrderRequestedToShip.Date)
                calculatedWorkingDays++;
        date_OrderRequestedToShip = date_OrderRequestedToShip.AddDays(calculatedWorkingDays);
        if (date_OrderRequestedToShip.DayOfWeek == DayOfWeek.Sunday)
        {
            calculatedWorkingDays = 1;
        }
        else if (date_OrderRequestedToShip.DayOfWeek == DayOfWeek.Saturday)
        {
            calculatedWorkingDays = 2;
        }

        return date_OrderRequestedToShip.AddDays(calculatedWorkingDays);
    }

The holiday list contains the month and day (MM/dd) part only with out the year part:

{
    "holidayName": "Memorial Day",
    "MonthAndDayValue": "05-30"
}

Can anyone help to tune up or suggest changes to make the code more efficient?

First, you should separate a process of parsing your input from your business logic. So, your first step is to convert your holidays input to a structure you'll be working with. So, let's say you have a List<Holiday> where

public struct Holiday
{
    public readonly int Month;
    public readonly int Day;

    public Holiday(int month, int day)
    {
        Month = month;
        Day = day;
    }
}

Now, in your place I'd start with implementation of something that enumerates working days like so:

static IEnumerable<DateTime> GetWorkingDays(DateTime startDate, List<Holiday> holidays)
{
    var date = startDate;

    for (;;date = date.AddDays(1))
    {
        if (date.DayOfWeek != DayOfWeek.Saturday &&
            date.DayOfWeek != DayOfWeek.Sunday &&
            holidays.All(holiday => holiday.Day != date.Day ||
                                    holiday.Month != date.Month))
        {
            yield return date;
        }
    }
}

Note that this iterator is endless, so you should be cautious enumerating it.

Now, your problem can be solved with just one line

var result = GetWorkingDays(orderDate, holidays).Skip(workingDays).First();

Complete example here: https://dotnetfiddle.net/W3dalr

Don't put the braces on the same line with the statement, ie:

if(...) {
}

do this instead:

if (...)
{
}

Why?
Putting the brace on the same line as the statement makes it harder to read the code and match the braces. Putting them on a separate improves your ability to read the code faster.
And every manager in the world want's you to code faster...


Find all possible sumation numbers to a specified sum This is a problem that I had in mind for a loong time and today I decided to give it a go, the basic idea is you enter a number/sum and the program outputs all the possible ways this sum can be formed let's say we have 3 as input the output will be (1, 1, 1),(1, 2) I don't think that the code is really efficient so any tips are appreciated.

    static void Main(string[] args)
    {
        int sum = int.Parse(Console.ReadLine());
        List<List<int>> subsets = GetSubsets(sum);
        for (int i = 0; i < subsets.Count; i++)
        {
            Console.WriteLine(string.Join(",", subsets[i]));
        }
        Console.ReadKey();
    }

    private static List<List<int>> GetSubsets(int sum)
    {
        List<List<int>> subsets = new List<List<int>>();
        int[] allNumbers = new int[sum - 1];
        List<int> baseSubset = PopulateWith(1, allNumbers.Length + 1);
        int baseNumberIndex = 0;
        int additiveNumberIndex = 1;
        for (int i = 0; i < allNumbers.Length; i++)
        {
            allNumbers[i] = i + 1;
        }
        if (sum > 1)
        {
            // if 1 is entered as a sum we dont want to return 1 as result because that's not a sum of numbers it's just 1.
            subsets.Add(baseSubset);
        }
        while (baseNumberIndex < allNumbers.Length && additiveNumberIndex < allNumbers.Length)
        {
            List<int> currentSubset = PopulateWith(allNumbers[baseNumberIndex], (int)Math.Round((double)sum / allNumbers[baseNumberIndex]));
            int currentSum = currentSubset.Sum();
            while (true)
            {
                if (currentSum + allNumbers[additiveNumberIndex] > sum)
                {
                    currentSubset.Remove(allNumbers[baseNumberIndex]);
                }
                else
                {
                    currentSubset.Add(allNumbers[additiveNumberIndex]);
                }
                currentSum = currentSubset.Sum();
                if (currentSum == sum && !subsets.Any(seq => seq.SequenceEqual(currentSubset)))
                {
                    subsets.Add(currentSubset.ToList());
                }
                if (currentSum + allNumbers[additiveNumberIndex] > sum && !currentSubset.Contains(allNumbers[baseNumberIndex]))
                {
                    break;
                }
            }
            additiveNumberIndex++;
            if (additiveNumberIndex == allNumbers.Length)
            {
                baseNumberIndex++;
                additiveNumberIndex = baseNumberIndex + 1;
            }
        }
        return subsets;
    }

    private static List<int> PopulateWith(int number, int size)
    {
        List<int> collection = new List<int>();
        for (int i = 0; i < size; i++)
        {
            collection.Add(number);
        }
        return collection;
    }

Rather than go through a line-by-line critique of your program, let me just say:

It is way too long and too complicated
You are getting hung up on all the adds and removes.  This problem is much easier to solve if you never add or remove anything. Treat sequences as immutable data structures.

You should be able to do this in about four lines of code in your method body, and some little helper methods.  Here's an example:

using System;
using System.Collections.Generic;
using System.Linq;
public class Program
{
    static IEnumerable<T> Singleton<T>(T t)
    {
        yield return t;
    }
    static IEnumerable<IEnumerable<int>> AllSums(int n, int min = 1)
    {
        for (int i = min; i <= n / 2; ++i)
            foreach(var seq in AllSums(n - i, i))
                yield return Singleton(i).Concat(seq);
        yield return Singleton(n);
    }
    public static void Main()
    {
        foreach(var result in AllSums(7))
            Console.WriteLine(string.Join(",", result));
    }
}

The key here is to make a clear specification for the AllSums method.  It takes positive integers n and min and returns a sequence of sequences that all have the following properties:

They sum to n
The smallest number in the sequence is not smaller than min
The sequence is non-decreasing

Once you have a method that has these properties, the recursion becomes much easier to reason about.

Go through this implementation very carefully and annotate each line of code with its meaning and purpose in the algorithm.

For another application of this general principle of generating a sequence of data structures that all have a sum property, see my series of articles which begins here:

https://blogs.msdn.microsoft.com/ericlippert/2010/04/19/every-binary-tree-there-is/

Recursively breaks the number down into pairs.

//test
foreach (List<int> li in BreakMeDown(7).Distinct())
    Debug.WriteLine(string.Join(", ", li));
//end test

This is getting close.  It produces all the answers but it produces duplicates.

    public static IEnumerable<List<int>> BreakMeDown5(int n)
    {
        for (int i = 1, j = n - 1; i <= j; i++, j--)
        {
            List<int> breakMeDown = new List<int>();
            breakMeDown.Add(i);
            breakMeDown.Add(j);
            yield return breakMeDown;

            foreach (List<int> li in BreakMeDown5(i))
                yield return breakMeDown.Skip(1)
                                        .Concat(li)
                                        .ToList();

            if (i != j)
            {
                foreach (List<int> li in BreakMeDown5(j))
                    yield return breakMeDown.Take(1)
                                            .Concat(li)
                                            .ToList();
            }
        }
    }


Reverse complement of a DNA string I have a function in python where I want the reverse complement of a DNA string.

This is what I have now:

def inversComplement(input):
    output = ''
    for letter in input:
        letter = letter.upper()

        if letter == 'A':
            output += 'T'
        elif letter == 'T':
            output += 'A'
        elif letter == 'G':
            output += 'C'
        else:
            output += 'G'

    return(output[::-1])

I was thinking of using the string.replace() method. But the problem was that if I use this the T-> A would be changed afterwards with the A -> T replacement

    string.replace('A','T').replace('T','A').replace('G','C').replace('C','G')

You can do this with str.translate, which was changed slightly in Python  3. All you need to do is pass it a translation table, and it'll do what your above code is doing. However to build the translation table is hard in Python 2 and requires the helper function string.maketrans, which was changed to str.maketrans in Python 3.

But here is how you'd use it in Python 2:

>>> import string
>>> trans = string.maketrans('ATGC', 'TACG')
>>> 'ATTAGCC'.translate(trans)
'TAATCGG'

And here is how you can use it in Python 3:

>>> trans = str.maketrans('ATGC', 'TACG')
>>> 'ATTAGCC'.translate(trans)
'TAATCGG'
>>> trans = {ord('A'): 'T', ord('T'): 'A', ord('G'): 'C', ord('C'): 'G'}
>>> 'ATTAGCC'.translate(trans)
'TAATCGG'

You can re-implement this yourself, but the above is written in C, and so should out perform any Python code.

I have found a solution with the string.replace() method!

def inversComplement(input):
    return(input.upper().replace('A', 'temp').replace('T', 'A').replace('temp', 'T').replace('G', 'temp').replace('C','G').replace('temp','C')[::-1])


Generating arithmetic quiz questions I have created a quiz in Python that will generate a series of 10 questions to test the basic arithmetic skills of primary school children. To do this, I have used a while loop and random functions to generate random numbers and operations.

name = input("What is your name? ")
print("Hello, "+name+". You will be completing a quiz that will ask you 10 questions which will test you on adding, subtracting and multiplying two numbers together. Try your best at each question and good luck!")

import random
from operator import add, sub, mul

count = 0
score = 0
while count <= 9:
    ops = (add, sub, mul)
    op = random.choice(ops)
    x = random.randint(1,10)
    y = random.randint(1,10)

    if op == add:
        print("What is", x, "+",y, "? ")
        question_add = int(input())
        answer_add = op(x,y)
        if question_add == answer_add:
            print("Well done, this is correct!")
            score = score + 1
            count = count + 1
        else:
            print("Sorry, but this is incorrect.")
            count = count + 1

    elif op == sub:
        print("What is", x, "-", y, "? ")
        question_sub = int(input())
        answer_sub = op(x,y)
        if question_sub == answer_sub:
            print("Well done, this is correct!")
            score = score + 1
            count = count + 1
        else:
            print("Sorry, but this is incorrect.")
            count = count + 1

    elif op == mul:
        print("What is", x, "x", y, "? ")
        question_mul = int(input())
        answer_mul = op(x,y)
        if question_mul == answer_mul:
            print("Well done, this is correct!")
            score = score + 1
            count = count + 1
        else:
            print("Sorry, but this is incorrect.")
            count = count + 1

    if count == 10:
        print("Well done "+name+"! You have completed the quiz. Your final score out of 10 is "+str(score)+".")

I have done it, however, I do not think that this is the most efficient way to write this program. Can this program be condensed down further into fewer lines of code?

Looping

You want to perform an action 10 times. The loop counter never matters for your code, you just need to ask 10 questions. The way you wrote the code, you have:

count = count + 1

in 6 places, all of which are necessary, otherwise you would loop too many times. That is very error prone. If you added support for division, you would have to write it two more times. Good rule of thumb is never repeat yourself!

The best way to loop 10 times in Python, when you don't care about the loop count, is:

for _ in range(10):
    # stuff

No need to worry about incrementing count. Let range() do it for you.

Don't Repeat Yourself

The code for adding, subtracting, and multiplying is the same. The only difference is what operation you do and what operation you print. You mostly already have that refactored. You do use op() to perform the operation. So let's just go one step further:

ops = (('+', add), ('-', sub), ('*',mul))
op_name, op = random.choice(ops)

And then, I'm pretty much copying your logic for add with just one real change (and renaming some variables):

print("What is", x, op_name ,y, "? ")  # op_name, not +
actual = int(input())
expected = op(x,y)
if actual == expected:
    print("Well done, this is correct!")
    score += 1
else:
    print("Sorry, but this is incorrect.")

I also renamed your variables to actual and expected, instead of question and answer - since question didn't really refer to a question. Also += 1 is a more concise way of expressing incrementing by one.

Tell the user the answer?

If the user gets it wrong, do you want to tell them the correct answer?

Finishing up

You don't need to check count (or, now, _) to know when you're done. Just wait for the loop to end, at which point you can print the score. So the flow looks like:

score = 0
for _ in range(10):
    # prompt a problem and potentially increment score

print("Well done "+name+"! You have completed the quiz. Your final score out of 10 is "+str(score)+".")

Though typically you'd express the score as X/10, not out of 10 is X. That just reads funny.

You can use a dictionary for your operations and a for-loop for count:

import random
from operator import add, sub, mul

name = input("What is your name? ")
print("Hello, {}. You will be completing a quiz that will ask you 10 questions which will test you on adding, subtracting and multiplying two numbers together. Try your best at each question and good luck!".format(name))

score = 0
for count in range(10):
    ops = {'+': add, '-': sub, 'x': mul}
    op = random.choice(ops.keys())
    x = random.randint(1,10)
    y = random.randint(1,10)

    print("What is {} {} {}? ".format(x, op, y))
    question = int(input())
    answer = ops[op](x,y)
    if question == answer:
        print("Well done, this is correct!")
        score += 1
    else:
        print("Sorry, but this is incorrect.")

print("Well done {}! You have completed the quiz. Your final score out of 10 is {}.".format(name, score))


Design watchdog, reconnecting to server I have console application that sends and process XML data. I have created a fault mechanism that the connection drops. I go in a "fault" mode. And try to reconnect to my server. I'm sending keepalives and so is the server. When I'm not receiving any keepalives and any others messages in a timespan of 2 minutes the programs enters a fault mode. Is this the correct method of doing such operations?

This is my watchdog class (reduced code).

internal class Watchdog
{
    private Timer _timer = new Timer(1000);
    public bool Status
    {
        get { return _status; }
        set
        {
            _status = value;
            OnStatusChanged(new CustomEventArgs(value));
        }
    }

    protected virtual void OnStatusChanged(CustomEventArgs e)
    {
        var handler = StatusChanged;
        if (handler != null) handler(this, e);
    }

    public void Stop()
    {
        _timer.Enabled = false;
    }

    public void SetStatus()
    {
        _latestAction = DateTime.Now;
    }

    public void SetManually()
    {
        _latestAction = DateTime.Now.AddDays(-1); //fool the timer
        Status = true;
    }

    void _timer_Elapsed(object sender, ElapsedEventArgs e)
    {
        if ((DateTime.Now - _latestAction) > TimeSpan.FromSeconds(22))
        {
            Status = true;
        }
        if ((DateTime.Now - _latestAction) < TimeSpan.FromSeconds(22))
        {
            Status = false;
        }
    }
}

This is other class where I call the handler. And try to reconnect.

    void _watchdog_StatusChanged(object sender, CustomEventArgs e)
    {
        if (e.Status) //no message has been processed in two minutes or nothing can be sent, something can be wrong
        {
            _watchdog.Stop(); //stop timer otherwise this method has not enough time to execute and will be recalled
            Console.WriteLine(DateTime.Now + " Error mode initiated!");
            if (!Connection.Connected)
            {
                ((IDisposable) Connection).Dispose();
                openConnection(); //try to reopen conn
                if (Connection.Connected)
                {
                    initialize(); //init all settings
                    _watchdog.SetStatus(); //set live tick
                }
            }
            _watchdog.Start();
        }
        else //fault has been restored.
        {
            Console.WriteLine(DateTime.Now + " Error has been resolved!");
        }
    }

And the event handlers where I receive messages I set a live tick.

    private void _tcpReader_AlarmResponseComplete(object sender, CustomEventArgs e)
    {
        Console.WriteLine("alarmresponse received");
        _processAlarmResponsesToDatabase.Add(e.AlarmResponse);
        _watchdog.SetStatus();
    }

And where I try to send a message

    private void send(string msg) //TODO: must be set to private is now so for testing purpose public
    {
        if (Connection.Connected)
        {
            if (!_tcpReader.IsStopped)
            {
                _writer.Write(msg);
                _writer.Flush();
            }
            else
                Console.Error.WriteLine(
                    "Unable to send message to the server, a request has been made to terminate the connection.");
            _watchdog.SetStatus(); //give live tick to watchdog
        }
        else
        {
            Console.Error.WriteLine("Unable to send message, not connected to the server.");
            _watchdog.Stop(); //the next statement generates an event and the timer would generate the same events wich causes to many events
            _watchdog.SetManually(); //set manually that connection has been terminated
        }
    }

I'll express my thoughts step by step reading your code (just a note: you should post a complete compilable code, at least of your main class Watchdog, if you take out what you think is not relevant then it is much harder to understand what you're doing).

  internal class Watchdog : IDisposable

internal modifier (unless Watchdog is a nested class) is superfluous. Moreover if you're not supposed to derive any class from Watchdog I'd declare it as sealed (note that it's an internal class then you can simply remove it when you will need to derive, there is not any public interface to respect).

sealed class Watchdog : IDisposable

IDisposable interface usually comes together with a common pattern implementation, I see no reason to avoid that (even in simple cases):

~Watchdog() {
    Dispose(false);
}

public void Dispose() {
    Dispose(true);
    GC.SuppressFinalize(this);
}

private void Dispose(bool disposing) {
    try {
        if (disposing)
            _timer.Elapsed -= _timer_Elapsed;
    }
    finally {
        _timer.Dispose();
    }
}

  private Timer _timer = new Timer(1000);

_timer is initialized only here (or, better, in constructor) then it should be market as readonly:

private const int TimerTickInMilliseconds = 1000;
private readonly Timer _timer;

public Watchdog() {
    _timer = new Timer(TimerTickInMilliseconds);
    _timer.Elapsed += _timer_Elapsed;
}

  public bool Status { ... }

In your setter you do not check if value changed, if you repeatedly assign true to Status then you'll fire StatusChanged each time.

public bool Status {
    get {
        return _status;
    }
    set {
        if (_status != value) {
            _status = value;
            OnStatusChanged(EventArgs.Empty);
        }
    }
}

That said Status name is pretty misleading. What does it mean? Does it indicate whether timer is enabled? It actually means watchdog is running and after a fixed amount of time watched operation didn't complete. Make it clear, change its name to, for example, Hanged.

public bool Hanged { ... }

  public void SetStatus() { ... }

Your methods have too vague name, as for Status property it should be clear what they do. I'd suggest to rename them:

public void Stop() { ... }
public void Start() { ... }

SetManually() is pretty weird (IMO) but I didn't read caller code (so far) so I'd temporary...drop it. Externally they may simply set Hanged to true.

  void _timer_Elapsed(object sender, ElapsedEventArgs e) { ... }

You're performing this check:

if (a > b)
    doThis();
if (a < b)
    doThat();

It may be simplified to (note that we're also handling a == b case).

if (a > b)
    doThis();
else
    doThat();

You're also setting Status (now Hanged) to false but you don't actually need it, its value must be set in Start() method. I'd also rename this method to something slightly more meaningful and move out 22 seconds magic number:

public void Start() {
    _timer.Enabled = true;
    Hanged = false;
}

private void OnTimerTick(object sender, ElapsedEventArgs e) {
    if (DateTime.Now - _watchedActivityStartTime > Timeout)
        Hanged = true;
}

Few notes:

Timeout may be private readonly TimeSpan Timeout = TimeSpan.FromSeconds(22) or a public accessible property.
You do not need to set Hanged to false (see previous paragraph).
I renamed _latestAction to _watchedActivityStartTime trying to make clear what it contains.
You may consider (it depends which kind of watchdog you want to implement) to also stop the timer when you detected that task hanged (just add a call to Stop()).
If you need to also detect if operation recover (after signaling it was hanged) then you should add this logic in this method (checking Hanged status and adding another DateTime _lastHangOccurence and eventually adding a Reset() method to update _watchedActivityStartTime).

Note that you do not need to perform this check each second, timer can directly be initialized to = new Timer((int)Timeout.TotalMilliseconds) and stopped after hang has been detected (no need to check if (DateTime.Now - _watchedActivityStartTime > Timeout)):

public Watchdog() {
    _timer = new Timer((int)Timeout.TotalMilliseconds);
    _timer.Elapsed += TimerOnElapsed;
}

private void OnTimerTick(object sender, ElapsedEventArgs e) {
    Hanged = true;
    Stop();
}

One final minor note: you should consider to group your methods (somehow), you're now mixing public/protected/private methods/variables in apparently random order.

I skip review of calling code because there isn't (really!) enough context to suggest anything but basic design notes.

All these said I'd consider to change your overall design. As it is now you have an utility class (Watchdog) that is nothing more than a timer. All the logic is spanned between this and callers. A better (IMO) design should centralize all this logic. Just a proof of concept (I omit not relevant code) of one way to do it:

sealed class Watchdog : IDisposable {
    public Watchdog(Action action) {
    }

    public void Start() {
    }

    public event EventHandler Hanged;
}

Used like this:

var watchdog = new Watchdog(TryConnect);
watchdog.Hanged += OnConnectionHanged;
watchdog.Start();

I said one way because (according to your usage pattern) you may have completely different interfaces (you may, for example, have a Start() method that accepts two delegates). The main point is: remove all watchdog logic from callers, what they have to provide are just two actions to call:

One action to execute, this is the watched task.
One action to call when watched task hanged.

Everything else must stay inside your Watchdog class.

Your logic seems really odd. I would start out with my watchdog class like this:

When you rewrite watch dog to this it becomes so close to Timer that you are probably better off just using a timer and just remove the watchdog class completely.

internal class WatchDog : IDisposable
{
    private readonly Timer _timer;

    public WatchDog( TimeSpan timespan )
    {
      _timer = new Timer( timespan.TotalMilliseconds );
      _timer.Elapsed += TimerOnElapsed;
      _timer.Start();
    }

    public void Reset()
    {
      _timer.Stop();
      _timer.Start();
      IsRunning = true;
    }

    private void TimerOnElapsed( object sender, ElapsedEventArgs elapsedEventArgs )
    {
      TimedOut();
      IsRunning = false;
    }

    public bool IsRunning { get; set; }

    public event Action TimedOut = delegate { };

    public void Dispose()
    {
      _timer.Elapsed -= TimerOnElapsed;
      _timer.Dispose();
    }
}


Fill an array of objects with some default object I have an array of objects. Whatever may be the size of this array, I need to ensure the size of the array as n using a default object.

 function fillRemaining(arr, n) {
        var defaultObject = {'id' : 0, 'name' : ''};
        var temp = 0;
        while(arr.length + temp < n ) {
           var template = $.extend({}, defaultObject);
           template.id = new Date().getTime();
           arr.push(defaultObject);
           temp++;
        }
    }

Is there a better method of doing it?

I see a few ways this can be improved.

First, you are creating the template variable as a copy of the defaultObject, but then you are inserting defaultObject rather than template. I am going to assume this is a bug.

I would also say that there is not a big reason why you should have a defaultObject, then create and modify a copy to add. Just create objects on the fly.

Second, you are using a while loop, but constructing it like a for loop (loop variable, incremented every iteration). You actually don't even use the loop variable, so it can be removed completely. There is also a second potential bug where you are checking against arr.length + temp in your conditional, but arr.length increases every time you add an element to the array, so you are counting by 2's instead of by 1's.

Lastly, the name fillRemaining does not clearly describe what the function is supposed to do. Since it is padding onto the end of the array, perhaps padArrayWithDefault or something similar might be more descriptive.

Here's what I would suggest:

function padArrayWithDefault(arr, n) {
    while(arr.length < n) {
        arr.push({
            "id": new Date().getTime(),
            "name": ""
        });
    }
}

Im not sure if this is the proper forum for this question. But if I understand correctly, you're trying to create an array that is prefilled with a default object.

If this is the case:

You could simplify by calling the Array constructor with n and then just map over it.

function Fill(n, _default) {
   return Array.apply(null,Array(n)).map(function (val) {
     return _default || { };
   });
}


Improvements to data access layer public class EmployeeDBHandler
    {
        public List<Employee> GetEmployees()
        {
            List<Employee> empList = new List<Employee>();
            string sqlConnstr = Utils.GetDBConnection();
            SqlConnection sqlConn = new SqlConnection(sqlConnstr);
            SqlCommand sqlCmd = new SqlCommand("GetAllEmployee", sqlConn);
            sqlCmd.CommandType = CommandType.StoredProcedure;
            sqlConn.Open();
            using (SqlDataReader reader = sqlCmd.ExecuteReader())
            {
                while (reader.Read())
                {
                    var emp = new Employee()
                    {
                        EId = (int)(reader["EmpID"]),
                        FirstName = Convert.ToString(reader["FirstName"]),
                        LastName = Convert.ToString(reader["LastName"])
                    };
                    empList.Add(emp);
                }
            }
            return empList;
        }
        public List<Employee> FetchEmployee(int empid)
        {
            List<Employee> empList = new List<Employee>();
            string sqlConnstr = Utils.GetDBConnection();
            SqlConnection sqlConn = new SqlConnection(sqlConnstr);
            SqlCommand sqlCmd = new SqlCommand("usp_FetchEmployeeDetails", sqlConn);
            sqlCmd.CommandType = CommandType.StoredProcedure;
            sqlCmd.Parameters.AddWithValue("@EmpID", empid);
            sqlConn.Open();
            using (SqlDataReader reader = sqlCmd.ExecuteReader())
            {
                while (reader.Read())
                {
                    var emp = new Employee()
                    {
                        EId = (int)(reader["EmpID"]),
                        FirstName = (reader["FirstName"].ToString()),
                        LastName = (reader["LastName"].ToString())
                    };
                    empList.Add(emp);
                }
            }
            return empList;
        }
        public void AddEmployeeInfo(string firstname, string lastname)
        {
            string sqlConnstr = Utils.GetDBConnection();
            SqlConnection sqlConn = new SqlConnection(sqlConnstr);
            SqlCommand sqlCmd = new SqlCommand("AddEmployee", sqlConn);
            sqlCmd.CommandType = CommandType.StoredProcedure;
            sqlCmd.Parameters.AddWithValue("@FirstName", firstname);
            sqlCmd.Parameters.AddWithValue("@LastName", lastname);
            sqlConn.Open();
            sqlCmd.ExecuteNonQuery();
            sqlConn.Close();
        }
        public void UpdateEmployee(int empid,string firstname, string lastname)
        {
            string sqlConnstr = Utils.GetDBConnection();
            SqlConnection sqlConn = new SqlConnection(sqlConnstr);
            SqlCommand sqlCmd = new SqlCommand("usp_UpdateEmployeeDetails", sqlConn);
            sqlCmd.CommandType = CommandType.StoredProcedure;
            sqlCmd.Parameters.AddWithValue("@EmpID", empid);
            sqlCmd.Parameters.AddWithValue("@FirstName", firstname);
            sqlCmd.Parameters.AddWithValue("@LastName", lastname);
            sqlConn.Open();
            sqlCmd.ExecuteNonQuery();
            sqlConn.Close();
        }
        public void DeleteEmployee(int empid)
        {
            string sqlConnstr = Utils.GetDBConnection();
            SqlConnection sqlConn = new SqlConnection(sqlConnstr);
            SqlCommand sqlCmd = new SqlCommand("usp_DeleteEmployeeDetails", sqlConn);
            sqlCmd.CommandType = CommandType.StoredProcedure;
            sqlCmd.Parameters.AddWithValue("@EmpID",empid);
            sqlConn.Open();
            sqlCmd.ExecuteNonQuery();
            sqlConn.Close();
        }

    }

I'm try to build a 3-tier arch. but I feel these code can be improvised so please review my above Data Access Layer code and give suggestions and corrections, if any, to improvise my code.

something along these lines

public static class DTExtensions
{
    public static List<T> ToList<T>(this System.Data.DataTable dt) where T : new()
    {
        var obj = dt.Rows.OfType<System.Data.DataRow>().Select(dr => dr.ToObject<T>()).ToList();
        return obj;
    }
    public static T ToObject<T>(this System.Data.DataRow dataRow) where T : new()
    {
        T item = new T();
        var itemType = item.GetType();
        foreach (System.Data.DataColumn column in dataRow.Table.Columns)
        {
            System.Reflection.PropertyInfo property = itemType.GetProperty(column.ColumnName);

            if (property != null && dataRow[column] != DBNull.Value)
            {
                var result = Convert.ChangeType(dataRow[column], property.PropertyType);
                property.SetValue(item, result, null);
            }
        }

        return item;
    }
}

public class DataHandler
{
    private readonly string _ConnectionInfo = String.Empty;
    public DataHandler(string connectioninfo)
    {
        _ConnectionInfo = connectioninfo;
    }

    public async Task<System.Data.DataTable> DataAsync( string commandSQL, IEnumerable<System.Data.SqlClient.SqlParameter> listParameter=null)
    {
        System.Data.DataTable dt = new System.Data.DataTable("data");

        using (System.Data.SqlClient.SqlConnection connection = new System.Data.SqlClient.SqlConnection(_ConnectionInfo))
        {
            try
            {
                await connection.OpenAsync().ConfigureAwait(false);
            }
            catch (InvalidOperationException ex)
            {
                return null;
            }
            catch (System.Data.SqlClient.SqlException ex)
            {
                return null;
            }
            using (System.Data.SqlClient.SqlCommand command = new System.Data.SqlClient.SqlCommand(commandSQL, connection))
            {
                command.CommandType = System.Data.CommandType.StoredProcedure;
                if (listParameter != null && listParameter.Any())
                {
                    command.Parameters.AddRange(listParameter.Where(p => p != null).ToArray());
                }
                using (var r = await command.ExecuteReaderAsync(System.Data.CommandBehavior.SequentialAccess).ConfigureAwait(false))
                {
                    dt.Load(r);
                }
                command.Parameters.Clear();
            }
            connection.Close();
        }

        return dt;
    }
}

public sealed class Employee
{
    public string EmpID { get; set; }
    public string FirstName { get; set; }
    public string LastName { get; set; }
}

public class EmployeeDBHandler
{
    private readonly DataHandler _DataHandler;
    public EmployeeDBHandler(DataHandler datahandler)
    {
        _DataHandler = datahandler ?? throw new System.ArgumentNullException("datahandler");
    }

    public async Task<List<Employee>> GetEmployees()
    {
        System.Data.DataTable dt = await _DataHandler.DataAsync("GetAllEmployee").ConfigureAwait(false);
        if (dt != null)
        {
            //OR look into using yeild
            //yield return employee
            //OR
            var list = dt.ToList<Employee>();
            return list;
        }
        return null;
    }

    public async Task<List<Employee>> FetchEmployee(int empid)
    {
        List<Employee> empList = new List<Employee>();
        var parameters = new List<System.Data.SqlClient.SqlParameter> { new SqlParameter("@EmpID", empid) };
        System.Data.DataTable dt = await _DataHandler.DataAsync("usp_FetchEmployeeDetails", parameters).ConfigureAwait(false);
        if (dt != null && dt.Rows.Count>0)
        {
            var list = dt.ToList<Employee>();
            return list;
        }
        return null;
    }
}

//  main program:
public class MainProgram
{
    public void DoSomeWork()
    {
        string sqlConnstr = @"Data Source=<...>; Initial Catalog=<...>; User ID=username; Password=password"; //Utils.GetDBConnection();
        var dbHandler = new DataHandler(sqlConnstr);
        var employeeHandler = new EmployeeDBHandler(dbHandler);

        var employees = employeeHandler.GetEmployees();
        if (employees != null)
        {
            //.....
        }
    }
}


Implementing a set using a linked list My task is to add the specified element to the linked list if it is not already present

/**
* Adds the specified element to this set if it is not already present.
* @param o element to be added to this set.
* @return  true if this set did not already contain the specified element.
* @throws NullPointerException - if the specified element is null and this set  does
  not support null elements
 */

This is my solution

public boolean add(E o)  {

    if (o == null)
        throw new NullPointerException();

    ListNode newNode = new ListNode(o, null);
    if (contains(o))
    {
     return false;
    }
    else {

    head.next=newNode;
    head=newNode;
    return true;
   }
}

Can please anybody check it and correct if there are mistakes, or give hints to correct it?

Let's say that you are starting with a list containing five elements:

$$
\newcommand{ptr}[1]{\overset{\mathtt{#1}}{\longrightarrow}}
\mathtt{head} \ptr{} \fbox{first}  \ptr{next}
                     \fbox{second} \ptr{next}
                     \fbox{rest}   \ptr{next}
                     \fbox{of}     \ptr{next}
                     \fbox{list}   \ptr{next} \mathtt{null}
$$

Then, you add a non-duplicate item called "another".  After executing head.next=newNode:

$$
% "align" allows us to do vertical alignment
% "\\" separates lines
% "&" is an alignment marker. The markers in different rows
% are positioned above each other, and horizontally centered.
\newcommand{ptr}[1]{\overset{\mathtt{#1}}{\longrightarrow}}
\begin{align*}
                                             &\mathtt{newNode} \\
                                             &\quad\downarrow  \\
\mathtt{head} \ptr{} \fbox{first} \ptr{next} &\fbox{another} \ptr{next} \mathtt{null} \\
                                             &\fbox{second}  \ptr{next}
                                              \fbox{rest}    \ptr{next}
                                              \fbox{of}      \ptr{next}
                                              \fbox{list}    \ptr{next}
                                              \mathtt{null}
\end{align*}
$$

You've just orphaned and discarded everything starting from the second element.  Then it gets worse when you set head=newNode.

The "else" part of the if-else is redundant, since the if contains a return statement.

Also what happens when you add the first element? If head is null, the head.next=newNode line will give a NullPointerException.


Simple foreach adding to a list I am having a simple foreach where I add objects to a list:

    List<SoonestDrawDateModel> soonestDrawDateModel = new List<SoonestDrawDateModel>();

    foreach (var item in drawDates)
    {
        SoonestDrawDateModel sdModel = new SoonestDrawDateModel();
        sdModel.DrawDay = item;
        sdModel.DrawDayId = item.DayId;
        sdModel.CutOffDayId = item.CutOffDayId;

        soonestDrawDateModel.Add(sdModel);

        sdModel = new SoonestDrawDateModel();
        sdModel.DrawDayId = item.DayId + 7;
        sdModel.CutOffDayId = item.CutOffDayId;
        sdModel.DrawDay = item;
        soonestDrawDateModel.Add(sdModel);
    }

    var realDrawDates = soonestDrawDateModel.OrderBy(x => x.DrawDayId);

Is there any way to do this with Linq with fewer lines?

What you're doing here, essentially, is creating two Model items for each item in your source list, and storing them in a list. Here's a somewhat contrived usage of the SelectMany LINQ function which flattens a hierarchical list:

drawDates
  .Select(dd => new SoonestDrawDateModel[2]  // create a 2-item array for each item.
                 {
                   new SoonestDrawDateModel
                    {
                      DrawDay = item,
                      DrawDayId = item.drawDayId,
                      CutOffDayId = item.CutOffDayId
                    },
                   new SoonestDrawDateModel
                    {
                      DrawDay = item,
                      DrawDayId = item.drawDayId + 7,
                      CutOffDayId = item.CutOffDayId
                    }
                }) // end of Select method.
 .SelectMany(items => items) // flatten collection of arrays into one collection.
 .OrderBy(x => x.DrawDayId); // order by.
 .ToList() //convert to a List<SoonestDrawDateModel>.

What we're doing here is creating a 2-item array or each Draw item, meaning we have an IEnumerable<SoonestDrawDateModel[]>, then use SelectMany to take all the members of each SoonestDrawDateModel[] and flatten them into one big IEnuemrable<SoonestDrawDateModel>, which we then sort and return.

You can simplify the syntax by extracting the main creation block into a method:

private SoonestDrawDateModel[] CreateDateModelPair(DrawItem item)
{
    return new SoonestDrawDateModel[2]
    {
     new SoonestDrawDateModel
     {
        DrawDay = item,
        DrawDayId = item.drawDayId,
        CutOffDayId = item.CutOffDayId
     },
     new SoonestDrawDateModel
     {
        DrawDay = item,
        DrawDayId = item.drawDayId + 7,
        CutOffDayId = item.CutOffDayId
     }
  }
}

then call it with this LINQ call that conveys the intent rather well:

drawDates
  .Select(CreateDateModelPair)
  .SelectMany (dateModels => dateModels)
  .OrderBy (dateModel => dateModel.DrawDayId)
  .ToList();

Or, as @anaximander suggests, an even terser (but less explicit) version that combines the Select and SelectMany calls. It saves a step, but it's less clear that we have two things going on - one to convert the DrawDate to two SoonestDrawDateModels, and another to flatten that list of lists.

drawDates
  .SelectMany (CreateDateModelPair)
  .OrderBy (dateModel => dateModel.DrawDayId)
  .ToList();

Repeated Code

I can see the repeated code, which can be removed by introducing following method:

public static void AddToSoonestDrawDateModel(string day, int dayId, int cutOffDayId)
{
    var sdModel = new SoonestDrawDateModel
    {
        DrawDay = day,
        DrawDayId = dayId,
        CutOffDayId = cutOffDayId
    };

    soonestDrawDateModel.Add(sdModel);
}

After refactoring code should be like this:

var soonestDrawDateModel = new List<SoonestDrawDateModel>();

foreach (var item in drawDates)
{
    AddToSoonestDrawDateModel(item.Day, item.DayId, item.CutOffDayId);
}

var realDrawDates = soonestDrawDateModel.OrderBy(x => x.DrawDayId);

Obviously, it is not LINQ, but it is more neat, readable and less repetitive code.


Polling a email using async / await I've created a create a console app that would:

Call a method to check an email account (I've done this step)
Convert the attachment to pdf (I've done this step)
Then once the conversion is complete wait 30 seconds
Repeat the previous 3 steps continuously

I've done Step 1 and 2 int the ProcessMailMessages() method.
The following code works but I want to know if I am on the right track or is there a better way to poll a email client?

    private static int secondsToWait = 30 * 1000;

    static void Main(string[] args)
    {
        bool run = true;
        do
        {
            try
            {
                Task theTask = ProcessEmailTaskAsync();
                theTask.Wait();
            }
            catch (Exception e)
            {
                Debug.WriteLine("
Error in Client

Exception

" + e.Message + "

" + e.StackTrace + "
 ");
            }
            GC.Collect();

        } while (run);

    }

    static async Task ProcessEmailTaskAsync()
    {
        var result = await EmailTaskAsync();
    }

    static async Task<int> EmailTaskAsync()
    {
        await ProcessMailMessages();
        await Task.Delay(secondsToWait);
        return 1;
    }

    static async Task ProcessMailMessages()
    {
        ...............................................................................
    }

I would rather do smth. like that (I added CancellationToken in case you would want to be able to cancel email parsing while it's in progress):

private static TimeSpan timeToWait = TimeSpan.FromSeconds(30);

//somewhere in your code:
_cancellationTokenSource = new CancellationTokenSource();

private static void Main(string[] args)
{
    CancellationToken token = _cancellationTokenSource.Token;
    do
    {
        try
        {
            ProcessEmailsAndWaitAsync(token).Wait();
        }
        catch (Exception e)
        {
            Debug.WriteLine("
Error in Client

Exception

" + e.Message + "

" + e.StackTrace + "
 ");
        }
    } while (!token.IsCancellationRequested);
}

private static async Task ProcessEmailsAndWaitAsync(CancellationToken token)
{
    await ProcessMailMessages(token);
    await Task.Delay(timeToWait, token);
}

private static async Task ProcessMailMessages(CancellationToken token)
{
    //...............................................................................
}

I believe you could use a much more elegant solution using the ContineWith method on the Task object

Here is some basic pseudo code...

function RecursiveEmailPollRoutine()
{
    var PollEmailTask = new Task(() => {
                                       //Poll the email
                                       //Pause for your delay
                                   }).ContinueWith(()=> {
                                                            //check your cancel var
                                                            //and cancel if need be
                                                            //otherwise recursively
                                                            //call this function
                                                        }).Start();
)

This hasn't been tested but you get the gist.  I hope this helps.


Building a number pyramid I have been working a number pyramid program. This is a question from Y. Daniel Laing's Introduction to programming using Python (which is based on Python 3.2). The question is from chapter 5, and it is number 5.19 in programming exercises portion of the chapter.  The question is stated as follows:

  Write a program that prompts the user to enter an integer from 1 to 15
  and displays it as the following sample run:

I don't have the image, but it is something like this:

  Enter the number of lines: 3

                          1
                       2  1  2
                    3  2  1  2  3

The solution I have come up with is this:

p = eval(input("Enter the number of lines: "))

num = 0
y = 1

while num < p:
    y = num

    line = str(y+1) + "  "

while y >= 1:

    y -= 1
    line += str(y+1) + "  "

while  y < num:

    line += str(y + 2) + "  "

    y +=1

print(format(line, "^80s"))

num +=1

If anyone has a better solution that would be simpler, please let me know.

Like 200_success said, the key is in thinking at the higher level. But I think his approach can be simplified.

Lets first define a row function, which simply calculates a list of numbers in an n'th row of our output:

# When n=3, returns [3, 2, 1, 2, 3]
def row(n):
    return list(reversed(range(2, n+1))) + list(range(1, n+1))

And now we can use this to print out the lines:

for n in range(1, 5):
    print(row(n))

Giving us the following output:

[1]
[2, 1, 2]
[3, 2, 1, 2, 3]
[4, 3, 2, 1, 2, 3, 4]

The core of the problem is now solved. The only thing left is formatting. For this we can write another function, which formats each number in list into a three-character box (with "%3s"), and also appends a certain amount of padding to make sure lines are aligned like a pyramid:

def formatted_row(n, num_rows):
    padding = '   ' * (num_rows-n)
    return padding + ''.join(["%3s" % x for x in row(n)])

Now we're almost there. Just need to tie it all together:

# When n=3, returns [3, 2, 1, 2, 3]
def row(n):
    return list(reversed(range(2, n+1))) + list(range(1, n+1))

def formatted_row(n, num_rows):
    padding = '   ' * (num_rows-n)
    return padding + ''.join(["%3s" % x for x in row(n)])

num_lines = eval(input("Enter the number of lines: "))
for n in range(1, num_lines+1):
    print(formatted_row(n, num_lines))

Agree, the first block definitely puts you in an infinite loop. Also the three while loops don't make it clear what you are doing - better to break up the padding from the pyramid construction.

I'm also working from Yang's book. This challenge in Ch.5 assumes no knowledge of functions but the above  got me close to a solution with this:

lines = input("enter the number of lines")

lines = int(lines)

count = 1

while count < lines+1:

    padding = '   '*(lines-count)

    digits = list(range(1,count+1))

    output = padding + str(digits[::-1]) + str(digits[1:])
    print(output)
    count+=1


C system() function vulnerability Suppose we have the following program:

#include <stdlib.h>
#include <stdio.h>

int main()
{
        char *user = getenv("USER");
        char buffer[4096];

        if (user) {
                snprintf(buffer, sizeof buffer, "/bin/echo %s", user);
                system(buffer);
        }

        return 0;
}

Is it possible for a user to cause this program to execute arbitrary commands?

If this is actually in live code somewhere, then whomever wrote it should be forced to write I will never call system on tainted user input again 1 googol times with a dull pencil.  Yes.  As written, this code contains an exploit.  Here's a simple example.

tmp$ cat foo.c
#include <stdlib.h>
#include <stdio.h>

int
main() {
    char *user = getenv("USER");
    char buffer[4096];
    if (user) {
        snprintf(buffer, sizeof buffer, "/bin/echo %s", user);
        printf("running %s\n", buffer);
        system(buffer);
    }
    return 0;
}
tmp$ gcc foo.c
tmp$ mkdir /tmp/xxx
tmp$ ls -ld /tmp/xxx
drwxr-xr-x  2 daveshawley  users  68 Nov 30 22:25 /tmp/xxx/
tmp$ USER='foo; rm -fr /tmp/xxx' ./a.out
running /bin/echo foo; rm -fr /tmp/xxx
foo
tmp$ ls -ld /tmp/xxx
ls: /tmp/xxx: No such file or directory
tmp$

It is not possible.  You'd have to write something that changes the USER environment parameter that effectively backspaces over the previous characters, and system("string") does not actually use a shell through a pseudo-terminal to execute, which is where backspace processing happens.

edit:
Despite comments below, and a down-vote, they are incorrect.

/bin/sh processing (or whatever shell is specified) only processes backspace & other edit characters when attached to a terminal device for input.

In addition, the original author is intent on subverting a program not of his own making.


Test if a number is within 2 of a multiple of 10 I have solved this exercise, but it looks really redundant. Is there a built-in function in Java or a cleaner way to caluclate mod of a value + deviation without multiple OR statements? If I had to calculate this for a deviation of 5, this would then require a lot of repetition.

  Given a non-negative number "num", return true if num is within 2 of a multiple of 10. Note: (a % b) is the remainder of dividing a by b, so (7 % 5) is 2.

    public boolean nearTen(int num) {
        return isMod10(num) || isMod10(num - 1) || isMod10(num + 1) || isMod10(num - 2) || isMod10(num + 2);
    }

    private boolean isMod10(int num) {
        return num % 10 == 0;
    }

You should read about congruent integers and modular arithmic. The distance of a number to a base is the minimum distance of its congruent value and its inverted congruent value.

 public boolean nearBase(int num, int base, int deviation) {

        // ..TODO check guards, normalize input or throw exceptions
        // - base and deviation are expected strict positive integers
        // - deviation is expected smaller than base
        // - num is expected a positive integer

        var congruent = num % base;
        var inverted = base - congruent;
        var distance = Math.min(congruent, inverted);
        return distance <= deviation;
    }

 // your method rewritten ->
 public boolean nearTen(int num) {
        return nearBase(num, 10, 2);
    }

Thanks to @RoastedPotatoe's answer above; I now understand that we want to consider only those numbers which are in the range \$((10-2)\%10)\$ to \$((10+2)\%10)\$. For example \$(8\%10)\$ to \$(12\%10)\$.
This means our use cases to be passed are:

\$(8\%10=8)\$,
\$(9\%10=9)\$,
\$(10\%10=0)\$,
\$(11\%10=1)\$, and
\$(12\%10=2)\$.

From here I know I can just say:

if the remainder comes out to be 8 or 9 return true. Otherwise,
if the remainder to be between 0 and 2 return true. Otherwise,
return false.

public boolean nearTen(int num){
    if((num%10==8 || num%10==9) || (num%10 >= 0  && num%10 <= 2)) return true;
    else return false;
}


HttpURLConnection response code handling This snippet from a downloader callable handles HTTP status codes. I need critique on both the style (for or do-while loop better here?) and functionality. Should I manage the delay differently? Do I need to handle InterruptedException specifically?

HttpURLConnection connection = null;
boolean connected=false;
outer:
for(int retry=0;retry<=RETRIES&&!connected;retry++)
{
 if(retry>0) {log.warning("retry "+retry+"/"+RETRIES);Thread.sleep(RETRY_DELAY_MS);}
 connection = (HttpURLConnection)entries.openConnection();
 connection.connect();
 switch(connection.getResponseCode())
 {
  case HttpURLConnection.HTTP_OK: log.fine(entries +" **OK**");connected=true;break; // fine, go on
  case HttpURLConnection.HTTP_GATEWAY_TIMEOUT: log.warning(entries+" **gateway timeout**");break;// retry
  case HttpURLConnection.HTTP_UNAVAILABLE: System.out.println(entries+ "**unavailable**");break;// retry, server is unstable
  default: log.severe(entries+" **unknown response code**.");break outer; // abort
 }
}
connection.disconnect();
log.severe("Aborting download of dataset.");

Readability
Let's reformat your code quickly:

    HttpURLConnection connection = null;
    boolean connected = false;
    outer: for (int retry = 0; retry <= RETRIES && !connected; retry++) {
        if (retry > 0) {
            log.warning("retry " + retry + "/" + RETRIES);
            Thread.sleep(RETRY_DELAY_MS);
        }
        connection = (HttpURLConnection) entries.openConnection();
        connection.connect();
        switch (connection.getResponseCode()) {
            case HttpURLConnection.HTTP_OK:
                log.fine(entries + " **OK**");
                connected = true;
                break; // fine, go on
            case HttpURLConnection.HTTP_GATEWAY_TIMEOUT:
                log.warning(entries + " **gateway timeout**");
                break;// retry
            case HttpURLConnection.HTTP_UNAVAILABLE:
                System.out.println(entries + "**unavailable**");
                break;// retry, server is unstable
            default:
                log.severe(entries + " **unknown response code**.");
                break outer; // abort
        }
    }
    connection.disconnect();
    log.severe("Aborting download of dataset.");

Fixed:

1-liners are hard to read:
  case HttpURLConnection.HTTP_UNAVAILABLE: System.out.println(entries+ "**unavailable**");break;// retry, server is unstable

spacing inside for-loop conditions:
  for(int retry=0;retry<=RETRIES&&!connected;retry++)

indentation - 1-space indentation is 'just wrong'™.

Functionality
Right, how does this code work? There are a few problems I can (now) see in here:

On HTTP_OK, it breaks out of the switch, then it exits the loop (because && !connected), and then immediately disconnects and logs a severe error? This can't be right?
HTTP_GATEWAY_TIMEOUT logs a warning.... great, but HTTP_UNAVAILABLE does a System.out.println

The InterruptedException handling is not shown here. There are articles on how to do this properly. Google up on that.
Suggestion
I recommend a sub-function, with a do-while loop (the following is a quick hack-up - untested):
private static final HttpURLConnection getConnection(URL entries) throws InterruptedException{
    int retry = 0;
    boolean delay = false;
    do {
        if (delay) {
            Thread.sleep(RETRY_DELAY_MS);
        }
        HttpURLConnection connection = (HttpURLConnection)entries.openConnection();
        switch (connection.getResponseCode()) {
            case HttpURLConnection.HTTP_OK:
                log.fine(entries + " **OK**");
                return connection; // **EXIT POINT** fine, go on
            case HttpURLConnection.HTTP_GATEWAY_TIMEOUT:
                log.warning(entries + " **gateway timeout**");
                break;// retry
            case HttpURLConnection.HTTP_UNAVAILABLE:
                log.warning(entries + "**unavailable**");
                break;// retry, server is unstable
            default:
                log.severe(entries + " **unknown response code**.");
                break; // abort
        }
        // we did not succeed with connection (or we would have returned the connection).
        connection.disconnect();
        // retry
        retry++;
        log.warning("Failed retry " + retry + "/" + RETRIES);
        delay = true;

    } while (retry < RETRIES);

    log.severe("Aborting download of dataset.");

}

Working Solution

    public int callAPI() {
        return 1;
    }

    public int retrylogic()  throws InterruptedException, IOException{
        int retry = 0;
        int status = -1;
        boolean delay = false;
        do {
            if (delay) {
                Thread.sleep(2000);
            }

            try {
                status = callAPI();
            }
            catch (Exception e) {
                System.out.println("Error occured");
                status = -1;
            }
            finally {
                switch (status) {
                case 200:
                    System.out.println(" **OK**");
                    return status;
                default:
                    System.out.println(" **unknown response code**.");
                    break;
                }
                retry++;
                System.out.println("Failed retry " + retry + "/" + 3);
                delay = true;
            }
        }while (retry < 3);

        return status;
    }


Accessing the only element of Java ResultSet Does this code look OK?

public Map<String, String> getById(long id) {
    String sql = "select * from " + this._TABLE_NAME + " where id = ?";
    PreparedStatement p;
    Map<String, String> result = new HashMap<String, String>();
    try {
        p = conn.prepareStatement(sql);
        p.setLong(1, id);
        ResultSet rs = p.executeQuery();

        ResultSetMetaData rsmd = rs.getMetaData();
        int fieldsCount = rsmd.getColumnCount();
        // is this ok?
        rs.next();

        for (int i=1;i<fieldsCount+1;i++) {
            String cName = rsmd.getColumnName(i);
            result.put(cName, rs.getString(cName));
        }

    } catch (SQLException e) {
        e.printStackTrace();
    }

    return result;

}

Here is a bit reworked version of you code. Some explanation:

Assume _TABLE_NAME is a constant, so there is no need to build query in every method call.
Force getting connection and close resources to avoid side effects.
Propagate SQLException to caller (caller should decide what to do with thrown exception).

Code:

private final static String GET_DATA_BY_ID = "select * from " + _TABLE_NAME + " where id = ?";

public Map<String, String> getById(long id) throws SQLException {

    Connection conn = null;
    Map<String, String> result = new HashMap<String, String>();

    try {
        conn = getConnection();

        PreparedStatement preparedStatement = null;

        ResultSet rs = null;
        try {
            preparedStatement = conn.prepareStatement(GET_DATA_BY_ID);
            preparedStatement.setLong(1, id);
            try {
                rs = preparedStatement.executeQuery();

                if (rs.next()) {
                    ResultSetMetaData rsmd = rs.getMetaData();
                    int fieldsCount = rsmd.getColumnCount();

                    for (int i = 1; i < fieldsCount + 1; i++) {
                        String cName = rsmd.getColumnName(i);
                        result.put(cName, rs.getString(cName));
                    }
                }
            } finally {
                if (rs != null)
                    rs.close();
            }
        } finally {
            if (preparedStatement != null)
                preparedStatement.close();
        }
    } finally {
        if (conn != null)
            conn.close();
    }
    return result;
}

Why not just do it like:

while(rs.next()) {
    result.put(....);
    return result;
}

You can also wrap it in a try-catch-finally.

Also, the use of prepared statement may not be justified in this example, since you use it only once.


Fastest way to create random pixel image I want to create a very big gigantic PNG image. The image pixel are generated randomly, being
How can I improve the below code to create a fast random pixel image in java?
import java.io.File;
import java.io.IOException;
import java.awt.image.BufferedImage;
import javax.imageio.ImageIO;
public class RandomImage
{
    public static void main(String args[])throws IOException
    {
        // Image file dimensions
        int width = 30000, height = 24000;

        // Create buffered image object
        BufferedImage img = null;
        img = new BufferedImage(width, height, BufferedImage.TYPE_INT_ARGB);

        // file object
        File f = null;

        // create random values pixel by pixel
        for (int y = 0; y < height; y++)
        {
            for (int x = 0; x < width; x++)
            {
                int a = (int)(Math.random()*256); //generating
                int r = (int)(Math.random()*256); //values
                int g = (int)(Math.random()*256); //less than
                int b = (int)(Math.random()*256); //256

                int p = (a<<24) | (r<<16) | (g<<8) | b; //pixel

                img.setRGB(x, y, p);
            }
        }

        // write image
        try
        {
            f = new File("G:\\Out.png");
            ImageIO.write(img, "png", f);
        }
        catch(IOException e)
        {
        System.out.println("Error: " + e);
        }
    }
}

You can write arrays of Integers directly on the WritableRaster of the BufferedImage:
public static BufferedImage createRandomImage(final int width, final int height) {
    final BufferedImage result = new BufferedImage(width, height, BufferedImage.TYPE_INT_ARGB);

    final long bytesPerPixel = 4L;

    final int[] pixelData = new SplittableRandom().ints(bytesPerPixel * width * height, 0, 256).toArray();

    result.getRaster().setPixels(0, 0, width, height, pixelData);

    return result;
}

This performs more than three times faster on my machine in a very crude benchmark (3.92S vs 1.26S), with the caveat that for very big images like the one you desire, you have a lot of data duplication, because you basically have to allocate twice the amount of memory, since you're writing the random data to a buffer first.
This can easily be solved by writing arrays of random Integers line by line though, which nets comparable performance:
public static BufferedImage createRandomImage(final int width, final int height) {
    final BufferedImage result = new BufferedImage(width, height, BufferedImage.TYPE_INT_RGB);

    final long bytesPerPixel = 4L;

    for (int y = 0; y < height; y++) {
        final int[] pixelData = new SplittableRandom().ints(bytesPerPixel * width, 0, 256)
                                                      .toArray();

        result.getRaster().setPixels(0, y, width, 1, pixelData);
    }

    return result;
}

BufferedImage.setRGB is very slow, because for every pixel you set, it has to go fetch the corresponding data element:
public void setRGB(int x, int y, int rgb) {
    raster.setDataElements(x, y, colorModel.getDataElements(rgb, null));
}

which is a comparatively heavy operation.

Parallelization
Crude parallelization improves performance slightly, but it does not scale linearly with number of cores. On my 24 thread machine, the following yields a 30-40% improvement compared to the loop WritableRaster variant:
public static BufferedImage createRandomImage(final int width, final int height) {
    final BufferedImage result = new BufferedImage(width, height, BufferedImage.TYPE_INT_ARGB);

    final long bytesPerPixel = 4L;

    IntStream.range(0, height).parallel().forEach(y -> {
        final int[] pixelData = new SplittableRandom().ints(bytesPerPixel * width, 0, 256)
                                                      .toArray();
        result.getRaster().setPixels(0, y, width, 1, pixelData);
    });

    return result;
}

In regards to speed, I'm not seeing a whole lot you could improve. One thing that comes to mind is the use of lower level APIs -- i.e. don't use File and BufferedImage. You could, for example, use a DataOutputStream (https://docs.oracle.com/javase/10/docs/api/java/io/DataOutputStream.html) which, from my understanding, is one of Java's lowest level APIs for writing data to a file. You could do something like this:
DataOutputStream stream = new DataOutputStream(new FileOutputStream("path/to/file"));

Then with this, you can use methods like writeByte to continuously populate the image file, rather than doing it in one whole buffer. I don't know if in the end this produces a faster result, but it produces a result such that the user could stop the program partway through and still end up with an image.
The caveat here is that you need to research what the proper metadata for a PNG file looks like and output that manually.

Something you could also try is using one loop as opposed to two.
To do that, you would do something like this:
for(int i = 0; i < width * height; i++) {
    //row=floor(n / width)
    //col=n-(row*width)
}

(I think I did the math right on those row and col numbers but you might want to check me).
This might be faster -- I don't know if the extra computations will slow things down, but it'd be worthy to try out.

Finally, unrelated to speed: a lot of things in your code are hardcoded right now (i.e. the width, height, and file name).
I would instead export everything to a separate function for generating random images, and that function could take parameters width, height, and file name. Then, in your main function, you use some sort of user input to determine what to pass to that function (e.g. command-line arguments or STDIN).
Additionally, having a separate function makes it unit-testable.


Multiplying/Dividing before Adding/Subtracting I'm working on a calculator for educational purposes and was asked to include a rule that multiplies/divides before it adds/subtracts.

Let's assume I have the following equation:

2 + 2 * 2

Windows built-in calculator would return 8, since it just goes down the line without parantheses.

The correct answer however would be 6.

So my code first splits the entire equation into a List<string>:

string equation = "2 + 2 * 2"
List<string> equationList = equation.Split(' ').ToList();
//Result:
//2
//+
//2
//*
//2

Then it checks if equationList contains an asterisk or slash:

if (equationList.Contains("*")) equationList = PunktVorStrich("*", equationList);
if (equationList.Contains("/")) equationList = PunktVorStrich("/", equationList);

This automatically calls the following function PunktVorStrich:

private List<string> PunktVorStrich(string symbol, List<string> equationList)
{
    while (equationList.Contains(symbol))
    {//loops in case there are multiple instances of the given symbol
        int position = equationList.IndexOf(symbol); //position of the symbol
        int before = int.Parse(equationList.ElementAt(position - 1)); //position of the number before the symbol
        int after = int.Parse(equationList.ElementAt(position + 1)); //position of the number after the symbol
        equationList.RemoveAt(position - 1);
        equationList.RemoveAt(position - 1);
        equationList.RemoveAt(position - 1);//Remove all 3 elements
        //finally calculate and "replace" the equation-part with the result
        if(symbol == "*")
        {
            equationList.Insert(position - 1, (before * after).ToString());
        }else if(symbol == "/")
        {
            equationList.Insert(position - 1, (before / after).ToString());
        }
    }
    return equationList;
    //equationList =
    //2
    //+
    //4
}

And finally the program loops through the remaining equation:

bool number = true; //This ensures that the equation is split between operators and values
int total = 0; //Sum of the current equation
string lastOperator = String.Empty; //Stores the last operator

foreach (string term in equationList)
{
    if (number)
    {
        if (lastOperator == "") total = int.Parse(term);
        if (lastOperator == "+") total = total + int.Parse(term);
        if (lastOperator == "-") total = total - int.Parse(term);
    }
    number= !number;
    lastOperator = term;
}

Finally I can return the equation with

MessageBox.Show(total.ToString());

I'm however not exactly happy with this solution, since I personally think that took a few steps too much to achieve my goal. Is there anything I can do better?

Review

The calculator requires a very specific syntax and depends on whitespace delimiters. As a consumer it is really easy to make a mistake in using the calculator.
I would expect a basic calculator to include parentheses parsing as well as the basic operators +-/*.
You split the input in a series of string tokens List<string> equationList. String is hard to work with once you start extending the calculator. Invest in a custom class Token that has at least a Value (string) and a Type (Operator, Operand, Parenthesis, Whitespace, ..)
Method PunktVorStrich return the same list as provided in the parameters. It could be made void if you allow the provided list to get changed in the method body.
I would not perform the multiplication/division evaluation in the preprocessor. It should be done in a later phase.

Limitations

You have created a preprocessor function that transforms any input string into one that only uses addition and subtraction.

"1 + 1 * 3"

comes out as

"1 + 3"

Since the expression still remains to be evaluated, you'd have to go over the expression again, this time only dealing with the basic operators. I would not use a function like PunktVorStrich in the preprocessing phase, I would include it in the expression evaluator. This would gain performance, by only iterating the expression once.

By the way, how would you evaluate this expression? There is no consensus in math.

"2 / 3 * 4"

If you want to extend your calculator, you'll notice it would get harder to maintain the evaluator, since you have a fixed format <number><whitespace><operator>.. and evaluate left to right. As you noticed, a simple multiplication and division already are special cases to this left to right scanner. The following simple expressions can currently not be parsed.

"1+1"
"(1 + 1) + 1"

Expression Tree Evaluator

What you could do, is tokenize the input, build an expression tree and evaluate the tree. For instance "(1 + 1) + 1" could have a syntax tree:

//             add
//     add          1
//  1       1

You should read more about evaluating expressions using lexers and parsers. Try refactoring the simple evaluator with only +-/* to this structure.

this is called Operators Precedence, you can work with this by placing "(" around your formulas.

Say you plan to do

(buy-Sell) * Sales Tax = Tax on Invoice

leave away the brackets and it would execute like this

buy - (Sell * Sales Tax) = BAD Tax on Invoice

You can "google" Operators Precedence to see the operators precedence applied for a given language as it applies to more than * and /

Have a look at this link that goes and explains a bit in C#


Express middleware to validate URL and tags I am building an API where the client can send a url and some comma-delimited tags:

{
   "url": "https://www.youtube.com/watch?v=pquxHIBx8ks",
   "tags": "JavaScript, Sequelize"
}

Before processing this data, I want to validate the input. To do that, I wrote a middleware function called validateSubmitScreencastReq:

import config from 'config'
import youtubeUrl from 'youtube-url'
import db from 'sequelize-connect'
import * as youtubeClient from '../../source/util/youtubeClient'

async function validateUrl (url) {
  if (!url) {
    return 'url cannot be undefined'
  }

  if (!youtubeUrl.valid(url)) {
    return 'url must be a valid YouTube URL'
  }

  const client = youtubeClient.create(config.youtubeApiKey)
  if (!await client.videoExists(url)) {
    return 'url must link to an existent, public YouTube video'
  }

  const foundScreencast = await db.models.screencast.findOne({
    where: {
      url: url
    }
  })
  if (foundScreencast !== null) {
    return 'url has already been submitted'
  }

  return undefined
}

function validateTags (tags) {
  if (!tags) {
    return 'tags cannot be undefined'
  }

  if (typeof tags !== 'string') {
    return 'tags must be a string'
  }

  return undefined
}

export async function validateSubmitScreencastReq (req, res, next) {
  try {
    const urlError = await validateUrl(req.body.url)
    const tagError = validateTags(req.body.tags)
    const errors = []
    if (urlError) {
      errors.push({
        field: 'url',
        message: urlError
      })
    }
    if (tagError) {
      errors.push({
        field: 'tags',
        message: tagError
      })
    }
    if (errors.length !== 0) {
      res.status(400).json({errors})
    } else {
      next()
    }
  } catch (error) {
    next(error)
  }
}

I'm not too happy with this code.

The function essentially aggregates error messages, which makes me think of using reduce but I can't see how to make it work elegantly.

What's more, I don't think I'm expressing that the function aggregates errors very clearly.

How can I improve the readability the above code, preferably using functional concepts?

(If it's at all useful, here is the validateSubmitScreencastReq function on GitHub and here are the corresponding unit tests.)

export async function validateSubmitScreencastReq (req, res, next) {
  try {
    const errors = [
        {field: 'url', message: await validateUrl(req.body.url)},
        {field: 'tags', message: validateTags(req.body.tags)}
    ].filter((obj) => obj.message !== undefined);

    if (errors.length !== 0) {
      res.status(400).json({errors})
    } else {
      next()
    }
  } catch (error) {
    next(error)
  }
}

This removes any non-errors from the originally constructed array and should give the same end-result as your code.

You can also improve your validate url/valide tag functions:

const assert = require("assert");//common nodejs package

async function validateUrl (url) {
  try {
    assert(url, "url can not be undefined");
    assert(youtubeUrl.valid(url), 'url must be a valid YouTube URL');

    const client = youtubeClient.create(config.youtubeApiKey)

    assert(await client.videoExists(url), 'url must link to an existent, public YouTube video');
    assert(!(await db.models.screencast.findOne({where: {url: url}})), 'url has already been submitted');
  } catch (error) {
    /* remove the if, if you want to also return whatever those functions may
       have thrown as an error */
    if (error instanceof AssertionError) {
      return error.message;
    }
  }
  //No return needed if you want to return undefined
};

Avoiding the '!' and nesting your conditions would be good. They would then throw relevant errors.

if (url) {
    if (youtubeUrl.valid(url)) {
        const client = youtubeClient.create(config.youtubeApiKey)
        if (await client.videoExists(url)) {
            const foundScreencast = await db.models.screencast.findOne({
                where: {
                    url: url
                }
            });
            if (foundScreencast === null) {
                return;
            } else {
                return 'url has already been submitted'
            }
        } else {
            return 'url must link to an existent, public YouTube video'
        }
    } else {
        return 'url must be a valid YouTube URL'
    }
} else {
    return 'url cannot be undefined'
}

No errors would be aggregated then, and only a single error will be thrown. That would be relevant enough. You errors array would be very clean as well.


Algorithm to determine if a string is all unique characters This my solution to one of the practice problems from Cracking the Coding Interview: 150 Programming Interview Questions and Solutions [Book]

  implement an algorithm to determine of a string has all unique characters. What if you cannot use additional data structures ?

public class PracticeProblems {

    public void questionOne(String input) {
        /*-- implement an algorithm to determine if a string has all unique characters.
         * What if you cannot use additional data structures?   --*/

        boolean[] chars = new boolean[26];
        int x = 0;

        for(int i = 0; i < input.length(); i++) {

            if(!chars[(int)input.toUpperCase().charAt(i) - 64]) {
                chars[(int)input.toUpperCase().charAt(i) - 64] = true;
            }
            else {
                System.out.println("not unique");
                x = -1;
                break;
            }
        }

        if(x == 0)
            System.out.println("unique");
    }

    public static void main(String[] args) {
        PracticeProblems test = new PracticeProblems();
        test.questionOne("dsfdddft");
    }
}

I was wondering if there was a better solution to this, or if there is a better way of handling the last part, where I am initializing an x variable to be able to determine if I all the characters are not unique, not to print "it is unique". If don't have the condition for the x value it always prints "it is unique".

You probably shouldn't call toUpperCase() twice on each character.

If you wanted to split your code properly (if it was for a real-life project for instance), it would make sense to make the documentation a bit better and to define a method taking a String as an argument and returning a boolean. Let's keep things simple for the time being; you can return immediately after printing "not unique". If you do so, there's no need for the test on x and there's no need for x at all.

You probably should check that the characters are in the right range before accessing chars.

I'd rather read if (c) { A } else { B } than if (!c) { B } else { A } even though it depends from one situation to another. In our case, it also allows to remove a level of nesting because of the return.

You don't need an instance of PracticeProblems at all, and the function could just be static.

Finally, I do not know if Java optimises out the different call to length() so we might ensure we don't call it every time.

public class PracticeProblems {
    public static void questionOne(String input) {
        boolean[] chars = new boolean[26];
        String upper = input.toUpperCase();

        for(int i = 0, n = upper.length(); i < n; i++)
        {
            char c = upper.charAt(i);
            if ('A' <= c && c <= 'Z')
            {
                if(chars[(int)c - 'A'])
                {
                    System.out.println("not unique");
                    return;
                }
                chars[(int)c - 'A'] = true;
            }
        }
        System.out.println("unique");
    }

    public static void main(String[] args) {
        questionOne("dsfdddft");
    }
}

If string has only lowercase or upper case characters then you can use this O(1) solution. No extra memory or any additional data structure.

bool checkUnique(string s){
    if(s.size() >26)
        return false;
    int unique=0;
    for (int i = 0; i < s.size(); ++i) {
        int j= s[i]-'a';
        if(unique & (1<<j)>0)
            return false;
        unique=unique|(1<<j);
    }
    return true;
}


Extracting spreadsheet cells using Apache POI I'm working with Apache POI framework.

This code go through sheets, row and get value of cells. I will try to get value from directly indicated column. I need this because I want convert only some information from xls to csv.

FileOutputStream fos = new FileOutputStream(outputFile);

Workbook workbook = null;
for (File input : inputFile) {
  String ext = FilenameUtils.getExtension(input.toString());

  if (ext.equalsIgnoreCase("xlsx")) {
    workbook = new XSSFWorkbook(new FileInputStream(input));
  } else if (ext.equalsIgnoreCase("xls")) {
    workbook = new HSSFWorkbook(new FileInputStream(input));
  }

  int numberOfSheets = workbook.getNumberOfSheets();
  Row row;
  Cell cell;
  Sheet sheet = null;
  int[] kolumny = {1, 2, 3, 4, 5, 6, 7, 50, 51, 52, 53, 54, 55};

  for (int i = 1; i < numberOfSheets; i++) {
    sheet = workbook.getSheetAt(i);
    if (sheet.getLastRowNum() > 24) {
      Iterator<Row> rowIterator = sheet.iterator();
      while (rowIterator.hasNext()) {
        row = rowIterator.next();
        for (int j = 23; j < sheet.getLastRowNum(); j++) {
          if (row.getRowNum() == j) {
            if (row.getCell(1) == null || row.getCell(1).getCellType() == Cell.CELL_TYPE_BLANK) {
              break;
            }
            Iterator<Cell> cellIterator = row.cellIterator();
            while (cellIterator.hasNext()) {
              cell = cellIterator.next();
              for (int kolumna : kolumny) {
                if (cell.getColumnIndex() == kolumna) {
                  if (cell != null) {
                    switch (cell.getCellType()) {
                      case Cell.CELL_TYPE_BOOLEAN:
                        data.append(cell.getBooleanCellValue() + ",");
                        break;
                      case Cell.CELL_TYPE_NUMERIC:
                        data.append(cell.getNumericCellValue() + ",");
                        break;
                      case Cell.CELL_TYPE_STRING:
                        data.append(cell.getStringCellValue() + ",");
                        break;
                      case Cell.CELL_TYPE_BLANK:
                        data.append("" + ",");
                        break;
                      default:
                        data.append(cell + ",");
                    }
                  }
                }
              }
            }
            data.append('\n');
          }
        }
      }
    }
  }
}

You could extract the content of inner loops to methods with speaking names.

E.g. You could introduce a method like

public static void processSheet(Sheet sheet)
{
  if (sheet.getLastRowNum() > 24) {
      Iterator<Row> rowIterator = sheet.iterator();
      while (rowIterator.hasNext()) {
        row = rowIterator.next();
  ...

}

Within this method you can extract another method e.g.

public static void processRow(Row row){
  for (int j = 23; j < sheet.getLastRowNum(); j++) {
    if (row.getRowNum() == j) {
      if (row.getCell(1) == null || row.getCell(1).getCellType() == Cell.CELL_TYPE_BLANK) {
        break;
    ...

}

And so on until you have only short methods with speaking names. This makes the code easier to read and understand.

You can try to use this jar library:

https://afajardomorera.github.io/ExcelReader/

You can define a Java Object with the fields you need, configure a property file (where you can define concrete cells to extract and/or data rows from a table to extract) and invoke the reader method from the jar.

In the github code, you find a jar_generated folder with the latest version of the jar to use it.


Filtering a name Array in JavaScript I was wondering if there is a better, more elegant way to write the following JavaScript code block:

var searchString = 'ma';
var names = ['Mark Kennel', 'Hellen Smith', 'Jane Mary Annet', 'Peter'];

var filteredNames = names.filter(name => {
  var words = name.toLowerCase().split(' ');
  for(var i = 0; i < words.length; i++) {
    if (words[i].indexOf(searchString.toLowerCase()) === 0 )
      return true;
  }
}

This runs in a function that filters an autocomplete input results. The filtering rule is:

  Show the names that have a word which begins with the searchString

The above code (with searchString = 'ma') should return an array with two names: 'Mark Kennel' and 'Jane Mary Annet'.

Well, better and more elegant are quite opinion based and depend on different things: a code can be better regarding performance but worse regarding clarity.

That being said, I'd like to drop my two cents: since you just want to know if a given word begins with the input string (which is different from containing the input string) you can use lastIndexOf, setting fromIndex as 0. That way, you don't search the whole string, but just its beginning:

var searchString = 'ma';
var names = ['Mark Kennel', 'Hellen Smith', 'Jane Mary Annet', 'Peter'];

function filterNames(arr, str) {
  return arr.filter(function(thisName) {
    return thisName.toLowerCase().split(" ").some(function(d) {
      return d.lastIndexOf(str.toLowerCase(), 0) === 0
    })
  })
}

console.log(filterNames(names, searchString))

Another obvious option here is using startsWith. However, unlike lastIndexOf, startsWith don't wok on IE (if you care for that).

You can also use includes()

var searchString = 'ma';
var names = ['Mark Kennel', 'Hellen Smith', 'Jane Mary Annet', 'Peter'];

var filteredNames = names.filter(name => {
  var words = name.toLowerCase().split(',');
  for(let key in words) {
    if(words[key].includes(searchString))
     return true;
  }
});