Functional Knapsack Problem in Python This is the knapsack problem from rosettacode.org:

  A tourist wants to make a good trip at the weekend with his friends. They will go to the mountains to see the wonders of nature, so he needs to pack well for the trip. He has a good knapsack for carrying things, but knows that he can carry a maximum of only 4kg in it and it will have to last the whole day. He creates a list of what he wants to bring for the trip but the total weight of all items is too much. He then decides to add columns to his initial list detailing their weights and a numerical value representing how important the item is for the trip.

(The list of items, together with their weight in decagrams and their value, is given in the code below.)

  Which items does the tourist carry in his knapsack so that their total weight does not exceed 400 decagrams [4 kg], and their total value is maximised?

My solution is below. This seems almost seems to be too little.  Just sort by their efficiency (value/weight) and add the most efficient item until capacity is reached.

from itertools import takewhile

ITEMS = (
    ("map", 9, 150), ("compass", 13, 35), ("water", 153, 200), ("sandwich", 50, 160),
    ("glucose", 15, 60), ("tin", 68, 45), ("banana", 27, 60), ("apple", 39, 40),
    ("cheese", 23, 30), ("beer", 52, 10), ("suntan cream", 11, 70), ("camera", 32, 30),
    ("t-shirt", 24, 15), ("trousers", 48, 10), ("umbrella", 73, 40),
    ("waterproof trousers", 42, 70), ("waterproof overclothes", 43, 75),
    ("note-case", 22, 80), ("sunglasses", 7, 20), ("towel", 18, 12),
    ("socks", 4, 50), ("book", 30, 10),
    )

def item_efficiency(item):
    name, weight, value = item
    return float(value)/float(weight)

def pack_bag(item):
    name, weight, value = item
    pack_bag.max_weight -= weight
    return pack_bag.max_weight > 0
pack_bag.max_weight = 400  # static variable implementation

# pack the most efficient item until pack_bag.max_weight is reached.
pack = list(takewhile(pack_bag, reversed(sorted(ITEMS, key=item_efficiency))))

# print output
for item in pack:
    print item[0]

table = zip(*pack)
print "Total Value: %i" % sum(table[2])
print "Total Weight: %i" % sum(table[1])

I understand that this solution doesn't scale if capacity is multidimensional.  This gives the correct answer but I haven't seen any solution like it.  It could be because I am exporting a lot of the workload to sorted, reversed, and takewhile.

1. Code review

There's no documentation. What do the functions do, what arguments do they take, and what do they return?
The code is not portable to Python 3.
The program has code at the top level, which makes it inconvenient to test from the interactive interpreter, because as soon as it imports the module the code runs. Better to encapsulate the top-level code within a function.
The program uses a global variable, pack_bag.max_weight. Global variables make code inconvenient to test, because you have to get the globals into the right state before running each test case. It is more convenient (and more robust) to encapsulate all the state inside a function or object.
item_efficiency doesn't use the name of the item. It's conventional to indicate unused values in tuple unpacking by using the name _. Alternatively, you could use a collections.namedtuple and then you'd be able to write item.weight and item.value.
Use sorted(..., reverse=True) rather than reversed(sorted(...)).
As noted by Janne Karila in comments, itertools.takewhile stops as soon as it encounters an item for which pack_bag returns False, but there might be (less efficient, but lighter) items which can still be packed. This could be fixed by using filter instead of takewhile.

2. Revised code

from collections import namedtuple

Item = namedtuple('Item', 'name weight value'.split())

ITEMS = [
    Item("map", 9, 150),
    Item("compass", 13, 35),
    Item("water", 153, 200),
    Item("sandwich", 50, 160),
    Item("glucose", 15, 60),
    Item("tin", 68, 45),
    Item("banana", 27, 60),
    Item("apple", 39, 40),
    Item("cheese", 23, 30),
    Item("beer", 52, 10),
    Item("suntan cream", 11, 70),
    Item("camera", 32, 30),
    Item("t-shirt", 24, 15),
    Item("trousers", 48, 10),
    Item("umbrella", 73, 40),
    Item("waterproof trousers", 42, 70),
    Item("waterproof overclothes", 43, 75),
    Item("note-case", 22, 80),
    Item("sunglasses", 7, 20),
    Item("towel", 18, 12),
    Item("socks", 4, 50),
    Item("book", 30, 10),
    ]

def efficiency(item):
    """Return efficiency of item (its value per unit weight)."""
    return float(item.value) / float(item.weight)

def packing(items=ITEMS, max_weight=400):
    """Return a list of items with the maximum value, subject to the
    constraint that their combined weight must not exceed max_weight.

    """
    def pack(item):
        # Attempt to pack item; return True if successful.
        if item.weight <= pack.max_weight:
            pack.max_weight -= item.weight
            return True
        else:
            return False

    pack.max_weight = max_weight
    return list(filter(pack, sorted(items, key=efficiency, reverse=True)))

Note that in order to allow pack to modify max_weight each time it is called, I've stored the value as an attribute. This is a slightly ugly workaround that's necessary in Python 2. In Python 3 one would just use a nonlocal statement to give the pack access to the variable max_weight.

3. Algorithm

Your program implements the well-known "greedy approximation algorithm" for the knapsack problem (first described by George Dantzig in 1957).

As the name says, this is an approximation algorithm: that is, it does not always return the best solution. For example, when max_weight=80 the greedy algorithm (as implemented above) picks the following items, with total value 430:

map                     9  150
glucose                15   60
suntan cream           11   70
note-case              22   80
sunglasses              7   20
socks                   4   50
TOTAL                  68  430

But the following choice of items has total value 445:

map                     9  150
compass                13   35
glucose                15   60
suntan cream           11   70
note-case              22   80
socks                   4   50
TOTAL                  74  445

So even in small examples like this, you really do need to use the tabular method (aka "dynamic programming") to get the best answer. See this answer for one way to implement it in Python.


Batch convert images to PDF I wrote a simple program that converts graphic files in a folder to a single PDF using Magic.NET. It works fine. I'm interested in general comments about the code as well as suggestions how I can make it run faster. Answers with some code are especially welcome.

static void Main(string[] args)
{
    Stopwatch stopWatch = new Stopwatch();
    stopWatch.Start();

    // Any() needs using System.Linq
    if ( Directory.EnumerateFileSystemEntries(args[0], "*.tif").Any() == false )
    {
        Console.WriteLine("No TIF files found.");
        Environment.Exit(-1);
    }

    string[] directory = Directory.GetFiles(args[0], "*.tif");

    // gets a directory name even when a relative path is given
    // http://stackoverflow.com/a/674495/2657875
    string filename = new FileInfo(directory[0]).Directory.FullName + ".pdf";

    // returns the pathname where the currently executing assembly resides
    MagickNET.Initialize(Path.GetDirectoryName
        (Assembly.GetExecutingAssembly().Location) + @"\MagicSettings");
    Console.WriteLine(Path.GetDirectoryName(Assembly.GetExecutingAssembly().Location));
    using (MagickImageCollection collection = new MagickImageCollection())
    {
        foreach (string file in directory)
        {
            MagickImage image = new MagickImage(file);
            image.Quality = 100;
            image.Sharpen(0, 3);
            image.CompressionMethod = CompressionMethod.Group4;
            collection.Add(image);
        }

        try
        {
            collection.Write(filename);
        }
        catch (Exception ex)
        {
            Console.WriteLine(ex.Message);
        }

        // Get the elapsed time as a TimeSpan value.
        TimeSpan ts = stopWatch.Elapsed;
        // Format and display the TimeSpan value.
        string elapsedTime = String.Format("{0:00}:{1:00}:{2:00}",
            ts.Hours, ts.Minutes, ts.Seconds);
        Console.WriteLine("Done in " + elapsedTime);
    }
}

I've got a couple quick notes. Overall it looks like this method will work.

Validating args[0]

The only validation that you do on args[0] is to check if there are any images in that directory, but you don't check other possible failure conditions, such as if the directory provided doesn't exist.  For a user, no images being found and the directory itself not being found would have different actions required to fix the problem.  It would be helpful to perform better validation here and providing meaningful feedback, and perhaps a different return code.

Duplicate Calls to Get Path

There are a couple instances that call Path.GetDirectoryName(Assembly.GetExecutingAssembly().Location).  This could be factored out to a variable.

Use TimeSpan Formatting

TimeSpan provides functionality for converting to a string in a particular format (see https://msdn.microsoft.com/en-us/library/ee372286(v=vs.110).aspx)  In this case, I think you'd want the "g" format.

Return Codes

You return -1 in the event of no files being found, but you don't handle returning a different value if collection.Write(filename) fails.  If this was being called from a script, I'd expect any failure to return a failed return code.

Catching Exceptions

While the exceptions thrown by collection.Write(filename) are likely all IO related, it would be helpful to catch at least some of the individual errors and displaying helpful information where possible.  For example, if an exception was thrown because of a write permission issue, it would be helpful to let the user know that they need to correct their permissions, as opposed to hoping that the library you use provides a helpful description.

There are also some instances that I bet could throw an exception that you don't handle.  For example, I would expect collection.Add(image) to throw an exception in some circumstances.  If some exceptions are handles and displayed in a user-friendly way, I'd expect all of them to.

Commenting

Some of the comments are helpful, such as the reference to documentation for a method from StackOverflow.  However, there are also some trivial comments (stopWatch.Elapsed is pretty clearly going to get the elapsed time). But there are also instances where comments would be helpful, such as what the values you set on the MagickImage are (what does image.Sharpen(0, 3) do?)


Handing focusing and unfocusing of multiple form fields Here is a link to my problem

$('#name').focus(function() {
    if ($(this).val() == 'Full Name*')
        $(this).val('');
        }).blur(function() {
    if($(this).val() == '')
        $(this).val('Full Name*');
});
    $('#email').focus(function() {
    if ($(this).val() == 'Email Address*')
        $(this).val('');
        }).blur(function() {
    if($(this).val() == '')
        $(this).val('Email Address*');
});
    $('#subject').focus(function() {
    if ($(this).val() == 'Subject*')
        $(this).val('');
        }).blur(function() {
    if($(this).val() == '')
        $(this).val('Subject*');
});
    $('#msg').focus(function() {
    if ($(this).val() == 'Your Message*')
        $(this).val('');
        }).blur(function() {
    if($(this).val() == '')
        $(this).val('Your Message*');
});

I've been trying to simplify this code, but because of my limited javascript knowledge I haven't been able to achieve this. I'm almost 100% sure that this could be written in much simpler - DRY form.

Use a custom date attribue, lets say placeholder on your inputs, and then give them a common class as well:

<input type="text" id="email" data-placeholder="Email Address" class="required" />
<input type="text" id="name" data-placeholder="Full Name" class="required" />
<input type="text" id="subject" data-placeholder="Subject" class="required" />
<input type="text" id="msg" data-placeholder="Your Message" class="required" />

And then use 1 event for all!

$(".required").focus(function() {
    var placeholder = $(this).data("placeholder");
    if (this.value == placeholder) this.value = "";
}).blur(function() {
    if (this.value == "") this.value = $(this).data("placeholder");
}).blur(); //trigger on load

A fiddle: http://jsfiddle.net/odpu5gbn/


Classify values depending on predicates I've to do a generic method Classify<T> that, given a sequence of elements of type T and an arbitrary number of predicates, returns an array of n+1 lists (where n is the number of the predicates). More precisely, if the element satisfies the first predicate then it is added to the first list. If not, then if the element satisfies the second predicate it is added to the second list, etc. If none of the predicates can be applied to one element, then that element is added to the last list (the n+1 th).

I managed to do that with the following code. Is there any way to do it better, maybe with Linq?

public List<T>[] Classify<T>(IEnumerable<T> sequence, params Predicate<T>[] predicates)
{
    var result = new List<T>[predicates.Length+1];
    var counter = 0;
    foreach (var elem in sequence)
    {
        var valid = false;
        foreach (var predicate in predicates)
        {
            if(result[counter]==null)
                result[counter] = new List<T>();
            if (predicate(elem) && valid==false)
            {
                var partition = result[counter];
                partition.Add(elem);
                valid = true;
            }
            counter++;
        }
        if (!valid)
        {
            if (result[counter] == null)
                result[counter] = new List<T>();
            result[counter].Add(elem);
        }
        counter=0;
    }
    return result;
}

public List<T>[] Classify<T>(IEnumerable<T> sequence, params Predicate<T>[] predicates)
{
    var result = new List<T>[predicates.Length + 1];
    // It's clearer to create elements of the array explicitly
    for (int i = 0; i < result.Length; i++)
        result[i] = new List<T>();

    foreach (var elem in sequence)
    {
        var index = predicates.TakeWhile(p => !p(elem))
                              .Count();

        result[index].Add(elem);
    }
}

There is a few LINQ methods to get an index, f.e. Select((i, e) => ...). If the list of unsatisfied elements was the first (0th), I would use Select. But since it is the last, TakeWhile gives the shorter code.


Segregate an array of 0s and 1s I think that my code contains a lot of if and else statements that are probably not required.I suppose that this code could be condensed to a shorter form where the logic applied would look clearer than it does now.
Suggestions are welcome and any help in refactoring would be highly appreciated.

public class Segregate{
    public static void doSegregation(int[] a){
        int i = 0;
        int j = a.length-1;

      while(i<=j){
        if(a[i]==a[j]){
          if(a[i]==1 && a[j]==1){
            j--;
          }
        else if(a[i]==0 && a[j]==0){
            i++;
        }
    }
    else{
        if(a[i]>a[j]){
            int temp = a[i];
            a[i] = a[j];
            a[j] = temp;
            i++;
            j--;
        }
        else if(a[i]<a[j]){
            i++;
            }
         }
      }
   }

   public static void main(String[] args){
      int[] a = {1,0,1,0,1,1,0,1,1,1};
      System.out.println("Array before segragation : ");
      for(int i=0; i<a.length; i++){
         System.out.print(a[i]+" ");
      }
      System.out.println();
      doSegregation(a);
      System.out.println("Array after segragation : ");
      for(int i=0; i<a.length; i++){
        System.out.print(a[i]+" ");
       }
    }
 }

With the premise that you're not allowed to use Arrays.sort It'd be more succinct to write your own sorting method.

All you really need to do is initialize an array of the size, count the 1s and set that many positions from the end to 1.

This takes advantage of the fact that integers in java are initialized to 0; the resulting solution becomes straightforward.

Try it yourself, then look at this implementation:

public class Segregate {
  public static void main(String[] args) {
    int[] a = {1, 0, 1, 0, 1, 1, 0, 1, 1, 1};
    System.out.println("Before segregation: " + join(a, ", "));
    System.out.println("After  segregation: " + join(segregate(a), ", "));
  }

  private static int[] segregate(int[] onesAndZeros) {
    int[] segregated = new int[onesAndZeros.length];
    int end = segregated.length - 1;
    for (int count = countOnes(onesAndZeros); count > 0; count--) {
      segregated[end--] = 1;
    }
    return segregated;
  }

  // Obviously only works in this specific case.
  private static int countOnes(int[] onesAndZeros) {
    int count = 0;
    for (int n : onesAndZeros) {
      count += n;
    }
    return count;
  }

  private static String join(int[] nums, String delim) {
    StringBuilder joined = new StringBuilder();
    for (int num : nums) {
      joined.append(delim).append(num);
    }
    return joined.substring(delim.length());
  }
}

Output:


Angular/bootstrap radio button directive I found creating radio buttons in my application a bit of a hassle in terms of having to configure the HTML to do what I wanted when the user clicks/unclicks the items etc.

I figured reducing it to a directive was the best way to simplify things.

The functionality I want is:

Be able to pass in a list of items that are displayed as radio button options
Be able to specify which item is checked by default
Can be displayed as a horizontal or vertical list
Have the option to make the buttons unselectable (i.e. clicking them again will uncheck the item)
The controller knows which value is checked.
The controller executes a function when the checked value changes
The controller can change which item is checked

Here is a working plunker.

Here is an example usage of the directive:

<dwj-radio-group
    dwj-items = "items"
    dwj-changefn = "changeFn"
    dwj-unselectable = "true"
    dwj-align = "vertical"
    dwj-boundvalue = "foo"
    >
</dwj-radio-group>

Here is the directive code:

var radioDirective = app.directive('dwjRadioGroup', function($compile, $templateCache ) {

    var getTemplate = function(item){
        var template = 'ERROR';
        console.log(item);
        switch (item){
        case 'horizontal':
            template = $templateCache.get("radio-group-horizontal.html");
            break;
        case 'vertical':
            template = $templateCache.get("radio-group-vertical.html");
            break;
        }

        return template;
    };

    return {
        scope : {
            items : '=dwjItems',
            changeFn : '=dwjChangefn',
            selectedItem: "=dwjBoundvalue",
            unselectable: "=dwjUnselectable",
            align:          "=dwjAlign"

        },

        link : function(scope, element, attrs) {

            scope.unselectable =scope.unselectable === 'true';                          //default false
            scope.align = scope.align === "vertical"? "vertical": "horizontal";         //default horizontal

            //http://onehungrymind.com/angularjs-dynamic-templates/
            var content = getTemplate(scope.align);
            console.log(content);
            element.html(content);
            $compile(element.contents())(scope);

            scope.changeItem = function(x){
                if (scope.unselectable && x === scope.selectedItem){
                    scope.selectedItem = null;
                }
                else{
                    scope.selectedItem = x;
                }

                scope.changeFn(scope.selectedItem);
            };
        }
    };
});

Currently, from what I can see, the directive does everything I want it to.

I'm looking for feedback on my use of the directive:

Are my naming conventions correct?
Am I using the directive scope binding correctly?
Is using $templateCache and $compile a good way to do this, or is there a much simpler way to do it?
Is there anything completely broken that I'm missing?

items : '=dwjItems',
changeFn : '=dwjChangefn',
selectedItem: "=dwjBoundvalue",
unselectable: "=dwjUnselectable",
align:          "=dwjAlign"

Your indentation needs some work.

scope : {
    items : '=dwjItems',
    changeFn : '=dwjChangefn',
    selectedItem: "=dwjBoundvalue",
    unselectable: "=dwjUnselectable",
    align:          "=dwjAlign"
},

Not sure why you need to prefix "dwj" to your directive attributes. It's not like it will collide with anything else. Besides, you're using the directive as an element. No potential DOM attribute name collisions.

scope.unselectable =scope.unselectable === 'true';                          //default false
scope.align = scope.align === "vertical"? "vertical": "horizontal";         //default horizontal

//http://onehungrymind.com/angularjs-dynamic-templates/
var content = getTemplate(scope.align);
console.log(content);
element.html(content);
$compile(element.contents())(scope);

Not entirely sure why you need to dynamically compile the template. You can simply just define both layouts in the same template and hide one of them when the other is selected. You can use ng-if for that. A developer can come in and say "Oh, two layouts for a directive" than "Oh, I have to take care of 2 template files."


Loops for removing unnecessary lines I have the following method:

private void removeUnnecessaryLines(List<ScatterViewItem> list)
{
    List<Line> remove = new List<Line>();
    foreach (Line line in lines)
    {
        SourceFile destination = (line.Tag as Call).getCallee();

        foreach (ScatterViewItem svi in list)
        {
            SourceFile test = svi.Tag as SourceFile;
            if (test.Equals(destination))
            {
                remove.Add(line);
            }
        }
    }

    foreach (Line l in remove)
    {
        lines.Remove(l);
        Dependencies.Children.Remove(l);
    }
}

As you can see, there is a lot of iteration and casting. Is there a simple way to improve that?

The most obvious improvement would be to use LINQ's Where method to filter the lines list instead of building up a list of to-be-deleted items and then deleting them. In addition to being easier to read and understand it will have the benefit of its runtime not being quadratic in the length of lines.

You can also use the List<T>.RemoveAll method which also takes a predicate like Where (though the predicate specifies the items to be removed, so it's in a way the opposite of Where), but modifies the list in-place, like your solution did.


Making an alphabatical list grouped by letters I have a pet project (open source on GitHub) that I built as a learning exercise. It is a lyrics web application built in .NET Core MVC.
On the artists page of the site (view on mobile size viewport, it is only optimised for mobiles so far), I list all artists alphabetically. The artists are grouped by letter.
The code that builds that page is the following:
Controller
[Route("artists")]
public async Task<IActionResult> Index()
{
  IDictionary<char, List<LibraryArtistViewModel>> viewModel = await _artistsService
    .GetAllArtistsAsync();

  return View(viewModel);
}

Service
public async Task<IDictionary<char, List<LibraryArtistViewModel>>> GetAllArtistsAsync()
{
  List<LibraryArtistViewModel> artists = new List<LibraryArtistViewModel>();

  await using NpgsqlConnection connection = new NpgsqlConnection(databaseOptions.ConnectionString);
  await connection.OpenAsync();

  await using NpgsqlCommand command = new NpgsqlCommand("select a.first_name, a.last_name, \"as\".name as primary_slug, ai.data as image_data, count(l.title) as number_of_lyrics from artists a left join artist_images ai on ai.artist_id = a.id inner join artist_slugs \"as\" on \"as\".artist_id = a.id left join lyrics l on l.artist_id = a.id where a.is_approved = true and a.is_deleted = false and \"as\".is_primary = true and l.is_approved = true and l.is_deleted = false group by a.id, \"as\".name, ai.data order by a.first_name asc;", connection);

  await using NpgsqlDataReader reader = await command.ExecuteReaderAsync();

  while (await reader.ReadAsync())
  {
    LibraryArtistViewModel artist = new LibraryArtistViewModel();
    string firstName = Convert.ToString(reader[0]);
    string lastName = Convert.ToString(reader[1]);
    string fullName = textInfo.ToTitleCase($"{firstName} {lastName}");
    string primarySlug = Convert.ToString(reader[2]);
    bool hasImage = reader[3] != System.DBNull.Value;
    int numberOfLyrics = Convert.ToInt32(reader[4]);

    artist.FirstName = firstName;
    artist.LastName = lastName;
    artist.FullName = fullName;
    artist.PrimarySlug = primarySlug;
    artist.HasImage = hasImage;
    artist.NumberOfLyrics = numberOfLyrics;

    artists.Add(artist);
  }

  IDictionary<char, List<LibraryArtistViewModel>> dictionary = BuildDictionary(artists);

  return dictionary;
}

private IDictionary<char, List<LibraryArtistViewModel>> BuildDictionary(List<LibraryArtistViewModel> artists)
{
  List<char> letters = new List<char>();

  IDictionary<char, List<LibraryArtistViewModel>> dictionary =
    new Dictionary<char, List<LibraryArtistViewModel>>();

  foreach (LibraryArtistViewModel artist in artists)
  {
    char firstLetter = char.ToUpper(artist.FirstName[0]);

    if (!letters.Contains(firstLetter))
    {
      letters.Add(firstLetter);

      dictionary.Add(firstLetter, new List<LibraryArtistViewModel>());
    }
  }

  foreach (char letter in letters)
  {
    foreach (LibraryArtistViewModel artist in artists)
    {
      char firstLetter = char.ToUpper(artist.FirstName[0]);

      if (letter == firstLetter)
      {
        List<LibraryArtistViewModel> artistsBeginningWithTheLetter = dictionary[letter];
        artistsBeginningWithTheLetter.Add(artist);
      }
    }
  }

  return dictionary;
}

The code above works, but I feel like it isn't efficient. I feel like there's a better way of doing this.
Also, I am very unsure about the way I serve up images. My images are currently stored in the database as bytes and I have a controller that serves up the image. This seems to be really killing my performance metrics on https://web.dev.
I built this project to learn things. So I'd really love to learn how to do things "properly" even if they are deemed an overkill.
I'd appreciate some pointers on the above and also on how exactly I can improve the artist images situation.

Well, you have a good learning exercise project. here is some notes on the artiest code you've provided :

You're using string query, which opens SQL Injections. To overcome that, you'll need first to mark your query as const or readonly and end the query with a semicolon (which you did), and ensure that the query itself not having any SQL bugs that might be used in inappropriate way. However, if you want the proper way to manager your database, you can use Object-relational Mapping AKA ORM such as Entity Framework.
Store images in a psychical disk and store their path (or filename) into the database. Storing the file bytes into the database would impact the system performance as you'll need to read and render the images every time you select them. While if you store the files physically, it would be faster to render. So, create a folder inside the project folder, store the images in that folder (in any structure you want) then store the path of each image in the database. Then, in your application, you just get the path, adjust the path to be viewable to the user. For example, if the stored path images/artists/someartist.png then you just add the current web url to it like https://www.somewebsite.com/images/artists/someartist.png.
Always use PNG images for web, as png images are more balanced between the quality and size, and would be better option than other types for the web.

In your code, sorting operations should not be done on the presentation layer, in fact, it would be more beneficial if it's moved to early application stages like sorting artists in your case, your application has a core feature to filter artists names in alphabetical order. This means, this functionality needs to be applied to the data before it's even inserted to the database, because it's used as a core feature and not an addon functionality.
There are different ways to do it though, one way is to add a property stores the first name letter like this :
public class LibraryArtistViewModel
{
    public char FirstLetter => FirstName?.Length > 0 ? FirstName.ToUpper()[0] : char.MinValue;

    public string FirstName { get; set; }

    public string LastName { get; set; }

    public string FullName { get; set; }

    public string PrimarySlug { get; set; }

    public bool HasImage { get; set; }

    public int NumberOfLyrics { get; set; }

}

Then, you would do this in GetAllArtistsAsync():
  while (await reader.ReadAsync())

      {
        var artist = new LibraryArtistViewModel
        {
            FirstName = reader[0]?.ToString(),
            LastName = reader[1]?.ToString(),
            FullName = textInfo.ToTitleCase($"{reader[0]?.ToString()} {reader[1]?.ToString()}"),
            PrimarySlug = reader[2]?.ToString(),
            HasImage = reader[3] != System.DBNull.Value;
            NumberOfLyrics = int.TryParse(reader[4]?.ToString(), out int number) ? number : 0;
        };

        artists.Add(artist);
      }

    var dictionary = artists.GroupBy(x=> x.FirstLetter).ToDictionary(x => x.Key, x => x.ToList()).OrderBy(x=> x.Key);

Though this would give you what you want, it would take out some tiny performance. As I stated before, since your artists are supposed to be sorted, you need to sort it from the time you insert the data, either from the database which would be faster, or on CreateNewArtistAsync action. So, in your Get you won't need to re-sort on every request.


Insert sort on a linked list I want to do insert sort in a linked list without using dummy nodes.

This is my code. How can this be improved? Any input is appreciated.

public static Node insertSort(Node node){

   Node sortedList = null;
   while(node != null){
    Node current = node;
    node = node.next;
    Node x;
    Node previous = null;
    for(x = sortedList; x != null; x = x.next){
        if(current.value < x.value){
                break;
         }
         previous = x;
    }
    if(previous == null){
          current.next = sortedList;
          sortedList = current;
    }
    else{
       current.next = previous.next;
       previous.next = current;
    }
  }
  return sortedList;
}

Algorithmically, your implementation seems fine. One improvement would be to initialize the sortedList to the first node right away. This necessitates some error checking up front, which I think is better style anyway.

I would also run a check for the current node being the new sorted head first. That way I can automatically assume that I'm inserting after the search position. That allows elimination of the previous variable.

But this is all stylistic stuff to make the logic flow easier to understand. As I said, I think your implementation functions perfectly well.

public static Node insertNode(Node node) {
    if (node == null)
        return null;

    // Make the first node the start of the sorted list.
    Node sortedList = node;
    node = node.next;
    sortedList.next = null;

    while(node != null) {
        // Advance the nodes.
        final Node current = node;
        node = node.next;

        // Find where current belongs.
        if (current.value < sortedList.value) {
            // Current is new sorted head.
            current.next = sortedList;
            sortedList = current;
        } else {
            // Search list for correct position of current.
            Node search = sortedList;
            while(search.next != null && current.value > search.next.value)
                search = search.next;

            // current goes after search.
            current.next = search.next;
            search.next = current;
        }
    }

    return sortedList;
}

See it run in JavaScript: http://jsfiddle.net/2LDGD/2/


Improving Lottery Simulation I'm working on a project for an Intro to C/Python class and am looking to improve the efficiency of the program. The program is a lottery simulation where the user inputs the number of tickets they want to buy, then generates tickets, and finally outputs the total winnings and net gain (usually loss).

This is my code (in Python, as required):

def main():

    numb_tickets = int(input("How many tickets would you like to buy?\n"))

    #Calculate Winnings
    winnings = 0

    for i in range(numb_tickets):

        #For testing only, gives feedback progress of program
        print(i,"     ",winnings)

        #Creating winning ticket/your ticket, find number of matches
        win_tic = getRandomTicket(MAX_VALUE, TIX_SIZE)
        my_tic = getRandomTicket(MAX_VALUE, TIX_SIZE)
        numb_win = numMatches(win_tic, my_tic)

        #Add appropriate payout for number of matches
        if numb_win == 6:
            winnings += WIN_SIX
        elif numb_win == 5:
            winnings += WIN_FIVE
        elif numb_win == 4:
            winnings += WIN_FOUR
        elif numb_win == 3:
            winnings += WIN_THREE

    #Calculate cost of purchasing tickets
    cost_tics = numb_tickets * COST_TIC

    if winnings >= cost_tics:
        profit = winnings - cost_tics
        print("You won $",winnings,", for a net earnings of $",profit,".", sep="")
    elif winnings < cost_tics:
        loss = cost_tics - winnings
        print("You won $",winnings,", for a net loss of $",loss,".", sep="")

main()

Note: the getRandomTicket() and numMatches() functions were provided by the professor to generate a lottery ticket and check the number of matches it has, respectively.

My program works fine for smaller numbers of tickets, but when testing the required 1,000,000 tickets, takes a massive amount of time. It makes sense that the time increases rapidly as the range of the loop increases, but I don't know of a better way to loop this yet. Any input or suggestions are greatly appreciated.

It's more of a style preference, but instead of the paragraph with if and elif's, I would do

winnings += payouts_dictionary[numb_wins]

where

payouts_dictionary = {6: WIN_SIX, 5: WIN_FIVE, 4: WIN_FOUR, 3: WIN_THREE, 2: 0, 1: 0, 0: 0}

is defined as a constant before the loop.


Number guessing game i'm a beginner and this is the second program that i have written by myself from scratch, i wrote a basic snake game with PYgame but i didn't do it by myself, so anyway, what do you guys think of this? i want honest opinions, no sugar-coating ! does it have any problems? is there any improvements that can be done?

import random
print "Guess my number!"
print " What's your name?"
user_name = raw_input()
print "Hello %s, i'm thinking of a number between 1 and 20." %(user_name)
def gameloop():
    random_number = random.randrange(1,21)
    print "Take a guess."
    vectory_message = "Well done %s, You beat me!" %(user_name)
    defeat_message = "Sorry %s, You lost!" %(user_name)
    attempts = 0
    maximum_attempts = 7
    user_number = int(raw_input())
    attempts += 1
    print "You have %s  lives left." %(maximum_attempts - attempts)
    while True:
        if user_number == random_number:
            print vectory_message
            attempts = 0
            second_chance()

        elif user_number > random_number:
            user_number  = int(raw_input("A bit lower ..."))
            attempts += 1
            if attempts == maximum_attempts:
                print defeat_message
                second_chance()
            print "You have %s  lives left." %(maximum_attempts - attempts)
            continue

        elif user_number < random_number:
            user_number = int(raw_input("Guess higher ..."))
            attempts += 1
            if attempts == maximum_attempts:
                print defeat_message
                second_chance()
            print "You have %s  lives left." %(maximum_attempts - attempts)
            continue

def second_chance():
    while True:
        choice = raw_input("wanna give it another shot?(y/n)")
        if choice == "y":
            gameloop()
        elif choice == "n":
            quit()
        else:
            print "That wasn't a (y) or (n)!, Try again."
            continue
gameloop()

Your code is pretty good, but some of your logic isn't.

The game loop should be structured differently.
Also I'm ignoring second_chance.

There are two main things that should happen after I pick a number:

I'm correct, you say I win and the game ends.
I'm wrong, you tell me if I'm higher or lower.

This in psudocode could be:

while True:
    user-input = get-user-input()
    if user-input == random-number:
        stop game
    else:
        display if user is higher or lower.

This should highlight that it's pretty much Python.
And we could do with another function.

First, stop game should exit out of the game, and tell us if we won or not.
This can simply be return True.

The logic to display if the user is higher or lower then the guess is an if and an else.
If the guess is larger then you want to display "A bit lower ..." otherwise (the else) "Guess higher ...".
To do this simpler I will use a turnery operator.

And so it'd be:

guess_text = "Take a guess."
random_number = random.randrange(1,21)
while True:
    user_number = int(raw_input(guess_text))
    if user_number == random_number:
        return True
    else:
        guess_text = "A bit lower..." if user_number > random_number else "Guess higher..."

Ok so we now have a solid loop, but it's not much of a game.
And the rest of your elif's are pretty good here.
I'd change the attempts = 0 to return false and remove the continue.

This should get you:

guess_text = "Take a guess."
random_number = random.randrange(1,21)
attempts = 0
maximum_attempts = 7
while True:
    user_number = int(raw_input(guess_text))
    if user_number == random_number:
        print "Well done %s, You beat me!" %(user_name)
        return True
    else:
        guess_text = "A bit lower..." if user_number > random_number else "Guess higher..."

        attempts += 1
        if attempts == maximum_attempts:
            print "Sorry %s, You lost!" %(user_name)
            return False
        print "You have %s  lives left." %(maximum_attempts - attempts)

Now you should know that return is only allowed in a function.
This means you need to make a function that gets passed the user_name.
So:

def play_game(user_name):
    guess_text = "Take a guess."
    attempts = 0
    ...

Finally you want to be able to play multiple games!
I'll use second_chance for the basis for this loop.

You should change quit() to break.
Remove the continue.
Put all the prints and definition for user_name at the top (not in the loop).
Rename it as main.

This should get you:

def main():
    print "Guess my number!"
    print " What's your name?"
    user_name = raw_input()
    print "Hello %s, i'm thinking of a number between 1 and 20." %(user_name)
    while True:
        choice = raw_input("wanna give it another shot?(y/n)")
        if choice == "y":
            play_game(user_name)
        elif choice == "n":
            break
        else:
            print "That wasn't a (y) or (n)!, Try again."

Finally I'd recommend using str.format over %.

So as you should guess, your code was pretty good!
Apart from the duplicate questions.


Selectively printing elements on web page using a bookmarklet There is a web page I visit which I would like to print. However, I only want to print a couple of things on it, not everything.
So, I am making a bookmarklet that I can use whenever I visit the page to hide everything from the printer except what I want to print.
The site already has a class, 'd-print-none', to hide an element from the printer.
The following code is the javascript I made to base my bookmarklet on.
My idea is to first apply the 'd-print-none' class to everything. Then selectively remove it.
I remove the class from the element that contains what I would like to print as well as any of its children.
Then I recursively remove it from the parents until I reach the document element.
function removeClass(t_selector, t_class){
    document.querySelectorAll(t_selector + ' *').forEach(x=>x.classList.remove(t_class))

    node = document.querySelector(t_selector)
    while (node && node.nodeType != Node.DOCUMENT_NODE) {
        node.classList.remove.(t_class);
        node = node.parentNode;
    }
}

document.querySelectorAll( '*' ).forEach(x=>x.classList.add('d-print-none'))
removeClass('app-flex-chart', 'd-print-none')
removeClass('app-flex-grid', 'd-print-none')

Adding the class to all elements was pretty straightforward and succinct as was removing it from all of the target element's children.
Climbing the tree to apply it to the element itself and any of its parents was more verbose and error prone. Sadly, there appears to be no css selector to find all parents of an element.
I paired the loop down to be as succinct and readable as I could.
I am looking for ideas on how to make it more robust, succinct, or readable. Open to any and all suggestions and criticisms.
For that while loop, I had to check if the node I was on existed and check to make sure it wasn't the document node, either case would cause an error.
EDIT: To show generic structure of page
<html>
    <head>...</head>
    <body>
        <many>
            <nested>
                <tags>
                    <app-flex-chart>
                        <many-more-nested-tags>Actual Content</many-more-nested-tags>
                    </app-flex-chart>
                </tags>
            </nested>
        </many>
    </body>
</html>

The app-flex-chart tag is the only unique piece along the chain to the content. All other classes and tags seem to be reused throughout the page.

Declare variables before using them - your node variable is never declared. Use let node = instead.
Typo? node.classList.remove.(t_class) should be node.classList.remove(t_class);
Variable casing The vast majority of professional JavaScript uses camelCase for most variables, including parameters. There are a few exceptions (like ALL_CAPS for things like constant magic numbers, and PascalCase for constructors and namespaces).
Variable names It's not entirely clear on the initial encounter of removeClass what the parameters mean. Maybe change:

t_selector -> selectorToShow
t_class -> hiddenClassName

or something similar.
Always use strict equality, avoid sloppy equality to avoid readers of the code having to understand coercion weirdness to be 100% confident of what the code is doing.
Semicolons Either use semicolons when appropriate, or (if you don't like them and you're an expert who can avoid the pitfalls of ASI) don't. To be stylistically consistent, choose one style and stick with it. Consider a linting rule.
Relying on a site's classes is unreliable, especially for sites that are being actively maintained and developed. Many of those who write bookmarks and userscripts are familiar with this issue. The site may change their CSS, resulting in d-print-none doing nothing. Bookmarklets and userscripts are prone to requiring updates when a site changes regardless - but reducing the chance of needing maintenance when a change occurs is good.
Whatever d-print-none does currently, I'd append another <style> tag that inserts your own class (or data attribute) that does the same thing, eg:
const style = document.body.appendChild(document.createElement('style'));
style.textContent = `
@media print
{
    .melted-bookmarklet-print-none, .melted-bookmarklet-print-none *
    {
        display: none !important;
    }
}
`;

Then toggle that class instead (and do style.remove() at the end if you want).
(You might be tempted to iterate through the elements and apply the style directly to elements, but that could break the site's built-in display properties if it happens to have styles already applied directly to elements.)
Or, you could use CSS alone instead of iterating through elements. How exactly to do this depends on where the app-flex-chart and app-flex-grid are located on the page, but it could look something like this, if they're both descendants of the <body>:
const style = document.body.appendChild(document.createElement('style'));
style.textContent = `
@media print
{
    body > *:not(app-flex-chart):not(app-flex-grid),
    body > *:not(app-flex-chart):not(app-flex-grid) * {
        display: none !important;
    }
}
`;

(feel free to engineer to be more flexible and less repetitive if you think it'd be helpful)
This way, all elements which are not the .app-flex-chart nor .app-flex-grid will be hidden, without any intermediate classes or element iteration.
Or save a reference to the two elements you want to print, remove everything, then append the two back:
const elms = ['app-flex-chart', 'app-flex-grid'].map(s => document.querySelector(s));
document.body.textContent = '';
for (const elm of elms) {
  document.body.appendChild(elm);
  // if needed to put them on separate lines:
  document.body.appendChild(document.createElement('div'));
}

That's the version I'd prefer, if feasible. You may have to insert intermediate elements for the page's styles to apply. (If that gets too complicated, use a different approach.) If you want the original page to remain interactable, you could open a new window which gets populated with the content of elms instead. (You'll also have to append <style> and CSS link tags from the original page)
Bookmarklet or userscript? You're currently using this as a bookmarklet. It depends how you're managing them, but if you aren't using a special utility for it, I think userscripts are more maintainable, due to a few factors:

Userscript editor interfaces are meant to be usable as code editors. In contrast, a bookmarklet, when installed, isn't very user-friendly to edit:

Userscripts can be saved in separate external files, rather than existing in the browser's internal ether.

Userscripts are more flexible in when they're run. Most importantly, they can run automatically without requiring a click. You may find this helpful.

If you want to try this out as a userscript, try Tampermonkey.
Depending on how you're using the printed page and how often you're printing, you can consider using Puppeteer in Node to scrape and print the elements completely automatically.


Making a job completed, rolling back on error I have the following construction in the program:

  while(true) {
    <...>
    try {
        JobManager.markJobCompleted(unitOfWork.getSqlFactory(), jobId, dataOut);
    } catch (DbLogicException e) {
        logger.error(JobManager.CANNOT_MARK_JOB_COMPLETED + jobId, e);
        try {
            unitOfWork.rollback();
        } catch (DbLogicException e1) {
            logger.error(UnitOfWork.CANNOT_ROLLBACK_TRANSACTION, e1);
            unitOfWork.closeSilently();
            return;
        }
        continue;
    }
    <...>
  }

So, the logic is the following:

Trying to mark the job completed
If we fail, try to rollback transaction and proceed to next job item
If transaction rollback failed, something is seriously wrong here, aborting everything

This construction looks a little clumsy to me. Is there a way to refactor it so it would become... beautiful?

Yes.

Throw specialized exceptions like CannotMarkJobCompletedException and CannotRollbackTransactionException...
Make your toString() method on exceptions to return something you want what your logger to log
Instead of doing: ...CAN_..._COMPLETED + jobId, you may do: thrown exception already knows this jobId
Store the thrown exception in a variable and call your logger once

By doing all of that you will solve the problem of having nested try-catch blocks.


Getting a list of outdated versions with Python I have a list of boxes and need to list only the outdated ones.

For example, input will look something like this:

maier/alpine-3.1.3-x86_64 (virtualbox, 1.2.0)
maier/alpine-3.1.3-x86_64 (virtualbox, 1.3.0)
ubuntu/precise64          (virtualbox, 20151202.0.0)
ubuntu/vivid64            (virtualbox, 20150821.0.0)
ubuntu/vivid64            (virtualbox, 20150903.0.0)
ubuntu/vivid64            (virtualbox, 20151113.0.0)
ubuntu/vivid64            (virtualbox, 20151127.0.0)
ubuntu/vivid64            (virtualbox, 20151211.0.0)
ubuntu/wily64             (virtualbox, 20151203.0.0)

Output will look roughly like this:

maier/alpine-3.1.3-x86_64 (virtualbox, 1.2.0)
ubuntu/vivid64            (virtualbox, 20150821.0.0)
ubuntu/vivid64            (virtualbox, 20150903.0.0)
ubuntu/vivid64            (virtualbox, 20151113.0.0)
ubuntu/vivid64            (virtualbox, 20151127.0.0)

Note the format consists of: name, provider, version

It seems like there would be an easy way to do this from Bash scripting alone, e.g. I can version sort a particular column with sort, but I wasn't quite able to figure it out so I roughed it out in Python instead.

Is there are better way to do this?

#!/usr/bin/python3
# -*- coding: utf-8 -*-

"""Attempt to remove outdated Vagrant boxes"""

import os
import re
import subprocess

from distutils.version import StrictVersion

def search(text):
    """Return a dictionary containing all the named subgroups of the match."""

    patt = r'^(?P<name>[^ ]+) +\((?P<provider>[^ ]+), +(?P<version>[^ ]+)\)$'
    return re.search(patt, text).groupdict()

output = subprocess.check_output(['vagrant', 'box', 'list'],
                                 stderr=subprocess.DEVNULL).decode().rstrip()

box_list = (search(line) for line in output.splitlines())

box_dict = {}

for box in box_list:
    name = box.get('name')
    version = box.get('version')
    box_dict[name] = box_dict.get(name, []) + [version]

outdated_boxes = {}

for box in box_dict:
    versions = box_dict[box]
    # remove the most recent version
    if len(versions) > 1:
        outdated_boxes[box] = sorted(versions, key=StrictVersion)[:-1]

for box in outdated_boxes:
    versions = outdated_boxes[box]
    for version in versions:
        command = ['vagrant', 'box', 'remove', box, '--box-version', version]
        print(*command)
        os.system(' '.join(command))
        # process = subprocess.Popen(command)
        # process.wait()
        print()

Since everything is printed in order, the "obsolete" ones are all but the last version for the first word. That's tricky. But if we looked at the text backwards, we're printing all but the first one.

And that's easy with awk:

awk '!($1 in seen){print }{seen[$1] = 1}'

And then just invert the input twice:

run_your_version_cmd_here | tac | awk '$1 not in seen{print }{seen[$1] = 1}' | tac


JavaScript validation form Could this code be improved? Would you suggest more sophisticated way of doing client-side signup validation?

<!DOCTYPE html>

SignUp Form

<form name="registrationForm" onsubmit="return validateForm()" action="registration" method="post">
    Email:<input type="text" name="username"/>

    Password:<input type="password" name="password"/>

    Repeat Password:<input type="password" name="passwordAgain"/>

    <input type="submit" value="signup"/>
</form>

An alternative is to simply use what HTML has built-in:

<input type="password" required minlength="8">

will make that input mandatory and enforce a minimum of eight characters. (You can also use these attributes with type="text".) It's no less secure than doing the validation in JavaScript, as you should still validate it server-side anyway. However, it isn't possible to check using HTML attributes that the two password fields are the same.

The :optional and :required CSS pseudo-classes can be used for styling the input based on the required attribute, but you can also use the more general :valid and :invalid pseudo-classes, which will also respond to things like minlength. You can check these pseudo-classes in JavaScript using Element#matches, but you might have to also check for non-standard naming as ${prefix}MatchesSelector.

The only problem with this approach is the browser compatibility: minlength is not supported except by Chrome and Opera, but you can replace it with the pattern attribute which has wider support, along with the required attribute. Except for Safari and IE <10.

More information about <input> is available at MDN and WebPlatform Docs.


Basic volume issue representation I have the following struct:

public struct VolumeIssue
{
    public readonly string Volume;

    public readonly string Issue;

    public VolumeIssue(string volume, string issue)
    {
        this.Volume = volume;
        this.Issue = issue;
    }

    public bool IsNullOrWhiteSpace()
    {
        return string.IsNullOrWhiteSpace(this.Volume) && string.IsNullOrWhiteSpace(this.Issue);
    }

    public override string ToString()
    {
        StringBuilder stringRep = new StringBuilder("");

        if (!string.IsNullOrWhiteSpace(this.Volume))
        {
            stringRep.AppendFormat("v. {0}", this.Volume);
            if (!string.IsNullOrWhiteSpace(this.Issue))
            {
                stringRep.AppendFormat(" iss. {0}", this.Issue);
            }
        }

        return stringRep.ToString();
    }
}

Overall I am happy with it. But being still a relatively inexperienced developer, I would like some input on the design of this struct. Ideas on how to flesh it out are also greatly appreciated.

In order to add more context, here is one of the use cases:

public class IssueRangeCoverage
{
    public VolumeIssue Start { get; set; }

    public VolumeIssue End { get; set; }

    public override string ToString()
    {
        if (this.Start.IsNullOrWhiteSpace() && this.End.IsNullOrWhiteSpace())
        {
            return "";
        }

        string endString = this.End.ToString();

        if (string.IsNullOrWhiteSpace(endString))
        {
            endString = "Current";
        }
        return this.Start.ToString() + " - " + endString;
    }
}

Figured I would raise a few points:

Having read your responses/replies, we can gather you have the following specs (roughly):

Small memory footprint
Optimization

Before I go over those two points, Let's first step through the code:

I feel the name VolumeIssue is not very descriptive. In fact, it seems odd to have the struct name be the combination of both of its fields.

Next, I would keep the readonly members but make them private, also drop the uppercase. Instead, use publicly accessible properties in order to access these fields as follows:

private readonly string volume;
public string Volume { get { return this.volume; } }

At this stage, you are most likey asking "why?" and probably for good reason. I mean, you have the following case covered:

Immutability - You've exposed the fields but they are readonly

Why would you bother adding more code when the current solution gives you the same solution for less...or does it?

The code may be functionally equivalent, but we there are more things to be considered. The struct is public. This means that you are allowing other developers to interact/use/inspect and interface to your value type.

Jon Skeet states the following:

  For every type you write, you should consider its interface to the
  rest of the world

You see, by exposing your fields, you are exposing the implementation of your struct, and I would recommend against this.

Jon goes on to say:

  A property communicates the idea of "I will make a value available to
  you, or accept a value from you." It's not an implementation concept,
  it's an interface concept.

Currently, you are violating the design principle of hiding the implementation and for this reason I would also argue in favouring the property/field combo. I've linked the article quotes at the bottom, it's a good read and should give you some interesting points to consider.

Having looked at you use of the struct, it is important to note that you only use the fields within the struct itself. Now, as these fields will only be given once (during the constructor), I would build the contents of the ToString method there and then as @dreza pointed out.

Back to your use case

I would say you are right in choosing to use a struct. Your struct only contains two fields and is embedded inside another object. See more detail here.

Briefly back to the first two points:

Memory footprint

Yup, you've got this covered.

Optimisation

One key thing that should be pointed out is that your value type should implement IEquatable. I would always do this.
This way, you avoid the cost of boxing/unboxing your value type upon any equivalence comparison.


Calculating how long sales reps are idle I wrote a procedure to calculate the number of months elapsed since a sales agent' last sale. It works but runs in over 10 minutes... for 20 rows.

Sub Latency()
' this sub-routine is used to calculate the no of recent idle months by an agent
Application.ScreenUpdating = False
For Each rng In Range("BL1:BL13000")
j = 1
Do Until ActiveCell.Offset(0, -23 - j) > 0    ' this reads the sales flag
j = j + 1
If j > 12 Then Exit Do
Loop
ActiveCell.Value = (j - 1)   ' inserts the value to the cell
ActiveCell.Offset(1, 0).Select
Next rng
End Sub

First, let's give this some proper indentation so we can see what's going on.

Sub Latency()
    ' this sub-routine is used to calculate the no of recent idle months by an agent
    Application.ScreenUpdating = False
    For Each rng In Range("BL1:BL13000")
        j = 1
        Do Until ActiveCell.Offset(0, -23 - j) > 0    ' this reads the sales flag
            j = j + 1
            If j > 12 Then Exit Do
        Loop
        ActiveCell.Value = (j - 1)   ' inserts the value to the cell
        ActiveCell.Offset(1, 0).Select
    Next rng
End Sub

Do you see it now? Freeman was absolutely right. You're not looping over 20 rows. You're looping over 13000. Why such a large range? Ohhh I know. You want to make sure you're getting all of the data, right? Okay, but what if the data range extends past row 13k? You don't just have a performance problem, you also have a correctness problem. Find the last row instead.

Let's see what else we've got while we're here though.

Sub Latency()

The default scope for VBA procedures is Public. So, you should ask yourself if this is the right thing here. I don't have enough context to say, but you should really be explicit whenever possible anyway. Don't rely on another developer's knowledge. Ever. Be explicit.

' this sub-routine is used to calculate the no of recent idle months by an agent

Cool! I like seeing short comments like this explaining what the purpose of the procedure is. It's a good comment. The problem is though, I can't figure out how the Sub's name (Latency) has anything to do with this. Try to come up with a better name for it.

Application.ScreenUpdating = False

Cool! Good idea! Make it look magical when you turn the screen updating back on! But wait.... you never turn it back on. That's bad. Real bad. If you're going to use this, you need to not only turn it back on at the end of the procedure, but you'll also need to make sure it always gets turned back on with an error handler.

For Each rng In Range("BL1:BL13000")

We talked about this already. Find the last row instead.

j = 1

Why are we starting with j? If you're going to use a generic counter variable, start at i and work your way up through j and k. It's just an expected convention.

What I'm actually worried about here is that you've not declared the variable. That means two things.

You've not declared Option Explicit.
This is implicitly a Variant type.

I cannot impress this on you enough. Use Option Explicit. It turns run time bugs into compile time bugs by forcing you to define all of your variables up front.

Why is Variant bad? It's bad because a variant can be anything. Yes. Anything. I could do this and VBA wouldn't blink twice.

j = 1
j = ActiveSheet

Seriously, yes. You can do this if you're using variant types. So don't.

Do Until ActiveCell.Offset(0, -23 - j) > 0    ' this reads the sales flag
    j = j + 1
    If j > 12 Then Exit Do
Loop

What are all these numbers??? I have no idea what this means at all. Use some well named constants instead.
Great comment again! However, instead of using a comment, create a function that returns a boolean value.
Avoid Activate and Select.


Quick Sort Implementation I have written the below code for quick sort in Java:

void quicksort (int[] a, int lo, int hi)
    {
 //  lo is the lower index, hi is the upper index
 //  of the region of array a that is to be sorted
 int i=lo, j=hi, h;

 // comparison element x
 int x=a[(lo+hi)/2];

 //  partition
 do
 {
     while (a[i]<x) i++;
    while (a[j]>x) j--;
    if (i<=j)
    {
        h=a[i];
        a[i]=a[j];
        a[j]=h;
        i++;
        j--;
    }
 } while (i<=j);

 //  recursion
  if (lo<j) quicksort(a, lo, j);
 if (i<hi) quicksort(a, i, hi);
}

Please review and possibly offer a better solution.

Some small notes:

Instead of commenting here:

void quicksort (int[] a, int lo, int hi) {
    //  lo is the lower index, hi is the upper index
    //  of the region of array a that is to be sorted
    int i=lo, j=hi, h;

rename the variables:

void quicksort (final int[] a, final int lowerIndex, final int upperIndex)

It's easier to read.

(Check Clean Code by Robert C. Martin, page 53-54 also.)
Try to minimize the scope of local variables. It's not necessary to declare them at the beginning of the method.

(Effective Java, Second Edition, Item 45: Minimize the scope of local variables. (Google for "minimize the scope of local variables", it's on Google Books too.))
This:

h=a[i];
a[i]=a[j];
a[j]=h;

can be extracted out to a swap method:

public void swap(final int[] arr, final int pos1, final int pos2) {
    final int temp = arr[pos1];
    arr[pos1] = arr[pos2];
    arr[pos2] = temp;
}

Maybe you should provide an easier to use helper method too for the clients:

public void quicksort(final int[] data) {
    if (data == null) {
        throw new NullPointerException("data cannot be null");
    }
    if (data.length == 0) {
        return;
    }
    quicksort(data, 0, data.length - 1);
}

Note the input validation.


Digital Clock in Java I'm just looking for some feedback:

import javax.swing.JFrame;
import java.awt.Color;
import javax.swing.JComboBox;
import javax.swing.JPanel;

public class Clock extends JFrame {

    public Clock () {
        setContentPane(new ScreenSaver());
    }

    public static void main(String[] args) {

        Clock clock = new Clock();

        clock.setSize(685, 80);
        clock.setUndecorated(true);
        clock.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
        clock.setBackground(Color.BLACK);
        clock.setResizable(false);
        clock.setVisible(true);
        clock.toFront();

    }
}

Second class:

public class ScreenSaver extends JPanel implements ActionListener {

    public String stringDate;
    public JComboBox colorList;
    public Color userColor = Color.RED;
    public JPanel panel = new JPanel();
    public JButton button = new JButton("Alarm Clock");
    public static String time = "";
    public static Date date;
    public static DateFormat df;

    public ScreenSaver () {
        date();
        setLayout(new BorderLayout());
        Timer timer = new Timer(1000, this);
        timer.start();

        panel.setBackground(new Color(0,0,0,0));
        panel.setLayout(new BorderLayout());

        add(panel, BorderLayout.PAGE_START);

        String [] colorName = {"Red", "Blue", "Yellow", "Green", "Pink", "Grey"};
        colorList = new JComboBox(colorName);
        colorList.setBackground(Color.BLACK);
        panel.add(colorList, BorderLayout.LINE_START);

    }

    public void date() {
        df = new SimpleDateFormat("EEE,MMM d yyyy - h:mm:ss a");
        date = new Date();
        stringDate = df.format(date);

    }

    public void actionPerformed(ActionEvent e) {
        if (colorList.getSelectedItem().equals("Red"))
        userColor = Color.RED;
        else if (colorList.getSelectedItem().equals("Blue"))
        userColor = Color.BLUE;
        else if (colorList.getSelectedItem().equals("Yellow"))
        userColor = Color.YELLOW;
        else if (colorList.getSelectedItem().equals("Green"))
        userColor = new Color(5, 200, 51);
        else if (colorList.getSelectedItem().equals("Pink"))
        userColor = new Color(249, 100, 178);
        else if (colorList.getSelectedItem().equals("Grey"))
        userColor = Color.GRAY;

        colorList.setForeground(userColor);

    }

    public void paintComponent(Graphics g) {
       date();
       super.paintComponent(g);
       g.setColor(Color.BLACK);
       g.fillRect(0, 0, 685, 100);

        Font font = null;
        try {
            font = Font.createFont(Font.TRUETYPE_FONT, getClass().getResource("/DIGITALDREAMFAT.ttf").openStream());
        } catch(FontFormatException e) {
            e.printStackTrace();
        } catch(IOException e) {
            e.printStackTrace();
        }

        GraphicsEnvironment gen = GraphicsEnvironment.getLocalGraphicsEnvironment();
        gen.registerFont(font);
        font = font.deriveFont(30f);
        g.setFont(font);

       g.setColor(userColor);
       g.drawString(stringDate, 10, 60);
       repaint();

    }
}

CPU Load Bug: Wrong Usage of Timer and repaint()

repaint() calls paintComponent(). The way your code is written, your Java program is repainting all the time, consuming a CPU core completely. Instead it should repaint only when a change happens, and that is if the user changed the color or the timer was triggered. The call to repaint() should not be in paintComponent() but in actionPerformed().

It also means that the call to date() is in the wrong method. While it works being in paintComponent(), it should actually be in actionPerformed() - the timestamp shall be updated when the timer expires, not when the paint happens, because painting might happen much more frequently.

Remove unused symbols

Your fields button and time are not used and their initializations involve no (relevant) side-effects, you can remove them completely.

Fields should be private

All your fields are public. Fields should be private. Technically you can make public fields, but that's considered bad design: It violates data hiding, one of the fundamental principles of object-oriented design. The consequence would be a breach of encapsulation and cohesion with too tight coupling to internals.

Prefer delegation over inheritance (and follow LSP)

It's often misunderstood that inheritance is at the core of OOP. It's not. What's at the core of OOP is polymorphic abstraction. Inheritance is the cream on top of it, and just as too much cream isn't healthy for the body, too much inheritance is not healthy for a design.

Using inheritance can also lead to a violation of the LSP - Liskov Substitution Principle. Subclasses are to be able to replace their superclasses in all cases. But your Clock class cannot be used or extended by yet another class to get an even more specialized version of a Window (or Frame or JFrame) in a meaningful way.

Another sign of wrong inheritance is that your class Clock does not add any behavior to its base class JFrame.

So, instead, you should not inherit from JFrame but just use a JFrame in your main method.

Also, pick the right superclass: Use JComponent instead of JPanel.

Your ScreenSaver is actually not a JPanel. A JPanel is a component designed to be a container with a layout for reuse. What you want as superclass is JComponent. You have multiple components, and that's possible with JComponent, but you don't want anyone to take your ScreenSaver and mess around with its LayoutManager and its contained components.

Think of data structures for replacing switch case - OCP

switch case you might ask? You have a long chain of if else which is just a switch case in disguise. In this case, it could easily be replaced by a Map<String, Color>. You could even use that map to initialize the JComboBox.

This is also related to a principle known as OCP - Open Closed Principle. It was first described by Bertrand Meyer and is one of the first five principles of object-oriented class design collected by Robert C. Martin. It says that, when a new feature needs to be implemented, a class (or actually any software entity on its level of abstraction) should be open for extension but closed for modification. It can also be seen as a special case of the SRP - Single Responsibility Principle: a class (or actually any software entity on its level of abstraction) should have only one reason to change. Which is interesting, as in a side track that defines responsibility as reason to change.

Now what is that theoratical babble all about? Imagine you want to add a new color: In your program you have to make a change in two (actually three) places:

The list of items given to the JComboBox,
and the color names actionPerformed() method that selects the color,
and the actual colors in the actionPerformed() method.

That can easily get out of sync. But even worse: There could also be more subtile errors, like in the JComboBox grey being written as "Grey", and in the actionPerformed() method grey being written as "Gray". Such errors can be very difficult to spot.

When you use a single central Map<String, Color>, then in your system there will be only one authority over the information of colors and their display names. Nothing can get out of sync. Much easier to maintain, much less error prone. You could even later easily load this stuff from a file then.

Use verbs for method names

Your method date is a noun. Okay, it can be argued that date actually works very very well as a verb, too. Just maybe not in this context, in this context date is thought of as a noun. So you could use updateDate instead.

Avoid mutable static fields

Mutable static fields are a dangerous source of error. They prevent multiple instances of the class to work properly independent of each other.

You might argue that in this case, there would never be two instances with different values for date and df. It would still be wrong, for two reasons:

You might want to run two instances in the same VM on different clocks (see below for Java 8 datetime API), for example to show different time zones, or just for testing. Then sharing the data between the instances would be wrong.
While the fields are global (once), there still are multiple Timers (one for each instance of Clock).

Reduce scope as much as possible - use local variables.

The variables panel, date and df do not need to be global, you can turn them into local variables of the respective methods.

Put the right amount of code inside the try-Block.

That you initialize font with null, then catch exceptions, which means font might still be null, and then continue using font afterwards, means that the remaining code eventually uses null. null should be avoided as much as possible, and those situations where null cannot be avoided, should be contained and controlled as much as possible.

Load and Initialize Only Once.

You do not need to load the font and register it with the graphics environment every time the component is repainted. It's sufficient to load and register the font once during application startup.

Similarly you do not need to create a new DateFormat every time the Timer calls your actionPerformed() method. It's sufficient to create only one global DateFormat object.

Collapse catch blocks

You can use Java 7 multicatch to collapse multiple catch blocks into one when the code executed for the catch blocks is identical.

try {
    // some code
} catch (Exception1 e) {
    e.printStackTrace();
} catch (Exception2 e) {
    e.printStackTrace();
}

can be replaced with

try {
    // some code
} catch (Exception1 | Exception2 e) {
    e.printStackTrace();
}

Make smaller methods

Your methods are quite big. Ideal methods do only one thing, they do it well and they do it only. For example, loading the font (including registering) is one thing, handling the exceptions in case the font couldn't be loaded, and when it was loaded successfully, setting it for the component, is another thing.

Make the change of color immediate

It feels a bit weird for a user to change the color and not seeing anything happening (until the second expires). For me, a second is a pretty long time. I can type 10 characters in a second, or shoot 3 people in Unreal Tournament when I'm lucky. You could change your program in a way that changes to the color become immediate.

Let Swing keep track of what it can

Swing can keep track of a lot of stuff. Explore the API. Your ScreenSaver is a JPanel. A JPanel is a JComponent. As such, it has font, foreground color and background color. You can just set the font and the color, and the Graphics object for the paintComponent() method will be initialized accordingly.

You might also want to consider simply involving a JLabel.

Add @Override annotation when implementing or overriding methods

This way the tools can help you when you misspell your method name and for example override paintcomponent(Graphics g) instead of paintComponent(Graphics g).

Consider import static

Often, it's plain obvious where static methods and constants are coming from. For example, in your context, it's plain obvious that RED is coming from class Color. In cases where it is plain obvious, import static can help making the source code more terse without losing clarity.

The Timer should only be running while the component is visible

So you should have a ComponentListener which starts the timer in componentShown() and stops the timer in componentHidden().

Refer to static symbols via the declaring class.

It's better to refer to symbols via the declaring class and not via any of the subclasses. For example, EXIT_ON_CLOSE is declared in interface WindowConstants. So it should be referred to as WindowConstants.EXIT_ON_CLOSE and not JFrame.EXIT_ON_CLOSE.

Prefer DISPOSE_ON_CLOSE over EXIT_ON_CLOSE.

It's better to use DISPOSE_ON_CLOSE then EXIT_ON_CLOSE. Yes, it is harder to program correctly with DISPOSE_ON_CLOSE, but that's exactly the intention: When your application doesn't properly stop and exit with DISPOSE_ON_CLOSE, there's a flaw (usually in the threading model). Using EXIT_ON_CLOSE instead of DISPOSE_ON_CLOSE just bypasses that flaw. That can even be error-prone if there is another thread in the application that's still busy doing important stuff (like saving a user's file). That thread would be terminated in the middle of its work.

Prefer main(String... args) over main(String[] args)

That's not really big and important. It's just easier for testing:

With main(String[] args) a no args test-call is main(new String[0]).
With main(String... args) a no args test-call is simply main().

Consider using final for variables as much as possible

It helps reducing programming mistakes and drives you towards the functional programming paradigm.

Split ScreenSaver in 2-3 classes

Actually ScreenSaver is doing more than one thing. It's displaying the date in a color of the user's liking, and it's enabling the user to pick the color of their liking. While initially it requires some thought of how to split this, in the end the code will become simpler if you achieve this.

Names: Is ScreenSaver really a screen saver? And is the Clock really a Clock?

Also, I would avoid using names that are in the API already, and there already is a class called Clock in the API. It isn't always possible (and sometimes not even intended) to avoid name-clashes, obviously. Just a thought.

Use Java 8 Date API

The classes Date, Calendar and such are actually horrible, and code that uses them is difficult to test.
You may consider using the new Java 8 API for datetimes, java.time.
It is really awesome.

Calling deriveFont() the right way

In case your font couldn't be loaded, the default font will have the default size instead of 30pt.
I think that's not intended.
It's better to call setFont(...) and then derive the font by setting it again using setFont(getFont().deriveFont(30f)).

Use the correct thread for the job

The setup of your application runs in the main thread. Ideally, modifications (and even reads) on Swing components happen only on the AWT Event Thread, unless the method that's called is explicitely documented as being thread-safe.

In your case it will probably not matter. But it's still safer and best practice to even create the UI not in the main thread but on the AWT Event Thread. The class SwingUtilities provides the methods invokeAndWait() and invokeLater() for that.

Your main() method could look like this:

public static void main(String... args) {
    SwingUtilities.invokeLater(() -> {
        JFrame frame = new JFrame();
        frame.setContentPane(new ScreenSaver());
        frame.setSize(685, 80);
        frame.setUndecorated(true);
        frame.setDefaultCloseOperation(DISPOSE_ON_CLOSE);
        frame.setBackground(Color.BLACK);
        frame.setResizable(false);
        frame.setVisible(true);
        frame.toFront();
    });
}

Or, putting the code for setting up the UI in a separate method, like this:

public static void main(String... args) {
    SwingUtilities.invokeLater(Clock::setupUI);
}

private static void setupUI() {
    JFrame frame = new JFrame();
    frame.setContentPane(new ScreenSaver());
    frame.setSize(685, 80);
    frame.setUndecorated(true);
    frame.setDefaultCloseOperation(DISPOSE_ON_CLOSE);
    frame.setBackground(Color.BLACK);
    frame.setResizable(false);
    frame.setVisible(true);
    frame.toFront();
}

Last but not least

Consider testing your application.
The most popular test frameworks (test runners) for Java probably are JUnit, Cucumber and TestNG.


Is this the only or best method for classes serializing in Java? I usually use the static methods from this designed class in order to perform objects serializing of custom classes.

Even though I'm getting good results, I wonder if this is the most appropriate way for it and if there are any improvements for it?

public class InputOuput {
    private static FileOutputStream         flujoSalida;
    private static ObjectOutputStream       flujoSalidaObjetos;
    private static File                     archivo;
    private static final FileChooser        seleccionador = new FileChooser();

    private static FileInputStream          flujoEntrada;
    private static ObjectInputStream        flujoEntradaObjetos;

    public static boolean serialize(Object anObject) {
        seleccionador.setTitle("Dónde guardar el estado del programa");

        try {
            archivo = seleccionador.showSaveDialog(null);
            if (archivo == null) return false;

            flujoSalida = new FileOutputStream(archivo);
            flujoSalidaObjetos = new ObjectOutputStream(flujoSalida);
            flujoSalidaObjetos.writeObject(anObject);
            flujoSalidaObjetos.close();
            flujoSalida.close();
        } catch (IOException error) {
            System.out.println(error.toString());
            System.out.println(error.getStackTrace());
        }
        return true;
    }

    public static Object deserialize() {
        Object m=null;

        try {
            archivo = seleccionador.showOpenDialog(null);
            if (archivo == null) return null;
            flujoEntrada = new FileInputStream(archivo);
            flujoEntradaObjetos = new ObjectInputStream(flujoEntrada);
            m= (Object) flujoEntradaObjetos.readObject();
            flujoEntradaObjetos.close();
            flujoEntrada.close();
        } catch (IOException | ClassNotFoundException error) {
            System.out.println(error.toString());
            System.out.println(error.getStackTrace());
        }
        return m;
    }
}

*I change Object for the custom intended class

One good point to ask: how could I be not making the InputOutput class to implement Serializable, yet it works properly anyway?

All of your variables should be local, except for maybe the FileChooser.

You swallow exceptions.  Printing a stack trace is no substitution for proper exception handling.  (By the way, logging should be done using java.util.logging.  If you must print a stack trace manually, print it to System.err instead of System.out.)  Serialization could completely fail, and the function will still return true.  The most appropriate behaviour should be to propagate the exception to the caller.

For greater flexibility and clarity with little additional effort, I suggest overloading serialize() and deserialize() with several variants which call each other:

public class InputOutput {
    private static final FileChooser seleccionador = new FileChooser();

    /**
     * Serializes an object to a file chosen by the user using a FileChooser.
     * @return true if completed, false if user cancels the FileChooser.
     */
    public static boolean serialize(Object o) throws IOException {
        seleccionador.setTitle("Dónde guardar el estado del programa");
        File f = seleccionador.showSaveDialog(null);
        if (f == null) return false;
        serialize(o, f);
        return true;
    }

    public static void serialize(Object o, File f) throws IOException {
        try (FileOutputStream fos = new FileOutputStream(f)) {
            serialize(o, fos);
        }
    }

    public static void serialize(Object o, FileOutputStream fos) throws IOException {
        try (ObjectOutputStream oos = new ObjectOutputStream(fos)) {
            oos.writeObject(o);
        }
    }

    /**
     * Deserializes an object from a file chosen by the user using a FileChooser.
     * @return the deserialized object, or null if user cancels the FileChooser.
     */
    public static Object deserialize() throws IOException { ... }
    public static Object deserialize(File f) throws IOException { ... }
    public static Object deserialize(FileInputStream fis) throws IOException { ... }
}


Determining maximum profit to be made from selling shares Question

  Your algorithms have become so good at predicting the market that you
  now know what the share price of Wooden Orange Toothpicks Inc. (WOT)
  will be for the next \$N\$ days.

  Each day, you can either buy one share of WOT, sell any number of
  shares of WOT that you own, or not make any transaction at all. What
  is the maximum profit you can obtain with an optimum trading strategy?

  Input

  The first line contains the number of test cases \$T\$. \$T\$ test cases
  follow:

  The first line of each test case contains a number \$N\$. The next line
  contains \$N\$ integers, denoting the predicted price of WOT shares for
  the next \$N\$ days.

  Output

  Output \$T\$ lines, containing the maximum profit which can be obtained
  for the corresponding test case.

  Constraints

  \$1 <= T <= 10\$
  \$1 <= N <= 50000\$

  All share prices are between 1 and 100000

Code

import java.io.*;
import java.util.*;
import java.text.*;
import java.math.*;
import java.util.regex.*;
class Solution{
public static void main(String[] args){
    Scanner stdin = new Scanner(System.in);
    int t = stdin.nextInt();
    for(int m=0;m<t;m++){
        long n = stdin.nextLong();
        long[] ar = new long[(int)n];
        for(int i=0;i<n;i++){
            ar[i] = stdin.nextLong();
        }
        long max = maximum(ar);
        System.out.println(max);
    }
}
public static long maximum(long[] ar){
    int j=0;
    long costPrice=0;
    long sellingPrice=0;
    int k=0;
    long max = maximumKey(j,ar);
    while(j<ar.length){
        if(ar[j]<max){
            costPrice += ar[j];
            j++;
        }
        else if(ar[j]==max){
            sellingPrice += ((j-k)*ar[j]);
            j++;
            k=j;
            max = maximumKey(j,ar);
        }
    }
    return sellingPrice-costPrice;
}
    public static long maximumKey(int j, long[] ar){
        long max = 0;
        for(int i=j;i<ar.length;i++){
            if(max<ar[i])
                max = ar[i];
        }
            return max;
    }
}

In the above code I didn't use DP. But this is supposed to be solved using DP with time complexity \$O(n)\$.

The trick to getting this problem in O(n) is some clever pre-processing.

Examples

Let's take 2 trials:

Trial 1: 1, 2, 3, 4, 5

Trail 2: 5, 4, 3, 2, 1

Obviously best case in Trial 1 is to buy for 4 days and sell on the 5th, for profit of 10.
In Trial 2, there is no profit to be gained, because the price never increases, because there is never a higher maximum down the line.

Trick

This points towards the trick: starting from the end make a note of the maximum encountered thus far.

For Trial 1, the maximums are 5, 5, 5, 5, 5

For Trial 2, the maximums are the same as the prices

Let's try this for a more complex scenario: 10, 1, 9, 2, 8, 3, 7

The corresponding maximums would be: 10, 9, 9, 8, 8, 7, 7

Then, for every day that price < maximum, you should buy. If price = maximum, you should sell, and if price > maximum, wait it out.

Code

This yields the following code, assuming you parse the prices into a non-empty long[] named prices:

long[] maximums = new long[prices.length];
maximums[prices.length - 1] = prices[prices.length - 1];
// fill maximums array, from the end
for (int i = prices.length - 2; i >= 0; i--) {
    if (prices[i] > maximums[i + 1])
        maximums[i] = prices[i];
    else
        maximums[i] = maximums[i + 1];
}

// Trade!
long profit = 0;
for (int i = 0; i < prices.length; i++) {
    if (prices[i] < maximums[i]) {
        // BUY now and SELL at max
        profit += maximums[i] - prices[i];
    }
}


Python simple Class with sqlite and unit testing I am trying to learn best practices for OOP and unit testing. I have implemented a simple User class here which checks if the user exists in the database. If yes, then it loads the user data into the instance. If not, then it should create a new entry in the database.

My questions:

How should I create unit tests for the methods? For example, for check_user_exists() should I mock for conn or cursor or exists or three of them? I tried to research for this but none of the tutorials for mocking in Python really gave me any true understanding of how good mocking is done.
In each of the methods, all the database connection related codes are repeated (i.e. conn.cursor(), conn.close()). Can these be refactored away? My guess would be to create a Database class, but I'm not sure about that.
Is it weird for User class to have a database_path attribute? So far this is the best that I have come up with. Please give any suggestions on how to structure a class where it has to always query databases.

user.py

import sqlite3

class User:
    def __init__(self, username, database_path):
        self.username = username
        self.database_path = database_path

        if not self.check_user_exists():
            self.create_user()
        else:
            self.get_user()

    def check_user_exists(self):
        conn = sqlite3.connect(self.database_path)
        cursor = conn.cursor()

        cursor.execute('SELECT rowid FROM user WHERE name = ?', (self.username,))
        exists = cursor.fetchone()

        conn.close()

        if exists is None:
            return False
        else:
            return True

    def create_user(self):
        conn = sqlite3.connect(self.database_path)
        cursor = conn.cursor()

        cursor.execute("INSERT INTO user (name) VALUES (?)", (self.username,))

        conn.commit()
        conn.close()

    def get_user(self):
        conn = sqlite3.connect(self.database_path)
        cursor = conn.cursor()

        cursor.execute("SELECT data FROM user WHERE name=?", (self.username,))
        self.data = cursor.fetchone()[0]

        conn.close()

This is an interesting question. Thanks for posting it!

IMO, you don't actually have a User class here. A User would be a system object that has a name, maybe an id, a password, some relationships to material, perhaps one or more privilege levels.

What you have here is an object that provides an interface to a database table - specifically, the user table. This is a Table Data Gateway in the parlance of Martin Fowler.

Let's write some sample code:

from private_data import database_path

username = 'yanxun'
user = User(username, database_path)

if user.check_user_exists():
    print("User {} is in the database".format(user.username))

That looks pretty clunky. If you rename your class to UserTable, it starts to make more sense:

from private_data import database_path

username = 'yanxun'
users = UserTable(database_path)

if username in users:      # implemented by .__contains__ method
    print("User {} is in the database.".format(username))
else:
    print("User {} not found. Adding to the database.".format(username))

    users.add(username)

Note that I am treating the table as a collection, specifically, like a set, and presuming you write method names that are inspired by existing Python collection names.

Now, with that out of the way, let me propose that you should be lazy in how you handle creating and deleting database connections. Specifically, make the caller do it!

Making the caller pass you in a connection is the best way to write code that you want to test, as well as the best way to write code that is maximally portable. If the caller mocks a database, you don't know and don't care. If the caller connects you to Oracle instead of sqlite, you don't know and don't care (provided your SQL syntax is valid).

def __init__(self, dbconn):
    self.dbconn = dbconn

Now you don't have to worry about database username/password, database path, or anything else. You can just request a cursor from the connection, and do your thing.

(Note: transaction support, and two-phase-commit support, are connection level operations, rather than cursor operations. That's why I suggest passing in a connection, rather than a cursor. See PEP-249 for details.


Wet Boxes problem: counting points on a triangular grid As a solution to the B. Wet Boxes problem :

  B. Wet Boxes

  Bob works in a warehouse which contains a large pile of boxes. The
  position of a box can be described with a pair of integers (x, y).
  Each box either stands on the ground (y = 0) or stands on top of two
  boxes with positions (x, y - 1) and (x + 1, y - 1) (see the figure).

  Sometimes the contents of a box leak out and the box gets wet. When a
  box becomes wet, so do the two boxes below it. Given a list of boxes
  that leak in succession, help Bob count how many dry boxes became wet
  after each leak. Don't include boxes that were already wet.

My solution is :

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

static int count;

typedef struct {
    int x;
    int y;
}Box;

Box* push(Box* memptr, int x, int y){
    int i;
    for (i = 0; i < count; i++){
        if ((memptr[i].x == x) && (memptr[i].y == y))
            return memptr;
    }
    count++;
    if (count == 1)
        memptr = (Box*)malloc(sizeof(Box));
    else
        memptr = (Box*)realloc(memptr, sizeof(Box) * count);
    memptr[count - 1].x = x;
    memptr[count - 1].y = y;

    return memptr;
}

Box* find_wet_boxes(Box* memptr, int x, int y){
    if ((x * y) < 0)
        return memptr;
    else {
        memptr = push(memptr, x, y);
        find_wet_boxes(memptr, x, y - 1);
        return find_wet_boxes(memptr, x + 1, y - 1);
    }
}

int main(){
    Box* memptr = NULL;
    // int i;
    memptr = find_wet_boxes(memptr, 1, 3);
    // memptr = find_wet_boxes(memptr, 3, 2);
    // memptr = find_wet_boxes(memptr, 0, 6);
    // memptr = find_wet_boxes(memptr, 1, 1);
    printf("count = %d\n", count);
    return 0;
}

This works fine for each individual box co-ordinates but when I try to run for all the 4 co-ordinates, it gives me time limit exceed error during submission. (Coordinates may be as large as 109.  There may be up to 105 leaking boxes.  Time limit is 0.7 seconds.) Clearly my algorithm is not good enough. Can someone please give me a better solution for this?

If it the answer is too broad and there could be many solution, then I expect at least one of them which will at least pass the runtime limit test

Possible Solution:

  So, a single leaked box covers a smaller triangle of boxes. In short,
  we maintain a set that contains the vertices of such triangles that
  have not been covered by any other triangle yet. When a box leaks
  (i.e., a new triangle is added), we find the vertices of the triangles
  that are inside the new one, and subtract the area that was first
  covered by this box and is inside the new triangle. Then we can remove
  them from the set and insert the new triangle vertex in the set. This
  results in \$O(n\ \log n)\$ solution (for each new triangle, there can be
  also two vertices that are not inside the new triangle, but overlap
  with it: these vertices remain in the set, but that does not change
  the complexity).

This is the possible solution as quoted by someone but I don't understand how to implement it or will that actually solve the problem.

It's been a while since I have worked in C, so a language-specific review would not be so accurate. The only thing I can say in that aspect is that you should put curly bracket even in single-command body ifs as the code gets more readable and less error prone.

Taking a look at the problem and at your solution I think that you're overcomplicating it. You are allocating memory for each box and working with it when you actually don't need them. All you need is a square matrix - to be more precise, a lower triangular matrix - of ints (in this case).

The idea is the following:

Represent the boxes' status via the matrix \$A\$. If the box in the position \$(x, y)\$ is wet then \$A_{x,y} = 1\$, otherwise \$A_{x,y} = 0\$. So, initially, every item in the matrix is \$0\$.
When the box in position \$(x, y)\$ leaks you just set all the appropriate cells of \$A\$ to \$1\$ and count the number of cells whose value has changed from \$0\$ to \$1\$.
Return the value of the counter for each case.

In pseudo-code (I'm not so practical in C), it becomes something like the following:

int[,] A = new int[7,7];

void initialize_matrix(){
    for(int i = 0; i < 7, i++){
        for(int j = 0; j < 7; j++){
            A[i, j] = 0;
        }
    }
}

int count_wet_boxes(int x, int y){
    int counter = 0;

    for(int i = x; i >= 0; i--){
        for(int j = 0; (i + j) <= (x + y); j++){
            if(A[i, j] == 0){
                A[i, j] = 1;
                counter++;
            }
        }
    }

    return counter;
}

int main(){
    initialize_matrix();

    // the rest of the logic goes here
}

You can also try to ask in the Mathematics SE site if there is a formula you can use.

Let me know if anything's unclear.