Canceling out unnecessary punctuation I have 25 branches but I'm only allowed to have 12 branches maximum. The objective of this task was to cancel out unnecessary punctuation, find the common elements that exist in the sentence and dictionary and then to translate the result into English into the other language or the other language to English, depending on what we are given. How do I lessen the number of branches of code I have?

sentence = "?hello boolean bring, !mango! and, country ban,ana wish yum  apple!"
dictionary = "a aa apple banana pear strawberry mango country wish boolean bring"
punctuation = "?!,"
language = "English"

"""answer = buwulean bringah mango countryeeh wishey apple"""

"""The above lines aren't in my final code, this is just for information
about what I'm trying to solve for."""

def translate(sentence, dictionary, punctuation, language):
    """R"""

    sentence = ' '.join([x.strip(punctuation) for x in sentence.split()])
    """Removing unnecessary punctuation"""

    dictionary = dictionary.split()
    sentence = sentence.split()
    result = []

    for element in sentence:
        if element in dictionary:
            result.append(element)

    result2 = []

    for word in result:
        if language == "English":
            if word[-1] == "y":
                word = "{}eeh".format(word)
                result2.append(word)
            elif word[-3:] == "ing":
                word = "{}ah".format(word)
                result2.append(word)
            elif word[-2:] == "sh":
                word = "{}ey".format(word)
                result2.append(word)
            elif "oo" in word:
                word = word.replace("oo", "uwu")
                result2.append(word)
            elif word == "is":
                word = "oy"
                result2.append(word)
            elif word == "are":
                word = "argh"
                result2.append(word)
            elif word == "he" or word == "she" or word == "it":
                word = "hana"
                result2.append(word)
            elif word == "his" or word == "her":
                word = "hami"
                result2.append(word)
            elif word == "ham" or word == "sausage" or word == "bacon":
                word = "{}!".format(word)
                result2.append(word)
        else:
            if word[-3:] == "eeh":
                word = word[:-3]
                result2.append(word)
            elif word[-2:] == "ah":
                word = word[:-2]
                result2.append(word)
            elif word[-2] == "ey":
                word = word[:-2]
                result2.append(word)
            elif "uwu" in word:
                word = word.replace("uwu", "oo")
                result2.append(word)
            elif word == "oy":
                word = "is"
                result2.append(word)
            elif word == "argh":
                word = "are"
                result2.append(word)
            elif word == "hana":
                word = "he"
                result2.append(word)
            elif word == "hami":
                word = "his"
                result2.append(word)
            elif word == "ham!":
                word = "ham"
                result2.append(word)
            elif word == "sausage!":
                word = "sausage"
                result2.append(word)
            elif word == "bacon!":
                word = "bacon"
                result2.append(word)
            else:
                result2.append(word)
    return " ".join(result2)

translated = translate(sentence, dictionary, punctuation, language)
print(translated)

"""The last two lines are also not in my final code"""

Focusing on the specific part of the code that handles translations for English, here's a way of doing it with a dictionary and a function table:

english_swaps = {
    # Dictionary of 1:1 word replacements.
    "is": "oy",
    "are": "argh",
    "he": "hana",
    "she": "hana",
    "it": "hana",
    "his": "hami",
    "her": "hami",
    "ham": "ham!",
    "sausage": "sausage!",
    "bacon": "bacon!",
}

english_rules = [
    # List of arbitrary functions to transform a word.
    lambda w: f"{w}eeh" if w[-1] == "y" else None,
    lambda w: f"{w}ah" if w[-3:] == "ing" else None,
    lambda w: f"{w}ey" if w[-2:] == "sh" else None,
    lambda w: w.replace("oo", "uwu") if "oo" in w else None,
    lambda w: english_swaps.get(w, None),
]

def translate(word: str) -> str:
    if language == "English":
        for rule in english_rules:
            # Attempt to apply each rule in turn, using the first one to match.
            translated = rule(word)
            if translated is not None:
                return translated
        return word
    else:
        # ...

print(" ".join(
    translate(w) for w in "he is eating sausage and loving every bit of it".split()
))
# prints: hana oy eatingah sausage! and lovingah everyeeh bit of hana

You could apply this same pattern to have a dictionary and function table for each language, and put those into another dictionary that maps each language to a function table, so that ultimately you could do something like:

for rule in rules[language]:
   translated = rule(word)
   ... etc ...


Insert function for a trie in Ocaml I don't know how could I write smarter/more elegant code for the add function , it just seems so meaty and hard to read.
This trie only stores words and marks their end with a bool.

type trie = Node of bool * (char * trie) list
let empty = Node (false, [])

let rec add w tr = match w, tr with
    | [], Node (_, lvl_l) -> Node (true, lvl_l)
    | wh :: wt, Node (b, lvl_l) ->
        try let ins_point = (List.assoc wh lvl_l) in
        Node(b, (wh, add wt ins_point)::(List.remove_assoc wh lvl_l))
        with Not_found -> Node (b, (wh, add wt empty)::lvl_l)

let explode word =
       let rec explode' i acc =
           if i < 0 then acc else explode' (i-1) (word.[i] :: acc)
            in explode' (String.length word - 1) []

Output Example :

# let x = add (explode "horror") empty;;
val x : trie =
  Node (false,
   [('h',
     Node (false,
      [('o',
        Node (false,
         [('r',
           Node (false,
            [('r',
              Node (false, [('o', Node (false, [('r', Node (true, []))]))]))]))]))]))])
# let x = add (explode "horrific") x;;
val x : trie =
  Node (false,
   [('h',
     Node (false,
      [('o',
        Node (false,
         [('r',
           Node (false,
            [('r',
              Node (false,
               [('i',
                 Node (false,
                  [('f',
                    Node (false,
                     [('i', Node (false, [('c', Node (true, []))]))]))]));
                ('o', Node (false, [('r', Node (true, []))]))]))]))]))]))])

Sometimes just adding whitespace helps:

let rec add w tr = match w, tr with
    | [], Node (_, lvl_l) ->
        Node (true, lvl_l)
    | wh :: wt, Node (b, lvl_l) ->
        try
            let ins_point = List.assoc wh lvl_l in
            Node (b, (wh, add wt ins_point)::(List.remove_assoc wh lvl_l))
        with Not_found ->
            Node (b, (wh, add wt empty)::lvl_l)

The match expression was upgraded in recent years to match on exceptions. So you can also do this:

let rec add w tr = match w, tr with
    | [], Node (_, lvl_l) ->
        Node (true, lvl_l)
    | wh :: wt, Node (b, lvl_l) ->
        begin match List.assoc wh lvl_l with
            | ins_point ->
                Node (b, (wh, add wt ins_point)::(List.remove_assoc wh lvl_l))
            | exception Not_found ->
                Node (b, (wh, add wt empty)::lvl_l)
        end

(I had to wrap the match in a begin/end block to avoid confusing which match the patterns are applied.)

Since the return value has a common form, you could use match to just compute the second part of the pair:

let rec add w tr = match w, tr with
    | [], Node (_, lvl_l) ->
        Node (true, lvl_l)
    | wh :: wt, Node (b, lvl_l) ->
        Node (b, match List.assoc wh lvl_l with
                 | ins_point ->
                     (wh, add wt ins_point) :: (List.remove_assoc wh lvl_l)
                 | exception Not_found ->
                     (wh, add wt empty) :: lvl_l)

It's subjective whether these are any better than your example. I do think, however, that visually showing the structure helps me understand the code better. For instance, my last version shows Node being produced in both match cases. You could elevate the constructor "one more level up" and have the nested match expressions compute the pair to apply to the constructor. This wasn't immediately obvious from your example.


Classes for Questions and Answers I made a simple Question "system":

Answer

Stores an answer (string) and a boolean value to check if the answer is correct or not.

public class Answer {

    private String answer;
    private boolean isCorrect;

    public Answer(String answer, boolean isCorrect) {
        this.answer = answer;
        this.isCorrect = isCorrect;
    }

    public String getAnswer() {
        return answer;
    }

    public boolean isCorrect() {
        return isCorrect;
    }

}

Question

I don't know if I should use "Answer[] answers" or "Answers... answers" in the Question constructor.

public class Question {

    private String question;
    private Answer[] answers;

    public Question(String question, Answer... answers) {
        this.question = question;
        this.answers = answers;
    }

    public String getQuestion() {
        return question;
    }

    public Answer[] getAnswers() {
        return answers;
    }

    public boolean isCorrect(String answer) {
        for(Answer tempAnswer : answers)
            if(tempAnswer.getAnswer().equals(answer) && tempAnswer.isCorrect())
                return true;

        return false;
    }

    public boolean isCorrectIgnoreCase(String answer) {
        for(Answer tempAnswer : answers)
            if(tempAnswer.getAnswer().equalsIgnoreCase(answer) && tempAnswer.isCorrect())
                return true;

        return false;
    }
}

SAMPLE

private static Scanner scanner = new Scanner(System.in);

    public static void main(String[] args) {
        Question questionA = new Question("Do chickens fly?",
                                          new Answer("Yes", true),
                                          new Answer("No", false));
        System.out.println(questionA.getQuestion());

        while(true) {
            if(questionA.isCorrectIgnoreCase(scanner.nextLine()))
                break;
            else
                System.out.println("No, that is incorrect. Please try again.");
        }

        System.out.println("Correct.");
    }

About your primary question: I'd definitely go with Answer... whenever I have the possibility to do so. As you can pass on an array to a varargs parameter, but not varargs to an array parameter, varargs is the better option IMO.

Other comments

Naming

private boolean isCorrect;

It is convention to not prefix boolean fields with is. The name of the method should be is + variable name with upper case starting letter, so:

public boolean isCorrect() {
    return correct;
}

final keyword

It is good practice to mark all possible fields with final, this applies in your case to:

private final String answer;
private final boolean correct; // note the name change

and in the Question class:

private final String question;
private final Answer[] answers;

Mutability

This code might seem harmless to you:

public Question(String question, Answer... answers) {
    this.question = question;
    this.answers = answers;
}

public Answer[] getAnswers() {
    return answers;
}

But the fact is, this is slightly dangerous. Although your Answer class is immutable, this is not. Imagine the following code:

Question questionA = new Question("Do chickens fly?",
     new Answer("Yes", true),
     new Answer("No", false));
questionA.getAnswers()[0] = null;
questionA.getAnswers()[1] = new Answer("Always right", true);

Whoops, now we've cleared one answer and changed the other!

Solution: Make array copies using Arrays.copyOf:

public Question(String question, Answer... answers) {
    this.question = question;
    this.answers = Arrays.copyOf(answers, answers.length);
}

public Answer[] getAnswers() {
    return Arrays.copyOf(answers, answers.length);
}

Possible extension: Make it a score

Your classes supports multiple answers to be correct, that is nicely done. But what if one answer is still "better" than the other? My suggestion is that you use a int or a float for score instead of a boolean correct.

Java 8

If you have Java 8 available, your isCorrect(String answer) method can be improved:

public boolean isCorrect(String answer) {
    return Arrays.stream(answers).anyMatch(a -> a.getAnswer().equals(answer));
}

The same can of course be applied to your isCorrectIgnoreCase method:

public boolean isCorrect(String answer) {
    return Arrays.stream(answers).anyMatch(a -> a.getAnswer().equalsIgnoreCase(answer));
}


REST-ish API Account Controller I still have to implement API keys for client auth, but so far this is what I have for users.  This was built using WebAPI and SimpleMembership with Forms Auth:

Is Authenticated:

public class AccountController : ApiController
{
public static DtoService _service = new DtoService();

// GET/api/isAuthenticated
[System.Web.Http.HttpGet]
public HttpResponseMessage IsAuthenticated()
{
    try
    {
        if (User.Identity.IsAuthenticated)
            return Request.CreateResponse(HttpStatusCode.OK, WebSecurity.GetUserId(User.Identity.Name));
        else
            return Request.CreateResponse(HttpStatusCode.OK, false);
    }
    catch (Exception e)
    {
        return Request.CreateResponse(HttpStatusCode.InternalServerError, e);
    }

}

Login:

// POST /api/login
// [System.Web.Http.AllowAnonymous]
[System.Web.Http.HttpPost]
public HttpResponseMessage LogIn(LoginModel model)
{
    if (!ModelState.IsValid)
        return Request.CreateErrorResponse(HttpStatusCode.BadRequest, ModelState);
    try
    {
        if (User.Identity.IsAuthenticated)
            return Request.CreateResponse(HttpStatusCode.Conflict, "already logged in.");
        if (!WebSecurity.UserExists(model.UserName))
            return Request.CreateResponse(HttpStatusCode.BadRequest, "User does not exist.");
        if (WebSecurity.Login(model.UserName, model.Password, persistCookie: model.RememberMe))
        {
            FormsAuthentication.SetAuthCookie(model.UserName, model.RememberMe);
            return Request.CreateResponse(HttpStatusCode.OK, "logged in successfully");
        }
        return Request.CreateResponse(HttpStatusCode.BadRequest, "Login Failed.");
    }
    catch (Exception e)
    {
        return Request.CreateResponse(HttpStatusCode.InternalServerError, e);
    }
}

Log Out:

// POST /api/logout
[System.Web.Http.HttpPost]
////[ValidateAntiForgeryToken]
// [System.Web.Http.AllowAnonymous]
[Authorize]
public HttpResponseMessage LogOut()
{
    try
    {
        if (User.Identity.IsAuthenticated)
        {
            WebSecurity.Logout();
            return Request.CreateResponse(HttpStatusCode.OK, "logged out successfully.");
        }
        return Request.CreateResponse(HttpStatusCode.Conflict, "already done.");
    }
    catch (Exception e)
    {
        return Request.CreateResponse(HttpStatusCode.InternalServerError, e);
    }
}

Register:

// POST: /api/register
[System.Web.Http.HttpPost]
//[ValidateAntiForgeryToken]
public HttpResponseMessage Register(RegisterModel model)
{
    if (!ModelState.IsValid)
    {
        return Request.CreateErrorResponse(HttpStatusCode.BadRequest, ModelState);
    }
    try
    {
        if (User.Identity.IsAuthenticated)
            return Request.CreateResponse(HttpStatusCode.Conflict, "User Already Registered and Logged In");
        if (WebSecurity.UserExists(model.UserName))
            return Request.CreateResponse(HttpStatusCode.Conflict, "User Already Registered");
        else
        {
            // Attempt to register the user
            WebSecurity.CreateUserAndAccount(model.UserName, model.Password);
            WebSecurity.Login(model.UserName, model.Password);
            InitiateDatabaseForNewUser(WebSecurity.GetUserId(model.UserName));

            FormsAuthentication.SetAuthCookie(model.UserName, createPersistentCookie: false);
            return Request.CreateResponse(HttpStatusCode.Created, WebSecurity.GetUserId(model.UserName));
        }

    }
    catch (Exception e)
    {
        return Request.CreateResponse(HttpStatusCode.InternalServerError, e);
    }
}

Lets start with your URI's.

GET /api/isAuthenticated - This is not restful I guess. isAuthenticated doesn't sound like it is a resource, instead it sounds like it is a method returning true or false. May be it would help if you read more on Restful architecture.

POST /api/login - Even this sounds like a process but not resource

Instead I would go with /api/authentication URI

On GET it returns authenticated user information
on POST you can send credentials to create a new authentication
on DELETE you delete the authentication resource which means logout.

Above is just for example about how to use Restful URI's. To be blunt, login process should not be done through RESTful URI's as it introduces stateful system where as RESTful API should be stateless. Instead you should use authentication headers of HTTP for authenticating and authorizing a user.

I hope this helps you in understanding RESTful API.


Simple telephone address book I am new to Java programming, and to help me learn, I created a simple telephone address book.  The code runs as expected, but I would be interested to here from more advanced programmers if they think I should do it differently.

import com.mongodb.MongoClient;
import com.mongodb.MongoException;
import com.mongodb.DB;
import com.mongodb.DBCollection;
import com.mongodb.BasicDBObject;
import com.mongodb.DBCursor;

import java.net.UnknownHostException;
import java.util.Scanner;

import org.bson.types.ObjectId;

public class MongodbPhonebook {
    public static void main(String[] args) {
        boolean boolContinue = true;
        String stringCommand;
        Scanner input = new Scanner(System.in);

        try {
            MongoClient mongoClient = new MongoClient();
            DB db = mongoClient.getDB( "myphonelist" );
            DBCollection coll = db.getCollection("myphonelist");

            do {
                System.out.println("Enter command (add, find, show, quit): ");
                stringCommand = input.nextLine();
                if(stringCommand.equals("quit")){
                    boolContinue = false;
                }
                else if(stringCommand.equals("show")){
                    displayAll(coll);
                }
                else if(stringCommand.equals("add")){
                    insertNumber(coll);
                }
                else if(stringCommand.equals("find")){
                    findNumber(coll);
                }
                else {
                    System.out.println("Unknown command!");
                }
            } while(boolContinue == true);
        }
        catch (UnknownHostException e) {
            e.printStackTrace();
        }
        catch (MongoException e) {
            e.printStackTrace();
        }
    }

    public static void displayAll(DBCollection collDisplay) {
        DBCursor cursor = collDisplay.find();
        try {
            while(cursor.hasNext()) {
                System.out.println(cursor.next());
            }
        } finally {
            cursor.close();
        }
        return;
    }

    public static void insertNumber (DBCollection collInsert) {
        Scanner input = new Scanner(System.in);
        ObjectId stringID;
        String stringName = "";
        String stringNumber = "";
        System.out.println("Enter name: ");
        stringName = input.nextLine();
        System.out.println("Enter number: ");
        stringNumber = input.nextLine();
        BasicDBObject doc = new BasicDBObject("name", stringName).
                append("number", stringNumber);
        collInsert.insert(doc);
        stringID = (ObjectId)doc.get( "_id" );
        System.out.println("Result: " + stringID.toString());
        return;
    }

    public static void findNumber (DBCollection collFind) {
        Scanner input = new Scanner(System.in);
        String stringName = "";
        System.out.println("Enter name: ");
        stringName = input.nextLine();
        BasicDBObject query = new BasicDBObject("name", stringName);

        DBCursor cursor = collFind.find();
        cursor = collFind.find(query);

        try {
            while(cursor.hasNext()) {
                System.out.println("found " + cursor.next());
            }
        } finally {
            cursor.close();
        }
    }
}

Overall, it looks pretty good. Here are some points to consider:

There is no need to explicitly return at the end of void methods, for example, at the end of displayAll().

Keep your variable declarations close to where they are used. For example, in insertNumber(), I would change this...

String stringName = "";
String stringNumber = "";
System.out.println("Enter name: ");
stringName = input.nextLine();
System.out.println("Enter number: ");
stringNumber = input.nextLine();

...to this...

System.out.println("Enter name: ");
String stringName = input.nextLine();
System.out.println("Enter number: ");
String stringNumber = input.nextLine();

Also, limit the scope of variables to the block where they are used. This follows from the point above. For example, in main(), stringCommand is declared near the top, but is only used inside the do/while block. I would remove the declaration above and change this line...

stringCommand = input.nextLine();

...to this...

String stringCommand = input.nextLine();

These will reduce the number of lines slightly, but will also make reading and debugging the code easier.

As a slighter bigger project, you could change the methods to be non-static, then create an instance of MongodbPhonebook in main() to interact with instead. It may also make sense to move some of the code from main() somewhere else. For example, you could make the mongo variables private members of MongodbPhonebook and initialize them in the constructor.

public class MongodbPhonebook {

    private MongoClient mongoClient;
    private DB db;
    private DBCollection coll;

    public MongodbPhonebook() {
        this.mongoClient = new MongoClient();
        this.db = mongoClient.getDB( "myphonelist" );
        this.coll = db.getCollection("myphonelist");
    }

    ...

}

If you did this, you would no longer need to pass the DBCollection around to the other functions.

Beyond that, consider separating the user interaction and database logic into separate functions. For example, split findNumber() into two functions: one to ask the user for input and display the results, the other to perform the search based on the supplied input. This would allow the database logic functions to be reused in different circumstances, for example, a web application.


Convert a custom object to a query string I wrote an extension method that takes a custom object and converts its properties to a query string for use in a URL. You can specify what properties you want included by using a [QueryString] attribute. For example:

class Thing
{
    [QueryString]
    public int Id {get; set;}
    [QueryString]
    public string Name {get; set;}
    public object MetaData {get; set;}
}

var thing = new Thing { Id = 1, Name = "Test", MetaData = someObj };
thing.ToQueryString(); // outputs `?Id=1&Name=Test

Here's the code for the extension method:

public static string ToQueryString(this object obj)
{

    StringBuilder queryStringBuilder = new StringBuilder("?");

    Type objType = obj.GetType();
    // Get properties marked with `[QueryString]`
    List<PropertyInfo> props = objType.GetProperties()
        .Where(p => Attribute.IsDefined(p, typeof(QueryStringAttribute)))
        .ToList();

    foreach (var prop in props)
    {
        string name = prop.Name;
        var value = prop.GetValue(obj, null);

        if (value != null)
        {
            // Check's if this is the last property, if so, don't add an '&'
            if (props.IndexOf(prop) != (props.Count - 1))
            {
                queryStringBuilder.Append($"{Uri.EscapeDataString(name)}={Uri.EscapeDataString(value.ToString())}&");
            }
            else
            {
                queryStringBuilder.Append($"{Uri.EscapeDataString(name)}={Uri.EscapeDataString(value.ToString())}");
            }
        }
    }

    return queryStringBuilder.ToString();
}

The code works just fine, but it seems a bit messy to me. Specifically, checking if we are on the last property seems a bit messy, because we end up doing the same thing, just nixing one character. Is there a way to make that a little cleaner? Are there any other way's this method could be simplified?

I wouldn't go the StringBuilder route, simply because you need to handle the "what if I'm at the start/end" problem. Instead, simply collect the "URL_translated" key-value pairs into an IEnumerable<string> and then apply string.join().

If you're working in .NET Core, Microsoft does all the lifting for you. Here's how they do it.


Testing an action returns a View with a ViewModel using xUnit and Moq This is my first test, I think it's testing what I need it to test but wanted to get some feedback.

I wanted to test to make sure the controller action returns a view with a certain ViewModel.

The code that it will be testing:

Controller:

public class UserController : Controller
{
    private readonly ILogger<UserController> _logger;
    private readonly IViewModelService _vmService;

    public UserController(ILogger<UserController> logger, IViewModelService vmService)
    {
        _logger = logger;
        _vmService = vmService;
    }

    public async Task<IActionResult> Index()
    {
        return View(await _vmService.GetIndexVM());
    }
}

ViewModel

public class UserListVM
{
    public IQueryable<DimUser> UserList { get; set; }
}

ViewModelService

public async Task<UserListVM> GetIndexVM()
{
    return new UserListVM()
    {
        UserList = await _userRepo.GetUserList()
    };
}

The Test:

public class UserControllerTests
{
    [Fact]
    public async Task Index_ReturnsAViewResult_WithUserListVM()
    {
        // Arrange
        var logger = new Mock<ILogger<UserController>>();
        var vmService = new Mock<IViewModelService>();
        var myList = new List<DimUser>
        {
            new DimUser()
            {
                UserId = 1,
                FirstName = "Test",
                Surname = "TestSur",
                RefNumber = "ABC1111111",
                DateOfBirth = DateTime.Now
            }
        };

        vmService.Setup(x => x.GetIndexVM()).ReturnsAsync(new UserListVM()
        {
            UserList = myList.AsQueryable()
        });

        var userController = new UserController(logger.Object, vmService.Object);

        // Act
        var result = await userController.Index();
        var viewResult = Assert.IsType<ViewResult>(result);
        var model = Assert.IsType<UserListVM>(viewResult.ViewData.Model);

        // Assert
        Assert.IsType<UserListVM>(model);
    }
}

Let's review each code segment one by one:

public class UserController : Controller
{
    private readonly ILogger<UserController> _logger;
    private readonly IViewModelService _vmService;

    public UserController(ILogger<UserController> logger, IViewModelService vmService)
    {
        _logger = logger;
        _vmService = vmService;
    }

    public async Task<IActionResult> Index()
    {
        return View(await _vmService.GetIndexVM());
    }
}

I would generally recommend to avoid code like this:

return View(await _vmService.GetIndexVM());

It is hard to add proper error handling, add transformation logic, add conditional branching, etc. A better approach would be to separate these two operations:

var indexViewModel = await _vmService.GetIndexVM();
return View(indexViewModel);

public async Task<UserListVM> GetIndexVM()
{
    return new UserListVM()
    {
        UserList = await _userRepo.GetUserList()
    };
}

First of all the same applies here as above, do not mix object creation logic and async calls.

Secondly, in this simple code you have repeated four times the underlying collection type, which is an implementation detail. If you need to change that implementation detail in the lower layer that would propagate through several layers. Remember that hiding implementation details will help you minimize the scope of a change. A better approach would be:

public async Task<UsersVM> GetIndexVM()
{
    return new UsersVM()
    {
        User = await _userRepo.GetUsers()
    };
}

There is another thing, this service now has two responsibilities:
1) Retrieve data via a lower layer
2) Transform data to the presentation layer

In other words, this layer is an adapter between your presentation layer and repository layer. Generally speaking the service layer is the place where your business logic should reside. Because there is no business logic here, that's why it acts as an adapter.

I have seen the following two approaches regarding object mapping:

Each layer transforms its objects to the lower layer's object
model
Each layer accepts upper layer's object model and it
transforms it into its own model

The first one fits nicely into the n-layer architecture model where each layer only knows about that layer, which is directly beneath it. So, presentation layer knows about service layer. Service layer knows about repository layer.

The second approach violates this rule. Service layer knows about repository layer and knows about presentation layer's domain model. It is not bad, but the first approach (in my opinion) separates the concerns better.

public class UserListVM
{
    public IQueryable<DimUser> UserList { get; set; }
}

Here your naming and data type is not matching. With this name you are stating that it should contain a List, which implies that you could use such operators like Add, Remove, etc. IQueryable does not provide such API.

IQueryable is a type, which is used for deferred execution. In other words it indicates that this is just a query not the materialised form of the query. The problem with this is that it will execute the query when you somehow iterate through it (via foreach or calling .Count, etc.) If you do this in your view then your repository's datacontext might already be disposed.

A better approach would be to expose it like this:

public class UsersVM
{
    public IList<DimUser> Users{ get; set; }
}

Your test's Arrange part looks good, so I will spend some thought on the rest:

// Act
var result = await userController.Index();
var viewResult = Assert.IsType<ViewResult>(result);
var model = Assert.IsType<UserListVM>(viewResult.ViewData.Model);

// Assert
Assert.IsType<UserListVM>(model);

Your Act section should consist only of the call of the Index function of the userController. The assertions should go the under the Assert section.

I would also consider to use IsAssignableForm<T> instead of IsType<T>, because the former supports inheritance as well.


Digital root computation with benchmarking I've read this question about computing the digital root of some integer, and wondered how it would look like in Python.

As a reminder,

  "If you take the digits of any number and add them together, and then add the digits of the resulting number together, and continue doing that until you get a single digit, that single digit is the digital root of the original number."

This is not much about the algorithm performance itself, since it was shown in this answer that instead of summing the digits, you could do a simple division by 9 to get the same result. The question is more about the readability, elegance, and pythonicity (if this word exists) of my code. I checked its correctness with a tool from Thonky.

More, I'd be interested in knowing your opinion on the way I deal with sys.argv, the user interaction, and the benchmarking.

Finally, is it OK to use the int type for such long numbers? It seems to work fine even with 1000-digit numbers, but intuitively it shouldn't...

Any suggestion would be highly appreciated. Thanks.

digitalroot.py

# -*- encoding: utf-8 -*-

import time
import random
import sys

def request_user_number():
    while True:
        try:
            nb = int(raw_input('Enter a number\n'))
            return nb
        except (ValueError, NameError):
            print 'This is not a number. Try again.'

def request_user_again():
    while True:
        try:
            ans = str(raw_input('New computation? (y/n)\n')).lower()
            if ans[0] == 'n':
                again = False
                print 'Bye !'
                break
            elif ans[0] == 'y':
                again = True
                break
            else:
                print 'Did not understand. Try again.'
        except IndexError:
            print 'Say something.'
    return again

def digits_generator(nb):
    tmp = nb
    while tmp != 0:
        yield tmp % 10
        tmp /= 10

def digital_root(nb):
    if nb < 10:
        return nb
    else:
        s = 0
        for d in digits_generator(nb):
            s += d
        return digital_root(s)

def usage():
    print 'Wrong usage'
    print '\'python digitalroot.py ui\' for a user chosen number'
    print '\'python digitalroot.py benchmark nb_size tries\' for benchmarking'
    exit(1)

if __name__ == '__main__':
    if len(sys.argv) < 2 or sys.argv[1] not in ['ui', 'benchmark']:
        usage()
    if sys.argv[1] == 'ui':
        again = True
        while again:
            nb = request_user_number()
            print 'Computing digital root of', nb
            start_time = time.clock()
            dr = digital_root(nb)
            end_time = time.clock()
            print nb, '-->', dr, '(time :', end_time-start_time, 's)'
            again = request_user_again()
    elif sys.argv[1] == 'benchmark':
        try:
            nb_size = int(sys.argv[2])
            tries = int(sys.argv[3])
        except (ValueError, NameError, IndexError):
            usage()
        global_start = time.clock()
        s = 0
        for i in xrange(tries):
            nb = random.randint(10**(nb_size-1), 10**nb_size-1)
            start_time = time.clock()
            d = digital_root(nb)
            end_time = time.clock()
            s += end_time-start_time
            if random.randint(1, tries/5) == 1:
                print 'Random control : i =', i, ',', nb, '-->', d
        print nb_size, 'digits,', tries, 'tries'
        print s, 'seconds (average =', float(s)/tries, 's/try)'
        print 'Total benchmark time :', time.clock()-global_start

I think your algorithmic part is exceedingly verbose.  One of the nicenesses of Python is how it can lead to code that reads almost like English.  And for this particular problem, even leaving performance considerations aside, I think iteration is clearer than recursion.  So I would rewrite things as:

def digits_of(number):
    """Yields the digits of an integer."""
    while number > 0:
        yield number % 10
        number //= 10

def digital_root(number):
    """Computes the digital root of an integer."""
    while number > 9:
        number = sum(digits_of(number))
    return number

I'm pretty sure my 6 year old daughter could make sense of that last function with very little help.


Stored procedure to classify policies by sales date Currently this is the store procedure I am working on, it is working according to some testing that was done. I feel that it could be improved though, could use some optimization, feedback or recommendations.

What is it supposed to do?  For given policy id passed in, grab all coverages associated with the policy. For each coverage get units that are tied to the coverage. I need to check for 3 conditions for all units for a coverage which is based on Sales_Closing_Date. Depending on the condition update the coverage table with the flag.

DECLARE @Policy_ID BIGINT = 0,
        @CurrCoverageID BIGINT = 0,
        @Year INT = 0
DECLARE @Coverages TABLE(CID INT IDENTITY(1,1),CoverageID BIGINT NOT NULL)
DECLARE @Units TABLE(UID INT IDENTITY(1,1),CoverageID BIGINT NOT NULL,SalesClosing DATE)
DECLARE @CoveragesOut TABLE(CoverageID BIGINT NOT NULL, MultiFlag CHAR(1))

SET @Year = (SELECT p.Reins_Year FROM dbo.Policy p WHERE p.Policy_ID = @Policy_ID)

--INSERT ALL COVERAGES FOR THIS POLICY
INSERT INTO @Coverages(CoverageID)
SELECT Coverage_ID FROM dbo.Coverages c WHERE c.Policy_ID = @Policy_ID

--GO THROUGH COVERAGE AND GET THEIR SALES CLOSING DATE
WHILE (SELECT COUNT(*) FROM @Coverages) > 0
BEGIN
    SET @CurrCoverageID = (SELECT TOP 1 CoverageID FROM @Coverages)
    INSERT INTO @Units(CoverageID,SalesClosing)
    SELECT DISTINCT
        @CurrCoverageID,
        CAST(pu.Sales_Closing_Date AS DATE)
    FROM dbo.Policy_Units pu
    WHERE pu.Coverage_ID = @CurrCoverageID
        AND pu.Sales_Closing_Date IS NOT NULL

    --WE SHOULD HAVE THE SALES CLOSING DATES NOW
    --INSERT INTO OUTPUT TABLE NOW...
    INSERT INTO @CoveragesOut(CoverageID,MultiFlag)
    SELECT DISTINCT
        u.CoverageID,
        CASE (SELECT COUNT(*) FROM @Units)
            WHEN 1 THEN
                CASE
                    --SPRING--
                    WHEN u.SalesClosing BETWEEN CAST('1/1/' + CAST(@Year AS VARCHAR) AS DATE) AND CAST('4/30/' + CAST(@Year AS VARCHAR) AS DATE) OR u.SalesClosing >= CAST('1/1/' + CAST(@Year AS VARCHAR) AS DATE) AND u.SalesClosing < CAST('5/1/' + CAST(@Year AS VARCHAR) AS DATE) THEN 'S'
                    --FALL--
                    WHEN u.SalesClosing BETWEEN CAST('7/1/' + CAST(@Year - 1 AS VARCHAR) AS DATE) AND CAST('12/31/' + CAST(@Year - 1 AS VARCHAR) AS DATE) OR u.SalesClosing >= CAST('7/1/' + CAST(@Year - 1 AS VARCHAR) AS DATE) AND u.SalesClosing < CAST('1/1/' + CAST(@Year AS VARCHAR) AS DATE) THEN 'F'
                END
            WHEN 2 THEN 'B'
            ELSE NULL
        END AS MultiFlag
    FROM @Units u

    --REMOVE THE CURRENT COVERAGE FROM TABLE VAR
    DELETE FROM @Coverages WHERE CoverageID = @CurrCoverageID

    --REMOVE UNITS FROM TABLE VAR
    DELETE FROM @Units

END

--FOR TESTING PURPOSES
SELECT c.Description,c.County_Description, co.*
FROM @CoveragesOut co
INNER JOIN dbo.Coverages c
ON c.Coverage_ID = co.CoverageID

If you need anything else, please let me know. Again, this query is working just fine.

I will try to cover several points:

1) Naming

Try to use homogeneous naming. Pascal/Camel case seem too be used most often, so stick to it. E.g. @Policy_ID should be @PolicyId, CoverageID should be CoverageId.

2) Improve readability of your declarations

Your table variables could be declared like this:

DECLARE @Coverages TABLE
(
   CID INT IDENTITY(1,1),
   CoverageID BIGINT NOT NULL
)

DECLARE @Units TABLE
(
   UID INT IDENTITY(1,1),
   CoverageID BIGINT NOT NULL,
   SalesClosing DATE
)

DECLARE @CoveragesOut TABLE
(
   CoverageID BIGINT NOT NULL,
   MultiFlag CHAR(1)
)

3) Put clauses on separate lines for better readability

First statements could be rewritten like this:

SET @Year =
   (
      SELECT p.Reins_Year
      FROM dbo.Policy p
      WHERE p.Policy_ID = @Policy_ID
   )

--INSERT ALL COVERAGE FOR THIS POLICY
INSERT INTO @Coverages (CoverageID)
SELECT Coverage_ID
FROM dbo.Coverages c
WHERE c.Policy_ID = @Policy_ID

4) Defensive set

If by any chance, the select for year returns more that one year, set statement will fail. You might consider assigning with SELECT and/or using TOP 1, just to be sure it does not fail (however, it may return an incorrect value).

SET @Year =
   (
      SELECT TOP 1 p.Reins_Year
      FROM dbo.Policy p
      WHERE p.Policy_ID = @Policy_ID
   )

5) Easier date arithmetic

You have a case for spring and fall that performs some ad-hoc computation, that can be simplified. Also, since @Year is computed before the cycle, all those boundaries can be precomputed. You did not specify SQL Server version, so I hope for the best (2012+):

 DECLARE @StartOfYear DATE = DATEFROMPARTS(@Year, 1, 1)
 DECLARE @EndOfApril DATE = DATEFROMPARTS(@Year, 4, 30)
 DECLARE @FirstOfMay DATE = DATEFROMPARTS(@Year, 5, 1)

so, that the "Spring" CASE would be something like this (reformatted for better readability):

WHEN u.SalesClosing BETWEEN @StartOfYear AND @EndOfApril
   OR u.SalesClosing >= @StartOfYear
   AND u.SalesClosing < @FirstOfMay
THEN 'S'

However, logic for the dates is unclear for me, even after reading operator precedence (AND takes precedence over OR and BETWEEN). It is strongly recommended to use parenthesis when using multiple operators with close precedence.

6) Set based logic

WHILE and other looping constructs are intruders in T-SQL language. They will usually behave (much) poorer than set based approaches. So, your WHILE can be converted into something this:

;WITH U_CTE AS (
    SELECT DISTINCT C.CoverageID, CAST(pu.Sales_Closing_Date AS DATE),
        COUNT(1) OVER (PARTITION BY C.CoverageId) AS Cnt
    FROM dbo.Policy_Units PU
        JOIN @Coverages C ON PU.Coverage_ID = C.CoverageID
)
INSERT INTO @CoveragesOut (CoverageID, MultiFlag)
SELECT DISTINCT
    u.CoverageID,
    CASE
        WHEN @Cnt = 1 THEN
            CASE
               WHEN -- spring logic
               WHEN -- autumn logic
        WHEN 2 THEN 'B'
    ELSE NULL
FROM U_CTE

Also, @Units table is not required anymore and the whole block is slimier.

7) Proper data types

CAST(pu.Sales_Closing_Date AS DATE) suggests that Sales_Closing_Date is not a DATE (maybe a DATETIME or VARCHAR). If possible, use the most appropriate data type to avoid explicit or implicit (silently occurring in queries) casts.

8) Comments

It's a matter of taste, but I like comments written as normal text, not using CAPS


Base for iterating over git history I wanted to do some statistics over the history of a Git repository. At first I tried using GitPython, but it wasn't as trivial as I imagined it to be. In the end, I just call the necessary git commands with the submodule module.

Can this code be considered clean and readable or does it have some style issues?

import argparse
import os
import subprocess

def main(args):
    if not os.path.isdir(args.path):
        print "not a directory"
        return
    if ".git" not in os.listdir(args.path):
        print "not a repo"
        return

    os.chdir(args.path)

    commitIDs = []
    start = 0       #load commits in batches, to avoid too long hangs
    max_count = 100
    while True:
        text = subprocess.check_output(["git", "rev-list", args.branch, "--max-count=%s" % max_count, "--skip=%s" % start])
        start += max_count
        for line in text.splitlines():
            commitIDs.append(line)
        #print "loaded", len(commits), "commits"
        if len(text.splitlines()) != max_count:
            break

    for commitID in commitIDs[::-args.skip]:    #start with the oldest commit
        print commitID
        #do something with the commit
        #for example:
        #devnull = open(os.devnull, 'w')
        #subprocess.call(["git", "checkout", commitID], stdout=devnull)

if __name__ == '__main__':
    parser = argparse.ArgumentParser()
    parser.add_argument('path', nargs="?", default=".")
    parser.add_argument('--branch', '-b', default="master")
    parser.add_argument('--skip', '-s', type=int, default=1, help='use every n-th commit')
    args = parser.parse_args()

    main(args)

Update:

import argparse
import os
import subprocess
import sys
import git

def main(args):
    try:
        repo = git.Repo(args.path)
    except:
        sys.exit("no such repo")

    try:
        text = repo.git.rev_list(args.branch).splitlines()
    except:
        sys.exit("no such branch")

    commit_ids = text[::args.skip]

    print "loaded %s commits" % len(commit_ids)

    for commit_id in commit_ids[::-1]:  #start with the oldest commit
        print commit_id
        #do something with the commit
        #for example:
        #repo.git.checkout(commit_id)

if __name__ == '__main__':
    parser = argparse.ArgumentParser()
    parser.add_argument('path', nargs="?", default=".")
    parser.add_argument('--branch', '-b', default="master")
    parser.add_argument('--skip', '-s', type=int, default=1, help='use every n-th commit')
    args = parser.parse_args()

    main(args)
    sys.exit(0)

Great, glad you got rid of that loop: some more things I'd like to point out. As this is essentially another question now I'll add another answer

I don't think a function should call sys.exit(). I'd rather you raised an exception instead and call sys.exit() in __main__

I.e. instead of:

try:
    repo = git.Repo(args.path)
except:
    sys.exit("no such repo")

I would prefer:

from git import Repo, InvalidGitRepositoryError
#...

try:
    repo = git.Repo(args.path)
except InvalidGitRepositoryError, e:
    raise InvalidGitRepositoryError("%s is not a git repository" % (str(e)))

This enables other Python classes to use your main function (which is a terrible name) in a more predictable way (Python doesn't close calling your function).
I would also rather you check for the branch than assume if rev-list gives an error that the branch doesn't exist. There may be some other case that causes it to throw an exception.

class InvalidBranchError(Exception):
    pass

if args.branch not in repo.branches:
   raise InvalidBranchError("Branch does not exist: %s" % (args.branch))

Instead of main taking an arguments parser, I would prefer you used regular arguments (again so other python modules can use your code more readily):

def main(path=".", branch="master"):
   pass

main(path=args.path, branch=args.branch)


Print out a Zig-Zag Matrix in Haskell My task is to code something that prints out this:

This is what I have done so far.

diag matrix = diag2 [] [] matrix where
 diag2 []  []           []       = []
 diag2 xss []           (zs:zss) =     diag2 []       (reverse $ zs:xss) zss
 diag2 xss ((y:ys):yss) zss      = y : diag2 (ys:xss) yss                zss
 diag2 xss ([]    :yss) zss      =     diag2 xss      yss                zss
 diag2 xss []           []       =     diag2 []       (reverse xss)      []

Is there an easier way to code this?  Also, how can I solve it if this is a very big matrix?

One inefficiency of your solution is that is calls reverse. This means that it will have to traverse an entire row. It also means it won't work with infinite lists.

The best implementation I've found is Data.Universe.Helpers.diagonals from the universe-base package.

It can even operate on an infinite list of infinite lists:

import Data.Universe.Helpers
import Control.Monad

mat = [ [i..] | i <- [1..] ]

example n = forM_ (take n $ diagonals mat) $ print

e.g.:

ghci> example 4
[1]
[2,2]
[3,3,3]
[4,4,4,4]

More info available at this SO answer: https://stackoverflow.com/a/32469565/866915


Pre-calculate the winning combinations for an n-sized tic tac toe board using C++ functional programming This code pre-calculates the winning combinations for an n-sized tic tac toe board.

I first created my function using an imperative approach.  This is just how I naturally write code most of the time.  I then tried to think about the problem using a "What I want to accomplish?" approach, rather than "What do I want it to do?"  In the end I think I still ended up with an imperative approach, just using STL algorithms as loops.

Imperative approach:

std::vector<std::vector<int>> rules3(const int x){
    using namespace std;

    vector<vector<int>> seqs;
    for(int n=0; n<x; ++n){
        vector<int> seq;
        for(int m=0; m<x; ++m){
            seq.push_back(n*x+m);
        }
        seqs.push_back(seq);
    }
    for(int n=0; n<x; ++n){
        vector<int> seq;
        for(int m=0; m<x; ++m){
            seq.push_back(n+x*m);
        }
        seqs.push_back(seq);
    }
    vector<int> seq;
    for(int n=0; n<x; ++n){
        seq.push_back(n*x+n);
    }
    seqs.push_back(seq);
    seq.clear();
    for(int n=0; n<x; ++n){
        seq.push_back(x-1+n*(x-1));
    }
    seqs.push_back(seq);

    return seqs;
}

Getting more functional:

std::vector<std::vector<int>> rules2(const int x){
    using namespace std;

    vector<vector<int>> seqs;
    vector<int> iter(x);
    iota(iter.begin(), iter.end(), 0);

    for(auto n: iter){
        vector<int> seq;
        for(auto m: iter){
            seq.push_back(n*x+m);
        }
        seqs.push_back(seq);
    }
    for(auto n: iter){
        vector<int> seq;
        for(auto m: iter){
            seq.push_back(n+x*m);
        }
        seqs.push_back(seq);
    }
    vector<int> seq;
    seq.clear();
    for(auto n: iter){
        seq.push_back(n*x+n);
    }
    seqs.push_back(seq);
    seq.clear();
    for(auto n: iter){
        seq.push_back(x-1+n*(x-1));
    }
    seqs.push_back(seq);

    return seqs;
}

Functional approach? (still feels imperative):

std::vector<std::vector<int>> rules1(const int x){
    using namespace std;

    vector<vector<int>> seqs;
    vector<int> iter(x);
    iota(iter.begin(), iter.end(), 0);

    transform(iter.begin(), iter.end(), back_inserter(seqs), [&](const int& n){
        vector<int> seq;
        transform(iter.begin(), iter.end(), back_inserter(seq), [&](const int& m){
            return n*x+m;
        });
        return seq;
    });
    transform(iter.begin(), iter.end(), back_inserter(seqs), [&](const int& n){
        vector<int> seq;
        transform(iter.begin(), iter.end(), back_inserter(seq), [&](const int& m){
            return n+x*m;
        });
        return seq;
    });
    vector<int> seq;
    transform(iter.begin(), iter.end(), back_inserter(seq), [&](const int& n){
        return n*x+n;
    });
    seqs.push_back(seq);
    seq.clear();
    transform(iter.begin(), iter.end(), back_inserter(seq), [&](const int& n){
        //return (x-n-1)*x+n; // 6,4,2
        return x-1+n*(x-1); // 2,4,6
    });
    seqs.push_back(seq);

    return seqs;
}

Output when printing:

rules3
std::vector(0, 1, 2)
std::vector(3, 4, 5)
std::vector(6, 7, 8)
std::vector(0, 3, 6)
std::vector(1, 4, 7)
std::vector(2, 5, 8)
std::vector(0, 4, 8)
std::vector(2, 4, 6)
rules2
std::vector(0, 1, 2)
std::vector(3, 4, 5)
std::vector(6, 7, 8)
std::vector(0, 3, 6)
std::vector(1, 4, 7)
std::vector(2, 5, 8)
std::vector(0, 4, 8)
std::vector(2, 4, 6)
rules1
std::vector(0, 1, 2)
std::vector(3, 4, 5)
std::vector(6, 7, 8)
std::vector(0, 3, 6)
std::vector(1, 4, 7)
std::vector(2, 5, 8)
std::vector(0, 4, 8)
std::vector(2, 4, 6)

Here are some suggestions.  This answers uses the Range-v3 library and assumes

#include <range/v3/all.hpp>

namespace view = ranges::view;

Please include the #includes and supply a small test program in the future.  You probably have written them anyway, so why not post them to save reviewers' time? :)
int may be too small for indexes.  Consider using std::size_t.  You can define a type alias for flexibility.

using index_t = std::size_t;

The type std::vector<std::vector<index_t>> occurs many times.  Save time by writing

using rule_t = std::vector<index_t>
using rules_t = std::vector<rule_t>;

The concept "\$n \times n\$ board" only makes sense when \$n\$ is a positive integer.  Therefore, enforce the pre-condition \$n \ge 1\$.  Also, the name x is a bit vague.  size may be better.

rules_t rules(index_t size)
{
    if (size == 0)
        throw std::invalid_argument{"..."};
    // ...
}

rules(1) currently returns {{0}, {0}, {0}, {0}}, which is definitely wrong.  It should return {{0}}.  Your general logic is "\$n\$ rows + \$n\$ columns + \$2\$ diagonals", which only applies to \$n \ge 2\$.  The easiest approach would be a special case:

rules_t rules(index_t size)
{
    // ...
    else if (size == 1)
        return {{0}};
}

Following the previous bullet, why not make separate functions to make your logic clear?

rules_t rules(index_t size)
{
    // ...
    else {
        auto rows = rules_row(size);
        auto columns = rules_column(size);
        auto diagonals = rules_diagonal(size);
        return ranges::to<rules_t>(view::concat(rows, columns, diagonals));
    }
}

where view::concat concatenates the three vectors and ranges::to converts the result to the desired return type.  (Note that view::concat is disabled with rvalues to prevent dangling iterators, so we first store the subvectors.)
The rules_row function is easy to write:

rules_t rules_row(index_t size)
{
    return view::ints(index_t(0), size * size) | view::chunk(size);
}

view::ints generate a left-inclusive sequence of integers {0, 1, 2, ..., size * size - 1}, and view::chunk(size) breaks it into chunks each of size size.
The rules_column function is a bit tricky, because the Range library does not provide a function like chunk that splits like this.  We do have a function stride, so we can write a manual "loop":  (it took me quite a while to figure this out, so tell me if there is a better way!)

rules_t rules_column(index_t size)
{
    return view::ints(index_t(0), size) |
               view::transform([=](index_t col) {
                   return view::ints(index_t(col), size * size) | view::stride(size);
               });
}

view::ints(index_t(0), size) generates the column numbers.  Each of them is passed to the lambda.  The lambda returns the corresponding rule for each column.
The rules_diagonal function is moderately simple:

rules_t rules_diagonal(index_t size)
{
    return {
        view::ints(index_t(0), size) |
            view::transform([=](index_t r) { return r * (size + 1); }),
        view::ints(index_t(1), size + 1) |
            view::transform([=](index_t r) { return r * (size - 1); })
    };
}

Here, we always have two rules: one for the primary diagonal

{0 * size + 0, 1 * size + 1, 2 * size + 2, ..., (size - 1) * size + (size - 1)}

which is equivalent to

{0, 1, 2, ..., size - 1} * (size + 1)

and the other for the secondary diagonal

{size - 1, 2 * size - 2, 3 * size - 3, ... size * size - (size - 1)}

which is equivalent to

{1, 2, 3, ..., size + 1} * (size - 1)

Putting everything together:

#include <vector>
#include <range/v3/all.hpp>

namespace view = ranges::view;

using index_t = std::size_t;
using rule_t = std::vector<index_t>;
using rules_t = std::vector<rule_t>;

rules_t rules_row(index_t size)
{
    return view::ints(index_t(0), size * size) | view::chunk(size);
}

rules_t rules_column(index_t size)
{
    return view::ints(index_t(0), size) |
               view::transform([=](index_t col) {
                   return view::ints(index_t(col), size * size) | view::stride(size);
               });
}

rules_t rules_diagonal(index_t size)
{
    return {
        view::ints(index_t(0), size) |
            view::transform([=](index_t r) { return r * (size + 1); }),
        view::ints(index_t(1), size + 1) |
            view::transform([=](index_t r) { return r * (size - 1); })
    };
}

rules_t rules(index_t size)
{
    if (size == 0)
        throw std::invalid_argument{"A board cannot have size zero"};
    else if (size == 1)
        return {{0}};
    else {
        auto rows = rules_row(size);
        auto columns = rules_column(size);
        auto diagonals = rules_diagonal(size);
        return view::concat(rows, columns, diagonals);
    }
}

(live demo, tests the function for size = 1, 2, 3, ..., 10)


Transferring data for a given month from one sheet to another I have dollar amounts sorted by month that I want to transfer from sheet entry to sheet actuals. I only want to do it for a specified month prompted from user. Each dollar amount has a corresponding cost center/ID and description in separate columns. I want to copy the amounts and description from entry to the row in actuals matching the same ID.

Sometimes, multiple amounts share the same ID so I need to place them one below the other.

Sub month()

Dim actualsWS As Worksheet
Dim dataWS As Worksheet
Dim answer As String
Dim month As String
Dim loc As Range
Dim start As Integer
Dim rowCol As Variant
Dim dropRow As Integer
Dim dropCol As Integer
Dim locActual As Range

'store worksheets into variables
Set actualsWS = ThisWorkbook.Worksheets("Actuals by Month")
Set dataWS = ThisWorkbook.Worksheets("FAS Data Entry")

MsgBox ("If macro dies, it's likely a missing cost center.") 'warning for user

answer = InputBox(prompt:="What month?") 'prompt user for current month

'set month from user input
Select Case answer

    Case 1
    month = "January"
    'dropCol = 9
    Case 2
    month = "February"
    'dropCol = 10
    Case 3
    month = "March"
    'dropCol = 11
    Case 4
    month = "April"
    'dropCol = 12
    Case 5
    month = "May"
    'dropCol = 13
    Case 6
    month = "June"
    'dropCol = 14
    Case 7
    month = "July"
    'dropCol = 15
    Case 8
    month = "August"
    'dropCol = 16
    Case 9
    month = "September"
    'dropCol = 17
    Case 10
    month = "October"
    'dropCol = 18
    Case 11
    month = "November"
    'dropCol = 19
    Case 12
    month = "December"
    'dropCol = 20
    Case Else
    MsgBox ("Bad month. Exiting...")
    Exit Sub

End Select

Dim ccArray(99) 'cost center
Dim amountArray(99) As Currency 'amount
Dim descriptArray(99) 'description
Dim ccArrayLength As Integer

With dataWS

    'locate starting range for current month
    Set loc = .Cells.Find(month)

    'check previous row for carried over month (revising asset) Will break if 2 or more
    If loc.Offset(-1, 0) = "" Then 'if previous row is blank
        start = loc.row 'starting row is same row as found location above
    Else
        start = loc.row - 1 'else it's 1 row above
    End If

    i = 0

    'store data into arrays
    Do While .Cells(start + i, 1) <> "" 'loop through column A while there is a month present (not blank)

        ccArray(i) = .Cells(start + i, 11).Value 'store cost center
        amountArray(i) = .Cells(start + i, 13).Value 'store amount
        descriptArray(i) = .Cells(start + i, 9).Value 'store description

        i = i + 1

    Loop

End With

ccArrayLength = Application.Count(ccArray) 'store number of items

With actualsWS

    .Columns("V:W").ClearContents 'clear previous data

    'hide unnecessary months
    If month = "January" Or month = "February" Or month = "March" Or month = "April" Then
        .Columns("M:Q").EntireColumn.Hidden = True
    End If

    'drop data into respective cells onto "actuals by month" tab
    For x = 0 To ccArrayLength - 1

        On Error Resume Next
        Set locActual = .Cells.Find(ccArray(x)) 'find cost center location
        If locActual = 0 Then GoTo died 'not found
        'Debug.Print locActual
        'rowCol = locActual.Address
        dropRow = .Range(locActual.Address).row 'store row of cost center

        'for loop to find first empty cell to drop into
        For y = 0 To 99 'no cost center should have over 99 rows

            If .Cells(dropRow + y, 22).Value = "" Then 'column v blank
                .Cells(dropRow + y, 22).Value = amountArray(x) 'drop amount
                .Cells(dropRow + y, 22).Offset(0, 1).Value = descriptArray(x) 'drop description on row below
                Exit For
            End If

        Next

    Next

    .Cells(8, 22).Select 'bring cursor to top

End With

died:
'quit before msgbox if on last item
If x = ccArrayLength Then Exit Sub

MsgBox ccArray(x) & " not found." 'cost center not found
Exit Sub

End Sub

Sub month()

Dim month As String

Something isn't right, one of these two has the wrong name. Procedure names should start with a verb, they do something. If you can name that something, then you can name the procedure. If you can't name that something, then your procedure is doing too many things and needs to be broken down into something that can be named.

So we're getting the month name for some user input - if that's always going to be in English, then there's no need to hard-code that in a Select Case block.

Public Function ToMonthName(ByVal value As Integer) As String
    On Error GoTo CleanFail 'remove if runtime error 5 should bubble up to caller instead

    Dim result As String
    result = MonthName(value)

CleanExit:
    ToMonthName = result
    Exit Function
CleanFail:
    result = vbNullString
    Resume CleanExit
End Function

So, now we have the month name (or an empty string), that month procedure can actually start focusing on its task.

It's confusing that every code paths execute the died: label. There's no error handling - seeing that line label I was expecting to see On Error GoTo died somewhere.

So two things are happening here. First we're iterating dataWS and populating three arrays in parallel - there's a design smell right here: you have 3 data structures that are "in sync", when there should really be only one.

Make a small class module, and expose the 3 properties you're interested in:

Option Explicit
Private Type TModel
    CostCenter As String
    Description As String
    Amount As Single
End Type

Private this As TModel

Public Property Get CostCenter() As String
    CostCenter = this.CostCenter
End Property

Public Property Let CostCenter(ByVal value As String)
    this.CostCenter = value
End Property

Public Property Get Description() As String
    Description = this.Description
End Property

Public Property Let Description(ByVal value as String)
    this.Description = value
End Property

Public Property Get Amount() As Single
    Amount = this.Amount
End Property

Public Property Let Amount(ByVal value As Single)
    this.Amount = value
End Property

Now you can store instances of this class, which encapsulates the data you're interested in. This:

Do While .Cells(start + i, 1) <> "" 'loop through column A while there is a month present (not blank)

    ccArray(i) = .Cells(start + i, 11).Value 'store cost center
    amountArray(i) = .Cells(start + i, 13).Value 'store amount
    descriptArray(i) = .Cells(start + i, 9).Value 'store description

    i = i + 1

Loop

Becomes this:

Dim data As Collection
Set data = New Collection

Dim item As DataModel 'assuming class name was "DataModel"

Do While .Cells(start + i, 1) <> vbNullString 'loop through column A while there is a month present (not blank)

    Set item = New DataModel
    item.CostCenter = .Cells(start + i, 11).Value
    item.Amount = .Cells(start + i, 13).Value
    item.Description = .Cells(start + i, 9).Value

    data.Add item
    i = i + 1

Loop

And now if the body of that loop was extracted into its own function...

Private Function ReadModel(ByVal source As Range) As DataModel
    Dim result As New DataModel
    result.CostCenter = source.Cells(1, 11).Value
    result.Amount = source.Cells(1, 13).Value
    result.Description = source.Cells(1, 9).Value
    Set ReadModel = result
End Function

You could also define an Enum to get rid of the magic values there:

Private Enum ColumnPosition
    DescriptionColumn = 9
    CostCenterColumn = 11
    AmountColumn = 13
End Enum

And then use them to enhance readability here:

Private Function ReadModel(ByVal source As Range) As DataModel
    Dim result As New DataModel
    result.CostCenter = source.Cells(1, CostCenterColumn).Value
    result.Amount = source.Cells(1, AmountColumn).Value
    result.Description = source.Cells(1, DescriptionColumn).Value
    Set ReadModel = result
End Function

The calling code will now look like this:

Dim data As Collection
Set data = New Collection

Do While .Cells(start + i, 1) <> vbNullString 'loop through column A while there is a month present (not blank)

    data.Add ReadModel(.Range("A" & start + i).EntireRow)
    i = i + 1

Loop

And now if you ever need to read more values, that code doesn't even need to change - you can simply modify the DataModel accordingly, and the function that reads a row's values into an instance. And this loop and collection should go into its own function, too.

The number of items in the collection would be simply data.Count.

Next the loop that consumes this data, could be a For Each loop:

Dim item As DataModel
For Each item In data
    '...
Next

Again, this should be in its own procedure. On Error Resume Next shouldn't need to be there. What errors are being thrown under the carpet? They should be handled.


Project Euler: Find 10.001st prime I have just started learning C# and decided to do some problems from Project Euler. I wrote a code to find 10 001st prime and I thought it would be cool optimize it to make it as fast as possible. How could I improve this to make it even faster?

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;
using System.Diagnostics;

namespace primes
{
    class Program
    {
        static void Main(string[] args)
        {
            long prime = 2;
            long largestprime = 2;
            long potentialprime = 3;
            Console.WriteLine("Enter the prime you want to find: ");
            long primenum = Convert.ToInt64(Console.ReadLine());
            long[] primearray = new long[primenum + 1];
            primearray[1] = 2;
            Stopwatch sw = new Stopwatch();
            sw.Start();
            while (prime <= primenum)
            {
                bool isprime = true;
                for (long x = 1; x < prime; x += 3)
                {
                    if (primearray[x] * primearray[x] > potentialprime)
                         break;
                    if (prime > 3 && (potentialprime % primearray[x] == 0 || potentialprime % primearray[x+1] == 0 || potentialprime % primearray[x+2] == 0))
                         isprime = false;
                    if (prime <= 3 && potentialprime % primearray[x] == 0)
                         isprime = false;

                 }
                 if (isprime)
                {
                    primearray[prime] = potentialprime;
                    prime += 1;
                    largestprime = potentialprime;
                }
                potentialprime += 2;
            }
            sw.Stop();
            Console.WriteLine(sw.ElapsedMilliseconds + "ms to find ");
            Console.WriteLine(largestprime);
            Console.ReadKey();
         }
    }
}

As mentioned in the comments, you should go for more readable code:

1) Split by responsibility:

One method for user input:

ulong GetPrimeIndex() { // TODO: Get User input from Console }

The algorithm itself:

ulong FindNthPrime(ulong n) { // TODO: the algorithm }

print the result...

public static void Main(string[] args)
{
  ulong primeIndex = GetPrimeIndex();

  Stopwatch watch = Stopwatch.StartNew();
  ulong nthPrime = FindNthPrime(primeIndex);
  watch.Stop();

  Console.WriteLine($"{primeIndex} prime is: {nthPrime}");
  Console.WriteLine($"Elapsed time: {watch.ElapsedMilliseconds}");
}

2) Naming:

In C# local variable names use camelCase: largestprime should be largestPrime

Because primes are always  positive, you should use ulong instead of long. It will give you a broader range to operate in.

I think you're missing a couple of breaks in these tests:

                if (primearray[x] * primearray[x] > potentialprime)
                     break;
                if (prime > 3 && (potentialprime % primearray[x] == 0 || potentialprime % primearray[x+1] == 0 || potentialprime % primearray[x+2] == 0))
                     isprime = false;
                if (prime <= 3 && potentialprime % primearray[x] == 0)
                     isprime = false;

It should be:

  if (primearray[x] * primearray[x] > potentialprime)
    break;
  if (prime > 3 && (potentialprime % primearray[x] == 0 || potentialprime % primearray[x + 1] == 0 || potentialprime % primearray[x + 2] == 0))
  {
    isprime = false;
    break;
  }
  if (prime <= 3 && potentialprime % primearray[x] == 0)
  {
    isprime = false;
    break;
  }

Be aware that

  primearray[x] * primearray[x]

may overflow the domain range. You should maybe make the check by sqrt instead?


Labeling modified words I'm working on a Python function that takes a piece of text and indicates if words are modified by words like very or not.

I'm looking for instance to label very nice differently from not nice. This does what it's supposed to do but I feel it might be clunky or there might be a more very efficient approach I'm not aware of.

def polarizer(input_sentence, modifier_dict, window_length):
    """
    The polarizer attaches the label given in modifier_dict to all items in input_sentence occurring a window_length after the modifier.
    modifier_dict needs to be formatted like so: {'not':'-', 'yes':'+', ..}
    Returns list with labels added.
    """
    #find positions of modifiers
    indices= [(range(n+1, n+1+window_length), modifier_dict[i]) for n,i in enumerate(input_sentence) if i in modifier_dict.keys()]
    #iterate over modifiers
    for ind in indices:
        for n in ind[0]:
            #add label unless there is already a label attached
            if n < len(input_sentence) and not input_sentence[n].endswith(tuple(modifier_dict.values())):
                input_sentence[n]=input_sentence[n]+ind[1]
    outputsentence= input_sentence
    return outputsentence

#Test sentences
modifier_dict={'very':'+'}

test1="This is fine".split(" ")
test2="This is very fine".split(" ")

print polarizer(test1, modifier_dict, 2)
print polarizer(test2, modifier_dict, 2)

When there're comments in the code that tell what it does, it usually indicates that the following piece of code should be a separate function with a meaningful name. I'd create a separate function that the positions of the words to be modified (and it something like get_indices_to_modify) and a separate function that checks if a word already has a label.
You can make your code more readable the tuples from the indices list:

for modification_range, label in indices looks better than

for ind in indices: # Do something with ind[0] and ind[1]
You can also use the += operator in this line: input_sentence[n] += ind[1]
The names n and i are kind of confusing in your code. i normally stands for a counter while n stands for the number of elements. For instance, I'd change n and i in the first line of your polarize function to idx and word (when i is not the counter, the thing gets pretty confusing).
Assuming that it's python 2, you can save some space by using xrange instead of range. It can a have a serious impact if the window_length gets really big.
What is the point of creating the outputsentence variable just to return it? Why not return the input_sentence directly?
The polarize function actually changes the input_sentence. Your documentation doesn't say anything about it. This could lead to unexpected results for the users of your code. You should either make a copy and not change the input, or document the fact the input is changed explicitly.
Instead of printing the return value of the function to the standard output (and checking that it's correct by looking at it), I'd recommend to write unit-tests. This way, you would be able to check that code remains correct after your change it automatically (moreover, it's quite easy to overlook something when you just look at the output with your eyes).


CS50 - Bleep suggestions I finished CS50's bleep.py from pset6. I was hoping to get some feedback on my code (e.g., redundant code, bad practice, etc...).

Program function

load in a dictionary of banned words
accept a message from the user
if words in the message appear in the banned list, censor the word with *

from cs50 import get_string
from sys import argv, exit

def main():
    # ensure correct usage
    if len(argv) != 2:
        print("Usage: python bleep.py dictionary")
        exit(1)

    # open dictionary of bad words
    name = argv[1]
    text = open(name, "r", encoding="latin_1")
    if not text:
        print("Could not open {}.".format(name))
        exit(1)

    # store banned words in list
    bannedList = []
    for ban in text:
        ban = ban.replace('\n', '')
        bannedList.append(ban)

    # get user input to censor and split into words
    toCensor = input("What message would you like to censor? ")
    toCensor = toCensor.split()

    # for censored phrase
    final = []

    # check if word needs to be censored
    for word in toCensor:
        if word.lower() in bannedList:
            censored = word.replace(word, "*"*len(word))
            final.append(censored)
        else:
            final.append(word)

    print(' '.join(final))

if __name__ == "__main__":
    main()

EDIT

I implemented the suggestions, and I like it a lot better now. It even handles commas, periods, and other punctuation now. Thanks again!

from cs50 import get_string
from sys import argv, exit
import re

def censor(text, banned_words):
    # for censored phrase
    final = []

    # check if word needs to be censored
    for word in text:
        if (word.lower()).strip(",.!?") in banned_words:
            censored = re.sub('[a-zA-Z]', '*', word)
            final.append(censored)
        else:
            final.append(word)

    #return censored phrase
    return ' '.join(final)

def main():
    # ensure correct usage
    if len(argv) != 2:
        print("Usage: python bleep.py dictionary")
        exit(1)

    # open dictionary of bad words
    name = argv[1]
    with open(name, 'r', encoding="UTF-8") as f:
        lines = [line.rstrip() for line in f]

    # get user input to censor and split into words
    to_censor = input("What message would you like to censor? ")
    to_censor = to_censor.split()

    #censor and obtain new phrase
    censored = censor(to_censor, lines)
    print(censored)

if __name__ == "__main__":
    main()

Having a single main function that reads from os.stdin and writes the unrelated prompt to os.stdout is a bad architecture. In Python it is easy to create reusable modules, by using the following structure:

def censor(text, banned_words):
    """
    Returns a copy of the given text in which all words from
    banned_words are replaced with asterisks of the same length.
    """

    # TODO: insert some code here.

def main():
    # contains all the input/output stuff
    # TODO: insert some code here.

if __name__ == "__main__":
    main()

Programmed this way, other Python code can just import censor and then use the censoring code, no matter where the set of banned words comes from.

Having the code split into two separate functions also makes sense for understanding what the code does. If you're interested in how censoring works, look at the censor function. If you are interested in reading and writing to files, look in main.

Some other random remarks:

text = open(name, "r", encoding="latin_1")

Please don't use the latin_1 encoding if possible. It's been en vogue in the 1990, nowadays we prefer to use the full Unicode character set (including Arabic, Bengali, Chinese, Devanagari, Emoji, and many more) and UTF-8 to save Unicode text into byte-oriented files.

Most probably that file contains only ASCII characters anyway since CS50 takes place in the USA, where foreign languages and writing systems are often ignored.

text = open(name, "r", encoding="latin_1")
if not text:
    ...

This will not work since the open function either succeeds by returning a file object, or it fails and throws an exception. It never returns None. That's probably a leftover from your last exercise in the C programming language. But Python is very different from C.

Whenever you open a file, you must also close it when you're done with it. The simplest way is:

with open(name, 'r', encoding='UTF-8') as f:
    lines = f.readlines()

The with statement will automatically close the file at the end. It's not visible in the source code, you have to just know this.

toCensor = input("What message would you like to censor? ")

Be sure to only ever run this program in Python 3, since in Python 2 the input function behaves differently.

The name toCensor should rather be to_censor since in Python variable names and function names are written in snake_case instead of camelCase.

In the sentence This is BAD, BAD, BAD., assuming that BAD is a bad word, what would the expected output be? How can you change your program to do the expected thing? What should it do about BADABOOM?


List of all prime factors of a number, including repeats This code will return a list of the prime factors of n, and the prime factors are repeated so that the product of the list is equal to n.

import math

def product(iterable):
    """
    Returns product of all elements in iterable
    """
    p = 1
    for x in iterable:
        p *= x
    return p

def generate_primes(n):
    """
    Returns list of all primes less than or equal to n
    """
    l = []
    for i in range(1,n+1):
        prime = True # Assume i is prime
        for j in l:
            if j > math.sqrt(n):
                break
            if i % j == 0 and j != 1:
                prime = False # Number i has been shown to be composite
                break
        if prime:
            l.append(i)
    return l

def prime_factor_set(n):
    """
    Returns set of the prime factors of n
    """
    return {i for i in generate_primes(n) if n % i == 0 and i != 1}

def prime_factor_list(n):
    """
    Returns full list of the prime factors of n, such that the product of this
    list is equal to n
    """
    l = list(prime_factor_set(n))
    while product(l) != n:
        r = int(n/product(l)) # Remaining number to be factored into primes
        prime_factors = prime_factor_set(r)
        l.extend(prime_factors)
    return sorted(l)

Example:

prime_factor_list(2000)

[2, 2, 2, 2, 5, 5, 5]

Some general comments:

Hooray for docstrings! It’s great that you’re writing these – it makes your code much easier to read and review, because I know what it’s supposed to do (and so can judge whether it does it correctly). And in general, these are pretty good docstrings.

Do note that PEP 257 says that one-line docstrings should end in a period.
Don’t be stingy with variable names – characters are cheap, and if you write more descriptive names, the code becomes much easier to follow.

For example, in generate_primes(), known_primes is better than l.
There are a few minor PEP 8 violations – in particular, space after comma in function arguments (for i in range(1,n+1)) and a double-space before inline comments.
Use generators! Generators allow you to do lazy evaluation of large iterables (such as a list of prime numbers) – which can make a big difference for performance. If you’re not familiar with generators, they’re a really powerful tool in Python. I recommend Ned Batchelder’s PyCon talk.

Okay, now comments on the specific functions:

product() – no comments, looks fine.
generate_primes() – it’s a bit hard to follow, because of the short variable names. It might be helpful to have some logic explaining the motivation behind the different steps, although on inspection it appears to be mostly sound.

It does return 1 as a prime number. Convention usually dictates that 1 isn’t  a prime number – there might be a good reason to deviate from convention, but you should document it if so.

You could also hard-code 2 as the only even prime number, and then just check odd numbers after that – skip half the checks you have to do.

This is what a revised version of that function might look like, with better comments and variables names, and using generators:

def generate_primes(n):
    """Generates all the primes less than or equal to n."""
    if n == 1:
        return

    known_primes = [2]
    yield 2

    for i in range(3, n+1, 2):

        # Assume that i is prime until proven otherwise.
        is_prime = True

        for existing_prime in known_primes:

            # As i <= n, if i is coprime to all the primes <= sqrt(n),
            # it must be that i itself is prime.
            if existing_prime > math.sqrt(n):
                break

            # If i is a multiple of another prime number, then i itself
            # cannot be prime.
            if i % existing_prime == 0:
                is_prime = False
                break

        if is_prime:
            known_primes.append(i)
            yield i

prime_factor_set() – aside from the quirk with i != 1 to account for generate_primes(), this is fine. But let’s make it a lazy generator:

def prime_factors(n):
    """Generates the prime factors of n."""
    for prime in generate_primes(n):
        if n % prime == 0:
            yield prime

prime_factor_list() – I find the docstring and name a little confusing. It sounds like you’re returning a list of prime factors – in fact, what you have is the prime factorisation of n.

Your approach makes many calls into prime_factor_set, which in turn calls generate_primes. This is quite an expensive function, so you minimise the number of calls you require. In fact, you can get away with just 1. Get a list of all the prime factors of n, and then use them to chip away at n until they’re exhausted. Like so:

def prime_factorization(n):
    """Generates the prime factorization of n."""
    for factor in prime_factors(n):
        while n % factor == 0:
            yield factor
            n /= factor
        if n == 1:
            break

With our new generators-based approach, this is significantly faster than the original, because it only computes the prime factors of n as it requires them.


NLP sentiment analysis in Norwegian Please keep in mind that I am very new to data science and completely new to NLP! I am trying to create a model to classify customer reviews as either negative or positive. However, my main problem is that most of them are in Norwegian, which seems not to be very well supported. I have found this repo https://github.com/ltgoslo/norsentlex/tree/master/Fullform containing both negative and positive lexicons and have decided to use it.

with open("Fullform_Positive_lexicon.txt") as f:
    reader = csv.reader(f)
    positive_lexicon = []
    for row in reader:
        print(str(row))
        positive_lexicon.append(row[0])

with open("Fullform_Negative_lexicon.txt") as f:
    reader = csv.reader(f)
    negative_lexicon = []
    for row in reader:
        negative_lexicon.append(row[0])

#adjusted to use it with NaiveBayesClassifier
def get_tokens_for_model(cleaned_tokens_list):
    final_token_list = []
    for token in cleaned_tokens_list:
        token_dict = {}
        token_dict.update({token : True})
        final_token_list.append(token_dict)
    return final_token_list

positive_tokens_for_model = get_tokens_for_model(positive_lexicon)
negative_tokens_for_model = get_tokens_for_model(negative_lexicon)

positive_dataset = [(token, "Positive")
                     for token in positive_tokens_for_model]

negative_dataset = [(token, "Negative")
                     for token in negative_tokens_for_model]

dataset = positive_dataset + negative_dataset
#shuffle dataset
for i in range(len(dataset)-1, 0, -1):
    j = random.randint(0, i + 1)
    dataset[i], dataset[j] = dataset[j], dataset[i]

train_data = dataset[:20000]
test_data = dataset[7742:]

classifier = NaiveBayesClassifier.train(train_data)

print("Accuracy is:", classify.accuracy(classifier, test_data))

So I am just training my model based on this lexicon and then I am trying to apply it to my comments. I am getting here 87%, which is not so bad. However, it looks worse when used with whole sentences.

with open("stopwords.csv") as f:
    reader = csv.reader(f)
    norwegian_stopwords = []
    for row in reader:
        norwegian_stopwords.append(row[0])

customer_feedback = pd.read_excel("classification_sample.xlsx")
customer_feedback = customer_feedback.dropna()
customer_feedback['Kommentar'] = customer_feedback['Kommentar'].apply(remove_punctuation)

list_of_comments = list(customer_feedback['Kommentar'])

for comment in list_of_comments:
    custom_tokens = word_tokenize(comment)
    filtered_tokens = []
    for word in custom_tokens:
        if word not in  norwegian_stopwords:
            filtered_tokens.append(word)

    classification = classifier.classify(dict([token, True] for token in filtered_tokens))
    probdist = classifier.prob_classify(dict([token, True] for token in filtered_tokens))
    pred_sentiment = probdist.max()
    print("Sentence: " + comment)
    print("Classified as: " + str(classification))
    print("Key words: " + str(custom_tokens))
    print("Probability: " + str(round(probdist.prob(pred_sentiment), 2)))
    print("-----------------------------------------------------------")

I know my code is not the highest quality right now(although suggestions regarding this are also appreciated). I am looking mostly for some feedback what more can I do to improve my accuracy. What is currently not very clear to me, is how to properly train the model on words and then achieve the very same accuracy on sentences.

All of this:

def get_tokens_for_model(cleaned_tokens_list):
    final_token_list = []
    for token in cleaned_tokens_list:
        token_dict = {}
        token_dict.update({token : True})
        final_token_list.append(token_dict)
    return final_token_list

positive_tokens_for_model = get_tokens_for_model(positive_lexicon)
negative_tokens_for_model = get_tokens_for_model(negative_lexicon)

positive_dataset = [(token, "Positive")
                     for token in positive_tokens_for_model]

negative_dataset = [(token, "Negative")
                     for token in negative_tokens_for_model]

dataset = positive_dataset + negative_dataset
#shuffle dataset
for i in range(len(dataset)-1, 0, -1):
    j = random.randint(0, i + 1)
    dataset[i], dataset[j] = dataset[j], dataset[i]

Is just a very verbose way to write:

dataset = [({token : True}, "Positive") for token in positive_lexicon]
dataset.extend([({token : True}, "Negative") for token in negative_lexicon])
random.shuffle(dataset)

The shuffling probabilities might be a bit different, though. random.shuffle basically has for each element equal probability to end up at any index. I think your method has a bias, but I'm not quite sure.

At least your custom shuffling seems to be unbiased:

However, I had to fix it to this, because otherwise it raises an IndexError, because random.randint is inclusive of the end (in contrast to range, slices and random.randrange):

#shuffle dataset
for i in range(len(dataset)-1, 0, -1):
    j = random.randint(0, i)
    dataset[i], dataset[j] = dataset[j], dataset[i]


AES Encryption C# .NET I have written a tool for encrypting string using the AesCryptoServiceProvider in .NET. The folllowing parameters are used:

Block Cipher Mode: CBC
Initialization Vector: 16 bytes (randomized
per encryption operation), stored in last 16 bytes of final message
Key/Block size: 128 bit Padding: PKCS #7

Overview of code
I have added two functions below. The first prepares a message for encryption, takes the result and appends the Initialization Vector to the cipher message. The second function encrypts the provided message using the specified key and initialization vector.

Obviously, i'm interested in any security holes that exist with this code. Also any optimizations are very much appreciated.

Side questions. In development I always store bytes as strings in their hex representation, seen below. Is this bad? The only other alternative I see is  base64 encoding? Which is better?

public void cryptoExample {

    AesCryptoServiceProvider aes = new AesCryptoServiceProvider();
    aes.KeySize = 128;
    aes.BlockSize = 128;

    aes.Key = Helper.StringToByteArray("FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF"); // Sample key
    aes.GenerateIV();

    String plainMessage = "Hello AES Crypto!!";

    String encryptedMessage = Helper.ByteArrayToString(CryptoHelper.EncryptStringToBytes_Aes(plainMessage, aes.Key, aes.IV));

    // Must always use a random IV, meaning it needs to be stored with the cipher message.
    // Append it to the end. IV is a constant 16 bytes so we just extract and remove it before decryption.
    String strIv = Helper.ByteArrayToString(aes.IV);

    String messageToTransmit = encryptedMessage += strIv;
}
public static byte[] EncryptStringToBytes_Aes(string plainText, byte[] Key, byte[] IV)
{
    // Check arguments.
    if (plainText == null || plainText.Length <= 0)
        throw new ArgumentNullException("plainText");
    if (Key == null || Key.Length <= 0)
        throw new ArgumentNullException("Key");
    if (IV == null || IV.Length <= 0)
        throw new ArgumentNullException("IV");
    byte[] encrypted;
    // Create an AesCryptoServiceProvider object
    // with the specified key and IV.
    using (AesCryptoServiceProvider aesAlg = new AesCryptoServiceProvider())
    {
        aesAlg.Key = Key;
        aesAlg.IV = IV;
        aesAlg.Mode = CipherMode.CBC;
        aesAlg.Padding = PaddingMode.PKCS7;
        // Create a decrytor to perform the stream transform.
        ICryptoTransform encryptor = aesAlg.CreateEncryptor(aesAlg.Key, aesAlg.IV);

        // Create the streams used for encryption.
        using (MemoryStream msEncrypt = new MemoryStream())
        {
            using (CryptoStream csEncrypt = new CryptoStream(msEncrypt, encryptor, CryptoStreamMode.Write))
            {
                using (StreamWriter swEncrypt = new StreamWriter(csEncrypt))
                {

                    //Write all data to the stream.
                    swEncrypt.Write(plainText);
                }
                encrypted = msEncrypt.ToArray();
            }
        }
    }
}

You can simplify it by replacing all those streams with a call to encrypter.TransformFinalBlock.
You don't have a MAC, so an attacker who can send a modifier message to the decrypter can probably exploit a padding oracle to decrypt the message
Why ask the caller for an IV? You could generate it internally. If you want to keep a deterministic implementation around, at least create an overload without IV.
Helper.ByteArrayToString and StringToByteArray should have a name that says which encoding (hex, Base64, etc.) they use. I'd use BytesToHexString and HexStringToBytes.


A Basic HashMap (Python) A hashmap is a data structure that implements an associative array abstract data type using keys and values and has a hash function to compute an index into an array, from which the desired value can be set and get [reference].

Based on some tutorial, I wrote this hashmap class:

class HashMap():
    def __init__(self):
        self.hahsmap_size = 32
        self.hashmap_data = [None] * self.hahsmap_size

    def __get_hash_mod_size(self, key):
        hash_key_var = hash(key+str(self.hahsmap_size*0.01))
        return hash_key_var % self.hahsmap_size

    def set_key_value(self, key, value):
        key_var = self.__get_hash_mod_size(key)
        key_value_list = [key, value]

        if self.hashmap_data[key_var] is None:
            self.hashmap_data[key_var] = list([key_value_list])
            return True
        else:
            for pair in self.hashmap_data[key_var]:
                print(pair)
                if pair[0] == key:
                    pair[1] = value
                    return True
            self.hashmap_data[key_var].append(key_value_list)
            return True

    def get_key(self, key):
        key_var = self.__get_hash_mod_size(key)
        if self.hashmap_data[key_var] is not None:
            for pair in self.hashmap_data[key_var]:
                if pair[0] == key:
                    return pair[1]
            return None

    def remove_key(self, key):
        key_var = self.__get_hash_mod_size(key)

        if self.hashmap_data[key_var] is not None:
            return False
        for i in range(len(self.hashmap_data[key_var])):
            if self.hashmap_data[key_var][i][0] == key:
                self.hashmap_data[key_var].pop(i)
                return True
    def print_hashmap(self):
        for item in self.hashmap_data:
            if item is not None:
                print(item)

hm = HashMap()
hm.set_key_value('A', '1')
hm.set_key_value('A', '2')
hm.set_key_value('B', '1')
hm.set_key_value('A', '3')
hm.set_key_value('A', '4')
hm.set_key_value('C', '1')
hm.set_key_value('D', '1')
hm.set_key_value('E', '1')
hm.set_key_value('E', '2')
hm.remove_key('A')
hm.remove_key('B')
hm.remove_key('B')
hm.print_hashmap()

If you had time, I'd appreciate a review.

The first thing that stands out here to me is that you should be using __get_item__ and __set_item__, and __repr__ instead of get-key, set-key-value and print_hashmap. This will opt you into the syntax of collections in python where you use d[key] to get a key, d[key] = val to set a value, and print(d) to print it. This may seem trivial, but the fact that python lets containers you have written to feel as nice as base ones is a large part of why Python is such a natural feeling language as opposed to something like Java, where this style would be considered correct.


Performing a sum with Swift I wrote the following in Swift to perform a sum:

let total:Double = session.courses.reduce(0) { $0 + $1.totalPrice() }

It's concise, it does the job but it does not seems very readable to me. Wouldn't a "classic" for loop be more explicit/readable/maintainable?

var total:Double = 0
for dish in session.courses {
    total += dish.totalPrice()
}

Should closures always be the way to go in Swift?

Both solutions are correct. Some advantages of the "functional" approach
with reduce():

Concise code.
Returns a constant. It is clear to the reader that
total is assigned to exactly once, and the compiler may use this
information for optimizing code.
The type annotation on the result value is not needed:

let total = session.courses.reduce(0) { $0 + $1.totalPrice() }

because that can be inferred from the compiler automatically.
This will continue to compile without changes if the type of
totalPrice() is changed, e.g. from Double to Float.

To increase legibility, you can use named closure parameters instead
of the $n shortcuts:

let total = session.courses.reduce(0) { (partialSum, dish) in
    partialSum + dish.totalPrice()
}


Reproduce Linux move command in C I have the following code, made in C language, which reproduces the functionality of the mv (move) command from linux. The problem is that the code is very inefficient. How I can optimize or make the code more efficient without changing the structure too much?

#include    <limits.h>
#include    <fcntl.h>
#include    "ourhdr.h"
#include    <sys/stat.h>
#include    <dirent.h>
int main(int argc,char *argv[])
{
    if(argc!=3)
    {
        printf("<Sintaxa> <fisier sursa> <fisier destinatie> \n");
        exit(1);
    }
    int ren1,ren2;
    struct stat buf1;
    struct stat buf2;
    lstat(argv[2],&buf1);
    lstat(argv[1],&buf2);
    char src[PATH_MAX];
    strcpy(src,argv[1]);
    char dst[PATH_MAX];
    strcpy(dst,argv[2]);
    int src_size = (int)strlen(src);
    int dst_size = (int)strlen(dst);
    dst[dst_size] = '/';
    dst[dst_size + 1] = '\0';
    dst_size++;
    int i;
    int index=0;
    int index1=src_size;
    while(--index1>0)
    {
        if(src[index1]=='/')
        {
            index1=0;
        }
        else
            index++;
    }
    for (i = src_size-index-1; i < src_size; i++)
    {
        dst[dst_size++] = src[i];
    }
    dst[dst_size] = '\0';
    if (access(src,F_OK)==-1)
    {
        err_ret("%s: cannot stat '%s'", argv[0],argv[1]);
        return -1;
    }
    if(S_ISDIR(buf1.st_mode))
    {
        if(((S_ISDIR(buf2.st_mode))&&(!(opendir(argv[1]))))||(!(opendir(argv[2]))))
        {
            err_quit("%s: cannot move '%s' to '%s': Permission denied \n", argv[0],argv[1],dst);
        }
        ren1=rename(src,dst);
        if(ren1!=0)
        {
            err_quit("Error: unable to move the file");
        }
    }
    else
    {
        ren2=rename(argv[1],argv[2]);
        if(ren2!=0)
        {
            err_quit("Error: unable to rename the file ");
        }
    }
}

The code above works fine but is way to inefficient

This is a small program, so some of the problems are not likely that big. If this were part of the API, problems may stack up and then make an actual difference.

There are several points which you should work on:

opendir leak: opendir opens a file descriptor. You open 2 directories, none of which are closed. Make sure to call closedir. If this is part of an API, those memory leaks will stack up. Otherwise, this might not be a problem, since when the process ends, the descriptors should be cleared automatically.
Error checks: If I were to run this code a lot of times, some of which are intended to end in errors, there would be some clatter due to the positioning of your error checks. Checks like does src exist or can dst dir be accessed, should be done first. This will eliminate the need for some operations. Generally, it is always best to perform error checks first.
Finding the name: A big part of the code is dedicated to finding the name of the src, and copying it to dst. You might improve it by using built-in functions instead. Use strrchr to find the last / and strcpy to copy the name.

Maybe there are other possible changes, but that should be the gist of it. Most of those problems have a small effect. This is a small program, so you won't notice must performance changes (unless this is an API).