Path: csiph.com!usenet.pasdenom.info!news.redatomik.org!newsfeed.xs4all.nl!newsfeed1a.news.xs4all.nl!xs4all!post.news.xs4all.nl!not-for-mail
To: python-list@python.org
From: Peter Otten <__peter__@web.de>
Subject: Re: Step further with filebasedMessages
Date: Tue, 05 May 2015 13:08:16 +0200
Organization: None
References: <87oalzh0d5.fsf@Equus.decebal.nl>
Mime-Version: 1.0
Content-Type: text/plain; charset="ISO-8859-1"
Content-Transfer-Encoding: 7Bit
User-Agent: KNode/4.13.3
Precedence: list
Newsgroups: comp.lang.python
Message-ID: <mailman.123.1430824107.12865.python-list@python.org>
Lines: 122
NNTP-Posting-Host: 2001:888:2000:d::a6
Xref: csiph.com comp.lang.python:89960

Cecil Westerhof wrote:

> I now defined get_message_slice:
>     ### Add step
>     def get_message_slice(message_filename, start, end):

Intervals are usually half-open in Python. I recommend that you follow that 
convention.

>         """
>         Get a slice of messages, where 0 is the first message
>         Works with negative indexes
>         The values can be ascending and descending
>         """
> 
>         message_list    = []
>         real_file       = expanduser(message_filename)
>         nr_of_messages  = get_nr_of_messages(real_file)
>         if start < 0:
>             start += nr_of_messages
>         if end < 0:
>             end += nr_of_messages
>         assert (start >= 0) and (start < nr_of_messages)

You should raise an exception. While asserts are rarely switched off in 
Python you still have to be prepeared, and an IndexError would be a better 
fit anyway.

>         assert (end   >= 0) and (end   < nr_of_messages)
>         if start > end:
>             tmp             = start
>             start           = end
>             end             = tmp
>             need_reverse    = True
>         else:
>             need_reverse    = False
>         with open(real_file, 'r') as f:
>             for message in islice(f, start, end + 1):
>                 message_list.append(message.rstrip())
>         if need_reverse:
>             message_list.reverse()
>         return message_list
> 
> Is that a good way?
> 
> I also had:
>     def get_indexed_message(message_filename, index):
>         """
>         Get index message from a file, where 0 gets the first message
>         A negative index gets messages indexed from the end of the file
>         Use get_nr_of_messages to get the number of messages in the file
>         """
> 
>         real_file       = expanduser(message_filename)
>         nr_of_messages  = get_nr_of_messages(real_file)
>         if index < 0:
>             index += nr_of_messages
>         assert (index >= 0) and (index < nr_of_messages)
>         with open(real_file, 'r') as f:
>             [line] = islice(f, index, index + 1)
>             return line.rstrip()
> 
> But changed it to:
>     def get_indexed_message(message_filename, index):
>         """
>         Get index message from a file, where 0 gets the first message
>         A negative index gets messages indexed from the end of the file
>         Use get_nr_of_messages to get the number of messages in the file
>         """
> 
>         return get_message_slice(message_filename, index, index)[0]
> 
> Is that acceptable? 

Yes.

> I am a proponent of DRY.

But note that you are implementing parts of the slicing logic that Python's 
sequence already has. Consider becoming a pronent of DRWTOGAD*.

> Or should I at least keep the assert in it?
 
No.

I see you have a tendency to overengineer. Here's
how I would approach case (1) in Chris' answer, where memory is not a 
concern:

import os

def read_messages(filename):
    with open(os.path.expanduser(filename)) as f:
        return [line.rstrip() for line in f]

# get_messages_slice(filename, start, end)
print(read_messages(filename)[start:end+1])

# get_indexed_message(filename, index)
print(read_messages(filename)[index])

Should you later decide that a database is a better fit you can change 
read_messages() to return a class that transparently accesses that database. 
Again, most of the work is already done:

class Messages(collections.Sequence):
    def __init__(self, filename):
        self.filename = filename)
    def __getitem__(self, index):
        # read record(s) from db
    def __len__(self): 
        # return num-records in db

def read_messages(filename):
    return Messages(filename)

By the way, where do you plan to use your functions? And where do the 
indices you feed them come from?

(*) Don't repeat what those other guys already did. Yeah sorry, I have a 
soft spot for lame jokes...