User can add and delete customers I am using asp.net core entitiy framework.  The page displays the customers.  The user can add new customers or delete existing ones.  I am looking for ways to improve my code.

Here is my cshtml page

@Html.Partial("Index")
@model ecommerce.Models.Users

Add a new customer

<form asp-controller="Customer" asp-action="AddCustomer" method="post" role="form">

        Name
        <input asp-for="name"/>
        <button type="submit">Add</button>
        <input asp-for="created_at" value="@DateTime.Now" class="time"/>
        <input asp-for="updated_at" value="@DateTime.Now" class="time"/>
</form>
@{
        if(ViewBag.NotUnique!=null)
        {

Name is alread in DB

        }

        if(ViewBag.Empty!=null)
        {

Name is empty

        }

                        Customer Name
                        Created Date
                        Actions

        @if(ViewBag.Users != null)
        {
                foreach(var user in ViewBag.Users)
                {

                                @user.name
                                @user.created_at.ToString("MMM")  @user.created_at.ToString("dd") @user.created_at.ToString("yyyy")

                                  <form action="/DeleteCustomer/@user.id" method="post">
                                        <input type="submit" name="submit" value="Delete"/>
                                  </form>

                }
        }

}

Here is my cs file

using System;
using System.Collections.Generic;
using System.Threading.Tasks;
using Microsoft.AspNetCore.Mvc;
using ecommerce.Models;
using ecommerce.Controllers;
using System.Linq;

namespace ecommerce.Controllers
{
    public class CustomerController : Controller
    {
        // GET: /Home/

        private YourContext _context;

        public CustomerController(YourContext context)
        {
            _context = context;//connection for my db
        }

        [HttpGet]
        [Route("customers")]
        public IActionResult Customers()
        {
            ViewBag.Users = _context.Users.AsEnumerable();
            return View();
        }

        [HttpPost]
        [Route("deleteCustomer/{id}")]
        public IActionResult DeleteCustomer(int id)
        {
            var temp = _context.Users.Where(x => x.id == id).SingleOrDefault();
            _context.Users.Remove(temp);
            _context.SaveChanges();
            return RedirectToAction("Customers");
        }

        [HttpPost]
        [Route("addCustomer")]
         public IActionResult AddCustomer(String name, DateTime created_at)
        {
            Users user = new Users();
            user.name = name;
            user.created_at = created_at;
            user.updated_at = created_at;

            var temp = _context.Users.AsEnumerable().ToList();
            if(temp.Any(x => x.name == name))
            {
                ViewBag.NotUnique = true;
                return RedirectToAction("Customers");
            }
            if (user.name != null)
            {
                // Success

                _context.Users.Add(user);
                _context.SaveChanges();
                return RedirectToAction("Customers");
            }

            ViewBag.Empty = true;
            // If there were errors, re-render the view but bind the model that was submitted.
            return RedirectToAction("Customers");
        }
    }
}

You don't need to enumerate twice here

var temp = _context.Users.AsEnumerable().ToList();

You can just call .ToList().
SingleOrDefault() also accepts predicates so the first .Where() call is unnecessary, you can just do it like this :

Your code :

var temp = _context.Users.Where(x => x.id == id).SingleOrDefault();

Can become :

var temp = _context.Users.SingleOrDefault(x => x.id == id);

I don't see any reason to call .ToList() here :

    var temp = _context.Users.ToList();
    if (temp.Any(x => x.name == name))
    {
        ViewBag.NotUnique = true;
        return RedirectToAction("Customers");
    }

You are not performing any List<T> specific operations, (I assume Users is  IEnumerable<T>), you are good with a pure IEnumerable<T> here. In fact, you don't even need the temp variable, you can just directly access _context.Users.
You can have just ActionResult instead of IActionResult as a return type for your methods.

Design

Usually asp.net-mvc projects work with multiple DTO's (Data Transfer Object), which are almost identical to your actual model, but you can remove some things and add others, so that you can define the way your data will be passed around your application. Often the ViewModel is mapped in a way to it's respective DTO, to ease the creation of both objects.

Even tho simple projects probably wont need any additional DTO's.
Your models should have  proper validation instead of some check against null in your controller, which should only verify that the validation is successful. For example DataAnnotations offers quite some variety of pre-written validation attributes. You can of course extend this with your own, but it already has the core things you might need. You can find examples here


Clean up If or syntax in JavaScript Fiddle: http://jsfiddle.net/fhBd5/

So the above is working but I'm having trouble trying to come up with a clean and concise way to do some type of loop on the if or statements.

Any help is very much appreciated!

var checkPositionOnLoad = function (e) {
        var currentPos = 0;
        var listItem = document.getElement('li.current');
        var currentSelected = $('list').getChildren('li').indexOf(listItem) + 1;

        if (currentSelected <= 3) {
            return false;
        }
        if(currentSelected==4||currentSelected==5||currentSelected==6){
            currentPos = -300 * 1;
            $('list').setStyle('left', currentPos);
        }
        else if(currentSelected== 7||currentSelected==8||currentSelected==9){
            currentPos = -300 * 2;
            $('list').setStyle('left', currentPos);
        }
        else if(currentSelected==10||currentSelected==11||currentSelected==12){
            currentPos = -300 * 3;
            $('list').setStyle('left', currentPos);
        }
        else if(currentSelected==13||currentSelected==14||currentSelected==15){
            currentPos = -300 * 4;
            $('list').setStyle('left', currentPos);
        };

    };

Here is my slightly "mathematical" version of the function you have posted:

var checkPositionOnLoad = function (e) {
    var currentPos = 0;
    var listItem = document.getElement('li.current');
    var currentSelected = $('list').getChildren('li').indexOf(listItem) + 1;

    currentPos = -300 * (Math.floor((currentSelected + 2) / 3) - 1);

    $('list').setStyle('left', currentPos);
};

I will try to explain how I got to that succinct version.

I removed the duplication and wrote the function as follows:

var checkPositionOnLoad = function (e) {
    var currentPos = 0;
    var listItem = document.getElement('li.current');
    var currentSelected = $('list').getChildren('li').indexOf(listItem) + 1;

    if (currentSelected <= 3) {
        currentPos = -300 * 0;
    }
    if (currentSelected == 4 || currentSelected == 5 || currentSelected == 6) {
        currentPos = -300 * 1;
    }
    else if (currentSelected == 7 || currentSelected == 8 || currentSelected == 9) {
        currentPos = -300 * 2;
    }
    else if (currentSelected == 10 || currentSelected == 11 || currentSelected == 12) {
        currentPos = -300 * 3;
    }
    else if (currentSelected == 13 || currentSelected == 14 || currentSelected == 15) {
        currentPos = -300 * 4;
    };

    $('list').setStyle('left', currentPos);
};

Then I realized you want to figure where a given selected element falls (in which group of triplets that is {1, 2, 3} {4, 5, 6}, etc..). I played around and came up with a mathematical formula:

Math.floor((i + 2) / 3)

This basically tells you that for i = 7, the selected element falls in the third group {7, 8, 9} which is the third triplet after {1, 2, 3} and {4, 5, 6}

And the left property can be set using:

var currentPos = -300 * (Math.floor((i + 2) / 3) - 1); // (-1 to make it zero-based index)

And that's it.


Project Euler #2 with a fibSequence and result I have a background in Linguistics and my journey as an aspiring developer has just begun.

I was hoping you could give me a feedback on the solution I have come up with for the second challenge of Project Euler, and maybe explain me why many of the other solutions I have come across were wrapped inside a single function.

  Each new term in the Fibonacci sequence is generated by adding the
  previous two terms. By starting with 1 and 2, the first 10 terms will
  be:

  1, 2, 3, 5, 8, 13, 21, 34, 55, 89, ...

  By considering the terms in the Fibonacci sequence whose values do not
  exceed four million, find the sum of the even-valued terms.

let fibSequence = [0, 1];
let result = 0;

function fibonacci(array) {
  return array[array.length - 1] + array[array.length - 2];
}

while(fibSequence[fibSequence.length - 1] < 4e+6) {
  fibSequence.push(fibonacci(fibSequence));
}

fibSequence.forEach(function (number) {
  if(number % 2 === 0) {
    result += number;
  }
});

console.log(result);

Welcome to coding! You've put together a solution that works, but as you get more experienced in both programming, and Project Euler you will discover that there's almost always a better way.... (tm)

On the programming side, you need to realize that you don't need to remember previously computed values in the Fibonacci sequence (no need for the array), and in the Project Euler space, you need to realize that every 3rd value is even. Another project euler optimization is that the sequence can be thought to start from 0, 1, ...., not from 1, 2, ..... If you start at 0, 1, ... you get the sequence 0, 1, 1, 2, 3, 5, 8, ..... (this helps because the first value 0 can be treated as being even....)

Finally, it is almost always a good idea to have a single function that represents the task at hand. You have created a global variable result to store your computation, but really that should be encapsulated in a function of its own. This makes the logic obvious, and also reusable.

Putting these two aspects together, it becomes really a case of simplifying the process down to:

function evenFibSum(limit) {
  let current = 0;
  let next = 1;
  let sum = 0;
  while (current < limit) {
    // add in our current even value (started at 0)
    sum += current;
    // advance to the next even fib number
    // we already have next, so use tmp for the value after next
    const tmp = current + next;
    // the new current is the sum of the previous 2 values
    current = next + tmp;
    // the new next is the new current plus the previous value.
    next = tmp + current;
  }
  return sum;
}

With the above code, you can easily have:

console.log(evenFibSum(4e+6));

Note how with the above code, there's no global values, there's no slow array (adding new members to the array is time consuming and takes space), and also note that the function is reusable with multiple different input values.


Implementation of a Card class in Java I have just coded a Card class which gives a suit, rank and a color to a card. However, I have serious doubts that my code is not well organized and not well structured.

I would like to receive tips on how to improve my code if possible so I can learn from them.

Card class:

import java.util.Random;

public class Card {

    // Setting variables
    String cardSuit, cardRank, cardColor;
    Random generator = new Random();

    // Setting two arrays with the suits and ranks options
    String[] suits = {"hearts", "spades", "diamonds", "clubs"};
    String[] ranks = {"Ace", "2", "3", "4", "5", "6", "7", "8",
    "9", "10", "Jack", "Queen", "King"};

    int randomSuits = generator.nextInt(suits.length);
    int randomRanks = generator.nextInt(ranks.length);

    // Really not sure if I am using the Constructor the right way
     Card() {
         /* Using Random to get a random element from the suits array
            and an random element from the ranks array.
         */
         cardSuit = suits[randomSuits];
         cardRank = ranks[randomRanks];
         // Setting the color of the card in dependence of the suit
         if (cardSuit.equals("hearts") || cardSuit.equals("diamonds")) {
             cardColor = "red";
         }
         else {
             cardColor = "black";
         }
     }

     // Methods to print out the results
     public void getSuit() {
         System.out.println(cardSuit);
     }

     public void getRank() {
         System.out.println(cardRank);
     }

     public void getColor() {
         System.out.println(cardColor);
     }
}

NewCard class:

public class NewCard {
    public static void main(String[] args) {

        // Creating a new Card object
        Card card1 = new Card();
        card1.getSuit();
        card1.getRank();
        card1.getColor();
    }
}

Naming

Your Card class is currently composed of 3 fields, which represent its suit, its rank and its color:

String cardSuit, cardRank, cardColor;

Note that in the sentence above, the term used where "suit", "rank" and "color" and not "cardSuit", "cardRank" and "cardColor". What this means is that you should have

String suit, rank, color;

There is no need to repeat the class name in the fields composing it. The name given to each field should best represent what it stores. If we take the example of cardSuit then this field really represent the suit (of this card).

Consider also the case of accessing such field, you would have someCard.cardSuit with your current naming: this introduces a repetition and a long name; it is clearer to have someCard.suit.

Controlling access to the fields

When desiging a class, you should also decide on which access to give to each members. For example, a public variable will be accessible by everybody but a private variable will only be visible inside your own class.

When you are declaring the fields as

String cardSuit, cardRank, cardColor;

they actually use the default access, which means every class in the same package as of the Card class will be able to access them. This is most likely not what you want here: these are internal fields and it would be preferable to keep them private. It ensures that only modifications made inside the Card class are allowed, which goes line in line with the OOP principle: encapsulating your state without leaving the chance of others to mess it up.

So, instead, you should have:

private String cardSuit, cardRank, cardColor;

Then there is the matter of the constructor. Currently, the constructor doesn't have an access modifier also, which means it can be accessed by classes in the same package (just like above). This also means that the only classes that will be able to construct Card instances are only classes present in the same package.

Such a restriction is probably not what you want: you want everyone to be able to constructor a Card instance. So, in this case, make the constructor public:

public Card() {
    // ...
}

Constants

Instance fields are specific to each instance of a class. So when you declare

// Setting two arrays with the suits and ranks options
String[] suits = {"hearts", "spades", "diamonds", "clubs"};
String[] ranks = {"Ace", "2", "3", "4", "5", "6", "7", "8",
"9", "10", "Jack", "Queen", "King"};

you actually declare a suits and ranks array with the same content for each card instance. However, all the possible values for the suits and the ranks are not specific to any card instance. In fact, it applies for every card and it will always be the same.

In this case, it is preferable to use a constant variable; that is to say a variable defined at a single place, that every instance will share, and that cannot be modified. The typical pattern is to declare them private static final, with a name in uppercase:

private static final String[] SUITS = {"hearts", "spades", "diamonds", "clubs"};
private static final String[] RANKS = {"Ace", "2", "3", "4", "5", "6", "7", "8", "9", "10", "Jack", "Queen", "King"};

Scope of variables

To get a random suit and a random rank, you currently generate two integers as instance members of the Card class:

int randomSuits = generator.nextInt(SUITS.length);
int randomRanks = generator.nextInt(RANKS.length);

Card() {
    cardSuit = SUITS[randomSuits];
    cardRank = RANKS[randomRanks];
    // ...
}

But note this is the only place where you are using those variables.

This is generally not a good idea: it means that you just created two variables that can be used and referenced anywhere in your class, but you only need them in a specific, particular piece of code. The scope of a variable should be as minimal as possible: only declare variable where they are needed.

In this case, they are only needed inside the constructor, so declare them here:

Card() {
    int randomSuits = generator.nextInt(SUITS.length);
    int randomRanks = generator.nextInt(RANKS.length);
    cardSuit = SUITS[randomSuits];
    cardRank = RANKS[randomRanks];
    // ...
}

and, when you do that, you realize that maybe you don't even need to store them at all: they are used only once, so this would be even more easier to read:

Card() {
    cardSuit = SUITS[generator.nextInt(SUITS.length)];
    cardRank = RANKS[generator.nextInt(RANKS.length)];
    // ...
}

Accessors

Accessors, or getter / setter, is the typical way to expose your private fields to the outside world. They are typically named get followed by the name of the field.

In your class, you have

public void getSuit() {
    System.out.println(cardSuit);
}

public void getRank() {
    System.out.println(cardRank);
}

public void getColor() {
    System.out.println(cardColor);
}

but those are not getter. They do not return the value of the field. They print it instead.

Make them real getters with:

public String getSuit() {
    return cardSuit;
}

public String getRank() {
    return cardRank;
}

public String getColor() {
    return cardColor;
}

There implication is not that the Card class now has a way to let the outside world know the value of the suit, the rank and the color of a card. So it is up to the calling code to print the result with:

Card card1 = new Card();
System.out.println(card1.getSuit());
System.out.println(card1.getRank());
System.out.println(card1.getColor());

Random integers

Instances of Random should be reused. Currently, each card instance will create its own Random field with

Random generator = new Random();

It is preferable to have a single Random object that is shared. In this case, we can make it constant:

private static final Random GENERATOR = new Random();

This way, the same Random will be used for all cards.

Putting it all together

By putting all this into code, one could have:

public class Card {

    private static final Random GENERATOR = new Random();

    // Setting two arrays with the suits and ranks options
    private static final String[] SUITS = { "hearts", "spades", "diamonds", "clubs" };
    private static final String[] RANKS = { "Ace", "2", "3", "4", "5", "6", "7", "8", "9", "10", "Jack", "Queen", "King" };

    // Setting variables
    private String suit, rank, color;

    public Card() {
        suit = SUITS[GENERATOR.nextInt(SUITS.length)];
        rank = RANKS[GENERATOR.nextInt(RANKS.length)];
        // Setting the color of the card in dependence of the suit
        if (suit.equals("hearts") || suit.equals("diamonds")) {
            color = "red";
        } else {
            color = "black";
        }
    }

    public String getSuit() {
        return suit;
    }

    public String getRank() {
        return rank;
    }

    public String getColor() {
        return color;
    }
}


Trip planning algorithm I was presented with an interview question described as follows:

Receiving an int[] A of cities, where each A[i] has an appeal value. We want to plan our trip based on the highest appeal possible, taking in mind that is calculated as follows A[i] + A[j] + (i - j). This means the appeal values of 2 cities summed, plus their distance.

Function signature:

public static int solution(int[] A);

The constraints were the following:

N is an integer within [1, 100,000]
A[i] is an integer within [-1,000,000,000, 1,000,000,000]

Using the same value is a possible valid solution, so for example, if we have A = {1, 3, -3}; this should return 6 as visiting city A[0] twice gives the max appeal value A[0] + A[0] + (0 - 0) = 6.

So given that, a possible solution is the same value twice, I did not found other solution than:

int highestAppeal = 0;

for (int i = 0; i < A.length; i++) {
    for (int j = 0; j < A.length; j++) {
        int currentAppeal = A[i] + A[j] + (i - j);
        highestAppeal = currentAppeal > highestAppeal ? currentAppeal : highestAppeal;
    }
}

return highestAppeal;

This solution was marked as pretty bad. I realize that a \$O(n^2)\$ solution is far from efficient, but in this case, I did not see how to improve it. Later on, I thought about sorting A values in descending order and, for repeated values, using indexes in ascending order. But I don't see that going forward.

What would be a better and more efficient solution for this?

Thinking this more, there are three things affecting the appeal.

A[i]
A[j]
i - j

Since A[i] + A[j] is the same as A[j] + A[i], you don't need to traverse the whole array in the inner loop.

Since the third component increases the appeal only when j < i, you can restrict the inner loop to run from 0 to i.

You also need to take into account the fact that all cities may have negative appeal. Integer.MIN_VALUE is safe initial value, as the lowest valid value (2 * -1000000000 - 100000) is still greater than that.

Using Math.max would have been more readable.

    int highestAppeal = Integer.MIN_VALUE;

    for (int i = 0; i < A.length; i++) {
        for (int j = 0; j <= i; j++) {
            final int currentAppeal = A[i] + A[j] + (i - j);
            highestAppeal = Math.max(highestAppeal, currentAppeal);
        }
    }

    return highestAppeal;

And you can't overestimate the value of commented code in an interview answer.


Creating C++ classes and objects I'm brand new to programming (in general) and C++ (in particular).  I'm learning the basics of classes and objects.

I'm trying to do the following:

Create a class called X.  It has 4 member functions, with 0 arguments, 1 argument, 2 arguments and 3 arguments, respectively.
In main(), make an object of this X class that calls each of these 4 member functions.
Modify the class so that it has a single member function with all of the arguments defaulted.

#include "stdafx.h"
#include <iostream>
#include <cmath>
using namespace std;

class X
{
public:

    void setNum_1(int);
    void setNum_2(int);
    void setNum_3(int);
    void setNum_4(int);

    double getNum_1();
    double getNum_2(int num_1);
    double getNum_3(int num_1, int num_2);
    double getNum_4(int num_1, int num_2, int num_3);

private:
    int num_1;
    int num_2;
    int num_3;
    int num_4;
};

int _tmain(int argc, _TCHAR* argv[])
{
    X testObject;

    int lNum_1 = 0;
    int lNum_2 = 0;
    int lNum_3 = 0;
    int lNum_4 = 0;

    cout << endl;
    cout << "Please enter an integer: ";
    cin >> lNum_1;

    cout << "Please enter an integer: ";
    cin >> lNum_2;

    cout << "Please enter an integer: ";
    cin >> lNum_3;

    cout << "Please enter an integer: ";
    cin >> lNum_4;

    testObject.setNum_1(lNum_1);
    testObject.setNum_2(lNum_2);
    testObject.setNum_3(lNum_3);
    testObject.setNum_4(lNum_4);

    cout << endl;
    cout << "The 1st number returned is: " << testObject.getNum_1() << endl;
    cout << "The 2nd number returned is: " << testObject.getNum_2(lNum_1) << endl;
    cout << "The 3rd number returned is: " << testObject.getNum_3(lNum_1, lNum_2) << endl;
    cout << "The 4th number returned is: " << testObject.getNum_4(lNum_1, lNum_2, lNum_3) << endl;
    cout << endl;

    return 0;
}

void X::setNum_1(int n_1)
{
    num_1 = n_1;
}

void X::setNum_2(int n_2)
{
    num_2 = n_2;
}

void X::setNum_3(int n_3)
{
    num_3 = n_3;
}

void X::setNum_4(int n_4)
{
    num_4 = n_4;
}

double X::getNum_1()
{
    return num_1;
}

double X::getNum_2(int num_1)
{
    return num_1 + num_1;
}

double X::getNum_3(int num_1, int num_2)
{
    return num_1 + num_2;
}

double X::getNum_4(int num_1, int num_2, int num_3)
{
    return num_1 + num_2 + num_3;
}

I was wondering if you can recommend some ways that I can improve this code (or let me know if I've totally botched something, which is likely the case!).

First of all, you should avoid "getters" and "setters" whenever possible.  This is because they can be bad for encapsulation as they reveal the internal members.  This would be an ideal example if you were merely trying to understand getters and setters, but it's still not the best approach to learning how classes should work, especially when you start using them for more complex programs.

You should become aware of object construction.  That is, when you first create an object to hold some data at the start, it should be given to that object upon construction.

Instead of using setters, you should construct the object after getting the input:

X testObject(lNum_1, lNum_2, lNum_3, lNum_4);

This will construct the object right away with the initial values.  However, you'll first need to provide a constructor that will handle this.

You can use an initializer list:

X::X(int num_1, int num_2, int num_3, int num_4)
    : num_1(num_1)
    , num_2(num_2)
    , num_3(num_3)
    , num_4(num_4)
{}

As for the getter, it would only be useful if you need the value.  If you just need to display the members, then you can create a display() function:

void X::display() const
{
    std::cout << "num_1: " << num_1 << "\n";
    std::cout << "num_2: " << num_2 << "\n";
    std::cout << "num_3: " << num_3 << "\n";
    std::cout << "num_4: " << num_4 << "\n";
}

(Notice that I've made this function const.  Since it only displays data members and doesn't modify them, adding this keyword will prevent any accidental modifications.)

It would then be called by itself, rather than calling the getter for each member:

testObject.display();

There's much that can be said about all of this, but I've given you a basic idea on where to start.

Miscellaneous:

Try not to get into the habit of using using namespace std.  If you start writing larger programs, you may run into name-clashing issues if this is misused.  More info about that here.
Since you're not using <cmath>, it should just be removed.
_tmain() should just be main(); the former does not exist in C++.
You could leave out the command line parameters if you're not going to use them.

Otherwise, you could certainly replace your input with command line arguments:

./program_name 1 2 3 4

If you do this, you should also be sure to check the value of argc so that the program won't try to run with too few arguments:

if (argc < 5)
{
    std::cerr << "You must input four numbers!";
    return EXIT_FAILURE;
}

The program name is counted as an argument, so it should be 5.


Scraping a whole site using vba I've created a parser which is able to parse the full content of houzz.com. After going to the given address, it parses the sub-category links from the left sided bar. Each sub-category links spreads across several pages with pagination. Then it goes to the main page of each sub category links and parses the content over there. I tried to do the whole thing with my level best.

Sub HouzzFull()
Const url = "https://www.houzz.com/professionals/"
Dim http As New ServerXMLHTTP60
Dim html As New HTMLDocument, htm As New HTMLDocument, hmm As New HTMLDocument
Dim str As String, link As String, main As String
Dim topics As Object, topic As Object
Dim links As Object, post As Object, gist As HTMLHtmlElement

With http
    .Open "GET", url, False
    .send
    html.body.innerHTML = .responseText
End With
Set topics = html.getElementsByClassName("sidebar-item-label")

    For i = 1 To topics.Length - 1
    str = topics(i).href

    With http
        .Open "GET", str, False
        .send
        htm.body.innerHTML = .responseText
    End With
    Set links = htm.getElementsByClassName("pageNumber")
        For y = 0 To links.Length - IIf(links.Length > 0, 1, 0)

        If y > 0 Then
        link = links(y).href

        With http
            .Open "GET", link, False
            .send
            htm.body.innerHTML = .responseText
        End With

        End If

        Set topic = htm.getElementsByClassName("pro-title")
            For Each post In topic
            main = post.href

            With http
                .Open "GET", main, False
                .send
                hmm.body.innerHTML = .responseText
            End With

                For Each gist In hmm.getElementsByClassName("container profile-carded")
                    x = x + 1
                    With gist.getElementsByClassName("profile-full-name")
                        If .Length Then Cells(x, 1) = .Item(0).innerText
                    End With
                    With gist.getElementsByClassName("info-list-text")
                        If .Length > 1 Then Cells(x, 2) = Replace(.Item(1).innerText, "Contact:", "")
                    End With
                    With gist.getElementsByClassName("info-list-text")
                        If .Length > 2 Then Cells(x, 3) = Replace(.Item(2).innerText, "Location:", "")
                    End With
                    With gist.getElementsByClassName("click-to-call-link text-gray-light trackMe")
                        If .Length Then Cells(x, 4) = .Item(0).phone
                    End With
                    With gist.getElementsByClassName("proWebsiteLink")
                        If .Length Then Cells(x, 5) = .Item(0).href
                    End With
                Next gist
            Next post
        Next y
    Next i
End Sub

I have a couple thoughts:

Be explicit in defining your constant called url, i.e.

Private Const url As String = "https://www.houzz.com/professionals/"

Be consistent in how you group your variable declarations. You've grouped some, but not others.

I'm thinking you can have one function that returns responseText each time you do this:

With http
    .Open "GET", url, False
    .send
    html.body.innerHTML = .responseText
End With

...and for htm, and hmm

When you use With, End With but only have one line between them, the excessive nesting doesn't aid readability and defeats the purpose of using With, End With. IMHO it would be clearer to just name it out explicitly.I'm thinking something along these lines:

Set foo = gist.getElementsByClassName("profile-full-name")
If foo.Length Then Cells(x, 1) = foo.Item(0).innerText

The code inside For Each post in Topic should be indented like you indented For Each gist In hmm.getElementsByClassName("container profile-carded").


Finding the mode of an array/iterable I'm currently using SciPy's mode function to find the most occurring item in different iterable objects. I like the mode function because it works on every object type I've thrown at it (strings, floats, ints).

While this function seems reliable, it is very slow on bigger lists:

from scipy.stats import mode

li = range(50000)
li[1] = 0

%timeit mode(li)
1 loops, best of 3: 7.55 s per loop

Is there a better way to get the mode for a list? If so, would the implementation be different depending on the item type?

Originally I thought you must have been doing something wrong, but no: the scipy.stats implementation of mode scales with the number of unique elements, and so will behave very badly for your test case.

As long as your objects are hashable (which includes your three listed object types of strings, floats, and ints), then probably the simplest approach is to use collections.Counter and the most_common method:

In [33]: import scipy.stats

In [34]: li = range(50000); li[1] = 0

In [35]: scipy.stats.mode(li)
Out[35]: (array([ 0.]), array([ 2.]))

In [36]: timeit scipy.stats.mode(li)
1 loops, best of 3: 10.7 s per loop

but

In [37]: from collections import Counter

In [38]: Counter(li).most_common(1)
Out[38]: [(0, 2)]

In [39]: timeit Counter(li).most_common(1)
10 loops, best of 3: 34.1 ms per loop


Finding the longest path in an equally-weighted tree Its purpose is to find the longest path in an equally-weighted tree denoted as such:

3
1 2
2 3

The first number denotes both the number of vertices and the number of edges (3 - 1).

(defn farthest [edg current-node entry-node len]
    (let [
        farthest-key (atom current-node)
        farthest-val (atom len)
        ]
        (do
            (dorun
                (map
                    (fn [n]
                        ; check if theres a way to this node and that we're not going back
                        (if (not= n entry-node)
                            (let [new (farthest edg n current-node (+ len 1))
                                  new-val (second new)
                                  new-key (first new)]
                                (if (> new-val @farthest-val)
                                    (do
                                        (reset! farthest-val new-val)
                                        (reset! farthest-key new-key)
                                    )
                                )
                            )
                        )
                    )
                    (aget edg current-node)
                )
            )

            [@farthest-key @farthest-val]
        )
    )
)

(defn main []
    (let [edge-count (read)
          edges (atom
                    (to-array
                        (repeat edge-count [])
                    )
                )]

        (dotimes [n (- edge-count 1)]
            (let [edge-a (- (read) 1)
                  edge-b (- (read) 1)]
                (aset @edges edge-a
                    (conj (aget @edges edge-a) edge-b)
                )
                (aset @edges edge-b
                    (conj (aget @edges edge-b) edge-a)
                )
            )
        )

        (let [
            ft (farthest @edges 0 0 0)
            ftv (first ft)
            sol (farthest @edges ftv ftv 0)
            ]
            (println (second sol))
        )
    )
)

(main)

I've taken multiple attempts to optimize it, but I'm still struggling to make it run under the allowed time (1 second for a tree of up to 10000 vertices/connections). The most notable improvement was a change of the data structure from a connection matrix to a bunch of neighbourhood lists.

It is a university assignment, but I'm perfectly fine with disclosing that I got help here. I feel that I've already given it everything that I can and it simply requires more intricate knowledge about Clojure and its performance in particular, rather than a purely algorithmic change. (I know for a fact that one my colleagues managed to fit in the allocated time using the same algorithm on the same grading system).

(defn farthest [edg current-node entry-node len]
    (let [
        farthest-key (atom current-node)
        farthest-val (atom len)
        ]
        (do
            (dorun
                (map
                    (fn [n]
                        ...
                    )
                    (aget edg current-node)
                )
            )

            [@farthest-key @farthest-val]
        )
    )
)

Put )s and ] on the same line as the expression they close.
Put the first let-binding and [ on the same line.
You can use doseq instead of (dorun (map ...))
You can drop the do, since let has an implicit do around its body

E.g.,

(defn farthest [edg current-node entry-node len]
  (let [farthest-key (atom current-node)
        farthest-val (atom len)]
    (doseq [n (aget edg current-node)]
      ...)
    [@farthest-key @farthest-val]))

But also, think functional:

You don't need atoms (e.g., use reduce to find the maximum path, rather than an imperative loop).
Don't use arrays.  Use Clojure's persistent data structures instead.  For example, represent the tree as a map from node ids to a set connected nodes.


Remove every line that include and follows the second occurrence of a given pattern From a file, I am trying to remove every line that include and follows the second occurrence of a given pattern in Bash (Mac OSX). Note, that a file may have only one match of the given pattern (and so nothing need to be removed).

Here an example of file

bar
second
third
fourth
bar
sixth

and here is a code that does the job

# Find the index of the second occurrence
i=$(grep -nrH ${pattern} ${file} | awk -F':' '{if (NR==2)print $2}')

# If there was a second occurrence, then remove everything that followed it
if [[ ${i} != "" ]];then
    head -n $(( ${i} - 1 )) ${file} > tmp && mv tmp ${file};
fi

The resulting file is

bar
second
third
fourth

Is there a more elegant or faster solution?

This should be pretty speedy:

awk '/bar/ {n++} n==2 {exit} {print}' file

To pass the pattern from the shell:

awk -v p="$pattern" '$0 ~ p {n++} n==2 {exit} {print}' file


Working with user input and dictionaries inside of lists Coming to the close of week 1 learning a programming language (Python)

Looking for general review, comments, and tips

Objective of Code

The objective here was to practice pulling information from dictionaries within lists and also adding dictionaries created from user input.

Code

def make_album(artist, album, tracks =''):
    catalog = {}
    catalog['artist_name'] = artist
    catalog['album_title'] = album
    catalog['tracks'] = tracks

    return catalog

def ask_user(message):
    user_input =''
    while not user_input:
        user_input = input(message)
    return user_input

riven = make_album('Zeppelin', 'Houses of the Holy', 8)
jax = make_album('Tool', 'Lateralus', 13)
vayne = make_album('Pink Floyd', 'Dark Side of the Moon')

differents = [riven, jax, vayne]

while True:
    print("type q to quit")
    band = ask_user("Enter artist: ")
    if band == "q":
        break
    album = ask_user("Enter album: ")
    if album == 'q':
        break
    numbers = input("Enter number of tracks: ")
    if numbers == 'q':
        break
    fire = make_album(band.title(), album.title(), numbers)
    differents.append(fire)

for i in differents:
    if i['tracks']:
        print('\n' + i['album_title'] + ' by ' \
            + i['artist_name'] + ' it has '\
            + str(i['tracks']) +' tracks.')
    else:
        print('\n' +  i['album_title'] + ' by ' + i['artist_name'] \
            + '.')

Output

vash@localhost:~/pcc/8$ python3 lot2learn.py
type q to quit
Enter artist: circa survive
Enter album: juturna
Enter number of tracks: 11
type q to quit
Enter artist: dance gavin dance
Enter album: happiness
Enter number of tracks:
type q to quit
Enter artist: q

Houses of the Holy by Zeppelin it has 8 tracks.

Lateralus by Tool it has 13 tracks.

Dark Side of the Moon by Pink Floyd.

Juturna by Circa Survive it has 11 tracks.

Happiness by Dance Gavin Dance .
(xenial)vash@localhost:~/pcc/8$

You should put all code that isn't in a function in a if __name__ == '__main__' guard. So that it doesn't run when it shouldn't.
You can simplify quitting by using exceptions. If you make a function that raises say KeyboardInterupt if the input is 'q' then you can reduce the amount of code needed.
I like to not use variables when they're not needed, or increase readability, and so you may want to move riven straight into the creation of differents.
You can use f-strings or str.format to build your prints for you:

>>> i = {'title': 'title', 'album': 'album'}
>>> i['title'] + ' by ' + i['album']
'title by album'
>>> "{i[title]} by {i[album]}".format(i=i)
'title by album'
# Python 3.6+
>>> f"{i['title']} by {i['album']}"
'title by album'

You can simplify make_album by using dictionary sugar:

def make_album(artist, album, tracks =''):
    return {
        'artist_name': artist,
        'album_title': album,
        'tracks': tracks
    }

You can alturnatly use collections.namedtuple:

# Python 3.7
Album = collections.namedtuple('Album', 'artist album tracks', defaults=('',))

# Otherwise
Album = collections.namedtuple('Album', 'artist album tracks')

def make_album(artist, album, tracks=''):
    return Album(artist, album, tracks)

I'd create a couple more functions. But honestly your code is pretty great. Half my suggestions I wouldn't expect a beginner to use.

import collections

Album = collections.namedtuple('Album', 'artist album tracks', defaults=('',))

def ask_user(message):
    user_input = None
    while not user_input:
        user_input = input(message)
    return user_input

def check_quit(user_input):
    if user_input == 'q':
        raise KeyboardInterupt()
    return user_input

def get_additional_albums():
    albums = []
    try:
        print("type q to quit")
        while True:
            albums.append(Album(
                cheack_quit(ask_user("Enter artist: ")).title(),
                cheack_quit(ask_user("Enter album: ")).title(),
                cheack_quit(input("Enter number of tracks: "))
            ))
    except KeyboardInterupt:
        pass
    return albums

if __name__ == '__main__':
    differents = [
        Album('Zeppelin', 'Houses of the Holy', 8),
        Album('Tool', 'Lateralus', 13),
        Album('Pink Floyd', 'Dark Side of the Moon')
    ]
    differents += get_additional_albums()

    for i in differents:
        output = f'{i.title} by {i.album}'
        if i.tracks:
            output = f'{output} it has {i.tracks} tracks'
        print(f'\n{output}.')


codekata: Binary search on an int array I'm following Karate Chop kata for a binary search. I really wanted to avoid built in functionality within the language. I'm pretty new to C# so any recommendations is greatly appreciated.

Spec: Binary chop on an int array to find the index.

  A binary chop (sometimes called the more prosaic binary search) finds
  the position of value in a sorted array of values. It achieves some
  efficiency by halving the number of items under consideration each
  time it probes the values: in the first pass it determines whether the
  required value is in the top or the bottom half of the list of values.
  In the second pass in considers only this half, again dividing it in
  to two. It stops when it finds the value it is looking for, or when it
  runs out of array to search. Binary searches are a favorite of CS
  lecturers.

http://codekata.com/kata/kata02-karate-chop/

public static int Chop(int val, int[] subject)
{
    // if the array count is odd
    if (subject.Length % 2 != 0)
    {
        // if the last index is the value, we're done
        if (subject[subject.Length - 1] == val)
        {
            return subject.Length - 1;
        }
        // remove the last index so that the array length is even
        subject = new ArraySegment<int>(subject, 0, subject.Length - 1).ToArray();
    }
    // get the mid point
    var mid = subject.Length / 2;
    if (mid > 4)
    {
        int[] chopped = new int[mid];
        Array.Copy(subject, chopped, mid);
        if (chopped[chopped.Length - 1] > val)
        {
            Chop(val, chopped);
        }
        else
        {
            var segment = new ArraySegment<int>(subject, mid, subject.Length - mid);
            Chop(val, segment.ToArray());
        }
    }
    int index = -1;

    for (int j = 0; j < subject.Length; j++)
    {
        if (val == subject[j])
        {
            if (index != -1)
            {
                duplicateIndexes.Add(index);
                duplicateIndexes.Add(j);
            }
            index = j;
        }
    }
    return index;
}

Review

You make a special branch to distinguish odd from even array lengths. if (subject.Length % 2 != 0) This is not required because getting the mid of an array can just be retrieved as (end - start) / 2. In case of an even array, you get the left index of the mid index pair. This is not a bad thing, we are not calculating a median here.
You write alot of comments // if the array count is odd. Because you are a beginner, this is ok. But try writing code in a way these obvious comments are not required. Pick good method and variable names.
You have taken the chopping part a bit literally. Array.Copy(subject, chopped, mid); is an expensive operation. Perhaps you could have worked on the source array, but played around with the start and end index instead.

Alternative

Use an algorithm that keeps refining start and end index until a match is found. This way, you don't need to make new array segments. This works for both odd and even arrays.

public static int Chop(int value, int[] subject)
{
    subject = subject ?? throw new ArgumentNullException(nameof(subject));
    if (subject.Length == 0) return -1;
    var start = 0;
    var end = subject.Length - 1;
    return Chop(value, subject, start, end);
}

private static int Chop(int value, int[] subject, int start, int end)
{
    var span = end - start;
    var mid = start + span / 2

    if (subject[mid] == value) return mid;
    if (span == 0)  return -1;

    if (subject[mid] > value)
    {
        return Chop(value, subject, start, mid);
    }

    return Chop(value, subject, mid + 1, end);
}

Test

static void Main(string[] args)
{
    Console.WriteLine(Chop(3, new[] { 2 }));
    Console.WriteLine(Chop(3, new[] { 3 }));
    Console.WriteLine(Chop(3, new[] { 3, 4 }));
    Console.WriteLine(Chop(3, new[] { -1, 3, 4 }));
    Console.WriteLine(Chop(3, new[] { 1, 2, 3, 4 }));
    Console.WriteLine(Chop(3, new int[0]));
    Console.ReadKey();
}


Grouping string to list of strings The following is function that I have written to group a string to groups, based on whether there's consecutive repeated occurrence. Fop example AAAABBBBAAB is grouped as [A+,B+,A+,B]. Is it possible to make below code more pythonic?  If yes, how?

def create_groups(alphabets):
    """ function group the alphabets to list of A(+)s and B(+)s """
    index = 1
    current = alphabets[0]
    count = 0
    groups = []
    accumulate = False
    while index < len(alphabets):
        if current == alphabets[index]:
            count += 1
            accumulate = True
        else:
            accumulate = False
        if accumulate == False or index == len(alphabets)-1:
            group_indicator = current + '+' if count > 0 else current
            groups.append(group_indicator)
            current = alphabets[index]
            count = 0
        index += 1
    return groups

First of all, your method is not really correct: for AAAABBBBAAB it returns [A+, B+, A+] instead of the required [A+, B+, A+, B]. That's because the last group is never added to the list of groups.

This is not very Pythonic:

if accumulate == False:

Write it this way instead:

if not accumulate:

Also, instead of iterating over the "alphabet" using indexes, it would be more Pythonic to rewrite to iterate over each letter, in the style for letter in alphabet.

"alphabets" is not a good name. It seems letters would be better.

The algorithm can be simplified, and you could eliminate several intermediary variables:

def create_groups(letters):
    """ function group the alphabets to list of A(+)s and B(+)s """
    prev = letters[0]
    count = 0
    groups = []
    for current in letters[1:] + '\0':
        if current == prev:
            count += 1
        else:
            group_indicator = prev + '+' if count > 0 else prev
            groups.append(group_indicator)
            count = 0
        prev = current
    return groups

In the for loop, I appended '\0' to the end, as a dirty trick to make the loop do one more iteration to append the last letter group to groups. For this to work, it must be a character that's different from the last letter in letters.

The above is sort of a "naive" solution, in the sense that probably there is a Python library that can do this easier. Kinda like what @jonrsharpe suggested, but he didn't complete the solution of converting [['A', 'A', 'A', 'A'], ['B', 'B', 'B', 'B'], ['A', 'A'], ['B']] in the format that you need. Based on his solution, you could do something like this:

from itertools import groupby

def create_groups(letters):
    return [x + '+' if list(g)[1:] else x for x, g in groupby(letters, str)]

What I don't like about this is the way we put the letters in a list just to know if there are 2 or more of them (the list(g)[1:] step). There might be a better way.


ASP.net proper login - logout classes - control login status Would you have any suggestions improvements for the below functions classes ?

Ok here how do i make a registered member login

    HttpCookie LoginInfo = new HttpCookie("LoginInfo");
    LoginInfo.Values["UserName"] = srUserName;
    LoginInfo.Values["Password"] = srPassword;
    LoginInfo.Values["selectedLanguage"] = srSelectedLanguage;
    Response.Cookies.Add(LoginInfo);

Here how do i check visitor is logged in or not

public static void controlOfLoginStatus()
{
    string srQuery = "";
    string srUserName = "";
    string srPassword = "";
    string srLang = "";

    if (HttpContext.Current.Session["UserId"] == null)
    {
        if (HttpContext.Current.Request.Cookies["LoginInfo"] != null)
        {
            try
            {
                srUserName = HttpContext.Current.Request.Cookies["LoginInfo"]["UserName"].ToString();
                srPassword = HttpContext.Current.Request.Cookies["LoginInfo"]["Password"].ToString();
                srLang = HttpContext.Current.Request.Cookies["LoginInfo"]["selectedLanguage"].ToString();
            }
            catch
            {

            }
        }
        string srUserIdTemp = csPublicFunctions.ReturnUserIdUsernamePassword(srUserName, srPassword);
        if (srUserIdTemp == "0")
        {
            HttpContext.Current.Session.Clear();
            HttpContext.Current.Session.Abandon();
            HttpContext.Current.Response.Redirect("Login");
        }
        else
        {
            csPublicFunctions.insertIntoOnlineUsers(srUserIdTemp, HttpContext.Current.Session.SessionID);
            HttpContext.Current.Session["UserId"] = srUserIdTemp;
            if (HttpContext.Current.Session["lang"] == null)
                HttpContext.Current.Session["lang"] = srLang;
        }
    }

    srQuery = "SELECT UserId " +
     " FROM BannedUsers" +
     " WHERE UserId = " + HttpContext.Current.Session["UserId"].ToString();
    using (DataTable dtTemp = DbConnection.db_Select_DataTable(srQuery))
    {
        if (dtTemp.Rows.Count > 0)
        {
            HttpContext.Current.Response.Redirect("exit.aspx");
        }
    }
}

Here how do i log-out

public static void exitLogout()
{
    string srQuery = "delete from OnlineUsers where UserId=" + HttpContext.Current.Session["UserId"].ToString();
    DbConnection.db_Update_Delete_Query(srQuery);

    try
    {
        HttpContext.Current.Session["UserId"] = "0";
        HttpContext.Current.Session.Clear();
        HttpContext.Current.Session.Abandon();
    }
    catch
    {

    }

    try
    {
        HttpCookie LoginInfo = new HttpCookie("LoginInfo");
        LoginInfo.Values["UserName"] = "21412zxcvzxc343245243vvc";
        LoginInfo.Values["Password"] = "21412zxcvzxc343245243vvc";
        LoginInfo.Values["selectedLanguage"] = "en";
        HttpContext.Current.Response.Cookies.Add(LoginInfo);
    }
    catch
    {
    }
}

csPublicFunctions.ReturnUserIdUsernamePassword uses parametrized queries so no possible risk of SQL injection

In addition to losing the hungarian notation there are a few things worth mentioning:

Don't use try-catch construction with empty 'catch' clause. It is almost always a bad idea to hide an exception if it was thrown.
It would be better to check explicitly if the key is in the collection using appropriate methods. BTW exception handling is quite expensive speedwise and explicit checking will work faster.
Try to refactor your controlOfLoginStatus() method into several distinct functions with more meaningful purpose.
Refactor your database code into separate class.
Use parameterized queries while working with database. It will save you from sql injections and it will make your queries run faster.


Interactive Tower of Hanoi I created this program some time ago. It shows you how to solve tower of hanoi for n disks, step by step.

Here's the code:

from os import system
from colorama import Fore, Style

clear = lambda: system('cls')

class TowerOfHanoi:
    def __init__(self, n, r1, r2, r3):
        self.n = n

        self.r1 = r1
        self.r2 = r2
        self.r3 = r3

        clear()
        print('Initial: ')
        self.parser(a, b, c)

        input('\nEnter To Continue...')

        self.solve()

    def parser(self, a1, b1, c1):
        x, y, z = [], [], []

        a1, b1, c1 = [0] * (self.n - len(a1)) + a1, \
                     [0] * (self.n - len(b1)) + b1, \
                     [0] * (self.n - len(c1)) + c1

        m = self.n

        for i in a1:
            s = ' ' * (m - i + 1)
            h = Fore.RED + s + '██' * i + s + Style.RESET_ALL

            x.append(h)

        for i in b1:
            s = ' ' * (m - i + 1)
            h = Fore.RED + s + '██' * i + s + Style.RESET_ALL

            y.append(h)

        for i in c1:
            s = ' ' * (m - i + 1)
            h = Fore.RED + s + '██' * i + s + Style.RESET_ALL

            z.append(h)

        print(' ' * self.n + str(self.r1)
              + ' ' * (self.n * 2 + 1) + str(self.r2)
              + ' ' * (self.n * 2 + 1) + str(self.r3))

        print()

        print('\n'.join(i + ' ' * (self.n - len(i))
                + j + ' ' * (self.n - len(j))
                + k for i, j, k in zip(x, y, z)))
        print()

    def solve(self, n = None, r1 = None, r2 = None, r3 = None, x = 0, y = 1, z = 2):
        if n is None:
            n = self.n

        if r1 is None:
            r1 = self.r1
        if r2 is None:
            r2 = self.r2
        if r3 is None:
            r3 = self.r3

        if n == 1:
            clear()
            print("Move disk 1 from rod", r1, "to rod", r2)
            print()
            [a, b, c][y].insert\
                (0, [a, b, c][x].pop(0))
            self.parser(a, b, c)
            return

        self.solve(n - 1, r1, r3, r2, x, z, y)
        input('\nEnter To Continue...')

        clear()

        print("Move disk", n, "From Rod", r1, "To Rod", r2)
        print()

        [a, b, c][y].insert\
            (0, [a, b, c][x].pop(0))

        self.parser(a, b, c)

        input('\nEnter To Continue...')
        print()
        self.solve(n - 1, r3, r2, r1, z, y, x)

print('Welcome to Tower of Hanoi!')
print('You have some number of rings of increasing length')
print('Like this:')
print()
print(Fore.RED + '  ██  ' + Style.RESET_ALL)
print(Fore.RED + ' ████ ' + Style.RESET_ALL)
print(Fore.RED + '██████' + Style.RESET_ALL)
print()
print('The rules are simple: ')
print('  * You can move a disk from a rod to another rod')
print('      if the current disk is smaller than the topmost disk in the rod you are moving')
print()
print('  * The goal is to move all disks from rod 1 to rod 2')
print()
print('This program shows you how to move the disks from rod 1 to rod 2')
print()

while True:
    input('Press enter to start!')
    print()

    n1 = int(input('Enter number of rings: '))

    a = [i for i in range(1, n1 + 1)]
    b = []
    c = []

    toh = TowerOfHanoi(n1, 1, 2, 3)

    print()
    play_again = input('Do you want to play again? <Y>es or <N>o').lower()

    while play_again != 'y' or play_again != 'n':
        play_again = input('Do you want to play again? <Y>es or <N>o').lower()

    if play_again == 'n':
        print('Bye!')
        break

This code seems a bit lengthy, as well as ugly. How do I improve it?

Any tips are welcome!

Bugs

Colorama Not Initialized

Colorama should be initialized, to ensure proper operation:

import colorama

colorama.init()

Clear Screen

  sh: cls: command not found

On non-Windows machines, cls is not defined

Instead, simply use colorama to clear the screen.

def clear():
    print(colorama.ansi.clear_screen())

Play again

Do you want to play again? <Y>es or <N>oY
Do you want to play again? <Y>es or <N>oN
Do you want to play again? <Y>es or <N>oy
Do you want to play again? <Y>es or <N>on
Do you want to play again? <Y>es or <N>o

No answer seems to provide the expected behaviour, due to:

while play_again != 'y' or play_again != 'n':

always being True.  You need the and conjunction, not or.

Unused variables

toh = TowerOfHanoi(n1, 1, 2, 3)

toh is never used after this point.  So, why store it in a variable?

Global variables

a = [i for i in range(1, n1 + 1)]
b = []
c = []

toh = TowerOfHanoi(n1, 1, 2, 3)

a, b, and c are used inside the TowerOfHanoi class, but are not passed to the class.  Instead they are globals variables that must be properly initialized corresponding to the value n1 passed to the TowerOfHanoi constructor.

These lists should be created by the class itself, based on the argument passed to the constructor.  Moreover, they should be members of the class, not globals.

Code Duplication

This code:

    for i in a1:
        s = ' ' * (m - i + 1)
        h = Fore.RED + s + '██' * i + s + Style.RESET_ALL

        x.append(h)

is replicated 3 times, with a1 changing to b1 and c1, and x changing to y and z.  Perhaps you could refactor this into its own function, taking the rod list as input, and generating & returning the lists internally?

    def rod_images(self, rod):
        # Fill rod with required 0's at top
        rod = [0] * (self.n - len(rod)) + rod

        image_rows = []
        for i in rod:
            spaces = ' ' * (self.n - i + 1)
            row = Fore.RED + spaces + '██' * i + spaces + Style.RESET_ALL
            image_rows.append(row)

        return image_rows

Then you could simply write:

    def parser(self, a1, b1, c1):

        x = self.rod_images(a1)
        y = self.rod_images(b1)
        z = self.rod_images(c1)

        # ... etc ...

Zero, One & Many

In programming, there are 3 important numbers: zero, one, and many.  How many rods do you have?  More than one, so ... you have many rods.  Many items are stored in containers, such as lists, not individual variables.  So instead of this:

    self.r1 = r1
    self.r2 = r2
    self.r3 = r3

write something like:

    self.rods = [r1, r2, r3]

Then, instead of

    print(' ' * self.n + str(self.r1)
          + ' ' * (self.n * 2 + 1) + str(self.r2)
          + ' ' * (self.n * 2 + 1) + str(self.r3))

you can take advantage of the fact that you are doing something with each item in the list, in order.  In this case, you are converting each item to a string, and then joining them together with a bunch of spaces in-between:

    print(' ' * self.n
          + (' ' * (self.n * 2 + 1)).join(str(rod) for rod in self.rods)

But ... that still looks ugly.  What you are really doing is printing out 3 values, each centred in a n * 2 + 2 character wide field:

    width = self.n * 2 + 2
    print((f"{{:^{width}}}"*3).format(*self.rods))

With 3 disks, this creates the format string "{:^8}{:^8}{:^8}" which centres the 3 values each in 8 character wide fields.

A similar method could be used to centre the discs when printing, as long as the colour/style codes are removed from the disc "values" and moved to the format string itself, so they don't mess up the character counts for automatic centring.

Unnecessary Default Arguments

Calling solve(), with no arguments, is only done from __init__().  So the only time the default arguments are used is that one call.  So why complicate the method with default arguments which must be set via a number of if _ is None: tests?  Simply use:

self.solve(self.n, self.r1, self.r2, self.r3, 0, 1, 2)

in the __init__() method, and remove the default arguments and default argument substitution code.


Showing statistics of popular JavaScript frameworks from GitHub API I am currently working on a simple app to check the stats of popular javascript frameworks. For this I am using the Github API to get certain data sets and have come close to finishing my project.

const frameworks = [
  {
    name: "angular",
    logo: "img/angular.png",
    url: "https://api.github.com/repos/angular/angular.js"
  }
  /*
  {
    name: "ember",
    logo: "img/ember.png",
    url: "https://api.github.com/repos/emberjs/ember.js"
  },
  {
    name: "react",
    logo: "img/react.png",
    url: "https://api.github.com/repos/facebook/react"
  },
  {
    name: "vue",
    logo: "img/vue.png",
    url: "https://api.github.com/repos/vuejs/vue"
  }
  */
];

const frameworkData = url => {
  return fetch(url)
    .then(resp => {
      return resp.json();
    })
    .then(data => {
      return {
        data: {
          name: data.name,
          forks: data.forks_count,
          stars: data.stargazers_count,
          issues: data.open_issues_count
        }
      };
    });
};

const setup = () => {
  let now = new Date();
  const row = document.getElementById("row");

  frameworks.forEach(framework => {
    const promises = [];
    promises.push(frameworkData(framework.url));

    Promise.all(promises).then(data => {
      const obj = data[0].data;
      console.log(obj);
      const card = `

${framework.name}

Watchers: ${obj.forks}

Stars: ${obj.stars}

Commits: ${obj.issues}

`;
      row.innerHTML += card;
    });
  });
};

setup();

Being this is one of my first javascript 'apps' I feel it was done to the best of my ability. However, I feel there is a code smell in here that I have been trying to figure out how to fix but have not.

As you may be able to see in the frameworks for each loop, I am settings this variable const obj = data[0].data;. To me this is strange and I could not seem to find away around it. This is done because the single object for that API call is being returned in an array and I was not sure how to get that data other than calling the first index on the array since it will always be 0. Is there a better way to do this? How was my first javascript app?

Your questions

  How was my first javascript app?

Overall the code seems somewhat fine for a first attempt (barring the flaw mentioned in the first section below). It isn't difficult to read though it does have a few extra levels of nesting which could be cleaned up with abstraction code functions out to named functions.

  Is there a better way to do this?

As was suggested by others, there isn't really a point to returning the data nested under a key "data". Refer also to the other suggestions below for simplifying the code.

Other Feedback/Suggestions

Likely implementation flaw

frameworks.forEach(framework => {
  const promises = [];
  promises.push(frameworkData(framework.url));
  Promise.all(promises)

This code will call Promises.all() passing an array with a single promise, which seems pointless. While there is no rule that says there has to be two or more promises it basically defeats the purpose.

Presumably you meant to push the promises into an array and pass the array with all promises to the call to Promise.all().

Creating array of promises

One could use Array.prototype.map() to construct the array.

const promises = frameworks.map(framework => frameworkData(framework.url));

utilizing data

If you take the advice above, the first argument in a callback to .then() chained to Promise.all() should be an array of the data objects returned by the last promise callback within each call to frameworkData(), and there won’t be any reference to ‘frameworkso you might either need to pass that object along through the call toframeworkData` or look it up by index.

Template inline

The template literal used to define card could be moved out to a template tag with slots  or a

Then that could be referenced via the JavaScript:

const cardTemplate = document.getElementById('card-template').innerHTML;

You would then need interpolate the string, and unfortunately that isn't in a template literal anymore. However, an approach like outlined in this post should suffice.

The following function added in the JavaScript like this:

String.prototype.interpolate = function(params) {
  const names = Object.keys(params);
  const vals = Object.values(params);
  return new Function(...names, `return \`${this}\`;`)(...vals);
}

Would allow for a line like this:

row.innerHTML += cardTemplate.interpolate(data);

Simplifying single line returns in arrow statements

Arrow functions that simply have one line in the block, like:

resp => {
  return resp.json();
}

Can be simplified to a single line without the block and return:

resp => resp.json()

Unused variable

The following variable doesn't appear to be utilized:

let now = new Date();

It can be removed unless you plan to utilize it in the future. If the value never changes, it would be wise to declare it with const.

Putting it all together

See simplified code below.

String.prototype.interpolate = function(params) {
  const names = Object.keys(params);
  const vals = Object.values(params);
  return new Function(...names, `return \`${this}\`;`)(...vals);
}
const frameworks = [{
    name: "angular",
    logo: "img/angular.png",
    url: "https://api.github.com/repos/angular/angular.js"
  },
  {
    name: "ember",
    logo: "img/ember.png",
    url: "https://api.github.com/repos/emberjs/ember.js"
  },
  {
    name: "react",
    logo: "img/react.png",
    url: "https://api.github.com/repos/facebook/react"
  },
  {
    name: "vue",
    logo: "img/vue.png",
    url: "https://api.github.com/repos/vuejs/vue"
  }
];

const frameworkData = framework => {
  return fetch(framework.url)
    .then(resp => resp.json())
    .then(data => {
      return {
          logo: framework.logo,
          name: data.name,
          forks: data.forks_count,
          stars: data.stargazers_count,
          issues: data.open_issues_count
      };
    });
};

const setup = () => {
  const row = document.getElementById("row");
  const cardTemplate = document.getElementById('card-template').innerHTML;
  const promises = frameworks.map(framework => frameworkData(framework));
  Promise.all(promises).then(data => {
    row.innerHTML = data.map(obj => cardTemplate.interpolate(obj));
  });
};

document.addEventListener('DOMContentLoaded', setup);


Set of radio buttons I wrote a quick function to create a set of radio buttons, three per table row. I would like some feedback about how I can shorten this bit of code using algebraic functions, or different jQuery functionality.

var arr = [ "earth", "mars", "jupiter", "saturn", "venus", "argus", "pluto", "janus", "canary", "orange", "butter pineapple", "eventual bliss"];

var obj = { one:"earth", two:"mars", three:"jupiter", four:"saturn", five:"venus" };

var start = ""; end = "";
var count = 0;
$("#radio_container").append("
");
    $.each(arr, function(i, val) {

        if(count == 3)
        {
            start = "
";
            end = "
"
            count= 0;
        }

        $("#radio_container").append(start);
        $("#radio_container").append("<input name='finalcategory' type='radio' value='"+val+"'></input>	"+val+"	");
        $("#radio_container").append(end);
        count++;
        start = "";
        end = "";

    });
$("#radio_container").append("
");

Here are some points to consider:

Your end variable is declared without a var, so that it's leaking into the global namespace. Don't do that.
As it stands now, your obj isn't used anywhere in the code. I'll assume it's not applicable here.
Instead of manually keeping count of your iteration, use the % (Modulus) operator to calculate your rows.
jQuery's append method doesn't deal with code fragments the way you seem to think it does. append("
") actually appends a whole table, not just an opening tag (that's not even possible).
Access to the DOM is not free. Every time you hit the DOM you incur some overhead. Try keeping DOM modification to a minimum.
Unlike others, when dealing with a simple HTML construct such as this, I prefer to build my fragment from a concatenated string since it's much faster.

With all that in mind, here's how I would do it:

var arr = ["earth", "mars", "jupiter", "saturn", "venus", "argus", "pluto", "janus", "canary", "orange", "butter pineapple", "eventual bliss"],
    snippet = '

';

$.each(arr, function(i, val) {

    if (i % 3 == 0) snippet += '

';

    snippet += '<input name="finalcategory" type="radio" value="' + val + '" />	' +
               '' + val + '	';
});

snippet += '

';

$("#radio_container").append( snippet );

See it here in action: http://jsfiddle.net/DWcSN/

The code above works, but the text is not associated with the radio button in any way.

To remedy that, consider wrapping labels around your radio buttons:

snippet += '' +
               '<input name="finalcategory" type="radio" value="' + val + '" />' +
               val +
           '	';

See it here in action: http://jsfiddle.net/DWcSN/1/ (clicking the text activates the corresponding button).

P.S. style3 is not very descriptive. You should consider coming up with a more intuitive naming convention.


Project Euler 5: Smallest multiple in Functional Programming (fp) I wanted to practice functional programming (fp) without using any library but using vanilla JS only. So I took the a problem from project euler:

/*jshint esversion: 6*/
(function () {
  'use strict';
  const leastCommonMultiple = (n, multiple, max) => n === max ?
    multiple :
    leastCommonMultiple(n + 1, multiple * (n + 1) / greatestCommonDivisor(
      multiple, (n + 1)), max);

  const greatestCommonDivisor = (a, b) => 0 === b ? a : greatestCommonDivisor(
    b, a % b);

  console.log("Solution: ", leastCommonMultiple(1, 1, 20));
})();

Is there a way to write leastCommonMultiple with less parameters and still be consistent with fp? Any other improvement suggestions are welcomed as well!

Well, this is Code Review, so let's be picky.  First, remember that superLongVariableNamesThatTakeSeveralMinutesToWrite are for IDE-assisted object-oriented enterprise programming in Java where a recursive call is so unusual you'd record it as a Very Special Day in your diary.

Functional programmers don't have time for that nonsense, so start by writing:

const gcd = (a,b) => 0 === b ? a : gcd(b, a % b)

When it comes to writing lcm, a functional programmer probably wouldn't write a version that calculates the (1) least common multiple of (2) several values produced by (3) iterating over integers because that's three different concepts, and functional programmers' heads are so crammed with greek letters and category theory that they can only concentrate on one thing at a time.  So, we'll have to be content with an lcm function that operates on only two numbers:

const lcm = (a,b) => a * b / gcd(a,b)

Second, no self-respecting functional programmer would be caught dead without a fold function (AKA reduce), whether she needed it or not, so let's be sure to write one now and worry about finding a way to use it later:

const reduce = (f,xs) => 1 === length(xs) ? xs[0]
      : f(xs[0], reduce(f,xs.slice(1)))

A couple tests ensure it's working:

> reduce((x,y) => x+y, [1,2,3,4,5])
15
> reduce((x,y) => x*y, [1])
1
> reduce((x,y) => x*y, [])
RangeError: Maximum call stack size exceeded
   at reduce (repl:1:72)
   at reduce (repl:1:60)
   ...

Oh, well, close enough.  we'll leave unit testing to some junior programmer (or an intern, who would probaly be clever enough to realize that Array.reduce is already vanilla JS).

Third, since we agree that explicit iteration is for imperative programming chumps, we'll write a tail recursive function for generating arrays of integer ranges:

const seq = (m,n) => m > n ? [] : [i].concat(step(m+1,n))

and a few tests:

> seq(5,10)
[5,6,7,8,9,10]
> seq(1,20)
[1,2,3,4,5,6,7,8,9,10,11,12,13,14,15,16,17,18,19,20]
> seq(1,1000000)
RangeError: Maximum call stack size exceeded
>

Oh, yeah, we forgot to make it tail recursive...  Again, we really need to hire that junior programmer.

ANYWAY, armed with this selection of functions:

const gcd = (a,b) => 0 === b ? a : gcd(b, a % b)
const lcm = (a,b) => a * b / gcd(a,b)
const seq = (m,n) => m > n ? [] : [m].concat(seq(m+1,n))
const reduce = (f,xs) => 1 === xs.length ? xs[0]
      : f(xs[0], reduce(f,xs.slice(1)))

the program really writes itself:

> console.log(reduce(lcm, seq(1, 20))
232792560
>

And seriously, while I was writing this up a little tongue-in-cheek, this is really how functional programming is supposed to work.

Instead of writing a function leastCommonMultiple with a long name that tries to do multiple things:

count up from 1 to max
maintain an accumulator of results
calculate pairwise LCMs using the greatestCommonDivsor helper

you:

write functions with short names like lcm that do simple things like calculating the LCM of two numbers
take algorithms like folding a list of values into an accumulator using a binary operator and abstract them into higher order, reusable functions like reduce
compose these simple functions together to write more complex programs

As a side note, from a practical standpoint, my seq and reduce leave much to be desired.  In Javascript, building an array recursively with concat is not a sane way to create an array of sequential integers (and my reduce is really no better).  Of course, that's why we have libraries like Lodash, to provide optimized implementations of these common building blocks.

Which brings me to my last point.  If you're an FP beginner, I don't think you're likely to benefit much from trying to write FP code in vanilla Javascript without any external libraries.  The main reason these external libraries exist is that FP programming in vanilla Javascript is tedious, inefficient, error-prone, unpleasant, and a host of other distasteful adjectives.

A programmer who is already experienced with FP can write decent FP programs in vanilla Javascript (but is probably smart enough not to try).  Programmers who are trying to improve their FP skills are better off using an FP-friendly library that will encourage writing good FP code.

If you want to get better at FP in Javascript, install a library that supports FP programming, work through its tutorials, and make heavy use of it:

// Euler 1
var _ = require('lodash')
console.log(_.sum(_.filter(_.range(1000),
    (x) => (x % 3 === 0) || (x % 5 === 0))))
console.log("Awesome!")


Basic C++ IOT Weather Station I am making an IOT weather station based on a particle photon that sends data via webhook to one of my other projects. I am very new to c++ and programming in general.

What can I do better with my code? What can be optimised? And what are industry best practices that I can put into place?

Here's my code:

#include <Adafruit_DHT/Adafruit_DHT.h>

#define DHTPIN 2

#define DHTTYPE DHT11

DHT dht(DHTPIN, DHTTYPE);

void setup() {
Serial.begin(9600);
dht.begin();
}

void loop() {

delay(2000);

//Variables
//DHT11 sensor floats
float h = dht.getHumidity();
float t = dht.getTempCelcius();
float f = dht.getTempFarenheit();
float hi = dht.getHeatIndex();
//UV sensor floats
float sensorValue = analogRead(A0);
int UVLevel = 0;

//Error Retry
if (isnan(h) || isnan(t) || isnan(f))
{
Serial.println("Failed to read from DHT sensor!");
return;
}

if(sensorValue <=10)
{
     UVLevel = 0;
}
if(sensorValue <= 46 && sensorValue > 10)
{
     UVLevel = 1;
}
if(sensorValue <= 65 && sensorValue > 46)
{
     UVLevel = 2;
}
if(sensorValue <= 83 && sensorValue > 65)
{
     UVLevel = 3;
}
if(sensorValue <= 103 && sensorValue > 83)
{
     UVLevel = 4;
}
if(sensorValue <= 124 && sensorValue > 103)
{
     UVLevel = 5;
}
if(sensorValue <= 142 && sensorValue > 124)
{
     UVLevel = 6;
}
if(sensorValue <= 162 && sensorValue > 142)
{
     UVLevel = 7;
}
if(sensorValue <= 180 && sensorValue > 162)
{
     UVLevel = 8;
}
if(sensorValue <= 200 && sensorValue > 180)
{
     UVLevel = 9;
}
if(sensorValue <= 221 && sensorValue > 200)
{
     UVLevel = 10;
}
if(sensorValue <= 240 && sensorValue > 221)
{
     UVLevel = 11;
}
if(sensorValue > 240)
{
     UVLevel = 12;
}

//Serial Print DHT
Serial.println();
Serial.println();
Serial.print("Humid: ");
Serial.print(h);
Serial.print("%");
Serial.println();
Serial.print("Temp: ");
Serial.print(t);
Serial.print("C ");
Serial.println();
Serial.print("Apparent Temperature: ");
Serial.print(hi);
Serial.println();
Serial.println();

//Serial Print UV
Serial.print("UV Level =");
Serial.print(UVLevel);
Serial.println();

//Publish Data To Particle Cloud
Particle.publish("Humidity", String(h));
Particle.publish("Temperature", String(t));
Particle.publish("Apparent Temperature", String(hi));
Particle.publish("UV Index",String(UVLevel));
delay(5000);
}

For reference these are the sensors I am using:

DHT 11 Temp/Humidity sensor

UV sensor

I'll assume the indentation is an artifact of pasting the code here.

The big problem here is that long series of if statements.  A simple improvement is to make use of the else clause, along with knowledge of what values you've tested for already, it can be simplified to

if (sensorValue <= 10)
    UVLevel = 0;
else if (sensorValue <= 46)
    UVLevel = 1;
// etc.

But we can do better.  Since the range of values you're checking with is contiguous, and the result (UVLevel) is linear, we can set up an array to hold the data, then just use a loop to find the right place.

static values[] = { 10, 46, 65, /* ... */, 240 };
UVLevel = std::size(values);    // 12
for (int i = 0; i < std::size(values); ++i)
    if (values[i] <= sensorValue) {
        UVLevel = i;
        break;
    }
}

We initially set UVLevel the the maximum value so that if the sensor value is greater than 240 we get the correct result without having to check after the loop.

For a larger range of possible values, the for loop used here can be replaced with a more complicated binary search.


Single consumer and single producer lock-free circular buffer I'm pretty new to multithreading. The code below is a simple implementation of a single consumer/single producer circular buffer that does not use any locking, that I wrote for fun.

The question is, will this kind of container work in actual real world situation or would it crash miserably because of some data race that I have not noticed?

#include <array>
#include <memory>
#include <atomic>
#include <stdexcept>
#include <iostream>
#include <thread>
#include <random>

template<typename T, int S=5>
class CircularBuffer{
public:
  CircularBuffer():start(0),end(0){
  }
  bool AddElement(std::unique_ptr<T> && obj,bool block=true){

    int end_idx=end.load();
    if(block)
      while(end_idx+1==start.load());//Busy wait until there is place
    else if(end_idx+1==start.load())
      return false;

    buffer[end_idx++]=std::move(obj);
    if(end_idx==buffer.size())
      end.store(0);
    else
      end.store(end_idx);
    return true;
  }
  std::unique_ptr<T> GetElement(bool block=true){
      int start_idx=start.load();
      if(block)
        while(end.load()==start_idx);
      else if(end.load()==start_idx)
        return std::unique_ptr<T>();

      auto ret_obj=std::move(buffer[start_idx]);
      start_idx++;
      if(start_idx==buffer.size())
    start.store(0);
      else
    start.store(start_idx);
      return ret_obj;
  }

private:
  std::array<std::unique_ptr<T>,S+1> buffer;
  std::atomic<int> start;
  std::atomic<int> end;

};

std::random_device rd;
std::mt19937 gen(rd());
std::uniform_int_distribution<> dis(0,500);

void Producer(CircularBuffer<int> * buffer){

  int i=0;
  while(i<20){
    std::this_thread::sleep_for(std::chrono::milliseconds(dis(gen)));
    auto val=dis(gen);
    std::cout << "Adding " << val <<std::endl;
    buffer->AddElement(std::unique_ptr<int>(new int(val)));
    i++;
  }
}
void Consumer(CircularBuffer<int> * buffer){

  int i=0;
  while(i<20){
    std::this_thread::sleep_for(std::chrono::milliseconds(dis(gen)));
    std::cout << "Got " << *buffer->GetElement() << std::endl;
    i++;
  }

}

int main(){

  CircularBuffer<int> buffer;

  std::thread t[2];
  t[0]=std::thread(Producer,&buffer);
  t[1]=std::thread(Consumer,&buffer);

  t[0].join();
  t[1].join();
}

A few notes, mostly unrelated to the actual lock-free nature of the code:

Some of your names are a lot less meaningful than I'd like. A prime example is your template parameter S in: template<typename T, int S=5>. This should probably be renamed to something like Size instead. Although not quite as problematic, I'd say your dis and gen have pretty much the same problem as well. (Note that although T as the type parameter is just as short, it's so common I'd accept it, much like I'd accept i as the index for a loop).
Although some people avoid them, I'd at least consider using something like size_t for the types of CircularBuffer::begin and CircularBuffer::end. It appears that neither should ever be negative.
You might want to consider using std::atomic_int (or std::atomic_uint) instead of std::atomic<int>. It's unlikely to make much difference, but does give the library design a little extra chance for optimization (i.e., atomic_int might just be a typedef for atomic<int>, but could be a separate type instead, if the library designer sees an advantage in doing so).
It may just be a side-effect of some strange tab expansion (or something similar) but at least as posted, your indentation is pretty strange in a few places. For one obvious example:

  if(start_idx==buffer.size())
start.store(0);
  else
start.store(start_idx);

I'd have to do some looking to be absolutely certain, but I believe your use of a global for the random number generator causes a data race that leads to undefined behavior. Specifically, a pseudo-random number generator has an internal state that's modified when you obtain a random number from it. Doing that modification simultaneously from two or more threads gives undefined behavior.
This sequence:

  start_idx++;
  if(start_idx==buffer.size())
start.store(0);
  else
start.store(start_idx);

Looks to me like it's subject to race conditions. For example, let's assume start_idx starts out equal to buffer_size-1, then we execute the code above from two threads:

thread A: start_idx++;
thread B: start_idx++;

thread A: if (start_idx == buffer.size())
thread B: if (start_idx == buffer.size())

Both if statements return false--start_idx is now one greater than buffer.size(). We then store start_idx into start.

The next time the function is called, it attempts to use:

auto ret_obj=std::move(buffer[start_idx]);

...and start_idx indexes past the end of buffer, giving undefined behavior.

Limiting to single-producer, single-consumer may be enough to prevent this problem from manifesting, but 1) I find that an essentially useless model, and 2) I'm not at all sure it's sufficient even then.

Although it may sound defeatist, let me give one little bit of advice: when you first do multithreaded programming, you're a lot better off sticking to using locks, mutexes, and so on. Getting just that much right will keep you busy for quite a while.

(Only) once you've gotten to the point that it's easy to handle that, then start to move on to working at lock-free programming (but be aware that it's an area where even the very best, brightest and most experienced still get things wrong quite frequently.


Fetching user details, posts, and comments using promises in ExpressJS I'm creating an API service in NodeJS using Express (version 4) Framework. Created an MVC architecture for the same (no views, only model, and controller as this is an API).

Also, there are routes. When a request is made, the router redirects the request to the controller function and from there functions written in models are called and the response is returned to users in JSON format.

I gotta make multiple asynchronous requests from the controller for some functions. I earlier nested the asynchronous calls (callback hell) before I switched to Promises.

My NodeJS version is 6.12 and it does not support async/await.

I wanna make sure the way I implemented promises is correct.

Here is a sample function from my controller using which I fetch user details.
The user passes a userID and using that I fetch user details. And using details from the 1st async request, I fetch user's posts and after that, I fetch user comments. I wanna make sure my implementation is correct.

Note: I removed error handling from some asynchronous requests to make the code look more understandable.

exports.fetchUserInformation = function (req, res) {
  var response = {};

  if (req.query.userId) {
    var userId = req.query.userId;
    response['userId'] = userId;
    new Promise(function (resolve, reject) {
      var param = {
        userId: userId
      };
      //1 - Call to Model function to fetch user info
      User.getUserDetails(param, function (err, rows) {
        if (err) {
          reject(err);
        } else {
          resolve(rows);
        }
      });
    }).then(function (result1) {
      var result2;
      response['userDetails'] = result1;

      //Pass result from previous promise to fetch next result
      var params1 = {
        userId: userId,
        result: result1
      };
      //2 - Call to Model function to fetch user timeline posts
      User.fetchUserPosts(params1, function (err, rows) {
        result2 = rows;
      });
    }).then(function (result2) {
      response['userPosts'] = result2;

      var params2 = {
        userId: userId,
        userType: 'user'
      };
      User.fetchUserComments(params2, function (err, rows) {

        response['userComments'] = rows;

        //return the response to user
        res.json({
          success: true,
          response: response
        });

      });
    }).catch(function (error) {
      //error logging starts
      var logData = {
        stackTrace: error
      };
      logHlpr.logThisError(logData); //calling another function to log data
      //error logging ends

      res.json({
        success: false
      });
    });
  }
};

I've few doubts:

I'm making 3 async requests to the Model - User.getUserDetails, User.fetchUserPosts, User.fetchUserComments using one promise. Should I implement 3 promises?

I'm pretty new to NodeJS and promises.

If you use 1 promise, you would encounter a long chain of then method and all of async operations would be executed in sequence style which increases response time.

If you create 3 promises, all of that can be performed in parallel. So the answer is yes, you should implement with 3 promises.

By the way, with help of bluebird library, You can write more readable JS code as I refactored show below, (I will explain more):

const bluebird = require('bluebird')
const User = require('./model/User')
bluebird.promisifyAll(User)

function asyncHandler(fn) {
    const promiseFn = bluebird.method(fn)
    return function(req, res) {
        let resultPromise = promiseFn.call(this, req,res)
        resultPromise.then(function(result) {
            res.json({
                success: true,
                response: result,
            })
        }).catch(function(err) {
            logHlpr.logThisError({stackTrace: err});
            res.json({
                success: false,
            })
        })
    }
}

exports.fetchUserInformation = asyncHandler(function(req, res) {
    if (!req.query.userId) {
        throw new Error("UserId is required")
    }
    let userId = req.query.userId
    let userDetails = User.getUserDetailsAsync({userId: userId})
    let userPosts = userDetails.then(result => User.fetchUserPostsAsync({userId: userId, result: result}))
    let userComments = User.fetchUserCommentsAsync({userId: userId, userType: 'user'})
    return bluebird.props({
        userId: userId,
        userDetails: userDetails,
        userPosts: userPosts,
        userComments: userComments,
    })
})

Most of the time, when you write JavaScript in Node.js, you need to work with callbacks, so convert all callback-style to promise-style by using bluebird.promisifyAll() or bluebird.promisify() will allow you to work with code easier, please see bluebird document.

After that most of handler function you write would deal with promise, so you can create higher-order function, like asyncHandler, which send success response in case that promise is fulfilled. You can also move asyncHandler function to shared-file and require() it later to use at multiple places.

For handler function, fetchUserInformation from your code sample, you want to return result as an object. This object contains result from many promises. You can use bluebird.props() which create a wrapper promise that wait all object's values to be fulfilled.

The last things, I would suggest you find a way to reimplement User.fetchUserPosts() that it wouldn't require userDetails as part of the parameter, so this method can be performed without waiting for User.getUserDetails() result and can make the code more elegant.


Python code for checking request headers I am trying to validate a HTTP REST API request in my python code. It checks for some request headers (X-Foo and X-Bar) and two keys in the json body/payload (someKey and someNestedKey). I have written this code with if/else conditions as well as a simple try-catch.

headers = event["headers"]
if "X-Foo" not in headers or "X-Bar" not in headers:
    return {
        'statusCode': 401,
        'body': '{"message":"Some error message"}'
    }

# Do something with request headers here
payload = json.loads(event["body"])

if "someKey" not in payload:
    some_flag = False
else:
    if "someNestedKey" not in payload["someKey"]:
        some_flag = False
    else:
        some_flag = True

if not some_flag:
    return {
        'statusCode': 400,
        'body': '{"message":"Some other error message"}'
    }

# if we get this far, then its ok
foo_bar = payload["someKey"]["someNestedKey"]

vs

try:
    headers = event["headers"]
    # Do something with request headers here
    payload = json.loads(event["body"])
    foo_bar = payload["someKey"]["someNestedKey"]
except KeyError:
    return {
            'statusCode': 401,
            'body': '{"message":"Some error message"}'
        }

I'd like a review on all aspects of this code.

I am not sure this question can still be salvaged as some relevant details are missing.

One opening remark though: HTTP header fields are case-insensitive, as stipulated by several RFCs, including RFC 7230 (See 3.2.  Header Fields).

  Each header field consists of a case-insensitive field name followed
  by a colon (":"), optional leading whitespace, the field value, and
  optional trailing whitespace.

If you are using the requests module to collect your headers with key values you would have an object of type requests.structures.CaseInsensitiveDict, and you don't have to take care of this aspect. Therefore, "X-Foo" in req.headers and "x-foo" in req.headers will both match the same.

But it is good to keep this in mind as some clients may take some liberty and decide on a different casing like all-lowercase - after all they are still RFC-compliant, and so should you.

This aspect is also discussed in the doc for the requests module: Response Headers

So you are checking the existence of two headers but aren't you interested in their values ? What is the point of using those two headers ? Is the code slightly incomplete or did I miss something ?

Exception handling: your whole procedure should be covered by a comprehensive exception handling routine.
For specific tasks like decoding the JSON payload, watch out for JSONDecodeError exception. It will be triggered if an invalid JSON string is sent to your script (could be an attacker). See below.

An unhandled exception could in certain circumstances defeat or bypass the various checks you make. Bottom line: identify the sensitive parts of your code (in particular those processing external input), always handle exceptions, never ignore them.

The code can be simplified, for example:

if "someKey" not in payload:
    some_flag = False
else:
    if "someNestedKey" not in payload["someKey"]:
        some_flag = False
    else:
        some_flag = True

if not some_flag:
    return {
        'statusCode': 400,
        'body': '{"message":"Some other error message"}'
    }

If the goal is to check for the existence of a key in a JSON string and extract the corresponding value you could wrap the functionality in small functions to declutter the code.

In the example below I am using a simplistic JSON string like {"AuthToken": "abcdef", "UserID": 1234}. Since you've mentioned nested keys you'll have to adapt the code a little bit but you get the idea.

def parse_json(json_string):
    try:
        # return dict - maybe (see later)
        return json.loads(json_string)
    except json.JSONDecodeError:
        # stop immediately or alternatively return an empty dict
        print('JSONDecodeError triggered, do something')
        #logger.error('JSONDecodeError triggered', exc_info=True)
        #sys.exit(1)

def get_value_from_key(payload, key_to_check):
    if key_to_check in payload.keys():
        # key found, get the matching value
        return payload[key_to_check]
    else:
        return None

Testing the functions:

json_string = """{"AuthToken": "abcdef", "UserID": 1234}"""
payload = parse_json(json_string)

invalid_json_string = """{"AuthToken": "UserID": 1234}"""
# this will trigger an handled exception, and you should not proceed any further
invalid_payload = parse_json(invalid_json_string)

key_to_check = 'AuthToken'
# value will be None if the key was not found, then you could raise your 401 error or whatever
print(get_value_from_key(payload, key_to_check))

If you don't want to choke on the exception but proceed as casually as possible you could return an empty dict but at least you've handled the invalid input.

It seems to me that the way you are handling KeyError in your code will therefore not suffice to handle all the possible bad situations. The best is of course to test your script in adverse conditions, feed it garbage and see how it reacts.

NB: unlike the headers collection routine get_value_from_key is case-sensitive. So if I want to retain the ability of doing case-insensitive searching on the keys I would probably do this, reusing structures already available to us:

from requests.structures import CaseInsensitiveDict

if key_to_check in CaseInsensitiveDict(payload):
    ...

If your code is already using requests you are not adding extra dependencies

But wait. Actually, json.loads will not always return a dict, than depends.

  Deserialize s (a str, bytes or bytearray instance containing a JSON
  document) to a Python object using this conversion
  table.

Source: json — JSON encoder and decoder

So I would advise you to check that the return value is actually of a suitable type like dict or list. It's always important to read the doc so that there is  no ambiguity as to the data type returned by a given function, because there may be several possibilities.

The proposed code is for demonstration purposes but not comprehensive, hopefully it can help you avoid a few pitfalls.


File manipulation with Haskell I am trying to pick up Haskell!

But I feel like my code is a bit painful to write and read.

Do you have any pointers on how to improve its readability / style?

main = do
    contents <- fmap tail . fmap lines . readFile $ "testInput"
    let input = map pair_to_tuple .(map (map read)) . map words $ contents :: [(Int,Int)]
    let result = map (uncurry waffles) $ input
    let decorated_result = ["Case #" ++ show i ++ ": " ++ show s | (i,s) <- zip [1..length result] result]
    writeFile "output.txt" $ unlines $ decorated_result

waffles row col = (row - 1)*(col - 1)

pair_to_tuple [a,b] = (a,b)

This script reads a file whose first line is a title, then every line is a couple of numbers. Then it drops the first line, and processes every pair of numbers using the function waffles, then it writes it back to an output file.

contents <- fmap tail . fmap lines . readFile $ "testInput"

can be written as

contents <- (tail . lines) <$> readFile "testInput"

Here we take advantage of the law fmap f . fmap g == fmap (f . g). <$> is an infix version of fmap. $ is extraneous here.

let input = map pair_to_tuple .(map (map read)) . map words $ contents :: [(Int,Int)]

can be written as

let input = map (pair_to_tuple . (map read) . words) contents :: [(Int,Int)]

For lists map = fmap, so map f . map g == map (f . g) also holds.

In

let result = map (uncurry waffles) $ input

the $ sign is also extraneous.

let decorated_result = ["Case #" ++ show i ++ ": " ++ show s | (i,s) <- zip [1..length result] result]

You can exploit Haskell's lazy evaluation and create infinite list to zip with:

let decorated_result = ["Case #" ++ show i ++ ": " ++ show s | (i,s) <- zip [1..] result]

Otherwise I'd say the code is fine.