Executing an operator in an RPN evaluator Is there any way to take this chunk of an else if and make it its own method? I would need the values of valid and throwLine() to be done in the method containing this if else statement, but I'm around 45 lines in my method which is 15 more than I'm allowed to have for simplicity.

 else if(isOperator(readIn))
         {
            System.out.print(readIn);
            if(myStack.isEmpty())
               valid = false;
            else
               op2 = (Fraction)myStack.pop();

            if(myStack.isEmpty())
            {
               throwLine(readIn);
               valid = false;
            }
            else
            {
               runTheOperator(op2, op1, readIn);
               readIn = stdin.next();
            }

As @DavidHarkness points out, you didn't provide enough context for this question to make sense.  However, I see that this code comes from a doTypeCheck() method, which is at the heart of your RPN calculator implementation.  I'll reproduce that method here, along with its call site:

private void doTypeCheck(Fraction op1, Fraction op2)
{
    Fraction answer = null;
    String readIn = "";
    boolean valid = true;
    readIn = stdin.next();

    while(!readIn.equals("#") && valid == true)
    {
       if(!isOperator(readIn) && isOperand(readIn))
       {
          processOperand(readIn);
          readIn = stdin.next();
       }
       else if(isOperator(readIn))
       {
          System.out.print(readIn);
          if(myStack.isEmpty())
             valid = false;
          else
             op2 = (Fraction)myStack.pop();

          if(myStack.isEmpty())
          {
             valid = false;
             throwLine(readIn);
          }
          else
          {
             runTheOperator(op2, op1, readIn);
             readIn = stdin.next();
          }
       }
       else
       {
          System.out.print(readIn);
          valid = false;
          throwLine(readIn);
       }
    }
    System.out.println();
    if(myStack.isEmpty())
       valid = false;
    else
       answer = (Fraction)myStack.pop();
    if(!myStack.isEmpty())
       valid = false;
    checkMessageValid(valid, answer);
}

private void runOnce(int count)
{
    Fraction op1 = null;
    Fraction op2 = null;
    clear(myStack, myQueue);

    System.out.print("Expression " + count++ + " is: ");
    doTypeCheck(op1, op2);
 }

The root problem is that your method has no clear purpose, and tries to do many things:

Read tokens from the input stream, and decide whether they are operands or operators
If it sees invalid input, discard the rest of the input and display an error
Pop an operand from the stack, with error handling
Feed two operands to the calculating function
Check that the stack is empty afterwards
Display the answer

As I mentioned in my other answer, that's not a good way to write an RPN calculator.

The main loop of an RPN evaluator should probably look something like this:

try {
    while (in.hasNext()) {
        String token = in.next();
        RpnOperator op = RpnOperator.forSymbol(token);
        if (op != null) {
            op.operate(calcStack);
        } else {
            calcStack.push(new Fraction(token));
        }
    }
    System.out.println(calcStack.peek());
} catch (EmptyStackException emptyStack) {
    System.out.println("Error: empty stack");
}

That's all!  To summarize:

Let the operators manipulate the stack.  The main loop just needs to dispatch to the appropriate operators.
Use exceptions for error handling.


PHPMailer hack prevention I have been in conversation with a fellow web developer on whose servers I am doing some client work. Part of the current project is to add an email sending system from a web based form to my clients email via PHPMailer.

I have used PHPMailer repeatedly with no issues whatsoever, however, this developer has said to me that they do not want PHPMailer on the server due to having found previous instances on their server of PHPMailer being 'hacked'.

Please note that with all form to email systems I use I do believe I fully escape and clean the input strings as far as possible.

I'm asking here:

Is hacking PHPMailer version 5.2.8 specifically even possible?
How is this possible?
What can be done to secure PHPMailer in these instances?

Is there a halfway clear way that the above script could be hacked or otherwise hijacked by a hacker or similar?

Please note: all $member array values are from a database and have already been pre-cleaned.

require "PHPMailerAutoload.php"; ///version 5.2.8

        $message = "The following enquiry about " . $member['bizname'] . " was sent to " . $member['contname'] . " :
    Enquirer's Name: " . $name . ",
    Enquirer's Address:
         " . $addr1 . "
         " . $addr2 . "
         " . $addr3 . "
         " . $postcode . "
    Telephone Number: " . $tel . ",
    Email address: " . $email . ",
    Dates they are interested in: " . $dates . ",
    Comments: " . $comments . ".

    NOTE: This is an automatic notification email for your records.
    Please do not reply to this email.
        ";

         $mail           = new PHPMailer();
        $mail->Host     = "localhost";
        $mail->WordWrap = 78;
        $mail->Mailer   = "smtp";
        $mail->From     = "server@website.co.uk";
        $mail->FromName = "Website Server";

///this is the client email and would not change.
        $mail->AddAddress("static@email-address.co.uk");
        $mail->Subject = $member['bizname'] . " Website Enquiry";
        $mail->Body    = $message;
        if (!$mail->Send()) {
            $dispmsg = "Mailer 1 error: " . $mail->ErrorInfo;
        } else {
            $dispmsg = "Your email has been sent to " . $member['bizname'] . " who will be in touch with you shortly.";
        }
    ////////////end email 1

is hacking PHPMailer version 5.2.8 specifically even possible? How is this possible?

That's really hard to say. If you use external libraries there is always a chance that they contain vulnerabilities, but the same is true for your custom code (and nobody is going to check that for vulnerabilities).

I didn't find any public disclosures for that version, so that's good.

The last ones also seem to be quite old (input validation issues in 1.7.3, code exec in 1.7, DOS in 1.7.2, email injection in 2012).

  what can be done to secure PHPMailer in these instances?

You could look at the PHPMailer code yourself, and try to find vulnerabilities (the main class is only 3.5k lines, and a lot of it are comments).
You should only put those files in an open directory that are needed (ie don't include the test/example files).
You could also use an application firewall/intrusion detection system such as mod_security to prevent the exploitation of possible vulnerabilities.
And you should always use the latest version, which seems to be 5.2.9, not 5.2.8.

Misc

your indentation and spacing is off, which makes your code a bit harder to read.
don't shorten variable names. For example, displayMessage is easier to read than dispmsg.
you didn't include the actual echoing of $dispmsg, but if $member['bizname'] is user supplied and $dispmsg not sanitized, your code would be open to XSS.
having // end something comments is a sign of bad design. Your code should be organized in a way that it is obvious when something ends. That means extracting functionality into functions, and avoiding deep nesting.


Reading Min and Max From Text File I am trying to write a program that will read the highest and lowest voltage along with its associated amp and time. I am able to find the max, but I am getting all zeros for the minimum value. I need help. Please and thank you.

Sample of txt file:

Time    Volt    Ampere
0.0001  9.77667 0.147408
0.00015 9.76583 0.147525
0.0002  9.76833 0.147692
0.00025 9.75833 0.147442
0.0003  9.76833 0.147192
0.00035 9.78167 0.1473
0.0004  9.76667 0.147317
0.00045 9.765   0.14715
0.0005  9.75667 0.147
0.00055 9.765   0.14695
0.0006  9.77    0.1471
0.00065 9.7675  0.147417
0.0007  9.7725  0.147417
0.00075 9.755   0.14735
0.0008  9.765   0.147725
0.00085 9.76583 0.147783

Expected Min Output:

Time = 0.00075  Volt = 9.755 Ampere = 0.14735

Expected Max Output:

Time= 0.00035 Volt = 9.78167 Ampere = 0.1473

int main(void)
{
//Declare Variables
string a, b, c;

double time, volt, ampere;
double maxVolt = 0, maxTime, maxAmpere;
double minVolt = 10, minTime, minAmpere;

//Read from the Volts.txt file
ifstream myFile("D:\\tabit\\Documents\\Volts.txt");

//Check if the file can be opened
if (myFile.is_open())
{
    while (myFile >> a >> b >> c)
    {
        time = atof(a.c_str());
        volt = atof(b.c_str());
        ampere = atof(c.c_str());

        if (volt >= maxVolt)
        {
            maxTime = time;
            maxVolt = volt;
            maxAmpere = ampere;
        }
        if (volt < minVolt)
        {
            minTime = time;
            minVolt = volt;
            minAmpere = ampere;
        }
    }
    //Close the file
    myFile.close();
}
//Give error message if the file cannot be opened
else return(1);

//Display the Maximum results
cout << "Max Volt: " << maxVolt << endl;
cout << "Max Time: " << maxTime << endl;
cout << "Max Ampere: " << maxAmpere << endl;

//Display the Minimum results
cout << "Min Volt: " << minVolt << endl;
cout << "Min Time: " << minTime << endl;
cout << "Min Ampere: " << minAmpere << endl;

return 0;
}

The value of atof("Volt") is zero.  The corresponding values of atof("Time") and atof("Ampere") are also zero, which gives you the minimum values of zero you observed.

You need to skip the first line of the file.  Adding this before the while loop would work:

myFile.ignore(80, '\n');

Use std::numeric_limits<std::streamsize>::max() instead of 80 for a more correct (but verbose) limit of characters to skip before the new line character.

After you add the skipping of the first line, you no longer need the a, b & c variables and atof() calls;  you can read the values directly:

while (myFile >> time >> volt >> ampere)


Markdown.pl extending I'm not used with perl but had the need to add a couple of features, I wish I can adopt a more perl-ish  coding.

Any help is welcome.

This is to add style classes to paragraphs if strarting with !, ? and a clear: both with %

Original:

foreach (@grafs) {
    unless (defined( $g_html_blocks{$_} )) {
        $_ = _RunSpanGamut($_);
        s/^([ \t]*)/
/;
        $_ .= "
";
    }
}

Modded:

foreach (@grafs) {
    unless (defined( $g_html_blocks{$_} )) {
        $_ = _RunSpanGamut($_);
        if ( m/^[ \t]*\?/ ){
            s/^([ \t]*\?[ \t]*)/
/;
        } elsif ( m/^[ \t]*\!/ ){
            s/^([ \t]*\![ \t]*)/
/;
        } elsif ( m/^[ \t]*%/ ){
            s/^([ \t]*%[ \t]*)/
/;
        } else {
            s/^([ \t]*)/
/;
        }
        $_ .= "
";
    }
}

This is to add float to images like: !<[alt](url) to get a float right (and form this the % above to clear)

if (defined($align)) {
    if ($align eq "<") {
        $result .=  " style=\"float: left; margin: 0 ${padding}px ${padding}px 0;\"";
    } else {
        $result .=  " style=\"float: right; margin: 0 0 ${padding}px ${padding}px;\"";
    }
}

Note that a substitution only occurs if there is a match, and then it will return the number of substitutions it makes. This means that a true value is returned if a match/substitution happens.

With that in mind, you don't need the m/.../ in addition to the substitution. You can also make use of or to achieve the short-circuiting that you had with the if ... elsif ... else ... statements.

foreach (@grafs) {
    unless (defined $g_html_blocks{$_}) {
        $_ = _RunSpanGamut($_);
        s/^([ \t]*\?[ \t]*)/
/
            or
        s/^([ \t]*\![ \t]*)/
/
            or
        s/^([ \t]*%[ \t]*)/
/
            or
        s/^([ \t]*)/
/;
        $_ .= "
";
    }
}

For the image floating, you could use a hash instead of if ... else ...:

my %float = (
    '<' => 'float: left',
    '>' => 'float: right',
    '%' => 'clear: both'
);
if (defined $align) {
    $result .= " style=\"$float{$align}; margin: 0 ${padding}px ${padding}px 0;\"";
}

A minor point: I also removed the parens from the defined checks because they're not needed and it gives you a little more white space to reduce a little clutter.


Templated Point class of any dimension I needed to make a point class for a term project that involved arbitrary dimensional points.  I came up with this, and I know that it works fine, but I'm looking to improve it so I can reuse it in the future.  Any criticisms or suggestions are very much appreciated.

#ifndef POINT_HPP
#define POINT_HPP

#include <array>
#include <cmath>
#include <initializer_list>
#include <iostream>

template <size_t D>
struct Point;

template <size_t D>
struct Point {
  Point(const std::initializer_list<double>& v) {
    size_t i = 0;
    for (auto e : v) {
      coordinates_[i++] = e;
    }
  }

  explicit Point(const std::array<double, D>& arr) : coordinates_{arr} {}

  double operator[](size_t dimension) const { return coordinates_[dimension]; }
  Point<D>& operator=(const Point& other) {
    this->coordinates_ = other.coordinates_;
    return *this;
  }
  std::array<double, D> coordinates_;
};

/*
 * Finds euclidian distance between two Point classes of same dimension
 */
template <size_t D>
double distance(const Point<D>& lhs, const Point<D>& rhs) {
  double dist = 0;
  for (size_t i = 0; i < D; i++) dist += std::pow(lhs[i] - rhs[i], 2);
  return std::sqrt(dist);
}

/*
 * Finds euclidian distance between Point and axis
 */
template <size_t D>
double distance(const Point<D>& point, double value, int axis) {
  return std::abs(point[axis] - value);
}

/*
 * Print the point components of a Point in ascending order.
 * Prints in the form {x, y, z} with no newline.
 */
template <size_t D>
std::ostream& operator<<(std::ostream& os, const Point<D>& point) {
  os << "{";
  for (size_t i = 0; i < D - 1; i++) {
    os << point[i] << ", ";
  }
  os << point[D - 1] << "}";
  return os;
}

template <size_t D>
bool operator==(const Point<D>& lhs, const Point<D>& rhs) {
  return lhs.coordinates_ == rhs.coordinates_;
}

namespace std {
/*
 * Make our Point class hashable
 */
template <size_t D>
struct hash<Point<D>> {
  size_t operator()(const Point<D>& x) const {
    return hash<array<double, D>>()(x.coordinates_);
  }
};

/*
 * Also make it swappable for sorting
 */
template <size_t D>
void swap(Point<D>& lhs, Point<D>& rhs) {
  swap(lhs.coordinates_, rhs.coordinates_);
}
}  // namespace std

#endif  // POINT_HPP

Overall, this seems to be a small but useful template.  However, there are a few things that I think might be improved.

Sorting requires more than swap

While the provided swap function would assist in some kind of sorting, without a comparison operator, we still can't actually sort.  This may well be by design, in which case a comment in the code pointing this out would be helpful.

Consider default constructor

There is no simple way at the moment to create a generic templated origin point (all coordinates zero).  This makes it a bit more difficult to create something like a distance_from_origin function that is generic.

Consider projection operator

It's often useful to project, say, a 3D point onto a 2D canvas.  This could also likely be made generic.

Do a range check

The distance function that measures from an axis does no range checking on the axis number and there does not seem to be a good generic way to add one.  Consider providing a range checking version.


NodeJS logging module This code is for a NodeJS logging module. I created it because I wanted to find a way to log different type of things but not show them all at the same time. I also wanted to format the logs in a custom manner.

Is there anything I could do to improve this code, such as making it shorter? Are there still any bugs?

// original way of adding config
//var config  = require('config');
var config  = {
     debug: true,
     debugArray:['info','warn']//add remove log types to show
     //'info','warn','error','log','system','socketio','sql'
};

var dateFormat = require('dateformat');
var chalk = require('chalk');
var path = require('path');
var Logger = Object.create({});
var define = Object.defineProperty.bind(undefined, Logger);

function doLog(msg, type, search, stack){
  if(config.debug && ( config.debugArray.indexOf(search) > -1)){
      var fdate = dateFormat(new Date(), 'yyyy-mm-dd hh:MM:ss.l');
      var fFilename = stack.getFileName().substr(path.dirname(require.main.filename).length + 1);
      var lineNumber = stack.getLineNumber();
      var preMsg =  fdate + ' ' +  type + ' [' + chalk.yellow(fFilename + ':' + lineNumber) + '] ';
      console.log(preMsg + msg);
  }
}
Logger.log = function (txt){
  doLog(txt,'LOG', 'log', this.stack[1]);
};

Logger.system = function (txt){
  doLog(chalk.gray(txt), chalk.gray('SYSTEM'), 'system', this.stack[1]);
};

Logger.socketio = function (txt){
  doLog(chalk.cyan(txt), chalk.cyan('SOCKET'), 'socketio', this.stack[1]);
};

Logger.sql = function (txt){
  doLog(txt, chalk.cyan('SQL'), 'sql', this.stack[1]);
};

Logger.info = function (txt){
  doLog(chalk.cyan(txt), chalk.cyan('INFO'), 'info', this.stack[1]);
};

Logger.warn = function (txt){
  doLog(chalk.yellow(txt), chalk.yellow('WARN'), 'warn', this.stack[1]);
};

Logger.error = function (txt){
  doLog(chalk.red(txt), chalk.red('ERROR'), 'error', this.stack[1]);
};

define('stack', {
  get: function(){
    var originalStack = Error.prepareStackTrace;
    Error.prepareStackTrace = function(_, stack){ return stack; };
    var err = new Error();
    Error.captureStackTrace(err, arguments.callee);
    var stack = err.stack;
    Error.prepareStackTrace = originalStack;
    return stack;
  }
});

module.exports = Logger;

You have an unneeded comma in line 5. Although it won't make Node.js crash it is invalid JavaScript. And as you are using semicolons you should use them everywhere. So you should end your statement in line 7 with a semicolon.


Invoicing Program Python This is my invoicing program I made for myself to keep track of purchases people made from me. (Yes I know eBay does this for me but I wanted to see what I could make.) Basically it can search the invoice file for a keyword (the invoice number) or lets you enter a new invoice to a file named usb.txt. It is usb.txt because I was selling USB cables at the time.

#Invoice Entering Program Ebay

print("Ebay Invoice Program\n")

while True:
    file = input("Enter 0 to Quit. Enter S to Search The File. Press ENTER to Input new Entries *Case Sensitive*: " )
    if file == "0":
        print("Exiting Program...")
        break
    elif file=="S":
        keyword = input("Enter Invoice Number to Search For: ")
        look_in = "usb.txt"
        search_file = open(look_in,"r")
        with open(look_in, "r") as search_file:
            for line in search_file:
                if keyword in line:
                    print(line)

    else:

        Name = input("Enter Buyer Name: ")
        InNum = input("Enter Ebay Invoice Number: ")
        Quant = int(input("Enter Amount Bought: "))
        Price = Quant * 5
        with open("usb.txt", "at")as out_file:
            out_file.write(
                "Name: %s || Invoice Number: %s || Quantity: %s || Price:$ %s \n\n" % (Name, InNum, Quant, Price))
            out_file.close()
            print("File edits made")

I'm pretty new to python and programming in general. If you have any suggestions on how I could improve it or any ideas to add functionality I'd love to hear it! This was written in python 3.5.1.

What happens if the user types "s"?  Your program says:

Enter Buyer Name:

Oops!  It was supposed to be uppercase, wasn't it?  I'm not sure why you want it to be case-sensitive, but you should tell the user of his mistake instead of just going on as if you know what he meant.  That means, I would change else: to elif not file: and add an else: that prints something like Sorry, I don't know what that means.

Your search_file = ... line is useless because you redefine search_file on the next line (with open(...) as search_file:).  By the way, I am happy to see that you are using with blocks.  That is rare, but good practice.

Your naming is not consistent with PEP 8, the Python style guide, on Method Names and Instance Variables:

  Use the function naming rules: lowercase with words separated by underscores as necessary to improve readability...

You use int(input(...)) without a try block.  That is dangerous.  If the user types something that is not a valid integer, there will be a very nasty error.  You should use a while loop that keeps going until the user types a valid integer.  The validity test would, of course, be with a try-except block.

out_file.close() is unnecessary in your with block.  The with block will take care of closing the file.

You might want to take a look at the csv module.  With its help, you are more flexible with the format in which the entries are printed.  Your printing code might end up looking something like this:

template = "Name: {name} || Invoice Number: {invoice} || Quantity: {quantity} || Price: ${price}\n\n"

...

with open(look_in, "rt") as search_file:
    for line in csv.DictReader(search_file):
        if line['invoice'] == keyword:
            print(template.format(**line))

Your writing code:

with open("usb.txt", "at") as out_file:
    writer = csv.DictWriter(out_file, (
      'name', 'invoice', 'quantity', 'price')):

    writer.writeheader()
    writer.writerow({
        'name': Name, 'invoice': InNum, 'quantity': Quant,
        'price': Price})

As it is, an entry might be displayed just because its name includes the invoice number.  With a csv file, that is no longer possible.


How can I optimize this hash string function? I'm writing a function which gets two strings and a number k as an input, and outputs a string with length k which is present in both strings.  I want to do this using the function hash_sequence().

I need my code to be as efficient as possible because my inputs are very large strings.

def hash_sequence(string, k):

    dictionary={}
    for i in range(len(string)-(k-1)):
        triplet = string[i:i+k]
        dictionary.setdefault(triplet,[]).append(i)
    return dictionary

def intersects(string1, string2, k):
    itr=min(len(hash_sequence(string1,k)),len(hash_sequence(string2,k)))
    for i in range(itr):
        if list(hash_sequence(string1,k))[i] in hash_sequence(string2,k):
            return list(hash_sequence(string1,k))[i]
    return None

Your hash_sequence() function is quite good. The only thing I would suggest about that function is rename your triplet variable to something more suitable like sequence. Currently, I expect triplet to contain strings of length 3. A name like sequence is more general and is a better representation of what will be stored in it.

Your intersects() function is where the vast improvement can be found. Each iteration of the for loop you are re-running the hash_sequence() function. If your strings are really long and k really small (i.e. 1), hash_sequence() would have to scan through the entirety of both strings each iteration of the loop!

Instead, we only have to hash one of the strings! Before the loop save the returned dict to a variable (this way the hash_sequence function is only ran once):

def intersects(string1, string2, k):
    dictionary= hash_sequence(string1, k)

Next, instead of iterating over the length of the dicts we can iterate over the keys in the dict, then we can check if that key is 'in' the other string:

for key in dictionary:
    # Check if the key (string of length k) is in the other string
    if key in string_two:
        return [key]

Here is my full version of your code:

def hash_sequence(string, length):
    dictionary = {}
    for i in range(len(string) - (length-1)):
        sequence = string[i:i+length]
        dictionary.setdefault(sequence, []).append(i)

    return dictionary

def intersects(string_one, string_two, length):
    dictionary = hash_sequence(string_one, length)

    for key in dictionary:
        if key in string_two:
            return [key]
    return None

As a final note, take a look at PEP8, the official Python style conventions. It will help make your code more Pythonic.


String reversal, capitalize vowels, lowercase consonants This was a portion of a skills test given to me by a company. The task was to write a method that takes a string, reverses it, and sets the vowels as capitals and consonants to lowercase. I was also told to prove that my implementation works.

using System;
using System.Diagnostics;

namespace StringOperation
{
    using static Console;

    internal class Program
    {
        #region Entry

        private static void Main()
        {
            while (true)
            {
                WriteLine(MessageStrings.REQUEST_NAME);
                var strResult = ReadLine();

                var result = ProcessInput(string.IsNullOrEmpty(strResult) ? MessageStrings.PROOF : strResult );

                if ((ReadKey(true).Key ^ ConsoleKey.Y) == 0)
                    continue;
                break;
            }
        }

        #endregion

        #region Logic

        private static string ProcessInput(string input)
        {
            var output = new char[input.Length];

            var desc = output.Length;
            var asnd = -1;

            while(asnd++ <= desc--)
            {
                // unroll and reverse
                output[asnd] = Shift(input[desc]);
                output[desc] = Shift(input[asnd]);
            }

            return new string(output);
        }

        private static char Shift(char target)
        {
            // switching the 32 bit and returning the new value
            var i = (target | 0x20);

            // Checking if a the new value is a symbol
            if (i < 97 || i > 122)
                return target;

            switch (i)
            {

                case 97:
                case 101:
                case 105:
                case 111:
                case 117:
                    // Returning a capital vowel
                    return (char) (target & ~0x20);
                default:
                    // returning Lower case consonant
                    return (char) (i);
            }
        }

        #endregion
    }
}

We have a namespace called StringOperation,
a class called Program,
and methods called Main,
ProcessInput, Shift,
regions called Entry, Logic.

Looking from a high level,
this program could do anything,
as these terms are completely meaningless.
Strive to find more meaningful names for your program elements,
so that readers can have a clue what the program is about without having to fully read the implementation.
If I can see in front of me the logical pieces of the puzzle,
the mental burden of understanding what goes on in each element would be much reduced.

I know that all self-respecting programmers know by heart that 97 means the ASCII code of the letter A, 101 the letter E, 105 the letter I, ...
Wait a minute, no, actually that's not a requirement,
I don't think anybody should have to know at a glance what these numbers mean in your case statements.
A better way would be to write like this:

case 'A':
case 'E':
case 'I':
// ...


Parse a date in various formats I have a problem similar to the question, namely reading in text which describes a date - in various formats. I want to use the parseTimeM function from Date.Time module in time package.

My current solution can probably be improved style wise but it should remain easy to read and easy to extend. Suggestion?

readDate3 :: Text ->   UTCTime
readDate3 datestring  =

    case shortMonth of
        Just t -> t
        Nothing -> case longMonth of
            Just t2 -> t2
            Nothing -> case monthPoint of
                Just t3 -> t3
                Nothing -> case germanNumeralShort of
                  Just t3 -> t3
                  Nothing -> case germanNumeral of
                    Just t3 -> t3
                    Nothing -> case isoformat of
                      Just t4 -> t4
                      Nothing -> errorT   ["readDate3", datestring, "is not parsed"]

    where
        shortMonth = parseTimeM True defaultTimeLocale
            "%b %-d, %Y" (t2s datestring) :: Maybe UTCTime
        longMonth = parseTimeM True defaultTimeLocale
            "%B %-d, %Y" (t2s datestring) :: Maybe UTCTime
        monthPoint = parseTimeM True defaultTimeLocale
            "%b. %-d, %Y" (t2s datestring) :: Maybe UTCTime
        germanNumeral = parseTimeM True defaultTimeLocale
            "%-d.%-m.%Y" (t2s datestring) :: Maybe UTCTime
        germanNumeralShort = parseTimeM True defaultTimeLocale
            "%-d.%-m.%y" (t2s datestring) :: Maybe UTCTime
        isoformat = parseTimeM True defaultTimeLocale
            "%Y-%m-%d" (t2s datestring) :: Maybe UTCTime

Don't repeat yourself

Your code violates the DRY principle. If we replace

germanNumeralShort = parseTimeM True defaultTimeLocale
    "%-d.%-m.%y" (t2s datestring) :: Maybe UTCTime
isoformat = parseTimeM True defaultTimeLocale
    "%Y-%m-%d" (t2s datestring) :: Maybe UTCTime
...

with

parse format = parseTimeM True defaultTimeLocale format (t2s datestring)

germanNumeralShort = parse "%-d.%-m.%y"
isoformat          = parse "%Y-%m-%d"
...

then we immediately notice that we use parse on all formats after another till we find a suitable one.

This can be modelled with map parse, e.g.

map parse
    [ "%b %-d, %Y"
    , "%B %-d, %Y"
    , "%b. %-d, %Y"
    , "%-d.%-m.%Y"
    , "%-d.%-m.%y"
    , "%Y-%m-%d"
    ]

We could use <|> alternatively, e.g.

parse "%b %-d, %Y" <|> parse "%B %-d, %Y" <|> ...

but that's less flexible than the list approach.

Use asum to get a single Maybe from a list of Maybes

To get a single Maybe a from [Maybe a], we can use asum. To get the errorT, we just need to pattern match on a single result and end up with

readDate3 :: Text ->   UTCTime
readDate3 datestring = case result of
   Nohing -> errorT ["readDate3", datestring, "is not parsed"]
   Just t -> t
 where
   parse format = parseTimeM True defaultTimeLocale format (t2s datestring)
   result = asum . map parse $
        [ "%b %-d, %Y"
        , "%B %-d, %Y"
        , "%b. %-d, %Y"
        , "%-d.%-m.%Y"
        , "%-d.%-m.%y"
        , "%Y-%m-%d"
        ]

As the current strings are missing some documentation, we could introduce additional types to remedy that:

data DateFormat = ShortMonth
                | LongMonth
                | MonthPoint
                | GermanNumeral
                | GermanNumeralShort
                | ISOFormat

toFormatString :: DateFormat -> String
toFormatString f = case f of
    ShortMonth -> "%b %-d, %Y"
    LongMonth  -> "%B %-d, %Y"
    MonthPoint -> "%b. %-d, %Y"
    -- other left as an exercise

We can also use fromMaybe to get rid of the last pattern match and end up with

import Data.Foldable (asum)
import Data.Maybe (fromMaybe)

readDate :: Text ->   UTCTime
readDate datestring =
    fromMaybe (errorT ["readDate", datestring, "is not parsed"]) $
      asum . map parse $
        [ ShortMonth
        , LongMonth
        , MonthPoint
        , GermanNumeral
        , GermanNumeralShort
        , ISOFormat
        ]
 where
   parse format = parseTimeM True defaultTimeLocale (toFormatString format) (t2s datestring)


Defining transpose on a collection of irregular collections I was asked to submit my request for code review on https://stackoverflow.com/questions/10672046/defining-transpose-on-a-collection-of-irregular-collections here.

This is a follow-up to the post I made in https://stackoverflow.com/questions/1683312/is-there-a-safe-way-in-scala-to-transpose-a-list-of-unequal-length-lists/10663749#10663749. I want to propose a definition of transpose for a list of unequal-sized lists by viewing List[A] as a mapping from Int to A. First, here's why I think the transpose defined in the referenced post is problematic.

scala>  val l = List(List(1, 2, 3), List(4, 5), List(6, 7, 8))
l: List[List[Int]] = List(List(1, 2, 3), List(4, 5), List(6, 7, 8))

scala> val lt = transpose( l )
lt: List[List[Int]] = List(List(1, 4, 6), List(2, 5, 7), List(3, 8))

scala> val ltt = transpose( lt )
ltt: List[List[Int]] = List(List(1, 2, 3), List(4, 5, 8), List(6, 7))

scala> l == ltt
res0: Boolean = false

So, transpose( transpose( l ) ) is not the same as l. The problem arises from the fact that this definition of transpose does not respect the precise indices associated with the elements (there is implicit "shifting" of elements when preparing the inner lists).

So I am proposing that transpose on a list of unequal-sized lists (List[List[A]]) be more abstractly viewed as an operation on a Map[Int,Map[Int,A]] (because a List[A] can be viewed as a Map[Int,A]). This operation is well-defined and can be further abstracted to operate on a Map[A,Map[B,C]] as follows:

def transpose[A,B,C]( mapOfMaps: Map[A,Map[B,C]] ) : Map[B,Map[A,C]] = {
  val innerKeys = ( mapOfMaps map { p => ( p._2.keys ) toList } ) flatten
  val outerKeys = ( mapOfMaps keys ) toList
  val allValues = innerKeys map { ik => outerKeys filter { ok => mapOfMaps( ok ).contains( ik ) } map { ok => ( ok, mapOfMaps( ok )( ik ) ) } toMap }
  ( innerKeys zip allValues ) toMap
}

To transform a list of lists to the more-abstract map of maps, zipping is convenient.

def listOfListsToMapOfMaps[A]( l : List[List[A]] ) : Map[Int,Map[Int,A]]= {
  val listOfMaps = l map { x => ( x.indices zip x ) toMap }
  ( listOfMaps.indices zip listOfMaps ) toMap
}

Now, things are clean:

scala> val m = listOfListsToMapOfMaps( l )
m: Map[Int,Map[Int,Int]] = Map(0 -> Map(0 -> 1, 1 -> 2, 2 -> 3), 1 -> Map(0 -> 4, 1 -> 5), 2 -> Map(0 -> 6, 1 -> 7, 2 -> 8))

scala>  val mt = transpose( m )
mt: Map[Int,Map[Int,Int]] = Map(0 -> Map(0 -> 1, 1 -> 4, 2 -> 6), 1 -> Map(0 -> 2, 1 -> 5, 2 -> 7), 2 -> Map(0 -> 3, 2 -> 8))

scala> val mtt = transpose( mt )
mtt: Map[Int,Map[Int,Int]] = Map(0 -> Map(0 -> 1, 1 -> 2, 2 -> 3), 1 -> Map(0 -> 4, 1 -> 5), 2 -> Map(0 -> 6, 1 -> 7, 2 -> 8))

scala> m == mtt
res1: Boolean = true

I see plenty of use for this generalized transpose operation in my work (and I'm sure for many others - across languages). But I'd like for people to review my transpose definition and implementation before I put it into my core library and start using it (I'm relatively new to Scala and am sure my implementation can be improved).

Yes, traspose defined such that it shrings the result to the size of the shortest list isn't an involution. Your idea of viewing lists as maps is nice, but I suppose once you leave Lists, there are other easier options.

As you observed, List[A] is also a PartialFunction[Int,A] that works on integers that are between 0 and the size of the list. Now if we have a list of lists, it's PartialFunction[Int,PartialFunction[Int,A]]. Now we can convert such functions into a single function by a process called un-currying. While Scala has uncurried for plain functions, it lacks it for PartialFunctions. But it's not difficult to define it:

def uncurried[A,B,C](f: PartialFunction[A,PartialFunction[B,C]]):
    PartialFunction[(A,B),C] =
  Function.unlift((x: (A,B)) => f.lift(x._1).flatMap(_.lift(x._2)));

So we can apply uncurried immediately on List[List[A]] (or rather to Seq[Seq[A]] to avoid O(n) access for lists) and we get a functional representation that accepts a pair of indices and gives the result. If we swap the indices in a pair, we get a transposed view.

The problem with this representation is that we can't enumerate all matching indices easily. If this is required, we could instead convert a list of lists into Map[(Int,Int),A], perhaps a SortedMap so that we can enumerate the indices in a proper order.  Again, this will be a PartialFunction[(Int,Int),A], but also with this additional ability.


Query URL parameters in downloading pictures from network cameras I have some URLs saved in my database, to download pictures from network cameras. Because each camera has it's own URL and parameters, we have to store the entire URL in a field in the database.

My code gets this URL from the database, creates a WebClient to make the call and downloads the byte array from it.

Some cameras use security credentials that we save as parameters in the URL, they are saved like this because it was determined it would be this way, I would have preferred to have fields in the database with username and password, but they decided to go this way, so my code got more messy.

Here is the part of the code that deals with the credentials:

var uri = new Uri(url);
if (uri.Query.Length > 0)
{
    var querys = uri.Query;
    if (querys.Contains("username") && querys.Contains("password"))
    {
        var userName = querys.Split('&').SkipWhile(x => !x.StartsWith("username")).First().Split('=').Last();
        var password = querys.Split('&').SkipWhile(x => !x.StartsWith("password")).First().Split('=').Last();
        var type = querys.Contains("type") ? querys.Split('&').SkipWhile(x => !x.StartsWith("type")).First().Split('=').Last() : "Digest";

        cache = new CredentialCache { { uri, type, new NetworkCredential(userName, password) } };
    }
}

Later on the code I will use the cache variable in the WebClient to make the call with it.

I used a lot of LINQ here to get the username and password from the URL.

My questions here are:

Is my code readable enough?
Is it the best approach? If not, how could I have made this better?
Should I be worried about performance in this implementation?

Here is an example of the URL:

http://192.168.2.33/cgi-bin/video.cgi?msubmenu=jpg&username=admin&password=4321&type=Digest

Is my code readable enough?

Unfortunatelly there is no objective answer to this question. Whether code is readable is mainly a personal opinion. I think you code isn't readable because it calls a lot of methods one after anthoer in a single line. I find it's hard to tell where one ends and the other starts.

So I suggest adding a few line breaks to clearly separate all calls:

var userName =
    query
        .Split('&')
        .SkipWhile(x => !x.StartsWith("username"))
        .First()
        .Split('=')
        .Last();

If you wrote it like this from the beginning it would also make your code much longer because you use very similar code in several places. This would probaly make you think does it have to be this way? and make you look for a different solution. This new solution is called encapsulation.

You don't want to have anything more the once. But more about this under the next question.

  Is it the best approach? if not how could i have made this better?

You need to define what best means to you.

Is the code best when it's short or when it's testable?
Is it best when it is quick&dirty written or is it best when it's maintainable?
Is it best when it's everything in one place or is is it best if you have dedicated modules for everything and use a lot of dependency injection?

My No. 1 definition of best (production) code is: testable + maintainable (this does not necessarily mean short and/or simple). To achieve this two goals you code needs to be at least DRY and it should satisfy the  Single Responsibility Principle.

This will make sure you can test each algorithm only once and if you find a bug you need to fix it in only one place.

Currently your code is not DRY. It repeats the same logic several times (parsing). It's also does not satisfy the SRP because it parses the query string and fills the cache by the same method.

You should encapsulate the parsing either by a helper method if this is the only place you need it or write a dedicated class and use it as a service with Dependency Incjection.

If you have everything already properly encapsulated you can always upgrade the code to a service if you need it later somewhere else. Why should you implement it again? I wouldn't, I'm lazy :-)

Your code could e.g. parse the query string into a dictionary which is much easier to use then always run through the entire string (worst case scenario) with Contains. Parse it once and reuse the results. I would, I'm lazy - but lazy is a good thing - it makes you write DRY code.

var query =
    uri
        .Query
        .TrimStart('?')
        .Split('&')
        .Select(x => x.Split('='))
        .ToDictionary(
            x => x[0],
            x => x.ElementAtOrDefault(1)
        );

I don't know if you always have pairs but in case you don't, the ElementAtOrDefault will make this not crash if a value is missing.

  Should i be worried with performance in this implementation?

This question can be answered only by you. You need to know how often it's called and whether it might become a potential bottleneck. But I'm pretty sure you won't see it in any benchmarks. There are parts in your code that are with certainty much slower. The best way to tell whether your code needs optimization and where, is to use a profiler.


Parsing Title from any given website I've created a very small script in vba to parse Title from almost any website. I've checked it with few websites and it is providing with the accurate results. I used split function on responsetext to achieve the result. Websites should be placed in column A and the results will be displayed in column B adjacent to each site. I tried to make it flawless.

Sub Title_Data()
Dim http As New MSXML2.XMLHTTP60

For Each cel In Range("A1:A" & Cells(Rows.Count, 1).End(xlUp).row)
    With http
        .Open "GET", cel.Value, False
        .send

        L = L + 1
        Cells(L, 2) = Split(Split(.responseText, "<title>")(1), "</title>")(0)
    End With
Next cel
Set http = Nothing
End Sub

Error handling

What if there is no <title> in a web page,
or if it's written as <TITLE>?
The script will crash.

It we would be good to check the result of Split to make sure you don't access an invalid index.

Extracting substrings

Instead of Split,
it would be better to use Instr to find the start and end positions of the title, and then Mid to extract the substring.
This is better because Split creates an array of strings, and scan until the end of the input string. Instr can stop immediately when it finds a match and doesn't need to create new objects.


Jump Search in Python I have implemented Jump search in Python, here is my code. Am I using the algorithm correctly ? Is there any pythonic way to achieve same ?

import math
def jump_search_2(arr,search):
    interval = int(math.sqrt(len(arr)))

    ''' find last lowest element '''
    for i in range(0,len(arr),interval):
        if arr[i] < search:
            low = i
        elif arr[i] == search:
            return i
        else:
            break
    ''' apply linear search '''
    l_index = [e for e,i in enumerate(arr[low:low+interval]) if i == search]
    if l_index[0]:
        return low+l_index[0]
    return "Not Found"

Usage example:

arr = [ i for i in range(1,300,15)]
res = jump_search_2(arr, 16)
print(res)

For the moment, I did not dive into the details of your implementation, but one thing is obvious:

When you find search, you return an integer value corresponding to its position.
When you do not find it, you return a string .

This adds a little complication as how to call (and test) your function. But if you replace the returned string by None, it will be easier to call your function:

# Let us use your working example
if jump_search_2(arr, 16):
   # print("Found !")
else:
   # print("Not Found")
   # This is more pythonic than:
   # elif (jump_search_2(arr, 16) == "Not Found")


Finding one string located in another Can anyone help me improve my instr() function?

int myInstr(wchar_t *str, wchar_t c, int start, int dir){
    int pos = 0, result = 0, len;
    wchar_t *p1;

    //Left
    if(dir == 0)
    {
        p1 = str;
        len = lstrlen(str);
        while(pos < len)
        {
            if(*p1 == c)
            {
                result = pos;
                return result;
            }
            p1++;
            pos++;
        }
    }
    else
    {
      //Right
      pos = len = lstrlen(str);
      p1 = str;
      p1 += len;
      while(pos >= 0)
      {
          if(*p1 == c)
          {
              result = pos;
              return result;
          }
          p1--;
          pos--;
      }
    }
    return 0;
}

Here are my comments:

Functionality:

Remove the superfluous start parameter.
Remove all needlessly complex (and slow) pointer arithmetic.
Use the standard wcslen() function from wchar.h
Position 0 is a valid array index, replace it with an invalid one if value not found.

Performance:

Only call the strlen function once.
Minimize the code inside the if/else statement.

Readability and style:

str isn't modified by the function, so make it const (const correctness).
Rewrite the loop to a conventional one.
Only return once from a function.
Replace all "magic numbers" with constants or enums.

-

#include <stddef.h>
#include <wchar.h>
#include <stdio.h>

typedef enum
{
  DIR_POS,
  DIR_NEG
} dir_t;

#define NOT_FOUND -1

int myInstr (const wchar_t*  str,
             wchar_t         c,
             dir_t           dir)
{
  int start;
  int end;
  int step;
  int length;
  int i;
  int result = NOT_FOUND;

  length = wcslen(str);

  if(dir == DIR_POS)
  {
    start = 0;
    end   = length;
    step  = 1;
  }
  else /* if (dir == DIR_NEG) */
  {
    start = length - 1;
    end   = -1;
    step  = -1;
  }

  for(i=start; i!=end; i+=step)
  {
    if(str[i] == c)
    {
      result = i;
      break;
    }
  }

  return result;
}

int main()
{
  wchar_t str [] = L"1234567890";

  printf("%d\n", myInstr(str, '3', DIR_POS)); // prints 2
  printf("%d\n", myInstr(str, '6', DIR_NEG)); // prints 5
  printf("%d\n", myInstr(str, 'X', DIR_POS)); // prints -1
}


Processing a company list in which every element is an employee list Now, there is a company list in which an element is an employee list in every company. I calculate the salary of employees in every company with a RESTful API one by one,. This is very slow, so I join 1000 elements to one list and post it to the service.

import requests

def _calculate_employee_salaries(employees):
    response = requests.post(url, json={"input": employees})
    salaries = parse_response(response)
    return salaries

def get_calculate_employee_salaries(company_employees_lis):
    begin, step = 0, 1000
    company_employee_salaries_lis = []
    while begin < len(company_employees_lis):
        employees = []
        employee_num_lis = []
        for comp_employees in company_employees_lis[begin:(begin+step)]:
            employees.extend(comp_employees)
            employee_num_lis.append(len(comp_employees))
        salaries = _calculate_employee_salaries(employees)

        idx = 0
        for num in employee_num_lis:
            company_employee_salaries_lis.append(salaries[idx:(idx + num)])
            idx += num
        begin += step
    return company_employee_salaries_lis

if __name__ == "__main__":
    company_employees_lis = [["employees_id_1", "employees_id_2", "employees_id_3"], ["employees_id_4"], ["employees_id_5", "employees_id_6"]]
    company_employee_salaries_lis = get_calculate_employee_salaries(company_employees_lis)

I think my code is bad, because I use a variable employee_num_lis to remember the number of employees in every company. And there must be clean, artful and pythonic code. This code is just a sample, not what I should do.

Yes, you can simplify this. Let's break down the algorithm needed for this:

Chain all employees into one stream
Chunk this stream into 1000 employees

For each chunk get the salaries

Distribute the salaries back again into the different companies

The first part can be done using itertools.chain.from_iterable. The second one can use e.g. this grouper recipe. And the final part can be easily done with itertools.islice if we keep everything before as generators.

from itertools import chain, islice

def grouper(iterable, n):
    it = iter(iterable)
    return iter(lambda: tuple(islice(it, n)), ())

def get_salaries(employees):
    respone = requests.post(url, json={"input": employees})
    return parse_respone(respone)

def employee_salaries_per_company(company_employees):
    all_employees = chain.from_iterable(company_employees)
    salaries = chain.from_iterable(get_salaries(employees)
                                   for employees in grouper(all_employees, 1000))
    return [list(islice(salaries, n)) for n in map(len, company_employees)]

Note that in english the plural of employee is employees and not emploices; and the plural of salary is salaries and not salarices.


Python's str.isAlpha(), reimplemented without Unicode support I've created an alphabet checker that is similar to str.isAlpha().
I know that python has its built-in function.

  str.isalpha()
  Return true if all characters in the string are alphabetic and there is at least one character, false otherwise. Alphabetic characters are those characters defined in the Unicode character database as “Letter”, i.e., those with general category property being one of “Lm”, “Lt”, “Lu”, “Ll”, or “Lo”. Note that this is different from the “Alphabetic” property defined in the Unicode Standard.

I put my implementation code here:
https://gist.github.com/anonymous/7833d25fc450e69ee323bfc39bdd0da5

I included my code snippet below:

import string
    # Hint: Use these string constants to ignore capitalization and/or punctuation
string.ascii_lowercase = 'abcdefghijklmnopqrstuvwxyz'
string.ascii_uppercase = 'ABCDEFGHIJKLMNOPQRSTUVWXYZ'
string.ascii_letters = string.ascii_lowercase + string.ascii_uppercase

def ascii_letters(text):
    #This m ethod will return True if the string
    # is an uppercase or lowercase letter from A to Z.
    # If the string contains any non-letter characters, then it will return False.
    for index in text:
        if index in string.ascii_letters:
            return True
        else:
            return False
    return None

In this answer I am assuming that high performance is not needed

In general that approach is okay but I do not think that code is fulfilling the requirements. For example

ascii_letters("A2") == True
ascii_letters("") == None

I propose following steps:

Fix your algorithm (some unit tests would be great!)
Think about if is this really necessary to check whether the letter is in set of containing both lower and upper case letters.
Next, think about iteration over whole string: is this needed? There are functions in python which can check logical condition on whole list.
Why do you need to include string ? There is no need to override already existing variables.
Try to do pythonic one liner using all points above.


Another Python prime-listing algorithm I wrote a small program in Python that finds primes (how original) up to some limit. I tried to produce very clean code, so I'd be vary grateful if you could point out any improvement I could do to make it more pythonic and more readable.

I'm also interested in knowing whether or not it can be sped up (with the same method I used here). I tried to use generator, but since I want to read my list of primes multiple times it didn't work... I'm also wondering if a set or some other structure could be better than a list here.

Thank you very much !

from __future__ import print_function
import time

def measure_time(func):
    def wrapper(*args, **kw):
        starting_time = time.clock()
        res = func(*args, **kw)
        ending_time = time.clock()
        print('Duration : %fs' % (ending_time - starting_time))
        return res
    return wrapper

def ask_user_input():
    """Relentlessly ask user to enter an integer >1."""
    while True:
        try:
            lim = int(raw_input('What\'s the limit?   '))
            if lim == int(lim) and lim > 2:
                break
            else:
                print('Must be an integer larger than 1. Try again.')
        except (NameError, ValueError):
            print('Not a number. Try again.')
    return lim

def print_primes(primes):
    for p in primes:
        print(p)

@measure_time
def construct_primes_list(lim):
    """Builds the list of primes smaller than lim."""
    prime_divisors = [2]

    for num in xrange(2, lim):
        for d1 in prime_divisors:
            if num % d1 == 0:
                break
        else:
            prime_divisors.append(num)

    return prime_divisors

def main():
    lim = ask_user_input()
    primes = construct_primes_list(lim)
    # print_primes(primes)

if __name__ == '__main__':
    main()

Here is the benchmarking on my computer:

lim      | time
-----------------------
100      | 0.000143s
1000     | 0.003231s
10000    | 0.071654s
100000   | 2.598098s
1000000  | 169.052574s

EDIT :

As ChatterOne's comment suggested, I tried to compute the square root of num because I know I couldn't find any divisors after that. But it gets way slower. Here is the code :

for num in xrange(3, lim):
    sqrt_num = int(sqrt(num)+1)
    for d1 in [p for p in prime_divisors if p < sqrt_num]:
        if num % d1 == 0:
            break
    else:
        prime_divisors.append(num)

Any thoughts?

You will probably get better performance from the sieve of Eratosthenes. The key difference is that the sieve never performs modulo. Instead it assumes primes, and then crosses them off as you find multiples of primes.

def sieve(n):
   """Return a list of the primes below n."""
   prime = [True] * n
   result = [2]
   append = result.append
   sqrt_n = (int(n ** .5) + 1) | 1    # ensure it's odd
   for p in range(3, sqrt_n, 2):
       if prime[p]:
           append(p)
           prime[p*p::2*p] = [False] * ((n - p*p - 1) // (2*p) + 1)
   for p in range(sqrt_n, n, 2):
       if prime[p]:
           append(p)
   return result


Free Code Camp - Pairwise I'm working through the Free Code Camp syllabus and I'm on to Intermediate JavaScript Algorithms. This Pairwise problem was the last challenge in that section. The section came just after "Object Oriented JavaScript." So I figured they were looking for an OO solution, but the instructions included a link to MDN's array.reduce(). My solution doesn't use array.reduce() and I'd really appreciate some feedback on  what I could have done better to make my code more compact and efficient. It feels a little clunky but passes all the tests.

The instructions

  Return the sum of all indices of elements of 'arr' that can be paired
  with one other element to form a sum that equals the value in the
  second argument 'arg'. If multiple sums are possible, return the
  smallest sum. Once an element has been used, it cannot be reused to
  pair with another.

  For example, pairwise([1, 4, 2, 3, 0, 5], 7) should return 11 because
  4, 2, 3 and 5 can be paired with each other to equal 7.

  pairwise([1, 3, 2, 4], 4) would only equal 1, because only the first
  two elements can be paired to equal 4, and the first element has an
  index of 0!

  Remember to use RSAP if you get stuck. Try to pair program. Write your
  own code.

  Here are some helpful links:

Array.reduce()

My Solution

function pairwise(arr, arg) {
    this.objects = [];
    var total = 0;

    function Element(value, index) {
        this.value = value;
        this.index = index;
        this.used = 0;
    }

    for (var i = 0; i < arr.length; i++) {
        this.objects.push(new Element(arr[i], i));
    }

    for (var j = 0; j < objects.length; j++) {
        if (objects[j].used === 0) {
            for (var k = 0; k < objects.length; k++) {
                if (objects[k].used === 0 && objects[k].index != objects[j].index) {
                    if (arg - objects[j].value == objects[k].value) {
                        total = total + objects[j].index + objects[k].index;
                        objects[j].used = 1;
                        objects[k].used = 1;
                        break;
                    }
                }
            }
        }
    }

    return total;
}

pairwise([1,1,1], 2);

I looked through your code and it is a valid solution, but you could reduce your code base by better leveraging the functions that JavaScript already provides, such as Array.prototype.indexOf().

For example, instead of building a new class-like-function (Element) to track the appearance of a certain index, I simply made a deep copy of the the initial array and parsed it with indexOf().

Moreover, in your code, when you first declare this.objects = [], this actually refers to the global scope (window object). As you can see, you are calling pairwise without building a new instance (new keyword). In this case, thus the this keyword is bound to the global window object.

Please find below my take on it:

function pairwise(arr, arg) {

  var result = 0,
      newArr = [],
      //Used to hold the indices that we have already used to form our sum
      indices = [];

  //Loop through arr and create a deep copy of it in newArr
  for(var k = 0; k < arr.length; k++) {
    newArr.push(arr[k]);
  }

  //Loop through arr
  for(var i = 0; i < arr.length; i++) {

    //Loop through newArr
    for(var j = 0; j < newArr.length; j++) {
      //Since we want to add different elements of the array, we want to avoid adding the same element
      if(i !== j) {
        //If the sum of two elements is equal to arg AND the indices that we have in i and j are not part of the indices array
        //Indices array is used to hold the already used indices, thus ensuring the accurate parsing of the parameters
        if(arr[i] + newArr[j] === arg && indices.indexOf(i) === -1 && indices.indexOf(j) === -1) {
          //Sum the indices up
          result += i + j;
          //Push the indices in the indices array in order to not use them in further iterations
          indices.push(i, j);
        }
      }
    }
  }

  return result;
}

pairwise([1,4,2,3,0,5], 7);


Python program to find the factorial of a number using recursion A factorial of a number is the product of all the integers from 1 to that number.

For example, the factorial of 5 (denoted as 5!) is 1*2*3*4*5 = 120. The factorial of 0 is 1.

def recur_factorial(x):

    if x == 1:
        return 1
    else:
        return (x * recur_factorial(x - 1))

num = int(input("Enter number: "))

print("The factorial of", num, "is", recur_factorial(num))

So I would like to know whether I could make this program shorter and more efficient.

Also, I would like to know the advantages and disadvantages of recursion.

Looking over your code, this is what I have for you:

Your function can be turned into a one-liner
You should use if __name__ == '__main__' to ensure you're not running this program externally.

Refactored Code:

def recur_factorial(x):
  return 1 if x == 1 else (x * recur_factorial(x - 1))

def main():
  num = int(input("Enter number: "))
  print("The factorial of", num, "is", recur_factorial(num))

if __name__ == '__main__':
  main()

You can read this StackOverflow question for more information about the advantages and disadvantages of recursion


Dapper helper service I've written a very light wrapper around Dapper to help with unit testing and streamlining the Dapper commands I need.

Execute() is a method created to remove the code duplication of opening and closing the database connection and enlisting it in a transaction. The code is very quick at inserts with this method (200,000 inserts in less than 30 secs).

I'm wondering if I've made any obvious mistakes with regards to Dapper and performance.

For instance, should a single insert or read explicitly enlist a transaction, or would they be faster with an ambient transaction?

public class DappperService
{
    private readonly string sqlConnection;

    public DappperService(string sqlConnection)
    {
        this.sqlConnection = sqlConnection;
        DapperExtensions.DapperExtensions.DefaultMapper = typeof(PluralizedAutoClassMapper<>);
    }

    public T Get<T>(int id) where T : class
    {
        return Execute((x, t) => x.Get<T>(id, t));
    }

    public bool Delete<T>(T dto) where T : class
    {
        return Execute((x, t) => x.Delete(dto,t));
    }

    public dynamic Insert<T>(T dto) where T : class
    {
        return Execute((x, t) => x.Insert(dto,t));
    }

    public void Insert<T>(List<T> dtoList) where T : class
    {
        IEnumerable<T> enu = dtoList;

        using (var con = new SqlConnection(sqlConnection))
        {
            con.Open();
            var trans = con.BeginTransaction();
            con.Insert(enu, trans);
            trans.Commit();
            con.Close();
        }
    }

    public IEnumerable<T> GetList<T>() where T : class
    {
        return Execute((x, t) => x.GetList<T>(transaction:t));
    }

    public dynamic Execute(Func<SqlConnection, SqlTransaction, dynamic> action)
    {
        using (var con = new SqlConnection(sqlConnection))
        {
            con.Open();
            var trans = con.BeginTransaction();
            var result = action(con, trans);
            trans.Commit();
            con.Close();
            return result;
        }
    }
}

Naming

I recommend not using acronyms or shortened names where possible for your variables. You might as well use the full name, it aids in quick reading.

e.g.

public dynamic Insert<T>(T dataTransferObject) where T : class
{
    return Execute((x, t) => x.Insert(dataTransferObject,t));
}

Although for that method item may work just as well.

The same goes for single-letter variable names in your lambda expressions. If there's only parameter to your lambda, it's probably ok, but with multiple parameters it can quickly become confusing.

return Execute((connection, transaction) => connection.Insert(dataTransferObject,transaction));

Shows the exact purpose of your lambda much more clearly.

I also note that you have a method called GetList that does not return a List. This is just plain confusing.

Lastly, you should avoid putting the type of the variable in its name.

E.g.

public void Insert<T>(List<T> dtoList)

would be better as:

public void Insert<T>(List<T> dtos)

Otherwise it can lead to confusion should you decide to change it to a different type, and forget to update the variable name.

var

You have used var appropriately and consistently, making this (as far as I'm aware) the first code review I've ever posted that does not contain some guidance on var. Well done.

Parameter types

You take a List with this method:

public void Insert<T>(List<T> dtoList) where T : class
{
    IEnumerable<T> enu = dtoList;
    // yadda yadda yadda...

And then immediately convert it to an IEnumerable in your body. Just take the parameter as IEnumerable in the first place, passing a List in will still work. By requiring a List you're actually making it harder to use your own function (because you can't, for example, pass in an array) for no reason at all.


A generic "manual" loop unroller I want to force my compiler (C++ or perhaps CUDA) to unroll certain loops that it would rather not unroll. So, I've written the following:

template <size_t N> struct unroller {
    template <typename ForwardIterator, typename Function>
    static void unroll(const Function& f, ForwardIterator& it) {
        unroll<N/2, ForwardIterator, Function>(f, it);
        ++it;
        unroll<N-N/2, ForwardIterator, Function>(f, it);
    }
};
template <> struct unroller<0> {
    template <typename ForwardIterator, typename Function>
    static void unroll(const Function& f, ForwardIterator& it) { }
};

template <> struct unroller<1> {
    template <typename ForwardIterator, typename Function>
    static void unroll(const Function& f, ForwardIterator& it) { f(it); }
};

I can also manually write in the instances at sizes 2,3,4 or so to perhaps reduce the amount of work the compiler needs to do. However, I was thinking maybe I should be doing something involving template packs and index lists to really cut down on the amount of instantiation. What do you think?

Sample usage:

#include <iostream>

// unroller code here

int main()
{
    constexpr unsigned unroll_factor = 3;
    constexpr unsigned total_iterations = 12;

    static_assert(total_iterations % unroll_factor == 0,
        "Unroll factor must divide the number of iterations");

    auto f = [](int n) { std::cout << n << ' ' << std::flush; };
    std::cout << "no manual unrolling: ";
    for(int i = 0; i < total_iterations; i++) { f(i); }
    std::cout << std::endl << "manual unrolling x " << unroll_factor << ": ";
    for(int i = 0; i < total_iterations; i++) {
        unroller<unroll_factor>::unroll(f, i);
    }
    std::cout << std::endl;
    return 0;
}

First, there are some errors that need to be fixed in order for your code to compile:

size_t needs to be std::size_t, or alternatively there needs to be an using std::size_t; declaration.
no matching function for call; you need to specify the template instantiation of the structure based on the "depth" as well as the function:

// WRONG:
unroll<N/2, ForwardIterator, Function>(f, it);
// FIXED:
unroller<N/2>::template unroll<ForwardIterator, Function>(f, it);

From a style perspective, I'd change the order of the template parameters to reflect the order in which the typenames appear in the parameter list (first the Function, then the ForwardIterator).

Then there's one thing that's a bit odd: Functions in the standard library which accept function objects (like std::sort, std::transform, ...) generally accept those function objects by value. This is done (probably) because that allows those functions objects to have mutable state. Your unroller accepts only references to constant function objects. That makes it (a bit) harder to have mutable state in the function object, like when you get one returned by some function:

auto make_that_function_object() {
    bool flag = true;
    return [flag](int n) mutable {
        if (flag) std::cout << n << ' ' << std::flush;
        flag = !flag; };
}

The lambda returned by above function cannot be used with your unroller. To fix it (as a user of your unroller), you need another (non-mutable) lambda which references the original one:

auto original = make_that_function_object();
auto wrapper = [&original](int n) { original(n); };

You could also solve this by accepting non-const references, though I'm not sure whether that's really better. (Accepting the function object by value does not solve the issue because then any changes to the internal state cannot be transferred "back up" the call chain or past "major" iterations of the top level loop.)


Calculating Julian dates Refactor this code so that I don't have to insert the parameters any time I need to use one of the functions.

I have a class, say julian, for calculating Julian dates of today, a date I insert and the difference between these two Julian dates.

        int julian::calc_julianToday()
        {
            time_t t = time(0);
            struct tm *now = localtime( &t );
                todDay = now->tm_mday;
                todMonth = now->tm_mon+1;
                todYear = now->tm_year+1900;

              int at = (14 - todMonth) / 12;
              int yt = todYear + 4800 - at;
              int mt = todMonth + 12 * at - 3;
              if (todYear > 1582 || (todYear == 1582 && todMonth > 10) ||
    (todYear == 1582 && todMonth == 10 && todDay >= 15))
                return julToday =  todDay + (153 * mt + 2) / 5 + 365 * yt
    + yt / 4 - yt / 100 + yt / 400 - 32045;
              else
                return julToday = todDay + (153 * mt + 2) / 5 + 365 * yt + yt / 4 - 32083;

        }
        int julian::calc_juliandate(int day, int month, int year)
        {

              int a = (14 - month) / 12;
              int y = year + 4800 - a;
              int m = month + 12 * a - 3;
              if (year > 1582 || (year == 1582 && month > 10) ||
(year == 1582 && month == 10 && day >= 15))
                return julStart = day + (153 * m + 2) / 5
    + 365 * y + y / 4 - y / 100 + y / 400 - 32045;
              else
                return julStart = day + (153 * m + 2) / 5 + 365 * y + y / 4 - 32083;

        }

        int julian::dates_diff(int day, int month, int year)
        {
            int start = calcJulStartDate(day, month, year);
            int today = calcJulTodayDate();
            differ = today-start;
            return differ;
        }

Please note that all the variable types are declared in a header file. What I would like to do in main() is to be able to use calc_juliandate() without having to call it and pass its attributes each and every time I need it in another function, as in dates_diff().
Any suggestions about coding style or implementing this?

Welcome to Code Review,

I do not know how to solve your problem, however, your code could use polishing.

Magical constants; your code is littered with them, use constants instead
Undocumented magical constants, I can guess what 365 or 12 is used for, other constants like 32083 could use a comment or could show the calculation of how you got there
Naming; todDay, todMonth and todYear are terrible names
Naming; at, ty and mt are even worse
Casing; be consistent, you mix underscores with casing and even your casing is not consistent ( calc_julianToday <> calc_juliandate )
Indenting! Your indenting is all over the place and distracts the reader
Repeating / DRY;

I do not write C++ but can you not write calc_julianToday() as

int julian::calc_julianToday()
{
  time_t t = time(0);
  struct tm *now = localtime( &t );
  return julToday =  calc_juliandate( now->tm_mday, now->tm_mon+1, now->tm_year+1900 ) ;
}


Print Z pattern from 2D String array I am trying to print the Z pattern, but the time complexity is coming to \$O(n^2)\$. Is there any way to reduce the time complexity?

package matrix;

public class ZPattern
{
    public static void main(String[] args) {
        String a[][] = { {"1","2","3","4","5"},
                {"a","b","c","d","e"},
                {"6","7","8","9", "f"},
                {"g","h","i","j","k"},
                {"m","n","o","p","q"}
            };
        for(int i=0;i<a.length;i++)
        {
            for(int j=0;j<a.length;j++)
            {
                if(i==0||i==a.length-1)
                {
                    System.out.print(a[i][j]);
                }
                else if(i+j==a.length-1)
                {
                    System.out.print(a[i][j-1]);
                }
                else
                {
                    System.out.print(" ");
                }
            }
            System.out.println("");
        }
    }
}

Bug?

The output from your version:

12345
   c
  7
 g
mnopq

But the output should be

12345
   d
  8
 h
mnopq

It's unclear why you chose this.

                    System.out.print(a[i][j-1]);

This could be

                    System.out.print(a[i][j]);

Then it prints the output correctly.

Avoid unnecessary comparisons in loops

You iterate over each element in the entire square and print different things based on an if/else structure.  But you don't actually have to do that.  Consider

    public static void drawZ(String[][] matrix) {
        System.out.println(String.join("", matrix[0]));

        int n = matrix.length - 1;
        for (int i = 1; i < n; i++)
        {
            int m = matrix.length - i - 1;
            for (int j = m; j > 0; j--)
            {
                System.out.print(" ");
            }

            System.out.println(matrix[i][m]);
        }

        System.out.println(String.join("", matrix[n]));
    }

This is in a separate method, which makes it easy to reuse.

This prints the first and last line separately.  So we don't need that part of the if.

This prints the spaces separately from the line of the Z.  So we don't need those parts of the if either.

Rather than calculating i + j - a.length - 1 on every iteration, this code just counts down from m, which it calculates once per row.

This calls a Java 8 built-in method to join the characters in a row rather than rolling our own for loop.

This prints the equivalent part of the matrix.  If that's not what you want, you can replace [m] with [m - 1].  That would give the original behavior.


Improving Design for structured Keys On several occasions I had to use a fixed set of keys (constants in the sense of static final String) with some additional logic (grouping in different keySets). After some more or less lefthanded approaches with static final String constants, I learned the advantages of enum. Since everything is known from the start I tend to use enum. But I don't see how to make this approach reusable for different sets of keys (avoid copy paste). Here is the pattern I found unseful so far. Hope you have some ideas how to improve.

import java.util.Collection;
import java.util.Collections;
import java.util.HashSet;

public class TestKeys
{
    public enum Groups
    {
        BASIC,
        BROAD(BASIC), // Keys defined in group BASIC are also part of group BROAD
        SEPARATE;

        private Collection<Groups> m_subGroups = new HashSet<Groups>();
        private Collection<String> m_keySet = new HashSet<String>();

        private Groups()
        {
            m_subGroups.add(this);
        }

        private Groups(Groups... groups)
        {
            this();
            for (Groups group : groups)
            {
                m_subGroups.add(group);
            }
        }

        public boolean contains(String keyString)
        {
            return m_keySet.contains(keyString);
        }

        public boolean contains(Keys key)
        {
            return m_keySet.contains(key.toString());
        }

        public boolean containsIgnoreCase(String keyString)
        {
            for (String key : m_keySet)
            {
                if (key.equalsIgnoreCase(keyString)) return true;
            }
            return false;
        }
    }

    public enum Keys {
        KEY_BASE1("Base 1", Groups.BASIC),
        KEY_BASE2("Base 2", Groups.BASIC),
        KEY_BROAD1("Broad 1", Groups.BROAD),
        KEY_BROAD2("Broad 2", Groups.BROAD),
        KEY_DIFF1("Different 1", Groups.SEPARATE)
        ;

        private String m_str;
        private Groups m_group;

        Keys(String str, Groups group)
        {
            m_str = str;
            m_group = group;
            if (group == null)
            {
                throw new AssertionError("group mustn't be null");
            }
        }

        public Groups getGroup()
        {
            return m_group;
        }

        @Override
        public String toString()
        {
            return m_str;
        }

        public boolean equals(String string)
        {
            if (toString().equals(string)) return true;
            return false;
        }

        public Keys getKeyIgnoreCase(String keyString)
        {
            for (Keys key : Keys.values())
            {
                if (key.toString().equalsIgnoreCase(keyString)) return key;
            }
            return null;
        }
    }

    static
    {
        fillKeySets();
        integrityCheck();
    }

    public static void integrityCheck() throws AssertionError
    {
        for (Groups bereich : Groups.values())
        {
            checkMultipleKeys(bereich);
        }
    }

    public static void checkMultipleKeys(Groups bereich)
            throws AssertionError
    {
        HashSet<String> set = new HashSet<String>();
        for (Keys key : Keys.values())
        {
            if (set.contains(key.toString()))
            {
                throw new AssertionError("Multiple Key \"" + key + "\".");
            }
            set.add(key.toString());
        }
    }

    public static void fillKeySets()
    {
        for (Groups bereich : Groups.values())
        {
            for (Keys key : Keys.values())
            {
                if (bereich.m_subGroups.contains(key.m_group))
                {
                    bereich.m_keySet.add(key.toString());
                }
            }
            bereich.m_keySet = Collections.unmodifiableCollection(bereich.m_keySet);
        }
    }

    public static Collection<String> keySet(Groups group)
    {
        return group.m_keySet;
    }
}

My initial thought is that you don't :)

The whole idea behind the enum approach starts from your premise:

  Since everything is known from the start I tend to use enum

Which means you're dealing with a specific set with it's own rules and checks that you put into that enum.

If you were to start from the idea of reusing code to represent different kinds of sets you wouldn't go for enums (or static finals for that matter).
That requires a completetly different design.

In this case I'll point out Martin Fowler's rule of three.

More specifically, you should only consider refactoring this for reusability if you actually want to reuse it in the first place.

Also: copy-pasting some code isn't a deadly sin. It's only a bad smell, that you have to decide to remove or not.


INSERTing a student info record based on form inputs I want to improve my coding skills. How do I write this code with concatenation

string query = "insert into StudentInfo values ('" + textBox1.Text.Trim() + "', '" + Name.Trim() + "', '" + comboBox1.SelectedItem.ToString() + "') ";
SqlCommand command = new SqlCommand(query, cn);
command.ExecuteNonQuery();

Like I had mentioned in my comment this is vulnerable to sql injection. Assume the user of your application inserts Robert'; DROP TABLE StudentInfo;-- that would result in a query text of "insert into StudentInfo values ('Robert','',''); DROP TABLE StudentInfo;-- ', 'theName', 'theComboboxValue') " resulting in deleting your database table StudentInfo which isn't what you want !

The solution to this is using SqlParameter's which will escape the provided values in a way that this can't happen like so

string query = "insert into StudentInfo values (@first, @second, @third)";
SqlCommand command = new SqlCommand(query, cn);
command.Parameters.AddWithValue("@first", textBox1.Text.Trim());
command.Parameters.AddWithValue("@second", Name.Trim());
command.Parameters.AddWithValue("@third", comboBox1.SelectedItem.ToString());
command.ExecuteNonQuery();

I have named the parameters first, second and third, because I don't know what their meaning is, so you should give them proper names yourself. The @ needs to be at the beginning so that the names can be parsed to the desired parameters.

Naming is very important in programming, so you should always use proper names for anything you do. If you come back in 2 weeks to this piece of code you won't know for what e.g textBox1 or comboBox1 are standing.


Pig Latin game resolved with a regex in Java I recreated the Pig Latin game on Java. The program takes an arbitrary number of command line arguments as words, and prints to the standard output these words translated with following rules:

For words that begins with a consonants, all letters until first vowel are moved to the end, and it is added the sequence "ay";
For words that begins with a vowel, it is simply added the sequence "way".

About the implementation, I've used a regular expression to split the word. I think that it is an elegant solution, maybe less efficient against a specific implementation of the rules.

The code is composed only with a class, PigLatin. You can compile it with javac PigLatin.java and run with java PigLatin <word> .... Here there is PigLatin.java:

/**
 * This small program implements the game Pig Latin. You can specify one or more
 * words as a command line argument, and the program will print the modified
 * version on the standard output.
 *
 * The rules of the game are:
 *      - For words that begin with a consonant, all letters before the first
 *        vowel are placed at the end of the word, then the sequence "ay" is
 *        appended.
 *      - For words that begin with a vowel, it is appended the sequence "way".
 *
 * Note that command line arguments are limited.
 *
 * Note that you should only use uppercase or lowercase letters.
 */
public class PigLatin {
    public static void main(String[] args) {
        for (int i = 0; i < args.length; i++) {
            System.out.print(convertWord(args[i]));

            if (i < args.length - 1)
                System.out.print(" ");
            else
                System.out.println();
        }
    }

    /*
     * This method convert a word using Pig Latin rules.
     *
     * @param word the word to convert
     *
     * @return the converted word
     */
    private static String convertWord(String word) {
        /* If the word begins with a consonant (or more), this regex splits the
         * word into two groups: the first are the consonants before the first
         * vowel, the second is the rest of the string.
         * If the word begins with a vowel, only a single group is returned.
         */
        String[] tokens = word.split("^([^aeiouAEIOU]*)(\\w*)");

        if (tokens.length == 2)
            return String.join(tokens[1], tokens[0], "ay");
        else
            return word + "way";
    }
}

Your code looks fine. I would write this with a different approach. It is up to you, which is better.

public static String pigLatin(String word) {
    Pattern firstVowelPattern = Pattern.compile("^([aeiouAEIOU])");
    Matcher matcher = firstVowelPattern.matcher(word);
    if (matcher.find()) return word + "way";
    return word.replaceAll("^([^aeiouAEIOU]*)(.+)", "$2$1ay");
}

I do not use any arrays and splitting a string, but I solve it with more regex expressions.  The key element of this solution is how replaceAll works. In the first parameter, I pass two regex expression and in second parameter I switch results of them using $2$1.

As you can see I did not use if-else statement, only if. In your solution else also is redundant.


Print numbers in a concentric square pattern I am trying to solve a programming problem on a coding platform. When I execute it on my PC, it works perfectly, but when I submit the code on the coding platform, it throws a "Time Limit Exceeded" error. Can someone check my solution and help optimize it?

Given a number N, the program must print the pattern as described below.
Input Format:The first line contains the value of the N which represent the number N.
Output Format:The pattern as described below in the Example.

> Input: 4

> Output:
4444444
4333334
4322234
4321234
4322234
4333334
4444444

My Approach
Print Upper rectangle and then print the lower rectangle.
The problem statement and my code can also be found here.

import java.util.Scanner;

public class Test004_3 {
    public static void main(String[] args) {
        Scanner sc = new Scanner(System.in);
        int N = sc.nextInt();
        /*  PRINT UPPER PART
            4444444
            4333334
            4322234
            4321234
        */
        for(int i = N; i>(1); i--)
        {
            for(int j = N; j >i; j--)
                System.out.print(j);
            for(int j = 0; j<(i*2)-1; j++)
               System.out.print(i);
            for(int j = i+1; j<=N; j++)
                System.out.print(j);
            System.out.println("");
        }
        /*
            PRINT LOWER PART
            4322234
            4333334
            4444444
        */
        for(int i = 1; i<=(N); i++)
        {
            for(int j = N; j>i; j--)
                System.out.print(j);
            for(int j = 0; j<(i*2)-1; j++)
                System.out.print(i);
            for(int j = i+1; j <=N; j++)
                System.out.print(j);
            System.out.println("");
        }
    }
}

As has been said by @200_success, outputting individual characters at a time to the console is going to be relatively slow.  You'd be better off building up the string using a StringBuilder and then writing the whole thing at the end.

Assuming a maximum number of 9, you can predict the size of the required buffer which will prevent the buffer from needing to resize.  A safe value would be:

StringBuilder sb = new StringBuilder(N*2*N*2)

This number will need to be bigger if you have to support multiple digit numbers, however the StringBuilder will scale this for you if required.

Instead of calling System.out.print, you then append to the StringBuilder:

sb.append(j);

Adding new lines when required:

sb.append("\n");

And at the end, write out the entire buffer in one go:

System.out.print(sb);

Putting it together

public static void main(String[] args) {
    Scanner sc = new Scanner(System.in);
    int N = sc.nextInt();
    StringBuilder sb = new StringBuilder(N*2*N*2);
    for(int i = N; i>(1); i--) //Print upper part
    {
        for(int j = N; j >i; j--)
            sb.append(j);
        for(int j = 0; j<(i*2)-1; j++)
           sb.append(i);
        for(int j = i+1; j<=N; j++)
            sb.append(j);
        sb.append("\n");
    }
    for(int i = 1; i<=(N); i++)// print lower part
    {
        for(int j = N; j>i; j--)
            sb.append(j);
        for(int j = 0; j<(i*2)-1; j++)
            sb.append(i);
        for(int j = i+1; j <=N; j++)
            sb.append(j);
        sb.append("\n");
    }
    System.out.print(sb);
}


Java Singly Linked List Implementation I've implemented a singly linked list in Java as a practice exercise and would appreciate a review.

LinkedList.java

public class LinkedList {

    private Node head;
    private int size;

    public LinkedList() {
    }

    public void addToEnd(char data) {
        addAtIndex(data, size);
    }

    public void addToBeginning(char data) {
        addAtIndex(data, 0);
    }

    public void addAtIndex(char data, int index) {
        Node temp = new Node(data);
        if (index < 0 || index > size) {
            throw new IndexOutOfBoundsException();
        } else if (index == 0) {
            temp.setNextNode(head);
            head = temp;
            incrementCount();
        } else {
            Node current = head;
            for (int i = 1; i < index; i++) {
                current = current.getNextNode();
            }
            temp.setNextNode(current.getNextNode());
            current.setNextNode(temp);
            incrementCount();
        }
    }

    public void deleteFromBeginning() {
        deleteFromIndex(0);
    }

    public void deleteFromEnd() {
        deleteFromIndex(size - 1);
    }

    public void deleteFromIndex(int index) {
        if (index < 0 || index >= size) {
            throw new IndexOutOfBoundsException();
        } else if (index == 0) {
            head = head.getNextNode();
            decrementCount();
        } else {
            Node current = head;
            for (int i = 1; i < index; i++) {
                current = current.getNextNode();
            }
            current.setNextNode(current.getNextNode().getNextNode());
            decrementCount();
        }
    }

    public char getFirst() {
        return getAtIndex(0);
    }

    public char getLast() {
        return getAtIndex(size - 1);
    }

    public char getAtIndex(int index) {
        if (index < 0 || index >= size) {
            throw new IndexOutOfBoundsException();
        } else {
            Node current = head;
            for (int i = 0; i < index && current != null; i++) {
                current = current.getNextNode();
            }
            return current.getData();
        }
    }

    public void decrementCount() {
        size--;
    }

    public void incrementCount() {
        size++;
    }

    public int getSize() {
        return size;
    }

    public Node getHead() {
        return head;
    }
}

Node.java

public class Node {

    private char data;
    private Node next;

    public Node(char data) {
        this.data = data;
    }

    public char getData() {
        return data;
    }

    public Node getNextNode() {
        return next;
    }

    public void setData(char data) {
        this.data = data;
    }

    public void setNextNode(Node next) {
        this.next = next;
    }
}

Your code seems good to me. It's readable, well structured and formatted. So just a couple of minor points:

When implementing a list, it's always a good idea to implement the List interface, as it's well thought out.

The main differences to your interface I am seeing:

naming: your method names contain an xIndex at the end. This isn't really needed, as the argument name index already suggests this.
extra methods: you have a lot of methods which deal with the corner cases of other methods. So eg you have deleteFromBeginning, deleteFromEnd, and deleteFromIndex (same for add and get). This doesn't really seem necessary. List for example only has this for add, as adding happens quite often. It doesn't happen so often to want to delete from the beginning/end though.
extra public methods: you also have some methods which are just not needed. decrementCount isn't needed and will cause problems when used, as the size will be wrong then. getHead is also not needed, it just exposes Node, which isn't really needed outside of the list context. Just make these private.
you are missing some useful methods and your list is not generic, I'm assuming that both are on purpose as this is just for practice.

Misc

Node should be a private class of LinkedList, as it's not needed outside of the list at all.
I would use this for fields. It's a question of preference, but I prefer it as it makes it explicit where the variable comes from.
initialize size explicitly to 0 in the constructor.


Get a grouped list with relational data I am new to C# and am leaning ASP.NET MVC and Entity Framework and I am trying to create an application where a user can manager Job Postings and Applicants where an Applicant can apply for many Jobs.

I am trying to pull a list of Applicants, along with a list of the Jobs they have applied for.

The following is the solution I have come up with.

HomeViewModel.cs

public class HomeViewModel
{
    public IEnumerable<ApplicantList> Applicants { get; set; }
    public IEnumerable<Jobs> Jobs { get; set; }
}

public class ApplicantList
{
    public Applicants Applicant { get; set; }
    public List<Jobs> Jobs { get; set; }
}

HomeController.cs

using(var context = new HR())
{
    HomeViewModel viewModel = new HomeViewModel();

    // Get grouped applicants list by Email Address
    var applicants = context.Applicants.GroupBy(a => a.EmailAddress)
                             .Select(grp=>grp.FirstOrDefault())
                             .ToList() ;
    List<ApplicantList> applicantList = new List<ApplicantList>();

    foreach (var a in applicants)
    {
        //get Jobs for each applicants
        var query = from dba in context.Applicants.Include("Jobs")
                    where a.EmailAddress == dba.EmailAddress
                    select dba.Jobs;
        List<Jobs> jobs = query.ToList();
        applicantList.Add(
            new ApplicantList (){ Applicant = a, Jobs = jobs }
        );
    }

    viewModel.Applicants = applicantList;

    viewModel.Jobs = context.Jobs.ToList();

    return View(viewModel);
}

HomeView.cshtml

@foreach (var applicant in Model.Applicants)
{
    Applicant name: @applicant.Applicant.FirstName

            @foreach(var job in applicant.Jobs)
            {
               * @job.JobTitle

            }

}

Result in View

Applicant name: Bob Lois

Job Title 1
Job Title 2

Applicant name: Kevin Bacon

Job Title 1

It works, but it's horrible code produced by a beginner like me.I want to know how I can achieve the same result in a much more eloquent way.

I would recommend moving the database access stuff from your "View Controller" and into more of a data access controller (think DAL).

Also, it's been a bit since I've used Entity Framework, but if I remember right, you can take the following code:

var applicants = context.Applicants.GroupBy(a => a.EmailAddress)
                                 .Select(grp=>grp.FirstOrDefault())
                                 .ToList() ;
List<ApplicantList> applicantList = new List<ApplicantList>();

foreach (var a in applicants)
{
   //get Jobs for each applicants
   var query = from dba in context.Applicants.Include("Jobs")
                        where a.EmailAddress == dba.EmailAddress
                        select dba.Jobs;
   List<Jobs> jobs = query.ToList();
   applicantList.Add(new ApplicantList (){ Applicant = a, Jobs = jobs });
}

viewModel.Applicants = applicantList;

viewModel.Jobs = context.Jobs.ToList();

And replace it with:

var applicants = context.Applicants.Include("Jobs").ToList();

If Entity Framework is set up right, it should pull in the Jobs that are tied to each applicant automatically.  It is highly recommended to create DTOs based off of your Entity Framework entities though, so you can cut off any circular reference pitfalls (in this case, make sure that the Jobs object doesn't reference the Applicants object).

For the most part though, this doesn't look as bad as a lot of the code that I have run into that has made it into actual production environments.


Finding all legal bishop moves I am trying to create a two player Chess game. To do this I want to generate a list of legal moves. I am having trouble with the brevity of the code to generate the moves for the rooks and bishops.

The code I have pasted below works (i.e. shows legal bishop moves) but there is a lot of repetition, which is not ideal. To make this work for both black and white bishops I would need to add even more repetitive lines.

Is there a better way to do this? I do not want to use any 'chess modules' as this is a learning exercise for me; as you can tell, I am a beginner.

import numpy as np

# chess board is a numpy array
# white pieces are capitalised, black pieces are not
board = np.zeros((8,8),dtype=str)

# example piece positions
white_bishop = (4,5)
white_pawn = (6,7)
black_knight = (0,1)
board[white_bishop] = 'B'
board[white_pawn] = 'P'
board[black_knight] = 'n'

def bishop(insquare):
    '''
    Gets all legal moves for bishop on insquare
    output is a list (outsquares) of indices of possible squares
    '''
    out_squares = []
    y,x = insquare

    y1 = y-1 # these are steps in one of the four directions bishops can go
    x1 = x-1
    while (x1 > -1) and (y1 > -1): # edge of the board
        if not board[(y1,x1)].isupper(): # if not a white piece...
            out_squares.append((y1,x1)) # square is legal
            if board[(y1,x1)].islower(): # if black piece...
                y1 = -1 # look no further in this direction
        else: # if white piece...
            y1 = -1 # look no further in this direction
        y1 -= 1 # check next square
        x1 -= 1

    # check all other directions
    y1 = y+1
    x1 = x+1
    while (x1 < 8) and (y1 < 8):
        if not board[(y1,x1)].isupper():
            out_squares.append((y1,x1))
            if board[(y1,x1)].islower():
                y1 = 8
        else:
            y1 = 8
        y1 += 1
        x1 += 1

    y1 = y + 1
    x1 = x - 1
    while (x1 > -1) and (y1 < 8):
        if not board[(y1,x1)].isupper():
            out_squares.append((y1,x1))
            if board[(y1,x1)].islower():
                y1 = 8
        else:
            y1 = 8
        y1 += 1
        x1 -= 1

    y1 = y - 1
    x1 = x + 1
    while (x1 < 8) and (y1 > -1):
        if not board[(y1,x1)].isupper():
            out_squares.append((y1,x1))
            if board[(y1,x1)].islower():
                y1 = -1
        else:
            y1 = -1
        y1 = y1 - 1
        x1 = x1 + 1
    return out_squares

# Get all legal moves for the white bishop
print bishop((4,5))
# >>> [(3, 4), (2, 3), (1, 2), (5, 6), (5, 4), (6, 3), (7, 2), (3, 6), (2, 7)]

When you find yourself having a lot of duplicate code, you should factor out the commonalities. In this case, we have four separate directions we need to consider. So let's write one function that considers one direction:

def append(dx, dy):
    for i in itertools.count(start=1):
        newx = x + dx*i
        newy = y + dy*i
        sq = (newy, newx)

        if 0 <= newx < 8 and 0 <= newy < 8:
            if not board[sq].isupper():
                out_squares.append(sq)
                if board[sq].islower():
                    break
            else:
                break
        else:
            break

And then just call it 4 times:

for dx in (-1, 1):
    for dy in (-1, 1):
        append(dx, dy)

Full solution:

def bishop(insquare):
    '''
    Gets all legal moves for bishop on insquare
    output is a list (outsquares) of indices of possible squares
    '''
    out_squares = []
    y,x = insquare

    def append(dx, dy):
        for i in itertools.count(start=1):
            newx = x + dx*i
            newy = y + dy*i

            if 0 <= newx < 8 and 0 <= newy < 8:
                sq = (newy, newx)
                if not board[sq].isupper():
                    out_squares.append(sq)
                    if board[sq].islower():
                        break
                else:
                    break
            else:
                break

    for dx in (-1, 1):
        for dy in (-1, 1):
            append(dx, dy)

    return out_squares