User input for student grades is too verbose I need some help with this beginner C++ program (it's an assignment). How can I use a single variable and make the program get the six inputs I need instead of using a variable for each input?

#include <iostream>
using namespace std;

int main()

{

double grade1,grade2,grade3,grade4,grade5,grade6;
double totalGrade;
cout << "program to calculate grade" << endl << endl;
cout << "hi please enter the six grades ";
     << endl << endl;

cin >> grade1;
cin >> grade2;
cin >> grade3;
cin >> grade4;
cin >> grade5;
cin >> grade6;

grade1 = grade1 * 0.125;
grade2 = grade2 * 0.125;
grade3 = grade3 * 0.05;
grade4 = grade4 * 0.05;
grade5 = grade5 * 0.05;
grade6 = grade6 * 0.1;

totalGrade = grade1 + grade2 + grade3 + grade4 + grade5 + grade6;

if((totalGrade >= 9.5) && (totalGrade <= 10)) {
    cout << "grade is: >> " << totalGrade << endl << endl;
    cout << "you have approved + " << endl;
    }
else if ((totalGrade >= 9) && (totalGrade <= 9.5)) {
    cout << "grade is >> " << totalGrade << endl << endl;
    cout << "you have approved -" << endl;
    }
else if (totalGrade < 9) {
    cout << "grade is: >> " << totalGrade << endl << endl;
    cout << "you have reproved  "<< endl;

    }

return 0;
}

You should move most of the logic out of the main() function and into its own function or functions.  But perhaps you're not there yet in your class.

Similarly, using namespace std is not recommended.  See here.  Obviously this won't matter in this program, but it's a bad habit that you may want to avoid starting.

double grade1,grade2,grade3,grade4,grade5,grade6;

Whenever I see something like this, I want to rewrite it as an array:

const int NUMBER_OF_GRADES = 6;
double grades[NUMBER_OF_GRADES];

Personally, I'd also put the grade weights into their own array, but that's up to you.  You could also use a switch statement or an if/else if construct to handle the grade weights.

If you use a for loop to iterate over the entries of the array and calculate the totalGrade incrementally in each iteration over the loop, I think that you will be able to see how to use a single grade variable to replace the entries of the array.  If your class hasn't done for loops yet, you can get the same effect by ordering all uses of grade[0] or grade1 before you do anything with grade[1] or grade2.  This means that you will have to replace

totalGrade = grade1 + grade2 + grade3 + grade4 + grade5 + grade6;

with six separate statements that have the same effect.

As a matter of policy, you should probably do more error checking when you do a cin.  If you look around this site, you should be able to find several examples of people using while loops with error checking.

When I've seen people put the } and else on separate lines, they've always been in the same column.  Like this:

if (totalGrade < 9) {
    cout << "grade is: >> " << totalGrade << endl << endl;
    cout << "you have reproved  "<< endl;
}
else if (totalGrade < 9.5) {
    // the else makes this implicitly greater than or equal to 9
    cout << "grade is >> " << totalGrade << endl << endl;
    cout << "you have approved -" << endl;
}
else if (totalGrade <= 10) {
    // the else makes this implicitly greater than or equal to 9.5
    cout << "grade is: >> " << totalGrade << endl << endl;
    cout << "you have approved + " << endl;
}

I personally put them on the same line.  Putting them on separate lines leads to people separating them, which can lead to confusion.

Note that I changed the order.  This makes no functional difference, but it reduces the number of comparisons that you need to do.  Also, I'm not sure that you have to check that totalGrade <= 10.  A simple else might be enough for the last one.

You might want to think about why I changed <= 9.5 to < 9.5.  Hint:  I had to do that to keep the effect the same after I changed the order.

cout << "hi please enter the six grades ";
     << endl << endl;

This looks horribly wrong.  It may work but it is fragile.  Either remove the ; from the first line or add a cout to the beginning of the second line.


An abundance of ternary operators I'd like to find a way to do what this method does in a cleaner, less hacky looking way

 def self.create_from_person!(person)
    spi = new(:person => person, :provider => person.provider)
    spi.age_eligible_code = participant.age_eligible?(person) ? 1 : 2
    spi.county_of_residence_code = participant.psu_county_eligible?(person) ? 1 : 2
    spi.first_prenatal_visit_code = participant.first_visit?(person) ? 1 : 2
    spi.pregnancy_eligible_code = participant.pbs_pregnant?(person) ? 1 : 2
    spi.save!
  end

Any thoughts?

Notes:

Any reason not to set the attributes in a single create step?
DRY by using procs for short-lived functions.
Methods should return meaningful values. Here it makes sense to return the newly created object.
(As Mark pointed out) The mismatch names between attributes makes the code more verbose that it should be. However, I guess part of this mismatch already comes from the app you are patching.
:provider => person.provider. Note that this can break referential integrity. Person may change its provider in the future, but this model will remain as it is.

I'd write:

def self.create_from_person!(person)
  code = lambda { |value| value ? 1 : 2 }
  create!({
    :person => person,
    :provider => person.provider,
    :age_eligible_code => code[participant.age_eligible?(person)],
    :county_of_residence_code => code[participant.psu_county_eligible?(person)],
    :first_prenatal_visit_code => code[participant.first_visit?(person)],
    :pregnancy_eligible_code => code[participant.pbs_pregnant?(person)],
  })
end


Simple JavaScript router with params I am building a JS router and would like to have some help doing it.  It will work like so:

A list of routes is given with a URL pattern attached in each one.

[
  { url : "/home"},
  { url : "/user/:id"}
]

I want a function that will find the appropriate pattern when we pass it a route URL and will return the object of parameters:

getRouteParams("/home");    // {params: {}}
getRouteParams("/user/42"); // {params: {id: "42"}}
getRouteParams("/contact"); // null

I tried to do something with some parts of code I have found on the web and here is what I ended up with:

var routes = [
  { url : "/home"},
  { url : "/user/:id"}
];

function getRouteParams(url) {
  var argsVal = null;
  var x = 0;
  var result = {};
  for(; routes.length; x++){
    var routeMatcher = new RegExp(routes[x].url.replace(/(:\w+)/g, '([\\w-]+)'));
    argsVal = url.match(routeMatcher);
    if(argsVal) {
      argsVal.shift();
      result.params = makeObj(argsVal, routes[x].url);
      break;
    }
  }
  return result;

  function makeObj(vals, url) {
    var routeParts = url.split('/');
    var options = {};
    for(var i=0, j=0; i<routeParts.length; i++) {
      if(routeParts[i].indexOf(":") !== -1) {
        options[routeParts[i].slice(1)] = vals[j++];
      }
    }
    return options;
  }
}

console.log(getRouteParams("/home"));    // {params: {}}
console.log(getRouteParams("/user/42")); // {params: {id: "42"}}
console.log(getRouteParams("/contact")); // null)

This is a bit more sophisticated but the idea is there. Could someone help me improve on what I did?

Your algorithm seems to be working.

Here is the few things I would do to improve it:

// The routes should be a parameter of the function
function getParams(routes, url) {
  var params = null;

  // Depending on the targeted browser, you could use the native forEach method
  routes.forEach(function (route) {
    // I personally prefer to use methods in here
    if (params === null && routeMatch(route.url, url)) {
      params = computeParams(route.url, url);
    }
  });

  return { params: params };

  function routeMatch(route, url) {
    // You don't need to use `new RegExp`
    // I've added ^ and $ to be sure that it would be working with nested routes
    var matcher = '^' + route.replace(/(:\w+)/g, '([\\w-]+)') + '$';
    return url.match(matcher);
  }

  function computeParams(route, url) {
    var routeParts = route.split('/');
    var urlParts = url.split('/');
    var options = {};

    for (var i = 0, nbOfParts = routeParts.length; i < nbOfParts; i++) {
      // I check that the url part exists
      // and use the binary operator ~ that simplifies the `indexOf` method
      if (urlParts[i] && ~routeParts[i].indexOf(':')) {
        options[routeParts[i].slice(1)] = urlParts[i];
      }
    }

    return options;
  }
}

var routes = [ { url: '/home' }, { url: '/user/:id' }, { url: '/user/:id/post/:postId' }, { url: '/user/:id/post/:postId/comment/:cId' } ];

printResult(getParams(routes, "/home").params, {});
printResult(getParams(routes, "/user/42").params, { id: '42' });
printResult(getParams(routes, "/user/42/post/52").params, { id: '42', postId: '52' });
printResult(getParams(routes, "/user/42/post/52/").params, null);
printResult(getParams(routes, "/user/42/post/52/comment/").params, null);
printResult(getParams(routes, "/user/42/post/52/comment/yolo-22").params, { id: '42', postId: '52', cId: 'yolo-22' });
printResult(getParams(routes, "/contact").params, {});

function printResult(result, expected) {
  console.log('expected: ', expected, 'result: ', result);
}

That's all I can think of.


Is there a PHP security exploit with $_POST in my code? I posted this question here.

And an answer stated that I should not do:

$table_name = 'survey_'.$_POST['surveyid'];

because

  It is easy for a hacker to exploit your site if you include $_GET or $_POST data directly in any SQL string.

Here is the code.  Do you see any security exploits?

if(ctype_digit($_POST['surveyid']) && $_POST['surveyid']>0){

    $table_name = 'survey_'.$_POST['surveyid'];

    $query = 'CREATE TABLE '.$table_name.' (
            `responseid` INT NOT NULL AUTO_INCREMENT,
            `textarea1` TEXT NULL,
            `textarea2` TEXT NULL,
            `textarea3` VARCHAR(255) NULL,
            `drop_down1` VARCHAR(255) NULL,
            `drop_down2` VARCHAR(255) NULL,
            `bool1` BIT NULL,
            `bool2` BIT NULL,
        PRIMARY KEY (`responseid`))';
}

I don't see a vulnerability.... why is $_POST['surveyid'] vulnerable? It is being sanitized by ctype_digit...

Since you validate that $_POST['surveyid'] contains at least one digit and contains only digits, your query is safe.

However, the CREATE TABLE operation that you are trying to do strikes me as a horrible thing to do.  CREATE TABLE is a Data Definition Language operation, and DDL commands should be executed only in special situations.

Basically, if you routinely create a new table to store responses from each survey, your database schema will be an unmaintainable mess.  I strongly recommend that you post your database schema and describe what you are trying to do in a question to http://dba.stackexchange.com to develop a sane schema that does not require new tables to be created routinely.


Writing numbers into a file I'm taking my first ever CS class and I have an assignment due Friday. I just wanted someone to check my code.

Instructions:

  Write a program that:

  gets the name of a text file of numbers from the user. Each number in the file is on its own line.
  reads in those numbers one at a time
  writes the even numbers to a file named even.txt
  writes the odd numbers to a file named odd.txt
  displays to the user the sum of the positive numbers and the count of the negative numbers.

Here is what I have:

def main():
    #Open text file for reading
    fileName   = input('Enter name of a file of numbers (include the whole path): ')
    numberFile = open(fileName, 'r')

#Priming read
    number = numberFile.readline()

#Setting up loop to continue reading until
#an empty line is reached
    total = 0
    count = 0
    while number != '':
        number = float(number)                  #convert from string to number
        if number%2 == 0:
            evenNumber = open('even.txt', 'w')  #writes even numbers into a file
            evenNumber.write(number + '\n')
        else:
            oddNumber  = open('odd.txt', 'w')   #writes odd numbers into a file
            oddNumber.write(number + '\n')

        for number in numberFile:
            number = float(number)      #convert from string to number

            if number <= 0:             #identify negative numbers
                count += 1              #count negative numbers

            if number >= 0:             #identify positive numbers
                total += number         #sum of positive numbers
        number = numberFile.readline()

    numberFile.close()                  #close file after program is complete

    print('The count of the negative numbers is: ', count)
    print('The sum of the positive numbers is: ', total)

main()

Example of a "file of numbers" (each number in a new line):

25
18
-7
4.2
99
103
-100

Some quick comments:

At least as written, your code doesn’t work for me. I created the following file:

1

and passing it to your script got the following error:

Enter name of a file of numbers (include the whole path): numbers2.txt
Traceback (most recent call last):
  File "parsenumbers.py", line 37, in <module>
    main()
  File "parsenumbers.py", line 20, in main
    oddNumber.write(number + '\n')
TypeError: unsupported operand type(s) for +: 'float' and 'str'

I’m using Python 3.4.3 on OS X.
I fixed that (fix hidden for you to work it out yourself):

   Cast number to str(number) before writing.

Then I created a longer file:

1
2
3
4
5

And ran the script again.  Still broken:

# odd.txt
1.0

# even.txt
(Empty)

It’s also strange that 1 has become 1.0 in the output file. I’d expect it to be carried through unchanged – what’s happened?

Once you’ve got the code working, some style comments:
* Run your code through a PEP 8 linter. This will catch style issues like variable names, whitespace, etc.

Everything is wrapped up in one massive main() function. That makes this code difficult to test and reuse – it would be better if you had separate functions, each of which took a list of numbers, and would do different things.

e.g. one function gets the sum of the negatives, another the positives, another to write the evens to a file.
Rather than calling main() at the top-level, put it in an if __name__ == '__main__' block. This means it only runs if the script is called directly – and other people can import from your script without side effects.
Use with open(filename) as f:, not f = open(filename) … f.close(). This is more idiomatic, and safer – you can’t leave files open accidentally. (You never actually close even.txt or odd.txt after you’ve opened them!)
None of your comments tell me anything I couldn’t get from reading the code.  They should explain why the code is written this way – how does it relate to the problem it’s trying to solve – not what it’s doing.  And as a sidebar, it’s really annoying if comments don’t line up with the code they’re describing.
Use better variable names. You have count and total, but it’s not obvious what these correspond to.  How about negative_count and positive_total – otherwise it sounds like you’re getting a count/total for the entire set.
Your code assumes that zero is negative, and will count it as such. That’s somewhat counter-intuitive: instinct tells me that zero is neither positive nor negative, but there are times when it’s more complicated.

This would be a really good spot for a comment: if it’s not completely ambiguous, state what choice you’re making (in plain English), and why you’ve chosen to do it this way.


What’s the largest MD5 hash you can find? I got inspired by Doorknob's blog post What’s the largest MD5 hash you can find? to get my hands dirty and learn some Python. After playing around with the code from that post, I decided to rewrite the whole thing and to make it more flexible (to get some practice).

This is what the code does:

It generates a random string (called alphabet) and calculates all of its permutations
It keeps the permutation with the largest MD5 representation (hashes are numbers)
This is repeated DEFAULT_CYCLES times, if no static alphabet is set. In the end, the winning string is printed.

It ended up having many functions and I'm not sure if this is the best way to go. This is a quite simple task, but takes almost 50 lines of code. However I'm afraid it will become less readable if I try to "compress" it or lump some of the functions together.

So, how could this code be improved and/or shortened? This is the first time I'm writing Python, so please be gentle. :)

from itertools import permutations
from hashlib import md5
from random import choice
from string import ascii_uppercase, ascii_lowercase, digits

# Number of alphabets to test. Ignored if using a static alphabet.
DEFAULT_CYCLES = 256

# Length of 8 recommended. Set to empty string for random values.
STATIC_ALPHABET = ''

def get_alphabet():
    return STATIC_ALPHABET if STATIC_ALPHABET != '' else ''.join(choice(ascii_uppercase + ascii_lowercase + digits) for _ in range(8))

def number_of_cycles():
    return DEFAULT_CYCLES if STATIC_ALPHABET == '' else 1

def numeric_md5(input_string):
    return int(md5(input_string.encode('utf-8')).hexdigest(), 16)

def unique_permutations(plain_string):
    return set([''.join(p) for p in permutations(plain_string)])

def string_with_largest_hash(plain_strings, current_leader):
    largest_hash = 0
    leading_plaintext = ''
    for current_string in plain_strings:
        current_hash = numeric_md5(current_string)
        if current_hash > largest_hash:
            largest_hash = current_hash
            leading_plaintext = current_string
    old_leader_hash = numeric_md5(current_leader)
    return leading_plaintext if largest_hash > old_leader_hash else current_leader

def find_largest_md5():
    cycles = number_of_cycles()
    print('Running {} cycles, alphabet: {}'.format(cycles, (STATIC_ALPHABET + ' (static)' if STATIC_ALPHABET != '' else 'random')))

    leading_string = ''
    for i in range(cycles):
        current_alphabet = get_alphabet()
        jumbled_strings = unique_permutations(current_alphabet)
        leading_string = string_with_largest_hash(jumbled_strings, leading_string)
    print('Largest MD5: {} from plaintext {}'.format(hex(numeric_md5(leading_string)), leading_string))

if __name__ == '__main__':
    find_largest_md5()

max() takes a key

The main thing I wanted to point out is that max() takes a key, which is a one-argument ordering function.

This lets you rewrite your string_with_largest_hash as:

def string_with_largest_hash(plain_strings, current_leader):
    strings = chain(plain_strings, (current_leader,))
    return max(strings, key=numeric_md5)

Which is quite a bit shorter, and in my opinion easier to understand.


Tested cartesian plane utility I am starting to explore automated testing of code, so I decided to write some trivial code about the cartesian plane and test it. I am particularly interested in automated testing conventions and tips.

cartesian.py

def pytagoras(a,b):
    return (a**2 + b**2)**0.5

def distance(point_1,point_2):
    return pytagoras((point_1[0]-point_2[0]),(point_1[1]-point_2[1]))

def point_in_line(point,line):
    def function(x):
        return line[0] * x + line[1]
    return function(point[0]) == point[1]

def average(lst):
    return sum(lst) / len(lst)

def middle_point(point_1,point_2):
    return [average([point_1[0],point_2[0]]),average([point_1[1],point_2[1]])]

def is_valid_triangle(sides):
    return sum(sides) - max(sides) > max(sides)

def heron(AB,BC,AC):
    if not is_valid_triangle((AB,BC,AC)):
        raise ValueError("The sides length given can't be a triangle.")
    sp = sum([AB,BC,AC]) / 2
    return (sp * (sp-AB) * (sp-BC) * (sp-AC) )**0.5

def area_of_tringle(points):
    AB = distance(points[0],points[1])
    BC = distance(points[1],points[2])
    AC = distance(points[0],points[2])
    return heron(AB,BC,AC)

test_cartesian.py

import unittest
import cartesian

class test_cartesian(unittest.TestCase):
    def test_pytagoras(self):
        self.assertEqual(cartesian.pytagoras(3,4),5)
        self.assertEqual(cartesian.pytagoras(6,8),10)

    def test_distance(self):
        self.assertEqual(cartesian.distance( (0,0) , (3,4) ),5)
        self.assertEqual(cartesian.distance( (1,2) , (4,6) ), 5)
        self.assertEqual(cartesian.distance( (1,1) , (1,1) ),0)

    def test_point_in_line(self):
        self.assertTrue(cartesian.point_in_line([4,6],[1,2]))
        self.assertTrue(cartesian.point_in_line([2,8],[3,2]))

    def test_average(self):
        self.assertEqual(cartesian.average([8,4,8,0]), 5)

    def test_middle_point(self):
        self.assertEqual(cartesian.middle_point([8,2],[0,4]),[4,3])
        self.assertEqual(cartesian.middle_point([5,5],[7,13]),[6,9])

    def is_valid_triangle(self):
        self.assertFalse(cartesian.is_valid_triangle(2,4,100))
        self.assertTrue(cartesian.is_valid_triangle(6,5,20))

    def test_heron(self):
        self.assertEqual(cartesian.heron(3,4,5),6)
        self.assertAlmostEqual(cartesian.heron(3,6,8),7.64,places=2)

    def test_area_of_triangle(self):
        self.assertAlmostEqual(cartesian.area_of_tringle(([4,2],[6,2],[7,0])),2)
        self.assertAlmostEqual(cartesian.area_of_tringle(([4,1],[3,4],[9,1])),7.5)

if __name__ == '__main__':
    unittest.main()

There are no docstrings. What do these functions do? What do they return?
The function pytagoras is already built into Python under the name math.hypot.
In point_in_line it appears that a line is being represented by a slope \$ m \$ and a y-intercept \$ b \$. But not all lines can be represented in this form! In particular, vertical lines have no slope or y-intercept.

It is more robust to represent a line in one of these three forms:

Two different points \$ p_0 \$ and \$ p_1 \$ on the line.
A point \$ p \$ on the line and a vector \$ v \$ along the line.
A vector \$ n \$ normal to the line, and its perpendicular distance from the origin \$ d \$.

The function average is built into Python under the name statistics.mean.
Much of this code is awkward because points are represented by Python tuples or lists. A simple operation like subtracting two points (in distance) requires disassembling the points into their elements, subtracting the elements, and then reassembling the result. If the code represented points using some kind of vector data structure, then a lot of it could be simplified.

For example, if the implementations were written very simply like this:

from math import hypot

def distance(p, q):
    """Return the distance from p to q."""
    return hypot(*(p - q))

def middle_point(p, q):
    """Return a point halfway between p and q."""
    return 0.5 * (p + q)

Then using the built-in turtle.Vec2D:

>>> from turtle import Vec2D
>>> distance(Vec2D(0, 0), Vec2D(3, 4))
5.0
>>> middle_point(Vec2D(1, 2), Vec2D(3, 4))
(2.00,3.00)

(Note that I don't actually recommend that you use turtle.Vec2D. This vector class is very basic: it doesn't have dot products or cross products or even division. But it's built into Python, so you can experiment with it without having to download or install anything. For actual use, NumPy arrays would be good, or pygame.math.Vector2, or something pure-Python like my vector.py.)
The unit test cases are always the same. There's nothing wrong with this, but it seems a bit wasteful, because every time you run the tests they go through the same steps. If a case passed last time you ran it, and you haven't changed the function under test, then it's certain to pass this time. All the test is protecting you against is a regression: it doesn't help you find new bugs.

A technique that sometimes does find new bugs is randomizing the test case. For example, we could test the distance function by generating random points that are a known distance apart:

import random

def test_distance(self):
    z = 1000000
    u = random.uniform
    p = Vec2D(u(-z, z), u(-z, z))
    self.assertEqual(cartesian.distance(p, p), 0)
    d = u(0, z)
    q = p + Vec2D(0, d).rotate(u(0, 360))
    self.assertAlmostEqual(cartesian.distance(p, q), d)
    self.assertAlmostEqual(cartesian.distance(q, p), d)

Then each time the test case is run a different set of points will be tested.

If you actually do get an error from a randomized test case, you'll want to reproduce it for debugging. How do you do that when it's random? You need to seed the random number generator when the test suite starts, and record the seed, for example like this:

import binascii
import os
seed = os.urandom(16)
print("seed:", binascii.hexlify(seed))
random.seed(seed)


Initialise simple_form object inline Using Rails 4 and a simple form, I'm rendering a couple of forms outside of their native controllers. Rather than scattering initialisers through my controllers, I wondered if it is cleaner to do the following:

<%= simple_form_for @event_submission || EventSubmission.new do |f| %>

I do stuff like that myself sometimes, but I'm not proud of it.

Strictly speaking it is the controller's responsibility to set up the new record. The controller should be be the sole source of whatever (primary) data the view needs. Instantiating a new record is business logic, not presentation logic.

In a similar vein, if we consider a simple blogging app, it'd be fine for the view to access secondary data, like:

<% @post.comments.each do |comment| %>

But I think you'd agree that it absolutely shouldn't be doing something like:

<% Post.find(params[:id]).comments.each do |comment %> <!-- please don't do this -->

Your code basically the same as the latter of those two examples. Although it seems less extreme, in both cases the view is doing the job of the controller.

Also: Assuming a typical Rails setup, your form is in its own partial, and you include it in both the edit and new views when rendering. Now, when editing, the @event_submission variable absolutely should exist - it should be set by the controller. It makes no sense for the form to create it. And when one "invocation" of the form requires it, it makes no sense for the other invocation to make it optional. So basically, the form requires an @event_submission variable - doesn't matter if it's new or not.

Letting for form instantiate the record also opens the door for hidden bugs and unintended behavior. If you navigate to /event_submissions/123/edit (or whatever the path is), but the controller somehow doesn't set the @event_submission to be edited, the form will just create its own. This means that:

You won't get an error (though you should!) because the form absorbed it. Your tests may even pass.
The form will be blank, which it shouldn't be when editing.
And since it's inferred that a form for a new record should point to the create action, if you submit the form, you're creating a new record when all you wanted was to edit one.

Everything works, there are no errors, but it's not at all doing what it's supposed to. Have fun debugging :)

Besides, your code works for HTML via ERB. But if you want a different format, or use a different templating language, you may not get away with it anymore. Since you'll then have to do the initialization in the controller anyway, you might as always do that.


How to improve this array manipulation function? The function takes 2 arguments: array: int[], cap: int

Members would be ranged from 0 to cap.

e.g.

array = [0,0,0,2,6,3,0,4,2,4,0]
cap = 6

The return value is a new array of same length based on these rules:

The function can only change values of consecutive elements which are > 0.  i.e. any where the original array has 0, the new array has 0 as well.
In a consecutive series of elements which are > 0, if the maximum of it equals cap, then in the new array they are changed to 1,1,...,1,cap of the same length. e.g.  0,2,6,4,0 -> 0,1,1,6,0
In a consecutive series of elements which are > 0, if the maximum of it is smaller than cap, then the elements in the new array are mapped to either 1 or maximum  (if the element value < maximum then 1, otherwise maximum ) . e.g.  0,2,5,5,0 -> 0,1,5,5,0

So the sample array would map to [0,0,0,1,1,6,0,4,1,4,0]

The following are my code which are implemented in both languages, it surprises me that F# takes same lines as C#.

I would like to suggestions to improve either language.

F#

open System

let private reorder cap (array: int[]) =
    if Array.isEmpty array then
        array
    else
        let max = array |> Array.max
        if max = cap then
            let newArray = Array.create array.Length 1
            newArray.[array.Length-1] <- cap
            newArray
        else
            array |> Array.map (fun elem -> if elem = max then max else 1)

let private getZeroes todo =
    todo |> Seq.takeWhile (fun elem -> elem = 0)
         |> Seq.toArray

let private getNumbers todo cap =
    todo |> Seq.takeWhile (fun elem -> elem <> 0)
         |> Seq.toArray
         |> reorder cap

let GetEquivalentPermutation (array: int[], cap) =

    let rec joinParts finished todo =
        if Seq.isEmpty todo then finished |> Seq.toArray
        else
            let zeroes = getZeroes todo
            let nextTodo = todo |> Seq.skip zeroes.Length
            let numbers = getNumbers nextTodo cap
            let finalTodo = nextTodo |> Seq.skip numbers.Length
            let newFinished = Seq.append (Seq.append finished zeroes) numbers
            joinParts newFinished finalTodo

    joinParts [] array

C#

    static int[] EquivalentOutput(int cap, int[] permutation)
    {

        IEnumerable<int> right = permutation;
        List<int> equivalentLine = new List<int>();

        while (right.Any())
        {
            var leftArray = right.TakeWhile(x => x != 0).ToArray();

            List<int> leftEquivalent;
            if (leftArray.Length == 0)
            {
                leftEquivalent = new List<int>();
            }
            else if (leftArray.Length != 1)
            {
                int max = leftArray.Max();
                if (max == cap)
                {
                    leftEquivalent = leftArray.Take(leftArray.Length - 1).Select(x => 1).ToList();
                    leftEquivalent.Add(cap);
                }
                else
                {
                    leftEquivalent = leftArray.Select(x => x < max ? 1 : max).ToList();
                }
            }
            else
            {
                leftEquivalent = leftArray.ToList();
            }
            equivalentLine.AddRange(leftEquivalent);
            equivalentLine.AddRange(right.SkipWhile(x => x != 0).TakeWhile(x => x == 0));
            right = right.SkipWhile(x => x != 0).SkipWhile(x => x == 0);

        }
        return equivalentLine.ToArray();
    }

Seems you might be overusing Linq since you are manipulating an array.  Here is an example that to me is a bit more readable (although the readability could be improved further) and it is faster.

private static int[] EquivalentOutput(int cap, int[] permutation)
{
    int[] copy = permutation.ToArray();

    for (int i = 0; i < copy.Length; i++)
    {
        if (copy[i] == 0)
            continue;

        var consecutiveElements = copy.Skip(i).TakeWhile(x => x != 0).ToArray();
        var max = consecutiveElements.Max();
        if (max == cap)
        {
            for (int j = 0; j < consecutiveElements.Length - 1; j++)
                copy[i++] = 1;
            copy[i++] = cap;
        }
        else
        {
            for (int j = 0; j < consecutiveElements.Length; j++)
                copy[i] = copy[i++] == max ? max : 1;
        }
    }
    return copy;
}

Update:
@svick pointed out that it feels a bit "weird" changing the loop variable inside the loop so I am providing an alternative which some people will consider easier to read...

private static int[] EquivalentOutput(int cap, int[] permutation)
{
    int[] copy = permutation.ToArray();

    int i = 0;
    while (i < copy.Length)
    {
        if (copy[i] > 0)
        {
            var consecutiveElements = copy.Skip(i).TakeWhile(x => x != 0).ToArray();
            var max = consecutiveElements.Max();
            if (max == cap)
            {
                for (int j = 0; j < consecutiveElements.Length - 1; j++)
                    copy[i++] = 1;
                copy[i++] = cap;
            }
            else
            {
                for (int j = 0; j < consecutiveElements.Length; j++)
                    copy[i] = copy[i++] == max ? max : 1;
            }
        }

        i++;
    }
    return copy;
}


Calculate fingerprint for an object I need to store serialized objects in a database and I want to avoid duplicates. In order to be able to tell wheter an object is already stored, I have to compare them somehow. My idea was to calculate a hash for some of its properties that I consider as equatable and store it in a separate column.

The algorithm is very simple: I implemented it with a builder that allows me to define a hash function and how each property should be formatted. E.g. strings may be case insensitive, floats may be rounded or date-time may be stripped of its millisecond part etc. This transformation is done by each expression. It then serializes each value into bytes and adds them to the buffer. Finally the hash-function calculates the hash-value. Sorting the expressions by member-name should prevent bugs caused by simply changing their order.

public class FingerprintBuilder<T>
{
    private readonly List<(string MemberName, Func<T, object> Fingerprint)> _fingerprints;

    public FingerprintBuilder()
    {
        _fingerprints = new List<(string Expression, Func<T, object> Fingerprint)>();
    }

    public static FingerprintBuilder<T> Empty => new FingerprintBuilder<T>();

    public FingerprintBuilder<T> For<TProperty>(Expression<Func<T, TProperty>> memberExpression, Expression<Func<TProperty, TProperty>> fingerprint)
    {
        var getProperty = memberExpression.Compile();
        var getFingerprint = fingerprint.Compile();
        _fingerprints.Add((
            ((MemberExpression)memberExpression.Body).Member.Name,
            (Func<T, object>)(obj => getFingerprint(getProperty(obj)))
        ));
        return this;
    }

    public Func<T, byte[]> Build(Func<byte[], byte[]> computeHash)
    {
        return obj =>
        {
            var buffer = new List<byte>();

            var binaryFormatter = new BinaryFormatter();
            foreach (var t in _fingerprints.OrderBy(e => e.MemberName, StringComparer.OrdinalIgnoreCase))
            {
                var fingerprint = t.Fingerprint(obj);
                using (var memory = new MemoryStream())
                {
                    binaryFormatter.Serialize(memory, fingerprint);
                    buffer.AddRange(memory.ToArray());
                }
            }

            return computeHash(buffer.ToArray());
        };
    }
}

In this example I also use two helper extensions. One for calculating the hash and the other one to get its string representation:

public static byte[] ComputeSHA1(this byte[] source)
{
    if (source == null) throw new ArgumentNullException(nameof(source));

    using (var sha1 = new SHA1Managed())
    {
        return sha1.ComputeHash(source);
    }
}

public static string ToHexString(this byte[] source)
{
    if (source == null) throw new ArgumentNullException(nameof(source));

    return
        source
            .Aggregate(new StringBuilder(), (current, next) => current.Append(next.ToString("X2")))
            .ToString();
}

Example

Testing it with these examples:

var p1 = new Person { FirstName = "John", LastName = "Doe", Age = 20 };
var p2 = new Person { FirstName = "John", LastName = "DOE", Age = 30 };
var p3 = new Person { FirstName = "Johny", LastName = "DOE", Age = 30 };

var personFingerprint =
    FingerprintBuilder<Person>.Empty
        .For(p => p.LastName, x => x.ToUpperInvariant().Trim())
        .For(p => p.FirstName, x => x.ToUpperInvariant().Trim())
        .Build(SHA1.ComputeSHA1);

personFingerprint(p1).ToHexString().Dump();
personFingerprint(p2).ToHexString().Dump();
personFingerprint(p3).ToHexString().Dump();

produces these three results:

D46E5C869CDBA5775D7F8C9AF6BC497D5EEA76C4
D46E5C869CDBA5775D7F8C9AF6BC497D5EEA76C4
F450C1CE6B1A2041D8D7D0FEAA31AF7C5C214D81

What do you think of this algorithm? Does it have any weaknesses or can anything else be improved?

It looks alright to me, although it seems odd having a young man of age 20 to equal an "elderly" man of age 30 :-).

I have 3 minor things:

1) One MemoryStream can be used to serialize all expressions:

  public Func<T, byte[]> Build(Func<byte[], byte[]> computeHash)
  {
    return obj =>
    {
      var binaryFormatter = new BinaryFormatter();
      using (var memory = new MemoryStream())
      {
        foreach (var t in _fingerprints.OrderBy(e => e.MemberName, StringComparer.OrdinalIgnoreCase))
        {
          var fingerprint = t.Fingerprint(obj);
          {
            binaryFormatter.Serialize(memory, fingerprint);
          }
        }

        return computeHash(memory.ToArray());
      }
    };
  }

2) The ToHexString() can be simplified like this:

  public static string ToHexString(this byte[] source)
  {
    if (source == null) throw new ArgumentNullException(nameof(source));

    return string.Join("", source.Select(ch => ch.ToString("X2")));
  }

3) FingerprintBuilder.Empty is an strange name. I would call it Builder or avoid the static instantiater.


Passing an array in a hyperlink Maybe this is fine the way it is, but it seems messy. Basically I have dynamically generated HTML that will display a list of songs in a given folder. Each song has an ID, and I want to add a button that allows you to add every song in the folder.

Right now it's creating the add all button with the array of song IDs as an argument. It just feels messy since the list can be incredibly long and is now exposed to the HTML, but maybe it's fine?

It's currently working like this:

if (songidarray.length > 0)
    backbuttonhtml += "Add Entire Folder";
window.$("#folderpagebackbutton").html(backbuttonhtml).trigger('create');

I could make the songidarray a global variable and make the button static. But that seems like a cop out for some reason, maybe I'm just asking a stupid question and that's the answer right there. Wondering if there is a way to do it without using a global variable, or if there's nothing wrong with using one.

Instead of making the songidarray global, use a closure.

$(window).load(function () { // for example, or wherever you get the data
    var songidarray = [1,2,3,4,5,9];
    // var songidarray = [];

    var $button = $("<button id='addEntireFolderButton' type='button'>")
        .html("Add Entire Folder")
        .prop("disabled", songidarray.length == 0)
        .click(function () {
            applib.addentirefolder(songidarray);
        });
    $('#folderpagebackbutton').append($button);
});

The click callback above will maintain a live reference to songidarray and will pass it to the addentirefolder function when triggered. The contents of the array stay out of the HTML.

And since this is Code Review:

The type attribute on <a> elements is a hint for the MIME type of the referenced resource. Use the class attribute if you need to tag an element for styling.  On a <button>  element, type="button" makes it do nothing by default as opposed to submitting or resetting any containing form.


Common ancestor in a binary tree This method finds the common ancestor of two nodes in a binary tree.

Any style suggestions or ways to make it more idiomatic? Is the algorithm correct? Rubocop says the cyclomatic complexity is too high, how would you break this up?

class Node
  def initialize(value = nil, left = nil, right = nil)
    @value = value
    @left = left
    @right = right
  end
end

def common_ancestor(node, value1, value2)
  return node unless node
  return node if node.value == value1 || node.value == value2
  anc_left = common_ancestor(node.left, value1, value2)
  anc_right = common_ancestor(node.right, value1, value2)
  return node if anc_left && anc_right
  anc_left || anc_right
end

I think you can add functionality to Node to make the common ancestor search more ruby like.  (I can add a search by value later, if you want.)  I've added a parent member that is set automatically, and I've added each_parent and each_child_or_selfenumerators to enable easy traversal up or down the tree.

Note: Thinking about it, the search function needs a little more work in case node 2 is higher in the tree than node 1, but I am out of time right now.  Let me know what you think. Testing indicates that this is not an issue.

class Node
    attr_accessor :value, :parent
    attr_reader :left, :right

    def initialize(value = nil, left = nil, right = nil)
        @parent = nil
        @value = value
        self.left = left
        self.right = right
    end

    def left=(node)
        unless node.nil?
            @left = node
            node.parent = self
        end
        @left
    end

    def right=(node)
        unless node.nil?
            @right = node
            node.parent = self
        end
        @right
    end

    def each_parent
        return enum_for(:each_parent) unless block_given?
        p = @parent
        while not p.nil?
            yield p
            p = p.parent
        end
    end

   def each_child_or_self
       return enum_for(:each_child_or_self) unless block_given?
       stack = [self]
       while not stack.empty?
           current = stack.pop
           stack.push current.left unless current.left.nil?
           stack.push current.right unless current.right.nil?
           yield current
       end
    end
end

The new search functions:

def common_ancestor(node1, node2)
    return nil if node1.nil? || node2.nil?
    return node1 if node1 == node2.parent
    return node2 if node2 == node1.parent
    node1_parents = node1.each_parent.to_a
    node2.each_parent { |p| return p if node1_parents.include? p }
    nil
end

def common_ancestor_by_value(root, value1, value2)
    return nil if root.nil?
    nodes = root
        .each_child_or_self
        .lazy
        .select { |n| n.value == value1 || n.value == value2 }
        .first(2)
    return nil unless nodes.length == 2
    common_ancestor(nodes.first, nodes.last)
end

And a test:

n41 = Node.new("DDD")
n42 = Node.new("EEE")
n43 = Node.new("FFF")
n44 = Node.new("GGG")
n21 = Node.new("BB", n41, n42)
n22 = Node.new("CC", n43, n44)
root = Node.new("A", n21, n22)

puts common_ancestor(n44, n21).value #common ancestor of 'GGG' and 'BB' is 'A'
puts common_ancestor(n21, n44).value #order doesn't matter
puts "---"
puts common_ancestor_by_value(root, "GGG", "BB").value #same applies when searching by value
puts common_ancestor_by_value(root, "BB", "GGG").value

Update 1: Added second enumerator and search function, fixed a bug

Update 2: Added lazy enumeration and profiled code against a tree with 10K nodes.  Searching for nodes with values in the middle, the new lazy code runs about 2.5 times faster than the poster's original implementation.  Also running rubocop, there are no Cyclomatic complexity warnings


Haskell encryption tips I wrote a variation of the Caeser Cipher and would like some feedback.
Is it idiomatic? Can you generally see anything that can be improved?

Use example:

let e = enc "Hello World" "This is the key"
 in dec e "This is the key"

Instead of taking a single value as the offset, it takes a String as the key and uses each respective character from the key as the offset; wrapping when it reaches the upper/lower bound.

module Encryption where

import Data.Char

type Key = String
type Message = String
type Direction = (Int -> Int -> Int)

--Upper and lower ascii code bounds
upperBound = 127 ; lowerBound = 32

shift :: Char -> Char -> (Int -> Int -> Int) -> Char
shift char charoff dir = fix $ dir (ord char) (ord charoff)

fix :: Int -> Char
fix charcode
        | charcode > upperBound = chr $ (lowerBound - 1) + excessOf (upperBound + lowerBound)
        | charcode < lowerBound = chr $ (upperBound + 1) - excessOf (lowerBound - lowerBound)
        | otherwise = chr charcode
        where   excessOf boundry = abs(boundry - charcode)

alter :: Message -> Key -> (Int -> Int -> Int) -> Message
alter msg key dir = [(shift c k dir) | (c,k) <- zip msg (cycle key)]

enc msg key = alter msg key (+)
dec msg key = alter msg key (-)

--Basic Caeser Cipher
{-
alter :: Message -> Key -> Direction -> Message
alter m k dir = [chr $ dir (ord out) k | out <- m]

enc m k = alter m k (+)
dec m k = alter m k (-)
-}

type Direction = (Int -> Int -> Int)

These parentheses are superfluous, though they don't really hurt if they help you to read the code.

upperBound = 127 ; lowerBound = 32

It's not very common to put definitions on the same line with ;, particularly top-level definitions, so this made me double-take a bit. It makes it hard to find a particular definition by scanning the left column of the file.

It's a good idea to give all top-level definitions type signatures, as it helps to catch type errors earlier and makes it easier for someone reading the code.

Also, with numeric types, overloading in the Num class can often give surprising results, so explicitly stating that these are Int is a good idea.

shift :: Char -> Char -> (Int -> Int -> Int) -> Char
shift char charoff dir = fix $ dir (ord char) (ord charoff)

Consider putting the function argument first here. The function is more likely to be useful partially applied as shift dir than as shift char, and it also makes for a more intuitive type signature which correctly gives the sense that a function Int -> Int -> Int is being transformed into a function Char -> Char -> Char.

fix :: Int -> Char

As someone mentioned on your StackOverflow post, fix is a standard library name for something different, so best not to reuse it here.

| charcode > upperBound = chr $ (lowerBound - 1) + excessOf (upperBound + lowerBound)
| charcode < lowerBound = chr $ (upperBound + 1) - excessOf (lowerBound - lowerBound)
    where   excessOf boundry = abs(boundry - charcode)

I find it hard to understand these two cases. You could perhaps add comments to explain what they are doing. Also lowerBound - lowerBound is just 0 so what are you trying to do here? I presume the whole thing is about clamping to within the valid ASCII range but it's quite confusing.

In general I'd have expected a Caesar cipher just to work on letters, rather than the whole ASCII range, but that's a specification question really.

alter :: Message -> Key -> (Int -> Int -> Int) -> Message
alter msg key dir = [(shift c k dir) | (c,k) <- zip msg (cycle key)]

Again I would put the function as the first argument here. I would also put the Key as the second argument, because then you can partially apply it with a key to get a specialised encryption or decryption function. It might also make sense to have the key first and the function second.

enc msg key = alter msg key (+)
dec msg key = alter msg key (-)

As before, type signatures would help here. Also, if you do change the order of the arguments to alter above then this can be abbreviated as enc = alter (+) or enc key = alter key (+), which is perhaps slightly more elegant.


Number of sub strings with same first and last character Problem Statement:

  You are given a function \$f(x)\$, where \$f(x)\$ is \$1\$ if the first and last characters of string \$x\$ are equal; else it is \$0\$.
  You are given a string \$S\$ and you have to find the sum of \$f(x)\$ for all substrings \$x\$ of given string \$S\$.

Sample Input:

7
ababaca

Sample Output:

14

Explanation:

f("a")=1, f("aba")=1, f("abaca")=1 but f("ab")=0, f("bac")=0. Hence counting all substrings we get 14.

The 14 substring are

a - 4(times)
b - 2
c - 1
aba - 2
bab - 1
aca - 1
ababa - 1
abaca - 1
ababaca - 1

My code:

l = int(input())
s = [x for x in input().strip()]
print(l + sum([1 for i in range(0,l-1) for j in range(i+1,l) if s[i] == s[j]]))

I have tried the following snippets too:

Timeout:

from collections import Counter
import math
l, s = int(input()), [x for x in input()]
print(l + sum([math.factorial(value-1) for value in Counter(s).values() if value != 1]))

Wrong answer:

import itertools
l, s = int(input()), input()
print(sum(int(i[0] == i[-1]) for i in itertools.combinations_with_replacement(s,2)))

The solutions is working fine except that it times out in certain test cases. Can anyone suggest a better way to do this?

First review

It is almost always a good idea to put your logic in a function (or class) that can be easily documented and tested. That way, in anything goes wrong during the optimisation, you'll know it straight-away. Also, this can be useful if you need to measure your result to ensure your optimisation is an actual optimisation.

Just moving the code around, here is what I start with :

def get_nb_substring(s):
    l = len(s)
    return(l + sum([1 for i in range(0,l-1) for j in range(i+1,l) if s[i] == s[j]]))

def test_from_input():
    _ = int(input()) # useful ?
    s = [x for x in input().strip()]
    print(get_nb_substring(s))

def automatic_tests():
    assert get_nb_substring("ababaca") == 14

if __name__ == '__main__':
    automatic_tests()

Now, the actual review may start.

you don't need 0 as a first argument for range.
you usually don't need the length when you are using iterations in Python. When you do need it, it usually means you are doing something wrong.
for that reason, what you are doing with indices can be achieved with itertools.combinations(s, 2)

Now, the code can be rewritten :

def get_nb_substring(s):
    return len(s) + sum(1 for i, j in itertools.combinations(s, 2) if i == j)

which is still 0(n^2).

Optimisations

An idea could be to check where the different letters appear. Then if a letter c appears at positions p1, p2, ..., pn, you know it will contribute for substrings : p1, p1-p2, p1-p3, ..., p1-pn, p2,p2-p3, p2-p4, ... p2-pn, .... pn. There will be 1+2+...+n = n*(n+1)/2 such substrings.

There, if I did everything correctly, I end up with O(n) code :

def get_nb_substring(s):
    position = dict()
    for i, c in enumerate(s):
        position.setdefault(c, []).append(i)
    return sum(l*(l+1)/2 for l in (len (l) for l in position.values()))

On the test example, it seems to work. On other cases, I obtain different results than your code so I don't know which is correct.

I've used a dict to map characters to a list of position but a mapping from characters to number of positions would have been enough. This corresponds to using the Counter collection)

def get_nb_substring(s):
    return sum(v*(v+1)/2 for v in collections.Counter(s).values())


Using BigInt in JavaScript, write a factorial function that takes either a Number or BigInt and returns a BigInt Is there anything missing or any pitfall when using BigInt?

// Assuming n is an integer and is 0 or greater
function factorial(n) {
    var i, result = BigInt(1);

    for (i = BigInt(2); i <= n; i++) {
        result *= i;
    }
    return result;
}

console.log(factorial(69));
console.log(factorial(200));

console.log(factorial(69n));
console.log(factorial(200n));

jsfiddle: https://jsfiddle.net/adr49tz2/

My answer is pretty heavily based on this resource on BigInt, so feel free to explore that a bit first.

With that said, here are some of the key points I saw:

BigInt can use all the same types of simple arithmetic as Number, along with 0 being considered falsy as usual

1n + 3n === 4n // true!

BigInt is also usable as a JS primitive. Neat!

typeof 1n === "bigint" // true!

BigInt and Number should not be mixed and matched. The coercion between the two is ambiguous, and as such JavaScript would throw an exception in that circumstance, so you should only either use one or the other.

1 + 1n // whoops

If the article still holds true, BigInt doesn't have the luxury of using operations in the built-in Math object. So you would either need to stick to simple arithmetic, or build (or otherwise find) those implementations yourself.
Since the supported operations on BigInt are not constant time (according to the article), it is recommended to use BigInt strictly when a use case will frequently involve numbers higher than the largest representable int in Number (253). Your question is one such case, since factorials grow extremely fast, so I think it's fine as is.


Hamming Distance in Python I was solving this Leetcode challenge about Hamming Distance. Would love feedback on my syntax and code style. Here's the challenge description:

  The Hamming distance between two integers is the number of positions at which the corresponding bits are different.

  Given two integers x and y, calculate the Hamming distance.

  Note:
  0 ≤ x, y < 231.

  Example:

  Input: x = 1, y = 4

  Output: 2

  Explanation:

1   (0 0 0 1)
4   (0 1 0 0)
       ↑   ↑

  The above arrows point to positions where the corresponding bits are
   different.

Here's my solution:

   def hammingDistance(x: 'int', y: 'int') -> 'int':

    bin_x = bin(x)[2:]
    bin_y = bin(y)[2:]
    len_x = len(bin_x)
    len_y = len(bin_y)

    if len_x != len_y:
        if len_x > len_y:
            zeros = '0' * (len_x - len_y)
            bin_y = zeros + bin_y
        else:
            zeros = '0' * (len_y - len_x)
            bin_x = zeros + bin_x

    dist = 0
    for x,y in zip(bin_x, bin_y):
        if x!=y:
            dist += 1

    return dist

That's a reasonable approach, but converting to strings early and then doing everything on strings leads to a lot of extra code. Note that this construct in your code, if x!=y, essentially XORs two bits and then does something if the result of that XOR is 1.

Doing that XOR directly on the integers makes the problem simpler, the whole problem of having to pad with zeroes disappears (which about halves the code size already), and the problem is reduced to "count the ones in binary". That could still be done with a similar loop, but there is a short-cut:

return bin(x ^ y).count('1')


Return Full Month Name From 3 Letter Month I have piece metaled this code together from multiple google searches and thanks to the function from the site mentioned in the code.  I am sure there are ways to optimize this for speed.  What do you guys suggest?

using System;
using System.Globalization;

public class Program
{
    public static void Main()
    {
        string shortmonth = "Mar";
        string num = GetMonthNumberFromAbbreviation(shortmonth);
        string monthname = CultureInfo.CurrentCulture.DateTimeFormat.GetMonthName(Convert.ToInt32(num));
        Console.WriteLine(monthname);
    }
    //https://blogs.msmvps.com/deborahk/converting-month-abbreviations-to-month-numbers/
    private static string GetMonthNumberFromAbbreviation(string mmm)
    {
       string[] monthAbbrev =
          CultureInfo.CurrentCulture.DateTimeFormat.AbbreviatedMonthNames;
       int index  = Array.IndexOf(monthAbbrev, mmm) + 1;
       return index.ToString("0#");
    }
}

The following avoids assumptions about and/or the need to convert according to culture, by letting the tried-and-tested DateTime plumbing do the work.  Creating a translation map once at first instantiation of the class (the static constructor) is the best I can think of to optimise performance.

If you use it, don't forget to do error-checking - specifically the result of TryParse.

using System;
using System.Collections.Generic;

public class Program
{
  private static readonly Dictionary<string, string> MonthNameMap;
  //  Static constructor to build the translation dictionary once
  static Program()
  {
    MonthNameMap = new Dictionary<string, string>();
    //var months = new List<string>() { "Jan", "Feb", "Mar", "Apr", "May", "Jun", "Jul", "Aug", "Sep", "Oct", "Nov", "Dec" };
    //foreach (var shortMonthString in months)
    //{
    //  DateTime.TryParse($"1 {shortMonthString} 2000", out var dt);
    //  MonthNameMap.Add(shortMonthString, dt.ToString("MMMM"));
    //}
    for (int i = 1; i <= 12; i++)
    {
      DateTime.TryParse($"2000-{i}-25", out var dt);
      MonthNameMap.Add(dt.ToString("MMM"), dt.ToString("MMMM"));
    }
  }

  public static void Main()
  {
    foreach (var entry in MonthNameMap)
    {
      Console.WriteLine($"{entry.Key}: {entry.Value}");
    }
  }
}

Results:

Jan: January
Feb: February
Mar: March
Apr: April
May: May
Jun: June
Jul: July
Aug: August
Sep: September
Oct: October
Nov: November
Dec: December


Safe static_cast in C++ in a codebase How would you review this way of implementing a std::static_cast

template <typename ToType, typename FromType>
ToType safe_static_cast( const FromType &from )
{
    assert( from <= std::numeric_limits<ToType>::max() );
    assert( from >= std::numeric_limits<ToType>::min() );

    return static_cast<ToType>(from);
}

Is assertions on the limits a good way?

This is a very interesting question.

First, I would reconsider the name. safe_static_cast implies that it is more general than it is. The types must have std::numeric_limits specializations in your implementation, so clearly you are thinking of numeric types. Maybe safe_numeric_cast or even numeric_cast would be a better name.

Secondly, assert is too often compiled out, so I would ask to consider alternatives which aren't. One alternative is throwing std::out_of_range. Another alternative is to saturate the destination type: if the source value is larger than the largest destination value, return the largest destination value.

Consider FromType = int, from = -1, ToType = unsigned int. Note that -1 >= 0u is true.

Consider the range of floating point types: min() is the lowest positive floating point value.

Consider floating point infinities: numeric_cast<double, double> should always be fine, right? But the comparison will fail for infinity.

Consider floating point nans: similarly, all comparisons fail for nans.


Readers-writers problem using wait notify Description of the Readers–writers problem.

It looks like it works, but I have some doubts:

class ReadWriteBuffer<T> {

    private static final Object writeLock = new Object();

    private static final Object readWriteLock = new Object();

    private int numberReaders = 0;

    private int numberWriters = 0;

    private int numberWriteRequests = 0;

    public void write(T object) throws InterruptedException {

        numberWriteRequests++;

        synchronized (readWriteLock) {
            while (numberReaders > 0) {
                readWriteLock.wait();
            }
        }

        synchronized (writeLock) {

            numberWriteRequests--;

            numberWriters++;

            System.out.println("Writer #" + Thread.currentThread().getId() + " started writing.");
            Thread.sleep(3000);
            System.out.println("Writer #" + Thread.currentThread().getId() + " finished writing.");

            numberWriters--;

            synchronized (readWriteLock) {
                readWriteLock.notifyAll();
            }
        }
    }

    public void read() throws InterruptedException {

        synchronized (readWriteLock) {
            while (numberWriters > 0 || numberWriteRequests > 0) {
                readWriteLock.wait();
            }
        }

        numberReaders++;

        System.out.println("Reader #" + Thread.currentThread().getId() + " started reading.");
        Thread.sleep(1000);
        System.out.println("Reader #" + Thread.currentThread().getId() + " finished reading.");

        numberReaders--;

        synchronized (readWriteLock) {
            readWriteLock.notifyAll();
        }
    }
}

class Reader implements Runnable {

    private final ReadWriteBuffer<Object> buffer;

    private int priority;

    public Reader(int priority, ReadWriteBuffer<Object> buffer) {
        this.priority = priority;
        this.buffer = buffer;
    }

    @Override
    public void run() {
        while (true) {
            try {
                Thread.sleep(priority);
                buffer.read();
            } catch (InterruptedException e) {
                e.printStackTrace();
            }
        }
    }
}

class Writter implements Runnable {

    private final ReadWriteBuffer<Object> buffer;

    private int priority;

    public Writter(int priority, ReadWriteBuffer<Object> buffer) {
        this.priority = priority;
        this.buffer = buffer;
    }

    @Override
    public void run() {
        while (true) {
            try {
                Thread.sleep(priority);
                buffer.write("Object");
            } catch (InterruptedException e) {
                e.printStackTrace();
            }
        }
    }
}

public class ReadersWriters {

    public static void main(String[] args) {

        ReadWriteBuffer<Object> sharedBuffer = new ReadWriteBuffer<Object>();

        new Thread(new Reader(1, sharedBuffer)).start();
        new Thread(new Reader(700, sharedBuffer)).start();
        new Thread(new Reader(100, sharedBuffer)).start();
        new Thread(new Writter(2000, sharedBuffer)).start();
        new Thread(new Writter(4000, sharedBuffer)).start();
    }
}

This code in the read() method is broken:

public void read() throws InterruptedException {

    synchronized (readWriteLock) {
        while (numberWriters > 0 || numberWriteRequests > 0) {
            readWriteLock.wait();
        }
    }

    numberReaders++;

    System.out.println("Reader #" + Thread.currentThread().getId() + " started reading.");
    Thread.sleep(1000);
    System.out.println("Reader #" + Thread.currentThread().getId() + " finished reading.");

    numberReaders--;

    synchronized (readWriteLock) {
        readWriteLock.notifyAll();
    }
}

Since numberReaders ++ and -- is not in a synchronized block, it essentially may as well not happen as far as other threads are concerned, or, worse, it could partially happen (like the ++ may happen, and the -- may not).

Additionally, they may not produce the right results, anyway. The ++ operator is not atomic, and it is possible for the value to be ++ in two different threads at the same time, and the result to be just one increment for both invocations.

Bottom line is that, from a multi-threaded perspective, this code is broken.

References:

Is the pre-incriment operator thread-safe?
Java Concurrency in Practice (read this if you can)
Java's ReadWriteReentrantLock


Is the way I'm handling deck (card) generation the wisest way to do it? My Deck Class:

(In case it isn't clear, a Card is just an object with two enums, one for suit and one for value).

I'm particularly proud of the readability of this little bit if I do say so myself.

public class Deck {

    private static Random random = new Random();

    private List<Card> cards = new ArrayList<Card>();

    Deck () {
        // Create one of each card to populate deck with.
        for (Card.Suit suit : Card.Suit.values()) {
            for (Card.Value value : Card.Value.values()) {
                Card card = new Card(value, suit);
                cards.add(card);
            }
        }
        // Shuffle multiple times to ensure randomness...
        this.shuffle();
        this.shuffle();
        this.shuffle();
    }

    public void shuffle() {
        List<Card> temp = new ArrayList<Card>();
        while(!cards.isEmpty()) {
            int randInt = random.nextInt(cards.size());
            temp.add(cards.remove(randInt));
        }
        cards = temp;
    }

    public void addCard(Card card) {
        cards.add(card);
    }

    public Card getNextCard() {
        return cards.remove(0);
    }

}

EDIT:

I revised the drawCard() method. What do you guys think?

public Card drawCard() {
    if(cards.isEmpty()) {
        System.err.println("Can't draw from an empty deck!");
        System.exit(-1);
    }
    return cards.remove(cards.size() - 1);
}

I would agree that your implementation is quite readable. Indentation and white space are consistent. The variables and operations are named clearly and concisely, with the exception of 'getNextCard' as noted by '200_success'. This allows us to focus much more at the low level nuts and bolts of your code.

    private static Random random = new Random();

Use caution in assigning an instance of the Random class as a static member as concurrent access to its operations can cause contention and consequent poor performance. A pattern I like to follow to ensure random instances are seeded uniquely is to have a package level static random member for seeding instances. This still risks contention, but limits the surface area for it to occur in. For simple gaming purposes the 48-bit seed of Random will suffice, but using java.security.SecureRandom will provide a better source of entropy:

    private static final SecureRandom SEEDER = new SecureRandom();
    private static final int ENTROPY = 256; // 2048-bit is good enough for banking
    private SecureRandom random = new SecureRandom(SEEDER.generateSeed(ENTROPY));

-

    this.shuffle();
    this.shuffle();
    this.shuffle();

If your shuffle method and random source are fair, than one shuffle is actually better than more -- you're skipping over entropy and risk introducing bias. If either is not fair shuffling more will not make them fair.

public void shuffle() {
    List<Card> temp = new ArrayList<Card>();
    while(!cards.isEmpty()) {
        int randInt = random.nextInt(cards.size());
        temp.add(cards.remove(randInt));
    }
    cards = temp;
}

While I would argue with mjolka that your implementation of shuffle is a non-in-place implementation of the Fisher-Yates shuffle, I would prefer to be more concise and use the base library utility method:

public void shuffle() {
    java.util.Collections.shuffle(cards, random);
}

I would join both mjolka and 200_success in recommending to guard your getNextCard implementation and throw an appropriate error as well as exposing an external status check method such as size() or isEmpty(). Also I agree the card should be popped from the end of the list when using an ArrayList as the backing collection.

Finally, you have a very well formed class with clear and concise public members. Be sure to add just enough javadoc comments to allow a the class to be consumed without needing to open the code.


Video playlist interface in Swing I wrote this piece of code for a GUI i am making, which is a playlist interface. I was wondering if I could write all of this code out differently, but to the point where it functions exactly the same, as I am intrigued in learning different ways to write java to boost my flexibility and knowledge in the language.

package java;

import java.awt.*;
import java.awt.event.*;
import javax.swing.*;
import static coursework.VideoData.setRating;

public class UpdateVideos extends JFrame implements ActionListener {

    JTextField trackNo = new JTextField(2);
    JTextField newrate = new JTextField(2);
    TextArea content = new TextArea(6, 50);
    JButton apply = new JButton("Apply");

    public UpdateVideos() {
        setLayout(new BorderLayout());
        setBounds(100, 100, 400, 200);
        setTitle("Check Videos");
        setDefaultCloseOperation(JFrame.DISPOSE_ON_CLOSE);

        JPanel top = new JPanel();
        top.add(new JLabel("Enter Video Number:"));
        top.add(trackNo);
        top.add(new JLabel("Enter Rating:"));
        top.add(newrate);
        top.add(apply);
        apply.addActionListener(this);
        add("North", top);

        JPanel middle = new JPanel();

        middle.add(content);
        add("Center", middle);
        setResizable(false);
        setVisible(true);
    }

    public void actionPerformed(ActionEvent e) {

        String key = trackNo.getText();
        String name = VideoData.getName(key);
        Integer ratingnum;
        String newratenum;
        newratenum = newrate.getText();
        ratingnum = Integer.parseInt(newratenum);

        if (e.getSource() == apply) {
            if (name == null) {
                content.setText("No such video number");
            } else {
                setRating(key,ratingnum);
                content.setText(name + " - " + VideoData.getDirector(key));
                content.append("\nRating: " + stars(VideoData.getRating(key)));
                content.append("\nPlay count: " + VideoData.getPlayCount(key));
            }

        }
    }

    private String stars(int rating) {
        String stars = "";
        for (int i = 0; i < rating; ++i) {
        stars += "*";
        }
        return stars;
    }
}

At the danger of repeating myself (not to you, but generally regarding swing code):

Don't learn swing. Swing has been EOL'd for over a year now and has been officially superseded by JavaFX. If you want to learn GUI programming for thick clients in java: Go with JavaFX
Somebody has to have a tutorial that does this wrong ... UpdateVideos extends JFrame implements ActionListener is one of those lines that should make you shudder. It's a generally accepted wisdom that to write SOLID code, one should use Composition over Inheritance. This means that instead of saying "UpdateVideos is a JFrame" you should say: "UpdateVideos has a JFrame".
This is one of the things that every swing program seems to have and I find it terrifying: setVisible(true); in the constructor... This is a violation of two things. First: The principle of single responsibility (SRP), the S in SOLID. A constructor is responsible for getting the object it initializes into a usable, valid state. This does explicitly not entail making the JFrame inside the object (or the JFrame object itself) visible. Second: The principle of least surprise. Let's say you go to a coffee shop and order a coffee... would you expect the the coffee you get to pour itself down your throat? Sure you'll want to drink it sooner or later, but on your own terms. It's not the coffee's (or the JFrame's) responsibility to make sure that you drink (or show) it, but yours.

After getting this out of the way, let's talk about your code: Aside from the two coding issues I already mentioned above your code is pretty clean. The only thing that really bothers me is how many magic numbers you use.

There is a minor optimization in stars. Consider the following code:

private String stars(int rating) {
    final char[] stars = new char[rating];
    Arrays.fill(stars, '*');
    return new String(stars);
}

This avoids the overhead of concatenating Strings and instead uses a char[] to basically build the string without an explicit for-loop. This is just a marginal benefit though and you might want to keep your method structure, but use a StringBuilder instead to increase performance.


Return row or column from a 2D array I have a 2D array class that is actually mapped to a 1D vector. For example, to obtain an element at a particular position, the code is as follows:

template <typename T>
T mat <T>::at(const size_t &row, const size_t &col) const
{
  return m_values[row*ncol() + col];
}

Here's the relevant section of the class in-context:

template <typename T>
class mat
{
private:
  std::vector <T> m_values;
  size_t m_nrow, m_ncol;
public:
  mat();

  size_t ncol() const{ return m_ncol; };
  size_t nrow() const{ return m_nrow; };

  T at(const size_t &row, const size_t &col) const;
  std::vector <T> row(const size_t &row) const;
  std::vector <T> col(const size_t &col) const;
};

m_nrow and m_ncol contain the number of rows and columns of the 2D array, respectively, while m_values contains the values of the array. They're assigned in another (long) constructor.

I now need to be able to return an entire row or column. Here's how that's currently implemented:

template <typename T>
std::vector <T> mat <T>::row(const size_t &row) const
{
  std::vector <T> tmp;
  for(size_t j=0; j < ncol(); j++)
  {
    tmp.push_back(at(row, j));
  }
  return tmp;
}

template <typename T>
std::vector <T> mat <T>::col(const size_t &col) const
{
  std::vector <T> tmp;
  for(size_t i=0; i < nrow(); i++)
  {
    tmp.push_back(at(i, col));
  }
  return tmp;
}

These implementations are easy to reason about but not particularly efficient: there's the pushing back to a temporary vector and the repeated calculation of row*ncol() in at(). Is there a more idiomatic way of doing this in C++11?

Lack of modifiability

Your at(), row() and col() return copies. This means that once you construct your mat, it's permanently const. Also, for some Ts, this makes these functions unnecessarily expensive.

Prefer instead for at() to return a reference:

T& at(size_t row, size_t col);
T const& at(size_t row, size_t col) const;

Although, typically with the standard library, the at() member functions also do range checking. So consider some other non-throwing indexing mechanism (maybe operator()) and have at() defer to that one while doing bounds checking. Note that you don't need to pass the size_ts by reference-to-const, value is fine.

Now, for the other two, prefer instead:

std::vector<std::reference_wrapper<T>>       row(size_t );
std::vector<std::reference_wrapper<const T>> row(size_t ) const;
std::vector<std::reference_wrapper<T>>       col(size_t );
std::vector<std::reference_wrapper<const T>> col(size_t ) const;

reference_wrapper<T> since you can't have vector<T&>.

Constructing those rows/cols

Since you're storing everything in row order, returning a row can just use the iterator-pair constructor of std::vector:

std::vector<std::reference_wrapper<T>> row(size_t r)
{
    auto start = m_values.begin() + r * ncol();
    return {start, start + ncol()};
}

Returning a column is much more annoying, but you definitely want to use reserve() to avoid any extra allocations:

std::vector<std::reference_wrapper<T>> col(size_t c)
{
    std::vector<std::reference_wrapper<T>> result;
    result.reserve(nrows());
    for (size_t r = 0; r < nrows(); ++r) {
        result.push_back((*this)(r, c));
    }
    return result;
}

Where I'm assuming operator() is the non-throwing version of at.


Series of jQuery click events I have the following series of click events in my jQuery code:

$('#morning .up .hour').click(function () {
  morning.hour++
  update_hour(morning);
});

$('#morning .up .min').click(function () {
  morning.min++
  update_min(morning);
});

$('#morning .up .sec').click(function () {
  morning.sec++
  update_sec(morning);
});

$('#morning .down .hour').click(function () {
  morning.hour--
  update_hour(morning);
});

// ...
// morning down min
// morning down sec
//
// evening up hour
// evening up min
// evening up sec
//
// evening down hour
// evening down min
// ...

$('#evening .down .sec').click(function () {
  evening.sec--
  update_sec(evening);
});

Each alarm has an object which contains the values for the hours, minutes and seconds, along with the selector for the values (this is used in the update functions explained below):

var morning = {
  selecthour: "#morning .time .hour",
  selectmin: "#morning .time .min",
  selectsec: "#morning .time .sec",
  hour: 7,
  min: 0,
  sec: 0
};

And finally the HTML code:

Up

Up

Up

            EX:AM:PL

Down

Down

Down

The basic structure of the page is:

There are several alarms, each of which has a unique ID (currently there are two: one named morning, one named evening.)
Each alarm has an hour, minute and second field (see div.time)
Each field, in each alarm, has an up and a down arrow which increases or decreases the time
In the selectors above, the ID is the alarm, the first class is the up or down arrow, and the second class is the field which the arrow is associated with (hour/min/sec)

When a button is clicked, the following should happen:

The hour, min or sec property of the alarm object (morning or evening) is incremented or decremented by 1, based on the arrow pressed
The update_hour, update_min, or update_sec function is called, with the alarm object as its argument (morning or evening)
The update function checks and corrects any invalid input, then updates the appropriate field of the appropriate alarm in the HTML.

It takes 6 functions, each 3 lines long, for every alarm. For just the two alarms that's 36 lines of repetitive code.

Is it possible to simplify this series of jQuery .click() statements? Bonus points if it works with more than the two specified alarms. (I have an array containing all of the alarms if that helps)

Your name of properties and selectors has consistency, so make a arrays of the parts that change.

I've also added the objects you're mutating and the functions you're calling to other objects so that they can be referenced using a variable.

var am_pms = ["morning", "evening"]
var dirs = ["up", "down"]
var units = ["sec", "min", "hour"]

var am_pm_obj = {
  morning: morning,
  evening: evening
}
var update_funcs = {
  sec: update_sec,
  min: update_min,
  hour: update_hour
}

Then iterate the arrays, patching in the parts. This will cover all the functions you need to create:

am_pms.forEach(function(morn_even) {
  dirs.forEach(function(dir) {
    var offset = dir === "up" ? 1 : -1

    units.forEach(function(s_m_h) {
      $('#' + morn_even + ' .' + dir + ' .' + s_m_h).click(function () {
        am_pm_obj[morn_even][s_m_h] += offset
        update_funcs[s_m_h](am_pm_obj[morn_even])
      })
    })
  })
})

morn_even will be "morning" or "evening"
dir will be "up" or "down"
offset will be 1 or -1, which replaces ++ and --
s_m_h will be "sec", "min" or "hour"

The string variables will be used to build the selector and also to access the objects you mutate (and pass) and the functions you call, which is why the objects and functions needed to be nested in another object.


Node module using promises and the revealing module pattern Can someone give me some feedback on this pattern?

I am writing a node.js module that connects to a remote API, caches JSON, returns it as output. I have a cli wrapper script that uses the module, and then returns the output to a non-node app for a completely different purpose.

Am I using promises correctly here? Does anyone have any good module pattern + promises examples to look at? Is it proper to promisify a non-promise library and then wrap that library inside of a new promise when creating a function?

In this example saveJSON, cachedJSON, and refreshJSON all use fs or request so I am returning promises for those.

The lib is similar to this:

foo.js:

var Promise = require('bluebird')
var fs = Promise.promisifyAll(require("fs"))
var request = Promise.promisifyAll(require("request"));

function Foo() {
    var json = {}

    var emptyJSON = function() {
        return {
            "entries": {
                "vars": {}
            }
        }
    }

    var saveJSON = function(json) {
        return new Promise(function(resolve, reject) {
            fs.writeFileAsync('foo.json', JSON.stringify(json))
              .then(function() {
                  resolve(cachedJSON())
              })
              .catch (function(e) {
                  reject(e)
              })
        })
    }

    var cachedJSON = function() {
        return new Promise(function(resolve, reject) {
            fs.readFileAsync('./foo.json')
              .then(JSON.parse)
              .then(function(json) {
                  resolve(json)
              })
              .catch(SyntaxError, function(e){
                  // File exists, but has a syntax issue
                  reject("Invalid json")
              })
              .catch (function(e) {
                  // Return empty json on error
                  resolve(emptyJSON())
              })
        });
    }

    var refreshJSON = function() {
        return new Promise(function(resolve, reject) {
            request.getAsync('http://berkeleymapper.googlecode.com/svn-history/r101/trunk/web/foo.json').get(1)
                   .then(JSON.parse)
                   .then(saveJSON)
                   .then(function(json) {
                        resolve(json)
                   })
                   .catch (function(e) {
                       // Return empty json on error
                       resolve(emptyJSON())
                   })
             })
    }

    return {
        cachedJSON: cachedJSON,
        refreshJSON: refreshJSON
    }

}

module.exports = new Foo()

Where this could be utilized in a situation like this:

runFoo.js:

var foo = require("./foo")

// Refresh and return the latest json from remote
foo.refreshJSON()
   .then(function(json) {
       console.log('### Refresh cached json with remote and return ###')
       console.log(json)
   });

// Return only the cached json
foo.cachedJSON()
   .then(function(json) {
       console.log('### Return cached json only ###')
       console.log(json)
   });

Overall it looks pretty good, but I do have some notes:

You export new Foo but your Foo function isn't really a constructor. It's just a function that returns an object. This works just fine (one of JS' oddities), but personally I'd just call the function foo - lowercase, not a constructor - and use module.exports = foo();.
You're skipping most semicolons - and you're skipping them inconsistently. Even if it was super consistent, I'd still give you my standard "I prefer to add semicolons myself, rather than let JS 'fix' my code"-speech, but since it's inconsistent that goes double. It's one thing to insist on skipping semis, it's another to kinda, sorta mostly skip them.

My usual speech is that since semicolons are required in some places, and since JS does semicolon insertion, then JS actually needs those semicolons. It's not that they're optional, it's that JS is trying to make sense of incomplete code. Personally, I'd just rather just write complete code.
I'd probably prefer to tell the module where to store its "foo.json" file. From the outside, there's no indication of how it's caching things - much less that it (over)writes a file in the working directory. That's a nasty surprise.
You could simply export your Foo pseudo-constructor function (rather than its return value), and then give it a path argument when creating it.

foo = require("foo")("/path/to/some/dir"); // or a path to a specific file

I'm on the fence about the emptyJSON thing. It's not really empty: There's a structure there. This implies that some other code of yours expects that structure to be there instead of doing proper error handling. But that's not your module's problem. I'd rather forward any errors.

On the other hand, you've hard coded the URL to fetch, so your code already has some major external coupling. As long as that URL's there, it's not a big deal to also hard code a blank response - but should that URL actually be there? Depends on how generic you want this to be. You current code is almost perfectly generic in the sense that it could fetch and cache any given JSON document, so I'd be tempted to make the URL an argument to. But it would also necessitate that you remove emptyJSON and forward errors, since you no longer know what even an "empty" response looks like.
Your flow is a little odd: saveJSON resolves its promise using cachedJSON. This means that you're first serializing the JSON, writing a file, then reading the file and deserializing the JSON, even though you have all of it in memory when you call saveJSON. It's just a very roundabout way of doing things. By the way, the json variable seems pointless.
Most of all, I'd like a function that returns JSON whether or not it's cached. Especially since you "hide" errors by returning JSON in all cases. That basically means that if I call cachedJSON and there isn't any cache, I still get a JSON object. An object I'll have to dig into to figure out if it actually contains anything (which implies I know what to look for etc.). And even so, I don't know if it's "empty" because there's no cache yet, or because the request failed, or because the JSON was invalid. I'm completely in the dark.

So I basically have to call refreshJSON exclusively. I'd always have to call it at least once to ensure that something's been cached. But a CLI script doesn't have an easy way of know if it's running for the first time (and needs to refresh) or the 234th time (and can try the cache). So net result is that I'm better off always calling refreshJSON and never bother with cachedJSON at all. Which, in turn, makes it all kinda pointless - you just give the the hard disk some unnecessary exercise (doubly so with the write-then-read thing).

A caching proxy like this should primarily be transparent, with the option of going straight for the cache (and perhaps getting null or undefined back), or forcibly refreshing the cache, even if it exists. Of course, here it gets kinda tricky, since one of the 2 hard problems in programming is knowing when to invalidate your cache. Thankfully files are timestamped, so you could just use that plus a max age value to know if and when you'd need to refresh the cache. The max age could be (yet another, but optional) argument to the exported function.

Basically, I'd like to use the module like this:

var cache = require('foo')('/some/path/to/use', 'http://example.com/index.json', 3600);

// just give me the JSON; I don't care if it's cached or not
cache.getJSON().then(function (json) {
  // yay, data.
}).catch(function (err) {
  // handle errors here
});

That's of course presuming a very generic implementation. You could also just hard code the URL, file path, and max age - but your code wouldn't be very reusable. But that's fair too, of course. Depends on your needs.

As a side-note: You say your module is "similar to this". The code itself looks pretty complete, so I can't imagine what you're left out. But whatever's been left out, I obviously can't review (nor can anyone else). Hence the rule that  questions must contain the real code to be on-topic for Code Review. As mentioned, your code looks pretty close to complete, so it's fine to review. But then wouldn't it have been easier to actually post the real code?


BingWallpapers: Fetches and applies the image of the day from Bing as the wallpaper I have written a small desktop program nabeelomer/BingWallpapers in Haskell. This is my first time writing Haskell.

I was wondering how I can make the code use more functional programming features/idioms/patterns.

Any other suggestions on how to improve the code are also welcome.

-- Copyright (C) Nabeel Omer 2017
-- See the LICENSE file for the license
--

import Network.HTTP.Conduit
import System.Process
import qualified Data.ByteString.Lazy.Char8 as L8
import GHC.IO.Exception

main :: IO ()
main = do
    -- using http://muzzammil.xyz/git/bing/
    metadata <- get "http://cdn.muzzammil.xyz/bing/bing.php?format=text&cc=IN"

    -- Guaranteed by the API
    putStrLn $ (split '>' $ (lines $ L8.unpack metadata)!!1)!!1
    let url = (lines $ L8.unpack metadata)!!0
    picture <- get $ (split '>' url)!!1

    L8.writeFile "/dev/shm/Bing-Wallpaper" picture
    exitcode <- setWallpaper "/dev/shm/Bing-Wallpaper"

    print exitcode
    return ()

-- From StackOverflow
split :: Eq a => a -> [a] -> [[a]]
split d [] = []
split d s = x : split d (drop 1 y) where (x,y) = span (/= d) s

get :: String -> IO L8.ByteString
get url = simpleHttp url

setWallpaper :: String -> IO GHC.IO.Exception.ExitCode
setWallpaper uri = system $ "gsettings set org.gnome.desktop.background picture-uri file://" ++ uri

Pattern matching instead of !!, inlining of single-use names where straightforward.

import Data.List.Split

main = do
  (_:picurl:_):(_:title:_):_ <- map (wordsBy (=='>')) . lines . L8.unpack
    <$> get "http://cdn.muzzammil.xyz/bing/bing.php?format=text&cc=IN"
  putStrLn title
  L8.writeFile "/dev/shm/Bing-Wallpaper" =<< get picurl
  print =<< setWallpaper "/dev/shm/Bing-Wallpaper"


Listing with JSON or plain HTML I have a website which has reviews about games. Right now, to list the advantages (+) and disadvantages (-) of that game, there's a textarea in the admin panel with a WYSIWYG editor with only  element. So my users list their (+) with list elements :

Game is too slow

Game is very hard

I want to update it and I was wondering if it wouldn't be a better way to have input fields to add their (+) and (-) about the game and each time they want to add one, they click on a button which adds an input field. Then, when the form is submitted, I could encode the datas with JSON like :

[
    ['Game is too slow'],
    ['Game is very hard']
]

Would be like:

<input name="advantages[]" /> +

and then:

// encode
$list = json_encode($_POST['advantages']);
//decode
echo '
';
foreach($list as $item) {
    echo '
'.$item.'
';
}
echo '
';

And when I want to show the datas, I could decode them and display them as a list, which would be like the first option. My point would be to get rid of the WYSIWYG and maybe have a more flexible system.

What do you think?

First of all, there's a problem with the HTML approach you have:

That's risky (XSS most of the time)
Do you expect them to be web developers? I can be a gamer/reviewer with zero HTML knowledge.
We're writing a review, we should only write reviews, not HTML. Leave HTML to the developers.

Now, as you said, you can do JSON, and your other approach seems to be right:

Form

<form>

Advantages

  <button>Add</button> // get nextAll advantages, find the last and append after
  <input type="text" name="advantages[]" />
  <input type="text" name="advantages[]" />
  <input type="text" name="advantages[]" />

Disadvantages

  <button>Add</button> // get nextAll disadvantages, find the last and append after
  <input type="text" name="disadvantages[]" />
  <input type="text" name="disadvantages[]" />
  <input type="text" name="disadvantages[]" />
</form>

Data structure to server

{
  advantages : ['foo','bar','baz'],
  disadvantages : ['foo','bar','baz']
}


A function that moves a tile in the game of fifteen I've created a a function that will move a tile that a user specifies within the game board.

The game board can be from 3*3 up to 9*9 and is filled with values 1 to d - 1.
This function is called within another function that keeps track of "space" array and the 2d "board"array.

This function works however, I feel that I might be using too many constants? How can I make this better?

/**
* If tile borders empty space, moves tile and returns true, else
* returns false.
*
* @param   int     tile    The tile the user wants to move
* @param   int[]   space   Space keeps track of where the space is on the board
* @return  bool
*/
bool move(int tile, int space[])
{
  //check the tile to the left of the space tile so we can swap the space tile with the tile the user wanted
  if ( board[ space[0] ][ space[1] + 1 ] == tile ) {

    board[ space[0] ][ space[1] + 1 ] = 0;

    board[ space[0] ][ space[1] ] = tile;
    space[1]++;

    return true;

  //check the tile to the right of the space tile so we can swap the space tile with the tile the user selected
  }else if ( board[ space[0] ][ space[1] - 1] == tile ) {

    board[ space[0] ][ space[1] - 1 ] = 0;

    board[ space[0] ][space[1] ] = tile;
    space[1]--;

    return true;

  //check the tile to the top of the space tile so we can swap the space tile with the tile the user selected
  }else if ( board[ space[0] + 1 ][ space[1] ] == tile ) {

    board[ space[0] + 1 ][ space[1] ] = 0;

    board[ space[0] ][space[1] ] = tile;
    space[0]++;

    return true;

  //check the tile to the bottom of the space tile so we can swap the space tile with the tile the user selected
  }else if ( board[ space[0] - 1 ][ space[1] ] == tile ) {

    board[ space[0] - 1 ][ space[1] ] = 0;

    board[ space[0] ][space[1] ] = tile;
    space[0]--;

    return true;

  } else {
    //the tile does not border the space tile
    return false;
  }
}

Reading past array bounds
Each of your four checks has the possibility of reading past array bounds.  Very bad things can happen when that happens.  I will illustrate with an example:
Let board be defined as int board[3][3], and lettile = 1, space[0] = 2, space[1] = 2.  When the code reaches this first check:

  if ( board[ space[0] ][ space[1] + 1 ] == tile ) {

    board[ space[0] ][ space[1] + 1 ] = 0;

    board[ space[0] ][ space[1] ] = tile;
    space[1]++;

    return true;
  }

It will read the value of board[2][3], which is past the end of the board array.  The memory location corresponding to board[2][3] is 4 bytes past the end of board, and will probably be occupied by some other global variable in your program.  Suppose this memory location contains the value 1.  In this case, you will swap the 1 with the 0 in board[2][2], and then you will set space[1] to 3 which is outside of your board dimensions.  So now:

Whatever global variable held the value 1 will now have the value 0 for no reason.
The next time you call into this function, you will be reading even further out of bounds because space[1] is 3, which will cause you to read from board[2][4].

Simplify with loop
Instead of writing each of your four direction checks separately, you can combine them into a single loop.  Here is how I would do that (with additional bounds checking as well to fix the first problem):
bool move(int tile, int space[])
{
    int dx[4] = {  0,  0,  1, -1 };
    int dy[4] = {  1, -1,  0,  0 };

    // Check in each of the four directions.
    for (int i = 0; i < 4; i++) {
        int x = space[0] + dx[i];
        int y = space[1] + dy[i];

        // If out of bounds, skip this direction.
        if (x < 0 || x >= BOARD_SIZE || y < 0 || y >= BOARD_SIZE)
            continue;

        // If we find the tile, swap them with the empty space.
        if (board[x][y] == tile) {
            board[x][y] = 0;
            board[space[0]][space[1]] = tile;
            space[0] = x;
            space[1] = y;
            return true;
        }
    }
    return false;
}